ci: resolve PR merge conflicts with Pi#7556
Merged
Merged
Conversation
cv
added
the
area: ci
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds GitHub Actions automation and supporting tools to discover same-repository conflicts, resolve them with a sandboxed Pi task, validate binary patches, and publish verified merge commits. Tests cover Git behavior, publication safeguards, workflow boundaries, and sandbox policy. ChangesPR conflict resolution automation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant discoverConflicts
participant OpenShellPi
participant publishResolution
participant GitHubAPI
GitHubActions->>discoverConflicts: scan open conflicts
discoverConflicts-->>GitHubActions: conflict matrix
GitHubActions->>OpenShellPi: run sandboxed resolution
OpenShellPi-->>GitHubActions: upload resolution patch
GitHubActions->>publishResolution: download patch artifact
publishResolution->>GitHubAPI: validate state and create merge commit
GitHubAPI-->>publishResolution: update head ref
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit c2117da in the TypeScript / code-coverage/cliThe overall coverage in commit c2117da in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: None 2 optional E2E recommendations
1 warning · 0 suggestionsWarningsWarnings do not block.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
tools/pr-merge-conflict-fixer/publish.mts (1)
359-361: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReading the whole patch just to check existence.
The buffer is discarded. Use
statSyncand, since the patch is sandbox-produced untrusted output, reject implausibly large files up front.♻️ Proposed change
- readFileSync(patchPath); + const patchStats = statSync(patchPath); + if (patchStats.size > 8 * 1024 * 1024) { + throw new ConflictFixerError("The resolution patch is unexpectedly large"); + }Update the import:
import { mkdtempSync, rmSync, statSync } from "node:fs";🤖 Prompt for 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. In `@tools/pr-merge-conflict-fixer/publish.mts` around lines 359 - 361, Replace the discarded readFileSync call near artifactDirectory and patchPath with statSync to check the patch exists without loading its contents. Validate the resulting file size and reject implausibly large sandbox-produced patches before continuing, and update the node:fs import accordingly.test/pr-merge-conflict-fixer.test.ts (2)
35-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIsolate the fixture from ambient Git config for determinism.
merge.conflictStyle,core.autocrlf, hook paths, or init templates from a contributor's global config can change conflict output and flake these assertions.♻️ Pin global/system config off
function git(repository: string, args: string[]): string { return execFileSync("git", args, { cwd: repository, encoding: "utf8", + env: { ...process.env, GIT_CONFIG_GLOBAL: "/dev/null", GIT_CONFIG_SYSTEM: "/dev/null" }, stdio: ["ignore", "pipe", "pipe"], }).trim(); }🤖 Prompt for 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. In `@test/pr-merge-conflict-fixer.test.ts` around lines 35 - 58, Update createConflictFixture and its git helper setup to isolate the temporary repository from ambient global and system Git configuration, disabling inherited settings such as merge.conflictStyle, core.autocrlf, hooks, and init templates while preserving the existing repository-specific configuration and fixture behavior.
189-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for malformed patches and unresolved index entries.
The objectives call out malformed resolver output, but nothing exercises
applyResolutionPatchrejection or the "leaves unresolved index entries" branch invalidateResolutionPatch. Both are cheap to add with the existing fixture.🤖 Prompt for 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. In `@test/pr-merge-conflict-fixer.test.ts` around lines 189 - 202, Add tests using the existing conflict fixture to cover malformed resolver output rejected by applyResolutionPatch and validateResolutionPatch rejecting patches that leave unresolved index entries. Add focused cases alongside the existing non-conflict-path test, asserting each path throws the expected validation error while preserving current fixture setup.tools/pr-merge-conflict-fixer/merge.mts (1)
33-56: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTwo copies of the same Git exec helper, both using the default 1 MiB
maxBuffer.cat-file bloband-zpath listings can exceed that limit and throwENOBUFS, aborting resolution or publication.
tools/pr-merge-conflict-fixer/merge.mts#L33-L56: add an explicitmaxBuffertogitText/gitBufferand exportgitBuffer.tools/pr-merge-conflict-fixer/publish.mts#L120-L125: delete the localgitBufferand import the shared one frommerge.mts.🤖 Prompt for 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. In `@tools/pr-merge-conflict-fixer/merge.mts` around lines 33 - 56, The Git helpers use the default buffer limit and are duplicated across modules. In tools/pr-merge-conflict-fixer/merge.mts:33-56, add an explicit sufficiently large maxBuffer to gitText and gitBuffer, and export gitBuffer; in tools/pr-merge-conflict-fixer/publish.mts:120-125, remove the local gitBuffer and import the shared helper from merge.mts.tools/pr-merge-conflict-fixer/discover.mts (2)
132-150: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOne unfetchable PR head aborts the whole scan.
prepareMergethrows (viaensureCommit/clone failures) when a head SHA was force-pushed or deleted between listing and analysis. That exception propagates out ofcheckConflictand kills discovery for every remaining PR. Prefer logging and skipping the individual PR.♻️ Skip the failing PR instead of aborting discovery
try { const merge = prepareMerge( sourceRepository, mergeRepository, pullRequest.head.sha, baseSha, ); return merge?.conflictPaths ?? null; + } catch (error) { + console.warn( + `Skipping PR #${pullRequest.number}: ${error instanceof Error ? error.message : String(error)}`, + ); + return null; } finally { rmSync(temporaryDirectory, { force: true, recursive: true }); }🤖 Prompt for 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. In `@tools/pr-merge-conflict-fixer/discover.mts` around lines 132 - 150, Update the checkConflict callback passed to selectConflictingPullRequests so prepareMerge failures for an individual pull request are caught, logged, and skipped by returning null instead of propagating the exception. Preserve the existing conflict-path result and temporary-directory cleanup, using the available pull request context in the log.
103-122: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the pagination loop and add a request timeout.
The loop has no upper page bound and
fetchhas no timeout, so a stalled or misbehaving API response can hang or spin the scan job indefinitely.♻️ Proposed bound + timeout
- for (let page = 1; ; page += 1) { + const maxPages = 50; + for (let page = 1; page <= maxPages; page += 1) { const response = await request( `https://api.github.com/repos/${repository}/pulls?state=open&base=main&per_page=100&page=${page}`, { headers: { Accept: "application/vnd.github+json", Authorization: `Bearer ${token}`, "X-GitHub-Api-Version": "2022-11-28", }, + signal: AbortSignal.timeout(30_000), }, ); if (!response.ok) { throw new ConflictFixerError( `GitHub pull request listing failed with HTTP ${response.status}`, ); } const pageItems = (await response.json()) as PullRequest[]; pullRequests.push(...pageItems); if (pageItems.length < 100) return pullRequests; } + throw new ConflictFixerError("GitHub pull request listing exceeded the page limit"); }🤖 Prompt for 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. In `@tools/pr-merge-conflict-fixer/discover.mts` around lines 103 - 122, Bound the pagination loop in the pull-request discovery flow to a finite maximum page count, and add an abort-based timeout to the request in the existing request call. Preserve the current response validation and page accumulation behavior, while ensuring stalled requests and non-terminating pagination exit through the established error handling.test/pr-merge-conflict-fixer-workflow-boundary.test.ts (1)
19-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPipeline reports increased branching in this file (3
ifstatements, up from 0).The Codebase Growth Guardrails check flagged this file. The
record/checkoutguard clauses could share one throwing helper to cut the branch count:♻️ Proposed consolidation
+function required<T>(value: T | undefined, message: string): T { + if (!value) throw new Error(message); + return value; +} + function namedStep(job: Record<string, unknown>, name: string): Record<string, unknown> { - const step = steps(job).find((candidate) => candidate.name === name); - if (!step) throw new Error(`Missing workflow step: ${name}`); - return step; + return required( + steps(job).find((candidate) => candidate.name === name), + `Missing workflow step: ${name}`, + ); } function checkout(job: Record<string, unknown>): Record<string, unknown> { - const step = steps(job).find((candidate) => - String(candidate.uses ?? "").startsWith("actions/checkout@"), - ); - if (!step) throw new Error("Missing checkout step"); - return step; + return required( + steps(job).find((candidate) => String(candidate.uses ?? "").startsWith("actions/checkout@")), + "Missing checkout step", + ); }Verify against the repo's guardrail script whether this satisfies the threshold, or whether test helpers are meant to be exempted.
🤖 Prompt for 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. In `@test/pr-merge-conflict-fixer-workflow-boundary.test.ts` around lines 19 - 41, Reduce branching in the test helpers by consolidating the throwing guard logic used by record/checkout, while preserving their current fallback and error behavior. Inspect the repository’s Codebase Growth Guardrails script to confirm the required threshold and whether test helpers are exempt; only adjust these helpers if needed to satisfy the guardrail.Source: Pipeline failures
.github/workflows/pr-merge-conflict-fixer.yaml (1)
15-48: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding
timeout-minutestoscanandpublish.Only
resolvesets a job timeout (Line 48).scanandpublishcall the GitHub API (viadiscover.mts/publish.mts) and could hang up to the default 360-minute job limit if a network call stalls, tying up a runner unnecessarily.Also applies to: 187-199
🤖 Prompt for 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. In @.github/workflows/pr-merge-conflict-fixer.yaml around lines 15 - 48, Set explicit timeout-minutes values on the scan and publish jobs, matching the bounded execution policy already used by resolve. Add the timeouts directly to the job definitions that run discover.mts and publish.mts, using an appropriate limit that prevents stalled GitHub API calls from consuming the default runner duration.
🤖 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 @.github/workflows/pr-merge-conflict-fixer.yaml:
- Around line 54-61: Remove runner.temp expressions from the job-level env
blocks in the resolve and publish jobs. In the relevant job steps, write
ARTIFACT_DIR, RESOLUTION_WORKDIR, and RESOLVER_CONFIG_DIR to $GITHUB_ENV using
the RUNNER_TEMP environment variable, while preserving the existing resolved
directory values and publish job behavior.
In `@test/pr-merge-conflict-fixer.test.ts`:
- Around line 254-283: Replace the conditional request routing inside the mock
request function with a route map keyed by the matching API paths, preserving
each endpoint’s existing response behavior and dynamic values such as
entry.pr_number, entry.base_sha, finalTree, and commitSha. Keep the Git blob SHA
calculation unchanged, including createHash("sha1"), and retain the
unexpected-request error for unmatched routes.
- Around line 94-105: In the merge test around prepareMerge, assert that merge
is non-null before accessing conflictTree, then pass the narrowed
merge.conflictTree directly to git diff instead of using the empty-string
fallback. Remove or avoid conditionals that could let the test pass without
exercising the intended conflict-resolution claim.
---
Nitpick comments:
In @.github/workflows/pr-merge-conflict-fixer.yaml:
- Around line 15-48: Set explicit timeout-minutes values on the scan and publish
jobs, matching the bounded execution policy already used by resolve. Add the
timeouts directly to the job definitions that run discover.mts and publish.mts,
using an appropriate limit that prevents stalled GitHub API calls from consuming
the default runner duration.
In `@test/pr-merge-conflict-fixer-workflow-boundary.test.ts`:
- Around line 19-41: Reduce branching in the test helpers by consolidating the
throwing guard logic used by record/checkout, while preserving their current
fallback and error behavior. Inspect the repository’s Codebase Growth Guardrails
script to confirm the required threshold and whether test helpers are exempt;
only adjust these helpers if needed to satisfy the guardrail.
In `@test/pr-merge-conflict-fixer.test.ts`:
- Around line 35-58: Update createConflictFixture and its git helper setup to
isolate the temporary repository from ambient global and system Git
configuration, disabling inherited settings such as merge.conflictStyle,
core.autocrlf, hooks, and init templates while preserving the existing
repository-specific configuration and fixture behavior.
- Around line 189-202: Add tests using the existing conflict fixture to cover
malformed resolver output rejected by applyResolutionPatch and
validateResolutionPatch rejecting patches that leave unresolved index entries.
Add focused cases alongside the existing non-conflict-path test, asserting each
path throws the expected validation error while preserving current fixture
setup.
In `@tools/pr-merge-conflict-fixer/discover.mts`:
- Around line 132-150: Update the checkConflict callback passed to
selectConflictingPullRequests so prepareMerge failures for an individual pull
request are caught, logged, and skipped by returning null instead of propagating
the exception. Preserve the existing conflict-path result and
temporary-directory cleanup, using the available pull request context in the
log.
- Around line 103-122: Bound the pagination loop in the pull-request discovery
flow to a finite maximum page count, and add an abort-based timeout to the
request in the existing request call. Preserve the current response validation
and page accumulation behavior, while ensuring stalled requests and
non-terminating pagination exit through the established error handling.
In `@tools/pr-merge-conflict-fixer/merge.mts`:
- Around line 33-56: The Git helpers use the default buffer limit and are
duplicated across modules. In tools/pr-merge-conflict-fixer/merge.mts:33-56, add
an explicit sufficiently large maxBuffer to gitText and gitBuffer, and export
gitBuffer; in tools/pr-merge-conflict-fixer/publish.mts:120-125, remove the
local gitBuffer and import the shared helper from merge.mts.
In `@tools/pr-merge-conflict-fixer/publish.mts`:
- Around line 359-361: Replace the discarded readFileSync call near
artifactDirectory and patchPath with statSync to check the patch exists without
loading its contents. Validate the resulting file size and reject implausibly
large sandbox-produced patches before continuing, and update the node:fs import
accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bc19778c-f1f3-4052-b550-883777be0670
📒 Files selected for processing (9)
.github/workflows/pr-merge-conflict-fixer.yamltest/helpers/vitest-watch-triggers.tstest/pr-merge-conflict-fixer-workflow-boundary.test.tstest/pr-merge-conflict-fixer.test.tstools/pr-merge-conflict-fixer/discover.mtstools/pr-merge-conflict-fixer/merge.mtstools/pr-merge-conflict-fixer/policy.yamltools/pr-merge-conflict-fixer/publish.mtstools/pr-merge-conflict-fixer/resolve.mts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/pr-merge-conflict-fixer.test.ts (1)
50-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
required()helper across test files.The same
required<T>implementation appears verbatim intest/pr-merge-conflict-fixer-workflow-boundary.test.ts. Consider extracting it into a shared test helper (e.g. undertest/helpers/) to avoid the two copies drifting apart.🤖 Prompt for 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. In `@test/pr-merge-conflict-fixer.test.ts` around lines 50 - 55, Extract the duplicated required<T> helper from pr-merge-conflict-fixer.test.ts and pr-merge-conflict-fixer-workflow-boundary.test.ts into a shared test helper under test/helpers, then import and reuse that single implementation in both files while preserving its existing assertions and return behavior.
🤖 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.
Nitpick comments:
In `@test/pr-merge-conflict-fixer.test.ts`:
- Around line 50-55: Extract the duplicated required<T> helper from
pr-merge-conflict-fixer.test.ts and
pr-merge-conflict-fixer-workflow-boundary.test.ts into a shared test helper
under test/helpers, then import and reuse that single implementation in both
files while preserving its existing assertions and return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 82818a64-a94b-47e4-8b72-8428529cfb5c
📒 Files selected for processing (5)
.github/workflows/pr-merge-conflict-fixer.yamltest/pr-merge-conflict-fixer-workflow-boundary.test.tstest/pr-merge-conflict-fixer.test.tstools/pr-merge-conflict-fixer/policy.yamltools/pr-merge-conflict-fixer/publish.mts
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/pr-merge-conflict-fixer.yaml
- tools/pr-merge-conflict-fixer/publish.mts
- test/pr-merge-conflict-fixer-workflow-boundary.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tools/pr-merge-conflict-fixer/resolve.mts`:
- Around line 191-217: Update defaultTools.run to pass a bounded timeout to
execFileSync for each command invocation, ensuring a hung gateway readiness
check fails and returns control to configureOpenShellInference’s retry loop. Use
the existing readiness-attempt timing or another appropriate per-call limit that
preserves the 30-attempt, 1-second polling behavior.
- Around line 108-135: Update gatewayConfiguration and
configureOpenShellInference to validate that bindAddress, derived from
OPENSHELL_GATEWAY_ENDPOINT, resolves to a loopback address before generating
configuration with disable_tls and allow_unauthenticated_users enabled. Reject
non-loopback hosts, including malformed or externally reachable addresses, while
preserving valid 127.0.0.1 and ::1 behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2c19f40a-da75-4d37-b882-c5db043443b8
📒 Files selected for processing (4)
.github/workflows/pr-merge-conflict-fixer.yamltest/pr-merge-conflict-fixer-workflow-boundary.test.tstest/pr-merge-conflict-fixer.test.tstools/pr-merge-conflict-fixer/resolve.mts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pr-merge-conflict-fixer.yaml
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
Adds experimental maintainer automation that resolves every conflicted same-repository PR after a push to
main. Pi resolves the disposable merge in a credential-free OpenShell sandbox, while trusted code validates and atomically publishes only the recorded conflict paths.Related Issue
Fixes #7542
Changes
mainand skip forks before Git merge analysis.mainSHA.azure/openai/gpt-5.6-terrathrough OpenShell inference, without GitHub or upstream model credentials in the sandbox./procor/tmp, no network allowlist, and only the required runtime directories plus container-local/devand/sandbox.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededc2117daa622481409280f47dfd9b015002a9ceaftightens the internal resolver gateway boundary and readiness subprocess behavior. It adds no user-facing command, supported configuration, integration, docs route, or canonical operating procedure. Issue ci: resolve PR merge conflicts with a sandboxed Pi agent #7542 remains the product record; 16 focused tests, CLI type-checking, source-shape, test-size, and normal hooks passed.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears as Verified in GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project integration test/pr-merge-conflict-fixer.test.ts test/pr-merge-conflict-fixer-workflow-boundary.test.ts— 2 files and 16 tests passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
main, resolves them in a sandbox, and publishes a verified merge/resolve commit when conflicts are found.