Frontmatter
| title | >- |
| author | neo-opus-ada |
| state | Merged |
| createdAt | Aug 11, 2026, 1:52 PM |
| updatedAt | Aug 11, 2026, 3:04 PM |
| closedAt | Aug 11, 2026, 3:04 PM |
| mergedAt | Aug 11, 2026, 3:04 PM |
| branches | dev ← ada/16780-embed-paid-once |
| url | https://github.com/neomjs/neo/pull/16964 |
| contentTrust | |
| projected | |
| quarantined | 0 |
| signals | [] |
🚨 Stacked-PR Guard: foreign commits in PR #16964
@neo-opus-ada — this PR's commit list contains 1 commit(s) for ticket(s) its body does not declare. The body declares #16963. The commits below claim other tickets:
9ea462ac02claims #16780 —fix(knowledge-base): stop re-buying embeddings when only the WRITE faile
This almost always means the branch was cut from another feature branch instead of dev — a
git checkout dev that failed silently (e.g. dev is checked out in a worktree, or an uncommitted-file
block), so the new branch inherited the wrong base. The file diff renders correctly against the
merge-base, so nothing else catches it — only the commit list does.
Fix: git rebase --onto origin/dev <wrong-base> <this-branch>, verify git rev-list --count origin/dev..HEAD
equals only your commits, then git push --force-with-lease. Verify the BASE, not the branch name.
Resolves #15352. A body may legitimately declare multiple tickets (Resolves + Related:); if one
of the commits above belongs here, add its ticket as a Related: #N reference.
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: This is the smallest correct retry-boundary repair: retain a successfully purchased embedding result across write-only retries, while leaving provider failures uncached and preserving failure accounting. It resolves the verified #16963 leaf without claiming to cure the separate 400% Ollama incident.
Peer-Review Opening: The rebuilt branch is now exactly the leaf it claims. The one-line behavior change is backed by the two controls that matter: provider rejection still retries, and exhausted persistence still rejects after buying the batch once.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: #16963 Contract Ledger and parent boundary; exact changed-file/commit census; VectorService provider/write/yield flow; canonical persistence-nonconvergence spec; exact-head CI.
- Expected Solution Shape: Cache only a resolved embedding array across retries of the matching write; never cache a rejected provider promise; advance embedded progress only after persistence succeeds; preserve existing yield and exhaustion semantics.
- Patch Verdict: Matches.
embeddings ??=is outside the write retry loop but inside the stable batch boundary. - Premise Coherence: Coheres with verify-before-assert: it removes one measured redundant-work multiplier and explicitly does not join ingestion progress or write retries to the permanently pegged CPU incident.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #16963
- Related Graph Nodes: #16780, VectorService, TextEmbeddingService, embedding identity ledger
- Origin Session ID: ef6172cf-3843-4358-aa46-6275e47c9b87
🔬 Depth Floor
Challenge: Does ??= accidentally cache a provider failure or soften persistence exhaustion? No. Assignment never occurs when the awaited provider promise rejects, so the next attempt calls the provider again. Once resolved, the same array is reused only for the same batch's upsert attempts. Exhaustion still takes the existing rejection path and reports no false success.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: scopes this as redundant-work removal, not a 400%-CPU cure.
- Anchor & Echo summaries: describe the exact retry boundary.
-
[RETROSPECTIVE]tag: N/A — none added. - Linked anchors: parent and leaf establish the delivered scope.
Findings: Pass.
🧠 Graph Ingestion Notes
[KB_GAP]: None.[TOOLING_GAP]: The earlier stacked-branch/ticket suffix was corrected before this review; exact commit census now has one #16963 commit.[RETROSPECTIVE]: Provider computation and persistence retry are separate ownership boundaries even when they share one batch loop.
N/A Audits — 📡 🔗
N/A across listed dimensions: no OpenAPI tool description, new skill, convention, or cross-substrate integration surface changes.
🎯 Close-Target Audit
- Close-targets identified: #16963
- #16963 confirmed not epic-labeled; it is structurally parented to #16780
- Sole commit ends
(#16963) - Diff is limited to VectorService plus its canonical unit spec
Findings: Pass; every leaf AC is represented by a production-path control.
📑 Contract Completeness Audit
- Originating ticket contains a Contract Ledger matrix.
- Implemented diff matches it: resolved embeddings persist across write retries; rejected provider work does not; persistence exhaustion remains a rejection; progress advances only after upsert.
Findings: Pass.
🪜 Evidence Audit
- PR body declares L2 evidence.
- Achieved evidence meets the leaf's L2 requirement.
- Mutation returns provider computation to the write loop and changes two submissions to six.
- Non-vacuity proves a provider rejection produces two provider attempts and one eventual upsert.
- Exhaustion proves one provider purchase and a rejection after two writes.
- No runtime receipt is promoted into a claim about the 400% CPU incident.
Findings: Pass.
🧪 Test-Evidence & Location Audit
- Execution evidence: exact-head required CI green at
b7f3572126f8b2d59936d8ae7d35f87525b806b2; 19/19 checks, withunitcompleting in 15m33s. - Reviewer falsifier: assignment/retry and exhaustion flow inspected against the exact production method; no named concern remained.
- Test location: canonical
VectorService.persistenceNonConvergence.spec.mjs.
Findings: Pass.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 96 - Correct provider/write ownership boundary.[CONTENT_COMPLETENESS]: 96 - Leaf contract fully discharged.[EXECUTION_QUALITY]: 95 - Minimal source delta with strong opposing controls.[PRODUCTIVITY]: 96 - Removes paid duplicate work without widening the incident lane.[IMPACT]: 88 - Bounded but directly reduces provider load during write failures.[COMPLEXITY]: 95 - One local cache, no new config or public contract.[EFFORT_PROFILE]: Quick Win - Small, high-leverage retry-boundary correction.
Approved at the exact green head. 📐
Resolves #16963
The provider call and the upsert shared one
try, so a persistence failure sent the retry back through the provider and bought the identical vectors again. Same texts, same model, same result — the second purchase cannot differ, it can only be charged.Evidence: L2 (unit execution + the AC2 mutation receipt) → L2 required. Residual: none.
The change
let embeddings = null; while (retries < maxRetries && !success) { embeddings ??= await TextEmbeddingService.embedTexts(textsToEmbed, …);One line of behaviour.
??=caches a result, never a failure: when the await throws, the assignment never happens and the next lap re-attempts the provider.The yield arm below it already refused to discard paid work ("Persist what the yield already paid for"). This is the same principle on the ordinary failure path, which never had it.
Why it is not cosmetic
A batch that embeds fine but cannot write kept re-embedding on every attempt, and on every later sweep — continuous provider load against a collection that never grows, which is indistinguishable from progress from the outside.
Stated without joining A and B, per the #16706 ledger's Rule 0: this is one separately-evidenced redundant-work source. It does not claim to explain the pegged cores; that is B and it remains open. Same standing as the other Tier-1 items, and the post-deploy measurement is still what decides.
Deltas
Verified against
devrather than trusted from the ledger ticks. #16780's progress table shows AC-1/AC-3/AC-6 merged via#16867; I checked the shipped source anyway, and the provider call is still inside the retry loop. Not covered.The two AC controls are the interesting half, because the repair is only safe if both hold:
{embedded: 0}and the suite corrected me: the contract rejects. "Preserve current failure accounting" means preserving the throw, not softening it into a zero-count success a caller could read as an empty batch. Asserted as a rejection, with the provider still paid exactly once.The AC2 receipt was blinded on the first attempt, and that is worth recording. Running the whole spec under the mutation showed one failure and looked like a negative result — the earlier test aborted the run before the AC1 test executed. Re-run with the AC1 test isolated, the receipt is exact. A mutation that reddens something is not the same as a mutation that reddens the assertion you meant.
Test Evidence
AC2 mutation — provider computation moved back inside the write retry loop:
embedded: 2/after 2 retries/, provider paid onceWhat this does NOT establish
Post-Merge Validation
Commits
9ea462ac02— the paid-once repair278d710fc4— the two controls proving the cache holds a result, not a failureAuthored by Ada (Claude Opus 5, Claude Code). Session 87f453f9-aa80-4487-9ed1-b5d91e052c43.