refactor(providers): extract shared error helpers to FnoxError methods by jdx · Pull Request #380 · jdx/fnox

jdx · 2026-04-04T13:12:16Z

Summary

Move clone_provider_error and map_batch_error from per-provider free functions to methods on FnoxError
Eliminates boilerplate that would otherwise be duplicated by every CLI-based provider (currently in infisical, and duplicated in the incoming Doppler PR feat(provider): add Doppler secrets manager provider #376)
Adds unit tests for the new methods on FnoxError

Test plan

All 12 infisical provider tests pass
All 6 error module tests pass (including 4 new ones)
Full cargo test passes (131/132, pre-existing keychain skip)

🤖 Generated with Claude Code

Note

Low Risk
Low risk refactor that centralizes provider error cloning/mapping logic; behavior should remain equivalent but could subtly affect provider error formatting if the fallback parameters are misused.

Overview
Moves provider-specific error helper functions into FnoxError as clone_provider_error and map_batch_error, so providers can reuse the same structured error cloning/mapping behavior.

Updates the Infisical batch secret fetch path to call e.map_batch_error(...) and removes the now-duplicated local helper functions, adding/adjusting unit tests to cover the new FnoxError methods and updated Infisical behavior.

^{Reviewed by Cursor Bugbot for commit 9fe2343. Bugbot is set up for automated code reviews on this repo. Configure here.}

Move `clone_provider_error` and `map_batch_error` from per-provider free functions to methods on `FnoxError`, eliminating boilerplate that would otherwise be duplicated by every CLI-based provider. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

gemini-code-assist

Code Review

This pull request refactors error handling by moving provider-specific error cloning and batch error mapping logic from the Infisical provider to the central FnoxError implementation in src/error.rs. New methods clone_provider_error and map_batch_error were added to FnoxError along with corresponding unit tests, and the Infisical provider was updated to utilize these methods to reduce code duplication. Feedback indicates that clone_provider_error is missing the ProviderSecretNotFound variant, which should be added for consistency across all provider-related error variants.

gemini-code-assist · 2026-04-04T13:13:37Z

+                hint: hint.clone(),
+                url: url.clone(),
+            },
+            _ => return None,


The clone_provider_error method is missing the ProviderSecretNotFound variant. While map_batch_error handles this variant explicitly to override the secret name, clone_provider_error is a public utility method that should ideally support all structured provider error variants for completeness and consistency.

FnoxError::ProviderSecretNotFound { provider, secret, hint, url, } => FnoxError::ProviderSecretNotFound { provider: provider.clone(), secret: secret.clone(), hint: hint.clone(), url: url.clone(), }, _ => return None,

greptile-apps

Summary

This PR cleanly extracts two free functions (clone_provider_error and map_batch_error) from src/providers/infisical.rs into methods on FnoxError in src/error.rs, making them available for reuse across all CLI-based providers. The approach is well-structured and the refactor is a clear improvement.

File analysis

src/error.rs

Two new pub methods added to FnoxError: clone_provider_error and map_batch_error.
Four new unit tests cover the main paths (clone auth failed, returns None for generic variant, replaces secret name in batch mapping, falls back to ProviderCliFailed for non-provider errors).
Minor issue: the docstring on clone_provider_error says "Returns None for non-provider error variants", but ProviderSecretNotFound, ProviderConfigCycle, and ProviderConfigResolutionFailed are all semantically "provider" variants yet also fall through the _ => return None arm. This can mislead future callers. (See inline comment.)

src/providers/infisical.rs

91 lines removed (the old free functions and their tests), 19 lines added (updated call sites using the new methods).
All existing tests are retained and updated correctly.
The batch error path now calls e.map_batch_error(...) directly on the error value — cleaner and consistent with the intended API.

Error flow (batch path)

get_secrets_batch()
  └─ execute_infisical_command() → Err(e)
       └─ e.map_batch_error(secret_name, ...)
            ├─ ProviderSecretNotFound → rebinds secret field to secret_name
            ├─ other cloneable Provider* variant → clone_provider_error()
            └─ non-cloneable / fallback → ProviderCliFailed { provider, details: e.to_string(), ... }

Confidence score: 4 / 5

Safe to merge. The single issue is a documentation inaccuracy on a pub method — not a behavioural bug, but worth fixing before the method is adopted by other providers to avoid confusion.

This comment was generated by Claude Code.

greptile-apps · 2026-04-04T13:15:32Z

+    /// Clone a provider error variant (all fields are `String`, so always cloneable).
+    ///
+    /// Returns `None` for non-provider error variants.
+    pub fn clone_provider_error(&self) -> Option<FnoxError> {


The docstring says "Returns None for non-provider error variants", but this is not quite accurate. ProviderSecretNotFound, ProviderConfigCycle, and ProviderConfigResolutionFailed are all provider-related variants (their names start with Provider) yet they also return None here.

Since this method is pub and intended to be reused across providers (e.g., the upcoming Doppler PR), a future caller looking at just the doc comment may reasonably assume ProviderSecretNotFound returns Some, only to be surprised at runtime. Consider clarifying which specific variants are handled:

/// Clones the subset of provider error variants that carry only `String` fields: /// `ProviderAuthFailed`, `ProviderCliNotFound`, `ProviderInvalidResponse`, /// `ProviderApiError`, and `ProviderCliFailed`. /// /// Returns `None` for all other variants, including `ProviderSecretNotFound` /// (handled separately by `map_batch_error`) and non-provider variants.

This comment was generated by Claude Code.

greptile-apps · 2026-04-04T13:16:50Z

Greptile Summary

This PR moves clone_provider_error and map_batch_error from infisical-specific free functions to reusable methods on FnoxError, eliminating boilerplate for CLI-based providers and preparing for the upcoming Doppler provider. The refactor is behaviorally correct and well-tested, with one documentation inconsistency flagged.

Refactor scope: Two free functions in src/providers/infisical.rs are deleted and replaced with pub methods on FnoxError in src/error.rs
API change: map_batch_error now takes explicit fallback_provider, fallback_hint, and fallback_url parameters (previously hardcoded to Infisical constants), making it provider-agnostic
Tests: 4 new unit tests added to src/error.rs; 4 existing infisical tests updated to use the new method signature
Doc issue: clone_provider_error docstring says "Returns None for non-provider error variants" but also returns None for ProviderSecretNotFound, ProviderConfigCycle, and ProviderConfigResolutionFailed, which are provider variants — misleading for future callers of this pub method

Confidence Score: 4/5

Safe to merge; the refactor is behaviorally correct and all existing tests pass

Score of 4 reflects a clean, well-tested refactor with one minor documentation inconsistency on a public method that could mislead future callers

src/error.rs — docstring on clone_provider_error should clarify which variants are handled vs. excluded

Important Files Changed

Filename	Overview
src/error.rs	Adds clone_provider_error and map_batch_error methods to FnoxError with 4 new unit tests; docstring on clone_provider_error is misleading about which variants return None
src/providers/infisical.rs	Removes old free functions and updates call sites to use the new FnoxError methods; tests updated with explicit fallback parameters

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Batch fetch returns error e] --> B{e is ProviderSecretNotFound?}
    B -- Yes --> C[Replace secret field with secret_name\npreserve provider/hint/url]
    C --> D[Return ProviderSecretNotFound]
    B -- No --> E[e.clone_provider_error]
    E --> F{Matches one of 5\nfetch-time variants?}
    F -- Yes --> G[Return cloned variant as-is]
    F -- No --> H[Fallback: ProviderCliFailed\nwith fallback_provider/hint/url]

_{Reviews (1): Last reviewed commit: "refactor(providers): extract shared erro..." | Re-trigger Greptile}

greptile-apps · 2026-04-04T13:16:53Z

+    /// Clone a provider error variant (all fields are `String`, so always cloneable).
+    ///
+    /// Returns `None` for non-provider error variants.


Misleading docstring on a pub method

