openspec-implementation-loop

Orchestrate an implementation loop around a single OpenSpec change. This skill is **hand-maintained** (not emitted by `make gen-openspec-skills`). Keep its directory name in `scripts/gen-openspec-skills.mjs` so regeneration does not delete it. **Store selection:** Only if the **user names** a store, run `openspec store list --json`, pick that id, and pass `--store <id>` on CLI commands that take it (`status`, `instructions`, `list`, `show`, `validate`, …). Treat that id as sticky for the rest of this run. Do **not** auto-discover a store because one is registered on the machine. Default for this repo: unscoped CLI and `make check-openspec` against the nearest local `openspec/`. **Input**: Optionally specify a change name. If omitted, you MUST ask the user which change to implement. **High-level flow** 1. Select the change 2. **Ask delivery mode (commit vs PR) - do this immediately after selecting the change, not after implementation** 3. Load OpenSpec context 4. **Triage: determine execution strategy** - classify as inline, single-implementor, or per-task based on change size and complexity 5. Determine remaining top-level tasks 6. Implement tasks using the chosen strategy 7. Run validation and review at the cadence determined by the strategy 8. Aggregate findings and fix; repeat until clean 9. Push the branch to `origin` 10. **Commit mode**: Watch GitHub Actions that run on a branch push (typically `Go`). Do not wait for `OpenSpec CI` — that workflow does not run on arbitrary branches. Surface Buildkite acceptance as out-of-band; never run or auto-fix it. **PR mode**: Create a PR, then monitor GitHub Actions (including `OpenSpec CI` when present), reviews, and comments. Delegate to `pr-monitoring-loop` when that skill is installed; that skill keeps a read-only watch until Buildkite acceptance finishes and never auto-fixes it. Otherwise watch with `gh` as described in **PR mode** (body step 11). 11. Report final outcome **Steps** 1. **Select exactly one change** If a name is provided, use it. Otherwise: - Run `openspec list --json` - Filter to **active** changes (not archived). Exclude entries already under `openspec/changes/archive/` or marked archived. Drop changes with no remaining implementation work (archive/sync/execute-acc-only). - Use **AskUserQuestion** (or an equivalent explicit user prompt) to let the user choose a single active change - Show the change name, schema if available, and status **IMPORTANT**: - Do NOT guess - Do NOT auto-select - Do NOT implement multiple changes in one run Always announce: `Using change: <name>`. 2. **Choose delivery mode (ask immediately - before loading context or starting the implementor)** Use **AskUserQuestion** (or an equivalent explicit user prompt) right after step 1. Do **not** defer this until after implementation or push. Offer two options: - **Commit-only**: Push your work to `origin` on the current branch. After each push, monitor GitHub Actions for the **branch / commits** you pushed. - **Pull request**: After the **initial** push of the implementation loop, **create a PR** (for example with `gh pr create`). Then monitor **PR** checks and reviews as described in body step 11 (PR mode). Record the user's choice and refer to it from push onward (steps 9–12). **Baseline (before any commit or push):** - If `git branch --show-current` is empty (detached HEAD), or the current branch is `master` or `main`, stop and ask the user for a feature branch. Do not commit or push to the default branch or a detached checkout. - If the working tree has unrelated dirty or untracked files (paths outside this change), stop and ask rather than mixing them into this change's commits or push. - Record a **fork point** for the pre-push scope check: `git merge-base HEAD origin/master` or `upstream/master` (or `main`). Do **not** use the current `HEAD` as the baseline — that misses pre-existing local commits. If `git rev-parse --abbrev-ref @{u}` succeeds, step 9 uses `@{u}...HEAD` instead. If neither a tracking ref nor a merge-base with the default branch exists, stop and ask. 3. **Load OpenSpec status and context** Run: ```bash openspec status --change "<name>" --json openspec instructions apply --change "<name>" --json ``` Parse the outputs to determine: - `schemaName` - current task progress - `state` - `contextFiles` - optional prompt-level `context` and `operationGuidance` (same contract as `openspec-apply-change`) - the ordered list of top-level tasks (for example `1`, `2`, `3`) and which of them are still incomplete Read every file listed in `contextFiles`. Treat `context` as required prompt-level input: apply relevant project facts, conventions, and constraints (changelog, Plugin Framework vs SDKv2, no `TF_ACC`) before triage and implementation. Treat `operationGuidance` as optional additive advice; follow entries that apply. `make lint` / `make build` / `make unit` from guidance are invalid unless they include `env -u TF_ACC` — never run those targets without it. If `context` conflicts with this skill, an explicit user choice, or a CLI-controlled value, report the conflict and preserve the controlling value. `operationGuidance` that says never run `make testacc` binds this entire skill, including the orchestrator. Nobody sets `TF_ACC`. In PR mode the orchestrator may **ask whether the human ran** named `TestAcc…` cases (7b.1); it must not run them. **Handle states**: - If `state: "blocked"`: stop and explain what artifact is missing; suggest continuing the change artifacts first - If `state: "all_done"`: skip triage and implementation (steps 4–6). Continue at step 7: run 7a and the **7d battery once** (that battery already runs 7b via the validation runner, then reviews, then 7b.1 once if 7b and reviews are green). Do **not** also run 7b/7b.1 separately. Do not use 7c or 7e. Then continue at step 8. - Otherwise: proceed to triage and implementation 4. **Triage: determine execution strategy** Evaluate the change to choose an execution strategy that balances implementation quality with subagent overhead. The goal is to avoid spawning unnecessary subagents for small changes while preserving full rigor for large ones. **Signals to evaluate:** | Signal | Source | |--------|--------| | Top-level task count | Count `## N.` headings in tasks | | Total subtask count | Count `- [ ]` / `- [x]` items | | File scope | Infer from task descriptions - single package/area vs. cross-cutting | | Inter-task coupling | Do later tasks build directly on earlier ones? | | Complexity | Do tasks involve non-trivial logic (custom types, plan modifiers, complex CRUD) or are they straightforward (config, docs, Makefile, CI, specs)? | **Choose one of three strategies:** | Strategy | When to use | Implementation | Review | |----------|-------------|----------------|--------| | **Inline** | ≤2 top-level tasks AND ≤~10 subtasks AND single area AND straightforward changes | Orchestrator implements directly, no implementor subagent | Run validation and `openspec-verify-change` directly; spawn other review subagents only if the change touches non-trivial logic | | **Single-implementor** | ≤4 top-level tasks **AND** ≤~15 subtasks, with coherent scope | One implementor subagent handles all remaining tasks | One round of parallel review after all tasks complete | | **Per-task** | >4 top-level tasks, OR >15 subtasks, OR multi-area scope, OR tasks are largely independent across different packages | Fresh implementor per top-level task | Task-scoped review after each task; `openspec-verify-change` only after the last | These thresholds are guidelines, not rigid rules. Evaluate in this order (user can still override): 1. If a **per-task** condition matches (>4 top-level tasks, >~15 subtasks, multi-area scope, or largely independent packages), use **per-task**. 2. Else if **inline** matches, use **inline**. Inline is a subset of single-implementor; do **not** pick single-implementor or per-task only because both size rows match. 3. Else if **single-implementor** matches, use **single-implementor**. 4. Else use **per-task**. Judgment still applies: - A 3-task change where each task is in a different package might warrant **per-task** - A 5-task change that is all in one file might be fine as **single-implementor** - A 2-task change with complex custom type logic might benefit from **single-implementor** over **inline** for the review coverage Announce the chosen strategy and reasoning. The user can override. 5. **Determine the remaining top-level tasks** Build an ordered queue of incomplete top-level tasks from the OpenSpec task list. Interpret a top-level task as the parent task number such as `1`, `2`, or `3`. Each top-level task includes all of its nested subtasks such as `1.1`, `1.2`, `1.3`. Omit a top-level task from this queue (and from step 9 completion) when its remaining work is only archive, sync-to-canonical-specs (`openspec-archive-change`, `openspec-sync-specs`, merge delta specs into `openspec/specs/`), execute-acc (`make testacc`, `TF_ACC=1`, Human/Buildkite runs a named `TestAcc…`), and/or the remaining **run** portion of a combined write/run `TestAcc…` task whose file/`func TestAcc…` already exists. Those run after this loop (or out-of-band). If the implementation queue is empty, skip step 6. At step 7 treat it like `all_done`: 7a and the **7d battery once** (that battery already runs 7b, reviews, and 7b.1 once). Do **not** use 7c or 7e. Do **not** run 7b.1 separately. For each remaining incomplete top-level task: - gather the subtasks that belong to it - understand the intended scope from the proposal/design/specs - process the top-level tasks sequentially unless the user explicitly asks for a different strategy 6. **Implement tasks using the chosen strategy** **Inline strategy:** The orchestrator implements all tasks directly without spawning an implementor subagent: - Do not open `openspec-apply-change` as a workflow. Tick `- [ ]` → `- [x]` yourself after each in-scope subtask. Do not follow apply's archive, auto-select, or run-all-tasks rules. Do not invoke `openspec-archive-change` or `openspec-sync-specs`. - do not run `openspec-sync-specs` or write delta requirements into canonical `openspec/specs/`; that is the later Land specs phase after verify. Never archive the change - ignore archive tasks, sync-to-canonical-specs tasks (`openspec-archive-change`, `openspec-sync-specs`, merge delta specs into `openspec/specs/`), execute-acc-only subtasks (Human/Buildkite runs a named `TestAcc…`, `make testacc`, or `TF_ACC=1` with no write/add), and the remaining **run** portion of a combined write/run `TestAcc…` task once the file/`func TestAcc…` already exists. Leave those checkboxes. 7b.1 is the only acc path. This loop never archives or syncs. After this skill returns, the user may run those skills (or apply the later `verify-openspec` label when that workflow exists) - refuse to commit if the branch is `master`, `main`, or detached (`git branch --show-current` empty); stop and ask for a feature branch - keep changes minimal and focused - create small, focused git commits as coherent pieces of work are completed - run targeted validation while implementing (`env -u TF_ACC make unit` scoped to touched packages is fine; `env -u TF_ACC make docs-generate` when schema/examples changed; never set `TF_ACC`) - do not push; the orchestrator pushes in step 9 **Single-implementor strategy:** Launch one write-capable subagent for all remaining tasks: - instruct it to implement all remaining top-level tasks in sequence, completing all nested subtasks within each before moving to the next - Do not open `openspec-apply-change` as a workflow. Tick `- [ ]` → `- [x]` yourself after each in-scope subtask. Do not follow apply's archive, auto-select, or run-all-tasks rules. Do not invoke `openspec-archive-change` or `openspec-sync-specs`. - do not run `openspec-sync-specs` or write delta requirements into canonical `openspec/specs/`; that is the later Land specs phase after verify. Never archive the change - ignore archive tasks, sync-to-canonical-specs tasks (`openspec-archive-change`, `openspec-sync-specs`, merge delta specs into `openspec/specs/`), execute-acc-only subtasks (Human/Buildkite runs a named `TestAcc…`, `make testacc`, or `TF_ACC=1` with no write/add), and the remaining **run** portion of a combined write/run `TestAcc…` task once the file/`func TestAcc…` already exists. Leave those checkboxes. 7b.1 is the only acc path. This loop never archives or syncs. After this skill returns, the user may run those skills (or apply the later `verify-openspec` label when that workflow exists) - refuse to commit if the branch is `master`, `main`, or detached (`git branch --show-current` empty); stop and ask for a feature branch - read the OpenSpec context files before editing - keep changes minimal and focused - create small, focused git commits as coherent pieces of work are completed - run targeted validation while implementing (`env -u TF_ACC make unit` scoped to touched packages is fine; `env -u TF_ACC make docs-generate` when schema/examples changed; never set `TF_ACC`) - do not push; the orchestrator pushes in step 9 Ask the implementor to report back with: - top-level tasks completed - nested subtasks completed - commits created - tests run - blockers or open questions **Per-task strategy:** Launch a dedicated fresh write-capable subagent for the current top-level task only. Do **not** reuse the prior task's implementor for later top-level tasks. The implementor prompt should instruct it to: - implement only the selected change - focus only on the current top-level task and complete all of its nested subtasks before stopping - Do not open `openspec-apply-change` as a workflow. Tick `- [ ]` → `- [x]` yourself after each in-scope subtask. Do not follow apply's archive, auto-select, or run-all-tasks rules. Do not invoke `openspec-archive-change` or `openspec-sync-specs`. - do not run `openspec-sync-specs` or write delta requirements into canonical `openspec/specs/`; that is the later Land specs phase after verify. Never archive the change - ignore archive tasks, sync-to-canonical-specs tasks (`openspec-archive-change`, `openspec-sync-specs`, merge delta specs into `openspec/specs/`), execute-acc-only subtasks (Human/Buildkite runs a named `TestAcc…`, `make testacc`, or `TF_ACC=1` with no write/add), and the remaining **run** portion of a combined write/run `TestAcc…` task once the file/`func TestAcc…` already exists. Leave those checkboxes. 7b.1 is the only acc path. This loop never archives or syncs. After this skill returns, the user may run those skills (or apply the later `verify-openspec` label when that workflow exists) - refuse to commit if the branch is `master`, `main`, or detached (`git branch --show-current` empty); stop and ask for a feature branch - read the OpenSpec context files before editing - keep changes minimal and focused - create small, focused git commits as coherent pieces of work are completed - run targeted validation while implementing (`env -u TF_ACC make unit` scoped to touched packages is fine; `env -u TF_ACC make docs-generate` when schema/examples changed; never set `TF_ACC`) - do not push; the orchestrator pushes in step 9 Ask the implementor to report back with: - the top-level task completed - nested subtasks completed - commits created - tests run - blockers or open questions **For all strategies**: if the implementor (or the orchestrator in inline mode) is blocked, stop and surface the blocker to the user. 7. **Run validation and review** The validation and review cadence depends on the execution strategy. If this invocation skipped triage because `state` was `all_done` **or the implementation queue is empty**, run 7a and the 7d battery once (do not also run 7b/7b.1 separately; do not use 7c or 7e), then go to step 8. **7a. Determine the review type** Inspect the change artifacts and implementation to decide whether this is a Terraform entity change. Treat it as a Terraform entity change when the change is centered on a Terraform resource or data source, for example: - specs reference `Resource implementation:` or `Data source implementation:` - changed code is primarily in a resource/data source package under `ec/ecresource/` or `ec/ecdatasource/` **7b. Validation requirements (all strategies)** Required baseline commands — **always** run each with `TF_ACC` unset. `make unit` is `go test ./...`; if the session inherited `TF_ACC=1`, `ec/acc` will run against the live API. Generate docs **before** lint when schemas/templates/examples changed so new entity docs exist for `tfproviderdocs` and the post-battery `git status` can catch a dirty `docs/` tree. `tfproviderdocs` (part of `make lint`) is a structural check; it does **not** fail merely because existing generated markdown is stale. Go CI runs `make lint` then `make docs-generate` then `git diff --exit-code docs/`. - `env -u TF_ACC make docs-generate` when resource/data-source schemas, templates, or examples changed. If those inputs did not change, skip it and say so. - `env -u TF_ACC make vendor` when `go.mod`/`go.sum` changed (before lint; Go CI runs `make vendor` then fails on a dirty `NOTICE`). If modules did not change, skip it and say so. - `env -u TF_ACC make lint` - `env -u TF_ACC make build` - `env -u TF_ACC make unit TEST=./... TESTARGS= TESTUNITARGS='-timeout 10m -race -cover -coverprofile=reports/c.out'` (pin `TEST`/`TESTARGS`/`TESTUNITARGS`; they are `?=` so inherited env can shrink the suite). Implementor-local scoped `TEST=./ec/…` is fine outside this battery. - `env -u TF_ACC make check-openspec` when the work touched **this repo's** `openspec/` (it is **not** part of `make lint`). If `openspec/` did not change, skip `make check-openspec` and say so. If the **user named** a `--store` id, that is **additive**: also run `env -u TF_ACC make setup-openspec`, then `OPENSPEC_TELEMETRY=0 ./node_modules/.bin/openspec validate --all --store <id>`. Do **not** skip this repo's `make check-openspec` merely because a store was named (`make check-openspec` is what OpenSpec CI runs on the PR). - `env -u TF_ACC make notice`. If `NOTICE` is dirty, commit it (same as Go CI `git diff --exit-code NOTICE`).