Conversation
Two bugs that only manifest in ACP/Zed (headless) mode: 1. 'this.isRunning is not a function' in the task tool: initInteractiveScheduler() copied scheduler.schedule into a facade without binding, losing the CoreToolScheduler receiver. The unbound method crashes on this.isRunning() because the facade lacks it. Only affects ACP/Zed because the CLI registers its own schedulerFactory with a correct closure. Fix: use an arrow-function closure to preserve the receiver. 2. todo_pause does not stop the continuation loop: AgenticLoop.buildNextMessage() unconditionally returns continueLoop: true for any successful tool, including todo_pause. In CLI mode the React UI masks this via its continuation gate, but ACP/Zed has no such outer layer and loops indefinitely. Fix: detect a successful pause in buildNextMessage(), eagerly record tool history, and return continueLoop: false. Also adds: - Cross-platform ACP reproduction script (scripts/test-acp-zed-bugs.mjs) - Cross-platform Zed agent setup script (scripts/setup-zed-agent.mjs) - 6 behavioral tests (4 for pause loop-break, 2 for scheduler receiver)
…en setup script - Scheduler receiver test now uses a regular prototype method instead of an arrow function property, so it actually catches the receiver-loss bug (verified: test fails with 'this.isRunning is not a function' when the production fix is reverted) - setup-zed-agent.mjs: replaced naive regex JSONC stripping with a string-aware state machine that doesn't corrupt Windows paths - setup-zed-agent.mjs: abort on parse error instead of silently overwriting with empty settings; create timestamped backup before every write - test-acp-zed-bugs.mjs: attach no-op catch to abandoned promptPromise to prevent unhandled rejection when race resolves via timeout - Add acp-logging-proxy.mjs for observing ACP traffic during Zed testing
github-actions
Bot
added
the
maintainer:e2e:ok
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change fixes ACP/Zed scheduler method binding and successful ChangesACP/Zed fixes and validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
LLxprt PR Review – PR #2655Issue AlignmentThe PR resolves both bugs from #2653 with targeted, minimal changes.
Side Effects
Code Quality
Tests and Coverage
VerdictReady. The implementation is correct, minimal, well-scoped, and backed by meaningful automated behavioral tests that directly exercise the fixed paths. |
OpenCodeReview — PR #2655
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
scripts/test-acp-zed-bugs.mjs (1)
244-255: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
timedOutflag is never set — the timeout-specific expectation check is dead code.
expectTodoPauseStopsLoopchecksdetails.timedOut === true, but nothing ever assignsresults.details.timedOut. Worse, a real timeout throws out ofPromise.raceand is caught by the outercatch(lines 404-407), skipping the expectations loop entirely — so this dedicated check can never run, even though catching exactly this timeout scenario is the script's stated purpose (detecting thetodo_pauseloop bug). Separately,waitForCompletion'sexitresolves (not rejects) for the crash case too, so an early crash with few tool calls could be misread as a pass via thestopReason === undefinedfallback.🔧 Suggested fix
waitForCompletion(totalTimeout) { return new Promise((resolve, reject) => { const timer = setTimeout(() => { + this._timedOut = true; reject(new Error(`Prompt did not complete within ${totalTimeout}ms`)); }, totalTimeout); this.proc.on('exit', (code, signal) => { clearTimeout(timer); - resolve({ exited: true, code, signal }); + resolve({ exited: true, crashed: code !== 0 && code !== null, code, signal }); }); }); }} catch (error) { results.errors.push(error.message); results.details.fatalError = error.message; + results.details.timedOut = client.proc.exitCode === null && !client.proc.killed; + for (const expectation of expectations) { + const check = expectation.check(client, results.details); + results.details[expectation.name] = check; + if (!check.passed) results.errors.push(check.message); + } console.error(`\n ✗ FATAL: ${error.message}`); }Also applies to: 388-419, 442-466
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test-acp-zed-bugs.mjs` around lines 244 - 255, Update waitForCompletion and the surrounding Promise.race handling so a timeout is represented in the returned details with timedOut set to true and still flows through the expectations loop, while other errors continue to propagate. Distinguish an early process exit or crash from a normal completion instead of resolving it into the stopReason === undefined fallback, and ensure expectTodoPauseStopsLoop receives the correct timeout or exit details.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.gitignore:
- Around line 106-108: Add the ACP logging proxy’s default log directory,
.acp-proxy-logs/, to .gitignore alongside the existing .acp-test-logs/ entry,
preserving the current test-log ignore rule.
In `@scripts/acp-logging-proxy.mjs`:
- Around line 40-48: Update the ignore rules for the logDir used by the ACP
proxy so the .acp-proxy-logs directory and its generated JSONL files are
excluded from version control. Keep the existing logging behavior in the
logStream setup unchanged and align the pattern with the actual directory name.
- Around line 75-88: Attach an error handler to child.stdin in the zed-to-agent
forwarding setup so EPIPE or other stdin write failures are handled without an
unhandled error crashing the proxy. Keep the existing data logging, write, and
end-forwarding behavior unchanged.
In `@scripts/setup-zed-agent.mjs`:
- Around line 230-244: Extract the duplicated settings backup logic from the
parse-error and pre-write paths into a shared helper, using the existing
settingsPath context. Update the helper to report backup failures instead of
silently swallowing them, and ensure the pre-write flow does not overwrite
settings.json when its backup cannot be created; preserve the existing
parse-error exit behavior and backup-success messaging.
- Around line 145-188: Validate required option values in parseArgs before
assigning profile, entry, binary, or agentName: ensure the next token exists and
is not another flag, otherwise print an error identifying the flag and exit with
status 1. Add and reuse a small requireValue helper for these cases, while
preserving the existing option parsing and defaults.
- Around line 34-92: Update stripJsoncComments and the surrounding settings
parsing flow to remove trailing commas from JSONC after comments are handled,
while preserving commas inside string literals and valid JSON values. Ensure
JSON.parse accepts objects and arrays with trailing commas in existing Zed
settings.
---
Nitpick comments:
In `@scripts/test-acp-zed-bugs.mjs`:
- Around line 244-255: Update waitForCompletion and the surrounding Promise.race
handling so a timeout is represented in the returned details with timedOut set
to true and still flows through the expectations loop, while other errors
continue to propagate. Distinguish an early process exit or crash from a normal
completion instead of resolving it into the stopReason === undefined fallback,
and ensure expectTodoPauseStopsLoop receives the correct timeout or exit
details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 43c2b428-ed1d-43c4-b2e7-8df4b2dcabac
📒 Files selected for processing (9)
.gitignorepackages/agents/src/core/agenticLoop/AgenticLoop.tspackages/agents/src/core/agenticLoop/__tests__/agenticLoop.todoPause.test.tspackages/agents/src/core/agenticLoop/loopHelpers.tspackages/agents/src/core/subagentExecution.scheduler-receiver.test.tspackages/agents/src/core/subagentExecution.tsscripts/acp-logging-proxy.mjsscripts/setup-zed-agent.mjsscripts/test-acp-zed-bugs.mjs
|
|
||
| # ACP test logs (from scripts/test-acp-zed-bugs.mjs) | ||
| .acp-test-logs/ |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Missing .gitignore entry for the ACP logging proxy's log directory.
scripts/acp-logging-proxy.mjs writes full ACP traffic (prompts, tool output) to .acp-proxy-logs/ by default, but this diff only ignores .acp-test-logs/. Add the proxy's log directory to avoid accidentally committing captured session content.
🔧 Suggested fix
# ACP test logs (from scripts/test-acp-zed-bugs.mjs)
.acp-test-logs/
+
+# ACP proxy logs (from scripts/acp-logging-proxy.mjs)
+.acp-proxy-logs/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # ACP test logs (from scripts/test-acp-zed-bugs.mjs) | |
| .acp-test-logs/ | |
| # ACP test logs (from scripts/test-acp-zed-bugs.mjs) | |
| .acp-test-logs/ | |
| # ACP proxy logs (from scripts/acp-logging-proxy.mjs) | |
| .acp-proxy-logs/ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.gitignore around lines 106 - 108, Add the ACP logging proxy’s default log
directory, .acp-proxy-logs/, to .gitignore alongside the existing
.acp-test-logs/ entry, preserving the current test-log ignore rule.
| // Log directory | ||
| const logDir = process.env.ACP_PROXY_LOG_DIR || join(__dirname, '..', '.acp-proxy-logs'); | ||
| if (!existsSync(logDir)) { | ||
| mkdirSync(logDir, { recursive: true }); | ||
| } | ||
|
|
||
| const timestamp = new Date().toISOString().replace(/[:.]/g, '-'); | ||
| const logFile = join(logDir, `acp-${timestamp}.jsonl`); | ||
| const logStream = createWriteStream(logFile, { flags: 'a' }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Logs full ACP traffic unredacted; verify .gitignore coverage.
logEntry persists raw/parsed request and response bodies (prompts, tool output, potentially secrets) to .acp-proxy-logs/*.jsonl. This directory name differs from .acp-test-logs/, which is the only pattern added to .gitignore in this PR — see the .gitignore comment for the concrete gap.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/acp-logging-proxy.mjs` around lines 40 - 48, Update the ignore rules
for the logDir used by the ACP proxy so the .acp-proxy-logs directory and its
generated JSONL files are excluded from version control. Keep the existing
logging behavior in the logStream setup unchanged and align the pattern with the
actual directory name.
| const child = spawn(agentCmd, agentCmdArgs, { | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| env: { ...process.env }, | ||
| }); | ||
|
|
||
| // zed -> agent (stdin forwarding with logging) | ||
| process.stdin.on('data', (chunk) => { | ||
| logEntry('zed->agent', chunk); | ||
| child.stdin.write(chunk); | ||
| }); | ||
|
|
||
| process.stdin.on('end', () => { | ||
| child.stdin.end(); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add an error handler on child.stdin to avoid crashing on EPIPE.
If the child process dies (or its stdin is already ended) while process.stdin still emits data, child.stdin.write(chunk) can raise an unhandled 'error' event and crash the proxy.
🔧 Suggested fix
+child.stdin.on('error', () => {
+ // Child stdin closed/died; ignore further write errors.
+});
+
// zed -> agent (stdin forwarding with logging)
process.stdin.on('data', (chunk) => {
logEntry('zed->agent', chunk);
child.stdin.write(chunk);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const child = spawn(agentCmd, agentCmdArgs, { | |
| stdio: ['pipe', 'pipe', 'pipe'], | |
| env: { ...process.env }, | |
| }); | |
| // zed -> agent (stdin forwarding with logging) | |
| process.stdin.on('data', (chunk) => { | |
| logEntry('zed->agent', chunk); | |
| child.stdin.write(chunk); | |
| }); | |
| process.stdin.on('end', () => { | |
| child.stdin.end(); | |
| }); | |
| const child = spawn(agentCmd, agentCmdArgs, { | |
| stdio: ['pipe', 'pipe', 'pipe'], | |
| env: { ...process.env }, | |
| }); | |
| child.stdin.on('error', () => { | |
| // Child stdin closed/died; ignore further write errors. | |
| }); | |
| // zed -> agent (stdin forwarding with logging) | |
| process.stdin.on('data', (chunk) => { | |
| logEntry('zed->agent', chunk); | |
| child.stdin.write(chunk); | |
| }); | |
| process.stdin.on('end', () => { | |
| child.stdin.end(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/acp-logging-proxy.mjs` around lines 75 - 88, Attach an error handler
to child.stdin in the zed-to-agent forwarding setup so EPIPE or other stdin
write failures are handled without an unhandled error crashing the proxy. Keep
the existing data logging, write, and end-forwarding behavior unchanged.
| /** | ||
| * Strip // line comments and /* block comments from JSONC, respecting | ||
| * string literals. Handles escaped quotes inside strings. | ||
| */ | ||
| function stripJsoncComments(text) { | ||
| let result = ''; | ||
| let i = 0; | ||
| let inString = false; | ||
|
|
||
| while (i < text.length) { | ||
| const ch = text[i]; | ||
| const next = text[i + 1]; | ||
|
|
||
| if (inString) { | ||
| result += ch; | ||
| if (ch === '\\' && next !== undefined) { | ||
| result += next; | ||
| i += 2; | ||
| continue; | ||
| } | ||
| if (ch === '"') { | ||
| inString = false; | ||
| } | ||
| i++; | ||
| continue; | ||
| } | ||
|
|
||
| if (ch === '"') { | ||
| inString = true; | ||
| result += ch; | ||
| i++; | ||
| continue; | ||
| } | ||
|
|
||
| if (ch === '/' && next === '/') { | ||
| // Line comment: skip to end of line | ||
| i += 2; | ||
| while (i < text.length && text[i] !== '\n') { | ||
| i++; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (ch === '/' && next === '*') { | ||
| // Block comment: skip to closing */ | ||
| i += 2; | ||
| while (i < text.length && !(text[i] === '*' && text[i + 1] === '/')) { | ||
| i++; | ||
| } | ||
| i += 2; // skip closing */ | ||
| continue; | ||
| } | ||
|
|
||
| result += ch; | ||
| i++; | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Zed editor's settings.json support trailing commas (JSONC)?
💡 Result:
Yes, the Zed editor's settings.json file supports trailing commas [1]. Zed's settings.json uses the JSONC (JSON with Comments) format, which explicitly permits both comments (using //) and trailing commas [2][1][3]. This support was implemented to provide a more flexible configuration experience [1]. While Zed natively supports this syntax in its settings files, users should be aware that external formatters like Prettier may sometimes interact unexpectedly with this format if not configured correctly, as standard JSON parsers used by some tools do not allow trailing commas [4][5]. Within Zed itself, however, the parser is configured to handle them without error [6][1].
Citations:
- 1: Allow support for trailing commas in JSON blocks (currently those break the JSON ZED config readout) zed-industries/zed#6074
- 2: Certain comments are not permitted in Zed's settings.json zed-industries/zed#12316
- 3: https://github.com/zed-industries/zed/blob/main/docs/src/configuring-zed.md
- 4: https://zed.dev/docs/languages/json
- 5: Prettier in Zed formats JSON files as JSONC zed-industries/zed#45446
- 6: Allow trailing commas in builtin JSONC schemas zed-industries/zed#43854
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant sections with line numbers.
git ls-files scripts/setup-zed-agent.mjs
wc -l scripts/setup-zed-agent.mjs
sed -n '1,220p' scripts/setup-zed-agent.mjs | cat -nRepository: vybestack/llxprt-code
Length of output: 7833
Handle trailing commas in Zed JSONC. settings.json allows comments and trailing commas, but JSON.parse(stripJsoncComments(...)) will still reject a valid config if it ends an object/array with a comma. Strip trailing commas too, or switch to a JSONC parser, so the script can read existing Zed settings without failing on common syntax.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/setup-zed-agent.mjs` around lines 34 - 92, Update stripJsoncComments
and the surrounding settings parsing flow to remove trailing commas from JSONC
after comments are handled, while preserving commas inside string literals and
valid JSON values. Ensure JSON.parse accepts objects and arrays with trailing
commas in existing Zed settings.
| function parseArgs() { | ||
| const args = process.argv.slice(2); | ||
| const opts = { | ||
| profile: 'gpt56high', | ||
| entry: null, | ||
| binary: null, | ||
| agentName: 'llxprt', | ||
| yolo: true, | ||
| debug: true, | ||
| }; | ||
|
|
||
| for (let i = 0; i < args.length; i++) { | ||
| switch (args[i]) { | ||
| case '--profile': | ||
| opts.profile = args[++i]; | ||
| break; | ||
| case '--entry': | ||
| opts.entry = args[++i]; | ||
| break; | ||
| case '--binary': | ||
| opts.binary = args[++i]; | ||
| break; | ||
| case '--agent-name': | ||
| opts.agentName = args[++i]; | ||
| break; | ||
| case '--no-yolo': | ||
| opts.yolo = false; | ||
| break; | ||
| case '--no-debug': | ||
| opts.debug = false; | ||
| break; | ||
| case '--help': | ||
| case '-h': | ||
| printHelp(); | ||
| process.exit(0); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| opts.binary = opts.binary || findBunBinary(); | ||
| opts.entry = opts.entry || findEntryPoint(); | ||
|
|
||
| return opts; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
No validation on flag values; missing/consumed-as-flag values silently write a broken config.
args[++i] for --profile, --entry, --binary, --agent-name doesn't check that the next token exists or isn't itself a flag. If the value is omitted, opts.<field> becomes undefined; when later placed in the args array and JSON.stringify'd (line 296), it becomes null in the written settings, silently corrupting the generated agent_servers entry with no error shown to the user.
🛡️ Proposed fix: validate flag values
for (let i = 0; i < args.length; i++) {
switch (args[i]) {
case '--profile':
- opts.profile = args[++i];
+ opts.profile = requireValue(args, ++i, '--profile');
break;
case '--entry':
- opts.entry = args[++i];
+ opts.entry = requireValue(args, ++i, '--entry');
break;
case '--binary':
- opts.binary = args[++i];
+ opts.binary = requireValue(args, ++i, '--binary');
break;
case '--agent-name':
- opts.agentName = args[++i];
+ opts.agentName = requireValue(args, ++i, '--agent-name');
break;function requireValue(args, i, flag) {
const value = args[i];
if (value === undefined || value.startsWith('--')) {
console.error(`Error: ${flag} requires a value`);
process.exit(1);
}
return value;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function parseArgs() { | |
| const args = process.argv.slice(2); | |
| const opts = { | |
| profile: 'gpt56high', | |
| entry: null, | |
| binary: null, | |
| agentName: 'llxprt', | |
| yolo: true, | |
| debug: true, | |
| }; | |
| for (let i = 0; i < args.length; i++) { | |
| switch (args[i]) { | |
| case '--profile': | |
| opts.profile = args[++i]; | |
| break; | |
| case '--entry': | |
| opts.entry = args[++i]; | |
| break; | |
| case '--binary': | |
| opts.binary = args[++i]; | |
| break; | |
| case '--agent-name': | |
| opts.agentName = args[++i]; | |
| break; | |
| case '--no-yolo': | |
| opts.yolo = false; | |
| break; | |
| case '--no-debug': | |
| opts.debug = false; | |
| break; | |
| case '--help': | |
| case '-h': | |
| printHelp(); | |
| process.exit(0); | |
| break; | |
| } | |
| } | |
| opts.binary = opts.binary || findBunBinary(); | |
| opts.entry = opts.entry || findEntryPoint(); | |
| return opts; | |
| } | |
| function parseArgs() { | |
| const args = process.argv.slice(2); | |
| const opts = { | |
| profile: 'gpt56high', | |
| entry: null, | |
| binary: null, | |
| agentName: 'llxprt', | |
| yolo: true, | |
| debug: true, | |
| }; | |
| for (let i = 0; i < args.length; i++) { | |
| switch (args[i]) { | |
| case '--profile': | |
| opts.profile = requireValue(args, ++i, '--profile'); | |
| break; | |
| case '--entry': | |
| opts.entry = requireValue(args, ++i, '--entry'); | |
| break; | |
| case '--binary': | |
| opts.binary = requireValue(args, ++i, '--binary'); | |
| break; | |
| case '--agent-name': | |
| opts.agentName = requireValue(args, ++i, '--agent-name'); | |
| break; | |
| case '--no-yolo': | |
| opts.yolo = false; | |
| break; | |
| case '--no-debug': | |
| opts.debug = false; | |
| break; | |
| case '--help': | |
| case '-h': | |
| printHelp(); | |
| process.exit(0); | |
| break; | |
| } | |
| } | |
| opts.binary = opts.binary || findBunBinary(); | |
| opts.entry = opts.entry || findEntryPoint(); | |
| return opts; | |
| } | |
| function requireValue(args, i, flag) { | |
| const value = args[i]; | |
| if (value === undefined || value.startsWith('--')) { | |
| console.error(`Error: ${flag} requires a value`); | |
| process.exit(1); | |
| } | |
| return value; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/setup-zed-agent.mjs` around lines 145 - 188, Validate required option
values in parseArgs before assigning profile, entry, binary, or agentName:
ensure the next token exists and is not another flag, otherwise print an error
identifying the flag and exit with status 1. Add and reuse a small requireValue
helper for these cases, while preserving the existing option parsing and
defaults.
| } catch (e) { | ||
| console.error( | ||
| `Error: could not parse existing settings.json: ${e.message}`, | ||
| ); | ||
| // Save a backup so the user can recover | ||
| try { | ||
| writeFileSync( | ||
| `${settingsPath}.bak`, | ||
| readFileSync(settingsPath, 'utf-8'), | ||
| ); | ||
| console.error(`A backup was saved to ${settingsPath}.bak`); | ||
| } catch {} | ||
| console.error('Please fix the file manually and re-run this script.'); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Backup failures are silently swallowed, and the backup logic is duplicated.
Both the parse-error backup (234-241) and the pre-write backup (283-291) wrap writeFileSync in an empty catch {}. If the backup itself fails (permissions, disk full, etc.) the failure is invisible, and in the pre-write case the script proceeds to overwrite the original settings.json regardless — losing the safety net the comment on line 283 explicitly promises ("in case of disk error mid-write"). The two blocks are also copy-pasted.
♻️ Proposed fix: extract a helper and surface backup failures
+function backupFile(path) {
+ try {
+ writeFileSync(`${path}.bak`, readFileSync(path, 'utf-8'));
+ return true;
+ } catch (e) {
+ console.error(`Warning: could not write backup to ${path}.bak: ${e.message}`);
+ return false;
+ }
+} } catch (e) {
console.error(
`Error: could not parse existing settings.json: ${e.message}`,
);
- // Save a backup so the user can recover
- try {
- writeFileSync(
- `${settingsPath}.bak`,
- readFileSync(settingsPath, 'utf-8'),
- );
- console.error(`A backup was saved to ${settingsPath}.bak`);
- } catch {}
+ if (backupFile(settingsPath)) {
+ console.error(`A backup was saved to ${settingsPath}.bak`);
+ }
console.error('Please fix the file manually and re-run this script.');
process.exit(1);
}- // Backup before write (in case of disk error mid-write)
- if (existsSync(settingsPath)) {
- try {
- writeFileSync(
- `${settingsPath}.bak`,
- readFileSync(settingsPath, 'utf-8'),
- );
- } catch {}
- }
+ // Backup before write (in case of disk error mid-write)
+ if (existsSync(settingsPath) && !backupFile(settingsPath)) {
+ console.error('Aborting: could not create a safety backup before writing.');
+ process.exit(1);
+ }Also applies to: 283-291
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/setup-zed-agent.mjs` around lines 230 - 244, Extract the duplicated
settings backup logic from the parse-error and pre-write paths into a shared
helper, using the existing settingsPath context. Update the helper to report
backup failures instead of silently swallowing them, and ensure the pre-write
flow does not overwrite settings.json when its backup cannot be created;
preserve the existing parse-error exit behavior and backup-success messaging.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/acp-logging-proxy.mjs`:
- Around line 103-117: Update the child.stderr data handler to preserve stderr
text across arbitrary chunk boundaries, buffering until newline boundaries or
maintaining a rolling tail before applying the existing isRunning, not a
function, and ERROR signature checks. Emit STDERR_ERROR records for matching
complete or split messages while continuing to write the original chunks to
process.stderr.
- Around line 120-139: Update the child error and exit handlers around
child.on('error') and child.on('exit') to flush logStream before terminating,
using logStream.end with the appropriate exit callback or process.exitCode.
Ensure shutdown is idempotent so overlapping error and exit events do not write
duplicate records or trigger termination more than once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3108a48a-cb1a-4428-9b68-1fa942252f81
📒 Files selected for processing (2)
scripts/acp-logging-proxy.mjsscripts/setup-zed-agent.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/setup-zed-agent.mjs
| if ( | ||
| text.includes('isRunning') || | ||
| text.includes('not a function') || | ||
| text.includes('ERROR') | ||
| ) { | ||
| logStream.write( | ||
| JSON.stringify({ | ||
| ts: new Date().toISOString(), | ||
| event: 'STDERR_ERROR', | ||
| text: text.trim(), | ||
| }) + '\n', | ||
| ); | ||
| } | ||
| process.stderr.write(chunk); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect the relevant region.
ast-grep outline scripts/acp-logging-proxy.mjs --view expanded || true
echo '---'
nl -ba scripts/acp-logging-proxy.mjs | sed -n '1,180p'Repository: vybestack/llxprt-code
Length of output: 777
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section with line numbers using awk, since nl is unavailable.
awk 'NR>=90 && NR<=140 { printf("%4d %s\n", NR, $0) }' scripts/acp-logging-proxy.mjsRepository: vybestack/llxprt-code
Length of output: 1542
Handle stderr signatures across chunk boundaries. child.stderr.on('data') receives arbitrary chunks, so a split this.isRunning is not a function/ERROR message can bypass STDERR_ERROR logging. Buffer stderr until newline boundaries or keep a rolling tail before matching.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/acp-logging-proxy.mjs` around lines 103 - 117, Update the
child.stderr data handler to preserve stderr text across arbitrary chunk
boundaries, buffering until newline boundaries or maintaining a rolling tail
before applying the existing isRunning, not a function, and ERROR signature
checks. Emit STDERR_ERROR records for matching complete or split messages while
continuing to write the original chunks to process.stderr.
| child.on('error', (err) => { | ||
| logStream.write( | ||
| JSON.stringify({ | ||
| ts: new Date().toISOString(), | ||
| event: 'child-error', | ||
| error: err.message, | ||
| }) + '\n', | ||
| ); | ||
| process.exit(1); | ||
| }); | ||
|
|
||
| child.on('exit', (code, signal) => { | ||
| logStream.write( | ||
| JSON.stringify({ | ||
| ts: new Date().toISOString(), | ||
| event: 'child-exit', | ||
| code, | ||
| signal, | ||
| }) + '\n', | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section with line numbers.
sed -n '1,220p' scripts/acp-logging-proxy.mjs | nl -ba | sed -n '90,170p'
# Find shutdown-related handlers in the file.
rg -n "process\.exit|process\.exitCode|logStream\.(end|write)|child\.on\('error'|child\.on\('exit'" scripts/acp-logging-proxy.mjsRepository: vybestack/llxprt-code
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pwd
ls -laRepository: vybestack/llxprt-code
Length of output: 3521
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('scripts/acp-logging-proxy.mjs')
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 100 <= i <= 170:
print(f"{i:4d}: {line}")
PYRepository: vybestack/llxprt-code
Length of output: 1456
Flush the log stream before exiting. process.exit() right after logStream.write() / logStream.end() can drop the final child-error and child-exit records; use logStream.end(() => process.exit(...)) or process.exitCode instead, and keep the shutdown path idempotent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/acp-logging-proxy.mjs` around lines 120 - 139, Update the child error
and exit handlers around child.on('error') and child.on('exit') to flush
logStream before terminating, using logStream.end with the appropriate exit
callback or process.exitCode. Ensure shutdown is idempotent so overlapping error
and exit events do not write duplicate records or trigger termination more than
once.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // The failed pause must NOT have terminated the loop — the model should | ||
| // have received the error and continued with a second turn. | ||
| expect(turnMessages.length).toBeGreaterThanOrEqual(2); |
There was a problem hiding this comment.
This assertion is too loose. With exactly 2 scripted turns, use
toBe(2)instead oftoBeGreaterThanOrEqual(2)to catch regressions where the loop continues beyond the expected second turn.
| schedule: ( | ||
| request: Parameters<typeof scheduler.schedule>[0], | ||
| signal: Parameters<typeof scheduler.schedule>[1], | ||
| ) => scheduler.schedule(request, signal), |
There was a problem hiding this comment.
The fix correctly wraps
scheduler.schedulein a closure to preserve thethisbinding. This is a standard pattern for avoiding context loss when passing methods as callbacks. The use ofParameters<typeof scheduler.schedule>ensures type safety without manual type duplication. No issues found with this change — it addresses the binding issue cleanly and is well-documented.
| child.on('exit', (code, signal) => { | ||
| logStream.write( | ||
| JSON.stringify({ | ||
| ts: new Date().toISOString(), | ||
| event: 'child-exit', | ||
| code, | ||
| signal, | ||
| }) + '\n', | ||
| ); | ||
| logStream.end(); | ||
| process.exit(code ?? 1); | ||
| }); |
There was a problem hiding this comment.
process.exit(code ?? 1)is called immediately afterlogStream.end(), but streamend()is asynchronous. Node.js does not wait for pending I/O callbacks whenprocess.exit()is invoked, so the finalchild-exitlog entry (and any other buffered writes) may be lost. This makes crash investigations incomplete because the proxy can terminate before logs are flushed to disk.Suggestion: wait for the log stream to finish before exiting.
Example:
child.on('exit', (code, signal) => {
logStream.write(
JSON.stringify({
ts: new Date().toISOString(),
event: 'child-exit',
code,
signal,
}) + '\n',
);
logStream.end(() => {
process.exit(code ?? 1);
});
});
| // zed -> agent (stdin forwarding with logging) | ||
| process.stdin.on('data', (chunk) => { | ||
| logEntry('zed->agent', chunk); | ||
| child.stdin.write(chunk); | ||
| }); | ||
|
|
||
| process.stdin.on('end', () => { | ||
| child.stdin.end(); | ||
| }); | ||
|
|
||
| // agent -> zed (stdout forwarding with logging) | ||
| child.stdout.on('data', (chunk) => { | ||
| logEntry('agent->zed', chunk); | ||
| process.stdout.write(chunk); | ||
| }); |
There was a problem hiding this comment.
Stream writes can fail if the receiving side is closed unexpectedly (e.g., Zed disconnects or the child exits while data is being forwarded). The current handlers call
child.stdin.write(chunk)andprocess.stdout.write(chunk)without handlingerrorevents. In Node.js, unhandled stream errors can crash the process.Suggestion: add
errorhandlers to both forwarding paths, and also attach an error handler toprocess.stdinso unexpected read failures do not terminate the proxy.Example:
process.stdin.on('error', (err) => {
logStream.write(JSON.stringify({ ts: new Date().toISOString(), event: 'stdin-error', error: err.message }) + '\n');
child.stdin.end();
});child.stdin.on('error', () => child.stdin.end());
process.stdout.on('error', () => {});
| const child = spawn(agentCmd, agentCmdArgs, { | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| env: { ...process.env }, | ||
| }); |
There was a problem hiding this comment.
The proxy forwards the full parent environment to the child (
env: { ...process.env }) and writes raw ACP message payloads to log files. If the environment contains secrets (API keys, tokens) or ACP messages contain sensitive user content, this will persist them to.acp-proxy-logsin plaintext. In addition,mkdirSyncuses default permissions, which on many systems is0o755, allowing other local users to read the logs.Suggestion: avoid blindly inheriting secrets in debug logging, and restrict log directory/file permissions when possible.
Example:
const child = spawn(agentCmd, agentCmdArgs, {
stdio: ['pipe', 'pipe', 'pipe'],
env: { ...process.env },
});// Consider:
// - filtering known secret env vars from logs or child env if debugging does not require them
// - creating the log dir withmkdirSync(logDir, { recursive: true, mode: 0o700 })
| results.details.toolCallCount = client.toolCalls.length; | ||
| results.details.messageCount = client.agentMessages.length; | ||
| results.details.stderrErrors = client.errors.length; | ||
| results.details.stderrErrorSnippets = client.errors.slice(0, 3); |
There was a problem hiding this comment.
The test never validates that the specific tools requested in the prompt (
todo_writeandtodo_pause) were actually invoked. The agent could call zero tools, wrong tools, or loop on unrelated tool calls, and this expectation would still pass as long as the total count stays below 10 and stderr is clean. Assert on the expected tool names to make this a meaningful integration test.
| this.proc.on('exit', (code, signal) => { | ||
| clearTimeout(timer); | ||
| resolve({ exited: true, code, signal }); | ||
| }); |
There was a problem hiding this comment.
The script treats any process exit as a normal completion. If the child crashes due to Bug 1 (exit code != 0),
waitForCompletionresolves and the test continues analyzing results instead of immediately failing. Capture the exit code in results and fail fast on non-zero exits.
| if (!settings.agent_servers) { | ||
| settings.agent_servers = {}; | ||
| } | ||
|
|
||
| settings.agent_servers[opts.agentName] = { |
There was a problem hiding this comment.
Missing validation for
agent_serverstype. If the existing Zed settings file definesagent_serversas an array, string, or other non-object type, this script will either silently fail to update the configuration (for arrays/strings whereJSON.stringifyignores non-index properties) or corrupt the file. Add a type check and reset/reject invalid structures before assignment.
| console.error(`A backup was saved to ${settingsPath}.bak`); | ||
| } catch {} |
There was a problem hiding this comment.
Silent backup failure swallows filesystem errors (e.g., read-only disk, out of space, permission denied). If the backup cannot be written, the user proceeds without a safety net. At minimum, log a warning so the user is aware.
| try { | ||
| writeFileSync(`${settingsPath}.bak`, readFileSync(settingsPath, 'utf-8')); | ||
| } catch {} |
There was a problem hiding this comment.
Silent backup failure on the final pre-write backup. If this write fails (e.g., disk full), the user has no indication that the original file might be at risk.
…y, formatting) - Remove TODO-triggering words from comments (sonarjs/todo-tag) - Extract performAcpHandshake, collectAcpMetrics, evaluateExpectations helpers to reduce runScenario below 80-line limit (max-lines-per-function) - Simplify conditional expression to reduce ternary nesting (sonarjs/expression-complexity) - Add explanatory comments to empty catch blocks (no-empty) - Refactor stripJsoncComments to avoid deep nesting (sonarjs/nested-control-flow) - Fix prefer-const, remove unused imports
| const result = await initInteractiveScheduler( | ||
| { | ||
| schedulerFactory: async () => ({ | ||
| schedule: async (req: unknown, sig: unknown) => | ||
| realScheduler.schedule(req, sig), | ||
| dispose: () => { | ||
| void realScheduler.dispose(); | ||
| }, | ||
| }), | ||
| }, | ||
| ctx, | ||
| ); |
There was a problem hiding this comment.
This test does not actually verify receiver preservation in the schedulerFactory path. The factory returns
schedule: async (...) => realScheduler.schedule(req, sig), which is an arrow function that lexically bindsthistorealScheduler. Because of that, the test will pass even ifinitInteractiveSchedulerwere to copyscheduler.schedulewithout binding. To meaningfully cover the factory path and match the intent of issue #2653, makeschedulea regular prototype method on the factory-returned object so receiver-loss could be detected if the facade copied it directly.
| const { client, history } = createScriptedAgentClient([ | ||
| [ | ||
| toolCallRequestEvent('todo_pause', 'pause-2', { | ||
| reason: 'need to stop', | ||
| }), | ||
| finishedEvent(), | ||
| ], | ||
| ]); |
There was a problem hiding this comment.
This test verifies that tool_response blocks exist in history, but it does not assert that the loop actually stopped after the pause tool call. If the implementation incorrectly continues to a second model turn after a successful pause, this test would still pass because the first turn's tool response would remain in history. Consider also destructuring
turnMessagesand asserting its length is 1, similar to the first test, to ensure the loop truly terminated.
| // The failed pause must NOT have terminated the loop — the model should | ||
| // have received the error and continued with a second turn. | ||
| expect(turnMessages.length).toBeGreaterThanOrEqual(2); |
There was a problem hiding this comment.
This test verifies that the loop continues after a failed pause tool, but it only checks the number of turns (
turnMessages.length >= 2). It does not verify that the error from the failed pause tool was actually propagated to the model in the next turn. Consider inspecting the second turn's messages to confirm they include the error result from the failedtodo_pauseexecution, ensuring errors are not silently swallowed.
| const { client, history } = createScriptedAgentClient([ | ||
| [ | ||
| toolCallRequestEvent('todo_pause', 'pause-2', { | ||
| reason: 'need to stop', | ||
| }), | ||
| finishedEvent(), | ||
| ], | ||
| ]); |
There was a problem hiding this comment.
This test verifies that a tool_response block exists in history, but it does not confirm the loop actually terminated. If the implementation incorrectly continues to a second model turn after a successful pause, this test would still pass because the first turn's tool response remains in history. You already have
turnMessagesavailable fromcreateScriptedAgentClient; addexpect(turnMessages).toHaveLength(1)here to match the first test and ensure the loop truly stopped.
| // The failed pause must NOT have terminated the loop — the model should | ||
| // have received the error and continued with a second turn. | ||
| expect(turnMessages.length).toBeGreaterThanOrEqual(2); |
There was a problem hiding this comment.
This test verifies the loop continues after a failed pause tool, but it only checks turn count (
turnMessages.length >= 2). It does not validate that the error from the failedtodo_pauseexecution was actually propagated to the model in the next turn. Inspect the second turn's messages to confirm they include the error result, ensuring errors are not silently swallowed.
| process.on('SIGTERM', () => { | ||
| child.kill('SIGTERM'); | ||
| }); | ||
| process.on('SIGINT', () => { | ||
| child.kill('SIGINT'); | ||
| }); |
There was a problem hiding this comment.
Signal handlers call
child.kill()but do not wait for the child process to actually exit before the proxy process terminates. If the proxy exits before the child has finished, the child may be left running as an orphan. Add a wait loop or listen for the child's'exit'event before callingprocess.exit()inSIGTERM/SIGINThandlers.
| opts.binary = opts.binary || findBunBinary(); | ||
| opts.entry = opts.entry || findEntryPoint(); | ||
|
|
||
| return opts; | ||
| } |
There was a problem hiding this comment.
--entryand--binaryare accepted without any validation that the files exist or are accessible. If a typo or stale path is provided, Zed will receive an invalid command and the agent will fail to start. Consider validating these paths withexistsSyncbefore writing the settings file, and fail fast with a clear error message.
| if (existsSync(settingsPath)) { | ||
| try { | ||
| const raw = readFileSync(settingsPath, 'utf-8'); | ||
| settings = JSON.parse(stripJsoncComments(raw)); | ||
| } catch (e) { | ||
| console.error( | ||
| `Error: could not parse existing settings.json: ${e.message}`, | ||
| ); | ||
| // Save a backup so the user can recover | ||
| try { | ||
| writeFileSync( | ||
| `${settingsPath}.bak`, | ||
| readFileSync(settingsPath, 'utf-8'), | ||
| ); | ||
| console.error(`A backup was saved to ${settingsPath}.bak`); | ||
| } catch { | ||
| // If backup also fails, nothing more we can do | ||
| } | ||
| console.error('Please fix the file manually and re-run this script.'); | ||
| process.exit(1); | ||
| } | ||
| } else { | ||
| // Ensure directory exists | ||
| const dir = dirname(settingsPath); | ||
| if (!existsSync(dir)) { | ||
| mkdirSync(dir, { recursive: true }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The script reads and writes
settings.jsonwithout any file locking or atomic-update strategy. If Zed is running and writing to the same file concurrently, or if multiple instances of this script run in parallel, the file can be corrupted or changes can be lost. Consider using an advisory lock (for example,open+flockon Unix, or a lock-file approach) around the read-modify-write sequence.
| this.buffer += chunk.toString(); | ||
| const lines = this.buffer.split('\n'); |
There was a problem hiding this comment.
The
sendmethod appends every incoming chunk tothis.bufferwithout any size limit. If the subprocess emits a continuous stream of non-JSON data without newlines (e.g., verbose debug logs, binary garbage, or a runaway agent outputting partial UTF-8 sequences), the buffer grows unbounded and will eventually exhaust memory. Add a max buffer size (e.g., 10MB) and discard old data or terminate the process when the limit is exceeded.
| // Wait for prompt to complete (or timeout) | ||
| // Attach a no-op catch to prevent unhandled rejection if the race | ||
| // resolves via the timeout/process-exit path first. | ||
| promptPromise.catch(() => {}); |
There was a problem hiding this comment.
promptPromise.catch(() => {})silently discards all errors from thesession/promptrequest. If the server returns a malformed response, session-not-found error, or any other protocol-level failure, the error is swallowed and the test proceeds as if nothing went wrong. Instead, capture the error so it can be reported in the results, or at least attach it toresults.detailsfor diagnosis.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/test-acp-zed-bugs.mjs`:
- Line 352: Update waitForCompletion() and its race handling so timeout outcomes
populate results.details.timedOut or details.timeout. Ensure the timeout
rejection path sets the marker, or resolve a clearly identified timeout result,
allowing expectTodoPauseStopsLoop() to enter its timeout branch while preserving
normal completion fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cb37a03a-6df4-4ac6-a0d0-65d6b79c8b3b
📒 Files selected for processing (3)
scripts/acp-logging-proxy.mjsscripts/setup-zed-agent.mjsscripts/test-acp-zed-bugs.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/setup-zed-agent.mjs
- scripts/acp-logging-proxy.mjs
| const client = new AcpClient(child); | ||
| client.attachStderr(); | ||
|
|
||
| const results = { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether AcpClient.waitForCompletion returns a `timedOut` field,
# and whether it's ever copied into `results.details` anywhere in the file.
ast-grep run --pattern 'waitForCompletion($$$) { $$$ }' --lang javascript scripts/test-acp-zed-bugs.mjs
rg -n 'timedOut' scripts/test-acp-zed-bugs.mjsRepository: vybestack/llxprt-code
Length of output: 590
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== scripts/test-acp-zed-bugs.mjs around relevant lines ==="
sed -n '340,470p' scripts/test-acp-zed-bugs.mjs | cat -n
echo
echo "=== search for details.timedOut and promptResult.timedOut in repo ==="
rg -n 'details\.timedOut|promptResult\.timedOut|timedOut\s*===' scripts/test-acp-zed-bugs.mjs .Repository: vybestack/llxprt-code
Length of output: 5288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== find waitForCompletion definitions/usages ==="
rg -n 'waitForCompletion' .
echo
echo "=== inspect likely client implementation files if found ==="
fd -a 'AcpClient|acp.*client|client' . | head -n 50Repository: vybestack/llxprt-code
Length of output: 5434
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== scripts/test-acp-zed-bugs.mjs waitForCompletion implementation ==="
sed -n '210,255p' scripts/test-acp-zed-bugs.mjs | cat -n
echo
echo "=== any timeout shape references in the file ==="
rg -n 'timedOut|timeout|Promise\.race|waitForCompletion\(' scripts/test-acp-zed-bugs.mjsRepository: vybestack/llxprt-code
Length of output: 2557
details.timedOut (and details.timeout) never get populated, so the timeout branch can’t fire. waitForCompletion() rejects on timeout and only resolves { exited: true, code, signal }, while the race only copies stopReason into results.details. If you want expectTodoPauseStopsLoop() to report the timeout path, set the flag in the rejection path or have waitForCompletion() resolve a timeout marker.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test-acp-zed-bugs.mjs` at line 352, Update waitForCompletion() and
its race handling so timeout outcomes populate results.details.timedOut or
details.timeout. Ensure the timeout rejection path sets the marker, or resolve a
clearly identified timeout result, allowing expectTodoPauseStopsLoop() to enter
its timeout branch while preserving normal completion fields.
1 participant
Summary
Fixes two bugs that only manifest in ACP/Zed mode (not in CLI/interactive mode).
Closes #2653
Bug 1:
this.isRunning is not a functionintasktoolRoot cause:
initInteractiveScheduler()inpackages/agents/src/core/subagentExecution.tscreated a facade that copiedscheduler.schedulewithout binding it (schedule: scheduler.schedule). When called,thiswas the facade object (which has noisRunningmethod), causingCoreToolScheduler.schedule()to crash when it calledthis.isRunning().Why ACP-only: The CLI registers its own
schedulerFactoryProviderviainteractiveToolScheduler.ts, which uses a correct arrow-function closure. Zed/ACP has no such registration, so the fallback path ininitInteractiveSchedulerwas taken — the broken facade.Fix: Changed
schedule: scheduler.scheduleto an arrow-function closure:schedule: (request, signal) => scheduler.schedule(request, signal), preserving the original scheduler asthis.Bug 2:
todo_pausedoes not stop the continuation loopRoot cause:
AgenticLoop.buildNextMessage()unconditionally returnedcontinueLoop: truefor any successful tool — includingtodo_pause. In CLI mode, the React UI layer (useTodoContinuation.ts) has a separate continuation gate that checkstodoPaused. ACP/Zed has no such outer gate, so the loop continued aftertodo_pause.Fix: Added
hasSuccessfulTodoPause()check inbuildNextMessage()that detects a successfultodo_pausecompletion and returnscontinueLoop: false, recording tool history viarecordCompletedToolHistory()before stopping.Changes
Production code (
packages/agents)subagentExecution.ts: Arrow-function closure to preserve scheduler receiveragenticLoop/AgenticLoop.ts:hasSuccessfulTodoPause()check inbuildNextMessage()agenticLoop/loopHelpers.ts: ExportedrecordCompletedToolHistoryfor terminal tool historyTests (
packages/agents)subagentExecution.scheduler-receiver.test.ts: 2 tests verifying scheduler receiver preservation (uses regular prototype method, verified to catch the bug when fix is reverted)agenticLoop/__tests__/agenticLoop.todoPause.test.ts: 4 behavioral tests (successful pause stops loop, failed pause continues, multi-tool batch, history persistence)Test infrastructure (
scripts/)test-acp-zed-bugs.mjs: Cross-platform ACP reproduction scriptsetup-zed-agent.mjs: Cross-platform Zed agent configuration (hardened per OCR review)acp-logging-proxy.mjs: ACP traffic logging proxy for Zed testingVerification
stopReason: end_turnSummary by CodeRabbit