Skip to content

test: verify augment-pr-review workflow enhancements - #47

Closed
martinatpeek wants to merge 4 commits into
mainfrom
test/augment-review-workflow
Closed

martinatpeek wants to merge 4 commits into
mainfrom
test/augment-review-workflow

Conversation

@martinatpeek

@martinatpeek martinatpeek commented Apr 27, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Test PR to verify the enhanced augment-pr-review workflow, including Jira ticket context fetching, formal review decisions, and stale-review dismissal.

Tickets

  • DEVOP5-4079

Related PRs / Issues

Relates to #48

Risk Assessment

Deployment risk: Low — workflow-only changes, no production code modified

Affected systems: GitHub Actions PR review workflow only

Rollback plan: Close PR, nothing to revert

Testing

  • Augment review runs with claude-sonnet-4-6
  • Jira ticket DEVOP5-4079 context appears in the review guidelines
  • Formal APPROVE or REQUEST_CHANGES decision is posted by the bot user (if PAT_GITHUB_TOKEN is set)
  • Push a second commit and verify old bot review is dismissed as outdated

Introduces a SOC II/PCI compliant release workflow and supporting tooling.

## What's included

**bin/release** — three-command release script:
- `init`    — creates the `release` branch, sets branch protection (1 reviewer,
              enforce_admins, dismiss stale, self-approval blocked), creates the
              `release` label. Idempotent — re-run to restore protection settings.
- `open`    — opens a main→release PR with auto-detected version (convco),
              linked merged PRs, changelog, risk assessment template, Slack notification.
              Applies the `release` label so the PR title lint check is bypassed.
- `publish` — verifies PR was merged, tags release branch, creates GitHub release,
              posts Slack notification.

**Why main is unprotected:** main maps to sandbox (fast iteration). Only release maps
to production, satisfying SOC II CC8.1 / PCI DSS 6.3.x — every production change
must arrive via a reviewed PR to release.

**.github/workflows/pr-title.yml** — enforces Conventional Commits on PR titles via
amannn/action-semantic-pull-request@v5. Allowed types: feat, fix, chore, docs, style,
refactor, perf, test, ci, build. PRs labelled `release` are exempt.

**.github/pull_request_template.md** — pre-populates PRs with Description, Related
PRs/Issues, Risk Assessment (deployment risk, affected systems, rollback notes),
and a Testing checklist.

**.claude/skills/pr/SKILL.md** — /pr skill for opening PRs with Conventional Commits
title, Jira ticket links, and filled-out PR template with Risk Assessment level.

**.claude/skills/starter-kit-update/SKILL.md** — fixes master→main, uses jq for
portable list reversal, adds step 9 to open a PR via /pr after pushing.

**README + CONTRIBUTING** — Release Workflow section with full usage docs, compliance
rationale, Conventional Commits guidance with version-bump rules.

[DOWNSTREAM INSTRUCTIONS]
- Add .github/workflows/pr-title.yml from the starter kit — enforces Conventional
  Commits on PR titles. Copy as-is.
- Add .claude/skills/pr/SKILL.md from the starter kit — /pr skill for opening PRs.
  Copy as-is.
- If .github/pull_request_template.md does not exist, add it from the starter kit.
  If one already exists, merge the Risk Assessment section into it.
- Update CONTRIBUTING.md: replace the Commit Messages section with Conventional
  Commits guidance (types, version-bump rules). Update PR Title Format section to
  note automated linting via pr-title.yml.
- Update .claude/skills/starter-kit-update/SKILL.md: apply the master→main fix and
  the jq reverse fix (replace tac/tail -r with [.[].sha] | reverse | .[]).
  Add step 9 instructing use of /pr to open the PR after pushing.
- Copy bin/release from the starter kit as-is. The script detects the repo name
  at runtime via gh CLI — no project-specific substitutions needed.
- Update README.md: add or replace the Release Workflow section using the starter
  kit's version as a template, adapting any project-specific names. The section
  should cover: prerequisites (convco, jq), bin/release init/open/publish usage,
  Slack notification setup, and the Conventional Commits format requirement.
  Also add the PR Title Linting entry under the GitHub Actions section.
[/DOWNSTREAM INSTRUCTIONS]
…d stale-review cleanup

- Use PAT_GITHUB_TOKEN (when set) so review comments appear from a bot user
- Submit REQUEST_CHANGES or APPROVE based on whether Augment left inline comments
- Dismiss outdated bot reviews when a new commit is pushed or a human reviews
- Fetch linked Jira ticket context (summary, status, priority) and prepend to
  review guidelines when JIRA_API_TOKEN is set
- Pin Augment review model to claude-sonnet-4-6
@martinatpeek martinatpeek self-assigned this Apr 27, 2026
@martinatpeek
martinatpeek requested review from coladarci and removed request for coladarci April 27, 2026 02:52

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two concrete issues found: a test artifact left in the PR template that will appear in every future PR, and a weak merged-PR guard in bin/release publish that doesn't confirm the version being published.


🤖 Automated review complete. Please react with 👍 or 👎 on the individual review comments to provide feedback on their usefulness.


<!-- What does this PR do? What problem does it solve? -->

<!-- TEST: dummy change to verify augment-pr-review workflow -->

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test comment was left in the shipped template and will appear in every PR description created from this template going forward.

Current code:

<!-- TEST: dummy change to verify augment-pr-review workflow -->

Suggested improvement: remove line 5 entirely before merging.

Comment thread bin/release
--json title \
--jq ".[0].title" 2>/dev/null || echo "")

if [[ -z "$merged" ]]; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guard only checks that some merged PR from main → release exists — it doesn't verify the title contains $version. A stale release PR from a prior cycle (e.g. "Release 1.6.0") would pass this check, allowing bin/release publish 1.7.0 to tag the wrong commit.

Current code:

if [[ -z "$merged" ]]; then
  echo "Error: no merged PR found from main → $RELEASE_BRANCH"

Suggested improvement:

if [[ -z "$merged" ]]; then
  echo "Error: no merged PR found from main → $RELEASE_BRANCH"
  echo "  The review PR must be approved and merged before publishing."
  exit 1
fi

if [[ "$merged" != *"$version"* ]]; then
  echo "Error: the most recent merged release PR is '$merged', which does not match version $version."
  echo "  Ensure the correct release PR is merged before publishing."
  exit 1
fi

@martinatpeek

Copy link
Copy Markdown
Collaborator Author

Superseded by #48

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two concrete issues found — one will affect every future PR in this repo, and one can allow bin/release publish to proceed without a proper review gate.


🤖 Automated review complete. Please react with 👍 or 👎 on the individual review comments to provide feedback on their usefulness.


<!-- What does this PR do? What problem does it solve? -->

<!-- TEST: dummy change to verify augment-pr-review workflow -->

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test comment will be included in every future PR's description because it lives in the shared template. It should be removed before merging.

Current code:

<!-- TEST: dummy change to verify augment-pr-review workflow -->

Suggested improvement: remove line 5 entirely — the <!-- What does this PR do? --> comment on line 3 is the correct placeholder.

Comment thread bin/release
--head main \
--state merged \
--json title \
--jq ".[0].title" 2>/dev/null || echo "")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The merged-PR verification check only confirms that some PR from main → release was merged, not the one for $version. If a previous release PR was already merged and you run bin/release publish 2.0.0 before opening that PR, the guard passes incorrectly and the publish proceeds without a review gate.

Current code:

merged=$(gh pr list \
  --base "$RELEASE_BRANCH" \
  --head main \
  --state merged \
  --json title \
  --jq ".[0].title" 2>/dev/null || echo "")

Suggested improvement — filter by title to pin the check to this specific version:

merged=$(gh pr list \
  --base "$RELEASE_BRANCH" \
  --head main \
  --state merged \
  --json title \
  --jq ".[] | select(.title == \"Release $version\") | .title" \
  2>/dev/null || echo "")

@martinatpeek

Copy link
Copy Markdown
Collaborator Author

Superseded by #48 — test branch with cleanup needed

@martinatpeek martinatpeek reopened this Apr 27, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two concrete issues found — one a leftover test artifact in the PR template, and one a logic gap in the release publish guard. Everything else looks solid.


🤖 Automated review complete. Please react with 👍 or 👎 on the individual review comments to provide feedback on their usefulness.


<!-- What does this PR do? What problem does it solve? -->

<!-- TEST: dummy change to verify augment-pr-review workflow -->

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a test artifact that should be removed before merging — it will appear in every future PR's pre-filled description.

Current code:

<!-- TEST: dummy change to verify augment-pr-review workflow -->

Suggested improvement: remove line 5 entirely so the ## Description section only keeps the intended placeholder comment on line 3.

Comment thread bin/release
--head main \
--state merged \
--json title \
--jq ".[0].title" 2>/dev/null || echo "")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The merge guard checks whether any past PR from main → release was ever merged, not whether a PR for this specific version was merged. After the very first release, this check will always pass and can no longer catch someone running bin/release publish without going through the review flow first.

Current code:

merged=$(gh pr list \
  --base "$RELEASE_BRANCH" \
  --head main \
  --state merged \
  --json title \
  --jq ".[0].title" 2>/dev/null || echo "")

Suggested improvement: narrow the query to a title that matches the current release version so the guard remains meaningful for every release, not just the first:

merged=$(gh pr list \
  --base "$RELEASE_BRANCH" \
  --head main \
  --state merged \
  --json title \
  --jq ".[] | select(.title | test(\"Release $version\")) | .title" 2>/dev/null | head -1 || echo "")

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workflow enhancements look well-structured overall — the stale-review dismissal logic, loop-prevention guard, and Jira context injection all follow sound patterns. One logical bug in the compliance-critical bin/release publish path is worth addressing before this lands.


🤖 Automated review complete. Please react with 👍 or 👎 on the individual review comments to provide feedback on their usefulness.

Comment thread bin/release
--head main \
--state merged \
--json title \
--jq ".[0].title" 2>/dev/null || echo "")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The merge verification check is not version-specific, which undermines the SOC II/PCI compliance goal it's meant to enforce.

--jq ".[0].title" returns the title of the most-recently-merged PR from main → release, then only checks whether the result is non-empty. After the first release has been shipped, merged will always be non-empty — so bin/release publish 2.0.0 will succeed even if the 2.0.0 review PR was never opened, approved, or merged.

Current code:

merged=$(gh pr list \
  --base "$RELEASE_BRANCH" \
  --head main \
  --state merged \
  --json title \
  --jq ".[0].title" 2>/dev/null || echo "")

if [[ -z "$merged" ]]; then
  echo "Error: no merged PR found from main → $RELEASE_BRANCH"

Suggested improvement — filter by the specific release title:

merged=$(gh pr list \
  --base "$RELEASE_BRANCH" \
  --head main \
  --state merged \
  --json title \
  --jq ".[] | select(.title == \"Release $version\") | .title" 2>/dev/null | head -1 || echo "")

if [[ -z "$merged" ]]; then
  echo "Error: no merged PR found for 'Release $version' from main → $RELEASE_BRANCH"

This way the guard actually verifies that the specific version's PR went through review before the tag is pushed.

@martinatpeek
martinatpeek deleted the test/augment-review-workflow branch May 26, 2026 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant