Skip to content

fix: key deny remediation by rule identity in the regex tier - #7906

Merged
iamwhatever merged 1 commit into
mainfrom
fix/deny-remediation-7890
Sep 3, 2026
Merged

fix: key deny remediation by rule identity in the regex tier#7906
iamwhatever merged 1 commit into
mainfrom
fix/deny-remediation-7890

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

A blocked tool call is handed remediation guidance chosen by the CLASS of thing
the gate refused, and the class was recovered from the refusal text. For the
built-in rule tier that reads the class out of the rule's own REGEX SOURCE, which
is accidental. Ten credential-exfil rules -- the ones that block moving AWS
credentials OUT -- name the AWS credential environment variables in their
patterns, because that is exactly what they exist to catch, so they landed in the
AWS credential-READ class. Its prose tells the caller that AWS CLI calls are not
themselves blocked and to go ahead and run the command it actually wanted:

credential-exfil-echo-aws-secret               credential-exfil-curl-aws-session
credential-exfil-echo-aws-access               credential-exfil-export-aws-access
credential-exfil-echo-aws-session              credential-exfil-export-aws-secret
credential-exfil-curl-aws-secret               credential-exfil-python-boto3-get-credentials
credential-exfil-curl-aws-access               credential-exfil-python-botocore-credentials

Measured on this base before the change, over the whole built-in catalog: 115 of
148 rules resolved to no guidance at all, self-protection rules reaching the
self-protection class 0 of 7, any rule reaching the exfiltration-shape class 0.

Why it matters

This is fail-wrong rather than fail-silent, which the module's own docstring
names as the worse of the two. The guidance exists because an agent that is
refused and told nothing retries the same shape under a different reader and then
reports the capability missing; guidance that instead invites it to rerun the
refused command spends a turn and teaches it that the advice is untrustworthy.
The affected class is outbound credential transfer, where "try it again" is the
last thing the refusal should say.

Separately, adding a rule to the catalog was a fully green change that shipped
with no remediation and nothing turning red -- and that is the normal way this
catalog grows.

What changed (motivation -> approach -> change)

Symptom: an outbound-transfer refusal receives credential-read prose. Root
cause: the class is inferred from refusal TEXT for every tier, including the one
tier that already knows which rule fired. Fix: key that tier by rule identity.

The refusal's first line is DENY_REASON_PREFIX plus the pattern of the rule
that fired, so classify_deny reads that line, resolves the rule, and answers
from two tables:

  • _CATEGORY_CLASSES -- the three categories that have a sanctioned path at all
    (sensitive-file-read, credential-exfil, self-protection). Mapping
    credential-exfil to the outbound-transfer answer is what fixes the ten
    headline rules; no per-rule row is needed for them and none is shipped, since a
    row repeating its category's answer would have no effect on the lookup (a
    census now fails on one).
  • _RULE_CLASSES -- the 20 rules whose answer is NOT their category's. The three
    instance-metadata rules become credential-read, which is what they are (they
    ACQUIRE a credential, and the sensitive-path floor already answers that way for
    the same address, so the two enforcement routes agree). The two product-token
    rules become self-protection, matching the argv-structural floor. Two legacy
    secret-READ rules filed under the exfiltration category become the
    credential-material answer. The nine AWS-profile rules in sensitive-file-read
    are named explicitly rather than left to the .aws anchor in their pattern.

A refusal that names a rule is answered by that rule and nothing else, and the
anchor scan runs only for producers that name none -- the fnmatch overlay, the
sensitive-path floor, the exfiltration audit -- which is where a classifier is the
right tool. Two consequences, each pinned by a test:

  • Identity wins even when the rule has NO guidance. A rule's silence is an
    answer; letting it fall through meant aws ec2 terminate-instances --instance-ids ... --profile sso was answered with enterprise-SSO login prose,
    matched from a word in its own arguments. (Reachable before this change and on
    the previous head of this PR; found by GPT 5.6 review.)
  • The subject/title is read on the scan path only. A command refused for moving a
    credential necessarily contains that credential's name, so weighing the subject
    against a rule's own verdict re-created the defect by another route:
    aws s3 cp ./report.aws s3://bucket drew credential-read prose. (Also found by
    GPT 5.6 review, on the first head.)

Measured after: all 61 rules in the three remediation categories resolve, zero
non-metadata credential-exfil rule reaches the credential-read class, and the
87 rules in the other seven categories answer silence authoritatively (a destructive
rm explains itself).

No producer in security.py changed, so the wire reason is byte-identical.

The security import is at module scope, per the repo's top-level-imports rule
and because there is no cycle to avoid: security does not import this module,
and platform/defaults.py:29 -- which this module already imports at module scope
-- imports security itself, so that edge existed before this change and nothing
now loads eagerly that did not before. Verified by importing deny_guidance
first in a fresh interpreter and via security, cli_doctor, dashboard.state,
hooks and platform.defaults; all six orders are clean.

What is deferred is the index BUILD, and only for one reason: an edition's rules
arrive through the DeniedRuleProvider seam and are not knowable until it is
composed, so an index built at import time would omit them permanently. A
successful lookup is cached -- including the empty list an ungoverned host returns
-- while a raise is not, so one transient composition fault at a process's first
refusal cannot pin a built-ins-only index and put exactly the enterprise-added
rules back on the anchor scan.

Two deliberate deviations from the issue's "Agreed shape", both called out for
the reviewer:

  1. It proposed an alternative field on DeniedCommandRule delivered through
    is_denied(reason_notes=...), plus DeniedRuleProvider.denied_rules()
    returning full rule objects so a composed edition can carry its own per-rule
    remediation. This PR does neither. Routing to the existing class prose meets
    all five acceptance criteria with one table instead of ~61 copies of that
    prose on rule rows, and without changing the wire reason for 61 rules or the
    provider interface. The edition-extensibility seam is a real want but it is a
    separate concern from the ten wrong pairings and is left open.
  2. Acceptance criterion 1 says no credential-exfil rule resolves to the
    AWS-credential class. Three of the 27 still do, on purpose: the
    metadata-endpoint rules refuse a credential ACQUISITION, not an outbound
    transfer, so the read answer is the actionable one there and diverging from
    the floor would make the two routes contradict each other. The criterion's
    operative clause -- an outbound-transfer refusal never receives text stating
    that AWS CLI calls are not blocked -- is asserted over the whole category with
    those three named as the exception.

Tests

All in test/test_deny_guidance.py. 32 of them fail on this base without the
source change; 116 pass with it.

  • TestRuleIdentityRoutesTheRegexTier::test_each_pairing -- one case per
    corrected pairing (the ten defects, the three metadata, the two token, the two
    legacy, plus category-default and anchors-still-win cases).
  • test_an_outbound_transfer_never_hears_that_aws_calls_are_allowed -- the
    decisive case end to end through the real producer chain, first asserting that
    neither always-on floor answers it, then asserting the OUTPUT prose lacks the
    credential-read sentence.
  • test_the_command_title_cannot_pull_a_rule_off_its_own_class -- the fix would
    hold only for a reason inspected without its command if the subject were read
    with equal authority.
  • test_a_rule_tier_self_protection_refusal_matches_what_the_floor_says -- the
    two enforcement routes to the same rule must say the same thing.
  • test_an_operator_note_is_not_read_as_a_rule_identity,
    test_the_index_survives_a_reset.
  • TestCatalogCensus -- every rule in the catalog resolves from its identity rather than the anchor scan; every rule in a remediation category resolves to prose; no two rules share a pattern with different classes; no _RULE_CLASSES row merely repeats its category; no
    outbound-transfer rule gets the read answer; the other seven categories still
    get nothing (so a future table entry cannot spray prose over destructive
    rules); every table key names a rule id / category that exists (a rename must
    fail here rather than silently lose its correction); every routed class has
    prose.
  • TestWireReasonFirstLineIsUnchanged -- the first line of every one of the 148
    rules' refusals is exactly the prefix plus the pattern, and an operator note
    never reaches it. This is the contract RecoveryCard.tsx parses and the one
    the identity lookup now also depends on.

Run: pytest test/test_deny_guidance.py -n 2 -- 116 passed. black --check,
flake8, isort --check-only and mypy clean on the touched files.

Manual verification

N/A -- unit coverage is sufficient: the assertions drive the real producers in
security.py and the real classifier output, which is the whole surface this
change touches. The before/after census quoted above was produced by the issue's
own repro against this branch.

Related Issues

Closes #7890

Pattern harvest

Rule candidate: review-prompt
Pattern: a value recovered by re-parsing text that a caller already held
structurally -- here the deny class inferred from a rule's regex source when the
rule's own id and category were in hand, so the classification tracked the
author's incidental word choice instead of the rule's purpose.

@chenmingwei23
chenmingwei23 requested a review from a team as a code owner September 2, 2026 16:00
@chenmingwei23
chenmingwei23 requested a review from Zedmor September 2, 2026 16:00
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Root-cause fix at the right seam: the tier that knows which rule fired now answers from identity, and text scanning is confined to producers that genuinely have only text.

The one structural risk — the identity lookup re-parsing the refusal's first line — is a contract RecoveryCard already depends on and TestWireReasonFirstLineIsUnchanged pins over all 148 rules, and the wire-compat rationale for not threading the rule object through is sound. The permanent index cache is safe because reset_context() is test-only, so no runtime recomposition can strand it stale. Both deviations from the issue's agreed shape are argued and leave the edition-extensibility seam open rather than foreclosed.

[DESIGN-REVIEWED] a6eea32

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/deny_guidance.py:313 -- _rule_class_state = index permanently caches built-ins when the edition provider transiently fails because edition_denied_rules() converts the failure to [], so recovered edition-rule denials keep receiving anchor-based guidance -> Fix: do not cache an index built from an empty edition result.

[GPT-REVIEWED] a6eea32

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

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

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

All claims verified: the fix has 3 production consumers (chat_runner.py:586, state.py:2649, state.py:2745), no pre-existing reason→rule reverse lookup exists anywhere in src/ (grepped DENY_REASON_PREFIX: only the new _rule_class reads it back), _RULE_CLASSES holds exactly the 20 rows the description claims, and both declared deviations from the issue's shape are argued and pinned by tests.

First-Principles-Verdict: PASS

One root cause — the rule-naming tier inferred class from regex wording — fixed at that cause, with every sibling route (argv floor, git-publish id, subject leak) covered.

What this change ships

Intent: stop a blocked credential-exfil command from receiving guidance that invites rerunning it — a FIX.

  1. Outbound-transfer refusals no longer told "AWS CLI calls are NOT blocked" — justified
  2. A rule with no guidance now answers silence; --profile sso in a destructive command no longer draws SSO prose — justified, same cause
  3. Command text can no longer steer a rule-named refusal off its class — justified, same cause
  4. Id-led git-publish refusals recognized as rule-named, not scanned — justified sibling fix
  5. 20 rules explicitly re-classed (metadata→read, token→self-protection, legacy→secret-file, nine .aws reads) — justified, deviation declared
  6. Edition rules indexed lazily; a transient fault is retried, not cached — justified (DeniedRuleProvider seam)
  7. reset_rule_class_index() test seam — matches existing reset_credential_tool_hint_cache precedent
  8. Census tests: adding a catalog rule can no longer ship unremediated or anchor-classed — justified (closes the declared "fully green" gap)
  9. Spec updated same commit — mandated by AGENTS.md

Watch

_RULE_CLASSES/_CATEGORY_CLASSES are string-keyed tables living apart from the catalog they describe; a rename elsewhere silently no-ops an entry. The change itself pins this (test_every_routing_key_names_something_that_exists), so it holds only while that census stays in sync with any second catalog source an edition adds.

[FIRST-PRINCIPLES-REVIEWED] a6eea32

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

The sole candidate is a cache-staleness hypothesis on _rule_class_index(): a successful edition_denied_rules() (possibly empty) is cached (if edition_resolved: _rule_class_state = index), and the candidate speculates edition rules could arrive or change later in the same process leaving a stale index. Its (a) never resolves past "if composition is strictly one-shot at startup this never manifests" — the candidate itself cannot establish that the composed DeniedRuleProvider set mutates after a successful resolution, and edition_denied_rules reads a context composed once at startup. Both the empty-list success caching and the raise-is-not-cached path are deliberate and pinned by tests (test_a_transient_edition_failure_is_not_cached, test_an_edition_lookup_failure_degrades_to_the_built_ins). Even granting the premise, the documented worst case is "less apt prose" — guidance carries no enforcement authority, so no crash, data loss, or removed guard. Fails (a) and does not reach any BLOCKING class.

[OPUS-REVIEWED] a6eea32

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

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

@chenmingwei23
chenmingwei23 force-pushed the fix/deny-remediation-7890 branch 2 times, most recently from 22ca2dd to 72866a9 Compare September 2, 2026 17:38
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 2, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Audit note — #7907 is being closed in favour of this PR

Two independent adjudicators both ruled this the surviving implementation.

What the two shared

Same function, same mechanism: src/kiro_crew/deny_guidance.py classify_deny (anchor-only at origin/main:296) gains rule_class = _rule_class(reason); if rule_class is not None: return rule_class in #7906 and declared = _declared_class(reason); if declared: return declared in #7907, both immediately before the pre-existing text = f"{reason or ''} {subject or ''}".lower().strip(); both parse the first line after security.DENY_REASON_PREFIX (security.py:2844, emitted by _deny_reason at security.py:12855) against a pattern-keyed map over security.BUILTIN_DENIED_RULES (#7906 _rule_class_index() with setdefault(rule.pattern, _RULE_CLASSES.get(rule.id) or _CATEGORY_CLASSES.get(rule.category, "")); #7907 _DECLARED_CLASS_BY_PATTERN = {rule.pattern: rule.deny_class ...}). Coverage asymmetry favouring #7906: #7907's guard is truthiness, so an aws-destructive refusal (e.g. rule aws-destructive-ec2-terminate-instances, security.py:522) falls through to the scan and --profile sso in the subject matches the sso anchor at origin/main deny_guidance.py:64-66, returning SSO login prose; #7906 returns the rule's "" as final (test_a_named_rule_with_no_guidance_answers_silence_not_an_anchor). Prose-fit asymmetry: #7907 declares credential-exfil-kirocrew-token and -argv as trust_root, whose REMEDIATION text is about an unreachable policy path ("let the user edit it themselves"), while the self_protection text verbatim says "reach the product's own credential mint" — what those two rules refuse; #7906's _RULE_CLASSES routes them to self_protection and pins agreement with the argv-structural floor route.

Why this one

Yes — #7906 is the right survivor. It fixes a strict superset of the reported defect class: the same ten AWS-named exfil rules, plus the subject-driven misclassification on the 87 rules that have no sanctioned path, which #7907 leaves live because it only short-circuits on a truthy declaration (verified reachable: the sso anchor at origin/main deny_guidance.py:64-66 matches --profile sso in the command title). It is also the more apt on the two credential-mint rules that #7907 routes to trust_root, it carries the larger census (8 catalog guards including test_no_rule_takes_its_class_from_the_anchor_scan and TestWireReasonFirstLineIsUnchanged over all 148 rules), and it keeps the wire reason byte-identical by construction with zero churn in the keystone security.py. #7907's counter-arguments are real but weaker: all advisory lanes PASS versus an unaddressed design-lane CONCERNS on #7906 — but that CONCERNS names the git-publish ID-led gap, which #7907 shares and does not disclose, so the delta is disclosure rather than defect count. One caveat against #7906 worth recording: its deferred-import rationale is partly overstated, since cli_doctor.py:102 already imports kiro_crew.security at module level.

