[GH-ISSUE #419] [groom] The 65-line `Record token usage` Python heredoc is pasted verbatim into three jobs across two workflows #10

New issue

Open

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

bsakaguchi commented

2026-10-26 22:34:31 +00:00

Owner

Originally created by @cloud-code-bot[bot] on GitHub (Oct 12, 2026).
Original GitHub issue: https://github.com/Comfy-Org/github-workflows/issues/419

Groom auto-builder — Comfy-Org/github-workflows: this CONFIRMED finding could not be auto-built (patch modifies a CI-privileged or dataset-of-record path (per .github/groom/patch_policy.py — workflow/action defs, lockfiles, build/test config that executes in pre-review CI, graded eval cases under a suites/**/cases/ tree, or a symlink into such a tree) — a human must author these changes), so it is filed for a human. · run

Verified. The same ~65-line python3 -I - <<PY program — the count() / mapping() validators, the six-key totals dict, the modelUsage loop with its 1-to-100-char name guard, the malformed-field reporting and the measured flag — appears at cursor-review.yml:4227 (direct cells, reading /tmp/claude-result.json), cursor-review.yml:4786 (direct judge, /tmp/judge-claude-result.json) and cursor-axis-base.yml:883 (direct axis, /tmp/axis-claude-result.json). Only the result path and the REVIEW_TYPE env differ, and two tests pin that: test_workflow_judge_direct.py:449 and .github/cursor-approve/tests/test_axis_direct.py:336 each assert the copy equals the cells' block with the path substituted.

Corrections to the finder's framing — the fix still stands. The sanitiser is NOT 'untested by construction': test_workflow_panel_integrity.py:1259 (RecordTokenUsageTest) extracts the step and actually RUNS it through the suite's run_step harness, covering the no-result, truncated-JSON, malformed-count and no-leak cases. So extraction does not buy first-time testability; it buys one implementation plus ordinary unit tests in place of two cross-file byte-parity assertions (one test METHOD each, not 'two byte-parity test classes'). The honest win is ~130 duplicated lines removed and a future change (a new usage key, a tightened model-name guard) applied once rather than three times.

How. Extract to .github/cursor-review/record-usage.py taking --result / --model / --review-type / --out. All three jobs already have the assets: CURSOR_REVIEW_ASSETS is a workflow-level env: at cursor-review.yml:853 and is set at cursor-axis-base.yml:127, and all three already invoke fence-diff.py, agent-error.py, build-ledger.py or install-cursor-cli.sh from it. The two parity assertions become imports of one module; RecordTokenUsageTest's cases move onto it directly, which also lets the malformed-field branches be exercised without a workflow-step harness.

Risk. Low. Pure data transformation over a file, no credentials, every call site is continue-on-error: true and two are timeout-minutes-boxed, and the output JSON shape is already asserted. The one new failure mode is a missing or broken assets checkout, which would already have failed the same job long before this step, since the prompt build and the CLI pin read from the same directory. Not security-flagged: the record carries only integers plus a shape-bounded model id, the agent in these cells has no Write tool (--tools "Read,Grep,Glob", cursor-review.yml:4118) and the result file is a host-side stdout capture — accounting hygiene rather than an exfiltration control.

Originally created by @cloud-code-bot[bot] on GitHub (Oct 12, 2026). Original GitHub issue: https://github.com/Comfy-Org/github-workflows/issues/419 **Groom auto-builder** — Comfy-Org/github-workflows: this CONFIRMED finding could not be auto-built (patch modifies a CI-privileged or dataset-of-record path (per .github/groom/patch_policy.py — workflow/action defs, lockfiles, build/test config that executes in pre-review CI, graded eval cases under a suites/**/cases/ tree, or a symlink into such a tree) — a human must author these changes), so it is filed for a human. · [run](https://github.com/Comfy-Org/github-workflows/actions/runs/38232458823) **Verified.** The same ~65-line `python3 -I - <<PY` program — the `count()` / `mapping()` validators, the six-key totals dict, the `modelUsage` loop with its 1-to-100-char name guard, the malformed-field reporting and the `measured` flag — appears at cursor-review.yml:4227 (direct cells, reading /tmp/claude-result.json), cursor-review.yml:4786 (direct judge, /tmp/judge-claude-result.json) and cursor-axis-base.yml:883 (direct axis, /tmp/axis-claude-result.json). Only the result path and the REVIEW_TYPE env differ, and two tests pin that: test_workflow_judge_direct.py:449 and .github/cursor-approve/tests/test_axis_direct.py:336 each assert the copy equals the cells' block with the path substituted. **Corrections to the finder's framing — the fix still stands.** The sanitiser is NOT 'untested by construction': test_workflow_panel_integrity.py:1259 (RecordTokenUsageTest) extracts the step and actually RUNS it through the suite's `run_step` harness, covering the no-result, truncated-JSON, malformed-count and no-leak cases. So extraction does not buy first-time testability; it buys one implementation plus ordinary unit tests in place of two cross-file byte-parity assertions (one test METHOD each, not 'two byte-parity test classes'). The honest win is ~130 duplicated lines removed and a future change (a new usage key, a tightened model-name guard) applied once rather than three times. **How.** Extract to .github/cursor-review/record-usage.py taking --result / --model / --review-type / --out. All three jobs already have the assets: CURSOR_REVIEW_ASSETS is a workflow-level `env:` at cursor-review.yml:853 and is set at cursor-axis-base.yml:127, and all three already invoke fence-diff.py, agent-error.py, build-ledger.py or install-cursor-cli.sh from it. The two parity assertions become imports of one module; RecordTokenUsageTest's cases move onto it directly, which also lets the malformed-field branches be exercised without a workflow-step harness. **Risk.** Low. Pure data transformation over a file, no credentials, every call site is `continue-on-error: true` and two are `timeout-minutes`-boxed, and the output JSON shape is already asserted. The one new failure mode is a missing or broken assets checkout, which would already have failed the same job long before this step, since the prompt build and the CLI pin read from the same directory. Not security-flagged: the record carries only integers plus a shape-bounded model id, the agent in these cells has no Write tool (`--tools "Read,Grep,Glob"`, cursor-review.yml:4118) and the result file is a host-side stdout capture — accounting hygiene rather than an exfiltration control.

bsakaguchi added the

groom

label

2026-10-26 22:34:31 +00:00

bsakaguchi referenced this issue

2026-10-26 22:35:27 +00:00

[PR #10] [MERGED] ci: auto-bump cursor-review SHA in all caller repos on release #20