Conversation
…l 5 permissionMode values BREAKING CHANGE: The `mode` field in frontmatter and `/mode` slash command have been removed as they were non-functional (Agent SDK ignores --mode parameter). Use `permission_mode` field and `/permission` command instead. Changed: - Remove mode field from frontmatter (vibing.nvim: true sessions) - Remove /mode slash command handler and registration - Update /permission command completion to include all 5 valid permissionMode values: - default: Standard permission behavior with canUseTool callbacks - acceptEdits: Auto-approve file operations (Edit, Write, mkdir, etc.) - bypassPermissions: Auto-approve ALL tools (use with caution) - plan: Read-only mode, no tool execution (planning only) - dontAsk: TypeScript-only, deny instead of prompting - Update PERMISSION_MODES constant to include all 5 values - Remove mode parameter passing from command_builder.lua - Remove --mode argument parsing from args-parser.ts - Remove mode option from agent-wrapper.ts - Update documentation (architecture.md, commands-reference.md, README.md, README.ja.md) Reason: Agent SDK only supports permissionMode parameter, not mode. The mode parameter was completely ignored by the SDK, making it non-functional. Migration: - Existing chat files with mode: field are harmless (simply ignored) - Use /permission command to set permission mode instead of /mode - Configure permissions.mode in setup() instead of agent.default_mode
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR removes the old "mode" concept across CLI, commands, frontmatter, rendering, and adapter plumbing; replaces it with permission-mode semantics (expanded with Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI / Agent Wrapper
participant Chat as Chat Module
participant Storage as Frontmatter Store
participant Agent as Agent SDK
User->>Chat: issue `/permission plan`
Chat->>Storage: update frontmatter `permissions_mode: plan`
Storage-->>Chat: update success
User->>CLI: start session (no --mode flag)
CLI->>Agent: buildQueryOptions(permissionMode=plan)
User->>Chat: send query
Chat->>Agent: execute query with permissions metadata
Agent-->>Chat: result
Chat-->>User: display response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can get early access to new features in CodeRabbit.Enable the |
Contributor
コードレビューこのPRは、Agent SDKがサポートしていない 🚨 重大な問題1. 定数の不一致 (Critical Bug)
現状: -- lua/vibing/application/chat/handlers/permission.lua:3
local VALID_MODES = { "default", "acceptEdits", "bypassPermissions" } -- ❌ 古い3値-- lua/vibing/core/constants/modes.lua:11
M.PERMISSION_MODES = { "default", "acceptEdits", "bypassPermissions", "plan", "dontAsk" } -- ✅ 新しい5値影響:
修正方法: -- lua/vibing/application/chat/handlers/permission.lua
local modes_constants = require("vibing.core.constants.modes")
local VALID_MODES = modes_constants.PERMISSION_MODESまたは、 local modes_constants = require("vibing.core.constants.modes")
-- ...
if not modes_constants.is_valid_permission_mode(mode) then
notify.error(string.format("Invalid permission mode: %s", mode))
notify.info("Valid modes: " .. table.concat(modes_constants.PERMISSION_MODES, ", "))
return false
end
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
47-51:⚠️ Potential issue | 🟡 MinorFix pipeline failures flagged by CI.
The CI linting reported two issues:
- Line 47: Line length exceeds 120 characters (actual: 121)
- Line 51: Fenced code block missing language specifier
Per coding guidelines, run
npm run lint:mdto identify and fix these issues before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 47 - 51, The CI raised two markdown lint issues: shorten the long sentence containing "Claude **direct access to your Neovim instance**" so it is <=120 characters (break into two sentences or wrap the clause across lines) and add a language specifier to the fenced code block that starts around the phrase "Autonomously explore your codebase" (e.g., ```markdown or ```text) to satisfy the fenced-code-block-language rule; after making these edits run npm run lint:md and fix any remaining warnings before merging.lua/vibing/core/constants/modes.lua (1)
9-25:⚠️ Potential issue | 🔴 CriticalCritical: Four locations have hardcoded permission mode lists missing
plananddontAsk.The modes are correctly defined in
M.PERMISSION_MODEShere, but the following files still use outdated hardcoded lists and must be updated:
lua/vibing/application/chat/handlers/permission.lua(line 3): Handler'sVALID_MODESrejects the new modeslua/vibing/infrastructure/completion/providers/frontmatter.lua(lines 19-23): Completion suggestions don't includeplan/dontAsklua/vibing/config.lua(line 378): Config validation rejects the new modes with misleading error messagelua/vibing/core/types.lua(line 45): Type annotation omits the new modesWithout updating all four, users will encounter validation failures, incomplete completions, and type mismatches despite the modes being added to the central constants.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/vibing/core/constants/modes.lua` around lines 9 - 25, Update the four places that still use hardcoded permission-mode lists so they include the new "plan" and "dontAsk" modes or, better, reference the central M.PERMISSION_MODES constant: replace the handler's VALID_MODES (symbol: VALID_MODES in the permission handler) with M.PERMISSION_MODES or append "plan" and "dontAsk"; update the frontmatter completion provider's hardcoded suggestions/allowed modes (symbol: the completion provider's modes/suggestions list) to include those two modes or reference M.PERMISSION_MODES; change the config validation logic (symbol: the config validation function or CONFIG_SCHEMA/validation that constructs the error message) to accept "plan" and "dontAsk" and update the validation error message to reflect all allowed modes; and update the PermissionMode type annotation (symbol: the type union in core/types.lua) to include 'plan' and 'dontAsk'. Ensure all four places either import/use the central M.PERMISSION_MODES constant or explicitly add the two modes so runtime validation, completions, config errors, and type annotations stay consistent.
🧹 Nitpick comments (1)
lua/vibing/infrastructure/storage/frontmatter.lua (1)
224-226: Minor inconsistency in line limits between file and buffer checks.
is_vibing_chat_filereads 20 lines whileis_vibing_chat_buffer(line 246) reads 50 lines. The comment at line 244-245 explains the buffer version was increased to "capture long permission arrays." If permission arrays can exceed 20 lines,is_vibing_chat_filemight fail to detect valid chat files in edge cases.Consider using the same limit (50) for consistency, or document why 20 is sufficient for file detection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/vibing/infrastructure/storage/frontmatter.lua` around lines 224 - 226, The file-reader version is only concatenating the first 20 lines in is_vibing_chat_file while is_vibing_chat_buffer reads 50 lines, which can miss long permission arrays; update is_vibing_chat_file to read the first 50 lines (change the 20 argument in vim.fn.readfile used in is_vibing_chat_file) so both functions use the same limit, or alternatively add a clear comment in is_vibing_chat_file explaining why 20 lines is guaranteed sufficient for file-based detection if you choose to keep it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/vibing/application/chat/init.lua`:
- Around line 61-65: The command description lists five modes but the handler
validation in lua/vibing/application/chat/handlers/permission.lua still only
allows three; update the validation logic in the permission handler (the
function that parses/validates the mode input in permission.lua) to accept the
full set of modes defined in modes.lua (default, acceptEdits, bypassPermissions,
plan, dontAsk) and ensure any error messages or usage text in that handler match
the commands.register description; also reconcile any enum/list in modes.lua
with the handler so both use the same canonical source of allowed modes.
---
Outside diff comments:
In `@lua/vibing/core/constants/modes.lua`:
- Around line 9-25: Update the four places that still use hardcoded
permission-mode lists so they include the new "plan" and "dontAsk" modes or,
better, reference the central M.PERMISSION_MODES constant: replace the handler's
VALID_MODES (symbol: VALID_MODES in the permission handler) with
M.PERMISSION_MODES or append "plan" and "dontAsk"; update the frontmatter
completion provider's hardcoded suggestions/allowed modes (symbol: the
completion provider's modes/suggestions list) to include those two modes or
reference M.PERMISSION_MODES; change the config validation logic (symbol: the
config validation function or CONFIG_SCHEMA/validation that constructs the error
message) to accept "plan" and "dontAsk" and update the validation error message
to reflect all allowed modes; and update the PermissionMode type annotation
(symbol: the type union in core/types.lua) to include 'plan' and 'dontAsk'.
Ensure all four places either import/use the central M.PERMISSION_MODES constant
or explicitly add the two modes so runtime validation, completions, config
errors, and type annotations stay consistent.
In `@README.md`:
- Around line 47-51: The CI raised two markdown lint issues: shorten the long
sentence containing "Claude **direct access to your Neovim instance**" so it is
<=120 characters (break into two sentences or wrap the clause across lines) and
add a language specifier to the fenced code block that starts around the phrase
"Autonomously explore your codebase" (e.g., ```markdown or ```text) to satisfy
the fenced-code-block-language rule; after making these edits run npm run
lint:md and fix any remaining warnings before merging.
---
Nitpick comments:
In `@lua/vibing/infrastructure/storage/frontmatter.lua`:
- Around line 224-226: The file-reader version is only concatenating the first
20 lines in is_vibing_chat_file while is_vibing_chat_buffer reads 50 lines,
which can miss long permission arrays; update is_vibing_chat_file to read the
first 50 lines (change the 20 argument in vim.fn.readfile used in
is_vibing_chat_file) so both functions use the same limit, or alternatively add
a clear comment in is_vibing_chat_file explaining why 20 lines is guaranteed
sufficient for file-based detection if you choose to keep it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e854f517-12a1-49f3-b8ac-e23f0d28bdea
📒 Files selected for processing (14)
.claude/rules/architecture.md.claude/rules/commands-reference.mdREADME.ja.mdREADME.mdbin/agent-wrapper.tsbin/lib/args-parser.tslua/vibing/application/chat/commands.lualua/vibing/application/chat/handlers/mode.lualua/vibing/application/chat/init.lualua/vibing/application/commands/handler.lualua/vibing/core/constants/modes.lualua/vibing/infrastructure/adapter/modules/command_builder.lualua/vibing/infrastructure/storage/frontmatter.lualua/vibing/presentation/chat/modules/renderer.lua
💤 Files with no reviewable changes (6)
- bin/agent-wrapper.ts
- lua/vibing/infrastructure/adapter/modules/command_builder.lua
- lua/vibing/application/commands/handler.lua
- lua/vibing/application/chat/handlers/mode.lua
- lua/vibing/presentation/chat/modules/renderer.lua
- bin/lib/args-parser.ts
Agent SDK only supports 3 permissionMode values, not 5: - default - acceptEdits - bypassPermissions Removed unsupported values (plan, dontAsk) from: - PERMISSION_MODES constant - /permission command completion - Command description - Documentation (architecture.md, commands-reference.md, README.md, README.ja.md) This fixes the error: Invalid --permission-mode value: "plan". Must be one of: default, acceptEdits, bypassPermissions
Contributor
コードレビュー結果このPRは 🔴 重大な問題1. PRタイトルが不正確問題: タイトルに「update permissions to support all 5 permissionMode values」とありますが、実際にサポートするのは3つのみ（ 推奨対応: gh pr edit 401 --title "refactor: remove invalid mode field, update permissions to support 3 permissionMode values"2.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lua/vibing/core/constants/modes.lua (1)
23-25: Use this validator as the single source of truth across handlers.
lua/vibing/application/chat/handlers/permission.luastill keeps its ownVALID_MODESand manual loop (Lines 3, 25-30). Consider routing that handler throughM.is_valid_permission_mode(mode)(and shared constants for display text) to prevent future drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/vibing/core/constants/modes.lua` around lines 23 - 25, The handler duplicates VALID_MODES and a manual loop; replace that local constant and loop in lua/vibing/application/chat/handlers/permission.lua with the shared M.PERMISSION_MODES and the validator M.is_valid_permission_mode(mode), and pull any user-facing display text from the shared constants module instead of inlining strings so the permission validation and display values are single sources of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lua/vibing/core/constants/modes.lua`:
- Around line 23-25: The handler duplicates VALID_MODES and a manual loop;
replace that local constant and loop in
lua/vibing/application/chat/handlers/permission.lua with the shared
M.PERMISSION_MODES and the validator M.is_valid_permission_mode(mode), and pull
any user-facing display text from the shared constants module instead of
inlining strings so the permission validation and display values are single
sources of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5efbbb59-b112-41b9-a1c8-17513d5e1ce8
📒 Files selected for processing (7)
.claude/rules/architecture.md.claude/rules/commands-reference.mdREADME.ja.mdREADME.mdlua/vibing/application/chat/commands.lualua/vibing/application/chat/init.lualua/vibing/core/constants/modes.lua
💤 Files with no reviewable changes (2)
- .claude/rules/commands-reference.md
- lua/vibing/application/chat/init.lua
🚧 Files skipped from review as they are similar to previous changes (3)
- lua/vibing/application/chat/commands.lua
- README.md
- README.ja.md
Root cause: vibing.nvim's args-parser.ts had hardcoded validation that was rejecting valid Agent SDK permissionMode values (plan and dontAsk). The error message made it appear as if Agent SDK didn't support these values, but they were being blocked by vibing.nvim's own validation. Changes: - Updated bin/lib/args-parser.ts validPermissionModes array from 3 to 5 values - Restored lua/vibing/core/constants/modes.lua PERMISSION_MODES to 5 values - Restored lua/vibing/application/chat/commands.lua completion to 5 values - Restored lua/vibing/application/chat/init.lua command description to 5 values - Updated documentation to reflect all 5 supported permissionMode values: - default: Standard permission behavior with canUseTool callbacks - acceptEdits: Auto-approve file operations - bypassPermissions: Auto-approve ALL tools (dangerous) - plan: Read-only planning mode (no tool execution) - dontAsk: Deny instead of prompting Verification: - Build successful - All tests passing - Agent SDK version: @anthropic-ai/claude-agent-sdk ^0.1.76 Fixes #401 (partial - validation bottleneck identified and fixed)
Contributor
コードレビューこのPRを詳細にレビューしました。全体的に非常に良い品質のリファクタリングです。以下、詳細なフィードバックを提供します。 ✅ 高評価ポイント1. 正確な問題把握と解決
2. 一貫性のある変更
3. 優れたドキュメント更新
4. 後方互換性への配慮
|
| 項目 | 評価 |
|---|---|
| コード品質 | ⭐⭐⭐⭐☆ (4/5) |
| 一貫性 | ⭐⭐⭐⭐⭐ (5/5) |
| ドキュメント | ⭐⭐⭐⭐☆ (4/5) |
| テストカバレッジ | ⭐⭐⭐⭐☆ (4/5) |
| 後方互換性 | ⭐⭐⭐⭐⭐ (5/5) |
総合: ⭐⭐⭐⭐☆ (4.2/5)
✅ マージ推奨度
条件付きでマージ推奨 - 以下の修正後:
- 必須: 型定義の不一致を修正(
bin/types.ts:85,permissionModeの型) - 必須:
modeフィールドをAgentConfigから完全削除(bin/types.ts:83) - 推奨:
planとdontAskのサポート状況を明確化(サポートしないなら削除) - 推奨: ドキュメントの矛盾を解消
上記の修正後であれば、このPRは非常に価値のあるリファクタリングとして自信を持ってマージできます。
素晴らしいリファクタリング作業でした！お疲れ様です 👏
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/lib/args-parser.ts (1)
199-200:⚠️ Potential issue | 🟠 MajorUnknown CLI flags are silently accepted and can corrupt prompt parsing.
After removing
--mode, unsupported flags (including legacy ones) are not rejected. Their next token can be treated as a positional prompt, producing misleading behavior instead of a clear CLI error.💡 Proposed fix
- } else if (!args[i].startsWith('--')) { + } else if (!args[i].startsWith('--')) { config.prompt = args[i]; + } else { + console.error(`Unknown option: ${args[i]}`); + process.exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/args-parser.ts` around lines 199 - 200, The parser currently treats any non-flag token as the prompt (config.prompt) which lets unknown/legacy flags like "--legacy" be skipped and their next token become the prompt; update the arg parsing logic (the code that iterates over args and sets config.prompt) to explicitly validate flags: if a token startsWith('--') but is not a recognized known flag, throw or return a CLI error instead of skipping it, and only assign config.prompt for tokens that do NOT start with '--' and when config.prompt is currently unset; ensure you reference the same identifiers (args[], config.prompt) and the parsing function that contains this loop so unknown flags are rejected and cannot corrupt prompt parsing.
♻️ Duplicate comments (1)
lua/vibing/application/chat/init.lua (1)
64-64:⚠️ Potential issue | 🟠 Major
/permissiondescription may still be out of sync with handler validation.If
lua/vibing/application/chat/handlers/permission.luastill validates only three values,/permission planand/permission dontAskwill be advertised but rejected at runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/vibing/application/chat/init.lua` at line 64, The command help string in init.lua advertises "/permission plan" and "/permission dontAsk" but the permission handler in lua/vibing/application/chat/handlers/permission.lua only validates three values; fix this by making the handler accept the additional modes advertised (add "plan" and "dontAsk" to its validation/allowed-values check in the handler function, e.g., the validate/allowed-values list or switch inside the permission handler), or alternatively update the description string in init.lua to only list the modes the handler currently accepts; modify whichever is appropriate so the advertised description and the handler's validation (in the permission handler function) match.
🧹 Nitpick comments (2)
lua/vibing/infrastructure/completion/providers/skills.lua (1)
86-88: Cache variable name is misleading.
_bundled_cachecurrently stores only dynamic SDK results, not the merged bundled list. Renaming it (e.g.,_dynamic_cache) would reduce confusion.Also applies to: 155-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/vibing/infrastructure/completion/providers/skills.lua` around lines 86 - 88, The variable _bundled_cache is misleading because it only holds dynamic SDK results; rename it to _dynamic_cache throughout the file (declaration, assignments and all references such as the checks at _bundled_cache and uses later around the second occurrence) and update any comments to reflect that it contains only dynamic SDK results, ensuring the true merged/ bundled list remains a separate variable name.lua/vibing/application/chat/commands.lua (1)
200-200: Use shared constants for permission completions to avoid drift.
permissioncompletions are hardcoded here and can diverge fromlua/vibing/core/constants/modes.lua.♻️ Proposed refactor
+local Modes = require("vibing.core.constants.modes") + function M.get_argument_completions(command_name) local completions = { - permission = { "default", "acceptEdits", "bypassPermissions", "plan", "dontAsk" }, + permission = vim.deepcopy(Modes.PERMISSION_MODES), model = { "opus", "sonnet", "haiku" }, } return completions[command_name] end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/vibing/application/chat/commands.lua` at line 200, The permission list in the command definition (permission = { "default", "acceptEdits", "bypassPermissions", "plan", "dontAsk" }) is hardcoded and can drift from the canonical set in lua/vibing/core/constants/modes.lua; replace the literal array with the shared constant exported from modes.lua (e.g., require the module and reference Modes.PERMISSIONS or the exact exported name used there) or merge any command-specific extras into that constant, updating the commands.lua definition to use that imported symbol instead of duplicating the values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/vibing/infrastructure/completion/providers/skills.lua`:
- Around line 101-116: The code uses config.node.executable (resolved into local
executable) directly when building cmd for vim.fn.systemlist, which can be
"auto" and thus invalid; resolve/validate the runtime like in
command_builder.lua before calling systemlist: detect "auto" or missing
executable, choose a concrete runtime (node, bun, or ts-node) based on
availability and dev_mode (for dev_mode ensure .ts uses ts-node or bun), fall
back to returning {} if no runtime found, and then build cmd from the resolved
executable, script_path, and any dev flags so that vim.fn.systemlist(cmd) is
always invoked with a real executable name (refer to executable, dev_mode,
script_path, list_commands_script and systemlist in this file).
---
Outside diff comments:
In `@bin/lib/args-parser.ts`:
- Around line 199-200: The parser currently treats any non-flag token as the
prompt (config.prompt) which lets unknown/legacy flags like "--legacy" be
skipped and their next token become the prompt; update the arg parsing logic
(the code that iterates over args and sets config.prompt) to explicitly validate
flags: if a token startsWith('--') but is not a recognized known flag, throw or
return a CLI error instead of skipping it, and only assign config.prompt for
tokens that do NOT start with '--' and when config.prompt is currently unset;
ensure you reference the same identifiers (args[], config.prompt) and the
parsing function that contains this loop so unknown flags are rejected and
cannot corrupt prompt parsing.
---
Duplicate comments:
In `@lua/vibing/application/chat/init.lua`:
- Line 64: The command help string in init.lua advertises "/permission plan" and
"/permission dontAsk" but the permission handler in
lua/vibing/application/chat/handlers/permission.lua only validates three values;
fix this by making the handler accept the additional modes advertised (add
"plan" and "dontAsk" to its validation/allowed-values check in the handler
function, e.g., the validate/allowed-values list or switch inside the permission
handler), or alternatively update the description string in init.lua to only
list the modes the handler currently accepts; modify whichever is appropriate so
the advertised description and the handler's validation (in the permission
handler function) match.
---
Nitpick comments:
In `@lua/vibing/application/chat/commands.lua`:
- Line 200: The permission list in the command definition (permission = {
"default", "acceptEdits", "bypassPermissions", "plan", "dontAsk" }) is hardcoded
and can drift from the canonical set in lua/vibing/core/constants/modes.lua;
replace the literal array with the shared constant exported from modes.lua
(e.g., require the module and reference Modes.PERMISSIONS or the exact exported
name used there) or merge any command-specific extras into that constant,
updating the commands.lua definition to use that imported symbol instead of
duplicating the values.
In `@lua/vibing/infrastructure/completion/providers/skills.lua`:
- Around line 86-88: The variable _bundled_cache is misleading because it only
holds dynamic SDK results; rename it to _dynamic_cache throughout the file
(declaration, assignments and all references such as the checks at
_bundled_cache and uses later around the second occurrence) and update any
comments to reflect that it contains only dynamic SDK results, ensuring the true
merged/ bundled list remains a separate variable name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82ecacc2-b185-4edd-aae6-87a4015a941d
📒 Files selected for processing (12)
.claude/rules/architecture.md.claude/rules/commands-reference.mdREADME.ja.mdREADME.mdbin/lib/args-parser.tsbin/list-commands.tslua/vibing/application/chat/commands.lualua/vibing/application/chat/init.lualua/vibing/application/daily_summary/collector.lualua/vibing/core/constants/modes.lualua/vibing/infrastructure/completion/providers/skills.luascripts/build.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- README.ja.md
- .claude/rules/commands-reference.md
- README.md
Comment on lines
+101
to
+116
| local executable = config.node and config.node.executable or "node" | ||
| local dev_mode = config.node and config.node.dev_mode or false | ||
| local script_path = list_commands_script | ||
|
|
||
| if not dev_mode then | ||
| -- Production mode: use compiled JS | ||
| script_path = plugin_dir .. "/dist/bin/list-commands.js" | ||
| if vim.fn.filereadable(script_path) ~= 1 then | ||
| -- Fallback to empty list if compiled version doesn't exist | ||
| return {} | ||
| end | ||
| end | ||
|
|
||
| -- Execute the script to get SDK-provided skills | ||
| local cmd = { executable, script_path } | ||
| local result = vim.fn.systemlist(cmd) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Inspect Node config contract/defaults"
rg -n --type=lua -C2 'NodeConfig|node\.executable|dev_mode|"auto"|executable\s*=' lua/vibing/config.lua
echo
echo "2) Inspect executable resolution call sites"
rg -n --type=lua -C3 'config\.node\.executable|systemlist\(|list-commands\.ts|list-commands\.js' lua
echo
echo "Expected: either (a) executable is normalized from \"auto\" before use, and dev_mode TS execution runtime is explicit, or (b) this path can fail and return empty skills."Repository: shabaraba/vibing.nvim
Length of output: 13532
Resolve config.node.executable before invoking systemlist.
config.node.executable defaults to "auto", but this code passes it directly to systemlist() without resolution. When executable = "auto", the command becomes systemlist({"auto", script_path}), which will fail. In dev mode, the .ts script execution also lacks explicit runtime handling (bun or ts-node requirement). While failures gracefully return an empty skill list, the code should properly resolve or validate the executable before attempting execution, similar to the pattern used in command_builder.lua.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lua/vibing/infrastructure/completion/providers/skills.lua` around lines 101 -
116, The code uses config.node.executable (resolved into local executable)
directly when building cmd for vim.fn.systemlist, which can be "auto" and thus
invalid; resolve/validate the runtime like in command_builder.lua before calling
systemlist: detect "auto" or missing executable, choose a concrete runtime
(node, bun, or ts-node) based on availability and dev_mode (for dev_mode ensure
.ts uses ts-node or bun), fall back to returning {} if no runtime found, and
then build cmd from the resolved executable, script_path, and any dev flags so
that vim.fn.systemlist(cmd) is always invoked with a real executable name (refer
to executable, dev_mode, script_path, list_commands_script and systemlist in
this file).
Previously, permissionMode was parsed from CLI args but not passed to Agent SDK's query() function. This caused 'plan' mode to not work as expected - tools could still execute instead of read-only planning. Changes: - Add permissionMode to query options in buildQueryOptions() - Ensures Agent SDK respects all 5 permissionMode values: default, acceptEdits, bypassPermissions, plan, dontAsk
- Use shared PERMISSION_MODES constant in handlers and completions - Fix README.md markdown linting issues (line length, code block language) - Add validation to reject unknown CLI flags in args-parser
Contributor
コードレビュー結果このPRは、無効な 🔴 Critical Issues (修正必須)1. TypeScript型定義の不整合ファイル: 問題: 現在の定義: 期待される定義: 影響: TypeScriptの型安全性が損なわれる 修正方法: 2. 未使用の
|
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.
概要
Agent SDKが
--modeパラメータをサポートしていないため、完全に無効だったmodeフィールドを削除し、permission_modeでAgent SDKが実際にサポートする5つのpermissionMode値すべてをサポートするよう更新しました。破壊的変更
削除されたもの:
mode:フィールド/modeスラッシュコマンド移行方法:
/modeの代わりに/permissionコマンドを使用config.agent.default_modeの代わりにconfig.permissions.modeで設定mode:が含まれていても無害(単に無視されます)変更内容
削除
lua/vibing/application/chat/handlers/mode.lua- modeハンドラーを削除lua/vibing/application/chat/init.lua-/modeコマンド登録を削除lua/vibing/application/commands/handler.lua-/modeコマンドを削除lua/vibing/core/constants/modes.lua-VALID_MODES定数とis_valid_mode()を削除lua/vibing/infrastructure/storage/frontmatter.lua- 優先順位リストからmodeを削除lua/vibing/presentation/chat/modules/renderer.lua- フロントマター描画からmodeを削除lua/vibing/infrastructure/adapter/modules/command_builder.lua-resolve_mode()と--modeフラグを削除bin/lib/args-parser.ts---modeパース処理を削除(ただし--permission-modeの検証は更新)bin/agent-wrapper.ts-options.mode設定を削除更新
bin/lib/args-parser.ts-validPermissionModesを3つから5つの値に更新planとdontAskを拒否していましたlua/vibing/application/chat/commands.lua-/permissionコマンドの補完を5値に更新lua/vibing/application/chat/init.lua-/permissionコマンドの説明を5値に更新lua/vibing/core/constants/modes.lua-PERMISSION_MODESを5値に更新:default- 標準の権限動作(canUseToolコールバック使用)acceptEdits- ファイル操作を自動承認(Edit, Write, mkdirなど)bypassPermissions- 全ツールを自動承認(注意して使用)plan- 読み取り専用の計画モード(ツール実行なし)dontAsk- プロンプトの代わりに拒否.claude/rules/architecture.md.claude/rules/commands-reference.mdREADME.mdREADME.ja.mdなぜこの変更が必要か
1.
modeフィールドは完全に無効だったAgent SDKのTypeScript Options型定義には
modeフィールドが存在しません。permissionModeのみがサポートされています。vibing.nvimは
--modeフラグをAgent SDKに渡していましたが、これは完全に無視されていました。2. サポートされる
permissionModeは5つすべて(修正済み)当初のエラー:
根本原因:
このエラーメッセージから「Agent SDKは3つのみサポート」と判断しましたが、実際にはvibing.nvim自身の
bin/lib/args-parser.ts(10行目)にハードコードされた検証配列がplanとdontAskを拒否していました。修正内容:
Agent SDK自体は5つすべての値をサポートしており、vibing.nvimのバリデーションがボトルネックでした。
テスト結果
チェックリスト
関連Issue
なし(内部リファクタリング)
パッケージバージョン
@anthropic-ai/claude-agent-sdk@^0.1.76Summary by CodeRabbit
New Features
/permissioncommand to handle mode configuration.Documentation
/modecommand.Refactor
/modeslash command; mode functionality integrated into permissions system.