Skip to content

Test/e2e setup - #395

Merged
failerko merged 24 commits into
mainfrom
test/e2e-setup
Jul 24, 2026
Merged

Test/e2e setup#395
failerko merged 24 commits into
mainfrom
test/e2e-setup

Conversation

@failerko

@failerko failerko commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • New Features
    • Added a Playwright + Electron E2E test setup for the packaged desktop app, including seeded-data validation, mock LLM coverage, and UI flows.
    • Introduced reusable E2E harness helpers for launching the app, installing the local embedder model, seeding/configuring the test database, and using i18n-based stable locators.
    • Added deterministic fixture ID remapping with contract tests to keep seeded references consistent.
  • CI
    • Added an E2E job that builds/packages Linux and runs pnpm test:e2e against the packaged build.
  • Documentation
    • Added project guidance for E2E test conventions and AI-assisted test edits.

failerko and others added 6 commits July 23, 2026 19:59
Canonical spec for the Playwright + Electron E2E layer: packaged-build target (Android dropped, web-only impossible without the preload DB bridge), local-vs-CI launch modes, the e2e/ harness layout with reusable locators/flows, the fixture/seed contract, embedder-on-disk seeding, mock LLM, and the DB-first three-tier selector strategy. Unit + component discipline stays in code-conventions.md.

Records the fixture prerequisite: seed IDs under a substitutable prefix must be real prefix_<uuid>, not mnemonics — substituteIds passes char_kael through untouched so turns fail on the placeholder return trip. Fix lands as a standalone slice next.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mnemonic seed ids (char_kael) never match ID_PATTERN, so substituteIds passes them through untouched and a turn on seeded data dies on the placeholder return trip. buildSeedSteps now rewrites every substitutable id to a deterministic prefix_<uuid> (pure-JS hash, stable under both the Node seed script and Hermes reseed), correcting the two authored off-spec (fac→fact, thread→thr per data-model.md) and re-canonicalizing relationship pairs whose a_id<b_id order the uuid remap inverts.

Non-substitutable ids (branch_, entry_, aw_) keep their readable mnemonics; conforming them to the data-model prefix table is a separate cleanup.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The fixture was hand-authored with ad-hoc prefixes that never matched the ID table in data-model.md: branch_→br_, e_→entry_, aw_→haw_, inv_→hinv_, ea_→ast_, and the seed_act_ action grouping→act_. Runtime code already mints these via generateId, so this was seed-only drift; correcting it keeps the fixture faithful if future code keys on the prefix. Readable suffixes stay (br_hero_main) since nothing requires the uuid shape for non-LLM ids; media assets, era flips, and the app_settings singleton have no spec prefix and are left as-is.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
First cut of the Playwright + Electron harness from docs/testing.md: seed a throwaway userData (real db:seed), launch unpackaged main against a static-served dist, select the app window by origin (dodging the dev DevTools race), and assert through both the DOM (i18n-resolved locators) and the fixture DB. Proves the two open unknowns — i18next resolves standalone in Node, and the launch→seed→select→assert loop holds. Local mode only; packaged app:// tier, mock LLM, embedder seeding, and the wizard UI flow are the next increments. Requires a prior build:web + electron:compile.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds the packaged launch mode to the harness (electron-builder --dir binary loading app://bundle, selected by scheme origin) behind AVENTURAS_E2E_MODE — dev stays the local default, packaged is the CI target of record. New e2e job builds the web bundle, compiles main, packages unpacked (Electron binary cached on the lockfile), then runs the suite under xvfb against a per-run seeded userData. Verified locally end-to-end in both modes; xvfb and fresh-install native deps are the CI-only unknowns to confirm on the first run.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The section described the id drift as an open bug and named UUIDv5; the fix shipped as a deterministic pure-JS hash (v4-shaped) in seed-ids.ts, plus the fac_→fact_ / thread_→thr_ corrections and relationship re-canonicalization. Reframe as the satisfied contract and correct the mechanism.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@failerko, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 3 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 53a886fc-a394-4fe8-aae1-f75f7e7a6489

📥 Commits

Reviewing files that changed from the base of the PR and between 0d43356 and 830b73c.

