fix(mt#4380): Credit a CLI spec read, so an MCP outage stops denying session_start - #3393
Merged
Merged
Conversation
…session_start [no-deploy-impact]
The bind/advance spec-read guard enumerated MCP tool names only, so a session that
read and amended a task's spec through `minsky tasks spec get` / `minsky tasks edit
--spec-file` — the documented fallback when the MCP daemon is down — looked identical
to one that never opened the task, and was DENIED at session_start.
Widen the EVIDENCE, not the matcher. The guard already fires correctly on the guarded
MCP action; only its discharge evidence was blind. mt#4536's prescription for the
REGISTRATION-blind guards (add `Bash` to the matcher) would be wrong here and costly:
this module reads the transcript to decide, so it would pay a full transcript parse on
every bash call. Nothing in .claude/settings.json changed.
Commands resolve through the completion-manifest `commandId` stamp (mt#4144), the same
oracle cli-mcp-substitution uses — never a hand-rolled pattern. 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.
- detect-cli-mcp-substitution: extract `resolveCommandNode` (deepest node + the argv
tail after the last SUBCOMMAND) and export `optionTakesValue` / `firstPositional` /
`hasAnyFlag`. `resolveCommandId` delegates, behaviour unchanged. Slicing at the last
subcommand rather than at the walk's stopping point is load-bearing: the walk
consumes a token after an unrecognised flag, so `tasks spec get --json mt#4380`
would otherwise swallow the id.
- check-task-spec-read: credit `tasks spec get`, `tasks get --include-spec` as reads
and `tasks edit --spec{,-file,-content}` as authorship, on both Bash and
session_exec. Checked only after the MCP predicates miss, so the common path never
touches disk. An unlisted option is assumed to take a value — over-consuming can
only MISS a positional, while under-consuming would read a flag's value as a task id.
- The widening reaches duplicate-check-candidate-read by construction (it COMPOSES
specWasSurfaced); that propagation is deliberate and now pinned by tests.
- Canary gains a CLI-read control, so it can no longer be satisfied by a guard that
denies unconditionally.
Not credited, deliberately: CLI `tasks create --spec-file`, whose MCP credit
correlates the minted id out of the tool RESULT. Named as a residual in the spec.
Filed mt#4653: hook-module-inventory's tier-1 derivation uses a bare substring match,
so naming ensureHookDomainBootstrap in a COMMENT files a module under ADR-026 tier 1.
Worked around here with a comment; fixing the derivation has migration-wave blast
radius that does not belong in this PR.
Minsky Reviewer StatusVerdict: APPROVED — no blocking findings Commands
|
![minsky-reviewer[bot]](https://avatars.githubusercontent.com/u/52695?s=60&v=4){kind=small}
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Solid, well-reasoned change: widens the guard’s evidence via the generated CLI manifest, preserves the hot MCP path by deferring the CLI scan, and adds thorough tests and docs. I focused on maintenance and safety edges introduced by the CLI channel. Non-blocking nits: (1) hardcoded CLI spec-edit flags risk drift from the CLI source-of-truth; (2) hardcoded --include-spec requirement duplicates knowledge already available in the manifest/CLI layer; (3) memoizing a null manifest for the process lifetime can mask transient read failures. None are blockers; tests are extensive and documentation was updated. Approve to merge.
Findings
- [NON-BLOCKING] .minsky/hooks/check-task-spec-read.ts:268 — Hand-maintained list of spec-edit flags risks drifting from CLI source of truth
CLI_SPEC_WRITE_FLAGShardcodes["--spec", "--spec-file", "--spec-content"](see.minsky/hooks/check-task-spec-read.ts:268-271). While these do mirror the currenthasSpecOperationset inedit-commands.ts, this is now a second source of truth. If another spec-writing flag is added to the CLI (e.g., an alias) and the manifest includes it,hasAnyFlaghere will not recognize it without updating this list. Consider centralizing this set in a shared constant in the CLI command-definition layer and importing it, or deriving it from the manifest node fortasks.edit(e.g., by matching flags whose names containspecand are whitelisted), to avoid future drift. Tests will catch regressions, but the duplication raises maintenance risk. - [NON-BLOCKING] .minsky/hooks/check-task-spec-read.ts:248 — Hardcoded
--include-specrequirement duplicates CLI source-of-truth
CLI_SPEC_READ_COMMANDShardcodes thattasks.getrequires--include-specto count as a spec read (.minsky/hooks/check-task-spec-read.ts:248-256). While correct today, this duplicates knowledge that already exists in the CLI command definition and in the generated manifest (thetasks.getleaf’soptionslist). If the flag name or behavior changes (alias added, short form introduced), this table must be updated manually. Consider deriving the gating condition from the manifest node fortasks.get(e.g., consult itsoptionsto accept any flag that maps to the include-spec behavior) or centralizing the requirement in a shared exported constant from the CLI layer to avoid drift. - [NON-BLOCKING] .minsky/hooks/check-task-spec-read.ts:276 — Null manifest cached for process lifetime — transient read errors won't recover within process
manifestOnce()memoizes the result ofreadManifest()includingnulland never retries for the life of the process (.minsky/hooks/check-task-spec-read.ts:276-283). If the manifest read fails transiently (e.g., races with generation), subsequent evaluations in the same process will skip the CLI channel permanently. Consider caching only successful reads and re-attempting on a null with backoff, or adding a fast TTL on the null case. The current design is safe-direction (under-credits only) but can mask availability restoration until process restart.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
A session that read a task's spec via the CLI (minsky tasks spec get <id>) is treated as having read it, and session_start for that task is not denied. |
Met | Code: .minsky/hooks/check-task-spec-read.ts:241-259 calls cliEngagementInTranscript(..., "read") after MCP predicates; CLI parser in cliSpecEngagements detects tasks.spec.get. Tests: .minsky/hooks/check-task-spec-read.test.ts:1194-1210 — AT1 asserts CLI spec read discharges specWasSurfaced for the target across multiple CLI invocations. |
A session that AMENDED the spec via minsky tasks edit --spec-file / --spec-content is likewise treated as having authored it. |
Met | Code: .minsky/hooks/check-task-spec-read.ts:329-355 and :261-311 — CLI_SPEC_WRITE_FLAGS with hasAnyFlag, and specWasAuthored calls cliEngagementInTranscript(..., "authored"). Tests: .minsky/hooks/check-task-spec-read.test.ts:1262-1274 — AT2 asserts CLI tasks edit with spec flags counts as authored and not as read. |
| The check still denies on the case it exists for: a session with no spec read or authorship through ANY channel. A test covers that this is not weakened into a pass-through. | Met | Tests: .minsky/hooks/check-task-spec-read.test.ts:1290-1301 — AT3 builds a transcript with no spec engagement and asserts both specWasSurfaced and specWasAuthored are false. The code change short-circuits the CLI scan behind MCP checks, preserving deny behavior when none are present. |
The mechanism is stated in docs/architecture/hooks/ alongside the guard, naming which channels count as evidence and why. |
Met | Docs updated in this PR: docs/architecture/hooks/bind-advance-spec-read-guard.md:160-249 adds “The CLI evidence channel (mt#4380)” with explicit table and rationale covering MCP and CLI spellings and negatives. |
| A CLI spec read naming a DIFFERENT task does not discharge the check for the target task — the id is compared, not merely the command. A test covers this. | Met | Code normalizes and compares task ids (normalizeTaskId + firstPositional): .minsky/hooks/check-task-spec-read.ts:300-311 and :213-239. Tests: .minsky/hooks/check-task-spec-read.test.ts:1314-1324 and :1326-1334 — AT5 pair asserts different-task CLI read/edit does not discharge the target while discharging the named task. |
duplicate-check-candidate-read.ts, which imports specWasSurfaced, is exercised against a CLI-read candidate and its changed behavior is stated deliberately rather than inherited. |
Met | Test additions: .minsky/hooks/duplicate-check-candidate-read.test.ts:166-207 add a suite “the CLI read channel reaches this detector too (mt#4380)” asserting a CLI tasks spec get cleans the candidate and a bare tasks get does not; demonstrates propagation via specWasSurfaced composition. |
Does NOT cover — Crediting a CLI minsky tasks create --spec-file as authorship (deliberately excluded). |
Met | Docs explicitly exclude it: docs/architecture/hooks/bind-advance-spec-read-guard.md:208-219. Code: specWasAuthored does not attempt to credit CLI tasks create and only credits authorship via MCP tasks_create result correlation or CLI tasks edit spec flags (.minsky/hooks/check-task-spec-read.ts:311-357). |
Documentation impact
- updated-in-pr — This PR adds and updates documentation to describe the new CLI evidence channel and its rationale. See
docs/architecture/hooks/bind-advance-spec-read-guard.md:160-249which details which CLI/MCP channels count, negative cases, resolver mechanism, and propagation toduplicate-check-candidate-read. No other docs appear to contradict the new behavior.
Affected: docs/architecture/hooks/bind-advance-spec-read-guard.md
…est [no-deploy-impact] Addresses the two non-blocking drift findings. The command IDs already resolve through the generated manifest, but which flags MEAN "spec read" / "spec authorship" is a policy set this module holds by hand, and the manifest cannot supply that semantic — it lists a command's options without marking any of them spec-writing. What IS derivable is that each flag we name still EXISTS on its command, and that is now asserted. The finding matters because the drift would be a silent regression of this very fix: a renamed `--include-spec` would simply stop crediting that channel with every other test still green. Now it fails a test that names the flag. Negative control: renaming the asserted flag to `--include-spec-RENAMED` turns the new test red (1 fail / 111 pass); restored and re-verified at 133 pass / 0 fail across both affected files. Finding 3 (a null manifest cached for the process lifetime) is left as-is and documented at the cache: a guard invocation is a short-lived process, so re-reading a file that just failed within it buys almost nothing, while the cache keeps the would-deny path from re-reading once per candidate transcript. The degradation is to exactly the pre-change MCP-only evidence, never to a crash and never to a blanket pass.
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Re-verified against the current spec and diff. The prior round had only non-blocking notes; the follow-up commit adds test coverage that pins the CLI flag names against the generated manifest, addressing drift risk without changing production behavior. The only file changed since R1 is the test suite, which now asserts that --include-spec and the three tasks edit spec flags exist on their respective commands via resolveCommandNode. Production wiring remains as reviewed: specWasSurfaced/specWasAuthored consult the CLI channel only after MCP predicates miss; the docs were updated to describe the mechanism. I found no new critical defects introduced by this fix, and all success criteria are met. Verdict: APPROVE.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
A session that read a task's spec via the CLI (minsky tasks spec get <id>) is treated as having read it, and session_start for that task is not denied. |
Met | .minsky/hooks/check-task-spec-read.test.ts — test "AT1 — a transcript whose only engagement is a CLI spec read counts as read" asserts specWasSurfaced([...Bash 'minsky tasks spec get <id>'], TARGET) === true. Backed by production path in .minsky/hooks/check-task-spec-read.ts where specWasSurfaced delegates to cliEngagementInTranscript(..., 'read') after MCP predicates miss. |
A session that AMENDED the spec via minsky tasks edit --spec-file / --spec-content is likewise treated as having authored it. |
Met | .minsky/hooks/check-task-spec-read.test.ts — test "AT2 — a transcript whose only engagement is a CLI spec edit counts as authored" asserts specWasAuthored([...Bash 'minsky tasks edit <id> --spec-file ...'], TARGET) === true and specWasSurfaced(...) === false. Implementation wired in .minsky/hooks/check-task-spec-read.ts via cliEngagementInTranscript(..., 'authored') and CLI_SPEC_WRITE_FLAGS. |
| The check still denies on the case it exists for: a session with no spec read or authorship through ANY channel. A test covers that this is not weakened into a pass-through. | Met | .minsky/hooks/check-task-spec-read.test.ts — test "AT3 — a transcript with no spec engagement through ANY channel still fails both" asserts specWasSurfaced(...) === false and specWasAuthored(...) === false. Guard’s denial path remains unchanged in .minsky/hooks/check-task-spec-read.ts (no MCP/CLI evidence => deny). |
The mechanism is stated in docs/architecture/hooks/ alongside the guard, naming which channels count as evidence and why. |
Met | docs/architecture/hooks/bind-advance-spec-read-guard.md — section "The CLI evidence channel (mt#4380)" documents accepted CLI read/authorship channels, manifest-based resolution, and why matcher is unchanged. |
| A CLI spec read naming a DIFFERENT task does not discharge the check for the target task — the id is compared, not merely the command. A test covers this. | Met | .minsky/hooks/check-task-spec-read.test.ts — tests “AT5 — a CLI spec read of a DIFFERENT task does not discharge the target” and its EDIT sibling assert false for TARGET and true for OTHER. Parsing uses normalizeTaskId(firstPositional(...)) ensuring id comparison in .minsky/hooks/check-task-spec-read.ts. |
duplicate-check-candidate-read.ts, which imports specWasSurfaced, is exercised against a CLI-read candidate and its changed behavior is stated deliberately rather than inherited. |
Met | .minsky/hooks/duplicate-check-candidate-read.test.ts — suite “run — the CLI read channel reaches this detector too (mt#4380)” asserts that a Bash minsky tasks spec get <candidate> switches outcome to clean, while a bare tasks get does not. This composes specWasSurfaced by construction. |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| detect-cli-mcp-substitution.resolveCommandNode | function | .minsky/hooks/check-task-spec-read.ts: import resolveCommandNode for CLI parsing, ./.minsky/hooks/check-task-spec-read.test.ts:1149 — tests import resolveCommandNode to assert flags exist | Adopted | Exported helper refactored from existing code; now reused by the guard and tests. |
Documentation impact
- updated-in-pr — The guard’s documentation was amended to describe the new CLI evidence channel, accepted spellings, and manifest-based resolution. See docs/architecture/hooks/bind-advance-spec-read-guard.md — section “The CLI evidence channel (mt#4380)” and “Which channels count as evidence, and why each.”
Affected: docs/architecture/hooks/bind-advance-spec-read-guard.md
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Re-verified this iteration against the current spec and diff. The fix widens the evidence channel by parsing CLI invocations via the generated completion manifest and adds a safe argv walk (resolveCommandNode) plus helpers, then consults this CLI path only after MCP predicates miss. Tests comprehensively pin the behavior — correct credits, wrong-task negative controls, flag presence assertions against the live manifest, propagation to duplicate-check-candidate-read, and a strengthened canary. Docs were updated with the “CLI evidence channel” section. I find no unresolved prior blocking issues and no new critical defects introduced by the fix. Verdict: APPROVE.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
A session that read a task's spec via the CLI (minsky tasks spec get <id>) is treated as having read it, and session_start for that task is not denied. |
Met | Tests added in .minsky/hooks/check-task-spec-read.test.ts under “specWasSurfaced / specWasAuthored — CLI channel (mt#4380)” — case “AT1 — a transcript whose only engagement is a CLI spec read counts as read” constructs a Bash tool_use with minsky tasks spec get mt#4311 and asserts specWasSurfaced(..., TARGET) === true. |
A session that AMENDED the spec via minsky tasks edit --spec-file / --spec-content is likewise treated as having authored it. |
Met | .minsky/hooks/check-task-spec-read.test.ts — test “AT2 — a transcript whose only engagement is a CLI spec edit counts as authored” asserts specWasAuthored([bashCommand("minsky tasks edit mt#4311 --spec-file /tmp/s.md")], TARGET) === true and specWasSurfaced(...) === false. |
| The check still denies on the case it exists for: a session with no spec read or authorship through ANY channel. A test covers that this is not weakened into a pass-through. | Met | .minsky/hooks/check-task-spec-read.test.ts — test “AT3 — a transcript with no spec engagement through ANY channel still fails both” asserts both specWasSurfaced and specWasAuthored return false for a transcript with unrelated tool calls. |
The mechanism is stated in docs/architecture/hooks/ alongside the guard, naming which channels count as evidence and why. |
Met | docs/architecture/hooks/bind-advance-spec-read-guard.md — new section “The CLI evidence channel (mt#4380)” documents MCP and CLI spellings that count, negatives that do not, and the use of the generated manifest to resolve commandId and parse flags/positionals. |
| A CLI spec read naming a DIFFERENT task does not discharge the check for the target task — the id is compared, not merely the command. A test covers this. | Met | .minsky/hooks/check-task-spec-read.test.ts — test “AT5 — a CLI spec read of a DIFFERENT task does not discharge the target” asserts specWasSurfaced([bashCommand("minsky tasks spec get mt#9999")], TARGET) === false and ... === true for the OTHER id. A sibling test covers the EDIT case as well. |
duplicate-check-candidate-read.ts, which imports specWasSurfaced, is exercised against a CLI-read candidate and its changed behavior is stated deliberately rather than inherited. |
Met | .minsky/hooks/duplicate-check-candidate-read.test.ts — new describe “run — the CLI read channel reaches this detector too (mt#4380)” adds tests asserting a CLI tasks spec get discharges the candidate, while a bare tasks get does not. This pins the deliberate propagation of the widened predicate. |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| detect-cli-mcp-substitution.resolveCommandNode | function | .minsky/hooks/check-task-spec-read.ts: import resolveCommandNode and use in cliSpecEngagements(), /.claude/hooks/check-task-spec-read.ts: same import and usage, /.minsky/hooks/check-task-spec-read.test.ts: imports resolveCommandNode for flag-presence assertions | Adopted | New helper extracted to return deepest node plus argv tail; immediately consumed by the widened evidence channel and its tests. |
| check-task-spec-read.cliSpecEngagements | function | .minsky/hooks/check-task-spec-read.test.ts: imports cliSpecEngagements for parser/evidence tests, /.claude/hooks/check-task-spec-read.ts: exported alongside main predicates (generated copy) | Adopted | Exported primarily for testing the pure parser; production code uses it via the private wrapper cliEngagementInTranscript. |
Documentation impact
- updated-in-pr — The change adds a new documented evidence channel for the guard.
docs/architecture/hooks/bind-advance-spec-read-guard.mdwas updated with a dedicated section (“The CLI evidence channel (mt#4380)”) describing which MCP and CLI spellings count, negatives that do not, and why the generated manifest is used. This matches the implementation changes.
Affected: docs/architecture/hooks/bind-advance-spec-read-guard.md
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Re-verified this iteration against the current spec and diff. The fix widens the evidence channel by parsing CLI invocations via the generated completion manifest, adds a safe argv walk (resolveCommandNode) and helpers, and consults this CLI path only after MCP predicates miss. Tests comprehensively pin behavior (CLI reads/writes, wrong-task negatives, flag existence against the live manifest, propagation to duplicate-check-candidate-read, and a strengthened canary). Docs were updated with the “CLI evidence channel” section. I found no unresolved prior blocking findings (none existed) and no new critical defects introduced by this change. Verdict: APPROVE.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
A session that read a task's spec via the CLI (minsky tasks spec get <id>) is treated as having read it, and session_start for that task is not denied. |
Met | Tests added exercising the CLI channel: .minsky/hooks/check-task-spec-read.test.ts:~1210-1253 — case “AT1 — a transcript whose only engagement is a CLI spec read counts as read” asserts specWasSurfaced returns true for minsky … tasks spec get <id> and variants. |
A session that AMENDED the spec via minsky tasks edit --spec-file / --spec-content is likewise treated as having authored it. |
Met | .minsky/hooks/check-task-spec-read.test.ts:~1255-1270 — case “AT2 — a transcript whose only engagement is a CLI spec edit counts as authored” asserts specWasAuthored([... 'minsky tasks edit … --spec-file …']) === true and specWasSurfaced(...) === false. |
| The check still denies on the case it exists for: a session with no spec read or authorship through ANY channel. A test covers that this is not weakened into a pass-through. | Met | .minsky/hooks/check-task-spec-read.test.ts:~1290-1305 — “AT3 — a transcript with no spec engagement through ANY channel still fails both” asserts both predicates are false for a transcript with unrelated tool use and bash calls. |
The mechanism is stated in docs/architecture/hooks/ alongside the guard, naming which channels count as evidence and why. |
Met | docs/architecture/hooks/bind-advance-spec-read-guard.md:~160-249 — new section “The CLI evidence channel (mt#4380)” explicitly lists MCP/CLI channels credited, parsing approach, and why. |
| A CLI spec read naming a DIFFERENT task does not discharge the check for the target task — the id is compared, not merely the command. A test covers this. | Met | .minsky/hooks/check-task-spec-read.test.ts:~1307-1325 — “AT5 — a CLI spec read of a DIFFERENT task does not discharge the target” asserts false for target, true for the other; a sibling test covers EDIT similarly. |
duplicate-check-candidate-read.ts, which imports specWasSurfaced, is exercised against a CLI-read candidate and its changed behavior is stated deliberately rather than inherited. |
Met | .minsky/hooks/duplicate-check-candidate-read.test.ts:~170-213 — new describe block “the CLI read channel reaches this detector too (mt#4380)” asserts a CLI tasks spec get discharges the candidate and a bare tasks get does not. |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| cliSpecEngagements | function | .minsky/hooks/check-task-spec-read.test.ts:1149 — imports cliSpecEngagements for direct unit tests of CLI parsing |
Adopted | New export from .minsky/hooks/check-task-spec-read.ts intended for pure-function testing; no production wiring required by the spec. |
| resolveCommandNode | function | .minsky/hooks/check-task-spec-read.ts:~118 — imports resolveCommandNode, /.minsky/hooks/check-task-spec-read.test.ts:~1150 — imports resolveCommandNode to assert flag presence against the manifest |
Adopted | New helper exported from detect-cli-mcp-substitution.ts and used both in production guard logic and tests. |
| firstPositional | function | .minsky/hooks/check-task-spec-read.ts:~118 — imports firstPositional to extract taskId from argv tail, /.claude/hooks/check-task-spec-read.ts:~121 — same import in generated copy, /.minsky/hooks/check-task-spec-read.test.ts:~1150 — indirectly exercised via cliSpecEngagements tests |
Adopted | Exported from detect-cli-mcp-substitution.ts and used by the widened evidence path. |
| hasAnyFlag | function | .minsky/hooks/check-task-spec-read.ts:~118 — imports hasAnyFlag for CLI evidence parsing, /.claude/hooks/check-task-spec-read.ts:~121 — same import in generated copy |
Adopted | Exported from detect-cli-mcp-substitution.ts; used by the widened evidence path. |
Documentation impact
- updated-in-pr — This PR changes the guard’s behavior by crediting CLI-based spec reads/edits and updates the architecture doc accordingly. The new section “The CLI evidence channel (mt#4380)” in
docs/architecture/hooks/bind-advance-spec-read-guard.md:160-249documents which CLI/MCP channels count and the parsing mechanism.
Affected: docs/architecture/hooks/bind-advance-spec-read-guard.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The bind/advance spec-read guard denied
session_startwith the task-hijack message ("this sessionhas 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 getandminsky tasks edit --spec-file— thedocumented 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 thematcher to
Bash|mcp__minsky__session_exec) is scoped to the REGISTRATION-blind guards, which neverrun 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.jsonchanged, and the two-binding-site requirement is thereforeinapplicable. 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
commandIdstamp insrc/generated/completion-manifest.json(mt#4144), the same oraclecli-mcp-substitutionuses. The manifest also carries each leaf's positionalargumentsandper-option
takesValue— which is what lets the guard recover the target task id rather thanmerely 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— extractedresolveCommandNode(deepest node plus theargv tail after the last subcommand) and exported
optionTakesValue/firstPositional/hasAnyFlag.resolveCommandIdnow delegates; behaviour unchanged, asserted by its existingtests. 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#4380would otherwise silently swallow the id on every flagged invocation.
check-task-spec-read.ts— creditstasks spec getandtasks get --include-specas reads,and
tasks edit --spec/--spec-file/--spec-contentas authorship, across bothBashandmcp__minsky__session_exec. Checked only after the MCP predicates miss, so the common pathnever touches the manifest or the disk. The CLI rows mirror their MCP counterparts including the
negatives: a bare
tasks getis not a read, a--kind-only edit is not authorship.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.tsgets the widening by construction — it COMPOSESspecWasSurfacedrather than duplicating it, so before this change a candidate whose spec wasread 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
## Scopeand wasadded at planning as a gate (h) failure.
unconditionally.
Not credited, deliberately: a CLI
minsky tasks create --spec-file. The MCP credit fortasks_createcorrelates the minted id out of the tool RESULT; recovering it from stdout text is adifferent 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.tsover the four changed source files — 2967 pass, 0 fail across61 files. Typecheck: 0 errors across 8 projects. Lint: 0 errors, 0 warnings, 4099 files.
Execution evidence:
Per-AT coverage, using mt#4380's own numbering:
AT1 — a transcript whose only engagement is a CLI spec read counts as read, coveringminsky …,bun run src/cli.ts …, and a path-qualified binary. PASS.AT2 — a transcript whose only engagement is a CLI spec edit counts as authored, whichalso asserts it is authorship and NOT a read, the same split the MCP predicates draw. PASS.
AT3 — a transcript with no spec engagement through ANY channel still fails both. PASS.19 added are all new
describeblocks.resolveCommandId's own tests also pass, confirming therefactor preserved its behaviour. PASS.
AT5 — a CLI spec read of a DIFFERENT task does not discharge the targetand its…EDIT of a different task…sibling. Each asserts the inverse in the same test (the other taskIS discharged for itself), so the
falsecannot be satisfied by a channel that credits nothing.PASS.
tasks get counts only with --include-spec,all three tasks-edit spec flags are authorship, in both spellings, anda metadata-only tasks edit is NOT authorship. PASS.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 channelsection added todocs/architecture/hooks/bind-advance-spec-read-guard.md, SC5 → AT5, SC6 → AT7. No criterion namesa 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:
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):All seven failures are mt#4380 assertions. Two things this control establishes that the count alone
does not: the
cliSpecEngagementspure-function tests still PASS, isolating the failure to thewiring 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.isDeploySurfaceFilereturnsfalsefor 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):Notes for review
Filed mt#4653 rather than fixing it here.
hook-module-inventory.test.tsderives the ADR-026tier-1 set with a bare
readFileSync(...).includes("ensureHookDomainBootstrap"), which does notdistinguish a call from a mention. A comment in this PR explaining why the guard avoids the domain
bootstrap flipped the guard into
liveTier1and failed the census, inviting the reconciliation "addthe 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 isconverting
.codexinto 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.