fix(orchestrator): sanitize sub-agent completion relay — swarm synthesis raw finalText, exec-record JSON, unborn-HEAD change-set (#11578) - #11605
Merged
Conversation
…sis raw finalText, exec-record JSON, unborn-HEAD change-set (#11578) Three leak paths let coding sub-agent (codex ACP) transcript reach Discord: A) Swarm synthesis posted the ACP turn finalText VERBATIM. That finalText contains the orchestrator's own [tool output: ...] envelope blocks, and the synthesis path (swarm-coordinator maybeFireSwarmComplete -> server-helpers-swarm buildTaskResultLine -> connector) had NO stripping, unlike the sub-agent-router twin. Fix: extract the router's transcript stripper into a shared services/transcript-sanitizer.ts (hardened for empty-title, unterminated, and multi-block envelopes + a length cap), sanitize completionSummary at the SOURCE in the coordinator, and again defense-in-depth in buildTaskResultLine (evidence URLs preserved). Router behavior is unchanged (re-uses the shared stripper; its tests stay green). B) normalizeToolOutput JSON.stringify'd codex exec records (call_id, command, exit_code, ...) into envelopes. Fix: detect the exec-record shape and render a compact `$ <command> -> exit <code>` one-liner (+ capped stdout/stderr tail); never stringify a record carrying call_id. C) captureChangeSet threw on an unborn HEAD (git diff HEAD with zero commits), so the caller fell back to the weak narration path. Fix: diff against the empty-tree hash on unborn HEAD, and (unborn HEAD only, to preserve the born-HEAD shared-workspace clutter invariant) merge untracked files so shell-written scaffolding produces a change set. Follow-up (not in this PR): the synthesis path DOUBLE-POSTS alongside the planner's own clean reply — a router-vs-synthesis ownership design question. Tests: +27 (sanitizer robustness, coordinator source sanitize + default fallback, buildTaskResultLine strip + URL preservation, exec-record one-liner, unborn-HEAD/untracked change-set). All green; no regressions vs baseline. Co-authored-by: wakesync <shadow@shad0w.xyz>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
This was referenced Jul 2, 2026
This was referenced Jul 3, 2026
lalalune
pushed a commit
that referenced
this pull request
Jul 3, 2026
…nt files the unborn-HEAD change-set scoop (3781312, #11605) unions 'ls-files --others --exclude-standard' into changedFiles, but a fresh scaffold that runs npm install BEFORE writing .gitignore has thousands of untracked node_modules paths (--exclude-standard has nothing to honor yet). those flooded the 60-file cap, and because agent-written tool paths were spread LAST (Set dedupe keeps first occurrence), the flood evicted the agent's real files: 'what did you change' answered with node_modules noise and diffs rendered junk. - filter vendor/build dirs (node_modules, .venv, dist, ...) from the unborn-HEAD untracked scoop only; born-HEAD never scoops untracked and explicit tool-written paths are always kept - spread agentWritten first so explicit edit/write tool calls survive the MAX_CHANGED_FILES cap - drop the truncated garbage tail line when the ls-files listing was cut at maxBuffer (ENOBUFS)
Contributor
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
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
1 participant
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.
Fixes the coding sub-agent (codex ACP) transcript leak into Discord:
[tool output: …]envelope blocks, raw exec-record JSON, and file bodies reaching the user. Two prior attempts took a weaker path; this fixes all three confirmed root causes with tests.Leak paths fixed
maybeFireSwarmCompletederivedcompletionSummaryfrom the ACP turn'sresponse(finalText), which contains the orchestrator's own[tool output: …]envelope blocks appended bycaptureTerminalToolOutput.buildTaskResultLine→routeSynthesisToConnector→ Discord relayed it with no stripping, unlike the sub-agent-router twin. Fix: extracted the router's privatestripToolTranscriptinto a sharedservices/transcript-sanitizer.ts(hardened against empty-title[tool output: ""], unterminated/dangling blocks, and multiple blocks, plus aelideLongBlockshard cap ≈2000 chars). Sanitize at the source in the coordinator, and again defense-in-depth inbuildTaskResultLine(evidence URLs preserved). The router re-uses the shared stripper — behavior unchanged, its tests stay green.normalizeToolOutputfell back toJSON.stringifyfor codex exec records (call_id,command,exit_code, …), dumping the raw record into an envelope. Fix: detect the exec-record shape (hascall_idandcommand) and render a one-liner$ <command> → exit <code>(+ capped stdout/stderr tail); never stringify a record carryingcall_id.captureChangeSetthrew ongit diff HEADin a repo with zero commits → caller caught → weak narration path (why rounds 1/2 leaked). Fix: diff against the empty-tree hash on unborn HEAD, and — unborn HEAD only, to preserve the born-HEAD shared-workspace clutter invariant — mergegit ls-files --others --exclude-standardso shell-written scaffolding produces a change set.Follow-up (not in this PR)
The synthesis path double-posts alongside the planner's own clean reply (live round-3: user got the raw-envelope message and a clean "done ✅" for one task). That's a router-vs-synthesis ownership design question, deliberately left out of this fix.
Tests (+27, all green; no regressions vs baseline)
maybeFireSwarmComplete: envelope-ladenresponse→ cleancompletionSummary; tool-only response →"Task completed."fallbackbuildTaskResultLine(viahandleSwarmSynthesis): envelopes stripped, evidence URL preservednormalizeToolOutput: exec-record → one-liner (array/string command, capped tail, stringified record, non-record untouched)captureChangeSet: unborn HEAD + shell-written files → change set; untracked included only on unborn HEAD; born-HEAD clutter invariant preservedOrchestrator suite: 996 passing (was 969; +27 mine). The 15 pre-existing failures are unrelated environment/dependency issues (drizzle-orm, @noble/curves, i18n codegen) — identical count with and without this change.
[sol-relay] — [sol-orch]