fix(tokenless): address bot review comments on init command - #2318
fix(tokenless): address bot review comments on init command#2318Forrest-ly wants to merge 3 commits into
Conversation
github-actions
Bot
added
component:tokenless
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
There was a problem hiding this comment.
[P1] run_detect 对 JSON / 文本输出过于乐观地归为 Ready，可能误报适配器已安装。
[P2] 适配器目录查找硬编码类 Unix share 路径，对自定义安装位置和其他平台缺乏兜底或显式说明。
[P2] --framework 模式在缺少 install 脚本时打印警告但仍返回成功，调用方无法区分安装失败。
🤖 Generated by Qoder • View workflow run
| .env("ANOLISA_ADAPTER_DIR", adapter_dir) | ||
| .output() | ||
| { | ||
| Ok(out) => match out.status.code() { |
There was a problem hiding this comment.
[P1] run_detect 对非 Codex JSON 输出假定为 Ready
init.rs:125-138 中只在 {"installed": false} 时改为 Installable，否则自动视为 Ready。若探测脚本输出其他 JSON（如 {"installed": true, "missing": [..]} 或额外状态）或非 JSON 文本，都会被归为 Ready，可能误报安装完成。建议：仅在明确 installed == true 时归为 Ready，其他 JSON / 文本统一回落到 Installable 或 MissingPrereqs，并在探测约定中说明。
🤖 Generated by Qoder • Fix in Qoder
There was a problem hiding this comment.
Fixed: run_detect now only reports Ready when detector stdout explicitly contains {"installed": true}; otherwise it falls back to Installable.
| install_script: Option<String>, | ||
| } | ||
|
|
||
| /// Find the adapter directory by checking known locations. |
There was a problem hiding this comment.
[P2] 适配器目录查找不区分 Windows / 非标准路径
init.rs:63-78 只检查 $HOME/.local/share、/usr/share 和 /usr/local/share，对 Windows 用户或通过 anolisa install --prefix 安装到其他 share 目录的情况，将直接报 "Adapter directory not found"。虽然 tokenless 主要定位 Linux/macOS，但文档中未明确平台约束。建议：在文档或 CLI 帮助中标明仅支持类 Unix 路径，或允许通过环境变量覆写 adapter 目录以兼容自定义安装位置。
🤖 Generated by Qoder • Fix in Qoder
There was a problem hiding this comment.
Fixed: find_adapter_dir() now honors ANOLISA_ADAPTER_DIR as an override before checking the default Unix share paths, and the not-found error mentions this option for custom install prefixes.
| } | ||
| println!("\nAdapter directory: {}", adapter_dir.display()); | ||
|
|
||
| // --framework: check/install one specific framework |
There was a problem hiding this comment.
[P2] --framework 模式下无安装脚本仅打印警告但仍返回成功
init.rs:232-252 中，当指定框架状态为 Installable 且缺少 install_script 时，只调用 eprintln!("tokenless: no install script for {}") 然后 return Ok(())。这会让 tokenless init --framework ... 在无法安装时仍退出码 0，调用方难以感知失败。建议：在该分支返回 Err((.., 1)) 或至少区分成 "未配置 install 脚本" 的非成功状态。
🤖 Generated by Qoder • Fix in Qoder
There was a problem hiding this comment.
Fixed: when --framework selects an installable adapter with no configured install script, the command now returns Err((..., 1)) instead of printing a warning and exiting successfully.
![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: 72c2860b72
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if let Some(name) = &framework { | ||
| let fw = frameworks.iter().find(|f| &f.name == name); | ||
| match fw { | ||
| Some(f) if f.status == DetectStatus::Installable => { |
There was a problem hiding this comment.
When --list is combined with --framework, this branch runs before the list_only guard below, so an installable adapter is installed even though the flag is documented as listing status without installing. Reject this flag combination in clap or return early for list_only before handling a selected framework.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: run() now rejects --framework combined with --list at the start with a non-zero error, so --list never triggers installation.
| return DetectStatus::Installable; | ||
| } | ||
| } | ||
| DetectStatus::Ready |
There was a problem hiding this comment.
For qoder and codex, the manifest points at detectors that do not verify the Tokenless adapter is installed: qoder only checks that qodercli supports plugin lifecycle commands, and codex emits installed: true when the tokenless binary is on PATH. Because every zero exit falls through to Ready, a fresh qoder/codex setup is reported as already ready and tokenless init --framework ... never invokes the install script.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: aligned with the run_detect change — only explicit {"installed": true} is treated as Ready. Availability-only detectors now report Installable, so tokenless init --framework ... will run the install script.
Adds `tokenless init` — a community entry point that detects installed agent frameworks via each adapter's detect.sh, reports status (ready / installable / missing prereqs), and runs install.sh for the selected framework. Supports --framework, --all, --list flags and interactive selection when stdin is a terminal. Community users installing via npm no longer need to manually locate and run adapter install scripts; `tokenless init` guides them through framework detection and adapter registration in one step. Assisted-by: Qoder:1.0.45 Signed-off-by: 林生 <linyan.lin@alibaba-inc.com> Co-authored-by: multica-agent <github@multica.ai>
- Remove install hints from --list mode (P1: list should be read-only)
- Parse JSON stdout in detect to catch detectors that exit 0 but report
{"installed": false}, fixing false-ready status for Codex and Qoder (P1)
- Capture stderr in run_install and include it in error messages (P2)
- Propagate --all install failures as Err instead of silently continuing (P2)
- Remove CHANGELOG entry from feature PR (belongs in release PR only) (P2)
- Document adapter-directory-missing error and recovery in EN/ZH docs (P2)
Co-authored-by: multica-agent <github@multica.ai>
Co-authored-by: multica-agent <github@multica.ai>
dba36d1 to
c905751
Compare
|
已解决与 main 的合并冲突（关联 AGE-2135）：
因 rebase 重写了提交，使用 |
1 participant
Summary
Addresses all 6 bot review comments (2×P1 + 4×P2) on PR #2264.
Changes
P1 fixes:
--listmode no longer callsprint_install_hints()— the flag semantics are "show status only", so install guidance was misleadingrun_detectnow parses stdout JSON when exit code is 0: detectors like Codex that always exit 0 but emit{"installed": false}when the tool is absent are now correctly mapped toInstallableinstead ofReady, preventing false "already installed" reports for Qoder and Codex adaptersP2 fixes:
run_installnow usesCommand::output()and includes captured stderr in the error message for easier community troubleshooting--allinstall loop now collects failures and returnsErrat the end if any install failed, consistent with--frameworkbehavior[Unreleased]CHANGELOG entry — per the documentation standard, CHANGELOG summaries belong in release/version-bump PRs onlyframework-integration.mdTest plan
cargo testinsrc/tokenless/crates/tokenless-clipasses (25/25 green)tokenless init --listno longer shows install hintstokenless init --framework codexcorrectly detects Codex as installable when tokenless binary is absenttokenless init --allreturns non-zero exit when any adapter install fails