What #7907 had that this PR does not

Please pick these up (or say they are not wanted):

Three items from #7907, none of them blocking for #7906. (1) The classification reading for four rules — credential-exfil-python-boto3-get-credentials, credential-exfil-python-botocore-credentials, credential-exfil-export-aws-access, credential-exfil-export-aws-secretaws_credential, where #7906 sends them to exfil_shape; exfil_shape's prose is "move a local file's contents off this host … name the file and the destination", which fits neither export AWS_SECRET… nor boto3.Session().get_credentials(), whereas the aws_credential text ("the SDK inside the aws process still reads it for you") is actionable for both. Both authors flag #7890's acceptance criterion 1 as needing exceptions (3 in #7906, 7 in #7907), so a maintainer should settle those four. (2) The deny_class: str | None = None field on the frozen DeniedCommandRule (src/kiro_crew/security.py:72-90) plus the matching docs/system-specs/modules/security.md field-list sentence — worth carrying only if the edition-remediation seam #7906 explicitly leaves open is wanted; it delivers nothing today in either PR. (3) Not from #7907 but must survive the close: both leave the git-publish gated floor's ID-led refusal (return _reason(tag, …), security.py:13126) on the anchor+subject scan, so git push origin fix-sso-login still draws SSO prose from its own branch name; #7906's new module docstring and its learn-cron-dashboard.md paragraph claim that floor is identity-routed, so either add setdefault(rule.id, …) to the index or correct the claim before merge (flagged by #7906's design lane and GPT lane; #7907 shares the gap silently).


From a repository-wide duplicate/overlap audit of every pull request open against main (2026-09-02, 330 PRs, one reviewer per PR). Each PR was read as its full merge-base diff plus its description and every comment and review, then compared against each candidate PR's own diff and against origin/main at 1a765b88ceb7. This PR is not being closed — the note is informational. If the reading is wrong, please correct the reasoning rather than just the conclusion.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Thanks for the audit note. All three items are answered below; two are now in the
branch at 923a6b4ffeaf2bec07aab50514207b8ad60adccf (rebased onto 556f08f93,
still one commit).

(1) The four rules -- taken, and your reading is the right one

credential-exfil-python-boto3-get-credentials,
credential-exfil-python-botocore-credentials,
credential-exfil-export-aws-access and credential-exfil-export-aws-secret now
resolve to aws_credential. Their own catalog descriptions settle it: the two
interpreter rules "resolve and can print the active AWS credentials from the
credential chain", and the two export rules "inject an attacker-chosen AWS
... key into the environment for later use by AWS tooling". Nothing leaves the
host in either shape, so exfil_shape's "name the file and the destination and
let the user send it themselves" answers a question that was never asked, while
"the SDK inside the aws process still reads it for you" is the step the caller
was actually reaching for.

That makes the exception list to #7890's acceptance criterion 1 seven rules, not
three, and the census test names all seven with that reasoning rather than
carrying a bare allow-set. The boundary I did NOT cross: credential-exfil-echo-aws-*
stays exfil_shape, because its description is "print ... to stdout/logs where it
can be captured" -- the harm is the value leaving into a log, which is what that
prose is for.

(2) The deny_class field -- not wanted, and I would rather say so than carry it

You called it correctly as delivering nothing today. Both review lanes on this PR
have separately asked me to delete zero-consumer surface from this change (an
unused reset hook, then ten table rows that duplicated their category's answer),
and adding a field to the frozen DeniedCommandRule in the keystone security.py
that no code path reads would be the largest instance of the same thing. The
edition-remediation seam from #7890's "Agreed shape" stays explicitly open and
unimplemented here; when it lands it should bring the field with a consumer.

(3) The git-publish ID-led gap -- fixed, and your mechanism is right, but the example is not reachable

Fixed by indexing identities rather than patterns alone: _rule_class_index now
does setdefault over both rule.pattern and rule.id, so the gated floor's
_reason(tag, ...) refusal resolves to its rule's class ("" for git-publish)
and is never scanned. A new census asserts no two identities can collide with
different classes, since patterns and ids now share one namespace.

One correction for the record: git push origin fix-sso-login is not denied --
an explicit feature-branch push is allowed to fall through, so it never reaches
the classifier and cannot draw SSO prose. The mechanism you describe is real
though, and I verified it on a command the floor does refuse:

git push origin main # rotate the sso session first
-> Blocked by security policy: git-publish-push-protected-branch-name
-> before: sso_credential -> "This is a live enterprise SSO bearer credential..."
-> after:  "" (no guidance), which is what a git-publish rule declares

That is now pinned by test_a_refusal_that_leads_with_a_rule_id_is_also_recognised.
The push itself was and remains refused either way; only what the caller is told
next changed.

Also taken from your note: the deferred-import rationale was overstated and now
says what is actually true -- cli_doctor.py already imports kiro_crew.security
at module scope, so the deferral is about keeping the edge out of the import graph
(nothing in security imports this module today), not about load time.

Base-owned red

Dependency Audit / Audit Production Dependencies was red on the previous head on
four fast-uri advisories in website/electron/package-lock.json, published
2026-09-02 15:41-15:44Z. Not diff-caused: the identical gate passed at 16:10Z and
failed at 17:17Z on the same lockfile. fast-uri went 3.1.5 -> 3.1.7 on main
during that window, so the rebase absorbs the fix rather than this PR folding one
in.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 2, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/deny-remediation-7890 branch from 923a6b4 to d10b575 Compare September 2, 2026 21:38
Ten credential-exfil rules were answered with AWS credential-READ guidance
telling the caller that AWS CLI calls are not blocked and to rerun the command
it wanted. The class was recovered from the refusal text, so for a refusal that
names a rule it was read out of that rule's own regex source -- and those rules
name the AWS credential environment variables because that is what they exist
to catch.

A producer that names a catalog rule now takes the class from that rule's
identity and nothing else: _CATEGORY_CLASSES for the three categories that have
a sanctioned path, _RULE_CLASSES for the 20 rules whose answer is not their
category's. That holds when the rule has NO guidance too -- a rule's silence is
an answer, so `aws ec2 terminate-instances --profile sso` no longer draws
enterprise-SSO login prose from a word in its own arguments.

The index covers the EFFECTIVE catalog, built-ins plus whatever the edition
contributes through the DeniedRuleProvider seam, composed the way hooks.py
composes the enforced set. Indexing built-ins alone would leave exactly the
rules an enterprise adds on the scan. It is keyed by both the pattern and the
rule id, because a refusal leads with whichever its producer chose: the regex
tier and the argv floor report the pattern, the git-publish gated floor reports
the id. Keying patterns alone left that floor scanned, so `git push origin main
# rotate the sso session first` was answered with live-SSO-credential prose.

The anchor scan now answers only for producers that refuse generically -- the
fnmatch overlay, the sensitive-path floor, the exfiltration audit -- and it is
the only path that reads the subject: a command refused for moving a credential
necessarily contains that credential's name, so `aws s3 cp ./report.aws
s3://bucket` would otherwise be answered as a credential read.

Seven credential-exfil rules keep the credential-read answer because they move
nothing off the host: three fetch from the metadata endpoint, two resolve and
print through an SDK, two inject an attacker-chosen credential into the
environment. The outbound-transfer prose ("name the file and the destination")
answers a question those never ask.

All 61 rules in the three categories with a sanctioned path resolve; the other
87 answer silence. The wire reason is untouched -- no producer changed. A
catalog census fails when a rule in a remediation category resolves to no
guidance, when any rule's class comes from the anchor scan rather than its
identity, when two identities collide with different classes, and when a
_RULE_CLASSES row merely repeats its category's answer.

Closes #7890
@chenmingwei23
chenmingwei23 force-pushed the fix/deny-remediation-7890 branch from d10b575 to a6eea32 Compare September 2, 2026 22:49
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Rebutting the GPT 5.6 finding on a6eea3226d6792c9ed641068531616d292c1bb91 rather
than actioning it, with reasoning rather than just a conclusion. No code change,
so the head is unchanged.

The finding's premise is correct. edition_denied_rules() re-raises only
PlatformCompositionError and converts any other exception to []
(security.py, except Exception: ... return []). So my edition_resolved flag
cannot distinguish "the provider legitimately has no rules" from "the provider
threw and the wrapper swallowed it", and in the latter case a built-ins-only index
is cached. That much is accurate and I am not disputing it.

The prescribed remedy is wrong. "Do not cache an index built from an empty
edition result" reads as a narrow guard, but [] is the normal, permanent answer
on this edition: DefaultDeniedRuleProvider.denied_rules() returns []
unconditionally (platform/defaults.py). So the rule "never cache on empty" is
"never cache" for 100% of public installs, rebuilding the whole 148-rule index on
every denial. That is a real option, but it should be argued as "delete the cache",
not adopted by way of a guard that looks conditional and is not.

The finding does not reach a defect, for a structural reason. The scenario needs
the same composed provider to fail once and succeed later in one process. Design
Review verified on this head that the composed context is one-shot -- reset_context()
is test-only, so no runtime recomposition can strand the index -- and Opus
independently ruled the same candidate out because it "cannot establish that the
composed DeniedRuleProvider set mutates after a successful resolution". Reaching
it therefore requires a third-party provider whose own denied_rules() is
non-deterministic. No such provider exists in this tree, and the documented worst
case is less apt prose: this path explains a block that already happened and
carries no enforcement authority, so there is no crash, no data loss and no removed
guard.

Residual, recorded rather than hidden. If an edition ever ships a provider that
resolves non-deterministically, a swallowed failure at the first denial of a process
would leave its own credential-exfil rules on the anchor scan for that process.
The two failure paths that ARE reachable today are both fixed and pinned:
test_a_transient_edition_failure_is_not_cached (a raise is retried, not cached --
and it asserts the degraded answer is the wrong prose, which is what makes caching
it consequential) and test_an_edition_lookup_failure_degrades_to_the_built_ins.

I have not pushed for this. The head currently has zero blocking findings with
Design Review PASS, First Principles PASS and Opus reporting no findings; a push
re-rolls all five non-deterministic lanes, and spending that on an advisory whose
remedy I would decline is a bad trade. Happy to delete the cache outright if a
maintainer prefers that shape -- it is a three-line subtraction and would also drop
reset_rule_class_index(), whose only consumers are tests.

@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 Sep 3, 2026
@iamwhatever
iamwhatever enabled auto-merge (squash) September 3, 2026 02:06

@iamwhatever iamwhatever left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Tier 1 auto-approve: fix (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: deny remediation prose keyed by rule identity instead of inferred from the rule's regex source, which mis-keyed ten credential-exfil rules onto credential-read guidance. Spec files changed as a ride-along (a minority of the diff on both file count and changed lines), not reviewed as a design decision: docs/system-specs/modules/learn-cron-dashboard.md.

@iamwhatever
iamwhatever merged commit fe5009b into main Sep 3, 2026
67 of 74 checks passed
@iamwhatever
iamwhatever deleted the fix/deny-remediation-7890 branch September 3, 2026 02:06
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 3, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • PR #6230 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6230: REBASE. The rename is textually conflict-free (git merge-tree against current origin/main auto-merges all four code files with no conflict) but semantically stale against code merged after the PR's base. On rebase, either keep the literal 'write-protected config path' phrasing or add both the generalized phrase and the TLS-alias reason to deny_guidance._CLASS_ANCHORS under DENY_CLASS_TRUST_ROOT. Files: src/kiro_crew/hooks.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

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.

Per-rule deny remediation: ten exfiltration refusals get read guidance

3 participants