refactor: remove dead guardrails wrapper, cut redundancies, fix registry trim

[SantiagoDePolonia]

Summary

A focused code-quality pass: removes dead and duplicated code and fixes one routing bug, with no change to public API behavior and test coverage preserved. Net ~−469 lines.

Bug fix

• fix(registry): Provider types were stored verbatim at registration while every reader except ProviderByType trimmed on lookup. A configured type with surrounding whitespace (e.g. from YAML/env) therefore made type-based routing silently return nil. The type is now normalized at registration alongside the name, so all lookups stay consistent. Added regression test TestProviderByTypeAndNameTrimConfiguredValues.

Dead code removal

• Deleted the GuardedProvider wrapper (guardrails/provider.go, 518 lines) and the unused RequestPatcher/BatchPreparer in executor.go (52 lines). The live request path applies guardrails through WorkflowRequestPatcher / WorkflowBatchPreparer (wired in app.go); the wrapper was only reachable from its own tests.
• Live clone helpers moved to guardrails/clone.go.
• Coverage preserved: the shared rewrite functions (processGuardedChat / processGuardedResponses / processGuardedBatchRequest) are still exercised end-to-end via a trimmed test-only harness (provider_harness_test.go). The 6 pure-delegation tests were dropped (they only checked that the wrapper forwarded to its inner provider), and the two server tests were retargeted onto the live Workflow* patchers.

Redundancy cleanups

• providers/router.go: collapsed 3 near-identical Native{File,Batch,Response}ProviderTypes loops into one providerTypesSupporting helper.
• core/errors.go: added NewEmptyProviderResponseError, replacing 8 duplicated NewProviderError(... "provider returned empty response" ...) constructions across native_response_service.go and inference_execute.go.
• Replaced 4 hand-rolled map[string]any clone helpers with stdlib maps.Clone (identical nil → nil semantics).
• anthropic: dropped an unreachable len(tokens) == 0 branch (strings.Split never returns empty).

Verification

• make test-race, make lint, go mod tidy, gofmt, and the hot-path performance guard all pass (enforced by pre-commit).

Follow-up (not in this PR)

• workflows/service.go duplicates a 6-way scope classification across 4 sites; unifying it cleanly means restructuring the snapshot and touches resolution-order logic, so it warrants its own reviewed PR.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Improved and standardized error handling when upstream providers return empty responses, including during response retrieval and related response actions.
  • Fixed provider registration/lookup to tolerate extra whitespace in configured provider names and types.
  • Guardrail rewriting now deep-copies request/chat data to prevent unintended mutation of the original inputs.

• Tests

  • Updated guardrail test setup to use workflow-aware request patching and batch preparation with a static pipeline resolver.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: d5db969c-ee00-4ff4-babb-a94d4c69a2d3

📥 Commits

Reviewing files that changed from the base of the PR and between b306fae and 4a6b804.

📒 Files selected for processing (5)

• internal/core/errors.go
• internal/gateway/inference_execute.go
• internal/guardrails/clone.go
• internal/guardrails/provider_harness_test.go
• internal/guardrails/provider_test.go

---

📝 Walkthrough

Walkthrough

Adds a dedicated empty-provider-response error used by gateway and native response paths. Moves guardrails request handling into a test harness with cloning helpers and workflow-aware test updates. Simplifies map cloning, trims registry input, removes one Anthropic validation check, and consolidates router capability filtering.

Changes

Empty Provider Response Error

Layer / File(s)	Summary
Error constructor and usage internal/core/errors.go, internal/gateway/inference_execute.go, internal/server/native_response_service.go	Adds NewEmptyProviderResponseError(provider) and switches nil-response branches in gateway execution and native response endpoints to use it.

Guardrails Workflow Refactor

Layer / File(s)	Summary
Message and content cloning helpers internal/guardrails/clone.go	Adds deep-copy helpers for tool calls, message envelopes, content parts, and nested content fields used during guardrails rewrites.
Removal of exported guardrail wrappers internal/guardrails/executor.go	Removes the exported request/batch wrapper types from executor code, leaving only the internal guarded processing helpers.
GuardedProvider test harness internal/guardrails/provider_harness_test.go	Adds a test-only GuardedProvider with batch toggle behavior, request rewrites for chat and responses, native routing checks, and batch cleanup handling.
Test and handler updates internal/guardrails/provider_test.go, internal/server/handlers_test.go	Updates guardrails tests to use the new harness and workflow-aware patchers/preparers, and removes the trailing delegation test block.

Map Cloning and Provider Registry Cleanup

Layer / File(s)	Summary
maps.Clone adoption across helpers internal/guardrails/responses_message_apply.go, internal/providers/responses_adapter.go, internal/responsecache/stream_cache.go, internal/usage/extractor.go	Replaces manual map copy logic with maps.Clone in four cloning helpers.
Registry trimming and router capability filter internal/providers/registry.go, internal/providers/registry_test.go, internal/providers/anthropic/request_translation.go, internal/providers/router.go	Trims providerType whitespace in registry registration, adds a trimming test, removes the empty-token check in anthropic image parsing, and centralizes native provider type filtering in the router.

Sequence Diagram(s)

sequenceDiagram
  participant NativeResponseService
  participant Provider
  participant core

  NativeResponseService->>Provider: GetResponse / CancelResponse / DeleteResponse
  Provider-->>NativeResponseService: resp == nil
  NativeResponseService->>core: NewEmptyProviderResponseError(providerType)
  core-->>NativeResponseService: GatewayError (502 Bad Gateway)

Loading

sequenceDiagram
  participant Test
  participant GuardedProvider
  participant processGuardedBatchRequest
  participant batchrewrite
  participant InnerProvider

  Test->>GuardedProvider: CreateBatch(providerType, req)
  GuardedProvider->>processGuardedBatchRequest: rewrite request
  processGuardedBatchRequest-->>GuardedProvider: rewritten request
  GuardedProvider->>batchrewrite: RecordPreparation
  GuardedProvider->>InnerProvider: CreateBatch(rewritten)
  InnerProvider-->>GuardedProvider: response or error
  GuardedProvider->>batchrewrite: CleanupFileFromRouter / CleanupSupersededFileFromRouter

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~55 minutes

Possibly related PRs

• ENTERPILOT/GoModel#163: Both change how guardrails deep-copy opaque JSON fields during message/tool-call cloning.
• ENTERPILOT/GoModel#243: Both touch native response handling and the nil-provider-response path in native_response_service.

Poem

A rabbit hopped through guardrail code so neat,
Cloning each message with nimble feet.
Empty responses got a name,
Router paths now do the same,
Hop hop—cleaner paths, no copy-repeat 🐰

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 17.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately reflects the main changes: removing dead guardrails code, reducing duplication, and fixing provider registry trimming.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch chore-quality-improvements

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 59.34066% with 37 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/providers/router.go	0.00%	16 Missing ⚠️
internal/guardrails/clone.go	80.00%	10 Missing and 2 partials ⚠️
internal/server/native_response_service.go	0.00%	7 Missing ⚠️
internal/core/errors.go	0.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[greptile-apps]

Confidence Score: 5/5

Safe to merge. The registry whitespace fix is narrow and well-tested; all dead-code deletions were verified to be unreachable from the live request path; clone and refactoring changes are semantically equivalent to what they replace.

The registry fix trims providerType at write time, consistent with the existing providerName trim on the same line, and a regression test confirms both type-based and name-based lookups work correctly. The GuardedProvider deletion is safe because app.go has always wired guardrails through the Workflow* patchers, not through the wrapper. The maps.Clone substitutions preserve nil-in/nil-out semantics. The providerTypesSupporting refactoring produces identical output to the three loops it replaced. No behavior change is introduced on any live path.

No files require special attention. internal/guardrails/provider_harness_test.go is the only structurally new file and it is test-only; internal/providers/registry.go carries the one real behavior change and is well-covered by the added test.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Client
    participant Handler
    participant WRP as WorkflowRequestPatcher
    participant Pipeline as Guardrails Pipeline
    participant Provider

    Note over Handler,Provider: Production path (before and after this PR)
    Client->>Handler: POST /v1/chat/completions
    Handler->>WRP: PatchChatRequest(ctx, req)
    WRP->>Pipeline: Process(ctx, messages)
    Pipeline-->>WRP: modified messages
    WRP-->>Handler: patched req
    Handler->>Provider: ChatCompletion(ctx, patched req)
    Provider-->>Handler: response
    Handler-->>Client: 200 OK

    Note over Handler,Provider: Removed dead path — GuardedProvider.ChatCompletion() called processGuardedChat()
    Note over Handler,Provider: then delegated to inner provider, but was never wired into the live path

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Client
    participant Handler
    participant WRP as WorkflowRequestPatcher
    participant Pipeline as Guardrails Pipeline
    participant Provider

    Note over Handler,Provider: Production path (before and after this PR)
    Client->>Handler: POST /v1/chat/completions
    Handler->>WRP: PatchChatRequest(ctx, req)
    WRP->>Pipeline: Process(ctx, messages)
    Pipeline-->>WRP: modified messages
    WRP-->>Handler: patched req
    Handler->>Provider: ChatCompletion(ctx, patched req)
    Provider-->>Handler: response
    Handler-->>Client: 200 OK

    Note over Handler,Provider: Removed dead path — GuardedProvider.ChatCompletion() called processGuardedChat()
    Note over Handler,Provider: then delegated to inner provider, but was never wired into the live path

Loading

_{Reviews (1): Last reviewed commit: "refactor: remove dead guardrails wrapper..." | Re-trigger Greptile}

[coderabbitai]

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 `@internal/guardrails/clone.go`:
- Around line 38-53: The fallback path in cloneMessageContent is returning the
original non-standard content value instead of cloning it, which breaks the
no-mutation contract. Update cloneMessageContent so the default branch in
internal/guardrails/clone.go either produces a real deep copy for unsupported
shapes or, if that is not feasible, explicitly documents the limitation and
restricts callers accordingly; use the existing cloneContentParts and
core.NormalizeContentParts flow to keep behavior consistent for all content
types.

In `@internal/guardrails/provider_harness_test.go`:
- Around line 25-28: The exported Options type in the test harness is too
generic and can collide with future package-level types in guardrails. Rename
the type in provider_harness_test.go to a more specific identifier such as
GuardedProviderOptions, and update all references to it in the harness helpers
and tests so the package namespace stays unambiguous.

🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: c789cca4-e7b2-4c19-b86e-eecd34bc9a09

📥 Commits

Reviewing files that changed from the base of the PR and between abc9129 and b306fae.

📒 Files selected for processing (17)

• internal/core/errors.go
• internal/gateway/inference_execute.go
• internal/guardrails/clone.go
• internal/guardrails/executor.go
• internal/guardrails/provider.go
• internal/guardrails/provider_harness_test.go
• internal/guardrails/provider_test.go
• internal/guardrails/responses_message_apply.go
• internal/providers/anthropic/request_translation.go
• internal/providers/registry.go
• internal/providers/registry_test.go
• internal/providers/responses_adapter.go
• internal/providers/router.go
• internal/responsecache/stream_cache.go
• internal/server/handlers_test.go
• internal/server/native_response_service.go
• internal/usage/extractor.go

💤 Files with no reviewable changes (3)

• internal/providers/anthropic/request_translation.go
• internal/guardrails/provider.go
• internal/guardrails/executor.go