Skip to content

fix(teach-deploy): keep --dry-run read-only in CI mode; merge with --no-ff - #505

Merged
Data-Wise merged 4 commits into
devfrom
feature/deploy-ci-dryrun
Aug 24, 2026
Merged

Data-Wise merged 4 commits into
devfrom
feature/deploy-ci-dryrun

Conversation

@Data-Wise

Copy link
Copy Markdown
Owner

Two fixes to teach deploy, both found by trying to drive it from a non-interactive caller.

1. --dry-run was neither read-only nor usable in CI mode

ci_mode auto-detects whenever stdin is not a TTY (teach-deploy-enhanced.zsh:530), so every
automation or agent caller gets it. In that mode two preflight conditions aborted the run before
the plan could render
:

  • "production has new commits" (:170-186)
  • "uncommitted changes" (:668)

Neither can affect a dry run, which mutates nothing. Both teaching repos I tested hit the first
one, so a dry run could not render a plan on the ordinary state of a working course repo.

The uncommitted-changes handler had a worse edge. Interactively it prompts to commit, and any
pty wrapper answers the default Y. Probing whether script -q /dev/null could defeat the CI-mode
detection, it did exactly that and created a real commit in a live course repo during what was
announced as a read-only dry run. That commit was reverted and the repo restored, but the hazard is
in the handler, not in the wrapper — which is why the fix is here rather than a note telling people
not to use a pty.

Both mutation-oriented gates now skip when dry_run is true. Real deploys are untouched and still
abort on both conditions.

2. The direct merge omitted --no-ff

A deploy of N draft commits fast-forwarded production to draft's own tip, leaving no single commit
representing the deploy. Since teach deploy --rollback reverts the deploy's commit, a
fast-forwarded deploy could only be rolled back one commit at a time — it lost rollback granularity
silently rather than failing. git-helpers.zsh:693 already assumed otherwise, filtering "merge
commits from --no-ff deploys" when detecting production conflicts.

Verified in a sandboxed fixture deploying two commits:

before:  linear history, production tip == draft's own content commit
after:   70c864d parents=[d7a7215 6f52c5f]   (a merge commit)

Test evidence

All 8 deploy suites: 246 passed, 1 failed, 1 skipped.

dogfood-teach-deploy-v2.zsh              71/71 passed
e2e-teach-deploy-v2.zsh                  44 passed, 0 failed, 1 skipped
test-teach-deploy-dryrun-readonly.zsh    10 passed, 0 failed   (new)
test-teach-deploy-merge-topology.zsh      2 passed, 0 failed   (new)
test-teach-deploy-unit.zsh               25 passed, 1 failed   <- see below
test-teach-deploy-v2-integration.zsh     22 passed, 0 failed
test-teach-deploy-v2-unit.zsh            58 passed, 0 failed
test-teach-deploy.zsh                    14 passed, 0 failed

The single failure is Test 24: Calculate commit count between branches, and it reproduces
identically on unmodified dev
— pre-existing, not introduced here.

Both new suites check repository state before and after every case, so any write during a
"read-only" path is caught rather than assumed.

Positive controls, run separately for each fix:

  • Reverting only the dry-run change fails exactly the 3 dry-run assertions while all 4
    real-deploy controls still pass — so the controls are not tautological.
  • Reverting only the --no-ff fails both topology assertions; restoring it passes both.

One trap documented in the test header

After a successful deploy, draft and production legitimately point at the same commit, because
the deploy syncs draft from production afterwards (draft@{0}: merge origin/production: Fast-forward). Ref equality is therefore not evidence of a fast-forwarded deploy — parent
count is. An earlier check of mine got this wrong and reported a fast-forward that had not
happened.

Changelog

Both CHANGELOG.md and docs/CHANGELOG.md updated under [Unreleased]. Note the two files
already differ by roughly 20KB of pre-existing drift; these entries are mirrored into both rather
than reconciling that separately.

Context

Found while making cc-config's /deploy delegate here instead of reimplementing the
draft-to-production workflow itself (cc-config#7). That delegation stays unshipped until this
lands and a release reaches Homebrew, since teach resolves to the brew build.

Test User added 4 commits August 23, 2026 19:47
ci_mode auto-detects whenever stdin is not a TTY, so every automation or
agent caller gets it. In that mode two preflight conditions were fatal
even under --dry-run:

  - "production has new commits" aborted before the plan could render
  - "uncommitted changes" aborted before the plan could render

Neither can affect a dry run, which mutates nothing. Both teaching repos
tested hit the first one, so a dry run could not render a plan on the
normal state of a working course repo.

The uncommitted-changes handler had a second, worse edge: interactively
it PROMPTS to commit. A pty wrapper answers the default Y and silently
creates a real commit. Observed once against a live course repo.

Fix: skip both mutation-oriented gates when dry_run is true. Real deploys
are untouched - they still abort on both conditions, which the two
controls in the new suite assert.

tests/test-teach-deploy-dryrun-readonly.zsh: 10 assertions over sandboxed
fixtures, each checking repository state before and after so any write is
caught. 10 passed, 0 failed.
Added to [Unreleased] in both the root and docs/ changelogs. Note the two
files already differ by ~20KB of pre-existing drift; this entry is mirrored
into both rather than reconciling that separately.
A deploy of N draft commits used to fast-forward production to draft's own
tip, leaving no single commit representing the deploy. Since
teach deploy --rollback reverts the deploy's commit, a fast-forwarded
deploy could only be rolled back one commit at a time - it silently lost
rollback granularity rather than failing.

The codebase already assumed merge commits exist: git-helpers.zsh:693
filters "merge commits from --no-ff deploys" when detecting production
conflicts.

Verified in a sandboxed fixture. Before: linear history, production tip
was draft's own content commit. After: production tip is a merge commit
with parents [base, draft-tip].

tests/test-teach-deploy-merge-topology.zsh asserts both properties.
Positive control: reverting only the --no-ff fails both assertions, and
restoring it passes both.

One trap documented in the test header - after a successful deploy, draft
and production legitimately point at the SAME commit, because the deploy
syncs draft from production afterwards. Ref equality is therefore not
evidence of a fast-forwarded deploy; parent count is. An earlier check of
mine got this wrong.

All 8 deploy suites: 246 passed, 1 failed, 1 skipped. The failure is
Test 24 in test-teach-deploy-unit.zsh, which reproduces identically on
unmodified dev.
Self-review of this PR: skipping the uncommitted-changes handler on a dry
run has a side effect the first commit did not account for. The handler
previously committed dirty work before the plan rendered, so the
"Would deploy N files" list included it. Skipping the handler is the right
call - a dry run should not assume you would commit - but it means the
list is committed work only, and can silently understate what a real
deploy would ship.

The report now names the count of excluded uncommitted files and says a
real deploy would offer to commit them first. The preflight already showed
"[--] Working tree has uncommitted changes"; this connects that line to the
file count below it.

Covered by a new assertion in test-teach-deploy-dryrun-readonly.zsh
(11 passed). All 8 deploy suites re-run: 247 passed, 1 failed, 1 skipped -
the failure is the same pre-existing Test 24 that reproduces on dev.
@Data-Wise
Data-Wise merged commit 90e17b4 into dev Aug 24, 2026
2 checks passed
Data-Wise added a commit that referenced this pull request Aug 24, 2026
…gating) (#506)

* ci: close four coverage gaps found in a gap analysis

1. TWO SUITES MERGED WITHOUT CI EVER RUNNING THEM (my regression, from #505).
   run-all.sh takes an explicit run_test list, not a glob, so
   test-teach-deploy-dryrun-readonly and test-teach-deploy-merge-topology
   went in green while CI never executed their 12 assertions. Both wired;
   both run in under 2s, well inside the 30s default.

2. 74 SUITES THAT HAD NEVER RUN ON A RUNNER. Audited all 118 test-*.zsh
   files unreferenced by run-all.sh: 74 pass, 37 fail, 6 exceed 20s, 1 is
   TTY-only. The 74 are now in tests/run-extended.sh behind a NON-BLOCKING
   soak job. Deliberately not promoted straight into the required check -
   "passes on a Mac" is not evidence about ubuntu-latest, and one
   environment-dependent suite would break the gate for everyone. This
   mirrors how full-suite itself was introduced, per test.yml's own comment.
   Verified locally: 74 passed, 0 failed.

   The 37 broken ones are left out and are NOT silently dropped - the count
   is recorded in run-extended.sh's header. They fail in the main checkout
   too, so this is rot, not worktree sensitivity.

3. CHANGELOG PARITY, GATED WHERE IT CAN ACTUALLY BE GATED. CHANGELOG.md and
   docs/CHANGELOG.md are meant to mirror and nothing checked it. They have
   drifted BIDIRECTIONALLY: 23 sections only in root, 41 only in docs, and
   of 33 shared versions only 12 have identical bodies. Neither file is a
   superset, so this cannot be fixed by copying one over the other - 21
   shared versions need a human to pick the correct body.

   Reconciling that is explicitly NOT attempted here; picking a winner 21
   times would destroy release history. Instead test-changelog-parity.zsh
   gates the [Unreleased] section, which is identical today, so new entries
   can no longer land in one file only. Historical drift is reported as
   advisory. Positive control: planting an entry in one file's [Unreleased]
   fails the test and names the offending line.

4. DOCS QUALITY WAS UNGATED. mkdocs.yml had no strict setting while docs.yml
   runs `mkdocs gh-deploy --force` on push, so broken links deployed
   silently. strict: true enabled, verified safe first (current tree builds
   with 0 warnings). scripts/lint-docs.sh existed but was wired into nothing
   - now runs in CI as ADVISORY, because 394 errors would fail every run and
   138 of those are MD040, which is real work rather than a config toggle.

   Also fixes a real bug: .markdownlintignore is a markdownlint-cli **v1**
   file and this script runs cli2, which never reads it - so the repo's
   intent to skip .archive/ was inert and 49 errors came from archived docs.
   Excluded via a negated glob cli2 honours. 443 -> 394.

NOT DONE, deliberately: no `lint-docs.sh --fix`. It resolves 302 of 443 but
renumbers ordered lists (MD029) and rewrote `#| label:` to `# | label:` in
docs/plans/2026-01-31-teach-validate-lint.md, breaking a Quarto chunk
directive. ~20 of 48 touched files had genuine content edits including a
security spec whose items 3-9 were renumbered to 1-7. Measured, reverted,
and documented in the CI step so the next person does not repeat it.

Full blocking suite locally: 2 failures (e2e-em-dispatcher,
test-atlas-contract), both reproducing on unmodified dev and both green in
CI - local environment, not this branch.

* ci: narrow the soak list to the 60 that pass on the runner

First CI run of the soak job did its job: 60 of the 74 passed on
ubuntu-latest, 14 failed. They need a tool or service a fresh runner does
not have - brew, atlas, the macOS keychain, a real mail account - which is
exactly the 'passes on a Mac is not evidence about CI' case the job exists
to surface.

The 14 are excluded and named in the file with the reason, not silently
dropped. A soak job that is permanently red gets ignored, and then it
protects nothing; green means a later failure is real signal.

The right fix for each is to exit 77 (this repo's clean-skip code) when its
dependency is absent, at which point it can rejoin the list. Until then they
sit where they already were - outside CI.

Local: 60 passed, 0 failed.

---------

Co-authored-by: Test User <test@example.com>
Data-Wise added a commit that referenced this pull request Sep 5, 2026
.STATUS had not been touched since 2026-07-07 (f740eb5): the header still
read Phase: Released (v7.16.0) while v7.17.0, v7.17.1 and v7.17.2 had all
shipped, and Focus: held the literal string "--help" -- a bad write, not a
focus.

- Header: Phase -> Released (v7.17.2); Focus -> a real one-line focus.
- Added four session entries reconstructed from CHANGELOG.md, the merged PR
  bodies (#505, #506, #509, #513, #499, #514, #515) and the release tags:
  v7.17.0 (teach deploy safety + CI coverage), v7.17.1 (alias-proof
  pipelines), v7.17.2 (CI hardening + PATH fix), plus the 2026-07-08..07-19
  housekeeping commits and a session entry for this resync itself.
- Demoted the stale "Current Session (2026-07-07)" and two older headings
  that had been left as "Current Session" against the file's own convention.
- Test baseline refreshed from evidence rather than carried forward:
  84 passed / 0 failed / 0 timeout / 1 skipped across 85 registered suites,
  per CI run 33946019980 on dev.
- Next Action re-verified against repo state: added stale-issue triage
  (#487/#488/#489), the PR #500 rebase decision, .gitignore for
  .token-optimizer, and the advisory markdown-lint backlog.

Verified: scripts/check-status.zsh clean on the updated file (validator
confirmed non-vacuous via a planted "Progress: banana" defect, which it
caught); _flow_status_field reads all four header fields correctly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Data-Wise
Data-Wise deleted the feature/deploy-ci-dryrun branch September 6, 2026 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant