fix(github): lead wide shard groups with coverage instead of walling the plan comment

[aparajon]

Why this matters

A sharded plan comment names every shard in each group heading, so at real shard counts the reader scrolls a wall of backticked names before reaching the first DDL statement — and the list answers nothing, because the question at plan time is coverage, not enumeration. This PR makes a wide group lead with its coverage on a single collapsed line, with the names one click away.

What it does

Group headings pick a shape by width:

shard group heading
  ├─ ≤8 shards  → inline names: **shards `-40`, `40-80`**
  └─ wider      → one collapsed line: ▸ all 32 shards   (or "19 of 32 shards" for a subset)
                   expands to the full backticked name list

Shard-list suffixes that ride inside line items — the unsafe, blocked, and direct-execution finding lines the reviewer consents against — state their keyspace coverage past the same width limit, so a subset never reads like whole-keyspace coverage:

- `mutes` (12 of 32 shards): DROP COLUMN removes data     (or "all 32 shards")

A finding line has no room for a collapsed list, so the full names stay reachable in the DDL section's collapsed shard groups. Shard names are normalized through markdownInlineCode so a name from the topology can't break the code span or the comment structure.

Plan: Many Shards (32)

Schema Change Plan — Production

Database: cdb_resolute | Type: Strata

Requested by @jackjackbits at 2026-01-01 00:00:00 UTC · planned from abcdef1

Keyspace: cdb_resolute_sharded

all 32 shards

-08, 08-10, 10-18, 18-20, 20-28, 28-30, 30-38, 38-40, 40-48, 48-50, 50-58, 58-60, 60-68, 68-70, 70-78, 78-80, 80-88, 88-90, 90-98, 98-a0, a0-a8, a8-b0, b0-b8, b8-c0, c0-c8, c8-d0, d0-d8, d8-e0, e0-e8, e8-f0, f0-f8, f8-

ALTER TABLE `mutes` ADD INDEX `created_at`(`created_at`);

📋 Plan: 1 table to alter

---

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

schemabot apply -e production

How it moves us toward the northstar

Sharded databases are only a first-class target if "plan once, fan out per shard" stays legible at fleet-real shard counts. This is the plan-comment half of that; the apply-comment half (keyspace grouping, table-altitude progress) follows in #1189–#1191.

Opened by Claude (Fable 5).

[Copilot]

Pull request overview

Improves the readability of sharded plan PR comments for wide Strata/Vitess keyspaces by leading shard groups with coverage (e.g., “all 32 shards” / “15 of 16 shards”) and moving long shard name lists behind a collapsible disclosure, avoiding a large inline “wall of shards” before the DDL.

Changes:

• Add wide-keyspace shard group headings (“all N shards” / “N of M shards”) and collapse long shard name lists behind a Show shards <details> block.
• Compact shard suffixes in list-item contexts (e.g., unsafe findings) to a count once they exceed the same inline-name limit.
• Add preview + tests covering uniform-wide, divergent-wide, and unsafe-wide shard scenarios.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
TEMPLATES.md	Adds a rendered example of the “Many Shards (32)” plan output.
pkg/webhook/templates/sharded_plan_test.go	Adds regression tests for wide shard groups (collapsed lists + coverage headings) and compacted unsafe suffixes.
pkg/webhook/templates/preview_sharded.go	Adds a new preview generator for a wide uniform sharded plan and a helper to generate keyrange-like shard names.
pkg/webhook/templates/plan.go	Implements wide-group headings, the inline-name limit constant, and collapsed shard list rendering; compacts list-item shard suffixes.
pkg/webhook/templates/apply.go	Reuses the shared shard inline-name limit constant for apply progress shard summaries.
pkg/cmd/internal/templates/preview_comment.go	Exposes the new “Many Shards (32)” preview in the CLI preview output.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1188, 669a25d.

Verdict: 5 findings — 3 non-blocking (gate-line information loss, an inaccurate doc claim, a boundary coverage gap), 2 suggestions. No blocking issues; the "all N shards" coverage claim is sound.

Non-blocking

1. The compact gate-line form drops the denominator, so a subset and a whole keyspace read identically on the line the reviewer consents against.
pkg/webhook/templates/plan.go:848-850

	if len(shards) > shardNamesInlineLimit {
		return fmt.Sprintf("%d shards", len(shards))
	}

writeShardGroupHeading deliberately distinguishes all 32 shards from 12 of 32 shards (plan.go:880-883), but planShardList — reached from the unsafe, blocked and direct-execution finding lines (plan.go:953, plan.go:996, plan.go:1013, apply_commands.go:232) — emits a bare count with no total.

Failure scenario, rendered at head: a DROP COLUMN unsafe on 12 of a 32-shard keyspace renders

- `mutes` (12 shards): DROP COLUMN removes data

directly under a DDL heading that says all 32 shards. Pre-PR the reviewer read the 12 names on the gate line itself; now they get a count that does not say whether 12 is the keyspace or a third of it, and must correlate it by hand against a statement-keyed group in the DDL section. Since these four call sites are precisely the destructive/blocked/direct-execution consent lines, %d of %d shards (the total is already available at each call site as len(ks.Shards)) is worth the plumbing.

2. The doc comment justifies the drop with a fallback that does not exist — the CLI plan renders no shard names.
pkg/webhook/templates/plan.go:846

// for a collapsed name list; the per-shard detail stays in the CLI plan.

grep -in shard pkg/cmd/commands/plan.go returns nothing: writePlanBody builds from result.FlatTables() / result.UnsafeChanges() and never touches PlanResponse.Shards. Only the --json branch carries the shard list, via raw marshalling. An operator who follows the stated fallback and runs schemabot plan -e production gets namespace-level output with no shard names at all. Either fix the claim (point at --json, or at the collapsed block in the DDL section) or make finding 1's change so the claim is unnecessary.

3. Nothing pins the 8/9 boundary the new constant defines.
pkg/webhook/templates/sharded_plan_test.go:157

The added tests build 32, 15+1 and 12 shard groups; grep -rn shardNamesInlineLimit --include='*_test.go' pkg/webhook/templates returns no hits. I probed the real behavior via go test -overlay against the head worktree and it is correct today:

n=1 line="**shard `s00`**"
n=7 line="**shards `s00`, … `s06`**"
n=8 line="**shards `s00`, … `s07`**"
n=9 line="<summary><b>all 9 shards</b></summary>"

but flipping plan.go:876 if len(shards) <= shardNamesInlineLimit to < moves an 8-shard group from an inline heading to a collapsed all 8 shards block with no test failing. A two-case table at 8 and 9 would cost four lines.

General suggestions

4. The apply-side twin still walls — and could at least share the new constant.
pkg/webhook/templates/sharded_apply.go:256-265

func shardList(shards []ShardStatus) string {
	names := make([]string, len(shards))
	for i, s := range shards {
		names[i] = fmt.Sprintf("`%s`", s.Shard)
	}

Its sole caller (sharded_apply.go:195) emits the group heading immediately above writeShardStatusTable, which already prints one row per shard — so on a wide divergent apply every name is walled twice in the comment the operator watches during the rollout. The PR body explicitly scopes the apply half to #1189–#1191, so this is not a defect of this PR; flagging it so the follow-up picks up both the shardNamesInlineLimit cap and the markdownInlineCode normalization rather than leaving the raw fmt.Sprintf("%s", …) quoting behind (note writeShardStatusTable at sharded_apply.go:341 quotes raw too).

5. quotedShardNames is the third copy of the same map-to-code-spans loop; the reusable shape is generic.
pkg/webhook/templates/plan.go:861-867

The identical for i, name := range xs { quoted[i] = markdownInlineCode(name) } followed by strings.Join(quoted, ", ") already exists twice in the same package (errors.go:49-53, errors.go:301-304). A markdownInlineCodeList([]string) []string next to markdownInlineCode would collapse all three and give the next list — namespaces, tables, environments — one place to copy from.

The one thing that could have broken, verified

Can all N shards ever claim coverage the plan does not have? This is the only new assertion in the comment, and it is the artifact a reviewer approves, so an overstated claim would be a correctness bug rather than a rendering nit. It holds, for two independent reasons.

Structurally: groupKeyspaceShardsByStatements (plan.go:804-820) appends every input shard into exactly one group, so it partitions the slice. The all %d shards branch is only reachable when len(shards) == totalShards (plan.go:880-883), and totalShards is len(shards) of the slice being partitioned (plan.go:777, plan.go:784). A group can therefore print all only when it is the whole slice; every multi-group call necessarily passes a strict subset and takes the fraction branch. writeShardedPlanDDL has exactly one caller (plan.go:742, writeShardedPlanDDL(sb, ks.Shards)), so the "total" is never a partial view.

Upstream: the only path that shortens ks.Shards relative to the real keyspace is the malformed-shard continue at pkg/webhook/plan.go:866 — a shard reporting changes whose DDL came back empty. That same branch appends "shard %q … plan is incomplete for this shard" to data.Errors, which RenderPlanComment renders in the same comment, so the incompleteness is disclosed rather than hidden behind the coverage claim. It is also not a regression: pre-PR that path listed the surviving names and never mentioned the missing shard either. Not a finding.

Verified correct

• Boundary behavior is right and internally consistent: planShardList collapses at len > 8 (plan.go:848) and writeShardGroupHeading goes inline at len <= 8 (plan.go:876) — the same pivot, no gap and no double-collapse. Probed at 1/7/8/9 (output quoted in finding 3).
• Zero- and one-shard cases are safe: a group always holds ≥1 shard (plan.go:815), and every other planShardList call site is guarded by if len(c.Shards) > 0 (plan.go:952, plan.go:995, plan.go:1012, apply_commands.go:231), so the degenerate "shards " string is unreachable. A single shard still reads shard x``.
• The apply-side constant swap is a pure rename: the deleted const inlineLimit = 8 and the new package-level shardNamesInlineLimit = 8 (plan.go:841) hold the same value under the same <= at apply.go:883; same package, no import, byte-identical output.
• Narrow keyspaces render unchanged: the <=8 arm reproduces the deleted statement exactly (fmt.Fprintf(sb, "**%s**\n\n", planShardList(shards))), which is why the TEMPLATES.md diff is addition-only — every existing 2–3 shard fixture is untouched.
• Shard-name sanitization is a strict improvement, not a behavior change for real names: markdownInlineCode (errors.go:319-322) strips backticks and collapses whitespace, so a hex Vitess keyrange is unchanged while a hostile topology name can no longer break the code span. Matches the AGENTS.md rule at line 154 about never rendering untrusted strings raw in PR-facing markdown.
• TEMPLATES.md is regenerated consistently. I ran go run ./pkg/cmd preview comment_sharded_all at head and the PLAN: MANY SHARDS (32) block it emits — <summary><b>all 32 shards</b></summary> plus the full 32-name list from -08 to f8- — matches the committed section byte for byte, and the derived anchor plan-many-shards-32 matches the checked-in <a name=…>. The new entry is also in the file's alphabetical slot in preview_comment.go:353.
• previewShardRange(i, 32) produces correct contiguous keyranges (width 8, -08, 08-10, 10-18, … f8-) with the first and last correctly open-ended via the case 0 / case n-1 arms. The 256 / n truncation for non-power-of-two n is unreachable — the sole caller passes 32.
• The emitted <details> is well formed and nests correctly inside the outer keyspace <details> from writeCollapsibleKeyspaceChanges: blank line after <summary>, blank line before </details>, \n\n before the ```sql fence. Same blank-line-delimited pattern the rest of the package already uses.
• Satisfied groups still read correctly at width: a wide satisfied group gets its collapsed N of M shards heading followed by _Already applied — no change._ rather than an empty fence, and shardGroupSignature (plan.go:828-835) still keys on satisfied-vs-changing, so a satisfied shard can never be counted inside a changing group's coverage number.
• Every changed symbol is package-private to pkg/webhook/templates and every caller was swept: planShardList has four call sites, shardNamesInlineLimit two readers, writeShardGroupHeading and quotedShardNames one each. Nothing outside the package sees the changed semantics.
• ./pkg/webhook/... passes at head (ok pkg/webhook 3.7s, ok pkg/webhook/templates 1.2s), matching green CI, and the PR body carries the rendered collapsed preview as AGENTS.md line 51 requires.

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

[aparajon]

🤖 All 5 findings addressed; 1–3 and 5 land in d9f8367, 4 is sequenced for the apply-side stack. Per finding:

1. Gate lines state coverage — the unsafe/blocked/direct change data now carries TotalShards (the keyspace's planned-shard count, plumbed from the webhook builders), and past the inline limit planShardList renders 12 of 32 shards — or all 32 shards when the change spans the keyspace — via the same shardCoveragePhrase the DDL headings use, so the two surfaces can never disagree on what "all" means. With no known total it falls back to the bare count rather than overstating coverage. Covered by TestRenderPlanComment_UnsafeChangeWideShardsStatesCoverage (subset + whole-keyspace), a planShardList table incl. the fallback, and a builder assertion on TotalShards; mutation-checked (count == totalShards → true and an off-by-one in the builder both fail their tests). PR body updated with the rendered line.

2. Doc claim fixed — the comment no longer points at a CLI fallback that doesn't render shard names; it now says the wide form states coverage and the full names stay reachable in the DDL section's collapsed shard-group blocks, which finding 1 makes accurate.

3. Boundary pinned — TestWriteShardGroupHeading_InlineLimitBoundary renders at shardNamesInlineLimit (inline, every name, no <details>) and shardNamesInlineLimit+1 (collapsed coverage line); flipping the <= to < fails it (mutation-verified).

4. Apply-side twin — agreed, and deliberately not in this PR: both ingredients (shardNamesInlineLimit and the coverage-aware planShardList) are introduced here, so the stack's base doesn't have them until it rebases onto this merge. shardList's cap, markdownInlineCode normalization, and the double-walling against writeShardStatusTable are queued for the fix(github): give multi-keyspace Strata applies the keyspace-grouped shard layout #1189–feat(github): render live row-copy progress on sharded table lines #1191 rebase (the raw quoting at sharded_apply.go:341 included).

5. Helper extracted — markdownInlineCodeList now lives next to markdownInlineCode, and all three copies (quotedShardNames — deleted, renderEnvironmentLine, RenderInvalidEnv) route through it.

Verification: build + vet clean, go test -race ./pkg/webhook/... green, gofmt clean, TEMPLATES.md regenerated byte-identical (no preview exercises the compact gate line; the wide-heading path is unchanged output).

This reply was generated by Claude Code (Claude Fable 5).