Skip to content

docs(review-instructions): drop maintainer history from REVIEW.md - #630

Merged
kyle-sexton merged 2 commits into
mainfrom
fix/620-review-md-prompt-audit
Sep 28, 2026
Merged

kyle-sexton merged 2 commits into
mainfrom
fix/620-review-md-prompt-audit

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Closes #620

Summary

A prompt audit flagged two medium-confidence findings in REVIEW.md. Managed Code Review injects this file verbatim as its highest-priority instruction, so maintainer-history prose in it reaches the review agent as an instruction rather than as documentation for people editing the file.

Fix

  • F1 (lines 39-42): removed the hedge that recognizing a blocking-tagged criterion is "reviewer judgment, piloted rather than asserted reliable" and the now-stale claim that "No criterion file currently carries one." Kept the two load-bearing rules: a blocking tag always means Important, and blocking is this file's own tag rather than a conventions/review/ severity.
  • F2 (lines 54-60): removed the editor-guidance paragraph starting "Each line below is written to survive losing that depth...". It gives the reviewer nothing to check and cites depth that line 46 already says Managed Code Review cannot read.

Root REVIEW.md is the canonical source for the review-instructions sync component (distribution/sync-manifest.yml); there is no separate components/review-instructions/ tree, and no other repository file duplicates its content or hash.

Verification

  • git grep confirms no test or doc asserts the removed sentences verbatim.
  • npx markdownlint-cli2 --config .markdownlint-cli2.jsonc REVIEW.md: 0 issues.
  • bash distribution/sync-manifest.test.sh (after npm ci --prefix distribution): 296/296 assertions pass against the committed change (manifest-structure tests only; none assert REVIEW.md prose).
  • Pre-commit hooks (editorconfig, typos, markdownlint, gitleaks) passed on the commit.
  • Not run: the issue's suggested before/after review comparison on a security-scoped PR — that requires a live Managed Code Review pass outside this worktree.

Related

Closes #620. Proposed diff in the issue matched the current file's line ranges (39-42, 54-60) exactly; applied verbatim.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LCEBhuXxZAT8K619tC6T5f

Removes maintainer-history prose the review agent reads as
highest-priority instruction. The `blocking` severity crosswalk no
longer hedges reviewer judgment or claims no criterion carries the tag;
the editor-guidance paragraph about surviving lost citation depth is
gone since it gives the reviewer nothing to check.

Closes #620

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LCEBhuXxZAT8K619tC6T5f
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-27T22:40:48.273258Z 9e1ee45 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 18s —— View job


Security review

  • Gather context (PR diff, changed files)
  • Run /review:security-review criteria
  • Post results

Result: No security review needed.

