Repository navigation
Report missing work queue pools with actionable recovery diagnostics - #71495
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot work queue pools should be auto installed on first use. admin need not required. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
![github-advanced-security[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (!installedPolicy || installedPolicy.authorization !== "aw" || Object.hasOwn(installedPolicy.pools, operation.pool)) throw queueError("policy_invalid", "only new pools can be installed into an AW-managed Policy"); | ||
| const policy = structuredClone(installedPolicy); | ||
| policy.pools[operation.pool] = structuredClone(operation.config); | ||
| validatePolicy(policy); |
There was a problem hiding this comment.
Fixed in 94d6500: PoolInstall rejects reserved pool identities before lookups or mutation, and inserts an own data property with Object.defineProperty. Regression tests cover reserved keys and malformed/oversized identities.
|
@copilot Review spec and TLA+ model |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented first-use automatic installation for new AW-managed pools in
The normative spec already allowed additive |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
|
✅ PR Code Quality Reviewer completed the code quality 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.
|
|
✅ 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.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
🏗️ Design Decision Gate — ADR RequiredThis PR adds 402 new lines of core business logic (more than the 100-line threshold) in 📄 Draft ADR committed:
Decision found in the diff: this PR adds a new
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and check that the implementation matches the decision. ❓ Why ADRs MatterADRs keep a searchable, permanent record of why the codebase looks the way it does. This PR widens who can write Policy ( 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
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://cdn.statically.io/img/avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The new additive pool path has two merge-blocking authorization problems: authoritative Go replay accepts compiler-unapproved worker routes from producer/dispatcher operations, and dispatch now mutates the live Policy despite the PR's stated explicit-administrator recovery boundary. See the inline comments for the required fixes.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~4 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
| return queueError("policy_invalid", "only new pools can be installed into an AW-managed Policy") | ||
| } | ||
| if _, exists := state.Policy.Pools[install.Pool]; exists { | ||
| return queueError("policy_invalid", "only new pools can be installed into an AW-managed Policy") |
There was a problem hiding this comment.
This replay path accepts arbitrary worker routes from any producer or dispatcher, so a direct pool_install request can add a workflow/ref/contract that was never compiler-approved; the allowlist check exists only in the JS helper and is bypassed by replaying the ledger.
💡 Enforce approval at the authoritative boundary
installReplayPool checks only effect scope and principal, then commits the supplied profile. Since PoolInstall is explicitly permitted for producer/dispatcher roles, callers can submit a crafted operation without going through installWorkQueuePools. Enforce a verifiable approval (for example, a signed/proposal binding) during replay, or narrow this operation to an authority that can establish compiler approval; otherwise the stated worker-route safety is not enforced by the ledger.
There was a problem hiding this comment.
Fixed in 94d6500. Both Go and JS replay now require each profile to exactly match existing same-named worker authority or an available registered Deployment revision. Raw operations cannot introduce a workflow, ref, contract, or broader authority; regression tests bypass the helper.
| const stableId = requestIdForIntent(trustedContext, message.intent_id); | ||
| const prior = latest.projection.requests.get(stableId); | ||
| if (!prior) { | ||
| latest = (await installWorkQueuePools(configured, trustedContext, [requested.pool])) || latest; |
There was a problem hiding this comment.
This dispatch path now appends a live PoolInstall commit for a missing pool, contradicting the PR contract that new missing-pool requests are rejected and that live recovery still requires an explicit administrator Policy update; a worker-facing dispatch therefore mutates installed Policy before deployment synchronization.
💡 Preserve the documented recovery boundary
installWorkQueuePools publishes a durable Policy mutation and loadQueue immediately observes it. That is not merely diagnostic handling and can change the active queue configuration from a dispatch request. Either reject the missing pool before synchronization as described, or redesign the authorization and documentation around an explicit administrator-approved installation; do not silently turn dispatch into Policy administration.
There was a problem hiding this comment.
The maintainer's first-use auto-install requirement supersedes the original diagnostics-only scope. 94d6500 documents that explicitly in the specification and ADR. Installation remains non-administrator and additive, but replay now prevents introducing new worker authority; existing-pool/economic Policy changes still require administrator authorization and quiescence.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on a prototype-pollution gap in the new pool-install path.
📋 Key Themes & Highlights
Key Themes
- Prototype pollution risk:
work_queue_replay.cjs:1238assigns topolicy.pools[operation.pool]on a plain object.identity()validates length/control-chars but not__proto__/constructor/prototype, matching the GitHub Advanced Security finding already on this PR. The same bracket-lookup pattern inwork_queue_pool_install.cjs:22(proposal.pools[poolName]) silently resolves__proto__toObject.prototype(truthy), bypassing the intendedwork_queue_pool_not_installedguard. - Test coverage gap: the new
work_queue_dispatch.test.cjssuite thoroughly covers policy-shape rejections (foreign effect scope, mismatched proposals, duplicate installs) but has no case for a malicious pool name. Given this PR's own framing — "Trusted controls masked...failures as unclassified errors" — a__proto__pool name would currently escape detection with a confusing crash rather than a cleanwork_queue_pool_not_installed/ledger_invaliderror.
Positive Highlights
- ✅ Clear separation of the new
PoolInstalloperation fromPolicy, with matching validation in both the JS (work_queue_replay.cjs) and Go (pkg/workqueue/replay.go) replay engines. - ✅ Good safety invariants already enforced and tested: epoch preservation, no mutation of existing pools, repository-scoped
effect_scope/no fixedprincipalon auto-installed profiles. - ✅ Error code renaming (
work_queue_profile_not_approved→work_queue_pool_not_installedfor the missing-pool case) is a real diagnostic improvement matching the PR's stated goal, and the correspondingwork_queue_logging.cjsexplanations are clear and actionable.
Note: the pre-fetched diff (1750 lines) appeared complete for all 38 changed files, so no files were skipped for size reasons.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~99 AIC · ⌖ ~16 AIC · ⊞ 10.2K
Comment /matt to run again
| const installedPolicy = state.policy; | ||
| if (!installedPolicy || installedPolicy.authorization !== "aw" || Object.hasOwn(installedPolicy.pools, operation.pool)) throw queueError("policy_invalid", "only new pools can be installed into an AW-managed Policy"); | ||
| const policy = structuredClone(installedPolicy); | ||
| policy.pools[operation.pool] = structuredClone(operation.config); |
There was a problem hiding this comment.
[/diagnosing-bugs] policy.pools[operation.pool] = structuredClone(operation.config) writes to a plain object using an attacker-influenced key. identity() rejects empty/control-char strings but allows __proto__, constructor, or prototype, so a crafted PoolInstall operation can pollute Object.prototype here.
💡 Suggested fix
Use a null-prototype pools map (or reject dangerous keys explicitly) before assignment, matching the guard already needed in work_queue_pool_install.cjs:
if (["__proto__", "constructor", "prototype"].includes(operation.pool)) throw queueError("policy_invalid", "invalid pool name");
// or construct policy.pools with Object.create(null) at genesisThis is the root cause GitHub Advanced Security already flagged on this line — the comment was truncated, so I am restating it with a concrete repro: proposal.pools["__proto__"] resolves to Object.prototype (truthy), which also defeats the "pool is absent" check in poolFromProposal (work_queue_pool_install.cjs:22). No regression test currently exercises a __proto__/constructor pool name.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 94d6500: reserved pool keys are rejected before access, and replay uses Object.defineProperty instead of the prototype-sensitive assignment. JS and Go regression tests cover malicious pool identities.
| for (const [name, pool] of Object.entries(state.policy.pools)) { | ||
| if (canonical(proposal.pools[name]) !== canonical(pool)) throw queueError("work_queue_policy_proposal_mismatch", "automatic pool installation cannot change an installed pool"); | ||
| } | ||
| const pool = proposal.pools[poolName]; |
There was a problem hiding this comment.
[/diagnosing-bugs] proposal.pools[poolName] is a bracket lookup on a plain object. If poolName is __proto__, this resolves to Object.prototype (truthy), so the if (!pool) throw ... missing-pool guard on the next line is bypassed and pool.profiles is undefined, producing a confusing downstream crash instead of the intended work_queue_pool_not_installed error.
💡 Suggested fix
const pool = Object.hasOwn(proposal.pools, poolName) ? proposal.pools[poolName] : undefined;
if (!pool) throw queueError("work_queue_pool_not_installed", "the requested pool is absent from the compiled queue policy");Same pattern applies to state.policy.pools[poolName] at line 71 and the Object.entries(state.policy.pools) loop at line 19 — entries() is safe, but any future bracket access on these maps should use Object.hasOwn first.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 94d6500. Proposal lookup now requires an own pool property, and reserved pool identities are rejected before any map access. Missing proposals retain the classified missing-pool diagnostic.
| @@ -1908,6 +1909,110 @@ describe("native queue launch fencing and conservative recovery", () => { | |||
| } | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
[/tdd] Good coverage for the happy-path auto-install and the policy-mutation rejection, but there is no test exercising a malicious/edge-case pool name (__proto__, constructor, empty after normalization, very long UTF-8). Given the prototype-pollution risk flagged on work_queue_replay.cjs:1238 and work_queue_pool_install.cjs:22, a regression test here (or in work_queue_replay.test.cjs) would catch the real vulnerability class, not just the policy-shape checks already covered.
💡 Suggested test
it("rejects pool installation with a __proto__ pool name", async () => {
const { fixture, options } = setup({ granted: false, configurePolicy: policy => { policy.authorization = "aw"; } });
const proposal = structuredClone(fixture.policy);
proposal.pools.__proto__ = structuredClone(proposal.pools.default);
const trusted = { ...fixture.dispatcher, authenticated: true, roles: ["dispatcher"] };
await expect(installWorkQueuePools({ ...options, policyProposal: proposal }, trusted, ["__proto__"]))
.rejects.toThrow();
expect(Object.prototype).not.toHaveProperty("profiles");
});@copilot please address this.
There was a problem hiding this comment.
Added regression coverage in 94d6500 for proto, constructor, prototype, empty identities, oversized UTF-8 names, and valid 256-byte ASCII/UTF-8 names. Tests cover both helper publication and raw replay, including preservation of existing state on rejection.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved runtime, installation, recovery, and conformance failures prevent reliable queue operation.
8 open issues
What changed in this PR
Extends gh-aw’s native work queue with additive first-use pool installation and actionable routing diagnostics, supported by Go and JavaScript replay implementations.
Changes:
- Add
PoolInstallrequests, schemas, authorization, and epoch-preserving replay. - Integrate pool installation into submission and dispatch, with clearer routing diagnostics.
- Expand regression fixtures, formal models, and operator documentation.
| File | Description |
|---|---|
specs/work-queue/transactions.tsp |
Define pool-install operations and request kind. |
specs/work-queue/README.md |
Document additive installation and model checks. |
specs/work-queue/QueuePoolInstall.tla |
Model installation safety and authority boundaries. |
specs/work-queue/PoolInstallUnauthorized.cfg |
Check unauthorized publication is blocked. |
specs/work-queue/PoolInstallProducerWitness.cfg |
Demonstrate authorized producer installation. |
specs/work-queue/PoolInstallInvalidRoute.cfg |
Check immutable-route requirements. |
specs/work-queue/PoolInstallInvalidProposal.cfg |
Check existing Policy preservation. |
specs/work-queue/PoolInstallInvalidPool.cfg |
Check invalid pool rejection. |
specs/work-queue/PoolInstall.cfg |
Configure the valid installation model. |
specs/work-queue/fixtures/generate_prefix.py |
Generate AW-authorized pool-install fixture coverage. |
specs/work-queue/fixtures/canonical-prefix.json |
Record the expanded canonical transaction fixture. |
specs/work-queue/check.sh |
Register pool-install model checks. |
pkg/workqueue/types.go |
Add the pool-install operation type. |
pkg/workqueue/schema/RequestParameters.json |
Allow pool installation in request parameters. |
pkg/workqueue/schema/QueueRequest.json |
Validate pool-install requests. |
pkg/workqueue/schema/QueueOperation.json |
Include pool installation in operation validation. |
pkg/workqueue/schema/QueueCommit.json |
Accept pool-install transactions. |
pkg/workqueue/schema/PoolInstallOperation.json |
Define standalone pool-install validation. |
pkg/workqueue/schema/OperationsParameters.json |
Extend operation-based parameters. |
pkg/workqueue/request.go |
Authorize pool-install request roles. |
pkg/workqueue/replay.go |
Replay additive pools while preserving Policy state. |
pkg/workqueue/replay_test.go |
Test preservation and unsafe installation rejection. |
pkg/workqueue/policy.go |
Add operation permissions and request matching. |
pkg/workqueue/policy_permitted_test.go |
Cover pool-install role permissions. |
pkg/workqueue/fixtures_test.go |
Expect thirteen shared operation kinds. |
pkg/workqueue/deployment.go |
Initialize deployments for selected pools. |
docs/src/content/docs/specs/work-queue-specification.md |
Specify additive installation constraints. |
docs/src/content/docs/reference/work-queue.md |
Explain first-use installation behavior. |
docs/src/content/docs/patterns/workqueue-ops.md |
Describe installation and administrator update boundaries. |
actions/setup/js/work_queue_replay.cjs |
Validate and replay pool-install operations. |
actions/setup/js/work_queue_pool_install.cjs |
Verify proposals and publish additive installations. |
actions/setup/js/work_queue_policy.cjs |
Authorize JavaScript pool-install requests. |
actions/setup/js/work_queue_logging.test.cjs |
Cover routing diagnostics and data privacy. |
actions/setup/js/work_queue_logging.cjs |
Explain missing pools and unapproved routes. |
actions/setup/js/work_queue_dispatch.test.cjs |
Exercise installation and preservation behavior. |
actions/setup/js/work_queue_dispatch.cjs |
Install missing pools before new queue requests. |
actions/setup/js/work_queue_deployment.cjs |
Preserve existing deployments during pool initialization. |
.github/aw/work-queue.md |
Update internal queue-operation guidance. |
Review effort: Max
| const { verifyWorkerRoute } = require("./work_queue_provisioning.cjs"); | ||
| const { futurePolicy } = require("./work_queue_deployment.cjs"); | ||
| const { approvedWorkerProfiles, synchronizeDeployments } = require("./work_queue_deployment_control.cjs"); | ||
| const { installWorkQueuePools } = require("./work_queue_pool_install.cjs"); |
There was a problem hiding this comment.
Fixed in 94d6500: the module is included in both setup.sh deployment lists and SessionParserSources. Added explicit packaging regressions; setup bundle dependency tests pass.
| const stableId = requestIdForIntent(trustedContext, message.intent_id); | ||
| const prior = latest.projection.requests.get(stableId); | ||
| if (!prior) { | ||
| latest = (await installWorkQueuePools(configured, trustedContext, [requested.pool])) || latest; |
There was a problem hiding this comment.
Retained automatic installation as requested by the maintainer. 94d6500 reconciles the specification and ADR with that scope: producer/dispatcher first use may add a pool without quiescence, using only existing approved worker authority. Changing existing pools or scheduling economics still requires an administrator's quiescent Policy update.
| for (const [name, pool] of Object.entries(state.policy.pools)) { | ||
| if (canonical(proposal.pools[name]) !== canonical(pool)) throw queueError("work_queue_policy_proposal_mismatch", "automatic pool installation cannot change an installed pool"); | ||
| } |
There was a problem hiding this comment.
Fixed in 94d6500. Proposal comparison uses stable worker boundaries rather than caller ref/contract, preserves installed entries, and copies an exact available approved profile for the new pool. Tests cover different bootstrap/caller revisions and subsequent verified deployment synchronization.
| for (const profile of Object.values(pool.profiles)) { | ||
| const name = profile.workflow.replace(/^\.github\/workflows\//, "").replace(/\.(?:lock\.yml|yml|yaml)$/, ""); | ||
| if (profile.effect_scope !== state.repository || Object.hasOwn(profile, "principal") || !workflows.has(name) || !contexts.has(name)) | ||
| throw queueError("work_queue_worker_not_compiler_approved", "new pool routes must be compiler-approved and preserve repository-scoped AW authority"); |
There was a problem hiding this comment.
Fixed in 94d6500. Installation resolves profiles against repository-wide installed/registered worker authority and verifies the selected routes, rather than requiring every profile in the caller-local target list. Submission and dispatch still enforce caller-local targets. Added a caller approved for only one of multiple installed workers.
| state.policy = policy; | ||
| initializeDeployments(state, [operation.pool]); |
There was a problem hiding this comment.
Fixed in 94d6500. Both consumers reconstruct Policy through PoolInstall at the accepted request's ledger position. Checkpoints retain complete Policy/PoolInstall history in JS and Go, while existing checkpoint hashes remain recoverable. Tests cover accepted submit retry and claim-scoped dispatch_next readback after later epochs and repeated checkpoints.
| "mode": "weighted-priority", "class_weights": [8, 4, 2, 1, 1], | ||
| "accounting_weights": {"": 1}, | ||
| "producers": {"1001": {"pools": ["default"], "priorities": [1, 2, 3, 4, 5], "fairness_keys": [""]}}, | ||
| "authorization": "aw", "producers": {}, |
There was a problem hiding this comment.
Fixed in 94d6500: the worker-child generator removes inherited authorization before adding entitlement-based cases. Regeneration matches the committed fixture without a JSON diff; worker-child JS/Go tests pass.
| append("dispatcher", "pool-install", "pool_install", 5300, [{ | ||
| "kind": "PoolInstall", "pool": "smoke", "config": new_pool, | ||
| }], reason="grants_paused") |
There was a problem hiding this comment.
Fixed in 94d6500: the JavaScript conformance assertion and test title now expect thirteen operation kinds. Also corrected the canonical fixture's later Policy so its PoolInstall inherits already authorized worker authority.
|
|
||
| for (const [poolName, pool] of candidates) { | ||
| const operation = { kind: "PoolInstall", pool: poolName, config: pool }; | ||
| const request = requestForIntent(trustedContext, `pool-install:${poolName}`, "pool_install", { operations: [operation] }); |
There was a problem hiding this comment.
Fixed in 94d6500 using pool-install:${digest(poolName)}. Regression tests successfully install 256-byte ASCII and UTF-8 names, keep request IDs bounded, and verify retry idempotence.
|
@copilot address the following outstanding work in one pass:
|
…e-issue Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…recovery Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Merged latest main with make merge-main in c34dfee and pushed the fixes in 94d6500. All 14 listed review threads have a direct reply. First-use installation remains non-administrator, as requested; the specification/ADR now explicitly describe that scope, and replay prevents introducing new worker authority. Validation: full Go workqueue/setup suites, final impacted Go tests, 312 impacted JS tests, documentation checks, seven TLC configurations, and secret scanning passed. The final gate remains blocked by unchanged custom-linter findings in checkpoint.go. Two negative-control benchmark timeouts reproduce with the pre-fix runtime. CodeQL timed out and instructed against retrying; read-only security/code reviews found no remaining high-confidence issues. No CI was triggered. The available tools support replies but not thread resolution or PR-body edits, so the 14 threads still need resolution and the outdated administrator-only PR description needs reconciliation. Human review and CI verification of the pushed HEAD remain required. |
| validatePolicy(policy); | ||
| if (Object.values(operation.config.profiles).some(profile => profile.effect_scope !== state.repository || Object.hasOwn(profile, "principal"))) | ||
| throw queueError("policy_invalid", "automatically installed AW pools must preserve repository-scoped authority"); | ||
| for (const [name, profile] of Object.entries(operation.config.profiles)) | ||
| if (!approvedPoolProfile(state, name, profile)) throw queueError("policy_invalid", "pool profiles must match installed or registered worker revision authority"); |
| const stableId = requestIdForIntent(trustedContext, message.intent_id); | ||
| const prior = latest.projection.requests.get(stableId); | ||
| if (!prior) { | ||
| latest = (await installWorkQueuePools(configured, trustedContext, [requested.pool])) || latest; |
|
@copilot address the following outstanding work in one pass:
General fix: during replay of Best concrete fix in this file:
This preserves functionality for legitimate installs while preventing non-admin widening of repository authority during replay. - #71495 (comment) The safest minimal fix is to remove implicit pool installation during dispatch intent handling and fail closed when the requested pool is missing. That preserves normal dispatch behavior for existing pools while preventing this endpoint from autonomously creating new pools from untrusted/semi-trusted input. Best single fix in this file/region:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 94d6500
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 8786c98: JS and Go replay now reject PoolInstall repository allowlists that exceed installed authority, and ordinary submit/dispatch intents reject missing pools before deployment synchronization. Updated the bounded model and recovery docs as well. |
|
@copilot address the following outstanding work in one pass:
General fix: during replay of Best concrete fix in this file:
This preserves functionality for legitimate installs while preventing non-admin widening of repository authority during replay. - #71495 (comment) The safest minimal fix is to remove implicit pool installation during dispatch intent handling and fail closed when the requested pool is missing. Best single fix in this file/region:
No new imports or dependencies are needed. - #71495 (comment) Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 8786c98
|
The replay guards already reject |
|
🎉 This pull request is included in a new release. Release: |
The smoke dispatcher failed because
smoke-work-queuewas absent from the installed Policy, despite appearing inaw.json. Trusted controls masked both submission and dispatch failures as unclassified errors.work_queue_pool_not_installedfor missing pools.