Freeze workflow contracts and make PR reviews deterministic - #25
Conversation
📝 WalkthroughWalkthroughThis change adds immutable workflow contracts, frozen SCM grants, exact pull-request revisions, canonical review plans, changed-line validation, structured runner review output, and Console inspection of execution and coverage metadata. ChangesWorkflow contract and deterministic review
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
orchestrator/internal/api/review.go (1)
69-91: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove now-dead revision defaults from
newReviewRun.
newReviewRunsetsPRHeadBranch: src.GitBranch,PRBaseBranch: svc.DefaultBranch, andPRHeadSHA: src.CommitSHA(lines 146-148), buthandleRequestReviewunconditionally overwrites all three (plusPRBaseSHA, whichnewReviewRunnever sets) right after construction, at lines 77-80. The constructor's values are never observable in the current code path.Keep the source of truth in one place. Drop the dead assignments in
newReviewRunand setPRHeadBranch/PRBaseBranch/PRHeadSHA/PRBaseSHAonly where they are actually frozen (handleRequestReview). Leaving misleading defaults here creates a real risk: a future maintainer could see the constructor already "setting" these fields and remove the explicit overwrite inhandleRequestReview, silently reintroducing unfrozen, potentially stale head/base values instead of the PR's live revisions.🐛 Proposed fix
func newReviewRun(src *domain.Run, svc *domain.Service, triggeredBy *string) *domain.Run { return &domain.Run{ ID: domain.NewID(), ProjectID: src.ProjectID, ServiceID: src.ServiceID, Prompt: "AI review of PR " + src.PRURL, Status: domain.StatusQueued, Kind: domain.RunKindReview, Phase: "Queued", TriggeredByUserID: triggeredBy, - PRHeadBranch: src.GitBranch, - PRBaseBranch: svc.DefaultBranch, - PRHeadSHA: src.CommitSHA, PRURL: src.PRURL, PRNumber: src.PRNumber, Attempt: 1, CreatedAt: time.Now().UTC(), } }Also applies to: 136-154
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/api/review.go` around lines 69 - 91, Remove the PR revision field assignments from newReviewRun, including PRHeadBranch, PRBaseBranch, and PRHeadSHA; do not add PRBaseSHA there. Keep all four revision fields assigned only in handleRequestReview after reviewTargetAtCreation succeeds, preserving the frozen live PR revisions.
🟡 Minor comments (13)
orchestrator/internal/store/plugins_test.go-662-665 (1)
662-665: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFail when fixture setup fails.
The test ignores errors from project, service, provider-config, and provider-config lookup setup. A setup failure can make
ClaimRunDispatchreturnErrDispatchClaimUnavailablefor an unrelated reason. Check each error witht.Fatal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/store/plugins_test.go` around lines 662 - 665, Update the test fixture setup around CreateProject, CreateService, UpsertProviderConfig, and GetProviderConfig to capture each returned error and call t.Fatal when any operation fails. Ensure ClaimRunDispatch runs only after all project, service, provider configuration, and lookup setup succeeds.orchestrator/internal/reconciler/pr_test.go-130-131 (1)
130-131: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCheck the discarded errors in the test setup.
Lines 130-131 discard the errors from
cipher.EncryptString. Line 166 discards the error fromst.GetPluginInstallation. If the lookup on Line 166 fails,liveis nil and Line 167 panics with a nil dereference instead of reporting the real setup failure. Assert each error witht.Fatalso a setup break names its own cause.💚 Proposed fix
- oldToken, _ := cipher.EncryptString("old-bot-token") - newToken, _ := cipher.EncryptString("new-bot-token") + oldToken, err := cipher.EncryptString("old-bot-token") + if err != nil { + t.Fatal(err) + } + newToken, err := cipher.EncryptString("new-bot-token") + if err != nil { + t.Fatal(err) + }- live, _ := st.GetPluginInstallation(ctx, installation.ID) + live, err := st.GetPluginInstallation(ctx, installation.ID) + if err != nil { + t.Fatal(err) + }Rename the later
candidates, err :=binding accordingly so the shadowing stays valid.Also applies to: 166-166
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/reconciler/pr_test.go` around lines 130 - 131, In the test setup around the EncryptString calls and GetPluginInstallation lookup, stop discarding returned errors and fail immediately with t.Fatal, including each underlying error. Preserve the existing live lookup and candidate logic, renaming the later candidates, err binding if needed to avoid shadowing after introducing the checked error variable.runner/entrypoint.sh-547-554 (1)
547-554: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReport oversized review diffs with
review_input_too_large.
post-review-planuploads the entire diff, and the orchestrator rejects any diff larger thanMaxReviewDiffBytes(2 << 20) withreview_input_too_large. The currentdiemessage cannot distinguish that from upload failures. CheckREVIEW_DIFF_BYTESlocally or include the server error reason when the upload fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@runner/entrypoint.sh` around lines 547 - 554, The review-plan upload path should distinguish oversized diffs from generic upload failures. Use the existing REVIEW_DIFF_BYTES value after computing the diff and, when it exceeds MaxReviewDiffBytes (2 << 20), fail with setup_failed and the review_input_too_large reason; otherwise preserve the existing post-review-plan upload handling and include any server-provided failure reason when available.docs/29-workflow-contract-and-review-design.md-75-79 (1)
75-79: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the migration filename.
This design names
0064_workflow_contract.sql, but the reviewed migration isorchestrator/internal/store/migrations/0064_workflow_contract_review_plan.sql. Update the reference so operators and reviewers can locate the migration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/29-workflow-contract-and-review-design.md` around lines 75 - 79, Update the migration reference in the Data design section to use 0064_workflow_contract_review_plan.sql, matching the reviewed migration filename so operators and reviewers can locate it.docs/30-open-source-agent-workflow-benchmark.md-60-61 (1)
60-61: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the timeout conclusion with the POC budgets.
The sentence says a 30-minute maximum covers the first POC scenarios.
docs/31-workflow-case-conversations-and-video-agent-poc.mdlists 45–180 minute budgets. A 30-minute limit cannot cover those cases. State the required maximum separately from the benchmark observation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/30-open-source-agent-workflow-benchmark.md` around lines 60 - 61, Update the timeout conclusion in the benchmark document to distinguish the observed 30-minute server maximum from the POC requirement. State that the listed POC scenarios require a higher maximum based on their 45–180 minute budgets, rather than claiming 30 minutes covers them.docs/31-workflow-case-conversations-and-video-agent-poc.md-61-66 (1)
61-66: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the unsupported deprecation assertion.
The template flow is described as the current
/v1/video_template_generationcreate/query flow; only add a provider compatibility seam if the decision is product-risk, not because that endpoint is officially deprecated. If you keep the seam, add the official deprecation source and cutoff to avoid documenting false API lifecycle status.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/31-workflow-case-conversations-and-video-agent-poc.md` around lines 61 - 66, Update the “API 名称校正” section to remove the unsupported claim that /v1/video_template_generation is officially deprecated. Describe the template create/query flow as current, and retain the provider compatibility seam only if justified as a product-risk decision; if retained, cite the official deprecation source and cutoff.design/README.md-102-105 (1)
102-105: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove these entries into the Page map table.
Line 102 adds a literal
@@marker. Lines 103-105 are outside the table that ends earlier in the document. Remove the marker and place both rows with the other Page map entries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@design/README.md` around lines 102 - 105, Remove the literal @@ marker and move the workflow-contract.html and review-coverage.html rows into the existing Page map table, preserving their descriptions and table formatting.design/review-coverage.html-3-3 (1)
3-3: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSet the document language to match the fixture content.
Line 3 declares Simplified Chinese, but the main headings and descriptions are English. Screen readers can select the wrong pronunciation rules. Set
lang="en"or translate the document content consistently.Proposed fix
-<html lang="zh-CN" data-theme="light"> +<html lang="en" data-theme="light">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@design/review-coverage.html` at line 3, Update the document language declaration on the root html element to lang="en" so it matches the existing English headings and descriptions, leaving the fixture content unchanged.orchestrator/internal/store/memory.go-632-634 (1)
632-634: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winBoth stores validate a trimmed revision pair but persist the raw value. The shared root cause is that
strings.TrimSpaceis applied only inside theValidCommitSHAargument, so a paddedPRHeadSHAorPRBaseSHApasses creation and is stored with whitespace.SetReviewPlanthen compares the plan revisions against the stored values and returnsErrConflict, so the review Run can never accept a plan.
orchestrator/internal/store/memory.go#L632-L634: assign the trimmed values tor.PRHeadSHAandr.PRBaseSHAbefore validating them.orchestrator/internal/store/pg.go#L585-L587: apply the same normalization increateRunTxbefore the validation and theINSERT.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/store/memory.go` around lines 632 - 634, Normalize and persist trimmed review revision values before validation: in orchestrator/internal/store/memory.go lines 632-634, update the review Run fields r.PRHeadSHA and r.PRBaseSHA before calling domain.ValidCommitSHA; apply the same change in orchestrator/internal/store/pg.go lines 585-587 within createRunTx so validation and the INSERT use the normalized values.orchestrator/internal/store/pg.go-703-720 (1)
703-720: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReturn the serialization error instead of writing SQL NULL.
Both helpers map a marshal failure to
nil, which persists NULL. The Run then looks like a legacy Run with no contract and no plan, sohandleIngestReviewstops requiring structured output and an accepted plan (orchestrator/internal/api/source.goLines 368-387). A serialization failure would therefore downgrade the determinism guarantee without any error. Propagate the error socreateRunTxaborts.🔧 Proposed error propagation
-func workflowContractJSON(contract *domain.WorkflowContract) any { +func workflowContractJSON(contract *domain.WorkflowContract) (any, error) { if contract == nil { - return nil + return nil, nil } encoded, err := json.Marshal(contract) if err != nil { - return nil + return nil, fmt.Errorf("encode execution contract: %w", err) } - return encoded + return encoded, nil } -func reviewPlanJSON(plan *domain.ReviewPlan) any { +func reviewPlanJSON(plan *domain.ReviewPlan) (any, error) { encoded, err := domain.MarshalStoredReviewPlan(plan) - if err != nil || len(encoded) == 0 { - return nil + if err != nil { + return nil, fmt.Errorf("encode review plan: %w", err) } - return encoded + if len(encoded) == 0 { + return nil, nil + } + return encoded, nil }Resolve both values before the
INSERTand return early on error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/store/pg.go` around lines 703 - 720, Update workflowContractJSON and reviewPlanJSON to return serialization errors instead of converting failures to nil/SQL NULL, and adjust createRunTx to resolve both values before the INSERT, returning immediately if either serialization fails so the transaction aborts. Preserve nil handling for genuinely absent optional values.orchestrator/internal/api/source.go-312-320 (1)
312-320: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClassify the oversize case with a sentinel error, not a substring match.
strings.Contains(err.Error(), "review_input_too_large")ties the HTTP status to the exact wording of a domain error message. Any rewording of that message silently changes the response from 413 to 400, and no test in this package would catch it. Export a sentinel fromdomainand useerrors.Is.🔧 Proposed sentinel check
plan, err := domain.BuildReviewPlan(input) if err != nil { - if strings.Contains(err.Error(), "review_input_too_large") { + if errors.Is(err, domain.ErrReviewInputTooLarge) { writeError(w, http.StatusRequestEntityTooLarge, "review_input_too_large", err.Error()) } else { writeError(w, http.StatusBadRequest, "invalid_review_plan", err.Error()) } return }Define
ErrReviewInputTooLargeinorchestrator/internal/domain/review_plan.goand wrap it in every cap violation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/api/source.go` around lines 312 - 320, Replace the string-based check in the review-plan error handling with errors.Is against a domain.ErrReviewInputTooLarge sentinel. Define and export that sentinel in domain/review_plan.go, wrap it for every review-input cap violation while preserving the original error context, and keep the 413 response for matching errors and 400 for other failures.orchestrator/internal/domain/workflow_contract.go-312-329 (1)
312-329: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMove run-identity fields out of the execution-contract hash input.
ResolveWorkflowContracthashesWorkflowContract, which includesTrigger.Triggeralways hasRequestID = run.ID, andIdempotencyKeyfalls back torun.IDwhen the run-specific keys are empty. Keep run identity in audit fields and hash only the shared frozen configuration if thisHashis meant to identify that configuration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/domain/workflow_contract.go` around lines 312 - 329, Update ResolveWorkflowContract and the WorkflowContract hash input so run-specific Trigger fields such as RequestID and the run.ID fallback for IdempotencyKey are excluded from the hashed execution contract. Preserve these values in the returned trigger and audit data, while hashing only the shared frozen configuration used to identify the contract.orchestrator/internal/api/plugin_webhook_test.go-480-490 (1)
480-490: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake the lookup counter race-free.
lookupCalls++runs on thehttptestserver goroutine. Line 620 reads it on the test goroutine. Useatomic.Int64sogo test -racecannot report a data race on this counter.🔒 Proposed atomic counter
- lookupCalls := 0 + var lookupCalls atomic.Int64 upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/merge_requests/7") { - lookupCalls++ + lookupCalls.Add(1)Then compare with
lookupCalls.Load() != 1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/api/plugin_webhook_test.go` around lines 480 - 490, Replace the plain lookupCalls integer in the httptest server handler with an atomic.Int64, increment it atomically inside the request handler, and use lookupCalls.Load() when asserting the expected count on the test goroutine.
🧹 Nitpick comments (14)
orchestrator/internal/store/plugins_test.go (1)
684-687: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that the rejected claim created no snapshots.
The test checks only the Run fields. Also require
ListRunPluginSnapshots(ctx, "r")to return an empty result. This protects the atomic dispatch contract against a partial snapshot write.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/store/plugins_test.go` around lines 684 - 687, Extend the rejected-claim assertions in the test around GetRun to call ListRunPluginSnapshots(ctx, "r") and verify it returns no snapshots, while preserving the existing Run status and TokenHash checks. Fail the test if either the Run mutation or any partial snapshot creation occurs.runner/test-integration.sh (1)
229-229: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMatch
JCODE_TASK_in the Git diff file header.Both tests can pass when the prefix occurs only in changed content.
runner/test-integration.sh#L229-L229: match^+++ b/JCODE_TASK_inART_CONTENT.runner/test.sh#L121-L124: match^+++ b/JCODE_TASK_in$OUT/diff.patch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@runner/test-integration.sh` at line 229, Update the artifact diff assertion in runner/test-integration.sh at lines 229-229 to match the Git diff file header pattern ^+++ b/JCODE_TASK_ rather than any occurrence in changed content; apply the same header-anchored match to the diff assertion in runner/test.sh at lines 121-124 against $OUT/diff.patch.runner/test-fetch-source.sh (1)
92-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate the captured review-plan payload.
The mock accepts any non-empty request body. Parse
received.review-planand require the expectedbase_sha,head_sha,merge_base_sha, and a non-emptydiff. This test must reject a plan with incorrect frozen revisions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@runner/test-fetch-source.sh` around lines 92 - 95, Update the mock handling for /review-plan near the received.review-plan write to parse the captured JSON payload and validate base_sha, head_sha, and merge_base_sha against the expected frozen revisions, while requiring diff to be non-empty. Make the test fail when any revision is incorrect or the diff is missing, rather than accepting any non-empty body.orchestrator/internal/reconciler/session.go (1)
59-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider checking the delivery target here too.
shouldOpenPRindecision.gorequires bothDelivers(domain.OutputCreatePullRequest)andDeliveryTarget(...) == "service_repository".sessionPushEligiblechecks only the output type. A contract that targetstrigger_prtherefore passes this gate, andpushSessionRunwould open a new PR on the frozen repository for it. Session runs do not resolvetrigger_prtoday, so this is not currently reachable. Add the target check so the three delivery gates stay identical.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/reconciler/session.go` around lines 59 - 63, Update sessionPushEligible to require both Delivers(domain.OutputCreatePullRequest) and DeliveryTarget(...) == "service_repository", matching shouldOpenPR in decision.go; preserve the existing historical-run fallback when ExecutionContract is nil.orchestrator/internal/reconciler/pr_test.go (1)
844-866: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the target-mismatch cases.
The test covers the output-type branch of each gate. It does not cover the target branch. Add one case where a
create_pull_requestcontract targetstrigger_prandshouldOpenPRmust return false, and one where it targetsservice_repositoryandshouldUpdatePushmust return false. Those two assertions are what pin the target checks added indecision.goat Lines 179 and 219.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/reconciler/pr_test.go` around lines 844 - 866, Extend TestDeliveryGatesUseFrozenContractAfterServiceModeChanges with target-mismatch assertions: verify shouldOpenPR returns false for a create_pull_request output targeting trigger_pr, and verify shouldUpdatePush returns false for a create_pull_request output targeting service_repository. Keep the existing output-type and frozen-contract coverage unchanged.orchestrator/internal/reconciler/reconciler.go (2)
1077-1097: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the frozen-grant selection into one helper.
scmContextForRunselects the first snapshot withHasFrozenRepositoryGrant().addGitEnvat Lines 2489-2496 repeats the same selection. If the selection rule changes (for example, preferring the snapshot bound to the Service), the two sites can diverge and send the pod to a different repository than the delivery path uses. Extract a single helper such asfrozenRepositoryGrant(ctx, runID) (*domain.RunPluginSnapshot, error)and call it from both.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/reconciler/reconciler.go` around lines 1077 - 1097, Extract the frozen-grant snapshot selection currently in scmContextForRun into a shared frozenRepositoryGrant helper, accepting ctx and runID and returning the selected *domain.RunPluginSnapshot or an error. Update both scmContextForRun and addGitEnv to use this helper, preserving the existing first-HasFrozenRepositoryGrant selection behavior while ensuring both paths use the same snapshot.
717-732: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsolidate the repeated "resolve frozen SCM context, park on credential error, split repository path" block. Five delivery passes now open with the same three steps: call
scmContextForRun, classifycredentials.ErrIntegrationCredentialandcredentials.ErrPluginCredentialUnavailablethroughnoteIntegrationCredentialFailure, then callprovider.SplitRepo. The shared root cause is a missing helper, so each new consumer must re-implement the credential classification correctly. A pass that forgets one sentinel would retry a permanent configuration failure on every tick instead of parking it. Add one helper that returns the context plus resolved owner/repo and a "handled" flag, then call it from each site.
orchestrator/internal/reconciler/reconciler.go#L717-L732: replace the resolve, park, andSplitRepoblock inopenPullRequestwith the shared helper, passing the pass label"pull request delivery".orchestrator/internal/reconciler/reconciler.go#L913-L922: replace the resolve and park block inupdatePushRunwith the helper, passing"update push".orchestrator/internal/reconciler/reconciler.go#L1015-L1029: replace the resolve, park, andSplitRepoblock inpostReviewwith the helper, passing"review post".orchestrator/internal/reconciler/session.go#L79-L94: replace the resolve, park, andSplitRepoblock inpushSessionRunwith the helper, passing"session push".orchestrator/internal/reconciler/session.go#L258-L271: replace the resolve, park, andSplitRepoblock inreconcileSessionPRReadywith the helper, passing"mark PR ready", and keepcontinueas the loop-local control flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/reconciler/reconciler.go` around lines 717 - 732, The repeated SCM resolution, credential-error handling, and repository splitting should be centralized in one helper returning the SCM context, owner/repo, and a handled flag. Replace the duplicated blocks in orchestrator/internal/reconciler/reconciler.go lines 717-732, 913-922, and 1015-1029 with calls using labels “pull request delivery”, “update push”, and “review post”; replace the corresponding blocks in orchestrator/internal/reconciler/session.go lines 79-94 and 258-271 with calls using “session push” and “mark PR ready”, preserving continue for the loop-local control flow at the latter site.orchestrator/internal/reconciler/decision.go (1)
177-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse named constants for PR delivery targets.
DeliveryTargetreturns hard-coded strings, andshouldOpenPR/shouldUpdatePushcompare the same strings again. A typo in either place can silently disable contract-backed PR delivery. Export target constants fromdomainand use them in the contract resolver and reconciler checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/reconciler/decision.go` around lines 177 - 187, Define exported constants in domain for the supported PR delivery targets, then update the contract resolver and the shouldOpenPR/shouldUpdatePush checks to compare against those constants instead of hard-coded strings such as "service_repository". Preserve the existing target values and behavior while ensuring all contract-backed PR delivery paths reuse the named symbols.docs/31-workflow-case-conversations-and-video-agent-poc.md (1)
77-83: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEncode the exact Hailuo-2.3 parameter matrix in tests.
The MVP requires validation but does not state the matrix. Current official documentation lists 6 seconds with
768Por1080P, and 10 seconds with768Ponly forMiniMax-Hailuo-2.3. Put this matrix in provider contract tests and reject all other combinations. (platform.minimaxi.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/31-workflow-case-conversations-and-video-agent-poc.md` around lines 77 - 83, Update the provider contract tests for MiniMax-Hailuo-2.3 to explicitly allow only 6 seconds with 768P or 1080P, and 10 seconds with 768P. Add rejection cases for every other duration and resolution combination, ensuring the validation matrix is enforced for both supported generation modes.orchestrator/internal/api/api_test.go (1)
339-374: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that retry preserves
PRHeadSHA/PRBaseSHA.The fixture at Line 354 now sets
PRHeadSHAandPRBaseSHA, but the retry assertions at Line 371-373 check onlyPRHeadBranch/PRBaseBranch. Add assertions for the SHA fields so a regression that drops or resets the frozen revision pair on retry is caught here.♻️ Proposed fix
if retry.PRHeadBranch != "jcode/run-abc12345" || retry.PRBaseBranch != "main" { t.Fatalf("retry PR assoc = %q..%q, want preserved", retry.PRBaseBranch, retry.PRHeadBranch) } + if retry.PRHeadSHA != rev.PRHeadSHA || retry.PRBaseSHA != rev.PRBaseSHA { + t.Fatalf("retry revision pair = %q..%q, want preserved", retry.PRBaseSHA, retry.PRHeadSHA) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/api/api_test.go` around lines 339 - 374, Add assertions to TestRetryPreservesReviewIdentity verifying retry.PRHeadSHA and retry.PRBaseSHA equal the seeded 40-character SHA values. Keep the existing branch identity assertions and ensure both frozen revision fields are checked for preservation.orchestrator/internal/domain/review_plan.go (1)
262-321: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse named fields for
storedReviewPlanconversions.
stored()(Line 282) andUnmarshalStoredReviewPlan(Line 312) build 16-field structs with positional literals. Several fields share the same type:BaseSHA,HeadSHA, andMergeBaseSHAare allstring;ChangedFiles,EligibleFiles,IndexedFiles,ChangedHunks,IndexedHunks, andChangedLinesare allint. If a future change reorders fields in eitherReviewPlanorstoredReviewPlan, the compiler accepts the mismatch silently. The result is a corruptedplan_hashor a swapped counter, without a build error.Use named-field struct literals in both conversions to make the mapping compiler-checked.
♻️ Proposed fix
func (p ReviewPlan) stored() storedReviewPlan { - return storedReviewPlan{p.SchemaVersion, p.PlanHash, p.BaseSHA, p.HeadSHA, p.MergeBaseSHA, p.RulesRevision, p.Coverage, p.ChangedFiles, p.EligibleFiles, p.IndexedFiles, p.ChangedHunks, p.IndexedHunks, p.ChangedLines, p.Files, p.Anchors, p.CreatedAt} + return storedReviewPlan{ + SchemaVersion: p.SchemaVersion, PlanHash: p.PlanHash, BaseSHA: p.BaseSHA, HeadSHA: p.HeadSHA, + MergeBaseSHA: p.MergeBaseSHA, RulesRevision: p.RulesRevision, Coverage: p.Coverage, + ChangedFiles: p.ChangedFiles, EligibleFiles: p.EligibleFiles, IndexedFiles: p.IndexedFiles, + ChangedHunks: p.ChangedHunks, IndexedHunks: p.IndexedHunks, ChangedLines: p.ChangedLines, + Files: p.Files, Anchors: p.Anchors, CreatedAt: p.CreatedAt, + } } @@ - p := &ReviewPlan{value.SchemaVersion, value.PlanHash, value.BaseSHA, value.HeadSHA, value.MergeBaseSHA, value.RulesRevision, value.Coverage, value.ChangedFiles, value.EligibleFiles, value.IndexedFiles, value.ChangedHunks, value.IndexedHunks, value.ChangedLines, value.Files, value.Anchors, value.CreatedAt} + p := &ReviewPlan{ + SchemaVersion: value.SchemaVersion, PlanHash: value.PlanHash, BaseSHA: value.BaseSHA, HeadSHA: value.HeadSHA, + MergeBaseSHA: value.MergeBaseSHA, RulesRevision: value.RulesRevision, Coverage: value.Coverage, + ChangedFiles: value.ChangedFiles, EligibleFiles: value.EligibleFiles, IndexedFiles: value.IndexedFiles, + ChangedHunks: value.ChangedHunks, IndexedHunks: value.IndexedHunks, ChangedLines: value.ChangedLines, + Files: value.Files, Anchors: value.Anchors, CreatedAt: value.CreatedAt, + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/domain/review_plan.go` around lines 262 - 321, Replace the positional struct literals in ReviewPlan.stored and UnmarshalStoredReviewPlan with named-field literals. Explicitly map every storedReviewPlan and ReviewPlan field, including the SHA values, counters, Files, Anchors, and CreatedAt, while preserving the existing values and hash validation behavior.orchestrator/internal/store/workflow_contract_test.go (1)
67-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the review-plan status gate.
The test covers first write, identical retry, and hash conflict. It does not cover the
ErrInvalidTransitiongate for a Run that is queued, terminal, or of kindagent(orchestrator/internal/store/memory.goLines 1894-1896). That gate is the only guard that stops a plan write outside the scheduling/running window. Add one sub-case that assertsErrInvalidTransitionfor a queued review Run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/store/workflow_contract_test.go` around lines 67 - 92, The TestMemStoreSetReviewPlanFirstWriteIdempotency test should also cover the review-plan status gate. Add a sub-case creating a queued review Run, attempt SetReviewPlan for it, and assert the returned error is ErrInvalidTransition while preserving the existing first-write, retry, and conflict assertions.orchestrator/internal/api/plugin_webhook_test.go (1)
350-376: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the frozen Gitea revision pair.
The payload now supplies
head.shaandbase.sha, but the assertion checks only branches and PR number. The GitLab test assertsPRHeadSHAandPRBaseSHA(Lines 617-619). Add the same assertion here so the Gitea path keeps proving that the exact revisions are frozen before the Run is queued.💚 Proposed assertion
if runs[0].Kind != domain.RunKindReview || runs[0].PRHeadBranch != "feature" || runs[0].PRBaseBranch != "main" || runs[0].PRNumber != 7 { t.Fatalf("review run=%+v", runs[0]) } + if runs[0].PRHeadSHA != strings.Repeat("a", 40) || runs[0].PRBaseSHA != strings.Repeat("b", 40) { + t.Fatalf("review revisions were not frozen: %+v", runs[0]) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/api/plugin_webhook_test.go` around lines 350 - 376, Extend the final validation in the Gitea webhook test around the existing run metadata assertion to also compare PRHeadSHA with the supplied 40-character “a” revision and PRBaseSHA with the supplied 40-character “b” revision. Keep the existing branch and PR number checks unchanged so the test verifies the complete frozen revision pair before queueing.orchestrator/internal/api/plugin_webhook.go (1)
307-363: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated PR-revision-fetch-and-validate logic.
The manual-review block (lines 307-363) and the automatic-enrichment block (364-393) both: resolve a provider client, split or take an owner/repo pair, call
PRByNumber, check thatHeadRef/BaseRef/HeadSHA/BaseSHAare all non-empty, and assign the same foureventfields on success. Extract this into one helper function that takes a client, owner, repo, and PR number, and returns the validated*provider.PRor an error. Call it from both sites.This reduces the risk that a future change to the revision contract (for example, adding a required field) gets applied to only one of the two call sites.
♻️ Proposed extraction
+func fetchCompletePR(ctx context.Context, client gitprovider.Provider, owner, repo string, number int) (*provider.PR, error) { + pr, err := client.PRByNumber(ctx, owner, repo, number) + if err != nil || pr == nil || pr.HeadRef == "" || pr.BaseRef == "" || pr.HeadSHA == "" || pr.BaseSHA == "" { + if err == nil { + err = errors.New("pull request revision pair is incomplete") + } + return nil, err + } + return pr, nil +}Also applies to: 364-393
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator/internal/api/plugin_webhook.go` around lines 307 - 363, Extract the shared pull-request revision lookup and validation from the manual-review flow and the automatic-enrichment flow into one helper that accepts a gitprovider.Provider, owner, repository, and PR number, then returns a validated *provider.PR or an error. Use the helper at both call sites, preserving the required non-empty HeadRef, BaseRef, HeadSHA, and BaseSHA checks and the existing event-field assignments and error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/11-api.md`:
- Around line 1292-1310: Normalize the malformed review-plan error contract: in
docs/11-api.md lines 1292-1310, document the endpoint’s 400 malformed-input
response with the stable error code; in
docs/29-workflow-contract-and-review-design.md lines 434-448, replace
invalid_review_plan with the failure-table identifier and update the related
tests to assert that code consistently.
In `@docs/28-workflow-contract-and-deterministic-review-prd.md`:
- Around line 66-70: Clarify the timeout contract in the PRD by explicitly
defining whether execution.timeout_seconds represents the per-run RUN_TIMEOUT
budget or the Kubernetes activeDeadlineSeconds deadline. Document timeoutGrace
separately, specify the conversion between the two values, and require the
inspector, runner, scheduler, and JobSpec.TimeoutSeconds handling to use the
same effective deadline.
In `@docs/29-workflow-contract-and-review-design.md`:
- Around line 276-285: The Review Plan contract must require canonical full
40-character hexadecimal Git object IDs rather than abbreviated SHAs. Update the
schema example and surrounding documentation in
docs/29-workflow-contract-and-review-design.md (lines 276-285), and revise the
audit hardening wording in outputs/workflow-platform-audit-2026-08-01.md (lines
62-64) to match; ensure queue creation either canonicalizes IDs before
persistence or rejects values shorter than 40 hex characters.
In `@docs/30-open-source-agent-workflow-benchmark.md`:
- Around line 170-177: Update the PR Review row in the “对四个目标场景的落地” table to
include ripgrep alongside the existing required capabilities, keeping the row’s
other fields unchanged.
In `@orchestrator/internal/domain/workflow_contract.go`:
- Around line 375-411: Update Validate after resolving the built-in definition
in the switch to require c.Requirements to exactly match def.Requirements,
rejecting both missing and extra requirements; retain the existing whitelist
validation only as applicable, and perform this comparison before validating the
definition hash.
In `@orchestrator/internal/reconciler/reconciler.go`:
- Around line 2489-2496: Update addGitEnv and jobEnv to propagate
ListRunPluginSnapshots errors instead of silently retaining mutable Service
values; ensure createJob handles the propagated error by leaving the run queued
for retry, or reuse the snapshots already loaded for ClaimRunDispatch and pass
the resolved grant into addGitEnv.
In `@orchestrator/internal/store/pg.go`:
- Around line 21-23: Change PGStore’s workflowRunTimeoutSecs and
workflowSessionTTLSecs fields to atomic.Int64, then update
ConfigureWorkflowTimeoutDefaults to store values atomically and createRunTx to
load them atomically, following the existing MemStore pattern.
- Around line 505-523: Update scanRun’s execution-contract handling to decode
without DisallowUnknownFields and treat validation failures as a skipped
ExecutionContract projection rather than returning a row-scan error. Preserve
errors for malformed contract data, and report skipped validation failures
through the existing logging or counter path with the Run id so ListRuns and
ListRunsByService continue loading other rows.
---
Outside diff comments:
In `@orchestrator/internal/api/review.go`:
- Around line 69-91: Remove the PR revision field assignments from newReviewRun,
including PRHeadBranch, PRBaseBranch, and PRHeadSHA; do not add PRBaseSHA there.
Keep all four revision fields assigned only in handleRequestReview after
reviewTargetAtCreation succeeds, preserving the frozen live PR revisions.
---
Minor comments:
In `@design/README.md`:
- Around line 102-105: Remove the literal @@ marker and move the
workflow-contract.html and review-coverage.html rows into the existing Page map
table, preserving their descriptions and table formatting.
In `@design/review-coverage.html`:
- Line 3: Update the document language declaration on the root html element to
lang="en" so it matches the existing English headings and descriptions, leaving
the fixture content unchanged.
In `@docs/29-workflow-contract-and-review-design.md`:
- Around line 75-79: Update the migration reference in the Data design section
to use 0064_workflow_contract_review_plan.sql, matching the reviewed migration
filename so operators and reviewers can locate it.
In `@docs/30-open-source-agent-workflow-benchmark.md`:
- Around line 60-61: Update the timeout conclusion in the benchmark document to
distinguish the observed 30-minute server maximum from the POC requirement.
State that the listed POC scenarios require a higher maximum based on their
45–180 minute budgets, rather than claiming 30 minutes covers them.
In `@docs/31-workflow-case-conversations-and-video-agent-poc.md`:
- Around line 61-66: Update the “API 名称校正” section to remove the unsupported
claim that /v1/video_template_generation is officially deprecated. Describe the
template create/query flow as current, and retain the provider compatibility
seam only if justified as a product-risk decision; if retained, cite the
official deprecation source and cutoff.
In `@orchestrator/internal/api/plugin_webhook_test.go`:
- Around line 480-490: Replace the plain lookupCalls integer in the httptest
server handler with an atomic.Int64, increment it atomically inside the request
handler, and use lookupCalls.Load() when asserting the expected count on the
test goroutine.
In `@orchestrator/internal/api/source.go`:
- Around line 312-320: Replace the string-based check in the review-plan error
handling with errors.Is against a domain.ErrReviewInputTooLarge sentinel. Define
and export that sentinel in domain/review_plan.go, wrap it for every
review-input cap violation while preserving the original error context, and keep
the 413 response for matching errors and 400 for other failures.
In `@orchestrator/internal/domain/workflow_contract.go`:
- Around line 312-329: Update ResolveWorkflowContract and the WorkflowContract
hash input so run-specific Trigger fields such as RequestID and the run.ID
fallback for IdempotencyKey are excluded from the hashed execution contract.
Preserve these values in the returned trigger and audit data, while hashing only
the shared frozen configuration used to identify the contract.
In `@orchestrator/internal/reconciler/pr_test.go`:
- Around line 130-131: In the test setup around the EncryptString calls and
GetPluginInstallation lookup, stop discarding returned errors and fail
immediately with t.Fatal, including each underlying error. Preserve the existing
live lookup and candidate logic, renaming the later candidates, err binding if
needed to avoid shadowing after introducing the checked error variable.
In `@orchestrator/internal/store/memory.go`:
- Around line 632-634: Normalize and persist trimmed review revision values
before validation: in orchestrator/internal/store/memory.go lines 632-634,
update the review Run fields r.PRHeadSHA and r.PRBaseSHA before calling
domain.ValidCommitSHA; apply the same change in
orchestrator/internal/store/pg.go lines 585-587 within createRunTx so validation
and the INSERT use the normalized values.
In `@orchestrator/internal/store/pg.go`:
- Around line 703-720: Update workflowContractJSON and reviewPlanJSON to return
serialization errors instead of converting failures to nil/SQL NULL, and adjust
createRunTx to resolve both values before the INSERT, returning immediately if
either serialization fails so the transaction aborts. Preserve nil handling for
genuinely absent optional values.
In `@orchestrator/internal/store/plugins_test.go`:
- Around line 662-665: Update the test fixture setup around CreateProject,
CreateService, UpsertProviderConfig, and GetProviderConfig to capture each
returned error and call t.Fatal when any operation fails. Ensure
ClaimRunDispatch runs only after all project, service, provider configuration,
and lookup setup succeeds.
In `@runner/entrypoint.sh`:
- Around line 547-554: The review-plan upload path should distinguish oversized
diffs from generic upload failures. Use the existing REVIEW_DIFF_BYTES value
after computing the diff and, when it exceeds MaxReviewDiffBytes (2 << 20), fail
with setup_failed and the review_input_too_large reason; otherwise preserve the
existing post-review-plan upload handling and include any server-provided
failure reason when available.
---
Nitpick comments:
In `@docs/31-workflow-case-conversations-and-video-agent-poc.md`:
- Around line 77-83: Update the provider contract tests for MiniMax-Hailuo-2.3
to explicitly allow only 6 seconds with 768P or 1080P, and 10 seconds with 768P.
Add rejection cases for every other duration and resolution combination,
ensuring the validation matrix is enforced for both supported generation modes.
In `@orchestrator/internal/api/api_test.go`:
- Around line 339-374: Add assertions to TestRetryPreservesReviewIdentity
verifying retry.PRHeadSHA and retry.PRBaseSHA equal the seeded 40-character SHA
values. Keep the existing branch identity assertions and ensure both frozen
revision fields are checked for preservation.
In `@orchestrator/internal/api/plugin_webhook_test.go`:
- Around line 350-376: Extend the final validation in the Gitea webhook test
around the existing run metadata assertion to also compare PRHeadSHA with the
supplied 40-character “a” revision and PRBaseSHA with the supplied 40-character
“b” revision. Keep the existing branch and PR number checks unchanged so the
test verifies the complete frozen revision pair before queueing.
In `@orchestrator/internal/api/plugin_webhook.go`:
- Around line 307-363: Extract the shared pull-request revision lookup and
validation from the manual-review flow and the automatic-enrichment flow into
one helper that accepts a gitprovider.Provider, owner, repository, and PR
number, then returns a validated *provider.PR or an error. Use the helper at
both call sites, preserving the required non-empty HeadRef, BaseRef, HeadSHA,
and BaseSHA checks and the existing event-field assignments and error handling.
In `@orchestrator/internal/domain/review_plan.go`:
- Around line 262-321: Replace the positional struct literals in
ReviewPlan.stored and UnmarshalStoredReviewPlan with named-field literals.
Explicitly map every storedReviewPlan and ReviewPlan field, including the SHA
values, counters, Files, Anchors, and CreatedAt, while preserving the existing
values and hash validation behavior.
In `@orchestrator/internal/reconciler/decision.go`:
- Around line 177-187: Define exported constants in domain for the supported PR
delivery targets, then update the contract resolver and the
shouldOpenPR/shouldUpdatePush checks to compare against those constants instead
of hard-coded strings such as "service_repository". Preserve the existing target
values and behavior while ensuring all contract-backed PR delivery paths reuse
the named symbols.
In `@orchestrator/internal/reconciler/pr_test.go`:
- Around line 844-866: Extend
TestDeliveryGatesUseFrozenContractAfterServiceModeChanges with target-mismatch
assertions: verify shouldOpenPR returns false for a create_pull_request output
targeting trigger_pr, and verify shouldUpdatePush returns false for a
create_pull_request output targeting service_repository. Keep the existing
output-type and frozen-contract coverage unchanged.
In `@orchestrator/internal/reconciler/reconciler.go`:
- Around line 1077-1097: Extract the frozen-grant snapshot selection currently
in scmContextForRun into a shared frozenRepositoryGrant helper, accepting ctx
and runID and returning the selected *domain.RunPluginSnapshot or an error.
Update both scmContextForRun and addGitEnv to use this helper, preserving the
existing first-HasFrozenRepositoryGrant selection behavior while ensuring both
paths use the same snapshot.
- Around line 717-732: The repeated SCM resolution, credential-error handling,
and repository splitting should be centralized in one helper returning the SCM
context, owner/repo, and a handled flag. Replace the duplicated blocks in
orchestrator/internal/reconciler/reconciler.go lines 717-732, 913-922, and
1015-1029 with calls using labels “pull request delivery”, “update push”, and
“review post”; replace the corresponding blocks in
orchestrator/internal/reconciler/session.go lines 79-94 and 258-271 with calls
using “session push” and “mark PR ready”, preserving continue for the loop-local
control flow at the latter site.
In `@orchestrator/internal/reconciler/session.go`:
- Around line 59-63: Update sessionPushEligible to require both
Delivers(domain.OutputCreatePullRequest) and DeliveryTarget(...) ==
"service_repository", matching shouldOpenPR in decision.go; preserve the
existing historical-run fallback when ExecutionContract is nil.
In `@orchestrator/internal/store/plugins_test.go`:
- Around line 684-687: Extend the rejected-claim assertions in the test around
GetRun to call ListRunPluginSnapshots(ctx, "r") and verify it returns no
snapshots, while preserving the existing Run status and TokenHash checks. Fail
the test if either the Run mutation or any partial snapshot creation occurs.
In `@orchestrator/internal/store/workflow_contract_test.go`:
- Around line 67-92: The TestMemStoreSetReviewPlanFirstWriteIdempotency test
should also cover the review-plan status gate. Add a sub-case creating a queued
review Run, attempt SetReviewPlan for it, and assert the returned error is
ErrInvalidTransition while preserving the existing first-write, retry, and
conflict assertions.
In `@runner/test-fetch-source.sh`:
- Around line 92-95: Update the mock handling for /review-plan near the
received.review-plan write to parse the captured JSON payload and validate
base_sha, head_sha, and merge_base_sha against the expected frozen revisions,
while requiring diff to be non-empty. Make the test fail when any revision is
incorrect or the diff is missing, rather than accepting any non-empty body.
In `@runner/test-integration.sh`:
- Line 229: Update the artifact diff assertion in runner/test-integration.sh at
lines 229-229 to match the Git diff file header pattern ^+++ b/JCODE_TASK_
rather than any occurrence in changed content; apply the same header-anchored
match to the diff assertion in runner/test.sh at lines 121-124 against
$OUT/diff.patch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ffcb284-1283-47ed-bea1-78473b80f258
📒 Files selected for processing (70)
CONTEXT.mdconsole/src/api/types.tsconsole/src/i18n/locales/en.tsconsole/src/i18n/locales/zh-Hans.tsconsole/src/pages/RunDetailPage.module.cssconsole/src/pages/RunDetailPage.test.tsxconsole/src/pages/RunDetailPage.tsxdesign/README.mddesign/assets/workflow-contract.cssdesign/assets/workflow-contract.jsdesign/review-coverage.htmldesign/workflow-contract.htmldocs/11-api.mddocs/28-workflow-contract-and-deterministic-review-prd.mddocs/29-workflow-contract-and-review-design.mddocs/30-open-source-agent-workflow-benchmark.mddocs/31-workflow-case-conversations-and-video-agent-poc.mdorchestrator/cmd/orchestrator/main.goorchestrator/internal/api/api.goorchestrator/internal/api/api_test.goorchestrator/internal/api/automation_test.goorchestrator/internal/api/plugin_webhook.goorchestrator/internal/api/plugin_webhook_test.goorchestrator/internal/api/review.goorchestrator/internal/api/review_test.goorchestrator/internal/api/runs.goorchestrator/internal/api/runs_projection_test.goorchestrator/internal/api/source.goorchestrator/internal/api/source_review_test.goorchestrator/internal/api/webhook.goorchestrator/internal/domain/domain.goorchestrator/internal/domain/plugins.goorchestrator/internal/domain/review_plan.goorchestrator/internal/domain/review_plan_test.goorchestrator/internal/domain/workflow_contract.goorchestrator/internal/domain/workflow_contract_test.goorchestrator/internal/provider/fake.goorchestrator/internal/provider/gitea.goorchestrator/internal/provider/github.goorchestrator/internal/provider/gitlab.goorchestrator/internal/provider/provider.goorchestrator/internal/reconciler/decision.goorchestrator/internal/reconciler/guardrails_test.goorchestrator/internal/reconciler/permission_test.goorchestrator/internal/reconciler/pr_test.goorchestrator/internal/reconciler/reconciler.goorchestrator/internal/reconciler/session.goorchestrator/internal/reconciler/session_test.goorchestrator/internal/scmevent/event.goorchestrator/internal/scmevent/event_test.goorchestrator/internal/scmevent/normalize.goorchestrator/internal/store/automation_execution.goorchestrator/internal/store/memory.goorchestrator/internal/store/migrations/0064_workflow_contract_review_plan.sqlorchestrator/internal/store/pg.goorchestrator/internal/store/pg_workflow_contract_integration_test.goorchestrator/internal/store/plugin_kanban_occurrence.goorchestrator/internal/store/plugins.goorchestrator/internal/store/plugins_test.goorchestrator/internal/store/store.goorchestrator/internal/store/workflow_contract_test.gooutputs/workflow-platform-audit-2026-08-01.mdrunner/README.mdrunner/entrypoint.shrunner/mockllm/main.gorunner/orchclient/main.gorunner/test-fetch-source.shrunner/test-integration.shrunner/test-session-scrub-matrix.shrunner/test.sh
| ### 5.7 `POST /internal/v1/runs/{id}/review-plan` — 冻结确定性评审输入 | ||
|
|
||
| 仅 `kind=review` 且处于 `scheduling|running` 的 Run；请求体为单个严格 JSON 对象： | ||
|
|
||
| ```json | ||
| { | ||
| "base_sha": "1111111111111111111111111111111111111111", | ||
| "head_sha": "2222222222222222222222222222222222222222", | ||
| "merge_base_sha": "1111111111111111111111111111111111111111", | ||
| "diff": "diff --git a/main.go b/main.go\n..." | ||
| } | ||
| ``` | ||
|
|
||
| 服务端核对 Run 已冻结的 base/head，解析右侧新增行 anchor，丢弃 raw diff，只持久化 | ||
| canonical `review_plan`。首次写入 `201`，相同 plan 重试 `200`，不同 plan | ||
| `409 review_plan_conflict`，revision 不符 `409 review_revision_mismatch`，超过 | ||
| 2 MiB/400 files/2000 hunks 返回 `413 review_input_too_large`。公开 Run JSON 只返回 | ||
| coverage 与逐文件计数，不返回私有 anchor；随后结构化 Review Result 的每个 finding | ||
| 都必须命中该计划的 changed right-side line。 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Normalize the review-plan error contract.
The endpoint, failure model, and API documentation must use one stable error code for malformed review-plan input.
docs/11-api.md#L1292-L1310: document the400malformed-input response.docs/29-workflow-contract-and-review-design.md#L434-L448: aligninvalid_review_planwith the failure-table identifier and update the tests.
📍 Affects 2 files
docs/11-api.md#L1292-L1310(this comment)docs/29-workflow-contract-and-review-design.md#L434-L448
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/11-api.md` around lines 1292 - 1310, Normalize the malformed review-plan
error contract: in docs/11-api.md lines 1292-1310, document the endpoint’s 400
malformed-input response with the stable error code; in
docs/29-workflow-contract-and-review-design.md lines 434-448, replace
invalid_review_plan with the failure-table identifier and update the related
tests to assert that code consistently.
| Ship-R1 不交付 Runner Profile Catalog、Composer live readiness 或 Custom Profile | ||
| CRUD。合同中的 timeout 直接冻结当前 Project override 或 Cluster default 的实际 | ||
| effective value，必须与 Kubernetes `activeDeadlineSeconds` 一致。Ship-R2 引入 | ||
| operator-owned Runner manifest 和单一 evaluator，再检查 toolchain/capability 并在 | ||
| 创建前返回 typed blocker；任何阶段都不得静默换模型、换 profile、改 output 或降级 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define which timeout the contract freezes.
The PRD requires the contract timeout to match Kubernetes activeDeadlineSeconds. The reconciler tests distinguish RUN_TIMEOUT as the per-run budget from JobSpec.TimeoutSeconds as RUN_TIMEOUT + timeoutGrace. State whether execution.timeout_seconds stores the budget or the deadline, and model the grace separately so the inspector, runner, and scheduler use the same value.
🧰 Tools
🪛 LanguageTool
[grammar] ~69-~69: Ensure spelling is correct
Context: ...单一 evaluator，再检查 toolchain/capability 并在 创建前返回 typed blocker；任何阶段都不得静默换模型、换 profile、改 ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/28-workflow-contract-and-deterministic-review-prd.md` around lines 66 -
70, Clarify the timeout contract in the PRD by explicitly defining whether
execution.timeout_seconds represents the per-run RUN_TIMEOUT budget or the
Kubernetes activeDeadlineSeconds deadline. Document timeoutGrace separately,
specify the conversion between the two values, and require the inspector,
runner, scheduler, and JobSpec.TimeoutSeconds handling to use the same effective
deadline.
| ```json | ||
| { | ||
| "schema_version": 1, | ||
| "plan_hash": "sha256:…", | ||
| "rules_revision": "review-v2", | ||
| "base_ref": "main", | ||
| "head_ref": "feature/x", | ||
| "base_sha": "40hex", | ||
| "head_sha": "40hex", | ||
| "merge_base_sha": "40hex", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^docs/29-workflow-contract-and-review-design\.md$|^outputs/workflow-platform-audit-2026-08-01\.md$|REVIEW|workflow|PRReadyPolicy|contract)' || true
echo
echo "== target snippets =="
if [ -f docs/29-workflow-contract-and-review-design.md ]; then
sed -n '250,310p' docs/29-workflow-contract-and-review-design.md
fi
echo
if [ -f outputs/workflow-platform-audit-2026-08-01.md ]; then
sed -n '45,75p' outputs/workflow-platform-audit-2026-08-01.md
fi
echo
echo "== search SHA/validation references =="
rg -n "sha|SHA|40hex|abbreviated|abbreviate|longform|full .*id|object ID|commit ID|ready policy|PRReadyPolicy|REVIEW\.json|plan_hash|base_sha|head_sha|merge_base_sha" docs outputs -g '*.md' -g '*.json' -g '*.yaml' -g '*.yml' || true
echo
echo "== workflow/contract source files =="
git ls-files | rg -i 'review|workflow|contract|ready|policy|runner|platform|audit' | sed -n '1,200p'Repository: cnjack/cloud
Length of output: 25170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== orchestrator review/domain files outline =="
for f in orchestrator/internal/api/review.go orchestrator/internal/domain/review.go orchestrator/internal/domain/review_plan.go orchestrator/internal/domain/workflow_contract.go; do
echo "--- $f"
wc -l "$f"
ast-grep outline "$f" || true
done
echo
echo "== relevant orchestrator snippets =="
for f in orchestrator/internal/api/review.go orchestrator/internal/domain/review.go orchestrator/internal/domain/review_plan.go orchestrator/internal/domain/workflow_contract.go; do
echo "--- $f"
rg -n "SHA|sha|Commit|commit|plan_hash|base_sha|head_sha|merge_base_sha|Validate|ValidateAgainst|IsZero|String" "$f" || true
done
echo
echo "== migrations and tests relevant to SHA validation =="
for f in orchestrator/internal/store/migrations/0007_runner_contract.sql orchestrator/internal/store/migrations/0064_workflow_contract_review_plan.sql orchestrator/internal/store/workflow_contract_test.go orchestrator/internal/store/pg_workflow_contract_integration_test.go orchestrator/internal/domain/review_test.go orchestrator/internal/domain/review_plan_test.go; do
if [ -f "$f" ]; then
echo "--- $f"
rg -n "SHA|sha|Commit|commit|base_sha|head_sha|merge_base_sha|plan_hash|Validate|Invalid|regex|7-64|64" "$f" || true
echo
fi
doneRepository: cnjack/cloud
Length of output: 12945
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== review_plan.go SHA validation implementation =="
sed -n '53,214p' orchestrator/internal/domain/review_plan.go | cat -n
echo
echo "== behavior probe for ValidCommitSHA inputs =="
python3 - <<'PY'
import re
SHA_REGEX = re.compile(r'^[0-9a-fA-F]+$', re.ASCII)
def valid_commit_sha(value: str) -> bool:
value = value.strip()
return bool(SHA_REGEX.match(value)) and 7 <= len(value) <= 64
samples = [
"1111111",
"111111",
"1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcd",
"1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef",
"zzzzzzz",
"1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdeA",
"",
"1111111 ",
]
for s in samples:
print(f"{s!r}: length={len(s)} valid={valid_commit_sha(s)}")
PY
echo
echo "== source references to ValidCommitSHA and BuildReviewPlan =="
rg -n "ValidCommitSHA|BuildReviewPlan|review_revision_invalid|merge_base_sha|base_sha|head_sha" orchestrator runner docs outputs -g '*.go' -g '*.sh' -g '*.md' || trueRepository: cnjack/cloud
Length of output: 14830
Reject abbreviated commit SHA input for Review Plans.
docs/29-workflow-contract-and-review-design.md#L283-L285 shows canonical 40hex SHA placeholders, but orchestrator/internal/domain/review_plan.go#L208-L219 and the audit hardening line accept 7–64 hex strings. This contradicts freeze/replay requirements: queue creation should canonicalize full Git object IDs before persistence, or reject all values shorter than 40 hex. Correct the audit hardening wording to match the enforced canonical requirement.
📍 Affects 2 files
docs/29-workflow-contract-and-review-design.md#L276-L285(this comment)outputs/workflow-platform-audit-2026-08-01.md#L62-L64
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/29-workflow-contract-and-review-design.md` around lines 276 - 285, The
Review Plan contract must require canonical full 40-character hexadecimal Git
object IDs rather than abbreviated SHAs. Update the schema example and
surrounding documentation in docs/29-workflow-contract-and-review-design.md
(lines 276-285), and revise the audit hardening wording in
outputs/workflow-platform-audit-2026-08-01.md (lines 62-64) to match; ensure
queue creation either canonicalizes IDs before persistence or rejects values
shorter than 40 hex characters.
| ## 7. 对四个目标场景的落地 | ||
|
|
||
| | 场景 | Trigger adapter | Profile | Required capabilities | Typed outputs | 关键完成条件 | | ||
| | --- | --- | --- | --- | --- | --- | | ||
| | PR Review | SCM verified event | Reviewer | source.read、git、scm.review.write | provider_review | fixed SHA、coverage、valid anchors、idempotent publish | | ||
| | 竞品研究 → JType | Manual/Cron | Product Manager | web/MCP fetch、jtype.create-card | create_jtype_card、publish_report | sources、request schema、Card receipt | | ||
| | JType request → PR | JType receipt/claim | Developer | source.read/write、toolchain、scm.pr.write | create_pull_request | tests、commit、Ready PR、Card backlink | | ||
| | 周报/Changelog | Cron occurrence | Developer 或 PM | source.read、scm.read、optional write | publish_report/update_changelog | fixed time window、dedupe、report/PR receipt | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add ripgrep to the PR Review capability row.
docs/29-workflow-contract-and-review-design.md declares ripgrep as a Ship-R1 Reviewer requirement and requires rg during bootstrap. This table omits it. Add ripgrep or mark the table as non-authoritative.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/30-open-source-agent-workflow-benchmark.md` around lines 170 - 177,
Update the PR Review row in the “对四个目标场景的落地” table to include ripgrep alongside
the existing required capabilities, keeping the row’s other fields unchanged.
| for _, req := range c.Requirements { | ||
| if !slices.Contains(validWorkflowRequirements, req) { | ||
| return fmt.Errorf("unknown workflow requirement %q", req) | ||
| } | ||
| } | ||
| var def builtinWorkflowDefinition | ||
| switch c.Workflow.ID { | ||
| case BuiltinImplementationTaskID: | ||
| def = implementationDefinition | ||
| if c.Workflow.Revision != 1 || c.Profile.ID != BuiltinDeveloperProfileID || c.Execution.RunKind != RunKindAgent || c.Execution.WorkspaceAccess != WorkspaceReadWrite { | ||
| return errors.New("invalid implementation workflow mapping") | ||
| } | ||
| out := c.Delivery.Outputs[0] | ||
| if out.Type != OutputDiffOnly && out.Type != OutputCreatePullRequest { | ||
| return errors.New("implementation workflow has an invalid output") | ||
| } | ||
| if out.Type == OutputCreatePullRequest && !ValidPRReadyPolicy(out.ReadyPolicy) { | ||
| return errors.New("pull-request output requires ready_policy") | ||
| } | ||
| if out.Type == OutputDiffOnly && out.ReadyPolicy != "" { | ||
| return errors.New("diff output forbids ready_policy") | ||
| } | ||
| case BuiltinPullRequestReviewID: | ||
| def = reviewDefinition | ||
| out := c.Delivery.Outputs[0] | ||
| if c.Workflow.Revision != 1 || c.Profile.ID != BuiltinReviewerProfileID || c.Execution.RunKind != RunKindReview || c.Execution.WorkspaceAccess != WorkspaceReadOnly || out.Type != OutputProviderReview || out.ReadyPolicy != "" { | ||
| return errors.New("invalid review workflow mapping") | ||
| } | ||
| if c.Verification.Mode != "structured_review" || c.Verification.RulesRevision != "review-v2" { | ||
| return errors.New("invalid review verification contract") | ||
| } | ||
| default: | ||
| return errors.New("unknown built-in workflow") | ||
| } | ||
| if c.Workflow.DefinitionHash != hashJSON(def) { | ||
| return errors.New("workflow definition hash mismatch") | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether the run-creation API can decode a client-supplied execution contract.
fd -t f 'runs.go' orchestrator/internal/api --exec rg -n -C6 'Decode|createRunRequest|ExecutionContract'
rg -n -C4 --type=go 'ExecutionContract\s*[:=]' orchestrator/internal/apiRepository: cnjack/cloud
Length of output: 5154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files 'orchestrator/internal/api/*.go' 'orchestrator/internal/domain/*.go' 'orchestrator/internal/store/*.go' | sed -n '1,160p'
echo
echo "== runs.go outline/size/slices =="
wc -l orchestrator/internal/api/runs.go
rg -n -C8 'createRun|CreateRun|Run\{|struct|json:"|Decode|Unmarshal|execution_contract|workflow_contract|WorkflowContract' orchestrator/internal/api/runs.go orchestrator/internal/domain/workflow_contract.go
echo
echo "== domain workflow_contract relevant =="
sed -n '320,425p' orchestrator/internal/domain/workflow_contract.go
echo
echo "== store relevant =="
rg -n -C8 'ExecutionContract|Validate\(.*WorkflowContract|workflow_contract|CreateRunTx|CreateRun' orchestrator/internal/store/pg.go orchestrator/internal/store/memory.goRepository: cnjack/cloud
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== domain Run and CreateRun signatures =="
rg -n -C6 'type Run struct|ExecutionContract|SchemaVersion|func CreateRunTx|func CreateRun\b' orchestrator/internal/domain/orchestration.go orchestrator/internal/domain/domain.go orchestrator/internal/domain/workflow_contract.go orchestrator/internal/store/pg.go orchestrator/internal/store/memory.go
echo
echo "== api decode/util =="
rg -n -C8 'func decodeJSON|decodeJSON\(' orchestrator/internal/api
echo
echo "== run creation body =="
sed -n '65,180p' orchestrator/internal/api/runs.go
echo
echo "== retry and resume relevant =="
sed -n '465,516p' orchestrator/internal/api/runs.go
sed -n '580,670p' orchestrator/internal/api/runs.go
echo
echo "== decodeJSON implementations =="
rg -n -C8 'func decodeJSON' .Repository: cnjack/cloud
Length of output: 16940
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== decodeJSON implementations =="
rg -n -C8 'func decodeJSON' .
echo
echo "== run creation body =="
sed -n '65,180p' orchestrator/internal/api/runs.go
echo
echo "== retry and resume relevant =="
sed -n '465,516p' orchestrator/internal/api/runs.go
sed -n '580,670p' orchestrator/internal/api/runs.go
echo
echo "== decodeJSON implementations across API files =="
rg -n -C5 'decodeJSON\b' orchestrator/internal/apiRepository: cnjack/cloud
Length of output: 50368
Bind Requirements to the resolved built-in definition.
Validate accepts any whitelisted requirement and permits extra requirements. A self-consistent contract can pass with a create_pull_request output while omitting RequirementSCMPullRequestWrite, or with arbitrary additional requirements. Compare c.Requirements with the def.Requirements selected by the built-in workflow hash.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@orchestrator/internal/domain/workflow_contract.go` around lines 375 - 411,
Update Validate after resolving the built-in definition in the switch to require
c.Requirements to exactly match def.Requirements, rejecting both missing and
extra requirements; retain the existing whitelist validation only as applicable,
and perform this comparison before validating the definition hash.
| if snapshots, err := r.st.ListRunPluginSnapshots(ctx, run.ID); err == nil { | ||
| for i := range snapshots { | ||
| if snapshots[i].HasFrozenRepositoryGrant() { | ||
| defaultBranch, repoURL = snapshots[i].DefaultBranch, snapshots[i].CloneURL | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not silently fall back to mutable Service values when the snapshot read fails.
addGitEnv discards the error from ListRunPluginSnapshots. If that read fails transiently, defaultBranch and repoURL keep the mutable Service values, and the pod clones whatever the live binding points at. This is the drift this change prevents everywhere else: createJob at Lines 1562-1570 and scmContextForRun both fail or retry instead of degrading. A dispatched run already has a committed grant, so a read failure here is not an acceptable fallback.
Return the error from addGitEnv/jobEnv and let createJob leave the run queued for the next tick, or load the snapshots once in createJob (they are already read there for ClaimRunDispatch) and pass the resolved grant into addGitEnv.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@orchestrator/internal/reconciler/reconciler.go` around lines 2489 - 2496,
Update addGitEnv and jobEnv to propagate ListRunPluginSnapshots errors instead
of silently retaining mutable Service values; ensure createJob handles the
propagated error by leaving the run queued for retry, or reuse the snapshots
already loaded for ClaimRunDispatch and pass the resolved grant into addGitEnv.
| pool *pgxpool.Pool | ||
| workflowRunTimeoutSecs int64 | ||
| workflowSessionTTLSecs int64 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check when ConfigureWorkflowTimeoutDefaults runs relative to serving traffic.
rg -n -C8 'ConfigureWorkflowTimeoutDefaults' orchestrator/cmd/orchestrator/main.go orchestrator/internal/reconciler/reconciler.goRepository: cnjack/cloud
Length of output: 3048
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== orchestrator/internal/store/pg.go relevant symbols =="
ast-grep outline orchestrator/internal/store/pg.go --view compact || true
echo
echo "== ConfigureWorkflowTimeoutDefaults and createRunTx context =="
rg -n -C12 'func configureWorkflowTimeoutDefaults|func createRunTx|workflowRunTimeoutSecs|workflowSessionTTLSecs' orchestrator/internal/store/pg.go
echo
echo "== Memory store context =="
rg -n -C12 'workflowRunTimeoutSecs|workflowSessionTTLSecs|mu' orchestrator/internal/store/memory.go
echo
echo "== New calls around store.New and configure default in orchestrator =="
rg -n -C8 'New\(ctx, cfg\.DatabaseURL\)|ConfigureWorkflowTimeoutDefaults|Reconciler\.New|reconciler\.New' orchestrator -g '*.go'Repository: cnjack/cloud
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== storage.go ConfigureWorkflowTimeoutDefaults =="
for f in $(git ls-files 'orchestrator/internal/store/*.go'); do
if rg -q 'func ConfigureWorkflowTimeoutDefaults|workflowRunTimeoutSecs|workflowSessionTTLSecs' "$f"; then
echo "--- $f ---"
rg -n -C6 'func ConfigureWorkflowTimeoutDefaults|configureWorkflowTimeoutDefaults|workflowRunTimeoutSecs|workflowSessionTTLSecs' "$f"
fi
done
echo "== callers and concurrent create-run paths =="
rg -n -C5 'ConfigureWorkflowTimeoutDefaults|CreateRun|createRunTx|http\.Handle|mux.*Create|Runs|POST.*run|Runs.*POST' orchestrator -g '*.go' | sed -n '1,220p'Repository: cnjack/cloud
Length of output: 24309
Guard PGStore timeout defaults with synchronization.
ConfigureWorkflowTimeoutDefaults writes workflowRunTimeoutSecs and workflowSessionTTLSecs, while createRunTx reads them without any lock. Calls can occur from main and reconciler.New while reconcile paths are already using the same store. Use atomic.Int64 for both fields as shown for MemStore.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@orchestrator/internal/store/pg.go` around lines 21 - 23, Change PGStore’s
workflowRunTimeoutSecs and workflowSessionTTLSecs fields to atomic.Int64, then
update ConfigureWorkflowTimeoutDefaults to store values atomically and
createRunTx to load them atomically, following the existing MemStore pattern.
| if len(executionContract) > 0 { | ||
| decoder := json.NewDecoder(strings.NewReader(string(executionContract))) | ||
| decoder.DisallowUnknownFields() | ||
| var contract domain.WorkflowContract | ||
| if err := decoder.Decode(&contract); err != nil { | ||
| return nil, fmt.Errorf("scan run execution contract: %w", err) | ||
| } | ||
| if err := contract.Validate(); err != nil { | ||
| return nil, fmt.Errorf("scan run execution contract: %w", err) | ||
| } | ||
| r.ExecutionContract = &contract | ||
| } | ||
| if len(reviewPlan) > 0 { | ||
| parsed, err := domain.UnmarshalStoredReviewPlan(reviewPlan) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("scan run review plan: %w", err) | ||
| } | ||
| r.ReviewPlan = parsed | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not fail a row scan because a stored contract no longer validates.
scanRun rejects the whole row when the persisted execution_contract has an unknown field or fails Validate. Two normal events trigger that:
- A rolling upgrade where a newer orchestrator writes a field this binary does not know.
DisallowUnknownFieldsthen fails the scan. - An intentional edit to a built-in definition.
ValidatecomparesWorkflow.DefinitionHashwithhashJSON(def)(orchestrator/internal/domain/workflow_contract.goLines 409-411), so every already-persisted Run stops loading.
Because ListRuns and ListRunsByService return on the first scan error, one bad row hides an entire project's Run list, not just its contract projection. Decode tolerantly, skip the projection on validation failure, and log the Run id.
🛡️ Proposed tolerant decode
if len(executionContract) > 0 {
- decoder := json.NewDecoder(strings.NewReader(string(executionContract)))
- decoder.DisallowUnknownFields()
var contract domain.WorkflowContract
- if err := decoder.Decode(&contract); err != nil {
+ if err := json.Unmarshal(executionContract, &contract); err != nil {
return nil, fmt.Errorf("scan run execution contract: %w", err)
}
- if err := contract.Validate(); err != nil {
- return nil, fmt.Errorf("scan run execution contract: %w", err)
+ if err := contract.Validate(); err == nil {
+ r.ExecutionContract = &contract
}
- r.ExecutionContract = &contract
}Report the validation failure through a counter or log at the call site so drift stays visible.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(executionContract) > 0 { | |
| decoder := json.NewDecoder(strings.NewReader(string(executionContract))) | |
| decoder.DisallowUnknownFields() | |
| var contract domain.WorkflowContract | |
| if err := decoder.Decode(&contract); err != nil { | |
| return nil, fmt.Errorf("scan run execution contract: %w", err) | |
| } | |
| if err := contract.Validate(); err != nil { | |
| return nil, fmt.Errorf("scan run execution contract: %w", err) | |
| } | |
| r.ExecutionContract = &contract | |
| } | |
| if len(reviewPlan) > 0 { | |
| parsed, err := domain.UnmarshalStoredReviewPlan(reviewPlan) | |
| if err != nil { | |
| return nil, fmt.Errorf("scan run review plan: %w", err) | |
| } | |
| r.ReviewPlan = parsed | |
| } | |
| if len(executionContract) > 0 { | |
| var contract domain.WorkflowContract | |
| if err := json.Unmarshal(executionContract, &contract); err != nil { | |
| return nil, fmt.Errorf("scan run execution contract: %w", err) | |
| } | |
| if err := contract.Validate(); err == nil { | |
| r.ExecutionContract = &contract | |
| } | |
| } | |
| if len(reviewPlan) > 0 { | |
| parsed, err := domain.UnmarshalStoredReviewPlan(reviewPlan) | |
| if err != nil { | |
| return nil, fmt.Errorf("scan run review plan: %w", err) | |
| } | |
| r.ReviewPlan = parsed | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@orchestrator/internal/store/pg.go` around lines 505 - 523, Update scanRun’s
execution-contract handling to decode without DisallowUnknownFields and treat
validation failures as a skipped ExecutionContract projection rather than
returning a row-scan error. Preserve errors for malformed contract data, and
report skipped validation failures through the existing logging or counter path
with the Run id so ListRuns and ListRunsByService continue loading other rows.
1 participant
Summary
Closes #24
Verification
cd orchestrator && go test ./...cd console && corepack pnpm test(76 files / 606 tests)corepack pnpm typecheck && corepack pnpm build && corepack pnpm lint:tokensrunner/test-fetch-source.sh(credential-free source + exact-SHA Review Plan + structured output)runner/test.sh(containerized no-TTY full loop)glm-5.2, and Grok CLIgrok-4.5adversarial audits; final verdict GODelivery
This is intentionally a ready-for-review PR, not a draft. Cloud never auto-approves or auto-merges provider PRs.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation