Skip to content

fix: pass error when failed to parse toml#386

Merged
jdx merged 1 commit into
jdx:mainfrom
gaojunran:fix-parse-config
Apr 26, 2026
Merged

fix: pass error when failed to parse toml#386
jdx merged 1 commit into
jdx:mainfrom
gaojunran:fix-parse-config

Conversation

@gaojunran

Copy link
Copy Markdown
Contributor

@greptile-apps

greptile-apps Bot commented Apr 26, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a silent-failure bug in all_merged_from: when a pitchfork.toml could not be parsed, the error was only printed to stderr and processing continued. The fix returns the error (wrapped with the file path) so callers abort with a clear message. The doc comment is also corrected to reflect the new hard-fail contract, and the test that relied on the old soft-fail behavior is removed.

Confidence Score: 5/5

Safe to merge — the change is minimal, correct, and the doc comment is already updated.

Only P2 findings (missing test for the new hard-fail path); no logic or security issues introduced.

tests/test_e2e_namespace.rs — no test verifies the new hard-fail behavior introduced by the fix.

Important Files Changed

Filename Overview
src/pitchfork_toml.rs Error branch in all_merged_from changed from eprintln! (soft-fail/continue) to return Err(e.wrap_err(...)) (hard-fail/propagate); doc comment updated to match.
tests/test_e2e_namespace.rs Two tests removed: one correctly retired (it asserted the now-removed soft-fail behavior), one (test_local_namespace_override_mismatch_reports_error) removed without a clear replacement; no new test covers the hard-fail path.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[all_merged_from] --> B[list_paths_from]
    B --> C{for each path}
    C --> D[Self::read]
    D --> E{Result?}
    E -- Ok --> F[namespace collision check]
    F --> G[pt.merge]
    G --> C
    E -- "Err (old)" --> H["eprintln! — continue loop"]
    H --> C
    E -- "Err (new)" --> I["return Err(e.wrap_err(...))"]
    I --> J[Caller receives error]
Loading

Reviews (3): Last reviewed commit: "fix: pass error when failed to parse tom..." | Re-trigger Greptile

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request modifies the error handling in src/pitchfork_toml.rs to propagate errors encountered during file reading instead of just printing them. A review comment correctly identifies that the current implementation will fail to compile because wrap_err is being called on the error object itself rather than a Result type. The feedback also suggests using wrap_err_with for better performance and notes that the function's documentation needs to be updated to reflect the new error-returning behavior.

Comment thread src/pitchfork_toml.rs
@gaojunran gaojunran marked this pull request as ready for review April 26, 2026 10:05
@jdx jdx merged commit 2c05243 into jdx:main Apr 26, 2026
5 checks passed
@jdx jdx mentioned this pull request Apr 26, 2026
jdx added a commit that referenced this pull request Apr 26, 2026
## 🤖 New release

* `pitchfork-cli`: 2.6.0 -> 2.7.0

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

## [2.7.0](v2.6.0...v2.7.0) -
2026-04-26

### Added

- *(supervisor)* run daemons as a configured user
([#384](#384))
- *(watch)* impl poll mode
([#353](#353))
- *(cli)* stop / restart --all-global / --all-local
([#385](#385))
- version check in IPC
([#354](#354))

### Fixed

- pass error when failed to parse toml
([#386](#386))

### Other

- *(deps)* update rust crate xx to v2.5.4
([#378](#378))
- *(deps)* lock file maintenance
([#371](#371))
- *(deps)* update rust crate xx to v2.5.4
([#363](#363))
- *(deps)* update rust crate hyper-rustls to v0.27.9
([#359](#359))
- *(deps)* update rust crate rmcp to v1.5.0
([#364](#364))
- *(deps)* update rust crate libc to v0.2.185
([#360](#360))
- *(deps)* update rust crate tokio to v1.52.1
([#365](#365))
- *(deps)* update rust crate uuid to v1.23.1
([#362](#362))
- *(deps)* update rust crate rustls to v0.23.38
([#361](#361))
- *(deps)* update rust crate clap to v4.6.1
([#358](#358))
- *(deps)* update rust crate axum to v0.8.9
([#357](#357))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Low Risk**
> Low risk release bookkeeping: only bumps the `pitchfork-cli` version
and regenerates changelog/docs metadata, with no runtime code changes.
> 
> **Overview**
> Updates project metadata for the `v2.7.0` release.
> 
> Bumps `pitchfork-cli` from `2.6.0` to `2.7.0` across `Cargo.toml`,
`Cargo.lock`, and the generated CLI docs (`docs/cli/*` and
`pitchfork.usage.kdl`), and adds the `2.7.0` entry to `CHANGELOG.md`.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
e93c44b. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.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.

2 participants