[PR #375] [MERGED] fix(cursor-approve): allow the checkout axes the shell they were meant to have #376

New issue

Closed

opened 2026-10-26 22:39:11 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:39:11 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/375
Author: @mattmillerai
Created: 10/8/2026
Status: ✅ Merged
Merged: 10/8/2026
Merged by: @mattmillerai

Base: main ← Head: matt/axis-checkout-precomputed-diff

📝 Commits (3)

9f977b7 fix(cursor-approve): give checkout axes the git output instead of a shell they never had
220a7ab fix(cursor-approve): allow the checkout axes a shell instead of handing them a diff
ccf1100 fix(cursor-approve): remove PR-authored .cursorignore files before an axis runs

📊 Changes

3 files changed (+60 additions, -11 deletions)

View changed files

📝 .github/cursor-approve/tests/test_context_axes.py (+15 -0)
📝 .github/workflows/cursor-axis-base.yml (+35 -7)
📝 docs/callers/cursor-approve.md (+10 -4)

📄 Description

ELI-5

The correctness, conformance and completeness reviewers are told to read the PR's diff with git. Every shell command they try gets silently rejected, so they never see the diff and approve by guessing from files on disk. This PR gives them the shell they were always meant to have. The reviewer gets the merge-base range and decides how to review it. If the shell ever breaks again, the job now fails visibly instead of returning a blind verdict.

What was wrong

cursor-axis-base.yml writes .cursor/cli.json with allow: []. Unless no_shell is set, it also writes deny: []. cursor-agent --print has no one to approve a command, so it rejects any shell call that no allow rule matches. On checkout axes, every git diff and git log came back {"rejected": {..., "reason": ""}}. The agents fell back to Read and Grep and said so in their verdicts ("Shell/git was unavailable to me"). The caller docs promised the completeness axis "keeps a shell for git", which was never true.

Change

Explicit allow rule. When no_shell is false (the checkout axes), the config adds Shell(*) to allow. Business and design are unchanged: they get no shell, the existing deny list, and the existing check that fails the job if a shell call appears anyway.
Guards against a quiet regression:
- Every axis now fails if its transcript has no recognizable tool call. That check used to run only on no_shell axes.
- A checkout axis fails if every shell call it made was rejected, which is exactly this bug.
Docs and the input header describe the shell and the guards.
PR-authored ignore files are removed before the agent runs: .cursorignore and .cursorindexingignore are deleted alongside the PR's .cursor/ config, so a PR cannot hide its own changes from the read and search tools of the reviewer judging it.

Revised after review

The first version precomputed the diff into .git/ and pointed the agent at it. @huntcsg pointed out that a large diff can blow up the agent's context, and that with a shell the agent can choose its own approach: --stat first, read only the relevant hunks, or script a check over a mechanical change. The prompt already names the range (git diff <merge_base>...<head>), so this version drops the precompute.

Risk

This is the same unsandboxed shell over PR code that the cursor-review panel already accepts (see the cursor-review.yml header). The axis job has contents: read, a checkout with persist-credentials: false, and CURSOR_API_KEY in the environment. For completeness, the existing note about the Linear proxy token on a passwordless-sudo runner still applies, and the docs keep it.

Testing

python3 -m unittest discover -s .github/cursor-approve/tests -p 'test_*.py': OK. Two tests are new: the allow rule plus the all-rejected guard, and the every-axis tool-call guard.
The cursor-review and workflow-pins suites are OK.
actionlint: nothing new. Its only warnings are the existing job.workflow_sha ones.
Not yet verified end to end. I couldn't run cursor-agent locally to confirm that Shell(*) turns rejected into executed calls. The new guard turns the first real round into the test: either the transcript shows shell calls succeeding, or the axis job fails with "all N shell call(s) were rejected".

Provenance

Authored by: agent-work loop
Verified:
- cursor-approve unittest suite: 164 tests OK at the current head, including the .cursorignore removal assertion.
- check_workflow_pins.py, check_agents_md.py --root . and the org-repo-literal lint pass.
- PR CI checks are green at the current head. The end-to-end cursor-agent run is still unverified (see Testing).
Deviations:
- The precomputed-diff approach from the first revision was replaced with an explicit Shell(*) allow rule after review.
- Also removes PR-authored .cursorignore and .cursorindexingignore files. This was not in the original scope.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/375 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/8/2026 **Status:** ✅ Merged **Merged:** 10/8/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/axis-checkout-precomputed-diff` --- ### 📝 Commits (3) - [`9f977b7`](https://github.com/Comfy-Org/github-workflows/commit/9f977b7edad32f87dc9a8f96a415c9538202f728) fix(cursor-approve): give checkout axes the git output instead of a shell they never had - [`220a7ab`](https://github.com/Comfy-Org/github-workflows/commit/220a7abbb1c5d13eb65136cc53295b944c23ae84) fix(cursor-approve): allow the checkout axes a shell instead of handing them a diff - [`ccf1100`](https://github.com/Comfy-Org/github-workflows/commit/ccf110035cf45b1d4194386dfcb93cff17bad957) fix(cursor-approve): remove PR-authored .cursorignore files before an axis runs ### 📊 Changes **3 files changed** (+60 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-approve/tests/test_context_axes.py` (+15 -0) 📝 `.github/workflows/cursor-axis-base.yml` (+35 -7) 📝 `docs/callers/cursor-approve.md` (+10 -4) </details> ### 📄 Description ## ELI-5 The correctness, conformance and completeness reviewers are told to read the PR's diff with `git`. Every shell command they try gets silently rejected, so they never see the diff and approve by guessing from files on disk. This PR gives them the shell they were always meant to have. The reviewer gets the merge-base range and decides how to review it. If the shell ever breaks again, the job now fails visibly instead of returning a blind verdict. ## What was wrong `cursor-axis-base.yml` writes `.cursor/cli.json` with `allow: []`. Unless `no_shell` is set, it also writes `deny: []`. `cursor-agent --print` has no one to approve a command, so it rejects any shell call that no allow rule matches. On checkout axes, every `git diff` and `git log` came back `{"rejected": {..., "reason": ""}}`. The agents fell back to Read and Grep and said so in their verdicts ("Shell/git was unavailable to me"). The caller docs promised the completeness axis "keeps a shell for `git`", which was never true. ## Change - **Explicit allow rule.** When `no_shell` is false (the checkout axes), the config adds `Shell(*)` to `allow`. Business and design are unchanged: they get no shell, the existing deny list, and the existing check that fails the job if a shell call appears anyway. - **Guards against a quiet regression:** - Every axis now fails if its transcript has no recognizable tool call. That check used to run only on `no_shell` axes. - A checkout axis fails if every shell call it made was `rejected`, which is exactly this bug. - **Docs and the input header** describe the shell and the guards. - **PR-authored ignore files are removed** before the agent runs: `.cursorignore` and `.cursorindexingignore` are deleted alongside the PR's `.cursor/` config, so a PR cannot hide its own changes from the read and search tools of the reviewer judging it. ## Revised after review The first version precomputed the diff into `.git/` and pointed the agent at it. @huntcsg pointed out that a large diff can blow up the agent's context, and that with a shell the agent can choose its own approach: `--stat` first, read only the relevant hunks, or script a check over a mechanical change. The prompt already names the range (`git diff <merge_base>...<head>`), so this version drops the precompute. ## Risk This is the same unsandboxed shell over PR code that the cursor-review panel already accepts (see the `cursor-review.yml` header). The axis job has `contents: read`, a checkout with `persist-credentials: false`, and `CURSOR_API_KEY` in the environment. For completeness, the existing note about the Linear proxy token on a passwordless-`sudo` runner still applies, and the docs keep it. ## Testing - `python3 -m unittest discover -s .github/cursor-approve/tests -p 'test_*.py'`: OK. Two tests are new: the allow rule plus the all-rejected guard, and the every-axis tool-call guard. - The `cursor-review` and `workflow-pins` suites are OK. - `actionlint`: nothing new. Its only warnings are the existing `job.workflow_sha` ones. - **Not yet verified end to end.** I couldn't run `cursor-agent` locally to confirm that `Shell(*)` turns `rejected` into executed calls. The new guard turns the first real round into the test: either the transcript shows shell calls succeeding, or the axis job fails with "all N shell call(s) were rejected". ## Provenance - **Authored by:** agent-work loop - **Verified:** - `cursor-approve` unittest suite: 164 tests OK at the current head, including the `.cursorignore` removal assertion. - `check_workflow_pins.py`, `check_agents_md.py --root .` and the org-repo-literal lint pass. - PR CI checks are green at the current head. The end-to-end `cursor-agent` run is still unverified (see Testing). - **Deviations:** - The precomputed-diff approach from the first revision was replaced with an explicit `Shell(*)` allow rule after review. - Also removes PR-authored `.cursorignore` and `.cursorindexingignore` files. This was not in the original scope. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-10-26 22:39:11 +00:00

bsakaguchi referenced this issue

2026-10-26 22:39:11 +00:00

[PR #376] [MERGED] fix(cursor-approve): keep Slack out of the business axis #377

bsakaguchi closed this issue

2026-10-26 22:39:11 +00:00