amirbena / amirbena/code-review-skill
Treat reviews targeting the wrong repository base as an immediate P0 across both review skills
- Dominant language
- Python
- Stars
- 7
- Forks
- 2
- Avg merge
- 27m
- Merged PRs (30d)
- 188
Description
## Problem
A change under review targets an *integration base* — the branch it's meant to merge into. When the review can reliably establish that this base violates the target repository's own review-base policy, the change is aimed at the wrong place: a workflow/repository-policy violation that should block immediately, independent of implementation findings.
Neither Skill enforces this today, and the two could silently diverge: `github-pr-review` has the PR base ref but ignores it for this purpose; `local-code-review` has a "review base" concept (`skills/local-code-review/policies/repository-state.md`) but no notion that a wrong base is a violation.
## Goal
One shared, repository-relative invariant, defined once in canonical shared policy and applied identically by both Skills: when the repository-resolved review base is reliably known and the base under review violates it, emit a single P0 before implementation findings (blocking, per the mechanical derivation in `shared/policies/severity.md`). When the base cannot be reliably resolved, emit nothing for this rule.
## Scope
- Resolve the repository-resolved review base from existing Review Context / Review Target / repository-state contracts (#45, #72) — no parallel model.
- `github-pr-review`: use the PR's actual base ref/SHA.
- `local-code-review`: resolve dynamically from repository/review context (typically the configured default/integration branch); an explicitly supplied base may override resolution only where existing contracts already allow it.
- The invariant must be **repository-relative**: no hardcoded branch name (not always `main`), and Git `HEAD` is never treated as the canonical base merely because it is `HEAD`.
- Unknown/ambiguous/unavailable base (either mode): fail closed — no finding, no invented branch name.
- Add the P0 to canonical shared policy; both Skills reference it, no duplicated copy.
- Finding text names the actual base under review and the resolved base it should have targeted.
- Add contract/drift tests so the two Skills cannot diverge on this rule.
## Non-goals
- Retargeting or editing a PR's base branch.
- Changing GitHub branch protection, rulesets, or required checks.
- General branch-naming validation unrelated to the review base.
- Publishing a GitHub status/check for this outcome (separate GitHub-native enforcement work, see #49).
## Acceptance criteria
- Canonical shared policy defines the wrong-base P0 as repository-relative with no branch name; both Skills reference it.
- `github-pr-review`: PR base reliably violates policy → one blocking P0 before implementation findings; satisfies policy → no finding.
- `local-code-review`: resolves to the repository's configured integration branch; a violating branch → P0; `HEAD` is never substituted as the base merely because it's `HEAD`.
- Unresolved/ambiguous/unavailable base (either mode) → no finding, failing closed.
- Contract/drift tests assert both Skills behave identically on these cases.
## Dependencies
Relates #45, #72 (both open — Review Context / Review Target models this resolves against).
## Validation
- Fixtures covering each acceptance-criteria case for both Skills, including fail-closed and HEAD-not-substituted cases.
- Contract/drift run showing both Skills agree on every shared-semantics fixture.
- Standard validation suite and packaged artifacts pass.
Contributor guide
Assessment
This issue has not been assessed yet.