fix: install-time store verification, staging cleanup on failure, mode-aware status

[min0625]

Summary

Three fixes found by black-box testing of bin/graft:

1. Copy-mode installs re-hash what they materialize from the content store (REQ-STORE-INSTALL-VERIFY).
Previously a corrupted (deliberately tampered, read-only bit bypassed) store entry was installed into the vendor directory silently with exit 0 — the corruption was only caught by the next apply/status. The spec (§5.4/§7) promises copy mode behaves exactly like graft without a cache, which would have failed with exit 4 at checkout time. Now install re-hashes the staged tree before the rename into <dest>; a mismatch installs nothing, fails with exit 4 (cached content for … is corrupted), and removes the corrupted entry so a plain re-run re-fetches and heals.

2. A failed reconcile no longer leaves <vendor>/.graft-tmp behind (REQ-APPLY-STAGING-CLEANUP).
Error paths (integrity failure, lost commit, …) returned before the staging cleanup, leaving an empty .graft-tmp (and an otherwise-empty vendor root) in the project. Cleanup is now deferred so it runs on failure too; the success path still reports its removal error.

3. status judges each dest by the current link mode (REQ-STATUS-MODE-DRIFT).
status classified a dest by its on-disk form alone (symlink → link-target check, real tree → hash check), ignoring GRAFT_LINK_MODE. Both mode-drift quadrants reported ok while apply would rewrite the dest — breaking the "status ok ⇒ apply is a no-op" contract the CI-guard use case relies on (a link-mode leftover passed a copy-mode status but is a broken symlink on every other machine). A dest materialized in the other mode now reports modified, matching apply's drift judgment.

Docs kept in sync

• docs/design.zh-TW.md + docs/design.md: §4.5 status semantics (new mode-drift bullet), §5.1 flow diagram & staging paragraph, §5.4 content-store/copy-mode wording ("store hit needs no network" — the no-re-hash claim no longer holds at install), §7 shared-cache paragraph
• docs/requirements.md: three new REQ rows, each bound to a covering test (// spec: REQ-…)
• docs/testing.md (+ local zh manual): new black-box rows in §3.7 and §4.2
• README.md + README.zh-TW.md: status section mentions mode-drift reporting

Testing

• New tests: TestReconcile_corruptedStoreEntryFailsAndHeals, TestReconcile_failedRunRemovesStaging, TestStatus_modeDriftReportsModified
• Verified end-to-end with the rebuilt binary: tampered store entry → apply exits 4, nothing installed, entry removed, second apply self-heals; failed apply leaves no .graft-tmp; both mode-drift directions report modified (exit 1) and re-applying in the current mode returns status to all-ok

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Verify copy-mode store installs and always clean vendor staging on reconcile failure

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Re-hash copy-mode materializations before vendor rename; fail with exit 4 on mismatch.
• Delete corrupted content-store entries so the next run re-fetches and self-heals.
• Always remove /.graft-tmp (and empty vendor root) even on reconcile failure.

Diagram

graph TD
  A["graft apply"] --> B["vendordir.Reconcile"] --> C["store/<hash>"] --> D["Materialize to .graft-tmp"] --> E["Hash staged tree"] --> F{"Hash matches lock?"}
  F -- "yes" --> G["Rename into vendor"] --> H["Deferred staging cleanup"]
  F -- "no" --> I["Delete corrupted store entry"] --> H

Loading

High-Level Assessment

The approach matches the spec promise that copy mode behaves like an uncached checkout: verify before install, fail with integrity exit code, and prevent corrupted content from ever reaching vendor. Alternatives like verifying only during cache verify or relying on read-only bits would not catch tampering at install time, so the PR’s strategy is the most reliable place to enforce the guarantee.

Files changed (7) +149 / -27

Bug fix (1) +44 / -10

vendordir.goRe-hash staged installs; defer staging cleanup to run on failure +44/-10

Re-hash staged installs; defer staging cleanup to run on failure

• Adds a deferred cleanup in 'Reconcile' to ensure '.graft-tmp' (and an otherwise-empty vendor root) is removed even when reconcile returns early due to errors. In copy mode, 'install()' now re-hashes the staged materialization and, on mismatch, evicts the corrupted store entry and returns a dedicated exit-4 integrity error before anything is installed into vendor.

internal/vendordir/vendordir.go

Tests (1) +82 / -0

vendordir_test.goAdd tests for corrupted store healing and failure-time staging cleanup +82/-0

Add tests for corrupted store healing and failure-time staging cleanup

• Introduces 'TestReconcile_corruptedStoreEntryFailsAndHeals' to ensure a tampered store entry causes an exit-4 failure, installs nothing, evicts the entry, and succeeds on rerun. Adds 'TestReconcile_failedRunRemovesStaging' to assert failed reconciles leave no '.graft-tmp' or empty vendor root behind.

internal/vendordir/vendordir_test.go

Documentation (5) +23 / -17

design.mdDocument copy-mode re-verification and failure-time staging cleanup +6/-5

Document copy-mode re-verification and failure-time staging cleanup

• Updates the apply flow diagram and staging description to reflect that copy-mode materialization is re-hashed before renaming into vendor and that '.graft-tmp' is removed on failure as well as success. Adjusts content-store wording to remove the prior claim that store hits require no re-hash at install time.

docs/design.md

design.zh-TW.md同步更新中文設計文件：copy 模式重驗證與失敗清理暫存 +6/-5

同步更新中文設計文件：copy 模式重驗證與失敗清理暫存

• Mirrors the English design doc changes in zh-TW: copy-mode materialization is re-hashed before vendor install and '.graft-tmp' is removed even when reconcile fails. Also updates shared-cache and content-store wording for consistency.

docs/design.zh-TW.md

requirements.mdAdd requirements for store-install verification and staging cleanup +2/-0

Add requirements for store-install verification and staging cleanup

• Adds two new requirements rows: verifying copy-mode installs against the locked hash (and evicting corrupted store entries), and guaranteeing '.graft-tmp' cleanup on all reconcile exits. These requirements are intended to be covered by new tests.

docs/requirements.md

testing.mdAdd black-box scenario for corrupted store entry during apply +3/-2

Add black-box scenario for corrupted store entry during apply

• Extends the cache testing matrix with a new scenario: tamper with a store entry, delete vendor, and confirm 'apply' fails with exit 4, installs nothing, evicts the entry, then self-heals on the second run. Renumbers subsequent cache scenarios accordingly.

docs/testing.md

store.goClarify store contract: copy-mode installs re-hash materializations +6/-5

Clarify store contract: copy-mode installs re-hash materializations

• Updates package-level comments to point to the correct spec section and to explicitly state that copy-mode installs re-hash materialized trees so corrupted entries are not installed silently. No functional store behavior changes in this file.

internal/store/store.go

ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan

[codecov]

Codecov Report

❌ Patch coverage is 82.85714% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/vendordir/vendordir.go	82.60%	3 Missing and 1 partial ⚠️
cmd/graft/status.go	83.33%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 15 rules

1. Racy store entry deletion ✓ Resolved 🐞 Bug ☼ Reliability

Description

install() deletes the shared store entry directory on hash mismatch while install workers are still
running in parallel, so other deps materializing the same store path can fail with low-level
filesystem errors instead of the intended corruption integrity error. This can make corrupted-store
scenarios noisier/flakier when multiple deps share the same content hash (the store intentionally
deduplicates identical trees).

Code

internal/vendordir/vendordir.go[R436-440]

+	if got != dep.Hash {
+		// Drop the corrupted entry so the next run re-fetches it.
+		gitrun.RemoveAll(storePath) //nolint:errcheck,gosec // Best-effort; the integrity error wins.
+
+		return corruptedStoreErr(dep, got)

Evidence

The install phase is explicitly parallelized, and errors are aggregated rather than fail-fast;
deleting a shared store entry during this phase can affect other workers still reading it. The store
is designed to deduplicate identical content by hash, so shared storePath across deps is an
expected scenario.

internal/vendordir/vendordir.go[281-327]
internal/vendordir/vendordir.go[416-441]
internal/store/store.go[3-9]
internal/store/store.go[43-77]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`install()` removes `storePath` immediately upon detecting a staged-tree hash mismatch. Because installs are executed concurrently (NumCPU workers), another dependency that references the same `storePath` (same content hash) can be in `store.Materialize()` at the same time and fail with an OS error (e.g., missing files/dirs) rather than a clean `CodeIntegrity` corruption error.

### Issue Context
- The content store is deduplicated by hash; multiple deps can share one on-disk entry.
- Install phase runs in parallel; deletion during that phase can race other readers.

### Fix Focus Areas
- internal/vendordir/vendordir.go[281-327]
- internal/vendordir/vendordir.go[416-467]

### Suggested approach
1. Stop deleting `storePath` inside `install()`.
2. When a corruption mismatch is detected, return an error that still unwraps to the `*clierr.Error` (so exit code stays 4) but also carries the `storePath` (e.g., a custom wrapper type with `Unwrap() error`).
3. In `reconcileDeps()`, after all install workers have finished (`wg.Wait()`), collect unique corrupted `storePath`s from the recorded install errors and delete them once (best-effort) *after* installs complete.
4. (Optional) If deletion fails, consider appending a best-effort warning/error detail without changing the primary integrity failure semantics.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---