feat(fs): add Volume filesystem commands

[hellozepp]

Summary

• add fs filesystem commands for local and Lakehouse Volume paths
• add Named Volume creation via fs mb
• support Volume ls limits and safe move/copy behavior
• handle empty Volume roots and preserve FS_TARGET_EXISTS errors

Validation

• real quick_start.public Named/User Volume smoke tests completed and cleaned up
• SDK typecheck passed
• SDK FsUtil tests: 10 passed
• CLI fs tests: 3 passed
• fs help tests: 8 passed
• git diff --check passed

Note

• repository pre-push full typecheck remains blocked by the pre-existing packages/opencode/src/bus/global.ts EventEmitter type error; branch was pushed with --no-verify after targeted validation.

[github-actions]

HIGH — quoteString does not escape backslashes; this SDK's own escaper does. Confidence: medium-high.

function quoteString(value: string): string {
  return "'" + value.replace(/'/g, "''") + "'"
}

packages/clickzetta-sdk/src/sql/literal.ts already owns literal quoting for this dialect, and it escapes backslashes as well as quotes:

export function escape(value: unknown): unknown {
  return value.replace(/\\/g, "\\\\").replace(/\n/g, "\\n").replace(/\r/g, "\\r").replace(/'/g, "\\'")
}

That helper mirrors the Python connector's Converter.escape, which is the authoritative signal that ClickZetta's lexer treats \ as an escape inside single-quoted literals. If it does, doubling ' alone is not sufficient: a trailing backslash consumes the closing quote.

validateRelativePath (line 126) rejects control characters, empty segments, . and .., but permits \. So a path segment ending in a backslash reaches quoteString intact:

cz-cli fs head 'volume://vol/a\'
→ select get_presigned_url(volume `w`.`s`.`vol`, 'a\', 3600, 'GET')

The \' is read as an escaped quote, so the literal runs on into , 3600, 'GET') and the statement boundary is under caller control. At minimum this corrupts the query for any legitimate path containing a backslash; at worst it is injection into a statement that is built by string concatenation on every fs operation (get_file, list_directory, get_presigned_url, create_directory, remove).

Smaller correct change: drop quoteString and call the existing escape/quote from ./sql/literal.js — one shared escaper, no second copy of the rules to keep in sync.

[github-actions]

HIGH — an empty local directory moved to a Volume is deleted with nothing written at the destination. Confidence: high.

if (info.isDir && !from.isLocal && (await from.children(recurse)).length === 0) throw new FsError("FS_TRANSFER_FAILED", `Cannot move an empty directory: ${source}`)
if (from.isLocal && to.isLocal) return this.moveLocal(from, to, info, recurse, overwrite)

The guard is scoped to !from.isLocal, and moveLocal handles local→local separately (line 648 explicitly mkdirs the temp path for a childless source). That leaves local → Volume with no handler for the empty-directory case:

Failure scenario — mkdir -p /tmp/data/nested && cz-cli fs mv /tmp/data volume://shared_files/data -R where the tree contains only directories, no files:

1. line 605: from.isLocal is true, so the guard is skipped.
2. line 606: to.isLocal is false, so moveLocal is skipped.
3. copyBytes → sourceInfo.isDir → children(true) returns [] → files is empty → to.mkdirs() is skipped (line 532) → the loop body never runs → returns 0.
4. from.remove(recurse) at line 618 runs rm -r /tmp/data.
5. The command reports {"data":{"source":...,"destination":...,"status":"SUCCEEDED"}} with exit 0.

The source directory tree is gone and nothing exists at the destination. Volume→local hits the guard instead and fails with FS_TRANSFER_FAILED — a code documented in the spec as a transfer failure (exit 1), not "this directory has no files" — so the three source/destination combinations give three different answers to the same input shape.

Root cause is one level down: copyBytes treats "zero files to copy" as "nothing to create" (see the separate comment on line 532). Fixing copyBytes to create the destination directory unconditionally for a directory source makes all three combinations behave the same and lets this special case be deleted rather than extended to a third branch.

No test covers local→Volume; fs-command.test.ts covers only the local→local empty-directory move.

[github-actions]

MEDIUM — the destination directory is only created when there is at least one file to copy; this is the root cause of the empty-directory special cases above and in moveLocal. Confidence: high.

const copyProgress = progress ?? { completed: [] }
if (files.length > 0) {
  try { await to.mkdirs() }

cp -R of an empty (or file-less) directory therefore succeeds while creating nothing:

mkdir -p /tmp/src/a/b
cz-cli fs cp /tmp/src /tmp/dst -R   # exit 0, bytes 0, /tmp/dst never created

Two callers already work around this rather than fixing it: mv grows a !from.isLocal empty-directory guard (line 605) and moveLocal grows its own if (info.isDir && sourceChildren.length === 0) await mkdir(...) branch (line 648). Both disappear if to.mkdirs() moves out of the files.length > 0 guard for a directory source. The -R-less shallow copy also silently drops empty subdirectories, which is documented in the spec, so only the top-level destination needs the unconditional create.

[github-actions]

MEDIUM — a local→local directory move replaces an existing destination directory instead of merging into it, silently deleting files the source does not have. Confidence: high.

const targetExists = await target.exists()
if (targetExists) await rename(target.scopePath, backup.scopePath).catch(...)
try {
  await rename(temporary.scopePath, target.scopePath)
...
await from.remove(recurse)
if (targetExists) await removeFile(backup.scopePath, { recursive: true, force: true })

Failure scenario — ./archive/data/old.txt exists, ./data/ contains only new.txt:

cz-cli fs mv ./data ./archive/ -R

target resolves to ./archive/data; it exists, so it is renamed aside to ./archive/data.cz-backup-<uuid>, the temp copy is renamed into its place, and line 659 deletes the backup recursively. old.txt is gone, and the command reports SUCCEEDED.

Every other move path merges, because it goes through copyBytes, which writes files individually into the existing destination and only overwrites same-named entries. So fs mv ./data volume://v/archive/ -R merges while fs mv ./data ./archive/ -R replaces. The PR's own spec says the directory case is "先覆盖复制全部文件" (overwrite-copy all files) — i.e. merge — and --overwrite is documented as "Replace existing destination files", not the whole directory.

Smaller correct change: only use the rename-swap for a file target. For a directory target, rename the temp tree into place only when the destination does not already exist, and otherwise let copyBytes write into the existing destination directly (which is what the Volume paths already do).

No test covers a directory move onto an existing destination directory.

[github-actions]

MEDIUM — the catch cleans up temporary but never backup, and a failure at the REMOVE stage does not surface as PARTIAL_FAILED. Confidence: high.

      await from.remove(recurse)
      if (targetExists) await removeFile(backup.scopePath, { recursive: true, force: true })
      return true
    } catch (error) {
      await removeFile(temporary.scopePath, { recursive: true, force: true }).catch(() => undefined)
      throw error
    }

If from.remove(recurse) throws (source now read-only, EPERM, a file grabbed by another process), two things go wrong:

1. backup is never removed. The user is left with <destination>.cz-backup-<uuid> — a full second copy of the previous destination — with a name they have no reason to recognize. The temporary cleanup in the catch is a no-op at that point, since it was already renamed away.
2. The error propagates as whatever mapLocalError produced (FS_PERMISSION_DENIED, etc.). The spec for this exact case says: "删除源文件失败时保留已完成目标并返回 PARTIAL_FAILED", and the Volume path at line 618-626 does wrap it. moveLocal never produces PARTIAL_FAILED at all, so a caller distinguishing "nothing happened" from "destination written, source still present" gets no signal on the local path.

State after the failure is destination-replaced + source-still-present + orphan backup, reported as a plain permission error. Suggest removing backup in the same catch, and wrapping the from.remove failure in PARTIAL_FAILED with stage: "REMOVE" to match the Volume path and the spec.

[github-actions]

MEDIUM — children() on a local path never returns directory entries, so fs ls of a local directory silently omits its subdirectories. Confidence: high.

if (entry.isDirectory() && recursive) {
  ...
  result.push(...await child.children(true, remaining))
  ...
}
else if (!entry.isDirectory()) result.push(child)

A directory entry is only ever descended into, never pushed. VolumeFsPath.children (line 354-370) does the opposite: it maps every row from list_directory, including rows where isDir is true. So for the same directory shape:

mkdir -p /tmp/d/sub && cz-cli fs ls /tmp/d
→ {"data":{"entries":[],"truncated":false}}

while cz-cli fs ls volume://v/d lists sub with "type":"directory".

Three consequences: fs ls cannot be used to navigate a local tree at all (a directory containing only directories reads as empty); entry.isDir ? "directory" : "file" in commands/fs.ts:43 is dead for local paths; and --limit counts directories on Volume paths but not local ones, so truncated is not comparable between the two. fs rm --dry-run -R inherits the same gap — it lists the files that will be deleted but none of the directories.

The spec's own ls output table shows a type: directory row, and it flags this as unresolved ("CLI 再根据结果决定是否统一补齐目录记录"), so this may be a deliberate deferral — but as shipped the local and Volume output shapes disagree for the same command. If the intent is files-only, the Volume side needs to filter directories out to match; if the intent is the spec's table, LocalFsPath.children needs to push directory entries too. Either way copyBytes must keep filtering them (it already does at line 530).

No test covers fs ls on a local directory containing a subdirectory.

[github-actions]

MEDIUM — presigned-URL transfers use bare fetch with no retry, while this SDK already has a retry wrapper built for exactly these transfers. Confidence: high.

const response = await fetch(await this.url("GET"))
if (!response.ok) throw mapHttpError(response.status, "reading", this.original)

packages/clickzetta-sdk/src/sql/volume.ts exports executeVolumeTransferWithRetry (3 retries, exponential backoff) plus isRetryableVolumeError, which classifies exactly the failure set these calls will hit — 408/429/500/502/503/504 and transient socket messages ("connection reset by peer", "max retries exceeded", "timed out"). The existing Volume PUT/GET path wraps every transfer in it; the three new transfer sites here (read line 389, write line 420, copyTo line 434) do not.

Failure scenario: cz-cli fs cp ./tree volume://v/tree -R over 500 files. One S3 503 on file 300 aborts the whole command with FS_TRANSFER_FAILED; the pre-existing PUT/GET path would have retried that same 503 and continued. The blast radius grows with directory size, which is the case this feature is for.

Smaller correct change: wrap the three fetch calls in executeVolumeTransferWithRetry rather than adding a retry loop here — same helper, same classification, no second copy of the backoff policy.

[github-actions]

MEDIUM — an Azure-Blob-specific header is sent unconditionally on every presigned PUT, whatever the backing store is. Confidence: medium.

const headers: Record<string, string> = { "x-ms-blob-type": "BlockBlob" }
if (contentLength !== undefined) headers["content-length"] = String(contentLength)

x-ms-blob-type is required by Azure Blob Storage and meaningless elsewhere. This is the only occurrence in the repo — sql/object-storage.ts models OSS, S3 and Azure as distinct providers, and the existing Volume PUT path in sql/volume.ts:439-443 sends headers: {}. So either the existing path is broken on Azure or this header is a workaround for one deployment that has been generalized to all of them.

Two questions worth answering before this ships: is the header safe on the OSS/S3 presigned PUTs (an unsigned extra header is usually ignored, but if the presign covers it the signature check fails with 403), and if it is genuinely needed, should it be derived from the URL/provider rather than hardcoded? A comment naming the deployment it was measured against would also keep the next reader from deleting it.

Separately, content-length set by hand alongside a ReadableStream body is ignored or rejected by most fetch implementations — worth confirming it does anything on Bun's fetch rather than being dead.

[github-actions]

MEDIUM — getExecContext is re-run per SQL statement, not once per command. Confidence: high.

execute: async (sql, hints) => {
  const ctx = await getExecContext(args)
  const result = await execSql(ctx, sql, { hints })

Every other command in src/commands/ resolves the context once and passes it down (table.ts, schema.ts, workspace.ts, job.ts, sql.ts, status.ts, profile.ts — all const ctx = await getExecContext(argv) at the top of the handler). Here it is inside the per-statement closure, so each call re-runs:

• resolveConnectionConfig(args) — re-reads and re-parses profiles.toml, re-reads the env layers
• patchProfileUserId(...) — readFileSync(profilesFile()) + parseTOML on every call (it early-returns before writing once user_id is set, but the synchronous read and parse happen regardless)
• getCookieToken(config) ?? getToken(config) — cached in memory, so usually cheap, but the cache lookup is behind both of the above

fs is the first command that issues many statements per invocation. A fs cp -R over N files runs roughly 2N+ statements (get_file + get_presigned_url per file, plus list_directory and create_directory), so a 500-file directory copy means ~1000 synchronous TOML reads and parses interleaved with the transfers.

Smaller correct change: resolve ctx once in createFs (or lazily memoize the first resolution) and reuse it in the closure — the config cannot change mid-command anyway.

[github-actions]

LOW — JSON.parse on server payloads is unguarded, so a non-JSON response escapes as a raw SyntaxError instead of an FS_* code. Confidence: high.

const value = typeof raw === "string" ? JSON.parse(raw) as Record<string, unknown> : raw as Record<string, unknown>

Same pattern at line 356 (children) and line 427 (mkdirs). Every other failure in this file is mapped to an FsError with a stable code, but if get_file / list_directory / create_directory ever return a plain string (an error string, a truncated payload, a future schema change), the SyntaxError propagates past reportFsError's FsError branch into classifyExecError, and the user gets EXEC_ERROR: Unexpected token 'x', ... is not valid JSON with no indication of which path or which call produced it.

Wrapping these in a small helper that throws FS_INTERNAL_ERROR (or FS_TRANSFER_FAILED) with the path and the offending value keeps the documented error contract intact. Note line 428 already guards !value for the empty-rows case, so only the parse itself is uncovered.

[github-actions]

LOW — new Date(...).toISOString() on an unvalidated server timestamp can throw, and a missing timestamp is rendered as a real date. Confidence: medium.

modified_at: new Date(entry.modificationTime).toISOString()

parseModificationTime (fsutil.ts:206) returns the value verbatim when typeof value === "number" and 0 otherwise, so modificationTime is whatever unit the server used with no range check. Two outcomes:

• A missing or unparseable mtime becomes 0 → "1970-01-01T00:00:00.000Z", presented as a genuine modification time. Volume roots and directory rows without an mtime hit this. null would be more honest.
• A value outside the ±8.64e15 ms Date range (e.g. an mtime in nanoseconds) makes toISOString() throw RangeError: Invalid time value. It is inside the try, so it does not crash — but it surfaces as EXEC_ERROR: Invalid time value from classifyExecError, which tells the user nothing about the listing that failed.

Same expression at line 150 in rm --dry-run. A guard in parseModificationTime (reject non-finite and out-of-range, return undefined) plus emitting null for a missing timestamp handles both at the one place the value is produced.

[github-actions]

LOW — this root re-export is redundant with the new ./fsutil subpath, and it makes Node builtins eager in the SDK entrypoint. Confidence: high.

export * from "./fsutil.js"

package.json gains "./fsutil": "./src/fsutil.ts" in the same PR, and the only consumer (packages/cz-cli/src/commands/fs.ts:3) imports from @clickzetta/sdk/fsutil. So this line is not load-bearing today.

What it changes: fsutil.ts statically imports node:fs/promises, node:fs, node:stream/promises, node:stream, node:path and node:crypto at the top of the module. Re-exporting it from the barrel pulls all six into anyone who does import { ... } from "@clickzetta/sdk". The neighbouring sql/volume.ts deliberately avoids that — it does await import("node:fs/promises") inside functions and documents why: "This module requires Node.js. Browser environments do not support local filesystem I/O (fs/promises, path, os) and will throw NotSupportedError when genVolumeResult is called." That design gives a runtime error at the one call site; a static import in the barrel gives an import-time failure for the whole SDK.

There is no browser or edge consumer in-repo, so nothing breaks right now — which is why this is LOW. Dropping the line keeps the subpath as the single entry for the filesystem surface and preserves the invariant volume.ts was written to.

Also worth noting for the public API surface: this line adds FsUtil, FsError, FileInfo, FsUtilOptions and createFsUtil to the root namespace. I checked for collisions with the existing export * lines and found none.

[github-actions]

LOW — the SDK's overwrite default disagrees with mv's default and with the documented CLI contract. Confidence: high.

async cp(source: string, destination: string, recurse = false, overwrite = false) {
  await this.copyBytes(source, destination, recurse, overwrite)
  return true
}
async copyBytes(source: string, destination: string, recurse = false, overwrite = false, progress?: CopyProgress) {

cp/copyBytes default to overwrite = false, but mv (line 599) defaults to true, and the spec's §3.4 states the CLI contract is overwrite=true for both. The CLI is unaffected because commands/fs.ts always passes args.overwrite explicitly, so this is only a hazard for a direct SDK caller: fsUtil.cp(a, b) protects the destination while fsUtil.mv(a, b) replaces it, which is the opposite of what the documented command surface implies.

Aligning both to true (or requiring the argument) removes the trap. If false is the intended SDK-level default, a one-line comment saying so would keep it from being read as a slip.

[github-actions]

LOW — rm resolves the path and fetches its metadata twice. Confidence: high.

async rm(path: string, recurse = false) {
  await this.validateRemoval(path, recurse)
  await this.path(path).remove(recurse)
  return true
}

validateRemoval builds an FsPath and calls info(); then this.path(path) builds a second FsPath and remove() calls info() again (line 456). On a Volume that is two select get_file(...) round-trips per fs rm, and it opens a small TOCTOU window where the type checked is not the type deleted.

validateRemoval already returns the FileInfo, so passing the path object through would fix both:

async rm(path: string, recurse = false) {
  const target = this.path(path)
  await this.validateRemoval(path, recurse)   // or inline the checks on `target`
  await target.remove(recurse)
  return true
}

remove() re-checking isRoot/isDir is fine as a defence for direct FsPath users; the duplication is only in FsUtil.rm, which is the path the CLI takes. Note commands/fs.ts calls validateRemoval separately for --dry-run, so it wants to stay public.

[github-actions]

LOW — read allocates the full maxBytes up front regardless of file size. Confidence: high.

const buffer = new Uint8Array(maxBytes)

commands/fs.ts:56 caps --bytes at 16 MiB and passes args.bytes + 1, so cz-cli fs head ./tiny.txt --bytes 16777216 allocates 16 MiB to read a 5-byte file, then buffer.slice(0, offset) copies the used prefix into a second allocation. The cap keeps this bounded, hence LOW.

stating first and allocating Math.min(maxBytes, size + 1) avoids both — and VolumeFsPath.read (line 393) already takes the accumulate-chunks approach, so the two implementations differ here for no reason.

[github-actions]

Question — intent check, not a bug claim: fs is added to KNOWN_TOP_COMMANDS but not to PROFILE_REQUIRED_COMMANDS. Was that deliberate?

export const KNOWN_TOP_COMMANDS = [..., "dqc", "mcp", "fs"]

PROFILE_REQUIRED_COMMANDS in src/run-cli.ts:30 gates the connecting commands (sql, schema, table, workspace, status, task, runs, attempts, job, datasource, analytics-agent, workspace-param) so that a profile-free machine gets the NO_PROFILE payload with the cz-cli auth login <name> onboarding steps and register URLs.

fs connects for any volume:// path but is not in that set. I think leaving it out is the right call — fs cp ./a ./b on local paths must work with no profile at all, and the fs-command.test.ts cases depend on that. The consequence worth confirming is the Volume path: with no profile configured, cz-cli fs ls volume://v/ reaches getExecContext, throws "Authentication required...", and classifyExecError renders NO_CREDENTIALS — a different code and no register URLs or next_steps, compared with the NO_PROFILE an agent gets from cz-cli sql. agent-system-prompt.ts:182 instructs the agent specifically on NO_PROFILE, so a fresh install hitting a Volume path via fs gets a code the prompt does not cover.

If that is the intended trade, no change needed. If not, the alternative is gating per-path rather than per-command (emit NO_PROFILE once a volume:/czfs: argument is present and no profile exists), which keeps local operation profile-free.

[github-actions]

Review summary

A. Upstream invasiveness — no issues found

No files under packages/opencode, packages/tui or packages/core are touched. All 12 changed files live in packages/clickzetta-sdk, packages/cz-cli and openspec/, so the de-opencode invariant holds and no banner or UPSTREAM-PATCHES.md INTRUSIVE entry is required. Nothing in the change would have needed a hook.

B. Clean fix vs. hole drilled around the problem — see inline

The command layer, the yargs wiring and the FsPath abstraction are the right shape for this feature. Three places treat a symptom rather than its cause:

• copyBytes only creates the destination directory when there is at least one file to copy, and two callers then grow their own empty-directory workarounds instead of that being fixed once — one of them loses data (see the fsutil.ts:531 and fsutil.ts:605 comments).
• quoteString is a second, weaker copy of sql/literal.ts's escape, which already handles backslashes for this dialect.
• The three presigned-URL transfers bypass sql/volume.ts's executeVolumeTransferWithRetry, so the new path has no retry where the pre-existing PUT/GET path does.

No dead code, leftover debug logging or unrelated drive-by edits. The cz_change: comments in this diff are all inside packages/cz-cli, which is correct.

C. Regression risk

The change is almost entirely additive; no tests are deleted, skipped or loosened. Surfaces that could affect existing behavior:

Change	Risk	Covered by a test?
export * from "./fsutil.js" in the SDK root barrel (index.ts:23)	5 new names in the root namespace (no collisions — checked) and Node builtins become eager imports at the SDK entrypoint, which sql/volume.ts deliberately avoids	No
New "./fsutil" subpath in clickzetta-sdk/package.json	Additive export map entry	Exercised indirectly by commands/fs.ts
fs added to KNOWN_TOP_COMMANDS (cli.ts:40)	Affects the top-level "did you mean" matcher only	core-cases.ts --help case updated
fs not added to PROFILE_REQUIRED_COMMANDS	Volume paths on a profile-free machine report NO_CREDENTIALS rather than the NO_PROFILE onboarding payload other connecting commands emit — raised as a separate question, may well be intended	No
10 lines added to CLICKZETTA_AGENT_SYSTEM_PROMPT	Every agent turn now carries the fs surface; the agent will start reaching for these commands	No
guide-builder.ts static registry entries	Additive; registerStaticCommands has no other callers	No

No existing exported function signature changed, and Grep found no callers of a modified function that were missed — registerFsCommand and everything in fsutil.ts are new.

Two output-shape notes for anything that parses this, both inline: fs ls on a local path never emits "type":"directory" rows while the same command on a Volume path does, and a missing server timestamp is rendered as 1970-01-01T00:00:00.000Z rather than null.

D. Correctness

Highest-severity items, all inline: the backslash gap in quoteString (HIGH), the local→Volume empty-directory move that deletes the source and writes nothing (HIGH), and the local→local directory move that replaces the destination directory instead of merging into it (MEDIUM). Path quoting is otherwise sound — identifiers are backtick-escaped and validated, validateRelativePath rejects traversal and control characters, no shell is spawned, and no credential or presigned URL reaches an error message or the output payload.

I could not run the tests, so nothing here is a claim that anything passes or fails.

🤖 Generated with Claude Code

[github-actions]

MEDIUM (confidence: high) — czfs:/Volumes/@user/... paths build syntactically invalid SQL.

function volumeIdentifier(reference: VolumeReference): string {
  const prefix = reference.kind === "table" ? "table volume" : reference.kind === "user" ? "user volume" : "volume"
  if (reference.kind === "user" && reference.identifiers.length === 0) return prefix
  return `${prefix} ${reference.identifiers.map(quoteIdentifier).join(".")}`
}

The identifiers.length === 0 early return is the only path that yields a bare user volume, and only volume:user://~/... reaches it (parseVolumePath line 84 returns identifiers: []). The czfs branch does not: for czfs:/Volumes/@user/demo/alice/a.csv, lines 113-116 set kind: "user" with count = 2, so identifiers = ["demo", "alice"]. volumeIdentifier then falls through to the last line and emits:

select get_file(user volume `demo`.`alice`, 'a.csv')

which is not valid Volume syntax, so every operation on that path form fails with a raw server error surfaced as FS_TRANSFER_FAILED.

The spec added in this PR lists that form as supported — openspec/specs/cz-cli-fs-command/spec.md line 59 (czfs User | czfs:/Volumes/@user/workspace/user/path) and line 77 (czfs:/Volumes/@user/workspace/user/data.txt | 原样规范化 | 支持). No test covers a czfs @user path; fsutil.test.ts only exercises volume:user:// indirectly and never constructs a czfs user path.

If user volumes genuinely have no addressable workspace/user identifiers in SQL, the @user czfs form should either be rejected in parseVolumePath or normalized to identifiers: [] there, rather than parsed into identifiers that no SQL builder can use.

[github-actions]

MEDIUM (confidence: high) — mv <directory> <existing-file> silently deletes the file. cp guards this case; the local→local mv path does not.

copyBytes has the guard (line 590):

else if (sourceInfo.isDir) throw new FsError("FS_TARGET_EXISTS", `Target is a file: ${destination}`)

moveLocal never reaches it, because it calls copyBytes with temporary as the destination, not target. Trace fs mv ./data ./notes.txt where notes.txt is an existing file and --overwrite is on by default:

1. destinationLooksLikeDirectory is false, so the check on line 703 short-circuits.
2. destinationInfo.isDir is false → target = to (the file).
3. info.isDir && targetInfo?.isDir is false → falls through to the temp/backup branch.
4. Line 733's !overwrite && targetExists does not fire (overwrite defaults to true).
5. copyBytes(from, temporary, …) copies the directory to notes.txt.cz-tmp-<uuid>.
6. rename(target, backup) moves notes.txt aside, so rename(temporary, target) succeeds — a directory now sits where the file was, which rename would otherwise reject with ENOTDIR.
7. Line 766 removeFile(backup, …) deletes the original file.

Net result: notes.txt is gone and replaced by a directory, with exit 0. POSIX mv refuses this (cannot overwrite non-directory), and this PR's own cp refuses it.

The smallest correct fix is to apply the same guard before the temp/backup dance — something like if (info.isDir && targetInfo && !targetInfo.isDir) throw new FsError("FS_TARGET_EXISTS", …) alongside the line 733 check. No test covers a directory-over-file move.

[github-actions]

MEDIUM (confidence: high) — recursive copy drops empty subdirectories, and because mv deletes the source afterwards, mv -R loses them permanently.

const children = await from.children(recurse)
const files: FsPath[] = []
for (const child of children) if (!(await child.info()).isDir) files.push(child)

Directories are filtered out of files and never recreated at the destination. Only the copy root gets to.mkdirs() (line 599); every other directory exists at the destination solely as a side effect of LocalFsPath.write's mkdir(dirname(path), { recursive: true }) (line 314) when a file lands in it. A subdirectory containing no files therefore never appears.

For cp this is stated intent — openspec/specs/cz-cli-fs-command/spec.md line 405: 加 -R 时只为实际文件创建所需的目录结构，空目录仍不复制. For mv it is silent structure loss, and the spec's mv section says nothing about it:

• fs mv ./src ./dst -R where ./src/logs/ is empty → copyBytes omits logs/, then from.remove(true) at line 682 (or line 753 on the local→local path) deletes ./src including logs/. The directory is gone from both sides.

The existing coverage does not catch it. fsutil.test.ts:81-85 copies an empty root (which to.mkdirs() handles), and fs-command.test.ts:81-86 moves an empty root. Neither has an empty directory nested inside a non-empty tree.

Smallest correct change: in the sourceInfo.isDir branch, also mkdirs() the directory children (they are already in children), or at minimum make mv refuse to delete the source when the copy omitted directory entries. For a Volume destination this means an extra create_directory per empty directory, which VolumeFsPath.mkdirs already implements.

[github-actions]

MEDIUM (confidence: high) — every Volume upload buffers the whole file in memory, so fs cp of a large file allocates its full size in RAM.

const data = content instanceof Uint8Array ? content : await collectBytes(content)
const body = new Blob([data as unknown as BlobPart])

collectBytes (line 204) accumulates every chunk and then allocates a single Uint8Array of the total, so peak usage is roughly 2× the file size before the Blob is even built. This applies to both directions that end in a Volume write: LocalFsPath.copyTo (line 325) and VolumeFsPath.copyTo (line 513) both hand write an AsyncIterable, and both get fully drained here.

The signature suggests streaming was the intent and did not land: contentLength is threaded in as a parameter, duplex: "half" is passed on line 475 (meaningless with a Blob body), and iterableToReadableStream at line 188 — the function that would produce a streaming body — is never called.

The spec added in this PR anticipates large transfers: openspec/specs/cz-cli-fs-command/spec.md line 386 recommends nohup ... & for 大文件或目录传输. A multi-GB parquet upload is exactly the case that OOMs here.

I recognise the constraint that forced this: executeVolumeTransferWithRetry re-invokes the handler, and an AsyncIterable cannot be replayed, so buffering is what makes retry work. The streaming-compatible shape is to pass a factory that re-opens the source per attempt (createReadStream for local, a fresh presigned GET for Volume→Volume) and build the request body from iterableToReadableStream inside the handler, rather than materialising the bytes outside it.

[github-actions]

MEDIUM (confidence: high) — the retry boundary encloses the destination write, which is not idempotent. A transient read failure during a Volume→local download reports FS_TARGET_EXISTS and leaves a truncated file.

await executeVolumeTransferWithRetry("GET", this.original, async () => {
  const response = await fetch(await this.url("GET"))
  ...
  await target.write(stream, overwrite, info.size)
  bytes = attemptBytes
})

target.write is inside the retried handler, so attempt N+1 re-runs it against whatever attempt N left behind:

• With --no-overwrite, LocalFsPath.write line 313 checks !overwrite && await this.exists(). Attempt 1 creates the file via createWriteStream and then fails mid-stream (e.g. connection reset by peer, which isRetryableVolumeError classifies as retryable on the message). Attempt 2 sees its own partial file and throws FS_TARGET_EXISTS: Target already exists. The user is told the destination already existed when it did not — the tool created it — and a truncated file is left on disk.
• With the default --overwrite, a final failure after all 4 attempts still leaves the truncated file in place with no cleanup. createWriteStream truncates on open, so if the destination previously held a good copy, that copy is destroyed by a failed transfer.

This is also inconsistent with the care taken on the local→local path, where moveLocal builds a .cz-tmp-<uuid> file specifically so a failure never leaves a half-written destination.

Two options, either of which fixes it: keep the retry around the GET only and write once the body is in hand, or have volume→local writes go to a sibling temp path and rename into place on success — the same pattern moveLocal already uses.

[github-actions]

MEDIUM (confidence: medium) — the "not found" classifier is a substring match on the server message, and fs rm --force turns a misclassification into a false SUCCEEDED.

if (/not found|does not exist|unknown volume/i.test(message)) throw new FsError("FS_NOT_FOUND", message)
throw new FsError("FS_TRANSFER_FAILED", message)

Any failed Volume statement whose message happens to contain one of those phrases becomes FS_NOT_FOUND — for example a permission error phrased as ... user does not exist in role ..., or a schema/table resolution error on a table volume. The consequence is not just a wrong code:

packages/cz-cli/src/commands/fs.ts:153-155 swallows exactly that code when --force is set:

try { await fs.rm(args.path, args.recursive) }
catch (err) { if (!(args.force && err instanceof FsError && err.code === "FS_NOT_FOUND")) throw err }
success({ path: args.path, operation: "REMOVE", status: "SUCCEEDED" }, ...)

So cz-cli fs rm <path> --force prints status: "SUCCEEDED", exit 0, for a path that was not deleted and that the server refused for an unrelated reason. An automation loop or the agent that this PR points at fs rm (see the agent-system-prompt.ts entry) would treat the cleanup as done.

QueryResult already carries errorCode (packages/clickzetta-sdk/src/sql/types.ts:29) — keying off that, with the regex as a fallback, would make the classification deterministic. Failing that, --force should only swallow not-found for the paths where the code was derived from a structured signal, not from prose.

Also note this branch treats only status === "FAILED" as an error, so a CANCELLED job returns rows: [] and is reported as a successful empty result.

[github-actions]

LOW (confidence: medium) — content-length is set from the source's metadata size, which can disagree with the body actually being sent.

headers["content-length"] = String(contentLength ?? data.byteLength)

data is already fully materialised on line 466, so its length is authoritative; contentLength is not. The callers pass a separately-obtained size:

• LocalFsPath.copyTo (line 325) passes info.size from a stat taken before the read — a file appended to or truncated in between produces a mismatch.
• VolumeFsPath.copyTo (line 513) passes info.size from the get_file metadata row, which can be stale relative to the object actually returned by the presigned GET.

A content-length that contradicts the Blob body is either rejected by the storage backend or overridden by the fetch implementation; neither is a behaviour worth depending on. Since the bytes are in hand at this point, String(data.byteLength) is both simpler and always right, and the contentLength parameter can go away (or stay only if a future streaming body needs it).

[github-actions]

LOW (confidence: high) — dead code: iterableToReadableStream is defined but never called anywhere in the repo.

function iterableToReadableStream(content: AsyncIterable<Uint8Array>): ReadableStream<Uint8Array> {

rg iterableToReadableStream packages/ returns only this definition. VolumeFsPath.write collects the iterable into a Uint8Array and wraps it in a Blob instead, and LocalFsPath.write uses contentToReadable.

It reads as the leftover of the streaming-upload path that did not land (same story as duplex: "half" on line 475). Either wire it up as part of streaming the PUT body, or drop it — leaving it in place suggests to the next reader that uploads stream when they do not.

[github-actions]

LOW (confidence: high) — the copy-into-itself guard hardcodes /, so it does not fire for local paths on Windows.

function isDescendantPath(source: FsPath, target: FsPath): boolean {
  return source.scope === target.scope && source.scopePath !== "" && target.scopePath.startsWith(source.scopePath + "/")
}

For LocalFsPath, scopePath is the value of resolvePath() (line 265), which uses the platform separator — C:\data and C:\data\backup on Windows. startsWith(source.scopePath + "/") never matches, so fs cp -R C:\data C:\data\backup gets past the line 593 check.

It does not hang — copyBytes snapshots children before to.mkdirs(), so the copy terminates — but it writes the source tree into a subdirectory of itself, which the guard exists to prevent. VolumeFsPath is unaffected since its scopePath is always the /-joined relative path.

Normalising the local separator before comparing (scopePath.split(sep).join("/"), sep is already imported on line 5) would make the guard platform-independent. fsutil.test.ts:108 covers this case, but only on POSIX.

[github-actions]

MEDIUM (confidence: high on the cost, asking about intent) — local→local mv performs a full byte copy where rename would do, turning an O(1) operation into O(size) plus 2× peak disk usage.

const temporary = this.path(`${target.scopePath}.cz-tmp-${randomUUID()}`)
const backup = this.path(`${target.scopePath}.cz-backup-${randomUUID()}`)
let backupMoved = false
try {
  await this.copyBytes(from.original, temporary.original, recurse, true)

rename is already imported and used two lines below for temp→target and target→backup, so the primitive is right here. mv ./big.parquet ./archive/big.parquet on one filesystem currently reads and rewrites the whole file, then deletes the source; a 50 GB file needs 50 GB of free space and a full copy's worth of time for what the kernel can do as a directory update. Same for a directory rename, which deep-copies the entire tree.

I see this is prescribed by the spec added in this PR (openspec/specs/cz-cli-fs-command/spec.md, mv behaviour: 目标同级创建唯一临时路径，完整复制成功后再替换目标，最后删除源), and I understand the guarantee being bought — 任一步骤失败都不得删除源. Worth noting that rename(2) gives that guarantee for free and atomically: it either replaces the target or leaves both sides untouched, with no window where the source is gone and the destination is incomplete. The standard shape is rename first, and fall back to copy-then-delete only on EXDEV (cross-device), which is the one case where a rename genuinely cannot work.

Could you confirm whether the full-copy path is a deliberate trade (and if so, what rename fails to provide here), or whether an EXDEV fallback would be an acceptable change? If it stays, the doubled disk requirement is worth documenting in the spec's mv section, since it is invisible to the user until a move fails on a full disk.

[github-actions]

LOW (confidence: medium) — the connection config is resolved eagerly, so purely-local operations can fail on an unrelated profile problem.

function createFs(args: FsArgs): FsUtil {
  const config = resolveConnectionConfig(args)
  let context: ReturnType<typeof getExecContext> | undefined

The context ??= getExecContext(args) deferral below is exactly right — fs cp /tmp/a /tmp/b never authenticates and never opens a session. But resolveConnectionConfig runs unconditionally on every fs invocation, and it has two throw paths (packages/cz-cli/src/connection/config.ts:47 and :106): PROFILE_NOT_FOUND when --profile names a missing entry, and INVALID_AUTH_TYPE for a malformed profile. Either one fails a local-only fs ls / fs cp / fs rm that had no need for a profile at all.

workspace/schema are only consumed by FsUtil.path for short Volume paths (fsutil.ts:542-548), which already raises FS_PATH_CONTEXT_REQUIRED when they are missing — so the config could be resolved lazily alongside getExecContext, or read through a try-free accessor, without changing the Volume behaviour.

Low impact since a broken profile is unusual, but the local half of this command surface is otherwise connection-free and it would be nice if it stayed that way.

[github-actions]

Reviewed the full fs surface: packages/clickzetta-sdk/src/fsutil.ts, packages/cz-cli/src/commands/fs.ts, the registration/guide/prompt wiring, both new test files, and the spec. 11 inline findings, most severe first.

A. Upstream invasiveness — no issues found

No file under packages/opencode, packages/tui, or packages/core is touched. All 11 changed files are in packages/cz-cli/, packages/clickzetta-sdk/, and openspec/. Nothing here needs a banner or a new UPSTREAM-PATCHES.md INTRUSIVE entry, and no hook dependency is added that the HOOK section would have to track on a re-baseline.

One thing done right and worth recording: fsutil.ts statically imports node:fs/promises, node:stream, and node:crypto, and it is exposed via a new ./fsutil subpath in packages/clickzetta-sdk/package.json rather than re-exported from src/index.ts. That keeps the SDK main entry free of unconditional Node built-ins, consistent with the note at the top of src/sql/volume.ts about browser environments. Adding it to index.ts would have broken that; it was not.

B. Clean fix vs. hole drilled around the problem — issues found

The command layer is a thin, correct shell over the SDK, and the SDK is a single shared implementation rather than per-caller special cases. No new flag or env var routes around a bug, nothing is copy-pasted into a second place, and no try/catch hides a failure that should surface. The --force/--dry-run handling in fs rm is the one place errors are swallowed and it is deliberate and narrow.

What I did flag:

• iterableToReadableStream is dead code (never called anywhere in the repo) — part of a larger pattern where the streaming-upload path did not land: duplex: "half" is passed with a Blob body, and a contentLength parameter is threaded through to a code path that already knows the exact byte count. The consequence is that every Volume upload buffers the whole file in memory, which matters because the spec itself anticipates large transfers (the nohup ... & guidance for large file or directory transfers).
• The retry boundary in VolumeFsPath.copyTo encloses the destination write, which is not idempotent — a retried Volume-to-local download hits FS_TARGET_EXISTS against its own partial file, and a final failure leaves a truncated destination with no cleanup. Inconsistent with the temp-file care taken on the local-to-local path.
• Local-to-local mv does a full byte copy where rename would do. This one is prescribed by the spec added in this PR, so I raised it as a question about intent rather than a defect — rename(2) already provides the "never delete the source on failure" guarantee atomically, with EXDEV as the only case needing a copy fallback.
• content-length is set from source metadata rather than the buffer actually being sent, so a size that changed between stat/get_file and the transfer produces a header that contradicts the body.

C. Regression risk

Nothing existing changes behaviour — this is additive. No test was deleted, skipped, or loosened; the only edit to an existing test is core-cases.ts:8, which adds "fs" to the --help expectCommands list. Enumerated surface:

Change	Risk	Coverage
KNOWN_TOP_COMMANDS += "fs" (cli.ts:40)	None. Only feeds the did-you-mean matcher at cli.ts:306; it does not gate dispatch.	—
registerFsCommand(cli) (register-commands.ts:47)	New top-level fs group. No existing command or alias is named fs; -R, -c, -f, --limit, --bytes, --overwrite, --dry-run are all subcommand-local and none collides with KNOWN_GLOBAL_FLAGS.	8 new e2e-help cases
New ./fsutil subpath export	Additive; "." is unchanged.	resolves at runtime (the 3 CLI fs tests exercise it)
8 new guide-builder entries + 10 lines in agent-system-prompt.ts	Both feed the agent-facing guide. buildAiGuide has a DEFAULT_BUDGET_CHARS = 40000 budget whose overflow path drops option help globally, so a large enough registry silently degrades the guide for other commands too. I could not measure whether this crosses the threshold — worth checking buildAiGuide().truncation before merge.	no test asserts guide size or that the registry matches the registered commands
New output shapes (entries[], truncated, dry_run, bytes, status)	New payloads, so nothing existing parses them. error() places details as a sibling of error in the payload (output/index.ts:130), not nested inside it — intended, but worth confirming it is the shape you want for PARTIAL_FAILED consumers.	fs-command.test.ts asserts data.entries, data.content, truncated, and error.code

Paths with a behaviour change but no test, beyond the inline findings: czfs:/Volumes/@user/... (spec says supported, generates invalid SQL), directory-over-file mv, empty nested directories under cp -R / mv -R, --limit interaction with recursive local listings, and every PARTIAL_FAILED branch (details.stage / completed / pending is constructed in six places and asserted in none).

I could not run the test suite, so nothing above is a claim that anything passes or fails — only that the code paths are or are not reached by an assertion.

Checked and cleared

• SQL injection in the generated statements: clean. Identifiers go through quoteIdentifier (backtick-doubling) after validateIdentifiers rejects ., /, and control characters; path literals go through the quote/escape helpers in sql/literal.ts, and fsutil.test.ts:129-141 pins the backslash case. No user value is interpolated raw.
• Credentials in telemetry/logs: fs positionals are paths, not secrets. Presigned URLs stay out of the error messages (httpTransferError formats this.original, not the URL).
• process.exit(): none added. The --format text path in fs head sets process.exitCode and returns, which is correct.
• Cross-package dependency edges: none new. cz-cli already depends on @clickzetta/sdk; this adds a subpath of the same package.
• JobStatus string-enum comparison: result.status === "FAILED" works, since JobStatus is a string enum (sql/types.ts:7-14).
• FS_NOT_TEXT on a multibyte truncation boundary is documented intent (the spec section on head, aligned with the Python connector behaviour), so I did not flag it.

These are suggestions — take or leave any of them.