Repository navigation
Protocol evaluation and P0 implementations: RLS + SCIM 2.0 - #219
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Assess current protocol for adjustments and optimizations
Protocol evaluation and P0 implementations: RLS + SCIM 2.0
Jan 26, 2026
hotlong
marked this pull request as ready for review
January 26, 2026 11:19
Contributor
There was a problem hiding this comment.
Pull request overview
Adds two enterprise-critical protocol specs to close P0 gaps identified in a broader protocol evaluation: Row-Level Security (RLS) and SCIM 2.0 (user/group provisioning), along with tests and updated exports.
Changes:
- Introduces an RLS protocol schema + helper and a comprehensive Vitest suite.
- Introduces a SCIM 2.0 protocol schema + helper and a comprehensive Vitest suite.
- Updates auth/permission barrel exports and adds a protocol optimization plan document.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/spec/src/permission/rls.zod.ts | New Zod schemas/types for RLS policies, config, user context, evaluation results + helper factory. |
| packages/spec/src/permission/rls.test.ts | Tests validating RLS schemas/defaults and helper outputs. |
| packages/spec/src/permission/index.ts | Exposes the new RLS protocol from the permission namespace. |
| packages/spec/src/auth/scim.zod.ts | New Zod schemas/types for SCIM resources/messages + helper factory. |
| packages/spec/src/auth/scim.test.ts | Tests validating SCIM schemas/defaults and helper outputs. |
| packages/spec/src/auth/index.ts | Exposes the new SCIM protocol from the auth namespace. |
| PROTOCOL_OPTIMIZATION_PLAN.md | Adds the evaluation/roadmap document that motivated the P0 additions. |
Comment on lines
+411
to
+423
| const SCIMSchemaURISchema = z.enum([ | ||
| SCIM_SCHEMAS.USER, | ||
| SCIM_SCHEMAS.GROUP, | ||
| SCIM_SCHEMAS.ENTERPRISE_USER, | ||
| SCIM_SCHEMAS.RESOURCE_TYPE, | ||
| SCIM_SCHEMAS.SERVICE_PROVIDER_CONFIG, | ||
| SCIM_SCHEMAS.SCHEMA, | ||
| SCIM_SCHEMAS.LIST_RESPONSE, | ||
| SCIM_SCHEMAS.PATCH_OP, | ||
| SCIM_SCHEMAS.BULK_REQUEST, | ||
| SCIM_SCHEMAS.BULK_RESPONSE, | ||
| SCIM_SCHEMAS.ERROR, | ||
| ]); |
There was a problem hiding this comment.
SCIMSchemaURISchema is declared but never used. With noUnusedLocals: true (see root tsconfig.json), this will fail TypeScript compilation. Either remove it or use it (e.g., as the element schema for schemas arrays where appropriate).
Comment on lines
+802
to
+803
| schemas: z.array(z.string()) | ||
| .default([SCIM_SCHEMAS.ERROR]) |
There was a problem hiding this comment.
SCIMErrorSchema.schemas defaults to the Error URI but does not validate caller-provided values (it will accept arrays that omit the Error schema URI). To prevent parsing invalid SCIM error payloads, add .min(1) and a refinement requiring SCIM_SCHEMAS.ERROR.
Suggested change
| schemas: z.array(z.string()) | |
| .default([SCIM_SCHEMAS.ERROR]) | |
| schemas: z.array(z.string()) | |
| .min(1) | |
| .default([SCIM_SCHEMAS.ERROR]) | |
| .refine( | |
| (schemas) => schemas.includes(SCIM_SCHEMAS.ERROR), | |
| { message: `schemas must include ${SCIM_SCHEMAS.ERROR}` }, | |
| ) |
| /** | ||
| * SCIM schema URI | ||
| */ | ||
| schemas: z.array(z.string()) |
There was a problem hiding this comment.
SCIMPatchRequestSchema.schemas defaults to the PatchOp URI but does not enforce that user-provided arrays include SCIM_SCHEMAS.PATCH_OP. SCIM PATCH requests require this schema identifier; add .min(1) and a refinement (or enum) to validate it.
Suggested change
| schemas: z.array(z.string()) | |
| schemas: z.array(z.string()) | |
| .min(1) | |
| .refine( | |
| (schemas) => schemas.includes(SCIM_SCHEMAS.PATCH_OP), | |
| { message: 'SCIM PATCH requests must include the PatchOp schema URI' } | |
| ) |
| */ | ||
| [SCIM_SCHEMAS.ENTERPRISE_USER]: SCIMEnterpriseUserSchema | ||
| .optional() | ||
| .describe('Enterprise user attributes'), |
There was a problem hiding this comment.
SCIMUserSchema allows the enterprise extension object ([SCIM_SCHEMAS.ENTERPRISE_USER]) to be present even when schemas does not include SCIM_SCHEMAS.ENTERPRISE_USER. Add a refinement to require the extension URI whenever the extension attribute object is provided (to keep payloads RFC 7643-compliant).
Suggested change
| .describe('Enterprise user attributes'), | |
| .describe('Enterprise user attributes'), | |
| }).superRefine((data, ctx) => { | |
| const hasEnterpriseExtension = data[SCIM_SCHEMAS.ENTERPRISE_USER] != null; | |
| if (!hasEnterpriseExtension) { | |
| return; | |
| } | |
| const schemas = Array.isArray((data as any).schemas) ? (data as any).schemas as string[] : []; | |
| if (!schemas.includes(SCIM_SCHEMAS.ENTERPRISE_USER)) { | |
| ctx.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| path: ['schemas'], | |
| message: `schemas must include "${SCIM_SCHEMAS.ENTERPRISE_USER}" when enterprise user extension attributes are present`, | |
| }); | |
| } |
Comment on lines
+299
to
+301
| */ | ||
| using: z.string() | ||
| .describe('Filter condition (PostgreSQL SQL WHERE clause syntax with parameterized context variables)'), |
There was a problem hiding this comment.
using is required for all policies, but the docstring above says USING only applies to SELECT/UPDATE/DELETE. For INSERT-only policies, PostgreSQL-style RLS relies on check (WITH CHECK) and does not require USING. Consider making the schema a discriminated union on operation (or add a refinement) so check is required for insert and using is optional/omitted there.
Comment on lines
+667
to
+669
| - `packages/spec/src/auth/scim.zod.ts` | ||
| - `packages/spec/src/auth/scim-user.zod.ts` | ||
| - `packages/spec/src/auth/scim-group.zod.ts` |
There was a problem hiding this comment.
The Sprint 1 plan lists scim-user.zod.ts and scim-group.zod.ts, but this PR implements SCIM in a single auth/scim.zod.ts file. To keep the plan actionable, update this section (and the checklist later) to match the actual file layout (or add the missing split files).
Suggested change
| - `packages/spec/src/auth/scim.zod.ts` | |
| - `packages/spec/src/auth/scim-user.zod.ts` | |
| - `packages/spec/src/auth/scim-group.zod.ts` | |
| - `packages/spec/src/auth/scim.zod.ts` (包含 SCIM User / Group 资源定义) |
| /** | ||
| * SCIM schema URI | ||
| */ | ||
| schemas: z.array(z.string()) |
There was a problem hiding this comment.
SCIMListResponseSchema.schemas currently accepts any array (including empty or values that don't include the ListResponse URI). Per SCIM, this field must include urn:ietf:params:scim:api:messages:2.0:ListResponse; consider adding .min(1) and a refinement (or reuse a URI enum) to enforce that.
Suggested change
| schemas: z.array(z.string()) | |
| schemas: z.array(z.string()) | |
| .min(1) | |
| .refine( | |
| (schemas) => schemas.includes(SCIM_SCHEMAS.LIST_RESPONSE), | |
| { message: `schemas must include ${SCIM_SCHEMAS.LIST_RESPONSE}` }, | |
| ) |
Comment on lines
+493
to
+499
| /** | ||
| * Tenant/Organization ID | ||
| */ | ||
| tenantId: z.string() | ||
| .optional() | ||
| .describe('Tenant/Organization ID'), | ||
|
|
There was a problem hiding this comment.
In this file, the documented context variable is current_user.tenant_id, but RLSUserContextSchema models it as tenantId. The helper tenantPolicy() also generates current_user.tenant_id. Either align on tenantId/current_user.tenantId everywhere or explicitly document the mapping (camelCase user context → snake_case expression variables) to avoid confusion for implementers.
Comment on lines
+752
to
+753
| - [ ] `auth/scim.zod.ts` - SCIM 2.0 User | ||
| - [ ] `auth/scim-group.zod.ts` - SCIM 2.0 Group |
There was a problem hiding this comment.
In the P0 checklist, auth/scim-group.zod.ts is listed but does not exist in this PR (SCIM Group is included in auth/scim.zod.ts). Update the checklist items to match the implemented file structure so the checklist remains accurate.
Suggested change
| - [ ] `auth/scim.zod.ts` - SCIM 2.0 User | |
| - [ ] `auth/scim-group.zod.ts` - SCIM 2.0 Group | |
| - [ ] `auth/scim.zod.ts` - SCIM 2.0 User & Group |
Contributor
|
@copilot fix |
…apping docs Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Contributor
Author
Fixed all 9 code review issues in commit 53fdd4d: SCIM 2.0:
RLS:
Documentation:
|
This was referenced Sep 24, 2026
akarma-synetal
pushed a commit
to akarma-synetal/framework
that referenced
this pull request
Sep 28, 2026
…ocblock corpus (objectstack-ai#20057) Fixes objectstack-ai#19017 Clause-②: no Executes ruling `5805901449` on objectstack-ai#19017 (batch objectstack-ai#219 item 2, letter 甲, 「只报告」; maintainer 「同意」): the spec docblock gate family gains a narrow present-tense phrase reading, report-only. ## What changed One file: `scripts/check-spec-docblock-symbol-anchors.mjs` (+403 / -3). - `PRESENT_TENSE_PHRASES` is exactly the ruled list: `this phase`, `is a follow-up`, `not yet implemented`, `currently no`. Matching is case-insensitive and whole-word (`currently no` does not match `currently none`). Comment delimiters and the JSDoc gutter are blanked and whitespace is folded across lines, so a phrase wrapped over two lines is one hit, reported at the line where it starts. - **Population and projection are the anchor corpus's own**: the files `sweepCorpus` walked (its `byDoc`), read through `commentProse`. So "docblock" means what it means for this gate: every comment, line or block, and never code or a string literal. Test sources are skipped. - **Report-only by construction.** `reportPresentTense` has no exit path and sets no exit code. It turns any throw into a `NOT MEASURED` line. It runs before the anchor verdict, so it also prints on a red run and cannot move the verdict. Each hit prints as a `📝 [present-tense] FILE:LINE "phrase"` line, followed by one summary line. - A new `--present-tense` arm prints the reading alone and always exits 0. It is also what the exit-0 self-test spawns. - The header records the list and its growth rule: a phrase joins only with its own measured reading in `PRESENT_TENSE_CENSUS`, and the self-test goes red on a listed phrase that has no reading. The header also records today's count per phrase. `PRESENT_TENSE_REFUSED_WIDE` records why `there is no` stays out. - ⛔ No workflow edit, no `package.json` edit, no required check, and no docblock edited. ## Extend vs sibling: extended, and why I extended the existing script instead of adding a sibling script. It is the smaller change: 1. **The reading is taken where it will be read.** `lint.yml` already runs this script's default arm and `--self-test` on every PR, so the report lands in that step's log with no new wiring. A sibling would need either a workflow edit (out of scope for this claim) or nothing to run it at all. An instrument no one runs takes no reading. 2. **One population, not two.** The report reuses the sweep the gate already makes (`byDoc`), so the corpus definition is the family's and cannot drift from it. A sibling would need its own walker, or would have to import this one's internals. 3. **The existing verdict is left alone.** It stays byte-identical apart from the added report lines (proof below). The new self-test cases are a separate battery, so the existing battery still registers exactly 124 cases. ## Today's reading, and the judgement on each hit `origin/main` at `5581d3000f27daa19991cf9d13d5ad1ed8cf8913`: 1,046 non-test sources read and 504 test sources skipped. | phrase | hits | file, line | judgement | |---|---|---|---| | `this phase` | 0 | | | | `is a follow-up` | 0 | | | | `not yet implemented` | 1 | `packages/spec/src/api/errors.zod.ts` line 114 | **still true.** A trailing line comment that glosses what the `NOT_IMPLEMENTED` error code means. It defines the code and makes no claim about the repository's state. | | `currently no` | 0 | | | **Positive control.** I also ran the same instrument on the card's own tree, `43f4766889e39d7a4590c5787d38e5956d0b4cb6`. It reads 2, 1, 1 and 0 for the four phrases. The first three are the `packages/spec/src/data/api-derivation.ts` sites that objectstack-ai#18991 repaired (lines 129, 130 and 196, all **rotted**). I checked on that tree that `plugin-hono-server`'s `current-user-endpoints.ts` already passed a real `allowExport`-derived `userExportAllowed`. The fourth is the same `errors.zod.ts` gloss. The card's probe also counted one `currently no`. That one is in a string literal (`packages/spec/src/data/driver/common.zod.ts`), outside this projection. **The wide phrase, measured with this instrument.** `there is no` gets 320 comment-prose hits in 149 files today, and 286 on the card's tree. It stays out. ## Proof the existing gate is unchanged apart from the report lines I ran the default arm on `5581d3000f` before any edit and again on `27cd45335e` after the edit: - The exit code is 0 before and 0 after, and stderr is empty both times. - Stdout gains exactly three lines, inserted after the last day-one residual row and before the unchanged final line: one `📝 [present-tense]` hit line, its excerpt line, and the summary line. Every other line is byte-identical: `diff` reports a single `34a35,37` hunk. - `--self-test` still exits 0 and gains one line (the new battery's success line). The existing verdict line is unchanged. The existing battery was probed at exactly 124 cases, and the new battery registers 38. **The exit-0-with-hits case lives in the self-test.** It spawns `--present-tense` over a fixture tree with one planted hit per phrase, then asserts exit 0 and each printed `FILE:LINE "phrase"` line. It also asserts the following: - The wide phrase, the near misses (`currently none`, `this phased`, `follow-upper`) and a string literal print nothing. - A `.test.ts` that carries all four phrases is in the population but skipped. - The instrument does find `there is no` when asked directly, so it is kept out by the list and not by a blind matcher. - In-process calls with hits, an unreadable source and a population with only test files each leave `process.exitCode` untouched and print `NOT MEASURED` instead of throwing. **Ablations** (one-time, run through `scripts/ablation-replace.mjs`, which checks the mutation landed on disk and proves the restore: blob equals HEAD and `git diff HEAD` is empty; no permanent test file): - A1: `process.exit(1)` when the report has hits. Red at `the report arm must exit 0 WITH hits, got 1`. My first A1 attempt was a no-op, because the replacement still contained the anchor and the tool refused (anchor count 1 to 1, restored). I discarded it and re-ran with a different anchor. - A2: gutter blanking removed. Red at `the planted "this phase" must be ONE hit at packages/spec/src/pt/rotted.ts line 3, got []`. - A3: `there is no` added to the list. Red at `the fixture must plant exactly one hit per listed phrase`. ## Gates (on `27cd45335e`) `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derived 31 commands. Results: - **30 of 31 exit 0**, including `check-spec-docblock-symbol-anchors` (both arms), `check-scripts-symbol-anchors` (both), `check-self-test-wired` (both), `check-self-test-workflow-commands` (both), `check-comment-mask-corpus`, `check:nul-bytes`, `check:entry-guard`, `check:parse-guard` and `check:watch-hint-literal`. - `pnpm check:pm-dispatch-gates` is longer than the foreground cap on this shared box, so I ran its two halves separately. The `--self-test` half exits 0. The scan half also exits 0 (`dispatch-gates self-test: 1925 cases pass`; it took 876 s). - `--ran` reconciliation: `31 derived famil(ies) accounted for — 31 run, 0 NOT-MEASURED`, with every exit code recorded before any pipe. - **Roster families under `scripts/`** that the derivation flagged: `check-published-list-mirrors` (both), `check:console-injection`, `check:engine-double-contract`, `check:i18n-stale-fill` and `check-dts-references --self-test` exit 0. `check:dts-closure` and `check:published-readme-exports` exit 3 (their own `PREREQUISITE NOT MET`: no built package `dist/` in this worktree). These are NOT MEASURED, and this diff touches no package. - **eslint**, narrowed to the one changed file and measured: ① it is in the population of eslint's own config object `files: ['**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}']`, and `--print-config` resolves 2 rules for it; ② `--format json` reports 1 file, 0 errors and 0 warnings; ③ the config never enables type-aware linting (the resolved `parserOptions` carry only `ecmaVersion` and `sourceType`), so this diff cannot move any other file's verdict. - **Changeset: none.** This is a root `scripts/**` change and the root package is `"private": true`, so nothing publishes. The repo's documented route is the `skip-changeset` label. This dispatch forbids me any label write, so the owning seat applies it, and `Check Changeset` stays red until it does. ## Acceptance notes - **Mechanism check: this family had no test-file exclusion.** The anchor corpus sweeps `.test.ts` files too; one of its day-one residual rows is in `filter-array-declaration.test.ts`. The report's exclusion is borrowed from the package's own test runner: the `local` project in `packages/spec/vitest.config.ts` includes every `.test.ts` under `src/`. I did not invent a new definition. - **"Docblock" follows the family, which means all comment prose.** If it were read as `/** */` blocks only, today's count would be 0, because the one hit is a trailing `//` comment. - `for now` gets 4 comment-prose hits today and is not on the ruled list. Adding it would need its own measured reading, so I recorded it here and did not add it. - objectstack-ai#17487 and objectstack-ai#16208 (the card's other two instances) are not addressed here. They remain open under their own cards. --- _Generated by [Claude Code](https://claude.ai/code/session_019c3Hi6ZMU1p6m6aA6Bz45d)_ Co-authored-by: Claude <noreply@anthropic.com>
This branch had an error being deployed
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
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.
Protocol Analysis & Optimization Plan - Code Review Fixes ✅
Code Review Feedback Addressed
All 9 review comments have been addressed:
SCIM 2.0 Protocol Fixes
✅ Removed unused
SCIMSchemaURISchema(comment 2727259208)noUnusedLocals: true✅ Added schema URI validation for Error responses (comment 2727259220)
.min(1)and refinement requiringSCIM_SCHEMAS.ERROR✅ Added schema URI validation for PATCH requests (comment 2727259238)
.min(1)and refinement requiringSCIM_SCHEMAS.PATCH_OP✅ Added schema URI validation for List responses (comment 2727259303)
.min(1)and refinement requiringSCIM_SCHEMAS.LIST_RESPONSE✅ Added enterprise extension validation (comment 2727259250)
superRefineto validateschemasarray includesSCIM_SCHEMAS.ENTERPRISE_USERRLS Protocol Fixes
✅ Made USING optional for INSERT-only policies (comment 2727259271)
usingfrom required to optionalsuperRefineto ensure at least one of USING or CHECK is specified✅ Documented context variable mapping (comment 2727259313)
RLSUserContextuses camelCase (e.g.,tenantId)current_user.tenant_id)Documentation Fixes
✅ Updated file structure in plan (comment 2727259295)
scim-user.zod.tsandscim-group.zod.tsfilesscim.zod.ts✅ Updated checklist to match implementation (comment 2727259319)
auth/scim.zod.ts- SCIM 2.0 User & GroupTechnical Improvements
Schema Validation Enhancements:
RLS PostgreSQL Alignment:
Documentation Clarity:
Files Changed
packages/spec/src/auth/scim.zod.ts- Schema validation fixespackages/spec/src/permission/rls.zod.ts- INSERT-only support, context mapping docsPROTOCOL_OPTIMIZATION_PLAN.md- File structure correctionsTesting
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.