Agent hardening: verify-by-retrieval download, truthful agent surfaces, keystore UX#30
Conversation
incur 0.4.19 fixes the doubled group prefix in --llms output
("foc-cli piece piece list").
Closes #23.
Files now stream to providers via Readable.toWeb(createReadStream()) instead of being buffered whole; only stat sizes are read up front, so peak memory stays flat for any file size. multi-upload keeps its all-or-nothing gate by checking readability before streaming. BREAKING: dataset upload is removed — upload already creates a dataset automatically, and the low-level path duplicated it with a worse interface. Closes #24.
storage.download() validates the received bytes against the piece CID, so a successful download is itself the retrievability + integrity proof — no separate verify command needed. Error taxonomy separates what retrying can fix: INTEGRITY_MISMATCH (source served wrong bytes, never retryable), PROVIDER_NOT_FOUND, WRITE_FAILED (local, retrieval succeeded) vs DOWNLOAD_FAILED (transient, flagged retryable). Closes #7.
--url now only accepts docs.filecoin.cloud pages (full URL or bare docs path); anything else fails INVALID_DOCS_URL before any request, and redirects are refused so the restriction holds end-to-end. Pretty paths are rewritten to their .md mirror and an HTML backstop refuses to hand agents sidebar markup on both the --url and auto-fetch paths. --deep searches the full ~1,800-page sitemap (SDK API reference, changelogs), also used automatically when the curated index has no matches — a failed sitemap fetch fails loudly instead of masquerading as "no matches". Requests carry an identifying User-Agent with the configured source tag. Includes the MCP read-only annotation for the tool. Closes #25.
prepare() without a context falls back to smart provider selection, which pings endorsed providers and failed with "No endorsed provider available". Build contexts from the user's own live, managed, non-terminating datasets instead — one createContext() call per dataset, because the plural createContexts() rejects datasets sharing a provider and each dataset has its own rail and lockup to cost. Empty dataset list falls back to default selection. Also carries the command's MCP read-only annotation and truthful output schema. Closes #26.
Keystore extraction now uses execFileSync with an argv array (the path is data, never shell syntax) and expands ~ itself so MCP/config paths work without a shell. wallet init --keystore validates it points at an encrypted keystore file — directories and non-keystore JSON fail as KEYSTORE_INVALID at init instead of surfacing later as a raw decrypt error. Use-time failures now decode themselves: ENOENT means cast is not installed, "Mac Mismatch" means wrong password, and no tty means keystore mode cannot run under MCP/CI. wallet balance on a brand-new address returns ADDRESS_NOT_ON_CHAIN with a wallet fund CTA instead of the raw viem multicall dump — it is the first command a new user runs. Includes the two commands' MCP annotations and truthful output schemas. Closes #27.
Every command's declared output now goes through commandOutput(), which adds the processLog step trail and optional cta block that agent-mode responses actually carry — a bare z.object emits additionalProperties:false, which every real response violated, so --schema lied to anyone validating against it. MCP consumers cannot run -h or --schema, so the tool definitions now compensate: every command carries mcp.annotations (human title, readOnlyHint on reads, explicit destructiveHint on dataset terminate / piece remove) and descriptions state consequences — uploads commit USDFC onchain, terminate is irreversible. The download, docs, costs, balance, and wallet init annotations ride their own commits; this one completes the set. Paginated lists (piece list, dataset details) also gain a fetch-all CTA alongside next-page, and the interactive spinner no longer blanks step labels or leaks orphan glyphs when info/success interleave with steps. Closes #28.
New coverage: download success/taxonomy (integrity mismatch, provider not found, local write failure, default output path), docs URL restriction + .md normalization + HTML backstop + deep sitemap search + User-Agent attribution, per-dataset costs contexts, keystore init validation, and the new-address balance error. skills-consistency pins both skills' frontmatter version/license and every foc-cli@x.y.z doc pin to cli/package.json so releases cannot drift from the skills.
Both skills now open command guidance with the self-discovery rule (run <cmd> -h and <cmd> --schema --format json once before first use) and document flag syntax truthfully: camelCase and kebab spellings both parse, booleans are presence-only switches (--flag true leaks the value into positionals; --flag=false is the explicit form). Keystore mode is documented as interactive-CLI-only everywhere it matters — the password prompt reads the tty at use time, so MCP/CI need a private-key wallet; no password-in-config option will exist. Three new references: keystore-setup (creation recipe included — cast wallet new needs the directory to exist), mainnet-funding, and a troubleshooting catalog of every error code with retry semantics. Frontmatter gains version/license (CI-pinned to package.json) and openclaw install metadata. Closes #1. Closes #2. Closes #3. Closes #4. Closes #5.
Badges, one-table command map, quick start that walks init->fund->costs->deposit->upload->download (download doubles as the storage proof), wallet/key-safety and chain/funding sections that link the skill references. ClawHub install lines assume the skills are published there — publish first or drop that line before release. Closes #29.
Keep a Changelog format, reconstructed from git history and npm publish records: 0.0.4 (initial), 0.1.0 (SDK upgrade + CI), 0.1.1 (Synapse v1 migration + CLI hardening — published from the hardening branch tip, verified via the npm gitHead), and the Unreleased agent-hardening set with its breaking dataset-upload removal flagged for a 0.2.0 bump.
The npm description now names the whole surface — CLI, MCP server, and agent skills — and the CLI root description reads it from package.json so the two can never drift. Keywords gain ai-agent/agent-skills/openclaw/clawhub/cli — the skills already ship openclaw install metadata in their frontmatter, so ClawHub compatibility is a present fact even before registry publication.
|
Preliminary findings before a deep review pass: My read: PR #30 needs a risk-first, contract-first review. It combines security hardening, money-path changes, filesystem behavior, agent/MCP contracts, breaking CLI changes, and substantial documentation. Current state:
Recommended review lanes
Preliminary pressure pointsThese are leads, not yet formal findings:
I recommend beginning the deep pass with lanes 1–4, then validating agent contracts and packaging. Any live-chain work should remain read-only; uploads, deposits, funding, or other transactions would require separate authorization. |
snissn
left a comment
snissn
left a comment
There was a problem hiding this comment.
CI is green and the change set is well organized, but I found several correctness and safety issues that should be fixed before merge:
wallet costspricesextraBytesagainst every active dataset rather than the contexts/copy count the next upload will use, so the estimate depends on historical dataset count and is wrong for common 1- or 3+-dataset wallets.- The docs-host boundary is bypassed by URLs parsed from
llms.txt; the auto-fetch path does not revalidate them. - Both streaming upload paths accept non-regular files during preflight and can execute preparation/funding before a directory, FIFO, or device fails or blocks during streaming.
wallet init --autodoes not reliably replace the configured wallet, and MCP can still configure an interactive-only keystore.downloadcan overwrite arbitrary existing files while advertisingdestructiveHint: false.- The new actor-not-found CTA directs mainnet users to the Calibration-only faucet.
Please also complete the pre-merge gates already called out in the PR body: make the breaking dataset upload removal a consistent 0.2.0 release, and resolve the ClawHub publication/install-row gate.
The new happy-path and error-envelope tests are useful; the inline notes identify the missing regression cases.
snissn
left a comment
snissn
left a comment
There was a problem hiding this comment.
Follow-up deep-pass findings beyond the seven blockers already submitted:
- The high-level upload's explicit
--withCDNpath violates the pinned SDK option contract and fails before storage. wallet initis the only executable command without an output schema, so both CLI schema discovery and MCP tool details omit its result contract.- Keystore validation still checks only for directories, allowing special files such as FIFOs to reach a synchronous read and potentially block the CLI/MCP process.
These were verified against exact head eec371df6c469991fd935e7721d65f1aef7713f0. The inline comments include the required regression coverage.
Publishing the skills to ClawHub is deferred by decision, so the README no longer advertises a ClawHub install or links the registry. The skills remain ClawHub-compatible — openclaw frontmatter and the openclaw/clawhub npm keywords stay.
stat() succeeds for directories, FIFOs, sockets, and devices, so both upload paths could execute the funding transaction before the stream failed or blocked in store(). Gate on Stats.isFile() before provider selection, and open the upload stream only right before it is consumed.
The pinned SDK rejects upload options that contexts were already built from — upload --withCDN failed after the funding transaction had run. CDN preference rides in via createContexts alone; the upload mock now mirrors the SDK guard so every upload test enforces the contract.
Only --url went through resolveDocsUrl; entries parsed out of llms.txt were auto-fetched verbatim, so a planted external link bypassed the boundary. One validator now gates every URL that arrives inside fetched content (index entries, sitemap locs, shards): https on the docs host or dropped at parse time.
--auto ran after the existing-key shortcut, so it reported already_configured instead of replacing; with a keystore configured it set a key the keystore silently outranked. Explicit methods now run first and clear the alternate credential. Agent mode rejects --keystore (unusable there: cast prompts on the terminal at use time) and its guidance no longer offers it. Tests pin isAgent to the context flag — the runner's non-TTY stdout made every context agent mode.
Only directories were screened, so a FIFO or device node reached the synchronous readFileSync and could block the CLI/MCP process indefinitely. Gate on isFile() and require the keystore's crypto field to be an object, not merely present.
init was the only executable command without an output declaration, so --schema and MCP get_tool_details omitted its result contract. Declare both envelopes (configured / already_configured) and add a discovery test that walks src/commands so the next schema-less command fails CI.
The actor-not-found guidance suggested wallet fund on every chain, but the faucet is Calibration-only — an agent on chain 314 would fund the wrong network. The CTA now appears only on 314159; mainnet gets prose directing FIL/USDFC to the address.
download overwrote any existing --out path while advertising destructiveHint: false to MCP clients. Default to exclusive creation (EEXIST -> FILE_EXISTS with a --force CTA), add --force for explicit overwrite, and declare destructiveHint: true since forced overwrite remains possible.
costs built one context per active dataset, and prepare() applies dataSize once per context — so the quote scaled with historical dataset count (1 dataset priced 1 copy, 3 priced 3) instead of the requested upload. Price exactly --copies contexts (default 2, like upload): existing datasets first, new-dataset placeholders for the rest, which also keeps the empty-wallet path away from provider selection.
New codes (NOT_A_FILE, FILE_EXISTS, KEYSTORE_INTERACTIVE_ONLY) join the troubleshooting catalog, agent init guidance drops the keystore method, and the Unreleased changelog records the PR #30 review round: costs pricing by copies, download overwrite protection, init replacement semantics, the docs index allowlist, and the withCDN contract fix.
Live smoke (2026-07-23) showed Glif Calibration now fails a never-funded address's eth_call with 'failed to apply on state with gas' instead of 'actor not found', so the ADDRESS_NOT_ON_CHAIN humanization silently regressed to the raw multicall dump. Match both wordings.
The success CTA recommended 'piece upload', a command removed from the surface — agents following it hit an unknown-command error. Suggest upload (provider selection reuses the new dataset) and dataset details instead.
summary's timeRemaining concatenated the same duration in five units
('17468h 727d 103w 25m 2y'), live-observed reading as nonsense. Pick
the largest unit that fits ('~2y'), with 'mo' for months so it cannot
be misread as minutes.
|
Responding to both CHANGES_REQUESTED reviews: all nine blockers (plus the multi-upload half of the streaming finding) verified against source and confirmed — nothing rebutted. Each is fixed in its own commit, now pushed; per-finding details are in the inline threads. The suite grew 78 → 98 tests, and every commit in the fix range is individually test/lint/build-green.
Pre-merge gates from the review:
On "evidence presently unauditable from the PR": after the fixes, a full live re-smoke ran on Calibration against the funded test wallet — all 18 leaf commands plus integrations (completions, skills add/list, mcp add/doctor, raw
The re-smoke also surfaced and fixed three further bugs now in the branch: the fresh-address humanization had regressed against current Glif RPC wording ( |
|
Follow-up review at Exact-head CI is green. I rechecked the prior review findings against the implementation rather than relying only on thread replies. The original concrete blockers are substantially addressed: index-derived docs URLs are allowlisted, non-regular upload inputs fail before preparation/funding, the SDK Merge assessmentI do not see a massive architectural, data-loss, or unauthorized-spend blocker remaining. I do see two bounded issues that should be handled before merge—or explicitly narrowed so the implementation work can defer. 1. Merge blocker, but small/mechanical: preserve
|
A command run with --chain 314 could hand an agent a CTA that silently defaulted back to Calibration — reaching destructive and fund-moving workflows (terminate, remove, deposit, pagination). One chainCta() helper stamps options.chain onto every chain-aware CTA (docs and wallet init have no --chain and are exempt); a mainnet sweep test rejects any suggested command that loses the chain.
upload selects reachable unique providers and matches source/CDN metadata, so it may not reuse the datasets costs estimates against — creation fees, CDN lockups, and depositNeeded can differ. Say so in the command description, README, and skill instead of calling the estimate the source of truth; the upload re-quotes via its own prepare() before spending. Full provider-selection alignment is tracked separately.
|
Both pre-merge items are addressed at the new head:
Deferred items are tracked in #ISSUE_DEFERRED (TOCTOU, fresh-address RPC-match tightening, existing-keystore no-op + Merge hygiene: the PR body's closure references for #1–#5 and #7 are downgraded to "addresses", with auto-close kept only for the evidence-based reports (#23–#29); #1/#3/#5/#7 acceptance stays an explicit owner decision. On the 0.2.0 gate we'll follow the release-metadata-PR pattern — no npm/skill publication until version, frontmatters, pins, and changelog are consistently 0.2.0. |
|
cc: @snissn |
snissn
left a comment
snissn
left a comment
There was a problem hiding this comment.
Follow-up review at 173cc53f65d3ea94b0413918b7aa40b237f3d65e: approved.
I rechecked the two commits pushed after the last merge assessment:
chainCta()preserves an explicitly supplied chain and is applied only where every suggested command accepts--chain. The mainnet regression sweep covers pagination, destructive/fund-moving follow-ups, download success/error paths, and wallet flows. This resolves the cross-network CTA blocker.wallet costsnow describes the result as an approximation against existing datasets, explicitly identifies the provider/source/CDN mismatch risk, and tells callers that upload-timeprepare()is the final quote before spending. This is an acceptable deferral of full provider-selection alignment rather than a misleading exact-pricing contract.- The prior docs-host allowlist thread is verified and resolved.
- Exact-head CI is green for tests, formatting/lint, typecheck, and build.
I found no new blocking correctness, security, local-data, or spend-safety issue in the pushed changes. The remaining items are safe follow-ups: full cost/provider-selection alignment, the documented check-then-stream TOCTOU, broader RPC error classification, and attaching reproducible smoke evidence.
Two non-blocking release/PR-description cleanups remain:
- Keep the breaking
dataset uploadremoval out of a published0.1.1; land the atomic0.2.0package/skill/doc-pin bump before publishing. - The PR body now correctly says only #23–#29 auto-close, but task 5 still says merging auto-closes #1–#5 and #7. Please reconcile that stale line so the acceptance decision is unambiguous.
From a code-review perspective, this can merge.
2 participants
Hardens every surface an agent (or human) touches, driven by a 609-invocation live smoke campaign on Calibration and a keystore field test. Ten commits, grouped by concern.
What's changed
New capability
download <pieceCid>— retrieval as verification: the SDK validates received bytes against the piece CID, so a successful download is the storage proof; no separate verify command. Error taxonomy separates what retrying can fix (DOWNLOAD_FAILED, retryable) from what it can't (INTEGRITY_MISMATCH,PROVIDER_NOT_FOUND,WRITE_FAILED). Addresses Add download/verify commands for file retrieval and verification #7 — acceptance decision below.docs --deep— full-sitemap search (~1,800 pages incl. SDK API reference), auto-invoked when the curated index has no matches, failing loudly if the sitemap is unreachable.Hardening
docsis now a docs fetcher, not an HTTP client:--urlrestricted todocs.filecoin.cloud, redirects refused,.md-mirror normalization, HTML backstop on both fetch paths, attributed User-Agent.execFileSyncargv arrays (no shell interpolation) with~expansion;wallet init --keystorerejects directories and non-keystore JSON (KEYSTORE_INVALID) instead of "You're all set!".Readable.toWeb(createReadStream)) — flat memory at any file size; multi-upload keeps its all-or-nothing readability gate.Fixes
wallet costsno longer dies on "No endorsed provider available": costs are computed per existing dataset (onecreateContexteach — the plural API rejects same-provider datasets), falling back to default selection when the wallet has none.wallet balanceon a brand-new address returnsADDRESS_NOT_ON_CHAINwith awallet fundCTA instead of a raw viem multicall dump.Mac Mismatch→ wrong password; no tty → use a private-key wallet).Agent surfaces
--schemanow tells the truth: every command's declared output includes theprocessLog/ctaenvelope real responses carry (previously violated its ownadditionalProperties: false).readOnlyHint/destructiveHint) and consequence-stating descriptions, since MCP consumers can't run-h/--schema.--llms.Docs & skills (addresses #1 #2 #3 #4 #5 — acceptance decision below)
<cmd> -h+<cmd> --schema --format jsonbefore first use) and document flag syntax truthfully (camelCase/kebab both parse; booleans are presence-only switches;--flag=falseis the explicit form).skills-consistencytest pins skill frontmatter version/license and everyfoc-cli@x.y.zdoc pin tocli/package.json.Removed (breaking)
dataset upload—uploadalready auto-creates datasets; the low-level duplicate had a worse interface.Commit → issue traceability
Every commit references the issue it addresses; merging auto-closes only #23–#29 (evidence-based bug reports fully fixed here). #1–#5 and #7 are docs-redirect deliveries — closed manually only if the owners accept that scope (see pre-merge task 5).
chore(deps)bump synapse-sdk 1.1.0, incur 0.4.19--llmsprefix doubling)refactor(upload)streaming + dropdataset uploadfeat(download)verify-by-retrievalfeat(docs)host allowlist + deep searchfix(costs)per-dataset contextsfix(wallet)keystore validation + first-run UXfeat(agent)truthful schemas + MCP annotationstest:coverage for all of the abovedocs(skills)self-discovery, keystore/funding guidesdocs(readme)compact rewriteDeliberately not addressed here: #6 (create-app scaffold).
Verification
bun test78/78 (5 new suites' worth of cases),bun run lint(tsc + biome) andbun run buildclean at head. Smoke evidence in the (local-only)docs/smoke-report-2026-07-21.md.Note: commits are grouped by concern for reviewability; intermediate commits are not individually test-green (tests land together in the
test:commit). Merge with a merge commit or squash — don't cherry-pick individual commits.Tasks before merging
dataset uploadremoval is a breaking CLI-surface change → bump0.1.1→0.2.0(pre-1.0 semver). Must change together (the skills-consistency test enforces it):cli/package.json, both SKILL.mdversion:frontmatter, and thefoc-cli@0.1.1pin examples in README + skills.--flag truefor booleans (misleading — value parses as a positional), acronym kebab-casing (--with-c-d-n),--schemaprints TOON unless--format json. Worth filing on wevm/incur; local docs work around all three.--llmson command groups doubles the group prefix #23–README drifted from the CLI surface #29 are fully fixed here and are closed at merge./code-review ultraon the branch for a multi-agent review pass before requesting human review.What needs review (focus areas)
cli/src/commands/docs.tsfetch surface —resolveDocsUrl/normalizeDocsPath, redirect refusal, traversal rejection (three SSRF variants were smoke-tested, but this is the code that holds the line).cli/src/client.tskeystore exec path.wallet/costs.tsper-dataset context building (live-verified: 1 GiB/mo → 0.1322 USDFC on Calibration, up from the under-counting 0.1058); uploadprepare()now sizes fromstatinstead of the read buffer.dataset uploadremoval (and whether any downstream scripts depend on it).commandOutput()widens every command's declared output with optionalprocessLog/cta— anyone validating responses against the old schemas sees new optional fields.