feat(agent): enforce durable long-horizon execution - #66
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80ff6bbd00
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const directory = fs.openSync(path.dirname(file), 'r'); | ||
| try { | ||
| fs.fsyncSync(directory); |
There was a problem hiding this comment.
On Windows, opening a directory with fs.openSync(..., 'r') or syncing its descriptor can fail with EPERM/EINVAL; the existing session-store durability helper explicitly skips this operation there. Since acquiring or renewing every execution-owner lease calls writeJsonAtomic, durable graph scheduling can fail before any node starts on Windows. Guard the parent-directory fsync or tolerate the known unsupported errors.
AGENTS.md reference: AGENTS.md:L57-L57
Useful? React with 👍 / 👎.
| return this.store.append(graphId, { | ||
| expectedRevision: graph.revision, | ||
| type: 'node.retry_requested', | ||
| nodeId, | ||
| data: { requestedBy: 'user' }, |
There was a problem hiding this comment.
When restart recovery blocks an interrupted mutation, or a node enters merge_conflict, the CLI/Web UI exposes a retry action that reaches this method. It always appends node.retry_requested, but the projector only permits that event from failed or interrupted, so retries for both displayed recovery states throw EXECUTION_STATE_INVALID and leave the task unrecoverable. Either transition these states to ready here or stop presenting them as retryable.
AGENTS.md reference: AGENTS.md:L148-L148
Useful? React with 👍 / 👎.
| ctx.spawnSubagent!({ | ||
| task: t.task, | ||
| ...(t.writePaths ? { writePaths: t.writePaths } : {}), | ||
| scope: resolvedScopes[i], |
There was a problem hiding this comment.
For a full fan-out task, this now creates an isolated lease and the child returns workspaceLeaseId/patchId, but the fulfilled-result renderer below emits only r.summary; the patch is created after that summary was generated, so it cannot contain those handles. Consequently the parent cannot call merge_subagent_patch, while the active lease remains retained indefinitely and the implementation changes are effectively stranded. Include the lease and patch identifiers in each successful fan-out section.
Useful? React with 👍 / 👎.
| kind: 'analysis', | ||
| title: `expert ${nodeIndex + 1}`, | ||
| dependencies: [], | ||
| requiredCapabilities: ['architecture'], |
There was a problem hiding this comment.
The case recorded as subagents_concurrency calls runExecutionGraph with four ordinary analysis nodes and an in-process callback; it never invokes fan_out_subagents, the subagent runner, role routing, or workspace isolation. The test therefore remains green if actual concurrent subagent execution is completely broken, while long-horizon-results.json reports 10/10 real subagent concurrency evidence. Drive the real subagent/routed execution path before recording this result.
AGENTS.md reference: AGENTS.md:L135-L135
Useful? React with 👍 / 👎.
| writePaths: assignment.writePaths, | ||
| }) | ||
| : undefined; | ||
| const selected = router.route(assignment); |
There was a problem hiding this comment.
When a graph node supplies the newly public roleId, this call still routes without passing it as the preferred role. If multiple roles share the required kind and capabilities, the router silently chooses by capability count and ID, so the executor may receive a different role with different instructions, tools, or budget than the graph requested. Pass node.roleId to router.route so explicit role assignments are enforced.
Useful? React with 👍 / 👎.
| const agentRoleRegistry = | ||
| config.agentRoleRegistry ?? | ||
| new AgentRoleRegistry({ allowIsolatedWrite: config.allowPluginIsolatedWrite }); | ||
| const roleDisposers = (config.agentRoles ?? []).map((role) => agentRoleRegistry.register(role)); |
There was a problem hiding this comment.
When a caller supplies an external agentRoleRegistry and multiple config.agentRoles, registration happens eagerly inside Array.map. If a later role is duplicate or invalid, construction throws before the disposers are attached to pluginHost, leaving every earlier role registered in the caller-owned registry; retrying with corrected configuration can then fail on those leaked registrations. Stage the roles first or dispose already registered entries in the error path.
Useful? React with 👍 / 👎.
| if (routed.role.kind === 'implementer' && result.status === 'PASS') { | ||
| if (!workspaceLease || !result.patchRef || !input.authorizeMerge) { | ||
| invalid = true; | ||
| } else { | ||
| await input.authorizeMerge(workspaceLease, result.patchRef); | ||
| const merge = await input.workspaceLeases.mergeStored(workspaceLease.id, result.patchRef); |
There was a problem hiding this comment.
The result-contract checks set invalid before this block, but a result with status: 'PASS' is merged even when its assignment ID, role ID, or evidence references are invalid, and even when unmetCriteria is non-empty. The node is marked failed afterward, but the untrusted patch has already mutated the parent workspace. Require the complete result contract and zero unmet criteria before authorizing or applying the patch.
Useful? React with 👍 / 👎.
| for (const [index, criterion] of assignment.acceptanceCriteria.entries()) { | ||
| if (result.unmetCriteria.includes(criterion)) continue; | ||
| evidence.push({ | ||
| id: `criterion-${assignment.id}-${index}`, | ||
| kind: 'expert_claim', | ||
| nodeId: node.id, | ||
| summary: `Acceptance criterion satisfied: ${criterion}`, | ||
| createdAt: Date.now(), | ||
| metadata: { criterion, roleId: routed.role.id, runId: result.runId ?? assignment.id }, |
There was a problem hiding this comment.
Criterion evidence is emitted whenever the criterion is absent from unmetCriteria, regardless of result.status or the already computed invalid flag. A failed or contract-invalid attempt can therefore leave durable evidence claiming that acceptance criteria passed; after a later retry succeeds, CompletionArbiter matches criteria globally and can verify the graph using those stale false claims. Emit these records only after the result has passed every contract check.
AGENTS.md reference: AGENTS.md:L135-L135
Useful? React with 👍 / 👎.
| result: { | ||
| success: false, | ||
| error: errorMessage(error), | ||
| failureFingerprint: `executor:${error instanceof Error ? error.name : 'unknown'}`, | ||
| }, |
There was a problem hiding this comment.
Every ordinary thrown Error receives the same executor:Error fingerprint. Three unrelated failures—for example an authentication error, a transient network error, and an invalid response—are consequently counted as three consecutive occurrences of the same failure and block the node, even though the retry policy is intended to block repeated identical failures. Include a stable normalized cause or error code in the fingerprint.
Useful? React with 👍 / 👎.
| events(graphId: string, after = 0): readonly ExecutionEvent[] { | ||
| return (this.graphs.get(graphId) ?? []) | ||
| .filter((event) => event.seq > after) | ||
| .map((event) => ({ ...event, data: { ...event.data } })); |
There was a problem hiding this comment.
The in-memory adapter stores input.data by reference and returns only a shallow copy from events(). Nested objects such as data.evidence therefore remain shared with the authoritative event stream, so a caller that mutates its original input or an event returned for inspection silently changes future graph projections without an append or revision increment. Clone nested event data on ingress and egress to preserve the advertised immutable-event contract.
Useful? React with 👍 / 👎.
…tion feat(agent): enforce durable long-horizon execution
1 participant
Summary
Verification