This PR touches only REVIEW.md (the code-review lane's prompt-instructions doc), removing two prose passages that gave the reviewer nothing to check (a hedge/stale claim about the blocking tag, and an editor-guidance paragraph about surviving lost context depth). The diff has no code, configuration, or security-relevant surface — it changes what instructions a review agent reads, not any executable behavior, data handling, or trust boundary.

Per this skill's skip gate: "Change has no security-relevant surface after reading the diff." Stopping here with no findings.

@claude

claude Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 37s —— View job


Code review complete

  • Gather context (diff, REVIEW.md, CLAUDE.md)
  • Check skip gate
  • Review changed file for correctness/alignment
  • Post findings

Scope: REVIEW.md only (+2/-12), per gh/git diff origin/main...HEAD.

Findings: none. This is a clean, well-scoped prose deletion from an injected-instruction file, and I could not find a correctness issue a careful reviewer would block or flag on the changed lines.

What I checked and independently verified (not just author-claimed):

  • F1's removed "no criterion file currently carries [blocking]" claim: confirmed via grep -rn "Tagged \blocking`"across*.mdthat onlyREVIEW.mditself uses that exact annotation, and confirmed no file underconventions/review/ (security.md, multi-tenancy.md, error-handling.md, observability.md) assigns a blocking tag to a criterion. The retained line 39-40 ("blockingis this file's own tag, not a severity aconventions/review/` criterion assigns") is still accurate post-edit, so the deletion doesn't leave stale or contradicted text behind.
  • Sync-manifest claim: distribution/sync-manifest.yml:305 has exactly one REVIEW.md: REVIEW.md mapping, confirming the PR body's claim that there's no separate components/review-instructions/ tree or duplicate.
  • Formatting: inspected the edited regions (## Severity, ## Depth) directly — no stray blank lines, no trailing whitespace, headers still correctly spaced after both deletions.
  • Remaining prose coherence: read the full post-edit file top to bottom; both trimmed sections (lines 37-40, 42-50) still read as complete, self-contained statements with no dangling references to the removed hedges.

Author-claimed but not independently re-run here (no shell-operator/network access in this pass): markdownlint-cli2, distribution/sync-manifest.test.sh (296/296), and pre-commit hooks. No reason to doubt them given the diff is prose-only and outside any file the manifest tests assert against.

No cross-file or whole-PR findings to anchor to a commit-blob permalink.

@kyle-sexton
kyle-sexton enabled auto-merge (squash) September 27, 2026 23:28
kyle-sexton added a commit that referenced this pull request Sep 28, 2026
…ockstep-drift (#634)

Closes #633

## Summary

`pr-convention-lockstep` failed on every open PR with `drift: hook
validator: KEYWORD_ERE / NO_ISSUE_ERE declarations not found`.
melodic-software/claude-code-plugins#4636 rewrote the hook validator
around a per-line `scan_linkage` and renamed its closing pattern to
`CLOSING_ERE`, so `parseValidatorPatterns` could not find it. The
validator and the ci-workflows `pr-contract` composite still agree, so
this was a parse failure, not drift.

## Fix

- `parseValidatorPatterns` reads `CLOSING_ERE` (plus the unchanged
`NO_ISSUE_ERE`) and translates `[[:blank:]]` as well as `[[:space:]]`.
- The new EREs are lowercase with no `i` flag, matched against
`${line,,}`. The parser now asserts that lowercasing is present and the
probes lowercase their input, the same way the composite check already
asserts `lower = tolower(line)`.
- The test fixture takes the new validator shape; a new test fails when
the validator stops lowercasing.
- `policy.json` is unchanged. Every closing keyword and no-issue marker
it lists is still accepted by both the composite (`run.sh` `scan_line`
and the `no-issue` test) and the validator. The new `Refs:` / `Relates
to:` acceptance and negated-closer rejection are enforced by both, but
`policy.json` has no field for them and `pr-convention-policy.mjs` and
the pinned reusables do not implement them, so recording them would be a
separate change.

## Verification

- `npm run test:pr-convention-lockstep`: 40/40 pass.
- `npm run test:pr-convention-policy`: 11/11 pass.
- `npm run lint:pr-convention-lockstep` against live sources with a
token: `pr-convention lockstep: all copies and consumer pins match
policy.json`, exit 0.
- `biome check` on both touched files: clean.

## Related

- melodic-software/claude-code-plugins#4636 (the validator rewrite)
- melodic-software/ci-workflows#544 (the `pr-contract` negation and
`Refs:` change the validator mirrors)
- Unblocks #628, #630, #631, #632

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01LCEBhuXxZAT8K619tC6T5f

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 24s —— View job


Security review

  • Gather context (PR diff, changed files)
  • Run /review:security-review criteria
  • Post results

Result: No security review needed.

Skip gate applies on two independent grounds:

  • No security-relevant surface. git diff origin/main...HEAD -- REVIEW.md shows only prose deletions in a documentation file (the code-review lane's prompt-instructions doc): a hedge/stale claim about the blocking tag, and an editor-guidance paragraph about surviving lost context depth. No code, configuration, or trust-boundary changes.
  • Already reviewed at this content. git diff 9e1ee45 665946e -- REVIEW.md is empty — the current head (665946e) only merges in unrelated main commits (cd38de2) on top of the commit (9e1ee45) that already received a successful security review with the same conclusion. REVIEW.md itself is unchanged since then.

@kyle-sexton
kyle-sexton merged commit f887488 into main Sep 28, 2026
49 checks passed
@kyle-sexton
kyle-sexton deleted the fix/620-review-md-prompt-audit branch September 28, 2026 13:42
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.

REVIEW.md: two prompt-audit findings for the review-instructions component

1 participant