Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds terminal-focus tracking and focus-aware bell/compaction notifications; extends UI queued-input and compaction flow (pre-submit/front/back queuing, deferred compaction notifications); improves ask-question multi-line/markdown rendering; adds a file-read context post-processor; normalizes cancellation error messages across tool/manager/gateway/client; and changes macOS launchd bootstrap recovery to replace stale loaded services. ChangesTerminal focus, bell hooks, UI wiring & queued-input / compaction flow
Ask-question multi-line and markdown rendering
File-read context post-processing (shell tools)
Cancellation/error messaging across manager, tools, gateway, and client
Launchd service bootstrap/reload recovery (macOS)
Minor updates & docs
Sequence Diagram(s)sequenceDiagram
participant Terminal
participant UI as UI Model
participant Focus as Terminal Focus
participant Bell as Bell Hooks
participant Runtime as Turn Queue Hook
Terminal->>UI: tea.FocusMsg / tea.BlurMsg
UI->>Focus: MarkFocused / MarkBlurred
Focus-->>UI: Focus State Updated
UI->>UI: dispatchQueuedInput (queued /compact)
UI->>UI: startQueuedCompaction
UI->>Runtime: Notify compaction started
Runtime-->>Bell: OnTurnQueueDrained
Bell->>Focus: FocusedForAttention?
alt Terminal Focused
Bell-->>UI: Suppress Notification
else Terminal Unfocused
Bell->>Terminal: notifyIfUnfocused (Compaction Finished)
end
UI->>Runtime: OnUserCompactionCompleted(queueDrained)
Runtime->>Bell: Handle Completion
sequenceDiagram
participant User
participant Client
participant Gateway
participant Tool as Tool (write_stdin / exec_command)
participant Manager
User->>Client: RPC request (with cancel)
Client->>Gateway: send request
Gateway->>Tool: invoke tool
Tool->>Manager: background write / start
Manager-->>Tool: context.Canceled / canceled during polling
Tool->>Tool: formatToolCallError(..., err)
Tool-->>Gateway: ErrorResultWith(formatted message)
Gateway->>Client: protocol.ErrCodeRequestCanceled + "request canceled by client"
Client->>User: requestCanceledError (unwraps to context.Canceled)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
shared/client/remote_rpc.go (1)
417-421:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle empty request-canceled messages before applying the generic fallback.
ErrCodeRequestCanceledcurrently receives"protocol request failed"whenresp.Messageis empty, so it no longer normalizes to the cancellation message.Proposed fix
func protocolError(resp *protocol.ResponseError) error { if resp == nil { return nil } message := strings.TrimSpace(resp.Message) - if message == "" { - message = "protocol request failed" - } + if resp.Code == protocol.ErrCodeRequestCanceled { + return requestCanceledError{message: message} + } + if message == "" { + message = "protocol request failed" + } switch resp.Code { @@ - case protocol.ErrCodeRequestCanceled: - return requestCanceledError{message: message} default: return errors.New(message) } }Also applies to: 450-451
🤖 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 `@shared/client/remote_rpc.go` around lines 417 - 421, The code currently sets a generic fallback message when resp.Message is empty which overwrites the cancellation normalization; update the logic in remote_rpc.go so that before applying the generic "protocol request failed" fallback you check if resp.Code == ErrCodeRequestCanceled (and resp.Message is empty) and set message = "request canceled" (or the normalized cancellation text) instead, and only then apply the generic fallback for other codes; make the same change around the other occurrence referenced (near the second block at the 450-451 area) so canceled responses preserve the cancellation message.cli/builder/service_backend_darwin_test.go (1)
107-203:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd tests for fallback failure branches (
bootoutfail and retrybootstrapfail).The new recovery behavior is tested for success, but not for its two failure exits. That leaves part of the new business logic unguarded.
As per coding guidelines:
{server,shared,cli,cmd}/**/*.go: “All business logic must be covered by tests; production code must be written to be unit-testable”.🤖 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 `@cli/builder/service_backend_darwin_test.go` around lines 107 - 203, Add unit tests covering the two failure branches when recovering from a transient bootstrap error: one where the fallback bootout command fails and one where the retry bootstrap also fails. Duplicate the existing TestLaunchdRestartIfInstalledReplacesStaleLoadedServiceAfterTransientBootstrapError and TestLaunchdStartReplacesStaleLoadedServiceAfterTransientBootstrapError patterns but modify the captured command handler in captureLaunchdServiceCommands to return an error/result for the "launchctl bootout gui/.../<serviceLaunchdLabel>" case (simulate failure) and another variant where the second "launchctl bootstrap gui/... <path>" returns a non-zero Code/Err (simulate retry bootstrap failure), then assert that serviceSubcommand(...{"restart","--if-installed"}) and (launchdServiceBackend{}).Start(...) return the expected non-zero exit/code error and that *calls records the expected sequence (including failed bootout/failed bootstrap) using countLaunchdCommand and the same helpers used by the existing tests.cli/app/ui_input_submission.go (1)
39-44:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPrepending here can reorder older queued work.
m.queuedcan already contain earlier follow-ups whilependingQueuedDrainAfterHydrationis waiting on transcript sync. In that state, a fresh Enter hits this branch and inserts the new submit at index 0, so it runs before the older queued items. That breaks FIFO and the queue-order preservation this PR is trying to restore.Please keep direct submits behind existing queued work, or split the “active pre-submit item” from
m.queuedso only queued-drain paths reinsert at the front.🤖 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 `@cli/app/ui_input_submission.go` around lines 39 - 44, The current prepend (m.queued = append([]string{text}, m.queued...)) can make a new direct submit jump ahead of earlier queued items; change the behavior so direct submits do not reorder FIFO: either append the new text to the end of m.queued (m.queued = append(m.queued, text)) so it runs after existing queued work, or stop mutating m.queued here and keep the submit only in m.pendingPreSubmitText (i.e., split the “active pre-submit item” from m.queued) so only the queued-drain code reinserts items at the front; update the branch in m.hasRuntimeClient() that touches preSubmitCheckToken, pendingPreSubmitText, and m.queued accordingly.
🧹 Nitpick comments (1)
server/tools/shell/postprocess/runner_test.go (1)
162-168: 💤 Low valueAdd clarifying comments to document intentional asymmetry in test file setup
The asymmetry between
TestRunnerBuiltinFileReadAddsTotalLineCountForPartialSed(line 89, WITH trailing newline) andTestRunnerBuiltinFileReadAddsTotalLineCountForHeadTailAwkAndPowerShell(line 162, WITHOUT trailing newline) is intentional—the tests verify correct line counting in both POSIX-compliant and non-POSIX file scenarios. The implementation'scountSmallRegularFileLines()correctly handles both cases via itsif seenBytes && !endedWithNewlinecheck.Consider adding a comment to line 162 briefly explaining the intentional difference:
// File without trailing newline — verify non-POSIX file handling🤖 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 `@server/tools/shell/postprocess/runner_test.go` around lines 162 - 168, Add a brief clarifying comment above the test file creation at the writeTextFile call in TestRunnerBuiltinFileReadAddsTotalLineCountForHeadTailAwkAndPowerShell explaining that this file intentionally omits a trailing newline to exercise non-POSIX behavior (e.g., "// File without trailing newline — verify non-POSIX file handling"); reference the asymmetry with TestRunnerBuiltinFileReadAddsTotalLineCountForPartialSed and note that countSmallRegularFileLines() handles the seenBytes && !endedWithNewline case.
🤖 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 `@cli/builder/service_backend_darwin_test.go`:
- Around line 158-160: Replace the substring-based assertion using
strings.Contains on stdout with an exact-line equality or structured check:
capture/produce the expected full line (e.g., expected := "Restarted Builder
background service.") and assert stdout.String() == expected (or scan stdout
with bufio.Scanner and assert one scanned line equals expected). Update the test
assertion that currently references stdout and strings.Contains to use the exact
equality/line match so the check in the test function verifies the precise
output rather than a substring.
In `@prompts/skills/skill-creator/SKILL.md`:
- Line 6: Change the opening sentence in SKILL.md from "Skills is a specialized
technical documentation standard..." to use correct subject-verb agreement by
replacing "Skills is" with "Skills are", i.e., update the opening definition
line that currently starts with the phrase "Skills is …" so it reads "Skills are
…".
- Line 61: Update the ambiguous header rule in SKILL.md to clarify that the
prohibition is against adding a global H1 header and against inserting extra
blank lines after headers; replace "Do not add spaces after headers" with a
clearer phrase such as "Do not include a global H1 header like `# My Skill`. Do
not add extra blank lines immediately after header lines." Reference this change
in the SKILL.md header rules so skill creators follow standard Markdown spacing
(e.g., `# Title`) while avoiding unwanted blank lines.
In `@server/tools/shell/postprocess/file_read_context.go`:
- Around line 69-80: The classifier is mislabeling many full-file awk
invocations as partial reads because awkProgramMentionsNR is too broad; change
classifyFileRead to stop handling awk variants by removing or disabling the
"awk", "gawk", "mawk", "nawk" case (i.e., no longer call classifyAwkFileRead
from classifyFileRead and instead return (fileReadCandidate{}, false) for awk),
and update or remove classifyAwkFileRead/awkProgramMentionsNR usage until a
safer, bounded-read allowlist is implemented; ensure tests reflect that awk
commands are treated as unknown/unsupported rather than partial reads.
In `@server/transport/gateway_test.go`:
- Around line 125-129: Replace the substring checks that use strings.Contains on
err.Error() with exact equality assertions against the precomputed message
variable: instead of checking if err == nil || !strings.Contains(err.Error(),
"Canceled polling by user, process active") and similarly rejecting "context
canceled" substrings, assert err != nil and err.Error() == message (and remove
the second Contains check). Update the t.Fatalf messages to reflect exact
mismatch (e.g., expected %q, got %q) and reference the err variable and message
variable in the assertions.
In `@shared/client/remote_test.go`:
- Around line 9-10: Remove the substring-based assertion that uses
strings.Contains to check for cancellation; instead assert the exact
cancellation error/message. Locate the test assertions in remote_test.go that
call strings.Contains(err.Error(), "...canceled...") (also around the region
mentioned ~642-647) and replace them with an exact comparison to the canonical
cancellation value (for example compare err.Error() == context.Canceled.Error()
or assert.Equal(t, context.Canceled, errors.Unwrap(err)) as appropriate), and
then remove the now-unused "strings" import.
---
Outside diff comments:
In `@cli/app/ui_input_submission.go`:
- Around line 39-44: The current prepend (m.queued = append([]string{text},
m.queued...)) can make a new direct submit jump ahead of earlier queued items;
change the behavior so direct submits do not reorder FIFO: either append the new
text to the end of m.queued (m.queued = append(m.queued, text)) so it runs after
existing queued work, or stop mutating m.queued here and keep the submit only in
m.pendingPreSubmitText (i.e., split the “active pre-submit item” from m.queued)
so only the queued-drain code reinserts items at the front; update the branch in
m.hasRuntimeClient() that touches preSubmitCheckToken, pendingPreSubmitText, and
m.queued accordingly.
In `@cli/builder/service_backend_darwin_test.go`:
- Around line 107-203: Add unit tests covering the two failure branches when
recovering from a transient bootstrap error: one where the fallback bootout
command fails and one where the retry bootstrap also fails. Duplicate the
existing
TestLaunchdRestartIfInstalledReplacesStaleLoadedServiceAfterTransientBootstrapError
and TestLaunchdStartReplacesStaleLoadedServiceAfterTransientBootstrapError
patterns but modify the captured command handler in
captureLaunchdServiceCommands to return an error/result for the "launchctl
bootout gui/.../<serviceLaunchdLabel>" case (simulate failure) and another
variant where the second "launchctl bootstrap gui/... <path>" returns a non-zero
Code/Err (simulate retry bootstrap failure), then assert that
serviceSubcommand(...{"restart","--if-installed"}) and
(launchdServiceBackend{}).Start(...) return the expected non-zero exit/code
error and that *calls records the expected sequence (including failed
bootout/failed bootstrap) using countLaunchdCommand and the same helpers used by
the existing tests.
In `@shared/client/remote_rpc.go`:
- Around line 417-421: The code currently sets a generic fallback message when
resp.Message is empty which overwrites the cancellation normalization; update
the logic in remote_rpc.go so that before applying the generic "protocol request
failed" fallback you check if resp.Code == ErrCodeRequestCanceled (and
resp.Message is empty) and set message = "request canceled" (or the normalized
cancellation text) instead, and only then apply the generic fallback for other
codes; make the same change around the other occurrence referenced (near the
second block at the 450-451 area) so canceled responses preserve the
cancellation message.
---
Nitpick comments:
In `@server/tools/shell/postprocess/runner_test.go`:
- Around line 162-168: Add a brief clarifying comment above the test file
creation at the writeTextFile call in
TestRunnerBuiltinFileReadAddsTotalLineCountForHeadTailAwkAndPowerShell
explaining that this file intentionally omits a trailing newline to exercise
non-POSIX behavior (e.g., "// File without trailing newline — verify non-POSIX
file handling"); reference the asymmetry with
TestRunnerBuiltinFileReadAddsTotalLineCountForPartialSed and note that
countSmallRegularFileLines() handles the seenBytes && !endedWithNewline case.
🪄 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: CHILL
Plan: Pro
Run ID: 69af86d7-dca2-4b01-82e8-3f417cbc2d54
📒 Files selected for processing (38)
cli/app/embedded_server.gocli/app/runtime_factory.gocli/app/terminal_bell.gocli/app/terminal_bell_test.gocli/app/terminal_focus.gocli/app/turn_queue_hooks.gocli/app/ui.gocli/app/ui_ask_controller.gocli/app/ui_focus_test.gocli/app/ui_input_queue.gocli/app/ui_input_submission.gocli/app/ui_loop.gocli/app/ui_part4_test.gocli/app/ui_state.gocli/app/ui_terminal_cursor_test.gocli/app/ui_test.gocli/app/ui_turn_completion_bell_test.gocli/builder/service_backend_darwin.gocli/builder/service_backend_darwin_test.gocli/tui/markdown_renderer.gocli/tui/model_rendering_tools.gocli/tui/model_test.godocs/src/content/docs/command-postprocessing.mdprompts/skills/skill-creator/SKILL.mdprompts/worktree_mode_prompt.mdserver/tools/shell/exec_command_tool.goserver/tools/shell/manager.goserver/tools/shell/manager_types.goserver/tools/shell/postprocess/file_read_context.goserver/tools/shell/postprocess/runner.goserver/tools/shell/postprocess/runner_test.goserver/tools/shell/tool.goserver/tools/shell/tool_test.goserver/tools/shell/write_stdin_tool.goserver/transport/gateway.goserver/transport/gateway_test.goshared/client/remote_rpc.goshared/client/remote_test.go
![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: 09d48897cb
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55d579c998
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
prompts/skills/skill-creator/SKILL.md (1)
67-67: ⚡ Quick winClarify external-reference policy for workspace skills to avoid conflicting interpretation.
This line can be read as forbidding non-repo references, but Line 66 and the “Good” example in this same bullet allow public URLs. Tighten wording so the restriction clearly applies to machine-local file paths, not canonical web docs.
✏️ Proposed wording tweak
-- For workspace skills, only point to files in the skill directory and the repository (workspace). For global skills, avoid pointing to any machine-local files outside the skill dir. +- For workspace skills, point file paths only to files in the skill directory and the repository (workspace). Public canonical URLs are allowed when they are the source of truth. For global skills, avoid pointing to any machine-local files outside the skill dir.🤖 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 `@prompts/skills/skill-creator/SKILL.md` at line 67, The bullet that begins "For workspace skills, only point to files in the skill directory and the repository (workspace)..." is ambiguous; update this sentence in SKILL.md to explicitly restrict only "machine-local file paths" (e.g., ~/Desktop/file, C:\Users\...) while allowing canonical public URLs and repository paths; rephrase to something like: "For workspace skills, do not reference machine-local file paths outside the skill directory (e.g., ~/Desktop/sample.sql); public URLs and repository-relative files are allowed." Ensure the original examples ("Bad: 'Example query at ~/Desktop/sample.sql'"/"Good: 'FlowMVI docs index https://...'") remain consistent with the new wording.server/tools/shell/postprocess/file_read_context.go (1)
17-17: ⚡ Quick winReplace the
\x00-prefixed sentinel string with a typed sed-script item.
unknownSedScriptis a string sentinel placed into the same[]stringas user-supplied sed scripts, andclassifySedScripts/classifySedScriptre-discriminate via string equality (line 329). The\x00prefix makes collisions practically impossible, but the pattern conflates "literal script" and "from-file (unknown content)" into one string-typed channel that has to be re-interpreted later — exactly the kind of brittle string-based metadata the guidelines call out.A small typed wrapper makes the intent explicit and removes the sentinel comparison entirely:
♻️ Suggested refactor sketch
type sedScriptItem struct { text string fromFile bool // -f / --file=...: contents unknown } // classifySedFileRead collects []sedScriptItem instead of []string. // classifySedScripts iterates and treats fromFile as classified-but-not-full // without needing a sentinel value.As per coding guidelines, "Develop type-safe data structures and store structured data or metadata that can be reliably extracted instead of using brittle string-based logic."
Also applies to: 253-326
🤖 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 `@server/tools/shell/postprocess/file_read_context.go` at line 17, Replace the brittle sentinel string unknownSedScript with a small typed wrapper sedScriptItem and change the sed-script collection from []string to []sedScriptItem: define type sedScriptItem { text string; fromFile bool } (fromFile true indicates "-f/--file" unknown contents), update all producers to push sedScriptItem values instead of injecting unknownSedScript, and update classifySedScripts and classifySedScript to use the fromFile flag and text field (remove any string-equality checks against unknownSedScript) so callers can distinguish literal scripts vs. file-origin scripts in a type-safe way.cli/app/ui_input_controller_keys.go (1)
179-183: Consider movinghandleEnteredSlashCommandInputcall to after draft capture for consistency.While the current code is safe—
handleEnteredSlashCommandInputinternally captures and restores the draft even when it callsclearInput()—the new busy path (lines 179–183) calls this function before the draft capture at line 188, whereas the non-busy path calls it after (line 204). This inconsistency in ordering makes the control flow less clear.Moving the busy-path call to after line 188 would align both paths and make the draft-handling strategy uniform across the function, improving code readability without any functional change.
The busy-state safety concern is addressed: the function has a guard (
if m.busy && !command.RunWhileBusy) that prevents non-runnable commands from executing while busy, and the commands that can run while busy do not trigger new submissions.🤖 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 `@cli/app/ui_input_controller_keys.go` around lines 179 - 183, The call to handleEnteredSlashCommandInput is invoked in the busy branch before the draft is captured, causing inconsistent ordering versus the non-busy path; move the handleEnteredSlashCommandInput invocation in the m.busy branch so it runs after the draft capture/restore logic (the same place it is called in the non-busy branch) to make draft handling consistent across both paths—ensure you keep the existing clearInput() behavior intact and preserve the m.busy guard and checks inside handleEnteredSlashCommandInput.
🤖 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 `@server/tools/shell/postprocess/file_read_context.go`:
- Around line 39-82: The classifier functions (classifyFileRead ->
classifySedFileRead/classifyHeadTailFileRead/classifyPowerShellGetContent)
assume ParsedArgs[0] is the command name and call args[1:], which can drop a
real arg if callers set CommandName and ParsedArgs independently; add a boundary
validation before calling classifyFileRead: ensure len(req.ParsedArgs) > 0 &&
req.ParsedArgs[0] == req.CommandName and return early if not, or alternatively
change classifyFileRead signature to accept argsWithoutCmd (and update callers)
and document the invariant in normalizeRequest; reference classifyFileRead,
classifySedFileRead, classifyHeadTailFileRead, classifyPowerShellGetContent,
ParsedArgs and CommandName while making the change.
---
Nitpick comments:
In `@cli/app/ui_input_controller_keys.go`:
- Around line 179-183: The call to handleEnteredSlashCommandInput is invoked in
the busy branch before the draft is captured, causing inconsistent ordering
versus the non-busy path; move the handleEnteredSlashCommandInput invocation in
the m.busy branch so it runs after the draft capture/restore logic (the same
place it is called in the non-busy branch) to make draft handling consistent
across both paths—ensure you keep the existing clearInput() behavior intact and
preserve the m.busy guard and checks inside handleEnteredSlashCommandInput.
In `@prompts/skills/skill-creator/SKILL.md`:
- Line 67: The bullet that begins "For workspace skills, only point to files in
the skill directory and the repository (workspace)..." is ambiguous; update this
sentence in SKILL.md to explicitly restrict only "machine-local file paths"
(e.g., ~/Desktop/file, C:\Users\...) while allowing canonical public URLs and
repository paths; rephrase to something like: "For workspace skills, do not
reference machine-local file paths outside the skill directory (e.g.,
~/Desktop/sample.sql); public URLs and repository-relative files are allowed."
Ensure the original examples ("Bad: 'Example query at
~/Desktop/sample.sql'"/"Good: 'FlowMVI docs index https://...'") remain
consistent with the new wording.
In `@server/tools/shell/postprocess/file_read_context.go`:
- Line 17: Replace the brittle sentinel string unknownSedScript with a small
typed wrapper sedScriptItem and change the sed-script collection from []string
to []sedScriptItem: define type sedScriptItem { text string; fromFile bool }
(fromFile true indicates "-f/--file" unknown contents), update all producers to
push sedScriptItem values instead of injecting unknownSedScript, and update
classifySedScripts and classifySedScript to use the fromFile flag and text field
(remove any string-equality checks against unknownSedScript) so callers can
distinguish literal scripts vs. file-origin scripts in a type-safe way.
🪄 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: CHILL
Plan: Pro
Run ID: d9016530-b942-42c1-9764-f79ed339a30a
📒 Files selected for processing (15)
cli/app/ui_input_controller_commands.gocli/app/ui_input_controller_keys.gocli/app/ui_input_queue.gocli/app/ui_input_submission.gocli/app/ui_part4_test.gocli/builder/service_backend_darwin.gocli/builder/service_backend_darwin_test.godocs/src/content/docs/command-postprocessing.mddocs/src/content/docs/server.mdprompts/skills/skill-creator/SKILL.mdserver/tools/shell/postprocess/file_read_context.goserver/tools/shell/postprocess/runner_test.goserver/transport/gateway_test.goshared/client/remote_rpc.goshared/client/remote_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/src/content/docs/command-postprocessing.md
🚧 Files skipped from review as they are similar to previous changes (5)
- cli/app/ui_input_queue.go
- shared/client/remote_test.go
- shared/client/remote_rpc.go
- cli/app/ui_part4_test.go
- server/tools/shell/postprocess/runner_test.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2340a3dedf
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4e0a9c2fa
ℹ️ 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".
1 participant
Summary
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Documentation