Slice 3.3 — periodic classifier: extraction, reconciliation, provenance, barrier - #406
Slice 3.3 — periodic classifier: extraction, reconciliation, provenance, barrier#406Pento95 wants to merge 37 commits into
Conversation
C3 needs it reachable from lib/actions' reversal path without importing lib/pipeline. Mirrors M2.2's gate move.
One bracket owns the classifier drain and reversalInProgress window; the clamp ships as ops spliced into the sweep transaction so it can never land without the reversal. Slices 3.9/3.10 consume this, not the sweep. B is the target entry itself on both arms: rollbackToEntry removes the clicked entry (02-entry-arms.md), not just its successors.
json_set on one key, not a read-modify-write of classifier_status: the classifier writes state/retryCount on the same blob, including on the teardown of the run the bracket just aborted. WHERE guard subsumes both no-op cases, so the clamp needs no ctx. Also: bracket rejects re-entry, extra ops survive an empty row set, concurrency uses the shared kind.
The M1.5 arm orphaned both link tables; the periodic classifier's reconcile path is the first consumer that can produce the orphans.
The reverse-replay engine now stays generic: tables that cascade deletes declare a restoreCascade hook in their registration to extract and re-insert children on undo. This avoids hardcoding domain-specific behavior into the generic engine and prevents accidental key deletion for unrelated tables.
Adds cascadeDeleteOps hook for redo path (was orphaning children on delete→undo→redo). Inverts CascadeRestore to return cascadeKeys so engine strips them (prevents accidental forgetting). Single buildChildDeleteOps helper shared by forward delete and redo.
Round-trip delete→undo→redo test verifies cascaded child deletes sync to stores. CascadeDeleteOps now returns child rows so redo can emit proper delete patches. Test confirms fix catches the bug.
Cosine errors no longer silently read as no-signal. Adds exact TAU_HIGH/TAU_LOW boundary tests, wrong-vector-count and null-description edge cases, and documents cosine's rounding on the export itself.
Every emitted delta carries its own survival anchor: single-turn to that turn, awareness to the turn that narrated the learning, sub-rows to the parent, unattributed to the window head.
occurred_at_entry_id no longer head-falls back: a bogus handle is a story-time claim, so it degrades to temporal and is recorded instead. Awareness and relationship refs now require a character; involvements stay polymorphic. The entity snapshot became a mutable index, so flip guards see same-reply promotes/creates and duplicate flips collapse.
retryDelayForStatus owns the attempt-count to delay mapping; the scheduler re-reads persisted status rather than indexing BACKOFF_MS a second time, so the reducer and the scheduler cannot drift by one.
inFlight/timer were module-global despite a per-branch API: a retry
pending for branch A was destroyed by a cadence tick for branch B.
Key both under a per-branch map. runNow() now returns
StartRunOutcome | {outcome:'busy'} instead of silently declining, so
[Run classifier now] can report why nothing happened.
buildClassifierContext built lore/definition/calendarVocabulary that the template never read; removed from the builder and VARIABLES so the group matches template consumption. The parity test now asserts set equality instead of one-directional presence, since renderTemplate does no context validation. Template also fixes nil-unsafe `size == 0` checks and tightens Liquid whitespace control.
One structured call, one abort-free commit burst, watermark written after the deltas so a crash leaves recovery's reverse-replay consistent.
An empty window can still be advanceable: coversThrough counts capped candidates before the system-entry filter, so returning early left the cadence re-firing forever. Also match narrativePhase's storyId guard.
PLACEHOLDER_FIELDS mirrors plan.ts's resolveRef sites, so it belongs beside the schema; a schema-derived partition test is the link the two lists lacked. Covers the success status write and the reconcile seam, both of which survived mutation.
Narrows the reader's isGenerating to foreground kinds: a no-gate background classifier run was lighting up the streaming placeholder and gating the composer. Persists classifier_status.state: 'running' at run start (key-scoped json_set, restored on abort) so the M7.2 status panel gets its Running state and shouldCadenceFire's guard is live.
state: 'running' has no reporting path and no way out in v1 (the manual-run UI is M7.2), so a crash mid-pass would silently stop the classifier forever — the exact memory-decay failure this milestone exists to prevent. Reset it to idle as a boot-time orphan sweep, key-scoped to $.state so retrying/failed-persistent (real, reported errors) and processedThrough are untouched.
Two specs: the graph-population happy path, and a disambiguation-seam regression test for the "nobody calls configureClassifierEmbedder" defect, invisible to unit tests because each half (injected embedder, boot wiring) passes while the composition is broken. Verified red before the bootstrap fix, green after.
Records the three decisions that constrain future slices: the unbuilt field-scoped entities.status gate, watermark-after-burst and why it is correct against boot recovery, and bracketProseReversal as the only entry to the sweep. Involvement drift outlived the slice and moves to followups; the Storybook full-suite flake is filed there too, confirmed on main.
The pass read its window from entriesStore, which holds only the last ENTRIES_WINDOW_SIZE entries, so older backlog was skipped and then buried under the advancing watermark; it now reads story_entries. Also: handle / placeholder collisions rebound refs, reconciliation scored one namesake on possibly mismatched dims, and pre-flight failures armed no backoff.
Two notes asserted properties the code no longer has: watermark-after was called "coherent by construction" on the premise that boot reverse-replays an interrupted pass (nothing does), and the cosine truncation plus first-match namesake index were recorded as deliberate. Adds the window source, the reserved handle namespace, and pre-flight failure recording.
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (22)
📝 WalkthroughWalkthroughThe PR adds periodic classifier extraction, reconciliation, scheduling, retries, provenance tracking, and E2E coverage. It also adds cascade-aware undo and redo handling, classifier watermark clamping, and separate foreground and background generation state. ChangesPeriodic classifier and pipeline
Cascade-aware undo and redo
Generation state and UI
Pipeline hardening and exports
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ForegroundTurn
participant Bootstrap
participant ClassifierScheduler
participant PeriodicClassifier
participant StructuredGeneration
participant Database
ForegroundTurn->>Bootstrap: emit completed turn
Bootstrap->>ClassifierScheduler: trigger branch evaluation
ClassifierScheduler->>Database: read cadence and classifier state
ClassifierScheduler->>PeriodicClassifier: start classifier run
PeriodicClassifier->>Database: load window and context
PeriodicClassifier->>StructuredGeneration: request structured extraction
StructuredGeneration-->>PeriodicClassifier: return extraction
PeriodicClassifier->>Database: persist actions, status, and watermark
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/actions/story-entries/undo.ts (1)
72-99: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWrap redo inside
bracketProseReversal.
applyRedowrites deltas and patched story rows, butredoLastActiononly setsreversalInProgress; it does not drain the in-flight periodic classifier first. Keep the manual guard/finallyaround the helper, and run the actualapplyRedo()block under the prose-reversal barrier so redo cannot race a live classifier pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/actions/story-entries/undo.ts` around lines 72 - 99, The actual redo application in redoLastAction must run under bracketProseReversal so it cannot race the periodic classifier. Preserve the existing generation guard and finally-based reversalInProgress cleanup, but wrap the applyRedo call and its committed-error handling in the prose-reversal barrier.
🧹 Nitpick comments (12)
lib/classifier/reconcile.ts (2)
23-35: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider rejecting mismatched vector lengths inside
cosine.
cosinescores the shared prefix when the two vectors differ in length.reconcileNewCharacterprevents that case through itsusableguard, so the current behavior is safe.cosineis also exported fromlib/classifier/index.ts, so a future caller can reintroduce the fabricated-similarity failure mode that the guard exists to block. Return 0 on a length mismatch to make the invariant local to the function.♻️ Proposed change
export function cosine(a: Float32Array, b: Float32Array): number { - const n = Math.min(a.length, b.length) + if (a.length !== b.length) return 0 + const n = a.length🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/classifier/reconcile.ts` around lines 23 - 35, Update the exported cosine function to return 0 immediately when the input Float32Array lengths differ, before calculating the shared-length loop; preserve the existing zero-norm handling and similarity calculation for equal-length vectors.
58-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the embedder failure before degrading to
no-signal.The catch block discards the error. A sustained embedder outage then surfaces only as flagged creates on the collision-review surface, with no signal that reconciliation never had vectors. Log the error at warn level so the cause is diagnosable.
♻️ Proposed change
let vectors: Float32Array[] | null = null try { const result = await deps.embedDescriptions([ candidate.description, ...namesakes.map((n) => n.description ?? ''), ]) vectors = result.vectors - } catch { + } catch (error) { + console.warn('[classifier] namesake embedding failed; flagging create', error) vectors = null }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/classifier/reconcile.ts` around lines 58 - 67, Update the catch block around embedDescriptions in the reconciliation flow to capture the thrown error and log it at warn level before setting vectors to null. Preserve the existing no-signal fallback while including enough context and the original error for diagnosis.lib/prompts/templateContextMap.ts (1)
78-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the documented
turnstype to match what the context actually carries.The registry declares
turnsasWindowTurn[], which includesentryIdandposition.buildClassifierContextinlib/pipeline/definitions/classifier-context.tsprojects each turn to{ handle, content }only — the same narrowing pattern already applied tohappeningshere. A reader of this registry can wrongly assumeentryIdorpositionare available to the template.📝 Proposed fix
{ name: 'turns', - type: 'WindowTurn[]', + type: '{ handle, content }[]', category: 'Story', description: 'Unclassified prose window, each turn carrying the provenance handle (t1..tN) the model must tag its facts with.', required: true, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/prompts/templateContextMap.ts` around lines 78 - 86, Update the `classifierContext` registry entry’s `turns` type from `WindowTurn[]` to the projected `{ handle: string; content: string }[]` shape produced by `buildClassifierContext`, matching the existing narrowed `happenings` declaration and removing the implication that `entryId` or `position` are available.lib/classifier/window.ts (1)
19-29: 🩺 Stability & Availability | 🔵 TrivialGuard against a non-positive
maxEntries.If
maxEntriesis0or negative,candidates.slice(0, maxEntries)always returns an empty array.coversThroughthen falls back toflooron every call and never advances past it. The classifier keeps reprocessing the same window forever without making progress.Add an explicit guard so a misconfigured
maxEntriesfails loudly instead of stalling silently.🛡️ Proposed guard
export function buildClassifierWindow(args: { entries: readonly StoryEntry[] processedThrough: number | null maxEntries: number }): ClassifierWindow { const { entries, processedThrough, maxEntries } = args + if (maxEntries <= 0) { + throw new RangeError(`maxEntries must be positive, got ${maxEntries}`) + } const floor = processedThrough ?? 0Please confirm whether the caller (app settings schema) already enforces a minimum of 1 for this value; if so, this guard is still cheap insurance against a future caller that does not.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/classifier/window.ts` around lines 19 - 29, Add an explicit validation at the start of buildClassifierWindow that rejects maxEntries values less than 1, using the project’s established error type or assertion style so misconfiguration fails immediately before candidates.slice is reached.lib/db/app-settings/app-settings-schema.ts (1)
86-89: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider an upper bound for
classifierWindowMaxEntries.The field accepts any positive safe integer. A large value makes
readWindowEntriesload that many rows andbuildClassifierContextrender them all into one prompt, which can exceed the model context window. An explicit.max()keeps the truncation cap self-limiting.♻️ Proposed bound
- classifierWindowMaxEntries: z.number().int().positive().default(20), + classifierWindowMaxEntries: z.number().int().positive().max(200).default(20),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/db/app-settings/app-settings-schema.ts` around lines 86 - 89, Update the classifierWindowMaxEntries schema definition to enforce an explicit maximum value in addition to its existing positive-integer validation and default, using the project’s intended safe prompt-size bound.e2e/harness/seed.ts (1)
110-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd row-existence preconditions to the three new seed helpers. Each helper casts a query result to a non-optional shape without checking that the target row exists. A wrong id then either throws an opaque
TypeErrorinsidebeforeAllor silently produces a setup the spec cannot detect.
e2e/harness/seed.ts#L110-L113: type thestoriesrow as optional and throw an error that names the missingstoryId.e2e/harness/seed.ts#L138-L159: assert thatbranchIdexists before the insert loop, becauseCOALESCE(MAX(position), 0)and theUPDATE branchesboth succeed for an unknown branch.e2e/harness/seed.ts#L169-L176: type theapp_settingsrow as optional and throw an error when thesingletonrow is absent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/harness/seed.ts` around lines 110 - 113, Add row-existence checks to the three seed helpers in e2e/harness/seed.ts: at lines 110-113, type the stories query result as optional and throw an error naming the missing storyId before parsing settings; at lines 138-159, verify branchId exists before the insert loop; at lines 169-176, type the app_settings query result as optional and throw when the singleton row is absent.lib/pipeline/definitions/periodic-classifier.test.ts (1)
67-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the
clamp(0)call at line 74.At line 74
classifier_statusis still NULL, because thebranchesinsert at line 60 does not set it.json_setreturns NULL when its first argument is NULL, so the statement writes NULL and changes nothing. The next statement at line 75 replaces the whole column anyway. The call reads as a baseline seed that it is not.♻️ Proposed cleanup
- clamp(0) sqlite.prepare(`UPDATE branches SET classifier_status = ? WHERE id = 'b1'`).run(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/pipeline/definitions/periodic-classifier.test.ts` around lines 67 - 84, Remove the clamp helper and its clamp(0) invocation from the test setup before the classifier_status initialization; the subsequent UPDATE that seeds the complete classifier_status JSON should remain unchanged.lib/actions/delta/redo.ts (1)
34-34: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueKey
cascadeInfoby table and target ID.
cascadeInfouses onlydelta.targetIdas the key. A single redo batch can contain delete deltas for more than one registered table. If two such deltas ever share atargetId, the secondsetoverwrites the first, and the post-commit loop emits child delete patches for the wrong parent. Current ID prefixes make this collision unlikely, so this is a hardening change rather than a live defect.♻️ Proposed composite key
- if (entry.cascadeDeleteOps) { + if (entry.cascadeDeleteOps) { const { ops: childOps, children } = await entry.cascadeDeleteOps( delta.branchId, delta.targetId, ctx, ) ops.push(...childOps) - cascadeInfo.set(delta.targetId, children) + cascadeInfo.set(`${delta.targetTable}:${delta.targetId}`, children) }- const children = cascadeInfo.get(delta.targetId) + const children = cascadeInfo.get(`${delta.targetTable}:${delta.targetId}`)Also applies to: 69-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/actions/delta/redo.ts` at line 34, Update cascadeInfo and its access sites in the redo flow to use a composite key containing both the registered table and delta.targetId, ensuring delete cascades from different tables cannot overwrite each other. Preserve the existing child-delete patch behavior by using the same composite key when populating and reading cascadeInfo.lib/actions/happenings/register-happenings.ts (1)
153-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the repeated child predicates.
The same
and(eq(branchId), eq(happeningId))expression appears four times, twice per child table. Bind one predicate per table and reuse it for the select and the delete. This keeps the read filter and the delete filter provably identical.♻️ Proposed refactor
async function buildChildDeleteOps(branchId: string, happeningId: string, ctx: DbCtx) { - const involvements = await ctx.db - .select() - .from(happeningInvolvements) - .where( - and( - eq(happeningInvolvements.branchId, branchId), - eq(happeningInvolvements.happeningId, happeningId), - ), - ) - const awareness = await ctx.db - .select() - .from(happeningAwareness) - .where( - and( - eq(happeningAwareness.branchId, branchId), - eq(happeningAwareness.happeningId, happeningId), - ), - ) + const invWhere = and( + eq(happeningInvolvements.branchId, branchId), + eq(happeningInvolvements.happeningId, happeningId), + ) + const awareWhere = and( + eq(happeningAwareness.branchId, branchId), + eq(happeningAwareness.happeningId, happeningId), + ) + const involvements = await ctx.db.select().from(happeningInvolvements).where(invWhere) + const awareness = await ctx.db.select().from(happeningAwareness).where(awareWhere) const ops = [ - ctx.db - .delete(happeningInvolvements) - .where( - and( - eq(happeningInvolvements.branchId, branchId), - eq(happeningInvolvements.happeningId, happeningId), - ), - ) - .toSQL(), - ctx.db - .delete(happeningAwareness) - .where( - and( - eq(happeningAwareness.branchId, branchId), - eq(happeningAwareness.happeningId, happeningId), - ), - ) - .toSQL(), + ctx.db.delete(happeningInvolvements).where(invWhere).toSQL(), + ctx.db.delete(happeningAwareness).where(awareWhere).toSQL(), ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/actions/happenings/register-happenings.ts` around lines 153 - 192, Update buildChildDeleteOps to hoist one branchId/happeningId predicate for happeningInvolvements and one for happeningAwareness, then reuse each corresponding predicate in both the select and delete queries so their filters remain identical.lib/actions/story-entries/prose-reversal.test.ts (1)
99-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a present status with
processedThrough: null.The suite covers
processedThroughabove the clamp, below the clamp, and a fully absentclassifierStatus. It does not cover a stored status whoseprocessedThroughisnull. Thestatushelper already acceptsnull, so the case is one call. For that rowjson_extract(classifier_status, '$.processedThrough')yields SQL NULL, the>comparison yields NULL, and the update must not match. Pinning that behavior protects the branch that has a status blob but no watermark yet.💚 Proposed test
it('no-ops when the branch has no classifier status yet', async () => { const { runInTransaction, read } = await seedBranch(null) await runInTransaction(classifierWatermarkClampOps('b1', 5)) expect(await read()).toBeNull() }) + + it('no-ops when the status exists but has no watermark yet', async () => { + const { runInTransaction, read } = await seedBranch(status(null)) + await runInTransaction(classifierWatermarkClampOps('b1', 5)) + expect(await read()).toEqual(status(null)) + }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/actions/story-entries/prose-reversal.test.ts` around lines 99 - 109, Add a test beside the existing classifier watermark clamp no-op cases using seedBranch(status(null)), invoke classifierWatermarkClampOps('b1', 5), and assert the stored status remains status(null). This should specifically cover a present classifier status whose processedThrough value is null without changing the existing absent-status or numeric watermark cases.lib/actions/delta/registry.ts (1)
19-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one shape for cascaded child rows.
CascadeRestorereturns children as an array of{ table, rows }.CascadeDeleteOpsreturns children asRecord<table, rows>. Both express the same concept: child table name to rows. Two shapes force each consumer to handle a different traversal (for..ofin reverse-replay,Object.entriesinapplyRedo). Aligning both on the record form would simplify the engine code and reduce the chance of mismatched handling in future cascade domains.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/actions/delta/registry.ts` around lines 19 - 37, Unify cascaded child-row output on the record shape used by CascadeDeleteOps: update CascadeRestore to return children keyed by table name, then adjust its consumers to iterate the record consistently while preserving the existing rows and cascadeKeys behavior.lib/actions/happenings/register-happenings.test.ts (1)
215-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated seed block.
Four tests repeat the same setup: create
hap_1throughapplyDeltaAction, then insertinv_1andhaw_1. The only variation isdecayResistance(nullversus0.5). Extract one helper, for exampleseedHappeningWithChildren(db, ctx, { decayResistance }), and call it from each test. This removes about 80 duplicated lines and keeps the child-row fixture in one place.Also applies to: 261-312, 314-409, 447-517
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/actions/happenings/register-happenings.test.ts` around lines 215 - 259, Extract the repeated happening setup from the affected tests into a shared helper such as seedHappeningWithChildren, accepting db, ctx, and decayResistance. Move the createHappening action plus happeningInvolvements and happeningAwareness inserts into that helper, then replace each duplicated setup block in the referenced tests with helper calls while preserving each test’s decayResistance value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/actions/classifier/deps.ts`:
- Around line 70-85: Update recordClassifierPreflightFailure to persist the
complete status returned by nextStatusOnFailure as a single JSON value, using
Drizzle’s JSON serialization rather than json_set path updates. Replace the
partial-field update while preserving the existing branch lookup and
failure-state calculation, so classifier_status always contains all required
ClassifierStatus fields.
In `@lib/actions/happenings/register-happenings.ts`:
- Around line 153-200: Update buildChildDeleteOps and its
deleteHappening/applyRedo callers so child-row reads and deletes are serialized
per happening: either acquire the same happening-specific lock used by
applyDeltaAction before reading, or move both SELECTs and DELETE operations into
one transaction/critical section. Ensure concurrent creation of
happeningInvolvements or happeningAwareness rows cannot occur between building
the undo payload and deleting the rows.
In `@lib/actions/story-entries/operational.test.ts`:
- Around line 314-316: Update the no-op rollback test around rollbackToEntry to
capture its return value and assert the successful status before checking
branches.processedThrough. Follow the sibling test’s success-status assertion
pattern so a rejection such as STORY_ENTRY_REJECTION.inFlight cannot satisfy the
clamp assertion.
In `@lib/actions/story-entries/prose-reversal.ts`:
- Around line 13-23: The awaitRunTerminal call in bracketProseReversal searches
globally by PERIODIC_CLASSIFIER_KIND and may cancel a classifier run on a
different branch. Either scope the classifier wait to the current branch before
calling awaitRunTerminal, or document that global classifier shutdown is
intentional behavior. Additionally, add a bounded wait with a timeout when
awaiting run.terminal since the current unbounded await has no visible timeout
protection while classifier commit and abort operations lack explicit timeout
guarantees.
In `@lib/pipeline/definitions/classifier-context.ts`:
- Around line 15-21: Update classifierContextMap’s context construction so
entities are mapped to only id, kind, name, description, status, and
injectionMode before being passed to substituteIds. Preserve the existing turns
and happenings projections, and prevent additional Entity row fields from
reaching the model prompt.
In `@lib/stores/generation/generation.ts`:
- Around line 134-138: Update awaitRunTerminal to require branchId and select
the run matching both branchId and kind before aborting or awaiting it.
Propagate the new argument through the reader, story settings, and
prose-reversal callers, and add a regression test proving waits and cancellation
remain isolated between two branches.
---
Outside diff comments:
In `@lib/actions/story-entries/undo.ts`:
- Around line 72-99: The actual redo application in redoLastAction must run
under bracketProseReversal so it cannot race the periodic classifier. Preserve
the existing generation guard and finally-based reversalInProgress cleanup, but
wrap the applyRedo call and its committed-error handling in the prose-reversal
barrier.
---
Nitpick comments:
In `@e2e/harness/seed.ts`:
- Around line 110-113: Add row-existence checks to the three seed helpers in
e2e/harness/seed.ts: at lines 110-113, type the stories query result as optional
and throw an error naming the missing storyId before parsing settings; at lines
138-159, verify branchId exists before the insert loop; at lines 169-176, type
the app_settings query result as optional and throw when the singleton row is
absent.
In `@lib/actions/delta/redo.ts`:
- Line 34: Update cascadeInfo and its access sites in the redo flow to use a
composite key containing both the registered table and delta.targetId, ensuring
delete cascades from different tables cannot overwrite each other. Preserve the
existing child-delete patch behavior by using the same composite key when
populating and reading cascadeInfo.
In `@lib/actions/delta/registry.ts`:
- Around line 19-37: Unify cascaded child-row output on the record shape used by
CascadeDeleteOps: update CascadeRestore to return children keyed by table name,
then adjust its consumers to iterate the record consistently while preserving
the existing rows and cascadeKeys behavior.
In `@lib/actions/happenings/register-happenings.test.ts`:
- Around line 215-259: Extract the repeated happening setup from the affected
tests into a shared helper such as seedHappeningWithChildren, accepting db, ctx,
and decayResistance. Move the createHappening action plus happeningInvolvements
and happeningAwareness inserts into that helper, then replace each duplicated
setup block in the referenced tests with helper calls while preserving each
test’s decayResistance value.
In `@lib/actions/happenings/register-happenings.ts`:
- Around line 153-192: Update buildChildDeleteOps to hoist one
branchId/happeningId predicate for happeningInvolvements and one for
happeningAwareness, then reuse each corresponding predicate in both the select
and delete queries so their filters remain identical.
In `@lib/actions/story-entries/prose-reversal.test.ts`:
- Around line 99-109: Add a test beside the existing classifier watermark clamp
no-op cases using seedBranch(status(null)), invoke
classifierWatermarkClampOps('b1', 5), and assert the stored status remains
status(null). This should specifically cover a present classifier status whose
processedThrough value is null without changing the existing absent-status or
numeric watermark cases.
In `@lib/classifier/reconcile.ts`:
- Around line 23-35: Update the exported cosine function to return 0 immediately
when the input Float32Array lengths differ, before calculating the shared-length
loop; preserve the existing zero-norm handling and similarity calculation for
equal-length vectors.
- Around line 58-67: Update the catch block around embedDescriptions in the
reconciliation flow to capture the thrown error and log it at warn level before
setting vectors to null. Preserve the existing no-signal fallback while
including enough context and the original error for diagnosis.
In `@lib/classifier/window.ts`:
- Around line 19-29: Add an explicit validation at the start of
buildClassifierWindow that rejects maxEntries values less than 1, using the
project’s established error type or assertion style so misconfiguration fails
immediately before candidates.slice is reached.
In `@lib/db/app-settings/app-settings-schema.ts`:
- Around line 86-89: Update the classifierWindowMaxEntries schema definition to
enforce an explicit maximum value in addition to its existing positive-integer
validation and default, using the project’s intended safe prompt-size bound.
In `@lib/pipeline/definitions/periodic-classifier.test.ts`:
- Around line 67-84: Remove the clamp helper and its clamp(0) invocation from
the test setup before the classifier_status initialization; the subsequent
UPDATE that seeds the complete classifier_status JSON should remain unchanged.
In `@lib/prompts/templateContextMap.ts`:
- Around line 78-86: Update the `classifierContext` registry entry’s `turns`
type from `WindowTurn[]` to the projected `{ handle: string; content: string
}[]` shape produced by `buildClassifierContext`, matching the existing narrowed
`happenings` declaration and removing the implication that `entryId` or
`position` are available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b4efed80-437a-4016-b5a0-3f8bcfb3ba47
⛔ Files ignored due to path filters (3)
docs/followups.mdis excluded by!docs/**docs/implementation/milestones/03-memory-floor/slices/03-classifier.mdis excluded by!docs/**lib/prompts/bundled/__snapshots__/periodic-classifier.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (64)
app/reader-composer/[branchId].tsxapp/story-settings/[storyId].tsxcomponents/compounds/generation-status-pill.tsxe2e/harness/mock-llm.tse2e/harness/seed.tse2e/tests/periodic-classifier.spec.tslib/actions/classifier/deps.test.tslib/actions/classifier/deps.tslib/actions/classifier/run-now.test.tslib/actions/classifier/run-now.tslib/actions/delta/redo.tslib/actions/delta/registry.tslib/actions/delta/reverse-replay.test.tslib/actions/delta/reverse-replay.tslib/actions/embedder-swap/index.tslib/actions/happenings/register-happenings.test.tslib/actions/happenings/register-happenings.tslib/actions/index.tslib/actions/story-entries/operational.test.tslib/actions/story-entries/operational.tslib/actions/story-entries/prose-reversal.test.tslib/actions/story-entries/prose-reversal.tslib/actions/story-entries/undo.test.tslib/actions/story-entries/undo.tslib/boot/bootstrap.tslib/boot/classifier-scheduler.test.tslib/classifier/index.tslib/classifier/kind.tslib/classifier/plan.test.tslib/classifier/plan.tslib/classifier/reconcile.test.tslib/classifier/reconcile.tslib/classifier/scheduler.test.tslib/classifier/scheduler.tslib/classifier/schema.test.tslib/classifier/schema.tslib/classifier/status.test.tslib/classifier/status.tslib/classifier/substitute.test.tslib/classifier/substitute.tslib/classifier/window.test.tslib/classifier/window.tslib/db/app-settings/app-settings-defaults.tslib/db/app-settings/app-settings-schema.test.tslib/db/app-settings/app-settings-schema.tslib/pipeline/__tests__/concurrency-entry.test.tslib/pipeline/__tests__/preflight-orchestration.test.tslib/pipeline/definitions/classifier-context.test.tslib/pipeline/definitions/classifier-context.tslib/pipeline/definitions/periodic-classifier.test.tslib/pipeline/definitions/periodic-classifier.tslib/pipeline/index.tslib/pipeline/runtime/concurrency.tslib/pipeline/runtime/orchestrator.tslib/prompts/bundled/index.tslib/prompts/bundled/periodic-classifier.test.tslib/prompts/bundled/periodic-classifier.tslib/prompts/ids.tslib/prompts/templateContextMap.tslib/prompts/types.tslib/stores/app-settings/app-settings.tslib/stores/generation/generation.test.tslib/stores/generation/generation.tslib/stores/index.ts
💤 Files with no reviewable changes (1)
- lib/pipeline/runtime/orchestrator.ts
The key-scoped json_set writers patched onto a bare '{}', so a branch whose
classifier_status was still NULL persisted an object missing the keys that
writer does not own — the column type-lied about ClassifierStatus. They now
seed a complete idle status, which keeps the writes total without making any
writer touch a key it doesn't own. awaitRunTerminal matched on kind alone,
letting a reversal abort another branch's run; branchId is now required.
|
Four of six actioned in bc180bb. Partial
No-op clamp test assertion — valid, fixed. Happening cascade race — premise accurate, conclusion not. It needs a concurrent writer attaching a child to an already existing happening; the only dispatcher of Entity projection — premise incorrect. The template references five fields and Liquid renders only what's referenced, so nothing extra reaches the prompt (see the committed snapshot). The hygiene point is fair but repo-wide: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/story-settings/`[storyId].tsx:
- Around line 117-122: Update the generation-state selection around
generatingBranchId to consider only foreground per-turn runs, excluding
periodic-classifier activity; use the existing kind-scoped helper such as
isForegroundGenerating(...) or filter for PER_TURN_KIND before deriving the
branch ID and isGenerating state. Keep classifier status separate so the active
phase and cancellation flow only use the foreground generation run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e852a79-2e63-44ec-bbb5-4d91bef8178b
📒 Files selected for processing (16)
app/reader-composer/[branchId].tsxapp/story-settings/[storyId].tsxlib/actions/classifier/deps.tslib/actions/classifier/run-now.test.tslib/actions/happenings/register-happenings.tslib/actions/story-entries/operational.test.tslib/actions/story-entries/operational.tslib/actions/story-entries/prose-reversal.test.tslib/actions/story-entries/prose-reversal.tslib/actions/story-entries/undo.tslib/classifier/index.tslib/classifier/status.tslib/pipeline/__tests__/concurrency-entry.test.tslib/pipeline/definitions/periodic-classifier.tslib/stores/generation/generation.test.tslib/stores/generation/generation.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- lib/actions/story-entries/operational.test.ts
- lib/pipeline/tests/concurrency-entry.test.ts
- lib/stores/generation/generation.ts
- lib/actions/story-entries/operational.ts
- app/reader-composer/[branchId].tsx
- lib/actions/story-entries/undo.ts
- lib/actions/happenings/register-happenings.ts
- lib/classifier/index.ts
- lib/actions/classifier/deps.ts
- lib/pipeline/definitions/periodic-classifier.ts
- lib/actions/classifier/run-now.test.ts
- lib/actions/story-entries/prose-reversal.test.ts
The retry policy could only act on a call that returned, so a provider that never answered left 'running' persisted — gating both the cadence and runNow, with no cancel affordance and no way out before the next boot. The expiry now routes to nextStatusOnFailure so it burns a retry. Separately, E2E launches Electron against build output, so a stale bundle passed green while testing old code; the runner compares mtimes per output and refuses to run.
Both context builders passed whole drizzle rows, so every column was template surface whether the bundled template rendered it or not — and packs are user-authored, which makes a column reachable today undroppable tomorrow. templateContextMap's field lists are now enforced rather than descriptive; classifierContext stays narrower than generationContext, since a prose classifier has no use for injectionMode.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/pipeline/definitions/periodic-classifier.ts`:
- Around line 78-100: Update boundedSignal’s relay handler to clear the timeout
before aborting the controller when the outer signal fires. Keep dispose’s
cleanup and the timer’s expired flag behavior unchanged, ensuring outer
cancellation cannot later be classified as a timeout.
In `@scripts/e2e.ts`:
- Line 31: Update the tracked inputs in the freshness check within
scripts/e2e.ts to include package.json, babel.config.js, metro.config.js,
app.json, and the project’s dependency lockfile alongside the existing source
directories, so build configuration and dependency changes invalidate stale dist
output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 88f03893-6836-4624-a323-f569dc4a2d77
⛔ Files ignored due to path filters (1)
docs/implementation/milestones/03-memory-floor/slices/03-classifier.mdis excluded by!docs/**
📒 Files selected for processing (7)
lib/pipeline/definitions/classifier-context.test.tslib/pipeline/definitions/classifier-context.tslib/pipeline/definitions/generation-context.test.tslib/pipeline/definitions/generation-context.tslib/pipeline/definitions/periodic-classifier.test.tslib/pipeline/definitions/periodic-classifier.tsscripts/e2e.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/pipeline/definitions/classifier-context.ts
- lib/pipeline/definitions/classifier-context.test.ts
- lib/pipeline/definitions/periodic-classifier.test.ts
Handlers have marked benign rejections code: 'noop' since M2, but nothing ever read it: any rejection threw ActionRejectedError, so a state patch restating an entity's current values — or a promotion the other writer already landed — took the user's whole turn down with the reversal sweep. Found by the slice 3.3 real provider smoke, where a 4B model hit it on most turns. Also lands that smoke as an opt-in spec; it skips unless SMOKE_LLM_URL is set.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e/tests/classifier-real-provider.smoke.spec.ts (2)
279-282: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider asserting that the pass wrote at least one happening.
Every hard invariant below is a
COUNT(*) == 0check. All of them pass when the classifier wrote nothing.writtenis printed only. If a real provider returns an empty extraction, the test still passes while the graph stays empty.If an empty extraction is an acceptable real-model outcome, keep this observational and state that in the comment. Otherwise assert
written[0][0]is greater than 0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tests/classifier-real-provider.smoke.spec.ts` around lines 279 - 282, Ensure the classifier smoke test validates that the current run wrote at least one happening by asserting written[0][0] is greater than zero after the queryApp call. Keep the existing diagnostic output, and preserve the current COUNT(*) checks for the other invariants.
121-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the test title and comments with
TARGET_TURNS = 1.The title states "three turns" and Line 189 states "Cadence 2 over three turns", but
TARGET_TURNSis 1 and Lines 75-78 explain that one committed turn is intentional. Update the title and the two comments so the stated coverage matches the code.Also applies to: 142-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tests/classifier-real-provider.smoke.spec.ts` at line 121, Align the test title and both turn-count comments with TARGET_TURNS = 1, including the cadence comment near the later assertion. Preserve the intentional single committed-turn behavior described near TARGET_TURNS and update only the misleading “three turns” wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/tests/classifier-real-provider.smoke.spec.ts`:
- Around line 190-209: Capture the deterministic parked watermark in the test’s
beforeAll setup and store it in a shared variable. Update the startWatermark
baseline used by the classifier polling assertion to reuse that parked value
instead of querying the database after the turn commits; keep the existing
greater-than polling behavior unchanged.
---
Nitpick comments:
In `@e2e/tests/classifier-real-provider.smoke.spec.ts`:
- Around line 279-282: Ensure the classifier smoke test validates that the
current run wrote at least one happening by asserting written[0][0] is greater
than zero after the queryApp call. Keep the existing diagnostic output, and
preserve the current COUNT(*) checks for the other invariants.
- Line 121: Align the test title and both turn-count comments with TARGET_TURNS
= 1, including the cadence comment near the later assertion. Preserve the
intentional single committed-turn behavior described near TARGET_TURNS and
update only the misleading “three turns” wording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b10c65f-94e2-4428-b968-990cf6691c6d
⛔ Files ignored due to path filters (1)
docs/implementation/milestones/03-memory-floor/slices/03-classifier.mdis excluded by!docs/**
📒 Files selected for processing (4)
e2e/tests/classifier-real-provider.smoke.spec.tslib/actions/entities/state-patch-actions.tslib/pipeline/__tests__/orchestrator-hardening.test.tslib/pipeline/runtime/orchestrator.ts
The slice named window_head_fallback as M7.5's tuning signal; the first real run contradicts it, so the correction goes to the triage inbox rather than a slice nobody has authored yet. The staleness-flag race is a test-design trap, not a product bug, so it goes to lessons-learned.
…-classifier Nine conflicts, all from the two slices touching the same seams. The two that were not additive: buildGenerationContext keeps 3.7a's branch filter and this branch's field projection (branchEntities.map(promptEntity)), and the pill's kind split gains the classifier's phase plus the branchId awaitRunTerminal now requires — 3.7a's suggestion-refresh call sites were still on the 2-arg form.
The merge applied isBackgroundKind to the reader but left story-settings on 3.7a's selector, so a classifier pass read as narrative there and offered a cancel targeting PER_TURN_KIND, which matches no run. Also: the classifier's timeout timer stayed armed after an outer abort, letting a late fire record a cancel as a timeout; the E2E freshness check ignored root build config and the lockfile; and the smoke read its watermark baseline after the pass could land.
1 participant
Summary
Slice 3.3 — the periodic classifier: the background pass that populates the
structured graph retrieval queries against. Ships the
periodic-classifierpipeline declaration, its cadence scheduler and auto-retry policy over
per-branch
classifier_status, the extraction → reconciliation → plannerchain, per-fact survival-anchor provenance, and the C3 prose-reversal barrier
that Slice 3.9
and Slice 3.10
build on.
Slice doc:
03-classifier.md.What lands
periodic-classifier(no-gate, self-blockingand blocked by
chapter-close, pill-only affordance), a pure per-branchscheduler wired off
pipelineEventBus'srun_complete, 30s → 2m → 5mbackoff, cadence suspension in
failed-persistent, and an exported manualrun as the only escape from it.
processedThrough; happenings with involvements and severity-judgedawareness, relationship UPSERT-merge, status flips, first-introduction
descriptions. Nothing embeds on the write path.
against every namesake, τ-banded into create / promote / flag.
awaitRunTerminalmoved into the generation store;bracketProseReversalas the single sanctioned entry to the reversal sweep,carrying the classifier drain and the
processedThroughclamp.deleteHappeningnow drops and restores its involvement andawareness rows through registry hooks, on undo and redo alike.
Review pass
A critical review of the finished branch found four defects that shipped green
under the existing tests; the last two commits fix them and cover them.
entriesStore, which holds only the last 50entries. Any backlog older than the reader's window was skipped, and the
watermark then advanced past it — prose permanently never classified. Now
read from SQLite. Unit tests that hydrate the store are structurally unable
to catch this, so it is covered end to end.
newCharactershandle colliding with a live placeholder silentlymisattributed facts. The return trip substitutes refs before the planner
runs, so a model naming a new character
c1had every ref to it rewritteninto the existing entity
c1— no unresolved ref, no warning. The prompt nowreserves a
new:namespace and the walker refuses to rewrite any ref adeclared handle claims, so a non-compliant model cannot reach the failure.
classifieragent halts before phase 0, so the phase's failure bookkeepingnever ran: status stayed idle, no backoff, no
lastError, and the cadencere-fired the doomed run on every committed turn.
cosinetruncated tothe shorter vector, fabricating a similarity from a shared prefix when the
two sides came back on different models mid embedder-swap; and the namesake
index was first-match-wins, which degrades exactly as create-with-flag
manufactures more namesakes.
The slice doc is reconciled against this: two of its Implementation notes had
recorded properties the code no longer has, including a "coherent by
construction" claim about watermark-after that rested on a boot recovery path
that does not exist for this pipeline.
Known gap (filed, not fixed)
A crash between the delta burst and the watermark write leaves the deltas on
disk with
processedThroughunmoved, and the replay duplicates the window'shappenings — there is no run marker and no idempotency key. Closing it touches
the orchestrator's transactional contract, so it is a
followup rather than a fix here.
Test Plan
pnpm typecheckpnpm lintpnpm vitest run --project unit— 1865 passingpnpm test:e2e— 25 passing (rebuild the bundle first: thedevproject launches Electron against a static
dist/, sopnpm build:web && pnpm electron:compilemust run or the suitesilently exercises stale code)
classifierCadence2, three turns — confirm graph population and watchwhether
classifier.window_head_fallbackwarnings dominate the log(that is the signal for the M7.5 tuning pass, not a bug)
Summary by CodeRabbit