fix: code-review findings — registry index lock/temp collision, publish auth, CI#195
Merged
Conversation
Attacks the bugs surfaced by the max-effort code review of PR #194. Finding #1 & #2 (data integrity, index_manager.rs atomic_update): Sidecar lock and temp paths were derived with Path::with_extension, which REPLACES the last dotted segment. Skill index files are named by the bare package (no fixed extension) and package names may contain dots, so: - org/web.scraper and org/web.crawler both mapped to org/web.lock, forcing unrelated publishes to share one lock (false contention, possible 30s lock-timeout failures); and - org/data's temp path org/data.tmp was the literal index file of skill org/data.tmp, so the atomic rename destroyed that skill's index. Fix: append_suffix() derives the lock path by appending (distinct per skill), and unique_temp_path() tags the temp name with pid + a monotonic counter so it can never equal another skill's index. Two regression tests added (both verified to fail on the pre-fix code). Finding #3 (cli/commands/publish.rs): Restore the no-token guard removed in #194 — without a token, publish now fails fast with the actionable "Run `fastskill auth login`" message instead of sending an unauthenticated request and surfacing a raw 401. Finding #4 (.github/workflows/test.yml): Clippy only linted --all-features. Add a default-feature clippy pass, since release binaries are built from the default (registry-publish OFF) config. Finding #5 (orphaned root tests/): Update stale /api/registry/... mocks/snapshots to the /api/v1/registry/... contract from #194. (These files live in the virtual-workspace-root tests/ tree and are not compiled; wiring-in vs deleting is left as a follow-up.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses the code-quality review of the first commit.
- Bug CLASS, not two leaves: the collision lived in the canonical
`utils::atomic_write` too (`path.with_extension("tmp")`). Move the
collision-safe `append_suffix` into utils, route `atomic_write` through it,
and derive the index lock path via a new `get_skill_index_lock_path`
co-located with `get_skill_index_path`. Now every write path is hardened in
one place instead of patching index_manager alone.
- Kill duplication: `index_manager::atomic_update` re-implemented the
write-tmp/fsync/rename tail with the ENOSPC "filesystem full" check
copy-pasted three times. Serialize once into a buffer and classify ENOSPC
once via `is_disk_full`. index_manager.rs shrinks 571 -> 523 lines.
- index_manager keeps a UNIQUE temp name (it holds its own sidecar lock across
the read-modify-write, and a deterministic temp could equal another skill's
index); atomic_write keeps a DETERMINISTIC temp (it doubles as the advisory
lock token). Tests in utils/lock updated to the new tmp derivation.
- Drop the dead-test churn from the first commit (those root tests/ files are
uncompiled); wiring-in vs deleting tracked separately.
All four index_manager regression tests still pass (and still fail on the
pre-fix code); utils/lock/registry_index tests green; clippy clean on both
default and --all-features.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Attacks the bugs surfaced by a max-effort code review of #194 (registry-publish feature gate). Five findings; the first two are data-integrity bugs in the registry index writer.
Findings & fixes
#1 — Sidecar lock collision (data integrity) ·
index_manager.rsatomic_updatederived the sidecar lock path withPath::with_extension("lock"), which replaces the last dotted segment. Skill index files are named by the bare package (no fixed extension) and package names may contain dots, soorg/web.scraperandorg/web.crawlerboth mapped ontoorg/web.lock— forcing unrelated publishes to share one lock (false contention, possible 30s lock-timeout failures).Fix:
append_suffix()derives the lock by appending, keeping it distinct per skill.#2 — Temp-file clobber (data loss) ·
index_manager.rsThe temp path
with_extension("tmp")could equal another skill's real index file: writingorg/dataused temporg/data.tmp, which is the literal index of skillorg/data.tmp, so the atomic rename destroyed that skill's index.Fix:
unique_temp_path()tags the temp name with pid + a monotonic counter, so it can never collide with an index file.Two regression tests added — both verified to fail on the pre-fix code and pass after.
#3 — Restore the no-token guard ·
cli/commands/publish.rs#194 dropped the early validation, so a missing token sent an unauthenticated request and surfaced a raw 401. Restored the actionable error:
Run \fastskill auth login``.#4 — Lint the shipped config in CI ·
.github/workflows/test.ymlClippy only ran
--all-features. Added a default-feature clippy pass, since release binaries are built from the default (registry-publishOFF) config.#5 — Stale URLs in orphaned tests ·
tests/Updated
/api/registry/...→/api/v1/registry/...mocks/snapshot to match the #194 contract. (These files live in the virtual-workspace-roottests/tree and are not compiled — wiring-in vs deleting left as a follow-up.)Verification
cargo fmt,clippy(default and--all-features), and the full test suite (174 tests) pass.Deliberately not done (needs a decision)
publish—release.ymlbuilds the default config and never passes-F registry-publish. Whether the distributed CLI should keep thepublishcommand is a product call; left untouched.🤖 Generated with Claude Code