Bring the sample response back on reopen, kept only for the life of the tab - #573
Open
gabrieljablonski wants to merge 19 commits into
Open
Bring the sample response back on reopen, kept only for the life of the tab#573gabrieljablonski wants to merge 19 commits into
gabrieljablonski wants to merge 19 commits into
Conversation
gabrieljablonski
force-pushed
the
feat/566-sample-persist
branch
from
September 7, 2026 17:40
84fc529 to
efc463d
Compare
…the browser that captured it
The Sample response field in the HTTP tool editor was local state, cleared on every open. So an
operator coming back to adjust a response template — the most common reason to reopen an HTTP tool —
faced pickers that offered nothing, and two ways out: paste a response again by hand, or press Send
a test request, spending a real call against the customer's API to recover what we had on screen
once. Measured on the base: reopening also removes the preview entirely and does not render the
"Insert a field" button at all, so the template on screen cannot be checked from that dialog.
The sample is a real response and routinely carries names, e-mail addresses, phone numbers, CPF,
prices and internal ids. Storing it would put a third party's personal data in a column nobody
thinks of as data storage, and from there into backups.
The screen has two consumers of it and they need different things — the pickers and the caret
completion need PATHS, and only the preview needs VALUES — so the answer follows that line:
- the SHAPE goes in the row (`tool_definitions.sample_shape`: `{status, body}` with every value
replaced by a stand-in of its type). Pickers, completion and preview all keep working, on any
machine, for any operator on the tenant;
- the response itself stays in `localStorage`, keyed by tool id, on the machine that captured it.
"We never store it" is a stronger invariant than a per-tool opt-in: no backup question, no bundle
question, no export rule to keep in step. The redaction runs on the client so the response never
travels, and AGAIN in the service so the invariant belongs to the column rather than to whichever
client wrote the row.
A stand-in preserves the string's LENGTH, which is the one part that is not decoration: the
per-value clip and the 4000-character model limit are what #456 exists to prevent, and a preview
over a fixed `"string"` would show a template fitting comfortably where the real values overflow.
Capped just past `MAX_VALUE_CHARS`, so a value long enough to be clipped still previews as clipped.
Falling back to the shape is never silent: both captions change, because the box promises "exactly
what the agent would receive". An empty box does NOT clear the stored shape — it is empty both when
the operator emptied it and when the response is simply not on this machine, and the second is the
ordinary case.
The bundle does not carry it, and the omission is fenced rather than left to a comment, because the
`appointment` field beside it in `transfer.ts` teaches the opposite reflex for a measured reason.
The column is compared but not projected into the audit trail, like the four shapes next to it.
Fixes #566
…stay refused
Six findings, all of them real; the P1 refutes the header this module was written under.
**A key is data unless it looks like a schema.** Redacting values alone did not keep the promise:
an API answering `{"users": {"ana@example.com": {…}}}`, or a map keyed by CPF, copied a person's
identifier into the column verbatim. A key outside `^[A-Za-z_$][A-Za-z0-9_$-]*$` now takes its
subtree with it — dropped rather than redacted, because a redacted key is a path that resolves
against nothing and the picker would offer it, and because a path through a map is worthless anyway:
it resolves for exactly one customer. That also closes a save that would have failed at the
database, since a key decoded from a NUL escape or holding a lone surrogate is refused by Postgres
inside a jsonb write.
**A refused number must stay refused.** Measured: both readers answer `undefined` for
`Number.MAX_SAFE_INTEGER + 1` (past 2^53 `JSON.parse` has already lost the digits), so the real
response does not offer that path — and the shape turned it into `0`, which every reader accepts.
Refused numbers now redact to the smallest refused integer, and the rest to all nines of the same
width, which also fixes the rendered length the preview was promising.
**And the redaction is now a fixed point of itself**, because the service redacts a second time on
write and the browser's fingerprint is computed before that pass.
**An own `__proto__` key survived nothing.** `JSON.parse` makes it an ordinary own property and
`walkPath` resolves it, but assigning it onto `{}` runs the legacy prototype setter: the key vanished
from the shape and a working path was missing after a reopen. Built on a null prototype now, spread
back to an ordinary object — measured to keep it, because spread uses CreateDataProperty.
**The browser's copy is matched to the row, not to the tool id.** It carries the fingerprint of the
shape it produced, and a mismatch drops it: another machine saving a newer sample left this one
previewing values the tool no longer describes, and its next save deriving a shape from them. The
fingerprint sorts keys, because jsonb stores an object with its keys REORDERED — measured on the row
this feature writes.
**Deleting a tool takes the browser's copy with it**, fenced over every delete site.
Two things the battery and the tree caught, which are about the tests and not the code:
- the delete fence was GREEN with the call removed, because the file still imported the name. A
fence that asks "is it mentioned?" answers yes for the import that survives the deletion it exists
to catch, so it requires the CALL now;
- both fences rolled their own comment stripper. `tests/lib/source-text.test.ts` fences exactly that
and went red; they use the repo's own scanner now.
And the live arm found a cost that was not reasoned: written as `1e308`, the refused sentinel came
back from jsonb as three hundred and nine digits, while the size cap had counted it as six
characters on the way in. The smallest refused integer stores in the width of an ordinary id — the
same row went from 440 bytes to 134.
Round 2 came back with the same question round 1 had asked, on the other leg, and
the reviewer is right: no lexical rule establishes that a key is a field name
rather than customer data. Round 1 answered it by dropping every key outside
`^[A-Za-z_$][A-Za-z0-9_$-]*$`, which is a heuristic, and round 2 pointed at what
the heuristic accepts: `{"users": {"Ana": …}}` is a map keyed by a first name and
passes the pattern whole. A third heuristic would have been the third answer to
one question, so the model changes instead of the rule.
The database half is gone. No `sample_shape` column, no migration, no
`sample-shape.ts`, no redaction on read or on write, and nothing in the bundle to
fence. What remains is the browser half that was already there: the response,
kept in `localStorage` under `@app:toolSample:<toolId>` with the status it came
back under, written on SAVE rather than per keystroke, restored on open, and
removed when the tool is deleted.
That makes the sentence the whole feature was written under true without any
qualification: the customer's response never leaves the browser it arrived in.
No per-tool opt-in, no backup question, no export rule, and no column whose
redaction a future reader has to re-derive before trusting it. And a path through
a map, the thing the dropped keys would have bought, resolves for exactly one
customer, so the column was paying a privacy question for an offer no operator
could use.
What it costs is written into the module header rather than papered over: an
operator on a second machine, or one whose site data was cleared, gets what they
get today, which is no offer and "Send a test request" as the way back.
Kept from round 1, because they belong to the half that survives:
- the entry is REMOVED FIRST and unconditionally, so a `setItem` that throws on a
full quota leaves nothing rather than leaving the previous response to be
restored as though this save had persisted;
- deleting an HTTP tool takes the browser's copy with it, fenced over every
delete site, since a response left behind outlives the row it described and
tool ids come from a sequence;
- the fence requires the CALL, not the mention, because the file still imports
the name after the call is deleted.
Measured live against this tree, on a real save: the POST body carries no field
about the sample and neither the name nor the CPF appears anywhere in it, the
table has no sample column at all, the reopen restores the response with the
preview rendering real values, clearing site data reproduces the second machine
(empty field, no preview, the test-request button as the way back), and deleting
the tool empties the store.
The battery: eight mutations, eight deaths. Three survived the first pass, all in
`readLocalSample`/`writeLocalSample` robustness against a value something else
wrote into the store, and the tests that kill them are new here (a whitespace-only
sample, a non-string `text`, a `status` that is a string).
One test was a false green and is fixed: happy-dom backs `Storage` with a Proxy,
so `localStorage.setItem = fn` STORES AN ITEM CALLED `setItem` and the real method
keeps running. The quota test now swaps the whole object.
Fixes #566
Round 3 refuses the second half the way round 2 refused the first, and it is
right. `docs/ui.md` carries a standing product rule that names this case in its
own words: localStorage is not admissible for product data, and "History, save,
remember, resume" belong to a backend with `tenant_id` and RLS. A captured API
response is content data and not a UI preference, and my own caption said "comes
back the next time you open this tool" two hundred lines below the rule it was
breaking. The reviewer's second point is the same defect from another side: a
copy in storage outlives every deletion that does not go through this browser,
so a tool dropped over REST or MCP leaves the customer's response behind.
Both places a value could be kept are now refused, for two different reasons:
- **a redacted SHAPE in a column** (round 2), because no lexical rule establishes
that a key is a field name rather than customer data. `{"users": {"Ana": …}}`
is a map keyed by a first name and passes any identifier pattern whole, and a
path THROUGH a map resolves for exactly one customer anyway;
- **the response in `localStorage`** (round 3), for the rule above.
So it is kept where the response already is while the modal is open: in memory,
for the life of the tab, keyed by the tenant selector and the tool id. Closing
the modal keeps it and so does navigating away and back; a reload, a second tab
and a logout do not. Nothing is written down anywhere, which is what lets "we
never store the customer's response" stand with no qualification: no column, no
per-tool opt-in, no backup question, no export rule, no retention policy.
The cost is stated rather than papered over, in the module header and in the
caption the operator reads: after a reload the field is empty and "Send a test
request" is the way back, exactly as before the feature.
Two seams empty it, both fenced over the tree rather than remembered at the one
site that exists: deleting an HTTP tool, because a response left behind describes
a row that is gone; and logging out, because a tab left on the login screen would
otherwise hold the responses of the operator who just signed out of it. The cache
is bounded on both axes (eight entries, evicting the least recently SAVED, plus a
per-entry size cap), since it holds response bodies for as long as the tab lives.
Measured live against this tree, identified before anything was read: saving a
tool with a pasted response leaves both `localStorage` and `sessionStorage`
untouched (only the branding and sidebar-width entries, which are the preferences
the rule allows); reopening the modal and navigating away and back both bring the
response and its preview back; a reload empties the field, kills the preview and
leaves the test-request button, with the caption on screen saying that is what
happens.
The battery: 11 mutations, 11 deaths, including one that writes the sample into
`localStorage` (the rule this round is about) and one that evicts the oldest entry
rather than the least recently saved. A twelfth was a no-op I wrote by mistake, a
`set` followed by the `delete` it was meant to replace, and it is not counted.
Fixes #566
… button Two findings, both real, and the first is the more serious of the two because the path it names is the common one. **The logout button is not how sessions usually end.** A 401 on any request and the socket's auth-loss close both dispatch `auth:unauthorized`, which clears the user without going anywhere near the logout handler. So an expired session left the tab on the login screen with the map still full, and the next sign-in on that same tab was offered the previous operator's responses. The clearing now hangs off `clearUser`, the single `setUser(null)` in the tree, which the explicit logout goes through as well, and the fence counts THAT site rather than the logout request. An explicit logout clearing the user by itself, beside a `clearUser` that also does, is exactly how one of two paths ends up not clearing this. Measured live, in one tab and with no reload (one navigation entry): saved a tool with a pasted response, signed out through the menu, signed back in, reopened the tool. The field is empty and there is no preview. **A save is in flight for as long as the operator's API takes**, and both things that end a sample's life can happen inside that window. The response then arrives and writes the sample back in, which is a deletion or a logout being undone by a request that was already on the wire. Every clearing now bumps an epoch, the save reads it BEFORE its request and hands it back to the write, and a stale one is a no-op. Three things that make the epoch hard to get wrong, rather than merely correct today: - it is a REQUIRED parameter. Optional, dropping the argument at the one call site survived the whole battery; required, `tsc` answers with `Expected 3 arguments, but got 2`; - what `tsc` cannot see is a call that reads it AT the write, which type-checks and always compares equal to itself. That is a question about ORDER, so a fence asks it of the source: the read precedes the request and the write is handed a name, not a call. Scoped to `save()`, because `.v1.tools` appears in that module long before it (the load, and the test-request dialog) and a whole-file index compares two unrelated positions; - deleting one tool and losing the session are now two functions, `forgetToolSample` and `forgetToolSamples`, separate from `rememberToolSample(id, null)`. A save saying there is no sample is not a lifecycle event; the other two are, and only they invalidate what is in flight. Battery: 18 mutations, 18 deaths, counting the type error as the kill it is. New here: a `clearUser` that does not empty, either forget failing to bump the epoch, the epoch ignored inside the module, the argument dropped at the call site, and the epoch read inline at the write. `bun check` green in the master worktree: 10924 pass, 0 fail. Fixes #566
…ept asking for a spelling The finding is real and it is round 4's finding again, on a third path: a `/me` that answers with a null user is how a refresh observes a session the server has already ended, and it went through `setUser(data.user ?? null)`, which clears the user and left the map full. What matters more than the third path is why the fence let it through. It has now asked for a spelling twice and been walked past twice: first the logout REQUEST, which a 401 and the socket's auth-loss close do not make; then `setUser(null)`, which `setUser(data.user ?? null)` is not. Both times the tree had another way of saying the same thing, and the sweep answered about the words. So this is a chokepoint rather than a third call added beside the other two. `AuthContext` has exactly one caller of `setUser`, `applyUser`, and what a transition to unauthenticated costs is written there, where the transition IS, instead of at each of the places that can cause one. Every path goes through it, the explicit logout included. The fence counts calls to the raw setter and requires exactly one, with a positive control over the four spellings that have appeared in this file, including the two that got past its earlier forms; the `useState` destructuring is not a call and is asserted not to count. Battery: 21 mutations, 21 deaths. The three new ones are the chokepoint not emptying, and each of the other two paths going back to calling the setter directly, which is the round 5 finding exactly. `bun check` green in the master worktree: 10924 pass, 0 fail. Fixes #566
…not all of them Two findings, both right. **The identity can change without passing through null.** The cookie is shared across tabs, so a tab sitting on A while another tab signs out and back in as B sees `/me` answer B directly. Asking only whether the user went away missed it, and the entries are keyed by tenant and tool, so B opening the same tool would be handed A's captured response. The rule moved into the module, where it can be exercised without React: `noteOperator(id)` is told who the console is answering for at every transition, and it empties the map whenever that is not who it was. `AuthContext` calls it unconditionally beside its single `setUser`, which also deletes the special case the previous round had written there. Scope, since it is easy to overstate: the finding is right about the code path, and I did not reproduce it live. Today the only consumer of this context's `refresh` is `SetupPage`, so an ordinary console does not observe B in a live tab without a reload, and a reload takes the map with it. The path exists on the context, a reload's boot goes through the same call, and the guard costs nothing, which is why it is unconditional rather than argued about. **A global invalidation over-rejects.** Deleting tool B while tool A's save was in flight dropped A's write too, and the operator sees a tool they never touched come back with an older response or none. Invalidation is per scope now: a clock that ticks on every forgetting, a mark for the last global clear, and a mark per tool. A save's ticket is rejected by a clear that came after it, or by a forgetting of THAT tool; a forgetting of some other tool is not its business. The per-tool marks are dropped on a global clear, since nothing older than one can be accepted anyway, which is also what keeps that map from growing an entry per tool ever deleted in this tab. And one about the tests, which the battery caught: **the fence over the ticket's read order had been deleted**, by an earlier round's edit that replaced the block around it. Two mutations of that call site went from dead to alive between rounds and nothing else said so. It is back, with the accident written into it, because a missing test looks exactly like a passing one. Battery: 21 mutations, 21 deaths, run after a control at `0 fail` and followed by a byte comparison of the code files against their pre-battery state. New here: `noteOperator` not emptying, `noteOperator` asking only about null, either mark failing to move the clock, and a per-tool forgetting made global. `bun check` green in the master worktree: 10940 pass, 0 fail. Fixes #566
…under The finding is right, and it is the third round to find the same SHAPE: something the continuation reads at the end that had already changed while the request was out. Rounds 4 and 6 were the clock; this one is the tenant. `keyFor` read the selector at call time, and that selector lives in `localStorage`, which is shared across tabs and can move mid-flight, as `activeTenant.ts` says in as many words. So a save whose selector moved cached the response under a tenant nobody asked about, and a delete cleared the wrong scope and left the deleted tool's response in memory. Three rounds of one shape is the signal to change the model rather than add a third captured value, so the ticket IS the scope now: one value, read before the request, carrying the clock and the tenant it went out under. The reader still reads the selector at call time, which is right, because a render asks about the tenant on screen now; the writers take it from the ticket, because a continuation asks about the tenant its request went out under. The delete site takes a ticket for the first time, and a second source fence asks the same order question of it that the save already had. Severity, since the finding could be read as worse than it is and the module should not overclaim: `ToolDefinition.id` is a plain autoincrement on one table, so two tenants never share a tool id and a mis-keyed entry is unreachable rather than aliased. What was actually lost was the entry (cached under a key nobody asks for) and what lingered was a deleted tool's response. The tenant in the key is depth, not the thing that stops one tenant's response reaching another, and both the module header and the test that covers it now say so instead of letting a future reader over-trust them. Battery: 25 mutations, 25 deaths, after a control at `0 fail` and followed by a byte comparison of the code files against their pre-battery state. New here: the write and the delete each reading the selector on the way back, the key dropping the tenant, the delete taking no ticket, and the delete reading one inline. `bun check` green in the master worktree: 10943 pass, 0 fail. Fixes #566
…ers most Two findings. One is right and is the worst of the round, one I refute with a measurement, and both leave a fence behind. **A 404 with an empty body lost its status.** The save dropped a sample with no text, and the status went with it. On reopen `sampleStatus` is null, which the preview reads as 200, so a template that reads no field was shown as APPLIED under a box that promises exactly what the agent would receive, while the runtime bypasses it for a non-2xx. Same tool, same template, two different answers before and after a reopen, and the second one is the wrong one. What counts as nothing is now neither text nor status. The reason that was possible is worth more than the fix: **the rule was written twice**, at the call site and in the module, and the call site's copy said something else. So the caller now hands over what is on screen and the module decides alone, with a fence refusing a conditional in that argument. Measured: with the judgement duplicated, reverting the call site's copy survived the whole battery. **The tenant read and the request's own read cannot diverge**, so the second finding does not hold. Measured, not argued: Eden evaluates its `headers` callback INSIDE the call expression, in the same synchronous block, and there is no `await` between `sampleTicket()` and the request at either site. Another tab's `localStorage` write becomes visible at a task boundary, and there is no boundary between the two reads. That refutation is only as good as it stays true, so it is a fence now rather than a paragraph: at both sites, **the first suspension after the ticket is read is the request itself**. An `await` added in between would open exactly the window the finding describes, and would look like an innocent refactor; it now fails. Battery: 29 mutations, 29 deaths, after a control at `0 fail` and followed by a byte comparison of the code files against their pre-battery state. New here: the module dropping an empty body that carries a status, the call site judging again, and an `await` inserted before each request. `bun check`: 10945 pass, 0 fail. One earlier run of the four reported a single failure that did not recur in the three since, and I cannot attribute it: the output of that run was discarded by the pipeline I read it through. Recorded rather than rounded off, with CI as the second sample. Fixes #566
**A cached sample outlived the definition it described.** Change a tool's URL or its response contract from another tab, over REST or over MCP, and the id stays the same, which is exactly what an id-keyed cache matches on: the picker went on offering paths that describe nothing and the preview went on rendering over them, which is how an operator saves a template whose fields never resolve. The entry carries the row's `updatedAt` now, the editor asks about the revision it just loaded, and a mismatch gets what a tool this tab has never opened gets: nothing, and "Send a test request". Two details worth their comments. The revision stored is the one the SAVE returned, not the one the form opened with, since this save is what moved it and the old value would make the next open discard the sample just kept. And it is stringified at the boundary because the treaty TYPES that field as `Date` while the wire carries a string, the `parseDate: false` trap `docs/eden-treaty.md` exists for. The second finding is real about the product and is not this PR's to fix, so it is not fixed here. A tab whose cookie changed to another operator learns nothing about it: measured, nothing revalidates `/me` on focus (only `UpdatesContext` refreshes its own data). Such a tab is already wrong in every way a tab can be wrong before any cache is involved, showing A's name and A's tenant while acting as B, and the fix is broadcasting session changes or revalidating on focus, which changes the auth model for every screen. That belongs to its own issue and its own review, and it is with the maintainer to decide where it is recorded. Battery: 33 mutations, 33 deaths. New here: the revision ignored on read, not stored, taken from the wrong place at the save, and the editor asking about no revision at all. The one that survived first is the same class as round 8's, a call site the module cannot see, and it is fenced the same way: the call must name `data.tool.updatedAt`. `bun check`: 10947 pass, 0 fail. Two runs out of nine reported a single failure each, both unattributed because the output went through a pipeline that discarded the failing test's name, and neither recurred across seven clean runs. Recorded rather than rounded off; CI is the independent sample. Fixes #566
A revision mismatch answered `null` and left the entry in place. Two things come of that, and the second is the one this feature is built around: a known-useless entry occupies one of the eight slots, so seven of them evict the one sample the operator is actually working with; and a customer's response stays in memory after the moment it stopped being servable. The read drops it now, which is the only moment anyone looks at that entry again. One test had to change with it, and the change is the point: it proved "a tool that did not change still gets its sample" by asking about the SAME id, which the drop now empties. It asks about a different tool, and says why. Battery: 34 mutations, 34 deaths. `bun check`: 10949 pass, 0 fail. Fixes #566
Pasting a sample is an unsaved change since #566, so Save is how it is kept, and `payloadOf` sends nothing about it. That made Save on a sample-only change PATCH the whole definition from a form loaded before anyone else's edit, and advance `updatedAt` for a change the row does not contain. The lost-update window is not new (any save in this dialog rewrites the whole row) but this PR is what created a reason to press Save with nothing for the server, so it is this PR's to close. The gate is a value, not a shape: `sendsNothing` compares the persisted half of the form against the baseline captured on open, and is only ever true for an edit that has both a baseline and a known revision. The revision written is a value too, `revisionForSave`: the row that came back when something was sent, and the one this dialog opened with when nothing was. Both are exported and tested directly, which is deliberate. Two earlier rounds found this call site holding a judgement the module could not see, and the fences that caught them were fences over a SPELLING; the second one broke on this round's refactor while the ordering it protects was untouched. A value is checked by asking it. The suspension fence is reworded for the same reason and now asks the property instead of the spelling: the api call must sit INSIDE the awaited operand, asked as "no statement boundary between the `await` and the call", which a parenthesised ternary satisfies and an inserted `await …;` does not. Measured live, on this tree, reading the network log and the row: - sample-only save → no PATCH at all, `updated_at` unchanged at `20:31:38.196`, and the sample comes back on reopen; - a real edit right after → PATCH sent, `updated_at` moves to `20:32:20.671`, and the sample STILL comes back, which is the round 9 and round 11 changes agreeing: it survives only because the save stored the revision the response carried. Battery: 40 mutations, 39 deaths. The survivor is understood and said out loud in the code: the `revision !== null` guard at the write is unreachable, because the gate sends the request whenever the opened revision is unknown, and the type system cannot see that chain. A mutation deleting a guard no state reaches cannot fail a test, and pretending otherwise would be worse than the comment. `bun check`: 10951 pass, 0 fail. Fixes #566
…irst one
**A failed logout ended the session on screen but not on the server.** I first
read this as a regression this PR introduced, by moving the clearing into a
`finally`. It is not, and the correction matters more than the fix: measured
against the treaty with a fetcher that rejects, Eden reports a transport failure
as a VALUE, `{ data: null, error }`, and does not raise. So the previous shape
(`await`, clear, with a `catch`) cleared on the common failure too; the `catch`
only ever saw the rarer case where the client itself throws. The cookie is
HttpOnly, so only the response's `Set-Cookie` can end a session: clearing anyway
shows the login screen to someone still signed in, and on a shared device a reload
brings the session back for whoever is sitting there.
The rule is now `performLogout(post, endSession)`, a function taking its two
effects, and that shape is not decoration either. Two attempts to test it through
the provider failed for reasons worth writing down:
- `mock.module("@/client/lib/api", …)` replaces the module for the whole test
PROCESS, not the file. It took 237 tests in other files down with it, all of
them importing that client and getting a stub shaped for this one;
- with the transport stubbed at `fetch` instead, the test passed alone and failed
in the suite: another file mocks `@/client/contexts/AuthContext` the same way,
so rendering the real `AuthProvider` here gets that stub.
Four tests cover the function directly, and the four mutations of it die.
**A sample can stop describing the tool it was captured from.** A test request
fills it, the operator then edits the URL, the method, the headers, the body, the
query or the credential, and saves: the sample passes the revision check, because
that very save is what set the revision. `requestShapeOf` compares the part of the
definition that decides which response comes back, as an EXCLUSION list, and the
direction is the point: a field added later counts as response-affecting until
someone says otherwise, so the failure is a sample dropped too eagerly rather than
a stale one kept.
**An older opening's response could land on a newer one's sample.** Dismiss a slow
save, reopen the same tool, save again: the first response arrives last, and the
revision cannot tell them apart because the second opening loaded the revision the
first save committed. Each key records when it was last written, and a ticket older
than that is refused.
Also here: the cross-tab account switch from round 9 and round 12's P1 is written
into `docs/roadmap.md` rather than fixed. A tab whose cookie changed to another
operator learns nothing about it (measured: nothing revalidates `/me` on focus or
visibility, no event is broadcast), which makes it wrong in every way a tab can be
wrong before any cache is involved. The fix is broadcasting session changes or
revalidating on focus, which changes the auth model for every screen.
Battery: 50 mutations, 50 deaths. Two survived a first pass and each became a real
test rather than a fence: the save consulting the capture shape, and the logout
rule itself.
`bun check`: 10984 pass, 0 fail, on a base that moved twice today (#574, #575).
The branch was rebased onto it and the diff hash of its own changes is unchanged
either side, `72facc5b`.
Fixes #566
gabrieljablonski
force-pushed
the
feat/566-sample-persist
branch
from
September 7, 2026 21:29
f18a08c to
9c2b903
Compare
…ibes Three findings, all right, and all one shape: the marker that says which definition the sample on screen describes was maintained by hand at four sites, and two of them recorded the wrong thing. Three rounds of one shape asks for a model, not a third patch, so it is one function with one rule now, and the four arrivals are a table in the test. **A sample restored from this tab recorded no definition at all.** That is the worst of the three, because it is round 12's defect surviving a reopen: the refusal built there lasted exactly as long as the modal stayed open. Reopen the tool, change the URL, save, and the old response was stamped with the new revision — offering paths that describe an endpoint nobody calls, which is the whole thing that round fixed. **Format re-captured.** The button re-indents whitespace and changes no value, but it went through the same setter as a fresh capture, so it re-recorded the definition on screen NOW. Paste a sample, edit the URL, press a pretty-printer, and the mismatch the save exists to refuse was erased by a button that only changes line breaks. **And a status with no body recorded nothing either**, for the same reason round 8 existed: the judgement was spelled here as `text.trim()` while the module's own rule says an empty body with a status IS a sample. That rule is exported now and asked rather than spelled again. **A credential is a row of its own, and the revision cannot see it.** The picker inlined in this very modal edits the selected credential's base URL or its secret while the reference stays the same word: the tool's `updatedAt` never moves, the payload is identical, and a relative `urlTemplate` resolves against another host (`credential-wiring.ts` does the joining) with another authorization. The module listens for `VAULT_CHANGED_EVENT` itself, because a credential is edited from three screens and this editor is mounted for at most one of them. It drops the entries that carry a reference and refuses an in-flight save that carried one — not the whole map, since a tool with no credential cannot be affected and round 6 already paid for a global invalidation. It is not scoped further: the event says THAT the vault changed and never which entry, and the secret is server-side, so between keeping a sample that may describe another host and asking for one more test request, this asks for the test request. Two things the battery found rather than the review. A save that CLEARED the sample field is how an operator throws one away, and it reaches the module only through the "nothing was captured" branch: without it the save decides there is no revision to write under, never calls the module, and the deleted response comes back on the next open. And what the save hands over is a value now (`sampleToRemember`), because an object literal at a call site is a field a mutation can change with nothing to notice: the credential taken from the form instead of the payload survived the whole battery while it was written inline. Battery: 56 mutations, 55 deaths. The survivor is equivalent and is left as one: dropping the per-tool marks on a global clear cannot change an answer, because `clearedAt` is checked first and refuses everything older than it. Those marks exist so the maps do not grow an entry per tool touched in this tab, which is a memory property, and the only way to assert it is to export the maps. `bun check`: 11004 pass, 4 skip, 0 fail. Fixes #566
…, and a ticket orders itself Two findings, both right, and both are the previous round's own fix seen from one step further out. **Round 13 dropped the stored entry and left the copy on screen.** Editing the selected credential through the picker inlined in this very modal empties the map, but the response is also in the form, with the definition it was captured against recorded beside it, and the save that follows takes its ticket AFTER the change: nothing refuses it and the sample goes straight back in, describing a request against the host the credential used to name. So the marker carries WHICH VAULT this tab had when the sample was captured, and the comparison at the save finds it no longer matches. That is one string in one function (`captureShapeOf`) rather than a second listener maintaining a second copy of the marker, which is exactly what round 13 removed. The sample itself stays on screen: the operator got that text from somewhere and we cannot get it back for them, so what changes is that the tab stops remembering it, not that it disappears from under them. **And two tickets could be the same number.** The clock only moved when something LANDED, so two saves of one tool that started before either finished were issued the same `at`, and equal numbers cannot be ordered: whichever response arrived first marked the key and the other was refused as stale. The save the operator made LAST could lose to the one they made first, which is round 12's fix firing on the wrong one of the pair. Issuing is what orders them now, and a write records its ticket's own number rather than the moment it landed, because what is being marked is which REQUEST answered for that key. A DELETION records when it landed instead, and the asymmetry is the interesting part: a write is one of several answers competing for a key, so it is ordered by when its request went out, while a deletion ENDS the key. The row is gone and nothing will ever ask for that entry again, so it beats a save that started after the delete request went out too, rather than only one that started before it. The battery is what asked the question: with both marks written the same way, that mutation survived. Battery: 59 mutations, 58 deaths. The survivor is equivalent and stays one: dropping the per-tool marks on a global clear cannot change an answer, because `clearedAt` is checked first and refuses everything older than it, and asserting what those marks are for (the maps not growing an entry per tool touched in this tab) would mean exporting them. `bun check`: 11010 pass, 4 skip, 0 fail. Fixes #566
…same P1 **A vault edit refused samples that no credential could affect.** The marker carried the vault generation unconditionally, so any credential saved anywhere in the console made `sampleDescribes` false for a tool that names none: the save reports success and closes, and the response is silently not kept. That is round 6's over-rejection arriving through the marker instead of through the map. It is carried only for a payload that names a credential now. **And it counted announcements rather than changes.** `refreshVault` notifies twice for one mutation, on the drop and again when the new list lands, so a sample captured between the two halves was marked stale by the second half of the change it already describes. The marker reads the vault's OWN revision (`vaultRevision`, exported for this), and the listener ignores an announcement that did not move it. **A save with nothing to keep never reached the module.** Guarded by `if (keep !== null)`, a sample that stopped describing the definition was refused at the call site and the previous entry stayed in the map, holding a customer's response nobody can be served and occupying one of the eight slots (round 10's finding, one layer up). Null is what deletes it, so null goes over too, which also removes the last judgement at that call site. **A failed logout still navigated.** Round 12 kept the session on a failure and stopped there: both callers navigate to `/login` when `logout()` resolves, and `LoginPage` sends a signed-in visitor straight to `redirectTo`. So the account menu cost the operator the route they were on, and "Switch account" on the consent screen came back as the same operator, both without a word on screen. `performLogout` answers whether the session ended, and `afterLogout` is that decision as a value, because it is made at two sites and a mutation of an `if` written twice survives anything a source fence can ask (measured: a call site reading `if (!ended && false)` walked past the battery). Both rules moved to `src/client/lib/logout.ts`. `AuthContext` is mocked for the whole test PROCESS by another file, so a component importing a rule from there gets `undefined` in every suite that mock reaches — which is the same fact that made `performLogout` a function in round 12, applied one step further. The `UserMenu` and `Header` suites now render inside `ToastProvider`, and the test that asserted navigation "even if logout rejects" asserts the opposite, which is the behaviour this round is about. The P1 is the cross-tab account switch, and it is the third round to raise it. It is right about the product and it is not this PR's to fix: measured, nothing revalidates `/me` on focus or visibility and no event is broadcast, so such a tab shows A's name and A's tenant selection while its requests act as B — wrong in every way a tab can be wrong before any cache is involved. The fix is broadcasting session changes or revalidating on focus, which changes the auth model for every screen in the console. It is written up in `docs/roadmap.md` with the measurement and the shape of the fix. Battery: 68 mutations, 67 deaths, the survivor equivalent for the reason recorded last round. `bun check`: 11015 pass, 4 skip, 0 fail. An earlier run of the same suite reported one failure in `runSandboxedCode` (an offset-less date read in TIMEZONE), which did not recur and which nothing here touches; recorded rather than rounded off, with CI as the second sample. Fixes #566
…n half-closing it One finding, and it is right: a credential edited in a SECOND window, over REST or over MCP, is invisible here. `VAULT_CHANGED_EVENT` is a `window` event dispatched by the window that made the change, and editing a credential does not move the tool's `updatedAt` either, so a sample captured against the old resolution is restored on reopen with nothing refusing it. It is not fixed here, and the reason is what the measurement turned up: this is `vaultCache`'s gap, and the sample is the fourth consumer of it, not the first. The same window shows the stale base URL under the URL field, judges `relativeWithoutBase` on it, answers `useVaultBaseUrls` from it (which is what decides "endpoint missing" on the agent editor), and sends a test request against it. A fix scoped to the cache closes one symptom of four. And closing it is not a missing field. The credential's facts reach the editor from the picker's vault load, which lands AFTER the form does, so a restore that must consult them has to be deferred until they arrive, and the entry has to carry the facts it was captured under instead of recomputing them from whatever the tab sees now — an ordering change in how the editor opens, on top of a server-visible revision that `listVaultInfos` does not select today. So what this round changes is the comment that could be over-trusted. The listener's header now says what it does NOT hear, `docs/ui.md` says the same where it describes the seam, and `docs/roadmap.md` carries the gap with the measurement, the four consumers, and the shape of the fix. Everything this window CAN see is still refused, unchanged from round 15: a vault change here drops the entries that carry a credential and rejects an in-flight save that carried one, and the marker carries the vault's own revision. Battery: 68 mutations, 67 deaths, the survivor equivalent for the reason recorded in round 14. No behaviour changed this round, which is why the count did not move. Fixes #566
… rounds have read it as more One finding, and it is the fourth round to raise the same one: a tab sitting on operator A while another tab signs in as B is never told, and its cached samples are still A's. It is recorded in `docs/roadmap.md` with the measurement and the shape of the fix, and the reasoning has not changed — such a tab shows A's name, A's tenant selection and A's permissions while every request it sends is authenticated as B, so it is wrong in every way a tab can be wrong before any cache is involved, and closing it means broadcasting session changes or revalidating on focus, which changes the auth model for every screen. What HAS changed four times is that the review reads `docs/ui.md` as claiming otherwise, and on that it is right: "a change of operator empties the whole map" is a sentence about what happens when the console is TOLD, written where a reader is asking whether the case is covered. Nothing after it said that a tab which is merely sitting there is never told. So the claim is now conditional where it is made, in the doc and in the module header both: every transition this console MAKES empties the map, and noticing a transition it did not make is the auth model's job, with the pointer to where that is written down. No behaviour changed, so the battery is unchanged at 68 mutations and 67 deaths. Fixes #566
…not after the request The finding is right, and it is the fourth round to find the same SHAPE: something the continuation reads at the end that had already moved. Rounds 4, 6 and 7 were the clock and the tenant selector; this one is the marker that says which definition the sample describes. Everything else that continuation uses is a value this closure captured when Save was pressed — `sample`, `sampleStatus`, and `payload` — and that one is a REF, so reading it after the `await` asks what the form says NOW. Dismiss a slow save and reopen, and the opening that follows writes its own answer into it: the response that comes back is then judged against a capture it has nothing to do with. It can only turn a right answer into a wrong one, in both directions — the sample that DID describe this payload discarded, or one that did not, kept — because at the moment Save was pressed the marker and the payload agreed by construction. So it is read beside the ticket, and the source fence that already asks the ticket's order asks this one's too: read before the request, and read ONCE, since a second read is a second answer and only one of them went out with it. Battery: 70 mutations, 69 deaths, the survivor equivalent for the reason recorded in round 14. New here: the marker read on the way back, and read twice. `bun check`: 11015 pass, 4 skip, 0 fail. Two earlier runs of the same suite failed differently and neither reproduced: eight in `stale-base guard` (that file alone takes 15s warm and 58s under load, and the failures were its 30s timeout), then two in `mirror guard` and `chatwoot-reset`, which pass in 7s on their own. Both look like this machine under the mutation battery rather than anything here, and nothing in this change is reachable from those files; recorded rather than rounded off, with CI as the independent sample. Fixes #566
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
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.
Reopening an HTTP tool threw the sample response away, so the path pickers were re-earned with a real call against the customer's API. Fixes #566.
What the base does, measured
Opened a tool with
Cliente: {{cliente.nome}}saved and a sample pasted, saved it, reopened it onmain:The issue described the first of those. The other two are worse than it claimed, and they are the reason this falls on the operator who is being careful.
The design, arrived at by two refusals
There are exactly two places a captured response could be kept, and review refused both, for two different reasons. The module header carries both, because together they are the design.
A redacted SHAPE in a column (rounds 1 and 2). The first draft answered the issue's own analysis (the pickers need paths; only the preview needs values) by storing the keys with every value replaced by a stand-in, so the pickers would work on any machine. Round 1 said a key is data too and was answered with a rule: drop every key outside
^[A-Za-z_$][A-Za-z0-9_$-]*$, subtree and all. Round 2 came back on the other leg of the same question and is right. That pattern refuses{"users": {"ana@example.com": …}}and accepts{"users": {"Ana": …}}, a map keyed by a first name, whole. No lexical rule separates a schema key from customer data, and a third heuristic would have been the third answer to one question. A path THROUGH a map resolves for exactly one customer anyway, so the column was paying a privacy question for an offer no operator could use.The response in
localStorage(round 3).docs/ui.md:201carries a standing product rule that names this case in its own words: localStorage is not admissible for product data, and "History, save, remember, resume" belong to a backend withtenant_idand RLS. A captured response is content data and not a UI preference, and the caption I had written said "comes back the next time you open this tool" two hundred lines below the rule it was breaking. The reviewer's second point is the same defect from another side: a copy in storage outlives every deletion that does not go through this browser, so a tool dropped over REST or MCP leaves the customer's response behind.So it is kept where the response already is while the modal is open: in memory, for the life of the tab, keyed by the tenant selector and the tool id, with the status it came back under, and written on SAVE rather than per keystroke so what comes back is the sample the tool was last saved with and not a draft that was abandoned.
localStorage, not insessionStorageThe cost is stated rather than papered over, in the module header and in the caption the operator reads: "It is never saved: it stays open for as long as this tab is, and is gone after a reload." What it buys is that "we never store the customer's response" needs no qualification: no column, no per-tool opt-in, no backup question, no export rule, no retention policy, and nothing whose redaction a future reader has to re-derive before trusting it.
Live exercise, against this tree
Dev server on this worktree's tree, identified before anything was read: the served bundle carries the new caption and contains no
@app:toolSamplekey.Created a tool with the template
Cliente: {{cliente.nome}}and this sample:{"cliente":{"nome":"Ana Prado","cpf":"12345678901"},"pedidos":[{"id":9001}]}localStorageholds@app:brandingand@app:sidebar-widthand nothing else,sessionStorageis empty. Neither the name nor the CPF is anywhere in either;Cliente: Ana Prado, and "Insert a field" is there;An earlier round measured the other half of the invariant on the same seam and it still holds: the POST body of a real save carries no field about the sample, and neither the name nor the CPF appears in it.
What empties it, and both are fenced over the tree
/methat answers with a null user all end a session without one; and a shared cookie can move a live tab from operator A straight to B with no null in between, which asking about the null misses entirely. So the module is askednoteOperator(id), whether this is who it was, andAuthContextcalls it unconditionally beside its single caller ofsetUser. Measured live for the session case, one tab, no reload (one navigation entry): saved a tool with a pasted response, signed out through the menu, signed back in, reopened the tool. Empty field, no preview. Not measured live for the A-to-B case, and the scope is worth stating rather than overstating: today the only consumer of this context'srefreshisSetupPage, so an ordinary console does not observe B in a live tab without a reload, and a reload takes the map with it. The path exists on the context and a reload's boot goes through the same call, so the guard is unconditional rather than argued about.The fence over the second one counts calls to the raw
setUserand requires exactly one. It had asked for a spelling twice and been walked past twice: first the logout REQUEST, which a 401 and the socket do not make, thensetUser(null), whichsetUser(data.user ?? null)is not. Both times the tree had another way of saying the same thing, and the sweep answered about the words.A request is in flight for as long as the operator's API takes
Three review rounds found the same shape here: something the continuation reads at the end that had already changed while the request was out. So there is one value rather than a habit of capturing things. The ticket is the world as it was when the request went out: read before it, handed back to whatever mutates the cache afterwards, carrying the clock and the tenant.
The clock half. Both things that end a sample's life can happen while a save is out, and the response then arrives and writes it back in, undoing a deletion or a logout with a request that was already on the wire. Checked per SCOPE and not globally, because a global check over-rejects: deleting tool B while tool A's save is out would drop A's write too, and the operator sees a tool they never touched come back with an older response or none. A clock ticks on every forgetting, one mark records the last global clear and one per tool records its own; the per-tool marks are dropped on a global clear, since nothing older than one can be accepted anyway, which is also what keeps that map bounded.
The tenant half.
keyForused to read the selector at call time, and that selector lives inlocalStorage, shared across tabs and free to move mid-flight (activeTenant.tssays so in as many words). A save whose selector moved cached the response under a tenant nobody asked about; a delete cleared the wrong scope and left the deleted tool's response in memory. The reader still reads at call time, which is right (a render asks about the tenant on screen now); the writers take it from the ticket (a continuation asks about the tenant its request went out under).Worth stating rather than letting it read as worse:
ToolDefinition.idis a plain autoincrement on one table, so two tenants never share a tool id and a mis-keyed entry is unreachable rather than aliased. What was lost was the entry; what lingered was a deleted tool's response. The tenant in the key is depth, not the thing that stops one tenant's response reaching another, and the module header and the test that covers it both say so.Two things make the ticket hard to get wrong rather than merely correct today:
tscanswersExpected 3 arguments, but got 2;tsccannot see is a call that reads it AT the write, or anawaitslipped in before the request. Both are questions about ORDER, so a fence asks them of the source, once per site: the read precedes the request, the write is handed a name and not a call, and the first suspension after the read is the request itself. That last one is what makes the ticket and the request see the same tenant selector, and it is measured rather than assumed: Eden evaluates itsheaderscallback INSIDE the call expression, in the same synchronous block, and another tab'slocalStoragewrite becomes visible only at a task boundary. There is no boundary between the two reads, and anawaitadded in between (which would look like an innocent refactor) now fails.Also separated: deleting one tool and losing the session are two entry points (
forgetToolSample,noteOperator), neither of themrememberToolSample(id, null). A save saying there is no sample is not a lifecycle event; the other two are, and only they invalidate what is in flight.Tests
tests/client/tool-sample-persistence.test.ts(69) andtests/client/auth-logout-failure.test.ts(7), in eight parts: what the editor opens with; that nothing is sent and nothing is stored, the second asserted as a before/after snapshot of both stores rather than as "the store is empty", because the suite shares one global environment and other files leave entries behind, so emptiness would measure them and not this; what the tab remembers (round trip, oversize, nothing at all, a status that came back with an empty body, the eviction order, the tenant a request went out under, a browser that refuses storage entirely, one tool gone, the session gone, one operator becoming another); a save that lands after the sample's life ended, in every direction plus the control that it still writes when nothing relevant happened; and the source fences, each with a positive control.A sample describes one version of a tool. Change a tool's URL or response contract from another tab, over REST or over MCP, and the id stays the same, which is exactly what an id-keyed cache matches on: the picker went on offering paths that describe nothing. The entry carries the row's
updatedAt, the editor asks about the revision it just loaded, and a mismatch gets what a tool this tab has never opened gets. The revision stored is the one the SAVE returned, since the save is what moved it, and it is stringified at the boundary because the treaty TYPES that field asDatewhile the wire carries a string (theparseDate: falsetrap indocs/eden-treaty.md).A status with no body is the sample that matters most. A test that came back 404 with nothing in it is what makes the runtime bypass the template, and a template reading no field previews perfectly well over an empty body. Dropped for having no text, the status went with it, and the reopened tool previewed that same template as APPLIED, under the box that promises exactly what the agent would receive. What counts as nothing is neither text nor status, and that judgement lives in the module alone: the rule used to be written twice, and the copy at the call site is what said something else.
Mutation battery: 68 mutations, 67 deaths, counting the type error as the kill it is. Among them: writing the sample into
localStorage(the rule round 3 is about), evicting the oldest entry rather than the least recently saved, keying without the tenant selector,noteOperatornot emptying or asking only about null, either mark failing to move the clock, a per-tool forgetting made global, the ticket ignored, the argument dropped at either call site, the ticket read inline at either write, the write or the delete reading the tenant selector on the way back, the key dropping the tenant, the module dropping an empty body that carries a status, the call site judging what to keep again, anawaitinserted before each request, the revision ignored on read or taken from the wrong place at the save, and each auth path going back to calling the setter directly. The control printed0 failfirst and the code files were byte-compared against their pre-battery state afterwards.The battery also caught something about itself, which is why it is run every round rather than once: a fence had been deleted by a later edit that replaced the block around it, and two mutations of that call site went from dead to alive with nothing else saying so. A missing test looks exactly like a passing one.
bun checkin the master worktree: 11015 pass, 4 skip, 0 fail, on a base that moved twice during the work (#574, #575); the branch was rebased onto it and the diff hash of its own changes was unchanged either side. The one battery survivor is equivalent and recorded as one: dropping the per-tool marks on a global clear cannot change an answer, because the global mark is checked first.What empties it, continued
Set-Cookieends a session; clearing the console's user on a failure shows the login screen to someone still signed in, and a reload brings the session back for whoever is at the machine. Measured against the treaty with a fetcher that rejects: Eden reports a transport failure as a VALUE,{ data: null, error }, and does not raise, so the previous shape (await, clear, with acatch) cleared on the common failure too. The rule isperformLogout(post, endSession), a function taking its two effects, tested directly.Two things about the tests that are worth more than the code they cover
performLogoutis a function and not a rendered provider because two attempts to test it through one failed, each for a reason this repo should not learn twice:mock.module("@/client/lib/api", …)replaces the module for the whole test process, not the file. It took 237 tests in other files down with it, every one of them importing that client and getting a stub shaped for this one;fetchinstead (inbeforeAll, restored inafterAll), the test passed alone and failed in the suite: another file mocks@/client/contexts/AuthContextthe same way, so rendering the realAuthProviderhere gets that stub.And the mutation battery caught a fence that an earlier round's edit had deleted, by replacing the block around it: two mutations of that call site went from dead to alive with nothing else saying so. A missing test looks exactly like a passing one, which is why the battery is run every round rather than once.
What the marker says, and how many ways it was wrong
A sample describes ONE version of a tool, so the entry carries the row's
updatedAtand a mismatch discards it. But the revision alone cannot answer the question, because the save is what sets it: a response captured against one URL, followed by an edit to the URL, the method, the headers, the body, the query or the credential, passes that check by construction. So the editor also records what the sample DESCRIBES —requestShapeOfover the part of the definition that decides which response comes back, as an EXCLUSION list whose direction is the point: a field added later counts as response-affecting until someone says otherwise.That marker was maintained by hand at four sites, and review found two of them wrong at once, which is why it is one rule (
shapeOfArrival) with the four arrivals as a table in the test rather than four call sites:And
credentialRefis a NAME. What it resolves to is a row in the vault, which the picker inlined in this very modal can edit: the tool'supdatedAtnever moves, the payload is byte for byte the same, and a relativeurlTemplatenow resolves against another host (credential-wiring.tsdoes the joining) with another authorization. So the marker carries the vault's own revision too — only for a payload that names a credential, since prefixing it unconditionally made any credential saved anywhere in the console refuse a sample nothing could have invalidated, and the vault's OWN revision rather than a count of announcements, sincerefreshVaultannounces twice for one change.What empties it, continued
Set-Cookieends a session; clearing the console's user on a failure shows the login screen to someone still signed in, and a reload brings the session back for whoever is at the machine. Measured against the treaty with a fetcher that rejects, Eden reports a transport failure as a VALUE,{ data: null, error }, and does not raise, so the previous shape (await, clear, with acatch) cleared on the common failure too./loginon any resolution, andLoginPagesends a signed-in visitor straight toredirectTo: the account menu cost the operator the route they were on, and "Switch account" on the consent screen came back as the same operator, both without a word on screen.performLogoutanswers whether the session ended andafterLogoutis what the two buttons do with that answer, as a value, because anifwritten at two call sites survives anything a source fence can ask (measured: one readingif (!ended && false)walked past the battery).Two things about the tests that are worth more than the code they cover
Both logout rules live in
src/client/lib/logout.tsand not inAuthContext, because two attempts to test them through the provider failed for reasons this repo should not learn twice:mock.module("@/client/lib/api", …)replaces the module for the whole test process, not the file. It took 237 tests in other files down with it;fetchinstead, the test passed alone and failed in the suite: another file mocks@/client/contexts/AuthContextthe same way, so anything imported from there is a stub in every suite that mock reaches.And the mutation battery is what found four things the review did not: a fence an earlier round's edit had deleted (two mutations of that call site went from dead to alive with nothing else saying so), the save that CLEARED the sample field never reaching the module, the credential taken from the form instead of the payload, and the deletion mark that has to beat a save which started after the delete went out. A missing test looks exactly like a passing one.
Two findings from review are deliberately not fixed, and both are written down
Neither is a defect this PR introduces; each is a property of a layer underneath it, and each would be half-closed by a fix scoped to this feature. Both are in
docs/roadmap.mdwith the measurement, why it is not this PR's, and the shape of the fix.A tab whose cookie changed to another operator learns nothing about it (raised in rounds 9, 12, 15 and 17). Measured: after boot nothing revalidates
/meon focus orvisibilitychange, and no event is broadcast. Such a tab already shows A's name, A's tenant selection and A's permissions while every request it sends is authenticated as B — wrong in every way a tab can be wrong before any cache is involved. The fix is broadcasting session changes or revalidating on focus, which changes the auth model for every screen.A vault change made in another window is invisible to this one (round 16).
VAULT_CHANGED_EVENTis awindowevent from the window that made the change, and editing a credential does not move the tool'supdatedAt. The sample cache is the FOURTH consumer of that gap, not the first: the same window shows the stale base URL under the URL field, judgesrelativeWithoutBaseon it, answersuseVaultBaseUrlsfrom it, and sends test requests against it. Closing it needs a server-visible revision thatlistVaultInfosdoes not select today, plus an ordering change in how the editor opens.