PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The leaf belongs in this substrate lane, but the diff currently freezes a narrower contract than #14433's live owner record and omits one maintenance path for the new collection. This is iterative fix territory, not Drop+Supersede.

Peer-Review Opening: Thanks for getting the temporal-summary storage leaf into a reviewable shape. The pure schema module and explicit collection branches are the right general direction; the remaining issues are contract reconciliation and one concrete cache-recovery bug before this can close #14433.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14433 body and live comments; PR changed-file list; ADR 0017, ADR 0019, ADR 0024, ADR 0028; current dev source for ChromaManager/HealthService/config-template tests; GitHub unit log at head 9cd5bed6; focused local schema/config/manager runs; direct ChromaManager cache probe.
• Expected Solution Shape: Leaf A should add the temporal-summary collection and SUMMARY_* schema without adding writers, while keeping the storage/metadata contract aligned with the live #14433 owner addenda. New Memory Core collection plumbing should mirror memory/session/summary/graph paths, including cache invalidation and config-template coverage.
• Patch Verdict: Partially matches, but contradicts the expected shape in two places: the schema closes over only the initial five-field contract while later owner addenda remain live, and invalidateCollectionCache() does not include the new collection even though HealthService now routes temporal-summary retries through it.
• Premise Coherence: coheres: this is a Body/Brain boundary-respecting Brain substrate leaf; conflicts: claiming Resolves #14433 before reconciling the live owner addenda violates verify-before-assert on the close target.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14433
• Related Graph Nodes: #12679, #14434, #14435, #14453, #14743, #11976, #11983, #12456, ADR 0017, ADR 0019, ADR 0024, ADR 0028

🔬 Depth Floor

Challenge: The PR treats the five-field metadata contract as exact and complete, but #14433's live thread added direction/window semantics, pending observer/event/author body-fold demands, and the release event-boundary window class after the initial body. Either those addenda must be folded/disposed, or the PR cannot honestly close #14433.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates (no overshoot)
• Anchor & Echo summaries: precise codebase terminology, no metaphor or source-code snapshot anchor (ticket/PR/lane/AC/cycle/line number) that overshoots durable intent
• [RETROSPECTIVE] tag: accurately characterizes what shipped (no inflation of architectural significance)
• Linked anchors: cited tickets/PRs actually establish the claimed pattern (no borrowed authority)

Findings: Required Action. The PR's Resolves #14433 framing overshoots the live ticket record until the later owner addenda are implemented or explicitly disposed.

🧠 Graph Ingestion Notes

• [KB_GAP]: ADR 0019 C3 remains a live contradiction in the unit suite: tests still import gitignored config.mjs overlays despite the sanctioned template-import form.
• [TOOLING_GAP]: The current red unit job has two distinct coordination paths: #14743 covers the clock-brittle handoffRetrospective hard failure; the overlay-import retry failure needs a live #11976/#11983 successor or reopen path, not a flake waiver.
• [RETROSPECTIVE]: Adding a new Memory Core collection requires checking the complete collection lifecycle, not just creation and direct access: config leaves, manager memoization, cache invalidation, health retry, backup/restore, destructive guard, and tests.

🎯 Close-Target Audit

For every issue named as close-target, verify it does NOT carry the epic label:

• Close-targets identified: #14433
• For each #N: confirmed not epic-labeled (or flagged as Required Action below)

Findings: Pass on epic check; Required Action on close-target completeness. #14433 is not epic-labeled, but its live owner addenda are not reconciled by this PR.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix
• Implemented PR diff matches the Contract Ledger exactly (no drift)

Findings: Contract drift flagged. #14433's initial ledger is present, but the live addenda changed the effective contract before implementation: direction/window semantics, pending observerBoundary/event-class/authorKind fold, and the release event-boundary window-class/partition vocabulary need implementation or explicit deferral before Resolves #14433 is truthful.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line (or N/A justified inline)
• Achieved evidence ≥ close-target required evidence, OR residuals are explicitly listed in the PR's ## Residual / Post-Merge Validation section
• If residuals exist: close-target issue body has the residuals annotated as [L<N>-deferred — operator handoff needed]
• Two-ceiling distinction: PR body distinguishes "shipped at L because sandbox ceiling" from "shipped at L because author didn't probe further"
• Evidence-class collapse check: review language does NOT promote L1/L2 evidence to L3/L4 framing without explicit sandbox-ceiling caveat

Findings: Evidence-AC mismatch flagged. Focused schema coverage is real, but local probing found the new collection is not included in cache invalidation, and the GitHub unit job is red at exact head. #14743 is already claimed for the clock-brittle hard fail; the overlay-import failure still needs live ticket routing.

N/A Audits — 📡

N/A across listed dimensions: this PR does not touch ai/mcp/server/*/openapi.yaml or MCP tool descriptions.

📜 Source-of-Authority Audit

This review cites live owner/operator authority, so I checked the primary artifacts directly: #14433 comments through the July 4 addendum, ADR 0019 C3, #11976/#11983/#12435/#12456 state, and the current unit log. The operator's current clarification supersedes the old #11976 body assumption that mutation/setup imports are harmless: overlays exist to be mutated and tests must not consume them as fixtures.

Findings: Required Action. Do not rely on the closed #11976/#11983 disposition as a waiver for the current overlay-import CI evidence.

🔌 Wire-Format Compatibility Audit

This PR adds database/collection/schema surface for temporal summaries.

Findings: Required Action. The exact-field metadata validator and partition validator may be valid only if the later #14433 fields are intentionally non-metadata or deferred; that disposition must be public before merge. Otherwise, the new schema will reject contract fields the live ticket says should exist.

🔗 Cross-Skill Integration Audit

• Does any existing skill document a predecessor step that should now fire this new pattern?
• Does AGENTS_STARTUP.md §9 Workflow skills list need updating?
• Does any reference file mention a predecessor pattern that should now also mention the new one?
• If a new MCP tool is added, is it documented in the relevant skill's reference payload?
• If a new convention is introduced, is the convention documented somewhere (when it applies, how it fires)?

Findings: All checks pass for skill integration. The PR updates ADR 0024 and keeps the new graph vocabulary in a pure schema module; no skill workflow or MCP tool doc update is introduced by this leaf.

🧪 Test-Execution & Location Audit

• Branch checked out locally (e.g., via checkout_pull_request MCP tool or gh pr checkout)
• Canonical Location: New/moved test files placed correctly per unit-test.md (e.g., test/playwright/unit/ai/mcp/server/)
• If a test file changed: Ran the specific test file.
• If code changed: Verified if there are tests, or if new tests are needed.

Findings: Test failures and coverage gaps flagged in Required Actions. Local evidence: NEO_CHROMA_PORT_TEST=18188 npm run test-unit -- test/playwright/unit/ai/graph/temporalSummarySchema.spec.mjs passed 12/12; NEO_CHROMA_PORT_TEST=18189 npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs test/playwright/unit/ai/services/memory-core/managers/ChromaManager.spec.mjs passed 26/26 after materializing ignored configs; git diff --check origin/dev...HEAD passed. Direct cache probe failed for temporal-summary invalidation. GitHub unit is red.

📋 Required Actions

To proceed with merging, please address the following:

• Reconcile the live #14433 schema addenda before closing #14433. Either fold the accepted direction/window/observer/event/author/release vocabulary into schema/docs/tests, or publish an owner-backed disposition that explicitly defers/removes those addenda and adjust the PR close-target framing.
• Add temporalSummary to ChromaManager.invalidateCollectionCache() for both targeted invalidation and all, then add a regression that proves stale temporalSummaryCollection and _temporalSummaryCollectionPromise are cleared.
• Extend test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs to cover collections.temporalSummaryProd, temporalSummaryTest, and the resolved per-worker test collection formula, matching the existing memory/session pattern.
• Keep the PR blocked on red unit evidence until the unrelated #14743 hard failure lands and the ADR-0019 C3 overlay-import failure is routed to an active reopen/successor ticket. Do not describe the overlay collision as merely known flaky while #11976/#11983 are closed and the tree still has 83 unit-test config.mjs imports.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.

• [ARCH_ALIGNMENT]: 72 - Good module placement and explicit collection branches, but contract drift and incomplete lifecycle invalidation block approval.
• [CONTENT_COMPLETENESS]: 68 - Core storage surfaces are present; live ticket addenda, config coverage, and cache invalidation are incomplete.
• [EXECUTION_QUALITY]: 66 - Focused tests pass, but direct probe finds a real stale-cache bug and GitHub unit is red.
• [PRODUCTIVITY]: 78 - The PR advances a needed leaf and avoids bundling writer logic, but the close-target overclaim would create review/repair churn if merged as-is.
• [IMPACT]: 82 - Temporal-summary substrate is high-impact for the pyramid lane once contract-complete.
• [COMPLEXITY]: 70 - Moderate-high due to cross-cutting collection lifecycle and evolving schema contract.
• [EFFORT_PROFILE]: Architectural Pillar - This is foundational Brain storage substrate consumed by later temporal-pyramid leaves.

Please re-request review after the contract disposition and invalidation/test updates are at a new head. A2A will reference this review id once GitHub returns it.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 2 follow-up / re-review

Opening: Rechecking the prior Request Changes review at exact head bb3e54a37b3c74d16c0880f47701f4cfda21103e: the stale red-CI blocker is cleared, but the close-target contract and two lifecycle/test gaps still reproduce.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Prior review PRR_kwDODSospM8AAAABE-4VIg; current PR view/checks/body/reviews; live #14433 body and comments; ADR 0019 read gate for the config-template touch; exact-head git grep / git show against origin/pr/14733; current ChromaManager, HealthService, temporal schema, and config-template spec surfaces.
• Expected Solution Shape: The current delta should either implement or publicly dispose #14433's live direction/window/observer/event/author/release addenda before closing #14433. The new temporalSummary collection should join the same collection lifecycle as memory/session/graph, including stale-handle invalidation, and its config-template Prod/Test/formula leaves should have unit coverage matching the existing memory/session pattern.
• Patch Verdict: Improves the prior state on CI/freshness: gh pr checks 14733 --watch=false is all green and mergeStateStatus is CLEAN. It still contradicts the expected shape because the schema/spec remain five-field only against a larger live #14433 contract, HealthService now retries through invalidateCollectionCache('temporalSummary') while ChromaManager ignores that type, and the config-template spec does not assert the new temporal-summary leaves/formula.
• Premise Coherence: coheres: the re-review honors verify-before-assert by dropping the stale red-unit/overlay blocker once exact-head checks passed; conflicts: the PR still claims Resolves #14433 / Residual: none before the live source-of-authority addenda are folded or explicitly disposed.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The PR remains the right leaf and is close, so this is not Drop+Supersede. The remaining blockers are merge-gate correctness issues: close-target truth, a real stale-handle retry gap, and missing config-template coverage for a new collection name.

⚓ Prior Review Anchor

• PR: #14733
• Target Issue: #14433
• Prior Review Comment ID: PRR_kwDODSospM8AAAABE-4VIg
• Author Response Comment ID: N/A — no separate author-response comment observed; current re-review is against the moved head.
• Latest Head SHA: bb3e54a

🔁 Delta Scope

Summarize what changed since the prior review:

• Files changed: Same PR surface at the new head: temporal schema/spec, Memory Core config template, Chroma/Storage/Database/Health/guard plumbing, ADR 0024 row.
• PR body / close-target changes: Still Resolves #14433; PR body still says Residual: none.
• Branch freshness / merge state: clean; all GitHub checks successful at exact head.

✅ Previous Required Actions Audit

For each prior Required Action, mark the current state:

• Addressed: Keep the PR blocked on red unit evidence until the unrelated hard failure lands and overlay-import routing is resolved — exact-head GitHub checks are all green and the stale CI premise is no longer active for this PR.
• Still open: Reconcile the live #14433 schema addenda before closing #14433 — live comments still include the direction slot/breakdown, windowSemantics, duration event-pairs, observerBoundary, event-class taxonomy, authorKind, and release window-class addenda; exact-head grep shows no matching schema/spec fold except unrelated existing windowSemantics lines in ADR 0024's business-goal section.
• Still open: Add temporalSummary to ChromaManager.invalidateCollectionCache() for targeted and all invalidation — exact-head HealthService calls ChromaManager.invalidateCollectionCache('temporalSummary'), but the manager still only clears memory/summary/graph and silently ignores the new type.
• Still open: Extend test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs for temporalSummaryProd, temporalSummaryTest, and the active formula — the implementation adds the sanctioned ADR-0019 Prod/Test/formula leaves, but the spec still asserts only memory/session/graph collection resolution.

🔬 Delta Depth Floor

• Delta challenge: The freshness/CI delta is real, but it does not falsify the close-target blocker: the PR body still closes #14433 with Residual: none while #14433's own live thread records unmerged schema obligations for this leaf.

🔎 Conditional Audit Delta

Expanded affected dimensions only: source-of-authority/contract, wire-format schema, and test execution. MCP OpenAPI and cross-skill integration remain out of scope for this delta.

📜 Source-of-Authority Audit

Findings: Required Action remains. The live #14433 conversation is the source-of-authority for the leaf's schema contract, and it still contains accepted/owner addenda not represented by the current schema or PR-body disposition. ADR 0019 read-gate is satisfied for the config-template touch; the implementation shape there is correct, but coverage is missing.

🔌 Wire-Format Compatibility Audit

Findings: Required Action remains. validateTemporalSummaryMetadata() still rejects unknown metadata fields by design while #14433's live record demands additional record/schema vocabulary. If those fields are non-metadata, deferred, or removed, that disposition needs to be public before this PR can close #14433.

🧪 Test-Execution & Location Audit

• Changed surface class: code + tests + decision-record docs.
• Location check: temporal schema spec placement is correct; missing coverage belongs in the existing test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs and test/playwright/unit/ai/services/memory-core/managers/ChromaManager.spec.mjs surfaces.
• Related verification run: gh pr checks 14733 --watch=false -> all checks pass at exact head. Static exact-head checks still prove the missing tests: no config-template spec assertions for temporal-summary leaves/formula, and the ChromaManager invalidation regression does not cover _temporalSummaryCollectionPromise / temporalSummaryCollection.
• Findings: fail on coverage completeness, not current CI status.

📑 Contract Completeness Audit

• Findings: contract drift still flagged. The ticket has a Contract Ledger plus later owner/peer addenda; the implemented schema matches the original five-field ledger but not the later live addenda, and the PR body does not document a deferral/removal disposition.

📊 Metrics Delta

Verdict weights still apply: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.

Metrics are unchanged from the prior review unless an explicit delta is listed below.

• [ARCH_ALIGNMENT]: 72 -> 74 - CI/freshness improved, and the config shape follows ADR 0019, but collection lifecycle and live contract drift still block approval.
• [CONTENT_COMPLETENESS]: 68 -> 70 - Red CI is gone; #14433 close-target completeness is still not established.
• [EXECUTION_QUALITY]: 66 -> 72 - Exact-head checks are green, but the stale-handle invalidation path and config-template guard remain incomplete.
• [PRODUCTIVITY]: 78 -> 80 - This is now a narrower repair: three precise blockers rather than broad CI uncertainty.
• [IMPACT]: unchanged from prior review - Temporal-summary substrate remains high-impact for the temporal-pyramid lane once contract-complete.
• [COMPLEXITY]: unchanged from prior review - Moderate-high due to cross-cutting Memory Core lifecycle and evolving schema authority.
• [EFFORT_PROFILE]: unchanged from prior review - Architectural Pillar; foundational Brain storage substrate consumed by later leaves.

📋 Required Actions

To proceed with merging, please address the following:

• Reconcile #14433's live schema addenda before this PR closes #14433. Either fold the accepted direction/window/observer/event/author/release vocabulary into schema/docs/tests, or post an owner-backed disposition that explicitly defers/removes those addenda and adjust the PR body close-target/residual framing.
• Add temporal-summary cache invalidation to ChromaManager.invalidateCollectionCache() for both targeted temporalSummary invalidation and all, then extend the regression so stale temporalSummaryCollection and _temporalSummaryCollectionPromise are proven cleared.
• Extend test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs to assert collections.temporalSummaryProd, temporalSummaryTest, and the resolved per-worker test formula, matching the existing memory/session template coverage.

📨 A2A Hand-Off

After posting this follow-up review, capture the new commentId and send it via A2A to the next actor so they can fetch the delta directly.