fix: allow per-node backend selection alongside global --backend flag#74
fix: allow per-node backend selection alongside global --backend flag#74clintecker merged 4 commits intomainfrom
Conversation
Per-node 'backend' attribute now always wins over the global --backend CLI flag, enabling mixed-backend pipelines (issue #70). A node with 'backend: native' uses the native LLM client even when --backend claude-code is set globally. Changes: - selectBackend: document 3-level priority (per-node > global > default) - registerCodergenHandler: register CodergenHandler when graph has per-node backend attrs, even if global default is native with no --backend flag set - ensureNativeBackend: surface a clear, actionable error when a 'backend: native' node has no API keys configured alongside --backend claude-code - New tests: TestSelectBackendNodeAttrWinsOverGlobalClaudeCode, TestSelectBackendNodeAttrClaudeCodeOverGlobalNative, TestSelectBackendNativeAttrWithGlobalClaudeCodeNoClient, TestSelectBackendGlobalClaudeCodeUsedWhenNoNodeAttr, TestGraphHasPerNodeBackend Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 15 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe pull request implements per-node backend attribute support, allowing individual graph nodes to override the global Changes
Sequence Diagram(s)sequenceDiagram
participant Node as Node in Graph
participant SelectBackend as selectBackend()
participant Registry as Handler Registry
participant BackendInstance as Backend Instance<br/>(native or claude-code)
Node->>SelectBackend: Request backend selection
alt Per-node 'backend' attribute exists
SelectBackend->>SelectBackend: Use per-node value
else Per-node attribute missing
SelectBackend->>SelectBackend: Check global defaultBackendName
alt Global flag set
SelectBackend->>SelectBackend: Use global flag
else Global flag not set
SelectBackend->>SelectBackend: Default to native
end
end
SelectBackend->>Registry: Report selected backend
alt graphHasPerNodeBackend detected
Registry->>Registry: Register full CodergenHandler
else No per-node backends
Registry->>Registry: Use existing registration logic
end
Registry->>BackendInstance: Initialize selected backend
alt backend == 'native'
BackendInstance->>BackendInstance: Initialize native LLM client
else backend == 'claude-code'
BackendInstance->>BackendInstance: Initialize claude CLI backend
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pipeline/handlers/codergen.go (1)
170-176: Tighten native-error wording to include thecodergenalias.On Line 172, the message says
backend: native, but this branch is also reachable viabackend: codergen. Consider mentioning both to avoid user confusion.✏️ Suggested wording tweak
- "node requests native backend via \"backend: native\" attr, but no API keys are configured — "+ + "node requests native backend via \"backend: native|codergen\" attr, but no API keys are configured — "+🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pipeline/handlers/codergen.go` around lines 170 - 176, Update the error message returned in the branch that checks h.defaultBackendName (the clause where defaultBackendName == "claude-code" || defaultBackendName == "acp") to mention both "backend: native" and the alias "backend: codergen" so users know this branch is reachable via either attribute; modify the fmt.Errorf call in codergen.go to include both labels (e.g., '"backend: native" (alias "backend: codergen")') while keeping the existing guidance about configuring LLM provider API keys and the --backend %s insertion using h.defaultBackendName.pipeline/handlers/registry.go (1)
216-223: Consider adding a nil-graph guard ingraphHasPerNodeBackendfor defensive safety.Line 217 dereferences
graph.Nodesdirectly. While the current code flow prevents nil from reaching this function, a defensive check prevents accidental panics if the function is called with nil in the future.🛡️ Defensive fix
func graphHasPerNodeBackend(graph *pipeline.Graph) bool { + if graph == nil { + return false + } for _, n := range graph.Nodes { if n.Attrs["backend"] != "" { return true } } return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pipeline/handlers/registry.go` around lines 216 - 223, Add a nil-guard at the start of graphHasPerNodeBackend: check if the incoming *pipeline.Graph (graph) is nil and return false immediately to avoid dereferencing graph.Nodes; keep the existing loop and logic unchanged (the range over graph.Nodes is safe if graph is non-nil). This change should be applied inside the graphHasPerNodeBackend function to prevent panics when a nil graph is passed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pipeline/handlers/codergen.go`:
- Around line 170-176: Update the error message returned in the branch that
checks h.defaultBackendName (the clause where defaultBackendName ==
"claude-code" || defaultBackendName == "acp") to mention both "backend: native"
and the alias "backend: codergen" so users know this branch is reachable via
either attribute; modify the fmt.Errorf call in codergen.go to include both
labels (e.g., '"backend: native" (alias "backend: codergen")') while keeping the
existing guidance about configuring LLM provider API keys and the --backend %s
insertion using h.defaultBackendName.
In `@pipeline/handlers/registry.go`:
- Around line 216-223: Add a nil-guard at the start of graphHasPerNodeBackend:
check if the incoming *pipeline.Graph (graph) is nil and return false
immediately to avoid dereferencing graph.Nodes; keep the existing loop and logic
unchanged (the range over graph.Nodes is safe if graph is non-nil). This change
should be applied inside the graphHasPerNodeBackend function to prevent panics
when a nil graph is passed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0759834-8a53-4b41-b27a-765cbbc18d64
📒 Files selected for processing (5)
CHANGELOG.mdCLAUDE.mdpipeline/handlers/backend_claudecode_test.gopipeline/handlers/codergen.gopipeline/handlers/registry.go
There was a problem hiding this comment.
Pull request overview
This PR fixes mixed-backend pipeline support by ensuring the codergen handler is registered when any node specifies a per-node backend attribute, even if no global --backend flag is set. It also clarifies and tests backend selection precedence and improves the error message when a node requires the native backend but no API keys/client are available.
Changes:
- Register the full
CodergenHandlerwhen the graph contains any per-nodebackendattrs, and make the codergen stub override (codergenFunc) take priority. - Document backend selection precedence (per-node attr > global flag > native default) and improve the “native backend missing client” error when global backend is external.
- Add tests covering backend precedence, missing-client error messaging, and registry detection of per-node backend usage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
pipeline/handlers/registry.go |
Registers CodergenHandler for graphs with per-node backend attrs; adds graphHasPerNodeBackend helper; prioritizes stub handler. |
pipeline/handlers/codergen.go |
Documents backend selection precedence; adds actionable error for backend: native when no client is available under external global backend. |
pipeline/handlers/backend_claudecode_test.go |
Adds tests for backend precedence, error message content, and graphHasPerNodeBackend. |
CLAUDE.md |
Updates documentation to reflect correct precedence of per-node backend over global flag. |
CHANGELOG.md |
Adds an Unreleased “Fixed” entry describing the mixed-backend fix and improved errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot please address all the review feedback |
… alias Agent-Logs-Url: https://github.com/2389-research/tracker/sessions/685feb0b-23b3-490b-a72b-62ab254e33fa Co-authored-by: clintecker <26985+clintecker@users.noreply.github.com>
Agent-Logs-Url: https://github.com/2389-research/tracker/sessions/685feb0b-23b3-490b-a72b-62ab254e33fa Co-authored-by: clintecker <26985+clintecker@users.noreply.github.com>
Addressed both CodeRabbit nitpicks in commit
All 15 packages pass with no regressions. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // selectBackend chooses the appropriate AgentBackend based on node attributes | ||
| // and global default settings. When --backend claude-code is set, ALL nodes | ||
| // go through the claude CLI — non-Anthropic provider/model attrs are stripped | ||
| // so the CLI uses its default model under the user's subscription. | ||
| // and global default settings. Priority: | ||
| // 1. Per-node "backend" attribute (highest — always wins over global flag) | ||
| // 2. Global --backend flag (fallback for nodes without explicit backend attr) | ||
| // 3. Default: native |
There was a problem hiding this comment.
The updated header comment correctly documents that per-node "backend" overrides the global flag, but later in this function there’s still an inline comment saying the global --backend flag applies to all nodes. Please update/remove that inline comment so the function’s internal documentation is consistent with the stated priority.
| // graphHasPerNodeBackend returns true if any node in the graph specifies an | ||
| // explicit "backend" attribute, indicating a mixed-backend pipeline. |
There was a problem hiding this comment.
The docstring says "explicit "backend" attribute", but the implementation only returns true when the value is non-empty (n.Attrs["backend"] != ""). Either adjust the comment to say "non-empty backend attr" or change the check to key existence if empty values should still be treated as explicitly set.
| // graphHasPerNodeBackend returns true if any node in the graph specifies an | |
| // explicit "backend" attribute, indicating a mixed-backend pipeline. | |
| // graphHasPerNodeBackend returns true if any node in the graph specifies a | |
| // non-empty "backend" attribute, indicating a mixed-backend pipeline. |
- Remove stale inline comment in selectBackend claiming global flag applies to all nodes. - graphHasPerNodeBackend docstring says 'non-empty backend attr' to match the actual check. Refs #70 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
registerCodergenHandlerinregistry.goonly registered the fullCodergenHandlerwhenllmClient != nilordefaultBackendwasclaude-code/acp. This meant pipelines with per-nodebackend: claude-codeattrs (but no global--backendflag) didn't get the handler registered. Additionally, the comment onselectBackendincorrectly stated that--backend claude-coderoutes ALL nodes through the claude CLI — in reality, the per-node attr always wins (the code was already correct, just undocumented).registerCodergenHandlernow detects graphs with per-nodebackendattrs viagraphHasPerNodeBackend()and registers the full handler in that casecodergenFunc) now explicitly takes priority (early return), cleaning up the logicensureNativeBackendgives a clear actionable error when abackend: nativenode has no API keys alongside--backend claude-codeselectBackendcomment documents the 3-level priority: per-node attr > global flag > native defaultTest plan
TestSelectBackendNodeAttrWinsOverGlobalClaudeCode—backend: nativeattr overrides global--backend claude-codeTestSelectBackendNodeAttrClaudeCodeOverGlobalNative—backend: claude-codeattr works when global default is nativeTestSelectBackendNativeAttrWithGlobalClaudeCodeNoClient— clear error whenbackend: native+ no API keys + global claude-codeTestSelectBackendGlobalClaudeCodeUsedWhenNoNodeAttr— global--backend claude-codestill applies when no per-node attrTestGraphHasPerNodeBackend— registry detection of mixed-backend graphs (4 subtests)Closes #70
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes