feat: turn the session-storage rework on by default - #5560
Conversation
dosubot
Bot
added
the
size:L
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughSession reconstruction, last-message-only request reduction, and durable record persistence now default to enabled. The literal ChangesSession behavior defaults
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant buildAgentRequest
participant reconstructHistoryIfNeeded
participant HistoryAPI
Client->>buildAgentRequest: build last-message-only request
buildAgentRequest->>reconstructHistoryIfNeeded: provide minimal history
reconstructHistoryIfNeeded->>HistoryAPI: fetch history unless flag is "false"
HistoryAPI-->>reconstructHistoryIfNeeded: return reconstructed history
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/packages/agenta-playground/tests/unit/agentRequest.test.ts (2)
186-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid introducing
anyin the runtime-env helper.The new
globalThis as anycast bypasses type checking in a workspace package. Define a typed__envshape instead.Proposed fix
+type TestRuntimeGlobal = typeof globalThis & { + __env?: Record<string, string> +} + const setFlag = (value: string) => { - ;(globalThis as any).__env = {NEXT_PUBLIC_SESSIONS_LAST_MESSAGE_ONLY: value} + ;(globalThis as TestRuntimeGlobal).__env = { + NEXT_PUBLIC_SESSIONS_LAST_MESSAGE_ONLY: value, + } }As per coding guidelines, workspace packages should avoid
anyand legacy compatibility shims.Source: Coding guidelines
199-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover trimmed, case-insensitive
"false"values.The parser contract is case-insensitive and trims whitespace, but these tests only exercise exact values. Add a case such as
" FALSE "to prevent regressions in normalization.Proposed test
+ it("treats trimmed, case-insensitive false as disabled", async () => { + setFlag(" FALSE ") + expect(await outMessages([u1, a1, u2])).toEqual([u1, a1, u2]) + })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2deb58c2-e7aa-4f85-bd6e-5e17e66e2201
📒 Files selected for processing (17)
hosting/docker-compose/ee/docker-compose.dev.ymlhosting/docker-compose/ee/docker-compose.gh.ymlhosting/docker-compose/ee/env.ee.dev.examplehosting/docker-compose/ee/env.ee.gh.examplehosting/docker-compose/oss/docker-compose.dev.ymlhosting/docker-compose/oss/docker-compose.gh.ymlhosting/docker-compose/oss/env.oss.dev.examplehosting/docker-compose/oss/env.oss.gh.exampleservices/runner/src/engines/sandbox_agent/reconstruct-history.tsservices/runner/src/engines/sandbox_agent/run-turn.tsservices/runner/src/sessions/persist.tsservices/runner/tests/setup/hermetic-env.tsservices/runner/tests/unit/session-persist.test.tsservices/runner/tests/unit/session-reconstruct-history.test.tsweb/packages/agenta-playground/src/state/execution/agentRequest.tsweb/packages/agenta-playground/tests/unit/agentRequest.test.tsweb/packages/agenta-shared/src/api/env.ts
| * from the durable record log. ON unless set to the literal "false"; absent AND empty both mean | ||
| * on (compose passes `${VAR:-}`, which sets an empty string when unset). Disable it ONLY | ||
| * together with the backend `AGENTA_SESSIONS_RECONSTRUCT=false`, or a cold turn loses its | ||
| * context. | ||
| */ | ||
| export const isSessionsLastMessageOnlyEnabled = (): boolean => { | ||
| const raw = getEnv("NEXT_PUBLIC_SESSIONS_LAST_MESSAGE_ONLY") || "false" | ||
| return SANDBOX_LOCAL_TRUTHY.has(raw.trim().toLowerCase()) | ||
| const raw = getEnv("NEXT_PUBLIC_SESSIONS_LAST_MESSAGE_ONLY") | ||
| return raw.trim().toLowerCase() !== "false" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve explicitly empty runtime overrides.
The new default-on behavior is correct only if getEnv returns the runtime value. However, Line 52 treats runtimeEnv[envKey] as a truthy presence check, so __env.NEXT_PUBLIC_SESSIONS_LAST_MESSAGE_ONLY = "" falls back to build-time processEnv. A build produced with "false" will therefore remain disabled despite the documented empty-value default.
Use an own-property check so absent and explicitly empty values remain distinct.
Proposed fix
- if (runtimeEnv && Object.keys(runtimeEnv).length > 0 && runtimeEnv[envKey]) {
+ if (
+ runtimeEnv &&
+ Object.keys(runtimeEnv).length > 0 &&
+ Object.prototype.hasOwnProperty.call(runtimeEnv, envKey)
+ ) {
AGENTA_SESSIONS_RECONSTRUCT, AGENTA_RECORDS_DURABLE, and
NEXT_PUBLIC_SESSIONS_LAST_MESSAGE_ONLY now read as enabled unless set to the
literal "false". Absent and empty both mean on, because the compose files pass
the vars through as ${VAR:-}, which materializes an empty string that overrides
env_file values.
771cd87 to
15ed54d
Compare
|
🤖 The AI agent says: Addressed all three review comments in commit 15ed54d:
Verified with the focused agentRequest suite (39 tests), package build and lint for @agenta/shared and @agenta/playground, and the repository frontend lint-fix pass. |
Railway Preview Environment
|
1 participant
Context
The session-storage rework shipped dark in v0.106.1: durable record persistence in the runner (
AGENTA_RECORDS_DURABLE), history reconstruction from records (AGENTA_SESSIONS_RECONSTRUCT), and last-message-only sends from the web app (NEXT_PUBLIC_SESSIONS_LAST_MESSAGE_ONLY). All three defaulted to off and only the literal"true"enabled them. The feature was QA'd on a dedicated stack with the flags on and the findings from that pass are fixed and merged, so the flags should now be the default path.Change
All three read sites flip to on unless explicitly disabled: the predicate is now
value !== "false"(case-insensitive, trimmed) instead ofvalue === "true".Absent AND empty both mean on, deliberately. The compose files pass the variables through as
${VAR:-}, which materializes an empty string when the variable is unset in the shell, and a composeenvironment:entry overridesenv_file. A changed fallback string alone would therefore have left every compose deployment off. Only the literal"false"disables.Before: unset, empty, or anything but
"true"→ feature off.After: unset, empty, or anything but
"false"→ feature on. Set the variable tofalseto disable.Also in this commit:
env.*.examplefiles now document the flags as default-on with=falseto disable.services/runner/tests/setup/hermetic-env.tspinsAGENTA_SESSIONS_RECONSTRUCT=false: with the new default, 75 engine unit tests would otherwise reach for a live records endpoint. The reconstruct suite deletes the pin to test the real default."false"=off cases.AGENTA_RECORDS_SMART_TRUNCATION(a fourth, API-side flag) is out of scope and stays opt-in.Tests
@agenta/playground: 211/211 green.@agenta/shared: 294/294 green.pnpm lint-fixclean, reformatted nothing.What to QA
[sessions/persist] ingest OK), a resumed conversation reconstructs from records, and the web app sends only the trailing message on a turn.falseand recreate runner + web: behavior returns to the pre-rework path (full history sent, no durable ingest).https://claude.ai/code/session_01HyTAyASedjizTRN7sTbw4W