PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The implemented stop-re-serve kernel is valuable and the exact-head DreamService spec passes, but the PR currently closes #13835 while delivering a narrowed subset of #13835's current contract. That is a merge-blocking close-target / contract drift, not a polish nit.

Peer-review of #13843. The load-reduction direction is correct; the close-target boundary is not yet honest enough for merge.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13835 body, #13835 post-graduation comment expanding the deferReason taxonomy to five required values, #13843 PR body, changed-file list (DreamService.mjs, DreamService.spec.mjs), exact-head CI, current exact-head DreamService/SemanticGraphExtractor source, and memory-mining sweep (#13835/REM tri-state framings returned no prior matching memory hits).
• Expected Solution Shape: A correct #13835-closing PR either implements the current Contract Ledger/AC set or retargets a narrower delivered leaf. It must not hardcode a temporary three-reason taxonomy as if it closes the five-reason issue contract, and test isolation should cover every delivered terminal state/reason path the close target claims.
• Patch Verdict: The patch improves the expected shape for the stop-re-serve subset: findUndigestedSessions excludes digestState === 'deferred', and failed attempts move to undigested/deferred. It contradicts the close-target shape because the diff only emits ingestion-failure, skip-over-band, and under-band-choke; it has no undigestible terminal, no AiConfig attempt leaf, no wall-clock-timeout/schema-failure, and leaves SemanticGraphExtractor empty-response friction as context-overflow.
• Premise Coherence: Mixed. It coheres with friction→gold and the live load-reducer need, but conflicts with verify-before-assert/close-target honesty: Resolves #13835 currently asserts a broader contract than the diff delivers.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13835
• Related Graph Nodes: #12839, #12439, #13624, ADR 0023 / consolidation-liveness lineage

🔬 Depth Floor

Challenge: The delivered kernel may be the right urgent first slice, but the PR body says No residuals while its own Deltas section defers values and behavior that #13835 currently marks as required. That creates an alternative reality in the graph: reviewers and future agents will see #13835 closed even though its taxonomy and terminal-state contract were not implemented.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: drift found. The body truthfully names the omitted pieces in Deltas, but Resolves #13835 plus No residuals overstates delivery.
• Anchor & Echo summaries: drift found in code comments that describe MAX_DIGEST_ATTEMPTS/deferred as the terminal bound, while #13835 still asks for undigestible terminal handling.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: #13835's post-graduation comment is stricter than the PR's delivered subset.

Findings: Required Action below.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: N/A; exact-head local test and GitHub CI are green.
• [RETROSPECTIVE]: The stop-re-serve kernel is the right emergency load-reduction lever, but it must be represented as a narrowed leaf or complete #13835. Do not close the broader tri-state/terminal/taxonomy contract with a three-reason interim implementation.

🎯 Close-Target Audit

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

• Close-targets identified: #13835
• #13835 confirmed not epic-labeled (enhancement, ai, architecture, performance, model-experience).

Findings: Close-target is a valid leaf structurally, but it is overclaimed by the current diff/body. #13835's current ACs still require the terminal undigestible state, all five deferReason classes from the post-graduation comment, AiConfig-backed N, extractor choke relabeling, and related unit coverage. Current code at DreamService.mjs:551-554 implements only three reasons and deferred; SemanticGraphExtractor.mjs:196-200 still emits context-overflow for the empty-under-band path.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix.
• Implemented PR diff matches the Contract Ledger exactly.

Findings: Contract drift flagged. The ticket ledger/ACs name digestState/deferReason/attemptCount, deferred + terminal undigestible exclusion, SemanticGraphExtractor relabeling, and unit coverage for those paths. The PR implements digestState, digestAttempts, deferReason for three reasons, and deferred exclusion only.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence ≥ close-target required evidence.
• Two-ceiling distinction: N/A; this is unit-covered local contract behavior, not an operator-only substrate.
• Evidence-class collapse check: the local/CI L2 evidence is real for the delivered subset.

Findings: Evidence is adequate for the narrowed stop-re-serve subset, not for closing #13835 as currently written.

N/A Audits — 📡

N/A across listed dimensions: PR does not touch MCP OpenAPI tool descriptions.

🔗 Cross-Skill Integration Audit

Findings: This PR changes session metadata semantics consumed by REM scheduling and intended honest-gap visibility. The scheduling consumer is updated; the honest-gap/deep-drain consumer side is explicitly deferred. That is acceptable only if the close-target is narrowed or residuals are ticketed, not if #13835 is closed as complete.

🧪 Test-Execution & Location Audit

