fix(guards): fold shell continuations in the hooksPath guard via one shared helper (#15961)

[mrveiss]

Refs #15961

Single-issue rationale: this addresses one named gap in #15961, the shell-continuation escape in its static guard, by extracting the helper #16128 introduced. #15961 stays open: AC1 (runtime detection of a transient -c core.hooksPath= override) is a separate mechanism.

Thinking Path

repo_tests/hooks_path_override_15961_test.py matched physical lines, so a shell \ continuation hid the most harmful spelling, the persistent override:

git config \
  core.hooksPath /tmp/x        -> escaped the guard

#16128 fixed the same defect in the sibling guard, git_merge_rejects_pull_only_flags_15938_test.py, with a _logical_lines() helper. The plan recorded on #15961 was one helper, not a second copy: pasting the fold into this guard would give base two implementations of one rule, and they would drift.

What Changed

• Review fix: logical_lines no longer folds out of a whole-line comment. Bash ends a comment at the newline, so a trailing \ inside one is text. Folding it glued the next line behind a #, and _offending_lines skips comment lines, so an override written straight after such a comment went unreported. The old physical-line scan caught it. Regression cases are in _scan_helpers_test.py and in the guard's own catch list.

• New tools/lint/_scan_helpers.py logical_lines(): folds backslash-newline continuations and reports each command's first physical line number. It has its own unit tests in tools/lint/_scan_helpers_test.py, covering no continuation, two lines, three lines, a mid-line backslash that isn't folded, and the reported line number.

• hooks_path_override_15961_test.py scans logical lines. New fixtures cover both directions:

  • git config \ + core.hooksPath /x is caught.
  • -c split from core.hooksPath= across lines is caught.
  • git config \ + --get core.hooksPath stays allowed.

• git_merge_rejects_pull_only_flags_15938_test.py imports the shared helper in place of its private copy. The algorithm is byte-identical, so its behaviour is unchanged.

• repo_tests/slm_frontend_shell_publish_test.py keeps its own _logical_lines. It has different semantics: it returns strings without line numbers, and strips each continued line before joining. Moving it onto the shared shape would change its behaviour, not just remove a duplicate. The difference is documented in its docstring.

• The helper's docstring and its unit tests use neutral tokens (some-tool … --dangerous-flag). The hooksPath guard scans every tracked file, the helper's own files included, so a real offending spelling there tripped it twice at pre-push. The real spellings stay only in the guards' own fixtures.

Verification

• Before any edit, the matcher was checked by hand against the five continuation cases, using a copy run outside the repo.
• py_compile and ast.parse pass on every touched file, and the full pre-commit suite passed at commit.
• core.hooksPath override silently disables every hook in a worktree #15961 AC status, checked against base:
  • AC2 (the worktree case): already on base in test_a_relative_hookspath_defeats_hooks_in_a_worktree.
  • AC4 (the remediation message): already on base.
  • AC5 (docs beside the --no-verify rule): already on base, in CLAUDE_RULES.md and CLAUDE_GIT.md.
  • AC1 (runtime detection) is not delivered, and no tracked file reads GIT_CONFIG_PARAMETERS.
  • AC3 (named actor exemptions) doesn't apply to a static sweep.
  • The issue stays open for AC1.

Model Used

Claude Opus 5 (coordinator) with a Claude Sonnet 5 subagent for the implementation.

🤖 Generated with Claude Code

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 058432b9-acbc-46cf-a828-00cf62b88f82

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[mrveiss]

Review of bffba5b97: approve. 1 commit (verify), 0 trailers, 0 behind, includes #16300. Refs #15961 with a single-issue rationale that correctly scopes this to one named gap and leaves AC1 (runtime detection) open — matches the batching-check requirement and doesn't overclaim.

Verified: behavior-preserving extraction, not a rewrite. The #15938 guard's diff is pure deletion of its private _logical_lines plus an import swap to the shared tools.lint._scan_helpers.logical_lines — same docstring content moved wholesale, same fold algorithm. Both guards now import the identical function (hooks_path_override_15961_test.py:65, git_merge_rejects_pull_only_flags_15938_test.py:35), so there's exactly one fold implementation to get right instead of two that can drift apart, which is the whole point of #16128's original finding recurring in #15961.

Verified: the helper itself is well-tested, including the case that actually matters (_scan_helpers_test.py:238 test_logical_lines_does_not_fold_a_mid_line_backslash — a fold that's too eager is as wrong as one that's too narrow) and first-line attribution (:246).

Verified: both directions covered for the consuming guard. hooks_path_override_15961_test.py:235 test_a_continuation_does_not_hide_the_flag and :248 test_a_continuation_in_valid_usage_is_still_left_alone — a continuation that should trip the guard, and one that shouldn't.

Verified: the guard's own dogfooding note is accurate. _scan_helpers.py:288-290 deliberately keeps its docstring example neutral ("the guards that call this scan tracked files, this one included, so a real offending spelling here would trip them") — I checked this is true: both consuming guards scan the guard/helper source tree itself, so a literal dangerous-flag example in this docstring would have tripped its own guard. That's why the pre-push hook needed a third try per the coordinator's note, and the fix (neutral tokens) is the right one, not a suppression.

Nothing else found.

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

[github-actions]

Notice: 29 open PRs — past the runaway threshold (25)

There is no PR queue limit, and this is not a request to defer this PR. Work proceeds one issue at a time without a cap on open PRs; review capacity is the constraint.

This notice only means the count is high enough to be worth a glance for a runaway — something opening PRs in a loop, or a merge pipeline that has stalled so nothing is draining.

Currently open:

• fix(security): deny an SLM token when its revocation check cannot run, HS256 and RS256 (#16387, #16412) #16413 fix(security): deny an SLM token when its revocation check cannot run, HS256 and RS256 (security(slm-auth): token revocation checks fail open when Redis errors, so a revoked token is accepted #16387, security(slm-auth): RS256 authority-token revocation check fails open when Redis errors #16412)
• fix(security): generate the vault root key from 32 random bytes, not alphanumeric characters (#16405) #16410 fix(security): generate the vault root key from 32 random bytes, not alphanumeric characters (security(secrets): the vault root key is base64 of 32 alphanumeric chars (~190 bits), not 32 random bytes #16405)
• chore(docker): retire the four agent Dockerfiles whose modules no longer exist; agents run in-process (#16361) #16409 chore(docker): retire the four agent Dockerfiles whose modules no longer exist; agents run in-process (chore(docker): four agent Dockerfiles under autobot-infrastructure/autobot-backend/docker/ are built by nothing: wire in or retire #16361)
• chore(deps): carry dependabot's npm security bumps onto Dev_new_gui (hono 4.13.7 and transitive; supersedes #16171) #16408 chore(deps): carry dependabot's npm security bumps onto Dev_new_gui (hono 4.13.7 and transitive; supersedes chore(deps): bump the npm_and_yarn group across 6 directories with 7 updates #16171)
• fix(tests): stop gating a disk-bound model load on a CPU work budget, and prove it stays offline and import-free (#15055, #15235) #16404 fix(tests): stop gating a disk-bound model load on a CPU work budget, and prove it stays offline and import-free (test(perf): startup benchmarks assert absolute wall-clock budgets and fail on runner load — passed and failed on the same code minutes apart #15055, test(perf): three calibrated budgets are drifting or disk-bound — the startup margin is 1.4x, not the 1.9x its note records #15235)
• fix(ci): keep the drift-checked SLM agent mirror out of the duplication scan (#16401) #16402 fix(ci): keep the drift-checked SLM agent mirror out of the duplication scan (ci(duplication-guard): the SLM scope counts the drift-checked agent mirror, so every agent change fails the absolute pin #16401)
• fix(ci): bring CI's pytest environment up to its declared floors, and gate on them (#16264) #16391 fix(ci): bring CI's pytest environment up to its declared floors, and gate on them (ci: the pytest environment sits below two declared floors, and the floor banner tells CI readers the opposite #16264)
• fix(security): the dev proxy stops granting admin without a login, and the session check endpoint is tested (#16382, #16385) #16390 fix(security): the dev proxy stops granting admin without a login, and the session check endpoint is tested (SLM Vite dev-server proxy injects X-Internal-API-Key on /autobot-api with no session check #16382, SLM backend: add direct test coverage for GET /api/auth/me #16385)
• fix(security): the SLM proxy checks the caller's SLM session before it adds the backend key (#16374) #16386 fix(security): the SLM proxy checks the caller's SLM session before it adds the backend key (security(slm): the SLM nginx /autobot-api/ proxy adds the internal API key for every caller, so anyone reaching the SLM host is a backend admin #16374)
• fix(ci): retire the CodeQL suite file nothing loads, and record the real guard behind alert #1030 (#16283, #16302) #16377 fix(ci): retire the CodeQL suite file nothing loads, and record the real guard behind alert feat: Voice conversation mode — Hands-free with VAD (Tier 2) #1030 (ci(security): codeql.yml is disabled, so the reviewed codeql-config.yml exclusions apply to nothing #16283, ci(security): codeql-config.yml's file-scoped query-filters exclude nothing, under either setup #16302)
• fix(security): write the service-key export only to its configured directory, never into a checkout, and keep one (#16348) #16372 fix(security): write the service-key export only to its configured directory, never into a checkout, and keep one (security(service-auth): generate_service_keys.py writes its live-key export relative to the working directory, so a run inside a checkout drops keys into the work tree #16348)
• fix(security): authenticate every skills route, and require admin to change, fetch or execute (#16368) #16370 fix(security): authenticate every skills route, and require admin to change, fetch or execute (security(api): every route in api/skills.py (catalog fetch/install, enable, execute) is unauthenticated, and the router-auth guards can't see feature routers #16368)
• fix(ci): let the system-package retry recover from its own timeout, and fetch before unpacking (#15515) #16367 fix(ci): let the system-package retry recover from its own timeout, and fetch before unpacking (ci(runner): apt provisioning times out (exit 124) three attempts in a row and fails the shard before any test runs #15515)
• fix(guards): tell a baseline entry that matches nothing apart from one that under-matches (#16334) #16365 fix(guards): tell a baseline entry that matches nothing apart from one that under-matches (bug(guards): --audit-baseline says "no longer match anything" for an entry that still matches, so the reader deletes a live exemption #16334)
• fix(ci): stop the regen bot's approved runs cancelling the job that approved them (#16360) #16363 fix(ci): stop the regen bot's approved runs cancelling the job that approved them (ci: the regen bot's own push cancels the job that approves its parked runs, so a PR's CI can silently never run #16360)
• fix(ci): keep line numbers out of the secrets baseline, so a line move no longer rewrites it (#16353) #16362 fix(ci): keep line numbers out of the secrets baseline, so a line move no longer rewrites it (ci(secrets): .secrets.baseline stores a line number per finding, so every PR that shifts lines in a file with findings rewrites it and conflicts with every other such PR #16353)
• test(guards): name the deliberate fixed sleeps, and stop a random temp name deciding a language test (#16255, #16341) #16358 test(guards): name the deliberate fixed sleeps, and stop a random temp name deciding a language test (tests: 32 more fixed sleeps are followed by an assertion, and some decide the outcome by the clock #16255, test(voice): test_no_filename_hint flakes when tempfile's random name forms a language-code token (e.g. _lv_) #16341)
• fix(ci): gate the frontend npm audit on one report, retry it, and report 'could not check' apart from advisories (#16337) #16357 fix(ci): gate the frontend npm audit on one report, retry it, and report 'could not check' apart from advisories (ci(security): the npm audit gate fails an audit-endpoint error exactly like a high advisory — retry, and report 'could not check' distinctly #16337)
• fix(docker): retry apt fetches and fail apt-get update on a lost index, set once for every image (#16291) #16356 fix(docker): retry apt fetches and fail apt-get update on a lost index, set once for every image (ci(docker): an unreachable Ubuntu mirror surfaces as a misleading 'held broken packages' error; apt steps neither retry nor fail on index errors #16291)
• feat(slm): Redis control and detailed health live in the SLM only; remove the unreachable user-GUI copies (#16245, #16252) #16352 feat(slm): Redis control and detailed health live in the SLM only; remove the unreachable user-GUI copies (tech-debt(frontend): move SettingsPanel's detailed system health and Redis service management into the SLM #16245, bug(frontend): RedisServiceControl calls /api/service-monitor/services/* and no mounted router serves those paths #16252)
• fix(code-sync): delete files removed from source through the updater, and check drift across every file (#16310, #16322) #16351 fix(code-sync): delete files removed from source through the updater, and check drift across every file (deploy: the builtin updater never removes files deleted from source, so deleted modules pile up on every host #16310, deploy: ansible synchronize never deletes files removed from source on remote fleet nodes #16322)
• fix(pre-commit): run detect-secrets serially and refresh the baseline's line numbers (#16343) #16349 fix(pre-commit): run detect-secrets serially and refresh the baseline's line numbers (pre-commit: detect-secrets runs in parallel batches that race on .secrets.baseline, so large commits and base merges can't commit #16343)
• fix(kb): make three summarisation paths do what they claim (#14840, #15700, #16110) #16345 fix(kb): make three summarisation paths do what they claim (context-overflow summarization can never succeed — _get_gateway imports a module and a symbol that do not exist #14840, sec(kb): the chat-to-knowledge summariser interpolates whole transcripts unframed, and the result is persisted to the knowledge base #15700, bug(kb): summary quality is scored against text[:200] — a 300-word summary judged on a 200-character window #16110)
• ci(python-suite): queue the shards first in, first out across PRs, with a per-shard job-level queue: max group (#16320) #16328 ci(python-suite): queue the shards first in, first out across PRs, with a per-shard job-level queue: max group (ci: queue python-suite shards FIFO across PRs (queue: max), so one PR's suite finishes before the next starts #16320)
• docs(env): move the generated env-var table into ENV_VARS.md and drop its count line, so env-var PRs stop conflicting (#16317) #16318 docs(env): move the generated env-var table into ENV_VARS.md and drop its count line, so env-var PRs stop conflicting (docs: the generated env-var table and its count line live in CLAUDE_RULES.md, so every env-var PR conflicts with every other #16317)
• fix(terminal): render TerminalModals in TerminalWindow, with each action reporting the parent's real outcome (#16285) #16313 fix(terminal): render TerminalModals in TerminalWindow, with each action reporting the parent's real outcome (fix(terminal): TerminalModals reports success after a fixed delay, and nothing renders it; TerminalWindow still has the modals inline #16285)
• refactor(slm-backend): retire the 7 dead SLM monitoring modules whose function is live elsewhere (#16282) #16308 refactor(slm-backend): retire the 7 dead SLM monitoring modules whose function is live elsewhere (tech-debt(slm-backend): 11 of the 12 modules in monitoring/ are imported by no live code #16282)
• feat(auth): one permission vocabulary, key scopes as bundles, /me permissions, and API-key scope enforcement (#16270, #16040) #16298 feat(auth): one permission vocabulary, key scopes as bundles, /me permissions, and API-key scope enforcement (auth: extend Permission to cover chat, teams and webhooks, define key scopes as permission bundles, and return effective permissions from /me #16270, API key scopes are stored and displayed but never enforced — keys inherit full user authority #16040)
• feat(slm): nodes report their GPUs in the heartbeat, and the SLM serves them per node (#16280, #16281) #16292 feat(slm): nodes report their GPUs in the heartbeat, and the SLM serves them per node (feat(slm-agent): report per-node GPU telemetry in the heartbeat, from one shared nvidia-smi/rocm-smi parser #16280, feat(slm-backend): serve per-node GPU state (GET /api/monitoring/gpu/nodes) and the autobot_gpu_* series Grafana already queries #16281)

If the queue is draining normally, ignore this. Otherwise:

1. Check whether CI is dispatching at all — see the ci-dispatch-watchdog status on these PRs
2. Merge the ones whose CI has finished and review has passed: gh pr merge <number> --squash --delete-branch
3. Look for a loop opening near-identical PRs

Warn-only runaway detector — .github/workflows/pr-queue-gate.yml. It never blocks a merge.

[mrveiss]

Review at 5a6376c83 requested changes. Both are addressed in 6f222a838; a delta check is running, and it merges on a settled green after #16487.

Finding	Fix
Guard bypass. logical_lines folded a whole-line comment ending in \ into the next line. Bash ends a comment at the newline, so # note \ doesn't continue. _offending_lines skips lines starting with #, so a real git config core.hooksPath … straight after such a comment went unreported. The old physical-line scan caught it	A whole-line comment now always stands alone in logical_lines. Regression cases: test_logical_lines_never_folds_out_of_a_comment_line, plus a comment-then-override case in the guard's catch list. The #15938 guard, the only other consumer, has no leading-comment case, so its behaviour is unchanged apart from gaining the same protection
Accidental close. The body's rationale line ("…closes one named gap in #15961… It doesn't close #15961…") made closingIssuesReferences include #15961, although AC1 is undelivered	Reworded. closingIssuesReferences is now [], checked after the push

The earlier "approve" comment on this PR was a bot notice against bffba5b97, three commits back. It isn't review evidence.

[mrveiss]

Delta 5a6376c83..6f222a838: verified, approved. It merges on a settled green after #16487. Traced, not run:

• logical_lines now emits a comment line reached with an empty buffer as its own entry, so it can't absorb the next line. A real continuation whose later line is comment-shaped still folds, because that branch needs an empty buffer.
• Both regression cases fail on the old code. test_logical_lines_never_folds_out_of_a_comment_line gets one merged entry instead of two, and the new guard case is skipped as a comment, so _offending_lines returned [].
• The Keep worktrees and PR-less branches on a fresh base #15938 guard is untouched. Its fixtures are all single-line, so their output is byte-identical.
• closingIssuesReferences is [] at 6f222a838.

[github-actions]

AutoBot Phase Validation Results

System Maturity: 96.5%

Phase Status:

PASS Phase 1: Core Infrastructure: 100.0%
PASS Phase 2: Knowledge Base and Memory: 100.0%
IN PROGRESS Phase 3: LLM Integration: 66.7%
PASS Phase 4: Security and Authentication: 100.0%
PASS Phase 5: Agent Orchestration: 100.0%
PASS Phase 6: Enhanced UI/UX: 100.0%
PASS Phase 7: Testing and Validation: 100.0%
PASS Phase 8: Advanced Features: 100.0%
PASS Phase 9: Multi-Modal AI: 100.0%
PASS Phase 10: Production Readiness: 100.0%

Recommendations:

• 🟡 MEDIUM: Phase 3: LLM Integration requires attention (66.7% complete): Review and implement
• ✅ System is production-ready - consider advanced features and scaling: Review and implement