A stamp-shaped directory name is not ownership proof: pruneSkillBackups
counted and RemoveAll'd any 20260819-120000-shaped entry under
~/.dws/skill-backups, so a user or tool that created such a directory
lost its contents once DWS held five backups, and the Go backup path
(MkdirAll) adopted a same-named foreign root outright. The PowerShell
installers already implemented the correct contract; every other
surface now matches it.
Go stamps a freshly created root with the exact marker bytes the
install scripts write (.dws-skill-backup = "dws skill backup v1")
before any payload moves in, claims the root with mkdir so an existing
unproven root bumps to a collision suffix instead of being adopted,
and prunes only roots whose marker verifies — unmarked or wrongly
worded stamp-shaped directories are foreign data, preserved and never
counted against the keep limit. The shell installers (install.sh,
install-skills.sh, install-event.sh, install-devapp.sh) and the npm
installer apply the same rule in their backup collision loops, with
roots recorded as created by the running process exempt from marker
re-verification so a mid-run marker permission failure still reuses
this run's own root and keeps the sibling payload intact.
Regression tests cover every surface: pruning an unmarked/wrongly
marked stamp-shaped directory alongside marked ones, refusing to adopt
a foreign root (payload moves to a suffixed root, foreign data and its
nonexistent marker untouched), same-stamp reuse of a proven root, and
marker-write failure cleaning the empty fresh root.
The platform coverage gates execute only TestCrossPlatformCoverage-named
tests, so the child-move error and dispatch branches that the full local
suite covered incidentally were reported as uncovered changed code on
Windows (96.78% vs the 100% target). Adds a seam-driven edge suite for
the child-move fallback — source/claim/child stat and read failures,
per-child link and symlink collisions and publish failures, rollback
rename failure, foreign-entry abort, mode-restore failure, source shell
removal failure, nested-directory and simulated-symlink children, and
post-rename content drift — plus the retained-destination notice for a
dependent uncertain target in skill setup. The POSIX file identity impl
now consults the lstat seam so its degradation branches are coverable
the same way. Verified against the gate's own changed-line computation:
zero uncovered changed statements in internal/upgrade.
The publish-confirmation and tunneled-replacement tests physically
removed and reseeded the destination to simulate a concurrent swap. On
NTFS the recreation can immediately reuse the freed MFT record, making
the file ID (volume serial + file index) compare equal and the proof
pass against a replaced object — the Windows coverage gate observed the
confirmation falling through to the fingerprint branch instead of the
identity branch. Both tests now force the replacement through the two
primitives the platform proof consults (os.SameFile on Unix, the file-ID
seam on Windows), matching the technique the tunneled-rollback case
already used for Unix inode recycling.
skill_publication_identity_linux_test.go pinned the remote line's
Statx/birth-time identity design (skillPathStatx seam); the merged head
proves ownership with dev:ino plus the fingerprint backstop instead, so
the test no longer compiles on Linux. Caught by CI's Linux lint job,
which builds what macOS-local vet skips behind the linux build tag.
Reconciles the two parallel evolutions of PR #996 with this session's
publication design as authoritative:
- internal/upgrade, internal/app: ours — mkdir-claim identity witness
(dev:ino on POSIX, volume file ID on Windows), three-state ownership,
ErrSkillPathPublicationUncertain, copy-fallback short-circuits. Drops
the remote line's xattr publication-mark design and its six follow-up
fixes (retract contracts, Statx token); skill_publication_mark_*.go
removed accordingly.
- scripts/, build/npm/, test/scripts/, docs/rfc: theirs — same replayed
install hardening plus main's evolution and the npm no-clobber child
moves; no xattr dependency, consistent with the claim model.
- .changes: their npm/Shell/PowerShell narrative with the Go-design
sentences rewritten for the uncertain-publication contract.
Verified: go build, go vet (tests compiled), gofmt, and package tests
for internal/upgrade, internal/app, test/scripts all green on this tree.
The mkdir->rename->remove->rename directory fallback had a TOCTOU window
between the second remove and the second rename: a concurrent writer
creating an entry at the destination was silently clobbered. The fallback
now claims the destination once with mkdir and never unlinks it: the
fast-path rename publishes over the claim (Linux), and platforms that
refuse directory renames (macOS, Windows) move the staged children into
the claim through atomic no-clobber primitives (mkdir/os.Link/os.Symlink),
consuming the emptied source shell on success.
renameSkillPathNoReplace now returns the mkdir-claim identity captured by
the child-move path. PublishSkillPathNoReplace uses it as a three-state
ownership witness: the atomic/fast paths keep the staged-inode proof, the
child-move path proves dest is still the mkdir claim, and a mismatch
reports the new ErrSkillPathPublicationUncertain sentinel with the
destination retained. The witness is real on POSIX now: darwin and linux
report the dev:ino file identity instead of the empty no-op.
Upstream consumers honor the sentinel: the mono/multi upgrade copy
fallbacks no longer retry over an uncertain destination (the retry would
displace the concurrent writer's object), and skill setup reports the
retained destination instead of claiming a rollback.
Rewrites the fallback tests that pinned the removed remove-and-retry flow
and adds regression coverage: concurrent claim entries abort with the
destination retained, wholesale replacement after child-move reports the
uncertain sentinel, staged-set transactions pass the sentinel through,
and both copy fallbacks short-circuit (ablation-verified).
Windows coverage gate only runs TestCrossPlatformCoverage*, and the
xattr mark helpers are Unix-only. Inject seams so marked dest is
retracted on owned drift, left in place when the mark is gone, and
the helper error paths are exercised on every platform.
Linux overlayfs recycles device+inode, so SameFile and a lone inode
token treated a replacement as owned and retracted it. Stamp staged
inodes with an xattr mark, prove Linux/Darwin identity with birth
time, and make shell copied-set rollback check dest first with inode
plus child names.
Record dest on occupy and retract it when confirmation, verify, or
staging cleanup fails. Restore unmatched quarantine with a no-replace
publish. Event/devapp copy uses mkdir-claim; shell rollback claims dest
before delete. Release copy now says npm/PowerShell create junctions and
Go uses os.Symlink, with copy fallback when linking is unavailable.
Cross-filesystem Skill moves now record publication identity as soon
as the staging path is renamed onto dest. A later mode-restore, copy
verification, or staging-cleanup failure retracts that proven dest so
retries are not blocked by an untracked leftover. A failed retract
reports an uncertain state naming both retained locations.
The POSIX shell installers staged shared Skill links and published them
with mv after an existence check; a file or symlink another process
created at the destination between the check and the move was silently
replaced, and the inode confirmation could not detect the loss. Publish
by creating each link directly at its destination instead — symlink(2)
refuses an occupied path with EEXIST, so the creation itself is the
atomic no-replace check. A directory that appears at the destination
turns ln -s into a container; the nested link is removed after an
identity check and the transaction rolls back, leaving the foreign
directory untouched. Applied to install.sh, install-skills.sh,
install-event.sh, and install-devapp.sh. Also covers the remaining
retraction branches of the Go shell-removal fallback so changed-code
coverage is complete. Regression tests inject a concurrent occupant at
the publish instant for regular-file and directory cases and assert the
foreign object and its contents stay completely unchanged.
On filesystems without atomic no-replace rename, the degraded
publication moves the source children into a fresh claim and leaves an
emptied source shell for the caller to remove once the move is
confirmed. If that removal failed, moveSkillPathRecoverably reported a
plain failure claiming both locations were preserved while the data
existed only at the destination, so backupAndRemoveSkillDir never
recorded the backup and the original path was left empty. Move the
children back into the shell and withdraw the destination instead; a
failed retraction reports the data location explicitly. Restores the
contract that a failed move keeps the source intact.
The no-replace file fallback links the destination and then removes the
source. If the removal fails, the caller treats the publish as failed,
but no publication record exists to roll the new destination back, and
a backup restore would refuse the occupied path. Remove the destination
behind an identity check — only the proven linked object may be deleted
— and report when the retraction itself fails or the destination was
concurrently replaced.
The previous wrapper returned the first observed ID for both paths, so
expected == actual still held on Windows and the proof accepted the
swap. Return a distinct ID for the second probe.
The constant file-ID stub made both IDs equal, so the Windows proof
(expected == actual) accepted the publication and the subtest failed
there; Unix stayed green because its proof ignores the ID strings and
the swapped os.SameFile seam already forced the failure. Return the
first observed ID for both paths so staged and published identities
differ on every platform while real IDs still flow through the wrapper.
The physical same-content swap relied on the recreated destination
getting a fresh inode, but CI runners' ext4/overlayfs recycle inodes
eagerly, so the swap was undetectable on Linux and the subtest failed
there (while passing on macOS). Swap the same-file identity seam instead
so the confirmation's fast-path rejection contract is pinned on every
platform.
The degraded directory publication claimed the destination with mkdir, then
— on platforms whose rename refuses to replace a directory (macOS refuses
even an empty target, verified empirically) — removed the claim and retried
a plain rename. Between the unlink and the retry a foreign directory could
appear at the destination and be silently overwritten, breaking the
no-replace contract the fallback exists to provide.
Hold the claim for the whole transaction instead: rename over the claim
where the platform permits it (Linux), otherwise move the source children
into the claim one by one. The destination is never unlinked, so a
concurrent creator can only ever lose the mkdir race; every child rename
targets a nonexistent path inside the empty claim, and a failed move
restores the children and removes only the claim.
The child move legitimately changes the publication's identity, which the
confirmation now handles: a rename that consumed the staged path is still
proven by identity, while a child move is proven by the pre-rename content
fingerprint. The emptied source shell doubles as the signal distinguishing
the two shapes; moveSkillPathRecoverably removes it to keep move semantics.
The backup stamp has second precision and pruning kept only the newest 5
stamps, so a canonical migration that retires copies across many Agent
roots deleted its own earlier backups mid-run. That silently voided the
reversibility guarantee the transaction depends on for rollback: a probe
retiring 8 paths lost 3 of them permanently.
Record every stamp directory this process creates, keyed by normalized
absolute path, and prune only the oldest foreign stamps.
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.
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).
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).
A failed canonical publish only failed the upgrade when
hasDependentSkillRoot reported a non-universal link target; that helper
explicitly skipped universal agents, which are exactly the direct consumers
of ~/.agents/skills. On a universal-only machine (e.g. only Codex
installed), UpgradeSkillLocations* returned a nil error with nothing
installed, contradicting the documented "canonical publication is
mandatory and fails the upgrade loudly" contract.
Canonical publish failures now return an error unconditionally in both the
mono and multi branches, and hasDependentSkillRoot is removed. The test
that pinned the old standalone-does-not-fail-fast behavior now asserts
error propagation in both modes.
The allowSystemApps gate (homeDir == systemHome) was effectively a no-op in
production: systemHome came from os.UserHomeDir, which honors the $HOME env
override just like homeDir, so the two were always equal and the gate never
fired when $HOME was overridden.
ResolveSystemHomeDir now prefers the OS user database (getpwuid on Unix),
which is independent of $HOME, falling back to $HOME only when the user record
cannot be resolved. Production behavior is unchanged (a real $HOME still
matches); an isolated/overridden HOME now correctly skips machine-wide
/Applications discovery for zcode/minimax. The app surface references the same
shared resolver.
This is the correct fix for the hermeticity concern (machine-wide state leaking
into an isolated HOME): there is no cross-surface production inconsistency to
port — script installers always operate on the real user HOME in practice, so
they need no gate.
玉澜