Skip to content

GTM-1590: scan renamed files and first-party vendor/, and state the denominator - #5

Merged
scott-wueschinski-GTMify merged 3 commits into
mainfrom
worktree-gtm1590-secret-scan-renames-vendor
Aug 31, 2026
Merged

GTM-1590: scan renamed files and first-party vendor/, and state the denominator#5
scott-wueschinski-GTMify merged 3 commits into
mainfrom
worktree-gtm1590-secret-scan-renames-vendor

Conversation

@scott-wueschinski-GTMify

@scott-wueschinski-GTMify scott-wueschinski-GTMify commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Closes GTM-1590.

The pre-commit secret scan passed while reading a fraction of what was staged. It reported that fraction honestly, and the honesty changed nothing: both shortfalls were in plain text and were read past. This is the gate the estate relies on after the config-repo compromise that put 16 credentials into rotation, so a hole in it is a hole in the thing that was supposed to catch that.

Premise re-verified before building

Reproduced from scratch on 2026-08-31, in a throwaway repo, against the unmodified gate. A git mv plus an edit in one commit, carrying a synthetic AWS-shaped value:

>> secret scan passed. 0 path(s) checked, mode=staged.

And a modification under vendor/, same result. Then end to end through the real global pre-commit hook, with a GTMify-owned origin so the hook's allowlist applied, which is an exact reproduction of Scott's c5925f8 measurement:

>> junk-file gate passed. 0 path(s) checked, mode=staged.
>> secret scan passed. 0 path(s) checked, mode=staged.
[main f8dddc4] retire the register entry
 1 file changed, 1 insertion(+)
 rename register.md => register_retired.md (51%)

The commit landed with the synthetic value in it. The ticket is accurate and was not already fixed.

What changed

Cause 1, renamed files. --diff-filter=ACM becomes ACMRT. Excluding D is right, since a deleted path has no content left to scan; excluding R was wrong, because git classifies an edited-and-moved file as R once similarity clears the threshold and the content still ships. T joins for the same reason: a symlink becoming a regular file is new content arriving. --name-only prints a rename's destination and not its source, so the counts stay comparable.

Cause 2, vendor/. Removed from the unconditional skip list and qualified instead: skipped only where a package manager actually marked the tree vendored, meaning vendor/modules.txt (Go) or vendor/autoload.php (Composer). Neither marker exists anywhere in this estate, so gtmify/app/vendor is now scanned, and a repo that genuinely vendors its dependencies still gets the skip without needing to know this gate exists. This is the ticket's second option; I did not take the first, because dropping the name and leaning on .secret-scan-allow would have meant allowlisting whole documentation files forever.

The verdict states its denominator. N of M path(s) checked, with every skip enumerated by reason, plus a warning if the parts do not add up to the total. Refusals now print coverage too, because knowing the gate found something still says nothing about how much of the commit it read.

>> secret scan passed. 2 of 5 path(s) checked, mode=staged.
>>   skipped: 1 deleted, 1 dependency or build directory, 1 encrypted or binary file type

The part the ticket did not anticipate, and the measurement behind it

Un-skipping vendor/ exposed the gate to twenty trees of vendor documentation, and it fired on 17 lines in gtmify/app/vendor, every one of them documentation. Shipping that unaddressed would have earned this gate a permanent --no-verify, which is a worse outcome than the hole it closes. Four narrow refinements take 17 to 1, each pinned by a test:

Class Measured Refinement
Stripe's own published sample key, in 8 files of stripe-docs 8 Exact literal, never a shape
Stripe refund object ids, which match the Resend re_ detector 5 Guard on values of a JSON "id" field, a place a credential is never legitimately assigned
Secret-named variables assigned from process.env and friends 4 Lookahead. Reading a secret from the environment is the correct handling of one, not a leak of one
Documented placeholder values (replace-with-..., asdfasdf, tr_preview_1234567890) 3 Four terms added to the existing PLACEHOLDER list

None of these widens the gate against a value that is actually a credential. Counts overlap because some lines matched two detectors.

🔴 The one remaining hit needs a one-line follow-up in the APP repo, which I could not make from here. vendor/triggerdev-docs/pages/self-hosting/docker.md:192 carries TRIGGER_WORKER_TOKEN=tr_wgt_..., which I read in context: it is a verbatim mirror of Trigger.dev's public self-hosting docs showing example console output, not a GTMify credential. I deliberately did not add tr_wgt_ to any placeholder list, because that is a real Trigger.dev token shape and a real one could leak. The app repo has no .secret-scan-allow at all today, so once this merges, a commit staging that one file will be refused. Remedy, in GTMify/GTMify:

vendor/triggerdev-docs/pages/self-hosting/docker.md  # Vendor's own published sample worker token in mirrored self-hosting docs. GTM-1590.

