feat(mt#1270): tighten reviewer countChangedLines header regex and add listFiles truncation safeguard#840
Merged
Conversation
…es truncation safeguard Two reliability fixes to the mt#1188 reviewer scope classifier: 1. countChangedLines now uses anchored regex on the unified-diff file-header form (+++ a/, +++ b/, +++ /dev/null and the --- equivalents) instead of startsWith(+++)/startsWith(---). Content lines whose payload itself starts with +++/--- (Hugo frontmatter, fenced metadata, ASCII rules) are now counted correctly. Previously they would render as ++++/---- in the diff and get skipped, undercounting and mis-classifying frontmatter-heavy content as trivial. 2. classifyPRScope accepts an optional changedFilesCount sourced from pr.changed_files. When the listFiles fetch returns fewer files than the API count (cap exceeded, error fallback, pagination edge), the classifier downgrades to normal rather than classify on a partial view. Defense-in- depth on top of the existing pagination + 1000-file cap in fetchListFiles. Tests: 12 new (5 header-anchor + 7 truncation), all 402 reviewer-service tests pass.
![minsky-reviewer[bot]](https://avatars.githubusercontent.com/u/52695?s=60&v=4){kind=small}
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown
reviewer-service error: chain-of-thought leakage detected
The upstream model emitted raw internal reasoning into the review body. The reviewer service sanitised the output but could not locate a valid Findings section to preserve, so the leaked content was discarded. The PR will receive a fresh review on the next commit. See docs/architecture/critic-constitution-reliability.md for details.
Contributor
Author
There was a problem hiding this comment.
Review: tighten countChangedLines header regex and add listFiles truncation safeguard
CI status: 2/2 checks passed (Prevent Placeholder Tests, build)
Findings
No blocking or non-blocking issues found.
Checked and clear
services/reviewer/src/pr-scope.ts—ADDED_HEADER_RE/REMOVED_HEADER_REregex traced manually against all cases:+++ a/path,+++ b/path,+++ /dev/null,----,+++data,+++ ---. All cases resolve correctly. Truncation safeguard sits at correct position in the precedence chain (after opt-out marker, after empty-files, before pattern checks).services/reviewer/src/github-client.ts—changedFilesCount: numberadded as required field. Only one construction site (fetchPullRequestContextat line 172), which includespr.changed_files. Verified no other inlinePullRequestContextobject literals exist in the codebase (all other usages arePick<>narrowings that are unaffected).services/reviewer/src/review-worker.ts—changedFilesCount: pr.changedFilesCountthreaded intoclassifyPRScopecall. No structural changes.services/reviewer/src/pr-scope.test.ts— 12 new tests verified sharp: the 3 header-anchoring tests (++++× 12,----× 12, spec-diff × 3) produce counts of 0 / 0 / 6 under old logic (all classify trivial or below threshold) and 12 / 12 / 15 under new logic (all cross the 10-line threshold intonormal). The 7 truncation tests cover the full decision matrix including back-compat (nochangedFilesCountfield), opt-out marker precedence, empty-files interaction, and the defense-in-depth case wherefilesChanged.length > changedFilesCount.
Acceptance test cross-check (spec criterion verbatim):
countChangedLines("+++ ---\n+content\n+++data\n-removed\n---data")→ 5 under new logic (traced: all 5 lines match eitherstartsWith('+')orstartsWith('-')and none matchADDED_HEADER_RE/REMOVED_HEADER_RE). Under old logic: 2 (lines 1, 3, 5 skipped viastartsWith("+++")/startsWith("---")). Criterion met.- PR with
changed_files: 500,filesChanged.length: 300→normal. Criterion met; covered by test at line 335 of the new test suite.
Out-of-scope items confirmed untouched: DOCS_FILE_PATTERN, TEST_FILE_PATTERN, prompt construction, full listFiles pagination logic.
Spec verification
Task: mt#1270
| Criterion | Status | Evidence |
|---|---|---|
countChangedLines correctly counts added/removed lines whose content starts with +++/--- (tightened regex anchoring) |
Met | ADDED_HEADER_RE = /^\+\+\+ (?:[ab]\/|\/dev\/null)/ in pr-scope.ts; verified by manual regex trace and 5 new sharp tests |
classifyPRScope falls back to normal when filesChanged.length < pr.changed_files |
Met | Truncation guard at pr-scope.ts (post-PR ~line 103); changedFilesCount surfaced in github-client.ts and threaded in review-worker.ts |
| New tests for both edge cases | Met | 5 header-anchoring tests + 7 truncation tests in pr-scope.test.ts |
No unmet criteria. No criteria deferred.
Documentation impact
No update needed — this is a pure bugfix and defense-in-depth safeguard to an internal classifier. The classifier is not documented in docs/architecture.md or user-facing docs; it is internal to the reviewer service (mt#1188 lineage). No documented behavior changes.
(Had Claude look into this — AI-assisted Chinese-wall review, reading actual source files to verify each finding independently of the PR description.)
edobry
added a commit
that referenced
this pull request
Apr 27, 2026
…onse and fix misleading status log
## Summary
`session_pr_create` previously returned `{success: true}` even when the post-PR-create IN-REVIEW transition was skipped or failed silently — the only signals were a `log.warn` and a misleading `log.cli("Updated task ... status to IN-REVIEW")` that fired unconditionally. Callers had to follow up with a separate `tasks_status_get` to verify the transition (which mt#1270's `/verify-task` flow had to do manually after PR #840).
This PR adds a structured `statusTransition` receipt to the response and routes all five outcomes through it, so callers can disambiguate success, skip, and failure without out-of-band checks.
## Key Changes
- **New helper `applyInReviewTransition(noStatusUpdate, taskId, taskService)`** in `src/domain/session/session-pr-operations.ts`. Pure, exported, unit-testable. Returns `StatusTransitionReceipt { from, to, succeeded, reason? }`.
- **Five-case taxonomy** covered by the receipt:
- `succeeded=true` — write happened, `from` carries pre-transition status
- `succeeded=false, reason="skipped: noStatusUpdate"` — caller opt-out
- `succeeded=false, reason="skipped: no taskId on session"` — session not task-bound
- `succeeded=false, reason="skipped: no taskService in deps"` — DI gap
- `succeeded=false, reason="setTaskStatus threw: <msg>"` — write attempted and failed; PR still created
- **Pre-transition `getTaskStatus` read** so the receipt's `from` field reflects the actual prior state. A read failure is non-fatal — we still attempt the write and the receipt carries `from=null`.
- **Fix misleading log**: moved `log.cli("Updated task ... status to IN-REVIEW")` inside the success branch. Pre-fix it fired even after the `else` branch logged "No taskService in deps — skipping..." — a textbook lying log line.
- **Thread receipt through types**: `StatusTransitionReceipt` is now a required field on `SessionPrResult` (`src/domain/session/types.ts`) and on the `sessionPrCreate` return type (`src/domain/session/commands/pr-create-subcommand.ts`). The MCP adapter (`src/adapters/shared/commands/session/pr-create-command.ts:308`) spreads `result` via `...rest`, so the field flows to the wire format with no adapter changes.
## Testing
- **12 new unit tests** in `src/domain/session/session-pr-status-transition.test.ts`:
- Successful transition with from/to/succeeded shape
- Read-failure non-fatal behavior (write still attempted)
- Each of the three skip cases with reason-pattern assertions
- Three taskId-edge variants (undefined/null/empty-string)
- setTaskStatus-throw surfaced in receipt + does-not-throw guarantee
- Precedence ordering (`noStatusUpdate` beats missing taskService; missing taskId beats missing taskService)
- **Source-text regression guard**: asserts there's exactly one occurrence of the success log line in the file, and that the most recent control-flow event before it is a `setTaskStatus` call (not a `log.warn`).
- **All 316 session tests pass** (12 new + 304 pre-existing, no regressions)
- **Typecheck**: 0 errors
- **Lint**: 0 errors / 0 warnings across 1072 files
- **Format**: clean
## Out of scope
- Generalized state-receipt redesign across all session_* tools (covered separately by `feedback_state_mutations_need_verifiable_receipts`; this is the specific session_pr_create instance)
- The pre-push origin-ahead safety guard (mt#1304, PR #842 — different file)
- The `name: sessionId` parameter shape (mt#1261/PR #777, marked SUPERSEDED — different code path)
Closes mt#1378.
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two reliability improvements to the mt#1188 PR scope classifier (
services/reviewer/src/pr-scope.ts), both flagged by the reviewer bot on PR #767.Header-anchor regex for
countChangedLines— replacestartsWith("+++")/startsWith("---")with anchored regex on the actual unified-diff file-header form (+++ a/,+++ b/,+++ /dev/nulland the---equivalents). Content lines whose payload itself starts with+++/---(Hugo frontmatter delimiters, fenced metadata, ASCII rules) are now counted correctly. Previously they rendered as++++/----in the diff and were skipped — undercounting line totals and mis-classifying frontmatter-heavy PRs astrivial.listFiles truncation safeguard —
PullRequestContextnow carrieschangedFilesCount: numbersourced frompr.changed_files.classifyPRScopeaccepts an optionalchangedFilesCountand falls through tonormalwhenfilesChanged.length < changedFilesCount. This catches cap-exceeded (>1000 files) and error-fallback cases wherefetchListFilesreturns[], plus any future pagination edge where the partial view would mis-classify a PR whose later pages contain code asdocs-only/test-only.The spec's
per_page: 300description was already obsolete — the file fetcher (mt#1188 follow-up) already paginates withper_page: 100and a 1000-file cap. The new check is defense-in-depth on top of that.Key Changes
services/reviewer/src/pr-scope.ts:ADDED_HEADER_RE/REMOVED_HEADER_REconstants with/^\+\+\+ (?:[ab]\/|\/dev\/null)/anchoring.countChangedLinesnow uses regex test instead ofstartsWithprefix check.classifyPRScopeinput gains optionalchangedFilesCount; truncation check returnsnormalwhen partial view is detected (sits between empty-files early return and pattern checks).services/reviewer/src/github-client.ts:PullRequestContextgains requiredchangedFilesCountfield, populated frompr.changed_files.services/reviewer/src/review-worker.ts: threadspr.changedFilesCountinto theclassifyPRScopecall.Testing
pr-scope.test.ts:countChangedLines header anchoring (mt#1270)— frontmatter++++/----payloads, the spec's exact synthetic diff, real-header skip regression guard,/dev/nullheader skip.pagination/truncation safeguard (mt#1270)— docs/test downgrade-to-normal, exact match, length-greater (no downgrade), back-compat without the field, opt-out marker precedence, empty-list interaction.Closes mt#1270.