fix(workspace): open the link pickers on a workspace that already has the project's name - #1379
Conversation
… the project's name When no workspace is linked, both link pickers opened on "create a quick workspace", even when the list showed a workspace with exactly the name the create would use. Enter then made a second workspace with that name, splitting the team's skills and memory between the two. - `sameNamedWorkspace()` (workspace-name.ts) finds a listed workspace whose name matches the proposed one, comparing case- and whitespace-insensitively. - `altimate-code link` and the TUI's on-demand picker open on that workspace and mark it "same name as this project". - Creating a workspace with a name that is already listed asks for confirmation first (defaulting to no in the CLI). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172qrhMa5TQgETASi5hxMqD
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe CLI and TUI workspace pickers detect listed workspaces whose names match the project name. When applicable, they select and label a matching workspace. They ask for confirmation before creating another workspace with that name. ChangesNamesake Workspace Selection
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Picker as CLI or TUI picker
participant Matcher as Workspace name utilities
participant User
participant Creation as Workspace creation
Picker->>Matcher: Find namesakes for the proposed name
Matcher-->>Picker: Return matching workspaces and caller-owned match
Picker->>User: Show picker with namesake selection and hints
User->>Picker: Choose a create option
Picker->>User: Request confirmation when namesakes exist
User->>Picker: Confirm creation
Picker->>Creation: Create workspace
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The changes favor caller-owned matching workspaces and ask before duplicate creation. No actionable merge-blocking risk remains; normal checks should precede merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The change improves duplicate prevention and avoids defaulting to someone else's workspace during a stable sign-in. However, a pending namesake selection can outlive an account or tenant change, creating a conditional risk of linking the project to a different workspace than the one displayed. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks names by the soft moonlight Comment |
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
@codex review Please try to falsify these claims with concrete input:
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a3d3e92e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .replace(/[\u0000-\u001F\u007F-\u009F\u2028\u2029]+/g, " ") | ||
| .replace(/\s+/g, " ") | ||
| .trim() | ||
| .toLowerCase() |
There was a problem hiding this comment.
For an unlinked project named STRASSE with a listed workspace named Straße, sameNamedWorkspace([{ id: 1, name: "Straße" }], "STRASSE") returns undefined: lowercasing produces straße versus strasse, although these are case variants under Unicode caseless matching. Because remote and directory names may contain arbitrary Unicode, both pickers then remain on Create and skip the duplicate-name confirmation, contrary to the claimed case-insensitive behavior; use Unicode case folding rather than toLowerCase().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. Fixed in 1788d02: the comparison now upper-cases before lower-casing, which gives full case folding, so Straße and STRASSE compare equal. Test case added: "case differs beyond ASCII (full case folding)".
Lower-casing alone leaves `Straße` and `STRASSE` different; upper-casing first gives full case folding, so a namesake that differs only by case is found. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172qrhMa5TQgETASi5hxMqD
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1788d02ee3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .toUpperCase() | ||
| .toLowerCase() |
There was a problem hiding this comment.
Fresh evidence against this fix is the concrete pair ẞ and SS: the new sequence normalizes them to ß and ss, respectively, so sameNamedWorkspace([{ id: 1, name: "ẞ" }], "SS") still returns undefined even though Unicode full case folding maps both names to ss. Thus a project differing only by this case variant can still bypass the duplicate-name confirmation; use an actual full case-fold implementation rather than composing locale-neutral upper- and lower-casing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed; fixed in the next commit: the comparison now lower-cases, upper-cases, then lower-cases again, so ẞ and SS both reach ss. Straße / STRASSE still match, and both pairs are in the test matrix. That is Unicode case mapping rather than the complete CaseFolding table, which JavaScript does not provide. I have recorded that as a residual in the description rather than hand-maintaining the table for workspace names.
…hes SS An upper-then-lower pass leaves `ẞ` as `ß` while `SS` becomes `ss`; lower-casing first lets the expanding mappings meet. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172qrhMa5TQgETASi5hxMqD
|
@codex review A fresh round on the current head. Please try to falsify the three claims from the earlier summon (the namesake default, the confirmation before a duplicate, and nothing else about the pickers changing), plus the residual about Unicode case mapping. Report only what concrete input shows false. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fc43d3a15
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (pick === CREATE_NEW_SENTINEL) { | ||
| if (namesake) { |
There was a problem hiding this comment.
For an unlinked freemium project named analytics with an existing analytics workspace, selecting “Set up in browser” still calls runBrowserHandoff(identifier, autoName, ...) and creates another workspace without reaching this confirmation, because the namesake check is confined to the quick-create sentinel branch. Apply the duplicate-name confirmation to every creation option, including browser setup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed; fixed in 0ebb311. The namesake confirmation now runs before either create path, "Set up in browser" included, since both start from the project's name.
| .toLowerCase() | ||
| .toUpperCase() | ||
| .toLowerCase() |
There was a problem hiding this comment.
Fresh input against the Unicode fix is ı versus i: Unicode default case folding keeps dotless ı distinct, but this lower/upper/lower sequence maps both strings to i. Consequently, sameNamedWorkspace([{ id: 1, name: "ı" }], "i") returns that workspace, causing both pickers to default to an unrelated workspace and warn about a duplicate that does not exist.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed; fixed in 0ebb311 by switching from chained case mapping to a Unicode collator that ignores case but not accents, with ß spelled out first. ı/i and café/cafe now stay distinct, while Straße/STRASSE, ẞ/SS, ΟΔΟΣ/οδος and finance/FINANCE still match. All of these are in the test matrix.
…esakes on the browser path too - `sameNamedWorkspace` compares names with a case-insensitive, accent-sensitive Unicode collator (after spelling out ß), instead of chained case mapping that merged dotless ı with i. `Straße`/`STRASSE`, `ẞ`/`SS`, `ΟΔΟΣ`/`οδος` and `finance`/`FINANCE` match; `ı`/`i` and `café`/`cafe` do not. - `altimate-code link` asks before either create path when a namesake is listed, including "Set up in browser", which also starts from the project's name. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172qrhMa5TQgETASi5hxMqD
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
@codex review Scoped round on 0ebb311, the fix for your two findings in the last round. Please try to falsify these with a concrete input:
Out of scope: the TUI dialog wiring from earlier rounds, which is unchanged. Residual: JavaScript has no complete CaseFolding table, so a collator difference from full Unicode case folding on some other script is accepted. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| return | ||
| } | ||
| props.api.ui.dialog.replace(() => ( | ||
| <props.api.ui.DialogConfirm |
There was a problem hiding this comment.
WARNING: Default the duplicate-name confirmation to No
The new confirmation uses DialogConfirm, whose initial store.active is "confirm" (packages/tui/src/ui/dialog-confirm.tsx:23). After selecting Create, one more Enter immediately calls createAndBindInline without the user explicitly choosing Yes. This is especially easy with repeated Enter input and defeats the safety guard this PR adds; use a cancel-first selection here (or make this dialog support an initial cancel choice).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in ce6be7c. The duplicate-name question is now a DialogSelect that opens on "No, don't create it". "Yes, create another" has to be chosen, so a stray Enter cannot create the duplicate. DialogConfirm has no cancel-first option, and adding one would change the shared TUI component and the plugin API.
| description: | ||
| dm.id === props.currentlyLinkedDatamateId | ||
| ? "currently linked to this project" | ||
| : dm.id === namesake()?.id |
There was a problem hiding this comment.
WARNING: Compute the namesake once per picker update
namesake() scans every listed workspace and constructs a new Intl.Collator, and this expression invokes it for every row rendered by options(). A list of N workspaces therefore triggers N full scans (O(N²) comparisons) on load or rerender, potentially freezing the TUI for large accounts. Resolve the match once outside list.map and reuse its ID for the descriptions and selected value.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in ce6be7c. namesake is a createMemo, computed once per list rather than once per row.
Code Review SummaryStatus: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
The previous finding remains unresolved at the current HEAD. On an account mismatch, Files Reviewed (1 files)
There are no source changes between Fix these issues in Kilo Cloud Previous Review Summaries (7 snapshots, latest commit 9713c92)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 9713c92)Status: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit cb26ed1)Status: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
The new guard rejects a switch completed before selection, but Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit c7c18d2)Status: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Previous review (commit 77716eb)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit 7ae2ff6)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 files)
Previous review (commit ce6be7c)Status: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 0ebb311)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (4 files)
Reviewed by gpt-sol-latest · Input: 0 · Output: 0 · Cached: 0 Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…once per list The TUI question is a select that opens on No, so a stray Enter cannot create the duplicate. The namesake is computed once per list, and naming and comparing share one normaliser. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172qrhMa5TQgETASi5hxMqD
778a630 to
ce6be7c
Compare
| // Both create paths start from the project's name, so both confirm a namesake. | ||
| if ((pick === SET_UP_IN_BROWSER_SENTINEL || pick === CREATE_NEW_SENTINEL) && namesake) { | ||
| const again = await prompts.confirm({ | ||
| message: `A workspace named "${inertWorkspaceName(namesake.name)}" already exists. Create another one with the same name?`, |
There was a problem hiding this comment.
WARNING: Preserve bidi-control stripping in the confirmation
Replacing stripControlChars with inertWorkspaceName drops the existing removal of U+200E/U+200F, U+202A–U+202E and U+2066–U+2069. The latter helper only replaces C0/C1 controls and line separators, so a server-supplied namesake containing a right-to-left override (\u202e) now reaches prompts.confirm intact and can visually reorder the name or following confirmation text in bidi-aware terminals. The picker still strips these characters, making the same workspace display differently at the confirmation boundary. Compose the existing sanitizer with the new one to retain both protections.
| message: `A workspace named "${inertWorkspaceName(namesake.name)}" already exists. Create another one with the same name?`, | |
| message: `A workspace named "${inertWorkspaceName(stripControlChars(namesake.name))}" already exists. Create another one with the same name?`, |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
@codex review Scoped round on ce6be7c, the fixes for the bot findings: the TUI same-name question is a select that opens on No, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce6be7c1d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Both create paths start from the project's name, so both confirm a namesake. | ||
| if ((pick === SET_UP_IN_BROWSER_SENTINEL || pick === CREATE_NEW_SENTINEL) && namesake) { | ||
| const again = await prompts.confirm({ | ||
| message: `A workspace named "${inertWorkspaceName(namesake.name)}" already exists. Create another one with the same name?`, |
There was a problem hiding this comment.
When a server-controlled namesake contains a bidi override such as U+202E, inertWorkspaceName() leaves that character intact, whereas the replaced stripControlChars() call explicitly removed U+202A–U+202E and U+2066–U+2069. The raw Clack confirmation can therefore render visually reordered or misleading text even though every other workspace-name display in this CLI retains the bidi sanitization; preserve stripControlChars() for this terminal-facing interpolation, potentially after the shared formatter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in 7ae2ff6. The prompt now renders stripControlChars(inertWorkspaceName(name)): one line and bounded, then the C0/C1 and bidi controls removed, like every other terminal-facing name in this command.
inertWorkspaceName keeps the name on one line but leaves bidi overrides, which stripControlChars removes for every other terminal-facing workspace name. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172qrhMa5TQgETASi5hxMqD
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ae2ff686e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Both create paths start from the project's name, so both confirm a namesake. | ||
| if ((pick === SET_UP_IN_BROWSER_SENTINEL || pick === CREATE_NEW_SENTINEL) && namesake) { | ||
| const again = await prompts.confirm({ | ||
| message: `A workspace named "${stripControlChars(inertWorkspaceName(namesake.name))}" already exists. Create another one with the same name?`, |
There was a problem hiding this comment.
When a server-supplied name contains U+061C ARABIC LETTER MARK, this composition leaves it intact: inertWorkspaceName() does not treat it as whitespace or a line separator, and stripControlChars() omits it from its bidi-control ranges. U+061C is an invisible Unicode bidi control with bidi class AL, so it can still alter the ordering of adjacent numbers and punctuation in this confirmation prompt; include U+061C in the sanitizer and its exhaustive bidi-control test.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in 77716eb. stripControlChars now removes U+061C as well, and the exhaustive bidi scan in release-v0.11.2-adversarial.test.ts includes it. The helper predates this PR, so every workspace name this command prints gets the fix too.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172qrhMa5TQgETASi5hxMqD
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Review: PR #1379
Solid fix for the two pickers. One shared helper means the CLI and TUI can't drift. Precedence is existing link, then namesake, then create. The confirmation defaults to No on both surfaces, and the TUI uses DialogSelect rather than DialogConfirm for that reason. DialogSelect also re-runs on current after the async list loads, so the cursor really does move to the namesake. The new test and the adversarial suite pass locally (37 pass, 0 fail).
The one MAJOR finding is inline: the post-scan OfferDialog still creates duplicates. The remaining findings follow.
Minor
1. new Intl.Collator("und", …) follows the host's default locale (workspace-name.ts:78)
"und" isn't a supported collator locale (Intl.Collator.supportedLocalesOf(["und"]) → []), so it resolves to the process default. Under Node with LC_ALL=tr_TR.UTF-8 it resolves to tr-TR, and sameNamedWorkspace([{ name: "ANALYTICS" }], "analytics") returns undefined: Turkish pairs I with ı, not i. Any name containing I/i stops matching, and the user gets the pre-PR behavior back. With a Danish default, "aa" and "å" compare equal, a false-positive namesake. The tests pass only because they run in an en-like locale. Pin it and hoist it, since it is currently rebuilt on every call:
const NAME_COLLATOR = new Intl.Collator("en", { sensitivity: "accent", usage: "search" })Verified: under a tr_TR host, Intl.Collator("en", …) resolves to en and compares ANALYTICS/analytics as 0. A test asserting resolvedOptions().locale === "en" would pin it.
2. The TUI confirmation shows names less safely than the CLI (workspace.tsx:1067, :1071 vs link.ts:345)
The title uses inertWorkspaceName(twin.name), which leaves bidi controls (ALM, LRM/RLM, LRE..RLO, LRI..PDI) in a server-controlled name. The CLI wraps the same name in stripControlChars(...), the protection this PR's ALM commit exists for. The "Yes, create another …" option interpolates the raw props.defaultName. Moving the bidi strip into workspace-name.ts would let both surfaces share it.
3. The default Enter now links to, and seeds memory into, a namesake that may be a colleague's (workspace.tsx:1039 → bindOrRebindInline → recordApprovedBinding; link.ts:334; workspace-name.ts:79)
This is what #1378 asks for, but it changes what a reflexive Enter does. It used to create a private workspace. Now it links to whichever visible workspace shares the name, which may only be shared with the user ("Linking needs only visibility"), and uploads this machine's memory into it. runFlow avoids exactly that without an explicit Attach (:1257-1262). With two namesakes, server list order decides. Consider preferring a namesake whose ownerId matches the caller, and/or marking ones the caller doesn't own. Not a blocker, but it should be a conscious choice.
4. No tests for the picker or confirmation behavior
The matrix covers only the helper. Nothing asserts any of the following:
- initial-selection precedence in either picker;
- that both CLI create sentinels are gated;
- that No/cancel makes no create or bind call;
- that the TUI Yes path calls
createAndBindInlineonce.
A refactor could drop the gate and every test would still pass.
5. Contract edges the matrix doesn't pin (same-named-workspace.test.ts)
Worth adding cases for:
- a control character in the proposed name;
- NFC vs NFD input (
"cafe\u0301"vs"café"); - zero-width or bidi characters in a listed name. These are collation-ignorable, so
"analy\u061Ctics"does match"analytics", which a test should pin as intended; - locale independence (see 1).
Nits
workspace-name.ts:73: the bare(codex)attribution in a shipped doc comment. Drop it or cite the review round, as other comments in the file do.workspace.tsx:1039: passingnamesake()?.idascurrentmakesDialogSelectdraw its●gutter on that row (dialog-select.tsx:585,701-705). In this picker a●in the title means "currently linked", so on an unlinked project it can read as "linked here". The description text disambiguates.workspace.tsx:1003: "Once per list, not once per row: the options below read it for every workspace" reads as if the per-row read is the cost. Something like "computed once per list; each row reads the cached value" would be clearer. ThesameNamedWorkspacedoc could also say that ligature and final-sigma equivalence come from collation, not fromcomparableName.
…y my own The post-scan setup dialog created a same-named workspace on Enter. Both of its create actions now confirm first, and it offers the caller's own namesake. Pickers preselect only a namesake the caller owns and label a colleague's. Names compare under a collator pinned to `en`. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172qrhMa5TQgETASi5hxMqD
Re-review disposition — c7c18d2MAJOR: the setup dialog's create actions: fixed. Details are on the inline thread. Minor
Nits: all addressed.
Verification
|
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…isted it Workspace ids are per tenant, so an account switch while a picker or the setup dialog is open could link a different workspace with the same id and send memory there. A switch since the list loaded now links nothing, and the record and seed are pinned to that account. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172qrhMa5TQgETASi5hxMqD
|
@codex review Round on cb26ed1, covering the changes since 77716eb (setup-dialog gate, owner-aware preselection, account pin). Please try to falsify:
Residual, out of scope: when the workspace list cannot be read, there is nothing to compare against and create proceeds without the confirmation. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb26ed1bee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Listed workspaces already named what a quick create would use. The picker opens on | ||
| // the caller's own (never a colleague's), and creating another namesake is confirmed. |
There was a problem hiding this comment.
If credentials switch after listDatamates() completes but while the CLI prompt is open, this newly added whoami() runs as the new account and compares its user id with rows from the old account; after selection, bindOrRebind() also captures that new account and can bind the same numeric workspace id there. Because workspace and owner ids are tenant-local, this can preselect an unrelated row and send the project's memory to an unintended workspace instead of linking nothing. Capture one credential before loading the list and use it for the list, ownership lookup, bind, record, and seed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed; fixed in 9713c92. The link flows now capture the credential once and pass it to every request (actAs): the CLI before it lists workspaces, the TUI picker on mount, the setup dialog in runFlow (and only when it is still the account the flow started under). The list, whoami, bindExisting and both rebind endpoints (including the CLI's 409 fallback) run as that credential; the record and seed are pinned to its digest. A configured account that differs before the bind still links nothing; one that changes during the bind cannot redirect the request, only stop the local record and seed. Wiring tests assert the list and the bind ran as the captured credential (fail with the bind unpinned).
| accountChanged() | ||
| return | ||
| } | ||
| try { |
There was a problem hiding this comment.
When credentials change after this check resolves but before WorkspaceApi.bindExisting() or the rebind helper resolves its own credentials, the request still executes under the new account because those APIs read ambient credentials independently. The later recordApprovedBinding check can suppress the local record and seed, but it cannot undo the server-side bind that already occurred under another tenant, so the UI may claim nothing was linked even though another account's project was modified. Pass the captured credential through to the bind request rather than relying on a pre-request digest comparison.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed; fixed in 9713c92. The link flows now capture the credential once and pass it to every request (actAs): the CLI before it lists workspaces, the TUI picker on mount, the setup dialog in runFlow (and only when it is still the account the flow started under). The list, whoami, bindExisting and both rebind endpoints (including the CLI's 409 fallback) run as that credential; the record and seed are pinned to its digest. A configured account that differs before the bind still links nothing; one that changes during the bind cannot redirect the request, only stop the local record and seed. Wiring tests assert the list and the bind ran as the captured credential (fail with the bind unpinned).
…ntial A digest check before the bind left a window where the request still ran as whatever account was configured. The CLI and TUI link flows now capture the credential once and pass it to the list, `whoami`, bind and rebind, and pin the record and seed to it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172qrhMa5TQgETASi5hxMqD
| ): Promise<{ namesakes: Namesakes<DatamateRef>; listedAs: ActAs | null }> { | ||
| const none = { namesakes: findNamesakes([] as DatamateRef[], defaultName, undefined), listedAs: null } | ||
| const actAs = await WorkspaceApi.captureCredentials() | ||
| if (!actAs || digestOf(actAs) !== flowAccount) return none |
There was a problem hiding this comment.
WARNING: Abort or refresh the offer when its account changes
This new mismatch branch returns an empty namesake result without attempting the list, but both runFlow callers still open OfferDialog. For example, start a post-scan flow under A, switch to B while the pre-check is pending, and let it return unbound. If B already has an analytics workspace and browser handoff is unavailable, this branch discards discovery, the dialog defaults to quick create, and Enter reaches createAndBindInline under B without any duplicate-name confirmation. This introduces a bypass even when B's workspace list is readable; previously namesakesFor would fetch that list and gate creation. On account drift, stop/restart the flow or refresh discovery and the pre-check coherently under the new credential instead of presenting an unchecked create action. Add coverage for a switch before namesake discovery, not only after the offer has loaded.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172qrhMa5TQgETASi5hxMqD
Resolves `state.ts` in favour of main's `credentialDigest`; the link flows now call it directly. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172qrhMa5TQgETASi5hxMqD
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Re-review at cd23385: approving
Every item from my earlier review is fixed, and I verified each one against the code rather than the disposition comment.
| Earlier finding | Status |
|---|---|
MAJOR: the post-scan OfferDialog created namesakes unchecked |
Fixed. runFlow passes namesakes in. Both create actions go through createUnlessNamesake and the shared No-default NamesakeConfirmDialog. Your own namesake becomes the first, default choice. |
| The collator followed the host locale | Fixed. NAME_COLLATOR is pinned to en and hoisted, and a test pins resolvedOptions().locale. |
| The TUI showed names less safely than the CLI | Fixed. stripBidiControls and displayWorkspaceName cover the title, the Yes option and the CLI prompt. |
| The default Enter linked to a colleague's namesake | Fixed. The picker opens on your own namesake only (ownerId === whoami). A colleague's row is labelled and not preselected. |
| No tests for the picker or confirm behavior | Fixed. Pure matrices plus wiring tests that render the real dialogs. |
| Contract edges | Fixed. NFC/NFD, ignorable characters, Danish aa/å and the locale pin are all in the matrix. |
Nits ((codex), ● gutter, comment wording) |
Fixed. |
Verification on this head:
- The link, plugin, CLI-link and adversarial suites pass: 147 pass, 0 fail.
tsgo --noEmitis clean.- I mutation-tested the gates. Each of these makes the suite fail, so the tests prove what they claim:
- removing the quick-create gate in
OfferDialog(2 failures); - removing its browser gate (1);
- reverting the collator to
"und"(1); - making
confirmsNamesakealways false (8); - ignoring
ownerIdinfindNamesakes(9).
- removing the quick-create gate in
Non-blocking residuals. All of these need the user to switch Altimate accounts within the few seconds a flow is running. Each falls back to the pre-PR behavior or to an existing guard, and none sends data to an account the user didn't choose:
-
kilo's open WARNING (
namesakesFor): an account change between the flow's start and the namesake lookup gives an empty namesake set, and the setup dialog offers create without the confirmation. The doc comment onnamesakesForalready discloses this. Worth a one-line reply on that thread. -
The two cubic threads on
link.tsshow "✅ Addressed in cd23385", but that commit is the merge frommain, and the code is unchanged:- the pre-check
getBindingForProject(:235) still runs on ambient credentials beforecaptureCredentials()(:250); - quick create (
createThenBindOrRebind,:384) doesn't takeactAs.
Both are covered in practice: the rebind's
expectedCurrentDatamateIdprecondition and the create path's ownaccountDigestcheck. ThreadingactAsthrough both would make the CLI consistent with the TUI. That can be a follow-up, but the threads should get a reply rather than the bot's auto-resolution. - the pre-check
2 participants
Issue for this PR
Closes #1378
Type of change
What does this PR do?
With no workspace linked, every place that offers a quick create (the
altimate-code linkCLI, the TUI's on-demand picker, and the setup dialog shown after a project scan) made a second workspace with the project's name on a plain Enter, even when one already existed.en, so the host locale can't change the result. "Straße" matches "STRASSE"; "café" and "cafe", or dotless and dotted i, stay different.Not covered: when the workspace list can't be read, there is nothing to compare against, and create works as before.
How did you verify your code works?
Matrix tests for the helper: matching (case, sharp S, final sigma, ligatures, NFC and NFD, zero-width and bidi characters, dotless i, accents, Danish aa), which namesake may be preselected, the row labels, where a picker opens, and which choices confirm.
Wiring tests render the setup dialog and the on-demand picker against a stand-in select and drive their callbacks:
Each test fails when its gate is removed.
Typecheck is clean. The workspace, plugin and CLI link suites pass.
The CLI and the TUI picker were driven in a terminal against a local stand-in workspace API. The setup dialog opens only after an agent's project scan, so it was covered by the wiring tests rather than driven.
Screenshots / recordings
An unlinked "analytics" repo. Before (main): the picker opens on create, and Enter makes a second "analytics".
After, with only a colleague's "analytics": it is labelled, not preselected.
Enter on create asks first, with No as the default, and No changes nothing.
After, with your own "analytics": the picker opens on it.
The TUI picker, in the same two cases, and its confirmation:
Checklist
🤖 Generated with Claude Code
https://claude.ai/code/session_0172qrhMa5TQgETASi5hxMqD