diff --git a/CONTEXT.md b/CONTEXT.md index e640a6690..75ecdd9ff 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -90,7 +90,7 @@ A **Run** asked to end with its pull request merged rather than left for review, _Avoid_: auto-merge (GitHub's own feature, which thirdshift does not use) **Run notification**: -A message thirdshift sends when a **Run** or a **Spec run** ends, whatever its outcome (ready, merged, failed or interrupted), to the address given with the email flag or the default in the **User config**. A Run or Spec run sends one only when asked to, by the flag or by the User config; a Spec run's notification lists each **Ticket**'s outcome, and a Ticket's **Run** never sends one of its own. Failing to send one never changes the outcome. +A message thirdshift sends when a **Run**, a **Spec run** or an **Architect run** ends, whatever its outcome (ready, merged, failed or interrupted; for an Architect run that dispatched nothing, plan published, idea filed, idea already filed or review failed), to the address given with the email flag or the default in the **User config**. Each sends one only when asked to, by the flag or by the User config; a Spec run's notification lists each **Ticket**'s outcome, and a Ticket's **Run** never sends one of its own. An Architect run's notification tells how its **Architecture review** ended, naming the plan or idea issue, and how the Spec run or Run it dispatched ended, which sends none of its own. Failing to send one never changes the outcome. _Avoid_: completion email, alert **User config**: diff --git a/README.md b/README.md index 72734d805..29b60df7d 100644 --- a/README.md +++ b/README.md @@ -124,11 +124,13 @@ It sends one [Run notification](#run-notifications), whatever the outcome. To ha - **stdout** carries only the pull request's URL: on success, and on a Failed run that leaves an open pull request, a draft or, after a policy refusal, one ready for review. The exit code tells the two apart, so script it as `url=$(thirdshift "$issue") && echo "ready: $url"`. - **stderr** carries everything else: errors, cleanup problems, and progress lines while sessions run. Each line starts with the local time it was printed, as in `thirdshift: 12:14:49 pushing issue-7`, so a quiet terminal shows how long the Run has been on its last step. A successful Run's last line names the pull request too: `PR is ready for review`, or `PR is merged` after a Merge run, followed only by a `warning:` line if a [Run notification](#run-notifications) can't be sent. -- **Exit code** `0` means the Run ended with a pull request the factory stands behind, merged in a Merge run. Once the Self-merge has merged, the Run succeeds even if deleting the Issue branch on `origin` or closing the issue then fails: the merge can't be undone, so each failed step is a `warning:` line on stderr naming the command to run by hand, and the Run still exits `0` with the URL on stdout. Ctrl-C likewise: before the merge it makes a Failed run, after it thirdshift finishes these steps and exits as merged. `2` means the Issue URL is missing or isn't a GitHub Issue URL, there is an argument other than the URL and the Run flags, a Run flag is repeated or contradicts another, or `parallel` isn't followed by a whole number from 1 up; the error and the help text go to stderr, before any work. A [User config](#user-config) thirdshift can't use exits `1`, also before any work. Any other failure exits `1`. +- **Exit code** `0` means the Run ended with a pull request the factory stands behind, merged in a Merge run. Once the Self-merge has merged, the Run succeeds even if deleting the Issue branch on `origin` or closing the issue then fails: the merge can't be undone, so each failed step is a `warning:` line on stderr naming the command to run by hand, and the Run still exits `0` with the URL on stdout. Ctrl-C likewise: before the merge it makes a Failed run, after it thirdshift finishes these steps and exits as merged. `2` means the command is one thirdshift can't use: the Issue URL is missing or isn't a GitHub Issue URL, there is an argument other than the URL and the Run flags, a Run flag is repeated or contradicts another, or `parallel` isn't followed by a whole number from 1 up; the error and the help text go to stderr, before any work. A [User config](#user-config) thirdshift can't use exits `1`, also before any work. Any other failure exits `1`. The other commands: ```sh +thirdshift architect [] # review the Base branch's architecture, publish a plan and run it (see Architect runs) +thirdshift architect [] --plan-only # publish the plan, mark it ready and stop there thirdshift email-test [
] # send a test email through Resend (see Email) thirdshift setup # choose your defaults and write the User config with every setting (see User config) thirdshift update # update to the latest release (see Updating) @@ -136,7 +138,7 @@ thirdshift version # print thirdshift thirdshift help # print every form of the command, each with a one-line description ``` -`version` and `help` print to stdout and exit `0`. `update`, `setup` and `email-test` follow the Run's rule: stdout stays empty, messages go to stderr. +`version` and `help` print to stdout and exit `0`. `update`, `setup` and `email-test` follow the Run's rule: stdout stays empty, messages go to stderr. `architect` follows it too: stdout carries the pull request's URL of the run it dispatches, or, in its place, the URL of the issue it ended on: its plan with `--plan-only`, or its idea when the review published no plan. Uncommitted changes in your clone are fine: the Run works in its own worktree from `origin`, so they are simply left out. Unpushed commits on the Base branch are not: push them first, or the Run stops. @@ -249,7 +251,7 @@ no Resend API key. Either: (a crontab line, CI secret, or a shell profile the Run's shell reads) ``` -The Credentials are read only when `RESEND_API_KEY` is unset or empty, and only by `email-test`, `setup`, and a Run that asks for a notification. A missing file just means no key from it. One that isn't valid TOML, or holds anything but a quoted `resend.key`, such as a typo like `kye`, stops the command with exit `1`, naming the file and the offending key, and nothing is sent. One that others can read is still used, with a `warning:` line on stderr saying to `chmod 600` it. Nothing is sent to check the key itself. When Resend accepts the email, it prints `accepted by Resend; check your inbox` and exits `0`; that is all it can verify, so check that the email arrives. When Resend refuses it, for example for a bad key or a sender it won't send from, it prints Resend's error text word for word, with where the key came from, and exits `1`. It gives up after 30 seconds without an answer. +The Credentials are read only when `RESEND_API_KEY` is unset or empty, and only by `email-test`, `setup`, and a Run or an Architect run that asks for a notification. A missing file just means no key from it. One that isn't valid TOML, or holds anything but a quoted `resend.key`, such as a typo like `kye`, stops the command with exit `1`, naming the file and the offending key, and nothing is sent. One that others can read is still used, with a `warning:` line on stderr saying to `chmod 600` it. Nothing is sent to check the key itself. When Resend accepts the email, it prints `accepted by Resend; check your inbox` and exits `0`; that is all it can verify, so check that the email arrives. When Resend refuses it, for example for a bad key or a sender it won't send from, it prints Resend's error text word for word, with where the key came from, and exits `1`. It gives up after 30 seconds without an answer. #### Run notifications @@ -262,6 +264,8 @@ A Run that asks for a notification, by the flag or by `email.always`, makes the A [Spec run](#spec-runs) sends at most one notification for the whole Spec, under the same rules, with its checks made once before any Ticket starts. Its subject names the Spec, its outcome is the Spec run's, and its body, after what a Run's holds (the Spec PR, if any), lists each Ticket's outcome, one line per Ticket as in the summary on stderr, such as `#21 landed with https://github.com/acme/widgets/pull/1` or `#22 blocked by #21`. The Ticket Runs inside it never send a notification of their own, whatever the User config says. +An [Architect run](#architect-runs) takes the same flags and the same `email.always`, and sends [one notification](#one-run-notification) covering its review and the run it dispatched. + A notification that can't be sent is a `warning:` line on stderr with Resend's error. It never changes the Run's outcome, stdout or exit code. ### Logs @@ -271,6 +275,7 @@ Each session's full transcript, as Claude Code's `stream-json` output, is writte ``` ~/.thirdshift/logs/--issue---implement.jsonl ~/.thirdshift/logs/--issue---repair-.jsonl +~/.thirdshift/logs/--architect--architecture-review.jsonl ``` A Resume is logged as its session's kind plus `-resume`, e.g. `implement-resume.jsonl`. @@ -387,6 +392,60 @@ thirdshift: 03:12:40 Tickets not done: #21, #22, #24, #25, #26, #27 Running the Spec again picks up where the last Spec run stopped. The Spec branch is on `origin`, so the Spec run continues it ([ADR-0002](docs/adr/0002-existing-issue-branch-means-continue.md)) and updates its draft Spec PR rather than opening another. Closed Tickets are done and not run again, and a Ticket that failed still has its Issue branch and draft pull request, so its Run is a [Continuation](#continuation) into the Spec branch. With every Ticket closed, the Spec run goes straight to the Spec review and the Spec PR. With every Ticket closed and no Spec branch, as when the Spec was done some other way, it stops with `every Ticket is closed and there is no Spec branch; nothing to do` and exits `1`, creating no branch or pull request and leaving the Spec open. +## Architect runs + +`thirdshift architect` starts an **Architect run**: the factory looks for architecture work itself, with no **Issue URL**, publishes a plan for the top opportunity, and implements it. Start it from a clone of the repository, on the **Base branch**: + +```sh +cd ~/repos/widgets +thirdshift architect # review the whole codebase, then run the plan +thirdshift architect "the Spec run" # point the review at an area +thirdshift architect merge parallel 2 # merge the plan's pull request, two Tickets at once +thirdshift architect base-fix # the run the plan is dispatched as may start a Base fix +thirdshift architect "the Spec run" --plan-only # publish the plan, mark it ready and stop +thirdshift architect --email you@example.com # email how the Architect run ended +``` + +The focus is optional free text, given as one argument, anywhere among the flags; it goes into the Session prompt. `architect` is a command only as the first argument. + +`merge`, `no-merge`, `base-fix`, `no-base-fix` and `parallel ` (or `--merge`, `--no-merge`, `--base-fix`, `--no-base-fix` and `--parallel `) are for the run the plan is dispatched as, and mean what they do for `thirdshift `, so none of them is ever read as the focus. `--plan-only` stops the Architect run once the plan is marked ready, and dispatches nothing. `email`, optionally followed by an address, and `no-email` (or `--email` and `--no-email`) are for the Architect run's own [Run notification](#one-run-notification), with or without `--plan-only`. The word after `email` is the address only if it contains `@`, so a focus without one is never taken for it. + +A second focus, a repeated flag, `merge` with `no-merge`, `base-fix` with `no-base-fix`, `email` with `no-email`, `parallel` without a whole number from 1 up, any other argument starting with a dash, or an empty focus is an argument error (exit `2`). So is `merge`, `no-merge`, `base-fix`, `no-base-fix` or `parallel` with `--plan-only`, since nothing is dispatched for them to apply to. + +An Architect run: + +1. Makes the checks a Run makes that don't need an issue, before creating anything: `origin` is a GitHub repository, git has a `user.name` and `user.email`, HEAD is not detached, and the Base branch exists on `origin` with your local copy not ahead of it. There is no **Origin match**, since there is no Issue URL: the repository is the one `origin` names. With `launch.pull = true` in the [User config](#user-config), it then fast-forwards your checkout of the Base branch, as a Run does. +2. Creates a git worktree next to your clone, named `-architect`, detached at the head of the Base branch on `origin`, with no **Issue branch**. Your checkout, its uncommitted changes and its untracked files are never touched or scanned. +3. Runs the **Architecture review** there: a headless Claude Code session with the **Factory skills** loaded, started with the [Architecture review prompt](prompts/architecture-review.md). It looks for deepening opportunities, skips any an open issue already covers, and takes the top recommendation. If it is Strong, the review publishes it as the plan, labelled `needs-triage`: a **Spec** with **Tickets**, or a single Ticket when one session is enough. It may edit files in the worktree to check an idea, but commits and pushes nothing. It ends its final message with one line naming the plan: `Architecture review plan: `. thirdshift reads only that line. With [no Strong candidate](#no-strong-candidate) the line names another issue, and steps 5 to 7 are skipped. +4. Removes the worktree and the temporary plugin directory once the session ends, whatever the outcome. +5. Checks the plan: the issue is in this repository, is open, was created after the Architect run started, and carries no other label that says it is not agent work (`ready-for-human`, `needs-info` or `wontfix`). +6. Marks the plan ready, in one request: `needs-triage` is swapped for `ready-for-agent`, and its other labels are kept. A Spec's Tickets are left as the review labelled them. +7. Dispatches the plan, unless given `--plan-only`, exactly as `thirdshift ` would from the same clone: a [Spec run](#spec-runs) when the plan has sub-issues, a Run otherwise. `merge` and `no-merge` apply to the Spec PR or the Run's pull request, `parallel ` to the Spec run, and `base-fix` and `no-base-fix` to whether that run may start a [Base fix](#base-fix), as if given to that command, and the [User config](#user-config) sets what they leave unsaid: `merge.always`, `spec.parallel`, `base.fix`, `launch.pull` and `logs.dir`. The Architecture review itself never watches CI, so a Base fix can only happen in the dispatched run. The one difference is that it sends no [Run notification](#run-notifications) of its own, whatever `email.always` says: the Architect run sends [the one](#one-run-notification). + +The dispatched run's ending is the Architect run's: its exit code, its pull request's URL alone on stdout, and its last line on stderr, `PR is ready for review` or `PR is merged`. If it fails, the Architect run fails as that Failed run or Failed spec run does, with the cause and the session log on stderr and the pull request's URL on stdout if it left one. The plan stays `ready-for-agent`, for `thirdshift ` to take up again. `parallel ` on a plan that is a single Ticket fails the same way as it does for `thirdshift ` on an issue that isn't a Spec: `parallel is only for a Spec, and # has no sub-issues`, exit `1`, before any implementing, with the plan left ready to run without it. + +With `--plan-only`, it instead exits `0` with the plan's URL alone on stdout, and `plan is ready for an agent` as stderr's last line. Read or edit the plan, then run it with `thirdshift `. + +Progress lines on stderr say when the review starts, which plan it reported, when the labels are swapped, and when the plan is dispatched: `dispatching the plan , as thirdshift would`. The dispatched run's own progress lines follow. With no Strong candidate, the last line says which issue the Architect run ended on instead. + +A review session that fails or is interrupted, a final message without one of the lines the prompt asks for, or a plan that fails a check ends the Architect run as a failure: exit `1`, nothing on stdout, the cause on stderr and then the path of the session log. Nothing is dispatched and no label is changed, so a plan the review did publish stays `needs-triage` for you to finish or close. A review that finds no deepening opportunity at all has no issue to name, so it ends without one of those lines and the Architect run fails this way too; its session log says what it looked at. + +### One Run notification + +An Architect run asked for a [Run notification](#run-notifications), by `email` or by `email.always = true` in the [User config](#user-config) without `no-email`, sends exactly one, whatever its outcome and with or without `--plan-only`. It makes a Run's checks before any other work, an address and a Resend API key, and stops with exit `1` if either is missing. The email goes after the outcome is final and printed, and a failed send is only a `warning:` line on stderr: it changes neither the exit code nor stdout. + +- **Subject**: `[thirdshift] / Architect run: `. With a dispatched run, the outcome is that run's: `ready for review`, `merged`, `failed` or `interrupted`. Without one, it is the review's: `plan published` (with `--plan-only`), `idea filed`, `idea already filed`, `review failed` (also for a plan that fails a check) or `interrupted`. The repository is left out if `origin` doesn't name one on GitHub. +- **Body**, plain text: a `Review:` line saying how the Architecture review ended, with the URL of the plan or idea issue it named (`plan published: `, `idea filed: `, `idea already filed: `, `failed` or `interrupted`); when the plan was dispatched, a `Dispatched:` line with that run's outcome; then what a Run's notification holds, for the dispatched run or else the failed review: the pull request URL (if any), the failure cause (if failed), a `Base fix:` line for a dispatched run that started or waited on a [Base fix](#base-fix), the session log path (if any), the hostname and how long the Architect run took. After a dispatched Spec run, it ends with a line per Ticket, as a Spec run's notification does. + +### No Strong candidate + +Only a Strong top recommendation becomes a plan. When the review's top recommendation is Worth exploring or Speculative, it publishes no plan, and the Architect run ends in one of two ways, both a success, with or without `--plan-only`: exit `0`, with one issue's URL alone on stdout. thirdshift changes no label on that issue and dispatches nothing: there is nothing to run. + +- **An idea issue.** The review files its top recommendation as one issue labelled `needs-triage`, for the **Day shift** to flesh out, and ends its final message with `Architecture review idea: `. stdout carries the idea issue's URL, and stderr's last line is `no Strong candidate: the Architecture review filed the idea `. +- **Already filed.** An open issue already covers that recommendation, so the review files nothing and ends its final message with `Architecture review already filed: `. stdout carries that issue's URL, and stderr's last line is `no Strong candidate: already covers the Architecture review's top recommendation, so it filed nothing`. + +Start one Architect run per repository at a time. Nothing stops a second one, but two at once may pick the same opportunity and publish the same plan, and the second can't create its worktree while the first's is there. + ## Building from source Building needs the Rust toolchain. From a clone of this repository: diff --git a/prompts/README.md b/prompts/README.md index 666bfc89e..df45b80f5 100644 --- a/prompts/README.md +++ b/prompts/README.md @@ -22,6 +22,7 @@ claude -p --permission-mode auto --plugin-dir --output-format strea | Continuation, with no pull request | Starts the implement session when the Run is a Continuation of an Issue branch that has no pull request. | [continuation.md](continuation.md) | | Continuation, with an open pull request | Starts the implement session when the Run is a Continuation of an Issue branch whose pull request is open. | [continuation-pr.md](continuation-pr.md) | | Spec review | In a Spec run, starts the Spec review once every Ticket has landed on the Spec branch, before the Spec PR, a draft until then, is marked ready. | [spec-review.md](spec-review.md) | +| Architecture review | Starts the Architecture review, the session an Architect run opens with, in a worktree at the head of the Base branch. The line naming the focus is left out when the command gives none. | [architecture-review.md](architecture-review.md) | | Conflict Repair | Starts a Repair session when merging the Base branch into the Issue branch leaves conflicts. | [conflict-repair.md](conflict-repair.md) | | Conflict Repair, on Foreign commits | In a Merge run, starts a Repair session when merging Foreign commits from the Issue branch on origin into the local one leaves conflicts. | [foreign-conflict-repair.md](foreign-conflict-repair.md) | | Review Repair | In a Merge run, starts a Repair session once Foreign commits are merged into the Issue branch, to review them from the head the Run last knew as its own before they can be merged. | [review-repair.md](review-repair.md) | diff --git a/prompts/architecture-review.md b/prompts/architecture-review.md new file mode 100644 index 000000000..00d42859b --- /dev/null +++ b/prompts/architecture-review.md @@ -0,0 +1,28 @@ + + +# Architecture review + +Starts the Architecture review, the session an Architect run opens with, in a worktree at the head of the Base branch. The line naming the focus is left out when the command gives none. + +``` +/thirdshift:improve-codebase-architecture + +This is an Architecture review of the base branch : this worktree is checked out at its head on origin, on no branch. + +Focus the review on: + +Find the deepening opportunities with /thirdshift:improve-codebase-architecture, using /thirdshift:codebase-design for the vocabulary. Skip any that an open issue already covers, and take the top recommendation. + +If it is Strong, settle its design yourself and publish it as the plan with /thirdshift:to-spec and /thirdshift:to-tickets: a Spec with Tickets, or a single Ticket when one session is enough. Label the plan's top issue, the Spec or the single Ticket, `needs-triage`, not `ready-for-agent`: thirdshift marks it ready once you are done. + +If it is not Strong, file it as one issue labelled `needs-triage`, unless an open issue already covers it. + +Do not commit or push anything. You may edit files here to check an idea: the worktree is thrown away when you finish. A change the plan needs to CONTEXT.md or an ADR is a Ticket's work, not yours. + +End your final message with one of these lines, as its last line, with the issue's full URL and nothing else on the line: +Architecture review plan: +Architecture review idea: +Architecture review already filed: + +You run headless: nobody is watching, and ending your turn ends the session. Run tests and other long commands in the foreground, raising the Bash timeout if needed. If a command is moved to the background, wait for that task by its own task id or output file, never by process names or patterns (`pgrep`, `ps | grep`, and the like): other sessions on this machine run the same commands. Never end your turn while a background task you depend on is still running: ending the turn kills it. +``` diff --git a/site/prompts/index.html b/site/prompts/index.html index 8175af7b9..4ccb07286 100644 --- a/site/prompts/index.html +++ b/site/prompts/index.html @@ -124,6 +124,32 @@

Spec review

Include "Closes #<n>" in the PR body. +You run headless: nobody is watching, and ending your turn ends the session. Run tests and other long commands in the foreground, raising the Bash timeout if needed. If a command is moved to the background, wait for that task by its own task id or output file, never by process names or patterns (`pgrep`, `ps | grep`, and the like): other sessions on this machine run the same commands. Never end your turn while a background task you depend on is still running: ending the turn kills it. + + +
+

Architecture review

+

Starts the Architecture review, the session an Architect run opens with, in a worktree at the head of the Base branch. The line naming the focus is left out when the command gives none.

+

Sent by an Architect run, before any unit

+
/thirdshift:improve-codebase-architecture
+
+This is an Architecture review of the base branch <base>: this worktree is checked out at its head on origin, on no branch.
+
+Focus the review on: <focus>
+
+Find the deepening opportunities with /thirdshift:improve-codebase-architecture, using /thirdshift:codebase-design for the vocabulary. Skip any that an open issue already covers, and take the top recommendation.
+
+If it is Strong, settle its design yourself and publish it as the plan with /thirdshift:to-spec and /thirdshift:to-tickets: a Spec with Tickets, or a single Ticket when one session is enough. Label the plan's top issue, the Spec or the single Ticket, `needs-triage`, not `ready-for-agent`: thirdshift marks it ready once you are done.
+
+If it is not Strong, file it as one issue labelled `needs-triage`, unless an open issue already covers it.
+
+Do not commit or push anything. You may edit files here to check an idea: the worktree is thrown away when you finish. A change the plan needs to CONTEXT.md or an ADR is a Ticket's work, not yours.
+
+End your final message with one of these lines, as its last line, with the issue's full URL and nothing else on the line:
+Architecture review plan: <URL of the Spec or the single Ticket you published>
+Architecture review idea: <URL of the issue you filed>
+Architecture review already filed: <URL of the open issue that already covers it>
+
 You run headless: nobody is watching, and ending your turn ends the session. Run tests and other long commands in the foreground, raising the Bash timeout if needed. If a command is moved to the background, wait for that task by its own task id or output file, never by process names or patterns (`pgrep`, `ps | grep`, and the like): other sessions on this machine run the same commands. Never end your turn while a background task you depend on is still running: ending the turn kills it.
 
@@ -326,6 +352,216 @@

code-review/agents/openai.yam
interface:
   display_name: "Code Review"
   short_description: "Review a diff on standards and spec"
+
+ +
+

codebase-design

+

Used by the Architecture review, in an Architect run

+

codebase-design/SKILL.md

+
---
+name: codebase-design
+description: Shared vocabulary for designing deep modules. Use when designing or improving a module's interface, finding deepening opportunities, deciding where a seam goes, making code more testable or AI-navigable, or when another skill needs the deep-module vocabulary.
+---
+
+# Codebase Design
+
+Design **deep modules**: a lot of behaviour behind a small interface, placed at a clean seam, testable through that interface. Use this language and these principles wherever code is being designed or restructured. The aim is leverage for callers, locality for maintainers, and testability for everyone.
+
+## Glossary
+
+Use these terms exactly: don't substitute "component," "service," "API," or "boundary." Consistent language is the whole point.
+
+**Module**: anything with an interface and an implementation. Deliberately scale-agnostic: a function, class, package, or tier-spanning slice. _Avoid_: unit, component, service.
+
+**Interface**: everything a caller must know to use the module correctly: the type signature, but also invariants, ordering constraints, error modes, required configuration, and performance characteristics. _Avoid_: API, signature (too narrow, they refer only to the type-level surface).
+
+**Implementation**: what's inside a module, its body of code. Distinct from **Adapter**: a thing can be a small adapter with a large implementation (a Postgres repo) or a large adapter with a small implementation (an in-memory fake). Reach for "adapter" when the seam is the topic; "implementation" otherwise.
+
+**Depth**: leverage at the interface. The amount of behaviour a caller (or test) can exercise per unit of interface they have to learn. A module is **deep** when a large amount of behaviour sits behind a small interface, **shallow** when the interface is nearly as complex as the implementation.
+
+**Seam** _(Michael Feathers)_: a place where you can alter behaviour without editing in that place; the *location* at which a module's interface lives. Where to put the seam is its own design decision, distinct from what goes behind it. _Avoid_: boundary (overloaded with DDD's bounded context).
+
+**Adapter**: a concrete thing that satisfies an interface at a seam. Describes *role* (what slot it fills), not substance (what's inside).
+
+**Leverage**: what callers get from depth. More capability per unit of interface they learn. One implementation pays back across N call sites and M tests.
+
+**Locality**: what maintainers get from depth. Change, bugs, knowledge, and verification concentrate in one place rather than spreading across callers. Fix once, fixed everywhere.
+
+## Deep vs shallow
+
+**Deep module** = small interface + lots of implementation:
+
+```
+┌─────────────────────┐
+│   Small Interface   │  ← Few methods, simple params
+├─────────────────────┤
+│                     │
+│  Deep Implementation│  ← Complex logic hidden
+│                     │
+└─────────────────────┘
+```
+
+**Shallow module** = large interface + little implementation (avoid):
+
+```
+┌─────────────────────────────────┐
+│       Large Interface           │  ← Many methods, complex params
+├─────────────────────────────────┤
+│  Thin Implementation            │  ← Just passes through
+└─────────────────────────────────┘
+```
+
+When designing an interface, ask:
+
+- Can I reduce the number of methods?
+- Can I simplify the parameters?
+- Can I hide more complexity inside?
+
+## Principles
+
+- **Depth is a property of the interface, not the implementation.** A deep module can be internally composed of small, mockable, swappable parts; they just aren't part of the interface. A module can have **internal seams** (private to its implementation, used by its own tests) as well as the **external seam** at its interface.
+- **The deletion test.** Imagine deleting the module. If complexity vanishes, it was a pass-through. If complexity reappears across N callers, it was earning its keep.
+- **The interface is the test surface.** Callers and tests cross the same seam. If you want to test *past* the interface, the module is probably the wrong shape.
+- **One adapter means a hypothetical seam. Two adapters means a real one.** Don't introduce a seam unless something actually varies across it.
+
+## Designing for testability
+
+Good interfaces make testing natural:
+
+1. **Accept dependencies, don't create them.**
+
+   ```typescript
+   // Testable
+   function processOrder(order, paymentGateway) {}
+
+   // Hard to test
+   function processOrder(order) {
+     const gateway = new StripeGateway();
+   }
+   ```
+
+2. **Return results, don't produce side effects.**
+
+   ```typescript
+   // Testable
+   function calculateDiscount(cart): Discount {}
+
+   // Hard to test
+   function applyDiscount(cart): void {
+     cart.total -= discount;
+   }
+   ```
+
+3. **Small surface area.** Fewer methods = fewer tests needed. Fewer params = simpler test setup.
+
+## Relationships
+
+- A **Module** has exactly one **Interface** (the surface it presents to callers and tests).
+- **Depth** is a property of a **Module**, measured against its **Interface**.
+- A **Seam** is where a **Module**'s **Interface** lives.
+- An **Adapter** sits at a **Seam** and satisfies the **Interface**.
+- **Depth** produces **Leverage** for callers and **Locality** for maintainers.
+
+## Rejected framings
+
+- **Depth as ratio of implementation-lines to interface-lines** (Ousterhout): rewards padding the implementation. We use depth-as-leverage instead.
+- **"Interface" as the TypeScript `interface` keyword or a class's public methods**: too narrow: interface here includes every fact a caller must know.
+- **"Boundary"**: overloaded with DDD's bounded context. Say **seam** or **interface**.
+
+## Going deeper
+
+- **Deepening a cluster given its dependencies**, see [DEEPENING.md](DEEPENING.md): dependency categories, seam discipline, and replace-don't-layer testing.
+- **Exploring alternative interfaces**, see [DESIGN-IT-TWICE.md](DESIGN-IT-TWICE.md): spin up parallel sub-agents to design the interface several radically different ways, then compare on depth, locality, and seam placement.
+
+

codebase-design/DEEPENING.md

+
# Deepening
+
+How to deepen a cluster of shallow modules safely, given its dependencies. Assumes the vocabulary in [SKILL.md](SKILL.md): **module**, **interface**, **seam**, **adapter**.
+
+## Dependency categories
+
+When assessing a candidate for deepening, classify its dependencies. The category determines how the deepened module is tested across its seam.
+
+### 1. In-process
+
+Pure computation, in-memory state, no I/O. Always deepenable: merge the modules and test through the new interface directly. No adapter needed.
+
+### 2. Local-substitutable
+
+Dependencies that have local test stand-ins (PGLite for Postgres, in-memory filesystem). Deepenable if the stand-in exists. The deepened module is tested with the stand-in running in the test suite. The seam is internal; no port at the module's external interface.
+
+### 3. Remote but owned (Ports & Adapters)
+
+Your own services across a network boundary (microservices, internal APIs). Define a **port** (interface) at the seam. The deep module owns the logic; the transport is injected as an **adapter**. Tests use an in-memory adapter. Production uses an HTTP/gRPC/queue adapter.
+
+Recommendation shape: *"Define a port at the seam, implement an HTTP adapter for production and an in-memory adapter for testing, so the logic sits in one deep module even though it's deployed across a network."*
+
+### 4. True external (Mock)
+
+Third-party services (Stripe, Twilio, etc.) you don't control. The deepened module takes the external dependency as an injected port; tests provide a mock adapter.
+
+## Seam discipline
+
+- **One adapter means a hypothetical seam. Two adapters means a real one.** Don't introduce a port unless at least two adapters are justified (typically production + test). A single-adapter seam is just indirection.
+- **Internal seams vs external seams.** A deep module can have internal seams (private to its implementation, used by its own tests) as well as the external seam at its interface. Don't expose internal seams through the interface just because tests use them.
+
+## Testing strategy: replace, don't layer
+
+- Old unit tests on shallow modules become waste once tests at the deepened module's interface exist; delete them.
+- Write new tests at the deepened module's interface. The **interface is the test surface**.
+- Tests assert on observable outcomes through the interface, not internal state.
+- Tests should survive internal refactors, since they describe behaviour, not implementation. If a test has to change when the implementation changes, it's testing past the interface.
+
+

codebase-design/DESIGN-IT-TWICE.md

+
# Design It Twice
+
+To explore alternative interfaces for a chosen deepening candidate, use this parallel sub-agent pattern. Based on "Design It Twice" (Ousterhout): your first idea is unlikely to be the best.
+
+Uses the vocabulary in [SKILL.md](SKILL.md): **module**, **interface**, **seam**, **adapter**, **leverage**.
+
+## Process
+
+### 1. Frame the problem space
+
+Before spawning sub-agents, write down the problem space for the chosen candidate:
+
+- The constraints any new interface would need to satisfy
+- The dependencies it would rely on, and which category they fall into (see [DEEPENING.md](DEEPENING.md))
+- A rough illustrative code sketch to ground the constraints, not a proposal, just a way to make the constraints concrete
+
+Nobody is there to read it, so don't wait: proceed to Step 2. It is what you compare the designs against in Step 3.
+
+### 2. Spawn sub-agents
+
+Spawn 3+ sub-agents in parallel. Each must produce a **radically different** interface for the deepened module.
+
+Prompt each sub-agent with a separate technical brief (file paths, coupling details, dependency category from [DEEPENING.md](DEEPENING.md), what sits behind the seam). The brief is independent of the problem-space explanation in Step 1. Give each agent a different design constraint:
+
+- Agent 1: "Minimize the interface: aim for 1–3 entry points max. Maximise leverage per entry point."
+- Agent 2: "Maximise flexibility: support many use cases and extension."
+- Agent 3: "Optimise for the most common caller: make the default case trivial."
+- Agent 4 (if applicable): "Design around ports & adapters for cross-seam dependencies."
+
+Include both [SKILL.md](SKILL.md) vocabulary and CONTEXT.md vocabulary in the brief so each sub-agent names things consistently with the architecture language and the project's domain language.
+
+Each sub-agent outputs:
+
+1. Interface (types, methods, params, plus invariants, ordering, error modes)
+2. Usage example showing how callers use it
+3. What the implementation hides behind the seam
+4. Dependency strategy and adapters (see [DEEPENING.md](DEEPENING.md))
+5. Trade-offs: where leverage is high, where it's thin
+
+### 3. Compare and decide
+
+Compare the designs in prose. Contrast by **depth** (leverage at the interface), **locality** (where change concentrates), and **seam placement**.
+
+After comparing, decide: which design is strongest and why. If elements from different designs would combine well, take a hybrid. Be opinionated: the choice is yours to make, and nobody picks from a menu. Record the decision and its reason, with the designs you turned down and why.
+
+

codebase-design/agents/openai.yaml

+
interface:
+  display_name: "Codebase Design"
+  short_description: "Vocabulary for deep-module design"
 
@@ -354,6 +590,263 @@

implement/agents/openai.yaml +

+
+

improve-codebase-architecture

+

Used by the Architecture review, in an Architect run

+

improve-codebase-architecture/SKILL.md

+
---
+name: improve-codebase-architecture
+description: "Scan a codebase for deepening opportunities, take the top recommendation, and publish it to the issue tracker: as a plan when it is Strong, as an idea otherwise. Only for an Architecture review."
+disable-model-invocation: false
+---
+
+# Improve Codebase Architecture
+
+Surface architectural friction and propose **deepening opportunities**: refactors that turn shallow modules into deep ones. The aim is testability and AI-navigability.
+
+This is an **Architecture review**. It runs headless: nobody reads a report, picks a candidate or answers a question. Never open a browser, ask a question or wait for an answer. Every decision is yours, and each one is written down with its reason.
+
+It changes nothing in the repository. Never commit, push, create a branch or open a pull request, and never edit `CONTEXT.md` or an ADR: any change the refactor needs to them is work for a Ticket. The worktree is thrown away when the session ends, so a throwaway prototype in it is fine, and nothing in it survives. What the review produces is issues on the issue tracker.
+
+This command is _informed_ by the project's domain model and built on a shared design vocabulary:
+
+- Call the Skill tool with "thirdshift:codebase-design" for the architecture vocabulary (**module**, **interface**, **depth**, **seam**, **adapter**, **leverage**, **locality**) and its principles (the deletion test, "the interface is the test surface", "one adapter = hypothetical seam, two = real"). Use these terms exactly in every suggestion, and don't drift into "component," "service," "API," or "boundary."
+- The domain language in `CONTEXT.md` gives names to good seams; ADRs in `docs/adr/` record decisions this command should not re-litigate.
+
+The issue tracker and triage label vocabulary should have been provided to you. If `docs/agents/issue-tracker.md` is missing, fall back to the `gh` CLI. If `docs/agents/triage-labels.md` is missing, use the label names as written here, creating a label the tracker lacks (`gh label create`).
+
+## Process
+
+### 1. Explore
+
+**Scope before you scan: YAGNI.** Deepening a module pays off by making future changes to it easier, so put extra weight on the parts of the codebase that have recently changed. Decide *where* to look before you look:
+
+- If the Session prompt named a focus (a module, a subsystem, a pain point), take it, and skip the inference below.
+- Otherwise, walk back a good stretch of the commit history (`git log --oneline`) to find the codebase's hot spots, the files and areas that keep coming up, and let those paths pull your attention first. If the changes are scattered with no clear hot spot, widen the net.
+
+Read the project's domain glossary (`CONTEXT.md`) and any ADRs in the area you're touching first.
+
+Read the open issues too, titles and bodies: Specs, Tickets and ideas alike, whatever their labels. They are what a candidate may already be covered by.
+
+Then spawn a sub-agent to walk the codebase. Don't follow rigid heuristics; explore organically and note where you experience friction:
+
+- Where does understanding one concept require bouncing between many small modules?
+- Where are modules **shallow**, with an interface nearly as complex as the implementation?
+- Where have pure functions been extracted just for testability, but the real bugs hide in how they're called (no **locality**)?
+- Where do tightly-coupled modules leak across their seams?
+- Which parts of the codebase are untested, or hard to test through their current interface?
+
+Apply the **deletion test** to anything you suspect is shallow: would deleting it concentrate complexity, or just move it? A "yes, concentrates" is the signal you want.
+
+### 2. Take the top recommendation
+
+Weigh the candidates in the session. Write no report file. For each candidate, settle:
+
+- **Files**: which files/modules are involved
+- **Problem**: why the current architecture is causing friction
+- **Solution**: plain English description of what would change
+- **Benefits**: explained in terms of locality and leverage, and how tests would improve
+- **Recommendation strength**: one of `Strong`, `Worth exploring`, `Speculative`
+- **Covered by**: the open issue that already covers it, if one does
+
+A candidate is **Strong** only when all of these hold:
+
+- The friction is real and you can point to it in the code.
+- The deepening passes the deletion test.
+- You can settle its whole design from the code, `CONTEXT.md` and the ADRs. A candidate with a decision only a human can make is `Worth exploring` at most.
+- It contradicts no ADR, or the friction is real enough to warrant reopening that ADR.
+
+A candidate is **covered** when an open issue proposes the same deepening of the same modules, whether as a Spec, a Ticket or an idea. An issue that only touches the same files doesn't cover it.
+
+Rank the candidates: which you'd tackle first and why. Then take the **top recommendation**:
+
+1. The highest-ranked `Strong` candidate that no open issue covers. It becomes the plan: go on to step 3.
+2. If there is none, the highest-ranked candidate of all. If an open issue covers it, file nothing: go to step 6 and name that issue. Otherwise it becomes an idea: go to step 5.
+
+Every other candidate is dropped. Don't file it, and don't list it in the plan or the idea: the next Architecture review starts from the codebase as it is by then.
+
+If the scan surfaced no candidate at all, file nothing, and say so plainly in place of the final line of step 6: there is no issue for it to name.
+
+**Use CONTEXT.md vocabulary for the domain, and the `/thirdshift:codebase-design` vocabulary for the architecture.** If `CONTEXT.md` defines "Order," talk about "the Order intake module," not "the FooBarHandler," and not "the Order service."
+
+**ADR conflicts**: if a candidate contradicts an existing ADR, only keep it when the friction is real enough to warrant revisiting the ADR. Mark it clearly in the report (e.g. a warning callout: _"contradicts ADR-0007, but worth reopening because…"_). Don't raise every theoretical refactor an ADR forbids.
+
+Do NOT propose interfaces yet.
+
+### 3. Settle the design
+
+Only for a `Strong` top recommendation.
+
+Map the decisions as a **design tree**: every decision branches into the decisions that hang off it. Walk it: constraints, dependencies, the shape of the deepened module, what sits behind the seam, what tests survive.
+
+Work the tree in **rounds**. The **frontier** is every decision whose prerequisites are already settled. Settle the whole frontier yourself, then recompute it. Finding _facts_ is a lookup: when a decision needs a fact from the environment (filesystem, tools, etc.), dispatch a sub-agent to find it. The _decisions_ are yours too. For each one, write down what you decided and why. The design is settled when the frontier is empty: every branch of the design tree visited, nothing left silently assumed.
+
+If a decision turns out to need a human, the candidate was not `Strong`: make it `Worth exploring` and take the top recommendation again, from step 2.
+
+Side effects are recorded, never written:
+
+- **Naming a deepened module after a concept not in `CONTEXT.md`?** Record the term and its definition as work for a Ticket.
+- **Sharpening a fuzzy term as you decide?** Record the sharper definition as work for a Ticket.
+- **Reopening an ADR, or making a decision worth one?** Record the new or changed ADR as work for a Ticket. A decision is worth an ADR only when it is hard to reverse, surprising without context, and the result of a real trade-off.
+- **Want to explore alternative interfaces for the deepened module?** Call the Skill tool with "thirdshift:codebase-design" and use its design-it-twice parallel sub-agent pattern.
+
+### 4. Publish the plan
+
+Only for a `Strong` top recommendation whose design is settled.
+
+Decide the plan's size. If one session, a single fresh context window, is enough for the whole change, the plan is a single **Ticket**. Otherwise it is a **Spec** with **Tickets**. Don't force a small refactor into a Spec.
+
+- **A Spec with Tickets**: call the Skill tool with "thirdshift:to-spec" to publish the Spec, then with "thirdshift:to-tickets" to publish its Tickets as the Spec's sub-issues.
+- **A single Ticket**: call the Skill tool with "thirdshift:to-tickets" to publish one standalone Ticket, with no parent.
+
+The plan's top issue, the Spec or the single Ticket, is labelled `needs-triage` and nothing else. thirdshift marks it ready once the session has ended and the plan passes its checks; you never do. Tickets under a Spec are labelled `ready-for-agent`.
+
+The plan's top issue carries, whichever shape it takes:
+
+- **Every decision from step 3, each with its reason.** In a Spec, under Implementation Decisions and Testing Decisions. In a single Ticket, under a `## Decisions` heading added after the issue template's sections.
+- **The report**: the candidate's card as Markdown with a Mermaid before/after diagram. In a Spec, under Further Notes. In a single Ticket, under an `## Architecture review` heading added last, after `## Decisions`. See [REPORT.md](REPORT.md) for the card.
+
+The `CONTEXT.md` and ADR changes recorded in step 3 go into the acceptance criteria of the Ticket whose work they belong to, a Spec's Ticket or the single Ticket, so they land through its pull request.
+
+### 5. File the idea
+
+Only for a top recommendation that is not `Strong` and that no open issue covers.
+
+Publish one issue to the issue tracker, labelled `needs-triage`, for a human to flesh out. Its title names the deepening. Its body is the report: the candidate's card as Markdown with a Mermaid before/after diagram, its recommendation strength, and what stopped it being `Strong`. See [REPORT.md](REPORT.md) for the card.
+
+Publish nothing else: no Spec, no Tickets, and no second idea.
+
+### 6. End with the final line
+
+Unless the scan surfaced no candidate at all, end your final message with the one final line the Session prompt asks for, in exactly the format it gives. The format is defined there and nowhere else. The line names one issue:
+
+- the plan's top issue, when you published a plan;
+- the idea issue, when you filed one;
+- the open issue that already covers the top recommendation, when you filed nothing.
+
+thirdshift reads only that line, so everything else you want read belongs in the issue.
+
+

improve-codebase-architecture/REPORT.md

+
# Report Format
+
+The Architecture review's report is Markdown, written into the issue it publishes: the plan's top issue, or the idea issue. GitHub renders Mermaid in a fenced `mermaid` block, so the diagrams live where the issue is read. Write no file, and open nothing.
+
+The report is one candidate's card: the top recommendation's. The candidates that weren't chosen get no card.
+
+## Candidate card
+
+The diagrams carry the weight. Prose is sparse, plain, and uses the glossary terms (from the `/thirdshift:codebase-design` skill) without ceremony.
+
+```md
+**Strength**: Strong · **Dependencies**: in-process
+
+**Files**: `src/order/handler.rs`, `src/order/validator.rs`, `src/order/repo.rs`
+
+**Before**
+
+<a mermaid block>
+
+**After**
+
+<a mermaid block>
+
+**Problem**: one sentence. What hurts.
+
+**Solution**: one sentence. What changes.
+
+**Wins**
+
+- Tests hit one interface
+- Pricing logic stops leaking
+
+> [!WARNING]
+> Contradicts ADR-0007, but worth reopening because…
+```
+
+- **Strength**: the recommendation strength (`Strong`, `Worth exploring` or `Speculative`), plus the dependency category (`in-process`, `local-substitutable`, `ports & adapters`, `mock`). On an idea issue, add one line saying what stopped it being `Strong`.
+- **Files**: a list in code spans. The report is the one place file paths belong; the rest of a Spec or Ticket leaves them out.
+- **Before / After diagram**: the centrepiece. One Mermaid block each, the same modules in both so the eye can compare. See patterns below.
+- **Problem**: one sentence. What hurts.
+- **Solution**: one sentence. What changes.
+- **Wins**: bullets, ≤6 words each. e.g. "Tests hit one interface", "Pricing logic stops leaking", "Delete 4 shallow wrappers".
+- **ADR callout** (if applicable): one line in a `> [!WARNING]` alert.
+
+No paragraphs of explanation. If the diagram needs a paragraph to be understood, redraw the diagram.
+
+## Diagram patterns
+
+Pick the pattern that fits the candidate. Before and after use the same pattern.
+
+### Flowchart (the workhorse for dependencies / call flow)
+
+Use a Mermaid `flowchart` when the point is "X calls Y calls Z, and look at the mess." Style with `classDef` to colour leakage edges red and give the deep module a thick border. A dashed edge is leakage across a seam.
+
+````md
+```mermaid
+flowchart LR
+  A[OrderHandler] --> B[OrderValidator]
+  B --> C[OrderRepo]
+  C -.leak.-> D[PricingClient]
+  classDef leak stroke:#dc2626,stroke-width:2px;
+  class C,D leak
+```
+````
+
+### Sequence diagram (good for round-trips)
+
+Use a `sequenceDiagram` for "before: 6 round-trips; after: 1."
+
+### Subgraph collapse (good for layered shallowness and call-graph collapse)
+
+Before: the shallow modules as separate nodes, each call an edge. After: one `subgraph` labelled with the consolidated responsibility, the now-internal calls inside it, and one edge in from the caller.
+
+````md
+```mermaid
+flowchart LR
+  Caller --> Intake
+  subgraph Intake[Order intake]
+    direction LR
+    validate --> price --> store
+  end
+  classDef deep stroke-width:3px;
+  class Intake deep
+```
+````
+
+## Style guidance
+
+- Keep each diagram small: a dozen nodes at most. Leave out whatever doesn't change between before and after.
+- Colour sparingly: red for leakage, a thick border for the deep module, nothing else.
+- Label nodes with the module's name in the domain language, not a sentence.
+- Check that each block is valid Mermaid before publishing: a diagram that doesn't render is worse than none.
+
+## Tone
+
+Plain English, concise, but the architectural nouns and verbs come straight from the `/thirdshift:codebase-design` skill. Concision is not an excuse to drift.
+
+**Use exactly:** module, interface, implementation, depth, deep, shallow, seam, adapter, leverage, locality.
+
+**Never substitute:** component, service, unit (for module) · API, signature (for interface) · boundary (for seam) · layer, wrapper (for module, when you mean module).
+
+**Phrasings that fit the style:**
+
+- "Order intake module is shallow: interface nearly matches the implementation."
+- "Pricing leaks across the seam."
+- "Deepen: one interface, one place to test."
+- "Two adapters justify the seam: HTTP in prod, in-memory in tests."
+
+**Wins bullets** name the gain in glossary terms: *"locality: bugs concentrate in one module"*, *"leverage: one interface, N call sites"*, *"interface shrinks; implementation absorbs the wrappers"*. Don't write *"easier to maintain"* or *"cleaner code"*, because those terms aren't in the glossary and don't earn their place.
+
+No hedging, no throat-clearing, no "it's worth noting that…". If a sentence could be a bullet, make it a bullet. If a bullet could be cut, cut it. If a term isn't in the `/thirdshift:codebase-design` glossary, reach for one that is before inventing a new one.
+
+

improve-codebase-architecture/agents/openai.yaml

+
interface:
+  display_name: "Improve Codebase Architecture"
+  short_description: "Find and publish the top architecture improvement"
+policy:
+  allow_implicit_invocation: true
 
@@ -751,6 +1244,209 @@

tdd/tests.md

expect(calculateTotal([{ price: 10 }, { price: 5 }])).toBe(15); }); ``` +
+
+
+

to-spec

+

Used by the Architecture review, in an Architect run

+

to-spec/SKILL.md

+
---
+name: to-spec
+description: "Turn the current session into a spec and publish it to the project issue tracker: no interview, just synthesis of what you've already settled. Only for an Architecture review."
+disable-model-invocation: false
+---
+
+This skill takes the current session context and codebase understanding and produces a spec. Nobody is there to interview or to ask; just synthesize what you already know.
+
+The issue tracker and triage label vocabulary should have been provided to you. If `docs/agents/issue-tracker.md` is missing, fall back to `gh issue create`. If `docs/agents/triage-labels.md` is missing, use the label names as written here, creating a label the tracker lacks (`gh label create`).
+
+The spec is all this skill writes. Write nothing to the repository: no spec file, commits or branches. A `CONTEXT.md` or ADR change the spec needs is work for one of its tickets.
+
+## Process
+
+1. Explore the repo to understand the current state of the codebase, if you haven't already. Use the project's domain glossary vocabulary throughout the spec, and respect any ADRs in the area you're touching.
+
+2. Sketch out the seams at which the feature will be tested. Existing seams should be preferred to new ones. Use the highest seam possible. If new seams are needed, propose them at the highest point you can. The fewer seams across the codebase, the better - the ideal number is one.
+
+Decide the seams yourself and record them, with the reason for each, under Testing Decisions.
+
+3. Write the spec using the template below, then publish it to the project issue tracker. Apply the `needs-triage` triage label and no other: thirdshift marks the spec ready once the session has ended and the plan passes its checks.
+
+<spec-template>
+
+## Problem Statement
+
+The problem that the user is facing, from the user's perspective.
+
+## Solution
+
+The solution to the problem, from the user's perspective.
+
+## User Stories
+
+A LONG, numbered list of user stories. Each user story should be in the format of:
+
+1. As an <actor>, I want a <feature>, so that <benefit>
+
+<user-story-example>
+1. As a mobile bank customer, I want to see balance on my accounts, so that I can make better informed decisions about my spending
+</user-story-example>
+
+This list of user stories should be extremely extensive and cover all aspects of the feature.
+
+## Implementation Decisions
+
+A list of implementation decisions that were made, each with its reason. This can include:
+
+- The modules that will be built/modified
+- The interfaces of those modules that will be modified
+- Architectural decisions
+- Schema changes
+- API contracts
+- Specific interactions
+
+Do NOT include specific file paths or code snippets. They may end up being outdated very quickly.
+
+Exception: if a prototype produced a snippet that encodes a decision more precisely than prose can (state machine, reducer, schema, type shape), inline it within the relevant decision and note briefly that it came from a prototype. Trim to the decision-rich parts, not a working demo, just the important bits.
+
+## Testing Decisions
+
+A list of testing decisions that were made, each with its reason. Include:
+
+- A description of what makes a good test (only test external behavior, not implementation details)
+- The seams the feature will be tested at
+- Which modules will be tested
+- Prior art for the tests (i.e. similar types of tests in the codebase)
+
+## Out of Scope
+
+A description of the things that are out of scope for this spec.
+
+## Further Notes
+
+Any further notes about the feature.
+
+</spec-template>
+
+

to-spec/agents/openai.yaml

+
interface:
+  display_name: "To Spec"
+  short_description: "Turn a session into a spec"
+policy:
+  allow_implicit_invocation: true
+
+
+
+

to-tickets

+

Used by the Architecture review, in an Architect run

+

to-tickets/SKILL.md

+
---
+name: to-tickets
+description: Break a plan, spec, or the current session into a set of tracer-bullet tickets, each declaring its blocking edges, published to the issue tracker with native sub-issue and blocking links. Only for an Architecture review.
+disable-model-invocation: false
+---
+
+# To Tickets
+
+Break a plan, spec, or session into a set of **tickets**: tracer-bullet vertical slices, each declaring the tickets that **block** it.
+
+The issue tracker and triage label vocabulary should have been provided to you. If `docs/agents/issue-tracker.md` is missing, fall back to the `gh` CLI. If `docs/agents/triage-labels.md` is missing, use the label names as written here, creating a label the tracker lacks (`gh label create`).
+
+The tickets are all this skill writes. Write nothing to the repository: no ticket files, commits or branches. A `CONTEXT.md` or ADR change the work needs is part of a ticket's work, named in its acceptance criteria.
+
+## Process
+
+### 1. Gather context
+
+Work from whatever is already in the session context. If you were given a reference (an issue number or URL) as an argument, fetch it and read its full body and comments.
+
+### 2. Explore the codebase (optional)
+
+If you have not already explored the codebase, do so to understand the current state of the code. Ticket titles and descriptions should use the project's domain glossary vocabulary, and respect ADRs in the area you're touching.
+
+Look for opportunities to prefactor the code to make the implementation easier. "Make the change easy, then make the easy change."
+
+### 3. Draft vertical slices
+
+Break the work into **tracer bullet** tickets.
+
+<vertical-slice-rules>
+
+- Each slice cuts a narrow but COMPLETE path through every layer (schema, API, UI, tests): vertical, NOT a horizontal slice of one layer
+- A completed slice is demoable or verifiable on its own
+- Each slice is sized to fit in a single fresh context window
+- Any prefactoring should be done first
+
+</vertical-slice-rules>
+
+Give each ticket its **blocking edges**: the other tickets that must complete before it can start. A ticket with no blockers can start immediately.
+
+**Wide refactors are the exception to vertical slicing.** A **wide refactor** is one mechanical change (rename a column, retype a shared symbol) whose **blast radius** fans across the whole codebase, so a single edit breaks thousands of call sites at once and no vertical slice can land green. Don't force it into a tracer bullet; sequence it as **expand–contract**. First expand: add the new form beside the old so nothing breaks. Then migrate the call sites over in batches sized by blast radius (per package, per directory), each batch its own ticket blocked by the expand, keeping CI green batch to batch because the old form still exists. Finally contract: delete the old form once no caller remains, in a ticket blocked by every migrate batch. When even the batches can't stay green alone, keep the sequence but let them share an integration branch that all block a final integrate-and-verify ticket; green is promised only there.
+
+When the whole change fits a single fresh context window, the breakdown is one ticket with no blocking edges. Don't split it to make a set.
+
+### 4. Check the breakdown
+
+Nobody is there to approve the breakdown, so check it yourself. Write the proposed breakdown as a numbered list. For each ticket:
+
+- **Title**: short descriptive name
+- **Blocked by**: which other tickets (if any) must complete first
+- **What it delivers**: the end-to-end behaviour this ticket makes work
+
+Answer for yourself:
+
+- Is the granularity right? (too coarse / too fine)
+- Are the blocking edges correct: does each ticket only depend on tickets that genuinely gate it?
+- Should any tickets be merged or split further?
+
+Iterate until every answer holds.
+
+### 5. Publish the tickets to the issue tracker
+
+Publish one issue per ticket in dependency order (blockers first) so each ticket's blocking edges can reference real identifiers. Use the issue template below.
+
+The links are native, never only text in a body: thirdshift reads the tracker's own sub-issue and "blocked by" links to order the work, and ignores what a body says.
+
+- **Parent**: when the tickets come from a spec's issue, make each one a sub-issue of it: `gh api --method POST repos/<owner>/<repo>/issues/<spec>/sub_issues -F sub_issue_id=<ticket-db-id>`.
+- **Blocked by**: add each blocking edge as an issue dependency: `gh api --method POST repos/<owner>/<repo>/issues/<ticket>/dependencies/blocked_by -F issue_id=<blocker-db-id>`.
+
+Both take the issue's numeric **database id** (`gh api repos/<owner>/<repo>/issues/<n> --jq .id`), not its `#number` or `node_id`. Once published, read the links back and fix any that are missing.
+
+Apply the `ready-for-agent` triage label to each ticket of a spec; the tickets are agent-grabbable by construction. A single standalone ticket, with no parent, is the whole plan: label it `needs-triage` and nothing else, as thirdshift marks it ready once the session has ended and the plan passes its checks.
+
+Publish only: don't start work on any ticket.
+
+Do NOT close or modify any parent issue, beyond linking its sub-issues.
+
+<issue-template>
+
+## Parent
+
+A reference to the parent issue on the tracker (if the source was an existing issue, otherwise omit this section).
+
+## What to build
+
+The end-to-end behaviour this ticket makes work, from the user's perspective, not layer-by-layer implementation.
+
+## Acceptance criteria
+
+- [ ] Criterion 1
+- [ ] Criterion 2
+
+## Blocked by
+
+- A reference to each blocking ticket, or "None (can start immediately)".
+
+</issue-template>
+
+Avoid specific file paths or code snippets: they go stale fast. Exception: if a prototype produced a snippet that encodes a decision more precisely than prose can (state machine, reducer, schema, type shape), inline it and note briefly that it came from a prototype. Trim to the decision-rich parts, not a working demo, just the important bits.
+
+

to-tickets/agents/openai.yaml

+
interface:
+  display_name: "To Tickets"
+  short_description: "Split a plan into tracer-bullet tickets"
+policy:
+  allow_implicit_invocation: true
 
diff --git a/skills/codebase-design/DEEPENING.md b/skills/codebase-design/DEEPENING.md new file mode 100644 index 000000000..cd94075cf --- /dev/null +++ b/skills/codebase-design/DEEPENING.md @@ -0,0 +1,37 @@ +# Deepening + +How to deepen a cluster of shallow modules safely, given its dependencies. Assumes the vocabulary in [SKILL.md](SKILL.md): **module**, **interface**, **seam**, **adapter**. + +## Dependency categories + +When assessing a candidate for deepening, classify its dependencies. The category determines how the deepened module is tested across its seam. + +### 1. In-process + +Pure computation, in-memory state, no I/O. Always deepenable: merge the modules and test through the new interface directly. No adapter needed. + +### 2. Local-substitutable + +Dependencies that have local test stand-ins (PGLite for Postgres, in-memory filesystem). Deepenable if the stand-in exists. The deepened module is tested with the stand-in running in the test suite. The seam is internal; no port at the module's external interface. + +### 3. Remote but owned (Ports & Adapters) + +Your own services across a network boundary (microservices, internal APIs). Define a **port** (interface) at the seam. The deep module owns the logic; the transport is injected as an **adapter**. Tests use an in-memory adapter. Production uses an HTTP/gRPC/queue adapter. + +Recommendation shape: *"Define a port at the seam, implement an HTTP adapter for production and an in-memory adapter for testing, so the logic sits in one deep module even though it's deployed across a network."* + +### 4. True external (Mock) + +Third-party services (Stripe, Twilio, etc.) you don't control. The deepened module takes the external dependency as an injected port; tests provide a mock adapter. + +## Seam discipline + +- **One adapter means a hypothetical seam. Two adapters means a real one.** Don't introduce a port unless at least two adapters are justified (typically production + test). A single-adapter seam is just indirection. +- **Internal seams vs external seams.** A deep module can have internal seams (private to its implementation, used by its own tests) as well as the external seam at its interface. Don't expose internal seams through the interface just because tests use them. + +## Testing strategy: replace, don't layer + +- Old unit tests on shallow modules become waste once tests at the deepened module's interface exist; delete them. +- Write new tests at the deepened module's interface. The **interface is the test surface**. +- Tests assert on observable outcomes through the interface, not internal state. +- Tests should survive internal refactors, since they describe behaviour, not implementation. If a test has to change when the implementation changes, it's testing past the interface. diff --git a/skills/codebase-design/DESIGN-IT-TWICE.md b/skills/codebase-design/DESIGN-IT-TWICE.md new file mode 100644 index 000000000..a9eefffc6 --- /dev/null +++ b/skills/codebase-design/DESIGN-IT-TWICE.md @@ -0,0 +1,44 @@ +# Design It Twice + +To explore alternative interfaces for a chosen deepening candidate, use this parallel sub-agent pattern. Based on "Design It Twice" (Ousterhout): your first idea is unlikely to be the best. + +Uses the vocabulary in [SKILL.md](SKILL.md): **module**, **interface**, **seam**, **adapter**, **leverage**. + +## Process + +### 1. Frame the problem space + +Before spawning sub-agents, write down the problem space for the chosen candidate: + +- The constraints any new interface would need to satisfy +- The dependencies it would rely on, and which category they fall into (see [DEEPENING.md](DEEPENING.md)) +- A rough illustrative code sketch to ground the constraints, not a proposal, just a way to make the constraints concrete + +Nobody is there to read it, so don't wait: proceed to Step 2. It is what you compare the designs against in Step 3. + +### 2. Spawn sub-agents + +Spawn 3+ sub-agents in parallel. Each must produce a **radically different** interface for the deepened module. + +Prompt each sub-agent with a separate technical brief (file paths, coupling details, dependency category from [DEEPENING.md](DEEPENING.md), what sits behind the seam). The brief is independent of the problem-space explanation in Step 1. Give each agent a different design constraint: + +- Agent 1: "Minimize the interface: aim for 1–3 entry points max. Maximise leverage per entry point." +- Agent 2: "Maximise flexibility: support many use cases and extension." +- Agent 3: "Optimise for the most common caller: make the default case trivial." +- Agent 4 (if applicable): "Design around ports & adapters for cross-seam dependencies." + +Include both [SKILL.md](SKILL.md) vocabulary and CONTEXT.md vocabulary in the brief so each sub-agent names things consistently with the architecture language and the project's domain language. + +Each sub-agent outputs: + +1. Interface (types, methods, params, plus invariants, ordering, error modes) +2. Usage example showing how callers use it +3. What the implementation hides behind the seam +4. Dependency strategy and adapters (see [DEEPENING.md](DEEPENING.md)) +5. Trade-offs: where leverage is high, where it's thin + +### 3. Compare and decide + +Compare the designs in prose. Contrast by **depth** (leverage at the interface), **locality** (where change concentrates), and **seam placement**. + +After comparing, decide: which design is strongest and why. If elements from different designs would combine well, take a hybrid. Be opinionated: the choice is yours to make, and nobody picks from a menu. Record the decision and its reason, with the designs you turned down and why. diff --git a/skills/codebase-design/SKILL.md b/skills/codebase-design/SKILL.md new file mode 100644 index 000000000..37e47d19e --- /dev/null +++ b/skills/codebase-design/SKILL.md @@ -0,0 +1,114 @@ +--- +name: codebase-design +description: Shared vocabulary for designing deep modules. Use when designing or improving a module's interface, finding deepening opportunities, deciding where a seam goes, making code more testable or AI-navigable, or when another skill needs the deep-module vocabulary. +--- + +# Codebase Design + +Design **deep modules**: a lot of behaviour behind a small interface, placed at a clean seam, testable through that interface. Use this language and these principles wherever code is being designed or restructured. The aim is leverage for callers, locality for maintainers, and testability for everyone. + +## Glossary + +Use these terms exactly: don't substitute "component," "service," "API," or "boundary." Consistent language is the whole point. + +**Module**: anything with an interface and an implementation. Deliberately scale-agnostic: a function, class, package, or tier-spanning slice. _Avoid_: unit, component, service. + +**Interface**: everything a caller must know to use the module correctly: the type signature, but also invariants, ordering constraints, error modes, required configuration, and performance characteristics. _Avoid_: API, signature (too narrow, they refer only to the type-level surface). + +**Implementation**: what's inside a module, its body of code. Distinct from **Adapter**: a thing can be a small adapter with a large implementation (a Postgres repo) or a large adapter with a small implementation (an in-memory fake). Reach for "adapter" when the seam is the topic; "implementation" otherwise. + +**Depth**: leverage at the interface. The amount of behaviour a caller (or test) can exercise per unit of interface they have to learn. A module is **deep** when a large amount of behaviour sits behind a small interface, **shallow** when the interface is nearly as complex as the implementation. + +**Seam** _(Michael Feathers)_: a place where you can alter behaviour without editing in that place; the *location* at which a module's interface lives. Where to put the seam is its own design decision, distinct from what goes behind it. _Avoid_: boundary (overloaded with DDD's bounded context). + +**Adapter**: a concrete thing that satisfies an interface at a seam. Describes *role* (what slot it fills), not substance (what's inside). + +**Leverage**: what callers get from depth. More capability per unit of interface they learn. One implementation pays back across N call sites and M tests. + +**Locality**: what maintainers get from depth. Change, bugs, knowledge, and verification concentrate in one place rather than spreading across callers. Fix once, fixed everywhere. + +## Deep vs shallow + +**Deep module** = small interface + lots of implementation: + +``` +┌─────────────────────┐ +│ Small Interface │ ← Few methods, simple params +├─────────────────────┤ +│ │ +│ Deep Implementation│ ← Complex logic hidden +│ │ +└─────────────────────┘ +``` + +**Shallow module** = large interface + little implementation (avoid): + +``` +┌─────────────────────────────────┐ +│ Large Interface │ ← Many methods, complex params +├─────────────────────────────────┤ +│ Thin Implementation │ ← Just passes through +└─────────────────────────────────┘ +``` + +When designing an interface, ask: + +- Can I reduce the number of methods? +- Can I simplify the parameters? +- Can I hide more complexity inside? + +## Principles + +- **Depth is a property of the interface, not the implementation.** A deep module can be internally composed of small, mockable, swappable parts; they just aren't part of the interface. A module can have **internal seams** (private to its implementation, used by its own tests) as well as the **external seam** at its interface. +- **The deletion test.** Imagine deleting the module. If complexity vanishes, it was a pass-through. If complexity reappears across N callers, it was earning its keep. +- **The interface is the test surface.** Callers and tests cross the same seam. If you want to test *past* the interface, the module is probably the wrong shape. +- **One adapter means a hypothetical seam. Two adapters means a real one.** Don't introduce a seam unless something actually varies across it. + +## Designing for testability + +Good interfaces make testing natural: + +1. **Accept dependencies, don't create them.** + + ```typescript + // Testable + function processOrder(order, paymentGateway) {} + + // Hard to test + function processOrder(order) { + const gateway = new StripeGateway(); + } + ``` + +2. **Return results, don't produce side effects.** + + ```typescript + // Testable + function calculateDiscount(cart): Discount {} + + // Hard to test + function applyDiscount(cart): void { + cart.total -= discount; + } + ``` + +3. **Small surface area.** Fewer methods = fewer tests needed. Fewer params = simpler test setup. + +## Relationships + +- A **Module** has exactly one **Interface** (the surface it presents to callers and tests). +- **Depth** is a property of a **Module**, measured against its **Interface**. +- A **Seam** is where a **Module**'s **Interface** lives. +- An **Adapter** sits at a **Seam** and satisfies the **Interface**. +- **Depth** produces **Leverage** for callers and **Locality** for maintainers. + +## Rejected framings + +- **Depth as ratio of implementation-lines to interface-lines** (Ousterhout): rewards padding the implementation. We use depth-as-leverage instead. +- **"Interface" as the TypeScript `interface` keyword or a class's public methods**: too narrow: interface here includes every fact a caller must know. +- **"Boundary"**: overloaded with DDD's bounded context. Say **seam** or **interface**. + +## Going deeper + +- **Deepening a cluster given its dependencies**, see [DEEPENING.md](DEEPENING.md): dependency categories, seam discipline, and replace-don't-layer testing. +- **Exploring alternative interfaces**, see [DESIGN-IT-TWICE.md](DESIGN-IT-TWICE.md): spin up parallel sub-agents to design the interface several radically different ways, then compare on depth, locality, and seam placement. diff --git a/skills/codebase-design/agents/openai.yaml b/skills/codebase-design/agents/openai.yaml new file mode 100644 index 000000000..3180715ed --- /dev/null +++ b/skills/codebase-design/agents/openai.yaml @@ -0,0 +1,3 @@ +interface: + display_name: "Codebase Design" + short_description: "Vocabulary for deep-module design" diff --git a/skills/improve-codebase-architecture/REPORT.md b/skills/improve-codebase-architecture/REPORT.md new file mode 100644 index 000000000..44224c032 --- /dev/null +++ b/skills/improve-codebase-architecture/REPORT.md @@ -0,0 +1,111 @@ +# Report Format + +The Architecture review's report is Markdown, written into the issue it publishes: the plan's top issue, or the idea issue. GitHub renders Mermaid in a fenced `mermaid` block, so the diagrams live where the issue is read. Write no file, and open nothing. + +The report is one candidate's card: the top recommendation's. The candidates that weren't chosen get no card. + +## Candidate card + +The diagrams carry the weight. Prose is sparse, plain, and uses the glossary terms (from the `/thirdshift:codebase-design` skill) without ceremony. + +```md +**Strength**: Strong · **Dependencies**: in-process + +**Files**: `src/order/handler.rs`, `src/order/validator.rs`, `src/order/repo.rs` + +**Before** + + + +**After** + + + +**Problem**: one sentence. What hurts. + +**Solution**: one sentence. What changes. + +**Wins** + +- Tests hit one interface +- Pricing logic stops leaking + +> [!WARNING] +> Contradicts ADR-0007, but worth reopening because… +``` + +- **Strength**: the recommendation strength (`Strong`, `Worth exploring` or `Speculative`), plus the dependency category (`in-process`, `local-substitutable`, `ports & adapters`, `mock`). On an idea issue, add one line saying what stopped it being `Strong`. +- **Files**: a list in code spans. The report is the one place file paths belong; the rest of a Spec or Ticket leaves them out. +- **Before / After diagram**: the centrepiece. One Mermaid block each, the same modules in both so the eye can compare. See patterns below. +- **Problem**: one sentence. What hurts. +- **Solution**: one sentence. What changes. +- **Wins**: bullets, ≤6 words each. e.g. "Tests hit one interface", "Pricing logic stops leaking", "Delete 4 shallow wrappers". +- **ADR callout** (if applicable): one line in a `> [!WARNING]` alert. + +No paragraphs of explanation. If the diagram needs a paragraph to be understood, redraw the diagram. + +## Diagram patterns + +Pick the pattern that fits the candidate. Before and after use the same pattern. + +### Flowchart (the workhorse for dependencies / call flow) + +Use a Mermaid `flowchart` when the point is "X calls Y calls Z, and look at the mess." Style with `classDef` to colour leakage edges red and give the deep module a thick border. A dashed edge is leakage across a seam. + +````md +```mermaid +flowchart LR + A[OrderHandler] --> B[OrderValidator] + B --> C[OrderRepo] + C -.leak.-> D[PricingClient] + classDef leak stroke:#dc2626,stroke-width:2px; + class C,D leak +``` +```` + +### Sequence diagram (good for round-trips) + +Use a `sequenceDiagram` for "before: 6 round-trips; after: 1." + +### Subgraph collapse (good for layered shallowness and call-graph collapse) + +Before: the shallow modules as separate nodes, each call an edge. After: one `subgraph` labelled with the consolidated responsibility, the now-internal calls inside it, and one edge in from the caller. + +````md +```mermaid +flowchart LR + Caller --> Intake + subgraph Intake[Order intake] + direction LR + validate --> price --> store + end + classDef deep stroke-width:3px; + class Intake deep +``` +```` + +## Style guidance + +- Keep each diagram small: a dozen nodes at most. Leave out whatever doesn't change between before and after. +- Colour sparingly: red for leakage, a thick border for the deep module, nothing else. +- Label nodes with the module's name in the domain language, not a sentence. +- Check that each block is valid Mermaid before publishing: a diagram that doesn't render is worse than none. + +## Tone + +Plain English, concise, but the architectural nouns and verbs come straight from the `/thirdshift:codebase-design` skill. Concision is not an excuse to drift. + +**Use exactly:** module, interface, implementation, depth, deep, shallow, seam, adapter, leverage, locality. + +**Never substitute:** component, service, unit (for module) · API, signature (for interface) · boundary (for seam) · layer, wrapper (for module, when you mean module). + +**Phrasings that fit the style:** + +- "Order intake module is shallow: interface nearly matches the implementation." +- "Pricing leaks across the seam." +- "Deepen: one interface, one place to test." +- "Two adapters justify the seam: HTTP in prod, in-memory in tests." + +**Wins bullets** name the gain in glossary terms: *"locality: bugs concentrate in one module"*, *"leverage: one interface, N call sites"*, *"interface shrinks; implementation absorbs the wrappers"*. Don't write *"easier to maintain"* or *"cleaner code"*, because those terms aren't in the glossary and don't earn their place. + +No hedging, no throat-clearing, no "it's worth noting that…". If a sentence could be a bullet, make it a bullet. If a bullet could be cut, cut it. If a term isn't in the `/thirdshift:codebase-design` glossary, reach for one that is before inventing a new one. diff --git a/skills/improve-codebase-architecture/SKILL.md b/skills/improve-codebase-architecture/SKILL.md new file mode 100644 index 000000000..571918d39 --- /dev/null +++ b/skills/improve-codebase-architecture/SKILL.md @@ -0,0 +1,131 @@ +--- +name: improve-codebase-architecture +description: "Scan a codebase for deepening opportunities, take the top recommendation, and publish it to the issue tracker: as a plan when it is Strong, as an idea otherwise. Only for an Architecture review." +disable-model-invocation: false +--- + +# Improve Codebase Architecture + +Surface architectural friction and propose **deepening opportunities**: refactors that turn shallow modules into deep ones. The aim is testability and AI-navigability. + +This is an **Architecture review**. It runs headless: nobody reads a report, picks a candidate or answers a question. Never open a browser, ask a question or wait for an answer. Every decision is yours, and each one is written down with its reason. + +It changes nothing in the repository. Never commit, push, create a branch or open a pull request, and never edit `CONTEXT.md` or an ADR: any change the refactor needs to them is work for a Ticket. The worktree is thrown away when the session ends, so a throwaway prototype in it is fine, and nothing in it survives. What the review produces is issues on the issue tracker. + +This command is _informed_ by the project's domain model and built on a shared design vocabulary: + +- Call the Skill tool with "thirdshift:codebase-design" for the architecture vocabulary (**module**, **interface**, **depth**, **seam**, **adapter**, **leverage**, **locality**) and its principles (the deletion test, "the interface is the test surface", "one adapter = hypothetical seam, two = real"). Use these terms exactly in every suggestion, and don't drift into "component," "service," "API," or "boundary." +- The domain language in `CONTEXT.md` gives names to good seams; ADRs in `docs/adr/` record decisions this command should not re-litigate. + +The issue tracker and triage label vocabulary should have been provided to you. If `docs/agents/issue-tracker.md` is missing, fall back to the `gh` CLI. If `docs/agents/triage-labels.md` is missing, use the label names as written here, creating a label the tracker lacks (`gh label create`). + +## Process + +### 1. Explore + +**Scope before you scan: YAGNI.** Deepening a module pays off by making future changes to it easier, so put extra weight on the parts of the codebase that have recently changed. Decide *where* to look before you look: + +- If the Session prompt named a focus (a module, a subsystem, a pain point), take it, and skip the inference below. +- Otherwise, walk back a good stretch of the commit history (`git log --oneline`) to find the codebase's hot spots, the files and areas that keep coming up, and let those paths pull your attention first. If the changes are scattered with no clear hot spot, widen the net. + +Read the project's domain glossary (`CONTEXT.md`) and any ADRs in the area you're touching first. + +Read the open issues too, titles and bodies: Specs, Tickets and ideas alike, whatever their labels. They are what a candidate may already be covered by. + +Then spawn a sub-agent to walk the codebase. Don't follow rigid heuristics; explore organically and note where you experience friction: + +- Where does understanding one concept require bouncing between many small modules? +- Where are modules **shallow**, with an interface nearly as complex as the implementation? +- Where have pure functions been extracted just for testability, but the real bugs hide in how they're called (no **locality**)? +- Where do tightly-coupled modules leak across their seams? +- Which parts of the codebase are untested, or hard to test through their current interface? + +Apply the **deletion test** to anything you suspect is shallow: would deleting it concentrate complexity, or just move it? A "yes, concentrates" is the signal you want. + +### 2. Take the top recommendation + +Weigh the candidates in the session. Write no report file. For each candidate, settle: + +- **Files**: which files/modules are involved +- **Problem**: why the current architecture is causing friction +- **Solution**: plain English description of what would change +- **Benefits**: explained in terms of locality and leverage, and how tests would improve +- **Recommendation strength**: one of `Strong`, `Worth exploring`, `Speculative` +- **Covered by**: the open issue that already covers it, if one does + +A candidate is **Strong** only when all of these hold: + +- The friction is real and you can point to it in the code. +- The deepening passes the deletion test. +- You can settle its whole design from the code, `CONTEXT.md` and the ADRs. A candidate with a decision only a human can make is `Worth exploring` at most. +- It contradicts no ADR, or the friction is real enough to warrant reopening that ADR. + +A candidate is **covered** when an open issue proposes the same deepening of the same modules, whether as a Spec, a Ticket or an idea. An issue that only touches the same files doesn't cover it. + +Rank the candidates: which you'd tackle first and why. Then take the **top recommendation**: + +1. The highest-ranked `Strong` candidate that no open issue covers. It becomes the plan: go on to step 3. +2. If there is none, the highest-ranked candidate of all. If an open issue covers it, file nothing: go to step 6 and name that issue. Otherwise it becomes an idea: go to step 5. + +Every other candidate is dropped. Don't file it, and don't list it in the plan or the idea: the next Architecture review starts from the codebase as it is by then. + +If the scan surfaced no candidate at all, file nothing, and say so plainly in place of the final line of step 6: there is no issue for it to name. + +**Use CONTEXT.md vocabulary for the domain, and the `/thirdshift:codebase-design` vocabulary for the architecture.** If `CONTEXT.md` defines "Order," talk about "the Order intake module," not "the FooBarHandler," and not "the Order service." + +**ADR conflicts**: if a candidate contradicts an existing ADR, only keep it when the friction is real enough to warrant revisiting the ADR. Mark it clearly in the report (e.g. a warning callout: _"contradicts ADR-0007, but worth reopening because…"_). Don't raise every theoretical refactor an ADR forbids. + +Do NOT propose interfaces yet. + +### 3. Settle the design + +Only for a `Strong` top recommendation. + +Map the decisions as a **design tree**: every decision branches into the decisions that hang off it. Walk it: constraints, dependencies, the shape of the deepened module, what sits behind the seam, what tests survive. + +Work the tree in **rounds**. The **frontier** is every decision whose prerequisites are already settled. Settle the whole frontier yourself, then recompute it. Finding _facts_ is a lookup: when a decision needs a fact from the environment (filesystem, tools, etc.), dispatch a sub-agent to find it. The _decisions_ are yours too. For each one, write down what you decided and why. The design is settled when the frontier is empty: every branch of the design tree visited, nothing left silently assumed. + +If a decision turns out to need a human, the candidate was not `Strong`: make it `Worth exploring` and take the top recommendation again, from step 2. + +Side effects are recorded, never written: + +- **Naming a deepened module after a concept not in `CONTEXT.md`?** Record the term and its definition as work for a Ticket. +- **Sharpening a fuzzy term as you decide?** Record the sharper definition as work for a Ticket. +- **Reopening an ADR, or making a decision worth one?** Record the new or changed ADR as work for a Ticket. A decision is worth an ADR only when it is hard to reverse, surprising without context, and the result of a real trade-off. +- **Want to explore alternative interfaces for the deepened module?** Call the Skill tool with "thirdshift:codebase-design" and use its design-it-twice parallel sub-agent pattern. + +### 4. Publish the plan + +Only for a `Strong` top recommendation whose design is settled. + +Decide the plan's size. If one session, a single fresh context window, is enough for the whole change, the plan is a single **Ticket**. Otherwise it is a **Spec** with **Tickets**. Don't force a small refactor into a Spec. + +- **A Spec with Tickets**: call the Skill tool with "thirdshift:to-spec" to publish the Spec, then with "thirdshift:to-tickets" to publish its Tickets as the Spec's sub-issues. +- **A single Ticket**: call the Skill tool with "thirdshift:to-tickets" to publish one standalone Ticket, with no parent. + +The plan's top issue, the Spec or the single Ticket, is labelled `needs-triage` and nothing else. thirdshift marks it ready once the session has ended and the plan passes its checks; you never do. Tickets under a Spec are labelled `ready-for-agent`. + +The plan's top issue carries, whichever shape it takes: + +- **Every decision from step 3, each with its reason.** In a Spec, under Implementation Decisions and Testing Decisions. In a single Ticket, under a `## Decisions` heading added after the issue template's sections. +- **The report**: the candidate's card as Markdown with a Mermaid before/after diagram. In a Spec, under Further Notes. In a single Ticket, under an `## Architecture review` heading added last, after `## Decisions`. See [REPORT.md](REPORT.md) for the card. + +The `CONTEXT.md` and ADR changes recorded in step 3 go into the acceptance criteria of the Ticket whose work they belong to, a Spec's Ticket or the single Ticket, so they land through its pull request. + +### 5. File the idea + +Only for a top recommendation that is not `Strong` and that no open issue covers. + +Publish one issue to the issue tracker, labelled `needs-triage`, for a human to flesh out. Its title names the deepening. Its body is the report: the candidate's card as Markdown with a Mermaid before/after diagram, its recommendation strength, and what stopped it being `Strong`. See [REPORT.md](REPORT.md) for the card. + +Publish nothing else: no Spec, no Tickets, and no second idea. + +### 6. End with the final line + +Unless the scan surfaced no candidate at all, end your final message with the one final line the Session prompt asks for, in exactly the format it gives. The format is defined there and nowhere else. The line names one issue: + +- the plan's top issue, when you published a plan; +- the idea issue, when you filed one; +- the open issue that already covers the top recommendation, when you filed nothing. + +thirdshift reads only that line, so everything else you want read belongs in the issue. diff --git a/skills/improve-codebase-architecture/agents/openai.yaml b/skills/improve-codebase-architecture/agents/openai.yaml new file mode 100644 index 000000000..b7184f856 --- /dev/null +++ b/skills/improve-codebase-architecture/agents/openai.yaml @@ -0,0 +1,5 @@ +interface: + display_name: "Improve Codebase Architecture" + short_description: "Find and publish the top architecture improvement" +policy: + allow_implicit_invocation: true diff --git a/skills/to-spec/SKILL.md b/skills/to-spec/SKILL.md new file mode 100644 index 000000000..3fc5ec829 --- /dev/null +++ b/skills/to-spec/SKILL.md @@ -0,0 +1,77 @@ +--- +name: to-spec +description: "Turn the current session into a spec and publish it to the project issue tracker: no interview, just synthesis of what you've already settled. Only for an Architecture review." +disable-model-invocation: false +--- + +This skill takes the current session context and codebase understanding and produces a spec. Nobody is there to interview or to ask; just synthesize what you already know. + +The issue tracker and triage label vocabulary should have been provided to you. If `docs/agents/issue-tracker.md` is missing, fall back to `gh issue create`. If `docs/agents/triage-labels.md` is missing, use the label names as written here, creating a label the tracker lacks (`gh label create`). + +The spec is all this skill writes. Write nothing to the repository: no spec file, commits or branches. A `CONTEXT.md` or ADR change the spec needs is work for one of its tickets. + +## Process + +1. Explore the repo to understand the current state of the codebase, if you haven't already. Use the project's domain glossary vocabulary throughout the spec, and respect any ADRs in the area you're touching. + +2. Sketch out the seams at which the feature will be tested. Existing seams should be preferred to new ones. Use the highest seam possible. If new seams are needed, propose them at the highest point you can. The fewer seams across the codebase, the better - the ideal number is one. + +Decide the seams yourself and record them, with the reason for each, under Testing Decisions. + +3. Write the spec using the template below, then publish it to the project issue tracker. Apply the `needs-triage` triage label and no other: thirdshift marks the spec ready once the session has ended and the plan passes its checks. + + + +## Problem Statement + +The problem that the user is facing, from the user's perspective. + +## Solution + +The solution to the problem, from the user's perspective. + +## User Stories + +A LONG, numbered list of user stories. Each user story should be in the format of: + +1. As an , I want a , so that + + +1. As a mobile bank customer, I want to see balance on my accounts, so that I can make better informed decisions about my spending + + +This list of user stories should be extremely extensive and cover all aspects of the feature. + +## Implementation Decisions + +A list of implementation decisions that were made, each with its reason. This can include: + +- The modules that will be built/modified +- The interfaces of those modules that will be modified +- Architectural decisions +- Schema changes +- API contracts +- Specific interactions + +Do NOT include specific file paths or code snippets. They may end up being outdated very quickly. + +Exception: if a prototype produced a snippet that encodes a decision more precisely than prose can (state machine, reducer, schema, type shape), inline it within the relevant decision and note briefly that it came from a prototype. Trim to the decision-rich parts, not a working demo, just the important bits. + +## Testing Decisions + +A list of testing decisions that were made, each with its reason. Include: + +- A description of what makes a good test (only test external behavior, not implementation details) +- The seams the feature will be tested at +- Which modules will be tested +- Prior art for the tests (i.e. similar types of tests in the codebase) + +## Out of Scope + +A description of the things that are out of scope for this spec. + +## Further Notes + +Any further notes about the feature. + + diff --git a/skills/to-spec/agents/openai.yaml b/skills/to-spec/agents/openai.yaml new file mode 100644 index 000000000..0f8210743 --- /dev/null +++ b/skills/to-spec/agents/openai.yaml @@ -0,0 +1,5 @@ +interface: + display_name: "To Spec" + short_description: "Turn a session into a spec" +policy: + allow_implicit_invocation: true diff --git a/skills/to-tickets/SKILL.md b/skills/to-tickets/SKILL.md new file mode 100644 index 000000000..a15acbe9e --- /dev/null +++ b/skills/to-tickets/SKILL.md @@ -0,0 +1,100 @@ +--- +name: to-tickets +description: Break a plan, spec, or the current session into a set of tracer-bullet tickets, each declaring its blocking edges, published to the issue tracker with native sub-issue and blocking links. Only for an Architecture review. +disable-model-invocation: false +--- + +# To Tickets + +Break a plan, spec, or session into a set of **tickets**: tracer-bullet vertical slices, each declaring the tickets that **block** it. + +The issue tracker and triage label vocabulary should have been provided to you. If `docs/agents/issue-tracker.md` is missing, fall back to the `gh` CLI. If `docs/agents/triage-labels.md` is missing, use the label names as written here, creating a label the tracker lacks (`gh label create`). + +The tickets are all this skill writes. Write nothing to the repository: no ticket files, commits or branches. A `CONTEXT.md` or ADR change the work needs is part of a ticket's work, named in its acceptance criteria. + +## Process + +### 1. Gather context + +Work from whatever is already in the session context. If you were given a reference (an issue number or URL) as an argument, fetch it and read its full body and comments. + +### 2. Explore the codebase (optional) + +If you have not already explored the codebase, do so to understand the current state of the code. Ticket titles and descriptions should use the project's domain glossary vocabulary, and respect ADRs in the area you're touching. + +Look for opportunities to prefactor the code to make the implementation easier. "Make the change easy, then make the easy change." + +### 3. Draft vertical slices + +Break the work into **tracer bullet** tickets. + + + +- Each slice cuts a narrow but COMPLETE path through every layer (schema, API, UI, tests): vertical, NOT a horizontal slice of one layer +- A completed slice is demoable or verifiable on its own +- Each slice is sized to fit in a single fresh context window +- Any prefactoring should be done first + + + +Give each ticket its **blocking edges**: the other tickets that must complete before it can start. A ticket with no blockers can start immediately. + +**Wide refactors are the exception to vertical slicing.** A **wide refactor** is one mechanical change (rename a column, retype a shared symbol) whose **blast radius** fans across the whole codebase, so a single edit breaks thousands of call sites at once and no vertical slice can land green. Don't force it into a tracer bullet; sequence it as **expand–contract**. First expand: add the new form beside the old so nothing breaks. Then migrate the call sites over in batches sized by blast radius (per package, per directory), each batch its own ticket blocked by the expand, keeping CI green batch to batch because the old form still exists. Finally contract: delete the old form once no caller remains, in a ticket blocked by every migrate batch. When even the batches can't stay green alone, keep the sequence but let them share an integration branch that all block a final integrate-and-verify ticket; green is promised only there. + +When the whole change fits a single fresh context window, the breakdown is one ticket with no blocking edges. Don't split it to make a set. + +### 4. Check the breakdown + +Nobody is there to approve the breakdown, so check it yourself. Write the proposed breakdown as a numbered list. For each ticket: + +- **Title**: short descriptive name +- **Blocked by**: which other tickets (if any) must complete first +- **What it delivers**: the end-to-end behaviour this ticket makes work + +Answer for yourself: + +- Is the granularity right? (too coarse / too fine) +- Are the blocking edges correct: does each ticket only depend on tickets that genuinely gate it? +- Should any tickets be merged or split further? + +Iterate until every answer holds. + +### 5. Publish the tickets to the issue tracker + +Publish one issue per ticket in dependency order (blockers first) so each ticket's blocking edges can reference real identifiers. Use the issue template below. + +The links are native, never only text in a body: thirdshift reads the tracker's own sub-issue and "blocked by" links to order the work, and ignores what a body says. + +- **Parent**: when the tickets come from a spec's issue, make each one a sub-issue of it: `gh api --method POST repos///issues//sub_issues -F sub_issue_id=`. +- **Blocked by**: add each blocking edge as an issue dependency: `gh api --method POST repos///issues//dependencies/blocked_by -F issue_id=`. + +Both take the issue's numeric **database id** (`gh api repos///issues/ --jq .id`), not its `#number` or `node_id`. Once published, read the links back and fix any that are missing. + +Apply the `ready-for-agent` triage label to each ticket of a spec; the tickets are agent-grabbable by construction. A single standalone ticket, with no parent, is the whole plan: label it `needs-triage` and nothing else, as thirdshift marks it ready once the session has ended and the plan passes its checks. + +Publish only: don't start work on any ticket. + +Do NOT close or modify any parent issue, beyond linking its sub-issues. + + + +## Parent + +A reference to the parent issue on the tracker (if the source was an existing issue, otherwise omit this section). + +## What to build + +The end-to-end behaviour this ticket makes work, from the user's perspective, not layer-by-layer implementation. + +## Acceptance criteria + +- [ ] Criterion 1 +- [ ] Criterion 2 + +## Blocked by + +- A reference to each blocking ticket, or "None (can start immediately)". + + + +Avoid specific file paths or code snippets: they go stale fast. Exception: if a prototype produced a snippet that encodes a decision more precisely than prose can (state machine, reducer, schema, type shape), inline it and note briefly that it came from a prototype. Trim to the decision-rich parts, not a working demo, just the important bits. diff --git a/skills/to-tickets/agents/openai.yaml b/skills/to-tickets/agents/openai.yaml new file mode 100644 index 000000000..dbfd91609 --- /dev/null +++ b/skills/to-tickets/agents/openai.yaml @@ -0,0 +1,5 @@ +interface: + display_name: "To Tickets" + short_description: "Split a plan into tracer-bullet tickets" +policy: + allow_implicit_invocation: true diff --git a/src/architect.rs b/src/architect.rs new file mode 100644 index 000000000..03ab53afe --- /dev/null +++ b/src/architect.rs @@ -0,0 +1,303 @@ +//! An Architect run up to its plan: an Architecture review of the Base +//! branch, from the Launch directory with no Issue URL, then the checks on +//! the plan it published and the label swap that marks the plan ready, for +//! the command to stop at or to dispatch. A review that found no Strong +//! candidate published no plan, and the Architect run ends on the issue it +//! named instead: the idea issue it filed for its top recommendation, or the +//! open issue that already covers it. + +use std::fmt; +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result, anyhow, bail}; +use chrono::{DateTime, Utc}; + +use crate::failed_run::FailedRun; +use crate::git::Git; +use crate::github; +use crate::interrupt; +use crate::issue::{IssueUrl, Repo}; +use crate::plugin::Plugin; +use crate::preflight; +use crate::progress; +use crate::prompt; +use crate::run; +use crate::session::{Logs, Sessions}; +use crate::spec_run::{self, NEEDS_TRIAGE, READY_FOR_AGENT}; +use crate::worktree::ReviewWorktree; + +/// The Architecture review session's kind, in its progress lines and log +/// name. +const REVIEW: &str = "architecture-review"; + +/// How an Architect run ended, short of a failure, with the issue it ended +/// on. Its `Display` is the line that says how it ended, naming that issue. +#[derive(Debug)] +pub enum Outcome { + /// The plan the review published is marked ready. + PlanReady(IssueUrl), + /// The review found no Strong candidate, and filed its top + /// recommendation as this issue. + IdeaFiled(IssueUrl), + /// The review found no Strong candidate, and filed nothing: this open + /// issue already covers its top recommendation. + AlreadyFiled(IssueUrl), +} + +impl Outcome { + /// How the Architecture review ended, as the Architect run's Run + /// notification says it. + pub fn review(&self) -> &'static str { + match self { + Self::PlanReady(_) => "plan published", + Self::IdeaFiled(_) => "idea filed", + Self::AlreadyFiled(_) => "idea already filed", + } + } + + /// The URL of the issue the Architect run ended on. + pub fn url(&self) -> &str { + match self { + Self::PlanReady(issue) | Self::IdeaFiled(issue) | Self::AlreadyFiled(issue) => { + &issue.url + } + } + } +} + +impl fmt::Display for Outcome { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let url = self.url(); + match self { + Self::PlanReady(_) => write!(f, "plan {url} is ready for an agent"), + Self::IdeaFiled(_) => write!( + f, + "no Strong candidate: the Architecture review filed the idea {url}" + ), + Self::AlreadyFiled(_) => write!( + f, + "no Strong candidate: {url} already covers the Architecture review's top recommendation, so it filed nothing" + ), + } + } +} + +/// Run an Architecture review of the branch checked out in the Launch +/// directory, the Base branch, pointed at `focus` if given, and mark the plan +/// it publishes ready. A review that reports an idea issue it filed, or the +/// open issue that already covers its top recommendation, instead of a plan, +/// changes no label. With `launch_pull`, the Launch directory's checkout of +/// the Base branch is first brought up to date with origin. The review's +/// worktree and the plugin directory are gone when this returns. A failure +/// after the plan is published leaves its labels as the review left them. +pub fn run(focus: Option<&str>, logs_dir: &Path, launch_pull: bool) -> Result { + let started = Utc::now(); + let timestamp = started.format("%Y%m%dT%H%M%SZ").to_string(); + let (launch, origin, repo) = launch()?; + preflight::check_identity(&launch)?; + let base = launch + .run(&["symbolic-ref", "--quiet", "--short", "HEAD"]) + .ok() + .context("HEAD is detached; check out the branch the Architecture review should scan")?; + preflight::check_base_branch(&launch, &base)?; + if launch_pull { + run::pull_base_branch(&launch, Some(&base), &base); + } + + if interrupt::requested() { + return Err(anyhow!("interrupted").into()); + } + let worktree = ReviewWorktree::create(&launch, &repo.name, &base)?; + let logs = Logs::of_architect_run(&repo, logs_dir, ×tamp); + let mut log = logs.path(REVIEW); + review(worktree, &base, focus, &logs, &mut log) + .and_then(|final_message| conclude(final_message.as_deref(), &origin, started)) + .map_err(|error| FailedRun { + log: log.exists().then_some(log), + ..FailedRun::from(error) + }) +} + +/// The Launch directory, the URL of its `origin`, and the GitHub repository +/// that names: an Architect run's, as it has no Issue URL to name one. +fn launch() -> Result<(Git, String, Repo)> { + let launch = Git::new(std::env::current_dir().context("no current directory")?); + let origin = launch.run(&["config", "remote.origin.url"])?; + let repo = Repo::of_origin(&origin) + .with_context(|| format!("origin {origin} is not a GitHub repository"))?; + Ok((launch, origin, repo)) +} + +/// The repository an Architect run started from the Launch directory is on. +pub fn repo() -> Result { + launch().map(|(_, _, repo)| repo) +} + +/// The Architecture review session in `worktree`, which is removed once the +/// session ends. Returns the session's final message. `log` is left at the +/// most recent session's log. +fn review( + worktree: ReviewWorktree, + base: &str, + focus: Option<&str>, + logs: &Logs, + log: &mut PathBuf, +) -> Result> { + let plugin = Plugin::write()?; + let sessions = Sessions { + logs, + worktree: worktree.path(), + plugin_dir: plugin.path(), + }; + match focus { + Some(focus) => progress::step(format_args!( + "starting the Architecture review of {base}, focused on: {focus}" + )), + None => progress::step(format_args!("starting the Architecture review of {base}")), + } + sessions.run_to_final_message(REVIEW, &prompt::architecture_review(base, focus), log) +} + +/// What an Architecture review reported in the last line of its final +/// message. +#[derive(Debug, PartialEq, Eq)] +enum Report { + /// The plan it published: a Spec, or a single Ticket. + Plan(IssueUrl), + /// The issue it filed for a top recommendation that is not Strong. + Idea(IssueUrl), + /// The open issue that already covers that recommendation. + AlreadyFiled(IssueUrl), +} + +impl Report { + /// Read the last line of `final_message` that isn't blank. `None` unless + /// it is one of the lines the Architecture review prompt asks for, with + /// an Issue URL and nothing else after it. + fn read(final_message: &str) -> Option { + let line = final_message + .lines() + .map(str::trim) + .rfind(|line| !line.is_empty())?; + let naming = |start: &str, kind: fn(IssueUrl) -> Report| { + let url = line.strip_prefix(start)?; + IssueUrl::parse(url).ok().map(kind) + }; + naming(prompt::PLAN_LINE, Report::Plan) + .or_else(|| naming(prompt::IDEA_LINE, Report::Idea)) + .or_else(|| naming(prompt::ALREADY_FILED_LINE, Report::AlreadyFiled)) + } +} + +/// End the Architect run on the issue the last line of the review's +/// `final_message` names: a plan is marked ready, and an idea issue, or the +/// issue that already covers the top recommendation, is left as it is. Fails +/// if the session had no final message, if its last line is not one the +/// prompt asks for, or if the plan can't be marked ready. +fn conclude(final_message: Option<&str>, origin: &str, started: DateTime) -> Result { + match final_message.and_then(Report::read) { + Some(Report::Plan(plan)) => { + mark_plan_ready(&plan, origin, started)?; + Ok(Outcome::PlanReady(plan)) + } + Some(Report::Idea(idea)) => Ok(Outcome::IdeaFiled(idea)), + Some(Report::AlreadyFiled(issue)) => Ok(Outcome::AlreadyFiled(issue)), + None => bail!("the Architecture review ended without the final line its prompt asks for"), + } +} + +/// Mark `plan` ready: check it, then swap its `needs-triage` for +/// `ready-for-agent`, in one request. Fails, changing no label, if the plan +/// fails its checks. +fn mark_plan_ready(plan: &IssueUrl, origin: &str, started: DateTime) -> Result<()> { + progress::step(format_args!( + "the Architecture review published the plan {}", + plan.url + )); + let kept = labels_to_keep(plan, origin, started)?; + if interrupt::requested() { + bail!("interrupted"); + } + progress::step(format_args!( + "marking the plan ready: swapping {NEEDS_TRIAGE} for {READY_FOR_AGENT} on #{}", + plan.number + )); + let mut labels: Vec<&str> = kept + .iter() + .map(String::as_str) + .filter(|&label| label != READY_FOR_AGENT) + .collect(); + labels.push(READY_FOR_AGENT); + github::set_labels(plan, &labels) +} + +/// The labels `plan` keeps once it is marked ready: all it has but +/// `needs-triage`. Fails unless the plan passes its checks: it is in the +/// repository at `origin`, open, created since the Architect run `started`, +/// and has no other label that makes an Unready Ticket. +fn labels_to_keep(plan: &IssueUrl, origin: &str, started: DateTime) -> Result> { + if !plan.matches_origin(origin) { + bail!( + "the plan {} is not in the repository at origin {origin}", + plan.url + ); + } + let issue = github::issue(plan)?; + if !issue.is_open { + bail!("the plan {} is closed", plan.url); + } + // GitHub's times are to the second. + if issue.created.timestamp() < started.timestamp() { + bail!( + "the plan {} was created before this Architect run started", + plan.url + ); + } + let mut kept = issue.labels; + kept.retain(|label| label != NEEDS_TRIAGE); + if let Some(unready) = spec_run::unready_label(&kept) { + bail!("the plan {} is labelled {unready}", plan.url); + } + Ok(kept) +} + +#[cfg(test)] +mod tests { + use super::*; + + const URL: &str = "https://github.com/acme/widgets/issues/8"; + + fn issue() -> IssueUrl { + IssueUrl::parse(URL).unwrap() + } + + #[test] + fn the_report_is_the_last_line_whichever_of_the_three_it_is() { + for (line, report) in [ + ("Architecture review plan: ", Report::Plan(issue())), + ("Architecture review idea: ", Report::Idea(issue())), + ( + "Architecture review already filed: ", + Report::AlreadyFiled(issue()), + ), + ] { + let message = format!("Reviewed the codebase.\n\n{line}{URL}\n\n"); + assert_eq!(Report::read(&message), Some(report), "{line}"); + } + } + + #[test] + fn a_final_message_without_the_line_as_asked_for_reports_nothing() { + for message in [ + "", + "Published the plan.", + "Architecture review plan: https://github.com/acme/widgets/pull/8", + "Architecture review plan: #8", + "Architecture review plan: https://github.com/acme/widgets/issues/8 (a Spec)", + "The plan: https://github.com/acme/widgets/issues/8", + "Architecture review plan: https://github.com/acme/widgets/issues/8\nThat's all.", + ] { + assert_eq!(Report::read(message), None, "{message:?}"); + } + } +} diff --git a/src/args.rs b/src/args.rs index 1add5a89b..d8601d05c 100644 --- a/src/args.rs +++ b/src/args.rs @@ -1,5 +1,7 @@ -//! The command line: which command, and for a Run, its Issue URL and flags. +//! The command line: which command, and for a Run, its Issue URL and flags, +//! or for an Architect run, its focus and flags. +use std::iter::Peekable; use std::mem::discriminant; use std::num::NonZeroUsize; @@ -19,9 +21,37 @@ pub enum Command { Setup, /// `email-test`, with the address it was given, if any. EmailTest(Option), + Architect(ArchitectArgs), Run(RunArgs), } +/// The flag that stops an Architect run once its plan is published and +/// marked ready. +const PLAN_ONLY: &str = "--plan-only"; + +/// An Architect run's arguments. +#[derive(Debug, PartialEq, Eq)] +pub struct ArchitectArgs { + /// The free text that points the Architecture review at an area, if + /// given. + pub focus: Option, + /// What `email` or `no-email` asked for, if either was given; without + /// one, the User config decides. + pub email: Option, + /// The flags for the Spec run or Run the plan is dispatched as, or none + /// with [`PLAN_ONLY`], which dispatches nothing. + pub dispatch: Option, +} + +/// What an Architect run's flags ask of the Spec run or Run it dispatches +/// its plan as, each meaning what it does in [`RunArgs`]. +#[derive(Debug, Default, PartialEq, Eq)] +pub struct DispatchArgs { + pub goal: Option, + pub parallel: Option, + pub base_fix: Option, +} + /// The word that lets a Run start a Base fix, which a Spec run passes on to /// each Ticket's Run. pub const BASE_FIX: &str = "base-fix"; @@ -55,9 +85,9 @@ pub struct RunArgs { } /// Parse the arguments after the program name. `help`, `version`, `update`, -/// `setup` and `email-test` are commands only as the first argument. -/// Otherwise it is a Run: one Issue URL, with each Run flag at most once, -/// before or after it. +/// `setup`, `email-test` and `architect` are commands only as the first +/// argument. Otherwise it is a Run: one Issue URL, with each Run flag at +/// most once, before or after it. /// `email` may be followed by the address to send the Run notification to, /// and `parallel` must be followed by a whole number from 1 up. /// `merge` and `no-merge` contradict each other, as do `email` and `no-email`, @@ -80,45 +110,18 @@ pub fn parse(args: &[String]) -> Result { [_, extra, ..] => bail!("unexpected argument after the address: {extra}"), }; } + Some("architect") => return parse_architect(&args[1..]).map(Command::Architect), _ => {} } let mut issue = None; - let mut goal = None; - let mut email = None; - let mut parallel = None; - let mut base_fix = None; + let mut flags = RunFlags::default(); let mut child = None; let mut args = args.iter().peekable(); while let Some(arg) = args.next() { + if flags.take(arg, &mut args)? { + continue; + } match arg.as_str() { - "merge" | "--merge" => ask_once(&mut goal, Goal::Merged, arg, MERGE_FLAGS)?, - "no-merge" | "--no-merge" => { - ask_once(&mut goal, Goal::ReadyForReview, arg, MERGE_FLAGS)? - } - "email" | "--email" => { - let to = args.next_if(|next| is_address(next)).cloned(); - ask_once(&mut email, NotificationAsk::Send(to), arg, EMAIL_FLAGS)? - } - "no-email" | "--no-email" => { - ask_once(&mut email, NotificationAsk::Skip, arg, EMAIL_FLAGS)? - } - "parallel" | "--parallel" => { - if parallel.is_some() { - bail!("repeated argument: {arg}"); - } - let n = args.next(); - let Some(n) = n.and_then(|n| n.parse::().ok()) else { - let given = n.map(|n| format!(", not {n}")).unwrap_or_default(); - bail!("{arg} must be followed by a whole number from 1 up{given}"); - }; - parallel = Some(n); - } - BASE_FIX | "--base-fix" => { - ask_once(&mut base_fix, BaseFixAsk::Allow, arg, BASE_FIX_FLAGS)? - } - "no-base-fix" | "--no-base-fix" => { - ask_once(&mut base_fix, BaseFixAsk::Forbid, arg, BASE_FIX_FLAGS)? - } SPEC_BRANCH | BASE_FIX_INTO => { if child.is_some() { bail!("repeated argument: {arg}"); @@ -143,14 +146,106 @@ pub fn parse(args: &[String]) -> Result { }; Ok(Command::Run(RunArgs { issue, - goal, - email, - parallel, - base_fix, + goal: flags.goal, + email: flags.email, + parallel: flags.parallel, + base_fix: flags.base_fix, child, })) } +/// Parse the arguments after `architect`: at most one focus, and its flags, +/// each at most once, in any order. [`PLAN_ONLY`] dispatches nothing, so the +/// flags for the dispatched run, `merge`, `no-merge`, `parallel`, `base-fix` +/// and `no-base-fix`, can't go with it. `email` and `no-email` are for the +/// Architect run's own Run notification, so they can. None of a Run's flags +/// is ever the focus, and any other argument that starts with a dash is +/// unexpected rather than a focus. +fn parse_architect(args: &[String]) -> Result { + let mut focus = None; + let mut plan_only = false; + let mut flags = RunFlags::default(); + let mut args = args.iter().peekable(); + while let Some(arg) = args.next() { + if flags.take(arg, &mut args)? { + continue; + } + match arg.as_str() { + PLAN_ONLY => { + if plan_only { + bail!("repeated argument: {arg}"); + } + plan_only = true; + } + _ if arg.starts_with('-') => bail!("unexpected argument after architect: {arg}"), + _ if focus.is_some() => bail!("unexpected argument after the focus: {arg}"), + _ if arg.trim().is_empty() => bail!("the focus is empty"), + _ => focus = Some(arg.clone()), + } + } + let dispatch = DispatchArgs { + goal: flags.goal, + parallel: flags.parallel, + base_fix: flags.base_fix, + }; + if plan_only && dispatch != DispatchArgs::default() { + bail!( + "merge, no-merge, parallel, base-fix and no-base-fix can't be used with \ + {PLAN_ONLY}: it dispatches no run for them to apply to" + ); + } + Ok(ArchitectArgs { + focus, + email: flags.email, + dispatch: (!plan_only).then_some(dispatch), + }) +} + +/// The flags a Run and an Architect run both take, as given so far: each +/// field is what [`RunArgs`] says of the one it becomes. +#[derive(Default)] +struct RunFlags { + goal: Option, + email: Option, + parallel: Option, + base_fix: Option, +} + +impl RunFlags { + /// Record what `arg` asks for, if it is one of these flags, with or + /// without its dashes, taking from `rest` the address after `email`, if + /// one is there, and the number after `parallel`. False, taking nothing, + /// if `arg` is none of them. + fn take<'a>( + &mut self, + arg: &str, + rest: &mut Peekable>, + ) -> Result { + match arg { + "merge" | "--merge" => ask_once(&mut self.goal, Goal::Merged, arg, MERGE_FLAGS)?, + "no-merge" | "--no-merge" => { + ask_once(&mut self.goal, Goal::ReadyForReview, arg, MERGE_FLAGS)? + } + "email" | "--email" => { + let to = rest.next_if(|next| is_address(next)).cloned(); + ask_once(&mut self.email, NotificationAsk::Send(to), arg, EMAIL_FLAGS)? + } + "no-email" | "--no-email" => { + ask_once(&mut self.email, NotificationAsk::Skip, arg, EMAIL_FLAGS)? + } + "parallel" | "--parallel" => ask_parallel(&mut self.parallel, arg, rest.next())?, + BASE_FIX | "--base-fix" => { + ask_once(&mut self.base_fix, BaseFixAsk::Allow, arg, BASE_FIX_FLAGS)? + } + "no-base-fix" | "--no-base-fix" => { + ask_once(&mut self.base_fix, BaseFixAsk::Forbid, arg, BASE_FIX_FLAGS)? + } + _ => return Ok(false), + } + Ok(true) + } +} + const MERGE_FLAGS: &str = "merge and no-merge"; const EMAIL_FLAGS: &str = "email and no-email"; const BASE_FIX_FLAGS: &str = "base-fix and no-base-fix"; @@ -169,8 +264,23 @@ fn ask_once(given: &mut Option, asked: T, arg: &str, flags: &str) -> Resul Ok(()) } +/// Record in `parallel` the number `n` that follows the flag `arg`: a whole +/// number from 1 up, given once. +fn ask_parallel(parallel: &mut Option, arg: &str, n: Option<&String>) -> Result<()> { + if parallel.is_some() { + bail!("repeated argument: {arg}"); + } + let Some(n) = n.and_then(|n| n.parse::().ok()) else { + let given = n.map(|n| format!(", not {n}")).unwrap_or_default(); + bail!("{arg} must be followed by a whole number from 1 up{given}"); + }; + *parallel = Some(n); + Ok(()) +} + /// Is `arg`, after `email`, the address to send to? Only if it looks like -/// one, so the Issue URL is never taken for it. +/// one, so the Issue URL is never taken for it, nor is an Architect run's +/// focus unless it has an `@`. fn is_address(arg: &str) -> bool { arg.contains('@') && !arg.starts_with("https://") } @@ -181,16 +291,280 @@ mod tests { const URL: &str = "https://github.com/acme/widgets/issues/7"; + fn parse_strs(args: &[&str]) -> Result { + let args: Vec = args.iter().map(|arg| arg.to_string()).collect(); + parse(&args) + } + /// The Run `args` parse to. fn run_args(args: &[&str]) -> RunArgs { - let args: Vec = args.iter().map(|arg| arg.to_string()).collect(); - match parse(&args) { + match parse_strs(args) { Ok(Command::Run(run_args)) => run_args, Ok(_) => panic!("{args:?}: not a Run"), Err(error) => panic!("{args:?}: {error:#}"), } } + /// The Architect run `args` parse to. + fn architect_args(args: &[&str]) -> ArchitectArgs { + match parse_strs(args) { + Ok(Command::Architect(architect_args)) => architect_args, + Ok(_) => panic!("{args:?}: not an Architect run"), + Err(error) => panic!("{args:?}: {error:#}"), + } + } + + /// The error `args` are rejected with. + fn rejection(args: &[&str]) -> String { + match parse_strs(args) { + Ok(_) => panic!("{args:?}: not rejected"), + Err(error) => format!("{error:#}"), + } + } + + #[test] + fn architect_takes_plan_only_with_or_without_a_focus_on_either_side_of_it() { + let focus = || Some("the Spec run".to_string()); + for (args, expected) in [ + (vec!["architect", "--plan-only"], None), + (vec!["architect", "the Spec run", "--plan-only"], focus()), + (vec!["architect", "--plan-only", "the Spec run"], focus()), + ] { + let architect_args = architect_args(&args); + assert_eq!(architect_args.focus, expected, "{args:?}"); + assert_eq!(architect_args.dispatch, None, "{args:?}"); + } + } + + #[test] + fn architect_without_plan_only_dispatches_its_plan_with_or_without_a_focus() { + for (args, focus) in [ + (vec!["architect"], None), + ( + vec!["architect", "the Spec run"], + Some("the Spec run".to_string()), + ), + ] { + let architect_args = architect_args(&args); + assert_eq!(architect_args.focus, focus, "{args:?}"); + assert_eq!( + architect_args.dispatch, + Some(DispatchArgs::default()), + "{args:?}" + ); + } + } + + #[test] + fn architect_takes_the_merge_and_parallel_flags_for_the_run_it_dispatches() { + let two = NonZeroUsize::new(2); + for (args, goal, parallel) in [ + (vec!["architect", "merge"], Some(Goal::Merged), None), + (vec!["architect", "--merge"], Some(Goal::Merged), None), + ( + vec!["architect", "no-merge"], + Some(Goal::ReadyForReview), + None, + ), + ( + vec!["architect", "--no-merge"], + Some(Goal::ReadyForReview), + None, + ), + (vec!["architect", "parallel", "2"], None, two), + ( + vec!["architect", "--parallel", "2", "the Spec run", "merge"], + Some(Goal::Merged), + two, + ), + ] { + assert_eq!( + architect_args(&args).dispatch, + Some(DispatchArgs { + goal, + parallel, + base_fix: None + }), + "{args:?}" + ); + } + let args = ["architect", "--parallel", "2", "the Spec run", "merge"]; + assert_eq!(architect_args(&args).focus.as_deref(), Some("the Spec run")); + } + + #[test] + fn architect_takes_the_base_fix_flags_for_the_run_it_dispatches_and_never_as_the_focus() { + for (args, base_fix) in [ + (vec!["architect"], None), + (vec!["architect", "base-fix"], Some(BaseFixAsk::Allow)), + (vec!["architect", "--base-fix"], Some(BaseFixAsk::Allow)), + (vec!["architect", "no-base-fix"], Some(BaseFixAsk::Forbid)), + ( + vec!["architect", "the Spec run", "--no-base-fix", "merge"], + Some(BaseFixAsk::Forbid), + ), + ] { + let architect_args = architect_args(&args); + let dispatch = architect_args.dispatch.expect("a dispatch"); + assert_eq!(dispatch.base_fix, base_fix, "{args:?}"); + let focus = args.contains(&"the Spec run").then_some("the Spec run"); + assert_eq!(architect_args.focus.as_deref(), focus, "{args:?}"); + } + } + + #[test] + fn architect_takes_the_email_flags_with_or_without_plan_only_and_never_as_the_focus() { + let bare = || Some(NotificationAsk::Send(None)); + let to_me = || Some(NotificationAsk::Send(Some("me@example.com".to_string()))); + let focus = || Some("the Spec run".to_string()); + for (args, email, focus) in [ + (vec!["architect"], None, None), + (vec!["architect", "email"], bare(), None), + (vec!["architect", "--email", "--plan-only"], bare(), None), + (vec!["architect", "email", "the Spec run"], bare(), focus()), + (vec!["architect", "email", "merge"], bare(), None), + (vec!["architect", "email", "me@example.com"], to_me(), None), + ( + vec!["architect", "the Spec run", "--email", "me@example.com"], + to_me(), + focus(), + ), + ( + vec!["architect", "--plan-only", "no-email"], + Some(NotificationAsk::Skip), + None, + ), + ( + vec!["architect", "--no-email", "the Spec run"], + Some(NotificationAsk::Skip), + focus(), + ), + ] { + let architect_args = architect_args(&args); + assert_eq!(architect_args.email, email, "{args:?}"); + assert_eq!(architect_args.focus, focus, "{args:?}"); + } + } + + #[test] + fn architect_rejects_contradictory_and_repeated_email_flags() { + for (args, error) in [ + ( + vec!["architect", "email", "--no-email"], + "email and no-email can't be used together", + ), + ( + vec!["architect", "no-email", "--plan-only", "--email"], + "email and no-email can't be used together", + ), + ( + vec!["architect", "--email", "me@example.com", "email"], + "repeated argument: email", + ), + ] { + assert_eq!(rejection(&args), error, "{args:?}"); + } + } + + #[test] + fn architect_rejects_contradictory_repeated_and_malformed_dispatch_flags() { + for (args, error) in [ + ( + vec!["architect", "merge", "--no-merge"], + "merge and no-merge can't be used together", + ), + ( + vec!["architect", "no-merge", "the Spec run", "merge"], + "merge and no-merge can't be used together", + ), + ( + vec!["architect", "merge", "--merge"], + "repeated argument: --merge", + ), + ( + vec!["architect", "parallel", "2", "parallel", "3"], + "repeated argument: parallel", + ), + ( + vec!["architect", "base-fix", "--no-base-fix"], + "base-fix and no-base-fix can't be used together", + ), + ( + vec!["architect", "no-base-fix", "the Spec run", "base-fix"], + "base-fix and no-base-fix can't be used together", + ), + ( + vec!["architect", "base-fix", "--base-fix"], + "repeated argument: --base-fix", + ), + ( + vec!["architect", "parallel"], + "parallel must be followed by a whole number from 1 up", + ), + ( + vec!["architect", "parallel", "the Spec run"], + "parallel must be followed by a whole number from 1 up, not the Spec run", + ), + ] { + assert_eq!(rejection(&args), error, "{args:?}"); + } + } + + #[test] + fn architect_rejects_the_dispatch_flags_with_plan_only_which_dispatches_nothing() { + for args in [ + vec!["architect", "--plan-only", "merge"], + vec!["architect", "--no-merge", "--plan-only"], + vec!["architect", "the Spec run", "parallel", "2", "--plan-only"], + vec!["architect", "--plan-only", "base-fix"], + vec!["architect", "--no-base-fix", "--plan-only"], + ] { + assert_eq!( + rejection(&args), + "merge, no-merge, parallel, base-fix and no-base-fix can't be used with \ + --plan-only: it dispatches no run for them to apply to", + "{args:?}" + ); + } + } + + #[test] + fn architect_rejects_stray_arguments() { + for (args, error) in [ + ( + vec!["architect", "the Spec run", "the Run", "--plan-only"], + "unexpected argument after the focus: the Run", + ), + ( + vec!["architect", "--plan-only", "--plan-only"], + "repeated argument: --plan-only", + ), + ( + vec!["architect", "--verbose", "--plan-only"], + "unexpected argument after architect: --verbose", + ), + (vec!["architect", " ", "--plan-only"], "the focus is empty"), + ] { + assert_eq!(rejection(&args), error, "{args:?}"); + } + } + + #[test] + fn architect_is_a_command_only_as_the_first_argument() { + for (args, error) in [ + ( + vec![URL, "architect"], + "unexpected argument after the Issue URL: architect", + ), + ( + vec!["merge", "architect", "--plan-only"], + "not a GitHub issue URL: architect", + ), + ] { + assert_eq!(rejection(&args), error, "{args:?}"); + } + } + #[test] fn a_bare_flag_asks_for_email_to_and_an_address_after_the_flag_is_the_one_asked_for() { let to_flag_address = || NotificationAsk::Send(Some("flag@example.com".to_string())); diff --git a/src/base_fix.rs b/src/base_fix.rs index 98552cae0..49d1223b5 100644 --- a/src/base_fix.rs +++ b/src/base_fix.rs @@ -19,6 +19,7 @@ use crate::github::{self, Check, CheckState}; use crate::issue::IssueUrl; use crate::poll; use crate::progress; +use crate::spec_run::READY_FOR_AGENT; /// The label that marks a Base fix issue, which a Run finds an open one by. const BASE_FIX_LABEL: &str = "base-fix"; @@ -27,7 +28,7 @@ const BASE_FIX_LABEL: &str = "base-fix"; /// the repository with if the repository lacks it. const LABELS: [(&str, &str); 2] = [ (BASE_FIX_LABEL, "A Base fix: CI is red on a Base branch"), - ("ready-for-agent", "Ready for an agent to take on"), + (READY_FOR_AGENT, "Ready for an agent to take on"), ]; /// What starts the line on stderr saying what became of the Base fix a Run diff --git a/src/config.rs b/src/config.rs index d0820947a..be0fbbbaf 100644 --- a/src/config.rs +++ b/src/config.rs @@ -45,8 +45,8 @@ pub struct UserConfig { /// file holds no secret. #[derive(Debug, Default, PartialEq, Eq)] pub struct EmailSettings { - /// `email.always`: every Run sends a Run notification unless told - /// `no-email`. + /// `email.always`: every Run, and every Architect run, sends a Run + /// notification unless told `no-email`. pub always: bool, /// `email.to`: the address email goes to when the command names none. pub to: Option, diff --git a/src/github.rs b/src/github.rs index 1cf9b4812..df941ea13 100644 --- a/src/github.rs +++ b/src/github.rs @@ -5,41 +5,87 @@ use std::fmt; use std::process::Command; use anyhow::{Context, Result, bail}; +use chrono::{DateTime, Utc}; use serde_json::Value; use crate::issue::IssueUrl; -/// Whether `issue` is open. -pub fn issue_is_open(issue: &IssueUrl) -> Result { - let json = gh_json(&[ +/// The `fields` of `issue`, as JSON. +fn issue_view(issue: &IssueUrl, fields: &str) -> Result { + gh_json(&[ "issue", "view", &issue.number.to_string(), "--repo", &issue.repo_slug(), "--json", - "state", - ])?; + fields, + ]) +} + +/// Whether the issue `json` describes, with its `state`, is open. +fn state_is_open(json: &Value) -> Result { Ok(json["state"].as_str().context("gh output has no state")? == "OPEN") } +/// Whether `issue` is open. +pub fn issue_is_open(issue: &IssueUrl) -> Result { + state_is_open(&issue_view(issue, "state")?) +} + /// The title of `issue`. pub fn issue_title(issue: &IssueUrl) -> Result { - let json = gh_json(&[ - "issue", - "view", - &issue.number.to_string(), - "--repo", - &issue.repo_slug(), - "--json", - "title", - ])?; + let json = issue_view(issue, "title")?; Ok(json["title"] .as_str() .context("gh output has no title")? .to_string()) } +/// An issue as an Architect run reads the plan its Architecture review +/// published. +pub struct Issue { + pub is_open: bool, + pub labels: Vec, + pub created: DateTime, +} + +/// `issue`'s state, labels and when it was created. +pub fn issue(issue: &IssueUrl) -> Result { + let json = issue_view(issue, "state,labels,createdAt")?; + let created = json["createdAt"] + .as_str() + .context("gh output has no createdAt")?; + Ok(Issue { + is_open: state_is_open(&json)?, + labels: json["labels"] + .as_array() + .context("gh output has no labels")? + .iter() + .filter_map(|label| label["name"].as_str().map(String::from)) + .collect(), + created: DateTime::parse_from_rfc3339(created) + .with_context(|| format!("gh output has an unreadable createdAt {created}"))? + .to_utc(), + }) +} + +/// Set `issue`'s labels to exactly `labels`, in one request, so a swap of +/// one label for another can't stop halfway. Through the REST API: `gh issue +/// edit` fails on the GitHub Projects (classic) sunset in older `gh`. +pub fn set_labels(issue: &IssueUrl, labels: &[&str]) -> Result<()> { + let path = format!("repos/{}/issues/{}/labels", issue.repo_slug(), issue.number); + let fields: Vec = labels + .iter() + .map(|label| format!("labels[]={label}")) + .collect(); + let mut args = vec!["api", "--method", "PUT", &path, "--silent"]; + for field in &fields { + args.extend(["-f", field]); + } + gh(&args) +} + /// A Spec's sub-issue, as a Spec run reads it. pub struct Ticket { pub number: u64, diff --git a/src/issue.rs b/src/issue.rs index 030919fd8..be14261ce 100644 --- a/src/issue.rs +++ b/src/issue.rs @@ -1,9 +1,9 @@ -//! The Issue URL and the Origin match. +//! The Issue URL, the repository `origin` names, and the Origin match. use anyhow::{Result, bail}; /// A parsed `https://github.com///issues/`. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct IssueUrl { pub url: String, pub owner: String, @@ -51,20 +51,88 @@ impl IssueUrl { /// The Origin match: does `origin_url` (the raw configured origin, HTTPS or /// SSH, with or without `.git`) name this issue's repository? Case-insensitive. pub fn matches_origin(&self, origin_url: &str) -> bool { - normalise_origin(origin_url).is_some_and(|origin| { - origin == format!("github.com/{}", self.repo_slug()).to_lowercase() + Repo::of_origin(origin_url).is_some_and(|origin| { + origin.owner.eq_ignore_ascii_case(&self.owner) + && origin.name.eq_ignore_ascii_case(&self.repo) }) } } -/// `github.com//`, lowercased, or `None` for a non-GitHub origin. -fn normalise_origin(origin_url: &str) -> Option { - let lower = origin_url.trim().to_lowercase(); - let path = lower - .strip_prefix("https://github.com/") - .or_else(|| lower.strip_prefix("git@github.com:")) - .or_else(|| lower.strip_prefix("ssh://git@github.com/"))?; - let path = path.trim_end_matches('/'); - let path = path.strip_suffix(".git").unwrap_or(path); - Some(format!("github.com/{path}")) +/// The GitHub repository an `origin` remote names, spelled as its URL spells +/// it. +#[derive(Debug, PartialEq, Eq)] +pub struct Repo { + pub owner: String, + pub name: String, +} + +impl Repo { + /// The repository `origin_url` names (the raw configured origin, HTTPS or + /// SSH, with or without `.git`), or `None` for a non-GitHub origin. + pub fn of_origin(origin_url: &str) -> Option { + const GIT: &str = ".git"; + let url = origin_url.trim(); + let path = [ + "https://github.com/", + "git@github.com:", + "ssh://git@github.com/", + ] + .into_iter() + .find_map(|prefix| { + let start = url.get(..prefix.len())?; + start + .eq_ignore_ascii_case(prefix) + .then(|| &url[prefix.len()..]) + })?; + let path = path.trim_end_matches('/'); + let path = match path + .len() + .checked_sub(GIT.len()) + .and_then(|at| path.get(at..)) + { + Some(end) if end.eq_ignore_ascii_case(GIT) => &path[..path.len() - GIT.len()], + _ => path, + }; + match path.split('/').collect::>().as_slice() { + [owner, name] if !owner.is_empty() && !name.is_empty() => Some(Repo { + owner: owner.to_string(), + name: name.to_string(), + }), + _ => None, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn the_repository_of_an_origin_keeps_its_spelling_whatever_the_form_of_the_url() { + let repo = |owner: &str, name: &str| { + Some(Repo { + owner: owner.to_string(), + name: name.to_string(), + }) + }; + for (origin, expected) in [ + ( + "https://github.com/acme/widgets.git", + repo("acme", "widgets"), + ), + ("https://github.com/Acme/Widgets", repo("Acme", "Widgets")), + ( + "HTTPS://GitHub.com/acme/widgets.GIT/", + repo("acme", "widgets"), + ), + ("git@github.com:acme/widgets.git\n", repo("acme", "widgets")), + ("ssh://git@github.com/acme/widgets", repo("acme", "widgets")), + ("https://gitlab.com/acme/widgets.git", None), + ("https://github.com/acme", None), + ("https://github.com/acme/widgets/extra", None), + ("../origin.git", None), + ] { + assert_eq!(Repo::of_origin(origin), expected, "{origin}"); + } + } } diff --git a/src/main.rs b/src/main.rs index 9b4fcd9a1..a7566639e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,3 +1,4 @@ +mod architect; mod args; mod base_fix; mod branch; @@ -30,26 +31,28 @@ mod worktree; use std::io::Write; use std::process::ExitCode; -use args::{Command, RunArgs}; -use base_fix::{BaseFix, BaseFixAsk}; -use child_run::Kind; +use args::{ArchitectArgs, Command, RunArgs}; +use base_fix::BaseFixAsk; use config::UserConfig; -use notification::{NotificationAsk, RunNotification}; -use run::Goal; +use failed_run::FailedRun; +use notification::{ArchitectNotification, NotificationAsk, RunNotification}; +use run::{Ended, Goal}; use spec_run::Parallel; const HELP: &str = "\ thirdshift turns a GitHub issue into a ready-for-review pull request, or a merged one, unattended. -usage: thirdshift Run the factory on the issue, from the clone on the Base branch - thirdshift merge Run the factory on the issue, then merge its pull request - thirdshift --no-merge Run the factory on the issue and leave its pull request for review - thirdshift --email Run the factory on the issue, then email how the Run ended - thirdshift email-test [
] Send a test email through Resend, to check the email setup - thirdshift setup Choose your defaults, then write the User config with every setting - thirdshift update Update thirdshift to the latest release - thirdshift version Print thirdshift's version - thirdshift help Print this help +usage: thirdshift Run the factory on the issue, from the clone on the Base branch + thirdshift merge Run the factory on the issue, then merge its pull request + thirdshift --no-merge Run the factory on the issue and leave its pull request for review + thirdshift --email Run the factory on the issue, then email how the Run ended + thirdshift architect [] Review the Base branch's architecture, publish a plan for a refactor, and run it + thirdshift architect [] --plan-only Publish and mark ready the plan for a refactor, and stop there + thirdshift email-test [
] Send a test email through Resend, to check the email setup + thirdshift setup Choose your defaults, then write the User config with every setting + thirdshift update Update thirdshift to the latest release + thirdshift version Print thirdshift's version + thirdshift help Print this help merge, --no-merge, --email, --no-email, base-fix, --no-base-fix and parallel (or --parallel ) go before or after the Issue URL, in any order. @@ -91,6 +94,38 @@ Tickets always merge into the Spec branch, whatever the command or the User conf merge on a Spec merges the Spec PR into the Base branch once it is ready, mergeable and green, as does merge.always; without either, or with --no-merge, the Spec PR is left ready for review. +architect starts an Architect run from the clone on the Base branch, with no Issue URL. Its +Architecture review, an agent session in its own worktree at the Base branch's head on origin, +looks for deepening opportunities and publishes the top one as a plan: a Spec with Tickets, or +a single Ticket. thirdshift then checks that the plan is open, new and not labelled +ready-for-human, needs-info or wontfix, swaps its needs-triage label for ready-for-agent, +and dispatches it as thirdshift would: a Spec run on a Spec, a Run on a single +Ticket. The Architect run ends as that run does, with its exit code and its PR's URL. +merge, --no-merge, base-fix, --no-base-fix and parallel apply to that run, as do the +User config's defaults; parallel fails it if the plan is a single Ticket. The review +itself watches no CI, so only that run can start a Base fix. With --plan-only, the +Architect run prints the plan's URL and stops instead, for you to read, edit and run with +thirdshift , and takes none of those flags. is free text, one argument, +that points the review at an area: + + thirdshift architect \"the Spec run\" + +A review that finds no Strong candidate publishes no plan. It files its top recommendation as +one idea issue labelled needs-triage, or names the open issue that already covers it, and +thirdshift prints that issue's URL instead, changing no label. Its last line says which: the +review filed the idea, or it filed nothing. + +A review that fails, is interrupted, or ends without naming one of these issues fails the +Architect run and leaves any plan it published labelled needs-triage. One that finds no +deepening opportunity at all has no issue to name, so it fails the Architect run too. Start +one Architect run per repository at a time: two at once may publish the same plan. + +--email, --email
and --no-email ask an Architect run for its Run notification as +they do a Run, with or without --plan-only, and email.always sets the default. It sends one +for the whole Architect run, however it ends: how the review ended, with the plan or idea +issue it named, and how the run the plan was dispatched as ended, with a line on each Ticket +of a Spec run. The run the plan is dispatched as sends none of its own. + The User config, ~/.thirdshift/config.toml, sets defaults for every Run on this machine; thirdshift setup asks for your defaults and writes one listing every setting, to edit. With merge.always set, every Run is a Merge run unless given --no-merge: @@ -156,15 +191,13 @@ fn main() -> ExitCode { UserConfig::load().and_then(|config| email::send_test(to, &config.email)), ); } + Ok(Command::Architect(architect_args)) => return architect(architect_args), Ok(Command::Run(run_args)) => run_args, Err(error) => return argument_error(format_args!("{error:#}")), }; - let config = match config::offer_setup().and_then(|()| UserConfig::load()) { + let config = match user_config() { Ok(config) => config, - Err(error) => { - progress::step(format_args!("{error:#}")); - return ExitCode::FAILURE; - } + Err(failure) => return failure, }; // A child Run, a Ticket's Run in a Spec run or a Base fix, is always a // Merge run, and leaves the Run notification, the Launch directory and @@ -185,40 +218,110 @@ fn main() -> ExitCode { }; // First, so no interrupt can end the Run once its notification is checked. if let Err(error) = interrupt::install() { - progress::step(format_args!("{error:#}")); - return ExitCode::FAILURE; + return failure(&error); } - let notification = match email { - NotificationAsk::Send(to) => RunNotification::new(to, &config.email, &issue).map(Some), - NotificationAsk::Skip => Ok(None), - }; - let notification = match notification { + let notification = match asked(email, |to| RunNotification::new(to, &config.email, &issue)) { Ok(notification) => notification, - Err(error) => { - progress::step(format_args!("{error:#}")); - return ExitCode::FAILURE; - } + Err(error) => return failure(&error), }; - let mut base_fix = BaseFix::new(child.as_ref(), base_fix); - let parallel = Parallel { - tickets: parallel.unwrap_or(config.spec_parallel), - asked: parallel.is_some(), - }; - let ended = run::run( + let ended = run::run_to_end( &issue, goal, &config.logs_dir, launch_pull, - parallel, - child.as_ref().map(Kind::base), - &mut base_fix, + Parallel::new(parallel, config.spec_parallel), + child.as_ref(), + base_fix, ); - let base_fix = base_fix.report(); + let code = run_outcome(&ended); + if let Some(notification) = notification { + notification.send(&ended); + } + code +} + +/// An Architect run: the Architecture review and its plan marked ready, then, +/// unless the command asked to stop at the plan, the plan dispatched as +/// `thirdshift ` with the same flags would be, whose ending is the +/// Architect run's, with the Base fix it took, if any. One that stops at the +/// plan, or whose review found no Strong candidate and so published no plan +/// to dispatch, puts the URL of the issue it ended on on stdout: the plan, +/// the idea issue the review filed, or the issue that already covers its top +/// recommendation. One whose review or plan fails puts the cause and the +/// session log on stderr. If asked, by the command or the User config, it +/// sends one Run notification, however it ended; the run it dispatched sends +/// none of its own. +fn architect(args: ArchitectArgs) -> ExitCode { + let config = match user_config() { + Ok(config) => config, + Err(failure) => return failure, + }; + // First, so no interrupt can end the Architect run once its notification + // is checked. + if let Err(error) = interrupt::install() { + return failure(&error); + } + let email = args.email.unwrap_or(config.email.default_ask()); + let notification = match asked(email, |to| ArchitectNotification::new(to, &config.email)) { + Ok(notification) => notification, + Err(error) => return failure(&error), + }; + let reviewed = architect::run(args.focus.as_deref(), &config.logs_dir, config.launch_pull); + let dispatched = match (&reviewed, &args.dispatch) { + (Ok(architect::Outcome::PlanReady(plan)), Some(dispatch)) => { + progress::step(format_args!( + "dispatching the plan {url}, as thirdshift {url} would", + url = plan.url + )); + Some(run::run_to_end( + plan, + dispatch.goal.unwrap_or(config.default_goal()), + &config.logs_dir, + config.launch_pull, + Parallel::new(dispatch.parallel, config.spec_parallel), + None, + dispatch.base_fix.unwrap_or(config.default_base_fix()), + )) + } + _ => None, + }; + let code = match (&reviewed, &dispatched) { + (_, Some(ended)) => run_outcome(ended), + (Ok(outcome), None) => { + // Also on stderr, so the outcome shows even when stdout is captured. + progress::step(format_args!("{outcome}")); + print_url(outcome.url()); + ExitCode::SUCCESS + } + (Err(failed), None) => report(failed), + }; + if let Some(notification) = notification { + notification.send(&reviewed, dispatched.as_ref()); + } + code +} + +/// The Run notification `email` asks for, if it asks for one: what `checked` +/// makes of the address it gave, or its error if a check fails. +fn asked( + email: NotificationAsk, + checked: impl FnOnce(Option) -> anyhow::Result, +) -> anyhow::Result> { + match email { + NotificationAsk::Send(to) => checked(to).map(Some), + NotificationAsk::Skip => Ok(None), + } +} + +/// How a Run or a Spec run that `ended` shows: what became of its Base fix, +/// if it took one, then its pull request's URL on stdout once it reached its +/// goal, or as a Failed run does. +fn run_outcome(ended: &Ended) -> ExitCode { // Before the outcome, which a failed child Run's last lines are read as. - if let Some(report) = &base_fix { + if let Some(report) = &ended.base_fix { progress::step(format_args!("{}{report}", base_fix::REPORT)); } - let code = match &ended { + match &ended.outcome { Ok(reached) => { // Also on stderr, so the outcome shows even when stdout is captured. progress::step(format_args!( @@ -226,30 +329,46 @@ fn main() -> ExitCode { reached.pr_url, reached.goal.outcome() )); - print_pr_url(&reached.pr_url); + print_url(&reached.pr_url); ExitCode::SUCCESS } - Err(failed) => { - progress::step(format_args!("{:#}", failed.error)); - if let Some(log) = &failed.log { - progress::step(format_args!("{}{}", failed_run::SESSION_LOG, log.display())); - } - if let Some(pr_url) = &failed.pr_url { - print_pr_url(pr_url); - } - ExitCode::FAILURE - } - }; - if let Some(notification) = notification { - notification.send(&ended, base_fix.as_deref()); + Err(failed) => report(failed), } - code } -/// The PR's URL on stdout. A failed write, as once the terminal has closed, -/// is ignored, so the Run notification still goes. -fn print_pr_url(pr_url: &str) { - let _ = writeln!(std::io::stdout(), "{pr_url}"); +/// The User config, after offering Setup where there is none, or the failure +/// to exit with, its error reported. +fn user_config() -> Result { + config::offer_setup() + .and_then(|()| UserConfig::load()) + .map_err(|error| failure(&error)) +} + +/// `error` on stderr, and the exit code of a failure. +fn failure(error: &anyhow::Error) -> ExitCode { + progress::step(format_args!("{error:#}")); + ExitCode::FAILURE +} + +/// How a Failed run, or a failed Architect run, shows: its cause and its +/// session log on stderr, and its pull request's URL, if it left one, on +/// stdout. +fn report(failed: &FailedRun) -> ExitCode { + progress::step(format_args!("{:#}", failed.error)); + if let Some(log) = &failed.log { + progress::step(format_args!("{}{}", failed_run::SESSION_LOG, log.display())); + } + if let Some(pr_url) = &failed.pr_url { + print_url(pr_url); + } + ExitCode::FAILURE +} + +/// A pull request's URL, or the URL of the issue an Architect run ended on, +/// on stdout. A failed write, as once the terminal has closed, is ignored, so +/// the Run notification still goes. +fn print_url(url: &str) { + let _ = writeln!(std::io::stdout(), "{url}"); } /// The end of a command other than a Run: the line that says how it went, @@ -260,10 +379,7 @@ fn outcome(result: anyhow::Result) -> ExitCode { progress::step(outcome); ExitCode::SUCCESS } - Err(error) => { - progress::step(format_args!("{error:#}")); - ExitCode::FAILURE - } + Err(error) => failure(&error), } } diff --git a/src/notification.rs b/src/notification.rs index a3237d3d6..2209c94fd 100644 --- a/src/notification.rs +++ b/src/notification.rs @@ -1,22 +1,24 @@ -//! The Run notification: one email when a Run or a Spec run ends, whatever -//! its outcome, through the same checks and the same send as `email-test`. +//! The Run notification: one email when a Run, a Spec run or an Architect run +//! ends, whatever its outcome, through the same checks and the same send as +//! `email-test`. use std::path::Path; use std::time::{Duration, Instant}; use anyhow::Result; +use crate::architect; use crate::config::EmailSettings; use crate::email::Resend; use crate::failed_run::FailedRun; use crate::github; use crate::host; -use crate::issue::IssueUrl; +use crate::issue::{IssueUrl, Repo}; use crate::progress; -use crate::run::Reached; +use crate::run::Ended; -/// What a Run asks about its Run notification, by its command or, without -/// `email` or `no-email`, by the User config. +/// What a Run or an Architect run asks about its Run notification, by its +/// command or, without `email` or `no-email`, by the User config. #[derive(Debug, PartialEq, Eq)] pub enum NotificationAsk { /// Send one, to this address, else to `email.to`. @@ -54,52 +56,160 @@ impl RunNotification { }) } - /// Send the notification for the Run that `ended`, with `base_fix`, what - /// became of the Base fix it started or waited on, if any. A failed send is - /// only a warning: it never changes the Run's outcome. - pub fn send(self, ended: &Result, base_fix: Option<&str>) { - let (outcome, pr_url, cause, log, tickets) = match ended { - Ok(reached) => ( - reached.goal.outcome(), - Some(reached.pr_url.as_str()), - None, - reached.log.as_deref(), - &reached.ticket_lines[..], - ), - Err(failed) => { - let (outcome, cause) = if failed.interrupted { - ("interrupted", None) - } else { - ("failed", Some(format!("{:#}", failed.error))) - }; - ( - outcome, - failed.pr_url.as_deref(), - cause, - failed.log.as_deref(), - &failed.ticket_lines[..], - ) - } + /// Send the notification for the Run that `ended`, with what became of + /// the Base fix it started or waited on, if any. A failed send is only a + /// warning: it never changes the Run's outcome. + pub fn send(self, ended: &Ended) { + let ending = Ending::of(ended); + let subject = subject(&self.issue, self.title.as_deref(), ending.outcome); + send(&self.resend, &subject, self.started, None, &ending); + } +} + +/// An Architect run's Run notification, checked and waiting for the Architect +/// run to end. +pub struct ArchitectNotification { + resend: Resend, + /// The repository the Launch directory's `origin` names, if it names one. + repo: Option, + started: Instant, +} + +impl ArchitectNotification { + /// The Run notification for an Architect run starting now from the Launch + /// directory, sent to `to`, else to `email.to`. Fails, before any work, + /// with the checks [`RunNotification::new`] makes. + pub fn new(to: Option, settings: &EmailSettings) -> Result { + let started = Instant::now(); + let resend = Resend::new(to, settings)?; + Ok(ArchitectNotification { + resend, + // Left out of the subject if origin names none; the Architect + // run's own preflight reports why. + repo: architect::repo().ok(), + started, + }) + } + + /// Send the one notification for the Architect run whose Architecture + /// review ended as `reviewed` and, if its plan was dispatched, whose Spec + /// run or Run `dispatched`, with what became of the Base fix that took, + /// if any. A failed send is only a warning: it never changes the + /// Architect run's outcome. + pub fn send( + self, + reviewed: &Result, + dispatched: Option<&Ended>, + ) { + let review = match reviewed { + Ok(outcome) => format!("{}: {}", outcome.review(), outcome.url()), + Err(failed) => failure_outcome(failed, "failed").to_string(), }; - let host = host::name(); - let body = Body { - pr_url, - cause: cause.as_deref(), - base_fix, - log, - host: host.as_deref().unwrap_or("unknown host"), - took: self.started.elapsed(), - tickets, + let ending = match (reviewed, dispatched) { + (_, Some(ended)) => Ending::of(ended), + (Ok(outcome), None) => Ending { + outcome: outcome.review(), + pr_url: None, + cause: None, + base_fix: None, + log: None, + tickets: &[], + }, + (Err(failed), None) => Ending { + outcome: failure_outcome(failed, "review failed"), + ..Ending::of_failure(failed) + }, + }; + let lines = ArchitectLines { + review: &review, + dispatched: dispatched.map(|_| ending.outcome), }; - let subject = subject(&self.issue, self.title.as_deref(), outcome); - if let Err(error) = self.resend.send(&subject, &body.text()) { - progress::step(format_args!( - "warning: could not send the Run notification: {error:#}" - )); + let subject = architect_subject(self.repo.as_ref(), ending.outcome); + send(&self.resend, &subject, self.started, Some(lines), &ending); + } +} + +/// How a Run, a Spec run or an Architecture review ended, as a Run +/// notification tells it. +struct Ending<'a> { + outcome: &'static str, + pr_url: Option<&'a str>, + cause: Option, + /// What became of the Base fix the Run started or waited on, if any. + base_fix: Option<&'a str>, + log: Option<&'a Path>, + /// In a Spec run, a line on each Ticket, as in its summary on stderr. + tickets: &'a [String], +} + +impl<'a> Ending<'a> { + fn of(ended: &'a Ended) -> Self { + let base_fix = ended.base_fix.as_deref(); + match &ended.outcome { + Ok(reached) => Ending { + outcome: reached.goal.outcome(), + pr_url: Some(&reached.pr_url), + cause: None, + base_fix, + log: reached.log.as_deref(), + tickets: &reached.ticket_lines, + }, + Err(failed) => Ending { + base_fix, + ..Ending::of_failure(failed) + }, + } + } + + fn of_failure(failed: &'a FailedRun) -> Self { + Ending { + outcome: failure_outcome(failed, "failed"), + pr_url: failed.pr_url.as_deref(), + cause: (!failed.interrupted).then(|| format!("{:#}", failed.error)), + base_fix: None, + log: failed.log.as_deref(), + tickets: &failed.ticket_lines, } } } +/// `interrupted`, or `failure` for a failure that was not an interrupt. +fn failure_outcome(failed: &FailedRun, failure: &'static str) -> &'static str { + if failed.interrupted { + "interrupted" + } else { + failure + } +} + +/// Send the notification with `subject` for what `started` then and ended as +/// `ending`, in an Architect run with its `architect` lines first. A failed +/// send is only a warning. +fn send( + resend: &Resend, + subject: &str, + started: Instant, + architect: Option, + ending: &Ending, +) { + let host = host::name(); + let body = Body { + architect, + pr_url: ending.pr_url, + cause: ending.cause.as_deref(), + base_fix: ending.base_fix, + log: ending.log, + host: host.as_deref().unwrap_or("unknown host"), + took: started.elapsed(), + tickets: ending.tickets, + }; + if let Err(error) = resend.send(subject, &body.text()) { + progress::step(format_args!( + "warning: could not send the Run notification: {error:#}" + )); + } +} + /// `[thirdshift] /# : <outcome>`, without the title /// if it isn't known. fn subject(issue: &IssueUrl, title: Option<&str>, outcome: &str) -> String { @@ -111,8 +221,26 @@ fn subject(issue: &IssueUrl, title: Option<&str>, outcome: &str) -> String { ) } +/// `[thirdshift] <owner>/<repo> Architect run: <outcome>`, without the +/// repository if it isn't known. +fn architect_subject(repo: Option<&Repo>, outcome: &str) -> String { + let repo = repo + .map(|repo| format!(" {}/{}", repo.owner, repo.name)) + .unwrap_or_default(); + format!("[thirdshift]{repo} Architect run: {outcome}") +} + +/// What an Architect run's notification says before what a Run's does. +struct ArchitectLines<'a> { + /// How the Architecture review ended, with the issue it ended on. + review: &'a str, + /// How the Spec run or Run the plan was dispatched as ended, if it was. + dispatched: Option<&'a str>, +} + /// What the notification's plain-text body says. struct Body<'a> { + architect: Option<ArchitectLines<'a>>, pr_url: Option<&'a str>, cause: Option<&'a str>, /// What became of the Base fix the Run started or waited on, if any. @@ -127,6 +255,12 @@ struct Body<'a> { impl Body<'_> { fn text(&self) -> String { let mut text = String::new(); + if let Some(architect) = &self.architect { + text += &format!("Review: {}\n", architect.review); + if let Some(dispatched) = architect.dispatched { + text += &format!("Dispatched: {dispatched}\n"); + } + } if let Some(pr_url) = self.pr_url { text += &format!("Pull request: {pr_url}\n"); } @@ -187,6 +321,7 @@ mod tests { #[test] fn the_body_leaves_out_what_the_run_does_not_have() { let body = Body { + architect: None, pr_url: None, cause: Some("origin mismatch"), base_fix: None, @@ -219,6 +354,7 @@ mod tests { #[test] fn the_body_reports_the_base_fix_after_the_cause() { let body = Body { + architect: None, pr_url: Some("https://github.com/acme/widgets/pull/1"), cause: Some("claude exited 1"), base_fix: Some("https://github.com/acme/widgets/issues/8 merged"), @@ -244,6 +380,7 @@ mod tests { "#22 blocked by #21".to_string(), ]; let body = Body { + architect: None, pr_url: None, cause: Some("Tickets not done: #21, #22"), base_fix: None, @@ -264,6 +401,58 @@ mod tests { ); } + #[test] + fn an_architect_runs_subject_names_the_repository_if_known_and_the_outcome() { + let repo = Repo::of_origin("https://github.com/acme/widgets.git").unwrap(); + assert_eq!( + architect_subject(Some(&repo), "plan published"), + "[thirdshift] acme/widgets Architect run: plan published" + ); + assert_eq!( + architect_subject(None, "review failed"), + "[thirdshift] Architect run: review failed" + ); + } + + #[test] + fn an_architect_runs_body_starts_with_the_review_and_the_dispatched_runs_outcome() { + let body = Body { + architect: Some(ArchitectLines { + review: "plan published: https://github.com/acme/widgets/issues/8", + dispatched: Some("merged"), + }), + pr_url: Some("https://github.com/acme/widgets/pull/1"), + cause: None, + base_fix: None, + log: None, + host: "droplet-1", + took: Duration::from_secs(4), + tickets: &[], + }; + assert_eq!( + body.text(), + "Review: plan published: https://github.com/acme/widgets/issues/8\n\ + Dispatched: merged\n\ + Pull request: https://github.com/acme/widgets/pull/1\n\ + Host: droplet-1\n\ + Took: 4s\n" + ); + let body = Body { + architect: Some(ArchitectLines { + review: "idea filed: https://github.com/acme/widgets/issues/8", + dispatched: None, + }), + pr_url: None, + ..body + }; + assert_eq!( + body.text(), + "Review: idea filed: https://github.com/acme/widgets/issues/8\n\ + Host: droplet-1\n\ + Took: 4s\n" + ); + } + #[test] fn took_is_to_the_second() { for (seconds, text) in [ diff --git a/src/preflight.rs b/src/preflight.rs index 8b7f7c3fe..5cd1cbe92 100644 --- a/src/preflight.rs +++ b/src/preflight.rs @@ -1,5 +1,5 @@ -//! Pre-flight checks: everything that must hold before a Run creates -//! anything. +//! Pre-flight checks: everything that must hold before a Run or an Architect +//! run creates anything. use anyhow::{Result, bail}; @@ -21,6 +21,11 @@ pub fn check(launch: &Git, issue: &IssueUrl) -> Result<()> { if !github::issue_is_open(issue)? { bail!("issue #{} is closed", issue.number); } + check_identity(launch) +} + +/// Check that git has the name and email an agent commits with. +pub fn check_identity(launch: &Git) -> Result<()> { for (key, example_value) in [ ("user.name", r#""Your Name""#), ("user.email", "you@example.com"), diff --git a/src/progress.rs b/src/progress.rs index 9ac070b1b..5be0dd9ba 100644 --- a/src/progress.rs +++ b/src/progress.rs @@ -86,6 +86,8 @@ pub struct Progress { /// Signed in with a claude.ai subscription rather than an API key. subscription: bool, turns_and_cost: Option<(u64, f64)>, + /// The text of the last `result`, if it had any. + final_message: Option<String>, } impl Progress { @@ -117,6 +119,7 @@ impl Progress { Some("result") => { self.killed.clear(); self.failed = event["subtype"] != "success" || event["is_error"] == true; + self.final_message = event["result"].as_str().map(String::from); // Both are cumulative across a session's result events. if let (Some(turns), Some(cost)) = ( event["num_turns"].as_u64(), @@ -143,6 +146,13 @@ impl Progress { Some(format!("{turns} turns, ${cost:.2}{basis}")) } + /// The session's final message: the text of its last `result` event, + /// what the agent said as it ended its last turn. None if that `result` + /// had no text, as one that is an error may not. + pub fn final_message(&self) -> Option<&str> { + self.final_message.as_deref() + } + /// The session's id, from its first `init` event. pub fn session_id(&self) -> Option<&str> { self.session_id.as_deref() @@ -410,6 +420,16 @@ mod tests { assert_eq!(progress.summary().as_deref(), Some("34 turns, $1.82")); } + #[test] + fn the_final_message_is_the_text_of_the_last_result() { + let said = |text: &str| json!({ "type": "result", "subtype": "success", "result": text }); + let (progress, _) = lines(&[said("Waiting on the tests."), said("Done.")]); + assert_eq!(progress.final_message(), Some("Done.")); + + let (progress, _) = lines(&[said("Done."), result(3, 0.1)]); + assert_eq!(progress.final_message(), None); + } + #[test] fn on_a_subscription_the_cost_is_marked_as_at_api_prices() { let (progress, _) = lines(&[ diff --git a/src/prompt.rs b/src/prompt.rs index 9494d3381..7123eadd7 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -2,6 +2,7 @@ use crate::ci::{self, FailedChecks}; use crate::issue::IssueUrl; +use crate::spec_run::{NEEDS_TRIAGE, READY_FOR_AGENT}; /// Every prompt ends with this. Sessions run with `claude -p`, which exits /// once the agent ends its turn, killing any background task still running. @@ -89,6 +90,46 @@ pub fn spec_review(spec: &IssueUrl, base: &str, branch: &str, pr_url: &str) -> S ) } +/// What the last line of an Architecture review's final message starts with, +/// before the URL of the issue it names: the plan it published, the idea it +/// filed instead, or the open issue that already covers that idea. The +/// Architecture review prompt asks for the line, and the Architect run reads +/// it back. +pub const PLAN_LINE: &str = "Architecture review plan: "; +pub const IDEA_LINE: &str = "Architecture review idea: "; +pub const ALREADY_FILED_LINE: &str = "Architecture review already filed: "; + +/// The Architecture review prompt, for an Architect run's worktree at the +/// head of the Base branch `base` on origin, pointed at `focus` if the +/// command gave one. +pub fn architecture_review(base: &str, focus: Option<&str>) -> String { + let focus = match focus { + Some(focus) => format!("Focus the review on: {focus}\n\n"), + None => String::new(), + }; + format!( + "/thirdshift:improve-codebase-architecture\n\ + \n\ + This is an Architecture review of the base branch {base}: this worktree is checked out at its head on origin, on no branch.\n\ + \n\ + {focus}\ + Find the deepening opportunities with /thirdshift:improve-codebase-architecture, using /thirdshift:codebase-design for the vocabulary. Skip any that an open issue already covers, and take the top recommendation.\n\ + \n\ + If it is Strong, settle its design yourself and publish it as the plan with /thirdshift:to-spec and /thirdshift:to-tickets: a Spec with Tickets, or a single Ticket when one session is enough. Label the plan's top issue, the Spec or the single Ticket, `{NEEDS_TRIAGE}`, not `{READY_FOR_AGENT}`: thirdshift marks it ready once you are done.\n\ + \n\ + If it is not Strong, file it as one issue labelled `{NEEDS_TRIAGE}`, unless an open issue already covers it.\n\ + \n\ + Do not commit or push anything. You may edit files here to check an idea: the worktree is thrown away when you finish. A change the plan needs to CONTEXT.md or an ADR is a Ticket's work, not yours.\n\ + \n\ + End your final message with one of these lines, as its last line, with the issue's full URL and nothing else on the line:\n\ + {PLAN_LINE}<URL of the Spec or the single Ticket you published>\n\ + {IDEA_LINE}<URL of the issue you filed>\n\ + {ALREADY_FILED_LINE}<URL of the open issue that already covers it>\n\ + \n\ + {HEADLESS}", + ) +} + /// The conflict Repair prompt, for a merge of `origin/<merging>` left in /// progress with conflicts: the Base branch, or in a Merge run the Issue /// branch with Foreign commits on it. diff --git a/src/prompts_page.rs b/src/prompts_page.rs index 06e7d5b07..54c121c80 100644 --- a/src/prompts_page.rs +++ b/src/prompts_page.rs @@ -18,6 +18,8 @@ use crate::plugin::SKILLS; use crate::prompt; use crate::session::claude_args; +use Sender::{ArchitectRun, Units}; + /// A press unit on the home page, which each prompt and skill links back to. struct Unit { anchor: &'static str, @@ -52,10 +54,11 @@ const CHECK: &str = "<failing check>"; const CHECK_URL: &str = "<check URL>"; const INHERITED_CHECK: &str = "<Inherited failure>"; const BACKGROUND_WORK: &str = "<background work>"; +const FOCUS: &str = "<focus>"; const PLUGIN_DIR: &str = "<plugin dir>"; const SESSION_ID: &str = "<session id>"; const PROMPT: &str = "<prompt>"; -const PLACEHOLDERS: [&str; 15] = [ +const PLACEHOLDERS: [&str; 16] = [ ISSUE_URL, SPEC_URL, NUMBER, @@ -68,18 +71,27 @@ const PLACEHOLDERS: [&str; 15] = [ CHECK_URL, INHERITED_CHECK, BACKGROUND_WORK, + FOCUS, PLUGIN_DIR, SESSION_ID, PROMPT, ]; -/// A prompt as the page shows it: when a Run sends it, from which units, and -/// its text with placeholders. +/// Who sends a prompt. +enum Sender { + /// A Run, from one of these press units. + Units(&'static [Unit]), + /// An Architect run, which the press units, a Run's, don't cover. + ArchitectRun, +} + +/// A prompt as the page shows it: when it is sent, by whom, and its text +/// with placeholders. struct Prompt { id: &'static str, title: &'static str, when: &'static str, - units: &'static [Unit], + sender: Sender, text: String, } @@ -112,70 +124,77 @@ fn prompts() -> Vec<Prompt> { id: "prompt-fresh", title: "Fresh", when: "Starts the implement session, which goes on to review and open the pull request, when the Run starts a new Issue branch.", - units: &[IMPLEMENT], + sender: Units(&[IMPLEMENT]), text: prompt::fresh(&issue, BASE, BRANCH), }, Prompt { id: "prompt-continuation", title: "Continuation, with no pull request", when: "Starts the implement session when the Run is a Continuation of an Issue branch that has no pull request.", - units: &[IMPLEMENT], + sender: Units(&[IMPLEMENT]), text: prompt::continuation(&issue, BASE, BRANCH, None), }, Prompt { id: "prompt-continuation-pr", title: "Continuation, with an open pull request", when: "Starts the implement session when the Run is a Continuation of an Issue branch whose pull request is open.", - units: &[IMPLEMENT], + sender: Units(&[IMPLEMENT]), text: prompt::continuation(&issue, BASE, BRANCH, Some(PR_URL)), }, Prompt { id: "prompt-spec-review", title: "Spec review", when: "In a Spec run, starts the Spec review once every Ticket has landed on the Spec branch, before the Spec PR, a draft until then, is marked ready.", - units: &[REVIEW], + sender: Units(&[REVIEW]), text: prompt::spec_review(&spec, BASE, SPEC_BRANCH, PR_URL), }, + Prompt { + id: "prompt-architecture-review", + title: "Architecture review", + when: "Starts the Architecture review, the session an Architect run opens with, in a worktree at the head of the Base branch. The line naming the focus is left out when the command gives none.", + sender: ArchitectRun, + text: prompt::architecture_review(BASE, Some(FOCUS)), + }, Prompt { id: "prompt-conflict-repair", title: "Conflict Repair", when: "Starts a Repair session when merging the Base branch into the Issue branch leaves conflicts.", - units: &[FINISH], + sender: Units(&[FINISH]), text: prompt::conflict_repair(&issue, BASE, BRANCH, PR_URL), }, Prompt { id: "prompt-foreign-conflict-repair", title: "Conflict Repair, on Foreign commits", when: "In a Merge run, starts a Repair session when merging Foreign commits from the Issue branch on origin into the local one leaves conflicts.", - units: &[FINISH], + sender: Units(&[FINISH]), text: prompt::conflict_repair(&issue, BRANCH, BRANCH, PR_URL), }, Prompt { id: "prompt-review-repair", title: "Review Repair", when: "In a Merge run, starts a Repair session once Foreign commits are merged into the Issue branch, to review them from the head the Run last knew as its own before they can be merged.", - units: &[FINISH], + sender: Units(&[FINISH]), text: prompt::review_repair(&issue, BRANCH, PR_URL, OWN_HEAD), }, Prompt { id: "prompt-ci-fix-repair", title: "CI-fix Repair", when: "Starts a Repair session when CI fails on the pull request's head commit, listing each failed check.", - units: &[FINISH], + sender: Units(&[FINISH]), text: prompt::ci_fix_repair(&issue, BASE, BRANCH, PR_URL, &failed), }, Prompt { id: "prompt-ci-fix-repair-inherited", title: "CI-fix Repair, with Inherited failures", when: "Starts a Repair session when CI fails on the pull request's head commit and some of the failed checks, but not all, are Inherited failures: they also failed on the Base branch commit the head last merged in, so they are listed apart as not to fix.", - units: &[FINISH], + sender: Units(&[FINISH]), text: prompt::ci_fix_repair(&issue, BASE, BRANCH, PR_URL, &failed_with_inherited), }, Prompt { id: "prompt-resume", title: "Resume", when: "Continues any session, once, that ended its turn while waiting on background work, which was killed with it.", - units: &[IMPLEMENT, REVIEW, FINISH], + sender: Units(&[IMPLEMENT, REVIEW, FINISH]), text: prompt::resume(&[BACKGROUND_WORK]), }, ]; @@ -296,13 +315,17 @@ fn fence(text: &str) -> String { "`".repeat(longest.max(2) + 1) } -/// The unit whose session uses each Factory skill, and when, if not always. -fn skill_unit(skill: &str) -> (Unit, Option<&'static str>) { +/// What uses each Factory skill: the unit whose session does, linked, and +/// when, if not always; or the Architecture review, which is no unit of a Run. +fn skill_used_by(skill: &str) -> String { match skill { - "implement" | "tdd" => (IMPLEMENT, None), - "code-review" | "pr" => (REVIEW, None), - "resolving-merge-conflicts" => (FINISH, Some("in a Repair")), - _ => panic!("the Factory skill {skill} has no unit: add it to skill_unit"), + "implement" | "tdd" => unit_links(&[IMPLEMENT]), + "code-review" | "pr" => unit_links(&[REVIEW]), + "resolving-merge-conflicts" => format!("{}, in a Repair", unit_links(&[FINISH])), + "improve-codebase-architecture" | "to-spec" | "to-tickets" | "codebase-design" => { + "the Architecture review, in an Architect run".to_string() + } + _ => panic!("the Factory skill {skill} has no user: add it to skill_used_by"), } } @@ -358,14 +381,14 @@ fn prompt_section(html: &mut String) { r##" <article class="job-sheet" id="{id}" aria-labelledby="{id}-title"> <h3 id="{id}-title">{title}</h3> <p>{when}</p> - <p class="sent-by">Sent by {units}</p> + <p class="sent-by">Sent by {sender}</p> <pre class="job-text"><code>{text}</code></pre> </article> "##, id = prompt.id, title = prompt.title, when = prompt.when, - units = unit_links(prompt.units), + sender = sent_by(&prompt.sender), text = highlighted(&prompt.text), ); } @@ -385,15 +408,13 @@ fn skills_section(html: &mut String) { skills.sort_by_key(|dir| dir.path()); for skill in skills { let name = skill.path().to_string_lossy(); - let (unit, note) = skill_unit(&name); - let note = note.map_or(String::new(), |note| format!(", {note}")); let _ = write!( html, r##" <article class="job-sheet" id="skill-{name}" aria-labelledby="skill-{name}-title"> <h3 id="skill-{name}-title">{name}</h3> - <p class="sent-by">Used by {unit}{note}</p> + <p class="sent-by">Used by {used_by}</p> "##, - unit = unit_links(&[unit]), + used_by = skill_used_by(&name), ); let mut files = Vec::new(); collect_files(skill, &mut files); @@ -460,6 +481,14 @@ fn file_block(html: &mut String, file: &File, indent: &str) { ); } +/// Who sends a prompt, as its "Sent by" line ends. +fn sent_by(sender: &Sender) -> String { + match sender { + Units(units) => unit_links(units), + ArchitectRun => "an Architect run, before any unit".to_string(), + } +} + /// Links to `units` on the home page, as a list ending "or". fn unit_links(units: &[Unit]) -> String { let links: Vec<_> = units @@ -690,7 +719,7 @@ mod tests { id: "prompt-example", title: "Example", when: "When an example runs.", - units: &[IMPLEMENT], + sender: Units(&[IMPLEMENT]), text: "Say ```hi```.\n".to_string(), }; let markdown = markdown(&prompt); diff --git a/src/run.rs b/src/run.rs index 74e81ea27..58dab0144 100644 --- a/src/run.rs +++ b/src/run.rs @@ -5,8 +5,9 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result, anyhow, bail}; -use crate::base_fix::BaseFix; +use crate::base_fix::{BaseFix, BaseFixAsk}; use crate::branch::{self, Selection}; +use crate::child_run::Kind; use crate::ci::{self, Ci}; use crate::failed_run::{self, FailedRun, PolicyRefusal}; use crate::git::Git; @@ -51,6 +52,42 @@ pub struct Reached { pub ticket_lines: Vec<String>, } +/// How a Run, or a Spec run, ended. +pub struct Ended { + pub outcome: Result<Reached, FailedRun>, + /// What became of the Base fix it started or waited on, if any, as + /// [`BaseFix::report`] tells it. + pub base_fix: Option<String>, +} + +/// [`run`] the Run on `issue` that asked `base_fix` about a Base fix, as the +/// child Run `child` if another thirdshift started it, whose Base branch is +/// then the one it was given, to its end. +pub fn run_to_end( + issue: &IssueUrl, + goal: Goal, + logs_dir: &Path, + launch_pull: bool, + parallel: Parallel, + child: Option<&Kind>, + base_fix: BaseFixAsk, +) -> Ended { + let mut base_fix = BaseFix::new(child, base_fix); + let outcome = run( + issue, + goal, + logs_dir, + launch_pull, + parallel, + child.map(Kind::base), + &mut base_fix, + ); + Ended { + outcome, + base_fix: base_fix.report(), + } +} + /// Take `issue` to a ready PR, or in a Merge run a merged one. Any failure /// after the worktree exists, including a merge that fails, goes through the /// Failed run path. The worktree, the local @@ -70,7 +107,7 @@ pub struct Reached { /// /// `base_fix` is the one Base fix the Run, or a Spec run for its Spec PR, may /// start, or wait on, when its only red checks are Inherited failures. -pub fn run( +fn run( issue: &IssueUrl, goal: Goal, logs_dir: &Path, @@ -123,11 +160,7 @@ pub fn run( Worktree::continue_existing(&launch, &issue.repo, &branch, &base)? } }; - let logs = Logs { - issue, - dir: logs_dir, - timestamp: ×tamp, - }; + let logs = Logs::of_run(issue, logs_dir, ×tamp); if !tickets.is_empty() { return spec_run::run( issue, @@ -167,7 +200,7 @@ pub fn run( /// if `base` is ahead of it. The Run doesn't depend on this, so a failure, /// such as uncommitted changes in the way, is only a warning, and those /// changes are left as they were. -fn pull_base_branch(launch: &Git, checked_out: Option<&str>, base: &str) { +pub fn pull_base_branch(launch: &Git, checked_out: Option<&str>, base: &str) { if checked_out != Some(base) { return; } diff --git a/src/session.rs b/src/session.rs index d5d290bbb..fd457d8ce 100644 --- a/src/session.rs +++ b/src/session.rs @@ -12,32 +12,50 @@ use std::time::{Duration, Instant}; use anyhow::{Context, Result, anyhow, bail}; use crate::interrupt; -use crate::issue::IssueUrl; +use crate::issue::{IssueUrl, Repo}; use crate::progress::{self, Progress}; use crate::prompt; -/// Where a Run's session logs go: in `dir`, the User config's `logs.dir` or -/// `~/.thirdshift/logs`, each named for the Run's `issue` and `timestamp`. +/// Where a Run's or an Architect run's session logs go: in `dir`, the User +/// config's `logs.dir` or `~/.thirdshift/logs`, each named for what is run +/// and its `timestamp`. pub struct Logs<'a> { - pub issue: &'a IssueUrl, - pub dir: &'a Path, - pub timestamp: &'a str, + /// What every log's name starts with: `<owner>-<repo>-issue-<n>` for a + /// Run, `<owner>-<repo>-architect` for an Architect run. + name: String, + dir: &'a Path, + timestamp: &'a str, } -impl Logs<'_> { +impl<'a> Logs<'a> { + /// The logs of the Run on `issue` that started at `timestamp`. + pub fn of_run(issue: &IssueUrl, dir: &'a Path, timestamp: &'a str) -> Self { + Logs { + name: format!("{}-{}-issue-{}", issue.owner, issue.repo, issue.number), + dir, + timestamp, + } + } + + /// The logs of the Architect run on `repo` that started at `timestamp`. + pub fn of_architect_run(repo: &Repo, dir: &'a Path, timestamp: &'a str) -> Self { + Logs { + name: format!("{}-{}-architect", repo.owner, repo.name), + dir, + timestamp, + } + } + /// Where a session's stream is logged: - /// `<dir>/<owner>-<repo>-issue-<n>-<timestamp>-<kind>.jsonl`. + /// `<dir>/<name>-<timestamp>-<kind>.jsonl`. pub fn path(&self, kind: &str) -> PathBuf { - let issue = self.issue; - self.dir.join(format!( - "{}-{}-issue-{}-{}-{kind}.jsonl", - issue.owner, issue.repo, issue.number, self.timestamp - )) + self.dir + .join(format!("{}-{}-{kind}.jsonl", self.name, self.timestamp)) } } -/// Where a Run's sessions run: in `worktree`, with the Factory skills plugin -/// at `plugin_dir` loaded, each logged in the Run's `logs`. +/// Where a Run's or an Architect run's sessions run: in `worktree`, with the +/// Factory skills plugin at `plugin_dir` loaded, each logged in its `logs`. pub struct Sessions<'a> { pub logs: &'a Logs<'a>, pub worktree: &'a Path, @@ -51,6 +69,17 @@ impl Sessions<'_> { /// as `<kind>-resume`; if that ends the same way, this fails and names the /// killed work. pub fn run(&self, kind: &str, prompt: &str, log: &mut PathBuf) -> Result<()> { + self.run_to_final_message(kind, prompt, log).map(drop) + } + + /// Run a session as [`Sessions::run`] does, and return its final message: + /// what the agent said as it ended its last turn, if anything. + pub fn run_to_final_message( + &self, + kind: &str, + prompt: &str, + log: &mut PathBuf, + ) -> Result<Option<String>> { let mut ended = self.start(kind, None, prompt, log)?; let killed = ended.killed_background_work(); if let (false, Some(session_id)) = (killed.is_empty(), ended.session_id()) { @@ -67,7 +96,7 @@ impl Sessions<'_> { } let killed = ended.killed_background_work(); match killed[..] { - [] => Ok(()), + [] => Ok(ended.final_message().map(String::from)), [task] => bail!( "{kind} session ended while waiting on a background task ({task}), which was killed" ), diff --git a/src/spec_run.rs b/src/spec_run.rs index bca0bf448..1c08b1e8c 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -27,8 +27,17 @@ use crate::run::{self, Goal, Reached}; use crate::session::{Logs, Sessions}; use crate::worktree::Worktree; +/// The triage label of an issue no one has evaluated yet, which an +/// Architecture review publishes its plan with. +pub const NEEDS_TRIAGE: &str = "needs-triage"; + +/// The triage label of an issue an agent can take on: what thirdshift swaps +/// a plan's `needs-triage` for once the plan passes its checks, and what a +/// Base fix issue is opened with. +pub const READY_FOR_AGENT: &str = "ready-for-agent"; + /// The triage labels that make an open Ticket an Unready Ticket. -const UNREADY_LABELS: [&str; 4] = ["ready-for-human", "needs-info", "wontfix", "needs-triage"]; +const UNREADY_LABELS: [&str; 4] = ["ready-for-human", "needs-info", "wontfix", NEEDS_TRIAGE]; /// The Spec review session's kind, in its progress lines and log name. const SPEC_REVIEW: &str = "spec-review"; @@ -46,6 +55,17 @@ pub struct Parallel { pub asked: bool, } +impl Parallel { + /// As many Tickets at once as the command `asked` for with + /// `parallel <n>`, else as the User config's `default` says. + pub fn new(asked: Option<NonZeroUsize>, default: NonZeroUsize) -> Self { + Parallel { + tickets: asked.unwrap_or(default), + asked: asked.is_some(), + } + } +} + /// Whether there are `tickets`, as a Spec has, and every one is closed. pub fn all_closed(tickets: &[Ticket]) -> bool { !tickets.is_empty() && tickets.iter().all(|ticket| !ticket.is_open) @@ -319,17 +339,17 @@ fn next_ready( .any(|blocker| running.contains(blocker)) && !outcomes.contains_key(&ticket.number) && !running.contains(&ticket.number) - && unready_label(ticket).is_none() + && unready_label(&ticket.labels).is_none() }) .map(|ticket| ticket.number) .min() } -/// The first of the Unready Ticket labels `ticket` has, if any. -fn unready_label(ticket: &Ticket) -> Option<&str> { +/// The first of the Unready Ticket labels among an issue's `labels`, if any. +pub fn unready_label(labels: &[String]) -> Option<&'static str> { UNREADY_LABELS .into_iter() - .find(|label| ticket.labels.iter().any(|name| name == label)) + .find(|label| labels.iter().any(|name| name == label)) } /// A line on each Ticket that landed in this Spec run, with its PR, and on @@ -392,7 +412,7 @@ fn standing( } if let Some(landed) = landed { format!("{landed}, but is still open") - } else if let Some(label) = unready_label(ticket) { + } else if let Some(label) = unready_label(&ticket.labels) { format!("unready: labelled {label}") } else if ticket.has_sub_issues { "unready: has sub-issues".to_string() @@ -659,7 +679,7 @@ mod tests { fn each_unready_label_keeps_a_ticket_from_running_and_is_named_in_its_line() { for label in UNREADY_LABELS { let mut unready = ticket(21, true, &[], &[]); - unready.labels = vec!["ready-for-agent".to_string(), label.to_string()]; + unready.labels = vec![READY_FOR_AGENT.to_string(), label.to_string()]; let tickets = [unready, ticket(22, true, &[], &[])]; let outcomes = BTreeMap::new(); diff --git a/src/worktree.rs b/src/worktree.rs index 1cab71244..3419fb65b 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -1,6 +1,7 @@ //! The Run's worktree: a sibling of the launch repository, on the Issue //! branch, removed together with the local Issue branch when dropped unless -//! it is kept. +//! it is kept. And the Architecture review's: a sibling too, on no branch, +//! always removed when dropped. use std::fs::File; use std::path::{Path, PathBuf}; @@ -75,21 +76,12 @@ impl Worktree { branch_args: &[&str], start: &str, ) -> Result<Self> { - let root = PathBuf::from(launch.run(&["rev-parse", "--show-toplevel"])?); - let path = root - .parent() - .context("the repository root has no parent directory")? - .join(format!("{repo}-{branch}")); - let path_arg = path.to_str().context("worktree path is not UTF-8")?; - + let (root, path) = sibling(launch, &format!("{repo}-{branch}"))?; progress::step(format_args!( "creating worktree {} on {branch} from {start}", path.display() )); - let mut args = vec!["worktree", "add"]; - args.extend_from_slice(branch_args); - args.extend([path_arg, start]); - launch.run(&args)?; + add_worktree(launch, branch_args, &path, start)?; Ok(Worktree { launch: Git::new(root), branch: branch.to_string(), @@ -284,6 +276,73 @@ impl Drop for Worktree { } } +/// The worktree an Architecture review runs in: detached at the head of the +/// Base branch on origin, with no Issue branch, and removed when dropped, +/// whatever the session left in it. Nothing in it is committed or pushed. +pub struct ReviewWorktree { + launch: Git, + path: PathBuf, +} + +impl ReviewWorktree { + /// Check out `origin/<base>`, detached, in a new worktree next to the + /// launch repository's root, named `<repo>-architect`. + pub fn create(launch: &Git, repo: &str, base: &str) -> Result<Self> { + let _lock = lock_launch(launch)?; + launch.run(&["fetch", "origin", base])?; + let (root, path) = sibling(launch, &format!("{repo}-architect"))?; + let start = format!("origin/{base}"); + progress::step(format_args!( + "creating worktree {} detached at {start}", + path.display() + )); + add_worktree(launch, &["--detach"], &path, &start)?; + Ok(ReviewWorktree { + launch: Git::new(root), + path, + }) + } + + pub fn path(&self) -> &Path { + &self.path + } +} + +impl Drop for ReviewWorktree { + fn drop(&mut self) { + progress::step("cleaning up the worktree"); + let _lock = lock_launch(&self.launch).inspect_err(|error| { + progress::step(format_args!("cleaning up without the lock: {error:#}")) + }); + let path = self.path.to_string_lossy(); + if let Err(error) = self.launch.run(&["worktree", "remove", "--force", &path]) { + progress::step(format_args!("cleanup incomplete: {error:#}")); + } + } +} + +/// The launch repository's root, and the path of the worktree `name` next to +/// it. +fn sibling(launch: &Git, name: &str) -> Result<(PathBuf, PathBuf)> { + let root = PathBuf::from(launch.run(&["rev-parse", "--show-toplevel"])?); + let path = root + .parent() + .context("the repository root has no parent directory")? + .join(name); + Ok((root, path)) +} + +/// `git worktree add <checkout> <path> <start>` in the launch repository, +/// where `checkout` says what the worktree is on: a branch, or nothing. +fn add_worktree(launch: &Git, checkout: &[&str], path: &Path, start: &str) -> Result<()> { + let path = path.to_str().context("worktree path is not UTF-8")?; + let mut args = vec!["worktree", "add"]; + args.extend_from_slice(checkout); + args.extend([path, start]); + launch.run(&args)?; + Ok(()) +} + /// Wait for, then hold until the file is dropped, the Launch directory's /// worktree lock, so the Runs of a Spec run's Tickets add and remove their /// worktrees and local Issue branches one at a time: `git worktree add -b` diff --git a/tests/architect_run.rs b/tests/architect_run.rs new file mode 100644 index 000000000..319723445 --- /dev/null +++ b/tests/architect_run.rs @@ -0,0 +1,1380 @@ +//! Architect runs: `thirdshift architect` runs the Architecture review in its +//! own worktree, detached at the Base branch's head on origin, then checks +//! the plan the review published and swaps its `needs-triage` for +//! `ready-for-agent`. With `--plan-only` it prints the plan's URL and stops. +//! Without, it dispatches the plan as `thirdshift <plan URL>` would, a Spec +//! run or a Run, and ends as that does. A review with no Strong candidate +//! has no plan: the Architect run prints the URL of the idea issue it filed, +//! or of the open issue that already covers it, and dispatches nothing. +//! Asked to, by `email` or the User config, it sends one Run notification, +//! through Resend, here a local stand-in, however it ended, and the run it +//! dispatched sends none. + +mod support; + +use std::fs; + +use support::resend::ResendStandIn; +use support::{REPO, RunResult, Scenario}; + +/// The first issue the fake agent creates: the scenario starts with issue #7. +const PLAN_URL: &str = "https://github.com/acme/widgets/issues/8"; + +/// The first pull request opened on the fake GitHub. +const PR_URL: &str = "https://github.com/acme/widgets/pull/1"; + +const NO_FINAL_LINE: &str = + "the Architecture review ended without the final line its prompt asks for"; + +/// The agent publishes a single Ticket as the plan, labelled `needs-triage`, +/// and names it in the last line of its final message. +const AGENT_PUBLISHES_A_TICKET: &str = r#" +url=$(gh issue create --title "Deepen the session module" --body "The plan" --label needs-triage) +printf 'Published the plan.\n\nArchitecture review plan: %s\n' "$url" > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#; + +/// A scenario whose repository has the labels the fake agent publishes its +/// issues with, as `gh issue create` refuses a label the repository lacks. +fn scenario() -> Scenario { + let scenario = Scenario::new(); + let mut gh = scenario.gh_state(); + gh["repo_labels"] = serde_json::json!(["needs-triage", "ready-for-agent", "architecture"]); + scenario.write_gh_state(&gh); + scenario +} + +/// A script in which the agent publishes the plan as +/// [`AGENT_PUBLISHES_A_TICKET`] does, then does `then`. +fn publishes_a_ticket_then(then: &str) -> String { + format!("{AGENT_PUBLISHES_A_TICKET}{then}\n") +} + +/// A script in which the agent for issue `issue` commits its work and opens +/// its PR into `base`, leaving the pushing to thirdshift. +fn agent_opens_pr(issue: u32, base: &str) -> String { + format!( + r#" +echo "{issue}" > issue-{issue}.txt +git add issue-{issue}.txt +git commit -q -m "Work on {issue}" +gh pr create --base {base} --head issue-{issue} --title "Work on {issue}" --body "Closes #{issue}" +"# + ) +} + +/// The Architecture review, the first session, publishes a single Ticket as +/// the plan, #8, and the session that implements #8 opens its PR into `main`. +fn single_ticket_plan() -> Scenario { + let scenario = scenario(); + scenario.agent_does_in_session(1, AGENT_PUBLISHES_A_TICKET); + scenario.agent_does_for(8, &agent_opens_pr(8, "main")); + scenario +} + +/// The Architecture review, the first session, publishes a Spec, #8, with +/// two Tickets that don't block each other, #9 and #10, as the plan. Each +/// Ticket's session opens its PR into the Spec branch, #10's after doing +/// `before_ticket_10`. +fn spec_plan(before_ticket_10: &str) -> Scenario { + let scenario = scenario(); + scenario.agent_does_in_session( + 1, + r#" +spec=$(gh issue create --title "Deepen the session module" --body "The Spec" --label needs-triage) +gh issue create --title "Move the logs" --body "A Ticket" --label ready-for-agent +gh issue create --title "Move the sessions" --body "A Ticket" --label ready-for-agent +gh fake sub-issues 8 '[9, 10]' +printf 'Architecture review plan: %s\n' "$spec" > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#, + ); + scenario.agent_does_for(9, &agent_opens_pr(9, "issue-8")); + scenario.agent_does_for( + 10, + &format!("{before_ticket_10}\n{}", agent_opens_pr(10, "issue-8")), + ); + scenario +} + +/// The newest pull request from `head` on the fake GitHub. +fn pr_from(scenario: &Scenario, head: &str) -> serde_json::Value { + let prs = scenario.gh_state()["prs"].clone(); + let from_head = prs + .as_array() + .unwrap() + .iter() + .rfind(|pr| pr["head"] == head); + from_head + .unwrap_or_else(|| panic!("no PR from {head}: {prs}")) + .clone() +} + +/// The issue number each agent session after the Architecture review was +/// for, the first Issue URL its prompt names, in order. +fn dispatched_sessions(scenario: &Scenario) -> Vec<String> { + scenario.claude_calls()[1..] + .iter() + .map(|call| { + let prompt = call["prompt"].as_str().unwrap(); + let (_, after) = prompt.split_once("/issues/").unwrap(); + after.chars().take_while(char::is_ascii_digit).collect() + }) + .collect() +} + +/// Assert the Architect run ended with `pr` in `outcome`, as a Run or a Spec +/// run that reached its goal does: exit 0, the PR's URL alone on stdout, and +/// the outcome as the last line on stderr. +fn assert_ended_with_pr(result: &RunResult, pr: &serde_json::Value, outcome: &str) { + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let url = pr["url"].as_str().unwrap(); + assert_eq!(result.stdout, format!("{url}\n")); + assert_eq!( + result.stderr.lines().last(), + Some(format!("thirdshift: PR {url} is {outcome}").as_str()), + "stderr: {}", + result.stderr + ); +} + +/// A script in which the agent ends with `final_message`, publishing nothing. +fn ends_with(final_message: &str) -> String { + format!("printf '%s\\n' '{final_message}' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"\n") +} + +/// Assert the Architect run left no worktree and no temp directory, and the +/// Launch directory with no branch but `main`. +fn assert_nothing_left_behind(scenario: &Scenario) { + assert_eq!(scenario.entries("work"), vec![REPO]); + assert_eq!( + scenario + .launch_git(&["worktree", "list", "--porcelain"]) + .matches("worktree ") + .count(), + 1 + ); + assert_eq!( + scenario.launch_git(&["branch", "--format=%(refname:short)"]), + "main\n" + ); + assert_eq!(scenario.entries("tmp"), Vec::<String>::new()); +} + +/// What every failed Architect run shares once its review has started: exit +/// 1, nothing on stdout, `cause` on stderr, then the session log as its last +/// line, and nothing left behind. +fn assert_failed(scenario: &Scenario, result: &RunResult, cause: &str) { + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert_eq!(result.stdout, ""); + assert!( + result.stderr.contains(&format!("thirdshift: {cause}\n")), + "expected {cause:?} in stderr: {}", + result.stderr + ); + let logs = scenario.entries("home/.thirdshift/logs"); + assert_eq!(logs.len(), 1, "logs: {logs:?}"); + assert!( + logs[0].starts_with("acme-widgets-architect-") + && logs[0].ends_with("-architecture-review.jsonl"), + "logs: {logs:?}" + ); + let log = scenario.path("home/.thirdshift/logs").join(&logs[0]); + assert_eq!( + result.stderr.lines().last(), + Some(format!("thirdshift: session log: {}", log.display()).as_str()), + "stderr: {}", + result.stderr + ); + assert_nothing_left_behind(scenario); +} + +/// What every Architect run whose review found no Strong candidate shares: +/// exit 0, `url` alone on stdout, `outcome` as stderr's last line, the +/// review as its only session, and nothing left behind. +fn assert_ended_without_a_plan(scenario: &Scenario, result: &RunResult, url: &str, outcome: &str) { + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{url}\n")); + assert_eq!( + result.stderr.lines().last(), + Some(format!("thirdshift: {outcome}").as_str()), + "stderr: {}", + result.stderr + ); + assert_eq!(scenario.claude_calls().len(), 1); + assert_nothing_left_behind(scenario); +} + +#[test] +fn plan_only_marks_the_published_ticket_ready_and_prints_its_url() { + let scenario = scenario(); + scenario.agent_does(AGENT_PUBLISHES_A_TICKET); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PLAN_URL}\n")); + assert_eq!(scenario.issue_labels(8), ["ready-for-agent"]); + assert_nothing_left_behind(&scenario); +} + +#[test] +fn plan_only_marks_a_published_spec_ready_and_leaves_its_tickets_and_other_labels() { + let scenario = scenario(); + scenario.agent_does( + r#" +spec=$(gh issue create --title "Deepen the session module" --body "The Spec" --label needs-triage,architecture) +gh issue create --title "Move the logs" --body "A Ticket" --label ready-for-agent +printf 'Architecture review plan: %s\n' "$spec" > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#, + ); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PLAN_URL}\n")); + assert_eq!( + scenario.issue_labels(8), + ["architecture", "ready-for-agent"] + ); + assert_eq!(scenario.issue_labels(9), ["ready-for-agent"]); +} + +#[test] +fn the_session_prompt_names_the_factory_skills_the_base_branch_and_the_final_line() { + let scenario = scenario(); + scenario.origin_has_branch("develop", "main", &["Develop work"]); + scenario.launch_checks_out("develop"); + scenario.agent_does(AGENT_PUBLISHES_A_TICKET); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let prompt = scenario.first_prompt(); + for expected in [ + "/thirdshift:improve-codebase-architecture", + "/thirdshift:codebase-design", + "/thirdshift:to-spec", + "/thirdshift:to-tickets", + "the base branch develop", + "\nArchitecture review plan: <", + "\nArchitecture review idea: <", + "\nArchitecture review already filed: <", + ] { + assert!(prompt.contains(expected), "expected {expected:?}: {prompt}"); + } + assert!(!prompt.contains("Focus"), "{prompt}"); +} + +#[test] +fn a_focus_goes_into_the_session_prompt() { + for args in [ + ["architect", "the Spec run", "--plan-only"], + ["architect", "--plan-only", "the Spec run"], + ] { + let scenario = scenario(); + scenario.agent_does(AGENT_PUBLISHES_A_TICKET); + + let result = scenario.run(&args); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let prompt = scenario.first_prompt(); + assert!( + prompt.contains("\nFocus the review on: the Spec run\n"), + "{args:?}: {prompt}" + ); + } +} + +#[test] +fn the_review_runs_detached_at_origins_base_branch_and_the_launch_directory_is_never_touched() { + let scenario = scenario(); + scenario.origin_has_commit("main", "upstream.txt", "upstream\n", "Upstream work"); + let launch = scenario.launch_dir(); + fs::write(launch.join("README.md"), "widgets, edited\n").unwrap(); + fs::write(launch.join("untracked.txt"), "mine\n").unwrap(); + let launch_head = scenario.launch_git(&["rev-parse", "HEAD"]); + let launch_status = scenario.launch_git(&["status", "--porcelain"]); + let origin_refs = scenario.origin_git(&["for-each-ref"]); + scenario.agent_does(&publishes_a_ticket_then( + r#" +root=$(dirname "$FAKE_CLAUDE_RECORD") +git rev-parse HEAD > "$root/review-head" +ls > "$root/review-files" +echo "a throwaway prototype" > prototype.txt +echo "widgets, by the review" > README.md +"#, + )); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let call = &scenario.claude_calls()[0]; + assert_eq!( + call["cwd"], + scenario.path("work/widgets-architect").to_str().unwrap() + ); + assert_eq!(call["branch"], "", "the worktree is on a branch"); + assert_eq!( + fs::read_to_string(scenario.path("review-head")).unwrap(), + scenario.origin_git(&["rev-parse", "refs/heads/main"]) + ); + assert_eq!( + fs::read_to_string(scenario.path("review-files")).unwrap(), + "README.md\nupstream.txt\n" + ); + assert_eq!(scenario.launch_git(&["rev-parse", "HEAD"]), launch_head); + assert_eq!( + scenario.launch_git(&["status", "--porcelain"]), + launch_status + ); + assert_eq!( + fs::read_to_string(launch.join("README.md")).unwrap(), + "widgets, edited\n" + ); + assert_eq!(scenario.origin_git(&["for-each-ref"]), origin_refs); + assert_nothing_left_behind(&scenario); +} + +#[test] +fn progress_lines_cover_the_review_starting_the_plan_it_reported_and_the_label_swap() { + let scenario = scenario(); + scenario.agent_does(AGENT_PUBLISHES_A_TICKET); + + let result = scenario.run(&["architect", "the Spec run", "--plan-only"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let mut rest = result.stderr.as_str(); + for line in [ + "thirdshift: starting the Architecture review of main, focused on: the Spec run\n" + .to_string(), + "thirdshift: architecture-review: session started\n".to_string(), + format!("thirdshift: the Architecture review published the plan {PLAN_URL}\n"), + "thirdshift: marking the plan ready: swapping needs-triage for ready-for-agent on #8\n" + .to_string(), + ] { + let Some(at) = rest.find(&line) else { + panic!("expected {line:?}, in order, in stderr: {}", result.stderr); + }; + rest = &rest[at + line.len()..]; + } + assert_eq!( + result.stderr.lines().last(), + Some(format!("thirdshift: plan {PLAN_URL} is ready for an agent").as_str()) + ); +} + +#[test] +fn a_failing_session_fails_the_architect_run_and_leaves_the_plan_needing_triage() { + let scenario = scenario(); + scenario.agent_does(&publishes_a_ticket_then("exit 3")); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_failed(&scenario, &result, "claude exited 3"); + assert_eq!(scenario.issue_labels(8), ["needs-triage"]); +} + +#[test] +fn an_interrupted_session_fails_the_architect_run_and_leaves_the_plan_needing_triage() { + let scenario = scenario(); + scenario.agent_does(&publishes_a_ticket_then( + r#"touch "$(dirname "$FAKE_CLAUDE_RECORD")/started" +sleep 60"#, + )); + + let result = scenario.run_and_signal(&["architect", "--plan-only"], "started", "INT"); + + assert_failed(&scenario, &result, "interrupted"); + assert_eq!(scenario.issue_labels(8), ["needs-triage"]); +} + +#[test] +fn a_session_that_ends_without_a_valid_final_line_fails_the_architect_run() { + for final_message in [ + None, + Some("Published the plan: https://github.com/acme/widgets/issues/8"), + Some("Architecture review plan: #8"), + Some("Architecture review plan: https://github.com/acme/widgets/issues/8, a Ticket"), + ] { + let scenario = scenario(); + let publish = "gh issue create --title Plan --body Plan --label needs-triage > /dev/null\n"; + let ending = final_message.map(ends_with).unwrap_or_default(); + scenario.agent_does(&format!("{publish}{ending}")); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_failed(&scenario, &result, NO_FINAL_LINE); + assert_eq!( + scenario.issue_labels(8), + ["needs-triage"], + "{final_message:?}" + ); + } +} + +#[test] +fn a_review_that_files_an_idea_prints_its_url_and_changes_no_label() { + let scenario = scenario(); + let idea = scenario.issue_url(8); + scenario.agent_does( + r#" +url=$(gh issue create --title "Deepen the session module" --body "The idea" --label needs-triage) +printf 'No Strong candidate.\n\nArchitecture review idea: %s\n' "$url" > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#, + ); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_ended_without_a_plan( + &scenario, + &result, + &idea, + &format!("no Strong candidate: the Architecture review filed the idea {idea}"), + ); + assert_eq!(scenario.issue_labels(8), ["needs-triage"]); +} + +#[test] +fn a_review_whose_idea_is_already_filed_prints_that_issues_url_and_files_and_changes_nothing() { + let scenario = scenario(); + scenario.issue_labelled(7, &["needs-triage"]); + let url = scenario.issue_url(7); + scenario.agent_does(&ends_with(&format!( + "Architecture review already filed: {url}" + ))); + let github = scenario.gh_state(); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_ended_without_a_plan( + &scenario, + &result, + &url, + &format!( + "no Strong candidate: {url} already covers the Architecture review's top recommendation, so it filed nothing" + ), + ); + assert_eq!(scenario.gh_state(), github); +} + +#[test] +fn a_review_with_no_strong_candidate_dispatches_nothing_without_plan_only() { + let scenario = scenario(); + scenario.issue_labelled(7, &["needs-triage"]); + let url = scenario.issue_url(7); + scenario.agent_does(&ends_with(&format!("Architecture review idea: {url}"))); + let github = scenario.gh_state(); + + let result = scenario.run(&["architect", "merge"]); + + assert_ended_without_a_plan( + &scenario, + &result, + &url, + &format!("no Strong candidate: the Architecture review filed the idea {url}"), + ); + assert_eq!(scenario.gh_state(), github); +} + +#[test] +fn a_closed_plan_is_refused() { + let scenario = scenario(); + scenario.agent_does(&publishes_a_ticket_then("gh fake issue 8 CLOSED")); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_failed( + &scenario, + &result, + &format!("the plan {PLAN_URL} is closed"), + ); + assert_eq!(scenario.issue_labels(8), ["needs-triage"]); +} + +#[test] +fn a_plan_older_than_the_architect_run_is_refused() { + let scenario = scenario(); + scenario.issue_labelled(7, &["needs-triage"]); + scenario.issue_created(7, "2026-09-30T23:59:59Z"); + let url = scenario.issue_url(7); + scenario.agent_does(&ends_with(&format!("Architecture review plan: {url}"))); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_failed( + &scenario, + &result, + &format!("the plan {url} was created before this Architect run started"), + ); + assert_eq!(scenario.issue_labels(7), ["needs-triage"]); +} + +#[test] +fn a_plan_with_another_unready_label_is_refused() { + for label in ["ready-for-human", "needs-info", "wontfix"] { + let scenario = scenario(); + scenario.agent_does(&publishes_a_ticket_then(&format!( + r#"gh fake labels 8 '["needs-triage", "{label}"]'"# + ))); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_failed( + &scenario, + &result, + &format!("the plan {PLAN_URL} is labelled {label}"), + ); + assert_eq!(scenario.issue_labels(8), ["needs-triage", label]); + } +} + +#[test] +fn a_plan_in_another_repository_is_refused() { + let scenario = scenario(); + let url = "https://github.com/acme/gadgets/issues/8"; + scenario.agent_does(&ends_with(&format!("Architecture review plan: {url}"))); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_failed( + &scenario, + &result, + &format!( + "the plan {url} is not in the repository at origin https://github.com/acme/widgets.git" + ), + ); +} + +#[test] +fn a_detached_head_is_rejected_before_any_work() { + let scenario = scenario(); + scenario.launch_git(&["checkout", "-q", "--detach"]); + + let result = scenario.run(&["architect", "--plan-only"]); + + scenario.assert_rejected_before_any_work( + &result, + "HEAD is detached; check out the branch the Architecture review should scan", + ); +} + +#[test] +fn a_base_branch_ahead_of_origin_is_rejected_before_any_work() { + let scenario = scenario(); + scenario.commit_locally("local.txt", "local\n", "Local work"); + + let result = scenario.run(&["architect", "--plan-only"]); + + scenario.assert_rejected_before_any_work( + &result, + "local main is 1 commit(s) ahead of origin/main; push them first", + ); +} + +#[test] +fn a_missing_git_identity_is_rejected_before_any_work() { + let scenario = scenario(); + scenario.git_email_is(None); + + let result = scenario.run(&["architect", "--plan-only"]); + + scenario.assert_rejected_before_any_work(&result, "git user.email is not set"); +} + +#[test] +fn an_origin_that_is_not_on_github_is_rejected_before_any_work() { + let scenario = scenario(); + scenario.set_origin_url("https://gitlab.com/acme/widgets.git"); + + let result = scenario.run(&["architect", "--plan-only"]); + + scenario.assert_rejected_before_any_work( + &result, + "origin https://gitlab.com/acme/widgets.git is not a GitHub repository", + ); +} + +#[test] +fn a_closed_issue_in_the_repository_does_not_stop_an_architect_run() { + let scenario = scenario(); + scenario.issue_is(7, "CLOSED"); + scenario.agent_does(AGENT_PUBLISHES_A_TICKET); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PLAN_URL}\n")); +} + +#[test] +fn launch_pull_brings_the_launch_directorys_base_branch_up_to_date_first() { + let scenario = scenario(); + scenario.origin_has_commit("main", "upstream.txt", "upstream\n", "Upstream work"); + scenario.user_config_is("[launch]\npull = true\n"); + scenario.agent_does(AGENT_PUBLISHES_A_TICKET); + + let result = scenario.run(&["architect", "--plan-only"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!( + scenario.launch_git(&["rev-parse", "refs/heads/main"]), + scenario.origin_git(&["rev-parse", "refs/heads/main"]) + ); + assert!( + result + .stderr + .contains("thirdshift: updating main in the Launch directory from origin/main\n"), + "stderr: {}", + result.stderr + ); +} + +#[test] +fn a_single_ticket_plan_starts_a_run_on_it_and_ends_as_that_run_does() { + let scenario = single_ticket_plan(); + + let result = scenario.run(&["architect"]); + + let pr = pr_from(&scenario, "issue-8"); + assert_ended_with_pr(&result, &pr, "ready for review"); + assert_eq!(pr["url"], PR_URL); + assert_eq!(scenario.issue_labels(8), ["ready-for-agent"]); + let calls = scenario.claude_calls(); + assert_eq!(calls.len(), 2, "sessions: {calls:?}"); + let prompt = calls[1]["prompt"].as_str().unwrap(); + assert!( + prompt.starts_with(&format!("/thirdshift:implement {PLAN_URL}\n")), + "{prompt}" + ); + assert_eq!(calls[1]["branch"], "issue-8"); + assert_eq!(pr["base"], "main"); + assert_eq!(pr["state"], "OPEN"); + assert_eq!( + scenario.origin_log("issue-8").unwrap()[0], + "Work on 8".to_string() + ); + scenario.assert_cleaned_up("issue-8"); +} + +#[test] +fn a_plan_with_tickets_starts_a_spec_run_on_it_and_ends_as_that_spec_run_does() { + let scenario = spec_plan(""); + + let result = scenario.run(&["architect"]); + + let spec_pr = pr_from(&scenario, "issue-8"); + assert_ended_with_pr(&result, &spec_pr, "ready for review"); + assert_eq!(spec_pr["base"], "main"); + assert_eq!(spec_pr["state"], "OPEN"); + assert_eq!(spec_pr["isDraft"], false); + assert_eq!(scenario.issue_labels(8), ["ready-for-agent"]); + let dispatch = format!("dispatching the plan {PLAN_URL}, as thirdshift {PLAN_URL} would\n"); + let dispatched = result.stderr.find(&dispatch); + let started = result.stderr.find("thirdshift: starting #9\n"); + assert!( + dispatched.is_some() && dispatched < started, + "stderr: {}", + result.stderr + ); + let mut sessions = dispatched_sessions(&scenario); + assert_eq!(sessions.pop().as_deref(), Some("8"), "the Spec review"); + sessions.sort(); + assert_eq!(sessions, ["10", "9"]); + let gh = scenario.gh_state(); + for ticket in [9, 10] { + let pr = pr_from(&scenario, &format!("issue-{ticket}")); + assert_eq!(pr["base"], "issue-8"); + assert_eq!(pr["state"], "MERGED"); + assert_eq!(gh["issues"][ticket.to_string()], "CLOSED"); + assert_eq!( + scenario.origin_file("issue-8", &format!("issue-{ticket}.txt")), + Some(format!("{ticket}\n")) + ); + } + scenario.assert_cleaned_up("issue-8"); +} + +#[test] +fn merge_merges_the_runs_pull_request_or_the_spec_pr() { + for scenario in [single_ticket_plan(), spec_plan("")] { + let result = scenario.run(&["architect", "merge"]); + + let pr = pr_from(&scenario, "issue-8"); + assert_ended_with_pr(&result, &pr, "merged"); + assert_eq!(pr["base"], "main"); + assert_eq!(pr["state"], "MERGED"); + assert!( + scenario.origin_log("issue-8").is_none(), + "issue-8 is still on origin" + ); + } +} + +#[test] +fn the_user_configs_merge_default_applies_unless_no_merge_is_given() { + for plan in [single_ticket_plan, || spec_plan("")] { + for (args, outcome, state) in [ + (vec!["architect"], "merged", "MERGED"), + (vec!["architect", "--no-merge"], "ready for review", "OPEN"), + ] { + let scenario = plan(); + scenario.user_config_is("[merge]\nalways = true\n"); + + let result = scenario.run(&args); + + let pr = pr_from(&scenario, "issue-8"); + assert_ended_with_pr(&result, &pr, outcome); + assert_eq!(pr["state"], state, "{args:?}"); + } + } +} + +#[test] +fn parallel_passes_through_to_the_spec_run() { + // #10's session only succeeds if #9 landed on the Spec branch before it + // started, as it has only when the Tickets run one at a time. + let scenario = spec_plan("test -f issue-9.txt"); + + let result = scenario.run(&["architect", "parallel", "1"]); + + assert_ended_with_pr(&result, &pr_from(&scenario, "issue-8"), "ready for review"); + assert_eq!(dispatched_sessions(&scenario), ["9", "10", "8"]); +} + +#[test] +fn parallel_on_a_single_ticket_plan_fails_as_it_does_for_an_issue_that_is_not_a_spec() { + let scenario = single_ticket_plan(); + + let result = scenario.run(&["architect", "parallel", "2"]); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert_eq!(result.stdout, ""); + assert_eq!( + result.stderr.lines().last(), + Some("thirdshift: parallel is only for a Spec, and #8 has no sub-issues"), + "stderr: {}", + result.stderr + ); + assert_eq!(scenario.claude_calls().len(), 1, "a Run was started"); + assert_eq!(scenario.issue_labels(8), ["ready-for-agent"]); + assert!(scenario.origin_log("issue-8").is_none()); + assert_nothing_left_behind(&scenario); +} + +const RED: &str = r#"[{"name": "test", "conclusion": "failure"}]"#; + +const GREEN: &str = r#"[{"name": "test", "conclusion": "success"}]"#; + +/// A script that sets the check runs on the session's head to `checks`. +fn checks_on_head(checks: &str) -> String { + format!("gh fake checks \"$(git rev-parse HEAD)\" '{checks}'\n") +} + +/// A script that sets the check runs on origin's `branch` to `checks`. +fn checks_on_origin(branch: &str, checks: &str) -> String { + format!("gh fake checks \"$(git rev-parse origin/{branch})\" '{checks}'\n") +} + +/// A script in which the agent for the Base fix issue `issue` commits a fix +/// and opens its PR into `base`, with `test` green on its head. +fn base_fix_opens_pr(issue: u32, base: &str) -> String { + format!( + r#" +echo "fixed" > ci-fix.txt +git add ci-fix.txt +git commit -q -m "Fix CI on {base}" +gh pr create --base {base} --head issue-{issue} --title "Fix CI on {base}" --body "Closes #{issue}" +{green}"#, + green = checks_on_head(GREEN) + ) +} + +/// A single-Ticket plan, #8, whose Run opens its PR with `test` red on its +/// head and on `main`: an Inherited failure. The agent for the Base fix +/// issue, #9, the next issue, fixes it. +fn single_ticket_plan_that_inherits_a_failure() -> Scenario { + let scenario = single_ticket_plan(); + scenario.agent_does_for( + 8, + &format!( + "{}{}{}", + agent_opens_pr(8, "main"), + checks_on_head(RED), + checks_on_origin("main", RED) + ), + ); + scenario.agent_does_for(9, &base_fix_opens_pr(9, "main")); + scenario +} + +/// The cause of a Run that failed on `test`, an Inherited failure from +/// `main` as it was when the scenario started. +fn inherited_failure(scenario: &Scenario) -> String { + let red_base = scenario.origin_git(&["rev-parse", "main"]); + format!( + "CI red on test, which also fails on main at {}; fix main first", + &red_base[..7] + ) +} + +#[test] +fn base_fix_lets_the_dispatched_run_start_a_base_fix() { + for flag in ["base-fix", "--base-fix"] { + let scenario = single_ticket_plan_that_inherits_a_failure(); + + let result = scenario.run(&["architect", flag]); + + let pr = pr_from(&scenario, "issue-8"); + assert_ended_with_pr(&result, &pr, "ready for review"); + assert!( + result.stderr.contains( + "thirdshift: starting Base fix #9 into main: https://github.com/acme/widgets/issues/9\n" + ), + "stderr: {}", + result.stderr + ); + let gh = scenario.gh_state(); + assert_eq!(gh["titles"]["9"], "CI red on main: test", "{flag}"); + let fix = pr_from(&scenario, "issue-9"); + assert_eq!(fix["base"], "main"); + assert_eq!(fix["state"], "MERGED"); + assert_eq!( + scenario.origin_file("issue-8", "ci-fix.txt").as_deref(), + Some("fixed\n") + ); + } +} + +#[test] +fn the_user_configs_base_fix_applies_to_the_dispatched_run_unless_no_base_fix_is_given() { + let scenario = single_ticket_plan_that_inherits_a_failure(); + scenario.user_config_is("[base]\nfix = true\n"); + + let result = scenario.run(&["architect"]); + + assert_ended_with_pr(&result, &pr_from(&scenario, "issue-8"), "ready for review"); + assert_eq!(pr_from(&scenario, "issue-9")["state"], "MERGED"); + + for flag in ["no-base-fix", "--no-base-fix"] { + let scenario = single_ticket_plan_that_inherits_a_failure(); + scenario.user_config_is("[base]\nfix = true\n"); + let cause = inherited_failure(&scenario); + + let result = scenario.run(&["architect", flag]); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert!( + result.stderr.contains(&format!("thirdshift: {cause}\n")), + "stderr: {}", + result.stderr + ); + // Only the plan was created: no Base fix issue. + assert_eq!(scenario.gh_calls_of("issue", "create").len(), 1, "{flag}"); + } +} + +#[test] +fn without_base_fix_the_dispatched_run_fails_on_an_inherited_failure() { + let scenario = single_ticket_plan_that_inherits_a_failure(); + let cause = inherited_failure(&scenario); + + let result = scenario.run(&["architect"]); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert!( + result.stderr.contains(&format!("thirdshift: {cause}\n")), + "stderr: {}", + result.stderr + ); + assert_eq!(scenario.gh_calls_of("issue", "create").len(), 1); +} + +#[test] +fn base_fix_or_the_user_configs_reaches_each_tickets_run_of_the_dispatched_spec_run() { + for (args, config) in [ + (vec!["architect", "base-fix", "parallel", "1"], None), + ( + vec!["architect", "parallel", "1"], + Some("[base]\nfix = true\n"), + ), + ] { + // The first Ticket's Run, #9's, meets an Inherited failure from the + // Spec branch. One Ticket at a time, so the Spec branch can't move + // under it, which would have it merged in again instead. The Base + // fix issue is the next after the Tickets, #11. + let scenario = spec_plan(""); + scenario.agent_does_for( + 9, + &format!( + "{}{}{}", + agent_opens_pr(9, "issue-8"), + checks_on_head(RED), + checks_on_origin("issue-8", RED) + ), + ); + scenario.agent_does_for(11, &base_fix_opens_pr(11, "issue-8")); + if let Some(config) = config { + scenario.user_config_is(config); + } + + let result = scenario.run(&args); + + assert_ended_with_pr(&result, &pr_from(&scenario, "issue-8"), "ready for review"); + assert!( + result.stderr.contains( + "#9: starting Base fix #11 into issue-8: https://github.com/acme/widgets/issues/11\n" + ), + "{args:?}: stderr: {}", + result.stderr + ); + let fix = pr_from(&scenario, "issue-11"); + assert_eq!(fix["base"], "issue-8"); + assert_eq!(fix["state"], "MERGED"); + for ticket in ["issue-9", "issue-10"] { + assert_eq!(pr_from(&scenario, ticket)["state"], "MERGED", "{ticket}"); + } + for file in ["issue-9.txt", "issue-10.txt", "ci-fix.txt"] { + assert!(scenario.origin_file("issue-8", file).is_some(), "{file}"); + } + } +} + +#[test] +fn a_dispatched_run_that_fails_fails_the_architect_run_as_a_failed_run_does() { + let scenario = single_ticket_plan(); + scenario.agent_does_for(8, &format!("{}exit 3\n", agent_opens_pr(8, "main"))); + + let result = scenario.run(&["architect"]); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PR_URL}\n")); + assert!( + result.stderr.contains("thirdshift: claude exited 3\n"), + "stderr: {}", + result.stderr + ); + let last = result.stderr.lines().last().unwrap(); + assert!( + last.starts_with("thirdshift: session log: ") && last.ends_with("-implement.jsonl"), + "stderr: {}", + result.stderr + ); + assert_eq!(scenario.issue_labels(8), ["ready-for-agent"]); +} + +#[test] +fn progress_lines_show_the_dispatch_after_the_label_swap_and_before_the_run() { + let scenario = single_ticket_plan(); + + let result = scenario.run(&["architect"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let mut rest = result.stderr.as_str(); + for line in [ + "thirdshift: marking the plan ready: swapping needs-triage for ready-for-agent on #8\n" + .to_string(), + format!("thirdshift: dispatching the plan {PLAN_URL}, as thirdshift {PLAN_URL} would\n"), + "thirdshift: implement: session started\n".to_string(), + ] { + let Some(at) = rest.find(&line) else { + panic!("expected {line:?}, in order, in stderr: {}", result.stderr); + }; + rest = &rest[at + line.len()..]; + } + assert!( + !result.stderr.contains("is ready for an agent"), + "stderr: {}", + result.stderr + ); +} + +const KEY: &str = "re_test_123"; + +const ACCEPTED: &str = r#"{"id":"49a3999c-0ce1-4ea6-ab68-afcd6dc2e794"}"#; + +/// The environment for an Architect run against `resend`, with a Resend API +/// key. +fn resend_env(resend: &ResendStandIn) -> [(&str, &str); 2] { + [ + ("THIRDSHIFT_RESEND_URL", resend.url()), + ("RESEND_API_KEY", KEY), + ] +} + +/// Run thirdshift with `args` against `resend`, with a Resend API key in the +/// environment. +fn run_with_resend(scenario: &Scenario, resend: &ResendStandIn, args: &[&str]) -> RunResult { + scenario.run_with_env(args, &resend_env(resend)) +} + +/// The subject and the text of the one Run notification `resend` received. +fn the_one_notification(resend: &ResendStandIn) -> (String, String) { + let requests = resend.requests(); + assert_eq!(requests.len(), 1, "{requests:?}"); + let body = &requests[0].body; + let part = |name: &str| body[name].as_str().unwrap().to_string(); + (part("subject"), part("text")) +} + +#[test] +fn plan_only_sends_one_notification_naming_the_plan() { + let scenario = scenario(); + scenario.agent_does(AGENT_PUBLISHES_A_TICKET); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "--plan-only", "--email", "me@example.com"], + ); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PLAN_URL}\n")); + assert_eq!(resend.requests()[0].body["to"], "me@example.com"); + let (subject, text) = the_one_notification(&resend); + assert_eq!( + subject, + "[thirdshift] acme/widgets Architect run: plan published" + ); + assert!( + text.starts_with(&format!("Review: plan published: {PLAN_URL}\n")), + "{text}" + ); + assert!(!text.contains("Dispatched:"), "{text}"); + assert!(text.contains("Took:"), "{text}"); +} + +#[test] +fn a_review_that_files_an_idea_sends_one_notification_naming_the_idea() { + let scenario = scenario(); + scenario.agent_does( + r#" +url=$(gh issue create --title "Deepen the session module" --body "The idea" --label needs-triage) +printf 'Architecture review idea: %s\n' "$url" > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#, + ); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "email", "me@example.com"], + ); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let (subject, text) = the_one_notification(&resend); + assert_eq!( + subject, + "[thirdshift] acme/widgets Architect run: idea filed" + ); + assert!( + text.starts_with(&format!("Review: idea filed: {PLAN_URL}\n")), + "{text}" + ); + assert!(!text.contains("Dispatched:"), "{text}"); +} + +#[test] +fn a_review_whose_idea_is_already_filed_sends_one_notification_naming_that_issue() { + let scenario = scenario(); + let url = scenario.issue_url(7); + scenario.agent_does(&ends_with(&format!( + "Architecture review already filed: {url}" + ))); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "--email", "me@example.com", "--plan-only"], + ); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let (subject, text) = the_one_notification(&resend); + assert_eq!( + subject, + "[thirdshift] acme/widgets Architect run: idea already filed" + ); + assert!( + text.starts_with(&format!("Review: idea already filed: {url}\n")), + "{text}" + ); +} + +#[test] +fn a_failed_review_sends_one_notification_with_the_cause_and_the_session_log() { + let scenario = scenario(); + scenario.agent_does(&publishes_a_ticket_then("exit 3")); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "--email", "me@example.com"], + ); + + assert_failed(&scenario, &result, "claude exited 3"); + let (subject, text) = the_one_notification(&resend); + assert_eq!( + subject, + "[thirdshift] acme/widgets Architect run: review failed" + ); + let log = result.stderr.lines().last().unwrap(); + let log = log.strip_prefix("thirdshift: session log: ").unwrap(); + assert!( + text.starts_with(&format!( + "Review: failed\n\ + Cause: claude exited 3\n\ + Session log: {log}\n" + )), + "{text}" + ); +} + +/// The User config of a machine where every Run and every Architect run +/// sends a Run notification. +const EMAIL_ALWAYS: &str = "[email]\nalways = true\nto = \"config@example.com\"\n"; + +#[test] +fn email_always_sends_one_notification_for_the_review_and_the_run_it_dispatched() { + let scenario = single_ticket_plan(); + scenario.user_config_is(EMAIL_ALWAYS); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend(&scenario, &resend, &["architect"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PR_URL}\n")); + assert_eq!(resend.requests()[0].body["to"], "config@example.com"); + let (subject, text) = the_one_notification(&resend); + assert_eq!( + subject, + "[thirdshift] acme/widgets Architect run: ready for review" + ); + assert!( + text.starts_with(&format!( + "Review: plan published: {PLAN_URL}\n\ + Dispatched: ready for review\n\ + Pull request: {PR_URL}\n" + )), + "{text}" + ); +} + +#[test] +fn the_one_notification_says_what_became_of_the_dispatched_runs_base_fix() { + let scenario = single_ticket_plan_that_inherits_a_failure(); + scenario.user_config_is(EMAIL_ALWAYS); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend(&scenario, &resend, &["architect", "base-fix"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let (subject, text) = the_one_notification(&resend); + assert_eq!( + subject, + "[thirdshift] acme/widgets Architect run: ready for review" + ); + assert!( + text.starts_with(&format!( + "Review: plan published: {PLAN_URL}\n\ + Dispatched: ready for review\n\ + Pull request: {PR_URL}\n\ + Base fix: https://github.com/acme/widgets/issues/9 merged\n" + )), + "{text}" + ); +} + +#[test] +fn a_dispatched_spec_run_sends_no_notification_of_its_own_and_its_tickets_are_in_the_one_sent() { + let scenario = spec_plan(""); + scenario.user_config_is(EMAIL_ALWAYS); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend(&scenario, &resend, &["architect", "merge"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let (subject, text) = the_one_notification(&resend); + assert_eq!(subject, "[thirdshift] acme/widgets Architect run: merged"); + let spec_pr = pr_from(&scenario, "issue-8"); + assert!( + text.starts_with(&format!( + "Review: plan published: {PLAN_URL}\n\ + Dispatched: merged\n\ + Pull request: {}\n", + spec_pr["url"].as_str().unwrap() + )), + "{text}" + ); + let (_, tickets) = text.split_once("\nTickets:\n").expect(&text); + for ticket in [9, 10] { + let pr = pr_from(&scenario, &format!("issue-{ticket}")); + let line = format!("#{ticket} landed with {}\n", pr["url"].as_str().unwrap()); + assert!(tickets.contains(&line), "expected {line:?} in: {text}"); + } +} + +#[test] +fn a_dispatched_run_that_fails_sends_one_notification_with_its_outcome_and_cause() { + let scenario = single_ticket_plan(); + scenario.agent_does_for(8, &format!("{}exit 3\n", agent_opens_pr(8, "main"))); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "--email", "me@example.com"], + ); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + let (subject, text) = the_one_notification(&resend); + assert_eq!(subject, "[thirdshift] acme/widgets Architect run: failed"); + assert!( + text.starts_with(&format!( + "Review: plan published: {PLAN_URL}\n\ + Dispatched: failed\n\ + Pull request: {PR_URL}\n\ + Cause: claude exited 3\n" + )), + "{text}" + ); +} + +#[test] +fn no_email_skips_the_notification_email_always_asks_for() { + let scenario = single_ticket_plan(); + scenario.user_config_is(EMAIL_ALWAYS); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend(&scenario, &resend, &["architect", "--no-email"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert!(resend.requests().is_empty()); +} + +#[test] +fn an_architect_run_that_is_not_asked_for_a_notification_sends_none() { + let scenario = single_ticket_plan(); + scenario.user_config_is("[email]\nto = \"config@example.com\"\n"); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend(&scenario, &resend, &["architect"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert!(resend.requests().is_empty()); +} + +#[test] +fn with_no_address_known_the_architect_run_stops_before_any_work_and_sends_nothing() { + let scenario = scenario(); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend(&scenario, &resend, &["architect", "--email"]); + + scenario.assert_rejected_before_any_work(&result, "no email address"); + assert!(resend.requests().is_empty()); +} + +#[test] +fn a_failed_send_is_a_warning_that_changes_neither_the_exit_code_nor_stdout() { + let scenario = scenario(); + scenario.agent_does(AGENT_PUBLISHES_A_TICKET); + let resend = ResendStandIn::replying(500, "upstream exploded"); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "--plan-only", "--email", "me@example.com"], + ); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PLAN_URL}\n")); + assert_eq!(resend.requests().len(), 1); + let warning = result.stderr.lines().find(|line| line.contains("warning:")); + assert!( + warning.is_some_and(|warning| warning.contains("upstream exploded")), + "stderr: {}", + result.stderr + ); +} + +#[test] +fn an_interrupted_review_sends_one_notification_that_it_was_interrupted() { + let scenario = scenario(); + scenario.agent_does( + r#"touch "$(dirname "$FAKE_CLAUDE_RECORD")/started" +sleep 60"#, + ); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = scenario.run_and_signal_with_env( + &["architect", "--email", "me@example.com"], + &resend_env(&resend), + "started", + "TERM", + ); + + assert_failed(&scenario, &result, "interrupted"); + let (subject, text) = the_one_notification(&resend); + assert_eq!( + subject, + "[thirdshift] acme/widgets Architect run: interrupted" + ); + assert!(text.starts_with("Review: interrupted\n"), "{text}"); + assert!(!text.contains("Cause:"), "{text}"); +} + +#[test] +fn a_plan_that_fails_its_checks_sends_one_notification_naming_it_in_the_cause() { + let scenario = scenario(); + scenario.agent_does(&publishes_a_ticket_then("gh fake issue 8 CLOSED")); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "--email", "me@example.com"], + ); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + let (subject, text) = the_one_notification(&resend); + assert_eq!( + subject, + "[thirdshift] acme/widgets Architect run: review failed" + ); + assert!( + text.starts_with(&format!( + "Review: failed\n\ + Cause: the plan {PLAN_URL} is closed\n" + )), + "{text}" + ); +} + +#[test] +fn a_dispatched_spec_run_that_fails_sends_one_notification_with_each_tickets_outcome() { + let scenario = spec_plan("exit 3"); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "--email", "me@example.com"], + ); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + let (subject, text) = the_one_notification(&resend); + assert_eq!(subject, "[thirdshift] acme/widgets Architect run: failed"); + assert!( + text.starts_with(&format!( + "Review: plan published: {PLAN_URL}\n\ + Dispatched: failed\n" + )), + "{text}" + ); + let (_, tickets) = text.split_once("\nTickets:\n").expect(&text); + let landed = pr_from(&scenario, "issue-9"); + let landed = format!("#9 landed with {}\n", landed["url"].as_str().unwrap()); + assert!(tickets.contains(&landed), "expected {landed:?} in: {text}"); + assert!(tickets.contains("#10 failed: "), "{text}"); +} diff --git a/tests/command_surface.rs b/tests/command_surface.rs index 73ba8dc0e..b061c88e9 100644 --- a/tests/command_surface.rs +++ b/tests/command_surface.rs @@ -12,6 +12,8 @@ fn assert_help_text(text: &str) { "thirdshift <Issue URL>", "thirdshift merge <Issue URL>", "thirdshift --email <Issue URL>", + "thirdshift architect [<focus>]", + "thirdshift architect [<focus>] --plan-only", "thirdshift email-test [<address>]", "thirdshift setup", "thirdshift update", @@ -66,6 +68,91 @@ fn help_does_not_mention_the_flag_aliases() { } } +#[test] +fn help_documents_architect_its_focus_the_dispatch_plan_only_and_one_architect_run_at_a_time() { + let scenario = Scenario::new(); + + let help = scenario.run(&["help"]).stdout; + + // However the lines are wrapped. + let help = help.split_whitespace().collect::<Vec<_>>().join(" "); + for mention in [ + "<focus> is free text", + "dispatches it as thirdshift <Issue URL> would: a Spec run on a Spec, a Run on a single Ticket", + "merge, --no-merge, base-fix, --no-base-fix and parallel <n> apply to that run", + "as do the User config's defaults", + "With --plan-only, the Architect run prints the plan's URL and stops instead", + "thirdshift architect \"the Spec run\"", + "A review that finds no Strong candidate publishes no plan", + "thirdshift prints that issue's URL instead, changing no label", + "Start one Architect run per repository at a time", + "--email, --email <address> and --no-email ask an Architect run for its Run notification", + "It sends one for the whole Architect run, however it ends", + "The run the plan is dispatched as sends none of its own", + ] { + assert!(help.contains(mention), "help lacks {mention:?}: {help}"); + } +} + +const PLAN_ONLY_DISPATCHES_NOTHING: &str = "merge, no-merge, parallel, base-fix and no-base-fix can't be used with \ + --plan-only: it dispatches no run for them to apply to"; + +#[test] +fn architect_with_arguments_it_cant_use_prints_an_error_and_the_help_to_stderr() { + let scenario = Scenario::new(); + + for (args, error) in [ + ( + vec!["architect", "the Spec run", "the Run", "--plan-only"], + "unexpected argument after the focus: the Run", + ), + ( + vec!["architect", "--plan-only", "--verbose"], + "unexpected argument after architect: --verbose", + ), + ( + vec!["architect", "merge", "no-merge"], + "merge and no-merge can't be used together", + ), + ( + vec!["architect", "parallel", "0"], + "parallel must be followed by a whole number from 1 up, not 0", + ), + ( + vec!["architect", "--plan-only", "merge"], + PLAN_ONLY_DISPATCHES_NOTHING, + ), + ( + vec!["architect", "no-merge", "--plan-only"], + PLAN_ONLY_DISPATCHES_NOTHING, + ), + ( + vec!["architect", "--plan-only", "parallel", "2"], + PLAN_ONLY_DISPATCHES_NOTHING, + ), + ( + vec!["architect", "base-fix", "--no-base-fix"], + "base-fix and no-base-fix can't be used together", + ), + ( + vec!["architect", "--plan-only", "base-fix"], + PLAN_ONLY_DISPATCHES_NOTHING, + ), + ( + vec!["architect", "no-base-fix", "--plan-only"], + PLAN_ONLY_DISPATCHES_NOTHING, + ), + ] { + let result = scenario.run(&args); + + assert_argument_error(&scenario, &result, error); + assert!( + scenario.claude_calls().is_empty(), + "{args:?} started a review" + ); + } +} + #[test] fn version_prints_the_package_version_on_stdout() { let scenario = Scenario::new(); diff --git a/tests/fakes/claude.rs b/tests/fakes/claude.rs index e8086fc7b..dc4f75f1f 100644 --- a/tests/fakes/claude.rs +++ b/tests/fakes/claude.rs @@ -25,7 +25,9 @@ //! The `init` line carries session id `fake-session-<n>` for the n-th call. //! Lines the script appends to the file named by $FAKE_CLAUDE_AFTER_RESULT //! are emitted after the closing `result` line, e.g. to kill a background -//! task as the session ends. +//! task as the session ends. What the script writes to the file named by +//! $FAKE_CLAUDE_FINAL_MESSAGE is the closing `result` line's `result`: the +//! session's final message. //! //! Called without --output-format, as `claude -p` with its prompt on stdin, //! it records stdin as `stdin` and prints only what the script prints, as @@ -187,16 +189,21 @@ pub fn main(argv: Vec<String>) { println!("{init}"); // Beside the record, not in $TMPDIR, which tests expect to be left empty. let after_result = format!("{}.after-result.{session}", record_path.display()); + let final_message = format!("{}.final-message.{session}", record_path.display()); let status = bash() .env("FAKE_CLAUDE_AFTER_RESULT", &after_result) + .env("FAKE_CLAUDE_FINAL_MESSAGE", &final_message) .status() .unwrap(); let code = exit_code(status); - let result = object([ + let mut result = object([ ("type", string("result")), ("subtype", string("success")), ("is_error", Bool(code != 0)), ]); + if let Ok(message) = fs::read_to_string(&final_message) { + result.set("result", string(message)); + } println!("{result}"); if let Ok(lines) = fs::read_to_string(&after_result) { print!("{lines}"); diff --git a/tests/fakes/gh.rs b/tests/fakes/gh.rs index 84fc0d737..b1d5a354d 100644 --- a/tests/fakes/gh.rs +++ b/tests/fakes/gh.rs @@ -6,6 +6,7 @@ //! {"repo": "owner/repo", //! "issues": {"<number>": "OPEN" | "CLOSED"}, //! "titles"?: {"<number>": "<title>"}, +//! "created"?: {"<number>": "<ISO 8601 time>"}, //! "bodies"?: {"<number>": "<body>"}, //! "comments"?: {"<number>": ["<body>", ...]}, //! "prs": [{"number", "url", "head", "base", "state", "isDraft", "title", "body", @@ -37,7 +38,11 @@ //! `--json` rejects a field the fake does not have, such as a PR's //! `closingIssuesReferences`, with gh's `Unknown JSON field`, as gh 2.45 does. //! -//! An issue's title is its entry in `titles`, else `Issue <number>`. +//! An issue's title is its entry in `titles`, else `Issue <number>`, and its +//! `createdAt` its entry in `created`, else the first second of 2020. +//! +//! `gh api --method PUT repos/<repo>/issues/<number>/labels -f +//! labels[]=<label> ...` sets an issue's labels to exactly those given. //! //! `gh api user` answers with the signed-in user's profile, whose public //! `email` is `user_email`, else null. `gh fake fails 'api user'` makes it @@ -82,10 +87,11 @@ //! //! `gh issue create --title <title> --body <body> --label <label>,<label>` //! opens an issue numbered one past the highest issue, with that title, body -//! (in `bodies`) and labels, and prints its URL. A label the repository does -//! not have, one not in `repo_labels`, fails it, as gh does. `gh label list -//! --json name` lists the repository's labels, and `gh label create <name>` -//! adds one, failing if the repository has it already. +//! (in `bodies`) and labels, and the time as its `createdAt`, and prints its +//! URL. A label the repository does not have, one not in `repo_labels`, +//! fails it, as gh does. `gh label list --json name` lists the repository's +//! labels, and `gh label create <name>` adds one, failing if the repository +//! has it already. //! //! A PR's mergeable state reports as UNKNOWN for its first `unknown_polls` //! reads. A check run reports as in progress for its first `pending_polls` @@ -126,6 +132,9 @@ //! gh fake user-email '<JSON>' set the public profile email, an //! address or null //! gh fake labels <number> '<JSON list>' set issue <number>'s labels +//! gh fake created <number> <time> set issue <number>'s `createdAt` +//! gh fake sub-issues <number> '<JSON list>' set issue <number>'s sub-issues, +//! by number, making it a Spec //! gh fake repo-labels '<JSON list>' set the repository's labels //! ``` //! @@ -591,14 +600,69 @@ fn issue_fields(state: &Json, n: &str, issue_state: &Json) -> Json { None => string(format!("Issue {n}")), }; let url = format!("https://github.com/{}/issues/{n}", state.at("repo").str()); + let labels = issue_labels(state, n) + .iter() + .map(|label| object([("name", label.clone())])) + .collect(); + let created = match state.get("created").and_then(|created| created.get(n)) { + Some(created) => created.clone(), + None => string("2020-01-01T00:00:00Z"), + }; object([ ("number", number(n.parse::<i64>().unwrap())), ("state", issue_state.clone()), ("title", title), ("url", string(url)), + ("labels", Array(labels)), + ("createdAt", created), ]) } +/// The labels of issue `n`. +fn issue_labels<'a>(state: &'a Json, n: &str) -> &'a [Json] { + state + .get("labels") + .and_then(|labels| labels.get(n)) + .map(Json::items) + .unwrap_or_default() +} + +/// The time, as GitHub writes one: `2026-10-01T12:00:00Z`. +fn now() -> String { + let date = Command::new("date") + .args(["-u", "+%Y-%m-%dT%H:%M:%SZ"]) + .output() + .unwrap(); + String::from_utf8(date.stdout).unwrap().trim().to_owned() +} + +/// `gh api --method PUT repos/<repo>/issues/<number>/labels -f +/// labels[]=<label> ...`: set the issue's labels to exactly those given. +fn issue_labels_put(state: &mut Json, args: &[&str]) { + let (positional, _) = parse(args); + let issue = positional.first().and_then(|path| { + let (repo, rest) = repo_prefix(path)?; + let n = rest.strip_prefix("issues/")?.strip_suffix("/labels")?; + Some((repo, n)) + }); + let fields = args.windows(2).filter(|pair| pair[0] == "-f"); + let labels: Option<Vec<Json>> = fields + .map(|pair| pair[1].strip_prefix("labels[]=").map(string)) + .collect(); + let (Some((repo, n)), Some(labels)) = (issue, labels) else { + die( + &format!("fake gh: unsupported api PUT {}", python_list(args)), + 2, + ) + }; + check_repo_is(state, Some(repo)); + if !state.at("issues").has(n) { + die("gh: Not Found (HTTP 404)", 1); + } + state.entry("labels", object([])).set(n, Array(labels)); + save(state); +} + /// After the issue's `on-issue-view` script, if it has one still to run. fn issue_view(state: &mut Json, positional: &[String], flags: &Flags) { let n = &positional[0]; @@ -701,6 +765,7 @@ fn issue_create(state: &mut Json, flags: &Flags) { state.entry("bodies", object([])).set(&n, string(body)); let labels = labels.into_iter().map(string).collect(); state.entry("labels", object([])).set(&n, Array(labels)); + state.entry("created", object([])).set(&n, string(now())); save(state); println!("https://github.com/{}/issues/{n}", state.at("repo").str()); } @@ -1365,6 +1430,10 @@ fn fake_command(state: &mut Json, args: &[&str]) { ["after-merge", script] => state.set("after_merge", string(*script)), ["issue", n, issue_state] => state.at_mut("issues").set(n, string(*issue_state)), ["labels", n, labels] => state.entry("labels", object([])).set(n, parse_json(labels)), + ["created", n, time] => state.entry("created", object([])).set(n, string(*time)), + ["sub-issues", n, tickets] => state + .entry("sub_issues", object([])) + .set(n, parse_json(tickets)), ["repo-labels", labels] => state.set("repo_labels", parse_json(labels)), ["user-email", email] => state.set("user_email", parse_json(email)), ["fails", call] => state @@ -1460,6 +1529,7 @@ pub fn main(args: Vec<String>) { } ["api", "graphql", rest @ ..] => graphql(&state, rest), ["api", "--method", "PATCH", rest @ ..] => pr_patch(&mut state, rest), + ["api", "--method", "PUT", rest @ ..] => issue_labels_put(&mut state, rest), ["api", path, ..] if generate_notes_repo(path).is_some() => { generate_notes(&state, &args[1..]); } diff --git a/tests/fresh_run.rs b/tests/fresh_run.rs index fd2feca03..52fcc9ab5 100644 --- a/tests/fresh_run.rs +++ b/tests/fresh_run.rs @@ -116,6 +116,10 @@ fn loads_every_factory_skill_as_the_thirdshift_plugin() { "pr", "tdd", "resolving-merge-conflicts", + "improve-codebase-architecture", + "to-spec", + "to-tickets", + "codebase-design", ] { let skill_md = files[format!("skills/{skill}/SKILL.md")].as_str(); assert!( @@ -123,10 +127,17 @@ fn loads_every_factory_skill_as_the_thirdshift_plugin() { "skill {skill} missing from the plugin" ); } - assert!( - files["skills/tdd/tests.md"].is_string(), - "a skill's supporting files are missing" - ); + for supporting in [ + "skills/tdd/tests.md", + "skills/improve-codebase-architecture/REPORT.md", + "skills/codebase-design/DEEPENING.md", + "skills/codebase-design/DESIGN-IT-TWICE.md", + ] { + assert!( + files[supporting].is_string(), + "the supporting file {supporting} is missing from the plugin" + ); + } } #[test] diff --git a/tests/packaging.rs b/tests/packaging.rs index 969685639..5e0352172 100644 --- a/tests/packaging.rs +++ b/tests/packaging.rs @@ -40,6 +40,13 @@ fn package_carries_the_factory_skills_with_their_license_and_credits() { "skills/LICENSE", "skills/pr/CREDITS.md", "skills/implement/SKILL.md", + "skills/improve-codebase-architecture/SKILL.md", + "skills/improve-codebase-architecture/REPORT.md", + "skills/to-spec/SKILL.md", + "skills/to-tickets/SKILL.md", + "skills/codebase-design/SKILL.md", + "skills/codebase-design/DEEPENING.md", + "skills/codebase-design/DESIGN-IT-TWICE.md", "src/main.rs", "README.md", "LICENSE", diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 6ef30ad39..c546d6838 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -24,6 +24,7 @@ //! claude-calls.json what the fake agent was asked to do //! claude-calls.json.after-result.<n> stream lines the n-th session emits //! after its closing result +//! claude-calls.json.final-message.<n> the n-th session's final message //! gh-calls.json every gh command run, by thirdshift or the fake agent //! ``` @@ -502,6 +503,23 @@ impl Scenario { self.write_gh_state(&gh); } + /// Set when issue `number` was created on the fake GitHub, as GitHub + /// writes a time: `2026-10-01T12:00:00Z`. + pub fn issue_created(&self, number: u32, time: &str) { + let mut gh = self.gh_state(); + gh["created"][number.to_string()] = json!(time); + self.write_gh_state(&gh); + } + + /// The labels of issue `number` on the fake GitHub. + pub fn issue_labels(&self, number: u32) -> Vec<String> { + let labels = &self.gh_state()["labels"][number.to_string()]; + let labels = labels.as_array().into_iter().flatten(); + labels + .map(|label| label.as_str().unwrap().to_string()) + .collect() + } + /// Give issue `number` the title `title` on the fake GitHub. pub fn issue_titled(&self, number: u32, title: &str) { let mut gh = self.gh_state();