feat: improve PR walkthrough impact summaries (#3706)

2026-09-15 23:14:07 +01:00
parent 71dbffec0c
commit dcca12b964
9 changed files with 566 additions and 25 deletions
@@ -1,11 +1,16 @@
 ---
 name: pr-walkthrough
-description: Generate a single-file HTML walkthrough for a pull request. Use when the user wants a visual, scrollable page that explains a PR's problem, approach, architecture, key code changes, data flow, and trade-offs for reviewers. Trigger on "walkthrough for this PR", "explain this PR visually", "generate a PR walkthrough", or a PR URL with a request to explain it. Not for code review: do not load this skill to critique a PR, post review findings, or approve or request changes. Use a code-review skill for that.
+description: >-
+  Generate a single-file HTML walkthrough that explains a PR's purpose, user impact,
+  interface changes, compatibility risks, and implementation. Use for requests to
+  generate a PR walkthrough or explain a PR visually. Not for code review,
+  review findings, or approval verdicts.
 ---

 # PR Walkthrough

-Generate one HTML file that orients a reviewer to a pull request. The page is a vertical, center-contained "story": the reviewer scrolls from the problem, through the architecture and the key code changes, to the data model and the trade-offs.
+Generate one HTML file that explains why a PR exists and what changes for its users.
+Lead with the problem, outcome, and compatibility impact. Follow with architecture, code, and trade-offs.

 This skill is **not** a code-review skill. Do not produce review findings, approve/request-changes verdicts, or a full critique. Explain the change so a reviewer understands it fast.

@@ -31,6 +36,7 @@ The renderer is `references/build.py` (Python standard library only). It reads y
 The renderer does not judge whether the content is true, clear, or useful. That is your job, and it is the whole job. A page that passes the build but has a vague `tldr`, a wrong `sig`, or a diagram that does not match the code is a failed walkthrough. Spend your effort on the quality of each JSON section, not on the mechanics the renderer already handles. Each section below states the bar it must meet.

 See `references/example.json` for a complete, working data file. Copy its shape.
+Read [impact.md](references/impact.md) before collecting evidence or writing the impact sections.

 ### Managed CI filesystem mode

@@ -102,25 +108,37 @@ Do not build the page from the diff alone. Read the full current version of each

 Read each file once and keep it in mind. Do not re-open the same file to copy one more excerpt; scroll back to what you already read. Copy code excerpts straight from the diff and the first read, not from a second `view` of the same file.

-Scale the page to the PR size. A small PR gets a short page with 2-3 code changes and one diagram. A large PR gets more sections, but never filler. If two blocks teach the same fact, merge them.
+Scale the page to the PR size. A small PR can use one code change and no diagram.
+Use short bullets and tables. If two blocks teach the same fact, merge them.
+Inspect all five impact categories from `references/impact.md`, including callers and registrations.
+Distinguish confirmed absence from missing evidence. Do not infer user impact from filenames alone.

 ### 3. Plan the sections

-The JSON drives these sections. Keep only the ones that add value. The renderer draws a section when its key is present, and skips `architecture` and `data` when you omit them. The keys map to sections like this:
+The page order is: header, why, impact, optional data diagram, architecture, code, risk, and review notes.
+New walkthroughs must include `impact` with all five categories. The renderer accepts older files without it for compatibility.
+Unchanged categories occupy one summary line each. Only changed categories get detail tables.
+The keys map to sections like this:

 1. **Header / TL;DR** (`pr`) - the PR title, URL, base and head, file and line counts, and a one line `tldr`. The renderer builds the `<h1>`, the badges, and the topbar. The topbar **Review split button** opens the GitHub review pane and copies `gh pr review <number> --repo <repo> --approve`. It never approves on its own; the page holds no credentials. Set `pr.repo` to the `owner/repo` slug (for example `example-org/parcel-service`) so the copied command is correct.
   - Quality bar: `tldr` states, in one sentence, what the PR changes and why. A reader who reads only this line knows the point of the PR. Do not restate the title. Do not use vague words such as "improve" or "update" without the concrete change.
-2. **Why and what** (`why`) - `why.problem` is one or two sentences. `why.what` is a bullet list of what the change does.
-   - Quality bar: `why.problem` names the concrete problem the PR fixes, not the solution. `why.what` lists the real changes a reviewer will see in the diff, one per bullet, with no filler and no repeat of `tldr`.
-3. **Architecture, end to end** (`architecture`) - one high-level Mermaid `flowchart` in `architecture.mermaid`, with a short `architecture.caption`. Omit the key for a PR that needs no diagram. Choose the flow direction from the first token: use `flowchart LR` (left to right) for a linear pipeline so it fills the full-width container and stays short, and `flowchart TD` (top to bottom) when the flow branches enough that `LR` would grow too wide. The renderer passes the direction through unchanged; it is your choice, not a fixed default.
-   - Quality bar: the diagram shows the real components and the real flow the PR touches, with names that match the code. It is not a generic box diagram. Omit the section rather than draw a diagram that does not match the change.
-4. **Key code changes** (`changes`, `edges`) - the code canvas plus a linear fallback list. See "Changes" below. Use 2-6 changes.
-   - Quality bar: each change points at a real file and shows real code from the head commit. Each `why` says what the code does, not that it "was added". Each `sig` is the true signature. Each `edge` is a real call or data flow. A reviewer can trust the canvas as a map of the change. Show changed code as a diff so the reviewer sees what moved: use `patch` for a real hunk, or `diff: true` for an excerpt that is entirely new in this PR (it renders green). Use a plain `code` block only for context that the PR does not change.
-5. **Data and storage** (`data`) - a Mermaid diagram in `data.mermaid`, or a `data.fields` table of `field`, `type`, and `note`. Keep only what the PR touches. Omit the key when the PR touches no data.
+2. **Why this PR exists** (`why`) - name the affected user in `audience`, the concrete failure or limitation in `problem`, and the benefit in `outcome`.
+   - State the trigger and previous consequence. For new capabilities, explain the workflow that users cannot complete before this PR.
+   - Use one short sentence per field and 1-3 `what` bullets for the mechanism. Do not repeat the impact tables or enumerate test files.
+   - Derive the explanation from code, tests, and linked requirements. Label inferred motivation when the PR does not establish it.
+3. **Impact at a glance** (`impact`) - breaking changes, visible UX, plugin interfaces, MCP tools by context, and database changes.
+   - Read [impact.md](references/impact.md) for the schema, evidence checklist, count rules, and examples.
+   - Put compatibility alerts first. Describe actual before/after behavior, including new errors that replace automatic fallback.
+   - Include UI changes only when users see or do something different. A component refactor alone is not a UX change.
+4. **Data and storage** (`data`) - an optional Mermaid diagram in `data.mermaid`, or a non-database `data.fields` table of `field`, `type`, and `note`. Database migrations belong in `impact.database`. Do not duplicate that table here.
   - Quality bar: every field or entity is one the PR adds or changes. Types match the code. Omit the section for a PR that touches no data model.
-6. **Risk** (`risk`) - a score from 1 to 10 (10 = highest risk) in `risk.score`, and short bullets in `risk.reasons`. See "Risk score" below.
+5. **Architecture, end to end** (`architecture`) - one high-level Mermaid `flowchart` in `architecture.mermaid`, with a short `architecture.caption`. Omit the key for a PR that needs no diagram. Choose the flow direction from the first token: use `flowchart LR` (left to right) for a linear pipeline so it fills the full-width container and stays short, and `flowchart TD` (top to bottom) when the flow branches enough that `LR` would grow too wide. The renderer passes the direction through unchanged; it is your choice, not a fixed default.
+   - Quality bar: the diagram shows the real components and the real flow the PR touches, with names that match the code. It is not a generic box diagram. Omit the section rather than draw a diagram that does not match the change.
+6. **Key code changes** (`changes`, `edges`) - the code canvas plus a linear fallback list. See "Changes" below. Use 2-6 changes.
+   - Quality bar: each change points at a real file and shows real code from the head commit. Each `why` says what the code does, not that it "was added". Each `sig` is the true signature. Each `edge` is a real call or data flow. A reviewer can trust the canvas as a map of the change. Show changed code as a diff so the reviewer sees what moved: use `patch` for a real hunk, or `diff: true` for an excerpt that is entirely new in this PR (it renders green). Use a plain `code` block only for context that the PR does not change.
+7. **Risk** (`risk`) - a score from 1 to 10 (10 = highest risk) in `risk.score`, and short bullets in `risk.reasons`. See "Risk score" below.
   - Quality bar: the score follows from the reasons, and each reason is a real signal from this PR (blast radius, test coverage, rollback cost, data or contract change). Do not give a default middle score with generic reasons.
-7. **Trade-offs and review notes** (`review`) - `review.tradeoffs` is a bullet list. `review.focus` is an ordered "where to look first" list.
+8. **Trade-offs and review notes** (`review`) - `review.tradeoffs` is a bullet list. `review.focus` is an ordered "where to look first" list.
   - Quality bar: `review.tradeoffs` names real choices the PR makes and what it gives up. `review.focus` orders the files or areas a reviewer should read first, most important first. Do not fill it with "check the tests" boilerplate.

 For a section the PR needs but the schema does not cover (for example a state-machine diagram or a config table), tell the user which section you cannot express and ask how to proceed. Do not edit `shell.html` to add it.
@@ -444,9 +444,15 @@ def sec_tldr(pr):

 def sec_why(why):
    items = "".join(f'<li>{esc(w)}</li>' for w in why["what"])
+    context = "".join(
+        f'<p class="mb-3"><strong>{label}:</strong> {esc(why[key])}</p>'
+        for key, label in (("audience", "Who is affected"), ("outcome", "Result"))
+        if why.get(key)
+    )
    return f'''<section id="why" class="anchor-target mb-14">
-      <h2 class="text-xl font-semibold mb-3">Why this change</h2>
+      <h2 class="text-xl font-semibold mb-3">Why this PR exists</h2>
      <p class="mb-4">{esc(why["problem"])}</p>
+      {context}
      <h3 class="text-sm font-semibold uppercase tracking-wide text-slate-500 dark:text-slate-400 mb-2">What it does</h3>
      <ul class="list-disc pl-5 space-y-1.5 marker:text-brand">{items}</ul>
    </section>'''
@@ -562,14 +568,132 @@ def sec_review(review):
    </section>'''


+IMPACT_COLUMNS = {
+    "breaking": ("Breaking changes", (("audience", "Affected users / trigger"),
+        ("before", "Before"), ("after", "After"), ("action", "Required action"))),
+    "ux": ("User experience", (("surface", "Entry point"),
+        ("before", "Before"), ("after", "After"))),
+    "plugins": ("Kandev plugin interfaces", (("name", "API / interface"),
+        ("change", "Change"), ("contract", "Before / after contract"),
+        ("compatibility", "Compatibility / action"))),
+    "mcp": ("MCP tools", (("name", "Tool"), ("context", "Context / audience"),
+        ("change", "Change"), ("contract", "Before / after contract"),
+        ("compatibility", "Compatibility / action"))),
+    "database": ("Database changes", (("migration", "Migration / database"),
+        ("object", "Table / column / index"), ("change", "Before / after schema"),
+        ("upgrade", "Existing data / upgrade / rollback"))),
+}
+IMPACT_STATUS = {"changed": "Changed", "none": "No changes found", "unknown": "Not verified"}
+
+
+def is_safe_repo_path(value):
+    """Return true for a repository-relative POSIX path.
+
+    Impact source links must identify a file in the PR. Reject path syntax that
+    can never name a repository file before the managed manifest check.
+    """
+    if not isinstance(value, str) or not value.strip():
+        return False
+    if value.startswith(("/", ":")) or "\\" in value or "\x00" in value:
+        return False
+    parts = value.split("/")
+    return all(part not in {"", ".", ".."} for part in parts)
+
+
+def validate_impact(impact, changed_paths=None):
+    require(isinstance(impact, dict), "impact must be an object")
+    for key, (_, columns) in IMPACT_COLUMNS.items():
+        ctx = f"impact.{key}"
+        section = impact.get(key)
+        require(isinstance(section, dict), f"{ctx} must be an object")
+        status = section.get("status")
+        require(isinstance(status, str) and status in IMPACT_STATUS, f"{ctx}.status is invalid")
+        items = section.get("items")
+        require(isinstance(items, list), f"{ctx}.items must be a list")
+        require(bool(items) == (status == "changed"), f"{ctx}.items must match status")
+        if status == "unknown" or "note" in section:
+            require(isinstance(section.get("note"), str) and section["note"].strip(),
+                    f"{ctx}.note must be a non-empty string")
+        identities = set()
+        for i, item in enumerate(items):
+            row = f"{ctx}.items[{i}]"
+            require(isinstance(item, dict), f"{row} must be an object")
+            for field in [c[0] for c in columns] + ["file"]:
+                require(isinstance(item.get(field), str) and item[field].strip(),
+                        f"{row}.{field} must be a non-empty string")
+            require(is_safe_repo_path(item["file"]),
+                    f"{row}.file must be a repository-relative POSIX path")
+            if changed_paths is not None:
+                require(item["file"] in changed_paths,
+                        f"{row}.file must identify a changed file in the prepared manifest")
+            if key in ("plugins", "mcp"):
+                require(item["change"] in ("added", "changed", "removed"),
+                        f"{row}.change must be added, changed, or removed")
+            if key == "mcp":
+                identity = (item["context"], item["name"])
+                require(identity not in identities, f"{row} repeats a tool in the same context")
+                identities.add(identity)
+
+
+def impact_table(columns, items, pr):
+    headings = "".join(f'<th scope="col">{esc(label)}</th>' for _, label in columns)
+    rows = []
+    for item in items:
+        cells = "".join(f'<td data-label="{esc_attr(label)}">{esc(item[key])}</td>'
+                        for key, label in columns)
+        # Evidence always uses the trusted PR diff, never a supplied URL override.
+        url = file_url({"file": item["file"]}, pr)
+        rows.append(f'<tr>{cells}<td data-label="Source"><a href="{esc_attr(url)}">'
+                    f'{esc(item["file"])}</a></td></tr>')
+    return ('<table class="impact-table"><thead><tr>' + headings +
+            '<th scope="col">Source</th></tr></thead><tbody>' + "".join(rows) + '</tbody></table>')
+
+
+def mcp_counts(items):
+    contexts = {}
+    for item in items:
+        counts = contexts.setdefault(item["context"], dict.fromkeys(("added", "changed", "removed"), 0))
+        counts[item["change"]] += 1
+    return '<ul class="impact-counts">' + "".join(
+        f'<li>{esc(context)}: +{c["added"]} added, {c["changed"]} changed, '
+        f'-{c["removed"]} removed; net {c["added"] - c["removed"]:+d}</li>'
+        for context, c in contexts.items()
+    ) + '</ul>'
+
+
+def sec_impact(impact, pr):
+    summary, details = [], []
+    for key, (title, columns) in IMPACT_COLUMNS.items():
+        section = impact[key]
+        status = section["status"]
+        label = IMPACT_STATUS[status]
+        if key == "breaking" and status == "changed":
+            label = "Breaking changes detected"
+        summary.append(f'<li><strong>{esc(title)}:</strong> {label}'
+                       + (f' ({len(section["items"])})' if status == "changed" else '')
+                       + (f' <span>{esc(section["note"])}</span>' if section.get("note") else '') + '</li>')
+        if status != "changed":
+            continue
+        counts = mcp_counts(section["items"]) if key == "mcp" else ""
+        details.append(f'<section id="impact-{key}" class="impact-detail anchor-target">'
+                       f'<h3>{title}</h3>{counts}'
+                       f'{impact_table(columns, section["items"], pr)}</section>')
+    alert = ' impact-alert' if impact["breaking"]["status"] == "changed" else ''
+    return (f'<section id="impact" class="anchor-target mb-14{alert}">'
+            '<h2 class="text-xl font-semibold mb-3">Impact at a glance</h2>'
+            '<ul class="impact-summary">' + "".join(summary) + '</ul>' + "".join(details) + '</section>')
+
+
 def render_content(data):
    pr = data["pr"]
    parts = [sec_tldr(pr), sec_why(data["why"])]
+    if "impact" in data:
+        parts.append(sec_impact(data["impact"], pr))
+    if data.get("data"):
+        parts.append(sec_data(data["data"]))
    if data.get("architecture"):
        parts.append(sec_arch(data["architecture"]))
    parts.append(sec_changes(data["changes"], data.get("edges", []), pr))
-    if data.get("data"):
-        parts.append(sec_data(data["data"]))
    parts.append(sec_risk(data["risk"]))
    parts.append(sec_review(data["review"]))
    return "\n\n    ".join(parts)
@@ -589,13 +713,20 @@ def require_str_list(value, ctx):
                f"{ctx}[{k}] must be a non-empty string")


-def validate(data):
+def validate(data, *, require_impact=False, changed_paths=None):
    require("pr" in data, 'missing "pr"')
    pr = data["pr"]
    for k in ("number", "title", "url", "base", "head", "repo"):
        require(pr.get(k) not in (None, ""), f'pr.{k} is required')
    require(data.get("why", {}).get("problem"), "why.problem is required")
    require_str_list(data.get("why", {}).get("what"), "why.what")
+    for key in ("audience", "outcome"):
+        if require_impact or key in data["why"]:
+            require(isinstance(data["why"].get(key), str) and data["why"][key].strip(),
+                    f"why.{key} is required and must be a non-empty string")
+    if require_impact or "impact" in data:
+        require("impact" in data, 'impact is required for new walkthroughs')
+        validate_impact(data["impact"], changed_paths=changed_paths)
    changes = data.get("changes") or []
    require(len(changes) >= 1, "changes needs at least one entry")
    for i, c in enumerate(changes):
@@ -616,8 +747,8 @@ def validate(data):
    require("review" in data, 'missing "review"')


-def build(data):
-    validate(data)
+def build(data, *, require_impact=False, changed_paths=None):
+    validate(data, require_impact=require_impact, changed_paths=changed_paths)
    pr = data["pr"]
    content = render_content(data)
    shell = SHELL.read_text(encoding="utf-8")
@@ -12,7 +12,9 @@
    "tldr": "Webhook delivery now retries transient failures with bounded backoff. Permanent failures stop immediately, which protects downstream services from repeated invalid requests."
  },
  "why": {
+    "audience": "Operators who depend on webhook delivery during brief endpoint outages.",
    "problem": "The delivery worker treats every send failure the same. A short network interruption loses a webhook, while repeated retries of a rejected payload waste worker capacity.",
+    "outcome": "Transient failures receive up to three attempts. Permanent failures stop immediately.",
    "what": [
      "Classify transient and permanent send failures.",
      "Retry transient failures with three attempts and exponential backoff.",
@@ -20,6 +22,26 @@
      "Add tests for recovery, permanent failure, and exhausted retries."
    ]
  },
+  "impact": {
+    "breaking": {
+      "status": "changed",
+      "items": [{
+        "audience": "Endpoint owners who return permanent error responses for temporary failures",
+        "before": "The worker retries rejected payloads.",
+        "after": "The worker stops immediately for permanent errors.",
+        "action": "Return 429 or a 5xx response for transient failures.",
+        "file": "src/webhooks/webhook-sender.ts"
+      }]
+    },
+    "ux": { "status": "none", "items": [] },
+    "plugins": { "status": "none", "items": [] },
+    "mcp": { "status": "none", "items": [] },
+    "database": {
+      "status": "unknown",
+      "items": [],
+      "note": "This illustrative excerpt does not include persistence code or migrations. Database changes are not verified."
+    }
+  },
  "architecture": {
    "caption": "The worker checks the retry policy before each send. Exhausted jobs move to the dead-letter queue.",
    "mermaid": "flowchart LR\n  Worker[Delivery worker] --> Policy[Retry policy]\n  Policy --> Sender[Webhook sender]\n  Sender --> Endpoint[Remote endpoint]\n  Policy -- exhausted --> DeadLetter[(Dead-letter queue)]"
@@ -78,7 +100,7 @@
    { "from": "n3", "to": "n1", "label": "returns outcome" }
  ],
  "data": {
-    "caption": "Each delivery record stores retry state so a resumed worker can continue from the last attempt.",
+    "caption": "The delivery job carries retry state in memory. This excerpt does not establish database persistence.",
    "fields": [
      { "field": "attempts", "type": "integer", "note": "number of sends already made" },
      { "field": "status", "type": "enum", "note": "pending, delivered, or failed" },
@@ -0,0 +1,123 @@
+# Impact authoring
+
+Read this reference before inspecting a PR and writing its walkthrough JSON.
+All content describes the comparison base and PR head, not a guessed release history.
+
+## Concise content
+
+- Lead with who encounters the problem, the trigger, and the previous consequence.
+- Explain the outcome in user terms. Avoid claims such as "improves reliability" without a concrete result.
+- Keep table cells to one short sentence or a signature plus a short explanation.
+- Use one row per distinct UX outcome, API, tool in a context, or database object.
+- Group repeated mechanical schema edits only when their constraints and upgrade effects match.
+- Do not cap interface or migration inventories at the code canvas limit. Account for every changed public contract.
+- Use diagrams only to explain relationships that the tables cannot show clearly.
+
+## JSON shape
+
+Add `why.audience` and `why.outcome` beside the existing `problem` and `what` fields.
+Add an `impact` object with these five keys: `breaking`, `ux`, `plugins`, `mcp`, and `database`.
+Each category has `status`, `items`, and an optional `note`:
+
+| Status | Items | Meaning |
+| --- | --- | --- |
+| `changed` | At least one row | Evidence establishes a change. |
+| `none` | Empty array | Relevant code and contracts show no change. |
+| `unknown` | Empty array | Evidence is unavailable. A `note` must explain the limit. |
+
+If known changes coexist with evidence gaps, use `changed` and explain the remaining uncertainty in `note`.
+Never use `none` merely because the PR description omits the topic.
+All row fields in the next table are required, non-empty strings.
+Every row also needs `file`, a repository-relative evidence path. The renderer links it to the PR diff.
+Use a changed file that establishes the behavior. Mention supporting unchanged contracts in the cell when necessary.
+
+| Category | Row fields | Evidence and content |
+| --- | --- | --- |
+| `breaking` | `audience`, `before`, `after`, `action` | Identify affected users and the trigger. State previous behavior, new failure or incompatibility, and migration action. |
+| `ux` | `surface`, `before`, `after` | Name the entry point, such as Settings or Create task. Explain visible controls, navigation, defaults, messages, and outcomes. Include phone differences when present. |
+| `plugins` | `name`, `change`, `contract`, `compatibility` | List exact public API names. Identify Host UI, frontend types, SDK, wire protocol, manifest, or package contract. Show changed signatures or behavior and required plugin updates. |
+| `mcp` | `name`, `context`, `change`, `contract`, `compatibility` | Use canonical tool names. State inputs, outputs, permissions, defaults, or behavior that changes. Identify caller access and registration scope. |
+| `database` | `migration`, `object`, `change`, `upgrade` | Name the migration ID/path and database. Identify tables, columns, indexes, types, nullability, defaults, constraints, or removals. Explain backfills, existing rows, and rollback limits. |
+
+For plugins and MCP, `change` is `added`, `changed`, or `removed`.
+Use `compatibility` to state compatible, breaking, or unverified, followed by the reason and caller action.
+Repeat breaking contract changes in `breaking` as concise user consequences. Do not hide them in technical tables.
+For schema changes without a migration, explicitly state "No migration" and explain the effect on existing installations.
+Do not claim safe rollback, data preservation, or compatibility without evidence.
+
+## Inspection checklist
+
+### Visible UX
+
+Trace rendered controls and their routes, dialogs, handlers, and error states.
+A renamed React component or moved hook with identical behavior is `none`.
+A new settings page, task-dialog option, validation error, or changed default is a UX change.
+Describe the experience, not component names or CSS internals.
+
+### Kandev plugin interfaces
+
+Inspect public exports and their consumers, not only files with "plugin" in the name.
+Relevant boundaries include `apps/web/lib/plugins/types.ts`, `host-api.ts`, and the plugin API guide.
+Backend boundaries include `apps/backend/pkg/pluginsdk`, the plugin proto, and manifest/package validation.
+An internal host refactor without a public contract change is `none`.
+Additions can break exhaustive consumers or version negotiation. Assess the actual contract.
+
+### MCP tools and growth
+
+Trace registration and availability gates as well as handlers and schemas.
+Identify external MCP, task MCP, Office task MCP, configuration MCP, or another actual server context.
+Include role restrictions and flags in the context or contract. Do not assume every tool reaches every agent.
+Use a consistent context label for all tools on the same surface.
+
+The renderer derives added, changed, removed, and net counts for each context from the rows.
+These are counts of exposed tool names, not handlers or schema fields.
+A renamed tool is one removal and one addition. A tool moved between contexts follows the same rule.
+A tool exposed in two contexts needs two rows. Do not count either row twice in its context.
+A schema-only edit is `changed`, with zero net growth.
+Explain why a new tool exists and whether it extends or replaces an existing tool in `contract`.
+If the PR does not establish that rationale, say it is not established. Do not invent a justification.
+
+### Database changes
+
+Inspect migration registration, SQL, repository initialization, and upgrade tests.
+Separate persisted database changes from in-memory structs and API payload fields.
+Record destructive operations, new required columns, defaults for existing rows, and migration order when relevant.
+
+### Breaking behavior
+
+Compare successful base behavior with head behavior for existing users and callers.
+Include stricter validation, removed fallback, changed permissions, incompatible defaults, and new required configuration.
+Do not limit this section to removed APIs or major version changes.
+
+Illustrative row, only when the inspected PR proves this change:
+
+```json
+{
+  "audience": "Users whose executor profile model differs from Settings",
+  "before": "Task startup selects a fallback model automatically.",
+  "after": "Task startup stops with a model mismatch error.",
+  "action": "Select a matching model before starting the task.",
+  "file": "path/to/changed/model-validation.go"
+}
+```
+
+This example is not evidence about the current PR.
+If compatibility cannot be established, report the uncertainty instead of inventing a breaking change or declaring compatibility.
+
+## Validation
+
+Verify that impact appears before architecture and code.
+Check that unchanged categories have no empty detail tables.
+Read the page at desktop and phone widths. Phone tables must show labeled cards with all fields and source links.
+Verify breaking alerts in both themes and MCP counts against the registration diff.
+
+After renderer or shell changes, run these commands from the repository root:
+
+```bash
+python3 -m unittest discover -s .agents/skills/pr-walkthrough/references -p test_build.py
+python3 .agents/skills/pr-walkthrough/scripts/pr-walkthrough-render.test.py
+node .agents/skills/pr-walkthrough/references/test_browser.cjs
+```
+
+The browser check uses workspace Playwright and an installed Chromium browser.
+It renders synthetic impact rows and verifies phone cards, desktop tables, both themes, source links, and section navigation.
@@ -248,6 +248,26 @@
    #canvas-wrap.canvas-full-bleed { width: 100vw; margin-left: 50%; transform: translateX(-50vw); }
    /* 100vw counts the scrollbar, so clip the tiny horizontal overflow it adds. */
    body { overflow-x: hidden; }
+    .impact-summary, .impact-counts { padding-left: 1.25rem; list-style: disc; }
+    .impact-summary li, .impact-counts li { margin-bottom: .4rem; }
+    .impact-detail { margin-top: 1.75rem; }
+    .impact-detail h3 { font-size: 1.1rem; font-weight: 650; margin-bottom: .75rem; }
+    .impact-alert #impact-breaking { border-left: 3px solid #e11d48; padding-left: 1rem; }
+    .impact-table { width: 100%; table-layout: fixed; font-size: .875rem; border-collapse: collapse; }
+    .impact-table th, .impact-table td {
+      padding: .65rem; text-align: left; vertical-align: top;
+      border-bottom: 1px solid var(--wt-border); overflow-wrap: anywhere;
+    }
+    .impact-table th { font-weight: 650; }
+    .impact-table a { text-decoration: underline; }
+    @media (max-width: 767px) {
+      .impact-table, .impact-table tbody { display: block; }
+      .impact-table thead { position: absolute; width: 1px; height: 1px; overflow: hidden; clip-path: inset(50%); }
+      .impact-table tr { display: block; border: 1px solid var(--wt-border); border-radius: .75rem; margin-bottom: .75rem; padding: .5rem; }
+      .impact-table td { display: block; border: 0; padding: .4rem; }
+      .impact-table td::before { content: attr(data-label); display: block; font-weight: 650; margin-bottom: .15rem; }
+      .impact-table a { display: inline-flex; align-items: center; min-height: 44px; }
+    }
    /* Suppress text selection so panning does not highlight node text. */
    #canvas-wrap { cursor: grab; user-select: none; -webkit-user-select: none; }
    #canvas-wrap.panning { cursor: grabbing; }
@@ -409,21 +429,23 @@
        <span class="wt-brand-divider hidden sm:inline" aria-hidden="true"></span>
        <span class="wt-brand-pr">PR #{{PR_NUMBER}}</span>
      </a>
-      <nav class="hidden md:flex items-center gap-4 text-sm text-slate-500 dark:text-slate-400">
+      <nav class="hidden lg:flex items-center gap-4 text-sm text-slate-500 dark:text-slate-400">
        <a href="#why" class="hover:text-brand">Why</a>
+        <a id="nav-impact" href="#impact" class="hover:text-brand">Impact</a>
+        <a id="nav-data" href="#data" class="hover:text-brand">Data</a>
        <a id="nav-architecture" href="#architecture" class="hover:text-brand">Architecture</a>
        <a href="#changes" class="hover:text-brand">Changes</a>
-        <a id="nav-data" href="#data" class="hover:text-brand">Data</a>
        <a href="#risk" class="hover:text-brand">Risk</a>
        <a href="#review" class="hover:text-brand">Review</a>
      </nav>
-      <details id="mobile-nav" class="wt-mobile-nav md:hidden">
+      <details id="mobile-nav" class="wt-mobile-nav lg:hidden">
        <summary aria-label="Jump to walkthrough section">Sections</summary>
        <nav class="wt-mobile-menu" aria-label="Walkthrough sections">
          <a href="#why">Why</a>
+          <a id="mobile-nav-impact" href="#impact">Impact</a>
+          <a id="mobile-nav-data" href="#data">Data</a>
          <a id="mobile-nav-architecture" href="#architecture">Architecture</a>
          <a href="#changes">Changes</a>
-          <a id="mobile-nav-data" href="#data">Data</a>
          <a href="#risk">Risk</a>
          <a href="#review">Review</a>
        </nav>
@@ -484,7 +506,7 @@
    // build.py drops the Architecture and Data sections on a PR that has no
    // such data, so their topbar anchors would scroll nowhere. Hide each link
    // unless its section exists.
-    for (const id of ['architecture', 'data']) {
+    for (const id of ['architecture', 'data', 'impact']) {
      if (!document.getElementById(id)) {
        const link = document.getElementById('nav-' + id);
        if (link) link.hidden = true;
@@ -0,0 +1,80 @@
+// Run from the repository root: node .agents/skills/pr-walkthrough/references/test_browser.cjs
+// Uses the workspace Playwright installation and the standalone HTML renderer.
+const assert = require('node:assert/strict');
+const fs = require('node:fs');
+const os = require('node:os');
+const path = require('node:path');
+const { pathToFileURL } = require('node:url');
+const { createRequire } = require('node:module');
+const { execFileSync } = require('node:child_process');
+const workspaceRequire = createRequire(path.resolve(__dirname, '../../../../apps/web/package.json'));
+const { chromium } = workspaceRequire('@playwright/test');
+
+async function main() {
+  const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'walkthrough-browser-'));
+  const browser = await chromium.launch({ headless: true });
+  try {
+    const data = JSON.parse(fs.readFileSync(path.join(__dirname, 'example.json'), 'utf8'));
+    // Synthetic changes exercise every table without implying facts about a real PR.
+    const rows = {
+      ux: { surface: 'Settings > Models', before: 'Automatic fallback', after: 'Mismatch error' },
+      plugins: { name: 'HostUI.selectModel()', change: 'changed', contract: 'Requires a model ID', compatibility: 'Breaking: pass the configured ID' },
+      mcp: { name: 'select_model', context: 'Task MCP', change: 'added', contract: 'Selects a configured model', compatibility: 'Compatible addition' },
+      database: { migration: '042 / task database', object: 'tasks.model_id', change: 'Adds nullable TEXT column', upgrade: 'Existing rows retain NULL. Rollback removes the column.' },
+    };
+    for (const [key, row] of Object.entries(rows)) {
+      data.impact[key] = { status: 'changed', items: [{ ...row, file: 'src/model.ts' }] };
+    }
+    data.impact.breaking = { status: 'changed', items: [{
+      audience: 'Existing users', before: 'Automatic fallback', after: 'Mismatch error',
+      action: 'Select a matching model', file: 'src/model.ts',
+    }] };
+    const input = path.join(dir, 'test.json');
+    const output = path.join(dir, 'test.html');
+    fs.writeFileSync(input, JSON.stringify(data));
+    execFileSync('python3', [path.join(__dirname, 'build.py'), input, output]);
+    for (const width of [390, 767, 768, 1023, 1024, 1440]) {
+      const page = await browser.newPage({ viewport: { width, height: 900 } });
+      await page.goto(pathToFileURL(output).href);
+      await page.waitForFunction(() => getComputedStyle(document.querySelector('main')).maxWidth !== 'none');
+      for (const dark of [true, false]) {
+        await page.evaluate((value) => document.documentElement.classList.toggle('dark', value), dark);
+        assert.equal(await page.locator('#impact table').count(), 5);
+        assert.match(await page.locator('#impact').innerText(), /Breaking changes detected/);
+        assert.match(await page.locator('#impact-mcp').innerText(), /net \+1/);
+        assert.match(await page.locator('#impact-database').innerText(), /Existing rows retain NULL/);
+        const geometry = await page.locator('#impact').evaluate((el) => ({
+          fits: [...el.querySelectorAll('td')].every(cell => cell.getBoundingClientRect().right <= innerWidth),
+          rowDisplay: getComputedStyle(el.querySelector('tr')).display,
+          label: getComputedStyle(el.querySelector('td'), '::before').content,
+          overflow: document.documentElement.scrollWidth > innerWidth,
+        }));
+        assert.equal(geometry.fits, true);
+        assert.equal(geometry.overflow, false);
+        assert.equal(geometry.rowDisplay, width < 768 ? 'block' : 'table-row');
+        if (width < 768) {
+          assert.match(geometry.label, /Affected users/);
+          assert.ok((await page.locator('#impact-ux a').boundingBox()).height >= 44);
+        }
+      }
+      // The shipped menu opens below the shell's desktop navigation breakpoint.
+      const mobileLink = page.locator('#mobile-nav-impact');
+      const desktopLink = page.locator('#nav-impact');
+      if (await desktopLink.isVisible()) {
+        await desktopLink.click();
+      } else {
+        await page.locator('#mobile-nav summary').click();
+        await mobileLink.click();
+      }
+      assert.equal(new URL(page.url()).hash, '#impact');
+      assert.ok(await page.locator('#impact-ux a').getAttribute('href').then(url => url.includes('/files#diff-')));
+      await page.close();
+      console.log(`PASS ${width}px: impact content, themes, layout, source links, navigation`);
+    }
+  } finally {
+    await browser.close();
+    fs.rmSync(dir, { recursive: true, force: true });
+  }
+}
+
+main().catch(error => { console.error(error); process.exitCode = 1; });
@@ -41,6 +41,97 @@ def minimal_data():
    }


+class TestImpact(unittest.TestCase):
+    def data(self):
+        data = minimal_data()
+        data["impact"] = {
+            key: {"status": "none", "items": []}
+            for key in ("breaking", "ux", "plugins", "mcp", "database")
+        }
+        return data
+
+    def test_absent_is_not_reported_as_none(self):
+        self.assertNotIn('id="impact"', build.build(minimal_data()))
+        out = build.build(self.data())
+        self.assertIn("No changes found", out)
+        self.assertNotIn('id="impact-ux"', out)
+
+    def test_unknown_requires_reason_and_stays_distinct(self):
+        data = self.data()
+        data["impact"]["ux"] = {"status": "unknown", "items": []}
+        with self.assertRaises(build.BuildError):
+            build.build(data)
+        data["impact"]["ux"]["note"] = "UI files were unavailable."
+        self.assertIn("Not verified", build.build(data))
+
+    def test_new_contract_requires_why_and_impact(self):
+        with self.assertRaises(build.BuildError):
+            build.validate(minimal_data(), require_impact=True)
+        data = self.data()
+        data["why"] = {"problem": "p", "what": ["w"], "audience": "users", "outcome": "result"}
+        build.validate(data, require_impact=True, changed_paths={"a/b.go"})
+
+    def test_impact_source_paths_are_safe_and_in_inventory(self):
+        data = self.data()
+        data["impact"]["ux"] = {"status": "changed", "items": [{
+            "surface": "Settings", "before": "Old", "after": "New", "file": "a/b.go",
+        }]}
+        for value in ("/absolute.go", "../outside.go", "a/../b.go", "a\\b.go", "https://example.invalid/x"):
+            data["impact"]["ux"]["items"][0]["file"] = value
+            with self.subTest(value=value):
+                with self.assertRaises(build.BuildError):
+                    build.validate(data, changed_paths={"a/b.go"})
+        data["impact"]["ux"]["items"][0]["file"] = "a/other.go"
+        with self.assertRaises(build.BuildError):
+            build.validate(data, changed_paths={"a/b.go"})
+
+    def test_breaking_precedes_code_and_escapes_evidence(self):
+        data = self.data()
+        data["impact"]["breaking"] = {"status": "changed", "items": [{
+            "audience": "Existing users", "before": "Automatic fallback",
+            "after": "<script>alert(1)</script>", "action": "Select a matching model",
+            "file": "a/b.go",
+        }]}
+        out = build.build(data)
+        self.assertLess(out.index('id="impact-breaking"'), out.index('id="changes"'))
+        self.assertIn("&lt;script&gt;alert(1)&lt;/script&gt;", out)
+        self.assertIn("/files#diff-" + build.sha256_hex("a/b.go"), out)
+        self.assertIn("Breaking changes detected", out)
+
+    def test_mcp_counts_by_context_and_not_schema_edits(self):
+        data = self.data()
+        items = [{"name": name, "context": ctx, "change": change,
+                  "contract": "Input changes", "compatibility": "Compatible", "file": "tools.go"}
+                 for name, ctx, change in [("read", "task", "added"),
+                                           ("read", "office", "removed"),
+                                           ("write", "task", "changed")]]
+        data["impact"]["mcp"] = {"status": "changed", "items": items}
+        out = build.build(data)
+        self.assertIn("task: +1 added, 1 changed, -0 removed; net +1", out)
+        self.assertIn("office: +0 added, 0 changed, -1 removed; net -1", out)
+        items.append(dict(items[0]))
+        with self.assertRaises(build.BuildError):
+            build.build(data)
+
+    def test_rejects_incomplete_or_contradictory_categories(self):
+        for category in ("breaking", "ux", "plugins", "mcp", "database"):
+            for value in (None, [], {"status": "changed", "items": []},
+                          {"status": "none", "items": [{}]},
+                          {"status": "changed", "items": [{}]}):
+                with self.subTest(category=category, value=value):
+                    data = self.data()
+                    data["impact"][category] = value
+                    with self.assertRaises(build.BuildError):
+                        build.build(data)
+
+    def test_why_outcome_and_audience(self):
+        data = minimal_data()
+        data["why"].update({"audience": "Operators", "outcome": "Tasks start reliably"})
+        out = build.build(data)
+        self.assertIn("Operators", out)
+        self.assertIn("Tasks start reliably", out)
+
+
 class TestPatchToMarked(unittest.TestCase):
    def test_headers_dropped_context_kept_indices_recorded(self):
        patch = ("diff --git a/f b/f\nindex 1..2 100644\n--- a/f\n+++ b/f\n"
@@ -13,6 +13,7 @@ from pathlib import Path
 SKILL_DIR = Path(__file__).resolve().parents[1]
 TRUSTED_DIR = Path(".pr-walkthrough")
 DRAFT_PATH = TRUSTED_DIR / "draft.json"
+MANIFEST_PATH = TRUSTED_DIR / "head-context" / "manifest.json"
 OUTPUT_DIR = Path("docs/pr-walkthrough")
 REPO_RE = re.compile(r"^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$")

@@ -72,6 +73,30 @@ def load_draft() -> object:
        raise ValueError(f"fixed draft is not valid JSON: {DRAFT_PATH}") from exc


+def validate_managed_draft(data: dict[str, object]) -> None:
+    """Apply the new walkthrough contract against the trusted file inventory."""
+    if not MANIFEST_PATH.is_file():
+        raise ValueError(f"prepared context manifest is missing: {MANIFEST_PATH}")
+    try:
+        manifest = json.loads(MANIFEST_PATH.read_text(encoding="utf-8"))
+    except json.JSONDecodeError as exc:
+        raise ValueError(f"prepared context manifest is not valid JSON: {MANIFEST_PATH}") from exc
+    files = manifest.get("files") if isinstance(manifest, dict) else None
+    if not isinstance(files, list):
+        raise ValueError("prepared context manifest has no file inventory")
+    changed_paths = {
+        entry.get("path") for entry in files
+        if isinstance(entry, dict) and isinstance(entry.get("path"), str)
+    }
+    sys.path.insert(0, str(SKILL_DIR / "references"))
+    import build as trusted_build  # noqa: PLC0415
+
+    try:
+        trusted_build.validate(data, require_impact=True, changed_paths=changed_paths)
+    except trusted_build.BuildError as exc:
+        raise ValueError(str(exc)) from exc
+
+
 def render(data: dict[str, object]) -> tuple[Path, Path]:
    build_script = SKILL_DIR / "references" / "build.py"
    shell = SKILL_DIR / "references" / "shell.html"
@@ -110,6 +135,7 @@ def main() -> int:
            raise ValueError("renderer does not accept command arguments")
        data = load_draft()
        bound = bind_trusted_metadata(data)
+        validate_managed_draft(bound)
        json_path, html_path = render(bound)
    except (OSError, ValueError) as exc:
        print(f"walkthrough render failed: {exc}", file=sys.stderr)
@@ -28,6 +28,18 @@ class PRWalkthroughRenderTest(unittest.TestCase):
        self.data = json.loads(
            (self.skill / "references" / "example.json").read_text(encoding="utf-8")
        )
+        manifest_paths = {
+            item["file"]
+            for category in self.data["impact"].values()
+            for item in category.get("items", [])
+        }
+        manifest_paths.update(change["file"] for change in self.data["changes"])
+        manifest = {
+            "files": [{"path": path, "status": "M"} for path in sorted(manifest_paths)]
+        }
+        manifest_dir = self.worktree / ".pr-walkthrough" / "head-context"
+        manifest_dir.mkdir(parents=True, exist_ok=True)
+        (manifest_dir / "manifest.json").write_text(json.dumps(manifest), encoding="utf-8")
        self.env = {
            **os.environ,
            "PR_NUMBER": "42",
@@ -131,6 +143,22 @@ class PRWalkthroughRenderTest(unittest.TestCase):
        self.assertFalse((output / "pr-42.html").exists())
        self.assertIn("why.problem is required", result.stderr)

+    def test_managed_renderer_requires_new_impact_contract(self) -> None:
+        data = json.loads(json.dumps(self.data))
+        del data["impact"]
+        result = self.run_render(data)
+        self.assertNotEqual(result.returncode, 0)
+        self.assertIn("impact is required for new walkthroughs", result.stderr)
+
+    def test_managed_renderer_requires_manifest_evidence_path(self) -> None:
+        data = json.loads(json.dumps(self.data))
+        data["impact"]["ux"] = {"status": "changed", "items": [{
+            "surface": "Settings", "before": "Old", "after": "New", "file": "not-changed.ts"
+        }]}
+        result = self.run_render(data)
+        self.assertNotEqual(result.returncode, 0)
+        self.assertIn("changed file in the prepared manifest", result.stderr)
+

 if __name__ == "__main__":
    unittest.main()