Repository navigation
[3379 by Claude Opus 5.5] 0.9.0 release logic fix: multi-model runs on one issue silently drop every model whose job starts after the first model finishes, and leave those tasks stuck pending forever. Check current upstream main first. - #3384
integry merged 3 commits into
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…l now runs even if another model already finished and labelled the issue `AI-done`. A skipped job's task no longer stays `pending`; it ends `cancelled` with a reason. Upstream `main` was the same commit as this branch (700ce82), so nothing there already covered it. How sibling jobs are told apart - The matrix dispatcher now stamps every child job with a shared `dispatchId`, which is the dispatcher's BullMQ job id. A retried dispatch keeps that id, so children it re-adds are still siblings. Task replacements copy the field from their replay data. - In `checkLabelConditions` (`src/jobs/issueJob/github.ts`), an `AI-done` label no longer skips a job when all of these hold: - it has a `dispatchId`. - the issue was not already done when it was dispatched (from the label snapshot the dispatcher saves on the child). - its own task has not already completed. - The skip still applies to genuinely stale jobs: - jobs queued before this change (no `dispatchId`). - jobs dispatched after an earlier, separate request had finished the issue. - repeat deliveries of a model that already completed. Skipped tasks now end - In `src/jobs/processGitHubIssueJob.ts`, a skipped job marks its task `cancelled`. - The `AI-done` skip uses a new reason, `skipped_issue_done`, shown as "Skipped because the issue was already completed by another run." It is not in the set of reasons that send a cancellation notification. - The primary-tag-removed skip uses the existing `cancelled_label_removed`. - A task that has already ended (for example a duplicate's completed one) keeps its outcome. - I checked the other early exits in the issue job: user-stopped, withdrawn, authentication failure and workflow-policy failure already end their tasks. Labels: I took the issue's minimum option. The first model to finish still adds `AI-done`; it just no longer blocks the others. A later sibling re-adds `AI-processing` while it runs and swaps it back to `AI-done` when it finishes, so the issue ends `AI-done` only. I didn't delay `AI-done` until every sibling has ended, because that would mean changing every place that applies the label. Single-model runs behave as before. Tests - New `test/issueJobMultiModelSiblings.test.ts` (4 tests): - with concurrency 1, three models all run and complete after the first one labels the issue done. - stale jobs (queued before this change, or dispatched after the issue was done) are skipped and their tasks end `cancelled` / `skipped_issue_done`. - a repeat delivery of a completed model is skipped and its task stays `completed`. - the primary-tag-removed skip ends its task with `cancelled_label_removed`. To reach this path I had to turn off the withdrawal check, which normally catches a removed label first. - Against the original code, 3 of the 4 fail. The repeat-delivery test passes either way; it guards against the new check letting too much through. - I updated `test/worker.test.ts` with the new return values and sibling cases, and `test/issueJobDispatcher.handleDispatch.test.ts` now checks `dispatchId` on the children. Validation: all of these pass: - issue job, worker and task state suites: `test/worker*.test.ts`, `taskSubmissionLifecycle`, `issueJobDispatcher*`, `taskIntentWithdrawal`, `repositoryWorkflowAdmission`, `workerStateManager`, `workerTaskStateRecovery`, `issueJobCompletion`, plus the UI `TaskStatusTable` test. - root `tsc --noEmit` and `eslint --max-warnings 0 src/`. - `npm run lint --workspaces --if-present -- --max-warnings 0`. - the `release:verify` command you gave. `test/worker.test.ts` needs `PROPR_DEMO_MODE=true` to run in this container. I didn't run the full test suite. I added a 0.9.0 Fixed changelog bullet. Nothing was merged, tagged, published or deployed. No preview images, since the change isn't visible in the UI. Three connectors (Gmail, Google Calendar and Google Drive) need authorizing in your claude.ai connector settings before they can be used. This task didn't need them.
|
/review |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR fixes the normal sequential multi-model case, but needs a localized correction before merge: a partially successful dispatcher retry can still drop remaining models. ✅ Sibling continuation — Children dispatched before ✅ Explicit skip outcomes — Normal skipped jobs receive a cancellation reason, while already-terminal task outcomes are preserved. This review uses static analysis only; no commands were run. The supplied issue body is truncated, so requirements beyond its visible text were not assessed. Current checks report no failures and seven pending checks. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F1: 🔴 Preserve eligibility across dispatcher retries
SuggestionsThese are optional follow-ups and are not sent to No suggestions. ScoreThe normal execution path and terminal-reason handling are well covered, but dispatcher recovery still reproduces the central dropped-model failure. Pending checks have not reported failures. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
/fix F1 |
|
✅ Applied the requested follow-up changes in commit 894966a
Summary of ChangesI've fixed F1. A dispatch now saves the issue's labels once, before it enqueues its first child, and every retry of that dispatch reuses the saved labels instead of the issue it fetches again. In your partial-failure scenario, the remaining models now run after the retry. What changed
Same problem in replacement tasks. When a task is replaced, its replay data drops Tests
There's no visual change, so I didn't make previews. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…, before it enqueues its first child, and every retry of that dispatch reuses the saved labels instead of the issue it fetches again. In your partial-failure scenario, the remaining models now run after the retry. I've fixed F1. A dispatch now saves the issue's labels once, before it enqueues its first child, and every retry of that dispatch reuses the saved labels instead of the issue it fetches again. In your partial-failure scenario, the remaining models now run after the retry. **What changed** - **Dispatcher** (`src/jobs/issueJobDispatcher.ts`, `firstDispatchLabels`): on the first attempt, the issue's label names are written to the dispatcher job as `dispatchIssueLabels` via `job.updateData`, before any child is added. A retry finds them in its stored job data and uses them. Each child copies them. - **The check itself** (`src/jobs/issueJob/github.ts:isUnfinishedDispatchSibling`): it now reads `dispatchIssueLabels` instead of the per-attempt `issuePayload`. - **New dispatches are still rejected as before.** If a dispatch first sees the issue already labelled `AI-done`, that label is saved with it, so its children are still skipped. - The new field is declared on `IssueJobData` in `packages/core/src/queue/taskQueue.types.ts`. **Same problem in replacement tasks.** When a task is replaced, its replay data drops `issuePayload` (`TRANSIENT_JOB_FIELDS` in `src/taskReplacement/store.ts`). So a replacement for a sibling had a `dispatchId` but no labels to check, and was skipped once the issue was done. `dispatchIssueLabels` is not dropped on replay, so the same change fixes this. I added an assertion for it in `test/worker.test.ts`. **Tests** - **New regression** in `test/issueJobMultiModelSiblings.test.ts`, run through the real dispatcher and issue-job path: 1. The second `issueQueue.add` fails, so the dispatcher throws with only `model-a` queued. 2. `model-a` runs and labels the issue `AI-done`. 3. The dispatcher retries from its stored job data and re-adds `model-b` and `model-c`. 4. Both run, and all three tasks end `completed`. 5. A fresh dispatch on the now-done issue still has its child skipped as "Already done". - **It fails on the old code**, at "model-b ran after the retried dispatch", which is the scenario from the review. - **Fixture updates:** the existing sibling fixtures now use `dispatchIssueLabels`. The existing dispatcher test's job got an `updateData` stub, because the dispatcher now writes job data. - **Results:** - The sibling, dispatcher and worker suites pass (12/12). - The submission-lifecycle, dispatcher, replacement and retry suites pass (166/166). - `tsc --noEmit` is clean. - ESLint reports no problems in the changed `src/` files. The only warnings say the test files fall outside the lint config. There's no visual change, so I didn't make previews. PR: #3384 Comment by: @integry (ID: 6504368318) Model: Claude Opus 5.5
|
CI failed: Full Test Suite Shard 3/4 Please investigate and fix this CI failure.
Failure evidence
|
|
CI failed: Full Test Suite Shard 4/4 Please investigate and fix this CI failure.
Failure evidence
|
|
CI failed: Full Test Suite Shard 1/4 Please investigate and fix this CI failure.
Failure evidence
|
|
/review |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR addresses delayed sibling execution by preserving dispatch eligibility and records terminal outcomes for skipped tasks. No merge-blocking code defect was verified from the supplied code, but the current failed checks prevent calling it merge-ready. ✅ Retry-stable eligibility — The dispatcher persists its original labels before enqueueing children and reuses them on retries. ✅ Targeted regression coverage — The added test covers partial enqueue failure, completion of the first child, and successful execution of remaining models after dispatcher retry. ✅ Preserved task outcomes — Skip handling preserves existing terminal states and supplies explicit cancellation reasons. This was a static review; no commands or tests were run. The supplied issue body is truncated, so objective validation is limited to its visible requirements. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Investigate current test failuresThe authoritative status reports failures in test shards 1, 3, and 4. Inspect their diagnostics to establish whether they involve this change. The supplied evidence does not connect those failures to changed code, so they are not code findings; further investigation is needed to establish merge readiness. ScoreThe implementation and targeted regression coverage support the visible objective. Current test failures leave validation unresolved despite no verified code blocker. Score: 7/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
The full test suite fails on 894966a. All 7 failures are in the task follow-up-issue family, so this looks like a regression from the dispatcher or label-check change, not a flake:
Shard logs show repeated |
|
✅ Applied the requested follow-up changes in commit 1174b4b Summary of ChangesI've fixed the 7 follow-up issue test failures. It was a regression from the F1 change, not a flake, and all 7 files now pass locally. No test assertions were changed. Cause: the F1 change saves the issue's labels by calling The timeout messages are not the cause. Fix (
The multi-model sibling behaviour, the F1 retry eligibility and the Checks:
There are no visual changes, so I didn't create any previews. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…gression from the F1 change, not a flake, and all 7 files now pass locally. No test assertions were changed.
I've fixed the 7 follow-up issue test failures. It was a regression from the F1 change, not a flake, and all 7 files now pass locally. No test assertions were changed.
**Cause:** the F1 change saves the issue's labels by calling `job.updateData` inside `handleDispatchWithDeps`. Before that, the dispatcher never wrote to the job directly; every outside call came in through its injected dependencies. The follow-up issue tests pass the dispatcher a plain `{ id, name, data }` job with no `updateData` method. So each dispatch failed with `TypeError: job.updateData is not a function` before it could add any child job.
**The timeout messages are not the cause.** `Adding 'AI' to #14 failed: Request timed out` and `Connection timed out` appear 4 times both before and after the fix. They come from scenarios the tests simulate deliberately.
**Fix** (`src/jobs/issueJobDispatcher.ts`):
- The dispatcher's dependencies now include an optional `saveDispatchData`. The real dispatcher sets it to `job.updateData`, so production still saves the labels once, before the first child is added.
- Callers that don't supply it (the follow-up and core submission tests) get the dispatcher's behaviour from before F1, for that harness only.
- The F1 retry regression test in `test/issueJobMultiModelSiblings.test.ts` now passes `saveDispatchData` explicitly.
The multi-model sibling behaviour, the F1 retry eligibility and the `cancelled` / `skipped_issue_done` end state for skipped jobs are unchanged.
**Checks:**
- All 30 related test files pass: every `packages/api/test/taskFollowupIssue*.test.ts`, the root `issueJob*` and dispatcher tests, `processGitHubIssueJob*`, `worker.test.ts`, and the core `taskSubmissionService` and `taskSubmissionIntake` tests.
- With `saveDispatchData` removed, the F1 regression test fails at "model-b ran after the retried dispatch", so it still catches the original bug.
- Root `tsc --noEmit` passes, and ESLint with `--max-warnings 0` is clean on the dispatcher.
There are no visual changes, so I didn't create any previews.
PR: #3384
Comment by: @integry (ID: 6505723310)
Model: Claude Opus 5.5
|
/review |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR is merge-ready within the visible scope, conditional on the pending Validate Changes check completing successfully. ✅ Retry-stable eligibility — The dispatcher saves its initial labels before enqueueing children and reuses them on retries. The partial-enqueue regression covers a sibling finishing before dispatch resumes. ✅ Explicit skipped-task outcomes — Skipped jobs receive cancellation reasons, while existing terminal outcomes are preserved. Replacement payloads retain dispatch eligibility. ✅ Focused regression coverage — Tests cover sequential model execution, stale dispatches, duplicate deliveries, and removed trigger labels. The supplied current-head status reports all four full-suite shards passing. This was a static review; no commands were run. The original issue description is explicitly truncated, so assessment is limited to its visible requirements and the supplied authorized follow-up. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S2: 🟢 Cover custom trigger labelsAdd a sibling-execution case using a configured trigger other than ScoreThe implementation addresses the visible objective with focused changes and relevant regression tests. Current checks show no failures, with one validation check still pending. Score: 9/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
AI Implementation Summary
Closes #3379
Branch:
3379/claude-opus-5-5-0-9-0-release-logic-fix-m-20261029-1556-v73Commits: ✅ Changes committed (fb8d98f)
AI Processing Completed
Execution Details:
pendingforever. Check current upstream main first. #3379Summary:
I've fixed #3379: when one request asks for several models, each model now runs even if another model already finished and labelled the issue
AI-done. A skipped job's task no longer stayspending; it endscancelledwith a reason. Upstreammainwas the same commit as this branch (700ce82), so nothing there already covered it.How sibling jobs are told apart
dispatchId, which is the dispatcher's BullMQ job id. A retried dispatch keeps that id, so children it re-adds are still siblings. Task replacements copy the field from their replay data.checkLabelConditions(src/jobs/issueJob/github.ts), anAI-donelabel no longer skips a job when all of these hold:dispatchId.dispatchId).Skipped tasks now end
src/jobs/processGitHubIssueJob.ts, a skipped job marks its taskcancelled.AI-doneskip uses a new reason,skipped_issue_done, shown as "Skipped because the issue was already completed by another run." It is not in the set of reasons that send a cancellation notification.cancelled_label_removed.Labels: I took the issue's minimum option. The first model to finish still adds
AI-done; it just no longer blocks the others. A later sibling re-addsAI-processingwhile it runs and swaps it back toAI-donewhen it finishes, so the issue endsAI-doneonly. I didn't delayAI-doneuntil every sibling has ended, because that would mean changing every place that applies the label. Single-model runs behave as before.Tests
test/issueJobMultiModelSiblings.test.ts(4 tests):cancelled/skipped_issue_done.completed.cancelled_label_removed. To reach this path I had to turn off the withdrawal check, which normally catches a removed label first.test/worker.test.tswith the new return values and sibling cases, andtest/issueJobDispatcher.handleDispatch.test.tsnow checksdispatchIdon the children.Validation: all of these pass:
test/worker*.test.ts,taskSubmissionLifecycle,issueJobDispatcher*,taskIntentWithdrawal,repositoryWorkflowAdmission,workerStateManager,workerTaskStateRecovery,issueJobCompletion, plus the UITaskStatusTabletest.tsc --noEmitandeslint --max-warnings 0 src/.npm run lint --workspaces --if-present -- --max-warnings 0.release:verifycommand you gave.test/worker.test.tsneedsPROPR_DEMO_MODE=trueto run in this container. I didn't run the full test suite.I added a 0.9.0 Fixed changelog bullet. Nothing was merged, tagged, published or deployed. No preview images, since the change isn't visible in the UI.
Three connectors (Gmail, Google Calendar and Google Drive) need authorizing in your claude.ai connector settings before they can be used. This task didn't need them.
Detailed Logs:
03348913-c353-48f0-ba05-4e09aaf68c82Log files stored at:
/usr/src/app/logs/executions/issue-3379-2026-10-29T16-09-50-261Z-conversation.json/usr/src/app/logs/executions/issue-3379-2026-10-29T16-09-50-261Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3379.
💡 Need changes?
Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!