Skip to content

feat(orchestrate): enforce the read boundary with a PreToolUse guard - #377

Merged
rodrigorjsf merged 1 commit into
orchestrate/umbrella-prd352-20260803-015333from
orchestrate/slice-361
Aug 3, 2026
Merged

rodrigorjsf merged 1 commit into
orchestrate/umbrella-prd352-20260803-015333from
orchestrate/slice-361

Conversation

@rodrigorjsf

Copy link
Copy Markdown
Owner

Slice #361 of PRD #352 — orchestrate run prd352-20260803-015333.

The orchestrator's rule against opening a slice's report or progress record was prose only, and prose guards erode late in a long run — exactly when the context saving matters most. This adds a plugin-level PreToolUse hook that denies the read and hands the model a reason naming what to do instead.

One acceptance criterion was refuted, and the honest reading is narrower

AC8 asked for a test over "a hook matcher that names an agent type". Only SubagentStart and SubagentStop match on agent type (claude-hook-reference-doc.md:229); this slice adds a PreToolUse, which matches on tool name. Inventing a SubagentStart matcher just to have something to test would have been an unrequested hook.

What shipped instead is a general invariant over hooks.json, tested in three layers. The load-bearing one is the bare-name trap: a matcher naming reviewer-deep must be a violation even though a definition carries that frontmatter name, because the shipped identifier is orchestrate:reviewer-deep. Without that layer the test is decorative — a matcher on the bare name silently never fires, and a guard that silently never fires is worse than no guard, because it is trusted.

Design decisions that keep the guard from being silently inert, or silently harmful

  • matcher: "Read|Bash", never ".*". This is what makes AC9 true by tool-name disjointness: recover_slice_progress never reaches the handler, so structured recovery is unaffected by construction rather than by a special case.
  • No async key. The watchdog's PostToolUse block sets async: true because it only observes. An async PreToolUse cannot block — copying that key would leave AC2 unmet with the whole suite green. Asserted against the real hooks.json, not just commented.
  • The path rule is narrower than the run directory. Denying everything under runs/<id>/ would catch run-state.json, the orchestrator's own checkpoint, breaking every run silently and only in the field. Deny requires dirname equality against the resolved run directory and a slice-<n>-(progress.json|report.md) basename. dirname rather than a prefix test, because runs/<id> is a string prefix of runs/<id>-suffix — and it neutralises .. traversal for free.
  • agent_id, not agent_type. agent_type is also present when the session runs with --agent, so it can be non-empty on a main thread. agent_id is documented as present only inside a subagent call (:644 vs :645).

Honest limitations, stated in the module rather than implied away

PreToolUse does not fire for files referenced with @ in a prompt — their contents are inserted while building the prompt, with no tool call, including for hooks matching Read (:1400). The documented closure is a Read deny rule, which is precisely the alternative ADR-0017:113-115 rejected: permission rules apply session-wide and would restrict the executor too. That hole is larger than any Bash-heuristic gap and is unclosable in this design — which is a fourth reason, beyond organizational policy, that the prose rule stays load-bearing.

Review confirmed three further holes the first draft's comment did not admit to, and pinned two with tests: the subcommand splitter is quote-blind, so grep -E "A|B" <path> splits on the pattern's pipe and is allowed, while a --body containing ; cat <path> is denied though nothing is read. The splitter was deliberately left alone — a quote-aware parser buys little coverage and adds false-deny risk, and AC7 makes over-blocking the expensive direction.

Verification

Red-green in three stages, not one: a permissive stub turned all 29 deny cases red; a deliberately over-broad implementation turned all 27 allow cases red together, including run-state.json and mere-mention; only the narrow rules close at 1 failure. A one-sided red proof would have accepted a guard that blocks everything.

Both implementer and reviewer independently piped real hook events into the committed dist/read-guard.js — every unit test exercises the pure module, but AC2 grades the hook. Reviewer also rebuilt the bundle and diffed it against the committed one: byte-identical.

Orchestrator pre-merge gate: build passed; tests 807/808 with the single known RE-ENTRANCY baseline failure; read-guard.test.ts 61 green, hook-matcher-consistency.test.ts 12 green; dist/ adds only read-guard.js.

Known, deferred

plugins/orchestrate/README.md is 470 lines against a 400-line cap in .claude/rules/readme-files.md. It was already 463 at HEAD — a pre-existing violation this slice worsens by 7 lines. Fixing it means deleting content other slices added; carried to #363, which owns the README.

Closes #361

@rodrigorjsf
rodrigorjsf merged commit 17dd985 into orchestrate/umbrella-prd352-20260803-015333 Aug 3, 2026
1 check passed
@rodrigorjsf
rodrigorjsf deleted the orchestrate/slice-361 branch August 3, 2026 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant