Skip to content

Make a ref guard deletion, on every path into the record - #6

Merged
CraftOS-dev merged 5 commits into
mainfrom
korivi-delete-guard
Sep 17, 2026
Merged

CraftOS-dev merged 5 commits into
mainfrom
korivi-delete-guard

Conversation

@korivi-CraftOS

@korivi-CraftOS korivi-CraftOS commented Sep 15, 2026 •

Copy link
Copy Markdown
Collaborator

The hole

An app that refuses to delete a client who still has invoices has guarded one way in: its own UI, through its own operation. The generic record route is the other:

a2app <app> data clients delete c_acme     → 200 OK

The row is gone and every invoice still points at it. The delete reported success.

The author cannot close this themselves — record deletion is adapter code, which toolkit-sync owns and overwrites — so the only move available was to write the hole down as a known limitation.

The fix

The check moves to where both doors pass through. Nothing new is asked of the app: a ref or list<ref> already names the entity it points at, and that is the entire declaration needed.

a2app <app> data clients delete c_acme
→ 409 record_referenced
  Cannot delete clients "c_acme": a record still references it (invoices.client).
  referencedBy: [{ entity: "invoices", field: "client", ids: ["inv_1"] }]
  resolution:   Remove or repoint the referencing records first, or run an
                operation the app provides for this.

A refusal with no location is a wall, not an answer — so it names the entity, the field, the blocking records, and what would make the delete legal.

This changes behaviour, deliberately

A delete that used to succeed and orphan now returns 409. That is the fix, not a side effect of it. Orphaning silently is invisible at the moment it happens and surfaces later, somewhere else, as a record pointing at nothing.

An app that genuinely means for references to outlive the record says so in one word:

{ name: "client", type: "ref", entity: "clients", onDelete: "ignore" }

There is deliberately no cascade or detach. Both would let one delete write to records the caller never named — which an agent cannot approve up front and an audit log cannot explain afterwards. An app that wants that declares an operation, where it is named, described and approved like any other write.

Verified against a real app, not just unit tests

Scaffolded a fresh app from blueprint-react-node — the blueprint whose server.mjs the issue names — with exactly the model described: clients, and invoices with a ref to them, plus a delete-client operation carrying the app's own rule.

$ a2app ./billing data clients delete c_acme
✗ Cannot delete clients "c_acme": a record still references it (invoices.client).
exit=1

Before this change that command returned 200 and the invoice was left pointing at nothing.

And nothing was half-done:

client:  {"id":"c_acme","name":"Acme Corp"}
invoice: {"id":"inv_1","total":4200,"client":"c_acme"}

The rest of the flow is unaffected: the app's own delete-client operation still behaves as it did (approval gate intact), and once the invoice is removed the client deletes normally.

Scope — all three adapters

Adapter Status
adapter-core (TypeScript) ✅ fixed, verified against a running app
FastAPI blueprint (Python) ✅ fixed, covered by the adapter self-test
PocketBase blueprint ✅ fixed — but not runtime-verified, see below

PocketBase

It serves records natively and guards writes by hooking the request. Create and update were hooked; delete never was. onRecordDeleteRequest now refuses the same case and returns the same record_referenced shape, so an agent meets one answer whatever stack it is talking to.

It fails open on its own errors, deliberately. This hook cannot be executed here — the blueprint's install step prints "download the pocketbase binary", so there is no runtime in CI and the gate only parses the file. Untestable code that failed closed could brick every delete in an app; failing open means the worst case is the behaviour that already existed. It still needs a run against a real PocketBase before anyone calls it verified.

The part that can be tested was moved out of the hook: referencingFields lives in the shared rules file and is covered by the gate's self-test. Only the query is PocketBase's.

A second bug, found on the way

The PocketBase adapter duplicates mapFields/normalize inline — the header explains why (pooled Goja handlers cannot close over file scope) — and the copies had drifted. The describe handler's copy carries a relation's target entity; the identity handler's copy did not, and that is the copy schemaVersion is computed from.

So retargeting a ref at a different collection changed what describe published without moving the fingerprint, and a client caching against it kept a describe pointing the ref at the old collection. The fingerprint's own contract names that case explicitly. Both copies now carry the target.

Original scope note (superseded)

expand

two of three adapters

Adapter Owns its delete path? Status
adapter-core (TypeScript) yes ✅ fixed
FastAPI blueprint (Python) yes — identical hole ✅ fixed
PocketBase blueprint serves records natively ❌ still open

PocketBase is not fixed. Its hooks cover onRecordCreateRequest and onRecordUpdateRequest but not delete, so the same hole is open there and the fix belongs in an onRecordDeleteRequest hook. I did not write it: that file is system-owned and hash-locked, there is no PocketBase runtime in CI to execute it against, and a guard that errs toward refusing would break every delete in the app. It needs someone who can run it.

Each adapter scans by paging rather than pushing a filter down — filter grammar is per-backend, and a policy that silently did nothing against one of them would be worse than no policy. Only entities that actually declare a restricting ref are read, and the scan reports examples rather than every blocking row.

Verification

Conformance had no DELETE coverage at all — part of why this survived. It now has 7 checks, self-contained so they depend on neither seed data nor ordering: the refusal, what it names, that the record is still there afterwards, that removing the reference removes the block, and that a missing record is still 404 rather than 409.

The new adapter-core unit test fails 17 ways against the previous behaviour, including the one that matters:

a referenced client is not deleted    expected: 409   actual: 200
the client is still there             expected: 200   actual: 404   ← it was deleted

The Python self-test covers the same scan and runs inside the scaffolded app (✓ python syntax + adapter self-test).

Before After
Conformance 57/60 64/67
pnpm -r build / typecheck clean clean
Python SDK pass pass

Note on the 3 failing conformance checks

blueprint-base, blueprint-python-fastapi and blueprint-pocketbase-react fail scaffold → canon → validate on main as well — verified against a clean checkout. The reason is unrelated to this change: those templates ship reference/requirements.md with 43 unfilled <REPLACE: …> placeholders, which validate rejects. blueprint-react-node passes.

Follow-up worth considering

describe does not publish onDelete, so an agent planning five deletes still learns about the constraint by being refused. Publishing it (and folding it into schemaFingerprint) would let the agent plan correctly the first time. Left out here to keep this focused on closing the hole.

An app that refuses to delete a client who still has invoices has guarded one
way in: its own UI, through its own operation. The generic record route is the
other — `a2app <app> data clients delete <id>` — and it went straight to the
store. So the rule held right up until an agent took the path the rule did not
cover, and the orphan it left behind was reported as a successful delete.

The author cannot close that themselves. Record deletion is adapter code, which
toolkit-sync owns and overwrites, so the only move available was to write the
hole down as a known limitation.

So the check moves to where both doors pass through. Nothing new is asked of the
app: a `ref` or `list<ref>` ALREADY names the entity it points at, which is the
whole declaration needed. Deleting a record something still points at is now
refused with `record_referenced`, naming the entity, the field and the records
in the way, plus what would make the delete legal — a refusal with no location
is a wall, not an answer.

DEFAULT IS TO REFUSE. This changes behaviour for existing apps: a delete that
used to succeed and orphan now returns 409. That is the fix, not a side effect
of it. Orphaning silently is invisible at the moment it happens and surfaces
later, somewhere else, as a record pointing at nothing — and an app that means
for references to outlive the record now says so in one word:

    { name: "client", type: "ref", entity: "clients", onDelete: "ignore" }

There is deliberately no `cascade` or `detach`. Both would let one delete write
to records the caller never named, which an agent cannot approve up front and an
audit log cannot explain afterwards. An app that wants that declares an
operation, where it is named, described and approved like any other write.

Fixed in both adapters that own their delete path — adapter-core (TypeScript)
and the FastAPI blueprint's Python adapter, which had the identical hole. Each
scans by paging rather than by pushing a filter down, because filter grammar is
per-backend and a policy that silently did nothing on one of them would be worse
than no policy; only entities that actually declare a restricting ref are read,
and the scan reports examples rather than every blocking row.

Conformance had no DELETE coverage at all, which is part of why this survived.
It now has seven checks, self-contained so they do not depend on seed data or
ordering: the refusal, what it names, that the record is still there afterwards,
that removing the reference removes the block, and that a missing record is
still 404 rather than 409. Adapter-core gains a unit test that fails 17 ways
against the previous behaviour, and the Python self-test covers the same scan.

NOT fixed: the PocketBase blueprint. It serves records natively and its hooks
cover create and update but not delete, so the same hole is open there and the
fix belongs in an `onRecordDeleteRequest` hook. Shipping runtime code into that
hash-locked file without a PocketBase runtime to execute it against would be
guessing, and a guard that errs toward refusing would break every delete in the
app.
PocketBase serves records natively, so this adapter guards writes by hooking the
request — and delete was never hooked. Create and update were; delete went
straight through. Same hole as the other two adapters, one layer down.

