packages/cli/test/csv-parse.test.ts (1)

4-33: Good test coverage for parseCsv.

The tests cover key scenarios including quoted fields, escaped quotes, and CRLF handling.

Consider adding a test for quoted fields containing newlines, as this is a common CSV edge case:

it('handles quoted fields with embedded newlines', () => {
  expect(parseCsv('"line1\nline2",b')).toEqual([['line1\nline2', 'b']]);
});

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cli/test/csv-parse.test.ts` around lines 4 - 33, Add a test to cover
quoted fields that contain embedded newlines for parseCsv: create an it block
that calls parseCsv with a CSV string where a field is quoted and includes a
newline (e.g. "\"line1\nline2\",b") and assert the parsed output contains that
field with the newline preserved (['line1\nline2','b']). Place this new test
alongside the other parseCsv tests so parseCsv is exercised for embedded-newline
quoted fields.

packages/cli/src/util/csv-parse.ts (1)

68-77: Consider logging a warning for invalid cell references.

parseStartRef silently falls back to {row: 1, col: 1} when the input is invalid. While this provides a safe default, users may not realize their --start argument was malformed. Consider returning the fallback with a console warning, or documenting this behavior clearly.

💡 Optional: Add warning for invalid input

 export function parseStartRef(ref: string): { row: number; col: number } {
   const match = ref.match(/^([A-Z]+)(\d+)$/i);
-  if (!match) return { row: 1, col: 1 };
+  if (!match) {
+    console.warn(`Invalid cell reference "${ref}", defaulting to A1`);
+    return { row: 1, col: 1 };
+  }
   const letters = match[1].toUpperCase();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/util/csv-parse.ts` around lines 68 - 77, The parseStartRef
function currently silently returns the default {row: 1, col: 1} for invalid
refs; update parseStartRef to emit a clear warning (e.g., console.warn) when the
input ref fails the regex so users know their --start argument was malformed,
include the original ref value in the message for debugging, and then continue
returning the existing fallback; keep the function name parseStartRef and its
return shape unchanged.

packages/cli/test/config.test.ts (1)

81-125: Consider cleaning up temp directories after tests.

The migration tests create temporary directories in tmpdir() but don't clean them up. While this is typically fine for CI environments that reset between runs, accumulated test artifacts could clutter local development environments over time.

♻️ Proposed cleanup using afterEach

