From a16a52d52644b7704945cccd21984f3e70751a7e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 06:59:05 +0000 Subject: [PATCH 1/4] feat: per-role effort, current-model rosters, CLI version warning Roles can now set `effort: low|medium|high|xhigh|max`, validated at save time. Claude Code receives it as --effort; Codex as a root-level model_reasoning_effort override (max maps to xhigh, Codex's ceiling). Unset keeps the CLI's default for the model. The agent_start trace event records the effort a phase ran at. The simple-sdlc and plan-build-test examples move the builder and reviewer to `opus` (medium and high effort) and the planner to `sonnet`, following the current guidance to keep code edits and judgment on the main model. The haiku demos (smoke, scout, two-phase) are unchanged. `jig run` and `jig worker` now warn when the Claude Code CLI is older than 2.1.280, the first release that serves Opus 5.5, since roster aliases resolve to whatever model the installed CLI knows. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LwarQhEaL1ygzyMLvaXvZH --- README.md | 15 ++++++- cmd/jig/run.go | 3 ++ cmd/jig/runtime.go | 11 ++++- cmd/jig/worker.go | 3 ++ docs/quickstart.md | 2 +- examples/definitions/plan-build-test.yaml | 10 ++++- examples/definitions/simple-sdlc.yaml | 14 ++++-- internal/engine/phase.go | 3 +- internal/protocol/definition.go | 20 +++++++++ internal/protocol/definition_test.go | 38 ++++++++++++++++ internal/runtime/claudecode/adapter.go | 49 ++++++++++++++++++++ internal/runtime/claudecode/adapter_test.go | 50 +++++++++++++++++++++ internal/runtime/codex/adapter.go | 20 ++++++++- internal/runtime/codex/adapter_test.go | 23 ++++++++++ internal/runtime/runtime.go | 1 + 15 files changed, 252 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 805c42f..f44a709 100644 --- a/README.md +++ b/README.md @@ -65,6 +65,11 @@ You also need at least one agent CLI, authenticated: | Claude Code | `claude-code` (default) | `claude` logged in | `haiku`, `sonnet`, `opus`, … | | Codex | `codex` | `codex login` | Codex model ids, e.g. `gpt-5.6-sol` | +Model aliases (`haiku`, `sonnet`, `opus`) resolve inside the CLI to the +newest model it knows in that tier, so keep the CLI current: `jig run` and +`jig worker` warn at startup when Claude Code is older than 2.1.280, the first +release that serves Opus 5.5. + One process runs one runtime: the engine holds a single adapter per attempt, so `--runtime` is an operator choice, not a per-role one. The `model:` value in a definition's roster goes straight to that CLI, which is why the stock @@ -124,7 +129,8 @@ The shape: name: my-workflow roster: # one entry per agent role builder: - model: sonnet + model: opus + effort: medium # optional: low | medium | high | xhigh | max system_prompt: | # or system_prompt_path: You are a builder … user_prompt: | @@ -169,6 +175,13 @@ Rules worth knowing before you write one: - **Repair loops must be declared and bounded.** `on_fail` is the only loop construct; a cycle or a missing budget is rejected at save time, not discovered at 2 a.m. +- **Effort is per role, and optional.** `effort` goes to Claude Code as + `--effort`, and to Codex as `model_reasoning_effort` (Codex tops out at + `xhigh`, so `max` runs there). It sets how much a role thinks and verifies, + and thinking is billed as output. A useful split: `medium` for a builder + working to a plan, `high` for a reviewer hunting the edge cases the build + missed. Omit it to keep the CLI's default for the model, and omit it for + `haiku`, which does not take an effort level. - **Validation happens before anything runs.** `jig def validate ` is the same check the store applies at save time, offline. diff --git a/cmd/jig/run.go b/cmd/jig/run.go index 7f8ef33..7ba6be3 100644 --- a/cmd/jig/run.go +++ b/cmd/jig/run.go @@ -128,6 +128,9 @@ func runCommand(ctx context.Context, args []string, stdout, stderr io.Writer) in fmt.Fprintf(stderr, "jig run: %v\n", err) return exitInfraFailed } + if selected.Warning != "" { + fmt.Fprintf(stderr, "jig run: warning: %s\n", selected.Warning) + } result, err := controlplane.DirectRun(ctx, controlplane.DirectRunConfig{ DataDir: *dataDir, diff --git a/cmd/jig/runtime.go b/cmd/jig/runtime.go index 0913c87..f83ddff 100644 --- a/cmd/jig/runtime.go +++ b/cmd/jig/runtime.go @@ -48,6 +48,9 @@ type selectedRuntime struct { Runtime runtime.Runtime Capability protocol.RuntimeCapability Seeder engine.HomeSeeder + // Warning is an operator-facing note from the probe (an out-of-date CLI), + // or "". It never blocks: the probe already proved the CLI runs. + Warning string // Scripted reports the test hook: the enginetest fake needs no auth // material, so seeding is skipped for it entirely. Scripted bool @@ -97,7 +100,13 @@ func selectRuntime(ctx context.Context, name string, seedAuth bool) (selectedRun if !seedAuth { seeder = nil } - return selectedRuntime{Runtime: adapter, Capability: capability, Seeder: seeder}, nil + warning := "" + if name == runtimeClaudeCode { + warning = claudecode.VersionWarning(capability.Version) + } + return selectedRuntime{ + Runtime: adapter, Capability: capability, Seeder: seeder, Warning: warning, + }, nil } // seedClaudeAuth is the darwin-aware Claude Code seeder (KTD11): the minimum diff --git a/cmd/jig/worker.go b/cmd/jig/worker.go index 366a777..ce8cc86 100644 --- a/cmd/jig/worker.go +++ b/cmd/jig/worker.go @@ -114,6 +114,9 @@ func workerCommand(ctx context.Context, args []string, stdout, stderr io.Writer) fmt.Fprintf(stderr, "jig worker: %v\n", err) return exitInfraFailed } + if selected.Warning != "" { + fmt.Fprintf(stderr, "jig worker: warning: %s\n", selected.Warning) + } // Every control-plane write rides a context the interrupt does not // cancel: an interrupted attempt still has to report its terminal state, diff --git a/docs/quickstart.md b/docs/quickstart.md index 31fdf68..b71cf68 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -65,7 +65,7 @@ Common first-run failures: environment variable (and add that variable to the role's `env` allowlist). - *“seed codex auth: … is unreadable”* — run `codex login`, then `jig run --runtime codex …`. Also change the roster's `model:` values: the - stock definitions name Claude Code models (`haiku`, `sonnet`), and the value + stock definitions name Claude Code models (`haiku`, `sonnet`, `opus`), and the value is passed to whichever CLI you selected. Everything the run did is in the trace file, one JSON event per line: diff --git a/examples/definitions/plan-build-test.yaml b/examples/definitions/plan-build-test.yaml index 5c621ce..ba86ac3 100644 --- a/examples/definitions/plan-build-test.yaml +++ b/examples/definitions/plan-build-test.yaml @@ -22,10 +22,15 @@ # worktree, which is the honest default for an open-ended build but the # weakest write boundary jig can enforce (R10). Narrow it to the # directories the work belongs in. +# +# THE ROSTER. The builder writes the code, so it runs on `opus`; the planner +# only reads and plans, on `sonnet`. `effort: medium` sets how much the +# builder thinks and verifies while building to a plan — the test phase is +# its check. Leave `effort` out to take the CLI's default for the model. name: plan-build-test roster: planner: - model: haiku + model: sonnet system_prompt: | You are a read-only planner working in a git repository. You never create, modify, or delete any file; you produce a plan the builder @@ -44,7 +49,8 @@ roster: env: [PATH] writes: [] builder: - model: sonnet + model: opus + effort: medium system_prompt: | You are a builder working in a git repository. You execute the plan you are handed, you write only what the task requires, and you report diff --git a/examples/definitions/simple-sdlc.yaml b/examples/definitions/simple-sdlc.yaml index 9cb0887..7c8c6b7 100644 --- a/examples/definitions/simple-sdlc.yaml +++ b/examples/definitions/simple-sdlc.yaml @@ -27,10 +27,16 @@ # EDIT BEFORE USE: the test command (it detects common entry points and # fails loudly when it finds none) and the builder's `writes` allowlist # (`**` is the weakest boundary jig can enforce — narrow it, R10). +# +# THE ROSTER. The builder and reviewer write and judge code, so they run on +# `opus`; the planner only reads and plans, on `sonnet`. `effort` sets how +# much each role thinks and verifies: medium for building to a plan, high +# for a reviewer whose job is the edge cases a build missed. Leave `effort` +# out to take the CLI's default for the model. name: simple-sdlc roster: planner: - model: haiku + model: sonnet system_prompt: | You are a read-only planner working in a git repository. You never create, modify, or delete any file; you produce a plan the builder @@ -48,7 +54,8 @@ roster: env: [PATH] writes: [] builder: - model: sonnet + model: opus + effort: medium system_prompt: | You are a builder working in a git repository. You execute the plan you are handed, you write only what the task requires, and you report @@ -82,7 +89,8 @@ roster: env: [PATH] writes: ["**"] reviewer: - model: sonnet + model: opus + effort: high system_prompt: | You are a read-only reviewer. You judge the change against the task and the repository's own conventions, and you never create, modify, or diff --git a/internal/engine/phase.go b/internal/engine/phase.go index b4c9e84..2885f54 100644 --- a/internal/engine/phase.go +++ b/internal/engine/phase.go @@ -888,6 +888,7 @@ func (e *execution) runAgentPhaseAttempt( options: runtime.Options{ SystemPrompt: systemPrompt, Model: role.Model, + Effort: role.Effort, Tools: append([]string(nil), role.Tools...), WorkDir: e.attempt.WorktreePath, Env: subprocessEnv(e.runner.config.BaseEnv, role.Env, e.scratch.home), @@ -896,7 +897,7 @@ func (e *execution) runAgentPhaseAttempt( session := e.sessionFor(phase.Owner) e.emit.emit(protocol.EventAgentStart, phase.Name, phase.Owner, map[string]any{ "model": role.Model, "session": session.Key, "can_resume": e.capability.CanResume, - "tools": role.Tools, "phase_attempt": entry, + "effort": role.Effort, "tools": role.Tools, "phase_attempt": entry, }) // death wraps up one dead entry: roll the worktree back to the pre-phase diff --git a/internal/protocol/definition.go b/internal/protocol/definition.go index 60e7d3c..4867d2d 100644 --- a/internal/protocol/definition.go +++ b/internal/protocol/definition.go @@ -41,6 +41,19 @@ const ( RepairExhaustedProceed = "proceed" ) +// EffortLevels is the accepted `effort:` vocabulary, lowest to highest. It is +// Claude Code's `--effort` set; the Codex adapter maps it onto that CLI's +// reasoning-effort setting. +var EffortLevels = []string{"low", "medium", "high", "xhigh", "max"} + +var effortLevels = func() map[string]bool { + levels := make(map[string]bool, len(EffortLevels)) + for _, level := range EffortLevels { + levels[level] = true + } + return levels +}() + // DefinitionSpec is the parsed YAML form of a Job Definition (R1, KTD2): an // ordered phase chain, a per-role roster, and an acceptance predicate of // named checks. Validate enforces the save-time contract; nothing downstream @@ -56,8 +69,11 @@ type DefinitionSpec struct { // role runs with. Prompts are content or a repo-relative path, exclusively. // Env is the per-role environment allowlist (KTD10); SensitiveEnv names the // subset whose values are redacted from every persisted trace (R15). +// Effort, when set, is the per-role effort level handed to the runtime CLI; +// empty leaves the CLI's own default for the model. type RoleSpec struct { Model string `yaml:"model"` + Effort string `yaml:"effort"` Thinking string `yaml:"thinking"` SystemPrompt string `yaml:"system_prompt"` SystemPromptPath string `yaml:"system_prompt_path"` @@ -192,6 +208,10 @@ func (role RoleSpec) validate(name string) error { if strings.TrimSpace(role.Model) == "" { return fmt.Errorf("role %q: model is required", name) } + if role.Effort != "" && !effortLevels[role.Effort] { + return fmt.Errorf("role %q: effort %q is not one of %s", + name, role.Effort, strings.Join(EffortLevels, ", ")) + } if role.SystemPrompt == "" && role.SystemPromptPath == "" { return fmt.Errorf( "role %q: missing system prompt content — set system_prompt or system_prompt_path", name) diff --git a/internal/protocol/definition_test.go b/internal/protocol/definition_test.go index 0e97ae6..7e85879 100644 --- a/internal/protocol/definition_test.go +++ b/internal/protocol/definition_test.go @@ -338,3 +338,41 @@ phases: run: build `, "on_fial") } + +func TestRoleEffortIsParsedWhenInTheVocabulary(t *testing.T) { + for _, level := range EffortLevels { + spec := mustParse(t, ` +name: effort +roster: + builder: + model: opus + effort: `+level+` + system_prompt: s + user_prompt: u +phases: + - name: build + kind: agent + owner: builder +acceptance: [all_phases_passed] +`) + if got := spec.Roster["builder"].Effort; got != level { + t.Fatalf("effort = %q, want %q", got, level) + } + } +} + +func TestUnknownRoleEffortIsRejectedNamingRoleAndLevel(t *testing.T) { + mustReject(t, ` +name: bad +roster: + builder: + model: opus + effort: extreme + system_prompt: s + user_prompt: u +phases: + - name: build + kind: agent + owner: builder +`, `"builder"`, `"extreme"`, "low, medium, high, xhigh, max") +} diff --git a/internal/runtime/claudecode/adapter.go b/internal/runtime/claudecode/adapter.go index fc74a1e..81b5789 100644 --- a/internal/runtime/claudecode/adapter.go +++ b/internal/runtime/claudecode/adapter.go @@ -30,6 +30,7 @@ import ( "io" "os" "os/exec" + "strconv" "strings" "sync" "syscall" @@ -147,6 +148,51 @@ func (a *Adapter) Probe(ctx context.Context) (protocol.RuntimeCapability, error) }, nil } +// RecommendedVersion is the oldest Claude Code release that serves the +// current model generation (Opus 5.5 needs 2.1.280). An older CLI still runs, +// but a roster's `opus` alias resolves to whatever that build knew about. +const RecommendedVersion = "2.1.280" + +// VersionWarning reports why a probed version is older than +// RecommendedVersion, or "" when it is current. A version it cannot parse is +// not a reason to warn: the probe already proved the CLI runs. +func VersionWarning(version string) string { + have, ok := parseVersion(version) + if !ok { + return "" + } + want, _ := parseVersion(RecommendedVersion) + for i := range want { + if have[i] != want[i] { + if have[i] > want[i] { + return "" + } + return fmt.Sprintf("claude CLI %s is older than %s; roster model aliases may not "+ + "resolve to the newest models — run `claude update`", version, RecommendedVersion) + } + } + return "" +} + +// parseVersion reads a dotted major.minor.patch, ignoring any pre-release or +// build suffix on the patch number. +func parseVersion(version string) ([3]int, bool) { + var parsed [3]int + parts := strings.SplitN(version, ".", 3) + if len(parts) != 3 { + return parsed, false + } + parts[2], _, _ = strings.Cut(parts[2], "-") + for i, part := range parts { + number, err := strconv.Atoi(part) + if err != nil { + return parsed, false + } + parsed[i] = number + } + return parsed, true +} + // StartOrContinue sends one prompt into the session. The CLI-native session // id is minted here on first use; later sends resume it. The subprocess // environment is exactly opts.Env (KTD10) — nothing from the worker's own @@ -182,6 +228,9 @@ func (a *Adapter) StartOrContinue( if opts.Model != "" { arguments = append(arguments, "--model", opts.Model) } + if opts.Effort != "" { + arguments = append(arguments, "--effort", opts.Effort) + } if opts.SystemPrompt != "" { arguments = append(arguments, "--system-prompt", opts.SystemPrompt) } diff --git a/internal/runtime/claudecode/adapter_test.go b/internal/runtime/claudecode/adapter_test.go index 48f2fff..5cc08ed 100644 --- a/internal/runtime/claudecode/adapter_test.go +++ b/internal/runtime/claudecode/adapter_test.go @@ -597,3 +597,53 @@ func TestDrainStreamWaitsForTheStderrCaptureBeforeClosingIt(t *testing.T) { t.Fatalf("stderr tail = %q, want the CLI's diagnostic intact", got) } } + +func TestEffortIsPassedOnlyWhenTheRoleSetsIt(t *testing.T) { + adapter := NewWithExecutable(writeStub(t)) + for _, effort := range []string{"high", ""} { + stubDir := t.TempDir() + opts := runtime.Options{ + Model: "opus", Effort: effort, WorkDir: t.TempDir(), Env: stubEnv(stubDir, "ok"), + } + handle, err := adapter.StartOrContinue( + context.Background(), &runtime.Session{Key: "effort"}, "prompt", opts) + if err != nil { + t.Fatal(err) + } + drain(t, handle) + if _, err := handle.Result(); err != nil { + t.Fatal(err) + } + arguments, err := os.ReadFile(filepath.Join(stubDir, "args")) + if err != nil { + t.Fatal(err) + } + argText := string(arguments) + if effort != "" && !strings.Contains(argText, "--effort\n"+effort+"\n") { + t.Errorf("args missing --effort %s:\n%s", effort, argText) + } + if effort == "" && strings.Contains(argText, "--effort") { + t.Errorf("an unset effort must leave the CLI default, got:\n%s", argText) + } + } +} + +func TestVersionWarningFlagsOnlyReleasesOlderThanRecommended(t *testing.T) { + for version, wantWarning := range map[string]bool{ + RecommendedVersion: false, + "2.1.283": false, + "2.2.0": false, + "3.0.0": false, + "2.1.279": true, + "2.0.999": true, + "1.9.0": true, + "2.1.279-beta": true, + "not-a-version": false, + "": false, + } { + warning := VersionWarning(version) + if (warning != "") != wantWarning { + t.Errorf("VersionWarning(%q) = %q, want warning: %v", version, warning, wantWarning) + } + } +} diff --git a/internal/runtime/codex/adapter.go b/internal/runtime/codex/adapter.go index 800bdb8..3002500 100644 --- a/internal/runtime/codex/adapter.go +++ b/internal/runtime/codex/adapter.go @@ -335,7 +335,14 @@ func (a *Adapter) StartOrContinue( // `codex exec resume` accepts neither `--color` nor `--sandbox`, so the // bypass posture has to be expressed with the one flag both subcommands take. func (a *Adapter) arguments(session *runtime.Session, opts runtime.Options) ([]string, string) { - arguments := []string{"exec"} + var arguments []string + if effort := reasoningEffort(opts.Effort); effort != "" { + // A root-level config override, ahead of the subcommand: the root + // propagates -c into both `exec` and `exec resume`, so one placement + // serves both shapes. + arguments = append(arguments, "-c", `model_reasoning_effort="`+effort+`"`) + } + arguments = append(arguments, "exec") resumedID := session.NativeID if resumedID != "" { arguments = append(arguments, "resume") @@ -365,6 +372,17 @@ func (a *Adapter) arguments(session *runtime.Session, opts runtime.Options) ([]s return append(arguments, "-"), resumedID } +// reasoningEffort maps a role's effort level (Claude Code's vocabulary, the +// one definitions validate against) onto Codex's model_reasoning_effort. +// Codex has no level above xhigh, so max runs at xhigh; empty stays empty and +// leaves the CLI's own default in place. +func reasoningEffort(effort string) string { + if effort == "max" { + return "xhigh" + } + return effort +} + // codexHandle is one in-flight send: the CLI process, its anchor-led process // group, and the bounded stream capture. type codexHandle struct { diff --git a/internal/runtime/codex/adapter_test.go b/internal/runtime/codex/adapter_test.go index 27bea15..5500f61 100644 --- a/internal/runtime/codex/adapter_test.go +++ b/internal/runtime/codex/adapter_test.go @@ -806,3 +806,26 @@ func TestDrainStreamWaitsForTheStderrCaptureBeforeClosingIt(t *testing.T) { t.Fatalf("stderr tail = %q, want the CLI's diagnostic intact", got) } } + +func TestEffortBecomesARootReasoningOverrideOnBothShapes(t *testing.T) { + adapter := NewWithExecutable("codex") + for _, test := range []struct{ effort, want string }{ + {"low", `model_reasoning_effort="low"`}, + {"xhigh", `model_reasoning_effort="xhigh"`}, + {"max", `model_reasoning_effort="xhigh"`}, + } { + for _, nativeID := range []string{"", "thread-1"} { + arguments, _ := adapter.arguments( + &runtime.Session{NativeID: nativeID}, runtime.Options{Effort: test.effort}) + if len(arguments) < 3 || arguments[0] != "-c" || arguments[1] != test.want || + arguments[2] != "exec" { + t.Errorf("effort %q (resume=%v): args = %q, want -c %s ahead of exec", + test.effort, nativeID != "", arguments, test.want) + } + } + } + arguments, _ := adapter.arguments(&runtime.Session{}, runtime.Options{}) + if arguments[0] != "exec" { + t.Errorf("an unset effort must add no override, got %q", arguments) + } +} diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 3bd4f69..7ca1b4a 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -60,6 +60,7 @@ type Usage struct { type Options struct { SystemPrompt string Model string + Effort string Tools []string WorkDir string Env []string From 9242460c2027312483c4df3c4e934db289cbfd4f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 08:06:08 +0000 Subject: [PATCH 2/4] feat: risk gate, publish hold, per-role spend cap, and the factory definition Definitions gain four small primitives that together model the review- and-risk half of an agentic software factory: - `if:` guards may compare: `if: "risk == high"` runs a phase when the predicate holds, in the same ==|!= language as on_fail.when. A bare field name still tests truthiness. Malformed comparisons are rejected at save time. - Code phases may report: `reports_fields: true` makes the command's last output line a JSON object whose fields join the envelope view. A reporting phase that prints no report fails; the adapter's own fields are reserved. - Publish may be held: `publish: {hold_when: ""}` is judged after acceptance passes. When it holds, the attempt ends accepted_unpublished with the hold recorded in the result and a publish_held trace event; the publishing runner pushes nothing and the publish-only retry releases it. - Roles may cap spend: `budget_usd` goes to Claude Code as --max-budget-usd. Codex has no such flag, so the adapter refuses a capped role rather than run it uncapped, as it does for tools. examples/definitions/factory.yaml uses all four: plan -> build -> commit -> test -> three specialist reviewers each looping the builder until they approve -> a deterministic risk classifier -> an extra briefing review and a publish hold for high-risk work. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01LwarQhEaL1ygzyMLvaXvZH --- README.md | 31 ++ docs/quickstart.md | 5 + examples/definitions/factory.yaml | 351 ++++++++++++++++++++ internal/engine/code_phase.go | 57 +++- internal/engine/phase.go | 74 ++++- internal/engine/phase_test.go | 124 +++++++ internal/engine/repair.go | 19 +- internal/protocol/definition.go | 78 ++++- internal/protocol/definition_test.go | 85 +++++ internal/runtime/claudecode/adapter.go | 4 + internal/runtime/claudecode/adapter_test.go | 28 ++ internal/runtime/codex/adapter.go | 10 + internal/runtime/codex/adapter_test.go | 9 + internal/runtime/runtime.go | 1 + internal/worker/publish.go | 12 + internal/worker/publish_test.go | 28 ++ internal/worker/registration.go | 5 + 17 files changed, 882 insertions(+), 39 deletions(-) create mode 100644 examples/definitions/factory.yaml diff --git a/README.md b/README.md index f44a709..1d3b8c3 100644 --- a/README.md +++ b/README.md @@ -122,6 +122,7 @@ A definition is data, not a script. Five stock ones ship in | `two-phase.yaml` | an agent phase that writes, verified by a code phase | | `plan-build-test.yaml` | a **code-phase repair edge**: a red suite routes back to the builder | | `simple-sdlc.yaml` | an **agent-phase repair edge** (review → revise → re-review), a conditional retest, and per-phase commit messages | +| `factory.yaml` | the **software factory**: plan → build → commit → test → a panel of specialist reviewers looping the builder until they approve, then a deterministic **risk gate** that holds high-risk work for a person | The shape: @@ -131,6 +132,7 @@ roster: # one entry per agent role builder: model: opus effort: medium # optional: low | medium | high | xhigh | max + budget_usd: 8 # optional: cap on what one send may spend system_prompt: | # or system_prompt_path: You are a builder … user_prompt: | @@ -152,7 +154,17 @@ phases: kind: code if: revised # skipped unless a previous envelope set it truthy command: "go test ./..." + - name: classify-risk + kind: code + reports_fields: true # the last output line is a JSON object of envelope fields + command: "scripts/risk.sh" + - name: review-risk + kind: agent + owner: reviewer + if: "risk == high" # a comparison guard, same language as on_fail.when acceptance: [all_phases_passed, diff_matches_claims] +publish: + hold_when: "risk == high" # accepted but held for a person; publish retry releases it ``` Rules worth knowing before you write one: @@ -182,6 +194,25 @@ Rules worth knowing before you write one: working to a plan, `high` for a reviewer hunting the edge cases the build missed. Omit it to keep the CLI's default for the model, and omit it for `haiku`, which does not take an effort level. +- **Spend is capped per role, and optional.** `budget_usd` is the most one + send by that role may cost; Claude Code enforces it as `--max-budget-usd`. + Codex has no such flag, and a cap it cannot enforce fails the send rather + than run uncapped — the same posture as a `tools` allowlist there. +- **Code phases can report.** A code phase with `reports_fields: true` + prints a JSON object as its last output line, and those fields join the + envelope view that `if:` guards and `publish.hold_when` read. It is how a + deterministic script — a risk classifier scoring paths and diff size — gets + a say in routing without a model in the loop. The adapter's own fields + (`status`, `passed`, `exit_code`, …) are reserved. +- **Guards compare as well as test.** `if: revised` runs a phase when a + previous envelope set the field truthy; `if: "risk == high"` runs it when + the comparison holds, in the same ` ==|!= ` language as + `on_fail.when`. +- **Publish can be held.** `publish: {hold_when: ""}` is judged + after acceptance passes. When it holds, the attempt ends + `accepted_unpublished` with the hold recorded, nothing is pushed, and the + worktree and branch are retained — the publish-only retry is the human + sign-off that releases it. Low-risk work never waits. - **Validation happens before anything runs.** `jig def validate ` is the same check the store applies at save time, offline. diff --git a/docs/quickstart.md b/docs/quickstart.md index b71cf68..28f9f98 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -188,6 +188,11 @@ lands the job in `accepted_unpublished` — a distinct state with a publish-only retry, not a failure: the phases passed and the work is on the branch `jig//`. +The same state is where a definition's `publish.hold_when` parks work on +purpose: `factory.yaml` scores every change for risk and holds high-risk +work with `publish: held` in its result. Read the risk reviewer's briefing in +the trace, then the publish retry is your sign-off. + `Ctrl-C` stops the worker in order: no new claims, in-flight attempts cancelled, terminal states recorded, ephemeral scratch destroyed. diff --git a/examples/definitions/factory.yaml b/examples/definitions/factory.yaml new file mode 100644 index 0000000..092abac --- /dev/null +++ b/examples/definitions/factory.yaml @@ -0,0 +1,351 @@ +# factory.yaml — the agentic software factory, as far as it reaches before a +# person is needed. Plan, build, commit, test, then a panel of specialist +# reviewers that each send the work back to the builder until they approve; +# a deterministic risk classifier scores the result, and high-risk work is +# held for human review instead of shipped. +# +# jig run --def examples/definitions/factory.yaml "add rate limiting to the API" +# +# THE REVIEW LOOP. Three reviewers run in turn — correctness, security, +# maintainability — and each carries its own repair edge: a rejection +# (`approved == false`) dispatches the builder with THAT review's blocking +# list, then re-runs the same reviewer. A reviewer approves the work as it +# stands when it runs, so a revision demanded by the security reviewer is +# seen by the maintainability reviewer but not re-judged by the correctness +# reviewer — order the panel from the review most likely to demand changes to +# the one least likely. `budget: 3` bounds each loop and `exhausted: fail-job` +# means work a reviewer never accepts does not ship. +# +# THE RISK GATE. `classify-risk` is a code phase with `reports_fields: true`: +# its last output line is a JSON object, and jig merges those fields into the +# envelope view the guards and the publish hold read. The script is +# deterministic — paths touched and lines changed, no model — so the same +# diff always scores the same. `if: "risk == high"` runs one more review only +# for high-risk work, and `publish.hold_when` parks it: the attempt ends +# accepted_unpublished with the hold on record, the worktree and branch are +# retained, and the operator's publish retry is the human sign-off. Low-risk +# work publishes on its own. +# +# THE ROSTER. Builders and reviewers are `opus`; the planner and the extra +# high-risk reviewer are `sonnet`. `effort` is medium for building to a plan +# and high for reviewers hunting what the build missed. `budget_usd` caps what +# one send may spend; the cap is enforced by the Claude Code CLI, and the +# Codex runtime refuses a capped role rather than run it uncapped. +# +# EDIT BEFORE USE: the test command (it detects common entry points and +# fails loudly when it finds none), the builder's `writes` allowlist (`**` is +# the weakest boundary jig can enforce — narrow it, R10), and the risk +# classifier's path pattern and line threshold, which should name what is +# dangerous in YOUR repository. +name: factory +roster: + planner: + model: sonnet + budget_usd: 2 + system_prompt: | + You are a read-only planner working in a git repository. You never + create, modify, or delete any file; you produce a plan the builder + will execute, and you report exclusively through your final JSON + envelope. + user_prompt: | + Task: {{prompt}} + + Read enough of this repository to plan the change: which files to + touch, in what order, how the result will be verified, and what + "done" looks like — the concrete condition the reviewers will check. + + Your final JSON must also include: + - "notes_for_next_agent": the plan itself, concrete enough to + execute without re-deriving it + env: [PATH] + writes: [] + builder: + model: opus + effort: medium + budget_usd: 8 + system_prompt: | + You are a builder working in a git repository. You execute the plan + you are handed, you write only what the task requires, and you report + honestly: claim only files you actually created or modified. + + You may be run several times in one job. The first time you are + handed a plan. Every later time you are handed a REVIEW that rejected + your work — its "blocking" list is what you must fix, and nothing + else. Never weaken or delete a test to satisfy a reviewer. + + Before you finish, write your commit message — one subject line, then + a blank line, then why — to the file $HOME/commit-message. It is your + own ephemeral home, outside the repository; it is never committed as a + file, and jig uses it as the message of the commit that carries your + work. + user_prompt: | + Task: {{prompt}} + + The previous phase's report is above. If it is a review with + "approved": false, fix every blocking item it names. Otherwise, + execute the plan. + + Write $HOME/commit-message, then report. + + Your final JSON must also include: + - "changed_files": the repo-relative paths of every file you + created or modified + - "artifacts": the same paths, for existence verification + - "revised": true ONLY if you were fixing a review's blocking + items on this run; false when you were executing the plan + env: [PATH] + writes: ["**"] + correctness-reviewer: + model: opus + effort: high + budget_usd: 4 + system_prompt: | + You are a read-only correctness reviewer. You judge whether the change + does what the task asked, completely and without regressions, against + the repository's own conventions. You never create, modify, or delete + any file. + + A finding is a requirement you checked, with "met" true or false. Your + verdict must agree with your findings: never approve while a finding + is unmet or a blocking item stands. Never report "revised" — that + field belongs to the builder. List only problems you would block the + merge for; for each, give the file and line, why it is wrong, and how + to show it fails. + + Your "status" is "success" whenever you completed the review. The + verdict travels in "approved", never in "status": a rejection you + reported correctly is a phase that worked. + user_prompt: | + Task: {{prompt}} + + Review the change in this worktree — `git diff HEAD~1` shows the + committed work, and any uncommitted changes are a revision in + progress. + + Your final JSON must also include: + - "approved": true when the change is complete and correct + - "findings": a list of {"requirement": "", + "met": true|false, "evidence": ""} + - "blocking": a list of strings naming exactly what must change + before approval (an empty list when you approve) + env: [PATH] + writes: [] + security-reviewer: + model: opus + effort: high + budget_usd: 4 + system_prompt: | + You are a read-only security reviewer. You look only for security + defects the change introduces or leaves in place: injection, broken + authentication or authorization, secrets in code, unsafe + deserialization, path traversal, unvalidated input reaching a + dangerous sink, and dependencies pulled in without need. You never + create, modify, or delete any file, and you do not review style or + correctness — other reviewers own those. + + A finding is a requirement you checked, with "met" true or false. Your + verdict must agree with your findings: never approve while a finding + is unmet or a blocking item stands. Never report "revised". + + Your "status" is "success" whenever you completed the review. The + verdict travels in "approved", never in "status". + user_prompt: | + Task: {{prompt}} + + Review the change in this worktree for security defects — `git diff + HEAD~1` shows the committed work, and any uncommitted changes are a + revision in progress. Read the code paths the change touches far + enough to know where its inputs come from. + + Your final JSON must also include: + - "approved": true when you found nothing you would block for + - "findings": a list of {"requirement": "", + "met": true|false, "evidence": ""} + - "blocking": a list of strings naming exactly what must change + before approval (an empty list when you approve) + env: [PATH] + writes: [] + maintainability-reviewer: + model: opus + effort: high + budget_usd: 4 + system_prompt: | + You are a read-only maintainability reviewer. You judge whether the + change reads like the surrounding code — naming, structure, comment + density, idiom — and whether its tests would catch the bug it fixes + or the regression it risks. You never create, modify, or delete any + file, and you do not re-review correctness or security. + + Block only for what a maintainer would refuse to merge; a preference + is a note in "findings" with "met": true, not a blocking item. Your + verdict must agree with your findings. Never report "revised". + + Your "status" is "success" whenever you completed the review. The + verdict travels in "approved", never in "status". + user_prompt: | + Task: {{prompt}} + + Review the change in this worktree — `git diff HEAD~1` shows the + committed work, and any uncommitted changes are a revision in + progress. + + Your final JSON must also include: + - "approved": true when a maintainer would merge this as-is + - "findings": a list of {"requirement": "", + "met": true|false, "evidence": ""} + - "blocking": a list of strings naming exactly what must change + before approval (an empty list when you approve) + env: [PATH] + writes: [] + risk-reviewer: + model: sonnet + effort: high + budget_usd: 3 + system_prompt: | + You are a read-only reviewer brought in because a deterministic + classifier scored this change HIGH RISK — it touches sensitive paths + or is large. Your job is to write the briefing the human approver will + read: what the change does to the risky paths, what could go wrong in + production, and what to check before releasing it. You never create, + modify, or delete any file, and you do not block: the person does. + + Your "status" is "success" whenever you completed the briefing. + user_prompt: | + Task: {{prompt}} + + The classifier's report is above; its "risky_paths" are where to + look first. `git diff HEAD~1` shows the committed work. + + Your final JSON must also include: + - "approved": true (you inform; the release decision is human) + - "briefing": a short plain-language summary for the approver + - "release_checks": a list of strings — what to verify before + releasing this change + env: [PATH] + writes: [] +phases: + - name: plan + kind: agent + owner: planner + - name: build + kind: agent + owner: builder + gates: + - {name: artifacts_exist} + - {name: diff_matches_claims} + # The commit phases stage with `git add -A` deliberately: by the time a + # code phase runs, the write boundary has already verified every path the + # agent changed against its allowlist and rolled back anything else (R10), + # so what is left in the worktree is exactly the authorized work. + - name: commit-build + kind: code + owner: builder + command: | + set -e + message="$HOME/commit-message" + git add -A + if git diff --cached --quiet; then + echo "commit-build: nothing to commit" + exit 0 + fi + if [ -s "$message" ]; then + git -c user.name=jig -c user.email=jig@localhost \ + -c core.hooksPath=/dev/null commit -q -F "$message" + rm -f "$message" + else + echo 'commit-build: the build phase wrote no $HOME/commit-message; using the fallback' + git -c user.name=jig -c user.email=jig@localhost \ + -c core.hooksPath=/dev/null commit -q -m "jig: build phase" + fi + - name: test + kind: code + owner: builder + command: > + if [ -f Justfile ] || [ -f justfile ]; then just test; + elif [ -f Makefile ]; then make test; + elif [ -f go.mod ]; then go test ./...; + elif [ -f package.json ]; then npm test --silent; + else echo "factory: no test command detected — edit the test phase"; exit 1; fi + on_fail: {run: build, then: rerun-self, budget: 3, exhausted: fail-job} + - name: review-correctness + kind: agent + owner: correctness-reviewer + gates: + - {name: verdict_consistent} + on_fail: {when: "approved == false", run: build, then: rerun-self, budget: 3, exhausted: fail-job} + - name: review-security + kind: agent + owner: security-reviewer + gates: + - {name: verdict_consistent} + on_fail: {when: "approved == false", run: build, then: rerun-self, budget: 3, exhausted: fail-job} + - name: review-maintainability + kind: agent + owner: maintainability-reviewer + gates: + - {name: verdict_consistent} + on_fail: {when: "approved == false", run: build, then: rerun-self, budget: 3, exhausted: fail-job} + - name: commit-revision + kind: code + owner: builder + if: revised + command: | + set -e + message="$HOME/commit-message" + git add -A + if git diff --cached --quiet; then + echo "commit-revision: nothing to commit" + exit 0 + fi + if [ -s "$message" ]; then + git -c user.name=jig -c user.email=jig@localhost \ + -c core.hooksPath=/dev/null commit -q -F "$message" + rm -f "$message" + else + echo 'commit-revision: the builder wrote no $HOME/commit-message; using the fallback' + git -c user.name=jig -c user.email=jig@localhost \ + -c core.hooksPath=/dev/null commit -q -m "jig: revision after review" + fi + - name: retest + kind: code + owner: builder + if: revised + command: > + if [ -f Justfile ] || [ -f justfile ]; then just test; + elif [ -f Makefile ]; then make test; + elif [ -f go.mod ]; then go test ./...; + elif [ -f package.json ]; then npm test --silent; + else echo "factory: no test command detected — edit the retest phase"; exit 1; fi + # Deterministic risk classification. The commits jig made are authored + # "jig", so the base is the newest commit that is not — the run's pinned + # HEAD — and the diff from there is the whole change. The last line printed + # is the report; everything before it is ordinary output. + - name: classify-risk + kind: code + reports_fields: true + command: | + set -eu + base=$(git log --format='%H %an' HEAD | awk '$2 != "jig" { print $1; exit }') + pattern='(^|/)(auth|authn|authz|login|session|secret|token|password|crypto|migrations?|schema|deploy|infra|terraform|helm|k8s)([./_-]|$)|(^|/)\.github/workflows/|(^|/)Dockerfile|(^|/)go\.mod$|(^|/)package(-lock)?\.json$' + threshold=400 + files=$(git diff --name-only "$base" HEAD) + lines=$(git diff --numstat "$base" HEAD | awk '{ total += $1 + $2 } END { print total + 0 }') + risky=$(printf '%s\n' "$files" | grep -E "$pattern" || true) + risky_json=$(printf '%s\n' "$risky" | sed '/^$/d' | sed 's/\\/\\\\/g; s/"/\\"/g; s/.*/"&"/' | paste -sd, -) + risk=low + reason="no sensitive paths, $lines lines changed (threshold $threshold)" + if [ -n "$risky" ]; then + risk=high; reason="touches sensitive paths" + elif [ "$lines" -gt "$threshold" ]; then + risk=high; reason="$lines lines changed exceeds threshold $threshold" + fi + echo "classify-risk: $risk — $reason" + printf '{"risk":"%s","risk_reason":"%s","changed_lines":%s,"risky_paths":[%s]}\n' \ + "$risk" "$reason" "$lines" "$risky_json" + - name: review-risk + kind: agent + owner: risk-reviewer + if: "risk == high" +acceptance: [all_phases_passed, verdict_consistent, diff_matches_claims] +publish: + hold_when: "risk == high" diff --git a/internal/engine/code_phase.go b/internal/engine/code_phase.go index 49b51ea..72a392f 100644 --- a/internal/engine/code_phase.go +++ b/internal/engine/code_phase.go @@ -9,8 +9,10 @@ package engine import ( "context" "encoding/json" + "errors" "fmt" "os/exec" + "strings" "syscall" "time" @@ -80,10 +82,45 @@ func runShellCommand( } } +// reportedFields reads a reports_fields code phase's report: its last +// non-empty output line, which must be a JSON object. Everything before it +// is ordinary output — a script may log freely and report once, at the end. +func reportedFields(outputTail string) (map[string]any, error) { + lines := strings.Split(strings.TrimRight(outputTail, "\n"), "\n") + last := "" + for i := len(lines) - 1; i >= 0; i-- { + if trimmed := strings.TrimSpace(lines[i]); trimmed != "" { + last = trimmed + break + } + } + if last == "" { + return nil, errors.New("the command printed nothing to report") + } + var fields map[string]any + if err := json.Unmarshal([]byte(last), &fields); err != nil { + return nil, fmt.Errorf("the last output line is not a JSON object: %w", err) + } + for key := range fields { + if adapterReservedFields[key] { + return nil, fmt.Errorf("reported field %q is reserved for the adapter envelope", key) + } + } + return fields, nil +} + +// adapterReservedFields are the envelope fields the adapter owns; a +// reports_fields script may not overwrite the record of its own exit. +var adapterReservedFields = map[string]bool{ + "status": true, "summary": true, "passed": true, "failures": true, + "exit_code": true, "output_tail": true, +} + // adapterEnvelope wraps a command result as an envelope (R8): the // sssf:VerifyOutput shape — status, passed, failures — plus the exit status -// and output tail as the evidence a repair-target agent works from. -func adapterEnvelope(command string, result commandResult) parsedEnvelope { +// and output tail as the evidence a repair-target agent works from, plus +// any fields a reports_fields phase declared. +func adapterEnvelope(command string, result commandResult, reported map[string]any) parsedEnvelope { status := protocol.EnvelopeFail summary := fmt.Sprintf("command %q exited %d", command, result.ExitCode) var failures []string @@ -100,14 +137,16 @@ func adapterEnvelope(command string, result commandResult) parsedEnvelope { default: failures = append(failures, fmt.Sprintf("exit %d", result.ExitCode)) } - fields := map[string]any{ - "status": status, - "summary": summary, - "passed": result.Passed(), - "failures": failures, - "exit_code": result.ExitCode, - "output_tail": result.OutputTail, + fields := make(map[string]any, len(reported)+6) + for key, value := range reported { + fields[key] = value } + fields["status"] = status + fields["summary"] = summary + fields["passed"] = result.Passed() + fields["failures"] = failures + fields["exit_code"] = result.ExitCode + fields["output_tail"] = result.OutputTail raw, err := json.Marshal(fields) if err != nil { raw = []byte(`{"status":"fail","summary":"adapter envelope encoding failed"}`) diff --git a/internal/engine/phase.go b/internal/engine/phase.go index 2885f54..2f8fd2f 100644 --- a/internal/engine/phase.go +++ b/internal/engine/phase.go @@ -216,18 +216,21 @@ type exchange struct { // execution is one attempt's run state. type execution struct { - runner *Runner - attempt Attempt - spec *protocol.DefinitionSpec - capability protocol.RuntimeCapability - emit *emitter - scratch *attemptScratch - timeouts timeoutConfig - deadline time.Time - sessions map[string]*runtime.Session - sessionSeq int - transcripts map[string][]exchange - fieldView map[string]any + runner *Runner + attempt Attempt + spec *protocol.DefinitionSpec + capability protocol.RuntimeCapability + emit *emitter + scratch *attemptScratch + timeouts timeoutConfig + deadline time.Time + sessions map[string]*runtime.Session + sessionSeq int + transcripts map[string][]exchange + fieldView map[string]any + // publishHeld is the hold reason once publishHold() fired, so the summary + // says "held" where it would otherwise say "not_attempted". + publishHeld string results []protocol.PhaseResult gateReports map[string]protocol.GateReport seededRoles map[string]bool @@ -302,11 +305,34 @@ func (e *execution) run(ctx context.Context) worker.Outcome { Error: "acceptance predicate failed: " + acceptanceDiagnostic(acceptance), Result: e.summaryJSON(&acceptance)} } + // The declared publish hold is judged last, over the merged field view: + // accepted work a classifier marked for a person ends here with the hold + // on record, and the publishing runner reads it rather than pushing. + if hold := e.publishHold(); hold != "" { + e.publishHeld = hold + e.emit.emit(protocol.EventLog, "", "publish_held", map[string]any{"reason": hold}) + return worker.Outcome{State: protocol.AttemptAcceptedUnpublished, + Result: e.summaryJSON(&acceptance), PublishHold: hold} + } // Publish is U7; direct runs mark no-publish explicitly. return worker.Outcome{State: protocol.AttemptAcceptedUnpublished, Result: e.summaryJSON(&acceptance)} } +// publishHold evaluates the definition's publish.hold_when against the field +// view, returning the human-readable reason when it holds and "" otherwise. +func (e *execution) publishHold() string { + if e.spec.Publish == nil || e.spec.Publish.HoldWhen == "" { + return "" + } + predicate, err := protocol.ParsePredicate(e.spec.Publish.HoldWhen) + if err != nil || !predicateHolds(predicate, e.fieldView) { + return "" + } + return fmt.Sprintf("publish.hold_when %q held (%s = %v)", + e.spec.Publish.HoldWhen, predicate.Field, e.fieldView[predicate.Field]) +} + func acceptanceDiagnostic(acceptance acceptanceResult) string { var failed []string for _, check := range acceptance.Checks { @@ -340,6 +366,10 @@ func (e *execution) summaryJSON(acceptance *acceptanceResult) string { "changed_paths": paths, "publish": "not_attempted", } + if e.publishHeld != "" { + summary["publish"] = "held" + summary["publish_hold"] = e.publishHeld + } if acceptance != nil { summary["acceptance"] = acceptance } @@ -472,7 +502,7 @@ func (e *execution) runChain(ctx context.Context) chainEnd { return chainEnd{endCeiling, ""} } e.freshenLease(ctx) - if phase.If != "" && !truthy(e.fieldView[phase.If]) { + if phase.If != "" && !guardHolds(phase.If, e.fieldView) { e.recordResult(protocol.PhaseResult{ Phase: phase.Name, Kind: phase.Kind, Status: phaseStatusSkipped, }) @@ -754,10 +784,21 @@ func (e *execution) runCodePhase(ctx context.Context, phase protocol.PhaseSpec) if e.cancelled() { return phaseRun{outcome: phaseCancelled, failure: "cancelled during phase " + phase.Name} } - envelope := adapterEnvelope(phase.Command, result) + var reported map[string]any + if phase.ReportsFields && result.Passed() { + fields, err := reportedFields(result.OutputTail) + if err != nil { + // A reporting phase that did not report is a failed phase, not a + // silent success with nothing for the guards downstream to read. + result.ExitCode = -1 + result.StartError = "reports_fields: " + err.Error() + } + reported = fields + } + envelope := adapterEnvelope(phase.Command, result, reported) e.emit.emit(protocol.EventLog, phase.Name, "command_result", map[string]any{ "exit_code": result.ExitCode, "timed_out": result.TimedOut, - "output_tail": result.OutputTail, + "output_tail": result.OutputTail, "reported_fields": reported, }) // Gates verify the adapter envelope's claims too; there is no session to @@ -889,6 +930,7 @@ func (e *execution) runAgentPhaseAttempt( SystemPrompt: systemPrompt, Model: role.Model, Effort: role.Effort, + BudgetUSD: role.BudgetUSD, Tools: append([]string(nil), role.Tools...), WorkDir: e.attempt.WorktreePath, Env: subprocessEnv(e.runner.config.BaseEnv, role.Env, e.scratch.home), @@ -897,7 +939,7 @@ func (e *execution) runAgentPhaseAttempt( session := e.sessionFor(phase.Owner) e.emit.emit(protocol.EventAgentStart, phase.Name, phase.Owner, map[string]any{ "model": role.Model, "session": session.Key, "can_resume": e.capability.CanResume, - "effort": role.Effort, "tools": role.Tools, "phase_attempt": entry, + "effort": role.Effort, "budget_usd": role.BudgetUSD, "tools": role.Tools, "phase_attempt": entry, }) // death wraps up one dead entry: roll the worktree back to the pre-phase diff --git a/internal/engine/phase_test.go b/internal/engine/phase_test.go index 940c632..184d25b 100644 --- a/internal/engine/phase_test.go +++ b/internal/engine/phase_test.go @@ -1487,3 +1487,127 @@ acceptance: [all_phases_passed] t.Fatalf("correction does not name the type problem: %.300q", calls[2].Prompt) } } + +// ---- scenario: reported fields, comparison guards, the publish hold -------- + +// factorySnapshot is the risk-gated tail of the factory shape: a code phase +// classifies the change by reporting fields, a comparison guard routes an +// extra review only for high risk, and the publish hold parks high-risk +// work for a person. +const factorySnapshot = ` +name: factory +roster: + builder: + model: test-model + system_prompt: Build. + user_prompt: "Build the app." + writes: ["src/"] + reviewer: + model: test-model + system_prompt: Review. + user_prompt: "Review the work." + writes: [] +phases: + - {name: build, kind: agent, owner: builder} + - name: classify + kind: code + reports_fields: true + command: | + echo "scanning the diff" + if [ -f src/auth.txt ]; then echo '{"risk":"high","touched":["src/auth.txt"]}'; + else echo '{"risk":"low","touched":[]}'; fi + - {name: security-review, kind: agent, owner: reviewer, if: "risk == high"} +acceptance: [all_phases_passed] +publish: + hold_when: "risk == high" +` + +func TestHighRiskChangeGetsTheExtraReviewAndHoldsPublish(t *testing.T) { + repo := initRepo(t) + fake := enginetest.New( + enginetest.Step{Files: map[string]string{"src/auth.txt": "token check"}, + Text: envelope(map[string]any{"status": "success", "summary": "built auth"})}, + enginetest.Step{Text: envelope(map[string]any{"status": "success", "approved": true, + "summary": "security ok"})}, + ) + sink := &recordingSink{} + runner := newTestRunner(t, fake, sink, nil) + outcome := runner.Execute(context.Background(), testAttempt(factorySnapshot, nil, repo)) + if outcome.State != protocol.AttemptAcceptedUnpublished { + t.Fatalf("state = %q (%s), want accepted_unpublished", outcome.State, outcome.Error) + } + if fake.Remaining() != 0 { + t.Fatal("the security review did not run for a high-risk change") + } + if !strings.Contains(outcome.PublishHold, `"risk == high"`) || + !strings.Contains(outcome.PublishHold, "risk = high") { + t.Fatalf("PublishHold = %q, want the held predicate and the field value", outcome.PublishHold) + } + if !sink.has(protocol.EventLog, "publish_held") { + t.Fatal("no publish_held trace event") + } + var summary map[string]any + if err := json.Unmarshal([]byte(outcome.Result), &summary); err != nil { + t.Fatal(err) + } + if summary["publish"] != "held" || summary["publish_hold"] == nil { + t.Fatalf("summary publish = %v / %v, want held with a reason", summary["publish"], summary["publish_hold"]) + } +} + +func TestLowRiskChangeSkipsTheExtraReviewAndIsNotHeld(t *testing.T) { + repo := initRepo(t) + fake := enginetest.New( + enginetest.Step{Files: map[string]string{"src/app.txt": "v1"}, + Text: envelope(map[string]any{"status": "success", "summary": "built"})}, + ) + sink := &recordingSink{} + runner := newTestRunner(t, fake, sink, nil) + outcome := runner.Execute(context.Background(), testAttempt(factorySnapshot, nil, repo)) + if outcome.State != protocol.AttemptAcceptedUnpublished || outcome.PublishHold != "" { + t.Fatalf("outcome = %+v, want accepted_unpublished with no hold", outcome) + } + if fake.Remaining() != 0 { + t.Fatal("the security review ran for a low-risk change") + } + if !sink.has(protocol.EventLog, "phase_skipped") || sink.has(protocol.EventLog, "publish_held") { + t.Fatal("expected the review skipped and no hold") + } + var summary map[string]any + if err := json.Unmarshal([]byte(outcome.Result), &summary); err != nil { + t.Fatal(err) + } + if summary["publish"] != "not_attempted" { + t.Fatalf("summary publish = %v, want not_attempted", summary["publish"]) + } +} + +func TestAReportingPhaseThatDoesNotReportFails(t *testing.T) { + repo := initRepo(t) + fake := enginetest.New( + enginetest.Step{Files: map[string]string{"src/app.txt": "v1"}, + Text: envelope(map[string]any{"status": "success", "summary": "built"})}, + ) + snapshot := strings.Replace(factorySnapshot, + `else echo '{"risk":"low","touched":[]}'; fi`, `else echo "no report here"; fi`, 1) + outcome := newTestRunner(t, fake, &recordingSink{}, nil). + Execute(context.Background(), testAttempt(snapshot, nil, repo)) + if outcome.State != protocol.AttemptFailed || !strings.Contains(outcome.Error, "reports_fields") { + t.Fatalf("outcome = %+v, want a failed attempt naming reports_fields", outcome) + } +} + +func TestAReportedFieldMayNotOverwriteTheAdaptersOwn(t *testing.T) { + repo := initRepo(t) + fake := enginetest.New( + enginetest.Step{Files: map[string]string{"src/app.txt": "v1"}, + Text: envelope(map[string]any{"status": "success", "summary": "built"})}, + ) + snapshot := strings.Replace(factorySnapshot, + `else echo '{"risk":"low","touched":[]}'; fi`, `else echo '{"passed":true,"risk":"low"}'; fi`, 1) + outcome := newTestRunner(t, fake, &recordingSink{}, nil). + Execute(context.Background(), testAttempt(snapshot, nil, repo)) + if outcome.State != protocol.AttemptFailed || !strings.Contains(outcome.Error, "reserved") { + t.Fatalf("outcome = %+v, want a failed attempt naming the reserved field", outcome) + } +} diff --git a/internal/engine/repair.go b/internal/engine/repair.go index 8047073..9d10bf2 100644 --- a/internal/engine/repair.go +++ b/internal/engine/repair.go @@ -43,9 +43,22 @@ func literalEquals(value any, literal string) bool { } } -// truthy is the `if: ` guard test (KTD2): the phase runs -// iff a previous envelope set the field to something other than false, -// zero, empty, or null. +// guardHolds is the `if:` guard test (KTD2) over the chain's merged field +// view. A bare field name is truthy(field); a ` ==|!= ` +// predicate is the same comparison repair edges use. Validation already +// proved a multi-token guard parses, so a parse error here is unreachable +// and reads as "guard did not hold" rather than a crash. +func guardHolds(guard string, fields map[string]any) bool { + if !protocol.IsPredicate(guard) { + return truthy(fields[guard]) + } + predicate, err := protocol.ParsePredicate(guard) + return err == nil && predicateHolds(predicate, fields) +} + +// truthy is the bare-field `if:` guard test (KTD2): the phase runs iff a +// previous envelope set the field to something other than false, zero, +// empty, or null. func truthy(value any) bool { switch typed := value.(type) { case nil: diff --git a/internal/protocol/definition.go b/internal/protocol/definition.go index 4867d2d..0ce426d 100644 --- a/internal/protocol/definition.go +++ b/internal/protocol/definition.go @@ -63,6 +63,18 @@ type DefinitionSpec struct { Roster map[string]RoleSpec `yaml:"roster"` Phases []PhaseSpec `yaml:"phases"` Acceptance []string `yaml:"acceptance"` + Publish *PublishSpec `yaml:"publish"` +} + +// PublishSpec is the definition's say over delivery. HoldWhen is a declared +// envelope predicate (the `on_fail.when` language) evaluated against the +// chain's merged field view once acceptance has passed: when it holds, the +// attempt ends accepted_unpublished with a recorded hold instead of a branch +// and pull request, and the operator's publish retry is the human review +// step that releases it. A change a classifier scored `risk == high` waits +// for a person; everything else ships. +type PublishSpec struct { + HoldWhen string `yaml:"hold_when"` } // RoleSpec is one roster entry: the model, prompts, and allowlists an agent @@ -70,10 +82,14 @@ type DefinitionSpec struct { // Env is the per-role environment allowlist (KTD10); SensitiveEnv names the // subset whose values are redacted from every persisted trace (R15). // Effort, when set, is the per-role effort level handed to the runtime CLI; -// empty leaves the CLI's own default for the model. +// empty leaves the CLI's own default for the model. BudgetUSD, when +// positive, caps what one send by this role may spend; the runtime CLI +// enforces it, so a runtime with no such control refuses the send rather +// than run uncapped. type RoleSpec struct { Model string `yaml:"model"` Effort string `yaml:"effort"` + BudgetUSD float64 `yaml:"budget_usd"` Thinking string `yaml:"thinking"` SystemPrompt string `yaml:"system_prompt"` SystemPromptPath string `yaml:"system_prompt_path"` @@ -87,16 +103,21 @@ type RoleSpec struct { // PhaseSpec is one link of the chain. Agent phases have an Owner role from // the roster; code phases have a Command (and Owner only if they need a -// role's environment). If names an envelope field: the phase is skipped -// unless a previous envelope set that field truthy (KTD2). +// role's environment). If guards the phase (KTD2): a bare field name runs +// the phase only when a previous envelope set that field truthy; a +// ` == ` predicate runs it only when the comparison holds. +// ReportsFields, on a code phase, makes the command's last stdout line a +// JSON object of envelope fields — the way a deterministic script (a risk +// classifier, say) speaks to `if:` guards and the publish hold. type PhaseSpec struct { - Name string `yaml:"name"` - Kind string `yaml:"kind"` - Owner string `yaml:"owner"` - Command string `yaml:"command"` - If string `yaml:"if"` - Gates []GateSpec `yaml:"gates"` - OnFail *RepairEdge `yaml:"on_fail"` + Name string `yaml:"name"` + Kind string `yaml:"kind"` + Owner string `yaml:"owner"` + Command string `yaml:"command"` + If string `yaml:"if"` + ReportsFields bool `yaml:"reports_fields"` + Gates []GateSpec `yaml:"gates"` + OnFail *RepairEdge `yaml:"on_fail"` } // GateSpec configures one built-in gate on a phase. Budget is the @@ -133,6 +154,12 @@ type EnvelopePredicate struct { Literal string } +// IsPredicate reports whether an `if:` guard is a comparison rather than a +// bare field name: anything with more than one whitespace-separated token +// must parse as a predicate, so a typo like `risk = high` is a save-time +// error rather than a guard on a field literally named "risk = high". +func IsPredicate(guard string) bool { return len(strings.Fields(guard)) > 1 } + // ParsePredicate parses a declared envelope predicate, rejecting anything // that is not exactly ` <==|!=> `. func ParsePredicate(expression string) (EnvelopePredicate, error) { @@ -201,7 +228,23 @@ func (spec *DefinitionSpec) Validate() error { if err := spec.validateRepairAcyclic(phasesByName); err != nil { return err } - return spec.validateAcceptance() + if err := spec.validateAcceptance(); err != nil { + return err + } + return spec.validatePublish() +} + +func (spec *DefinitionSpec) validatePublish() error { + if spec.Publish == nil { + return nil + } + if strings.TrimSpace(spec.Publish.HoldWhen) == "" { + return fmt.Errorf("publish: hold_when is required when publish is declared") + } + if _, err := ParsePredicate(spec.Publish.HoldWhen); err != nil { + return fmt.Errorf("publish: hold_when: %w", err) + } + return nil } func (role RoleSpec) validate(name string) error { @@ -212,6 +255,9 @@ func (role RoleSpec) validate(name string) error { return fmt.Errorf("role %q: effort %q is not one of %s", name, role.Effort, strings.Join(EffortLevels, ", ")) } + if role.BudgetUSD < 0 { + return fmt.Errorf("role %q: budget_usd must not be negative", name) + } if role.SystemPrompt == "" && role.SystemPromptPath == "" { return fmt.Errorf( "role %q: missing system prompt content — set system_prompt or system_prompt_path", name) @@ -238,6 +284,11 @@ func (spec *DefinitionSpec) validatePhase(phase PhaseSpec, phases map[string]Pha if phase.Command != "" { return fmt.Errorf("phase %q: agent phases do not take a command", phase.Name) } + if phase.ReportsFields { + return fmt.Errorf( + "phase %q: reports_fields is for code phases — an agent phase reports through its envelope", + phase.Name) + } case PhaseKindCode: if strings.TrimSpace(phase.Command) == "" { return fmt.Errorf("phase %q: code phases require a command", phase.Name) @@ -246,6 +297,11 @@ func (spec *DefinitionSpec) validatePhase(phase PhaseSpec, phases map[string]Pha return fmt.Errorf("phase %q: kind %q is not %q or %q", phase.Name, phase.Kind, PhaseKindAgent, PhaseKindCode) } + if IsPredicate(phase.If) { + if _, err := ParsePredicate(phase.If); err != nil { + return fmt.Errorf("phase %q: if: %w", phase.Name, err) + } + } if phase.Owner != "" { if _, defined := spec.Roster[phase.Owner]; !defined { return fmt.Errorf("phase %q: owner role %q is not defined in the roster", diff --git a/internal/protocol/definition_test.go b/internal/protocol/definition_test.go index 7e85879..d7e89f6 100644 --- a/internal/protocol/definition_test.go +++ b/internal/protocol/definition_test.go @@ -376,3 +376,88 @@ phases: owner: builder `, `"builder"`, `"extreme"`, "low, medium, high, xhigh, max") } + +func TestComparisonIfGuardParsesAndMalformedOneIsRejected(t *testing.T) { + spec := mustParse(t, ` +name: guard +roster: + builder: {model: opus, system_prompt: s, user_prompt: u} +phases: + - {name: classify, kind: code, command: "echo '{\"risk\":\"high\"}'", reports_fields: true} + - {name: extra-review, kind: agent, owner: builder, if: "risk == high"} + - {name: retest, kind: code, command: "true", if: revised} +acceptance: [all_phases_passed] +`) + if !IsPredicate(spec.Phases[1].If) || IsPredicate(spec.Phases[2].If) { + t.Fatalf("IsPredicate misclassified the guards %q / %q", spec.Phases[1].If, spec.Phases[2].If) + } + mustReject(t, ` +name: bad +roster: + builder: {model: opus, system_prompt: s, user_prompt: u} +phases: + - {name: build, kind: agent, owner: builder, if: "risk = high"} +`, `"build"`, "if:", `"="`) +} + +func TestReportsFieldsIsRejectedOnAgentPhases(t *testing.T) { + mustReject(t, ` +name: bad +roster: + builder: {model: opus, system_prompt: s, user_prompt: u} +phases: + - {name: build, kind: agent, owner: builder, reports_fields: true} +`, `"build"`, "reports_fields is for code phases") +} + +func TestPublishHoldWhenMustBeAPredicate(t *testing.T) { + spec := mustParse(t, ` +name: hold +roster: + builder: {model: opus, system_prompt: s, user_prompt: u} +phases: + - {name: build, kind: agent, owner: builder} +publish: + hold_when: "risk == high" +`) + if spec.Publish == nil || spec.Publish.HoldWhen != "risk == high" { + t.Fatalf("publish = %+v", spec.Publish) + } + mustReject(t, ` +name: bad +roster: + builder: {model: opus, system_prompt: s, user_prompt: u} +phases: + - {name: build, kind: agent, owner: builder} +publish: + hold_when: risky +`, "publish: hold_when", "not of the form") + mustReject(t, ` +name: bad +roster: + builder: {model: opus, system_prompt: s, user_prompt: u} +phases: + - {name: build, kind: agent, owner: builder} +publish: {} +`, "hold_when is required") +} + +func TestNegativeRoleBudgetIsRejected(t *testing.T) { + spec := mustParse(t, ` +name: budget +roster: + builder: {model: opus, budget_usd: 2.5, system_prompt: s, user_prompt: u} +phases: + - {name: build, kind: agent, owner: builder} +`) + if spec.Roster["builder"].BudgetUSD != 2.5 { + t.Fatalf("budget_usd = %v", spec.Roster["builder"].BudgetUSD) + } + mustReject(t, ` +name: bad +roster: + builder: {model: opus, budget_usd: -1, system_prompt: s, user_prompt: u} +phases: + - {name: build, kind: agent, owner: builder} +`, `"builder"`, "budget_usd must not be negative") +} diff --git a/internal/runtime/claudecode/adapter.go b/internal/runtime/claudecode/adapter.go index 81b5789..d7d1e36 100644 --- a/internal/runtime/claudecode/adapter.go +++ b/internal/runtime/claudecode/adapter.go @@ -231,6 +231,10 @@ func (a *Adapter) StartOrContinue( if opts.Effort != "" { arguments = append(arguments, "--effort", opts.Effort) } + if opts.BudgetUSD > 0 { + arguments = append(arguments, "--max-budget-usd", + strconv.FormatFloat(opts.BudgetUSD, 'f', -1, 64)) + } if opts.SystemPrompt != "" { arguments = append(arguments, "--system-prompt", opts.SystemPrompt) } diff --git a/internal/runtime/claudecode/adapter_test.go b/internal/runtime/claudecode/adapter_test.go index 5cc08ed..59a4a32 100644 --- a/internal/runtime/claudecode/adapter_test.go +++ b/internal/runtime/claudecode/adapter_test.go @@ -647,3 +647,31 @@ func TestVersionWarningFlagsOnlyReleasesOlderThanRecommended(t *testing.T) { } } } + +func TestBudgetIsPassedAsMaxBudgetUSDOnlyWhenSet(t *testing.T) { + adapter := NewWithExecutable(writeStub(t)) + for budget, want := range map[float64]string{2.5: "--max-budget-usd\n2.5\n", 0: ""} { + stubDir := t.TempDir() + handle, err := adapter.StartOrContinue(context.Background(), &runtime.Session{Key: "budget"}, + "prompt", runtime.Options{Model: "opus", BudgetUSD: budget, WorkDir: t.TempDir(), + Env: stubEnv(stubDir, "ok")}) + if err != nil { + t.Fatal(err) + } + drain(t, handle) + if _, err := handle.Result(); err != nil { + t.Fatal(err) + } + arguments, err := os.ReadFile(filepath.Join(stubDir, "args")) + if err != nil { + t.Fatal(err) + } + argText := string(arguments) + if want != "" && !strings.Contains(argText, want) { + t.Errorf("budget %v: args missing %q:\n%s", budget, want, argText) + } + if want == "" && strings.Contains(argText, "--max-budget-usd") { + t.Errorf("an unset budget must add no flag, got:\n%s", argText) + } + } +} diff --git a/internal/runtime/codex/adapter.go b/internal/runtime/codex/adapter.go index 3002500..7b2d0ac 100644 --- a/internal/runtime/codex/adapter.go +++ b/internal/runtime/codex/adapter.go @@ -120,6 +120,13 @@ var ErrSessionDiscontinuity = errors.New("codex resumed a different thread than var ErrToolAllowlistUnsupported = errors.New( "codex exec has no tool allowlist flag; a role tools: list cannot be enforced") +// ErrBudgetUnsupported reports a role that declared a spend cap against a +// runtime with no way to enforce one. `codex exec` has no per-run dollar +// budget flag, and a cap that is silently dropped is worse than none: the +// definition promised a bound the run does not have. It fails the send. +var ErrBudgetUnsupported = errors.New( + "codex exec has no spend-cap flag; a role budget_usd cannot be enforced") + // Adapter runs the Codex CLI. The zero value is not usable; New resolves the // executable once so agent subprocesses never depend on PATH from their own // (allowlisted) environment to find the CLI. @@ -212,6 +219,9 @@ func (a *Adapter) StartOrContinue( return nil, fmt.Errorf("%w (role asked for %s)", ErrToolAllowlistUnsupported, strings.Join(opts.Tools, ",")) } + if opts.BudgetUSD > 0 { + return nil, fmt.Errorf("%w (role asked for $%.2f)", ErrBudgetUnsupported, opts.BudgetUSD) + } arguments, resumedID := a.arguments(session, opts) // The system prompt has no flag on `codex exec`; the whole prompt arrives diff --git a/internal/runtime/codex/adapter_test.go b/internal/runtime/codex/adapter_test.go index 5500f61..aa91f46 100644 --- a/internal/runtime/codex/adapter_test.go +++ b/internal/runtime/codex/adapter_test.go @@ -829,3 +829,12 @@ func TestEffortBecomesARootReasoningOverrideOnBothShapes(t *testing.T) { t.Errorf("an unset effort must add no override, got %q", arguments) } } + +func TestADeclaredBudgetFailsTheSendRatherThanBeingDropped(t *testing.T) { + adapter := NewWithExecutable(writeStub(t)) + _, err := adapter.StartOrContinue(context.Background(), &runtime.Session{Key: "attempt-budget"}, "p", + runtime.Options{WorkDir: t.TempDir(), Env: stubEnv(t.TempDir(), "ok"), BudgetUSD: 3}) + if !errors.Is(err, ErrBudgetUnsupported) { + t.Fatalf("error = %v, want ErrBudgetUnsupported", err) + } +} diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 7ca1b4a..02a54ca 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -61,6 +61,7 @@ type Options struct { SystemPrompt string Model string Effort string + BudgetUSD float64 Tools []string WorkDir string Env []string diff --git a/internal/worker/publish.go b/internal/worker/publish.go index 472651c..e7ee8fc 100644 --- a/internal/worker/publish.go +++ b/internal/worker/publish.go @@ -96,6 +96,9 @@ func publishCode(err error) string { const ( PublishStatePublished = "published" PublishStateFailed = "failed" + // PublishStateHeld: the definition's publish.hold_when held, so nothing + // was pushed. Not a failure — the retry action is the release. + PublishStateHeld = "held" ) // PublishSummary is what publish writes into the attempt's result: the @@ -230,6 +233,15 @@ func (r *PublishingRunner) Run(ctx context.Context, prepared *PreparedAttempt) O // publish applies to it. return outcome } + if outcome.PublishHold != "" { + // Accepted, and deliberately not shipped: the definition asked for a + // person here. The worktree is retained like any unpublished accept + // (R16), so the publish-only retry can release it later. + outcome.State = protocol.AttemptAcceptedUnpublished + outcome.Result = withPublishSummary(outcome.Result, PublishSummary{ + State: PublishStateHeld, Code: "publish_held", Detail: outcome.PublishHold}) + return outcome + } worker := r.boundWorker() if worker == nil { return Outcome{State: protocol.AttemptAcceptedUnpublished, diff --git a/internal/worker/publish_test.go b/internal/worker/publish_test.go index adb7361..cc79507 100644 --- a/internal/worker/publish_test.go +++ b/internal/worker/publish_test.go @@ -873,3 +873,31 @@ func TestMilestone2ExitGate(t *testing.T) { t.Fatalf("job state = %q, want accepted", jobAfter.State) } } + +func TestAPublishHoldEndsAcceptedUnpublishedWithoutTouchingTheRemote(t *testing.T) { + gateway := newFakeGateway() + inner := RunnerFunc(func(context.Context, *PreparedAttempt) Outcome { + return Outcome{State: protocol.AttemptAcceptedUnpublished, + Result: engineResult(t, []string{"src/auth.txt"}), PublishHold: "risk == high held"} + }) + // Deliberately unbound: a hold must never reach the publish pipeline, so + // an unbound runner is the proof — bound or not, nothing is pushed. + runner := NewPublishingRunner(inner, gateway, PublishOptions{}) + outcome := runner.Run(context.Background(), &PreparedAttempt{}) + if outcome.State != protocol.AttemptAcceptedUnpublished || outcome.Error != "" { + t.Fatalf("outcome = %+v, want accepted_unpublished with no error", outcome) + } + var document struct { + Publish PublishSummary `json:"publish"` + } + if err := json.Unmarshal([]byte(outcome.Result), &document); err != nil { + t.Fatal(err) + } + if document.Publish.State != PublishStateHeld || document.Publish.Code != "publish_held" || + document.Publish.Detail != "risk == high held" { + t.Fatalf("publish summary = %+v, want held with the reason", document.Publish) + } + if created, adopted := gateway.counts(); created+adopted != 0 { + t.Fatal("a held publish must not create or adopt a pull request") + } +} diff --git a/internal/worker/registration.go b/internal/worker/registration.go index 4b596b2..f284f07 100644 --- a/internal/worker/registration.go +++ b/internal/worker/registration.go @@ -89,6 +89,11 @@ type Outcome struct { State string Result string Error string + // PublishHold, when set, is the reason accepted work must NOT be + // published by this runner: the definition's publish.hold_when held. The + // attempt ends accepted_unpublished with the hold recorded, and the + // publish-only retry is how a person releases it. + PublishHold string } // AttemptRunner executes one prepared attempt. U4's phase engine implements From 4caf2628f212b8b2297f50d2be31a8277b0b8111 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 08:17:44 +0000 Subject: [PATCH 3/4] fix: protect reported fields, pin the risk base, keep report lines whole Addresses three review findings on the risk gate: - An agent envelope could overwrite a field a reports_fields phase set, so a reviewer reporting `risk: low` escaped a publish hold keyed on the classifier's `risk: high`. Agent envelopes now merge around reported fields; the attempted overwrite is traced as reported_field_protected. - factory.yaml's classifier guessed its diff base from commit authors, which a builder-authored commit could defeat. Code phases and gates now receive JIG_BASE_SHA, the run's pinned commit, and the classifier diffs against it (failing loudly when it is unset). - The report was parsed from the 4 KiB output tail, so a longer report line was cut into invalid JSON. The final line is now captured whole (up to 32 KiB) alongside the tail; a longer one fails the phase rather than being truncated. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LwarQhEaL1ygzyMLvaXvZH --- README.md | 5 +- examples/definitions/factory.yaml | 11 +++-- internal/engine/code_phase.go | 71 +++++++++++++++++++++++----- internal/engine/phase.go | 41 ++++++++++++++-- internal/engine/phase_test.go | 78 ++++++++++++++++++++++++++++++- internal/protocol/limits.go | 4 ++ 6 files changed, 187 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 1d3b8c3..b59d062 100644 --- a/README.md +++ b/README.md @@ -203,7 +203,10 @@ Rules worth knowing before you write one: envelope view that `if:` guards and `publish.hold_when` read. It is how a deterministic script — a risk classifier scoring paths and diff size — gets a say in routing without a model in the loop. The adapter's own fields - (`status`, `passed`, `exit_code`, …) are reserved. + (`status`, `passed`, `exit_code`, …) are reserved, and a reported field is + protected: a later agent envelope cannot overwrite it. Code phases and + gates receive `JIG_BASE_SHA`, the commit the run was pinned to, so a script + can diff the whole change without guessing a base from history. - **Guards compare as well as test.** `if: revised` runs a phase when a previous envelope set the field truthy; `if: "risk == high"` runs it when the comparison holds, in the same ` ==|!= ` language as diff --git a/examples/definitions/factory.yaml b/examples/definitions/factory.yaml index 092abac..b39323b 100644 --- a/examples/definitions/factory.yaml +++ b/examples/definitions/factory.yaml @@ -316,16 +316,17 @@ phases: elif [ -f go.mod ]; then go test ./...; elif [ -f package.json ]; then npm test --silent; else echo "factory: no test command detected — edit the retest phase"; exit 1; fi - # Deterministic risk classification. The commits jig made are authored - # "jig", so the base is the newest commit that is not — the run's pinned - # HEAD — and the diff from there is the whole change. The last line printed - # is the report; everything before it is ordinary output. + # Deterministic risk classification. JIG_BASE_SHA is the commit the run was + # pinned to, so the diff from there is the whole change, however many + # commits it took. The last line printed is the report; everything before + # it is ordinary output. Fields a reporting phase sets are protected: a + # later agent envelope cannot overwrite `risk`. - name: classify-risk kind: code reports_fields: true command: | set -eu - base=$(git log --format='%H %an' HEAD | awk '$2 != "jig" { print $1; exit }') + base="${JIG_BASE_SHA:?JIG_BASE_SHA is required}" pattern='(^|/)(auth|authn|authz|login|session|secret|token|password|crypto|migrations?|schema|deploy|infra|terraform|helm|k8s)([./_-]|$)|(^|/)\.github/workflows/|(^|/)Dockerfile|(^|/)go\.mod$|(^|/)package(-lock)?\.json$' threshold=400 files=$(git diff --name-only "$base" HEAD) diff --git a/internal/engine/code_phase.go b/internal/engine/code_phase.go index 72a392f..efbb69b 100644 --- a/internal/engine/code_phase.go +++ b/internal/engine/code_phase.go @@ -7,10 +7,12 @@ package engine import ( + "bytes" "context" "encoding/json" "errors" "fmt" + "io" "os/exec" "strings" "syscall" @@ -27,6 +29,11 @@ type commandResult struct { TimedOut bool StartError string OutputTail string + // LastLine is the last non-empty output line, kept whole (up to + // MaxReportLineBytes) independently of the bounded tail, so a + // reports_fields report longer than the tail still parses. + LastLine string + LastLineOverflow bool } // Passed reports a clean exit. @@ -58,8 +65,10 @@ func runShellCommand( shell.Env = env shell.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} tail := &tailWriter{limit: protocol.MaxCommandOutputTailBytes} - shell.Stdout = tail - shell.Stderr = tail + lines := &lastLineWriter{limit: protocol.MaxReportLineBytes} + output := io.MultiWriter(tail, lines) + shell.Stdout = output + shell.Stderr = output if err := shell.Start(); err != nil { return commandResult{ExitCode: -1, StartError: err.Error()} } @@ -67,7 +76,8 @@ func runShellCommand( go func() { done <- shell.Wait() }() select { case err := <-done: - result := commandResult{OutputTail: tail.String()} + last, overflow := lines.Last() + result := commandResult{OutputTail: tail.String(), LastLine: last, LastLineOverflow: overflow} if exitError, ok := err.(*exec.ExitError); ok { result.ExitCode = exitError.ExitCode() } else if err != nil { @@ -85,15 +95,11 @@ func runShellCommand( // reportedFields reads a reports_fields code phase's report: its last // non-empty output line, which must be a JSON object. Everything before it // is ordinary output — a script may log freely and report once, at the end. -func reportedFields(outputTail string) (map[string]any, error) { - lines := strings.Split(strings.TrimRight(outputTail, "\n"), "\n") - last := "" - for i := len(lines) - 1; i >= 0; i-- { - if trimmed := strings.TrimSpace(lines[i]); trimmed != "" { - last = trimmed - break - } +func reportedFields(lastLine string, overflow bool) (map[string]any, error) { + if overflow { + return nil, fmt.Errorf("the last output line exceeds %d bytes", protocol.MaxReportLineBytes) } + last := strings.TrimSpace(lastLine) if last == "" { return nil, errors.New("the command printed nothing to report") } @@ -176,3 +182,46 @@ func (w *tailWriter) Write(value []byte) (int, error) { } func (w *tailWriter) String() string { return string(w.bytes) } + +// lastLineWriter keeps the last non-empty line written to it, whole, up to +// limit bytes. A line longer than limit is not truncated into something that +// might still parse: it is reported as overflowed. +type lastLineWriter struct { + limit int + current []byte + currentOverflow bool + last []byte + lastOverflow bool +} + +func (w *lastLineWriter) Write(value []byte) (int, error) { + for _, b := range value { + if b == '\n' { + w.finishLine() + continue + } + if len(w.current) < w.limit { + w.current = append(w.current, b) + } else { + w.currentOverflow = true + } + } + return len(value), nil +} + +func (w *lastLineWriter) finishLine() { + if w.currentOverflow || len(bytes.TrimSpace(w.current)) > 0 { + w.last = append(w.last[:0], w.current...) + w.lastOverflow = w.currentOverflow + } + w.current = w.current[:0] + w.currentOverflow = false +} + +// Last returns the last non-empty line, counting an unterminated final line. +func (w *lastLineWriter) Last() (string, bool) { + if w.currentOverflow || len(bytes.TrimSpace(w.current)) > 0 { + w.finishLine() + } + return string(w.last), w.lastOverflow +} diff --git a/internal/engine/phase.go b/internal/engine/phase.go index 2f8fd2f..efacba7 100644 --- a/internal/engine/phase.go +++ b/internal/engine/phase.go @@ -26,6 +26,7 @@ import ( "log/slog" "os" "path/filepath" + "reflect" "sort" "strings" "time" @@ -200,6 +201,7 @@ func (r *Runner) Execute(ctx context.Context, attempt Attempt) worker.Outcome { sessions: make(map[string]*runtime.Session), transcripts: make(map[string][]exchange), fieldView: make(map[string]any), + reportedBy: make(map[string]string), gateReports: make(map[string]protocol.GateReport), seededRoles: make(map[string]bool), phaseEntries: make(map[string]int), @@ -230,7 +232,13 @@ type execution struct { fieldView map[string]any // publishHeld is the hold reason once publishHold() fired, so the summary // says "held" where it would otherwise say "not_attempted". - publishHeld string + publishHeld string + // reportedBy names, per field, the reports_fields code phase that last + // set it. Those values are deterministic facts about the change (a risk + // score), and an agent envelope may not overwrite them: a reviewer that + // reports `risk: low` must not be able to talk its way past a publish + // hold keyed on the classifier's `risk: high`. + reportedBy map[string]string results []protocol.PhaseResult gateReports map[string]protocol.GateReport seededRoles map[string]bool @@ -750,7 +758,11 @@ func (e *execution) phaseEnv(phase protocol.PhaseSpec) []string { if phase.Owner != "" { allow = append(allow, e.spec.Roster[phase.Owner].Env...) } - return subprocessEnv(e.runner.config.BaseEnv, allow, e.scratch.home) + env := subprocessEnv(e.runner.config.BaseEnv, allow, e.scratch.home) + // The run's pinned base, so a command can diff the whole change against + // the one commit the attempt started from rather than guess it from + // history the agents wrote. + return append(env, "JIG_BASE_SHA="+e.attempt.BaseSHA) } func (e *execution) mergeFields(fields map[string]any) { @@ -759,6 +771,24 @@ func (e *execution) mergeFields(fields map[string]any) { } } +// mergeAgentFields merges an agent envelope into the field view, except for +// fields a reports_fields code phase already set: those keep the reported +// value, and the attempted overwrite is traced. +func (e *execution) mergeAgentFields(phase, role string, fields map[string]any) { + for key, value := range fields { + if reporter, reported := e.reportedBy[key]; reported { + if !reflect.DeepEqual(e.fieldView[key], value) { + e.emit.emit(protocol.EventLog, phase, "reported_field_protected", map[string]any{ + "role": role, "field": key, "reported_by": reporter, + "kept": e.fieldView[key], "ignored": value, + }) + } + continue + } + e.fieldView[key] = value + } +} + // ---- code phases ----------------------------------------------------------- func (e *execution) runCodePhase(ctx context.Context, phase protocol.PhaseSpec) phaseRun { @@ -786,7 +816,7 @@ func (e *execution) runCodePhase(ctx context.Context, phase protocol.PhaseSpec) } var reported map[string]any if phase.ReportsFields && result.Passed() { - fields, err := reportedFields(result.OutputTail) + fields, err := reportedFields(result.LastLine, result.LastLineOverflow) if err != nil { // A reporting phase that did not report is a failed phase, not a // silent success with nothing for the guards downstream to read. @@ -811,6 +841,9 @@ func (e *execution) runCodePhase(ctx context.Context, phase protocol.PhaseSpec) status = protocol.EnvelopeSuccess } e.mergeFields(envelope.Fields) + for key := range reported { + e.reportedBy[key] = phase.Name + } e.recordResult(protocol.PhaseResult{ Phase: phase.Name, Kind: phase.Kind, Status: status, PhaseAttempt: entry, Envelope: envelope.Raw, Gates: report, StartedAt: &started, @@ -1078,7 +1111,7 @@ func (e *execution) runAgentPhaseAttempt( map[string]any{"role": phase.Owner, "paths": touched}) } - e.mergeFields(envelope.Fields) + e.mergeAgentFields(phase.Name, phase.Owner, envelope.Fields) e.emit.emit(protocol.EventHandoff, phase.Name, phase.Owner, map[string]any{ "artifacts": envelope.Base.Artifacts, "summary": envelope.Base.Summary, }) diff --git a/internal/engine/phase_test.go b/internal/engine/phase_test.go index 184d25b..94d6251 100644 --- a/internal/engine/phase_test.go +++ b/internal/engine/phase_test.go @@ -1035,12 +1035,14 @@ acceptance: [all_phases_passed] values[name] = value } } - // `sh` itself contributes these three; everything else must have been - // composed by the engine. + // `sh` itself contributes PWD, SHLVL and _; everything else must have + // been composed by the engine — the allowlist, the HOME/XDG family, and + // JIG_BASE_SHA, the run's pinned commit. allowed := map[string]bool{ "PATH": true, "FOO": true, "HOME": true, "XDG_CONFIG_HOME": true, "XDG_CACHE_HOME": true, "XDG_DATA_HOME": true, "XDG_STATE_HOME": true, + "JIG_BASE_SHA": true, "PWD": true, "SHLVL": true, "_": true, } for name := range values { @@ -1611,3 +1613,75 @@ func TestAReportedFieldMayNotOverwriteTheAdaptersOwn(t *testing.T) { t.Fatalf("outcome = %+v, want a failed attempt naming the reserved field", outcome) } } + +func TestAnAgentCannotOverwriteAReportedFieldToEscapeTheHold(t *testing.T) { + repo := initRepo(t) + fake := enginetest.New( + enginetest.Step{Files: map[string]string{"src/auth.txt": "token check"}, + Text: envelope(map[string]any{"status": "success", "summary": "built auth"})}, + // The high-risk reviewer tries to downgrade the classifier's verdict. + enginetest.Step{Text: envelope(map[string]any{"status": "success", "approved": true, + "risk": "low", "summary": "nothing to see"})}, + ) + sink := &recordingSink{} + outcome := newTestRunner(t, fake, sink, nil). + Execute(context.Background(), testAttempt(factorySnapshot, nil, repo)) + if outcome.PublishHold == "" { + t.Fatalf("outcome = %+v: an agent's risk=low escaped the hold", outcome) + } + if !sink.has(protocol.EventLog, "reported_field_protected") { + t.Fatal("the attempted overwrite was not traced") + } +} + +func TestCodePhasesReceiveThePinnedBaseSHA(t *testing.T) { + repo := initRepo(t) + attempt := testAttempt(` +name: base +roster: + writer: {model: test-model, system_prompt: s, user_prompt: u} +phases: + - {name: check, kind: code, command: 'test "$JIG_BASE_SHA" = "pinned-sha"'} +acceptance: [all_phases_passed] +`, nil, repo) + attempt.BaseSHA = "pinned-sha" + outcome := newTestRunner(t, enginetest.New(), &recordingSink{}, nil). + Execute(context.Background(), attempt) + if outcome.State != protocol.AttemptAcceptedUnpublished { + t.Fatalf("outcome = %+v, want the command to see JIG_BASE_SHA", outcome) + } +} + +func TestAReportLongerThanTheOutputTailStillParses(t *testing.T) { + repo := initRepo(t) + long := strings.Repeat("x", protocol.MaxCommandOutputTailBytes*2) + outcome := newTestRunner(t, enginetest.New(), &recordingSink{}, nil). + Execute(context.Background(), testAttempt(` +name: long +roster: + writer: {model: test-model, system_prompt: s, user_prompt: u} +phases: + - {name: report, kind: code, reports_fields: true, command: "printf '{\"risk\":\"low\",\"note\":\"`+long+`\"}'"} + - {name: gated, kind: code, command: "false", if: "risk == high"} +acceptance: [all_phases_passed] +`, nil, repo)) + if outcome.State != protocol.AttemptAcceptedUnpublished { + t.Fatalf("outcome = %+v, want the long, unterminated report to parse", outcome) + } +} + +func TestAReportLongerThanTheReportCapFailsRatherThanTruncating(t *testing.T) { + repo := initRepo(t) + outcome := newTestRunner(t, enginetest.New(), &recordingSink{}, nil). + Execute(context.Background(), testAttempt(fmt.Sprintf(` +name: huge +roster: + writer: {model: test-model, system_prompt: s, user_prompt: u} +phases: + - {name: report, kind: code, reports_fields: true, command: "head -c %d /dev/zero | tr '\\0' x; echo"} +acceptance: [all_phases_passed] +`, protocol.MaxReportLineBytes+10), nil, repo)) + if outcome.State != protocol.AttemptFailed || !strings.Contains(outcome.Error, "exceeds") { + t.Fatalf("outcome = %+v, want a failed phase naming the overflow", outcome) + } +} diff --git a/internal/protocol/limits.go b/internal/protocol/limits.go index 6be62ac..d44be43 100644 --- a/internal/protocol/limits.go +++ b/internal/protocol/limits.go @@ -130,6 +130,10 @@ const ( // output lives in the attempt trace; the envelope keeps the tail an agent // can act on. MaxCommandOutputTailBytes = 4 << 10 + // MaxReportLineBytes caps a reports_fields phase's final report line, + // kept whole alongside the tail. A longer report fails the phase rather + // than being cut into JSON that does not parse. + MaxReportLineBytes = 32 << 10 // MaxEventPayloadBytes caps one trace event's payload everywhere the // engine emits it (KTD8). Oversized payloads are truncated with a marker, From 8f1ab82a47640c8695274815c598cc5bb49268e4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 08:20:16 +0000 Subject: [PATCH 4/4] fix: gofmt the containment test; make the factory risk gate fail closed - phase_test.go was not gofmt-clean after naming JIG_BASE_SHA in the env-containment test, failing CI's format-check. - factory.yaml's guard and publish hold said `risk == high`, which does not hold when `risk` was never reported, so unclassified work would publish. Both now say `risk != low`: anything the classifier did not explicitly score low is reviewed and held. A new engine test proves a `!=` hold fires when the field is absent. - README states the sign-off model: with one trusted operator and no control-plane authentication, the operator's publish retry is the approval, and predicates should be written to fail closed. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LwarQhEaL1ygzyMLvaXvZH --- README.md | 7 ++++++- examples/definitions/factory.yaml | 12 +++++++----- internal/engine/phase_test.go | 24 ++++++++++++++++++++++-- 3 files changed, 35 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index b59d062..b4ad9ea 100644 --- a/README.md +++ b/README.md @@ -215,7 +215,12 @@ Rules worth knowing before you write one: after acceptance passes. When it holds, the attempt ends `accepted_unpublished` with the hold recorded, nothing is pushed, and the worktree and branch are retained — the publish-only retry is the human - sign-off that releases it. Low-risk work never waits. + sign-off that releases it. Low-risk work never waits. Write the predicate + to fail closed — `risk != low` holds work whose risk was never reported, + where `risk == high` would ship it. The sign-off is only as strong as + access to the control plane: jig has one trusted operator and no + authentication (see *Loopback only*), so the operator's retry is the + approval, with no separate approver identity. - **Validation happens before anything runs.** `jig def validate ` is the same check the store applies at save time, offline. diff --git a/examples/definitions/factory.yaml b/examples/definitions/factory.yaml index b39323b..d802d36 100644 --- a/examples/definitions/factory.yaml +++ b/examples/definitions/factory.yaml @@ -20,11 +20,13 @@ # its last output line is a JSON object, and jig merges those fields into the # envelope view the guards and the publish hold read. The script is # deterministic — paths touched and lines changed, no model — so the same -# diff always scores the same. `if: "risk == high"` runs one more review only -# for high-risk work, and `publish.hold_when` parks it: the attempt ends +# diff always scores the same. Anything but `risk: low` runs one more review +# and trips `publish.hold_when`, which parks the work: the attempt ends # accepted_unpublished with the hold on record, the worktree and branch are # retained, and the operator's publish retry is the human sign-off. Low-risk -# work publishes on its own. +# work publishes on its own. Both predicates say `risk != low` rather than +# `risk == high` so they fail closed: work the classifier never scored — a +# missing or unexpected `risk` — is held, not shipped. # # THE ROSTER. Builders and reviewers are `opus`; the planner and the extra # high-risk reviewer are `sonnet`. `effort` is medium for building to a plan @@ -346,7 +348,7 @@ phases: - name: review-risk kind: agent owner: risk-reviewer - if: "risk == high" + if: "risk != low" acceptance: [all_phases_passed, verdict_consistent, diff_matches_claims] publish: - hold_when: "risk == high" + hold_when: "risk != low" diff --git a/internal/engine/phase_test.go b/internal/engine/phase_test.go index 94d6251..3832d6f 100644 --- a/internal/engine/phase_test.go +++ b/internal/engine/phase_test.go @@ -1042,8 +1042,7 @@ acceptance: [all_phases_passed] "PATH": true, "FOO": true, "HOME": true, "XDG_CONFIG_HOME": true, "XDG_CACHE_HOME": true, "XDG_DATA_HOME": true, "XDG_STATE_HOME": true, - "JIG_BASE_SHA": true, - "PWD": true, "SHLVL": true, "_": true, + "JIG_BASE_SHA": true, "PWD": true, "SHLVL": true, "_": true, } for name := range values { if !allowed[name] { @@ -1685,3 +1684,24 @@ acceptance: [all_phases_passed] t.Fatalf("outcome = %+v, want a failed phase naming the overflow", outcome) } } + +func TestANotEqualHoldFailsClosedWhenTheFieldWasNeverReported(t *testing.T) { + repo := initRepo(t) + fake := enginetest.New( + enginetest.Step{Files: map[string]string{"src/app.txt": "v1"}, + Text: envelope(map[string]any{"status": "success", "summary": "built"})}, + ) + outcome := newTestRunner(t, fake, &recordingSink{}, nil).Execute(context.Background(), testAttempt(` +name: fail-closed +roster: + builder: {model: test-model, system_prompt: Build., user_prompt: Build., writes: ["src/"]} +phases: + - {name: build, kind: agent, owner: builder} +acceptance: [all_phases_passed] +publish: + hold_when: "risk != low" +`, nil, repo)) + if outcome.State != protocol.AttemptAcceptedUnpublished || outcome.PublishHold == "" { + t.Fatalf("outcome = %+v, want work with no risk report held", outcome) + } +}