Repository navigation
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
🏗️ ADR gate result: existing ADR verifiedI found two existing ADRs on this branch that cover the architectural decision in this PR:
Both records include the required Michael Nygard sections ( Evidence checked
Alignment resultFull alignment. I did not find code in the PR diff that contradicts the stated ADR decisions, nor major scope creep beyond the ADR extension already present on the branch. Next actionNo new ADR is required for this PR. Keep the implementation and any follow-up changes aligned with these existing ADR commitments.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
I did not find a changed-line defect here that clearly blocks merge.
The risky paths I checked
I focused on the ledger-version upgrade, FIFO/selection parity between the Go and JavaScript implementations, and the stale-claim fail-closed publication path. The new shared fixtures and retry/reselection coverage line up with the implementation, and I did not find a concrete correctness regression in the changed lines that justified a blocking review comment.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 60.9 AIC · ⌖ 7.26 AIC · ⊞ 20.2K
Comment /review to run again
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
FIFO validation, per-handler staged behavior, and fallback artifact handling have unresolved correctness issues.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Adds durable FIFO ordering and declarative work selection across the Go CLI, JavaScript MCP runtime, workflow compiler, and formal specification.
Changes:
- Introduces version-3 queue records, FIFO sequences, filters, sorting, grouping, and
claim-next. - Adds trusted claim publication, revalidation, assignment verification, and workflow dispatch integration.
- Extends documentation, shared conformance tests, and TLA+ models.
| File | Description |
|---|---|
specs/dispatch-work-coordinator/WorkQueueSelection.tla |
Adds selection refinement. |
specs/dispatch-work-coordinator/transactions.tsp |
Defines version-3 fields. |
specs/dispatch-work-coordinator/Submission.cfg |
Adds submission profile. |
specs/dispatch-work-coordinator/Selection.cfg |
Adds selection profile. |
specs/dispatch-work-coordinator/selection-fixtures.json |
Shares selection fixtures. |
specs/dispatch-work-coordinator/RecoveryWitness.cfg |
Configures queue constants. |
specs/dispatch-work-coordinator/Recovery.cfg |
Configures queue constants. |
specs/dispatch-work-coordinator/README.md |
Documents protocol semantics. |
specs/dispatch-work-coordinator/ExternalEffectWitness.cfg |
Configures queue constants. |
specs/dispatch-work-coordinator/DispatchWorkCoordinator.tla |
Models FIFO and reconciliation. |
specs/dispatch-work-coordinator/DispatchWorkCoordinator.cfg |
Configures queue constants. |
specs/dispatch-work-coordinator/CompetingClaimsWitness.cfg |
Configures queue constants. |
specs/dispatch-work-coordinator/check.sh |
Runs expanded TLC profiles. |
specs/dispatch-work-coordinator/BrokenTerminal.cfg |
Configures negative control. |
specs/dispatch-work-coordinator/BrokenSelection.cfg |
Adds selection negative control. |
specs/dispatch-work-coordinator/BrokenQueue.cfg |
Adds reconciliation negative control. |
specs/dispatch-work-coordinator/BrokenCAS.cfg |
Configures negative control. |
pkg/workqueue/selection.go |
Implements queue selection. |
pkg/workqueue/selection_values.go |
Compares filter and sort values. |
pkg/workqueue/selection_test.go |
Tests selection and retries. |
pkg/workqueue/schema/WorkTransaction.json |
Adds version and sequence schema. |
pkg/workqueue/schema/WorkCancellationTransaction.json |
Adds version schema. |
pkg/workqueue/schema/CompletionTransaction.json |
Adds version schema. |
pkg/workqueue/schema/ClaimTransaction.json |
Adds version schema. |
pkg/workqueue/schema/ClaimCancellationTransaction.json |
Adds version schema. |
pkg/workqueue/replay.go |
Upgrades replay to version 3. |
pkg/workqueue/replay_test.go |
Tests numeric interoperability. |
pkg/workqueue/protocol.go |
Implements protocol upgrades. |
pkg/workqueue/protocol_test.go |
Tests version codemods. |
pkg/workqueue/intents.go |
Validates queue intents. |
pkg/workqueue/branch.go |
Migrates and upgrades branches. |
pkg/workqueue/branch_test.go |
Tests legacy migration. |
pkg/workflow/mcp_renderer_test.go |
Tests claim-next exposure. |
pkg/workflow/mcp_renderer_builtin.go |
Exposes the MCP tool. |
pkg/workflow/mcp_manifest.go |
Registers the MCP tool. |
pkg/workflow/dispatch_work_coordinator_compilation_integration_test.go |
Tests compiler wiring. |
pkg/workflow/compiler_yaml_step_lifecycle.go |
Copies claim intents. |
pkg/workflow/compiler_yaml_post_agent.go |
Uploads claim intents. |
pkg/workflow/compiler_safe_outputs_job.go |
Reconciles trusted claims. |
pkg/constants/constants.go |
Defines claim-intent path. |
pkg/cli/work_command.go |
Adds claim-next. |
pkg/cli/work_command_test.go |
Tests CLI claiming. |
docs/src/content/docs/reference/tools.md |
Documents work-queue tools. |
docs/adr/dispatch-work-coordinator-protocol-upgrades.md |
Records protocol decisions. |
actions/setup/setup.sh |
Packages new runtime scripts. |
actions/setup/js/write_dispatch_work_coordinator_snapshot.test.cjs |
Tests upgraded snapshots. |
actions/setup/js/write_dispatch_work_coordinator_snapshot.cjs |
Verifies assignment payloads. |
actions/setup/js/publish_dispatch_work_claims.test.cjs |
Tests trusted publication. |
actions/setup/js/publish_dispatch_work_claims.cjs |
Publishes staged claims. |
actions/setup/js/finish_dispatch_work_claim.test.cjs |
Tests reconciliation behavior. |
actions/setup/js/finish_dispatch_work_claim.cjs |
Integrates claim publication. |
actions/setup/js/dispatch_workflow.test.cjs |
Tests verified dispatches. |
actions/setup/js/dispatch_workflow.cjs |
Injects trusted assignments. |
actions/setup/js/dispatch_work_coordinator_store.test.cjs |
Tests storage migration. |
actions/setup/js/dispatch_work_coordinator_store.cjs |
Unifies branch storage. |
actions/setup/js/dispatch_work_coordinator_selection.test.cjs |
Tests selection conformance. |
actions/setup/js/dispatch_work_coordinator_selection.cjs |
Implements JavaScript selection. |
actions/setup/js/dispatch_work_coordinator_replay.test.cjs |
Tests version-3 replay. |
actions/setup/js/dispatch_work_coordinator_replay.cjs |
Implements version-3 replay. |
actions/setup/js/dispatch_work_coordinator_mcp_server.test.cjs |
Tests MCP claim staging. |
actions/setup/js/dispatch_work_coordinator_mcp_server.cjs |
Adds dispatch_claim_next. |
actions/setup/js/dispatch_work_coordinator_codemods.test.cjs |
Tests codemod preservation. |
actions/setup/js/dispatch_work_coordinator_codemods.cjs |
Adds ordered upgrades. |
.github/workflows/smoke-dispatch-work-coordinator.lock.yml |
Regenerates smoke workflow wiring. |
| if tx.Version == CurrentVersion && (tx.Sequence < 1 || tx.Sequence > MaxSequence) { | ||
| return errors.New("work sequence must be a positive safe integer") |
There was a problem hiding this comment.
Added Work-sequence ownership validation in both Go and JavaScript replay/upgrade paths; distinct work identities can no longer share a FIFO rank. Commit: e63d6141a3.
| var staged *TemplatableBool | ||
| if data.SafeOutputs != nil { | ||
| staged = data.SafeOutputs.Staged | ||
| } | ||
| if value := resolveSafeOutputsStagedValue(c.trialMode, staged); value != nil { | ||
| steps = append(steps, " env:\n") | ||
| steps = append(steps, buildTemplatableBoolEnvVar("GH_AW_SAFE_OUTPUTS_STAGED", value)...) |
There was a problem hiding this comment.
Reconciliation now combines global and dispatch-workflow handler staging, including templated values, so staged dispatches do not durably publish claims. Commit: e63d6141a3.
| } | ||
| if isDispatchWorkCoordinatorEnabled(data) { | ||
| paths = append(paths, constants.DispatchCoordinatorFinishIntentPath) | ||
| paths = append(paths, constants.DispatchCoordinatorClaimIntentPath) |
There was a problem hiding this comment.
Included the claim-intent sidecar in the fallback artifact upload and regenerated the smoke workflow lock. Commit: e63d6141a3.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — commenting with a few targeted suggestions; no blocking issues found.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap: the
MaxSequence/Number.MAX_SAFE_INTEGERexhaustion guard is implemented in three Go call sites and one JS call site, but none is exercised by a test that actually drives a work log to the limit. - Duplicated invariant: the "track max sequence per WorkID, never reuse, never exceed
MaxSequence" logic is copy-pasted acrossassignHistoricalSequences,normalizeTransactions, and the selection/claim path — a good candidate for a single shared helper. - Minor naming asymmetry: the historical branch name
"dispatch-coordinator"is an inline literal next to the namedDefaultBranchconstant. - Validation ordering: in
dispatch_workflow.cjs, the "verified publication" check runs before the "same-repo + aw_context" shape check, which can mask which invariant actually failed for a given input.
Positive Highlights
- ✅ Excellent protocol-upgrade design: version-2→3 codemods are data-driven, symmetric between Go and JS, and extensively fixture-tested (
selection-fixtures.jsonshared across both runtimes). - ✅ The legacy-branch migration (
branch.go) correctly preserves commit ancestry and only triggers forDefaultBranch, with a dedicated regression test (TestBranchMigratesLegacyRuntimeBranch). - ✅ Staged claim re-validation in
publish_dispatch_work_claims.cjscorrectly fails closed on stale selections rather than silently substituting a different work item — matches the TLA+-verified safety property described in the PR body. - ✅ Formal verification (TLA+) backing the new FIFO/selection semantics is a strong signal of correctness for the core protocol logic, beyond what unit tests alone typically provide.
Note: /tmp/gh-aw/agent/pr-diff.patch was truncated at 3000 lines (64 changed files, 3002 additions). Several files — including pkg/workqueue/selection.go, selection_values.go, selection_test.go, the TLA+ spec, and the schema/spec fixture files — were outside the truncated diff and were fetched separately via gh api to review. Skill selection used the pr-triage agent's output (/tdd, /codebase-design).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 211.3 AIC · ⌖ 14.8 AIC · ⊞ 10K
Comment /matt to run again
| if hasStatus(err, http.StatusNotFound) { | ||
| if b.Name == DefaultBranch { | ||
| legacyBranch := b | ||
| legacyBranch.Name = "dispatch-coordinator" |
There was a problem hiding this comment.
[/codebase-design] The historical runtime branch name "dispatch-coordinator" is a magic string with no symbolic home, while DefaultBranch right above it is a named constant. This asymmetry makes the migration path harder to discover and easy to typo if a second caller needs it later.
💡 Suggested fix
Add a LegacyBranch = "dispatch-coordinator" constant next to DefaultBranch in replay.go, and reference it here instead of the inline literal. This keeps the historical/legacy vocabulary discoverable in one place, consistent with how DefaultBranch, FileName, and CurrentVersion are already named.
@copilot please address this.
There was a problem hiding this comment.
Added and used the LegacyBranch constant for the historical runtime branch. Commit: e63d6141a3.
| sequence, exists := sequences[tx.WorkID] | ||
| if !exists { | ||
| if maximum == MaxSequence { | ||
| return nil, errors.New("work queue sequence exhausted") |
There was a problem hiding this comment.
[/tdd] assignHistoricalSequences, normalizeTransactions, and nextSequence all implement the MaxSequence exhaustion guard (lines 122, 162, 181), but no Go test in protocol_test.go, replay_test.go, or selection_test.go drives a work log to MaxSequence to exercise this branch. The JS counterpart in dispatch_work_coordinator_codemods.cjs (Number.MAX_SAFE_INTEGER) is similarly untested per dispatch_work_coordinator_codemods.test.cjs.
💡 Suggested test
func TestSequenceExhaustionIsRejected(t *testing.T) {
tx := Transaction{Kind: "Work", WorkID: "w", Work: json.RawMessage(`{}`), Version: CurrentVersion, Sequence: MaxSequence}
if _, err := nextSequence([]Transaction{tx}); err == nil {
t.Fatal("expected sequence exhaustion error")
}
}This is a boundary condition reachable in production after ~9 quadrillion work items (unlikely but part of the documented safety contract), so a regression here would silently corrupt FIFO ordering rather than fail loudly if ever hit with a smaller test boundary constant.
@copilot please address this.
There was a problem hiding this comment.
Added Go and JavaScript sequence-exhaustion coverage, including the maximum-rank boundary. Commit: e63d6141a3.
| schema, ok := schemas[tx.Kind] | ||
| if !ok { | ||
| return parsedRecord{}, fmt.Errorf("unknown transaction kind %q", tx.Kind) | ||
| } |
There was a problem hiding this comment.
[/codebase-design] The sequence-assignment loop (track max, detect existing, increment-on-exhaustion-check) is duplicated nearly verbatim across assignHistoricalSequences (here), normalizeTransactions (below), and nextSequence in selection.go/intents.go. Three independent copies of the same invariant (never exceed MaxSequence, never reuse a WorkID's sequence) is a maintainability risk — a future fix to one path won't automatically propagate to the others.
💡 Suggested refactor
Extract a small sequenceAllocator helper (track sequences map[string]int64 + maximum int64, expose allocate(workID string) (int64, error) and record(workID string, seq int64)) and have all three call sites use it. This turns the "sequence exhausted" and "reuse existing sequence" rules into a single deepened abstraction instead of three parallel implementations that must be kept in sync by hand.
@copilot please address this.
There was a problem hiding this comment.
Centralized Go sequence recording and allocation in sequenceAllocator, including uniqueness and exhaustion checks. Commit: e63d6141a3.
| // Only workflows listed in aw_context_workflows (populated at compile time) support this. | ||
| const queueClaimID = message.inputs?.work_queue_claim_id; | ||
| if (queueClaimID !== undefined && !isStaged && process.env.GH_AW_DISPATCH_CLAIMS_VERIFIED !== "true") { | ||
| throw new Error("work_queue_claim_id requires verified publication by tools.work-queue"); |
There was a problem hiding this comment.
[/diagnosing-bugs] Order-of-checks risk: the "verified publication" gate (line 300) throws before the "same-repo worker with aw_context support" gate (line 303). If GH_AW_DISPATCH_CLAIMS_VERIFIED isn't "true" (e.g. staged mode off, or env var simply unset because of an unrelated bug), an attacker-controlled queueClaimID for a cross-repo or non-aw_context workflow fails with the generic "requires verified publication" error instead of the more specific "requires a same-repository worker workflow" error — masking what would otherwise be two independently testable security invariants.
💡 Suggested fix
Swap the order so the same-repo/aw_context shape check runs first, then the verified-publication check. This way each invariant fails for its own reason and the test matrix (dispatch_workflow.test.cjs) can assert both failure modes independently rather than one potentially masking the other.
@copilot please address this.
There was a problem hiding this comment.
Moved worker shape validation ahead of the verified-publication gate and added regression coverage. Commit: e63d6141a3.
…election # Conflicts: # actions/setup/js/write_dispatch_work_coordinator_snapshot.test.cjs Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the listed review feedback in |
|
@copilot resolve the merge conflicts on this branch. |
…election # Conflicts: # .github/workflows/smoke-work-queue.lock.yml # actions/setup/js/finish_work_queue_claim.cjs # actions/setup/js/work_queue_codemods.cjs # actions/setup/js/work_queue_mcp_server.cjs # actions/setup/js/work_queue_mcp_server.test.cjs # actions/setup/js/work_queue_replay.cjs # actions/setup/js/work_queue_replay.test.cjs # actions/setup/js/work_queue_store.cjs # actions/setup/js/work_queue_store.test.cjs # actions/setup/js/work_queue_summary.test.cjs # actions/setup/js/write_dispatch_work_coordinator_snapshot.cjs # actions/setup/js/write_work_queue_snapshot.test.cjs # actions/setup/setup.sh # docs/adr/dispatch-work-coordinator-protocol-upgrades.md # pkg/constants/constants.go # pkg/workflow/compiler_safe_outputs_job.go # pkg/workflow/compiler_yaml_post_agent.go # pkg/workflow/mcp_manifest.go # pkg/workflow/mcp_renderer_builtin.go # pkg/workflow/mcp_renderer_test.go # pkg/workflow/work_queue_compilation_integration_test.go # pkg/workqueue/replay.go # specs/dispatch-work-coordinator/transactions.tsp # specs/work-queue/BrokenQueue.cfg # specs/work-queue/BrokenSelection.cfg # specs/work-queue/README.md # specs/work-queue/Selection.cfg # specs/work-queue/Submission.cfg # specs/work-queue/WorkQueueSelection.tla # specs/work-queue/check.sh # specs/work-queue/selection-fixtures.json Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Resolved the merge conflicts with current |
|
Great work on the FIFO work-queue claims! 🎯 Tests, validation and docs are all in place. This looks ready for review.
|
Summary
Dispatchers need a reliable default queue order and a declarative way to select work by priority, cost, or other objectives. This adds consistent FIFO and selection semantics across the Go CLI and JavaScript MCP runtime.
Branch.ClaimNext, CLIgh aw work-queue claim-next, and MCPdispatch_claim_nextusing a unified version-3 ledger.Migration and safety
Stop older writers before upgrading. Historical logs retain identities and acquire FIFO sequences from first-seen Work order; submission order already erased by prior compaction cannot be recovered. Legacy records without payloads or run provenance use explicit placeholders, not trusted provenance.
Go and JavaScript apply ordered version-2-to-3 codemods that change only the message version, preserving payloads, identities, FIFO sequences, provenance, and terminal facts. Missing required version-2 metadata and unknown future versions fail validation without partial publication. The activation snapshot envelope remains independently versioned at 2.
The unified default branch is
gh-aw-dispatch-work-coordinator; readers recognize the historical runtime branch and trusted publication preserves its ancestry. Payload numbers outside the JavaScript-safe range fail validation instead of being rounded. Explicit operator claims can bypass selection policies, so cooperating dispatchers must use the same grouping policy. Automatic orphan recovery remains out of scope; a failed downstream dispatch may require operator claim cancellation.Validation
make fmtandmake buildpassed.go test ./pkg/workqueue -count=1and focused Go queue, CLI, and compiler tests passed, including version-2 compatibility, field preservation, rejection of incomplete/future messages, and existing queue behavior.make agent-report-progress-no-testpassed, including Go/custom/JavaScript lint and the drift check for all 302 workflow lock files. Impacted Go unit tests passed earlier in this PR.