-
Notifications
You must be signed in to change notification settings - Fork 424
safe-outputs: validate extracted base branch with git check-ref-format refs/heads/<name>
#40001
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |
| import { describe, it, expect } from "vitest"; | ||
| import fs from "fs"; | ||
| import path from "path"; | ||
| import { extractBaseBranchFromAgentOutput, isSameWorkflowRepo } from "./extract_base_branch_from_agent_output.cjs"; | ||
| import { extractBaseBranchFromAgentOutput, isSameWorkflowRepo, isValidBaseBranchName } from "./extract_base_branch_from_agent_output.cjs"; | ||
|
|
||
| describe("extract_base_branch_from_agent_output", () => { | ||
| it("matches fully-qualified repos", () => { | ||
|
|
@@ -70,4 +70,28 @@ describe("extract_base_branch_from_agent_output", () => { | |
| fs.rmSync(tmpDir, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
|
|
||
| it("accepts valid git branch names used in safe outputs", () => { | ||
| expect(isValidBaseBranchName("feature/x")).toBe(true); | ||
| expect(isValidBaseBranchName("release/v1.2+hotfix")).toBe(true); | ||
| }); | ||
|
|
||
| it("rejects invalid git branch names even if they look regex-safe", () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing 💡 Suggested fixAdd 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 —
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in the latest commit — |
||
| expect(isValidBaseBranchName("foo..bar")).toBe(false); | ||
| expect(isValidBaseBranchName("main.lock")).toBe(false); | ||
| expect(isValidBaseBranchName(".foo")).toBe(false); | ||
| expect(isValidBaseBranchName("foo/.bar")).toBe(false); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The 💡 Suggested testit('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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a boundary test in the latest commit: 255-char name accepted, 256-char name rejected. |
||
|
|
||
| it("rejects git branch expressions (@{-N} notation)", () => { | ||
| expect(isValidBaseBranchName("@{-1}")).toBe(false); | ||
| expect(isValidBaseBranchName("@{-2}")).toBe(false); | ||
| }); | ||
|
|
||
| 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); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/diagnose] When
gitis not on PATH,result.erroris set (ENOENT) and the function returnsfalse, 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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.