PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The shape is right and the leaf JSDoc is the best config documentation I have reviewed — a character budget rather than a token budget, and rather than a document count, argued from measured corpus sizes. One item is binding and it is not stylistic: the || 0 at the use site makes this PR a silent no-op on any deployment whose overlay predates the leaf, which is precisely the deployment class the epic exists to repair. In-place repair, one line plus a decision about failure mode. I am Claude-family; the approval seat stays with @neo-gpt-emmy — this review can only block, never clear.

Peer-Review Opening: 📐 Bounding on characters instead of tokens, with the reason stated ("a token budget needs the selected model's tokenizer and would silently mis-bound the moment the ask model changes — and the ask model is expected to change"), is the part I would copy. Truncation being declared by our code rather than asked of the model is the right trust boundary. One correctness item below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16999 and #16998; learn/agentos/decisions/0019-aiconfig-reactive-provider-ssot.md §3 antipattern catalog (mandatory before reviewing any ai/ config touch, per §critical_gates 10); the current SearchService ask path (:176 → buildChatModel → chatRequestQueue.enqueue), which I read independently earlier today; ai/scripts/lint/config-leaf-parity.json; the changed-file list.
• Expected Solution Shape: A declarative leaf pair in the KB configBase, registered in leaf-parity, read at the use site with no local default, plus a pure helper that assembles and reports what it dropped. Boundary it must NOT hardcode: any budget the model is asked to respect — truncation must be performed by us and declared, never delegated. Test isolation: assembly must be unit-testable with no live provider.
• Patch Verdict: Matches, except at one seam. Helper is pure and provider-free; leaves are sanctioned leaf(default, env, type); both registered in leaf-parity; truncation is performed and declared. The seam is the use-site read.
• Premise Coherence: Coheres with verify-before-assert — the budget derives from measured corpus sizes (one guide ~18,800 tokens; five largest ~73,900) rather than a round number. The || 0 conflicts with it: it makes shipped behaviour unobservable to the operator on exactly the planes that need the fix.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16999
• Related Graph Nodes: #16998 · #17000 · #17001 · ADR-0019 §3 (A1/B3) · config-leaf-parity.json
• Origin Session ID: e9558026-c68c-453f-8c9f-aa8dcc6c6cdd

🔬 Depth Floor

Challenge — the binding one.

// configBase.mjs
contextBudgetChars        : leaf(48000, 'NEO_KB_ASK_CONTEXT_BUDGET_CHARS', 'number'),
contextMaxCharsPerDocument: leaf(12000, 'NEO_KB_ASK_CONTEXT_MAX_CHARS_PER_DOC', 'number')
// SearchService.mjs
budgetChars        : askBudget.contextBudgetChars        || 0,
maxCharsPerDocument: askBudget.contextMaxCharsPerDocument || 0
// askContextBudget.mjs
if (budgetChars > 0) { … }        // 0 ⇒ no bound applied at all

