feat(sdk): build-an-agent reference files + hardened playbook rules - #5106
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR bundles two new reference markdown files (config-schema and trigger-inputs) into the build-an-agent skill via ChangesBuild-an-agent skill reference bundling and instructions
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
| elif isinstance(value, str) and ( | ||
| _AG_EMBED_MARKER in value or _AG_SNIPPET_MARKER in value | ||
| ): | ||
| elif isinstance(value, str) and _AG_SNIPPET_MARKER in value: |
There was a problem hiding this comment.
Why this narrowing is load-bearing for this PR: the new config-schema.md documents the @ag.embed syntax, and this skill ships inlined through the static catalog, so its body/files legitimately contain that literal text. The old string branch raised SkillValidationError on it (caught by test_build_kit_overlay). Still caught after narrowing: a structural embed (mapping keyed @ag.embed) and @{{ snippet tokens, which are the two shapes a real unresolved embed takes; both existing parsing tests pass unchanged. Known one-sided piece: middlewares/running/resolver.py still substring-matches @ag.embed on strings, but there it only gates an extra resolve round-trip (a no-op on inlined configs), not an error. Follow-up candidate.
| - Dropping `harness`, `runner`, or `sandbox` from a fresh full-object commit. Prefer a narrow | ||
| `delta.set` that touches only what you change, so the boilerplate survives the deep merge. | ||
|
|
||
| ## Mistakes that commit fine but break the run |
There was a problem hiding this comment.
This section exists because adversarial review caught the first draft claiming a wrong model alias "fails the commit". It does not: the model field is a free string, so the commit succeeds and the run silently falls back (the validation in #5104 checks the claude/provider pairing, not model naming). The reference now separates commit-time failures from run-time fallbacks, and points at test_run's resolved block, agreeing with the remediation section in the skill body.
| @@ -0,0 +1,89 @@ | |||
| """Drift protection for the ``build-an-agent`` skill's bundled reference files. | |||
There was a problem hiding this comment.
Drift protection for reviewers: this test derives every top-level AgentTemplateSchema field name and every tool type discriminator from the pydantic models at runtime and asserts the hand-written reference mentions them. When the schema grows a field, CI fails until the doc catches up. This was the chosen alternative to generating the markdown from the schema (readability won; the doc explains semantics, not just shapes).
|
@coderabbitai review |
✅ Action performedReview finished.
|
| } | ||
| ``` | ||
|
|
||
| You are a `pi_agenta` agent. Keep `harness`, `runner`, `sandbox`, and `llm` as they are unless |
There was a problem hiding this comment.
Why are you saying you're a Pi agent? I mean, that could be a claude agent, no?
There was a problem hiding this comment.
Fixed in 23794a1 — the reference is harness-neutral now: "your own harness may be pi_agenta, claude, or pi_core. Whatever it is, keep harness, runner, sandbox, and llm as they are unless the user explicitly asks to change one." The preamble sentence names both harnesses it actually applies to and drops the Pi-only persona claim.
|
|
||
| ### llm | ||
|
|
||
| - `model` — the model. How you NAME it depends on the harness (this is the trap): |
There was a problem hiding this comment.
I mean, you should tell it that it's unlikely that it needs to change that in the Delta, but it should keep things as they are unless the user explicitly asks for that.
There was a problem hiding this comment.
Fixed in 23794a1 — the llm section now leads with: "You almost never touch llm in a delta. Keep it exactly as it is unless the user explicitly asks to change the model, provider, or connection. The rules below matter only when they do ask."
| - `claude`: an alias — `default`, `sonnet`, `opus`, or `haiku` — never a raw model id. | ||
| - `provider` — the provider family (`openai`, `anthropic`, ...); inferred from the model string | ||
| when unset. The `claude` harness reaches `anthropic` only. | ||
| - `connection` — `{ "mode": "agenta" | "self_managed", "slug": "<vault-connection>" }`. `agenta` |
There was a problem hiding this comment.
Covered by the same lead-in (it governs the model, provider, connection, and extras bullets).
| configure yourself with — which severs them on your next run. | ||
| - `remove` takes dotted paths, e.g. `parameters.agent.tools`. | ||
|
|
||
| ## Mistakes that fail the commit |
There was a problem hiding this comment.
make sure to update this re other decissions
There was a problem hiding this comment.
Updated in 23794a1 for the #5104 removal: the two sections are now one honest "Mistakes that break your agent" — the commit accepts whatever you send; each bullet states where the mistake actually bites (skill fails to load at run time / config parse rejects it on the next run / Model & Harness never resolves / silent model fallback only visible in test_run's resolved). Verified against the real failure points (parse_skill_templates, coerce_tool_configs, no schema parse anywhere on the commit path).
| `pi_core`/`pi_agenta` harness (Pi selects by provider/id). The model field is a free string, so | ||
| the commit succeeds — the run then silently falls back to a default model. Match the naming to | ||
| the harness and check `test_run`'s `resolved` block to catch a fallback. | ||
| """ |
There was a problem hiding this comment.
I think it should definitely provide real examples of requests. Especially since 90% of the time the two changes that would be made are the instructions and the skills.
There was a problem hiding this comment.
Done in 23794a1 — four complete example payloads, ordered instructions → skill → gateway tool → delta.remove, with the lead noting the first two cover the 90% case. The gateway example carries an explicit caveat: the list shown is shortened, resend your ENTIRE current tools list.
| Passing no `inputs_fields` at all gives the agent the raw context object as inputs — fine for a | ||
| smoke test, but a real agent should get an explicit `messages` entry so the run starts from an | ||
| imperative instruction. | ||
| """ |
There was a problem hiding this comment.
I think you need examples here, and I feel you don't mention anything about variants and how to say which variant to use, etc. latests etc.. part of our discussion
There was a problem hiding this comment.
Added in 23794a1 — the reference-tool bullet now has three concrete entries: bare ref_by: variant slug = runs that variant's LATEST revision (follows every commit), version: "3" = pinned, ref_by: environment = follows what is deployed there. (Trigger revision-binding semantics are deliberately not stated yet — that's the trigger-latest-binding design doc in flight.)
| Everything else is fixed unless the user explicitly asks to change it. Configure yourself with | ||
| `commit_revision` by setting `parameters.agent` fields; do not create a separate app. | ||
|
|
||
| Read `references/config-schema.md` before your first `commit_revision`: it gives the exact shape |
There was a problem hiding this comment.
Reformatted in 23794a1 — one item per line, and the last item renamed to match the new section title.
| `remove_subscription`) over harness builtins. Touch Terminal, RemoteTrigger, File tools, or raw | ||
| HTTP only when your wired tools cannot do the job, and say so when you do. | ||
|
|
||
| ## When something fails |
There was a problem hiding this comment.
update given context on validation
There was a problem hiding this comment.
Updated in 23794a1 — the remediation bullet no longer assumes server validation: "The commit does not validate your config: a wrong shape commits fine and surfaces at run time... verify with test_run after every commit: read resolved and the executed tool list, and when something is off, check the shape against references/config-schema.md and re-commit the fix; do not start over."
mmabrouk
left a comment
There was a problem hiding this comment.
reviewed pelase update and move lane to stack on correct thing
…t references Mahmoud's PR #5106 review applied: harness-neutral wording, llm keep-as-is framing, honest no-server-validation failure modes (one 'mistakes that break your agent' section grounded in the real run-time failure points), instructions+skills examples first with a resend-your-entire-list caveat, reference-tool variant/latest/pinned examples, and the remediation rewrite. Claude-Session: https://claude.ai/code/session_01N2djTMgXnpk84EqtugHDJB
1c73d4b to
23794a1
Compare
Context
Stacked on #5105. A builder agent committing "the wrong things in the wrong format" was the observed symptom: at run time nothing showed the model what
parameters.agentactually looks like.commit_revision.delta.setis a free object, and the skill named four field paths and stopped. The external agenta-skills kit solved this with reference files the agent reads before acting; every merged PR there fixed a real agent mistake. This ports that knowledge to the internal skill, adapted to internal reality.Changes
BUILD_AN_AGENT_SKILLnow bundles two reference files (viaSkillTemplate.files, materialized next to SKILL.md by the runner):references/config-schema.md: the exactparameters.agentshape derived field-by-field fromAgentTemplateSchemaand the tool/skill/MCP models: the six tooltypes with per-type required fields, the skill-template limits and path rules, llm/harness/runner/sandbox enums, delta merge semantics, plus "mistakes that fail the commit" and "mistakes that commit fine but break the run" (model naming is harness-dependent: aliases on claude, provider/id on the Pi harnesses).references/trigger-inputs.md: theinputs_fieldstemplate language, the{event, subscription, scope}fire-context shape, the synthetic schedule event, and the imperative-messages+ sibling-payload pattern.The skill body gains the externally-hardened rules: per-integration connection state is authoritative over the headline match; read the matched event's description before wiring (right integration is not enough); stop when nothing plausibly matches; all four
test_runverdicts with the read-the-side-effect-back rule for external writes; prefer narrow filtered tools over list dumps; write the persona as an explicit imperative; a remediation section (validation error means fix the named fields and re-commit; re-point triggers after any commit; aresolvedmismatch means silent fallback).One SDK fix rides along: the skill parser's unresolved-embed scan flagged any string containing the literal text
@ag.embed, which the new documentation legitimately contains. The string branch now checks only the@{{snippet token; structural embeds (a mapping keyed@ag.embed) are still caught.Scope / risk
Skill content plus the parser narrowing; no op behavior changes. Drift protection: a new test derives every top-level
AgentTemplateSchemafield name and every tooltypediscriminator from the pydantic models and asserts the reference file mentions them, so the hand-written docs fail CI when the schema grows. A string that embeds"@ag.embed"inside JSON-encoded text would no longer be caught by the parser scan; the structural and snippet checks still cover the real unresolved-embed shapes (the two existing parsing tests pass unchanged).Tests
test_agenta_builtins_reference_files.py(drift protection, path validation, wire serialization): part of the 549-test agents suite, all green; apitest_static_catalog.py(31) andtest_build_kit_overlay.py(6) green, confirming the files survive the static catalog and overlay path.POST /api/workflows/revisions/retrievefor__ag__build_an_agenton a dev stack returned both reference files with full content.How to QA
Prerequisites: local dev stack.
Steps:
POST /api/workflows/revisions/retrievewith the reserved slug__ag__build_an_agent.references/config-schema.md.Expected result: the skill carries
fileswith both references; a run materializes them next to SKILL.md.Automated tests:
Edge cases: a config whose skills list contains a real unresolved
@ag.embedmapping must still fail parsing with the clear typed error (covered bytest_parsing.py).https://claude.ai/code/session_01N2djTMgXnpk84EqtugHDJB