[rig-claude] Improve Claude dynamic-workflow compatibility for rig - #449
Conversation
… call.json schema Adds two tests to src/workflow.test.ts that cover two documented compatibility points between Claude dynamic workflows and rig that had no automated coverage: 1. warnAgents advisory warning: verifies that a run emits exactly one 'warning' event when the warnAgents threshold is crossed — the rig counterpart to the session-level advisory warning in Claude dynamic workflows. 2. call.json with a non-object schema (s.enum): verifies that call.json accepts any s.* schema, not just s.object. This is a documented rig advantage over Claude dynamic workflows, which only support object schemas. Both tests are placed in the 'dynamic-workflow parity' describe block, making them discoverable by anyone porting a Claude dynamic workflow to rig. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — commenting on two test-quality issues; no blocking correctness errors.
📋 Key Themes & Highlights
Key Themes
- Weak message assertion (line 256):
stringContaining("2")is too loose; pin the exact message to make the test a real executable spec. - Missing
configureAgentteardown (line 262): the stub factory is set globally and not reset, which can leak state into unrelated tests running in the same vitest process.
Positive Highlights
- ✅ Both tests are well-placed in the
dynamic-workflow paritydescribe block — immediately discoverable for anyone checking Claude compatibility. - ✅ The
call.json+s.enumtest is an excellent executable spec for a documented rig advantage; the approach and stub are clean. - ✅ PR description is thorough and maps each test to the documented compatibility point it covers.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 31.6 AIC · ⌖ 5.93 AIC · ⊞ 6.3K
Comment /matt to run again
| }); | ||
| const warnings = events.filter((event) => event.type === "warning"); | ||
| expect(warnings).toHaveLength(1); | ||
| expect(warnings[0]).toMatchObject({ type: "warning", message: expect.stringContaining("2") }); |
There was a problem hiding this comment.
[/tdd] The stringContaining("2") assertion is weak — it would pass for any message containing the digit 2. The test should pin the exact message string emitted by the runtime to be a true executable spec.
💡 Suggested assertion
expect(warnings[0]).toMatchObject({
type: "warning",
message: `Workflow scheduled more than 2 agents.`,
});This matches the literal string in rig.ts: `Workflow scheduled more than ${warnAgents} agents.`
| it("call.json accepts a non-object schema (s.enum) — rig advantage over Claude dynamic workflows", async () => { | ||
| // Claude dynamic workflows only support object schemas in agent(prompt, { schema }). | ||
| // rig's call.json accepts any s.* schema: s.enum, s.array, s.string, etc. | ||
| configureAgent(() => ({ |
There was a problem hiding this comment.
[/tdd] configureAgent sets a global factory but is not reset after the test. If vitest runs tests in the same process (default), subsequent tests that do not call configureAgent themselves will inherit this stub factory, causing confusing failures.
💡 Suggested fix
Add cleanup, matching the pattern used in workflow one-off agents:
it("call.json accepts a non-object schema...", async () => {
const originalFactory = /* capture before */ undefined;
configureAgent(() => ({ ask: async () => high, close: async () => {} }));
// ... test body ...
// reset at the end, or use afterEach in a describe block
});Or wrap this test (and the one-off agents tests) in a shared describe with an afterEach(() => configureAgent(defaultAgentFactory())) to keep teardown in one place.
1 participant
Compatibility gap addressed
Two documented Claude dynamic-workflow compatibility points had no automated test coverage:
warnAgentsadvisory warning — Claude dynamic workflows emit a session-level advisory warning when many agents are scheduled. Rig maps this to awarningevent emitted afterwarnAgentsagent calls. No test verified this behavior, making it easy to regress without notice.call.jsonwith non-object schemas — Claude dynamic workflows only support object schemas inagent(prompt, { schema }). Rig'scall.jsonaccepts anys.*schema (s.enum,s.array,s.string, etc.) — a documented rig advantage. No test covered this, so users porting Claude workflows couldn't confirm the richer schema support was real.Why this improves transfer from Claude dynamic workflows to rig
Both tests live in the
dynamic-workflow paritydescribe block insrc/workflow.test.ts, making them immediately discoverable by anyone searching for Claude compatibility coverage. The tests serve as executable specs that:warnAgents→warningevent mapping works as documented inclaude-workflow-conversion.mdcall.jsonis not limited to object schemas, matching the documented advantage over Claude workflowsFiles changed
src/workflow.test.ts— added two tests to thedynamic-workflow paritydescribe blockValidation run
Remaining intentional differences
effortoption — use a more capable model id instead (claude-opus-4-5overclaude-haiku-3-5)agentType: "Explore"— use prompt wording plus a narrowtoolslistmaxTurns+repair()), not process-based ({ retries })limits.maxAgents), not tokens