[cameronsjo]

refs #77

What

parse_workspace_id accepts a positional convenience (w_N or bare N, the Nth workspace in display order, documented in the socket API) and returned that index unbounded. public_workspace_id indexes the workspace list bare, so pane.move with destination.type = "new_tab" and workspace_id: "999999" panicked the server from one socket request. The WorkspaceFocus shell navigation path had the same exposure. The guard some callers carry sits in a different arm and never covered the panic path.

The fix is one .filter at the source bounding the index to the live list. The positional convenience survives; an out-of-range id now yields the same workspace_not_found every caller already returns for an unknown id. Four comments that said the function does not bounds-check are corrected; the three hand-rolled guards stay as defense in depth and the fork ledger says so, so an upstream sync does not read them as stale.

What the issue got wrong

Stale ids already failed closed: live ids are w<base32> with an alphabet that has no underscore, so a stale w5 never matched the positional shapes. plugin_context_for_workspace_id never calls this function. The real defect was the panic.

Tests

Two: the bound itself over in-range, out-of-range, exact-match, and stale-shaped ids; and pane.move to a new tab with an out-of-range workspace id returning workspace_not_found with the source pane still attached. The client_views.rs path is safe by construction and has no test harness of its own. Not run locally (the vendored Zig build does not link on this Mac); CI is the proving run.

Review

Code review and an independent security review ran against the diff at c83d4754, both clean. The follow-up commit fills the PR: line in the changes ledger.

Summary by CodeRabbit

• Bug Fixes

  • Invalid numeric workspace identifiers are now rejected when they fall outside the range of live workspaces.
  • Pane and tab move operations no longer risk unexpected failures or unintended changes when given an out-of-range workspace ID.
  • Valid positional and exact workspace identifiers continue to resolve correctly.

• Tests

  • Added regression coverage for valid, invalid, and out-of-range workspace identifiers.

• Documentation

  • Updated workspace and pane-management documentation to reflect the corrected identifier handling.

[cameronsjo]

Code review and an independent security review ran against the diff at c83d4754. Both clean: no Critical, no Important. The delta to this head is one line in docs/fork/CHANGES.md (the PR link); source is identical, so the review binds here.

Security review traced all call sites: the .filter closes the panic on both unguarded surfaces (pane.move and WorkspaceFocus), parse_tab_id/parse_pane_id already re-bound via .get(), no targeting or authorization outcome changes, and checked_sub(1) is safe on 0 and usize::MAX-shaped input. Nits, not actioned: public_workspace_id remains an unchecked index at the sink (herdr#80); the new test does not pin 0/w_0/usize::MAX.

Not run locally (vendored Zig build does not link here); CI is the proving run.

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{On-demand reviews are free for the next 28 days. After that, they cost $0.25 per reviewed file.}

Or wait 13 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available. Your 55 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 96286a87-78d7-4343-9e95-bb56f6efead2

📥 Commits

Reviewing files that changed from the base of the PR and between 4a4f0f2 and 6710a5f.

📒 Files selected for processing (1)

• src/app/api/panes.rs

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Essentials

Run ID: e62832dc-538b-4b53-aeac-3c5a1e467c61

📥 Commits

Reviewing files that changed from the base of the PR and between ee4e981 and 4a4f0f2.

📒 Files selected for processing (6)

• docs/fork/CHANGES.md
• src/app/api/panes.rs
• src/app/api/tabs.rs
• src/app/api/workspaces.rs
• src/app/ids.rs
• src/app/mod.rs

Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.

---

📝 Walkthrough

Walkthrough

parse_workspace_id now rejects out-of-range positional workspace identifiers. API handlers return workspace_not_found without invalid indexing or pane state changes. Regression tests cover parser and pane-move behavior.

Changes

Workspace ID bounds

Layer / File(s)	Summary
Bounded parsing and resolution tests src/app/ids.rs, src/app/mod.rs	parse_workspace_id rejects out-of-range bare and w_-prefixed identifiers. Tests cover valid positional, exact, and invalid identifiers.
API handling and regression coverage src/app/api/panes.rs, src/app/api/tabs.rs, src/app/api/workspaces.rs, docs/fork/CHANGES.md	API comments and changelog entries describe bounded parsing and defensive checks. Pane-move regression coverage verifies workspace_not_found and unchanged state.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: ogulcancelik

Merge Risk: ⚪ Minimal · up to 4a4f0

Out-of-range workspace IDs now return a not-found response rather than reaching invalid indexing paths. No remaining merge-blocking risk was identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 5 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: bounding positional workspace ID fallback to the live workspace list.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 5 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue/77-workspace-id-positional-fallback

---

_{A rabbit checks each bound,
Safe workspaces hold their ground,
Invalid IDs fade,
Panes remain where they were placed,
Tests guard the state.}

_{Comment @coderabbitai help to get the list of available commands.}

[cameronsjo]

Rebinds the review at c83d4754. Two commits since: the ledger PR link, and a test-only fix (the pane-move bound test now sets an active workspace so the state invariant check at its end holds; CI caught it, the local build cannot link). Production source is identical to the reviewed head. CI green on macOS and ubuntu at this head.