Repository navigation
Prevent shell-permission stops in scheduled agents - #65401
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. Testing CLI write-intent availability for PR review workflow.
|
|
✅ 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.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR (#65401). Only .github/skills/agentic-workflows/SKILL.md and workflow markdown/lock.yml files (code-scanning-fixer, daily-formal-spec-verifier, daily-spdd-spec-planner) were changed. Test Quality Sentinel skipped.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Patch measurement is unreliable, SPDD compound reads remain insufficiently addressed, and an unrelated routing regression was introduced.
Review effort: Balanced
Findings: 3
Open (3)
What changed in this PR
Updates scheduled-agent prompts and allowlists to reduce shell permission denials.
Changes:
- Adds exact cache-read, discovery, and patch-size commands.
- Guides agents toward built-in tools and safe outputs.
- Regenerates corresponding workflow lock files.
| File | Description |
|---|---|
.github/workflows/daily-spdd-spec-planner.md |
Restricts cache reads and discovery commands. |
.github/workflows/daily-spdd-spec-planner.lock.yml |
Regenerates the SPDD workflow. |
.github/workflows/daily-formal-spec-verifier.md |
Clarifies standalone discovery and file reads. |
.github/workflows/daily-formal-spec-verifier.lock.yml |
Regenerates the verifier workflow. |
.github/workflows/code-scanning-fixer.md |
Restricts shell usage and adds patch measurement. |
.github/workflows/code-scanning-fixer.lock.yml |
Regenerates the fixer workflow. |
.github/skills/agentic-workflows/SKILL.md |
Reorders references and removes work-queue routing. |
| @@ -106,7 +106,6 @@ After loading the matching workflow prompt or skill, follow it directly: | |||
| - Render compact markdown charts: `.github/aw/asciicharts.md` | |||
| - Map CLI commands to MCP usage: `.github/aw/cli-commands.md` | |||
| - Choose workflow architecture and patterns: `.github/aw/patterns.md` | |||
| Before emitting `create-pull-request`, preflight the complete generated patch, including binary removals: | ||
| - Measure `git diff --binary --no-ext-diff` in bytes. The default `create-pull-request` safe-output limit is 4,096 KB (4,194,304 bytes). | ||
| - Measure the patch in bytes with the explicitly allowed single command `git diff --binary --no-ext-diff | wc -c`. The default `create-pull-request` safe-output limit is 4,096 KB (4,194,304 bytes). |
| Use the allowed shell commands above or built-in file inspection tools only for read-only analysis. Do not modify repository files. | ||
|
|
||
| **File Discovery**: Use the allowed bash command `find specs docs scratchpad -type f -name "*.md"` to list spec files. Do not use the `glob` tool on the workspace root directory — it will be denied and consume tool-denial budget. | ||
| **File Discovery**: Run exactly the allowed bash command `find specs docs scratchpad -type f -name "*.md"` to list spec files, then select relevant paths from its output. Do not append `grep`, `sort`, or other shell commands, chain it with other commands, or use the `glob` tool on the workspace root directory — those calls will be denied and consume tool-denial budget. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
These edits are aimed at stopping scheduled agents from exhausting their shell and tool budget, but two of the new prompt changes still leave the agent with contradictory execution rules in the hot path.
Blocking themes
- code-scanning-fixer now says to create the PR directly after edits even though step 7 requires a patch-size preflight first.
- daily-spdd-spec-planner now forbids separate shell write preflights while the same section still requires one.
Those conflicts are likely to recreate the same kind of stalled runs this PR is trying to eliminate.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 63.1 AIC · ⌖ 7.18 AIC · ⊞ 20.2K
Comment /review to run again
| - Create pull request: emit a `create-pull-request` safe output after edits | ||
| - Run each allowed shell command separately; do not chain commands with `;` or `&&`, use heredocs or redirection to write files, or wrap commands in `python3`. Do not run builds or formatters through the restricted shell. | ||
| - Write cache records with the `edit` tool, not shell redirection. | ||
| - Create pull request: emit a `create-pull-request` safe output directly after edits; do not invoke `safeoutputs` through bash |
There was a problem hiding this comment.
Saying to emit create-pull-request directly after edits conflicts with step 7's required patch-size preflight, so the agent can skip the size check and recreate the same safe-output failure path this PR is trying to close.
Why this blocks the fix path
The later section is explicit that the agent must measure the patch before creating the PR. This new sentence introduces a stronger local instruction that says to create the PR immediately after editing, which gives the agent two incompatible orderings for the same action.
Please change this to say the PR safe output should run only after the step-7 patch-size preflight passes, while keeping the rule against invoking safeoutputs through bash.
| - If reading `rotation.json` returns a miss because the file is absent, initialize from index 0 | ||
| - If `rotation.json` exists but cannot be read/written, do not reinitialize; report the setup error so existing rotation state is preserved | ||
| - Persist rotation state using the `write` tool at that exact path (do not use shell write commands for cache updates) | ||
| - Persist rotation state using the `write` tool at that exact path; treat a write failure as a setup error (do not use shell write commands or separate shell write preflights) |
There was a problem hiding this comment.
The new no-separate-write-preflight rule now contradicts the still-required write preflight two bullets above, so this workflow is again giving the agent mutually exclusive instructions right in the rotation setup path.
Why this is risky
Right above this line the agent is told to run a write preflight in /tmp/gh-aw/cache-memory/spdd-daily/, but this new sentence says not to use separate shell write preflights. That means a compliant implementation can be rejected either way, which is exactly the sort of ambiguity that burns denial budget and stalls the run.
Pick one contract and make the whole section consistent. If the intent is to treat the actual write failure as the setup check, remove the earlier preflight requirement and keep the explicit write-failure handling.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /grill-with-docs (prompt-only changes to scheduled-agent workflows; no app code). Compiled all three workflows locally to confirm the .lock.yml diffs match source — they do.
📋 Key Themes & Highlights
Key Themes
- Fragile literal-compound allowlist entry:
code-scanning-fixer.md's new bash entry"git diff --binary --no-ext-diff | wc -c"relies on Copilot CLI's prefix matching treating the whole piped string as one literal token. That's a valid but undocumented use of the mechanism — any reformatting of the command (spacing, flag order) will silently fall back to being denied, recreating the exact bug this PR fixes. - Inconsistent guidance phrasing across the three workflows: each workflow now explains "don't chain shell commands" in its own words. Since this PR exists specifically to fix a repeated cross-workflow failure pattern, converging on shared wording (or a shared snippet) would make the fix easier to audit and reuse next time.
Positive Highlights
- ✅ Root cause correctly diagnosed: tool-denial budget exhaustion from compound/chained shell commands, not a missing-permission bug.
- ✅ Verified by recompiling all three workflows with the PR's source changes — generated
.lock.ymlfiles match the committed ones exactly, so the allowlist entries are faithfully reflected in the compiled Copilot CLI--allow-toolarguments. - ✅ Clear, workflow-specific instructions steering agents toward
edit/safe-outputs instead of shell redirection for cache writes, which is the safer pattern already used elsewhere in the codebase.
No blocking issues — the two inline notes above are suggestions for robustness/consistency, not correctness blockers.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 153.6 AIC · ⌖ 14.7 AIC · ⊞ 10K
Comment /matt to run again
| tools: | ||
| cli-proxy: true | ||
| bash: ["cat:*", "git diff:*", "git restore:*", "git status:*", grep, "head:*", jq, ls, "sed:*", tail, wc] | ||
| bash: ["cat:*", "git diff:*", "git diff --binary --no-ext-diff | wc -c", "git restore:*", "git status:*", grep, "head:*", jq, ls, "sed:*", tail, wc] |
There was a problem hiding this comment.
[/codebase-design] The new allowlist entry "git diff --binary --no-ext-diff | wc -c" works only because Copilot CLI does literal prefix matching on the entire string, pipe included — it is not a generic "pipes are allowed" mechanism, it's one hard-coded exact string.
💡 Why this is fragile
If the agent (or a future edit) reformats the command with different spacing, quoting, or flag order (e.g. git diff --no-ext-diff --binary | wc -c), the prefix match fails and the denial-budget problem this PR fixes recurs silently. This repo's own sanitizeCopilotShellCommand comment documents prefix matching for simple commands like shell(jq), but a multi-token piped command baked into the allowlist as one literal string is a different, more brittle use of that mechanism and isn't called out anywhere as a supported pattern.
Consider instead keeping bash command granularity (git diff:*, wc) and only using narrower instruction text (as already done) to tell the agent to chain them in one invocation, rather than encoding the full compound command as a single allowlist literal. If this literal-compound-command approach is intentional, it's worth a one-line comment in the workflow or a doc note so future editors don't "simplify" it back to separate tokens and silently break the fix.
@copilot please address this.
| - Continue from next file on the next run | ||
| - Run a write preflight in `/tmp/gh-aw/cache-memory/spdd-daily/` and treat any permission/write failure as a setup error (do not continue) | ||
| - If reading `rotation.json` returns a miss, confirm the file is truly absent before initializing from index 0 | ||
| - Read rotation state with the standalone allowed command `cat /tmp/gh-aw/cache-memory/spdd-daily/rotation.json`; do not combine it with `mkdir`, `ls`, `touch`, `cd`, or other shell commands |
There was a problem hiding this comment.
[/grill-with-docs] Good fix for the rotation-read denial, but the vocabulary for "don't combine shell commands" is now worded differently in all three touched workflows (run each allowed shell command separately... here vs Run exactly the allowed bash command...then select vs the formal-spec-verifier's Run it separately from the cache read). @copilot please address this.
💡 Suggestion
Since this PR is explicitly about a recurring cross-workflow failure mode (compound/chained shell commands exhausting the tool-denial budget), it would be worth converging on one canonical phrasing — e.g. a shared snippet in shared/ imported by all three, or at minimum identical wording — so the next scheduled-agent fix doesn't have to rediscover and re-word this guidance a fourth time. Right now each workflow explains the same constraint with its own ad hoc phrasing, which makes it harder to grep for or audit this class of fix across workflows later.
There was a problem hiding this comment.
Impeccable Skills Review — no actionable issues
This PR is a workflow-configuration/documentation change (no UI/application code), so standard Impeccable UI modes (audit, critique, harden, distill, extract, clarify) don't map cleanly onto it. I verified the changes directly instead, focusing on correctness and consistency:
.github/skills/agentic-workflows/SKILL.md: moved thework-queue.mdbullet to alphabetical order and removed a duplicate "Orchestrate durable work..." entry —work-queue.mdstill exists and is referenced correctly elsewhere.code-scanning-fixer.md: new literal bash allowlist entrygit diff --binary --no-ext-diff | wc -cmatches the exact command referenced in the "Preflight" instructions; recompiling the workflow reproduces the committed.lock.ymlbyte-for-byte.daily-formal-spec-verifier.md/daily-spdd-spec-planner.md: new standalonecat .../rotation.jsonbash entries match the updated prose instructing the agent not to chain cache reads with other shell commands; lock files are in sync with source.
No blocking or high-signal issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 181 AIC · ⌖ 13.1 AIC · ⊞ 8.1K
|
🎉 This pull request is included in a new release. Release: |
Three scheduled agents exhausted their tool-denial budget after issuing compound shell commands outside their allowlists.
editfor cache writes and safe outputs for PR creation. Adds one exact command for patch-size measurement:git diff --binary --no-ext-diff | wc -c.