Repository navigation
Conversation
When `develop` conflicts with `next`, the `Sync branches` workflow now hands the merge to Claude instead of opening a manual PR. Claude runs in a credential-free container, resolves the conflicts in groups, and the merged tree is formatted and type-checked, with one fix-up pass. The result is published to a single reusable `sync/develop-into-next` PR that later develop pushes update. Earlier sync PRs, maintainer remarks and fix commits are fed back as precedent, and decisions between develop and next behavior request review from @supabase/cli. Landing still requires an approval through the existing fast-forward flow. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The resolve job now runs the full `check:all` set, in a container that mise provisions from the merged tree, and commits one round of fixes on top of the merge. The publish job starts the AI review on the sync PR. A new `Sync repair` workflow waits until the sync PR's checks and AI review settle, re-runs failed jobs once, then lets Claude fix what the merge broke and answer each review finding, declining findings the merge did not introduce. It runs at most two rounds per resolution before handing the PR to a maintainer as a draft. `Require fast-forward` is ignored, since branch policy fails it on sync PRs by design. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/ai-review |
There was a problem hiding this comment.
Superseded by a newer AI review
🤖 AI Review
Both independent reviews were available. Verified all 11 supplied findings and merged the overlapping repair findings into one entry. Also found a major bug confusing check-run IDs with Actions job IDs. No fundamental conflict with the listed next-branch work was found. Stats count the supplied findings.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🔴 CRITICAL | .github/scripts/sync-resolve.ts:341 |
security |
codex | The precedent filter grants instruction authority to commenters without verifying repository write permission. Organization membership and collaborator association can admit accounts without write access, whose instructions can influence edits subsequently executed by CI with repository secrets. |
| 🟠 MAJOR | .github/scripts/sync-checks.ts:123 |
correctness |
claude | runChecks treats the advisory config API comparison as a required check. Intentional differences between next and develop therefore trigger unnecessary agent fixes and failing-check reports. |
| 🟠 MAJOR | .github/scripts/sync-publish.ts:597 |
error-handling |
claude+codex | Failed or incomplete repair rounds consume unanswered review findings by posting bot replies. With green CI, settle then reports readiness without escalating those unaddressed findings. A claimed fix without a pushed change is also mislabeled as declined. |
| 🟠 MAJOR | .github/scripts/sync-settle.ts:308 |
api-integration |
codex | The settle step uses a check-run ID as an Actions job ID, causing failed-check attempt lookups to fail and preventing rerun or repair decisions. |
| 🟡 MINOR | .github/scripts/sync-publish.ts:149 |
correctness |
codex | History validation rejects a legitimate resolution when an earlier planned merge already incorporates a later planned tip. |
| 🟡 MINOR | .github/scripts/sync-settle.ts:295 |
correctness |
codex | When same-named check runs include an older completed run and a newer queued run with no started_at, latest-run selection retains the older run and can miss pending CI. |
| 🟡 MINOR | .github/scripts/sync-settle.ts:171 |
pagination |
codex | The repair loop ignores AI review findings beyond the first 100 review threads, allowing incomplete review state to be treated as ready. |
| 🟡 MINOR | .github/scripts/sync-agent.ts:216 |
budget-accounting |
codex | Valid JSON without reported usage is charged as zero, undermining conservative accounting of the run's spending budget. |
| 🟡 MINOR | .github/scripts/sync-checks.ts:84 |
reporting |
codex | Repair-agent edits are described as formatter changes in the quality-check record when the first check passes. |
| 🟡 MINOR | .github/workflows/sync-branches.yml:88 |
supply-chain |
claude | The agent image and check-image base use a mutable Node tag, making these dependency inputs unpinned and reducing reproducibility. |
| ⚪ NIT | apps/cli/docs/release-process.md:350 |
documentation |
claude | The runbook and fix prompt claim the quality checks include a workflow linter, but the checked-out check scripts do not run one. |
Stats
Claude findings: 4 · Codex findings: 7 · Confirmed: 11 · Refuted: 0 · Uncertain: 0
Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: manual · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
| function byMaintainer(remark: Remark): boolean { | ||
| return remark.user?.type !== "Bot" && TRUSTED_ASSOCIATIONS.has(remark.author_association); | ||
| } | ||
|
|
||
| function isMaintainerRemark(remark: Remark): boolean { | ||
| return byMaintainer(remark) && Boolean(remark.body?.trim()); |
There was a problem hiding this comment.
🔴 CRITICAL · security · source: codex
The precedent filter grants instruction authority to commenters without verifying repository write permission. Organization membership and collaborator association can admit accounts without write access, whose instructions can influence edits subsequently executed by CI with repository secrets.
Evidence: sync-resolve.ts:68,341-346 accepts OWNER, MEMBER and COLLABORATOR using only author_association. Lines 411-432 include their remarks as maintainer precedent. resolve-prompt.md:4-5,41-44 and repair-prompt.md:4-5 explicitly grant that precedent instruction authority.
Suggested fix: Verify effective repository permission for each commenter, require write, maintain or admin access, cache lookups, and fail closed when verification fails.
| "pnpm run --if-present check:config-api", | ||
| ].join(" && "); |
There was a problem hiding this comment.
🟠 MAJOR · correctness · source: claude
runChecks treats the advisory config API comparison as a required check. Intentional differences between next and develop therefore trigger unnecessary agent fixes and failing-check reports.
Evidence: sync-checks.ts:118-124 includes check:config-api in an && chain and lines 165-168 derive success from its exit status. tools/config-api-compare.ts:63-71 defaults to origin/develop and line 419 returns 1 for differing declarations. test.yml:81-83 explicitly uses continue-on-error.
Suggested fix: Remove the comparison from the required checks or run it separately as advisory output without sending its differences to the fix agent.
| for (const finding of plan.findings) { | ||
| const reply = replies.get(finding.commentId); | ||
| const fixed = moved && reply?.disposition === "fixed"; | ||
| const body = reply | ||
| ? `${fixed ? "Fixed" : "Declined"} in repair round ${plan.round}: ${neutralize(reply.reply)}` | ||
| : `Repair round ${plan.round} did not address this finding; it needs a maintainer.`; | ||
| await io.replyToReviewComment(plan.pullRequest, finding.commentId, body); | ||
| if (fixed) { | ||
| await io.resolveReviewThread(finding.threadId); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR · error-handling · source: claude+codex
Failed or incomplete repair rounds consume unanswered review findings by posting bot replies. With green CI, settle then reports readiness without escalating those unaddressed findings. A claimed fix without a pushed change is also mislabeled as declined.
Evidence: sync-publish.ts:599-603 replies even when no disposition exists, and treats fixed without a moved head as Declined. sync-settle.ts:346 excludes every thread containing a release-bot reply; lines 120-121 return ready before the unchanged-head escalation guard at lines 124-129.
Suggested fix: Keep unaddressed findings eligible for repair or escalation. Distinguish actual fixed/declined dispositions from failure notices and claims of fixes without pushed changes.
| ? (await githubRequest<{ run_attempt: number }>(token, `${base}/actions/jobs/${run.id}`)) | ||
| .run_attempt |
There was a problem hiding this comment.
🟠 MAJOR · api-integration · source: codex
The settle step uses a check-run ID as an Actions job ID, causing failed-check attempt lookups to fail and preventing rerun or repair decisions.
Evidence: sync-settle.ts:282-286 reads IDs from the check-runs endpoint, then line 308 requests /actions/jobs/${run.id} and line 316 stores that ID as jobId. sync-repair.ts:231 uses it for Actions logs, while line 248 uses it for check-run annotations.
Suggested fix: Resolve the corresponding Actions job through its workflow run and check_run_url, and retain separate jobId and checkRunId fields for job attempts/logs and check annotations.
| for (const merge of [...plan.merges].reverse()) { | ||
| const parents = gitOrThrow(git, ["rev-list", "--parents", "-n", "1", commit]) | ||
| .split(" ") | ||
| .slice(1); | ||
| if (parents.length !== 2 || parents[1] !== merge.sha) { | ||
| return { | ||
| error: `\`${short(commit)}\` is not the merge of \`${merge.ref}\` (\`${short(merge.sha)}\`) the plan expects.`, |
There was a problem hiding this comment.
🟡 MINOR · correctness · source: codex
History validation rejects a legitimate resolution when an earlier planned merge already incorporates a later planned tip.
Evidence: sync-branches.ts:207-209 builds both merges against the original sync head. sync-resolve.ts:140-143 accepts a successful already-up-to-date merge without requiring a new commit. sync-publish.ts:149-155 nevertheless requires a separate two-parent commit for every plan entry.
Suggested fix: Omit redundant planned tips or explicitly validate ancestry-proven no-op merges, preserving alignment with the merge records.
| reviewThreads(first: 100) { | ||
| nodes { | ||
| id | ||
| isResolved | ||
| comments(first: 50) { |
There was a problem hiding this comment.
🟡 MINOR · pagination · source: codex
The repair loop ignores AI review findings beyond the first 100 review threads, allowing incomplete review state to be treated as ready.
Evidence: sync-settle.ts:171 requests reviewThreads(first: 100) without pagination metadata. Lines 333-339 issue one query. Resolved and non-AI threads are filtered only afterward at lines 341-348 and therefore consume the limit.
Suggested fix: Paginate all review threads before deciding readiness, and paginate comments when determining whether a thread has a bot disposition.
| session, | ||
| }; | ||
| } | ||
| const costUsd = output.total_cost_usd ?? 0; |
There was a problem hiding this comment.
🟡 MINOR · budget-accounting · source: codex
Valid JSON without reported usage is charged as zero, undermining conservative accounting of the run's spending budget.
Evidence: sync-agent.ts:31 specifies charging the allocated budget when usage is unreported, and line 212 does so for invalid JSON. Line 216 instead defaults missing total_cost_usd to zero, which line 258 adds to spentUsd.
Suggested fix: Accept only finite, nonnegative reported costs; otherwise charge the call's allocated budget.
| const listed = new Set(edits.map(({ path }) => path)); | ||
| const fixes = [ | ||
| ...edits.filter(({ path }) => changed.includes(path)), | ||
| ...changed | ||
| .filter((path) => !listed.has(path)) | ||
| .map((path) => ({ path, resolution: FORMATTED, precedent: null })), |
There was a problem hiding this comment.
🟡 MINOR · reporting · source: codex
Repair-agent edits are described as formatter changes in the quality-check record when the first check passes.
Evidence: sync-repair.ts:212 stages repair edits before calling checkAndFix. sync-checks.ts:51 leaves edits empty unless the check fixer runs, and lines 84-89 label every otherwise unlisted staged path as Formatted with the repository formatter.
Suggested fix: Pass the repair descriptions into checkAndFix or separate pre-existing staged edits from changes produced by the formatter.
| AGENT_IMAGE: node:24-bookworm-slim | ||
| CHECK_IMAGE: sync-check:local |
There was a problem hiding this comment.
🟡 MINOR · supply-chain · source: claude
The agent image and check-image base use a mutable Node tag, making these dependency inputs unpinned and reducing reproducibility.
Evidence: sync-branches.yml:88 and sync-repair.yml:85 use node:24-bookworm-slim; check.Dockerfile:2 uses the same floating base. sync-agent.ts:136-137 passes ANTHROPIC_API_KEY to the agent container. The mise image at check.Dockerfile:6 is digest-pinned.
Suggested fix: Pin both Node image references by digest and configure routine updates.
| A `develop-into-next` conflict is resolved by Claude (`claude-opus-5-5`) instead, in two extra jobs of the same run: | ||
|
|
||
| 1. **`Resolve conflicts with Claude`** replays the merge and runs Claude Code on each conflicting merge. The agent runs in a container that sees only the worktree (with `.git` read-only), the context files, and `ANTHROPIC_API_KEY`; it has file read and edit tools and no shell. Its context is the history each side brought to every conflicted file, how earlier sync PRs resolved the same files (`git show --remerge-diff`), and those PRs' resolution records, maintainer reviews, comments, and fix commits. Remarks from accounts without write-level association are dropped, since anyone can comment. Conflicted files are resolved in groups of up to eight neighbouring paths, one agent call each, so a large merge stays within each call's turn and spending caps; the run as a whole stops once its budget is spent or after 70 minutes, and hands the merge to a person. The agent delegates broad searches to a read-only explorer subagent on a cheaper model. A group whose result is rejected or that hits a cap is resumed once in the same session, with the reason. | ||
| After the merge commit, the job runs the formatter and the same quality checks as `Check code quality` (`check:all`, including the workflow linter) on the merged tree, in a container with no credentials whose toolchain mise installs from the tree's own `mise.toml`. A merge can fail them even without textual conflicts, when one side still uses something the other renamed or removed. On failure, one more agent call fixes the reported errors and the checks run again; the formatter's and the agent's changes become one fix commit on top of the merge. Failures that remain are listed in the PR comment. |
There was a problem hiding this comment.
⚪ NIT · documentation · source: claude
The runbook and fix prompt claim the quality checks include a workflow linter, but the checked-out check scripts do not run one.
Evidence: release-process.md:350 and fix-prompt.md:10 name a workflow linter. package.json:18 runs types, lint, format, knip and Effect lint; line 20 uses oxlint. turbo.json and workspace lint scripts contain no workflow-lint task.
Suggested fix: Remove the workflow-linter claim or wire an actual workflow-lint step into the checks.
…ow-next-sync-5c9aa1 # Conflicts: # .github/MAINTAINERS.md # .github/workflows/sync-branches.yml
- Precedent remarks count only from authors with write permission, not just an owner, member, or collaborator association. - A failed or partial repair round leaves unanswered findings open, so the settle step hands them to a maintainer instead of reporting ready. - The advisory `check:config-api` comparison no longer fails the checks. - A queued check run counts as the latest, review threads paginate past 100, calls without a reported cost are charged their budget, and edits staged before the checks are not labeled as formatting. - The agent and check images are pinned by digest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/ai-review |
There was a problem hiding this comment.
Superseded by a newer AI review
🤖 AI Review
Both independent reviews were available. Merged 15 findings into 12 entries: 11 confirmed and one uncertain. Confirmed issues include unbounded repair retries, stale review attribution, publication races, merge validation, API response parsing, check ordering, and comment escaping. Container timeout behavior remains uncertain.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟠 MAJOR | .github/scripts/sync-settle.ts:102 |
cost-control |
claude+codex | Manually resolved fallback PRs can run unlimited repair rounds because, without a resolution marker, settle discards every repair record and repeatedly schedules round 1. |
| 🟠 MAJOR | .github/scripts/sync-settle.ts:104 |
review-attribution |
codex | A review of an older resolution posted after a newer resolution record can satisfy the newer resolution's review wait and supply stale findings to its repair. |
| 🟡 MINOR | .github/scripts/sync-settle.ts:124 |
cost-control |
claude | Repair workflow failures and rejected publications do not consume a recorded round, allowing subsequent settle runs to retry the same head and round indefinitely. |
| 🟡 MINOR | .github/workflows/sync-branches.yml:161 |
error-handling |
claude | A failed resolve job skips publication without creating a manual fallback PR or notifying an existing PR, leaving the sync failure visible only in Actions. |
| 🟡 MINOR | .github/scripts/sync-checks.ts:127 |
reliability |
claude | The spawnSync timeouts may fail to stop the check or agent containers, potentially allowing execution beyond the intended deadline. |
| 🟡 MINOR | .github/scripts/sync-publish.ts:249 |
output-escaping |
claude+codex | Agent-controlled paths and manual failure reasons bypass comment neutralization, allowing unintended mentions and Markdown injection in release-bot comments. |
| 🟡 MINOR | .github/scripts/sync-publish.ts:582 |
error-handling |
claude | Repair publication treats every failed push as superseded, hiding authentication, permission, and network failures as routine branch movement. |
| 🟡 MINOR | .github/workflows/sync-repair.yml:9 |
ci |
claude | The repair completion triggers omit the CI workflow, so completion of its relevant PR checks can depend on the 30-minute cron before settle runs again. |
| 🟡 MINOR | .github/scripts/sync-publish.ts:417 |
concurrency |
codex | A stale unsuccessful resolution can draft a PR whose head has since been repaired; settle escalation likewise drafts without validating the evaluated head. |
| 🟡 MINOR | .github/scripts/sync-resolve.ts:140 |
merge-validation |
codex | Resolution publication rejects valid history when an earlier planned merge already contains a later planned commit, making that later merge a successful no-op. |
| 🟡 MINOR | .github/scripts/promotion-shared.ts:84 |
error-handling |
codex | The failed-job rerun request can succeed but still fail settle because its empty 201 response is parsed as JSON, preventing subsequent rerun requests in the loop. |
| 🟡 MINOR | .github/scripts/sync-settle.ts:299 |
check-ordering |
codex | An older completed cancellation with no start time outranks a newer successful check, causing settle to retain the obsolete cancellation. |
Stats
Claude findings: 8 · Codex findings: 7 · Confirmed: 11 · Refuted: 0 · Uncertain: 1
Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: manual · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
| const lineage = lineageStart === -1 ? [] : state.comments.slice(lineageStart); | ||
| const resolvedAt = lineageStart === -1 ? undefined : Date.parse(lineage[0]?.createdAt ?? ""); | ||
| const reviewed = | ||
| state.aiReview !== undefined && | ||
| (resolvedAt === undefined || Date.parse(state.aiReview.submittedAt) >= resolvedAt); | ||
| if (!reviewed && resolvedAt !== undefined && state.now - resolvedAt < AI_REVIEW_WAIT_MS) { | ||
| return { action: "wait", reason: "the AI review has not finished" }; | ||
| } | ||
|
|
||
| const failing = checks.filter( | ||
| ({ conclusion }) => conclusion !== null && FAILED_CONCLUSIONS.has(conclusion), | ||
| ); | ||
| const firstAttempts = [ | ||
| ...new Set(failing.filter(({ attempt }) => attempt < 2).map(({ runId }) => runId)), | ||
| ]; | ||
| if (firstAttempts.length > 0) { | ||
| return { action: "rerun", runIds: firstAttempts }; | ||
| } | ||
| if (failing.length === 0 && state.findings.length === 0) { | ||
| return { action: "ready" }; | ||
| } | ||
|
|
||
| const rounds = lineage.filter((c) => byBot(c) && c.body.startsWith(REPAIR_MARKER)); | ||
| if (rounds.some((c) => REPAIR_HEAD.exec(c.body)?.[1] === head)) { | ||
| return { | ||
| action: "escalate", | ||
| reason: `a repair round on \`${head.slice(0, 7)}\` already ran, and ${failing.length} failing job${failing.length === 1 ? "" : "s"} and ${state.findings.length} unanswered review finding${state.findings.length === 1 ? "" : "s"} remain`, | ||
| }; | ||
| } | ||
| if (rounds.length >= MAX_REPAIR_ROUNDS) { | ||
| return { | ||
| action: "escalate", | ||
| reason: `${rounds.length} repair rounds did not settle the checks and the review`, | ||
| }; | ||
| } | ||
| return { | ||
| action: "repair", | ||
| plan: { | ||
| source: pair.source, | ||
| target: pair.target, | ||
| pullRequest: pull.number, | ||
| head, | ||
| round: rounds.length + 1, |
There was a problem hiding this comment.
🟠 MAJOR · cost-control · source: claude+codex
Manually resolved fallback PRs can run unlimited repair rounds because, without a resolution marker, settle discards every repair record and repeatedly schedules round 1.
Evidence: sync-settle.ts:99-103 assigns an empty lineage when no resolution marker exists; :124-144 derives both escalation checks and the next round from that lineage. sync-publish.ts:422-437 creates the fallback PR without a resolution record, and sync-settle.ts:252 accepts that bot-authored PR.
Suggested fix: Include existing repair records when no resolution marker exists, or exclude such PRs from automatic repair. Cover failing CI and previous same-head repairs without a resolution marker.
| const rounds = lineage.filter((c) => byBot(c) && c.body.startsWith(REPAIR_MARKER)); | ||
| if (rounds.some((c) => REPAIR_HEAD.exec(c.body)?.[1] === head)) { | ||
| return { | ||
| action: "escalate", | ||
| reason: `a repair round on \`${head.slice(0, 7)}\` already ran, and ${failing.length} failing job${failing.length === 1 ? "" : "s"} and ${state.findings.length} unanswered review finding${state.findings.length === 1 ? "" : "s"} remain`, | ||
| }; | ||
| } | ||
| if (rounds.length >= MAX_REPAIR_ROUNDS) { | ||
| return { | ||
| action: "escalate", | ||
| reason: `${rounds.length} repair rounds did not settle the checks and the review`, | ||
| }; | ||
| } | ||
| return { | ||
| action: "repair", | ||
| plan: { | ||
| source: pair.source, | ||
| target: pair.target, | ||
| pullRequest: pull.number, | ||
| head, | ||
| round: rounds.length + 1, |
There was a problem hiding this comment.
🟡 MINOR · cost-control · source: claude
Repair workflow failures and rejected publications do not consume a recorded round, allowing subsequent settle runs to retry the same head and round indefinitely.
Evidence: sync-settle.ts:124 counts published repair-marker comments only. sync-repair.yml:142-147 makes publish depend on successful repair completion. sync-publish.ts:571-579 rejects invalid results before the record at :623-626, and :706-708 exits without recording the attempt.
Suggested fix: Record started attempts before running the agent, or reliably record failed attempts through a failure handler, and count them toward the retry limit.
| publish: | ||
| name: Publish the sync pull request | ||
| needs: | ||
| - sync | ||
| - resolve | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🟡 MINOR · error-handling · source: claude
A failed resolve job skips publication without creating a manual fallback PR or notifying an existing PR, leaving the sync failure visible only in Actions.
Evidence: sync-branches.yml:161-166 declares publish with dependencies on sync and resolve and no failure condition. The resolve job has no failure-reporting step. sync-resolve.ts:639-643 exits on thrown errors without producing a fallback result.
Suggested fix: Add a failure handler that creates the manual conflict PR when needed, or comments on the existing PR and hands it to a maintainer.
| const result = spawnSync( | ||
| "docker", | ||
| [ | ||
| "run", | ||
| "--rm", | ||
| "--user", | ||
| user, | ||
| "--env", | ||
| "HOME=/home/check", | ||
| "--env", | ||
| "CI=true", | ||
| "--env", | ||
| "MISE_YES=1", | ||
| "--env", | ||
| "MISE_TRUSTED_CONFIG_PATHS=/work", | ||
| "--env", | ||
| "MISE_DATA_DIR=/home/check/mise", | ||
| "--env", | ||
| "MISE_CACHE_DIR=/home/check/mise-cache", | ||
| "--volume", | ||
| `${workDir}:/work`, | ||
| "--volume", | ||
| `${join(workDir, ".git")}:/work/.git:ro`, | ||
| "--volume", | ||
| `${checkHome}:/home/check`, | ||
| "--workdir", | ||
| "/work", | ||
| image, | ||
| "sh", | ||
| "-c", | ||
| `mise install --quiet && mise exec -- sh -c '${steps}'`, | ||
| ], | ||
| { | ||
| encoding: "utf8", | ||
| maxBuffer: 64 * 1024 * 1024, | ||
| timeout: 20 * 60 * 1000, | ||
| env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" }, | ||
| }, |
There was a problem hiding this comment.
🟡 MINOR · reliability · source: claude
The spawnSync timeouts may fail to stop the check or agent containers, potentially allowing execution beyond the intended deadline.
Evidence: sync-checks.ts:127-164 launches docker run with sh -c and a 20-minute spawnSync timeout, without --init, a container name, or explicit cleanup. sync-agent.ts:129-188 similarly applies a timeout to the Docker CLI without explicit container termination.
Suggested fix: Give each container an owned identifier and explicitly terminate it after timeout; use an init process for signal forwarding and verify that cleanup completes.
Adjudication (uncertain): The missing explicit cleanup is verified, but actual termination depends on Docker signal forwarding and the container process handlers. Docker daemon access was denied, and the installed Claude CLI's signal behavior could not be inspected. The claim that Node necessarily ignores SIGTERM was not established.
| `${index + 1}. ${neutralize(decision.question)} (${decision.paths.map((path) => `\`${path}\``).join(", ")})`, | ||
| ` - Chosen: ${neutralize(decision.chosen)}`, | ||
| ` - Alternative: ${neutralize(decision.alternative)}`, | ||
| ]), | ||
| ]; | ||
| } | ||
|
|
||
| function renderMerge(ref: string, sha: string, resolution: AgentResolution | null): string[] { | ||
| const heading = `#### \`${ref}\` (\`${short(sha)}\`)`; | ||
| if (resolution === null) { | ||
| return [heading, "", "Merged cleanly."]; | ||
| } | ||
| const lines = [ | ||
| heading, | ||
| "", | ||
| neutralize(resolution.summary), | ||
| ...renderDecisions(resolution.decisions), | ||
| ]; | ||
| lines.push("", "**Resolved files**", ""); | ||
| for (const file of resolution.files) { | ||
| const precedent = file.precedent === null ? "" : ` (follows #${file.precedent})`; | ||
| lines.push(`- \`${file.path}\`: ${neutralize(file.resolution)}${precedent}`); | ||
| } | ||
| for (const path of resolution.deletedFiles) { | ||
| lines.push(`- \`${path}\`: deleted`); | ||
| } | ||
| return lines; | ||
| } | ||
|
|
||
| function renderCheck(check: CheckResult): string[] { | ||
| const fixes = check.fixes.map( | ||
| ({ path, resolution }) => `- \`${path}\`: ${neutralize(resolution)}`, |
There was a problem hiding this comment.
🟡 MINOR · output-escaping · source: claude+codex
Agent-controlled paths and manual failure reasons bypass comment neutralization, allowing unintended mentions and Markdown injection in release-bot comments.
Evidence: sync-publish.ts:249, :270-273, :280, and :527 interpolate paths directly into backticks; :376 interpolates result.reason directly. sync-agent.ts:71-103 accepts arbitrary strings for reported paths. sync-resolve.ts:97-98 and :171-177 propagate agent summaries into manual reasons. The neutralizer exists at sync-publish.ts:72-73 but is omitted at these sites.
Suggested fix: Neutralize all agent-derived strings before publication and escape Markdown delimiters in paths. Apply the same handling to manual failure reasons.
| const reviewed = | ||
| state.aiReview !== undefined && | ||
| (resolvedAt === undefined || Date.parse(state.aiReview.submittedAt) >= resolvedAt); | ||
| if (!reviewed && resolvedAt !== undefined && state.now - resolvedAt < AI_REVIEW_WAIT_MS) { | ||
| return { action: "wait", reason: "the AI review has not finished" }; |
There was a problem hiding this comment.
🟠 MAJOR · review-attribution · source: codex
A review of an older resolution posted after a newer resolution record can satisfy the newer resolution's review wait and supply stale findings to its repair.
Evidence: sync-settle.ts:104-106 attributes reviews solely by submission time. :327-337 retains no reviewed commit identity, and :352-370 collects unresolved AI findings without filtering by resolution. ai-review/post-review.ts:1127-1150 posts results without a reviewed-head freshness check.
Suggested fix: Bind review publication and findings to the captured full resolution SHA, then select reviews by that identity while allowing repair descendants to share the resolution's review.
| if (plan.pullRequest !== null) { | ||
| await io.comment(plan.pullRequest, renderManualComment(plan, result)); | ||
| await io.convertToDraft(plan.pullRequest); |
There was a problem hiding this comment.
🟡 MINOR · concurrency · source: codex
A stale unsuccessful resolution can draft a PR whose head has since been repaired; settle escalation likewise drafts without validating the evaluated head.
Evidence: sync-publish.ts:417-419 comments and converts the existing PR to draft without checking expectedSyncHead. sync-settle.ts:413-427 posts escalation and fetches only the PR node ID before conversion. sync-repair.yml:23-27 places repair in a separate concurrency group from sync.
Suggested fix: Before takeover comments or draft conversion, verify that the PR remains open and its current head matches the evaluated plan; discard superseded outcomes.
| const merge = git(["-c", "merge.conflictStyle=zdiff3", "merge", "--no-ff", "-m", message, sha]); | ||
| if (merge.status === 0) { | ||
| merges.push({ ref, sha, resolution: null }); | ||
| continue; |
There was a problem hiding this comment.
🟡 MINOR · merge-validation · source: codex
Resolution publication rejects valid history when an earlier planned merge already contains a later planned commit, making that later merge a successful no-op.
Evidence: sync-branches.ts:207-209 selects next and develop by comparing each only against the original sync head. sync-resolve.ts:140-143 records every successful merge command, including no-ops. sync-publish.ts:149-155 nevertheless requires a distinct two-parent merge for every plan entry.
Suggested fix: Remove redundant ancestor merges from the plan or explicitly represent and validate successful no-op merges.
| if (response.status === 204) { | ||
| return undefined as T; | ||
| } | ||
| return (await response.json()) as T; |
There was a problem hiding this comment.
🟡 MINOR · error-handling · source: codex
The failed-job rerun request can succeed but still fail settle because its empty 201 response is parsed as JSON, preventing subsequent rerun requests in the loop.
Evidence: promotion-shared.ts:84-87 skips JSON parsing only for status 204. sync-settle.ts:405-408 awaits the rerun-failed-jobs endpoint sequentially without handling an empty 201 response.
Suggested fix: Handle successful empty responses independently of status, or use a request helper that expects no response body for rerun endpoints.
| // A queued run has no start time yet and is the newest. | ||
| const started = (check: CheckRunResponse) => check.started_at ?? "\uffff"; | ||
| const newer = | ||
| !current || | ||
| started(run) > started(current) || | ||
| (started(run) === started(current) && run.id > current.id); |
There was a problem hiding this comment.
🟡 MINOR · check-ordering · source: codex
An older completed cancellation with no start time outranks a newer successful check, causing settle to retain the obsolete cancellation.
Evidence: sync-settle.ts:300 replaces every null started_at with the highest sorting sentinel regardless of status. :301-304 compares that value before check IDs, so a real newer timestamp cannot replace the older null-start check.
Suggested fix: Order by creation identity or an appropriate creation timestamp; do not treat completed checks with missing start times as newest.
- Un-export unused symbols flagged by knip, and pin the checkAndFix test fixture to `main` so it passes where git's default branch differs. - Settle records a repair round before it runs, so a crashed repair still uses up its round, and counts every round on a sync PR a maintainer resolved by hand. - A check run cancelled before it started no longer outranks a newer run, and empty 2xx API responses (such as a job re-run) parse. - Agent-reported paths and failure reasons are escaped in bot comments, a failed repair push that did not lose a race is an error, and the `CI` workflow also triggers the repair loop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/ai-review |
There was a problem hiding this comment.
Superseded by a newer AI review
🤖 AI Review
Both independent reviews were available. All 11 findings are confirmed after reading the cited code and relevant surrounding paths; none are duplicates. The missing secret redaction is classified as critical, while the omitted edit inventory is a minor auditability concern. Targeted Bun probes verified secret preservation in rendered comments and timeout overruns when a child ignores SIGTERM. No fundamental conflict with the supplied next-branch work was identified.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🔴 CRITICAL | .github/scripts/sync-publish.ts:72 |
security |
claude | Agent output containing credentials is published in public PR bodies and comments without secret redaction, creating a disclosure path for the Anthropic key available inside the agent container. |
| 🟠 MAJOR | .github/scripts/sync-checks.ts:101 |
error-handling |
codex | An invalid root package.json aborts resolution before the fix agent or maintainer handoff can run. |
| 🟡 MINOR | .github/scripts/sync-checks.ts:128 |
error-handling |
claude | The Docker subprocess timeouts do not reliably enforce container shutdown, so an unresponsive container can exceed the check and agent deadlines. |
| 🟡 MINOR | .github/workflows/sync-branches.yml:161 |
error-handling |
claude | A crashed or timed-out resolve job skips publication without opening a conflict PR or recording a maintainer handoff. |
| 🟡 MINOR | .github/scripts/sync-resolve.ts:189 |
auditability |
claude | Non-conflicted files edited during resolution are committed but can be omitted from the resolution record, obscuring which changes the agent introduced. |
| 🟡 MINOR | .github/scripts/sync-checks.ts:57 |
error-handling |
claude | Infrastructure and check-setup failures are treated as code failures and sent to the fix agent, which can commit unnecessary edits. |
| 🟡 MINOR | .github/scripts/sync-branches.ts:207 |
correctness |
codex | A plan containing a source already included in an earlier target merge is rejected by the publisher despite successful replay. |
| 🟡 MINOR | .github/scripts/sync-resolve.ts:185 |
correctness |
codex | A later resolver group can reintroduce conflict markers into an earlier group's files without triggering the resolution guard. |
| 🟡 MINOR | .github/scripts/sync-settle.ts:105 |
correctness |
codex | An older resolution's review submitted after a newer resolution record can satisfy the newer resolution's review wait. |
| 🟡 MINOR | .github/scripts/sync-publish.ts:421 |
concurrency |
codex | A stale manual-resolution result can convert a newer maintainer-resolved sync PR back to draft. |
| ⚪ NIT | mise.lock:220 |
scope |
claude | The PR includes an unrelated provenance_verified flag change for the macOS arm64 jactionlint artifact. |
Stats
Claude findings: 6 · Codex findings: 5 · Confirmed: 11 · Refuted: 0 · Uncertain: 0
Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: manual · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
| function neutralize(text: string): string { | ||
| return text.replace(/@(?=[\w-])/g, "@\u200b").replace(/<!--/g, "<!--"); | ||
| } |
There was a problem hiding this comment.
🔴 CRITICAL · security · source: claude
Agent output containing credentials is published in public PR bodies and comments without secret redaction, creating a disclosure path for the Anthropic key available inside the agent container.
Evidence: sync-publish.ts:72-74 only neutralizes mentions and HTML comment openers; lines 269-275 render agent summaries and resolutions, and lines 671-672 post comment bodies unchanged. sync-agent.ts:136-137 passes ANTHROPIC_API_KEY into the container, and lines 167-168 permit file-reading tools. promotion-shared.ts:76-79 serializes API bodies without redaction.
Suggested fix: Apply redactSecretsDeep before writing or uploading agent-result artifacts and redactSecrets at every publication boundary. Strip the literal API key while it is available in the resolve or repair job.
| "docker", | ||
| [ | ||
| "run", | ||
| "--rm", | ||
| "--user", | ||
| user, | ||
| "--env", | ||
| "HOME=/home/check", | ||
| "--env", | ||
| "CI=true", | ||
| "--env", | ||
| "MISE_YES=1", | ||
| "--env", | ||
| "MISE_TRUSTED_CONFIG_PATHS=/work", | ||
| "--env", | ||
| "MISE_DATA_DIR=/home/check/mise", | ||
| "--env", | ||
| "MISE_CACHE_DIR=/home/check/mise-cache", | ||
| "--volume", | ||
| `${workDir}:/work`, | ||
| "--volume", | ||
| `${join(workDir, ".git")}:/work/.git:ro`, | ||
| "--volume", | ||
| `${checkHome}:/home/check`, | ||
| "--workdir", | ||
| "/work", | ||
| image, | ||
| "sh", | ||
| "-c", | ||
| `mise install --quiet && mise exec -- sh -c '${steps}'`, | ||
| ], | ||
| { | ||
| encoding: "utf8", | ||
| maxBuffer: 64 * 1024 * 1024, | ||
| timeout: 20 * 60 * 1000, | ||
| env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" }, |
There was a problem hiding this comment.
🟡 MINOR · error-handling · source: claude
The Docker subprocess timeouts do not reliably enforce container shutdown, so an unresponsive container can exceed the check and agent deadlines.
Evidence: sync-checks.ts:130-157 runs a shell as container PID 1 without --init, and line 162 sets only spawnSync's timeout. sync-agent.ts:132-182 likewise omits --init and sets a subprocess timeout. Neither path explicitly kills the container.
Suggested fix: Use --init and named containers, terminate the Docker client forcibly on timeout, and explicitly kill and remove the container before retrying or staging changes.
| publish: | ||
| name: Publish the sync pull request | ||
| needs: | ||
| - sync | ||
| - resolve | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🟡 MINOR · error-handling · source: claude
A crashed or timed-out resolve job skips publication without opening a conflict PR or recording a maintainer handoff.
Evidence: sync-branches.yml:161-166 makes publish depend on successful sync and resolve jobs without a failure condition. sync-resolve.ts:636 writes result.json only after resolution and checks complete; lines 644-648 exit with failure on exceptions. sync-branches.ts:280-290 returns an agent plan without first creating a PR.
Suggested fix: Add a failure-path job that uses the sync plan to open a manual conflict PR or safely hand an unchanged existing PR to a maintainer.
| gitOrThrow(git, ["add", "-A"]); | ||
| gitOrThrow(git, ["-c", "core.hooksPath=/dev/null", "commit", "--no-verify", "-m", message]); | ||
| const { strayEdits } = protectedPathChanges(git, "HEAD"); | ||
| if (strayEdits.length > 0) { | ||
| return giveUp( | ||
| `The agent edited \`${strayEdits.join("`, `")}\`, which did not conflict; files under \`${PROTECTED_PATH_PREFIX}\` may change only to resolve a conflict.`, | ||
| ); | ||
| } | ||
| merges.push({ ref, sha, resolution: combined }); |
There was a problem hiding this comment.
🟡 MINOR · auditability · source: claude
Non-conflicted files edited during resolution are committed but can be omitted from the resolution record, obscuring which changes the agent introduced.
Evidence: sync-resolve.ts:189 stages all changes. sync-branches.ts:113-124 compares only .github/ paths against Git's automatic merge. sync-publish.ts:272-278 lists only agent-reported files and deletions. resolve-prompt.md permits necessary call-site edits while instructing the agent to report only its conflict-group files.
Suggested fix: Compare the completed merge against Git's automatic merge across the entire tree and include every additional agent-edited path in the resolution record.
| if (!last.passed) { | ||
| const outcome = await fix(last.output.slice(-MAX_CHECK_OUTPUT)); | ||
| if ( | ||
| typeof outcome === "string" || | ||
| outcome.status === "unresolved" || | ||
| outcome.deletedFiles.length > 0 | ||
| ) { | ||
| dropUnstaged(git); | ||
| } else { | ||
| gitOrThrow(git, ["add", "-A"]); | ||
| last = check(); | ||
| keepCheckChanges(git); | ||
| edits = outcome.files; | ||
| decisions = outcome.decisions; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MINOR · error-handling · source: claude
Infrastructure and check-setup failures are treated as code failures and sent to the fix agent, which can commit unnecessary edits.
Evidence: sync-checks.ts:120-125 combines dependency installation, formatting, and quality checks; line 157 also includes mise installation. Lines 166-169 classify success solely by exit status without inspecting result.error or the failing stage. Lines 57-70 invoke the fixer for every failed result and retain its accepted edits.
Suggested fix: Distinguish setup or execution failures from quality-check failures. Skip code repair for infrastructure failures and publish an explicit checks-could-not-run result.
| JSON.parse(readFileSync(join(workDir, "package.json"), "utf8")) as { | ||
| scripts?: Record<string, string>; | ||
| } | ||
| ).scripts?.["check:all"]; |
There was a problem hiding this comment.
🟠 MAJOR · error-handling · source: codex
An invalid root package.json aborts resolution before the fix agent or maintainer handoff can run.
Evidence: sync-checks.ts:101 parses package.json without recovery, and line 124 calls that helper before spawning Docker. checkAndFix calls the checker at line 53 without catching exceptions. sync-resolve.ts:617-636 runs checks before writing result.json, and lines 644-648 turn an exception into job failure.
Suggested fix: Convert manifest-read and parsing failures into diagnostic check results so the fixer can repair them, or produce a manual handoff result instead of aborting.
| const merges = [pair.target, pair.source] | ||
| .filter((ref) => !isAncestor(git, `refs/remotes/origin/${ref}`, syncHead)) | ||
| .map((ref) => ({ ref, sha: gitOrThrow(git, ["rev-parse", `refs/remotes/origin/${ref}`]) })); |
There was a problem hiding this comment.
🟡 MINOR · correctness · source: codex
A plan containing a source already included in an earlier target merge is rejected by the publisher despite successful replay.
Evidence: sync-branches.ts:207-209 filters both candidate merges against the original syncHead. sync-resolve.ts:143-146 records every successful git merge, including an already-up-to-date no-op. sync-publish.ts:154-160 requires a distinct two-parent commit for each planned merge.
Suggested fix: Remove candidate merges already contained in earlier planned tips, or explicitly represent and validate no-op merges.
| const combined = combine(resolutions); | ||
| for (const path of combined.deletedFiles) { | ||
| gitOrThrow(git, ["rm", "-q", "--ignore-unmatch", "--", path]); | ||
| } | ||
| gitOrThrow(git, ["add", "-A"]); | ||
| gitOrThrow(git, ["-c", "core.hooksPath=/dev/null", "commit", "--no-verify", "-m", message]); |
There was a problem hiding this comment.
🟡 MINOR · correctness · source: codex
A later resolver group can reintroduce conflict markers into an earlier group's files without triggering the resolution guard.
Evidence: sync-resolve.ts:174 calls checkGroup only for the current group's files. Lines 185-190 combine reports and stage and commit the entire worktree without checking allFiles again. The subsequent protectedPathChanges check inspects protected-path edits, not conflict-marker removal.
Suggested fix: Run checkGroup over all originally conflicted files using the combined resolution immediately before deletion, staging, and commit.
| const reviewed = | ||
| state.aiReview !== undefined && | ||
| (resolvedAt === undefined || Date.parse(state.aiReview.submittedAt) >= resolvedAt); | ||
| if (!reviewed && resolvedAt !== undefined && state.now - resolvedAt < AI_REVIEW_WAIT_MS) { | ||
| return { action: "wait", reason: "the AI review has not finished" }; |
There was a problem hiding this comment.
🟡 MINOR · correctness · source: codex
An older resolution's review submitted after a newer resolution record can satisfy the newer resolution's review wait.
Evidence: sync-settle.ts:105-107 compares only submittedAt against resolvedAt. Lines 330-340 discard review commit identity and select the last marker-bearing bot review. The AI review workflow permits overlapping automatic and manually dispatched runs through separate concurrency groups.
Suggested fix: Associate reviews with the resolution commit SHA and retain that association across its repair commits, instead of identifying the reviewed resolution solely by submission time.
| if (result.status === "manual") { | ||
| if (plan.pullRequest !== null) { | ||
| await io.comment(plan.pullRequest, renderManualComment(plan, result)); | ||
| await io.convertToDraft(plan.pullRequest); | ||
| return { status: "needs-maintainer", pullRequest: plan.pullRequest }; |
There was a problem hiding this comment.
🟡 MINOR · concurrency · source: codex
A stale manual-resolution result can convert a newer maintainer-resolved sync PR back to draft.
Evidence: sync-publish.ts:421-425 comments and converts an existing PR to draft before any branch fetch or lease check. expectedSyncHead is checked only through pushSyncBranch at lines 402-409, which this branch bypasses. convertToDraft at lines 660-669 reads only the PR node ID.
Suggested fix: Verify that the PR remains open and its current head matches plan.expectedSyncHead before applying a manual handoff; discard superseded results.
- Agent-written text is passed through the AI review's secret redaction before it reaches a public sync PR comment. - An unparsable root package.json is reported as a failing check instead of aborting the resolve run. - Every conflicted file is checked for conflict markers after all groups resolve, since a later group can edit an earlier group's files. - Revert an unrelated mise.lock change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/ai-review |
There was a problem hiding this comment.
Superseded by a newer AI review
🤖 AI Review
Verified all 12 supplied findings and merged two overlapping pairs into 10 entries. Confirmed credential-output exposure, merge-history rejection, ineffective check timeouts, missing failure handoffs, and the reported provenance, reporting, and documentation issues. The timeout finding holds for check containers; Claude's own signal handling was not established.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🔴 CRITICAL | .github/workflows/sync-branches.yml:154 |
security |
claude+codex | Raw agent results and generated commits leave the credential-bearing sandbox without secret filtering. Credential-bearing output can reach downloadable artifacts or the public sync branch before comment redaction applies. |
| 🟠 MAJOR | .github/scripts/sync-branches.ts:207 |
correctness |
codex | An update plan can include develop even when the preceding planned next merge already contains it, causing publication to reject a successfully resolved update. |
| 🟠 MAJOR | .github/scripts/sync-checks.ts:162 |
timeout-handling |
claude+codex | The twenty-minute check timeout does not reliably bound execution or terminate the check container, allowing a hanging check to consume the workflow timeout instead of producing a publishable result. |
| 🟠 MAJOR | .github/workflows/sync-branches.yml:163 |
error-handling |
claude | A resolve crash or publication rejection has no scripted maintainer handoff. When no sync PR exists, subsequent develop pushes retry the same conflict with a fresh agent budget. |
| 🟡 MINOR | .github/scripts/sync-repair.ts:111 |
correctness |
claude | Matching a commented line anywhere in a branch's file can incorrectly label a merge-introduced finding as pre-existing, particularly for common syntax such as closing braces. |
| 🟡 MINOR | .github/scripts/sync-repair.ts:163 |
correctness |
codex | Unavailable finding provenance is presented as proof that the merge introduced the commented line. |
| 🟡 MINOR | .github/scripts/sync-settle.ts:419 |
correctness |
claude | The escalation message promises that repairs resume after a maintainer fixes the branch and marks the PR ready, but a consumed two-round budget still prevents further repairs on the new head. |
| 🟡 MINOR | .github/sync-branches/resolve-prompt.md:34 |
review-records |
codex | The resolution prompt instructs the agent to omit permitted auxiliary edits from its per-file resolution report. |
| 🟡 MINOR | .github/scripts/sync-publish.ts:316 |
output-sanitization |
claude | Untrusted check output can close its Markdown fence and inject rendered text or mentions into public bot comments. |
| ⚪ NIT | .github/scripts/sync-resolve.ts:410 |
documentation |
claude | The docstring and release-process documentation describe remark filtering by association instead of the actual write-permission requirement. |
Stats
Claude findings: 7 · Codex findings: 5 · Confirmed: 10 · Refuted: 0 · Uncertain: 0
Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: manual · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
| - name: Upload resolution | ||
| uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 | ||
| with: | ||
| name: sync-resolution | ||
| path: ${{ runner.temp }}/sync-resolution | ||
| retention-days: 7 |
There was a problem hiding this comment.
🔴 CRITICAL · security · source: claude+codex
Raw agent results and generated commits leave the credential-bearing sandbox without secret filtering. Credential-bearing output can reach downloadable artifacts or the public sync branch before comment redaction applies.
Evidence: sync-agent.ts:136-137 passes ANTHROPIC_API_KEY into the agent container. sync-resolve.ts:640-650 and sync-repair.ts:331-341 export bundles and unredacted result.json. sync-branches.yml:154-159 and sync-repair.yml:137-142 upload those directories. sync-publish.ts:460-470 validates history and pushes without scanning file contents; neutralize at :76-79 only sanitizes rendered text.
Suggested fix: Deep-redact result.json before upload and reject credential-bearing generated changes before exporting bundles or pushing commits.
| const merges = [pair.target, pair.source] | ||
| .filter((ref) => !isAncestor(git, `refs/remotes/origin/${ref}`, syncHead)) | ||
| .map((ref) => ({ ref, sha: gitOrThrow(git, ["rev-parse", `refs/remotes/origin/${ref}`]) })); |
There was a problem hiding this comment.
🟠 MAJOR · correctness · source: codex
An update plan can include develop even when the preceding planned next merge already contains it, causing publication to reject a successfully resolved update.
Evidence: sync-branches.ts:207-209 compares both tips independently with the original syncHead. sync-resolve.ts:140-146 records every successful git merge, including an already-up-to-date merge. sync-publish.ts:160-167 requires a separate two-parent commit for each planned merge.
Suggested fix: Omit merge tips already reachable through an earlier planned tip, or support no-op merges consistently in replay and validation. Add coverage for next containing develop while both are absent from syncHead.
| `mise install --quiet && mise exec -- sh -c '${steps}'`, | ||
| ], | ||
| { | ||
| encoding: "utf8", | ||
| maxBuffer: 64 * 1024 * 1024, | ||
| timeout: 20 * 60 * 1000, | ||
| env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" }, |
There was a problem hiding this comment.
🟠 MAJOR · timeout-handling · source: claude+codex
The twenty-minute check timeout does not reliably bound execution or terminate the check container, allowing a hanging check to consume the workflow timeout instead of producing a publishable result.
Evidence: sync-checks.ts:132-169 launches docker run with sh -c, no --init or explicit container cleanup, and a spawnSync timeout. sync-agent.ts:129-188 similarly lacks explicit container cleanup. Both resolve and repair workflows have a 120-minute job timeout.
Suggested fix: Track each owned container and explicitly terminate it when its deadline expires, with a bounded Docker-client wait. Add an init process for signal forwarding and apply lifecycle cleanup to agent calls too.
| needs: | ||
| - sync | ||
| - resolve | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 15 | ||
| # `actions: write` lets the job's own token start the AI review on the sync pull request. |
There was a problem hiding this comment.
🟠 MAJOR · error-handling · source: claude
A resolve crash or publication rejection has no scripted maintainer handoff. When no sync PR exists, subsequent develop pushes retry the same conflict with a fresh agent budget.
Evidence: sync-branches.yml:93-208 has no failure fallback, and publish depends on successful resolve completion. sync-resolve.ts:658-662 exits on exceptions; sync-publish.ts:738-740 exits on rejection. sync-branches.ts:280-291 returns needs-resolution without creating a PR.
Suggested fix: Add failure handling that creates a manual conflict PR or comments on and drafts an existing PR, persisting the handoff so later pushes stop retrying automatically.
| export function findingOrigins(git: GitRunner, plan: RepairPlan, finding: RepairFinding): string[] { | ||
| if (finding.line === null) { | ||
| return []; | ||
| } | ||
| const file = git(["show", `${plan.head}:${finding.path}`]); | ||
| const line = file.status === 0 ? file.stdout.split("\n")[finding.line - 1]?.trim() : undefined; | ||
| if (!line) { | ||
| return []; | ||
| } | ||
| return [plan.source, plan.target].filter((branch) => { | ||
| const side = git(["show", `refs/remotes/origin/${branch}:${finding.path}`]); | ||
| return side.status === 0 && side.stdout.split("\n").some((text) => text.trim() === line); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 MINOR · correctness · source: claude
Matching a commented line anywhere in a branch's file can incorrectly label a merge-introduced finding as pre-existing, particularly for common syntax such as closing braces.
Evidence: sync-repair.ts:115-122 trims one line and searches for any identical trimmed line in each branch's file. writeRepairContext at :163-164 labels matches as likely predating the merge. repair-prompt.md:30-37 uses that provenance when deciding which findings to decline.
Suggested fix: Compare surrounding context or mapped line history against the merge parents, and omit provenance hints for trivial lines.
| branches.length > 0 | ||
| ? `The commented line already exists on ${branches.map((branch) => `\`${branch}\``).join(" and ")}, so the issue likely predates the merge.` | ||
| : `The commented line is on neither \`${plan.source}\` nor \`${plan.target}\`; the merge or its fixes wrote it.`, |
There was a problem hiding this comment.
🟡 MINOR · correctness · source: codex
Unavailable finding provenance is presented as proof that the merge introduced the commented line.
Evidence: sync-repair.ts:112-118 returns [] for a null line, failed file read, or unavailable or blank line. At :158-165, the same empty array produces the assertion that neither branch contains the line and the merge or its fixes wrote it. sync-settle.ts:367-372 passes review-comment line values through to the repair plan.
Suggested fix: Represent unavailable provenance separately from a successful comparison with no matches, and describe unavailable cases as unknown.
| await githubRequest(token, `${base}/issues/${pullRequest}/comments`, { | ||
| body: [ | ||
| `${ESCALATED_MARKER} head=${state.pull?.headSha} -->`, | ||
| `@${owner}/${REVIEW_TEAM_SLUG}: ${decision.reason}, so this pull request is now a draft and needs a maintainer. Fix it on the branch, then mark it ready for review; syncs and repairs resume after that.`, |
There was a problem hiding this comment.
🟡 MINOR · correctness · source: claude
The escalation message promises that repairs resume after a maintainer fixes the branch and marks the PR ready, but a consumed two-round budget still prevents further repairs on the new head.
Evidence: sync-settle.ts:99-104 defines lineage from the latest resolution comment, and :125-137 escalates after two repair markers. The escalation marker at :87-90 is head-specific. Line 419 promises that syncs and repairs resume after the maintainer's intervention.
Suggested fix: Either record an explicit repair-budget reset when maintainer intervention resumes automation, or state that repairs remain exhausted until a new resolution record starts a lineage.
| ## Your task | ||
|
|
||
| 1. Resolve every file in your group, listed in the merge section. Remove every conflict marker. | ||
| Report only those files in your JSON. |
There was a problem hiding this comment.
🟡 MINOR · review-records · source: codex
The resolution prompt instructs the agent to omit permitted auxiliary edits from its per-file resolution report.
Evidence: resolve-prompt.md:33-34 restricts reporting to the conflict group, while :38-40 permits edits to other files. sync-resolve.ts:203-204 stages and commits all changes. sync-publish.ts:278-284 renders only the reported files and deletions.
Suggested fix: Require reporting every edited file while retaining conflict-group boundaries for unresolved conflicts and deletion requests.
| "<details><summary>Check output</summary>", | ||
| "", | ||
| "````text", | ||
| (check.remaining ?? "").slice(-6000), |
There was a problem hiding this comment.
🟡 MINOR · output-sanitization · source: claude
Untrusted check output can close its Markdown fence and inject rendered text or mentions into public bot comments.
Evidence: sync-publish.ts:313-319 inserts check.remaining directly between fixed four-backtick fences. The neutralize function at :76-79 handles secrets, mentions, and HTML-comment openers but is not applied here. Both resolution and repair records call renderCheck.
Suggested fix: Neutralize check output and use a fence longer than any backtick run in the output, or escape it before rendering.
|
|
||
| /** | ||
| * Collects earlier sync pull requests for the agent: recorded resolutions, maintainer remarks, and the commits | ||
| * that landed. Remarks from anyone without write-level association are left out, since anyone can comment. |
There was a problem hiding this comment.
⚪ NIT · documentation · source: claude
The docstring and release-process documentation describe remark filtering by association instead of the actual write-permission requirement.
Evidence: sync-resolve.ts:410 and apps/cli/docs/release-process.md:350 say write-level association. sync-resolve.ts:366-388 checks collaborator permission against admin, maintain, and write after screening trusted associations; :358-363 uses the resulting writer set.
Suggested fix: Describe the filter as requiring repository write permission, optionally noting the preliminary association screen.
The agent's container holds the Anthropic key, so the text it writes is scanned before it leaves the job: the lines each merge adds beyond git's own merge, and every fix or repair commit. A secret-shaped match discards the result and hands the merge to a maintainer; the publish jobs repeat the scan before pushing, and result artifacts are redacted. Also fence check output so it cannot close its code block, report every file the agent edits, and stop promising more repair rounds after an escalation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/ai-review |
There was a problem hiding this comment.
Superseded by a newer AI review
🤖 AI Review
Both independent reviews completed. Verified all nine findings against the checked-out code: eight are confirmed and the container-timeout finding remains uncertain. The credential-scan bypass is critical. Other confirmed issues concern concurrent publication, skipped validation, stale reviews, false-positive secret detection, and three polish concerns. No fundamental conflict with the supplied next-branch work was identified.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🔴 CRITICAL | .github/scripts/sync-branches.ts:107 |
security |
codex | The secret scanner omits agent-written credentials in added lines beginning with two plus signs and in binary files, allowing those credentials into uploaded bundles and pushed commits. |
| 🟡 MINOR | .github/scripts/sync-publish.ts:414 |
concurrency |
claude | A repair already in progress can publish while a later sync resolution is running, causing the resolution's leased push to throw and leaving the source changes unsynchronized until another sync is triggered. |
| 🟡 MINOR | .github/scripts/sync-settle.ts:112 |
correctness |
codex | The settle step treats skipped or neutral required checks as passing and can stop recovery while required validation remains incomplete. |
| 🟡 MINOR | .github/scripts/sync-settle.ts:105 |
concurrency |
codex | An older resolution's review posted after a newer resolution comment can satisfy the newer resolution's review wait and trigger repairs using stale findings. |
| 🟡 MINOR | .github/scripts/sync-resolve.ts:641 |
false-positive |
claude | The secret gate rejects legitimate edits containing committed fake-key fixtures or ordinary identifiers matching its broad credential patterns. |
| 🟡 MINOR | .github/scripts/sync-agent.ts:182 |
robustness |
claude | The Docker client timeouts may fail to terminate the underlying agent or check container, allowing work to continue or blocking beyond the intended deadline. |
| ⚪ NIT | .github/scripts/sync-repair.ts:104 |
prompt-injection |
claude | Fixed four-backtick fences can be closed by embedded backtick runs, breaking the intended delimitation of untrusted agent context. |
| ⚪ NIT | .github/scripts/sync-resolve.ts:255 |
performance |
claude | Conflict context generation runs git show for every precedent commit before limiting the output to five relevant results. |
| ⚪ NIT | .github/scripts/sync-publish.ts:323 |
rendering |
claude | Check output is HTML-escaped before entering a code fence, causing literal escape text to appear in the rendered diagnostics. |
Stats
Claude findings: 6 · Codex findings: 3 · Confirmed: 8 · Refuted: 0 · Uncertain: 1
Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: manual · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
| const diff = gitOrThrow(git, ["diff", "--no-color", "--unified=0", from, commit]); | ||
| added.push( | ||
| ...diff | ||
| .split("\n") | ||
| .filter((line) => line.startsWith("+") && !line.startsWith("+++")) | ||
| .map((line) => line.slice(1)), |
There was a problem hiding this comment.
🔴 CRITICAL · security · source: codex
The secret scanner omits agent-written credentials in added lines beginning with two plus signs and in binary files, allowing those credentials into uploaded bundles and pushed commits.
Evidence: sync-branches.ts:107-112 filters out every diff line starting with '+++', including added content beginning with '++'. Binary diff notices contribute no added lines. sync-resolve.ts:641-664 uses this helper before creating the bundle, and sync-publish.ts:470-480 repeats it before pushing.
Suggested fix: Inspect changed blobs and paths directly, including binary content. If parsing textual diffs, distinguish file headers from added content using hunk boundaries.
| function pushSyncBranch(git: GitRunner, plan: ResolutionPlan, sha: string): void { | ||
| const branch = syncBranchName(plan); | ||
| gitOrThrow(git, [ | ||
| "push", | ||
| `--force-with-lease=refs/heads/${branch}:${plan.expectedSyncHead ?? ""}`, | ||
| "origin", | ||
| `${sha}:refs/heads/${branch}`, | ||
| ]); | ||
| } |
There was a problem hiding this comment.
🟡 MINOR · concurrency · source: claude
A repair already in progress can publish while a later sync resolution is running, causing the resolution's leased push to throw and leaving the source changes unsynchronized until another sync is triggered.
Evidence: sync-repair.yml:26-28 and sync-branches.yml:28-30 use different concurrency groups. sync-settle.ts:83-85 checks for active syncs only while planning. publishRepair at sync-publish.ts:607-619 can push against the unchanged old head, while pushSyncBranch at :414-422 throws on the resolution's subsequently stale lease.
Suggested fix: Coordinate publication between the workflows, or handle a stale resolution lease as superseded and automatically schedule another sync.
| const failing = checks.filter( | ||
| ({ conclusion }) => conclusion !== null && FAILED_CONCLUSIONS.has(conclusion), | ||
| ); | ||
| const firstAttempts = [ | ||
| ...new Set(failing.filter(({ attempt }) => attempt < 2).map(({ runId }) => runId)), | ||
| ]; | ||
| if (firstAttempts.length > 0) { | ||
| return { action: "rerun", runIds: firstAttempts }; | ||
| } | ||
| if (failing.length === 0 && state.findings.length === 0) { | ||
| return { action: "ready" }; |
There was a problem hiding this comment.
🟡 MINOR · correctness · source: codex
The settle step treats skipped or neutral required checks as passing and can stop recovery while required validation remains incomplete.
Evidence: sync-settle.ts:94-96 requires check presence and completed status, but :112-114 recognizes only FAILED_CONCLUSIONS, which excludes skipped and neutral. Lines 121-122 then return ready. fast-forward.ts:275 separately requires a success conclusion.
Suggested fix: Require success for every required check before returning ready, and route other completed conclusions to an explicit recovery or maintainer outcome.
| const reviewed = | ||
| state.aiReview !== undefined && | ||
| (resolvedAt === undefined || Date.parse(state.aiReview.submittedAt) >= resolvedAt); | ||
| if (!reviewed && resolvedAt !== undefined && state.now - resolvedAt < AI_REVIEW_WAIT_MS) { | ||
| return { action: "wait", reason: "the AI review has not finished" }; |
There was a problem hiding this comment.
🟡 MINOR · concurrency · source: codex
An older resolution's review posted after a newer resolution comment can satisfy the newer resolution's review wait and trigger repairs using stale findings.
Evidence: sync-settle.ts:105-107 checks only submission time. Lines 330-340 retain no reviewed commit or resolution identifier, and :355-373 collect unresolved findings without a lineage check. sync-publish.ts:496-505 posts the resolution comment before dispatching its review.
Suggested fix: Record the actual reviewed commit or resolution identifier in the review and require it to match the current resolution lineage before consuming its summary and findings.
| ); | ||
| result = { ...replayed, head: gitOrThrow(git, ["rev-parse", "HEAD"]), check }; | ||
| // The agent's container holds the API key; nothing secret-shaped it wrote may reach the artifact or the branch. | ||
| const written = agentWrittenText(git, plan.base, result.head); |
There was a problem hiding this comment.
🟡 MINOR · false-positive · source: claude
The secret gate rejects legitimate edits containing committed fake-key fixtures or ordinary identifiers matching its broad credential patterns.
Evidence: sync-resolve.ts:641-653 and sync-publish.ts:470-473 reject any scanned text changed by redactSecrets. ai-review/post-review.ts:487-498 includes the unbounded sk- prefix pattern. Fake-key fixtures exist at ai-review/post-review.test.ts:990-993 and sync-publish.test.ts:240.
Suggested fix: Account for matching fixture text already present on either merge parent, while retaining detection of actual exposed credentials.
| encoding: "utf8", | ||
| maxBuffer: 64 * 1024 * 1024, | ||
| stdio: ["ignore", "pipe", "inherit"], | ||
| timeout: options.timeoutMinutes * 60 * 1000, |
There was a problem hiding this comment.
🟡 MINOR · robustness · source: claude
The Docker client timeouts may fail to terminate the underlying agent or check container, allowing work to continue or blocking beyond the intended deadline.
Evidence: sync-agent.ts:129-188 applies spawnSync's timeout to docker run without --init, a container name, or explicit container termination. Its error branch at :190-198 returns without cleanup. sync-checks.ts:132-174 similarly times out docker run with sh as the command and performs no explicit cleanup.
Suggested fix: Give each container a unique name, use --init, and explicitly terminate and verify container shutdown on timeout or subprocess errors before continuing.
Adjudication (uncertain): The missing explicit cleanup is verified. However, the claimed failure depends on Docker/runtime behavior and the external CLI's SIGTERM handling. The pinned Claude CLI implementation was unavailable, so its alleged signal handling and resulting continued edits or blocking could not be established.
| function fenced(text: string): string { | ||
| return ["````text", text, "````"].join("\n"); | ||
| } |
There was a problem hiding this comment.
⚪ NIT · prompt-injection · source: claude
Fixed four-backtick fences can be closed by embedded backtick runs, breaking the intended delimitation of untrusted agent context.
Evidence: sync-repair.ts:104-106 uses a fixed fence for logs and review text at :144, :168, and :175. sync-resolve.ts:226-228 uses the same fixed delimiter for diffs. sync-publish.ts:84-88 already computes a fence longer than every embedded backtick run.
Suggested fix: Share the dynamically sized fence implementation across context writers.
| const head = side("HEAD"); | ||
| const incoming = side(conflict.sha); | ||
| const earlier = precedentCommits | ||
| .map(({ sha, pullRequest, isMerge }) => { | ||
| const shown = git([ | ||
| "show", | ||
| ...(isMerge ? ["--remerge-diff"] : []), | ||
| `--format=#### ${isMerge ? "Resolution" : "Maintainer fix"} %h from #${pullRequest}`, | ||
| sha, | ||
| "--", | ||
| file, | ||
| ]).stdout; | ||
| return shown.includes("\ndiff ") ? capLines(shown, 200) : ""; | ||
| }) | ||
| .filter(Boolean) | ||
| .slice(0, 5); |
There was a problem hiding this comment.
⚪ NIT · performance · source: claude
Conflict context generation runs git show for every precedent commit before limiting the output to five relevant results.
Evidence: sync-resolve.ts:257-270 eagerly maps all precedentCommits through git show, then filters and slices. gatherPrecedents requests up to 20 pull requests at :421-429 and collects their resolution commits at :489-495. groupResolver regenerates context for each attempt at :562.
Suggested fix: Iterate through precedents and stop after collecting five non-empty results.
| "", | ||
| "<details><summary>Check output</summary>", | ||
| "", | ||
| ...fencedOutput(neutralize((check.remaining ?? "").slice(-6000))), |
There was a problem hiding this comment.
⚪ NIT · rendering · source: claude
Check output is HTML-escaped before entering a code fence, causing literal escape text to appear in the rendered diagnostics.
Evidence: sync-publish.ts:323 passes neutralize(output) into fencedOutput. neutralize at :77-80 replaces HTML comment openers with <!-- and inserts zero-width characters into mentions; fencedOutput at :84-87 then wraps the transformed text.
Suggested fix: Apply secret redaction without HTML or mention escaping to output protected by the dynamically sized code fence.
Parse the diff by hunk instead of skipping lines that start with `+++`, so agent content beginning with `++` is scanned, and diff binaries as text. Check output inside a code fence is redacted rather than HTML-escaped, which rendered as literal entities. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/ai-review |
| const resolvedAt = lineageStart === -1 ? undefined : Date.parse(lineage[0]?.createdAt ?? ""); | ||
| const reviewed = | ||
| state.aiReview !== undefined && | ||
| (resolvedAt === undefined || Date.parse(state.aiReview.submittedAt) >= resolvedAt); |
There was a problem hiding this comment.
⚪ Severity: LOW
This treats any AI review submitted after the latest resolution comment as covering the current PR, without binding the review to pull.headSha. A delayed review of the prior head can therefore satisfy the gate after new sync edits land, allowing an unreviewed head to reach maintainer approval.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Bind the AI review to the current head commit instead of relying only on timestamps. GitHub's review objects expose commit_id. (1) Add commitId: string to the aiReview type in SettleState (line 51). (2) Where the latest AI review is mapped (around lines 335-339), map commit_id as well, for example .map(({ body, submitted_at, commit_id }) => ({ body, submittedAt: submitted_at, commitId: commit_id })). (3) In decideSettle (line 107), require state.aiReview.commitId === pull.headSha as well as the timestamp check. Update the test fixtures in sync-settle.test.ts to include commitId. A review of an earlier head then no longer counts as reviewed for the new head, and the gate keeps waiting for the AI review (or the 2h timeout) until a review of the current head arrives.
There was a problem hiding this comment.
🤖 AI Review
Both independent reviews were available. Verified all seven findings against the checked-out code: six are confirmed, including one defense-in-depth nit; the full Docker timeout-hang claim remains uncertain. No findings overlap, and no fundamental conflict with the supplied next-branch work was identified.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟠 MAJOR | .github/scripts/sync-branches.ts:245 |
correctness |
codex | A resolution plan can include a redundant develop merge, causing the publisher to reject a successfully resolved history. |
| 🟠 MAJOR | .github/scripts/sync-publish.ts:433 |
concurrency |
codex | An obsolete manual-resolution result can convert a sync PR to draft after its branch has already been repaired. |
| 🟡 MINOR | .github/scripts/sync-checks.ts:132 |
error-handling |
claude | The Docker subprocess timeouts may leave containers running and block the script instead of reaching its maintainer-handoff path. |
| 🟡 MINOR | .github/scripts/sync-checks.ts:88 |
correctness |
claude | Unreported fix-agent edits are incorrectly described as repository formatter changes in the published record. |
| 🟡 MINOR | .github/scripts/sync-settle.ts:105 |
review-attribution |
codex | A review containing findings from an older resolution can satisfy the review wait for a newer resolution because freshness is based only on submission time. |
| 🟡 MINOR | .github/scripts/sync-repair.ts:164 |
correctness |
codex | Unknown finding origins are presented to the repair agent as evidence that the merge introduced the commented line. |
| ⚪ NIT | .github/scripts/sync-publish.ts:439 |
security |
claude | The manual-result path pushes result.sha without explicitly validating that it belongs to the resolution plan. |
Stats
Claude findings: 3 · Codex findings: 4 · Confirmed: 6 · Refuted: 0 · Uncertain: 1
Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: manual · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
| const result = spawnSync( | ||
| "docker", | ||
| [ | ||
| "run", | ||
| "--rm", | ||
| "--user", | ||
| user, | ||
| "--env", | ||
| "HOME=/home/check", | ||
| "--env", | ||
| "CI=true", | ||
| "--env", | ||
| "MISE_YES=1", | ||
| "--env", | ||
| "MISE_TRUSTED_CONFIG_PATHS=/work", | ||
| "--env", | ||
| "MISE_DATA_DIR=/home/check/mise", | ||
| "--env", | ||
| "MISE_CACHE_DIR=/home/check/mise-cache", | ||
| "--volume", | ||
| `${workDir}:/work`, | ||
| "--volume", | ||
| `${join(workDir, ".git")}:/work/.git:ro`, | ||
| "--volume", | ||
| `${checkHome}:/home/check`, | ||
| "--workdir", | ||
| "/work", | ||
| image, | ||
| "sh", | ||
| "-c", | ||
| `mise install --quiet && mise exec -- sh -c '${steps}'`, | ||
| ], | ||
| { | ||
| encoding: "utf8", | ||
| maxBuffer: 64 * 1024 * 1024, | ||
| timeout: 20 * 60 * 1000, |
There was a problem hiding this comment.
🟡 MINOR · error-handling · source: claude
The Docker subprocess timeouts may leave containers running and block the script instead of reaching its maintainer-handoff path.
Evidence: .github/scripts/sync-checks.ts:132-169 runs docker with a 20-minute spawnSync timeout, without --init or explicit container cleanup. .github/scripts/sync-agent.ts:129-189 uses the same pattern for agent calls. .github/workflows/sync-branches.yml:98-99 ultimately limits the resolve job to 120 minutes.
Suggested fix: Give each container a unique name and enforce the deadline with an asynchronous subprocess and explicit docker kill cleanup before inspecting the worktree.
Adjudication (uncertain): The missing cleanup is verified. A bounded experiment using the pinned Bun 1.4.2 confirmed that spawnSync waits beyond its timeout when the child ignores SIGTERM. However, Docker daemon access is unavailable, and the installed Claude CLI's signal handling is outside the checked-in code, so the complete container-hang claim could not be verified.
| ...edits.filter(({ path }) => changed.includes(path)), | ||
| ...changed | ||
| .filter((path) => !listed.has(path) && !stagedBefore.has(path)) | ||
| .map((path) => ({ path, resolution: FORMATTED, precedent: null })), |
There was a problem hiding this comment.
🟡 MINOR · correctness · source: claude
Unreported fix-agent edits are incorrectly described as repository formatter changes in the published record.
Evidence: .github/scripts/sync-checks.ts:66 stages all agent changes, while :86-91 labels every committed path absent from outcome.files and stagedBefore as FORMATTED. .github/scripts/sync-publish.ts:297-318 renders those descriptions into the resolution record.
Suggested fix: Track changes made by each check separately from agent changes. Describe unreported agent edits explicitly instead of assigning them to the formatter.
| await io.convertToDraft(plan.pullRequest); | ||
| return { status: "needs-maintainer", pullRequest: plan.pullRequest }; | ||
| } | ||
| pushSyncBranch(git, plan, result.sha); |
There was a problem hiding this comment.
⚪ NIT · security · source: claude
The manual-result path pushes result.sha without explicitly validating that it belongs to the resolution plan.
Evidence: .github/scripts/sync-publish.ts:433-454 passes result.sha directly to pushSyncBranch. The resolved path at :457-473 instead validates the bundle head and planned history. The trusted producer assigns manual SHAs from plan.merges in .github/scripts/sync-resolve.ts:142-158 and :643-648.
Suggested fix: Reject a manual result unless its ref and SHA match an entry in plan.merges before pushing.
| const merges = [pair.target, pair.source] | ||
| .filter((ref) => !isAncestor(git, `refs/remotes/origin/${ref}`, syncHead)) | ||
| .map((ref) => ({ ref, sha: gitOrThrow(git, ["rev-parse", `refs/remotes/origin/${ref}`]) })); |
There was a problem hiding this comment.
🟠 MAJOR · correctness · source: codex
A resolution plan can include a redundant develop merge, causing the publisher to reject a successfully resolved history.
Evidence: .github/scripts/sync-branches.ts:245-247 checks both incoming refs against the original syncHead. .github/scripts/sync-resolve.ts:145-148 records every successful merge command, including a no-op. .github/scripts/sync-publish.ts:168-175 requires a distinct two-parent commit with the expected second parent for each planned merge.
Suggested fix: Exclude incoming commits already contained in earlier planned merges, or support validated no-op merges consistently in replay and history validation.
| if (result.status === "manual") { | ||
| if (plan.pullRequest !== null) { | ||
| await io.comment(plan.pullRequest, renderManualComment(plan, result)); | ||
| await io.convertToDraft(plan.pullRequest); | ||
| return { status: "needs-maintainer", pullRequest: plan.pullRequest }; |
There was a problem hiding this comment.
🟠 MAJOR · concurrency · source: codex
An obsolete manual-resolution result can convert a sync PR to draft after its branch has already been repaired.
Evidence: .github/scripts/sync-publish.ts:434-437 comments and converts the PR to draft without reading its current state or remote head. The lease at :414-420 applies only to pushes. .github/scripts/sync-branches.ts:241-242 skips draft PR updates.
Suggested fix: Verify that the PR remains open and its remote head matches plan.expectedSyncHead before publishing a manual handoff; discard superseded results.
| const reviewed = | ||
| state.aiReview !== undefined && | ||
| (resolvedAt === undefined || Date.parse(state.aiReview.submittedAt) >= resolvedAt); |
There was a problem hiding this comment.
🟡 MINOR · review-attribution · source: codex
A review containing findings from an older resolution can satisfy the review wait for a newer resolution because freshness is based only on submission time.
Evidence: .github/scripts/sync-settle.ts:105-107 compares submittedAt with the resolution comment timestamp. At :330-340 it retains only review body and submission time. .github/scripts/sync-publish.ts:336-337 records only a shortened resolution SHA, and .github/scripts/ai-review/post-review.ts:766 does not bind the review payload to the analyzed commit.
Suggested fix: Record the full resolution SHA and carry the actual analyzed SHA through review posting and settlement. Require a matching resolution review while allowing repair commits to retain that review.
| branches.length > 0 | ||
| ? `The commented line already exists on ${branches.map((branch) => `\`${branch}\``).join(" and ")}, so the issue likely predates the merge.` | ||
| : `The commented line is on neither \`${plan.source}\` nor \`${plan.target}\`; the merge or its fixes wrote it.`, |
There was a problem hiding this comment.
🟡 MINOR · correctness · source: codex
Unknown finding origins are presented to the repair agent as evidence that the merge introduced the commented line.
Evidence: .github/scripts/sync-repair.ts:113-119 returns an empty origins array for null positions, unavailable files, and missing or blank lines. At :159-166 every empty array produces the assertion that neither branch contains the line and the merge or its fixes wrote it.
Suggested fix: Represent unknown attribution separately from verified absence on both branches, and require inspection of the original review location and history when attribution is unknown.
TL;DR
nextnow catches up withdevelopwithout a maintainer hand-resolving every conflict. When a push todevelopconflicts withnext, Claude resolves it in a sandbox, runs the repository's quality checks, and publishes the result to one reusablesync/develop-into-nextpull request. Once that PR's CI and AI review finish, a bounded repair loop fixes what the merge broke and answers the review. Landing onnextstill needs a maintainer's approval.Before
flowchart LR push["push to develop"] --> merge{"merge into next"} merge -->|clean| next[("next")] merge -->|conflict| pr["manual sync PR<br/>(develop tip)"] pr --> human["maintainer merges,<br/>resolves, pushes"] human --> approve["approve"] --> next pr -.->|"later pushes skip"| skip["next falls behind"]After
flowchart LR push["push to develop"] --> merge{"merge into next"} merge -->|clean, no PR| next[("next")] merge -->|conflict| claude["Claude resolves<br/>+ check:all + fix"] merge -->|"PR open"| update["update the one sync PR"] claude --> pr["single sync PR<br/>+ AI review"] update --> pr pr --> settle{"CI + review settled"} settle -->|"failures or findings"| repair["rerun once, then<br/>Claude repair round"] repair --> pr settle -->|green| approve["maintainer approves"] --> next settle -->|"2 rounds"| draft["draft + ping @supabase/cli"]Why
Every
develop→nextconflict opened a manual sync PR pointing at thedeveloptip, and every later sync skipped while it stayed open, sonextdrifted further behind each day. The last one fell 37 files behind before anyone resolved it.What changed
Sync branches): a conflict runs Claude in a container that sees only the worktree (.gitread-only), the context files, and the Anthropic key; no shell, no network tools. Conflicts are resolved in groups of up to eight files with per-call turn, budget, and time caps; a group that fails is resumed once in the same session. Broad searches go to a read-only explorer subagent on a cheaper model.git show --remerge-diffof how the same files were resolved) are fed back to the agent, so a decision is not asked twice. Remarks from accounts without write access are ignored.check:all(including the workflow linter) in a credential-free container provisioned by the tree's ownmise.toml; one agent call fixes what fails, committed on top of the merge.developpush. Choices betweendevelopandnextbehavior are recorded as decisions and request review from@supabase/cli; every.github/file a merge resolves or a fix changes is always a decision. The publish job validates the exact merge structure before pushing with a lease, then starts the AI review.Sync repair, new): once the head commit's checks and the AI review settle, failed jobs are re-run once, then Claude repairs what still fails and replies on every AI finding, fixing only issues the merge introduced and declining the rest as out of scope. At most two rounds per resolution; after that the PR becomes a draft and pings@supabase/cli.Require fast-forwardis ignored, since branch policy fails it on sync PRs by design.developornextplus maintainer remarks; this risk is documented in the release runbook.main-into-developkeeps its manual conflict PR. The runbook and maintainer guide describe the new flow.🤖 Generated with Claude Code