fix: always show the raw row in PR meta Impact table#447
Merged
Conversation
Follow-up to #444. The `## Meta` Impact table previously dropped the `raw` row when it numerically equalled `true`. It now renders the `raw` row whenever excludes are configured (Excluding != nil) — even on a no-op exclude — so a reader always sees raw alongside true. When no excludes are configured, `true` is definitionally identical to raw, so no separate raw row is rendered (no redundant duplicate). Updates prmeta render + goldens, the _cli-fab and SPEC-git-pr descriptions, and the pipeline memory prose. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates the PR ## Meta Impact table rendering so the raw row is always shown whenever excludes are configured, even if the exclude pass is a no-op (so readers consistently see raw alongside true when excludes apply).
Changes:
prmeta.renderImpact: show therawrow based onImpact.Excluding != nil(exclude-pass present) instead ofraw != true.- Update/extend golden tests to cover “excludes configured but no-op” and keep “no excludes → no duplicate raw row”.
- Sweep related documentation/spec prose to match the updated raw-row rule.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/kit/skills/_cli-fab.md | Updates CLI contract prose for when the raw Impact row is rendered. |
| src/go/fab/internal/prmeta/prmeta.go | Changes raw row condition to Impact.Excluding != nil and updates taxonomy comments. |
| src/go/fab/internal/prmeta/prmeta_test.go | Adds/adjusts golden expectations for the new raw-row rule. |
| docs/specs/skills/SPEC-git-pr.md | Updates spec prose to reflect raw-row rendering when excludes are configured. |
| docs/memory/pipeline/schemas.md | Updates memory docs prose for the new raw-row rule. |
| docs/memory/pipeline/execution-skills.md | Updates execution-skill docs prose for the new raw-row rule. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| **PR type system**: `/git-pr` supports 7 PR types (feat, fix, refactor, docs, test, ci, chore) derived from Conventional Commits. Types are resolved via a four-step chain: explicit argument → read from `.status.yaml` → infer from fab change intake → infer from diff. The type controls the PR title prefix (`{type}: {title}`). The PR body has two layers: an agent-generated `## Summary` + optional `## Changes` (prose synthesized from `intake.md`), and a mechanically-rendered `## Meta` block. Title derivation uses intake heading when available, commit subject otherwise, regardless of type. | ||
|
|
||
| **Mechanical `## Meta` block via `fab pr-meta`** (rj31): As of rj31, `/git-pr` Step 3c no longer assembles the `## Meta` block from natural-language formatting prose. The entire block — the 5-column top table (`Change ID | Type | Confidence | Plan | Review`), the `**Impact**:` table + caption, the optional `**Issues**:` line, and the `**Pipeline:**` line (six fixed-order stages with ` ✓` per `done` stage and hyperlinked intake/apply labels) — is rendered deterministically by the `fab pr-meta <change> --type <type> [--issues "<space-joined IDs>"]` subcommand, whose stdout the skill pastes verbatim (omitting the block on non-zero exit / empty stdout, matching the legacy `{has_fab} = false` path). **Element order (260625-pnao layout revision):** heading → top table → Impact table + caption → optional Issues → **Pipeline (LAST)** — Pipeline moved from before Impact to the final element, with the optional Issues line sitting just above it. Each table/paragraph is blank-line separated so GitHub renders them distinctly. The top table's first header is **`Change ID`** (was `ID`); a present id is backtick-wrapped (`` `pnao` ``), the empty-fallback `—` stays bare. `**Pipeline:**` carries the colon **inside** the bold span. **Impact is one normalized, self-labeling table** (pnao, replacing the prior multi-form rendering): a single `Impact | +/− | Net` table — the compact `+/−` is the *column header* only, while the data rows carry spaced `+A / −B` figures; the first column header is `Impact` (was `Scope`), so the table needs **no separate `**Impact**:` lead-in line**. Numeric columns are right-aligned. It carries the fixed taxonomy `raw / true / impl / tests / excluded` where `raw = true + excluded` and `true = impl + tests`. `true` is ALWAYS the post-exclude diff (the bold row, always present) — fixing the prior "total flips meaning" bug where the same label denoted the raw diff in one form and the post-exclude diff in another. The table adapts by DROPPING rows, never reshaping: the `raw` row appears only when it differs from `true` (excludes engaged), and nested `└ impl` / `└ tests` rows appear only when a tests pair is present. Below the table a `<sub>` (small-text, NOT italic) provenance caption co-locates the excludes note and the fab-kit **binary** version stamp — `<sub>excludes `fab/`, `docs/` · generated by fab-kit vX.Y.Z</sub>` (excludes clause omitted when none configured; `vdev` rendered honestly on dev builds). Emphasis stays bold-only and the caption uses `<sub>` because GitHub's Markdown sanitizer strips `style`/`class` (so row backgrounds / colored numbers are impossible) but keeps `<sub>` on its HTML allowlist. Type resolution (Step 0b), issue gathering (Step 1), and the agent-generated Summary/Changes stay skill-side; the progress token remains `✓ body — meta + summary + changes`. `fab pr-meta` is self-contained — it reads `.status.yaml`, parses `plan.md` task checkboxes, reads config (`true_impact_exclude`, `test_paths`, `project.linear_workspace`), computes impact via `internal/impact`, and resolves git/`gh` context (branch, owner/repo, merge-base) itself; the skill passes only `<change>`, `--type`, and optional `--issues`. This moves the Meta block's determinism into Go (tested via golden-output tests), eliminating per-run drift (e.g., the dropped exclude-list backticks the prose previously produced). `gh` failure degrades to plain-text Pipeline labels; a missing merge-base drops only the Impact block — never a hard `/git-pr` failure. `/git-pr` no longer calls `fab impact` directly (the subcommand remains public for other consumers). Blob URLs use `https://github.com/{owner}/{repo}/blob/{branch}/...` to resolve against the feature branch instead of main. See [schemas.md](/pipeline/schemas.md) for the `true_impact` render-time `impl` residual and [the `fab pr-meta` CLI reference](../../../src/kit/skills/_cli-fab.md) for the full signature, output contract, and exit codes. | ||
| **Mechanical `## Meta` block via `fab pr-meta`** (rj31): As of rj31, `/git-pr` Step 3c no longer assembles the `## Meta` block from natural-language formatting prose. The entire block — the 5-column top table (`Change ID | Type | Confidence | Plan | Review`), the `**Impact**:` table + caption, the optional `**Issues**:` line, and the `**Pipeline:**` line (six fixed-order stages with ` ✓` per `done` stage and hyperlinked intake/apply labels) — is rendered deterministically by the `fab pr-meta <change> --type <type> [--issues "<space-joined IDs>"]` subcommand, whose stdout the skill pastes verbatim (omitting the block on non-zero exit / empty stdout, matching the legacy `{has_fab} = false` path). **Element order (260625-pnao layout revision):** heading → top table → Impact table + caption → optional Issues → **Pipeline (LAST)** — Pipeline moved from before Impact to the final element, with the optional Issues line sitting just above it. Each table/paragraph is blank-line separated so GitHub renders them distinctly. The top table's first header is **`Change ID`** (was `ID`); a present id is backtick-wrapped (`` `pnao` ``), the empty-fallback `—` stays bare. `**Pipeline:**` carries the colon **inside** the bold span. **Impact is one normalized, self-labeling table** (pnao, replacing the prior multi-form rendering): a single `Impact | +/− | Net` table — the compact `+/−` is the *column header* only, while the data rows carry spaced `+A / −B` figures; the first column header is `Impact` (was `Scope`), so the table needs **no separate `**Impact**:` lead-in line**. Numeric columns are right-aligned. It carries the fixed taxonomy `raw / true / impl / tests / excluded` where `raw = true + excluded` and `true = impl + tests`. `true` is ALWAYS the post-exclude diff (the bold row, always present) — fixing the prior "total flips meaning" bug where the same label denoted the raw diff in one form and the post-exclude diff in another. The table adapts by DROPPING rows, never reshaping: the `raw` row is shown whenever `true_impact_exclude` is configured (`Excluding != nil`, even when raw equals true — a configured-but-no-op exclude still earns its own row), and is omitted only when no excludes are configured (`Excluding == nil`), where `true` is definitionally identical to `raw` so no redundant duplicate row appears *(260625-pnao follow-up — supersedes the prior "shown only when it differs from true" rule per the user's "always show raw when excludes are configured" decision)*; nested `└ impl` / `└ tests` rows appear only when a tests pair is present. Below the table a `<sub>` (small-text, NOT italic) provenance caption co-locates the excludes note and the fab-kit **binary** version stamp — `<sub>excludes `fab/`, `docs/` · generated by fab-kit vX.Y.Z</sub>` (excludes clause omitted when none configured; `vdev` rendered honestly on dev builds). Emphasis stays bold-only and the caption uses `<sub>` because GitHub's Markdown sanitizer strips `style`/`class` (so row backgrounds / colored numbers are impossible) but keeps `<sub>` on its HTML allowlist. Type resolution (Step 0b), issue gathering (Step 1), and the agent-generated Summary/Changes stay skill-side; the progress token remains `✓ body — meta + summary + changes`. `fab pr-meta` is self-contained — it reads `.status.yaml`, parses `plan.md` task checkboxes, reads config (`true_impact_exclude`, `test_paths`, `project.linear_workspace`), computes impact via `internal/impact`, and resolves git/`gh` context (branch, owner/repo, merge-base) itself; the skill passes only `<change>`, `--type`, and optional `--issues`. This moves the Meta block's determinism into Go (tested via golden-output tests), eliminating per-run drift (e.g., the dropped exclude-list backticks the prose previously produced). `gh` failure degrades to plain-text Pipeline labels; a missing merge-base drops only the Impact block — never a hard `/git-pr` failure. `/git-pr` no longer calls `fab impact` directly (the subcommand remains public for other consumers). Blob URLs use `https://github.com/{owner}/{repo}/blob/{branch}/...` to resolve against the feature branch instead of main. See [schemas.md](/pipeline/schemas.md) for the `true_impact` render-time `impl` residual and [the `fab pr-meta` CLI reference](../../../src/kit/skills/_cli-fab.md) for the full signature, output contract, and exit codes. |
Collaborator
Author
There was a problem hiding this comment.
Fixed — dropped the stale **Impact**: label here too; the enumeration now reads "the Impact table + caption", matching the later "no separate **Impact**: lead-in line" statement. (2c760f4)
Drop the stale `**Impact**:` table label in the pr-meta element enumeration in schemas.md and execution-skills.md — the Impact section is a self-labeling table with no `**Impact**:` lead-in, so the old label contradicted the later "no separate `**Impact**:` lead-in" clause. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #444. The
## MetaImpact table previously dropped therawrow when it numerically equalledtrue. It now renders therawrow whenever excludes are configured (Excluding != nil) — even on a no-op exclude — so a reader always seesrawalongsidetrue. When no excludes are configured,trueis definitionally identical toraw, so no separaterawrow is rendered (no redundant duplicate).Changes
prmeta.renderImpact: raw-row conditionraw != trueDiff→Excluding != nil(+ comment/taxonomy doc updates)prmeta_test.go: golden for "excludes configured but no-op (raw == true): raw row still shown"; "no excludes → no duplicate raw row" re-keyed onExcluding == nil_cli-fab.md,SPEC-git-pr.md: raw-row rule prose updateddocs/memory/pipeline/execution-skills.md,schemas.md: raw-row rule prose sweptVerified:
go build/test ./internal/prmeta/... ./cmd/fab/...pass,gofmt/vetclean.🤖 Generated with Claude Code