test(synapse): SYN-12 Performance Benchmarks + E2E Testing#136
test(synapse): SYN-12 Performance Benchmarks + E2E Testing#136Pedrovaleriolopez merged 5 commits intomainfrom
Conversation
…-12] Add comprehensive E2E test suite (53 tests across 6 files) and performance benchmark infrastructure for the SYNAPSE context engine pipeline. - Pipeline benchmark with cold/warm modes, p50/p95/p99 percentiles, isolated formatter timing, and per-layer metrics (100+ iterations) - E2E tests: full pipeline, bracket scenarios (FRESH/MODERATE/DEPLETED/CRITICAL), agent scenarios (@dev/@qa/@devops/@architect), hook integration (stdin/stdout protocol), DEVMODE on/off with per-call override, regression guards - Fix hook async bug: add await to engine.process() in synapse-engine.js - Add SYNAPSE benchmark CI job (Node 18+20 matrix, E2E + benchmark) - All performance targets met: pipeline p95 <100ms, layers <20ms, startup <10ms - 96.7% statement coverage on synapse modules (target 85%) - 5775 existing tests pass with 0 regressions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
type: docs
WalkthroughAwaited SynapseEngine.process in the hook and added safeExit to avoid hard process.exit during tests. Added a CI benchmark job and a pipeline benchmark script. Introduced multiple Synapse E2E test suites and regression guards. Minor timer cleanup added in an integration test. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📊 Coverage ReportCoverage report not available
Generated by PR Automation (Story 6.1) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@tests/synapse/e2e/hook-integration.e2e.test.js`:
- Around line 100-115: The test 'hook output additionalContext is a string
conforming to expected format' is missing the pre-parse guards; before calling
JSON.parse(stdout) assert the hook ran successfully by checking the runHookSync
return values (e.g., verify exitCode === 0 and that stdout is truthy) — use the
same pattern as Test 1: call runHookSync(input), assert exitCode is 0 and stdout
is present, then safely parse stdout into result and continue extracting
result.hookSpecificOutput.additionalContext for the remaining expectations.
In `@tests/synapse/e2e/regression-guards.e2e.test.js`:
- Around line 77-83: The test currently measures startup only once
(startupDurations populated when i === 0) so computing p95 on startupDurations
is meaningless; fix by measuring startup across ITERATIONS instead: inside the
loop where ITERATIONS is used, instantiate a fresh SynapseEngine each iteration
(new SynapseEngine(SYNAPSE_PATH, { manifest, devmode: false })), record the
elapsed time into startupDurations, and then properly dispose/stop the engine
before the next iteration to avoid resource leakage; ensure you remove the i ===
0 guard and handle warm/cold mode semantics consistently (or alternatively, if
you want to keep a single warm startup sample, rename the test and variable from
p95 to single-sample/startupDuration to reflect that behavior).
- Around line 120-128: The test "pipeline p95 should be within target (<70ms) or
warn" is inconsistent: it warns at >=70ms but asserts p95 < 100ms; either
enforce the 70ms target or make the name/assertion match the 100ms hard limit.
Fix by one of: (A) change the assertion for p95 (computed from pipelineDurations
using percentile) to expect(p95).toBeLessThan(70) so the test fails if the
target is missed (keep the console.warn as informational), or (B) if the intent
is only to warn at 70ms and enforce the 100ms hard limit, rename the test to
indicate the hard limit (e.g., mention 100ms) or remove the misleading "(<70ms)"
from the test title; alternatively, if you want no assertion at 70ms, replace
the hard assertion with only the console.warn and no expect. Ensure you update
the test title and the expect call referencing p95 accordingly.
🧹 Nitpick comments (9)
.github/workflows/ci.yml (1)
428-434: Both E2E and benchmark steps silently swallow failures — consider at least surfacing results.With
continue-on-error: trueon both steps, any test failure or benchmark regression will be invisible in the PR check status. This is acceptable for benchmarks, but the E2E tests (line 429) are correctness checks, not just informational. If a test fails, the PR author may not notice.Consider removing
continue-on-error: truefrom the E2E test step, or at minimum adding a summary step that logs warnings:Suggested approach
- name: Run SYNAPSE E2E tests run: npx jest tests/synapse/e2e/ --ci --verbose - continue-on-error: true - name: Run SYNAPSE pipeline benchmark run: node tests/synapse/benchmarks/pipeline-benchmark.js --iterations=50 --json continue-on-error: trueIf the intent is to keep both informational because
.synapse/may not exist in CI, that's reasonable — but worth a comment explaining why.tests/synapse/e2e/full-pipeline.e2e.test.js (1)
20-21: Unconditionalrequirecalls execute even when.synapse/is missing.Lines 20-21 require
SynapseEngineandparseManifestoutside thedescribeIfSynapseguard. If the.aios-core/modules are always present in the repo, this is fine. But if they could be absent (e.g., in a partial checkout), the test file would throw at load time rather than gracefully skipping.Compare with
bracket-scenarios.e2e.test.jswhich checksfs.existsSync(ENGINE_PATH)and defers therequireintobeforeAll. Consider moving these requires inside thedescribeIfSynapseblock for consistency.tests/synapse/e2e/bracket-scenarios.e2e.test.js (1)
137-148: Tautological assertion — theifguard makes theexpectinside it always pass.Lines 145-147 check
if (result.xml.includes('[MEMORY HINTS]'))and then assertexpect(result.xml).toMatch(/\[MEMORY HINTS\]/). The assertion will trivially pass since the condition already confirmed the match. If the intent is to validate the format of memory hints content, consider asserting on the content within the section instead:Suggested improvement
if (result.xml.includes('[MEMORY HINTS]')) { - expect(result.xml).toMatch(/\[MEMORY HINTS\]/); + // Validate memory hints have actual content after the header + expect(result.xml).toMatch(/\[MEMORY HINTS\][\s\S]+\w+/); }tests/synapse/benchmarks/pipeline-benchmark.js (3)
158-175: Engine instance management in warm mode is unnecessarily convoluted.The logic across lines 158-175 creates
engineasnullfor warm-mode iterations after the first, then usesengineToUse = engine || cachedEnginewith a further fallback(engineToUse || engine)on line 186. This works but is hard to follow.Clearer approach
- let cachedEngine = null; + let cachedEngine = options.cold + ? null + : new SynapseEngine(SYNAPSE_PATH, { manifest, devmode: false }); + + // Measure startup for the initial engine creation + if (!options.cold) { + const s0 = performance.now(); + cachedEngine = new SynapseEngine(SYNAPSE_PATH, { manifest, devmode: false }); + startupDurations.push(performance.now() - s0); + } for (let i = 0; i < iterations; i++) { - // Startup measurement - const startupStart = performance.now(); - const engine = options.cold - ? new SynapseEngine(SYNAPSE_PATH, { manifest, devmode: false }) - : (i === 0 ? new SynapseEngine(SYNAPSE_PATH, { manifest, devmode: false }) : null); - const startupEnd = performance.now(); - - if (options.cold || i === 0) { - startupDurations.push(startupEnd - startupStart); - } - - const engineToUse = options.cold ? engine : (engine || cachedEngine); - if (i === 0 && !options.cold) { - cachedEngine = engine; - } + let engineToUse; + if (options.cold) { + const s0 = performance.now(); + engineToUse = new SynapseEngine(SYNAPSE_PATH, { manifest, devmode: false }); + startupDurations.push(performance.now() - s0); + } else { + engineToUse = cachedEngine; + }
258-261: Formatter report uses L0 layer targets instead of a dedicated formatter target.Lines 260-261 compare the formatter's p95 against
TARGETS.layerL0(5ms target / 10ms hard limit). The formatter is not a "layer" — it's the output serialization step. Consider adding an explicitformatterentry toTARGETSfor clarity and correctness, especially if formatter performance expectations differ from L0.Suggested addition
const TARGETS = { pipeline: { target: 70, hardLimit: 100 }, layer: { target: 15, hardLimit: 20 }, layerL0: { target: 5, hardLimit: 10 }, layerL7: { target: 5, hardLimit: 10 }, startup: { target: 5, hardLimit: 10 }, sessionIO: { target: 10, hardLimit: 15 }, + formatter: { target: 5, hardLimit: 10 }, };Then update lines 260-261 to use
TARGETS.formatter.
42-46:percentilefunction is duplicated inregression-guards.e2e.test.js.The same
percentileimplementation exists in both this file andregression-guards.e2e.test.js(lines 42-46). Since this benchmark module already exportspercentileandcalcStats, the regression guards test could import from here instead of re-implementing.Also applies to: 72-76
tests/synapse/e2e/hook-integration.e2e.test.js (3)
36-54: Consider capturing stderr for test debugging.The helper is well-structured with proper error handling. One small observation:
stderris piped (Line 44) but never captured or returned. When tests fail in CI, having stderr available can save debugging time.💡 Optional: capture stderr for diagnostics
} catch (err) { // execSync throws on non-zero exit OR timeout return { stdout: (err.stdout || '').toString(), + stderr: (err.stderr || '').toString(), exitCode: err.status != null ? err.status : 1, }; }
183-204: Test silently passes whenresult.xmlis empty — consider logging or failing.The guard on Line 200 (
if (result.xml.length > 0)) means if the engine ever stops producing XML (a regression), this test still passes. That undermines its purpose of verifying CONSTITUTION content. Consider either:
- Asserting
result.xml.length > 0so the test catches the regression, or- At minimum, logging when the branch is skipped so CI output makes it visible.
♻️ Proposed: assert non-empty XML
expect(typeof result.xml).toBe('string'); - - if (result.xml.length > 0) { - expect(result.xml).toContain('<synapse-rules>'); - expect(result.xml).toMatch(/CONSTITUTION/i); - } + expect(result.xml.length).toBeGreaterThan(0); + expect(result.xml).toContain('<synapse-rules>'); + expect(result.xml).toMatch(/CONSTITUTION/i);
221-233: Re-stringify comparison is fragile and assumes minified JSON output.
JSON.stringify(result)produces minified JSON. If the hook ever outputs pretty-printed JSON (e.g.,JSON.stringify(obj, null, 2)), this assertion will fail even though the output is perfectly valid single-object JSON. Also, same as Test 2, there's no guard beforeJSON.parse(stdout).A more resilient trailing-data check:
♻️ Proposed: more robust trailing-data detection
const input = buildInput(); const { stdout } = runHookSync(input); + expect(stdout).toBeTruthy(); + // Parse should succeed without leftover characters const result = JSON.parse(stdout); expect(typeof result).toBe('object'); expect(result).not.toBeNull(); - // Re-stringify and compare length to detect trailing data - const reparsed = JSON.stringify(result); - expect(stdout.trim()).toBe(reparsed); + // Verify no trailing data after the JSON object + // Trim and re-parse to confirm single object + const trimmed = stdout.trim(); + expect(() => JSON.parse(trimmed)).not.toThrow(); + // Ensure nothing meaningful follows the JSON by checking + // that trimmed content re-parses to the same structure + expect(JSON.parse(trimmed)).toEqual(result);Note: If you specifically want to detect
{"a":1}{"b":2}(concatenated objects),JSON.parsewill silently ignore the second object in both approaches. A character-level scan or a streaming JSON parser would be needed for that, but it's likely overkill for this test.
| for (let i = 0; i < ITERATIONS; i++) { | ||
| // Startup measurement (first iteration only for warm mode) | ||
| if (i === 0) { | ||
| const s0 = performance.now(); | ||
| engine = new SynapseEngine(SYNAPSE_PATH, { manifest, devmode: false }); | ||
| startupDurations.push(performance.now() - s0); | ||
| } |
There was a problem hiding this comment.
Startup p95 is computed from a single sample — statistically meaningless.
In the measurement loop (lines 77-83), startup is only measured once (if (i === 0)), yielding exactly one data point in startupDurations. The test on line 158-161 then computes "p95" of a single-element array, which is just that value. This doesn't provide any percentile-based confidence.
Either measure startup across multiple iterations (which requires creating a new engine per iteration — effectively cold mode), or rename the test to reflect what it actually checks:
Option A: Honest naming
- test('startup p95 < 10ms (hard limit)', () => {
+ test('startup (warm, single measurement) < 10ms', () => {
const sorted = [...startupDurations].sort((a, b) => a - b);
- const p95 = percentile(sorted, 95);
- expect(p95).toBeLessThan(10);
+ expect(sorted[0]).toBeLessThan(10);
});Also applies to: 155-162
🤖 Prompt for AI Agents
In `@tests/synapse/e2e/regression-guards.e2e.test.js` around lines 77 - 83, The
test currently measures startup only once (startupDurations populated when i ===
0) so computing p95 on startupDurations is meaningless; fix by measuring startup
across ITERATIONS instead: inside the loop where ITERATIONS is used, instantiate
a fresh SynapseEngine each iteration (new SynapseEngine(SYNAPSE_PATH, {
manifest, devmode: false })), record the elapsed time into startupDurations, and
then properly dispose/stop the engine before the next iteration to avoid
resource leakage; ensure you remove the i === 0 guard and handle warm/cold mode
semantics consistently (or alternatively, if you want to keep a single warm
startup sample, rename the test and variable from p95 to
single-sample/startupDuration to reflect that behavior).
…-12] - Add exitCode/stdout guards before JSON.parse in hook test 2 - Measure startup across 50 iterations instead of 1 for statistical significance - Enforce 70ms target assertion instead of 100ms hard limit in target test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/synapse/e2e/hook-integration.e2e.test.js`:
- Around line 224-236: Add the same exit-code and stdout guards used in Tests 1
and 2 before calling JSON.parse in the "hook output is a single well-formed JSON
object" test: assert that the result of runHookSync(input).exitCode === 0 and
that stdout is truthy (or non-empty) before parsing, so JSON.parse(stdout) only
runs when the hook succeeded; update references around runHookSync, stdout and
buildInput to mirror the guard pattern used in the earlier tests.
In `@tests/synapse/e2e/regression-guards.e2e.test.js`:
- Around line 178-184: The test name "total synapse E2E tests >= 30 (coverage
guard)" doesn't match the assertion (it checks testFiles.length >= 5), so either
rename the test to reflect you're asserting file count (change the string passed
to the test(...) call) or change the assertion to count individual tests across
files: read each file from testFiles, sum occurrences of test(...) and it(...)
(or parse AST) into a totalTests variable and assert
expect(totalTests).toBeGreaterThanOrEqual(30); update references to testFiles
and e2eDir accordingly and keep the comment consistent with the chosen behavior.
🧹 Nitpick comments (3)
tests/synapse/e2e/hook-integration.e2e.test.js (1)
36-54:runHookSynchelper is well structured.Good use of try/catch to normalize
execSyncbehavior into a predictable{ stdout, exitCode }shape. Theerr.status != nullcheck correctly covers bothnullandundefined.One minor observation:
stderris piped (line 44) but never surfaced. If a test unexpectedly fails, having stderr available in the return value could speed up debugging.💡 Optionally expose stderr for debugging
function runHookSync(stdinData, opts = {}) { const timeout = opts.timeout || 10000; try { const stdout = execSync(`node "${HOOK_PATH}"`, { input: stdinData, encoding: 'utf8', timeout, windowsHide: true, stdio: ['pipe', 'pipe', 'pipe'], }); - return { stdout: stdout || '', exitCode: 0 }; + return { stdout: stdout || '', stderr: '', exitCode: 0 }; } catch (err) { return { stdout: (err.stdout || '').toString(), + stderr: (err.stderr || '').toString(), exitCode: err.status != null ? err.status : 1, }; } }tests/synapse/e2e/regression-guards.e2e.test.js (2)
26-34: Unconditionalrequire()calls will crash beforedescribe.skipcan activate.Lines 26–34 import engine, domain-loader, and session-manager unconditionally. If any of those files are missing (e.g., a partial checkout or a CI matrix variant without
.aios-core), the test runner crashes withMODULE_NOT_FOUNDbeforedescribeIfSynapseon line 48 can skip the suite. The defensive gate at line 24 becomes moot.Consider deferring these imports into
beforeAll(where you already have the conditionaldescribeIfSynapsewrapper), or wrapping them in a try/catch at module level.💡 Move requires inside the describe block
-const { SynapseEngine } = require( - path.join(PROJECT_ROOT, '.aios-core', 'core', 'synapse', 'engine.js') -); -const { parseManifest } = require( - path.join(PROJECT_ROOT, '.aios-core', 'core', 'synapse', 'domain', 'domain-loader.js') -); -const { loadSession } = require( - path.join(PROJECT_ROOT, '.aios-core', 'core', 'synapse', 'session', 'session-manager.js') -); +let SynapseEngine, parseManifest, loadSession; const describeIfSynapse = synapseExists ? describe : describe.skip; describeIfSynapse('SYNAPSE E2E: Regression Guards', () => { // ... beforeAll(async () => { + ({ SynapseEngine } = require( + path.join(PROJECT_ROOT, '.aios-core', 'core', 'synapse', 'engine.js') + )); + ({ parseManifest } = require( + path.join(PROJECT_ROOT, '.aios-core', 'core', 'synapse', 'domain', 'domain-loader.js') + )); + ({ loadSession } = require( + path.join(PROJECT_ROOT, '.aios-core', 'core', 'synapse', 'session', 'session-manager.js') + )); manifest = parseManifest(MANIFEST_PATH);Also applies to: 48-48
135-155: Layer performance tests pass vacuously iflayerDurationsis empty.If
engine.processnever populatesresult.metrics.per_layer(e.g., due to a regression in metrics reporting), both the "each layer p95" test (line 135) and the "edge layers" test (line 146) will pass with zero assertions — silently hiding a problem.Consider adding a guard that at least one layer was measured:
💡 Add a non-empty assertion
test('each layer p95 < 20ms (hard limit)', () => { + expect(Object.keys(layerDurations).length).toBeGreaterThan(0); for (const [name, durations] of Object.entries(layerDurations)) { const sorted = [...durations].sort((a, b) => a - b); const p95 = percentile(sorted, 95); expect(p95).toBeLessThan(20); } });
- Add exitCode/stdout guards before JSON.parse in hook test 9 - Fix test name mismatch: "test files >= 5" instead of "tests >= 30" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…-002) - Add safeExit() guard checking JEST_WORKER_ID before process.exit() - Track and cleanup abandoned timer in pipeline-memory timeout test - Prevents CI failures in pipeline-memory-integration and template-engine tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Temporarily clear JEST_WORKER_ID in run() test so safeExit() calls process.exit() as expected, then restore it in finally block. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
engine.process()was not awaited insynapse-engine.js, causingresult.xmlto beundefinedE2E Test Coverage
full-pipeline.e2e.test.jsbracket-scenarios.e2e.test.jsagent-scenarios.e2e.test.jshook-integration.e2e.test.jsdevmode-scenarios.e2e.test.jsregression-guards.e2e.test.jsPerformance Results
All targets met with comfortable margin:
QA Gate
PASS (100/100) — All 9 ACs met. Zero regressions (5775 tests pass).
Test plan
npx jest tests/synapse/e2e/ --ci)npm test) — zero regressionsnode tests/synapse/benchmarks/pipeline-benchmark.js)synapse-benchmarkjob🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Bug Fixes
Chores