safe-outputs: validate extracted base branch with git check-ref-format refs/heads/<name>#40001
Conversation
|
Hey A few things to address before this is ready for review:
If you'd like a hand completing the implementation, here's a ready-to-use prompt:
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
git check-ref-format --branch
There was a problem hiding this comment.
Pull request overview
Aligns safe-outputs base-branch validation with Git’s canonical ref rules by switching from a regex to git check-ref-format --branch, ensuring workflows neither accept invalid branch names nor reject valid ones (e.g., release/v1.2+hotfix) when extracting base_branch from agent_output.json.
Changes:
- Replaced regex-based branch validation with
git check-ref-format --branchin theextract-base-branchsetup script (keeping the 255-char guard). - Added focused vitest coverage for previously problematic “valid but regex-rejected” and “regex-safe but Git-invalid” branch names.
- Updated ADR-30071 normative wording to specify
git check-ref-format --branchsemantics.
Show a summary per file
| File | Description |
|---|---|
| docs/adr/30071-decouple-safe-outputs-base-branch-from-event-context.md | Updates the normative spec to require git check-ref-format --branch semantics (plus max length). |
| actions/setup/js/extract_base_branch_from_agent_output.cjs | Implements Git-native validation via git check-ref-format --branch and exports the validator for testing. |
| actions/setup/js/extract_base_branch_from_agent_output.test.cjs | Adds unit tests covering representative accepted/rejected branch-name cases motivating the change. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40001 does not have the 'implementation' label (has_implementation_label=false) and has 0 new lines of code in business logic directories (≤100 threshold). Both enforcement conditions are unmet. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
Go: 0 ( Proportional growth: test file +13 lines vs. production +17 lines (ratio 0.76 — no inflation). Verdict
|
![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 and /tdd — approving with minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Robustness gap:
spawnSynclacks atimeoutoption; a hunggitprocess would block the step indefinitely (low probability but easy to fix). - Missing boundary test:
MAX_BRANCH_NAME_LENGTH = 255is enforced in code but the 255/256 boundary is never exercised by the test suite. - Silent fail-closed:
gitunavailability causes silent branch rejection — correct behavior, but worth a one-line comment for future readers.
Positive Highlights
- ✅ Root cause properly addressed: regex replaced with
git check-ref-format --branch, the canonical validator — eliminates both false accepts (foo..bar,main.lock) and false rejects (release/v1.2+hotfix). - ✅ Correct use of
--branchflag prevents a leading-hyphen argument from being misinterpreted as a git option (no command-injection risk). - ✅
isValidBaseBranchNameexported and unit-tested, making regression coverage precise and maintainable. - ✅ ADR-30071 normative wording updated in lockstep with the implementation — prevents future regex drift.
- ✅ Fail-closed on subprocess error (
!result.error && result.status === 0) is the safe default.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer
| return false; | ||
| } | ||
|
|
||
| const result = spawnSync("git", ["check-ref-format", "--branch", branchName], { stdio: "ignore" }); |
There was a problem hiding this comment.
[/diagnose] spawnSync has no timeout option — if git stalls (degraded runner, network-mounted git install), validation will block the step indefinitely.
💡 Suggested fix
Add a timeout (e.g. 5 seconds) so the function stays bounded:
const result = spawnSync("git", ["check-ref-format", "--branch", branchName], {
stdio: "ignore",
timeout: 5000,
});With timeout set, a hung subprocess is killed and result.error will be set (ETIMEDOUT), so the existing !result.error guard safely returns false.
There was a problem hiding this comment.
Added timeout: 5000 to the spawnSync call in the latest commit.
| expect(isValidBaseBranchName("main.lock")).toBe(false); | ||
| expect(isValidBaseBranchName(".foo")).toBe(false); | ||
| expect(isValidBaseBranchName("foo/.bar")).toBe(false); | ||
| }); |
There was a problem hiding this comment.
[/tdd] The MAX_BRANCH_NAME_LENGTH = 255 invariant is enforced in code but never exercised by a test — the exact boundary (255 valid, 256 rejected) is not confirmed.
💡 Suggested test
it('enforces the 255-character length limit', () => {
const atLimit = 'a'.repeat(255);
const overLimit = 'a'.repeat(256);
expect(isValidBaseBranchName(atLimit)).toBe(true);
expect(isValidBaseBranchName(overLimit)).toBe(false);
});Without a boundary test, a future refactor that changes or removes the guard goes undetected by the suite.
There was a problem hiding this comment.
Added a boundary test in the latest commit: 255-char name accepted, 256-char name rejected.
| } | ||
|
|
||
| const result = spawnSync("git", ["check-ref-format", "--branch", branchName], { stdio: "ignore" }); | ||
| return !result.error && result.status === 0; |
There was a problem hiding this comment.
[/diagnose] When git is not on PATH, result.error is set (ENOENT) and the function returns false, silently dropping the base branch. This is the safe fail-closed behavior, but it is worth a brief comment so the next reader does not treat it as a bug.
💡 Suggested comment
// Fail-closed: if git is unavailable or the ref is invalid, reject the branch.
// A missing git binary sets result.error (ENOENT); a timeout sets it (ETIMEDOUT).
return !result.error && result.status === 0;The ADR already documents the conservative-rejection intent, but a one-liner here saves a future reader from needing to cross-reference it.
There was a problem hiding this comment.
Added an inline comment in the latest commit explaining ENOENT (missing git) and ETIMEDOUT (timeout) both set result.error, making the guard fail-closed in both cases.
There was a problem hiding this comment.
REQUEST_CHANGES — --branch introduces a correctness regression: git special expressions like @{-1} now pass validation and can reach checkout steps as relative ref expressions, not literal branch names.
Details
Root cause
git check-ref-format --branch accepts @{-N} notation by design (it is a valid branch expression meaning the N-th previously-checked-out branch). The old regex ^[a-zA-Z0-9/_.-]+$ rejected @{-1} because @, {, } are not in the allowed character set — an accidental but correct side-effect. This PR removes that guard.
Verified on the runner:
git check-ref-format --branch "@{-1}" → exit 0 (PASSES — bug)
git check-ref-format "refs/heads/@{-1}" → exit 1 (rejects — correct)
Fix
Switch to refs/heads/<branchName> instead of --branch. This validates the string as a literal branch name, accepts release/v1.2+hotfix, and still rejects foo..bar, .foo, etc. Also add @{-1} / @{-2} assertions to the test suite to pin the behaviour.
🔎 Code quality review by PR Code Quality Reviewer
| return false; | ||
| } | ||
|
|
||
| const result = spawnSync("git", ["check-ref-format", "--branch", branchName], { stdio: "ignore" }); |
There was a problem hiding this comment.
--branch causes @{-N} git expressions to pass validation, silently allowing values like @{-1} through to core.setOutput where they would be interpreted as relative branch references, not literal names.
💡 Suggested fix
git check-ref-format --branch is designed for branch expressions, not just literal branch names. It explicitly accepts @{-N} notation (e.g. @{-1} = previously-checked-out branch). Confirmed on this runner:
$ git check-ref-format --branch "@{-1}" → exit 0 ← BUG: passes
$ git check-ref-format "refs/heads/@{-1}" → exit 1 ← correct: rejects
The old regex ^[a-zA-Z0-9/_.-]+$ accidentally blocked this because @, {, and } were not in the allowed character set. This PR reopens that hole.
Use the full refs/heads/<branchName> form instead — it validates the name as a literal ref without accepting expressions:
const result = spawnSync("git", ["check-ref-format", `refs/heads/${branchName}`], { stdio: "ignore" });This still accepts release/v1.2+hotfix (→ refs/heads/release/v1.2+hotfix exits 0) and still rejects foo..bar, .foo, main.lock, etc.
There was a problem hiding this comment.
Fixed in the latest commit. Switched to refs/heads/${branchName} — this validates the name as a literal ref and correctly rejects @{-1}, @{-2}, and similar expressions. The --branch form has been removed.
| expect(isValidBaseBranchName("release/v1.2+hotfix")).toBe(true); | ||
| }); | ||
|
|
||
| it("rejects invalid git branch names even if they look regex-safe", () => { |
There was a problem hiding this comment.
Missing @{-N} test cases: the current test suite never exercises @{-1} or similar git expressions, so the regression in the validator is not caught.
💡 Suggested fix
Add to the "rejects invalid" block:
expect(isValidBaseBranchName("@{-1}")).toBe(false);
expect(isValidBaseBranchName("@{-2}")).toBe(false);With the current implementation these assertions will fail (confirming the bug — @{-1} passes the --branch validator). After switching to refs/heads/<branchName>, they will correctly pass as expected rejections and prevent future regressions.
There was a problem hiding this comment.
Added in the latest commit — @{-1} and @{-2} are now asserted to return false, confirming the refs/heads/ fix closes the gap.
|
@copilot run pr-finisher skill |
…ent, add boundary tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
git check-ref-format --branchgit check-ref-format refs/heads/<name>
The auto-injected
extract-base-branchstep used a regex that diverged from Git ref rules, accepting invalid branch names and rejecting valid ones (for examplerelease/v1.2+hotfix). This change aligns extraction-time validation with Git's canonical branch validation semantics.Validation semantics
extract_base_branch_from_agent_output.cjswithgit check-ref-format refs/heads/<name>.refs/heads/form (not--branch) to validate as a literal ref, preventing@{-N}git branch expressions (e.g.@{-1}) from passing validation.timeout: 5000so validation stays bounded ifgitstalls on a degraded runner.<=255length guard.Behavioral coverage
feature/x,release/v1.2+hotfixfoo..bar,main.lock,.foo,foo/.bar,@{-1},@{-2}Spec alignment
git check-ref-format refs/heads/<name>semantics.