(MOT-4162) harness: minimal identity prompt + on-demand agent playbook skills#558
(MOT-4162) harness: minimal identity prompt + on-demand agent playbook skills#558rohitg00 wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe default harness prompt now enforces contract-first function calls and delegates orchestration, finishing, and building guidance to on-demand skill playbooks. Prompt tests load these playbooks directly and validate their key invariants. ChangesHarness playbook extraction
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
skill-check — worker0 verified, 47 skipped (no docs/).
Four for four. Nicely done. |
The docs moved SDK references from /docs/api-reference/sdk-* to /docs/reference/sdk-*, and the engine wire protocol page lives at /docs/reference/engine-protocol; the old paths 404. Verified live: old paths return 404, new paths return 200 and serve markdown with the .md suffix. Updated the playbook and the prompt invariants that pin the URLs.
There was a problem hiding this comment.
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 `@harness/prompts/default.txt`:
- Around line 21-35: Update the discovery and contract-retrieval instructions in
Steps 1–2 to apply only to undocumented function ids. Explicitly exempt
documented calls such as directory::skills::get, registry calls, and
harness::react trigger targets, while preserving the existing discovery
requirements for all other functions and the documented batching and re-fetch
exceptions.
In `@harness/skills/finishing.md`:
- Around line 21-26: Update Step 3 in the validator registration guidance to
limit parent_session_id filters to children spawned within the current turn. For
trigger-fired or direct-spawn fan-outs without parent links, require per-child
session_id subscriptions or state-key joins so completions reach the validator
and turn_complete is written.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 99bdfe6e-1087-42d2-8041-d3ab40a3180f
📒 Files selected for processing (6)
harness/prompts/default.txtharness/skills/SKILL.mdharness/skills/building.mdharness/skills/finishing.mdharness/skills/orchestration.mdharness/src/prompt/tests.rs
| Step 1. Find the id. Call `engine::functions::list` with `{ search: "<name>" }` or | ||
| `{ prefix: "<worker>::" }` or `{ worker: "<name>" }`. The one-line description is a hint, | ||
| not the contract. | ||
|
|
||
| Step 2. Get the contract. Call `engine::functions::info` with the id you found, e.g. | ||
| `{ function_id: "shell::fs::ls" }`. The answer is the API reference: the request schema, the | ||
| response schema, the description, the owning worker, and the bound triggers. BEFORE the FIRST | ||
| call to a function this session, you must do this step. The `function_id` must be the function | ||
| you want to call. Never pass `engine::functions::info` itself or any `engine::*` / `worker::*` | ||
| discovery function as the id — that only returns metadata about the info function (worker | ||
| `iii-engine-functions`). The discovery functions are documented here; never introspect them. | ||
| If you forget the `function_id` argument, the call fails with `missing field`. A contract you | ||
| fetched earlier this session stays valid — do not fetch it again before later calls; fetch it | ||
| again only when a call fails with `invalid_arguments` / `serialization error` / a missing | ||
| field, or a registry-change notice appears. Need more than one contract at once? Pass | ||
| `{ function_ids: ["a::b", "c::d"] }` and it returns `{ functions: [...] }`, one per id — one | ||
| call, never one per id. | ||
|
|
||
| Step 3. Call the function. The `payload` is a JSON OBJECT, never a string. Match the | ||
| contract exactly: every required field, no extra fields, and the right value formats | ||
| (single binary vs argv array, inline string vs base64, "K=V" entries). Guessing field names | ||
| burns turns and can put workers into degraded states. If a value is long or multi-line | ||
| (source code, JSON, markdown), it is still just a string VALUE of one field — do not turn the | ||
| whole payload into a string. | ||
|
|
||
| Step 4. If you get an error, read it and change something. Never send the same `function` + | ||
| `payload` again unchanged. | ||
|
|
||
| <example> | ||
| user: List the files under /tmp. | ||
| assistant: [calls engine::functions::list { search: "ls" } and finds shell::fs::ls] | ||
| [calls engine::functions::info { function_id: "shell::fs::ls" } to get the contract] | ||
| [calls agent_trigger with function: "shell::fs::ls", payload: { path: "/tmp" }] | ||
| </example> | ||
|
|
||
| # Payload rules | ||
|
|
||
| The most common mistake is sending `payload` as a JSON-encoded string. The worker rejects it | ||
| with `invalid_arguments` / `serialization error: invalid type: string ..., expected struct`. | ||
| `{ function_id: "shell::fs::ls" }`, BEFORE the FIRST call to a function this session. | ||
| The answer is the API reference: the request schema, the response schema, the | ||
| description, the owning worker, and the bound triggers. A contract you fetched stays | ||
| valid all session — re-fetch only after `invalid_arguments` / `serialization error` / a | ||
| missing field, or a registry-change notice. Need several at once? Pass | ||
| `{ function_ids: ["a::b", "c::d"] }` — one call, never one per id. If you forget the | ||
| `function_id` argument, the call fails with `missing field`. Never pass an `engine::*` / | ||
| `worker::*` discovery function as the id — that only returns | ||
| metadata about the info function; the discovery functions are documented here, never | ||
| introspect them. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the discovery loop explicit about documented exceptions.
These lines require discovery and contract retrieval for every function, but the loaded skills explicitly require direct use of documented calls such as directory::skills::get, registry calls, and harness::react as a trigger target. Agents following one rule will violate the other. Narrow this rule to undocumented function ids and enumerate the documented exceptions.
Proposed clarification
- Before every call: did I find the id with `engine::functions::list` ...
+ Before every call to an undocumented function: did I find the id with
+ `engine::functions::list` and retrieve its contract?
+ Use function ids explicitly documented in this prompt or a loaded skill
+ directly, including documented skill/registry calls and trigger targets.Also applies to: 116-120
🤖 Prompt for 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.
In `@harness/prompts/default.txt` around lines 21 - 35, Update the discovery and
contract-retrieval instructions in Steps 1–2 to apply only to undocumented
function ids. Explicitly exempt documented calls such as directory::skills::get,
registry calls, and harness::react trigger targets, while preserving the
existing discovery requirements for all other functions and the documented
batching and re-fetch exceptions.
…caveat Step 2 now states explicitly that functions documented in the prompt (the discovery functions and directory::skills::get) skip the find-id and get-contract steps; everything else keeps the full discipline. finishing.md Step 3 documents the parent_session_id boundary: the filter only matches children that carry the parent link (direct spawns from the session, or reaction spawns whose metadata names it). Reaction- or wave-dispatched children without the link need the run-scope state keys or an explicit per-child join, otherwise their completions never reach the validator and turn_complete is never written.
What
prompts/default.txtshrinks from ~9,450 to 1,676 tokens (tokenizer-measured viacontext::count-tokens). The universal call loop, discovery surface, payload and error rules, security, and conventions stay.skills/orchestration.md(3,686 tok),skills/finishing.md(2,362 tok), andskills/building.md(1,172 tok). iii-directory already serves worker-shipped skills, so agents pull them asharness/orchestration,harness/finishing,harness/building. The prompt instructs: pull the matching playbook BEFORE the first action of that kind, once per session.prompt/tests.rskeeps every content invariant. Tests for moved sections now pin the skill files viainclude_str!, so the doctrine stays test-locked exactly as before; two new tests pin the skill pointers and playbook contents.metadata.session_idso the composed result delivers back into the registering chat. In live testing an agent pinned it and the answer landed in a side session.Why
Test evidence
Live A/B on a rig (engine 0.21.6, harness 1.4.2, claude-sonnet-5, 13 sessions): identical task set, baseline identity prompt vs this minimal core via the router operator override.
harness/orchestrationbefore its firstengine::register_triggerandharness::spawn, unpromptedFAILED:first line,@fnpills)Note: the rig ran harness 1.4.2, where an in-turn spawn still parks and returns the child result; the playbook text intentionally keeps the 1.5.x fire-and-forget doctrine and was not weakened by that observation.
Rollout
default.txtis the router-absent fallback, so this PR alone changes behavior only on rigs without a provider identity prompt.auto_downloadcovers new adds).Draft: direction review wanted before the provider prompts move.
Linear
Closes MOT-4162. Part of MOT-4157.
Coordination
shell::*/coder::*steps insidefp::pipeunder the forwarded fs_scope; the finishing playbook paragraph listing them as refused in a pipe gets updated once that lands.Summary by CodeRabbit
Documentation
Tests