Slice M3.2 - piggyback layer - #392
Merged
Merged
Conversation
added 12 commits
July 20, 2026 18:47
…flatten distinguishing
… classifier phase
Filter out non-existent entity IDs in buildPiggybackActions to prevent action rejection aborts, enhance pipeline orchestrator error logging, and add state block toggle icon to EntryCard header.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds structured piggyback state emission and parsing, entity state-patch actions, per-turn classifier fallback handling, calendar-aware prompt context, expanded narrative prompts, and related UI, logging, schema, and test coverage. ChangesPiggyback state pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Pento95
changed the title
Pento95
changed the title
Pento95
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
lib/piggyback/apply.test.ts (1)
452-482: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd assertions for item/stackable transfer filtering, not just visual changes.
This test exercises
transfers.items/transfers.stackableswith a non-existentfromid, but only asserts onvisualActions. Add assertions confirming the destination-side (char_kael) action is still correctly emitted while the non-existentfrom(Andrea) is safely dropped — this directly verifies the PR's stated invariant for transfers, not just visual changes.✅ Proposed additional assertions
const visualActions = result.actions.filter((a) => a.kind === 'updateEntityVisualState') expect(visualActions).toEqual([ { kind: 'updateEntityVisualState', source: 'ai_classifier', payload: { branchId: 'main', id: 'char_kael', visual: { attire: 'leather jacket' } }, }, ]) + + const itemActions = result.actions.filter((a) => a.kind === 'updateEntityInventory') + expect(itemActions).toEqual([ + { + kind: 'updateEntityInventory', + source: 'ai_classifier', + payload: { branchId: 'main', id: 'char_kael', equipped_items: [], inventory: ['item_blade'] }, + }, + ]) + + const stackableActions = result.actions.filter((a) => a.kind === 'updateEntityStackables') + expect(stackableActions).toEqual([ + { + kind: 'updateEntityStackables', + source: 'ai_classifier', + payload: { branchId: 'main', id: 'char_kael', stackables: { coins: 5 } }, + }, + ])🤖 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/piggyback/apply.test.ts` around lines 452 - 482, Add assertions to the existing buildPiggybackActions test for both item and stackable transfer actions, verifying transfers targeting char_kael are emitted with the expected payload while entries involving the non-existent from ID Andrea are omitted. Keep the existing visualActions assertion and filter result.actions by the transfer action symbols used by the implementation.lib/actions/delta/apply-delta-action.ts (1)
26-35: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
inFlightByKeyentries are never cleaned up.Each unique
promoteStagedEntity:${branchId}:${id}key stays in the map forever once used, even after its promise settles. Over a long-running session this grows unboundedly with the number of distinct entities ever promoted.♻️ Proposed cleanup on settle
function withKeyLock<T>(key: string, run: () => Promise<T>): Promise<T> { const settled = (inFlightByKey.get(key) ?? Promise.resolve()).then(run, run) - inFlightByKey.set( - key, - settled.catch(() => undefined), - ) + const tracked = settled.catch(() => undefined) + inFlightByKey.set(key, tracked) + tracked.finally(() => { + // Only remove if no newer caller has queued on top of this entry. + if (inFlightByKey.get(key) === tracked) inFlightByKey.delete(key) + }) return settled }🤖 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/apply-delta-action.ts` around lines 26 - 35, Update withKeyLock so each inFlightByKey entry is removed after its associated promise settles, but only when the map still points to that same promise; preserve chaining so concurrent calls for a key remain serialized and rejected work does not break later calls.lib/pipeline/definitions/per-turn-piggyback.ts (2)
80-101: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReroll branch has no test coverage.
generateClassifierState's reroll-on-negative-worldTimeDeltapath isn't exercised by any test inper-turn-piggyback.test.ts(only success/failure-without-reroll paths are covered). Given this logic guards a documented invariant (never let a negative delta through without a second attempt), a test mocking twogenerateStructuredMockcalls (first negative, second non-negative/failed) would give confidence this path works as intended.🤖 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/per-turn-piggyback.ts` around lines 80 - 101, Add tests in per-turn-piggyback.test.ts covering generateClassifierState’s reroll path: mock generateStructuredMock to return an initial successful result with a negative worldTimeDelta, then verify a second call occurs and that a non-negative successful result is returned. Also cover a second-call failure and verify the original negative result is retained.
150-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFailure logs drop error detail relative to the PR's stated logging goal.
Both warn calls here only log
status(or the substitutionfieldnames) and omit any richer failure context (e.g. error kind/detail from the failedgenerateStructuredresult, or substitution failure reasons beyond field name). The PR objective states pipeline error logging was enhanced with explicitreason/errorKind/errorDetailfields "across relevant pipeline events" — these two log sites in the same cohort appear to have been missed.♻️ Possible enrichment (adjust field names to match the actual result/failure shape)
if (result.status !== 'ok') { - ctx.log.warn('classifier.piggyback_fallback_failed', { status: result.status }) + ctx.log.warn('classifier.piggyback_fallback_failed', { + reason: result.status, + errorKind: 'kind' in result ? result.kind : undefined, + errorDetail: 'detail' in result ? result.detail : undefined, + }) return { status: 'completed' } }Please confirm the actual shape of a non-
'ok'GenerateStructuredResult(andsubstitutePiggybackIdsfailure entries) to pick the right field names.🤖 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/per-turn-piggyback.ts` around lines 150 - 160, Enrich the warning logs in the fallback handling around substitutePiggybackIds with the actual failure context. Inspect the non-ok GenerateStructuredResult shape and substitution failure entries, then include their available reason/errorKind/errorDetail or equivalent detail fields alongside status and field names in classifier.piggyback_fallback_failed and classifier.piggyback_fallback_parse_failed.
🤖 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.
Nitpick comments:
In `@lib/actions/delta/apply-delta-action.ts`:
- Around line 26-35: Update withKeyLock so each inFlightByKey entry is removed
after its associated promise settles, but only when the map still points to that
same promise; preserve chaining so concurrent calls for a key remain serialized
and rejected work does not break later calls.
In `@lib/piggyback/apply.test.ts`:
- Around line 452-482: Add assertions to the existing buildPiggybackActions test
for both item and stackable transfer actions, verifying transfers targeting
char_kael are emitted with the expected payload while entries involving the
non-existent from ID Andrea are omitted. Keep the existing visualActions
assertion and filter result.actions by the transfer action symbols used by the
implementation.
In `@lib/pipeline/definitions/per-turn-piggyback.ts`:
- Around line 80-101: Add tests in per-turn-piggyback.test.ts covering
generateClassifierState’s reroll path: mock generateStructuredMock to return an
initial successful result with a negative worldTimeDelta, then verify a second
call occurs and that a non-negative successful result is returned. Also cover a
second-call failure and verify the original negative result is retained.
- Around line 150-160: Enrich the warning logs in the fallback handling around
substitutePiggybackIds with the actual failure context. Inspect the non-ok
GenerateStructuredResult shape and substitution failure entries, then include
their available reason/errorKind/errorDetail or equivalent detail fields
alongside status and field names in classifier.piggyback_fallback_failed and
classifier.piggyback_fallback_parse_failed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e71cec24-12b2-4a6b-a3c0-ed01348eba2b
⛔ Files ignored due to path filters (6)
docs/architecture.mdis excluded by!docs/**docs/data-model.mdis excluded by!docs/**docs/implementation/milestones/03-memory-floor/slices/02-piggyback.mdis excluded by!docs/**docs/memory/piggyback.mdis excluded by!docs/**docs/parked.mdis excluded by!docs/**lib/prompts/bundled/__snapshots__/per-turn.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (57)
components/compounds/delta-log-row.tsxcomponents/compounds/entry-card.tsxelectron/main.tslib/actions/delta/apply-delta-action.tslib/actions/entities/register.tslib/actions/entities/state-patch-actions.test.tslib/actions/entities/state-patch-actions.tslib/actions/turns/submit-turn.test.tslib/actions/turns/submit-turn.tslib/actions/types.tslib/ai/generate.tslib/ai/index.tslib/ai/model-capabilities.test.tslib/ai/model-capabilities.tslib/calendar/index.tslib/calendar/vocabulary.test.tslib/calendar/vocabulary.tslib/db/app-settings/app-settings-schema.tslib/db/entities/entities-types.tslib/db/entities/entity-state-schema.test.tslib/db/entities/entity-state-schema.tslib/db/index.tslib/db/story-entries/entry-metadata.test.tslib/db/story-entries/entry-metadata.tslib/db/story-entries/inherited-metadata.test.tslib/db/story-entries/inherited-metadata.tslib/db/system/system.table.tslib/piggyback/apply.test.tslib/piggyback/apply.tslib/piggyback/index.tslib/piggyback/parse.test.tslib/piggyback/parse.tslib/piggyback/substitute.test.tslib/piggyback/substitute.tslib/piggyback/tags.tslib/piggyback/types.tslib/piggyback/world-time.test.tslib/piggyback/world-time.tslib/pipeline/definitions/generation-context.test.tslib/pipeline/definitions/generation-context.tslib/pipeline/definitions/per-turn-piggyback.test.tslib/pipeline/definitions/per-turn-piggyback.tslib/pipeline/definitions/per-turn.test.tslib/pipeline/definitions/per-turn.tslib/pipeline/runtime/orchestrator.tslib/prompts/bundled/index.tslib/prompts/bundled/per-turn.test.tslib/prompts/bundled/per-turn.tslib/prompts/bundled/state-emission.tslib/prompts/bundled/structural-floor.test.tslib/prompts/filters.test.tslib/prompts/filters.tslib/prompts/ids.tslib/prompts/templateContextMap.tslib/spellcheck/index.test.tslib/spellcheck/index.tslib/undo/index.ts
added 2 commits
July 22, 2026 18:32
… add reroll and transfer tests
…uard null lastSeenAt locationId Preflight now receives storySettings so the piggyback resolver's when-predicate sees the real piggybackMode; null locationId no longer produces a silently rejected lastSeenAt delta; unguarded DB row destructure now throws a clear Error.
failerko
requested changes
Jul 22, 2026
failerko
requested changes
Jul 23, 2026
Bracket-form and unresolved IDs now fail closed instead of leaking into entry metadata; the fallback classifier's prompt moves into lib/prompts/bundled behind buildGenerationContext like every other story prompt; the narrative prompt skips staged/locations/calendar/state-emission when the fallback will fire anyway; and direct tagged-block deltas get their own source (piggyback_tagged_block) instead of sharing ai_classifier with unrelated writes.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/actions/delta/apply-delta-action.ts (1)
41-49: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSerialize delta-position allocation per branch, not per entity.
The lock key includes
payload.id, so concurrent promotions of different staged entities on the same branch still execute in parallel. Both can calculate the sameMAX(logPosition) + 1; the unique(branchId, logPosition)index inlib/db/system/system.table.tsthen causes one transaction to fail. Hold a branch-scoped lock around position allocation (or make allocation atomic in SQLite).🤖 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/apply-delta-action.ts` around lines 41 - 49, Update applyDeltaAction so promoteStagedEntity operations use a lock key scoped only to action.payload.branchId, removing action.payload.id from the withKeyLock key. Keep the lock around applyDeltaActionUnlocked to serialize log-position allocation for all promotions on the same branch while preserving parallelism across different branches.
🤖 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.
Outside diff comments:
In `@lib/actions/delta/apply-delta-action.ts`:
- Around line 41-49: Update applyDeltaAction so promoteStagedEntity operations
use a lock key scoped only to action.payload.branchId, removing
action.payload.id from the withKeyLock key. Keep the lock around
applyDeltaActionUnlocked to serialize log-position allocation for all promotions
on the same branch while preserving parallelism across different branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6482e585-31e3-4953-9c80-95e4b869373e
⛔ Files ignored due to path filters (5)
docs/data-model.mdis excluded by!docs/**docs/memory/classifier.mdis excluded by!docs/**docs/memory/piggyback.mdis excluded by!docs/**docs/ui/patterns/delta-log-row.mdis excluded by!docs/**lib/prompts/bundled/__snapshots__/per-turn.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (20)
components/compounds/delta-log-row.tsxlib/actions/delta/apply-delta-action.tslib/actions/types.tslib/db/system/system.table.tslib/piggyback/apply.tslib/piggyback/substitute.test.tslib/piggyback/substitute.tslib/pipeline/definitions/generation-context.tslib/pipeline/definitions/per-turn-piggyback.test.tslib/pipeline/definitions/per-turn-piggyback.tslib/pipeline/definitions/per-turn.test.tslib/pipeline/definitions/per-turn.tslib/pipeline/runtime/orchestrator.tslib/prompts/bundled/index.tslib/prompts/bundled/per-turn.test.tslib/prompts/bundled/per-turn.tslib/prompts/bundled/piggyback-fallback-classifier.tslib/prompts/bundled/state-emission.tslib/prompts/ids.tslib/prompts/templateContextMap.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- lib/actions/types.ts
- lib/prompts/bundled/index.ts
- lib/prompts/bundled/state-emission.ts
- lib/pipeline/runtime/orchestrator.ts
- lib/pipeline/definitions/per-turn.ts
- lib/prompts/bundled/per-turn.ts
- lib/pipeline/definitions/per-turn.test.ts
- lib/pipeline/definitions/per-turn-piggyback.ts
- lib/pipeline/definitions/per-turn-piggyback.test.ts
- lib/piggyback/apply.ts
…iting Visual/inventory/stackable piggyback actions only checked that a referenced id existed, not that it belonged to a character — a location/item/faction id could get character-only state fields merged onto it unvalidated. The direct tagged-block path also skipped type-enum validation the fallback classifier's zod schema already enforced, letting an arbitrary key pollute state.visual. Both paths now share one VISUAL_CHANGE_TYPES source of truth.
failerko
requested changes
Jul 23, 2026
added 2 commits
July 23, 2026 17:30
…sifier The fallback classifier only saw the AI's reply, missing state changes described in the user's own action. It also allocated a fresh IdBiMap instead of the one the narrative phase already stored on ctx.intermediates, renumbering placeholders mid-turn for no reason.
Fix 3 had gated the whole staged/locations/calendar block behind piggybackFires, but the narrative model still needs that info to write consistently (e.g. introducing a staged character) even when the fallback classifier will do the extraction afterwards. Only the bracketed IDs and the ID-usage instructions tied to the tagged-block output are piggyback-only now.
failerko
approved these changes
Jul 23, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements Slice 3.2 — Piggyback layer (
docs/implementation/milestones/03-memory-floor/slices/02-piggyback.md), providing the per-turn fast path of the memory cadence. The narrative model emits a tagged<state>block alongside prose, which is parsed and applied to entry metadata, entity states, and computed bookkeeping, with a synchronous per-turn fallback classifier when tagged blocks are unreliable or fail to parse.Delivered Scope
<state>blocks (sceneEntities,currentLocation,worldTimeDelta,visualChanges,transfers,summary) withjsonrepairfallback and bracketed placeholder substitution (c1,l1-> entity UUIDs).<state>emission instructions, staged-entity prompt framing, and calendar vocabulary (tier names, weekdays, eras) in per-turn prompt template.sceneEntities,currentLocationId, andworldTimedelta to entry metadata; applies visual state updates and item/stackable transfers; filters out non-existent entity IDs to enforce "piggyback creates no rows" and prevent action rejection crashes.current_location_idtracking for characters in scene andlastSeenAttransitions upon scene exit.<scene_entities>.piggybackFallbackClassifierPhaseto run a synchronous classifier pass when model capability is uncurated/unreliable or when trailing block parsing fails.<summary>enrichment hand-off: Extracts optional trailing summary for next turn's Q2 structural digest.EntryCardto strip raw<state>XML blocks from narrative text while providing aGlobeheader action icon to toggle an expandable preview of state metadata.Test Plan
pnpm typecheckpassed with 0 TypeScript errors.lib/piggyback/(31/31 tests passing).per-turn-piggyback(17/17 tests passing).Summary by CodeRabbit
New Features
Bug Fixes
data:/blob:connections and refined spellcheck lint suppression.