feat(review): add versioning carve-out, drift, and ordering bars to code-quality - #183
Conversation
…ode-quality Versioning carve-out: qualifiers legitimate only under External-contract or Production-obligation exposure per the legacy-and-migration-debt classifier; internal V2 identifiers are temporary expand-and-contract states. Stale-descriptor drift bar in Authoring hygiene. Suggestion-tier alphabetical-ordering bar in Style with the tool-sortable escalation path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019LAz83zmsgyd4cTCKuYtS6
Exact classifier tier tokens, explicit Important severity for the unreasoned internal-V2 case, staleness ownership moved wholly to the stale-descriptor bar, and a section-anchored escalation link. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019LAz83zmsgyd4cTCKuYtS6
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c163448d6c
ℹ️ 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".
|
Claude finished @kyle-sexton's task in 3m 19s —— View job Review
Three changes in scope: the alphabetical-ordering bar added to Style; the version-qualifiers bar expanded with the carve-out and mechanism preference; a stale-descriptor bar added to Authoring hygiene with ownership of the "false current fact" moved here from the comments bar. On the Codex P1 (missing primary-source citations)Partially confirmed, but narrowed. AGENTS.md says: "New or changed review criteria are authored fresh from primary sources and cite them." The Sources section is unchanged across all three additions. Alphabetical ordering — this is the one with a real gap. The criterion is sound but has no traceable primary external source. It's not covered by Fowler/Martin/Ousterhout (the existing three). A community style guide (e.g., a PEP, a Google Style Guide section) would supply it; none is cited. This criterion is new enough that the AGENTS.md rule applies squarely. Version qualifiers carve-out — the expand-and-contract concept is already covered by the Fowler & Beck cite; the exposure classifier it references lives in Stale-descriptor bar — traceable to Clean Code (Ch. 4 principles on comment honesty, keeping code and its documentation in sync), already cited. No new external source needed. So the Codex finding is accurate in identifying a gap, but overstates it as covering all three bars. The concrete deficit is: the alphabetical-ordering bar lacks a primary external source in the Sources section. Fix this → Severity: Suggestion — AGENTS.md's requirement is real, but alphabetical ordering is a universal convention without one canonical source (unlike a named Fowler pattern). A style guide citation would satisfy the letter; whether the spirit requires it here is a judgment call for the author. Correctness and consistency checks
Staleness ownership transfer — clean. The comments bar (line 32) now reads "a comment left stating a false current fact belongs to the stale-descriptor bar under Authoring hygiene"; the stale-descriptor bar (line 40) owns the check. No duplication; pointer is unambiguous. Version qualifiers severity — the carve-out correctly applies Mechanism preference placement — the Suggestion-tier URL/metadata note lives inside the version-qualifiers bar. The PR description records the deliberate decision not to move it to Expand-and-contract reference — grounded. OverallThe three additions are internally consistent, cross-doc references check out, severity assignments follow the file's established patterns, and the ownership consolidation of stale-descriptor is correct. One substantive gap: the AGENTS.md-required primary source citation for the alphabetical-ordering bar is missing from the Sources section. |
Google C++ include-order and Java import-order guides, anchor-verified, inline and in Sources per the AGENTS.md primary-source requirement. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019LAz83zmsgyd4cTCKuYtS6
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ee4159e33
ℹ️ 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".
|
Claude finished @kyle-sexton's task in 2m 16s —— View job Review
On the Codex P2 findingNot confirmed as a genuine gap. The P2 finding repeats the same structural complaint as P1 ("only internal cross-doc pointers") without accounting for the existing Sources coverage. Here is the per-item breakdown: Versioning carve-out + internal-V2 severity rule URL-versioning preference (prefer headers/media type over URL paths) Stale-descriptor bar P1 fix verificationCommit One Suggestion: Martin citation missing Ch. 4The stale-descriptor bar is most precisely grounded in Clean Code Ch. 4 (Comments), where Martin argues that inaccurate comments are actively harmful. Adding OverallThe three additions are correct and internally consistent. The P1 fix is properly applied. The Codex P2 finding is not substantiated — the versioning and stale-descriptor criteria are already covered by existing citations; the URL-versioning preference is Suggestion-tier editorial guidance that sits below the threshold where a missing external cite is a clear requirement. The only actionable item is the Martin Ch. 4 annotation, which is a Suggestion-level precision improvement rather than a blocking gap. |
Microsoft API design guidance for the versioning-mechanism landscape, Clean Code Ch. 4 named for stale-comment coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019LAz83zmsgyd4cTCKuYtS6
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89d8a77a7f
ℹ️ 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".
|
Claude finished @kyle-sexton's task in 1m 46s —— View job Review
On the Codex P2 finding (line 38: internal-V2 clause)Confirmed. The conflict is genuine. The relevant texts:
Two specific problems in the current wording: 1. Expand-and-contract is the wrong frame for internal surfaces. 2. The conditional severity is the operative conflict. A reviewer who reads the current bar and sees a Suggested rewrite for the internal-V2 clauseReplace:
With something like:
This removes the expand-and-contract framing, makes Important unconditional, and gives reviewers the right action when a ticket exists (note it, not drop the finding). Severity: Important — the current wording lets Everything elseThe rest of the diff is clean: the alphabetical-ordering bar, the stale-descriptor bar, staleness ownership transfer from comments, source additions (Google style guides, Microsoft API design guidance, Martin Ch. 4), and the exposure carve-out for the version-qualifiers bar all hold up. The only substantive issue is this internal-V2 clause. |
Internal surfaces are never versioned per code-organization.md; a V2 identifier is tolerated only as transient expand-and-contract state of an external contract, still flagged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019LAz83zmsgyd4cTCKuYtS6
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7dd76fea4
ℹ️ 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".
|
Claude finished @kyle-sexton's task in 1m 48s —— View job Review
On the Codex P2 finding (line 40: stale-descriptor scope too narrow)Confirmed. The gap is real. Two relevant texts at HEAD:
The ownership transfer from the comments bar to stale-descriptor is correct in intent, but the stale-descriptor bar's trigger condition — "a change that alters behavior" — is strictly narrower than the concept it now owns. A docs-only or comment-only diff that introduces a false statement about current behavior does not fire the stale-descriptor trigger as written, even though the comments bar delegates exactly that case there. Practical failure path: reviewer sees a docs-only PR update a comment to say something incorrect about the current code. Goes to comments bar → "false current fact → stale-descriptor." Goes to stale-descriptor → title says "after a behavior change," body says "a change that alters behavior." No behavior change in this diff. Reviewer may in good faith conclude the bar doesn't apply and not flag the false statement. That is the coverage gap. Suggested fix: Drop the behavior-change precondition from the bar. The fundamental problem — a descriptor that doesn't match actual behavior — is the same whether the mismatch was left behind by a refactor or introduced directly by a docs-only change. Suggested rewrite:
This satisfies the comments bar's delegation for "false current fact" without a behavior-change precondition, and remains compatible with the ownership consolidation already applied. Severity: Suggestion — the gap is genuine but bounded to an uncommon PR type (docs-only changes that actively introduce false statements). The most common case (behavior change → forgotten update) is correctly covered by the current text; only the independent false-descriptor case falls through. Everything elseThe rest of the current file is clean: internal-V2 / |
Drift bar broadened to any edit introducing a descriptor false about current behavior, closing the docs-only gap left when false-current-fact ownership moved here from the comments bar. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019LAz83zmsgyd4cTCKuYtS6
|
Claude finished @kyle-sexton's task in 3m 1s —— View job Review
Reviewed the file at HEAD ( Fix verificationP1 — Alphabetical ordering citations ( P2 — Versioning mechanism and stale-descriptor sources ( P2 — Internal-V2 / P2 — Stale-descriptor coverage gap ( One residual SuggestionLine 38 — severity condition could be read as applying outside the tolerated context The sentence reads:
The conditional ("Important when…") strictly governs the tolerated case (mid-migration state for an external rollout). In that case the logic is sound: a tracked removal plan makes the transience credible, so the finding stays open but drops in urgency. The potential read-across: a reviewer encountering an internal V2 that is not inside any external contract rollout — a straight rule violation per A one-clause addition would close this: e.g., after "still flagged — Important when it carries no recorded reasoning or removal path," add "(Important regardless when the identifier falls outside any active external contract rollout)." Suggestion only — the text's current structure already limits tolerance to the external-rollout context, and a careful reader reaches the same conclusion; the ambiguity arises only if the conditional is read in isolation. Everything elseAll clean: alphabetical-ordering Suggestion tier and escalation routing correct; staleness ownership consolidation consistent; Sources section complete with accurate chapter and section annotations; |
…#225) Adopt the new zizmor SARIF-guard gating shipped in ci-workflows #183. The zizmor lane now runs `--format=sarif` through the in-lane guard and **blocks on any high-severity (SARIF `error`) workflow-security finding** — template-injection, dangerous-triggers, unpinned-uses, etc. — while lower severities still only annotate. Gating rides the existing required `ci-status` lane: no code-scanning, no `security-events` grant, no org-wide ruleset change. ## Changes - `ci.yml`: bump the `zizmor.yml` pin to `31a5b76` and set `fail-on-severity: high`; comment updated. - `components/runner-policy/policy.json`: register the new runner-input contract for `zizmor.yml@31a5b76` with `fail-on-severity` in `allowedInputs` — `runner-policy` would otherwise reject the new pin + input (its lane is in `ci-status`). ## Safety Pre-flight `zizmor --persona=regular` (online) on this repo → **0 high findings** (3 notes: 2 `adhoc-packages`, 1 `use-trusted-publishing`). The `high` gate does not block existing workflows. `lint:runner-policy` + `test:runner-policy` (227/227) pass locally. ## Related No related issue: rolls out the consumer-side opt-in for the gating capability added in melodic-software/ci-workflows#183. The 3 remaining note-level findings are a separate follow-up (documented-ignore / OIDC trusted-publishing), below this gate's threshold. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
What
Three additions to
conventions/review/code-quality.md. A carve-out on the version-qualifiers bar: qualifiers are legitimate only under External contract or Production obligation exposure per thelegacy-and-migration-debt.mdclassifier (versioned public APIs and published message contracts qualify; contract evolution itself stays owned bymessaging.mdandarchitecture.md), with internalV2identifiers as temporary expand-and-contract states — Important when unreasoned — and a Suggestion-tier, non-prescriptive metadata-over-URL preference. A stale-descriptor drift bar in Authoring hygiene: a behavior change leaving comments, docs, names, or examples describing the old behavior (Suggestion inline; Important for user-facing or operator docs), now the single owner of comment-staleness (the "false current fact" clause moved here from the comments bar). An alphabetical-by-default ordering bar in Style for order-independent enumerable sets, with semantic and community-prescribed orders winning and tool-sortable surfaces routed to the enforcement-escalation path.Phase 4 of the criteria-backlog program — #179 tracks the full sequence.
Reconciliation checklist
../engineering/legacy-and-migration-debt.md(classifier cited with exact tier tokens, not restated),messaging.mdandarchitecture.md(contract-evolution ownership pointed at, not restated),../engineering/enforceability-tiers.md(tool-sortable routing cited),code-design.md(no overlap with its cross-unit scope).REVIEW.mdchecked-not-contradicted; the add-a-line decision is deliberately batched at program Phase 6 (Criteria-backlog authoring program: timebombs, date-time, atomicity, hygiene bars (7 phases) #179), not made per-PR.architecture.md— the note exists to answer "what instead of a v2 identifier" at the point of flagging, the plan locked it to this bar, andarchitecture.mdis edited by the in-flight Phase 5 branch.npm run lint:mdgreen; sanity checks pass (alphabetical, exposure, expand-and-contract greps all ≥ 1).🤖 Generated with Claude Code
https://claude.ai/code/session_019LAz83zmsgyd4cTCKuYtS6
Related
No linked issue: this PR advances the criteria-backlog program without closing it.