Skip to content

feat: CLI onboarding + editablePathPatterns - #211

Merged
lishuceo merged 15 commits into
mainfrom
feat/onboarding-cli
Apr 8, 2026
Merged

feat: CLI onboarding + editablePathPatterns#211
lishuceo merged 15 commits into
mainfrom
feat/onboarding-cli

Conversation

@lishuceo

@lishuceo lishuceo commented Apr 8, 2026

Copy link
Copy Markdown
Owner

Summary

  • editablePathPatterns: readonly agent 也能编辑指定路径的文件(如 config/personas/*),PM bot 可以直接修改自己的人设和知识库
  • setup.sh 精简: 从 428 行缩减到 ~160 行,仅保留系统检查 + npm install + API Key + 编译验证
  • CLI onboarding agent: npm run onboard 启动 Agent SDK 多轮对话,引导用户完成飞书配置、团队信息、Bot 人格和可选功能设定

Test plan

  • 新机器部署测试:删除 .env 和 config/agents.json,运行 setup.sh → onboard
  • editablePathPatterns:PM bot 能编辑 config/personas/pm.md 但不能编辑 src/ 下的文件
  • npx vitest run 全部通过(1323 passed,1 pre-existing flaky failure in memory quality)

🤖 Generated with Claude Code

lishuceo and others added 3 commits April 8, 2026 14:55
新增 editablePathPatterns 配置项,允许 readonly agent 编辑指定路径的文件。
PM agent 默认可编辑 config/personas/* 和 config/knowledge/*,
用户可以直接让 chatbot 修改自己的人格和知识库配置。

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
setup.sh 从 428 行精简到 ~160 行,仅保留:
- 系统依赖检查 (Node.js, python3/make/gcc)
- npm install + native 模块验证
- ANTHROPIC_API_KEY 配置
- 编译验证

飞书配置、团队信息、Bot 人格等交由 npm run onboard 完成。

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
新增 npm run onboard 命令,通过 Agent SDK 在终端启动多轮对话,
引导用户完成飞书应用配置、团队信息、Bot 人格和可选功能设定。

- scripts/onboard.ts: CLI 多轮对话入口
- src/onboarding/bootstrap.ts: prompt 生成 + 完成状态检测
- .env.example: 添加 ONBOARDING_COMPLETED 文档

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 8, 2026

Copy link
Copy Markdown

Claude finished @lishuceo's task in 4m 1s —— View job


PR Review: feat: CLI onboarding + editablePathPatterns

  • Check for previous review comments — none found
  • Read and review all 14 changed files
  • Analyze security model of editablePathPatterns
  • Post summary

Approved — Clean, well-structured PR with no issues found at high confidence.

Review Summary

editablePathPatterns (executor.ts, config-schema.ts, types.ts, config-loader.ts, event-handler.ts)

  • Security model is sound: matchesEditablePattern uses path.resolve() which normalizes .. traversal attempts, and the /* vs /** wildcard ordering is correct ('/**'.endsWith('/*') is false, so both branches are reachable)
  • The check is correctly placed in canUseTool — after toolAllow (which provides broader override) but before the general WRITE_TOOLS deny
  • Only Edit and Write are gated by editablePathPatterns, not Bash (which has its own bashAllowPatterns control) — this separation is clean
  • Config inheritance from defaults works correctly via input.editablePathPatterns ?? defaults.editablePathPatterns
  • Tests (editable-path-patterns.test.ts, config-loader.test.ts) cover single-level wildcards, recursive wildcards, exact match, multiple patterns, absolute patterns, and edge cases

CLI onboarding (bootstrap.ts, onboard.ts)

  • bootstrap.ts: Clean separation of concerns — prompt generation, completion detection, and state management. markOnboardingCompleted correctly handles append, replace (commented-out), and replace (existing value) cases
  • onboard.ts: Session resumption via sessionId works correctly. Error handling appropriately distinguishes fatal (API key) vs non-fatal errors. Readline cleanup is reached in all code paths
  • Tests (bootstrap.test.ts) use process-specific temp dirs and properly restore process.cwd in afterEach

setup.sh simplification

  • Reduced from ~428 to ~160 lines. Retains essential steps (system deps, npm install, API key, build verification) and delegates complex interactive config to the onboarding agent — good separation

Overall: The feature design is well-thought-out — editablePathPatterns provides a narrowly-scoped write capability for readonly agents without compromising the security model. Test coverage is thorough across all three features.

修复 `npm run onboard` 报 "Not logged in" 的问题:
- 添加 `import 'dotenv/config'` 加载 .env 中的 ANTHROPIC_API_KEY
- query options 传递 `env: process.env` 确保子进程继承 API Key 和 Base URL

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 8, 2026

Copy link
Copy Markdown

Claude finished @lishuceo's task in 2m 4s —— View job


PR Review: feat/onboarding-cli (re-review after sync)

  • Check for previous inline review comments — none found
  • Read full diff and all 14 changed files
  • Trace cross-module interactions (canUseTool chain, config inheritance, onboard session flow)
  • Post findings

Approved — No new issues found. Clean, well-structured PR.

Review Summary

editablePathPatterns (executor.ts:151-175, config-schema.ts, types.ts, config-loader.ts, event-handler.ts)

  • Security model is correct: matchesEditablePattern uses path.resolve() which normalizes .. traversal attempts before comparison
  • Wildcard ordering is safe: '/**'.endsWith('/*') is false (last 2 chars of /** are **, not /*), so both branches are reachable
  • Correctly placed in canUseTool — after toolAllow override but before the general WRITE_TOOLS deny at line 851
  • Only gates Edit and Write (not Bash/NotebookEdit), which is the right scoping for config file editing
  • Config inheritance via input.editablePathPatterns ?? defaults.editablePathPatterns works correctly
  • Pass-through in event-handler.ts is consistent across all 3 execution paths (lines 2063, 2158, 2547)
  • Test coverage is thorough: single-level *, recursive **, exact match, multiple patterns, absolute patterns, edge cases

CLI onboarding (onboard.ts, bootstrap.ts)

  • onboard.ts: canUseTool allows all tools — appropriate for a local CLI where the user is the operator. Readline is closed in all exit paths (early exit at line 37-38, loop break falls through to line 148). Session resumption via sessionId correctly carries context across rounds
  • bootstrap.ts: markOnboardingCompleted correctly handles all three cases (append, replace commented, replace existing value). isOnboardingCompleted reads .env directly (not process.env) which is correct since the file may have been modified by the agent mid-session
  • Tests (bootstrap.test.ts) use process-specific temp dirs (/tmp/onboard-test-${process.pid}) and properly restore process.cwd in afterEach

setup.sh simplification

  • Reduced from ~428 to ~160 lines. Retains essential steps (system deps, npm install, API key, build verification) and delegates interactive config to the onboarding agent. The exec npm run onboard at the end (line 658) replaces the shell process cleanly.

Overall: The editablePathPatterns feature provides a narrowly-scoped write capability for readonly agents without compromising the security model. The onboarding CLI cleanly separates prompt generation from the interactive loop. No issues at confidence ≥ 75.


Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 8, 2026

Copy link
Copy Markdown

Claude finished @lishuceo's task in 1m 38s —— View job


PR Review: feat/onboarding-cli + editablePathPatterns (re-review after sync)

  • Check for previous inline review comments — none found
  • Read full diff and all 14 changed files
  • Trace cross-module interactions (canUseTool chain, config inheritance, onboard session flow)
  • Post findings

Approved — No issues found at confidence ≥ 75. Clean, well-structured PR.

Review Summary

editablePathPatterns (executor.ts:151-175, config-schema.ts, types.ts, config-loader.ts, event-handler.ts)

  • Security model is correct: path.resolve() normalizes .. traversal attempts before prefix comparison, so config/personas/../../etc/passwd resolves to an absolute path outside the allowed directory and is rejected
  • Priority chain is sound: toolDeny (line 801) → source repo protection (line 806) → toolAllow (line 815) → editablePathPatterns (line 843) → general WRITE_TOOLS deny (line 851). editablePathPatterns cannot override toolDeny or source repo protection
  • Only gates Edit and WriteBash has its own bashAllowPatterns control, NotebookEdit/Skill remain blocked in readOnly mode. This scoping is appropriate for config file editing
  • Wildcard ordering is safe: '/**'.endsWith('/*') is false, so both branches are reachable
  • Config inheritance via input.editablePathPatterns ?? defaults.editablePathPatterns works correctly
  • Pass-through in event-handler.ts is consistent across all 3 execution paths (lines 2063, 2158, 2547)
  • Test coverage (editable-path-patterns.test.ts, config-loader.test.ts) is thorough: single-level *, recursive **, exact match, multiple patterns, absolute patterns, edge cases

CLI onboarding (onboard.ts, bootstrap.ts)

  • onboard.ts: canUseTool allows all tools — appropriate for a local CLI where the user is the operator. Readline is closed in all exit paths. Session resumption via sessionId correctly carries context across rounds
  • bootstrap.ts: markOnboardingCompleted correctly handles all three cases (append, replace commented, replace existing value). isOnboardingCompleted reads .env directly (not process.env) which is correct since the file may have been modified by the agent mid-session
  • Tests (bootstrap.test.ts) use process-specific temp dirs and properly restore process.cwd in afterEach

setup.sh simplification

  • Reduced from ~428 to ~160 lines. Retains essential steps (system deps, npm install, API key, build verification) and delegates interactive config to the onboarding agent. exec npm run onboard at the end replaces the shell process cleanly

Overall: The editablePathPatterns feature provides a narrowly-scoped write capability for readonly agents without compromising the security model. The onboarding CLI cleanly separates prompt generation from the interactive loop. No issues at confidence ≥ 75.

- setup.sh: 检查 ANTHROPIC_API_KEY 是否以 sk- 开头,防止误填飞书 ID
- onboard.ts: 清理传给子进程的 env(去掉 CLAUDECODE 嵌套检测变量)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 8, 2026

Copy link
Copy Markdown

PR Review: feat/onboarding-cli + editablePathPatterns

  • Check for previous inline review comments
  • Read full PR diff and all changed files
  • Trace cross-module interactions and security model
  • Post review findings

View job run

- setup.sh: 只校验非空,不限制 key 格式(兼容第三方代理)
- onboard.ts: 认证 401 错误时明确提示检查 ANTHROPIC_API_KEY 和 BASE_URL
- onboard.ts: 首次连接时显示等待提示,避免用户以为卡死

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 8, 2026

Copy link
Copy Markdown

Claude finished @lishuceo's task in 1m 55s —— View job


PR Review: feat/onboarding-cli + editablePathPatterns

  • Check for previous inline review comments — none found
  • Read full PR diff and all 14 changed files
  • Trace cross-module interactions and security model
  • Post review findings

Approved — No issues found at confidence ≥ 75. Clean, well-structured PR.

Review Summary

editablePathPatterns (executor.ts:151-175, config-schema.ts, types.ts, config-loader.ts, event-handler.ts)

  • Security model is correct: path.resolve() normalizes .. traversal attempts before prefix comparison, so config/personas/../../etc/passwd resolves to an absolute path outside the allowed directory and is rejected
  • Priority chain is sound: toolDeny (line 801) → source repo protection (line 806) → toolAllow (line 814) → editablePathPatterns (line 842) → general WRITE_TOOLS deny (line 851). editablePathPatterns cannot override toolDeny or source repo protection
  • Only gates Edit and WriteBash has its own bashAllowPatterns control, NotebookEdit/Skill remain blocked in readOnly mode. Appropriate scoping for config file editing
  • Wildcard ordering is safe: '/**'.endsWith('/*') is false (last 2 chars are **), so both branches are reachable
  • Config inheritance via input.editablePathPatterns ?? defaults.editablePathPatterns correctly allows agent-level override (even [] to explicitly disable) while falling back to defaults
  • Pass-through in event-handler.ts is consistent across all 3 execution paths (lines 2063, 2158, 2547)
  • Test coverage (editable-path-patterns.test.ts, config-loader.test.ts) is thorough: single-level *, recursive **, exact match, multiple patterns, absolute patterns, edge cases

CLI onboarding (onboard.ts, bootstrap.ts)

  • onboard.ts: canUseTool allows all tools — appropriate for a local CLI where the user is the operator. Readline is closed in all exit paths (early exit at lines 84-85, loop break at 175/187, and end of file at line 218). Session resumption via sessionId correctly carries context across rounds
  • bootstrap.ts: markOnboardingCompleted correctly handles all three cases (append, replace commented, replace existing value). isOnboardingCompleted reads .env directly (not process.env) — correct since the file may have been modified by the agent mid-session
  • Tests (bootstrap.test.ts) use process-specific temp dirs (/tmp/onboard-test-${process.pid}) and properly restore process.cwd in afterEach

setup.sh simplification

  • Reduced from ~428 to ~160 lines. Retains essential steps (system deps, npm install, API key, build verification) and delegates interactive config to the onboarding agent. exec npm run onboard at the end replaces the shell process cleanly

Overall: The editablePathPatterns feature provides a narrowly-scoped write capability for readonly agents without compromising the security model. The onboarding CLI cleanly separates prompt generation from the interactive loop. No issues at confidence ≥ 75.

- Phase 1 拆分为先问 App ID 再问 App Secret,不同时要两个值
- 重要规则强调「每次只问一个问题」
- 终端不支持 markdown 链接,改为直接写 URL

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 8, 2026

Copy link
Copy Markdown

Claude finished @lishuceo's task in 2m 49s —— View job


PR Review: feat/onboarding-cli + editablePathPatterns

  • Check for previous inline review comments — none found
  • Read full PR diff and all 14 changed files
  • Trace cross-module interactions and security model
  • Post review findings

Approved — No issues found at confidence ≥ 75. Clean, well-structured PR.

Review Summary

editablePathPatterns (executor.ts:151-175, config-schema.ts, types.ts, config-loader.ts, event-handler.ts)

  • Security model is correct: path.resolve() normalizes .. traversal attempts before prefix comparison — config/personas/../../etc/passwd resolves to an absolute path outside the allowed directory and is rejected
  • Priority chain is sound: toolDeny (line 801) → source repo protection (line 806) → toolAllow (line 814) → editablePathPatterns (line 843) → general WRITE_TOOLS deny (line 851). editablePathPatterns cannot override toolDeny or source repo protection
  • Only gates Edit and WriteBash has its own bashAllowPatterns control, NotebookEdit/Skill remain blocked in readOnly mode. Appropriate scoping for config file editing
  • Wildcard ordering is safe: '/**'.endsWith('/*') is false (last 2 chars are **), so both branches are reachable
  • Config inheritance via input.editablePathPatterns ?? defaults.editablePathPatterns correctly allows agent-level override while falling back to defaults
  • Pass-through in event-handler.ts is consistent across all 3 execution paths (lines 2063, 2158, 2547)
  • Test coverage (editable-path-patterns.test.ts, config-loader.test.ts) is thorough: single-level *, recursive **, exact match, multiple patterns, absolute patterns, edge cases

CLI onboarding (onboard.ts, bootstrap.ts)

  • onboard.ts: canUseTool allows all tools — appropriate for a local CLI where the user is the operator. Readline is closed in all exit paths (early exit at lines 37-38, loop break at 129/141, max rounds at 172). Session resumption via sessionId correctly carries context across rounds
  • bootstrap.ts: markOnboardingCompleted correctly handles all three cases (append, replace commented, replace existing value). isOnboardingCompleted reads .env directly (not process.env) — correct since the file may have been modified by the agent mid-session
  • Tests (bootstrap.test.ts) use process-specific temp dirs (/tmp/onboard-test-${process.pid}) and properly restore process.cwd in afterEach

setup.sh simplification

  • Reduced from ~428 to ~160 lines. Retains essential steps (system deps, npm install, API key, build verification) and delegates interactive config to the onboarding agent. exec npm run onboard at the end (line 215) replaces the shell process cleanly

Overall: The editablePathPatterns feature provides a narrowly-scoped write capability for readonly agents without compromising the security model. The onboarding CLI cleanly separates prompt generation from the interactive loop. No issues at confidence ≥ 75.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 8, 2026

Copy link
Copy Markdown

PR Review: feat/onboarding-cli + editablePathPatterns

  • Check for previous inline review comments
  • Read full PR diff and all changed files
  • Trace cross-module interactions and security model
  • Post review findings

View job run

- 权限(权限管理)和事件订阅(事件与回调)是飞书后台不同页面,分两步引导
- card.action.trigger 标注为必须,否则卡片按钮不生效
- 提醒用户配置后需要发布版本

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 8, 2026

Copy link
Copy Markdown

Claude finished @lishuceo's task in 2m 26s —— View job


PR Review: feat/onboarding-cli + editablePathPatterns

  • Check for previous inline review comments — none found
  • Read full PR diff and all 14 changed files
  • Trace cross-module interactions and security model
  • Post review findings

Approved — No issues found at confidence ≥ 75. Clean, well-structured PR.

Review Summary

editablePathPatterns (executor.ts:151-175, config-schema.ts, types.ts, config-loader.ts, event-handler.ts)

  • Security model is correct: path.resolve() normalizes .. traversal attempts before prefix comparison — config/personas/../../etc/passwd resolves to an absolute path outside the allowed directory and is rejected
  • Priority chain is sound: toolDeny (line 801) → source repo protection (line 806) → toolAllow (line 814) → editablePathPatterns (line 843) → general WRITE_TOOLS deny (line 851). editablePathPatterns cannot override toolDeny or source repo protection
  • Only gates Edit and Write (not Bash/NotebookEdit/Skill) — appropriate scoping for config file editing. Bash has its own bashAllowPatterns control
  • Wildcard ordering is safe: '/**'.endsWith('/*') is false (last 2 chars are **), so both branches are reachable
  • Config inheritance via input.editablePathPatterns ?? defaults.editablePathPatterns correctly allows agent-level override (even [] to explicitly disable) while falling back to defaults
  • Pass-through in event-handler.ts is consistent across all 3 execution paths (lines 2063, 2158, 2547)
  • Test coverage (editable-path-patterns.test.ts) is thorough: single-level *, recursive **, exact match, multiple patterns, absolute patterns, and edge cases

CLI onboarding (onboard.ts, bootstrap.ts)

  • onboard.ts: canUseTool allows all tools — appropriate for a local CLI where the user is the operator. Readline is closed in all exit paths (early exit at line 56, loop break at lines 146/158/175/180, max rounds at 186, and final rl.close() at line 190). Spinner cleanup is safe (checked if (spinnerTimer) before clearing). Session resumption via sessionId correctly carries context across rounds
  • bootstrap.ts: markOnboardingCompleted correctly handles all three cases (append, replace commented, replace existing value). isOnboardingCompleted reads .env directly (not process.env) — correct since the file may have been modified by the agent mid-session. clearOnboardingCompleted correctly comments out the flag
  • Tests (bootstrap.test.ts) use process-specific temp dirs (/tmp/onboard-test-${process.pid}) and properly restore process.cwd in afterEach. All state transitions are covered

setup.sh simplification

  • Reduced from ~428 to ~160 lines. Retains essential steps: system dep checks, npm install with native module verification, API key configuration, and build verification
  • sed_escape function correctly handles sed replacement special chars (&, |, \) with | as delimiter
  • Delegates interactive config to the onboarding agent via exec npm run onboard (line 215) which replaces the shell process cleanly

Minor pre-existing note (not introduced by this PR): bashAllowPatterns is passed in the main execution path (line 2062) but missing from the workspace-restart path (line ~2158). editablePathPatterns is correctly added to all 3 paths.

用户在 onboarding 阶段还未启动 bot,不知道自己的飞书 open_id,
不应该问 ALLOWED_USER_IDS。系统有 autoDetectOwner 机制自动将
第一个发消息的用户设为管理员。

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 8, 2026

Copy link
Copy Markdown

PR Review: feat/onboarding-cli + editablePathPatterns

  • Check for previous inline review comments
  • Read full PR diff and all changed files
  • Trace cross-module interactions and security model
  • Post review findings

View job run

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 8, 2026

Copy link
Copy Markdown

Claude finished @lishuceo's task in 3m 36s —— View job


PR Review: feat/onboarding-cli + editablePathPatterns

  • Check for previous inline review comments — none found
  • Read full PR diff and all 14 changed files
  • Trace cross-module interactions and security model
  • Post review findings

Approved — One minor defensive issue found (confidence 78%). Overall clean, well-structured PR.

Findings

🟡 Missing stopSpinner() after for await loopscripts/onboard.ts:140 (confidence: 78%)

The spinner is started before query() (line 74) and stopped inside the assistant message handler (line 119). If the async iterable completes without emitting any assistant messages, the spinner timer continues running when rl.question() is reached (line 156), causing garbled terminal output. Adding stopSpinner() after the for await loop (line 140) is a trivial defensive fix.

In practice the Agent SDK almost always emits at least one assistant message per query, so this is unlikely to trigger — but the fix is zero-cost and eliminates the possibility.

Fix this →

Review Summary

editablePathPatterns (executor.ts:151-175, config-schema.ts, types.ts, config-loader.ts, event-handler.ts)

  • Security model is correct: path.resolve() normalizes .. traversal attempts before prefix comparison — config/personas/../../etc/passwd resolves to an absolute path outside the allowed directory and is rejected
  • Priority chain in canUseTool is sound: toolDeny (line 801) → source repo protection (line 806) → toolAllow (line 814) → editablePathPatterns (line 842) → general WRITE_TOOLS deny (line 851). editablePathPatterns cannot override toolDeny or source repo protection
  • Only gates Edit and WriteBash has its own bashAllowPatterns control, NotebookEdit/Skill remain blocked in readOnly mode. Appropriate scoping for config file editing
  • Wildcard ordering is safe: '/**'.endsWith('/*') is false (last 2 chars are **), so both branches are reachable
  • Config inheritance via input.editablePathPatterns ?? defaults.editablePathPatterns correctly allows agent-level override while falling back to defaults
  • Pass-through in event-handler.ts is consistent across all 3 execution paths (lines 2063, 2158, 2547)
  • Test coverage (editable-path-patterns.test.ts, config-loader.test.ts) is thorough

CLI onboarding (onboard.ts, bootstrap.ts)

  • onboard.ts: canUseTool allows all tools — appropriate for a local CLI where the user is the operator. Readline is closed in all exit paths. Session resumption via sessionId correctly carries context across rounds
  • bootstrap.ts: markOnboardingCompleted correctly handles all three cases (append, replace commented, replace existing value). isOnboardingCompleted reads .env directly (not process.env) — correct since the file may have been modified by the agent mid-session
  • Tests (bootstrap.test.ts) use process-specific temp dirs and properly restore process.cwd in afterEach

setup.sh simplification

  • Reduced from ~428 to ~160 lines. Retains essential steps (system deps, npm install, API key, build verification) and delegates interactive config to the onboarding agent. exec npm run onboard at the end replaces the shell process cleanly

修复单 bot 模式下 bot 不知道自己名字的问题:
- client.ts: fetchBotInfo 存储 app_name
- event-handler.ts: buildBotIdentityContext 单 bot 模式也构建身份上下文
  (从飞书 API 的 botName + agent config 的 displayName 取名字)

新增 /config 命令(Owner only):
- 显示 agent 配置(模型、工具策略、回复模式等)
- 显示关联文件路径(persona、knowledge)
- 预览人设和知识内容

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 8, 2026

Copy link
Copy Markdown

Claude finished @lishuceo's task in 2m 12s —— View job


PR Review: feat/onboarding-cli + editablePathPatterns

  • Check for previous inline review comments — none found
  • Read full PR diff and all 14 changed files
  • Trace cross-module interactions and security model
  • Post review findings

Approved — One minor defensive issue found (confidence 78%). Overall clean, well-structured PR.

Findings

🟡 Missing stopSpinner() after for await loopscripts/onboard.ts:140 (confidence: 78%)

The spinner is started before query() (line 74) and stopped inside the assistant message handler (line 119). If the async iterable completes without emitting any assistant messages (e.g. query completes with only system + result messages), the spinner timer continues running when rl.question() is reached (line 156), causing garbled terminal output. Adding stopSpinner() after the for await loop (after line 140) is a trivial defensive fix.

In practice the Agent SDK almost always emits at least one assistant message per query, so this is unlikely to trigger — but the fix is zero-cost.

Fix this →

Review Summary

editablePathPatterns (executor.ts:151-175, config-schema.ts, types.ts, config-loader.ts, event-handler.ts)

  • Security model is correct: path.resolve() normalizes .. traversal attempts before prefix comparison — config/personas/../../etc/passwd resolves to an absolute path outside the allowed directory and is rejected
  • Priority chain in canUseTool is sound: toolDeny (line 801) → source repo protection (line 806) → toolAllow (line 814) → editablePathPatterns (line 843) → general WRITE_TOOLS deny (line 851). editablePathPatterns cannot override toolDeny or source repo protection
  • Only gates Edit and WriteBash has its own bashAllowPatterns control, NotebookEdit/Skill remain blocked in readOnly mode. Appropriate scoping
  • Wildcard ordering is safe: '/**'.endsWith('/*') is false (last 2 chars are **), so both branches are reachable
  • Config inheritance via input.editablePathPatterns ?? defaults.editablePathPatterns correctly allows agent-level override while falling back to defaults
  • Pass-through in event-handler.ts is consistent across all 3 execution paths (lines 2141, 2236, 2625)
  • Test coverage (editable-path-patterns.test.ts, config-loader.test.ts) is thorough: single-level *, recursive **, exact match, multiple patterns, absolute patterns, edge cases

CLI onboarding (onboard.ts, bootstrap.ts)

  • onboard.ts: canUseTool allows all tools — appropriate for a local CLI where the user is the operator. Readline is closed in all exit paths. Session resumption via sessionId correctly carries context across rounds
  • bootstrap.ts: markOnboardingCompleted correctly handles all three cases (append, replace commented, replace existing value). isOnboardingCompleted reads .env directly (not process.env) — correct since the file may have been modified by the agent mid-session
  • Tests (bootstrap.test.ts) use process-specific temp dirs (/tmp/onboard-test-${process.pid}) and properly restore process.cwd in afterEach

setup.sh simplification

  • Reduced from ~428 to ~160 lines. Retains essential steps (system deps, npm install, API key, build verification) and delegates interactive config to the onboarding agent. exec npm run onboard at the end (line 215) replaces the shell process cleanly
  • sed_escape function correctly escapes sed replacement special chars (&, |, \) with | as delimiter

Pre-existing note (not introduced by this PR): bashAllowPatterns is passed in the main execution path (line 2140) but missing from both the restart path (~line 2236) and the direct task path (~line 2625). editablePathPatterns correctly covers all 3 paths.

- config/personas/pm.example.md → assistant.example.md(通用名,不绑定角色)
- agents.example.json displayName 改为 "<onboarding 中设定>" 明确是占位符
- agents.example.json persona 路径改为 assistant.md
- bootstrap prompt 动态配置所有 agent 的 displayName 和 persona,
  不再硬编码写入 pm.md

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 8, 2026

Copy link
Copy Markdown

Claude finished @lishuceo's task in 2m 12s —— View job


PR Review: feat/onboarding-cli + editablePathPatterns

  • Check for previous inline review comments — none found
  • Read full PR diff and all 16 changed files
  • Trace cross-module interactions and security model
  • Post review findings

Approved — One minor defensive issue found (confidence 80%). Overall clean, well-structured PR.

Findings

🟡 Missing stopSpinner() after for await loopscripts/onboard.ts:140 (confidence: 80%)

The spinner is started before query() and only stopped inside the case 'assistant' handler. If the async iterable completes without emitting any assistant messages, the spinner timer continues running when rl.question() is reached, causing garbled terminal output. Adding stopSpinner() after the for await loop is a zero-cost defensive fix. See inline comment →

Review Summary

editablePathPatterns (executor.ts:151-175, config-schema.ts, types.ts, config-loader.ts, event-handler.ts)

  • Security model is correct: path.resolve() normalizes .. traversal attempts before prefix comparison — config/personas/../../etc/passwd resolves to an absolute path outside the allowed directory and is rejected
  • Priority chain in canUseTool is sound: toolDeny (line 801) → source repo protection (line 806) → toolAllow (line 814) → editablePathPatterns (line 842) → general WRITE_TOOLS deny (line 851). editablePathPatterns cannot override toolDeny or source repo protection
  • Only gates Edit and WriteBash has its own bashAllowPatterns control, NotebookEdit/Skill remain blocked in readOnly mode. Appropriate scoping for config file editing
  • Wildcard ordering is safe: '/**'.endsWith('/*') is false (last 2 chars are **), so both branches are reachable
  • Config inheritance via input.editablePathPatterns ?? defaults.editablePathPatterns correctly allows agent-level override while falling back to defaults
  • Pass-through in event-handler.ts is consistent across all 3 execution paths (lines 2141, 2236, 2625)
  • Test coverage (editable-path-patterns.test.ts, config-loader.test.ts) is thorough: single-level *, recursive **, exact match, multiple patterns, absolute patterns, edge cases

CLI onboarding (onboard.ts, bootstrap.ts)

  • onboard.ts: canUseTool allows all tools — appropriate for a local CLI where the user is the operator. Readline is closed in all exit paths. Session resumption via sessionId correctly carries context across rounds
  • bootstrap.ts: markOnboardingCompleted correctly handles all three cases (append, replace commented, replace existing value). isOnboardingCompleted reads .env directly (not process.env) — correct since the file may have been modified by the agent mid-session
  • Tests (bootstrap.test.ts) use process-specific temp dirs (/tmp/onboard-test-${process.pid}) and properly restore process.cwd in afterEach

setup.sh simplification

  • Reduced from ~428 to ~160 lines. Retains essential steps (system deps, npm install, API key, build verification) and delegates interactive config to the onboarding agent. exec npm run onboard at the end (line 215) replaces the shell process cleanly

Pre-existing note (not introduced by this PR): bashAllowPatterns is only passed in the main execution path (line 2140) but missing from the restart path (~line 2236) and the direct task path (~line 2625). editablePathPatterns correctly covers all 3 paths.

Comment thread scripts/onboard.ts
- onboard.ts: for-await 循环后调用 stopSpinner(),防止无 assistant 消息时
  spinner 残留导致终端输出错乱 (review feedback)
- image-compress.test.ts: GIF 压缩测试在 CI 上偶发超时,15s 更安全

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 8, 2026

Copy link
Copy Markdown

Claude finished @lishuceo's task in 2m 48s —— View job


PR Review: feat/onboarding-cli + editablePathPatterns

  • Check and resolve previous inline review comments — 1 previous issue fixed (stopSpinner)
  • Read full PR diff and all 17 changed files
  • Trace cross-module interactions and security model
  • Post review findings

Approved — No new issues found at confidence ≥ 75. Previous stopSpinner issue has been fixed.

Review Summary

editablePathPatterns (executor.ts:151-175, config-schema.ts, types.ts, config-loader.ts, event-handler.ts)

  • Security model is correct: path.resolve() normalizes .. traversal attempts before prefix comparison — config/personas/../../etc/passwd resolves to an absolute path outside the allowed directory and is rejected
  • Priority chain in canUseTool is sound: toolDeny (line 801) → source repo protection (line 806) → toolAllow (line 814) → editablePathPatterns (line 842) → general WRITE_TOOLS deny (line 851). editablePathPatterns cannot override toolDeny or source repo protection
  • Only gates Edit and WriteBash has its own bashAllowPatterns control, NotebookEdit/Skill remain blocked in readOnly mode
  • Wildcard ordering is safe: '/**'.endsWith('/*') is false (last 2 chars are **), so both branches are reachable
  • Config inheritance via input.editablePathPatterns ?? defaults.editablePathPatterns correctly allows agent-level override while falling back to defaults
  • Pass-through in event-handler.ts is consistent across all 3 execution paths (lines 2141, 2236, 2625)
  • Test coverage (editable-path-patterns.test.ts, config-loader.test.ts) is thorough: single-level *, recursive **, exact match, multiple patterns, absolute patterns, edge cases

CLI onboarding (onboard.ts, bootstrap.ts)

  • onboard.ts: canUseTool allows all tools — appropriate for a local CLI where the user is the operator. Readline is closed in all exit paths. Session resumption via sessionId correctly carries context across rounds. The previously flagged stopSpinner() issue is now fixed at line 142
  • bootstrap.ts: markOnboardingCompleted correctly handles all three cases (append, replace commented, replace existing value). isOnboardingCompleted reads .env directly (not process.env) — correct since the file may have been modified by the agent mid-session
  • Tests (bootstrap.test.ts) use process-specific temp dirs (/tmp/onboard-test-${process.pid}) and properly restore process.cwd in afterEach

New /config command (event-handler.ts:1098-1159)

  • Properly owner-gated via isOwner(userId). Displays agent config, persona preview, and knowledge preview with sensible 200-char truncation

buildBotIdentityContext enhancement (event-handler.ts:1770-1789)

  • Single-bot mode now injects bot identity from feishuClient.botName + agent displayName. Clean handling of name deduplication when both are available

setup.sh simplification

  • Reduced from ~428 to ~160 lines. Retains essential steps (system deps, npm install, API key, build verification) and delegates interactive config to the onboarding agent. exec npm run onboard at line 215 replaces the shell process cleanly
  • sed_escape function correctly escapes sed replacement special chars (&, |, \) with | as delimiter

Pre-existing note (not introduced by this PR): bashAllowPatterns is only passed in the main execution path (line 2140) but missing from both the restart path (~line 2233) and the direct task path (~line 2625). editablePathPatterns correctly covers all 3 paths.

@lishuceo
lishuceo merged commit 7b49795 into main Apr 8, 2026
8 checks passed
@lishuceo
lishuceo deleted the feat/onboarding-cli branch April 8, 2026 10:05
lishuceo added a commit that referenced this pull request Apr 8, 2026
* feat: editablePathPatterns — readonly agent 也能编辑配置文件

新增 editablePathPatterns 配置项,允许 readonly agent 编辑指定路径的文件。
PM agent 默认可编辑 config/personas/* 和 config/knowledge/*,
用户可以直接让 chatbot 修改自己的人格和知识库配置。

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor: 精简 setup.sh,配置交给 CLI onboarding agent

setup.sh 从 428 行精简到 ~160 行,仅保留:
- 系统依赖检查 (Node.js, python3/make/gcc)
- npm install + native 模块验证
- ANTHROPIC_API_KEY 配置
- 编译验证

飞书配置、团队信息、Bot 人格等交由 npm run onboard 完成。

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat: CLI onboarding agent — 对话式首次配置

新增 npm run onboard 命令,通过 Agent SDK 在终端启动多轮对话,
引导用户完成飞书应用配置、团队信息、Bot 人格和可选功能设定。

- scripts/onboard.ts: CLI 多轮对话入口
- src/onboarding/bootstrap.ts: prompt 生成 + 完成状态检测
- .env.example: 添加 ONBOARDING_COMPLETED 文档

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: onboard 脚本加载 dotenv 并传递 env 给 SDK 子进程

修复 `npm run onboard` 报 "Not logged in" 的问题:
- 添加 `import 'dotenv/config'` 加载 .env 中的 ANTHROPIC_API_KEY
- query options 传递 `env: process.env` 确保子进程继承 API Key 和 Base URL

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: onboard 添加 stderr 和调试输出,排查子进程无响应

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: setup.sh 校验 API Key 格式 + onboard 清理 env 传递

- setup.sh: 检查 ANTHROPIC_API_KEY 是否以 sk- 开头,防止误填飞书 ID
- onboard.ts: 清理传给子进程的 env(去掉 CLAUDECODE 嵌套检测变量)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: 去掉 API Key 格式校验 + 认证失败时给出清晰修复指引

- setup.sh: 只校验非空,不限制 key 格式(兼容第三方代理)
- onboard.ts: 认证 401 错误时明确提示检查 ANTHROPIC_API_KEY 和 BASE_URL
- onboard.ts: 首次连接时显示等待提示,避免用户以为卡死

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: onboarding prompt 严格一次只问一个问题

- Phase 1 拆分为先问 App ID 再问 App Secret,不同时要两个值
- 重要规则强调「每次只问一个问题」
- 终端不支持 markdown 链接,改为直接写 URL

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat: onboard 等待 agent 回复时显示 spinner 动画

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: onboarding prompt 权限和事件订阅分开讲,强调 card.action.trigger 必须

- 权限(权限管理)和事件订阅(事件与回调)是飞书后台不同页面,分两步引导
- card.action.trigger 标注为必须,否则卡片按钮不生效
- 提醒用户配置后需要发布版本

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: onboarding 移除用户访问控制配置,autoDetectOwner 自动处理

用户在 onboarding 阶段还未启动 bot,不知道自己的飞书 open_id,
不应该问 ALLOWED_USER_IDS。系统有 autoDetectOwner 机制自动将
第一个发消息的用户设为管理员。

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: 配置完成后同时提示 npm run dev 和 pm2 启动方式

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat: 单 bot 模式注入身份上下文 + /config 诊断命令

修复单 bot 模式下 bot 不知道自己名字的问题:
- client.ts: fetchBotInfo 存储 app_name
- event-handler.ts: buildBotIdentityContext 单 bot 模式也构建身份上下文
  (从飞书 API 的 botName + agent config 的 displayName 取名字)

新增 /config 命令(Owner only):
- 显示 agent 配置(模型、工具策略、回复模式等)
- 显示关联文件路径(persona、knowledge)
- 预览人设和知识内容

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor: persona 模板去 PM 耦合 + agents.example 占位符语义化

- config/personas/pm.example.md → assistant.example.md(通用名,不绑定角色)
- agents.example.json displayName 改为 "<onboarding 中设定>" 明确是占位符
- agents.example.json persona 路径改为 assistant.md
- bootstrap prompt 动态配置所有 agent 的 displayName 和 persona,
  不再硬编码写入 pm.md

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: spinner 循环后兜底 stopSpinner + GIF 测试超时加到 15s

- onboard.ts: for-await 循环后调用 stopSpinner(),防止无 assistant 消息时
  spinner 残留导致终端输出错乱 (review feedback)
- image-compress.test.ts: GIF 压缩测试在 CI 上偶发超时,15s 更安全

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant