fix(github): sanitize engine error text rendered into PR comments

[Kiran01bm]

Summary

Engine and task error text was rendered raw into PR comment blockquotes. A multi-line or adversarial error could escape the blockquote and break the comment layout, leak connection endpoints (DSN fragments, host:port pairs, IP addresses) into a public PR, or flood the comment with unbounded output.

What

• Add a shared sanitizeCommentError seam in pkg/webhook/templates/common.go: normalizes CRLF/CR to LF, strips ANSI escape sequences and control characters, redacts connection endpoints, trims whitespace, and clamps by rune count with a truncation marker.
• Route both error writers (writeErrorBlock, writeTableErrorLine) through it, and quote continuation lines in both so multi-line errors stay inside the blockquote.
• taskErrorAddsDetail now compares the sanitized representation actually rendered, so the table-level error dedupe cannot be defeated by differences sanitization erases (e.g. two dial errors differing only in the redacted endpoint).
• Messages that sanitize to empty write nothing.

Why

Follows up the apply-comment UX stream: now that failed-table errors are surfaced in the comment, every rendered error must be safe for a public surface. Genuine engine errors (e.g. a Spirit preflight check reason) render intact; only infrastructure detail is redacted. The raw error remains in server logs with the apply identifiers, so triage loses nothing.

before:                             after:
┌─────────────┐                     ┌─────────────┐
│ raw error   │──▶ blockquote       │ raw error   │──▶ sanitize ──▶ blockquote
└─────────────┘    (escapes quote,  └─────────────┘    (redacted,     (contained)
                    leaks hosts)          │             clamped)
                                          └──▶ server logs (unchanged, full detail)

[Copilot]

Pull request overview

This PR hardens SchemaBot’s GitHub PR-comment rendering by sanitizing untrusted engine/task error text before it is emitted into Markdown blockquotes, preventing layout breakouts and reducing the risk of leaking sensitive connection endpoint details.

Changes:

• Added a shared sanitizeCommentError helper to normalize line endings, strip control characters / ANSI escapes, redact endpoint-like substrings, and clamp rendered error length.
• Routed writeErrorBlock, writeTableErrorLine, and taskErrorAddsDetail through the sanitized representation and ensured multi-line errors remain within blockquotes.
• Added focused unit tests covering sanitization behavior, clamping, and quoting.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
pkg/webhook/templates/common.go	Introduces error sanitization + blockquote containment and applies it to error rendering/deduping paths.
pkg/webhook/templates/common_test.go	Adds unit tests validating sanitization, redaction, clamping, and multi-line quoting behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head f2c20e2.

Verdict: the sanitization seam is correct and applied at the right boundary — approving, with a follow-up slice to cover the remaining surfaces.

Findings

Other comment surfaces still render engine/apply error text with only HTML escaping, outside the new seam. Verified at head: shardStatusCell in pkg/webhook/templates/sharded_apply.go renders s.Error into a table cell (a | or newline in the error breaks the shard table — exactly the failure mode this PR fixes for blockquotes), and writeShardFirstFailure (same file), writeAggregateFirstFailure (multi_apply.go), RenderRollbackNotAccepted (rollback.go), and plan's writeErrors all render error text without endpoint redaction or newline/pipe neutralization. The PR body's "every rendered error must be safe" is the right goal; these sites need the same seam.

The single-label host:port branch of the redaction regex can redact benign word:NN text (e.g. chunk:42). Safe direction — the raw error stays in logs — so noted only.

The sanitizer does not neutralize HTML or backticks. An error containing </details> could break the frozen-comment details wrapper; sibling renderers use html.EscapeString.

Action items

1. Follow-up slice: route the remaining error-rendering surfaces (shardStatusCell, writeShardFirstFailure, writeAggregateFirstFailure, RenderRollbackNotAccepted, plan writeErrors) through sanitizeCommentError.
2. (optional) HTML-escape (or strip </> and backticks) inside the sanitizer so a hostile error can't close a <details> wrapper.

Verified (tried to break, couldn't)

Sanitization is applied at the render boundary only — stored ErrorMessage/LastError stay raw and server logs keep the full error with the LogAttrs() triage identifiers, so operators can still diagnose from logs alone; taskErrorAddsDetail correctly dedupes on the rendered representation so redaction can't be used to defeat it; all four Copilot findings are addressed on head (single-label endpoint redaction, unicode.Cf bidi stripping, both regression tests present); tests assert specific sanitized outputs for CRLF/CR, ANSI + control characters, bidi/zero-width, DSN-with-password, dotted and single-label host:port, dual IPs, and the 500-rune clamp, plus false-positive guards (line 3:14, orders.line_items); no tests were deleted (common_test.go is entirely new); no main-side changes touch pkg/webhook/templates/ since base and a merge-tree check is clean; no internal details in title/body/diff; all 32 CI checks pass on head.

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

[aparajon]

🤖 Approving on Armand's behalf after the adversarial correctness review above (no blocking findings). This stamp was left by Claude Code (claude-fable-5).