feat(harness): multi-harness adapter pattern for analysis providers#54
Open
serrrfirat wants to merge 5 commits into
Open
feat(harness): multi-harness adapter pattern for analysis providers#54serrrfirat wants to merge 5 commits into
serrrfirat wants to merge 5 commits into
Conversation
Introduce a pluggable harness adapter system supporting Claude Code, OpenClaw, Codex, Cursor Agent, OpenCode, and Amp — with ordered fallback chains. This consolidates ~1000 lines of duplicated provider-branching logic from index.ts and daemon.ts into a single shared `runAnalysisWithHarness()` entrypoint. Key changes: - New `src/harness/` module: types, parsers, registry, chain resolver, runner - 6 adapter implementations with spawn error handling and EPIPE guards - CLI arg size validation for cursor/opencode (OS execve limit) - `--analysis-harness` config option with backward-compat `analysisProvider` - Harness invocation telemetry (`analysis_harness_invocation` event) - 93 tests passing (38 new harness tests + 55 existing) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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:
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughIntroduces a harness-based orchestration system for executing multiple AI code analysis tools (Claude Code, OpenClaw, Codex, Cursor, OpenCode, Amp). Adds tool-specific adapters, a shared registry and executor, output parsers, and integrates harness chain resolution into core configuration and analysis flows, replacing the previous provider-centric approach. Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Daemon
participant Config as Config
participant Registry as Harness Registry
participant Chain as Chain Executor
participant Adapter as Tool Adapter
participant Tool as External Tool
participant Parser as Output Parser
Client->>Config: Get analysisHarness or analysisProvider
Config-->>Client: Return harness chain
Client->>Registry: resolveHarnessChain(config, openclawEnabled)
Registry-->>Client: HarnessId[] (fallback chain)
Client->>Chain: runAnalysisWithHarness(prompt, harnessChain)
loop For each harness in chain
Chain->>Adapter: getHarness(id)
Registry-->>Chain: HarnessAdapter instance
Chain->>Adapter: isAvailable()
alt Unavailable
Adapter-->>Chain: false
Chain->>Chain: Skip to next harness
else Available
Adapter-->>Chain: true
Chain->>Adapter: run({prompt, timeout})
Adapter->>Tool: findBinary() → spawn with args
Adapter->>Tool: Feed prompt to stdin
Tool-->>Adapter: stdout/stderr
Adapter->>Parser: extractText(stdout)
Parser-->>Adapter: Parsed text & metadata
Adapter-->>Chain: HarnessRunResult {ok, rawText, metadata}
alt Success or Partial Result
Chain-->>Client: Return result with harness used
Note over Chain: Stop iteration on success/partial
else Failure
Chain->>Chain: Collect error, continue loop
end
end
end
alt All harnesses failed/unavailable
Chain-->>Client: Error result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @serrrfirat, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the analysis provider system by introducing a flexible, pluggable harness architecture. This change allows for seamless integration and ordered fallback across multiple AI analysis tools, centralizing previously dispersed logic and enhancing the system's configurability and resilience. The new pattern simplifies the addition of future analysis providers and improves the overall robustness of the analysis process. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a well-structured harness adapter system for analysis providers, which significantly improves code maintainability by consolidating provider-specific logic into a pluggable architecture. The implementation includes support for six different providers with ordered fallback chains and maintains backward compatibility for legacy configuration flags. The addition of comprehensive tests and the optimization of telemetry processing in synapse-db.ts are also noteworthy improvements that enhance the robustness of the system.
Comment on lines
+82
to
+88
| else if (parsed.type === 'assistant' && parsed.message?.content) { | ||
| for (const block of parsed.message.content) { | ||
| if (block.type === 'text' && block.text) { | ||
| fullText = block.text; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
In the assistant message parser, the loop over content blocks overwrites fullText in each iteration. If an assistant message contains multiple text blocks, only the last one will be preserved. The blocks should be accumulated instead, and then assigned to fullText to maintain the intended behavior of using the assistant message as a complete backup.
else if (parsed.type === 'assistant' && parsed.message?.content) {
let assistantText = '';
for (const block of parsed.message.content) {
if (block.type === 'text' && block.text) {
assistantText += block.text;
}
}
if (assistantText) fullText = assistantText;
}
Comment on lines
+1747
to
+1763
| for (const r of insightEventRows) { | ||
| const event: ParsedEvent = { | ||
| eventType: r.eventType, | ||
| metadata: r.metadata ? (JSON.parse(r.metadata) as Record<string, unknown>) : {}, | ||
| }; | ||
| switch (event.eventType) { | ||
| case 'insight_ranked': | ||
| ranked.push(event); | ||
| break; | ||
| case 'insight_selected_for_surface': | ||
| selected.push(event); | ||
| break; | ||
| case 'insight_suppressed': | ||
| suppressed.push(event); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
The JSON.parse call on r.metadata is not wrapped in a try-catch block. If the database contains a row with corrupted or invalid JSON metadata, this loop will throw an exception and cause the entire getInsightQualityMetrics function to fail. Adding error handling here would make the telemetry reporting more robust against data corruption.
for (const r of insightEventRows) {
try {
const event: ParsedEvent = {
eventType: r.eventType,
metadata: r.metadata ? (JSON.parse(r.metadata) as Record<string, unknown>) : {},
};
switch (event.eventType) {
case 'insight_ranked':
ranked.push(event);
break;
case 'insight_selected_for_surface':
selected.push(event);
break;
case 'insight_suppressed':
suppressed.push(event);
break;
}
} catch (e) {
// Skip events with corrupted metadata
continue;
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/insight-ranker.ts (1)
217-233:⚠️ Potential issue | 🟠 Major
keepdiverges from final selection after exploration/fallback.
recordRankingTelemetry(and any consumer ofranked.keep) will misreport if exploration swaps in a novel candidate or fallback selects below-threshold items. AfterselectWithExplorationGuarantee,keepshould reflect the final selection.✅ Fix by re-syncing keep with the final selection
const selectedSet = new Set(selected.map(entry => entry.rank)); + ranked.forEach(entry => { + entry.keep = selectedSet.has(entry.rank); + }); const suppressed = ranked.filter(entry => !selectedSet.has(entry.rank));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/insight-ranker.ts` around lines 217 - 233, After calling selectWithExplorationGuarantee (and after the fallback branch) the ranked[].keep flags may no longer match the final selected array; update the original ranked entries so their keep property reflects the final selection by building the selectedSet (from selected.map(e => e.rank)) and iterating ranked to set entry.keep = selectedSet.has(entry.rank) (and adjust any related suppressionReasons only if you changed keep for a fallback entry), ensuring recordRankingTelemetry and other consumers read the correct final keep values.
🧹 Nitpick comments (3)
src/harness/adapters/opencode.ts (1)
62-116: Consider capturing stderr for better diagnostics.Similar to the Amp adapter, stderr isn't captured. If OpenCode outputs diagnostic information to stderr on failure, it won't be included in error messages.
Also, since the prompt is passed via CLI args rather than stdin, the
stdio: ['pipe', ...]for stdin is unused. This is harmless but could be simplified to'ignore'.♻️ Optional: Simplify stdin and capture stderr
const child = spawn(binary, args, { cwd: options.cwd || process.cwd(), - stdio: ['pipe', 'pipe', 'pipe'], + stdio: ['ignore', 'pipe', 'pipe'], }); const timeoutId = setTimeout(() => { @@ ... @@ const outputChunks: Buffer[] = []; + const stderrChunks: Buffer[] = []; child.stdout.on('data', (data) => { outputChunks.push(Buffer.from(data)); }); + child.stderr.on('data', (data) => { + stderrChunks.push(Buffer.from(data)); + }); child.on('close', (code) => { if (resolved) return; resolved = true; clearTimeout(timeoutId); const output = Buffer.concat(outputChunks).toString('utf-8'); const durationMs = Date.now() - startMs; if (code !== 0) { + const stderr = Buffer.concat(stderrChunks).toString('utf-8').trim(); resolve({ ok: false, - error: `OpenCode exited with code ${code}`, + error: `OpenCode exited with code ${code}${stderr ? `: ${stderr.slice(0, 300)}` : ''}`, durationMs, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/harness/adapters/opencode.ts` around lines 62 - 116, The spawn call in the OpenCode adapter currently ignores stderr and leaves stdin as 'pipe'; update the child process handling in the Promise returned by the function that spawns OpenCode so stderr is captured and included in error diagnostics (add a stderrChunks Buffer array, listen on child.stderr 'data' and push into it, and include the concatenated stderr text in error responses like the child.on('error') and non-zero-exit resolve payloads), and simplify stdio to not keep stdin as a pipe (use 'ignore' for stdin) when calling spawn; refer to the existing variables/handlers (child, outputChunks, timeoutId, startMs, extractTextFromOpenCodeOutput) to locate where to add stderr aggregation and include it in error messages.src/harness/adapters/amp.ts (1)
68-109: Consider capturing stderr for more informative error messages.The adapter captures stdout but doesn't collect stderr. If Amp outputs diagnostic information or error details to stderr, they won't be included in the error message when the process exits with a non-zero code.
♻️ Proposed enhancement to capture stderr
const outputChunks: Buffer[] = []; + const stderrChunks: Buffer[] = []; child.on('error', (err) => { @@ ... @@ child.stdout.on('data', (data) => { outputChunks.push(Buffer.from(data)); }); + child.stderr.on('data', (data) => { + stderrChunks.push(Buffer.from(data)); + }); child.on('close', (code) => { if (resolved) return; resolved = true; clearTimeout(timeoutId); const output = Buffer.concat(outputChunks).toString('utf-8'); const durationMs = Date.now() - startMs; if (code !== 0) { + const stderr = Buffer.concat(stderrChunks).toString('utf-8').trim(); resolve({ ok: false, - error: `Amp exited with code ${code}`, + error: `Amp exited with code ${code}${stderr ? `: ${stderr.slice(0, 300)}` : ''}`, durationMs, }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/harness/adapters/amp.ts` around lines 68 - 109, The adapter currently only captures stdout (outputChunks) and ignores stderr, so when the spawned process (child) fails you should also collect stderr and include it in error responses; add a stderrChunks: Buffer[] (or similar) and push Buffer.from(data) on child.stderr 'data', then when resolving on non-zero exit (in the child.on('close') handler) include the collected stderr text (joined/trimmed) in the returned error message or metadata (e.g., append to the error field or set metadata.stderr) and also include stderr where child.on('error') resolves; reference the existing symbols child, outputChunks, extractTextFromAmpOutput, and the child.on('close')/child.on('error') handlers to locate where to add stderr collection and include it in error responses.src/index.ts (1)
768-786: Consider using a stricter type cast.The validation ensures only valid harness IDs are saved, but the
as any[]cast on line 783 loses type safety. SinceVALID_HARNESS_IDSalready enforces valid values, casting toHarnessId[]would be more precise.♻️ Suggested improvement
- saveConfig({ analysisHarness: rawIds as any[] }); + saveConfig({ analysisHarness: rawIds as HarnessId[] });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 768 - 786, The cast to any[] when saving validated harness IDs weakens type safety; replace the broad cast with the correct HarnessId[] type. In the block where options.analysisHarness is handled (symbols: VALID_HARNESS_IDS, rawIds, saveConfig), assert or convert rawIds to HarnessId[] (e.g., via a typed map or an explicit cast to HarnessId[] after validation) and pass that to saveConfig so only HarnessId[] is saved instead of any[].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/harness/adapters/codex.ts`:
- Around line 52-66: The code writes to child.stdin without checking it, which
can crash if spawn fails and child.stdin is null; change the logic in
src/harness/adapters/codex.ts (and mirror in claude-code.ts and amp.ts) to
attach the child.stdin 'error' handler before any writes and guard child.stdin
!== null before calling child.stdin.write(options.prompt) and child.stdin.end();
if stdin is null, handle the spawn failure path (clear timeout, kill child if
needed and resolve/reject the promise consistently) so the process falls back
instead of throwing a null-deref.
In `@src/harness/index.ts`:
- Around line 109-143: The loop currently calls adapter.run(...) without
guarding against exceptions, so a thrown error aborts the fallback chain; wrap
the adapter.run invocation in a try/catch (around the await adapter.run call in
the harnessChain loop), and on catch convert the exception into a failure: push
a descriptive entry like `${harnessId}: ${err?.message || String(err)}` onto the
errors array (or set result = { ok: false, error: ... }) and continue the loop
so the next harness is attempted; preserve the existing behavior that only when
result.rawText exists you return immediately, but ensure thrown exceptions do
not short-circuit the chain.
---
Outside diff comments:
In `@src/insight-ranker.ts`:
- Around line 217-233: After calling selectWithExplorationGuarantee (and after
the fallback branch) the ranked[].keep flags may no longer match the final
selected array; update the original ranked entries so their keep property
reflects the final selection by building the selectedSet (from selected.map(e =>
e.rank)) and iterating ranked to set entry.keep = selectedSet.has(entry.rank)
(and adjust any related suppressionReasons only if you changed keep for a
fallback entry), ensuring recordRankingTelemetry and other consumers read the
correct final keep values.
---
Nitpick comments:
In `@src/harness/adapters/amp.ts`:
- Around line 68-109: The adapter currently only captures stdout (outputChunks)
and ignores stderr, so when the spawned process (child) fails you should also
collect stderr and include it in error responses; add a stderrChunks: Buffer[]
(or similar) and push Buffer.from(data) on child.stderr 'data', then when
resolving on non-zero exit (in the child.on('close') handler) include the
collected stderr text (joined/trimmed) in the returned error message or metadata
(e.g., append to the error field or set metadata.stderr) and also include stderr
where child.on('error') resolves; reference the existing symbols child,
outputChunks, extractTextFromAmpOutput, and the
child.on('close')/child.on('error') handlers to locate where to add stderr
collection and include it in error responses.
In `@src/harness/adapters/opencode.ts`:
- Around line 62-116: The spawn call in the OpenCode adapter currently ignores
stderr and leaves stdin as 'pipe'; update the child process handling in the
Promise returned by the function that spawns OpenCode so stderr is captured and
included in error diagnostics (add a stderrChunks Buffer array, listen on
child.stderr 'data' and push into it, and include the concatenated stderr text
in error responses like the child.on('error') and non-zero-exit resolve
payloads), and simplify stdio to not keep stdin as a pipe (use 'ignore' for
stdin) when calling spawn; refer to the existing variables/handlers (child,
outputChunks, timeoutId, startMs, extractTextFromOpenCodeOutput) to locate where
to add stderr aggregation and include it in error messages.
In `@src/index.ts`:
- Around line 768-786: The cast to any[] when saving validated harness IDs
weakens type safety; replace the broad cast with the correct HarnessId[] type.
In the block where options.analysisHarness is handled (symbols:
VALID_HARNESS_IDS, rawIds, saveConfig), assert or convert rawIds to HarnessId[]
(e.g., via a typed map or an explicit cast to HarnessId[] after validation) and
pass that to saveConfig so only HarnessId[] is saved instead of any[].
Comment on lines
+52
to
+66
| return new Promise((resolve) => { | ||
| const child = spawn(binary, args, { | ||
| cwd: options.cwd || process.cwd(), | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| }); | ||
|
|
||
| const timeoutId = setTimeout(() => { | ||
| try { child.kill('SIGTERM'); } catch { /* already dead */ } | ||
| }, timeoutMs); | ||
|
|
||
| let resolved = false; | ||
|
|
||
| child.stdin.on('error', () => {}); // ignore EPIPE if child exits early | ||
| child.stdin.write(options.prompt); | ||
| child.stdin.end(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/harness/adapters/codex.ts | head -100Repository: serrrfirat/synapse-daemon
Length of output: 3555
🏁 Script executed:
rg -n "child\.stdin" src/harness/adapters -g '*.ts' -C 2Repository: serrrfirat/synapse-daemon
Length of output: 1420
🌐 Web query:
Node.js spawn child.stdin null on spawn failure
💡 Result:
In Node.js, child_process.spawn() always returns a ChildProcess object, but child.stdin (and .stdout/.stderr) can be null or undefined in two common cases:
-
You didn’t create a pipe for fd 0 (stdin)
If you spawn withstdio: 'inherit',stdio: 'ignore', or otherwise setstdio[0]to something other than'pipe', thenchild.stdinwill benull. Node documents: “If the child process was spawned withstdio[0]set to anything other than'pipe', then this will benull.” (nodejs.org) -
The process failed to spawn (e.g.,
ENOENT)
If the command can’t be started (missing executable, badcwd, permissions, etc.), Node emits an'error'event, and the.stdin/.stdout/.stderrproperties may benull/undefinedbecause there is no child process / pipes to attach to. Node documents: “Thesubprocess.stdinproperty can benullorundefinedif the child process could not be successfully spawned.” (nodejs.org)
What to do
- Always attach an
errorhandler and treat it as “spawn failed; streams may not exist”. - Only write to
child.stdinif it exists (or ensurestdio[0]is'pipe').
import { spawn } from "node:child_process";
const child = spawn("some-command", ["arg"], { stdio: ["pipe", "pipe", "pipe"] });
child.on("error", (err) => {
// spawn failed (e.g. ENOENT). stdin/stdout/stderr may be null/undefined.
console.error("spawn failed:", err.code, err.message);
});
if (child.stdin) {
child.stdin.write("input\n");
child.stdin.end();
}If you share stdin with the parent (e.g. stdio: 'inherit'), child.stdin being null is expected—there is no writable pipe exposed to you. (nodejs.org)
Citations:
- 1: https://nodejs.org/download/release/v22.17.1/docs/api/child_process.html?utm_source=openai
- 2: https://nodejs.org/download/release/v22.17.1/docs/api/child_process.html?utm_source=openai
- 3: https://nodejs.org/download/release/v22.17.1/docs/api/child_process.html?utm_source=openai
Guard child.stdin before writing to avoid null-deref on spawn failure.
If the binary is not found or spawn fails (e.g., ENOENT, permissions), Node.js can leave child.stdin as null even with stdio: ['pipe', 'pipe', 'pipe']. The current code attempts to write to it at lines 64–66 before the error handler is attached, causing a crash before fallback handling runs.
The same vulnerability exists in src/harness/adapters/claude-code.ts (lines 57–59) and src/harness/adapters/amp.ts (lines 64–66).
🛠️ Suggested fix
let resolved = false;
+ if (!child.stdin) {
+ resolved = true;
+ clearTimeout(timeoutId);
+ resolve({
+ ok: false,
+ error: 'Failed to spawn: stdin unavailable',
+ durationMs: Date.now() - startMs,
+ });
+ return;
+ }
+
child.stdin.on('error', () => {}); // ignore EPIPE if child exits early
child.stdin.write(options.prompt);
child.stdin.end();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/harness/adapters/codex.ts` around lines 52 - 66, The code writes to
child.stdin without checking it, which can crash if spawn fails and child.stdin
is null; change the logic in src/harness/adapters/codex.ts (and mirror in
claude-code.ts and amp.ts) to attach the child.stdin 'error' handler before any
writes and guard child.stdin !== null before calling
child.stdin.write(options.prompt) and child.stdin.end(); if stdin is null,
handle the spawn failure path (clear timeout, kill child if needed and
resolve/reject the promise consistently) so the process falls back instead of
throwing a null-deref.
serrrfirat
commented
Feb 20, 2026
serrrfirat
left a comment
serrrfirat
left a comment
Owner
Author
There was a problem hiding this comment.
Review: src/harness/adapters/cursor.ts
Bug: stderr pipe not consumed — potential deadlock
Severity: High
The adapter opens stderr as a pipe (stdio: ['pipe', 'pipe', 'pipe']) but never reads from it. If cursor-agent writes more than the OS pipe buffer size (~64KB on macOS) to stderr, it will block on the write and deadlock the process — it'll hang forever since the close event never fires.
The claude-code adapter handles this correctly by consuming stderrChunks. The fix here is either:
- Capture stderr (preferred — useful for error diagnostics):
Then include stderr in the error message on non-zero exit.
const stderrChunks: Buffer[] = []; child.stderr.on('data', (data) => { stderrChunks.push(Buffer.from(data)); });
- Or set stderr to
'ignore'or'inherit'in the spawn options if you don't need it.
This same issue exists in opencode.ts, amp.ts, and codex.ts.
Bug: stdin pipe opened but never closed
Severity: Medium
The spawn options open stdin as 'pipe', but the cursor adapter never writes to it or calls child.stdin.end(). The child process gets a dangling open stdin pipe. If cursor-agent ever reads from stdin (even accidentally), it will hang waiting for input.
Compare with amp.ts/codex.ts/claude-code.ts which all properly do:
child.stdin.write(options.prompt);
child.stdin.end();Since cursor passes the prompt via CLI args, it should either:
- Call
child.stdin.end()immediately to signal EOF, or - Set stdin to
'ignore'in the spawn options:stdio: ['ignore', 'pipe', 'pipe']
Same issue in opencode.ts.
Missing: No EPIPE guard on stdin
Severity: Low (since stdin isn't written to, but inconsistent with other adapters)
The PR description mentions "EPIPE guards on stdin-piping adapters," and amp.ts, codex.ts, and claude-code.ts all have:
child.stdin.on('error', () => {}); // ignore EPIPE if child exits earlycursor.ts (and opencode.ts) lack this. Not critical since they don't write to stdin, but if you fix the dangling-stdin issue above by closing it, you'd want the guard.
Nit: Prompt visible in process listing
Severity: Informational
The prompt is passed as a CLI argument (-p, options.prompt), making it visible to all users via ps aux. If the prompt contains sensitive codebase context, this is a minor privacy leak. The MAX_ARG_BYTES guard is good for preventing execve failures, but stdin piping (like claude-code/codex/amp do) would be more secure. This may just be a cursor-agent CLI limitation.
Nit: No stderr in error messages
On non-zero exit, the error only reports the exit code:
error: `Cursor Agent exited with code ${code}`,The claude-code adapter includes stderr in the error message (stderr.slice(0, 300)), which is much more useful for debugging. Even if you don't capture stderr as a buffer, at minimum inheriting it would let the user see what went wrong.
Nit: sessionId silently ignored
The HarnessRunOptions interface has a sessionId field. The cursor adapter silently ignores it. This is probably fine if cursor-agent doesn't support sessions, but a comment noting this limitation would help maintainability.
Design note: heavy duplication across adapters
cursor.ts, opencode.ts, amp.ts, and codex.ts are ~95% identical — same findBinary() pattern, same isAvailable(), same run() structure with only the binary name, CLI args, and parser differing. Not a blocker for this PR, but a base class or factory function could eliminate ~300 lines of near-duplicate code in a follow-up.
Summary
| Issue | Severity |
|---|---|
| stderr pipe not consumed (deadlock risk) | High |
| stdin pipe opened but never closed | Medium |
| Missing EPIPE guard (inconsistent) | Low |
| No stderr in error messages | Low |
| Prompt visible in process list | Informational |
sessionId silently ignored |
Informational |
| Heavy adapter duplication | Design debt |
The stderr deadlock is the main actionable issue — it can cause the adapter to hang indefinitely in production if cursor-agent is chatty on stderr.
serrrfirat
commented
Feb 20, 2026
serrrfirat
left a comment
serrrfirat
left a comment
Owner
Author
There was a problem hiding this comment.
Review: REQUEST CHANGES
Blocking Issues
-
HIGH: Daemon lost stale session error detection specificity — Old code checked for specific markers (
'no conversation found with session id','session not found'). New code doesharnessResult.error?.includes('session')— far too broad, matching any error containing "session", causing false-positive session resets and wasting tokens. -
HIGH: Daemon lost build proposal detection logic — Old code had extensive build proposal creation, skill gap detection, and semantic gap detection. New code calls
processStructuredAnalysis(extracted.analysis). Verify thatprocessStructuredAnalysisincludes ALL of this logic, or this is a feature regression. -
MEDIUM: Null exit code treated as success — Claude Code adapter coalesces
code ?? 0, so a SIGTERM-killed process (null exit code) is treated as successful with partial output. -
MEDIUM: Timeout handler does not force-resolve Promise — If
child.kill('SIGTERM')fails and process never exits, the Promise hangs forever. Add a fallback resolve after a grace period.
Non-Blocking Issues
- OpenClaw adapter missing
contextOverflowdetection saveConfig({ analysisHarness: rawIds as any[] })— unsafe cast, should beHarnessId[]- SIGTERM without SIGKILL escalation across all adapters
isAvailable()only checks file existence, not executability- PATH splitting uses
:— breaks on Windows (usepath.delimiter) - Missing stderr collection in non-Claude adapters
similarity()denominator change (same as other PRs)
…type safety - Consume stderr in all adapters (cursor, opencode, amp, codex) to prevent deadlock when child process writes >64KB to stderr; include stderr in error messages on non-zero exit - Change stdin to 'ignore' for cursor/opencode adapters since they pass prompts via CLI args and never write to stdin - Treat null exit code as failure in claude-code adapter (SIGTERM-killed process should not be treated as success) - Make stale session detection in daemon more specific: match known error strings instead of broad 'session' substring - Re-sync ranked[].keep flags after selectWithExplorationGuarantee to keep the ranked array consistent with the final selected set - Wrap adapter.run() in try/catch in harness chain loop so a thrown exception does not abort the fallback chain - Replace `as any[]` with `as HarnessId[]` for type safety - Use true Jaccard similarity (union denominator) in insight ranker Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/scripts/daemon.ts (1)
1368-1394:⚠️ Potential issue | 🟡 MinorGate the Claude-specific recovery hint by harness capability.
The recovery prompt hard-codes
claude-memand asearchMCP tool. On non-Claude harnesses this can produce unsupported tool calls or degrade responses. Consider only injecting it when the primary harness supports these tools (or make the hint provider-agnostic).Suggested fix
- if (isRecoverySession) { + const supportsClaudeMem = harnessChain[0] === 'claude_code'; + if (isRecoverySession && supportsClaudeMem) { const recoveryHint = ` **IMPORTANT: Session Recovery Mode** This is a fresh session after context overflow. You have access to claude-mem which has observations from previous Synapse analysis sessions. Before analyzing, use the \`search\` MCP tool to query "synapse analysis patterns insights" to retrieve relevant context from prior runs. This will help you maintain continuity and avoid re-suggesting patterns that were already handled. `; finalPrompt = recoveryHint + prompt; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/daemon.ts` around lines 1368 - 1394, The recoveryHint currently hardcodes claude-mem and the search MCP tool and should only be injected when the selected harness actually supports those capabilities; update the logic around resolveHarnessChain/harnessChain and isRecoverySession so that before setting finalPrompt you detect whether the primary harness (from harnessChain or an available harness capability object) supports claude-mem and the search MCP, and only prepend recoveryHint when those capabilities are present—otherwise inject a provider-agnostic hint or skip the tool-specific guidance; update references to resolveHarnessChain, harnessChain, isRecoverySession, finalPrompt, and recoveryHint accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/harness/adapters/cursor.ts`:
- Around line 69-114: The spawn result `child` may have null
`child.stdout`/`child.stderr` on spawn failure which causes TypeError when
attaching listeners; update the listener setup in the Cursor spawn block to
guard access (e.g., check `if (child.stdout) { child.stdout.on('data', ...) }`
and similarly for `child.stderr`) and ensure you still collect empty buffers
when streams are absent so the existing `close`/`error`/timeout handling
(variables `resolved`, `timeoutId`, `outputChunks`, `stderrChunks`) works
unchanged; keep the rest of the `child.on('error')` and `child.on('close')`
logic intact.
In `@src/harness/adapters/opencode.ts`:
- Around line 62-110: The child process streams (child.stdout / child.stderr)
must be null-checked before attaching listeners to avoid synchronous throws when
spawn fails; update the spawn handling in the Promise around the "child"
returned by spawn so that you only call child.stdout.on(...) and
child.stderr.on(...) if child.stdout/child.stderr are truthy, and if either
stream is missing immediately clearTimeout(timeoutId), mark resolved = true and
resolve with ok: false and an informative error (e.g., "Missing stdio streams
from spawned process") including durationMs; keep the existing child.on('error')
and child.on('close') behavior otherwise so failure paths remain consistent.
In `@src/scripts/daemon.ts`:
- Around line 1490-1541: The run currently treats failed structured extraction
as a successful run: after calling extractAnalysisFromResponseText it still
updates state.runCount/lastUsedAt, calls saveSessionState, recordAnalysisRun,
updates streaks and may send notifications; instead, change the control flow so
that when extracted.analysis is falsy you do not increment state.runCount or
update lastUsedAt and you do not call recordAnalysisRun,
getConsecutiveDaysStreak or sendNotification. Keep the existing retry/reset
logic (sessionId reset, saveSessionState when forcing a fresh session) and
always ensure synapseDb.close() runs, but move the state updates,
saveSessionState, recordAnalysisRun, streak check (getConsecutiveDaysStreak) and
reflection/notification logic into the branch where extracted.analysis exists
(after await processStructuredAnalysis) so only successful extractions are
recorded.
---
Outside diff comments:
In `@src/scripts/daemon.ts`:
- Around line 1368-1394: The recoveryHint currently hardcodes claude-mem and the
search MCP tool and should only be injected when the selected harness actually
supports those capabilities; update the logic around
resolveHarnessChain/harnessChain and isRecoverySession so that before setting
finalPrompt you detect whether the primary harness (from harnessChain or an
available harness capability object) supports claude-mem and the search MCP, and
only prepend recoveryHint when those capabilities are present—otherwise inject a
provider-agnostic hint or skip the tool-specific guidance; update references to
resolveHarnessChain, harnessChain, isRecoverySession, finalPrompt, and
recoveryHint accordingly.
---
Duplicate comments:
In `@src/harness/adapters/amp.ts`:
- Around line 62-88: The code assumes child.stdin/stdout/stderr are non-null and
writes/listens unguarded, which can crash on spawn failure; update the block
around child (the writes and 'error' handler attachments) to first null-check
each stdio before using it: only call child.stdin.on('error'),
child.stdin.write(options.prompt) and child.stdin.end() if child.stdin exists
(guard EPIPE handling), and only attach child.stdout.on('data', ...) and
child.stderr.on('data', ...) if child.stdout/child.stderr are non-null; keep the
existing child.on('error') logic but ensure it still clears timeout and resolves
when invoked even if stdio were skipped.
In `@src/harness/adapters/claude-code.ts`:
- Around line 42-63: Spawn can fail and leave stdio handles null, so guard uses
of child.stdin/stdout/stderr to avoid crashes: before calling
child.stdin.on('error', ...), child.stdin.write(options.prompt), or
child.stdin.end(), check if child.stdin is non-null; likewise when attaching
data listeners or collecting outputChunks/stderrChunks, check child.stdout and
child.stderr exist first. Update the code around spawn and the variables
outputChunks/stderrChunks to conditionally attach listeners and write only when
the corresponding handle is present (use simple if (child.stdin) { ... } and if
(child.stdout) { ... } etc.), and ensure any timeout/cleanup still runs if stdio
is missing so the Promise resolves/rejects appropriately.
In `@src/harness/adapters/codex.ts`:
- Around line 62-88: The code currently assumes child.stdio streams exist and
writes/listens unguarded, which can throw if spawn fails; update the block
around child to first check for and guard child.stdin before calling
child.stdin.on('error'), child.stdin.write(options.prompt) and
child.stdin.end(), and only attach 'data' handlers to child.stdout and
child.stderr if those properties are non-null/defined; keep the existing
child.on('error') logic (using resolved, timeoutId, startMs) unchanged but
ensure the guarded checks prevent crashes when spawn returns null/undefined
stdio.
Comment on lines
+69
to
+114
| const child = spawn(binary, args, { | ||
| cwd: options.cwd || process.cwd(), | ||
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| }); | ||
|
|
||
| const timeoutId = setTimeout(() => { | ||
| try { child.kill('SIGTERM'); } catch { /* already dead */ } | ||
| }, timeoutMs); | ||
|
|
||
| child.on('error', (err) => { | ||
| if (resolved) return; | ||
| resolved = true; | ||
| clearTimeout(timeoutId); | ||
| resolve({ | ||
| ok: false, | ||
| error: `Failed to spawn Cursor Agent: ${err.message}`, | ||
| durationMs: Date.now() - startMs, | ||
| }); | ||
| }); | ||
|
|
||
| const outputChunks: Buffer[] = []; | ||
| const stderrChunks: Buffer[] = []; | ||
|
|
||
| child.stdout.on('data', (data) => { | ||
| outputChunks.push(Buffer.from(data)); | ||
| }); | ||
|
|
||
| child.stderr.on('data', (data) => { | ||
| stderrChunks.push(Buffer.from(data)); | ||
| }); | ||
|
|
||
| child.on('close', (code) => { | ||
| if (resolved) return; | ||
| resolved = true; | ||
| clearTimeout(timeoutId); | ||
| const output = Buffer.concat(outputChunks).toString('utf-8'); | ||
| const durationMs = Date.now() - startMs; | ||
|
|
||
| if (code !== 0) { | ||
| const stderr = Buffer.concat(stderrChunks).toString('utf-8').trim(); | ||
| resolve({ | ||
| ok: false, | ||
| error: `Cursor Agent exited with code ${code}${stderr ? ': ' + stderr.slice(0, 300) : ''}`, | ||
| durationMs, | ||
| }); | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -type f -name "cursor.ts" 2>/dev/null | head -5Repository: serrrfirat/synapse-daemon
Length of output: 51
🏁 Script executed:
# Search for the cursor.ts file
find . -type f -name "cursor.ts" 2>/dev/nullRepository: serrrfirat/synapse-daemon
Length of output: 101
🏁 Script executed:
# List the structure to understand the repository
git ls-files | grep -E "(cursor|adapter)" | head -20Repository: serrrfirat/synapse-daemon
Length of output: 259
🏁 Script executed:
wc -l ./src/harness/adapters/cursor.tsRepository: serrrfirat/synapse-daemon
Length of output: 105
🏁 Script executed:
cat -n ./src/harness/adapters/cursor.ts | sed -n '60,120p'Repository: serrrfirat/synapse-daemon
Length of output: 2274
🌐 Web query:
Node.js spawn stdio pipe stdout stderr null when can be null
💡 Result:
In Node.js, child.stdout / child.stderr from child_process.spawn() are only non-null when the corresponding stdio entry is 'pipe'.
They can be:
Readablestream: whenstdio[1] === 'pipe'(forstdout) and/orstdio[2] === 'pipe'(forstderr) (this is the default if you don’t overridestdio). [1]null: when you spawn with that fd set to anything other than'pipe'—e.g.'inherit','ignore', a file descriptor number, or a stream. [1]undefined: if the child process could not be successfully spawned (spawn failure). [1]
So if you set stdio: 'inherit' (or otherwise don’t pipe fd 1/2), child.stdout / child.stderr will be null. [1]
🏁 Script executed:
# Check similar patterns in other adapter files
grep -A 10 "spawn(" ./src/harness/adapters/*.ts | grep -E "(stdout|stderr)" | head -20Repository: serrrfirat/synapse-daemon
Length of output: 51
🌐 Web query:
Node.js spawn error event timing child process stdio pipe stream initialization
💡 Result:
In child_process.spawn(), the ChildProcess object is returned immediately, but a failed spawn is reported asynchronously via the child’s 'error' event (e.g., ENOENT when the command or cwd doesn’t exist). (nodejs.org)
Event ordering (reliable rules)
- Success path:
'spawn'is emitted first, before anystdout/stderrdata events. (nodejs.org) - Failure-to-spawn path:
'spawn'is not emitted; instead'error'is emitted. (nodejs.org) - If the child fails to spawn,
'close'will be emitted after'error'. (nodejs.org) - After an
'error','exit'may or may not fire. (nodejs.org)
Stdio pipe stream initialization implications
Even if you request stdio: 'pipe', the stdio handles may not be usable when spawn fails:
subprocess.stdiocan beundefinedif the child could not be spawned. (nodejs.org)subprocess.stdout/stdin/stderrcan benullorundefinedif the child could not be spawned. (nodejs.org)
So, don’t assume child.stdout exists right after spawn()—guard it (or attach stream listeners after you know spawn succeeded, e.g., in 'spawn', or check if (child.stdout) ...). (nodejs.org)
Citations:
- 1: https://nodejs.org/download/release/v22.19.0/docs/api/child_process.html?utm_source=openai
- 2: https://nodejs.org/download/release/v22.19.0/docs/api/child_process.html?utm_source=openai
- 3: https://nodejs.org/download/release/v22.19.0/docs/api/child_process.html?utm_source=openai
- 4: https://nodejs.org/download/release/v22.19.0/docs/api/child_process.html?utm_source=openai
- 5: https://nodejs.org/download/release/v22.19.0/docs/api/child_process.html
- 6: https://nodejs.org/download/release/v22.19.0/docs/api/child_process.html
- 7: https://nodejs.org/download/release/v22.19.0/docs/api/child_process.html
- 8: https://nodejs.org/download/release/v22.19.0/docs/api/child_process.html
🏁 Script executed:
# Check if there are other error handling patterns in the codebase
grep -r "child.stdout" ./src --include="*.ts" -B 2 -A 2Repository: serrrfirat/synapse-daemon
Length of output: 2251
Guard stdout/stderr before attaching listeners.
On spawn failure, child.stdout and child.stderr can be null; attempting to attach listeners on null streams will throw a TypeError and leave the promise unresolved.
🛠️ Proposed fix
const timeoutId = setTimeout(() => {
try { child.kill('SIGTERM'); } catch { /* already dead */ }
}, timeoutMs);
+ if (!child.stdout || !child.stderr) {
+ resolved = true;
+ clearTimeout(timeoutId);
+ resolve({
+ ok: false,
+ error: 'Failed to spawn Cursor Agent: stdio unavailable',
+ durationMs: Date.now() - startMs,
+ });
+ return;
+ }
+
child.on('error', (err) => {
if (resolved) return;
resolved = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/harness/adapters/cursor.ts` around lines 69 - 114, The spawn result
`child` may have null `child.stdout`/`child.stderr` on spawn failure which
causes TypeError when attaching listeners; update the listener setup in the
Cursor spawn block to guard access (e.g., check `if (child.stdout) {
child.stdout.on('data', ...) }` and similarly for `child.stderr`) and ensure you
still collect empty buffers when streams are absent so the existing
`close`/`error`/timeout handling (variables `resolved`, `timeoutId`,
`outputChunks`, `stderrChunks`) works unchanged; keep the rest of the
`child.on('error')` and `child.on('close')` logic intact.
Comment on lines
+62
to
+110
| return new Promise((resolve) => { | ||
| let resolved = false; | ||
|
|
||
| const child = spawn(binary, args, { | ||
| cwd: options.cwd || process.cwd(), | ||
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| }); | ||
|
|
||
| const timeoutId = setTimeout(() => { | ||
| try { child.kill('SIGTERM'); } catch { /* already dead */ } | ||
| }, timeoutMs); | ||
|
|
||
| child.on('error', (err) => { | ||
| if (resolved) return; | ||
| resolved = true; | ||
| clearTimeout(timeoutId); | ||
| resolve({ | ||
| ok: false, | ||
| error: `Failed to spawn OpenCode: ${err.message}`, | ||
| durationMs: Date.now() - startMs, | ||
| }); | ||
| }); | ||
|
|
||
| const outputChunks: Buffer[] = []; | ||
| const stderrChunks: Buffer[] = []; | ||
|
|
||
| child.stdout.on('data', (data) => { | ||
| outputChunks.push(Buffer.from(data)); | ||
| }); | ||
|
|
||
| child.stderr.on('data', (data) => { | ||
| stderrChunks.push(Buffer.from(data)); | ||
| }); | ||
|
|
||
| child.on('close', (code) => { | ||
| if (resolved) return; | ||
| resolved = true; | ||
| clearTimeout(timeoutId); | ||
| const output = Buffer.concat(outputChunks).toString('utf-8'); | ||
| const durationMs = Date.now() - startMs; | ||
|
|
||
| if (code !== 0) { | ||
| const stderr = Buffer.concat(stderrChunks).toString('utf-8').trim(); | ||
| resolve({ | ||
| ok: false, | ||
| error: `OpenCode exited with code ${code}${stderr ? ': ' + stderr.slice(0, 300) : ''}`, | ||
| durationMs, | ||
| }); | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/harness/adapters/opencode.ts | head -150Repository: serrrfirat/synapse-daemon
Length of output: 4607
🌐 Web query:
Node.js child_process spawn stdio pipe stdout stderr null conditions
💡 Result:
In child_process.spawn(cmd, args, { stdio }), the stdio option controls what the child’s fds (0=stdin, 1=stdout, 2=stderr, …) are connected to.
When are child.stdout / child.stderr a stream vs null?
child.stdoutis a Readable stream only ifstdio[1] === 'pipe'. Otherwise it isnull. [1]child.stderris a Readable stream only ifstdio[2] === 'pipe'. Otherwise it isnull. [1]- Both are aliases of
child.stdio[1]/child.stdio[2]. [1]
What does 'pipe' mean?
'pipe'creates a pipe between parent and child, exposing the parent end aschild.stdio[fd](and for fds 0–2 also aschild.stdin/stdout/stderr). [2]
What do null / undefined mean in stdio?
nullorundefinedmeans “use the default”:- For fds 0, 1, 2: default is create a pipe.
- For fd 3 and up: default is
'ignore'. [1]
Why might you see undefined?
- If the child process could not be successfully spawned,
subprocess.stdio(and therefore.stdout/.stderr) can beundefined. [1]
Example
spawn('prg', [], { stdio: ['pipe', null, null] })→ stdout/stderr default to pipes, sochild.stdoutandchild.stderrare streams. [1]
Sources: Node.js child_process docs (v22.17.1). [1] and background semantics for 'pipe'. [2]
[1] (nodejs.org)
[2] (nodejs.org)
Citations:
- 1: https://nodejs.org/download/release/v22.17.1/docs/api/child_process.html?utm_source=openai
- 2: https://nodejs.org/download/release/v0.8.20/docs/api/child_process.html?utm_source=openai
🌐 Web query:
Node.js spawn stdio undefined when spawn fails error event timing
💡 Result:
When child_process.spawn() fails before the OS process is actually created (e.g., ENOENT, EACCES), Node still returns a ChildProcess object, but its stdio streams may not exist yet—subprocess.stdio can be undefined, and subprocess.stdin/stdout/stderr can be undefined (or null depending on stdio config). This is explicitly documented. [1]
Event timing/ordering in this failure case:
- No
'spawn'event is emitted if the child didn’t spawn successfully; instead'error'is emitted. [2] - After an
'error','exit'may or may not fire, so don’t rely on it for cleanup. [2] 'close'will be emitted after'exit'(if it happens) or after'error'if the child failed to spawn. [2]
Practical pattern to avoid “stdio undefined” bugs:
- Attach an
'error'handler immediately after callingspawn(). - Only touch
cp.stdio/cp.stdout/cp.stderrafter:- you’ve seen the
'spawn'event, or - you’ve checked they’re not
undefined/null.
- you’ve seen the
Also: if spawning fails, cp.pid is undefined. [2]
Sources:
[1] Node.js docs: subprocess.stdio/stdin/stdout/stderr can be undefined if spawn fails https://nodejs.org/download/release/v22.17.1/docs/api/child_process.html
[2] Node.js docs: 'error', 'spawn', 'exit'/'close' behavior and ordering https://nodejs.org/download/release/v22.19.0/docs/api/child_process.html
Guard child.stdout and child.stderr before attaching listeners.
On spawn failure (e.g., binary not found, permission denied), child.stdout/child.stderr can be undefined. The current code attaches listeners synchronously at lines 88–94 without null checks, which will throw before the async error handler at line 74 fires.
🛠️ Proposed fix
const timeoutId = setTimeout(() => {
try { child.kill('SIGTERM'); } catch { /* already dead */ }
}, timeoutMs);
+ if (!child.stdout || !child.stderr) {
+ resolved = true;
+ clearTimeout(timeoutId);
+ resolve({
+ ok: false,
+ error: 'Failed to spawn OpenCode: stdio unavailable',
+ durationMs: Date.now() - startMs,
+ });
+ return;
+ }
+
child.on('error', (err) => {
if (resolved) return;
resolved = true;📝 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.
Suggested change
| return new Promise((resolve) => { | |
| let resolved = false; | |
| const child = spawn(binary, args, { | |
| cwd: options.cwd || process.cwd(), | |
| stdio: ['ignore', 'pipe', 'pipe'], | |
| }); | |
| const timeoutId = setTimeout(() => { | |
| try { child.kill('SIGTERM'); } catch { /* already dead */ } | |
| }, timeoutMs); | |
| child.on('error', (err) => { | |
| if (resolved) return; | |
| resolved = true; | |
| clearTimeout(timeoutId); | |
| resolve({ | |
| ok: false, | |
| error: `Failed to spawn OpenCode: ${err.message}`, | |
| durationMs: Date.now() - startMs, | |
| }); | |
| }); | |
| const outputChunks: Buffer[] = []; | |
| const stderrChunks: Buffer[] = []; | |
| child.stdout.on('data', (data) => { | |
| outputChunks.push(Buffer.from(data)); | |
| }); | |
| child.stderr.on('data', (data) => { | |
| stderrChunks.push(Buffer.from(data)); | |
| }); | |
| child.on('close', (code) => { | |
| if (resolved) return; | |
| resolved = true; | |
| clearTimeout(timeoutId); | |
| const output = Buffer.concat(outputChunks).toString('utf-8'); | |
| const durationMs = Date.now() - startMs; | |
| if (code !== 0) { | |
| const stderr = Buffer.concat(stderrChunks).toString('utf-8').trim(); | |
| resolve({ | |
| ok: false, | |
| error: `OpenCode exited with code ${code}${stderr ? ': ' + stderr.slice(0, 300) : ''}`, | |
| durationMs, | |
| }); | |
| return; | |
| return new Promise((resolve) => { | |
| let resolved = false; | |
| const child = spawn(binary, args, { | |
| cwd: options.cwd || process.cwd(), | |
| stdio: ['ignore', 'pipe', 'pipe'], | |
| }); | |
| const timeoutId = setTimeout(() => { | |
| try { child.kill('SIGTERM'); } catch { /* already dead */ } | |
| }, timeoutMs); | |
| if (!child.stdout || !child.stderr) { | |
| resolved = true; | |
| clearTimeout(timeoutId); | |
| resolve({ | |
| ok: false, | |
| error: 'Failed to spawn OpenCode: stdio unavailable', | |
| durationMs: Date.now() - startMs, | |
| }); | |
| return; | |
| } | |
| child.on('error', (err) => { | |
| if (resolved) return; | |
| resolved = true; | |
| clearTimeout(timeoutId); | |
| resolve({ | |
| ok: false, | |
| error: `Failed to spawn OpenCode: ${err.message}`, | |
| durationMs: Date.now() - startMs, | |
| }); | |
| }); | |
| const outputChunks: Buffer[] = []; | |
| const stderrChunks: Buffer[] = []; | |
| child.stdout.on('data', (data) => { | |
| outputChunks.push(Buffer.from(data)); | |
| }); | |
| child.stderr.on('data', (data) => { | |
| stderrChunks.push(Buffer.from(data)); | |
| }); | |
| child.on('close', (code) => { | |
| if (resolved) return; | |
| resolved = true; | |
| clearTimeout(timeoutId); | |
| const output = Buffer.concat(outputChunks).toString('utf-8'); | |
| const durationMs = Date.now() - startMs; | |
| if (code !== 0) { | |
| const stderr = Buffer.concat(stderrChunks).toString('utf-8').trim(); | |
| resolve({ | |
| ok: false, | |
| error: `OpenCode exited with code ${code}${stderr ? ': ' + stderr.slice(0, 300) : ''}`, | |
| durationMs, | |
| }); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/harness/adapters/opencode.ts` around lines 62 - 110, The child process
streams (child.stdout / child.stderr) must be null-checked before attaching
listeners to avoid synchronous throws when spawn fails; update the spawn
handling in the Promise around the "child" returned by spawn so that you only
call child.stdout.on(...) and child.stderr.on(...) if child.stdout/child.stderr
are truthy, and if either stream is missing immediately clearTimeout(timeoutId),
mark resolved = true and resolve with ok: false and an informative error (e.g.,
"Missing stdio streams from spawned process") including durationMs; keep the
existing child.on('error') and child.on('close') behavior otherwise so failure
paths remain consistent.
Comment on lines
+1490
to
1541
| // ── Extract structured analysis ─────────────────────────────────── | ||
| const responseText = harnessResult.rawText || ''; | ||
| const extracted = extractAnalysisFromResponseText(responseText); | ||
|
|
||
| // Detect context overflow errors | ||
| const errorStr = String(e).toLowerCase(); | ||
| const isContextOverflow = | ||
| outputLower.includes('prompt is too long') || | ||
| outputLower.includes('context length') || | ||
| outputLower.includes('maximum context') || | ||
| errorStr.includes('unterminated string') || // JSON parse error often from truncation | ||
| (code !== 0 && state.runCount >= MAX_SESSION_RUNS); | ||
|
|
||
| if (isContextOverflow && state.sessionId && !isRetry) { | ||
| // Check if we already retried recently (within 1 hour) to prevent token waste | ||
| const lastRetry = state.lastRetryAt ? new Date(state.lastRetryAt).getTime() : 0; | ||
| const hourAgo = Date.now() - 60 * 60 * 1000; | ||
|
|
||
| if (lastRetry > hourAgo) { | ||
| log('CONTEXT OVERFLOW - already retried within the hour, skipping to prevent token waste'); | ||
| } else { | ||
| log('CONTEXT OVERFLOW DETECTED - retrying once with fresh session'); | ||
| // Clear session to force fresh start | ||
| state.sessionId = null; | ||
| state.runCount = 0; | ||
| state.contextOverflow = false; | ||
| state.lastRetryAt = new Date().toISOString(); | ||
| saveSessionState(state); | ||
| synapseDb.close(); | ||
| // Retry once with fresh session | ||
| resolve(); | ||
| await runAnalysis(hours, mode, true); | ||
| return; | ||
| } | ||
| } else if (isContextOverflow) { | ||
| log('CONTEXT OVERFLOW on retry - giving up, will try next cycle'); | ||
| } | ||
| } | ||
| if (!extracted.analysis) { | ||
| log('Could not parse structured analysis from response'); | ||
| log(`Response preview: ${responseText.slice(0, 200)}`); | ||
|
|
||
| // Update session state | ||
| if (newSessionId) { | ||
| state.sessionId = newSessionId; | ||
| log(`Session ID: ${newSessionId.slice(0, 8)}...`); | ||
| } | ||
| state.lastUsedAt = new Date().toISOString(); | ||
| state.runCount++; | ||
| state.mode = mode; | ||
| // Check if this might be a session/context issue | ||
| if (!isRetry && state.runCount >= MAX_SESSION_RUNS) { | ||
| log('May be context overflow after many runs - retrying with fresh session'); | ||
| state.sessionId = null; | ||
| state.runCount = 0; | ||
| state.lastRetryAt = new Date().toISOString(); | ||
| saveSessionState(state); | ||
|
|
||
| // Record analysis run | ||
| const endTime = new Date(); | ||
| const startTime = new Date(endTime.getTime() - hours * 60 * 60 * 1000); | ||
| const patternsFound = state.runCount; // Use run count as proxy | ||
| recordAnalysisRun(synapseDb, startTime, endTime, patternsFound); | ||
|
|
||
| // Log consecutive days streak (derived from analysis_runs) | ||
| const consecutiveDays = getConsecutiveDaysStreak(synapseDb); | ||
| log(`Consecutive days of usage: ${consecutiveDays}`); | ||
|
|
||
| // Check if reflection should run (daily trigger) | ||
| if (shouldRunReflection(synapseDb)) { | ||
| log('Reflection trigger: observations accumulated. Run "synapse reflect" to review skill improvements.'); | ||
| sendNotification( | ||
| 'Synapse Reflect', | ||
| 'Skill observations ready for reflection. Run "synapse reflect" to review.' | ||
| ); | ||
| } | ||
|
|
||
| synapseDb.close(); | ||
| log(`Run #${state.runCount} complete`); | ||
| await runAnalysis(hours, mode, true); | ||
| return; | ||
| } | ||
| } else { | ||
| // ── Process structured analysis ──────────────────────────────── | ||
| await processStructuredAnalysis(extracted.analysis); | ||
| } | ||
|
|
||
| resolve(); | ||
| }; | ||
| // ── Update session state ────────────────────────────────────────── | ||
| state.lastUsedAt = new Date().toISOString(); | ||
| state.runCount++; | ||
| state.mode = mode; | ||
| saveSessionState(state); | ||
|
|
||
| // Listen for stdout to fully end | ||
| claude.stdout.on('end', () => { | ||
| stdoutEnded = true; | ||
| processOutput(); | ||
| }); | ||
| // Record analysis run | ||
| const endTime = new Date(); | ||
| const startTime = new Date(endTime.getTime() - hours * 60 * 60 * 1000); | ||
| const patternsFound = extracted.analysis?.patterns?.length || 0; | ||
| recordAnalysisRun(synapseDb, startTime, endTime, patternsFound); | ||
|
|
||
| // Log consecutive days streak | ||
| const consecutiveDays = getConsecutiveDaysStreak(synapseDb); | ||
| log(`Consecutive days of usage: ${consecutiveDays}`); | ||
|
|
||
| // Check if reflection should run | ||
| if (shouldRunReflection(synapseDb)) { | ||
| log('Reflection trigger: observations accumulated. Run "synapse reflect" to review skill improvements.'); | ||
| sendNotification( | ||
| 'Synapse Reflect', | ||
| 'Skill observations ready for reflection. Run "synapse reflect" to review.' | ||
| ); | ||
| } | ||
|
|
||
| // Listen for process to exit | ||
| claude.on('close', (code) => { | ||
| exitCode = code ?? 0; | ||
| // Clean up: clear timeout and remove from tracking | ||
| clearTimeout(timeoutId); | ||
| if (claudePid) { | ||
| currentSessionPids.delete(claudePid); | ||
| } | ||
| processOutput(); | ||
| }); | ||
| }); | ||
| synapseDb.close(); | ||
| log(`Run #${state.runCount} complete`); | ||
| } |
There was a problem hiding this comment.
Don’t record analysis runs when structured extraction fails.
If extraction fails, the code still updates session state and records analysis_runs, which can suppress retries and inflate usage streaks even though the run produced no actionable analysis. Consider treating this as a failed run (save session ID if needed, but skip recordAnalysisRun and streak updates).
Suggested fix
if (!extracted.analysis) {
log('Could not parse structured analysis from response');
log(`Response preview: ${responseText.slice(0, 200)}`);
// Check if this might be a session/context issue
if (!isRetry && state.runCount >= MAX_SESSION_RUNS) {
log('May be context overflow after many runs - retrying with fresh session');
state.sessionId = null;
state.runCount = 0;
state.lastRetryAt = new Date().toISOString();
saveSessionState(state);
synapseDb.close();
await runAnalysis(hours, mode, true);
return;
}
+
+ // Treat as a failed analysis: persist session metadata but skip run recording.
+ state.lastUsedAt = new Date().toISOString();
+ saveSessionState(state);
+ synapseDb.close();
+ return;
} else {
// ── Process structured analysis ────────────────────────────────
await processStructuredAnalysis(extracted.analysis);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scripts/daemon.ts` around lines 1490 - 1541, The run currently treats
failed structured extraction as a successful run: after calling
extractAnalysisFromResponseText it still updates state.runCount/lastUsedAt,
calls saveSessionState, recordAnalysisRun, updates streaks and may send
notifications; instead, change the control flow so that when extracted.analysis
is falsy you do not increment state.runCount or update lastUsedAt and you do not
call recordAnalysisRun, getConsecutiveDaysStreak or sendNotification. Keep the
existing retry/reset logic (sessionId reset, saveSessionState when forcing a
fresh session) and always ensure synapseDb.close() runs, but move the state
updates, saveSessionState, recordAnalysisRun, streak check
(getConsecutiveDaysStreak) and reflection/notification logic into the branch
where extracted.analysis exists (after await processStructuredAnalysis) so only
successful extractions are recorded.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Kept HEAD's harness-based analysis flow, added main's new blockedWindows/blockedUrls config fields and userProfile to test fixtures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
.dmux-hooks/examples/run_test.example (1)
21-52: Use mktemp + trap for safer temp files and guaranteed cleanup.A fixed
/tmpfilename risks collisions, andset -ecan exit before cleanup. A temp file +trapavoids both.♻️ Proposed fix
-OUTPUT_FILE="/tmp/dmux-test-$DMUX_PANE_ID.txt" +OUTPUT_FILE="$(mktemp "/tmp/dmux-test-${DMUX_PANE_ID}.XXXXXX")" +cleanup() { rm -f "$OUTPUT_FILE"; } +trap cleanup EXIT @@ -# Cleanup -rm -f "$OUTPUT_FILE"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.dmux-hooks/examples/run_test.example around lines 21 - 52, Replace the fixed /tmp filename with a safely created temporary file via mktemp (replace OUTPUT_FILE="/tmp/dmux-test-$DMUX_PANE_ID.txt" with an mktemp-backed assignment), and register a trap to remove that temp file on EXIT (to guarantee cleanup even if the script exits early or set -e is enabled); keep the existing test run flow and STATUS assignment logic (the if pnpm test > "$OUTPUT_FILE" 2>&1; then ... else ... fi and the curl reporting using $OUTPUT) but ensure the trap runs before running tests so rm -f "$OUTPUT_FILE" is redundant and safe to keep or remove. Ensure you reference the OUTPUT_FILE variable in the mktemp/trap changes so the rest of the script continues to work..dmux-hooks/examples/post_merge.example (1)
34-41: Use a portable issue-number extractor (grep -oP is unavailable on macOS/BSD).The
-Pflag is a GNU grep extension that doesn't exist on macOS or BSD systems (which ship FreeBSD grep). The proposedsed -nEreplacement is portable and produces equivalent results:♻️ Portable alternative
-ISSUE_NUM=$(echo "$DMUX_PROMPT" | grep -oP '#\K\d+' | head -1) +ISSUE_NUM=$(printf '%s' "$DMUX_PROMPT" | sed -nE 's/.*#([0-9]+).*/\1/p' | head -1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.dmux-hooks/examples/post_merge.example around lines 34 - 41, Replace the non-portable GNU grep usage that extracts the issue number with a portable sed-based extractor: update the ISSUE_NUM assignment (which reads from DMUX_PROMPT) to use sed -nE to capture the first sequence of digits following a ‘#’ and preserve the existing head -1 behavior; keep the rest of the hook logic (the conditional on ISSUE_NUM, and the gh issue close call that references DMUX_SLUG and DMUX_TARGET_BRANCH) unchanged so the hook behaves identically on macOS/BSD and Linux.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.dmux-hooks/examples/run_dev.example:
- Around line 29-36: The port detection block using grep -oP is non-portable and
can cause the script to exit under set -e; replace the grep -oP call that
populates PORT with a portable pipeline (e.g., use grep -E or awk/sed to extract
the digits after "localhost:" from "$LOG_FILE") and ensure the command never
causes the script to abort (wrap with "|| true" or capture output via a
subshell) so the subsequent if [ -z "$PORT" ] fallback to PORT=3000 still runs;
update references to PORT and LOG_FILE accordingly and keep the head -1 behavior
to pick the first match.
In @.dmux-hooks/examples/worktree_created.example:
- Around line 36-38: Before appending to
"$DMUX_ROOT/.dmux/worktree_history.log", ensure the target directory exists by
creating "$DMUX_ROOT/.dmux" (e.g., mkdir -p using DMUX_ROOT variable) so the
echo redirect in the worktree_created script (the line writing to
.dmux/worktree_history.log) cannot fail under set -e; add that directory-create
step just prior to the echo that writes the log.
In `@src/index.ts`:
- Around line 353-370: The early return when harnessResult.ok is false skips
extracting usable rawText; update the logic around harnessResult, harnessUsed,
and harnessChain so that if harnessResult.ok is false but harnessResult.rawText
exists you still call extractAnalysisFromResponseText(responseText) and proceed
with extraction (and any downstream processing) before closing synapseDb and
returning. Adjust the control flow in the block that currently logs the error
and does synapseDb.close() so it only returns immediately when no rawText is
present, otherwise continue to the existing provider metadata/logging and
extraction steps (keeping references to harnessChain[0], harnessUsed,
harnessResult.metadata, and extractAnalysisFromResponseText).
In `@src/scripts/daemon.ts`:
- Around line 1463-1484: When harnessResult.ok is false we currently return
early and skip any salvageable parsing; update the !harnessResult.ok branch to
first check for harnessResult.rawText and attempt to run the existing
extraction/parsing routine (e.g., the function that normally converts raw
harness output into an analysis result — reference harnessResult.rawText and the
parser/extractor used elsewhere in this module) and proceed with the normal
success flow if extraction yields usable data; only if extraction fails should
you continue with the stale-session logic (isStaleSession, retry via
runAnalysis) and the current sendNotification/synapseDb.close return path,
ensuring you still preserve state.sessionId reset/saveSessionState and
synapseDb.close when falling back.
---
Duplicate comments:
In `@src/scripts/daemon.ts`:
- Around line 1500-1535: The code currently increments state.runCount, updates
state.lastUsedAt, saves session state, calls recordAnalysisRun, and logs
consecutive days even when extracted.analysis is falsy; move the session-state
updates (state.lastUsedAt, state.runCount++, state.mode and saveSessionState),
the recordAnalysisRun(...) call, and the getConsecutiveDaysStreak/log call into
the branch that handles successful extraction (the else that calls
processStructuredAnalysis(extracted.analysis)), so these actions only execute
when extracted.analysis exists; ensure the retry path that sets
state.sessionId=null and calls runAnalysis(...) still returns early and does not
run any of the moved post-success code.
---
Nitpick comments:
In @.dmux-hooks/examples/post_merge.example:
- Around line 34-41: Replace the non-portable GNU grep usage that extracts the
issue number with a portable sed-based extractor: update the ISSUE_NUM
assignment (which reads from DMUX_PROMPT) to use sed -nE to capture the first
sequence of digits following a ‘#’ and preserve the existing head -1 behavior;
keep the rest of the hook logic (the conditional on ISSUE_NUM, and the gh issue
close call that references DMUX_SLUG and DMUX_TARGET_BRANCH) unchanged so the
hook behaves identically on macOS/BSD and Linux.
In @.dmux-hooks/examples/run_test.example:
- Around line 21-52: Replace the fixed /tmp filename with a safely created
temporary file via mktemp (replace
OUTPUT_FILE="/tmp/dmux-test-$DMUX_PANE_ID.txt" with an mktemp-backed
assignment), and register a trap to remove that temp file on EXIT (to guarantee
cleanup even if the script exits early or set -e is enabled); keep the existing
test run flow and STATUS assignment logic (the if pnpm test > "$OUTPUT_FILE"
2>&1; then ... else ... fi and the curl reporting using $OUTPUT) but ensure the
trap runs before running tests so rm -f "$OUTPUT_FILE" is redundant and safe to
keep or remove. Ensure you reference the OUTPUT_FILE variable in the mktemp/trap
changes so the rest of the script continues to work.
Comment on lines
+29
to
+36
| # Detect port from log output | ||
| # Adjust the grep pattern for your dev server's output format | ||
| PORT=$(grep -oP '(?<=localhost:)\d+' "$LOG_FILE" | head -1) | ||
|
|
||
| if [ -z "$PORT" ]; then | ||
| echo "[Hook] Warning: Could not detect port from logs, using default 3000" | ||
| PORT=3000 | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read the context around lines 29-36
if [ -f ".dmux-hooks/examples/run_dev.example" ]; then
echo "=== File exists, showing context (lines 1-40) ==="
head -40 ".dmux-hooks/examples/run_dev.example" | cat -n
else
echo "File not found. Searching for similar files..."
find . -name "*run_dev*" -o -name "*dmux*" 2>/dev/null | head -20
fiRepository: serrrfirat/synapse-daemon
Length of output: 1499
🏁 Script executed:
# Test grep -oP portability on this system
echo "=== Testing grep -oP support ==="
echo "localhost:3000" | grep -oP '(?<=localhost:)\d+' 2>&1 && echo "SUCCESS: grep -oP works" || echo "FAILED: grep -oP not supported"
# Test the proposed alternative
echo ""
echo "=== Testing proposed alternative ==="
echo "localhost:3000" | grep -oE 'localhost:[0-9]+' | head -1 | sed -E 's/.*localhost:([0-9]+).*/\1/' || true
echo "Alternative result: $?"Repository: serrrfirat/synapse-daemon
Length of output: 195
🏁 Script executed:
# Check if set -e is actually enabled in the script
if [ -f ".dmux-hooks/examples/run_dev.example" ]; then
echo "=== Checking for 'set -e' in the script ==="
grep -n "set -e" ".dmux-hooks/examples/run_dev.example" || echo "No 'set -e' found"
fiRepository: serrrfirat/synapse-daemon
Length of output: 121
Port detection fails on macOS due to non-portable grep and set -e interaction.
grep -oP (Perl regex) is not supported by BSD grep (macOS default). When the pattern doesn't match, grep returns non-zero on macOS, causing the script to abort due to set -e before reaching the fallback to port 3000. The fallback check is unreachable in this scenario.
🐛 Recommended fix (portable alternative)
-PORT=$(grep -oP '(?<=localhost:)\d+' "$LOG_FILE" | head -1)
+PORT=$(grep -oE 'localhost:[0-9]+' "$LOG_FILE" | head -1 | sed -E 's/.*localhost:([0-9]+).*/\1/' || true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.dmux-hooks/examples/run_dev.example around lines 29 - 36, The port
detection block using grep -oP is non-portable and can cause the script to exit
under set -e; replace the grep -oP call that populates PORT with a portable
pipeline (e.g., use grep -E or awk/sed to extract the digits after "localhost:"
from "$LOG_FILE") and ensure the command never causes the script to abort (wrap
with "|| true" or capture output via a subshell) so the subsequent if [ -z
"$PORT" ] fallback to PORT=3000 still runs; update references to PORT and
LOG_FILE accordingly and keep the head -1 behavior to pick the first match.
Comment on lines
+36
to
+38
| # Create a log entry | ||
| echo "[$(date)] Created worktree: $DMUX_SLUG | Agent: $DMUX_AGENT | Prompt: $DMUX_PROMPT" \ | ||
| >> "$DMUX_ROOT/.dmux/worktree_history.log" |
There was a problem hiding this comment.
Ensure the history directory exists before appending.
set -e will abort if .dmux/ doesn’t exist.
🛠️ Proposed fix
-# Create a log entry
+# Create a log entry
+mkdir -p "$DMUX_ROOT/.dmux"
echo "[$(date)] Created worktree: $DMUX_SLUG | Agent: $DMUX_AGENT | Prompt: $DMUX_PROMPT" \
>> "$DMUX_ROOT/.dmux/worktree_history.log"📝 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.
Suggested change
| # Create a log entry | |
| echo "[$(date)] Created worktree: $DMUX_SLUG | Agent: $DMUX_AGENT | Prompt: $DMUX_PROMPT" \ | |
| >> "$DMUX_ROOT/.dmux/worktree_history.log" | |
| # Create a log entry | |
| mkdir -p "$DMUX_ROOT/.dmux" | |
| echo "[$(date)] Created worktree: $DMUX_SLUG | Agent: $DMUX_AGENT | Prompt: $DMUX_PROMPT" \ | |
| >> "$DMUX_ROOT/.dmux/worktree_history.log" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.dmux-hooks/examples/worktree_created.example around lines 36 - 38, Before
appending to "$DMUX_ROOT/.dmux/worktree_history.log", ensure the target
directory exists by creating "$DMUX_ROOT/.dmux" (e.g., mkdir -p using DMUX_ROOT
variable) so the echo redirect in the worktree_created script (the line writing
to .dmux/worktree_history.log) cannot fail under set -e; add that
directory-create step just prior to the echo that writes the log.
Comment on lines
+353
to
370
| if (!harnessResult.ok) { | ||
| console.error(chalk.red(`Analysis failed: ${harnessResult.error}`)); | ||
| synapseDb.close(); | ||
| return; | ||
| } | ||
|
|
||
| claude.stdout.on('data', (data) => { | ||
| const chunk = data.toString(); | ||
| output += chunk; | ||
| chunkCount++; | ||
| if (harnessUsed !== harnessChain[0]) { | ||
| console.log(chalk.yellow(`Fallback: used ${harnessUsed} instead of ${harnessChain[0]}`)); | ||
| } | ||
|
|
||
| if (verbose) { | ||
| // Parse stream-json format: each line is a JSON object | ||
| const lines = chunk.split('\n').filter((l: string) => l.trim()); | ||
| for (const line of lines) { | ||
| try { | ||
| const parsed = JSON.parse(line); | ||
| const eventType = `${parsed.type}${parsed.event?.type ? ':' + parsed.event.type : ''}`; | ||
| eventTypes.add(eventType); | ||
|
|
||
| // Handle stream_event with content_block_delta containing text_delta | ||
| // Format: {"type":"stream_event","event":{"type":"content_block_delta","delta":{"type":"text_delta","text":"..."}}} | ||
| if (parsed.type === 'stream_event' && | ||
| parsed.event?.type === 'content_block_delta' && | ||
| parsed.event?.delta?.type === 'text_delta' && | ||
| parsed.event?.delta?.text) { | ||
| const text = parsed.event.delta.text; | ||
| process.stdout.write(chalk.white(text)); | ||
| fullText += text; | ||
| } | ||
| // Handle final assistant message (backup) | ||
| else if (parsed.type === 'assistant' && parsed.message?.content) { | ||
| for (const block of parsed.message.content) { | ||
| if (block.type === 'text' && block.text) { | ||
| fullText = block.text; | ||
| } | ||
| } | ||
| } | ||
| // Handle result type (final result with usage stats) | ||
| else if (parsed.type === 'result') { | ||
| if (parsed.result) { | ||
| fullText = parsed.result; | ||
| } | ||
| // Extract token usage | ||
| if (parsed.usage) { | ||
| tokenUsage = { | ||
| input: parsed.usage.input_tokens || 0, | ||
| output: parsed.usage.output_tokens || 0, | ||
| cacheRead: parsed.usage.cache_read_input_tokens || 0, | ||
| cacheCreate: parsed.usage.cache_creation_input_tokens || 0, | ||
| cost: parsed.total_cost_usd || 0 | ||
| }; | ||
| } | ||
| } | ||
| } catch { | ||
| // Not JSON or incomplete line, ignore | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| if (harnessResult.metadata?.model) { | ||
| console.log(chalk.gray(`Provider metadata: ${harnessResult.metadata.provider || harnessUsed}/${harnessResult.metadata.model}`)); | ||
| } | ||
|
|
||
| let stderrOutput = ''; | ||
| claude.stderr.on('data', (data) => { | ||
| const errStr = data.toString(); | ||
| stderrOutput += errStr; | ||
| if (verbose) { | ||
| console.log(chalk.red(`[STDERR] ${errStr}`)); | ||
| } | ||
| }); | ||
| // ── Extract structured analysis from response text ────────────────── | ||
| const responseText = harnessResult.rawText || ''; | ||
| const extracted = extractAnalysisFromResponseText(responseText); | ||
|
|
There was a problem hiding this comment.
Attempt extraction when a harness returns rawText but ok=false.
A non‑OK harness result may still include usable rawText (e.g., parser error). Currently the flow returns early and never attempts extraction.
🛠️ Proposed fix
- if (!harnessResult.ok) {
- console.error(chalk.red(`Analysis failed: ${harnessResult.error}`));
- synapseDb.close();
- return;
- }
+ if (!harnessResult.ok && !harnessResult.rawText) {
+ console.error(chalk.red(`Analysis failed: ${harnessResult.error}`));
+ synapseDb.close();
+ return;
+ }
+ if (!harnessResult.ok && harnessResult.rawText) {
+ console.log(chalk.yellow(`Analysis returned non-OK status; attempting to extract from raw output...`));
+ }📝 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.
Suggested change
| if (!harnessResult.ok) { | |
| console.error(chalk.red(`Analysis failed: ${harnessResult.error}`)); | |
| synapseDb.close(); | |
| return; | |
| } | |
| claude.stdout.on('data', (data) => { | |
| const chunk = data.toString(); | |
| output += chunk; | |
| chunkCount++; | |
| if (harnessUsed !== harnessChain[0]) { | |
| console.log(chalk.yellow(`Fallback: used ${harnessUsed} instead of ${harnessChain[0]}`)); | |
| } | |
| if (verbose) { | |
| // Parse stream-json format: each line is a JSON object | |
| const lines = chunk.split('\n').filter((l: string) => l.trim()); | |
| for (const line of lines) { | |
| try { | |
| const parsed = JSON.parse(line); | |
| const eventType = `${parsed.type}${parsed.event?.type ? ':' + parsed.event.type : ''}`; | |
| eventTypes.add(eventType); | |
| // Handle stream_event with content_block_delta containing text_delta | |
| // Format: {"type":"stream_event","event":{"type":"content_block_delta","delta":{"type":"text_delta","text":"..."}}} | |
| if (parsed.type === 'stream_event' && | |
| parsed.event?.type === 'content_block_delta' && | |
| parsed.event?.delta?.type === 'text_delta' && | |
| parsed.event?.delta?.text) { | |
| const text = parsed.event.delta.text; | |
| process.stdout.write(chalk.white(text)); | |
| fullText += text; | |
| } | |
| // Handle final assistant message (backup) | |
| else if (parsed.type === 'assistant' && parsed.message?.content) { | |
| for (const block of parsed.message.content) { | |
| if (block.type === 'text' && block.text) { | |
| fullText = block.text; | |
| } | |
| } | |
| } | |
| // Handle result type (final result with usage stats) | |
| else if (parsed.type === 'result') { | |
| if (parsed.result) { | |
| fullText = parsed.result; | |
| } | |
| // Extract token usage | |
| if (parsed.usage) { | |
| tokenUsage = { | |
| input: parsed.usage.input_tokens || 0, | |
| output: parsed.usage.output_tokens || 0, | |
| cacheRead: parsed.usage.cache_read_input_tokens || 0, | |
| cacheCreate: parsed.usage.cache_creation_input_tokens || 0, | |
| cost: parsed.total_cost_usd || 0 | |
| }; | |
| } | |
| } | |
| } catch { | |
| // Not JSON or incomplete line, ignore | |
| } | |
| } | |
| } | |
| }); | |
| if (harnessResult.metadata?.model) { | |
| console.log(chalk.gray(`Provider metadata: ${harnessResult.metadata.provider || harnessUsed}/${harnessResult.metadata.model}`)); | |
| } | |
| let stderrOutput = ''; | |
| claude.stderr.on('data', (data) => { | |
| const errStr = data.toString(); | |
| stderrOutput += errStr; | |
| if (verbose) { | |
| console.log(chalk.red(`[STDERR] ${errStr}`)); | |
| } | |
| }); | |
| // ── Extract structured analysis from response text ────────────────── | |
| const responseText = harnessResult.rawText || ''; | |
| const extracted = extractAnalysisFromResponseText(responseText); | |
| if (!harnessResult.ok && !harnessResult.rawText) { | |
| console.error(chalk.red(`Analysis failed: ${harnessResult.error}`)); | |
| synapseDb.close(); | |
| return; | |
| } | |
| if (!harnessResult.ok && harnessResult.rawText) { | |
| console.log(chalk.yellow(`Analysis returned non-OK status; attempting to extract from raw output...`)); | |
| } | |
| if (harnessUsed !== harnessChain[0]) { | |
| console.log(chalk.yellow(`Fallback: used ${harnessUsed} instead of ${harnessChain[0]}`)); | |
| } | |
| if (harnessResult.metadata?.model) { | |
| console.log(chalk.gray(`Provider metadata: ${harnessResult.metadata.provider || harnessUsed}/${harnessResult.metadata.model}`)); | |
| } | |
| // ── Extract structured analysis from response text ────────────────── | |
| const responseText = harnessResult.rawText || ''; | |
| const extracted = extractAnalysisFromResponseText(responseText); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 353 - 370, The early return when harnessResult.ok
is false skips extracting usable rawText; update the logic around harnessResult,
harnessUsed, and harnessChain so that if harnessResult.ok is false but
harnessResult.rawText exists you still call
extractAnalysisFromResponseText(responseText) and proceed with extraction (and
any downstream processing) before closing synapseDb and returning. Adjust the
control flow in the block that currently logs the error and does
synapseDb.close() so it only returns immediately when no rawText is present,
otherwise continue to the existing provider metadata/logging and extraction
steps (keeping references to harnessChain[0], harnessUsed,
harnessResult.metadata, and extractAnalysisFromResponseText).
Comment on lines
+1463
to
+1484
| if (!harnessResult.ok) { | ||
| log(`Analysis failed: ${harnessResult.error}`); | ||
| const isStaleSession = harnessResult.error && ( | ||
| harnessResult.error.includes('no conversation found with session id') || | ||
| harnessResult.error.includes('session not found') || | ||
| harnessResult.error.includes('session_id') | ||
| ); | ||
| if (!isRetry && isStaleSession) { | ||
| log('Stale session error detected - retrying once with fresh session.'); | ||
| state.sessionId = null; | ||
| state.contextOverflow = false; | ||
| state.runCount = 0; | ||
| state.lastRetryAt = new Date().toISOString(); | ||
| saveSessionState(state); | ||
| synapseDb.close(); | ||
| await runAnalysis(hours, mode, true); | ||
| return; | ||
| } | ||
| sendNotification('Synapse Warning', `Analysis failed: ${harnessResult.error?.slice(0, 100)}`); | ||
| synapseDb.close(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Attempt extraction when rawText exists even if ok=false.
A non‑OK harness result can still contain parseable output; returning early skips that recovery path.
🛠️ Proposed fix
- if (!harnessResult.ok) {
- log(`Analysis failed: ${harnessResult.error}`);
+ if (!harnessResult.ok && !harnessResult.rawText) {
+ log(`Analysis failed: ${harnessResult.error}`);
const isStaleSession = harnessResult.error && (
harnessResult.error.includes('no conversation found with session id') ||
harnessResult.error.includes('session not found') ||
harnessResult.error.includes('session_id')
);
@@
- sendNotification('Synapse Warning', `Analysis failed: ${harnessResult.error?.slice(0, 100)}`);
- synapseDb.close();
- return;
- }
+ sendNotification('Synapse Warning', `Analysis failed: ${harnessResult.error?.slice(0, 100)}`);
+ synapseDb.close();
+ return;
+ }
+ if (!harnessResult.ok && harnessResult.rawText) {
+ log('Analysis returned non-OK status; attempting to extract from raw output...');
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scripts/daemon.ts` around lines 1463 - 1484, When harnessResult.ok is
false we currently return early and skip any salvageable parsing; update the
!harnessResult.ok branch to first check for harnessResult.rawText and attempt to
run the existing extraction/parsing routine (e.g., the function that normally
converts raw harness output into an analysis result — reference
harnessResult.rawText and the parser/extractor used elsewhere in this module)
and proceed with the normal success flow if extraction yields usable data; only
if extraction fails should you continue with the stale-session logic
(isStaleSession, retry via runAnalysis) and the current
sendNotification/synapseDb.close return path, ensuring you still preserve
state.sessionId reset/saveSessionState and synapseDb.close when falling back.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
Summary
src/index.tsandsrc/scripts/daemon.tsinto a single sharedrunAnalysisWithHarness()entrypoint--analysis-harnessconfig option (e.g.synapse config --analysis-harness codex,claude_code) with full backward compatibility for the legacy--analysis-providerflagNew files
src/harness/types.tsHarnessAdapterinterface,HarnessIdtype, run options/result typessrc/harness/parsers.tssrc/harness/index.tsresolveHarnessChain(),runAnalysisWithHarness()src/harness/adapters/*.tssrc/harness/__tests__/*.tsRobustness hardening (from self-review)
child.on('error')handlers on all adapters to prevent Promise hangs when spawn failschild.stdin.on('error')EPIPE guards on stdin-piping adaptersrunAnalysisWithHarness()Test plan
npx tsc --noEmitpassesbun test— 93 tests pass (38 new + 55 existing), 0 failuressynapse config --analysis-harness codex,claude_codesets chain correctlysynapse config(show) displays harness chain and effective chainsynapse analyzewith default config uses Claude Code (backward compat)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
--analysis-harnessoption for analysis runsBug Fixes