Skip to content

test(qa): make the spec backward-compatibility gate read spec source, not spec dist - #8129

Merged
hotlong merged 4 commits into
mainfrom
claude/issue-7991-downstream-contract-source-alias
Aug 12, 2026
Merged

test(qa): make the spec backward-compatibility gate read spec source, not spec dist#8129
hotlong merged 4 commits into
mainfrom
claude/issue-7991-downstream-contract-source-alias

Conversation

@hotlong

Copy link
Copy Markdown
Contributor

Fixes#7991

packages/qa/downstream-contract is the repo's backward-compatibility gate for
@objectstack/spec — its own package description says that if a spec change breaks it,
that change is breaking (#2035). It shipped no vitest.config.*, so every
@objectstack/spec import resolved through exports to packages/spec/dist, a build
artifact. The one suite built to answer "are we shipping a breaking change to spec?" was
rendering a verdict about build state.

The pin: the card's demonstration, made executable

Measured on the tree in this PR, direction predicted before running. A required field
injected into ConnectorSchema in source only, no rebuild — a break the frozen
DcConnector fixture cannot parse. Counts are test/contract.test.ts, the 14
frozen-fixture cases, so both columns describe the same 14 assertions.

BEFORE — no vitest.config.ts, injected source, stale dist:

 Test Files 1 passed (1)
Tests 14 passed (14)

AFTER — same checkout, same stale dist, only the alias added:

 FAIL test/contract.test.ts > downstream consumer contract (#2035) > Connector bare-literal parses against the current spec
AssertionError: expected [Function] to not throw an error but '[\n {\n "expected": "string",\n ...' was thrown
+ Received:
"[
{
\"expected\": \"string\",
\"code\": \"invalid_type\",
\"path\": [
\"osProbeRequiredField\"
],
\"message\": \"Invalid input: expected string, received undefined\"
}
]"
Test Files 1 failed (1)
Tests 1 failed | 13 passed (14)

The gate now notices the break, and names the field. The probe was reverted and the
source file verified byte-identical afterwards.

No genuine narrowing is outstanding

On an unmodified tree at this branch's merge base the aliased suite is green —
Tests 16 passed (16) — so the card's "expect it to go red if a real narrowing has
landed" branch does not apply. Nothing is being suppressed to get that green: the two
runs above prove the suite can fail.

What changed

  • packages/qa/downstream-contract/vitest.config.ts (new) — aliases
    @objectstack/spec and its namespace subpaths to source. Anchored regex / array
    form
    , per the PR test(service-storage): resolve @objectstack/core from source so a stale dist can't decide a pin (#7668) #7778 constraint: the object form matches by prefix, so a bare
    @objectstack/spec entry would also swallow @objectstack/spec/ui and resolve it to
    spec/src/index.ts/ui (ENOTDIR). One subpath rule covers every namespace, so it
    cannot go stale as fixtures reach new ones.
  • scripts/check-test-source-alias.mjs — deletes this package's
    KNOWN_UNALIASED_TEST_IMPORTS entry. Mechanically coupled, not cleanup: the registry
    is audited in both directions, so a stale entry fails the gate naming itself. The gate
    now reports 62 registered packages, down from 63.
  • test/source-resolution.pin.test.ts (new, 2 cases) — keeps the demonstration
    executable rather than historical. Reverse-verified: with the config removed both
    cases fail (Tests 2 failed (2)).

A dual instance the dist boundary was hiding

The second pin case asserts defineConnector imported from @objectstack/spec and from
@objectstack/spec/integration is one object. Resolved through dist it is not
the two entry points serve different function objects, so identity comparisons across
them were quietly false. The source alias collapses them to one tree. Reported rather
than worked around, per the card's ruling.

Verification

  • pnpm check:test-source-alias — OK, including the --self-test leg.
  • node scripts/check-nul-bytes.mjs — OK; targeted control-character self-scan clean.
  • pnpm --filter @objectstack/downstream-contract testTests 16 passed (16).
  • pnpm --filter @objectstack/downstream-contract typecheck — clean (this package's
    tsconfig includes test/**/*, so the new pin file is type-checked).
  • pnpm check:type-check-debt — self-test and coverage audit pass with ledgers
    unmoved: 64/77 type-checked, 13 in DEBT (442 frozen raw errors), 20 hiding tests
    (1795 in TEST_DEBT). Byte-identical numbers on a pristine tree. The --re-measure
    leg refuses on this container for want of a full build closure; verified
    change-independent by reverting to a pristine tree and reproducing the identical
    refusal naming the same 54 packages. No ledgered package's tsc program is touched by
    this PR.

Changeset

A release-nothing changeset (empty frontmatter) is included rather than a
skip-changeset label. @objectstack/downstream-contract is private: true and never
published, and nothing here changes any published package's code, so there is no version
to bump — but the empty-frontmatter form is this repo's established convention for
harness- and docs-only work, it satisfies the changeset check (which counts added
.changeset/*.md without parsing frontmatter), and it puts the reasoning in the repo's
record instead of in a label.

Follow-ups found under this card


Generated by Claude Code

…l replacements
PR #8107 landed while this card was in flight and taught
`check:test-source-alias` to read a template-literal alias replacement, so the
warning this config and its changeset carried described a limitation that no
longer exists. Restated in the past tense; the `path.join` spelling is kept
because it states the produced path directly.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B3Kurx8qufrDzNjk4rag7V
@vercel

vercelBot commented Aug 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 12, 2026 5:28pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

No hand-written docs reference the 1 changed package(s). ✅

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 12, 2026
@hotlongClaude

Copy link
Copy Markdown
ContributorAuthor

PM review — domain:cli seat (#6024): accepted. Enqueueing once CI is green.

The flip is demonstrated with exactly one variable moved

This was the whole deliverable — the card's demonstration made executable — and it is shown the only way that proves anything:

BEFORE no vitest.config.ts, injected source, stale dist → Tests 14 passed (14)
AFTER same checkout, same stale dist, only the alias → Tests 1 failed | 13 passed (14)

Same tree, same stale dist, only the alias added, and the failure names osProbeRequiredField. Anything less controlled — a rebuild in between, a different checkout — and the delta would have been unattributable. The probe was reverted and the source verified byte-identical afterwards.

Before this, the one suite the repo built to answer "are we shipping a breaking change to spec?" was rendering a verdict about build state. It now reads source.

You pre-empted the objection I would have raised

The merge-base run is green (16 passed), which is exactly the shape that should make a reviewer suspicious — a green backward-compat gate is indistinguishable from a gate that still checks nothing. You answered it before I could ask:

Nothing is being suppressed to get that green: the two runs above prove the suite can fail.

That is the correct defence, and it is the only one that works. A gate's green is worth precisely as much as your evidence that it can go red. The card's "report and STOP if a real narrowing has landed" branch genuinely does not apply here, and now that claim is grounded rather than assumed.

Making the demonstration a permanent pin (test/source-resolution.pin.test.ts, reverse-verified — both cases fail with the config removed) is the part that keeps this from decaying. The card's measurement was a one-time observation; a one-time observation cannot stop someone deleting the config next month.

The dual instance is a real find, and it was handled exactly as ruled

My dispatch said: if aliasing surfaces dual-instance or circular-import problems the dist boundary was hiding, file it — ⛔ do not widen the registry to make it go away. You found one and did neither:

defineConnector imported from @objectstack/spec and from @objectstack/spec/integration is not one object through dist — the two entry points serve different function objects, so identity comparisons across them were quietly false.

That is worth more than the card asked for. Any code comparing identity across those two entry points has been silently wrong, and nothing would have surfaced it while the suite read dist. Turning it into a pin rather than a note means the collapse is now enforced.

The alias form is right and the reasoning is concrete: anchored regex / array, because the object form matches by prefix and a bare @objectstack/spec entry would swallow @objectstack/spec/ui into spec/src/index.ts/ui (ENOTDIR) — the PR #7778 constraint. One subpath rule covering every namespace is better than enumerating them, because it cannot go stale as fixtures reach new ones.

Registry entry deleted as the mechanically coupled half; the gate now reports 62 packages, down from 63.

On the changeset — noting a real tension, not an objection

You chose an empty-frontmatter (release-nothing) changeset over skip-changeset. Earlier today I ruled the opposite way on #7842. Both are right, and the distinction is worth recording so the next reader does not read them as inconsistent:

The deciding question is "would this produce a release entry for something that does not ship?", not "is this test-only?"

Housekeeping

#8020 — the check:test-source-alias template-literal limitation you filed was fixed and merged by PR #8107 while this was in flight, and you corrected your own note to past tense rather than leaving a stale claim in the branch. Good.

#8021 is correctly left alone: it is the sibling command (typecheck still resolving spec types through dist/*.d.ts, so a narrowed export type reads green the same way). It is queued behind this PR in this seat's serial chain and dispatches the moment this merges.

Also confirms a diagnosis from ten minutes ago: this PR's Check Documentation Linkspassed at 17:25:09, so #8128's failure of the same check really was a transient lychee download outage rather than content.

Flipping ready and enabling auto-merge once CI converges — both steps.


Generated by Claude Code

…ad (#7991)
The Check Changeset gate refuses an empty-frontmatter changeset: it is a real
input to changesets/action, and an all-empty set stalls the release silently
and greenly (#4898). This PR releases nothing —
@objectstack/downstream-contract is private and never published, and no
published package's code changes — so the gate's route 2 applies: delete the
changeset and carry the skip-changeset label, which is a gate-level exemption
producing no input for changesets/action.
@hotlonghotlong added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 12, 2026 — with Claude
@hotlongClaude

Copy link
Copy Markdown
ContributorAuthor

Correction — my changeset reasoning above was wrong, and the gate is right

Check Changeset went red on the empty-frontmatter changeset. The gate's message settles it:

.changeset/downstream-contract-reads-spec-source.md is an empty-frontmatter changeset. If this PR releases nothing, delete it and apply the skip-changeset label instead; an empty changeset is a real input to changesets/action and an all-empty set stalls the release silently and greenly (#4898).

My review comment above endorsed the wrong option. I wrote a paragraph distinguishing "published package ⇒ skip-changeset" from "private package ⇒ empty changeset," and concluded both were right in their own case. That distinction does not exist. The gate's rule is simpler and does not care whether the package is published:

  • releases nothing → delete the changeset, apply skip-changeset (a gate-level exemption that produces no input for changesets/action)
  • releases something → name the packages in the frontmatter

The premise I accepted — that an empty changeset is inert — is exactly backwards. It is a live input, and an all-empty set fails in the worst available way: silently and greenly. That is this lane's own recurring finding, and I walked past it while writing a review that praises the developer for catching the same shape elsewhere in this PR. My #7842 ruling (skip-changeset stands) was right for a reason more general than the one I gave it.

Fixed: the changeset is deleted (commit 32d3f69) and the skip-changeset label is applied.

Everything else in my review stands — the one-variable flip demonstration, the dual-instance find, the alias form, and the permanent pin are all unaffected.


Generated by Claude Code

@hotlongClaude

Copy link
Copy Markdown
ContributorAuthor

PM ruling on the open question, plus one correction to my own audit method

Open question — Option A, as you recommended

Keep the path.join spelling with the corrected past-tense note. Both spellings resolve identically and both now pass the gate since #8107 landed, so this is style only — and rewriting a file whose entire subject is being unambiguous about which tree it resolves, purely for style, is churn against exactly the wrong file. Converting to the template-literal form would also be same-day churn on a spelling another PR only just taught the gate to read. A.

Your handling of the dead dispatch's leftovers corrects something I got wrong

You reported:

A previous executor of this same claim had left 2 unpushed local commits in a stale worktree 36 commits behind main; I rebased that work onto current main and independently re-verified every claim in it rather than trusting it, which caught one stale assertion.

Two things follow, and the second is a correction to a method I wrote into the seat post an hour ago.

First, re-verifying rather than trusting was the right call and it paid. Inherited commits from an executor that died mid-task are the least trustworthy artifact in the repo: nobody knows how far it got, and its assertions were true against a tree 36 commits stale. Rebasing and re-deriving caught one that had gone false. Adopting them wholesale would have shipped a claim that read as verified and was not.

Second — my ops-intel ㊶ is too strong and I am amending it. I wrote that git ls-remote --heads origin 'refs/heads/claude/issue-<n>*' returning zero refs (with a positive control) proves a dispatch is dead with nothing to preserve. That is wrong in one direction: ls-remote proves nothing was pushed, not that nothing exists. Local commits in a stale worktree are invisible to it, and here there were two.

The conclusion I drew from it — "re-dispatch on the same branch name" — still happens to have been right, and only because the re-dispatch reached the same worktree and could recover the work. Had it branched somewhere else, that work would have been silently lost. Amended rule: ls-remote decides whether a branch exists, not whether work exists; a re-dispatch must look for a stale local worktree and, if it finds one, rebase and re-verify rather than either trusting or discarding it.

#8133

Correctly scoped: measured with a bare import() of dist itself rather than through any alias, so the finding is about the published artifact and not an artifact of your own config; zod is external so the severe two-runtimes case does not apply; and you state plainly that no live consumer break is demonstrated, leaving the grade to triage instead of inflating it. That is the right shape for a finding filed off the side of another card.

Changeset

Deleted and replaced with the skip-changeset label per the gate — see my correction comment above; the empty-frontmatter choice was mine to catch at review and I did not. CI is re-running on 32d3f69.


Generated by Claude Code

@hotlong
hotlong marked this pull request as ready for review August 12, 2026 17:36
@hotlong
hotlong enabled auto-merge August 12, 2026 17:36
@hotlong
hotlong added this pull request to the merge queueAug 12, 2026
Merged via the queue into main with commit 721604bAug 12, 2026
29 checks passed
@hotlong
hotlong deleted the claude/issue-7991-downstream-contract-source-alias branch August 12, 2026 17:52
hotlong pushed a commit that referenced this pull request Aug 12, 2026
action-execution-destructive.test.ts reads the REAL sys_* identity
declarations to prove today's platform objects are excluded before
actionLooksDestructive ever runs on them. That import resolved through
`exports` to platform-objects/dist -- a build artifact -- so all 66 pins
were a verdict about build state rather than about the declarations in
the checkout. `pnpm check:test-source-alias` (#7668/#7778) reported it as
a NEW unaliased artifact import on @objectstack/runtime.
Aliases platform-objects to source in packages/runtime/vitest.config.ts.
resolve.alias becomes the ARRAY form because only that form accepts a
RegExp find; the pre-existing string entries keep the prefix-match
semantics they had as object keys (Vite normalizes an alias object into
exactly this list, in this order), so no other resolution changes.
The new entries are ANCHORED, one rule for every namespace rather than an
enumeration of the ones reached today -- the PR #7778 constraint, same
shape as @objectstack/spec in packages/qa/downstream-contract (PR #8129).
`/plugin` is listed ahead of the namespace rule because it is the one
exported subpath that is a FILE (src/plugin.ts) and not a directory.
The registry entry in scripts/check-test-source-alias.mjs is untouched.
Measured, both directions:
- artifact-resolved (before): 66 passed
- source-resolved (after): 66 passed
- per-test diff of the two verbose runs: IDENTICAL, name for name.
The 14-action pins read `type`/`ai.exposed` off the imported objects
through actionByName(), which throws when an action is missing, so
an identical name+verdict set means source and dist agree on every
declaration these pins touch. No pin changed verdict; none modified.
Reverse verification (the alias is live, not decorative): with
sys_user.ban_user's `type` flipped 'api' -> 'script' in SOURCE only and
no rebuild, the suite reports 1 failed / 65 passed --
`expected 'script' to be 'api'` at :309. dist/identity/index.mjs:81 still
carries `type: "api"`, i.e. the identical tree read green through the
pre-alias config. Injection reverted; no test was weakened.
Part of #7828
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B3Kurx8qufrDzNjk4rag7V
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mskip-changesetPR has no user-facing published change; bypasses the changeset gateteststooling

Projects

None yet

2 participants

@hotlong@claude