No other app path is affected, and no commit that does not touch that file is affected.

Proof, both directions

A gate that fires on everything is as broken as one that fires on nothing, so both directions are shown.

Refuses what it used to miss. Same staged index, two gates:

OLD  >> secret scan passed. 0 path(s) checked, mode=staged.          exit=0
NEW  >> SECRET SCAN FAILED. ... renamed_target.md:9  aws access key id
     Coverage: 1 of 1 path(s) in scope were read.                    exit=1

Same for vendor/demo-docs/api.md. End to end through the real pre-commit hook, GTMIFY_CI_WORKFLOWS pointed at this branch, the hook refused the git mv commit naming the detector.

Still passes ordinary work. The same hook, same branch, a clean commit including a vendor/ path:

>> junk-file gate passed. 2 path(s) checked, mode=staged.
>> secret scan passed. 2 of 2 path(s) checked, mode=staged.
[main bad01f3] ordinary clean work

Deliberate-failure cycle on the new suite, 22 cases, each defect reintroduced then restored:

Defect reintroduced Suite result Restored
--diff-filter=ACMRT back to ACM 20 passed, 2 failed, both cause-1 assertions 22 passed, 0 failed
vendor/ back in SKIP_DIRS 13 passed, 9 failed 22 passed, 0 failed
Summary back to a bare N path(s) checked 13 passed, 9 failed 22 passed, 0 failed
Sample-key exemption mutated 21 passed, 1 failed 22 passed, 0 failed

No test was accepted without being observed to fail. Grepped for DELIBERATE-FAILURE DRILL afterwards: no matches. shellcheck --severity=warning clean on both files, matching what CI runs.

self-test.yml gains a restore-and-go-red step for each cause, not one for both, plus an audit of this repo by its own secret scan.

Two things worth a reviewer's attention

This gate now scans its own source, and that cost something. The installed gate refused my first commit, because KNOWN_PUBLIC_SAMPLES held the Stripe literal, and GitHub push protection then refused the push over the same literal in a test fixture. I did not use --no-verify and did not click the allow-secret URL. Both are written as prefix plus body instead, split exactly at the boundary the detector matches, so the runtime comparison is unchanged and the literal is absent from the tree. The alternative was allowlisting scripts/secret_scan_gate.sh, which would have made the one file where a credential must never hide the one file nobody reads.

.secret-scan-allow is new in this repo and has exactly one entry, tests/secret_scan_gate_test.sh. A scanner's test suite has to contain strings matching every shape the scanner detects, including a PEM private key header, or it proves nothing. Every value in that file is synthetic.

Out of scope, filed here so it is not lost: junk_file_gate.sh:152 has cause 1 too, --diff-filter=AM, which is why the junk gate also printed 0 path(s) checked on the rename commit above. It is a different gate reading names rather than content, so I left it alone rather than widening this PR.

Gate

advisor() before the first edit: answered by the primary, cx-consult (Codex), read against the tree. It caught that my end-to-end proof would silently exercise the unchanged gate in the main clone unless I set GTMIFY_CI_WORKFLOWS and used a GTMify-owned origin, which is why the hook evidence above is real rather than accidental.

⚠️ advisor() at completion fell back: the primary was unavailable, and the verdict came from or-consult, x-ai/grok-4.6, which has no file access and saw only a condensed transcript. Its two substantive points are both addressed above and both were worth having: classify the last remaining app-repo hit rather than leaving it counted (done, with the follow-up named), and drill the vendor cause in CI rather than only the rename cause (added as a second commit). A reviewer should know the completion pass reviewed the plan and not the code.

Lint baseline: not touched. This repo has none.

🤖 Generated with Claude Code

Added after the first CI run: this gate had never worked on Linux

The new suite's first run on ubuntu-latest came back 1 passed, 21 failed, and the cause was pre-existing rather than anything in this change. mktemp -t ssg_paths is a BSD spelling: macOS treats the argument as a prefix and appends its own randomness, GNU coreutils treats it as a template and refuses with too few X's in template. $SSG_LIST was therefore empty on every Linux host, python was handed an empty path, and the gate died with IsADirectoryError: Is a directory: '.'.

It failed closed, so nothing was ever let through by it, and everything on this estate runs on Scott's Mac, which is why it went unseen. But a gate that refuses every commit on a platform is a gate that gets uninstalled there, and this PR is the first thing ever to run the secret scan in CI at all. Fixed with an explicit XXXXXX template, which both implementations accept, plus an explicit error instead of a traceback if the temp file cannot be made.

⚠️ The same BSD spelling is in the pre-commit hook itself, at ~/.claude/hooks/git/pre-commit:166, mktemp -t register_staged, in the vendor-register drift check. That file lives in gtmify-config rather than here so I did not touch it. It is guarded by if git show ... >"$staged_blob", so on Linux it warns rather than crashing, but it is the same latent defect and worth a follow-up.