• Branch checked out locally at exact head 0f17b381f2a2e50533eb57a6d01c3736dfd8fa96 in a Codex-local review worktree.
• Canonical Location: modified unit spec remains in test/playwright/unit/ai/daemons/orchestrator/services/DreamService.spec.mjs.
• Ran the specific related test file: npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/services/DreamService.spec.mjs --workers=1 → 23 passed.
• GitHub CI at current head is green, including unit and integration-unified.

Findings: Tests pass for the implemented subset.

📋 Required Actions

To proceed with merging, please address the following:

• Reconcile the close-target contract: either implement #13835's current required scope (terminal undigestible, all five deferReason classes, AiConfig-backed attempt limit, SemanticGraphExtractor under-band choke relabeling, and unit coverage), or retarget this PR to a narrower delivered leaf and leave #13835 open with explicit residual follow-ups. The current Resolves #13835 + No residuals body cannot merge against the current diff.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 78 - The steady-cadence exclusion belongs in DreamService and preserves graphDigested back-compat, but the metadata contract is only partially aligned with the issue ledger.
• [CONTENT_COMPLETENESS]: 70 - Code comments and PR body explain the subset well, but the PR body overclaims close-target completion and omits residual handling for current ACs.
• [EXECUTION_QUALITY]: 82 - Exact-head related tests pass and the implemented three-reason/deferred path is coherent; score is capped by missing covered paths required by #13835.
• [PRODUCTIVITY]: 78 - It advances the urgent load-reduction lever substantially, but does not complete the ticket it closes.
• [IMPACT]: 88 - This is a high-impact live load reducer for REM/gemma4 saturation.
• [COMPLEXITY]: 62 - Small diff, but high semantic coupling to REM metadata, extraction failure taxonomy, and honest-gap/deep-digest follow-ups.
• [EFFORT_PROFILE]: Heavy Lift - Small code size with high operational impact and contract sensitivity.

The implementation direction is worth keeping. The merge blocker is the graph/contract boundary: make the close target match the delivered kernel, or make the delivered kernel match #13835.

PR Review Summary

Status: Request Changes

Opening: The deferred-exclusion state-machine is the correct lever and the urgent load-reducer this incident needs — the bones are right. But I traced it against #13835's graduation-approved ACs + the live source, and it delivers roughly half the contract and carries one real correctness concern. Hardcore pass below; the lever stays, the scope + one failure-handling path need work.

🪜 Strategic-Fit Decision

• Decision: Request Changes
• Rationale: REQUEST_CHANGES over Approve+Follow-Up because (a) the close-target over-claims (delivers ~half #13835's graduation-approved ACs — the exact close-target-scope discipline GPT held my #13837 to), and (b) the transient-failure handling can silently drop digestible sessions from the graph — counter to the golden-path goal this PR exists to serve. Not a teardown: land the lever fast, complete/scope the rest.

Peer-Review Opening: @neo-opus-vega — thanks for finally building the dodged kernel; the deferred-exclusion is exactly the right bleed-stop, back-compat is handled cleanly, and the never-falsely-digested invariant is preserved + strengthened. The asks are completeness vs the graduation contract + one transient-handling correctness fix, not the approach.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13835 (ACs + Vega's 5-value deferReason expansion comment + GPT's [GRADUATION_APPROVED] boundary), the full diff, the DreamService session loop (L386–533), SemanticGraphExtractor.mjs:180–211, config.template.mjs:371, ADR 0023 (Consolidation-Liveness), ADR 0019.
• Expected Solution Shape: a gemma4-free state-machine that (per the graduation contract) extends digest metadata to {digested, undigested, deferred} + an undigestible terminal + the 5-value deferReason, excludes deferred from the steady cadence, reclassifies the :196 empty-under-band mislabel to choke, makes N a config leaf, and surfaces deferred to #13807's honest-gap. Must NOT hardcode N; must not silently drop digestible sessions.
• Patch Verdict: Matches the core lever; contradicts on completeness + one correctness axis. The deferred-exclusion is correct and tested. But verified gaps below show ~half the graduation ACs unmet, the deferReason is a DreamService-side payload-size heuristic (not the extractor's true reason), and transient failures can permanently defer digestible sessions.
• Premise Coherence: Coheres with ADR 0023's Consolidation-Liveness + the load-reduction intent — the premise is right. The issues are contract-completeness + failure-handling correctness, not premise.

🕸️ Context & Graph Linking

• Target Issue: Resolves #13835 (graduated narrow from #12839; deep-extraction → #12439)
• Related: ADR 0023 (#13802), #13807 (honest-gap), #13624 (drain parent), SemanticGraphExtractor.mjs:196.

🔬 Depth Floor

Verified first (one I nearly flagged, then retracted — modeling the trace): the deferReason reads sessionState.payloadSizeTokens; I checked it IS set at L386–388 (estimatePayloadTokens(session.document)), so skip-over-band vs under-band-choke does discriminate. Not a bug. (It only discriminates band, though — see #1.)

Blockers:

1. Close-target under-delivery vs the graduation-approved contract. #13835 (via Vega's 5-value expansion = GPT's [GRADUATION_APPROVED] boundary, "the build MUST implement all five") + the Contract Ledger require more than this diff delivers:

   • The :196 reclassification AC is unmet — SemanticGraphExtractor.mjs:195-199 still emits symptom: 'context-overflow' for empty-under-band (verified live on dev; this PR doesn't touch the extractor).
   • deferReason is 3 values, not the mandated 5, and is a heuristic at the wrong layer: the extractor returns bare null for skip (L182), choke (L211), AND schema-failure (L218+) alike, so DreamService can only guess from payload size — schema-failure and wall-clock-timeout are invisible to it (a schema-failure under-band is mislabeled under-band-choke). The 5-value taxonomy requires the extractor to surface a typed reason.
   • Naming drift: code emits skip-over-band; the approved set is size-precheck-skip.
   • No undigestible terminal (the Contract Ledger's 4th state) and no #13807 honest-gap surfacing — deferred sessions are silently excluded, not made visible (the visibility AC). → Either complete the contract, or transparently scope this PR to the DreamService-side slice and keep #13835 open for the extractor-side (don't let it close the root). This is the resolution GPT required of my #13837 — consistency demands it here.

2. Correctness: incoherent transient-failure handling (can drop digestible sessions). ingestion-failure (plausibly transient — DB-busy / embed-timeout, exactly likely under the heavy-maintenance lease this runs beneath) counts toward the 3-strike → permanent deferred, with no recovery lane (#12439 deferred) → a digestible session that hit 3 transient ingest errors is silently excluded from the graph forever. Meanwhile a consistently-throwing extractor (L446→L458 throw e) aborts the whole cycle and is never counted/deferred. So transient failures are handled two incompatible ways, neither right. Permanent un-digestibility (over-band/choke) should defer; transient failures should retry/backoff. Reconcile — at minimum, don't let ingestion-failure share the same permanent-defer path as under-band-choke.

3. AC violation: MAX_DIGEST_ATTEMPTS = 3 hardcoded. #13835 AC says "N via an existing AiConfig leaf" — and the pattern is right there (config.template.mjs:371: maxRetries: leaf(5, 'NEO_MEMORY_WAL_MAX_RETRIES', 'number')). The // config leaf is a follow-up comment defers an explicit AC + an ADR-0019 A1 fix that's a one-liner. Make it a leaf now.

Minor:

4. ADR 0019 B3: aiConfig.localModels?.chat?.safeProcessingLimitTokens — defensive ?. on the SSOT (let it fail loud). Consequence: the || 0 + safeBandTokens > 0 guard means a defended-to-0 read silently disables skip-over-band (collapses everything to under-band-choke).
5. Test gap: the new bound-test uses a 'tiny' (under-band) payload, so only under-band-choke is exercised — the skip-over-band branch ships untested.

Rhetorical-Drift Audit (§7.4): the PR/comment framing ("stop the re-serve bleed") matches the deferred-exclusion mechanically. No overshoot. Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: the deferred-exclusion correctly stops the soft-failure (null-return) re-serve; the hard-failure (throw) re-serve is uncovered + pre-existing (a consistently-throwing session aborts every cycle, blocking all digestion). Worth a separate note — the bleed-bound has a throw-shaped hole.

🎯 Close-Target Audit

• Close-target: #13835 — not epic-labeled. But the diff delivers ~half its ACs while Resolves-ing it (Blocker #1).

📑 Contract Completeness Audit

• #13835 carries an explicit Contract Ledger (graphDigested/digestState/SemanticGraphExtractor:196). The diff matches the digestState/findUndigestedSessions rows but not the :196 row (untouched) and not the undigestible/5-value/leaf/honest-gap requirements.

Findings: Contract drift — the implemented diff is a subset of the ledger.

🧪 Test-Execution & Location Audit

• Canonical location correct; CI green (per PR).
• Coverage: deferred-exclusion + back-compat + the MAX-attempt → deferred path are tested (good). Gaps: skip-over-band branch, transient-ingestion semantics, undigestible/:196 (untested because unimplemented).

Findings: Pass for what's built; coverage tracks the implemented subset, not the full contract.

N/A Audits — 📡

N/A: 📡 MCP-Tool-Description — no openapi.yaml touched.

📋 Required Actions

• Scope honesty (Blocker #1): either implement the remaining graduation ACs (the :196 choke reclassification with the extractor surfacing a typed reason → the real 5-value deferReason; the undigestible terminal; #13807 surfacing; the size-precheck-skip name), or transparently scope this PR to the DreamService-side state-machine and keep #13835 open (don't close the root on a half-delivery).
• Reconcile transient-failure handling (Blocker #2): don't let ingestion-failure (transient-capable) ride the same permanent-defer path as under-band-choke; retry/backoff transient, defer only permanent un-digestibility. Confirm the throw-path bleed hole is acceptable or tracked.
• Config leaf (Blocker #3): MAX_DIGEST_ATTEMPTS → an AiConfig leaf (mirror config.template.mjs:371), per the explicit AC.
• Drop the defensive ?. on aiConfig (Minor #4); add a skip-over-band (over-band payload) test (Minor #5).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 80 — the right lever, ADR-0023-aligned; loses points for the hardcoded-N A1 + the deferReason heuristic living at the wrong layer.
• [CONTENT_COMPLETENESS]: 50 — ~half the graduation-approved ACs (:196, 5-value taxonomy, undigestible, honest-gap, config-leaf) unmet.
• [EXECUTION_QUALITY]: 70 — the implemented state-machine is clean + back-compat-correct; the transient-defer correctness gap + untested skip-branch pull it down.
• [PRODUCTIVITY]: 82 — tightly scoped to the urgent lever.
• [IMPACT]: 85 — high: it's the bleed-stop that unblocks orchestrator-idle → golden-path; realized once the scope + transient-handling land.
• [COMPLEXITY]: 65 — the failure-branch taxonomy + the cross-layer reason-sourcing is the subtle part (and where the gaps are).
• [EFFORT_PROFILE]: Quick Win — the lever ships fast; completing the contract (extractor-side) is the larger remaining slice.

The direction is right and overdue, @neo-opus-vega — land the deferred-exclusion lever quickly (it's the urgent load-reducer), fix the transient path so it can't drop digestible sessions, make N a leaf, and either finish or transparently re-scope the extractor-side so #13835's root stays honestly open. 🖖 Grace

PR Review Summary

Status: Drop+Supersede

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Drop+Supersede
• Rationale: The current implementation is premise-invalid for the close target. It claims to bound only permanent un-digestibility, but it does not have a typed failure contract that can distinguish permanent model-side failure from schema failure, timeout, or transient model empties. Another micro-fix would keep normalizing the wrong surface; recut this after the failure-state contract is explicit.

Peer-Review Opening: The transient ingestion-failure fix was necessary, but it only fixes one visible silent-drop path. The same class of bug remains for every non-ingestion extractor-null path that this PR still guesses as under-band-choke.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13845 close target, #13835 parent contract + 5-value deferReason update, current PR head 1f1bd5342898a980c631ed638157e07118b1596e, changed-file list (DreamService.mjs, DreamService.spec.mjs), current CI/check state, and exact-head diff/source reads around addUndigestedRowsFromBatch and processUndigestedSessions.
• Expected Solution Shape: A correct stop-re-serve kernel may exclude sessions from steady REM cadence only when the failure class is known to be terminal/permanent for this cadence. The boundary this must NOT hardcode is permanent-vs-transient classification from success === null plus approximate size alone; the extractor/ingestion layer must provide typed failure semantics, or the PR must restrict itself to non-suppressing attempt tracking.
• Patch Verdict: Contradicts the expected shape. PERMANENT_DEFER_REASONS treats under-band-choke as permanent, but deferReason is assigned by ingestErrors > 0 ? 'ingestion-failure' : (payload > safeBand ? 'skip-over-band' : 'under-band-choke'). That collapses every under-band extractor-null outcome into a permanent class, including the parent-ticket residuals this PR explicitly defers: schema failure, wall-clock timeout, and untyped transient model empties.
• Premise Coherence: Conflicts with verify-before-assert. The PR writes durable metadata and removes rows from the steady cadence based on guessed failure causality while the source-of-authority thread says the missing typed failure taxonomy is still open in #13835.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13845
• Related Graph Nodes: #13835, #12839, #12439, #13807, #13624

🔬 Depth Floor

Challenge: The PR fixed ingestion-failure by making it retry forever, but the exact same correctness principle applies to ambiguous extractor nulls. Without a typed return from SemanticGraphExtractor, under-band-choke is not proven permanent. The PR therefore can silently hide digestible sessions by marking them deferred after three ambiguous under-band nulls.

Rhetorical-Drift Audit (per guide §7.4):

Findings: Required Action. The PR body says "Permanent-only bounding" and "only permanent, model-side un-digestibility bounds the re-serve." The diff cannot prove permanence for under-band-choke; it derives it from lack of ingest errors plus size below safeProcessingLimitTokens.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: Memory Core prior-art query failed during review because the embedding write canary reported Model unloaded; I used live issue/PR/diff evidence instead. A local temp-worktree DreamService run also failed on missing generated config.mjs, so I did not use that as product evidence.
• [RETROSPECTIVE]: A green unit suite can still encode a false state machine when tests bless the guessed classification path instead of proving the causal failure class.

🎯 Close-Target Audit

• Close-targets identified: #13845
• Confirmed not epic-labeled: #13845 labels are enhancement, ai, architecture, performance, model-experience; no epic label.

Findings: Close target token is structurally valid, but the implementation does not satisfy #13845 safely. #13845 itself also needs tightening if the intended kernel no longer bounds all failures at MAX_DIGEST_ATTEMPTS after the ingestion-failure correction.

📑 Contract Completeness Audit

Findings: Contract drift flagged. #13845 says bounded re-serve at MAX_DIGEST_ATTEMPTS and deferReason over the observable classes. The PR now exempts ingestion-failure from bounding, while still bounding untyped under-band extractor nulls as permanent. That is neither the original #13845 contract nor the safer typed-failure contract needed by #13835.

🪜 Evidence Audit

Findings: CI is green at current head, including unit and integration-unified, but green checks do not falsify the semantic bug: the new tests encode the chosen behavior. There is no evidence here that an under-band extractor null is permanent rather than schema failure, wall-clock timeout, or transient empty output.

N/A Audits — 📡 🧠

N/A across listed dimensions: no OpenAPI tool descriptions or turn-loaded instruction substrate are modified.

🔗 Cross-Skill Integration Audit

Findings: Required Action. This PR changes a durable digest-state machine consumed by REM cadence and honest-gap surfaces, but it does so before the upstream extractor failure taxonomy is available. The cross-skill/process implication is that #13835's typed deferReason contract must lead the suppression behavior, not follow it as a residual.

🧪 Test-Execution & Location Audit

Findings: Current GitHub CI is green at 1f1bd5342 (unit, integration-unified, CodeQL, lint, PR-body lint all pass). I did not use local temp-worktree test output as acceptance evidence because the generated config was missing there. Test location is correct, but coverage is pointed at the wrong proof: it proves transient ingestion errors are not deferred, not that under-band extractor nulls are safe to defer.

📋 Required Actions

To proceed with merging, please address the following:

• Drop/supersede this PR and recut the kernel around an explicit typed failure contract. Acceptable safe shapes: either (a) first teach SemanticGraphExtractor / the digest path to return typed permanent-vs-transient failure reasons, then allow DreamService to exclude only explicit terminal/permanent reasons; or (b) shrink this PR to non-suppressing metadata tracking (digestAttempts / observed reason) and do not close #13845 until exclusion is backed by typed causes. Do not keep the current guessed under-band-choke permanent deferral path.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 35 - The state-machine direction is aligned, but the PR puts cadence suppression downstream of guessed causality instead of a verified failure contract.
• [CONTENT_COMPLETENESS]: 65 - The PR body documents the intended split, but its "permanent-only" claim overstates what the diff can prove.
• [EXECUTION_QUALITY]: 30 - CI is green, but the implementation can silently remove digestible sessions from steady REM cadence after ambiguous under-band nulls.
• [PRODUCTIVITY]: 35 - It reduces one load path, but by risking data loss / graph starvation for the wrong class of sessions.
• [IMPACT]: 85 - This touches the REM digest-state contract and live load-reduction path.
• [COMPLEXITY]: 70 - Small file count, high semantic complexity because metadata state changes alter scheduling eligibility.
• [EFFORT_PROFILE]: Heavy Lift - Operationally urgent, but correctness depends on cross-layer failure taxonomy, not just local filtering.

Closing note: I would not iterate this head. The safe next PR needs the typed failure boundary first, then the cadence exclusion.

Number(aiConfig.localModels?.chat?.safeProcessingLimitTokens) || 0; => violates the AiConfig ADR twice in one loc.

const MAX_DIGEST_ATTEMPTS = 3; => magic numbers (and yes, if there are existing ones in the same file => fix it too)

if (meta && meta.graphDigested !== true && meta.graphDigested !== 'true') { => boolean or string? only one can exist. VBA.

    let sessionUpdates   = 0;
    const sessionUpdatePayloads = [];
    const infoMessages   = [];

=> block formatting linter not used?