[PR #402] [MERGED] feat(cursor-review): say why a direct-API cell failed instead of discarding it #405

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/direct-lane-error-visibility

📝 Commits (3)

0db11aa feat(cursor-review): say why a direct-API cell failed instead of discarding it
fef9377 test(cursor-review): execute the cell-outcome step, not just lint it
b537a26 fix(cursor-review): close the review's six findings on the error reporter

📊 Changes

5 files changed (+1126 additions, -10 deletions)

View changed files

📝 .github/cursor-review/README.md (+1 -0)
➕ .github/cursor-review/agent-error.py (+310 -0)
➕ .github/cursor-review/tests/test_agent_error.py (+737 -0)
📝 .github/workflows/cursor-review.yml (+52 -10)
📝 docs/callers/cursor-review.md (+26 -0)

📄 Description

The hole

Both direct-API lanes send their agent's structured output to a file that is never echoed:

lane	step	output
Cursor cells	`Run cursor review`	`head -c 500 /tmp/review-raw.txt` + all of stderr — diagnosable
direct-OpenAI	`Run direct OpenAI review`	`> /tmp/codex-events.jsonl` — never echoed, never uploaded
direct-Anthropic	`Run direct Anthropic review`	`> /tmp/claude-result.json` — same hole

The redirect is correct — those files hold the agent's reasoning, tool calls and findings, i.e. model output steered by PR-authored text, which Report cell outcome already refuses to echo for that reason. The problem is that codex exec --json reports API-level failures as events on stdout, into that same file. So a failing cell's log read:

Reading prompt from stdin...
Error: Process completed with exit code 1.

and the lane's failures had to be inferred from job duration.

Not hypothetical. An OpenAI insufficient_quota / credit_balance_exhausted — an error that named its own cause and linked the billing page — was discarded exactly this way, and the outage was diagnosed by probing the key by hand instead.

The fix

agent-error.py walks the file and prints, from objects that look like errors, an allowlist of short provider metadata (type, subtype, code, param, status, message) plus a census of event types. Wired into Report cell outcome in both lanes, on the non-ok branch only.

The motivating payload now reads:

=== why the cell failed (adversarial/gpt-5.6-sol) ===
events: 3 (error×1, insufficient_quota×1, item.completed×1, reasoning×1, session.created×1)
error: type=error
error: type=insufficient_quota code=credit_balance_exhausted message=You have no credits remaining. Add credits to continue using the API at https://platform.openai.com/settings/organization/billing/.

Three decisions worth a reviewer's attention

The allowlist is the safety property, not a convenience, and it covers both fields and descent. result, content, text, input, output, arguments, reasoning, delta and thinking are not field-allowlisted, and an allowlisted message holding content blocks is skipped rather than flattened. Restricting fields alone was not enough, though. The agent chooses its own MCP tool-call arguments, so a planted {"error": {...}} there would have been printed. The walk therefore descends only through keys a CLI owns (ENVELOPE_KEYS), so it never reaches the model's writable subtrees. An unanticipated provider shape reports only the census, which is the safe way for this to fail. AllowlistTest asserts the absence of prose rather than the presence of fields, so a future key added to ERROR_FIELDS that happens to carry content fails there.

An error KEY is read on its own terms rather than by heuristic. A provider's error object need not describe itself as an error: OpenAI's says insufficient_quota / credit_balance_exhausted, and neither contains the substring "error". The first draft of this script parsed that exact payload and still printed only the envelope's own type=error — missing the one message worth having. NestedEnvelopeTest pins the shape so the regression cannot come back.

The raw file is deliberately NOT uploaded as an artifact. The Anthropic lane has a Refuse a record carrying the API key guard that screens findings.json before publishing it; shipping the unscreened transcript alongside would route the agent's own prose straight past that guard. That felt like the obvious add-on and I left it out on purpose. When the allowlist plus the census is not enough to explain a failure, the fix is to widen the allowlist, not to publish the file.

Safety and failure behaviour

Sanitized for a ::workflow command:: line by replacing disallowed characters, never deleting them, so a crafted value cannot collapse into different text. Every :: is broken to : :, runs of three or more colons included, and ##[ is broken the same way. The --label is sanitized too.
Exits 0 on anything, including a missing file, a directory, an empty file, plain text, a truncated last event, invalid UTF-8 and deeply nested well-formed JSON (RecursionError is caught at both decode sites, with a broad guard in main). Both lanes also call it with || true under bash -e. It runs in an always() step after the cell already failed; a traceback here would replace the diagnosis with a crash.
Bounded against an agent-written file: value length, line count, total bytes, walk depth and node count. It reads the file's tail and walks newest-first, because the event that explains the failure comes last. When it cuts input it says so.
Claude Code's failure cause stays withheld. It sits in result, the same field that holds the review prose. That lane reports is_error=true plus the census, and the caller doc states this gap per lane. Duplicate error objects collapse; the census is capped at 10 kinds.
Loaded from $CURSOR_REVIEW_ASSETS, i.e. the pinned ref of this repo — never the caller's checkout, so a PR cannot rewrite the code reporting on it. Pinned by test_the_script_is_loaded_from_the_pinned_assets.

Tests

test_agent_error.py has 65 tests; the .github/cursor-review suite (1400) and .github/cursor-approve (205) pass. StepBehaviourTest extracts the real Report cell outcome body from cursor-review.yml and runs it against fixtures for both lanes. WorkflowWiringTest pins that both lanes call it, each on the file its own agent writes (a rename on one side only would leave the diagnosis reading a file nobody writes, and could not read is a plausible-looking output).

I mutation-tested the suite rather than trusting a green run. In the first round, five of six deliberate breakages were caught (dropping the nested-envelope branch → 6 failures; admitting result to the allowlist → 3; dropping the depth guard → error; sanitizing by deletion → 1; not breaking :: → 2).

The sixth was a miss, and it was my test comment that was wrong: I had claimed the isinstance(value, bool) guard was what stopped "status": true being read as a status code. It isn't — True >= 400 is already false, so deleting the guard changes nothing and no test can catch it. The guard stays as a statement of intent for anyone who lowers the threshold, and the comment now says that instead of overstating it.

Not in scope

No new input, so no caller knob to document. The caller page gets one Gotchas note on where to look when a direct cell fails.
CURSOR_PANEL_MODELS is now misleading on both repos (it names three Cursor models while two are replaced at runtime) — separate cleanup.
The per-lane failure budget (count models, not cells) is still unbuilt; approve_max_failed_reviewers: 2 remains a per-cell count.

Provenance

Authored by: agent-work loop
Verified:
- python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py': 1400 pass at b537a26
- .github/cursor-approve suite: 205 pass
- check_workflow_pins.py and check_agents_md.py --root . are clean
- Neither direct lane runs in this repo's CI (ci-cursor-review.yml sets no direct model), so the changed step is exercised only by StepBehaviourTest
Deviations:
- Field allowlisting was not sufficient on its own; descent is now allowlisted too (review round, b537a26)
- The Claude lane's cause in result is withheld; it reports is_error only

🤖 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/402 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/10/2026 **Status:** ✅ Merged **Merged:** 10/10/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/direct-lane-error-visibility` --- ### 📝 Commits (3) - [`0db11aa`](https://github.com/Comfy-Org/github-workflows/commit/0db11aa91419b68fcb856b5f36b6bc8d08305b48) feat(cursor-review): say why a direct-API cell failed instead of discarding it - [`fef9377`](https://github.com/Comfy-Org/github-workflows/commit/fef9377b041a0c80bc656463cbd6be7350ec6f1f) test(cursor-review): execute the cell-outcome step, not just lint it - [`b537a26`](https://github.com/Comfy-Org/github-workflows/commit/b537a269647d503ae88aa50e257359e2c1a0a9bd) fix(cursor-review): close the review's six findings on the error reporter ### 📊 Changes **5 files changed** (+1126 additions, -10 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+1 -0) ➕ `.github/cursor-review/agent-error.py` (+310 -0) ➕ `.github/cursor-review/tests/test_agent_error.py` (+737 -0) 📝 `.github/workflows/cursor-review.yml` (+52 -10) 📝 `docs/callers/cursor-review.md` (+26 -0) </details> ### 📄 Description ## The hole Both direct-API lanes send their agent's structured output to a file that is **never echoed**: | lane | step | output | |---|---|---| | Cursor cells | `Run cursor review` | `head -c 500 /tmp/review-raw.txt` + all of stderr — **diagnosable** | | direct-OpenAI | `Run direct OpenAI review` | `> /tmp/codex-events.jsonl` — never echoed, never uploaded | | direct-Anthropic | `Run direct Anthropic review` | `> /tmp/claude-result.json` — same hole | The redirect is correct — those files hold the agent's reasoning, tool calls and findings, i.e. model output steered by PR-authored text, which `Report cell outcome` already refuses to echo for that reason. The problem is that **`codex exec --json` reports API-level failures as events on stdout**, into that same file. So a failing cell's log read: ``` Reading prompt from stdin... Error: Process completed with exit code 1. ``` and the lane's failures had to be inferred from job **duration**. Not hypothetical. An OpenAI `insufficient_quota` / `credit_balance_exhausted` — an error that named its own cause *and linked the billing page* — was discarded exactly this way, and the outage was diagnosed by probing the key by hand instead. ## The fix `agent-error.py` walks the file and prints, from objects that look like errors, an **allowlist** of short provider metadata (`type`, `subtype`, `code`, `param`, `status`, `message`) plus a census of event types. Wired into `Report cell outcome` in **both** lanes, on the non-ok branch only. The motivating payload now reads: ``` === why the cell failed (adversarial/gpt-5.6-sol) === events: 3 (error×1, insufficient_quota×1, item.completed×1, reasoning×1, session.created×1) error: type=error error: type=insufficient_quota code=credit_balance_exhausted message=You have no credits remaining. Add credits to continue using the API at https://platform.openai.com/settings/organization/billing/. ``` ## Three decisions worth a reviewer's attention **The allowlist is the safety property, not a convenience, and it covers both fields and descent.** `result`, `content`, `text`, `input`, `output`, `arguments`, `reasoning`, `delta` and `thinking` are not field-allowlisted, and an allowlisted `message` holding *content blocks* is skipped rather than flattened. Restricting fields alone was not enough, though. The agent chooses its own MCP tool-call `arguments`, so a planted `{"error": {...}}` there would have been printed. The walk therefore descends only through keys a CLI owns (`ENVELOPE_KEYS`), so it never reaches the model's writable subtrees. An unanticipated provider shape reports only the census, which is the safe way for this to fail. `AllowlistTest` asserts the **absence** of prose rather than the presence of fields, so a future key added to `ERROR_FIELDS` that happens to carry content fails there. **An `error` KEY is read on its own terms rather than by heuristic.** A provider's error object need not describe *itself* as an error: OpenAI's says `insufficient_quota` / `credit_balance_exhausted`, and neither contains the substring `"error"`. The first draft of this script parsed that exact payload and still printed only the envelope's own `type=error` — missing the one message worth having. `NestedEnvelopeTest` pins the shape so the regression cannot come back. **The raw file is deliberately NOT uploaded as an artifact.** The Anthropic lane has a `Refuse a record carrying the API key` guard that screens `findings.json` before publishing it; shipping the unscreened transcript alongside would route the agent's own prose straight past that guard. That felt like the obvious add-on and I left it out on purpose. When the allowlist plus the census is not enough to explain a failure, the fix is to **widen the allowlist**, not to publish the file. ## Safety and failure behaviour - Sanitized for a `::workflow command::` line by **replacing** disallowed characters, never deleting them, so a crafted value cannot collapse into different text. Every `::` is broken to `: :`, runs of three or more colons included, and `##[` is broken the same way. The `--label` is sanitized too. - **Exits 0 on anything**, including a missing file, a directory, an empty file, plain text, a truncated last event, invalid UTF-8 and deeply nested well-formed JSON (`RecursionError` is caught at both decode sites, with a broad guard in `main`). Both lanes also call it with `|| true` under `bash -e`. It runs in an `always()` step *after* the cell already failed; a traceback here would replace the diagnosis with a crash. - Bounded against an agent-written file: value length, line count, total bytes, walk depth and node count. It reads the file's **tail** and walks newest-first, because the event that explains the failure comes last. When it cuts input it says so. - **Claude Code's failure cause stays withheld.** It sits in `result`, the same field that holds the review prose. That lane reports `is_error=true` plus the census, and the caller doc states this gap per lane. Duplicate error objects collapse; the census is capped at 10 kinds. - Loaded from `$CURSOR_REVIEW_ASSETS`, i.e. the pinned ref of this repo — never the caller's checkout, so a PR cannot rewrite the code reporting on it. Pinned by `test_the_script_is_loaded_from_the_pinned_assets`. ## Tests `test_agent_error.py` has 65 tests; the `.github/cursor-review` suite (**1400**) and `.github/cursor-approve` (**205**) pass. `StepBehaviourTest` extracts the real `Report cell outcome` body from `cursor-review.yml` and runs it against fixtures for both lanes. `WorkflowWiringTest` pins that both lanes call it, each on the file **its own agent writes** (a rename on one side only would leave the diagnosis reading a file nobody writes, and `could not read` is a plausible-looking output). I mutation-tested the suite rather than trusting a green run. In the first round, five of six deliberate breakages were caught (dropping the nested-envelope branch → 6 failures; admitting `result` to the allowlist → 3; dropping the depth guard → error; sanitizing by deletion → 1; not breaking `::` → 2). The sixth was a **miss, and it was my test comment that was wrong**: I had claimed the `isinstance(value, bool)` guard was what stopped `"status": true` being read as a status code. It isn't — `True >= 400` is already false, so deleting the guard changes nothing and no test can catch it. The guard stays as a statement of intent for anyone who lowers the threshold, and the comment now says that instead of overstating it. ## Not in scope - No new input, so no caller knob to document. The caller page gets one Gotchas note on where to look when a direct cell fails. - `CURSOR_PANEL_MODELS` is now misleading on both repos (it names three Cursor models while two are replaced at runtime) — separate cleanup. - The per-lane failure budget (count models, not cells) is still unbuilt; `approve_max_failed_reviewers: 2` remains a per-cell count. ## Provenance - **Authored by:** agent-work loop - **Verified:** - `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'`: 1400 pass at b537a26 - `.github/cursor-approve` suite: 205 pass - `check_workflow_pins.py` and `check_agents_md.py --root .` are clean - Neither direct lane runs in this repo's CI (`ci-cursor-review.yml` sets no direct model), so the changed step is exercised only by `StepBehaviourTest` - **Deviations:** - Field allowlisting was not sufficient on its own; descent is now allowlisted too (review round, b537a26) - The Claude lane's cause in `result` is withheld; it reports `is_error` only 🤖 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

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