+import { rmSync } from 'node:fs';
+
 describe('migrateConfigIfNeeded', () => {
+  const createdDirs: string[] = [];
+
+  afterEach(() => {
+    for (const dir of createdDirs) {
+      rmSync(dir, { recursive: true, force: true });
+    }
+    createdDirs.length = 0;
+  });
+
   it('copies config from old path to new path when new path does not exist', () => {
     const base = join(tmpdir(), `wfb-migrate-test-${Date.now()}`);
+    createdDirs.push(base);
     // ... rest of test

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cli/test/config.test.ts` around lines 81 - 125, Tests under the
migrateConfigIfNeeded describe block leave temporary directories in the OS
tmpdir; add cleanup by storing each test's base temp directory in a scoped
variable and adding an afterEach hook that recursively removes that base (using
fs.rmSync or fs.rmdirSync with { recursive: true } or try/catch around unlink)
so tests do not leave artifacts; reference the existing migrateConfigIfNeeded
tests and the tmpdir-based base variables (e.g., base, oldDir, newDir) and
ensure the afterEach is tolerant if the directory was never created.

packages/cli/src/commands/ctx.ts (1)

10-14: Truncated IDs may cause visual ambiguity.

The workspace ID is truncated to 8 characters for display (ws.id.slice(0, 8)). If workspace IDs are UUIDs, this is typically sufficient for uniqueness, but the switch command accepts the full ID, exact name, or prefix match. Users might try to copy-paste the truncated ID shown in list, which would then trigger prefix matching rather than exact ID match.

This works correctly due to the prefix-match fallback, but consider showing the truncation explicitly (e.g., abc12345...) to signal it's abbreviated.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/commands/ctx.ts` around lines 10 - 14, The displayed
workspace ID is being truncated with ws.id.slice(0, 8) which can mislead users
into thinking that shown text is the full ID; update the rendering in the
mapping that builds the list (the .map((ws) => { ... }) block) to explicitly
indicate truncation when you shorten an ID (e.g., show `${ws.id.slice(0,8)}...`
only when ws.id.length > 8), leaving the active marker logic (marker = ws.id ===
activeId ? '*' : ' ') unchanged so users know the ID is abbreviated.

packages/cli/src/config/session.ts (1)

59-68: Consider atomic write to prevent session file corruption.

The current implementation writes directly to the session file. If the process is interrupted during writeFileSync, the file could be left in a corrupted or partial state. This is a common issue with credential files that could leave users unable to authenticate.

A safer pattern is to write to a temporary file first, then atomically rename it to the target path.

♻️ Proposed atomic write pattern

+import { renameSync } from 'node:fs';
+
 export function saveSession(
   session: Session,
   path: string = getSessionPath(),
 ): void {
   const dir = dirname(path);
   mkdirSync(dir, { recursive: true });
-  writeFileSync(path, JSON.stringify(session, null, 2), { mode: 0o600 });
-  // Explicitly chmod in case the file already existed with different permissions
-  chmodSync(path, 0o600);
+  const tempPath = `${path}.tmp`;
+  writeFileSync(tempPath, JSON.stringify(session, null, 2), { mode: 0o600 });
+  renameSync(tempPath, path);
+  // Explicitly chmod in case rename preserved different permissions
+  chmodSync(path, 0o600);
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/config/session.ts` around lines 59 - 68, The saveSession
function currently writes directly to the target path and can corrupt the
session file if interrupted; change it to perform an atomic write by writing
JSON to a temporary file in the same directory (use dirname(getSessionPath()) or
dirname(path)), fs.writeFileSync the temp file with mode 0o600, fs.renameSync
the temp file to the target path to atomically replace it, then optionally
chmodSync the final path to 0o600; also ensure mkdirSync(dir, { recursive: true
}) runs before creating the temp and clean up the temp file with unlinkSync on
error to avoid leaving cruft.

packages/cli/src/commands/import.ts (1)

51-55: Inconsistent header handling for JSON array-of-objects.

When --no-header is passed for CSV, the first row is treated as data. However, for JSON array-of-objects, a header row is always synthesized from object keys (line 55), ignoring the localOpts.header flag. This could be intentional (objects inherently define their structure), but may surprise users expecting consistent behavior.

Consider documenting this behavior or applying the header flag consistently.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/commands/import.ts` around lines 51 - 55, The JSON
array-of-objects branch always synthesizes a header row (vars objs, keys, rows)
which ignores the CSV header flag; update the logic in import.ts where parsed is
an object array so it checks localOpts.header and only prepends keys when header
is true, otherwise convert all objects to value rows (e.g., map each object to
keys from the first object but do not add the keys row) so behavior matches the
--no-header option; alternatively add a clear comment documenting the
intentional difference if you prefer to keep current behavior.

docs/tasks/active/20260312-cli-todo.md (1)

61-68: Consider archiving this task file.

All phases and verification items are marked complete. Based on learnings, completed tasks should be archived with pnpm tasks:archive && pnpm tasks:index after pnpm verify:fast passes.

Note: The PR description mentions some manual E2E items are not all completed. If those need tracking, consider adding them to this file before archiving, or create a follow-up task.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/tasks/active/20260312-cli-todo.md` around lines 61 - 68, This task file
has all verification items checked and should be archived: first ensure the
suite passes locally by running pnpm verify:fast, then archive the task with
pnpm tasks:archive && pnpm tasks:index; before archiving, if there are
outstanding manual E2E checks referenced in the PR description, add them as
checklist items to this task file (or create a follow-up task) so those items
remain tracked prior to running the archive/index commands.

packages/backend/src/auth/github.strategy.ts (1)

23-30: Consider type-safe access to __cliStateToken.

The (req as any).__cliStateToken cast works but loses type safety. Since GitHubAuthGuard sets this property, consider augmenting the Express Request type to include it.

🔧 Add type declaration for `__cliStateToken`

Create or extend a declaration file (e.g., packages/backend/src/types/express.d.ts):

declare global {
  namespace Express {
    interface Request {
      __cliStateToken?: string;
    }
  }
}

export {};

Then update the access:

-    const cliState = (req as any).__cliStateToken as string | undefined;
+    const cliState = req.__cliStateToken;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/backend/src/auth/github.strategy.ts` around lines 23 - 30, Add a
proper type declaration for Express.Request to include the optional
__cliStateToken (e.g., create an ambient declaration that extends
Express.Request with __cliStateToken?: string and export {}), then remove the
unsafe cast in authenticate and access the property as req.__cliStateToken;
update authenticate(req: Request, ...) to use that typed property (referencing
the authenticate method and GitHubAuthGuard usage) so the code is type-safe
without (req as any).

packages/backend/src/auth/cli-auth.store.ts (1)

66-70: Consider rate-limiting or bounding the maps for production resilience.

The cleanup() method runs on every createState/createCode call, iterating all entries. Under high load or a DoS attack, the maps could grow faster than cleanup removes entries. For production, consider:

• A maximum size limit with oldest-entry eviction
• Periodic cleanup on a timer rather than synchronous iteration

This is a nice-to-have for CLI auth which typically has low concurrency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/backend/src/auth/cli-auth.store.ts` around lines 66 - 70, The
cleanup() method currently iterates the entire states and codes Maps on every
createState/createCode call which can cause unbounded growth under load; change
the strategy by (1) introducing configurable MAX_STATES and MAX_CODES constants
and evicting oldest entries when insertion would exceed those limits (use Map
insertion order or maintain a small FIFO/linked list to identify oldest) inside
createState/createCode rather than full cleanup, and (2) move full-scan expiry
removal to a periodic async timer (setInterval) that runs off the hot path and
expose a stop/teardown to clear the timer; update references to cleanup(),
createState, createCode, states and codes to implement these limits and the
periodic cleaner.

docs/tasks/active/20260315-cli-oauth-login-todo.md (1)

519-524: Minor: Add language specifier to fenced code block.

The output example block lacks a language specifier. While this doesn't affect functionality, it improves documentation consistency and syntax highlighting.

📝 Suggested fix

 Output format:
-```
+```text
 Logged in as hackerwins ([email protected])
 Server:    http://localhost:3000
 Workspace: hackerwins's Workspace (e98ff707-...)
 Session:   valid (expires 2026-03-15T10:00:00Z)

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @docs/tasks/active/20260315-cli-oauth-login-todo.md around lines 519 - 524,
The fenced code block showing the CLI login output is missing a language
specifier; update the triple-backtick fence for the example output in the
docs/tasks/active/20260315-cli-oauth-login-todo.md file to include a language
(e.g., add "text" after ), so the block becomes text to enable consistent
syntax highlighting for the Logged in as... Server... Workspace... Session
output example.

</details>

</blockquote></details>
<details>
<summary>packages/backend/src/auth/auth.controller.ts (2)</summary><blockquote>

`52-62`: **Consider underscore-prefix for unused parameters.**

The `void` statements work to suppress unused variable warnings, but underscore-prefixing is more idiomatic in TypeScript:

```typescript
async githubAuth(
  `@Query`('mode') _mode: string | undefined,
  `@Query`('port') _port: string | undefined,
  `@Req`() _req: Request,
) {
  // Guard handles CLI mode detection and injects state token
}
```

This makes intent clearer at declaration rather than requiring statements in the body.

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@packages/backend/src/auth/auth.controller.ts` around lines 52 - 62, Rename
the unused parameters in the async githubAuth method to use an underscore prefix
(e.g., change mode, port, req to _mode, _port, _req) and remove the trailing
void statements in the function body; update the function signature in
auth.controller.ts (githubAuth) accordingly so the intent is clear at
declaration and the body no longer contains the unused-variable suppressions.
```

</details>

---

`132-138`: **Consider adding a comment to clarify the dual-source token logic.**

The cookie-priority logic is correct but non-obvious to future readers. A brief comment would help:

```diff
+    // Support both cookie-based (web) and body-based (CLI) refresh flows.
+    // Cookie takes priority if present; CLI clients send token in body.
     const cookieToken = req.cookies?.[REFRESH_COOKIE_NAME];
     const bodyToken =
       typeof req.body?.refreshToken === 'string'
         ? req.body.refreshToken
         : undefined;
     const fromBody = !cookieToken && !!bodyToken;
     const refreshToken = cookieToken ?? bodyToken;
```

As per coding guidelines: "Write clear comments explaining the 'why' behind complex logic, not just the 'what'".

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@packages/backend/src/auth/auth.controller.ts` around lines 132 - 138, Add a
brief clarifying comment above the dual-source token logic that explains why
cookieToken takes precedence over req.body.refreshToken and what fromBody
represents; specifically annotate the block using the symbols
REFRESH_COOKIE_NAME, cookieToken, bodyToken, fromBody, and refreshToken to state
that cookies are preferred for refresh tokens for security/consistency and
fromBody is true only when no cookie is present and a string body token exists.
Ensure the comment explains the intended precedence and purpose (not just
restating the code).
```

</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>🤖 Prompt for all review comments with AI agents</summary>

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @packages/backend/src/auth/auth.controller.ts:

• Around line 86-98: When a stateToken is present but
  this.cliAuthStore.consumeState(stateToken) returns undefined the code silently
  falls through to the web flow; change the logic so that when stateToken is
  provided and consumeState returns undefined you throw a BadRequestException
  (e.g., "Invalid or expired CLI state") to surface the error to the CLI flow;
  also simplify the branch for state.mode === 'cli' by removing the redundant port
  range check (since GitHubAuthGuard already validates port) and proceed to
  generate the code via this.cliAuthStore.createCode(user.id) and redirect to the
  127.0.0.1:{port}/callback URL as before.

In @packages/cli/package.json:

• Line 23: Add an "engines" field to the CLI package manifest to require Node.js

=20 so users cannot install with incompatible runtimes; update
packages/cli/package.json by inserting an "engines" object (e.g., "node":
">=20") at the top-level of the JSON to enforce the minimum Node version and
prevent runtime errors from the "open" dependency v11.0.0.

In @packages/cli/src/client/http-client.ts:

• Around line 70-77: The token refresh path updates in-memory tokens but skips
  persistence when loadSession() returns null, causing silent loss; update the
  refresh logic (around loadSession/saveSession in http-client.ts) to handle a
  missing session file by either creating and saving a new session object with the
  refreshed accessToken/refreshToken/expiresAt (use decodeJwtExpiry) or by logging
  a warning and returning false to signal persistence failure; ensure you
  reference loadSession(), saveSession(), and decodeJwtExpiry() and that the
  function's return value reflects whether persistence succeeded.

In @packages/cli/src/commands/export.ts:

• Around line 9-14: The detectFormat function currently casts any formatFlag to
  'csv'|'json' without validation, so invalid values (e.g., 'xml') silently become
  treated as JSON; update detectFormat to validate formatFlag explicitly: check if
  formatFlag equals 'csv' or 'json' (case-insensitive if desired) and return it
  only then, otherwise fall back to extension-based detection or throw/return a
  default and surface an error. Locate the detectFormat function and the
  formatFlag parameter to implement this guard and ensure callers (e.g., where fmt
  is compared to 'csv') receive only valid values.

In @packages/cli/src/commands/import.ts:

• Around line 9-14: The function detectFormat currently unsafely casts the
  user-provided formatFlag to 'csv'|'json'; update detectFormat to explicitly
  validate formatFlag (check if formatFlag === 'csv' or formatFlag === 'json')
  before returning it, otherwise ignore it (or throw a clear error) and fall back
  to extname(file) detection; reference the detectFormat function and the
  formatFlag parameter and ensure any error message mentions the invalid format
  and accepted values so callers/users know to pass 'csv' or 'json'.

In @packages/cli/src/commands/login.ts:

• Around line 90-98: The workspace fetch currently fails silently and leaves
  workspaces empty; wrap the fetch call used to populate workspaces (the wsRes
  fetch to ${server}/workspaces using tokens.accessToken) in a try/catch and,
  when wsRes.ok is false or an exception is thrown, emit a clear warning (e.g.,
  console.warn or the CLI logger) that includes the HTTP status and response text
  or the caught error message so users know the fetch failed rather than
  legitimately having no workspaces; keep the existing behavior of defaulting
  workspaces to [] after logging the warning.

---

Nitpick comments:
In @docs/tasks/active/20260312-cli-todo.md:

• Around line 61-68: This task file has all verification items checked and
  should be archived: first ensure the suite passes locally by running pnpm
  verify:fast, then archive the task with pnpm tasks:archive && pnpm tasks:index;
  before archiving, if there are outstanding manual E2E checks referenced in the
  PR description, add them as checklist items to this task file (or create a
  follow-up task) so those items remain tracked prior to running the archive/index
  commands.

In @docs/tasks/active/20260315-cli-oauth-login-todo.md:

• Around line 519-524: The fenced code block showing the CLI login output is
  missing a language specifier; update the triple-backtick fence for the example
  output in the docs/tasks/active/20260315-cli-oauth-login-todo.md file to include
  a language (e.g., add "text" after ), so the block becomes text to enable
  consistent syntax highlighting for the Logged in as... Server... Workspace...
  Session output example.

In @packages/backend/src/auth/auth.controller.ts:

• Around line 52-62: Rename the unused parameters in the async githubAuth method
  to use an underscore prefix (e.g., change mode, port, req to _mode, _port, _req)
  and remove the trailing void statements in the function body; update the
  function signature in auth.controller.ts (githubAuth) accordingly so the intent
  is clear at declaration and the body no longer contains the unused-variable
  suppressions.
• Around line 132-138: Add a brief clarifying comment above the dual-source
  token logic that explains why cookieToken takes precedence over
  req.body.refreshToken and what fromBody represents; specifically annotate the
  block using the symbols REFRESH_COOKIE_NAME, cookieToken, bodyToken, fromBody,
  and refreshToken to state that cookies are preferred for refresh tokens for
  security/consistency and fromBody is true only when no cookie is present and a
  string body token exists. Ensure the comment explains the intended precedence
  and purpose (not just restating the code).

In @packages/backend/src/auth/cli-auth.store.ts:

• Around line 66-70: The cleanup() method currently iterates the entire states
  and codes Maps on every createState/createCode call which can cause unbounded
  growth under load; change the strategy by (1) introducing configurable
  MAX_STATES and MAX_CODES constants and evicting oldest entries when insertion
  would exceed those limits (use Map insertion order or maintain a small
  FIFO/linked list to identify oldest) inside createState/createCode rather than
  full cleanup, and (2) move full-scan expiry removal to a periodic async timer
  (setInterval) that runs off the hot path and expose a stop/teardown to clear the
  timer; update references to cleanup(), createState, createCode, states and codes
  to implement these limits and the periodic cleaner.

In @packages/backend/src/auth/github.strategy.ts:

• Around line 23-30: Add a proper type declaration for Express.Request to
  include the optional __cliStateToken (e.g., create an ambient declaration that
  extends Express.Request with __cliStateToken?: string and export {}), then
  remove the unsafe cast in authenticate and access the property as
  req.__cliStateToken; update authenticate(req: Request, ...) to use that typed
  property (referencing the authenticate method and GitHubAuthGuard usage) so the
  code is type-safe without (req as any).

In @packages/cli/src/commands/ctx.ts:

• Around line 10-14: The displayed workspace ID is being truncated with
  ws.id.slice(0, 8) which can mislead users into thinking that shown text is the
  full ID; update the rendering in the mapping that builds the list (the .map((ws)
  => { ... }) block) to explicitly indicate truncation when you shorten an ID
  (e.g., show ${ws.id.slice(0,8)}... only when ws.id.length > 8), leaving the
  active marker logic (marker = ws.id === activeId ? '*' : ' ') unchanged so users
  know the ID is abbreviated.

In @packages/cli/src/commands/import.ts:

• Around line 51-55: The JSON array-of-objects branch always synthesizes a
  header row (vars objs, keys, rows) which ignores the CSV header flag; update the
  logic in import.ts where parsed is an object array so it checks localOpts.header
  and only prepends keys when header is true, otherwise convert all objects to
  value rows (e.g., map each object to keys from the first object but do not add
  the keys row) so behavior matches the --no-header option; alternatively add a
  clear comment documenting the intentional difference if you prefer to keep
  current behavior.

In @packages/cli/src/config/session.ts:

• Around line 59-68: The saveSession function currently writes directly to the
  target path and can corrupt the session file if interrupted; change it to
  perform an atomic write by writing JSON to a temporary file in the same
  directory (use dirname(getSessionPath()) or dirname(path)), fs.writeFileSync the
  temp file with mode 0o600, fs.renameSync the temp file to the target path to
  atomically replace it, then optionally chmodSync the final path to 0o600; also
  ensure mkdirSync(dir, { recursive: true }) runs before creating the temp and
  clean up the temp file with unlinkSync on error to avoid leaving cruft.

In @packages/cli/src/util/csv-parse.ts:

• Around line 68-77: The parseStartRef function currently silently returns the
  default {row: 1, col: 1} for invalid refs; update parseStartRef to emit a clear
  warning (e.g., console.warn) when the input ref fails the regex so users know
  their --start argument was malformed, include the original ref value in the
  message for debugging, and then continue returning the existing fallback; keep
  the function name parseStartRef and its return shape unchanged.

In @packages/cli/test/config.test.ts:

• Around line 81-125: Tests under the migrateConfigIfNeeded describe block leave
  temporary directories in the OS tmpdir; add cleanup by storing each test's base
  temp directory in a scoped variable and adding an afterEach hook that
  recursively removes that base (using fs.rmSync or fs.rmdirSync with { recursive:
  true } or try/catch around unlink) so tests do not leave artifacts; reference
  the existing migrateConfigIfNeeded tests and the tmpdir-based base variables
  (e.g., base, oldDir, newDir) and ensure the afterEach is tolerant if the
  directory was never created.

In @packages/cli/test/csv-parse.test.ts:

• Around line 4-33: Add a test to cover quoted fields that contain embedded
  newlines for parseCsv: create an it block that calls parseCsv with a CSV string
  where a field is quoted and includes a newline (e.g. ""line1\nline2",b") and
  assert the parsed output contains that field with the newline preserved
  (['line1\nline2','b']). Place this new test alongside the other parseCsv tests
  so parseCsv is exercised for embedded-newline quoted fields.

</details>

---

<details>
<summary>ℹ️ Review info</summary>

<details>
<summary>⚙️ Run configuration</summary>

**Configuration used**: defaults

**Review profile**: CHILL

**Plan**: Pro

**Run ID**: `6a6f76f5-b94c-462f-a82e-c48497bcc05f`

</details>

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 865e7c142f86194722a9abe2fe381643695411a5 and 66d2a34090b4c6838b079c089fa3ae3a2626f21b.

</details>

<details>
<summary>⛔ Files ignored due to path filters (1)</summary>

* `pnpm-lock.yaml` is excluded by `!**/pnpm-lock.yaml`

</details>

<details>
<summary>📒 Files selected for processing (33)</summary>

* `docs/design/README.md`
* `docs/design/cli-oauth-login.md`
* `docs/design/rest-api-and-cli.md`
* `docs/tasks/active/20260312-cli-todo.md`
* `docs/tasks/active/20260315-cli-oauth-login-todo.md`
* `packages/backend/src/auth/auth.controller.spec.ts`
* `packages/backend/src/auth/auth.controller.ts`
* `packages/backend/src/auth/auth.module.ts`
* `packages/backend/src/auth/cli-auth.store.spec.ts`
* `packages/backend/src/auth/cli-auth.store.ts`
* `packages/backend/src/auth/github-auth.guard.ts`
* `packages/backend/src/auth/github.strategy.ts`
* `packages/backend/src/auth/jwt.strategy.ts`
* `packages/cli/package.json`
* `packages/cli/skills/SKILL.md`
* `packages/cli/skills/import-export.md`
* `packages/cli/src/bin.ts`
* `packages/cli/src/client/http-client.ts`
* `packages/cli/src/commands/ctx.ts`
* `packages/cli/src/commands/export.ts`
* `packages/cli/src/commands/import.ts`
* `packages/cli/src/commands/login.ts`
* `packages/cli/src/commands/logout.ts`
* `packages/cli/src/commands/status.ts`
* `packages/cli/src/config/config.ts`
* `packages/cli/src/config/session.ts`
* `packages/cli/src/schema/registry.ts`
* `packages/cli/src/util/csv-parse.ts`
* `packages/cli/test/config.test.ts`
* `packages/cli/test/csv-parse.test.ts`
* `packages/cli/test/ctx.test.ts`
* `packages/cli/test/session.test.ts`
* `packages/docs/api/cli.md`

</details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

packages/cli/src/commands/import.ts (1)

69-71: ⚠️ Potential issue | 🟠 Major

--no-header option is still not applied for CSV imports.

The localOpts.header flag is parsed (line 43) but the CSV branch at line 70 ignores it. When --no-header is passed, header is false, meaning the first row should be treated as data, not a header. Currently all rows are passed unchanged.

Per the context in csv-parse.ts, buildCellMap expects preprocessed data. The header handling should happen here before calling buildCellMap.

🐛 Proposed fix

         } else {
-          rows = parseCsv(raw);
+          const parsed = parseCsv(raw);
+          // When header is true (default), first row is a header and included in data
+          // When --no-header is passed (header=false), all rows are data
+          rows = parsed;
         }
+
+        // If header mode is on and we have CSV data, the first row is already
+        // part of the data array. No stripping needed since buildCellMap
+        // imports all rows including headers as cell values.

Note: If the intent is to skip the header row when importing (not import column names as cell values), the logic would be:

         } else {
-          rows = parseCsv(raw);
+          const parsed = parseCsv(raw);
+          // Skip header row when header=true (default), keep all rows when --no-header
+          rows = localOpts.header ? parsed.slice(1) : parsed;
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/commands/import.ts` around lines 69 - 71, The CSV branch
ignores the localOpts.header flag so --no-header isn’t applied; in the import
flow (the block that calls parseCsv and later buildCellMap) detect
localOpts.header (or localOpts.header === false) and, when header is false, do
not treat the first parsed row as a header — either pass the parsed rows
unchanged as data rows or explicitly prepend synthetic headers if buildCellMap
expects headers; update the code around parseCsv(...) and before
buildCellMap(...) to remove/retain the first row accordingly (refer to parseCsv,
rows and buildCellMap identifiers) so the first CSV row is treated as data when
--no-header is set.

packages/cli/src/commands/export.ts (1)

38-40: ⚠️ Potential issue | 🟡 Minor

Preserve the backend's error message on failed exports.

The error at line 40 reduces every non-OK response to HTTP ${res.status}, hiding useful details like missing tabs or invalid ranges. As per coding guidelines: "Use meaningful error messages that help with debugging."

🔧 Proposed fix

-        if (!res.ok) throw new Error(`HTTP ${res.status}`);
+        if (!res.ok) {
+          const body = res.data as { error?: { message?: string } } | null;
+          throw new Error(body?.error?.message ?? `HTTP ${res.status}`);
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/commands/export.ts` around lines 38 - 40, The current export
error replaces any non-OK response with a generic "HTTP ${res.status}", losing
backend diagnostics; update the handling after calling
getClient(...).getCells(docId, localOpts.tab, localOpts.range) to read the
response body (e.g., await res.text() or await res.json() depending on
content-type) and include that message along with the status in the thrown Error
so callers see the backend error (reference the res variable returned from
getCells and the surrounding try block where the throw happens).

packages/cli/src/commands/login.ts (1)

96-101: ⚠️ Potential issue | 🟠 Major

Workspace fetch failure still overwrites session with empty workspace state.

While a warning was added at line 100, a transient /workspaces failure (e.g., HTTP 500) still results in saving workspaces: [] and activeWorkspace: '' at lines 132-133. Subsequent workspace-scoped commands will fail or use empty workspace IDs.

Consider preserving the existing session's workspace state on fetch failure, or failing the login entirely rather than leaving the user in a broken state.

🔧 Proposed fix - preserve existing workspace state

       let workspaces: WorkspaceInfo[] = [];
+      let activeWorkspace = '';
       if (wsRes.ok) {
         workspaces = (await wsRes.json()) as WorkspaceInfo[];
       } else {
         console.warn(`Warning: failed to fetch workspaces (HTTP ${wsRes.status})`);
+        // Preserve existing workspace state if available
+        if (existing) {
+          workspaces = existing.workspaces;
+          activeWorkspace = existing.activeWorkspace;
+          console.warn('Preserving previous workspace context.');
+        }
       }

       // 8. Select workspace
-      let activeWorkspace = '';
-      if (workspaces.length === 0) {
+      if (workspaces.length === 0 && !activeWorkspace) {
         console.log('No workspaces found.');
       } else if (workspaces.length === 1) {

Also applies to: 125-136

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/commands/login.ts` around lines 96 - 101, The current login
flow overwrites the session with an empty workspace list when wsRes.ok is false;
instead, when the /workspaces fetch fails (wsRes.ok === false) preserve the
existing workspace state or abort the login: do not assign workspaces = [] or
activeWorkspace = '' on error—keep the previous session.workspaces and
session.activeWorkspace (or load them from the session store) and only
update/sync workspaces when wsRes.ok is true, or throw/return an error to stop
the save flow so the code path that writes the session (the block that persists
workspaces and activeWorkspace) does not run with an empty list. Ensure you
update the logic around the workspaces variable, wsRes handling, and the session
save to reflect this behavior.

packages/cli/test/config.test.ts (2)

15-16: Add a positive WAFFLEBASE_SESSION resolution test (and precedence check).

This suite now covers the missing-session path, but not the critical edge case where a real session file exists and should participate in resolveConfig precedence. Please add one test that uses a valid session file and asserts ordering versus flags/env/config.

As per coding guidelines: **/*.{test,spec}.{js,ts,jsx,tsx}: Write tests for critical business logic and edge cases.

Also applies to: 23-30

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cli/test/config.test.ts` around lines 15 - 16, Add a positive test
case to packages/cli/test/config.test.ts that creates a valid WAFFLEBASE_SESSION
file and verifies resolveConfig reads it and that its values take the correct
precedence versus CLI flags, environment variables, and config file;
specifically, call resolveConfig with conflicting inputs (e.g., set a flag, set
process.env.WAFFLEBASE_SESSION to point to the temp session file, and provide a
config object/file) and assert the resolved config matches the session file's
values, then clean up the temp session file and restore process.env (remove
WAFFLEBASE_SESSION) after the test.

---

85-126: Use per-test temp dirs with explicit cleanup to avoid fixture leakage.

These tests create directories/files in tmpdir() but never remove them. Over time this pollutes CI/dev machines and can cause rare collisions with timestamp-based naming. Prefer mkdtempSync + rmSync(..., { recursive: true, force: true }) in teardown/finally.

Suggested refactor

-import { mkdirSync, writeFileSync, existsSync, readFileSync } from 'node:fs';
+import { mkdirSync, writeFileSync, existsSync, readFileSync, mkdtempSync, rmSync } from 'node:fs';
 import { join } from 'node:path';
 import { tmpdir } from 'node:os';

 describe('migrateConfigIfNeeded', () => {
   it('copies config from old path to new path when new path does not exist', () => {
-    const base = join(tmpdir(), `wfb-migrate-test-${Date.now()}`);
+    const base = mkdtempSync(join(tmpdir(), 'wfb-migrate-test-'));
     const oldDir = join(base, 'old');
     const newDir = join(base, 'new');
     const oldPath = join(oldDir, 'config.yaml');
     const newPath = join(newDir, 'config.yaml');

-    mkdirSync(oldDir, { recursive: true });
-    writeFileSync(oldPath, 'profiles:\n  default:\n    server: https://old.example.com\n');
-
-    migrateConfigIfNeeded(newPath, oldPath);
-
-    expect(existsSync(newPath)).toBe(true);
-    expect(readFileSync(newPath, 'utf-8')).toContain('https://old.example.com');
+    try {
+      mkdirSync(oldDir, { recursive: true });
+      writeFileSync(oldPath, 'profiles:\n  default:\n    server: https://old.example.com\n');
+      migrateConfigIfNeeded(newPath, oldPath);
+      expect(existsSync(newPath)).toBe(true);
+      expect(readFileSync(newPath, 'utf-8')).toContain('https://old.example.com');
+    } finally {
+      rmSync(base, { recursive: true, force: true });
+    }
   });
 });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cli/test/config.test.ts` around lines 85 - 126, These tests leak
temp files by using join(tmpdir(), `...${Date.now()}`) without cleanup; change
each test to create a per-test temp directory with
mkdtempSync(path.join(tmpdir(), 'wfb-migrate-XXXXXX')), use that dir for
oldDir/newDir and call migrateConfigIfNeeded(newPath, oldPath) as before, and
ensure cleanup by calling rmSync(base, { recursive: true, force: true }) in a
finally block inside the test or in an afterEach teardown; reference
migrateConfigIfNeeded, mkdtempSync, rmSync, tmpdir and the
base/oldDir/newDir/newPath variables when making the edits.