Skip to content

feat(review): shorten and simplify review comments - #39

Merged
zfarrell merged 2 commits into
mainfrom
feat/review-comment-style
Aug 29, 2026
Merged

feat(review): shorten and simplify review comments#39
zfarrell merged 2 commits into
mainfrom
feat/review-comment-style

Conversation

@zfarrell

@zfarrell zfarrell commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Adds a ## Comment Style section telling the reviewer to write its comments in simplified technical English, one finding per comment, with the proof scaled to severity while the consequence is always stated.

Measured by rewriting 14 real posted comments from this repo (#38) and runtimedb (#1242, #1236) under each candidate ruleset:

Ruleset Total prose words
As posted today 2,338
Sentence caps + <details> folds −2%
Proof-scaling, first draft −60%
This PR −46%

Two earlier approaches were dropped with cause. Sentence caps failed three independent ways (a 4-sentence super nit cannot hold claim + evidence + consequence + fix; the count is gameable, since a fix costs 0 sentences as code and 1 as prose; and the word cap and sentence cap compete). <details> folds cut only 2% of real volume — they relocate text, and folded text returns whole through <prior_review_comments> on later cycles — while hiding scope-limiting facts, which made comments read more severe than the originals. A first draft of proof-scaling coupled proof to convention-existence and dropped the consequence on the two highest-impact findings in the set; that clause is gone and the consequence is now unconditional.

Known limits, worth watching rather than fixing blind:

  • Severity tiers conflate likelihood with blast radius, so an improbable-but-catastrophic nit gets the thinnest argument. The consequence is stated, so the gap is persuasiveness, not information.
  • Absence-claims ("X is not what this code does") structurally need two cited facts where a nit allows one.
  • One test comment lost its scope-limiting preconditions and read slightly more severe; the two reviewers read that rule differently.

The prompt document is measured at runtime and subtracted from the diff's context allowance, so this section is sized deliberately: 1,193 bytes, leaving 107 before tests/context-step-test.sh starts omitting diffs that currently fit.

Deploys org-wide from main on merge. Rollback is a one-file revert.

@zfarrell
zfarrell requested a review from a team as a code owner August 29, 2026 17:51
@zfarrell
zfarrell requested review from rohan-hotdata and removed request for a team August 29, 2026 17:51
Comment thread docs/claude-pr-review-prompt.md Outdated
Comment thread docs/claude-pr-review-prompt.md
claude[bot]
claude Bot previously approved these changes Aug 29, 2026

@pullfrog pullfrog 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.

Important

The one-finding rule conflicts with required blocking summaries and should be scoped before merge.

Reviewed changes Reviewed the new organization-wide review-comment style contract and its interaction with the existing output rules.

  • Controlled language — Adds sentence length, voice, terminology, uncertainty, tense, and pronoun constraints for inline and summary comments.

  • Comment structure — Requires claim-first feedback, fix-before-derivation ordering, and one finding per comment.

  • Severity-scaled evidence — Requires every finding to state its consequence while varying supporting proof by severity.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using GPT Sol𝕏

Comment thread docs/claude-pr-review-prompt.md Outdated

@claude claude 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.

Prior nits are addressed: line 155 scopes the one-finding rule to inline comments, and line 149 exempts the context disclosures. No new findings.

CI has not reported yet, so the prompt-document byte budget in tests/context-step-test.sh is unverified here.

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes Reviewed the prompt-contract fixes added since the prior Pullfrog review.

  • Inline finding scope — Limited the one-finding rule to inline comments, preserving the required aggregate blocking summary.
  • Context disclosures — Exempted mandatory partial-context disclosures from the subject-deletion rule.
  • Proof wording — Simplified the blocking-proof instruction without changing its requirement.

Pullfrog  | View workflow run | Using GPT Sol𝕏

@zfarrell
zfarrell merged commit 5f4dccf into main Aug 29, 2026
4 checks passed
@zfarrell
zfarrell deleted the feat/review-comment-style branch August 29, 2026 21:42
zfarrell added a commit that referenced this pull request Aug 29, 2026
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