CI evidence

Test and lint the gate scripts is green on 8dbfaa0, and the log shows the drills firing rather than just a green tick:

  ok    rename_plus_edit_is_scanned                expected fail got fail
  ok    vendor_path_is_scanned                     expected fail got fail
  22 passed, 0 failed
>> Mutation correctly turned the suite red.        (rename cause)
>> Restored gate is green again.
>> Mutation correctly turned the suite red.        (vendor cause)
>> Restored gate is green again.
>> secret scan passed. 13 of 14 path(s) checked, mode=audit (value layer unavailable: no local env file).

Denominators: 22 in the new suite, 35 in the junk-gate suite, 8 in the frontmatter suite, all pre-existing counts unchanged. The audit line correctly reports the value layer as unavailable in CI, where there is no env file, which is the documented behaviour.

…enominator

The pre-commit secret scan passed while reading a fraction of what was staged,
and reported that fraction honestly enough that nobody read it. Two causes.

Renamed files were never scanned. --diff-filter=ACM excluded R, and git
classifies an edited-and-moved file as R once similarity clears the threshold,
so the ordinary motion of editing a file and moving it in one commit rode
through unscanned. Measured on gtmify-config: ba71968 staged four renames and
one modification and reported "1 path(s) checked"; c5925f8 was a git mv plus an
edit and reported "0 path(s) checked" from both gates. Reproduced from scratch
before this change. The filter is now ACMRT, so renames and typechanges are
read and only deletions are excluded, which is correct because a deleted path
has no content left to scan.

Everything under vendor/ was skipped by name. That is the dependency-directory
convention, and in this estate it is false: gtmify/app/vendor is the vendor
documentation mirror, twenty first-party trees that sessions actively author,
and a worked API example is where a live key gets pasted. vendor/ is now
qualified rather than named, skipped only when a package manager actually
marked the tree vendored (vendor/modules.txt for Go, vendor/autoload.php for
Composer). Neither marker exists anywhere in this estate.

The verdict now carries its denominator. "N of M path(s) checked" with every
skip enumerated by reason, and a warning when the parts do not add up to the
total. A bare "0 path(s) checked" was read past twice in two days; "0 of 1
checked, 1 skipped: 1 deleted" cannot be.

Un-skipping vendor/ first exposed the gate to seventeen false-positive lines in
gtmify/app/vendor, every one of them documentation. Four narrow, measured
refinements take that to one: Stripe's own published sample key as an exact
literal, a guard for JSON object ids whose prefix collides with a credential
prefix (a Stripe refund id matches the Resend detector), four placeholder terms
the documents use to say the value is not real, and a lookahead so that
assigning a secret-named variable FROM process.env stops reading as a leak,
since that is the correct handling of a secret rather than a leak of one. None
of them widens the gate against a value that is actually a credential.

tests/secret_scan_gate_test.sh is new, 22 cases. Both causes are pinned as
regressions that fail before this change, every false-positive class above is
pinned as a MUST PASS so the gate cannot drift into refusing everything, and
the legitimate skips are asserted by count rather than by absence of an error.
self-test.yml gains a mutation step that restores the ACM filter and requires
the suite to go red, plus an audit of this repo by its own secret scan.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The self-test mutated only --diff-filter=ACM, which proves the suite can see
cause 1 and says nothing about cause 2. The vendor skip is the half that reaches
first-party content, so it gets its own restore-and-go-red step rather than
riding on the rename drill. Verified locally: reinstating vendor/ in SKIP_DIRS
turns 9 of 22 assertions red.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…suite

`mktemp -t ssg_paths` is a BSD spelling. macOS treats the argument as a prefix
and appends its own randomness; GNU coreutils treats it as a template and
refuses with "too few X's in template". The gate had only ever been run on
macOS, so on any Linux host it produced an empty SSG_LIST, handed python an
empty path, and died with IsADirectoryError.

It failed closed, so nothing was ever let through by it, but a gate that
refuses every commit on a platform is a gate that gets uninstalled there. An
explicit XXXXXX template works on both, and an empty result is now an explicit
error rather than a traceback.

Pre-existing, not introduced here. It surfaced because this branch is the first
thing to run the secret scan in CI at all: 1 passed, 21 failed on the first run
of the new suite on ubuntu-latest.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@scott-wueschinski-GTMify
scott-wueschinski-GTMify merged commit 94d04d8 into main Aug 31, 2026
1 check passed
@scott-wueschinski-GTMify
scott-wueschinski-GTMify deleted the worktree-gtm1590-secret-scan-renames-vendor branch August 31, 2026 08:07
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.

1 participant