fix(spec,plugin-approvals): the two approval vocabularies are derived, not hand-matched (#3786) - #4295
Merged
os-zhuang merged 2 commits intoJul 31, 2026
Conversation
…, not hand-matched (#3786) `sys_approval_request.status` and `sys_approval_action.action` spelled their option lists out — five values and twelve — each under a "Keep in sync with `ApprovalStatus` / `ApprovalActionKind` (spec/contracts)" comment, while the contract held the same sets as bare type unions. Seventeen strings matched by hand across a package boundary with nothing checking them. They did all still agree — verified verbatim before changing anything. But agreeing is not being held, and both directions of drift are quiet: a value the COLUMN accepts and the contract omits is invisible to every consumer typed against the contract (the row exists and nothing can narrow it), while a value the CONTRACT declares and the column rejects surfaces only at write time, on whichever tenant first reaches that transition. An audit vocabulary is a bad place for either. The contract now publishes the lists as values — APPROVAL_STATUSES and APPROVAL_ACTION_KINDS — with ApprovalStatus / ApprovalActionKind derived via `(typeof X)[number]`, and both columns spread the constants. The per-entry rationale (which action kinds move the flow, which are thread-only, why `returned` differs from `recalled`) moved onto the constants, where the values live. New exports, no behaviour change: the emitted option lists are byte-identical, checked against the built artifact before and after. Existing imports of the two types are unaffected — they resolve to the same unions. approval-vocabularies.test.ts pins the qualifier derivation alone cannot: the columns agree with the contract WHILE THE SPREAD IS THERE, so the test fails if either is re-inlined as a literal that drifted. It guards the guard (an unresolvable import would compare two empty lists and pass) and asserts the two vocabularies stay distinct — a copy-paste pointing one column at the other constant would satisfy "derived from the contract" while being the wrong vocabulary entirely. Mutation-tested in both directions: adding a value to APPROVAL_STATUSES propagates into the BUILT sys_approval_request.status options (the derivation is live, not a stale build), and re-inlining a drifted literal fails `sys_approval_request.status offers exactly the contract statuses, in order`. Verified: plugin-approvals 330 tests green, spec 7169 green, api-surface regenerated (0 breaking, 2 added), check:generated / exported-any / liveness / docs all PASS. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXGj3Z5TmwSV6RK2oGc3cb
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 107 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
github-actions
Bot
added
documentation
The docs-drift check turned up a FOURTH copy of the status set, in the highest- visibility place: `automation/approvals.mdx`'s "Statuses in full" line. Unlike the CORS case in #4282 this one is correct today — all five values, same order — so there is nothing to fix, only something unguarded. Points at APPROVAL_STATUSES and says plainly that this line is a copy nothing updates automatically. That is weaker than derivation and I am not pretending otherwise: prose docs legitimately restate values, and a path-coupled test asserting this exact sentence would be brittle. Recorded as a follow-up instead — `check:variant-docs` already solves this shape for schema variants and is the right place to extend if the set starts moving. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXGj3Z5TmwSV6RK2oGc3cb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The last known derivable copy from the #3786 sweep. (A fourth, non-derivable copy turned up while verifying — see the end.)
What it was
sys_approval_request.statusandsys_approval_action.actionspelled their option lists out — five values and twelve — each under a "Keep in sync withApprovalStatus/ApprovalActionKind(spec/contracts)" comment, while the contract held the same sets as bare type unions. Seventeen strings matched by hand across a package boundary, with nothing checking them.They did all still agree — that was verified verbatim before anything changed. But agreeing is not being held, and both directions of drift are quiet:
An audit vocabulary is a bad place for either: it is precisely what gets read back when someone asks what happened.
The fix
The contract publishes the lists as values —
APPROVAL_STATUSESandAPPROVAL_ACTION_KINDS— withApprovalStatus/ApprovalActionKindderived from them via(typeof X)[number]. Both columns spread the constants. The per-entry rationale (which action kinds move the flow, which are thread-only, whyreturneddiffers fromrecalled) moved onto the constants, where the values live.No behaviour change. The emitted option lists are byte-identical — checked against the built artifact before and after:
Existing imports of the two types are unaffected — they resolve to the same unions. Two new exports,
0 breakingoncheck:api-surface.The test pins what derivation cannot
Derivation makes disagreement unrepresentable while the spread is there.
approval-vocabularies.test.tspins that qualifier: it fails if either column is re-inlined as a literal that has drifted. It cannot catch an identical re-inline — correct today, fragile tomorrow — which is why the derivation, not the test, is the mechanism.It also guards the guard (an unresolvable import would compare two empty lists and pass), and asserts the two vocabularies stay distinct — a copy-paste pointing one column at the other constant would satisfy "derived from the contract" while being the wrong vocabulary entirely.
Verification
Mutation-tested in both directions, which matters here because the two failure modes are different:
APPROVAL_STATUSES, rebuild both packagessys_approval_request.statusoptions, andactionis untouched — the derivation is live, not a stale buildstatusas a drifted literalsys_approval_request.status offers exactly the contract statuses, in orderfailscheck:api-surface(regenerated: 0 breaking, 2 added) /check:generated/check:exported-any/check:liveness/check:docs— all PASSA fourth copy, found by the docs-drift check
content/docs/automation/approvals.mdxcarries a "Statuses in full: …" line — the same five values, in the highest-visibility place. Unlike the CORS case in #4282, this one is correct today, so there is nothing to fix; it is simply unguarded.The last commit points it at
APPROVAL_STATUSESand says plainly that it is a copy nothing updates automatically. That is weaker than derivation and this PR does not pretend otherwise: prose docs legitimately restate values, and a test asserting that exact sentence would be brittle against any doc restructure.check:variant-docsalready solves this shape for schema variants and is the right place to extend if the set starts moving — recorded as a follow-up rather than bolted on here.Note
pnpm --filter @objectstack/plugin-approvals exec tsc --noEmitreports a pre-existingreplaceAlllib-target error inaction-link-pages.ts— present on a clean tree, and that package has notypecheckscript (baretscisn't its build path). Its real build (tsup) is clean.