PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise and overall placement are right, so Drop+Supersede or a retrospective split would destroy converged value. Four bounded corrections are still required because exact-head falsifiers exposed runtime/authority defects that contradict #14938 and ADR 0028 despite green CI.

Peer-Review Opening: This is a substantial, coherent completion of the temporal-pyramid lane. The supervised-child/heavy-lease topology, L1/L2 fold, deterministic IDs, and retry-safe retention are the right shape. I reviewed the full 20-file / 16-commit delta at exact head 9b7144a06f4aa0174c835d4348f519a5e44d36ff; the remaining work is convergence in place, not another decomposition.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14938 and its Contract Ledger; the changed-file list; current dev; ADRs 0014, 0019, 0022, 0024, and 0028; the existing orchestrator registry/pipeline/backpressure and Base.ready() lifecycle contracts; the GitHub Discussion sync/query substrate; and the temporal-summary schema/scaffold.
• Expected Solution Shape: One default-off local-only lane, scheduled solely by the Orchestrator as a supervised one-shot child under exclusive-heavy backpressure. Only L1/L2 may reach durable stores; source facts must be complete or fail closed; replay must be deterministic and retention bounded.
• Patch Verdict: Mostly matches and materially improves the expected shape, but four edges contradict it: the Discussion mirror is treated as complete despite 50/20 caps and reply timestamps are misbound; the child externally calls initAsync(); the durable guard runs after the Chroma write; and the ledger/authority prose still describes a different contract.
• Premise Coherence: Coheres with verify-before-assert and the Body↔Brain organism: this is deterministic Brain substrate under the existing fairness topology, not a parallel scheduler. The findings are implementation-boundary misses, not a wrong premise.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14938
• Related Graph Nodes: #14434, #12679, #14811; ADR 0014 local-only topology; ADR 0022 fairness; ADR 0024 deterministic temporal nodes; ADR 0028 temporal pyramid.

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: The implementation's strongest premise is “the synced corpus is complete.” The actual sync query uses comments(first: $maxComments) / replies(first: $maxReplies) without nested pagination metadata, and DiscussionSyncer supplies 50/20. That means the service cannot distinguish complete evidence from silent truncation—the exact confidently-wrong-zero failure #14938 forbids.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: “complete corpus,” “durable/dynamic-boundary guarded,” and the initAsync() readiness framing overstate what the exact head enforces.
• Anchor & Echo summaries: ai/config.template.mjs still describes a standalone daemon/poll interval; the service header says the Orchestrator imports/drives it; version docs disagree on per-run versus contract version.
• [RETROSPECTIVE] tag: N/A — no such tag is used.
• Linked anchors: ADR/ticket anchors establish the intended topology and durable-tier contract.

Findings: Drift is bounded but substantive and maps to Required Action 4.

🧠 Graph Ingestion Notes

• [KB_GAP]: Base.initAsync() is framework-owned; external consumers await ready(). “Complete synced corpus” must be evidenced by pagination/exhaustion state, not inferred from a directory existing.
• [TOOLING_GAP]: Hosted CI and 201/201 focused local specs were green, yet direct falsifiers exposed duplicate initialization, a non-durable Chroma write, and two graduation-parser/source holes. The tests currently bless one of those paths.
• [RETROSPECTIVE]: The large lane is now architecturally coherent; late fan-out was valuable because independent lifecycle, persistence, and evidence audits found narrow boundary defects without reopening the design.

🎯 Close-Target Audit

• Close-targets identified: #14938 only.
• #14938 confirmed not epic-labeled; no parent/sub-issue magic-close collision. Related references are non-closing, and the 16 commit bodies contain no additional magic close targets.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix.
• Implemented PR diff matches it exactly: the ticket ledger omits AiConfig.orchestrator.localOnly.temporalSummaryEnabled and SUMMARY_SESSION, both shipped consumed surfaces.

Findings: Contract drift flagged; synchronize the ticket ledger before approval.

🪜 Evidence Audit

• PR body contains the required greppable Evidence: L<X> ... → L<Y> required ... declaration; the current prose-only Evidence: paragraph is not the ladder declaration.
• Real Chroma upsert, deterministic replay, and supervised-child execution provide material L3 evidence for AC8.
• No operator-gated residual is claimed.
• Evidence-class precision: “50 graph nodes” appears to be the hard searchNodes LIMIT 50, not an exact count against 71 Chroma rows; state it as capped/lower-bound evidence or use listNodeRecordsByType for the exact count.
• AC4 evidence is incomplete because the source cannot prove comment/reply exhaustion and reply event timestamps are not parsed.

Findings: Evidence/AC mismatch flagged for AC4; AC8 itself is materially proven but needs canonical, count-honest declaration.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no OpenAPI tool-description surface changed.

🛂 Provenance Audit

