Skip to content

[200_69] 修复 gfproject.json 合并逻辑,支持单命令回退#697

Merged
GatsbyUSTC merged 2 commits intomainfrom
cms/200_69/fix_gfproject_json_merge
Apr 13, 2026
Merged

[200_69] 修复 gfproject.json 合并逻辑,支持单命令回退#697
GatsbyUSTC merged 2 commits intomainfrom
cms/200_69/fix_gfproject_json_merge

Conversation

@MoonL79
Copy link
Copy Markdown
Collaborator

@MoonL79 MoonL79 commented Apr 13, 2026

No description provided.

@MoonL79 MoonL79 self-assigned this Apr 13, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 13, 2026

Greptile Summary

This PR refactors the gfproject.json loading logic in src/goldfish.hpp to support merging a project-local config over the lib config, enabling per-field deep overrides and a single-command fallback (when a local override causes a tool to fail, the lib's original implementation is tried automatically). The Scheme-side load-gfproject is simplified to call the new g_gfproject-load-config C++ glue, and a new test exercises both the deep-merge and the fallback path.

Confidence Score: 5/5

Safe to merge; all findings are P2 style and minor performance suggestions.

The refactoring is well-structured: struct-based error handling is cleaner than the old in-place cleanup, the merge logic is correct, and the new test covers both the deep-merge and fallback scenarios. The three findings are all P2 — a redundant config load (performance/TOCTOU), a missing blank line, and a leading comment in the test file that violates the project style guide. None of these affect correctness.

src/goldfish.hpp — redundant double-load of gfproject config (lines 5514–5516) and missing blank line between function definitions (line 4233).

Important Files Changed

Filename Overview
src/goldfish.hpp Major refactor of gfproject.json loading: split config load/merge into composable helpers, introduced struct-based error handling, and added single-command fallback; redundant double-load of config in repl_for_community_edition and a missing blank line between functions.
tools/help/liii/goldhelp.scm Simplified load-gfproject to delegate to the new C++ glue function g_gfproject-load-config; removes the Scheme-side file search that only covered the local directory.
tools/help/tests/liii/goldhelp/load-gfproject-test.scm New test covering deep-merge (field-level overlay) and single-command fallback; starts with leading comments violating CLAUDE.md convention that test files begin directly with (import ...).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[repl_for_community_edition] --> B[load_gfproject_config\ngf_lib]
    B --> C{command in\nmerged config?}
    C -- yes --> D[goldfish_run_tool]
    D --> E[resolve_gfproject_tool\nloads config AGAIN]
    E --> F{has local\noverride AND\nlib tool?}
    F -- yes --> G[goldfish_run_tool_with_config\nmerged_tool, allow_fallback=true]
    G -- success --> Z[return 0]
    G -- fail: -1 --> H[goldfish_run_tool_with_config\nlib_tool, allow_builtin_fallback]
    H -- success --> Z
    H -- fail: -1 --> I[fall through to\nhardcoded handlers]
    F -- no --> J[goldfish_run_tool_with_config\nmerged_tool, allow_builtin_fallback]
    J -- success --> Z
    J -- fail: -1 --> I
    C -- no --> I
    I --> K{command?}
    K -- help --> L[display_help]
    K -- version --> M[display_version]
    K -- eval/load/repl/run --> N[hardcoded handler]
Loading

Comments Outside Diff (1)

  1. src/goldfish.hpp, line 5514-5516 (link)

    P2 Redundant double-loading of gfproject config

    load_gfproject_config(gf_lib) is called here purely to check whether the command key exists, then goldfish_run_tool calls resolve_gfproject_tool → load_gfproject_config_bundle immediately after — loading and parsing the same JSON files a second time. Beyond the wasted I/O, a file write between the two reads is a theoretical TOCTOU: the outer check sees the command but the inner load does not (or vice versa). Consider passing the already-loaded config into goldfish_run_tool instead of re-loading it.

Reviews (1): Last reviewed commit: "[200_69] 修复 gfproject.json 合并逻辑,支持单命令回退" | Re-trigger Greptile

Comment on lines +1 to +3
;; 添加 tools/help 到 load path,以便导入 (liii goldhelp)
;; 注意:假设运行测试时工作目录是项目根目录
(set! *load-path* (cons "tools/help" *load-path*))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Test file should start with (import ...), not comments

Per the project convention in CLAUDE.md, test files must start directly with (import ...) (no license text, no leading comments). This file begins with two ;; comment lines and a (set! *load-path* ...) expression before the import block. The load-path setup is functionally necessary, but the leading comments should be removed to comply with the style rule.

Context Used: CLAUDE.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +4233 to +4235
}
static string
find_tool_root_by_command (const char* gf_lib, const string& command) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing blank line between function definitions

goldfish_run_tool closes on line 4233 and find_tool_root_by_command begins on the very next line with no blank line separator. All other adjacent function definitions in this file have at least one blank line between them.

Suggested change
}
static string
find_tool_root_by_command (const char* gf_lib, const string& command) {
}
static string
find_tool_root_by_command (const char* gf_lib, const string& command) {

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@GatsbyUSTC GatsbyUSTC merged commit 5fb2941 into main Apr 13, 2026
4 checks passed
@GatsbyUSTC GatsbyUSTC deleted the cms/200_69/fix_gfproject_json_merge branch April 13, 2026 09:07
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.

2 participants