[codex] create sessions from unmatched picker input#1
Merged
Conversation
|
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:
WalkthroughTUI セレクターの Enter 処理を拡張し、絞り込み結果が 0 件のとき入力クエリから新規セッション名を生成・確定できるようにした。関連して表示文言、セッション名正規化、テスト、ドキュメント、.serena メモ群と .gitignore を更新した。 ChangesTUI Select No-Match Session Creation
Serena memories and gitignore
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/select_test.go`:
- Around line 83-86: The test uses unchecked type assertions on updated.(model)
which triggers the lint rule; change both assertions to use the "value, ok :=
updated.(model)" form and if !ok call t.Fatalf (or t.Fatalf with context) so the
test fails instead of panicking; specifically update the two places where
updated, _ = m.Update(...) and m = updated.(model) and where got :=
updated.(model).selected to perform ok-checked assertions against the model type
and handle failures with t.Fatalf including helpful context (e.g., the message
type or updated value).
In `@internal/tui/select.go`:
- Around line 191-194: The code currently returns inputs beginning with "-" when
defaultItem is empty, which can be misinterpreted as CLI flags downstream (e.g.,
in internal/shpool/attach.go); in newSessionName() add a guard: if defaultItem
== "" and strings.HasPrefix(name, "-") then treat it as invalid by returning an
empty string (or another explicit error/indicator used by callers) instead of
returning the raw name, ensuring callers cannot accidentally pass option-looking
session names to shpool attach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce6094c8-34fe-44a6-955b-cfafcda0828e
📒 Files selected for processing (5)
DEV.mdREADME.mdcmd/shp/main.gointernal/tui/select.gointernal/tui/select_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/select.go`:
- Around line 200-205: The current early return in the block checking
defaultItem and strings.HasPrefix(name, "-") bypasses the containsSpace check
and allows names like "-another name"; to fix, ensure you validate
containsSpace(name) before accepting and returning defaultItem+name (or reorder
logic so the combined check is performed): update the conditional around
defaultItem and strings.HasPrefix(name, "-") in the function that uses
containsSpace to first reject when containsSpace(name) is true (return "",
false) and only then apply the defaultItem concatenation rule, or combine the
conditions so the concatenation only happens when !containsSpace(name).
In `@README.md`:
- Line 45: Update the sentence that explains Enter-creates-new-session behavior
(the line starting "絞り込み結果が0件の状態で Enter すると...") to also mention the behavior
when the input starts with a '-' and there are NO cwd candidates: clarify that
inputs like "-scratch" will be rejected (no new session created) to avoid
confusion, while keeping the existing note about suffixing when cwd candidates
exist (e.g., "work.company-a.api-another").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a5e78913-cfe3-4996-99c8-0bc49d83824e
📒 Files selected for processing (4)
DEV.mdREADME.mdinternal/tui/select.gointernal/tui/select_test.go
…, suggested commands, task completion, and tech stack
- select.go: reject whitespace before applying the default-suffix rule so inputs like "-another name" no longer produce a session name - select_test.go: use ok-checked type assertions via asModel helper - README.md: document that "-scratch" without cwd candidates is rejected
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.serena/memories/conventions.md:
- Line 6: The file ".serena/memories/conventions.md" is missing a trailing
newline; open the file and ensure the last line ("TUI filtering is
case-insensitive substring AND over whitespace-separated tokens; Backspace
deletes by rune; default cwd item is prepended/deduped and labeled `(cwd)`.") is
terminated with a single newline character so the file ends with exactly one
newline.
In @.serena/memories/core.md:
- Line 6: ファイル末尾に改行がありませんので、.serena/memories/core.md の行 "Read `mem:tech_stack`
for tools/deps, `mem:conventions` for implementation style,
`mem:suggested_commands` for local commands, `mem:task_completion` before
finishing coding tasks." の直後に単一の改行文字を追加してファイルを改行で終わらせてください。
In @.serena/memories/memory_maintenance.md:
- Line 33: The file .serena/memories/memory_maintenance.md is missing a trailing
newline at EOF; update the file so it ends with a single newline character
(i.e., add one blank line after the final line "Checking for stale memories
(e.g. after deletion): Call `serena memories check` for a report.") to conform
to Markdown best practices.
In @.serena/memories/suggested_commands.md:
- Line 7: The file ends without a trailing newline; ensure the markdown file
that contains the line `go run ./cmd/shp --help` is saved with a single newline
character at EOF (add one final '\n' so the file terminates with a single blank
line).
In @.serena/memories/task_completion.md:
- Line 6: task_completion.md currently lacks a trailing newline at EOF; open
task_completion.md and add a single newline character at the end of the file so
the file ends with exactly one trailing newline (no extra whitespace) to satisfy
Markdown best practices.
In @.serena/memories/tech_stack.md:
- Line 5: Add a single trailing newline character to the end of the file so it
ends with a newline; locate the line containing "Build/install/test tasks are
defined in `mise.toml`; plain `go` commands also work." and ensure there is
exactly one newline after that line (i.e., the file terminates with a single
newline).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52d17516-a7ff-4b6c-9a9e-11b1b5059e7f
📒 Files selected for processing (7)
.serena/.gitignore.serena/memories/conventions.md.serena/memories/core.md.serena/memories/memory_maintenance.md.serena/memories/suggested_commands.md.serena/memories/task_completion.md.serena/memories/tech_stack.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/select.go (1)
195-210:⚠️ Potential issue | 🟠 Major | ⚡ Quick win新規セッション名の入力は FromPath と同等の文字制約を通っていない
internal/tui/select.goのnewSessionNameは「空/空白のみ」「空白含み」「先頭-（defaultItem 無し時）」以外は無検証で返すため、foo@bar・foo/bar・foo;cmdのような文字でもそのまま確定され得ます。一方でinternal/session/name.goのFromPathは生成時に許容文字[a-zA-Z0-9 . - _]以外を_に置換するため、セッション名の期待仕様と整合しません。shpool.Attachへ渡るのはシェル経由ではなく argv ですが、セッション名コントラクト上の不整合として入力側でも同様のサニタイズ/拒否を行うのが安全です。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/select.go` around lines 195 - 210, newSessionName currently accepts characters that FromPath would sanitize (internal/session/name.go), causing a contract mismatch; update newSessionName to enforce the same character policy as FromPath by either (A) validating the input against the permitted character set [a-zA-Z0-9 ._-] and returning false for invalid input, or (B) calling/reusing FromPath’s sanitization logic and returning the sanitized result and true; ensure you reference newSessionName and FromPath (or the shared allowed-char regex/validator) so the TUI input behavior matches session name creation and avoids passing disallowed characters into shpool.Attach.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/tui/select.go`:
- Around line 195-210: newSessionName currently accepts characters that FromPath
would sanitize (internal/session/name.go), causing a contract mismatch; update
newSessionName to enforce the same character policy as FromPath by either (A)
validating the input against the permitted character set [a-zA-Z0-9 ._-] and
returning false for invalid input, or (B) calling/reusing FromPath’s
sanitization logic and returning the sanitized result and true; ensure you
reference newSessionName and FromPath (or the shared allowed-char
regex/validator) so the TUI input behavior matches session name creation and
avoids passing disallowed characters into shpool.Attach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3a7ca714-4b0a-44dd-9d00-bf230cdad7ca
📒 Files selected for processing (3)
README.mdinternal/tui/select.gointernal/tui/select_test.go
Free-text picker input bypassed the [A-Za-z0-9._-] normalization that cwd-derived names go through, so inputs like "foo/bar" or "../etc" became session names verbatim, diverging from the documented invariant. - Extract the char-mapping into session.Sanitize and share it between path-derived names (FromPath) and free-text names (newSessionName) - Add tests for normalization (foo/bar -> foo_bar) and full-width-space (U+3000) rejection, a documented Japanese-input boundary - Note the normalization in README/DEV
.claude/ holds machine-local runtime state such as scheduled_tasks.lock and must not be committed.
newSessionName built defaultItem+query for a '-'-prefixed query without
re-checking the combined result, so a cwd-derived default that itself
starts with '-' (e.g. a ~/-repo dir) yielded names like '-repo-another'.
That defeats the function's own option-injection guard: the name is
passed to 'shpool attach --dir . <name>' and misparsed as a flag.
- Reject dash-only input ('-', '--') and combined names that still lead
with '-' in newSessionName
- Defense-in-depth: emit 'shpool attach ... -- <name>' so any positional
name (including an existing '-'-named session selected from the list)
is never treated as an option
- Tests for both paths
After normalization, a query like "foo/bar" can sanitize to an existing session "foo_bar" that the raw-query Filter never matched, so the picker advertised "foo_bar (new)" and Enter silently reattached the existing session (ignoring --dir .). Check the candidate against the session list and label it (existing) vs (new) so the displayed state matches what Enter does. Also assert the Enter quit command in the success-path Update tests, for symmetry with the rejection tests.
orderWithDefault injects the cwd-derived default into m.all even when it is not a running session, so the new (existing)/(new) label treated a synthetic default as existing. Track the real shpool-list sessions separately and decide the label against that set, so an injected default that Enter would create is correctly shown as (new).
- README/DEV: state that leading/trailing whitespace is trimmed and only inputs with inner whitespace are rejected; document the (existing)/(new) label and the dash-leading guard. Split the long rule lines into bullets. - .serena/memories: add trailing newlines; drop a duplicated "shall".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
-anotherのように-から入力した場合は、<cwd-session>-anotherとして作成します。(new)として表示し、README / DEV / help の説明も更新しました。Why
候補に存在しないセッションを TUI から作れず、cwd 派生名にサフィックスを付けた別セッションへすばやく attach できませんでした。
Validation
go test ./...go vet ./...Summary by CodeRabbit
New Features
-は既定項目がある場合のみ既定項目＋サフィックス化）。Bug Fixes / UX
Documentation
Tests
Chores