fix(fs): close volume review follow-ups - #87
Conversation
| // Route the legacy User Volume root through the same resolver as czfs:/Volumes/@user | ||
| // so both spellings report identical czfs entry paths, matching how volume:table:// | ||
| // already normalizes to czfs output. | ||
| if ((hasCzfsScheme && czfsRoot === "/volumes/@user") || normalized === "volume:user://~") { | ||
| return this.listVolumeWorkspaceRoots("user", limit) | ||
| if (hasCzfsScheme && czfsRoot === "/volumes/@user") return this.listVolumeWorkspaceRoots("user", limit) | ||
| if (normalized === "volume:user://~") { | ||
| return this.listCurrentUserVolumeFiles(recursive, limit) | ||
| } |
There was a problem hiding this comment.
MEDIUM (confidence: high) — the comment now contradicts the code directly beneath it.
// Route the legacy User Volume root through the same resolver as czfs:/Volumes/@user
// so both spellings report identical czfs entry paths, matching how volume:table://
// already normalizes to czfs output.
if (hasCzfsScheme && czfsRoot === "/volumes/@user") return this.listVolumeWorkspaceRoots("user", limit)
if (normalized === "volume:user://~") {
return this.listCurrentUserVolumeFiles(recursive, limit)
}This comment was the written justification for routing volume:user://~ through listVolumeWorkspaceRoots — the exact behavior this PR is reverting. Left in place, it now asserts the opposite of what the code does: the two spellings deliberately no longer share a resolver, and they no longer report identical entries (czfs:/Volumes/@user → workspace roots; volume:user://~ → files under the current workspace/user).
The next reader — or the next person re-deriving this decision — will read the comment as the intent and re-collapse the branches, reintroducing the compatibility break this PR exists to fix. Worth replacing it with the reason for the split, e.g. that volume:user://~/ is the legacy spelling and must keep listing files, while czfs:/Volumes/@user is the namespace root that lists workspaces.
| if (normalized === "volume:user://~") { | ||
| return this.listCurrentUserVolumeFiles(recursive, limit) | ||
| } |
There was a problem hiding this comment.
LOW (confidence: high) — please confirm intent: volume:user://~/ now fails without a configured workspace, where it previously always succeeded.
if (normalized === "volume:user://~") {
return this.listCurrentUserVolumeFiles(recursive, limit)
}listCurrentUserVolumeFiles(recursive, limit) passes workspace: undefined, so resolveUserVolumeReference falls back to this.workspace (fsutil.ts:1010-1011) and throws when it is unset:
if (!workspace) throw new FsError("FS_PATH_CONTEXT_REQUIRED", "Workspace is required for User Volume root")The old target, listVolumeWorkspaceRoots("user", limit), only ran SHOW WORKSPACES and needed no workspace context. createFs in packages/cz-cli/src/commands/fs.ts:206-211 passes config.workspace straight through, which is undefined for a profile without one — so cz-cli fs ls volume:user://~/ goes from listing workspaces to FS_PATH_CONTEXT_REQUIRED (exit 2, since reportFsError classifies FS_PATH_CONTEXT_REQUIRED as a usage error).
If that is the intended legacy behavior being restored, no change needed. If it is, it is worth a test: fsutil.test.ts only covers the case where workspace: "workspace" is set, so nothing pins the no-workspace path either way. The spec line you updated (spec.md:130) does now say 当前 workspace, which reads like this is deliberate — just flagging that it is a new error path for existing invocations.
| // User namespace roots list workspaces; files require an explicit workspace. | ||
| expect((await fs.ls("volume:user://~/"))[0]).toMatchObject({ name: "workspace", isDir: true, path: "czfs:/Volumes/@user/workspace" }) | ||
| expect((await fs.ls("volume:user://~/"))[0]).toMatchObject({ name: "uploads", isDir: true, path: "czfs:/Volumes/@user/workspace/alice/uploads" }) |
There was a problem hiding this comment.
LOW (confidence: high) — the comment above the changed assertion now describes the old behavior.
// User namespace roots list workspaces; files require an explicit workspace.
expect((await fs.ls("volume:user://~/"))[0]).toMatchObject({ name: "uploads", isDir: true, path: "czfs:/Volumes/@user/workspace/alice/uploads" })The assertion right below now proves the opposite of the second clause: volume:user://~/ returns files, and it does so with no explicit workspace in the path (it comes from FsUtil's constructor context). The comment is still accurate for czfs:/Volumes/@user on line 501, so it reads as if it governs both — but the two lines now diverge, which is the whole point of the change.
Suggest splitting it so each line carries its own note, e.g. // The legacy volume:user://~ spelling keeps listing files under the current workspace/user. here, and leaving the existing comment with line 501.
| options: [{ flags: "--using", required: false, takes_value: true, help: "Input format: csv, parquet, orc, bson (default csv)" }, { flags: "--header", required: false, takes_value: false, help: "Treat the first CSV row as column names" }], | ||
| examples: [{ cmd: "cz-cli table load your_table czfs:/Volumes/your_workspace/your_schema/your_volume/ --header", desc: "Load CSV and skip its header" }, { cmd: "cz-cli table load your_table czfs:/Volumes/your_workspace/your_schema/your_volume/daily/ --using parquet", desc: "Load a directory" }, { cmd: "cz-cli table load your_table czfs:/Volumes/@user/your_workspace/your_user/data.csv", desc: "Load a User Volume file" }, { cmd: "cz-cli table load your_table czfs:/Volumes/@table/your_workspace/your_schema/source_table/exports/ --using parquet", desc: "Load a Table Volume directory" }] }, | ||
| examples: [{ cmd: "cz-cli table load your_table czfs:/Volumes/your_workspace/your_schema/your_volume/ --header --write", desc: "Load CSV and skip its header" }, { cmd: "cz-cli table load your_table czfs:/Volumes/your_workspace/your_schema/your_volume/daily/ --using parquet --write", desc: "Load a directory" }, { cmd: "cz-cli table load your_table czfs:/Volumes/@user/your_workspace/your_user/data.csv --write", desc: "Load a User Volume file" }, { cmd: "cz-cli table load your_table czfs:/Volumes/@table/your_workspace/your_schema/source_table/exports/ --using parquet --write", desc: "Load a Table Volume directory" }] }, |
There was a problem hiding this comment.
MEDIUM (confidence: high) — every example now passes --write, but the machine-readable options array on the line above still omits it.
options: [{ flags: "--using", ... }, { flags: "--header", ... }],
examples: [{ cmd: "... --header --write", ... }, ...],The guide is what agents consume to learn the flag surface. As it stands, table load advertises --using and --header as its only options while all four examples use a flag the schema says does not exist — an agent that validates a command against options before running it will reject its own documented examples, and one that trusts options will omit --write and hit WRITE_NOT_ALLOWED (table.ts:350-352).
The two other write-guarded commands in this same file already declare it, so there is an established shape to copy:
guide-builder.ts:256(sql):{ flags: "--write", required: false, takes_value: false, help: "Allow write operations" }guide-builder.ts:299(fs rm):{ flags: "--write", required: false, takes_value: false, help: "Confirm removal" }
Since table load requires it rather than merely accepting it, required: true with help like "Confirm the load; required as a safety guard" matches the actual handler behavior.
| cz-cli table load your_table czfs:/Volumes/your_workspace/your_schema/your_volume/data.csv --header --write | ||
| cz-cli table load your_table czfs:/Volumes/your_workspace/your_schema/your_volume/daily/ --using parquet --write | ||
| cz-cli table load your_table czfs:/Volumes/@user/your_workspace/your_user/data.csv --write | ||
| cz-cli table load your_table czfs:/Volumes/@table/your_workspace/your_schema/source_table/exports/ --using parquet --write |
There was a problem hiding this comment.
LOW (confidence: high) — the table load examples here gained --write, but the authoritative command table earlier in the same file did not.
spec.md:198 still reads:
| `table load` | `<table> <czfs-source>` | `--using`、`--header` | 仅做追加式 Volume → Table 导入;`COPY OVERWRITE` 和复杂场景使用 SQL |
--write is missing from the 专属参数 column, and the 默认行为 column does not mention the guard. The row two lines up (fs rm, spec.md:197) sets the convention for exactly this case — it lists --write and spells out 实际删除必须显式确认 --write.
Suggested row, mirroring fs rm:
| `table load` | `<table> <czfs-source>` | `--using`、`--header`、`--write` | 仅做追加式 Volume → Table 导入;实际写入必须显式确认 `--write`;`COPY OVERWRITE` 和复杂场景使用 SQL |
Since §3 is the summary a reader scans first, leaving it stale while §4.10 is current means the two halves of the spec disagree about whether the flag is required.
| (y) => y | ||
| .positional("file", { type: "string", demandOption: true, describe: "Local or Volume file path" }) | ||
| .option("bytes", { type: "number", default: 65536, describe: "Maximum bytes to read" }) | ||
| .option("bytes", { alias: "c", type: "number", default: 65536, describe: "Maximum bytes to read" }) |
There was a problem hiding this comment.
MEDIUM (confidence: high) — the -c alias is added to the parser but not to the three places that document the flag surface.
.option("bytes", { alias: "c", type: "number", default: 65536, describe: "Maximum bytes to read" })This changes the rendered help from --bytes to -c, --bytes, and the e2e case in this PR now asserts that. Three companion surfaces were not updated:
openspec/specs/cz-cli-fs-command/spec.md:371— §4.3 is a verbatimfs head --helpsnapshot and still shows--bytes Maximum bytes to read. It is now wrong against the binary. This PR already edits this spec file, so the omission looks like an oversight rather than a scoping decision.openspec/specs/cz-cli-fs-command/spec.md:191and:207— the 专属参数 / 参数语义 tables list--bytesalone.fs lsat:190writes its alias as-R, --recursive, so the convention is to name both spellings.packages/cz-cli/src/guide-builder.ts:278—options: [{ flags: "--bytes", ... }].fs rmat:299writesflags: "-R, --recursive", so the guide convention is also the combined form. Left as-is, an agent reading the guide will not know-cexists.
None of these break anything at runtime; they just mean the alias is real in exactly one place and absent from every document describing it.
| expectHeader: "cz-cli fs head", | ||
| expectOptions: ["file", "--bytes", "czfs:/Volumes/@user/your_workspace/your_user/demo.csv"], | ||
| forbid: ["-c, --bytes"], | ||
| expectOptions: ["file", "-c, --bytes", "czfs:/Volumes/@user/your_workspace/your_user/demo.csv"], |
There was a problem hiding this comment.
LOW (confidence: medium) — please confirm intent: this reverses a rejection that was explicitly asserted, and the reversal is now undocumented.
expectOptions: ["file", "-c, --bytes", "czfs:/Volumes/@user/your_workspace/your_user/demo.csv"],The previous state was not merely "no alias" — it was two positive assertions that -c must not exist: this forbid: ["-c, --bytes"] guard, plus fs-command.test.ts asserting -c returns USAGE_ERROR. A forbid entry is written to stop something from coming back, so someone removed the alias on purpose.
I searched openspec/ and the cz-cli sources and found no recorded rationale for either the removal or this restoration — the forbid line was the only record of the decision, and this PR deletes it. fs head is also the only command in the repo where -c means anything other than --continue (packages/cz-cli/src/commands/agent.ts:53), which may or may not have been the original objection.
Two things would make this durable:
- a sentence in the PR description or a
cz_change:comment atfs.ts:53saying why-cis wanted back (GNUhead -ccompatibility, presumably) and why the earlier objection no longer applies; - the spec update noted separately on
fs.ts:53, so §4.3 records-cas supported rather than leaving the parser as the only source of truth.
No global -c exists on the root parser (KNOWN_GLOBAL_FLAGS in cli.ts:39), so I see no collision — this is about the decision history, not a defect.
| const legacyShortFlag = await execute(`fs head ${quote(file)} -c 2 --format json`) | ||
| expect(legacyShortFlag.exitCode).toBe(2) | ||
| expect(JSON.parse(legacyShortFlag.output).error.code).toBe("USAGE_ERROR") | ||
| expect(legacyShortFlag.exitCode).toBe(1) | ||
| expect(JSON.parse(legacyShortFlag.output).error.code).toBe("FS_NOT_TEXT") |
There was a problem hiding this comment.
LOW (confidence: high) — this assertion is now a duplicate of the one eight lines up and no longer pins what it is named for.
const legacyShortFlag = await execute(`fs head ${quote(file)} -c 2 --format json`)
expect(legacyShortFlag.exitCode).toBe(1)
expect(JSON.parse(legacyShortFlag.output).error.code).toBe("FS_NOT_TEXT")fs-command.test.ts:43-45 already asserts exactly exitCode 1 / FS_NOT_TEXT for --bytes 2 on this same 4-byte a世 file. The -c case now re-tests the UTF-8 truncation path rather than the alias, so the only thing distinguishing it from a broken alias is indirect: if -c were dropped again, bytes would fall back to its 65536 default, the whole file would decode cleanly, and this would fail with exit 0 instead. That works, but it is a coincidence of the fixture rather than an assertion about the alias, and the test name (legacyShortFlag) promises otherwise.
A direct version asserts the alias reached args.bytes on a success path:
const legacyShortFlag = await execute(`fs head ${quote(file)} -c 1 --format json`)
expect(legacyShortFlag.exitCode).toBe(0)
const payload = JSON.parse(legacyShortFlag.output).data
expect(payload.content).toBe("a")
expect(payload.bytes).toBe(1)
expect(payload.truncated).toBe(true)That fails loudly if -c is ignored (content would be the whole file, bytes 4), and it does not overlap the truncation case above. Also worth noting the enclosing test is named "protects filesystem root and rejects invalid UTF-8 truncation" — an alias-acceptance check has drifted into it either way.
Review summary7 inline findings, none blocking. Highest are two MEDIUM doc/schema-sync gaps and a stale comment that argues against the change it now sits above. A. Upstream invasiveness — no issues foundThis PR touches no file under B. Clean fix vs. hole drilled around the problem — the fix is right; one leftoverThe The one leftover is the three-line comment above the split, which still states the rationale for the behavior being reverted — inline. Same pattern in the test comment at The C. Regression riskFour behavior changes, in descending order of blast radius:
No tests deleted or skipped. Two assertions were inverted to match the new behavior, which is correct, but the Exported API surface: unchanged. Doc/schema surfaces that did not keep up — the two MEDIUM findings: I did not run any tests, so nothing above is a claim that anything passes — the coverage notes are from reading the test files. |
Summary
--writefortable loadand reject mismatched User Volume identitiesvolume:user://~/file-listing behavior and restorefs head -cValidation
packages/clickzetta-sdk:bun test test/fsutil.test.ts— 34 passedpackages/clickzetta-sdk:bun typecheck— passedpackages/cz-cli:bun test test/fs-command.test.ts— 3 passedsingsight: cross-workspace qualifiedSHOW TABLES, User Volume identity rejection, legacy User Volume listing, and write guard verifiedKnown baseline
The CLI/full turbo typecheck is still blocked by the pre-existing
packages/opencode/src/bus/global.ts:14TS2416 error.