fix(rag): say that embedding generation runs, and test it through context.db (#1128, #1272) - #1332
Conversation
…text.db The secured write surface executes on the Prisma 8 collection since #1281, so automatic embedding generation runs end to end. Four places still told users and agents it was inert, one of them a pending changeset that would have published "the write throws on every invocation" as the release note for the feature. - Correct `.changeset/loud-comets-invent.md`, `.changeset/silver-moths-gather.md` and `packages/rag/CLAUDE.md`. `allowManualWrites` is presented only as the deliberate opt-out for an app that maintains its own vectors, never as a way around a broken write path. - Remove `isUnportedWriteSurface` and the reporter branch it gated, which was dead code logging a false "EMBEDDING GENERATION IS NOT RUNNING ... No config change works around it", plus the test that certified the unreachable branch. - Re-point the write-denial and search tests at `context.db` (#1272): rows are seeded by writing source text through the secured surface, so every vector under assertion is the plugin's own output. Same for core's multi-column write-access gate, and for the derived-source case that drove `hookPipeline`. - `hookPipeline` leaves `@opensaas/stack-core/internal`; nothing depends on it. - Guard the possibly-null `.first()` in the "Find Similar Items" sample. Refs #1128, #1272 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 7a05f35 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Coverage Report for Core Package Coverage (./packages/core)
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report for UI Package Coverage (./packages/ui)
File CoverageNo changed files found. |
Coverage Report for CLI Package Coverage (./packages/cli)
File CoverageNo changed files found. |
Coverage Report for Auth Package Coverage (./packages/auth)
File CoverageNo changed files found. |
Coverage Report for Storage Package Coverage (./packages/storage)
File CoverageNo changed files found. |
Coverage Report for RAG Package Coverage (./packages/rag)
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
Coverage Report for Storage S3 Package Coverage (./packages/storage-s3)
File CoverageNo changed files found. |
Coverage Report for Storage Vercel Package Coverage (./packages/storage-vercel)
File CoverageNo changed files found. |
borisno2
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
(Posted as a Comment review — the GitHub identity here is the PR's own author, so a formal Request-Changes event is refused.)
Reviewed at b35f316, base prisma-8 (de80685a). Everything below was run, not read.
The premise holds — verified independently
I did not take it from the PR body. A fresh probe on the Test harness (PGlite, own fake provider, own list):
| Probe | Result |
|---|---|
ctx.db.Doc.create({ data: { body: 'red' } }) |
commits, reads back {"vector":[1,0,0],"metadata":{…,"sourceHash":"2f0x"}} |
update body 'red' → 'blue' |
regenerates: vector:[0,1,0], new sourceHash |
nearest('bodyEmbedding', [0,1,0]) |
returns ranked rows |
ctx.db.Doc.delete(...) |
executes; row gone |
packages/rag full suite |
456 passed / 0 skipped / 20 files — matches the claim exactly |
Mutations reproduced (not taken on trust):
splitMultiColumnFields' denial →if (!canWrite && false)→ coreTHROWS when update access is denied…,THROWS when create access is denied…fail; ragan ordinary create naming the embedding throws,an ordinary update naming the embedding throws,a create the operation gate admits throws…fail. As reported.- generation hook returns before its sudo write → 15 failures: 11/13
search.test.tsplus 4 inembedding-write.test.ts. Under the oldwithOrigin('unsafe')seeding this mutation moved nothing, so the re-point is a genuine gain in discrimination.
The single-gate survivor framing is accurate, and if anything under-claims. the write the throw refused never reaches the columns stays green under the single-gate mutation because filterWritableFields still refuses with Validation failed: Cannot update "contentEmbedding" (via column "contentEmbedding"): field-level access denied. — which still contains the test's substring. That is not a blind test: it asserts an outcome (the columns stay null) that the second gate genuinely preserves. Calling it a limitation is honest and correct.
Also verified clean: isUnportedWriteSurface has zero remaining references anywhere outside dist/; removing its test removed coverage of nothing still live (the transient-classification arm is still tested). hookPipeline is genuinely unreleased — I unpacked published @opensaas/stack-core@0.42.2 and dist/internal.d.ts does not export it; the only remaining users are write-pipeline.ts and core's own tests by relative path. Removing the announcing paragraph from brisk-columns-refuse.md is correct, not erasure. NestedRelationInputError is genuinely thrown for a nested spelling (relationship-input.ts:158), so that corrected Known limits entry is true. No any, no casts (the unknown → {} narrowing in storedMultiColumn is legitimate). eslint and tsc --noEmit clean on every changed file. Both changesets are minor.
Blockers
B1 — This PR found silent data corruption and buried it in a test fixture
embedding-write.test.ts's Derived fixture guards its resolveInput with typeof resolvedData.title === 'string', and the comment says why: the plugin's sudo write re-runs the whole hook pipeline carrying only the embedding column. The PR body records this under "worth recording rather than hiding". But it is not a fixture quirk — it is a live, silent data-loss bug in application code, and this PR's own premise is what makes it reachable for the first time.
Probe, using the exact list-level resolveInput pattern the root CLAUDE.md documents, unguarded:
CLOBBER content: " "
CLOBBER embedding: {"vector":[1],"metadata":{...,"sourceHash":"w",...}}
The user's content is destroyed (join of two undefineds), and the stored embedding is the embedding of the clobbered text. Nothing is logged. Before #1281 this was unreachable because the sudo write never executed; it is reachable now.
Two consequences:
eager-vectors-arrive.mdannounces "Automatic embedding generation runs" with no caveat. That release note ships this.- The new test
the embedded text is the persisted source, not the caller's inputpasses only because the fixture defends itself. It certifies the workaround, not the contract.
Needed before merge: a Known limits entry in packages/rag/CLAUDE.md, a warning paragraph in the changeset, and a tracking issue. (The real fix — the sudo write skipping the list-level resolveInput, or carrying the persisted row — is out of scope here.)
B2 — A pending changeset still publishes the stale claim this PR exists to delete, and it is false
.changeset/humble-handles-narrow.md:30:
The Write Pipeline rebinds
ormHandlewherever it rebindscontext.db, exactly as it did before — this rename changes nothing about when a hook's database work is transactional. (Onprisma-8no write currently opens a transaction at all: #1205.)
Pending, publishes as a release note for stack-core/stack-auth/stack-rag. The sweep was grep -rn "1124\|#1127", so every instance citing only #1205 was invisible to it — including this one, in the highest-consequence surface the PR names.
And the claim is false. Probe: an afterOperation hook writes a row through context.ormHandle and then throws; the enclosing create rejects and the hook's row is gone. Control (same probe, no throw): the row lands. Same result driving context.db from the hook.
B3 — "Could not settle" on the two remaining doc claims is not accurate; they are settled, and false
docs/content/how-to/write-a-plugin.md:266 and docs/content/reference/context-api.md:188 carry the same false statement, left on the stated grounds that "a probe on the Test harness showed _transactionOpener absent there, so the harness cannot decide it."
That probe measured the wrong object. getContext sets _transactionOpener on the internal AccessContext (packages/core/src/context/index.ts:635) — the one populateDbDelegate binds every db delegate to — and then returns a separate StackContext literal (:1088-1100) that deliberately omits it. Its absence on the returned context is by design and proves nothing either way.
Both the harness (packages/core/src/testing/context.ts:450) and a real application (generated getContext, packages/cli/src/generator/context.ts:255) pass the Prisma 8 client as the 8th positional argument, so transactionOpenerFor returns an opener and writes open a transaction. Rollback works — B2's probe is the proof.
Flagging rather than asserting was the right instinct; the reason given for it is wrong, so the flag reads as "unknowable" when it was one probe away. Either fix all three sites, or restate the flag accurately.
Follow-ups (non-blocking)
- F1 —
delete()leaks the un-assembled multi-column shape. My probe'sctx.db.Doc.delete(...)returned{ bodyEmbedding: [0,1,0], bodyEmbeddingMetadata: {…} }, whilefirst()on the same row returns{ vector, metadata }and hides the sibling column. Pre-existing, not this PR's — but it is the same assembly contract the new tests assert on the read path, and no test here coversdelete's return value. Worth a ticket. - F2 — the
stack-corebump ineager-vectors-arrive.md. Core's only change is a test file plus removal of an unreleased/internalexport. The core paragraph of that release note describes test coverage, which is not a user-visible change. Accurate prose, over-claiming as aminor. - F3 —
toEqual→toMatchObjectinthe two columns read back as one stored embedding. Necessary (dynamicsourceHash/generatedAt), but it drops the "no extra keys" guarantee the old assertion carried — the very leak the next line checks for on the row. Consider pinningObject.keys(stored.contentEmbedding)to['vector','metadata']. - F4 —
test('the plugin’s sudo write reaches the column')escapes the apostrophe while its neighbours in the same file use the literal’. - F5 — the mutation table undercounts row 3: that mutation also takes down
the embedded text is the persisted source, not the caller's input(15 failures, not 14). Immaterial to the conclusion, but the table is presented as evidence.
Summary. The premise is real and I verified it myself; the deletions are safe; the re-point onto context.db is a genuine and measurable improvement in discrimination; the hookPipeline removal is correct on every ground claimed. Blocking on B1 (a live data-corruption path this PR discovered, guarded in a fixture, and announced as working with no caveat) and B2/B3 (the pattern this round exists to end, surviving in a pending changeset and two docs pages because the sweep was keyed on the wrong issue numbers and the "cannot decide" verdict rested on a mis-probe).
…066)
The RAG plugin wrote its generated embedding through `sudo().db.<list>.update()`.
That is an ordinary secured write, so it re-ran the list's whole hook pipeline
carrying the embedding column and nothing else — and a list-level `resolveInput`
deriving one field from other input, the pattern the root CLAUDE.md documents,
then recomputed that field from values that were not there. Measured unguarded:
`create({ data: { title: 'red', body: 'hot' } })` committed `content: 'red hot'`
and then overwrote it with `' '`, and embedded that. Silent, in the database and
in the index. Unreachable until #1281 made the write execute.
Core now owns the write as one field: `writePluginOwnedField` on
`@opensaas/stack-core/extend` splits the value through the field's own
`splitColumns` and issues one id-scoped UPDATE marked with the engine origin,
running no hook and reaching no other field — a narrower capability than the
escalated `db` update it replaces. It takes the AccessContext `Plugin.runtime`
receives; the StackContext `getContext` returns carries no ORM handle and is
refused by name.
The `Derived` fixture's guard against the corruption is deleted. Three rag tests
and six core tests pin the contract, and all fail against the write this
replaces.
Also settles the two documentation claims the previous round flagged as
unsettled. An `afterOperation` writing through `ormHandle` and then throwing
leaves no row, with a control confirming it lands without the throw; the earlier
probe read `_transactionOpener` off the returned StackContext, which omits it by
design. `write-a-plugin.md`, `context-api.md`, CONTEXT.md and the pending
`humble-handles-narrow.md` are corrected — the last was invisible to the previous
sweep, which was keyed on #1124/#1127 and missed every instance citing #1205.
Restores `toEqual` where the round had loosened it to `toMatchObject`, pinning
the assembled embedding to exactly its two keys.
Refs #1128, #1272.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Addressed at B1 — fixed, not documentedRan the corruption first, unguarded, with the exact list-level
You measured the same thing, and the second column is the half neither of us had counted: the write also double-fires every side-effect hook for one logical change. And The mechanism. Core now owns the write as a single field: The reasoning: the plugin's write is not an application update. It carries one column and completes a write the application already made, whose hooks have already run against the caller's real input. Running them again over a payload that names one field lies to every one of them. Carrying the persisted row — your other candidate — I rejected with a reason I can show. A field-level One wiring correction fell out of it. The fixture's guard is deleted and
And on core's own four, mutating The re-entry guard's job narrows accordingly: B2 — fixed, and the sweep found one more
That found one site beyond the one you named: Four sites in total: the changeset, The related class sweep also turned up three descriptions of the write as happening "under sudo" — B3 — settled, and I verified it rather than taking itYou are right that the probe measured the wrong object. Verified independently rather than on your word, and made it a committed test ( Follow-ups
Gates
Both new changeset entries are |
borisno2
left a comment
There was a problem hiding this comment.
Review — REQUEST CHANGES
Commit under review: 85007129 ("fix(core,rag): a plugin's write of its own column runs no hook (ADR-0066)") only. Earlier commits on this branch were reviewed in previous rounds and are not re-reviewed here.
Posted as a Comment review, not a formal
REQUEST_CHANGESevent: the reviewing GitHub identity (borisno2) is this PR's own author, and GitHub refuses a formal review event on your own PR. Treat the verdict line above as the review state.
The central claim holds. I reproduced the corruption on the parent and confirmed it is gone here, and I reproduced both halves of the bug and one of the reported mutations independently. What blocks is accuracy in the two permanent artefacts this commit adds — an ADR whose load-bearing rejection is false as written, and a how-to page that still teaches the deleted pattern 16 lines below the prose that deletes it.
Verified independently
The corruption, on b35f3168 (parent). I deleted the Derived fixture's typeof resolvedData.title === 'string' guard, added the unguarded derivation and an afterOperation counter, and ran:
PROBE content = " " (expected "red hot")
PROBE afterOperations = ["after","after"] (expected ["after"])
On 85007129 the same fixture — now committed unguarded — passes: content: 'red hot', vector [0,0,1], ['after']. packages/rag is 460 passed / 0 skipped / 20 files.
Both halves of the second defect (item 4), on the parent. Adding a list-level validate that records Object.keys(resolvedData):
PROBE validate saw = ["[\"title\",\"body\",\"content\"]","[\"contentEmbedding\"]"]
It ran twice, the second time over a record naming one field, and threw. One correction to how the PR body and ADR phrase this: it did not fail the caller's write — the generation hook's try/catch swallowed it and reported it through reportGenerationFailure as a transient provider failure, advising "If the cause has cleared, retry by writing the source field again." Silently no embedding, plus actively wrong advice. Worse in kind than "fails a committed write", and the ADR's phrasing should say what actually happens. Both halves pass on this commit (validate runs once).
The mechanism. updateFirst really is withOrigin('engine', …) (secured/write.ts:132-139), so the engine-origin claim is true. getContext's returned StackContext genuinely omits ormHandle, so the HandlelessPluginFieldWriteError guard is live rather than dead code. AccessContext.ormHandle is readonly and the Write Pipeline builds new context objects rather than mutating, and _sharedPlugins skips re-running runtimes on transaction rebind — so the handle the writer closes over is the top-level non-transactional one, which is correct for a write draining after afterTransaction.
Mutation reproduced. Replacing the splitColumns branch with { [fieldName]: value } fails exactly the four reported core tests (Unknown column "avatar" in table "Owned"). Restored after.
Same shape elsewhere — swept, none found. sudo().db writes across all packages: auth's two uses are findUnique reads only, and its better-auth adapter works off the raw ORM, so neither re-enters hooks. The only other context.db[…].update( in core is mcp/handler.ts:480, a caller-driven application write. This defect had one instance.
Gates. eslint clean on all six changed source files; tsc --noEmit clean on packages/core and packages/rag; changed core suites 34/34. No any and no casts in the production code added. Changeset is minor/minor — correct for a new public export — and its usage example matches the shipped five-argument signature.
Blockers
1. ADR-0066's load-bearing rejection is factually false
"
password()'s hook hashesresolvedData[fieldKey], so handing it back the stored hash double-hashes it."
Both halves are wrong. packages/core/src/fields/index.ts:822-836 reads inputData[fieldKey], not resolvedData[fieldKey], and carries an explicit guard:
// Idempotent: skip re-hashing a value that's already a hash.
if (isHashedPassword(inputValue)) {
return inputValue
}isHashedPassword matches /^\$2[aby]\$\d{2}\$.{53}$/, which is exactly what hashPassword emits. Verified empirically: hashing 'hunter2' and feeding the result back returns it byte-identical. password() would not double-hash.
The decision survives — the generic argument holds for any non-idempotent transforming hook an application writes (slugify-and-append, counter increment), and the ADR's other two objections in the same bullet ("turns a one-column write into a whole-row write", "only ever works for hooks that are pure functions of the row") are independently sufficient. But an ADR is a permanent record that later readers reason from, and this one cites a verified-false example as the reason for its central rejection. Replace the password() example with one that is actually true of this codebase, or drop it and lean on the two objections that hold.
The other rejections check out: skipHooks on context.db (correct — wrong surface for the capability), skip-only-resolveInput (correct — I measured validate and afterOperation failing too), plugin-writes-columns-itself (correct — duplicates ADR-0049's layout), and document-and-ship (correct — the failure is silent and unworkaroundable). The ADR otherwise matches what was built.
2. docs/content/how-to/rag-advanced.md:79 still teaches the pattern this commit deletes
This commit rewrote lines 44-63 of that page to say the write goes through writePluginOwnedField and runs no hook. Sixteen lines later the "Simplified hook implementation" block is untouched:
await writeUnderSudo(listName, item.id, fieldName, {Old name, old four-argument signature, and the page contradicts itself within one screen. This is the how-to for plugin authors — the audience for the new export — and a reader who copies the block reintroduces the exact corruption ADR-0066 exists to close.
Should fix before merge
3. The new public API's undefined semantics diverge by field shape
PluginOwnedFieldWrite.value is documented as "the field's logical value, or null to clear it". Passing undefined:
- with
splitColumns(embedding:isStoredEmbedding(value) ? value : null) →{ vector: null, metadata: null }— a silent wipe; - without →
{ [fieldName]: undefined }— an ORM no-op.
Same call, opposite outcomes, decided by a field-shape detail the caller isn't looking at. On a brand-new public export, reject undefined by name or normalise it to null before the split.
4. Two standing write errors are now misreported as transient
The previous commit removed isUnportedWriteSurface, which was generation-failure.ts's only standing-write branch. This commit adds two errors reachable from that same catch — HandlelessPluginFieldWriteError (new here) and WriteCollectionMissingError (whose own message says "Re-run opensaas generate"). Both fail identically on every row until wiring is fixed, and both now land in:
"…failed for a reason that is not a standing defect — … If the cause has cleared, retry by writing the source field again."
per row, forever. That is precisely the shape the module docblock says the classification exists to prevent, and it defeats the "refused by name" property the ADR and changeset advertise: the one place HandlelessPluginFieldWriteError can actually fire is the one place that swallows it into retry-later advice. I observed this class live while probing the parent (a ValidationError reported as a provider failure).
5. The as unknown as AccessContext cast survives — at the line the ADR diagnoses
ADR-0066's Context names it, but the commit only stops RAG from using sudo. packages/core/src/context/index.ts:652 is unchanged:
context.plugins[plugin.name] = plugin.runtime(
context,
() => sudo() as unknown as AccessContext,
)Plugin['runtime']'s sudo parameter is still declared () => AccessContext while delivering a StackContext with no ormHandle, _isSudo, _resolveOutputChain or _transactionOpener. The trap stays armed for the next plugin author. It also explains why plugin-field-write.ts:78's context.ormHandle === undefined guard is unreachable to the type checker (AccessContext.ormHandle is readonly ormHandle: OrmClient, non-optional) and why its own test has to cast to fire it. Narrowing that parameter's declared type is the real fix; the cast is the last thread of the bug you diagnosed.
Follow-ups (non-blocking)
6. "It reaches no other field" is caller discipline, not enforcement. Nothing validates fieldName against the list, or that fieldConfig is that field's config. Verified against a real database:
writePluginOwnedField({ context, listName: 'Thing', id, fieldName: 'secret', fieldConfig: {}, value: 'PWNED' })
// → secret === 'PWNED'This is not a privilege escalation — a plugin already holds context.ormHandle, which CONTEXT.md says "bypasses as much as unsafe does" — and the payload-shape narrowing is real per call. But the changeset and ADR both phrase a convention as a capability boundary ("It reaches no other field", "cannot be aimed at anything else"). One sentence in the docblock saying the field is the caller's assertion would settle it.
7. packages/rag/src/config/plugin.ts:534 still throws "…so a generated embedding has no sudo write to reach its write-denied column through." There is no sudo write any more.
On scope
Keep the ADR and the export here. You cannot fix the corruption without moving the write into core, so the new export is the fix, and the ADR is the record superseding ADR-0045's spelling of that write. Splitting them would park a live silent-data-corruption fix behind a second PR.
What should have been separated is the #1205 documentation sweep — CONTEXT.md, write-a-plugin.md, context-api.md, humble-handles-narrow.md and the new write-transaction.test.ts case. It is unrelated to ADR-0066, and it is the third distinct thing in a commit already carrying two. (The test itself is good: the control case confirms the rollback assertion isn't passing because the write never reached the database.)
Blockers 1 and 2 are text corrections. 3, 4 and 5 are small code changes. None of them undermines the design — the fix is at the right layer, the fixture's guard is genuinely gone, and the mutations bite.
…rrect ADR-0066 Review follow-up on #1332. ADR-0066 rejected "carry the persisted row" on the grounds that `password()`'s field hook would re-hash an already-hashed value. It would not: that hook reads `inputData[fieldKey]` and returns an already-hashed value unchanged via `isHashedPassword`. `storage`'s `file()`/`image()` hooks are idempotent for the same reason (ADR-0006), so no shipped field type would corrupt. The rejection now rests on what survives a whole-row payload: `validate` over a record no caller submitted, the double-fired side-effect hooks, and the generation hook's re-entry — none of which carrying the row removes. `docs/content/how-to/rag-advanced.md` still called the write by its old name and four-argument signature, sixteen lines below the prose that replaced it. Swept the class across every file: that was the only remaining call. The sweep also found `examples/rag-ollama-demo/README.md` still describing the write as happening "under sudo", and the stale "sudo write" string in the RAG plugin's own error. `writePluginOwnedField` now resolves the field against the config the context carries — `AccessContext` gains `_config` — instead of taking a layout from its caller, so "it reaches no other field" is enforced rather than asserted. A list or field the config does not declare, a context with no config, and `undefined` as the value (which wiped a multi-column field and no-opped a single-column one) are each refused by name. The RAG failure reporter routes those refusals, and `WriteCollectionMissingError`, to the standing-defect path rather than advising a retry that can never work. `Plugin['runtime']`'s `sudo` parameter is declared `StackContext`, which is what it always returned, removing the `as unknown as AccessContext` cast ADR-0066 diagnoses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Deployment failed for project stack-docs with the following error: Learn More: https://vercel.com/open-saas?upgradeToPro=build-rate-limit |
|
Addressed at 1 — ADR-0066's rejection was false, and is correctedYou are right, and I traced it rather than taking it. I also checked the only other transforming field hooks this repo ships: Does correcting it weaken the case? Slightly, and the ADR says that too. What is left of that objection is an application-authored non-idempotent hook, which the framework cannot detect — real, but weaker than a shipped example. What carries the decision instead is a fact I had recorded but not used: carrying the persisted row fixes Also corrected in the same record, from your previous round: the ADR said 2 — the sample, and what the class sweep found
Swept the class — every call of the replaced write, every file, every form ( Beyond the line you named it found three things:
Everything else that matched is a historical record that is correct as history: ADR-0045's own text (superseded by ADR-0066 as to spelling, which 0066 states), and 3 —
|
| Mutation | Fails |
|---|---|
drop the value === undefined guard |
refuses undefined by name rather than wiping the field with it (1 failure) |
ownedField returns a bare { type: 'text' } instead of refusing an undeclared field |
refuses a field the list does not declare, and writes nothing (1) |
writePluginOwnedField ignores splitColumns ({ [fieldName]: value }) |
5 failures, incl. writes the field's own columns past its write denial, clearing with null clears every column the field owns |
remove the isRefusedWrite branch from the reporter |
all 4 new reports a write core refused by name — … — as a standing defect cases |
All restored after.
Gates
| Gate | Result |
|---|---|
pnpm lint |
pass — 0 errors, 2 warnings, both pre-existing |
pnpm build (incl. docs) |
pass — 11/11 |
pnpm format / pnpm manypkg fix |
clean |
packages/core |
pass — 1629 passed / 1 skipped, 83 files (1624 → 1629, +5 new) |
packages/rag |
pass — 468 passed, 20 files (460 → 468, +4 cases × 2 projects) |
packages/cli |
pass — 407 passed, 42 files |
Both code samples I touched (the changeset's runtime snippet and rag-advanced.md's hook) were compiled against the built declarations in a scratch strict project under packages/rag/, harness falsified first: context: null reproduces TS2322: Type 'null' is not assignable to type 'AccessContext', and the old fieldConfig argument reproduces TS2353: … 'fieldConfig' does not exist in type 'PluginOwnedFieldWrite' — which is also what proves the declarations resolve rather than degrading to any. Changesets stay minor.
borisno2
left a comment
There was a problem hiding this comment.
Review — REQUEST CHANGES
Scope: commit 4456be4c only ("fix(core,rag): enforce the plugin write's field, refuse undefined, correct ADR-0066"). Earlier commits were reviewed in the previous two rounds and are not re-reviewed here.
Posted as a Comment review: the GitHub identity available to me is borisno2, the PR's own author, so GitHub refuses a formal REQUEST_CHANGES event. The verdict above is the review's verdict.
Verification performed: typechecked packages/core, packages/rag, packages/auth at 4456be4c (clean); ran packages/rag/src/config/plugin.test.ts (48 passed) and packages/core/src/access/multi-column-read-write.test.ts (33 passed); drove writePluginOwnedField directly against a stub ORM handle to probe the new enforcement; read every field-level resolveInput in the tree.
Blockers
1. The new enforcement is defeated by a prototype-chain key — packages/core/src/context/plugin-field-write.ts:78-96
ownedField resolves with bare index access (config.lists[listName], list.fields[fieldName]), so any key inherited from Object.prototype walks straight past the two === undefined guards. Probed at this commit against a stub handle:
| call | result |
|---|---|
fieldName: 'constructor' on a list whose fields are { label } |
not refused — reaches updateFirst with { constructor: 'PWNED' } |
fieldName: 'toString' |
not refused — reaches updateFirst with { toString: 'PWNED' } |
listName: 'constructor' |
bare TypeError: Cannot read properties of undefined (reading 'label') |
listName: '__proto__' |
same bare TypeError |
Both halves matter, and both land on claims this commit makes:
- The first two rows are exactly "write a column the named field does not own".
list.fields['constructor']resolves to theObjectconstructor, which is notundefined, so it is returned as theFieldConfig; itssplitColumnsisundefined, so the column set falls back to{ [fieldName]: value }and the write is issued. "It reaches no field butfieldName, and that is enforced rather than left to the caller" (plugin-field-write.ts:123), "enforced, since the caller supplies no column layout" (CONTEXT.md:114), "The narrowing is enforced, not asserted" (ADR-0066) and the changeset's "enforced rather than asked of the caller" are all false for these inputs. - The last two rows defeat the error classification this same commit added. A bare
TypeErrorhasname === 'TypeError', soisRefusedWritemisses it andcreateGenerationFailureReporterroutes it to the transient arm — printing "retry by writing the source field again" for a wiring defect that will never clear. That is precisely the misclassification section 5 of this PR exists to fix, reintroduced through the new code path.
This is not a privilege boundary and the docblock is right to say so — a plugin holding ormHandle can write anything. It is a correctness-of-claim defect, and the fix is two lines with an in-repo precedent the RAG plugin itself already uses for the identical hazard (packages/rag/src/config/plugin.ts:136-143, "Object.hasOwn rather than…"), alongside contract/derive.ts:67, config/label.ts:44 and validation/field-names.ts:105:
if (!Object.hasOwn(config.lists, listName)) return refuse(`the config declares no list "${listName}"`)
const list = config.lists[listName]
if (!Object.hasOwn(list.fields, fieldName)) return refuse(`list "${listName}" declares no field "${fieldName}"`)Worth one test row each in multi-column-read-write.test.ts's new block, since the existing 'nowhere' case cannot catch this.
2. docs/content/how-to/rag-advanced.md:76-102 — the rewritten snippet contradicts the caption this commit added directly beneath it
The snippet passes the afterTransaction hook's own destructured context into writePluginOwnedField. The paragraph added sixteen lines below says "context here is the AccessContext Plugin.runtime receives as its first argument". Those are two different objects, and the page's own prose thirty lines above the snippet already describes the real mechanism correctly ("reached by way of a module-private symbol").
The shipped plugin deliberately does not do what the snippet shows: plugin.ts:288 reaches the writer through embeddingWriter(args.context), which only looks up context.plugins.rag — the writer itself closes over the runtime-time context (plugin.ts:506-514). That indirection is load-bearing. For a source write issued inside context.transaction(...), bindContextToTransaction binds ormHandle to the transaction client, plugin runtimes are not re-run (_sharedPlugins), and afterTransaction is drained by the owner registry after that transaction settles — so a plugin author copying this snippet issues the escalated UPDATE on a settled transaction handle, a failure that only appears under context.transaction(). The changeset's own sample (quiet-columns-settle.md:44-47) gets this right and is the model.
Either show the runtime-captured-context indirection, or drop the caption's claim. Given this PR's premise is that the delivery's documentation made false statements about itself, a snippet that contradicts its own caption is in scope.
Verified and handled — not re-raised
_configand the optional-member question. EveryAccessContextcore builds sets it:getContext(context/index.ts:637),bindContextToTransaction(write-pipeline.ts:259, fromargs.config), andfield-visibility.ts:124carries it through its spread. The only consumer refuses on absence.StackBaseContextgenuinely omits it, sosudo()/getContext()return contexts that are refused by name rather than silently proceeding — the "compatibility, not a bypass" distinction holds.- The cast is gone, not relocated. No
as unknown as AccessContextsurvives in production code; the remainder are pre-existing test doubles (includingmulti-column-read-write.test.ts:557, unchanged by this commit).auth'sruntime: (context, sudo)only reachessudo().db, identical on both types;packages/authtypechecks clean. - ADR-0066's correction is factually right, and the decision still holds.
password()'sresolveInputdoes readinputData[fieldKey]and short-circuits onisHashedPassword(fields/index.ts:822-836);file()/image()pass an existing metadata object straight through (storage/src/fields/index.ts:314,:452). The survey is complete in effect — the tree ships exactly four field-levelresolveInputhooks, and the fourth,calendarDay()(fields/index.ts:655), is also idempotent (value instanceof Datereturns unchanged), so the "no shipped field type would corrupt" conclusion stands even though the enumeration omits it. Of the three replacement arguments, the double-firedbeforeOperation/afterOperationand the generation hook's re-entry are decisive and untouched by carrying the row;validateis materially weaker under a whole-row payload than under the one-field one, which the record is honest about. Rejecting the option on what remains is sound. - Error classification. All four refusals set
this.nameexplicitly (three inplugin-field-write.ts,WriteCollectionMissingErroratsecured/write.ts:67), so the name match is real rather than inheriting'Error'. The name-over-instanceofreasoning is right for a duplicatedstack-corecopy, and the newit.eachblock constructs the genuine core classes, so a rename in core fails the rag test rather than silently regressing — the usual weakness of name matching is closed.embeddingWriter's missing-plugin throw sits outside thetry, so it propagates loudly instead of being misreported as transient. The only gap is theTypeErrorin blocker 1. - The sweep.
OwnedFieldLayoutis gone everywhere;writeUnderSudohas no occurrences; no call site retains the four-argument form; no stale "under sudo" description of this write survives inpackages/rag,docs/content/**/rag*,examples/rag-*or.changeset/. I found no fourth. The two remaining occurrences aredocs/adr/0045(immutable record, whose spelling ADR-0066 explicitly supersedes) andspecs/prisma-8/architecture-spec.md:99,196("writing under sudo") — the latter is outside the PR's stated sweep scope and is a historical spec, but it is now the only living document that still describes the mechanism the wrong way. - Changeset.
quiet-columns-settle.mdis@opensaas/stack-core: minor/@opensaas/stack-rag: minor— correct for a new public export plus a behaviour change on both. Body matches the code, including theundefinedrefusal and thesudoretype. - No
any, no new casts in the commit's production code; the three error types follow the repo's<Condition>Error+ explicit.name+ TSDoc-on-the-class convention.
Fix blocker 1, resolve blocker 2 one way or the other, and this is good to merge.
🤖 Generated with Claude Code
`ownedField` resolved the list and the field with a bare index into
config-derived records, so every Object.prototype key walked past both
`=== undefined` guards. `fieldName: 'constructor'` was not refused: the
lookup returned the Object constructor, whose `splitColumns` is
undefined, so the column set fell back to `{ constructor: value }` and
the write was issued — the write of a column the named field does not
own, which is the thing the docblock, ADR-0066 and the changeset all
claim is enforced. `listName: 'constructor'` threw a bare TypeError,
whose name is not one of the refusals, so it missed `isRefusedWrite` and
was reported through the transient arm with "retry by writing the source
field again" — the misclassification this PR exists to fix, reintroduced
through the new code path.
Both lookups are now gated by `Object.hasOwn`, as `ragPlugin`'s
`providerIsDeclared` already does for the identical hazard. Six cases
cover it, three per lookup, asserting the refusal by name — the name
being what routes it to a consumer's standing arm — and that the row is
untouched. A rag case pins the other half: a bare TypeError is reported
transient and always would be, so core refusing by name is the only
thing keeping a wiring defect out of that arm.
`rag-advanced.md`'s snippet passed the `afterTransaction` hook's own
context to `writePluginOwnedField`, contradicting the caption beneath it
and the plugin that ships. It now shows the indirection: the writer
closes over the context `runtime` was handed, and the hook uses its own
context only to look that writer up. The indirection is load-bearing —
inside `context.transaction(...)` the hook's context is bound to the
transaction client and `afterTransaction` drains after it settles, so
the collapsed form writes on a settled handle — and the prose now says
so, since a reader who does not know that will simplify it back.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Addressed at 1 — the prototype-chain key, reproduced then closedI reproduced your probe first, at
The field rows confirm your reading exactly: the column name reaches Both lookups are now gated by const list = Object.hasOwn(config.lists, listName) ? config.lists[listName] : undefined
if (list === undefined) return refuse(`the config declares no list "${listName}"`)
const field: FieldConfig | undefined = Object.hasOwn(list.fields, fieldName)
? list.fields[fieldName]
: undefined
if (field === undefined) return refuse(`list "${listName}" declares no field "${fieldName}"`)Gating the existing guard rather than adding a second one beside it, so there is one refusal and one message per lookup, and an explicitly- Tests. Two The routing half needed one more test than that, because the core assertion only proves the name is right. Mutations (each applied alone, restored after):
Class sweep — bare index access into a config-derived record. Swept every added line under
One adjacent instance worth recording, pre-existing and not touched here: 2 — the snippet now shows what ships
The caption is rewritten to say why rather than just what, since the failure is invisible until someone uses Compile. The corrected snippet compiles clean against the built declarations in a scratch
The first two also prove the declarations resolve rather than degrading to Not touched
Changeset
Gates
🤖 Generated with Claude Code |
borisno2
left a comment
There was a problem hiding this comment.
Code review — commit 7a05f357 only (/code-review, effort high)
Verdict: APPROVE. No blockers. Three follow-ups below, none of them introduced by this commit.
Posted as a Comment review rather than a formal Approve event: the reviewing GitHub identity is borisno2, the PR's own author, and GitHub refuses a formal review event on your own PR.
Scope is 7a05f357 (fix(core,rag): gate the plugin write's lookups with Object.hasOwn). Earlier commits were reviewed three times and are re-examined only where this one changes their meaning.
1. The gate — verified by reproduction, not by report
Both lookups in ownedField are gated, and I tried to defeat them again rather than trusting the six cases in the diff. Probing twelve Object.prototype names — constructor, toString, toLocaleString, valueOf, hasOwnProperty, isPrototypeOf, propertyIsEnumerable, __proto__, __defineGetter__, __defineSetter__, __lookupGetter__, __lookupSetter__ — against both the list lookup and the field lookup, over a real PGlite database: all 24 refuse with UnknownPluginFieldWriteError, and all 24 return true from packages/rag's isRefusedWrite, i.e. every one routes to the standing-defect arm rather than "retry by writing the source field again". The gate is exact and the classification is by name, as claimed.
The commit message's account of the pre-fix behaviour checks out, including the part the previous review missed. Reverting each gate independently:
- field gate removed → the three field cases fail with
StructuredError: Unknown column "function…"/Unknown column "[object …". So the fallback{ [fieldName]: value }did issue the write, and what came back was the database's own error type —StructuredError, which is not one of the four names inREFUSED_WRITE_ERRORS. Those three cases were therefore misclassified as transient too, not merely wrong writes. Six, not four, confirmed. - list gate removed → the three list cases fail with a bare
TypeError, also unclassifiable.
The tests genuinely discriminate. I reproduced two mutations rather than trusting the report: dropping the field gate fails exactly the three field cases (36/39 pass); dropping the list gate fails exactly the three list cases. And the new rag case discriminates in the other direction — adding 'TypeError' to REFUSED_WRITE_ERRORS fails reports a bare TypeError as transient, which is why core refuses by name and nothing else. Suites green unmutated: core multi-column-read-write.test.ts 39/39, rag plugin.test.ts 49/49.
2. The docs snippet — accurate, and the explanation is load-bearing for the right reason
Every claim in the rewritten prose is verified against the shipping code, not just against the plugin:
getContextskips plugin runtimes when_sharedPluginsis passed (context/index.ts:644), and the transaction rebind passescontext.plugins(context/index.ts:1049). So the writer found on a transaction-bound context is still the one closing over the runtime-time context. ✅- The transaction child context is built over
opened.ormHandle— the transaction client — andrunAfterTransactionForListis invoked from a closure capturing that context (transaction-boundary.ts:335-339), drained bysettleTransactionOwneronly afterawait settled. So the collapsed form would issue the escalatedUPDATEon a settled handle. ✅ getContext's returnedStackContextcarries noormHandle. ✅
The snippet now matches what ships (await write(listName, item.id, fieldName, {…}), writer closing over the runtime context). A reader following it would not write on a settled handle.
3. Gates
No any, no casts, no non-null assertions in the added production code. Changeset is accurate — the new clause about inherited keys matches the shipped behaviour — and minor is right for both packages (this changeset publishes writePluginOwnedField as a new @opensaas/stack-core/extend export).
Follow-ups (none blocking this commit)
F1 — the absence still matters, for exactly one shape: virtual(). (packages/core/src/context/plugin-field-write.ts:154, from 85007129)
The task's question — does the database error type's absence from the refusal list still matter anywhere else — has one surviving answer, and I reproduced it rather than reasoning about it. A declared virtual() field passes both Object.hasOwn gates, has no splitColumns, falls to { [fieldName]: value }, and issues an UPDATE naming a column that does not exist:
DECLARED PROBE shout: StructuredError | isRefusedWrite=false | Unknown column "shout" in table "Owned"
isRefusedWrite is false, so the RAG reporter tells the reader to "retry by writing the source field again" — the precise misclassification this commit exists to eliminate, for a wiring defect that fails identically on every row. Impact is a misleading log, not data loss. The fix is one more refusal in ownedField: a field with no splitColumns and no scalar column of its own name is a mistake the docblock already claims to refuse.
I also probed relationship() on suspicion it had the same shape. It does not — writePluginOwnedField({ fieldName: 'owner', value: otherId }) correctly moves ownerId from null to the target id. Recording that so nobody chases it.
F2 — embeddingWriter sits outside the try it is classified by. (packages/rag/src/config/plugin.ts:288, from 85007129)
This commit's own framing is "core refusing by name is the only thing keeping a wiring defect out of that arm" — which is what makes this worth naming: there is one wiring defect that never reaches the arm at all. const write = embeddingWriter(args.context) is one line above the try, and embeddingWriter throws a plain Error when context.plugins.rag is absent. That throw propagates out of afterTransaction, is collected by runAfterTransactionForList, and is rethrown as AfterTransactionError from the caller's create/update — failing a write whose row already committed, which directly contradicts the hook's own Known-limits block ("a provider failure is logged, not thrown … reporting it as a failure would invite a retry that duplicates the row"). Reachable when getContext catches a throw from any plugin's runtime (context/index.ts:652 logs and continues, leaving plugins.rag unset). Low reachability, wrong contract when reached. Move the lookup inside the try.
F3 — nit: the third lookup on the same call chain is still ungated. writeCollection(context.ormHandle, listName) indexes ormHandle[listName] bare (secured/write.ts:72). It is currently unreachable via this path — listName is now constrained to declared lists, and isWriteCollection's typeof === 'object' catches function-valued prototype keys into WriteCollectionMissingError, which is in the refusal set. Defence-in-depth only.
(One thing I checked and am not raising: Object.hasOwn(config.lists, …) throwing on a config with no lists. OpenSaasConfig.lists and ListConfig.fields are both non-optional, so a typed caller cannot reach it.)
Nothing in this commit introduces a fresh instance of the class it fixes. The one lookup it left ungated (F3) is closed by the gate it added.
The base moved again during verification (#1332). Three conflicts, all purely additive: `_config` and `_rowLock` are separate optional members that each side appended to `AccessContext`, to `getContext`'s context literal and to `bindContextToTransaction`'s. Both are kept in each. `deriveResolveOutputContext` needed nothing — it spreads the context, so it carries both new members already. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-junction-edge-create Brings in #1332, which appends an optional `_config` member to the shared `AccessContext` type and to the context literals in `getContext` and `bindContextToTransaction`. This branch edits a different region of `packages/core/src/context/index.ts` (`ServerActionProps` and the `addRelated` handler), so git resolves both sides without a textual conflict; the merged tree is verified to carry `_config` on every path that builds a context. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The check's fixtures are keyed `file:line`, and #1318 (de80685) and #1332 (3138401) rewrote the documents it reads. Merging origin/prisma-8 shifted every line in rag-advanced.md and packages/rag/CLAUDE.md, and brought in .changeset/swift-pandas-listen.md, which the check had been skipping because the file did not yet exist on this branch. The orphan arm did its job: 14 of the 27 keys matched no block and were reported, so no shifted key silently excused a different block. The 13 that still matched were re-read against their current diagnostics rather than assumed, and each still describes the block it lands on. Every entry here is re-derived from the block's actual diagnostic. Two classifications crossed over in rag-advanced.md and would have been wrong had the keys been moved mechanically: the block now at 523 is one entry of a `lists:` object, not the `lists:` property its old key described, and the block now at 880 is the `lists:` property, not the `fields:` one. Three more reasons named things that are no longer true — the plugin hook at 66 closes over `writePluginOwnedField` and `embeddingWriter`, not `writeUnderSudo`; the `stored.vector` failure at CLAUDE.md:546 is a `{}` narrowed from `unknown`, not `unknown` itself; and the `NearestMatch` mismatch at 910 is stated as the diagnostic reports it. The block redeclaring `EmbeddingProvider` with an optional `embedBatch` no longer fails: #1318 made it required, so it now agrees with the shipped type and needs no entry. 97 blocks: 70 compile, 27 classified fragments, 2 redeclare an exported type and are checked against it. No failures, none stale, none orphaned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The RAG work in #1128 was built and documented while the secured write surface could not execute on the Prisma 8 collection. It executes now — #1281 landed the write pipeline — so the delivery's own statements about itself were inverted: for three rounds the docs oversold, and this round they undersold the one thing the spec exists to deliver.
Confirming the premise first
I did not take the write surface from a commit message. On the Test context (PGlite + pgvector,
createTestDatabase, a pure-function fake provider) I drove the real surface and then deleted the probe:context.db.Note.create/update/deletetitle: 'after', then the row is gonecontext.db.Article.create({ data: { content: 'red' } }){"vector":[1,0,0],"metadata":{…,"sourceHash":"2f0x"}}content'red'→'blue'vector:[0,1,0], newsourceHash— the re-entry gate does not wrongly short-circuit.nearest('contentEmbedding', [1,0,0])over plugin-written vectorsred@1.00 reddish@0.80 blue@0.00One probe failed first and was informative:
Notewith no declared operation access returnednullfromcreate, which is the documented silent failure, not a broken surface. With access declared it executes.And the delivery's own
the plugin's sudo write reaches the column— written to skip itself by name — now runs and passes.packages/ragreports 456 passed / 0 skipped.1 — The four claims that said the feature was inert
.changeset/loud-comets-invent.md— the sharpest, because it is pending and publishes as the release note. "Embedding generation does not run in this release… the write throws on every invocation… Semantic search over that field returns nothing… There is no config change that works around it." Replaced with what generation actually does, including thesourceHashshort-circuit..changeset/silver-moths-gather.md— the dimension-change recipe's "the plugin's write is inert on this branch, so nothing regenerates yet". Probe 2 regenerates on re-save.packages/rag/CLAUDE.md:363-365— the generation hook'sKnown limits.packages/rag/CLAUDE.md:633-636— "On theprisma-8branch that re-save regenerates nothing".allowManualWritessweep. Nine sites across.changeset/,packages/rag/CLAUDE.md,docs/content/how-to/rag-advanced.mdanddocs/content/reference/rag.md. Exactly one offered it as a way around the broken write path —loud-comets-invent.md's "If you need vectors before then, useembedding({ allowManualWrites: true })and write them yourself" — and it is gone. The other eight already frame it as the deliberate opt-out for an app that maintains its own vectors; unchanged.Also corrected, same class: the
Known limitsentry "a nested record is never embedded". ADR-0050 made a nested spelling under a relationship key aNestedRelationInputError, verified againstsecured-write.test.ts's eight-spelling table — the row that limit describes cannot be created at all, so the hook's warning is a backstop rather than something a write reaches.2 — The accommodations
isUnportedWriteSurfaceremoved, with the reporter branch it gated. It matchedfindUnique is not a function/Unknown column "data";findUniqueis now a collection member (context/index.ts:1126), and the branch was shipping aconsole.errorreadingEMBEDDING GENERATION IS NOT RUNNING … No config change works around it; track #1127to a user.plugin.test.ts'sreports a write that cannot execute as the standing defect it isremoved — it manufacturednew TypeError('model.findUnique is not a function')and was green coverage of an unreachable branch.#1124/#1127comment inplugin.ts, and two further#1127comments inplugin.test.ts(one of which pinned an assertion "until Spec: MCP and the admin UI on the secured surface #1127 lands").expect(said).not.toContain('#1127')no longer discriminates against anything.Sweep, re-run and widened.
grep -rn "1124\|#1127"over every.md/.ts/.tsxoutsidenode_modules,distandspecs/now returns zero hits. The two it previously left —docs/content/how-to/write-a-plugin.mdanddocs/content/reference/context-api.md, saying a hook's database work is not rolled back — were flagged as unsettleable on a mis-probe:_transactionOpeneris set on the internalAccessContextand deliberately omitted from the returnedStackContext, so its absence there proved nothing. They are settled and false, and corrected. Keying the sweep on #1205 as well found two more instances the first pass could not see: the pending.changeset/humble-handles-narrow.mdandCONTEXT.md's ORM handle entry. A committed test now pins it: anafterOperationwriting throughormHandleand then throwing leaves no row, with a control confirming it lands without the throw. Noctx.skipand no#1124/#1127Known limitsblock survives inpackages/ragorpackages/core.3 — Meeting the PRD's testing decision (#1272)
The PRD says a good test "seeds rows with real vectors through the secured surface". Every re-pointed suite now does.
embedding-write.test.ts— thewrite denialblock drivescontext.db.Article.create()/.update()instead ofhookPipeline. Rows are seeded bywriteSource(content), which writes source text and lets the plugin produce the vector.allowManualWritesmoved to the DB-backed describe, so it is asserted by reading the columns back rather than by inspecting resolved data. The sudo write'stry/catch+ctx.skipis gone.search.test.ts—seedPalette/seedAxiswrite source text throughcontext.db; the off-surfaceseed()helper and itswithOrigin('unsafe')import are gone.multi-column-read-write.test.ts— a new DB-backed describe drives the write-access gate throughcontext.dbover a field with a real contract (two columns), covering denied update, denied create, granted write,sudo()bypass, an ungated field and anullclear.plugin.test.ts'sembeds a source value a list-level resolveInput producedre-pointed intoembedding-write.test.tsasthe embedded text is the persisted source, not the caller's input, against a real row.hookPipelineun-exported from@opensaas/stack-core/internal— nothing depends on it now (core's own tests import it by relative path). The export is unreleased (c0d443fbpost-dates the lastVersion Packagestouchingpackages/core/CHANGELOG.md), so the paragraph announcing it was removed from.changeset/brisk-columns-refuse.mdrather than left to publish a lie.Two rows kept off the surface, deliberately and commented: metadata-present-with-no-vector, which the generation hook cannot produce (it writes both columns or neither).
Mutations — each re-pointed test broken on purpose, restored after
splitMultiColumnFields' denial →if (!canWrite && false)an ordinary create naming the embedding throws,an ordinary update naming the embedding throws,a create the operation gate admits throws; coreTHROWS when update access is denied,THROWS when create access is deniedfilterWritableFields' split-column-owner throw disabledthe write the throw refused never reaches the columnsthe two columns read back as one stored embedding,nearest ranks by the column the field declares,the embedded text is the persisted source, not the caller's input,the write the plugin owns still lands; 11 of 13search.test.tstests — 15 failures, not 14allowManualWritesignored (field always denies)allowManualWrites lets the same payload reach the columnssplitColumns' output not merged into the writeTHROWS when update access is denied,writes both per-part columns,sudo bypasses the gate,a field WITHOUT field-level access writesnullvalue dropped instead of splitclearing the field with null clears both columnsThe third row is the one that matters for the PRD's decision: under the old
withOrigin('unsafe')seeding, breaking generation moved nothing. It now takes down almost the whole search suite. The two survivors there assert the absence of an embedding and a missing id, correctly.Two mutation results worth recording rather than hiding:
the write the throw refused never reaches the columnsgreen, becausefilterWritableFieldsis a second, defence-in-depth gate that throws a differently-worded message. The exact-message assertions catch it; that test's substring does not. Disabling both gates kills it.Derivedfixture surfaced a live data-corruption bug, since fixed rather than guarded — see section 5.4 — The null dereference (
packages/rag/CLAUDE.md:541)const article = await …first(); const queryVector = article.contentEmbedding.vector—TS18047, and aTypeErrorat runtime whenever the Access Filter denies the row, which is the silent-failure contract the rootCLAUDE.mdmakes a named Critical Pattern.Harness, falsified before trusting it. A
stricttscproject rooted insidepackages/rag/so@opensaas/stack-*resolve through the workspace to the built declarations. The original block reproducesTS18047: 'article' is possibly 'null'exactly; the corrected block is clean; injecting the dereference back into the corrected block reproducesTS18049. Checked against the package's real exportedStoredEmbeddingrather than a local copy of it — the guard narrowsstored.vectortonumber[], which is what proves the declarations resolve rather than degrading toany.Sweep.
.first()/findUnique(/findFirst(across the whole RAG documentation surface:packages/rag/CLAUDE.md:540is the only one, so within scope the class has one member and it is fixed.Beyond scope, the same sweep over every markdown file in the repo found one more in a pending changeset —
.changeset/nine-otters-describe.md:32,const article = await …findFirst(); article.body— fixed here and compile-checked in the same harness, because it publishes.It also found the class is widespread on the docs site, in files this delivery does not own:
docs/content/concepts/field-types.md:632,docs/content/reference/fields-api.md:1663and:1718all dereference afindUniqueresult directly, and several more bind without guarding.SECURITY.md:177,docs/content/concepts/access-control.md:167anddocs/content/reference/context-api.md:859get it right and are the model. Not fixed here — it is a docs-site sweep of its own, and worth a ticket.5 — The silent data corruption, fixed (ADR-0066)
The
Derivedfixture was defending itself against a live bug, and the defence hid it. Removed, and fixed at the source.The corruption, run. The plugin's escalated write was
sudo().db.<list>.update()— an ordinary secured write, so it re-ran the list's whole hook pipeline carrying the embedding column and nothing else. A list-levelresolveInputderiving one field from other input, the pattern the rootCLAUDE.mddocuments, then recomputed that field from values that were not there.contentaftercreate({ data: { title: 'red', body: 'hot' } })afterOperationcalls' '— the join of twoundefineds, and the embedding is of' '['after', 'after']'red hot', vector[0,0,1],sourceHash: 'hvoym6'['after']Silent in both places: no error, and the stored vector is the vector of the destroyed text. Unreachable before #1281 made the write execute.
The mechanism, and why this one. Core now owns the write as a single field:
writePluginOwnedFieldon@opensaas/stack-core/extendsplits the value through the field's ownsplitColumnsexactly as the Write Pipeline does, issues one id-scopedUPDATEmarked with the engine origin, and runs no hook. It reaches no other field — a narrower capability than the escalateddbupdate it replaces, which could write any column on the row.The write is not an application update. It carries one column and completes a write the application already made, whose hooks have already run against the caller's real input; running them again over a payload naming one field lies to every one of them. Not only
resolveInput:validatesees a mostly-absent record, the side-effect hooks double-fire, and the generation hook's ownafterTransactionre-entered itself — which only the storedsourceHashwas stopping. Re-entry is now structural rather than guarded.Carrying the persisted row was considered and rejected, with a concrete reason: a field-level
resolveInputthat transforms rather than derives would re-transform its own stored output —password()'s hook hashesresolvedData[fieldKey], so handing back the stored hash double-hashes it. That trades one silent corruption for another. The other options and why they lost are in ADR-0066.One wiring correction fell out.
sudo()returns the publicStackContext, which omitsormHandle— it reachedPlugin.runtimethrough anas unknown as AccessContextcast, and a write over it found nothing. The write takes theAccessContextruntimereceives as its first argument; a context with no handle is refused by name. The RAG plugin'sruntimeno longer takessudoat all.The tests, and the mutation
The fixture's guard is gone;
Derivednow carries the derivation an application would write. Three rag tests and six core tests pin the contract.sudo().db.<list>.update()the embedded text is the persisted source, not the caller's input(expected ' ' to be 'red hot'),the generation write leaves the derived source it did not name intact(same),the generation write fires no second afterOperation(['after','after']vs['after'])writePluginOwnedFieldignoressplitColumnswrites the field's own columns past its write denial,leaves a derived field the list's resolveInput owns untouched,fires no list hook of its own,clearing with null clears every column the field ownsBoth restored after.
Gates
pnpm lintexamples/blog/test-singleton.ts,packages/cli/src/migration/generators/migration-generator.ts)pnpm build(incl. the docs build)pnpm manypkg fix/pnpm formatpackages/ragpackages/corepackages/cliCore's suite gained 13 tests and rag's skip count went 2 → 0.
Out of scope, untouched
#1310 (storage/tiptap test coverage), #1311 and #1304 (the ollama example's seed script, its lowercase
context.dbkey, the hand-rolled cosine scan, the suite running twice fromsrc/anddist/), and #1265, #1271, #1278, #1282, #1283, #1288, #1294, #1297, #1298, #1299, #1301, #1303, #1306, #1307, #1308, #1309, #1313, #1316, #1317, #1319, #1320, #1321, #1322. PR #1324 is untouched.Refs #1128, #1272.
🤖 Generated with Claude Code