From 95ef84fe336b268d14f487869c601b81f6df71c4 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 05:50:51 +0000 Subject: [PATCH] =?UTF-8?q?fix(spec):=20protocol-17=20rationale=20states?= =?UTF-8?q?=20the=20CURRENT=20area-gating=20fact=20=E2=80=94=20#4722=20clo?= =?UTF-8?q?sed=20the=20"server=20does=20not=20walk=20`areas`"=20caveat=20(?= =?UTF-8?q?#5337)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `MIGRATIONS_BY_MAJOR[17].rationale` still carried the caveat written at the #4651 retirement: per-item gating inside an area is enforced by the shell only, since the server does not walk `areas`. #4722 landed in the same 17.0.0 window and made that false — `filterAppForUser` runs the same `filterNav` over every `areas[].navigation`, so an item's `requiredPermissions` / `requiresService` is stripped server-side in both trees. That prose is not a comment: `docs/protocol-upgrade-guide.md` is a pure projection of it (ADR-0087 D4), i.e. the page an author upgrading 16 -> 17 reads, and the sentence sent them off to restructure their navigation tree for a gate they can now write in place. - registry.ts: history anchored ("At the time of the retirement …") and the caveat replaced with the corrected fact — #4722 named, both trees named, area-LEVEL keys explicitly still retired, `visible` (CEL) explicitly still client-side only at every level. - `.changeset/app-area-fail-open-gates-removed.md`: same sentence corrected (repo is in changesets pre mode, so that file is still a live input to the v17 GA notes). - `docs/protocol-upgrade-guide.md` regenerated via `gen:upgrade-guide`, not hand-edited. - migrations.test.ts: five pins on the step-17 rationale. Wording mirrors the schema-side prescriptions landed by PR #5336 and the `areas.navigation` note in `packages/spec/liveness/app.json`. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01559M8FVm6W6vDLABL3jvdW --- .../app-area-fail-open-gates-removed.md | 32 +++++++--- .../protocol-17-area-caveat-corrected.md | 39 ++++++++++++ docs/protocol-upgrade-guide.md | 2 +- .../spec/src/migrations/migrations.test.ts | 62 +++++++++++++++++++ packages/spec/src/migrations/registry.ts | 30 ++++++--- 5 files changed, 145 insertions(+), 20 deletions(-) create mode 100644 .changeset/protocol-17-area-caveat-corrected.md diff --git a/.changeset/app-area-fail-open-gates-removed.md b/.changeset/app-area-fail-open-gates-removed.md index 8d66af1971..97ac38ae2e 100644 --- a/.changeset/app-area-fail-open-gates-removed.md +++ b/.changeset/app-area-fail-open-gates-removed.md @@ -38,10 +38,11 @@ names* are genuinely enforced one level up and one level down: from the app's top-level `navigation` tree, and re-checked in the shell; item-level `visible` is a real CEL gate in the shell. -Three layers, of which the middle one was theatre — `filterAppForUser` reads the -app's `requiredPermissions` and then walks **only** `item.navigation`; it never -touches `item.areas`, and the client renders every area in the switcher. ADR-0078 -false compliance, the same shape as `capabilities.readOnly` (#4583). +Three layers, of which the middle one was theatre — at the time of the +retirement `filterAppForUser` read the app's `requiredPermissions` and then +walked **only** `item.navigation`; it never touched `item.areas`, and the client +rendered every area in the switcher. ADR-0078 false compliance, the same shape as +`capabilities.readOnly` (#4583). **Removed rather than enforced (ADR-0049), deliberately.** Enforcing area gates is not wrong, it is unscoped: it needs semantics settled first — when an area is @@ -51,8 +52,21 @@ not invent an authorization mechanism. Removing a gate that never gated is strictly safer than shipping a major with it still declared, which would have kept authors writing it for all of 17.x. -**One caveat the prescription carries rather than hides:** per-item gating -*inside* an area is enforced by the shell only, because the server does not walk -`areas`. Anything that must never reach the browser belongs in the app's -top-level `navigation` tree, or in its own app. Trading one false belief for a -weaker one would have repeated the defect this removal exists to end. +**The caveat this prescription used to carry is closed — in this same major.** +It read: per-item gating *inside* an area is enforced by the shell only, because +the server does not walk `areas`. #4722 landed inside the 17.0.0 window and made +that false: `filterAppForUser` now runs the **same** `filterNav` over every +`areas[].navigation`, so an **item**'s `requiredPermissions` / `requiresService` +is stripped server-side in **both** trees and a gated entry never ships in the +`/meta` body. So the retirement kit's advice needs no navigation restructuring — +gating the items of the area you already have is server-enforced. + +Read that as the boundary closing, **not** as the area-level keys coming back. +They stay retired; #4722 gave an area no gate of its own, it enforces the items +inside one. And the other half of the asymmetry is unchanged, which is why +`requiredPermissions` is the key to reach for: `visible` (CEL) and +`requiresObject` are still evaluated client-side **only** at every level — +server-side CEL needs a bound `user` context the read layer does not have. So +anything that must never reach the browser goes in `requiredPermissions`, never +in `visible`. Trading one false belief for a weaker one would have repeated the +defect this removal exists to end. diff --git a/.changeset/protocol-17-area-caveat-corrected.md b/.changeset/protocol-17-area-caveat-corrected.md new file mode 100644 index 0000000000..719a4a2ae7 --- /dev/null +++ b/.changeset/protocol-17-area-caveat-corrected.md @@ -0,0 +1,39 @@ +--- +'@objectstack/spec': patch +--- + +fix(spec): protocol-17 迁移步骤的 rationale 不再声称「服务端不走 `areas`」—— 该边界已由 #4722 在同一大版本内关闭 (#5337) + +`MIGRATIONS_BY_MAJOR[17].rationale` 里的 app-area 段落还带着 #4651 退役当时写下的 +caveat: + +> per-item gating INSIDE an area is enforced by the shell only, since the server +> does not walk `areas`, so anything that must never reach the browser belongs in +> the top-level tree or in its own app. + +**#4722 之后这句已不成立**,而且它是以现在时写给读者的操作建议。`filterAppForUser` +现在对每一棵 `areas[].navigation` 跑同一个 `filterNav`(`packages/rest/src/rest-server.ts`), +导航**项**的 `requiredPermissions` / `requiresService` 在两棵树被同等强制、被门禁掉的条目 +根本不会出现在 `/meta` 响应体里。 + +这段 prose 不是注释:`docs/protocol-upgrade-guide.md` 是它的纯投影(ADR-0087 D4, +`gen:upgrade-guide`),也就是**正在从 16 升到 17 的作者**读的那一页。原句会劝他为一个如今 +可以就地写下的门禁去重构整棵导航树。#4722 与 #4651 落在同一个 17.0.0 窗口内,所以读这份 +指南的人所处的世界已经是「服务端走两棵树」。 + +改后措辞与 PR #5336 落地的 schema 侧处方(`AREA_VISIBLE_RETIRED` / +`AREA_REQUIRED_PERMISSIONS_RETIRED`)、以及 `packages/spec/liveness/app.json` 的 +`areas.navigation` 记录一致: + +- **保留历史**:退役当时的状态叙述(为什么选 route B 而非 route A)原样保留,只是把时态 + 锚在「At the time of the retirement」,免得历史被读成现状; +- **点名 #4722**:命名两棵树、命名 `areas[].navigation`; +- ⛔ **不复活 area 级键**:`app.areas[].visible` / `app.areas[].requiredPermissions` + 依然退役,#4722 强制的是 area **内部的项**,area 自身没有门; +- ⛔ **不带歪 `visible` 的口径**:`visible`(CEL)与 `requiresObject` 在**每一层**仍然只在 + 客户端求值 —— 服务端 CEL 需要一个读取层没有的 `user` 绑定 —— 所以绝不可到达浏览器的东西 + 写在 `requiredPermissions`,永远不要写在 `visible`。 + +未发布的 `.changeset/app-area-fail-open-gates-removed.md` 里同一句一并订正(仓库处于 +changesets pre 模式,该文件仍是 v17 GA 发布说明的法定输入)。`docs/protocol-upgrade-guide.md` +由 `gen:upgrade-guide` 重新生成,未手改。 diff --git a/docs/protocol-upgrade-guide.md b/docs/protocol-upgrade-guide.md index be6af3a4ac..29bef973dd 100644 --- a/docs/protocol-upgrade-guide.md +++ b/docs/protocol-upgrade-guide.md @@ -170,7 +170,7 @@ Separately, `object.managedBy: 'system'` is retired in favour of `'system-data'` Finally, five keys retire because the advisory lint could never have warned about them (#4509): mapping `extractQuery` / `errorPolicy` / `batchSize`, and app `contextSelectors[].includeAll` / `.placement`. Four of the five carry schema DEFAULTS, and a default materialises at parse time — so the liveness lint cannot tell a value the author wrote from one the schema supplied, and marking them would have warned on every mapping and every selector in existence. For a key in that state removal is not the escalation after a warning; it is the only channel that ever reaches the author, which is why they ship inside the 17.0.0 window rather than after a deprecation cycle. What they claimed: `extractQuery` promised an export path no exporter implements (exports go through the ordinary query API); `errorPolicy` offered skip/abort/retry where error handling belongs to the import REQUEST; `batchSize` sized batches the write path sizes itself; `placement` offered a topbar that places nothing. `includeAll` is the one worth reading twice — it was not unread but deliberately DISOBEYED, because context selectors are mandatory-scope and an "All" row would clear the scope: on Studio's package selector that means listing the platform's own system/cloud kernel packages to a developer who scoped to their package. `STUDIO_APP` authored `includeAll: true` against a renderer that ignored it. The mapping prescription for `batchSize` deliberately offers no rename: bulk-action, connector, sync, offline, seed-loader and NoSQL-cursor `batchSize` are all live, but each is a different key sizing its own path — the same trap `datasource.retryPolicy` vs `hook`/`job` `retryPolicy` had to defuse one issue earlier. -The sharpest removal in this step is two keys wide: `app.areas[].visible` and `app.areas[].requiredPermissions` (#4651). Read the class before the count — these were not inert authoring keys but FAIL-OPEN access gates. The server-side authority (`filterAppForUser`) checks the app's `requiredPermissions` and then walks ONLY the top-level `navigation` tree; it never reads `item.areas` at all, and the client renders every area in the switcher. So an author writing `requiredPermissions: ['sales.admin']` on an area got a clean parse, a stored value, and an area visible to everybody — and had every reason to believe otherwise, because the SAME key names are genuinely enforced one level up and one level down: app-level `requiredPermissions` drops the whole app server-side, and a navigation ITEM's `requiredPermissions` / `requiresService` are stripped server-side and re-checked in the shell, whose item-level `visible` is a real CEL gate. Three layers, of which the middle one was theatre. Enforcing instead was weighed and deliberately not taken here: it needs semantics decided first (does filtering an area remove its items everywhere? does the server bind `user` for area CEL?), and a retirement must not invent an authorization mechanism — while shipping a major with the gate still declared would have kept authors writing it for all of 17.x. The rewrite is lossless in outcome (the keys changed nothing), so what an upgrading author has to re-decide is only where the gate really goes: onto the items inside the area, or onto the app. One honest caveat the prescription carries rather than hides — per-item gating INSIDE an area is enforced by the shell only, since the server does not walk `areas`, so anything that must never reach the browser belongs in the top-level tree or in its own app. +The sharpest removal in this step is two keys wide: `app.areas[].visible` and `app.areas[].requiredPermissions` (#4651). Read the class before the count — these were not inert authoring keys but FAIL-OPEN access gates. At the time of the retirement the server-side authority (`filterAppForUser`) checked the app's `requiredPermissions` and then walked ONLY the top-level `navigation` tree; it never read `item.areas` at all, and the client rendered every area in the switcher. So an author writing `requiredPermissions: ['sales.admin']` on an area got a clean parse, a stored value, and an area visible to everybody — and had every reason to believe otherwise, because the SAME key names are genuinely enforced one level up and one level down: app-level `requiredPermissions` drops the whole app server-side, and a navigation ITEM's `requiredPermissions` / `requiresService` are stripped server-side and re-checked in the shell, whose item-level `visible` is a real CEL gate. Three layers, of which the middle one was theatre. Enforcing instead was weighed and deliberately not taken here: it needs semantics decided first (does filtering an area remove its items everywhere? does the server bind `user` for area CEL?), and a retirement must not invent an authorization mechanism — while shipping a major with the gate still declared would have kept authors writing it for all of 17.x. The rewrite is lossless in outcome (the keys changed nothing), so what an upgrading author has to re-decide is only where the gate really goes: onto the items inside the area, or onto the app — and BOTH of those destinations are server-enforced. The caveat this prescription used to carry, that per-item gating INSIDE an area was enforced by the shell only because the server did not walk `areas`, was CLOSED by #4722 inside this same 17.0.0 window: `filterAppForUser` now runs the SAME `filterNav` over every `areas[].navigation`, so an ITEM's `requiredPermissions` / `requiresService` is stripped server-side in BOTH trees and a gated entry never ships in the `/meta` body at all. Read that as the boundary closing, NOT as the area-LEVEL keys coming back: those stay retired and #4722 gave an area no gate of its own — what it enforces are the items inside one. The other half of the asymmetry is unchanged and is why `requiredPermissions` is the key to reach for: `visible` (CEL) and `requiresObject` are still evaluated client-side ONLY at every level, because server-side CEL needs a bound `user` context the read layer does not have — so anything that must never reach the browser goes in `requiredPermissions`, never in `visible`. The same window converges the retry policy (#4661). `@objectstack/spec/automation` and `@objectstack/spec/system` each exported a `RetryPolicy`/`RetryPolicySchema` resolving to a DIFFERENT declaration, so which shape a consumer got depended only on the import path (#4411) — yet both computed `delay = base * multiplier^(retry-1)` and both executors implemented that same formula. One declaration now serves both entries with the union of their capabilities, so `job.retryPolicy` gains the `maxRetryDelayMs` ceiling and `jitter` (both enforced in `runWithPolicy`, not merely declared — jitter is what stops a fleet of jobs that failed on one outage from retrying in lockstep). The single authorable casualty is the automation spelling of the base delay: `retryDelayMs` → `backoffMs`, a pure rename that replays losslessly and is what the already-enforced retry policies (`job.retryPolicy`, `hook.retryPolicy`) call it. diff --git a/packages/spec/src/migrations/migrations.test.ts b/packages/spec/src/migrations/migrations.test.ts index 3085d5b38a..aa6e9580d0 100644 --- a/packages/spec/src/migrations/migrations.test.ts +++ b/packages/spec/src/migrations/migrations.test.ts @@ -52,6 +52,68 @@ describe('migration chain (ADR-0087 D3)', () => { }); }); + // The rationale is not decoration: `docs/protocol-upgrade-guide.md` is a pure + // projection of it (ADR-0087 D4, `gen:upgrade-guide`), so this string IS the + // page an author upgrading 16 → 17 reads. A stale present-tense claim here is + // published advice, which is why it gets pinned like a prescription. + describe('protocol-17 rationale — the app-area section states the CURRENT fact (#5337)', () => { + const rationale17 = () => MIGRATIONS_BY_MAJOR[17]!.rationale; + + it('does not repeat the retired "the server does not walk `areas`" claim', () => { + // #4722 (same 17.0.0 window) made `filterAppForUser` run the same + // `filterNav` over every `areas[].navigation`. The sentence that survived + // #4651 told an upgrading author to restructure their navigation tree for + // a gate they can now write in place. Same pin as the schema-side + // prescriptions carry since #5336 (`packages/spec/src/ui/app.test.ts`). + expect(rationale17()).not.toMatch(/does not walk/i); + // The rationale still QUOTES the retired boundary — naming what changed + // is how a reader who remembers the old advice knows to drop it — so the + // pin is on the tense, which is the whole defect: the claim may appear as + // history ("was enforced by the shell only"), never as current fact. + expect(rationale17()).not.toMatch(/is enforced by the shell only/i); + expect(rationale17()).toMatch(/was CLOSED by #4722/); + }); + + it('names #4722 and the two trees an item gate is now enforced in', () => { + const r = rationale17(); + expect(r).toMatch(/#4722/); + expect(r).toMatch(/BOTH trees/); + expect(r).toMatch(/areas\[\]\.navigation/); + }); + + it('does not read as reviving the area-LEVEL keys', () => { + // The retirement verdict is untouched: what #4722 enforces are the ITEMS + // inside an area, never a gate of the area's own. A rationale that merely + // went quiet about `areas[]` would leave the reader with the old boundary; + // one that over-corrects would read as an un-retirement. + const r = rationale17(); + expect(r).toMatch(/stay retired/); + expect(r).toMatch(/no gate of its own/); + }); + + it('keeps `visible` client-side only — the half #4722 did NOT change', () => { + // The newly tempting false belief is "areas are gated now, so `visible` + // is fine". `visible` (CEL) is still evaluated in the browser at every + // level, so it hides an entry that has already been served. + const r = rationale17(); + expect(r).toMatch(/client-side ONLY/); + expect(r).toMatch(/`visible` \(CEL\)/); + expect(r).toMatch(/never in `visible`/); + }); + + it('still carries the #4651 history the step exists to explain', () => { + // The first half is a record of the state AT the retirement and of why + // route B (remove) beat route A (enforce). Correcting the caveat must not + // erase it — an upgrading author needs to know the keys were fail-open, + // not merely unread. + const r = rationale17(); + expect(r).toMatch(/#4651/); + expect(r).toMatch(/FAIL-OPEN access gates/); + expect(r).toMatch(/At the time of the retirement/); + expect(r).toMatch(/must not invent an authorization mechanism/); + }); + }); + describe('composition (cross-major is the designed-for case)', () => { it('composes only the steps in (from, to]', () => { const chain = composeMigrationChain(10, 11); diff --git a/packages/spec/src/migrations/registry.ts b/packages/spec/src/migrations/registry.ts index 3b0f793dcc..6b0c49d558 100644 --- a/packages/spec/src/migrations/registry.ts +++ b/packages/spec/src/migrations/registry.ts @@ -653,12 +653,13 @@ const step17: MigrationStep = { + 'retryPolicy` vs `hook`/`job` `retryPolicy` had to defuse one issue earlier.\n\n' + 'The sharpest removal in this step is two keys wide: `app.areas[].visible` and ' + '`app.areas[].requiredPermissions` (#4651). Read the class before the count — these were ' - + 'not inert authoring keys but FAIL-OPEN access gates. The server-side authority ' - + '(`filterAppForUser`) checks the app\'s `requiredPermissions` and then walks ONLY the ' - + 'top-level `navigation` tree; it never reads `item.areas` at all, and the client renders ' - + 'every area in the switcher. So an author writing `requiredPermissions: [\'sales.admin\']` ' - + 'on an area got a clean parse, a stored value, and an area visible to everybody — and had ' - + 'every reason to believe otherwise, because the SAME key names are genuinely enforced one ' + + 'not inert authoring keys but FAIL-OPEN access gates. At the time of the retirement the ' + + 'server-side authority (`filterAppForUser`) checked the app\'s `requiredPermissions` and ' + + 'then walked ONLY the top-level `navigation` tree; it never read `item.areas` at all, and ' + + 'the client rendered every area in the switcher. So an author writing ' + + '`requiredPermissions: [\'sales.admin\']` on an area got a clean parse, a stored value, and ' + + 'an area visible to everybody — and had every reason to believe otherwise, because the SAME ' + + 'key names are genuinely enforced one ' + 'level up and one level down: app-level `requiredPermissions` drops the whole app ' + 'server-side, and a navigation ITEM\'s `requiredPermissions` / `requiresService` are ' + 'stripped server-side and re-checked in the shell, whose item-level `visible` is a real ' @@ -669,10 +670,19 @@ const step17: MigrationStep = { + 'major with the gate still declared would have kept authors writing it for all of 17.x. ' + 'The rewrite is lossless in outcome (the keys changed nothing), so what an upgrading ' + 'author has to re-decide is only where the gate really goes: onto the items inside the ' - + 'area, or onto the app. One honest caveat the prescription carries rather than hides — ' - + 'per-item gating INSIDE an area is enforced by the shell only, since the server does not ' - + 'walk `areas`, so anything that must never reach the browser belongs in the top-level ' - + 'tree or in its own app.\n\n' + + 'area, or onto the app — and BOTH of those destinations are server-enforced. The caveat ' + + 'this prescription used to carry, that per-item gating INSIDE an area was enforced by the ' + + 'shell only because the server did not walk `areas`, was CLOSED by #4722 inside this same ' + + '17.0.0 window: `filterAppForUser` now runs the SAME `filterNav` over every ' + + '`areas[].navigation`, so an ITEM\'s `requiredPermissions` / `requiresService` is stripped ' + + 'server-side in BOTH trees and a gated entry never ships in the `/meta` body at all. Read ' + + 'that as the boundary closing, NOT as the area-LEVEL keys coming back: those stay retired ' + + 'and #4722 gave an area no gate of its own — what it enforces are the items inside one. ' + + 'The other half of the asymmetry is unchanged and is why `requiredPermissions` is the key ' + + 'to reach for: `visible` (CEL) and `requiresObject` are still evaluated client-side ONLY ' + + 'at every level, because server-side CEL needs a bound `user` context the read layer does ' + + 'not have — so anything that must never reach the browser goes in `requiredPermissions`, ' + + 'never in `visible`.\n\n' + 'The same window converges the retry policy (#4661). `@objectstack/spec/automation` and ' + '`@objectstack/spec/system` each exported a `RetryPolicy`/`RetryPolicySchema` resolving ' + 'to a DIFFERENT declaration, so which shape a consumer got depended only on the import '