fix(setup): create_sub_agent could never work, and a failed setup left orphans - #661
Conversation
…alues, roll back orphans create_sub_agent passed apiKey=null with a comment claiming vault inheritance that nothing implemented, so setupAgent's required-API-key check rejected every provider needing one — including the anthropic default an omitted provider resolves to. resolveParentLlmProfile now reads the parent at its RESOLVED current version (readAgent(id, null) always throws checkNotNull, which the catch swallowed) and inherits provider, model and credential; only a vault REFERENCE is inherited, never a plaintext key. allowedProviders was checked against the raw argument, so omitting the parameter skipped the allow-list and then resolved to the default. allowedModels judged a model against any provider's list and paired it with the default, so a config restricting openai built an anthropic agent running an openai model. Both now use the effective provider. A failed setup orphaned every document created before the failing step, on a path an LLM can retry in a loop; added a compensating delete. agentName and systemPrompt are bounded. Rebased onto current main, which independently fixed the neighbouring findings. +12 tests.
|
Warning Review limit reached
Next review available in: 32 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull request overview
Hardens dynamic sub-agent setup, inheritance, guardrails, validation, and failure cleanup.
Changes:
- Adds parent LLM profile and credential inheritance.
- Enforces effective provider/model policies and input limits.
- Adds compensating rollback logic and regression tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
CreateSubAgentTool.java |
Resolves inherited provider, model, and credentials. |
AgentSetupService.java |
Adds validation, profile resolution, and rollback. |
DynamicAgentToolsTest.java |
Tests inheritance and allow-list behavior. |
AgentSetupHardeningTest.java |
Tests bounds, rollback, and profile resolution. |
docs/changelog.md |
Documents the setup hardening work. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Six findings, all valid:
- Model inheritance was independent of provider inheritance, so an anthropic
parent creating a sub-agent with provider=ollama and no model paired Ollama
with the parent's Claude model — and because Ollama needs no key nothing
downstream rejected it. Model is now inherited only when the resolved provider
still matches the parent's.
- The model allow-list was still skippable by omission: a null model skipped the
guard and AgentSetupService then substituted DEFAULT_MODEL, deploying a model
the policy never saw. Same fix already applied to the provider.
- resolveParams trims but the guardrail did not, so ' openai ' was accepted
downstream while being rejected by allowedProviders=['openai'] and failing to
match the parent for inheritance.
- isVaultReference only asks whether a value CONTAINS '${vault:', so
'plaintext${vault:key}' was treated as a safe reference and its plaintext half
copied into the child config. Now a full pattern match, as vaultApiKey already
used.
- The rollback iterated String values only, so createApiAgent's List of api-call
locations survived every failure. Now flattened.
- The auto-vaulted secret is created before the LLM document but was never
tracked, so a failed setup left a unique setup.<name>.<timestamp>.apiKey behind
and retries grew the vault without bound. Now recorded and removed on rollback.
…ardening # Conflicts: # docs/changelog.md
|
Merged current Cross-PR interaction worth recording, since #662 landed first: this PR inherits the parent's That is the correct behaviour — the child genuinely is a different agent using that secret — and it is inert by default in two ways: |
…ardening # Conflicts: # docs/changelog.md
Rebuilt onto current
main, which had independently fixed the neighbouring lifecycle and guardrail findings. Only the genuinely-missing work is carried over.AgentSetupServiceis whatcreate_sub_agentcalls, and it deploys to production from an LLM-controlled path.1.
create_sub_agentfailed outright for every provider that needs an API keyThe tool passed
apiKey = nullwith// apiKey — inherited from vault, and its@Pdocs promised "inherits parent if omitted". Nothing implemented either.setupAgentrejects a null key for any non-local provider, and an omitted provider resolves toanthropic— so sub-agent creation only ever worked forollama/jlama/bedrock/oracle-genai.resolveParentLlmProfilenow walks parent agent → workflow → LLM task.DynamicAgentConfig.inheritParentModel— a config field nothing read — drives provider/model inheritance.The parent must be read at its resolved current version:
RestVersionInfo.readdoescheckNotNull(version), soreadAgent(id, null)always threw, the catch swallowed it, and inheritance silently returned null — leaving the very error it was added to remove.Only a vault REFERENCE is inherited, never a plaintext key.
vaultApiKeyfalls back to plaintext when the vault is unconfigured; copying that into a second config would multiply the fallback's blast radius rather than reference one secret twice.2. The allow-lists were checked against the raw argument
allowedProviderswas skipped entirely when the caller omittedprovider— which then resolved to the default insideAgentSetupService. A group restricting providers was bypassable by simply not passing the parameter. Now checked against the effective provider, with the default exposed asAgentSetupService.DEFAULT_PROVIDERso the two files cannot drift.3. A model was judged against the wrong provider
allowedModelsmaps a provider to that provider's permitted models. With no provider named, the check accepted a model from any provider's list and then paired it with the default — so a config restricting openai togpt-4o-minibuilt an anthropic agent runninggpt-4o-mini, which fails at model load and was never authorised.An unnamed provider must now land on a provider the policy actually covers. A named provider keeps its documented "absent list = no restriction" semantics — that behaviour is pinned by tests stating it outright, and changing it is not part of closing this gap.
4. A failed setup orphaned everything created before the failing step
Six to eight documents across as many stores, no transaction, and a wrap-and-rethrow failure path — on a path an LLM can retry in a loop. Added a best-effort compensating delete in reverse creation order: permanent (a soft delete leaves the debris), never cascading (a cascade could reach resources it does not own), and each delete isolated so it can never mask the original failure. Applied to
createApiAgenttoo.5. No length bounds on
agentName/systemPromptBoth LLM-supplied, both persisted — the name into descriptors and the vault key namespace, the prompt verbatim into the LLM config. Bounded before any resource is created.
Not changed
Every auto-vaulted key carries
allowedAgents = ["*"].VaultSecretProviderdocuments that field as "NOT enforced at resolution time", so narrowing it here would imply an enforcement that does not exist. Addressed properly in the deploy-time grant checker PR instead.Testing
269 tests green across the setup and dynamic-agent suites, +12 new.