Skip to content

refactor(trigger-record-change): drop the ctx.__previous stash fallback, dead since #6656 (#6978 Item 1) - #7081

Merged
os-project-manager merged 1 commit into
mainfrom
claude/issue-6978-previous-stash-dead-limb
Aug 9, 2026
Merged

refactor(trigger-record-change): drop the ctx.__previous stash fallback, dead since #6656 (#6978 Item 1)#7081
os-project-manager merged 1 commit into
mainfrom
claude/issue-6978-previous-stash-dead-limb

Conversation

@os-project-manager

Copy link
Copy Markdown
Collaborator

Part of #6978

Item 1 only. The card carries two items; this PR delivers Item 1 (the dead
ctx.__previous fallback limb and the fixture that fed it, both in
packages/triggers/trigger-record-change). Item 2 — the four stale
captureBefore comments in packages/objectql, packages/metadata and
packages/spec — is deliberately untouched here and awaits the triage split
recorded in the claim comment: those files belong to domain:engine-core,
domain:metadata and the hard-owned spec seat respectively. So Part of, not
Fixes — the card stays open for Item 2.

What changed

packages/triggers/trigger-record-change/src/record-change-trigger.ts,
buildContext:

// before
const previous =
    (ctx.previous as Record< string, unknown > | undefined) ??
    ((ctx as unknown as { __previous?: Record< string, unknown > }).__previous ?? undefined);

// after
const previous = ctx.previous as Record< string, unknown > | undefined;

…plus the doc-comment sentence that described the stash, and the test case that
synthesised the key. A changeset is included (@objectstack/trigger-record-change,
patch) — see "Changeset, not skip-changeset" below.

Premise re-measured on today's origin/main

The card's census was taken on #6977's branch. Re-run on origin/main at
2c7e62d5 (2026-08-09), the grep pair still answers the same way — no new
producer appeared:

$ git grep -n "__previous" origin/main
.changeset/audit-consume-bound-previous.md:9          prose
.changeset/audit-consume-bound-previous.md:27         prose
packages/plugins/plugin-audit/src/audit-bound-previous.test.ts:319   comment
packages/plugins/plugin-audit/src/audit-writers.ts:693               comment
packages/runtime/src/sandbox/quickjs-runner.ts:494    setGlobalJson(vm, '__previous', ctx.previous)
packages/triggers/trigger-record-change/src/bulk-write-per-row-context.test.ts:15   comment
packages/triggers/trigger-record-change/src/record-change-trigger.test.ts:352       the fixture
packages/triggers/trigger-record-change/src/record-change-trigger.test.ts:362       the fixture
packages/triggers/trigger-record-change/src/record-change-trigger.ts:281            the doc comment
packages/triggers/trigger-record-change/src/record-change-trigger.ts:299            the limb

One hit needs disposing of explicitly, because it is the only one that is live
code rather than prose: quickjs-runner.ts:494 is
setGlobalJson(vm, '__previous', ctx.previous). That installs a global inside
the QuickJS sandbox VM
, read from the canonical ctx.previous — it is a
read-side convenience for script bodies, not a writer of the host
HookContext.__previous, and the VM is isolated by JSON copy so nothing inside
it can write back onto the host ctx. It is therefore not a producer. Every other
non-fixture hit is a comment or a changeset line.

So: producers of ctx.__previous on today's main = the test fixture, and
nothing else. Premise holds.

Why the limb is unreachable, not merely unused

The sharper check the card asked for. The engine binds the canonical
ctx.previous before it dispatches, on every write shape, so a consumer that
sees an unbound previous is seeing "no row was read", never "the row went
somewhere else":

write shape binding dispatch
by-id update() engine.ts:7010if (priorRecord) hookContext.previous = … :7012 triggerHooks('beforeUpdate', …)
by-id delete() engine.ts:7869 bindPreImage, called at :7897 :7899 triggerHooks('beforeDelete', …)
predicate write, before phase engine.ts:1825 — per-row ctx carries previous dispatchPerRowBeforeHooks
predicate write, after phase engine.ts:1746 — per-row ctx carries previous buildPerRowAfterContexts

(#5272 by-id delete, #5574 per-row before phase, #5846 the update-side
consolidation that retired sys_fetch_previous_update on exactly this ground.
Line numbers as of origin/main 2c7e62d5.)

