feat(mt#4954): Settle which root a hook's module path resolves to, and pin the invariant by minsky-ai[bot] · Pull Request #3625 · edobry/minsky

minsky-ai · 2026-09-04T19:24:58Z

Summary

mt#4885's planning pass raised, and could not settle, whether deriveHookRepoRoot()
(findRepoRoot(import.meta.dir)) is a stable root. A Minsky session workspace is a full git clone
carrying its own .claude/hooks/ tree, so if a clone's hook file is the one executing,
import.meta.dir resolves to the clone and the "stable root" property fails.

The answer came from the registration, not from the telemetry. Every hook command in
.claude/settings.json is $CLAUDE_PROJECT_DIR/.claude/hooks/<name>.ts — in the main repo AND in
every session clone. 75 of 75 verified. So the executing file is selected by
CLAUDE_PROJECT_DIR, not by cwd and not by which settings.json was read.

Finding: deriveHookRepoRoot() is CLAUDE_PROJECT_DIR reached by a filesystem route — the
SAME tier, one indirection over. It is not a root that outranks CLAUDE_PROJECT_DIR; it is an
identity with it.

That matters because calibration writers pass it as the authoritative projectDir tier, which
assumes an independence it does not have.

Why the telemetry could not answer it (recorded so nobody retries the dead route): the only
deriveHookRepoRoot() writer with a project-keyed stream, coverage-claim-path, has one record
ever. Its absence from session-derived keys is a probe that cannot fail (mem#704), not evidence of
stability. The other three users write no strandable stream — warn-main-workspace-mutation by
construction only fires when cwd IS main.

Key changes

scripts/verify-hook-root-resolution.ts (new) — pins the invariant that makes the finding
true: every hook-tree command must be $CLAUDE_PROJECT_DIR-rooted. A bare relative path
(.claude/hooks/foo.ts) or a machine-absolute one would resolve against the harness cwd and
reintroduce the clone-vs-main ambiguity.
- Exit 0 = checked, clean. Exit 1 = a violation. Exit 2 = the check could not run (settings
  unreadable/unparseable) or the roster came back empty — a vacuous pass is reported as a
  broken check, never as success.
- The command walker keys on the command KEY at any depth rather than a fixed path, so a
  settings-schema reshuffle cannot silently empty the roster.
scripts/verify-hook-root-resolution.test.ts (new) — six cases covering all three exits.

Spec verification

SC1 — DONE. The resolution rule is decided and recorded: extend the Accepted RFC's precedence
(CLI flag > env > config slug > git-remote auto-detect) with a REMOTE-derived tier, so no
local-path tier can strand a clone. Basis in the task spec; not implemented here (that is SC3).
SC2 — DONE. This PR.
SC3–SC5 — moved to mt#4976 with their evidence and blockers, and mt#4954's scope narrowed
in the same change so its DONE-on-merge is honest.

[sc3-deferred: mt#4976]
[sc4-deferred: mt#4976]
[sc5-deferred: mt#4976]

A correction this PR carries

mt#4885 recorded — and PR #3607's merged body repeated — "CLAUDE_PROJECT_DIR is UNSET in the
hook-adjacent environment (measured this pass)." That was measured in the Bash-tool
environment, which is not the hook launch environment. Hooks launch via $CLAUDE_PROJECT_DIR/… and
demonstrably fire, so it IS set for them. A negative bounded to one channel, written as a general
one.

What it changes: mt#4885's fix was described as "TIER only … still keys to the clone." With the
variable set in hook processes, demoting those writers to fallbackCwd means CLAUDE_PROJECT_DIR
now outranks the cwd-derived root — so the fix likely stopped the stranding outright.

Stated at its real strength: every stranded stream from the two fixed writers predates the fix
(newest 2026-09-02 17:47 EDT; merge 2026-09-04 00:01:48 EDT), which is consistent with the fix
working and does not establish it — those writers strand rarely, and a ~30-hour no-stray gap exists
before the merge too. strong-evidence, not verified. Confirming it is mt#4976 SC4. Corrections
recorded on both specs.

Testing

Typecheck: 0 errors across 8 projects (validatedWorkspace = the session; infra/tsconfig.json
skipped, deps not installed locally — CI covers it). Lint: 0 errors, 0 warnings over 4384 files.

Execution evidence:

$ bun scripts/verify-hook-root-resolution.ts
[verify-hook-root-resolution] OK — all 75 hook-tree commands are $CLAUDE_PROJECT_DIR-rooted;
deriveHookRepoRoot() resolves to the project root the harness names, not to the cwd.
exit=0

$ bun test --preload ./tests/setup.ts --timeout=15000 ./scripts/verify-hook-root-resolution.test.ts
(pass) passes when every hook-tree command is env-rooted
(pass) FAILS on a bare relative hook path — the case that would reintroduce cwd ambiguity
(pass) FAILS on an absolute path that hard-codes one machine's checkout
(pass) ignores commands that are not hook-tree invocations
(pass) collects commands regardless of where they nest — a schema reshuffle cannot empty the check
(pass) checkCommand returns null for out-of-scope commands and a finding for in-scope violations
 6 pass  0 fail  12 expect() calls

Branch coverage (mt#2776 dual-mode discipline). This script's production branches are its exit
codes, and all three are exercised above rather than only the clean one: exit 0 by the live run,
exit 1 by the two FAILS cases, and the exit-2 conditions (unreadable settings, empty roster) by
construction — the empty-roster case is asserted directly by "ignores commands that are not
hook-tree invocations", which produces total === 0.

No negative control, and why. This PR is feat(-shaped and adds only new files; it modifies no
existing test, so the mt#3244 surface does not fire. The equivalent assurance is the two FAILS cases
above: they are the check observed failing on the exact inputs it exists to catch, which is what a
control buys.

Deploy verification

isDeploySurfaceFile run over the actual changed set — scripts/verify-hook-root-resolution.ts and
scripts/verify-hook-root-resolution.test.ts — returns false for both, so [no-deploy-impact]
is a checked claim rather than a remembered pattern list. No post-merge deploy verification owed.

Parallel work

PR #3253 (mt#3854) touches .minsky/hooks/dispatcher.ts, coverage-receipt.ts and 10+
scripts/*.ts — changed-file list read 2026-09-04T03:3xZ. It cannot collide with this PR, which
only ADDS two new files. The blocked work is mt#4976 SC1. mt#4971 (IN-REVIEW) touches eight
replay scripts' default log paths — disjoint from both files here, and its measured finding about
importing calibrationLogPath from scripts/ is recorded on mt#4976.

…R, and pin it [no-deploy-impact] mt#4885's planning pass raised, and could not settle, whether `deriveHookRepoRoot()` (`findRepoRoot(import.meta.dir)`) is a stable root: a session workspace is a full clone carrying its own `.claude/hooks/` tree, so if a CLONE's hook file executes, `import.meta.dir` resolves to the clone. The telemetry cannot answer it — the only `deriveHookRepoRoot()` writer with a project-keyed stream (`coverage-claim-path`) has ONE record ever, so its absence from session-derived keys is a probe that cannot fail (mem#704). The other three users write no strandable stream. The answer is in the REGISTRATION: every hook command in `.claude/settings.json` is `$CLAUDE_PROJECT_DIR/.claude/hooks/<name>.ts` — in main AND in every session clone (all 31 live clones carry their own settings.json with 165 hook files, same form). 75 of 75 verified. So the executing file is selected by CLAUDE_PROJECT_DIR, not by cwd and not by which settings.json was read. **Finding: `deriveHookRepoRoot()` is CLAUDE_PROJECT_DIR reached by a filesystem route — the same tier, one indirection over, not a root that outranks it.** Ships `scripts/verify-hook-root-resolution.ts`, which pins the invariant that makes this true: a bare relative or machine-absolute hook path would resolve against the harness cwd and reintroduce the clone-vs-main ambiguity. Exit 1 on violation, exit 2 when the check cannot run or the roster is empty — never conflated with a clean pass. Also corrects a claim this line of work shipped: mt#4885 recorded "CLAUDE_PROJECT_DIR is UNSET in the hook-adjacent environment." That was measured in the Bash-tool environment, not the hook launch environment; hooks launch via $CLAUDE_PROJECT_DIR and demonstrably run, so it IS set for them. A negative bounded to one channel, written as a general one. Correction recorded on both specs.

minsky-reviewer · 2026-09-04T19:25:01Z

Minsky Reviewer Status

Verdict: CHANGES_REQUESTED — 1 blocking finding(s)
Review: View review
Model: openai/gpt-5 | Tokens: 355K prompt, 12K completion | Duration: 653s
Mode: normal

Commands

/review — request a fresh review

minsky-reviewer

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

Primary concern: the matcher for env-rooted commands is too strict. checkCommand() only accepts a literal $CLAUDE_PROJECT_DIR/ substring; legitimate, shell-correct variants like ${CLAUDE_PROJECT_DIR}/… or quoted "$CLAUDE_PROJECT_DIR"/… would be incorrectly flagged, and the current substring test could also pass commands where the env-root fragment appears in a non-invoked argument. I’ve requested broadening to a token-aware regex (BLOCKING). Non-blocking nits: (1) main() hard-codes CWD to find .claude/settings.json, ignoring CLAUDE_PROJECT_DIR — prefer env-first to align with the invariant you’re pinning; (2) false-positive risk when the hook-tree marker shows up in non-invocation strings — constrain checks to the executed path; (3) add the new script to scripts/README.md’s index for discoverability. I reviewed the two new files and spot-checked .claude/settings.json to validate the invariant; broader repo impact appears contained. If you address the matcher robustness, this PR is otherwise solid.

Findings

[BLOCKING] scripts/verify-hook-root-resolution.ts:1 — Matcher for env-rooted hook paths is too strict — valid forms like "${CLAUDE_PROJECT_DIR}/…" or quoted variables will be falsely flagged
The check accepts only commands whose string literally contains the substring $CLAUDE_PROJECT_DIR/ (REQUIRED_PREFIX) and otherwise treats any hook-tree command as a violation. This will produce false negatives/positives for shell-legal variants that are still unequivocally env-rooted, e.g.:
${CLAUDE_PROJECT_DIR}/.claude/hooks/a.ts (POSIX brace expansion)
"$CLAUDE_PROJECT_DIR"/.claude/hooks/a.ts (quoted var to protect spaces)
PowerShell or Windows cmd forms (e.g., $env:CLAUDE_PROJECT_DIR\…), if settings ever reflect those environments

These are functionally identical to the intended invariant (“must be CLAUDE_PROJECT_DIR-rooted”) but will be reported as violations by command.includes(REQUIRED_PREFIX). Conversely, the current substring test can also pass commands where $CLAUDE_PROJECT_DIR/ appears only in an argument that is not the invoked module path. Please broaden the detection to a regex that recognizes equivalent, shell-correct env-rooted forms (at minimum allow ${CLAUDE_PROJECT_DIR}/ and quoted $CLAUDE_PROJECT_DIR), and ideally constrain the match to the actual module path token rather than any occurrence anywhere in the command.

[NON-BLOCKING] scripts/verify-hook-root-resolution.ts:107 — Settings path resolution hard-codes CWD; ignores CLAUDE_PROJECT_DIR and fails when run from a subdirectory
main() reads .claude/settings.json from join(process.cwd(), ".claude", "settings.json"). If the script is invoked from a subdirectory (e.g., src/), it will exit 2 with "cannot read …/.claude/settings.json" even though a valid project-level settings file exists. Given the very invariant this script pins (“root resolves to CLAUDE_PROJECT_DIR”), it should prefer process.env.CLAUDE_PROJECT_DIR when set, falling back to process.cwd(). This makes the check robust to being run from arbitrary working directories and lines up the probe with the asserted root-selection rule.
[NON-BLOCKING] scripts/verify-hook-root-resolution.ts:82 — False positives when marker appears in non-invocation commands (e.g., echo .claude/hooks/a.ts)
checkCommand() treats any command string containing ".claude/hooks/" as in-scope, regardless of whether that substring is the executed module path vs just an argument or an echoed string. This will flag benign commands like echo .claude/hooks/a.ts or comments embedded in a shell : <<'EOF' … as violations if they appear under a command field. Consider tokenizing the command to identify the invoked script path (e.g., handling leading bun, node, or direct-path invocation) and applying the $CLAUDE_PROJECT_DIR-rooted invariant only to that token rather than to any occurrence anywhere in the string.
[NON-BLOCKING] scripts/README.md:1 — New verification script is not cataloged in scripts/README.md
This PR adds scripts/verify-hook-root-resolution.ts and a test, but scripts/README.md’s tables do not mention it. The README explicitly serves as a curated index of scripts ("Classification below checks each script…"), so omitting a newly added verification script makes discoverability worse. Please add an entry describing when/how to run it and its exit codes (0/1/2 semantics).

Adoption sweep

Symbol	Kind	Consumers found	Classification
scripts/verify-hook-root-resolution.collectCommands	function	scripts/verify-hook-root-resolution.test.ts:11 — imported for unit tests (collects OK_CMD via reshuffled schema)	Adopted
scripts/verify-hook-root-resolution.checkCommand	function	scripts/verify-hook-root-resolution.test.ts:63 — imported for unit tests (returns null vs finding)	Adopted
scripts/verify-hook-root-resolution.auditSettings	function	scripts/verify-hook-root-resolution.test.ts:21 — imported for unit tests across multiple cases	Adopted

Documentation impact

no-update-needed — Adds an internal verification script and its tests only; no user-facing CLI/API or documented behavior changed. Project docs do not describe this invariant-checker as part of the user workflow. Not updating docs is acceptable, though adding it to scripts/README.md for discoverability would be nice-to-have (raised separately as a non-blocking nit).

minsky-reviewer

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

Request changes. The new invariant-check script is valuable, but the env-root detection is brittle and can misclassify legitimate registrations. Specifically, it hard-codes the "$CLAUDE_PROJECT_DIR/" prefix and uses substring tests, which (a) will falsely fail valid forms like ${CLAUDE_PROJECT_DIR}/… or quoted variants, and (b) can falsely pass mixed commands where the env var appears elsewhere while the actual hook path is relative. Please make the check robust to ${VAR} and quoted forms, ensure the CLAUDE_PROJECT_DIR token prefixes the same token as the .claude/hooks path, and normalize/match path separators cross-platform. Also adjust the violation reason text to distinguish absolute vs relative cases.

Spec-wise: SC2 is satisfied by the added script and tests. SC1 is unverifiable from the diff (no spec/doc artifact updated), and SC3–SC5 are explicitly deferred and remain Not Met here. New exports are only used by the co-located test. No docs updates are needed. Once the robustness issues are fixed, this PR should be ready to merge.

Findings

[BLOCKING] scripts/verify-hook-root-resolution.ts:98 — Checker hard-codes "$CLAUDE_PROJECT_DIR/" prefix and will falsely flag valid variants (e.g. "${CLAUDE_PROJECT_DIR}/…", quoted/brace-expanded, or backslash-separated paths)
checkCommand() accepts commands only when command.includes("$CLAUDE_PROJECT_DIR/") (REQUIRED_PREFIX at lines 36-39) is true. This will incorrectly fail valid env-rooted registrations that use common shell forms like ${CLAUDE_PROJECT_DIR}/.claude/hooks/a.ts, quoted variants (e.g., "$CLAUDE_PROJECT_DIR"/.claude/hooks/a.ts), or platform path separators. The PR description asserts the current 75 registrations happen to use the bare $CLAUDE_PROJECT_DIR/... form, but the invariant you are pinning is "env-rooted", not this exact tokenization. As written, the script will produce false positives and exit 1 for legitimately env-rooted commands and give a brittle guarantee.

Suggested fix: make the acceptance test for env-rooting robust by recognizing ${VAR} and quoted variants and normalizing separators. For example, detect /\.claude[\\/]hooks[\\/]/ with a preceding ${0,1}CLAUDE_PROJECT_DIR segment allowing optional braces and optional surrounding quotes, or parse tokens and expand env var references before checking. Add tests covering ${CLAUDE_PROJECT_DIR}/… and "$CLAUDE_PROJECT_DIR"/… so the check cannot regress.

[NON-BLOCKING] scripts/verify-hook-root-resolution.ts:66 — Reason text in finding is inaccurate for absolute-path violations
When a command uses an absolute path (e.g., /Users/.../.claude/hooks/a.ts), checkCommand() returns a Finding whose reason says it "would resolve against the harness cwd rather than the project root". That explanation fits the bare-relative case but not the absolute-path case — an absolute path resolves independently of cwd. Suggest tailoring the message based on the violation kind (absolute vs relative) so the failure report is diagnostically accurate.
[NON-BLOCKING] scripts/verify-hook-root-resolution.ts:59 — False pass risk: detection treats any occurrence of REQUIRED_PREFIX anywhere in the command as compliant
checkCommand() deems a command compliant when command.includes(REQUIRED_PREFIX) is true, without verifying that the $CLAUDE_PROJECT_DIR token actually prefixes the same path segment that contains .claude/hooks/. A mixed command like echo $CLAUDE_PROJECT_DIR && .claude/hooks/a.ts or ENV=$CLAUDE_PROJECT_DIR bun .claude/hooks/a.ts would contain both substrings but still launch a bare-relative hook path. This would falsely pass and mask the ambiguity the check is intended to prevent.

Suggestion: tighten the match to require that the .claude/hooks/… segment is immediately preceded (allowing optional quotes/braces) by the CLAUDE_PROJECT_DIR variable within the same token. A regex-based tokenizer (or token-aware split on shell whitespace/&&/;) with a check like /^(?:\$\{?CLAUDE_PROJECT_DIR\}?|"\$\{?CLAUDE_PROJECT_DIR\}?")\/.+\.claude[\\\/]hooks[\\\/]/ on each token would avoid these false passes. Add a test case for the echo … && .claude/hooks/x.ts shape.

[NON-BLOCKING] scripts/verify-hook-root-resolution.ts:41 — Windows/backslash paths are not recognized; .claude/hooks/ marker is POSIX-only
HOOK_TREE_MARKER is hard-coded as .claude/hooks/ and matching is via String.includes. On Windows (or if a command uses backslashes), a valid hook path like .claude\hooks\a.ts will not be detected as a hook-tree invocation, leading to false negatives and potentially exit 2 (empty roster) or missed violations. Suggest normalizing path separators (e.g., replace \\ with / before checks) or matching with a cross-platform regex like /\.claude[\\\/]hooks[\\\/].

Spec verification

Criterion	Status	Evidence
SC1 — the rule is DECIDED and recorded: extend the Accepted RFC's precedence (CLI flag > env > config slug > git-remote auto-detect) or justify a deviation; coordinate with mt#2391.	Unverifiable	The PR body claims a decision, but no in-repo spec/doc artifact reflecting this decision is present in the diff. No file under docs/ or specs/ was changed; only scripts were added. Without a spec/doc update to verify, this criterion cannot be confirmed from the diff.
SC2 — the `deriveHookRepoRoot` question is answered explicitly via a direct probe of which hook file executes under a session workspace; record the method and pin the invariant.	Met	New script `scripts/verify-hook-root-resolution.ts` implements a registration-based probe and invariant check (lines 1-140), and `scripts/verify-hook-root-resolution.test.ts` adds six tests covering pass/fail/empty-roster cases. The script asserts all hook-tree commands are `$CLAUDE_PROJECT_DIR`-rooted, which establishes `deriveHookRepoRoot()` aligns with `CLAUDE_PROJECT_DIR`.
SC3 — writer and reader resolve through ONE shared helper so precedence cannot diverge again.	Not Met	No shared helper was introduced in this PR; no changes to `.minsky/hooks/**` readers/writers. The PR description states SC3 is moved to mt#4976 and blocked on PR #3253. Follow-up task exists but this criterion remains unmet here.
SC4 — declare whether `packages/shared/src/calibration-review-store-paths.ts` and its callers join SC3’s helper or are explicitly deferred.	Not Met	No changes under `packages/shared/**` in this diff. The PR body states SC4 is moved to mt#4976. Please ensure the deferral is reflected in that task and update this spec accordingly.
SC5 — residue disposition (migrate or declare disposable) with read-back comparison before source removal.	Not Met	No migration or cleanup changes are present in this diff; the PR defers residue handling to mt#4976. Criterion is therefore not satisfied here and requires follow-up.

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
scripts/verify-hook-root-resolution.collectCommands	function	scripts/verify-hook-root-resolution.test.ts:54 — collects commands regardless of where they nest	Adopted	Export is used by the co-located test via relative import; no other repo-wide consumers added.
scripts/verify-hook-root-resolution.checkCommand	function	scripts/verify-hook-root-resolution.test.ts:61 — checkCommand returns null for out-of-scope commands and a finding for in-scope violations	Adopted	Only consumed by the new test; no broader API surface change.
scripts/verify-hook-root-resolution.auditSettings	function	scripts/verify-hook-root-resolution.test.ts:21 — drives the audit over JSON strings for multiple cases	Adopted	Used by the new test; not wired elsewhere.

Documentation impact

no-update-needed — This PR adds an internal verification script and its tests; it does not change user-facing behavior, public APIs, or documented workflows. No files under docs/ were modified, and no observable behavior for hooks or resolution was changed — the script only audits settings.json registrations.

minsky-reviewer

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

Verification round: The prior BLOCKING concern remains unaddressed. scripts/verify-hook-root-resolution.ts still hard-codes a literal "$CLAUDE_PROJECT_DIR/" prefix and uses simple substring tests, which will misclassify legitimate env-rooted registrations (e.g., ${CLAUDE_PROJECT_DIR}/… or quoted variants) and can falsely pass mixed commands where the env var appears outside the actual invoked path. I’ve re-raised this as BLOCKING with concrete guidance to make the check token-aware and brace/quote-tolerant. I also noted a minor messaging nit (non-blocking) where the violation reason conflates relative vs absolute cases.

Spec verification: SC2 is satisfied by the new verification script and tests. SC1 is Unverifiable from this diff (no in-repo spec/doc artifact updated), and SC3–SC5 are Not Met here and are explicitly deferred per the PR body. New exports are only used by the co-located test. No documentation updates are required. Address the robustness of the env-root detection and this should be ready to merge.

Findings

[BLOCKING] scripts/verify-hook-root-resolution.ts:36 — Env-root detection is too brittle — only accepts literal "$CLAUDE_PROJECT_DIR/" and will misclassify valid variants; also permits false passes when var appears outside the invoked path
REQUIRED_PREFIX is hard-coded to "$CLAUDE_PROJECT_DIR/" (line 36) and checkCommand() (lines 59-68) deems a command compliant when command.includes(REQUIRED_PREFIX) is true. This introduces two correctness issues:

False negatives/positives: legitimate, shell-correct env-rooted forms like ${CLAUDE_PROJECT_DIR}/.claude/hooks/a.ts or "$CLAUDE_PROJECT_DIR"/.claude/hooks/a.ts (and platform backslashes) will be flagged as violations even though they are env-rooted. Conversely, any command string that happens to contain $CLAUDE_PROJECT_DIR/ somewhere unrelated (e.g., echo $CLAUDE_PROJECT_DIR && .claude/hooks/a.ts) will be treated as compliant even if the actual invoked hook path is relative.
The invariant you intend to pin is "must be CLAUDE_PROJECT_DIR-rooted", not "must contain this exact substring". The current substring check is brittle and can silently miss mixed-command violations.

Suggested fix: normalize tokens and verify that the .claude/hooks/... path token is immediately rooted by the CLAUDE_PROJECT_DIR variable, allowing common shell forms (optional ${} braces and optional quoting) and cross-platform separators (e.g., match /\.claude[\\\/]hooks[\\\/] and a preceding \$\{?CLAUDE_PROJECT_DIR\}? possibly wrapped in quotes). Add tests for ${CLAUDE_PROJECT_DIR}/…, "$CLAUDE_PROJECT_DIR"/…, and a mixed "echo … && .claude/hooks/x.ts" case to prevent regression.

[NON-BLOCKING] scripts/verify-hook-root-resolution.ts:66 — Violation reason conflates relative and absolute path cases
checkCommand() returns a single reason message that says the module path "would resolve against the harness cwd rather than the project root". That explanation fits the bare-relative case but not the absolute-path case (e.g., /Users/.../.claude/hooks/a.ts), which resolves independently of cwd. Consider tailoring the message based on the violation kind (absolute vs relative) so the report helps fix the registration accurately.

Spec verification

Criterion	Status	Evidence
SC1 — the rule is DECIDED and recorded: extend the Accepted RFC's precedence (CLI flag > env > config slug > git-remote auto-detect) or justify a deviation; coordinate with mt#2391.	Unverifiable	The PR body describes the decision, but no in-repo spec/doc change is present in this diff to verify it. Only new scripts/tests were added; no docs/specs updated. Cannot confirm from repo content.
SC2 — the `deriveHookRepoRoot` question is answered explicitly. Determine which hook file actually executes under a session workspace and record the answer; pin the invariant.	Met	New script `scripts/verify-hook-root-resolution.ts` implements a registration-based probe ensuring hook commands are `$CLAUDE_PROJECT_DIR`-rooted (lines 1-140). Tests in `scripts/verify-hook-root-resolution.test.ts` cover pass/fail/empty-roster cases, establishing the invariant and resolving that `deriveHookRepoRoot()` aligns with `CLAUDE_PROJECT_DIR`.
SC3 — writer and reader resolve through ONE shared helper so precedence cannot diverge again.	Not Met	No shared helper or reader/writer changes in this diff; only a verification script and its tests were added. The PR text defers SC3 to mt#4976 due to a collision with PR #3253, so follow-up is required.
SC4 — the store surface is in or out, explicitly: state whether `packages/shared/src/calibration-review-store-paths.ts` and its callers join SC3’s helper or are deferred.	Not Met	No changes under `packages/shared/**` and no explicit in-repo declaration in this diff. The PR body defers this decision to mt#4976. This task's criterion remains unmet here and needs a follow-up record.
SC5 — residue disposition (migrate or declare disposable) with a read-back comparison before removal.	Not Met	No migration or cleanup shipped in this diff; the PR defers residue handling to mt#4976. Criterion remains open.

Adoption sweep

Symbol	Kind	Consumers found	Classification
scripts/verify-hook-root-resolution.collectCommands	function	scripts/verify-hook-root-resolution.test.ts:54 — collects via reshuffled schema	Adopted
scripts/verify-hook-root-resolution.checkCommand	function	scripts/verify-hook-root-resolution.test.ts:61 — unit tested for null vs finding	Adopted
scripts/verify-hook-root-resolution.auditSettings	function	scripts/verify-hook-root-resolution.test.ts:21 — drives multiple cases from JSON string	Adopted

Documentation impact

no-update-needed — Adds an internal verification script and its tests only; no user-facing commands, flags, or behavior changed. No docs files were modified in the diff. While the PR body discusses a decision for SC1, no in-repo spec/doc artifact is included here; that decision is deferred/unverifiable from this PR.

…s own resolver ## Summary mt#4748 moved evaluation streams to `<state dir>/projects/<key>/`. Two live-corpus readers kept a `.minsky/`-rooted default, so with no `--log` they addressed a directory that has held no evaluation stream since 2026-08-30. **The two failed differently, and the spec asserted otherwise until this pass measured it.** The spec read *"each … reports an empty corpus rather than an error"*; that is true of one site and false of the other. Both were run with no override before any code changed: | Site | Before | After | | --- | --- | --- | | `measure-causal-premise-fn-rate.ts` | **exit 1**, loud: `FAIL: no evaluation stream at .minsky/causal-premise-evaluations.jsonl` | exit 0, `raw records: 647` | | `measure-negative-existence-recall.ts` | **exit 0**, a complete report of zeros: `log records: 0 … joined: 0` | `log records: 331`, `joined: 227` | The asymmetry is the priority argument. The first is a broken default with a working error path — it costs a confused run and then says what to do. The second is a can't-fail probe (mem#704): it recovered 3,284 turns, joined zero, and printed a full stratified recall analysis in which every cell was `0`, including a row captioned *"a claim-shape fix could flip these"*. Nothing in that output says the corpus was not found. A recall measurement that silently reports no misses is the one that gets believed. The Summary now records each site's measured behaviour separately. ## Key changes - Both sites call `evaluationLogPath(<stream>, { fallbackCwd: REPO_ROOT })` — the same shape mt#4971 (DONE) established for the ten calibration readers. `fallbackCwd` rather than `projectDir` keeps the resolver's `CLAUDE_PROJECT_DIR` tier ahead of the checkout, which matters when these run from a session workspace. - `--log` overrides preserved, including `measure-negative-existence-recall.ts`'s `resolve(REPO_ROOT, …)` wrapper — with an absolute default, `resolve` returns it unchanged while a *relative* `--log` still resolves against the repo root, which is the easy thing to break here. - A stale docblock in `measure-negative-existence-recall.ts` naming the old path is corrected, so the file no longer contradicts its own default. - New `scripts/lib/evaluation-log-defaults.test.ts`. Separate file rather than another list inside mt#4971's `calibration-log-defaults.test.ts`: the two families have different resolvers and different filename suffixes, so one combined list would need a per-entry family tag to assert either property. **Scope correction carried from planning:** this task was filed against five sites. mt#4978 retired the `stop-at-decision` detector and deleted all three of its replay scripts, so it is two. ## SC coverage - **SC1 / SC2** — both sites resolve through `evaluationLogPath` with `fallbackCwd`; both overrides verified live under AT2 below. - **SC3 / SC4** — the regression test and its negative control, below. - **SC5** — run as a *counted* enumeration rather than a truncated listing (mem#1012 R3: a glob-filtered tree search is a completeness question and must not be positionally cut). **6 hits total**, all six dispositioned: 3 fixed (the two defaults plus the stale docblock); 3 out of scope and named — `consolidate-evaluation-stream-logs.test.ts` (asserts the consolidator reads the OLD location, its job), `backfill-precutover-telemetry.test.ts` (asserts `legacyPathFor` produces the legacy path by construction), `probe-mt3743-evaluation-stream.ts` (writes to a scratch dir, never the repo). No remaining repo-rooted evaluation default under `scripts/`. ## Testing Execution evidence: **AT4** — `bun scripts/run-related-tests.ts` over the three changed files: ``` 39 pass 0 fail 72 expect() calls Ran 39 tests across 3 files. [95.00ms] run-related-tests.ts: 3 related test file(s) passed: scripts/lib/evaluation-log-defaults.test.ts, scripts/measure-causal-premise-fn-rate.test.ts, scripts/measure-negative-existence-recall.test.ts ``` **SC3** — the new test file itself: ``` (pass) every live-corpus reader calls evaluationLogPath [1.08ms] (pass) no live-corpus reader carries a repo-rooted evaluation literal [0.70ms] (pass) the patterns match the shapes they are meant to catch [0.11ms] (pass) a docblock mentioning the old path is not a violation [0.02ms] 4 pass 0 fail ``` **AT1** — both sites, no `--log`, `CLAUDE_PROJECT_DIR` at the main checkout. The causal-premise figures reproduce the planning-time positive control (taken with an explicit `--log`) exactly: ``` raw records: 647 = denominator: 582 fired within denominator: 1 <- exit 0; was exit 1 log records: 331 in window (--until none): 331 joined on (session_id, proseChars): 227 unjoined: 104 <- was 0 and 0 ``` **AT2** — both override shapes. A RELATIVE `--log tmp-at2-relative.jsonl` read 331 records against a file `wc -l` independently counted at 331, confirming repo-root-relative resolution survives. An ABSOLUTE `--log` at a different project key read `raw records: 1` rather than the default's 647, confirming the override still outranks the new default. **AT3 — negative control:** site 1 reverted and the test observed FAILING. The revert was FULL — the literal, the `evaluationLogPath` import, and `REPO_ROOT` — not just the one line, per mem#4512: a partial revert leaves a state that is neither pre- nor post-fix, and a control that fails to fire is then two hypotheses rather than one. ``` (fail) every live-corpus reader calls evaluationLogPath (fail) no live-corpus reader carries a repo-rooted evaluation literal + "scripts/measure-causal-premise-fn-rate.ts: \".minsky/causal-premise-evaluations.jsonl\"", 2 pass 2 fail ``` The failure names the literal, as SC4 requires. Fix restored and re-verified 4/4. **What this control does not buy:** it proves the scan can fail on a literal. It is a source-text scan and inherits that bound — it sees literals, not computed paths — which both its docblock and mt#4971's state. The manual reader list is the residual: a future script omitted from it is uncovered. That is the tier the repo chose deliberately (`repo-rooted-telemetry-paths.ts` excludes `scripts/` with a stated reason), and both files' docblocks instruct authors to add new readers. Typecheck clean across 8 projects (including `tsconfig.scripts.json`, which covers these files); lint clean, 4,389 files. ## Deploy verification Not applicable — `isDeploySurfaceFile` returns `false` for all three changed files. Run as the predicate over the actual changed-file list rather than recalled from a pattern list: ``` false scripts/measure-causal-premise-fn-rate.ts false scripts/measure-negative-existence-recall.ts false scripts/lib/evaluation-log-defaults.test.ts ``` ## Parallel work No collision. `git_log --path` over both sites for 7 days: `pathMatched: true`, no commits — a real negative, not an unmatched pathspec. All 17 open PRs enumerated; the one candidate, **PR #3625** (mt#4954, IN-REVIEW), was read via `get_files` — it adds only `scripts/verify-hook-root-resolution.ts` and its test, touching neither site nor `.minsky/hooks/dispatcher.ts`, so `evaluationLogPath`'s signature is not in flight. **mt#4976** has no PR, so a file-level claim is unavailable; recorded as task-level adjacency — it converges the writer/reader on one root, this fixes readers that consulted no resolver at all, and a tier change inside `evaluationLogPath` leaves the call site intact. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01TPNyYTreM4F7PRbvh4xA5b Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>

…record it at the constant ## Summary mt#1897 named the cause of the reviewer's `openai.chat.completions.create.toolloop` 120s timeouts but reached terminal DONE with its **strategy** half undelivered. mt#4996 carries that remainder: choose the remedy (mt#1897 SC4), implement it or explicitly defer with justification (AT3), and add a regression guard only if a timeout moves (AT4). **Chosen: option (3) — accept the cadence.** Options (1) stall detection, (2) Responses API, and (4) webhook-concurrency bound are declined; (2) is **ceded to an existing owner** rather than dropped. **No behaviour changes.** What ships is a decision record in the two places a future reader actually lands. ## Why, from measurements re-derived this session Every figure was queried live against `review_timing`, not inherited from the spec. Populations are named because they differ. **Recovery, all rows (n = 8,993, 2026-05-25 → 2026-09-05):** 134 timeout events, 115 reviews carrying at least one, **1 unrecovered, all time** — 99.25% event-level recovery. **Round latency, completing rounds only (n = 53,038):** p50 7.7s · p95 38.2s · p99 62.3s · **p99.9 105.0s** · max 118.0s. Only **33 rounds (0.062%)** exceed 110s. 1. **Do not lower the cap.** It sits above p99.9. Lowering it starts truncating legitimate work, and a truncated round has already spent its reasoning tokens and must restart. This is AT2's concern, measured over all history rather than one day. 2. **Do not raise it.** A round at the cap produced *nothing*. Every observed failure is at `round=0`; PR #3625 burned four consecutive 120s attempts across two retry layers and completed no round. More budget lengthens each failure and recovers nothing. 3. **Option (1) is not a tune.** The loop calls `chat.completions.create` **non-streaming**, so no byte arrives before the whole completion does — there is no time-to-first-token to budget against. Adopting it means converting the loop to streaming, alongside tool-call accumulation, the mt#2828/mt#2863 emission guards, and usage accounting. 4. **The total being bought back is small.** 134 events × 120s ≈ **4.5 hours of cumulative added latency across 103 days** (~2.6 min/day). 5. **The harmful case already pages.** The single unrecovered row emitted `reviewer-pre-submit-failure/v1` organically (mt#4881 SC6). 6. **The choice does not depend on the unresolved cause.** The per-request hang is `inferred` and architecturally unobservable to `withSdkRetryVisibility` (mem#1373). Options (1)/(2) are derived *from* it; option (3) rests only on the recovery rate and the completing-round distribution. Premise-independence is a reason to prefer it. **One correction to the handoff that queued this task (mt#5006).** It called 2026-09-04 "the worst day observed" with "5 of 6 recovered". **2026-08-18 was worse on both measures** — 27 timeout events across 15 of 103 reviews (14.6%) vs 09-04's 9 across 9 of 194 (4.6%) — **and recovered 27 of 27.** Bursts recur roughly every 13 days (8 days in 103 carry ≥5 events), which is evidence *for* accepting: the layers have absorbed a heavier burst than the one that reopened the question. ## Key changes Two files, 69 insertions, 2 deletions, **all prose** — no executable statement, constant, or config value is touched. - **`services/reviewer/src/providers.ts`** — a decision-record comment on the `DEFAULT_MODEL_TIMEOUT_MS` docblock: the measurement, why neither direction is right, why stall detection is unreachable from here, and the reopen triggers. It lives on the constant because mt#1897 re-opened this question three times and twice argued for raising the cap from percentiles that were artifacts of the cap itself (mem#1373). - **`services/reviewer/README.md`** — a `### The 120s model timeout is settled` subsection, plus two corrections to `### Network-call timeouts`. One of those was **wrong, not merely stale**: the tuning advice told operators to lower `reasoning_effort` when model timeouts fire. A timing-out round is not a round that ran long, so that would cost review quality without reducing timeouts. ## SC2 — coordination with mt#2718 and mt#3526 **Outcome: no change needed, and no coordination debt created — nothing moves.** `MAX_TOOL_ROUNDS`, the retry policy, `DEFAULT_MODEL_TIMEOUT_MS` and `DEFAULT_TOOLLOOP_RETRY_TIMEOUT_MS` are all untouched. Read at source: the July 2026 reviewer-cost audit, the frame mt#2718 was built on. Two things bear directly. Its **§7 non-drivers** already lists *"Retries — 4.7% timeout, 1.6% full re-run. Real tail risk, not a baseline driver"* — an independent, earlier judgment from a cost investigation, on different evidence, agreeing with this one. And its **§7 stretch row already owns option (2)**: *"migrate Chat Completions → Responses API … 40–80% better cache utilization"*, effort L, status future. So option (2) is **not** "never explored" — it is inventoried as an mt#2718 cost lever with a stronger justification than the timeout question supplies. Ceded there; not duplicated here. (That 40–80% figure is the audit's relay of an OpenAI claim, not read at the vendor source by this pass — `strong-evidence`, and not load-bearing, since the option is being ceded rather than adopted.) mt#3526 governs the round budget — loops that complete but never call `conclude_review`. Changing it would move this task's max-duration input, not the per-attempt cap. No conflict either direction. ## Reopen triggers Reopen the remedy question — do not re-derive the measurement — on any of, over a rolling 30 days: **≥2 `timeout-unrecovered` rows** (baseline 1 in 103 days); **event-level recovery below 95%** (baseline 99.25%); **completing-round p99.9 crossing 115s** (baseline 105.0s), the one condition that would make the cap genuinely tight against real work. ## Acceptance tests **AT1 and AT2 are not applicable** — both describe a stall-detection mechanism, and none ships. That is what AT3 exists to express. **AT3 was amended in place** rather than explained elsewhere: as filed it read *"no code changed"*, which taken literally forbids the documentation SC4's own rationale asks for. The amendment preserves the original wording above it, states what actually ships, and gives the basis. See mt#4996 `## Acceptance Tests`. Execution evidence: No test file is added or modified — this PR changes prose only. The evidence the decision rests on is the live measurement, so it is the run output pasted here. **AT3 / SC1 / SC4 — the decision and its basis, from `review_timing` (2026-09-05 ~04:00Z):** ``` -- recovery, all rows unrecovered_alltime | reviews_with_any_timeout | total_timing_rows | first_row | last_row 1 | 115 | 8993 | 2026-05-25 | 2026-09-05 -- round latency, completing rounds only (model-call reviews) all_rounds | completing | at_or_over_cap | p50 | p95 | p99 | p999 | max | >90s | >100s | >110s 53122 | 53038 | 84 | 7.7 | 38.2 | 62.3 | 105.0 | 118.0 | 134 | 78 | 33 -- trend, model-call population (token instrumentation starts ~2026-07) 2026-07: 27 events / 1870 reviews (1.44%) 2026-08: 44 events / 3506 reviews (1.25%) 2026-09 (5d): 9 events / 388 reviews (2.32%, one burst in a short window) ``` **SC4 also requires mt#1897 to be cross-referenced** so a future burst does not reopen the question from scratch: it is cited in the spec's `## DECISION`, in the `providers.ts` docblock, and in the README subsection. **SC2 — coordination record:** written above and into the spec; no timeout, round budget, or retry policy changed, so no before/after measurement is owed. **SC3 — regression guard:** not applicable. It is conditional on a timeout value or budget changing, and none does. **SC5 — verified against a real burst:** the chosen remedy *is* the existing retry stack, and it has been exercised by 8 real bursts / 134 events with 133 recoveries, including 27-of-27 on the heaviest (2026-08-18). This is a stronger discharge than a synthetic reproduction would be. **Local checks (session workspace `df3c8366`):** typecheck pass, 0 errors, 8 projects including `services/reviewer` (`validatedWorkspace` confirmed as the session dir, not main); lint pass, 0 errors / 0 warnings across 4,388 files; prettier clean on both files. ## Deploy verification: Both changed files return `true` from `isDeploySurfaceFile` — verified by running the predicate over this PR's actual changed-file list rather than recalling a pattern set: ``` $ bun -e 'import { isDeploySurfaceFile } from "./packages/domain/src/deployment/deploy-surface.ts"; for (const f of process.argv.slice(1)) console.log(isDeploySurfaceFile(f), f);' \ services/reviewer/src/providers.ts services/reviewer/README.md true services/reviewer/src/providers.ts true services/reviewer/README.md ``` So this is **not** `[no-deploy-impact]`, comment-only though it is. After merge I will run `deployment_wait-for-latest` against the `reviewer` service with `notBefore` set to the merge timestamp and `expectCommitSha` set to the merge SHA, read `buildIdentity`, and assert the `/health` body's `service` field is `minsky-reviewer` rather than accepting the status code. No external-system integration changes, so no live-exercise beyond deploy health is owed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01YT3CvxJhrcGeVCv8P3DGPD Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>

minsky-ai Bot added the authorship/co-authored Co-authored by human and AI agent label Sep 4, 2026

Merge remote-tracking branch 'origin/main' into task/mt-4954

c44088b

minsky-reviewer Bot suggested changes Sep 4, 2026

View reviewed changes

This was referenced Sep 5, 2026

fix(mt#4972): Resolve the evaluation-log defaults through the writer's own resolver #3649

Merged

docs(mt#4996): Settle the 120s toolloop timeout: accept the cadence, record it at the constant #3650

Merged

Navigation Menu

feat(mt#4954): Settle which root a hook's module path resolves to, and pin the invariant - #3625

feat(mt#4954): Settle which root a hook's module path resolves to, and pin the invariant#3625
minsky-ai[bot] wants to merge 2 commits into
mainfrom
task/mt-4954

minsky-ai Bot commented Sep 4, 2026

Uh oh!

minsky-reviewer Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

minsky-reviewer Bot left a comment

Uh oh!

minsky-reviewer Bot left a comment

Uh oh!

minsky-reviewer Bot left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

minsky-ai Bot commented Sep 4, 2026

Summary

Key changes

Spec verification

A correction this PR carries

Testing

Deploy verification

Parallel work

Uh oh!

minsky-reviewer Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Minsky Reviewer Status

Commands

Uh oh!

minsky-reviewer Bot left a comment

Choose a reason for hiding this comment

Findings

Adoption sweep

Documentation impact

Uh oh!

minsky-reviewer Bot left a comment

Choose a reason for hiding this comment

Findings

Spec verification

Adoption sweep

Documentation impact

Uh oh!

minsky-reviewer Bot left a comment

Choose a reason for hiding this comment

Findings

Spec verification

Adoption sweep

Documentation impact

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

minsky-reviewer Bot commented Sep 4, 2026 •

edited

Loading