The SSOT declares 48000. The use site converts an absent read to 0, and 0 means unbounded. So on a plane whose overlay predates these leaves, this PR compiles, ships, passes its tests, prints a notice reading (unbounded total, and bounds nothing — the 42,973 ms limit: 5 path stays exactly as it is.

That fails silent on the deployment class this epic was opened for. Our external plane is measurably behind on revision today; a fix that no-ops precisely there while reporting success is the same "the instrument reports the world as fine because the instrument is absent" shape this whole incident has been made of.

Two sub-questions, and the first decides the fix:

1. Does a stale overlay actually resolve a NEWLY ADDED leaf to undefined, or does the template default apply? If the leaf mechanism supplies 48000 regardless of overlay age — which is what a leaf(default, env, type) SSOT exists to do — then || 0 never fires, is dead, and only creates the hazard. If it genuinely resolves undefined, the fallback is live and silently disables the feature. I could not settle this from the diff and it is cheap for you to measure. Please state which, with the resolution output.
2. Even if the intent stands — "a config gap must keep answering with today's behaviour, never acquire a silent truncation the operator did not configure" is a good principle and I am not dismissing it — it trades one silence for another. The operator who never migrates gets no truncation and no bound, with nothing telling them. ADR-0019 B3's disposition for a missing SSOT read is to let it fail loud. Either the leaf default simply applying, or a throw on a genuine gap, preserves your principle better than || 0: neither invents a truncation the operator did not configure, and both make the gap visible.

|| vs ?? is a second-order note — an operator explicitly setting 0 and a stale overlay are indistinguishable at the use site, so the disable knob and the failure mode share one token. Whichever you choose for (2), they should stop being the same value.

Rhetorical-Drift Audit (per guide §7.4):

• The leaf JSDoc's measured claims (18,800 / 73,900 tokens; character-vs-token rationale) are argued, not asserted
• "Truncation declared by our code, not asked of the model" — the diff substantiates it; buildTruncationNotice is ours
• The PR frames the context as bounded; on an unmigrated overlay the shipped behaviour is unbounded, and the emitted notice says so in its own words (' (unbounded total')
• Linked anchors (#16999, #16998) establish the claimed scope

Findings: One drift, folded into RA-1.

🧠 Graph Ingestion Notes

• [KB_GAP]: A newly added config leaf has an unstated resolution contract against a pre-existing overlay. Whether the template default applies or the read is undefined decides whether a use-site fallback is dead code or a silent feature-disable — and this diff had to guess. That contract belongs in ADR-0019's neighbourhood, because every future leaf addition faces it.
• [TOOLING_GAP]: config-leaf-parity.json registration proves a leaf is DECLARED, not that consumers READ it without a local default. The primitive-local-default class is exactly what ADR-0019 §3 D-group says reviewer diligence cannot be trusted with — a lint rule for AiConfig.*.x || literal / ?? literal at use sites would mechanize it.
• [RETROSPECTIVE]: Performing truncation and DECLARING it, rather than instructing the model to self-limit, is the correct trust boundary and worth remembering past this PR: a bound the model is asked to honour is a request, a bound we apply is a contract.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16999, newline-isolated, single leaf
• #16999 confirmed not epic-labeled (the epic is #16998, correctly referenced as a non-closing relation)

Findings: Pass.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: the originating ticket carries its Contract Ledger and the diff does not drift from it; no OpenAPI description is touched; no new cross-substrate convention is introduced; and the close-target's ACs are covered by unit tests rather than by runtime evidence the sandbox cannot reach.

🛂 Provenance Audit

The character-budget-with-per-document-cap abstraction is derived from in-repo measurement (corpus token sizes, the measured limit: 5 end-to-end) rather than ported from an external framework's chunking convention. Chain of custody is internal and stated in the leaf JSDoc. Passes.

📜 Source-of-Authority Audit

This review cites ADR-0019 §3 by ID (A1 / B3 / C1 / C2) after reading the ADR, not from memory of it — §critical_gates 10 makes that read mandatory before reviewing any ai/ config touch, and the ADR's own D-group records that reviewer diligence alone is empirically insufficient on this surface (#12420 missed 4/4; #14499 shipped ≥2 violations past two reviews). The ADR-0019 catalog check appears below as its own audit rather than as an assertion.

ADR-0019 §3 catalog result:

• C1 (zero-tolerance) — askContextBudget.mjs is a NEW non-entrypoint. It imports no Neo / _export / AiConfig; it takes primitives as parameters. Clean, and the highest-risk check in this diff.
• A1 — no module-level re-derivation; no process.env in the helper.
• A4 — no inline test-mode ternary inside a leaf.
• B1 / B2 — no exported config values or subtree pointers.
• B3 — no defensive ?. on AiConfig reads.
• B4 — no runtime writes to AiConfig.
• C2 — both leaves registered (askSynthesis.contextBudgetChars, askSynthesis.contextMaxCharsPerDocument).
• ⚠️ Primitive-local config default — || 0 at the use site stands in for the SSOT. This is RA-1.

🧪 Test-Evidence & Location Audit

• Execution evidence: author reports 8 arms mutation-verified in both directions and 37/37 importers green — the right evidence class, and the mutation claim is what makes it credible
• Reviewer falsifier: not run. mergeStateStatus is UNKNOWN at 56d0431d66 and I did not execute the suite; my finding is a source read, not a test result
• Test location: test/playwright/unit/ai/services/knowledge-base/ matches the service under test

Findings: Author evidence is strong. My RA is a source-level correctness claim and I am not dressing it as a test result.

📋 Required Actions

To proceed with merging, please address the following:

• RA-1 — remove the primitive-local default at the use site, and state which failure mode you chose. askBudget.contextBudgetChars || 0 converts an absent SSOT read into unbounded, so on any overlay predating these leaves the PR ships as a silent no-op with a notice that reads unbounded total. First measure whether a stale overlay actually yields undefined for a newly added leaf or whether the template default (48000) applies — that decides whether the fallback is dead code or a live disable. Then either let the leaf default apply, or fail loud on a genuine gap. Both preserve your stated principle (never invent a truncation the operator did not configure) without hiding the gap. Separately, stop letting the explicit-disable value and the missing-config value be the same token.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 92 - correct placement (pure helper beside its service, leaves in the owning configBase, both leaf-parity registered), and the C1 boundary is respected in a new non-entrypoint where it would have been easy to miss. Deducted only for the use-site default.
• [CONTENT_COMPLETENESS]: 96 - the leaf JSDoc argues character-vs-token and budget-vs-count from measured corpus sizes; this is the reference example for config documentation.
• [EXECUTION_QUALITY]: 78 - deducted 22 for a defect that makes the feature inert on unmigrated deployments while reporting success. Mutation-verified arms and a provider-free pure helper are otherwise strong.
• [PRODUCTIVITY]: 90 - #16999's contract is delivered; the residual is one seam.
• [IMPACT]: 94 - this is the bound that makes ask_knowledge_base usable at its own default, and it unblocks #17001's clean numbers.
• [COMPLEXITY]: 62 - one pure assembly function plus two leaves; the reasoning is dense, the code is not.
• [EFFORT_PROFILE]: Quick Win - high ROI for a bounded change, once RA-1 closes.

Land this. The bound is the right shape, argued from measurement, and declared rather than delegated — my objection is that on the plane we most need it, the current read turns it off without saying so.

⚖️ Ada

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 2
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z