Skip to content

spec ratchet: checkRenameTable does not guard a rename that MERGES into a baseline target that already has keys — the carry silently clobbers #17383

Description

@hotlong

checkRenameTable (packages/spec/scripts/lib/authorable-defaults.ts and its key-side twin) guards two sources
landing on one target
. It does not guard a single rename landing on a baseline target that already has
keys
— i.e. a merge. Both the key carry and the defaults carry are Map.set, so the later write silently wins
and the baseline's own entry for that target is overwritten without a diagnostic.

Provenance — ⛔ not a defect introduced by any open PR

Found by the contract-review-tier reviewer of #17372 (step 3 of the #16325 cloud-subpath chain) while
adversarially checking that PR's in-place fix carryDefaultsThroughRenames. That fix is sound and was verified by
three-leg ablation; this is a pre-existing hole the defaults half inherited from the key half, surfaced by
reading the carry code, not by anything #17372 does.

#17372's only instance is cloud/Sha256Digest → system/Sha256Digest, which carries 0 keys — harmless, and
deliberately left alone there rather than fixed as a rider.

Why it is worth a card rather than a note

The ratchet exists so a change to an authorable default cannot land unannounced. A rename whose target already
holds keys is exactly the shape where a real default change would be indistinguishable from the merge: the
overwrite happens inside the carry, before any comparison runs, so the diff the gate reports is computed against
an already-clobbered baseline. Nothing in the current guard notices, and there is no output that would let a
reviewer notice either.

⚠️ This is a latent hole, not a live one — no current rename in the tree merges into a populated target. Stated
that way on purpose: the case for fixing it is that the gate's guarantee is narrower than it reads, not that
something is broken today.

Suggested shape, ⛔ not a specification

Make the merge case explicit rather than silent: either refuse a rename whose target already carries keys in the
baseline, or carry it and emit the collision, so the reviewer sees the merge instead of a clean diff computed
against clobbered input. Whichever way, the discriminating test is the one #17372's fix already established as the
standard here — a real default change on the merged target must still be reported as changed.

Verify against origin/main before acting: this describes the tree as of edfbc7f22's base and the shape may have
moved.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions