fix(spec): refuse a declared def rename that would collapse two baseline keys onto one - #17920
Merged
os-bill merged 2 commits intoSep 13, 2026
Merged
Conversation
…ine keys onto one `checkRenameTable` validates `RENAMED_DEFS` against the defs a build EMITS, which is all its call site can see. A single rename `A -> B` where the recorded BASELINE already holds a property name under both `A` and `B` is invisible there: it is not two sources onto one target, the source is unemitted and the target is emitted, so all four existing rules pass. Every carry in `build-schemas.ts` is a plain `Map.set` keyed by the carried key, so the two entries collapse and the later write wins. The loss is the recorded RETIRED state and the recorded DEFAULT of the losing side, and it happens inside the carry, before any comparison runs -- so each ratchet downstream adjudicates against input this script already corrupted, in both directions: a genuine default change on the merged key can read as no change at all, and a key whose default never moved can read as `changed`. `checkRenameBaselineCollisions` refuses exactly the intersection: the property names the baseline records under BOTH defs. A merge whose two defs share no property name writes every key exactly once and is deliberately left green -- refusing a populated target as such would redden 24 of the 39 committed entries and forbid the in-tree `cloud/Sha256Digest -> system/Sha256Digest` shape. It runs once per baseline this script carries, because the in-tree snapshot and the upstream anchor are different documents and a collision can exist in either. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MkQhmuuJAVDjmeWNixwDDH
… that modelled an impossible baseline Three pins, two of them about the cost this guard can impose. The existing carry fixture INJECTED the old key into the base while leaving the carried one in place, so the anchor recorded `source` under both defs at once — a shape no real landing produces (an anchor from before the rename holds the property under the OLD def only) and the one the new guard refuses outright. Measured, it now exits 1 before the check it was written for is reached, so the fixture removes the carried key as well and models the real pre-rename anchor. The new pins: the colliding baseline is refused, names both defs and the property, and writes nothing; and a rename into a target that is still POPULATED in the base stays green when no property name is shared, because `Map.set` can only collapse entries that are the same key. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MkQhmuuJAVDjmeWNixwDDH
Contributor
📓 Docs Drift CheckNothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs. What this run could not see
Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
This was referenced Sep 13, 2026
os-bill
marked this pull request as ready for review
September 13, 2026 04:33
os-bill
deleted the
claude/issue-17383-rename-merge-into-populated-target
branch
September 13, 2026 05:02
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.
Fixes #17383
Clause-②: no
checkRenameTablevalidatesRENAMED_DEFSagainst the defs a build emits — thatis all its call site can see. A single rename
A -> Bwhere the recorded baselinealready holds a property name under both
AandBis invisible to it: it is not twosources onto one target, the source is unemitted and the target is emitted, so all of
the existing rules pass. Every carry in
build-schemas.tsis a plainMap.setkeyedby the carried key, so the two entries collapse and the later write wins.
The premise this PR did not inherit
The card places
checkRenameTableinpackages/spec/scripts/lib/authorable-defaults.ts.It is not there. Re-derived by symbol on
origin/main:packages/spec/scripts/lib/renamed-defs.ts, consumed once inbuild-schemas.ts;build-schemas.ts(the surface gate's
prev, the deletion gate'sbaseSnapshot, and bothbaselineKeysbuilders of the defaults gate) pluscarryDefaultsThroughRenames;registeredRetiredKeys(), is not affected: it mergesdeliberately (earliest major wins) instead of clobbering.
A second correction: the card and the dispatch both say
checkRenameTablerefuses"exactly four shapes". It refuses five — the chained-rename rule (
A -> B -> C)is a separate pass at the end of the function.
The gap, reproduced with a lit control
Against the real carry code, one rename
integration/Old -> integration/New, with thebaseline holding
modeunder both defs:checkRenameTablereturns[]— no diagnosticthe TARGET def is already claimed by …prevcollapses 2 baseline entries to 1; the surviving retired flag is whichever sorted lastcarryDefaultsThroughRenamescollapses 2 fingerprints to 1"b", this build emits"a"— a genuine change — anddiffAuthorableDefaultsreports[]. Swallowed."b", this build emits"b"— no change at all — and the differ reportschanged: "a" -> "b". Manufactured.The corruption is bidirectional, which is what decides the disposition below.
The choice: REFUSE — and refuse the intersection, not the populated target
The card offers two shapes and specifies neither. This PR refuses, for three reasons,
and narrows what is refused for a fourth.
The damage is identical to the two-sources rule's, not merely similar: the same
maps, the same
Map.set, the same lost retired state and lost default. Where thedamage is identical, the disposition should be too.
Report-and-allow cannot work here, because the clobber happens inside the
carry, before any comparison runs. The rows a reviewer would read are themselves
computed against corrupted input — and per the table above the gate both swallows
real changes and manufactures false ones, so a reviewer holding an advisory
"collision" line beside a
changedrow cannot tell which of the two is real.Refusal is the only disposition that keeps the reported diff honest.
The remedy is available and is the one the two-sources rule already prescribes:
keep the rename, and retire the losing side explicitly with
retiredKey()plus itsregistered ADR-0087 conversion.
whose target already carries keys in the baseline" would redden
mainimmediately:in the committed
authorable-surface/(that snapshot is the post-rename one, sothe keys sit under the new name and the source has none);
cloud/Sha256Digest -> system/Sha256Digestentry is documented as"a rename onto a def that already existed", so rename-into-an-existing-target is
an already-sanctioned shape.
Map.setcan only collapse entries that are the same key, so a merge whose twodefs share no property name writes every key exactly once and loses nothing. The
refusal is therefore the intersection — the property names the baseline records
under both defs — which is exactly the damage and nothing more. Measured on
origin/main: 0 collisions in the committedauthorable-surface/and 0 inauthorable-surface.base.json.Both baselines are guarded, because they are different documents: the upstream anchor
is the pre-rename snapshot (source populated, target empty) and the in-tree snapshot
is the post-rename one. A real merge-into-populated-target shows up in the anchor
first, so guarding only the in-tree snapshot would have missed the card's own case.
Ablation — both directions, on-disk proof, hash restore
Each leg: unique-anchor check, occurrence counts before and after,
git hash-objectagainst the
HEADblob,trap … EXIT INT TERM, and restore verified byte-identicalplus an empty
git diff HEAD.Leg A — disable the guard. Blob
f32305e7->addc4481.name; is silent where checkRenameTable is loud; reports EVERY colliding
property; splits on the FIRST separator
BOTH defs, and writes nothing
f32305e7, byte-identical,git diff HEADemptyLeg B — make the guard over-fire (refuse when BOTH sides are populated, ignoring the
intersection). Blob
f32305e7->4f87b2b5. This is the cost this change can impose.target whose property names are disjoint, and the wiring pin does NOT refuse a
rename into a populated target when no property name is shared
RED, because the over-firing rule reports 3 properties where the real rule reports 2
f32305e7, byte-identical,git diff HEADemptyevery committed entry has an empty source side in the in-tree snapshot. The over-refusal
is caught only by these pins. That is why they exist.
⭐ Correction, from the at-tier contract review of this PR. An earlier revision of this
section said Leg B "is the leg the card's literal predicate would have shipped" while also
saying the real build stays green under it. Those cannot both be true, and the review
separated them by measurement:
4f87b2b5) — refuse when both sides are populated. Realcheck:authorable-surfaceexit 0; caught by the pins alone.mutation (the review's B2, blob
4a3848b9). Wired into the real build it exits 1with 24 problem lines:
❌ 24 declared def rename(s) would COLLAPSE keys of the committed authorable-surface/.⇒ The card's predicate is caught by the real build, loudly. The narrower over-fire is the
one only the pins catch. Both readings stand; only the sentence conflating them was wrong.
One fixture corrected, and why it is in scope
build-schemas-check-mode.test.tsseeded its carry fixture by injecting the old keyinto the base while leaving the carried key in place — so the anchor recorded
sourceunder both defs at once. That is the collision, and the new guard refuses it: measured,
the run now exits 1 before the check the fixture was written for is reached, so it
would have been asserting about a build that never got there. No real landing produces
that anchor (a pre-rename anchor holds the property under the old def only), so the
fixture now removes the carried key as well. It is a faithfulness fix, not an
accommodation — and it doubles as the over-refusal pin, since the target def still holds
six other keys in that base.
Verification
vitest run --project local scripts/renamed-defs.test.tsvitest run --project repo build-schemas-check-mode.test.ts -t 'deleted baseline lines must prove themselves'pnpm --filter @objectstack/spec typechecktsc+check:scripts-typecheck+check:test-typecheckall greenpnpm --filter @objectstack/spec run check:authorable-surfacebuild-schemas.ts --checkaccepts the committed tablescripts/pm/dispatch-gates.mjs --commands)dist/and were re-run after a buildNo changeset —
skip-changesetMeasured rather than assumed.
packages/spec'sfiles[]shipsdist,json-schema,liveness,prompts,llms.txt,README.md,src/**/*.zod.ts,CHANGELOG.md,api-surface,spec-changes.json. This diff touches onlypackages/spec/scripts/**.ConnectorSchema, a published symbol: 4 published paths hitcheckRenameBaselineCollisions(new): 0checkRenameTableandcarryAuthorableKey, siblings in the same build-script modulethat have shipped through many releases: 0
The sibling control is the load-bearing half: build-script symbols have never reached a⚠️ The dispatch's declared file surface listed
published path, so this is a measurement across releases rather than an argument from
construction. Nothing published moves.
"a changeset"; this deviates from it deliberately and is flagged in the round report so
the seat can reverse it in one step.
验收备注
范围外发现,均未立卡:
checkRenameTable"refuses exactly four shapes",实为五条 ——链式改名(
A -> B -> C)是函数末尾独立的一遍。属于卡片表述,不是代码缺陷。陈述的损害更宽;本次新规则只拒交集。这是一处不对称,但方向偏严、不违反任何契约,
因此记录不立卡。承接者:下一个向
RENAMED_DEFS添加合并型条目的 PR。registeredRetiredKeys()(build-schemas.ts)是第六个 carry 站点,它刻意合并(取最早的 major)而非覆盖,不受本次影响。记录以免下次读成遗漏。
维护者速读(草稿)
改了什么 ——
packages/spec的构建期闸门新增一条规则:声明的 def 改名,如果基线快照在源 def 和目标 def 下记录了同一个属性名,构建直接失败。新增
checkRenameBaselineCollisions,在build-schemas.ts的两处基线(in-tree 快照与upstream 锚点)各调用一次。只动构建脚本,不动任何已发布内容。
为什么改 —— 现有的
⚠️ 今天树里没有这种改名,这是潜在而非现行缺陷:修它的理由是闸门的保证比它读起来
checkRenameTable只能看到"这次构建发出了哪些 def",看不到基线。所以单条改名
A -> B撞上一个基线里已有 keys 的B时,五条现有规则全部放行,而所有 carry 都是
Map.set,两条记录塌成一条、后写的赢。丢掉的是属性的 retired 状态与 default 指纹,而且塌陷发生在任何比较之前,因此下游每个 ratchet 都在已被污染的
输入上判案 —— 双向出错:真实的 default 变更会被吞掉,没变的会被报成
changed。更窄,不是现在坏了。
风险与代价(含回滚) —— 代价是闸门可能误拒一次合法改名。卡片建议的字面判据
("目标在基线里已有 keys 就拒绝")实测会当场让
main变红:39 条已提交条目里有 24 条的目标在 in-tree 快照下已有 keys,而且
cloud/Sha256Digest -> system/Sha256Digest这条在树里就是"改名到一个已存在的 def"。所以本次只拒交集(两边同名的属性),
不相交的合并逐字节无损、照常放行,并由 Leg B 消融钉死。实测
main两份基线的交集均为 0,
check:authorable-surface绿。回滚成本极低:整个改动是构建脚本加一条规则,git revert即可,不涉及任何已发布载荷、导出符号或生成产物。席位意见 —— (留空,待席位定稿)
你要做的 —— 两件。① 裁一下方向:卡片把"拒绝"与"带碰撞报告放行"并列且不指定,
本 PR 选了拒绝并收窄到交集,理由与证据在正文;若你要"报告放行",需要先回答
guard four 为什么对同一损害判了拒绝。② 确认
skip-changeset这一路:实测没有任何已发布内容移动(带正控),但派发词的文件面写了"a changeset",两者冲突,由你定。
Generated by Claude Code