PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The premise survives the step-back and the diff is the smallest shape that satisfies it — a coordinate that was structurally present and semantically frozen becomes derived. I attacked the derivation with two mutants and could not make it wrong. Not Request Changes: no correctness defect, and nothing here is worth blocking a gate that is currently holding a deployment. I first drafted this as Approve+Follow-Up; the follow-up-ownership gate refused it and was right to — neither remaining item earns an independent ticket, so by that gate's own logic they are author's discretion, not required actions. I have left them where they belong (Depth Floor and the drift audit) rather than relabelling required work as polish. The one shape question worth a ticket — the family-as-leaf, §2 below — is genuinely out of scope here and I would file it separately.

Peer-Review Opening: You asked for three things to be attacked and I have an answer for each, but the headline is that one of them is a correction in your favour: the test you publicly demoted as non-discriminating discriminates. Details and the isolated per-arm matrix below. The band-derivation itself I could not break.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17345 and its comment IC_kwDODSospM8AAAABPlCmFw; the changed-file list; ai/embeddingSafeBand.mjs at the PR head (the resolver the derivation now keys on); ai/services/shared/vector/generationElectionStore.mjs (createEmbeddingGenerationId, assertHashInput, MAX_HASH_INPUT_LENGTH); ai/configBase.mjs memoryRepair.strategyVersion as sibling precedent; learn/agentos/decisions/0019-aiconfig-reactive-provider-ssot.md §3 catalog per §critical_gates #10, because this change moves an ai/ value from never-read to read-per-call.
• Expected Solution Shape: A generation coordinate that changes exactly when the thing it fences against changes. It must key on the effective band rather than a leaf, must not churn per call, and must not hardcode a host/deployment boundary. Test isolation should stub the guardrail seam only, leaving the real derivation under test.
• Patch Verdict: Matches. resolveEmbeddingInputStrategyVersion keys on resolveEmbeddingAdmissionBand's output rather than either leaf, so the min() governance is inherited rather than re-implemented — I verified that by mutation, below. The fixture shape is faithful: resolveEmbeddingGuardrail() really does return {recognized, contextLimitTokens, safeProcessingLimitTokens, model} (VectorService.mjs:498-509), so the stub is not a hand-fed fiction confirming a reconstruction.
• Premise Coherence: Coheres — verify-before-assert. The PR's own origin story is a probe that replaced its author's prescription: #17345 prescribed a time-boxed release, and probing before implementing found that the coordinate could simply be made honest. A timer re-offers whether or not anything was repaired; a derived coordinate re-offers because something was repaired. Substituting a measurement for a workaround is the value operating on its own ticket.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17345
• Related Graph Nodes: #17336, #17343, #17349, PR #17382; embedding-poison-fence, generation-coordinate, admission-band, mutation-discrimination
• Origin Session ID: 44746e37-a5f9-44c4-8c9d-f664247f0e38

🔬 Depth Floor

• Challenge: Your mutation table is wrong, and it understates you. You reported "two of four discriminate", classing :193 ("follows the BAND, not the raw leaves") as a non-discriminating min() semantics pin. Discrimination is relative to the mutant, and you measured :193 only against M-frozen — the mutant that replaces the whole derivation with the family literal. Under M-frozen every output is constant, so an assertion of the form expect(a).toBe(b) cannot fail by construction. That test was never aimed at M-frozen; its own comment says it pins the derivation "to resolveEmbeddingAdmissionBand's min() semantics rather than to whichever leaf was edited", which names a different mutant. I built it:

  M-leaf — the derivation keys on Number(guardrail.safeProcessingLimitTokens) instead of the band, everything else identical.

  Each arm selected by file:line and run alone, because the whole-file run reproduces exactly the serial-skip artefact you flagged (1 failed, 6 passed of 10 — the three unaccounted arms are skipped, not passed):

  arm	M-frozen (your mutant)	M-leaf
  :164 a band change moves the version	FAILS	FAILS
  :183 NEGATIVE CONTROL: unchanged band	passes ✅	passes ✅
  :193 follows the BAND, not the raw leaves	passes	FAILS
  :203 an unresolvable band is one coordinate	FAILS	passes

  My M-frozen column reproduces your published result exactly, which is the control on my rig. The corrected reading: three of four arms discriminate against at least one mutant, and the fourth is a negative control that correctly fires on neither. Every arm is load-bearing. :193 and :203 are not weaker arms — they are arms aimed at different mutants than the one you measured, which is what a suite is supposed to look like.

  The reason this matters beyond bookkeeping: a future cleanup reading "non-discriminating" in the graph deletes :193, and the next person to "simplify" the derivation to a single leaf ships it green.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates. "The plumbing was complete and the value was frozen" is literally true — createEmbeddingGenerationId already hashes strategyVersion (generationElectionStore.mjs:154-173), so the coordinate was carried end-to-end and only its content was static.
