fix(mt#4885): Demote cwd-derived roots to the fallbackCwd tier in two calibration writers - #3607
Merged
Merged
Conversation
… calibration writers [no-deploy-impact] `gate-walk-provenance.ts` and `require-execution-evidence-before-merge.ts` each resolve `const repoRootDir = findRepoRoot(input.cwd)` and then pass that value as `projectDir` — the ladder's AUTHORITATIVE top tier, which outranks `CLAUDE_PROJECT_DIR`. Both files asserted in a comment that the value "IS authoritative — it is resolved by the caller, not a raw shell cwd". That is false: the caller resolves it FROM the raw cwd. `calibrationLogPath`'s own docblock names this exact mistake: "Passing that cwd as `projectDir` would preserve the bug through the migration — the whole point is that it lands in the lower tier." In a session workspace `findRepoRoot` returns the CLONE, which is a real repo root and the wrong project, so the record is keyed to a transient workspace instead of the project. Six call sites demoted to `fallbackCwd`; both false comments corrected rather than left contradicting the code. This fixes the TIER only. With `CLAUDE_PROJECT_DIR` unset the ladder still falls through to this value and still keys to the clone — a separate defect (which root a session workspace SHOULD resolve to), tracked as mt#4885 SC3 and deliberately not changed here. Class-not-instance: the only other `projectDir` passer is `coverage-claim-path-detector.ts`, which uses `deriveHookRepoRoot()` (`findRepoRoot(import.meta.dir)`) — a stable root, not cwd-derived, so its use of the top tier is correct and is left alone. The two `dispatcher.ts` sites are pass-throughs of the caller's option, not sources.
Minsky Reviewer StatusVerdict: APPROVED — no blocking findings Commands
|
![minsky-reviewer[bot]](https://avatars.githubusercontent.com/u/52695?s=60&v=4){kind=small}
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Overall this is a focused change that demotes cwd-derived roots from projectDir to fallbackCwd at the two intended writers and adds a targeted test to pin the tiering behavior. I verified the call-site edits and comment corrections in both .minsky/ and generated .claude/ copies, and confirmed dispatcher.ts already documents the intended ladder semantics. Two minor non-blocking nits: stale references in newly-added comments still point to “mt#4885 SC3” while the spec/PR state that SC3–SC6 moved to mt#4954 — please update those to avoid misrouting follow-up work. No other blocking issues found. Documentation impact appears nil (internal tier correction).
Findings
- [NON-BLOCKING] .minsky/hooks/gate-walk-provenance.ts:551 — Stale task reference in comment — SC3 moved to mt#4954 but comment still points to mt#4885 SC3
The newly added comment block still says "tracked as mt#4885 SC3" even though the PR description/spec narrowing states SC3–SC6 were moved to mt#4954. This can mislead future readers about where the remaining work is tracked. Please update the reference to the correct task id (mt#4954) or reword to avoid an SC number tied to this task.
Anchor: .minsky/hooks/gate-walk-provenance.ts:541-557 (the “This fixes the TIER only…” paragraph).
- [NON-BLOCKING] .minsky/hooks/require-execution-evidence-before-merge.ts:1006 — Stale task reference in docblock — points to mt#4885 SC3 which the PR says was moved to mt#4954
The updated docblock still says "tracked as mt#4885 SC3" for the unresolved decision, but the PR description narrows scope and moves SC3–SC6 to mt#4954. Please update this reference to the correct follow-up task (mt#4954) to avoid misrouting future work.
Anchor: .minsky/hooks/require-execution-evidence-before-merge.ts:986-1023 — the paragraph ending with "tracked as mt#4885 SC3."
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| SC1 (discharged 2026-09-03) — the reversibility check is run and recorded. 6 of 11 project keys resolve to session workspaces; the keys are NOT other real projects, so the "close this task as void" branch does not apply. | Met | Task spec’s “## Findings (2026-09-03) — SC1 reversibility check RUN. The keys ARE session workspaces.” records the measurement and matches the spec’s discharge note. No code change required here. |
| SC2 — defect 1 (the stranding cause). gate-walk-provenance.ts and require-execution-evidence-before-merge.ts no longer pass a findRepoRoot(input.cwd)-derived root as projectDir. Each either passes it as fallbackCwd (its correct tier) or passes a stable root, and the :497 comment asserting repoRootDir "IS authoritative" is corrected or removed. Verified by reading the four call sites, not by the absence of new stray keys. | Met | Both writers demote the tier: .minsky/hooks/gate-walk-provenance.ts:541-557 changes logCalibrationRecord(..., { projectDir: repoRootDir }) to { fallbackCwd: repoRootDir } and corrects the comment. .minsky/hooks/require-execution-evidence-before-merge.ts:1006-1023 updates appendAtCoverageCalibration to use { fallbackCwd: repoRootDir }, and five call sites at :1289, :1314, :1329, :1358 switch to { fallbackCwd: repoRootDir }. Generated twins updated similarly under .claude/hooks/… . New test .minsky/hooks/calibration-root-tier.test.ts pins the tier behavior. |
| SC3 — defect 2 (the resolution rule). A recorded decision states what a session workspace's telemetry SHOULD key to — the clone it ran in, or the upstream project it is a clone of — with the reason. It must also state whether deriveHookRepoRoot() resolves to the main repo when the executing hook is a session clone's own copy. | N/A | Out of scope for this task per the “Scope narrowed to SC2 (2026-09-04) — the rest moved to mt#4954” section in the Task Specification. That section explicitly moves SC3 to mt#4954 with its blockers/evidence. |
| SC4 — defect 3 (the reader gap). Writer and reader resolve the project root through ONE shared helper, so the precedence cannot diverge again (the structural move mt#4784 made for the directory). | N/A | Moved to mt#4954 per “Scope narrowed to SC2 (2026-09-04) — the rest moved to mt#4954”. This PR does not introduce the shared helper; only SC2 is implemented here. |
| SC5 — surface boundary. The spec states explicitly whether packages/shared/src/calibration-review-store-paths.ts and its three callers are in scope for SC4, or why they are deferred. | N/A | Deferred to mt#4954 per the Task Specification’s narrowing section naming SC5 as moved. No change to these files in this PR. |
| SC6 — residue disposition. Existing stray streams are migrated into the owning project's key or explicitly declared disposable, with a read-back comparison before any source is removed. | N/A | Deferred to mt#4954 per the Task Specification’s “Scope narrowed to SC2 … the rest moved to mt#4954”. No migration code is present in this PR. |
Documentation impact
- no-update-needed — Behavior change is internal to hook path resolution tiering (projectDir→fallbackCwd at specific call sites). No new public exports, commands, or user-facing flags were added. Docs most likely to be affected would be ADR-028/D4 path semantics, which already state the tiering and are now more correctly honored. I did not find any docs in this PR; no invalidated sentences were identified.
…nce to mt#4954 [no-deploy-impact] Both non-blocking findings were correct. The comments introduced by this PR pointed the reader at "mt#4885 SC3" for the unresolved root-resolution question — but the same change moved SC3 to mt#4954, so the reference was stale the moment it was written. Retargeted in both sources; the generated .claude/hooks twins regenerate from them rather than being hand-edited. Class scan: `grep 'mt#4885 SC'` across .minsky/hooks and .claude/hooks returns only the generated copy, which this commit regenerates.
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
The follow-up commit resolves the prior non-blocking findings by retargeting stale SC3 references to mt#4954, and the PR satisfies SC2: all cwd-derived roots are now passed via fallbackCwd instead of projectDir across both affected writers, with corrected comments explaining the rationale. The generated .claude twins mirror the source updates. I see no new defects introduced by these changes and the unresolved resolution-rule work is correctly deferred to mt#4954. Approving to merge.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| SC1 — the reversibility check is run and recorded. 6 of 11 project keys resolve to session workspaces; the keys are NOT other real projects, so the "close this task as void" branch does not apply. | Met | Task spec itself records this as discharged and cites the findings; no code change required in this PR. See Success Criteria preamble and "## Findings (2026-09-03) — SC1 reversibility check RUN" in the spec. |
| SC2 — defect 1 (the stranding cause). gate-walk-provenance.ts and require-execution-evidence-before-merge.ts no longer pass a findRepoRoot(input.cwd)-derived root as projectDir. Each either passes it as fallbackCwd (its correct tier) or passes a stable root, and the :497 comment asserting repoRootDir "IS authoritative" is corrected or removed. Verified by reading the four call sites, not by the absence of new stray keys. | Met | .minsky/hooks/gate-walk-provenance.ts:506-521 — comment corrected and call changed to logCalibrationRecord(..., { fallbackCwd: repoRootDir }). .minsky/hooks/require-execution-evidence-before-merge.ts:991-1002 (docblock correction) and call sites at 1289, 1314, 1329, 1358 all changed from { projectDir: repoRootDir } to { fallbackCwd: repoRootDir }. The generated .claude/hooks twins mirror the same changes. |
| SC3 — defect 2 (the resolution rule). A recorded decision states what a session workspace's telemetry SHOULD key to — the clone it ran in, or the upstream project it is a clone of — with the reason. It must also state whether deriveHookRepoRoot() resolves to the main repo when the executing hook is a session clone's own copy. | N/A | Per the spec's "Scope narrowed to SC2 (2026-09-04) — the rest moved to mt#4954" section, SC3 was moved to mt#4954. This PR intentionally does not change the resolution rule; comments now point to mt#4954 for that decision. |
| SC4 — defect 3 (the reader gap). Writer and reader resolve the project root through ONE shared helper, so the precedence cannot diverge again (the structural move mt#4784 made for the directory). | N/A | Deferred to mt#4954 per the spec's narrowing note (2026-09-04). This PR does not introduce a shared helper for readers; it only corrects writer tier usage. |
| SC5 — surface boundary. The spec states explicitly whether packages/shared/src/calibration-review-store-paths.ts and its three callers are in scope for SC4, or why they are deferred. | N/A | Deferred to mt#4954 per the spec's narrowing note (2026-09-04). This PR does not touch the stores; comments in the changed files now reference mt#4954 for the unresolved rule. |
| SC6 — residue disposition. Existing stray streams are migrated into the owning project's key or explicitly declared disposable, with a read-back comparison before any source is removed. | N/A | Deferred to mt#4954 per the spec's narrowing note (2026-09-04). No migration is attempted in this PR; scope is limited to tier demotion and test coverage thereof. |
Documentation impact
- no-update-needed — Change is an internal tier-selection fix in guard hook writers (.minsky/.claude hooks) with no user-facing API/CLI changes. ADRs and docs describing the dispatcher/service remain accurate; no doc strings or public contracts were added or changed in this PR.
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Verified the SC2 fix: all six cwd-derived roots in the two affected writers have been demoted from the projectDir tier to fallbackCwd, and the misleading “IS authoritative” comments were corrected. I spot-checked the call sites in both .minsky/ sources and their generated .claude/ twins; all now pass { fallbackCwd: repoRootDir }. The new targeted test file correctly exercises the precedence with CLAUDE_PROJECT_DIR set and provides a negative control. No new defects introduced by the fix were observed, and the remaining resolution-rule work is rightly deferred to mt#4954. Approving to merge.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| SC1 — the reversibility check is run and recorded. 6 of 11 project keys resolve to session workspaces; the keys are NOT other real projects, so the "close this task as void" branch does not apply. | Met | Per the current spec, SC1 is discharged in planning and does not require code changes in this PR. The PR description reiterates the findings and narrows scope to SC2; no code work is expected here. |
SC2 — defect 1 (the stranding cause). gate-walk-provenance.ts and require-execution-evidence-before-merge.ts no longer pass a findRepoRoot(input.cwd)-derived root as projectDir. Each either passes it as fallbackCwd (its correct tier) or passes a stable root, and the :497 comment asserting repoRootDir "IS authoritative" is corrected or removed. Verified by reading the four call sites, not by the absence of new stray keys. |
Met | .minsky/hooks/gate-walk-provenance.ts:567-587 — appendCalibrationRecord now calls logCalibrationRecord(..., { fallbackCwd: repoRootDir }) with an updated docblock correcting the prior authoritative-claim. |
.minsky/hooks/require-execution-evidence-before-merge.ts:1006-1025 — appendAtCoverageCalibration now passes { fallbackCwd: repoRootDir } and its docblock is corrected. |
||
.minsky/hooks/require-execution-evidence-before-merge.ts:1336-1341, 1361-1366, 1380-1385, 1410-1415 — the SC-coverage, test-first, render-path, and consumer-account writers each pass { fallbackCwd: repoRootDir }. |
||
| The generated .claude twins mirror the same demotions in their respective hunks in the diff. |
Documentation impact
- no-update-needed — Bugfix-level change confined to hook internals: call sites now pass
fallbackCwdinstead ofprojectDirtologCalibrationRecord. No CLI/API surface, flags, or documented behavior changed. No docs in repo reference these internal option keys for calibration writers; no docs updated in this PR, and none appear necessary.
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Verified SC2 is satisfied: all six cwd-derived roots across the two affected writers are demoted from projectDir to fallbackCwd, and the misleading “authoritative” comments are corrected. The .claude/ generated twins mirror the source changes. The new targeted test file correctly constructs the discriminating environment and pins the intended precedence without altering unrelated behavior. I found no new defects introduced by the fix and the remaining resolution-rule work is properly deferred to mt#4954 per the spec narrowing. Approving to merge.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| SC1 (discharged 2026-09-03) — the reversibility check is run and recorded. 6 of 11 project keys resolve to session workspaces; the keys are NOT other real projects, so the "close this task as void" branch does not apply. | Met | This criterion is planning-only and recorded in the task spec's Findings section; no code change required in this PR. The PR body reiterates SC1 as discharged. No repository artifact to verify. |
SC2 — defect 1 (the stranding cause). gate-walk-provenance.ts and require-execution-evidence-before-merge.ts no longer pass a findRepoRoot(input.cwd)-derived root as projectDir. Each either passes it as fallbackCwd (its correct tier) or passes a stable root, and the :497 comment asserting repoRootDir "IS authoritative" is corrected or removed. Verified by reading the four call sites, not by the absence of new stray keys. |
Met | Both writers demote the tier: .minsky/hooks/gate-walk-provenance.ts:510-527 and .claude/hooks/gate-walk-provenance.ts:510-527 change logCalibrationRecord(..., { projectDir: repoRootDir }) to { fallbackCwd: repoRootDir } and replace the misleading comment with an explanatory note (mt#4885). Similarly, .minsky/hooks/require-execution-evidence-before-merge.ts:1000-1012, 1293-1299, 1318-1324, 1333-1339, 1362-1368 (and their .claude/ twins) switch five call sites from projectDir to fallbackCwd and update the docblock to correct the prior claim. New test .minsky/hooks/calibration-root-tier.test.ts asserts the precedence with CLAUDE_PROJECT_DIR set. |
SC3 — defect 2 (the resolution rule). A recorded decision states what a session workspace's telemetry SHOULD key to … and whether deriveHookRepoRoot() resolves to the main repo when executing from a clone. |
N/A | Out of scope for this PR per the spec's "Scope narrowed to SC2 (2026-09-04) — the rest moved to mt#4954" section. The work is deferred to mt#4954. |
| SC4 — defect 3 (the reader gap). Writer and reader resolve the project root through ONE shared helper, so the precedence cannot diverge again. | N/A | Deferred to mt#4954 as noted in the spec's narrowing update; not implemented in this PR. |
SC5 — surface boundary. The spec states explicitly whether packages/shared/src/calibration-review-store-paths.ts and its three callers are in scope for SC4, or why they are deferred. |
N/A | Deferred to mt#4954 per the spec's "Scope narrowed to SC2" note; this PR only addresses SC2. |
| SC6 — residue disposition. Existing stray streams are migrated into the owning project's key or explicitly declared disposable, with a read-back comparison before any source is removed. | N/A | Deferred to mt#4954 per the spec's narrowing; no migration is executed in this PR. |
Documentation impact
- no-update-needed — Change is an internal bugfix to tier selection for two calibration writers and accompanying tests. No CLI, API, or documented behavior surface changed; docs about calibration log paths (ADR-028 and related) remain accurate. No documentation files were touched in the PR.
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.
Summary
Two calibration writers resolve
const repoRootDir = findRepoRoot(input.cwd)and pass that valueas
projectDir— the top, AUTHORITATIVE tier ofcalibrationLogPath's ladder, which outranksCLAUDE_PROJECT_DIR. Both files asserted in a comment that the value "IS authoritative — it isresolved by the caller, not a raw shell cwd." That is false: the caller resolves it from the
raw cwd.
calibrationLogPath's own docblock names this exact mistake:In a session workspace
findRepoRootreturns the clone — a real repo root, and the wrongproject — so the record is keyed to a transient workspace instead of the project.
This fixes the TIER only. With
CLAUDE_PROJECT_DIRunset the ladder still falls through tothis value and still keys to the clone. That is a separate defect (which root a session workspace
should resolve to) and is tracked as mt#4954, not silently absorbed here.
Key changes
.minsky/hooks/gate-walk-provenance.ts— 1 call site demoted; the false "IS authoritative"comment replaced with the correction and its basis.
.minsky/hooks/require-execution-evidence-before-merge.ts— 5 call sites demoted(
appendAtCoverageCalibrationplus the SC-coverage, test-first, render-path and consumer-accountsurfaces); same comment correction on its docblock.
.minsky/hooks/calibration-root-tier.test.ts— new, pins the tier for both writers..claude/hooks/twins regenerated by the pre-commit step, not hand-edited.Class-not-instance scan. The only other
projectDirpasser iscoverage-claim-path-detector.ts:261, which usesderiveHookRepoRoot()(
findRepoRoot(import.meta.dir)) — a stable root, not cwd-derived — so its use of the top tier iscorrect and is left alone. The two
dispatcher.tssites are pass-throughs of the caller's option,not sources.
Spec verification
project keys hash to session-workspace paths, so the spec's own "this task is void if they are
other real projects" branch does not apply. Recorded in mt#4885
## Findings (2026-09-03).narrowed to SC1+SC2 in the same change, so its DONE-on-merge is honest rather than closing four
unmet criteria silently. Markers, one per criterion, since a per-criterion deferral is what the
coverage gate reads:
[sc3-deferred: mt#4954]
[sc4-deferred: mt#4954]
[sc5-deferred: mt#4954]
[sc6-deferred: mt#4954]
Testing
Typecheck: 0 errors across 8 projects (
validatedWorkspace= the session;infra/tsconfig.jsonskipped, dependencies not installed locally — CI covers it). Lint: 0 errors, 0 warnings over
4371 files.
Execution evidence:
sc2 — negative control: reverted the FULL behavioural change (all 6 sites,
fallbackCwd:→projectDir:, via sed) and re-ran; 2 of 3 cases went red.The third case stays green under the control by design — it is the
CLAUDE_PROJECT_DIR-unsetpath, which is tier-independent, and it exists to guard the other direction (demoting the tier must
not break the ordinary case). Restored afterwards;
grep -c 'projectDir: repoRootDir'returns 0 inboth files.
Why a dedicated test file rather than assertions in the two existing suites. Those 264 tests
pass identically before and after this fix, because they run with
CLAUDE_PROJECT_DIRunset —and with an empty top tier,
projectDir: rootandfallbackCwd: rootresolve to the same path.They cannot see this defect. The discriminating condition has to be constructed:
CLAUDE_PROJECT_DIRset, to a directory that is not the one the caller passes. A test that cannot fail against the
defect is not evidence about it (mem#704).
Deploy verification
isDeploySurfaceFilerun over the actual changed set —.minsky/hooks/gate-walk-provenance.ts,.minsky/hooks/require-execution-evidence-before-merge.ts,.minsky/hooks/calibration-root-tier.test.ts, and the generated.claude/hooks/twins — returnsfalse for every one, so
[no-deploy-impact]is a checked claim rather than a rememberedpattern list. No post-merge deploy verification is owed.
Parallel work
PR #3253 (mt#3854) touches
.minsky/hooks/dispatcher.tsand.minsky/hooks/coverage-receipt.ts— verified by reading its changed-file list, not its title. Neither file is touched here: this
PR's surface (
gate-walk-provenance.ts,require-execution-evidence-before-merge.ts) is entirelyclear of it. The blocked work is SC4, which moved to mt#4954; a merge watch on #3253 was armed
2026-09-04T03:34:47Z. PR #3412 was also checked — 10 hook files, none in scope.