Skip to content

fix(pre-merge): judge whole PR; don't fail unverifiable checks - #184

Merged
mk7luke merged 3 commits into
mainfrom
pre-merge-checks
Oct 8, 2026
Merged

mk7luke merged 3 commits into
mainfrom
pre-merge-checks

Conversation

@mk7luke

@mk7luke mk7luke commented Oct 8, 2026 •

Copy link
Copy Markdown
Owner

What

DiffSentry / Pre-Merge was turning red on PRs because of checks that had nothing to evaluate. One example is Interact-Event-Productions/atlas#361. Its two error-mode custom checks reported "Cannot verify — files not shown", and that counted as a failure. The ship check then reported the PR as blocked by DiffSentry's own status.

Why it happened

  1. Wrong input. Checks ran against the incremental slice: 6 of 48 files on #361. The walkthrough and drift detection already use the full PR file set; pre-merge checks had been missed.
  2. No "can't tell" answer. The prompt only allowed pass or fail, so "doesn't apply" and "can't see it" came back as failures. An unparseable model reply silently counted as a pass.
  3. No detail. The status said "Pre-merge checks failed" without naming the check, and the ship check listed only the status name.

Changes

  • Full PR input: checks now see the full PR file set (walkthroughContext). The model is told which files are in the PR but not shown to it: ignored, over the file cap, path-filtered, or omitted by the size budget.
  • Three outcomes: pass, fail, or inconclusive.
    • "Fail" requires concrete evidence.
    • A check that doesn't apply passes as "Not applicable".
    • Inconclusive checks are listed separately as "Couldn't verify" and never fail the status.
    • An unreadable model reply is now inconclusive.
  • Status names the check: e.g. Failed: alembic-revision-id-length, or Passed with 1 warning: PR Title. Capped at 140 characters.
  • Ship check: each failing-status blocker now includes that status's description.
  • Smaller changes:
    • Model messages are escaped so they can't break the results table.
    • Custom checks run in parallel.
    • The resolution column, which repeated the same text on every row, is gone.

Verification

  • tsc --noEmit is clean.
  • New tests/unit/pre-merge.test.ts covers the #361 case, along with parsing, the status description, and rendering. ship-check.test.ts still passes: 49/49 across both files.
  • Full suite: 7 failures in parity-corpus, parity-fixture, learnings-provenance and thread-resolved-note. The same 7 fail on unmodified main on Windows, so this change didn't cause them.
  • Not yet run against a live PR.

Summary

Pre-merge checks were updated to evaluate the full PR and distinguish evidenced failures from inconclusive evaluations. Status and ship-check output now identify the specific checks responsible for warnings or blockers.

Changes

File Changes
src/pre-merge.ts Replaced boolean check results with typed pass, failed, and inconclusive outcomes; added robust AI prompt and response parsing; parallelized custom checks; improved markdown escaping and rendering; and added bounded, check-specific status descriptions.
src/reviewer.ts Ran pre-merge checks against the full walkthrough context, supplied the AI with unavailable-file metadata, and published descriptive commit-status messages.
src/ship-check.ts Included failing commit-status descriptions alongside status contexts in ship-check blocker messages.
src/types.ts Introduced the CheckOutcome union and migrated CheckResult from a boolean passed flag to an outcome field.
tests/unit/pre-merge.test.ts Added unit coverage for outcome parsing, unavailable-file prompts, non-blocking inconclusive checks, status descriptions, markdown escaping, and check execution behavior.

… see their files

- Run pre-merge checks against the full PR file set (same as walkthrough and
  drift), not the incremental slice, and name the files the model can't see.
- Add an "inconclusive" outcome: a check that applies but whose evidence isn't
  visible no longer fails the `DiffSentry / Pre-Merge` status. Checks that don't
  apply pass as "Not applicable". Unreadable model answers are inconclusive
  instead of a silent pass.
- Status description names the failing checks; ship check shows each failing
  status's description.
- Escape table cells; run custom checks concurrently.
@diffsentry

diffsentry Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

✅ DiffSentry has completed the review — Looks good!

@diffsentry

diffsentry Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Pre-merge checks were updated to evaluate the full PR and distinguish evidenced failures from inconclusive evaluations. Status and ship-check output now identify the specific checks responsible for warnings or blockers.

Changes

Pre-merge evaluation

Layer / File(s) Summary
Outcome Model And Rendering
src/pre-merge.ts, src/types.ts
Pre-merge checks now produce passed, failed, or inconclusive outcomes, with only evidenced error-mode failures blocking merges. The results UI separates unverifiable checks from failures, safely escapes model content, and creates concise GitHub status descriptions.
Full PR Check Execution
src/reviewer.ts
Checks now run against the complete walkthrough PR context rather than the current incremental diff slice. The AI is explicitly told which PR files were unavailable so missing evidence produces an inconclusive result.
Regression Coverage
tests/unit/pre-merge.test.ts
New tests cover the prior false-blocking scenario, response parsing safeguards, status semantics, markdown formatting, and propagation of named check outcomes.

Status reporting

Layer / File(s) Summary
Ship Blocker Details
src/ship-check.ts
Ship-check blockers now include the associated commit-status description, exposing the failing check details directly in the blocker message.

Sequence Diagram(s)

sequenceDiagram
    participant Reviewer
    participant AI
    participant GitHub
    Reviewer->>AI: Send full PR context and check prompt
    AI-->>Reviewer: pass fail or inconclusive JSON
    Reviewer->>Reviewer: Render results and derive status
    Reviewer->>GitHub: Publish named pre-merge status
Loading
sequenceDiagram
    participant Ship
    participant GitHub
    participant Author
    Ship->>GitHub: Read commit statuses
    GitHub-->>Ship: Context and description
    Ship-->>Author: Show blocker with check details
Loading

Change: Bug fix

Estimated code review effort: 🎯 3 (Moderate) | ⏱️ ~30 minutes

Suggested labels: bug, enhancement, test

Risk factors — 10/100, 🟢 Low

Factor Weight Detail
Major findings +5 1 major issue flagged
Moderate change +5 426 lines changed across 5 files

Test coverage signal

🟢 183 prod / 128 test lines added.

Source files changed Test files changed Source lines + Test lines +
4 1 183 128

✍️ Commit message coach

1 of 3 commit messages could be stronger.

Commit Subject Issues
7b422f3 🟡 fix(pre-merge): judge the whole PR and stop failing checks that can't see their files Subject is 85 characters — keep under 72 to avoid truncation.

Tip: imperative mood (Add user lookup), under 72 chars, no trailing period. Conventional Commits like feat: / fix: are also fine.

📊 Confidence breakdown

1 finding — 🟢 0 high · 🟡 1 medium · 🔴 0 low. Treat the medium / low ones as hypotheses to verify.

Merge Risk: 🟢 Low · 10/100 · up to 77ee277

Driven by Major findings (+5) and Moderate change (+5). Full breakdown in the walkthrough.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Schema bump ⚠️ Warning Cannot verify this check because src/storage/db.ts is not among the shown diffs. Address before merging or downgrade to non-blocking.
Provider parity ⚠️ Warning Cannot verify: the provided PR diff does not include src/ai/anthropic.ts, src/ai/openai.ts, or src/ai/openai-compatible.ts. No concrete contract change or corresponding updates are visible. Address before merging or downgrade to non-blocking.
✅ Passed checks (3 passed)
Check name Status Explanation
PR Title ✅ Passed The title uses an allowed Conventional Commits prefix; the text after it begins with the imperative verb “judge”, is under 72 characters, and has no trailing period.
PR Description ✅ Passed The description clearly explains what changed (full-PR pre-merge input, three check outcomes, improved status/ship-check detail) and why (unverifiable checks were incorrectly failing PRs). It also links the motivating related PR as Interact-Event-Productions/atlas#361.
Pattern test coverage ✅ Passed Not applicable — this PR does not add rules to src/safety-scanner.ts or src/pattern-checks.ts.

✏️ Tip: You can configure your own custom pre-merge checks in your .diffsentry.yaml.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pre-merge-checks
📝 Generate docstrings (beta)
  • Create stacked PR
  • Commit on current branch
🧹 Simplify (beta)
  • Create PR with simplified code
  • Commit simplified code in branch pre-merge-checks
🪄 Autofix unresolved comments (beta)
  • Create stacked PR
  • Commit on current branch

Comment @diffsentry help to get the list of available commands and usage tips.

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

🛡️ This DiffSentry review has been superseded by a newer one. Its findings, if still present, appear there.

Comment thread src/pre-merge.ts Resolved
@diffsentry

diffsentry Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

📌 Status — last updated 77ee277

🔴 Changes requested

Reflects the PR's open threads, not just the latest pass: 1 unresolved blocking finding still open.

Risk score 10/100 (Low) ▁█▄
Unresolved threads 1 (1 blocking)
Failing checks 0
Pending checks 1
Files reviewed 2 (3 skipped)
Updated 2026-10-08 23:31Z

Live-updated by DiffSentry on every push. Use @diffsentry ship for a verdict, @diffsentry timeline for full history.

Comment thread src/pre-merge.ts Fixed
cell() escaped `|` but not `\`, so an input `\|` rendered as `\|` — an
escaped backslash followed by a bare pipe, which still split the cell
(CodeQL js/incomplete-sanitization). Escape backslashes first.

Check names come from repo config and went into the tables raw; a name
with `|` or a newline broke the row. Run them through cell() too.
@mk7luke mk7luke changed the title fix(pre-merge): judge the whole PR and stop failing checks that can't see their files fix(pre-merge): judge whole PR; don't fail unverifiable checks Oct 8, 2026
@mk7luke

mk7luke commented Oct 8, 2026

Copy link
Copy Markdown
Owner Author

Title coach (walkthrough, no fingerprint): the PR title was 85 characters. Fixed: shortened to fix(pre-merge): judge whole PR; don't fail unverifiable checks (63 chars).

The commit-message coach flagged the same 85-char subject on 7b422f3. Not changing it: rewriting a pushed commit isn't worth a force-push, and the PR will squash-merge under the new title.

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

🛡️ This DiffSentry review has been superseded by a newer one. Its findings, if still present, appear there.

Comment thread src/pre-merge.ts Resolved
Comment thread src/pre-merge.ts Resolved
A fail with an empty message cites no evidence; treat it (and a bare legacy
{"passed": false}) as inconclusive so it can't turn the status red on
nothing. Also pin that title/description checks set to `off` emit no result.

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

Actionable comments posted: 1

🪄 Autofix (Beta)

Fix all unresolved DiffSentry comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: .diffsentry.yaml

Review profile: ASSERTIVE

Run ID: b88e3400-528c-408e-b25d-ad1edfe7eea4

📥 Commits

Reviewing files that changed from b35c21c to 77ee277. Previously-reviewed commits are not re-reviewed.

📒 Files selected for processing (2)
  • src/pre-merge.ts
  • tests/unit/pre-merge.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/reviewer.ts
  • src/ship-check.ts
  • src/types.ts

Comment thread src/pre-merge.ts Resolved
@diffsentry

diffsentry Bot commented Oct 8, 2026

Copy link
Copy Markdown

Release Notes

New features

  • DiffSentry / Pre-Merge now reports checks as passed, failed, or inconclusive, and shows inconclusive checks separately without treating them as failures.
  • DiffSentry / Pre-Merge status descriptions now name failed checks or warning checks, and the ship check includes failing status descriptions in its blocker list.

Improvements

  • Custom checks under reviews.pre_merge_checks now evaluate the full PR file set rather than only the current push's changed-file slice.
  • Pre-merge check prompts now identify PR files whose diffs were unavailable, so checks can report "inconclusive" when required evidence is not visible.
  • Pre-merge results escape check names and model messages in markdown tables, preventing pipes, backslashes, and line breaks from corrupting the output.
  • Custom pre-merge checks now run concurrently.

Bug fixes

  • Fixed error-mode custom checks failing DiffSentry / Pre-Merge when the relevant files were omitted, ignored, capped, or filtered and the check could not be verified.
  • Fixed unreadable model responses being treated as passed by DiffSentry / Pre-Merge; they now appear as inconclusive.

Drafted automatically once the review of 77ee277 came back green, every check passed and every review thread was resolved, and rewritten in place on later commits. Re-run by hand with @diffsentry release-notes, or set release_notes.auto: false in .diffsentry.yaml to stop.

@mk7luke
mk7luke merged commit 3f1f0cd into main Oct 8, 2026
18 checks passed
@mk7luke
mk7luke deleted the pre-merge-checks branch October 8, 2026 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants