diff --git a/packages/core/src/__tests__/db-migrate.test.ts b/packages/core/src/__tests__/db-migrate.test.ts index 02fb4b2141..8deb30c15b 100644 --- a/packages/core/src/__tests__/db-migrate.test.ts +++ b/packages/core/src/__tests__/db-migrate.test.ts @@ -716,7 +716,6 @@ describe("schema migration", () => { const row = db.prepare("SELECT deletedAt FROM tasks WHERE id = 'FN-legacy'").get() as { deletedAt: string | null }; expect(row.deletedAt).toBeNull(); expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); - expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); db.close(); }); @@ -750,7 +749,6 @@ describe("schema migration", () => { { id: "WS-002", mode: "script", gateMode: "advisory" }, ]); expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); - expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); db.close(); }); @@ -801,7 +799,6 @@ describe("schema migration", () => { reviewerFallbackRetryCount: 0, }); expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); - expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); db.close(); }); @@ -831,7 +828,6 @@ describe("schema migration", () => { const columns = db.prepare("PRAGMA table_info(milestones)").all() as Array<{ name: string }>; expect(columns.map((column) => column.name)).toContain("acceptanceCriteria"); expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); - expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); db.close(); }); @@ -872,7 +868,6 @@ describe("schema migration", () => { const missionColumns = db.prepare("PRAGMA table_info(missions)").all() as Array<{ name: string }>; expect(missionColumns.map((column) => column.name)).toContain("autoMerge"); - expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); db.close(); }); @@ -908,7 +903,6 @@ describe("schema migration", () => { { id: "WS-003", mode: "prompt", enabled: 0, gateMode: "advisory" }, ]); expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); - expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); db.close(); }); @@ -946,7 +940,6 @@ describe("schema migration", () => { const indexes = db.prepare("PRAGMA index_list(mission_goals)").all() as Array<{ name: string }>; expect(indexes.some((index) => index.name === "idxMissionGoalsGoalId")).toBe(true); expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); - expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); db.close(); }); @@ -1226,7 +1219,6 @@ describe("schema migration", () => { .get() as { migrated_fragment_id: string | null }; expect(stepRow.migrated_fragment_id).toBeNull(); - expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); db.close(); }); @@ -1235,14 +1227,12 @@ describe("schema migration", () => { const db = new Database(fusionDir); db.init(); expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); - expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION); db.close(); // Re-open the same on-disk DB: already at 109, the 109 block must be a no-op. const reopened = new Database(fusionDir); reopened.init(); expect(reopened.getSchemaVersion()).toBe(SCHEMA_VERSION); - expect(reopened.getSchemaVersion()).toBe(SCHEMA_VERSION); const workflowColumns = reopened.prepare("PRAGMA table_info(workflows)").all() as Array<{ name: string }>; expect(workflowColumns.filter((c) => c.name === "kind")).toHaveLength(1); const stepColumns = reopened.prepare("PRAGMA table_info(workflow_steps)").all() as Array<{ name: string }>; diff --git a/packages/core/src/mission-store.ts b/packages/core/src/mission-store.ts index 0e83267dfc..250b69de3b 100644 --- a/packages/core/src/mission-store.ts +++ b/packages/core/src/mission-store.ts @@ -2987,6 +2987,11 @@ export class MissionStore extends EventEmitter { if (!run) { throw new Error(`Validator run ${runId} not found`); } + if (run.featureId !== sourceFeatureId) { + throw new Error( + `Validator run ${runId} belongs to feature ${run.featureId}, expected ${sourceFeatureId}`, + ); + } // R22 — idempotency across re-drives. // diff --git a/packages/dashboard/app/components/settings/save-split.ts b/packages/dashboard/app/components/settings/save-split.ts index 60afda1c28..ceb579f1e8 100644 --- a/packages/dashboard/app/components/settings/save-split.ts +++ b/packages/dashboard/app/components/settings/save-split.ts @@ -76,6 +76,11 @@ export function splitSettingsSave({ if (key === "persistAgentThinkingLog") { continue; } + // customProviders is a global key, but it is NOT written through the + // save-split form. It is persisted via its own REST routes + // (register-custom-provider-routes.ts -> store.updateGlobalSettings) which + // mask API keys on read (sanitizeProvider). Routing it through this patch + // would write the masked keys back and clobber the real credentials. if (key === "customProviders") { continue; } @@ -93,7 +98,7 @@ export function splitSettingsSave({ const projectPatch: Partial = {}; for (const [key, value] of Object.entries(payload)) { if (key === "githubTokenConfigured" || key === "prAuthAvailable") continue; // server-only - if (key === "customProviders") continue; + if (key === "customProviders") continue; // persisted via dedicated routes, not save-split (see global branch above) if (key === "githubTrackingDefaultRepo" && activeSection === "global-general") continue; if (!isProjectSettingsKey(key)) continue; diff --git a/packages/dashboard/app/hooks/useNodeSettingsSync.ts b/packages/dashboard/app/hooks/useNodeSettingsSync.ts index 4511423806..59d2bffda4 100644 --- a/packages/dashboard/app/hooks/useNodeSettingsSync.ts +++ b/packages/dashboard/app/hooks/useNodeSettingsSync.ts @@ -32,7 +32,7 @@ export interface ComputedNodeSyncStatus { */ export function computeSyncState(status: NodeSettingsSyncStatus): ComputedNodeSyncStatus { const { lastSyncAt, remoteReachable, diff } = status; - const workflowDiffCount = Object.values(diff.workflowSettings ?? {}) + const workflowDiffCount = Object.values(diff.workflowSettings) .reduce((total, keys) => total + keys.length, 0); const diffCount = diff.global.length + diff.project.length + workflowDiffCount; diff --git a/packages/dashboard/src/__tests__/routes-nodes-sync-contract.test.ts b/packages/dashboard/src/__tests__/routes-nodes-sync-contract.test.ts index 944c22d0c9..a07a817e66 100644 --- a/packages/dashboard/src/__tests__/routes-nodes-sync-contract.test.ts +++ b/packages/dashboard/src/__tests__/routes-nodes-sync-contract.test.ts @@ -114,6 +114,10 @@ class MockStore extends EventEmitter { }; } + async updateGlobalSettings(patch: Record) { + return patch; + } + listWorkflowSettingValuesForProject(): Record> { return {}; } diff --git a/packages/dashboard/src/__tests__/routes-nodes-sync.test.ts b/packages/dashboard/src/__tests__/routes-nodes-sync.test.ts index 5f4a4b50c0..d38186bf1d 100644 --- a/packages/dashboard/src/__tests__/routes-nodes-sync.test.ts +++ b/packages/dashboard/src/__tests__/routes-nodes-sync.test.ts @@ -361,7 +361,9 @@ describe("Node settings sync routes", () => { expect(res.status).toBe(200); expect(res.body.success).toBe(true); expect(res.body.syncedFields).toContain("defaultProvider"); - expect(res.body.syncedFields).toContain("workflowStepTimeoutMs"); + // Workflow-sourced fields are qualified with their workflowId so duplicate + // setting ids across workflows stay distinguishable. + expect(res.body.syncedFields).toContain("builtin:coding.workflowStepTimeoutMs"); const [, pushOptions] = mockFetch.mock.calls[0] as [string, { body?: string }]; expect(JSON.parse(pushOptions.body ?? "{}").workflowSettings).toEqual({ "builtin:coding": { workflowStepTimeoutMs: 120000 }, @@ -495,7 +497,8 @@ describe("Node settings sync routes", () => { ); expect(res.status).toBe(200); - expect(res.body.appliedFields).toContain("workflowStepTimeoutMs"); + // Pull qualifies workflow-sourced fields with their workflowId. + expect(res.body.appliedFields).toContain("builtin:coding.workflowStepTimeoutMs"); expect(res.body.workflowSettingsCount).toBe(1); expect(mockUpdateWorkflowSettingValues).toHaveBeenCalledWith( "builtin:coding", diff --git a/packages/dashboard/src/routes/register-settings-sync-routes.ts b/packages/dashboard/src/routes/register-settings-sync-routes.ts index 48a44fb878..cabba4f94a 100644 --- a/packages/dashboard/src/routes/register-settings-sync-routes.ts +++ b/packages/dashboard/src/routes/register-settings-sync-routes.ts @@ -3,6 +3,7 @@ import { isMovedSettingsKey } from "@fusion/core"; import { basename } from "node:path"; import { ApiError, badRequest, notFound } from "../api-error.js"; import { getFusionAuthPath } from "../auth-paths.js"; +import { invalidateAllGlobalSettingsCaches } from "../project-store-resolver.js"; import { classifySyncStatusDenialReason, fetchFromRemoteNode, @@ -58,7 +59,10 @@ async function applyWorkflowSettingsSection( .filter(([, value]) => value !== null) .map(([key]) => key); count += appliedKeys.length; - keys.push(...appliedKeys); + // Qualify workflow-sourced keys with their workflowId so callers can tell + // which workflow changed when two workflows share a setting id (e.g. + // "builtin:coding.workflowStepTimeoutMs" vs "builtin:review.workflowStepTimeoutMs"). + keys.push(...appliedKeys.map((key) => `${workflowId}.${key}`)); break; } catch (err) { const rejectedIds = extractRejectedSettingIds(err); @@ -228,7 +232,11 @@ export const registerSettingsSyncRoutes: ApiRouteRegistrar = (ctx) => { const syncedFields = [ ...Object.keys(globalSettings), ...Object.keys(projectSettings.project), - ...Object.values(workflowSettings).flatMap((values) => Object.keys(values)), + // Qualify workflow-sourced keys with their workflowId so duplicate setting + // ids across workflows remain distinguishable in the surfaced field list. + ...Object.entries(workflowSettings).flatMap( + ([workflowId, values]) => Object.keys(values).map((key) => `${workflowId}.${key}`), + ), ]; res.json({ success: true, syncedFields }); @@ -323,6 +331,19 @@ export const registerSettingsSyncRoutes: ApiRouteRegistrar = (ctx) => { ...payloadWithoutChecksum, checksum, }); + + // applyRemoteSettings() only validates/strips the global payload; it does NOT + // write the local global settings store. Persist the pulled global settings + // through the dashboard store (last-write-wins overwrites local values) so + // process-local caches and settings listeners stay consistent and the keys + // reported in appliedFields actually take effect — mirroring the inbound + // /settings/sync-receive path. The store's updateGlobalSettings() already + // strips moved (tombstoned) keys (KTD-8). + if (result.success && remoteSettings.global && typeof remoteSettings.global === "object") { + await store.updateGlobalSettings(remoteSettings.global); + invalidateAllGlobalSettingsCaches(); + } + const workflowApplyResult = result.success ? await applyWorkflowSettingsSection(store, remoteSettings.workflowSettings) : { count: 0, keys: [] }; diff --git a/packages/engine/src/__tests__/mission-verification.test.ts b/packages/engine/src/__tests__/mission-verification.test.ts index c6b71a04c6..9a673134be 100644 --- a/packages/engine/src/__tests__/mission-verification.test.ts +++ b/packages/engine/src/__tests__/mission-verification.test.ts @@ -325,11 +325,11 @@ describe("TestExecutionVerificationCapability", () => { makeScriptedBackend([{ outcome: "timeout", stdout: "", stderr: "", timeoutMs: 1000 }]), }); // A timeout surfaces as a thrown ETIMEDOUT inside runVerificationCommand, - // which the capability catches and maps to a non-pass. For the whole-suite - // channel a non-success result is a behavioral fail; but a timeout throw is - // caught by the capability's try/catch → inconclusive. + // which the capability catches and maps deterministically to inconclusive. + // An infra timeout must never be surfaced as a behavioral fail — the contract + // is that timeout/setup failures stay inconclusive. const outcome = await cap.verifyBehavioralAssertion(baseRequest()); - expect(["inconclusive", "fail"]).toContain(outcome.verdict); + expect(outcome.verdict).toBe("inconclusive"); expect(materializer.cleanCalls).toBe(1); }); diff --git a/packages/engine/src/mission-execution-loop.ts b/packages/engine/src/mission-execution-loop.ts index 346f1d5296..4fcd10bc85 100644 --- a/packages/engine/src/mission-execution-loop.ts +++ b/packages/engine/src/mission-execution-loop.ts @@ -587,13 +587,19 @@ export class MissionExecutionLoop extends EventEmitter { loopLog.log(`Validation session created for feature ${feature.id}`); // Run the validation with timeout + let timeoutHandle: ReturnType | undefined; const timeoutPromise = new Promise((_, reject) => { - setTimeout(() => reject(new Error("Validation timeout")), VALIDATION_TIMEOUT_MS); + timeoutHandle = setTimeout(() => reject(new Error("Validation timeout")), VALIDATION_TIMEOUT_MS); }); const validationPromise = this.runValidationSession(session.session, prompt); - await Promise.race([validationPromise, timeoutPromise]); + try { + await Promise.race([validationPromise, timeoutPromise]); + } finally { + // Always clear the timer so it does not stay armed across validations. + if (timeoutHandle) clearTimeout(timeoutHandle); + } // Get the validation result from the session // The agent should have returned structured JSON in its response @@ -656,8 +662,11 @@ export class MissionExecutionLoop extends EventEmitter { judgeResult: ValidationResult, ): Promise { // Preserve non-behavioral terminal verdicts untouched (error/blocked from the - // judge are not behavioral posture concerns). - if (judgeResult.status === "error") { + // judge are not behavioral posture concerns). A "blocked" verdict must short- + // circuit too: otherwise it falls through to the aggregate recompute below, + // which would rewrite it to "fail" and incorrectly route to a Fix Feature + // instead of handleValidationBlocked. + if (judgeResult.status === "error" || judgeResult.status === "blocked") { return judgeResult; } @@ -1036,10 +1045,15 @@ export class MissionExecutionLoop extends EventEmitter { } } - // If no assertion results but we have assertions, create default results based on status - if (results.length === 0 && assertions.length > 0) { + // Backfill any linked assertions the judge omitted from its response. A + // partial judge response must not silently drop assertions: every linked + // assertion needs a result so behavioral assertions still reach + // verifyBehavioralAssertion and the aggregate is computed over the full set. + if (assertions.length > 0) { + const seen = new Set(results.map((r) => r.assertionId)); const overallPassed = parsed.status === "pass"; for (const assertion of assertions) { + if (seen.has(assertion.id)) continue; results.push({ assertionId: assertion.id, passed: overallPassed, @@ -1256,6 +1270,10 @@ ${taskContext ? `\n\nImplementation context:\n${taskContext}` : ""}`; runId: string | undefined, result: ValidationResult, ): Promise { + // Tracks how autopilot should be notified. A retry-budget-exhausted feature + // transitions to blocked, so autopilot must be told "blocked" (not "failed") + // to stay in sync with the validator-run state. + let terminalStatus: "failed" | "blocked" = "failed"; try { // Record the failures const failures = result.assertions @@ -1332,6 +1350,7 @@ ${taskContext ? `\n\nImplementation context:\n${taskContext}` : ""}`; if (message.includes("retry budget exhausted") || message.includes("exhausted its retry budget")) { loopLog.warn(`Feature ${featureId} retry budget exhausted; marking as blocked`); // completeValidatorRun already handles the blocked transition when budget is exhausted + terminalStatus = "blocked"; this.logFeatureMissionEvent(featureId, "error", "retry_budget_exhausted", `Feature ${featureId} exhausted its retry budget`, { runId: runId ?? null, }); @@ -1348,7 +1367,7 @@ ${taskContext ? `\n\nImplementation context:\n${taskContext}` : ""}`; // Notify autopilot if configured if (this.missionAutopilot?.notifyValidationComplete) { - await this.missionAutopilot.notifyValidationComplete(featureId, "failed"); + await this.missionAutopilot.notifyValidationComplete(featureId, terminalStatus); } } catch (err) { loopLog.error(`Error handling validation fail for ${featureId}:`, err); diff --git a/packages/engine/src/mission-verification.ts b/packages/engine/src/mission-verification.ts index 445addf079..ec655e0d61 100644 --- a/packages/engine/src/mission-verification.ts +++ b/packages/engine/src/mission-verification.ts @@ -37,11 +37,12 @@ import { exec } from "node:child_process"; import { promisify } from "node:util"; import type { TaskStore } from "@fusion/core"; import type { SandboxCapabilities } from "./sandbox/index.js"; -import { __setSandboxBackendForTests, resolveSandboxBackend } from "./sandbox/index.js"; +import { resolveSandboxBackend } from "./sandbox/index.js"; import type { SandboxBackend } from "./sandbox/index.js"; import { detectBwrap } from "./sandbox/bubblewrap-detect.js"; import { detectSandboxExec } from "./sandbox/sandbox-exec-detect.js"; import { runVerificationCommand } from "./verification-utils.js"; +import type { VerificationCommandResult } from "./verification-utils.js"; import { createLogger } from "./logger.js"; const execAsync = promisify(exec); @@ -474,13 +475,14 @@ export class TestExecutionVerificationCapability implements VerificationCapabili const logTaskId = request.taskId ?? `verify-${assertionId}`; // Route runVerificationCommand through the explicitly-selected isolating - // backend rather than the no-arg native fallback (R18). runVerificationCommand - // resolves its backend via the no-arg resolveSandboxBackend(), so we pin the - // selected isolating backend via the test-override hook for the duration of - // the run and unconditionally restore afterwards. + // backend (R18). The backend is passed in by argument rather than the no-arg + // resolveSandboxBackend()/global test hook: applyBehavioralPosture dispatches + // assertions concurrently via Promise.all, so a process-global override would + // race — a sibling run could clear it mid-run and the no-arg resolver would + // then fall through to the unrestricted native backend, breaking fail-closed + // isolation. Threading the backend keeps each run pinned to its own isolating + // backend regardless of concurrency. const isolating = this.backendFactory(selection.backendId); - const restoreBackend = () => __setSandboxBackendForTests(null); - __setSandboxBackendForTests(isolating); let implCheckout: DisposableCheckout | undefined; let baselineCheckout: DisposableCheckout | undefined; @@ -499,8 +501,18 @@ export class TestExecutionVerificationCapability implements VerificationCapabili verifyLog, "reviewer", scrubbedEnv, + isolating, ); + // An infra failure (timeout / abort / setup error) is NOT behavioral + // evidence: it must resolve to inconclusive, never fold into a fail or — on + // the baseline — wrongly satisfy `!baselineResult.success` and upgrade a + // proof to pass. + const implInfra = infraFailureReason(implResult); + if (implInfra) { + return this.inconclusive(assertionId, `implementation verification could not complete: ${implInfra}`); + } + // R5/AE5: agent-supplied proof must fail on the merge-base baseline and // pass on the implementation. A test that passes on both is not exercising // the defect — reject it. @@ -519,8 +531,16 @@ export class TestExecutionVerificationCapability implements VerificationCapabili verifyLog, "reviewer", scrubbedEnv, + isolating, ); + // A timed-out / aborted baseline is not a real "fails on the baseline" + // signal; treating it as one would wrongly upgrade the proof to pass. + const baselineInfra = infraFailureReason(baselineResult); + if (baselineInfra) { + return this.inconclusive(assertionId, `baseline verification could not complete: ${baselineInfra}`); + } + if (baselineResult.success && implResult.success) { return { verdict: "fail", @@ -532,7 +552,7 @@ export class TestExecutionVerificationCapability implements VerificationCapabili if (!baselineResult.success && implResult.success) { return { verdict: "pass", assertionId, reason: "regression test fails on the pre-fix baseline and passes on the implementation" }; } - // Fails on the implementation → defect still reproduces. + // A real (non-infra) failure on the implementation → defect still reproduces. return { verdict: "fail", assertionId, @@ -541,7 +561,8 @@ export class TestExecutionVerificationCapability implements VerificationCapabili }; } - // Whole-suite channel: pass only when the suite passes. + // Whole-suite channel: pass only when the suite passes; a real (non-infra) + // failure is behavioral evidence. if (implResult.success) { return { verdict: "pass", assertionId, reason: "verification suite passed on the implementation checkout" }; } @@ -558,7 +579,6 @@ export class TestExecutionVerificationCapability implements VerificationCapabili // non-pass; we route it to inconclusive (infra, not behavioral). outcome = this.inconclusive(assertionId, `verification run could not complete: ${message}`); } finally { - restoreBackend(); await implCheckout?.dispose(); await baselineCheckout?.dispose(); } @@ -775,6 +795,21 @@ function errMessage(err: unknown): string { return err instanceof Error ? err.message : String(err); } +/** + * Return a human-readable reason when a verification command result represents an + * *infrastructure* outcome (timeout / abort / setup failure) rather than a real + * test verdict, or `null` when the result is a genuine pass/fail. Infra outcomes + * must resolve to `inconclusive`, never be folded into behavioral evidence + * (R9) — a timed-out suite is not a "fail", and a timed-out baseline must not + * satisfy the `!baselineResult.success` branch that upgrades a proof to "pass". + */ +function infraFailureReason(result: VerificationCommandResult): string | null { + if (result.timedOut) return "command timed out"; + if (result.aborted) return "command aborted"; + if (result.executionError) return "command could not be executed (setup/sandbox error)"; + return null; +} + function joinUrl(baseUrl: string, pathPart: string): string { const base = baseUrl.replace(/\/+$/, ""); if (!pathPart) return base; diff --git a/packages/engine/src/verification-utils.ts b/packages/engine/src/verification-utils.ts index 7cd54a68fe..1b9c2236e8 100644 --- a/packages/engine/src/verification-utils.ts +++ b/packages/engine/src/verification-utils.ts @@ -23,6 +23,24 @@ export interface VerificationCommandResult { success: boolean; /** True when this result was satisfied from the verification cache rather than running the command. */ cached?: boolean; + /** + * True when the command was terminated by the wallclock timeout rather than + * producing a real test/build verdict. Lets callers tell an *infrastructure* + * failure (timeout) apart from a genuinely failing test (`success === false` + * with a real exit code). + */ + timedOut?: boolean; + /** + * True when the command was aborted via the supplied `AbortSignal`. Like + * {@link timedOut}, this is an infra outcome, not behavioral evidence. + */ + aborted?: boolean; + /** + * True when the command could not be executed at all (spawn/setup failure, + * sandbox error) — distinct from a command that ran and exited non-zero. An + * infra outcome, not behavioral evidence. + */ + executionError?: boolean; } /** Result of running all verification commands */ @@ -121,8 +139,14 @@ function toLegacyExecResult( export async function execWithProcessGroup( command: string, options: SandboxRunStreamingOptions, + /** + * Explicit sandbox backend to run under. When omitted, falls back to the + * process-global resolution. Callers that must pin an isolating backend under + * concurrency (e.g. mission behavioral verification) pass it explicitly so they + * never depend on mutable global state. + */ + backend: SandboxBackend = getSandboxBackend(), ): Promise<{ stdout: string; stderr: string; bufferOverflow: boolean; aborted?: boolean }> { - const backend = getSandboxBackend(); const result = await backend.runStreaming(command, options); return toLegacyExecResult(command, result); } @@ -299,6 +323,12 @@ export async function runVerificationCommand( agentLabel?: string, /** Optional extra environment variables to inject into the child process (merged over process.env). */ extraEnv?: NodeJS.ProcessEnv, + /** + * Optional explicit sandbox backend. When omitted, the process-global backend + * is resolved. Pass this to pin an isolating backend without mutating global + * state (required for safe concurrent verification — see mission-verification). + */ + backend?: SandboxBackend, ): Promise { const logger = log ?? { log: console.log, error: console.error, warn: console.warn }; const label = (agentLabel ?? "merger") as AgentRole; @@ -324,13 +354,17 @@ export async function runVerificationCommand( const verificationStartedAt = Date.now(); try { - const { stdout, stderr, bufferOverflow } = await execWithProcessGroup(command, { - cwd: rootDir, - timeout: VERIFICATION_COMMAND_TIMEOUT_MS, - maxBuffer: VERIFICATION_COMMAND_MAX_BUFFER, - signal, - ...(extraEnv !== undefined && { env: extraEnv }), - }); + const { stdout, stderr, bufferOverflow } = await execWithProcessGroup( + command, + { + cwd: rootDir, + timeout: VERIFICATION_COMMAND_TIMEOUT_MS, + maxBuffer: VERIFICATION_COMMAND_MAX_BUFFER, + signal, + ...(extraEnv !== undefined && { env: extraEnv }), + }, + backend ?? getSandboxBackend(), + ); if (signal?.aborted) { throw Object.assign( @@ -391,6 +425,22 @@ export async function runVerificationCommand( || String(err?.message ?? "").includes("maxBuffer"); result.success = maxBufferExceeded && result.exitCode === 0; + // Classify infra outcomes so callers can tell a timeout/abort/setup failure + // apart from a real failing test (a command that ran and exited non-zero). + // A real test failure carries a numeric exit code; these do not. + if (!result.success && !maxBufferExceeded) { + const errish = err as { code?: number | string; killed?: boolean; aborted?: boolean }; + if (errish.code === "ETIMEDOUT" || (errish.killed && result.exitCode === null)) { + result.timedOut = true; + } else if (errish.code === "ABORT_ERR" || errish.aborted) { + result.aborted = true; + } else if (result.exitCode === null) { + // No exit code and not a recognized success → the command could not be + // run to a real verdict (spawn/setup/sandbox error), not a test failure. + result.executionError = true; + } + } + if (result.success) { logger.log(`${taskId}: ${type} command succeeded (exit 0, output exceeded buffer) in ${verificationDurationMs}ms`); await store.logEntry( diff --git a/packages/engine/vitest.config.ts b/packages/engine/vitest.config.ts index d1fffec9ef..691f8a3286 100644 --- a/packages/engine/vitest.config.ts +++ b/packages/engine/vitest.config.ts @@ -84,11 +84,12 @@ export default defineConfig({ "src/__tests__/heartbeat-monitor.test.ts", "src/__tests__/workflow-node-handlers.test.ts", ], - exclude: [ - "node_modules/**", - "dist/**", - "src/__tests__/merger-file-scope-invariant.test.ts", - ], + // No per-file quarantine excludes needed here: engine-core's + // membership is the explicit include allow-list above, so any + // quarantined file (e.g. merger-file-scope-invariant.test.ts) is + // already absent. The quarantine excludes live in engine-default, + // whose `src/**/*.test.ts` glob is what would otherwise pick them up. + exclude: ["node_modules/**", "dist/**"], }, }, { diff --git a/plugins/fusion-plugin-agent-browser/src/__tests__/driver.test.ts b/plugins/fusion-plugin-agent-browser/src/__tests__/driver.test.ts index 66fe8708cc..7cc6d494b1 100644 --- a/plugins/fusion-plugin-agent-browser/src/__tests__/driver.test.ts +++ b/plugins/fusion-plugin-agent-browser/src/__tests__/driver.test.ts @@ -11,7 +11,7 @@ import { // A mocked element/page/context/browser stack. Real browser automation is NOT // exercised in the merge gate — only the driver's wiring against this mock is. function makeMockStack(overrides?: { - selectorResolver?: (selector: string) => AutomationElement | null | "throw"; + selectorResolver?: (selector: string) => AutomationElement | null | "throw" | "throw-non-timeout"; pageUrl?: string; }) { const clickSpy = vi.fn(async () => {}); @@ -27,7 +27,17 @@ function makeMockStack(overrides?: { const gotoSpy = vi.fn(async () => ({})); const waitForSelectorSpy = vi.fn(async (selector: string) => { const r = overrides?.selectorResolver ? overrides.selectorResolver(selector) : element; - if (r === "throw") throw new Error("Timeout 10000ms exceeded waiting for selector"); + if (r === "throw") { + // Mimic Playwright's TimeoutError (identified by `name`): the selector + // never appeared → a real `absent` observation. + const timeoutErr = new Error("Timeout 10000ms exceeded waiting for selector"); + timeoutErr.name = "TimeoutError"; + throw timeoutErr; + } + if (r === "throw-non-timeout") { + // A non-timeout fault (e.g. a malformed selector): NOT absence. + throw new Error("Unknown engine 'bogus' while parsing selector"); + } return r; }); @@ -185,6 +195,15 @@ describe("browser driver — absence is a real observation, not inconclusive", ( const out = await launched.session.observe(".gone"); expect(out.status).toBe("absent"); }); + + it("observe of a non-timeout waitForSelector fault resolves to inconclusive (not absent)", async () => { + const { client } = makeMockStack({ selectorResolver: () => "throw-non-timeout" }); + const launched = await launchBrowserDriver({ client, executablePath: NODE_BIN }); + if (launched.status !== "ready") throw new Error("expected ready"); + const out = await launched.session.observe("css=bogus>>>nonsense"); + expect(out.status).toBe("inconclusive"); + if (out.status === "inconclusive") expect(out.reason).toBe("driver-error"); + }); }); describe("browser driver — teardown", () => { diff --git a/plugins/fusion-plugin-agent-browser/src/driver.ts b/plugins/fusion-plugin-agent-browser/src/driver.ts index e2196088fa..753df8b24c 100644 --- a/plugins/fusion-plugin-agent-browser/src/driver.ts +++ b/plugins/fusion-plugin-agent-browser/src/driver.ts @@ -247,9 +247,19 @@ function makeSession(browser: AutomationBrowser, context: AutomationContext, pag let el: AutomationElement | null; try { el = await page.waitForSelector(selector, { timeout, state: obsOpts?.expectAbsent ? "attached" : "visible" }); - } catch { - // waitForSelector rejects on timeout: the element never appeared. - return { status: "absent", url: page.url() }; + } catch (err) { + // Only a Playwright TimeoutError means the element never appeared — a real + // `absent` observation. Any other rejection (e.g. a malformed selector or + // invalid options) is a driver fault, not a negative observation, and must + // surface as `inconclusive` rather than be silently misreported as absent. + if (isTimeoutError(err)) { + return { status: "absent", url: page.url() }; + } + return { + status: "inconclusive", + reason: "driver-error", + detail: `observe ${selector} failed at ${page.url()}: ${errMsg(err)}`, + }; } if (!el) return { status: "absent", url: page.url() }; try { @@ -300,6 +310,18 @@ function errMsg(err: unknown): string { return err instanceof Error ? err.message : String(err); } +/** + * True only for Playwright's `TimeoutError`, which signals the selector never + * appeared within the timeout. Detected structurally by `name` so the driver + * stays decoupled from playwright-core's type surface (the real client is loaded + * lazily, and tests inject a mock that never imports it). `waitForSelector` can + * also reject for non-timeout reasons (malformed selectors, invalid options); + * those are NOT absence and must not be mapped to `absent`. + */ +function isTimeoutError(err: unknown): boolean { + return err instanceof Error && err.name === "TimeoutError"; +} + /** * Build the real automation client from `playwright-core`, adapting its * chromium API to the structural `BrowserAutomationClient` surface. Imported