fix(mt#1339): Reviewer prompt: align out-of-repo path narrative + unify no-tools exception guidance

[minsky-ai]

Summary

Addresses two BLOCKING findings from the mt#1190 PR #763 review that were not resolved before merge. Both are coherence gaps in the reviewer prompt that could cause calibration drift: one in the documented scope of the out-of-repo path detector, and one in contradictory sequencing of a rule and its exception.

Motivation and Context

PR #763 (mt#1190) merged with two BLOCKING review findings unaddressed. Both affect the accuracy of the Critic Constitution prompt delivered to the reviewer model — they are not behavioral defects with measurable impact today, but real honesty/consistency gaps that future calibration rounds will trip on.

Design and Approach

Both fixes are narrative changes only — the regex patterns and classification logic are unchanged.

BLOCKING #1 — absolute_system scope was ambiguous in the prompt text

The prompt listed /etc/..., /usr/..., /var/..., etc. under "Absolute system paths." The trailing etc. (et cetera) was ambiguous: a reader — human or model — might infer /home and /Users are also covered. The fix enumerates the exhaustive list (/etc, /usr, /var, /opt, /tmp, /root) and adds an explicit exclusion note explaining that /home and /Users are NOT included because they are routinely in-repo on developer and CI machines. Those paths are only detected when they contain minsky/sessions/.

BLOCKING #2 — rule and exception were in separate sections

buildInRepoCarveOut(false) said "must be NON-BLOCKING" and pointed forward to a separate section (NO_TOOLS_SECTION) that contained the exception ("may be BLOCKING" for diff-vs-description mismatches). A model applying the strong "must" in the out-of-repo section could miss the exception in the later section. The fix moves the exception inline into buildInRepoCarveOut(false) so the rule and its exception are contiguous. NO_TOOLS_SECTION now back-references the exception rather than re-stating it separately.

Key Changes

• services/reviewer/src/prompt.ts — buildCriticConstitutionFailureModes: enumerate absolute_system paths exhaustively; add explicit note that /home and /Users are excluded
• services/reviewer/src/prompt.ts — buildInRepoCarveOut(false): move the diff-vs-description exception inline; update JSDoc to describe the contiguous-rule rationale
• services/reviewer/src/prompt.ts — NO_TOOLS_SECTION: replace the self-contained exception with a back-reference to the now-inline exception in the out-of-repo section
• services/reviewer/src/prompt.test.ts — update test "no-tools variant out-of-repo section does not say 'may be BLOCKING' before the cross-file section" to assert the new correct structure (exception now IS inline in out-of-repo section); add new test asserting the absolute_system scope is explicitly bounded; add DIFF_VS_DESC_EXCEPTION shared constant to fix magic-string lint warnings

Testing

• 52 tests pass (51 original passing + 1 new)
• 1 test updated to reflect the structural fix (exception is now inline in out-of-repo section, not only in the later no-tools section)
• Pre-commit hooks: lint 0 errors/0 warnings, TypeScript clean

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Findings

[BLOCKING] services/reviewer/src/prompt.ts:156–160 — “Absolute system paths” list claims to be exhaustive but omits many standard system root directories

• Evidence: In buildCriticConstitutionFailureModes, the Out-of-repo references section now says:
  • "Absolute system paths: /etc/..., /usr/..., /var/..., /opt/..., /tmp/..., /root/... (this list is exhaustive — /home/... and /Users/... are NOT included here; …)"
• Issue: Marking this list "exhaustive" is factually incorrect for Unix-like systems and creates a misleading calibration for reviewers. Common absolute system roots such as /bin, /sbin, /lib, /lib64, /dev, /proc, /sys, /run, /mnt, /media, /srv, /boot (among others) are not included. If the intention is to narrow the pre-check scanner, that should be clarified separately from the normative reviewer guidance. As written, the constitution’s narrative asserts that the absolute-system category is limited to those six, which is not true and can cause reviewers to incorrectly treat other absolute system paths as out of scope.
• System-level risk: The detector regex (absolute_system) also only matches the enumerated roots (etc|usr|var|opt|tmp|root). By calling the list “exhaustive,” the prompt implies intent that paths like /bin/sh or /dev/null are not to be treated as out-of-repo, which is at odds with the broader category name and likely the policy intent of this section. Either:
  • remove “this list is exhaustive,” or
  • expand both the narrative and the regex to include other standard system roots, or
  • explicitly justify the rationale for excluding the other roots (and align tests accordingly).
• Concrete location: services/reviewer/src/prompt.ts:156–160.

[NON-BLOCKING] services/reviewer/src/prompt.ts:189–196 — Exception placement in NO_TOOLS_SECTION may still be overlooked due to strong MUST paragraph preceding it

• Evidence: NO_TOOLS_SECTION first states, in bold, that “Any claim … MUST be marked non-blocking… Do NOT mark such claims as BLOCKING…,” and only after that paragraph references the diff-vs-description exception.
• Issue: Although you did move the exception inline into the Out-of-repo section (good), the no-tools section still front-loads an absolute MUST, with the exception mentioned afterwards and not visually emphasized. A linear reader/model could still lock onto the MUST and miss the exception.
• Suggestion: Consider immediately following the MUST paragraph with a clearly marked exception line (e.g., bold “Exception — …”) to mirror the Out-of-repo section’s structure, or tighten cross-referencing (“As noted above in Out-of-repo references: Exception — …”).

[NON-BLOCKING] Narrative consistency around detection vs. policy scope could be clearer

• Evidence: The Out-of-repo references narrative is written as policy guidance, but it also closely mirrors the scanner’s detection patterns (absolute_system, session_workspace). For example, the bullet ties “/home” and “/Users” exclusions to a detection condition (“only when they contain minsky/sessions/”).
• Issue: This blends reviewer policy (what to treat as out-of-repo) with scanner implementation details (what will be auto-inserted into the prompt). If the scanner intentionally under-matches certain classes (e.g., not matching /bin), the reviewer policy should still be general (“Absolute system paths”), while the scanner section can describe its limited detection scope. Otherwise reviewers may infer that non-detected absolute system paths are not policy-relevant.
• Suggestion: Separate “policy examples” from “scanner coverage” explicitly, or add a clarifying note that the bullet list describes the scanner’s current coverage, not the entirety of “absolute system paths” in principle.

[NON-BLOCKING] Minor test brittleness around literal string checks

• Evidence: services/reviewer/src/prompt.test.ts adds DIFF_VS_DESC_EXCEPTION and relies on exact substring presence in sections delimited by headings.
• Issue: Small punctuation or whitespace changes to the headings could cause these tests to fail even if the logical structure remains correct.
• Suggestion: Consider factoring section extraction via regex anchors or utility helpers to be less brittle to formatting changes.

Spec verification table

• Task spec: None provided
• Criteria: N/A

Documentation impact

• Yes. The change materially redefines “Absolute system paths” scope in the Critic Constitution. If the intent is to limit the category to the six enumerated roots, update any related docs or architecture notes that describe out-of-repo detection/policy to reflect the narrower definition and rationale. If not, revise the prompt text to remove “this list is exhaustive” or expand the list.

Out-of-repo references observed in PR description

• [NON-BLOCKING] NEEDS VERIFICATION: out-of-repo path — reviewer cannot verify
  • /etc/ (PR description)
  • /usr/ (PR description)
  • /var/ (PR description)

Conclusion

REQUEST_CHANGES

[minsky-ai]

Review: Reviewer prompt — out-of-repo path narrative + no-tools exception unification

CI status: 2/2 checks passing (build + Prevent Placeholder Tests)

Findings

No blocking or non-blocking issues found.

Checked and clear

services/reviewer/src/prompt.ts

• buildInRepoCarveOut(false) now returns the general NON-BLOCKING rule with the diff-vs-description exception stated inline. The rule and exception are contiguous — the structural contradiction (strong "must" followed by a section-separated "may be BLOCKING") is resolved.
• buildCriticConstitutionFailureModes Out-of-repo bullet now enumerates /etc, /usr, /var, /opt, /tmp, /root explicitly and declares the list exhaustive, with an explicit note that /home/ and /Users/ are NOT in this set — matching the actual absolute_system regex ((?:etc|usr|var|opt|tmp|root)). No phantom coverage claimed.
• session_workspace bullet updated to show both macOS (/Users/.../minsky/sessions/...) and Linux (/home/.../.local/state/minsky/sessions/...) patterns, matching the actual regex.
• NO_TOOLS_SECTION no longer states the exception inline. It back-references the Out-of-repo section instead, eliminating the duplicate. The constant still contains "may be BLOCKING" (in the back-reference sentence), which is correct.
• JSDoc comment on buildInRepoCarveOut updated to match the new structure — no stale description of the old "handled separately in NO_TOOLS_SECTION" approach.
• OUT_OF_REPO_PATH_PATTERNS regex unchanged — the code fix is purely prose/documentation, which is the correct scope for option (b) chosen by the spec.

services/reviewer/src/prompt.test.ts

• New test (absolute_system scope is explicitly bounded): Asserts /etc/, /opt/, /tmp/, /root/ are present; asserts /home/ and /Users/ are present in the prose; asserts "NOT included" appears. All assertions are satisfiable by the new bullet text. Valid coverage of BLOCKING #1.
• Updated test (previously: "does not say 'may be BLOCKING' before the cross-file section"; now: "includes the diff-vs-description exception inline"): Old assertion not.toContain("may be BLOCKING") on the Out-of-repo section would fail against new code (correct to replace). New assertions check the section DOES contain DIFF_VS_DESC_EXCEPTION and "may be BLOCKING". These are satisfied by the new buildInRepoCarveOut(false) text embedded in the Out-of-repo section.
• DIFF_VS_DESC_EXCEPTION constant: Introduced in the test file and used in two tests. No duplicate string literals remain for the exception phrase in the test file — the constant centralizes it.
• describe("no-tools in-repo diff-vs-description exception clause"): Both tests updated to use DIFF_VS_DESC_EXCEPTION constant. The not.toContain test for the tools variant remains correct — buildInRepoCarveOut(true) does not include the Exception text.
• Existing tests for extractOutOfRepoReferences (lines 193-313) and buildReviewPrompt (lines 316-405) are unchanged and remain valid — the regex was not touched.

Documentation: services/reviewer/README.md contains no enumeration of absolute system paths or /home//Users references — no documentation drift created by this PR.

Spec verification

Task: mt#1339

Criterion	Status	Evidence
PR description / docs and code/tests agree on which paths are out-of-repo	Met	buildCriticConstitutionFailureModes now names /etc, /usr, /var, /opt, /tmp, /root exhaustively and explicitly excludes /home and /Users — matching the actual absolute_system regex. Tests assert both the enumeration and the exclusion note.
No-tools-variant guidance is internally consistent on in-repo claimed-but-not-in-diff findings	Met	Exception moved inline into buildInRepoCarveOut(false) so rule and exception are contiguous. NO_TOOLS_SECTION back-references rather than re-stating it. No contradictory "must" / "may be" split across sections.
Existing prompt.test.ts tests continue passing (or updated alongside chosen scope)	Met	Old test whose assertion would flip (the not.toContain("may be BLOCKING") on the Out-of-repo section) is replaced with the correct positive assertion. Unchanged tests remain valid against unchanged behavior.

Documentation impact

No update needed — the change is a documentation-only fix to the prompt prose and its tests. services/reviewer/README.md describes tool-access behavior and the behavioral contract of buildCriticConstitution at a high level; it does not enumerate the specific absolute-path prefixes and does not reference the no-tools exception structure, so no README update is required.

(Had Claude look into this — AI-assisted review via minsky-reviewer Chinese-wall protocol)