bindPreImage deliberately leaves previous UNBOUND rather than fabricating
{} when no row is found (#4649/#4775) — so the one residual "previous is
absent" state is a state where a stash would have had nothing to offer either.

Reverse verification — the inverted direction, as the card predicted

This is the inverted family (#5009 shape), and the report says so rather than
manufacturing a before-green/after-red story: the canonical key sits first in
the ?? chain, so deleting the limb changes no outcome any engine path can
produce, and all 55 pre-existing cases stay green unchanged. A positive test
can therefore never carry the weight here — it passes with or without the limb.

The weight is carried by an inverted negative pin, the same treatment #5671
gave the doc alias four cases up in this very file. Measured both ways:

  • limb deleted (this PR): 56 passed (56)
  • limb pasted back, everything else identical:
    Tests 1 failed | 43 passed (44) in record-change-trigger.test.ts, the one
    failure being
    AssertionError: expected { status: 'old' } to be undefined on
    does NOT read the __previous stash — no producer emits that key (#6978).

So the deletion is not invisible to the suite any more, which is the whole point
— the #4984 defect was that it was.

Surviving coverage for the canonical ctx.previous path

The diff deletes a test, so here is what still holds the canonical path, by name:

  • record-change-trigger.test.ts"fires the callback with a record context
    built from the hook ctx"
    : asserts ctx.previous arrives verbatim as the
    automation context's previous.
  • record-change-trigger.test.ts"does NOT read a doc alias off input":
    the neighbouring negative pin, which also exercises the absent-previous case.
  • record-change-trigger.test.ts"a record-after-write flow fires on both the
    insert hook and the update hook"
    : insert leg with previous: undefined,
    update leg with previous bound.
  • bulk-write-per-row-context.test.ts"fires PER ROW, with previous bound
    to each row's own pre-write state"
    : end-to-end through the real kernel, the
    cross-package contract for the predicate-write shape.
  • record-change-integration.test.ts"record-after-write start condition uses
    previous == null to discriminate create vs update (record_change flow start node binds to a single lifecycle event — no create-OR-update in one flow #3427)"
    : the canonical
    key consumed by a real flow condition.

The intended post-state, stated plainly

After this change, a hypothetical future producer of ctx.__previous is
silently ignored.
That is intended, not a regression: ADR-0049
enforce-or-remove plus PD #12 — declared = enforced, and an undeclared
side-channel key should not work. The declared way to hand this consumer a
pre-image is ctx.previous, which is what the engine binds.

Changeset, not skip-changeset

@objectstack/trigger-record-change is a published package and this is a
behaviour statement worth a release-notes line, so it ships a patch changeset
rather than the skip-changeset label — following the house precedent for this
exact family, .changeset/hook-ctx-doc-alias-reads-removed.md (#5906/#5671),
which also recorded "behaviour is unchanged" alias removals as a patch.

Verification

  • pnpm --filter '@objectstack/trigger-record-change^...' build — green (fresh
    worktree, deps built before testing).
  • pnpm --filter @objectstack/trigger-record-change testTest Files 5 passed (5) / Tests 56 passed (56).
  • pnpm --filter @objectstack/trigger-record-change typecheck — green.
  • Every check:* step enumerated from .github/workflows/lint.yml (not from
    memory), run one by one: all 37 ESLint-job steps green (pnpm lint through
    check:tenant-chokepoint), and all TypeScript-Type-Check-job steps green
    including the full turbo run build + turbo run typecheck over
    ./packages/* ./packages/*/* ./apps/* (121 tasks), check:type-check-debt,
    examples typecheck, downstream-contract typecheck, and the three i18n gates
    (which needed the workspace build to measure anything at all).

Out of scope, deliberately

bulk-write-per-row-context.test.ts:15 also names __previous, in the
"What this used to do, measured" header describing the pre-#5038 world. It is
past-tense history and remains accurate, so it is left alone — and it is outside
the file surface this claim fixed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01USNUyHEr7uaU6MoEWXitei


Generated by Claude Code

…back, dead since #6656 (#6978)

`plugin-audit`'s `captureBefore` was the only writer of `ctx.__previous` in the
repo; #6656 retired it, leaving `buildContext`'s `ctx.previous ?? ctx.__previous`
with a second operand nothing can bind. The engine is the single producer of the
pre-image and binds the declared key ahead of every dispatch (engine.ts:7010 for
by-id update, bindPreImage at :7869 for by-id delete, :1746/:1825 for the per-row
contexts of a predicate write — #5272/#5574/#5846).

Removed under ADR-0049 enforce-or-remove rather than kept "for safety": a future
producer of `__previous` is now ignored by design, which is the intended
post-state (declared = enforced, PD #12).

The test that fed the limb synthesised the key in its own body — the #4984 shape
that kept it looking live. Replaced with the inverted negative pin the deletion
needs, the same treatment #5671 gave the `doc` alias in this file: restoring the
limb turns it red on both assertions.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01USNUyHEr7uaU6MoEWXitei
@vercel

vercel Bot commented Aug 9, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Aug 9, 2026 2:06pm

Request Review

@github-actions github-actions Bot added the size/s label Aug 9, 2026
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/trigger-record-change.

1 release-owned page(s) reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx (via @objectstack/trigger-record-change)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/s tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants