refactor(templates): simplify strands python agent to barebones agent - #2179
Conversation
|
Claude Security Review: no high-confidence findings. (run) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #2179 +/- ##
===========================================
Coverage ? 97.07%
===========================================
Files ? 536
Lines ? 36955
Branches ? 0
===========================================
Hits ? 35874
Misses ? 1081
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Changes requested
Nice cleanup on the model-provider resolution and the barebones template — the split of harness vs. runtime provider resolution is much clearer, and gating templates in the resolver keeps schema validation simple.
One correctness gap and a small dead-code observation:
Orphaned credential on remove runtime (correctness)
src/core/project/manager.tsx — addResource(resourceType: "runtime") now auto-appends an ApiKeyCredentialProvider credential (e.g. myAgentAnthropicApiKey) and an .env.local entry when a non-Bedrock provider is chosen. removeResource (around line 560+) only cleans up env keys when input.resourceType === "credential" and only filters the runtimes collection — the runtime-scaffolded credential is left behind in the spec, and its .env.local value stays.
Concrete impact: add runtime foo --model-provider anthropic --api-key … → remove runtime foo → add runtime foo --model-provider anthropic --api-key … fails on the second add. The runtime uniqueness check passes (no runtimes left), the resolver pushes fooAnthropicApiKey again, and ProjectSpecSchema rejects the write via uniqueNames("credential") — a confusing error for what looks like a fresh add. Meanwhile the old API-key value in .env.local is silently kept because insertIfNew skips existing keys.
Options:
- In
removeResource, when removing a runtime, also strip any credentials whose names match${runtimeName}(Anthropic|OpenAI|Gemini)ApiKey(or track the association explicitly on the runtime spec entry) and clean their env keys the same way the credential branch does. - Have the runtime resolver record the credential name on the runtime spec (e.g.
modelProviderCredentialName) so removal can look it up without inferring from a naming convention. - At minimum, detect the pre-existing credential in
addResourceruntime branch and produce a clear error/repair instruction instead of relying on the schema failure downstream.
Minor: dead LiteLLM branch in agent-python-strands/model/load.py
ModelProviderSchema for runtime scaffolds is ["Bedrock", "Anthropic", "OpenAI", "Gemini"] and resolveRuntimeModelProvider never produces "LiteLLM", but model/load.py still carries a {{#if (eq modelProvider "LiteLLM")}} block (plus bedrock/us.anthropic.claude-sonnet-4-5-20250514-v1:0 inside it). It's unreachable in the runtime path and can be dropped, or the schema/table extended if LiteLLM is intentionally coming next.
Everything else — the .env.local write on create, the merged mergeSpecEntries move to types.ts, the MODEL_PROVIDERS alias map, the resolver-level guards for MCP and the agent-python template, and the expanded tests — looks good. EnvLocalFile is constructed with the right root path in both create and add paths, and the rollback in addResource correctly covers the env-file mutation for the spec-commit failure case.
4c321ec to
187fc5d
Compare
|
Claude Security Review: no high-confidence findings. (run) |
|
I think the harness reviewer is confused from the stacked PR.
|
Stacked on #2175
Problem
The strands agent for python has grown in complexity so significantly that its difficult to work with. Its attempting to scaffold everything for the user, which leads to an unreadable template, and a fair bit of templating complexity.
Solution
Verification
Also did the same flow in #2175 again for gemini and had agent test the others.