Skip to content

fix(review): canonicalize provider finding IDs - #1

Open
ftorga wants to merge 2 commits into
mainfrom
fix/1699-provider-finding-ids-rebased
Open

fix(review): canonicalize provider finding IDs#1
ftorga wants to merge 2 commits into
mainfrom
fix/1699-provider-finding-ids-rebased

Conversation

@ftorga

@ftorga ftorga commented Jul 24, 2026

Copy link
Copy Markdown
Owner

🔗 Linked Issue

Closes Gentleman-Programming#1699
Closes Gentleman-Programming#1699


🏷️ PR Type

What kind of change does this PR introduce?

  • type:bug — Bug fix (non-breaking change that fixes an issue)
  • type:feature — New feature (non-breaking change that adds functionality)
  • type:docs — Documentation only
  • type:refactor — Code refactoring (no functional changes)
  • type:chore — Build, CI, or tooling changes
  • type:breaking-change — Breaking change (fix or feature that changes existing behavior)

📝 Summary

  • Canonicalize missing provider finding IDs before artifact admission.
  • Reject malformed, cross-lens, duplicate, and colliding IDs without mutating authority.
  • Preserve candidate-causal IDs across capture/replay.

📂 Changes

File / Area What Changed
internal/cli/review_artifact.go Canonicalize provider finding IDs before artifact admission and preserve candidate-causal IDs during capture.
internal/cli/review_artifact_test.go Add CLI coverage for canonical provider finding ID behavior and invalid admission cases.
internal/cli/review_facade.go Carry provider finding ID context through the review facade.
internal/reviewtransaction/artifact_admission.go Validate malformed, cross-lens, duplicate, and colliding IDs without mutating authority.
internal/reviewtransaction/artifact_admission_test.go Add admission tests for invalid, duplicate, and collision scenarios.

🧪 Test Plan

Unit Tests

go test ./...

Passed: 7,031 tests across 61 packages.

Go Format

go run ./internal/gofmtcheck

Passed.

E2E Tests (Docker required)

cd e2e && ./docker-test.sh

Passed on Ubuntu, Arch, and Fedora: 237 passed, 0 failed, 6 opt-in skipped.

  • Unit tests pass (go test ./...)
  • Go format passes (go run ./internal/gofmtcheck)
  • E2E tests pass (cd e2e && ./docker-test.sh)
  • Manually tested locally

🤖 Automated Checks

The following checks run automatically on this PR:

Check Status Description
Check PR Cognitive Load PR should stay within 400 changed lines (additions + deletions) or use size:exception
Check Issue Reference PR body must contain Closes/Fixes/Resolves #N
Check Issue Has status:approved Linked issue must have been approved before work began
Check PR Has type:* Label Exactly one type:* label must be applied
Unit Tests go test ./... must pass
Go Format go run ./internal/gofmtcheck must pass
E2E Tests cd e2e && ./docker-test.sh must pass

✅ Contributor Checklist

  • PR is linked to an approved upstream issue
  • PR stays within 400 changed lines (174)
  • I have added the appropriate type:* label to this PR (requested type:bug; fork label is currently unavailable)
  • Unit tests pass (go test ./...)
  • Go format passes (go run ./internal/gofmtcheck)
  • E2E tests pass (cd e2e && ./docker-test.sh)
  • Documentation update is not required; no user-facing behavior documentation changed
  • My commits follow Conventional Commits format
  • My commits do not include Co-Authored-By trailers

💬 Notes for Reviewers

This is the fork-first validation PR; no upstream PR should be created yet.

The fork does not currently provide a type:bug label. The default bug label is not semantically equivalent, so it was not substituted. This is the remaining labeling blocker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(review): align optional finding IDs with candidate-causal admission

1 participant