Skip to content

Commit 344a22a

Browse files
os-zhuangos-support-aiclaude
authored
refactor(plugin-audit)!: retire export/permission_change from the sys_audit_log action enum (#8147) (#8200)
Retires the two action values with no writer anywhere in the repo, per the maintainer ruling of 2026-08-12 on #7675. Narrows the auth_events and config_changes list-view filters, regenerates the translation bundles, and registers the retirement under ADR-0087 as `audit-log-action-enum-retired`. `import` is deliberately NOT retired: plugin-auth's admin user-import writes a real run-level row with that action, pinned by dogfood case W4. Escalated on the issue for a maintainer ruling. Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 Co-authored-by: Claude <support@objectstack.ai> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent fa48973 commit 344a22a

12 files changed

Lines changed: 397 additions & 13 deletions

File tree

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
---
2+
"@objectstack/plugin-audit": minor
3+
"@objectstack/spec": minor
4+
"@objectstack/service-analytics": patch
5+
---
6+
7+
refactor(plugin-audit)!: retire `export` and `permission_change` from the `sys_audit_log` action enum — two declared actions nothing has ever written (#8147, #7675, ADR-0049/ADR-0087)
8+
9+
<!-- adr-0087: registered audit-log-action-enum-retired -->
10+
11+
**BREAKING** (shipped as `minor` under the launch-window lockstep convention).
12+
13+
`sys_audit_log.action` declared ten actions. Two of them named events this
14+
platform does not record, and has never recorded. Enumerating every
15+
`sys_audit_log` writer in the repo finds exactly two:
16+
17+
- `plugin-audit/src/audit-writers.ts` — the generic hook writer, whose
18+
`actionFor()` maps `afterInsert`/`afterUpdate`/`afterDelete` to
19+
`create`/`update`/`delete` and **nothing else**;
20+
- `plugin-auth/src/admin-import-users.ts` — the admin user-import run-level row.
21+
22+
Neither has ever emitted `export` or `permission_change`. The cost was not a
23+
dormant string: `sys_audit_log` ships **list views** filtered on those values and
24+
the platform dashboard ships **metric widgets** counting them, so an operator got
25+
a permanently empty "Permission Changes" tile and an Auth view whose filter could
26+
never match, while an auditor reading the enum believed the platform captured
27+
permission changes and data exports. That is false compliance on a compliance
28+
surface — the sharpest form of ADR-0049 declared-≠-enforced.
29+
30+
Maintainer ruling 2026-08-12 (#7675) split the finding in two: build the cheap
31+
writers (`login`/`logout` in #8144, `config_change` in #8145) and retire the enum
32+
values with no feature behind them. 原则记录:空 widget + 永远查不到东西的过滤器
33+
是可见产品缺陷;审计面宁窄勿谎。
34+
35+
### Migration: FROM → TO
36+
37+
| Wrote | Write instead |
38+
|:--|:--|
39+
| a filter, saved query or dashboard on `action = 'permission_change'` | filter the permission objects' own `create` / `update` rows by `object_name` — a grant or binding write is an ordinary record write and the generic writer already ledgers it |
40+
| a filter, saved query or dashboard on `action = 'export'` | delete it — no export feature ever wrote an audit row, so it returned nothing on every deployment |
41+
| a `switch` / badge map with arms for either value | delete those arms; an exhaustive `switch` over the action type now fails to compile if they stay |
42+
43+
Every such query returned an empty result set before this change and returns the
44+
same empty result set after it. What changed is that the contract stops promising
45+
otherwise.
46+
47+
⚠️ **Existing rows are untouched and must stay untouched.** The enum is not
48+
enforced on this object — `validateRecord` skips `readonly` fields and every
49+
`sys_audit_log` field is `readonly: true` — so stored history parses and reads
50+
back exactly as written. Audit history is append-only; do not migrate or delete
51+
rows to satisfy a schema narrowing.
52+
53+
### Also in this change
54+
55+
- `auth_events` list view: filter narrowed to `['login', 'logout']`.
56+
- `config_changes` list view: `export` dropped from the filter.
57+
- `plugin-audit`'s generated translation bundles regenerated for all four locales.
58+
- ADR-0087 registration as the semantic migration `audit-log-action-enum-retired`
59+
(D3 step 17). An enum-VALUE retirement, so nothing lands in
60+
`RETIRED_KEYS_BY_MAJOR` and the four surface ratchets are byte-identical by
61+
construction — no authorable key and no def changed.
62+
63+
### `import` is deliberately NOT retired
64+
65+
The 2026-08-12 ruling named `import` alongside the other two on the stated
66+
premise 无此 feature. That premise is measurably false and the value stays:
67+
`plugin-auth`'s admin user-import writes a real run-level row on every run
68+
(`action: 'import'`, `record_id: null`), pinned by case W4 of
69+
`packages/qa/dogfood/test/admin-identity-audit-trail.dogfood.test.ts`. Retiring
70+
it would make the enum deny a value the platform writes — and silently, since
71+
the enum is unenforced here. Referred back for a maintainer ruling on #8147.

‎docs/protocol-upgrade-guide.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -328,6 +328,9 @@ The action LOCATION vocabulary loses `global_nav` in this step (#6888, ADR-0049,
328328
- **`apimethod-enum-shrink`** — `data.object.enable.apiMethods (the eight legacy non-primitive values)` → the six primitives only — `get` / `list` / `create` / `update` / `delete` / `bulk`: replace each legacy value with the primitives it derives from, de-duplicate, and delete the key entirely if the result names all six
329329
- Why not automatic: The authored `enable.apiMethods` enum is now exactly the six primitives. The eight legacy values — `upsert`, `aggregate`, `history`, `search`, `restore`, `purge`, `import`, `export` — are no longer authorable, because they are DERIVED effective operations resolved by the server's single derivation table, and an enum that lets an author name both a primitive and something derived from it has two spellings for one fact. The FROM → TO is a table rather than a rename: `upsert` → `create` + `update`; `import` → `create` + `update`; `export`, `aggregate` and `search` → `list`; `history` → `get`; and `restore` / `purge` map to NOTHING — they never derived, because `enable.trash` was retired in #2377, so the value is deleted outright. That last row is why this is a semantic entry and not a mechanical conversion, and the reason is a security one: the mapping WIDENS. An allowlist naming `history` was granting read of one record's audit trail; rewritten to `get` it grants ordinary record reads, and an allowlist naming `search` becomes a grant of full `list`. A transform that applied the table silently would broaden real API permissions without anyone reading the diff, so the rewrite is delegated to the author with the widening flagged. The reporter codemod exists for exactly that shape: `node scripts/codemod/apimethods-legacy-to-primitives.mjs` scans, reports the exact replacement per site, and FLAGS the allowlists the mapping would widen so the edit stays reviewable — it reports, it does not rewrite. Stored metadata keeps parsing (permanent tolerance, narrowing only), so nothing breaks at rest; what changes is what an author may newly write. Registered by the #6350 stock reconciliation; #3543 (P2 of #3391) predates the #6148 completeness gate. ADR-0087, #3543 (backfilled #6350).
330330
- Done when: No authored `enable.apiMethods` array names a legacy value; `objectstack validate` passes. Run the reporter codemod first and read its widening flags before applying anything — ⚠️ the migration is only correct if each widened grant was INTENDED. For every object where `history` became `get` or `search` became `list`, confirm the broader operation is one the API should genuinely expose; where it is not, the answer is not a different value in this enum but a permission set that withholds the operation. Where the six primitives are all present, prefer deleting the key: that is equivalent to default-open and it tracks future primitives, whereas a hand-listed six silently stops granting anything added later. `restore` / `purge` are deleted with no replacement — if trash-like behaviour was being relied on, that capability left in #2377 and this entry is not where it returns.
331+
- **`audit-log-action-enum-retired`** — `sys_audit_log.action — the values 'export' and 'permission_change' left the select enum declared by plugin-audit (packages/plugins/plugin-audit/src/objects/sys-audit-log.object.ts). The same two values also left the shipped list-view filters on that object: 'permission_change' from the auth_events view and 'export' from the config_changes view` → nothing, for either value — both are removed rather than renamed, because neither named an event this platform records. For permission changes, read the ordinary `create` / `update` rows on the permission objects themselves: a grant or binding write is an ordinary record write and the generic audit writer already ledgers it, so a second semantically-duplicate row was never minted. For `export` there is no replacement and nothing is lost: no export feature ever wrote an audit row. A consumer filtering `sys_audit_log` on either value was reading an empty result set on every deployment, and still is — what changed is that the contract no longer promises otherwise
332+
- Why not automatic: Maintainer ruling 2026-08-12 (#7675), the retirement half of a two-half verdict: the cheap writers get built (#8144 login/logout, #8145 config_change) and the enum values with no feature behind them are retired. 原则记录:空 widget + 永远查不到东西的过滤器是可见产品缺陷;审计面宁窄勿谎. The defect was false compliance on a COMPLIANCE surface, which is the sharpest form of ADR-0049 declared-≠-enforced: an auditor reading the action enum believed the platform captured permission changes and data exports, and the shipped list views and dashboard widgets showed them a filter and a tile for exactly those events. Both were permanently empty. Measured by enumerating every `sys_audit_log` writer in the repo — there are exactly two: plugin-audit`s generic hook writer, whose `actionFor` maps afterInsert/Update/Delete to create/update/delete and nothing else, and plugin-auth`s admin user-import. Neither has ever emitted `export` or `permission_change`. This is an enum-VALUE retirement, so the bookkeeping differs from a key retirement in the two ways `hook-body-crypto-hash-removed`, `dataset-measure-array-string-agg-removed` and `action-global-nav-location-removed` already record: nothing lands in RETIRED_KEYS_BY_MAJOR (no authorable KEY changed) and the four surface ratchets are expected to be byte-identical (no def changed). It differs from all three in being a SEMANTIC entry rather than a D2 conversion, and the reason is that there is no source to rewrite: `sys_audit_log` is a platform-owned, append-only object whose every field is `readonly: true`. Nobody authors an audit row and nobody authors this enum — the values appear only in rows the runtime writes and in queries consumers send. A conversion rewrites authored metadata or a stored `sys_metadata` row; this surface is neither, so the disposition is the one `BatchOptions.validateOnly` and the notification cursor already take in this major. ⚠️ Historical ROWS are deliberately untouched. A deployment that somehow holds a row with either value keeps it, and keeps reading it back: the enum is not enforced on this object at all (`validateRecord` skips `readonly` fields, and every field here is readonly), so nothing rejects stored history and no backfill is required or wanted. Deleting audit history to satisfy a schema narrowing would be the one genuinely destructive reading of this change. ADR-0049 / ADR-0087, #8147.
333+
- Done when: No consumer filters `sys_audit_log` on `action = "export"` or `action = "permission_change"` expecting rows: both were empty everywhere before this change, so a query that returned data has not been identified and a query that returned nothing behaves identically. Concretely, check three places. (1) Saved queries, dashboards and reports over `sys_audit_log`: a filter naming either value should be deleted, not re-pointed — for permission auditing, filter the permission objects` own `create`/`update` rows by `object_name` instead. (2) Any code branching on the action string (a badge map, a label switch, an `if (row.action === ...)`): the arms for these two values are now unreachable and should go, and a `switch` with an exhaustiveness check over the enum type will now fail to compile if they stay — that compile error is the enforced channel for TypeScript consumers. (3) Custom objects or plugins inserting `sys_audit_log` rows with either value: this is the only case that needs a real decision, because the write will NOT be refused (readonly fields are not validated) — it will simply be a row whose action the object no longer declares. Pick a declared value or open an issue for the action you actually need. ⚠️ Do NOT migrate or delete existing rows: audit history is append-only and stays exactly as written.
331334
- **`auth-config-unadvertised-reserved-features`** — `api.authConfig.features.passkeys / api.authConfig.features.magicLink` → (removed — no replacement flag; the capabilities are not advertised)
332335
- Why not automatic: Both flags were served by `GET /api/v1/auth/config` from introduction and read by no client: no login UI anywhere renders a passkey or magic-link affordance off them, so the payload advertised two sign-in methods a user could never reach, and a deployer setting `plugins.passkeys` / `plugins.magicLink` flipped a switch with no observable effect (ADR-0049 enforce-or-remove; maintainer ruling 2026-08-11 on #7481 chose remove over keep-as-reserved). The two are not equally empty: nothing at all is wired behind `passkeys`, whereas `magicLink`'s better-auth endpoints are live and only their advertisement was withdrawn. This is a RESPONSE surface — nobody authors or persists an `AuthFeaturesConfig` — so there is no source for the chain to rewrite; the schema tombstones both keys via retiredKey() and consumers drop their read. The withdrawal is conditional: both return to the payload in the change that ships the login UI (objectui#4179). ADR-0049, #7481.
333336
- Done when: No client reads `features.passkeys` or `features.magicLink` off `/api/v1/auth/config`; a client that gated UI on either now treats the capability as absent rather than reading `undefined` as false by accident, and constructing an `AuthFeaturesConfig` with either key fails to parse with its own prescription instead of being silently stripped. Magic-link deployments keep working: `plugins.magicLink` still mounts `/api/v1/auth/magic-link/send` and `/magic-link/verify`, which a custom UI may call directly.
Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, it, expect } from 'vitest';
4+
import { SysAuditLog } from './index.js';
5+
6+
/**
7+
* #8147 — `export` and `permission_change` are RETIRED from the
8+
* `sys_audit_log.action` enum (maintainer ruling 2026-08-12 on #7675, ADR-0049
9+
* enforce-or-remove, registered under ADR-0087 as `audit-log-action-enum-retired`).
10+
*
11+
* This file exists because **nothing else in the repo can detect a regression
12+
* here.** The enum is not enforced on writes at all: `validateRecord` skips
13+
* `readonly` fields (`record-validator.ts`, insert branch) and every
14+
* `sys_audit_log` field is `readonly: true`, so re-adding a value refuses
15+
* nothing and rejects nothing. The generated translation bundles are the only
16+
* other committed artifact that moves with the enum, and they only pin that the
17+
* bundle and the enum AGREE — regenerate both and the drift disappears. An
18+
* object field has no `retiredKey()` tombstone to reject the name at authoring
19+
* time the way a spec property does, so this pin IS the tombstone for the
20+
* platform-owned declaration (the `sys_comment` retired-fields precedent, #4756).
21+
*
22+
* The expectations below are written as literals on purpose. A test that read
23+
* the allowed set out of the object and asserted the object matched it could
24+
* not fail — expectation and reality would derive from the same source.
25+
*
26+
* If a future change genuinely needs one of these actions back, it arrives
27+
* WRITER-FIRST — the emission point, its tests, and the list view/widget that
28+
* surfaces it — and updates this file deliberately, never as collateral.
29+
*/
30+
31+
const RETIRED_ACTIONS: ReadonlyArray<readonly [action: string, prescription: string]> = [
32+
[
33+
'permission_change',
34+
'permission-object writes are already on the ledger as ordinary `create` / `update` '
35+
+ 'rows written by the generic hook writer; a second semantically-duplicate row is '
36+
+ 'not minted. Filter the permission objects by `object_name` instead.',
37+
],
38+
[
39+
'export',
40+
'no export feature has ever written an audit row — `actionFor()` in audit-writers.ts '
41+
+ 'emits create/update/delete and nothing else. A filter on this value matched '
42+
+ 'nothing on every deployment that has ever run.',
43+
],
44+
];
45+
46+
/** Option values declared by the `action` select field. */
47+
function actionValues(): string[] {
48+
const field = (SysAuditLog as { fields?: Record<string, { options?: unknown }> })
49+
.fields?.action;
50+
const options = (field?.options ?? []) as Array<string | { value?: string }>;
51+
return options.map((o) => (typeof o === 'string' ? o : String(o.value)));
52+
}
53+
54+
/** Every value named by every `action` filter across every shipped list view. */
55+
function filteredActionValues(): Array<{ view: string; value: string }> {
56+
const views = (SysAuditLog as {
57+
listViews?: Record<string, { filter?: Array<{ field?: string; value?: unknown }> }>;
58+
}).listViews ?? {};
59+
const out: Array<{ view: string; value: string }> = [];
60+
for (const [view, def] of Object.entries(views)) {
61+
for (const clause of def.filter ?? []) {
62+
if (clause.field !== 'action') continue;
63+
const values = Array.isArray(clause.value) ? clause.value : [clause.value];
64+
for (const v of values) out.push({ view, value: String(v) });
65+
}
66+
}
67+
return out;
68+
}
69+
70+
describe('sys_audit_log — retired actions stay retired (#8147)', () => {
71+
it.each(RETIRED_ACTIONS)(
72+
'%s is not declared by the action enum',
73+
(action, prescription) => {
74+
expect(
75+
actionValues(),
76+
`sys_audit_log.action '${action}' was retired under ADR-0049 (#8147) — ${prescription}`,
77+
).not.toContain(action);
78+
},
79+
);
80+
81+
it.each(RETIRED_ACTIONS)(
82+
'%s is not named by any shipped list-view filter',
83+
(action, prescription) => {
84+
const offenders = filteredActionValues().filter((f) => f.value === action);
85+
expect(
86+
offenders,
87+
`a list view filters on the retired action '${action}' (${offenders
88+
.map((o) => o.view)
89+
.join(', ')}) — it can never match. ${prescription}`,
90+
).toEqual([]);
91+
},
92+
);
93+
94+
it('every list-view action filter names a value the enum still declares', () => {
95+
const declared = new Set(actionValues());
96+
const dangling = filteredActionValues().filter((f) => !declared.has(f.value));
97+
expect(
98+
dangling,
99+
'a list view filters `action` on a value the enum does not declare, so the view is '
100+
+ 'permanently empty — the visible product defect the 2026-08-12 ruling named '
101+
+ '(空 widget + 永远查不到东西的过滤器是可见产品缺陷). Narrow the filter with the enum.',
102+
).toEqual([]);
103+
});
104+
105+
/**
106+
* The deliberate NON-retirement. The 2026-08-12 ruling named `import`
107+
* alongside the other two on the premise 无此 feature, and that premise is
108+
* false: `plugin-auth`'s admin user-import writes a run-level row
109+
* (`admin-import-users.ts` — `action: 'import'`, `record_id: null`,
110+
* `object_name: 'sys_user'`) on every run, and case W4 of
111+
* `packages/qa/dogfood/test/admin-identity-audit-trail.dogfood.test.ts`
112+
* asserts that row exists.
113+
*
114+
* Retiring it would make the enum deny a value the platform writes, silently
115+
* — see the file docblock on why nothing would go red. This assertion is the
116+
* detector. If a maintainer rules that `import` should go, the WRITER and the
117+
* dogfood case go first, and this line goes with them.
118+
*/
119+
it('import is still declared — it has a live writer (#8147 escalation)', () => {
120+
expect(
121+
actionValues(),
122+
"sys_audit_log.action 'import' must stay declared: plugin-auth's admin user-import "
123+
+ 'writes a real run-level row with this action on every run (admin-import-users.ts), '
124+
+ 'pinned by dogfood case W4. Removing it makes the enum deny a value the platform '
125+
+ 'writes — and silently, because readonly fields are never validated.',
126+
).toContain('import');
127+
});
128+
});

0 commit comments

Comments
 (0)