Skip to content

fix(cli): os i18n extract --check --dry-run compares instead of exiting 0 having compared nothing - #16627

Merged
os-sales merged 2 commits into
mainfrom
claude/issue-16480-check-dry-run-false-green
Sep 7, 2026
Merged

fix(cli): os i18n extract --check --dry-run compares instead of exiting 0 having compared nothing#16627
os-sales merged 2 commits into
mainfrom
claude/issue-16480-check-dry-run-false-green

Conversation

@claude

@claude claude Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Fixes #16480

os i18n extract --check --dry-run --out=DIR exited 0 having compared nothing. Both flags mean "write nothing", so the pair reads as the safest spelling to put in CI — and it was the one spelling that measured nothing.

The defect, reproduced with its positive control

Driven on one drifted fixture, two invocations differing only by --dry-run:

invocation exit what it printed
... --out=DIR --check 1 missing: DIR/zh-CN.objects.generated.ts, then Translation bundles have drifted from the schema
... --out=DIR --check --dry-run 0 Dry run — no files written (pass --out=DIR to write). and no missing: / out of date: / in-sync line at all

The first run is the second one's control: the drift is provably there, and the second reported success.

Mechanism, read off the code rather than inferred from the exit codes: in packages/cli/src/commands/i18n/extract.ts the if (flags['dry-run'] || !flags.out) branch returned unconditionally, and it sits ahead of the --check comparison block. Nothing was compared.

The fix — route (a), the graded default

The return out of that branch is now conditional on --check being off. The stdout dump --dry-run asks for still happens; when --check is on, execution falls through to the comparison and reports it. Nothing is written on either path — the write loop past the comparison stays unreachable, because --check either returns in sync or exits 1.

Measured after the change, same fixture:

invocation exit reported wrote
--check --dry-run, nothing committed 1 missing: ... + drifted sentence nothing (dir still empty)
--check --dry-run, committed but stale 1 out of date: ... + drifted sentence nothing (stale bytes unchanged)
--check --dry-run, in sync 0 1 bundle(s) are in sync with the schema nothing
--dry-run --out=DIR (no check) 0 names the directory nothing
--dry-run, no --out 0 keeps the pass --out advice nothing

⚠️ Route (b) — refusing the pair as contradictory — was not taken and no reason to prefer it turned up.

Same path, same card: the --out advice

Dry run — no files written (pass --out=DIR to write) was printed even when --out was passed, which re-teaches the reader that their directory was ignored when it was not. A run with an --out now names it; a run without one keeps the advice unchanged.

The pin, and why it is shaped this way

packages/cli/test/i18n-extract-check-dry-run.test.ts — 5 cases, driving the real CLI.

⚠️ A case asserting only the exit code would pass against a --check that compares nothing, so every case pins the reported drift beside the code. The ablation proves that is not a theoretical worry: with the fix reverted, the in-sync case shows status: +0 on both sides and fails only on inSync: true vs false. An exit-code-only assertion would have stayed green there.

The two drifted cases are stated as an equality against the same invocation without --dry-run — the card's own method — and spell out the expected values, because an equality alone is also satisfied by two runs that are both broken. The in-sync case is the falsifier no unconditional failure can pass.

Reviewer's call, flagged deliberately: the file is not named .e2e, so it runs in the per-PR and merge-queue population rather than the nightly tier. Reasoning in the file header: this regression reads green, so between reintroduction and the next nightly, every run of the pair reports success about a comparison that is not happening. Measured cost of that choice on this (shared, contended) box: 7 CLI spawns, ~11s each, ~77s for the file. Renaming it to .e2e moves it to the nightly and costs nothing else.

Verification

  • Ablation (fix committed first; mutation = the pre-fix unconditional return): on-disk proof by grep -c both directions, blob hash differs from HEAD; 4 of 5 cases red, in the predicted direction (status: 0, drift: [], drifted: false). Restore leg git checkout HEAD -- ... proven by an empty git diff HEAD and a blob hash identical to HEAD's. The subject resolves through src/ (bin/run-dev.js is the source entry point, by its own documented design), so there is no dist leg to prove.
  • Tests: vitest run --project integration test/i18n-extract-check-dry-run.test.ts — 5 passed. packages/cli typecheck — exit 0.
  • Gates: derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands (no paths) at 5dc4c14e2b. Reconciliation total: 58 commands — 56 exit 0, 2 exit 3. Artifact rosters run separately: 40 — 38 exit 0, 2 non-verdicts (one of the three was merely unwired and passes once wired against this PR, below).
  • pnpm lint (eslint . --no-inline-config, whole repo, no narrowing): exit 0, 151s.

NOT MEASURED — recorded as such, never as a pass:

command why
pnpm check:dual-build-cjs-loads exit 3, PREREQUISITE NOT MET — reads built output; 12 packages have no dist/ in this worktree
pnpm check:i18n-coverage exit 3, COULD NOT MEASURE — 12 of 13 configs linted; the 13th needs @objectstack/connector-mcp built
node scripts/check-single-claim-paths.mjs exit 1 once wired with PR_NUMBER=16627 — its own API fetch answers GitHub API 401 for /repos/.../pulls/16627/files. An environment/auth condition, not a verdict about this PR
check:react-declaration-parity exit 1, MANIFEST is not set — its registry side is objectui's manifest, deliberately outside this session's repo scope

None of the four is a finding about this diff; the first two are local build state that CI builds.

Measured after the PR existed, which is the context it was missing: node scripts/check-partof-closing-keyword.mjs wired with this PR's real body and its 2 commits (PR_BODY, PR_COMMITS_FILE) — exit 0: "carries no Part-of/closing-keyword contradiction and no closing keyword bound to a card its own sentence says it is not closing, and its 2 commit message(s) carry no card-relation trailer". That matters here because this body names #16600, #14895, #16242 and #16470 alongside its one Fixes.

Changeset

Owed, and decided by measuring that the change reaches published dist rather than by a remembered rule: after pnpm --filter @objectstack/cli build, no files written to is present in packages/cli/dist/commands/i18n/extract.js (1 hit, exit 0), with a non-zero control — a nonsense token in the same file returns 0 hits, exit 1, so the probe can fail. npm pack --dry-run confirms dist/commands/i18n/extract.js is packed and 0 src/ files are. @objectstack/cli is not private.

Graded patch. ⚠️ The one judgement worth a second opinion: a pipeline running --check --dry-run against drifted bundles starts failing on this release. The changeset says so in as many words. The failure is not new — the drift was already there and the old exit code was wrong about it — which is why this reads as a fix rather than a breaking change.

