Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Check action run status in GitHub CI
Fix sharing.test.ts for discriminated union schema
Jan 25, 2026
hotlong
marked this pull request as ready for review
January 25, 2026 02:13
hotlong
merged commit Jan 25, 2026
f475c88
into
copilot/release-new-version-please-work
5 checks passed
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the test suite in sharing.test.ts to align with schema changes that introduced a discriminated union between CriteriaSharingRuleSchema and OwnerSharingRuleSchema. The changes ensure all 24 tests pass with the new schema structure.
Changes:
- Updated
sharedWithfrom simple string to structured object withtypeandvaluefields - Renamed
criteriafield toconditionfor criteria-based rules - Added
ownedByfield for owner-based rules - Removed
'manual'and'guest'as valid rule types (now only'owner'and'criteria') - Added
'full'as a valid access level alongside'read'and'edit'
| }); | ||
|
|
||
| it('should accept manual sharing rule', () => { | ||
| it('should accept user-specific sharing rule', () => { |
There was a problem hiding this comment.
The test name "should accept user-specific sharing rule" is misleading. This test is actually validating a criteria-based sharing rule that happens to share with a user recipient. The test name suggests it's testing a different rule type, but 'user-specific' is not a rule type in the schema - it's just using 'user' as a recipient type. Consider renaming to something like "should accept criteria rule with user recipient" to more accurately describe what's being tested.
Suggested change
| it('should accept user-specific sharing rule', () => { | |
| it('should accept criteria rule with user recipient', () => { |
| expect(rule.sharedWith.type).toBe('user'); | ||
| }); | ||
|
|
||
| it('should accept guest sharing rule', () => { |
There was a problem hiding this comment.
The test name "should accept guest sharing rule" is misleading. After the schema changes, 'guest' is no longer a sharing rule type - it's now just a recipient type within the ShareRecipientType enum. This test is actually validating a criteria-based sharing rule that shares with guest recipients. Consider renaming to something like "should accept criteria rule with guest recipient" to accurately reflect the current schema structure.
Suggested change
| it('should accept guest sharing rule', () => { | |
| it('should accept criteria rule with guest recipient', () => { |
Comment on lines
288
to
309
| it('should reject sharing rule without required fields', () => { | ||
| expect(() => SharingRuleSchema.parse({ | ||
| object: 'account', | ||
| sharedWith: 'group_id', | ||
| type: 'criteria', | ||
| condition: 'status = "Active"', | ||
| sharedWith: { type: 'group', value: 'group_id' }, | ||
| })).toThrow(); | ||
|
|
||
| expect(() => SharingRuleSchema.parse({ | ||
| name: 'test_rule', | ||
| sharedWith: 'group_id', | ||
| type: 'criteria', | ||
| condition: 'status = "Active"', | ||
| sharedWith: { type: 'group', value: 'group_id' }, | ||
| })).toThrow(); | ||
|
|
||
| expect(() => SharingRuleSchema.parse({ | ||
| name: 'test_rule', | ||
| object: 'account', | ||
| type: 'criteria', | ||
| condition: 'status = "Active"', | ||
| })).toThrow(); | ||
| }); |
There was a problem hiding this comment.
The test coverage for required fields should include validating that discriminated union requirements are enforced. Specifically, missing tests for:
- Owner-based rule without
ownedByfield should be rejected - Criteria-based rule without
conditionfield should be rejected
These are critical validations for the discriminated union schema. Consider adding test cases like:
- Testing that
{ name: 'test', object: 'account', type: 'owner', sharedWith: {...} }(missingownedBy) throws an error - Testing that
{ name: 'test', object: 'account', type: 'criteria', sharedWith: {...} }(missingcondition) throws an error
Comment on lines
+196
to
220
| it('should accept user-specific sharing rule', () => { | ||
| const rule = SharingRuleSchema.parse({ | ||
| name: 'manual_share', | ||
| name: 'user_specific_share', | ||
| object: 'opportunity', | ||
| type: 'manual', | ||
| type: 'criteria', | ||
| condition: 'stage != "Closed Won"', | ||
| accessLevel: 'edit', | ||
| sharedWith: 'user_john_doe', | ||
| sharedWith: { type: 'user', value: 'john_doe' }, | ||
| }); | ||
|
|
||
| expect(rule.type).toBe('manual'); | ||
| expect(rule.sharedWith.type).toBe('user'); | ||
| }); | ||
|
|
||
| it('should accept guest sharing rule', () => { | ||
| const rule = SharingRuleSchema.parse({ | ||
| name: 'public_access', | ||
| object: 'knowledge_article', | ||
| type: 'guest', | ||
| type: 'criteria', | ||
| condition: 'published = true', | ||
| accessLevel: 'read', | ||
| sharedWith: 'guest_users', | ||
| sharedWith: { type: 'guest', value: 'guest_users' }, | ||
| }); | ||
|
|
||
| expect(rule.type).toBe('guest'); | ||
| expect(rule.sharedWith.type).toBe('guest'); | ||
| }); |
There was a problem hiding this comment.
The tests don't cover the 'role_and_subordinates' recipient type, which is defined in ShareRecipientType enum (sharing.zod.ts:41). Consider adding a test case to validate that sharing rules can use this recipient type, e.g., sharedWith: { type: 'role_and_subordinates', value: 'sales_manager' }
This was referenced Sep 13, 2026
os-bill
pushed a commit
that referenced
this pull request
Sep 13, 2026
… shape and require `reference` on a `lookup` screen field Maintainer ruling A′ (decision batch #130 item 1, 2026-09-13, verbatim 「同意」), applied across the five items it names. 1. Server-side enforcement at resume STAYS — `min_value` / `max_value`, both already in the ADR-0114 D2 catalog, no new error code. 2. The string gap closes. `validateScreenInputs` gains its own pass, BEFORE the bound: a present value for a `type: 'number'` screen field that is not a finite JSON number is refused with `invalid_type` — ⛔ never coerced. The bound pass compares numbers, so before this every non-number satisfied it by never reaching it (`"25"` under a `max` of 20 was conformant). The pin that recorded that silence is INVERTED in place, not deleted, so a later re-widening has to come back through it. Narrow in two directions on purpose: it keys off `type: 'number'` and not off the presence of a bound, and it is presence-conditioned exactly as the bound is. 3. `ScreenFieldConfigSchema` requires `reference` when `type` is `lookup`, via a `superRefine` that leaves `.shape` enumerable and the key set unmoved. This REVERSES the optionality the card first shipped; ADR-0078's own example of silently-inert metadata is a `lookup` with no `reference`, and a degraded shipped twin is not a reason to bend the contract to it. A stored bare lookup has NO lossless conversion — nothing in the metadata says which object the author meant — so it registers as an ADR-0087 SEMANTIC entry (`screen-field-lookup-reference-required`, protocol 18), ⛔ never a D2 conversion that would have to invent a target. The entry is one file under `migrations/entries/semantic/`; `registry.ts` is its GENERATED projection (`gen:migration-registry`), never hand-merged. 4. The wording items, in every carrier: the bound's "re-checked when the submitted value is a number" qualifier is gone from the changeset, the flows guide, the generated node-config reference, both `.describe()` pairs, the `ScreenFieldSpec` doc block and the Studio designer form, because the qualifier is no longer true. `reference`'s requirement is stated wherever its optionality was. 5. `check:reference-carrier-shape` is green (exit 0): both `reference` sites this PR introduced reach their value through a name, which is the population the gate documents as unjudged. ⛔ No path ignore, and the `['a']` fixture still tests what it tested — it widened to four shapes, including the `{ object: 'x' }` carrier shape the gate exists for. Also repaired, each falsified by the above rather than pre-existing: - The changeset declared no break. It now carries `**BREAKING**` and the `<!-- adr-0087: registered screen-field-lookup-reference-required -->` disposition marker — a semantic entry with no declaration on the changeset is exactly what `check-adr-0087-registration` exists to notice. `minor` stays: the launch-window guard keeps breaks off `major` outside pre-mode. - `ScreenInputIssue.code` enumerated `required` and `unknown_field` only and spoke of "the same two conditions". This PR put three more codes through that field. - `ScreenFieldSpec.reference` claimed absence was "what every `lookup` screen field did before this key existed". It now states that the authoring schema refuses that shape, and why the WIRE type stays optional: a run suspended before the upgrade rehydrates a `ScreenSpec` stored under the old accept set. - `api-surface/automation.json` and `export-origins/automation.json` were stale — the new exported refusal constant had never been propagated. Both regenerated with the repo's own generators; one additive line each. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MkQhmuuJAVDjmeWNixwDDH
akarma-synetal
pushed a commit
to akarma-synetal/framework
that referenced
this pull request
Sep 17, 2026
…a lookup target (objectstack-ai#17913) Part of objectstack-ai#17306 Clause-②: yes `ScreenFieldConfigSchema` was `.strict` over exactly `name`/`label`/`type`/`required`/`options`/`defaultValue`/`placeholder`/`visibleWhen`, so three ordinary authoring intents had **no expression at all** — a numeric bound, help text, and a lookup target. They did not degrade quietly: `max`, `helpText` and every lookup-target spelling were refused BY NAME. But a loud refusal with no landing key is still a dead end, and the reference app worked around all three in prose. Implements **two** director-seat rulings on this card, and the second reverses two choices the first head of this PR had made: - **A** — decision batch objectstack-ai#120 item 2, 2026-09-12, maintainer 「17508 A AI负责翻译就行。**其他同意**」: the three intents land, spelled with `FieldSchema`'s own key names, and only together with their rendering. - **A′** — decision batch objectstack-ai#130 item 1, 2026-09-13, maintainer 「同意」: (1) server-side bound enforcement at resume stays; (2) the string gap closes — a value for a `number`-typed screen field that is not a JSON number is **refused**, ⛔ never coerced and ⛔ never silently passed; (3) `reference` is **REQUIRED** on `type: 'lookup'`, carried by an ADR-0087 **semantic** migration entry.⚠️ **A′ is what makes this release breaking, and everything below describes head `68d0ce67114`.** An earlier head of this same PR shipped the opposite on both points and pinned the opposite in tests; both assertions were inverted in place, with the reversal named at the assertion, so a later re-widening has to come back through them. ## The three spellings, and why each is the corresponding `FieldSchema` key The ruling requires names **derived from `FieldSchema`**, never invented. Read from `packages/spec/src/data/field.zod.ts`: | Intent | Adopted | Derived from | Why this is the corresponding key | |:---|:---|:---|:---| | Numeric bound | `min` / `max` | `FieldSchema.min` / `.max` (`field.zod.ts:1121-1122`) | Unambiguous — `FieldSchema` declares exactly these two names for a numeric bound. | | Help text | `inlineHelpText` | `FieldSchema.inlineHelpText` (`:1736`) | **Ambiguous, resolved below.** | | Lookup target | `reference` | `FieldSchema.reference` (`:1213`) | The canonical key; `FieldSchema` renames `relatedTo`/`referenceTo`/`target`/`targetObject`/`lookupObject` onto it (`:929`). The card's proposed `object`/`reference_to`/`referenceTo` are all non-canonical. | ### The one ambiguous case, and the test applied `FieldSchema` declares **two** plausible help-shaped keys: - `description` (`:1001`) — described as *"Tooltip/Help text"* - `inlineHelpText` (`:1736`) — described as *"Help text displayed below the field in forms"* **Test applied — the ruling's own "same spellings the object field uses":** ask what `FieldSchema` answers an author who reaches for this intent by its natural name. Its alias table (`:921`) reads `help: 'inlineHelpText', helpText: 'inlineHelpText', hint: 'inlineHelpText', tooltip: 'inlineHelpText'` — so the object field *itself* routes all four natural spellings, including the `helpText` this card asked for, onto `inlineHelpText`. `description` is never an alias target for them; it is a separate declared key for secondary/tooltip copy. ⇒ `inlineHelpText` is the spelling the object field uses for *this* intent. ## Reproduction of the gap (before the change) Twelve spellings against `ScreenFieldConfigSchema` on `origin/main`, with `placeholder` as the lit control: ``` [max] -> unrecognized_keys: Unrecognized key(s) on this screen field: `max`. [helpText] -> unrecognized_keys: ... `helpText`. [inlineHelpText] -> unrecognized_keys: ... `inlineHelpText`. [reference] -> unrecognized_keys: ... `reference`. [object] -> unrecognized_keys: ... `object`. (+ min, step, help, hint, description, referenceTo, targetObject) [placeholder CONTROL] -> __ACCEPTED__ ``` The control could have come back the other way, and it is aimed at this schema's key set — which is what makes the twelve refusals a reading. The card's claim that `max` and `helpText` are refused **by name** is confirmed. ## What lands Four keys (the bound pair counts as one intent, two keys): - **`min` / `max`** — forwarded onto `ScreenFieldSpec` for the client **and enforced server-side on resume** by `validateScreenInputs` (`min_value` / `max_value`). A screen field's declared contract is the only contract behind it, so a bound the dialog alone applied would be bypassed by any caller posting to `resume` directly — the gap objectstack-ai#4477 closed for `required`. **That guarantee holds for every submitted value, with no exception carved out for shape**, because the value SHAPE is checked first and in its own pass: on a `type: 'number'` field a present value that is not a finite JSON number is refused with `invalid_type`, ⛔ not coerced (ruling A′). All three codes — `invalid_type`, `min_value`, `max_value` — are existing ADR-0114 D2 catalog members: **no new error code**. - **`inlineHelpText`** — help under the input, distinct from `placeholder`, which the browser clears the moment the user types. That is precisely the carrier the reference app was forced to overload for a constraint that has to stay readable. - **`reference`** — the lookup target, so the field can resolve a record picker. **Required when `type` is `lookup`**, as it is on an object field (`field.zod.ts:1196-1199`: a `lookup`/`master_detail` whose `reference` is missing, empty or whitespace-only is refused at parse time). Optional in the shape, required by a `superRefine`, so the key set does not move and `.shape` stays enumerable. **Delivered with its rendering inside this repo.** The executor forwards all four and the Studio designer form offers all four as repeater columns; `builtin-node-form-zod-ledger.test.ts` reconciles the two key sets against the Zod **in both directions**, so a key declared here and absent from the form fails that test rather than shipping as a field nobody can author. The console dialog half is objectui#9248, which this card's acceptance includes. ## What this release BREAKS ⛔ This is **not** a purely additive release. The changeset declares **BREAKING** in two places and grades `minor` on both packages — `minor` because the launch-window guard (`check-changeset-no-major`) keeps breaking changes off `major` outside pre-mode, **not** because the narrowing is small. Under strict semver it would be major, and the changeset says so in its own words. Precedent: `schedule-flow-acting-organization-required` (`323da7a30d0`). **Break 1 — a stored bare `lookup` screen field parsed before and is refused now.** `ScreenFieldConfigSchema.superRefine` refuses `type: 'lookup'` with an absent or blank `reference`, addressed to `['reference']`, with the exported `SCREEN_FIELD_LOOKUP_REFERENCE_REQUIRED` as its message. A picker with no target object resolves nothing — ADR-0078's own worked example of silently-inert metadata — and ruling A′ held that a degraded shape which already ships is not a reason to bend the contract to it. The reference app's own hotcrm `close_case` "Resolved by Article" field is exactly such a site; it names `crm_knowledge_article` when it adopts this version (relayed to the hotcrm seat; the director seat has no write access there). There is **no lossless conversion** — nothing in the stored metadata says which object the author meant — so this is an ADR-0087 **semantic** entry (`screen-field-lookup-reference-required`, step 18, the unreleased major), a structured TODO naming the flow and the field for a human to answer, and ⛔ never a D2 conversion that would have to invent a target. The changeset carries the disposition marker and `registry.ts` is regenerated by the repo's own generator, byte-identical. **Break 2 — a resume bag accepted before can be refused now.** A non-number submitted for a `type: 'number'` screen field is refused on resume with `invalid_type` instead of passing silently. Before this, the bound pass compared numbers, so a value that never reached it satisfied it: under a `max` of `20` the string `"25"` was conformant. It was never doing what its author declared. **Everything else is additive.** The other three keys widen the accept set for every field; the bound itself fires only on a field that declares one, which nothing did before this release; and a `lookup` field that already names its target parses byte-identically.⚠️ **In-flight runs.** The WIRE type `ScreenFieldSpec.reference` stays optional on purpose: a run suspended at a screen **before** the upgrade rehydrates its `ScreenSpec` from context stored under the old accept set. Such a client has no target to resolve a picker against and falls back to a plain input — the pre-objectstack-ai#17306 behaviour, kept reachable rather than turned into a client-side crash. Drain or re-drive those runs rather than assuming the fix reaches them retroactively; the migration entry's acceptance criteria says so too. ## Both-direction pins Accepted: all four keys; the two hotcrm acceptance fixtures as declared metadata (`quote_generation`'s discount ceiling, `close_case`'s lookup **with** its target); a field with no `reference` on **every other** `type` — the requirement reads one member of an open widget vocabulary, so `undefined`/`text`/`number`/`select` are untouched. Still refused: a `lookup` with an absent, empty or whitespace-only `reference`, **addressed to `reference`** and carrying the exported message (which is also pinned to name the key, show the spelling and carry no tracker id); an undeclared key **by name** (`sparkles`) — a `.strict` schema that quietly widened past these four is the regression that catches; a non-number bound (`max: '20'` → `invalid_type`); a non-string `inlineHelpText`; and a non-string `reference` across four shapes (`['a']`, `{ object: 'crm_account' }`, `42`, `true`), each required to fail **on `reference`** with `invalid_type` rather than incidentally. Plus an **exact key-set pin** enumerating all twelve declared keys, which is the only thing that can see a fifth key arriving, and a pin that `.shape` stays enumerable — `superRefine` is a CHECK, not a wrapper, and a `ZodEffects` here would break the ledger test silently by making the key set unreadable rather than wrong. Refusal quality: `help`/`helpText`/`hint`/`tooltip` are refused **naming `inlineHelpText`**, and `object`/`referenceTo`/`targetObject`/`lookupObject`/`relatedTo`/`target` **naming `reference`**.⚠️ `object` means different things one level apart — on the screen **node** it renames to `objectName`, on a screen **field** it can only mean the lookup target — and a pin holds both readings apart. ## Ablation — measured on the pre-A′ tree, ⛔ NOT re-run at this head Two legs on `screen-input-contract.ts`, each proven on disk by occurrence count **and** `git hash-object`, with a `trap` restore: | Leg | Mutation | Result | |:---|:---|:---| | **A — main** | disable the `max` check | **3 pins RED** incl. *"refuses a value above `max`"*; restored (hash `1cb0df1…` == HEAD blob, `git diff HEAD` 0 paths) → **8/8 GREEN** | | **B — cost** | drop the hidden-field guard from the bound loop (over-refusal) | **exactly 1 pin RED** — *"does not fire on a field the user was never shown"*; restored by hash → **8/8 GREEN** | Leg B is the cost direction: it proves the pin that guarantees the bound does **not** start refusing values the user was never asked for.⚠️ **Both legs were run on the pre-A′ tree, when that file carried 8 pins.** At `68d0ce67114` it carries **10**: A′ added the value-domain pin and the shape/`required` split, and inverted the pin that used to assert the silence (*"does not fire on a non-numeric value"* → *"refuses a non-number for a `type: 'number'` field — the string gap is closed"*). The `8/8` totals above therefore describe a superseded tree. **NOT MEASURED at this head:** the two legs have not been re-run against the 10-pin file, and the A′ guards (the shape pass, the `lookup` refinement) have no ablation of their own. Their pins pass at head and in CI; that is coverage, not an ablation reading. ## One stale claim corrected, because this change falsified it The flows translation surface documented `help`'s exclusion as *"`ScreenFieldConfig` declares nothing help-shaped at all"* — in `translation.zod.ts`'s guidance string (which enumerated the old key set verbatim), its doc block, `i18n-resolver.ts`'s `FLOW_SCREEN_FIELD_COPY_KEYS`, and `packages/spec/liveness/translation.json`. The screen field now declares `inlineHelpText`, so the copy is real. The exclusion **stands** — the flows bundle still carries `label`/`placeholder` only, and growing that face is a ruled step against the objectstack-ai#7646 enumeration, not a resolver-side accretion — but its reason is now stated as a not-yet instead of telling an author the field has no help copy when it has. ⛔ No translation key added, no resolver behaviour moved. ## A pin caught a real divergence mid-round `automation-api.zod.test.ts` binds `TriggerFlowResponse['data']` **equal** to the `AutomationResult` contract interface. Widening `ScreenFieldSpec` alone reddened it by name (TS2344) — exactly the two-files-one-shape drift that pin exists to catch. The module-local `screenFieldSpecShape` now carries the same four keys. ## Generated artifacts `check:generated` is **0** at this head (it was 1 before the repair round below found it). The generated delta against the merge base: | Artifact | Delta | |:---|:---| | `authorable-surface/automation.json` | **+4 keys** — `inlineHelpText`, `max`, `min`, `reference` on `automation/ScreenFieldConfig`, and nothing else | | `api-surface/automation.json` | +1 — `SCREEN_FIELD_LOOKUP_REFERENCE_REQUIRED (const)` | | `export-origins/automation.json` | +1 — the same export, origin `src/automation/builtin-node-config.zod.ts` | | `src/migrations/registry.ts` | +1 entry under step 18 | | `content/docs/references/automation/builtin-node-config.mdx` | **+8 rows** (the four keys in each of the two screen-field tables), additive only | Registry wiring was verified generated, ⛔ not hand-typed: `registry.ts` copied aside, `gen:migration-registry` re-run (exit 0), `diff` exit 0 — byte-identical. ## Round history on this PR | Round | Head | What it did | |:---|:---|:---| | Initial | `e32ab94fb9b` … `6db3438f267` | The four keys, the wire binding, the pins. | | **R1** | `17db348fc3d` | `FLOW_SCREEN_FIELD_NO_HELP` (`translation.zod.ts`) carried an internal issue id inside text printed **at the author**; `Doc/skill authoring guard` refuses it with no per-string exemption by design — this was the CI red. The id was dropped from the string; the doc block above it still names the card for an internal reader, which the guard permits. The `translation.test.ts` twin keeps pinning the new wording and gained the prescribed **negative pin** — the refusal must not match an issue id. | | **R2** | `17db348fc3d` | `packages/spec/liveness/translation.json` was a **fourth** site still claiming the screen field *"has no help-shaped key at all"*. Verified hand-kept (only `state-counts.md` is generated under `liveness/`). Restated the way the three corrected sites are. | | **R3** | `d5843a793d6` |⚠️ **Superseded by A′ — see below.** This round corrected prose to state a numeric-value limit that A′ then removed from the product. | | Carrier gate | `c6ea7ea54a3` | Made both new `reference` sites legible to `check-reference-carrier-shape` (see below). | | **A′** | `68d0ce67114` | Enforced the value shape and made `reference` required on `lookup`; added the ADR-0087 semantic entry, the BREAKING declaration and the disposition marker; inverted the two pins that recorded the old behaviour. |⚠️ **R3 is the one round this body previously described as current, and it no longer is.** R3 read the resume loop as firing only on `typeof value === 'number' && isFinite`, called *"a caller that skips the dialog is refused too"* an overclaim, and edited six carriers — the changeset, `content/docs/automation/flows.mdx`, both `.describe()` pairs, the `ScreenFieldSpec` doc block, the `ScreenFieldConfigSchema` bound doc block, and the Studio designer form's `min`/`max` column descriptions — to state the numeric-value limit, leaving the loop untouched. **Ruling A′ closed the gap in code instead.** At this head those carriers state the guarantee unqualified again, and the designer columns read *"Enforced when the run resumes."* with no *"for a submitted value that is a number"* clause. The loop is **not** untouched: a value-shape pass now runs before the bound pass. ### R1 reproduction, before and after ```text before node scripts/check-doc-authoring.mjs -> exit 1 ✗ Internal issue-id reference(s) in CUSTOMER-FACING spec text: packages/spec/src/system/translation.zod.ts:622 [via FLOW_SCREEN_FIELD_NO_HELP] after node scripts/check-doc-authoring.mjs -> exit 0 ✓ 15163 customer-facing string(s) across 949 spec sources clean ``` (The `after` counts are the reading at **this** head; the R1 round measured 15160 / 948 on its own, smaller tree.) ### The second red is closed `check-reference-carrier-shape` was RED at `d5843a793d6` (exit 3, *"could not measure"*) on two sites this PR introduced: the designer form's `reference` column definition (`screen-nodes.ts`) and the deliberate non-string `reference` fixture in `builtin-node-config.test.ts`. Ruling A′ item 4 dispatched it: test the gate's third remedy first, ⛔ never change the `['a']` fixture to silence the gate, ⛔ never a path ignore. `c6ea7ea54a3` did that. The third remedy is **not** satisfiable at the designer-form site — the holder is a JSON-Schema `properties` map whose own `type` key holds a sub-schema, and all twelve of its keys are `data/Field` authorable keys, so no key can prove it is not a field definition. Both values are therefore reached through a **name** (`LOOKUP_TARGET_COLUMN`, `NON_STRING_LOOKUP_TARGETS`), which the gate documents as unjudged because it judges literals only. The gate is **unedited**, no path ignore was added, and the fixture was **widened** from one shape to four with a **stricter** assertion, never weakened. It exits **0** at this head. ⛔ **Not fixed here, for the seat to dedupe:** `check-reference-carrier-shape` has no holder rule for a JSON-Schema `properties` map, and its literal-only predicate leaves any named value unjudged. Patching a gate from inside a feature PR is the wrong place for it. ## Verification at head `68d0ce67114`⚠️ Every row below is anchored to **this** head. An earlier version of this body reported readings taken at `d5843a793d6`, two commits back. **CI on this head** (35 check-runs): **33 success, 2 skipped, 0 failure.** The two skips are `Console Pin Gate` (path-skipped — `.objectui-sha` and `build-console.sh` are untouched) and `Packed-tarball smoke (opt-in)`. `Lint & Repo Gates`, `TypeScript Type Check` (the job carrying the eight generated-artifact gates) and `Build Core` are all green. **Implementing seat, locally on this head:** | What | Exit | Reading | |:---|:---:|:---| | `pnpm --filter @objectstack/spec test` (`--project local`) | 0 | 474 files · **13497** tests | | `pnpm --filter @objectstack/spec test:repo` (`--project repo`) | 0 | 31 files · 523 tests | | `pnpm --filter @objectstack/service-automation test` | 0 | 133 files · **1574** tests | | both packages' `typecheck` | 0 | clean | | `check:generated` (15 artifacts) | 0 | was 1 before the repair round | | `check-adr-0087-registration` · `check:migration-registry` · `check:docs` · `check-doc-authoring` · `check-reference-carrier-shape` (+ every `--self-test`) | 0 | — | **At-tier contract review, independently on this head** (comment 5652925641, detached worktree, fresh install, 21/21 build closure): `check-adr-0087-registration --base origin/main` exit 0 — **1 declared-breaking changeset, `registered screen-field-lookup-reference-required`**, so the gate now has an input where the earlier green was vacuous; `check-changeset-no-major` exit 0 with the LEVEL AXIS clean; `check-empty-changeset` exit 0; `check-reference-carrier-shape` exit 0 (6685 files, 674 `reference` sites, 0 non-string literals); `check-doc-authoring` exit 0 (15163 strings / 949 sources); the three suites and `service-automation typecheck` exit 0, with the built `dist/automation/index.d.ts` carrying `inlineHelpText` as the positive control. **NOT MEASURED** (neither a pass nor a finding): the ablation legs at this head (above); `@objectstack/spec typecheck` in the review's container (SIGTERM); and five derived gate commands that exit non-zero for **checker-health or unbuilt-prerequisite** reasons rather than on this diff — `check:dual-build-cjs-loads`, `check:i18n-walk-parity`, `check:skill-examples` (all `PREREQUISITE NOT MET` on unbuilt output outside this change's dependency closure), `check:role-word` (its own `--self-test` fails in that container), `check:pm-prior-rulings` (script absent at this head). All of them live in jobs that build first, and CI is green on this head. ## 维护者速读(草稿) **改了什么** — 流程界面的输入框,现在能表达三件以前完全没法表达的事:数字上下限、一句帮助说明、以及查找框要从哪个对象里选记录。三个键的**名字全部沿用对象字段已有的拼法**(`min`/`max`、`inlineHelpText`、`reference`),没有发明新名字。同时收紧了两处旧行为:查找框**必须**写明目标对象;数字字段在流程恢复时提交的值**必须真的是数字**。 **为什么改** — 以前作者想写这三件事,只会被拒收,而且没有任何可用的替代键。参考应用只能把折扣上限写进标签和占位符里,把查找框降级成"请人工输入记录 id"。也就是说,我们自己的样板应用在用变通手段绕过我们自己的契约。至于那两处收紧:上下限如果只有对话框在管,绕过对话框直接提交就能突破,等于那条规则形同虚设;而没有目标对象的查找框,渲染出来就是一个查不到任何记录的选择器 —— 这正是 ADR-0078 拿来举例的"解析得过、却什么都不做"的元数据。 **风险与代价(含回滚)** — ⛔ **这是一次破坏性变更 —— 不是只往外加键。** changeset 两处写明 BREAKING,版本级别按发布窗口惯例记为 `minor`(门禁在非 pre 模式下不允许 `major`),按严格语义化版本它应当是 major,changeset 里也是这么写的。 具体到作者会遇到什么: 1. **已经写好的、没有写 `reference` 的 `lookup` 界面字段,从这个版本起解析会直接失败**,报错指向 `reference` 这个键并给出正确写法。⛔ 没有任何自动迁移能修它 —— 元数据里根本没有记录作者当初想指向哪个对象,所以升级链里放的是一条**结构化 TODO**(点名是哪个流程、哪个字段),由人来逐条回答。我们自己的参考应用 hotcrm 的 `close_case` 就是这样一处,它在采用这个版本时会补上 `crm_knowledge_article`。 2. **数字字段在恢复运行时,提交 `"25"` 这种数字字符串会被拒收**,以前是静默放过。⛔ 不做隐式转换 —— 把 `"25"` 偷偷读成 `25` 会让上下限的判定依赖一个契约里没声明过的转换。以前能被接受的提交,现在可能被拒。 3. **升级时正卡在界面步骤上的在途运行**,它们的界面定义是升级前存下来的旧形状,不会被追溯修正 —— 需要放干或重新驱动这些运行,不要假设修复能自动追上它们。 回滚代价:三个新键一旦发布,删键很贵;两处收紧则可以单独放宽(两条对应的测试都是**就地反转**而不是删除,所以任何一次回头放宽,都必须从那两条断言前面经过,不会悄悄发生)。 **席位意见** — (待席位填写) **你要做的(一个动作)** — 审阅这次公开面的扩宽与两处收紧是否如你所愿。⚠️ 请特别确认第 1 条的代价你能接受:已上线的流程如果有没写目标对象的查找框,升级后会解析失败,且只能人工逐条修。另外,objectui#9248(console 对话框渲染这三个键)落地之前,本卡不应关闭 —— PR 正文用的是 `Part of`,不是 `Fixes`。 --- _Generated by [Claude Code](https://claude.ai/code)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
akarma-synetal
pushed a commit
to akarma-synetal/framework
that referenced
this pull request
Sep 17, 2026
… not add (objectstack-ai#17712) (objectstack-ai#18146) Closes objectstack-ai#17712 `Clause-②: no` — this card only makes a gate refuse MORE. It loosens no accept set and widens no published surface. Implements **item 1 (A′)** of the ruling on objectstack-ai#17712 (director seat, decision batch objectstack-ai#130 item 3, 2026-09-13; maintainer's verbatim reply 「同意」). Item 2 (the `ISSUE-SLUG.md` naming line in governed dev-round guidance) is objectstack-ai#17748's and is not touched here. Item 3 — legacy names untouched, diff shape only — is honoured: nothing in this diff reads a changeset filename. ## The rule A PR may not MODIFY or DELETE a `.changeset/*.md` that exists on the merge base and was not added by this PR. Refused by name, one remedy: **rename yours; restore theirs from base**. "Added by this PR" is the file's absence on the merge base — which is exactly what git's status letters already say, so the rule needs no second reading and no filename predicate at all: | row | meaning | verdict | |:--|:--|:--| | `A` | absent at the merge base | this PR's own file, always ok | | `M` | present at the merge base, changed here | foreign, REFUSED | | `D` | present at the merge base, gone here | foreign, REFUSED | ## Where it is wired, and why Into `scripts/check-empty-changeset.mjs`, as a second axis answered in the same run — the shape `check-changeset-no-major.mjs` already uses for its two axes. **The PR touches no workflow file at all**, so it can be armed normally. Three things that come for free from that placement, each of which a standalone script would have had to re-derive: 1. **The `changeset-release/main` exemption.** `changeset version` DELETES every pending changeset, so the Version Packages PR is all-`D` rows. Outside `changeset-check`'s job-level `if:`, this gate would be structurally unsatisfiable on every release PR — the objectstack-ai#4422 / objectstack-ai#4894 shape, pointed at the release train. 2. **The merge base.** The `changeset-check` job already derives `MERGE_BASE` and hands it to this script. That matters more for this rule than for the others: run two-dot against a moving base tip instead of the merge base and every changeset `main` gained while the PR sat open reports as a `D` on this branch — an author refused by name for deleting files they never touched. The self-test pins that false red as a firing control beside the real reading. 3. **A self-test that cannot be skipped.** `check-empty-changeset.mjs --self-test` runs unconditionally in `lint.yml` (objectstack-ai#6509), outside the `skip-changeset` exemption — which is where this gate's own fixtures need to be, since a PR editing a CI-internal script is the textbook `skip-changeset` case. The file name still names rule 1 only. Renaming it would mean editing the workflow steps that spawn it; the header says so rather than leaving it to be noticed. **Rename detection is OFF for this pass** (`--no-renames`), the opposite of the `AMR` choice rule 1 makes in objectstack-ai#7045 — and for the same underlying reason, one letter over. Renaming somebody else's changeset DELETES their release note at its path; with detection on, that deletion folds into an `R` row and disappears. With it off the same edit reports `D theirs` + `A yours` and the `D` is refused. It costs nothing in the other direction: a PR renaming its OWN changeset across commits still shows one `A` row, because the old path was never on the merge base either. ## The four ruled cases, end to end on this repository Driven on a throwaway worktree at this branch's tip, against real commits (the gate reads commits, never the working tree): | case | diff rows vs merge base | exit | |:--|:--|--:| | foreign `M` — overwrite a sibling's changeset | `M .changeset/12271-published-entry-no-auto-transpile.md` | **1** | | foreign `D` — delete theirs, add mine | `D .changeset/12271-...md` + `A .changeset/17712-demo-only.md` | **1** | | own `M` across commits — add in commit 1, reword in commit 2 | `A .changeset/17712-demo-only.md` | **0** | | new `A` | `A .changeset/17712-demo-only.md` | **0** | The refusal, verbatim: ``` This PR changes a changeset it did not add: .changeset/12271-published-entry-no-auto-transpile.md present on the merge base and DELETED by this PR -- this is somebody else's release note Remedy: rename yours; restore theirs from base. ``` plus a `::error file=...` annotation carrying the same remedy, so it lands on the diff rather than only in a log. ## Ablation — the currently-false sentence this makes true Same two fixtures, with `scripts/check-empty-changeset.mjs` restored to its `origin/main` blob (`1c5638ace2`, marker count for `scanForeign` 0 on disk; restored to `24739e0fad`, marker count 12, `git diff HEAD` empty): | fixture | pre-change gate | post-change gate | |:--|--:|--:| | overwrite a sibling's changeset | **exit 0** | exit 1 | | delete theirs + add mine | **exit 0** | exit 1 | ## Reverse-read, both directions **Made false.** "`check-empty-changeset.mjs` exits 0 on any diff whose `.changeset/*.md` rows are all non-empty at head." It no longer does — case `M` above shows rule 1 printing its own green tick on the same run that exits 1. **Made true.** "A PR that changes or deletes another PR's release note is refused by name, with a remedy." Previously false in every member of the family: the only filename predicate in the changeset gates is prefix + extension, and `scan()`'s `--diff-filter=AMR` has no `D` at all, so a deletion was invisible to all three. Pinned as an assertion, not just asserted here: the foreign-`D` battery case also runs `scan()` on the same fixture and requires **zero** rule-1 violations — the control that this rule is not redundant with the one beside it. ## One premise in the card is not exactly right, measured The card says the collision would pass "with **every gate green** on both sides". Measured on real fixtures against `pr-automation.yml`'s own `--diff-filter=A` count: | shape | `ADDED` | pre-change verdict | |:--|--:|:--| | overwrite theirs, add nothing of your own | **0** | already RED today — but as *"this PR doesn't add a changeset"* | | overwrite theirs AND add your own | 1 | green | | delete theirs, add your own | 1 | green | So the narrowest shape of the incident was already refused — by the wrong gate, naming the wrong problem, and with a remedy ("add a changeset") that leaves the overwrite in place. The two shapes that actually reach `main` were green. The card's conclusion stands; its "every gate" is one row too strong, and the correction is recorded here rather than left for the next reader of that thread. ## Self-test New battery `A' (objectstack-ai#17712): a changeset the PR did not add is neither modified nor deleted` — **29 cases**, pinned in `SELF_TEST_BATTERIES` and reached (the roster's own size floor moves 21 → 22). The battery-floor machinery (objectstack-ai#13489) is what makes "every case held" distinguishable from "the cases never ran". Beyond the four ruled cases it covers: renaming a foreign changeset (with the rename-detection-ON control proving the `R` row really does swallow the `D`); renaming your own changeset across commits; `.changeset/README.md`; a nonsense control (a diff touching no changeset); objectstack-ai#6129 in this rule's direction, firing control first; objectstack-ai#4690 (no merge base throws, never falls back); and that the rendered report names the file and carries the remedy verbatim in both the body and the annotation. Every green in the battery carries a control that the fixture did what it claims — a `0` from a diff that was empty for an unrelated reason is a reading taken against nothing, not a pass. ``` ✓ check-empty-changeset --self-test: 147 assertions over real temp git repos (real scan() path) ``` ## Verification At `eeb4012a17`: - **`node scripts/pm/dispatch-gates.mjs --ran` — 34 derived families, 34 run, 0 NOT-MEASURED (derived, every entry carrying its exit code), 0 UNRUN.** All 34 exit 0. Re-derived after `git fetch origin main`; the set did not move. - **`npx eslint . --no-inline-config --format json` — exit 0 over 6751 files, 0 errors, 0 warnings.** The whole-repo run, not a narrowing: the population is eslint's own count from its `--format json` output, and this file is in it. - Control-character self scan over the edited file: 0 matches (firing control on a vertical tab: 1 match). - `node scripts/check-empty-changeset.mjs --base origin/main` on this branch: both axes green. No package is touched, so there is no dependency closure to build and no package test suite affected; `scripts/` is not TypeScript and no `*.test.ts` in the tree names this script (control: `check-nul-bytes` names three). ## No changeset — `skip-changeset` Measured rather than asserted: **0** published (non-`private`) packages have a `files[]` entry that would ship anything under `scripts/`. Firing control: **70** published packages ship `dist/`. Nothing published moves, so the label is the correct route and an empty-frontmatter changeset would be refused by rule 1 of this very script. ## 验收备注 Two cells are recorded rather than implied, both inherited from the job this step lives in and neither newly opened by it: - A PR carrying `skip-changeset` is exempt from the whole `changeset-check` job, so it is exempt from this rule too. Identical in shape to the cell `check-empty-changeset.mjs` already records for rule 1, and with the same reasoning: a consistent exemption beats a gate that reds one PR and greens an identical one. Motive is thin — a PR with the label adds no changeset of its own, so it has nothing to collide with — but deletion is still reachable under it. - `pr-automation.yml` runs on `pull_request` only, so this gate does not re-run in the merge queue. There is a recorded ⛔ in `lint.yml`'s `on:` block against adding `merge_group` to advisory workflows, so closing that would be a decision, not a wiring fix. What the ruling's "re-evaluated against the current base" needs is satisfied on the PR leg: the base is recomputed on every `synchronize`, and it is a merge base, never a branch tip. One maintainer-only question, deliberately left un-decided here: the ruling names no escape hatch for **deliberately** correcting somebody else's release note on `main`. The report tells such an author to say so on the PR and get it confirmed. If a mechanical route is wanted instead, that is a decision about release integrity and belongs to you, not to this PR. Noted, not filed: `scan()` and `scanForeign()` now hold two near-identical merge-base preambles, including the same throw text. Whoever next touches a third axis in this family will want one helper — no PR or person is queued on this file today, so there is no carrier: 承接者:无. --- _Generated by [Claude Code](https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU)_ Co-authored-by: Claude <noreply@anthropic.com>
This branch was successfully 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.
Tests in
sharing.test.tswere failing due to schema changes that introduced a discriminated union betweenCriteriaSharingRuleSchemaandOwnerSharingRuleSchema.Changes
Updated test data structure:
sharedWith: string →{ type: ShareRecipientType, value: string }criteriafield renamed tocondition(criteria-based rules)ownedByfield (owner-based rules)'manual'and'guest'as valid rule types'full'as valid access levelBefore:
After:
All 24 sharing tests now pass.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.