Skip to content

fix(fs): close volume review follow-ups - #87

Open
hellozepp wants to merge 1 commit into
mainfrom
volume-review-fixes
Open

fix(fs): close volume review follow-ups#87
hellozepp wants to merge 1 commit into
mainfrom
volume-review-fixes

Conversation

@hellozepp

Copy link
Copy Markdown
Collaborator

Summary

  • require --write for table load and reject mismatched User Volume identities
  • fully qualify workspace and schema for cross-workspace Table Volume listings
  • keep the empty Managed Volume workaround root-only so missing subdirectories remain errors
  • preserve legacy volume:user://~/ file-listing behavior and restore fs head -c
  • align help, agent guidance, and examples with the write guard

Validation

  • packages/clickzetta-sdk: bun test test/fsutil.test.ts — 34 passed
  • packages/clickzetta-sdk: bun typecheck — passed
  • packages/cz-cli: bun test test/fs-command.test.ts — 3 passed
  • live singsight: cross-workspace qualified SHOW TABLES, User Volume identity rejection, legacy User Volume listing, and write guard verified

Known baseline

The CLI/full turbo typecheck is still blocked by the pre-existing packages/opencode/src/bus/global.ts:14 TS2416 error.

Comment on lines 949 to 955
// 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)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +953 to 955
if (normalized === "volume:user://~") {
return this.listCurrentUserVolumeFiles(recursive, limit)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 498 to +499
// 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" })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 321 to +322
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" }] },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +641 to +644
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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" })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. openspec/specs/cz-cli-fs-command/spec.md:371 — §4.3 is a verbatim fs head --help snapshot 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.
  2. openspec/specs/cz-cli-fs-command/spec.md:191 and :207 — the 专属参数 / 参数语义 tables list --bytes alone. fs ls at :190 writes its alias as -R, --recursive, so the convention is to name both spellings.
  3. packages/cz-cli/src/guide-builder.ts:278options: [{ flags: "--bytes", ... }]. fs rm at :299 writes flags: "-R, --recursive", so the guide convention is also the combined form. Left as-is, an agent reading the guide will not know -c exists.

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"],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 at fs.ts:53 saying why -c is wanted back (GNU head -c compatibility, presumably) and why the earlier objection no longer applies;
  • the spec update noted separately on fs.ts:53, so §4.3 records -c as 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.

Comment on lines 51 to +53
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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions

Copy link
Copy Markdown
Contributor

Review summary

7 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 found

This PR touches no file under packages/opencode, packages/tui, or packages/core. The nine changed files are openspec/specs/, packages/clickzetta-sdk, and packages/cz-cli only, so the de-opencode invariant is not engaged: no banner is needed and no new UPSTREAM-PATCHES.md INTRUSIVE entry is due. The cz_change:-style comments I read while tracing callers were all inside packages/cz-cli, where the ledger says they are ordinary comments.

B. Clean fix vs. hole drilled around the problem — the fix is right; one leftover

The fsutil.ts change is the correct shape. volume:user://~ and czfs:/Volumes/@user were collapsed onto one resolver on the theory that both spellings should report identical entries; that silently changed what the legacy spelling listed. Splitting the two conditions restores the legacy contract while leaving the czfs namespace root alone, and it is a two-line change at the one dispatch site rather than a special case bolted onto a caller. Nothing here routes around a bug with a new flag, no fallback hides a failure, and I found no copy-pasted logic or dead code.

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 fsutil.test.ts:498inline.

The --write and -c hunks are documentation catching up to code that already shipped; the table load guard at table.ts:350-352 and the fs head parser are the source of truth in both cases. No drive-by edits — all nine files trace to the four bullets in the description.

C. Regression risk

Four behavior changes, in descending order of blast radius:

  1. volume:user://~/ output shape changes — from workspace directory entries (czfs:/Volumes/@user/<ws>) to file entries under the current workspace/user. Anything parsing this listing sees different rows and different paths. This is the PR's stated purpose. Covered by fsutil.test.ts:499.
  2. volume:user://~/ gains a failure mode — it now needs a workspace in context and returns FS_PATH_CONTEXT_REQUIRED (exit 2) without one, where the old resolver ran SHOW WORKSPACES and never needed one. Reachable via a profile with no workspace, since createFs passes config.workspace through unchanged. No test covers the unset-workspace path in either direction — inline.
  3. fs head -c goes from rejected to accepted — previously USAGE_ERROR exit 2. Covered by fs-command.test.ts:51-53 and core-cases.ts:180, both rewritten in this PR. I checked for a short-flag collision and found none: KNOWN_GLOBAL_FLAGS (cli.ts:39) has no c, and the only other -c is --continue on a different command (agent.ts:53). Worth confirming the intent, since the old state was guarded by an explicit forbidinline.
  4. Help output shape changes for fs head (--bytes-c, --bytes) and table load epilogue examples. Covered by the e2e help cases.

No tests deleted or skipped. Two assertions were inverted to match the new behavior, which is correct, but the -c one now duplicates the --bytes 2 case above it and no longer pins the alias directly — inline.

Exported API surface: unchanged. listCurrentUserVolumeFiles and listVolumeWorkspaceRoots are both private; listVirtualRoot is private too, so the change is contained to FsUtil.ls dispatch. I grepped for volume:user across the repo — the only non-test consumers are inside fsutil.ts itself. No new cross-package dependency edges.

Doc/schema surfaces that did not keep up — the two MEDIUM findings: guide-builder.ts:321 omits --write from table load's machine-readable options while all four of its examples now pass it (inline), and the -c alias is absent from the §4.3 help snapshot, the §3/§3.1 tables, and guide-builder.ts:278 (inline). Plus the §3 table load row (inline).

I did not run any tests, so nothing above is a claim that anything passes — the coverage notes are from reading the test files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant