fix(ship): a committed cache can no longer shadow the live one (sc-1489) - #385
Open
norvalbv wants to merge 2 commits into
Open
fix(ship): a committed cache can no longer shadow the live one (sc-1489)#385norvalbv wants to merge 2 commits into
norvalbv wants to merge 2 commits into
Conversation
## Problem `link_untracked_gate_configs` links gate inputs the base checkout cannot carry into the ephemeral ship worktree, and skips any path already present there. `.qavis/receipt.json` is in that candidate set — it is the gitignored, content-addressed cache `qavis qa` writes on a pass, which the ship-time `qavis-advisory` gate reads to clear its block. If a consumer commits that receipt by accident, the stale tracked copy rides the base checkout into `$WT`, the link is skipped, and the gate compares the staged sha against staleness forever. In frink (receipt committed 2026-07-28) that made every strict ship of a UI-affecting change **unsatisfiable-by-compliance**: two genuine `qavis qa --staged --route vision` passes still ended in "UI-affecting change with no qavis QA on this staged tree", with `GUARD_QAVIS_OK=1` the only exit — the exact shape report 690d2b15 already named. Shipping a fresh receipt as a PR path cannot help either: its sha covers every staged path, so a receipt cannot attest a set including itself. ## Fix A new `GATE_PROJECTION_CACHE_CANDIDATES` set (currently just `.qavis/receipt.json`) marks candidates whose bytes are a rebuildable cache rather than source. For those, a copy the BASE CHECKOUT materialised no longer wins: it is removed from the worktree and the live one linked over it, with a notice naming `git rm --cached` as the permanent fix. The shadow can never be silent again. `gate_projection_cache_is_shadowed` deliberately fires on nothing else: - a symlink — we placed it on an earlier pass - a path absent from `$WT`'s HEAD — change-application put it there, so those bytes are already live - any candidate not on the cache list — tracked source must keep winning ## Why this is safe for the shipped diff The removal is **working-tree only**. Nothing here runs `git add`/`git rm`, and the ship commit is index-only (no `-a`), so `write-tree` is invariant across the window `ship_assert_staged_unchanged` guards. A caller who also ships that path keeps their staged blob; the link just points the gate at the same live bytes. The stale comment in `assert-staged-set.sh` asserting the old "only UNTRACKED symlinks" invariant is updated to say what actually holds. ## Coverage `cli/__tests__/ship-gate-cache-shadow.test.mts` commits a `STALE` receipt, writes a `LIVE` one post-commit, and ships. It asserts the gate reads `LIVE`, that the notice names `git rm --cached`, and that the shipped commit still contains only `note.txt` — the tracked receipt is untouched by the override. Verified to fail on the pre-fix code (gate read `RECEIPT_STALE`). The hook greps CONTENT, not presence: the bug delivered a file to the gate, just the wrong one, so a `-e` probe passed straight through it. New file rather than an addition to `ship-branch.test.mts`, which is at its size ceiling. Scope: the ship/reship link path only. `qavis-advisory` is a pre-commit gate, so the review-projection branch never reads the receipt and is left unchanged. Closes sc-1489.
Contributor
|
Warning Review limit reached
Next review available in: 29 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe ship projection now detects tracked stale ChangesShip cache shadow handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Ship
participant Linker
participant Worktree
participant Cache
Ship->>Linker: create projection
Linker->>Worktree: remove stale receipt
Linker->>Cache: link live receipt
Linker-->>Ship: report override
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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__/ship-gate-cache-shadow.test.mts`:
- Line 34: Update the test environment setup around env in the ship-flow test to
remove DEVKIT_RUN_MODE, DEVKIT_REVIEW_ASSET_ROOT, and DEVKIT_REVIEW_PROGRESS
after copying process.env and GENV. Ensure these keys are absent before
seedRepo() invokes the script, preserving commit/ship mode regardless of the
parent environment.
🪄 Autofix
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 Plus
Run ID: 52cf06b4-ae54-498e-a2c6-500ef0c1a9fa
⛔ Files ignored due to path filters (2)
dist/cli/lib/ship/assert-staged-set.shis excluded by!**/dist/**dist/cli/lib/ship/link-gate-configs.shis excluded by!**/dist/**
📒 Files selected for processing (3)
cli/__tests__/ship-gate-cache-shadow.test.mtscli/lib/ship/assert-staged-set.shcli/lib/ship/link-gate-configs.sh
| function seedRepo() { | ||
| const dir = mkdtempSync(join(tmpdir(), 'shipcache-')); | ||
| dirs.push(dir); | ||
| const env = { ...process.env, ...GENV }; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear review-mode environment variables.
seedRepo() copies the parent environment. If the parent sets a review-mode key, this test can run the review projection instead of the ship flow. Delete DEVKIT_RUN_MODE, DEVKIT_REVIEW_ASSET_ROOT, and DEVKIT_REVIEW_PROGRESS from env before invoking the script.
Based on learnings, commit/ship mode requires the absence of these three environment keys.
Proposed fix
const env = { ...process.env, ...GENV };
+ delete env.DEVKIT_RUN_MODE;
+ delete env.DEVKIT_REVIEW_ASSET_ROOT;
+ delete env.DEVKIT_REVIEW_PROGRESS;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const env = { ...process.env, ...GENV }; | |
| const env = { ...process.env, ...GENV }; | |
| delete env.DEVKIT_RUN_MODE; | |
| delete env.DEVKIT_REVIEW_ASSET_ROOT; | |
| delete env.DEVKIT_REVIEW_PROGRESS; |
🤖 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-gate-cache-shadow.test.mts` at line 34, Update the test
environment setup around env in the ship-flow test to remove DEVKIT_RUN_MODE,
DEVKIT_REVIEW_ASSET_ROOT, and DEVKIT_REVIEW_PROGRESS after copying process.env
and GENV. Ensure these keys are absent before seedRepo() invokes the script,
preserving commit/ship mode regardless of the parent environment.
Source: Learnings
…acking can land (sc-1489) ## The bug `.qavis/receipt.json` is the gitignored, content-addressed cache `qavis qa` writes on a pass, read by the ship-time `qavis-advisory` gate to clear its block. `link_untracked_gate_configs` links it into the ephemeral ship worktree — but skips any path the base checkout already put there. frink committed that receipt by accident on 2026-07-28. The stale copy therefore rode the base checkout into the worktree, the link was skipped as "already present", and `qavis route` compared the staged sha against staleness forever. Two genuine `qavis qa --staged --route vision` passes still ended in "UI-affecting change with no qavis QA on this staged tree", with `GUARD_QAVIS_OK=1` the only exit. ## Two fixes, because the remedy has to be landable **1. A committed cache loses to the live one.** `GATE_PROJECTION_CACHE_CANDIDATES` names the gate inputs that are content-addressed caches rather than source. For those, a base-committed copy is removed from the gate worktree, the live one is linked over it, and it gets its own notice — never the `linked` list, whose "absent from the committed tree" wording is false for it. ```bash rm -f "$wt/$rel" # worktree only; the shipped commit is asserted unchanged by this file's test ``` Nothing there runs `git add`/`git rm` and the ship commit is index-only, so `write-tree` is invariant across the window `ship_assert_staged_unchanged` guards. `assert-staged-set.sh`'s parenthetical is corrected from "only create UNTRACKED symlinks" to "write only the WORKING TREE". **2. Ship no longer resurrects a path staged for deletion.** ```bash git -C "$WT" diff --cached --quiet --diff-filter=D -- "$f" || continue ``` The force-add pass exists to catch ignored files the diff MISSED, never to overrule one it EXPRESSED. Without this the notice's own remedy cannot land: `git rm` also deletes the operator's live receipt, so they re-run the tool, the cache comes back untracked AND gitignored, and the force-add pass sweeps it into the commit. This is general to ship, not cache-specific — it is what the original report meant by *"devkit ship cannot express deleting the tracked receipt while the gitignored cache exists on disk at the same path."* `reship.sh` has a similar shape but documented "current content of each path" semantics; unchanged, and recorded as a scope decision. ## Tests `cli/__tests__/ship-gate-cache-shadow.test.mts` — the hook greps the receipt's CONTENT, since the bug handed the gate a file, just the wrong one: 1. Committed stale receipt → gate reads `RECEIPT_LIVE`; the pasteable `git rm .qavis/receipt.json` is present and `--cached` is not; no `(untracked — commit it …)` contradiction; the shipped commit still contains only `note.txt`. 2. The untracking ship, **with the cache regenerated in between** → lands, receipt gone from the tree, notice quiet, gate still sees the live receipt. 3. Already-gitignored receipt → normal link path, no false positive. Each verified to fail against the defect it pins. Standalone file because `ship-branch.test.mts` is at its 2000-line ceiling. ## Rejected en route An earlier revision **aborted** the ship on a committed cache. It reads as the stricter fix, but the guard reads the ship BASE, so no ship can clear itself — the untracking would need a separate PR round-trip while every ship in the repo stays blocked. That is `qavis-advisory-gate.md` Rejected (c)'s shape (a block whose remedy the blocked path cannot carry), rebuilt while fixing its original instance. Recorded in the decision log along with the reship scope call. Closes sc-1489.
Owner
Author
|
Not applying this — the stated mechanism doesn't hold, verified by running it: The test can't take the review projection path from the environment:
The Happy to reconsider if there's a concrete path I've missed. |
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.
Problem
link_untracked_gate_configslinks gate inputs the base checkout cannot carry into the ephemeral shipworktree, and skips any path already present there.
.qavis/receipt.jsonis in that candidate set — itis the gitignored, content-addressed cache
qavis qawrites on a pass, which the ship-timeqavis-advisorygate reads to clear its block.If a consumer commits that receipt by accident, the stale tracked copy rides the base checkout into
$WT, the link is skipped, and the gate compares the staged sha against staleness forever. In frink(receipt committed 2026-07-28) that made every strict ship of a UI-affecting change
unsatisfiable-by-compliance: two genuine
qavis qa --staged --route visionpasses still ended in"UI-affecting change with no qavis QA on this staged tree", with
GUARD_QAVIS_OK=1the only exit —the exact shape report 690d2b15 already named. Shipping a fresh receipt as a PR path cannot help
either: its sha covers every staged path, so a receipt cannot attest a set including itself.
Fix
A new
GATE_PROJECTION_CACHE_CANDIDATESset (currently just.qavis/receipt.json) marks candidateswhose bytes are a rebuildable cache rather than source. For those, a copy the BASE CHECKOUT
materialised no longer wins: it is removed from the worktree and the live one linked over it, with a
notice naming
git rm --cachedas the permanent fix. The shadow can never be silent again.gate_projection_cache_is_shadoweddeliberately fires on nothing else:$WT's HEAD — change-application put it there, so those bytes are already liveWhy this is safe for the shipped diff
The removal is working-tree only. Nothing here runs
git add/git rm, and the ship commit isindex-only (no
-a), sowrite-treeis invariant across the windowship_assert_staged_unchangedguards. A caller who also ships that path keeps their staged blob; the link just points the gate at the
same live bytes. The stale comment in
assert-staged-set.shasserting the old "only UNTRACKEDsymlinks" invariant is updated to say what actually holds.
Coverage
cli/__tests__/ship-gate-cache-shadow.test.mtscommits aSTALEreceipt, writes aLIVEonepost-commit, and ships. It asserts the gate reads
LIVE, that the notice namesgit rm --cached, andthat the shipped commit still contains only
note.txt— the tracked receipt is untouched by theoverride. Verified to fail on the pre-fix code (gate read
RECEIPT_STALE). The hook greps CONTENT,not presence: the bug delivered a file to the gate, just the wrong one, so a
-eprobe passed straightthrough it.
New file rather than an addition to
ship-branch.test.mts, which is at its size ceiling.Scope: the ship/reship link path only.
qavis-advisoryis a pre-commit gate, so the review-projectionbranch never reads the receipt and is left unchanged.
Closes sc-1489.
Summary by CodeRabbit
Bug Fixes
Documentation