feat(ship): add deterministic dry-gates rehearsal (sc-2346) - #523
Merged
Conversation
## Overview Adds `devkit ship ... --dry-gates`, a no-commit rehearsal that uses ship's fetched base, explicit path staging, projected runtime/config/coverage, hook capture, timeout, and cleanup. It runs configured deterministic/structure/extra gates plus the changed-comment firewall, while skipping commit, push, PR creation, and the interactive/review gate families. Fixes Story #2346 and autonomous report `19bb0dca-531d-4441-b0ef-d6130e1706ff`. ## Direct relation to the report The report identified that the only deterministic entry point was the internal `guard-deterministic`, leaving callers to reconstruct ship's temporary index, hook arguments, runtime projection, and cleanup. This flag exposes that existing ship seam with the same base and path inputs; it does not add a second gate engine. ## Red without the fix Before implementation: ```text bun run test:run -- cli/__tests__/ship-dry-gates.test.mts 4/4 failed; the primary failure was: unknown flag: --dry-gates (pass a dash-leading file path after --) ``` The companion hook-routing regressions also failed before the implementation: dry-gates was not a recognized hook mode, and a coverage-only gate configuration emitted no deterministic orchestrator. These tests directly exercise the ticket: the happy path proves the fetched base and explicit staged paths are used without leaving a branch or commit; the blocking path proves the gate status is returned and cleanup still occurs; hook tests prove the intended deterministic/comment-only gate selection and coverage projection. ## Green with the fix - Broad suite before the reviewer-driven cleanup refinements: 281 files passed, 5,252 tests passed; 1 file and 13 tests skipped - Focused dry-gates regression: 4 passed, including locked-worktree cleanup and isolated proof logs - Focused ship/hook/help slice: 120 passed, including inherited dry-gates `0`/`1` isolation - Typecheck, lint, structure lint, shell syntax, dist integrity, semantic duplication, and clone gate: passed
Contributor
|
Warning Review limit reachedNext included review available in 14 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
norvalbv
added a commit
that referenced
this pull request
Sep 5, 2026
… verb fail typecheck (sc-2361) Closes sc-2361. `devkit review` runs the configured pre-commit chain — deterministic gates, decisions gates and the AI reviewer fleet — against the working tree with no branch, commit, push or PR. Nothing in `skills/` named it. `grep -rn "devkit review" skills/` returned zero hits, and zero across `templates/` and `agents/` too; no commit on `main` ever added them. An agent only learns a verb exists by reading `skills/`. So the lane was unreachable in practice, and agents discovered reviewer findings one full ship cycle at a time — the reporting run spent 11 `devkit ship` attempts on a single 10-file PR, 8 of them blocked by the AI stage, each paying the full reviewer fan-out plus a push-then-delete on origin. **Routing.** `skills/using-devkit/SKILL.md` gains a `devkit review` row beside the existing `--dry-gates` row, a Rules bullet for the three-lane division, and one frontmatter clause. The clause matters: the description is the skill's only firing surface and enumerated git/maintenance triggers only, so without it a row about pre-ship review would never be read. **A mechanism, so this cannot recur.** `CommandMeta.agentFacing` is now required. `COMMANDS` in `cli/index.mts` is annotated, so a new command that never classifies itself fails `typecheck` at its own registration line — classifying is part of adding a command, not a follow-up. `cli/__tests__/command-skill-routing.test.mts` then enforces what the type cannot: that `true` was followed through into `skills/`, and that `false` carries a `notRoutedBecause` a human can check. **The audit that forced.** `review` was not the only one. Three further agent-facing verbs were unrouted and are now routed — `base-status` (using-devkit), `baseline-status` and `prove-regression` (`skills/testing`, phrased runner-agnostically). Eight verbs are classified internal with written justifications: hook shims, wired package scripts, maintainer-only, and `upgrade` slices. Three corrections to the story's premise, each verified in source, because a row that overpromises is worse than no row: - **Review does not run the completeness judge.** `.husky/pre-commit:198` gates it on `DEVKIT_RUN_MODE != "review"`, and the comment at :190 says why — completeness is a commit gate, not part of a range review. Sentry is excluded too. The row says so, because a green review followed by a completeness block would otherwise read as the tooling contradicting itself. Two of the reporting run's eight blocks were completeness, so this lane would not have caught them. - **Review does not warm ship's cache.** Its persistent store is checkout-external (`cli/lib/ship/review/cache/root.mts`); ship reads the repo's `.devkit/review-cache.json`. The row states that ship re-pays every judge. - **Review is early signal, never a ship guarantee.** It covers `merge-base..HEAD` plus the whole dirty snapshot where ship covers only briefed paths. This restates the sc-2346 rejection in `ship-gates-converge-not-restart.md:44` as guidance rather than re-litigating it. Two lanes are deliberately **not** named. Ship's gate-failure footer is untouched: `ship-gates-converge-not-restart` names `commit-with-gate-capture.sh` literally in its Scope, and review earns a blocked `--resume` nothing. And on a non-protected branch a plain `git commit` already runs the same fleet on the same cache salt family, so the Rules bullet sends agents there first and scopes review to the protected-branch / cannot-commit case. - The gate-failure footer AC — see above. - Re-scoping sc-2346: already resolved. `--dry-gates` shipped as `77308735` / #523 and is routed at `SKILL.md:40`. - The proposed `--deterministic-only` flag on `devkit review` — explicitly rejected in the decision log; `--dry-gates` covers the fast inner loop. 49 tests in the new suite. A full CLI-suite run passed 2909/2909 on an earlier base; on this base the 8 suites covering the touched surface pass 186/186, and CI runs the rest. Every guard was verified to fail when its target is broken, then reverted. The instructive one: replacing the routing matcher with the naive `\b<verb>\b` form the design rejected left **all 14 gate assertions green** while the gate became meaningless. A healthy tree never exercises this gate's own logic — all 22 verbs either match or are exempt — so the suite unit-tests the predicate directly: 16 boundary cases pinning that a backticked bare `` `guard-branch` `` is not routing (the reason it is classified internal), that `devkit review-all` does not satisfy `review`, and that a line-wrapped `devkit\nreview` does not count. `diffRegistry` gets its own units because a rename moves both sides together, so the real tree cannot produce a divergence to detect. The verb-collision guard is separate for the same reason: two modules claiming one `meta.name` would otherwise have a classification silently dropped with the dispatch check none the wiser. - A decision note is recorded under the existing `review-gate-in-chain` axis, not a new one. - `.claude/`, `.cursor/` and `.devkit/skills-manifest.json` are `devkit sync-skills` output and are included so `doctor` stays clean. - The qavis advisory did not block (no UI-affecting change), so nothing was bypassed. The gate log carries no SKIP or BYPASSED line: all 11 reviewers, the decisions gates and the deterministic chain passed on their own. **Unrelated-looking line in the diff, explained:** `cli/commands/init.mts` sat exactly at its shrink-only ceiling, so adding one `agentFacing` field tripped `guard-size`. Rather than override it, the redundant one-line `structureAvailableFor` wrapper is inlined — the file already bypassed it at one of four call sites, so all four now read identically. On this base the file is 1139, inside its cap. **Allowlist entry, explained:** `guard-dup` blocked on `reconcile` (`cli/commands/reconcile.mts`) vs `reconcilePath` (`cli/lib/reconcile.mts`) at c=0.8342. I read both ranges: the first is the CLI entrypoint (flags, git-top-level assert, manifest load, render), the second decides restore/delete/warn for one path from upstream/worktree/index blobs. No shared logic — the matcher scored shared reconcile-domain vocabulary and a common guarded-early-return shape. It is a pre-existing latent pair that surfaced only because the one-line `agentFacing` field put the commands file into the staged set, so it is approved via the gate's own documented path with that reasoning recorded, not overridden. **Two bugs the local gate chain caught in this PR's own test.** Committing on a feature branch runs the same reviewer fleet this PR documents, and correctness-reviewer returned two findings against `command-skill-routing.test.mts`. Both were real and both are fixed here: - `.` is not a word character, so the original right boundary `(?![\w-])` let `See devkit review.md for details` satisfy the gate for `review` — a filename routing nobody. The boundary is now `(?![\w-]|\.\w)`, which still accepts a verb ending a sentence. - The registry cross-check compared `devkit --help` against `meta.name`, which are two views of one value. Rename `meta.name` to `inspect` while `COMMANDS` still keys the module under `review` and both views agree, leaving `devkit inspect` unreachable and `devkit review` unclaimed with nothing to compare. It now parses the literal `COMMANDS` keys from `cli/index.mts` — the only witness independent of `meta` — and checks the help surface separately. That rename now reports `unreachable: ['inspect'], unclaimed: ['review']`; before, it produced no dispatch failure at all. Both have regression cases. **On `dist/`.** It is tracked and expected to match source per-commit, so the rebuilt output is included — without it a consumer would install the version *without* the routing, which is exactly the failure this story is about. Four other `dist/` files are pre-existing drift, where commits on `main` changed their source without rebuilding; they are deliberately excluded, since clearing another change's debt here would only obscure this diff. **Supersedes #584**, which was opened before eight commits landed on `main` and could no longer be updated in place: GitHub could not auto-sync its branch (real conflicts), and `devkit ship --pr` requires the PR's `baseRefOid` to equal current `origin/main`, which only a branch sync advances. This PR is the same work reconciled onto current `main`, plus the two fixes above.
norvalbv
added a commit
that referenced
this pull request
Sep 5, 2026
… verb fail typecheck (sc-2361) Closes sc-2361. `devkit review` runs the configured pre-commit chain — deterministic gates, decisions gates and the AI reviewer fleet — against the working tree with no branch, commit, push or PR. Nothing in `skills/` named it. `grep -rn "devkit review" skills/` returned zero hits, and zero across `templates/` and `agents/` too; no commit on `main` ever added them. An agent only learns a verb exists by reading `skills/`. So the lane was unreachable in practice, and agents discovered reviewer findings one full ship cycle at a time — the reporting run spent 11 `devkit ship` attempts on a single 10-file PR, 8 of them blocked by the AI stage, each paying the full reviewer fan-out plus a push-then-delete on origin. **Routing.** `skills/using-devkit/SKILL.md` gains a `devkit review` row beside the existing `--dry-gates` row, a Rules bullet for the three-lane division, and one frontmatter clause. The clause matters: the description is the skill's only firing surface and enumerated git/maintenance triggers only, so without it a row about pre-ship review would never be read. **A mechanism, so this cannot recur.** `CommandMeta.agentFacing` is now required. `COMMANDS` in `cli/index.mts` is annotated, so a new command that never classifies itself fails `typecheck` at its own registration line — classifying is part of adding a command, not a follow-up. `cli/__tests__/command-skill-routing.test.mts` then enforces what the type cannot: that `true` was followed through into `skills/`, and that `false` carries a `notRoutedBecause` a human can check. **The audit that forced.** `review` was not the only one. Three further agent-facing verbs were unrouted and are now routed — `base-status` (using-devkit), `baseline-status` and `prove-regression` (`skills/testing`, phrased runner-agnostically). Eight verbs are classified internal with written justifications: hook shims, wired package scripts, maintainer-only, and `upgrade` slices. Three corrections to the story's premise, each verified in source, because a row that overpromises is worse than no row: - **Review does not run the completeness judge.** `.husky/pre-commit:198` gates it on `DEVKIT_RUN_MODE != "review"`, and the comment at :190 says why — completeness is a commit gate, not part of a range review. Sentry is excluded too. The row says so, because a green review followed by a completeness block would otherwise read as the tooling contradicting itself. Two of the reporting run's eight blocks were completeness, so this lane would not have caught them. - **Review does not warm ship's cache.** Its persistent store is checkout-external (`cli/lib/ship/review/cache/root.mts`); ship reads the repo's `.devkit/review-cache.json`. The row states that ship re-pays every judge. - **Review is early signal, never a ship guarantee.** It covers `merge-base..HEAD` plus the whole dirty snapshot where ship covers only briefed paths. This restates the sc-2346 rejection in `ship-gates-converge-not-restart.md:44` as guidance rather than re-litigating it. Two lanes are deliberately **not** named. Ship's gate-failure footer is untouched: `ship-gates-converge-not-restart` names `commit-with-gate-capture.sh` literally in its Scope, and review earns a blocked `--resume` nothing. And on a non-protected branch a plain `git commit` already runs the same fleet on the same cache salt family, so the Rules bullet sends agents there first and scopes review to the protected-branch / cannot-commit case. - The gate-failure footer AC — see above. - Re-scoping sc-2346: already resolved. `--dry-gates` shipped as `77308735` / #523 and is routed at `SKILL.md:40`. - The proposed `--deterministic-only` flag on `devkit review` — explicitly rejected in the decision log; `--dry-gates` covers the fast inner loop. 49 tests in the new suite. A full CLI-suite run passed 2909/2909 on an earlier base; on this base the 8 suites covering the touched surface pass 186/186, and CI runs the rest. Every guard was verified to fail when its target is broken, then reverted. The instructive one: replacing the routing matcher with the naive `\b<verb>\b` form the design rejected left **all 14 gate assertions green** while the gate became meaningless. A healthy tree never exercises this gate's own logic — all 22 verbs either match or are exempt — so the suite unit-tests the predicate directly: 16 boundary cases pinning that a backticked bare `` `guard-branch` `` is not routing (the reason it is classified internal), that `devkit review-all` does not satisfy `review`, and that a line-wrapped `devkit\nreview` does not count. `diffRegistry` gets its own units because a rename moves both sides together, so the real tree cannot produce a divergence to detect. The verb-collision guard is separate for the same reason: two modules claiming one `meta.name` would otherwise have a classification silently dropped with the dispatch check none the wiser. - A decision note is recorded under the existing `review-gate-in-chain` axis, not a new one. - `.claude/`, `.cursor/` and `.devkit/skills-manifest.json` are `devkit sync-skills` output and are included so `doctor` stays clean. - The qavis advisory did not block (no UI-affecting change), so nothing was bypassed. The gate log carries no SKIP or BYPASSED line: all 11 reviewers, the decisions gates and the deterministic chain passed on their own. **Unrelated-looking line in the diff, explained:** `cli/commands/init.mts` sat exactly at its shrink-only ceiling, so adding one `agentFacing` field tripped `guard-size`. Rather than override it, the redundant one-line `structureAvailableFor` wrapper is inlined — the file already bypassed it at one of four call sites, so all four now read identically. On this base the file is 1139, inside its cap. **Allowlist entry, explained:** `guard-dup` blocked on `reconcile` (`cli/commands/reconcile.mts`) vs `reconcilePath` (`cli/lib/reconcile.mts`) at c=0.8342. I read both ranges: the first is the CLI entrypoint (flags, git-top-level assert, manifest load, render), the second decides restore/delete/warn for one path from upstream/worktree/index blobs. No shared logic — the matcher scored shared reconcile-domain vocabulary and a common guarded-early-return shape. It is a pre-existing latent pair that surfaced only because the one-line `agentFacing` field put the commands file into the staged set, so it is approved via the gate's own documented path with that reasoning recorded, not overridden. **Two bugs the local gate chain caught in this PR's own test.** Committing on a feature branch runs the same reviewer fleet this PR documents, and correctness-reviewer returned two findings against `command-skill-routing.test.mts`. Both were real and both are fixed here: - `.` is not a word character, so the original right boundary `(?![\w-])` let `See devkit review.md for details` satisfy the gate for `review` — a filename routing nobody. The boundary is now `(?![\w-]|\.\w)`, which still accepts a verb ending a sentence. - The registry cross-check compared `devkit --help` against `meta.name`, which are two views of one value. Rename `meta.name` to `inspect` while `COMMANDS` still keys the module under `review` and both views agree, leaving `devkit inspect` unreachable and `devkit review` unclaimed with nothing to compare. It now parses the literal `COMMANDS` keys from `cli/index.mts` — the only witness independent of `meta` — and checks the help surface separately. That rename now reports `unreachable: ['inspect'], unclaimed: ['review']`; before, it produced no dispatch failure at all. Both have regression cases. **On `dist/`.** It is tracked and expected to match source per-commit, so the rebuilt output is included — without it a consumer would install the version *without* the routing, which is exactly the failure this story is about. Four other `dist/` files are pre-existing drift, where commits on `main` changed their source without rebuilding; they are deliberately excluded, since clearing another change's debt here would only obscure this diff. **Supersedes #584**, which was opened before eight commits landed on `main` and could no longer be updated in place: GitHub could not auto-sync its branch (real conflicts), and `devkit ship --pr` requires the PR's `baseRefOid` to equal current `origin/main`, which only a branch sync advances. This PR is the same work reconciled onto current `main`, plus the two fixes above.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Adds
devkit ship ... --dry-gates, a no-commit rehearsal that uses ship's fetched base, explicit path staging, projected runtime/config/coverage, hook capture, timeout, and cleanup. It runs configured deterministic/structure/extra gates plus the changed-comment firewall, while skipping commit, push, PR creation, and the interactive/review gate families.Fixes Story #2346 and autonomous report
19bb0dca-531d-4441-b0ef-d6130e1706ff.Direct relation to the report
The report identified that the only deterministic entry point was the internal
guard-deterministic, leaving callers to reconstruct ship's temporary index, hook arguments, runtime projection, and cleanup. This flag exposes that existing ship seam with the same base and path inputs; it does not add a second gate engine.Red without the fix
Before implementation:
The companion hook-routing regressions also failed before the implementation: dry-gates was not a recognized hook mode, and a coverage-only gate configuration emitted no deterministic orchestrator.
These tests directly exercise the ticket: the happy path proves the fetched base and explicit staged paths are used without leaving a branch or commit; the blocking path proves the gate status is returned and cleanup still occurs; hook tests prove the intended deterministic/comment-only gate selection and coverage projection.
Green with the fix
0/1isolation