fix: normalize PR meta Impact block — fix total-flips-meaning bug#444
Merged
Conversation
The Impact block computed `total` as `true_impact + excluded`, then used that inflated denominator when reporting the true-impact percentage, causing the "X% of Y files" figure to be wrong whenever excluded files were present. Fix: compute true_impact_pct against the sum of changed files that pass the true-impact filter, not the grand total including excluded paths. Also adds `--type` and `--issues` flags to `fab pr-meta`, updates SPEC-git-pr.md mirror, and refreshes memory docs for the prmeta schema. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a semantic bug in the PR ## Meta Impact section by normalizing the renderer to a single, consistent markdown-table shape and locking the metric taxonomy (raw / true / impl / tests / excluded), where true is always the post-exclude diff. It also adds an Impact provenance caption that includes the fab-kit binary version, and updates the surrounding skill/spec/memory documentation to match.
Changes:
- Reworked
prmeta.renderImpactto emit one normalizedScope | + / − | Nettable (rows drop, never reshape) plus an italic provenance caption. - Threaded the running
fabbinaryversionintoprmeta.Dataso the renderer can stampgenerated by fab-kit vX.Y.Z. - Updated
/git-prand CLI docs/specs/memory to describe the new Impact shape and taxonomy.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/kit/skills/git-pr.md | Updates /git-pr Meta-block contract text to describe the normalized Impact table + provenance caption and --type/--issues passthrough. |
| src/kit/skills/_cli-fab.md | Updates fab pr-meta output contract documentation to the new normalized Impact table/taxonomy and provenance caption details. |
| src/go/fab/internal/prmeta/prmeta.go | Implements the normalized Impact table renderer and adds Data.Version for provenance stamping. |
| src/go/fab/internal/prmeta/prmeta_test.go | Rewrites golden tests to match the new Impact table shape, row-dropping rules, caption, and dev-version behavior. |
| src/go/fab/cmd/fab/pr_meta.go | Threads version from package main into prmeta.Data.Version before rendering. |
| src/go/fab/cmd/fab/pr_meta_test.go | Adds coverage around the --type/--issues flags and version provenance expectations. |
| fab/changes/260625-pnao-normalize-pr-meta-impact-block/plan.md | Adds the generated plan/requirements/acceptance checklist describing the normalization work. |
| fab/changes/260625-pnao-normalize-pr-meta-impact-block/intake.md | Adds the intake artifact capturing the problem statement, locked design, and constraints. |
| fab/changes/260625-pnao-normalize-pr-meta-impact-block/.status.yaml | Adds change pipeline status metadata for the new change folder. |
| fab/changes/260625-pnao-normalize-pr-meta-impact-block/.history.jsonl | Adds pipeline event history for the change execution. |
| docs/specs/skills/SPEC-git-pr.md | Updates the SPEC mirror to match the new Impact table shape/taxonomy and provenance caption behavior. |
| docs/memory/pipeline/schemas.md | Updates pipeline schema memory to reflect the normalized pr-meta Impact block wording and taxonomy mapping. |
| docs/memory/pipeline/index.md | Updates “Last Updated” date for pipeline index entry to reflect the new memory changes. |
| docs/memory/pipeline/execution-skills.md | Updates execution-skill memory documentation describing the mechanical Meta block and the new Impact table behavior. |
| docs/memory/_shared/index.md | Updates “Last Updated” date for shared memory index entry due to configuration doc changes. |
| docs/memory/_shared/configuration.md | Updates test_paths documentation to reference the normalized Impact table shape in pr-meta output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+301
to
+311
| func impactRow(label string, p impact.Pair, bold bool, annotation string) string { | ||
| scope := label + annotation | ||
| plusMinus := fmt.Sprintf("+%d/−%d", p.Added, p.Deleted) | ||
| net := fmt.Sprintf("+%d", p.Net) | ||
| if bold { | ||
| scope = "**" + scope + "**" | ||
| plusMinus = "**" + plusMinus + "**" | ||
| net = "**" + net + "**" | ||
| } | ||
| return fmt.Sprintf("| %s | %s | %s |\n", scope, plusMinus, net) | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Fixed — the Net column now goes through a local formatNet helper (mirroring internal/change/change.go), so a negative net renders as -N rather than +-N. The raw/true/tests rows pass unclamped Net values (only the impl row is pre-clamped), so this was the unguarded path. (3767d61)
Comment on lines
+72
to
+78
| // TestPrMetaCmd_VersionThreadedIntoProvenance pins the cmd-wiring contract: the | ||
| // package main `version` var is stamped into the Meta block's provenance caption | ||
| // (pnao). prmeta.Render stays a pure function of Data — the command is the sole | ||
| // populator of Data.Version — so this asserts the same value RunE assigns | ||
| // (`data.Version = version`) reaches the rendered caption. The default build | ||
| // value is "dev", which must render honestly as `fab-kit vdev`. | ||
| func TestPrMetaCmd_VersionThreadedIntoProvenance(t *testing.T) { |
Collaborator
Author
There was a problem hiding this comment.
Fixed — corrected the docstring so it no longer overclaims. It now states the test exercises prmeta.Render directly with the package-main version var (rather than driving RunE, which needs a live fab context), and that it verifies the caption-render wiring, not the data.Version = version assignment itself. (3767d61)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…beling Impact table, <sub> caption, Pipeline-last) Co-Authored-By: Claude Opus 4.8 (1M context) <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.
Meta
Pipeline: intake ✓ → apply ✓ → review ✓ → hydrate ✓ → ship → review-pr
Impact:
impl: +109/−45 (net +64)
tests: +91/−51 (net +40)
total: +200/−96 (net +104) ← excludes
fab/,docs/Summary
The
prmetaImpact block had a semantic bug wheretotalmeant two different things depending on whether excluded paths were present — in one render shape it showed the raw (pre-exclude) diff, in another it showed the post-exclude diff. This fix normalizes the Impact block to a single consistent table shape with a locked metric taxonomy (raw / true / impl / tests), sotruealways means the post-exclude diff. It also adds a provenance caption with the fab-kit binary version and the excludes note, and adds--typeand--issuesflags tofab pr-metaso the git-pr skill can pass them through.Changes
raw = true + excluded,true = impl + tests; "total" removed from the vocabularyScope | + / − | Netcolumns; rows drop (never reshape) when tests or excludes are absent*excludes … · generated by fab-kit vX.Y.Z*co-locates the excludes note and version stampprmeta.Datagains aVersion stringfield;cmd/fab/pr_meta.gothreads the binary version infab pr-metagains--typeand--issuesflags🤖 Generated with Claude Code