Repository navigation
Make the posted thinking trace opt-in and bounded - #178
Merged
Merged
Conversation
The trace was prefixed onto every posted review body unconditionally. On a recent live review that was 29,810 of 31,477 bytes -- 95% of the review was the model narrating its own process, much of it about being blocked by the build-tool refusal shims rather than about the code. <details> hides that only in a browser; the GitHub API serves the whole thing to every other reader, this daemon included when it reads back its own prior review. REVIEWER_POST_REVIEW_TRACE (default 0) now governs whether the sidecar trace is prefixed, and REVIEWER_TRACE_MAX_BYTES (default 20000) bounds it when enabled, cutting on a line boundary and marking what it dropped -- GitHub rejects bodies over 65536 characters outright, so an uncapped trace did not degrade a review, it failed to post it. Capture is unchanged: the trace still reaches the runtime sidecar, research artifacts, and agy's transcripts. review_subject_from_body now skips a leading <details> block, so the prior review the daemon feeds itself summarizes as the review rather than as the wrapper. With the trace present it had been resolving to "Reference Links", the closing section of the body, because the trace writes bold paragraphs instead of headings.
…body REVIEWER_POST_REVIEW_TRACE only changes the body that reaches GitHub, which a dry run cannot observe: write_dry_run_artifact records the raw model response, and the trace is prefixed onto the formatted body afterwards. Capture the literal --data payload github_api_request hands curl and assert on the posted JSON's .body in both flag states, with a small REVIEWER_TRACE_MAX_BYTES so the line-boundary cut and truncation marker are exercised too.
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.
The model's thinking trace was prefixed onto every posted review body unconditionally.
Measured across every review this bot has posted — 171 reviews spanning 2026-07-13 to 2026-07-28, across
asavs/mog-templateandGoonistotle/third-person-multiplayer:So this is not a fix for one pathological review: roughly three quarters of every byte this bot has ever posted is trace rather than review. Those byte figures are exact.
The trace is not junk, and this PR is not a judgement on its quality. Bucketing the 1,569 trace section headers across those 169 traces by keyword: ~60% analysis or verification of the code, ~18% crafting and finalizing the review, ~3% tool or permission friction. The model is largely reasoning about the code, not padding or flailing. That split is a proxy and weaker evidence than the byte counts above — it classifies the model's own section titles, not the paragraphs beneath them, so a header reading "Verifying Error Handling" is not proof the body did so.
The case for taking it out of the posted body is about the channel, not the content:
<details>collapses it only in a browser. The GitHub API returns raw markdown, so every other reader ingests all of it — including this daemon when it reads back its own prior review;The change
REVIEWER_POST_REVIEW_TRACE(default0) governs whether the sidecar trace is prefixed onto the posted body.REVIEWER_TRACE_MAX_BYTES(default20000) bounds it when enabled, cutting on a line boundary and marking what it dropped. GitHub rejects review bodies over 65,536 characters outright, so an uncapped trace did not degrade a review — it failed to post it. At 20,000 the cap is a safety valve rather than routine truncation: only 2 of the 169 observed traces would have been cut.Capture is unchanged. The trace still reaches the runtime sidecar, research artifacts under consent, and agy's own transcripts. Only the posted body changes.
Also fixes
review_subject_from_body, which now skips a leading<details>block. The trace writes bold paragraphs rather than headings, so the first heading it found was the one after the trace — in production the prior-review summary the daemon fed itself was resolving toReference Links, the review's closing section.Not gated by the flag: the branch that collapses trace-like prose the model wrote into its own body. That text is the model's output rather than something the daemon appended, so only its formatting changes.
Verification
650 fixture assertions, green. The end-to-end test asserts on the JSON payload the stubbed
curlactually receives (--data→jq -r '.body'), running the loop under both flag settings.Proven non-vacuous: flipping the production default from
0to1turns the flag-off assertion red —— and restoring it returns the suite to 650 green.
One note for reviewers: a dry run cannot verify this.
write_dry_run_artifactis passed the raw model response, while the trace is prefixed later into the posted body, so a dry-run artifact shows no trace regardless of the flag. I initially "verified" it that way and it proved nothing. The fixture plants its sidecar the way a real run produces one (a stubbed brain transcript), becauserun_agy_reviewdeletes and regenerates a directly-plantedthinking.trace.Note: conflicts with #177 on the pinned
EXPECTED_ASSERTIONSline — both branches bump it from 633. Trivial to resolve, but guaranteed for whichever merges second.