Skip to content

Issue #3251: [Backend] Workflow run recovery capability and receipt contract - #3293

Open
louis4li wants to merge 2 commits into
feat/2026-08-07_issue-3250from
feat/2026-08-07_issue-3251
Open

Issue #3251: [Backend] Workflow run recovery capability and receipt contract#3293
louis4li wants to merge 2 commits into
feat/2026-08-07_issue-3250from
feat/2026-08-07_issue-3251

Conversation

@louis4li

@louis4li louis4li commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Issue

Closes #3251 — [Backend] Workflow run recovery capability and receipt contract

Implementation summary

See .implement-loop/runs/implement-issue-3251.md.

Stacked-PR position

🤖 Generated by codex-implement-loop. Reviewer is a Claude subagent (see PR comments for round-N review reports).

Expose typed recovery capability facts and routable fork receipt run ids while preserving existing actor-address receipt semantics.

Closes #3251

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@louis4li
louis4li requested a review from jason-aelf as a code owner August 7, 2026 08:21

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: df0cc74dd4

ℹ️ 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".

Comment on lines +95 to +96
RevisionId: seedView.RevisionId,
DefinitionVersion: Math.Max(0, seedView.DefinitionVersion)),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Clear source revision when workflow YAML is overridden

When InlineYaml or InlineSubYamls changes the source definition, this still stamps the new run with the source run's RevisionId and DefinitionVersion. The resulting committed state and recovery capability therefore claim an exact revision that the run did not execute, corrupting provenance and potentially causing later recovery to resolve the wrong definition; preserve these fields only when the effective YAML bundle is unchanged, otherwise resolve or clear the revision identity.

AGENTS.md reference: AGENTS.md:L62-L63

Useful? React with 👍 / 👎.

Comment on lines +550 to +557
if (ContainsAny(
error,
"authorization",
"unauthorized",
"forbidden",
"credential",
"access denied",
"grant"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Derive failure class from committed typed facts

When an ordinary tool or business error happens to contain a substring such as grant or configuration (for example, "grant proposal timed out"), this heuristic marks both retry and run-again as ineligible authorization/configuration failures even though recovery may be valid. Because FinalError is free-form display text, classify recovery from a committed typed failure outcome instead of reconstructing business semantics in the projector.

AGENTS.md reference: AGENTS.md:L83-L85

Useful? React with 👍 / 👎.

Comment on lines +53 to +54
if (source == null)
return new WorkflowRunRecoveryCapability();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Return legacy-unavailable actions when capability is absent

For a current-state document materialized before this field existed, source is null and this returns an empty capability whose actions have Unspecified eligibility and no reason. Such terminal historical documents normally receive no further committed event and the normal path does not replay them, so the nested legacy fallback below is never reached; construct both actions as Unavailable/LegacyUnavailable in this branch.

AGENTS.md reference: AGENTS.md:L84-L84

Useful? React with 👍 / 👎.

@louis4li

louis4li commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Review of PR #3293 — round 1

Verdict: rework
Issue: #3251 — [Backend] Workflow run recovery capability and receipt contract
Head: feat/2026-08-07_issue-3251 @ df0cc74
Base: feat/2026-08-07_issue-3250
Reviewed by: Claude subagent (codex-implement-loop)

Verdict rationale (one paragraph)

The PR adds the requested typed recovery fields and covers several acceptance cases, but two core contract points are not satisfied yet: recovery eligibility is derived from existing FinalError diagnostic text, giving error strings new control-flow meaning, and the production fork receipt populates the new routable RunId with the technical actor id even though the issue explicitly requires those identities to remain separate.

Findings

F1 — Recovery capability is classified from existing error text

  • Severity: blocking
  • Dimension: issue-intent
  • Location: src/workflow/Aevatar.Workflow.Projection/Projectors/WorkflowExecutionCurrentStateProjector.cs:544-587
  • Evidence: var error = state.FinalError ?? string.Empty; followed by substring checks for "authorization", "unauthorized", "credential", "configuration", and "workflow_input_file_binding_failed", then value.Contains(candidate, StringComparison.OrdinalIgnoreCase).
  • Why it's a problem: Issue [Backend] Workflow run recovery capability and receipt contract #3251 says "Existing error/status strings must not gain new control-flow meaning; new eligibility fields are the authority" and the architecture constraints say "Eligibility and recommendation must come from backend-owned capability/failure classification." This implementation creates typed fields, but their values are still controlled by legacy free-text diagnostics. It also conflicts with CLAUDE.md's strong-typing rule: "影响业务语义、控制流、稳定读取且仓库内可控的数据,必须建模为 proto field / typed option / typed sub-message".
  • What would change your verdict: Move the auth/configuration/missing-fact classification onto typed committed state/event facts or an existing typed failure classification produced by the owner of the failure. The projector should map those typed facts into WorkflowRunRecoveryCapabilityReadModel; it should not inspect FinalError substrings to decide eligibility or recommended actions. Update the tests so auth/config cases set typed failure facts rather than relying on particular error text.

F2 — Production receipt uses actor id as the new routable run id

  • Severity: blocking
  • Dimension: issue-intent
  • Location: src/workflow/Aevatar.Workflow.Infrastructure/Runs/WorkflowRunActorPort.cs:124-128 and src/workflow/Aevatar.Workflow.Infrastructure/Runs/WorkflowRunActorPort.cs:228-232
  • Evidence: return new WorkflowRunCreationReceipt(runActor.Id, definitionResolution.ActorId, createdActorIds, runActor.Id);
  • Why it's a problem: Issue [Backend] Workflow run recovery capability and receipt contract #3251 requires "Extend fork/dispatch acceptance receipt to expose the new routable runId when one exists" and "Keep newRunActorId as separate technical actor-address receipt data; it must not be used as run identity or URL parameter." The fake unit test returns "run-created" as actor id and "run-routable" as run id, but the real port passes runActor.Id for both ActorId and RunId. That means WorkflowCapabilityEndpoints.HandleForkRun can build /api/workflow/observatory/runs/{NewRunId} from the actor address in production, which is exactly the identity conflation the issue forbids.
  • What would change your verdict: Populate WorkflowRunCreationReceipt.RunId from an authoritative routable run identity distinct from the technical actor address, or leave it empty when no such identity exists so existing actor-address fallback remains honest. Add/adjust a test around the real WorkflowRunActorPort or resolver path that fails if RunId is silently copied from ActorId for newly created fork runs.

What's good

  • The proto/read-model additions are additive and strongly typed, with no field renumbering in the reviewed diff.
  • The observatory query surface now carries recovery capability through list and detail responses instead of requiring client-side inference.
  • The new tests cover eligible, missing-fact, classified auth/config, legacy unavailable, and receipt exposure paths, even though two of those paths need stronger source facts.

REVIEW_VERDICT:rework:typed recovery still derives from strings and actor id

Use typed recovery failure facts for eligibility and keep routable run ids distinct from actor addresses.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@louis4li

louis4li commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Review of PR #3293 — round 2

Verdict: pass
Issue: #3251 — [Backend] Workflow run recovery capability and receipt contract
Head: feat/2026-08-07_issue-3251 @ 33e1335
Base: feat/2026-08-07_issue-3250
Reviewed by: Claude subagent (codex-implement-loop)

Verdict rationale (one paragraph)

The round-2 diff satisfies the issue's backend recovery capability contract with additive typed proto/read-model fields for retry/run-again eligibility, unavailable reason codes, recommended actions, starting step, definition revision/version, output reuse, and cost semantics, and it preserves receipt compatibility while separating routable NewRunId from technical NewRunActorId. The two round-1 blockers are resolved: projection classification now maps WorkflowRunState.TerminalRecoveryFailureKind instead of inspecting FinalError, and production run creation no longer copies actor addresses into WorkflowRunCreationReceipt.RunId. I found no new blocking CLAUDE.md, scope, proto, test, external-repo, or stacked-PR hygiene issue in the reviewed diff.

Findings

No blocking findings.

What's good

  • The recovery capability surface is strongly typed end to end: WorkflowRecoveryEligibility, WorkflowRecoveryUnavailableReasonCode, WorkflowRecoveryRecommendedAction, WorkflowRecoveryActionCapability, and WorkflowRunRecoveryCapability are added as proto/read-model contracts rather than metadata bags, matching CLAUDE.md's rule that "影响业务语义、控制流、稳定读取且仓库内可控的数据,必须建模为 proto field / typed option / typed sub-message".
  • The query path stays read-model based: observatory list/detail responses clone snapshot.RecoveryCapability, consistent with CLAUDE.md "查询始终走 readmodel" and the issue's "Query APIs must read materialized read models only."
  • Tests cover the requested cases: eligible recovery, typed auth/config ineligibility, missing source facts, legacy unavailable failed-step facts, observatory exposure, receipt exposure, and the real WorkflowRunActorPort.CreateRunAsync regression that prevents RunId from mirroring ActorId.

Round comparison

  • Findings carried over from round 1: F1 (now resolved), F2 (now resolved).
  • New findings this round: none.
  • Net direction: improving

REVIEW_VERDICT:pass:typed recovery contract and run-id receipt separation pass

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant