Skip to content

fix(policy): reserve the rule names the guard decides under - #90

Merged
plusky merged 1 commit into
mainfrom
fix/reserve-synthetic-rule-names
Aug 12, 2026
Merged

fix(policy): reserve the rule names the guard decides under#90
plusky merged 1 commit into
mainfrom
fix/reserve-synthetic-rule-names

Conversation

@plusky

@plusky plusky commented Aug 12, 2026

Copy link
Copy Markdown
Owner

Closes #84.

Policy::validate never inspected Rule::name at all — no reserved words, no uniqueness, not even non-empty. So an operator could name a rule default, unavailable or min_bug_age_days, or end one with :unreadable-metadata, and that rule's decisions became indistinguishable in the audit stream from the guard's own synthetic names. Two rules could also share a name.

guard.rule is the only field saying what decided a per-bug assessment, and I3 keeps that fact from the client — so the audit stream is the only place it can hold. A name that identifies two things does not hold it.

What is rejected

Rejected Why
default, unavailable, min_bug_age_days the three literals classify/assess emit
any name ending :unreadable-metadata collides with the name generated for the rule it suffixes
two rules sharing a name a record naming it cannot say which decided
blank (empty or whitespace-only) the one check that is not about collision — a name with no content identifies nothing

Boot-breaking by design. A policy that started before now fails at startup, naming the offending rule and its position, rather than running while writing records that cannot answer what decided a call. A boot failure is recoverable in one edit; an ambiguous audit trail is not recoverable at all. README.md's policy reference gains an explicit upgrade warning, since there is no CHANGELOG.

Why exact, untrimmed comparison

The collision is byte equality on the string that lands in guard.rule, and nothing normalizes it between Rule::name and the JSON record — verified, not assumed. Default, " default" and unreadable-metadata never collided, so rejecting them would break policies that never had the problem. ends_with, not contains, for the same reason — and it correctly handles a base name containing a colon.

The blank check does trim, deliberately: it asks a different question (does this name carry content?) and matches the reading global.identity_login already gets in the same function. Documented as such.

The reservation is closed, not merely complete: no accepted name may end in the suffix and names are unique, so no generated name can equal a bare one either. The intersection is provably empty.

Adversarial review before opening

Three reviewers — guard/invariants, boundary/correctness, convention/docs. Two MERGE-SAFE, one NOT MERGE-SAFE. 21 of 23 mutations were killed on the first pass; both survivors were test defects, not code defects.

Finding Resolution
The anti-drift test was blind to unavailable. Found independently by two lenses. Renaming guard.rs's literal and the four guard_wiremock assertions a real rename would touch left the whole suite green — the reserved list would then reserve a dead spelling while the live one went unreserved, reintroducing #84 in silence. Fixed at the root: the four names are now pub(crate) constants referenced at all five emit sites, so renaming a decision renames what is reserved by construction. guard_wiremock now feeds the name assess actually emitted back through validation instead of asserting a literal. Verified: renaming the constant now releases the old spelling and reserves the new one — the drift class is deleted, not merely detected.
Duplicate detection had a coverage holereject_duplicate_rule_names used adjacent duplicates, so a mutant remembering only the previous name survived, wrongly accepting a, b, a. Production code was correct. Third rule inserted between the two. The mutant now dies.
DESIGN.md's Testing inventory gained nothing, though it already enumerates validation classes and the precedent commit added its own in the same commit. Clause added covering all five rejections, the legal near-misses, and the emitted-name check.
Nothing told an upgrader their working policy could now refuse to boot. Upgrade paragraph in the README policy reference, using the idiom already there in the opposite direction.
The blank-name error named nothingrule "": name must not be blank reproduces the exact failure the check exists to prevent, and is unfindable in a 40-rule file. Now positional: rule #2 (name = ""): …. A boot-breaking change is only defensible if its errors are fixable in one read.

Also applied: the exactness claim scoped to the collision checks (it sat beside the one check that trims), the audit.rs rustdoc qualified to "a policy loaded from TOML" (fields are pub, validate is private, so a hand-constructed Policy bypasses it), two rewrap artifacts reflowed, and the anti-drift test strengthened to assert which names were emitted rather than only that each was rejected.

Verified under refutation and left alone: validate() is provably on every production path (private, one call site in from_toml_str, main.rs the only production construction, policy.rules never mutated, no reload path); no CLI flag or env var can inject a rule or skip validation (I9); classification, capability and fail-closed logic are byte-identical; validation errors are startup-only and unreachable from MCP (I1/I12); rule names containing " or \n cannot break NDJSON framing.

Known and accepted, documented not fixed: a lone zero-width space is a legal rule name — str::trim uses Unicode White_Space, which excludes U+200B — so embargo and embargo\u{200B} are two accepted names any log viewer renders identically. The guarantee is byte-level, not human-reader-level; normalizing would break the exactness the collision checks rest on, and the policy file is the trust root, so this is operator self-harm across no privilege boundary.

Verification

cargo fmt --check, both clippy invocations, cargo test --workspace --all-targets --locked (431 passed, 0 failed) and cargo deny check all pass — re-run independently, not taken on report. cargo doc --no-deps --workspace emits the same 4 pre-existing warnings as main.

Every [[rule]] block in the repo was swept for names this change would newly reject: zero hits, so no fixture had to be renamed to accommodate the check.

Policy::validate never looked at Rule::name at all — not for reserved
words, not for uniqueness, not even for emptiness. So an operator could
name a rule "default", "unavailable" or "min_bug_age_days", or end one
with ":unreadable-metadata", and that rule's decisions became
indistinguishable in the audit stream from the guard's own synthetic
ones. Two rules could also share a name. A log consumer counting
default-decided calls by rule == "default" silently over-counts, and an
incident reviewer cannot tell a named rule's grant from the default's.

guard.rule is the only field saying what decided a per-bug assessment,
and I3 keeps that fact from the client, so the audit stream is the only
place it can hold. A name that identifies two things does not hold it.

Reject all of them at startup, plus blank names — the one check that is
not about collision, since a name carrying no content identifies nothing
either. This is boot-breaking by design: a policy that started before
now fails, naming the offending rule, rather than running while writing
records that cannot answer what decided a call. A boot failure is
recoverable in one edit; an ambiguous audit trail is not recoverable at
all.

The collision checks are exact and untrimmed, because the collision is
byte equality on the string that lands in the record and nothing
normalizes it on the way there. "Default", " default" and
"unreadable-metadata" never collided, so rejecting them would break
policies that never had the problem. The reservation is closed, not just
complete: no accepted name may end in the suffix and names are unique,
so no generated name can equal a bare one either.

Single-source the four names as constants and reference them at all five
emit sites, so renaming a decision renames what is reserved. Reserving a
list that classify no longer emits would reintroduce this issue in
silence — the guard_wiremock tests now feed the name assess actually
emitted back through validation rather than asserting a literal.

Namespacing the synthetics in the record instead was rejected: that is a
schema change and belongs with the v2 work in #34.

Closes #84
@plusky
plusky merged commit 3facec4 into main Aug 12, 2026
11 checks passed
@plusky
plusky deleted the fix/reserve-synthetic-rule-names branch August 12, 2026 07:30
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.

Nothing reserves the synthetic rule names, so an audit record cannot prove what decided it

1 participant