fix(auto-release): resolve manual release subjects from git - #44
Conversation
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 330a5ba651a7
Profile: claude-reviewer-gh - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| automation:ci-deploy | 0 |
| security:code-auditor | 0 |
| python-lambda:implementation-tests | 2 |
| harness-engineering:repo-health | 3 |
python-lambda:implementation-tests (2 findings)
Minor - actions/auto-release/test_workflow.py:105
This test calls
actions/conventional-commit/check.shdirectly instead of feeding the meta-produced subject through the composite gate script, so the non-release/invalid-subject cases never exercise the code path the PR is fixing.The rc mapping of
check.shitself is already covered byactions/auto-release/test_gate.sh(docs skip-> 1, invalid -> 2), so the only new value here is thatmetaemits a subject that the gate consumes correctly. That translation step —commit-subject->COMMIT_MESSAGE->rc->should-release— is exactly where the reported production failure occurred (emptyCOMMIT_MESSAGEproducedconventional-commit check failed (rc=2)and a failed job). As written, a regression in the action's rc handling (e.g. treating rc=1 as a hard error, or rc=2 as a skip) would leave all six tests green.Fix: run
ACTION["runs"]["steps"][0]["run"]the waytest_manual_release_and_dry_rundoes, passingCOMMIT_MESSAGE=meta["commit-subject"], and assert the outcomes: fordocs: update guideexpect returncode 0 withshould-release == "false", and fornot conventionalexpect a nonzero returncode with noshould-releasekey (or whatever the intended contract is). That keeps the assertion on the workflow's observable behavior rather than on a helper script already under test elsewhere.
Nits - actions/auto-release/test_workflow.py:51
dict(line.split("=", 1) for line in ...)raisesValueError: dictionary update sequence element #N has length 1when a step writes a line without=. That is precisely the failure mode the body-leak guard intest_manual_release_and_dry_runis meant to catch ("Body must not enter the output file."), so the most important regression this file guards against surfaces as an opaque unittest error in a helper rather than a named assertion failure.Fix: parse defensively and assert, e.g. split into
pairs = [line.split("=", 1) for line in lines],self.assertTrue(all(len(p) == 2 for p in pairs), output.read_text()), then build the dict. Cheap, and it makes a leaked multi-line subject report itself directly.
harness-engineering:repo-health (3 findings)
Minor - .github/workflows/auto-release.yml:70
This repo states it is the automation source of truth for callers (README.md:3-6), so behavioral contracts of a reusable workflow need to live in versioned files. The diff makes
workflow_dispatch-driven callers a supported path (subject now resolved fromgithub.sharather than the push-only payload), but the only place that contract is written down is the PR description: "Manual runs evaluate the selected commit, not all changes since the last release, and are not a force-release mechanism." The workflow's own header comment still reads "on push to main" (lines 3-5), and README.md says nothing about manual runs. A future agent or caller reading the file will conclude manual dispatch is unsupported, or will assume a manual run releases everything accumulated since the last tag — the path gate on a manual run only sees the single selected commit (github.event.beforeis empty, sogate.shtakes thediff-tree --rootbranch). Fix: extend the header comment to say the workflow serves both push and manual (workflow_dispatchin the caller) runs, and that a manual run evaluates only the selected commit against both gates and is not a force-release.
Nits - actions/auto-release/test_workflow.py:74
The workflow steps are resolved by identity (
next(step for step in STEPS if step.get("id") == "meta"), lines 15-17), but the composite action's gate step is resolved positionally asACTION["runs"]["steps"][0]["run"]here. Inserting any step ahead of the gate inactions/auto-release/action.yml— a pre-flight check, for instance — would make this test execute the wrong shell block, and the failure would point at the test rather than at the edit. Fix: hoist a module-levelGATE_STEP = next(s for s in ACTION["runs"]["steps"] if s.get("id") == "gate")and useGATE_STEP["run"], matching the lookup idiom already used for the workflow steps.
Nits - actions/auto-release/test_workflow.py:1
The sibling suite documents how to run itself (
test_gate.sh:2: "run locally and in CI") and needs nothing but bash. This file adds the repo's first Python/third-party dependency, and the only record of that requirement is the pin inside.github/workflows/test-actions.yml(pip install "PyYAML==6.0.2"). Someone runningpython actions/auto-release/test_workflow.pylocally, as the PR's own validation section instructs, gets a bareModuleNotFoundErrorwith no pointer to the fix. Fix: extend the module docstring to one more line, e.g. "Requires PyYAML (pip install PyYAML); run withpython actions/auto-release/test_workflow.py."
Reviewer Coverage
automation:ci-deploy— complete (constrained); skipped: none; constraints: Reviewed statically; workflows were not executed. Cross-checked against actions/auto-release/action.yml, actions/auto-release/gate.sh, and actions/conventional-commit/check.sh, which are unchanged by this PR.security:code-auditor— complete (constrained); skipped: none; constraints: Read actions/auto-release/action.yml and gate.sh as unchanged context to trace the commit-subject sink; those files are outside the assigned set. Reviewed only for exploitable security issues; correctness/style of the new tests not assessed. The gate and identity-check composites are consumed at @v1, so the exact code that runs in production depends on where that tag points.python-lambda:implementation-tests— complete (constrained); inspected 1 assigned file (3 inspected across reviewers):actions/auto-release/test_workflow.py; skipped: none; constraints: Only actions/auto-release/test_workflow.py was in scope; the workflow YAML changes were read for context but are not reported on. This reviewer's lane is Python AWS Lambda runtime/test quality; the assigned file is a Python test harness for GitHub Actions shell blocks, so findings are limited to test-harness adequacy and Python-level brittleness. Verified locally:python3 actions/auto-release/test_workflow.py -v-> 6 tests, OK (PyYAML 6.0.2).harness-engineering:repo-health— complete (constrained); skipped: none; constraints: Base branch had no.codereview/agents/; repo guidance was limited to root README.md and in-file comments. Ranpython3 actions/auto-release/test_workflow.pylocally (6 tests, OK); actionlint/ShellCheck were not run. Review scoped to the three assigned files;actions/auto-release/action.yml,gate.sh, andtest_gate.shwere read as context only.
Inspected files (3)
.github/workflows/auto-release.yml.github/workflows/test-actions.ymlactions/auto-release/test_workflow.py
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 34s | ~$2.79 (est.) | claude-opus-5 | cr 0.10.302
| Field | Value |
|---|---|
| Model | claude-opus-5 |
| Reviewers | automation:ci-deploy, security:code-auditor, python-lambda:implementation-tests, harness-engineering:repo-health |
| Engine | claude_cli · claude-opus-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 3m 34s wall · 9m 19s compute |
| Cost | ~$2.79 (est.) |
| Pricing basis | anthropic-public-2026-09-02 |
| Tokens | 78 in / 30.2k out |
Per-workstream usage
orchestrator-selection— claude-opus-5- In: 4
- Out: 2.1k
- Cache read: 16.6k
- Cache create: 18.6k
- Cost: ~$0.25 (est.)
- Duration: 34s
automation:ci-deploy— claude-opus-5- In: 22
- Out: 8.5k
- Cache read: 309.7k
- Cache create: 35.1k
- Cost: ~$0.72 (est.)
- Duration: 2m 30s
security:code-auditor— claude-opus-5- In: 10
- Out: 2.8k
- Cache read: 89.6k
- Cache create: 30.1k
- Cost: ~$0.42 (est.)
- Duration: 1m 05s
python-lambda:implementation-tests— claude-opus-5- In: 16
- Out: 7.9k
- Cache read: 191.7k
- Cache create: 30.4k
- Cost: ~$0.60 (est.)
- Duration: 2m 13s
harness-engineering:repo-health— claude-opus-5- In: 20
- Out: 8.1k
- Cache read: 268.6k
- Cache create: 34.4k
- Cost: ~$0.68 (est.)
- Duration: 2m 34s
orchestrator-rollup— claude-opus-5- In: 6
- Out: 926
- Cache read: 81.3k
- Cache create: 7.3k
- Cost: ~$0.14 (est.)
- Duration: 21s
| self.git("-c", "commit.gpgsign=false", "commit", "--allow-empty", "-qm", subject) | ||
| result, meta = self.meta(self.git("rev-parse", "HEAD")) | ||
| self.assertEqual(result.returncode, 0, result.stderr) | ||
| result = subprocess.run( |
There was a problem hiding this comment.
This test calls actions/conventional-commit/check.sh directly instead of feeding the meta-produced subject through the composite gate script, so the non-release/invalid-subject cases never exercise the code path the PR is fixing.
The rc mapping of check.sh itself is already covered by actions/auto-release/test_gate.sh (docs skip -> 1, invalid -> 2), so the only new value here is that meta emits a subject that the gate consumes correctly. That translation step — commit-subject -> COMMIT_MESSAGE -> rc -> should-release — is exactly where the reported production failure occurred (empty COMMIT_MESSAGE produced conventional-commit check failed (rc=2) and a failed job). As written, a regression in the action's rc handling (e.g. treating rc=1 as a hard error, or rc=2 as a skip) would leave all six tests green.
Fix: run ACTION["runs"]["steps"][0]["run"] the way test_manual_release_and_dry_run does, passing COMMIT_MESSAGE=meta["commit-subject"], and assert the outcomes: for docs: update guide expect returncode 0 with should-release == "false", and for not conventional expect a nonzero returncode with no should-release key (or whatever the intended contract is). That keeps the assertion on the workflow's observable behavior rather than on a helper script already under test elsewhere.
Reply inline to this comment.
| @@ -0,0 +1,126 @@ | |||
| """Exercise the actual workflow shell blocks without minting release tags.""" | |||
There was a problem hiding this comment.
The sibling suite documents how to run itself (test_gate.sh:2: "run locally and in CI") and needs nothing but bash. This file adds the repo's first Python/third-party dependency, and the only record of that requirement is the pin inside .github/workflows/test-actions.yml (pip install "PyYAML==6.0.2"). Someone running python actions/auto-release/test_workflow.py locally, as the PR's own validation section instructs, gets a bare ModuleNotFoundError with no pointer to the fix. Fix: extend the module docstring to one more line, e.g. "Requires PyYAML (pip install PyYAML); run with python actions/auto-release/test_workflow.py."
Reply inline to this comment.
| text=True, | ||
| capture_output=True, | ||
| ) | ||
| values = dict(line.split("=", 1) for line in output.read_text().splitlines()) |
There was a problem hiding this comment.
dict(line.split("=", 1) for line in ...) raises ValueError: dictionary update sequence element #N has length 1 when a step writes a line without =. That is precisely the failure mode the body-leak guard in test_manual_release_and_dry_run is meant to catch ("Body must not enter the output file."), so the most important regression this file guards against surfaces as an opaque unittest error in a helper rather than a named assertion failure.
Fix: parse defensively and assert, e.g. split into pairs = [line.split("=", 1) for line in lines], self.assertTrue(all(len(p) == 2 for p in pairs), output.read_text()), then build the dict. Cheap, and it makes a leaked multi-line subject report itself directly.
Reply inline to this comment.
| self.assertEqual(result.returncode, 0, result.stderr) | ||
| self.assertEqual(meta["commit-subject"], "feat: ship it") | ||
| result, gate = self.run_script( | ||
| ACTION["runs"]["steps"][0]["run"], |
There was a problem hiding this comment.
The workflow steps are resolved by identity (next(step for step in STEPS if step.get("id") == "meta"), lines 15-17), but the composite action's gate step is resolved positionally as ACTION["runs"]["steps"][0]["run"] here. Inserting any step ahead of the gate in actions/auto-release/action.yml — a pre-flight check, for instance — would make this test execute the wrong shell block, and the failure would point at the test rather than at the edit. Fix: hoist a module-level GATE_STEP = next(s for s in ACTION["runs"]["steps"] if s.get("id") == "gate") and use GATE_STEP["run"], matching the lookup idiom already used for the workflow steps.
Reply inline to this comment.
| version="$(tr -d ' \t\n\r' < "$WD/$vfile")" | ||
| # workflow_dispatch has no head_commit payload. Read the subject from | ||
| # the exact commit we will tag, for both push and manual runs. | ||
| subject="$(git log -1 --format=%s "$SHA" --)" |
There was a problem hiding this comment.
This repo states it is the automation source of truth for callers (README.md:3-6), so behavioral contracts of a reusable workflow need to live in versioned files. The diff makes workflow_dispatch-driven callers a supported path (subject now resolved from github.sha rather than the push-only payload), but the only place that contract is written down is the PR description: "Manual runs evaluate the selected commit, not all changes since the last release, and are not a force-release mechanism." The workflow's own header comment still reads "on push to main" (lines 3-5), and README.md says nothing about manual runs. A future agent or caller reading the file will conclude manual dispatch is unsupported, or will assume a manual run releases everything accumulated since the last tag — the path gate on a manual run only sees the single selected commit (github.event.before is empty, so gate.sh takes the diff-tree --root branch). Fix: extend the header comment to say the workflow serves both push and manual (workflow_dispatch in the caller) runs, and that a manual run evaluates only the selected commit against both gates and is not a force-release.
Reply inline to this comment.
2 participants
Summary
github.shabeing tagged instead of the push-onlygithub.event.head_commit.messagepayload.Evidence
The manual dry-run in https://github.com/open-cli-collective/google-cli/actions/runs/33848087132 failed with an empty
COMMIT_MESSAGEandconventional-commit check failed (rc=2).workflow_dispatchdoes not providehead_commit.Validation
bash actions/auto-release/test_gate.sh— passed.python actions/auto-release/test_workflow.py— 6 tests passed.git diff --check— passed.Rollout
After merge, promote the reviewed shared-automation commit to
v1using the normal release process so existing callers receive this fix. This PR does not move tags or publish a release. Manual runs evaluate the selected commit, not all changes since the last release, and are not a force-release mechanism.The separate google-cli release-path fix will include
.goreleaser.yamlso future packaging-only feat/fix commits qualify.