Repository navigation
Fix reviewer publication recovery without expanding permissions - #69474
Conversation
Remove availability-probe dependencies, keep optional reads within the existing tool contract, and document bounded incomplete reporting before the denial budget is exhausted. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ 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.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ 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 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: 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.
|
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 patch still leaves the reviewer with a broken failure path and a weak contract test, so it does not actually guarantee review publication under denial conditions.
Blocking themes
- The new fallback instructs the agent to call
report_incomplete, but that tool is not enabled for this workflow, so the documented recovery path can fail in the exact scenario it is supposed to save. - The new test for forbidden command probes uses a literal
NotContainscheck that does not match real YAML command entries, so CI will not reliably catch the regression this PR claims to prevent. - Static review only: I did not execute repo code or tests.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 37.4 AIC · ⌖ 5.94 AIC · ⊞ 12.4K
Comment /review to run again
| If a publication command is denied, distinguish an unavailable CLI from unallowed commands or file writes in the pipeline. Correct the command to the stdout-only `printf` pipeline above without widening permissions. Make at most one corrected publication attempt, then stop probing and emit `report_incomplete` through the available safe-output transport with the concrete blocker, before exhausting the five-denial budget; do not call `noop` or claim the review was published. With this workflow's CLI transport, use: | ||
|
|
||
| ```bash | ||
| printf '%s' '{"reason":"Review publication blocked: describe the concrete tool denial or CLI error; no review was published."}' | safeoutputs report_incomplete . |
There was a problem hiding this comment.
This recovery path points the reviewer at safeoutputs report_incomplete, but that tool is not enabled for this workflow, so the first publication failure still strands the run without a terminal safe output.
💡 Why this is still broken
The safe-output contract for this job exposes create_pull_request_review_comment, submit_pull_request_review, create_check_run, missing_tool, missing_data, and noop—not report_incomplete. That means the new fallback command will be denied in exactly the failure path where we need it most, so the reviewer still cannot emit the required terminal action.
Suggested fix:
Either add `report_incomplete` to the workflow's enabled safe-output tools, or change the documented recovery path to use one of the tools that is actually available here.
Right now this patch teaches the agent to take a recovery action that the sandbox cannot perform.
|
|
||
| text := string(content) | ||
| assert.NotContains(t, text, "command -v safeoutputs", "Publication must not depend on an unallowed availability probe") | ||
| assert.NotContains(t, text, " - command:*\n") |
There was a problem hiding this comment.
This assertion does not protect the contract you describe: assert.NotContains(..., " - command:*\n") only rejects that exact literal text, so a real YAML - command: entry still passes and CI gives false confidence.
💡 Use a pattern check instead of an impossible literal
The regression here is line-pattern based, not exact-string based. As written, the test only fails if someone literally pastes the asterisk into the workflow, which means the forbidden command stanza can come back without tripping this guard.
Suggested fix:
assert.NotRegexp(t, regexp.MustCompile(`(?m)^\s*- command:`), text)Any equivalent line-based matcher would work; the important part is validating actual YAML command entries instead of a single impossible literal.
There was a problem hiding this comment.
🟡 Changes recommended
A successful corrected publication can still trigger incomplete reporting and fail the run.
1 open finding
What changed in this PR
Updates the PR Code Quality Reviewer’s publication and recovery guidance without expanding permissions.
Changes:
- Directs stdout-only publication with bounded recovery.
- Specifies allowed file checks and static-only analysis.
- Adds regression assertions and refreshes the generated prompt hash.
| File | Description |
|---|---|
| pkg/cli/pr_code_quality_reviewer_workflow_contract_test.go | Adds publication, recovery, and tool-use assertions. |
| .github/workflows/pr-code-quality-reviewer.md | Clarifies publication pipelines, recovery, and analysis limits. |
| .github/workflows/pr-code-quality-reviewer.lock.yml | Updates only the prompt body hash. |
🧠 Review effort: Max
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| ``` | ||
|
|
||
| If a publication command is denied, distinguish an unavailable CLI from unallowed commands or file writes in the pipeline. Correct the command to the stdout-only `printf` pipeline above without widening permissions. If the corrected command still cannot run, emit `report_incomplete` through the available safe-output transport with the concrete blocker; do not call `noop` or claim the review was published. | ||
| If a publication command is denied, distinguish an unavailable CLI from unallowed commands or file writes in the pipeline. Correct the command to the stdout-only `printf` pipeline above without widening permissions. Make at most one corrected publication attempt, then stop probing and emit `report_incomplete` through the available safe-output transport with the concrete blocker, before exhausting the five-denial budget; do not call `noop` or claim the review was published. With this workflow's CLI transport, use: |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd (per pr-triage). This is a well-scoped, well-tested fix for a real bug: the reviewer workflow prepended an unauthorized command -v safeoutputs availability probe before publishing, burning denials against the default 5-denial budget (confirmed via pkg/constants/engine_constants.go DefaultMaxToolDenials = 5) before any terminal safe output could be emitted.
📋 Key Themes & Highlights
Root cause vs. symptom (/diagnosing-bugs)
- The fix addresses the actual root cause, not just the symptom: it replaces ambiguous instructions ("if reviewed before", "at least 3 bytes") that implicitly invited shell probing (
stat,test -f, command substitution) with explicit, allowed-tool instructions (globfor existence checks, exactwc -cinvocation). - Verified
globis a neutral builtin tool (COPILOT_SDK_NEUTRAL_BUILTIN_TOOLSinactions/setup/js/copilot_sdk_tool_config.cjs) that bypasses thetools.bashallowlist entirely — so routing the optional-file check throughglobis technically sound and doesn't require wideningtools.bash. - The explicit prohibition list (
stat,awk, shell tests, command substitution,|| true) directly matches what's absent from the workflow's actualtools.bashallowlist (cat, date, echo, find, git:*, grep, head, ls, nl, printf, pwd, sed, sort, safeoutputs, tail, uniq, wc, yq) — the fix is consistent with the real tool surface, not just aspirational. - The new bounded-recovery contract ("at most one corrected publication attempt... before exhausting the five-denial budget") converts an unbounded retry loop into a budgeted one, which is the correct fix shape for a denial-budget exhaustion bug.
Regression coverage (/tdd)
TestPRCodeQualityReviewerWorkflowToolContractand the extendedTestPRCodeQualityReviewerWorkflowPublicationContractassertions directly encode the new contract (absence of the probe, presence of theglob/wc -cinstructions, the bounded-recovery language) — these are genuine regression tests, not just restating implementation.- Left one minor inline nit: the
NotContains(" - command:*\n")assertion (line 70) would benefit from a short comment explaining its intent (guarding against re-adding thecommandbuiltin, which would re-enable the probe) — non-blocking.
Scope discipline
- Confirmed via the compiled lock file that permissions/budgets are genuinely unchanged (
GH_AW_MAX_TOOL_DENIALS: 5unchanged) — the PR's claim of not expanding permissions holds up.
No security, correctness, or architectural concerns found. Approving.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 205.9 AIC · ⌖ 16.3 AIC · ⊞ 10.1K
Comment /matt to run again
|
|
||
| text := string(content) | ||
| assert.NotContains(t, text, "command -v safeoutputs", "Publication must not depend on an unallowed availability probe") | ||
| assert.NotContains(t, text, " - command:*\n") |
There was a problem hiding this comment.
[/tdd] This NotContains(" - command:*\n") guard is a good regression test but its intent (prevent re-adding the command builtin to tools.bash, which would re-enable the command -v safeoutputs probe that caused the original bug) isn't obvious from the assertion alone.
💡 Suggestion
Add a short inline comment above the assertion, e.g.:
// Guard against re-introducing the `command` builtin, which would let the
// workflow probe tool availability via `command -v` again (the original bug).
assert.NotContains(t, text, " - command:*\n")This keeps the test self-documenting per /tdd (test names/comments should read as specifications) without changing behavior.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
Summary
The PR Code Quality Reviewer exhausted its five-denial budget after prefixing review publication with an unallowed availability probe, leaving no terminal safe output.
Keep the existing tool allowlist unchanged and instruct the reviewer to invoke the stdout-only
printf | safeoutputspublication pipelines directly, without availability probes. Specify allowed optional-file checks, keep analysis static instead of attempting unallowed test runners, and bound recovery to one corrected publication attempt followed by an explicitreport_incompletepipeline.Add regression assertions for the publication/recovery contract and regenerate the workflow lock. The generated lock changes only the prompt hash; permissions, credentials, output budgets, and runtime controls remain unchanged.
Validation
make fmtmake recompile(344 workflows compiled)go test ./pkg/cli -run '^TestPRCodeQualityReviewerWorkflow' -count=1make agent-report-progress(impacted tests, lint, schema checks, and workflow drift passed)gh aw compile pr-code-quality-reviewer --dry-run --schedule-seed github/gh-awpassed source/model checks and shellcheck with the existinggh-aw-detectionexperimental notice; optional scanners were not run.No workflow was dispatched. Hosted publication remains unverified.