Add the Slopinator to the review panel - #460
Conversation
Adds a seventh specialist to skillsaw-review-panel. It reads the writing in a diff as an editor: code comments, docstrings, docs, commit messages and PR prose. Part A catches review-history residue — comments that describe the review conversation instead of the shipped code. skillsaw PR stbenjam#451 merged four such comments that had to be removed afterward, including one describing a bug that never existed on main and test sections headed "Review follow-ups, round three". Part B applies AI-prose detection to technical writing: vocabulary tells, copula avoidance, rule-of-three padding, negative parallelism, signposting, sycophancy, hedging, Title Case headings, manufactured punchlines and generic conclusions. Prior art is the humanizer skill (https://github.com/blader/humanizer) and Wikipedia's "Signs of AI writing". The reviewer is deliberately calibrated to stay advisory: findings are SUGGESTION or NOTE by default, it can only reach BLOCKING when a comment is factually false about the shipped code and another specialist corroborates, and it never sets the disposition on its own. An explicit "what NOT to flag" section protects long "why" comments, project house style, domain vocabulary, correctly used em dashes and version-scoped documents like changelogs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe review panel now runs seven specialists plus an arbiter, adds Palimpsest review guidance and arbitration rules, updates serial execution and verdict rendering, and adds regression tests for roster, dispatch, synchronization, and template completeness. ChangesReview panel behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoAdd Palimpsest specialist reviewer to skillsaw review panel
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
|
Heads up — this conflicts with #459 on one line, in all three skill copies. #459 fixes a live bug in the parallel dispatch added by #458: the specialists are launched with Both PRs edit the same line — this one changes Suggest landing #459 first, since until it does the Palimpsest Reviewer would be dispatched and abandoned along with the other six. |
Code Review by Qodo
Context used✅ Compliance rules (platform):
136 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #460 +/- ##
=======================================
Coverage 93.54% 93.54%
=======================================
Files 145 145
Lines 11418 11418
=======================================
Hits 10681 10681
Misses 737 737 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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: c746515f5a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Fix conflicts and Palimpsest is a weird name. Call it slopinator. I think you made this from an outdated branch ... |
Resolves the conflict with stbenjam#459 as flagged on both PRs — `all 7` from here, `run_in_background: false` from there. Addresses the review findings: - **The BLOCKING criterion was unsatisfiable.** Palimpsest was told to escalate a factually false comment only when Architecture or the Technical Writer corroborated it — but specialists run isolated from one another, so it could never see that and would always self-downgrade, while synthesis never upgraded it back. A corroborated correctness defect could ship as a SUGGESTION under an APPROVE. It now reports a `BLOCKING CANDIDATE` and the arbiter assigns the final severity in a new synthesis step, which may not leave one unsettled. - **Regression coverage** for both this reviewer and the stbenjam#459 dispatch fix, in a new `tests/test_review_panel_skill.py`. The panel is a prompt, so its failure modes are textual and invisible to the rest of the suite — which is exactly how the headless silent-exit shipped green. The guards cover: dispatch is synchronous and background dispatch appears only in its own prohibition; the run may not end before the verdict; every specialist is rostered with a scope file that exists and a quality gate; Palimpsest reports a candidate rather than deciding; the three skill copies are byte-identical; and the verdict template has a section per specialist. Adding a seventh specialist pushed SKILL.md past the 3,000-token warn limit its own linter enforces — main was already at ~2,930. Serial mode moved to `references/serial-mode.md`: it is the non-default path, so an always-read copy costs every invocation for something most never use. That is the progressive-disclosure pattern the skill already applies to specialist scopes, and stbenjam#458 inlined dispatch specifically because it *is* always read. The file is now smaller than on main despite the addition, and a test pins the budget so the next specialist has to face the same question. No README change: the review panel is not documented there, so there is nothing for a new specialist to fall out of sync with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/test_review_panel_skill.py (3)
91-100: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the actual Palimpsest dispositions.
The arbiter test only checks marker text, while the “advisory by default” test checks
What NOT to flag, not theSUGGESTION/NOTEdefault. A regression to the wrong fallback severity could pass these tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_review_panel_skill.py` around lines 91 - 100, Strengthen test_the_arbiter_settles_candidates and test_the_reviewer_stays_advisory_by_default to assert the actual Palimpsest disposition values, including SUGGESTION/NOTE as the default advisory severity, rather than only checking descriptive marker text or “What NOT to flag.”
150-155: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert findings placeholders, not just reviewer names.
This test passes if the Palimpsest section exists but
{{PALIMPSEST_FINDINGS}}is missing. Use an explicit reviewer-to-placeholder mapping and assert each token.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_review_panel_skill.py` around lines 150 - 155, Update TestVerdictTemplate.test_every_specialist_has_a_findings_placeholder to use an explicit reviewer-to-placeholder mapping and assert each expected findings token, such as {{PALIMPSEST_FINDINGS}}, is present in verdict-template.md. Retain the existing section-name validation while ensuring every specialist has its corresponding placeholder.
73-77: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAnchor roster checks to exact sections and full names.
These assertions search broad bodies using shortened names. A quality-gate line can be removed while the roster still contains the name, and a serial entry can be satisfied by an unrelated occurrence of its first word. Assert exact full specialist names within the relevant section.
Also applies to: 128-132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_review_panel_skill.py` around lines 73 - 77, Update test_every_specialist_has_a_quality_gate and the corresponding assertion at the later roster check to parse the relevant quality-gate and serial sections separately, then require each exact full specialist name (without shortened-name matching) in its expected section. Ensure unrelated occurrences elsewhere in the document cannot satisfy either assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.apm/skills/skillsaw-review-panel/SKILL.md:
- Around line 121-125: Align the Palimpsest corroboration policy across all
three sites: in .apm/skills/skillsaw-review-panel/SKILL.md lines 121-125, define
the permitted specialist roles consistently; in
.apm/skills/skillsaw-review-panel/references/palimpsest.md lines 153-157,
require corroboration of the same factual claim rather than merely the same file
and apply the same role restriction; and in
.apm/skills/skillsaw-review-panel/SKILL.md lines 201-205, make arbiter promotion
follow that identical claim-and-role corroboration policy.
- Around line 168-169: Update the Step 3-to-Step 4 transition wording in the
synchronous dispatch workflow so Step 4 always proceeds after every specialist
has returned, including error or malformed-result responses; ensure the failure
is recorded and the verdict is posted rather than gating on complete findings.
Apply the Markdown change through MarkdownDoc.splice(...), not line-based
replacement.
In `@DEVELOPMENT.md`:
- Line 141: Update the reviewer behavior description near the
specialist-reviewer invocation to distinguish execution contexts: PR reviews
post the structured verdict as a PR comment, while local-branch reviews print
the verdict directly to the user. Preserve the existing reviewer and --serial
details.
---
Nitpick comments:
In `@tests/test_review_panel_skill.py`:
- Around line 91-100: Strengthen test_the_arbiter_settles_candidates and
test_the_reviewer_stays_advisory_by_default to assert the actual Palimpsest
disposition values, including SUGGESTION/NOTE as the default advisory severity,
rather than only checking descriptive marker text or “What NOT to flag.”
- Around line 150-155: Update
TestVerdictTemplate.test_every_specialist_has_a_findings_placeholder to use an
explicit reviewer-to-placeholder mapping and assert each expected findings
token, such as {{PALIMPSEST_FINDINGS}}, is present in verdict-template.md.
Retain the existing section-name validation while ensuring every specialist has
its corresponding placeholder.
- Around line 73-77: Update test_every_specialist_has_a_quality_gate and the
corresponding assertion at the later roster check to parse the relevant
quality-gate and serial sections separately, then require each exact full
specialist name (without shortened-name matching) in its expected section.
Ensure unrelated occurrences elsewhere in the document cannot satisfy either
assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d56278e8-36b8-4485-94d4-15af2bd7d8ee
⛔ Files ignored due to path filters (9)
.agents/skills/skillsaw-review-panel/SKILL.mdis excluded by!.agents/**.agents/skills/skillsaw-review-panel/references/palimpsest.mdis excluded by!.agents/**.agents/skills/skillsaw-review-panel/references/serial-mode.mdis excluded by!.agents/**.agents/skills/skillsaw-review-panel/verdict-template.mdis excluded by!.agents/**.claude/skills/skillsaw-review-panel/SKILL.mdis excluded by!.claude/**.claude/skills/skillsaw-review-panel/references/palimpsest.mdis excluded by!.claude/**.claude/skills/skillsaw-review-panel/references/serial-mode.mdis excluded by!.claude/**.claude/skills/skillsaw-review-panel/verdict-template.mdis excluded by!.claude/**.skillsaw-card.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
.apm/skills/skillsaw-review-panel/SKILL.md.apm/skills/skillsaw-review-panel/references/palimpsest.md.apm/skills/skillsaw-review-panel/references/serial-mode.md.apm/skills/skillsaw-review-panel/verdict-template.mdDEVELOPMENT.mdtests/test_review_panel_skill.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4193ec74ba
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Rename the reviewer to slopinator. |
Addresses the review threads on stbenjam#460. The corroboration rule was stated three times and meant three things: "another specialist", "reaches the same file", and "Architecture or the Technical Writer reached the same claim". All three now say the same thing — any other specialist independently reporting the same underlying defect, not merely a finding in the same file — and a test asserts the narrower phrasings do not come back. `BLOCKING CANDIDATE` was missing from the shared findings format, which is the whole contract an isolated sub-agent receives. It could not emit the label, so the arbiter's settle step could never fire. The label is now in the severity list, marked Palimpsest-only and arbiter-promoted. The Step 4 gate said "proceed only with every specialist's findings", contradicting the retry-record-proceed path below it. It now says not to start while results are still arriving, and that a failed specialist is handled in Step 4 rather than waited on. New fixture `tests/fixtures/palimpsest-review-residue/`: a small repo carrying an instance of every pattern the scope file names, beside clean counterparts the reviewer must leave alone — an earned "why" comment, house style, domain vocabulary, a correctly used em dash, a changelog. The tests pin the fixture to the scope file in both directions, so adding a pattern to one without the other fails. They do not assert the reviewer catches anything: the panel is a prompt, so recall and precision need a model in the loop. Two "what not to flag" rules are properties of the review situation rather than of a file and are declared un-representable rather than quietly dropped. Trimmed SKILL.md back under the 3,000-token skill budget its own linter enforces — the additions above had pushed it to 3,083 — by cutting duplicated statements about sub-agent isolation and scope-file paths. Splitting the disposition criteria under their own heading clears the content-section-length info the arbiter step had introduced. DEVELOPMENT.md no longer claims every invocation posts a PR comment; a local-branch review prints the verdict. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The scope file was itself diff-anchored: it introduced its examples by citing the review they came from, which is the pattern it tells the reviewer to flag. The examples stand on their own without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cec6418fa1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| body = skill_md(SOURCE) | ||
| for name, _ in SPECIALISTS: | ||
| short = name.replace(" Reviewer", "").replace(" Engineer", "") | ||
| assert f"- [ ] {name}" in body or short in body |
There was a problem hiding this comment.
If any quality-gate row is removed, this test still passes because every short value already occurs in the roster or other prose, satisfying the right side of the or without a checklist entry. This leaves the new Palimpsest gate—and every existing gate—without effective regression protection; restrict the assertion to ## Check the Quality Gates and require each expected bullet.
AGENTS.md reference: AGENTS.md:L32-L34
Useful? React with 👍 / 👎.
| def test_every_specialist_has_a_findings_placeholder(self): | ||
| template = (SOURCE / "verdict-template.md").read_text(encoding="utf-8") | ||
| for name, _ in SPECIALISTS: | ||
| assert name in template, f"{name} has no section in the verdict template" |
There was a problem hiding this comment.
If {{PALIMPSEST_FINDINGS}} is removed while its <summary> heading remains, this test still passes even though rendered verdicts can no longer include that specialist's output. Assert a name-to-placeholder mapping, including the exact {{PALIMPSEST_FINDINGS}} token, so the new verdict path has effective regression protection.
AGENTS.md reference: AGENTS.md:L32-L34
Useful? React with 👍 / 👎.
Per review feedback, the reviewer is now the Slopinator. Renames the scope file, the fixture directory, the verdict placeholder, and every reference in the roster, serial mode, and DEVELOPMENT.md. The scope file's opening no longer explains the palimpsest metaphor, which the new name does not carry; it states the residue problem directly instead.
|
Renamed to Slopinator in 5006f28 — scope file ( The scope file used to open by explaining the palimpsest metaphor, which the new name does not carry, so that paragraph now states the residue problem directly instead. On the conflict: #459 landed at 19:19 and is already this branch's merge base, so there is nothing left to resolve. The merged text is the resolution that was suggested — Full suite green (3043 passed), |
Per review feedback: skills are exercised by LLM-enabled evals, not by unit tests asserting on prompt text. Removes tests/test_review_panel_skill.py and the tests/fixtures/slopinator-review-residue/ corpus it was the only consumer of.
not-stbenjam
changed the title
|
You are too careful, slop is blocking. Palmpisest-style comments need to never merge. |
Slop in shipped text is a defect, not a matter of taste, so the Slopinator now sets BLOCKING like every other specialist. Drops the BLOCKING CANDIDATE label, the corroboration requirement, and the arbiter's settle step that existed only to promote it. Restores the palimpsest metaphor to the scope file as an explanation of what the reviewer is looking for, rather than as its name.
|
Slop blocks now, in aef37ac. Dropped the whole advisory apparatus: the Since a false positive now costs more, the restraint rules got firmer rather than looser — clustering is stated as a requirement, Also brought the palimpsest metaphor back into the scope file as the framing for what the reviewer looks for, just not as the name. One thing worth naming: 2965 passed, self-lint A+, all three skill copies identical. |
|
Fair — it was blocking in name only. Fixed in dfd40c6. The gap was the panel-wide line that said the panel "does not gate merge", which meant a Slopinator
Worth flagging so it is not a surprise: the panel posts its verdict with 2965 passed, self-lint back to 6 info violations, the exact |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aef37ac917
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| - `NOTE`: One-line polish — a filler phrase, a Title Case heading, a single | ||
| signature-echo docstring in a file that is otherwise fine. |
There was a problem hiding this comment.
For an otherwise clean change containing one signature-echo docstring or filler phrase, this NOTE rule conflicts with the earlier instruction that any Part A pattern is BLOCKING (lines 150–152), because both cases are listed as Part A patterns at lines 56–60. Since unresolved blocking findings produce REQUEST_CHANGES, the same input can receive opposite panel verdicts depending on which instruction the reviewer follows; explicitly exempt these isolated cases from the blocking rule or assign them one consistent severity.
Useful? React with 👍 / 👎.
2 participants
Motivation
Review conversations leak into shipped code. skillsaw PR #451 merged four
comments that only made sense next to the review thread, and all four had to be
removed afterward:
# Without this a Codex catalog fell through to the single-page renderermain— a reviewer's hypothetical frozen into a permanent claim about history# this was 99.2% of extract_docs runtime on a 180-plugin repository# The direct probes were fixed; the recursive scan was notReview follow-ups, round three/round fourNo existing specialist looks for this. The Technical Writer checks whether docs
are accurate and current; nobody checks whether the writing describes the code
that shipped or the process that produced it.
What this adds
A seventh specialist, the Slopinator. It reads the change as an editor
would, across two halves. A palimpsest — a manuscript whose erased earlier text
still shows through the new writing — is what an AI-authored diff reads like,
and the scope file uses that as its framing. Slop is a defect, so the reviewer
files
BLOCKINGon the same scale as every other specialist.Part A — review-history residue in code. Phantom bugs, round-scoped
benchmark numbers, fix-sequence narration, round-numbered test organization,
change announcements (
# now also handles X), prose restatements of the nextline, defensive over-commenting, signature-echo docstrings, conversational
filler, changelog narration inside a module. The working heuristic is a
stranger test: would this read as a description of the code to someone opening
the file a year from now who has never seen the PR?
Part B — humanizing technical writing. Applied to docs, docstrings, commit
messages and PR prose: AI vocabulary tells, copula avoidance ("serves as",
"boasts"), rule-of-three padding, negative parallelism, signposting, sycophancy,
hedging, em dash and boldface abuse, Title Case headings, manufactured
punchlines, generic conclusions, chatbot artifacts. Rewrite suggestions must
never invent a fact, number, name or rationale that is not in the source or the
code.
Prior art
Part B draws on the humanizer skill by
blader, which catalogues 33 anti-patterns of AI-generated prose, and on
Wikipedia:Signs of AI
writing from
WikiProject AI Cleanup, which humanizer is derived from. Both target
encyclopedic and marketing prose. This scope keeps the patterns that survive the
move to a codebase and drops the ones that do not. Humanizer's §30
"Diff-Anchored Writing" is the closest existing statement of the Part A problem;
this reference file expands it into the code-comment case with the concrete #451
examples. The citation is in the reference file's Reference section.
Calibration — this is not meant to be pedantic
Blocking raises the cost of a false positive, so the scope file is explicit
about restraint:
BLOCKINGcovers slop, and slop does not merge: an unresolved SlopinatorBLOCKINGrules outAPPROVEon its own and holds the verdict atREQUEST_CHANGES. It covers every Part A residue instance (residue isbinary, so one review-history comment is enough), any comment or docstring
that is factually false about the shipped code, and Part B tells
clustered densely enough that a passage reads as generated. Findings must
quote both the offending text and the proposed replacement, so one edit
clears them.
downgraded because the rest of the change is good.
SUGGESTIONandNOTEremain for a lone prose tell and for one-line polish.nothing. It is never a reason to let residue through.
their length, project house style (skillsaw's own files use em dashes and bold
bullet lead-ins throughout), domain vocabulary and terms of art, a single em
dash used correctly, version-scoped documents like changelogs and migration
guides, comments a reviewer explicitly asked for, and existing text a diff
merely moves.
handful and group the rest as one
NOTErather than filing one per instance.existing rule against manufacturing findings.
Changes
references/slopinator.md— new specialist scope fileSKILL.md— roster row,descriptionfrontmatter (6 → 7 specialists), the"6 specialist reviewers + 1 arbiter" line, parallel/serial dispatch counts and
roster order, severity calibration (slop added to
BLOCKING), quality-gatechecklist row, and the slop carve-out from the panel's advisory stance —
disposition criteria and arbiter biases now both state that slop does not
merge
references/serial-mode.md— roster orderverdict-template.md—{{SLOPINATOR_FINDINGS}}sectionDEVELOPMENT.md— reviewer count and list.agents/and.claude/copies regenerated viamake update;make verify-apmconfirms they match the.apm/sourceVerification
make test— 2965 passed, unchanged frommain(this PR ships no testchanges; the panel is a prompt, so its behavior is checked by running it, not
by asserting on its text)
make lint— cleanmake update/make verify-apm— all three skill copies identical.venv/bin/skillsaw lint .— 0 errors, 0 warnings, grade A+🤖 Generated with Claude Code