Skip to content

fix(security): hook edit gate judges the diff content block path too (#9297) - #9371

Merged
bolichen97 merged 2 commits into
mainfrom
fix/hooks-edit-gate-diff-path-9297
Sep 9, 2026
Merged

fix(security): hook edit gate judges the diff content block path too (#9297)#9371
bolichen97 merged 2 commits into
mainfrom
fix/hooks-edit-gate-diff-path-9297

Conversation

@NicholasRBowers

@NicholasRBowers NicholasRBowers commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

KiroCrew has two gates that judge a file-edit tool call. PR #9197 taught the always-enforced tier (llm_helpers._edit_target_denial) to judge an edit by the UNION of the paths named in its params and the path named by the tool call's {"type": "diff"} content block (event.diff_path), and to deny an edit whose union is empty. The parallel gate in hooks.on_tool_call — the tool_kind == "edit" branch — still read only target_paths(raw_params).

A backend can stream trusted params that carry no path key at all and name the file only in the diff content block. Such an edit was invisible to the hook gate: a write to a protected config file could pass it unjudged, and an edit naming no target at all was approved blind instead of denied. The two gates judged different target sets. This was raised as a Design Review Watch item on #9197 and tracked as deferred-finding issue #9297.

Why it matters

The hook gate is the ONLY edit gate on the channel dispatch paths (Slack, Discord, Telegram, messaging, task executor/planner, subagent manager, CLI chat, auto-improvement) — those drive TurnDriver directly, not llm_helpers._resolve_permission. On every one of those surfaces, an edit that names ~/.kiro/crew/config.json only in its diff block skipped the write-protection that exists so a prompt-injected agent cannot rewrite its own resource ceilings. The gap was exactly the shape #9197 fixed one tier up.

What changed (motivation → approach → change)

The defect was drift between two computations of the same fact, so the fix makes the fact single-sourced. A new helper, platform.tool_paths.edit_target_candidates(raw_params, diff_path), returns the union of every accepted path spelling in the params plus the diff block's path. A second helper, is_edit_call(tool_kind, diff_path), is the single routing predicate for the write plane: a call is an edit when it declares the edit kind OR its tool_call frame carried a {"type": "diff"} content block naming a path. The diff block is the edit's target of record — the diff_path cache is written only when a frame declares a file change, so no legitimate non-edit call carries one — and the ACP kind field is spec-optional and agent-influenced on permission frames, so it is never the gate: a kindless or read-labelled call carrying a diff block is judged as an edit, and the read allowance is keyed on the absence of a diff block. Both helpers live in platform.tool_paths for the same layering reason as target_paths: llm_helpers imports hooks, so hooks cannot import from llm_helpers without a cycle. A truncated walk returns as-is (no append past the work cap); the truncated flag keeps its hard-deny reading in both consumers.

hooks.on_tool_call gains a diff_path parameter. Its edit branch enters on raw_params is not None or diff_path (not truthiness — raw_params={} must be judged and denied on its empty union, exactly as _edit_target_denial judges any dict; a truthiness guard would be the falsy-guard fail-open class). The branch judges the shared union, denies an empty union, and keeps its own fail-closed reading of the truncated flag so a reorder of the keystone above cannot silently turn a partial scan into a pass. A diff-block path that is still relative after ~/env expansion is denied as unverifiable in BOTH tiers: the diff block's path is a verbatim backend field, and a relative one resolves against the gateway process CWD rather than the agent workspace, so a workspace symlink could point it at a protected file no gate would recognize under its unanchored spelling. An edit carrying raw_params=None and no diff block still falls through, matching the always-enforced tier, which such an edit never reaches. The deliberately-unmirrored empty/unknown tool_kind read allowance above the branch is untouched.

Every enforcing caller that hands raw_params to on_tool_call now hands event.diff_path alongside it: dashboard chat runner, Slack handler and transport dispatch, Discord/Telegram transport dispatch, messaging dispatch, subagent manager, task executor, task planner, CLI chat, the auto-improvement agent runner, and llm_helpers._resolve_permission. llm_helpers._edit_target_denial now consumes the shared helper instead of its own copy of the union. The governance plane consumes it too: classify_tool_args classifies every candidate in the union as a filesystem.write item (an unanchored diff path becomes a never-permittable marker item, the same construction as the truncated-scan marker), so a diff-only edit is judged against an operator ALLOW-mode write confinement instead of reaching it pathless. A tripwire test walks the source with ast and fails if any production on_tool_call call site (direct or passed as a callable, the asyncio.to_thread shape) passes raw_params without diff_path — this is the second round of this exact drift, so the invariant is now pinned mechanically.

Tests

test/test_hooks_edit_gate_diff_path.py (new, red-verified against the pre-fix behavior — 6 of the tests fail with the union widening and empty-union deny reverted):

  • an edit naming a write-protected path only in the diff content block is denied by the hook gate; both sources are judged; a diff-block-only edit with raw_params=None is still judged
  • a safe diff-block path and an ordinary params-named edit still pass
  • an edit whose params and diff block together name no target is denied; raw_params={} takes the same deny (not skipped by a truthiness guard); raw_params=None with no diff block falls through
  • a diff-block path still relative after ~/env expansion is denied as unverifiable in both tiers (it would resolve against the gateway CWD); a ~-spelled path expands deterministically and is judged on its merits; the shared helper sets the unanchored flag and withholds the path from the candidate set
  • governance classifies the same union: a diff-only edit outside an ALLOW-mode filesystem.write confinement is denied end-to-end through on_tool_call (and one inside it still passes); an unanchored diff path emits the never-permittable marker item; read/diff-less classification is pinned unchanged
  • the write plane routes on the diff block, not the kind: a kindless call carrying a diff block naming write-protected config is denied by the hook gate, a read-labelled call carrying one is denied too, a kindless diff-block call with a safe target passes, governance classifies the kindless shape as a write (keeping the read pairs the shape-inference fallback applies), and the diff-less read allowance stays pinned (kindless and read-kind config reads allowed)
  • the empty/unknown tool_kind read allowance is regression-pinned: a kindless config read stays allowed, a read-kind config read stays allowed, and a kindless call carrying a diff_path is not judged by the edit branch
  • end-to-end through acp._dispatch: a tool_call frame whose diff block names a protected path, followed by the permission frame, produces an event whose fields — handed to on_tool_call exactly as the dispatchers hand them — are denied
  • the two gates share one candidate computation (llm_helpers re-export is identity-checked); the truncated flag survives the union and the union never grows past the work cap
  • the edit branch's own truncated deny stays armed when the keystone is blinded (monkeypatch), so the reorder it guards against cannot land silently
  • the AST tripwire over all production call sites described above

Manual verification

N/A — unit coverage sufficient: the end-to-end test drives the real acp._dispatch frame parsing and the real HookManager, which is the exact plumbing every dispatcher uses. Full backend suite run locally: zero failure delta against the base commit (the host's pre-existing sandbox-probe failures are identical on both).

Related Issues

Closes #9297

Pattern harvest

Rule candidate: review-prompt
Pattern: "two parallel enforcement gates computing the same predicate independently — require the predicate to be single-sourced, and require every caller-threading change to carry a mechanical call-site tripwire (AST scan), because the same drift recurred one tier up within two months"

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...) — two here: the fix, plus a declared chore(comments) commit that rewords the history-narration markers in three files the fix touches (the base branch's comment-history cleanup lowered those files' baseline entries, so a tree carrying the old wording sits above its baseline and CI's shrink-only comment ratchet reds it)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

@NicholasRBowers
NicholasRBowers requested a review from a team as a code owner September 8, 2026 04:04
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 8, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

Intent: Make the hook-tier edit gate (hooks.on_tool_call) judge the same target set as the always-enforced tier: the union of the params' path spellings and the diff content block's path, with an empty union denied — single-sourced so the two gates cannot drift again (#9297).
Not a goal: Changing the empty/unknown tool_kind read allowance, the shell surface, or any judgement the always-enforced tier already makes.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of 815ea82896ffb0ce683420f290a92f9d5f416abe — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound root-cause fix — single-sourced union plus a mechanical drift tripwire — but the new relative-path hard-deny rests on an unverified backend-behavior premise.

Watch

The unanchored deny is a new fail-closed rule in the always-enforced tier, not just hook parity: before this PR a relative diff_path was appended and judged (effectively passing), now it hard-denies the whole edit even when params name a safe absolute target ("a diff-block path still relative after ~/env expansion is denied as unverifiable in BOTH tiers"). If any baseline-selectable backend emits workspace-relative diff-block paths, every file edit through it is denied on every surface — dashboard included — with no in-product override, and "Manual verification: N/A" means no real backend exercised this. The deny message is loud and a revert is clean, which is why this is Watch rather than Block.
Clears when: one real edit driven through kiro-cli (and the Claude backend) confirms diff content blocks carry absolute paths, or the ACP absolute-path contract is cited/pinned where _dispatch caches diff_path.

Suggestions

  • In governance's new edit route, a kindless diff-block call with a truncated params walk emits only the filesystem.write truncation marker; the pre-route kindless fallback emitted the filesystem.read marker too — mirror it to keep the route strictly additive, as the adjacent comment promises.

[DESIGN-REVIEWED] 815ea82

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 815ea82896ffb0ce683420f290a92f9d5f416abe — this comment is updated in place on each push.

Review details

Both premises now verified against source.

Candidate 1 (missing filesystem.read truncation marker in the kindless diff-routed edit branch): the truncated-scan flood lives in raw_params, and hooks.on_tool_call runs the always-on keystone target_paths(raw_params) at line 769–779, which hard-denies ANY truncated walk before the write-protected branch and long before the governance gate_decision call. So a truncated flood never reaches classify_tool_args through the tool gate. On top of that, the described "read confinement bypass" would additionally require an atypical ceiling that governs filesystem.read in allow-mode while leaving filesystem.write ungoverned (otherwise the still-emitted write truncation marker denies the whole call). Not reachable at the stated bar — below 80.

Candidate 2 (edit-kind call carrying url/uri now emits network.egress): requires a real file-EDIT tool whose params carry a url/uri string key, which the candidate itself concedes does not occur in practice. No concrete input, and the effect only tightens (a would-be false denial, not a security hole). Fails (a) — drop.

No self-originated finding meets the 80+ bar: the change is additive/tightening on the governance plane, both edit gates share one extraction helper, and truncated/unanchored/empty-union all fail closed with test coverage.

No findings.

[OPUS-REVIEWED] 815ea82

Verdict parsed from the review's SHA-scoped output markers for commit 815ea82896ffb0ce683420f290a92f9d5f416abe.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 815ea82896ffb0ce683420f290a92f9d5f416abe: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 815ea82896ffb0ce683420f290a92f9d5f416abe — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verification done — counts run, premises checked against the repo. Final review:

First-Principles-Verdict: CONCERNS

Both tiers now hard-deny any relative diff-block path on an unverified "backends emit absolute paths" premise; wrong once, every edit on that backend dies.

Not justified as shipped

  1. diff_path threaded by hand at 11 call sites — symptom-level: the cause is per-site hand-copying of event fields, and it persists (see Watch).
  2. Issue-number comment markers removed in discord/telegram/messaging/llm_helpers — rides along (comment-history ratchet reds the merge ref without it; CI-derived).
  3. test_governance_chokepoints.py black-formatted, black-baseline entry dropped — rides along (formatting ratchet on a touched file).

What this change ships

Inventory (8 items) — 5 justified

Intent: close #9197's deferred finding #9297 — the hook edit gate (the only gate on channel dispatch paths) must judge the diff content block's path, not just params. FIX.

  1. Edit naming a protected file only in the diff block is now denied on every channel surface — justified
  2. Edit whose params∪diff-block union is empty is denied instead of approved blind ({} included) — justified
  3. A kindless or read-labelled call carrying a diff block is judged as an edit; diff-less reads stay allowed — justified
  4. A relative diff-block path is denied as unverifiable in BOTH tiers (new deny in the always-enforced tier too) — justified
  5. Governance classifies the diff-block path as filesystem.write; unanchored → never-permittable marker — justified
  6. event.diff_path threaded at 11 enforcing call sites, pinned by an AST tripwire — symptom-level (hand-copying persists; 8 sites omit mcp_server_name)
  7. History-narration comment markers reworded in 4 files — rides along (CI-required)
  8. Black reformat + baseline removal for one test file — rides along

Watch

  • Item 4 rests on "the diff block's path is a verbatim backend field" arriving absolute. Unit tests use absolute/~ paths only; no live backend confirmed. A backend streaming workspace-relative diff paths loses every edit to a hard deny in both tiers. Clears when: confirmed absolute on a live kiro-cli (and Claude-backend) edit, or an ACP spec clause requiring absolute paths is cited.
  • The PR calls this "the second round of this exact drift," then pins only diff_path. Grepped enforcing on_tool_call sites for mcp_server_name (enforcement-relevant: deny targets, governance mcp_ref, app-ownership check): 8 of 11 omit it — slack/handler.py:3623, slack/transport_dispatch.py:578, discord/transport_dispatch.py:747, telegram/transport_dispatch.py:934, messaging/dispatch.py:307, task_executor.py:375, task_planner.py:315, auto_improvement/spine/agent_runner.py:528 — while cli_chat, chat_runner and subagent_manager pass it. Round three is already queued for the next field. Clears when: those surfaces are confirmed to never carry MCP identity, or a tracked issue covers threading event fields generally.

[FIRST-PRINCIPLES-REVIEWED] 815ea82

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 815ea82896ffb0ce683420f290a92f9d5f416abe and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 815ea82

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 815ea82896ffb0ce683420f290a92f9d5f416abe: <one-sentence reason>

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 8, 2026
@NicholasRBowers
NicholasRBowers force-pushed the fix/hooks-edit-gate-diff-path-9297 branch from e617a7c to 68c7f5f Compare September 8, 2026 04:41
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 8, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

self-added: yes
mechanism: unanchored flag on TargetPaths — a relative-after-expansion diff-block path is withheld from the candidate set and hard-denied by both tiers

  • fixed span=ab192f0ebeeb — Relative diff paths resolve against the gateway CWD (hooks.py edit branch)

Fixed in 68c7f5f. A diff content block path that is still relative after ~/$HOME expansion is now denied as unverifiable in BOTH edit gates, not resolved against the process CWD: platform.tool_paths.edit_target_candidates sets TargetPaths.unanchored and withholds the path from the candidate set, and both hooks.on_tool_call's edit branch and llm_helpers._edit_target_denial hard-deny on that flag before iterating ("Blocked: file edit names a relative target path that cannot be verified (deny-by-default)"), the same fail-closed shape as the truncated-walk deny.
This ruling covers the class for the diff-block path everywhere it is judged: the deny lives in the single shared candidate computation, so no consumer of the union can receive an unanchored diff path to mis-resolve. A ~-spelled path expands deterministically (no CWD involved) and stays judged on its merits. Pinned by TestAnUnanchoredDiffBlockPathIsDenied (hook tier with and without params, always-enforced tier, ~ non-regression, flag semantics), red-verified with the check disabled.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 8, 2026
@NicholasRBowers
NicholasRBowers force-pushed the fix/hooks-edit-gate-diff-path-9297 branch from 68c7f5f to 374e181 Compare September 8, 2026 04:59
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 8, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

self-added: yes
mechanism: governance classify_tool_args consumes the shared edit-target union (params ∪ diff_path); unanchored diff path emits a never-permittable filesystem.write marker item

  • fixed span=ab192f0ebeeb — Diff-only edits bypass filesystem.write governance (hooks.py edit branch / classify_tool_args)

Fixed in 374e181. The governance plane now consumes the SAME candidate computation as both edit gates: classify_tool_args(tool_kind, raw_params, diff_path) classifies every path in edit_target_candidates(raw_params, diff_path) as a filesystem.write item, and gate_decision/hooks._governance_denial/on_tool_call thread event.diff_path through. A diff-only edit whose target lives solely in the diff content block is therefore judged against an operator ALLOW-mode filesystem.write confinement instead of reaching it pathless — end-to-end pinned by test_governance_chokepoints.py::test_filesystem_write_denied_via_diff_only_edit (outside the allow-list denied, inside it permitted), red-verified with the diff path blinded (the probe run showed the exact bypass: the /etc/passwd diff-only edit was permitted).
This ruling covers the class for every consumer of the edit-target union: filesystem.write classification, the sensitive/write-protected tiers, and the always-enforced tier all read the one helper, so a target set present in one and absent in another can no longer exist. An unanchored (relative) diff path emits the never-permittable _UNANCHORED_TARGET_ITEM marker — the same construction as the truncated-scan marker — so a governed write scope denies the unverifiable edit while an ungoverned standalone host keeps its permit-by-default behavior. span=ab192f0ebeeb is at x2 on this path+lane; both hits were distinct defects (CWD resolution, then governance classification), each fixed at the shared-helper level rather than point-patched.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 8, 2026
@NicholasRBowers
NicholasRBowers force-pushed the fix/hooks-edit-gate-diff-path-9297 branch from 374e181 to a7c1fce Compare September 8, 2026 05:31
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 8, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

self-added: yes
mechanism: is_edit_call routing predicate — a diff content block's PRESENCE routes a call onto the write plane in all three consumers; the kind field is never the gate

  • fixed span=ab192f0ebeeb — Kindless diff edits bypass write protection (hooks.py edit branch)

Fixed in a7c1fce, at the class level after the mandated span-x3 retrospective. The routing invariant is now single-sourced beside the union: platform.tool_paths.is_edit_call(tool_kind, diff_path) — a call is on the write plane when it declares the edit kind OR its tool_call frame carried a {"type": "diff"} content block naming a path. The diff block is the edit's target of record: the diff_path cache is written in exactly two places in acp/_dispatch.py, both gated on a diff content block with a nonempty path, so no legitimate non-edit call carries one; the ACP kind field is spec-optional and agent-influenced, so it is one of two edit signals and never the gate. All three consumers route on it — the hook write-protected tier, governance classify_tool_args (which also keeps the read pairs the kindless shape-inference fallback applies, so no call loses a pair), and the always-enforced tier (target-gating now separate from document-scan suppression, so a kindless diff-block call keeps its document scan AND gains the target gate — strictly tightening).
This ruling covers the class for every spelling of a mislabelled edit: kindless, read-labelled, or any future kind value — the write plane is entered by the declared file change itself, so no kind spelling can skip it. The diff-less read allowance is preserved and pinned (a read emits no diff block, which is exactly what makes it a read). Red-verified: with the diff-block clause of is_edit_call disabled, four routing tests fail, including the kindless write-protected-config deny; the prior round's test that pinned the kindless skip as intended behavior was inverted, not kept.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

fixed — Undeclared provenance-stripping rider on a complete fix

Fixed in a7c1fce by splitting the PR into the two commits the repo allows: the fix commit no longer touches any of the thirteen citations, and a declared chore(comments) companion commit carries the scrub with a message naming what each removed id labelled, per the precedent this review cited. The scrub is not severable from the PR entirely: CI's comment-history ratchet judges every file the PR touches against a shrink-only baseline, and the five files sit above baseline on main, so touching them (which the fix must, to thread diff_path) reds Backend Lint until the counts come down. The "was refused" past tense in the _edit_target_denial docstring is restored, with the recorded defect named in prose ("the tool-input length-cap defect") rather than by id, since restoring the ids themselves would put the file back over its ratchet baseline.
The Watch item (unanchored-deny backend premise) shares its subject with Design Review's Watch and is answered in that lane's disposition: the deny is the fail-safe direction for a verbatim field the repo nowhere pins absolute, and scoping it to the empty-union case would let a relative diff block ride past judgement beside an absolute params path — a fail-open on the target of record.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

rebutted — Watch: unanchored diff-path hard-deny rests on an unverified backend premise

The deny is kept because the proposed scoping is a fail-open: the diff content block is the edit's target of record, so an edit whose params name /home/u/ok.md while the diff block names ../../.ssh/id_rsa must not be judged on the params alone — scoping the deny to "only when the union would otherwise be empty" is exactly that judgement. The deny is also narrower than the Watch reads: ~ and $HOME spellings expand deterministically before the absoluteness check (pinned by test_tilde_diff_path_is_anchored_not_denied_as_relative), so only a path that resolves against the process CWD is refused. Both cache write sites in acp/_dispatch.py take cb.get("path") verbatim from a declared file change; nothing in the repo pins that field absolute, which is precisely why an unanchored value is unverifiable rather than benign. If a shipped backend is ever observed emitting workspace-relative diff-block paths, the deny fails loud with a self-naming reason string, and anchoring against that backend's workspace is the follow-up — silently mis-resolving against the gateway CWD is the one behavior this PR exists to rule out.
The Suggestion (one security-context struct instead of loose kwargs across the call sites) is agreed as the deeper retirement of the drift class and is a wider refactor than this fix should carry; the AST tripwire is the in-scope guard until such a follow-up lands.

@NicholasRBowers
NicholasRBowers force-pushed the fix/hooks-edit-gate-diff-path-9297 branch from a7c1fce to 16a1a5d Compare September 8, 2026 05:47
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

self-added: yes
mechanism: none this round -- answer to the First Principles item

  • rebutted span=3d635f2192b2 -- the black reformat + baseline-line removal is mandated by the touched-file gate in the SAME change, not separable

CI's Backend Lint runs a diff-scoped black gate: a changed .py file not listed in .github/black-baseline.txt must be fully black-clean, so touching test_governance_chokepoints.py (which this fix must, to pin the hook edit gate through governance) pulls the WHOLE file into scope. Formatting it and removing its baseline line in this PR is the ratchet working as documented -- deferring the reformat to a separate PR would red THIS PR's lint lane. The finding itself classifies it as "the documented ratchet mechanism, harm-free rider".

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

self-added: yes
mechanism: none this round -- answer to the First Principles Watch item

  • rebutted span=9c452a1fde16 -- same ruling as the Design lane's anchoring Watch: deny-unverifiable is the safe direction absent backend evidence either way

The premise "the diff block's path is always absolute or ~-anchored" is not what the deny rests on. The deny rests on the converse: a path that is NOT anchored resolves against the gateway process CWD and therefore cannot be verified against the write-protected tier -- the same fail-closed reading this module gives a truncated walk. Pre-fix, a relative diff path was silently unjudged by every gate (the finding concedes "pre-fix a relative diff path passed both tiers" -- passed by being invisible, not by being verified safe), so no working behavior regresses; a backend streaming workspace-relative diff paths would surface as a clearly-reasoned deny, the observable and correctable failure mode. This ruling covers every instance of the anchored-diff-path evidence tradeoff, wherever it moves.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

self-added: yes
mechanism: none new this round -- docstring claim shrunk in place

  • fixed span=e72d30822187 -- the "SINGLE routing predicate for every write-plane consumer" claim is shrunk to the two consumers it is true of (commit c4044d8)

Fixed in c4044d8: is_edit_call's docstring now says it is the routing predicate the hook edit gate and governance classification SHARE, and that the always-enforced tier (llm_helpers._resolve_permission) composes the same two facts with its client-derived provenance flags before rerouting -- its route is this predicate narrowed, never a different reading of what an edit is. The over-claim is gone; the counted third spelling at llm_helpers.py is now described, not contradicted.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

self-added: yes
mechanism: none new this round -- stale _total normalized via the checker's own writer

  • rebutted span=46046f89a35d -- the gate's own writer measures messaging/dispatch.py at 1, refusing to lower it; the real slack (stale _total) is now normalized (commit c4044d8)

Ran the authoritative writer, python3 scripts/check_comment_history.py --write-baseline, on the branch: it reports "pruned 0 entr(y/ies), lowered 0" -- the checker still counts 1 marker in src/kiro_crew/messaging/dispatch.py, so lowering that entry by hand would desynchronize the baseline from its own tool and red the gate for the next PR touching the file. The finding's premise that the remaining hits are uncollected attribute docstrings does not match the collector's measurement.
The one piece of real slack the writer DID find -- the _total field left stale (7584 -> 7578) by the scrub commit -- is folded into that commit on head c4044d8.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 9, 2026
@NicholasRBowers
NicholasRBowers force-pushed the fix/hooks-edit-gate-diff-path-9297 branch from c4044d8 to 8996e4b Compare September 9, 2026 01:22
Nick Bowers added 2 commits September 9, 2026 01:26
…9297)

The always-enforced edit gate (llm_helpers._edit_target_denial) judges a
file edit by the UNION of the params' path spellings and the path the tool
call's diff content block named, and denies an empty union. The parallel
gate in hooks.on_tool_call read only target_paths(raw_params): an edit
naming its write-protected target only in the diff block was invisible to
it, and an empty target set was not denied.

The union now has a single source, platform.tool_paths.edit_target_candidates,
used by BOTH gates so they cannot drift again. A call is on the write plane
when it declares the edit kind OR its tool_call frame carried a diff content
block naming a path (platform.tool_paths.is_edit_call): the diff block is the
edit's target of record and only a call declaring a file change carries one,
so the spec-optional, agent-influenced kind field is never the gate — a
kindless or read-labelled call carrying a diff block is judged as an edit,
while the read allowance stays keyed on the absence of a diff block. The
hook's branch enters on 'raw_params is not None or diff_path' (an empty dict
is judged, not skipped), denies an empty union, and keeps its own fail-closed
reading of the truncated flag. A diff-block path still relative after
tilde/env expansion is denied as unverifiable in both tiers: it is a verbatim
backend field that resolves against the gateway CWD, not the agent workspace,
so a workspace symlink could point it at a protected file. The governance
plane consumes the same union and routing: classify_tool_args classifies
every candidate as a filesystem.write item (an unanchored diff path becomes a
never-permittable marker item, same construction as the truncation marker),
so a diff-only edit is judged against an operator ALLOW-mode write
confinement instead of reaching it pathless. Every enforcing caller that
hands raw_params to on_tool_call now threads event.diff_path alongside it,
pinned by an AST tripwire test.

Closes #9297
…es require

The base branch's comment-history cleanup lowered the baseline entries for
these three files by exactly the markers this change rewords. On the current
base, a tree carrying the old wording sits above its baseline and the gate
reds, so the rewording rides with the fix that touches these files.
@NicholasRBowers
NicholasRBowers force-pushed the fix/hooks-edit-gate-diff-path-9297 branch from 8996e4b to 815ea82 Compare September 9, 2026 01:32
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 9, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

self-added: yes
mechanism: none this round -- answer to the Design Watch item

  • rebutted span=f8fd597332e9 -- the unanchored deny is the deliberate fail-closed rule, in both tiers, for a target no gate can verify

A diff-block path still relative after ~/env expansion resolves against the gateway process CWD, so any sensitivity verdict computed from it is about the wrong file (a workspace symlink can point it at a protected one). Denying an unverifiable target mirrors the module's truncated-walk posture; the pre-change state ("appended and judged, effectively passing") was passing BECAUSE the check inspected the wrong resolution, not because the write was verified safe. A backend that streams relative diff paths surfaces as a clearly-reasoned deny -- observable and correctable -- rather than a silent wrong-file judgement. This ruling covers every instance of the anchored-diff-path evidence tradeoff on this PR, wherever it moves; live-backend capture evidence is follow-up work, not a precondition for shipping the fail-closed direction.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

self-added: yes
mechanism: none this round -- answer to the Design Suggestion

  • rebutted span=e51aa12c239d -- the truncation marker's scope pair already denies the call in ALLOW mode; duplicating it under filesystem.read adds no confinement

A truncated walk on the edit route emits the filesystem.write truncation marker, and a truncated scan is judged as unverifiable regardless of which scope carries the marker: an ALLOW-mode ceiling refuses the marker item, so the call is already denied without a second marker under filesystem.read. The read pairs the route mirrors for a kindless call are for the paths actually collected; the marker is a verdict-carrier, not a path, and doubling it would state the same unverifiability twice to the same evaluator. Strict additivity holds for every REAL pair the pre-route classification emitted.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

self-added: yes
mechanism: none this round -- deferral recorded for the threading drift

Correct: diff_path is threaded by hand at the call sites, the same way every other event field reached the gate, and that per-site copying is the root cause behind this drift class. Consolidating extraction into one helper touches the same 11 call sites this PR modifies, so doing it inside this fix would double the diff and conflict with itself; it is tracked as #9617 (deferred-finding, assigned NicholasRBowers, Due: 2026-09-30 in body), scoped to a hook_gate_kwargs(event) helper plus a parity test that fails when the event gains an enforcement-relevant field the helper does not extract.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

self-added: yes
mechanism: none this round -- answer to the rider item

  • rebutted span=98da0aeb5a6a -- the marker rewording is mandated by the merge ref's own gate, not separable

The finding itself records the mechanism: the comment-history ratchet reds the MERGE REF without these rewordings, because the base branch's cleanup lowered those files' baseline entries. A tree that touches these files and keeps the old wording cannot pass CI, so the rewording must ride in the same change; it is declared in the PR body's checklist and isolated in its own chore(comments) commit for reviewability.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

self-added: yes
mechanism: none this round -- answer to the rider item

  • rebutted span=d1c8ace994a1 -- the black reformat + baseline-line drop is the formatting ratchet's documented in-change requirement

CI's diff-scoped black gate requires a changed .py file not listed in .github/black-baseline.txt to be fully black-clean, so touching test_governance_chokepoints.py pulls the whole file into scope; formatting it and dropping its baseline line in the same change is the ratchet working as documented, and deferring it would red THIS PR's lint lane. The finding itself classifies it as a CI-derived, harm-free rider.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

self-added: yes
mechanism: none this round -- answer to the FP Watch item

  • rebutted span=37ed15140bed -- the deny does not rest on the always-absolute premise; it rests on refusing what cannot be verified

The rule is the converse of the premise the finding tests: a path that is NOT anchored after expansion cannot be resolved against the write-protected tier, and an unverifiable target is denied -- the same fail-closed reading this module gives a truncated walk. Pre-change, a relative diff path was judged against the wrong resolution (gateway CWD), which is strictly worse than a visible deny. A backend streaming workspace-relative diff paths loses edits to a deny that names its exact reason, the observable failure mode, and capturing live kiro-cli / Claude-backend frames to settle the anchoring question is evidence-gathering follow-up rather than a precondition for the safe direction. This ruling covers every instance of the anchored-diff-path evidence tradeoff on this PR.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

self-added: yes
mechanism: none this round -- deferral recorded for the wider threading audit

Verified against the tree: the slack/telegram/discord transport dispatchers, slack/handler's streaming site, messaging/dispatch, task_planner and task_executor pass no mcp_server_name/mcp_tool_name, so identity-keyed deny rules and governance @server/tool refs do not bind on those surfaces. Threading MCP identity per-site here would widen this PR beyond its stated intent and repeat the exact hand-copying the finding indicts; the general fix (one hook_gate_kwargs(event) extraction helper + a parity test over enforcement-relevant event fields, with per-surface verification of which fields genuinely exist there) is tracked as #9617 (deferred-finding, assigned NicholasRBowers, Due: 2026-09-30 in body).

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

🤖 Kiro Crew Auto-Pipeline [operator: NicholasRBowers#a942f9ca]

Review-ready at head 815ea82896ffb0ce683420f290a92f9d5f416abe.

  • Rollup: 65/65 checks green, PR Readiness passed, mergeable, 0 unresolved threads
  • All five AI review lanes complete: GPT 5.6 clean (one fail-closed non-completion cleared by targeted rerun), Opus clean, Design/First Principles/UX CONCERNS each answered with its own disposition (21 records total)
  • This drive absorbed two base advances: a rebase over the fix(security): judge a file edit by its target path, not by scanning its text #9197-adjacent drift, and a second rebase over main's mass comment-history cleanup (the chore(comments) commit now carries only the three marker rewordings the lowered baselines mandate)
  • Two review findings fixed in-flight: the governance edit route no longer sheds a call's network.egress pair (red-proven pin test), and is_edit_call's over-broad "single routing predicate" docstring claim was shrunk
  • Follow-up threading refactor (one extraction helper for hook-gate event fields; MCP identity gap on 8 of 11 call sites) tracked as deferred-finding hooks.on_tool_call event threading: one extraction helper instead of 11 hand-copied call sites #9617, due 2026-09-30

Remaining gate is the required human maintainer review. Auto-merge is NOT armed.

@bolichen97
bolichen97 enabled auto-merge (squash) September 9, 2026 07:23

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 815ea82 after trying to construct a bypass.

Single-sourced union: platform/tool_paths.py edit_target_candidates is consumed by both hooks.py and llm_helpers.py, identity pinned by test_llm_helpers_denial_uses_the_shared_helper. The always-enforced tier only tightens: _edit_target_gated = _edit_params is not None or bool(event.diff_path and not event.is_shell), while document-scan suppression stays keyed on _edit_params is not None alone, so no shell/kind forgery path opens. The hook branch enters on raw_params is not None or diff_path, denies truncated, unanchored (relative) and empty unions before the sensitivity loop; keystone handling above is unchanged. Governance plane is additive (write pairs for the union, read pairs for kindless routes, network.egress kept when a url rides along). All 11 production on_tool_call(raw_params=…) sites thread diff_path; the one un-threaded site is the non-enforcing Slack EVENT_TOOL_CALL path, and an AST tripwire pins it. security.md updated; no denied-rule count restated.

Body correction: the Tests section says a kindless call carrying a diff_path is NOT judged by the edit branch — the diff does the opposite (is_edit_call returns True on any diff_path; test_a_kindless_call_carrying_a_diff_block_is_judged_as_an_edit asserts DENY). Stale sentence. Also undisclosed: .github/black-baseline.txt graduates test_governance_chokepoints.py with 4 unrelated reformat hunks. Pre-existing residual worth a follow-up: a params-relative path (not diff-relative) still resolves against the gateway CWD rather than being denied — asymmetric with the new diff-path rule.

@bolichen97
bolichen97 merged commit 8d959b1 into main Sep 9, 2026
67 of 74 checks passed
@bolichen97
bolichen97 deleted the fix/hooks-edit-gate-diff-path-9297 branch September 9, 2026 07:59
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 9, 2026
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.

hooks.on_tool_call edit gate should judge the diff content block path too

2 participants