|
| 1 | +# Rig API review: duplication and simplification |
| 2 | + |
| 3 | +This review inventories the public `rig` surface after the addition of Claude |
| 4 | +background/dynamic workflow support, identifies duplicate or overlapping APIs, |
| 5 | +and proposes simplifications that keep the surface minimal. |
| 6 | + |
| 7 | +Compatibility with Claude dynamic workflows is treated as a hard constraint: a |
| 8 | +duplicate that exists *because* Claude injects a primitive under a given name is |
| 9 | +classified as intentional and kept. Everything else is a candidate for removal. |
| 10 | + |
| 11 | +Samples are intentionally left untouched — the proposals below are sequenced so |
| 12 | +sample rewrites happen in a follow-up PR. |
| 13 | + |
| 14 | +## 1. Surface inventory |
| 15 | + |
| 16 | +| Layer | Members | Notes | |
| 17 | +| --- | --- | --- | |
| 18 | +| Module exports (`skills/rig/rig.ts`) | 95 `export` statements | 33 runtime values, the rest types | |
| 19 | +| Schema helpers (`s.*`) | 24 | 13 primitives/aliases, 6 combinators, 5 constraint presets | |
| 20 | +| Prompt helpers (`p.*`) | 17 + callable tag | 11 are prompt *intents*, 6 are formatting helpers | |
| 21 | +| `PromptBuilder` methods | 9 | 6 re-expose `p.*` | |
| 22 | +| Agent surface | `agent`, `AgentSpec` (11 fields), `CallOptions` (4 fields), `AgentFn` (8 members) | | |
| 23 | +| Addons | `steering`, `repair`, `timeout`, `oncePerAgent` + `AgentAddon` middleware | | |
| 24 | +| Workflow surface | `workflow`, `runWorkflow`, `call`/`call.text`/`call.json`/`call.workflow`, `pipeline`, `parallel`, `until`, `phase`, `log`, `currentWorkflow`, `budget` | | |
| 25 | +| Engines | `copilotEngine`, `configureAgent`, `rig/engines/{anthropic,codex,gemini,pi}`, `RIG_ENGINE` env | | |
| 26 | + |
| 27 | +The schema and prompt layers are where the surface has grown fastest; the |
| 28 | +workflow layer is the most recent addition and is largely constrained by Claude |
| 29 | +parity. |
| 30 | + |
| 31 | +## 2. Duplication that is intentional (keep) |
| 32 | + |
| 33 | +These pairs look redundant but exist to preserve Claude dynamic-workflow shape. |
| 34 | +Removing them would break mechanical porting of `.claude/workflows/*.workflow.js` |
| 35 | +scripts, which is the primary reason the workflow layer exists. |
| 36 | + |
| 37 | +| Duplicate | Why it must stay | |
| 38 | +| --- | --- | |
| 39 | +| `context.{parallel, pipeline, until, phase, log}` **and** module-level `parallel`/`pipeline`/`until`/`phase`/`log` | Claude injects these as sandbox globals. Module-level exports let a half-ported script keep calling `phase()`/`log()` at top level; the context members are the typed, run-scoped form. Both are load-bearing. | |
| 40 | +| `call.text(prompt)` / `call.json(prompt, schema)` **and** `agent({...})` + `call(worker, input)` | `call.text`/`call.json` are the 1:1 image of `agent(prompt)` / `agent(prompt, { schema })`. The declared-agent form is rig's own idiom for reuse. Dropping either loses a use case. | |
| 41 | +| `CallOptions.model` / `maxTurns` overriding `AgentSpec` fields | Claude call options (`{ model, phase, label }`) are per-call overrides; this is override semantics, not duplication. | |
| 42 | +| `budget.total/spent()/remaining()` | Direct Claude parity, even though rig meters agent calls rather than tokens. | |
| 43 | + |
| 44 | +**Recommendation:** document these four explicitly as "intentional parity |
| 45 | +duplicates" in `references/claude-workflow-conversion.md` so future reviews do |
| 46 | +not try to collapse them. |
| 47 | + |
| 48 | +## 3. Duplication that should be removed |
| 49 | + |
| 50 | +### 3.1 `AgentFn.inputShape` / `AgentFn.outputShape` (dead aliases) |
| 51 | + |
| 52 | +`AgentFn` exposes `inputSchema`/`outputSchema` *and* `inputShape`/`outputShape` |
| 53 | +as documented aliases of each other, plus `spec.input`/`spec.output` as a third |
| 54 | +path to the same values. The `*Shape` names are a leftover from the removed |
| 55 | +shape-descriptor API and have **zero references** anywhere in the repository |
| 56 | +outside their own definition. |
| 57 | + |
| 58 | +**Action taken in this PR:** removed. No sample, doc, or test change required. |
| 59 | + |
| 60 | +### 3.2 `s.integer` vs `s.int` |
| 61 | + |
| 62 | +Two names, identical behaviour. Current usage: `s.int` 336 references, |
| 63 | +`s.integer` 11. |
| 64 | + |
| 65 | +**Proposal:** keep `s.int`, drop `s.integer`. Requires touching 11 sample/doc |
| 66 | +sites — defer to the sample-rewrite PR. Add a lint rule (`prefer-s-int`) with an |
| 67 | +autofix so the migration is mechanical. |
| 68 | + |
| 69 | +### 3.3 The `p.*` intent matrix (11 intents → 3 + a reference helper) |
| 70 | + |
| 71 | +This is the largest concentration of duplication. Eleven intents encode only |
| 72 | +three operations; the extra names exist purely to vary *where the operand comes |
| 73 | +from* (literal, single vs. many, input field, output field): |
| 74 | + |
| 75 | +| Operation | Literal | Many | From input field | Optional/fallback | |
| 76 | +| --- | --- | --- | --- | --- | |
| 77 | +| read | `p.read` | `p.readAll` | `p.readInput`, `p.readAllInput` | `p.readOptional` | |
| 78 | +| write | `p.write` | — | `p.writeInput` | — | |
| 79 | +| write generated value | `p.writeOutput` | — | `p.writeInput` | — | |
| 80 | +| shell | `p.bash`, `p.bashRaw` | `p.bashEach` | `p.bashEach` | — | |
| 81 | + |
| 82 | +`p.inputField(field)` already exists and returns the string `"input.<field>"`, |
| 83 | +i.e. rig already has a (stringly-typed) notion of a deferred reference. |
| 84 | + |
| 85 | +**Proposal:** promote references to first-class values and make the three |
| 86 | +operations take them as operands. |
| 87 | + |
| 88 | +- `p.input(field)` — deferred reference to an input field (replaces |
| 89 | + `p.inputField`, and becomes a real value rather than a magic string). |
| 90 | +- `p.output(field)` — deferred reference to an output field. |
| 91 | +- `p.read(source, options?)` where `source` is a path, an array of paths, or a |
| 92 | + reference — subsumes `read`, `readAll`, `readInput`, `readAllInput`, and with |
| 93 | + `{ optional: true, fallback }` also `readOptional`. |
| 94 | +- `p.write(target, content, options?)` where either operand may be a literal or |
| 95 | + a reference — subsumes `write`, `writeOutput`, `writeInput`. |
| 96 | +- `p.bash(command, options?)` with `{ each: p.input("field") }` — subsumes |
| 97 | + `bashEach`; `p.bashRaw` stays as the tagged-template escape hatch. |
| 98 | + |
| 99 | +Net effect: 11 intents → 3 intents + 2 reference helpers, with no loss of |
| 100 | +expressiveness and a single place to document intent options. The wire format |
| 101 | +(`PromptIntent.mode`) can keep its existing modes, so the runtime contract is |
| 102 | +unchanged and the change is purely a front-door consolidation. |
| 103 | + |
| 104 | +**Claude impact: none.** Claude dynamic workflows build prompts as plain |
| 105 | +strings; `p.*` has no counterpart there, so this layer is free to shrink. |
| 106 | + |
| 107 | +### 3.4 `PromptBuilder` methods that re-expose `p.*` |
| 108 | + |
| 109 | +`PromptBuilder.{bash, bashRaw, read, var, region}` forward to the identical |
| 110 | +`p.*` helper, and `PromptBuilder.file(path, contents)` is an undocumented alias |
| 111 | +for `p.write` under a different name. `region` additionally exists in two |
| 112 | +shapes: `p.region(...)` returns a string, `builder.region(...)` returns `this`. |
| 113 | + |
| 114 | +**Proposal:** keep only the builder-shaped, chainable methods (`write`, `line`, |
| 115 | +`region`, `var`, `get`, `toString`) and delete the pass-through methods, |
| 116 | +including `file`. Callers use `p.*` directly inside `builder.write(...)`, which |
| 117 | +is what every sample already does (`.use(`-style builder chaining and |
| 118 | +`builder.file` have no sample usage today). |
| 119 | + |
| 120 | +### 3.5 `p.json(value)` is a no-op wrapper |
| 121 | + |
| 122 | +`renderPromptPart` already serializes objects with `JSON.stringify(value, null, 2)`, |
| 123 | +so `${p.json(x)}` and `${x}` produce identical prompt text. |
| 124 | + |
| 125 | +**Proposal:** drop `p.json` and document that objects interpolate as |
| 126 | +pretty-printed JSON. 10 references to migrate. |
| 127 | + |
| 128 | +### 3.6 Three ways to configure per-turn timeouts |
| 129 | + |
| 130 | +`CallOptions.timeout`, the `timeout()` addon, and `CallOptions.signal` (with a |
| 131 | +caller-built `AbortSignal.timeout`) all express the same thing. |
| 132 | + |
| 133 | +**Proposal:** keep `CallOptions.timeout` and `signal`; drop the `timeout()` |
| 134 | +addon. It is the only addon that does not participate in the response cycle — it |
| 135 | +just mutates `context.signal` before `next()`, which the call option already |
| 136 | +does with less ceremony. |
| 137 | + |
| 138 | +### 3.7 Two ways to attach addons |
| 139 | + |
| 140 | +`AgentSpec.addons` and `AgentFn.use(addons)` do the same thing. `use()` has zero |
| 141 | +references in samples and references. |
| 142 | + |
| 143 | +**Proposal:** keep `AgentSpec.addons` as the single, declarative path and remove |
| 144 | +`use()`. This also removes the "is the agent still immutable after definition?" |
| 145 | +ambiguity that `use()` introduces, and it removes the need for the docs' repeated |
| 146 | +caveat that `.use()` accepts *only* addons. |
| 147 | + |
| 148 | +## 4. Growth pressure to watch (no action proposed yet) |
| 149 | + |
| 150 | +- **`s.*` constraint presets.** `nonEmptyString`, `nonEmptyArray`, |
| 151 | + `nonEmptyObject`, `positiveInt`, `nonNegativeInt`, `percent`, `url`, `path`, |
| 152 | + `date` are sugar over `s.string`/`s.number`/`s.array`/`s.record` with one |
| 153 | + constraint. They are individually cheap and all are used, but the pattern |
| 154 | + scales linearly with every new constraint. Prefer adding constraint *options* |
| 155 | + (`s.string({ minLength })`) over new named presets from here on. |
| 156 | +- **`s.literal(x)` vs `s.enum(x)`.** `s.literal` is a one-value `s.enum` with a |
| 157 | + narrower inferred type. Keep, but do not add further single-purpose |
| 158 | + constructors in this family. |
| 159 | +- **Engine selection has four entry points** (`configureAgent`, `copilotEngine`, |
| 160 | + the `rig/engines/*` subpath exports, and `RIG_ENGINE`/API-key autodetection). |
| 161 | + This is currently justified — the subpath exports keep the core typecheck |
| 162 | + light — but it should be documented as one decision table rather than four |
| 163 | + independent mechanisms. |
| 164 | +- **`debug()` is a public export** used mainly by the engine modules. Consider |
| 165 | + demoting it to an internal helper if no program is expected to call it. |
| 166 | + |
| 167 | +## 5. Claude background-workflow compatibility checklist |
| 168 | + |
| 169 | +Every proposal above was checked against the Claude primitive mapping in |
| 170 | +`references/claude-workflow-conversion.md`: |
| 171 | + |
| 172 | +| Proposal | Claude primitive affected | Verdict | |
| 173 | +| --- | --- | --- | |
| 174 | +| Remove `inputShape`/`outputShape` | none | safe | |
| 175 | +| Collapse `p.*` intents | none (prompts are strings in Claude) | safe | |
| 176 | +| Trim `PromptBuilder` pass-throughs | none | safe | |
| 177 | +| Remove `p.json` | none | safe | |
| 178 | +| Remove `timeout()` addon | `{ timeoutMs }` maps to `CallOptions.timeout` | safe | |
| 179 | +| Remove `AgentFn.use()` | none (Claude has no post-definition mutation) | safe | |
| 180 | +| Drop `s.integer` | `{ type: "integer" }` still maps to `s.int` | safe | |
| 181 | +| Keep context/module `phase`/`log`/`parallel`/`pipeline`/`until` | globals injected by the sandbox | **must keep both** | |
| 182 | +| Keep `call.text`/`call.json` | `agent(prompt)` / `agent(prompt, { schema })` | **must keep** | |
| 183 | +| Keep `budget` | `budget.total/spent/remaining` | **must keep** | |
| 184 | + |
| 185 | +Known remaining gaps versus Claude dynamic workflows, unchanged by this review: |
| 186 | +`{ effort }` and `{ agentType }` call options are not modeled, and resume |
| 187 | +journals, worktree isolation, and human checkpoints are runtime features rather |
| 188 | +than API surface. |
| 189 | + |
| 190 | +## 6. Sample audit |
| 191 | + |
| 192 | +All 58 TypeScript samples (`src/samples/*.ts`) and 331 markdown samples |
| 193 | +(`skills/rig/samples/*.md`) were audited against the current API. |
| 194 | + |
| 195 | +- **No compilation issues.** `npm run typecheck` is clean, and the extracted |
| 196 | + markdown programs typecheck (`npm run sample -- |
| 197 | + --testNamePattern="skill markdown samples typecheck"`). No sample referenced |
| 198 | + the removed `inputShape`/`outputShape` aliases, so no migration was required |
| 199 | + for this PR's deletion. |
| 200 | +- **Nine samples were repaired** — defects unrelated to the deletion but found |
| 201 | + while running the suite: |
| 202 | + - `99`, `109`, `113`, `172`, `181`, `182` declared an input on the *root* |
| 203 | + agent, so the launcher refused to run them (`Expected stdin program root |
| 204 | + agent to have no input`). They now source their operands from a delegated |
| 205 | + child agent, a literal, or `p.env`. |
| 206 | + - `174`, `176` imported from `"rig"` twice; merged into a single import. |
| 207 | + - `102` embedded `console.log` inside a `p.bash` snippet; switched to |
| 208 | + `node -p`. |
| 209 | +- **Remaining suite failures are the 30-line budget only.** 192 markdown |
| 210 | + samples exceed the per-sample line cap asserted in `scripts/run-sample.test.ts`. |
| 211 | + This predates the review, is not an API problem, and is left for a dedicated |
| 212 | + sample-trimming pass — CI only runs the typecheck portion of the suite. |
| 213 | + |
| 214 | +Intent usage across markdown samples quantifies the migration cost of the §3.3 |
| 215 | +consolidation: |
| 216 | + |
| 217 | +| Intent | Uses | Post-consolidation form | |
| 218 | +| --- | --- | --- | |
| 219 | +| `p.bash` | 300 | unchanged | |
| 220 | +| `p.readOptional` | 57 | `p.read(path, { optional: true, fallback })` | |
| 221 | +| `p.read` | 29 | unchanged | |
| 222 | +| `p.readInput` | 26 | `p.read(p.input(field))` | |
| 223 | +| `p.writeOutput` | 18 | `p.write(path, p.output(field))` | |
| 224 | +| `p.write` | 15 | unchanged | |
| 225 | +| `p.writeInput` | 3 | `p.write(p.input(field), p.output(field))` | |
| 226 | +| `p.bashEach` | 2 | `p.bash(template, { each: p.input(field) })` | |
| 227 | +| `p.readAll` / `p.readAllInput` | 0 | `p.read([...])` / `p.read(p.input(field))` | |
| 228 | + |
| 229 | +Roughly 110 call sites change, all mechanically — which supports doing the |
| 230 | +migration as one lint-autofixed PR rather than by hand. |
| 231 | + |
| 232 | +## 7. Suggested sequencing |
| 233 | + |
| 234 | +1. **This PR** — the review, removal of the dead `inputShape`/`outputShape` |
| 235 | + aliases (no sample impact), and the nine unrelated sample repairs from §6. |
| 236 | +2. **Next PR** — additive consolidation: introduce `p.input`/`p.output` and the |
| 237 | + unified `p.read`/`p.write`/`p.bash` signatures alongside the existing intents, |
| 238 | + with lint rules and autofixes. |
| 239 | +3. **Follow-up PR** — mechanical sample and reference migration driven by the |
| 240 | + lint autofixes, then delete the superseded intents, `p.json`, `p.inputField`, |
| 241 | + `s.integer`, `timeout()`, `AgentFn.use()`, and the `PromptBuilder` |
| 242 | + pass-throughs. |
| 243 | + |
| 244 | +Staging it this way keeps every intermediate commit green and confines sample |
| 245 | +churn to a single reviewable PR. |
| 246 | + |
| 247 | +## 8. Related references |
| 248 | + |
| 249 | +- [Converting Claude dynamic workflows to rig](../skills/rig/references/claude-workflow-conversion.md) |
| 250 | +- [Dynamic workflows](../skills/rig/references/dynamic-workflows.md) |
| 251 | +- [Agent API and schemas](../skills/rig/references/agent-api.md) |
| 252 | +- [Prompt intents](../skills/rig/references/prompt-intents.md) |
0 commit comments