Fix verify-pr-base PR resolution in default workflows#1906
Open
stephanieg467 wants to merge 2 commits into
Open
Fix verify-pr-base PR resolution in default workflows#1906stephanieg467 wants to merge 2 commits into
stephanieg467 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughEight Archon default workflow YAML files update their ChangesPR number resolution robustness in verify-pr-base
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/workflows/src/defaults/bundled-defaults.test.ts`:
- Around line 151-204: The test "verify-pr-base nodes should resolve and edit
PRs explicitly" is missing an assertion that an unresolved PR_NUMBER is
fail-fast; add a check that each block contains a guard like if [ -z
"$PR_NUMBER" ]; then ... exit 1 (or the equivalent using ${PR_NUMBER}) so the
workflow aborts when PR_NUMBER is unset. Locate the test by its name and the
getBundledVerifyPrBaseBlocks() call, and add a new failure condition (similar to
the existing checks using regex and artifactPrNumberIndex/branchFallbackIndex)
that verifies presence of the unset-PR guard and flags the workflowName when it
is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e16c5e96-b4a8-4259-93aa-f38756388d1e
📒 Files selected for processing (11)
.archon/workflows/defaults/archon-architect.yaml.archon/workflows/defaults/archon-feature-development.yaml.archon/workflows/defaults/archon-fix-github-issue.yaml.archon/workflows/defaults/archon-idea-to-pr.yaml.archon/workflows/defaults/archon-issue-review-full.yaml.archon/workflows/defaults/archon-piv-loop.yaml.archon/workflows/defaults/archon-plan-to-pr.yaml.archon/workflows/defaults/archon-ralph-dag.yaml.archon/workflows/defaults/archon-refactor-safely.yamlpackages/workflows/src/defaults/bundled-defaults.generated.tspackages/workflows/src/defaults/bundled-defaults.test.ts
stephanieg467
force-pushed
the
fix/verify-pr-base-resolution
branch
from
999a019 to
abe66c2
Compare
Use the stored .pr-number artifact before falling back to the current branch when verifying PR base branches. This avoids bare gh pr view resolving through an incorrect upstream such as origin/master. Regenerate bundled defaults and add regression coverage for explicit PR resolution in verify-pr-base nodes.
stephanieg467
force-pushed
the
fix/verify-pr-base-resolution
branch
from
abe66c2 to
2f39a69
Compare
…NUMBER guard Adds a regex check to the 'verify-pr-base nodes should resolve and edit PRs explicitly' test that flags any block missing an `if [ -z "$PR_NUMBER" ]; then … exit 1` guard, addressing the CodeRabbit inline review comment on PR coleam00#1906. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Describe this PR in 2-5 bullets:
verify-pr-basenodes in bundled workflows used baregh pr view, allowing GitHub CLI to infer the PR from branch/remote state.verify-pr-basenow resolves the PR explicitly from$ARTIFACTS_DIR/.pr-number, falls back to the current branch, and uses that PR number for both view and edit operations.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory (list every module-to-module edge, mark changes):
gh pr view/edit$PR_NUMBER.archon-create-prartifactverify-pr-basenode.pr-numberis the preferred PR identity source.bundled-defaults.generated.tsbundled-defaults.test.tsverify-pr-baseblocks.Label Snapshot
risk: lowsize: Sworkflows|testsworkflows:defaultsChange Metadata
bugworkflowsLinked Issue
Validation Evidence (required)
Commands and result summary:
verify-pr-baseblock and fails on baregh pr view --json baseRefName, missing.pr-numberlookup, missing branch fallback, or missing explicit$PR_NUMBERview/edit.Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, describe risk and mitigation: N/ACompatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoHuman Verification (required)
What was personally validated beyond CI:
verify-pr-basenodes..pr-numberartifact falls back to current-branch PR lookup; failure path emits an explicit error if no PR can be resolved.Side Effects / Blast Radius (required)
verify-pr-base; generated bundled defaults; bundled defaults tests..pr-numbernor current-branch PR lookup can resolve a PR.gh pr viewreintroduction in bundledverify-pr-basenodes.Rollback Plan (required)
verify-pr-basenode errors while resolving$PR_NUMBER, or PR base verification stops before review/sync.Risks and Mitigations
.pr-numberartifact and no PR associated with the current branch.Summary by CodeRabbit
Bug Fixes
.pr-numberartifact first, otherwise falling back to the current-branch lookup), failing fast when it can’t be determined, and retargeting the PR base only when it differs from the expected branch. Also refined success/error messaging and hardened shell execution behavior.Tests