API review: duplicate surface and simplification proposals

[Copilot]

Reviews the public rig API surface after the addition of Claude background/dynamic workflow support, looking for duplicate APIs and proposing simplifications. Claude compatibility is treated as a hard constraint, so every finding is classified as either intentional parity duplication (keep) or accidental duplication (removable), with a per-proposal compatibility verdict. Samples are deliberately left untouched.

docs/rig-api-review.md (new)

• Inventory — 95 module exports, 24 s.* helpers, 17 p.* helpers, 9 PromptBuilder methods, the workflow layer, four engine entry points.
• Keep (Claude parity) — context vs. module-level phase/log/parallel/pipeline/until, call.text/call.json alongside declared agents, per-call model/maxTurns overrides, budget. Removing any of these breaks mechanical porting of .claude/workflows/*.workflow.js.
• Remove — dead inputShape/outputShape; s.integer (336 s.int uses vs. 11); the 11-intent p.* matrix; PromptBuilder pass-throughs incl. the undocumented file() alias for p.write; the no-op p.json; the timeout() addon (duplicates CallOptions.timeout); AgentFn.use() (duplicates AgentSpec.addons).
• Watch list — linear growth of s.* constraint presets, four engine-selection mechanisms, public debug().
• Sequencing — additive consolidation + lint autofixes first, then a single mechanical sample-migration PR, then deletions. Keeps every intermediate commit green.

The largest finding is the p.* intent matrix: eleven intents encode three operations, varying only in where the operand comes from. Promoting deferred references to first-class values collapses them:

// today
p.read("a.ts"); p.readAll(["a.ts","b.ts"]); p.readInput("path"); p.readAllInput("files"); p.readOptional(".eslintrc", "{}");
p.write("out.md", body); p.writeOutput("report", "out.md"); p.writeInput("outPath", "rendered");
p.bash(cmd); p.bashEach(tmpl, "endpoints");

// proposed
p.read("a.ts"); p.read(["a.ts","b.ts"]); p.read(p.input("path")); p.read(p.input("files")); p.read(".eslintrc", { optional: true, fallback: "{}" });
p.write("out.md", body); p.write("out.md", p.output("report")); p.write(p.input("outPath"), p.output("rendered"));
p.bash(cmd); p.bash(tmpl, { each: p.input("endpoints") });

PromptIntent.mode is unchanged, so this is a front-door consolidation with no runtime-contract change. Claude impact is nil — dynamic workflows build prompts as plain strings.

Code changes

• skills/rig/rig.ts — removed the AgentFn.inputShape/outputShape aliases. They duplicate inputSchema/outputSchema (and spec.input/spec.output) and have zero references repo-wide, making this the only proposal with no sample impact.
• AGENTS.md — dropped the stale "Shape descriptors … promoted via SchemaLike" note; that API no longer exists.

[pelikhan]

@copilot review samples in the repository, fix compilation issues, adapt them to the new APIs, and assess the changes.

[Copilot]

@copilot review samples in the repository, fix compilation issues, adapt them to the new APIs, and assess the changes.

Audited all 58 TS samples and 331 markdown samples (a9dd129).

No compilation issues: npm run typecheck is clean and the extracted markdown programs typecheck. No sample referenced the removed inputShape/outputShape aliases, so the deletion required no adaptation.

Running the full sample suite did surface nine genuinely broken samples, now fixed:

• 99, 109, 113, 172, 181, 182 declared an input on the root agent, so the launcher refused to run them (Expected stdin program root agent to have no input) — including via the daily sampler workflow, which pipes the program to stdin. They now get their operands from a delegated child agent, a literal, or p.env.
• 174, 176 imported from "rig" twice.
• 102 embedded console.log inside a p.bash snippet; switched to node -p.

Remaining suite failures (192) are all the ≤30-line budget in scripts/run-sample.test.ts, which predates this PR and is not an API problem — CI only runs the typecheck portion of the suite, so I left that for a dedicated trimming pass.

Added §6 "Sample audit" to docs/rig-api-review.md with intent-usage counts: p.bash 300, p.readOptional 57, p.read 29, p.readInput 26, p.writeOutput 18, p.write 15, p.writeInput 3, p.bashEach 2, p.readAll/p.readAllInput 0. That's ~110 mechanical call sites for the proposed p.* consolidation, supporting a single lint-autofixed migration PR.

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

Skills-Based Review 🧠

Applied /codebase-design and /improve-codebase-architecture — approving with minor suggestions.

📋 Key Themes & Highlights

Positive Highlights

• ✅ Dead code removed cleanly. inputShape/outputShape had zero repository references; the deletion is surgical and correctly leaves no sample migration.
• ✅ Claude compatibility rigorously checked. The compatibility checklist (§5) is thorough and shows each proposal is safe to proceed with.
• ✅ Sequencing is well-reasoned. Additive consolidation before deletion keeps every intermediate commit green — exactly the right way to handle a 110-callsite migration.
• ✅ Nine samples repaired correctly. The p.env() pattern for removing root-agent input is consistent and idiomatic.
• ✅ AGENTS.md updated to remove the stale shape-descriptor note — keeps the project documentation honest.

Suggestions (non-blocking)

1. Intentional parity duplicates should also be documented in references/claude-workflow-conversion.md (not only in the new review doc) — see inline comment on line 75.
2. Sample 99 (99-bash-each.md) now primarily demonstrates delegation, not p.bashEach — consider a comment or rename to avoid confusion.
3. Sample 182 uses a hardcoded path that won't exist in most repos — p.env("TEMPLATE_PATH", "docs/commit-template.md") would be more consistent with the other fixes.
4. Type signature sketch for p.input/p.output would de-risk the next additive-consolidation PR before 110 call sites are migrated.
5. references/linting.md update for the proposed prefer-s-int rule could be done now to keep the lint reference current even before enforcement.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 76.1 AIC · ⌖ 4.81 AIC · ⊞ 6.3K
_{Comment /matt to run again}

[github-actions]

test comment

[github-actions]

[/codebase-design] Section 2 recommends documenting intentional parity duplicates in references/claude-workflow-conversion.md, but this PR only notes the recommendation in the new review doc — the actual reference update is missing.

💡 Suggested addition to skills/rig/references/claude-workflow-conversion.md

Adding a short "Intentional parity duplicates" section to that file now locks in the decision and prevents the next reviewer from re-examining the same four pairs:

## Intentional parity duplicates

These pairs look redundant but must not be removed:

| Pair | Reason |
|------|--------|
| `context.{parallel,pipeline,until,phase,log}` + module-level | Claude sandbox globals |
| `call.text`/`call.json` + `agent()` | inline-agent 1:1 image |
| `CallOptions.model`/`maxTurns` overrides | per-call override semantics |
| `budget.total/spent()/remaining()` | direct Claude primitive parity |

[github-actions]

[/codebase-design] Sample 99 is supposed to demonstrate p.bashEach, but the root healthProbe agent no longer calls it — it delegates to endpointProbe with hardcoded URLs. The sample still keeps p.bashEach in the child agent, but the root-level comment ("supply the endpoint list and delegate") now makes the sample illustrate delegation rather than its stated bash-each pattern.

💡 Consider

Either retitle to 99-delegation.md (since the primary demonstration is now delegation) or add a comment noting that endpointProbe carries the actual p.bashEach usage while the root agent satisfies the "no-input root" constraint. Without this clarification, readers browsing by filename will be confused by the root agent doing nothing but delegation.

[github-actions]

[/codebase-design] Sample 182 was fixed by removing the input field, but the new hardcoded path docs/commit-template.md is unlikely to exist in most repos where someone runs this sample. The original design (reading path from input) was correct for a template renderer; the fix trades a launcher constraint for a silent runtime failure.

💡 Alternative fix

Use p.env("TEMPLATE_PATH", "docs/commit-template.md") as the source, consistent with how 172-service-health-probe.md was fixed in this same PR (p.env("HEALTH_CHECK_URLS", ...)). This keeps the sample runnable without a matching repo layout and is consistent with the pattern established by the other sample fixes here.

instructions: p`Read the template using ${p.read(p.env("TEMPLATE_PATH", "docs/commit-template.md"))}...`

[github-actions]

[/improve-codebase-architecture] Section 3.3 proposes p.input(field) as a first-class reference value to replace stringly-typed p.inputField(field) — good direction. But the doc doesn't address type safety: p.inputField returns string, so p.read(p.inputField("path")) at least gives string to p.read. If p.input(field) returns an opaque InputRef object, every p.* signature that accepts it will need an overload. Consider sketching the minimal type signature in this doc to de-risk the "additive consolidation" next PR.

💡 Suggested sketch

// Minimal type contract to validate before implementation
type InputRef = { __kind: "input"; field: string };
type OutputRef = { __kind: "output"; field: string };
type PathLike = string | string[] | InputRef;
type ContentLike = string | OutputRef;

function read(source: PathLike, opts?: { optional?: boolean; fallback?: string }): PromptIntent;
function write(target: PathLike, content: ContentLike): PromptIntent;
function bash(cmd: string, opts?: { each?: InputRef }): PromptIntent;

Writing this now catches whether p.read(p.input("files")) needs the array case to allow InputRef (yes it does) before 110 call sites are migrated.

[github-actions]

[/improve-codebase-architecture] s.integer removal (§3.2) is proposed but not acted on in this PR — that's the correct sequencing. However, the proposal says to add a lint rule prefer-s-int, while skills/rig/references/linting.md already documents the linting reference. It's worth confirming whether the lint reference should be updated now (even without the enforcement PR) so the rule exists as a documented decision. Skipping that update means the next PR will need to both add the rule and update the reference, making the commit less atomic.