`onRecordDeleteRequest` now refuses a delete whose record other records still
point at, with the same `record_referenced` shape the other adapters return, so
an agent meets one answer whatever stack it is talking to. The scan asks
PocketBase for the relation targets rather than being told them again, and the
record id travels as a bound filter parameter rather than spliced into a filter
string.

IT FAILS OPEN ON ITS OWN ERRORS, deliberately. This hook cannot be executed
here: the blueprint's install step prints "download the pocketbase binary", so
there is no runtime in CI, and the gate only parses the file. Untestable code
that fails CLOSED could brick every delete in an app. Failing open means the
worst case is the behaviour that already existed, never an app that cannot
delete anything. It still needs a run against a real PocketBase before anyone
should call it verified.

The decision itself is testable, so it lives in the shared rules file as
`referencingFields` and is covered by the gate's self-test — which fields point
at this entity, scalar and list alike, and which opt out. Only the query is
PocketBase's.

Also fixes what hid this. The header warns that pooled Goja handlers cannot
close over file scope, so `mapFields`/`normalize` are duplicated inline — and
the copies had drifted. The describe handler's copy carries a relation's target
entity; the identity handler's copy did not, and that is the copy `schemaVersion`
is computed from. So retargeting a `ref` at a different collection changed what
describe published without moving the fingerprint, and a client caching against
it kept a describe pointing the ref at the old collection. The fingerprint's own
contract names that case. Both copies now carry the target.

Also corrects the message: one blocker read "a record still reference it".
@korivi-CraftOS

Copy link
Copy Markdown
Collaborator Author

Stacked on #7 so this PR shows a truthful green.

The red here was never this change: it was the three blueprint failures that have been red on main for five commits, which #7 fixes. Rather than leave a passing change looking broken, #7's branch is merged in — so CI now runs this fix against a suite that is green to begin with.

Total: 67/67 passed, 0 failed.

That is 60 from #7's corrected suite plus the 7 delete checks this PR adds.

Merge #7 first, and the extra commits disappear from this diff on their own, leaving only the delete guard. Nothing here needs to change.

Restores `reference/requirements.md` and `reference/tasks.md` in
blueprint-python-fastapi to the template as shipped, placeholders intact.
Merging this overwrites what reached main.

Those files were authored to make a red suite green. Wrong call: they are the
blueprint's own spec, and a starter that hands its author a pre-written one has
decided for them what the app is. The template is unfilled on purpose.

The suite no longer asks for one. blueprint-python-fastapi joins the other two
starting points, where `validate` refusing an unauthored spec is `validate`
working — the same reason already recorded for base and pocketbase-react. Its
runtime and model are complete; only the spec is deliberately not.

Conformance stays 64/64, now by stating what the blueprints are rather than by
supplying what they were missing.
…adapter

main moved the PocketBase adapter under it. Three things changed that this
branch had to be rewritten against, not merged around:

- Hook isolation. Handlers are now self-contained one-liners in `_a2app.pb.js`
  that `require()` `_a2app_impl.js` inside their own body, because PocketBase
  serializes each handler into an isolated runtime where file scope does not
  exist. The delete guard registration follows that shape, and its body moved
  into the impl module with the others.
- The 0.23+/0.26 API. `$app.dao()` is gone: collections come from
  `$app.findAllCollections`, fields from `col.fields.asMap()` with `f.type()` as
  a call, and a relation's target is `f.collectionId` rather than
  `f.options.collectionId`. This branch had nine `$app.dao()` calls; none remain.
- `_a2app.pb.js` was rewritten wholesale, so main's version is taken entirely
  and the guard re-applied on top rather than merged line by line.

The drift fix is re-applied where it now lives. `describe`'s field mapping
carries a relation's target and identity's copy still did not — and identity's
is the copy `schemaVersion` is built from, so retargeting a ref at a different
collection changed what describe published without moving the fingerprint. The
restructure did not close that; it moved it.

`referencingFields` and its self-test survived the merge untouched, so the
decision the guard makes is still covered by the gate. The guard still fails
open: the hook remains unexecutable here, and code that cannot be run must not
be able to brick every delete in an app.

Also resolved: adapter-core's test script, which both sides had appended to, and
its constants block, where neither side replaced the other. whoami and the
delete guard both survive — 80/80 conformance, both unit suites passing.
@CraftOS-dev
CraftOS-dev merged commit 94e46a2 into main Sep 17, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants