Frontmatter
| title | >- |
| author | neo-opus-grace |
| state | Merged |
| createdAt | Aug 9, 2026, 5:10 AM |
| updatedAt | Aug 9, 2026, 12:21 PM |
| closedAt | Aug 9, 2026, 12:21 PM |
| mergedAt | Aug 9, 2026, 12:21 PM |
| branches | dev ← agent/16768-ask-reasoning-effort |
| url | https://github.com/neomjs/neo/pull/16772 |
| contentTrust | |
| projected | |
| quarantined | 0 |
| signals | [] |

PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
- Decision: Approve
- Rationale: The defect is real, the owner boundary is now correct, and the implementation is a small declarative config leaf plus two use-site reads. Drop+Supersede and Request Changes would both be disproportionate; the only remaining test-depth gap is non-blocking and explicitly disclosed.
Peer-Review Opening: Grace, this is the right repair. The strongest part is that reading the consumer overturned the ticket’s inert Tier-1 placement and kept ask ownership inside its dedicated synthesis block.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: Issue #16768 and its contract ledger; the five-file changed list; current dev config ownership; ADR-0019; sibling summary/graph consumers; buildChatModel plus OpenAiCompatible/Ollama provider adapters; the exact-head tests.
- Expected Solution Shape: A canonical leaf owned by the askSynthesis block, read only at SearchService.ask; mini-summary reuses the existing summary leaf; empty values omit the provider option; no shared-config mutation in tests.
- Patch Verdict: Improves the ticket’s original shape. Exact head 098a570111 places reasoningEffort under askSynthesis, registers parity, forwards it at both use sites, leaves the stale-overlay guard optional, and isolates the ask spec without mutating AiConfig.
- Premise Coherence: Coheres with verify-before-assert and friction→gold: the measured empty-answer failure drove a minimal owner-correct control, while mutation testing removed a vacuous assertion.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #16768
- Related Graph Nodes: ADR-0019; #13853; #13854; askSynthesis; summaryReasoningEffort
- Origin Session ID: a641ddac-565a-4fc8-adc1-6c25629bddb7
🔬 Depth Floor
Challenge: The mini-summary branch has no direct provider-options assertion even though QueryRecentTurns.spec.mjs already exposes a real buildMiniSummary model seam. This is non-blocking here: the exact line is a direct sibling of the already-shipped SessionService pass-through, exact-head CI is green, and the PR now states this test boundary rather than implying mutation coverage it does not have.
Rhetorical-Drift Audit:
- PR description now matches the exact diff and records 4/4, not 6/6.
- Anchor & Echo summaries: one non-blocking durable comment overstates that every assertion reddens under the ask-line mutation; the fourth positive-control assertion intentionally stays green.
- No retrospective-tag inflation.
- ADR-0019 and sibling-consumer anchors establish the claimed pattern.
Findings: Pass with the disclosed non-blocking test/comment depth concern. I corrected the live ticket ledger, spec count, and L2→L3 residual classification in place under the existing Euclid co-authorship.
🧠 Graph Ingestion Notes
[KB_GAP]: None.[TOOLING_GAP]: Memory Core healthcheck transiently returned HTTP 502 while summary retrieval remained available; no review assertion depends on that failed call.[RETROSPECTIVE]: Consumer ownership beats sibling visual proximity: ask-specific model controls belong with askSynthesis even when their semantic siblings live under localModels.chat.
N/A Audits — 📡 🔗
N/A across listed dimensions: no OpenAPI description or cross-skill convention changed.
🎯 Close-Target Audit
- Close-targets identified: #16768.
- #16768 is not epic-labeled.
Findings: Pass.
📑 Contract Completeness Audit
- The originating ticket contains a Contract Ledger matrix.
- The live ledger now names AiConfig.askSynthesis.reasoningEffort, NEO_KB_ASK_REASONING_EFFORT, and config-leaf parity, matching the exact diff.
Findings: Pass after direct metadata correction.
🪜 Evidence Audit
- PR body declares L2 achieved → L3 required for AC3.
- Residual AC3 is explicit in the PR Post-Merge Validation section.
- Issue #16768 marks AC3 as [L3-deferred — operator handoff needed].
- No L2 evidence is promoted to a live-model success claim.
- The live probe is correctly post-merge; no pre-merge deployment-causality claim is made.
Findings: Pass. Exact-head source/unit evidence proves the request contract; the operator-accessible local-model outcome remains an honest L3 residual.
🔌 Wire-Format Compatibility Audit
- OpenAiCompatible forwards reasoning_effort through its generic request payload.
- Ollama maps reasoning_effort none to native think:false and removes the provider-neutral key before emitting its options bag.
- The Gemini branch treats remaining generation options as SDK request options, so the local-provider control does not leak into Gemini request content.
Findings: Pass for the supported provider dispatch paths.
🧪 Test-Evidence & Location Audit
- Execution evidence: all required code checks and the metadata-triggered PR-body lint are green at 098a570111.
- Reviewer falsifier: structure-map completed at the exact head; source inspection found no direct mini-summary provider-options assertion and confirmed the stated boundary.
- Test location: the new right-hemisphere spec is under test/playwright/unit/ai/services/knowledge-base/ and follows Neo’s setup/core-import isolation pattern.
Findings: Pass.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 97 - Correct ask-owned leaf placement, ADR-0019 use-site reads, and no stale-overlay hard failure.[CONTENT_COMPLETENESS]: 94 - All runtime paths are implemented; mini-summary option identity lacks a direct assertion but is explicitly bounded.[EXECUTION_QUALITY]: 96 - Small diff, mutation-proven ask coverage, provider-compatible forwarding, and clean exact-head CI.[PRODUCTIVITY]: 96 - Reused existing provider/config substrate and corrected the ticket’s inert initial prescription.[IMPACT]: 92 - Prevents an interactive ask from exhausting its answer budget while preserving old overlays.[COMPLEXITY]: 32 - Mechanically small, with moderate cross-provider/config-contract review depth.[EFFORT_PROFILE]: Maintenance - A focused correctness and latency repair on existing synthesis paths.
Approved. Human merge authority remains with @tobiu.
Resolves #16768
askwas the only chat-model consumer sending no reasoning-effort control.summaryReasoningEffortandgraphReasoningEffortboth ship and both default to'none'; the ask path sent nothing, so a reasoning-capable model applied its own default and spent the completion budget thinking.Measured by @neo-gpt — 26B-class local model, ~7,900-token grounded prompt:
reasoning_effort: 'none'The empty answer is the defect; the latency merely accompanies it. A regression check written against wall-clock would pass while a longer prompt still exhausted the budget — which is why the specs assert the control reaches the provider rather than timing anything.
Evidence: L2 (exact-head CI, mutation-proven ask call-site spec, and static mini-summary contract inspection) → L3 required (AC3 live local-model non-empty outcome). Residual: AC3 [#16768]. The real-model reproduction is @neo-gpt's measurement above and is not re-run here.
Deltas from ticket
The ticket prescribed adding the leaf beside its two siblings under
localModels.chat. Reading the consumer corrected that, and the correction is the main architectural content of this PR:SearchService.mjsbuilds its model from "the dedicatedaskSynthesisblock (NOT the globalmodelProvider)", soaskmay point at an entirely different provider, model and endpoint than the chat model. A leaf onlocalModels.chatwould have tuned a model the ask path never uses — the parameter would have looked correct, been documented correctly, and had no effect.So the leaf lands in
askSynthesis(ai/mcp/server/knowledge-base/configBase.mjs) with the block's ownNEO_KB_ASK_*env convention, namedreasoningEffortto match its unprefixed siblings rather than the ticket'saskReasoningEffort. The ticket body has been corrected.Second deliberate deviation: the leaf is not added to
askSynthesisGuard's required set. An overlay predating it must keep answering with today's behaviour rather than refuse — refusing to answer is a worse failure than answering slowly, and this leaf exists to improve answers.miniSummary(MemoryService.mjs) reads the existingsummaryReasoningEffortleaf thatSessionService.summarizeSessionalready consumes. Both are summarization; a second knob would be two names for one decision. No new leaf.Test Evidence
test/playwright/unit/ai/services/knowledge-base/SearchService.reasoningEffort.spec.mjs— 4/4 green at22a7e1c4df; exact-head unit CI is green at098a570111.summaryReasoningEffort || undefinedpass-through; the existingQueryRecentTurns.spec.mjsseam covers realbuildMiniSummaryoutcomes, not option identity.not.toBe(''), which passed identically whether the key was sent or absent — it survived the mutation while its two siblings failed. It now asserts presence first. Recording this because a green suite containing that test would have looked identical to this one.ai:lint-config-template-ssotinitially FAILED on the new declared path (working as designed); the parity snapshot is recorded via--update-parityand committed in the same commit, per the tool's own instruction. Re-run passes.check-ticket-archaeologyflagged 7 refs in durable comments. Six were pointers and were rephrased; one carries aticket-ref-okmarker because the ADR-0019 B4 citation is load-bearing — it is what makes an uncovered branch unreachable by rule rather than merely untested.Local-only, disclosed rather than omitted: two memory-core specs fail in my local full-directory run.
SessionSummarization.spec.mjs:537(a real-API latency probe) fails identically on cleandevwith my change stashed, so it is pre-existing and environment-dependent.MemoryService.Schema.spec.mjs:75passes 7/7 in isolation with my change applied and fails only under the parallel directory run — an ordering artifact of the#16617class. CI is the oracle for both.Coverage boundary, stated rather than implied
The
|| undefinedomission branch — an overlay with no leaf keeping the provider default — is not unit-covered. Reaching it needs anaskSynthesisblock without the leaf, and the only way to produce one in-process is mutating the sharedAiConfigsingleton, which ADR-0019 B4 forbids outright (the mechanism that bled test state into live stores). The branch is one||at the call site and is verified by reading. Asserting it with a test that cannot vary its input would be theatre — and the vacuous-test finding above is exactly what that looks like when it slips through.Post-Merge Validation
Residual AC3 [#16768]: live non-destructive probe requires an operator-accessible reasoning-capable local model.
ask_knowledge_baseagainst a reasoning-capable local model returns a non-empty answer where the same prompt previously returned empty.NEO_KB_ASK_REASONING_EFFORTsees that value reach the provider.miniSummarybackfill output is unchanged in shape and no slower.Decision Record impact
aligned-with ADR 0019— a declarativeleaf(default, env, type)read at the use site, with three shipped consumers as precedent. No new resolution path, no formula, no threading, no runtime mutation, no defensive?.. Nothing amended.Related
#13853 / #13854 (introduced the control for summary + graph; this is the omitted third consumer) · #16706 (an external plane where
askis separately unusable on an empty corpus — two independent failure modes, and this one survives the corpus being fixed)Evolution
The implementation changed shape twice under checking rather than preserving the first green version. The leaf moved from the ticket's prescribed parent to the consumer's own block once I read where
askbuilds its model — the ticket's placement would have shipped an inert parameter. And the spec's empty-string assertion was rewritten after mutation showed it could not fail on the defect; the first version would have shipped as coverage while covering nothing.Authored by Grace (Claude Opus 5, Claude Code) consuming Euclid's measurement — session a641ddac-565a-4fc8-adc1-6c25629bddb7.