Skip to content

Commit 5f4f726

Browse files
rsalusclaude
andauthored
fix: resolve 5 open bugs and eliminate test flakiness (#1045)
* fix: resolve 5 open bugs and eliminate test flakiness Bug fixes: - #1037: static analysis gate now detects project type (Node.js, .NET, Go, Rust) and runs appropriate toolchain instead of unconditionally requiring package.json - #1038: event append validates misplaced fields — returns VALIDATION_ERROR when event-type-specific fields appear at top level instead of inside the data envelope - #1040: expose checkpoint action in exarchos_workflow composite tool (handler already existed but was unreachable) - #1041: fix wrong phase names in refactor skill — "implement" → "polish-implement" in polish-implement.md and polish-validate.md - #1044: allReviewsPassed guard now enforces required review dimensions (spec-compliance, code-quality) for feature workflows, configurable via .exarchos.yml required-reviews field Test infrastructure: - Switch vitest pool to forks for module-level mock isolation - Add missing EventStore.initialize() calls in 4 test files - Eliminates intermittent test failures from PID lock contention Closes #1037, closes #1038, closes #1040, closes #1041, closes #1044 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review feedback from Seer and CodeRabbit - Fix go vet skip flag: skipTypecheck → skipLint (Seer) - Validate repoRoot exists on disk before project detection — return error instead of false-positive pass for non-existent paths (CodeRabbit) - Make _requiredReviews transient — delete from state after guard evaluation so it's not persisted to disk (CodeRabbit) - Update parity test mock to include statSync for new validation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent e6e64cd commit 5f4f726

22 files changed

Lines changed: 656 additions & 41 deletions

documentation/architecture/platform-portability.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ Point any MCP client at the stdio server:
9999
```
100100

101101
You get all four composite tools:
102-
- `exarchos_workflow` -- init, get, set, cancel, cleanup, reconcile
102+
- `exarchos_workflow` -- init, get, set, cancel, cleanup, reconcile, checkpoint
103103
- `exarchos_event` -- append, query, batch
104104
- `exarchos_orchestrate` -- convergence gates, runbooks, agent specs, script execution
105105
- `exarchos_view` -- pipeline, tasks, telemetry, convergence status

servers/exarchos-mcp/src/__tests__/workflow/integration.test.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -202,11 +202,14 @@ describe('Integration', () => {
202202
expect(toReview.success).toBe(true);
203203
expect((toReview.data as Record<string, unknown>).phase).toBe('review');
204204

205-
// review -> synthesize: requires all reviews passed (reviews is z.record -- schema field)
205+
// review -> synthesize: requires all reviews passed with required dimensions
206206
await handleSet(
207207
{
208208
featureId: 'full-saga',
209-
updates: { 'reviews.quality': { passed: true, reviewer: 'bot' } },
209+
updates: {
210+
'reviews.spec-compliance': { status: 'pass', reviewer: 'bot' },
211+
'reviews.code-quality': { status: 'pass', reviewer: 'bot' },
212+
},
210213
},
211214
stateDir,
212215
eventStore,

servers/exarchos-mcp/src/adapters/mcp.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ describe('createMcpServer', () => {
2222
beforeEach(async () => {
2323
tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mcp-adapter-test-'));
2424
const eventStore = new EventStore(tmpDir);
25+
await eventStore.initialize();
2526
ctx = { stateDir: tmpDir, eventStore, enableTelemetry: false };
2627
});
2728

servers/exarchos-mcp/src/config/resolve.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ export interface ResolvedProjectConfig {
2929
readonly workflow: {
3030
readonly skipPhases: readonly string[];
3131
readonly maxFixCycles: number;
32+
readonly requiredReviews: readonly string[];
3233
readonly phases: Readonly<Record<string, { readonly humanCheckpoint: boolean }>>;
3334
};
3435
readonly tools: {
@@ -80,6 +81,7 @@ export const DEFAULTS: ResolvedProjectConfig = deepFreeze({
8081
workflow: {
8182
skipPhases: [],
8283
maxFixCycles: 3,
84+
requiredReviews: [],
8385
phases: {},
8486
},
8587
tools: {
@@ -198,6 +200,7 @@ export function resolveConfig(project: ProjectConfig): ResolvedProjectConfig {
198200
// ── Workflow ──
199201
const skipPhases = [...(project.workflow?.['skip-phases'] ?? DEFAULTS.workflow.skipPhases)];
200202
const maxFixCycles = project.workflow?.['max-fix-cycles'] ?? DEFAULTS.workflow.maxFixCycles;
203+
const requiredReviews = [...(project.workflow?.['required-reviews'] ?? DEFAULTS.workflow.requiredReviews)];
201204
const phases: Record<string, { readonly humanCheckpoint: boolean }> = {};
202205
if (project.workflow?.phases) {
203206
for (const [name, phaseConfig] of Object.entries(project.workflow.phases)) {
@@ -229,7 +232,7 @@ export function resolveConfig(project: ProjectConfig): ResolvedProjectConfig {
229232
routing: { coderabbitThreshold, riskWeights },
230233
},
231234
vcs: { provider: vcsProvider, settings: vcsSettings },
232-
workflow: { skipPhases, maxFixCycles, phases },
235+
workflow: { skipPhases, maxFixCycles, requiredReviews, phases },
233236
tools: { defaultBranch, commitStyle, prTemplate, autoMerge, prStrategy },
234237
hooks: { on: hooksOn },
235238
};

servers/exarchos-mcp/src/config/yaml-schema.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ const PhaseConfig = z.object({
6666
const WorkflowConfig = z.object({
6767
'skip-phases': z.array(z.string()).optional(),
6868
'max-fix-cycles': z.number().int().min(1).max(10).optional(),
69+
'required-reviews': z.array(z.string().min(1)).optional(),
6970
phases: z.record(z.string(), PhaseConfig).optional(),
7071
}).strict();
7172

servers/exarchos-mcp/src/core/dispatch.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ describe('dispatch', () => {
2020
beforeEach(async () => {
2121
tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'dispatch-test-'));
2222
eventStore = new EventStore(tmpDir);
23+
await eventStore.initialize();
2324
});
2425

2526
afterEach(async () => {

servers/exarchos-mcp/src/event-store/tools.test.ts

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,92 @@ describe('handleEventAppend data validation', () => {
6464
});
6565
});
6666

67+
// ─── Misplaced Event Fields Detection ────────────────────────────────────────
68+
69+
describe('handleEventAppend misplaced fields', () => {
70+
it('rejects event with type-specific fields at top level', async () => {
71+
const result = await handleEventAppend(
72+
{
73+
stream: 'misplaced-test',
74+
event: {
75+
type: 'gate.executed',
76+
gateName: 'static-analysis',
77+
layer: 'D2',
78+
passed: true,
79+
details: { reason: 'builds clean' },
80+
},
81+
},
82+
tempDir,
83+
eventStore,
84+
);
85+
86+
expect(result.success).toBe(false);
87+
expect(result.error).toBeDefined();
88+
expect(result.error!.code).toBe('VALIDATION_ERROR');
89+
expect(result.error!.message).toContain('should be inside "data"');
90+
expect(result.error!.message).toContain('gateName');
91+
});
92+
93+
it('accepts event with fields correctly inside data envelope', async () => {
94+
const result = await handleEventAppend(
95+
{
96+
stream: 'correct-test',
97+
event: {
98+
type: 'gate.executed',
99+
data: {
100+
gateName: 'static-analysis',
101+
layer: 'D2',
102+
passed: true,
103+
details: { reason: 'builds clean' },
104+
},
105+
},
106+
},
107+
tempDir,
108+
eventStore,
109+
);
110+
111+
expect(result.success).toBe(true);
112+
});
113+
114+
it('allows unknown top-level fields for events without data schema', async () => {
115+
const result = await handleEventAppend(
116+
{
117+
stream: 'unknown-test',
118+
event: {
119+
type: 'workflow.started',
120+
data: { featureId: 'test', workflowType: 'feature' },
121+
correlationId: 'corr-123',
122+
},
123+
},
124+
tempDir,
125+
eventStore,
126+
);
127+
128+
expect(result.success).toBe(true);
129+
});
130+
});
131+
132+
describe('handleBatchAppend misplaced fields', () => {
133+
it('rejects batch with misplaced fields in any event', async () => {
134+
const result = await handleBatchAppend(
135+
{
136+
stream: 'batch-misplaced',
137+
events: [
138+
{ type: 'task.assigned', data: { taskId: 't1' } },
139+
{ type: 'gate.executed', gateName: 'lint', layer: 'D2', passed: true },
140+
],
141+
},
142+
tempDir,
143+
eventStore,
144+
);
145+
146+
expect(result.success).toBe(false);
147+
expect(result.error!.code).toBe('VALIDATION_ERROR');
148+
expect(result.error!.message).toContain('events[1]');
149+
expect(result.error!.message).toContain('gateName');
150+
});
151+
});
152+
67153
// ─── Prototype Pollution Prevention ─────────────────────────────────────────
68154

69155
describe('handleEventQuery field projection', () => {

servers/exarchos-mcp/src/event-store/tools.ts

Lines changed: 62 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,47 @@ import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
22
import { z, ZodError } from 'zod';
33
import { coercedStringArray } from '../coerce.js';
44
import { EventStore, SequenceConflictError } from './store.js';
5-
import type { EventType } from './schemas.js';
5+
import { EVENT_DATA_SCHEMAS, type EventType } from './schemas.js';
66
import { formatResult, pickFields, toEventAck, type ToolResult } from '../format.js';
77
import { buildValidatedEvent } from './event-factory.js';
88

9+
// ─── Misplaced Field Detection ──────────────────────────────────────────────
10+
11+
/** Known envelope fields that belong at the top level of an event. */
12+
const ENVELOPE_FIELDS = new Set([
13+
'type', 'data', 'correlationId', 'causationId', 'agentId', 'agentRole',
14+
'tenantId', 'organizationId', 'source', 'timestamp', 'idempotencyKey',
15+
'schemaVersion',
16+
]);
17+
18+
/**
19+
* Detect event-type-specific fields that were placed at the top level
20+
* instead of inside the `data` envelope. Returns misplaced field names
21+
* or an empty array if none are found.
22+
*/
23+
function detectMisplacedFields(event: Record<string, unknown>): string[] {
24+
const eventType = event.type as EventType | undefined;
25+
if (!eventType) return [];
26+
27+
const dataSchema = EVENT_DATA_SCHEMAS[eventType];
28+
if (!dataSchema) return [];
29+
30+
// Extract known field names from the Zod schema
31+
const schemaShape = (dataSchema as z.ZodObject<z.ZodRawShape>).shape;
32+
if (!schemaShape || typeof schemaShape !== 'object') return [];
33+
34+
const dataFieldNames = new Set(Object.keys(schemaShape));
35+
const misplaced: string[] = [];
36+
37+
for (const key of Object.keys(event)) {
38+
if (!ENVELOPE_FIELDS.has(key) && dataFieldNames.has(key)) {
39+
misplaced.push(key);
40+
}
41+
}
42+
43+
return misplaced;
44+
}
45+
946
// ─── Module-Level EventStore (removed — now threaded via DispatchContext) ─────
1047

1148
// ─── Event Append Handler ───────────────────────────────────────────────────
@@ -38,6 +75,18 @@ export async function handleEventAppend(
3875

3976
const store = eventStore;
4077

78+
// Detect fields that should be inside data but were placed at the top level
79+
const misplaced = detectMisplacedFields(args.event);
80+
if (misplaced.length > 0) {
81+
return {
82+
success: false,
83+
error: {
84+
code: 'VALIDATION_ERROR',
85+
message: `Event fields placed at wrong level — ${misplaced.map(f => `"${f}"`).join(', ')} should be inside "data", not at the top level. Wrap them: { type: "${eventType}", data: { ${misplaced.join(', ')}: ... } }`,
86+
},
87+
};
88+
}
89+
4190
try {
4291
// Validate at the system boundary (MCP tool handler = untrusted input)
4392
// Sequence 1 is a placeholder — appendValidated overwrites it with the real sequence
@@ -121,7 +170,7 @@ export async function handleBatchAppend(
121170
};
122171
}
123172

124-
// Validate all events have a type before passing to store
173+
// Validate all events have a type and no misplaced fields
125174
for (let i = 0; i < args.events.length; i++) {
126175
const eventType = args.events[i]?.type as EventType | undefined;
127176
if (!eventType) {
@@ -130,6 +179,17 @@ export async function handleBatchAppend(
130179
error: { code: 'INVALID_INPUT', message: `events[${i}].type is required` },
131180
};
132181
}
182+
183+
const misplaced = detectMisplacedFields(args.events[i]);
184+
if (misplaced.length > 0) {
185+
return {
186+
success: false,
187+
error: {
188+
code: 'VALIDATION_ERROR',
189+
message: `events[${i}]: fields placed at wrong level — ${misplaced.map(f => `"${f}"`).join(', ')} should be inside "data", not at the top level. Wrap them: { type: "${eventType}", data: { ${misplaced.join(', ')}: ... } }`,
190+
},
191+
};
192+
}
133193
}
134194

135195
const store = eventStore;

servers/exarchos-mcp/src/orchestrate/check-event-emissions.test.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -270,9 +270,11 @@ describe('handleOrchestrate integration', () => {
270270
const isolatedDir = mkdtempSync(join(tmpdir(), 'check-event-emissions-route-'));
271271
try {
272272
const { EventStore } = await import('../event-store/store.js');
273+
const eventStore = new EventStore(isolatedDir);
274+
await eventStore.initialize();
273275
const result = await handleOrchestrate(
274276
{ action: 'check_event_emissions', featureId: 'test' },
275-
{ stateDir: isolatedDir, eventStore: new EventStore(isolatedDir), enableTelemetry: false },
277+
{ stateDir: isolatedDir, eventStore, enableTelemetry: false },
276278
);
277279

278280
// Should NOT return UNKNOWN_ACTION — meaning the handler is registered

servers/exarchos-mcp/src/orchestrate/pure/static-analysis.parity.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ vi.mock('node:fs', () => ({
2323
})
2424
),
2525
existsSync: vi.fn(() => true),
26+
statSync: vi.fn(() => ({ isDirectory: () => true })),
2627
}));
2728

2829
function makePassRunner(): RunCommandFn {
@@ -52,6 +53,7 @@ describe('behavioral parity with static-analysis-gate.sh', () => {
5253
'## Static Analysis Report',
5354
'',
5455
'**Repository:** `/fake/repo`',
56+
'**Project type:** Node.js',
5557
'',
5658
'- **PASS**: Lint',
5759
'- **PASS**: Typecheck',
@@ -63,6 +65,7 @@ describe('behavioral parity with static-analysis-gate.sh', () => {
6365
].join('\n'),
6466
passCount: 2,
6567
failCount: 0,
68+
projectType: 'Node.js',
6669
});
6770
});
6871

@@ -76,6 +79,7 @@ describe('behavioral parity with static-analysis-gate.sh', () => {
7679
'## Static Analysis Report',
7780
'',
7881
'**Repository:** `/fake/repo`',
82+
'**Project type:** Node.js',
7983
'',
8084
'- **FAIL**: Lint — Lint errors found',
8185
'- **PASS**: Typecheck',
@@ -87,6 +91,7 @@ describe('behavioral parity with static-analysis-gate.sh', () => {
8791
].join('\n'),
8892
passCount: 1,
8993
failCount: 1,
94+
projectType: 'Node.js',
9095
});
9196
});
9297

@@ -101,6 +106,7 @@ describe('behavioral parity with static-analysis-gate.sh', () => {
101106
'## Static Analysis Report',
102107
'',
103108
'**Repository:** `/fake/repo`',
109+
'**Project type:** Node.js',
104110
'',
105111
'- **SKIP**: Lint — --skip-lint',
106112
'- **PASS**: Typecheck',
@@ -112,6 +118,7 @@ describe('behavioral parity with static-analysis-gate.sh', () => {
112118
].join('\n'),
113119
passCount: 1,
114120
failCount: 0,
121+
projectType: 'Node.js',
115122
});
116123
});
117124

@@ -126,6 +133,7 @@ describe('behavioral parity with static-analysis-gate.sh', () => {
126133
'## Static Analysis Report',
127134
'',
128135
'**Repository:** `/fake/repo`',
136+
'**Project type:** Node.js',
129137
'',
130138
'- **PASS**: Lint',
131139
'- **SKIP**: Typecheck — --skip-typecheck',
@@ -137,6 +145,7 @@ describe('behavioral parity with static-analysis-gate.sh', () => {
137145
].join('\n'),
138146
passCount: 1,
139147
failCount: 0,
148+
projectType: 'Node.js',
140149
});
141150
});
142151

@@ -177,6 +186,7 @@ describe('quality-check path', () => {
177186
'## Static Analysis Report',
178187
'',
179188
'**Repository:** `/fake/repo`',
189+
'**Project type:** Node.js',
180190
'',
181191
'- **PASS**: Lint',
182192
'- **PASS**: Typecheck',
@@ -188,6 +198,7 @@ describe('quality-check path', () => {
188198
].join('\n'),
189199
passCount: 3,
190200
failCount: 0,
201+
projectType: 'Node.js',
191202
});
192203
});
193204
});

0 commit comments

Comments
 (0)