feat: tell the agent what to do when a policy blocks a tool call - #7328
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of 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
[DESIGN-REVIEWED] d663bad |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of 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 shipsIntent: 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.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] d663bad |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've read the full diff, both new modules, the chat_runner integration, and verified how Candidate 1 ( Candidate 2 (recovery guidance applied regardless of cause): falsified at 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 False positive or not applicable? A repository writer can comment: |
c8bce8e to
6a96651
Compare
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- src/kiro_crew/deny_guidance.py:22 -- False positive or not applicable? A repository writer can comment: |
|
Opus 4.8's blocking finding was correct and is fixed in Verified against source first. Fixed as suggested — plus one step further. The extra step: the replacement prose does not name the bypass even in order A dead end was avoided deliberately. Simply deleting the advice would leave Pinned. |
6a96651 to
e037aa9
Compare
e037aa9 to
d0aeaa9
Compare
Disposition — round 4 review findings on
|
d0aeaa9 to
68c1a52
Compare
Disposition — GPT 5.6 round 5 on
|
UX Review (Fable 5) — ✅ PASSUX-level review of UX-Verdict: PASS Human-facing surfaces (doctor section, packaged doc, folded chat card) are accurate, follow sibling conventions, and every claimed pointer resolves. Suggestions
[UX-REVIEWED] d663bad |
68c1a52 to
3095e6f
Compare
Disposition — GPT 5.6 round 6 on
|
| 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 channels — SKILL.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/.
Disposition — the three advisory lanes on
|
3ecae97 to
b5e1536
Compare
All checks green on
|
Round 15 — the widest credential class is now provider-neutralPushed WhyA review question about extensibility turned out to have a measurable answer. The
So the routing was already correct — nothing fell on the floor, and no non-AWS store was What changed
New guard, mutation-verified
Two mutations, both caught:
The four earlier mutation sets were re-run on the new head — 21 mutations caught in total. Gates
Deliberately NOT in this PRGiving each non-AWS provider a first-class class (its own anchors and its own prose) |
b5e1536 to
22c6148
Compare
Disposition of both advisory CONCERNS on
|
| 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:
- 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. - For the two newest providers there is no category at all — zero matching builtin rules.
- For the other three the category is too coarse in exactly the wrong place. All three
collapse to onesensitive-file-readcategory while needing three different sanctioned
paths. The discriminator is the refused path, which is whyclassify_denytakessubject.
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
--profileclause 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 runaws configure list-profilesand
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_COMMANDSplacement — 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 usesplatform_compat.trusted_system_bin— changed
deliberately, becausePATHcan lead with an agent-writable worktree venvbinwhile doctor
runs as the operator.- The same comment says "doctor is operator-invoked rather than agent-reachable", while
cli_doctor.py:2001states "kirocrew doctoris 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.
Round 16 — two false clauses removed from the spawn waiverPushed This is First Principles' third Watch, which was a real defect rather than a design opinion.
A false justification in a security waiver is worth a CI round on its own: the next person to Re-verified on the new head: |
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.
Rebased onto current main
One conflict, and it was an import line. Invariants re-checked on the new head: still one commit, Re-verified after the rebase: Note for the review lanes: every verdict from |
Disposition — First Principles CONCERNS on
|
|
Reviewed the diff, then drove First, the part I would ship as-is: the bug is real and the six blocks of My question is where the mapping lives. It is not a style preference -- the What the classifier actually readsFor a built-in rule the gate emits So classification is keyword matching over regex source text, plus the tool Measured over the real catalogI AST-parsed
Two of the six classes are structurally unreachable from the regex tier. The The 10 that worry me are misdirection rather than silence: These are outbound-transfer rules. Their regex source literally contains Also silently unclassified today, all in The two questions I could not answer from the current designA new rule. I wrote four of the kind we would plausibly add next -- A wrong guess. There is no signal either. The prose is static and advisory Why per-rule, and why the seam is the real issue
The mechanism for per-rule remediation also already exists: On the edition point, the module docstring says an overlay "contributes bare Concrete shape
I am not asking to hold this PR for that. The prose is mutation-verified and Repro for the numbers, if useful: AST-parse |
|
Ran your census against the same catalog, then drove the shapes through the real Reproduced, exactly. 148 rules. 115/148 no guidance from the reason alone. The misdirection is reachable, and it is the finding I should have caught
Row one settles it: the outbound-transfer floor matches a So it is fail-wrong rather than fail-silent, it is reachable, and it ships here. One rebuttal I tried and could not sustainYour census calls the classifier with an empty subject, while production also One refinementThe four Both corrections to the diff, accepted
The note channel is free. Agreed: per-rule is the targetWorth naming why per-RULE specifically, since I had measured against Your shape is what I would build: field on the dataclass, filled only where a Filed as #7890 — it carries the census, the ten-rule reachability repro, the two Keeping it out of this PR rather than patching the anchors here: the honest fix |
chenmingwei23
left a comment
There was a problem hiding this comment.
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_hintcontradicts. 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
alternativewill not cover them. Worth knowing the outbound-transfer floor
is not actually identity-less:_BASH_EXFIL_RULE_BY_PATTERNgives every
pattern a rule id, andaudit_bash_exfiltrationalready 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.
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:
model as kiro-cli's fixed
User denied tool execution— indistinguishablefrom a human clicking No, because the ACP permission response carries only
outcome/optionId. The real reason arrives separately as an in-band notice.Blocked by security policy: .*cat.*/\.aws/.*— or, on an edition that adds fnmatch globs, a bare glob with no prose at all.
DeniedCommandRulealready carries a human-readable
descriptionfor every built-in rule, butcompute_effective_deniedreturns only regexes, so it never reaches the caller._DENY_CAUSE_TEXTsuggests"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 thesame 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:
reasonstring.reasonis parsedstructurally by the frontend —
RecoveryCard'sPOLICY_REis global andper-line, and
extractDenyReasonkeeps the last marker plus everything afterit — so appending to it would inflate the deny-pattern count or be swallowed
into the displayed reason. The wire
reasonis byte-identical to before, whichis what makes this a zero-risk change for every existing parser and test.
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_commandrefuses with a deliberately generic "accessessensitive 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.
through an MCP server" hint reuses the existing
capability_manager.list_mcp(). The public default reportsavailable() == False, so it returns "" without spawning anything; the lookupis 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-rootskills/tree never reachesa 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
doctorCredentials section.doctorreported kiro-cli sign-in and nothingabout 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/credentialsis probed for existenceonly, and from
~/.aws/configonly section headers and the presence of acredential_processkey are consulted.Tests
test_deny_guidance.py(36) andtest_doctor_credentials.py(14).Two guards carry most of the weight:
security.py, notagainst copied strings. A pinned copy would keep passing after a producer
reworded itself, silently dropping the guidance with nothing going red.
is_denied+is_sensitive_bash_command+audit_bash_exfiltration. Guidancethat 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.pygains threeBENIGN_SPAWNSentries, each separatelyjustified:
_credential_vendor_line,_aws_profile_namesand_aws_auto_refreshes.The first is deliberately not claimed as a pure
asyncio.runfalse positive: on acomposed edition the awaited
list_mcp()does reach that edition's packagemanager 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 twoaws configureentries carry fixedargv with nothing interpolated, resolve the binary through
platform_compat.trusted_system_binrather thanPATH(which can lead with anagent-writable worktree venv
bin), and exist so thatdoctornever parses~/.aws/configitself.Verification
pytest(full suite),isort,flake8,mypy --platform linux,docs-lint— all green.
origin/mainfile by file viagit stash. The failing set is identical: 104 pre-existing environmentalfailures on this host (
KIROCREW_HOMEnot pinned,AF_UNIX path too long, astale 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
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.
credential vendor, so the keyword match is generic and a companion supplies
richer text through its own adapter.