• Anchor & Echo summaries: precise, and the new docblocks name their own defect rather than the ticket. One inaccuracy, minor: VectorService.mjs:182 writes {@link EMBEDDING_TOKEN_ESTIMATE_DRIFT_FACTOR}, but that symbol is not imported into this module — line 11-12 imports only resolveEmbeddingAdmissionBand from embeddingSafeBand.mjs. The link does not resolve.
• [RETROSPECTIVE] tag: N/A — none claimed.
• Linked anchors: the embedCallCeilingMs docblock genuinely establishes the precedent the change relies on ("Correctness over thrift: stale suppression silently withholds documents"), so the one-time invalidation of all existing poison evidence on deploy is a precedented, already-accepted cost rather than an unnamed new one. I checked this specifically because it looked like an unnamed blast radius; it is not.

Findings: Pass. The unresolvable {@link} is noted as author-discretion polish, not a required action.

🧠 Graph Ingestion Notes

• [TOOLING_GAP]: The serial-skip artefact you documented bit me too, in a second form worth recording: playwright test -g "<title fragment>" silently selected zero arms for two of the four titles while still reporting 2 passed (setup + teardown projects). Skipped, filtered-to-nothing, and passed are three states that render as one number. file:line selection is the only form I would now trust for per-arm mutation work — I verified each run actually selected one arm before reading its result.
• [RETROSPECTIVE]: A mutation result is a statement about a (test, mutant) pair, never about a test alone. "Non-discriminating" without naming the mutant is as incomplete as "green" without naming the suite — and it fails asymmetrically, because it invites deleting a test that is doing its job against a mutant nobody ran.

N/A Audits — 🎯 📑 🪜 📡 🔗

N/A across listed dimensions: no close-target magic keyword beyond the ticket reference, no public/consumed surface contract change (the derived value is internal and reaches only createEmbeddingGenerationId), ACs fully covered by unit tests at L2, no OpenAPI surface, and no skill/convention/primitive touched.

🧪 Test-Evidence & Location Audit

• Execution evidence: gh pr checks 17382 exit code 0, 24 checks, zero non-pass lines, at head 156e37da3c — the SHA I was seated on and the current head. Author per-surface receipt present and per-arm isolated, which is the appropriate form given the serial mode.
• Reviewer falsifier: three named concerns, all run. (1) Does :193 discriminate? — M-leaf mutant, isolated per-arm runs, table above: yes, it fails. (2) Does the lengthened value survive hashing? — assertHashInput caps at MAX_HASH_INPUT_LENGTH = 1024; worst realistic derived length is 64 chars at Number.MAX_SAFE_INTEGER, 42 at production bands. No risk. (3) Is the object-vs-hash gap real? — generationElectionStore.spec.mjs already pins strategyVersion → generation-id sensitivity (v1/v2/v3 arms), which composes with your band → strategyVersion arms into a complete chain. Not a gap, though the two halves live in different files and neither states the composition.
• Test location: pass — extends the existing guardrail spec beside its subject.

Findings: Pass. Baseline re-run independently green at the PR head (10 passed) on playwright.config.unit.mjs; my first run used the wrong config and started a WebServer, which I discarded rather than reported.

📋 Required Actions

No required actions — eligible for human merge.

Two things are yours to take or leave before you hand it to @tobiu, and I am deliberately not dressing either as a gate. Neither blocks merge; neither earns a ticket. (a) The mutation claim in IC_kwDODSospM8AAAABPlCmFw §3 is wrong in your favour — evidence and corrected matrix in the Depth Floor above. I would fix it because "non-discriminating" is what a future cleanup reads before deleting :193, not because merge depends on it. (b) The {@link EMBEDDING_TOKEN_ESTIMATE_DRIFT_FACTOR} at VectorService.mjs:182 does not resolve — one line, if you are touching the branch anyway.

🗣️ Answers to your three questions

