Modernize the runtime and agent tooling - #49
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 168 files, which is 18 over the limit of 150. To get a review, narrow the scope: Upgrade to Pro+ to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (181)
You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one behavior worth confirming before merge, noted inline.
Reviewed changes — a broad runtime/tooling modernization plus one genuinely new persistence feature; the large majority of the 184 files are vendored agent-skill markdown, so review focused on the code and config diffs.
- Runtime + dependency bump — Node
>=24 <25, Bun1.3.14, TypeScript7.0.2, vitest/tsdown/oxlint/oxfmt/ultracite/turbo upgrades, and a deliberate@types/nodedowngrade (25.x→24.13.3) to match the Node 24 line. - Effect ecosystem aligned to
4.0.0-beta.101— with the v4 API migration:effect/ServiceMap→effect/Contextfor every*.Servicetag, andSchema.withDecodingDefaultKey(() => x)→Schema.withDecodingDefaultKey(Effect.succeed(x))acrosspackages/backend/src/routes/schemas.ts. - New durable chat list/queue persistence — migration
0003-chat-runtime-lists-and-queues(per-dialect DDL + indexes), newappendToList/getList/enqueue/dequeue/queueDepth/forceReleaseLockrepository methods inpersistence.ts, theChatRuntimeStateRepositorycontract intypes/domain.ts, adapter wiring incore/src/chat.ts, in-memory fixtures, and tests. - TypeScript 7 isolation —
check-tsdoc-coverage.tsnow imports@typescript/typescript6,packages/dsartypecheck movedtsgo→tscwhile the tsdown dts generator staystsgo, and asuppressWarningsentry hides the TS7-experimental-API notice. - Tooling — new
scripts/prepare-effect.sh+prepare:effectscript,chatSDK bump (^4.18→^4.34) with adapter/resend bumps, refreshed AGENTS.md/CLAUDE.md and GitHub Actions config.
ℹ️ New list/queue TTL path is untested
The added persistence test and the in-memory fixtures never exercise list or queue expiry — expiresAt is always set 60s in the future and appendToList is never called with a TTL. The two in-memory fixtures also disagree with the SQL path on expiry semantics (chat-memory-state.ts has no list-expiry concept at all; the chat-state.test.ts fixture skips queue-expiry filtering that the SQL implementation performs), so a regression in the SQL TTL logic would not be caught by the current tests.
Technical details
# New list/queue TTL path is untested
## Affected sites
- `packages/internals/persistence/test/persistence.test.ts:939` — new "persists bounded chat lists and queues" test never appends with a TTL and never lets an entry expire.
- `packages/core/src/test-fixtures/chat-memory-state.ts` — `appendToList`/`getList` ignore `ttlMs` entirely (no list expiry).
- `packages/core/test/chat-state.test.ts` — fixture `enqueue`/`dequeue`/`queueDepth` do not filter by `expiresAt`, unlike the SQL implementation and unlike the queue logic in `chat-memory-state.ts`.
## Required outcome
- A test that appends/enqueues with a short TTL, advances the clock past it, and asserts `getList`/`dequeue`/`queueDepth` drop the expired entries against the real SQL implementation.
## Suggested approach (optional)
The existing `runForTenant` harness with a controllable clock (as used by the lock-extension tests) is the natural place to add this — no new scaffolding needed.Claude Opus ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the single new commit b1aa4f3 reworks how chat list expiry is stored, directly addressing the prior review's concern about append-order-dependent per-row TTLs.
- Per-list expiry moved to a dedicated table — new
chat_state_list_keys (tenant_id, list_key, expires_at)owns each list's expiry;chat_state_listsdrops itsexpires_atcolumn and gains a compositeFOREIGN KEY ... ON DELETE CASCADEback to it. appendToListexpiry is now well-defined — a suppliedexpiresAtrefreshes the whole-list expiry, while a ttl-less append preserves the current expiry viaON CONFLICT DO UPDATE SET expires_at = COALESCE(excluded.expires_at, existing), eliminating the divergent per-rowNULLwrite.- Lazy expiry cleanup made transactional —
appendToList,getList, anddeleterun insidesql.withTransactionand consistently purge bothchat_state_listsandchat_state_list_keys; correctness does not depend on FK cascade enforcement since explicit deletes cover every path. - Migration + conformance updated — migration
0003creates the new table andidx_chat_list_keys_tenant_expiry, drops them on revert, and the conformance test asserts both the added table and index. - New TTL test —
keeps list TTL scoped to the whole list across ttl-less appendsverifies preservation across a ttl-less append, whole-list expiry, and a fresh no-ttl append after expiry.
The prior review's inline TTL thread is resolved by this commit. The in-memory fixture divergence noted in the initial review remains open but is unchanged by this commit, so it is not re-raised here.
Claude Opus ｜ 𝕏
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
Validation
bun install --frozen-lockfilebun x ultracite doctorbun x ultracite checkbun run checkbun run typecheckbun run testbun run buildCI=1 bun run --cwd packages/dsar buildNotes
4.0.0-beta.101, the newest release eligible under the repository age gate when refreshed.bun run smokerequires a running DSAR server and an API token, so it was not run in the local validation environment.