fix(#3691): let every reviewer lane take a prompt cap, and make the documented global resolve (#3832)

* test(#3691): failing-first coverage for the reviewer prompt budget No prompt cap can reach any CLI reviewer lane, by any configuration. Two independent defects compound: all nine `transport: spawn` lanes declare `promptBudgetKey: null`, so `budgetFor` returns on its first line; and the documented global `review.max_prompt_tokens` is advertised in the schema manifest but declared nowhere, so the resolver never materializes it and `budgetFor`'s fallback is dead code. Adds to tests/reviewer-config-federation.test.cjs, which already owns the per-reviewer budget config-set/config-get idiom: - a CLI lane inherits the global cap (RED: reports null) - an http lane with the -1 sentinel inherits the global cap (RED: reports null) - the resolved review surface carries max_prompt_tokens at all (RED: absent) - per-lane overrides the global on a CLI lane - the sentinel boundary: -1 inherits, 0 means do-not-trim and must NOT read as unset, 1 is the smallest real budget — the regression budgetFor's own comment warns about - anti-tightening pins that must stay green: an empty config leaves every lane null, the three existing budgeted lanes are unchanged, and config-set still rejects a per-reviewer key naming something that is not a declared lane - a fast-check property over the resolution contract itself, with -1, 0 and non-finite inputs generated explicitly rather than left to chance Every row was reproduced by hand against the real CLI before being written, so the RED/GREEN split is observed rather than predicted. Refs #3691 * fix(#3691): let every reviewer lane take a prompt cap, and make the global resolve No prompt cap could reach any CLI reviewer lane, by any configuration. Two independent defects compounded. The nine spawn-transport lanes — claude, coderabbit, antigravity, cursor, gemini, codex, kimi-code, opencode, qwen — declared `promptBudgetKey: null`, so `budgetFor` returned on its first line and `review-lane plan` reported `promptBudget: null` no matter what was configured. Each now declares `review.max_prompt_tokens_per_reviewer.<slug>` with the same `-1`-is-unset sentinel the three local-server lanes already use. Separately, the central `review.max_prompt_tokens` was listed in the schema manifest's validKeys and documented as a supported setting, but declared nowhere — the resolved surface is built from capability declarations plus the defaults manifest, and neither carried it. `configGet` returned undefined and `budgetFor`'s documented fallback was dead code. It is now declared with a `null` default, exactly as docs/CONFIGURATION.md already specified, so the default behavior is unchanged: nothing configured means nothing trims. Two things the diagnosis had not predicted, found and fixed while implementing: - `REVIEWER_LANES` in src/review-lane-descriptor.cts is a second, hardcoded registration site that `mergeReviewerLanes` prefers over the capability registry on a slug collision. Editing only the capability files left every CLI lane still null. Both sites now agree. - The generated `gsd-core/bin/lib/capability-registry.cjs` was stale and masked the capability edits; regenerated with `npm run gen:capability-registry` rather than hand-edited. docs/CONFIGURATION.md said "Only lanes that declare a budget key accept one — today ollama, lm_studio and llama_cpp". That is false as of this change and is corrected rather than left to rot. The trim-versus-refuse question the issue raises is deliberately not taken up here: the refusal path already exists for the case that matters — a reviewer whose minimum set exceeds its budget is skipped rather than sent a misleading prompt — and trimming above that floor is the documented, shipped design of the feature. Changing it would alter behavior for the three lanes that already work, which is not what the issue asks for. Fixes #3691 * fix(#3691): document the new global and narrow an invariant this change obsoleted The full suite surfaced two consequences of giving every CLI lane a budget key. `review.max_prompt_tokens` entered CONFIG_DEFAULTS without a matching entry in the planning-config reference, which config-field-docs guards. Documented, including the sentinel semantics a reader needs: a per-lane value overrides the global, `-1` means unset and inherits it, and `0` means "do not trim that lane" and is not unset. The #2797 federation guard asserted that "a lane with no model flag and no host owns no config keys". That held only because budget keys existed solely on the three local-server lanes, all of which have hosts. A lane can now legitimately own a config key for a third reason, so qwen tripped it. The assertion is narrowed rather than weakened: such a lane must still own no model key and no host key, and may own at most its own `review.max_prompt_tokens_per_reviewer.<slug>` — never another lane's. That is strictly more specific in the dimensions that still matter. Proven to still bite: hypothetically giving qwen a `review.models.qwen` key fails it with `model/host: review.models.qwen`. The name and comment cite #3691 for why the premise changed, so a reader sees a deliberate narrowing, not erosion. Checked the sibling assertions in that describe block; the other three do not rest on the obsolete premise and are untouched. Refs #3691 * fix(#3685): port the write-flag content-change contract to its three sibling sites #3685 fixed `phase complete`'s `roadmap_updated` / `state_updated`, which reported `fs.existsSync(path)` rather than whether the transaction wrote anything. Three sibling sites carried the identical defect and are ported here. - `cmdPhaseRemove` reported `roadmap_updated: true`, hardcoded. `updateRoadmapAfterPhaseRemoval` now returns whether the content changed and the flag reports it. #2640/#2974 already fixed `state_updated` at this same call site and left this one behind, so the correct shape was adjacent. - `cmdMilestoneComplete` reported `state_updated: fs.existsSync(statePath)` — byte-identical to #3685's bug in a different command. - `cmdMilestoneComplete` reported `milestones_updated: true`, hardcoded, never consulting the MILESTONES.md write. `gsd-core/workflows/remove-phase.md:100` extracts `roadmap_updated` for display and never branches on it, so the flip from always-true to content-based changes no workflow behavior. Verified by reading the step, not assumed. One trap found while implementing: the obvious in-memory `finalContent !== originalStateContent` comparison — copying `cmdPhaseComplete`'s shipped shape verbatim — gives a FALSE POSITIVE for milestone completion. `platformWriteSync` normalizes Markdown at write time, and the milestone-closure transform regenerates `## Current Position` fresh on every call, so its pre-normalize output always differs from the already-normalized file on disk even when the persisted bytes are identical. The comparison is therefore made against the post-write on-disk content. `cmdPhaseComplete`'s own comparisons are left untouched — their repeat-no-op tests pass, so they are not exposed to this artifact. `milestones_updated` has no reachable no-op: the MILESTONES.md write unconditionally appends an entry every call. Only the true direction is pinned, documented inline rather than faked with a passing test. Refs #3685 * fix(#3685): compare write-flag content through the writer's own normalizer An independent reviewer disproved a claim made while porting #3685's contract to its sibling sites: that `cmdPhaseComplete`'s comparisons were not exposed to the Markdown-normalization artifact already diagnosed in `cmdMilestoneComplete`. `platformWriteSync` normalizes on write — CRLF stripped, blank-line runs collapsed, a blank line inserted after a heading, a single trailing newline enforced. Every flag that compares the PRE-normalization in-memory string against the on-disk pre-image can therefore report a change when the persisted bytes are identical. `cmdMilestoneComplete` had been worked around by re-reading the file after the write; the other sites compared raw strings. All of them now go through one exported seam, `contentChangedAfterNormalize(filePath, before, after)`, which normalizes both sides exactly as the writer does. That removes the extra disk read the milestone workaround needed, and makes the sites agree by construction rather than by four independent implementations of one rule — the divergence the repo names as an anti-pattern. Reachability, stated precisely rather than uniformly: the seam is load-bearing at `cmdPhaseComplete`'s `roadmapUpdated`, `requirementsUpdated` and `stateUpdated`, where section-rewrite logic genuinely regenerates content into a different-but-normalization-equivalent shape. At `updateRoadmapAfterPhaseRemoval` it is defense-in-depth: the no-match branch never reassigns `content`, so the raw comparison was already correct there. The first analysis claimed the reverse; this is the corrected finding. Also fixes an unsound test premise the remote suite caught. The byte-identity precondition in `roadmap_updated is false when ROADMAP.md comes out byte-identical` asserted against a hand-authored, un-normalized fixture — so the very first write reformatted it and the file could not come back identical. The fixture is now written already-normalized, so the assertion compares a normalized pre-image against a normalized post-image and still fails if the flag regresses to a hardcoded `true`. Not platform-specific; it reproduces on macOS too, and the earlier local check simply never exercised it. The sibling true-direction and milestone tests were checked for the same premise and do not share it — they assert `notEqual`, or compare two post-write states produced through the same normalizing seam. Refs #3685 * chore(changeset): backfill PR number for #3691 fragment --------- Co-authored-by: sim <sim@local>
2026-08-24 19:39:51 -04:00
parent c933184b97
commit aaf47c5fc2
23 changed files with 792 additions and 54 deletions
--- a/src/config-loader.cts
+++ b/src/config-loader.cts
@@ -134,6 +134,7 @@ const CONFIG_DEFAULTS = {
  security_block_on: _getNestedConfigDefault('workflow', 'security_block_on'),
  post_planning_gaps: _getNestedConfigDefault('workflow', 'post_planning_gaps'),
  smart_zone_tokens: _getNestedConfigDefault('workflow', 'smart_zone_tokens'),
+  max_prompt_tokens: _getNestedConfigDefault('review', 'max_prompt_tokens'),
 };

 /**
@@ -903,6 +904,13 @@ function loadConfigResolved(cwd: string, options: Record<string, unknown> = {}):
      claude_md_path: get('claude_md_path') || null,
      claude_md_assembly: (parsed['claude_md_assembly']) || null,
      phase_id_convention: get('phase_id_convention') ?? null,
+      // #3691: the documented central review key. Declared here (not federated —
+      // it is central, see config-schema.manifest.json validKeys) so the existing
+      // `review.*` per-lane keys the federated overlay below adds land as SIBLINGS
+      // on this same object rather than being clobbered by it.
+      review: {
+        max_prompt_tokens: get('max_prompt_tokens', { section: 'review', field: 'max_prompt_tokens' }) ?? defaults.max_prompt_tokens,
+      },
    };

    // ADR-857 phase 3b: federated config overlay
--- a/src/milestone.cts
+++ b/src/milestone.cts
@@ -14,7 +14,7 @@ import planningWorkspace = require('./planning-workspace.cjs');
 import frontmatterMod = require('./frontmatter.cjs');
 // eslint-disable-next-line @typescript-eslint/no-require-imports -- state.cjs is an export= CommonJS module
 import stateMod = require('./state.cjs');
-import { platformWriteSync, platformReadSync, platformEnsureDir, execGit, retryRenameSync } from './shell-command-projection.cjs';
+import { platformWriteSync, platformReadSync, platformEnsureDir, execGit, retryRenameSync, contentChangedAfterNormalize } from './shell-command-projection.cjs';
 import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs';
 import { realClock } from './clock.cjs';
 import { transitionCore } from './state-transition.cjs';
@@ -925,6 +925,13 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo
  const accomplishmentsList = accomplishments.map((a) => `- ${a}`).join('\n');
  const milestoneEntry = `## ${version} ${milestoneName} (Shipped: ${today})\n\n**Phases completed:** ${phaseCount} phases, ${totalPlans} plans, ${totalTasks} tasks\n\n**Key accomplishments:**\n${accomplishmentsList || '- (none recorded)'}\n\n---\n\n`;

+  // #3685: mirror requirementsUpdated's diff-tracking contract — the result
+  // below used to report `milestones_updated: true` hardcoded, never
+  // consulting whether the MILESTONES.md write actually changed anything.
+  // Captured before the write branches below so the after-comparison reports
+  // a real content diff instead of an assumed one.
+  const milestonesBefore = fs.existsSync(milestonesPath) ? fs.readFileSync(milestonesPath, 'utf-8') : null;
+
  if (fs.existsSync(milestonesPath)) {
    const existing = fs.readFileSync(milestonesPath, 'utf-8');
    if (!existing.trim()) {
@@ -952,6 +959,11 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo
    platformWriteSync(milestonesPath, `# Milestones\n\n${milestoneEntry}`);
  }

+  // #3685: real content diff, not the hardcoded `true` this used to report —
+  // see the `milestonesBefore` capture above.
+  const milestonesAfter = fs.existsSync(milestonesPath) ? fs.readFileSync(milestonesPath, 'utf-8') : null;
+  const milestonesUpdated = milestonesAfter !== milestonesBefore;
+
  // #2142 BLOCKER 2 (review): opt-in quick-task archival. This call MUST sit
  // immediately adjacent to the STATE.md write block directly below it, with
  // NO unguarded IO in between (unlike the ROADMAP/REQUIREMENTS/audit/
@@ -992,6 +1004,13 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo
  // taken). `resync: true` mirrors `cmdPhaseComplete`'s posture (progress
  // recomputed from disk; only the preserve-when-unchanged deltas apply) —
  // milestone completion is the same kind of lifecycle transition.
+  // #3685: mirror requirementsUpdated's diff-tracking contract — this used to
+  // report `state_updated: fs.existsSync(statePath)`, true even on a no-op
+  // transaction. Declared here beside `stateUpdated`'s sibling flags and
+  // defaulted to `false` so the "STATE.md absent" case keeps today's answer
+  // (existsSync also returns false there) reached via a real content
+  // comparison instead.
+  let stateUpdated = false;
  if (fs.existsSync(statePath)) {
    withStateLock(statePath, () => {
      const originalStateContent = platformReadSync(statePath) || '';
@@ -1073,6 +1092,24 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo
        },
      );
      platformWriteSync(statePath, finalContent);
+      // #3685 / #3691: compare NORMALIZED bytes, not the pre-normalize
+      // `finalContent` string, against the pre-normalize `originalStateContent`
+      // read above. `platformWriteSync` runs Markdown normalization (blank-line
+      // insertion around headings/fences/lists) before persisting — the
+      // transition core (`transitionCore`'s `## Current Position` section
+      // reset) regenerates that section fresh on every call, including on a
+      // genuine no-op re-run, and its raw un-normalized output differs from
+      // the already-normalized on-disk original even though the write
+      // converges to byte-identical content. Comparing pre-normalize strings
+      // (mirroring cmdPhaseComplete's shape verbatim) was verified live to
+      // report `true` on three consecutive byte-identical writes.
+      // `contentChangedAfterNormalize` runs BOTH sides through the exact same
+      // normalizer `platformWriteSync` used to persist (no extra disk I/O,
+      // and immune by construction to this ordering artifact) — this used to
+      // re-read the file to get the same answer; #3691 hoisted that seam so
+      // this site, `updateRoadmapAfterPhaseRemoval`, and `cmdPhaseComplete`'s
+      // roadmap/state/requirements flags all agree by construction.
+      stateUpdated = contentChangedAfterNormalize(statePath, originalStateContent, finalContent);
      for (const field of divergedFields) {
        preservationWarnings.push({ field, reason: 'preserved-over-disagreeing-derived' });
      }
@@ -1151,8 +1188,11 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo
      phases_archive_skip_reason: phasesArchiveSkipReason,
      quick: !!quickArchiveResult && quickArchiveResult.archived > 0,
    },
-    milestones_updated: true,
-    state_updated: fs.existsSync(statePath),
+    // #3685: mirror requirementsUpdated's diff-tracking contract — both flags
+    // now report a real before/after content diff instead of the previous
+    // hardcoded `true` (milestones_updated) / bare fs.existsSync (state_updated).
+    milestones_updated: milestonesUpdated,
+    state_updated: stateUpdated,
    preservation_warnings: preservationWarnings,
  };

--- a/src/phase.cts
+++ b/src/phase.cts
@@ -64,7 +64,7 @@ import planningWorkspace = require('./planning-workspace.cjs');
 import frontmatterMod = require('./frontmatter.cjs');
 // eslint-disable-next-line @typescript-eslint/no-require-imports -- state.cjs is an export= CommonJS module
 import stateMod = require('./state.cjs');
-import { platformWriteSync, platformReadSync, platformEnsureDir, retryRenameSync } from './shell-command-projection.cjs';
+import { platformWriteSync, platformReadSync, platformEnsureDir, retryRenameSync, contentChangedAfterNormalize } from './shell-command-projection.cjs';
 import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs';
 import { realClock } from './clock.cjs';
 import { transitionCore } from './state-transition.cjs';
@@ -1705,15 +1705,23 @@ function findDataRowLine(sectionText: string, dataRowIndex: number): string | nu
  return null;
 }

+// #3685: mirror requirementsUpdated's diff-tracking contract — the caller
+// (cmdPhaseRemove) used to report `roadmap_updated: true` unconditionally,
+// hardcoded regardless of whether this transform actually changed
+// ROADMAP.md's content. Returning a real before/after comparison here lets
+// the caller report accurately, the same fix #3685 applied to
+// `cmdPhaseComplete` and #2640/#2974 already applied to this same function's
+// sibling `stateUpdated` flag a few lines below in `cmdPhaseRemove`.
 function updateRoadmapAfterPhaseRemoval(
  roadmapPath: string,
  targetPhase: string,
  isDecimal: boolean,
  removedInt: number,
  cwd: string,
-): void {
-  withPlanningLock(cwd, () => {
-    let content = fs.readFileSync(roadmapPath, 'utf-8');
+): boolean {
+  return withPlanningLock(cwd, () => {
+    const originalContent = fs.readFileSync(roadmapPath, 'utf-8');
+    let content = originalContent;
    const escaped = escapeRegex(targetPhase);
    // #3572: ROADMAP headings and rows carry the normalized (zero-padded) form
    // of a decimal id — `phase insert 1` writes `### Phase 01.1:` while the
@@ -1911,6 +1919,14 @@ function updateRoadmapAfterPhaseRemoval(
    }

    platformWriteSync(roadmapPath, content);
+    // #3685 / #3691: compare NORMALIZED bytes (what platformWriteSync actually
+    // persists), not the raw pre-normalize `content` string, against the raw
+    // pre-mutation `originalContent` read above — a raw `!==` here reports a
+    // false `true` whenever this transform's regenerated output takes a
+    // different-but-equivalent shape than the already-normalized on-disk
+    // original (same normalization-order artifact #3685 fixed at
+    // cmdMilestoneComplete; see contentChangedAfterNormalize's own doc).
+    return contentChangedAfterNormalize(roadmapPath, originalContent, content);
  });
 }

@@ -2038,7 +2054,7 @@ function cmdPhaseRemove(
    error(`Failed to renumber phase directories after removing phase ${targetPhase}: ${msg}`);
  }

-  updateRoadmapAfterPhaseRemoval(
+  const roadmapUpdated = updateRoadmapAfterPhaseRemoval(
    roadmapPath,
    targetPhase,
    isDecimal,
@@ -2130,7 +2146,11 @@ function cmdPhaseRemove(
      renamed_directories: renamedDirs,
      renamed_files: renamedFiles,
      renamed_file_collisions: renamedFileCollisions,
-      roadmap_updated: true,
+      // #3685: mirror requirementsUpdated's diff-tracking contract — true only
+      // when updateRoadmapAfterPhaseRemoval's content diff detected a real
+      // change, not hardcoded regardless of whether ROADMAP.md's content
+      // actually changed.
+      roadmap_updated: roadmapUpdated,
      state_updated: stateUpdated,
    },
    raw,
@@ -2686,7 +2706,12 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
          before: originalRoadmapContent,
          after: roadmapContent,
        });
-        roadmapUpdated = roadmapContent !== originalRoadmapContent;
+        // #3685 / #3691: normalize both sides before comparing — see
+        // contentChangedAfterNormalize's doc (shell-command-projection.cts).
+        // A raw `!==` here false-positives whenever this phase-complete
+        // roadmap mutation regenerates a section in a different-but-
+        // equivalent raw shape than the already-normalized on-disk original.
+        roadmapUpdated = contentChangedAfterNormalize(roadmapPath, originalRoadmapContent, roadmapContent);

        const reqPath = path.join(planningDir(cwd), 'REQUIREMENTS.md');
        if (fs.existsSync(reqPath)) {
@@ -3003,7 +3028,11 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
          // diff-tracking pattern used for the ROADMAP write above. A phase
          // whose citations match nothing (ghost REQ-IDs only) must report
          // `false`, not a bare "the file was present" `true`.
-          requirementsUpdated = reqContent !== originalReqContent;
+          // #3685 / #3691: normalize both sides before comparing — same
+          // false-positive shape as the sibling roadmapUpdated/stateUpdated
+          // flags in this same transaction; all three must agree by
+          // construction (see contentChangedAfterNormalize's doc).
+          requirementsUpdated = contentChangedAfterNormalize(reqPath, originalReqContent, reqContent);
        }
      }

@@ -3260,7 +3289,12 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
        }

        writes.push({ filePath: statePath, before: originalStateContent, after: stateContent });
-        stateUpdated = stateContent !== originalStateContent;
+        // #3685 / #3691: normalize both sides before comparing (same
+        // transitionCore-regenerated-section artifact cmdMilestoneComplete
+        // hit — see contentChangedAfterNormalize's doc). Reported "not
+        // exposed" by a previous agent; the reviewer disproved that by
+        // inspection and this branch closes it.
+        stateUpdated = contentChangedAfterNormalize(statePath, originalStateContent, stateContent);
      }

      anyPlanningWrite = writePlanningFileSet(writes) > 0;
--- a/src/review-lane-descriptor.cts
+++ b/src/review-lane-descriptor.cts
@@ -253,7 +253,7 @@ export const REVIEWER_LANES: ReadonlyArray<ReviewerLane> = Object.freeze([
    reviewsSection: 'Gemini',
    evidenceClass: 'source-grounded',
    requiresBinaries: [],
-    promptBudgetKey: null,
+    promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.gemini',
    modelConfigKey: 'review.models.gemini',
    handler: null,
  },
@@ -285,7 +285,7 @@ export const REVIEWER_LANES: ReadonlyArray<ReviewerLane> = Object.freeze([
    reviewsSection: 'Claude',
    evidenceClass: 'source-grounded',
    requiresBinaries: [],
-    promptBudgetKey: null,
+    promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.claude',
    modelConfigKey: 'review.models.claude',
    handler: null,
  },
@@ -313,7 +313,7 @@ export const REVIEWER_LANES: ReadonlyArray<ReviewerLane> = Object.freeze([
    reviewsSection: 'Codex',
    evidenceClass: 'source-grounded',
    requiresBinaries: [],
-    promptBudgetKey: null,
+    promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.codex',
    modelConfigKey: 'review.models.codex',
    handler: null,
  },
@@ -338,7 +338,7 @@ export const REVIEWER_LANES: ReadonlyArray<ReviewerLane> = Object.freeze([
    reviewsSection: 'CodeRabbit',
    evidenceClass: 'diff-only',
    requiresBinaries: [],
-    promptBudgetKey: null,
+    promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.coderabbit',
    // Accepts no model flag at all (review.md:367) — not merely "none configured".
    modelConfigKey: null,
    handler: null,
@@ -365,7 +365,7 @@ export const REVIEWER_LANES: ReadonlyArray<ReviewerLane> = Object.freeze([
    // Phase 5b: the handler reconstructs from the JSON stream with JSON.parse, so `jq` — absent on
    // stock Windows/Git-Bash (#2589) — is no longer a prerequisite for this lane.
    requiresBinaries: [],
-    promptBudgetKey: null,
+    promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.opencode',
    modelConfigKey: 'review.models.opencode',
    // Phase 5b (#2799): was `null`. The review is REBUILT from assistant `text` parts; a plain
    // stdout copy would write the raw JSON envelope as the review (#1936). See LaneHandler.
@@ -388,7 +388,7 @@ export const REVIEWER_LANES: ReadonlyArray<ReviewerLane> = Object.freeze([
    reviewsSection: 'Qwen',
    evidenceClass: 'source-grounded',
    requiresBinaries: [],
-    promptBudgetKey: null,
+    promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.qwen',
    modelConfigKey: null,
    handler: null,
  },
@@ -413,7 +413,7 @@ export const REVIEWER_LANES: ReadonlyArray<ReviewerLane> = Object.freeze([
    reviewsSection: 'Cursor',
    evidenceClass: 'source-grounded',
    requiresBinaries: [],
-    promptBudgetKey: null,
+    promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.cursor',
    modelConfigKey: null,
    handler: null,
  },
@@ -440,7 +440,7 @@ export const REVIEWER_LANES: ReadonlyArray<ReviewerLane> = Object.freeze([
    evidenceClass: 'source-grounded',
    // Phase 5b: the handler reads the transcript with JSON.parse per line, not `jq`.
    requiresBinaries: [],
-    promptBudgetKey: null,
+    promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.antigravity',
    // NOT `review.models.antigravity` — the shipped key is `review.models.agy` (review.md:291) and
    // Phase 4 federated it under that name. This lane is why the key is declared, not derived.
    modelConfigKey: 'review.models.agy',
@@ -570,7 +570,7 @@ export const REVIEWER_LANES: ReadonlyArray<ReviewerLane> = Object.freeze([
    reviewsSection: 'Kimi Code',
    evidenceClass: 'source-grounded',
    requiresBinaries: [],
-    promptBudgetKey: null,
+    promptBudgetKey: 'review.max_prompt_tokens_per_reviewer.kimi-code',
    modelConfigKey: 'review.models.kimi-code',
    handler: null,
  },
--- a/src/shell-command-projection.cts
+++ b/src/shell-command-projection.cts
@@ -1174,6 +1174,24 @@ export function normalizeContent(filePath: string, content: string, opts: { enco
  return { content: normalized, encoding };
 }

+/**
+ * True iff persisting `after` via `platformWriteSync` would land different
+ * on-disk bytes than `before` already has (or would have, normalized the
+ * same way). `platformWriteSync` runs Markdown normalization (CRLF strip,
+ * blank-line-run collapse, single trailing newline) before writing, so a
+ * caller comparing raw pre-normalize strings (`after !== before`) can report
+ * `true` even when the persisted bytes are byte-identical — e.g. a
+ * transform that regenerates a section fresh on every call, including a
+ * genuine no-op re-run, in a different-but-equivalent raw shape than the
+ * already-normalized on-disk original (#3685 / #3691). Any "did this write
+ * change the file?" flag MUST go through this seam (or an equivalent
+ * post-write re-read of the actual on-disk bytes) instead of a raw `!==` —
+ * do not simplify this back to a direct string comparison.
+ */
+export function contentChangedAfterNormalize(filePath: string, before: string, after: string): boolean {
+  return normalizeContent(filePath, after).content !== normalizeContent(filePath, before).content;
+}
+
 // Rename errnos that are transient on Windows: a concurrent reader (or an AV
 // scanner / indexer) holding the target open makes renameSync fail briefly.
 // Same idiom as capability-ledger.cts / capability-consent.cts.