⛔ Files ignored due to path filters (2)
  • docs/implementation/triage.md is excluded by !docs/**
  • docs/testing.md is excluded by !docs/**
📒 Files selected for processing (13)
  • .agents/skills/aventuras-plan-slice/SKILL.md
  • .github/workflows/ci.yml
  • e2e/harness/embedder.ts
  • e2e/harness/launch.ts
  • e2e/harness/seed.ts
  • e2e/tests/classifier.spec.ts
  • e2e/tests/embedder.spec.ts
  • e2e/tests/home.spec.ts
  • e2e/tests/structured-force-on.spec.ts
  • e2e/tests/turn.spec.ts
  • e2e/tests/wizard.spec.ts
  • package.json
  • playwright.config.ts
📝 Walkthrough

Walkthrough

Adds deterministic seed ID normalization, updates fixture contracts, and introduces a Playwright Electron harness covering seeded data, story creation, local embedding, mocked LLM turns, structured output, and packaged CI execution.

Changes

Seeded E2E and fixture contracts

Layer / File(s) Summary
Deterministic seed ID remapping
lib/db/devtools/seed-ids.ts, lib/db/devtools/seed-ids.test.ts
Canonicalizes mnemonic IDs into deterministic UUID-shaped identifiers and recursively remaps nested seed structures with unit coverage.
Seed dataset normalization
lib/db/devtools/seed-dataset.ts, lib/db/devtools/seed-dataset.test.ts
Updates seed prefixes, remaps generated rows, preserves relationship ordering, and validates ID formats, determinism, and cross-references.
Electron E2E harness
e2e/harness/*, e2e/locators/home.ts, e2e/tests/home.spec.ts, .claude/rules/testing.md, CLAUDE.md
Creates isolated seeded user data, queries SQLite fixtures, launches Electron, resolves localized selectors, and documents E2E conventions.
Story and embedder flows
e2e/flows/create-story.ts, e2e/locators/wizard.ts, e2e/tests/wizard.spec.ts, e2e/harness/embedder.ts, e2e/tests/embedder.spec.ts
Drives story creation and validates persisted entities and embeddings, while provisioning and testing the local embedder bridge.
Mock LLM and pipeline coverage
e2e/harness/mock-llm.ts, e2e/harness/seed.ts, e2e/tests/{turn,classifier,structured-force-on}.spec.ts, lib/{ai,pipeline}/*
Adds streamed and structured mock completions, configures seeded provider state, and verifies turn commits, fallback classification, and native JSON mode.
Packaged E2E execution
playwright.config.ts, .github/workflows/ci.yml, package.json, .gitignore, .agents/skills/*
Adds Playwright tooling, packaged Linux CI execution, native module unpacking, ignored reports, and E2E planning and verification guidance.

Estimated code review effort: 4 (Complex) | ~90 minutes

Possibly related PRs

Sequence Diagram(s)

sequenceDiagram
  participant Playwright
  participant Electron
  participant RendererBridge
  participant MockLlm
  participant SQLite
  Playwright->>Electron: launch with seeded userDataDir
  Playwright->>RendererBridge: submit story turn
  RendererBridge->>MockLlm: request streamed narrative
  MockLlm-->>RendererBridge: return narrative SSE
  RendererBridge->>MockLlm: request structured classifier
  MockLlm-->>RendererBridge: return JSON response
  RendererBridge->>SQLite: commit story entries
  Playwright->>SQLite: assert persisted turn data
Loading

Poem

A bunny remaps IDs in a UUID row,
Seeds SQLite where test carrots grow.
Mock clouds stream tales through Electron’s door,
Playwright checks bridges, models, and more.
“Green CI!” we cheer, and wiggle our nose.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 68.75% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title is related to the change, but it is too generic to convey the main E2E and harness work. Use a more specific title, e.g. "Add Playwright E2E harness and desktop test setup".
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@e2e/harness/launch.ts`:
- Around line 18-22: Replace the environment-variable E2E contract with explicit
checked-in runner/harness configuration: update e2e/harness/launch.ts lines
18-22 to receive mode and packaged app location through that configuration, and
lines 110-115 to pass the dev renderer endpoint through the same boundary; make
reporter selection deterministic or use the shared configuration in
playwright.config.ts line 15; update .github/workflows/ci.yml lines 141-144 to
invoke the packaged configuration directly without exporting AVENTURAS_E2E_MODE.
- Around line 89-125: Update the launch flow around electron.launch,
selectAppWindow, and the returned close methods to clean up resources on all
failures. Wrap each launch/setup path in try/catch, closing any started app and
removing userDataDir when initialization fails; for the development path, always
close the server and clean up user data in close() via finally, including when
app.close() rejects.
🪄 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: 0c7b3d8d-a36d-42dd-9bb2-3ada03eac553

📥 Commits

Reviewing files that changed from the base of the PR and between 56861d4 and 3f9739c.

⛔ Files ignored due to path filters (4)
  • docs/README.md is excluded by !docs/**
  • docs/code-conventions.md is excluded by !docs/**
  • docs/testing.md is excluded by !docs/**
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml, !pnpm-lock.yaml
📒 Files selected for processing (16)
  • .claude/rules/testing.md
  • .github/workflows/ci.yml
  • .gitignore
  • CLAUDE.md
  • e2e/harness/db.ts
  • e2e/harness/i18n.ts
  • e2e/harness/launch.ts
  • e2e/harness/seed.ts
  • e2e/locators/home.ts
  • e2e/tests/home.spec.ts
  • lib/db/devtools/seed-dataset.test.ts
  • lib/db/devtools/seed-dataset.ts
  • lib/db/devtools/seed-ids.test.ts
  • lib/db/devtools/seed-ids.ts
  • package.json
  • playwright.config.ts

Comment thread e2e/harness/launch.ts Outdated
Comment thread e2e/harness/launch.ts Outdated
failerko and others added 4 commits July 23, 2026 22:03
@huggingface/transformers pulls in sharp; its libvips native lib was packed inside the asar and failed to dlopen (ERR_DLOPEN_FAILED: libvips-cpp.so), so embedding threw an init error in every packaged build — release AppImage included — while dev worked from node_modules. Add sharp + @img to asarUnpack. Surfaced by the packaged E2E embedder test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Harness provisions the default catalog model (download from HF at the pinned revision, sha256-verified, cached under ~/.cache/aventuras-e2e) and copies it into each run's userData/embedders. New spec drives the renderer embedder bridge: listInstalled sees the seeded model, and smokeTest embeds through the real renderer → IPC → main → onnxruntime path returning the catalog dim. CI caches the model dir keyed on the catalog. Unblocks the wizard flow, whose create-story path embeds the lead entity.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Root cause was not the hypothesized wasm-backend / modelDir resolution but sharp's libvips native lib packed inside the asar (ERR_DLOPEN_FAILED); fixed in 74cb1d2 by adding sharp + @img to asarUnpack, verified on a built AppImage. Caught by the packaged E2E embedder test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drives the wizard UI end-to-end — New story → adventure mode (surfaces the lead) → calendar → opening + title → Finish — then asserts through the app's DB bridge that the story committed active, the lead entity persisted, and the real embedder populated its vec0 row (entities_vec_384). Exercises the wizard's hard embedder gate, the multi-step nav, and create-story's lead-embed path in one pass. All locators resolve via i18n role/name + placeholder, so no testIDs were needed; reusable flow lives in e2e/flows.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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/harness/embedder.ts`:
- Around line 39-44: The cacheRoot() configuration must not read the
AVENTURAS_E2E_MODEL_DIR environment variable. Remove this environment-based
override and use the repository-approved SQLite-backed configuration path,
preserving the existing default cache location when no configured value is
available.
- Around line 54-58: Update the cache-hit logic around sanitizeModelDirName and
the .e2e-complete marker to store catalog revision/hash metadata plus the
expected file manifest, then validate that metadata and all expected cached
files against the current model before returning. Treat missing, stale,
mismatched, or incomplete cache data as a cache miss and rerun model preparation
instead of trusting existsSync(marker).
- Around line 61-66: Update ensureEmbedderModel’s per-file fetch loop to use an
AbortSignal timeout for each Hugging Face download, and add bounded retries with
a clear final failure when the deadline or request fails. Preserve the existing
response validation and file-writing behavior, ensuring stalled downloads cannot
wait for the broader E2E timeout.

In `@e2e/tests/embedder.spec.ts`:
- Around line 23-30: Update the test setup around createSeededUserDataDir,
installEmbedderModel, and launchApp to retain userDataDir and clean it up when
setup throws before app is assigned. Add setup failure cleanup while preserving
normal app lifecycle cleanup and avoiding removal of user data still owned by
the running app.
🪄 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: 4236a18d-2b33-4bd5-b766-0fb9a1a53eca

📥 Commits

Reviewing files that changed from the base of the PR and between 3f9739c and 8594956.

⛔ Files ignored due to path filters (1)
  • docs/implementation/milestones/03-memory-floor/slices/01b-embedder-lifecycle.md is excluded by !docs/**
📒 Files selected for processing (8)
  • .github/workflows/ci.yml
  • e2e/flows/create-story.ts
  • e2e/harness/embedder.ts
  • e2e/locators/home.ts
  • e2e/locators/wizard.ts
  • e2e/tests/embedder.spec.ts
  • e2e/tests/wizard.spec.ts
  • package.json
🚧 Files skipped from review as they are similar to previous changes (2)
  • e2e/locators/home.ts
  • .github/workflows/ci.yml

Comment thread e2e/harness/embedder.ts Outdated
Comment thread e2e/harness/embedder.ts Outdated
Comment thread e2e/harness/embedder.ts
Comment thread e2e/tests/embedder.spec.ts Outdated
Comment on lines +23 to +30
const { userDataDir } = createSeededUserDataDir()
;({ modelId, dim: expectedDim } = await installEmbedderModel(userDataDir))
app = await launchApp({ userDataDir, cleanupUserData: true })
})

test.afterAll(async () => {
await app?.close()
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Clean up temporary user data when setup fails.

If installEmbedderModel() or launchApp() throws, app is never assigned and afterAll cannot remove the directory created by createSeededUserDataDir(). Track userDataDir and remove it in a setup catch/finally path.

🤖 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/embedder.spec.ts` around lines 23 - 30, Update the test setup
around createSeededUserDataDir, installEmbedderModel, and launchApp to retain
userDataDir and clean it up when setup throws before app is assigned. Add setup
failure cleanup while preserving normal app lifecycle cleanup and avoiding
removal of user data still owned by the running app.

failerko and others added 10 commits July 23, 2026 23:13
A turn on seeded data failed preflight with config-resolver/no-profile-assigned: with no classifier profile assigned, the pipeline planned the piggyback fallback classifier, whose resolve had nothing to bind. Assign a classifier profile (kind agent) and mark the narrative model taggedBlockReliable so piggyback also rides in-band. Turns and the periodic classifier now resolve on seeded data — the payoff of the earlier fixture-id fix.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Local OpenAI-compatible mock: one endpoint, routed by the request — streaming SSE prose for the narrative call, JSON chat completion for the non-streaming structured classifier — with CORS so the renderer's cross-origin fetch isn't blocked in dev. setProviderEndpoint repoints the seeded provider at it before launch. New spec submits a user action in the seeded reader and asserts, through the app DB bridge, that both the user action and the streamed AI reply commit to the branch — the real transport + pipeline, only the LLM mocked. Green in dev and packaged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The structured route served one canned body, so a second structured agent would get a mismatched shape. Route instead by the exact TypeScript block the app injects (schemaToTypeScriptBlock over each agent's zod schema) — self-syncing since it reuses the app's own renderer, so adding an agent is one STRUCTURED_AGENTS entry. New spec disables piggyback to force the per-turn fallback classifier, then asserts a single turn fans out on one endpoint into a streamed narrative and a structured call the mock matched to the classifier schema. Exports schemaToTypeScriptBlock and fallbackClassifierSchema for the match.

Covers the auto (prompt-injection) path we use; the force-on native path would match on response_format instead.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The section predated the implementation ("canned responses per prompt shape"). Describe what shipped: the dynamic endpoint override at seed time, CORS for the renderer's cross-origin fetch, and the one-endpoint routing — streaming SSE narrative vs structured calls matched to an agent by the exact schemaToTypeScriptBlock the app injects, with the auto-vs-force-on paths called out.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A force-on profile takes the native structured path, but the app never sets the openai-compatible provider's supportsStructuredOutputs, so the SDK emits response_format {type:json_object} with no schema and no prompt-injected block either. New spec forces the classifier call under a force-on profile and asserts exactly that, documenting why every other E2E relies on the prompt-embedded (auto) schema; it flags the change if the provider flag is ever wired. Doc updated, plus embedder-download flow marked manual-only.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The profile flag exists and round-trips through the DB but has no UI and never reaches the provider (supportsStructuredOutputs unset), so force-on emits schema-less json_object. Captures the wiring + capability-gate + optional-to-nullable schema work, pointing at the structured-force-on spec as the guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
E2E enters the chain as a planning decision, not a dedicated agent: plan-slice gains a "Decide E2E coverage" step (does the slice change a cross-subsystem seam or user flow?), and when it does the plan carries an E2E task + pnpm test:e2e as a verification command. verification-before-completion adds the flow-works row; test-driven-development names E2E as the third layer beside unit and Storybook. The how already auto-loads via .claude/rules/testing.md, so any per-task subagent that touches e2e/ gets it — no new agent type, and most slices skip it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The E2E discipline had every anchor but this one — canonical doc, auto-loading rule, README index, code-conventions pointer, skill-chain gate — yet the doc itself wasn't in the always-loaded reading list beside its peers. Add it; clarify code-conventions covers unit/component testing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Prettier turned the wrapped "Playwright + Electron" into a nested list item (the ' + '-separator wrap trap); use a slash so it stays one bullet.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The discipline said what E2E owns and when a slice needs one, but never how much. Pin it: broad across flows, shallow within — every happy path, common alternative flows, and common edge cases that cross a seam; pure-logic branches and rare permutations stay in unit/Storybook. The boundary test is whether a running app is the only place the behavior is observable. plan-slice's gate now points at it so "carry an E2E task" carries the depth.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
e2e/harness/seed.ts (1)

24-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract shared read/mutate/write boilerplate.

setProviderEndpoint, setProfileStructuredOutput, and disablePiggybackCapability each repeat the same open → read column → JSON.parse → mutate → UPDATEclose sequence against app_settings. Consider a small shared helper (e.g. withSingletonColumn(dbPath, column, mutate)) to remove the duplication.

♻️ Example shared helper
+function withSingletonColumn<T>(
+  dbPath: string,
+  column: string,
+  mutate: (value: T) => void,
+): void {
+  const db = new DatabaseSync(dbPath)
+  try {
+    const row = db.prepare(`SELECT ${column} FROM app_settings WHERE id = 'singleton'`).get() as
+      Record<string, string>
+    const value = JSON.parse(row[column]) as T
+    mutate(value)
+    db.prepare(`UPDATE app_settings SET ${column} = ? WHERE id = 'singleton'`).run(
+      JSON.stringify(value),
+    )
+  } finally {
+    db.close()
+  }
+}
🤖 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 24 - 90, Extract the repeated singleton
app_settings read/parse/mutate/update/close flow from setProviderEndpoint,
setProfileStructuredOutput, and disablePiggybackCapability into a shared helper
such as withSingletonColumn. Have the helper accept dbPath, the column name, and
a mutation callback, then update each function to use it while preserving their
existing mutations and cleanup behavior.
e2e/tests/classifier.spec.ts (1)

13-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate dbRows helper across specs. The exact same async dbRows(page, sql, params) helper (querying window.aventurasDb.query) is defined independently in both spec files.

  • e2e/tests/classifier.spec.ts#L13-L26: move this helper into a shared harness module (e.g. e2e/harness/db.ts) and import it here.
  • e2e/tests/turn.spec.ts#L9-L22: import the same shared helper instead of redefining it.
🤖 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.spec.ts` around lines 13 - 26, The duplicated dbRows
helper should be centralized in a shared harness module. Move the async dbRows
implementation from e2e/tests/classifier.spec.ts lines 13-26 into
e2e/harness/db.ts and import it there; in e2e/tests/turn.spec.ts lines 9-22,
remove the local definition and import the same shared helper.
lib/db/devtools/seed-dataset.ts (1)

1612-1649: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Keep 'story_hero' references usable after remapping. remapSeedIds() leaves story_hero unchanged, but the seed comment says mnemonic story IDs are “off-spec,” so future additions/remaps to non-canonical story_* seeds could break these e2e selectors. Import/extract the canonical stories.id value from the seed module or anchor these queries by a stable story attribute instead of hardcoding the memo mnemonic.

🤖 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/devtools/seed-dataset.ts` around lines 1612 - 1649, Replace hardcoded
story_hero selectors with the canonical stories.id value exported or extracted
from the seed module, or use a stable story attribute for lookup. Update the
affected references in lib/db/devtools/seed-dataset.ts (1612-1649),
e2e/tests/classifier.spec.ts (72-80), and e2e/tests/turn.spec.ts (65-67); keep
all selectors usable after remapSeedIds() and do not rely on mnemonic story 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.

Nitpick comments:
In `@e2e/harness/seed.ts`:
- Around line 24-90: Extract the repeated singleton app_settings
read/parse/mutate/update/close flow from setProviderEndpoint,
setProfileStructuredOutput, and disablePiggybackCapability into a shared helper
such as withSingletonColumn. Have the helper accept dbPath, the column name, and
a mutation callback, then update each function to use it while preserving their
existing mutations and cleanup behavior.

In `@e2e/tests/classifier.spec.ts`:
- Around line 13-26: The duplicated dbRows helper should be centralized in a
shared harness module. Move the async dbRows implementation from
e2e/tests/classifier.spec.ts lines 13-26 into e2e/harness/db.ts and import it
there; in e2e/tests/turn.spec.ts lines 9-22, remove the local definition and
import the same shared helper.

In `@lib/db/devtools/seed-dataset.ts`:
- Around line 1612-1649: Replace hardcoded story_hero selectors with the
canonical stories.id value exported or extracted from the seed module, or use a
stable story attribute for lookup. Update the affected references in
lib/db/devtools/seed-dataset.ts (1612-1649), e2e/tests/classifier.spec.ts
(72-80), and e2e/tests/turn.spec.ts (65-67); keep all selectors usable after
remapSeedIds() and do not rely on mnemonic story IDs.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 05184ce9-c32f-4fca-bbd4-a110b97e83fb

📥 Commits

Reviewing files that changed from the base of the PR and between 8594956 and 0d43356.

⛔ Files ignored due to path filters (2)
  • docs/implementation/triage.md is excluded by !docs/**
  • docs/testing.md is excluded by !docs/**
📒 Files selected for processing (13)
  • .agents/skills/aventuras-plan-slice/SKILL.md
  • .agents/skills/aventuras-test-driven-development/SKILL.md
  • .agents/skills/aventuras-verification-before-completion/SKILL.md
  • CLAUDE.md
  • e2e/harness/mock-llm.ts
  • e2e/harness/seed.ts
  • e2e/tests/classifier.spec.ts
  • e2e/tests/structured-force-on.spec.ts
  • e2e/tests/turn.spec.ts
  • lib/ai/index.ts
  • lib/db/devtools/seed-dataset.ts
  • lib/pipeline/definitions/per-turn-piggyback.ts
  • lib/pipeline/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • CLAUDE.md

failerko and others added 4 commits July 24, 2026 01:21
The suite is at happy-path core (one per seam); docs/testing.md sets the bar at thorough. Queue the known alt-flow and seam-crossing edge-case gaps on flows that already exist, so the backfill isn't assumed done — new slices meet the bar via the plan-slice gate.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Harden the harness per review: launchApp cleans up the dev server + temp dir on launch/close failure (try/catch + finally, not sequential awaits); the embedder cache stores the catalog revision in its marker and re-downloads on a revision mismatch or missing file (a stale cache could otherwise pass the test against the wrong model); the download fetch gets a per-file timeout so a stalled HF connection fails cleanly; and createSeededUserDataDir self-cleans on seed failure while specs remove their temp dir in afterAll so a setup failure between seed and launch doesn't orphan it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
getByText('E2E-FORCEON') matched both the AI reply and the user action ("E2E-FORCEON-USER …") — a strict-mode violation that surfaced intermittently as the two entries raced into the DOM. Give the reply a distinct E2E-FORCEON-REPLY token.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces the AVENTURAS_E2E_MODE / AVENTURAS_E2E_APP_PATH / AVENTURAS_E2E_MODEL_DIR env contract (CodeRabbit: the repo bans env-driven config) with dev/packaged Playwright projects — the harness reads the project name, `pnpm test:e2e` runs dev, `test:e2e:packaged` runs packaged, and CI invokes the packaged script instead of exporting a var. Reporter is deterministic (list + github, the latter a no-op off Actions) and retries:1 absorbs a transient window/IPC hiccup. EXPO_WEB_URL stays — it's the app's own dev-mode contract passed to the Electron child.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@failerko
failerko merged commit b62ac05 into main Jul 24, 2026
4 checks passed
@failerko
failerko deleted the test/e2e-setup branch July 24, 2026 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant