fix(safe_outputs): skip cross-repo items in extract-base-branch to prevent checkout failure#35174
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ckout failure Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #35174 does not have the 'implementation' label and has only 14 new lines of code in business logic directories (well below the 100-line threshold). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
🧪 Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose, /tdd, and /zoom-out — the fix is correct and well-targeted; commenting rather than blocking on two test-coverage suggestions and one unrelated change to clarify.
📋 Key Themes & Highlights
Key Themes
- Test coverage is structural, not behavioural — the new assertions check that the generated script contains the right strings, but no test runs the script with mixed-repo input to verify the filter works at runtime.
- Silent fallback — when
GITHUB_REPOSITORYis empty the fix correctly excludes cross-repo items, but does so silently; a missed-branch scenario could be hard to diagnose in production. - Unrelated cron change — the lock file diff includes a schedule change (daily → weekly-on-Monday) that isn't described in the PR.
Positive Highlights
- ✅ Root cause is correctly identified and precisely fixed with a minimal guard.
- ✅ The safe-fallback design (exclude when env is empty) is a reasonable defensive choice.
- ✅ Code comment updated to document the cross-repo skipping rationale.
- ✅ Changeset entry is accurate and appropriately scoped as a patch.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.2M
| // from being used to checkout the workflow repo where that branch doesn't exist. | ||
| assert.Contains(t, stepsContent, "GITHUB_REPOSITORY", "must read workflow repo from GITHUB_REPOSITORY to filter cross-repo items") | ||
| assert.Contains(t, stepsContent, "!i.repo || (workflowRepo && i.repo === workflowRepo)", "must skip items whose repo differs from the workflow repo") | ||
| } |
There was a problem hiding this comment.
[/tdd] The new assertions verify the structure of the generated code (string presence) but don't exercise the runtime behaviour — no test feeds an agent_output.json with mixed same-repo and cross-repo items to confirm the filter actually selects the right one.
💡 Suggested additional test approach
Consider a table-driven test that invokes the generated Node.js snippet with a temp JSON file containing:
- A cross-repo item (
repo: "other-org/other-repo") — should be skipped. - A same-repo item (
repo: "GITHUB_REPOSITORY value") — should be selected. - An item with no
repofield — should be selected. - All items are cross-repo,
GITHUB_REPOSITORYis empty — should produce no output.
This turns the structural assertion into a behaviour assertion and would have caught the original bug as a failing test before the fix.
| " const item = (data.items || []).find(i =>\n", | ||
| " (i.type === 'create_pull_request' || i.type === 'push_to_pull_request_branch') &&\n", | ||
| " i.base_branch\n", | ||
| " i.base_branch &&\n", |
There was a problem hiding this comment.
[/diagnose] The empty-GITHUB_REPOSITORY fallback (workflowRepo && i.repo === workflowRepo) excludes all items that have an explicit repo field when GITHUB_REPOSITORY is unset. That's documented as a "safe fallback", but it silently produces an empty base-branch output — the downstream checkout step would then fall back to the default branch with no warning.
💡 Suggestion
Consider emitting a warning to stderr when workflowRepo is empty and a repo-bearing item is skipped, so operators can diagnose the silent fallback:
const workflowRepo = process.env.GITHUB_REPOSITORY || '';
if (!workflowRepo) { process.stderr.write('warning: GITHUB_REPOSITORY is unset; cross-repo items will be excluded\n'); }Alternatively, add a comment in the generated snippet explaining the intentional fallback for future readers.
| schedule: | ||
| - cron: "19 2 * * *" | ||
| # Friendly format: daily (scattered) | ||
| - cron: "6 3 * * 1" |
There was a problem hiding this comment.
[/zoom-out] The cron schedule changed from daily (19 2 * * *) to weekly-on-Monday (6 3 * * 1). This is unrelated to the cross-repo checkout fix and isn't mentioned in the PR description. If intentional, it should be explained (or split into a separate PR) so reviewers don't assume it's a side-effect of the lock file regeneration.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a failure mode in the consolidated safe_outputs apply job where extract-base-branch could mistakenly extract base_branch from safe-output items targeting other repositories, leading to a checkout attempt of a non-existent branch in the workflow repo.
Changes:
- Update
extract-base-branchgeneration to ignore safe-output entries whoserepodiffers fromGITHUB_REPOSITORY. - Extend the unit test to assert the new
GITHUB_REPOSITORY-based filtering is present in the generated script. - Regenerate the
daily-compiler-threat-spec-optimizercompiled lock workflow and add a changeset entry for the patch release.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_safe_outputs_steps.go | Adds cross-repo filtering to base-branch extraction logic. |
| pkg/workflow/compiler_safe_outputs_steps_test.go | Updates test assertions to cover the new filter and env var usage. |
| .github/workflows/daily-compiler-threat-spec-optimizer.lock.yml | Regenerated compiled workflow output (schedule/config/materialized heredocs). |
| .changeset/patch-fix-cross-repo-base-branch-checkout.md | Adds a patch changeset describing the fix. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
| " const workflowRepo = process.env.GITHUB_REPOSITORY || '';\n", | ||
| " const item = (data.items || []).find(i =>\n", | ||
| " (i.type === 'create_pull_request' || i.type === 'push_to_pull_request_branch') &&\n", | ||
| " i.base_branch\n", | ||
| " i.base_branch &&\n", | ||
| " (!i.repo || (workflowRepo && i.repo === workflowRepo))\n", | ||
| " );\n", |
| // This prevents a cross-repo base_branch (e.g. feature/FOO-123 in another repo) | ||
| // from being used to checkout the workflow repo where that branch doesn't exist. | ||
| assert.Contains(t, stepsContent, "GITHUB_REPOSITORY", "must read workflow repo from GITHUB_REPOSITORY to filter cross-repo items") | ||
| assert.Contains(t, stepsContent, "!i.repo || (workflowRepo && i.repo === workflowRepo)", "must skip items whose repo differs from the workflow repo") |
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.4M
|
@copilot review all comments and review from GitHub-actions. Merge main |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed.
|
There was a problem hiding this comment.
@copilot refactor script to actions/setup/js .cjs file and convert to actions/github-script
There was a problem hiding this comment.
Implemented in bd677ee.
- Refactored extract-base-branch logic into
actions/setup/js/extract_base_branch_from_agent_output.cjs. - Converted the step to
actions/github-scriptand wired it throughsetup_globals.cjs. - Added JS coverage for repo normalization (including bare repo-name matching) and updated compiler step test assertions for the new wiring.
There was a problem hiding this comment.
Me saw follow-up. Smoke reply here. Thanks for fix.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · gpt55 8.6M
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in
|
|
🚀 Smoke Gemini MISSION COMPLETE! Gemini has spoken. ✨ Testing safeoutputs CLI |
|
🚀 Smoke Antigravity MISSION COMPLETE! Antigravity has spoken. ✨ |
|
🌑 The shadows whisper... Smoke Codex failed to deliver outputs. The oracle requires further meditation... |
|
💫 TO BE CONTINUED... Smoke Claude was cancelled! Our hero faces unexpected challenges... |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🚀 Smoke Pi MISSION COMPLETE! Pi delivered. 🥧 |
Smoke Test: Gemini
Overall Status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
|
Smoke Copilot: FAIL Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Smoke review done. Me see no blocker. Tool checks found environment gaps outside diff.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · gpt55 8.6M
| if (!workflowRepo) return false; | ||
| if (itemRepo === workflowRepo) return true; | ||
|
|
||
| // Safe-output repo values may be a bare repo name and get qualified at runtime. |
There was a problem hiding this comment.
Me check repo match helper. Bare repo path now guarded. Good smoke line.
| async function main() { | ||
| const baseBranch = extractBaseBranchFromAgentOutput(); | ||
| if (!baseBranch) return; | ||
| if (!SAFE_BRANCH_NAME_REGEX.test(baseBranch) || baseBranch.length > 255) return; |
There was a problem hiding this comment.
Me see branch validation still before output. Good guard stay alive after refactor.
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
In multi-repo
create-pull-requestworkflows (e.g.allowed-repos: ["org/*"]), theextract-base-branchstep was picking upbase_branchfrom items targeting other repos. That branch (e.g.feature/FOO-123) doesn't exist in the workflow repo, causing theCheckout repositorystep to fail with agit fetchexit code 1.Changes
pkg/workflow/compiler_safe_outputs_steps.go—buildExtractBaseBranchStepwas refactored to useactions/github-scriptand delegate extraction logic to a setup JS helper instead of inline bash/node script.actions/setup/js/extract_base_branch_from_agent_output.cjs— Added helper that:/tmp/gh-aw/agent_output.jsonGITHUB_REPOSITORYowner/repo) and bare repo names (repo) by matching repo-name suffixbase-branchoutputpkg/workflow/compiler_safe_outputs_steps_test.go— UpdatedTestBuildExtractBaseBranchStepassertions for the newactions/github-scriptwiring and helper usage.actions/setup/js/extract_base_branch_from_agent_output.test.cjs— Added JS unit tests for repo matching/normalization and branch extraction behavior, including bare repo-name matching.