feat: port ask CLI from Bun/TypeScript to Rust with npm/Homebrew/cargo distribution#119
Merged
Conversation
Phase 0 of the bun→Rust port (track rust-port-20260704). Adds a Cargo
workspace beside the existing TypeScript tree:
- crates/ask — lib+bin crate published as `ask-please` (short names `ask`
and `ask-cli` are taken on crates.io), binary `ask`. clap-derive CLI
mirroring the full command surface (install|add|remove|list|src|docs|
fetch|search|skills|cache{ls|gc|clean}); `--version` reads CARGO_PKG_VERSION.
- Un-ported commands return a `NotPorted` notice and exit 2 — the surface is
honest about what works while the TS build stays the source of truth.
- rust-toolchain.toml (1.94.1), rustfmt.toml, workspace release profile.
- Track spec/plan documenting topology, crate naming, the Homebrew asset
contract to preserve, and the phased migration.
Mirrors pleaseai/code-search's distribution blueprint (npm copy-over shim,
Homebrew, cargo) minus its csp-specific crates/deps.
Phase 1 of the Rust port (track rust-port-20260704). Adapts the proven
code-search (@pleaseai/csp) distribution machinery to ask:
- npm/ask/ — the @pleaseai/ask wrapper: bin/ask.js runtime launcher
(forwards argv/stdio/signals/exit-code), install.js postinstall copy-over
(replaces the JS shim with the native binary so .bin/ask hits native code
directly — no Node on the hot path), lib/resolve.js shared platform
resolver (musl-aware, never overwritten so npm rebuild is idempotent).
- optionalDependencies pin six platform packages
(@pleaseai/ask-{darwin-arm64,darwin-x64,linux-arm64,linux-x64,
linux-x64-musl,win32-x64}).
- npm/scripts/generate-platform-packages.mjs materializes npm/dist/<pkg>/
from the built ask-<target> assets at release time. The unix asset names
double as the Homebrew formula's download names — kept in sync deliberately.
Verified locally: the launcher fallback resolves target/release/ask and
forwards args + exit code; the generator produces a valid partial-matrix
wrapper + platform packages. Not yet wired to publish (Phase 2 cut-over).
Phase 2 of the Rust port (track rust-port-20260704). Stages the three distribution channels for the Rust binary without disturbing the live TypeScript pipeline (cut-over happens once Phase 3 reaches parity): - release-rust.yml — cross-compiles crates/ask to six targets. Emits the unix asset names (ask-darwin-arm64/x64, ask-linux-x64/arm64) IDENTICAL to the current Bun pipeline and the homebrew-tap ask.rb download names, so Homebrew is unaffected at cut-over; plus linux-x64-musl + windows-x64 for the npm platform packages. Tag validation accepts ask-v<semver> (monorepo tag). - release-cargo.yml — publishes ask-please to crates.io (cargo install ask-please → ask). dry-run by default on manual dispatch; idempotent (skips an already-published version). - release-please-config.json — the packages/cli component now bumps the Cargo workspace version (/Cargo.toml generic marker) and the npm wrapper (/npm/ask/package.json) in lockstep with @pleaseai/ask. Both workflows are workflow_dispatch + workflow_call (reusable) and are NOT yet wired into the auto-release — they must not publish the stub binary as @pleaseai/ask before parity. Verified: cargo publish --dry-run packages and compiles the crate in isolation; both workflow YAMLs parse.
…tests
Phase 3 of the Rust port (track rust-port-20260704) — first logic module.
Ports packages/cli/src/spec.ts: parse_spec → ParsedSpec {Npm|Github|Unknown},
library_name_from_spec, slugify_npm_name. Preserves the exact slug rules
(scoped npm flatten `@mastra/client-js`→`mastra-client-js`, github repo split
on the first slash, no-colon → unknown with empty ecosystem). The scoped-name
regex `^@[^/]+/[^/]+$` is reproduced without a regex dep via split_once. 9
tests cover the doc-comment examples plus edge cases (deep scope path,
subpath repo, unknown ecosystem passthrough).
Phase 3 of the Rust port. Ports packages/schema/src/ask-json.ts: AskJson / LibraryEntry (untagged string|object union) / LibraryEntryObject / StoreMode, plus specFromEntry/docsPathsFromEntry/entryFromSpec as methods and a free fn. serde handles structure + `deny_unknown_fields` (zod .strict() parity); an explicit validate() enforces the refinements serde can't express — the spec-string shape (`^[a-z][a-z0-9+-]*:.+$`, hand-rolled to avoid a regex dep) and non-empty docsPaths. Untagged serialization keeps the canonical no-override form a bare JSON string so ask.json stays diff-clean. 10 tests cover mixed entries, string roundtrip, camelCase docsPaths, strictness, and the refinement rejections. Adds serde/serde_json/thiserror to the crate.
Phase 3 of the Rust port. Ports packages/schema/src/resolved.ts: ResolvedEntry + ResolvedJson with Materialization (copy|link|ref|in-place) and EntryFormat (docs|intent-skills) kebab-case enums, optional fields omitted when None (skip_serializing_if), deny_unknown_fields (zod .strict()). validate() enforces the refinements serde can't: contentHash `sha256-<64hex>`, commit `<40hex>`, schemaVersion literal 1, and the superRefine rule (inPlacePath required when materialization is in-place). Datetime fields use a light RFC3339-with-offset shape check since the cache is machine-generated (a real date parser is deferred until a module actually constructs these). 10 tests. 33 tests green.
…ing) Phase 3 of the Rust port. Ports packages/cli/src/io.ts: sorted_json (deterministic key-sorted JSON via serde_json::Value normalization — the Map is a BTreeMap by default, giving the TS sortKeys byte-for-byte output), content_hash (sha256 over NUL-separated <relpath>\0<bytes>\0, order-independent), path helpers, find_entry (3-way match), read/write ask.json + resolved.json, upsert/remove resolved entries with the fetched_at-insensitive change skip. Also ports split_explicit_version into spec.rs (scope-marker vs version-sep disambiguation). Adds sha2 + time (RFC3339 now/format) deps. read_resolved_json falls back to the empty cache on any parse failure (safe-to-delete contract). 11 tests incl. tempdir roundtrips + a pinned sha256 vector. 44 tests green.
Phase 3 of the Rust port. Ports packages/cli/src/markers.ts: MarkerSyntax
{Html,Hash}, wrap/inject/remove/has. inject replaces an existing block in
place or appends with a blank-line separator (idempotent); remove normalizes
the surrounding blank lines. Marker delimiters are ASCII so str::find byte
offsets stay on char boundaries around multi-byte user content. 10 tests
cover the append/replace/idempotency and the blank-line normalization cases.
54 tests green.
Phase 3 of the Rust port. Ports the pure part of packages/cli/src/registry.ts:
parse_ecosystem, parse_doc_spec (ParsedDocSpec {Github|Ecosystem|Name} with the
github/ecosystem/name disambiguation and exact error messages via DocSpecError),
split_name_version, detect_ecosystem (marker-file → ecosystem, npm default).
The HTTP surface (fetchRegistryEntry/resolveFromRegistry, 10s-timeout fetch) +
RegistrySource type are deferred until the registry schema + HTTP client land
(noted in the module doc). 5 tests. 59 tests green.
Phase 3 of the Rust port. Ports the network-free part of packages/cli/src/lockfiles/: parse-helpers (strip_peer_suffix, strip_inline_comment, trim_quotes, clean_value, split_pkg_spec scoped-aware, is_registry_version protocol filter), the bun.lock reader (hand-rolled scan equivalent to the TS `"<name>@([^"@][^"]*)"` regex, no regex dep), package-lock.json (v2/v3 packages → v1 dependencies), and package.json (range fallback, exact:false, protocol strings skipped). npm_ecosystem_read probes the chain in priority order. The format-aware pnpm-lock.yaml / yarn.lock parsers (indent-aware stack + block parsers) are TODO — the chain const marks where they slot in to restore full priority parity. 11 tests. 70 tests green.
Phase 3 of the Rust port. Ports parseYarnLock (yarn.ts): the block-based parser handling classic v1 and Berry v2+ in one path — blank-line block splitting, comment/header/body separation, `, `-split multi-specifier headers, the `version` vs `versions:` key disambiguation, peer-suffix stripping, and the workspace-sentinel/protocol skip that lets a later block supply a real version. Wired into NPM_ECOSYSTEM_CHAIN in priority position (bun → package-lock → yarn → package.json); only pnpm-lock.yaml remains TODO. 3 tests. 73 tests green.
…arity
Phase 3 of the Rust port. Ports lockfiles/pnpm.ts: the indent-aware stack
machine (v5–v9) with a Frame enum, dependency-graph construction from
snapshots:/packages:, and BFS transitive resolution from importer roots.
Preserves the gotcha-critical behaviors: peer-suffix stripping so
`18.2.0(react@17)` never leaks the inner package, per-importer registry
filtering so a workspace/link entry can't block a real version, v5 `/name/version`
slash keys with `_peerhash` stripping, and the 4-tier priority (importer →
top-level → transitive BFS → packages fallback).
lockfiles.rs is now a module dir (lockfiles/{mod,pnpm}.rs) to stay under the
500-LOC limit. PNPM_LOCK_READER slots into NPM_ECOSYSTEM_CHAIN in priority
position, so the chain (bun → package-lock → pnpm → yarn → package.json) now
has full parity with lockfiles/index.ts. 7 tests. 79 tests green.
Phase 3 of the Rust port. Ports resolvers/utils.ts parseRepoUrl: normalizes git+https/https/git/ssh/bare github.com URLs (and ones with extra path segments) to owner/repo, stripping a trailing .git; returns None for non-GitHub or empty input. Uses the RE_GITHUB_URL regex verbatim via a LazyLock<Regex> (adds the regex dep, which upcoming agents/discovery modules also need). The per-ecosystem HTTP resolvers (npm/pypi/pub/maven) are deferred to the ureq phase. 3 tests. 82 tests green.
Phase 3 of the Rust port — the network foundation for the registry lookup,
ecosystem resolvers, and web/llms-txt sources. Adds an http module:
- HttpClient trait (get → HttpResponse{status, body}); transport failures are
Err, any HTTP status is Ok so callers branch on 404 vs other like the TS
response.ok/response.status.
- UreqClient: ureq 3.3 agent with a 10s global timeout (parity with the TS
AbortSignal.timeout(10_000)) and http_status_as_error(false).
- encode_uri_component: exact encodeURIComponent semantics (unreserved
A-Za-z0-9-_.!~*'(), per-byte UTF-8 %XX) for registry catch-all path segments
and Maven Solr params.
- mock::MockClient (cfg(test)): canned URL→response map so every network path
ports as a deterministic unit test instead of needing a live server.
3 tests. 85 tests green.
…registry)
Phase 3 of the Rust port. Adds registry/api.rs over the injectable HttpClient:
- RegistrySource: the npm|github|web|llms-txt discriminated union (serde
tag="type", kebab-case variants so LlmsTxt→"llms-txt", camelCase fields so
max_depth→maxDepth), ported from packages/schema/src/registry.ts.
- RegistryApiResponse / RegistryApiPackage: the flattened API response shape.
- fetch_registry_entry: URL-encodes the catch-all segments (scoped npm names),
returns None on 404 / non-2xx (warning with the server statusMessage, e.g. the
409 monorepo guidance) / transport failure / unparseable body — parity with
fetchRegistryEntry.
- resolve_from_registry: ecosystem/name/version split (explicit prefix or
detect_ecosystem), returns the primary source.
registry.rs is now registry/{mod,api}.rs. 7 tests via MockClient (no network).
92 tests green.
…ient
Phase 3 of the Rust port. Ports resolvers/{index,npm,pypi,pub,maven}.ts:
ResolveResult + get_resolver dispatch, and each resolver as a
`fn(&dyn HttpClient, name, version) -> Result<ResolveResult>`.
- npm: registry.npmjs.org metadata; dist-tag → semver-range best match (via the
semver crate, matching validRange+maxSatisfying) → exact; repository string|
{url,directory}; monorepo `<pkg>@<ver>`/`@v<ver>` changesets fallbacks.
- pypi: project_urls SOURCE_URL_KEYS scan → home_page fallback; v<ver> ref.
- pub: pub.dev; inverted ref convention (bare version primary, v<ver> fallback).
- maven: Search API (version + scm.url) → POM <scm><url> → POM <url>, with a
maven-metadata.xml fallback for latest; regex XML scraping via LazyLock<Regex>.
resolvers.rs is now resolvers/{mod,npm,pypi,pub_dev,maven}.rs. All network paths
tested via MockClient (no live requests). 21 new tests, 113 total green.
…ify)
Phase 3 of the Rust port. Ports store/index.ts as store/{paths,mod}.rs — the
foundation every source adapter writes through.
- paths.rs: resolve_ask_home (ASK_HOME tilde-expand → ~/.ask), npm/github/web/
llms-txt store paths, assert_contained (lexical_clean-based containment) +
assert_safe_segment traversal guards, normalize_url (scheme+host lowercase,
path case-preserved). Legacy github/db + github/checkouts paths intentionally
omitted per CLAUDE.md.
- mod.rs: write_entry_atomic + cp_dir_atomic (temp-dir + backup-rename swap;
cp preserves symlinks VERBATIM via read_link+symlink — the gitbutler broken-
symlink gotcha), acquire_entry_lock (create_new lock file + backoff, RAII
EntryLock, hit-detection returns None), stamp/verify_entry + hash_dir (sha256
over sorted `<rel>\0<bytes>\0`, skips .ask-hash), STORE_VERSION r/w,
quarantine_entry.
walkdir promoted to a normal dep; tempfile stays dev-only (prod temp names are
hand-rolled). 13 new tests incl. a unix verbatim-symlink test. 126 total green.
…lect
Phase 3 of the Rust port. Ports sources/{github,npm,llms-txt}.ts as
sources/{mod,github,npm,llms_txt}.rs (web deferred — loose html→md parity).
- mod.rs: DocFile/FetchResult/FetchMeta value types; is_doc_file, collect_doc_
files (md/mdx/txt/rst, sorted), detect_docs_path, extract_docs_from_dir (realpath
containment). NPM_/GITHUB_DOC_CANDIDATES.
- github.rs: authenticated_clone_url (opensrc #66 host-exact token injection via
the url crate) + redact_token + ref_candidates dedup — the security-critical
pure helpers, fully unit-tested. git SHELL-OUT clone-at-tag with ref fallback
+ ls-remote tag probe, .git strip + commit-SHA capture, store-hit verify/
quarantine short-circuit, tar.gz fallback (flate2+tar, get_bytes w/ Bearer auth
for private repos). End-to-end tested against a local bare repo (offline).
- npm.rs: local-first try_local_read (node_modules, node-semver version_matches —
bare version = exact, NOT Cargo caret) + tarball path (npm view + get_bytes +
extract). Traversal + realpath guards.
- llms_txt.rs: fetch + filename derivation + store write.
HttpClient gains get_bytes(url, headers) → BytesResponse (50 MiB cap) for archive
downloads; MockClient gains with_bytes. New deps: url, tar, flate2; tempfile
promoted to a normal dep (RAII temp dirs for clone/extract). 28 new tests, 154
total green.
Phase 3 of the Rust port. Ports storage.ts as storage.rs. - save_docs: copy (write files + INDEX.md), link (symlink project→store docs subdir, PermissionDenied → copy fallback), ref (no local materialization, returns effective_store_path). remove_docs by version or all. - list_docs: joins ask.json ⨝ resolved.json — declared-but-uninstalled entries surface as version "unresolved"; in-place + intent-skills branches handled (intent skill enumeration deferred with agents-intent). Sorted by name. Reuses ask_json::StoreMode (added Default=Copy). 7 new tests, 161 total green.
Phase 3 of the Rust port. Ports skill.ts and agents.ts. - skill.rs: generate_skill writes .claude/skills/<name>-docs/SKILL.md (lazy-first template referencing ask src/docs/search, major-version pin hint). get_skill_dir + remove_skill. - agents.rs: generate_agents_md maintains the <!-- BEGIN:ask-docs-auto-generated --> block — replace-in-place when both markers present, append when absent, strip on empty libraries (removing the file if the block was all), warn + leave untouched on an unmatched marker. Ensures CLAUDE.md carries a @AGENTS.md reference (no dupe). 15 new tests, 169 total green.
Phase 3 of the Rust port. Ports install.ts (run_install) and ignore-files.ts. - install.rs: lazy-first run_install — reads ask.json, resolves each library's version (github from the spec string with leading-v strip; npm from the lockfile chain), generates SKILL.md per library + a combined AGENTS.md, and runs manage_ignore_files. Creates an empty ask.json when missing; strips the AGENTS.md block + ignore markers on a full install with no libraries. No docs downloaded (agents fetch lazily). RunInstallOptions.only_specs for scoped installs. - ignore_files.rs: write/remove nested configs inside .ask/docs/ (byte-exact .gitattributes/eslint/biome/markdownlint templates) + patch/unpatch root ignore files (.prettierignore/.gitignore/sonar/markdownlint) via the Hash marker block. Opt-in gate (ask.json OR .ask/skills[-lock]). 9 new tests, 178 total green.
Ports list/{model,aggregate,render}.ts + display/table.ts as list.rs, and wires
the first two commands to real logic in cli.rs.
- list.rs: build_list_model (list_docs → ListModel, detect_conflicts), format_list
(header + column table with U+2500 separator + conflicts/warnings), JSON via
serde (itemCount/kebab source, matching ListModelSchema). Intent-skills tree
deferred (no skills until agents-intent).
- cli.rs run(): `ask install` → run_install(cwd); `ask list [--json]` →
build_list_model + format_list / to_string_pretty. Remaining commands still
NotPorted.
End-to-end verified: `ask install` resolves npm:react from package.json and emits
AGENTS.md/SKILL.md/ignore configs; `ask list` + `--json` render. 4 new tests,
182 total green.
Runs the same command through the authoritative TypeScript build (packages/cli/dist/cli.js) and the Rust binary against identical fixtures, then byte-diffs the generated files (SKILL.md, AGENTS.md, nested ignore configs, patched root ignores). Self-authored Rust unit tests share a mental model with the code and cannot catch template drift; the TS output is an external oracle. Compares files only, never stdout (consola vs eprintln legitimately differ). Covers install across three fixtures: github-only, npm+github with lockfile resolution and root-ignore patching, and the empty-libraries block-strip path. All identical at time of commit.
Cargo.lock picks up the source-layer deps (flate2, tar, url, walkdir) added while porting sources/*. bun.lock syncs @pleaseai/ask 0.4.6->0.4.8 to match the release-please package.json bump (stale-lockfile drift).
Rust port of commands/ensure-checkout.ts — the resolver shared by ask src/docs/fetch/add. Splits @Version, parses the spec, dispatches to the github source directly (owner/repo/ref) or via an ecosystem resolver (npm/pypi/pub/maven), computes the PM-unified store path, and short- circuits on a cache hit; on miss it fetches through github::fetch unless no_fetch (which raises NoCacheError for the offline-first add path). Preserves the recorded regressions: skip_doc_extraction=true (gitbutler no-docs-folder crash), implicit-default-ref leaves BOTH tag and branch unset so the default-branch fallback (main -> vmain -> master) fires, and the actual-ref reconciliation so a winning fallbackRef / v-rescued ref keeps checkout_dir and ref in agreement. The fetch step is injected via a CheckoutFetcher trait (mirrors TS deps.fetcher) so every path is unit-tested offline: cache hit, bare-spec main default, no_fetch miss -> NoCacheError, npm registry resolution via MockClient, bare-name npm dispatch, and a miss that triggers the fetcher and reports its store_path. 6 new tests, 188 total.
- ask src <spec>: prints the cached source path (or a JSON SrcModel with --json, the stable csp handoff), lazy-fetching on miss unless --no-fetch. run_src returns the stdout string; the CLI wrapper prints it and maps any error (NoCacheError, resolver failure) to stderr + exit 1, matching the TS exit(1) contract rather than the generic exit-2 path. - ask fetch <spec...>: warms the cache without printing paths; per-spec failures report to stderr and the rest still run, exiting 1 if any failed. run_fetch returns a structured FetchReport (stdout/stderr/had_errors) so the streaming + exit behaviour is unit-testable offline. Fixed SrcArgs to a single spec + --no-fetch + --json (was a specs Vec). 9 new unit tests. Verified byte-identical to the TS build via differential runs against a shared ASK_HOME with a pre-warmed checkout: 'ask src --json' matches the pretty-printed JSON field-for-field, and 'ask fetch' matches the 'already cached' + summary output exactly. 203 tests total.
- find_doc_like_paths (find-doc-paths.ts): pre-order DFS returning every
/doc/i subdir up to depth 4, dist/docs probed first, falling back to [root]
for README-only trees; skips node_modules/.git/.next/.nuxt/dist/build/
coverage + dotdirs. Traversal order mirrors the TS readdirSync walker
exactly so the emitted path list is byte-identical.
- ask docs <spec> (docs.ts): ensure_checkout, then emit doc-like paths from
node_modules/<pkg> (npm specs only) + the cached checkout. A persisted
ask.json docsPaths override, when present and non-stale, restricts output to
those pins (resolved against BOTH roots, containment-guarded via
assert_contained, existing-file wins); all-stale falls through to the walk
with a stderr warning. run_docs returns DocsRun{stdout,warnings} so warnings
don't fail; NoCacheError/resolver errors map to exit 1 in the CLI wrapper.
DocsArgs gains --no-fetch/--json. 13 new tests. Verified byte-identical to the
TS build across --json, text mode, and the docsPaths-override case (walk order
matches on APFS since both call the same getdirentries). 207 tests total.
…nces - ask remove <name> (inline removeCmd in index.ts): match by exact spec → library slug → npm package name (same findIndex rules), splice ask.json, delete the skill, re-run the lazy install to regen AGENTS.md, then re-sync the ignore markers (remove-mode when the list is now empty, else install-mode). run_remove returns a RemoveOutcome enum (NoAskJson / NoMatch / Removed); the CLI maps each to a consola-equivalent line, all exit 0. NOTE: this branch's removeCmd does NOT branch on entry format — intent-skills handling is not present here (intent is deferred), so the port stays faithful to the docs/skill-only teardown. - parity-diff.sh: run_case now runs a SEQUENCE of commands in order, adding install-then-remove cases (remove react from npm+github; remove next.js from github-only) that exercise skill teardown + AGENTS.md regen. All 5 cases byte-identical to the TS build. 5 new tests, 212 total.
- resolve_csp (resolve-csp.ts): CSP_BIN override (no probe) → PATH scan honouring
PATHEXT on Windows (dropping .cmd/.bat/.ps1 shims ask can't spawn) and the exec
bit on POSIX → None. Injectable CspEnv for offline tests.
- ask search <spec> <query> (search.ts): ensure_checkout to a version-pinned
checkout, then delegate to csp. csp is OPTIONAL — when absent, print the
checkout path + a shell-quoted copy-paste recipe and exit 0 (never fail solely
because csp is missing). When present, spawn with inherited stdio and forward
the exit code (a signal-killed csp maps to 128+signum, never a bogus 0).
Pure helpers ported with exact vectors: csp_exit_code, shell_quote (POSIX
single-quote with '\'' escaping), build_csp_args (path positional after query,
one --content per value, --top-k). run_search returns SearchReport
{stdout,stderr,exit_code}; the CLI wrapper parses --content csv + --top-k
(warning on non-numeric) and process::exit(exit_code).
12 new tests, 224 total. Verified end-to-end against the TS build: with CSP_BIN
pointed at a fake csp, BOTH builds spawn it with byte-identical argv (search,
query, checkoutDir, --content docs, --content code, --top-k 7) and forward its
exit code 42.
Deploying ask-registry with
|
| Latest commit: |
1230770
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://898a4baa.ask-registry.pages.dev |
| Branch Preview URL: | https://amondnet-rust.ask-registry.pages.dev |
SonarCloud (S6362): move the workflow-level 'permissions: contents: read' to the job that needs it in release-rust.yml (build) and release-cargo.yml (publish). release-rust's upload-release-assets job already declares its own 'contents: write'. Least-privilege per-job permissions.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: This large port from TypeScript to Rust introduces high risk to build, release, and distribution and requires human review for infrastructure, security, and architecture.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
20 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/ask/src/sources/llms_txt.rs">
<violation number="1" location="crates/ask/src/sources/llms_txt.rs:62">
P2: llms-txt fetch results currently lose source-origin metadata because `meta` is left at default. Populating `FetchMeta.urls` with the fetched URL would keep llms-txt behavior aligned with the shared `FetchMeta` contract and preserve traceability in downstream consumers.</violation>
</file>
<file name="crates/ask/src/resolvers/pypi.rs">
<violation number="1" location="crates/ask/src/resolvers/pypi.rs:63">
P2: Package resolution can incorrectly trust non-GitHub URLs that merely contain `github.com`, causing docs to be fetched from an unrelated repo. Consider validating the URL host is exactly `github.com` (for both `project_urls` and `home_page`) before accepting it.</violation>
</file>
<file name="crates/ask/src/skills/vendor.rs">
<violation number="1" location="crates/ask/src/skills/vendor.rs:72">
P2: Reinstall can drop an already-installed skill set when the final rename step fails, because the current `vendor_dir` is deleted before the staged directory is moved into place. Consider a backup/swap sequence so the previous directory is recoverable until the new directory is successfully committed.</violation>
</file>
<file name="crates/ask/src/ignore_files.rs">
<violation number="1" location="crates/ask/src/ignore_files.rs:195">
P2: Removing the last docs library can unintentionally stop ignoring vendored skills files, because Remove mode strips the shared root marker block that also contains `.ask/skills/` paths. Consider keeping root ignores installed when `.ask/skills` or `skills-lock.json` still exists, so lint/format tooling stays aligned with remaining skills artifacts.</violation>
</file>
<file name="crates/ask/src/commands/skills.rs">
<violation number="1" location="crates/ask/src/commands/skills.rs:260">
P2: A symlink conflict during install can leave partial artifacts that are no longer tracked. The early return inside the link loop exits before lock persistence, so successful earlier links/vendor copies become orphaned; consider rollback of created links/vendor dir (or deferred commit) on failure.</violation>
</file>
<file name="crates/ask/src/sources/github.rs">
<violation number="1" location="crates/ask/src/sources/github.rs:309">
P2: A v-prefixed tag request can miss cache on subsequent runs when the repo only has bare tags: first fetch succeeds via probe and stores under the bare tag, but store-hit lookup only checks the original v-prefixed key. Adding a stripped-tag candidate for tag-based cache lookup would keep `@v...` specs cacheable after fallback resolution.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Greptile SummaryThis PR ports the
Confidence Score: 4/5Safe to merge with one coordination gap in the release pipeline worth addressing before or shortly after the first live release run. The Rust port is thorough: atomic writes, symlink safety hardening, platform-aware GC scan roots, and byte-parity with the TS oracle all land cleanly. The npm shim is well-structured with correct idempotency, signal forwarding, and fallback behaviour. The one notable gap is that cargo-publish runs independently of build-binaries, so a failure in all binary builds can leave ask-please on crates.io without corresponding npm platform packages. .github/workflows/release.yml — the cargo-publish job dependency ordering relative to build-binaries. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
RP[release-please\ncli_release_created] --> BB[build-binaries\nrelease-rust.yml\n6-target matrix]
RP --> CP[cargo-publish\nrelease-cargo.yml\ncrates.io]
BB --> NP[npm-publish\ndownload + check all 6 binaries\ngenerate platform pkgs\npublish wrapper]
BB --> HB[update-homebrew-formula]
NP -->|platform pkgs first| PkgDarwinArm["@pleaseai/ask-darwin-arm64"]
NP -->|platform pkgs first| PkgLinux["@pleaseai/ask-linux-x64 …"]
NP -->|wrapper last| Wrapper["@pleaseai/ask\n(optionalDependencies pinned)"]
CP -->|independent| CratesIO["crates.io\nask-please@X.Y.Z"]
style CP fill:#ffe0b2
style CratesIO fill:#ffe0b2
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
RP[release-please\ncli_release_created] --> BB[build-binaries\nrelease-rust.yml\n6-target matrix]
RP --> CP[cargo-publish\nrelease-cargo.yml\ncrates.io]
BB --> NP[npm-publish\ndownload + check all 6 binaries\ngenerate platform pkgs\npublish wrapper]
BB --> HB[update-homebrew-formula]
NP -->|platform pkgs first| PkgDarwinArm["@pleaseai/ask-darwin-arm64"]
NP -->|platform pkgs first| PkgLinux["@pleaseai/ask-linux-x64 …"]
NP -->|wrapper last| Wrapper["@pleaseai/ask\n(optionalDependencies pinned)"]
CP -->|independent| CratesIO["crates.io\nask-please@X.Y.Z"]
style CP fill:#ffe0b2
style CratesIO fill:#ffe0b2
Reviews (7): Last reviewed commit: "docs(skill): drop brittle hard-coded tes..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Address the cubic AI review on PR #119. 14 findings fixed; 6 confirmed pre-existing-in-TS and deferred (see PR follow-up). Windows portability (cfg-gated; verified only by the release-rust CI matrix): - skills/symlinks.rs: remove directory symlinks with remove_dir on Windows (remove_file only removes file symlinks) via a shared remove_symlink helper — fixes skills relink/unlink leaving links behind on Windows. - store/mod.rs: resolve a relative symlink target against the source link's parent before the non-unix copy fallback (was copying the raw relative target against CWD). - cli.rs: parse ASK_GC_SCAN_ROOTS with std::env::split_paths (OS delimiter) so a Windows drive-letter root is not split on its ':' (unix behaviour unchanged). Security hardening (parity-orthogonal; fixtures never exercise these): - sources/npm.rs: apply the try_local_read lexical+realpath containment guard to the tarball --docs-path branch (could read arbitrary local paths). - sources/mod.rs: skip doc files whose symlink target escapes the docs root (per-file guard; the root-only guard missed symlinked files) + regression test. Schema/behaviour parity with the TS oracle (TS stricter → fix restores parity): - resolved.rs: reject empty spec/resolvedVersion (TS z.string().min(1)) + test. - ask_json.rs: reject JS line terminators in a spec payload (TS regex .+$) + test. - commands/docs.rs: propagate an invalid ask.json instead of masking it. - io.rs: read_resolved_json surfaces non-NotFound read errors instead of masking them as an empty cache (matches TS readFileSync outside the parse try/catch); returns Result — callers updated. - resolvers/maven.rs: accept latestVersion (default core) as well as v (core=gav) so a healthy Search API resolves 'latest' instead of falling through + test. - store/paths.rs: split the URL authority at the first of / ? # so a query/ fragment without a path is not lowercased into the store key (TS normalizeUrl preserves path/query/fragment case). Test harness: - scripts/parity-diff.sh: compare TS/Rust exit codes (a blanket '|| true' let the harness false-pass when both sides errored). Exempt the skills_install case with a documented note — the TS skills parent double-dispatches (subcommand + list shorthand → spurious exit 1); Rust dispatches install only, files are identical. CI publish safety: - release.yml: refuse to publish the @pleaseai/ask wrapper unless all 6 platform binaries are present — a partial build would ship a wrapper with missing optionalDependencies (npm does not error) and burn the version. 283 unit tests, clippy/fmt clean, actionlint clean, all differential parity cases byte-identical.
Contributor
There was a problem hiding this comment.
1 issue found across 14 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Follow-up to the relative-symlink resolution in the prior commit (cubic P1 on 73ae9a9). The non-unix fallback dereferences a symlink target and copies its bytes into the store entry; without a containment check a target that resolves outside the copy root (../…, absolute, or an escaping chain) would pull unrelated local files into the cached entry. Thread the copy root through copy_tree_verbatim and canonicalize the resolved target against it — skip the entry (copy nothing) when it escapes or cannot be resolved. Unix is unchanged: it records the link verbatim and never dereferences the target.
Contributor
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Adds a massive Rust codebase (17K+ lines) as a new crate with full CLI implementation, multiple CI/CD workflows, npm distribution changes, and a Homebrew release pipeline. This is a business-critical port affecting all deployment channels; requires human review for correctness and security.
Re-trigger cubic
greptile P1: `ask cache gc` defaulted its scan root to `std::env::var("HOME")`,
which is unset on native Windows (Windows uses USERPROFILE). An empty scan root
makes collect_referenced_store_paths return an empty set, so every entry looks
unreferenced and a non-dry-run gc deletes the entire store — including in-use
entries. Use the existing platform-aware store::paths::home_dir() (USERPROFILE on
Windows, $HOME on unix), matching TS os.homedir(). home_dir is now pub(crate).
Also document why release-rust.yml uses `macos-15-intel` (a second greptile P1
claimed it was invalid, but actionlint's runner-label DB confirms macos-15-intel
is the current Intel label and macos-13 has been retired — the original label was
correct; comment added to prevent a regression to the retired macos-13).
Contributor
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: Massive Rust port (17k+ lines) rewriting entire CLI from TypeScript to Rust with new build/release workflows, CI/CD changes, and distribution mechanisms. Touches core business logic, infrastructure, and deployment pipelines — requires human review despite passing automated checks.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (14)
.github/workflows/release-cargo.yml (2)
42-43: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider disabling credential persistence on checkout.
Static analysis flags default credential persistence on
actions/checkout. Job permission is scoped tocontents: readhere, so blast radius is limited, but hardening is cheap.🔒 Suggested fix
- name: Checkout code uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-cargo.yml around lines 42 - 43, The Checkout step in the release workflow should disable persisted credentials. Update the actions/checkout usage in the release job to explicitly turn off credential persistence, since the job only needs read access and should not leave GitHub credentials on disk.Source: Linters/SAST tools
45-70: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider migrating to crates.io Trusted Publishing to drop the long-lived
CARGO_REGISTRY_TOKEN.crates.io now supports OIDC-based Trusted Publishing via
rust-lang/crates-io-auth-action, eliminating the need to store a long-lived registry token in secrets. Static analysis also flags this. Requires a one-time Trusted Publisher registration on crates.io pluspermissions: id-token: write.Since I flagged an external tool/API capability, please verify current setup requirements before adopting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-cargo.yml around lines 45 - 70, Migrate the release workflow to crates.io Trusted Publishing instead of using the long-lived CARGO_REGISTRY_TOKEN secret. Update the publish step in the release job to use rust-lang/crates-io-auth-action with the required OIDC permissions, and remove the secret-based token wiring from the publish environment. Keep the existing ask-please packaging and idempotent publish logic in place, but ensure the cargo publish invocation is authenticated through Trusted Publishing after the one-time crates.io registration.Source: Linters/SAST tools
.github/workflows/release-rust.yml (1)
64-68: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSame persist-credentials hardening as release-cargo.yml.
Job permission is
contents: readonly, so risk is limited, but consider settingpersist-credentials: falsefor consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-rust.yml around lines 64 - 68, The Checkout step in the release-rust workflow should be hardened the same way as release-cargo by disabling credential persistence. Update the actions/checkout usage in the checkout step to set persist-credentials to false so the job does not leave GitHub credentials available after checkout.Source: Linters/SAST tools
.github/workflows/release.yml (1)
39-56: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
npm-publishandupdate-homebrew-formuladon't gate onbuild-binariessuccess.Both jobs
needs: [..., build-binaries]but theirif:conditions never checkneeds.build-binaries.result.npm-publish's Rust-binary step is self-protected by its explicit missing-binary check (good), butupdate-homebrew-formula(line 231) has no equivalent guard — ifbuild-binariesfails entirely, that job will still run and fail with a rawawk/missing-file error when reading checksums, rather than a clear message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 39 - 56, The release workflow jobs that depend on build-binaries are not properly gated on its success. Update the if conditions for npm-publish and update-homebrew-formula in release.yml to explicitly require needs.build-binaries.result == 'success' (in addition to the existing release-created checks), and make sure update-homebrew-formula follows the same pattern as npm-publish so it won’t try to read missing checksum files when build-binaries fails.crates/ask/src/resolvers/maven.rs (1)
165-218: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDuplicate identical HTTP request for
latestresolution.When
version == "latest",fetch_search_api's own query URL (line 177) is byte-identical tofetch_scm_url's query URL (line 230), yetfetch_scm_urlis called unconditionally (line 212), issuing a second request to the same Maven Search API endpoint for data already present in the first response. Mergingscm.urlintoMavenSearchDocavoids the redundant round-trip for the common "latest" case.♻️ Proposed fix
#[derive(Deserialize)] struct MavenSearchDoc { // `core=gav` responses (explicit version) carry `v`; the default core (used // for `latest`) carries `latestVersion` and has no `v`. Accept both so a // healthy Search API can resolve `latest` instead of erroring on the missing // `v` field and always falling through to maven-metadata.xml. v: Option<String>, #[serde(rename = "latestVersion")] latest_version: Option<String>, + #[serde(rename = "scm.url")] + scm_url: Option<String>, } @@ - // Best-effort scm.url from the artifact-level (non-GAV) core. - let scm_url = fetch_scm_url(client, group_id, artifact_id).ok().flatten(); + // For `latest`, scm.url is already present on this same response; only the + // GAV (explicit-version) query lacks it and needs the extra lookup. + let scm_url = if is_latest { + first_doc_scm_url + } else { + fetch_scm_url(client, group_id, artifact_id).ok().flatten() + };Also applies to: 220-242
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ask/src/resolvers/maven.rs` around lines 165 - 218, `fetch_search_api` is making a redundant Maven Search API call in the common `version == "latest"` path because it always invokes `fetch_scm_url` even though the first search response already contains the needed data. Update `fetch_search_api` and `MavenSearchDoc` so `scm.url` is parsed from the initial response and reused when resolving latest versions, and only fall back to `fetch_scm_url` for non-latest cases if necessary.crates/ask/src/sources/npm.rs (1)
158-169: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoffOptional:
dist.integrityis captured but never verified against the downloaded tarball.The SRI hash is fetched and propagated into
meta.integrity, yet the downloaded bytes are unpacked without checking them against it. Verifying the SHA-512 SRI before extraction would close a tamper/MITM gap. Note this changes behavior relative to the TS oracle, so confirm it won't break differential parity before adopting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ask/src/sources/npm.rs` around lines 158 - 169, The npm tarball download flow in the source that builds `integrity`, fetches `response`, and then unpacks with `GzDecoder`/`tar::Archive` currently ignores the captured `dist.integrity`. Update this path to verify the downloaded bytes against the SRI hash before extraction, failing fast on mismatch and only proceeding to unpack when the checksum matches. Keep the change localized around the `integrity` handling and tarball extraction logic, and make sure the verification is applied before `unpack(tmp.path())` in the npm source loader.crates/ask/src/commands/find_doc_paths.rs (1)
10-24: 🚀 Performance & Scalability | 🔵 TrivialDuplicated
SKIP_DIRS/walk scaffolding withfind_skill_paths.rs.The skip-list,
MAX_DEPTH, and recursivewalkfunction are copy-pasted (with only the predicate and root-inclusion behavior differing) infind_skill_paths.rs. Consider extracting a shared generic walker (parameterized by predicate fn and "include root" flag) to avoid the two implementations drifting apart.♻️ Sketch of a shared walker
fn walk_matching( root: &Path, max_depth: usize, skip_dirs: &[&str], is_match: impl Fn(&str) -> bool, out: &mut Vec<PathBuf>, ) { // shared recursive body used by both find_doc_like_paths and find_skill_like_paths }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ask/src/commands/find_doc_paths.rs` around lines 10 - 24, The directory-walking logic in find_doc_paths is duplicated with find_skill_paths, including the skip list and MAX_DEPTH scaffolding, so refactor the shared recursion into a generic walker. Extract a reusable function (for example, a shared walk_matching helper) that takes the root, max depth, skip directories, a predicate callback, and an include-root flag, then update find_doc_like_paths and the corresponding skill-path function to call it with their specific matching rules.crates/ask/src/commands/add.rs (1)
541-546: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
placeholderargument is never surfaced to the user.
default_textignores_placeholderentirely, so the example text ("npm:lodash, github:owner/repo@v1") passed at lines 541-546 and 559-562 is never shown during the real interactive prompt — users get no hint about accepted spec formats.dialoguer::Inputhas no built-in ghost-text placeholder (onlydefault()/initial_text(), which pre-fill an editable value rather than showing a hint), so embedding the example directly into the prompt message is the simplest fix.♻️ Proposed fix
-fn default_text(msg: &str, _placeholder: &str) -> String { +fn default_text(msg: &str, placeholder: &str) -> String { use dialoguer::Input; Input::<String>::new() - .with_prompt(msg) + .with_prompt(format!("{msg} (e.g. {placeholder})")) .allow_empty(true) .interact_text() .unwrap_or_default() }Please confirm dialoguer's current API doesn't offer a more idiomatic hint-text mechanism before applying.
Also applies to: 639-647
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ask/src/commands/add.rs` around lines 541 - 546, The interactive spec prompt in add.rs is passing a placeholder value that never gets displayed, so update the prompt flow in the manual input path (the text/default_text helper used by the specs prompt) to surface the example format directly in the visible message instead of relying on placeholder text. Verify against dialoguer::Input’s current API that there is no built-in hint/ghost-text mechanism beyond default()/initial_text(), and if none exists, bake the example into the prompt text consistently in both affected spec-entry locations.crates/ask/src/resolved.rs (1)
173-216: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider using
time's RFC3339 parser instead of hand-rolled shape validation.The crate already enables
time's"parsing"feature (workspaceCargo.toml, Line 30) but this validator re-implements RFC3339 shape checking by hand, and — per its own comment — doesn't catch invalid calendar values (month 13, Feb 30, etc.). Swapping toOffsetDateTime::parse(s, &time::format_description::well_known::Rfc3339)would remove ~40 lines of bespoke parsing and validate real calendar correctness for free.Please confirm
Rfc3339::parseaccepts the same offset/fractional-second shapes exercised bydatetime_offset_shapes(Lines 322-329) before swapping, since RFC3339 parser strictness can differ subtly from a custom checker.♻️ Proposed refactor sketch
+use time::format_description::well_known::Rfc3339; +use time::OffsetDateTime; + fn is_iso_datetime_offset(s: &str) -> bool { - let Some((date, rest)) = s.split_once('T') else { - return false; - }; - ... (hand-rolled shape check) ... + OffsetDateTime::parse(s, &Rfc3339).is_ok() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ask/src/resolved.rs` around lines 173 - 216, Replace the hand-rolled RFC3339 shape checker in is_iso_datetime_offset with time’s parser-backed validation. Use OffsetDateTime::parse with time::format_description::well_known::Rfc3339 so the validator enforces real calendar correctness instead of only string shape, and confirm it still matches the offset/fractional-second cases covered by datetime_offset_shapes before removing the custom logic. Keep the is_iso_datetime_offset entry point and its callers unchanged, but simplify its implementation to delegate parsing and return false on parse errors.crates/ask/src/lib.rs (1)
1-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale "Phase 0 skeleton" doc comment.
This describes the crate as a walking skeleton with commands ported "module by module in later phases," but cli.rs in this same cohort states every command is already backed by its ported implementation, consistent with the PR's full command-surface port. Worth updating so new contributors aren't misled about the crate's maturity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ask/src/lib.rs` around lines 1 - 8, Update the stale crate-level doc comment in the `ask` crate so it matches the current state of `cli` and the ported command implementations. Remove the “Phase 0 walking skeleton” and “not yet ported” wording, and describe that the command surface is now backed by the Rust port. Keep the summary aligned with the current `cli` module and the actual maturity of the crate so contributors aren’t misled.npm/ask/install.js (1)
20-92: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider wrapping
main()in a top-level guard to enforce the stated "MUST exit 0" contract.The header comment states this step is best-effort and must never fail the install, and today that holds because
resolveBinaryPath()internally swallows its own errors. Butmain()itself has no top-level try/catch, so if that invariant is ever violated by a future change tolib/resolve.js(e.g., a thrown error that isn't arequire.resolvefailure), this script would exit non-zero and breaknpm install/npm cifor every consumer.🛡️ Defensive wrapper
-main() +try { + main() +} +catch { + // Best-effort step; never fail the install. +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@npm/ask/install.js` around lines 20 - 92, Wrap the top-level main() entry point in a guard that guarantees the install step always exits successfully, even if a future change in resolveBinaryPath() or lib/resolve.js starts throwing unexpectedly. Keep the existing best-effort behavior in main(), but add a top-level try/catch around the call site so any uncaught error is swallowed and the npm install contract remains exit 0.npm/ask/lib/resolve.js (1)
20-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce cognitive complexity of
resolvePlatformPackage.Static analysis flags this function at complexity 19 (limit 15) due to nested platform/arch/musl branching. Consider flattening via a lookup table keyed by
platform:arch, with musl handled as a post-lookup override for the two Linux cases:♻️ Suggested restructuring
+const TARGET_TABLE = { + 'win32:x64': { pkg: '`@pleaseai/ask-win32-x64`', binary: 'ask.exe' }, + 'darwin:arm64': { pkg: '`@pleaseai/ask-darwin-arm64`', binary: 'ask' }, + 'darwin:x64': { pkg: '`@pleaseai/ask-darwin-x64`', binary: 'ask' }, + 'linux:x64': { pkg: '`@pleaseai/ask-linux-x64`', binary: 'ask' }, + 'linux:x64:musl': { pkg: '`@pleaseai/ask-linux-x64-musl`', binary: 'ask' }, + 'linux:arm64': { pkg: '`@pleaseai/ask-linux-arm64`', binary: 'ask' }, +} + function resolvePlatformPackage() { const { platform, arch } = process - if (platform === 'win32') { - ... - } - ... + if (platform === 'linux' && arch === 'x64' && isMusl()) { + return TARGET_TABLE['linux:x64:musl'] + } + if (platform === 'linux' && arch === 'arm64' && isMusl()) { + return null // no musl arm64 build exists + } + return TARGET_TABLE[`${platform}:${arch}`] ?? null }This also makes adding future targets a data change rather than a branch addition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@npm/ask/lib/resolve.js` around lines 20 - 52, The `resolvePlatformPackage` function is too branch-heavy and exceeds the cognitive complexity limit; flatten its platform/arch logic by replacing the nested `if` chain with a lookup table keyed by platform and architecture, and keep `isMusl()` as a small post-lookup override for the Linux x64 and arm64 cases. Preserve the current return values and the null fallback behavior, and keep the function name `resolvePlatformPackage` so the refactor is localized and easier to extend with new targets.Source: Linters/SAST tools
crates/ask/src/agents.rs (1)
6-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate marker inject/strip logic vs
crate::markers.This file hand-rolls its own begin/end marker search, replace, and strip logic, which largely duplicates
crate::markers::{inject, remove}(used byignore_files.rs). The two implementations have subtly different blank-line trimming semantics (trim_end_matches('\n')here vstrim_end()inmarkers.rs), which is a maintenance risk if one is updated without the other. Consider generalizingMarkerSyntax(or adding a custom-marker variant) soagents.rscan reuse the shared module instead of maintaining a parallel implementation.Also applies to: 42-124
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ask/src/agents.rs` around lines 6 - 7, `agents.rs` is reimplementing marker injection/removal logic that already exists in `crate::markers`, creating duplicated behavior and inconsistent trimming semantics. Update the `markers_for_*` / begin-end marker handling in `agents.rs` to reuse `crate::markers::{inject, remove}` (or extend `MarkerSyntax` to support this custom marker pair) instead of maintaining a separate search/replace/strip path. Keep the `BEGIN_MARKER` and `END_MARKER` symbols, but route all marker edits through the shared module so blank-line trimming and future fixes stay consistent.crates/ask/src/skills/vendor.rs (1)
72-75: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow non-atomic window between removing old vendor dir and renaming staging in.
If the process is killed between
remove_dir_all(&vendor_dir)andrename(&staging, &vendor_dir), the vendor directory is left absent (recoverable only by re-running install). This is a small deviation from the "atomic replace" guarantee described in the module doc comment.♻️ Proposed fix: swap-rename to shrink the unsafe window
- if vendor_dir.exists() { - std::fs::remove_dir_all(&vendor_dir)?; - } - std::fs::rename(&staging, &vendor_dir)?; + let backup = root.join(format!(".{spec_key}.old")); + if backup.exists() { + std::fs::remove_dir_all(&backup)?; + } + if vendor_dir.exists() { + std::fs::rename(&vendor_dir, &backup)?; + } + std::fs::rename(&staging, &vendor_dir)?; + if backup.exists() { + std::fs::remove_dir_all(&backup)?; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ask/src/skills/vendor.rs` around lines 72 - 75, The replace flow in vendor.rs is not fully atomic because VendorDir removal happens before the staging rename, leaving a gap where the directory can disappear if the process dies. Update the install/replace logic around vendor_dir, staging, and the rename step to use a swap-rename approach (or equivalent temp/backup rename sequence) so the old directory is only removed after the new one is safely in place. Keep the change localized to the vendor install path that currently calls remove_dir_all and rename.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 143-160: The publish step in the release workflow is interpolating
the release tag directly into the shell script via the TAG assignment, which
should be moved to an env-based input instead. Update the “Publish `@pleaseai/ask`
(Rust binary shim)” step to pass the tag through env alongside NODE_AUTH_TOKEN
and GH_TOKEN, then read it from the shell using TAG; keep the existing
fallback/version derivation logic in place and align this step with the safer
pattern already used by the workflow’s update-homebrew-formula job.
In `@crates/ask/src/agents.rs`:
- Around line 20-88: Filesystem updates in generate_agents_md, strip_block, and
update_claude_md are silently ignoring write/remove failures, so install can
succeed even when AGENTS.md or CLAUDE.md was not updated. Change these helpers
to return anyhow::Result values instead of discarding
std::fs::write/read_to_string/remove_file results, and propagate errors with ?
through generate_agents_md and update_claude_md. Then bubble the error up to
run_install so the caller can report the failure instead of always printing
success.
- Around line 65-84: The generated block update logic in agents.rs handles
malformed marker states inconsistently with strip_block: when only one of
BEGIN_MARKER or END_MARKER exists, it currently falls through the write path and
appends a new block on top of corrupted content. Update the generation flow to
detect the single-marker case in the same way as strip_block, warn the user, and
skip rewriting the file instead of adding another block. Keep the behavior
aligned with the existing marker-handling logic in the agents update routine and
strip_block helper.
In `@crates/ask/src/cli.rs`:
- Around line 236-251: The command dispatch in cli::run should match the TS CLI
failure exit behavior instead of bubbling errors to main() for command failures.
Update the match on Command (especially Install, Remove, and List when used for
JSON output) so these paths convert command errors into the same nonzero exit
code as the TS command runner, using the existing run_install, run_remove_cmd,
and run_list entry points as the places to handle the failure consistently.
Ensure the exit code returned for command errors aligns with the TS oracle
rather than relying on main()'s default propagation.
In `@crates/ask/src/commands/add.rs`:
- Around line 91-107: normalize_add_spec is incorrectly treating scoped npm
package names as GitHub owner/repo specs; update the spec check in is_owner_repo
and normalize_add_spec so inputs starting with '@' are not normalized to
github:. Follow the same guard used by normalize_add_spec_interactive and keep
the ambiguous-spec path aligned with AMBIGUOUS_SPEC_HINT so `@mastra/client-js`
returns the documented ambiguity error instead of a GitHub-prefixed spec.
In `@crates/ask/src/commands/ensure_checkout.rs`:
- Around line 123-140: The GitHub spec handling in ensure_checkout has a dead
branch because is_from_branch is only set when explicit_version is none, so
explicit github:owner/repo@ref inputs never reach github::fetch as branches.
Update the ParsedSpec::Github match logic in ensure_checkout to detect actual
branch refs for explicit_version instead of tying it to the implicit-default
case, and make sure the branch-vs-tag decision uses a real ref classification so
the else if is_from_branch path can be taken when appropriate.
In `@crates/ask/src/commands/skills.rs`:
- Around line 305-345: collect_skill_dirs is deduping by full PathBuf, so the
same skill basename can be collected twice from node_modules and checkout_dir
and then propagate into vendor.skill_names and duplicate LockSkill entries.
Change collect_skill_dirs to dedup on the skill directory basename (or another
stable skill identifier) instead of the full path, while still preserving the
first/last winner behavior expected by vendor_skills and lock handling. Use the
existing collect_skill_dirs and vendor_skills symbols to keep the fix aligned
with the current install flow and the installed-count summary.
In `@crates/ask/src/io.rs`:
- Around line 133-141: `write_ask_json` and `write_resolved_json` currently
write directly to the destination path with `std::fs::write`, which can leave
`ask.json` or `resolved.json` truncated if the process crashes mid-write. Update
both functions to write through a temporary file in the same directory and then
atomically rename it into place, reusing the existing `get_ask_json_path`,
`get_resolved_json_path`, and JSON serialization helpers. Keep the parent
directory creation, and ensure the temp-file/rename flow is applied consistently
in both `write_ask_json` and `write_resolved_json` so the writes are crash-safe.
In `@crates/ask/src/skills/symlinks.rs`:
- Around line 43-52: The create_symlink helper currently forwards the raw
Windows symlink failure from std::os::windows::fs::symlink_dir, which can be
confusing for users without Admin rights or Developer Mode. Update
create_symlink (and any run_skills_install path that surfaces its Result) to
detect the Windows permission failure and return or log a clearer message that
explains the symlink prerequisite, or provide a fallback path when symlink
creation is not permitted.
In `@crates/ask/src/skills/vendor.rs`:
- Around line 24-39: The recursive copier in copy_dir_recursive mishandles
directory symlinks because entry.file_type() reports the symlink itself, so
symlinks to directories fall into the file path and std::fs::copy fails. Update
the logic in copy_dir_recursive to detect symlink targets by resolving metadata
or otherwise checking the linked path, then recurse when the target is a
directory and only use std::fs::copy for regular files/symlinked files, keeping
the existing dereference behavior consistent with the comment.
In `@crates/ask/src/sources/llms_txt.rs`:
- Around line 66-76: The url_last_segment helper is dropping trailing empty path
segments, which changes how URLs ending in “/” are resolved. Update the logic in
url_last_segment to preserve the final empty segment from Url::path_segments()
and match the TS pathname.split('/').pop() || 'llms.txt' behavior, so paths like
a trailing slash fall back correctly instead of returning the prior segment.
Keep the fix localized to url_last_segment and its use of
path_segments()/rfind().
---
Nitpick comments:
In @.github/workflows/release-cargo.yml:
- Around line 42-43: The Checkout step in the release workflow should disable
persisted credentials. Update the actions/checkout usage in the release job to
explicitly turn off credential persistence, since the job only needs read access
and should not leave GitHub credentials on disk.
- Around line 45-70: Migrate the release workflow to crates.io Trusted
Publishing instead of using the long-lived CARGO_REGISTRY_TOKEN secret. Update
the publish step in the release job to use rust-lang/crates-io-auth-action with
the required OIDC permissions, and remove the secret-based token wiring from the
publish environment. Keep the existing ask-please packaging and idempotent
publish logic in place, but ensure the cargo publish invocation is authenticated
through Trusted Publishing after the one-time crates.io registration.
In @.github/workflows/release-rust.yml:
- Around line 64-68: The Checkout step in the release-rust workflow should be
hardened the same way as release-cargo by disabling credential persistence.
Update the actions/checkout usage in the checkout step to set
persist-credentials to false so the job does not leave GitHub credentials
available after checkout.
In @.github/workflows/release.yml:
- Around line 39-56: The release workflow jobs that depend on build-binaries are
not properly gated on its success. Update the if conditions for npm-publish and
update-homebrew-formula in release.yml to explicitly require
needs.build-binaries.result == 'success' (in addition to the existing
release-created checks), and make sure update-homebrew-formula follows the same
pattern as npm-publish so it won’t try to read missing checksum files when
build-binaries fails.
In `@crates/ask/src/agents.rs`:
- Around line 6-7: `agents.rs` is reimplementing marker injection/removal logic
that already exists in `crate::markers`, creating duplicated behavior and
inconsistent trimming semantics. Update the `markers_for_*` / begin-end marker
handling in `agents.rs` to reuse `crate::markers::{inject, remove}` (or extend
`MarkerSyntax` to support this custom marker pair) instead of maintaining a
separate search/replace/strip path. Keep the `BEGIN_MARKER` and `END_MARKER`
symbols, but route all marker edits through the shared module so blank-line
trimming and future fixes stay consistent.
In `@crates/ask/src/commands/add.rs`:
- Around line 541-546: The interactive spec prompt in add.rs is passing a
placeholder value that never gets displayed, so update the prompt flow in the
manual input path (the text/default_text helper used by the specs prompt) to
surface the example format directly in the visible message instead of relying on
placeholder text. Verify against dialoguer::Input’s current API that there is no
built-in hint/ghost-text mechanism beyond default()/initial_text(), and if none
exists, bake the example into the prompt text consistently in both affected
spec-entry locations.
In `@crates/ask/src/commands/find_doc_paths.rs`:
- Around line 10-24: The directory-walking logic in find_doc_paths is duplicated
with find_skill_paths, including the skip list and MAX_DEPTH scaffolding, so
refactor the shared recursion into a generic walker. Extract a reusable function
(for example, a shared walk_matching helper) that takes the root, max depth,
skip directories, a predicate callback, and an include-root flag, then update
find_doc_like_paths and the corresponding skill-path function to call it with
their specific matching rules.
In `@crates/ask/src/lib.rs`:
- Around line 1-8: Update the stale crate-level doc comment in the `ask` crate
so it matches the current state of `cli` and the ported command implementations.
Remove the “Phase 0 walking skeleton” and “not yet ported” wording, and describe
that the command surface is now backed by the Rust port. Keep the summary
aligned with the current `cli` module and the actual maturity of the crate so
contributors aren’t misled.
In `@crates/ask/src/resolved.rs`:
- Around line 173-216: Replace the hand-rolled RFC3339 shape checker in
is_iso_datetime_offset with time’s parser-backed validation. Use
OffsetDateTime::parse with time::format_description::well_known::Rfc3339 so the
validator enforces real calendar correctness instead of only string shape, and
confirm it still matches the offset/fractional-second cases covered by
datetime_offset_shapes before removing the custom logic. Keep the
is_iso_datetime_offset entry point and its callers unchanged, but simplify its
implementation to delegate parsing and return false on parse errors.
In `@crates/ask/src/resolvers/maven.rs`:
- Around line 165-218: `fetch_search_api` is making a redundant Maven Search API
call in the common `version == "latest"` path because it always invokes
`fetch_scm_url` even though the first search response already contains the
needed data. Update `fetch_search_api` and `MavenSearchDoc` so `scm.url` is
parsed from the initial response and reused when resolving latest versions, and
only fall back to `fetch_scm_url` for non-latest cases if necessary.
In `@crates/ask/src/skills/vendor.rs`:
- Around line 72-75: The replace flow in vendor.rs is not fully atomic because
VendorDir removal happens before the staging rename, leaving a gap where the
directory can disappear if the process dies. Update the install/replace logic
around vendor_dir, staging, and the rename step to use a swap-rename approach
(or equivalent temp/backup rename sequence) so the old directory is only removed
after the new one is safely in place. Keep the change localized to the vendor
install path that currently calls remove_dir_all and rename.
In `@crates/ask/src/sources/npm.rs`:
- Around line 158-169: The npm tarball download flow in the source that builds
`integrity`, fetches `response`, and then unpacks with
`GzDecoder`/`tar::Archive` currently ignores the captured `dist.integrity`.
Update this path to verify the downloaded bytes against the SRI hash before
extraction, failing fast on mismatch and only proceeding to unpack when the
checksum matches. Keep the change localized around the `integrity` handling and
tarball extraction logic, and make sure the verification is applied before
`unpack(tmp.path())` in the npm source loader.
In `@npm/ask/install.js`:
- Around line 20-92: Wrap the top-level main() entry point in a guard that
guarantees the install step always exits successfully, even if a future change
in resolveBinaryPath() or lib/resolve.js starts throwing unexpectedly. Keep the
existing best-effort behavior in main(), but add a top-level try/catch around
the call site so any uncaught error is swallowed and the npm install contract
remains exit 0.
In `@npm/ask/lib/resolve.js`:
- Around line 20-52: The `resolvePlatformPackage` function is too branch-heavy
and exceeds the cognitive complexity limit; flatten its platform/arch logic by
replacing the nested `if` chain with a lookup table keyed by platform and
architecture, and keep `isMusl()` as a small post-lookup override for the Linux
x64 and arm64 cases. Preserve the current return values and the null fallback
behavior, and keep the function name `resolvePlatformPackage` so the refactor is
localized and easier to extend with new targets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d55117a-1f49-47d9-aff6-6a4081b8022c
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockbun.lockis excluded by!**/*.lock
📒 Files selected for processing (73)
.claude/settings.json.github/workflows/release-cargo.yml.github/workflows/release-rust.yml.github/workflows/release.yml.gitignore.please/docs/tracks.jsonl.please/docs/tracks/active/rust-port-20260704/metadata.json.please/docs/tracks/active/rust-port-20260704/plan.md.please/docs/tracks/active/rust-port-20260704/spec.mdCargo.tomlcrates/ask/Cargo.tomlcrates/ask/README.mdcrates/ask/src/agents.rscrates/ask/src/ask_json.rscrates/ask/src/cli.rscrates/ask/src/commands/add.rscrates/ask/src/commands/docs.rscrates/ask/src/commands/ensure_checkout.rscrates/ask/src/commands/fetch.rscrates/ask/src/commands/find_doc_paths.rscrates/ask/src/commands/find_skill_paths.rscrates/ask/src/commands/mod.rscrates/ask/src/commands/remove.rscrates/ask/src/commands/resolve_csp.rscrates/ask/src/commands/search.rscrates/ask/src/commands/skills.rscrates/ask/src/commands/src.rscrates/ask/src/discovery/candidates.rscrates/ask/src/discovery/mod.rscrates/ask/src/http.rscrates/ask/src/ignore_files.rscrates/ask/src/install.rscrates/ask/src/io.rscrates/ask/src/lib.rscrates/ask/src/list.rscrates/ask/src/lockfiles/mod.rscrates/ask/src/lockfiles/pnpm.rscrates/ask/src/main.rscrates/ask/src/markers.rscrates/ask/src/registry/api.rscrates/ask/src/registry/mod.rscrates/ask/src/resolved.rscrates/ask/src/resolvers/maven.rscrates/ask/src/resolvers/mod.rscrates/ask/src/resolvers/npm.rscrates/ask/src/resolvers/pub_dev.rscrates/ask/src/resolvers/pypi.rscrates/ask/src/skill.rscrates/ask/src/skills/agent_detect.rscrates/ask/src/skills/lock.rscrates/ask/src/skills/mod.rscrates/ask/src/skills/spec_key.rscrates/ask/src/skills/symlinks.rscrates/ask/src/skills/vendor.rscrates/ask/src/sources/github.rscrates/ask/src/sources/llms_txt.rscrates/ask/src/sources/mod.rscrates/ask/src/sources/npm.rscrates/ask/src/spec.rscrates/ask/src/storage.rscrates/ask/src/store/cache.rscrates/ask/src/store/mod.rscrates/ask/src/store/paths.rsnpm/README.mdnpm/ask/bin/ask.jsnpm/ask/install.jsnpm/ask/lib/resolve.jsnpm/ask/package.jsonnpm/scripts/generate-platform-packages.mjsrelease-please-config.jsonrust-toolchain.tomlrustfmt.tomlscripts/parity-diff.sh
…y, symlink/URL parity Address the CodeRabbit review on PR #119. 6 findings fixed; 5 confirmed faithful mirrors of the TS oracle (fixing only Rust would break byte-parity) and deferred/answered. Fixed: - agents.rs: generate_agents_md/strip_block/update_claude_md now return Result and propagate fs write/remove errors instead of swallowing them via `let _ =`. TS uses bare writeFileSync (throws on failure); Rust was masking a failed AGENTS.md/CLAUDE.md write behind a "success" install. Bubbled through run_install. - main.rs: a runtime command failure (install/remove/list) now exits 1, matching the TS citty runMain (was exit 2). clap's own usage/parse errors still exit 2. - io.rs: write_ask_json/write_resolved_json write atomically (temp file + fsync + rename) so a crash mid-write can't truncate ask.json (the source of truth, no self-heal). Same final bytes → parity-preserving. - skills/vendor.rs: copy_dir_recursive stats the symlink TARGET (follows, like fs.cpSync) so a symlink-to-directory recurses instead of hitting fs::copy (which errors on a dir). Regression test added. - sources/llms_txt.rs: url_last_segment takes the LAST path segment (even a trailing-slash empty one) then falls back to llms.txt, matching TS `pathname.split('/').pop() || 'llms.txt'`; the previous rfind(non-empty) turned `.../docs/` into `docs`. Test extended. - release.yml: pass the release tag through env (CLI_TAG_NAME) instead of interpolating the GHA expression into the script body — the safe pattern the homebrew job already uses; silences the script-injection SAST flag. Deferred (faithful TS mirrors — see PR replies / #120): scoped-@ add classification, collectSkillDirs full-path dedup, ensure_checkout vestigial is_from_branch, agents single-marker append, Windows symlink error UX. 284 tests, clippy/fmt/actionlint clean, all parity cases byte-identical.
Contributor
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Follow-up to the directory-symlink dereferencing added in the prior commit (cubic P1 on a9f693a). Now that copy_dir_recursive follows a symlink-to-dir, an in-tree cycle (ln -s . loop) would infinite-recurse into a stack overflow and an outbound symlink (→ /) would copy arbitrary external data into the vendor dir. Thread the canonical copy root + a stack-scoped visited set through the recursion: a subdirectory is only followed when its real path stays within the root and is not already on the current descent. Seeded with the root so a self-loop is caught. Cycle + escape regression test added.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ask/src/skills/vendor.rs`:
- Around line 52-73: The file-copy path in vendor.rs still allows escaped file
symlinks because `std::fs::copy` dereferences them without a containment check.
Update the `copy_dir_inner` logic so the regular-file branch applies the same
root confinement rules used for directories: resolve `from` with
`std::fs::canonicalize`, verify the real path stays under `root`, and only then
copy; otherwise skip the entry. Keep the fix localized around `copy_dir_inner`,
`visited`, and the existing directory symlink guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ec07d4fc-76eb-4da5-be91-9ff04a846085
📒 Files selected for processing (7)
.github/workflows/release.ymlcrates/ask/src/agents.rscrates/ask/src/install.rscrates/ask/src/io.rscrates/ask/src/main.rscrates/ask/src/skills/vendor.rscrates/ask/src/sources/llms_txt.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/ask/src/agents.rs
- .github/workflows/release.yml
- crates/ask/src/install.rs
- crates/ask/src/sources/llms_txt.rs
- crates/ask/src/io.rs
Contributor
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: This is a high-risk port of the CLI from Bun/TypeScript to Rust that touches core release infrastructure and distributions. It requires thorough review of architecture, security, and behavioral parity before merging.
Re-trigger cubic
Adds .claude/skills/run-ask-rust/ — agent-facing run skill for the Rust port of the ask CLI. smoke.sh builds the release binary and drives the full user flow (add/list/fetch/src/docs/search/docs-paths/remove/npm-add/ cache ls) in an isolated temp project + temp ASK_HOME with per-step assertions; SKILL.md documents build, smoke, direct test invocation, the parity harness, and verified gotchas.
…endor copy - storage.rs (greptile P1): link-mode pre-creation cleanup used docs_dir.exists(), which FOLLOWS the link and returns false for a dangling symlink (left behind after `ask cache gc` deletes the store target). The stale link then survived and symlink_dir failed with a confusing EEXIST. Use symlink_metadata (examines the link itself); remove_dir_all removes the link for both live and dangling links (verified). TS's error path already uses lstat — its pre-creation check has the same gap, tracked for the TS side. - skills/vendor.rs (coderabbit): the file branch of copy_dir_inner still let an escaping file symlink (→ /etc/passwd) copy external bytes via std::fs::copy — the directory branch already guards this. Apply the same root-containment check to file symlinks (regular files are physically in-tree, so only symlinks are checked). Escape test extended to cover a file symlink. 285 tests, clippy/fmt/actionlint clean, all parity cases byte-identical.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/run-ask-rust/SKILL.md:
- Line 68: The runbook entry in SKILL.md has a stale hard-coded test count in
the `cargo test -p ask-please` comment. Update the note in that command’s inline
comment to the current total, or remove the exact number entirely so the
guidance does not drift. Locate the `cargo test -p ask-please` line and adjust
the accompanying “full suite” text accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fd19913f-0352-4946-8378-225cf90acade
📒 Files selected for processing (4)
.claude/skills/run-ask-rust/SKILL.md.claude/skills/run-ask-rust/smoke.shcrates/ask/src/skills/vendor.rscrates/ask/src/storage.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/ask/src/skills/vendor.rs
- crates/ask/src/storage.rs
Contributor
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Requires human review: Porting CLI from TypeScript to Rust with new build pipeline and distribution; untested cross-compile matrix and npm package creation require human validation.
Re-trigger cubic
…book The exact '285 tests' note drifts every time a test is added (coderabbit). Drop the number; the runbook still tells the reader how to run the full suite.
|
Contributor
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: This is a large-scale port of the entire CLI from TypeScript to Rust (17,500+ lines), touching distribution, CI, and core logic. High risk of breakage; requires human review for architectural, security, and infrastructure implications.
Re-trigger cubic
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
2 participants
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.
Summary
Ports the
@pleaseai/askCLI from Bun/TypeScript to Rust. The Rust implementation lives in a newcrates/askworkspace member (published to crates.io asask-please, binary nameask), with all CLI commands ported:install,add,remove,list,docs,fetch,search,src,skills, andcache. Correctness is proven with a differential parity harness that runs the Rust binary and the TypeScript oracle side-by-side and asserts byte-identical generated output (279 unit tests + 9 differential parity cases).The most recent commit on this branch (
15485d5) cuts over.github/workflows/release.ymlso releases ship the Rust binaries instead of the Bun-compiled TS binary:release-rust.ymlworkflow across a 6-target matrix (macOS arm64/x64-intel, Linux x64/arm64/x64-musl, Windows x64).@pleaseai/askon npm becomes a copy-over shim: it publishes one platform-specific npm package per target plus a small pinned wrapper package that resolves and installs the right platform binary at install time.ask-pleasepublishes to crates.io via a newrelease-cargo.ymlworkflow.Related issue
Pre-merge gates (human, cannot be done in CI locally)
release-rust.ymlviaworkflow_dispatchwith no tag (builds + uploads artifacts only, no release write) to prove cross-compile acrossmacos-15-intel,ubuntu-24.04-arm,x86_64-unknown-linux-musl, and thewindows-msvctargets before merge. musl and windows are the likely failers.@pleaseai/ask-darwin-arm64,@pleaseai/ask-darwin-x64,@pleaseai/ask-linux-x64,@pleaseai/ask-linux-arm64,@pleaseai/ask-linux-x64-musl,@pleaseai/ask-win32-x64). Verify the names are unclaimed under the@pleaseainpm scope, and thatNPM_TOKENcan create new packages in that scope — a token scoped only to existing packages will successfully publishask-schemaand then fail on the first platform package mid-release, leaving the release half-published.Verified locally
actionlintclean on all 3 release workflows (release.yml,release-rust.yml,release-cargo.yml)generate-platform-packages.mjssmoke testcargo build --release --lockedcleanask --version→ask 0.4.8)Checklist
bun run lint) — see pre-merge gates above for the parts that cannot be verified locallyBREAKING CHANGE:note is included — distribution mechanism changes (npm shim, crates.io) but CLI behavior is unchanged per the parity harnessSummary by cubic
Ports the
@pleaseai/askCLI from Bun/TypeScript to Rust and switches releases to ship native binaries across npm, Homebrew, and cargo. Adds a.clauderun skill with a smoke driver, completes hardening and parity fixes (including atomic writes and stricter symlink safety), and keeps CLI behavior unchanged.New Features
crates/askpublished asask-please(binaryask) with full command parity: install, add, remove, list, docs, fetch, src, search, skills, cache.@pleaseai/askwith platform packages (e.g.@pleaseai/ask-darwin-arm64); crates.io publish viarelease-cargo.yml.release-pleaseupdates/Cargo.tomland/npm/ask/package.jsontogether and guards publishing unless all platform binaries exist..claude/skills/run-ask-rustskill andsmoke.shto build and exercise the Rust CLI end-to-end in an isolated project andASK_HOME; runbook updated to remove a hard-coded test count to avoid drift.ask cache gc; vendor copy follows dir symlinks with cycle/escape guards and now guards file symlinks too; atomic writes forask.json/resolved.json; propagate write errors forAGENTS.md/CLAUDE.md; fixllms.txtfilename derivation; runtime failures exit 1; release tag passed via env.ask.json/resolved.jsonvalidation;ask docssurfaces invalidask.json; Maven resolver acceptslatestVersion;read_resolved_jsonsurfaces non-NotFound errors; URL normalization matches TS.Migration
.github/workflows/release-rust.yml) via workflow_dispatch with no tag to validate the 6-target matrix.NPM_TOKENcan create new@pleaseai/*platform packages to avoid a partial publish. End users can keep usingnpx @pleaseai/ask;brew install pleaseai/tap/askandcargo install ask-pleaseare supported.Written for commit 1230770. Summary will update on new commits.
Summary by CodeRabbit