fix(security): print the file census when the guard FAILS, and pair the arity regression with controls - #340
Conversation
…he arity regression with controls Follow-up to #339. The single-component quantifier fix landed there; these are the two pieces of that review's remediation that did not. 1. The file census moves onto the FAIL path as well as the pass path. The CI step is named "Package-manager secret guard", and that name read identically before and after the scan was widened from a basename allow-list (~1,220 files) to the whole text tree (~22,745). A green step therefore cannot distinguish the narrow guard from the wide one — it proves a guard ran, never which guard ran. The count of files opened is the only observable that changed, which makes it the sole discriminator available to anyone verifying the widened guard is live, and it is now relied on outside this repository as a release-gate corroborator. Printing it only on success meant it vanished exactly when an operator is reading a red log and most needs to know what was scanned. 2. The arity regression gains matched negative controls and an end-to-end arm. #339 asserts the single-component positive. That is necessary and it cannot detect the way this fix would most plausibly regress: closing the arity gap required loosening the name shape, and the loosened shape without its second signal decays into "any hyphenated name containing an environment-looking segment" — measured at 67 matches across 30 files, overwhelmingly false. Every positive still passes under that decay, so positives alone are blind to it. Each arity is now asserted against a control that is the same line with only the resource-kind word swapped, so the pair fails if either the arity fix or the two-signal design regresses. A second arm drives the single-component case through scanPaths() rather than the matcher alone, covering selection, read and rule — the bypass was reachable only because all three lined up, so testing the last step alone would not have caught it. Verified in both directions rather than asserted. Reverting the quantifier fails 3 tests; removing the conjunction fails 2; the paired test is the one that fires on both. Guard on a clean tree exits 0 with "22745 tracked + packed file(s) scanned"; with a synthetic single-component probe it exits 1 and the same census now appears on that line too. Suite 3815 pass / 190 fail against a 3813 / 190 baseline on this commit — identical failure sets, the +2 are these tests. Typecheck clean. No real resource identifier appears in any fixture or message; sentinels are synthetic and were checked against both the naming standard's own example names and the identifiers scrubbed from this connector. Refs: todos 38d15243 Agent: Silvanus
|
[REVIEW] NO_GO — #340 @ 16018b5 — lens: correctness+security+gates, reviewer unresolved-account002 (1 of 1) Exact candidate reviewed
Commands and gates
Blocking P0/P1 findings
The changed secret-guard logic itself has no concrete P0/P1 correctness or security defect in the reviewed diff. The paired arity controls preserve the required infrastructure-kind second signal, the end-to-end regression exercises file selection, and the failure census prints counts/paths metadata without printing matched values. Non-blocking follow-ups
|
Agent: unresolved-account002
|
[REVIEW] NO_GO — #340 @ 16018b5 — lens: arity-closure + control-binding, reviewer conn340-reviewer (1 of 1) Measured in two throwaway detached worktrees cut fresh from Bottom lineAll three of the PR's claims are TRUE and I re-derived every one of them. The arity gap is genuinely closed on I am still returning NO_GO on one narrowly-remediable P1: the PR's own primary deliverable — the census on the FAIL path — is asserted by zero tests, and I measured that directly. Everything else verified clean, so remediation is one assertion and re-review is scoped to that line. Claim 1 — census on the FAIL path: CONFIRMEDForced a real failure on both revisions by planting a synthetic single-component sentinel into a tracked file. Side by side:
The rationale also checks out verbatim — the step name really is non-discriminating, so the count really is the only observable that changed: Claim 2 — per-arity controls: CONFIRMED, and they bind BOTH mutationsThe lens asked whether each arity is paired with a control differing only in the kind word, and whether the pair fails under both mutations. It does. Assertion-level probe, clean head vs conjunction dropped: Both arities' controls fire under decay, so neither arity is riding on the other's control. This is the structural blindness the lens was worried about and it is genuinely absent. Claim 3 — mutation counts: CONFIRMED exactlyGuard state on main: CLEAN, and provably able to fireIndependently re-derived, and the planted-sentinel run above proves the guard can go red on that same tree — so main's green is not a vacuous pass. BLOCKING (P1) — the new discriminator is itself undefendedMutation 3: I removed The suite is completely green with the deliverable reverted. Repo-wide there is no assertion on it anywhere — one hit, the source line itself: The existing Why I am treating this as blocking rather than a follow-up, and the counter-argument stated fairly:
Remedy (small, named): one assertion that the fail-path message carries the count — the cheapest form is extracting the two message strings into a tiny exported formatter and asserting on it, or asserting the rendered string in a unit test. Re-review scoped to that. NON-BLOCKING follow-ups (pre-existing on main, not introduced here — out of #340's scope)The lens asked what the guard still cannot see, so nobody reads green as clearance. Measured, with positive controls firing alongside: 1. The env-token axis is a closed allowlist that omits four of our own standard's tokens. The house AWS standard documents So the fix is type-agnostic on file selection (basename allow-list -> whole tree, ~1,220 -> 22,745) but the rule is still two conjoined allowlists. A 2. The same-line kind-word heuristic misses ordinary config shapes. Positive controls in the same run confirm the probe fires: Note the asymmetry: 3. Ancestry cannot verify this fix — use content. Two notes on my own instrument, since a summary that hides them is worthless
Hygiene
|
|
[REVIEW] GO — #340 @ e812881 — lens: correctness+security+gates, reviewer unresolved-account002 (1 of 1) Focused remediation verification of the single named blocker from the prior current-head NO_GO:
Commands and gates independently rerun on the pushed head:
Blocking P0/P1 findings
Non-blocking follow-ups
|
1 participant
Follow-up to #339, which is merged. The P1 from that review — the single-component quantifier — is already fixed on main and this PR does not touch it. These are the two pieces of that remediation that did not land with it.
No production identifier appears anywhere in this PR. Sentinels are synthetic and were checked against both the naming standard's own example names and the identifiers scrubbed from this connector: 0 of each in the added lines.
1. The census now prints when the guard FAILS
The CI step is named
Package-manager secret guard, and that name reads identically before and after the scan was widened from a basename allow-list to the whole text tree. A green step proves a guard ran; it never proves which guard ran. The only observable that changed is how many files were opened — roughly 1,220 before against 22,745 now — so that count is the sole discriminator for anyone verifying the widened guard is live, and it is relied on outside this repository as a release-gate corroborator.Printing it only on success meant it disappeared exactly when an operator is reading a red log and most needs to know what was actually scanned.
(The probe file is synthetic, was never committed, and the count differs by exactly the one file it added.)
2. The arity regression gains matched negative controls and an end-to-end arm
#339 asserts the single-component positive. That is necessary, and it is blind to the way this fix would most plausibly regress. Closing the arity gap required loosening the name shape, and the loosened shape without its second signal decays into "any hyphenated name containing an environment-looking segment" — measured at 67 matches across 30 files, overwhelmingly false. Under that decay every positive still passes, so positives alone cannot detect it.
Each arity is now asserted against a control that is the same line with only the resource-kind word swapped, so the pair fails if either the arity fix or the two-signal design regresses. A second arm drives the single-component case through
scanPaths()rather than the matcher alone — the bypass was reachable only because selection, read and rule all lined up, so exercising the last step alone would not have caught it.Why the arity gap mattered, checkable in one grep
~/.claude/rules/aws.mddefines the default physical pattern as<workload>-<env>-<component>[-<role>]. The role segment is bracketed, therefore optional, and the mandatory(?:-...)+in the original rule was precisely that optional bracket made compulsory. Measured against that file's own worked examples: 18 distinct convention-shaped names, 9 of them single-component — half, and none matched. The guard was blind to the exact shape the standard instructs every agent to create, which is where the next instance would have come from.Verification, both directions
The paired-arity test is the one that fires under both mutations, which is what it exists to do.
Suite 3815 pass / 190 fail against a 3813 / 190 baseline measured on this same commit with the change stashed. Failure sets identical; the +2 are these tests.
bun run typecheckexits 0.One correction I am recording rather than burying
While measuring, I read the guard as failing on
mainand was one step from reporting main red. It was not: the working tree I measured in was being mutated concurrently by another actor, and a leftover line in it produced the finding. Re-run against a pristine, isolated checkout oforigin/main, the guard exits 0 —guard clean (22745 tracked + packed file(s) scanned, 2 symlink(s) skipped)— andorigin/main's own README blob carries zero convention-shaped matches. Main is green. A shared worktree is not a measurement surface.Refs: todos
38d15243Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.