Acceptance notes

  • Filed as cli: os i18n extract --check --json exits 0 having compared nothing — the second false green on this command #16600: os i18n extract --check --json exits 0 having compared nothing — the second false green on this command, same class, different branch (if (flags.json) { ... return; } also sits ahead of the comparison). Measured here with the same positive control. ⛔ Deliberately not folded in: removing it requires deciding a new public payload contract (what a --json --check run reports when it finds drift, and with what exit code), which is a contract decision rather than the control-flow correction this card is.
  • Clause-② re-derived from the delivered diff: still no, agreeing with triage. The mechanical floor is a new exported symbol or a new key on a published payload; this diff has neither — no export changes, and the --json payload is untouched.
  • Out of scope and untouched, as dispatched: cli: os i18n extract --check failure prints a regenerate command with --locales= empty and every other flag dropped — following it yields a different bundle that fails --check again #14895's --check failure hint (PR fix(cli): make i18n extract --check's regenerate hint the invocation it was given #16470 fenced it deliberately and has merged) and cli: os i18n extract --no-objects-only --source-hashes drops every non-objects provenance record — the committed-section list is the literal 'objects', not the sections the module holds #16242's --source-hashes provenance drop. Neither is addressed here.
  • The p2 grade's escalation criterion still does not fire in this repo (measured at claim time: 0 commands carry both flags, no .github/workflows/** in the co-occurrence set). ⚠️ That reading is partial — it does not cover objectui or cloud, which are outside this session's repo scope and are therefore unmeasured, not zero. No caller in either tree was encountered while doing this work.
  • Serial clearance re-checked at push time: no open PR holds extract.ts (22 open PRs; control — 9 touch packages/**, 2 touch packages/cli/**).
  • Docs-drift rows re-verified by hand; nothing edited, because nothing is falsified. content/docs/ui/translations.mdx documents --check"--check writes nothing: it re-renders and diffs against --out, naming each stale file and printing the regenerate command" — which was true before and is true now, and holds in one more case after this change; it never mentions --dry-run. content/docs/protocol/kernel/i18n-standard.mdx documents only --locales / --out / --json for this command and never mentions --check or --dry-run. Neither page states or implies the old behaviour, so both are left alone.
  • The drift tool's stated blind spot, swept independently: no page under content/docs/ pairs i18n extract with --dry-run. The single co-occurrence is content/docs/releases/v17.mdx, where the dry-run sentence is about the retired BatchOptions.validateOnly and is unrelated; that page's own os i18n extract --check sentence is accurate. content/docs/deployment/cli.mdx's --dry-run belongs to os generate, and that page does not document i18n extract at all.
  • content/docs/releases/** was listed by the tool and deliberately not touched — release notes are written centrally at release time. Nothing in v15/v16/v17 was found to be factually wrong about this command, so there is nothing to hand off either.

Generated by Claude Code

os-sales and others added 2 commits September 7, 2026 11:39
`--check --dry-run --out=DIR` exited 0 having compared nothing: the
`--dry-run` branch returned before the `--check` block was reached, so the
same tree that failed `--check` with `Translation bundles have drifted from
the schema` reported success once `--dry-run` was added. Both flags mean
"write nothing", so the pair reads as the safest spelling for CI, and a check
that cannot fail is indistinguishable from a check that finds nothing.

The dump `--dry-run` asks for still happens; the return out of that branch is
now conditional on `--check` being off, so the comparison below runs and
reports. Nothing is written on either path.

Same branch, same card: the `pass --out=<dir> to write` advice was printed
even to runs that had just passed `--out`, which reads as "your directory was
ignored". With an `--out` the line now names it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ
@github-actions github-actions Bot added the size/m label Sep 7, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/cli, touching 2 documentable anchor(s).

2 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/protocol/kernel/i18n-standard.mdx (via os i18n extract (command, read off packages/cli/src/commands/i18n/extract.ts))
  • content/docs/ui/translations.mdx (via os i18n extract (command, read off packages/cli/src/commands/i18n/extract.ts))

3 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v15.mdx (via os i18n extract (command, read off packages/cli/src/commands/i18n/extract.ts))
  • content/docs/releases/v16.mdx (via os i18n extract (command, read off packages/cli/src/commands/i18n/extract.ts))
  • content/docs/releases/v17.mdx (via os i18n extract (command, read off packages/cli/src/commands/i18n/extract.ts))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 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.

Coarse fallback — 22 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 55bbd92e3b5072d0f943891427c3a24e84c7ea90packageMentionDocs.

Which tree this was computed on

This run read content/docs from 1cb9a2274bc859dc3a8f66795c0c2ec5f4f37f75 — the merge of head 5dc4c14e2bac9276e7a017b9dfb675544b25d1f1 into base 55bbd92e3b5072d0f943891427c3a24e84c7ea90, 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 1cb9a2274bc859dc3a8f66795c0c2ec5f4f37f75 && git checkout 1cb9a2274bc859dc3a8f66795c0c2ec5f4f37f75
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 55bbd92e3b5072d0f943891427c3a24e84c7ea90 5dc4c14e2bac9276e7a017b9dfb675544b25d1f1 && git checkout -B drift-repro 55bbd92e3b5072d0f943891427c3a24e84c7ea90 && git merge --no-ff 5dc4c14e2bac9276e7a017b9dfb675544b25d1f1

node scripts/docs-audit/affected-docs.mjs --json 55bbd92e3b5072d0f943891427c3a24e84c7ea90

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 55bbd92e3b5072d0f943891427c3a24e84c7ea90 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

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 size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[finding] os i18n extract --check --dry-run exits 0 having compared nothing — a --check that passes without checking, i.e. a false green in CI

1 participant