typedJSONValue marshaled a typed value and then routed the result through
rawJSONValue, which runs json.Valid before decoding. On that path the input is
whatever json.Marshal has just produced, so the validation scan can only ever
succeed: it re-read every marshaled document for nothing.
The decode step is now shared by both entry points. rawJSONValue keeps its
json.Valid check, because it still accepts untrusted input, while typedJSONValue
decodes what it marshaled directly. Across the 1121-tool set this removes about a
third of the Schema Catalog projection work: the internal/app schema suite goes
from 26.0s to 17.2s uninstrumented, and from 291.1s to 241.0s under -race.
The delivered Catalog is byte-for-byte unchanged. check-generated-drift,
check-schema-catalog and check-schema-binary each regenerate the same
source_hash sha256:93b8d44eb163bd2898c78397d22af92d378e3dc4e20f56b33277b51e4342e2e6,
and the two error contracts are preserved: typedJSONValue still rejects a value
json.Marshal cannot encode, and rawJSONValue still rejects invalid JSON.
The five internal/app partitions ran end to end inside one job, so the app
shard's wall clock was the sum of all five: 780s in CI, of which the schema
partition owned 357s. Each partition is now its own matrix shard, so they run
concurrently and the shard's wall clock is set by its slowest partition rather
than by their total. Every partition shard still selects the same single
internal/app package, so the impacted-package query maps the shard name back to
app and the partition only chooses which tests run.
The helper gains a partition argument and a list-partitions mode. APP_PARTITIONS
is the single source of truth for the set, and the discovery pass still runs in
every job, so each one independently verifies that the partition patterns cover
every top-level test exactly once before running the one it was asked for.
Two fail-closed checks guard the split, because the helper's own coverage check
can no longer prove the whole package ran once the partitions are separate jobs:
- The helper cross-checks APP_PARTITIONS against the coverage counters in both
directions, so a counted partition that nothing dispatches and a dispatchable
partition with no counter both fail instead of silently skipping tests.
- TestCIAppRacePartitionMatrixMatchesHelper pins the workflow's app-<partition>
shards to list-partitions output in both directions, so a partition cannot
lose its job while every job stays green.
The discovery loop variable is renamed from partition to spec: it would
otherwise shadow the partition requested on the command line, which run mode
reads after the discovery pass completes.
The schema partition's 52 tests assert structural Schema-to-Cobra contracts over
a single goroutine: none of them call t.Parallel or start a goroutine, so the
race detector has no concurrent access to observe there. The process-global lazy
metadata that does need race coverage (schema_source_root's atomic.Value, the
parameter-binding lazy loaders) is exercised by internal/cli's concurrent tests,
which stay instrumented.
The instrumentation was not free here. The partition shares a single sync.Once
Catalog build whose work is allocation-heavy, and -race made it roughly 11x
slower: 26s -> 291s locally, and 357s of the app shard's 780s in CI. Within that
partition TestFinalSchemaToolsHaveExecutableBaseCommands alone accounted for
262s, not because the test is expensive but because it is the first caller to pay
for the shared snapshot; its 1121 subtests together measure 0.00s.
run_partition now takes the instrumentation mode explicitly and fails closed on
an unrecognized value, so a typo cannot silently drop -race from a partition that
is supposed to carry it.
The workflow contract pinned the focused path by literal: the job name
`Test (changed packages)`, the unsharded
`list "$TEST_BASE_REF" "$TEST_HEAD_REF"` call, and a single
`go test -timeout=15m` line standing in for internal/app's package-level
headroom. Sharding the job changed all three literals, so `Test (workflow
and release contracts)` failed on this branch even though every shard
selection test passed.
Each invariant the contract guarded still holds, so the assertions are
updated to the new shape rather than relaxed:
- the focused job must still exist, now as the matrix job, named the way
the contract already names `Test (race: ${{ matrix.shard }})`;
- package selection must still derive from the authoritative synthetic
merge base/head, now with an explicit shard argument, so pointing it at
any other ref still fails the contract;
- internal/app's headroom is asserted through the process-isolating
helper and the per-shard budgets, mirroring the assertions already
applied to test-race. That is stronger than the old single -timeout: it
pins the mechanism that keeps the suite inside its budget rather than
the number alone. release-scripts membership is asserted too, because
its dedicated job only runs at full-suite or release-sensitive scope,
so losing it here would silently stop testing test/scripts changes.
The shard comparisons in the focused job are quoted so that job reads
verbatim like test-race's.
Ablating the implementation one change at a time turns the contract red
in all five cases: removing the app helper call, dropping release-scripts
from the matrix, selecting from HEAD~1, collapsing the matrix back to a
single unsharded job, and dropping the cli/smoke timeout budget.
Reading the package list with `mapfile < file` has unambiguous line
semantics. Routing it through a step output and a here-string instead
would append an extra empty array element if the value ever carried a
trailing newline, and that element would reach go test as an empty
package argument. The step output now carries only a single-line boolean,
and the list travels through RUNNER_TEMP. An explicit empty-entry guard
fails closed if the file is ever malformed.
This job cannot execute on its own pull request — editing a workflow
routes the revision to full_suite, which skips the focused path — so the
implementation deliberately avoids depending on platform-specific
trailing-newline behavior that local verification cannot observe.
The focused path tested every impacted package in a single job with a
plain `go test -race`, so internal/app ran inside one long-lived process
alongside all of its reverse dependencies. That is exactly the shape
scripts/ci/run-app-race-tests.sh exists to avoid: a single app test
process retains every constructed command tree in framework registries,
so the run grows to 900s and the job stays alive long enough to be
reclaimed by the runner. Recent focused runs failed with SIGTERM after
9-10 minutes without a single test failure, and one earlier run failed
at `internal/app 902.651s`, 2.65s past the package timeout.
Fan the same package plan across the shard matrix test-race already
uses, and run each shard the way test-race runs it: internal/app through
the process-isolating helper, cli/smoke with their wider package budget,
release-scripts without race and with archive tooling.
changed-test-packages.sh gains `list-shard`, which intersects the
impacted set with scripts/ci/test-packages.sh shard membership so shard
definitions stay single-sourced — and so an unknown shard name aborts
there rather than reporting an empty selection, which would let a
mistyped shard skip every test while reporting success.
release-scripts is in the matrix on purpose: its dedicated job only runs
at full-suite or release-sensitive scope, so omitting it here would stop
testing test/scripts changes altogether. A test pins that the shard
selections partition the impacted set exactly, so shard-plan drift
cannot silently shrink focused coverage.
Every neighbouring string in the root help listing — service
descriptions, utility descriptions, global flag usage — is hardcoded
Chinese. Routing only the feedback label through i18n therefore rendered
it in English on any host whose LANG is not zh_*, leaving a lone English
line inside an otherwise Chinese screen.
Hardcode the label and drop the two locale entries it needed. A test
assertion now pins the Chinese label so the indirection cannot return
unnoticed.
`dws --help` now closes with a Feedback section that links the
user-experience survey form, tagged with source=dws-cli so submissions
arriving through the CLI can be told apart from other channels.
The entry is deliberately root-only: this CLI is driven mostly by AI
agents, and repeating a survey link in every subcommand help would be
pure context noise. A guard test pins that boundary.
The URL is printed on its own unwrapped line — it is longer than the
help rule width, and breaking it would stop terminals from recognizing
it as a clickable hyperlink.
Fifth-review addition: declaring InputFile silently claims the whole
@-prefixed value space, which matters in this product because at-mention
style values are common (--at-user @zhangsan would report a file read
failure), and declaring InputStdin makes a literal "-" unreachable. Both
are decided at declaration time and cannot be fixed downstream, so record
them next to the confirmation rule in the author rules.
Fourth-review fix: the FlagSpec sub-field table in the homology doc is
the named authority for "what each field does and whether it reaches
Schema parameters", and RFC §5.0.2 asserts declaration fields embed into
dws.schema.*. Input satisfied neither entry, leaving its deliberate
non-projection indistinguishable from an oversight. Add the table row and
the §5.0.2 exception note so the capability stays a declared fact (Usage
prose) rather than inviting an invented annotation.
Third-review fix for a CI blocker: run-platform-coverage-gate.sh only
executes ^(TestAllShortcuts|TestCrossPlatformCoverage) yet enforces 100%
coverage of changed production lines, so the TestResolveInputFlags names
left every new input.go statement reported as uncovered. Rename them to
the gate prefix, drop three unreachable pflag Set error branches that no
test could ever cover, and add the reachable stdin read-failure case.
Verified: changed code coverage 100.0000% (67 statements).
Second-review fix: explicitInputFlagName judged usability with an
unconditional TrimSpace while rawValue only trims when Trim is set. For
a non-Trim flag a whitespace main value is usable and shadows a changed
alias; the resolver could then rewrite the shadowed alias (and fail on
its @path) while the fallback chain still read the main value. Mirror
rawValue's usable() exactly and pin the shadow case with a regression
test whose alias path does not exist.
Self-review fixes: a Trim flag receiving " @path" judged usability on the
trimmed value (rawValue) while the source prefix check saw the raw value,
so the token would ship as a literal. Trim before the prefix check. Also
build the file-read error once with a conditional hint option, and pin
the default-value/env passthrough plus Trim edge with regression tests.
Dennis