Repository navigation
Fix work-queue tool transport and smoke failure detection - #65360
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. 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.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
One small simplification found.
net: -1 lines possible.
Generated by ✂️ Ponytail Reviewer for #65360 · codex · gpt56 · 10.4 AIC · ⌖ 6.45 AIC · ⊞ 13.4K
Comment /ponytail to run again
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.
|
There was a problem hiding this comment.
No blocking issues found in the changed lines. The CLI mount change, MCP content wrapping, finish-intent permission handling, and smoke-verification coverage look internally consistent.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 42.4 AIC · ⌖ 7.06 AIC · ⊞ 20.2K
Comment /review to run again
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The changes are coherent, well-tested, and corroborated by two successful audited smoke runs.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes work-queue CLI discovery, MCP responses, finish-intent collection, and smoke-test failure detection.
Changes:
- Mounts and documents the work-queue CLI transport.
- Returns valid MCP text content and cross-user-readable finish intents.
- Adds regression coverage and post-processing smoke verification.
| File | Description |
|---|---|
specs/work-queue/README.md |
Documents CLI invocation and transport behavior. |
pkg/workflow/work_queue_compilation_integration_test.go |
Tests smoke verification outcomes. |
pkg/workflow/mcp_cli_mount.go |
Advertises and allowlists the work-queue CLI. |
pkg/workflow/mcp_cli_mount_test.go |
Covers CLI selection across engines. |
actions/setup/js/work_queue_mcp_server.test.cjs |
Tests MCP wire responses and file failures. |
actions/setup/js/work_queue_mcp_server.cjs |
Serializes responses and adjusts intent permissions. |
.github/workflows/smoke-work-queue.md |
Adds final smoke-result verification. |
.github/workflows/smoke-work-queue.lock.yml |
Regenerates the compiled workflow. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — one minor clarity nit on the finish-intent permission fix; no blocking issues found.
📋 Key Themes & Highlights
Key Themes
- Permission fix verified: confirmed by local repro that
fs.chmodSync(not theappendFileSyncmodeoption) is what actually fixes cross-user readability for a pre-existing owner-only finish-intent file. Flagged as a readability nit only — left a suggestion to avoid a future accidental revert of the real fix. - MCP transport fix is correct: wrapping tool responses in
{content:[{type:"text",...}]}matchesmcp_server_core.cjs's expectation that handlers returncontent; the previous plain-object returns were silently discarded, matching the PR's stated root cause. - Smoke verification job:
verify_smoke_resultis wired withneeds: [agent, safe_outputs]and asserts the full success surface (nocreate_issue, a completed finish intent, and anoop), withTestWorkQueueSmokeVerificationexercising 6 success/failure permutations by extracting and running the shell script directly — solid regression coverage for a bash-in-YAML check that's otherwise easy to silently break. - CLI mount selection:
work-queueis hardcoded as a bare string in two call sites inmcp_cli_mount.gorather than a shared constant (unlikesafeoutputs/mcpscripts), but this matches the existing"qmd"precedent in the same switch, so it's a pre-existing pattern, not a new inconsistency.
Positive Highlights
- ✅ Good test coverage added across three layers: Go unit (
TestWorkQueueCLIMountSelection), Go integration (TestWorkQueueSmokeVerificationexecuting the actual generated bash), and JS unit/integration (stdio MCP transport round-trip, cross-user permission repair, and I/O failure reporting). - ✅ The PR body documents five live branch iterations that reproduced and resolved the failures, with two consecutive passing runs linked — strong evidence this was diagnosed end-to-end rather than patched blind.
- ✅ Error handling improvement:
createWorkQueueFinishToolnow wraps filesystem failures in a descriptiveErrorwithcause, replacing a previous silent failure mode, and this is covered by a dedicated test (vi.spyOn(fs, "appendFileSync")).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 108.6 AIC · ⌖ 13.6 AIC · ⊞ 10K
Comment /matt to run again
| fs.appendFileSync(outputPath, `${JSON.stringify({ outcome })}\n`, { encoding: "utf8", mode: 0o600 }); | ||
| try { | ||
| fs.mkdirSync(path.dirname(outputPath), { recursive: true }); | ||
| fs.appendFileSync(outputPath, `${JSON.stringify({ outcome })}\n`, { encoding: "utf8", mode: 0o644 }); |
There was a problem hiding this comment.
[/diagnosing-bugs] The mode: 0o644 option on appendFileSync only applies when the file is newly created — for a pre-existing 0o600 file it is silently ignored; the actual permission fix is fs.chmodSync on line 110.
💡 Why this matters
Verified locally: calling fs.appendFileSync(existingFile, data, {mode: 0o644}) on a file that already exists with mode 0600 leaves it at 0600 — Node only honors mode at file-creation time (O_CREAT), not on append to an existing descriptor. So this line's mode option is dead code for the exact scenario this PR is fixing (an existing owner-only intent file from an earlier run/user). The explicit fs.chmodSync(outputPath, 0o644) right after is doing all the real work.
Suggest dropping the mode: 0o644 from the appendFileSync call (or adding a short comment noting it only matters for first-write) to avoid a future reader believing both lines are necessary for the permission fix, which could lead to someone removing the chmodSync call as "redundant" and reintroducing this exact bug.
@copilot please address this.
There was a problem hiding this comment.
Impeccable Skills Review (Backend/No-UI change)
This PR touches backend Go/JS internals (MCP wire transport, file permissions, workflow compilation) with no UI surface, so Impeccable UI modes (audit/critique/harden/distill/extract/clarify) don't apply. Fell back to a direct correctness/security review of the diff, per the fallback instructions.
Verified:
go build ./...succeeds.go test ./pkg/workflow -run TestWorkQueueCLIMountSelection— pass (6/6 subtests).go test -tags=integration ./pkg/workflow -run TestWorkQueueSmokeVerification— pass (6/6 subtests) against the committedsmoke-work-queue.md/lock file.- Compiled
smoke-work-queue.mddirectly and confirmed the generatedverify_smoke_resultjob (needs: [agent, safe_outputs]) matches the diff.
Findings:
- The MCP
content: [{type: "text", text: JSON.stringify(...)}]wrapping inwork_queue_mcp_server.cjsmatches the established pattern used elsewhere (e.g.ledger_mcp_server.cjs) and correctly fixes a real bug:mcp_server_core.cjs'stools/callhandler only readshandlerResult.content, silently discarding plain-object tool returns. - The
0o644permission change plus explicitchmodSyncfor the finish-intent file is a reasonable, narrowly-scoped fix for cross-container-user artifact collection. The file only ever contains{"outcome": "completed"|"cancelled"}with no work/claim identifiers, so the wider read permission doesn't leak sensitive data, consistent with the PR description. - Wrapping the write in
try/catchand re-throwing now surfaces I/O failures instead of silently reportingrecorded: trueon failure — good reliability fix, and covered by a new regression test. isWorkQueueEnabled-gated CLI mounting inmcp_cli_mount.gofollows the existingqmd/GitHub CLI mounting pattern and is covered by a new table-driven test (TestWorkQueueCLIMountSelection).
No blocking issues found on the changed lines.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
codeload.github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "codeload.github.com"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 125.2 AIC · ⌖ 13.2 AIC · ⊞ 8.1K
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Why
Repeated
smoke-work-queueruns exposed failures hidden by successful workflow conclusions: the agent could not discover the queue CLI, tool responses were empty on the MCP wire, and container-owned finish intents could not be collected by the runner.Changes
work-queueCLI wrapper and include it in restricted shell allowlists when CLI transport is active. Clarify that queue tool names are subcommands, and thatsnapshot_shacan be null for an absent queue.noopoutput.Validation
Passed
make fmt,make fmt-cjs,make lint-cjs,make recompile, andmake agent-report-progress.Focused checks passed:
go test ./pkg/workflow -run 'MCPCLI|MountedCLI|WorkQueue' -count=1go test -tags=integration ./pkg/workflow -run '^TestWorkQueue(CompilationPhases|SmokeVerification)$' -count=1Five live branch iterations reproduced and resolved the failures. The final two runs passed consecutively on
bfb9c0a9f3: 37140377258 and 37140766550. Each audit confirms one queue read, one finish call, onenoop, and successful finish-artifact verification. The new verifier also correctly failed the intermediate run when the remaining runtime bugs were exercised.Coverage boundary: the durable queue branches were absent before and after the smoke loop, and no queue storage was mutated. Live verification covers the unassigned path; claimed-worker completion remains covered by local reconciliation tests.