Skip to content

spec: retire the CEL expression arms of SLI successCriteria and composite trace-sampling condition - #19084

Merged
os-steve merged 3 commits into
mainfrom
claude/issue-18118-retire-cel-expression-arms
Sep 18, 2026
Merged

os-steve merged 3 commits into
mainfrom
claude/issue-18118-retire-cel-expression-arms

Conversation

@os-steve

@os-steve os-steve commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

Fixes #18118

Clause-②: yes — retiring a published authorable surface. Carrier: the changeset
.changeset/18118-retire-observability-cel-arms.md, which declares the same line and the
ADR-0087 disposition. ⛔ The needs:contract-review label is the seat's act; this PR neither
hangs nor clears it.

Ruling: batch #160 item 3, letter A, maintainer 「同意」 2026-09-18T11:59Z — retire the two CEL
expression arms under ADR-0049 enforce-or-remove, by the spec-property-retirement playbook.

What was removed

Two union arms, not two keys:

slot was is
ServiceLevelIndicatorSchema.successCriteria z.union([{ threshold, operator, percentile? }, EvaluatedExpressionInputSchema]) the structured object alone
TraceSamplingConfigSchema.composite[].condition z.union([ StructuredFilterRecord, EvaluatedExpressionInputSchema ]) the structured filter record alone

EvaluatedExpressionInputSchema itself is untouched — packages/spec/src/shared/expression.zod.ts
is not in this diff at all (it is held by PR #18985, which is not addressed here). What left is two
references to it.

The card's zero, re-derived — and the instrument's radius

Re-derived on this branch's base 176b03582e600ee5628d21bff9422073c5a5530c, not inherited.

git grep -n over the whole tracked tree for successCriteria, ServiceLevelIndicator and
TraceSamplingConfig. Every hit outside packages/spec/src is a generated artefact
(api-surface*, authorable-surface*, authorable-defaults, declaration-map,
export-origins, json-schema.manifest, dropped-refinements.baseline.json), a reference page,
a changelog or changeset, or the shipped skill's prose row. Inside packages/spec/src the readers
are: the two schemas' own unit tests, shared/evaluated-slot-population.test.ts (a census), the
migration registry's prose, and one docblock in shared/evaluated-slot-union.ts. Outside the spec
package the only reader is packages/qa/dogfood/test/expression-conformance.ledger.ts — a
classification ledger, not an evaluator. No service, plugin, runtime or CLI path reads either key.

Reachable radius of the instrument: tracked files in THIS checkout at THIS commit. It does not
reach untracked or ignored build output, another repository, or a published npm tarball.

One known target outside it: the sibling repository objectstack-ai/objectui, which is on this
box but is a different git repository, so no git grep here can see it. It is named rather than
waved at, because the template-title-format row of the same ledger records exactly this limit for
a different key: an interpolation site that lives there and cannot be measured from here. ⛔ The argument that used to stand here was REJECTED by at-tier contract review and is withdrawn.
It claimed the radius was closed by a positive fact — that nothing in a sibling could evaluate these
slots without importing the symbols naming them, whose consumers export-origins/ and the
Console Pin Gate enumerate. That is wrong on three counts the review measured: export-origins/
records by its own description which SOURCE DECLARATION each exported name resolves to — origins,
NOT consumers; the Console Pin Gate is path-filtered and was SKIPPED on this very PR; and an
evaluator need not import either symbol, since a REST-served metrics config can be read by key.

What closes the radius instead is direct measurement outside it, with a lit control in each repo.
objectui @ 3e4f6324f7: successCriteria 0 files, ServiceLevelIndicator 0, TraceSamplingConfig 0;
controls in the same run — visibleWhen 340 files, ObjectSchema 652. hotcrm @ 087b7c5dc4
(887 tracked files; public, served by this session's git proxy — an earlier dispatch's 「unreachable」
was the seat's error): the same three at 0 plus slis 0; controls — visibleWhen 15, defineStack 47,
@objectstack/spec 269. sampling shows 4 files there and every one was read (an MCP capability
table row, two prose sentences, a CHANGELOG line — no trace-sampling config), so that zero stands on
inspection and not on the count.

Known targets still OUTSIDE the radius, named rather than waved at: objectstack-ai/cloud
(access denied to this session) and any third-party npm consumer of @objectstack/spec. Neither was
measured, by anyone. What BOUNDS the cost of a wrong zero there is the retirement's audibility (a bound, ⛔ not a closure — at-tier review's wording): a surviving predicate is a
tsc error or a parse refusal carrying the prescription, never a silent change.

Every symbol was located by its declaration site, and a literal inside a // or /** */ comment
was counted as prose, not as a reader — that is why shared/evaluated-slot-union.ts is listed as a
docblock and migrations/registry.ts as prose. Exit codes were captured before any pipe.

The retirement kit

  • The prescription hangs on the surviving schema's own error map, dispatched on issue.input
    (the HookBodyCapability / object.managedBy: 'system' pattern). retiredKey() and an ADR-0087
    D2 strip both retire a KEY; neither retires an ARM, and the keys survive here.
  • Where the prescription reaches, measured on zod 4.4 and pinned both ways. A schema's error
    map is consulted for the top-level invalid_type a NON-OBJECT raises and not for the child issues
    a wrong-shaped OBJECT raises. So on successCriteria the bare-string spelling carries the
    prescription and the { dialect, source } envelope is refused by the structured arm's own
    missing-key issues; on condition both spellings carry it, because the record arm's aborting
    dialect refine sees the object itself. The negative is pinned too: a value refused for a reason
    that is NOT the retirement must not borrow its sentence.
  • ADR-0087 disposition: a D3 SEMANTIC entry, observability-cel-predicates-retired
    (packages/spec/src/migrations/entries/semantic/18.observability-cel-predicates-retired.ts, with
    registry.ts regenerated, never hand-edited between the markers). A predicate is an intent no
    threshold/operator pair or attribute filter records. Both prescriptions therefore carry no
    os migrate meta sentence — owed only where a conversion covers the surface.
  • Changeset with the FROM → TO table and the one-line fix, @objectstack/spec minor with the
    BREAKING banner, per the ruling's Execution section.
  • Baselines and reference pages regenerated, never hand-edited: api-surface-declarations/,
    authorable-defaults/, the two content/docs/references/system/*.mdx pages.

Acceptance notes

  • FOUR published JSON Schemas change projection direction, mechanically (corrected from 「Two」 by at-tier contract review — see below). The retired arm held the
    last .transform() in the system/MetricsConfig and system/TracingConfig subtrees, so both defs
    now project in output mode instead of falling back to the input shape. Consequences, all declared
    in-diff: system/MetricsConfig:slis and system/TracingConfig:sampling publish the default the
    parser has always applied (declared in DEFAULT_CHANGES_BY_MAJOR with the ai/KnowledgeSource:refresh
    row as the precedent, that mechanism run backwards), and the nested type cells of both reference
    pages lose the ? from their default-bearing keys — the output-mode signature, and the same
    convention every transform-free def in the repo already publishes under. No runtime default
    moves
    : measured by byte-identity of the untouched .default(…) and by parsing a minimal config
    on the built package.
  • Consumers swept beyond the named file surface, because they break otherwise. The
    evaluated-slot-population.test.ts census drops 36 positions over 34 declaring lines to 34 over 32,
    naming both departures rather than subtracting them; the evaluated-slot-union.ts docblock drops
    five of 36 to three of 34; the ADR-0058 D7 ledger row cel-declared-unwired-observability closes
    with the removal, and its companion test's inline scan floor drops 3 to 1 with both positions
    named, exactly as that floor's own instruction requires.
  • The ruling's Execution section says Clause-②: no; the dispatch claim comment says
    Clause-②: yes.
    This PR carries the claim's line, because the claim comment is the carrier the
    clause-② check reads and the two must agree. Flagged rather than silently chosen. The yes reading
    also has independent support in this diff: two published JSON Schemas change projection direction.
  • Noted, not filed: the reference pages' inline nested type cells render post-parse optionality
    (a defaulted key reads as required) while the expanded Nested Shape: sections below them read the
    zod node and say optional (default: …). The two disagree for every output-mode def in the repo,
    not only these; it predates this card and this diff does not widen it. Carrier for anyone who picks
    it up: packages/spec/scripts/build-docs.ts.
  • Not in this diff, by the ruling: the structured arms (their own card);
    skills/objectstack-formula/SKILL.md, whose structured | cel row for metrics / tracing is the
    skills lane's at tier; and packages/spec/src/shared/expression.zod.ts.

Verification

Run under the shared verify lock; the judged line of each is quoted in the report on the card.


Generated by Claude Code

  • Same-major absorption (added after at-tier contract review found it missing — the round's one BLOCKING finding).
    The same unpublished step 18 carried entries/semantic/18.evaluated-expression-slots-source-required.ts,
    which still enumerated these two slots among 「the 36 declaring positions」 and still told an upgrader to
    give a sampling condition a dialect and a non-blank sourcethe exact envelope this head refuses.
    The playbook's same-major rule applies to the published D3 record exactly as it applied to the census test
    and the helper docblock. That entry now reads 34 positions, drops the two slots and the condition-specific
    sweep clause, and routes a hit at either slot to observability-cel-predicates-retired. Verified in the
    GENERATED output, not only the input: registry.ts carries 34 declaring positions once and
    36 declaring positions zero times.

  • Why D3 is right rather than merely available. Both error-map precedents this retirement copies its
    MECHANISM from also registered a D2 conversion, because for them a mechanical rewrite existed. Here none
    does: a strip leaves a REQUIRED successCriteria missing (the SLI stops parsing) and a composite branch
    with no condition at all.

  • The four defs, named (the two nested ones were disclosed nowhere before): system/MetricsConfig
    (default on slis, plus 8 required members) · system/TracingConfig (default on sampling, plus 4)
    · system/ServiceLevelIndicator (one required member, enabled) · system/TraceSamplingConfig
    (one required member, rules). ⭐ The last two are invisible to the default-changes.ts table by the ratchet's construction, not for lack of a
    default: that table records default VALUES per key, and enabled / rules already carried theirs (true, [])
    published at the base and unmoved here, so no row of it can express a required growth. ⛔ Corrected from an
    earlier wording of mine that said they had 「no default to declare」 — at-tier contract review measured that as
    loose; the exact form is the changeset's own: only the first two carry a default MOVE.


Body edits above made by the domain:spec#4 seat after at-tier contract review; the dev writes the body once, at creation.


Generated by Claude Code

…sampling condition

Both slots were z.union([<a structured arm>, EvaluatedExpressionInputSchema]).
The expression arm parsed, normalized a bare string to { dialect: 'cel', source },
registered and was served back, and nothing anywhere evaluated it — ADR-0049
enforce-or-remove. The arms are removed; the structured arms are untouched and
measured on their own card.

The prescription hangs on the surviving schema's own error map (dispatched on
issue.input), because the KEY survives and only one of its two arms went away:
retiredKey() and an ADR-0087 D2 strip both retire a key, neither retires an arm.
The disposition is a D3 semantic entry, observability-cel-predicates-retired, so
neither prescription carries an `os migrate meta` sentence.

Mechanical consequences, all declared: the retired arm held the last transform in
the system/MetricsConfig and system/TracingConfig subtrees, so both defs project
in output mode and publish the defaults the parser always applied
(DEFAULT_CHANGES_BY_MAJOR); dropped-refinements sites move off the union option
path; the ADR-0058 D7 ledger row cel-declared-unwired-observability closes with
the removal and the inline scan floor drops 3 to 1, naming both positions.

Claude-Session: https://claude.ai/code/session_01AmH9bKvGoLjiY86Q4Z3og2
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation protocol:system tests tooling labels Sep 18, 2026
@github-actions

github-actions Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

8 anchor(s) derived from 1 changed package(s); no hand-written page names any of them. ⚠️ 4 changed file(s) yielded no anchor (packages/spec/api-surface-declarations/system.txt, packages/spec/authorable-defaults/system.json, packages/spec/dropped-refinements.baseline.json, …), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

What this run could not see
  • 4 changed file(s) yielded no anchor (packages/spec/api-surface-declarations/system.txt, packages/spec/authorable-defaults/system.json, packages/spec/dropped-refinements.baseline.json, …) — pages documenting those are invisible to this run
  • 4 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 136 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json b7eaf6a617b7353825935631f73b5bdcf7b78f90packageMentionDocs.

Which tree this was computed on

This run read content/docs from 0326b87160643cfca37924365c815403bc48b09a — the merge of head 425912a3bb33e1ba5de502e4c0dc7b5ffcdf09b3 into base b7eaf6a617b7353825935631f73b5bdcf7b78f90, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 0326b87160643cfca37924365c815403bc48b09a && git checkout 0326b87160643cfca37924365c815403bc48b09a
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin b7eaf6a617b7353825935631f73b5bdcf7b78f90 425912a3bb33e1ba5de502e4c0dc7b5ffcdf09b3 && git checkout -B drift-repro b7eaf6a617b7353825935631f73b5bdcf7b78f90 && git merge --no-ff 425912a3bb33e1ba5de502e4c0dc7b5ffcdf09b3

node scripts/docs-audit/affected-docs.mjs --json b7eaf6a617b7353825935631f73b5bdcf7b78f90

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Copy link
Copy Markdown
Collaborator Author

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: 8968c29355f2551e0750608201fb2a87152aa697

Two worktrees: base 176b03582e (the PR's merge base) and head 8968c2935, both fresh detached checkouts with pnpm install --offline --frozen-lockfile (exit 0 each). Instruments: node v22.22.2, pnpm 10.31.0, tsc 6.0.3, zod 4.4.3 (the spec package's own), vitest 4.1.11. ⭐ Every dist read here was produced by the direct package script pnpm --filter @objectstack/spec build in the head worktree — not turbo, no cache. Exit codes captured before every pipe. CI at head: 34 success / 5 skipped / 0 non-green. 17 files; neither skills/objectstack-formula/SKILL.md nor packages/spec/src/shared/expression.zod.ts is among them — both fences held.

(1) Derived judgments

Accept set, measured on the BUILT head package. successCriteria: structured accepted (control); bare string refused with the retirement prescription; the { dialect, source } envelope refused by the structured arm's own child issues, prescription absent; a number refused with zod's own message, prescription absent. condition: { service } and { source } accepted (controls); bare string refused with the prescription; the envelope refused with the prescription (the aborting dialect refine); a number refused with zod's own message. ⇒ the asymmetry the dev pinned is real, and the negative holds. Runtime defaults unchanged. Focused suites: 276 passed.

Reverse verification, three legs against the REBUILT dist/system/index.d.ts. Leg A (retired string spellings): exit 2, two TS2322. Leg B (CONTROL, structured only): exit 0. ⭐ Leg C (envelope spellings, the reviewer's own addition): exit 2 with ONE error — TS2353 on successCriteria only; the condition envelope type-checks against Record and is refused only at parse. Without leg B the two errors would not be a reading.

The card's zero — re-derived with the reviewer's own control, and it HOLDS. Identity scan plus a second pass by property access; same-instrument controls lit (RecordAlertProps, api.transaction). Radius: tracked files of this checkout at this commit. Measured directly outside it: objectui @ 3e4f6324f7 zero hits, control lit; hotcrm @ 087b7c5dc4 (887 tracked files) zero hits, control lit. Known target still outside: objectstack-ai/cloud (access-denied) and any third-party npm consumer.

But the dev's "separate positive fact" does NOT close the radius and must not be re-used. export-origins/ records, by its own description, which SOURCE DECLARATION each exported name resolves to — origins, not consumers; the Console Pin Gate is path-filtered and was SKIPPED on this very PR; and an evaluator need not import either symbol — a REST-served metrics config can be read by key. The argument is wrong; the conclusion survives only because the sibling repos were actually measured.

The published JSON Schema — the "second axis", measured. Base→head: exactly four defs differ and all four lose x-io: inputsystem/MetricsConfig (required gains 8 keys, default appears on slis), system/TracingConfig (gains 4, default on sampling), system/ServiceLevelIndicator (gains enabled), system/TraceSamplingConfig (gains rules). Same-category control system/CacheConfig.json: byte-identical. Convention control at head: of 262 output-mode system defs, defaulted keys are listed required 241 times vs 1 not — the four merely joined the repo's existing convention. Mechanism confirmed: the retired arm held the last .transform() of each subtree.

The four widened consumers — each genuinely FORCED, verified one by one: default-changes.ts (the #4666 block exits 1 on an unauthorised default change; the row IS the gate's prescribed remedy, with a real precedent); dropped-refinements.baseline.json (the emitted site moved, so the #18670 ratchet fails on a site the build no longer observes); the census test + union helper (the two slots are no longer expression positions; the helper keeps two live consumers, not orphaned); the dogfood ledger + floor. ⭐ The floor's own failure message requires naming the departed positions and they are named; ablation (floor 1→3, then restored) fails with exactly that message.

The tool choice — RIGHT, by declaration site. A surviving KEY losing one ARM is the class the playbook says none of the three routes fits; the prescription hangs on the schema's own error map dispatched on issue.input, the HookBodyCapability / object.managedBy pattern, both verified at their declaration sites. ⚠️ Nuance the dev did not state: both cited precedents ALSO registered D2 conversions because a mechanical rewrite existed — here none does (a stripped predicate leaves an invalid SLI or a silently unconditional sampling branch), so the D3 semantic disposition is right. No os migrate meta sentence: right, per retired-key.ts:42-49.

F1 — BLOCKING. The same unreleased step carries a contradicting D3 entry the playbook requires absorbing. migrations/entries/semantic/18.evaluated-expression-slots-source-required.ts (step 18, landed by PR #18638 at 16:01Z today; protocol is still 17.0.0, so both entries first ship together in step 18) still names "the 36 declaring positions" including these two slots, and still instructs the upgrader that on condition an expression "needs a dialect this platform evaluates AND a non-blank source" — the exact envelope this head now refuses. Playbook §0 「同 major 记账」 requires absorbing an earlier change to the same key in the same unpublished major. The dev applied that principle to the census test (36→34) and the helper docblock (5→3) but not to the published D3 record, so the registry's own text disagrees with the census it cites. Fix: in the earlier entry drop the two positions and the condition-specific sweep instruction, then gen:migration-registry. PR #18985 does not touch that file.

F2 — non-blocking. Changeset, PR body and both default-changes.ts reasons say "Two published JSON Schemas change projection direction". Measured: four; the required growth on the two nested defs is disclosed nowhere.

F3 — non-blocking. The new entry's acceptance proof says authoring a retired spelling "is a tsc error … no longer admits a string or an envelope". Leg C: the condition envelope IS admitted by Record and is caught only at parse. Qualify it.

(2) Semver level

@objectstack/spec minor with the BREAKING banner, FROM→TO table, Clause-②: yes (narrowing), adr-0087 marker. Consistent with the ruling's Execution line and with the launch-window guard (check-changeset-no-major exit 0). The cited 17.5.0 is right at this head.

(3) Boundary flags

  • Clause-② no (ruling) vs yes (claim/PR/changeset) — SETTLED BY MEASUREMENT. The "second axis" claim holds and is undercounted: four shipped JSON Schema files, in the npm files, gain required members and two gain default keywords; authorable-defaults/system.json gains two rows. Under contract-review.md's floor rule and lanes/spec.md:20, yes is substantively supported, not merely a safe over-declaration — the seat need NOT correct the carriers to no. For completeness: check-widening-tells --declaration no exits 0 with no tell, so a no would not have been refused mechanically — the measurement, not the tell, is what settles it.
  • Out-of-scope (a), the structured arms equally unread: agree, file as the ruling anticipated.
  • Out-of-scope (b), reference-page nested cells: confirmed pre-existing and repo-wide (241:1), not widened here.
  • The dev's radius argument: answered above — ⛔ do not cite it again as a closure.

Implemented-by: claude/issue-18118-retire-cel-expression-arms
Reviewed-by: session_01AmH9bKvGoLjiY86Q4Z3og2

VERDICT: FAIL — one BLOCKING (F1: absorb the same-step D3 entry, one file + registry regen), two non-blocking wording findings to fold into the same patch round. Everything else measured here holds at this head. After the patch the head moves, so the record is re-issued for the new head.


Generated by Claude Code


Generated by Claude Code

… defs that change projection direction

Contract review F1 (blocking): the evaluated-expression-slots-source-required
semantic entry sits in the same unpublished step 18 as this retirement, and still
enumerated the two retired slots among "the 36 declaring positions" while telling
the upgrader to give a sampling condition a dialect and a non-blank source — the
exact envelope this head now refuses. The playbook's same-major absorption rule
applies to the published D3 record exactly as it applied to the census test and the
helper docblock: that entry now reads 34 positions, names the two absentees and the
retirement that took them, and routes a hit at either slot to
observability-cel-predicates-retired instead of to its own repair.

F2: four published JSON Schemas change projection direction, not two. system/MetricsConfig
and system/TracingConfig lose x-io input and gain a default; the nested
system/ServiceLevelIndicator and system/TraceSamplingConfig lose x-io input and gain a
required member (enabled, rules) with no default to declare, so no ratchet row can hold
them — stated in the changeset and in both default-change reasons instead.

F3: the new entry's acceptance proof claimed tsc refuses a string or an envelope at both
slots. Measured: tsc catches the string at both, and the envelope only at successCriteria;
the condition envelope is structurally admitted by the record arm and is refused at parse.
The proof now separates the two channels and says which spelling each one catches.

Also states the nuance the review asked for: both error-map precedents this retirement
copies its mechanism from also registered a D2 conversion because a mechanical rewrite
existed, and here none does.

Claude-Session: https://claude.ai/code/session_01AmH9bKvGoLjiY86Q4Z3og2
Co-authored-by: Claude <noreply@anthropic.com>

Copy link
Copy Markdown
Collaborator Author

Contract review — re-review round

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: dc9dacab6f8eebc033d2540a75070130dca79044

Re-review after the patch to the FAIL record on head 8968c2935 (comment 5735594818). Base unchanged 176b03582e. Worktrees: base-176b0358, head-8968c293 (retained) and a fresh head-dc9dacab, pnpm install --offline --frozen-lockfile exit 0. Scope: what moved plus what the patch could disturb.

What moved (git diff --stat 8968c2935..dc9dacab6): 5 files, +141 −81 — the changeset, default-changes.ts, the two step-18 semantic entries, the regenerated registry.ts. Base..head is now 18 files; neither fenced path among them.

Carry-forward basis, verified by BLOB SHA — not by diff silence. metrics.zod.ts 5d24d484…, tracing.zod.ts 43c78d08…, metrics.test.ts 3890d725…, tracing.test.ts 0308ab21…, expression-conformance.ledger.ts c7a4e634…, expression-conformance.test.ts 3d089bb2… — identical blobs at both heads; and the diff over the whole carry-forward artefact set is empty. So the accept-set readings, the three tsc legs (A exit 2 / B control exit 0 / C exit 2), the runtime defaults and the dogfood ablation stand for this head without re-measurement.

CI at this head: 46 runs = 39 success, 7 skipped, 0 non-green, all completed at read time.

(1) Derived judgments

F1 (was BLOCKING) — RESOLVED, and the absorption is COMPLETE. In the generated output: registry.ts carries 34 declaring positions ×1, 36 declaring positions ×0 (also the 36 , 36 evaluated, 36 positions: zero), lit control observability-cel-predicates-retired ×3. check:migration-registry exit 0. ⭐ Sweep of every step-18 entry file AND the whole step-18 region of registry.ts (lines 5102–12553) for successCriteria, TraceSamplingConfig, composite … condition, needs a dialect, dialect this platform evaluates: the only remaining mentions are the earlier entry's absorption note and its re-routing sentence («deliberately NOT on this sweep … Sweep those two under observability-cel-predicates-retired instead»). No text in step 18 steers an upgrader toward a spelling this head refuses.

F2 — RESOLVED; accounting right, one phrase loose. The FOUR table matches the measured projection exactly (+8 / +4 / +1 / +1 required; default on slis / sampling). ⚠️ The dev's phrase «gain a required member with no default to declare» is TRUE in substance — the #4666 ratchet records default VALUES per key and neither value moved, so it is blind to required growth by construction — but loose in wording: enabled and rules DO carry defaults (true, []), already published at the base and unchanged. The exact form is the changeset's own: «Only the first two carry a default MOVE, so only those two are declarable». The default-changes.ts reasons and the seat's PR-body edit carry the loose form.

F3 — RESOLVED, wording verified verbatim including «⛔ Do not read a clean tsc as a clean sweep of condition», consistent with legs A/B/C.

The D2-precedent nuance is present in the entry's reason and in the changeset, and matches conversions/registry.ts:2268,3686 by declaration site.

⚠️ F4 — NEW, non-blocking: the FOUR paragraph was spliced INTO THE MIDDLE OF A SENTENCE in BOTH default-changes.ts reasons. Rendered from the module at this head: the slis reason reads «…and what he now reads is what the ⚠️ FOUR published JSON Schemas change projection direction in this diff…» and «…not a new one. parser has always applied. To keep the old value…»; the sampling reason similarly. ⭐ The gate prints these reasons on every run that accepts the change — its own text says «reason is printed by every build that accepts the change, so write it for the consumer who is about to be surprised» — so that consumer meets two broken sentences. Not a contract defect, not in the npm files. Fix: move the paragraph to the END of each string and reword «no default to declare» per F2.

Gates re-run at this head: check:migration-registry, check:spec-changes, check:upgrade-guide, check-adr-0087-registration, check-changeset-no-major — all exit 0; check-widening-tells --declaration no: no tell. Tests: --project local over src/migrations + two shared pins → 300 passed; --project repo src/shared/retired-key-migrate-sentence.test.ts → 14 passed.

Correction to my own 8968c2935 record: that round's «276 passed» was a --project local run, and vitest excluded retired-key-migrate-sentence.test.ts because that pin lives in the repo project — the run said so and I quoted the count without reading the notice. It is now measured, on byte-identical inputs, so it holds for both heads.

The card's zero — the dev's replacement readings, reproduced. objectui zeros identical; its control counts 340/652 are the with-CHANGELOG figures (318/633 without) — the zero is the same either way. hotcrm controls match exactly; all four sampling hits read here too (CHANGELOG:4417 prose, a docs page, an MCP capability-table row, one code comment) — no trace-sampling config. The zero holds on inspection.

⭐ The audibility argument, judged: it is a BOUND on the cost of a wrong zero, not a closure of it — the PR body's «what covers them» overstates by one word. What it guarantees for any consumer reaching these slots through the spec's parse: an author of either retired spelling is refused with the prescription, and a stored row carrying one fails at the load seam naming the slot. So a wrong zero for cloud or a third-party consumer cannot be SILENT — the evaluator starves behind an audible refusal and is discovered. What it does NOT cover: a consumer reading stored JSON without the spec parse. With the ruling resting on the domain criterion as well as the zero, that bound is acceptable — but it is a bound.

(2) Semver level

Unchanged and re-verified: @objectstack/spec minor, BREAKING banner, FROM→TO table, Clause-②: yes (narrowing), adr-0087 marker. check-changeset-no-major exit 0. 17.5.0 still right.

(3) Boundary flags

  • Clause-② yes vs the ruling's no: unchanged — the second axis is measured, so yes is substantively supported; the seat need not correct the carriers.
  • Process reading (1) — VERIFIED: spec-changes.json and docs/protocol-upgrade-guide.md mention neither entry id (0 and 0); the builders project up to PROTOCOL_MAJOR 17; both checks exit 0 without regeneration. ⇒ registry.ts is the only published step-18 carrier today — the file F1 was about.
  • Note for the release editor, ⛔ not a finding on this PR: the pending .changeset/15811-evaluated-expression-slots-source-required.md (feat(spec)!: every engine-evaluated expression slot requires a non-blank source #18638's own record, untouched here) still says «36 declaring positions» including both slots and still teaches the condition envelope. It composes into the same release's CHANGELOG beside this retirement's FROM→TO table, which supersedes it. Whoever composes the release notes should read the two together.
  • F4 is the only open item and is non-blocking. If the seat has it fixed before enqueue, the head moves and this record is re-issued — cheap, since only that file would move.

Implemented-by: claude/issue-18118-retire-cel-expression-arms
Reviewed-by: session_01AmH9bKvGoLjiY86Q4Z3og2

VERDICT: PASS — F1 resolved and verified in the generated output and across all of step 18; F2 and F3 resolved; the carry-forward accepted on blob-sha byte-identity; every gate at this head exits 0; CI 39/7/0. One new non-blocking finding (F4) recorded for the seat's disposition.


Generated by Claude Code


Generated by Claude Code

…d tighten one phrase

Contract review F4: the FOUR paragraph was inserted BEFORE the last clause of each
reason string rather than after it, so both reasons rendered with two sentences cut
in half — "…what he now reads is what the ⚠️ FOUR published JSON Schemas change
projection direction…" and "…not a new one. parser has always applied." The gate's
own text says the reason is printed by every build that accepts the change and must
be written for the consumer who is about to be surprised; that consumer was being
handed broken sentences. The paragraph now sits at the end of each string, and both
reasons were read back as rendered from the module and from the accepting build's
own output.

Also tightens the phrase the review found loose. It said the two nested defs gain a
required member "with no default to declare". They do carry defaults — enabled is
true, rules is [] — and both were already published at the base: measured, not
inherited, at authorable-defaults/system.json lines 206 and 248 of the base blob,
whose base..head diff is exactly +slis and +sampling, two insertions and no
deletions. The wording is now the changeset's own: only the first two carry a
default MOVE, so only those two are declarable here, because this ratchet records
default VALUES per key and is blind to required growth by construction.

Claude-Session: https://claude.ai/code/session_01AmH9bKvGoLjiY86Q4Z3og2
Co-authored-by: Claude <noreply@anthropic.com>

Copy link
Copy Markdown
Collaborator Author

Contract review — re-issue round

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: 425912a3bb33e1ba5de502e4c0dc7b5ffcdf09b3

Re-issue after the F4 fix to the PASS record on head dc9dacab6. Base unchanged 176b03582e; 425912a3b^ = dc9dacab6. A fresh worktree head-425912a3 beside the three retained ones, pnpm install --offline --frozen-lockfile exit 0.

What moved (git diff --stat dc9dacab6..425912a3b): exactly one file, packages/spec/scripts/lib/default-changes.ts (+28 −20), and only the two reason strings inside itkey, from, to of both rows unchanged. ⭐ Every other file of the 18 in base..head has an identical blob at both heads (17 of 17 checked by git rev-parse rev:path). Fences: neither fenced path among the 18. ⇒ every reading of the dc9dacab6 record carries forward except F4, and the 8968c2935 readings beneath it carry with it — the inputs of all of them are byte-identical across the three heads.

CI at this head: 35 runs = 33 success, 2 skipped, 0 non-green, all completed.

(1) Derived judgments

F4 — RESOLVED, verified in THREE channels.

(a) Source: the ⚠️ FOUR published mid-sentence 0 · not a new one. parser has always 0 · not a new one. repo who 0 · no default to declare 0 · lit control FOUR published 2.

(b) The module, rendered with tsx and split into sentences: system/MetricsConfig:slis is 11 sentences — sentence 7 ends «…the key, its type and its default are unchanged.» and the FOUR paragraph is sentences 8–11; system/TracingConfig:sampling is 9 sentences, sentence 5 ends the same way, paragraph is 6–9; both strings end «…existing output-mode convention, not a new one.»

(c) ⭐ The channel the finding was actually about — the accepting gate's own printed output. check:authorable-surface at this head (direct package script; anchor = upstream baseline at the merge base; 「📌 2 declared default change(s) accepted (#4666)」; 1541 schemas generated; VERDICT command-exit 0) prints both reasons in full at log lines 1636 (2872 chars) and 1638 (2243 chars): zero splice signatures, zero loose phrase, FOUR published once each, both ending «not a new one.»

F2 precision — now a READING, confirmed off the base blob. authorable-defaults/system.json at 176b03582e: line 206 ServiceLevelIndicator:enabled = true, line 248 TraceSamplingConfig:rules = []; git diff --numstat base..head on that file = 2 0 (two insertions, zero deletions). The new sentence is the exact form.

The other consumers that one file could disturb, re-run at this head under the lock: check:scripts-typecheck (the project that compiles scripts/lib/) && vitest run --project local scripts/authorable-defaults.test.ts27 passed, VERDICT command-exit 0 (the && join makes the wrapper certify both). src/ never imports DEFAULT_CHANGES_BY_MAJOR — its six src mentions are comments and registry prose — so no runtime path is reachable from the change.

⭐ The dev's gate reconciliation (108 run / 1 NOT MEASURED, up from 107/2) — judged: it changes nothing I concluded. The two newly measured gates and the one still unmeasured all live in CI jobs that were green at every head of this card: check:lean-entry-closure and check:dual-build-cjs-loads are steps of Build Core; check:type-check-debt is the Type Check · debt ledger job. Both records already rested on those jobs' green readings; the local NOT MEASURED lines were an instrument condition of an unbuilt worktree, now partly converted — a second reading agreeing with the first, not a new fact. The remaining NOT MEASURED is measured by Build Core, success at this head; leaving it unconverted rather than holding the shared lock 15+ minutes, and never reaching for OS_SKIP_DTS, is the right call.

(2) Semver level

Unchanged and carried: @objectstack/spec minor, BREAKING banner, FROM→TO table, Clause-②: yes (narrowing), adr-0087 marker — the changeset's blob is identical to dc9dacab6, where check-changeset-no-major and check-adr-0087-registration both exited 0 base..head; Check Changeset success at this head.

(3) Boundary flags

  • Clause-② yes: stands; no carrier line moved; the second axis is measured; no carrier edit owed.
  • Audibility: carried as a BOUND, not a closure. All five owed PR-body edits read back present.
  • Note for the release editor (unchanged, ⛔ not a finding on this PR): the pending .changeset/15811-evaluated-expression-slots-source-required.md still teaches the condition envelope; it is superseded in the same CHANGELOG by this retirement's FROM→TO table.
  • Out-of-scope items (a) and (b) stand unchanged. Nothing else changed at this head; no open findings remain.

Implemented-by: claude/issue-18118-retire-cel-expression-arms
Reviewed-by: session_01AmH9bKvGoLjiY86Q4Z3og2

VERDICT: PASS — F4 resolved and verified in source, in the rendered module and in the accepting gate's own printed output; the one moved file's every consumer re-run at this head exits 0; all 17 other files carry identical blobs to the dc9dacab6 PASS head, so that record's readings and the 8968c2935 readings beneath them carry forward; CI 33/2/0. No open findings.


Generated by Claude Code


Generated by Claude Code

@os-steve
os-steve marked this pull request as ready for review September 18, 2026 22:00
@os-steve
os-steve added this pull request to the merge queue Sep 18, 2026
Merged via the queue into main with commit ee5812a Sep 18, 2026
39 of 40 checks passed
@os-steve
os-steve deleted the claude/issue-18118-retire-cel-expression-arms branch September 18, 2026 22:29
os-steve pushed a commit that referenced this pull request Sep 18, 2026
`#19084` (`ee5812a5e3`) retired the CEL expression arm of
`TraceSamplingConfigSchema.composite[].condition` at the very slot this
branch projects. Both intents stack: main's side of the slot is taken
whole — the record-only `condition` and its retirement prescription —
and its `!('dialect' in value)` predicate is declared through this
branch's `bannedKeys(['dialect'])` arm. The two renamed ledger rows go,
because the arm projects the site they name.

Claude-Session: https://claude.ai/code/session_01AmH9bKvGoLjiY86Q4Z3og2
Co-authored-by: Claude <noreply@anthropic.com>
os-steve pushed a commit that referenced this pull request Sep 18, 2026
`#19084` collapsed `TraceSamplingConfig.composite[].condition` to a
record, so the ban lands on `condition` itself rather than on a union
arm, the ledger rows are spelled `composite.element.condition`, and a
CEL envelope is now refused by the runtime too. The live-seam pins and
the changeset's accept-set sentence are re-derived on that tree.

Also: the changeset declares `Clause-②: yes`, matching the corrected
claim and the ruling; and the empty-key-list branch records the real
reason it drops — `enum: []` is an invalid schema, not a vacuous rule.

Claude-Session: https://claude.ai/code/session_01AmH9bKvGoLjiY86Q4Z3og2
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation protocol:system size/xl tests tooling

Projects

None yet

2 participants