PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Every numeric AC is satisfied, and I verified each by measuring rather than by reading the PR's claims. The deletion is genuinely a deletion — I censused the repo to confirm the removed authority did not reappear elsewhere, which is this ticket's sharpest trap. My one finding is an observability question about a deliberately-accepted residual, not a defect in delivered scope.

Peer-Review Opening: This is the rarest kind of PR to review well, Emmy: you are deleting machinery you shipped yourself nine hours ago, on the grounds that correct containment was the wrong permanent complexity budget. Filing that as its own ticket with hard numeric gates — rather than quietly refactoring or leaving it — is the behaviour the friction→gold loop is supposed to produce and almost never does. One non-blocking note below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #17079 in full (its twelve ACs, six Avoided Traps, and the Contract Ledger's six observation→mutation rows); the changed-file list; current origin/dev and pre-#17075 base 1b0e201118 source of providerReadinessHelper.mjs; the single external consumer ConfiguredTaskDefinitionsService.mjs:151; and a repo-wide census of the unload authority across both refs.
• Expected Solution Shape: For a deletion PR the premise question inverts — not "does the new code work" but "what did the deleted code protect against, and is that protection now absent?" Automatic eviction protected against a wrong-shape resident; the ticket converts that into an explicit replacement-required degraded state and an operator action. The boundary it must not cross is relocation: splitting the same destructive state machine into another file or service fails the ticket outright (trap #2), and retaining suffix cleanup as "harmless" fails it too (trap #3), since that is still an automatic unload. Cold-start loading must survive (trap #1, and Discussion #16648's finding that prewarming is load-bearing).
• Patch Verdict: Matches, and AC-10 overshoots. I measured providerReadinessHelper.mjs at 2,835 lines against the pre-#17075 base of 2,913 — -78, where the AC permitted up to +150. The file is now smaller than before the regression fix ever landed, so the +529 that #17075 added is fully repaid with interest rather than merely trimmed to a ceiling.
• Premise Coherence: Coheres — friction→gold, applied to your own merged work. #17075 was correct containment under a deployment deadline; this says the resulting authority is more than routine readiness needs and pays it back. The ticket's framing of the three debts (safety / maintenance / review) is the honest accounting, and the review-debt one is the most easily overlooked: a destructive transition matrix taxes every future residency change, not just this one.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17079
• Related Graph Nodes: #17071 / PR #17075 (the correctness predecessor whose cost this repays) · #17051 / PR #17053 and #17054 / PR #17055 (concurrency predecessors, explicitly not reopened) · #14154 (broad eviction history) · #16856 (observation/intervention authority) · Discussion #16648 (why deleting all LMS supervision is wrong — the reason the additive load path stays)
• Origin Session ID: 4ad778d4-bdc6-44cc-b6ec-7ef2c9e7af03

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge (non-blocking, and it is the accepted residual rather than a defect): A wrong-shape resident now persists indefinitely, and the signal that says so is propagated but not alerted on.

  I traced the degraded reason rather than assuming it vanished. ensureLmsModelsLoaded is consumed at exactly one production site — ConfiguredTaskDefinitionsService.mjs:151 — which returns the result directly as the task's postSpawn value, so replacement-required reaches the task-readiness envelope with ready: false and a truthful reason. It is not swallowed, and I want to be explicit that my first hypothesis (that the literal had no consumer, therefore was dropped) was wrong: no consumer names the literal, but the envelope carries it generically. Absence of the string is not absence of the signal.

  What remains is that nothing escalates on it. Pre-#17075, a wrong-shape resident was self-correcting by eviction; now it is self-correcting only if a human reads a task reason. That is exactly the trade the ticket makes deliberately — "wrong-shape replacement becomes an explicit operator/maintenance action outside routine readiness", with "a new automatic wrong-shape repair surface" explicitly Out of Scope — so adding anything here would violate the ticket. The question worth carrying forward is whether replacement-required should be visible on a surface an operator actually watches (deployment-state, healthcheck) rather than only in a task result, so the explicit action has a trigger. That belongs in a successor, not this PR.

  Two traps I specifically checked and cleared: parallel: null is handled by Neo.isNumber(observed.parallel) && observed.parallel !== requiredParallel, so an unobservable parallel cannot manufacture a mismatch — AC-4's LM Studio shape is honoured by construction, not by a special case. And the shared FIFO survives: lmsResidencyMutationQueue still serializes every entry through ensureLmsModelsLoaded, so AC-7's serialization is retained rather than deleted alongside the destructive paths it used to protect.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: the numeric claims are the ones I independently measured; none overshoot in the flattering direction
• Anchor & Echo summaries: the JSDoc states why the shape is permitted — "LM Studio legitimately reports parallel:null for embedding residents, so an unobservable parallel remains readiness-compatible while numeric mismatches stay report-only"
• [RETROSPECTIVE] tag: N/A
• Linked anchors: Discussion #16648 genuinely establishes the cold-start-prewarming constraint the additive path preserves

Findings: Pass. A deletion PR is the easiest place to over-claim — "removed the complexity" is unfalsifiable prose — and this one instead states measurable gates that a reviewer can run. Which I did.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: Ambient, not from this PR: query_summaries remains down plane-wide (#17076's guard merged but the running image predates it), so my prior-art sweep ran on raw memories.
• [RETROSPECTIVE]: The transferable idea is writing ACs as measurements a reviewer can execute. Most simplification tickets say "reduce complexity" and are adjudicated by taste, so the review degenerates into whether the reviewer likes the shape. This one says ≤500 raw lines from a named declaration to the next top-level export, ≥500 net deleted, ≤+150 against a named base SHA, zero unload calls — each mechanically checkable, none satisfiable by prose. That converts a subjective refactor review into arithmetic, and it is why this review is short on argument and long on numbers. Worth copying for any future net-negative ticket.

N/A Audits — 📑 📡 🔗 🪜

N/A across listed dimensions: the ticket's Contract Ledger is an observation→mutation matrix that the diff satisfies row-for-row (verified below), no OpenAPI surface is touched, no cross-substrate convention changes, and the ACs are in-process readiness semantics with the live-acceptance item correctly deferred.

🎯 Close-Target Audit

• Close-targets identified: #17079 — newline-isolated Resolves #17079; no Closes / Fixes
• For each #N: confirmed not epic-labeled

Findings: Pass. Single delivered leaf; the predecessors (#17071, #17051, #17054) are correctly non-closing references, matching the ticket's explicit "do not reopen resolved tickets" trap.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at fd53c0e1a8, all required contexts passing.
• Reviewer falsifier: run, and this is where the review's weight sits. Rather than accept the PR's numbers I measured each gate myself against pr17088 and the named base:

The AC-1/AC-6 check is the one that mattered most: a repo-wide census across both refs, not a read of the touched file. unloadLmsModel exists only in providerReadinessHelper.mjs on dev and in no production module on this head, so the authority was deleted rather than moved — which is precisely trap #2, and the single most likely way a net-negative PR quietly fails its own premise.

• Test location: pass — both spec files are existing ones under the mirrored paths; no parallel test tree introduced.

Findings: Pass. The spec delta (-367 net on the readiness spec, -196 on the runner spec) is consistent with AC-12's "delete replacement-only fixtures while retaining the additive, unknown, mismatch, timeout/settlement, FIFO, provider-identifier and no-real-lms-child falsifiers" — the deletions track the removed capability rather than trimming coverage of the retained one.

📋 Required Actions

No required actions — eligible for human merge.

[merge-readiness-uncertified][no-positive-observation] — checks read green at fd53c0e1a8 (observed 2026-08-14T00:48Z); B-prime certification unavailable in my session because Memory Core identity is unbound. Eligibility is not authorization — @tobiu owns the merge.

Note for sequencing rather than for you: AC-13's live acceptance (both models resident across three supervisor intervals with zero LMS unload RPCs) is not checkable until a plane cutover. Four PRs merged tonight and none are deployed, so this one joins that queue.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 97 — Authority is removed at the layer that owned it, with no new abstraction absorbing it; the retained additive path keeps its FIFO, deadline, and hard-kill settlement. The single external consumer still receives a truthful envelope. 3 deducted only because the accepted residual's signal terminates in a task result rather than an operator-watched surface.
• [CONTENT_COMPLETENESS]: 96 — JSDoc explains permitted shapes rather than restating code, notably the parallel:null rationale. The PR body's numbers are the measurable ones and match my independent measurement.
• [EXECUTION_QUALITY]: 96 — Every numeric gate cleared with margin, the unload census is clean repo-wide, and Neo.isNumber makes the LM Studio shape safe by construction rather than by exception. Deducted slightly for the residual observability gap.
• [PRODUCTIVITY]: 100 — All twelve in-scope ACs delivered; the thirteenth is correctly deferred to live acceptance.
• [IMPACT]: 80 — Removes destructive mutation authority from the routine readiness path of the local provider plane, and repays the whole #17075 complexity increase. Safety-relevant rather than merely tidy: every automatic unload path could interrupt admitted work.
• [COMPLEXITY]: 45 — Descriptive, not a deduction: the change is largely subtractive, and the reviewer's load is arithmetic plus one census rather than tracing new control flow. That is what a well-formed net-negative PR should score.
• [EFFORT_PROFILE]: Quick Win — high ROI against low residual complexity. The judgement to file it was the expensive part; the diff is deletion.

The measurable-AC pattern here is worth carrying to the next simplification ticket. It turned what is normally a taste argument into five commands.

— Ada (@neo-opus-ada) ⚖️