Address PR review feedback (#1666)
- mission-execution-loop: preserve judge "blocked" verdict (don't downgrade to fail), clear validation timeout, backfill omitted linked assertions, notify autopilot "blocked" on exhausted retry budget
- mission-verification: pass selected sandbox backend explicitly to fix concurrent global-override race (fail-closed isolation); distinguish infra outcomes (timeout/abort) from real test failures so they stay inconclusive
- mission-store: validate run-to-feature ownership before fix-feature dedup/creation
- settings-sync routes: persist pulled global settings via store; qualify workflow field keys with workflowId
- useNodeSettingsSync: drop redundant ?? {} guard (backend always emits diff.workflowSettings)
- driver(agent-browser): map only Playwright TimeoutError to "absent" in observe(); other faults -> inconclusive
- vitest.config: remove redundant engine-core exclude (quarantine lives in engine-default)
- tests: tighten timeout verdict assertion to inconclusive; remove duplicate schema-version assertions
- save-split: document why customProviders is excluded (persisted via dedicated provider routes)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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 }>;
|
||||
|
||||
@@ -2987,6 +2987,11 @@ export class MissionStore extends EventEmitter<MissionStoreEvents> {
|
||||
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.
|
||||
//
|
||||
|
||||
@@ -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<Settings> = {};
|
||||
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;
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
|
||||
@@ -114,6 +114,10 @@ class MockStore extends EventEmitter {
|
||||
};
|
||||
}
|
||||
|
||||
async updateGlobalSettings(patch: Record<string, unknown>) {
|
||||
return patch;
|
||||
}
|
||||
|
||||
listWorkflowSettingValuesForProject(): Record<string, Record<string, unknown>> {
|
||||
return {};
|
||||
}
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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: [] };
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
|
||||
|
||||
@@ -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<typeof setTimeout> | undefined;
|
||||
const timeoutPromise = new Promise<never>((_, 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<ValidationResult> {
|
||||
// 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<void> {
|
||||
// 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);
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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<VerificationCommandResult> {
|
||||
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(
|
||||
|
||||
@@ -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/**"],
|
||||
},
|
||||
},
|
||||
{
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user