fix(config): preserve provider while stripping CRLF - #110
Conversation
📝 WalkthroughWalkthroughThis PR updates config loading to sanitize CRLF content through temporary files before sourcing. It also adds structured LM Studio curl failure reporting with HTTP status and response-body capture, plus integration and unit test coverage. ChangesConfig loading and LM Studio failure reporting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant load_config
participant source_config_file
participant sanitize_config_file
participant TempFile
load_config->>source_config_file: source global or project config
source_config_file->>sanitize_config_file: sanitize config contents
sanitize_config_file->>TempFile: write sanitized config
source_config_file->>TempFile: source and remove temporary file
source_config_file-->>load_config: return success or failure
sequenceDiagram
participant execute_lmstudio_api
participant call_lmstudio_curl
participant curl
participant report_lmstudio_request_failure
execute_lmstudio_api->>call_lmstudio_curl: send endpoint and JSON payload
call_lmstudio_curl->>curl: request with HTTP status marker
curl-->>call_lmstudio_curl: response body and HTTP status
call_lmstudio_curl->>report_lmstudio_request_failure: report non-zero curl result
report_lmstudio_request_failure-->>execute_lmstudio_api: structured failure output
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/gga`:
- Around line 276-291: The source_config_file helper currently assumes mktemp
always succeeds, so an empty sanitized_config can lead to confusing
redirect/source failures. Update source_config_file to check the mktemp result
before calling sanitize_config_file or source, and if it fails, cleanly return 1
from source_config_file without attempting to use the temp file.
In `@lib/providers.sh`:
- Around line 655-674: The curl call, __GGA_HTTP_STATUS parsing, and error
handling in the LM Studio request path are duplicated in both request branches.
Extract that logic into a shared helper such as call_lmstudio_curl used by the
existing request function and execute_lmstudio_api_fallback, with the helper
handling endpoint/json_payload, parsing the marker, and calling
report_lmstudio_request_failure on errors. Since bash 3.2 has no nameref, return
results via clearly named globals or another simple shared mechanism so both
paths stay aligned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0f57c0bb-71e1-494e-ba69-93a5d9fd8587
📒 Files selected for processing (4)
bin/ggalib/providers.shspec/integration/commands_spec.shspec/unit/providers_spec.sh
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/gga (1)
268-273: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
sed | trhidessedfailures here.source_config_filecan treat an unreadable config as valid because the pipeline reportstr’s exit status, leaving an empty temp file that still sources cleanly. Return the first command’s status or enablepipefailbefore sourcing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/gga` around lines 268 - 273, The sanitize_config_file pipeline currently masks failures from sed, so source_config_file may proceed with an empty temp file instead of rejecting an unreadable config. Update sanitize_config_file to preserve the first command’s failure (for example by enabling pipefail in the relevant flow or explicitly checking the sed status) before the result is sourced, and keep the fix localized around sanitize_config_file and source_config_file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@bin/gga`:
- Around line 268-273: The sanitize_config_file pipeline currently masks
failures from sed, so source_config_file may proceed with an empty temp file
instead of rejecting an unreadable config. Update sanitize_config_file to
preserve the first command’s failure (for example by enabling pipefail in the
relevant flow or explicitly checking the sed status) before the result is
sourced, and keep the fix localized around sanitize_config_file and
source_config_file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d6ed33a5-a425-4b24-b7e1-644bbcefc0d0
📒 Files selected for processing (3)
bin/ggalib/providers.shspec/integration/commands_spec.sh
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/gga (1)
314-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider explicit error handling for
source_config_fileinload_config.The return value of
source_config_fileis not checked. Thesource_config_filefunction carefully returns exit codes on failure, butload_configignores them. The integration test at line 240 (The output should not include "Not configured") implies the script aborts viaset -ewhen sourcing fails — but relying onset -efor error propagation is fragile. If someone later wraps the call (e.g.,source_config_file "$PROJECT_CONFIG" || true), failures would be silently swallowed and the user would see "Not configured" instead of the actual error.Explicit error handling would make the contract clearer:
♻️ Suggested refactor
if [[ -f "$GLOBAL_CONFIG" ]]; then - source_config_file "$GLOBAL_CONFIG" + if ! source_config_file "$GLOBAL_CONFIG"; then + echo "Error: Failed to load global config: $GLOBAL_CONFIG" >&2 + return 1 + fi fi # Load project config (overrides global) PROJECT_CONFIG=".gga" if [[ -f "$PROJECT_CONFIG" ]]; then - source_config_file "$PROJECT_CONFIG" + if ! source_config_file "$PROJECT_CONFIG"; then + echo "Error: Failed to load project config: $PROJECT_CONFIG" >&2 + return 1 + fi fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/gga` around lines 314 - 320, Update load_config to explicitly check the return status of source_config_file for both GLOBAL_CONFIG and PROJECT_CONFIG loads, rather than relying on set -e. On failure, immediately propagate the nonzero status so configuration errors cannot be swallowed; use the source_config_file function’s existing return code and preserve the actual error output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@bin/gga`:
- Around line 314-320: Update load_config to explicitly check the return status
of source_config_file for both GLOBAL_CONFIG and PROJECT_CONFIG loads, rather
than relying on set -e. On failure, immediately propagate the nonzero status so
configuration errors cannot be swallowed; use the source_config_file function’s
existing return code and preserve the actual error output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 09a9fef6-8261-4523-8817-7fa11567be02
📒 Files selected for processing (2)
bin/ggaspec/integration/commands_spec.sh
1 participant
Linked Issue
Closes #109
PR Type
type:bug— Bug fixtype:feature— New feature or enhancementtype:docs— Documentation changes onlytype:refactor— Code refactor (no behavior change)type:chore— Maintenance (deps, CI, tooling)type:breaking-change— Breaking change (add!to commit type)Summary
Fixes
.ggaconfig loading soPROVIDERremains configured when project config files use CRLF line endings.The previous sanitization path could fail to preserve the
PROVIDERassignment on macOS Bash, causinggga runto reportNo provider configuredfor valid providers such aslmstudio,lmstudio:qwen/qwen3.5-9b, andminimax:MiniMax-M3.This also improves LM Studio request failure diagnostics so long-running API failures report the endpoint, curl exit code, HTTP status, and response body instead of the generic
Failed to connectmessage.Changes
bin/ggatr -d '\015'.lib/providers.shspec/integration/commands_spec.shPROVIDERkey and exact value with CRLF config files.spec/unit/providers_spec.shTest Plan
make lint(ShellCheck) passes locallymake testpasses locally (all unit tests)shellspec spec/unit/providers_spec.shpassesshellspec spec/integration/commands_spec.shpasses.ggaconfig loading with CRLF and validated provider valueslmstudio,lmstudio:qwen/qwen3.5-9b, andminimax:MiniMax-M3gga run --no-cacheagainst LM Studio athttp://100.107.225.97:1234/v1and receivedSTATUS: PASSEDFull
make testwas run locally, but it currently fails on existing OpenCode-related tests in untouched pre-existing provider behavior withopencode_args[@]: unbound variable. The focused suites for this PR pass.Automated Checks
The following checks run automatically on every PR:
Closes/Fixes/Resolves #Nstatus:approvedtype:*Labeltype:*labelbin/ggaandlib/*.shshellspec spec/unitpassesshellspec spec/integration/commands_spec.shpassesContributor Checklist
Closes #Nstatus:approvedtype:*label to this PRmake lintpasses (ShellCheck)make testpasses (all unit tests)spec/feat:,fix:,docs:, etc.)!in the commit type (feat!:,fix!:)Notes for Reviewers
Issue #109 still needs the
status:approvedlabel from a maintainer before this PR can pass the repository issue-first validation.The LM Studio diagnostics change intentionally does not change provider execution semantics. It only exposes the curl/HTTP details needed to distinguish network failures from LM Studio API failures.
Summary by CodeRabbit