The PR names its origin session and derives the abstraction from #14938 plus ADRs 0014/0022/0024/0028. The deterministic-writer and orchestrator-owner provenance is appropriate; no ungrounded external pattern was imported.

Findings: Pass.

📜 Source-of-Authority Audit

The demands below are source-bound: src/core/Base.mjs:601-604,956-957 mandates ready(); ADR 0028 §§2.1–2.3 requires L3–L5 zero-storage/query-time synthesis; #14938 AC4 requires a complete conversation, event timestamps, prose rejection, and fail-closed absence.

Findings: Exact-head behavior contradicts these authorities at the named boundaries.

🔌 Wire-Format Compatibility Audit

The five-field temporal metadata shape remains compatible, and IDs are deterministic. However, authority still calls version a per-reaggregation counter/new-document contract while this PR implements a stable material-contract version with same-version overwrite. Both policies can be coherent; shipping both descriptions cannot.

Findings: Synchronize schema/ADR/JSDoc to the implemented contract-version policy.

🔗 Cross-Skill Integration Audit

• No new workflow skill or MCP tool trigger is introduced.
• No startup workflow list update is needed.
• Existing architectural references do need updating: temporalSummarySchema.mjs, ADR 0024, config JSDoc, and service/engine JSDoc currently encode predecessor behavior.
• The new convention is otherwise anchored by #14938 and ADR 0028.

Findings: One same-PR authority synchronization gap, included in Required Action 4.

🧪 Test-Execution & Location Audit

• Branch checked out in an isolated exact-head worktree at 9b7144a06f.
• Test locations match the unit-test mirror structure.
• Ran all seven touched unit surfaces with one worker: 201/201 passed.
• Hosted exact-head unit/integration/lint/CodeQL checks are green; structure map and git diff --check pass.
• Direct falsifiers found missing coverage:
  • external initAsync() path: {"calls":2} versus ready() control {"calls":1};
  • weekly persist: {"chromaUpserts":1,"graphNodes":0};
  • tilde-fenced marker counted as an action;
  • reply marker stamped with the parent comment's timestamp;
  • synced conversations cap at 50 comments / 20 replies without exhaustion evidence.

Findings: Existing tests pass, but the four corrective contracts below need regression witnesses.

📋 Required Actions

To proceed with merging, please address the following:

• [P1][AC4] Make graduation evidence complete-or-fail-closed. The current sync uses capped top-level comments/replies without nested pagination or an exhaustion signal. Either paginate the conversation source fully or persist/validate completeness and refuse the window when truncated. Parse #### Reply depth=1 ... on <ISO> as its own event boundary, and reject both backtick- and tilde-fenced examples. Add falsifiers for >50 comments, >20 replies (or explicit incomplete-source rejection), reply timestamps, and ~~~ fences.
• [P1][lifecycle] Replace external await GraphService.initAsync() with await GraphService.ready(). Base explicitly forbids external initAsync(); the fresh-process probe proves the current child executes it twice. Pin the one-shot entry boundary so this cannot regress.
• [P1][ADR 0028] Enforce the durable/dynamic boundary before any store write. Resolve/validate the level before acquiring/upserting the Chroma collection; L3–L5 must produce zero Chroma writes and zero graph writes (reject or no-op consistently). Update the current weekly test, which only checks the graph side and therefore blesses one durable write.
• [P2][authority] Synchronize the shipped contract. Backfill #14938's ledger with the local-only leaf and SUMMARY_SESSION; reconcile temporalSummarySchema.mjs, ADR 0024, and engine parameter docs from “per-reaggregation counter/new doc” to the implemented material contract-version policy; update stale standalone-daemon/source-pending prose; and replace the PR's prose-only evidence paragraph with the canonical L3 declaration plus an exact or explicitly capped graph count.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 76 - The orchestrator-owned one-shot/heavy-lane placement is strong; lifecycle and durable-boundary violations prevent a higher score.
• [CONTENT_COMPLETENESS]: 72 - Broad AC coverage and rich JSDoc, but AC4 completeness and authority synchronization remain open.
• [EXECUTION_QUALITY]: 74 - 201 focused tests and green hosted CI are solid; four direct falsifiers expose missing negative-path coverage.
• [PRODUCTIVITY]: 82 - Sixteen commits converged a difficult substrate without scope escape; the remaining changes are narrow.
• [IMPACT]: 94 - This unlocks the durable temporal-pyramid foundation and future velocity consumers.
• [COMPLEXITY]: 68 - The heavy lift is separated into engine/service/orchestrator seams, but cross-store and source-completeness boundaries still carry high reasoning load.
• [EFFORT_PROFILE]: Architectural Pillar - Crosses deterministic aggregation, storage, graph, scheduling, deployment, and evidence contracts.

The lane should converge in this PR. These are four bounded repairs, not a redesign and not a reason to split or close the work.