The doc comment says "Returns None for non-provider error variants", implying all Provider* enum variants return Some. However ProviderSecretNotFound, ProviderConfigCycle, and ProviderConfigResolutionFailed are all provider error variants (with String-only fields) but still fall through to _ => return None.

The exclusion is intentional — map_batch_error handles ProviderSecretNotFound separately to rebind the secret field — but the public contract is misleading. Consider enumerating the handled variants explicitly:

Suggested change

/// Clone a provider error variant (all fields are `String`, so always cloneable).

///

/// Returns `None` for non-provider error variants.

/// Clone one of the five fetch-time provider error variants

/// (`ProviderAuthFailed`, `ProviderCliNotFound`, `ProviderInvalidResponse`,

/// `ProviderApiError`, `ProviderCliFailed`). All fields are `String`, so

/// these are always cheaply cloneable.

///

/// Returns `None` for all other variants, including `ProviderSecretNotFound`

/// (which `map_batch_error` handles separately to rebind the secret name).

@natefaerber

### 🚀 Features - **(provider)** add Doppler secrets manager provider by [@natefaerber](https://github.com/natefaerber) in [#376](#376) ### 🐛 Bug Fixes - **(ci)** pin LocalStack to v4 (last free community version) by [@jdx](https://github.com/jdx) in [#379](#379) - **(sync)** skip post-processing when resolving secrets for sync by [@rpendleton](https://github.com/rpendleton) in [#371](#371) ### 🚜 Refactor - **(providers)** extract shared error helpers to FnoxError methods by [@jdx](https://github.com/jdx) in [#380](#380) ### 📦️ Dependency Updates - lock file maintenance by [@renovate[bot]](https://github.com/renovate[bot]) in [#369](#369) - update taiki-e/upload-rust-binary-action digest to 0e34102 by [@renovate[bot]](https://github.com/renovate[bot]) in [#372](#372) - update dependency vue to v3.5.32 by [@renovate[bot]](https://github.com/renovate[bot]) in [#373](#373) ### New Contributors - @natefaerber made their first contribution in [#376](#376) - @rpendleton made their first contribution in [#371](#371)

gemini-code-assist Bot reviewed Apr 4, 2026

View reviewed changes

greptile-apps Bot reviewed Apr 4, 2026

View reviewed changes

jdx enabled auto-merge (squash) April 4, 2026 13:17

jdx merged commit 6b98998 into main Apr 4, 2026
18 checks passed

jdx deleted the refactor/provider-error-helpers branch April 4, 2026 14:11

mise-en-dev mentioned this pull request Apr 4, 2026

chore: release v1.20.0 #370

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(providers): extract shared error helpers to FnoxError methods#380

refactor(providers): extract shared error helpers to FnoxError methods#380
jdx merged 1 commit intomainfrom
refactor/provider-error-helpers

jdx commented Apr 4, 2026 •

edited by cursor Bot

Loading

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Apr 4, 2026

Uh oh!

greptile-apps Bot left a comment

Uh oh!

greptile-apps Bot Apr 4, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot commented Apr 4, 2026

Uh oh!

greptile-apps Bot Apr 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

-    /// Clone a provider error variant (all fields are `String`, so always cloneable).
-    ///
-    /// Returns `None` for non-provider error variants.
+    /// Clone one of the five fetch-time provider error variants
+    /// (`ProviderAuthFailed`, `ProviderCliNotFound`, `ProviderInvalidResponse`,
+    /// `ProviderApiError`, `ProviderCliFailed`). All fields are `String`, so
+    /// these are always cheaply cloneable.
+    ///
+    /// Returns `None` for all other variants, including `ProviderSecretNotFound`
+    /// (which `map_batch_error` handles separately to rebind the secret name).

Uh oh!

Conversation

jdx commented Apr 4, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Summary

File analysis

Error flow (batch path)

Confidence score: 4 / 5

Uh oh!

greptile-apps Bot Apr 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot commented Apr 4, 2026

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Flowchart

Uh oh!

greptile-apps Bot Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jdx commented Apr 4, 2026 •

edited by cursor Bot

Loading

greptile-apps Bot Apr 4, 2026 •

edited

Loading