[rig-claude] Improve Claude dynamic-workflow compatibility for rig - #344
Conversation
…e count Sample 360 used Promise.all instead of parallel(thunks), which bypasses the shared concurrency limiter and doesn't convert failures to null holes. This diverges from Claude dynamic-workflow semantics where parallel(thunks) is the idiomatic primitive. A user porting a Claude workflow would look for parallel and find a sample using the wrong primitive. Also: the sample was already failing the 30-line test at baseline (60 lines). This commit rewrites it to be idiomatic and within the 30-line limit. 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 /grill-with-docs — requesting changes on two issues: an unanchored schema field and a missing null-hole demonstration.
📋 Key Themes & Highlights
Issues
- Unanchored
labelfield:metricschema requireslabel: s.stringbut neither agent instruction describes what value to emit. This is a repair-loop risk every time the sample is run or adapted. - Missing null-hole guard: The PR's primary teaching claim — that
parallelconverts failures tonullholes — is asserted in prose but not shown in code. The sample should demonstrate it with a null check so readers understand the practical difference fromPromise.all.
Positive Highlights
- ✅ Correct use of
parallel(thunks)overPromise.all— good idiomatic fix - ✅ Shared
metricschema is a clean simplification that avoids TypeScript union-inference issues - ✅ Sample now fits within the 30-line validation limit
- ✅ Explanatory prose in the intro is a useful addition for Claude→rig porters
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 28.9 AIC · ⌖ 4.29 AIC · ⊞ 6.3K
Comment /matt to run again
| }); | ||
| // Agent role: measure branch staleness as a labeled metric. | ||
| const branchAgent = agent({ model: "small", output: metric, | ||
| instructions: p`Count stale/total branches.\n${p.bash("git branch -a 2>/dev/null || echo ''")}` }); |
There was a problem hiding this comment.
[/grill-with-docs] metric schema declares label: s.string, but neither agent instruction tells the model what value to put there — only numeric counting is described. This risks repair loops or silently hallucinated label values.
💡 Suggestion
Either drop the label field (the caller already knows which agent produced which slot), or explicitly instruct each agent to set it:
const branchAgent = agent({ model: "small", output: metric,
instructions: p`Emit { label: "stale-branches", value: <stale count> }.\n${p.bash("git branch -a 2>/dev/null || echo ''")}` });A schema field with no prompt instruction is a latent repair-loop.
| const [branches, commits] = await parallel([ | ||
| () => call(branchAgent, "measure"), | ||
| () => call(commitAgent, "measure"), | ||
| ]); |
There was a problem hiding this comment.
[/grill-with-docs] The PR's stated teaching goal is that parallel converts individual failures to null holes, but the sample destructures the result directly without any null guard. Readers learn the API name but not the semantics that justify using it.
💡 Suggestion
Add a brief inline guard to make the null-hole pattern explicit:
const [branches, commits] = await parallel([
() => call(branchAgent, "measure"),
() => call(commitAgent, "measure"),
]);
// branches or commits may be null if an agent failed
if (!branches || !commits) return "insufficient-data";This turns the sample into a real demonstration of the failure-semantic difference versus Promise.all.
1 participant
Compatibility gap addressed
Sample
360-parallel-branch-analysis-workflow.mdusedPromise.allinstead ofparallel(thunks), which is the idiomatic primitive in both Claude dynamic workflows and rig. This breaks knowledge transfer in two ways:Promise.allbypasses the shared concurrency limiter;parallel(thunks)respects it.Promise.allrejects the entire batch on any failure;parallel(thunks)converts individual failures tonullholes — matching Claude's behavior.A user porting a Claude dynamic workflow who searches for
parallelwould find a sample using the wrong primitive and learn incorrect patterns.Additionally, the original sample was already failing the 30-line validation test at baseline (60 lines vs the 30-line limit), causing
npm run sample -- --testNamePattern="skill markdown samples typecheck"to fail.Why this improves Claude→Rig transfer
The
claude-workflow-conversion.mdreference explicitly mapsparallel(thunks)→parallel(thunks)with identical semantics. Sample 360 is the most prominent heterogeneous-fan-out example but showedPromise.all. Fixing it makes the mapping obvious and keeps the sample runnable.Files changed
skills/rig/samples/360-parallel-branch-analysis-workflow.md— rewrote to useparallel(thunks), simplified to a sharedmetricoutput schema (avoids TypeScript union-type inference issues with heterogeneous thunks), added prose explaining whyparallelis preferred overPromise.allwhen porting Claude workflows, and reduced to exactly 30 lines to pass the test.Validation run
Remaining intentional differences
None introduced by this change. All existing documented differences between Claude dynamic workflows and rig (no
effortoption, noagentType: "Explore", no resume journal) remain as documented inreferences/claude-workflow-conversion.md.