pre-commit-loop-detector: hook runs twice on non-loop failure, capture markers leak to stderr, JSONL repo field unescaped #26

New issue

Closed

opened 2026-07-31 05:55:49 +00:00 by Mark · 1 comment

Mark commented

2026-07-31 05:55:49 +00:00

Owner

Summary

Minor hardening findings in scripts/hermes/pre-commit-loop-detector.sh (introduced by #23 fix, commit 2e07772). Found by post-deploy probe (kanban card t_5ecf9aa6).

Actual behavior

Hook runs twice on any failed commit. run_commit_with_capture "$@" runs git commit (line ~165) and discards its captured output, then the wrapper re-runs git commit "$@" at lines 177-181 before loop detection. A non-loop failure (hook exits 1, no loop signature) therefore executes hooks twice.
Capture markers leak to stderr. run_commit_with_capture echoes :::HERMES_COMMIT_CAPTURED_START::: / :::HERMES_COMMIT_CAPTURED_END::: / :::HERMES_COMMIT_RC::<rc>::: to stderr. These are consumed nowhere by the caller (the captured output is discarded) and appear in user-visible stderr.
JSONL repo field is unescaped. log_fallback embeds the repo identifier raw into the JSONL line ("repo":"%s"), unlike the command field which is python-escaped. A repo name containing quotes/backslashes would corrupt the log line.

Expected behavior

A commit attempt should run hooks exactly once per attempt (no duplicate hook execution on the plain non-loop failure path).
Internal capture markers should not leak to user-visible stderr.
All JSONL fields should be JSON-escaped consistently.

Reproduction steps

Set up a repo with a pre-commit hook that exits 1 (no loop signature) via the wrapper.
Run git commit through the wrapper.
Observe the hook fires twice and :::HERMES_COMMIT_CAPTURED_*::: markers appear in stderr.

Impact

Low — correctness of the wrapper's happy-path detection is unaffected, but duplicate hook execution can mislead hook authors and the marker leak pollutes output. Follow-on hardening candidate.

Filed by reporter lane from kanban card t_5ecf9aa6 (post-deploy probe of #23).

## Summary Minor hardening findings in `scripts/hermes/pre-commit-loop-detector.sh` (introduced by #23 fix, commit `2e07772`). Found by post-deploy probe (kanban card `t_5ecf9aa6`). ## Actual behavior 1. **Hook runs twice on any failed commit.** `run_commit_with_capture "$@"` runs `git commit` (line ~165) and discards its captured output, then the wrapper re-runs `git commit "$@"` at lines 177-181 *before* loop detection. A non-loop failure (hook exits 1, no loop signature) therefore executes hooks **twice**. 2. **Capture markers leak to stderr.** `run_commit_with_capture` echoes `:::HERMES_COMMIT_CAPTURED_START:::` / `:::HERMES_COMMIT_CAPTURED_END:::` / `:::HERMES_COMMIT_RC::<rc>:::` to stderr. These are consumed nowhere by the caller (the captured output is discarded) and appear in user-visible stderr. 3. **JSONL `repo` field is unescaped.** `log_fallback` embeds the repo identifier raw into the JSONL line (`"repo":"%s"`), unlike the `command` field which is python-escaped. A repo name containing quotes/backslashes would corrupt the log line. ## Expected behavior - A commit attempt should run hooks exactly once per attempt (no duplicate hook execution on the plain non-loop failure path). - Internal capture markers should not leak to user-visible stderr. - All JSONL fields should be JSON-escaped consistently. ## Reproduction steps 1. Set up a repo with a pre-commit hook that exits 1 (no loop signature) via the wrapper. 2. Run `git commit` through the wrapper. 3. Observe the hook fires twice and `:::HERMES_COMMIT_CAPTURED_*:::` markers appear in stderr. ## Impact Low — correctness of the wrapper's happy-path detection is unaffected, but duplicate hook execution can mislead hook authors and the marker leak pollutes output. Follow-on hardening candidate. --- Filed by reporter lane from kanban card `t_5ecf9aa6` (post-deploy probe of #23).

Mark added the

bug

label

2026-07-31 05:55:49 +00:00

Mark referenced this issue from a commit

2026-08-11 09:52:41 +00:00

fix(hooks,tests): loop-detector hardening + relax over-strict .next test (#26, #27)

Mark commented

2026-08-11 09:54:21 +00:00

Author

Owner

All three findings fixed on main by commit 07599d0:

Hook double-run: run_commit_with_capture now tees the first attempt's output to a capture file and the caller uses THAT output for loop detection — the wrapper no longer re-invokes git commit just to inspect output, so a plain non-loop failure runs hooks exactly once.
Marker leak: removed the :::HERMES_COMMIT_CAPTURED_*::: stderr echoes entirely (the captured output lives in a temp file, not stderr).
JSONL escaping: the repo field is now Python-json-escaped (same as command) instead of being spliced raw.

Added regression test test_non_loop_failure_runs_hooks_once_no_marker_leak asserting hook count == 1 and no :::HERMES_COMMIT_CAPTURED in stderr. 9/9 pytest pass.

All three findings fixed on main by commit 07599d0: 1. **Hook double-run**: `run_commit_with_capture` now tees the first attempt's output to a capture file and the caller uses THAT output for loop detection — the wrapper no longer re-invokes `git commit` just to inspect output, so a plain non-loop failure runs hooks exactly once. 2. **Marker leak**: removed the `:::HERMES_COMMIT_CAPTURED_*:::` stderr echoes entirely (the captured output lives in a temp file, not stderr). 3. **JSONL escaping**: the `repo` field is now Python-json-escaped (same as `command`) instead of being spliced raw. Added regression test `test_non_loop_failure_runs_hooks_once_no_marker_leak` asserting hook count == 1 and no `:::HERMES_COMMIT_CAPTURED` in stderr. 9/9 pytest pass.

Mark closed this issue

2026-08-11 09:54:21 +00:00

Mark referenced this issue

2026-08-11 09:54:36 +00:00

Pre-commit loop-detector fix (#23) never deployed — wrapper not installed, kilo-gsd/quickfix still run plain git commit #24

Mark referenced this issue

2026-09-18 23:25:51 +00:00

Pre-commit loop-detector fix (#23) never deployed — wrapper not installed, kilo-gsd/quickfix still run plain git commit #24