Repository navigation
spec(hooks): disambiguate Not decision-eligible and align subagentStop as Audit-only - #341
Conversation
|
This pull request's description says it closes #146, which is on the project roadmap. When it merges, GitHub closes that issue and the roadmap counts it as delivered. Please confirm the change delivers the whole issue before merging. |
ccd6e15 to
9fbd829
Compare
rocklambros
left a comment
There was a problem hiding this comment.
Raghuvar, "Not decision-eligible" is now gone from hooks.md and from every hook schema, and the postCompact line says what post-compact.json says. Your schema edit changes only description text, so the wire shape stays the same (I compared the file). The merged tree on integration 19b1197 passes every suite.
@bar-capsule @afogel, this PR needs your answer on issue #146 first, and I won't repeat that question here. The PR also adds something that question doesn't cover, so I have one more.
hooks.md:258 says redaction of subagent output "MUST be enforced". Is that meant to be a new MUST? I'm asking for three reasons.
- Every other redaction line in
hooks.mdis a MAY (lines 106, 132, 156, 232), and MODIFY is only SHOULD-support inconformance.md:98. The sentence also doesn't say who must do it (the Guardian, the client or the deployment). - In PR #22, the adapters can't do this at the hooks the sentence names. Claude Code treats
agentResponseandtoolCallResultas observation-only (adapters/claude-code/mapping.md:45). Cursor does the same foragentResponse(adapters/cursor/mapping.md:74), and it can rewrite tool output for MCP tools only (adapters/cursor/mapping.md:55). A subagent that ends asfailed,cancelledortimeoutcan also still carry asummary(the schema allows it), and itsagentResponsemay never have fired. - The other path, "when the parent subsequently acts", names no hook. By then the text is already in the parent's context, and a MODIFY on a later step changes only that step.
Issue #146 asks for the consequence to be recorded, and I don't think that takes a MUST. If you agree, this text would record it. It's your decision.
`subagentStop` gives the parent no gate on the returned content. A deployment that redacts subagent output does it at the subagent's own `toolCallResult` or `agentResponse`, before the output reaches the parent.
Once you've answered both, Raghuvar can push, and one of you can approve and merge. That merge closes issue #146.
Raghuvar, here is what's waiting and what isn't.
These wait on other people, so there's nothing for you to do on them:
- Bar's or Ariel's answer on issue #146, and their answer on the MUST above.
- PR #338 and PR #340. All three PRs add a test at the end of
tests/test_hook_taxonomy.py, so this PR lands after those two.
These don't wait:
- Removing the phrase and your
postCompactandturnEndlabels are right under either answer on issue #146. Only thesubagentStoptext depends on it. - Asks 1 to 3 below are yours, and you can make them now. Please push only once, after both items above are done, so you only rebase once.
If the answer is Audit-only, asks 1 to 3 don't change. If it's the postCompact rule, the subagentStop parts of asks 1 to 3 change, and this PR also needs the §6.5 change that issue #146 describes.
-
docs/topics/faq.md:46still listspostCompactas audit-only, but yourhooks.mdline says MODIFY is allowed there. The two pages disagree, which is the same problem as issue #146. The FAQ landed the day after you opened this PR, so you couldn't have seen it. Replace the sentence that starts "The other five" with this:The other five fire after the fact. At `postCompact` the Guardian can still rewrite the summary with `modify`. The other four are audit-only records: `subagentStop`, `skillUnload`, `turnEnd`, and `sessionEnd`. -
hooks.mdnow says "ALLOW only" forsubagentStopandturnEnd, but those two schemas rule out onlydenyandmodify. §6 has five dispositions, so the schemas leaveaskanddeferopen. Please change both:subagent-stop.json:5, replacereturns 'allow' (cannot return 'deny' or 'modify').withreturns 'allow'. No other disposition is legal.turn-end.json:5, replaceso the Guardian writes a ContextEntry but cannot return 'deny' or 'modify'.withso the Guardian writes a ContextEntry and returns 'allow'. No other disposition is legal.
Please leave
skillUnloadandsessionEndas they are. Issue #148 covers those. -
Your test at
tests/test_hook_taxonomy.py:330never comparessubagent-stop.jsonwithhooks.md, although its name says it does. I broke your text 12 ways, and it caught 5. It passed when I changedsubagentStopinhooks.mdto "ALLOW / MODIFY", when I changed the schema to "MAY return 'modify'", and when I put "not decision-eligible" back in lower case. Replace it with this test, which also checks asks 1 and 2. It failed on all 8 ways I broke the new text. End the file with one newline.def decision_line(hook): page = HOOKS_PAGE.read_text(encoding="utf-8") section = page.split(f"\n## {hook}\n")[1].split("\n---")[0] return next(line for line in section.splitlines() if line.startswith("**Decision:**")) def test_post_event_hooks_carry_one_label_on_every_surface(): """Issue #146: each rule has one label, and every surface says the same thing.""" hooks_text = HOOKS_PAGE.read_text(encoding="utf-8") schema_texts = [path.read_text(encoding="utf-8") for path in HOOK_SCHEMAS.glob("*.json")] for text in [hooks_text, *schema_texts]: assert "not decision-eligible" not in text.lower() assert "| ALLOW / MODIFY (no DENY" in hooks_text assert decision_line("postCompact").startswith("**Decision:** ALLOW / MODIFY (DENY not permitted)") for hook, schema in (("subagentStop", "subagent-stop.json"), ("turnEnd", "turn-end.json")): assert decision_line(hook).startswith("**Decision:** Audit only (ALLOW only)") description = json.loads((HOOK_SCHEMAS / schema).read_text(encoding="utf-8"))["description"] assert "Audit-only" in description assert "No other disposition is legal" in description faq = (DOCS / "topics" / "faq.md").read_text(encoding="utf-8") assert "`postCompact`" not in faq.split("audit-only records:")[1].split(".")[0]
-
Please rebase after PR #338 and PR #340 land. Git will show a conflict in
tests/test_hook_taxonomy.py. The conflict is only about position, so keep every test. I tried this on a scratch copy with all three PRs: 927 tests collected, 926 passed, 1 skipped (the usual one). Asks 1 to 3 pass there too.Test:
uv run pytest tests/test_hook_taxonomy.pypasses, and the PR #338, PR #340 and PR #341 tests are all in the file.
Issue #146 has no assignee yet, because GitHub still won't let me assign you. It's the same step as on your other PRs, so please leave a comment on the issue. After you push, I'll run all of this again.
…p as Audit-only Stop overloading 'Not decision-eligible' for both MODIFY-eligible (postCompact) and audit-only (subagentStop, turnEnd) hooks in hooks.md, docs/topics/faq.md, hooks/subagent-stop.json, and hooks/turn-end.json; label postCompact explicitly as ALLOW / MODIFY (DENY not permitted); and state 'No other disposition is legal' on subagent-stop.json and turn-end.json. Fixes GenAI-Security-Project#146 Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
|
@rocklambros Rebased onto |
9fbd829 to
b3f6e95
Compare
What changed
Removes the overloaded
"Not decision-eligible"phrase fromdocs/spec/instrument/hooks.mdandspecification/v0.1.0/hooks/subagent-stop.json, labelspostCompactexplicitly asALLOW / MODIFY (DENY not permitted), alignssubagent-stop.jsonwithhooks.mdandconformance.mdasAudit-only (ALLOW only)while documenting where subagent output redaction attaches, and adds a guard test intests/test_hook_taxonomy.py.Which issue does this implement
Fixes #146
meaning, so it needs no issue
Base branch
integration, because this touches the specification, schemas, a referenceimplementation, an adapter, tests, or CI
main, because every changed path is on the documentation lane allowlist inCONTRIBUTING.md
Type of change
Specification changes
Breaking for implementers? No wire shape changes. Resolves the terminology conflict between
subagent-stop.json,postCompact,turnEnd, andhooks.md.I tested this
uv run pytest -vpasses on my machineuv run mkdocs build --strictpasses on my machineChecklist
git commit -s(required by the DCO)Security
Documents explicitly on
subagentStopthat because it is audit-only on the parent chain, output redaction for subagents must be enforced inside the subagent's own session (toolCallResult,agentResponse) or when the parent acts on the returned content.