test(researcher): Playwright e2e suite and CI job - #680
Merged
Conversation
Researcher had no automated tests — the app with the only real Walrus writes was verified entirely by hand. This adds a 15-test Playwright suite and wires it into CI as `researcher-e2e`, mirroring the existing `chatbot-e2e` job. The suite's reason for existing is the first spec: the retired title-model P0 (c031c47) shipped because nothing would have caught it. `chat-stream.test.ts` now asserts the raw SSE frames directly — a failing title model must not put an `error` frame on the stream. Verified as a real regression test, not just a passing one: with the try/catch guard removed the spec fails with the exact production symptom, `{"type":"error","errorText":"Oops, an error occurred!"}`, while the answer still streams underneath. Four mock seams, all keyed off the existing `isTestEnvironment`, so a run never reaches OpenRouter, Sui, or the relayer: - `providers.ts` — picker model ids now map onto the three registered mock models. The pre-existing test branch passed the raw id straight through, so `languageModel("google/gemini-2.5-flash")` would have thrown NoSuchModelError on the first message; the branch had simply never run. - `models.mock.ts` — a `FAIL_TITLE_GENERATION` sentinel in a user message makes the title model reject, reproducing the P0 on demand. - `delegate-account.mock.ts` — fabricates the `MemWalAccount` object for two fixture identities. The real binding validation still runs, so unregistered keys and unknown accounts fail exactly as they do on-chain (both asserted). - `memwal.ts` — `MemWalMock` for the Walrus client, one instance per process, so CI can never write to the production relayer. Coverage: delegate-key login (form and rejections), streamed replies, reload persistence with zero uncaught page errors (guards the auto-resume TypeError from 39e102a), SSE frame protocol, and the Private/Public boundary across two real accounts. Two behaviors pinned as they actually are rather than as expected: a private chat opened by another user renders Next's not-found page with HTTP 200, because `<Suspense>` flushes the shell before `notFound()` runs; and anonymous visitors are redirected to /login even for public chats, since the auth check precedes the visibility check. Neither leaks content. The second is the known "Anyone with the link" product gap. `global-setup.ts` clears the auth limiter's Redis keys before each run. The limiter allows 10 verify attempts per IP per minute and a run spends five, so without it a second run inside a minute fails at sign-in with a 429 that looks nothing like the real problem. Both identities also sign in once in a setup project rather than per-test, which keeps CI retries clear of the limit. Verified locally: 15/15 green on three consecutive runs (~10s each), from cold with Playwright managing its own webServer and with AUTH_SECRET and the package id unset, so the config defaults used by CI are exercised.
ducnmm
pushed a commit
that referenced
this pull request
Aug 20, 2026
Noter had zero automated tests; the on-chain registration flow, delegate-key auth, and note CRUD were verified entirely by hand. 22 specs across app shell, auth, note lifecycle, and the memory API contract, running against a real Next.js dev server and a fresh Postgres. Mock seam for the new delegate-account binding check ------------------------------------------------------ connectDelegateKey now calls assertDelegateAccountBinding (a separate, already-merged change), which reads the claimed account off-chain via gRPC and rejects any key that isn't registered in its delegate_keys list — so a random, never-registered key/account pair can no longer reach an authenticated session the way it could before that change landed. Mirroring researcher's PR #680 pattern, delegate-account.ts now branches on lib/constants.ts's isTestEnvironment (set by playwright.config.ts passing PLAYWRIGHT=True to the webServer) and serves a fixture object from delegate-account.mock.ts instead of the gRPC read, so the real validation logic still runs meaningfully: an unknown account or an unregistered key fails the exact same way it would on-chain. Noter authenticates a fresh identity per test rather than reusing two shared identities across a whole run (researcher's approach) — with `workers: 2` and ~15 login call sites, two fixed identities would have concurrent tests collide on each other's notes. delegate-account.mock.ts and fixtures/delegate-key.ts instead generate the same 24-entry deterministic pool independently (index N -> accountId byte N repeated, privateKey byte N+0x40 repeated), and the test fixture hands out a never-yet-used entry per call, interleaved by Playwright's parallelIndex so two worker processes never claim the same one. public_key is stored base64, not hex: the binding check's parser tries fromBase64() before falling back to raw hex, and every 64-char hex string (alphabet 0-9a-f, always length-divisible-by-4) also happens to be valid-but-wrong base64, so a hex value there silently decodes to the wrong bytes instead of ever matching. The memory-write specs assert against the real relayer response for a fixture (unregistered) key, so there's no live-Walrus canary in this suite by design, same as #680 documents for researcher: the real remember -> recall round trip against production Walrus Memory stays a manual check. CI job ------ noter-e2e mirrors chatbot-e2e's shape (Postgres service container, cached Playwright browsers, report/trace upload on failure). noter-checks adds tsc --noEmit and a full `next build` so a type or build regression fails CI even on a change the e2e specs don't happen to cover.
ducnmm
added a commit
that referenced
this pull request
Aug 24, 2026
* fix(sidecar): durable-upload crash-recovery tag never matched the reconcile lookup
The whole point of PR #562's idempotency work is: if the sidecar crashes
right after minting a paid Walrus blob but before its journal write lands,
a retry must find and adopt that orphaned blob instead of minting a second
paid one. The lookup that's supposed to find it was searching for the wrong
tag.
The durable-upload register step (walrus-upload-journal.ts — the only path
every real /api/remember call goes through today) tagged the minted blob's
on-chain metadata with memwal_migration_job, an unrelated, dead-code
constant borrowed from the V1->V2 migration feature (its only reader,
findOwnedBlobObjects, has zero callers anywhere in the codebase). The
reconcile scan that runs on a lost journal, scanOwnerForJobBlob
(walrus-query.ts), searches for memwal_job_id instead. The two could never
match, so a crash at exactly the wrong moment would silently mint twice —
the precise failure PR #562 exists to prevent.
memwal_job_id was already the correct key, used correctly, by the legacy
(non-durable) upload path — but that path is dead code today, since every
current caller sets remember_job_id: Some(...), which always routes into
the durable path instead.
Fixes it by introducing a single shared MEMWAL_JOB_TAG_KEY constant in
util.ts and having both write sites (durable + legacy) and the read site
import it instead of each hardcoding the string literal, so the two sides
structurally cannot drift apart again without someone deliberately
un-importing the shared constant.
New regression test pins the constant's value and greps all three files to confirm none hardcodes a competing 'memwal_job_id' literal. 193/193 sidecar scripts tests pass (7/7 in the directly affected files). tsc --noEmit: no new errors (one pre-existing, unrelated error in mcp/__tests__/integration.test.ts, confirmed present on unmodified dev too).
* test(sdk): live e2e suite against the relayer
Mirrors the Python SDK's tests/test_integration.py: no-auth health,
compatibility and auth-rejection checks that always run, plus
authenticated remember/recall/analyze/restore coverage (and the
JS-only bulk, manual-mode and embed surfaces) that skips without
MEMWAL_PRIVATE_KEY / MEMWAL_ACCOUNT_ID. Authenticated writes land in
a per-run sdk-e2e-<random> namespace so the shared bench account's
real namespaces stay clean, and remember waits get 120s of headroom
over the SDK's 60s default (a live write measures ~44s on dev).
Named *.e2e.mjs so the offline unit glob (test/**/*.test.mjs) never
picks the suite up; run it via the new test:e2e script. [WALM-353]
* ci(sdk): unit test and e2e workflow against the dev relayer
Sibling of test-python-sdk.yml. The unit job runs the offline suite on
Node 22 and 24 for every PR touching packages/sdk — closing the gap
where SDK unit tests never ran in CI at all. The e2e job reuses the
benchmark-dev environment credentials, is limited to dev pushes,
manual dispatch and a weekly cron (PRs are excluded: fork PRs get no
environment secrets, and every authenticated run writes real
memories), fails loudly when credentials are missing instead of
green-skipping, and uploads a junit artifact plus a run summary.
The cron sits 30 minutes after the Python suite's slot so the two
weekly runs don't write through the shared bench account at the same
time. [WALM-353]
* test(sdk): drop e2e coverage for two unserved SDK contracts
Review of the authenticated half — which cannot run locally without
the bench credentials — caught two tests that were guaranteed to go
red on the first CI run:
- embed() POSTs /api/embed, which is absent from the relayer's
protected route table entirely.
- rememberManual() sends blob_id, but RememberManualRequest requires
encrypted_data and has no blob_id field, so axum 422s the call
before the handler sees it.
Both methods are covered only by MemWalMock today, which is why the
drift went unnoticed. The suite documents the omission inline rather
than deleting it silently; the SDK bug is tracked in WALM-371.
Also hardens the remaining live tests: the analyze fan-out gets twice
the single-write budget (N facts, one wallet-job pipeline each, on a
contended shared account), the one-shot status probe tolerates a
background 'failed' since it asserts acceptance rather than pipeline
health, and MEMWAL_REMEMBER_TIMEOUT_MS now rejects unparseable input
instead of silently becoming NaN. [WALM-353]
* ci(sdk): run the e2e suite against dev, staging and production
The dev-only restriction was inherited from test-python-sdk.yml, whose
comment justified it as 'staging and production follow once their
credentials exist'. That premise no longer holds: benchmark-staging
and benchmark-production both carry BENCH_DELEGATE_KEY,
BENCH_ACCOUNT_ID and BENCH_SERVER_URL today.
Each long-lived branch now tests the deployment it corresponds to —
dev, staging, main -> production — and workflow_dispatch takes an
explicit environment choice, following the selection pattern
benchmark-live.yml already uses. Scheduled runs stay on dev, since
cron fires on the default branch, so production is only reached by an
explicit main push or a deliberate dispatch.
Drops the hardcoded relayer URL fallback. With one environment a
literal default guarded against an unset variable; with three it
becomes the hazard it was meant to prevent, since a production run
with a missing variable would silently exercise dev. The credential
check now requires MEMWAL_SERVER_URL and fails the job without it.
The suite is safe to point at production because every authenticated
write is confined to a per-run sdk-e2e-<random> namespace; the only
reference to 'default' is the body of the 401-rejection tests, which
never write. [WALM-353]
* fix(mcp): resolve credentials per project and warn before replacing an account
Credentials lived in one global `~/.memwal/credentials.json`. Signing in from
one project repointed every other project on the machine at a different account
and delegate key, with the `label` field as the only visible signal and no
warning at the point of use. Memories written in that state land on the wrong
account, on immutable storage, with no delete path.
A `.memwal/credentials.json` in the working directory now takes precedence over
the global file, the way `.npmrc` and `.git/config` resolve. Presence-based on
purpose: creating the local file is the opt-in, so a machine without one behaves
exactly as before. Paths resolve per call rather than at module load, since the
working directory is not knowable at import time.
Replacing a DIFFERENT account now copies the outgoing file aside and reports
both ids — the account replaced and the one now in use. There was no backup of
any kind before, so an overwrite was unrecoverable. Same-account re-saves (a
label change, a rotated delegate) are left alone rather than churning a backup
per login.
`saveCreds` returns what it did instead of printing directly: the persistence
layer should not own user-facing output, and returning it keeps the behaviour
testable without capturing stdout.
Note on the recorded decision to refuse a different-account replacement unless
forced: the incoming `accountId` only arrives in the browser callback, after the
user approved and the delegate key was already registered on-chain. Refusing
there would discard a registration that cost gas and already grants access, so
this warns and backs up instead. A pre-approval notice belongs earlier in the
flow, where the outgoing account and target path are already known.
Refs #628 (WALM-361).
* fix(mcp): make the replacement notice testable and stop the success page lying
Two gaps in the previous commit, both found by asking what was actually
verified rather than what was written.
The warning itself had no test. The unit tests asserted what `saveCreds`
returns, not that anything is ever shown — so the user-facing half of the fix,
which is the whole point of GH #628, rested on inspection. Extracted
`formatReplacementNotice` as a pure function and covered both arms: a
replacement names the outgoing account, the incoming one and the backup, and a
first sign-in or same-account re-save stays quiet.
The browser success page hardcoded "Credentials saved to
~/.memwal/credentials.json". Now that a project-local file can be the
destination, that sentence was capable of naming a file the login had not
touched — telling a user the wrong credential path is worse than telling them
nothing, especially in the exact scenario this ticket exists to fix. It now
reports the path actually written.
Also verified with the real binary, which the unit tests do not cover: two
projects sharing one HOME, one with a local `.memwal/` and one without, load
different accounts. Same command, same environment, only the working directory
differs.
Refs #628 (WALM-361).
* fix(mcp): stop `login` deleting credentials before the new ones exist
Running the live check against the real binary showed the replacement warning
never fired from `memwal-mcp login`, and no backup was written — while the unit
tests were green.
`login` called `clearCreds()` up front, purely so the `loadCreds()` below would
return null and the sign-in flow would run. By the time `saveCreds` executed
there was no file left to compare against, so the account change was invisible
and there was nothing to back up.
The destructive part was never needed: forcing a fresh sign-in means ignoring
what is on disk, not deleting it. `login` now passes null instead, so the old
file survives until a successful sign-in replaces it.
That also closes a data-loss path that predates this ticket. Anyone who ran
`memwal-mcp login` and then abandoned the browser flow, or whose login timed
out or failed, was left with no credentials at all and nothing to recover from
— the delegate key on disk was gone before the new one was ever issued.
Adds the live check that caught it (`.live.mjs`, outside the `npm test` glob).
It drives the real login process end to end — its own listener, preflight,
callback parsing and save — and asserts on what the process actually printed.
The browser half is driven programmatically, so no wallet approval and no
on-chain registration; the replaced credentials are seeded locally, which is
all the account comparison keys off. 10/10 pass.
Refs #628 (WALM-361).
* docs(mcp): document credential locations, and warn before the browser step
Completes the two acceptance criteria left open on WALM-361.
Warn ahead of approval, not only after. The incoming account is unknown until
the callback arrives, by which point a delegate key is already registered
on-chain — so a warning that waits for both ids arrives too late to act on. The
browser step is the last moment the user can back out for free, so the account
currently saved and the file at risk are named before it opens. The
after-the-fact notice naming both ids stays; the two answer different questions.
Document the resolution order, migration, backup and recovery in the MCP
reference: how the two locations resolve, how to scope a project to its own
account, that migration is a no-op because creating the local file is the
opt-in, what a same-account versus different-account sign-in does, and how to
restore from a backup. Existing sections that hardcoded ~/.memwal — first-run
behaviour, the credential-file section, memwal_logout, and two CLI table rows —
now point at the resolved location instead of asserting the global one.
Also notes that backups are never pruned and each holds a delegate private key,
and that `.memwal/` belongs in .gitignore.
Refs #628 (WALM-361).
* test(mcp): correct the portability note on the credential sandbox
The comment claimed the realpath call was a macOS concern. It is not: cwd and
homedir both report resolved paths on every platform, and the sandbox already
sets HOME and USERPROFILE together, which is what os.homedir() needs on Windows
and POSIX respectively. Nothing in these tests is POSIX-only — the reason they
are unproven on Windows is that the repository has no Windows runner.
* fix(noter): stop bouncing authenticated users off /note and dropping the delegate-key error message
sessionAtom was atomWithStorage(..., { getOnInit: false }), so every fresh
page load (a window.location.href redirect after login, or a plain reload of
/note) rendered once with session=null before the async post-mount hydration
from sessionStorage caught up. useAuth's own effect read that stale null on
the same tick, concluded "logged out", and cleared isLoading before the real
session arrived, so /note's guard fired router.replace("/") on an already
authenticated user. getOnInit is safe to flip to true here: nothing renders
`session` directly, every page branches on the separate authAtom instead,
which always starts isLoading:true on both server and client, so there is no
markup for the eager read to mismatch against.
Separately, connectEnoki/connectDelegateKey flipped the global authAtom.isLoading
flag for the duration of their mutation, and app/page.tsx renders
<AuthButtonGroup /> only while !isAuthenticated && !isLoading. That unmounted
the login form mid-submit, taking its local `error` state with it before the
catch block's setError() could run, so an invalid delegate key just dropped
the user back on a silently collapsed form. isLoginPending (from the mutation
hooks themselves) already tracks per-call pending state for the submit
button, so the global flag no longer needs to move during a login attempt.
* fix(noter): migrate the client-side Enoki registration flow off deprecated Sui JSON-RPC
Sui's public JSON-RPC fullnodes were deprecated in 2026; fullnode.testnet.sui.io
now answers every JSON-RPC call with "Method not found ... migrate to gRPC or
GraphQL endpoints" and no CORS header, which Chrome reports as a generic
"blocked by CORS policy" failure. sui-providers.tsx's getJsonRpcFullnodeUrl and
enoki-login-card.tsx's useSuiClient() both hit that dead endpoint, so
registerEnokiWallets failed on mount ([enoki-login] Setup failed: TypeError:
Failed to fetch) and Google sign-in never got past the landing page.
dapp-kit's SuiClientProvider is still hard-typed to SuiJsonRpcClient even in
the latest published version (1.1.17), so it can't be pointed at a gRPC
client directly. Enoki's own `client` option and Transaction.build()'s
`client` option both accept the broader ClientWithCoreApi interface instead,
which SuiGrpcClient satisfies, so this bypasses SuiClientProvider only where
it was actually blocking things: registerEnokiWallets now gets a standalone
SuiGrpcClient (lib/sui/grpc-client.ts) instead of useSuiClientContext()'s
client, and enoki-login-card.tsx's on-chain reads (registry lookup, dynamic
field, transaction/event fetch) move to the gRPC client's include/mask-based
API. SuiClientProvider itself stays in place for WalletProvider's
wallet-standard connect/sign, which doesn't touch RPC directly.
gRPC object/dynamic-field/event reads return raw BCS bytes instead of
JSON-RPC's parsed `.fields`, so lib/sui/account-bcs.ts adds the BCS schemas
needed to decode them, verified by decoding a live testnet account and
matching its stored delegate public key, and round-tripping a registry
dynamic-field lookup back to the same account id.
* test(noter): add Playwright e2e suite and CI job
Noter had zero automated tests; the on-chain registration flow, delegate-key
auth, and note CRUD were verified entirely by hand. 22 specs across app
shell, auth, note lifecycle, and the memory API contract, running against a
real Next.js dev server and a fresh Postgres.
Mock seam for the new delegate-account binding check
------------------------------------------------------
connectDelegateKey now calls assertDelegateAccountBinding (a separate,
already-merged change), which reads the claimed account off-chain via gRPC
and rejects any key that isn't registered in its delegate_keys list — so a
random, never-registered key/account pair can no longer reach an
authenticated session the way it could before that change landed. Mirroring
researcher's PR #680 pattern, delegate-account.ts now branches on
lib/constants.ts's isTestEnvironment (set by playwright.config.ts passing
PLAYWRIGHT=True to the webServer) and serves a fixture object from
delegate-account.mock.ts instead of the gRPC read, so the real validation
logic still runs meaningfully: an unknown account or an unregistered key
fails the exact same way it would on-chain.
Noter authenticates a fresh identity per test rather than reusing two shared
identities across a whole run (researcher's approach) — with `workers: 2` and
~15 login call sites, two fixed identities would have concurrent tests
collide on each other's notes. delegate-account.mock.ts and
fixtures/delegate-key.ts instead generate the same 24-entry deterministic
pool independently (index N -> accountId byte N repeated, privateKey byte
N+0x40 repeated), and the test fixture hands out a never-yet-used entry per
call, interleaved by Playwright's parallelIndex so two worker processes
never claim the same one. public_key is stored base64, not hex: the binding
check's parser tries fromBase64() before falling back to raw hex, and every
64-char hex string (alphabet 0-9a-f, always length-divisible-by-4) also
happens to be valid-but-wrong base64, so a hex value there silently decodes
to the wrong bytes instead of ever matching.
The memory-write specs assert against the real relayer response for a
fixture (unregistered) key, so there's no live-Walrus canary in this suite by
design, same as #680 documents for researcher: the real remember -> recall
round trip against production Walrus Memory stays a manual check.
CI job
------
noter-e2e mirrors chatbot-e2e's shape (Postgres service container, cached
Playwright browsers, report/trace upload on failure). noter-checks adds
tsc --noEmit and a full `next build` so a type or build regression fails CI
even on a change the e2e specs don't happen to cover.
* fix(ci): merge duplicate noter-checks job that broke the QA workflow
Two separate PRs each added a job named noter-checks to the same workflow
file — one from an already-merged auth-hardening change (vitest unit tests),
one from this branch (tsc + next build). Neither touched the same lines, so
the merge went through cleanly with no conflict markers, but the resulting
file had two top-level jobs with the identical key. GitHub Actions rejects
that outright: the workflow run failed in 0s with no job output at all,
before any check even started, which is why this wasn't caught by the local
`tsc`/`next build`/e2e verification — those ran the commands directly, never
through the YAML that CI actually parses. python's yaml.safe_load didn't
catch it locally either; it silently keeps the last duplicate key rather
than erroring, which GitHub's stricter workflow parser does not do.
Folded the tsc/build steps into the existing job instead of renaming to
avoid the collision — one Noter CI job now covers unit tests, type-checking,
and the production build, sharing one checkout/install/SDK-build sequence
rather than paying for it twice.
* fix(noter): stop dapp-kit's sign hook from resolving move calls via deprecated JSON-RPC
useSignTransaction pulls its client from SuiClientProvider, which is still
JSON-RPC (dapp-kit's hook types are hard-wired to it). transaction.toJSON()
needs that client to resolve move-call ABIs before handing the transaction
to the wallet for signing, and fullnode.testnet.sui.io no longer serves
CORS headers on its JSON-RPC endpoint, so the browser blocked every
sponsored transaction with what looked like a CORS failure.
Pre-serialize with our own gRPC client and pass the resulting string
instead: dapp-kit's hook accepts transaction as Transaction | string and
skips its own resolution when given a string.
* test(noter): harden e2e fixtures and unstick CI playwright install
Share one delegate fixture pool between the gRPC mock and Playwright so
they cannot drift, parse hex public keys before base64, and fail-closed
isTestEnvironment in production. CI now sets PLAYWRIGHT=True, times out
browser install at 8m (was hanging the 25m job), and uses an isolated
Playwright cache key.
* chore: drop changesets to avoid a double bump on main
* fix(frontend): WALM-326 scope the dashboard no-key notice to this browser (#719)
* fix(mcp): resolve credentials from parent directories, and stop logout naming a file it did not delete
Review on #701 found two ways the project-scoping half recreated GH #628 in a
different shape.
Resolution checked `process.cwd()` only, while the docs compared it to `.npmrc`
and `.git/config`, which walk up. `cd src`, or an MCP host launched below the
project root, missed the project file and silently used the global account.
It now walks up from the working directory, bounded at the project root
(`.git`), the home directory, or the filesystem root.
The bound is not cosmetic: an unbounded walk climbs into shared parents, and
because the home stop only fires when `homedir()` is a real ancestor, a working
directory outside it reaches the actual `~/.memwal/credentials.json`. That
surfaced as sse-idle-watchdog picking up the developer's own credentials once
HOME was sandboxed.
`clearCreds()` was called for its side effect and the message built afterwards
from `credsPath()`, which by then resolved to the *next* file down the chain.
Logout named a file it had not deleted, and said nothing about the one that
takes over on the next run under a possibly different account. It now reports
what it removed and what survived, and both callers say so. `handleLocalLogout`
had the same bug twice and is fixed here rather than left to break on merge.
Docs updated to describe the walk and its bound, and the two em dashes the
style-guide audit flagged are gone.
Tests: 40 pass (35 before), five new covering subdirectory resolution, the
project-root bound, and each clearCreds outcome.
* fix(sdk): accept a bech32 suiprivkey delegate key, not just hex (#721)
`MemWal.create({ key })` fed the key straight into `hexToBytes`, which
only accepts hex. A Sui `suiprivkey1...` string therefore died at
construction with "hexToBytes: input contains non-hex characters",
before any request went out.
That is what broke the JS SDK e2e job against the dev relayer: all 11
authenticated tests failed while the 7 no-auth ones passed, because the
shared BENCH_DELEGATE_KEY secret is stored in bech32. The Python SDK
reads that same secret and passes, since it normalizes the key first
(`memwal/utils.py` `normalize_private_key`) — and its docstring already
claimed the TypeScript SDK took both forms. It did not. This closes that
gap rather than working around it in CI, because the two forms are both
in circulation: `sui keytool` and wallets hand out bech32, while
`generateDelegateKey()` returns hex.
Port `normalize_private_key` to `utils.ts` and call it everywhere a
user-supplied delegate key is parsed: both client constructors and the
two `delegateKeyTo*` helpers. `account.ts` is untouched — its
`hexToBytes` calls take public keys, where suiprivkey does not apply.
The bech32 decoder is hand-rolled rather than imported from
`@mysten/sui`, for the reason `u64ToLeHex` gives: the core path keeps
`@mysten/*` as peer dependencies so the client works without them, and
the constructor is synchronous so it cannot await a dynamic import. The
Python SDK hand-rolls it for the same reason. Verified against
`@mysten/sui` on 2000 random keys.
Co-authored-by: Le Tien Phat <91601109+Niko1444@users.noreply.github.com>
* fix(server): make storage quota admission atomic with insertion (#720)
WALM-359 / GH #532.
check_storage_quota read SUM(blob_size_bytes) inside a transaction holding
pg_advisory_xact_lock, then committed. Advisory xact locks are transaction
scoped, so the lock died on that commit; the quota comparison and the
caller's INSERT both ran unprotected. Concurrent requests from one owner
all observed the same pre-insert total, all passed, and all inserted.
Reproduced against real Postgres before the fix: 17 of 20 admitted on the
inline path (+16 MiB over a 1 GiB quota), and 20 of 20 with a 150 ms gap
standing in for the Walrus upload (+19 MiB).
Replace the check with a reservation recorded inside the locked
transaction, so admission commits the intent to write before releasing the
lock. Usage is now SUM(vector_entries.blob_size_bytes) plus live
reservations, which makes a later request in the same burst see an earlier
one's bytes even though no row exists yet.
One transaction spanning check and insert would close the inline paths but
not the enqueued ones: those admit, queue a wallet job, upload to Walrus,
and insert minutes later, and a Postgres transaction cannot be held across
that. A reservation table covers both groups on one mechanism.
Reservations are keyed by the caller's own id rather than a generated one:
enqueued paths use the remember_jobs.id the vector row will carry, so
jobs.rs releases from an id it already has and nothing new is threaded
through the serialized WalletOperation payloads; inline paths mint a local
UUID. Release happens after the row is committed, never before, so the
overlap errs toward over-counting rather than leaving a window where
neither the row nor the reservation counts.
Three layers keep a missed release from stranding quota: explicit release
on the success and terminal-failure paths, a periodic reconcile against
remember_jobs already in a terminal state, and a 15 minute TTL as the
backstop. The failure mode is bounded over-counting, never a permanently
unusable account.
Also removes the TOCTOU comment that claimed the old advisory lock closed
this race.
Covers all five call sites: remember(), remember_bulk, remember_manual,
and both the benchmark and main analyze paths. The 402 / "Storage quota
exceeded" response shape is unchanged.
Co-authored-by: Harry Phan <phanhoangvinhhien@gmail.com>
* fix: point empty 401s at memwal_login (#702)
* fix: point empty 401s at memwal_login (#696)
Unauthenticated 401s were surfacing as "Walrus Memory server error (401): <no message>". Return a short, actionable hint to run memwal_login instead.
* fix(sdk): use actionable 401 copy pointing at memwal_login
Walrus Memory isn't signed in. Call the memwal_login tool, then retry.
* fix(sdk): changelog, 0.1.4 bump, and empty-body-only 401 login hint
Manual changelog + dump/version for #696. Empty 401s keep the locked memwal_login copy; non-empty 401s keep AUTH_REJECTED triage.
* chore: drop extra changeset and ticket IDs from source
Manual changelog + version dump is enough. Comments describe behavior only.
* chore(sdk): restore package.json description em dash
* WALM-295/296/297: owner-scoped memory read API + bearer token auth for Console (#554)
* Add design spec for WALM-295/296 memory read API
Owner-scoped read API (namespaces/memories/agents) plus per-memory
expiry fields, covering both tickets since WALM-296 extends the same
memories response WALM-295 creates.
* Add updated_at/agent_id/package_id columns and pagination index to vector_entries
* Split vector_entries schema migration into per-lock-scope transactions
* Add AppError::Forbidden (403) for owner-mismatch responses
* Plumb agent_id/package_id into insert_vector across all write paths
* Normalize empty agent_id string to NULL in restore's provenance lookup
* Carry agent_id/package_id through the FinalizeUploadedBlob recovery path
* Add GET /v1/owners/{owner}/namespaces
* Add GET /v1/owners/{owner}/memories with keyset pagination
* Fix limit query error envelope and add real tie-breaking test for memories pagination
* Add GET /v1/owners/{owner}/agents (live on-chain delegate key list)
* Document the memory read API contract for Console
* Fix final review findings: updated_at conflict bump, URL-safe cursors, namespaces pagination, agents caching, route metrics, auth docs
* Fix flaky CI test: serialize concurrent CREATE EXTENSION IF NOT EXISTS vector races in memory_read tests
* Add end_epoch/expires_at/expiry_synced_at columns to vector_entries
* Add WALRUS_STAKING_POOL_ID config for expiry timestamp conversion
* Add Walrus staking-state fetch and epoch-to-timestamp conversion
* Fix off-by-one-epoch error in expires_at_from_epoch
* Surface end_epoch in the sidecar's walrus upload response
* Add end_epoch to WalrusUploadResponse and UploadResult
* Plumb end_epoch into insert_vector across all write paths
* Change end_epoch from i64 to i32 to match on-chain u32 and INTEGER column
* Expose end_epoch/expires_at and derive status on GET /v1/owners/{owner}/memories
* Add periodic background sweep to populate per-memory expiry
* Harden expiry sweep: index expiry_synced_at, avoid burst catch-up, add regression tests
* Document end_epoch/expires_at on the memories endpoint
* Anchor expiry formula on current epoch, fix sweep failure ordering, fix synced_at index
* Fix overflow panic in expires_at_from_epoch's epoch-delta subtraction
* Correct comment on why the epoch-delta multiplication uses saturating_mul
* Remove internal design-spec doc from the repo
* Remove internal design-spec doc from the repo
* feat(WALM-297): owner-scoped bearer token auth for the read API (Phase 1)
Console can never call WALM-295's owner-scoped read endpoints under the
existing Ed25519 signed-request scheme (auth.rs::verify_signature) because
it structurally never holds a delegate key — WALM-298's identity-link flow
proves control of an owner address entirely on Console's own side and never
grants signing capability. This adds the missing bridge: after Console has
proven control of owner Y itself, it authenticates as a trusted client via
a single shared service credential (team decision: service credential over
mTLS/signed-client-assertion, for lower implementation cost on both sides)
and mints a short-lived, owner-scoped bearer token limited to memories.read.
- owner_token_auth.rs: HMAC-signed opaque token mint/verify, mirroring the
existing security_delete_auth.rs token scheme; a FromRequestParts
extractor for token-gated routes.
- routes/owner_token.rs: POST /v1/owner-tokens (issuance, gated by a
constant-time service-credential check, Sui-address validation, and a
MemWalAccount existence check) plus GET /v1/owners/{owner}/_token_probe,
a minimal token-gated example route standing in for WALM-295's real
handlers, which don't exist on this branch yet (fresh branch off dev).
Wiring the OwnerToken extractor into the real namespaces/memories/agents
handlers is the small follow-up once this branch merges with WALM-295.
- rate_limit.rs: independent per-credential and per-owner issuance budgets,
fail-closed on Redis error.
- types.rs / routes/remember.rs: new Config fields + config-fixture updates.
- docs/api/owner-token-auth.md: full contract for Console, including the
four distinct 429/503 response shapes across the two independent rate
limiters and two independent unavailability paths, and an explicit note
on the shared-secret trust boundary (a leaked credential can mint a
memories.read token for any owner — accepted Phase-1 trade-off).
Verified end-to-end against a live local build (real HTTP, not just unit
tests): credential rejection, cross-owner 403, real expiry, forged tokens
with altered permissions/audience correctly rejected (proving the scope
check isn't hardcoded), rate-limit trip, and no secret leakage across
logs/responses. 321/321 lib tests, 432/432 bin tests, no regressions.
* Migrate /agents delegate-key listing to gRPC, matching auth's existing JSON-RPC-sunset migration
* Migrate /agents delegate-key listing to gRPC, matching auth's existing JSON-RPC-sunset migration
* fix(WALM-297): close review findings — IP rate limit, test coverage, doc drift
An adversarial multi-dimension review of commit 9c16e461 confirmed 2
major and 5 minor findings; the 2 major ones are fixed here, along with
3 of the cheap minors.
1. (major) POST /v1/owner-tokens had no throttling on guessing the
shared service credential: the credential-gate middleware rejected
bad guesses in-process before the per-credential rate limiter ever
ran, and that limiter is keyed by the guessed value anyway, so a
varying guess got a fresh Redis bucket every time. Added
owner_token_ip_rate_limit_middleware (mirrors
accounts_rate_limit_middleware/sponsor_rate_limit_middleware's
unconditional per-IP layer) as the true outermost layer on this
route, independent of credential validity.
2. (major) token_probe's actual authorization logic — owner-match via
same_owner and the memories.read permission-scope check, explicitly
documented as the copy-paste template WALM-295's real read handlers
will use — had zero test coverage despite needing no AppState/DB/
Redis to test. Added 5 unit tests covering the match/mismatch and
present/missing-scope/empty-scope cases.
3. (minor) docs/api/owner-token-auth.md's "validation order" bullet had
the service-credential check listed after Sui-address-format, when
in the real request pipeline the credential gate is the outermost
middleware and runs first. Corrected, and documented the new IP
rate-limit layer.
4. (minor) expires_at used chrono's plain to_rfc3339(), which always
renders the UTC offset as "+00:00" — never matching the "Z"-suffixed
example the response's own doc comment and the API doc promised.
Switched to to_rfc3339_opts(SecondsFormat::Secs, true). Also
replaced the silent "fall back to now()" on timestamp-overflow with
a real error, since silently reporting a valid token as already
expired is worse than failing loudly.
5. (minor) OWNER_TOKEN_TTL_SECS had no upper bound (only rejected 0),
which both defeats the "short-lived" security property the token
scheme's threat model rests on and could, for extreme values, push
the expires_at computation outside chrono's representable range.
Clamped to a new MAX_OWNER_TOKEN_TTL_SECS ceiling (24h).
Not fixed here (left as follow-ups, all minor/test-coverage-only,
noted in review): OWNER_TOKEN_AUDIENCE not deployment-scoped (relies on
OWNER_TOKEN_SECRET differing per environment, an operational
convention); no test for duplicate-nonce-still-verifies (intentional
per the nonce design note, but undertested); no test pinning the
per-owner-vs-per-credential rate-limiter independence; no test pinning
find_account_by_owner's DB-error path surfacing as 500 rather than
being swallowed.
Re-verified after all fixes: cargo check --lib / --bin clean, 321/321
lib tests + 437/437 bin tests pass (up from 432 — 5 new tests), no
regressions.
* Fix rolling-deploy migration race: set updated_at's default as its own statement in 010, before 011's backfill
* Add explicit has_more to namespaces/memories responses; fix doc overclaim on deletion visibility
* Fix e2e_test.py size-test assertions to check job_id, not id
/api/remember returns {"job_id": ..., "status": ...}. The 64KB and
large-size test functions asserted "id" in result, so they always
reported failure even when the underlying job completed successfully.
* fix(WALM-295): batch migration 011, split read-API rate budget, fix pagination/authz findings
Standalone commit of the review-fix round applied on top of c09bdd05,
independent of any other in-flight work on this PR — see commit body for
full detail on each of the 9 findings this addresses.
1. (critical) Migration 011 was a single unbatched full-table UPDATE —
crash-loop risk on real-sized tables (live-confirmed against the dev
DB, 113k rows, statement timeout -> rollback -> panic -> Railway
restart -> identical doomed retry, forever). Moved the backfill into
a Rust loop (db.rs, batches of 5000 rows, each its own committing
statement) invoked from VectorDb::new() between migrations 010 and
012; neutered 011's SQL body to a no-op, kept the file for numbering.
2. (major) Migration 012's SET NOT NULL took a full-scan ACCESS
EXCLUSIVE lock. Split into 012 (ADD CONSTRAINT ... NOT VALID +
VALIDATE CONSTRAINT, SHARE UPDATE EXCLUSIVE only) and new migration
014 (fast SET NOT NULL once the CHECK is validated, then drops it).
3. (major) Migration 013's CREATE INDEX CONCURRENTLY IF NOT EXISTS could
leave a permanent INVALID index after an interrupted build, silently
degrading every memories-listing query to a sequential scan forever.
Added a guard in db.rs that detects an invalid index via
pg_index.indisvalid and drops+rebuilds it before 013 runs.
4. (critical) The three new /v1/owners/{owner}/* read routes shared the
write-path's 30/min delegate-key rate budget with no dedicated
allowance -- live-reproduced this trips 429 under completely normal
read-pagination traffic. Gave the read API its own dedicated budget
(rate_limit.rs, main.rs router split) separate from the write path.
5. (major) storage/sui.rs's DelegateKeysCache never evicted entries --
unbounded process-lifetime growth. Added a periodic sweep task
(main.rs) mirroring the existing evict_expired_delegate_keys pattern.
6. (major) Namespace keyset pagination's (MAX(updated_at), namespace)
cursor could deliver an already-returned namespace a second time if
it was mutated mid-walk (the same failure mode the cursor was
designed to fix, in the opposite direction). Added a snapshot_at
boundary (memory_read.rs), mirroring the same fix already applied to
the memories cursor.
7. (minor) Owner-path 403 check was a raw case-sensitive string compare.
Switched to the existing same_owner canonical-address helper.
8. (minor) Memories keyset pagination could deliver the same memory_id
twice within one walk if a row's updated_at was bumped mid-walk (an
Apalis job retry re-upserting the same vector_id). Added a
snapshot_at boundary so rows mutated after a walk began are excluded
until the next fresh walk.
9. (major, docs) docs/api/memory-read-api.md described the old shared
rate-limit scheme and a "layer": "per-account-burst" value that was
never real. Rewrote the Rate Limiting section to describe the new
dedicated budget with real layer names/values.
Every fix was verified against a live local build: real Postgres
migration pipeline runs (12k+ seeded NULL rows crossing the batch
boundary, a genuinely-reproduced interrupted CONCURRENTLY build for the
invalid-index case), real HTTP rate-limit trips, and the full test
suite (cargo test --lib and --bin, no regressions) -- not just static
review.
Also includes an OpenAPI 3.0.3 spec for the three read endpoints
(services/server/docs/api/memory-read-api.openapi.yaml), written to
unblock a Console engineer who was blocked on it.
* fix(WALM-297): wire OwnerToken into the real WALM-295 read routes
Adds auth::verify_read_api_auth, a combined dispatcher for read_api_routes
that accepts either WALM-297's owner-scoped bearer token (Console) or the
existing Ed25519 signed-request scheme (SDK/dashboard), additive per
docs/api/owner-token-auth.md's stated trust model. Swaps it in for
auth::verify_signature on the three real routes; handlers are unchanged
since they already enforce owner-matching via the shared AuthInfo.
Fixes a rate-limit isolation bug the naive version would have had: the
bearer path's synthetic AuthInfo.public_key is prefixed per-owner
(ownertoken:{owner}) so read_api_rate_limit_middleware's per-key bucket
can't collapse every Console-proxied owner into one shared budget.
Adds unit tests for the new dispatch/scope-check helpers and opt-in
e2e_test.py coverage for the bearer path (success, owner mismatch,
invalid token, Ed25519 regression guard). Updates memory-read-api.md and
owner-token-auth.md, which both had drifted to actively say this wiring
didn't exist yet.
* fix(WALM-295): correct memory-read-api.openapi.yaml against live behavior
Addresses PR #554 review findings from Niko (Console team), verified
against dev:
- NamespacesResponse/MemoriesResponse: has_more was missing from both
properties and required, despite the live API always returning it and
memory-read-api.md documenting it as the only correct end-of-data
signal.
- next_cursor's description claimed null means "end of data" — backwards.
It's always returned on a non-empty page (including the final page of a
traversal); null only appears on an empty page. Confirmed live: draining
48 memories, the last page had a non-null cursor and has_more: false.
- MemoryItem was missing end_epoch/expires_at entirely — all of WALM-296,
which this branch ships. Documented that they're nullable independently
of each other (observed end_epoch set with expires_at: null on 4 of 5
rows in one batch on dev), not as a pair.
- status enum only listed "active" with "always active in Phase 1" — real
accounts on dev return "expired" too, per the derivation
memory-read-api.md already documents. A client generated from the old
spec would fail closed on its first real page with an expired row.
- Also added updated_at to both NamespaceSummary and MemoryItem, missing
from the same schemas for the same reason (spec drifted from the real
response shape) — not flagged in review but caught in the same pass.
* fix(WALM-295/296/297): close 4 correctness findings from PR #554 review
Addresses PR #554 review findings (Henry, GitHub review
pullrequestreview-4881462755), all verified against real behavior, not
just plausible-sounding:
1. Terminal next_cursor kept the originating snapshot_at, so an
incremental-sync client's next poll (using that terminal cursor, never
None) stayed frozen at the first walk's snapshot forever — any row
updated after that point could never appear again. Fixed by resetting
snapshot_at to None specifically on the terminal cursor (has_more:
false); continuation cursors mid-walk keep it as before. Applies to
both query_owner_namespaces and query_owner_memories. Extended both
existing "surfaces on next walk" tests to actually poll with the
terminal cursor (the realistic client pattern) instead of only
asserting the from-scratch-resync case, which the old buggy code also
satisfied trivially.
2. verify_signature stored the caller-supplied x-public-key header string
verbatim in AuthInfo.public_key. hex::decode accepts mixed-case input,
so the same delegate key could vary casing across requests to obtain
independent identities for account-resolution caching and — the actual
security-relevant one — read_api_rate_limit_middleware's per-key Redis
bucket, trivially defeating that abuse-prevention control. Fixed by
re-encoding from the decoded bytes (hex::encode(pk_array)) immediately
after decode, so every downstream use sees one canonical form.
3. set_memory_expiry never touched updated_at at all (by design, to stop
the ~24h re-verification sweep from making every row reappear in
incremental sync) — but that same guard also suppressed the one write
that must be cursor-visible: a row's first resolution from unknown to
known expiry. A client that had already synced that row before the
sweep ran would never see the populated end_epoch/expires_at on any
later poll, silently breaking WALM-296 for exactly the rows synced
before their expiry was known. Fixed with a conditional bump
(IS DISTINCT FROM, null-safe) — advances updated_at only when the
value actually changes, satisfying both constraints. Replaced the test
that encoded the old (wrong) assumption with one pinning all three
cases: first write bumps, unchanged re-verification doesn't, a later
real change bumps again.
4. WalrusUploadErrorResponse didn't declare end_epoch even though the
sidecar includes it on the metadata-transfer failure response too (not
just the success shape) — so it was silently dropped, and recovery via
SetMetadataAndTransfer -> FinalizeUploadedBlob finalized with
end_epoch = None regardless of what was actually known at upload time.
Threaded end_epoch through the full chain: WalrusUploadErrorResponse ->
UploadBlobError::MetadataTransferFailed -> WalletOperation's two
recovery variants -> the final insert_vector call, mirroring the exact
pattern agent_id/package_id already use for the same recovery path.
cargo check --lib/--bin: clean. Full suite green against a live database:
354 lib + 494 bin tests (0 failures), including new/updated regression
tests for all four findings. Also added the one doc clarification finding
3's fix newly guarantees (memory-read-api.md: expiry sync visibility).
* fix(WALM-295): reset snapshot_at on empty continuation pages too
PR #554 review (Henry, discussion_r3734942009): the terminal-cursor
snapshot reset from the previous fix only fired when the terminal page
still had rows. A continuation page (previous page had has_more: true)
can come back with zero rows if every remaining row races past
snapshot_at between pages -- rows.last() is None either way, so
next_cursor collapsed to plain None too. Per this API's contract, an
empty page's None cursor means "keep the cursor you already have" -- but
the client's held cursor is the previous page's continuation cursor,
which still carries the walk's now-stale snapshot_at. That freezes
incremental sync the same way the terminal-page bug did, just one page
later.
Fixed by re-encoding the incoming cursor's own watermark position (no row
was consumed, so it hasn't moved) with snapshot_at reset whenever a page
comes back empty but there was a real incoming cursor. has_more is always
false when rows is empty, so the reset is unconditional there; a
brand-new walk that finds nothing at all (no incoming cursor) still
correctly returns no cursor. Applies to both query_owner_namespaces and
query_owner_memories.
Added two new regression tests reproducing the exact race Henry
described (a peeked row updated past snapshot_at before the next page is
requested), and updated five pre-existing tests that asserted the old
"empty page always means None" contract -- each was reached via a real
prior cursor, so they now assert a reset cursor instead.
cargo check: clean. Full suite green against a live database: 353 lib +
497 bin tests (0 failures).
Also merges origin/dev (one real conflict in storage/walrus.rs -- two
independent test additions at the same insertion point, both kept).
* docs(WALM-295/296/297): clean up internal ticket refs, consolidate migration 011
Henry (internal review): shipped docs and code comments named internal
Linear ticket IDs (WALM-295/296/297/298 and unrelated 317/318/319),
GitHub PR numbers, and reviewer-thread citations
(discussion_r3734942009-style) that read as obscure to devs without
tracker/PR access. Stripped all of it from the two public API docs, the
OpenAPI spec, .env.example, and every Rust/Python file this PR touches,
rewording each comment to state the underlying technical rationale
plainly instead of pointing at a ticket for it. Also dropped a dangling
reference to an internal design-spec doc that had already been
deliberately removed from the repo in an earlier commit.
migrations/011_memory_read_api_backfill_updated_at.sql was a permanent
no-op: the backfill it originally contained (a single unbatched UPDATE)
turned out to crash-loop on a real-sized table under Railway's restart
policy, so the real implementation moved into Rust as
backfill_updated_at() in db.rs, leaving 011 doing nothing but preserving
a file number. Deleted it, folded its explanation into
backfill_updated_at()'s doc comment and migrations 010/012's headers,
and updated VectorDb::new()'s migration wiring (and the test helper)
accordingly. Left the resulting numbering gap at 011 rather than
renumbering 012-016 -- documented why in the new migrations/README.md,
which also covers the backfill's operational behavior (VectorDb::new()
blocks server startup on it, so the first boot after this ships has a
startup delay proportional to table size) -- the piece Henry flagged as
completely undocumented.
While in migrations/012's and db.rs's headers: fixed two unrelated,
pre-existing stale references to "migration 014" for the SET NOT NULL
step, which is actually migration 016 (014 is the unrelated
per-memory-expiry-columns migration).
cargo check --lib/--bin: clean. cargo test --lib: 353 passed, 0 failed
(comment/doc-only changes in this commit; no behavior touched).
* fix(WALM-296): stop expiry re-verification from spamming updated_at daily
set_memory_expiry's anti-spam guard compared both end_epoch and
expires_at via IS DISTINCT FROM, bumping updated_at only when either
changed. But expires_at is recomputed from a fresh now() on every
periodic expiry-sweep tick (main.rs's expires_at_from_epoch call), so it
differs from the previously stored value on essentially every routine
re-verification even when end_epoch hasn't moved at all -- the guard was
true almost every time, defeating the exact spam it exists to prevent.
Every synced memory would reappear in Console's updated_after incremental
sync roughly once a day regardless of whether anything real changed,
degrading updated_after into a daily full resync -- precisely the
outcome memory-read-api.md promises does not happen.
Fixed by comparing end_epoch only; expires_at is still refreshed on
every write (it's deliberately approximate, so keeping it current is
harmless), it just no longer drives whether updated_at bumps.
The existing regression test passed against the buggy code because it
re-verified with the exact same expires_at literal, which a real caller
never does. Strengthened it to use a different expires_at (same
end_epoch) on the "unchanged" case, matching real caller behavior.
Live-verified against a throwaway pgvector/pgvector:pg17 container: the
strengthened test fails on the pre-fix code and passes on the fix.
cargo test --lib against that same container: 353 passed, 0 failed,
0 skipped (every DB-integration test that's normally silently skipped
without DATABASE_URL ran for real).
Also fixes two next_cursor doc descriptions (memory-read-api.md and the
OpenAPI spec, both NamespacesResponse and MemoriesResponse) that had
gone stale after an earlier fix (7f0206c0) changed empty-continuation
pages to return a reset, non-null cursor instead of null. Both now state
the real current rule: null only on the first page of a fresh walk that
matches nothing; every other empty page returns a non-null, freshly
reset cursor superseding whatever the client already held.
* fix(migration): delete the 011 no-op placeholder
The `SELECT 1;` file was never committed — it was left as an untracked
artifact from the pre-commit-guarded attempt. README's reference to it
was inaccurate: "no such file" vs "deleted". Correct the README to
match reality. Migration numbering is unaffected since 011 was never
part of the committed tree.
* Fix memory read test migration list
* fix(read-api): deletions, metadata backfill, docs CI, token scope 400
Stacked on feat/console-integration-test (PR #554).
- WALM-363: namespace_watermarks + memory_tombstones so incremental
/namespaces and /memories see hard deletes
- WALM-364: metadata_synced_at + refill script backfill-read-api-metadata.sh
- Expose importance on MemoryItem; bump snapshot_version to 3
- POST /v1/owner-tokens returns 400 on unknown scope/permissions
- Docs: colon routes for freshness, no H1/em dashes, deletion SLA
* fix(read-api): atomic deletes, safe 150k backfill, OpenAPI, tests
- Wrap tombstone + watermark + DELETE in one transaction
- Clear tombstones on live insert; UNION ALL skips ids that are live again
- Backfill pages unsynced blob_ids, stamps only that page, never the rest of an owner
- Test drives VectorDb::delete_by_blob_id
- OpenAPI: importance, status=deleted, snapshot_version 3
- Docs: colon lists instead of semicolon-for-emdash
* docs(read-api): restore readable punctuation after em-dash strip
* fix(read-api): one sidecar listing per owner, batched SQL for 150k
* fix(read-api): WALM-363 deleted[] contract, not status-in-memories
Match the Linear-approved design (2026-08-18):
- Separate memory_tombstones (memory_id PK); one CTE DELETE RETURNING INSERT
- Additive deleted[] + must_resync; memories[] stays live-only
- snapshot_version stays 2 so COMG-568 ignores the new keys
- Namespace watermark = GREATEST(live MAX(updated_at), tombstone MAX(deleted_at))
- Dual cursor for the tombstone stream; v1 cursor replays 30 days
- 30-day retention sweep; security-delete finalization writes tombstones
* fix(read-api): must_resync only on stale tombstone cursor, not quiet live rows
* fix(read-api): address Harry review on #718
- Identify security-delete purges by memory id (tracking has no namespace;
Walrus erase still drops every row pointing at the blob)
- Do not pin a tombstone-only first page to snapshot_at (in-flight insert race)
- Cursor minted_at so v1 / stale cursors set must_resync
- Single TOMBSTONE_RETENTION for sweep and read path
- insert_plaintext clears tombstones in the same transaction
- GREATEST without redundant COALESCE; docs style (will / e.g. / passive)
* fix(read-api): expiry never reaching synced clients, spurious resync, wrong status contract
Three defects found reviewing the merged branch. Each ships with a test.
1. expires_at was invisible to a client that had already synced the row.
insert_vector writes end_epoch but not expires_at, so when the expiry
sweep first resolves a row on the mainline write path, end_epoch is
already equal and the `WHEN end_epoch IS DISTINCT FROM $1` guard is
false. expires_at went from NULL to a real value with no updated_at
bump, so the row never re-entered incremental sync and Console kept
expires_at: null for it indefinitely. That is WALM-296 silently not
working, on every new memory, for the up-to-5-minute window before the
sweep reaches it. The existing test only covered the legacy path where
end_epoch was NULL at insert, which is why it passed. Guard now also
fires while expires_at IS NULL, which keeps the anti-spam property the
comment argues for: a routine re-verification of a populated row still
does not bump.
2. must_resync fired on healthy clients.
stale_tombstone compared the cursor's deleted_at against the retention
cut, but minted_at is re-stamped on every cursor we hand out while
deleted_at freezes at the last tombstone the client saw. A client
polling every 5 minutes was therefore told to do a full resync 30 days
after its last observed deletion, recurring, which on a 150k-row owner
is roughly 300 requests. The check was also redundant: the tombstone
query already floors on deleted_at >= retention_cut, so a cursor below
the floor is subsumed and nothing is skipped or duplicated. must_resync
now keys off minted_at alone. Tests pin both directions.
3. The docs described a status value the code cannot emit.
memory-read-api.md said status is "deleted" for tombstoned rows. The
match on expires_at only ever produces "expired" or "active", and
DeletedMemory carries no status field. A Console client following that
line would branch on an unreachable value, never evict deleted rows, and
leave them visible, which is the exact problem WALM-363 exists to fix.
The OpenAPI enum was already correct. Replaced with a pointer to
deleted[] as the only deletion signal on this endpoint.
Also cleared the last 3 style-guide violations in the two API docs
(one Latin abbreviation, two future-tense verbs).
---------
Co-authored-by: Harry Phan <phanhoangvinhhien@gmail.com>
Co-authored-by: ducnmm <165614309+ducnmm@users.noreply.github.com>
Co-authored-by: Harry Phan <lekhacthanhtung.it@gmail.com>
* chore(github): add bug and feature issue forms
Disable blank issues and require surface, environment, and a repro so
placeholder and duplicate-looking reports are harder to file. Point
security reports at private advisories instead of the public tracker.
* fix(oauth): persist the client loopback redirect URI (GH #619)
Authorize matched RFC 8252 loopback URIs port-agnostically, then stored
the registered port-less URI. The auth code went to port 80 and token
exchange rejected the real ephemeral redirect_uri. Store the candidate.
* fix(sponsor): restore pending binding after sidecar execute failure (GH #617)
consume_pending_sponsor deleted the Redis record before the sidecar call,
so a transport/5xx/429 failure burned the sponsored tx. Restore the
binding on those failures so the client can retry. Leave 4xx consumed.
* fix(relayer): restore timeouts, oauth authorize cap, analyze idempotency
Restore maps sidecar query failures to 503, bounds embed fan-out, and
fails at 55s. Authorize is per-IP rate limited and abandoned pending
delegates are pruned. Analyze retries reuse per-fact jobs. SUI gas
selection errors classify as pool budget failures.
* fix(mcp): require a registered delegate before opening a session
Unregistered 64-hex bearers and missing Authorization no longer open
MCP sessions. On-chain lookup failures return 503 instead of 401.
Occupancy caps no longer advertise a 30s cooldown. The stdio bridge
drains handshake error bodies instead of aborting the socket.
* fix(mcp): fail non-TTY login and single-flight memwal_login
Explicit `login` with no TTY now exits 1 instead of booting the
auth-required stub. Concurrent memwal_login calls reuse one listener
and URL so a second flow cannot hang later remember/recall.
* fix(mcp): serialize SSE POSTs so concurrent recall cannot hang the bridge
Overlapping tools/call POSTs dropped the SSE session and left later
calls hung until restart. Outbound POSTs are serialized; stale queued
posts are skipped after reconnect. Login timeout warns that an on-chain
key may already exist.
* fix(analyze): forget releases idempotency keys and importance updates live rows
Forget now NULLs remember_jobs.idempotency_key in the same transaction as
the vector delete, so re-analyzing forgotten text is a new write instead
of a silent no-op. Same-text extracts with a different importance bucket
patch the live row rather than minting a second blob.
* fix(bench): retry Cloudflare HTML in the live latency check (#756)
The Memory API Latency job on the promote PR failed because 5/10 warm
recalls got a Cloudflare error page, not because /api/recall itself
failed. Retry HTML/5xx/429 flaps; leave auth and contract 4xx fatal.
Also apply the style-guide nits from that PR: onchain, through, active
voice, and an unstacked 0.1.4 changelog heading.
---------
Co-authored-by: Harry Phan <phanhoangvinhhien@gmail.com>
Co-authored-by: Le Tien Phat <91601109+Niko1444@users.noreply.github.com>
Co-authored-by: Hoang Duc Bach <124645604+HoangDucBach@users.noreply.github.com>
Co-authored-by: harrymove-ctrl <harry.phan@commandoss.com>
Co-authored-by: Nikola Le <91601109+nikola0x0@users.noreply.github.com>
Co-authored-by: Harry Phan <lekhacthanhtung.it@gmail.com>
nikola0x0
changed the base branch from
fix/researcher-title-model-chat-crash
to
dev
August 24, 2026 06:38
# Conflicts: # .github/workflows/test.yml
ducnmm
approved these changes
Aug 25, 2026
ducnmm
left a comment
ducnmm
left a comment
Collaborator
There was a problem hiding this comment.
Summary
This is the right test to add. Researcher had no automated coverage, and chat-stream.test.ts actually pins the title-model P0: a failing title generation must not put an error frame on the SSE stream while the answer still streams. Mock seams are gated on isTestEnvironment (PLAYWRIGHT), the CI job passed (Researcher / Unit + Playwright E2E 2m10s), and the two-account visibility specs correctly pass storage state explicitly instead of inheriting user A.
Two non-blocking notes below. Happy to merge as-is.
Issue counts
- bugs: 0
- suggestions: 2
… account Addresses review feedback on #680. Locally Playwright reuses whatever `pnpm dev` is already on the port. Started without PLAYWRIGHT that process runs none of the mock seams, so model calls reach OpenRouter, the delegate-account binding check Sui, and sprint saves the real Walrus relayer. `/ping` now reports the seam as `x-researcher-test-mode` and global setup aborts the run with the fix spelled out when it isn't set. Reuse stays on so `PLAYWRIGHT=True pnpm dev` keeps the fast `--ui` loop. The check also runs in CI, where it asserts the webServer env reached Next. The test MemWal client is now keyed on accountId rather than one process-wide mock, mirroring the real client's per-(key, accountId) construction, so a later remember-as-A / recall-as-B spec cannot pass for the wrong reason.
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
Closes the "e2e test researcher" half of the task: a 15-test Playwright suite plus a
researcher-e2eCI job, mirroring the existingchatbot-e2esetup.Researcher had no automated tests at all — the app with the only real Walrus writes was verified entirely by hand. That's how the title-model P0 in #673 reached production.
The test that would have caught the P0
e2e/chat-stream.test.tsasserts the raw SSE frames from/api/chat: a failing title model must not put anerrorframe on the stream.Verified as a real regression test, not merely a passing one — with the
try/catchguard removed it fails with the exact production symptom, while the answer still streams underneath:Mock seams
Four seams, all keyed off the existing
isTestEnvironment, so a run never reaches OpenRouter, Sui, or the relayer:lib/ai/providers.tslanguageModel("google/gemini-2.5-flash")would have thrownNoSuchModelErroron the first message. The branch had simply never run.lib/ai/models.mock.tsFAIL_TITLE_GENERATIONsentinel in a user message makes the title model reject, reproducing the P0 on demand.lib/auth/delegate-account.mock.ts(new)MemWalAccountobject for two fixture identities. The real binding validation still runs, so unregistered keys and unknown accounts fail exactly as they do on-chain — both asserted.lib/sprint/memwal.tsMemWalMockfrom the SDK, one instance per process, so remember → recall round-trips in memory and CI can never write to the production relayer.Coverage
auth.setup.tse2e/auth.test.tse2e/chat.test.tsTypeErrorfrom39e102ae.e2e/chat-stream.test.tse2e/visibility.test.tsTwo behaviors pinned as they are, not as expected
Both found while writing the specs; neither leaks content:
app/(chat)/chat/[id]/page.tsxwraps the async component in<Suspense>, so the streaming shell is flushed beforenotFound()runs. The user correctly sees the not-found page; only the status code is misleading (matters for monitoring/bots). The spec asserts the rendered page and the absence of the canary./logineven for public chats, sinceproxy.ts's auth check precedes the visibility check — the known "Public — Anyone with the link" product gap. Pinned deliberately, with a comment to update it if that product decision changes.Flakiness worked out up front
global-setup.tsclears the auth limiter's Redis keys before each run. The limiter allows 10 verify attempts per IP per minute and a run spends five, so without this a second run inside a minute fails at sign-in with a 429 that looks nothing like the real problem. Both identities also sign in once in a setup project rather than per-test, keeping CI's 2 retries clear of the limit.Also worth knowing for anyone adding specs (documented in
tests/README.md):browser.newContext()inherits the project'suse.storageState, so a context that must be someone else — including an anonymous one — has to pass its own explicitly. Miss it and the test still passes, for the wrong reason. My first draft of the visibility specs hit exactly this.Verification
AUTH_SECRET/NEXT_PUBLIC_MEMWAL_PACKAGE_IDunset — so the config defaults CI relies on are exercisedtsc --noEmitclean;pnpm test:unit14/14Notes
ultracite/biomeversion mismatch already breakspnpm lintrepo-wide (soft-failed for chatbot in the same workflow). Same follow-up.