feat(audits): role-based JSON error envelope validation - #79
Merged
Conversation
Reframes p2-must-json-errors from "envelope must contain literal keys
`error`, `kind`, and `message`" to "envelope must carry three semantic
roles: a discriminant, a type identifier, and a human-readable message".
The check now classifies field values by shape:
- Discriminant: error-coded string, `ok: false`, or a named field (error,
status, kind, code, reason, etc.) carrying a non-empty value. A
SUCCESS-coded value (`"ok"`, `"success"`, ...) in a discriminant-named
field is rejected (gaming-case guard).
- Type identifier: kebab-case / snake_case / constant-case string, no
spaces or period, ≤64 chars.
- Human-readable message: prose (contains whitespace OR ends in `.`/
`!`/`?`), ≥4 chars.
The traditional `{error, kind, message}` shape still passes. The
canonical `{status, reason, exit_code, message}` shape (codified in
`anc-cli-output-envelope-pattern-2026-04-29.md` and dogfooded by
`anc skill install`'s InstallEnvelope) now also passes. The check
measures roles, not vocabulary.
A first-pass implementation of the broader handoff plan at
`docs/plans/2026-06-02-001-refactor-role-based-audit-validators-plan.md`
— scoped to just p2-must-json-errors. The deeper role-coupling pass
(rejecting envelopes where each value plays a role from a different
field-name than implied) is deferred to a follow-up.
cargo test: 833 passed / 2 ignored.
… from auth scan
`p1-must-no-browser` had two bugs that combined to skip well-structured
Rust projects entirely:
1. The ast-grep pattern `fn $NAME(...)` matched only bare `fn name(...)`,
not `pub fn`, `pub(crate) fn`, `pub(super) fn`, `async fn`, or
`pub async fn`. A Rust library exposing `pub fn run_oauth2_flow`,
`pub fn refresh_oauth2_token`, etc. was invisible to the audit and the
audit returned `Skip("no auth code found")`.
2. The audit ran per-file and combined "has auth code" with "has flag" at
the same level — so a project with auth code in `src/auth/` and the
clap `--no-browser` flag in `src/cli/` could never reach `Pass` because
neither file had both signals.
Fixes:
- Extend `has_auth_functions` with a pattern set covering every common
visibility + async combination.
- Extract `has_headless_flag_definition` as a sibling scan that runs on
every parsed file independently of `has_auth_functions`.
- In `HeadlessAuthAudit::run`, accumulate the two signals across all
files, then combine at the project level.
Adds 4 new tests covering `pub fn`, `pub(crate) fn`, `pub async fn`, and
the cross-file case. Existing tests (using bare `fn`) continue to pass.
cargo test: 837 passed.
xurl-rs verification: `p1-must-no-browser` skip → pass.
brettdavies
added a commit
that referenced
this pull request
Jun 3, 2026
…llow-up plans Adds five plans surfaced by the post-merge adversarial review of PRs #73, #76, #77, and #79: - 2026-06-03-001 (umbrella, P1): structure-over-vocabulary audit philosophy. Spec is canonical; closed-vocabulary lookups (COMMON_VERBS, STANDARD_VERBS, DISCRIMINANT_FIELD_NAMES, SUCCESS_VALUE_STRINGS) are failed quality proxies; per-PR redesigns of p6-consistent-naming and p2-must-json-errors land under this plan. - 2026-06-03-002 (P1): PR #73 follow-up. Probe-failure now Warns instead of silent Pass, leaf non-verb requires a successful probe, drop emit from COMMON_VERBS, add 4 negative-case tests. - 2026-06-03-003 (P1): PR #76 follow-up. Scorecard transparency for .anc.toml domain_verbs (using_domain_verbs flag + domain_match_count + evidence string showing built-in vs domain ratio), trim 7 platform-specific verbs out of built-in STANDARD_VERBS, document the social-CLI .anc.toml pattern. - 2026-06-03-004 (P0): PR #77 follow-up. cfg(not(test)) is treated identically to cfg(test) in the new cfg-args parser, silently exempting production-only code from code-unwrap. Thread polarity through the recursive call site, add 8 negative-case tests, correct an inner-attribute comment. - 2026-06-03-005 (P1): PR #79 follow-up. Close three role-based-validator bypasses: numeric discriminant on info-coded values, success synonyms outside the 8-string closed set, and nested-recursion harvesting roles from a success-coded outer envelope. Add 6 adversarial tests. Plans 002, 003, and 005 are tactical stopgaps that stop active misclassification while plan 001 owns the long-term redesign. Plan 004 is a correctness bug fix and ships independently.
11 tasks
brettdavies
added a commit
that referenced
this pull request
Jun 4, 2026
…tests (#80) ## Summary Closes a P0 correctness regression in `code-unwrap` surfaced by the adversarial review of merged PR #77. PR #77 (commit 817d6fa) introduced a tree-sitter walk that exempts `.unwrap()` calls inside `#[cfg(test)]`-gated items. The cfg-args parser, however, treats `not(...)` identically to `any(...)` and `all(...)`: it descends into the inner argument list without polarity tracking and returns `true` whenever the bare identifier `test` appears at any depth. The effect was that `#[cfg(not(test))]` on production-only code silently exempted that code from the audit, a regression confirmed both by reading the parser and by an injected adversarial test that panicked under the buggy logic. This PR threads a `negated: bool` parameter through `cfg_args_contain_test`. The bare-test branch returns `true` only at even parity (`!negated`); at odd parity (under a `not(...)`) the predicate means production code and the walker keeps scanning siblings in case another predicate resolves to test-gated. The recursive call for `not(...)` flips polarity; `any(...)` and `all(...)` preserve it. Double-negation (`not(not(test))`) correctly restores even parity. Also corrects the inner-attribute comment block (which claimed sticky propagation across all siblings while the code actually does one-shot reset) and pins the intentional asymmetry with a named test. Per plan #4, the diff is surgical: only the parser function, the inner-attribute comment, and the test module are touched. ## Changelog ### Fixed - `code-unwrap`: `#[cfg(not(test))]` no longer silently exempts production-only code from the audit. The cfg-args parser now tracks polarity through `not(...)` wrappers; `not(test)`, `any(not(test), unix)`, and `all(not(test), feature = "x")` correctly classify as production. Polarity-pinning tests cover the double-negation case (`not(not(test))`), the `not(any(test))` shape, `cfg_attr(test, …)` non-gating, and the inner-attribute one-shot reset asymmetry. ## Type of Change - [x] `fix`: Bug fix (non-breaking change which fixes an issue) ## Related Issues/Stories - Plan: `docs/plans/2026-06-03-004-fix-pr77-code-unwrap-cfg-not-test-polarity-plan.md` - Origin: adversarial review of merged PR #77 (commit 817d6fa) - Umbrella plan (audit philosophy, supersedes plans #2 and #5): `docs/plans/2026-06-03-001-refactor-audit-philosophy-structure-over-vocabulary-plan.md` ## Testing - [x] Unit tests added/updated - [x] All tests passing **Test Summary:** - `cargo test`: 846 passed, 2 ignored (8 suites). 9 new unit tests in `src/audits/source/rust/unwrap.rs`: `cfg_not_test_does_not_exempt_production_unwrap`, `cfg_any_not_test_unix_does_not_exempt`, `cfg_all_not_test_feature_does_not_exempt`, `cfg_any_test_or_not_feature_still_exempts`, `cfg_attr_test_does_not_exempt`, `inner_attribute_one_shot_consumes_on_use_not_fn`, `mod_tests_without_gate_does_not_exempt`, `cfg_not_not_test_does_exempt`, `cfg_not_any_test_does_not_exempt`. - `cargo clippy --all-targets -- -Dwarnings`: clean. - Dogfood (`anc audit .`): `code-unwrap` flags one production unwrap at `src/audits/behavioral/json_errors.rs:192` (`trimmed.chars().next().unwrap()`). The unwrap has no cfg gate; it surfaces because PR #79 added it and the audit wasn't re-dogfooded after that merge. Verified identical behavior on pre-fix `dev` via `git stash` + rebuild, ruling out a regression from this PR. Owned by plan #5 (PR #79 follow-up) or addressable as a one-line fix in a separate PR. - Dogfood (`anc audit ~/dev/xurl-rs`): `code-unwrap` continues to `pass`. xurl-rs has zero `#[cfg(not(test))]` items, so the polarity fix is a no-op against that target. ## Files Modified **Modified:** - `src/audits/source/rust/unwrap.rs`: thread `negated: bool` through `cfg_args_contain_test`; flip polarity on `not(...)` recursion (preserve on `any`/`all`); rewrite the inner-attribute comment block to match one-shot reset semantics and name the pinning test; add 9 negative-case unit tests. **Created:** - None. **Renamed:** - None. **Deleted:** - None. ## Breaking Changes - [x] No breaking changes The audit becomes strictly more accurate. Callers that previously relied on `#[cfg(not(test))]` exempting production code from `code-unwrap` were doing so accidentally, since that path was an unintended consequence of PR #77's missing polarity tracking. ## Deployment Notes - [x] No special deployment steps required ## Checklist - [x] Code follows project conventions and style guidelines - [x] Commit messages follow Conventional Commits - [x] Self-review of code completed - [x] Tests added/updated and passing - [x] No new warnings or errors introduced - [x] Changes are backward compatible
11 tasks
brettdavies
added a commit
that referenced
this pull request
Jun 4, 2026
…ue (#82) ## Summary Addresses the `json_errors.rs:192` finding from PR #80's dogfood run: the `.unwrap()` on `trimmed.chars().next()` inside `is_type_id_value` was provably safe (an `is_empty()` guard ten lines earlier guarantees at least one char), but the `code-unwrap` source audit pattern-matches on `.unwrap()` call expressions and surfaced it during self-dogfood. Replaces the `.unwrap()` with a `let-else` that returns `false`, consistent with the function's "return false on any invalid input" pattern. Adds a comprehensive unit test (`is_type_id_value_validates_json_shapes`) covering all five non-string JSON value kinds (Number, Bool, Null, Array, Object), three valid identifier shapes (kebab-case, SCREAMING_SNAKE, snake_case), and three invalid string shapes (whitespace, period-separated, leading digit). ## Changelog ### Fixed - Fix `code-unwrap` self-dogfood finding at `json_errors.rs:192`: replace `.unwrap()` with a `let-else` returning `false` in `is_type_id_value`. ## Type of Change - [x] `fix`: Bug fix (non-breaking change which fixes an issue) ## Related Issues/Stories - PR #80 (dogfood surfaced `json_errors.rs:192` as the one remaining `code-unwrap` finding) - PR #77 (introduced the `code-unwrap` audit) - PR #79 (introduced the `.unwrap()` that this PR removes) ## Testing - [x] Unit tests added/updated - [x] All tests passing **Test Summary:** - `cargo test`: 848 passed, 2 ignored (8 suites). +1 new test `is_type_id_value_validates_json_shapes` in `src/audits/behavioral/json_errors.rs`. - `cargo clippy --all-targets -- -Dwarnings`: clean. - `cargo fmt --check`: clean. ## Files Modified **Modified:** - `src/audits/behavioral/json_errors.rs`: replace `.unwrap()` with `let-else` at line 192; add `is_type_id_value_validates_json_shapes` unit test. **Created:** - None. **Renamed:** - None. **Deleted:** - None. ## Breaking Changes - [x] No breaking changes ## Deployment Notes - [x] No special deployment steps required ## Checklist - [x] Code follows project conventions and style guidelines - [x] Commit messages follow Conventional Commits - [x] Self-review of code completed - [x] Tests added/updated and passing - [x] No new warnings or errors introduced - [x] Changes are backward compatible
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reframes
p2-must-json-errorsfrom "envelope must contain literal keyserror,kind, andmessage" to "envelope must carry three semantic roles: a discriminant, a type identifier, and a human-readable message". The field names no longer matter; the value shapes do.This is a first-pass implementation of the broader handoff plan at
docs/plans/2026-06-02-001-refactor-role-based-audit-validators-plan.md, scoped to just this one audit so the framework can be proven before applying it elsewhere.Why
ancitself emits envelopes in the canonical{status, reason, exit_code, message}shape (seesrc/skill_install.rs::InstallEnvelopeand theanc-cli-output-envelope-pattern-2026-04-29.mddoc). But the validator was hardcoded to require the legacy{error, kind, message}shape, so anc was failing its own dogfooded envelopes — and failing every consumer (xurl-rs, bird) that adopted the canonical shape.The fix is structural, not vocabulary: a well-formed error envelope needs to give an agent (a) a way to know it's an error, (b) a machine-parseable identifier to dispatch on, and (c) prose to surface to the user. Any field names that fill those three roles are fine.
What changed
src/audits/behavioral/json_errors.rs— full reframe withRoleSet,classify_envelope, and shape-based role predicates. SUCCESS-coded-value guard rejects gaming cases where a discriminant-named field carries"ok"/"success"/etc.ok: falsediscriminant, nested-object role lookup, and gaming-case rejection.Test plan
cargo test— 833 passed / 2 ignored.cargo clippy --all-targets -- -D warningsclean.cargo fmt --checkclean.anc audit . --output json | jaq '.results[] | select(.id == "p2-json-errors")'passes on anc itself.pass:60 fail:1topass:61 fail:0.Audit delta (on xurl-rs)
p2-must-json-errorsKnown limitation (follow-up planned)
A deeper gaming case
{error:"ok", kind:"<prose>", message:"<token>"}— where each field's value plays a role from a different field's expected role — can still slip past. Catching that requires field-name-to-role coupling, a larger refactor covered by the follow-up plan.