Feature: allow to overwrite the pi binary to use#9
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (3)
WalkthroughThis PR adds a new optional Possibly related PRs
🚥 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. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. 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: 3
🧹 Nitpick comments (1)
lua/pi/init.lua (1)
38-38: 💤 Low valueDead-code fallback —
cfg.binaryis always truthy.
M.defaults.binary = "pi"guaranteescfg.binaryis nevernilor falsy afterconfig.setup()runs (andvaluesis initialized fromM.defaultseven beforesetupis called). Thelocal binary = { "pi" }line and its surroundingif cfg.binary thenbranch are therefore unreachable dead code. Consider removing the fallback and unwrapping the branch for clarity.♻️ Proposed simplification
- local binary = { "pi" } - if cfg.binary then - if type(cfg.binary) == "table" then - binary = vim.list_extend({}, cfg.binary) - else - binary = { cfg.binary } - end - end + local binary + if type(cfg.binary) == "table" then + binary = vim.list_extend({}, cfg.binary) + else + binary = { cfg.binary } + end🤖 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 `@lua/pi/init.lua` at line 38, The branch that sets a fallback local binary = { "pi" } is dead because M.defaults.binary = "pi" (and config.setup()/values initialization) makes cfg.binary always truthy; remove the unreachable if cfg.binary then ... end block and instead directly use cfg.binary where needed (or assign local binary = cfg.binary without the conditional); update any references to the removed local to use cfg.binary so the code is simpler and no fallback duplication remains.
🤖 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 `@lua/pi/config.lua`:
- Around line 41-43: The validation for opts.binary in config.lua currently only
checks that opts.binary is a string or table but doesn't validate table element
types; update the check so when type(opts.binary) == "table" you iterate its
elements and ensure each element is a string (and error with the same message
"pi.nvim: binary must be a string or list of strings" if any element is not a
string), so invalid values like {123, true} fail validation before runtime.
In `@lua/pi/init.lua`:
- Around line 40-46: The code currently assigns binary = cfg.binary and then
calls vim.list_extend(binary, ...) which mutates cfg.binary; to fix, ensure you
shallow-copy cfg.binary before extending it (e.g., create a new table local
binary = vim.deepcopy(cfg.binary) or iterate to copy elements) so
vim.list_extend operates on a fresh table; update the code paths that set binary
from cfg.binary (the branch that assigns binary = cfg.binary) so the subsequent
vim.list_extend call does not modify cfg.binary and then build cmd from that
copied binary.
In `@README.md`:
- Line 80: The `binary` table description has a truncated sentence; update the
`binary` row so the clause completes the thought (e.g., change the fragment
"When omitted or set to `\"pi\"`." to a full sentence like "When omitted or set
to `\"pi\"`, the runtime defaults to the system `pi` binary.") — edit the
`binary` entry so it clearly states the default behavior and accepted types
(string or array of strings) referenced by the `binary` field.
---
Nitpick comments:
In `@lua/pi/init.lua`:
- Line 38: The branch that sets a fallback local binary = { "pi" } is dead
because M.defaults.binary = "pi" (and config.setup()/values initialization)
makes cfg.binary always truthy; remove the unreachable if cfg.binary then ...
end block and instead directly use cfg.binary where needed (or assign local
binary = cfg.binary without the conditional); update any references to the
removed local to use cfg.binary so the code is simpler and no fallback
duplication remains.
🪄 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: dddf9dfb-a229-46ad-9700-465a9fa52c34
📒 Files selected for processing (3)
README.mdlua/pi/config.lualua/pi/init.lua
While this is the case, I am leaving it. I did this to align with how other configuration values are handled, such as |
pablopunk
left a comment
pablopunk
left a comment
There was a problem hiding this comment.
yeah this is a no brainer, very good to have. Can you update the config example in the Readme? I try to keep all configs there
require("pi").setup({
-- ...
pi = "~/.bin/pi", -- or { "env", "FOO=1", "pi-wrapper" }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@README.md`:
- Line 58: Update the README example to use the correct config key name: replace
the shown `pi = "~/.bin/pi"` entry with `binary = "~/.bin/pi"` (or the
equivalent env wrapper form) so the documented option matches the implemented
configuration key `binary` instead of the nonexistent `pi`.
🪄 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: c0a08e3a-d41e-4df2-ab22-0cec9d2beb5f
📒 Files selected for processing (3)
README.mdlua/pi/config.lualua/pi/init.lua
🚧 Files skipped from review as they are similar to previous changes (2)
- lua/pi/config.lua
- lua/pi/init.lua
|
|
||
| ```lua | ||
| require("pi").setup({ | ||
| pi = "~/.bin/pi", -- or { "env", "FOO=1", "pi-wrapper" } |
There was a problem hiding this comment.
Use the correct config key in the setup example (binary, not pi).
Line 58 documents pi = ..., but the option introduced by this PR is binary. As written, users will configure a non-existent key.
Suggested doc fix
require("pi").setup({
- pi = "~/.bin/pi", -- or { "env", "FOO=1", "pi-wrapper" }
+ binary = "~/.bin/pi", -- or { "env", "FOO=1", "pi-wrapper" }
provider = "openrouter",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pi = "~/.bin/pi", -- or { "env", "FOO=1", "pi-wrapper" } | |
| binary = "~/.bin/pi", -- or { "env", "FOO=1", "pi-wrapper" } |
🤖 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 `@README.md` at line 58, Update the README example to use the correct config
key name: replace the shown `pi = "~/.bin/pi"` entry with `binary = "~/.bin/pi"`
(or the equivalent env wrapper form) so the documented option matches the
implemented configuration key `binary` instead of the nonexistent `pi`.
Signed-off-by: Jakob Beckmann <f4z3r-github@pm.me>
|
thankssssss |
2 participants
First of all, thanks for the plugin. Just the kind of minimal thing I love.
I am using a wrapper for
piin order to isolate the agent on my system to reduce blast radius in case it starts going crazy. Especially since I don't want it to pollute my system with stuff when it autonomously installs tools that it needs.Therefore it would be great if the binary/command that is used to start pi could be customized. I initially tried to achieve this simply using a shell alias. However, this is not ideal as I want a different wrapper for nvim than for the alias that I use in the shell. This quickly becomes messy, so I reverted to this small change.
I am opening this PR as I am guessing others might want similar flexibility.
Summary by CodeRabbit
New Features
Documentation