Repository navigation
Add trusted dispatch and fix work-queue history amplification - #68827
Conversation
Activate verified queue dispatchers through the GitHub CLI while preserving scheduler-selected immutable Claims. Require exact native run correlation and revision checks, and propagate dispatcher wait failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Exercise trusted controls, publication transports, deployment refresh, native bindings and idempotent recovery with deterministic API budgets. Add optional read-only benchmarks of captured live ledger Git history and fix same-epoch submission retries after Policy compaction. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Construct a cache-eligible queue client with the protected publisher token independently of dispatch credentials. Cover compiled credential isolation, preview behavior, full-step budgets and captured native history. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
pelikhan
changed the title
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (600 new lines across 📄 Draft ADR committed:
🔍 Evidence used
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES
The new trusted-dispatch path still has two boundary failures: it can launch an unverified revision on a default-branch race, and it does not verify that the committed lock still matches the source allowlist.
Themes
- Post-dispatch SHA verification is too late. Dispatch still targets the moving default branch, so a branch advance between route verification and workflow activation can execute a different dispatcher revision before the client detects the mismatch.
- Source/lock parity is not enforced. The dispatcher source and compiled lock are each validated in isolation, but the embedded compiled worker allowlist is never compared to
safe-outputs.dispatch-workflow.workflows. A stale or broadened lock can therefore bypass the narrowed source policy.
These both undermine the "trusted dispatcher" guarantee, so I'm treating them as merge blockers.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 67.1 AIC · ⌖ 8.16 AIC · ⊞ 21.4K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd, focused on the highest-impact files per pr-triage (pkg/workqueue/dispatcher.go, actions/setup/js/work_queue_dispatch.cjs, pkg/cli/run_workflow_revision.go, pkg/cli/work_command_dispatch.go, pkg/workflow/safe_outputs_env.go).
📋 Key Themes & Highlights
Key Themes
- Regression test gap for the core bug fix: the one-line change in
work_queue_dispatch.cjsthat removes thecompacted_parameters_digestguard (the stated root-cause fix for "same-epoch compacted-Policy recovery") isn't directly exercised by a test that constructs the previously-failing shape (same-epoch request with no digest). Existing coverage always routes throughcompactTransactions, which sets a digest, so this specific path was likely uncovered before and after the fix. - New native-run verification logic (
verifyExpectedWorkflowRun) has decent coverage for full-field mismatches but the@-suffix stripping onrun.Pathisn't independently tested with a suffixed path value. - Dispatcher lock validation (
validateDispatcherLock) correctly guards type assertions withokchecks (no panics), but a defensive-boundary test for malformedinputswould make that safety contract explicit and regression-proof.
Positive Highlights
- ✅ Clean credential separation: the new
GH_AW_WORK_QUEUE_TOKENplumbing incompiler_safe_outputs_job.go/safe_outputs_env.goreuses the exact existing token-resolution precedence (resolveSafeOutputGitHubTokenForConfig) rather than inventing a parallel path — good adherence to existing domain vocabulary. - ✅
pkg/cli/work_command_dispatch.goexplicitly rejects--branch/--request-idoverrides that would undermine the trusted dispatcher's authority boundary, and this is well covered byTestWorkQueueDispatchRejectsAuthorityOverrides. - ✅ Extensive, well-structured performance regression suite (
work_queue_control_performance.test.cjs) with explicit API-call budgets and negative controls for uncached clients — strong/tdddiscipline overall, which makes the one coverage gap above more notable by contrast. - ✅ Dispatcher name/source/lock validation (
dispatcher.go) is thorough, with a large table-driven rejection test covering the adversarial cases that matter (invalid names, worker-assignment-compatible locks, denied source reads).
Note: given the diff size (59 files, ~1300 additions), review focused on the non-generated, highest-impact files per the triage heuristic; lock files and docs were not line-by-line reviewed.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 198.1 AIC · ⌖ 15 AIC · ⊞ 11.1K
Comment /matt to run again
| Env map[string]any `yaml:"env"` | ||
| } `yaml:"jobs"` | ||
| } | ||
| if err := yaml.Unmarshal(compiled, &workflow); err != nil { |
There was a problem hiding this comment.
[/tdd] validateDispatcherLock treats any string value for work_queue_assignment input key presence as blocking, but it never checks the on.workflow_dispatch value could be nil (bare workflow_dispatch:) vs. a map — that path is handled, but there is no test asserting a non-map inputs (e.g. inputs: null or inputs: a-string) is rejected safely instead of panicking via a bad type assertion.
💡 Suggested test
{name: "malformed inputs", source: dispatcherSource, lock: strings.ReplaceAll(dispatcherLock, "workflow_dispatch:", "workflow_dispatch:\n inputs: not-a-map")},The current code does dispatch["inputs"].(map[string]any) behind an ok check, so it will not panic, but a regression test documents the safety boundary explicitly and guards against future refactors that might drop the ok guard.
@copilot please address this.
| state.transactions.flatMap(commit => commit.operations).find(operation => operation.kind === "Policy" && operation.epoch === prior.policy_epoch)?.policy ?? | ||
| (prior.compacted_parameters_digest && prior.policy_epoch === state.policy_epoch ? state.policy : undefined); | ||
| state.transactions.flatMap(commit => commit.operations).find(operation => operation.kind === "Policy" && operation.epoch === prior.policy_epoch)?.policy ?? (prior.policy_epoch === state.policy_epoch ? state.policy : undefined); | ||
| if (!policy) throw new Error("work_queue_policy_missing"); |
There was a problem hiding this comment.
[/diagnosing-bugs] This removes the prior.compacted_parameters_digest guard so same-epoch Policy recovery now also succeeds for accepted requests that were never compacted (no digest at all), fixing the bug described in the PR body. No new or modified test in this diff exercises the specific previously-failing case: policy_epoch matches current state, compacted_parameters_digest is undefined. The existing work_queue_deployment.test.cjs recovery test always goes through compactTransactions, which sets a digest, so it could not have caught this.
💡 Suggested regression test
Add a case where acceptedSubmissionParameters is called directly on a request object with policy_epoch equal to state.policy_epoch and compacted_parameters_digest left undefined (construct prior manually rather than via compactTransactions), asserting it now resolves instead of throwing work_queue_policy_missing. Without this, a future refactor could silently reintroduce the digest guard and regress the exact bug this PR fixes.
@copilot please address this.
| HeadSHA string `json:"head_sha"` | ||
| Path string `json:"path"` | ||
| Event string `json:"event"` | ||
| Repository struct { |
There was a problem hiding this comment.
[/codebase-design] verifyExpectedWorkflowRun strips a @ref suffix from run.Path with strings.Cut(run.Path, "@") before comparing to logsWorkflowsPathSegment+lockFile, but GitHub's Actions API path field for a run triggered from a reusable/composite context can also include query-like suffixes or be reported without the .github/workflows/ prefix in some edge cases. There's no test covering a path value that already omits the prefix (defensive parsing) or contains unexpected trailing data (e.g., a version ref with special characters).
💡 Suggested test
Add a path mismatch case to TestWorkQueueDispatchVerifiesNativeRevision that exercises a run path like .github/workflows/dispatcher.lock.yml@refs/heads/main to confirm the @-stripping logic is actually required and correctly exercised — the existing workflow mismatch case only swaps to an entirely different filename, not a suffixed one.
@copilot please address this.
There was a problem hiding this comment.
🔵 Needs a closer look
Trusted activation and credential-scoped caching affect security-sensitive paths and need human review, alongside unresolved compatibility issues.
4 open findings
What changed in this PR
Adds trusted dispatcher activation to gh-aw’s Git-backed work queue and reduces repeated checkpoint-history verification.
Changes:
- Adds verified dispatcher activation, preview, waiting, and native-run correlation.
- Wires publisher credentials into cache-eligible queue clients and regenerates 33 workflow locks.
- Fixes compacted submission retries and adds performance coverage, shared fixtures, and documentation.
| File | Description |
|---|---|
pkg/workqueue/provisioning_deployment.go |
Exposes the verified default route. |
pkg/workqueue/dispatcher.go |
Validates dispatcher source and compiled configuration. |
pkg/workqueue/dispatcher_test.go |
Tests dispatcher route validation. |
pkg/workflow/work_queue_dispatch_identity_integration_test.go |
Checks publisher-token identity. |
pkg/workflow/work_queue_dispatch_credentials_test.go |
Tests publisher and launch credential separation. |
pkg/workflow/safe_outputs_env.go |
Extracts the shared token resolver. |
pkg/workflow/compiler_safe_outputs_job.go |
Binds the queue publisher token. |
pkg/cli/work_command.go |
Registers the dispatch command. |
pkg/cli/work_command_test.go |
Checks dispatch command availability. |
pkg/cli/work_command_dispatch.go |
Implements activation, preview, and waiting. |
pkg/cli/work_command_dispatch_test.go |
Tests dispatch restrictions and run verification. |
pkg/cli/run_workflow_revision.go |
Verifies the activated native run. |
pkg/cli/run_workflow_execution.go |
Enforces expected revision and exact run identity. |
docs/src/content/docs/reference/work-queue.md |
Documents dispatch and performance testing. |
actions/setup/js/work_queue_store_checks.cjs |
Adds immutable fixture identifiers. |
actions/setup/js/work_queue_live_performance.test.cjs |
Measures offline captured-history reads. |
actions/setup/js/work_queue_dispatch.cjs |
Repairs same-epoch compacted-policy recovery. |
actions/setup/js/work_queue_control_performance.test.cjs |
Tests control-step budgets and retries. |
actions/setup/js/work_queue_control_adapter.cjs |
Constructs the publisher queue client. |
actions/setup/js/work_queue_control_adapter_checks.cjs |
Tests credential wiring and preview isolation. |
actions/setup/js/package.json |
Adds the performance-suite command. |
actions/setup/js/eslint_factory_portfolio.test.cjs |
Reuses shared native API fixtures. |
actions/setup/js/eslint_factory_native.test_helpers.cjs |
Shares compiled-policy and native API fixtures. |
.github/workflows/repo-tree-map.lock.yml |
Binds the queue publisher token. |
.github/workflows/eslint-refiner.lock.yml |
Binds the queue publisher token. |
.github/workflows/eslint-monster.lock.yml |
Binds the queue publisher token. |
.github/workflows/eslint-miner.lock.yml |
Binds the queue publisher token. |
.github/workflows/eslint-factory-dispatcher.lock.yml |
Binds the queue publisher token. |
.github/workflows/engine-conformance-pydantic-ai.lock.yml |
Binds the queue publisher token. |
.github/workflows/engine-conformance-pi.lock.yml |
Binds the queue publisher token. |
.github/workflows/engine-conformance-opencode.lock.yml |
Binds the queue publisher token. |
.github/workflows/engine-conformance-kiro.lock.yml |
Binds the queue publisher token. |
.github/workflows/engine-conformance-goose.lock.yml |
Binds the queue publisher token. |
.github/workflows/engine-conformance-gemini.lock.yml |
Binds the queue publisher token. |
.github/workflows/engine-conformance-dispatcher.lock.yml |
Binds the queue publisher token. |
.github/workflows/engine-conformance-deepseek-harness.lock.yml |
Binds the queue publisher token. |
.github/workflows/engine-conformance-cursor.lock.yml |
Binds the queue publisher token. |
.github/workflows/engine-conformance-crush.lock.yml |
Binds the queue publisher token. |
.github/workflows/engine-conformance-copilot.lock.yml |
Binds the queue publisher token. |
.github/workflows/engine-conformance-codex.lock.yml |
Binds the queue publisher token. |
.github/workflows/engine-conformance-claude.lock.yml |
Binds the queue publisher token. |
.github/workflows/engine-conformance-aider.lock.yml |
Binds the queue publisher token. |
.github/workflows/engine-conformance-agy.lock.yml |
Binds the queue publisher token. |
.github/workflows/deep-report.lock.yml |
Binds the queue publisher token. |
.github/workflows/daily-token-consumption-report.lock.yml |
Binds the queue publisher token. |
.github/workflows/daily-team-evolution-insights.lock.yml |
Binds the queue publisher token. |
.github/workflows/daily-secrets-analysis.lock.yml |
Binds the queue publisher token. |
.github/workflows/daily-report-dispatcher.lock.yml |
Binds the queue publisher token. |
.github/workflows/daily-repo-chronicle.lock.yml |
Binds the queue publisher token. |
.github/workflows/daily-regulatory.lock.yml |
Binds the queue publisher token. |
.github/workflows/daily-observability-report.lock.yml |
Binds the queue publisher token. |
.github/workflows/daily-issues-report.lock.yml |
Binds the queue publisher token. |
.github/workflows/daily-firewall-report.lock.yml |
Binds the queue publisher token. |
.github/workflows/daily-evals-report.lock.yml |
Binds the queue publisher token. |
.github/workflows/daily-compiler-quality.lock.yml |
Binds the queue publisher token. |
.github/workflows/artifacts-summary.lock.yml |
Binds the queue publisher token. |
.changeset/patch-work-queue-control-cache.md |
Records the caching improvement. |
.changeset/patch-work-queue-compacted-control-retry.md |
Records the compacted retry fix. |
.changeset/minor-work-queue-dispatch-command.md |
Records the new dispatch command. |
🧠 Review effort: Max
| if opts.ExpectedRevision != "" { | ||
| if runErr != nil { | ||
| return nil, fmt.Errorf("workflow activation was accepted but its run identity is unresolved; do not redispatch: %w", runErr) |
| } | ||
|
|
||
| func validateDispatcherSource(source []byte) error { | ||
| frontmatter, err := parser.ExtractFrontmatterFromContent(string(source)) |
| queue := tools["work-queue"] | ||
| enabled := queue == true |
| dispatch, ok := outputs["dispatch-workflow"].(map[string]any) | ||
| if !ok { | ||
| return queueError("dispatcher_invalid", "dispatcher must declare safe-outputs.dispatch-workflow") | ||
| } | ||
| workers, ok := dispatch["workflows"].([]any) | ||
| if !ok || len(workers) == 0 { | ||
| return queueError("dispatcher_invalid", "dispatcher must declare safe-outputs.dispatch-workflow.workflows") | ||
| } |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Reply to comment 6178759936: Acknowledged the completed skills-based review. This status comment requests no additional code change. |
|
Reply to comment 6178761195: Acknowledged the completed test quality analysis; |
|
Reply to comment 6178762020: Reviewed and completed the generated ADR, retaining Proposed status until maintainer acceptance, and linked it in the PR body. This completion notice requires no additional dispatcher change. |
|
Reply to comment 6178763523: Acknowledged the completed review; |
|
Reply to comment 6178768120: Both findings from this code quality review, the dispatch race and source-lock allowlist drift, were fixed in |
|
Reply to comment 6178868321: Accepted: completed the ADR with implementation-checked decisions, alternatives and consequences, and linked it in the PR body. Its Proposed status explicitly leaves maintainer acceptance pending. |
|
Reply to comment 6178899863: Both recorded themes were addressed in |
|
🎉 This pull request is included in a new release. Release: |
Summary
The ESLint factory investigation exposed a missing supported way to request trusted dispatch and repeated checkpoint-history verification that made trusted queue controls extremely slow. This adds the dispatcher CLI, fixes production cache wiring, and covers claiming, native-run binding, retries, and performance end to end.
gh aw work-queue --repo OWNER/REPO dispatch DISPATCHER, with preview and optional waiting. Dispatch goes through the trusted controller rather than assigning or rebinding immutable Claims; source/revision validation and native-run correlation protect the execution boundary.Performance results
The same full-step fixture has 32 ancestors, three deployment updates, 15 existing Work items, and 100 retained requests:
This removes 93.8% of API calls and reduces local fixture elapsed time by approximately 11.6x. The extra baseline requests are entirely Git commit/tree/blob reads. REST publication also stays within the single-history-traversal budget (338 total calls); full-batch retries make no writes or duplicate launches.
A fresh read-only live snapshot at
b4a11c106a7f5445a6cd8a6f4bb64c070b89c986contains 212,953 bytes, 15 Work items, 15 historical Claims, and 118 retained requests. Repeated reads cost 413 calls without caching versus two fresh probes with registered caching. The first cold read still costs 413 calls in either case.Cache eligibility rules, checkpoint verification, fresh authorization/ref probes, and compare-and-swap semantics are unchanged. Arbitrary injected/dynamic-auth clients without an explicit publisher token remain uncached negative controls; preview does not construct credential clients. Proof reuse is scoped to a registered, unchanged static-auth client within one invocation. Local timings use fake/offline transports, not GitHub latency.
Validation
make agent-report-progresspassed, including impacted tests and all 344 workflow drift checks; broad compilation reported 361 existing warnings. The 33 regenerated locks each add only the protected publisher-token environment binding. No authored workflow Markdown or provenance-managed locks changed.npm --prefix actions/setup/js run test:work-queue-performancewith the fresh captured ledger/history: all 16 tests passed.npm --prefix actions/setup/js run typecheck,make fmt, andmake lint-cjspassed; lint reported warnings but no errors.go test -tags=integration ./pkg/workflow -run 'TestWorkQueueCompiledPolicyAndLaunchIdentity$' -count=1passed, executing the generated control script with separate publisher/launch identities and rejection cases../gh-aw compile eslint-factory-dispatcher --dry-runpassed strict validation, model checks, and shellcheck with zero warnings. Optional scanners were not run; the production lock was restored afterward. An initial--dry-run --no-emitattempt was rejected as an unsupported flag combination and was replaced by this emitted-lock check.Architecture decision
ADR: ADR-68827: Route Trusted Work-Queue Dispatch Through the Controller and Build the Control Queue Client From the Publisher Token.