Conversation
- load_gfproject_config 只合并 tools section - lib tools 作为基础，本地 tools 覆盖（除 help 外） - help 工具不合并，始终使用内部实现 - goldhelp.scm 遵循相同的合并逻辑
- find-gfproject-path 'lib 优先使用 GF_LIB，否则从 executable 向上查找 - 遵循与 C++ 代码相同的搜索逻辑
- fix 工具不补全 organization/module（未完全迁移到 Scheme） - goldhelp.scm 手动遍历合并 tools，遵循相同合并规则 - help 工具不合并，始终使用内部实现
- 在 gfproject.json 工具加载时跳过 help - 添加显式的 help 命令处理，调用 display_help() - 确保无论是否有 local gfproject.json，help 都使用内部实现
- 当 gfproject.json 中工具配置缺少 organization/module 时，返回 -1 - 当工具目录不存在时，返回 -1 - 调用方会继续处理内置命令，实现 local 优先于内置
Greptile SummaryThis PR fixes the
Confidence Score: 4/5Mostly safe for the primary gf tool dispatch path, but the Scheme-side The C++ merge and help-bypass logic is correct and the primary
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["gf <command>"] --> B{command == 'help'?}
B -- Yes --> C["C++ display_help()\n(static, hardcoded)"]
B -- No --> D["load_gfproject_config()\nmerge lib + local gfproject.json"]
D --> E{command in\nmerged tools?}
E -- No --> F[Fall through to built-in handlers]
E -- Yes --> G["goldfish_run_tool()\ncalls load_gfproject_config() again"]
G --> H{org + module\npresent? tool_root found?}
H -- No --> I["return -1\nfall through to built-in"]
H -- Yes --> J["Import Scheme module\ncall main()"]
J --> K{return value}
K -- "-1" --> I
K -- "0 / positive" --> L[Return result]
I --> F
Reviews (1): Last reviewed commit: "[200_69] fix: 工具配置不完整时回退到内置实现" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| (define (load-gfproject) | ||
| "Load and parse gfproject.json, return tools object" | ||
| (let ((path (find-gfproject-path))) | ||
| (if path | ||
| (let ((content (path-read-text path))) | ||
| (string->json content)) | ||
| (string->json "{}")))) | ||
| "Load and merge gfproject.json from local and lib, local takes precedence" | ||
| (let ((local-path (find-gfproject-path 'local)) | ||
| (lib-path (find-gfproject-path 'lib))) | ||
| (let ((lib-config (if lib-path | ||
| (let ((content (path-read-text lib-path))) | ||
| (string->json content)) | ||
| (make-json))) | ||
| (local-config (if local-path | ||
| (let ((content (path-read-text local-path))) | ||
| (string->json content)) | ||
| (make-json)))) | ||
| (let ((lib-tools (json-ref lib-config "tools")) | ||
| (local-tools (json-ref local-config "tools"))) | ||
| (json-merge-tools lib-tools local-tools))))) |
There was a problem hiding this comment.
load-gfproject returns merged tools, not a full config object
json-merge-tools returns the tools object directly (e.g. {"help":{…},"fix":{…}}), so load-gfproject now returns that. But every caller still does (json-ref config "tools") on the return value — looking for a key named "tools" inside an object that has no such key. The result is json-null, causing display-help to show no dynamic tools and display-tool-help to always print "Unknown command: …".
The fix is to wrap the merged tools back into a config object:
(define (load-gfproject)
"Load and merge gfproject.json from local and lib, local takes precedence"
(let ((local-path (find-gfproject-path 'local))
(lib-path (find-gfproject-path 'lib)))
(let ((lib-config (if lib-path
(string->json (path-read-text lib-path))
(make-json)))
(local-config (if local-path
(string->json (path-read-text local-path))
(make-json))))
(let ((merged (make-json)))
(json-set! merged "tools"
(json-merge-tools (json-ref lib-config "tools")
(json-ref local-config "tools")))
merged))))| @@ -5325,6 +5364,15 @@ repl_for_community_edition (s7_scheme* sc, int argc, char** argv) { | |||
| // If tool_ret == -1, tool config exists but execution failed, fall through to check other commands | |||
| } | |||
There was a problem hiding this comment.
load_gfproject_config called twice per tool invocation
load_gfproject_config(gf_lib) is called here (line 5357) to check whether the command is present, and then called a second time at the very start of goldfish_run_tool (line 4030). Each call reads and parses both JSON files from disk. Passing the already-loaded config into goldfish_run_tool would avoid the duplicate I/O:
json gfproject_config = load_gfproject_config (gf_lib);
if (command != "help" && gfproject_config.contains ("tools") && gfproject_config["tools"].contains (command)) {
int tool_ret = goldfish_run_tool (sc, gf_lib, command, gfproject_config, errmsg, old_port, gc_loc);Then update goldfish_run_tool's signature to accept a const json& config parameter instead of loading it internally.
| (let ((exe-path (executable))) | ||
| (if exe-path | ||
| (let search ((dir (path-dirname (path exe-path)))) | ||
| (if dir | ||
| (let ((gfproject-path (path->string (path-join dir (path "gfproject.json"))))) | ||
| (if (file-exists? gfproject-path) | ||
| gfproject-path | ||
| (search (path-dirname dir)))) | ||
| #f)) | ||
| #f))))) |
There was a problem hiding this comment.
path-dirname search may not terminate
The fallback search walks up the directory tree by repeatedly calling (path-dirname dir). If path-dirname returns the same path for the filesystem root (e.g. "/" → "/") instead of returning #f, the search loop will spin forever. A termination guard comparing the current dir with its parent would make this safe:
(let search ((dir (path-dirname (path exe-path))))
(if dir
(let ((parent (path-dirname dir)))
(if (or (not parent)
(string=? (path->string dir) (path->string parent)))
#f
(let ((gfproject-path (path->string (path-join dir (path "gfproject.json")))))
(if (file-exists? gfproject-path)
gfproject-path
(search parent)))))
#f))| @@ -5325,6 +5364,15 @@ repl_for_community_edition (s7_scheme* sc, int argc, char** argv) { | |||
| // If tool_ret == -1, tool config exists but execution failed, fall through to check other commands | |||
There was a problem hiding this comment.
tool_ret == -1
The comment says "tool config exists but execution failed", but -1 is now returned for several non-failure conditions: missing organization/module fields, or missing tool_root. A more accurate comment would be:
| // If tool_ret == -1, tool config exists but execution failed, fall through to check other commands | |
| // If tool_ret == -1, tool config is incomplete (missing org/module or tool root not found), fall back to built-in |
2 participants
修复 gfproject.json 合并逻辑问题
修改内容
src/goldfish.hpp:
tools/help/liii/goldhelp.scm:
gfproject.json:
测试