Handle base-boundary commits in signed safe-output PR replay#35578
Conversation
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>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add integration git tests |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added integration git test coverage in The new test extends the boundary-case replay scenario to multiple commits and verifies we drop the base-boundary entry, replay only new commits in order, create the branch from the base OID, and avoid |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #35578 does not have the 'implementation' label and has 0 new lines of code in default business logic directories (threshold is 100). |
|
🧪 Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Fixes an edge case in the signed-commit replay path where git rev-list --parents <base>..HEAD can include the base “boundary” commit (notably in shallow/bundle flows), causing incorrect parent resolution and cascading fallbacks.
Changes:
- Resolve
baseRefto an OID once, then drop any rev-list entries whose SHA equals that base OID so the base boundary commit is not replayed. - When creating a new remote branch, prefer the resolved base OID as the initial
expectedHeadOid(falling back to per-commit parent resolution if base resolution fails). - Add integration coverage for injected boundary-style rev-list output, multi-commit ordering, and ensuring no boundary-parent resolution is attempted.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/push_signed_commits.cjs | Resolves base ref OID, filters boundary entries from replay set, and uses base OID for new-branch bootstrap expected head when available. |
| actions/setup/js/push_signed_commits.test.cjs | Adds integration tests that inject boundary-like rev-list output and validate correct replay behavior and OID chaining. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
![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 — the fix is correct and well-motivated; two observations worth addressing.
📋 Key Themes & Highlights
Key Themes
- TOCTOU window (minor):
rev-listis called beforebaseRefis resolved to an OID. Resolving the OID first and passing it as the range literal (<oid>..HEAD) is both safer and eliminates the need for the post-hoc filter entirely. - Untested fallback path: the
catchbranch that handles arev-parse baseReffailure has no dedicated test. The fallback torev-parse <sha>^is real production code with no regression guard.
Positive Highlights
- ✅ Root cause correctly identified and addressed at the right abstraction level
- ✅ Good defensive
try/catchwith a descriptive warning rather than a hard failure - ✅ Struct-based refactor of
revListEntries({ sha, fields, line }) is cleaner than the previous parallel arrays - ✅ Two new integration tests cover single-boundary drop, multi-commit ordering, chained
expectedHeadOid, and explicitly assertrev-parse <base>^is never attempted on the boundary SHA — exactly the right regression targets
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.1M
| .trim() | ||
| .split("\n") | ||
| .filter(Boolean) | ||
| .map(line => { |
There was a problem hiding this comment.
[/diagnose] Minor TOCTOU risk: rev-list runs first, then baseRef is resolved to an OID. In a concurrent environment (e.g. force-push racing the workflow), the base ref could advance between the two calls, and the resolved OID might not match what was actually used to bound the rev-list. Resolving baseRef → OID first and then passing that OID literal to rev-list (e.g. git rev-list ... <oid>..HEAD) eliminates the window entirely.
💡 Suggested reorder
// 1. Resolve base OID first
const { stdout: baseRefOut } = await exec.getExecOutput("git", ["rev-parse", baseRef], { cwd });
const baseRefOid = baseRefOut.trim() || undefined;
// 2. Use the OID literal as the range boundary (no race)
const rangeSpec = baseRefOid ?? `${baseRef}`;
const { stdout: revListOut } = await exec.getExecOutput(
"git", ["rev-list", "--parents", "--topo-order", "--reverse", `${rangeSpec}..HEAD`], { cwd });
// No filter needed — the OID literal already excludes the base commit.This also removes the need for the post-hoc SHA filter and the droppedBoundaryCount diagnostic (the boundary commit simply won't appear in the output).
| expect(committedHeadlines).toEqual(["Add boundary-alpha.txt", "Add boundary-beta.txt"]); | ||
|
|
||
| const attemptedBoundaryParentResolution = global.exec.getExecOutput.mock.calls.some(([program, args]) => program === "git" && args[0] === "rev-parse" && args[1] === `${baseRefOid}^`); | ||
| expect(attemptedBoundaryParentResolution).toBe(false); |
There was a problem hiding this comment.
[/tdd] Missing test: the catch branch where git rev-parse baseRef throws is never exercised. The fallback path (baseRefOid === undefined) takes a different branch-bootstrap route (rev-parse <sha>^) whose correctness is only tested indirectly by pre-existing tests. A dedicated test that injects an exec error for rev-parse <baseRef> and asserts the warning is logged — and that the function still succeeds via rev-parse <sha>^ — would lock in that safety net.
💡 Sketch
it('should fall back to rev-parse parent when baseRef OID resolution fails', async () => {
// Arrange: branch with one commit, make rev-parse baseRef throw
const realExec = makeRealExec(workDir);
global.exec = {
...realExec,
getExecOutput: vi.fn(async (program, args, opts = {}) => {
if (program === 'git' && args[0] === 'rev-parse' && args[1] === 'origin/main') {
throw new Error('simulated rev-parse failure');
}
return realExec.getExecOutput(program, args, opts);
}),
};
// Act: push should still succeed via the fallback path
await pushSignedCommits({ ... });
// Assert: warning logged, function succeeded
expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining('could not resolve baseRef'));
expect(githubClient.graphql).toHaveBeenCalledTimes(1);
});
There was a problem hiding this comment.
REQUEST_CHANGES — Two correctness issues and one observability gap.
### Blocking issues found
1. Annotated-tag OID mismatch (HIGH) — git rev-parse <tag> returns the tag object OID, not the commit OID. rev-list outputs commit SHAs. The boundary filter compares these two values, so it is always a no-op when baseRef is an annotated tag. The boundary commit leaks into the replay set and reproduces the original bug. Fix: use rev-parse ${baseRef}^{commit}.
2. No SHA format validation (MEDIUM) — trim() does not guard against embedded newlines from ambiguous refs or multi-line git rev-parse output. The raw string is used verbatim as a GraphQL expectedHeadOid, producing an opaque API error. Fix: validate against /^[0-9a-f]{40}$/ before accepting.
3. Silent fallback does not flag degraded state (MEDIUM) — When rev-parse throws, baseRefOid stays undefined and the boundary filter is silently skipped. The current warning message does not communicate that the fix is inactive for this run. Inline comment has a suggested message update.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 2M
| /** @type {string | undefined} */ | ||
| let baseRefOid; | ||
| try { | ||
| const { stdout: baseRefOut } = await exec.getExecOutput("git", ["rev-parse", baseRef], { cwd }); |
There was a problem hiding this comment.
Tag-object OID mismatch: filter is a no-op when baseRef is an annotated tag. git rev-parse on an annotated tag returns the tag object OID, not the commit OID. git rev-list always emits commit SHAs. These two OIDs never match, so the boundary filter silently does nothing — the boundary commit leaks into the replay set and the GraphQL mutation fails.
💡 Suggested fix
Dereference the tag to a commit OID with ^{commit} before assigning:
const { stdout: baseRefOut } = await exec.getExecOutput(
"git", ["rev-parse", `${baseRef}^{commit}`], { cwd }
);^{commit} is a no-op for commits and lightweight tags, but correctly peels annotated tags to their underlying commit. Without this, any baseRef set to an annotated tag (v1.0.0) defeats the entire boundary-filtering mechanism and reproduces the original bug.
| try { | ||
| const { stdout: baseRefOut } = await exec.getExecOutput("git", ["rev-parse", baseRef], { cwd }); | ||
| const trimmedBaseRefOid = baseRefOut.trim(); | ||
| if (trimmedBaseRefOid) { |
There was a problem hiding this comment.
No SHA format validation: ambiguous baseRef can produce multi-line or non-hex git rev-parse output that gets used verbatim as a GraphQL OID. trim() strips trailing whitespace but not embedded newlines. If baseRef matches multiple refs (e.g., both a tag and a branch), rev-parse emits multiple lines, and the concatenated result is passed as expectedHeadOid to the GitHub API, producing an opaque mutation error with no local context.
💡 Suggested fix
Validate the trimmed value is a 40-char hex SHA before accepting it:
const trimmedBaseRefOid = baseRefOut.trim();
if (/^[0-9a-f]{40}$/.test(trimmedBaseRefOid)) {
baseRefOid = trimmedBaseRefOid;
} else {
core.warning(
`pushSignedCommits: git rev-parse for baseRef returned unexpected output (${JSON.stringify(trimmedBaseRefOid)}); skipping boundary filter`
);
}This catches multi-line output, partial SHAs, and SHA-256 object hashes (64 chars) before they reach the GitHub API.
| } | ||
| } catch (baseRefResolveError) { | ||
| core.warning( | ||
| `pushSignedCommits: could not resolve baseRef '${baseRef}' to OID; will use per-commit parent resolution if branch creation needs it: ${baseRefResolveError instanceof Error ? baseRefResolveError.message : String(baseRefResolveError)}` |
There was a problem hiding this comment.
Silent fallback on rev-parse failure quietly disables the boundary filter and re-exposes the original bug. When git rev-parse baseRef throws (stale ref, shallow clone missing the ref, typo), baseRefOid stays undefined, the boundary filter is skipped entirely, and execution falls back to per-commit sha^ parent resolution — the exact path that triggered the issue this PR is fixing. The warning message does not flag this clearly.
💡 Suggested fix
Explicitly flag that the boundary filter is inactive in the warning:
} catch (baseRefResolveError) {
core.warning(
`pushSignedCommits: could not resolve baseRef OID — boundary-commit filter is DISABLED for this run. ` +
`If boundary commits appear in rev-list output the GraphQL replay may fail. ` +
`Error: ${baseRefResolveError instanceof Error ? baseRefResolveError.message : String(baseRefResolveError)}`
);
}This makes the degraded state visible in run logs instead of appearing as graceful pass-through behaviour.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (2 tests 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.
|
|
@copilot Fix all reviews and comments from copilot and GitHub-actions actor |
|
|
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>
Addressed the actionable Copilot/GitHub Actions review feedback in |
|
|
Safe-output PR creation could incorrectly replay the base-branch boundary commit when
git rev-list --parents <base>..HEADincluded it (seen in shallow/bundle flows). That caused parent resolution to fail (<sha>^), triggered unsigned push fallback, and then degraded into fallback issue creation under signed-commit branch rules.Root-cause correction in signed replay path
baseRefto its commit OID once at replay start (rev-parse <baseRef>^{commit}).Deterministic branch bootstrap for new remote branches
expectedHeadOidfor branch creation.422 already exists) races, refresh remote branch OID and use it asexpectedHeadOid.Operational diagnostics
Regression coverage
rev-listoutput and verify only actual new commits are replayed.expectedHeadOidbehavior, and verification that boundary SHAs are not used for parent-resolution (rev-parse <base>^).