Slice/m2.6#353
Open
failerko wants to merge 21 commits into
Open
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JSON.stringify(undefined) returns undefined at runtime; ?? '' makes the return type match the string signature. Also fixes the pre-existing byKind non-array test cast (unknown[] → never[]) and adds the symmetric active non-array guard test with the same cast form.
Closes the lib/prompts module boundary: barrel now re-exports loadPack, PackLoadError, LoadedPack, validatePackIncludes, and bundledPack alongside the new renderTemplate lazy singleton. Three-test load/render suite with snapshot. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Template ids are a closed static union (users edit pack content, not pipelines); pack selection becomes a separate arg. Records the M7.2 pack-id and forced-macro-prefix design in the slice doc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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 (5)
📝 WalkthroughWalkthroughIntroduces Slice 2.6 LiquidJS prompt-template engine: shared ChangesPrompt Template Engine and Bundled Pack
BUNDLED_PACK_ID Default Wiring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements a LiquidJS-based prompt template engine and introduces a bundled pack of templates and macros for story generation, along with custom filters, include validation, and database seeding updates. The code reviewer provided valuable feedback focused on enhancing runtime robustness. Specifically, they recommended adding defensive checks in the custom filters (byKind, active, and proseJoin) to handle potential null or undefined items, as well as adding defensive guards for the pack object in validatePackIncludes and createEngine to prevent runtime crashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
lib/prompts/filters.test.ts (1)
36-38: ⚡ Quick winAdd a regression test for non-serializable input in
jsonFilter.Please add a case asserting circular input does not throw and returns the fallback string, so the crash path stays covered.
🤖 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/filters.test.ts` around lines 36 - 38, Add a regression test case for the jsonFilter function that verifies it gracefully handles circular object references. In the test file lib/prompts/filters.test.ts, add a new test case near the existing jsonFilter test (around line 36-38) that creates an object with a circular reference, calls jsonFilter with that object, and asserts that it does not throw an error and instead returns the fallback string value. This ensures the error handling path in jsonFilter remains covered by tests.lib/prompts/templateContextMap.ts (1)
86-91: ⚡ Quick winTighten
TEMPLATE_GROUPStyping to enforce full TemplateId coverage at compile time.Right now,
Record<string, ContextGroup>allows silent omissions and relies on runtime validation/tests. Narrowing this toRecord<TemplateId, ContextGroup>(orsatisfies) will fail fast when a newTEMPLATE_IDSentry is added but not mapped.Proposed change
import { TEMPLATE_IDS } from './ids' +import type { TemplateId } from './ids' import type { ContextGroup } from './types' @@ -export const TEMPLATE_GROUPS: Record<string, ContextGroup> = { +export const TEMPLATE_GROUPS: Record<TemplateId, ContextGroup> = { [TEMPLATE_IDS.perTurnNarrative]: 'generationContext', [TEMPLATE_IDS.wizardOpening]: 'wizard', [TEMPLATE_IDS.wizardTitleChips]: 'wizard', [TEMPLATE_IDS.wizardDescription]: 'wizard', }🤖 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 86 - 91, The TEMPLATE_GROUPS constant is currently typed as Record<string, ContextGroup>, which allows any string key and silently permits missing TemplateId mappings. Change the type annotation to either Record<TemplateId, ContextGroup> or add a satisfies Record<TemplateId, ContextGroup> clause to enforce compile-time validation that every TemplateId entry has a corresponding context group mapping, preventing silent omissions when new template IDs are added to TEMPLATE_IDS.
🤖 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 `@docs/implementation/triage.md`:
- Around line 22-49: Verify and reframe two of the three affected documents to
reflect that composer-mode wrap is now driven by in-code i18n (Slice 2.5) rather
than pack macros (Slice 2.6). In principles.md under the "Composer mode"
section, change the statement that wrapping is "driven by pack templates" to
accurately describe the in-code i18n model. In milestone.md under the "C2 —
Pack-engine render surface" section, remove all references to "composer-wrap
macro per (mode × wrapPov) cell" and update Slice 2.5's description to drop its
documented dependency on Slice 2.6 wrap macros. For architecture.md under the
"Macros" section, verify whether composer-wrap macro examples are actually
present in the current state; if found, remove them as examples of
macros-as-formatters since the wrap is now in-code.
In `@lib/prompts/engine.ts`:
- Around line 10-11: The templates and macros are both being stored in the same
templates object, causing macros with duplicate IDs to silently overwrite
template sources without any error. Add collision detection before assigning
macro IDs to the templates object in the second loop. Either check if the ID
already exists in the templates object and throw a validation error with details
about the collision, or store macros in a separate object (e.g., a macros
variable) instead of reusing the templates object. This ensures that ID
conflicts are caught explicitly rather than silently overwriting values during
template/macro rendering.
In `@lib/prompts/filters.ts`:
- Around line 22-24: The jsonFilter function at line 22-24 does not handle
exceptions thrown by JSON.stringify when encountering circular references or
other non-serializable values, causing the render to crash. Wrap the
JSON.stringify call in a try-catch block to catch any errors that occur during
stringification, and return a safe fallback string value (such as an empty
string or a descriptive error message) in the catch block to ensure the function
always returns a string without crashing the render path.
---
Nitpick comments:
In `@lib/prompts/filters.test.ts`:
- Around line 36-38: Add a regression test case for the jsonFilter function that
verifies it gracefully handles circular object references. In the test file
lib/prompts/filters.test.ts, add a new test case near the existing jsonFilter
test (around line 36-38) that creates an object with a circular reference, calls
jsonFilter with that object, and asserts that it does not throw an error and
instead returns the fallback string value. This ensures the error handling path
in jsonFilter remains covered by tests.
In `@lib/prompts/templateContextMap.ts`:
- Around line 86-91: The TEMPLATE_GROUPS constant is currently typed as
Record<string, ContextGroup>, which allows any string key and silently permits
missing TemplateId mappings. Change the type annotation to either
Record<TemplateId, ContextGroup> or add a satisfies Record<TemplateId,
ContextGroup> clause to enforce compile-time validation that every TemplateId
entry has a corresponding context group mapping, preventing silent omissions
when new template IDs are added to TEMPLATE_IDS.
🪄 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: 988e8e5b-05e1-47ab-9c81-a353153f828a
⛔ Files ignored due to path filters (3)
lib/prompts/bundled/__snapshots__/bundled-pack.test.ts.snapis excluded by!**/*.snaplib/prompts/bundled/__snapshots__/per-turn.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
docs/implementation/milestones/02-first-user-loop/milestone.mddocs/implementation/milestones/02-first-user-loop/slices/03-wizard.mddocs/implementation/milestones/02-first-user-loop/slices/06-pack-engine.mddocs/implementation/milestones/02-first-user-loop/slices/07-wiring.mddocs/implementation/triage.mdlib/db/app-settings/app-settings-defaults.test.tslib/db/app-settings/app-settings-defaults.tslib/prompts/bundled/bundled-pack.test.tslib/prompts/bundled/index.tslib/prompts/bundled/output-format.tslib/prompts/bundled/per-turn.test.tslib/prompts/bundled/per-turn.tslib/prompts/bundled/structural-floor.test.tslib/prompts/bundled/wizard.tslib/prompts/constants-enforcement.test.tslib/prompts/engine.test.tslib/prompts/engine.tslib/prompts/filters.test.tslib/prompts/filters.tslib/prompts/ids.tslib/prompts/index.tslib/prompts/load-pack.tslib/prompts/templateContextMap.test.tslib/prompts/templateContextMap.tslib/prompts/types.tslib/prompts/validate-includes.test.tslib/prompts/validate-includes.tslib/stores/__tests__/namespace-shape.test.tspackage.jsonscripts/seed/dataset.ts
💤 Files with no reviewable changes (1)
- docs/implementation/milestones/02-first-user-loop/milestone.md
Comment on lines
+22
to
+49
| ### Composer-mode wrap: canonical reframe to in-code i18n | ||
|
|
||
| Decided in M2.6 (pack-engine) planning: composer-mode send-time wrap | ||
| (`Do` / `Say` / `Think` × `first` / `third`) is implemented in-code, | ||
| i18n-keyed, in | ||
| [Slice 2.5](./milestones/02-first-user-loop/slices/05-reader.md) — NOT | ||
| as pack/Liquid macros. The wrapped string is target-language user | ||
| content, but a pack is English-source with no per-language variant; an | ||
| in-code wrap keeps the `user-action-translation` phase fed clean | ||
| monolingual input. Slice 2.6 already dropped the wrap macros from scope. | ||
|
|
||
| Still needs the reframe (no single slice owns it): | ||
|
|
||
| - [`principles.md → Composer mode`](../ui/principles.md#composer-mode--send-time-transform-narration-aware) | ||
| says wrapping is "driven by pack templates" — change to the in-code | ||
| i18n model. | ||
| - [`architecture.md → Macros`](../architecture.md#macros--reusable-liquid-snippets-not-code-side-formatters) | ||
| uses the composer wrap as a macro-as-formatter example — drop or | ||
| recharacterize it. | ||
| - [`Milestone 2 → C2`](./milestones/02-first-user-loop/milestone.md#c2--pack-engine-render-surface) | ||
| pins wrap macros into the pack and lists Slice 2.5 as a wrap consumer | ||
| — remove the wrap clause; removing it likely also drops 2.5's only | ||
| dependency on 2.6 (revisit the dep graph). | ||
|
|
||
| M2 is English-only (same-language short-circuit), so nothing here blocks | ||
| M2; the in-code implementation is Slice 2.5-owned and surfaces at 2.5 | ||
| planning. Route the canonical reframe via aventuras-design or a focused | ||
| doc-amendment before Slice 2.5. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the three referenced doc sections exist and are referenceable.
# Check for principles.md Composer mode section
if [ -f "docs/ui/principles.md" ]; then
grep -n "Composer mode" docs/ui/principles.md && echo "✓ principles.md#Composer mode exists" || echo "✗ principles.md: no 'Composer mode' heading"
else
echo "✗ docs/ui/principles.md not found"
fi
# Check for architecture.md Macros section
if [ -f "docs/architecture.md" ]; then
grep -n "Macros" docs/architecture.md | head -5 && echo "✓ architecture.md#Macros exists" || echo "✗ architecture.md: no 'Macros' heading"
else
echo "✗ docs/architecture.md not found"
fi
# Check for milestone.md C2 section
if [ -f "docs/implementation/milestones/02-first-user-loop/milestone.md" ]; then
grep -n "^## C2" docs/implementation/milestones/02-first-user-loop/milestone.md && echo "✓ milestone.md#C2 exists" || echo "✗ milestone.md: no 'C2' section"
else
echo "✗ milestone.md not found"
fiRepository: AventurasTeam/Aventuras
Length of output: 434
🏁 Script executed:
#!/bin/bash
# Check milestone.md structure and look for C2 section
FILE="docs/implementation/milestones/02-first-user-loop/milestone.md"
if [ -f "$FILE" ]; then
echo "=== File found, checking structure ==="
wc -l "$FILE"
echo ""
echo "=== All section headings ==="
grep -n "^#" "$FILE" | head -20
echo ""
echo "=== Search for C2 anywhere ==="
grep -n "C2" "$FILE" | head -10
else
echo "File not found: $FILE"
fiRepository: AventurasTeam/Aventuras
Length of output: 805
🏁 Script executed:
#!/bin/bash
# Read the C2 section content to verify the triage claims
sed -n '192,218p' docs/implementation/milestones/02-first-user-loop/milestone.mdRepository: AventurasTeam/Aventuras
Length of output: 1506
🏁 Script executed:
#!/bin/bash
# Read principles.md Composer mode section to verify "driven by pack templates" claim
echo "=== principles.md Composer mode section (around line 687) ==="
sed -n '687,720p' docs/ui/principles.mdRepository: AventurasTeam/Aventuras
Length of output: 1692
🏁 Script executed:
#!/bin/bash
# Read architecture.md Macros section to verify the "macro-as-formatter example" claim
echo "=== architecture.md Macros section (around line 111) ==="
sed -n '111,160p' docs/architecture.mdRepository: AventurasTeam/Aventuras
Length of output: 2293
🏁 Script executed:
#!/bin/bash
# Search architecture.md comprehensively for composer-wrap references
echo "=== Searching architecture.md for composer-wrap or wrap macro mentions ==="
grep -n "wrap\|composer" docs/architecture.md | head -20Repository: AventurasTeam/Aventuras
Length of output: 274
🏁 Script executed:
#!/bin/bash
# Check if composer-wrap macro was ever documented as an example in architecture.md
# or if it was already removed
echo "=== Full search for any mention of compose/wrap in architecture.md ==="
rg -i "composer|wrap" docs/architecture.md -B2 -A2
echo ""
echo "=== Check 06-pack-engine.md for triage reference ==="
grep -n "triage" docs/implementation/milestones/02-first-user-loop/slices/06-pack-engine.mdRepository: AventurasTeam/Aventuras
Length of output: 756
🏁 Script executed:
#!/bin/bash
# More comprehensive search for composer-wrap or wrap-macro in architecture.md
echo "=== Complete architecture.md for wrap-related content ==="
wc -l docs/architecture.md
echo ""
rg -i "wrap.*macro|macro.*wrap|composer" docs/architecture.md -B3 -A3
echo ""
echo "=== Search for any 'example' sections mentioning wrap ==="
rg -i "example" docs/architecture.md -B2 -A2 | grep -i wrap -B2 -A2Repository: AventurasTeam/Aventuras
Length of output: 657
🏁 Script executed:
#!/bin/bash
# Check if composer-wrap or wrap-macro is mentioned anywhere in architecture.md
echo "=== All lines with 'wrap' in architecture.md ==="
grep -n "wrap" docs/architecture.md
echo ""
echo "=== Context around line 307 (composer UX mention) ==="
sed -n '300,320p' docs/architecture.mdRepository: AventurasTeam/Aventuras
Length of output: 1481
🏁 Script executed:
#!/bin/bash
# Double-check: search entire codebase for any wrap-macro-as-formatter pattern
echo "=== Searching entire docs for 'wrap.*macro' or 'macro.*wrap' patterns ==="
rg -i "wrap.*macro|macro.*wrap" docs/ | grep -v node_modules
echo ""
echo "=== Check git history: was wrap macro ever in architecture.md? ==="
git log --all --oneline --grep="wrap\|macro" docs/architecture.md 2>/dev/null | head -5Repository: AventurasTeam/Aventuras
Length of output: 1896
🏁 Script executed:
#!/bin/bash
# Check the full context of the 06-pack-engine.md mention of composer-wrap descoping
echo "=== 06-pack-engine.md around composer-wrap descoping ==="
sed -n '155,180p' docs/implementation/milestones/02-first-user-loop/slices/06-pack-engine.md
echo ""
echo "=== Check the full principles.md section for any mention of 'in-code' or updates ==="
sed -n '680,750p' docs/ui/principles.mdRepository: AventurasTeam/Aventuras
Length of output: 4563
Verify two of three referenced documents still need reframing; third appears already addressed.
This triage entry correctly queues a decision from M2.6 planning: composer-wrap moves from pack macros (Slice 2.6) to in-code i18n (Slice 2.5). The routing path (aventuras-design review before Slice 2.5 planning) is sound.
However, verification shows mixed status on the three reframes:
- principles.md (line 687, "Composer mode" section): Still states wrapping is "driven by pack templates." Needs reframing to reflect in-code i18n.
- milestone.md (line 192, "C2 — Pack-engine render surface"): Still says "the bundled pack ships one composer-wrap macro per
(mode × wrapPov)cell" and lists Slice 2.5 as a consumer. Needs updating to remove pack-macro references per the 06-pack-engine.md descoping note. - architecture.md (line 111, "Macros" section): No wrap macro example found in current state. The Macros section lists only
character_block,happening_for_memory, and output-format directives. Either already addressed or the claim was preemptive.
🤖 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 `@docs/implementation/triage.md` around lines 22 - 49, Verify and reframe two
of the three affected documents to reflect that composer-mode wrap is now driven
by in-code i18n (Slice 2.5) rather than pack macros (Slice 2.6). In
principles.md under the "Composer mode" section, change the statement that
wrapping is "driven by pack templates" to accurately describe the in-code i18n
model. In milestone.md under the "C2 — Pack-engine render surface" section,
remove all references to "composer-wrap macro per (mode × wrapPov) cell" and
update Slice 2.5's description to drop its documented dependency on Slice 2.6
wrap macros. For architecture.md under the "Macros" section, verify whether
composer-wrap macro examples are actually present in the current state; if
found, remove them as examples of macros-as-formatters since the wrap is now
in-code.
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
1 participant
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 by CodeRabbit
Release Notes
New Features
Tests
Documentation
Chores