Skip to content

fix(git-hooks): commits refuse when the secret check could not run - #183

Merged
chuycepeda merged 2 commits into
The-AIOS:mainfrom
matiasmacera:fix/commit-scan-fail-closed
Sep 25, 2026
Merged

chuycepeda merged 2 commits into
The-AIOS:mainfrom
matiasmacera:fix/commit-scan-fail-closed

Conversation

@matiasmacera

Copy link
Copy Markdown
Contributor

What

Four ways a commit went through with a check that never ran. All four are silent, and each one either lets a secret into history or reports "nothing to commit" when the command could not tell.

# Where What happened
1 hooks/aios-commit The self-scan covered the committed paths but not the commit message, so a token pasted into -m went into history.
2 hooks/aios-commit, hooks/git/pre-commit Both guarded the scan with [ -x "$SCAN" ] &&. A missing scanner, or one that had lost its +x bit (a zip, some Windows clones), meant an unscanned commit.
3 hooks/git/secret-scan.sh grep -I treats a file with a single NUL byte as binary and does not look inside it. A token next to a NUL passed as clean, and git committed the bytes anyway.
4 hooks/aios-commit --vault The sweep's git diff and git ls-files wrote their errors to /dev/null, and an empty result means "nothing to commit". A failed diff exited 0 with nothing committed, blaming a desynced index. A failed ls-files dropped new files silently.

Fix

  1. The message is written to a temp file and scanned with the paths; a hit is reported as the commit message. The temp file is removed on both outcomes.
  2. A missing scanner refuses the commit in both hooks. A scanner without +x runs through bash instead of being skipped.
  3. grep -a instead of -I.
  4. Each sweep call that fails emits a sentinel no real path can equal ($'\x01aios-sweep-failed'). If it appears, the commit is refused and the reason is named. Git's own error stays visible because the redirect is gone.

Proof

tests/commit-scan-fail-closed.test.sh (new, wired into validate.yml) rebuilds the hooks from a pinned pre-change commit and reproduces each defect against them. It then shows the current hooks refuse the same input.

  • The old hooks come from a fixed sha, never a moving main. After this merges, main's hooks are the new ones, and asserting they still show the defects would fail a correct tree.
  • If that commit is missing, or its hooks equal the current ones, the old-side reproductions skip with a message. The new-side checks still gate.
  • Case 4 uses a git shim on PATH, not file permissions. The shim fails exactly one sweep call (diff or ls-files) and delegates everything else. That makes each sentinel's test independent, and it runs as root and on Windows.
  • A scanner without +x is covered in both aios-commit and pre-commit. Reverting pre-commit to [ -x ] && exec …; exit 0 was checked to fail the suite.

19 pass under bash 3.2 and 5.3. The existing aios-commit (30) and secret-scan (28) suites are unchanged and pass, as does the full tests/ run.

Not in this PR

Two items from the same hooks audit need the lock, so they get their own PR: a rename whose destination was staged leaves both names in HEAD, and the push-pending marker is written outside the lock.

Scope

hooks/aios-commit, hooks/git/pre-commit, hooks/git/secret-scan.sh, the test, and the CHANGELOG entry. Follow-up to #157 in the series announced in #158.

🤖 Generated with Claude Code

- aios-commit scanned the committed paths but not the commit message;
  a token in -m went into history. The message is now scanned too.
- aios-commit and pre-commit guarded the scan with `[ -x "$SCAN" ] &&`:
  a missing scanner, or one that lost +x, meant an unscanned commit.
  A missing scanner now refuses; one without +x runs through bash.
- secret-scan.sh used `grep -I`, which skips a file with a NUL byte;
  a token next to one passed. Now `grep -a`.
- aios-commit --vault discarded the sweep's git errors, and "no paths"
  read as "nothing to commit". Each failing sweep call now leaves a
  sentinel that refuses the commit and names why.

tests/commit-scan-fail-closed.test.sh reproduces each defect against
hooks rebuilt from a pinned pre-change commit (skipped, with a message,
when that commit is absent) and uses a PATH git shim to fail one sweep
call at a time.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

AIOS-Session: 4ce35e79-7182-45d3-a479-d25e0004fda0
AIOS-Session: 4ce35e79-7182-45d3-a479-d25e0004fda0
@chuycepeda
chuycepeda merged commit c2f570e into The-AIOS:main Sep 25, 2026
16 checks passed
@chuycepeda

Copy link
Copy Markdown
Member

Shipped in v0.8.5 with your commits and authorship intact. Thank you. Added on top (4083b6c):

  • --vault in a repo with no commits now reads the staged paths; it used to refuse the first commit.
  • -a made a 200 MB attachment take ~12 s. Now one grep pass with every pattern, and files over 20 MB are scanned as text and named in the output: 0.4 s.
  • A pre-existing SIGPIPE: | head -3 exited early once grep's output overflowed the pipe buffer, so a real hit read as 'FAILED to scan' (exit 141). Output now goes through cut.

matiasmacera pushed a commit to matiasmacera/aios-contrib that referenced this pull request Sep 25, 2026
…ks in an empty repo

On top of The-AIOS#183.
- `--vault` in a repo with no commits yet: `git diff HEAD` has no HEAD,
  so the sweep read as failed and the first commit refused. It now reads
  the staged paths there.
- `-a` reads a binary in full: a 200 MB attachment took ~12 s (main
  0.04 s). One grep pass carries every pattern, and files over 20 MB
  (AIOS_SECRET_SCAN_CAP_MB) are scanned as text only and named in the
  output, so the gap is visible. Now 0.38 s.
- Pre-existing: `| head -3` exited early, and once grep's output
  overflowed the pipe buffer grep died of SIGPIPE (141) under pipefail,
  so a real hit read as "FAILED to scan". Output now goes through `cut`,
  which reads it all and caps each reported line.
- Tests: the empty repo, the size cap, and ~1.3 MB of hits (fails on
  main with exit 141).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LCjeYxWVWosFKnRiCf8Ae7
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.

2 participants