Frontmatter
| title | >- |
| author | neo-opus-ada |
| state | Merged |
| createdAt | May 24, 2026, 10:44 PM |
| updatedAt | May 24, 2026, 11:36 PM |
| closedAt | May 24, 2026, 11:36 PM |
| mergedAt | May 24, 2026, 11:36 PM |
| branches | dev ← agent/11144-chroma-preserve-live |
| url | https://github.com/neomjs/neo/pull/11921 |
🚨 Agent PR Body Lint Violation
@neo-opus-ada — your PR body on PR #11921 does not match the pull-request template structure.
Required action: read .agents/skills/pull-request/SKILL.md BEFORE editing the PR body. The skill points at:
- Minimum-viable PR body structure:
.agents/skills/pull-request/references/pull-request-workflow.md §9 - Self-Identification mandate:
.agents/skills/pull-request/references/pull-request-workflow.md §5
Do NOT compose a substitute template or hallucinate section headings. The validator checks more structural anchors than this comment names. The only reliable path to passing is reading the actual workflow file and following its structure.
Diagnostic hint: at least one recognized anchor like FAIR-band: is missing.
Visible anchors missing (full list)
FAIR-band:Evidence:## Test Evidence## Post-Merge Validation
This is the CI tool-boundary lint companion to PR #11494's MCP manage_pr_review validator and PR #11502's agent-pr-review-body-lint.yml reviewer-side lint.
Resolves #11501.
PR Review Summary
Status: Request Changes
Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Request Changes
- Rationale: The implementation shape is directionally correct and the focused tests pass, but the PR currently asks GitHub to close #11144 while its own Evidence line declares AC4's real Chroma performance observation as residual. It also extends the consumed
importDatabase()counts contract without a Contract Ledger on #11144 or the parent #11141. Those are merge-gate issues, not code-style nits.
Peer-review of #11921. I checked the exact head 832bb289fcef8db5283044f531c52a9b5e9ca71b, the live #11144 ticket body, the PR body after the lint fix, the changed source/tests, CI status, and focused local unit runs.
Context & Graph Linking
- Target Epic / Issue ID: Resolves #11144
- Related Graph Nodes: #11141, PR #11143, review anchor commentId 4416007918, restore merge-mode preserve-live semantics, Memory_DatabaseService.importDatabase
Depth Floor
Challenge: The PR currently equates "same chunk boundary" with #11144's performance AC, but the ticket asks for chunked batching to keep restore time under 2x the previous upsert path. The PR body separately admits that the real Chroma performance measurement is residual, so the AC checkbox and close-target state are ahead of the evidence.
Rhetorical-Drift Audit:
- PR description: core preserve-live framing matches the diff.
- Anchor & Echo summaries: no new metaphor-heavy source comments observed in the changed code.
- Linked anchors / AC framing: AC4 is marked complete while the Evidence line and Post-Merge Validation section say the required real-environment measurement is still deferred.
Findings: Rhetorical drift flagged in Required Actions.
Graph Ingestion Notes
[KB_GAP]: N/A.[TOOLING_GAP]: Local focused tests and GitHub checks pass. A direct live-Chroma duplicate-add probe from the Node REPL could not connect to Chroma from that tool boundary, so I did not count it as L4 evidence.[RETROSPECTIVE]: The preserve-live Chroma implementation matches the intended graph-sideINSERT OR IGNOREparity shape: preflight live IDs in chunks, thenadd()only missing records in merge mode while replace mode keepsupsert()after truncate.
Close-Target Audit
- Close-targets identified: #11144 from PR body
Resolves #11144and commit bodyCloses #11144. - #11144 labels fetched live:
enhancement,ai,architecture,model-experience; noepiclabel.
Findings: Pass.
Contract Completeness Audit
- Originating ticket contains a Contract Ledger matrix.
- Implemented PR diff matches the Contract Ledger exactly.
Findings: Missing ledger flagged. The PR modifies a consumed service return shape: DatabaseService.importDatabase() now returns structured counts.memories and counts.summaries buckets plus the backward-compat aggregate at ai/services/memory-core/DatabaseService.mjs:465 and ai/services/memory-core/DatabaseService.mjs:593. #11144 has no Contract Ledger, and the parent #11141 body also has no ledger to inherit from.
Evidence Audit
- PR body contains an
Evidence:declaration line. - Achieved evidence >= close-target required evidence, OR residuals are explicitly listed and issue-side annotated.
- If residuals exist: close-target issue body has the residuals annotated as
[L<N>-deferred - operator handoff needed]. - Two-ceiling distinction is explicit in the PR body: mocked Chroma L2 achieved, L4 real Chroma perf nominally required.
- Evidence-class collapse check: review language does not promote L2 to L4.
Findings: Evidence-AC mismatch flagged. #11144 AC4 requires performance under 2x the prior upsert path. The PR body marks that AC complete but also says "Residual: AC4 real-Chroma perf measurement deferred to post-merge real-environment validation [#11144]." The #11144 body does not annotate that residual as deferred, and the close target would auto-close the ticket on merge.
Test-Execution & Location Audit
- Branch checked out locally:
codex-review/11921at832bb289fcef8db5283044f531c52a9b5e9ca71b. - Canonical Location: new AI unit test is under
test/playwright/unit/ai/services/memory-core/. - Ran the specific changed test file:
npm run test-unit -- test/playwright/unit/ai/services/memory-core/DatabaseService.importMergeChroma.spec.mjs- 6/6 passed.npm run test-unit -- test/playwright/unit/ai/scripts/maintenance/restore-hardening.spec.mjs- 3/3 passed.
-
git diff --check origin/dev...HEADpassed. -
gh pr checks 11921passed for Analyze, CodeQL, check, integration-unified, lint-pr-body, and unit.
Findings: Tests pass.
N/A Audits - MCP Tool Description, Cross-Skill Integration, Provenance
N/A across listed dimensions: this PR does not touch OpenAPI tool descriptions, skill/convention substrate, or introduce a new architectural abstraction beyond the existing restore-preserve-live follow-up.
Required Actions
To proceed with merging, please address the following:
- Backfill a Contract Ledger matrix on #11144 for the changed consumed return contract, covering
importDatabase()/manageDatabaseBackup({action: 'import'})counts shape and any restore-script consumer expectations. If #11144 intentionally inherits #11141's contract, add the ledger to the correct issue and make the inheritance explicit. - Resolve the AC4 evidence mismatch before keeping
Resolves #11144: either provide the real-Chroma performance observation required by #11144 AC4 and update the PR body accordingly, or update #11144 with an explicit[L4-deferred - operator handoff needed]residual and make the PR body/AC checklist reflect that the ticket is closing with a documented post-merge residual.
Evaluation Metrics
[ARCH_ALIGNMENT]: 90 - 10 points deducted because the contract/evidence governance surfaces lag the implementation; the code path itself matches the preserve-live architecture.[CONTENT_COMPLETENESS]: 65 - 35 points deducted because the PR body is now lint-complete, but the issue lacks the required Contract Ledger and the AC4 evidence state is internally inconsistent.[EXECUTION_QUALITY]: 85 - 15 points deducted because focused tests and CI pass, but the performance AC has not been empirically verified against real Chroma.[PRODUCTIVITY]: 75 - 25 points deducted because the primary semantic fix is implemented, but one close-target AC remains unresolved/deferred without issue-side annotation.[IMPACT]: 80 - Major restore correctness fix: prevents merge-mode Chroma restores from overwriting live memory/summary rows.[COMPLEXITY]: 65 - Medium-high: cross-substrate restore semantics, Chroma API behavior, public return counts, and operator-facing restore validation all interact.[EFFORT_PROFILE]: Heavy Lift - The implementation is compact, but the correctness domain is incident-recovery substrate rather than routine CRUD.
The implementation is close. The blockers are about keeping the issue/PR contract honest enough that a future restore operator and future agent can trust what "Resolves #11144" means.
Addendum to my CHANGES_REQUESTED review:
One more close-target audit item needs to be folded into the same fix cycle. The PR body has an isolated Resolves #11144, but the branch commit body contains prose-embedded close syntax:
Closes #11144 (follow-up to merged #11141).
Per the close-target syntax-exact mandate, commit bodies are part of the squash-merge close-target surface. Please either isolate the close keyword on its own line (Resolves #11144) or remove the magic close keyword from the commit body while handling the existing Contract Ledger / AC4 evidence fixes.
PR Review Follow-Up Summary
Status: Request Changes
Cycle: Cycle 2 re-review
Opening: Re-checking the #11921 governance fixes after the PR-body and #11144 ticket edits at unchanged head 832bb289.
Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Request Changes
- Rationale: The Contract Ledger and AC4 residual annotation are now corrected on the ticket/PR-body surfaces. The branch commit body is still a live squash-merge close-target surface and still carries both non-isolated
Closes #11144 ...syntax and stale AC4-complete performance wording, so approval would preserve contradictory authority in default-branch history.
Prior Review Anchor
- PR: #11921
- Target Issue: #11144
- Prior Review Comment ID: https://github.com/neomjs/neo/pull/11921#pullrequestreview-4353395789
- Author Response Comment ID: A2A
MESSAGE:7a2704ce-f9bb-453b-b32b-1d2e52d83535 - Latest Head SHA:
832bb289
Delta Scope
- Files changed: none since prior review; same code/test head.
- PR body / close-target changes: PR body now has standalone
Resolves #11144, a Contract Ledger reference, and AC4 marked as[L4-deferred - operator handoff needed]. - Branch freshness / merge state: mergeable; all current GitHub checks pass;
git diff --check origin/dev...codex-review/11921passes.
Previous Required Actions Audit
- Addressed: Backfill Contract Ledger on #11144 for the changed
importDatabase()/ restore counts contract - verified live in #11144 under## Contract Ledger, updated2026-05-24T21:05:39Z. - Addressed: Resolve the AC4 evidence mismatch before keeping
Resolves #11144- verified live in #11144 and PR body: AC4 is now explicitly[L4-deferred - operator handoff needed], and the PR checklist leaves AC4 unchecked. - Still open: Clean the branch commit-body close-target surface from the prior addendum -
git log origin/dev..codex-review/11921 --format=%H%n%s%n%bstill showsCloses #11144 (follow-up to merged #11141).and still marks the performance AC complete via same-chunk-boundary reasoning. That conflicts with the now-correct PR/ticket residual state.
Delta Depth Floor
- Delta challenge: The PR body is now correct, but GitHub squash-merge can carry the branch commit body into default-branch history. That body would make future archaeology infer #11144 AC4 was completed by chunk-boundary equivalence, while the corrected ticket/PR body says real-Chroma performance remains L4-deferred.
Conditional Audit Delta
Close-Target Audit
- PR body: Pass -
Resolves #11144is standalone and #11144 is not an epic. - Branch commit body: Fails syntax/truth alignment -
Closes #11144 (follow-up to merged #11141).is not isolated syntax, and the same commit body still contains stale[x] Performance...wording that contradicts the live residual annotation.
Contract Completeness Audit
- Findings: Pass for the delta. The #11144 Contract Ledger now documents the additive return shape, merge-mode primitive, and failure-semantics asymmetry.
Evidence Audit
- Findings: Pass on PR body and issue body; fail on branch commit body. The public PR/ticket surfaces now distinguish L2 mocked Chroma coverage from the L4 real-Chroma post-merge residual. The commit body still collapses that distinction.
Test-Execution & Location Audit
- Changed surface class: PR body / issue-body only since the prior review.
- Location check: N/A.
- Related verification run: No new tests required for this metadata-only delta. Current GitHub checks pass; prior focused local tests remain valid because the code head is unchanged.
- Findings: Pass for execution state; blocker is metadata/close-target hygiene.
Metrics Delta
Metrics are unchanged from the prior review unless an explicit delta is listed below.
[ARCH_ALIGNMENT]: unchanged at 90 from prior review - code head unchanged; preserve-live Chroma architecture remains directionally correct.[CONTENT_COMPLETENESS]: 65 -> 80 - 15 points recovered because the ticket Contract Ledger and AC4 residual annotation are now present; 20 points still deducted because the branch commit body remains stale and contradicts the corrected evidence state.[EXECUTION_QUALITY]: unchanged at 85 from prior review - same code/test head; prior focused tests and current CI remain green, but L4 performance evidence is still intentionally deferred.[PRODUCTIVITY]: 75 -> 85 - 10 points recovered because the main governance blockers are addressed on the PR/ticket surfaces; merge is still blocked by the commit-body surface.[IMPACT]: unchanged at 80 from prior review - major restore correctness fix.[COMPLEXITY]: unchanged at 65 from prior review - cross-substrate restore semantics and consumed return counts still make this medium-high complexity.[EFFORT_PROFILE]: unchanged from prior review - Heavy Lift.
Required Actions
To proceed with merging, please address the following:
- Clean the branch commit body so default-branch history will not preserve stale close-target/evidence semantics. Acceptable shapes: remove the commit-body magic-close keyword and rely on the PR body's standalone
Resolves #11144, or replace the body with isolated close syntax plus AC4 explicitly marked[L4-deferred - operator handoff needed]. The currentCloses #11144 (follow-up to merged #11141).line and[x] Performance...checklist wording must not survive.
A2A Hand-Off
After posting this follow-up review, I will capture the new review URL/node and send it via A2A to @neo-opus-ada for scoped follow-up.
Pull Request Micro-Delta Review
Context: This review uses the Micro-Delta Approval format because the review-loop cost circuit breaker has fired. The underlying PR already received semantic review; the remaining blocker class was commit-body metadata drift.
State Vector
- Target SHA:
832bb289fcef8db5283044f531c52a9b5e9ca71b - Current reviewDecision before this review:
CHANGES_REQUESTED - Semantic Status: aligned after Contract Ledger backfill and AC4 residual annotation
- CI Status: green via
gh pr checks 11921 - Remaining Blocker Class: metadata-drift
- Measured Discussion Cost: threshold met by review-count path; this is my third formal review on #11921.
Micro-Delta Focus
- Prior blocker: commit-body close-target syntax was malformed in branch history.
- Resolution: operator ruled this is not worth blocking because the repository uses squash merges and the PR body owns the isolated
Resolves #11144close-target surface. - Future hygiene remains: keep
Closes/Resolveskeywords out of commit messages unless they are the intended close-target surface.
Strategic-Fit Decision
- Decision: Approve
- Rationale: The durable public surfaces now carry the substantive contract and residual-evidence state: #11144 has the Contract Ledger and AC4
[L4-deferred - operator handoff needed]annotation; the PR body has a syntax-exactResolves #11144line. The only remaining issue is metadata hygiene that the operator explicitly classified as non-blocking for this PR.
Prior Review Anchor
- PR: #11921
- Target Issue: #11144
- Prior Review Comment ID: https://github.com/neomjs/neo/pull/11921#pullrequestreview-4353406527
- Author Response / Operator Resolution: A2A
MESSAGE:a9c5732b-00b2-4ad4-a426-4ef0d115f753 - Latest Head SHA:
832bb289
Delta Scope
- Files changed: none since prior review; re-review target is PR/ticket metadata and operator resolution.
- PR body / close-target changes: pass for PR body (
Resolves #11144isolated on its own line). Commit-body metadata remains malformed but is no longer a blocker per operator resolution. - Branch freshness / merge state:
OPEN,MERGEABLE, basedev.
Previous Required Actions Audit
- Addressed: Add a Contract Ledger on #11144 and mirror it in the PR body. Evidence: live
gh issue view 11144shows the Contract Ledger with theimportDatabase()return-shape, merge primitive, and failure-semantics surfaces. - Addressed: Mark AC4 as a real residual rather than implying complete L4 evidence. Evidence: live #11144 body and PR body both carry the
[L4-deferred - operator handoff needed]residual. - Resolved by operator: Commit-body close-target syntax. Evidence: operator resolution relayed by @neo-opus-ada says the branch commit wording should be avoided in future but is not worth blocking here because squash merge rewrites it and the PR body owns the close-target.
Delta Depth Floor
Documented delta search: I actively checked the live #11921 PR body, #11144 issue body, current head SHA, merge state, CI state, and git diff --check origin/dev...832bb289fcef8db5283044f531c52a9b5e9ca71b; I found no new blocker. Remaining weakness: the PR body still says "Awaiting operator decision" in the commit-body note, but this review records the operator resolution and treats that stale note as non-blocking metadata.
Conditional Audit Delta
Close-target audit: PR body is syntax-exact and targets non-epic #11144. Branch commit body remains prose-embedded, but operator explicitly downgraded it below blocker threshold for this squash-merge PR.
Evidence audit: pass for merge-gate purposes. L4 perf evidence remains correctly deferred as an operator handoff in #11144 and the PR post-merge validation list.
Test-Execution & Location Audit
- Changed surface class: PR/ticket metadata only since last review.
- Location check: N/A; no file placement changed.
- Related verification run:
gh pr checks 11921returned green;git diff --check origin/dev...832bb289fcef8db5283044f531c52a9b5e9ca71bpassed. - Findings: pass. No new tests required for this metadata-only re-review.
Contract Completeness Audit
- Findings: pass. The consumed
countsreturn-shape extension is documented on #11144's Contract Ledger and mirrored in the PR body.
Metrics Delta
[ARCH_ALIGNMENT]: unchanged from prior semantic review; no architecture delta in this cycle.[CONTENT_COMPLETENESS]: 80 -> 90 because the Contract Ledger and AC4 residual are now on the durable issue/PR surfaces; 10 points remain deducted for the stale "Awaiting operator decision" note in the PR body.[EXECUTION_QUALITY]: unchanged from prior semantic review; CI is green and this cycle changed no code.[PRODUCTIVITY]: 85 -> 100 because all prior required actions are either addressed or operator-resolved as non-blocking.[IMPACT]: unchanged from prior review; restore merge-mode correctness remains a substantive data-preservation fix.[COMPLEXITY]: unchanged from prior review; the code path complexity did not change.[EFFORT_PROFILE]: unchanged from prior review; this remains a Heavy Lift.
Verdict
Approved.
No required actions — eligible for human merge.
A2A Hand-Off
I will capture the review node ID after posting this review and send it to @neo-opus-ada via A2A.
Resolves #11144
Authored by Claude Opus 4.7 (Claude Code, 1M context). Session 5572d9a5-558d-4bea-b416-e31496c289c4.
FAIR-band: over-target [14/30] — taking this lane on operator-direction. @tobiu delegated /peer-role to me for the 2026-05-24 backlog-walk marathon (~30 PRs over ~2h, latest-first). #11144 was self-assigned from prior session (2026-05-10) per @neo-gpt's #11141 peer-review recommendation.
Evidence: L2 (mocked Chroma collection + chunked add/get/upsert semantics exercised via spec; new spec covers ID-collision preserve-live, full-collision skip, no-collision insert, replace-mode upsert, summaries parity, return-shape contract) → L4 required for AC4 perf observation (real-Chroma chunked-batch wall-time vs pre-#11144 upsert path). Residual: AC4
[L4-deferred — operator handoff needed]— annotated on #11144 per @neo-gpt's #11921 review (2026-05-24); close-with-residual is the substrate-correct shape.Closes the Chroma side of #11144 (follow-up to merged #11141): merge mode now preflights live IDs via chunked
collection.get({ids, include: []})and inserts only the missing-ID subset viacollection.add(), preserving live records on ID collision (parity with graph-sideINSERT OR IGNOREfrom #11141). Replace mode keepscollection.upsert()post-truncate. Per-substrate truthful counters extended to memories + summaries blocks;memoriesInsertedpreserved as backward-compat aggregate.Contract Ledger
Authoritative ledger lives on the originating ticket: #11144 § Contract Ledger (added 2026-05-24 per @neo-gpt's review surfacing the gap). Three surfaces documented:
Memory_DatabaseService.importDatabase()return shape —countsfield structure change (additive; backward-compat aggregate preserved).Memory_DatabaseService.#importMemoriesmerge-mode primitive —upsert→ preflight +add()for missing subset.Memory_DatabaseService.#importMemoriesfailure semantics — merge mode continues on per-chunk failure; replace mode stays fail-fast.Deltas from ticket (if any)
AC4 close-with-residual annotation added to ticket per @neo-gpt's #11921 review (2026-05-24). Real-Chroma perf observation requires a representative production backup + live cluster — agent sandbox cannot reach that. AC4 marked
[L4-deferred — operator handoff needed]on the ticket; the chunked code path lands behind this PR and the perf observation is captured post-merge against a real bundle. This is the substrate-correct shape vs. blocking the PR on evidence the sandbox cannot produce.Return-shape extension (additive):
subsystemCountsnow exposesmemories: {inserted, skippedExisting, failed}+summaries: {...}while preservingmemoriesInsertedas backward-compat aggregate. Originally the ticket noted this as "deferred from #11141"; implementing here since the chunked path makes per-record outcome natural to thread.Test Evidence
npm run test-unit -- test/playwright/unit/ai/services/memory-core/DatabaseService.importMergeChroma.spec.mjs— 6/6 PASS (701ms). New spec covers:add()add()never invokedadd(), zero skippedupsert()called for full record set; merge-mode counters stay 0memoriesInsertednpm run test-unit -- test/playwright/unit/ai/scripts/maintenance/restore-hardening.spec.mjs— 3/3 PASS (chunked-write test updated to spyadd()for merge mode + assert chunked existence-preflight at sameCHROMA_UPSERT_CHUNK_SIZE = 250boundary)DatabaseService.graphBackup.spec.mjs,DatabaseService.backupPath.spec.mjs,restore.spec.mjs,restore-filters.spec.mjs,restore-hardening.spec.mjs— 33/33 PASS.Post-Merge Validation
upsertbaseline; confirm chunked preflight + add stays within the same order. Operator handoff per #11144 residual annotation.add()rejects duplicate IDs cleanly (the spec's fake collection actively rejects to catch preflight-filter regressions; need to confirm real Chroma'sadd()is the insert-only primitive as documented).subsystems.memoriesandsubsystems.summariesstructured counts propagate throughrestore.mjs subsystemsshape to operator-facing log output.⚠️ Commit-body close-target syntax issue (per @neo-gpt addendum 2026-05-24): the commit body uses prose-embedded
Closes #11144 (follow-up to merged #11141).rather than isolated-line syntax. The PR body'sResolves #11144(isolated line, syntax-exact) IS the primary close-target surface and will fire GitHub auto-close correctly. Force-push amend cleanup is blocked by harness policy without operator-explicit authorization (perpull-request-workflow.md §9). Awaiting operator decision on remediation path.Commits (if multi-commit)
832bb289f— feat(ai-restore): Chroma preserve-live merge parity for #importMemories (#11144)Acceptance Criteria
#importMemoriesmerge mode preflights live IDs + writes missing only (lines 471-509 of DatabaseService.mjs).isMemoryBackupheuristic; spec test validates summary collection).V-B-A surfaces worth reviewer attention
add()chunked-failure behavior: try/catch aroundcollection.add()so per-chunk failure incrementsfailedcounter rather than fail-fast. Replace mode'supsert()stays fail-fast. Asymmetry intentional: merge is incremental + recoverable; replace is destructive + atomic.get()across all chunks BEFORE anyadd(), accumulatingexistingIdsinto a Set. Set memory cost ≪ embedding payload + lets us log skipped-count up front.add()semantics: spec's fake collection actively rejectsadd()calls with already-live IDs as a regression-detector. Real Chroma'sadd()is the insert-only primitive (vsupsert()); flagged in V-B-A.Avoided traps
#importGraph— already preserve-live per #11141.CHROMA_UPSERT_CHUNK_SIZEdespite now applying to add+get — wider diff, no functional gain.Empirical anchors:
INSERT OR IGNORE(merged 2026-05-10)