Uh oh!
There was an error while loading. Please reload this page.
fix(cli): make os validate --json --strict exit 1 where --strict already does - #11299
Merged
Merged
Conversation
…already does (#11174) The `--json` branch emitted its payload and returned above the only `flags.strict` reader, which sat inside the text-rendering block. On one config with one flag the two faces answered differently: text exited 1 with "Strict mode: warnings treated as errors", `--json --strict` exited 0. The flag was accepted, documented for CI in `content/docs/deployment/cli.mdx`, and inert. Assemble the warning list once, above the `if (flags.json)` branch, and have both faces gate on that same list, so the two exit codes cannot drift apart again. The JSON status rides in `emitJson`'s `CliExitCode` slot — the declared channel for pairing a `--json` document with the status the shell reads — so the payload stays exactly one parseable document. The gate reads the text face's list rather than the payload's `warnings` field: the two differ by the ADR-0087 conversion notices, which the payload carries under `conversions`, and gating on the field would have left the same divergence for a conversion-only config. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR
Contributor
📓 Docs Drift CheckNothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 1 changed package(s)), so this run has no opinion about the docs. What this run could not see
Coarse fallback — 23 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
…` needs `conversions` too (#11174) The changeset told a pipeline to "drop `--strict` and gate on the payload's `warnings` array, which has carried the full advisory set since #10953". That is false in exactly the dimension this PR makes load-bearing: the payload's `warnings` field is the five-way spread WITHOUT the ADR-0087 conversion notices, which ride under `conversions`. A pipeline following that advice would have got a strictly weaker gate than `--strict` — a conversions-only config passes it and fails `--strict` — which is the same silent under-reporting this change exists to remove, published as guidance. Say `warnings.length > 0 || conversions.length > 0`, say plainly that `warnings` alone is narrower and why, and state the resulting payload shape outright: a conversions-only config exits 1 with `"warnings": []` and a populated `conversions`, so predicting the exit code from `warnings.length` is wrong for that config. Changeset prose only — no source, test or bump change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR
os-zhuang
marked this pull request as ready for review
August 23, 2026 09:42
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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#11174
commands/validate.tsemitted the--jsonpayload andreturned above the onlyflags.strictreader, which sat inside the text-rendering block. One config, one flag,two answers — reproduced on this branch's base before touching anything:
--strictwas accepted, documented and inert whenever--jsonwas passed.content/docs/deployment/cli.mdxspellsos validate --json --stricttwice in itsCI/CD section, once as a GitHub Actions step — that combination is the one audience the
flag exists for, and a pipeline gating on its exit status read 0 and called the stack
clean.
This is the second half of a pair. #10953 (PR #11175) made the four structural advisories
reachable in the payload, so a pipeline could at least gate on
warnings.length; ithoisted the advisories and did not touch the exit code, so a pipeline trusting the exit
status still could not.
Which set
--strictgates on — measured, not chosenThe card warns against reflexively moving the strict check up, because #11175 changed what
is in scope at that point in the file. The two faces turn out to gate on sets that differ
by one collection:
warningswarningscapProviderWarnings,unknownKeyWarnings,ruleAdvisories,docWarnings,structuralWarningsconversionNotices(ADR-0087 D2)conversionsfieldspecGap--strict)specVersionGapfieldSo gating the JSON face on the payload's
warningsfield would have left the identicaldivergence in place, one collection narrower: a config whose only advisories are
conversion notices would still exit 0 under
--json --strictand 1 without--json.The gate therefore reads the text face's own list, and that list is now assembled once
above the
if (flags.json)branch and consumed by both faces — the same "a single listcannot drift from itself" move this file already made for
unknownKeyWarningsand forstructuralWarnings. The push order is unchanged, so the text face's output isbyte-for-byte what it was.
This is a difference in payload shape, not a dropped advisory, and it is already
declared as such by
test/validate-json-warning-parity.e2e.test.ts, which excludesconversionsandspecVersionGapfrom its set equality and asserts both are empty forits fixtures rather than ignoring them.
Two smaller points, both deliberate:
valid: truebeside exit 1 is the text face verbatim. That path prints "Validationpassed" and then fails for strict. The stack is schema-valid;
--strictis whatpromotes its advisories to a failure. Pinned in the test, because the pairing is new
here and reads like a bug otherwise.
emitJson'sCliExitCodeslot, not a followingthis.exit(1).The failure paths above it use
this.exit(1)because they must stop a fall-through intothe text rendering; here the payload is complete and the
returnis right there. Theslot is the declared channel for pairing a
--jsondocument with the status the shellreads (
utils/format.ts, pinned byutils/format.exit-code.test.tsandtest/migrate-exit-code.e2e.test.ts), and it emits the one document with no ExitErrorunwinding through the catch.
Reverse verification — both directions, quoted
The new test was run on the pre-fix tree (
validate.tsunmodified, confirmed by anempty
git diffon it) before the fix was written, against a prediction recorded first:test 1 red, tests 2-4 green.
Pre-fix — red, and it reproduces the card's measurement on this head:
Post-fix — green:
The assertions are a parity matrix, not
expect(code).toBe(1): both sides are read fromthe real exit status of two real CLI runs and compared to each other, with an anti-vacuity
floor (the warning fixture's text run must exit non-zero) so equality is never asserted
over a run with nothing to fail on, a clean fixture pinning the zero end so neither face
can move to meet the other, and a run without
--strictseparating "gates on--strict"from "fails whenever
--jsonsees a warning".Test placement
packages/cli/src/commands/validate-json-strict-exit.e2e.test.ts, not beside its siblingsin
packages/cli/test/, because that directory was held by another in-flight card and wasread-only to this change.
src/is the stronger home anyway, for the reasonutils/format.exit-code.test.tsalready gives:packages/cli/tsconfig.jsonincludessrc, sopnpm typecheckcompiles the file, while no tsc program readspackages/cli/test/.tsconfig.build.jsonexcludessrc/**/*.test.ts, so nothing ships.Verification
All gates below were run at head
79844ec3. A later commit (bb07f74f) changes thechangeset prose only — no source, test or bump change — so only the three changeset gates
were re-run at that head, after committing:
check-adr-0087-registration(
1 declared-breaking changeset(s), each carrying an ADR-0087 disposition),check-changeset-no-major(introduces no \major` bump) andcheck-empty-changeset(1 declaring changeset(s) added), pluscheck:changeset-gate-self-tests,check:objectui-changesetandcheck:nul-bytes`. All green.pnpm lint(full repo,eslint . --no-inline-config) — green in 80s. No narrowing, sonothing to declare.
pnpm --filter @objectstack/cli typecheck—> tsc --noEmit, clean.@objectstack/cli—Test Files 10 passed (10),Tests 74 passed (74), including the three existingvalidate-*suites,emit-json-pipe,commands,authoring-rule-command-parityandutils/format.exit-code.test.ts.node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack(no paths — it reads its own change set). All green:
check:changeset-gate-self-tests,check:cross-package-test-inputs,check:objectui-changeset,check:published-files,check:slot-lookup,check:test-source-alias,check:type-source-resolution,check-adr-0087-registration,check-changeset-no-major,check-ci-filter-parity,check-empty-changeset,check-plugin-teardown-shape,docs-audit/check-affected-docs,plus the convention-triggered set the new test file moves —
check:query-options-erasure,check:type-check-coverage,check:type-check-debt --re-measure,check:engine-double-contract,check:where-matcher— andcheck:nul-bytes.check:type-check-debt --re-measurefirst refused outright (@objectstack/service-knowledgehad no built type entry point), which means not measured rather than not applicable. That
one package was built and the gate re-run to a real verdict:
33 ledger entr(ies) re-measured in 553.6s, 1897 raw tsc error(s) total, none above its recorded number.origin/mainmoved 5 commits during the work; none touchpackages/cliorpackages/spec, so the build state and the numbers above stand.Changeset
minor, argued in the body rather than defaulted. A pipeline runningos validate --json --strictover a stack that raises non-blocking advisories was greenand will now be red, so this must not ship in a release a CI system can take unattended —
but nothing is removed or renamed, the accept set is identical, and the new behaviour
restores what
--strictdeclares and the docs already advertise rather than contradictinga contract, so it is not a major either. Under this repo's launch-window convention
(breaking changes ship as
minorwhile the stack versions in lockstep),minoris thehonest slot. Marked
**BREAKING**with an ADR-0087not-required (no-migration-prescription)disposition; the gate accepted it and printed the reason back.
Follow-up filed
The conversions-only exit-code cell — a config whose only advisories are conversion
notices now exits 1 with
warnings: []and a populatedconversions— is stated in thechangeset but not pinned by a test: no fixture in this repo raises a conversion. Adding
one needs a new fixture plus a full dependency-closure rebuild, so it is filed unassigned
as #11301 rather than stretched into this PR.
Generated by Claude Code
Generated by Claude Code