Repository navigation
Prevent incomplete intents from reaching success and publication - #534
Conversation
jeromevdl
left a comment
jeromevdl
left a comment
There was a problem hiding this comment.
P2 — A saved skeleton that a new DAG no longer supports now fails the run instead of falling back
Where: lambda/agentcore/commands/promote-units.js:136 (keeps the saved value), lambda/v2-orchestrator/index.js:1902-1915 (saves the auto-default), lambda/v2-orchestrator/section.js:384 (fallback removed), index.js:1908 and :1939 (the run fails).
What happens
- On the first promotion,
effectiveSkeletonpicks the first dependency-free unit and stores it in UNITPLAN. Fan-out approval writes the effective skeleton again, even when the human didn't override it (index.js:1902-1915). After that, an auto-default and a human choice look the same. - A rewind to the unit-DAG stage or earlier regenerates the DAG. Units are often renamed, or their dependencies change.
- Re-promotion keeps the saved
walkingSkeletonunchanged (promote-units.js:136). It only emitsv2.units.decisions_invalid. defaultSkeletonFor(section.js:384) no longer replaces a slug that isn't in the plan. On the new path it returns the saved value.- The fan-out gate shows the problem text, but the only way to fix it is typing a JSON
walkingSkeletonoverride into the approve answer.GateCardhas no picker. - A plain approval reaches
skeletonProblem(index.js:1908), thenfail('invalid_walking_skeleton')(index.js:1939), and the intent becomesFAILED.
Before vs. after: before, a rewind with renamed units silently picked a valid dependency-free unit and continued. Now the same rewind followed by a plain approval fails the intent, which then needs the recovery flow.
Fix: keep a saved skeleton only while it is still a valid dependency-free unit in the new DAG; otherwise use the first candidate. Alternatively, ask again at the gate instead of failing. Add a test for re-promotion with renamed units.
P2 — Recovery is offered when the failure happened before the parallel section
Where: lambda/intents/lane-repair.js:12-44, frontend/src/pages/IntentView.tsx:373.
What happens
- A rewind to a shared stage before the section resets the unit rows to
PENDINGbut keeps UNITPLAN (index.js:4972). - That earlier stage then fails, and the intent becomes
FAILED. unitCompletionreportsincomplete, so:- the UI shows Incomplete work instead of
FAILED; - the banner says "the recorded completion did not finish the current plan";
- Preview recovery is offered.
- the UI shows Incomplete work instead of
incompleteSectionRepairnever checks whether the stages before the section completed. It plans a restart from the section's first stage.- Confirm and continue retires the run, archives artifacts, resets stages and relaunches.
- The orchestrator's upstream check (
v2-orchestrator/index.js:744-784) then fails immediately withrewind_upstream_incomplete.
Impact: the status shown is misleading, and a destructive recovery cycle runs only to fail.
Fix: in incompleteSectionRepair, require every stage before the section to be SUCCEEDED or SKIPPED (the same rule as the orchestrator's upstream check), and show Incomplete work only in that case.
P2 — SUCCEEDED intents can be re-run through the generic repair path
Where: lambda/intents/index.js:4148 (status check), index.js:4192 (prUnits), index.js:4622 (completionSkip: null).
What happens
-
SUCCEEDEDis now accepted for every repair type, not only incomplete-section recovery. -
For a complete intent, or one whose partial delivery was accepted,
incompleteRepairis null, so the older PR-review path applies. -
prUnitsfilters onrecords.unitswithout restricting to the current plan. A row from an earlier plan, or a human-skipped unit left inPR_DRAFT, can still qualify. -
The repair then:
- moves the intent from
SUCCEEDEDback toCREATED; - clears
completionSkip; - reruns the section.
This path doesn't require a preview token.
- moves the intent from
-
The UI never shows the button in this state. An owner or admin calling the API directly can still trigger it.
Impact: a delivered intent can be reopened, and a recorded human skip undone. That contradicts the docs: accepted partial delivery must not be automatically undone.
Fix: accept SUCCEEDED only when incompleteRepair is non-null.
P2 — The final-PR safety check passes when the graph record is missing
Where: lambda/intents/index.js:4303. The PR record comes from v2-orchestrator/index.js:2159-2200.
What happens
- Recovery should stop if the final intent PR is closed or merged.
- It looks for that PR only in Neptune
HAS_PRvertices (fetchPullRequests). - Those vertices are written by
record-pr, which is explicitly best-effort: a failure "never un-succeeds a run". - If that write failed,
finalPrsis empty, so recovery proceeds without checking the provider. The preview then says "Existing unit pull requests are reused". - A merged or closed final PR goes undetected.
Fix: if delivery already ran (the delivery stage row is SUCCEEDED, or a PR event exists) and no PR record is found, refuse recovery. Or look up the PR at the provider by intent branch.
| startedAt: meta.startedAt, | ||
| pendingHumanTaskId: null, | ||
| resumeRequired: null, | ||
| failureReason: 'lane_repair_in_progress', |
There was a problem hiding this comment.
P1 — An interrupted repair locks the intent permanently
lambda/intents/index.js:4447 sets the lock; the checks at :4153 and :1790-1802 refuse everything while it is set.
What happens:
the retirement write leaves the intent FAILED with failureReason: 'lane_repair_in_progress'. Only the success path or the JS catch clears it. If the Lambda times out (120s), runs out of memory or is killed mid-repair, the reason stays, and nothing makes it expire. After that:
repair and preview return 409 repair_in_progress;
rewind, recompose, cancel and DELETE return 409 from the new guard;
the only way out is POST /start. It accepts FAILED, isn't guarded and resets failureReason. But it relaunches without startAtStageId, so the reviewed recovery's guarantees are lost: no restart from the section boundary and no skeleton reselection. The UI's recovery button only calls /start when no stage row is FAILED; otherwise it calls rewind, which is blocked.
Why it's plausible: repair makes many serial calls for every lane: provider PR checks, StopDurableExecution, Neptune artifact archiving and session stops.
Before this PR: the same intermediate state could still be repaired or rewound. Now nothing in the product clears it; only a manual DynamoDB patch, which the docs forbid.
Tests: only the lane_repair_failed case is covered (
intents.test.js:6618). The docs' claim that "interrupted cleanup leaves recoverable FAILED state" holds only for thrown errors.
Fix: make the lock a lease. Store a repairStartedAt and treat an old lane_repair_in_progress as lane_repair_failed, so preview and recovery can take over. Add a test that leaves the lock set without the catch running. Optionally, guard /start the same way so recovery and restart can't race.
There was a problem hiding this comment.
@jeromevdl Fixed in cad1d0e. Repair now has a five-minute lease and retains the reviewed section, lane set and skeleton choice across a worker crash. An expired lease permits a fresh preview and conditional takeover; generic start cannot bypass it. Late-owner writes and cleanup are fenced, and the UI exposes interrupted recovery even for a stranded CREATED launch.
Validation: 324 targeted API/DynamoDB Local tests and 68 intent-view tests passed. The regression suspends the first worker without its catch running, expires its lease, recovers once, then resumes the old worker and verifies it cannot alter the successor. Normal hooks passed 1,438 tests. Final combined CI will follow the remaining review fixes.
There was a problem hiding this comment.
@jeromevdl Follow-up for the interrupted-repair finding: f2a88f3 preserves the reviewed recovery lease through the launch handoff. A successful Invoke acknowledgement no longer lets /start race the new execution while META is still CREATED. The guard retires when the orchestrator claims its new run; if that claim never happens, the expired lease offers the same reviewed recovery preview.
Validation: 350 intent API tests passed, including the CREATED handoff, blocked /start, expired-preview recovery, and successful new-run claim; normal commit hooks passed 363 tests. Combined final validation/CI is being run separately.
| sectionIndex: segment.index, | ||
| prompt: [ | ||
| `Lane failure in section ${segment.index} (${failedSlugs.length} unit(s) failed, ${blocked().length} blocked, ${ | ||
| `Unresolved work in section ${segment.index} (${failedSlugs.length} unit(s) need a decision, ${blocked().length} blocked, ${ |
There was a problem hiding this comment.
Minor: the halt prompt counts units twice. failedSlugs now includes blocked units, and blocked units are counted again separately.
There was a problem hiding this comment.
@jeromevdl Fixed in 077bb63. The halt prompt now counts failed and blocked units as disjoint categories and labels integrated work separately; explicitly omitted lanes are excluded from unresolved counts.
Validation: 122 orchestrator tests passed, including rendered gate prompts for blocked-only work (0 failed, 1 blocked) and a failed prerequisite with its blocked dependent (1 failed, 1 blocked). Both still stop shared work without a recorded outcome. Normal hooks passed 582 tests.
| resumeRequired: null, | ||
| failureReason: 'rewind_in_progress', | ||
| completedAt: null, | ||
| ifOrchestratorRunId: meta.orchestratorRunId, |
There was a problem hiding this comment.
the new ifOrchestratorRunId: meta.orchestratorRunId condition throws an uncaught ConditionalCheckFailedException when it loses a race. The caller gets a 500; a 409 would be clearer.
There was a problem hiding this comment.
@jeromevdl Fixed the rewind caller in 764bcf3. An ownership condition lost during retirement now returns 409 execution_changed with a refresh/review instruction. Unexpected storage errors remain errors.
Validation: 324 intents tests passed. The regression changes the run owner after archival but before the retirement write, then checks that the pending gate, completed stages and integrated unit remain intact and no session stop or orchestrator launch occurs. A separate storage-failure case still returns 500. Normal hooks passed.
| await store.updateExecution({ | ||
| executionId: intentId, | ||
| fromStatus: meta.status, | ||
| ifOrchestratorRunId: meta.orchestratorRunId, |
There was a problem hiding this comment.
the new ifOrchestratorRunId: meta.orchestratorRunId condition throws an uncaught ConditionalCheckFailedException when it loses a race. The caller gets a 500; a 409 would be clearer.
There was a problem hiding this comment.
@jeromevdl Fixed the separate recompose caller in 9a5be53. A lost retirement condition now returns 409 execution_changed and asks the caller to refresh the stage selection. Unexpected storage failures remain errors.
Validation: 326 intents tests passed, including an owner change between archival and retirement. The regression verifies that the pending gate and stage remain unchanged, the grid is not replaced, and no session stop or relaunch happens. The storage-failure control still returns 500. Normal hooks passed.
| if ( | ||
| intentId && | ||
| (httpMethod === 'DELETE' || | ||
| (httpMethod === 'POST' && /\/(rewind|recompose|cancel)$/.test(path ?? ''))) | ||
| ) { | ||
| const current = await store.getExecution(intentId, { consistentRead: true }); | ||
| if (current?.projectId === projectId && current.failureReason === 'lane_repair_in_progress') { | ||
| return response(409, { | ||
| error: 'Recovery is in progress. Refresh before changing this execution.', | ||
| code: 'repair_in_progress', | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
the early repair guard matches any DELETE with an intentId (httpMethod === 'DELETE', line 1792), not only intent deletion.
There was a problem hiding this comment.
@jeromevdl Fixed in 0ceb792. The early guard now matches the intent DELETE route and the named execution actions only. The actual intent-delete dispatcher also excludes unknown nested DELETE paths.
Validation: 327 intents tests passed. During a repair, root deletion still returns repair_in_progress; attachment deletion reaches its own DRAFT-only validation, and an unknown nested DELETE cannot delete the intent. Normal hooks passed.
| export const repairPreviewToken = (meta, records, preview) => | ||
| createHash('sha256') | ||
| .update( | ||
| JSON.stringify({ | ||
| run: meta.orchestratorRunId ?? null, | ||
| status: meta.status, | ||
| updatedAt: meta.updatedAt, | ||
| plan: records.unitPlan, | ||
| units: records.units, | ||
| stages: records.stages, | ||
| preview, | ||
| }), | ||
| ) |
There was a problem hiding this comment.
(repairPreviewToken, hashes meta.updatedAt at line 76 and records.stages at line 79), checked at lambda/intents/index.js:4396 and :4401: for a RUNNING orphaned-lane repair, sibling lanes that are still active can keep changing these and invalidating the preview.
There was a problem hiding this comment.
@jeromevdl Fixed in b6bcf11. Orphaned-lane preview tokens now ignore output timestamps, routine sibling stage progress and equivalent active states. They still bind the run, plan, candidate integration/skip decisions, and orphaned gate attempt/callback identity. The API re-reads META immediately before claiming retirement after provider checks.
Validation: 333 intents tests passed. A preview remains usable while a sibling finishes a stage and provider reads advance META; its completed stage is preserved. Separate run, DAG, integrated-candidate, autonomy and new-question changes all reject the old preview before cleanup. Normal hooks passed.
There was a problem hiding this comment.
@jeromevdl Follow-up for preview stability: a24a991 re-reads the relevant execution records after provider checks, before the conditional claim. Integration, decision or gate changes during those reads return 409 before cleanup; a sibling stage that finishes during the reads remains SUCCEEDED without a reset or attempt increment.
All four new API regressions failed before this correction and now pass. Validation: 354 intent API tests and 367 tests in normal commit hooks passed. Combined final validation/CI will be reported separately.
|
@jeromevdl Addressed “A saved skeleton that a new DAG no longer supports now fails the run instead of falling back” from your review in 5e9ec41. Re-promotion now preserves valid choices and selects a dependency-free default when the regenerated DAG invalidates the old selection. It records that change before the normal fan-out approval; original answers remain in history. Validation: 143 promotion/orchestrator tests passed, including the actual promotion command followed by plain approval for renamed/dependency-changed skeletons, the ceremony-disabled control, valid saved overrides and legacy absent/empty selections. Invalid choices in an already approved plan still cannot execute. Normal hooks passed. |
|
@jeromevdl Addressed “Recovery is offered when the failure happened before the parallel section” from your review in cebc3b4. Preview/admission now require every upstream stage instance to be SUCCEEDED or SKIPPED, matching the orchestrator. The detail API reports eligibility, so an earlier failure stays FAILED and retains its existing stage retry instead of offering destructive section recovery. Validation: 343 backend tests and 69 intent-view tests passed. Coverage includes missing/FAILED/PENDING/RUNNING upstream rows, SUCCEEDED/SKIPPED controls, earlier parallel instances, zero cleanup on rejection, and clicking the correct upstream retry action. Normal hooks passed. |
|
@jeromevdl Addressed “SUCCEEDED intents can be re-run through the generic repair path” from your review in 0c7bd2a. SUCCEEDED now requires an actual incomplete-section repair; generic orphan/PR-review repair cannot reopen completed or accepted partial delivery. Validation: 343 intents tests passed, including historical PR_DRAFT rows outside the plan, plan-bound skip receipts, legacy accepted halt/skip answers, and genuine incomplete recovery that leaves another unit’s skip intact. Rejected preview and direct API requests make no writes. Normal hooks passed. |
|
@jeromevdl Addressed “The final-PR safety check passes when the graph record is missing” from your review in 844b029. Recovery now refuses Validation: 353 backend tests and 69 intent-view tests passed. Tests remove the graph vertex and cover success, PR-open events, completed/skipped final shared work and tracker receipts; preview and direct recovery make no writes. Provider failures remain 502, closed/merged PRs remain blocked, and verified open-final-PR and pre-delivery recovery remain supported. Normal hooks passed. |
JWThewes
changed the title
|
@jeromevdl I've reworked this following your scope and completion-ownership feedback. The restructuring is in 324cb24; current head is a870e6b.
Validation: 5,287 backend tests passed (3 existing skips), 836 frontend tests and production build, 97 release tests, plus static checks and replay/ordinary-recovery regressions. All seven current-head CI checks pass. The two follow-ups were also merged locally without conflicts and passed their combined checks. No deployment was performed as part of this restructuring. Could you re-review the narrowed scope and shared completion ownership? |
There was a problem hiding this comment.
The restructure resolves most of my first review: the repair-recovery findings no longer apply, and the skeleton finding and the optional nits are fixed. Three new issues come from the completion check that is now part of the PR.
P1 — Units integrated before deploy in multi-repo intents block any later run
Where: lambda/shared/unit-completion.js:128-150 (integrationProblem, the repos.length > 1 branch at :147), lambda/v2-orchestrator/section.js:541-556.
What happens
- Before this PR, the merge step stored only
mergedAt. It now also stores per-repositoryintegrationResults. - For multi-repo intents that don't use one PR per unit, a
MERGEDrow withoutintegrationResultsis treated asrepository_evidence_missing, i.e.unknown. - A new history on a pre-deploy intent hits this. A new history comes from:
- rewind to a stage after the section;
/startretry of aFAILEDintent;- recompose.
- The result depends on where the run picks up:
- rewind past the section: everything runs, then the final step marks the intent
FAILEDwithcompletion_unverifiedand doesn't open the PR; /startretry: the section-start check (section.js:555) fails immediately.
- rewind past the section: everything runs, then the final step marks the intent
- The only way out is rewinding into the section, which resets and rebuilds work that was already integrated.
Before vs. after: before, these retries completed normally. Single-repo and one-PR-per-unit intents are not affected.
Fix: treat a MERGED row with no integrationResults attribute as a pre-deploy row and accept it as integrated, the same way policy 0 did. Every policy-2 merge writes the field, even as [], so a missing attribute reliably identifies an old row. Keep the strict per-repository check for rows that have the field. Please add a regression test: a multi-repo intent with pre-deploy MERGED rows, then rewind past the section and /start retry, both reaching SUCCEEDED.
P2 — Old successful intents without a release pin show "Completion unverified", even serial ones
Where:
lambda/shared/unit-completion.js:331, lambda/intents/index.js:5254 and frontend/src/pages/IntentView.tsx:346, :425, :596
What happens:
- GET on a terminal intent calls
assessExecutionCompletion. - That function returns unknown (
legacy_workflow_unpinned) whenever there is nomethodologyRelease, nomethodologyPins.STAGEand no saved completion result. This check runs before it looks at whether the intent has a unit plan at all. - Every pre-deploy
SUCCEEDEDintent created without a release pin is affected, including serial workflows with no units. That is a supported creation path:selectedReleasePincan be null,index.js:5464 and :5598. - Those intents get the badge Completion unverified, the banner "Completion could not be verified… before publishing", and "0 of 0 units verified integrated".
- No test covers an unpinned intent without units.
Fix: return null (or complete) when records.unitPlan?.units is empty, before the pin check. Add a GET test for a serial, unpinned SUCCEEDED intent.
Also move the CANCELLED/abort check ahead of the legacy_workflow_unpinned return: an unpinned CANCELLED intent with units is currently reported as unknown instead of aborted. #534's UI doesn't show this, but #539's /audit card and audit-completion --status CANCELLED do (counted as unknown, exit 2).
P2 (maintainability) — Policy 1 was never released; collapse to a single new version
completionPolicyVersion does not exist on main (git log -S completionPolicyVersion origin/main is empty), and the deploy workflows have never run from this branch. Policy 1 and policy 2 are both introduced by this PR, so the only histories in the field are marker-absent (policy 0).
The >= 1 / >= 2 split therefore keeps an unreachable third path: the frozen policy-1 branch in validateCompletion, checkedCompletion vs completionPolicyVersion >= 2 checks in section.js/index.js, test/fixtures/completion-v1-boundary.js and its replay tests. Every future change to the section runner would have to preserve it.
Unless policy 1 was deployed manually somewhere with live histories, please make the current behaviour version 1, keep the policy-0 legacy path, and drop the intermediate one.
A dependent walking skeleton or blocked-only lanes could let an intent finish while required current-plan work remained. Validate skeleton and wave selection, halt on unresolved lanes, and use one shared completion evaluator (lambda/shared/unit-completion.js) at section fan-in, final success and initial publication for new histories. Required integration writes are no longer best-effort: an unrecorded integration stops the lane, a lost response is reconciled from persisted state, and final status commits with its completion receipt under run, status, META revision and unit-plan conditions. Marker-absent histories keep their durable operation sequence. Review corrections included: re-promotion reselects a valid skeleton for a regenerated DAG, halt prompts count failed and blocked lanes separately, rewind/recompose return 409 execution_changed when retirement loses run ownership, and intent DELETE matches only the intent route. Squashes the reviewed restructure (originally 8018321..a870e6b) onto main; the deferred preservation-recovery work is not included.
9288d89 to
b6883ab
Compare
|
@jeromevdl Addressed Units integrated before deploy in multi-repo intents block any later run from your review in e6f64ed. A |
|
@jeromevdl Addressed Old successful intents without a release pin show "Completion unverified", even serial ones from your review in e6f64ed and b6883ab. An unpinned intent with no unit plan and no unit rows now gets |
|
@jeromevdl Addressed Policy 1 was never released; collapse to a single new version from your review in 3190291. The current behaviour is now completion policy 1 above the marker-absent policy-0 path. The intermediate branch in |
|
@JWThewes, a new blocker is introduced (same issue as #569), otherwise the rest is fixed. P0 — The orchestrator role lacks
|
The completion work adds the first orchestrator-issued transactions with ConditionCheck entries: updateForRun (integration writes, skip receipts, fan-out unit-plan decisions), deleteWorkflowCheckpoint with a run fence, and finalizeUnitCompletion with its unit-plan snapshot check. DynamoDB authorizes each ConditionCheck as dynamodb:ConditionCheckItem, which the orchestrator role never had. The call fails with AccessDeniedException rather than a conditional failure, so the orchestrator ends the intent FAILED with completion_unverified. DynamoDB Local does not enforce IAM, so the behavior tests cannot see this. Add a policy test, as the AgentCore role already has, and grant the action on the process table statement. Same class of failure as #569 for the intents role.
|
@jeromevdl Addressed The orchestrator role lacks A new |
jeromevdl
added
area/frontend
A dependent walking skeleton and blocked-only lanes could let an intent finish while required work remained. This change validates skeleton/wave selection, halts unresolved lanes, and uses one shared completion evaluator at section fan-in, final success, and initial publication.
The evaluator checks the resolved workflow's expected sections, current-plan units, persisted integration evidence and scoped approved omissions. Missing expected work, incomplete integrations and unavailable reads withhold success/publication and explain what remains. Required integration persistence failures stop dependent work; lost responses are reconciled against persisted evidence. Run/META/plan conditions fence finalization. Integrated labels distinguish code integration from final PR delivery.
Compatibility is explicit: pre-deploy MERGED rows without an integrationResults attribute remain integrated, including multi-repository retries and rewinds past completed sections. Every new-policy integration writes that attribute; explicit empty, invalid or failed evidence retains strict checks. PR-per-unit delivery still requires its authoritative repository records. Historical unpinned serial successes without unit work no longer show false unit-completion warnings. Cancellation and a valid current-run abort are classified before workflow-pin resolution; unfinished work remains visible and is not authorized for publication. Missing/truncated reads and unrelated or stale abort answers remain unverified.
The scope contains prevention and relevant review corrections. Existing orphan-lane repair and ordinary rewind remain, including omission reset, run-conflict 409 responses and exact intent-delete routing. Added preservation recovery for finished incomplete intents and its preview/lease/launch/compatibility protocols remain deferred with #536.
Original correction validation at e33d2cf on Node 24 with at most two backend workers and local Testcontainers:
Cancellation amendment at 9288d89: focused shared/API/final-completion/frozen-replay checks 391 passed; normal commit hooks 1,059 passed. Lint, formatting, secretlint, dependency/SDK checks and docs build passed. The 5,302-test local full-suite result above is historical for the preceding head; it was not repeated locally for this small read-only classification change. Exact-head CI is reported separately by GitHub.
Deploy the orchestrator, runtime, API and frontend from the same release. New histories capture completion policy 1 in the existing load-meta boundary. Marker-absent histories retain policy 0's sequence, including frozen checkpoints before and after terminal success. The unreleased intermediate policy path is removed; available main/release history and branch workflow records contain no evidence of its deployment. No status migration or runtime repinning is introduced.
This PR targets main. #537 (delivery status/retry) and #539 (bounded read-only audit) are independent follow-ups based directly on this PR; neither requires #536 or the other follow-up. CI and review approval remain separate gates.