feat(mt#3854): Make .codex a compile output so the harness config stops fossilizing by minsky-ai[bot] · Pull Request #3253 · edobry/minsky

minsky-ai · 2026-08-22T05:28:33Z

Summary

.codex/ held a hand-made port of the Claude harness config. Generated by nothing, it was
refreshed by nothing: measured today it carried 93 hook files against .claude/hooks's 167 — 75
guards missing, and it had not moved since 2026-07-29 while the source grew by 49 files. The spec
recorded 25 missing two weeks ago; the gap had tripled while the task waited.

ask#9256 answered 2026-08-22 with "Support it — make it generated output". This makes .codex/
a compile output: tracked, regenerated from .minsky/, and re-staged by pre-commit.

Key changes

Two targets, both plugged into the existing ADR-016 pipeline — extend, not deviate: that ADR
makes compile "the one pipeline for all behavioral-artifact compilation … to all targets".

codex-hooks — .minsky/hooks/** → .codex/hooks/**, with the same shebang, generation
banner, .minsky/ provenance line and 0o755 as the Claude output.
codex-agents — .minsky/agents/<n>/agent.ts → .codex/agents/<n>.toml.

Both are constructions over shared factories, not copies. Answering a drift task by duplicating
a 214-line target would move the drift into code, where it is harder to notice than a stale
directory is. hook-copy-target.ts and agent-target.ts hold the machinery once; each harness
target supplies only what genuinely differs — an output directory, and for agents a serializer.
claude-hooks and claude-agents were rebuilt on them with their public exports preserved, and
their 103 existing tests pass unchanged, which is the control for that refactor.

Codex is opt-in. The presence flag gates on .codex/ — the output tree, not a .minsky/
source. Every other flag asks "is there something to compile from"; this one asks "has this
workspace adopted Codex". Gating on the source would create a harness config on every clone in the
fleet, since .minsky/hooks and .minsky/agents are always present.

The TOML emitter is the only genuinely new logic, and its failure mode is silent corruption
rather than a crash — a mis-escaped prompt still writes a file and only differs from the source in
the middle of a 40KB body. So it is written against TOML v1.0.0's string rules rather than today's
three values: backslash escaped before quote, """ runs broken, a trailing quote escaped off the
closing delimiter, and delimiters carrying \ line-continuations so the value round-trips
exactly rather than gaining the leading/trailing newline Bun.TOML.parse leaves in.

Plus: .codex/agents/*.toml carries the shared COMPILE_GENERATED_BANNER, so the generated-file
guard (which decides by content, not path) denies a hand-edit; the pre-commit regen step now
refreshes and re-stages both hook trees together; and .codex/hooks/** joins .claude/hooks/**
in the no-raw-console exemption.

What is deliberately NOT here

The hooks manifest. Codex's PreToolUse intercepts Bash only — apply_patch,
Edit/Write/Read, web fetch and MCP tool calls do not fire it
(config docs,
hooks reference). Measured against
the hand-made .codex/hooks.json: 12 of its 13 PreToolUse matcher entries cannot fire, the
one exception being Bash|mcp__minsky__session_exec. The unreachable set includes the entire
merge-gate chain, which hangs off mcp__minsky__session_pr_merge.

That inverts this task's own risk argument — the spec said the danger is that "a harness config
carrying the merge gates can silently fossilize", and those gates do not fire under Codex whether
the manifest is fresh or 75 guards stale. Emitting the corpus verbatim would produce a manifest that
reads as current and enforces almost nothing, which is worse than the fossil because the fossil at
least looks stale. mt#4431 owns that decision. This PR ships the scripts; a fresh
.codex/hooks/** is necessary, not sufficient, and should not be read as "the guards now run under
Codex."

Testing

Execution evidence:

SC1 (source-copy half) / AT1 / AT3 — after explicit adoption, the guard sets match exactly.
The manifest half of SC1 is deferred: [sc1-deferred: mt#4431]

$ bun run src/cli.ts compile --target codex-hooks
$ bun run src/cli.ts compile --target codex-agents
$ ls .claude/hooks | wc -l   → 167        $ ls .codex/hooks | wc -l   → 167
$ ls .claude/agents | wc -l  →   7        $ ls .codex/agents | wc -l  →   7

AT3: guards in .claude but not .codex  → count: 0
AT3: guards in .codex but not .claude  → count: 0

The 75-guard gap is closed, in both directions. SC2 is the codex-agents line above: 7 sources
→ 7 .toml outputs.

SC3 / AT2 — bare compile must not create .codex/ on a checkout that never had it. Run in this
session before adoption, which is exactly that state:

$ ls -d .codex   →  confirmed absent before run
$ bun run src/cli.ts compile
[compile] Target "claude-skills": 46 file(s) written
... (claude targets only; no codex target listed)
$ ls -d .codex   →  still absent — AT2 PASSES

And the other half — once .codex/ exists, a bare compile refreshes it:

$ bun run src/cli.ts compile
... [compile] Target "codex-hooks": 167 file(s) written
    [compile] Target "codex-agents": 7 file(s) written

SC4 — repo-relative hook command paths — lives with the manifest that carries those paths:
[sc4-deferred: mt#4431]

SC5 — .codex/ no longer untracked-and-unignored. This commit adds 174 .codex/ files as
tracked generated output (188 files changed total).

SC6 / AT4 — the banner is one the guard actually recognises:

$ head -3 .codex/agents/auditor.toml
# Generated by minsky compile. Do not edit directly.
# Source: .minsky/agents/auditor/agent.ts
name = "auditor"

SC7 / AT5 — pre-commit regen. This commit's own pre-commit run reported:
✅ All compile outputs are up-to-date (… claude-hooks, codex-hooks, codex-agents) — the codex
targets are in the check set, live.

Unit tests:

$ bun test ./packages/domain/src/compile/targets/codex-targets.test.ts
 25 pass  0 fail  51 expect() calls

$ bun scripts/run-related-tests.ts packages/domain/src/compile/targets/codex-agents.ts \
    src/hooks/claude-hooks-compile-regen.ts packages/domain/src/compile/compile.ts
 171 pass  0 fail  352 expect() calls   (11 related test files)

Negative control — the escaper: escapeTomlMultilineString's delimiter test asserts the output no
longer matches /(^|[^\\])"""/, and a paired control asserts the unescaped body does match it.
Without that pair, a no-op escaper would pass the first assertion for the wrong reason. Same shape
for the SC6 banner test: one case asserts the emitted TOML matches the guard's own pattern list, its
control asserts an unbannered TOML does not.

The round-trip test earned its place — it failed on the first implementation, showing the parsed
body carrying a leading and trailing newline the source never had, which is what produced the
line-continuation design above. A test asserting "contains the body" would have passed that bug.

SC5 lint half: validate_lint in this session reports 0 errors / 0 warnings across 4069
files with .codex/ present and generated. Main, for contrast, still reports the 27
custom/no-raw-console errors from the un-exempted hand-made tree.

validate_typecheck 0 errors across 8 projects. format:check clean.

Deploy verification

This PR is deploy surface — verified by running the predicate rather than assuming:
isDeploySurfaceFile() returns true for all 13 changed packages/domain/** and src/hooks/**
files (and false for eslint.config.js and the generated .codex/** tree). Deploy verification
will run post-merge against the merge timestamp per §10; it is not waived.

…ps fossilizing ask#9256 chose "Support it — make it generated output". `.codex/` was a hand-made port of the Claude harness config, generated by nothing and therefore refreshed by nothing: measured today it carried 93 hook files against `.claude/hooks`'s 167 — 75 guards missing, unmoved since 2026-07-29 while the source grew by 49 files. The spec recorded 25 missing two weeks ago; the gap had tripled. Two new compile targets, both plugged into the ADR-016 pipeline (extend, not deviate — that ADR makes `compile` the one pipeline "to all targets"): - `codex-hooks` — `.minsky/hooks/**` → `.codex/hooks/**`, same shebang, banner, `.minsky/` provenance line and 0o755 as the Claude output. - `codex-agents` — `.minsky/agents/<n>/agent.ts` → `.codex/agents/<n>.toml`. Both are constructions over shared factories rather than copies. Answering a drift task by duplicating a 214-line target would move the drift into code, where it is harder to notice than a stale directory, so `hook-copy-target.ts` and `agent-target.ts` hold the machinery once and the harness targets supply only what genuinely differs: an output directory, and for agents a serializer. `claude-hooks` and `claude-agents` were rebuilt on them with their public exports preserved; their 103 existing tests pass unchanged, which is the control for the refactor. Codex is OPT-IN. The presence flag gates on `.codex/` — the OUTPUT tree, not a `.minsky/` source — because every other flag asks "is there something to compile from" and this one asks "has this workspace adopted Codex". Gating on the source would create a harness config on every clone in the fleet. Verified both ways: a bare compile in a checkout without `.codex/` emits neither target and creates nothing; after adopting explicitly, a bare compile refreshes both. The TOML emitter is the only genuinely new logic and its failure mode is silent corruption, so it is written against TOML v1.0.0's string rules rather than today's three values: backslash escaped before quote, `"""` runs broken, a trailing quote escaped off the closing delimiter, and delimiters carrying `\` line-continuations so the value round-trips EXACTLY rather than gaining the leading and trailing newline `Bun.TOML.parse` leaves in. That last one was found by the round-trip test, not by reading. Also: `.codex/agents/*.toml` carries the shared `COMPILE_GENERATED_BANNER`, so the generated-file guard — which decides by content, not path — denies a hand-edit; the pre-commit regen step now refreshes and re-stages both hook trees together, so a hooks commit cannot land with one fresh and one behind; and `.codex/hooks/**` joins `.claude/hooks/**` in the `no-raw-console` exemption, which removes the 27 errors that made a main-workspace lint read as "main is red". The hooks MANIFEST is deliberately not here. Codex's `PreToolUse` intercepts Bash only — MCP tool calls do not fire it — so 12 of the hand-made `.codex/hooks.json`'s 13 PreToolUse matchers cannot fire, including the whole merge-gate chain. Emitting that corpus verbatim would read as current while enforcing almost nothing. mt#4431 owns that decision; this commit ships the scripts, and a fresh `.codex/hooks/**` is necessary rather than sufficient. This IS deploy surface: isDeploySurfaceFile() returns true for the 13 changed packages/domain and src/hooks files, so the post-merge deploy verification runs rather than being waived.

minsky-reviewer · 2026-08-22T05:28:36Z

Minsky Reviewer Status

Verdict: CHANGES_REQUESTED — 14 blocking finding(s)
Review: View review
Model: openai/gpt-5 | Tokens: 8670K prompt, 138K completion | Duration: 2734s
Mode: normal

Commands

/review — request a fresh review

edobry · 2026-08-22T05:40:33Z

/review

…s own GitHub App ## Summary The reviewer service runs **two credential systems in one process**, and both background schedulers were reading the one this service does not provision. - The **review path** authenticates with the reviewer App's own credentials — `MINSKY_REVIEWER_APP_ID` / `_PRIVATE_KEY` / `_INSTALLATION_ID`, read via `requireEnv` (`config.ts:152-155`), so the service cannot boot without them. That is why reviews post as `minsky-reviewer[bot]`. - The **schedulers** called `createTokenProvider(cfg.github ?? {}, cfg.github?.token ?? "")` against the DOMAIN config, whose `github.serviceAccount` is populated from a different namespace entirely — `MINSKY_APP_ID` / `MINSKY_APP_PRIVATE_KEY_FILE` / `MINSKY_APP_INSTALLATION_ID` (`environment.ts:31-34`). This service has no reason to set those, so the provider fell through to `FallbackTokenProvider("")` and **every request went out unauthenticated** against GitHub's 60-per-hour per-IP budget. Both schedulers already **received** the working credentials and threw them away — `void config; // held for future use`, in `pr-watch-scheduler.ts` and `asks-reconcile-scheduler.ts` alike. **The symptom was not a crash.** `runWatcher` catches each watch's error into a counter every caller dropped, so a fully rate-limited scheduler logged `poll_complete` each cycle and delivered nothing: **18 rate-limit errors across 41 minutes**, in pairs on nearly every cycle after onset, with no escalation. The module docblock asserted the client was "backed by the Minsky implementer GitHub App token" and reasoned about a 5000/hour App budget — both were **intent, never behavior**, which is what kept this invisible. ## Key changes - **`services/reviewer/src/github-token-provider.ts` (new)** — one seam both schedulers share, building a `GitHubAppTokenProvider` from `ReviewerConfig`. Missing or malformed credentials **throw**; the defect being fixed was precisely a silent fallback. - **Both schedulers refuse to start** on missing credentials rather than polling uselessly, and both now thread `config` through to the provider. - **Rate-limit rejections are classified distinctly** in the domain watcher, keyed on the message rather than the status code — GitHub returns 403 for *both* a rate-limit and a permission denial, and those need opposite responses. - **The scheduler consumes the error count it used to drop.** A cycle in which every inspected watch failed starts an exponential backoff (capped so it still re-probes inside GitHub's hourly reset) and escalates **once** at 3 consecutive cycles — grounded in the observed 9-cycle incident rather than a round number. **Why the reviewer App and not the implementer App.** The accepted *"Hybrid read/write split for GitHub operations"* decision makes reads **identity-agnostic** ("any authenticated user can read PRs they have access to"), and both schedulers only read. Its **Open Question 1** names this exact case — *"trigger to revisit: when Minsky gets a hosted/headless execution mode"* — and the identity/provenance position paper calls the reviewer's separate credentials *"the load-bearing isolation that makes 'adversarial review'"* work. Importing the implementer App here would erode that for no benefit and require env vars this service lacks. **No new provisioning is needed:** the credentials are already `requireEnv`-mandatory, so they exist wherever the service boots at all. ## Testing Execution evidence: **SC1 / SC2 / SC6 + AT1 — the seam, and the scheduler refusing to start without it.** ``` $ cd services/reviewer && bun test --preload ../../tests/setup.ts \ src/github-token-provider.test.ts src/pr-watch-scheduler.test.ts \ src/asks-reconcile-scheduler.test.ts 32 pass 0 fail 49 expect() calls ``` AT1 asserts `startPrWatchScheduler` returns `null` for an empty private key and for a NaN app id, **with a paired control** asserting it returns a live handle for valid credentials — without that, both refusals would pass equally well for a scheduler that never starts at all. **SC3 + AT2 (classification half) — including the discriminating negative case.** ``` $ bun test --preload ./tests/setup.ts packages/domain/src/pr-watch/watcher.test.ts 40 pass 0 fail 141 expect() calls ``` 34 of those are pre-existing and are the control for the watcher edit. The 6 new ones assert `isGitHubRateLimitError` on the exact production string, the authenticated per-user form, the secondary-limit wording — and that `"Resource not accessible by integration"` is **not** classified as a rate limit. That last is the case that matters: a status-code check cannot separate the two, and misclassifying it would back off 30 minutes against a fault waiting never fixes. **SC4 / SC5 + AT2 (deferral half) / AT3** — asserted on the extracted pure decision function `evaluateCycleOutcome`: exponential growth, the 30-tick cap, escalate-exactly-once at the threshold, no re-escalation while the fault persists, re-escalation after a recovery, and that a zero-watch or partially-failing cycle is **not** a total failure. **AT5 — no live unauthenticated-capable construction remains.** ``` $ grep -rn 'createTokenProvider' services/reviewer/src --include='*.ts' \ | grep -v '//' | grep -v '^\S*: *\*' (zero lines) ``` All 4 remaining textual mentions are comments/docblocks quoting the old expression deliberately. **The spec's original grep for this AT was corrected during implementation** — it matched those comments and so would have reported failure on a correct fix. **Full related sweep:** `bun scripts/run-related-tests.ts …` → 5 related test files passed. `validate_typecheck` 0 errors across 8 projects. `validate_lint` **0 errors / 0 warnings** across 3900 files. `format:check` clean. Negative control: reverting to the shipped expression, evaluated against this service's actual config shape ``` $ bun -e 'import {createTokenProvider} from "@minsky/domain/auth"; const p = createTokenProvider({}, ""); console.log(p.isServiceAccountConfigured(), JSON.stringify(await p.getServiceToken("edobry/minsky")));' false "" ``` An **empty token string** — the unauthenticated request, demonstrated directly rather than inferred from the logs. The post-fix provider returns `isServiceAccountConfigured() === true`, which is the assertion the new test makes. Negative control — the test fixture: `asks-reconcile-scheduler.test.ts` carried `privateKey: ""` and passed for as long as it existed, because the scheduler never used the credentials it was handed. That is the production defect in miniature. Adding the credential guard made it fail (`expect(handle).not.toBeNull()` → received null); the fixture now carries a real-shaped key. ## Deviations recorded in the spec **Three**, each amended in the criterion's own text rather than explained elsewhere: 1. **AT5's grep** now excludes comments (above). 2. **SC5's backoff keys on total-failure, not the reset header.** The domain `GithubPrClient` interface returns domain types and **throws** — no response object or headers reach the caller, so honoring the reset header would mean widening three methods plus the Octokit adapter for a signal only one of several faults carries. Total-failure backoff reaches the same goal and also covers a revoked credential and a GitHub outage. Trade-off accepted: recovery is noticed up to 30 minutes late rather than at the exact reset instant. 3. **SC3 shipped PARTIALLY — classification yes, reset time no.** The criterion's parenthetical asks for the log line to carry "the reset time GitHub returns." It does not, for the same structural reason as (2): nothing surfaces `x-ratelimit-reset` to the caller. Deliberately not worked around by parsing a timestamp out of the message text — GitHub's rate-limit body does not reliably carry one, and a parser that silently yields nothing would be a can't-fail probe. Whoever widens `GithubPrClient` for (2) should add it to this log line in the same change. ## Live verification **UNVERIFIED — deferred to §10 post-deploy, because the fault is environment-bound.** The assertion that matters operationally is "the deployed scheduler now authenticates," and that cannot be produced from a session workspace: it depends on the reviewer service's own env, which holds the App credentials this change reaches for. Nothing here is blocked on operator access — the check simply has to run against the deployed process. Post-deploy, §10 will read the reviewer's logs and confirm **zero `rate limit exceeded` lines** and `poll_complete` cycles with `errors: 0` — which is **AT4**. Until that runs, this change is not confirmed working in production; deploy-SUCCESS alone would only prove the container started. Deploy verification: required and not waived. `isDeploySurfaceFile()` returns **true for all 8 changed files** (verified by running the predicate, not by recalling the pattern set), so §10 runs against the merge timestamp. ## Context Diagnosed while investigating why PR #3253 had zero reviews; that turned out to be a **separate** defect (mt#4434, GitHub's 20,000-line diff cap). These rate-limit errors sat in the same log window and are independent of it — two distinct failures in one 41-minute window. Closes mt#4435. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>

## Summary `ask-routing-deferral`'s `principal-reserved` patterns match noun-phrase claims — `needs your call`, `your decision to make` — and look no further than their own span. English lets the very next words invert one, so `mt#4458 needs your call on nothing — it needs the daemon.` fired as a deferral while asserting the opposite. The matcher had no way to tell it from the genuine form. Measured at planning against the shipped matcher, not inferred: | input | `principal-reserved` match (before) | | --- | --- | | `- **mt#4458 needs your call on nothing — it needs the daemon.` | yes | | `mt#4458 needs your call on the daemon question.` (control) | yes | | `That needs your call for nothing; it is already settled.` | yes | | `It needs your call about nothing.` | yes | `settlesDecision()` and `citesFiledAsk()` returned `false` on all four, so no shipped suppression reached them. ## Key changes - **`NEGATING_COMPLEMENT_RE`** — `on|for|about` + `nothing`, checked against the 24 chars following a match. Its docblock states what is covered and, per the first success criterion, what is deliberately **not**: sentence-level negation (the negator precedes the phrase, so no forward look sees it), quantifier complements (`on none of this`), and negation across an intervening clause. Each wants its own measured window under ADR-024 clause (b). - **`firstUnnegatedMatch`** — scans *every* occurrence rather than testing `exec`'s first. The detector takes one match per class, so skipping to the next pattern on a negated first hit would drop a genuine later mention — trading this false positive for a false negative. Pinned by a test. - **Scoped to one class.** Only `principal-reserved` consults the complement; `deferral-menu`'s patterns are interrogative or imperative shapes where the construction does not arise. ADR-024 placement: **Rung 1** — a deterministic prefilter targeting the precision axis, which the ADR names as "the default stopping point." Same extension the ask-citation token test made at `ask-routing-deferral-detector.ts:887-893` for mt#4201; no rung escalation is argued. ## Testing Execution evidence: ``` $ bun test --preload ./tests/setup.ts ./.minsky/hooks/ask-routing-deferral-detector.test.ts 130 pass 0 fail 241 expect() calls Ran 130 tests across 1 file. [90.00ms] $ bun run test:hooks 6187 pass 0 fail 12867 expect() calls Ran 6187 tests across 174 files. [25.04s] ``` **Captured context yields no match** — the 2026-08-23T18:34:29.951Z fire, verbatim, is a named fixture and asserts no `principal-reserved` match. All three complements (`on nothing`, `for nothing`, `about nothing`) plus an uppercase variant are covered. **`deferral-menu` untouched** — a turn carrying both a negated `principal-reserved` phrase and `What's your call?` still fires the menu class and not the reserved one. **Typecheck and lint clean** — `validate_typecheck` 0 errors across 8 projects including `tsconfig.hooks.json`; `validate_lint` 0 errors, 0 warnings over 3996 files. Both scoped to this session (`validatedWorkspace` confirmed). Negative control: reverted the one behaviour-determining line — the class-conditional back to a bare `pattern.exec(scanned)` — and re-ran. ``` (fail) SC2: the captured fire produces no principal-reserved match (fail) SC1: "on nothing" suppresses (fail) SC1: "for nothing" suppresses (fail) SC1: "about nothing" suppresses (fail) SC1: the complement match is case-insensitive (fail) a negated mention does not mask a genuine one later in the same turn (fail) SC4: deferral-menu still fires on a negated-complement turn 123 pass 7 fail ``` 7 of the 8 new tests fail pre-fix. The two that pass in **both** states are the two that must: the un-negated control (its job is to hold either way) and the pin on deliberately-uncovered sentence-level negation. **False-negative measurement (ADR-024 clause (b): 0 known-FP, ≤5% new FN).** Replayed all 260 captured match-contexts in `.minsky/ask-routing-deferral-calibration.jsonl` through this branch's detector and the main-workspace baseline, comparing verdicts: ``` captured match-contexts replayed (all classes): 260 principal-reserved verdict CHANGED by this fix: 1 - 2026-08-23T18:34:29.951Z base: true -> fixed: false | - **mt#4458 needs your call on nothing — it needs the daemon. ``` Exactly the target record. One known false positive removed, zero new false negatives. A first attempt at this measurement compared today's detector against *what production recorded at the time* and reported 6 suppressions. That baseline was wrong: 5 of the 6 had already stopped firing on main because of mt#4175 / mt#4311 / mt#4201, so it was measuring every change since those records were written, not this one. The comparison above isolates the delta by holding everything else fixed. ## Deploy impact `[no-deploy-impact]` verified rather than assumed — `isDeploySurfaceFile` returned `false` for all four candidate paths (both `.minsky/` sources and both generated copies). Only `.claude/hooks/` regenerated; `.codex/hooks/` is not yet a compile output (mt#3854, PR #3253), so this branch does not touch it — which also moots the file-level overlap with that PR flagged at planning. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>

Non-blocking, and material: the replay read a SINGLE `per_page=100` page. That is the API's cap, not a PR's size, so a larger PR silently truncated and every enumerated path in the missing pages read as UNTOUCHED — biasing the flag rate UPWARD, the direction that makes this check look noisier than it is. Not hypothetical. PR #3253 in this repo has 188 files, and a one-page read of it during this task's own planning returned exactly 100 with no indication anything was missing. Same trap, same session. Now paginates to a 10-page ceiling, and a PR past that ceiling is EXCLUDED and counted rather than measured on partial data — an undercounted diff manufactures false flags, and silently including them is worse than reporting a smaller n. The exclusion count is printed. Re-measured, and the honest reading is NOT the obvious one: zero comparable PRs exceeded the cap in either window and the flagged count is identical at 9, so the fix changed nothing on this corpus. The rate moved 42.9% -> 47.4% only because the two runs are ~20 minutes apart and the most-recent-40 window slid forward (comparable/nothing-to-compare went 21/19 -> 19/21). The fix is right in principle; it is not what moved the number, and the spec says so. Conclusion unchanged: ~43-47% either way, record-only, qualifier tune at mt#4582.

…session_start ## Summary The bind/advance spec-read guard denied `session_start` with the task-hijack message ("this session has never read or authored mt#X's spec") whenever the spec was read and amended through the **Minsky CLI** rather than the MCP tools. Its evidence predicates enumerate MCP tool names only, so a session that did an hour of real work via `minsky tasks spec get` and `minsky tasks edit --spec-file` — the documented fallback when the MCP daemon drops — looked identical to one that never opened the task. The guard was right about its own evidence and wrong about the world. That is mt#4536's **"blocks WRONGLY"** class, and this guard was its sole confirmed member: the only CLI-blind guard whose discharge evidence comes from prior tool-call records rather than from the call being guarded. ## Key changes **Widen the EVIDENCE, not the matcher.** The guard already fires correctly — it runs on `session_start`, sees the call, and reaches its decision. mt#4536's headline prescription (widen the matcher to `Bash|mcp__minsky__session_exec`) is scoped to the REGISTRATION-blind guards, which never run at all for a CLI call; applying it here would be both wrong and costly, since this module reads the transcript to decide and would pay a full transcript parse on every bash call in the session. **Nothing in `.claude/settings.json` changed**, and the two-binding-site requirement is therefore inapplicable. This is recorded in the spec's planning audit as a gate (m) scope-condition failure on an inherited claim. **Resolution goes through the generated oracle, never a pattern.** Commands resolve via the `commandId` stamp in `src/generated/completion-manifest.json` (mt#4144), the same oracle `cli-mcp-substitution` uses. The manifest also carries each leaf's positional `arguments` and per-option `takesValue` — which is what lets the guard recover the **target task id** rather than merely recognising the command. That closes the question mt#4536 explicitly left open ("whether a widened guard can actually PARSE what it now sees"). - **`detect-cli-mcp-substitution.ts`** — extracted `resolveCommandNode` (deepest node **plus** the argv tail after the last *subcommand*) and exported `optionTakesValue` / `firstPositional` / `hasAnyFlag`. `resolveCommandId` now delegates; behaviour unchanged, asserted by its existing tests. Slicing at the last subcommand rather than at the walk's stopping point is load-bearing: the walk consumes a token following an unrecognised flag, so `tasks spec get --json mt#4380` would otherwise silently swallow the id on every flagged invocation. - **`check-task-spec-read.ts`** — credits `tasks spec get` and `tasks get --include-spec` as reads, and `tasks edit --spec` / `--spec-file` / `--spec-content` as authorship, across both `Bash` and `mcp__minsky__session_exec`. Checked only *after* the MCP predicates miss, so the common path never touches the manifest or the disk. The CLI rows mirror their MCP counterparts including the negatives: a bare `tasks get` is not a read, a `--kind`-only edit is not authorship. - **Safe direction on ambiguity.** An option the manifest does not list is assumed to take a value. Over-consuming can only cause a positional to be MISSED (leaving the guard exactly as it behaves today); under-consuming would let a flag's *value* be read as a task id, inventing an engagement with a task nobody named. - **`duplicate-check-candidate-read.ts`** gets the widening by construction — it COMPOSES `specWasSurfaced` rather than duplicating it, so before this change a candidate whose spec was read on the CLI was reported UNREAD, the same false negative one guard over. Letting it propagate is the deliberate call (isolating it would preserve a known-wrong behaviour in the consumer); both directions are now pinned by tests. This consumer was missing from the spec's `## Scope` and was added at planning as a gate (h) failure. - **Canary** gains a CLI-read control, so it can no longer be satisfied by a guard that denies unconditionally. **Not credited, deliberately:** a CLI `minsky tasks create --spec-file`. The MCP credit for `tasks_create` correlates the minted id out of the tool RESULT; recovering it from stdout text is a different mechanism. Residual named in the spec, not solved. **Governance:** ADR-024's ladder does not govern — its rungs scope to trigger PHRASES in the agent's prose, and a parsed command string resolved against a generated oracle has no paraphrase axis (the boundary mt#4595 is writing into ADR-024). The Thin-hooks RFC (Accepted) constrains the *shape*: it is retiring four sweeper-written cache pipelines, so this reads a committed build artifact and adds no fifth. ## Testing `bun run test:hooks` — **6507 pass, 0 fail** across 178 files. `bun scripts/run-related-tests.ts` over the four changed source files — **2967 pass, 0 fail** across 61 files. Typecheck: 0 errors across 8 projects. Lint: 0 errors, 0 warnings, 4099 files. Execution evidence: ``` $ bun test --preload ./tests/setup.ts ./.minsky/hooks/check-task-spec-read.test.ts ./.minsky/hooks/duplicate-check-candidate-read.test.ts 133 pass 0 fail Ran 133 tests across 2 files. [112.00ms] $ bun run test:hooks 6507 pass 0 fail Ran 6507 tests across 178 files. [27.93s] $ bun scripts/run-related-tests.ts .minsky/hooks/check-task-spec-read.ts .minsky/hooks/detect-cli-mcp-substitution.ts .minsky/hooks/duplicate-check-candidate-read.ts scripts/lib/standalone-guard-canaries.ts 2967 pass 0 fail Ran 2967 tests across 61 files. [14.59s] ``` Per-AT coverage, using mt#4380's own numbering: - **AT1** — `AT1 — a transcript whose only engagement is a CLI spec read counts as read`, covering `minsky …`, `bun run src/cli.ts …`, and a path-qualified binary. PASS. - **AT2** — `AT2 — a transcript whose only engagement is a CLI spec edit counts as authored`, which also asserts it is authorship and NOT a read, the same split the MCP predicates draw. PASS. - **AT3** — `AT3 — a transcript with no spec engagement through ANY channel still fails both`. PASS. - **AT4** — the guard's existing tests pass unchanged: the file went 93 → 112 tests, 0 fail, and the 19 added are all new `describe` blocks. `resolveCommandId`'s own tests also pass, confirming the refactor preserved its behaviour. PASS. - **AT5** — `AT5 — a CLI spec read of a DIFFERENT task does not discharge the target` and its `…EDIT of a different task…` sibling. Each asserts the inverse in the same test (the other task IS discharged for itself), so the `false` cannot be satisfied by a channel that credits nothing. PASS. - **AT6** — `tasks get counts only with --include-spec`, `all three tasks-edit spec flags are authorship, in both spellings`, and `a metadata-only tasks edit is NOT authorship`. PASS. - **AT7** — `run — the CLI read channel reaches this detector too (mt#4380)`, both directions. PASS. Per-criterion coverage: SC1 → AT1, SC2 → AT2, SC3 → AT3 plus the AT5 pair (the not-weakened-into-a- pass-through control), SC4 → the `## The CLI evidence channel` section added to `docs/architecture/hooks/bind-advance-spec-read-guard.md`, SC5 → AT5, SC6 → AT7. No criterion names a runnable command with an expected result, so each is discharged by its named test rather than by pasted command output. Negative control — full pre-fix source revert, new tests retained: ``` $ git restore .minsky/hooks/check-task-spec-read.ts .minsky/hooks/detect-cli-mcp-substitution.ts scripts/lib/standalone-guard-canaries.ts $ bun test --preload ./tests/setup.ts ./.minsky/hooks/check-task-spec-read.test.ts ./.minsky/hooks/duplicate-check-candidate-read.test.ts SyntaxError: Export named 'cliSpecEngagements' not found in module '…/check-task-spec-read.ts'. (fail) run — the CLI read channel reaches this detector too (mt#4380) > a CLI `tasks spec get` for the candidate discharges it, like the MCP spelling 20 pass 2 fail ``` That half proves the capability is absent pre-fix, but the guard's own file could not load, so its assertions were never evaluated. Per mt#4512 — a control that does not exercise the behaviour is two hypotheses, not one — here is the sharper control that keeps every new helper and disables **only** the two call sites wiring them in: Negative control — wiring-only revert (helpers present, both call sites returning `false`): ``` (fail) … CLI channel (mt#4380) > AT1 — a transcript whose only engagement is a CLI spec read counts as read (fail) … CLI channel (mt#4380) > AT2 — a transcript whose only engagement is a CLI spec edit counts as authored (fail) … CLI channel (mt#4380) > the in-session shell surface counts too, not just Bash (fail) … CLI channel (mt#4380) > AT5 — a CLI spec read of a DIFFERENT task does not discharge the target (fail) … CLI channel (mt#4380) > AT5 — a CLI spec EDIT of a different task does not discharge the target either (fail) … CLI channel (mt#4380) > id spellings collapse the same way they do on the MCP side (fail) run — the CLI read channel reaches this detector too (mt#4380) > a CLI `tasks spec get` … 126 pass 7 fail ``` All seven failures are mt#4380 assertions. Two things this control establishes that the count alone does not: the `cliSpecEngagements` pure-function tests still PASS, isolating the failure to the wiring rather than the parser; and **AT3 still passes**, so the control did not simply break everything. Both files were restored from backup afterwards and re-verified at 133 pass / 0 fail with zero residual control markers. **What these controls do not buy** (per §7 item 7(e)): they prove the probe can fail for the cases reverted. The defect CLASS is "an evidence channel that cannot see one of two surfaces." Within this guard the class is now covered on both surfaces and both tool names; the class member deliberately NOT covered is CLI `tasks create`, named above and in the spec. ## Deploy verification `[no-deploy-impact]` — claimed from the predicate, not from memory. `isDeploySurfaceFile` returns `false` for all six changed source files and for all three generated copies (`.claude/hooks/check-task-spec-read.ts`, `.claude/hooks/detect-cli-mcp-substitution.ts`, `.codex/hooks/check-task-spec-read.ts`): ``` false .minsky/hooks/check-task-spec-read.ts false .minsky/hooks/check-task-spec-read.test.ts false .minsky/hooks/detect-cli-mcp-substitution.ts false .minsky/hooks/duplicate-check-candidate-read.test.ts false scripts/lib/standalone-guard-canaries.ts false docs/architecture/hooks/bind-advance-spec-read-guard.md false .claude/hooks/check-task-spec-read.ts false .claude/hooks/detect-cli-mcp-substitution.ts false .codex/hooks/check-task-spec-read.ts ``` ## Notes for review **Filed mt#4653 rather than fixing it here.** `hook-module-inventory.test.ts` derives the ADR-026 tier-1 set with a bare `readFileSync(...).includes("ensureHookDomainBootstrap")`, which does not distinguish a call from a mention. A comment in this PR explaining *why* the guard avoids the domain bootstrap flipped the guard into `liveTier1` and failed the census, inviting the reconciliation "add the row" — which would write a false persistence claim into a document used for migration-wave selection. Worked around here with a comment telling future editors not to name the identifier in prose; that comment is itself the tell, and mt#4653 removes it along with the underlying derivation. Fixing the derivation can move modules between migration waves, which does not belong in a guard fix. **`.codex/hooks/` is not regenerated by this PR.** Pre-commit regenerated the two `.claude/hooks/` copies and left `.codex/` alone, which is the repo's current behaviour — mt#3854 / PR #3253 is converting `.codex` into a compile output for exactly this reason. That PR touches `.codex/hooks/check-task-spec-read.ts`; it does **not** touch any of this PR's in-scope files (verified against its branch's actual commits, not its title), so the two are sequencing neighbours rather than a collision. Whichever lands second runs the live regeneration path. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>

…e the "repo" location ## Summary `verify-subagent-model.ts:234` resolved its log as `resolve(findRepoRoot(cwd), ".minsky/subagent-model-mismatch.jsonl")` — the **last telemetry writer depositing a file into whatever Minsky-managed project the agent happened to be in**, the condition mt#4748's SC2 forbids. Unlike its siblings nothing was BROKEN: reader and writer agreed. The defect was only that the place they agreed on was a working tree. This is the fourth and last task in the family (mt#4752, mt#4778, mt#4811, this). ## SC1's decision: FLAT, not project-keyed — and why The spec left this open because `resolveStreamPath` project-keys only the `calibration` and `evaluation` families; `subagent-model-mismatch` is `family: "special"`, so flipping it to `state-dir` lands it flat, and that decides what `special` means. Three pieces of evidence, in the order that settled it: 1. **Project attribution never depended on the path — read, not assumed.** `attachProjectIds` (`ingest-service.ts:313-328`) takes `sourcePath` but uses it only for the `source_path` column; `projectId` comes solely from `resolveProjectIds(sessionIds)`, resolving each record's `session_id` through `conversationRunStateTable.projectId`. This was the falsifier for the decision's own premise and was run **before** the decision was written. 2. **Measured against production.** The two flat state-dir families carry populated `project_id` at scale — `fire-log` 1,003,816 rows non-null, `guard-health-log` 121. Both of this stream's own rows are already in `guard_events` with `project_id` **non-null**. 3. **The record's subject is the DISPATCH, not the repo.** Every field written (`session_id`, `dispatching_agent_id`, `tool_name`, `requested`, `resolved`) describes model-tier resolution on an `Agent` dispatch. Nothing in it is a property of the repository being worked. ## Key changes - **`.minsky/hooks/verify-subagent-model.ts`** — `MISMATCH_LOG` is now the BARE name, byte-identical to the manifest row's `relativePath`; `getSubagentModelStateDir` / `getMismatchLogPath` resolve flat under the state dir. `appendMismatchRecord` takes `env` (injected, not read from the module) and no longer takes `cwd`. - **A latent reader/writer split, found while picking the resolver, and avoided.** `getMinskyStateDir()` (`@minsky/shared/paths`) keys on `XDG_STATE_HOME` **only** — mt#3965 records that as deliberate — while the ingest reader's `resolveStateDir` keys on `MINSKY_STATE_DIR`. Using the shared helper here would have reproduced exactly the writer/reader split mt#4811 found live in `ask-form-lint`, where the sweep read an empty corpus and reported *"this guard never fired."* This writer follows the reader's convention instead, and **AT3 pins the agreement** so the choice cannot silently drift. Note the wider split is pre-existing and out of scope: the calibration writers use the shared helper while the ingest reader does not, so they diverge under `XDG_STATE_HOME`. - **`"repo"` is retired from `GuardEventStreamLocation`**, and its `resolveStreamPath` branch is deleted. Re-introducing a repo-rooted stream is now a **type error** rather than something a later sweep has to find — strictly stronger than the grep AT4 asked for. `roots.repoRoot` stays load-bearing: it is `projectStateKey`'s input. - **SC5 — the family's one behaviour-scoped check** (`scripts/lib/repo-rooted-telemetry-paths.ts`). ## SC5: why the check is shaped this way Three prior sweeps were scoped by DIRECTORY (`.minsky/hooks/**`) while the population is defined by BEHAVIOUR — which is why none could see `ask-form-lint-calibration.ts` in `src/adapters/shared/commands/`. This scans for the behaviour: **a path expression rooted at the repo whose target is telemetry**, over `.minsky/hooks`, `src`, and `packages`. **It deliberately does NOT key on the fs write call, and that was measured rather than assumed.** A write-keyed draft was tried first and found **neither** remaining real writer: `calibration-review-cadence-detector.ts` and `calibration.ts` both write through a one-hop helper (`writeLastWarnedStore`, `writeFileMkdir`). Computing a repo-rooted telemetry path at all is the smell. Against the real tree it returns exactly 3 files, all genuine, all allowlisted with reasons: - `require-execution-evidence-before-merge.ts` — **TRACKED, not accepted**: mt#4755 owns it, and four of its five streams were observed still writing into the repo tree today. - `calibration-review-cadence-detector.ts` and `calibration.ts` — the watermark / last-warned / claims stores. **A deliberate carve-out whose justification is provisional**: the code says they "stay repo-rooted … still correctly gitignored," which is a minsky-repo-only property and therefore the premise mt#4748 exists to retire. Recorded in mt#4816's `## Context` as a boundary case someone must decide, not close silently. A `findStaleAllowlistEntries` test fails when an entry outlives the writer it excused — which is how mt#4755's entry will announce that it is ready to be removed. **What it cannot see, stated rather than implied:** a text scan over single-line assignments. It misses a path assembled across statements, one built by concatenation, and one whose root arrives through a parameter. It is a floor under a class that had no mechanical check at all, not a proof of absence. ## Success criteria - **SC1** — met. Decision + reasoning recorded above and in the spec's `## Decision — SC1`. - **SC2** — met. The writer no longer resolves under the repo root. - **SC3** — met, and stronger than asked: verified by asserting writer and reader agree, not by reading both. - **SC4** — met. **Deleted, not migrated**: both records (586 bytes, 2 lines, last write 2026-08-05) were confirmed already in `guard_events` — same two timestamps, ingested 2026-08-12 — so migrating would have created a duplicate and leaving it would have made a 62nd orphan for mt#4777. - **SC5** — met. See above. ## AT4 did not hold as literally written — reported, not narrowed `grep -c 'location: "repo"' packages/domain/src/guard-events/stream-sources.ts` returns **1**, not 0. The surviving match is the COMMENT recording the retirement — prose about the change satisfying a pattern written to detect its absence. The criterion's own text is amended in the spec. The property it is *about* is verified two ways a comment cannot satisfy: the type-level retirement, and a manifest-wide assertion. ## Consumers enumerated (gate (h)) Rows unioned: **Function / type signature** (`MISMATCH_LOG` and `appendMismatchRecord`, both exported, one caller per copy, no external importers) and **Config key / schema field** (the manifest row is a schema field value the ingest reads, and it is documented in `docs/`, which is why the doc consumers below are in scope and would not be under the first row alone). - `.claude/hooks/verify-subagent-model.ts` — regenerated via `bun run src/cli.ts compile`, verified to carry the change. - `.codex/hooks/verify-subagent-model.ts` — currently UNTRACKED; open **PR #3253** (mt#3854) ADDS it as a compile output. Coordinate: whichever lands second regenerates. Read from that PR's actual 188-file list, not inferred. - `ingest-runtime.ts:78` — the `location === "repo"` branch, removed. - `guard-calibration-stream-inventory.md` — §C row, §C prose, and §D's now-false "only §C's `subagent-model-mismatch` remains repo-rooted." - `guard-events-schema.ts:20` — **checked, no change needed**: it enumerates the stream by name and makes no location claim. - ADR-028 §D4 — second amendment. The mt#4748 amendment said "nothing writes into any working tree **for this family**," which was true and narrower than it read. - `hook-module-inventory.md` — the census test caught that this hook newly grep-matches `packages/domain` (three doc-comment references) without importing it, so it joined the Divergence table; count bumped 33 → 34. ## Testing Execution evidence: **AT1 + AT3 + SC3 — the hook's own suite (6 new cases):** ``` $ bun test --preload ./tests/setup.ts --timeout=15000 ./.minsky/hooks/verify-subagent-model.test.ts ./scripts/lib/repo-rooted-telemetry-paths.test.ts 34 pass 0 fail 67 expect() calls Ran 34 tests across 2 files. [568.00ms] ``` AT3 is asserted against `resolveStreamPath` and `resolveStateDir` **imported from the reader**, not against a hand-written expected path — a hand-written expectation is precisely what would have passed while ask-form-lint's reader and writer disagreed. Both the `MINSKY_STATE_DIR`-set and unset branches are asserted; asserting only the temp-dir case would pass even if the two DEFAULT branches diverged, which is the configuration nearly every real invocation runs in. **SC5 — the check against the real tree**, plus 5 synthetic controls including the historical `ask-form-lint` writer it must flag (included in the 34 above). **Full hooks suite** (`ROOTS` excludes this tree from the gated runner, so this is required before push): ``` $ bun run test:hooks 6826 pass 0 fail Ran 6826 tests across 185 files. [35.52s] ``` One earlier run of this suite showed `memory-search > both silent on a trivial (affirmative) prompt` failing on a 15.0s timeout. Confirmed a flake, not a regression: the diff touches nothing named memory, the file passes 72/72 in isolation, and the re-run above is clean. **Related-test selector:** ``` $ bun scripts/run-related-tests.ts packages/domain/src/guard-events/stream-sources.ts packages/domain/src/guard-events/ingest-runtime.ts .minsky/hooks/verify-subagent-model.ts scripts/lib/repo-rooted-telemetry-paths.ts 583 pass 0 fail Ran 583 tests across 25 files. ``` **Typecheck** — 0 errors across all 8 projects, `validatedWorkspace` the session dir (`infra/` skipped with its documented reason). **Lint** — 0 errors, 0 warnings, 4259 files. Negative control — control A: the manifest row and the reader's branch reverted to repo-rooted ``` (fail) SC3 — the stream is declared state-dir, and nothing in the manifest is repo-rooted (fail) SC3 — the writer's file name is byte-identical to the row's relativePath (fail) AT3 — the writer resolves to exactly the path the ingest reader computes (fail) AT3 — they still agree when MINSKY_STATE_DIR is unset (the default branch) (fail) AT3 — the resolved path does not depend on which project the agent is in 21 pass 5 fail ``` Negative control — control B: the writer reverted to repo-rooted, manifest and reader left intact ``` (fail) AT3 — the writer resolves to exactly the path the ingest reader computes (fail) AT3 — they still agree when MINSKY_STATE_DIR is unset (the default branch) (fail) AT1 — appending writes under the state dir and leaves no file in the repo tree 23 pass 3 fail ``` **Why two controls rather than one full revert, stated because the difference matters (mt#4512).** A single full revert of all three production files was attempted FIRST and produced `0 pass / 1 fail` — the whole test FILE failed to load, because the revert removes an exported symbol the new tests import. **A load failure is not evidence the assertions work**, so that control was discarded rather than reported. The two controls above isolate the two halves: A reverts the reader/manifest (AT1 correctly stays green — it asserts the writer), B reverts the writer. Together every new assertion is shown capable of failing, for its own reason. ## Deploy verification Ran the predicate over the actual changed files rather than recalling the pattern set: ``` true packages/domain/src/guard-events/stream-sources.ts true packages/domain/src/guard-events/ingest-runtime.ts false .claude/hooks/verify-subagent-model.ts false .minsky/hooks/verify-subagent-model.ts false scripts/lib/repo-rooted-telemetry-paths.ts false docs/architecture/*.md ``` So this **IS** deploy surface, and no `[no-deploy-impact]` claim is made here or in any commit message. Post-merge I will call `deployment_wait-for-latest` with `notBefore` = the merge timestamp and `expectCommitSha` = the merge SHA, read `buildIdentity`, and on `indeterminate` correlate the deploy workflow run's `head_sha` against the merge commit rather than treating a status code as identity. One post-merge re-check beyond the deploy: the OLD generated hook is still live in main until this merges, so an `Agent` dispatch with a model mismatch in that window could recreate the repo-tree file SC4 removed. Mismatches run ~2 per 26 days, so this is unlikely; I will re-check the path after merge. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>

…helper [no-deploy-impact] ## Summary `require-execution-evidence-before-merge.ts` carried its own `appendCalibrationRecord(record, repoRootDir, logRelPath)` and drove five sibling streams through it, each resolving against the **repo root**. It was the last writer group depositing telemetry into whatever Minsky-managed project the agent happened to be in — the condition mt#4748's SC2 forbids. mt#4752 migrated the other 18 writers and carved this one out by name, because the five path constants are exported and read from outside the ladder. **Reproduced live, more recently than the spec's own measurement.** Four of five streams were still being appended to today, well after mt#4748 merged: `render-path` 20:57:56 EDT (one minute before this pass resumed), `test-first` 18:05:57, `at-coverage` 17:21:27, `consumer-account` 16:53:23. `sc-coverage` has no file at all, consistent with mt#4219. ## Key changes - **The local writer is gone, and so is its `logRelPath` parameter.** Dropping the *parameter* rather than re-pointing it is `gate-walk-provenance`'s reasoning applied five modules over (mt#4752): a path-taking parameter is the degree of freedom that let a sibling's filename be misspelled (mt#2492). PR #2432 R1 had already removed this parameter's DEFAULT, after a caller who omitted the argument silently wrote into the acceptance-test log — corrupting both corpora with no failing test. Now every call site names a stream constant and `logCalibrationRecord` derives the filename, so neither failure has anywhere left to occur. - **All five path constants are DERIVED** — `` `.minsky/${X_STREAM}-calibration.jsonl` `` — so a declaration and the write it describes cannot disagree. The exports stay, because consumers read them (`scripts/at-coverage-reclassify.ts`, the declaration census). - **mt#4816's allowlist entry for this file is removed.** It was recorded as TRACKED-not-accepted naming this task, and `findStaleAllowlistEntries` is what confirms the entry is genuinely retired rather than merely unused. ## Success criteria - **SC1** — met. `export function appendCalibrationRecord(` no longer exists in the gate; asserted by a test, not by inspection. - **SC2** — met. Five derived constants, each exported where a consumer reads it. - **SC3** — met. `calibration-log-declarations.test.ts` already bound to the CONSTANTS rather than to hand-copied strings (mt#4064), so it kept resolving by construction; `at-coverage-reclassify.ts` imports the constant, unchanged. - **SC4** — met, and asserted against pinned literals rather than against another derivation: a derivation compared to a derivation proves the two expressions agree, not that the value did not move. - **SC5 — the criterion named something that does not exist, and is amended in the spec.** It asked that `registryCalibrationPaths()`'s denominator not fall; a repo-wide grep returns **zero hits** for that identifier. My own planning gate (b) accepted it on the strength of its *naming* a symbol rather than that symbol existing — recorded rather than quietly satisfied. The check it was about is real and is the right one: `hook-log-paths-gitignored.test.ts`'s `"mt#4748 SC2 — calibration/evaluation streams never resolve inside the repo"`, **71 pass / 0 fail**. ## Testing Execution evidence: **AT3 + SC4 + SC1 — four new cases in the declaration census** (the file that already owns these constants): ``` $ bun test --preload ./tests/setup.ts --timeout=15000 ./scripts/lib/calibration-log-declarations.test.ts 21 pass 0 fail ``` They assert the pairing (`path === `.minsky/${stream}-calibration.jsonl``), byte-identity to the five historical literals, that every `logCalibrationRecord(` call in the gate names one of the five stream constants — read from the gate's real source, since the pairing alone would hold even if the writer passed a different string — and that the local writer's definition is gone. **AT1 — the population scan:** zero hand-spelled `execution-evidence-*-calibration.jsonl` literals remain in non-test `.minsky/hooks/` files. The behaviour-scoped scan agrees independently: ``` $ bun test --preload ./tests/setup.ts ./scripts/lib/repo-rooted-telemetry-paths.test.ts 11 pass / 0 fail # incl. findStaleAllowlistEntries — the removed entry is confirmed retired ``` **AT4 — full hooks suite:** ``` $ bun run test:hooks 6890 pass 0 fail Ran 6890 tests across 186 files. [29.90s] ``` **Related-test selector** — 3528 pass / 0 fail across 73 files. **Typecheck** — 0 errors across all 8 projects, `validatedWorkspace` the session dir. **Lint** — 0 errors, 0 warnings, 4284 files. Negative control — control A: one constant re-spelled by hand (`execution-evidence-renderpath`, a realistic misspelling) ``` (fail) the execution-evidence merge gate's four calibration logs (mt#4064) > every log the gate writes is declared (fail) the execution-evidence merge gate's four calibration logs (mt#4064) > every log the gate writes resolves back to the gate (fail) the execution-evidence merge gate's four calibration logs (mt#4064) > the four log names are pinned, so a rename cannot pass silently (fail) mt#4755 ... > AT3: each path is exactly `.minsky/${STREAM}-calibration.jsonl` (fail) mt#4755 ... > SC4: every path is byte-identical to the literal it replaced 16 pass / 5 fail ``` Negative control — control B: the local repo-rooted writer reintroduced ``` (fail) mt#4755 ... > SC1: the local repo-rooted writer is gone from the gate 20 pass / 1 fail ``` Restored: 21 pass / 0 fail. The two controls isolate the two halves — A the derivation, B the writer's removal — and A additionally shows three PRE-EXISTING census cases catch the same re-spelling, which is the coverage this task was relying on for SC3. **One existing test was modified, and the modification is the point.** `require-execution-evidence-before-merge.test.ts` asserted the record lands at `join(tmpDir, ".minsky/execution-evidence-at-coverage-calibration.jsonl")` — the repo-rooted location this task removes — so it failed on the first `test:hooks` run. That failure was the fix working. It now resolves through `calibrationLogPath`, the **reader's own** function: a hand-written path would pass just as happily while writer and reader disagreed, which is the state mt#4811 found live in `ask-form-lint`. State-dir isolation checked rather than assumed — `tests/setup.ts` sets both `MINSKY_STATE_DIR` and `XDG_STATE_HOME` to a temp root (mt#3965), so the write cannot reach the operator's real state dir. 236 pass / 0 fail. ## Coordination **mt#4688** (TODO) touches `consumer-account-evidence.ts` too, and this PR does **not** satisfy it — verified by reading rather than inferred: `buildCalibrationLogToGuards` (`scripts/lib/calibration-log-declarations.ts:122-142`) builds its map from `GUARD_REGISTRY[].calibrationLog` and `STANDALONE_GUARD_CANARIES[].calibrationLog`, i.e. from **declarations, not write paths**, so routing the write through the shared helper leaves `execution-evidence-consumer-account` exactly as Unmapped as before. If mt#4688 adds a declaration it should reference `CONSUMER_ACCOUNT_STREAM` rather than re-spelling the literal — which is the drift SC2 exists to prevent. **mt#3672** shares `scripts/at-coverage-reclassify.ts` (unchanged here, but it imports a constant this PR redefines). **PR #3253** adds a `.codex/` generated mirror of the gate; whichever lands second regenerates. ## Deploy verification Ran the predicate over the actual changed files rather than recalling the pattern set — all **13** return `false`: ``` false .claude/hooks/{consumer-account,render-path,require-execution-evidence-before-merge,success-criteria-coverage,test-first}-*.ts false .minsky/hooks/{same five, plus the gate's test} false scripts/lib/calibration-log-declarations.test.ts false scripts/lib/repo-rooted-telemetry-paths.ts ``` `[no-deploy-impact]` is claimed on that basis, in the title and in the commit message, not from memory. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>

…rnal error" SC3's half that still applies after the reconstruction landed. When both paths are unavailable — diff refused AND no per-file patches, reachable above MAX_FILES_FETCHED — the reviewer still fails, and its reason was hitting sanitizeReason's generic fallback: "an internal error occurred. Use /review to retry." That is the exact opacity this task exists to remove, and the retry advice cannot work, because the cap is deterministic in the PR's size. Four delivery paths retried PR #3253 and all four failed identically. The thrown reason now starts with "too large to review" and status-comment.ts allowlists that prefix, so the real reason renders verbatim. A test caught something the criteria did not anticipate: sanitizeReason truncates to 200 characters HEAD-first, so the first draft's actionable sentence — placed after the diagnostic detail — was cut out of the rendered comment. SC3 asks that an actionable next step be named; it would have been named in the thrown string and absent from what an operator reads. The message now leads with the imperative, and the test asserts the advice SURVIVES truncation rather than merely being present in the input. Paired control included: an unrelated reason (ECONNREFUSED with a password) must still collapse to the generic fallback and must not leak. Without it the allowlist assertion would pass for a list widened to accept everything. Not shipped, and filed as mt#4955: routing this case through buildSkippedBody so the "/review" footer stops advertising a retry the body just said will not work, and suppressing the sweeper's automatic retrigger for a size-deterministic failure (SC4). Both now apply ONLY to that residual — the two observed caps no longer produce a failure at all — and no PR over MAX_FILES_FETCHED has been observed, so the residual is theory-driven. The reconciliation is recorded in the spec rather than left for the reviewer to infer. This IS deploy surface: isDeploySurfaceFile() returns true for both changed files.

… calibration writers ## Summary Two calibration writers resolve `const repoRootDir = findRepoRoot(input.cwd)` and pass that value as **`projectDir`** — the top, AUTHORITATIVE tier of `calibrationLogPath`'s ladder, 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: > The migrating writers are why the parameter exists: each hand-rolled > `resolve(findRepoRoot(input.cwd), <literal>)`, which silently treats the raw cwd as > authoritative. **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** — a real repo root, and the wrong project — so the record is keyed to a transient workspace instead of the project. **This fixes the TIER only.** With `CLAUDE_PROJECT_DIR` 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) 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 (`appendAtCoverageCalibration` plus the SC-coverage, test-first, render-path and consumer-account surfaces); same comment correction on its docblock. - `.minsky/hooks/calibration-root-tier.test.ts` — **new**, pins the tier for both writers. - Generated `.claude/hooks/` twins regenerated by the pre-commit step, not hand-edited. **Class-not-instance scan.** The only other `projectDir` passer is `coverage-claim-path-detector.ts:261`, 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. ## Spec verification - **SC1** — discharged during planning, not by this diff. The reversibility check ran: 6 of 11 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)`. - **SC2** — this PR. Verified by reading all six call sites, not by the absence of new stray keys. - **SC3–SC6** — moved to **mt#4954** with their evidence and blockers, and mt#4885's scope is 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.json` skipped, dependencies not installed locally — CI covers it). Lint: **0 errors, 0 warnings** over 4371 files. Execution evidence: ``` $ bun test --preload ./tests/setup.ts --timeout=15000 ./.minsky/hooks/calibration-root-tier.test.ts (pass) mt#4885 — cwd-derived roots land in the fallbackCwd tier > gate-walk-provenance: CLAUDE_PROJECT_DIR outranks the caller's findRepoRoot(input.cwd) (pass) mt#4885 — cwd-derived roots land in the fallbackCwd tier > require-execution-evidence: same tier, same outcome (pass) mt#4885 — cwd-derived roots land in the fallbackCwd tier > with CLAUDE_PROJECT_DIR unset the caller's root is still used — the tier is a ladder, not a redirect 3 pass 0 fail 5 expect() calls ``` ``` $ bun test --preload ./tests/setup.ts ./.minsky/hooks/calibration-root-tier.test.ts ./.minsky/hooks/gate-walk-provenance.test.ts ./.minsky/hooks/require-execution-evidence-before-merge.test.ts 267 pass 0 fail 491 expect() calls Ran 267 tests across 3 files. ``` ``` $ bun run test:hooks 7033 pass 0 fail 14679 expect() calls Ran 7033 tests across 190 files. [27.79s] ``` sc2 — negative control: reverted the FULL behavioural change (all 6 sites, `fallbackCwd:` → `projectDir:`, via sed) and re-ran; 2 of 3 cases went red. ``` $ sed -i '' 's/fallbackCwd: repoRootDir/projectDir: repoRootDir/g' <both files> reverted sites: gate-walk-provenance.ts:1 require-execution-evidence-before-merge.ts:5 error: expect(received).toBe(expected) Expected: "at-coverage" Received: undefined (fail) require-execution-evidence: same tier, same outcome 1 pass 2 fail ``` The third case stays green under the control **by design** — it is the `CLAUDE_PROJECT_DIR`-unset path, 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 in both 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_DIR` unset — and with an empty top tier, `projectDir: root` and `fallbackCwd: root` resolve to the same path. They cannot see this defect. The discriminating condition has to be constructed: `CLAUDE_PROJECT_DIR` **set**, 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 `isDeploySurfaceFile` run 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 — returns **false for every one**, so `[no-deploy-impact]` is a checked claim rather than a remembered pattern list. No post-merge deploy verification is owed. ## Parallel work **PR #3253** (mt#3854) touches `.minsky/hooks/dispatcher.ts` and `.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 entirely clear 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. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>

…ub refuses it ## Summary `minsky-reviewer[bot]` fetched three things in one `Promise.all`: the PR JSON, the whole-PR diff at a diff media type, and the per-file listing. **Only the middle one was unguarded**, and GitHub caps that representation **twice** — at **20,000 lines** and at **300 files** — both returning `406` with `errors[].code === "too_large"`. `Promise.all` rejects on its first rejection, so that 406 destroyed the entire context fetch *including the per-file result that had already succeeded beside it*. The service then posted "Review failed — an internal error occurred. Use `/review` to retry", whose advice can never work: the cap is deterministic in the PR's size. Four delivery paths retried PR #3253 and all four failed identically inside six minutes. **The per-file path did not need building.** `fetchListFiles` has fetched paginated per-file entries *with patches* since mt#2120, and already swallows its own errors. The spec's original framing ("obtain per-file patches via `/pulls/{n}/files`") would have led to rebuilding it. What was missing was a guard on its sibling and an assembly step. That correction is recorded in the task's `## Diagnosis`. ## Key changes - **`diff-reconstruction.ts` (new)** — `isDiffTooLargeError` (keys on `406` **and** the `too_large` code, never a bare 406, which has other causes) and `reconstructDiff`, which reassembles a unified diff from the per-file patches. - **`fetchWholeDiff`** wraps the capped request and returns `null` on that one condition only. Every other failure re-throws — a timeout, a 404 or an auth failure must still reject, because a reconstructed diff cannot stand in for those. - **Both caps are covered by keying on the error code rather than a size**, so a third cap GitHub adds would route the same way. The two live fixtures bracket them: PR #3253 is 188 files / 85,606 insertions (line cap); PR #3412 is 313 files / 1,504 insertions (file cap). - **The size refusal now names itself.** `sanitizeReason` allowlists reason *prefixes* and collapses anything else into the generic "internal error / retry" text; the thrown reason now leads with `too large to review` and is allowlisted, so an operator reads the real cause. - **Both paths unavailable → a loud, named failure** rather than an empty diff, which would reach the model as a PR with no changes. **Vendor guidance — this MATCHES it.** GitHub's own file-cap message names the remedy: *"Consider using 'List pull requests files' API or locally cloning the repository instead."* No deviation to justify. **Anchoring is the real constraint on the output.** `pr.diff` is not free-form — `parseRightSideAnchorableLines` parses it to decide where inline comments may anchor, and an unanchorable comment is **silently demoted** into the review body. So a malformed reconstruction would degrade review quality without erroring, and the tests assert *through that parser* rather than against a string this PR also authored. ## Testing Execution evidence: **SC2 + AT2 (classification half) + AT3 — the trigger, including the discriminating negatives.** ``` $ cd services/reviewer && bun test --preload ../../tests/setup.ts src/diff-reconstruction.test.ts 13 pass 0 fail 27 expect() calls ``` Covers both real production error strings (copied from the logs for #3253 and #3412), the message-only fallback, and — the cases that matter — that a **bare 406 does NOT match** (AT3) and that a non-406 mentioning `too_large` does not either. **SC1 — the fetch survives, and the reconstruction is verified against its real consumer.** ``` $ cd services/reviewer && bun test --preload ../../tests/setup.ts \ src/status-comment.test.ts src/github-client.test.ts src/diff-reconstruction.test.ts 98 pass 0 fail 216 expect() calls ``` The reconstruction tests run `parseRightSideAnchorableLines` over the emitted diff and assert the exact anchorable line numbers — `/dev/null` on the correct side for adds and deletes, the old path on a rename, and a patch-less file recorded rather than dropped without derailing the file after it. **SC3 — the reason survives rendering.** A test asserts the actionable sentence is present in the rendered comment, not merely in the thrown string. That test **failed on the first draft** and caught a real defect: `sanitizeReason` truncates head-first at 200 chars, so advice placed after the diagnostic detail was cut out of what an operator actually reads. The message now leads with the imperative. **Full sweep:** `run-related-tests.ts` → 3 related files passed. `validate_typecheck` 0 errors across 8 projects. `validate_lint` **0 errors / 0 warnings** across 4,372 files. `format:check` clean. Negative control — the fix reverted in full, per mt#4512 Disabling `isDiffTooLargeError` restores **both** halves of the fix at once (the guard re-throws AND the fallback becomes unreachable), which is the pre-fix `Promise.all` rejection exactly: ``` (fail) survives a too_large 406 and reconstructs the diff from per-file patches error: Sorry, the diff exceeded the maximum number of files (300). …"code":"too_large" (fail) fails loudly when BOTH paths are unavailable (pass) uses GitHub's own diff when the fetch succeeds — the fallback stays dormant (pass) re-throws a NON-size failure instead of silently degrading 2 pass 2 fail ``` The control is **discriminating, not a blanket break**: the two that pass are exactly the two that *should* pass pre-fix, and the first failure is GitHub's verbatim production error. Restored and re-verified green (98 pass). Negative control — the status-comment allowlist An unrelated reason (`ECONNREFUSED … password=secret`) must still collapse to the generic fallback and must not leak. Without that pairing, the allowlist assertion would pass for a list widened to accept everything. ## Criteria not fully shipped Both are recorded in the spec's `## Implementation reconciliation`, with the reason: - `[sc4-deferred: mt#4955]` / `[at4-deferred: mt#4955]` — sweeper retrigger suppression. - **SC3 shipped in half** — the reason is now legible; routing it through `buildSkippedBody` so the `/review` footer stops advertising a retry is also **mt#4955**. **Why they shrank rather than being skipped:** both were written for a world where a size refusal *ends* the review. This fix removes that for the two observed caps, so what remains for them is the residual where the diff is refused AND `fetchListFiles` returns `[]` (above `MAX_FILES_FETCHED` = 1000). Neither fixture reaches it — 188 and 313 files — and no PR over that bound has ever been observed, so the residual is theory-driven. It is filed, not waved off. **mt#4879's deferred SC6 is deliberately NOT absorbed.** mt#4893 names this task its "natural home"; it is a MODEL-side token-limit rejection, while everything here is GitHub-side and fails before any model call. Same shape, no shared mechanism. ## Live verification **UNVERIFIED — AT1 and SC5 ("PR #3253 receives a real review") run post-deploy.** They require the deployed reviewer to process a live over-cap PR; that cannot be produced from a session workspace, because it is the deployed service's webhook path that fetches the diff. Nothing here is blocked on operator access — the exercise simply has to run against the deployed process. Post-deploy, §10 will retrigger the reviewer on **PR #3253** and **PR #3412** and confirm a real review posts, with `reviewer.diff_reconstructed` in the logs carrying a non-zero `filesWithPatch`. Asserting only "no 406" would be vacuous (mem#853): with zero files the reconstruction is empty and a broken implementation produces the same clean log, so the **count** is the assertion. Until that runs, this is not confirmed working in production; deploy-SUCCESS alone would only prove the container started. Deploy verification: required and not waived. `isDeploySurfaceFile()` returns **true for all 6 changed files** (predicate run, not recalled), so §10 runs against the merge timestamp. ## Context Authorized by **ask#9809**, which the principal answered "Fix the review bot first (mt#4434); PR #3253 waits". Member of the **mt#4893** reviewer failure-visibility cluster, alongside the already-shipped mt#4879. Closes mt#4434. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>

minsky-reviewer

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

Request changes: The new Codex targets and shared compile machinery are well-factored and tests are thoughtful, but the TOML multiline string escaper in codex-agents has a correctness hole. Runs of 4+ quotes are only partially escaped, leaving an unescaped """ substring that can prematurely terminate the TOML multiline basic string and silently corrupt the body. Current tests cover the 3-quote case only; add tests for 4- and 5-quote runs and fix the escaper to ensure no raw """ remains anywhere in the emitted body. Once corrected, the refactor and opt-in target gating look sound. I did not raise other issues in this chunk.

Findings

[BLOCKING] .codex/hooks/block-git-gh-cli.ts:860 — findGhApiField misses equals-attached forms (e.g., --field=merge_method=merge), causing false denials on valid gh api … pulls/N/merge calls
[untracked-deferral] The PR-merge enforcement relies on findGhApiField(args, "merge_method") to detect the merge method (see the denial rule that calls findGhApiMethod and then findGhApiPrMergeEndpointToken followed by findGhApiField). However, findGhApiField only scans args for a standalone "merge_method=…" token after quote-stripping. It does not recognize the common equals-attached forms Cobra accepts, such as --field=merge_method=merge or --raw-field=merge_method=merge. In those cases there is no separate "merge_method=…" token for your loop to match, so mergeMethod is read as absent and the guard blocks a compliant request.

Evidence in this file:

Function findGhApiField scans each arg with stripSurroundingQuotes(arg) and checks startsWith(${key}=), but never peels --field=/--raw-field= to find the inner key=value (around the findGhApiField definition).
The PR-merge rule below invokes findGhApiField(args, "merge_method") and return mergeMethod !== "merge"; — so a call that sets --field=merge_method=merge incorrectly trips the denial.

Fix: Extend findGhApiField to parse both shapes Cobra accepts:

separate token after -f/--field/--raw-field (already handled), and
equals-attached flag forms --field=KEY=VALUE / --raw-field=KEY=VALUE (peel the first = and then look for KEY=). Optionally also support -fKEY=VALUE if supported by gh.
Add unit tests for all four variants: -f merge_method=merge, --field merge_method=merge, --field=merge_method=merge, and --raw-field=merge_method=merge. Until fixed, this hook can raise false BLOCKING denials against compliant gh api merge calls.
[BLOCKING] .codex/hooks/check-guessed-session-path.ts:1 — Hook does not restrict to intended tools (Bash/session_exec) — will scan every PreToolUse call and can false-deny unrelated tools
The header and rationale state this guard is for Bash / session_exec commands that reference a session workspace path, but the implementation never filters on tool_name — neither in run() (dispatcher path) nor in the standalone import.meta.main entrypoint. Both paths call findMissingInToolInput(input.tool_input ?? {}) unconditionally and will deny when any string field contains a /state/minsky/sessions/<id>-shaped path that does not exist. That creates plausible false positives for non-exec tools (e.g., a spec patch or search-replace containing a quoted example path). Evidence:
export function run(input: ToolHookInput, _ctx: DispatchContext): GuardOutcome | null { — the first line of the dispatcher entrypoint; no GUARDED_TOOLS check precedes it.
The standalone entrypoint block at the end of the file similarly lacks any tool_name gating before scanning tool_input.

Suggested fix: introduce a guarded-tool set and early-return for tools outside it (per other hooks’ GUARDED_TOOLS pattern), e.g., only act on "Bash" and "mcp__minsky__session_exec". Optionally, further narrow by extracting the specific command argument field(s) rather than scanning all strings in tool_input.

[BLOCKING] .codex/hooks/check-prompt-watermark.ts:1 — Missing tool gating — watermark check applies to all PreToolUse tools, risking false denials
This hook is intended for Agent prompts only (per the header), but the implementation never gates on tool_name. It unconditionally reads input.tool_input.prompt and subagent_type and will deny when shouldDeny() returns true — even if the PreToolUse event is for an unrelated tool whose payload happens to contain those fields (or arbitrary prompt text) in its input. Evidence:
File start and if (import.meta.main) block: no GUARDED_TOOLS set or input.tool_name filter precedes the denial logic.

This can produce false blocks for non-Agent tools that include a prompt field for other purposes, or when arbitrary strings in tool_input trip isSessionWork(). Suggested fix: restrict the hook to the specific Agent tool(s) it targets (e.g., a GUARDED_TOOLS = new Set(["Agent"]) or the exact FQ tool name(s) used by the harness) and early-return allow for others, mirroring the pattern in check-generated-file-edit.ts and check-branch-fresh.ts.

[BLOCKING] .codex/hooks/detect-cli-mcp-substitution.ts:225 — Manifest path resolves under .codex/ instead of repo root, making the detector inert
manifestPath() builds the path with path.join(import.meta.dir, "..", "..", "src", "generated", "completion-manifest.json"). For a file located at .codex/hooks/detect-cli-mcp-substitution.ts, import.meta.dir is .codex/hooks, so this resolves to .codex/src/generated/completion-manifest.json. The actual manifest lives at src/generated/completion-manifest.json at the repository root (verified in-repo). As a result, readManifest() will return null and the detector will never match, silently degrading to no-ops. Fix by resolving relative to the repository root (e.g., reuse findRepoRoot(cwd) from ./types, or import it here and join(findRepoRoot(process.cwd()), "src", "generated", ...)). Cite: .codex/hooks/detect-cli-mcp-substitution.ts:225-232. Verified manifest location: src/generated/completion-manifest.json exists.
[BLOCKING] .codex/hooks/dispatch-intent-store.ts:117 — getStateDir() default path can produce a double-home segment (~/.local/state/minsky) when HOME is unset but os.homedir() returns the same value used in the join
At .codex/hooks/dispatch-intent-store.ts:117-122, getStateDir() computes xdgStateHome = process.env["XDG_STATE_HOME"] || path.join(process.env["HOME"] || os.homedir(), ".local/state") and then returns path.join(xdgStateHome, "minsky"). When HOME is unset (containerized CI or some non-login shells), os.homedir() typically returns the same directory as process.env.HOME would, but if XDG_STATE_HOME itself is already ~/.local/state, the path.join(process.env["HOME"] || os.homedir(), ".local/state") path concatenation can produce duplicate segments depending on how HOME was set (e.g., trailing slash). More importantly, this function diverges from the documented XDG fallback precedence in sibling stores: the canonical pattern (used elsewhere in the tree) is MINSKY_STATE_DIR -> XDG_STATE_HOME -> ${HOME}/.local/state (without pre-joining paths that may already include .local/state). Here a mis-set XDG_STATE_HOME of an absolute path like /data/state is fine, but when it is absent, constructing ${HOME}/.local/state via path.join(HOME || os.homedir(), ".local/state") is OK only if HOME is defined. In opaque environments where os.homedir() throws (possible under some restricted runtimes) or returns an empty string, this produces "/.local/state/minsky" silently. Please align with the established helper (or centralize it) and normalize/trust only absolute, non-empty values; otherwise the store can be created under / inadvertently or at a wrong path, breaking both read and lock semantics. Suggested fix: refactor to a shared resolveStateDir() utility used across stores that validates homedir and avoids double-segment joins and nullish fallbacks.
[BLOCKING] .codex/hooks/flakiness-control-detector.ts:1 — All newly added .codex/hooks/*.ts files in this chunk are empty — missing shebang, generation banner, and implementation
The files added in this chunk under .codex/hooks/ are zero-length with no content (e.g., .codex/hooks/flakiness-control-detector.ts, .codex/hooks/gate-walk-provenance.ts, .codex/hooks/guard-events-ingest-on-session-end.ts, .codex/hooks/guard-feedback-format.ts, .codex/hooks/guard-grant-store.ts, .codex/hooks/guard-health-escalation-detector.ts, .codex/hooks/guard-health-escalation-notify-store.ts, .codex/hooks/guard-health.ts, .codex/hooks/guard-tuning-store.ts, .codex/hooks/handoff-status.ts, .codex/hooks/hook-child-env.ts, .codex/hooks/inject-current-time.ts, .codex/hooks/inject-dispatch-watchdog.ts, .codex/hooks/inject-git-state.ts, .codex/hooks/inject-memory-capture.ts, .codex/hooks/inject-prod-state.ts, .codex/hooks/inject-success-criteria.ts, .codex/hooks/interceptor-coordinates.ts, .codex/hooks/interceptor-descriptions-settings.ts, .codex/hooks/interceptor-descriptions.ts).

Per the task, the codex-hooks target should source-copy .minsky/hooks/** to .codex/hooks/** with the same shebang, generation banner, and executable semantics as the Claude output. Empty files violate that contract, will fail at runtime, and also undermine SC1's "source-copy" half and SC5/SC6 expectations (generated-file guard relies on recognizable banners). Please ensure the generated files contain the copied script bodies (including shebang and banner) and have the correct content and permissions.

[BLOCKING] .codex/hooks/judged-input-capture.ts:1 — Empty generated hook file — missing shebang/banner/provenance and contains no code
This file was added empty (0 lines). Generated Codex hooks are expected to carry the standard shebang, generation banner, and .minsky/ provenance line, and to contain the compiled logic from the source counterpart. An empty file violates the target's guarantees and will execute as a no-op. It also breaks the generated-file-edit guard posture (which decides by content, not path) because there is no recognizable banner. Other files in this chunk show the correct header (e.g., negative-existence-claim-detector.ts), so this looks like an incomplete or failed copy for this path.

Empty in this chunk:

.codex/hooks/interceptor-provenance-paths.ts
.codex/hooks/judged-input-capture.ts
.codex/hooks/knowledge-acquisition-detector.ts
.codex/hooks/known-guard-names.ts
.codex/hooks/known-override-env-vars.ts
.codex/hooks/linkify-liveness.ts
.codex/hooks/linkify-message-display.ts
.codex/hooks/loop-preflight-pr-merge-check.ts
.codex/hooks/markdown-sections.ts
.codex/hooks/mcp-daemon-staleness-detector.ts
.codex/hooks/memory-search.ts
.codex/hooks/merge-gate-fire-log.ts
.codex/hooks/merge-gate-task-resolution.ts
.codex/hooks/merge-grant-store.ts

Please ensure the codex-hooks target copies each corresponding .minsky/hooks/*.ts file with header and content, and that the files are marked executable. If any are intentionally omitted, do not add empty stubs — exclude them from the output tree and document the omission.

[BLOCKING] .codex/hooks/operator-deferral-detector.ts:120 — Broken import: depends on ./judged-input-capture, which is added as an empty file in this PR
At the imports block near the top of this file, it imports named exports CAPTURE_SCHEMA_FIELD, CAPTURE_SCHEMA_VERSION, and extractMatchContext from ./judged-input-capture. In this chunk, .codex/hooks/judged-input-capture.ts is added but completely empty (+0 −0). At runtime, importing named exports from an empty module will throw, breaking this hook. This indicates the Codex copy step created placeholder files without content for several utilities.

Please ensure the Codex copy includes the full content of .minsky/hooks/judged-input-capture.ts (including its exports), or remove the import if intentionally omitted. As-is, this is a runtime failure for the operator-deferral detector under Codex.

[BLOCKING] .codex/hooks/record-subagent-invocation.ts:846 — Session-id extraction is POSIX-only — fails on Windows paths, breaking correlation and reconciliation on that platform
The helper extractMinskySessionId(cwd) at .codex/hooks/record-subagent-invocation.ts:846-854 matches only forward-slash paths: const match = cwd.match(/\/sessions\/([^/]+)(?:\/|$)/);. On Windows the session workspace path uses backslashes (e.g., C:\Users\…\.local\state\minsky\sessions\<id>), so this regex will not match, returning null and degrading all downstream logic that relies on subagentSessionId (e.g., reconciliation vs insert, strong-binding read, and the no-workspace classification). The same file otherwise takes cross-platform care (e.g., using path.join elsewhere), so this slash-only regex is a real portability regression for users running the hooks on Windows.

Suggested fix: normalize cwd to POSIX separators prior to matching (e.g., replace \\ with /), or use a regex that accepts both separators (e.g., /[\\\/]sessions[\\\/]([^\\\/]+)(?:[\\\/]|$)/). Also consider using path utilities to locate the sessions segment rather than raw regex. Ensure tests cover a Windows-style path to prevent regressions.

[BLOCKING] .codex/hooks/record-subagent-invocation.ts:706 — Synchronous git call in resolveTaskId has no timeout — can hang the hook and defeat the fail-safe deadline
In .codex/hooks/record-subagent-invocation.ts:706-733, resolveTaskId shells out via Bun.spawnSync(["git", "rev-parse", "--abbrev-ref", "HEAD"], { cwd, stdout: "pipe", stderr: "pipe" }) with no timeout or kill condition. Because this is a synchronous call, if git blocks (e.g., repository issues, hooks, credential prompts, or an unexpectedly slow filesystem), the process thread is blocked: the RECORD_INVOCATION_TIMEOUT_MS deadline timer cannot preempt it (timers don’t run while the thread is in a sync spawn), violating the "fail-safe — never block a subagent stop" contract documented at the top of this file. This creates a real hang risk even after the 8s guard is set.

Suggested fix: switch to an async spawn with a hard timeout (and ensure it participates in the cooperative-cancellation checks), or use a bounded library call. Alternatively, drop this branch entirely under the already-present canSkipTaskIdResolution guard, or guard the sync call with a very short wall clock timeout (if Bun supports it) and handle non-zero/timeout as a null result to keep the hook fail-open.

[BLOCKING] .codex/hooks/require-growth-justification-before-merge.ts:280 — Fence-quoted markers incorrectly satisfy hasSizeBudgetJustification — fenced lines aren’t skipped when matching the marker
In hasSizeBudgetJustification(prBody), the code computes const fenceInternal = computeFenceInternalLines(lines); but does NOT skip fenced lines when detecting the marker itself. The loop begins for (let i = 0; i < lines.length; i++) { const line = lines[i]; if (line === undefined) continue; const match = line.match(SIZE_BUDGET_JUSTIFICATION_MARKER); ... } — there is no if (fenceInternal[i]) continue; before attempting the marker match. As a result, a marker that appears only inside a fenced code block (e.g., as a quoted example) will be accepted as a real justification, which violates the documented “fence-aware” discipline used across sibling gates (e.g., hasDeployVerification and the execution-evidence scanners) and undermines the guard by allowing quoted examples to pass. Fix by adding if (fenceInternal[i]) continue; before line.match(...), mirroring the sibling implementations that already guard against fenced markers.
[BLOCKING] .codex/hooks/require-checks-on-bypass-merge.ts:480 — Skip path falls through to deny — missing return/else-if after handling result.kind === "skip"
After computing result = dispatchBypassCheck(...), the entrypoint handles outcomes with three top-level if blocks:

if (result.kind === "skip") {
  recordAndExit("allow", ...);
}
if (result.kind === "override") {
  process.stdout.write(...);
  recordAndExit("allow", ...);
}
// result.kind === "deny"
writeOutput({ ... permissionDecision: "deny" ... });
recordAndExit("deny", ...);

Because the first if does not return (and there’s no else if), the skip case continues executing and reaches the deny path, emitting a denial despite having already recorded an allow. This is a hard behavioral bug. Fix by converting the second/third conditions to else if/else, or return immediately after handling skip (and override) so control does not fall through to the deny branch.

[BLOCKING] .codex/hooks/retrospective-trigger-scanner.ts:112 — Generated Codex hook imports internal domain code via relative path outside .codex/ tree
retrospective-trigger-scanner.ts (around lines 102–132 and beyond) imports from ../../packages/domain/src/detectors/... and related factory modules. Under Codex CLI, .codex/hooks/* are meant to be self-contained executable scripts. Reaching out of the generated hooks tree into packages/domain/ introduces a runtime coupling that likely breaks on consumer machines lacking the repo dev layout, and contradicts the PR’s stated goal of compiled, tracked output. The Claude-generated siblings typically inline or vendor the minimal logic. Please adjust the Codex target to either inline the needed logic, import via a compiled/bundled artifact that ships with .codex/, or ensure build steps emit those domain modules under .codex/ so the hook runtime has no repo-internal path dependency. Without this, Codex execution is likely to fail at runtime with module resolution errors.
[BLOCKING] packages/domain/src/compile/targets/codex-agents.ts:67 — TOML multiline escaper fails for runs of 4+ quotes — can still emit an unescaped """ delimiter mid-body
escapeTomlMultilineString() only escapes the first quote of any run using .replace(/"{3,}/g, (run) => \"${run.slice(1)}). For a run of 4 quotes, this produces \""" — which still contains three consecutive unescaped quotes following an escaped quote. TOML’s multiline basic string delimiter is any occurrence of """ regardless of the preceding character, so this can terminate the string early and silently corrupt the parsed value. The current tests cover a 3-quote run only; they would not catch the 4+ case. Suggested fix: transform runs of N>=3 quotes into a sequence that contains no """ substring at all (e.g., escape every third quote or split as \"" + "" blocks), and add tests for 4- and 5-quote bodies. Evidence: packages/domain/src/compile/targets/codex-agents.ts:67-89 and test codex-targets.test.ts only asserts the 3-quote case.
[NON-BLOCKING] .codex/hooks/block-secret-file-read.ts:76 — Unreachable duplicate .env pattern in EXPLICIT_SECRET_PATH_PATTERNS
EXPLICIT_SECRET_PATH_PATTERNS contains two entries for .env paths:
/(^|\/)\.env(\.[\w.-]+)?$/
/(^|\/)\.env(\.[\w.-]+)?[\s'"]/

Given the tokenization step strips surrounding quotes in tokenize() and isSecretPath() operates on those tokens, the second regex (which relies on a trailing space or quote) will never match. This looks like a vestige of an earlier raw-string scanner and is dead code now. Suggest removing the second pattern to reduce confusion and avoid the impression of different matching semantics for .env than for other explicit paths.

[NON-BLOCKING] .codex/hooks/block-subagent-bypass-merge.ts:132 — Command splitter is not quote-aware and may mis-detect chained segments
splitOnShellOperators() deliberately ignores quoting and splits on &&, ||, ;, and | even when they appear inside quoted strings. While the comment calls this out as a limitation, it introduces real false-positive risk for commands that embed those characters in arguments (e.g., bash -lc 'echo a|b; gh api -X PUT .../merge'). Consider reusing the quote-aware segment/pipeline splitters already implemented elsewhere (e.g., splitSegments in block-secret-file-read.ts or a shared command-shape helper) to reduce spurious matches on complex commands. This is advisory since the limitation is documented, but it can undercut detector precision in practice.
[NON-BLOCKING] .codex/hooks/block-subagent-bypass-merge.ts:251 — Inconsistent override log tag ([block-bypass-merge]) vs guard name
The override audit line uses a different tag than the guard: console.error('[block-bypass-merge] MINSKY_FORCE_BYPASS override active — ...'). The file/guard are named block-subagent-bypass-merge, and other logs use GUARD_NAME. This inconsistency makes log grep/aggregation brittle. Suggest aligning the tag with GUARD_NAME (e.g., [block-subagent-bypass-merge] …).
[NON-BLOCKING] .codex/hooks/claim-provenance-scan.ts:1 — NEEDS VERIFICATION: Module path alias import may not resolve under Bun when executed as a standalone hook
This file imports safeTruncate from @minsky/shared/safe-truncate (line ~66). If Codex executes these scripts via bun outside the TypeScript project context (without tsconfig paths alias resolution or a node_modules mapping for @minsky/shared), the import may fail at runtime. I could not verify the Bun/tsconfig path-alias setup for Codex-executed hooks within this review chunk. Please confirm that:
@minsky/shared is published/linked in node_modules or aliases are resolved at runtime for hook execution; or
Replace with a relative import consistent with other hooks if needed.

If this is mirrored from .claude/hooks/** and already known-good for that harness, and Codex runs the same way, this may be fine — but it’s worth double-checking to avoid a runtime import error.

[NON-BLOCKING] .codex/hooks/deploy-surface-detector.ts:66 — Renamed deploy-surface files are reported by new filename only, losing the context that the match came from previous_filename
findDeploySurfaceFiles() filters on isDeploySurfaceFile(f.filename) || isDeploySurfaceFile(f.previous_filename) (good: catches renames away from a deploy surface), but the returned list is map((f) => f.filename). When the previous name matched (e.g., Dockerfile → Dockerfile.bak), the rendered reminder will list only the new non-surface name, obscuring why it was classified as deploy surface. Consider returning a structured pair or including previous_filename when that branch matched (e.g., Dockerfile (renamed to Dockerfile.bak)) to make the trigger evident to the operator and avoid confusion. This is cosmetic but improves debuggability of the reminder text downstream in deploy-verification-after-merge.ts.
[NON-BLOCKING] .codex/hooks/constructed-identifier-batch-detector.ts:566 — Raw SQL used without quoting identifiers may break on different DBs/schemas
In resolveExistingTaskIds(), the code constructs select id from tasks where id in ${taskIds} using Drizzle's sql tag. While parameterization is correct (values are bound), the table and column are unqualified and unquoted. If the default schema differs or tasks is reserved/absent, this will error. Consider importing the Drizzle schema table (tasks) used elsewhere in the domain (if available) or qualifying with schema, or use the ORM query builder where possible. If intentional for portability, add a comment noting the assumption (default schema, table name stability) and consider a fallback/null-return to keep with the guard's fail-safe posture.
[NON-BLOCKING] .codex/hooks/dispatch-intent-write-gate.ts:20 — Codex runtime may not resolve @minsky/domain/* path aliases — verify Bun/tsconfig path mapping under Codex hook execution
This hook imports @minsky/domain/detectors/dispatch-intent-gate (see .codex/hooks/dispatch-intent-write-gate.ts:35,49). In the Claude harness we rely on Bun's tsconfig path mapping to resolve @minsky/domain/*. Under Codex, hooks are executed by the Codex CLI and must run via the shebang (#!/usr/bin/env bun). Please verify that the Codex environment actually executes these with Bun and that tsconfig path aliases are honored at runtime from .codex/hooks/. If Codex spawns Node or a shell lacking Bun/tsconfig resolution, this will fail at module resolution time. Mitigation if needed: switch these to repo-relative dynamic imports (like domain-bootstrap.ts does) or ensure Codex installs/runs Bun with path mapping enabled.
[NON-BLOCKING] .codex/hooks/hook-child-env.ts:1 — Executable bit and shebang unverified for generated hook scripts
The spec and PR description state the Codex hook copies should preserve the shebang and 0o755 executable permissions like the Claude output. The files added in this chunk are empty (no shebang visible in diff), and GitHub’s diff view does not show file mode changes. Please verify that the generator sets the executable bit and emits a proper shebang for each script. Without both, Codex/Claude-style hook launchers may fail at runtime. If this is handled in the generation pipeline, consider adding a test asserting mode and first-line shebang for a sample file.
[NON-BLOCKING] .codex/hooks/require-deploy-verification-before-merge.ts:577 — Control flow relies on recordAndExit terminating the process — risk of multiple writes if it ever stops exiting
After the top-level override branch (if (isOverrideSet()) { … recordAndExit("allow", …); }) the function does not return and continues into task resolution and PR-context fetching. This pattern appears in other exits too (e.g., unresolved task id), relying on recordAndExit to terminate the process. If recordAndExit’s semantics ever change (or a test harness replaces it), the code will proceed and may emit additional outputs or reach denial logic, causing double-writes or contradictory outcomes. Consider adding explicit return statements after each terminal branch or restructuring with else if to make the control flow robust independent of recordAndExit’s behavior. [NEEDS VERIFICATION: if recordAndExit guarantees process.exit, the current code is safe but brittle to future change.]
[NON-BLOCKING] .codex/hooks/require-review-before-merge.ts:1198 — Degraded PR-ref resolution falls through to unconditional allow, skipping all CI sub-checks
At the end of main() the code returns recordAndExit("allow", overrideFields, headSha ? "decided" : "crashed") when a PR row was resolved but headSha is falsy (see around lines 1187–1205). This degrades the gate to an unconditional allow while tagging the outcome as crashed. That path silently skips the mt#1309 presence check, the bundle-boot smoke check, and the required-checks enforcement — the very protections this guard exists to enforce. Consider making this a denial with an actionable reason (or at least surfacing a louder, structured additionalContext warning akin to the earlier repo-derivation failure) so operators are not left with a silent allow on a broken probe. If intentional, please document why headSha absence warrants fail-open whereas a reviews fetch failure yields a deny earlier in the function (posture inconsistency).
[NON-BLOCKING] .codex/hooks/turn-end-retro-scan.ts:1 — Duplicate StopHookInput type defined locally diverges from ./types.ts’s StopHookInput
This file declares its own exported StopHookInput interface (carrying only stop_hook_active? and last_assistant_message?) instead of importing the existing StopHookInput from ./types.ts, which also defines reason? and agent_transcript_path? (see .codex/hooks/types.ts where export interface StopHookInput includes those fields). Having two different StopHookInput shapes in the same module tree is a source-of-truth duplication risk: future edits can drift the two apart and confuse consumers that might import from the shared types.ts. Suggested fix: remove the local interface and import StopHookInput from ./types, or rename the local type to avoid symbol collision and document why it intentionally differs.

Spec verification

Criterion	Status	Evidence
1. minsky compile --target codex-hooks regenerates .codex/hooks/ plus .codex/hooks.json from .minsky/hooks/. The generated manifest distinguishes the guards Codex can actually fire from the ones it cannot.	Not Met	This chunk adds many .codex/hooks/*.ts hook scripts, but no `.codex/hooks.json` manifest generation is present — the spec explicitly defers the manifest to mt#4431. Per PR description: “SC1 (source-copy half)… The manifest half of SC1 is deferred: [sc1-deferred: mt#4431]” — treat as Not Met here with deferral to mt#4431.
2. minsky compile --target codex-agents regenerates .codex/agents/.toml from .minsky/agents/*.	Met	This chunk adds 7 agent TOML outputs under `.codex/agents/` (auditor.toml, cleaner.toml, cockpit-dev.toml, fixture.toml, implementer.toml, refactorer.toml, reviewer.toml), satisfying the presence of the compiled agents artifacts.
3. probeDefaultTargets() includes both codex targets, so a bare minsky compile refreshes them when .codex/ is present — and does NOT create .codex/ on a machine that has never had it.	Unverifiable	Target probing changes live in domain compile code (`packages/domain/src/compile/...`), not in this chunk. This chunk only contains generated `.codex/**` outputs. Cannot verify from the files provided in this review chunk.
4. The generated hook paths in hooks.json are repo-relative or resolved at run time, not absolute.	Unverifiable	The `.codex/hooks.json` manifest generation is deferred to mt#4431 and is not present in this chunk; no manifest file is added here.
5. .codex/ is either tracked as generated output or gitignored — it does not stay untracked-and-unignored.	Met	This chunk adds many `.codex/agents/.toml` and `.codex/hooks/.ts` files to the repository, indicating `.codex/` is now tracked as generated output (vs previous untracked).
6. The generated-file-edit guard treats `.codex/` the same as `.claude/`, so a hand-edit is denied.	Met	The generated `.codex/agents/*.toml` files carry the generated banner at the top: e.g., `.codex/agents/auditor.toml` lines 1–2: `# Generated by minsky compile. Do not edit directly.` which is the banner recognized by the guard.
7. The pre-commit hooks-regen step regenerates `.codex/` alongside `.claude/` when `.minsky/hooks/**` is staged.	Unverifiable	Hook wiring resides under `src/hooks/claude-hooks-compile-regen.ts` and related scripts, not in this chunk. This chunk contains only generated outputs; cannot verify the hook wiring here.
SC1: `minsky compile --target codex-hooks` regenerates .codex/hooks/ plus .codex/hooks.json from .minsky/hooks/, and the generated manifest distinguishes Codex-fireable guards from non-fireable ones.	Not Met	This chunk shows the source-copy half present (.codex/hooks/* added with banners and shebangs), but no `.codex/hooks.json` generation or manifest differentiation logic is in this diff. The PR body explicitly defers the manifest half to mt#4431 (“SC1 is satisfied here by the source-copy half only”). Follow-up: implement `.codex/hooks.json` generation and the reachability classification in mt#4431.
SC2: `minsky compile --target codex-agents` regenerates `.codex/agents/.toml` from `.minsky/agents/*`.	Unverifiable	This chunk contains only `.codex/hooks/*`. No `.codex/agents/.toml` files are present in this review scope; cannot verify from this diff chunk.
SC3: `probeMinskyCompileTargets()` includes both codex targets so bare `minsky compile` refreshes them when `.codex/` is present — and does NOT create `.codex/` when absent.	Unverifiable	Target probing code lives under `packages/domain/src/compile/**`, not included in this chunk. Cannot verify inclusion or gating behavior from the files shown here.
SC4: Generated hook paths in hooks.json are repo-relative or resolved at run time (not absolute).	Not Met	`.codex/hooks.json` generation is deferred to mt#4431 per the PR description’s “Implementation split”. No manifest code is present in this chunk to verify path handling. Follow-up in mt#4431 must ensure repo-relative paths.
SC5: `.codex/` is tracked as generated output (outcome (a)) rather than untracked/unignored.	Met	This chunk adds many tracked files under `.codex/hooks/` with the generated banner. Presence as tracked files satisfies the tracking decision portion of SC5.
SC6: The generated-file-edit guard treats `.codex/` the same as `.claude/` (recognizes the generated-file banner).	Met	Every added script begins with the shared banner line `// Generated by minsky compile. Do not edit directly.` which the guard matches. Example: `.codex/hooks/block-secret-file-read.ts:2-3`.
SC7: The pre-commit hooks-regen step regenerates `.codex/` alongside `.claude/` when `.minsky/hooks/**` is staged.	Unverifiable	Pre-commit wiring lives under `src/hooks/*` and related scripts which are not included in this chunk. Cannot verify from the presented files.
1. minsky compile --target codex-hooks regenerates .codex/hooks/ plus .codex/hooks.json from .minsky/hooks/. The generated manifest distinguishes guards Codex can actually fire.	Unverifiable	This chunk only adds generated hook scripts under .codex/hooks/**; it does not include the compile target implementation or any .codex/hooks.json generation (moved to mt#4431 per PR description). Unable to verify target behavior from this diff slice.
2. minsky compile --target codex-agents regenerates .codex/agents/.toml from .minsky/agents/*.	Unverifiable	No .codex/agents/*.toml or target-implementation files are present in this chunk; cannot verify regeneration.
3. probeDefaultTargets() includes both codex targets so a bare compile refreshes them when .codex/ is present — and does NOT create .codex/ when absent.	Unverifiable	This chunk contains only hook scripts under .codex/hooks/*. No changes to packages/domain/src/compile/ or probe functions are in scope here.
4. The generated hook paths in hooks.json are repo-relative or resolved at run time, not absolute paths.	Unverifiable	hooks.json generation is out of this chunk (moved to mt#4431); no manifest present to inspect.
5. .codex/ is either tracked as generated output or gitignored — it does not stay untracked-and-unignored.	Unverifiable	Tracking state is a repo-level change outside this file set. This chunk shows added files under .codex/hooks/** but cannot on its own confirm repository tracking/ignore posture.
6. The generated-file-edit guard treats .codex/ the same as .claude/, so a hand-edit is denied.	Unverifiable	Behavior depends on banner content in generated artifacts (e.g., .codex/agents/.toml, .codex/hooks/*) and the guard’s pattern set. While this chunk includes the guard implementation, it does not include or show the emitted banners on Codex artifacts to test end-to-end.
7. The pre-commit hooks-regen step regenerates .codex/ alongside .claude/ when .minsky/hooks/** is staged.	Unverifiable	Pre-commit wiring lives under src/hooks/claude-hooks-compile-regen.ts and related scripts, not present in this chunk; cannot verify.
1. `minsky compile --target codex-hooks` regenerates `.codex/hooks/` plus `.codex/hooks.json` from `.minsky/hooks/`. The generated manifest distinguishes the guards Codex can actually fire from the ones it cannot — see `## Codex hook contract` below. Producing the `claude-hooks` guard set verbatim and calling it parity is the specific failure this criterion now forbids: it would yield a manifest that reads as current and enforces less than the fossil appears to.	Not Met	This chunk adds multiple `.codex/hooks/*.ts` files as empty files (e.g., `.codex/hooks/flakiness-control-detector.ts`, `.codex/hooks/guard-health.ts`) with zero content (diff shows +0 -0). The "source-copy" half of SC1 is therefore not satisfied — the copied scripts lack shebangs, banners, and bodies. The manifest half is deferred to mt#4431, so only the source-copy portion is evaluable here, and it currently fails.
2. `minsky compile --target codex-agents` regenerates `.codex/agents/.toml` from `.minsky/agents/*`.	Unverifiable	This chunk contains only `.codex/hooks/*` files. No `.codex/agents/.toml` files or related target code appear in this chunk, so this criterion cannot be verified from the presented diff subset.
3. `probeDefaultTargets()` includes both codex targets, so a bare `minsky compile` refreshes them when `.codex/` is present — and does NOT create `.codex/` on a machine that has never had it.	Unverifiable	No changes to probing functions are present in this chunk. The files shown are generated `.codex/hooks/` artifacts only; target probing code changes (e.g., under `packages/domain/src/compile/`) are outside this chunk.
4. The generated hook paths in `hooks.json` are repo-relative or resolved at run time, not the absolute `/Users/edobry/...` paths the current hand-made file hardcodes.	Unverifiable	Per the task split, the manifest generation (`.codex/hooks.json`) moved to mt#4431. This chunk includes no `hooks.json` content or generator changes to inspect.
5. `.codex/` is either tracked as generated output (outcome (a)) or gitignored (outcomes (b)/(c)) — it does not stay in the current untracked-and-unignored state.	Met	This chunk adds multiple `.codex/hooks/*.ts` files to version control (new files shown in the diff), indicating `.codex/` is being tracked as generated output per the chosen outcome (a).
6. The generated-file-edit guard treats `.codex/` the same as `.claude/`, so a hand-edit is denied.	Not Met	The generated `.codex/hooks/*.ts` files in this chunk are empty and lack the `COMPILE_GENERATED_BANNER` (no header lines at all). Without the recognizable banner content, content-based generated-file guards cannot detect edits. Evidence: e.g., `.codex/hooks/interceptor-descriptions.ts` shows +0 -0 (empty).
7. The pre-commit hooks-regen step (mt#2977) regenerates `.codex/` alongside `.claude/` when `.minsky/hooks/**` is staged.	Unverifiable	No changes to `src/hooks/claude-hooks-compile-regen.ts` or related hook code are present in this chunk; only generated artifacts are shown. This criterion cannot be assessed from the provided files.
SC1: `minsky compile --target codex-hooks` regenerates `.codex/hooks/` plus `.codex/hooks.json` from `.minsky/hooks/` (manifest generation moved to mt#4431; source-copy half must match).	Not Met	Multiple `.codex/hooks/*.ts` files in this chunk are added completely empty (+0 −0), lacking the standard shebang/banner/provenance and any logic. Example: `.codex/hooks/judged-input-capture.ts` is empty, while its source `.minsky/hooks/judged-input-capture.ts` contains 400+ lines of code and exports used by other hooks. This indicates the codex-hooks source-copy did not reproduce file contents. Files observed empty: interceptor-provenance-paths.ts; judged-input-capture.ts; knowledge-acquisition-detector.ts; known-guard-names.ts; known-override-env-vars.ts; linkify-liveness.ts; linkify-message-display.ts; loop-preflight-pr-merge-check.ts; markdown-sections.ts; mcp-daemon-staleness-detector.ts; memory-search.ts; merge-gate-fire-log.ts; merge-gate-task-resolution.ts; merge-grant-store.ts.
SC2: `minsky compile --target codex-agents` regenerates `.codex/agents/.toml` from `.minsky/agents/*`.	Unverifiable	This chunk contains only `.codex/hooks/*` files; no `.codex/agents/.toml` are present to inspect. Cannot verify from the provided diff segment.
SC3: `probeDefaultTargets()` includes codex targets so a bare `minsky compile` refreshes them when `.codex/` is present — but does not create `.codex/` on a machine that never had it.	Unverifiable	Target probing code (`packages/domain/src/compile/**`) is outside this chunk. No evidence in the shown files to confirm default-target behavior.
SC4: Generated hook paths in `hooks.json` are repo-relative or resolved at run time, not absolute.	Unverifiable	Manifest generation was moved to mt#4431 per the task description; no `hooks.json` appears in this chunk to validate path handling.
SC5: `.codex/` is tracked as generated output (or gitignored under alternate outcomes).	Unverifiable	Repository tracking state and gitignore changes are not visible within this chunk; cannot confirm from the provided files.
SC6: The generated-file-edit guard treats `.codex/` the same as `.claude/`.	Unverifiable	Guard configuration and matching logic are not part of the changed files in this chunk. Also, multiple empty files lack the generated banner, which would undermine this criterion even if the guard is configured; needs whole-repo check.
SC7: The pre-commit hooks-regen step regenerates `.codex/` alongside `.claude/` when `.minsky/hooks/**` is staged.	Unverifiable	Pre-commit hook code (`src/hooks/claude-hooks-compile-regen.ts`) is not shown in this chunk; unable to verify wiring from the provided changes.
1. `minsky compile --target codex-hooks` regenerates `.codex/hooks/` plus `.codex/hooks.json` from `.minsky/hooks/`. The generated manifest distinguishes the guards Codex can actually fire from the ones it cannot — see `## Codex hook contract` below. Producing the `claude-hooks` guard set verbatim and calling it parity is the specific failure this criterion now forbids: it would yield a manifest that reads as current and enforces less than the fossil appears to.	Unverifiable	This chunk includes generated `.codex/hooks/*.ts` guard modules but not the compile target implementation or the `.codex/hooks.json` manifest generator (moved to mt#4431). From these files alone we cannot verify target behavior or manifest reachability annotations.
2. `minsky compile --target codex-agents` regenerates `.codex/agents/.toml` from `.minsky/agents/*`.	Unverifiable	No `.codex/agents/*.toml` files or the codex-agents target code are present in this chunk. Cannot verify from these additions.
3. `probeDefaultTargets()` includes both codex targets, so a bare `minsky compile` refreshes them when `.codex/` is present — and does NOT create `.codex/` on a machine that has never had it.	Unverifiable	This chunk contains only `.codex/hooks/**` generated scripts and registries. It does not include `packages/domain/src/compile/compile.ts` or probe code. Not verifiable here.
4. The generated hook paths in `hooks.json` are repo-relative or resolved at run time, not the absolute `/Users/edobry/...` paths the current hand-made file hardcodes.	Unverifiable	The `.codex/hooks.json` manifest is not in this chunk (moved to mt#4431). These files reference relative imports, but manifest path policy cannot be confirmed here.
5. `.codex/` is either tracked as generated output (outcome (a)) or gitignored (outcomes (b)/(c)) — it does not stay in the current untracked-and-unignored state.	Unverifiable	Chunk shows added `.codex/hooks/**` files which suggests tracking, but repository-level git status changes and .gitignore updates are not shown in this chunk; cannot conclusively verify.
6. The generated-file-edit guard treats `.codex/` the same as `.claude/`, so a hand-edit is denied.	Unverifiable	Policy for generated-file-edit guard is outside this chunk; only hook modules are present. Cannot verify guard configuration from these files.
7. The pre-commit hooks-regen step (mt#2977) regenerates `.codex/` alongside `.claude/` when `.minsky/hooks/**` is staged.	Unverifiable	Pre-commit hook code (`src/hooks/claude-hooks-compile-regen.ts` / `src/hooks/pre-commit.ts`) is not in this chunk. Cannot verify behavior here.
1. minsky compile --target codex-hooks regenerates .codex/hooks/ plus .codex/hooks.json from .minsky/hooks/. The generated manifest distinguishes the guards Codex can actually fire from the ones it cannot.	Not Met	This chunk adds .codex/hooks/*.ts source-copy files with the generated banner, but no .codex/hooks.json manifest is present here. The PR description explicitly defers the manifest work to mt#4431. Evidence: added files such as .codex/hooks/registry.ts (lines 1-5 show the compile banner) with no hooks.json in this chunk; PR body notes: “The hooks manifest moves to mt#4431.”
2. minsky compile --target codex-agents regenerates .codex/agents/.toml from .minsky/agents/*.	Unverifiable	This chunk contains only .codex/hooks/** files; no .codex/agents/*.toml files are included in this review scope, so their presence/shape cannot be verified from this diff chunk.
3. probeDefaultTargets() includes both codex targets, so a bare minsky compile refreshes them when .codex/ is present — and does NOT create .codex/ on a machine that has never had it.	Unverifiable	Target probing changes live under packages/domain/src/compile/ and src/hooks/; they are not in this chunk. This chunk only contains generated .codex/hooks files.
4. The generated hook paths in hooks.json are repo-relative or resolved at run time, not absolute /Users/... paths.	Not Met	The manifest generation (which would carry these paths) is deferred to mt#4431 per the PR description. No hooks.json is present in this chunk to verify path handling.
5. .codex/ is either tracked as generated output or gitignored — it does not stay untracked-and-unignored.	Met	This chunk adds numerous files under .codex/hooks/** with the generated-file banner, indicating .codex/ is now tracked as generated output. Example: .codex/hooks/registry.ts:1-5 includes “Generated by minsky compile. Do not edit directly.”
6. The generated-file-edit guard treats .codex/ the same as .claude/, so a hand-edit is denied.	Met	Generated outputs in this chunk carry the shared compile banner string the guard matches. Example: .codex/hooks/render-path-evidence.ts:1-3 and other files include the standard generated banner and source provenance, satisfying the guard’s content-based detection.
7. The pre-commit hooks-regen step (mt#2977) regenerates .codex/ alongside .claude/ when .minsky/hooks/** is staged.	Unverifiable	Pre-commit wiring changes reside in src/hooks/claude-hooks-compile-regen.ts and related hook code, not present in this chunk. This chunk only contains the generated .codex/hooks files, so we cannot confirm the pre-commit integration here.
1) minsky compile --target codex-hooks regenerates .codex/hooks/ plus .codex/hooks.json from .minsky/hooks/, and the generated manifest distinguishes guards Codex can fire from those it cannot.	Not Met	This PR intentionally defers the manifest half to mt#4431 per the PR description. The source-copy half is present (e.g., .codex/hooks/turn-end-retro-scan.ts and peers) but there is no .codex/hooks.json generation nor any reachability classification. Follow-up task mt#4431 owns this; the spec or PR should record this deferral explicitly (it is already stated in the PR text).
2) minsky compile --target codex-agents regenerates .codex/agents/.toml from .minsky/agents/*.	Unverifiable	This review chunk contains only .codex/hooks/.ts files. No .codex/agents/.toml are in-scope for this chunk; cannot verify from the provided files.
3) probeDefaultTargets() includes both codex targets so a bare compile refreshes them when .codex/ is present — and does NOT create .codex/ on a machine that has never had it.	Unverifiable	Target probing lives under packages/domain/src/compile/**; not present in this chunk. Cannot confirm behavior from the .codex/hooks files alone.
4) The generated hook paths in hooks.json are repo-relative or resolved at run time, not absolute.	Not Met	hooks.json generation is deliberately moved to mt#4431 per the PR description; no manifest is generated in this PR, so no path handling to verify. This is part of the same deferral as SC1 and should be tracked in mt#4431.
5) .codex/ is either tracked as generated output or gitignored — not left untracked-and-unignored.	Unverifiable	Repository tracking state is not inferable from the content of these files alone. This chunk shows generated .codex/hooks files with banners, but verifying git tracking/ignore status requires repo metadata outside this diff chunk.
6) The generated-file-edit guard treats .codex/ the same as .claude/, so a hand-edit is denied.	Met	Each generated file in this chunk carries the standard banner and provenance lines recognized by the guard (e.g., .codex/hooks/turn-end-retro-scan.ts:1-4 show the shebang, 'Generated by minsky compile. Do not edit directly.' and 'Source: .minsky/...'). This satisfies the banner content requirement used by the guard.
7) The pre-commit hooks-regen step regenerates .codex/ alongside .claude/ when .minsky/hooks/** is staged.	Unverifiable	Pre-commit wiring resides under src/hooks/** outside this chunk. No verification possible from the .codex/hooks scripts alone.

Documentation impact

blocking-needs-update — This PR adds two new compile targets (codex-hooks and codex-agents), changes target probing semantics (Codex targets included only when .codex/ exists), and extends the generated-file guard applicability. These are user-facing behaviors of the compile pipeline and pre-commit hooks. I did not read specific docs in docs/ in this call, but the task spec explicitly requires documenting default target behavior and CI/pre-commit interactions (mt#3134, mt#2977), and the PR description says hooks of what a bare compile regenerates should be added to hook-files.mdc. Given these behavior changes, docs must be updated to describe: how to adopt Codex (presence flag via .codex/), the new targets and their outputs, and that .codex/hooks are treated as generated with banner enforcement. Without such updates, existing compile/probe docs are likely inaccurate or incomplete. Affected docs likely include CLI/compile workflow docs and any hook-files documentation.
Affected: docs/development-workflow.md

minsky-reviewer

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

Overall the Codex targets and shared agent/hook extractions look coherent and are backed by targeted tests. However, I found a critical issue in the TOML emitter: escapeTomlMultilineString only escapes the first quote of any run, so 4+ consecutive quotes still leave an unescaped """ subsequence that can prematurely terminate the TOML multi-line string, silently corrupting prompts. This must be fixed and tests added for 4- and 5-quote runs. Separately, this PR introduces user-visible behavior (new Codex compile targets, opt-in gating by .codex presence, generated-file banner semantics, and pre-commit regeneration/orphan checks) without accompanying documentation updates — docs need to be updated before merge. I also noted a non-blocking observability/testability seam: direct process.stderr writes in hook-copy-target; consider an injected reporter or structured errors. Address the TOML escaping bug and add the documentation updates; with those resolved, the PR should be ready to merge.

Findings

[BLOCKING] .codex/hooks/block-git-gh-cli.ts:1006 — git add conflict-resolution carve‑out runs git diff in the hook’s own cwd — wrong repo for session_exec invocations
The carve‑out in checkDenial() for git add calls isConflictResolutionAdd(parsed.args, runGit), which in turn shells git diff --name-only --diff-filter=U via defaultRunGit with no cwd override. In a session_exec PreToolUse, the guarded git runs inside the session workspace, but this hook process executes in the host’s cwd (typically the project checkout). That means the carve‑out inspects the wrong repository and can (a) allow a git add <paths> that is not resolving conflicts in the session, or (b) deny a legitimate conflict‑resolution add in the session because the project repo has no unmerged paths. Evidence:
checkDenial at .codex/hooks/block-git-gh-cli.ts:1006–1040 — calls isConflictResolutionAdd unconditionally for git add.
isConflictResolutionAdd and defaultRunGit at .codex/hooks/block-git-gh-cli.ts:736–768 — execSync(cmd) uses the hook’s process cwd; no session cwd is applied.
Context routing is known above: the scope computation distinguishes bash vs session_exec (see toolContextFromName and the scope logic), but that context is not threaded into cwd for the carve‑out.
Fix options (pick one):
Easiest/safest: disable the carve‑out for session_exec (treat as deny unless covered by a purpose‑built tool), mirroring the other allowedInSessionExec rules.
Or: plumb the intended working directory into the carve‑out and run git diff there (e.g., add a cwd param to checkDenial → isConflictResolutionAdd, and use execSync(cmd, {cwd})). For session_exec, resolve the session workspace path from input.session_id (if that mapping exists reliably) before applying the carve‑out.
Until cwd is correct, applying the carve‑out in session_exec produces incorrect authorization decisions.
[BLOCKING] .codex/hooks/block-git-gh-cli.ts:702 — git add carve‑out ignores a leading cd and still shells in the hook process cwd, so it can evaluate the wrong repo even on Bash invocations
When a command starts with cd <path> && git add <files>, the guard’s scope logic can resolve the cd target (see resolveLeadingCdTarget() at .codex/hooks/block-git-gh-cli.ts:323–372) and uses it for scope classification. However, the conflict‑resolution carve‑out in checkDenial() still calls isConflictResolutionAdd() → defaultRunGit() with no cwd, which executes git diff --name-only --diff-filter=U in the HOOK PROCESS cwd (typically the project root), not the cd target where the git add will run. This can:
Allow a non‑conflict git add in the cd’d repo because the project repo happens to show unmerged paths; or
Deny a legitimate conflict‑resolution git add in the cd’d repo because the project repo has no unmerged paths.
Evidence:
resolveLeadingCdTarget exists and is used only for scope (not for the carve‑out).
checkDenial at .codex/hooks/block-git-gh-cli.ts:1006–1040 unconditionally calls isConflictResolutionAdd(parsed.args, runGit) for git add.
defaultRunGit at .codex/hooks/block-git-gh-cli.ts:760–768 shells without { cwd }.
Fix: Thread the effective working directory into the carve‑out and run git diff there (e.g., plumb a cwd param from the hook entrypoint → checkDenial → isConflictResolutionAdd → execSync(cmd, { cwd })), or disable the carve‑out unless the cwd is known to match the invocation’s target directory.
[BLOCKING] .codex/hooks/canary-runner.ts:188 — isCanaryRecord checks the wrong field name for fire-log records (uses session_id but fire-log records write sessionId)
isCanaryRecord(record: { session_id?: unknown }): boolean (around .codex/hooks/canary-runner.ts:188) returns true only when record.session_id === CANARY_SESSION_ID. However, fire-log records constructed by this PR use the camelCase field sessionId (see .codex/hooks/merge-gate-fire-log.ts:165-171 where record: { … sessionId: input.session_id, … } is written). As a result, canary rows in the fire log will not be recognized by isCanaryRecord, defeating the stated purpose (“filter canary rows out of guard-health denominators”). Fix: either (a) check both session_id and sessionId, or (b) align the helper to the actual fire-log schema and read sessionId. Add a unit test exercising this against a real recordFireLogEntry-shaped object.
[BLOCKING] .codex/hooks/block-secret-file-read.ts:365 — filePathCandidates misses attached grep pattern/file flags (-ePATTERN / -fFILE), causing false positives
In filePathCandidates(program, tokens) (around .codex/hooks/block-secret-file-read.ts:365+), the logic skips separate-value pattern flags -e/--regexp and -f/--file, and handles long attached forms --regexp= / --file=, but it does NOT handle the common short attached forms -ePATTERN and -fFILE. For grep-family readers, those attached forms place the PATTERN (or a file OF patterns) directly in the same token; your loop currently treats such a token as an ordinary argument, leaving it in out and making it eligible for isSecretPath scanning. This recreates the exact false-positive class called out in the comment (mt#3703): a pattern token resembling a credential filename (e.g., "CredentialRead" or "credentials.json") can be misclassified as a path and cause an unjustified deny. Fix: extend the skip logic to detect and skip ^-e.+ and ^-f.+ tokens (short-form with attached value) alongside the existing --regexp=/--file= handling. Consider adding tests for grep -ePATTERN … and grep -fFILE … to cover these forms.
[BLOCKING] .codex/hooks/chained-verification-commands-detector.ts:60 — Override env var handling is inconsistent with other guards (accepts only "1", not "true"/"yes")
isOverridden() (around .codex/hooks/chained-verification-commands-detector.ts:60+) returns true only when process.env[MINSKY_SKIP_CHAINED_VERIFICATION_SCAN] === "1". Across this diff, sibling guards accept a broader set for boolean envs ("1", "true", "yes") — e.g., block-subagent-merge-without-grant.ts:isOverrideActive, block-subagent-bypass-merge.ts:isOverrideActive, block-out-of-band-merge.ts:isOverrideSet, and block-secret-file-read.ts:isOverrideSet. Restricting to exactly "1" makes operator behavior inconsistent and can lead to surprises (an override that works for other guards silently fails here). Fix: adopt the common helper pattern and accept case-insensitive "1"/"true"/"yes"; add a test for all three forms.
[BLOCKING] .codex/hooks/block-secret-file-read.ts:430 — Missed input-redirect form <file (no space) leads to undetected secret read
In findSecretReads (around .codex/hooks/block-secret-file-read.ts:430+), detection of stdin redirection checks only for a < that appears BEFORE the secretToken (hasInputRedirect = /<\s*[^\s<>|]*$/.test(segment.slice(0, segment.indexOf(secretToken)))). This misses the common shell form with no space where the redirect operator is attached to the filename token itself (e.g., cmd <~/.env or cmd <.env), in which case secretToken will be the literal "<~/.env" and the < is not present in the preceding substring. Two consequences: (a) isSecretPath(secretToken) will likely return false because the token starts with <, and (b) hasInputRedirect will be false, so the segment is considered safe and the leak is not denied. This is a silent false negative on exactly the secret-read shape this guard exists to catch. Fix: normalize potential redirect-bearing tokens by stripping a leading < before path classification, or augment detection to treat any token matching ^<\S+ as an input-redirect path; also expand hasInputRedirect to detect ^<\s* patterns at the token itself. Add tests for cmd <~/.env and cmd <.env to cover both no-space variants.
[BLOCKING] .codex/hooks/check-branch-fresh.ts:154 — Import path escapes .codex/ tree into packages/domain/ — brittle in generated Codex hooks
The generated hook imports writeFreshnessMarker from ../../packages/domain/src/session/freshness-marker. In the Codex target this path crosses out of .codex/ into the source tree. Two risks:

Distribution/runtime: Codex hooks are expected to be self-contained outputs. Reaching back into packages/domain at runtime couples execution to repo layout and TypeScript sources, which may not be present/compiled in a Codex-only consumer.
Divergence between .claude/ and .codex/: The Claude variant of this hook likely imports the same helper using a relative path tailored to that tree. If the factory generating Codex hooks didn’t rewrite or parameterize this import, Codex hooks will try to resolve a path that may not exist in environments where only .codex/ artifacts are used.

If the design intentionally allows generated hooks to import source modules, this needs an explicit guarantee that the path is valid in Codex runs (e.g., compile bundles and copies this helper alongside; or the path is resolved via bun with TS transpilation available). Otherwise, mirror the approach used elsewhere in generated hooks: inline minimal helper or re-export from a stable hooks-local module to avoid cross-tree dependencies.

[BLOCKING] .codex/hooks/detect-cli-mcp-substitution.ts:150 — Env-var prefixes break minskyArgvOf parsing for both minsky … and bun … spellings
minskyArgvOf() builds tokens = segment.trim().split(/\s+/) and then decides based on leadingTokenOf(segment) whether the command is minsky/bun/bunx. However, it slices the argv tail using fixed offsets against the raw tokens array, which still includes any leading environment-variable assignments. Example: FOO=1 BAR=2 minsky tasks get yields first === "minsky" (via leadingTokenOf), but tokens.slice(1) returns ["minsky","tasks","get"], leaving minsky in place as argv[0]. That causes resolveCommandId to fail to descend into subcommands and silently miss a substitution. The same flaw affects the bun/bunx path: FOO=1 bun run src/cli.ts … sets first === "bun" but tokens[1] is still an env var or run at a different index, so the parser advances incorrectly.

This creates false negatives for CLI–MCP equivalence detection whenever env-var prefixes are present — a common shape in our command corpus and expressly supported by leadingCommandOf.

Suggested fix: explicitly strip all leading NAME=VALUE tokens before computing the program token and argv tail. Compute programIndex as the index of the first non-env token equal to minsky (or bun/bunx), and return tokens.slice(programIndex + 1). For the bun branch, similarly locate run (optional) and the script/binary token relative to the env-stripped array rather than assuming fixed positions.

[BLOCKING] .codex/hooks/dispatch-intent-store.ts:119 — Closed-world duplication of allowed intents (VALID_INTENTS) risks drift from domain source of truth
VALID_INTENTS is hardcoded to new Set(["read-only", "implementation"]) in this module (see .codex/hooks/dispatch-intent-store.ts:119-121). The record and predicates are explicitly sourced from @minsky/domain/detectors/dispatch-intent-gate, but this runtime validation set is a parallel source of truth: if the domain layer adds a new valid intent value, this validator will silently reject it at parse time, causing declarations to be dropped. This is the exact SoT-duplication pattern ADRs warn against (and the header claims to avoid by re-exporting from domain). Fix: remove the hand-maintained VALID_INTENTS and import/derive the allowed-intent guard from the domain module (e.g., export a type-guard/enum/readonly set there and consume it here), or validate by delegating to the domain predicate instead of redeclaring the value set.
[BLOCKING] .codex/hooks/operator-deferral-detector.ts:1536 — Standalone entrypoint omits the Act-Path Workaround surface present in the dispatcher path (silent behavior divergence)
In main()'s non-AskUserQuestion branch, the matches list includes only detectCapabilityDeferral, detectPermissionDeferral, detectDenialAnchoredDeferral, and detectAskJustificationAbsence (see .codex/hooks/operator-deferral-detector.ts:1536-1546). The run() dispatcher path above includes a fifth surface: detectActPathWorkaround(turnLines) (see .codex/hooks/operator-deferral-detector.ts:1322-1331). The comment in main() even claims "Same surfaces as run()", but the act-path surface is missing, creating a silent behavior difference between the two entrypoints. This will cause missed fires in standalone invocations compared to dispatcher-invoked runs. Please add ...detectActPathWorkaround(turnLines) to the main() path to keep parity (and update any tests asserting parity).
[BLOCKING] .codex/hooks/interceptor-provenance-paths.ts:1 — Generated hook file is empty — missing shebang, generation banner, and implementation (contradicts PR’s "same shebang/banner" claim)
This added file is completely empty (+0 −0 in the diff), lacking the expected #!/usr/bin/env bun shebang and the "Generated by minsky compile" banner that other generated hooks carry (e.g., .codex/hooks/operator-deferral-detector.ts). Several other hooks in this chunk show the same zero-length addition (.codex/hooks/judged-input-capture.ts, knowledge-acquisition-detector.ts, known-guard-names.ts, known-override-env-vars.ts, linkify-*, loop-preflight-pr-merge-check.ts, markdown-sections.ts, mcp-daemon-staleness-detector.ts, memory-search.ts, merge-* files, etc.). Empty stubs will not execute as standalone scripts and break the stated guarantee that Codex hooks have the same shebang/banner/provenance. This looks like an incomplete compile output or packaging error. Please regenerate to ensure these files contain the compiled content (with banner and 0o755) or remove unused outputs and their registrations.
[BLOCKING] .codex/hooks/record-subagent-invocation.ts:840 — Synchronous git call (Bun.spawnSync) in resolveTaskId can hang past the 8s deadline, violating the fail‑open contract
In resolveTaskId's Strategy 2, the code invokes Bun.spawnSync(["git", "rev-parse", "--abbrev-ref", "HEAD"], { cwd, stdout: "pipe", stderr: "pipe" }); to derive a task id from the branch name. This is a synchronous spawn with no timeout. If git blocks (e.g., waiting on credentials, repository lock contention, or a hung child), the process thread is blocked and the dispatcher’s 8s deadline (RECORD_INVOCATION_TIMEOUT_MS) and the Promise.race guard in the entrypoint cannot preempt it — timers won’t fire while the event loop is blocked by a sync call. That directly contradicts the module’s fail‑safe contract (“must NEVER block a subagent stop”) and reintroduces the very hang risk mt#3019 sought to remove from this path.

Evidence (file excerpt around the call):

Strategy 2 block under “Task ID resolution”: const result = Bun.spawnSync(["git", "rev-parse", "--abbrev-ref", "HEAD"], { cwd, stdout: "pipe", stderr: "pipe" }); followed by parsing result.

Requested fix:

Replace the sync spawn with an async, bounded call (e.g., Bun.spawn with a timeout and kill on expiry), or gate this strategy behind the same pre‑deadline checkpoints you added elsewhere, or skip Strategy 2 entirely under deadline/when a dispatch stamp exists (mirroring the earlier demotion rationale). The key is to ensure no synchronous, potentially unbounded operation runs on the path before the deadline can fire.
[BLOCKING] .codex/hooks/record-subagent-invocation.ts:129 — Synchronous transcript read in recoverDispatchStamp (readFileSync) can block the process and defeat the deadline
recoverDispatchStamp defaults its readFile parameter to readFileSync(p, "utf8") and is called on the hot path before the DB write. A large or slow transcript read (e.g., large JSONL on a slow/contended disk, NFS hiccup) will block the event loop. Since the entrypoint’s timeout/Promise.race depends on timers, a synchronous filesystem read can prevent the 8s RECORD_INVOCATION_TIMEOUT_MS guard from firing, violating the fail‑open contract (“must NEVER block a subagent stop”). This reintroduces a whole‑process hang risk outside Postgres.

Evidence:

Function signature: readFile: (path: string) => string = (p) => readFileSync(p, "utf8") and immediate findStampInTranscriptLines(readFile(agentTranscriptPath).split("\n")).

Requested fix:

Make the transcript read asynchronous and bounded (e.g., stream a capped prefix with an abortable Bun.file(...).stream() or fs.promises.readFile with an explicit timer/abort), or short‑circuit this path under deadline like you did for DB operations. Alternatively, only read the minimal prefix/suffix necessary to locate the stamp (since it’s written near the prompt head), not the whole file, and ensure it’s non‑blocking.
[BLOCKING] .codex/hooks/require-deploy-verification-before-merge.ts:76 — Hook imports a non-hook module outside the hooks tree — breaks the “dependency-free hooks” contract and risks runtime failure under Codex
This file imports classifyAndRecordMergeDeploySurface from ../../packages/domain/src/deployment/merge-deploy-surface-record (see .codex/hooks/require-deploy-verification-before-merge.ts:76). The header comment explicitly states “Dependency-free per .minsky/hooks/SPEC.md beyond its sibling hook modules.” Codex/Claude hook processes execute the compiled hook files in the hooks directory; resolving a relative import that climbs out of .codex/hooks into packages/domain is not guaranteed to work in the Codex harness and violates the stated isolation. This can cause the PreToolUse process to error at module-load time, silently skipping or fail-opening the gate. Fix by removing the cross-tree import: either (a) move the classification+record write behind an injected callback passed from a sibling hook module, (b) duplicate the minimal write logic in a local, dependency-free helper, or (c) refactor the recording function into a sibling .codex/hooks/* module that both producers can import without leaving the hooks tree.
[BLOCKING] .codex/hooks/require-duplicate-check-record.ts:117 — Override audit line echoes the raw env var value — leaks potentially sensitive configuration; siblings intentionally avoid echoing
At .codex/hooks/require-duplicate-check-record.ts:117-124, the override path builds auditLines with ack=${overrideVal} and writes it to STDERR. Other merge gates in this PR and recent rounds explicitly stopped echoing override values (e.g., require-deploy-verification-before-merge.ts and require-checks-on-bypass-merge.ts) to avoid leaking secrets or operator configuration into transcripts/artifacts. This file diverges and prints the raw value. Fix by removing the value echo (presence/name/timestamp only), aligning with the established “value not echoed” posture (see the comments in those sibling hooks), e.g., [require-duplicate-check-record] OVERRIDE active: MINSKY_SKIP_DUPLICATE_RECORD set (value not echoed) ….
[BLOCKING] .codex/hooks/require-growth-justification-before-merge.ts:257 — hasSizeBudgetJustification counts markers inside fenced code blocks — fence elision computed but not applied at marker line
In .codex/hooks/require-growth-justification-before-merge.ts, the function hasSizeBudgetJustification builds const fenceInternal = computeFenceInternalLines(lines); but never skips lines that are inside a fenced block when detecting the marker. The loop at lines ~257-268 only checks if (line === undefined) continue; and then immediately const match = line.match(SIZE_BUDGET_JUSTIFICATION_MARKER). This means a Size-budget justification: string inside a fenced example will satisfy the blocking gate, contradicting the comment (“mirrors hasExecutionEvidence discipline”) and the fence-aware conventions used elsewhere (e.g., hasDeployVerification, hasExecutionEvidence). Fix by adding if (fenceInternal[i]) continue; before testing the marker, so quoted examples don’t satisfy the gate; keep the existing fence-aware boundary check for ending the section.
[BLOCKING] .codex/hooks/require-review-before-merge.ts:1365 — Misleading denial reason when reviews fetch/parse fails — user-facing message says “No review on PR …” even when the probe crashed
In the block that handles reviews.length === 0, the hook writes a PreToolUse denial with permissionDecisionReason: "No review on PR #${pr}. Ensure the reviewer bot posts a review before merging.", but the code above explicitly tracks reviewsFetchFailed to distinguish a forge transport/parse failure from a genuinely empty review list. When reviewsFetchFailed is true, the outward-facing reason remains “No review on PR …”, which sends operators to chase a missing review when the real issue is a broken gh api probe. This is a diagnostic inversion and makes incidents harder to triage.

Evidence: .codex/hooks/require-review-before-merge.ts — the reviewsJson result is inspected (non-zero exitCode and JSON-parse error set reviewsFetchFailed = true), but the denial reason emitted in the if (reviews.length === 0) branch does not vary with reviewsFetchFailed. Immediately after, recordAndExit("deny", undefined, reviewsFetchFailed ? "crashed" : "decided") correctly records the crashed probe, proving the hook knows the distinction but does not reflect it in the human-facing message.

Request: vary the denial text based on reviewsFetchFailed. For example, when reviewsFetchFailed is true, emit an actionable transport/parse-failure diagnosis (e.g., “Unable to read PR reviews (gh api transport/parse failure) — investigate the forge/API before retrying”), consistent with how the other CI-related denials are phrased in this hook. Keep the genuine-empty-review message only for the decided path.

[BLOCKING] packages/domain/src/compile/targets/codex-agents.ts:1 — escapeTomlMultilineString fails for runs of 4+ quotes — leaves an unescaped """ subsequence
In escapeTomlMultilineString (codex-agents.ts), the replacement value.replace(/"{3,}/g, (run) => \"${run.slice(1)}) escapes only the first character of any run of quotes. For runs of length ≥4, this transforms e.g. """" into \""", which still contains the unescaped delimiter """ and will prematurely close the TOML multi-line basic string. This is a silent corruption risk on real prompts that may contain four (or more) consecutive quotes.

Evidence: packages/domain/src/compile/targets/codex-agents.ts — function escapeTomlMultilineString.

Suggested fix: ensure the transformation leaves no """ subsequence anywhere in the result. One approach is to insert a backslash before every third quote within each run, e.g. generate groups of ""\" repeatedly (or iterate the run and emit \" for indices where (i % 3) === 0). Add tests covering 4- and 5-quote runs to codex-targets.test.ts (e.g., 'before """" after' and 'before """"" after') to prevent regressions.

[BLOCKING] packages/domain/src/compile/targets/codex-agents.ts:1 — Documentation needs update for new Codex compile targets and pre-commit behavior
User-facing behavior changed: .codex/ is now a generated compile output with two new targets (codex-hooks, codex-agents), gated by .codex/ presence, and pre-commit regenerates/re-stages these outputs. The repository docs should be updated to cover: how to adopt Codex (create .codex/), what gets generated and where, the .toml agent format surface (name/description/developer_instructions and banner), and how --check/orphan detection applies to .codex/ directories. No documentation changes are present in this PR, so merging would ship undocumented behavior.
[NON-BLOCKING] .codex/hooks/check-generated-file-edit.ts:269 — Override audit log hardcodes =1, ignoring actual override value (true/yes)
emitOverrideAuditLog() prints OVERRIDE active (MINSKY_FORCE_EDIT_GENERATED=1) regardless of what value actually triggered the override. However, isOverrideSet() accepts "1" | "true" | "yes". This makes the stdout audit line inconsistent with the real env value used, which can hinder incident forensics. Consider including the acknowledged value (e.g., ack=<value>) as is done in other hooks (e.g., check-task-spec-read.ts), or at least not hardcoding =1.
[NON-BLOCKING] .codex/hooks/flakiness-control-detector.ts:142 — Standalone CLI path does not emit a JSON payload on success, which diverges from ADR-028’s “single JSON object on stdout” contract
In the import.meta.main block, the code reads input and computes outcome = run(...), but only writes auditLines to STDERR and then exits 0 without emitting any JSON on stdout (see lines 142-173). By contrast, sibling hooks like .codex/hooks/inject-current-time.ts construct a HookOutput and call writeOutput(...) for standalone invocation. If any tests or direct invocations rely on the stdout contract for this guard (ADR-028 §Output aggregation: one JSON object on stdout), this hook would appear silent and its additionalContext advisory would be lost. If this entrypoint is intentionally dispatcher-only, consider either (a) explicitly documenting that the script’s standalone entry is audit-only (no stdout JSON), or (b) emitting a minimal HookOutput JSON on stdout to align with the convention used by other hooks.
[NON-BLOCKING] .codex/hooks/negative-existence-claim-detector.ts:86 — Drizzle SQL IN-clause construction is likely incorrect; use inArray/parameterization instead of interpolating an array
resolveDoneTaskIds builds the SQL as sqlselect id from tasks where id in ${taskIds} and status = 'DONE' (.codex/hooks/negative-existence-claim-detector.ts:86-94). In Drizzle ORM, directly interpolating a JavaScript array into a tagged `sql template does not expand to a proper (..., ...) list; you typically need inArray(tasks.id, taskIds) or sqlid in (${sql.join(taskIds, sql,)})`` with parameters. As written, this is likely to error or produce a malformed query, causing the lookup to fall back to null and degrade the detector’s precision. Suggest: switch to the query builder with `inArray` (preferred) or use `sql.join` to build a parameterized IN list.
[NON-BLOCKING] .codex/hooks/post-merge-pull.ts:82 — git pull failure handling does not detect the common untracked-file overwrite case
In the pull-failure branch, the code distinguishes stale lock and a dirty tracked-tree via DIRTY_TREE_STDERR_MARKER ("Your local changes… would be overwritten"). However, another frequent failure mode is untracked files that would be overwritten by the pull (stderr typically contains: "The following untracked working tree files would be overwritten by merge"). That path will fall through to the generic stderr dump and exit(1), providing no targeted guidance. Consider adding an explicit check for the untracked-file marker and surfacing a clearer remediation (e.g., move/remove the untracked files or git clean with care). This improves diagnosability without changing behavior.
[NON-BLOCKING] .codex/hooks/parallel-work-guard-standalone.ts:31 — Doc comment references the wrong import source for TERMINAL_TASK_STATUSES
The comment above the TERMINAL_TASK_STATUSES usage states it is imported from ./types, but the code actually imports it from ./task-statuses (import { TERMINAL_TASK_STATUSES } from "./task-statuses";). This can mislead future readers during maintenance. Update the doc comment (or the import) to keep the source-of-truth consistent.
[NON-BLOCKING] .codex/hooks/truncated-outcome-read-detector.ts:1 — NEEDS VERIFICATION: Codex hook runner compatibility with TypeScript + bun shebang
These .codex/hooks/*.ts files are TypeScript with a #!/usr/bin/env bun shebang and import sibling .ts modules (e.g., ./registry, ./command-shape). This assumes the Codex hooks runtime will execute the files directly via the shebang and that bun is available on the host/container where Codex runs. If Codex instead loads hooks as CommonJS/ESM JavaScript (or invokes Node without honoring shebangs), these TS sources may not execute (or resolution without .ts extensions may fail). Please confirm the Codex hook runner honors the shebang and has bun present, or consider emitting JS for .codex/hooks/** if the runner executes code without bun. Anchored example: .codex/hooks/truncated-outcome-read-detector.ts:1.
[NON-BLOCKING] .codex/hooks/types.ts:650 — Codex host-cap lookup still points at .claude/settings.json — likely falls back to default under Codex
readHostCap()/findHostCapInSettings() resolve the budget cap from ${projectDir}/.claude/settings.json (see .codex/hooks/types.ts around the readHostCap and findHostCapInSettings implementations). Under Codex, the settings/manifest lives in a different file/format, and this PR explicitly defers the Codex manifest. That means Codex-invoked hooks will almost always miss a matcher entry and degrade to the default 15s cap with a warning. Please confirm this is intentional for Codex (temporary until mt#4431), or consider guarding the warning message to mention Codex explicitly to avoid confusing operators who don’t have .claude/settings.json in Codex workspaces.
[NON-BLOCKING] packages/domain/src/compile/targets/hook-copy-target.ts:1 — Hard-coded stderr write in library path reduces testability/observability seams
makeHookCopyTarget logs read errors with process.stderr.write(...) (around the hook read catch). While understandable to avoid a logger dependency, this side-effect couples the target to process IO and is hard to assert without patched-collaborator tests (see testing-standards.mdc §Testable Design). Consider injecting an optional reporter/callback into the target config or returning structured errors via the result so callers/tests can observe failures without monkey-patching global IO. Marking as advisory; not blocking.

Inline comments

.codex/hooks/command-shape.ts:132 — Minor: leadingCommandOf() returns the entire first stage minus env-var prefixes (joined back with spaces), while the docstring reads like it should return “the command whose exit status the segment reports,” which could be read as just the program token. Since leadingTokenOf() further reduces it to the first word anyway, this is fine for current consumers — but the mismatch between name/docs and return shape could surprise future callers. Consider either returning only the first token here, or clarifying in the doc that the full command (sans env prefixes and pipeline tail) is intentionally returned.
.codex/hooks/require-session-for-main-workspace-edits.ts:152 — Question: checkFilePathDenial allows when filePath is not absolute (!filePath.startsWith("/")). The comment says Edit/Write enforce absolute paths — can you confirm that holds for all tools this hook guards (including NotebookEdit)? If any caller can pass a relative path, this would be a silent bypass. If enforcement isn’t universal, consider normalizing to an absolute path (e.g., against MAIN_WORKSPACE) or denying non-absolute inputs.
.codex/hooks/retrospective-completeness-detector.ts:101 — Potential false-negative: parseTriageLevel only recognizes “minor correction,” “process failure,” and “repeated failure.” If the skill output uses slight lexical variants (e.g., “Process-level failure,” “Repeated incident”), this will return unknown and force the full section set. Was this vocabulary stabilized in the SKILL.md, or should this parser accept a few common synonyms (e.g., regexes anchored on the key noun)? Otherwise, retros that follow intent but vary phrasing could be over-graded as missing sections.

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
.codex/hooks/dispatch-intent-store.getDispatchIntentStorePath	function	.codex/hooks/dispatch-intent-write-gate.ts — used to locate the store file	Adopted
<many new hooks and exports under .codex/hooks> (cost-bounding rule)	capability	—	Missing consumers	This chunk introduces numerous generated hook modules and exported functions/types under .codex/hooks. These mirror existing .minsky/.claude hooks and are consumed within the hooks tree and harness via settings.json, not via TS imports. No additional intra-repo adopters are expected beyond the generated peers; treat as generated outputs. Recommend a follow-up audit to ensure settings registrations for Codex targets reference these compiled paths where applicable (tracked in mt#4431 per PR body).
new exports across generated Codex hook modules (cost-bounding rule)	capability	—	Missing consumers	This chunk adds numerous exported functions/constants in generated .codex/hooks/* modules (e.g., multiple run/main/evaluate helpers). These are compiled hook entrypoints intended for the Codex harness rather than in-repo imports, so lack of codebase consumers is expected. No spec in this chunk requires wiring beyond generation.
new exports (cost-bounding rule)	capability	—	Missing consumers	This chunk introduces many new exported functions/types across generated .codex hook files (e.g., decideStandaloneDuplicateGuard, runStandaloneDuplicateGuard, extractInScopeFiles, fetchPrContext, fetchCheckRunsRaw, detectPreNarration, etc.). As these live under the generated .codex tree and are primarily entrypoints or test seams, no in-repo consumers are expected beyond the corresponding compiled hooks/tests. Recommend a follow-up sweep only if these are intended to be imported elsewhere; otherwise, treat as internal to generated outputs.
new exports (cost-bounding rule)	capability	—	Missing consumers	This PR adds many new generated hook modules and exported functions under .codex/hooks/*. Per the cost-bounding rule, recording as a single capability entry. These are compile outputs intended to be invoked by the Codex harness rather than imported by in-repo modules, so missing in-repo consumers is expected. No action required beyond ensuring the harness picks them up.
16+ new public exports across compile targets (cost-bounding rule)	capability	packages/domain/src/compile/targets/codex-targets.test.ts — imports makeCodexHooksTarget, makeCodexAgentsTarget, buildAgentToml, escape helpers, packages/domain/src/compile/targets/claude-agents.ts — re-exports DynamicImportFn for compatibility, packages/domain/src/compile/targets/claude-hooks.ts — re-exports hook constants for compatibility	Missing consumers	Multiple new exported helpers/factories/constants are introduced across agent/hook target modules. Primary consumers are tests and sibling modules; broader adoption (e.g., CLI wiring already present via compile registry) appears intact. No spec-mandated additional wiring observed in this chunk.

Recommendation: file a follow-up adoption task to wire 5 missing consumers.

Documentation impact

blocking-needs-update — This PR introduces new compile targets and behavior affecting users: (a) Codex harness outputs are now generated (codex-hooks, codex-agents) and gated by .codex/ presence; (b) .codex/agents/*.toml format and generated-file banner semantics; (c) pre-commit now regenerates and re-stages Codex outputs. No docs were updated in this diff. The compile pipeline and contributor docs should describe the new targets, the opt-in gating rule, and how generated .codex/ files are treated by pre-commit and the generated-file guard. Absent updates, users will be unaware of the new outputs and gating semantics.

minsky-reviewer · 2026-09-04T05:48:00Z

+ * This is the axis {@link isReshapedRetry} compares on, because it is what a
+ * prefix-based permission allow-rule matches on: a rule granting `curl:*`
+ * applies when `curl` is the leading token and does not apply when `curl` is
+ * the third segment of a compound command.


Minor: leadingCommandOf() returns the entire first stage minus env-var prefixes (joined back with spaces), while the docstring reads like it should return “the command whose exit status the segment reports,” which could be read as just the program token. Since leadingTokenOf() further reduces it to the first word anyway, this is fine for current consumers — but the mismatch between name/docs and return shape could surprise future callers. Consider either returning only the first token here, or clarifying in the doc that the full command (sans env prefixes and pipeline tail) is intentionally returned.

minsky-reviewer · 2026-09-04T05:48:00Z

+  toolName: string,
+  filePath: string | undefined,
+  readFile: (path: string) => string = defaultReadFile,
+  mainWorkspace: string = MAIN_WORKSPACE,


Question: checkFilePathDenial allows when filePath is not absolute (!filePath.startsWith("/")). The comment says Edit/Write enforce absolute paths — can you confirm that holds for all tools this hook guards (including NotebookEdit)? If any caller can pass a relative path, this would be a silent bypass. If enforcement isn’t universal, consider normalizing to an absolute path (e.g., against MAIN_WORKSPACE) or denying non-absolute inputs.

minsky-reviewer · 2026-09-04T05:48:00Z

+
+/**
+ * Heading depth accepted for a section, used by BOTH `hasSection` and
+ * `TRIAGE_SECTION` (PR #2564 R1).


Potential false-negative: parseTriageLevel only recognizes “minor correction,” “process failure,” and “repeated failure.” If the skill output uses slight lexical variants (e.g., “Process-level failure,” “Repeated incident”), this will return unknown and force the full section set. Was this vocabulary stabilized in the SKILL.md, or should this parser accept a few common synonyms (e.g., regexes anchored on the key noun)? Otherwise, retros that follow intent but vary phrasing could be over-graded as missing sections.

… zero-rule fresh project ## Summary Phase 0 of the accepted RFC "The rules Minsky ships" (Notion `3ce937f0`, Accepted 2026-09-04 via ask#11287). Five measured defects in the rules-selection and `init`/`compile` surfaces, plus a docs correction. Every one is correct to fix under any outcome of the RFC's remaining open questions, which is why this phase "activates now". The defects compose into one user-visible failure: on a fresh Claude Code project, the first `rules disable` a user ran wrote an unvalidated id into the committed config, and that config then resolved to **zero active rules** — while a bare `minsky compile` wrote 6 `.cursor/rules/*.mdc` files and a 90-byte `AGENTS.md` that nothing in the project reads, and any re-init deleted the user's selection along with every other key `init` does not own. **All five defects were re-reproduced live at `d667c9634` before being fixed**, rather than inherited from the 2026-09-01 measurement in the spec. ## Key Changes **SC1 — `rules enable|disable` reject an unknown id** (`config-operations.ts`). Validation lives in the domain so CLI and MCP both get it, and runs *before* any config read/write so a rejection cannot leave a partial `rules:` block. Valid ids are the on-disk `.minsky/rules` sources ∪ `DEFAULT_TEMPLATES` — the template half is deliberate, since a user may decline a rule `init` is about to scaffold or one they deleted by hand, and neither appears in `listRules`. **SC2 — `init --overwrite` merges** (`init/config-merge.ts`, wired in `init.ts`). Top-level merge: keys `init` emits are refreshed, everything else is preserved. Deliberately **not** a deep merge — mt#4699 *stopped* emitting `tasks.strictIds` and the `mcp:` block, and a deep merge would resurrect them from old configs forever. `createFileIfNotExists` is untouched; its other callers (`setup.ts`, `mcp/registration.ts`, every rule-file write) rely on plain overwrite semantics. **SC2 says "byte-for-byte"; here is exactly what the merge guarantees, measured.** Every VALUE is preserved exactly, and the result is byte-identical for input already in the serializer's canonical block form — which is what `init` and `rules enable|disable` themselves write, so it holds for every machine-generated config. It is *not* byte-identical for hand-authored input in other styles: measured, `disabled: [minsky-workflow]` (flow style) round-trips to block style, and a comment line is dropped. Both follow from the parse/stringify round-trip the criterion already scopes comment preservation out of, and the criterion's substance holds in all cases — the `rules:` section and every unowned key SURVIVE, where before they were deleted outright. A CST-level round-trip (`yaml`'s `parseDocument`) would buy byte-exactness and is deliberately **not** taken here: it would preserve comments in `init`'s merge while `rules enable|disable` strip them on the very next command, which is a worse guarantee than a uniform one. Recorded on SC2 with mt#573 as the owner for doing both together. **SC3 — `cursor-rules-ts` and `agents.md` are gated on the recorded harness** (`compile.ts`, mirrored in `src/hooks/pre-commit.ts`). Under `workspace.harness: claude-code`, a bare `minsky compile` no longer writes them. Two escapes: an explicit `--target` never reaches the probe, and an output that already exists stays maintained (per-target, not all-or-nothing). `claude.md` and `claude-rules` are never gated — they are the two channels Claude Code implements (mt#3107). `readRecordedHarness` fails **open**: an unreadable config gates nothing, so the failure direction is toward writing more. **SC4 — `init`'s scaffolded set is pinned** (`init/rule-templates.ts`). Six of seven registered templates, now documented and pinned by a test that names the omitted one. Took the RFC's "document the omission" branch — see `## Outcome` on the task for why. **SC5 — removed an `init --interface` flag from the docs** that has never existed. The 21 remaining `--interface` occurrences belong to `rules generate`, where it is real and required. **SC6 — `disabled` subtracts from the corpus instead of emptying it** (`rule-selection.ts`). The active set now starts from the full corpus; `presets`/`enabled` add (intersected with the corpus), `disabled` subtracts. ### Consumer added by the READY gate, not in the original spec `src/hooks/pre-commit.ts`'s `compileCheckTargets` is a **parallel implementation** of `minskyCompileTargetsFromPresence` — its own docblock says it is "kept in sync with" it. Gating only the domain side would have made the pre-commit staleness check demand `cursor-rules-ts` / `agents.md` outputs that a bare compile no longer produces, telling a claude-code project those files are stale forever with no invocation able to refresh them. `compile.test.ts` and `compile-check-targets.test.ts` were added to scope for the same reason. This is a **no-op in this repository**, which commits both outputs and so takes the already-exists escape. ### Two decisions the criteria delegated, both recorded on the task - **SC3 — `agents.md` is gated, not harness-agnostic.** The RFC settles it: §Phase 0 says a bare compile "stops also writing `.cursor/rules/` **and `AGENTS.md`**". Counter-argument (AGENTS.md is a cross-vendor convention) considered and rejected on evidence — the file `init` produced was 90 bytes of generated header — and it loses cheaply, since creating the file once opts a project back in permanently. - **SC6 — no allow-list mode is introduced.** Consequence stated plainly because it is user-visible: at Phase 0 `presets`/`enabled` are **inert**, because the base set is already the whole corpus. That follows from the Phase-0 substitution (tier defaults do not exist until Phase 1a), not from an independent design choice. The corpus intersection is *not* inert even now — `RULE_PRESETS` names 13 Minsky-only ids and 3 that exist nowhere. ### Scope condition recorded for Phase 2 The RFC says the active set "starts from **the tier defaults at the project's rung**". Those do not exist at Phase 0, so "the full corpus" is the degenerate case of that sentence. **mt#573 must re-derive the base set from tier defaults rather than inherit "full corpus" as if the RFC said it** — recorded on SC6, in the `resolveActiveRules` docblock, and in the task's `## Outcome`. ## Testing Full main suite green, plus per-criterion live verification against real scratch projects. Execution evidence: ``` $ bun scripts/run-tests-main.ts 17571 pass 0 fail Ran 17581 tests across 1137 files. [326.88s] $ bun scripts/run-tests-gated.ts → Change-scoped against merge base d667c96. 218 pass 0 fail Ran 218 tests across 15 files. [2.04s] run-tests-gated.ts: all test steps passed. $ bun scripts/run-related-tests.ts <all 7 changed source files> 383 pass 0 fail Ran 383 tests across 26 files. [4.44s] ``` **AT1** (`rules disable --id no-such-rule` exits non-zero, prints the id, config unchanged) — run through the real CLI in a scratch repo, both directions: ``` === md5 BEFORE === a0e04dece4c9440fc534bfaafa357a7f === (a) UNKNOWN id === Validation error: Unknown rule id "no-such-rule" — it is neither a rule in .minsky/rules nor a rule minsky init can scaffold, so nothing would be selected. The project config was not written. EXIT CODE: 1 === md5 AFTER === a0e04dece4c9440fc534bfaafa357a7f <-- unchanged mentions id: 0 === (b) KNOWN id (positive control) === ✅ Success ... disabled: ["minsky-workflow"] EXIT CODE: 0 <-- still writes ``` Pre-fix control, measured at `d667c9634` on the unmodified tree: returned `{"enabled":[],"disabled":["no-such-rule-xyz"]}` and persisted the id into `.minsky/config.yaml`. **AT2** (re-init preserves `rules:` and an unrelated key; `tasks`/`persistence` refreshed): ``` BEFORE: tasks.backend: minsky | rules.disabled: [minsky-workflow] | someUnrelatedKey: preserve-me $ minsky init --repo <scratch> --backend github-issues ... --overwrite AFTER: tasks.backend: github-issues <-- REFRESHED rules.disabled: [minsky-workflow] <-- preserved someUnrelatedKey: preserve-me <-- preserved ``` The refreshed `tasks.backend` is the discriminating half: a merge that preserved everything by declining to write would also have kept the user keys. Pre-fix control at `d667c9634`: both `rules:` and `someUnrelatedKey` were deleted. Round-trip fidelity measured directly, both styles: ``` case A — canonical block input: byte-identical output case B — "rules:\n disabled: [minsky-workflow]" plus a "# a user comment" line -> "rules:\n disabled:\n - minsky-workflow\n" (flow normalized, comment dropped) ``` **AT3** (claude-code scratch repo, no `.cursor/`, no `AGENTS.md`; bare `minsky compile`): ``` $ minsky compile [compile] Target "claude.md": 1 file(s) written [compile] Target "claude-rules": 0 file(s) written AT3 PASS: .cursor/rules does not exist AT3 PASS: AGENTS.md does not exist $ minsky compile --target cursor-rules-ts # escape (a) -> .cursor/rules file count: 6 # escape (b): with .cursor/rules now present, a bare compile maintains it $ minsky compile [compile] Target "cursor-rules-ts": 6 file(s) written -> AGENTS.md still absent <-- gates are per-target, not all-or-nothing ``` Pre-fix control at `d667c9634`: the probe returned `["cursor-rules-ts","claude.md","agents.md","claude-rules"]`. **AT4** (a unit test asserts the scaffolded id set exactly) — `rule-templates.test.ts`, 5 pass. It also pins *which* template is omitted, so adding a template to the registry without deciding whether `init` scaffolds it fails here. Pre-fix control: no such test existed. **AT5** (`grep` finds no `init --interface`): ``` $ grep -n -- 'init --interface' docs/rules/template-system-guide.md PASS: zero occurrences of 'init --interface' $ grep -n -- '--interface' docs/rules/template-system-guide.md | wc -l 21 # all `rules generate`, where the flag is real and required ``` **AT6** (`rule-selection.test.ts` gains a `disabled`-only case) — 10 pass. Run against the **unmodified** resolver first, as AT6 requires; the failure is recorded below. **SC5** is the one mechanically-executable success criterion; its command and output are the AT5 block above. Negative control — SC6 resolver: the new `disabled`-only case run against the unmodified resolver ``` error: expect(received).toEqual(expected) - Set { "a", "b" } + Set {} (fail) resolveActiveRules > mt#4866: a lone `disabled` entry subtracts from the full corpus... 8 pass | 1 fail ``` Negative control — SC1 validation: `assertKnownRuleId` neutralized, restoring the full pre-fix behaviour ``` 3 pass 4 fail ``` The 4 failures are exactly the reject-asserting cases; the 3 that still pass are accept-cases, which pass either way — correct, since accepting a valid id is the unfixed behaviour too. For SC2 and SC3 the controls are the **live pre-fix measurements against the genuinely unmodified tree** at `d667c9634`, quoted under AT2 and AT3. Per mt#4512 that is stronger than a partial revert, which risks leaving a state that is neither pre-fix nor post-fix. Typecheck: 0 errors across 8 projects (`infra/` skipped — deps not installed locally; CI covers it). Lint: 0 errors, 0 warnings across 4382 files. ## Live verification The end-to-end runs above are the live verification: real `minsky init` / `minsky compile` / `minsky rules disable` invocations against scratch projects under a sandboxed `HOME`, not in-process fakes. No deployed service is exercised by this change. Not structural under `/implement-task` §7a — no new persistence path, model-output channel, external-system probe, deploy-target wiring, or schema migration — so no separate verification artifact ships. The two pure functions (`mergeProjectConfigYaml`, `resolveActiveRules`) carry full behavioural coverage; the two fs-touching paths carry the live runs above. Deploy verification: `packages/domain/**` and `src/**` are deploy surface per `isDeploySurfaceFile()` — verified by running the predicate over the changed files rather than recalling the pattern list, which is why no commit here carries `[no-deploy-impact]`. The post-merge deploy will be verified per §10 with `notBefore` set to the merge timestamp and `expectCommitSha` set to the merge commit. ## Testable-design note `enableRule`/`disableRule` reach `RuleService` and `fs` directly rather than taking them injected. Rather than patch those collaborators with `spyOn`, `config-operations.test.ts` gives them a **real scratch workspace** (`mkdtemp` + `afterEach` cleanup) — no module patching at all, which is the better option under `testing-standards.mdc §Testable Design`. It carries a scoped `eslint-disable custom/no-real-fs-in-tests` with that rationale, matching the existing precedent in the sibling `crud-operations.test.ts`. Extracting an fs seam would change exported signatures with live consumers, which RFC Phase 0 does not scope. ## Sequencing PR #3253 (mt#3854, IN-REVIEW) touches the same `compile.ts` / `compile.test.ts` / `pre-commit.ts` / `compile-check-targets.test.ts` — verified at file level via `get_files`, not inferred from its title. The two changes are opposite in direction (it adds two presence-gated `codex-*` targets; this tightens two ungated ones), so this is a rebase rather than a semantic conflict. SC1, SC2, SC4, SC5 and SC6 touch no file any of the 17 open PRs changes. Full planning audit, premise checks and per-gate verdicts: mt#4866 `## Planning Audit (READY)`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_012aTeW8XXjVuEF5GykykSwq --- ## Review round 1 — response The reviewer's structured findings channel was empty; mt#2685 synthesized one BLOCKING placeholder from the conclusion prose. The prose carried two real concerns, so this was **not** treated as a malformed-review bypass case. Both are addressed. **R1-a — init's merge silently destroyed a config it could not read. Fixed; this was the more serious of the two.** Two paths: an unreadable file warned and overwrote, and an *unparseable* file returned the fresh content with **no warning at all**. My own code comment defended the second as "recoverable, and matches the pre-mt#4866 behaviour" — but pre-mt#4866 behaviour is precisely the data loss SC2 exists to stop, and it is not recoverable for the user's keys. It was worst on the path where it is least visible: a `--overwrite` in CI, where the warning reaches nobody and the command still reports success. Both paths now throw `UnmergeableConfigError` naming the file and the remedy. SC2 requires every unowned key to survive; when the file cannot be read there is no way to honour that, so refusing is the only faithful outcome. A file that *parses* but holds no mapping still does not throw — there is nothing to lose, and refusing would block a legitimate re-init over an empty config. That discriminating pair is tested. Verified live on a config carrying user keys plus a deliberate syntax error: ``` BEFORE md5: 607f1b171afc8efdca26a1bb9848cfa3 $ minsky init --repo <scratch> ... --overwrite Error: Cannot merge into the existing config at <path>: it is not valid YAML, so the keys `minsky init` does not own cannot be preserved. Refusing to overwrite — that would silently discard them. Repair the file, or move it aside and re-run `minsky init --overwrite`. EXIT: 1 AFTER md5: 607f1b171afc8efdca26a1bb9848cfa3 <-- untouched ``` Before this round, that same input was replaced and reported success. **R1-b — the harness gate was invisible. Fixed.** The stated mechanism ("when no harness is recorded, behavior diverges") does not hold: with no harness both the compile probe and the pre-commit check fail open identically, which is the pre-mt#4866 set. But the underlying point is right — a gated-out target simply vanished from the checked set, which reads identically to a target that was never applicable, so a project could be told its outputs are current while two of them were not being maintained at all. Added `minskyCompileTargetsWithGateReport` / `compileCheckTargetsWithGateReport`. The pre-commit check now prints which targets it is not checking and why: ``` ℹ️ Not checking cursor-rules-ts, agents.md — this project records workspace.harness: claude-code and does not have those outputs on disk, so a bare `minsky compile` does not produce them either (mt#4866). Run `minsky compile --target <name>` to opt in. ``` The report-returning function is the implementation and the list-returning one delegates to it, with a test pinning that the two agree, so they cannot fork. **Documentation impact — checked at source, not assumed.** The finding named four areas as likely falsified and stated "I did not read other compile-related docs to verify consistency" and "I did not open those docs". I read each: | Claimed risk | Verified | | --- | --- | | Re-init semantics | Only `docs/rules/template-system-guide.md` mentions `--overwrite`, and it is `rules generate --overwrite` — a different command. Not falsified. | | enable/disable validation | Zero docs mention `rules enable` or `rules disable`. Nothing to update. | | Resolver semantics | `architecture.md:299` and `theory-of-operation.md:159` make *location* claims ("stored in `.minsky/config.yaml` under the `rules` key"), not semantics claims. No doc asserts allow-list behaviour. Not falsified. | | Compile target gating | No doc states which targets a bare `minsky compile` writes. Not falsified. | So nothing existing was made wrong. The underlying point — users need to understand the new behaviour — stands regardless, so `docs/rules/template-system-guide.md` gains three sections: re-init merge semantics including the refusal and the formatting-normalization caveat, compile target gating with both escapes, and enable/disable validation plus the Phase-0 inertness of `presets`/`enabled`. **Non-blocking items.** Phase-0 resolver inertness: now documented in the guide, the `resolveActiveRules` docblock, the PR body and the task's `## Outcome`. Machine-readable hint on fail-open harness read: covered by the gate report above. Shared gate helper to avoid mapping drift: **not** taken, deliberately — importing the domain compile module into a per-commit hook drags `createMinskyCompileService` and every target into its import graph, which is the reason `compileCheckTargets` already mirrors rather than imports. The duplication is documented on both copies; ADR-016 phase 4 (mt#2293) consolidates the two pre-commit checks and is where it is properly resolved. Known-id validation vs the future product corpus: validation reads `DEFAULT_TEMPLATES`, so when Phase 1 replaces the scaffold set with the package corpus the valid-id set follows it automatically. Execution evidence (R1): ``` $ bun scripts/run-related-tests.ts <4 changed source files> 309 pass 0 fail Ran 309 tests across 17 files. [3.02s] ``` Typecheck 0 errors across 8 projects; lint 0 errors, 0 warnings across 4382 files. Negative control — R1-a fail-closed: the live run above IS the control in both directions. The same input against the pre-R1 tree replaced the file and exited 0; against this tree it exits 1 with the file byte-identical. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>

…ch file was left alone ## Summary `minsky init` in a project that already has agent instructions silently destroyed them. **Measured pre-fix at `1c7a6366c`** (the control for AT1): a scratch repo carrying a hand-written **140-byte** `CLAUDE.md` — house conventions, a "tabs not spaces" rule, a "never touch `legacy/` without asking Dana" rule — ran `minsky init` and got back **15,085 bytes**, `grep -c` for the three original rules returning **0**, and **no warning**. In the same run its own `.claude/rules/acme-house-style.md` survived byte-identical. That asymmetry is the finding. Minsky plays nice with foreign content in every per-file channel it writes and claimed sole ownership only of the two monolithic markdown files — by **inheritance** rather than by choice, because in the Minsky repository `CLAUDE.md` genuinely IS wholly generated, so the plant's posture shipped into the product without the user-owned case ever being in frame. Raised by the principal mid-review of PR #3629: *"are we assuming sole ownership or will we play nice?"* ## What changed Ownership keys on the generation banner, in **one shared predicate** (`packages/domain/src/compile/monolithic-ownership.ts`) with three consumers that have to agree exactly — a disagreement between any two is either a destroyed user file or a Minsky repo that can no longer refresh its own `CLAUDE.md`: 1. **The two writers refuse the write** (`targets/claude-md.ts`, `targets/agents-md.ts`). This is the floor rather than target selection alone, because `runMinskyCompile` returns early on an explicit `--target claude.md` and never reaches the probe — a selection-only guard would leave the one invocation an operator reaches for by name still destroying the file. 2. **`listOutputFiles` reports no outputs** for a foreign file, which is what stops `--check` calling it stale. `checkStaleness` drives its whole comparison off that list, so answering there covers every caller at once. 3. **Target selection drops the target**, with a `kind: "foreign"` gate entry distinct from the existing `"harness"` one — the two have *opposite* remedies, and offering `--target` as the fix for a foreign skip would overwrite the file the gate is protecting. Absent is not foreign (a fresh project still gets the full output), and an unreadable file fails **open** — the other direction would silently stop maintaining a `CLAUDE.md` that is genuinely ours, which is an inert pipeline with no error anywhere. **SC3 needed a gate ADDED, not the narrowing the spec described.** `claude.md` was pushed unconditionally (`compile.ts:143`) and had no presence rule to narrow, so a literal reading of "narrow the presence-based rule" would not have reached the file this task is named for — and without it SC4 cannot pass, because `compile --check` would keep reporting a foreign `CLAUDE.md` stale. `AGENTS.md`'s already-exists escape is narrowed the same way and applied **unconditionally**: `existingOutputs.agentsMd` is only ever consulted under claude-code, so narrowing that field alone would have left a Cursor project's hand-written `AGENTS.md` unprotected. `src/hooks/pre-commit.ts`'s `compileCheckTargets` is a parallel implementation of the same mapping and is gated in lockstep — the same trap mt#4866 recorded on its own PR. Without the mirror, a project with its own `CLAUDE.md` is told at every commit that it is stale, and the invocation that would refresh it is refused by the writer, so the operator has no way out at all. **Silence would reproduce the defect quietly**, so both surfaces report. `init` warns *before* its unreachability warning — with `CLAUDE.md` left alone every base rule is also unreachable, and that message on its own sends the operator hunting for a frontmatter problem that does not exist. `runMinskyCompile` seeds the report from the gate, because a gated-out target never runs and so its own refusal never fires. The banner literal was duplicated in both writers; it now lives in `banner-constants.ts` beside the detection patterns, which is the module that exists (mt#1798) to keep emission and detection from drifting. Adding a third copy for the ownership predicate is exactly what it was built to prevent. ## Known interim cost — recorded, not papered over `.claude/rules/` only accepts rules that declare globs AND `alwaysApply: false` (`isEligibleForClaudeRules`), and every base rule is `alwaysApply: true`. So a project whose `CLAUDE.md` Minsky does not own now receives **zero always-apply rules**. Verified in the post-fix run: `.claude/rules/` got only the user's own file. That is strictly better than destroying their file, and it is not silent — it is what SC2's message says, in as many words. Where the rules should go instead is **ask#11711** (open, principal-owned), which mt#4986's spec scopes out of this change and which should be answered before mt#573 is planned. ## Execution evidence: `bun test --preload ./tests/setup.ts --timeout=15000 packages/domain/src/compile/ packages/domain/src/init-backend-selection.test.ts src/hooks/compile-check-targets.test.ts` ``` 381 pass 0 fail 952 expect() calls Ran 381 tests across 16 files. [2.05s] ``` Broader sweep — `bun scripts/run-related-tests.ts packages/domain/src/compile/compile.ts packages/domain/src/compile/monolithic-ownership.ts packages/domain/src/init.ts src/hooks/pre-commit.ts packages/domain/src/rules/compile/banner-constants.ts`: ``` 1155 pass 0 fail 4534 expect() calls Ran 1155 tests across 49 files. [4.20s] ``` Typecheck: 0 errors across 8 projects (`infra/` skipped, deps not installed locally; CI covers it). Lint: 0 errors, 0 warnings across 4,383 files. ### SC1 — a non-banner-carrying CLAUDE.md/AGENTS.md is never written Live, post-fix, in a scratch repo with a foreign `CLAUDE.md` (140 B) and `AGENTS.md` (56 B): ``` $ bun run src/cli.ts init --repo <scratch> --backend minsky --rule-format minsky --mcp false minsky init: wrote 4 base rule(s) to .minsky/rules. minsky init: <scratch>/CLAUDE.md was left untouched — it does not carry Minsky's generated-file banner, so it is treated as yours and is never overwritten. ... minsky init: 4 scaffolded rule(s) are not reachable by Claude Code — ... $ md5 -q CLAUDE.md AGENTS.md # unchanged from before the run 2449d2706880e62d6376cb5f1ee94342 4df191c745ef025e3b7a9c4dc40e5464 ``` The floor, via an explicit `--target` (the path that bypasses the probe): ``` $ bun run src/cli.ts compile --target agents.md [compile] <scratch>/AGENTS.md was left untouched — ... $ md5 -q AGENTS.md 4df191c745ef025e3b7a9c4dc40e5464 # byte-identical ``` Unit: `does not write a foreign CLAUDE.md, and leaves it byte-identical` / `... a foreign AGENTS.md ...` — both read the file back rather than trusting `filesWritten`. ### SC2 — the operator is told which file, why, and where the rules went Both surfaces, live. `init` above; bare `compile`: ``` $ bun run src/cli.ts compile [compile] <scratch>/CLAUDE.md was left untouched — it does not carry Minsky's generated-file banner, so it is treated as yours and is never overwritten. Minsky's rule sources are in .minsky/rules/; nothing loads them into your agent automatically while this file is yours, so an agent has to ask for one by name with `rules_get <name>`. To hand the file over to Minsky instead, move it aside and re-run. [compile] <scratch>/AGENTS.md was left untouched — ... [compile] Target "cursor-rules-ts": 4 file(s) written [compile] Target "claude-rules": 0 file(s) written ``` Unit: `reports the skip with a reason naming the file` (both targets); `foreignOutputSkipReason > names the file, the reason, and where the rules actually are`; and `reports the skip BEFORE the unreachability warning it explains` — the ordering is load-bearing, not cosmetic. ### SC3 — selection keys on the banner Unit, in `compile.test.ts` `foreign-ownership gate (mt#4986 SC3)`: `drops claude.md when CLAUDE.md is the user's`; `drops agents.md when AGENTS.md is the user's, on ANY harness`; `leaves the per-file targets alone`; `changes nothing when both monolithic outputs are ours`; plus the two gate-kind tests. End-to-end through the probe: `a hand-written AGENTS.md does NOT keep agents.md`, `a hand-written CLAUDE.md drops claude.md, which had no gate at all before`. Mirrored in `src/hooks/compile-check-targets.test.ts`. **Two existing tests were updated rather than left passing**, and this is a deliberate behavior change, not a fixture tidy-up: `mt#4866: an existing AGENTS.md keeps agents.md under claude-code` used a bare `"# Agents\n"` fixture. Under this change that is a *hand-written* file, so asserting it keeps the target would pin the exact behaviour mt#4986 removes. The fixture now carries the banner — which is what an `AGENTS.md` that escape was written for actually looks like — and the hand-written case is pinned separately as its own test. ### SC4 — a foreign file is not stale, and is still hand-editable ``` $ bun run src/cli.ts compile --check --target claude.md [compile] <scratch>/CLAUDE.md was left untouched — ... "filesWritten": [], "definitionsIncluded": [], "stale": false EXIT=0 ``` Edit guard, exercised live against the real hook rather than read: ``` $ echo '{"tool_name":"Edit","tool_input":{"file_path":"<scratch>/CLAUDE.md",...}}' \ | bun .minsky/hooks/check-generated-file-edit.ts EXIT=0 # foreign file — permitted $ echo '{"tool_name":"Edit","tool_input":{"file_path":"<scratch>/AGENTS.md",...}}' \ | bun .minsky/hooks/check-generated-file-edit.ts {"permissionDecision":"deny", ... "Marker: [HTML comment: Generated by]"} ``` The pair is the point: the guard can fail, and it discriminates correctly. ### SC5 — regression coverage for both targets, at `init` and at bare `compile` New: `monolithic-ownership.test.ts` (12 cases). Extended: `claude-md.test.ts` and `agents-md.test.ts` (5 each, target level), `compile.test.ts` (selection + probe), `init-backend-selection.test.ts` (3, the `init` surface), `compile-check-targets.test.ts` (3, the pre-commit mirror). ### AT3 — a banner-carrying file is still regenerated, and this repo is unaffected ``` $ printf '\n\n# stale\n' > AGENTS.md before: 83 bytes $ bun run src/cli.ts compile --target agents.md [compile] Target "agents.md" output size: 14968 chars after: 15112 bytes # regenerated, no "left untouched" message ``` Minsky's own repo, `bun run src/cli.ts compile --check` in the session: all 8 targets selected (`claude.md` and `agents.md` among them), no staleness, exit 0. Both gates are inert here because both files carry the banner. ### AT5 — the per-file channels are unchanged In the post-fix scratch run, `.claude/rules/acme-house-style.md` is byte-identical (md5 `50820f15b26c7eebaa643e56c6113efd`) and `cursor-rules-ts` wrote its 4 files normally. Pinned by `leaves the per-file targets alone`. ## Negative control: The fix routes every path through one predicate, so forcing `isForeignMonolith` to return `false` restores the **complete** pre-fix behaviour — writers write, selection never gates, `listOutputFiles` returns the path — rather than reverting one line and leaving a state that is neither pre- nor post-fix (mt#4512). Observed with that revert in place: ``` 78 pass 11 fail Ran 89 tests across 4 files. (fail) isForeignMonolith > is true only for the foreign case (fail) probeMinskyCompileTargets > mt#4986: a hand-written AGENTS.md does NOT keep agents.md (fail) probeMinskyCompileTargets > mt#4986: a hand-written CLAUDE.md drops claude.md, ... (fail) claudeMdTarget > does not write a foreign CLAUDE.md, and leaves it byte-identical (fail) claudeMdTarget > reports the skip with a reason naming the file (fail) claudeMdTarget > reports NO definitions as included when the output was not written (fail) claudeMdTarget > reports no output files for a foreign CLAUDE.md, so --check cannot call it stale (fail) agentsMdTarget > does not write a foreign AGENTS.md, and leaves it byte-identical (fail) agentsMdTarget > reports the skip with a reason naming the file (fail) agentsMdTarget > reports NO definitions as included when the output was not written (fail) agentsMdTarget > reports no output files for a foreign AGENTS.md, so --check cannot call it stale ``` Reverted before commit; the suite is green above. **What this control does NOT cover, stated rather than implied:** the `init`-reporting tests and the pre-commit mirror tests feed `skippedForeignOutputs` / `foreignOutputs` in directly, so they do not route through the predicate and stayed green under the revert. They test the reporting and mapping layers, which did not exist pre-fix at all — there is no earlier state in which they could have run. ## Live verification Everything under Execution evidence above was run live against a sandboxed scratch repo (empty `HOME`/`XDG_*`, `CLAUDECODE=1`) using the session's source via `bun run src/cli.ts` — not the bundle, which `scripts/cli-entry.ts` can serve stale. ## Deploy verification: All ten changed source files return `true` from `isDeploySurfaceFile`, so **no `[no-deploy-impact]` claim is made** and §10 applies: after merge, `deployment_wait-for-latest` for `minsky-mcp` and `reviewer` with `notBefore` = the merge timestamp and `expectCommitSha` = the merge SHA, health identity asserted from the response body, and build identity resolved by correlating the deploy workflow runs to the merge SHA if it comes back `indeterminate` (both are image-source services, so it will). No new external-system integration, so no live-integration exercise is owed. ## Judgment calls - **Proceeded despite a file-level collision with open PR #3253** (mt#3854), which modifies the same two functions in `compile.ts`. Its branch predates mt#4866 entirely — 230 lines against main's 325, with no harness gate at all — so it must rebase over a rewrite of those functions whether or not this lands. Waiting was the worse option in the other direction: #3253 is blocked on its own rebase, so "wait for it to merge" is an open-ended hold on a defect that destroys user files today. The four files carrying most of this change (`claude-md.ts`, `agents-md.ts`, `init.ts`, `staleness.ts`) are clear of it. - **`definitionsIncluded` is empty on the refusal path**, not the rules that would have been emitted. Reporting them as included would tell `init`'s reachability accounting (mt#4770) that four base rules reached the agent when they reached nothing — the same false-completion shape as listing an unwritten file in `filesWritten`. - **`cursorRules` is deliberately not narrowed.** `.cursor/rules/` is a per-file channel that already coexists correctly with hand-authored files; keying its selection on a banner would be a regression. Task: mt#4986 · Planning audit and gate walk on the task record. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>

…rget, not a new CLAUDE.md ## Summary mt#4986 stopped Minsky destroying a project's own `CLAUDE.md` and **deliberately left a gap**: `.claude/rules/` admitted only glob-scoped rules, so a project whose `CLAUDE.md` we do not own received **zero** always-apply rules. **ask#11711** (closed 2026-09-05) chose the channel. Selected option LABEL: *"Deliver via .claude/rules/ (agent recommendation)"*. ## The harness contract this rests on Read at source 2026-09-05 (`code.claude.com/docs/en/memory`), two exact statements: > Rules without `paths` frontmatter are loaded at launch with the same priority as `.claude/CLAUDE.md`. > Rules without a `paths` field are loaded unconditionally and apply to all files. **The near-miss is `paths: []`.** An empty list is still a `paths` **field**, so the rule is path-scoped with zero matching patterns — loaded never. The harness keys on the field's ABSENCE, so the serializer omits the whole frontmatter block, which also moves the banner to line 1 (still inside the five-line window the edit guard and the orphan sweep both scan). Corroborating, and a different KIND of channel: `.claude/rules/` path-scoped injection was observed working natively in a live Claude Code session in this repo, and a grep over `.minsky/hooks/` + `.claude/hooks/` confirms no Minsky hook performs it — it is the harness's own loader. ## What changed **One predicate keeps the two channels mutually exclusive.** `claudeMdIsOurs` answers "does the always-apply set already have a home?", and always-apply rules land in `.claude/rules/` only when the answer is no. Without it, widening the eligibility arm would have emitted **this repository's 138,167-char always-apply corpus into `.claude/rules/` as well as `CLAUDE.md`** — roughly doubling its always-loaded context and blowing the 135,000/145,000 budget from the other side. **`claude.md` is selected only for a `CLAUDE.md` we generated.** We MAINTAIN ours; we do not CREATE one. The absent case used to fall through to "write it", which is how a fresh managed project got a 15 KB `CLAUDE.md`. An operator can still opt in once with `--target claude.md` (that path returns early and never reaches the probe), after which the file carries the banner and stays maintained — the same affordance the `AGENTS.md` harness gate already offers. **Eligibility keys on `alwaysApply`, not on tier and not on a rule-name list.** That is what keeps the deliberately on-demand tier out (mt#4735 SC4 / mt#3107): all four operational-reference rules carry **no `alwaysApply` key at all**, so neither arm can reach them. The trap a name- or tier-keyed implementation would hit is real — `task-status-workflow-protocol` is on-demand in the plant and `alwaysApply: true` in the product corpus. Same id, opposite dispositions, two files. ## Two things the spec did not carry **The skip message became FALSE.** It told the operator *"nothing loads them into your agent automatically … an agent has to ask for one by name with `rules_get`"* — accurate under mt#4986, and wrong the moment `.claude/rules/` started carrying the set. Caught by reading the actual output of a bare `compile`, not by re-reading the diff. It is now **per-file**: `CLAUDE.md` gets the reassuring truth, `AGENTS.md` keeps the honest one, because `.claude/rules/` is Claude-Code-only and Claude Code reads `CLAUDE.md`, not `AGENTS.md`. One shared sentence made one of them a lie whichever way it was written. **`foreignOutputs` + `ownedOutputs` as separate booleans made `{foreign: true, owned: true}` representable** — a state that cannot exist, whose resolution depended on which check happened to run first. Collapsed into one `ownership` tri-state per file (`generated` / `foreign` / `unreadable` / `absent`), so the illegal state is unrepresentable rather than merely unreachable. The type change then found every affected fixture for me. ## Execution evidence: `bun scripts/run-related-tests.ts packages/domain/src/compile/targets/claude-rules.ts packages/domain/src/compile/compile.ts packages/domain/src/compile/monolithic-ownership.ts packages/domain/src/init.ts src/hooks/pre-commit.ts` ``` 410 pass 0 fail 2025 expect() calls Ran 410 tests across 21 files. [3.01s] ``` Typecheck: 0 errors across 8 projects. Lint: 0 errors, 0 warnings across 4,385 files. ### SC1 / SC2 — globless emission, banner still in the first five lines AT3's run (below) wrote four files; each opens with the banner on line 1 and has no frontmatter: ``` --- key-workflows.md ---  # Key Workflows (via skills) ``` Edit guard exercised live against the real hook on one of them: ``` $ echo '{"tool_name":"Edit","tool_input":{"file_path":"<scratch>/.claude/rules/key-workflows.md",...}}' \ | bun .minsky/hooks/check-generated-file-edit.ts {"permissionDecision":"deny", ... "Marker: [HTML comment: Generated by]"} ``` Unit: `emits an always-apply rule with NO frontmatter block at all` (asserts `not.toContain("paths:")`), `keeps the banner inside the five-line window the edit guard scans`, `still emits paths frontmatter for a glob-scoped rule`. ### SC3 — no CLAUDE.md is created ``` $ bun run src/cli.ts init --repo <fresh scratch, no CLAUDE.md> --backend minsky --rule-format minsky --mcp false minsky init: wrote 4 base rule(s) to .minsky/rules. minsky init: 13 declinable rule(s) ship with Minsky but were NOT installed — ... Project initialized successfully. $ test -e CLAUDE.md && echo YES || echo no $ ls .claude/rules/ key-workflows.md minsky-session-workflow.md operational-safety-dry-run-first.md task-status-workflow-protocol.md ``` **Pre-fix control:** this path wrote a 15,085-byte `CLAUDE.md` (measured 2026-09-04). Unit: `mt#5003: with no CLAUDE.md of ours, claude.md is NOT selected`, and end-to-end through the probe as a discriminating pair — `detects all four source dirs together` (with an owned CLAUDE.md) vs `the same repo WITHOUT a CLAUDE.md of ours omits claude.md`, identical fixtures but one file. ### SC4 — this repository is NOT duplicated ``` $ bun run src/cli.ts compile --check [compile] Target "claude.md": 1 file(s) written [compile] Target "claude.md" output size: 138167 chars [compile] Target "claude-rules": 17 file(s) written ... EXIT=0 ``` `claude.md` still selected, `ruleContentChars` still **138,167**, `.claude/rules/` still exactly **17** files, nothing stale. Unit: `listOutputFiles EXCLUDES an always-apply rule when a CLAUDE.md of ours carries it`. ### SC5 — reachability accounting The AT3 run above emits **no** *"not reachable by Claude Code"* warning: the base rules are now reached, and `init`'s accounting (mt#4770) reads that from the pipeline rather than re-deriving it. ### SC6 — the doc paragraph this makes false `docs/rules/template-system-guide.md`'s *"What it costs you today"* paragraph said the base rules reach the agent through no automatic channel. Replaced, with an explicit note that the earlier text was true between mt#4986 and mt#5003 and no longer is — so a reader who remembers it is corrected rather than confused. ### SC7 — the on-demand tier is untouched Unit: `leaves the deliberately on-demand tier ineligible under BOTH arms`. Measured basis, recorded on the spec: all four plant copies of `architectural-bypass-prevention`, `efficient-database-queries`, `task-status-workflow-protocol`, `verification-checklist` carry no `alwaysApply` key, so an `alwaysApply`-keyed predicate cannot reach them. ### AT2 — the mt#4986 regression, plus the new delivery ``` $ md5 CLAUDE.md # before init f3fe30755140644bc712bd4881404e5a $ bun run src/cli.ts init --repo <scratch> ... $ md5 CLAUDE.md # after f3fe30755140644bc712bd4881404e5a # byte-identical $ ls .claude/rules/ key-workflows.md minsky-session-workflow.md operational-safety-dry-run-first.md task-status-workflow-protocol.md ``` The user's file is untouched **and** they now receive the rules — which is the whole point. The per-file message, both branches, from one bare `compile`: ``` [compile] <scratch>/CLAUDE.md was left untouched — it does not carry Minsky's generated-file banner, so it is treated as yours and is never overwritten. Minsky's rules still reach your agent: the always-apply ones are written to .claude/rules/ as paths-less files, which Claude Code loads at launch at the same priority this file would have had. ... [compile] <scratch>/AGENTS.md was left untouched — ... Minsky's rule sources are in .minsky/rules/; this harness has no channel that loads them automatically while this file is yours, so an agent has to ask for one by name with `rules_get <name>`. ... ``` ### Parity harness — unaffected by design ``` $ bun scripts/verify-compile-parity.ts ✅ compile-parity: all 5 checks passed ``` Planning flagged this as a consumer that would report a false divergence. `includeAlwaysApply` defaults **false**, so the legacy twin and the harness compare like for like and no change to the script was needed. ## Negative control: `isEligibleForClaudeRules` is the single gate every path routes through, so forcing its always-apply arm off restores the complete pre-fix behaviour rather than reverting one line into a state that is neither pre- nor post-fix (mt#4512): ``` 61 pass 4 fail Ran 65 tests across 2 files. (fail) claude-rules: always-apply eligibility (mt#5003) > is eligible when the always-apply channel is this target (fail) claude-rules: always-apply eligibility (mt#5003) > needs no globs on the always-apply arm (fail) claude-rules: always-apply eligibility (mt#5003) > buildClaudeRulesContent includes always-apply rules only when told to (fail) claudeRulesTarget > listOutputFiles INCLUDES an always-apply rule when no CLAUDE.md of ours exists (mt#5003) ``` Reverted before commit; the suite is green above. **What it does NOT cover, stated rather than implied:** the target-SELECTION tests (`claude.md` no longer created) and the pre-commit mirror do not route through this predicate, so they stayed green under the revert. They cover a different half of the change, and the type-level collapse to `ownership` is what proved they were all updated — the compiler enumerated the sites, I did not. ## AT1 — the one check I could not run, and who can The spec's AT1 is a live `/context` in a scratch repo confirming a globless `.claude/rules/*.md` actually appears under **Memory files**. That needs a real Claude Code session started in that directory, which is on your side of the boundary, not mine. The claim is `strong-evidence` — vendor doc read at source, two exact quotes, plus the observed native path-scoped injection — and it is not live-probed for the globless case specifically, because every one of this repo's 17 rule files carries `paths:` and so provides no evidence either way. **If it fails, the premise is wrong and this should be reverted rather than worked around.** A scratch repo is set up at `/private/tmp/claude-501/-Users-edobry-Projects-minsky/c14a6eab-b66d-4761-92d7-ba30bc9ad394/scratchpad/sb5/repo` if you want to point a session at it. ## Deploy verification: Five of six changed files return `true` from `isDeploySurfaceFile` (the doc does not), so **no `[no-deploy-impact]` claim is made** and §10 applies: after merge, `deployment_wait-for-latest` for `minsky-mcp` and `reviewer` with `notBefore` = the merge timestamp and `expectCommitSha` = the merge SHA, health identity asserted from the response body, and build identity resolved by correlating both deploy workflow runs to the merge SHA when it returns `indeterminate` (both are image-source services, so it will). No new external-system integration. ## Judgment calls - **Proceeded despite the PR #3253 collision on `compile.ts`**, third time in this chain. Its branch predates mt#4866 AND mt#4986, so it must rebase over both rewrites of those functions regardless; this task gates mt#573 and the first external user. Files clear of it carry most of the work. - **The default is the NEW behaviour.** `ownership` absent means "absent", so a caller that forgets to probe stops creating `CLAUDE.md` rather than silently keeping the old behaviour. That is what made ~20 existing tests fail — each was asserting the old contract, and each now says which case it means. - **The legacy `rules/compile/targets/claude-rules.ts` twin is deliberately unchanged.** Its only consumers are on the retired `rules compile` path, and mt#2996 deletes it; widening it would grow the blast radius for no live consumer. Task: mt#5003 · Planning audit, the corrected duplicate-check record, and SC7's measured basis are on the task record. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>

… with one proximity check ## Summary `wall-of-text-detector`'s `DEPTH_REQUEST_PATTERNS` withholds the over-budget reminder when the principal recently asked for depth. Eight of its ten entries also matched the NEGATION of their phrase — "dont walk me through everything, just the summary", "no need to give me the full breakdown", "please do not go into more detail" — so the reminder was suppressed exactly when the principal asked for brevity (the unsafe direction by the list's own narrowness note). Verified at planning: all 8 return `matched: true` on the shipped list. Measured in the live calibration log (174 records since 2026-08-30, the only copy on disk): 16 depth-suppressed records, 13 replayable through the hook's own window logic, **all 13 `help-me-understand`, 0 negated** — so this ships as a latent robustness fix, sized accordingly. ## Key changes - `.minsky/hooks/wall-of-text-detector.ts` — `isNegatedDepthRequest(text, index)` + `DEPTH_REQUEST_NEGATOR_RE`: a match is rejected when a negator (`don't`/`dont`/`do not`/`no need to`/`never`/`not`/`rather than`/`instead of`/`without`) ends within three tokens before the phrase in the same clause (boundaries: `,;:.!?()—–`, newline, and contrastive `but`). `detectDepthRequest` now scans every occurrence of every entry, so a negated first mention does not hide a later genuine request. **One mechanism at the seam, not eight regex edits** (mt#4070's drift argument), and **not per-entry anchoring** like `tell-me-more`: 9 of the 13 live suppressions are mid-sentence ("…and also, help me understand…", "proceed, but first help me understand…") and the anchored form misses 4 of 6 live prompt shapes — anchoring would un-suppress most genuine requests, which is the friction incident mt#3112 exists to prevent. The negator list grows only on a calibration record naming a missed negator, per the list's own evidence-before-expansion discipline (docblock). - `.minsky/hooks/wall-of-text-depth-request.test.ts` — `mt#5052` describe block, 7 tests. - `.claude/hooks/wall-of-text-detector.ts` — regenerated mirror. ## Testing Spec ATs use the task's own numbering. Execution evidence: - **AT1** — the spec's eight negated prompts return `matched: false` (`AT1 — each negated prompt from the spec's table is left unmatched`). - **AT2** — every entry's calibrated phrase still matches under the same name, plus the mid-sentence live shapes and the benign-negator shape "I'm not sure I follow — help me understand the peez thing" (`AT2 — …` ×2, `the guard's window is tight …`). - **AT3** — `bun scripts/measure-depth-request-widening.ts --log <state-dir>/wall-of-text-calibration.jsonl` → `window: head -120 -> 120 records; 75 injected / 45 suppressed`, `AT1 — newly suppressed: 3 (expected exactly 3)`, `PASS — AT1, AT2, AT3 hold on this window`. (The script exercises the entries directly, so this proves the list is untouched; the guard's effect on the live matches is the planning replay — 0 of 13 negated — plus the mid-sentence fixtures above.) - **AT4** — `bun scripts/run-guard-canaries.ts` → `[PASS] wall-of-text-detector (registry, expects=calibration)`; `Total: 75 Passed: 73 Failed: 0 Missing: 2` (the two missing are pre-existing, unrelated to this guard). - **AT5** — the negative control below. ``` $ bun test --preload ./tests/setup.ts --timeout=15000 ./.minsky/hooks/wall-of-text-depth-request.test.ts ./.minsky/hooks/wall-of-text-detector.test.ts ./.minsky/hooks/wall-of-text-turn-window.test.ts 169 pass 0 fail Ran 169 tests across 3 files. [80.00ms] $ bun scripts/run-related-tests.ts .minsky/hooks/wall-of-text-detector.ts Ran 2899 tests across 61 files. [23.90s] run-related-tests.ts: 61 related test file(s) passed ``` `validate_typecheck` (session): 0 errors across 8 projects. ESLint on the two source files at `--max-warnings=0`: clean. `bun run format:check`: clean. Negative control: with the guard disabled in place (`if (true || !isNegatedDepthRequest(…))`) and the new tests kept: ``` (fail) mt#5052 — negated depth requests do not suppress > AT1 — each negated prompt from the spec's table is left unmatched (fail) mt#5052 — negated depth requests do not suppress > the guard's window is tight — a negator outside it does not defeat a request (fail) mt#5052 — negated depth requests do not suppress > the guard reaches a negator up to three tokens before the phrase 11 pass 3 fail Ran 14 tests across 1 file. ``` The AT2 tests (genuine phrases, mid-sentence shapes) pass in both states, as pinned; restored afterwards (`grep -c "NEGATIVE CONTROL"` → 0, 169/169). `[no-deploy-impact]` — `isDeploySurfaceFile` returns `false` for all three changed files (run over the diff, not recalled). ## Not in this PR The `.codex/hooks` mirror (PR #3253 makes it a compile output); PR #3412's disjoint edits to the same file (an import and `main()`'s catch) — no conflict with this region. 🤖 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>

## Summary Phase 4 of the compile-pipeline convergence (mt#2293 / ADR-016). The mt#3058 cutover moved `claude.md` / `agents.md` / `claude-rules` onto the new-pipeline `runCompileCheck` and left the legacy `runRulesCompileCheck` as a no-op shell still registered as pre-commit Step 9 under the instrumented name `rules-compile-check`. This deletes the remainder, so one compile-staleness step remains. ## Changes - `src/hooks/pre-commit.ts` — Step 9 registration and `runRulesCompileCheck` deleted; the surviving `compile --check` step is now Step 9 and the only compile-staleness step, over all seven targets, still carrying `MINSKY_SKIP_SIZE_BUDGET`. `classifyCompileCheckError` drops its `kind` parameter (the only caller passed `"compile"`; the legacy prefix has no emitter in the hook any more) and its stale "legacy only" comment on the size-budget branch is corrected — `packages/domain/src/compile/size-budget-report.ts:93,119` emits both budget markers under `[compile --check]` since the cutover. Its stale-case hint now reads `minsky compile --target <t>`, the same shape as the compile CLI's own hint (R1). Comments naming the deleted method updated. - `src/hooks/rules-compile-check.test.ts` → `src/hooks/compile-check.test.ts` — same 37 cases, markers repointed at `[compile --check]`; plus (R1) a pin on the hint shape and a two-test pin that pre-commit wires `compile-check` exactly once and the retired name not at all, and that the guard roster carries one and retires the other. - Interceptor registries: `rules-compile-check` moved to the RETIRED stratum rather than deleted (R1) — `RETIRED_GUARD_NAMES` entry (lastSeen 2026-09-11), a retired-stratum description with `provenance: [KNOWN_NAMES]`, retired coordinates, and the authored-point manifest in `interceptor-coordinates.test.ts` — so its 4,435 historical fire-log rows keep resolving instead of reading as anomalies. The `compile-check` description now says it covers the rule-edit gap and its provenance names the repointed test. Generated `.claude/hooks/` copies and `src/generated/interceptor-catalog.json` regenerated. - Docs: ADR-016 gains `### Phase 4 shipped (mt#2993)`; `docs/architecture.md:330` and a `crud-operations.ts` comment no longer name the deleted method. `docs/architecture/evaluation-loop-phase2.md`'s two mentions are a dated measurement record and are left as history. ## Deploy verification `isDeploySurfaceFile` flags `src/hooks/pre-commit.ts`, `src/generated/interceptor-catalog.json` and the comment-only `crud-operations.ts`; the runtime behaviour change is confined to the local pre-commit hook. After merge: `mcp__minsky__deployment_wait-for-latest` for the affected services with `notBefore` = merge time, workflow-run correlation at the merge SHA, and a health read — SUCCESS plus runtime started, recorded on the task. ## Execution evidence Per success criterion: - **SC1** — `grep -rn runRulesCompileCheck src/hooks/` returns nothing (only ADR-016's historical Context and its new Phase-4 note mention the name repo-wide); pinned by the new test. - **SC2** — repointed, not removed: `bun test --preload ./tests/setup.ts src/hooks/compile-check.test.ts` → `39 pass / 0 fail` (37 original + 2 R1 pins). - **SC3** — one step: the new test asserts `this.instrumented("compile-check", …)` appears exactly once in `pre-commit.ts` and `rules-compile-check` not at all; the step's log line names all seven targets (`pre-commit.ts:2386`, unchanged). (The commit's own pre-commit run also wrote one `compile-check` fire-log record and zero `rules-compile-check` — a local observation, now superseded by the in-repo pin.) - **SC4** — the `kind="rules"` branch is removed (parameter deleted), with the reason in the classifier docblock. - **SC5** — `MINSKY_SKIP_SIZE_BUDGET` threaded on the surviving step's registration and consulted inside `runCompileCheck` — verified, not assumed. - **SC6** — `claude-agents` is in the surviving step's target list; `pre-commit.ts` records that mt#2497 reconciled the drift and subsumed mt#1654. Nothing carried forward. - **SC7** — `bun run test:hooks`: `7177 pass / 0 fail` (195 files, includes the interceptor census tests, re-run after R1); `bun scripts/run-related-tests.ts` over the changed sources: `424 pass / 0 fail` (24 files); `validate_typecheck` 0 errors across 8 projects (`infra/` skipped, not installed locally); `validate_lint` clean on the changed files; prettier unchanged. Acceptance tests: **AT1** as SC1. **AT2** — appended a line to `CLAUDE.md` in the session workspace: `compile --check --target claude.md` → exit 1, `[compile --check] Target "claude.md" is STALE`; restored → exit 0. **AT3** as SC3. **Negative control (test-first, mt#3244):** the repointed `compile-check.test.ts` run against `main`'s `pre-commit.ts` (classifier defaulting to the legacy prefix) → `25 pass / 12 fail`; all pass against this branch. ## R1 (review round 1) - **BLOCKING — hint spelling.** `bun run minsky …` does run here (package.json `minsky` script) but is repo-only; the compile CLI (`compile-commands.ts:233,266`) and pre-commit's setup hint (`:2998`) both say `minsky …`. Aligned; pinned in the staleness test (`Run "minsky compile --target agents.md"`, and `not.toContain("bun run minsky")`). - **Census counts** — the first push had removed the name outright; the census's "zero silent drops" test then failed on the `RETIRED_GUARD_NAMES` entry, and the authored-point manifest on the coordinates. Both are the append-only manifests the repo uses for exactly this, now updated; `test:hooks` 7177 pass. - **Remediation-text assertion** — added (above). - **Sweep beyond the updated files** — `grep -rn 'rules-compile-check\|runRulesCompileCheck' src packages docs .minsky scripts tests`: only ADR-016's historical Context lines (`:11`, `:21`, describing the pre-convergence state the ADR decided against) and `evaluation-loop-phase2.md`'s dated measurement; no operational guidance names the step. - **Runtime one-step check** — added as a source-text pin (the step roster is inline `this.instrumented(…)` calls, not data), plus the roster/retired-set assertion. - **Out-of-repo fire-log evidence** — superseded by the in-repo pin; the observation is kept in SC3 as what prompted it. ## Parallel-work note Open PR #3253 (mt#3854) also edits `src/hooks/pre-commit.ts`, in the `compileCheckTargets` region (+15 lines of its own, adding a `codex` presence field); this PR touches that region only in one docblock sentence. Its branch predates mt#4866/mt#4986/mt#5003 and needs a rebase regardless. PR #3412 (mt#4639) lists the same files but makes no change of its own to them (three-dot diff empty). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01UG8FZC1RHk7otPDDfs2zrr Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>

edobry added 2 commits August 22, 2026 01:27

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

71a04f4

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

minsky-ai Bot mentioned this pull request Aug 22, 2026

fix(mt#4435): Authenticate the reviewer's background schedulers as its own GitHub App #3254

Merged

minsky-ai Bot mentioned this pull request Aug 25, 2026

fix(mt#4483): Let a negating complement cancel the phrase it follows #3330

Merged

This was referenced Aug 26, 2026

fix(mt#4622): Exempt the markdown compile SOURCES from Prettier, not just their outputs #3381

Merged

fix(mt#4380): Credit a CLI spec read, so an MCP outage stops denying session_start #3393

Merged

This was referenced Aug 27, 2026

feat(mt#4639): Convert 614 log sites to getLoggableErrorSummary and flip the rule to error #3412

Open

fix(mt#4816): Re-root the last repo-rooted telemetry writer and retire the "repo" location #3528

Merged

minsky-ai Bot mentioned this pull request Sep 1, 2026

fix(mt#4755): Route the execution-evidence ladder through the shared helper [no-deploy-impact] #3541

Merged

minsky-ai Bot mentioned this pull request Sep 4, 2026

fix(mt#4885): Demote cwd-derived roots to the fallbackCwd tier in two calibration writers #3607

Merged

minsky-ai Bot mentioned this pull request Sep 4, 2026

fix(mt#4434): Rebuild the review diff from per-file patches when GitHub refuses it #3609

Merged

minsky-reviewer Bot suggested changes Sep 4, 2026

View reviewed changes

minsky-ai Bot mentioned this pull request Sep 4, 2026

feat(mt#4866): Repair the selection and init/compile defects behind a zero-rule fresh project #3623

Merged

This was referenced Sep 4, 2026

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

Open

fix(mt#4986): Stop overwriting a project's own CLAUDE.md, and say which file was left alone #3643

Merged

minsky-ai Bot mentioned this pull request Sep 5, 2026

feat(mt#5003): Deliver always-apply rules through the claude-rules target, not a new CLAUDE.md #3646

Merged

minsky-ai Bot mentioned this pull request Sep 11, 2026

fix(mt#5052): Guard depth-request suppression against negated prompts with one proximity check #3727

Merged

minsky-ai Bot mentioned this pull request Sep 11, 2026

chore(mt#2993): Delete the retired legacy rules-compile pre-commit check #3738

Merged

Navigation Menu

feat(mt#3854): Make .codex a compile output so the harness config stops fossilizing - #3253

feat(mt#3854): Make .codex a compile output so the harness config stops fossilizing#3253
minsky-ai[bot] wants to merge 2 commits into
mainfrom
task/mt-3854

minsky-ai Bot commented Aug 22, 2026 •

edited

Loading

Uh oh!

minsky-reviewer Bot commented Aug 22, 2026 •

edited

Loading

Uh oh!

edobry commented Aug 22, 2026

Uh oh!

minsky-reviewer Bot left a comment

Uh oh!

minsky-reviewer Bot left a comment

Uh oh!

minsky-reviewer Bot Sep 4, 2026

Uh oh!

minsky-reviewer Bot Sep 4, 2026

Uh oh!

minsky-reviewer Bot Sep 4, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

minsky-ai Bot commented Aug 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Key changes

What is deliberately NOT here

Testing

Deploy verification

Uh oh!

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

Uh oh!

Minsky Reviewer Status

Commands

Uh oh!

edobry commented Aug 22, 2026

Uh oh!

minsky-reviewer Bot left a comment

Choose a reason for hiding this comment

Findings

Spec verification

Documentation impact

Uh oh!

minsky-reviewer Bot left a comment

Choose a reason for hiding this comment

Findings

Inline comments

Adoption sweep

Documentation impact

Uh oh!

minsky-reviewer Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

minsky-reviewer Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

minsky-reviewer Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

minsky-ai Bot commented Aug 22, 2026 •

edited

Loading

minsky-reviewer Bot commented Aug 22, 2026 •

edited

Loading