fix: implement codebase audit (spec 002) — security, correctness, WRITE-GATE#201
Merged
Conversation
Add the design artifacts from the audit + grilling + verification sessions. No implementation yet; these define the work and the decisions behind it. Specs: - specs/002-codebase-issues-audit.md — 38 verified findings (SEC-1..12, BUG-1..15, PARTIAL-1..11) with recommended approaches + interim workarounds; includes a verification pass (SEC-10 refuted, BUG-6 downgraded, BUG-10 escalated) and a coverage pass over previously unaudited modules (git.rs, hot_reload.rs, events/, build_cache.rs, skillopt/*). - specs/003-browser-skill-management.md — browser-based local skill manager (create/edit/add-from-source/delete/upgrade), editable-only edit rule, rides WRITE-GATE. ADRs: - 0003 — serve is read-only by default; mutations behind --enable-write; fastskill is not a security boundary (shared-deploy auth is external). - 0004 — a bare version constraint is an exact pin, not caret; preserve via normalization in the semver::VersionReq migration. CONTEXT.md: add the "Version constraint" glossary term. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ement Grilling settled spec 003's five open questions and corrected the model: the browser manages skills as install-units (install-from-source / update / remove), NOT form-based content authoring. A Skill is multi-file, so editing SKILL.md via a form is the wrong tool. - O1 (moot): no form create, so no id derivation. - O2: read-only SKILL.md view = escaped raw text, no Markdown->HTML in v1. - O3: lift add/install into fastskill-core (install_from_source), call in-process (no shell-out); update uses the already-core UpdateService. - O4 (moot): no create-from-form. - O5: update = re-fetch from source and overwrite (resembles `fastskill update`), versionless by default. Two resolution models documented: ref/source-based (local/git/zip, versionless) vs version-based (registry, ADR-0004). Version picker is a registry-only future refinement. Reconciled spec 002: PARTIAL-1 back to REMOVE (create_skill/update_skill were never the intended shape); WRITE-GATE write-set now POST /skills/install + POST /skills/update (was upgrade) + DELETE, dropping the form create/field-edit routes; triage order updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…I eval Implements spec 002 findings across disjoint core/CLI packages (all green: build + full test suite + clippy -D warnings). version/resolution (version.rs, reconciliation.rs, dependencies.rs, change_detection.rs, resolver.rs, dependency_resolver.rs): - BUG-2/3/4/5: VersionConstraint now backed by semver::VersionReq; bare version normalized to =X.Y.Z per ADR-0004 (exact pin, not caret). - BUG-9: reconciliation evaluates the declared constraint (Mismatch). - BUG-6: add_skill dedups reverse edges; Kahn uses saturating_sub. - BUG-7: change-detection uses Path::strip_prefix on components. - BUG-15: skill hash length-prefixes fields (no collision). - PARTIAL-8: rename resolve_dependency_list; cycle detection via ancestor set (no diamond false-positive). zip/git/install security (storage/zip.rs, zip_validator.rs, storage/git.rs, add/install.rs): - SEC-3: fixed decompression ceiling (50MiB/10MiB/10k/ratio 100), wired into the validator stubs; BUG-11: real directory entries. - SEC-11/12: git clone/checkout `--` + protocol allowlist + cred redaction; BUG-12: real CloneFailed error. - SEC-4: copy_dir_recursive rejects symlinks. core-misc (version_bump.rs, utils.rs, events/event_bus.rs, content_safety.rs): - BUG-1: version bump via toml_edit (preserves id/[tool]). - BUG-8: atomic_write uses unique temp + persist (no lock race). - BUG-13/14: event history VecDeque (keeps newest); dispatch releases lock before awaiting handlers. - SEC-8: content-safety patterns are advisory warnings, not blocking. CLI eval/runtime (eval/run.rs, score.rs, report.rs, validate.rs, common.rs, runtime_selector.rs, standard_validator.rs, routing.rs): - PARTIAL-2: wire eval --no-fail/--trials/--ci/--threshold. - PARTIAL-7: reject grid/xml for eval commands. - PARTIAL-5: real runtime detection via aikit_sdk::get_installed_agents. - PARTIAL-9: drop substring dir-structure check; remove stale comments. Also: lock.rs obsolete advisory-lock test replaced with last-writer-wins; added toml_edit direct dep. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…surface
Implements the remaining spec 002 findings (all green: workspace build +
461 tests + clippy -D warnings).
HTTP / WRITE-GATE (http/server.rs, handlers/{skills,reindex,status,registry},
cli serve.rs, config.rs):
- SEC-1/SEC-2 WRITE-GATE: serve is read-only by default; `--enable-write`
opts in. Write routes always registered but wrapped in from_fn_with_state
middleware returning 403 ("start server with --enable-write") when
disabled. enable_write threaded serve.rs -> FastSkillServer -> AppState.
- PARTIAL-1: removed the non-functional create_skill (POST /skills) and
update_skill (PUT /skills/{id}) handlers + routes.
- SEC-2 hardening: upgrade_skills validates skillId against known skills
and passes `--` before the positional id.
- SEC-7: HTML-escape skill name/description in the dashboard.
- SEC-9: unique tempfile dir for the sources temp file.
- SEC-10: reject "*" origin combined with allow_credentials.
- PARTIAL-6: reindex endpoints return honest 501 (core can't reach the CLI
reindex path) instead of a mock 200; write-gated.
- PARTIAL-9: drop dead _sources_path_override param (+ call sites).
install paths (add/sources.rs, install_utils.rs, registry/client.rs,
repository/client.rs):
- SEC-5: validate manifest `subdir` (reject ../absolute, contain in clone).
- SEC-6: validate registry `scope` before joining into storage path.
- SEC-9: unique tempfile dir for the repo clone.
- BUG-10: sort versions by semver at both sites (get_versions +
resolve_registry_version) — was installing the wrong version.
- PARTIAL-3: implement install_from_zip_url (download + SEC-3-capped extract).
removals:
- PARTIAL-4: delete the inert tool_calling subsystem (module, re-exports,
Service::tool_service, lib doc example).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Tests-only (no behavior change). ~200 new tests across the modules touched by the spec 002 implementation. All green: build + 664 tests + clippy -D warnings. Per-file line coverage (before -> after): - events/event_bus.rs 54% -> 99.8% - core/resolver.rs 14% -> 96.5% - core/change_detection.rs 60% -> 97.1% - core/dependencies.rs 90% -> 97.8% - storage/zip.rs 83% -> 90.4% - registry/client.rs 13% -> 98.0% - repository/client.rs 6% -> 91.3% - cli/src/utils.rs 31% -> 92.1% - http/server.rs 69% -> 90.4% - handlers: reindex 100%, status 96%, resolve 96%, manifest 83% New HTTP integration suites under crates/fastskill-core/tests/ (http_handler_route_tests, http_server_route_tests). Added wiremock + zip dev-deps to fastskill-cli for mocked-HTTP / zip-fixture tests. Residual <90% is network/subprocess/embedding-bound (git clone, live registry download, OpenAI semantic search) — not unit-testable without live services; left uncovered rather than adding flaky tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to the WRITE-GATE breaking change: serve-command.mdx now documents the --enable-write flag, the read-only-by-default model + 403 behavior, the not-a-security-boundary / external-sidecar stance, and the corrected endpoint table (create/update removed, write endpoints marked gated, reindex 501). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…val) Behavior-preserving structural cleanups surfaced by the code-quality review of this branch. Net ~250 fewer lines; 658 tests green, clippy -D warnings clean. P1 — de-duplication: - Hoist a single canonical semver sort into core::version (`sort_versions_desc` / `newest_version`); remove the copies in registry/client and the CLI `select_newest_version`. - Make `safe_subdir_join` one pub(crate) helper (was byte-identical in install_utils and add/sources, tests included). - Extract one `extract_zip` helper owning the thrice-copied ZipHandler error-mapping block (install_utils + two sites in add/sources). - Collapse the triplicated SEC-3 zip preflight: `ZipValidator::validate_zip_package` now delegates to the single `ZipHandler::validate_package`. P2 — dead code removed: - `VersionConstraint::as_req` (leaked the inner VersionReq; no callers). - `DependencyResolver::topological_sort` (identity function; no callers). - Free `http::server::serve` fn (no callers). - Dead `WouldBlock -> FileLocked` branches in both `save_to_file`s (advisory lock was removed with the atomic_write rewrite); drop the tautological fs2-only contention test and the now-unused `fs2` dependency. - `append_suffix` (test-only helper behind a dead_code allow). P3 — structural polish: - `FastSkillServer`: 4 constructors -> `new`/`from_ref` + `enable_write()` builder (mirrors `AppState::with_enable_write`); drop the positional bool. - `update_skill_version`: collapse 3 write sites + 2 duplicated doc builders into one path. - Reconciliation: carry `(status, expected)` together; drop the unreachable fallback and the mutable accumulator. - eval `--trials`: carry the honest parsed value so a negative input reports what the user typed instead of `4294967295`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ong) `.gitignore` ignores `*.html` and tried to rescue the embedded dashboard with `!src/http/static/index.html`, but the file lives at `crates/fastskill-core/src/http/static/index.html` — the negation path was missing the crate prefix, so index.html was never committed. app.js and styles.css were tracked, index.html was not. Locally the file exists on disk so `include_dir!` embedded it and tests passed; in CI's fresh checkout it was absent, so `GET /` 404'd. The new `static_assets_and_dashboard_and_index_mount` test is the first to exercise embedded UI serving in CI, which surfaced the long-standing mistake. Fix the negation path and commit index.html. 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.
Implements
specs/002-codebase-issues-audit.md— the full set of security, correctness, and partial-implementation fixes from the codebase audit (verified + coverage-boosted). Design docs (spec 002/003, ADR-0003/0004) are included on the branch.fastskill serve--enable-writeflag; otherwise they return403 { "error": "write operations disabled; start server with --enable-write" }.fastskillis explicitly not a security boundary (see ADR-0003) — shared deployments front it with a sidecar/proxy.POST /api/v1/skills(create) andPUT /api/v1/skills/{id}(edit). A skill is a multi-file directory, so it is installed from a source, not authored via a JSON body. Use install/update/remove (CLIadd/update/remove).POST /reindexnow returns an honest501(runfastskill reindexfrom the CLI) instead of a mock200.What's included
Security
upgradeskillId validation and--arg hardening (SEC-2)subdirand registryscopepath validation (SEC-5/6)git clone/checkout--+ protocol allowlist + credential redaction (SEC-11/12)"*"in an origin list); guarded anywayCorrectness
VersionConstraintbacked bysemver::VersionReq; a bare version stays an exact pin per ADR-0004 (BUG-2/3/4/5)toml_editversion bump preservesid/[tool](BUG-1); race-free atomic write (BUG-8); event-bus ring buffer + lock-free dispatch (BUG-13/14); collision-free skill hash (BUG-15); graph/change-detection/dead-error fixes (BUG-6/7/12)Partial-impl
--no-fail/--trials/--ci/--threshold(PARTIAL-2); reject unsupported eval--format(PARTIAL-7)aikit_sdk::get_installed_agents(PARTIAL-5); zip-url install (PARTIAL-3)tool_calling+ the create/update stubs (PARTIAL-1/4); reindex 501 (PARTIAL-6); validator/comment cleanups (PARTIAL-8/9)Testing & coverage
-D warningsclean, fmt clean — enforced by the pre-commit hook on every commit.Notes
webdocs/cli-reference/serve-command.mdxupdated for the read-only/--enable-writechange.🤖 Generated with Claude Code