feat(launchpad): assert ignore coverage, tracked files, agent config surface (#68) - #275
Conversation
…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.
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>
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.