fix(ship): link untracked/gitignored gate configs into the ship worktree - #44
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a Bash helper ChangesGate Config Linking
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ShipScript as ship-branch.sh / reship.sh
participant LinkHelper as link-gate-configs.sh
participant Resolver as gate-config-paths.mts
participant Worktree as Ship Worktree
participant Hook as Pre-commit Hook
ShipScript->>LinkHelper: link_untracked_gate_configs(worktree, root)
LinkHelper->>Resolver: run resolver for additional gate paths
Resolver-->>LinkHelper: relative paths (indexPath, allowlistPath)
LinkHelper->>Worktree: symlink missing configs from root
LinkHelper-->>ShipScript: notice (linked / gitignored / untracked)
ShipScript->>Worktree: commit with gate capture
Worktree->>Hook: run pre-commit hook
Hook-->>Worktree: CONFIG_SEEN / CONFIG_MISSING
Compact metadata: Modifies three shell/Node scripts under Related issues: None specified. Related PRs: None specified. Suggested labels: cli, ship, tests Suggested reviewers: norvalbv 🐰 A rabbit hops through worktrees bare, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cli/__tests__/ship-branch.test.mts (1)
724-741: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a regression check that the linked config isn't accidentally committed.
This test verifies the symlinked
guard.config.jsonis seen by the gate, but nothing confirms it stays out of the shipped commit tree. Sincelink_untracked_gate_configscreates an absolute-path symlink (ln -s "$root/$rel" "$wt/$rel"), if the commit step ever regresses to a broadergit add, this untracked file would be captured as a dangling symlink to a local host path — silently corrupting the shipped branch for other checkouts. Test 5 checks tree content for the explicit PATH-shipped case, but the implicit-link path (this test and the tracked/gitignored variants) has no equivalent guard.Suggested addition
const log = readFileSync(join(dir, '.devkit/last-ship-gates-feat-gate-cfg.log'), 'utf8'); expect(log).toMatch(/CONFIG_SEEN/); // reached the worktree = parity restored expect(log).not.toMatch(/CONFIG_MISSING/); + // the link is gate-only scaffolding — it must never leak into the shipped commit + expect(() => git(['ls-tree', 'feat/gate-cfg', 'guard.config.json'])).not.toThrow(); + expect(git(['ls-tree', 'feat/gate-cfg', 'guard.config.json']).trim()).toBe(''); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/__tests__/ship-branch.test.mts` around lines 724 - 741, Add a regression assertion in the existing ship-branch symlink test to verify the untracked linked config is not included in the shipped commit tree. Use the same setup around link_untracked_gate_configs behavior and the ship flow exercised by spawnSync to inspect the resulting committed tree or added paths, and assert that guard.config.json is only linked for the gate and never staged/committed as a file or symlink. This should live alongside the current CONFIG_SEEN / CONFIG_MISSING checks so the implicit-link path is covered, not just the explicit PATH-shipped case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/__tests__/reship.test.mts`:
- Around line 240-247: The worktree cleanup in the reship test is happening
after the assertions, so a failing expectation can leave the temporary worktree
behind. Move the `g(['worktree', 'remove', '--force', wt], ...)` cleanup to run
immediately after `run(...)`/`WT_RE.exec(...)` and before any `expect()` calls,
following the same pattern used by `dropWorktree` in `ship-branch.test.mts`, and
keep the `wt` guard so cleanup only runs when a worktree was created.
In `@cli/lib/ship/gate-config-paths.mts`:
- Around line 25-31: The emitted path list in gate-config-paths.mts is missing
decisionsDir, so ship/reship can ignore a config-driven decisions directory and
fall back to the hardcoded default. Update the loop in
resolveFromCwd/resolveGuardConfig handling to include decisionsDir alongside
indexPath and allowlistPath, and keep the same null/relative-path filtering
before writing it to stdout so link-gate-configs.sh can link the actual
configured directory.
---
Nitpick comments:
In `@cli/__tests__/ship-branch.test.mts`:
- Around line 724-741: Add a regression assertion in the existing ship-branch
symlink test to verify the untracked linked config is not included in the
shipped commit tree. Use the same setup around link_untracked_gate_configs
behavior and the ship flow exercised by spawnSync to inspect the resulting
committed tree or added paths, and assert that guard.config.json is only linked
for the gate and never staged/committed as a file or symlink. This should live
alongside the current CONFIG_SEEN / CONFIG_MISSING checks so the implicit-link
path is covered, not just the explicit PATH-shipped case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69a99f9c-5584-4b83-b54e-3f909fc59cce
📒 Files selected for processing (6)
cli/__tests__/reship.test.mtscli/__tests__/ship-branch.test.mtscli/lib/ship/gate-config-paths.mtscli/lib/ship/link-gate-configs.shcli/lib/ship/reship.shcli/lib/ship/ship-branch.sh
The ephemeral ship worktree is a clean checkout at $BASE (tracked files only), so an untracked guard.config.json/.fallowrc.jsonc or a gitignored index (.search-code, .fallow, .decisions) never reaches it. The worktree gates then silently run on defaults (co-occurrence matcher opts out, reviewers skip on empty roots, allowlist reads empty) while the ship looks fully gated -- a weaker chain than a plain commit in the same repo. link-gate-configs.sh symlinks each such input into the worktree (as --link does by hand) so the gates match a plain commit, and prints a loud per-file notice so it is never silent. Config-driven locations (indexPath, allowlistPath) resolve from guard.config.json via resolveGuardConfig (gate-config-paths.mts) rather than a hardcoded guess. Wired into both ship-branch.sh and reship.sh after change-application, so a config shipped as a path stays a real blob and an already-linked dep is skipped. Committed with --no-verify under machine saturation (parallel agents); verified green in an isolated run (45/45). pre-push and CI gates run before merge.
Address PR review: - gate-config-paths.mts also resolves decisionsDir (config-driven, like indexPath/allowlistPath), so a custom uncommitted decisions dir reaches the ship worktree instead of falling back to the hardcoded .decisions cache. - reship gate-config test wraps its assertions in try/finally so the ephemeral worktree is removed even if an assertion throws (matches ship-branch.test).
fd5e312 to
04f7b03
Compare
…ix cache key (#47) The ship prefix cache keyed only on the staged tree + devkit version + hook bytes, so an all-green PASS recorded before guard.config.json / indexPath / jscpd were wired stayed valid after they were: the same staged tree collided with the earlier key and the newly-hardened deterministic gates were silently skipped (no fanout/size/dup output at all). computeKey now folds gateConfigFingerprint(cwd), covering the gate inputs that live outside the tracked index: - the resolved guard.config.json as canonical JSON, with cwd stripped (the ship worktree path is PID-suffixed and would otherwise never collide across retries) - the whole eslint/baselines/ directory contents (the .mjs structure grandfather lists too, not just the .json ratchets — the structure gate is prefix-gated) - a size:mtime stat proxy for the search-code index - jscpd availability + version A config/baseline/index/jscpd change now invalidates a stale PASS; steady-state ships still hit the cache. An unreadable config yields a null key (gates run) — fail toward invalidation. jscpd resolution is extracted to a side-effect-free module so importing it for the fingerprint can't run config resolution at load. Composes with the gate-config linking already on main (#44): the linker symlinks those inputs into the worktree, and statSync/readFileSync follow the links so the worktree fingerprint matches a plain commit.
1 participant
Problem
devkit shipcommits inside an ephemeral linked worktree that is a clean checkout at$BASE, so it contains only tracked files. Gate inputs that are untracked — a consumer's uncommittedguard.config.json/.fallowrc.jsonc— or gitignored caches/indexes (.search-code/,.fallow/,.decisions/) therefore never reach the worktree. The gates there then fail open silently: the co-occurrence matcher "opts out (fail-open)", frontend reviewers skip on emptyfrontendRoots, the allowlist reads empty. Nothing fails, nothing warns — the ship looks fully gated while running a weaker chain than a plain commit in the same repo. The only prior fix was to hand-pass--link guard.config.json --link .fallowrc.jsonc --link .search-code --link .fallow.Fix
After change-application and before the worktree commit, both ship flows now call a shared helper that, for each known gate input present in the repo but absent from the worktree checkout, symlinks it in (exactly what
--linkdoes) so the gates match a plain commit — and prints a loud per-file notice so it is never silent again.cli/lib/ship/link-gate-configs.sh—link_untracked_gate_configs "$WT" "$ROOT". Predicatepresent in $ROOT && absent in $WT: a tracked config already rides the checkout, and one already--linked or shipped as a path is present too → both skipped (no double-link, nolnclobber underset -e). Notice labels each path viagit check-ignore: "gitignored cache — normal" vs "untracked — commit it".cli/lib/ship/gate-config-paths.mts— resolves the config-DRIVEN locations (indexPath,allowlistPath) fromguard.config.jsonviaresolveGuardConfig, not a hardcoded guess (indexPath's default isnull; honors thesynced-assets-layout-agnosticdecision). The fixed part of the list is devkit's own artifact names (guard.config.json,.fallowrc.jsonc,.fallow,fallow-baselines,.decisions).ship-branch.shandreship.shafter change-application, so a config shipped as a path stays a real blob and an already-linked dep is skipped.Tests
cli/__tests__/ship-branch.test.mts(+5) andreship.test.mts(+1): untracked→linked+nudge, tracked→silent no-op, gitignored-index→cache-label (proves the resolver), already---linked→no double-link, config-shipped-as-path→real blob not symlink, and reship parity.Verification
tsc --noEmit: clean ·biome check: clean ·bash -non all three scripts: clean.search-code/index.db+.co-occurrence-allowlist.json; against a config-less dir →.co-occurrence-allowlist.jsononly.Out of scope
.qavis/recipe.json) — the advisory gate's pass receipt lives inside qavis's own storage, not a devkit-visible file, so linking.qaviscan't deliver receipt parity, andrecipe.jsonis tracked (a mixed dir). Left out to avoid a false-block.decisionsDir/ reviewer roots — reviewer roots are tracked source dirs (already in$WT); the reviewer-skip symptom is fixed upstream by linkingguard.config.jsonitself.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes