Skip to content

ci(release): resolve and repair develop-into-next syncs with Claude - #7083

Open
avallete wants to merge 8 commits into
developfrom
avallete/ia-workflow-next-sync-5c9aa1
Open

avallete wants to merge 8 commits into
developfrom
avallete/ia-workflow-next-sync-5c9aa1

Conversation

@avallete

@avallete avallete commented Oct 9, 2026

Copy link
Copy Markdown
Member

TL;DR

next now catches up with develop without a maintainer hand-resolving every conflict. When a push to develop conflicts with next, Claude resolves it in a sandbox, runs the repository's quality checks, and publishes the result to one reusable sync/develop-into-next pull request. Once that PR's CI and AI review finish, a bounded repair loop fixes what the merge broke and answers the review. Landing on next still 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"]
Loading

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"]
Loading

Why

Every develop → next conflict opened a manual sync PR pointing at the develop tip, and every later sync skipped while it stayed open, so next drifted further behind each day. The last one fell 37 files behind before anyone resolved it.

What changed

  • Resolve (Sync branches): a conflict runs Claude in a container that sees only the worktree (.git read-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.
  • Precedent: earlier sync PRs (resolution and repair records, maintainer reviews, comments, and fix commits, and git show --remerge-diff of 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.
  • Checks before publishing: the merged tree runs the formatter and check:all (including the workflow linter) in a credential-free container provisioned by the tree's own mise.toml; one agent call fixes what fails, committed on top of the merge.
  • One PR: the sync PR is created once and updated on every later develop push. Choices between develop and next behavior 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.
  • Repair loop (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-forward is ignored, since branch policy fails it on sync PRs by design.
  • Trust model: agent-written changes, including workflow files, run in the sync PR's CI before review. The agent only reads code already reviewed into develop or next plus maintainer remarks; this risk is documented in the release runbook.
  • main-into-develop keeps its manual conflict PR. The runbook and maintainer guide describe the new flow.

🤖 Generated with Claude Code

avallete and others added 2 commits October 9, 2026 20:23
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>
@avallete
avallete requested review from a team and supabase-oss as code owners October 9, 2026 19:23
@avallete

avallete commented Oct 9, 2026

Copy link
Copy Markdown
Member Author

/ai-review

@github-actions github-actions Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread .github/scripts/sync-resolve.ts Outdated
Comment on lines +341 to +346
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());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

Comment thread .github/scripts/sync-checks.ts Outdated
Comment on lines +123 to +124
"pnpm run --if-present check:config-api",
].join(" && ");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

Comment on lines +597 to +607
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);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

Comment on lines +308 to +309
? (await githubRequest<{ run_attempt: number }>(token, `${base}/actions/jobs/${run.id}`))
.run_attempt

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

Comment on lines +149 to +155
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.`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread .github/scripts/sync-settle.ts Outdated
Comment on lines +171 to +175
reviewThreads(first: 100) {
nodes {
id
isResolved
comments(first: 50) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread .github/scripts/sync-agent.ts Outdated
session,
};
}
const costUsd = output.total_cost_usd ?? 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +84 to +89
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 })),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread .github/workflows/sync-branches.yml Outdated
Comment on lines +88 to +89
AGENT_IMAGE: node:24-bookworm-slim
CHECK_IMAGE: sync-check:local

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ 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.

avallete and others added 2 commits October 10, 2026 11:12
…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>
@avallete

Copy link
Copy Markdown
Member Author

/ai-review

@github-actions github-actions Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread .github/scripts/sync-settle.ts Outdated
Comment on lines +102 to +144
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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

Comment on lines +124 to +144
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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +161 to +166
publish:
name: Publish the sync pull request
needs:
- sync
- resolve
runs-on: ubuntu-latest

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +127 to +164
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 ?? "" },
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread .github/scripts/sync-publish.ts Outdated
Comment on lines +249 to +280
`${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)}`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +104 to +108
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" };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

Comment on lines +417 to +419
if (plan.pullRequest !== null) {
await io.comment(plan.pullRequest, renderManualComment(plan, result));
await io.convertToDraft(plan.pullRequest);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +140 to +143
const merge = git(["-c", "merge.conflictStyle=zdiff3", "merge", "--no-ff", "-m", message, sha]);
if (merge.status === 0) {
merges.push({ ref, sha, resolution: null });
continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread .github/scripts/promotion-shared.ts Outdated
Comment on lines 84 to 87
if (response.status === 204) {
return undefined as T;
}
return (await response.json()) as T;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread .github/scripts/sync-settle.ts Outdated
Comment on lines +299 to +304
// 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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>
@avallete

Copy link
Copy Markdown
Member Author

/ai-review

@github-actions github-actions Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +72 to +74
function neutralize(text: string): string {
return text.replace(/@(?=[\w-])/g, "@\u200b").replace(/<!--/g, "&lt;!--");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

Comment on lines +128 to +163
"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 ?? "" },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +161 to +166
publish:
name: Publish the sync pull request
needs:
- sync
- resolve
runs-on: ubuntu-latest

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +189 to +197
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 });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +57 to +72
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;
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread .github/scripts/sync-checks.ts Outdated
Comment on lines +101 to +104
JSON.parse(readFileSync(join(workDir, "package.json"), "utf8")) as {
scripts?: Record<string, string>;
}
).scripts?.["check:all"];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

Comment on lines +207 to +209
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}`]) }));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +185 to +190
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]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +105 to +109
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" };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +421 to +425
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 };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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>
@avallete

Copy link
Copy Markdown
Member Author

/ai-review

@github-actions github-actions Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +154 to +159
- name: Upload resolution
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
with:
name: sync-resolution
path: ${{ runner.temp }}/sync-resolution
retention-days: 7

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

Comment on lines +207 to +209
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}`]) }));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

Comment on lines +162 to +168
`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 ?? "" },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

Comment on lines +163 to +168
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

Comment on lines +111 to +124
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);
});
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +163 to +165
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.`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread .github/scripts/sync-settle.ts Outdated
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.`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread .github/sync-branches/resolve-prompt.md Outdated
## 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread .github/scripts/sync-publish.ts Outdated
"<details><summary>Check output</summary>",
"",
"````text",
(check.remaining ?? "").slice(-6000),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread .github/scripts/sync-resolve.ts Outdated

/**
* 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ 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>
@avallete

Copy link
Copy Markdown
Member Author

/ai-review

@github-actions github-actions Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread .github/scripts/sync-branches.ts Outdated
Comment on lines +107 to +112
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)),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

Comment on lines +414 to +422
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}`,
]);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +112 to +122
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" };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +105 to +109
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" };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +104 to +106
function fenced(text: string): string {
return ["````text", text, "````"].join("\n");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ 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.

Comment on lines +255 to +270
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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ 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.

Comment thread .github/scripts/sync-publish.ts Outdated
"",
"<details><summary>Check output</summary>",
"",
...fencedOutput(neutralize((check.remaining ?? "").slice(-6000))),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ 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>
@avallete

Copy link
Copy Markdown
Member Author

/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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ 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.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

Comment on lines +132 to +167
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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +88 to +91
...edits.filter(({ path }) => changed.includes(path)),
...changed
.filter((path) => !listed.has(path) && !stagedBefore.has(path))
.map((path) => ({ path, resolution: FORMATTED, precedent: null })),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ 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.

Comment on lines +245 to +247
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}`]) }));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

Comment on lines +433 to +437
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 };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

Comment on lines +105 to +107
const reviewed =
state.aiReview !== undefined &&
(resolvedAt === undefined || Date.parse(state.aiReview.submittedAt) >= resolvedAt);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +164 to +166
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.`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant