Skip to content

code-review: spec-fidelity + smell-baseline lenses (backward-compatible JSONL extension) v4.8.0 - #84

Merged
sumitake merged 1 commit into
mainfrom
dev/claude/code-review-spec-axis
Aug 3, 2026
Merged

code-review: spec-fidelity + smell-baseline lenses (backward-compatible JSONL extension) v4.8.0#84
sumitake merged 1 commit into
mainfrom
dev/claude/code-review-spec-axis

Conversation

@sumitake

@sumitake sumitake commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Summary

Deferred follow-up A from #83, per operator direction: the code-review skill gains a spec-fidelity lens and a Fowler smell baseline, designed against the concerns from the plan cross-check (see verbatim verdict below) rather than as naive prompt enrichment:

  • JSONL contract extended backward-compatibly — new Spec and Smell severities + optional spec_ref; a consumer filtering Critical|High|Medium|Low is untouched, so merge-blocking behavior cannot change silently. Documented as a schema extension with the skill version as the contract version.
  • Spec materialization rules: precedence (user-passed → issue refs in the reviewed commit range only → repo spec files); same-tier ambiguity → ask or spec unavailable: ambiguous; never synthesize a spec or infer from branch names; line-numbered snapshot with source ref for citations; spec content sent as labeled untrusted data with an explicit cannot-alter-instructions instruction; the diff remains the sole provenance-bearing artifact.
  • Smell baseline is subordinate and evidence-bound: repo-documented standards override; Smell severity, never Critical/High without an independently demonstrated consequence; tooling-skip decided from materialized config only.
  • Pipeline separation: spec/smell findings keep their own counts and sections through synthesis; readiness impact is a stated judgment, never an automatic aggregation.
  • Licensing: adapted-portion attribution (mattpocock/skills code-review @2ab95809, blob 2a0b5240); full MIT notice in the spec; member SPDX = LicenseRef-PolyForm-Strict-1.0.0 AND MIT (new MIXED_LICENSE_SKILL_MEMBERS); NOTICE generalized to reference the provenance doc (pinned constants + contract tests updated once, so PR B does not churn legal text).
  • Version 4.7.0 → 4.8.0 across all seven surfaces; changelog fragment; marketplace regenerated.

Validation

  • build_skills.py --check clean; check_release_consistency.py RESULT: OK; build-changelog.py --dry-run OK
  • unittest discover -s scripts 319/319 OK; unittest discover -s tests 658/658 OK

Cross-check (verbatim)

Codex (gpt-5.6-sol, effort medium, direct codex exec under the operator's broker-bypass authorization):

VERDICT: PROCEED-WITH-MODIFICATIONS

All 7 concerns integrated: (1) typed schema extension not prose-only (Spec/Smell severities, documented, contract-version = skill version); (2) ambiguity/trust/commit-range/snapshot/no-branch-inference rules added; (3) semantic separation through the pipeline; (4) smell baseline subordinate + evidence-bound + materialized-config tooling-skip; (5) full MIT notice in the touched spec + NOTICE/pinned constants/SPDX/tests updated (upstream blobs verified locally: code-review 2a0b5240, to-tickets 96deac51); (6)+(7) apply to PR B and are integrated there.

author: claude
author_model: claude-fable-5 (effort: default)
standing_directives: skill-change workflow; #1 trigger phrases (description unchanged — lenses are body-level); #3 release pending operator decision (4.6.0 precedent surfaced); #4 N/A (plugin change); #5 plugin README updated in this PR; #7 model+effort stated; #8 no fan-out
tier: 2
cross_check: PROCEED-WITH-MODIFICATIONS (codex gpt-5.6-sol, effort medium, direct codex exec under operator broker-bypass authorization 2026-08-03; raw verdict quoted in PR body; concerns 1-5 integrated here, 6-7 deferred to PR B where they apply)
post_condition: 319/319 + 658/658 tests OK; build --check clean; release-consistency OK; changelog dry-run OK; no coordinator/route change
mcp_coverage_gap: none — prompt-contract change only; gemini_mcp_path: retired / not applicable
contributor_rights: OPERATOR-CONFIRMED
operator_reserved: no
plugin_affected: this PR is the plugin change
readme_refresh_status: README updated (plugin repo); workspace companion at release

🤖 Generated with Claude Code

…ompatible JSONL extension) v4.8.0

Deferred follow-up from #83. Spec materialization with precedence,
ambiguity, no-synthesis, untrusted-data rules; Smell/Spec severities +
optional spec_ref as a documented schema extension (skill version =
contract version); spec/smell findings stay out of defect aggregation.
Mixed-license member (PolyForm AND MIT) in SPDX; NOTICE generalized to
point at the provenance document.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@sumitake
sumitake merged commit 13d86f7 into main Aug 3, 2026
16 checks passed
@sumitake
sumitake deleted the dev/claude/code-review-spec-axis branch August 3, 2026 20:45

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b181855840

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

6. Architectural anti-patterns (leaky abstractions, circular deps, hidden coupling, broken layering)

Additionally:
- SMELL BASELINE (heuristic, subordinate): flag Fowler smells (Mysterious Name, Duplicated Code, Feature Envy, Data Clumps, Primitive Obsession, Repeated Switches, Shotgun Surgery, Divergent Change, Speculative Generality, Message Chains, Middle Man, Refused Bequest) ONLY where the changed code itself shows the evidence. Use severity "Smell" for these; a documented repo standard overrides the baseline; skip anything the repo's linter/formatter config (provided below when found) already enforces.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Pass repository rules into the smell prompt

When reviewing the preferred diff-only artifact and the repository's standards or linter configuration are unchanged, the verifier receives neither of them—the template below provides only the optional spec and artifact—despite being told that this context is "provided below when found." Consequently, the always-on smell lens cannot apply either override and can report observations that the repository explicitly permits or tooling already enforces; materialize and attach the relevant rules/configuration, or perform these exclusions during verified synthesis.

Useful? React with 👍 / 👎.


Output ONLY JSONL (one JSON object per line, no preamble, no closing, no code fence) per this schema:
{"severity":"Critical|High|Medium|Low","file":"<path>","line":<number>,"issue":"<short description>","fix":"<concrete fix recommendation>"}
{"severity":"Critical|High|Medium|Low|Smell|Spec","file":"<path>","line":<number>,"issue":"<short description>","fix":"<concrete fix recommendation>","spec_ref":"<spec source:line — Spec findings only, omit otherwise>"}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Allow spec findings without code locations

For the newly requested case of a spec requirement that is entirely missing from the diff, there may be no truthful code file or line—the preceding instruction even says to provide those fields only "where available"—but this functional JSONL schema still requires both. The verifier must therefore invent a location or violate the schema and trigger malformed-output handling; make these fields optional for Spec findings or define an explicit representation for absent implementation locations.

Useful? React with 👍 / 👎.

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.

1 participant