Harden hooks/watch against context bloat and blocking scans#31
Harden hooks/watch against context bloat and blocking scans#31JordanCoin merged 4 commits intomainfrom
Conversation
![chatgpt-codex-connector[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6a83256b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if state == nil { | ||
| return true |
There was a problem hiding this comment.
When state.json is missing, shouldRestartDaemon returns true and session-start invokes stopDaemon, but watch.IsRunning only verifies that the PID in .codemap/watch.pid responds to signal 0 (see watch/state.go) and does not confirm it is a codemap watcher. If that PID file is stale and the PID has been reused, this new auto-restart path can send SIGTERM to an unrelated process.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 54f28fc. shouldRestartDaemon now requires daemon ownership verification before auto-stop/restart. I added watch.IsOwnedDaemon (PID cmdline check for watch daemon <absRoot>) and gate restart logic on it, so stale/reused PID files no longer trigger termination of unrelated processes.
There was a problem hiding this comment.
Pull request overview
This PR hardens the hook + watch daemon pipeline against long-running session bloat and hook blocking by adding retention limits, gitignore-aware watching, hook timeouts, and new/expanded tests + CI coverage enforcement.
Changes:
- Added watch daemon retention controls (bounded in-memory events, events.log trimming, smaller state snapshots) and gitignore-aware directory watching/event ingestion.
- Hardened hook execution: added a configurable hook timeout (fail-open) plus “no expensive fallback scans” behavior for interactive hooks; capped diff hook output size.
- Expanded Go test coverage around the new limits/timeout/gitignore behavior and added CI coverage floor enforcement + a Makefile coverage target.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| watch/events.go | Adds gitignore-based event exclusion, bounded event history, state snapshot event cap/copy, and log tail trimming. |
| watch/daemon.go | Skips watching nested gitignored directories during directory registration. |
| scanner/walker.go | Adds GitIgnoreCache.EnsureDir helper for memoized nested .gitignore loading. |
| watch/watch_test.go | Adds an fsnotify-based test asserting gitignored directories don’t produce watcher events. |
| watch/events_limits_test.go | Adds unit tests for bounded event retention and byte-based log trimming. |
| limits/limits.go | Introduces constants for daemon/state/log retention and hook tail-read limits. |
| limits/budget.go | Adds a max diff output budget for hook output truncation. |
| limits/limits_test.go | Adds tests for truncation, adaptive depth, repo budgets, and new retention limits. |
| cmd/hooks.go | Adds hook timeout plumbing, stale-daemon restart logic, tail-only event log reads, no-fallback hub lookups, and diff output truncation. |
| cmd/hooks_test.go | Adds tests for timeout/env parsing, stale-daemon restart decisions, tail-only log reading, and no-fallback hub lookup behavior. |
| main.go | Switches hook invocation to RunHookWithTimeout and fail-open behavior on timeout. |
| Makefile | Adds coverage target to generate and summarize coverage.out. |
| .github/workflows/ci.yml | Enforces a 30% coverage floor (ubuntu/go1.23) and uploads coverage output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cmd := exec.Command(exe, args...) | ||
| cmd.Stdout = os.Stdout | ||
| var buf strings.Builder | ||
| cmd.Stdout = &buf | ||
| cmd.Stderr = os.Stderr | ||
| cmd.Run() | ||
|
|
||
| output := buf.String() | ||
| if len(output) > limits.MaxDiffOutputBytes { | ||
| output = limits.TruncateAtLineBoundary( | ||
| output, | ||
| limits.MaxDiffOutputBytes, | ||
| "\n\n... (diff output truncated, run `codemap --diff` for full output)\n", | ||
| ) | ||
| } | ||
| fmt.Print(output) |
There was a problem hiding this comment.
showDiffVsMain captures the entire codemap --diff output into a strings.Builder and only truncates afterwards. For very large diffs this can still allocate a lot of memory (even if the final printed output is capped). Consider writing through a size-limiting writer (e.g., stop collecting after MaxDiffOutputBytes + a small slack) so the process never buffers unbounded diff output.
There was a problem hiding this comment.
Addressed in 54f28fc. Same root fix as above: diff capture now uses a bounded writer so we stop retaining arbitrarily large stdout in memory before truncation.
Summary
This extends the hook coverage work with runtime hardening for the real-world context bloat + hang/crash reports.
What changed
events.logrollover/trimprompt-submit,pre-edit,post-edit,pre-compact, session-stop fallback path) use cached daemon hub data only (no expensive fallback graph scans).CODEMAP_HOOK_TIMEOUT, default8s) so hooks cannot block Claude input indefinitely.session-startfor unhealthy/abandoned daemon state..gitignore-aware for both directory registration and event ingestion, including nested ignored repos.make coverage>=30%total on ubuntu/go1.23)Tests
Validation
go test ./...make coverage(currently total coverage:33.0%)Related