From cd1132935940d5c25a0bff690cf2ee7eb5b78915 Mon Sep 17 00:00:00 2001 From: Eugene Dobry Date: Thu, 3 Sep 2026 23:45:10 -0400 Subject: [PATCH 1/2] fix(mt#4885): Demote cwd-derived roots to the fallbackCwd tier in two calibration writers [no-deploy-impact] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .claude/hooks/gate-walk-provenance.ts | 22 +++- ...require-execution-evidence-before-merge.ts | 22 ++-- .minsky/hooks/calibration-root-tier.test.ts | 114 ++++++++++++++++++ .minsky/hooks/gate-walk-provenance.ts | 22 +++- ...require-execution-evidence-before-merge.ts | 22 ++-- 5 files changed, 178 insertions(+), 24 deletions(-) create mode 100644 .minsky/hooks/calibration-root-tier.test.ts diff --git a/.claude/hooks/gate-walk-provenance.ts b/.claude/hooks/gate-walk-provenance.ts index 53b43571b6..a948a18ef4 100755 --- a/.claude/hooks/gate-walk-provenance.ts +++ b/.claude/hooks/gate-walk-provenance.ts @@ -496,15 +496,27 @@ export function appendCalibrationRecord( repoRootDir: string ): void { // mt#4752: the shared helper derives the filename from the stream NAME, so it - // cannot drift from the convention the .gitignore globs encode. Unlike the - // detectors migrated alongside this one, `repoRootDir` here IS authoritative — - // it is resolved by the caller, not a raw shell cwd — so it goes in the - // `projectDir` tier, which outranks `CLAUDE_PROJECT_DIR`. + // cannot drift from the convention the .gitignore globs encode. + // + // mt#4885: `repoRootDir` goes in the `fallbackCwd` tier, NOT `projectDir`. + // This comment previously claimed it "IS authoritative — it is resolved by the + // caller, not a raw shell cwd". That was false: the only caller resolves it as + // `findRepoRoot(input.cwd)`, so it IS a raw shell cwd, merely one that has been + // walked up to a repo root. `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" — because in a + // session workspace `findRepoRoot` returns the CLONE, which is a legitimate repo + // root and the wrong project. Demoting it lets `CLAUDE_PROJECT_DIR` outrank it. + // + // This fixes the TIER only. When `CLAUDE_PROJECT_DIR` is unset the ladder still + // falls through to this value and still keys to the clone — that is a separate + // defect (which root a session workspace SHOULD resolve to), tracked as mt#4885 + // SC3 and deliberately not changed here. // // The former third parameter (`logRelPath`, defaulted to the path literal) is // gone: both call sites used the default, and a path-taking parameter is the // degree of freedom that let a sibling's filename be misspelled (mt#2492). - logCalibrationRecord(CALIBRATION_LOG_NAME, record, { projectDir: repoRootDir }); + logCalibrationRecord(CALIBRATION_LOG_NAME, record, { fallbackCwd: repoRootDir }); } // --------------------------------------------------------------------------- diff --git a/.claude/hooks/require-execution-evidence-before-merge.ts b/.claude/hooks/require-execution-evidence-before-merge.ts index 19497c4756..5b4afb0703 100755 --- a/.claude/hooks/require-execution-evidence-before-merge.ts +++ b/.claude/hooks/require-execution-evidence-before-merge.ts @@ -986,14 +986,22 @@ export function isAtCoverageSkipped(): boolean { * parameter itself is now gone, so neither failure has anywhere left to occur: every call site * names a stream constant, and `logCalibrationRecord` derives the filename from it. * - * `repoRootDir` is authoritative here — the caller resolved it, it is not a raw shell cwd — so it - * goes in the `projectDir` tier, which outranks `CLAUDE_PROJECT_DIR`. + * mt#4885: `repoRootDir` goes in the `fallbackCwd` tier, NOT `projectDir`. This docblock + * previously claimed it was "authoritative here — the caller resolved it, it is not a raw shell + * cwd". That was false: the caller resolves it as `findRepoRoot(input.cwd)`, so it IS a raw shell + * cwd walked up to a repo root. In a session workspace `findRepoRoot` returns the CLONE — a real + * repo root, and the wrong project — which is why `calibrationLogPath`'s docblock reserves the top + * tier for an ALREADY-RESOLVED authoritative directory and says passing a cwd there "would preserve + * the bug through the migration". Demoting it lets `CLAUDE_PROJECT_DIR` outrank it. + * + * TIER only. With `CLAUDE_PROJECT_DIR` unset the ladder still falls through to this value and still + * keys to the clone — a separate defect, tracked as mt#4885 SC3. */ export function appendAtCoverageCalibration( record: Record, repoRootDir: string ): void { - logCalibrationRecord(AT_COVERAGE_STREAM, record, { projectDir: repoRootDir }); + logCalibrationRecord(AT_COVERAGE_STREAM, record, { fallbackCwd: repoRootDir }); } /** Result of fetching a task's spec for the AT-coverage check. */ @@ -1284,7 +1292,7 @@ if (import.meta.main) { ); if (scCoverage.calibrationRecord) { logCalibrationRecord(SC_COVERAGE_STREAM, scCoverage.calibrationRecord, { - projectDir: repoRootDir, + fallbackCwd: repoRootDir, }); } if (scCoverage.warning) { @@ -1309,7 +1317,7 @@ if (import.meta.main) { ); if (testFirst.calibrationRecord) { logCalibrationRecord(TEST_FIRST_STREAM, testFirst.calibrationRecord, { - projectDir: repoRootDir, + fallbackCwd: repoRootDir, }); } if (testFirst.warning) { @@ -1324,7 +1332,7 @@ if (import.meta.main) { const renderPath = runRenderPathCalibration(task, context.prNumber, prFiles, prBody); if (renderPath.calibrationRecord) { logCalibrationRecord(RENDER_PATH_STREAM, renderPath.calibrationRecord, { - projectDir: repoRootDir, + fallbackCwd: repoRootDir, }); } if (renderPath.warning) { @@ -1353,7 +1361,7 @@ if (import.meta.main) { ); if (consumerAccount.calibrationRecord) { logCalibrationRecord(CONSUMER_ACCOUNT_STREAM, consumerAccount.calibrationRecord, { - projectDir: repoRootDir, + fallbackCwd: repoRootDir, }); } if (consumerAccount.warning) { diff --git a/.minsky/hooks/calibration-root-tier.test.ts b/.minsky/hooks/calibration-root-tier.test.ts new file mode 100644 index 0000000000..edea96bcd7 --- /dev/null +++ b/.minsky/hooks/calibration-root-tier.test.ts @@ -0,0 +1,114 @@ +/** + * mt#4885 — the two calibration writers put their cwd-derived root in the `fallbackCwd` + * tier, not `projectDir`. + * + * **Why a dedicated file rather than an assertion in each writer's own suite.** The existing + * suites (`gate-walk-provenance.test.ts`, `require-execution-evidence-before-merge.test.ts`, + * 264 tests between them) pass IDENTICALLY before and after the fix, because they run with + * `CLAUDE_PROJECT_DIR` unset — and with an empty top tier, `projectDir: root` and + * `fallbackCwd: root` resolve to the same path. A test that cannot fail against the defect is + * not evidence about it (mem#704), so the discriminating condition has to be constructed: + * `CLAUDE_PROJECT_DIR` SET, to a directory that is not the one the caller passes. + * + * Against the pre-fix code every assertion below inverts — the record lands under the passed + * root because `projectDir` outranks `CLAUDE_PROJECT_DIR`. That is the negative control, and + * it is recorded in the PR body. + */ +/* eslint-disable custom/no-real-fs-in-tests -- The assertion under test IS the on-disk location a writer resolves to. Injecting a mock fs would move the writes into a fake filesystem and leave the real path-resolution ladder — the only thing this file exists to pin — unexercised, so the rule's remedy would defeat the test. Blast radius is bounded to per-test `mkdtemp` directories under the OS temp dir, with `MINSKY_STATE_DIR` redirected so nothing touches the operator's real calibration streams. */ +import { describe, it, expect, beforeEach, afterEach } from "bun:test"; +import { mkdtempSync, rmSync, readFileSync, existsSync } from "fs"; +import { tmpdir } from "os"; +import { join } from "path"; + +/** Env var names, extracted so the ladder's two tiers are named once each. */ +const STATE_DIR_ENV = "MINSKY_STATE_DIR"; +const PROJECT_DIR_ENV = "CLAUDE_PROJECT_DIR"; + +import { calibrationLogPath } from "./dispatcher"; +import { appendCalibrationRecord } from "./gate-walk-provenance"; +import { + appendAtCoverageCalibration, + AT_COVERAGE_STREAM, +} from "./require-execution-evidence-before-merge"; + +/** + * `gate-walk-provenance.ts` keeps its stream name module-private, so it is spelled here rather + * than exported purely for this test. A rename there makes these cases fail (the record is not + * found under either root), which is the correct signal — not a silent pass. + */ +const GATE_WALK_STREAM = "gate-walk-provenance"; + +/** + * Two roots that cannot collapse to one key. Both live under the OS temp dir, which has no + * `.git` above it, so `findRepoRoot` degrades to its input unchanged (the documented behavior + * `calibrationLogPath`'s docblock relies on for synthetic paths) and the sha256 keys differ. + * + * `authoritativeRoot` stands in for the real project — what `CLAUDE_PROJECT_DIR` names. + * `callerRoot` stands in for the session-workspace clone a guard's `findRepoRoot(input.cwd)` + * actually returns. + */ +let stateDir: string; +let authoritativeRoot: string; +let callerRoot: string; +let priorStateDir: string | undefined; +let priorProjectDir: string | undefined; + +beforeEach(() => { + stateDir = mkdtempSync(join(tmpdir(), "mt4885-state-")); + authoritativeRoot = mkdtempSync(join(tmpdir(), "mt4885-project-")); + callerRoot = mkdtempSync(join(tmpdir(), "mt4885-clone-")); + + priorStateDir = process.env[STATE_DIR_ENV]; + priorProjectDir = process.env[PROJECT_DIR_ENV]; + process.env[STATE_DIR_ENV] = stateDir; + process.env[PROJECT_DIR_ENV] = authoritativeRoot; +}); + +afterEach(() => { + if (priorStateDir === undefined) delete process.env[STATE_DIR_ENV]; + else process.env[STATE_DIR_ENV] = priorStateDir; + if (priorProjectDir === undefined) delete process.env[PROJECT_DIR_ENV]; + else process.env[PROJECT_DIR_ENV] = priorProjectDir; + + for (const d of [stateDir, authoritativeRoot, callerRoot]) { + rmSync(d, { recursive: true, force: true }); + } +}); + +/** Read the one record a writer just appended, resolving the path the READER's own way. */ +function readRecordAt(stream: string, root: string): Record | null { + const path = calibrationLogPath(stream, { projectDir: root }); + if (!existsSync(path)) return null; + const body = readFileSync(path, "utf-8").trim(); + if (body === "") return null; + return JSON.parse(body.split("\n")[0] ?? "{}") as Record; +} + +describe("mt#4885 — cwd-derived roots land in the fallbackCwd tier", () => { + it("gate-walk-provenance: CLAUDE_PROJECT_DIR outranks the caller's findRepoRoot(input.cwd)", () => { + appendCalibrationRecord({ marker: "gate-walk" }, callerRoot); + + // The record belongs to the PROJECT, not the clone the guard happened to run in. + expect(readRecordAt(GATE_WALK_STREAM, authoritativeRoot)?.["marker"]).toBe("gate-walk"); + // The absence half — this is what fails pre-fix, where `projectDir` won. + expect(readRecordAt(GATE_WALK_STREAM, callerRoot)).toBeNull(); + }); + + it("require-execution-evidence: same tier, same outcome", () => { + appendAtCoverageCalibration({ marker: "at-coverage" }, callerRoot); + + expect(readRecordAt(AT_COVERAGE_STREAM, authoritativeRoot)?.["marker"]).toBe("at-coverage"); + expect(readRecordAt(AT_COVERAGE_STREAM, callerRoot)).toBeNull(); + }); + + it("with CLAUDE_PROJECT_DIR unset the caller's root is still used — the tier is a ladder, not a redirect", () => { + // Guards the OTHER direction: demoting the tier must not break the ordinary case where + // nothing outranks it. This is also why the fix does NOT by itself stop the stranding + // mt#4885 documents — that is SC3's separate decision about which root a clone resolves to. + delete process.env[PROJECT_DIR_ENV]; + + appendCalibrationRecord({ marker: "no-project-dir" }, callerRoot); + + expect(readRecordAt(GATE_WALK_STREAM, callerRoot)?.["marker"]).toBe("no-project-dir"); + }); +}); diff --git a/.minsky/hooks/gate-walk-provenance.ts b/.minsky/hooks/gate-walk-provenance.ts index 9c2ebb6703..9f368b4540 100644 --- a/.minsky/hooks/gate-walk-provenance.ts +++ b/.minsky/hooks/gate-walk-provenance.ts @@ -493,15 +493,27 @@ export function appendCalibrationRecord( repoRootDir: string ): void { // mt#4752: the shared helper derives the filename from the stream NAME, so it - // cannot drift from the convention the .gitignore globs encode. Unlike the - // detectors migrated alongside this one, `repoRootDir` here IS authoritative — - // it is resolved by the caller, not a raw shell cwd — so it goes in the - // `projectDir` tier, which outranks `CLAUDE_PROJECT_DIR`. + // cannot drift from the convention the .gitignore globs encode. + // + // mt#4885: `repoRootDir` goes in the `fallbackCwd` tier, NOT `projectDir`. + // This comment previously claimed it "IS authoritative — it is resolved by the + // caller, not a raw shell cwd". That was false: the only caller resolves it as + // `findRepoRoot(input.cwd)`, so it IS a raw shell cwd, merely one that has been + // walked up to a repo root. `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" — because in a + // session workspace `findRepoRoot` returns the CLONE, which is a legitimate repo + // root and the wrong project. Demoting it lets `CLAUDE_PROJECT_DIR` outrank it. + // + // This fixes the TIER only. When `CLAUDE_PROJECT_DIR` is unset the ladder still + // falls through to this value and still keys to the clone — that is a separate + // defect (which root a session workspace SHOULD resolve to), tracked as mt#4885 + // SC3 and deliberately not changed here. // // The former third parameter (`logRelPath`, defaulted to the path literal) is // gone: both call sites used the default, and a path-taking parameter is the // degree of freedom that let a sibling's filename be misspelled (mt#2492). - logCalibrationRecord(CALIBRATION_LOG_NAME, record, { projectDir: repoRootDir }); + logCalibrationRecord(CALIBRATION_LOG_NAME, record, { fallbackCwd: repoRootDir }); } // --------------------------------------------------------------------------- diff --git a/.minsky/hooks/require-execution-evidence-before-merge.ts b/.minsky/hooks/require-execution-evidence-before-merge.ts index 5b75aa839f..dfc28c8002 100755 --- a/.minsky/hooks/require-execution-evidence-before-merge.ts +++ b/.minsky/hooks/require-execution-evidence-before-merge.ts @@ -983,14 +983,22 @@ export function isAtCoverageSkipped(): boolean { * parameter itself is now gone, so neither failure has anywhere left to occur: every call site * names a stream constant, and `logCalibrationRecord` derives the filename from it. * - * `repoRootDir` is authoritative here — the caller resolved it, it is not a raw shell cwd — so it - * goes in the `projectDir` tier, which outranks `CLAUDE_PROJECT_DIR`. + * mt#4885: `repoRootDir` goes in the `fallbackCwd` tier, NOT `projectDir`. This docblock + * previously claimed it was "authoritative here — the caller resolved it, it is not a raw shell + * cwd". That was false: the caller resolves it as `findRepoRoot(input.cwd)`, so it IS a raw shell + * cwd walked up to a repo root. In a session workspace `findRepoRoot` returns the CLONE — a real + * repo root, and the wrong project — which is why `calibrationLogPath`'s docblock reserves the top + * tier for an ALREADY-RESOLVED authoritative directory and says passing a cwd there "would preserve + * the bug through the migration". Demoting it lets `CLAUDE_PROJECT_DIR` outrank it. + * + * TIER only. With `CLAUDE_PROJECT_DIR` unset the ladder still falls through to this value and still + * keys to the clone — a separate defect, tracked as mt#4885 SC3. */ export function appendAtCoverageCalibration( record: Record, repoRootDir: string ): void { - logCalibrationRecord(AT_COVERAGE_STREAM, record, { projectDir: repoRootDir }); + logCalibrationRecord(AT_COVERAGE_STREAM, record, { fallbackCwd: repoRootDir }); } /** Result of fetching a task's spec for the AT-coverage check. */ @@ -1281,7 +1289,7 @@ if (import.meta.main) { ); if (scCoverage.calibrationRecord) { logCalibrationRecord(SC_COVERAGE_STREAM, scCoverage.calibrationRecord, { - projectDir: repoRootDir, + fallbackCwd: repoRootDir, }); } if (scCoverage.warning) { @@ -1306,7 +1314,7 @@ if (import.meta.main) { ); if (testFirst.calibrationRecord) { logCalibrationRecord(TEST_FIRST_STREAM, testFirst.calibrationRecord, { - projectDir: repoRootDir, + fallbackCwd: repoRootDir, }); } if (testFirst.warning) { @@ -1321,7 +1329,7 @@ if (import.meta.main) { const renderPath = runRenderPathCalibration(task, context.prNumber, prFiles, prBody); if (renderPath.calibrationRecord) { logCalibrationRecord(RENDER_PATH_STREAM, renderPath.calibrationRecord, { - projectDir: repoRootDir, + fallbackCwd: repoRootDir, }); } if (renderPath.warning) { @@ -1350,7 +1358,7 @@ if (import.meta.main) { ); if (consumerAccount.calibrationRecord) { logCalibrationRecord(CONSUMER_ACCOUNT_STREAM, consumerAccount.calibrationRecord, { - projectDir: repoRootDir, + fallbackCwd: repoRootDir, }); } if (consumerAccount.warning) { From aa4c03d776e89ac83c9834afd39cf4d02876df43 Mon Sep 17 00:00:00 2001 From: Eugene Dobry Date: Thu, 3 Sep 2026 23:50:57 -0400 Subject: [PATCH 2/2] =?UTF-8?q?fix(mt#4885):=20PR=20#3607=20R1=20=E2=80=94?= =?UTF-8?q?=20retarget=20the=20deferred-defect=20cross-reference=20to=20mt?= =?UTF-8?q?#4954=20[no-deploy-impact]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .claude/hooks/gate-walk-provenance.ts | 4 ++-- .claude/hooks/require-execution-evidence-before-merge.ts | 2 +- .minsky/hooks/gate-walk-provenance.ts | 4 ++-- .minsky/hooks/require-execution-evidence-before-merge.ts | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.claude/hooks/gate-walk-provenance.ts b/.claude/hooks/gate-walk-provenance.ts index a948a18ef4..472e034eda 100755 --- a/.claude/hooks/gate-walk-provenance.ts +++ b/.claude/hooks/gate-walk-provenance.ts @@ -510,8 +510,8 @@ export function appendCalibrationRecord( // // This fixes the TIER only. When `CLAUDE_PROJECT_DIR` is unset the ladder still // falls through to this value and still keys to the clone — that is a separate - // defect (which root a session workspace SHOULD resolve to), tracked as mt#4885 - // SC3 and deliberately not changed here. + // defect (which root a session workspace SHOULD resolve to), tracked as mt#4954 + // and deliberately not changed here. // // The former third parameter (`logRelPath`, defaulted to the path literal) is // gone: both call sites used the default, and a path-taking parameter is the diff --git a/.claude/hooks/require-execution-evidence-before-merge.ts b/.claude/hooks/require-execution-evidence-before-merge.ts index 5b4afb0703..4a5dfb7c92 100755 --- a/.claude/hooks/require-execution-evidence-before-merge.ts +++ b/.claude/hooks/require-execution-evidence-before-merge.ts @@ -995,7 +995,7 @@ export function isAtCoverageSkipped(): boolean { * the bug through the migration". Demoting it lets `CLAUDE_PROJECT_DIR` outrank it. * * TIER only. With `CLAUDE_PROJECT_DIR` unset the ladder still falls through to this value and still - * keys to the clone — a separate defect, tracked as mt#4885 SC3. + * keys to the clone — a separate defect, tracked as mt#4954. */ export function appendAtCoverageCalibration( record: Record, diff --git a/.minsky/hooks/gate-walk-provenance.ts b/.minsky/hooks/gate-walk-provenance.ts index 9f368b4540..040dfaac15 100644 --- a/.minsky/hooks/gate-walk-provenance.ts +++ b/.minsky/hooks/gate-walk-provenance.ts @@ -507,8 +507,8 @@ export function appendCalibrationRecord( // // This fixes the TIER only. When `CLAUDE_PROJECT_DIR` is unset the ladder still // falls through to this value and still keys to the clone — that is a separate - // defect (which root a session workspace SHOULD resolve to), tracked as mt#4885 - // SC3 and deliberately not changed here. + // defect (which root a session workspace SHOULD resolve to), tracked as mt#4954 + // and deliberately not changed here. // // The former third parameter (`logRelPath`, defaulted to the path literal) is // gone: both call sites used the default, and a path-taking parameter is the diff --git a/.minsky/hooks/require-execution-evidence-before-merge.ts b/.minsky/hooks/require-execution-evidence-before-merge.ts index dfc28c8002..a73f5c208f 100755 --- a/.minsky/hooks/require-execution-evidence-before-merge.ts +++ b/.minsky/hooks/require-execution-evidence-before-merge.ts @@ -992,7 +992,7 @@ export function isAtCoverageSkipped(): boolean { * the bug through the migration". Demoting it lets `CLAUDE_PROJECT_DIR` outrank it. * * TIER only. With `CLAUDE_PROJECT_DIR` unset the ladder still falls through to this value and still - * keys to the clone — a separate defect, tracked as mt#4885 SC3. + * keys to the clone — a separate defect, tracked as mt#4954. */ export function appendAtCoverageCalibration( record: Record,