feat(github): withhold a plan body rather than overflow the comment

[aparajon]

A rollout of many targets against many tables renders past GitHub's 65,536
character comment cap. A comment over the cap is rejected outright, so the pull
request ends up with no plan at all rather than a long one.

An oversized comment now gives up DDL bodies until it fits, largest plan first,
and stops as soon as it does. What it never gives up is the summary lines: every
target is still named, every plan is still counted, and each withheld plan is
replaced by the command that prints it in full.

A group's plan identifier comes from the member plan rows its members were
stored with, so the pointer resolves to exactly the plan that was withheld. The
primary runs the reviewed plan itself and has no member plan row of its own, so
its group carries the reviewed plan's identifier.

Three targets planning independently: one needs a single statement, one needs
2,000, and one needs 2,500.

Before                                            After

+--------------------------------------------+    +--------------------------------------------+
| [open] my_db_1 - 1 DDL statement           |    | [open] my_db_1 - 1 DDL statement           |
| [+] my_db_2 - 2000 DDL statements          |    | [+] my_db_2 - 2000 DDL statements          |
| [+] my_db_3 - 2500 DDL statements          |    | [+] my_db_3 - 2500 DDL statements          |
+--------------------------------------------+    |     schemabot list-plans plan_91ab         |
| rendered: 66,583 characters                |    +--------------------------------------------+
|   x over GitHub's 65,536 cap               |    | rendered: 33,769 characters                |
|   x the comment is rejected; the PR        |    |   every target named and counted;          |
|     gets no plan at all                    |    |   the largest plan is a pointer            |
+--------------------------------------------+    +--------------------------------------------+

This upholds RV-3: what an operator consents to stays specific. A plan the
comment has no room for is named, counted, and reachable rather than silently
absent, and no summary of withheld DDL is invented in its place.

The rendered comment, with one plan withheld

Schema Change Plan — Production

Database: testapp | Type: MySQL

Started at 2026-09-17 18:42:05 UTC

✅ Planned separately for all 3 targets (primary/my_db_1, primary/my_db_2, primary/my_db_3) — 3 distinct plans. Each target applies its own.

`primary/my_db_1` (primary) — 1 DDL statement

ALTER TABLE `t0` ADD COLUMN `c` int;

`primary/my_db_2` — 2000 DDL statements

ALTER TABLE `t0` ADD COLUMN `c` int;

... 1998 more statements rendered in full ...

ALTER TABLE `t1999` ADD COLUMN `c` int;

`primary/my_db_3` — 2500 DDL statements

This plan is too large to render here. To read it in full:

schemabot list-plans plan_91ab

⚠️ Applying runs each target's own plan, including the ones collapsed above.

📋 Plan: 3 distinct plans on 3 targets

---

▶️ To apply all schema changes from this PR, comment:

schemabot apply -e production

Opened by Claude (Claude Opus 5).

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1427, 865e7d4.

Verdict: 4 findings — 3 non-blocking (withholding order, no terminal guarantee, multi-env gap), 1 suggestion.

Non-blocking

plan.go:386 — largestUnclampedGroup ignores Primary, so the reviewed plan's DDL is withheld first. With primary=3000 statements and eu/ap=2000 each, the primary is picked first and its <details open> block shows "This plan is too large to render here." while a collapsed non-reviewed target carries full DDL — the inverse of writePlanGroups' rule that the reviewed plan is "the block they should not have to expand". Aggravated by renderedDDLSize measuring raw statement bytes: every group body is already capped at 32768 by newDDLBlockBudget, so withholding the primary bought 59 bytes in the probe. Skip primary groups until they are the only candidate left.

plan.go:374 — the clamp loop has no terminal size guarantee. When every group is clamped and the body is still over the cap, largestUnclampedGroup returns -1, break falls through to return out, and postTrackedPlanComment takes a 422 from GitHub — no plan comment at all, the exact outcome the feature exists to prevent. writeUnsafeWarning (plan.go:1651) is the plausible source of unbounded non-group content (~65 B per unsafe finding, no budget), though I could not confirm a real plan reaching ~64 KB there. A final hard truncate at the cap closes it.

plan.go:2121 — the multi-environment comment never renders DeploymentDrift.Plans and has no groupsCarryWork guard. writeEnvironmentPlanSection short-circuits on the primary's totalChanges == 0 and prints "✅ No schema changes detected" for an environment whose apply will run DDL on a non-primary target, directly under a drift line saying "1 needs this change". The single-env path guards exactly this at plan.go:458; pre-existing from the stacked commit, but the stack should not merge without the sweep.

General suggestions

plan.go:369 — each clamp re-renders the whole comment, re-parsing every statement of every unclamped group. writePlanDDLBlock re-runs parser.Classify plus ddl.FormatDDLForDialect on every statement before truncation, so 10 plans × 2000 statements ≈ 9 rounds ≈ 100k+ SQL parses on the webhook request path; the PR's own TestRenderPlanComment_WithholdingStopsWhenTheCommentFits burns 0.30 s CPU versus 0.00 s unclamped. Only the newly clamped group changes between rounds, so per-group bodies could be cached and re-assembled.

The one thing that could have broken, verified

Clamping mutates the group slice the caller owns. It does not: plan.go:356-361 does a real copy-on-write — slices.Clone gives a fresh backing array, the DeploymentDriftData struct is copied by value, data is a value parameter, and only the value field clamped is written, so no Changes slice is aliased into and the caller's drift pointer is untouched. renderPlanComment re-reads data.planGroups(), which returns the clone, so each round's flag does reach writePlanGroupBody.

Verified correct

• plan.go:352 — the len(data.planGroups()) == 0 guard makes the line-358 *data.DeploymentDrift deref safe.
• plan.go:363-373 — the loop is bounded by len(groups) and terminates on -1 or on fitting.
• plan_drift.go:151 — i == 0 matches the existing Primary: i == 0 convention, and PlanID is stamped before SortStableFunc, so the sort cannot mis-pair it.
• plan_drift.go:66 — primaryPlan.GetPlanId() is non-empty wherever groups exist, so the line-1421 fallback is defensive, not reachable.
• plan_member_plans.go:86 — an unstorable member plan is reclassified errored (rollup not Clean), so no rendered group carries an empty non-primary PlanID.
• plan.go:1427 — schemabot list-plans <plan_id> really accepts a positional id and does not exclude member plan rows, so the pointer resolves.
• plan.go:334-339 — planCommentChromeHeadroom (1024) matches the existing commentChromeHeadroom for the same footer, and byte-counting over-counts against GitHub's character cap, so the budget errs conservative.

This review was generated by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Approved on Kiran's (@kmuddukrishna) behalf by the scheduled review agent — no blocking findings at 865e7d4. See the review comment above; non-blocking findings and suggestions, if any, are not merge gates.

[morgo]

🤖 Reviewed on Morgan's behalf, at 865e7d47. Approving — the withholding loop is correct, terminates, does not mutate the caller's data, and points at a command that really exists with the argument shape it uses. This also closes the budget multiplication I raised on #1425, which I confirmed rather than assumed. One finding: the guard is scoped to the grouped path, and the same fleet size overflows the comment on the path beside it.

1. A blocked rollout still renders past GitHub's cap, unguarded

RenderPlanComment returns before clamping when len(data.planGroups()) == 0. Grouping is gated on rollup.Clean (#1424), so a blocked rollup never has groups — and its per-member list is the one list #1426 deliberately left unclamped, for good reasons.

Measured, one entry per member with a routine timeout detail:

 500 blocked members ->  56,033 chars   (under)
 600 blocked members ->  67,133 chars   over GitHub's 65,536 cap
 800 blocked members ->  89,333 chars   over
1200 blocked members -> 133,933 chars   over

That is roughly 112 characters per member, so the cliff sits near 585. #1426's own motivation says "a targets: list can address hundreds of targets" and its example uses 144, so this is above the documented scale but the same order of magnitude — and it is the case where losing the comment costs the most, because a blocked rollup is the only place the comment says which member failed and why. The failure mode is exactly the one this PR's first sentence describes: the comment is rejected and the PR ends up with no plan at all.

RenderPlanComment's new doc comment opens "A rollout of N targets against M tables can render past what GitHub will accept, so an oversized comment gives up DDL bodies until it fits", which reads as a property of the function. It is a property of one branch of it.

I do not think the fix is to clamp the blocked list — #1426 argues convincingly against that, and I agree. The options that preserve both properties are a collapsed block holding the tail of the member list, or a final backstop that truncates with a visible marker when nothing else can be given up. The second is worth having regardless: today, if every group is clamped and the comment still does not fit, the loop breaks and returns the oversized string unchanged, so there is no path on which the function guarantees its own budget.

Notes

• renderedDDLSize under-weights plans with many short statements. It sums raw statement bytes and ignores what rendering adds — fences, keyspace and shard headings, blank lines. A group of 2,000 short ALTERs and a group of 20 long ones can sum alike while rendering very differently, so "largest first" can pick the wrong one and need an extra iteration. Only costs re-renders, since the loop keeps going until it fits — the comment already says it is for ordering only, which is the right disclaimer. Mentioning it because the re-render is a full renderPlanComment each time.
• g.Empty() in largestUnclampedGroup is redundant. An empty group has renderedDDLSize == 0, and size > bestSize starts at bestSize = 0, so it can never be selected. Removing the check leaves the suite green — which is fine, because there is nothing behind it to test. Harmless as defense in depth; noting it so it is not mistaken for a load-bearing guard later.
• "they are identical, so any one of them answers the question" is true of the work, not of the rows. Group members share a fingerprint, so each has its own stored plan row with its own identifier and the same canonical change set. schemabot list-plans <first member's id> therefore prints a row that is equivalent to, not the same as, the other members'. Right call; the comment could say "equivalent" to avoid a reader wondering why the members' IDs differ.
• The empty-PlanID fallback prints prose instead of a broken command, which is the right shape — and PlanIdentifier's own doc says empty is a legitimate state ("Empty means the member runs the plan the apply itself was created [from]"), so this is a reachable branch and not just defense.

What I checked rather than took on trust

• Fault injection — five of six. Disabling the clamp entirely → three tests; withholding smallest-first instead of largest → OversizedRolloutWithholdsTheLargestPlan + WithheldPlanWithoutAnIdentifierSaysSo; printing the command with an empty PlanID → WithheldPlanWithoutAnIdentifierSaysSo; clamping every group instead of stopping once it fits → WithholdingStopsWhenTheCommentFits; giving the primary's group its member plan ID instead of the reviewed plan's → GroupsCarryThePlanToPrintThem. The only green one is the redundant g.Empty() guard above.
• schemabot list-plans <id> is a real invocation. pkg/cmd/main.go:53 registers PlansCmd under name:"list-plans", and plans_test.go:34 parses list-plans plan-1784327902264169990 — a bare positional plan ID, exactly the shape rendered. The schemabot prefix matches the convention in apply.go and apply_commands.go.
• The loop terminates and cannot spin. for range groups bounds the iterations, largestUnclampedGroup returns −1 once every non-empty group is clamped, and each iteration sets exactly one clamped flag, so the worst case is one render per group plus the first.
• The caller's data is genuinely not mutated. slices.Clone copies the group slice, drift := *data.DeploymentDrift copies the struct, and data is a value parameter — so the clamped writes land only in this call's copy. The comment's claim holds through all three levels, which I traced rather than trusted because DeploymentDrift is a pointer field.
• This does close #1425's budget multiplication. With maxCommentDDLLen = 32768 per call to writeKeyspaceChanges, two large groups render ~65.5k, above planCommentBudget = 64512, so the loop engages and withholds one. The overflow I measured on #1425 (four large groups → 132,313 chars) cannot survive to the top of the stack.
• clamped is unexported on an exported struct, so a caller cannot preset it. Matches the field comment's claim that it is set while rendering and never by the caller.
• Merge-base against pr1426 is an ordinary commit; +233/−16, and the sixteen deletions are the two writeKeyspaceChanges call sites, the deploymentDriftPreview/deploymentPlanGroups signatures, and their doc lines. No test deletions. CI: 41 checks, no genuine failures.