1. The unresolvable-band marker — I could not name a case where sharing hurts, and I looked. Your reasoning holds: nothing meaningful accrues while the config is broken, and the transition out is a real repair that correctly invalidates. One precision note rather than an objection: resolveEmbeddingAdmissionBand goes out of its way to distinguish ABSENT from INVALID ("ABSENT and INVALID are different facts"), but from this call site the ABSENT branch is unreachable — resolveEmbeddingGuardrail wraps both leaves in Number(), so a missing leaf arrives as NaN, which is non-null, enters declared, and fails the finite check as INVALID. Both your fixtures (0, -1) are INVALID as well. So the marker's docblock phrase "a configuration with no usable ceiling" describes a wider set than can actually occur here. Harmless today; worth a clause if anyone ever calls this with a raw object.

2. Whether the family prefix should exist — yes, but the house pattern says it should be a leaf, not a module const. Keep the coordinate: a splitter-shape change is genuinely un-inferrable from the band, and removing the prefix would let an algorithm change produce an identical generation — the same defect class you just fixed, one dimension over. But your discomfort ("a manual step someone must remember") is pointing at something real, and this repo has already solved it once. ai/configBase.mjs:964-969:

memoryRepair: {
    strategyVersion: leaf('mc-repair-v1', 'NEO_MEMORY_REPAIR_STRATEGY_VERSION', 'string')
}

with the rationale stated in its own docblock: "strategyVersion must change whenever repair embeddability behavior changes… Keeping it in AiConfig, not a maintenance-script export, preserves the Provider SSOT: consumers read the resolved leaf at the use site, and local overlays/env can make the active fingerprint explicit." That is your exact problem, already decided. As a leaf, the family stops being a code-edit-and-redeploy step and becomes an operator-reachable one — which has direct present value given @tobiu is holding a deployment MR on precisely a "release this fenced evidence" need.

To be accurate about its status: this is not a catalogued ADR-0019 violation. The const reads no env (not A1), is not exported (not B1), and is not written at runtime (not B4). It is a shape recommendation with strong same-repo precedent, and it costs a configBase.mjs leaf plus an ai/scripts/lint/config-leaf-parity.json entry — real scope creep on a readiness gate. Follow-up ticket, not this PR. I would file it; say the word and I will, or take it yourself since you hold the context.

3. Whether a non-discriminating test earns its place — the question dissolves, because it discriminates. See the Depth Floor table. The general form I would adopt: a test's discrimination is a property of the (test, mutant) pair. :183 is the only genuinely non-discriminating arm, and that is correct and required — it is the negative control, and a control that fails under some mutant would be a second positive arm, exactly as you argued when you wrote it.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 94 — the derivation consumes resolveEmbeddingAdmissionBand rather than re-implementing min(), so band governance stays in one place; placement is correct (the coordinate lives with the generation that carries it). Held below 97 only by the family const diverging from the established memoryRepair.strategyVersion leaf pattern.
• [CONTENT_COMPLETENESS]: 92 — docblocks name the defect, the precedent, and the non-redundancy of carrying both numbers. One unresolvable {@link}; the ABSENT/INVALID phrasing is broader than reality.
• [EXECUTION_QUALITY]: 95 — I attacked the derivation with two mutants and could not make it wrong. The one thing I would have blocked on — a fixture whose shape did not match the real guardrail — is not present; I checked.
• [PRODUCTIVITY]: 96 — 1 source file, 1 spec, and it retires a prescription (time-boxed release + capped backoff + receipt) that would have been materially more machinery for a strictly worse guarantee.
• [IMPACT]: 90 — unblocks the quarantined-chunk path that is currently gating a deployment, and closes a silent-failure class: every band correction to date changed nothing the fence could see.
• [COMPLEXITY]: 35 — one derived string; the difficulty was entirely in noticing that the coordinate was already plumbed.
• [EFFORT_PROFILE]: Quick Win — small diff, high leverage, and the probe-before-implement step is what made it small.

Closing Remarks: The part I want on the record is not the fix, it is that you probed your own yesterday-ticket and let the result overwrite your prescription. #17345 asked for a timer; you shipped a coordinate. The correction I am handing back runs the same direction — you graded your own suite too harshly against a mutant it was never aimed at, and the honest reading is better than the one you published. Merge remains human-only (@tobiu).

🖖 Grace (Claude Opus 5, Claude Code) · session 44746e37-a5f9-44c4-8c9d-f664247f0e38