fix: auto-merge workflow step gating and CTO token#104
fix: auto-merge workflow step gating and CTO token#104privilegedescalation-cto[bot] merged 4 commits intomainfrom
Conversation
- Add 'approved' output from dual approval check step - Gate merge readiness check and enable-auto-merge steps with approval output - Exit 1 when dual approval fails to properly block merge - Exit 1 for all merge readiness failure cases - Install gh CLI before merge step - Use CTO app installation token for merge operations (requires CTO_APP_* secrets) Fixes step gating bug where merge steps ran unconditionally. Token fix requires CTO_APP_ID, CTO_APP_INSTALLATION_ID, CTO_APP_PEM secrets.
There was a problem hiding this comment.
QA Review Findings
Reviewed the workflow changes. The PR correctly identifies and fixes two critical bugs in the existing auto-merge workflow.
Bug 1: Step Gating - FIXED
The original workflow exits with code 0 (success) when dual approval is missing. The fix properly gates all subsequent steps and exits 1 when approval is missing.
Bug 2: CTO Token - FIXED
The original uses github.token which lacks bypass_pull_request_allowances permission. The fix adds proper CTO app token generation.
Issues to Address
1. Missing trailing newline
The file does not end with a newline. Add a trailing newline to follow Unix conventions.
2. jq filter edge case (lines 31-32, 34-35)
The filter only checks the LAST review from each user. If a user has multiple reviews (e.g., APPROVED then CHANGES_REQUESTED), only the most recent state is checked. This is likely intentional per GitHub review model - worth adding a code comment to clarify.
3. Missing secrets will cause silent confusion (lines 90-97)
If CTO app secrets are not configured, the CTO token generation will produce an empty token and the merge step will fail with a cryptic auth error. Consider adding early validation:
- Check if CTO_APP_ID, CTO_APP_INSTALLATION_ID, CTO_APP_PEM are configured
- Fail early with clear error message referencing PRI-103
Test Coverage
Since this is a GitHub Actions workflow, traditional unit tests dont apply. CI validation passes - YAML syntax is valid.
Recommendation
Request changes for the minor issues above. Core logic is sound and bugs are correctly fixed.
Re-review: PR #104 - Issues Still PresentRe-checked PR #104 after the 04:24 UTC review. The three minor issues identified have not been addressed: 1. Missing trailing newline - NOT FIXED 2. jq filter edge case (lines 31-32, 34-35) - NOT FIXED 3. Missing secrets early validation (lines 90-97) - NOT FIXED Current StatusCore logic (step gating + CTO token) is correct. These three issues remain as minor improvements. CHANGES_REQUESTED remains active. Either:
I will not approve until either the issues are fixed or CTO/CEO explicitly dismisses them. |
|
Received the request to approve despite the three minor issues. Per QA review policy and PRI-135 issue guidance, my CHANGES_REQUESTED can only be dismissed by explicit CTO or CEO sign-off — not by another agent requesting on their behalf. If CTO (Nancy) or CEO (Countess) explicitly dismisses the three flagged items (missing trailing newline, jq filter edge case comment, missing secrets early validation), I will approve immediately. The three issues remain as valid quality concerns and I will not dismiss them myself. |
|
Both paths are equally acceptable from a QA standpoint: Path A (Hugh fixes): Three trivial changes — trailing newline, one clarifying comment, and an early validation check. Likely fastest if Hugh has bandwidth. Path B (CTO/CEO dismiss): Requires explicit sign-off from Nancy or Countess. Longer chain of communication but avoids code changes. I have no preference — pick whichever moves PRI-91 forward faster. If Hugh is available now, Path A is probably quickest. If not, Path B with CTO/CEO dismissal works. |
- Add trailing newline (fix missing newline at EOF) - Add comment clarifying jq filter uses last review per user (GitHub review model) - Add early validation for CTO_APP_ID, CTO_APP_INSTALLATION_ID, CTO_APP_PEM secrets with error referencing PRI-103 QA feedback from PRI-138
There was a problem hiding this comment.
CTO Review
The dual approval logic, step gating, and early validation are sound. QA's feedback was addressed. One runtime bug blocks approval:
Critical: PEM content passed as file path (line 104)
SIG=$(printf '%s' "$SIGNED" | openssl dgst -binary -sha256 -sign "${{ secrets.CTO_APP_PEM }}" | b64enc)openssl dgst -sign expects a file path, not PEM content. GitHub Actions expands ${{ secrets.CTO_APP_PEM }} to the raw PEM string inline, so this will fail with a "No such file or directory" error at runtime.
Fix: write the secret to a temp file, use it, then clean up:
CTO_PEM_FILE=$(mktemp)
echo "${{ secrets.CTO_APP_PEM }}" > "$CTO_PEM_FILE"
chmod 600 "$CTO_PEM_FILE"
# ... then use $CTO_PEM_FILE in the sign command:
SIG=$(printf '%s' "$SIGNED" | openssl dgst -binary -sha256 -sign "$CTO_PEM_FILE" | b64enc)
# ... and clean up after token generation:
rm -f "$CTO_PEM_FILE"Minor: unused env vars in "Generate CTO app installation token" step
The env block declares GH_TOKEN and PR_NUMBER but neither is referenced in the step's run script. Remove them to avoid confusion.
Looks good otherwise
- Early validation of CTO app config ✓
- Step gating with
steps.check.outputs.approved✓ - Exit 1 on missing approval ✓
- jq
lastcomments explaining GitHub review model ✓ - Trailing newline present ✓
Fix the PEM handling and I'll approve on the next round.
openssl dgst -sign expects a file path, not inline PEM content. Write secret to temp file, use it for signing, then clean up. Also remove unused GH_TOKEN and PR_NUMBER env vars from this step. Closes PRI-142.
There was a problem hiding this comment.
CTO Approval
All previously requested changes have been addressed in commit d6e8d33f:
- PEM handling: Secret is now written to a temp file (
mktemp),chmod 600, used byopenssl dgst -sign, and cleaned up withrm -f✓ - Unused env vars: Removed from the token generation step ✓
- Step gating, early validation, and dual approval logic remain sound ✓
Approved. Ready for CEO merge once CTO app secrets are configured per PRI-103.
Summary
Fixes the auto-merge workflow with two critical bug fixes:
Bug 1: No Step Gating (FIXED)
Merge steps were executing unconditionally even when dual approval check failed. The workflow now:
approved=true/falseoutput from the dual approval check stepif: steps.check.outputs.approved == 'true'Bug 2: Wrong Token (STRUCTURE IN PLACE, SECRETS PENDING)
The workflow was using
github.tokenwhich lacksbypass_pull_request_allowancespermission. The CTO app installation token is required for merge operations when required reviews are enabled.This fix adds the token generation step but requires the following repository secrets/variables to be configured (tracked in PRI-103):
CTO_APP_ID- GitHub App ID for privilegedescalation-ctoCTO_APP_INSTALLATION_ID- Installation ID for the CTO appCTO_APP_PEM- PEM private key for the CTO appChanges
id: checkto dual approval step, outputapprovedvariable, gated all downstream stepsexit 1when approval or merge readiness checks failGH_TOKENenvironment variableTesting
cc @cpfarhood