Repository navigation
fix(advisor): preserve long consult audit records - #652
Open
rudycelekli wants to merge 1 commit into
Open
rudycelekli wants to merge 1 commit into
rudycelekli wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A completed Advisor consult with a long summary can exit with SIGPIPE before writing its advice:
jq | head -c 6000runs underpipefail. The hook has already incremented the consult count and cleared pending edits, so the user loses the audit record while the state reports completion.Consume the summary before applying the existing 6,000-byte bound, append the record first, and only then mark the consult complete. A failed log append now leaves it pending and uncounted. This does not add a general transaction or concurrency lock.
Validation: all nine actual hook-subprocess tests pass under macOS Bash 3.2 and jq; identical tests against main pass seven and fail two. Coverage includes a 500,000-character summary, a Unicode boundary, missing summaries, a real failed log append, and unrelated/unfinished consults. A focused Ubuntu workflow runs the suite. AI-assisted implementation and testing; no model calls required.
Note
Low Risk
Localized hook ordering and piping fix with regression tests; no auth or broad infrastructure changes.
Overview
Fixes a data-loss bug in the Advisor
record-consulthook where very long consult summaries could causejq | headto hit SIGPIPE underpipefail, failing the script after the consult was already counted and pending was cleared.The hook now fully reads the summary, applies the existing 6,000-byte cap in a second step, appends to
log.mdfirst (with|| exit 1), and only then increments consult state and removes the pending marker—so failed writes stay pending and uncounted.Adds a Node test suite (
record-consult.test.mjs) covering long/Unicode summaries, missing summaries, failed log writes, and ignored non-completed consults, plus a PR workflow that runs those tests whenadvisor/hooks/**changes.Reviewed by Cursor Bugbot for commit 6b02360. Bugbot is set up for automated code reviews on this repo. Configure here.