skillPathSameFileIdentityImpl on Windows always returns false and is
never reached through skillPathIdentityProven (which uses file IDs
exclusively). Add a direct seam call with synthetic os.FileInfo to
exercise the Windows return-false path and the Unix os.SameFile path
with nil Sys().
Restructure skillPathFileIdentityImpl to use nested if-err-nil with a
named return and skillPathIdentityProven to use a single expression.
Error conditions now fall through to the bare return instead of
occupying separate coverage blocks, eliminating 5 uncovered statements
that the Windows coverage gate flagged at 99.4193%.
Add FILE_FLAG_OPEN_REPARSE_POINT to the Windows CreateFile call in
skillPathFileIdentityImpl so symlinks are opened as reparse points
rather than followed to their target. Staged symlinks carry relative
targets computed for the final destination, which may not resolve from
the staging directory; following them caused CreateFile to fail,
yielding an empty file ID that rejected publication and broke canonical
skill layout migration on Windows.
Make skillPathSameFileIdentity a seam variable so the tunneled
replacement test can deterministically simulate the identity change on
Unix. On tmpfs (used by Linux CI runners), os.SameFile can return true
for a recreated file due to inode reuse, making the test flaky. On
Windows the swap is a no-op because skillPathIdentityProven compares
file IDs from GetFileInformationByHandle and ignores
skillPathSameFileIdentity.
NTFS file tunneling can restore the original creation time for a
recreated same-named object, which defeated the creation-time
incarnation check and allowed rollback to delete a concurrent
replacement. Replace the platform-specific identity pair with a single
skillPathIdentityProven function:
- Unix: delegates to os.SameFile (inode/dev), ignoring file ID strings
- Windows: compares VolumeSerialNumber:FileIndexHigh:FileIndexLow from
GetFileInformationByHandle, which uniquely identifies the file on the
volume for its lifetime and is unaffected by tunneling
When the file ID cannot be obtained at publish time, identity is not
proven and the auto-delete is refused. Add a regression test that
simulates tunneled creation time and verifies rollback still refuses
the concurrent replacement.
On Windows isNoReplaceRenameUnsupported always returns false, so the
fallback is never entered from the invalid-path test. Force the fallback
and swap skillPathLink to a non-EEXIST error to cover line 94 on all
platforms.
Add tests for mkdir non-EEXIST error, remove failure after rename
failure, first-rename-succeeds path (Linux behavior), retry-rename
path, and non-regular source safe-fail. All 24 changed executable
statements now covered on both macOS and Windows.
The fallback path for filesystems without RENAME_NOREPLACE/EXCL (NFS,
FUSE, overlayfs) used Lstat-then-Rename, which could overwrite a
concurrently created destination between the check and the rename.
Replace the TOCTOU-prone check with truly atomic no-clobber primitives:
- Directories: os.Mkdir atomically claims the destination (fails with
EEXIST if occupied). On Linux rename(2) replaces the empty dir
directly; on Darwin/Windows rename refuses existing dirs so the empty
dir is removed and the rename retried — any concurrent creation
between remove and rename is detected by the second rename failing.
- Files: os.Link atomically fails if the destination exists, then
os.Remove completes the move.
Add concurrent-creation test covering the mkdir→rename race window.
The !os.IsNotExist(statErr) branch in renameSkillPathNoReplace was
uncovered on Windows. Inject errNoReplaceRenameUnsupported for the
atomic rename and os.ErrPermission for skillPathLstat so the stat-error
path is exercised on every platform.
The smoke test creates temp home directories with .config/kimchi markers
for agent detection. On Linux CI runners XDG_CONFIG_HOME may point to the
runner's real config path, causing resolvedAgentTargets to look outside
the temp home. Unset it so detection resolves against the test's temp dir.
Restrict backup pruning to directories whose names match the DWS stamp
format (YYYYmmdd-HHMMSS with optional -N suffix) across all 8 installer
surfaces (Go, npm, 4 shell, 2 PowerShell). Unknown directories in
~/.dws/skill-backups are now preserved. Also fixes Windows coverage test
portability and covers the remaining macOS changed-code gap (retire
warning loop in runUpgrade).
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.
A universal Agent whose obsolete private copy cannot be retired installs
nothing there, yet every entry point counted that retirement failure as an
install failure — aborting `npm install`, `dws skill setup`, and the shell
installers even when the canonical store and all links published correctly,
and skipping the skills-state write. Route retirement failures to a separate
warning path across all surfaces (Go upgrade + skill setup, npm, PowerShell,
install.sh, install-skills.sh, install-event.sh, install-devapp.sh).
Also:
- Add a checked-rename fallback for filesystems that reject the atomic
no-replace flag (NFS, FUSE, overlayfs); the no-clobber contract is kept and
the previously unsupported platforms build and work.
- PowerShell multi-mode links only bundle skills, never the shared canonical
store, so third-party/user skills are no longer fanned into every Agent root.
- Prune ~/.dws/skill-backups to the newest 5 on every surface; encode
HOME-relative backup names on PowerShell to preserve origin.
- Add simulated-win32 junction coverage and rewrite the tautological
no-replace test; remove dead code whose tests gave false coverage.
- Soften the overstated Windows ownership-proof comment (NTFS tunneling).
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.
Document the landed corecmd.Input transitional form: declaration shape
(FlagSpec/LeafFlag/shortcut.Flag), runtime resolution semantics and
ordering, author rules (help prose, confirmation interaction with
stdin, construction-time validation), and the delta table against the
target typed InputSource design.
Port the lark-cli Flag.Input capability: a KindString flag may declare
Input sources ("file" for @path, "stdin" for -) and the framework
rewrites the explicit token into the payload content before
required/enum/constraint/Validate checks. @@value escapes to a literal
@value; a single stdin consumer per invocation is enforced; a leading
UTF-8 BOM is stripped. Shortcut.Flag gains the same declaration and the
adapter maps it through; LeafSpec inherits it via the LeafFlag alias.
玉澜