Skip to content

feat(review): add read-only mode, ask before stopping other engines - #12

Merged
stasadev merged 1 commit into
mainfrom
20261030_stasadev_review_less_personal
Oct 30, 2026
Merged

stasadev merged 1 commit into
mainfrom
20261030_stasadev_review_less_personal

Conversation

@stasadev

Copy link
Copy Markdown
Member

Short Summary (TL;DR)

/ddev:review and /ddev:ci-failure now ask before they power off DDEV on other local engines, and /ddev:review gets a read-only mode. All three skills drop one maintainer's phrases and habits, because every DDEV maintainer uses them.

The Issue

Parts of the skills assume one maintainer's setup and voice:

  • poweroff-others.sh powered off DDEV on every other local engine and told the user afterwards. That can stop a project someone is working in.
  • Every /ddev:review built binaries and ran Docker, even when the user wanted only a read of the diff.
  • /ddev:ci-failure assumed the user's machine matches the CI runner.
  • The descriptions listed personal phrases, such as "happy now?", "good commit name?" and "let's fix it once and for all". /ddev:review used "happy" as its verdict, and the writing rules had personal style, such as no em dashes.

How This PR Solves The Issue

Change Behavior
poweroff-others.sh Lists the engines that run DDEV. It powers them off only with --poweroff.
/ddev:review and /ddev:ci-failure List the engines before the lock, ask the user, then run --poweroff under the lock. The lock isn't held while the agent waits for an answer.
/ddev:review modes A new read-only mode runs steps 1 and 3 without builds or Docker, and says that nothing ran.
/ddev:review verdict "Ready to merge", "Not yet", or "Can't conclude yet".
/ddev:ci-failure step 2 Says when this machine doesn't match the failing job, and asks before it builds a replica environment.
Descriptions Keep only common requests, such as "commit", "review again" and "is it intermittent?".
Writing rules Drop the em dash rule and "read it aloud". The ASD-STE100 rules stay, because they are for readers who are not native English speakers.

Manual Testing Instructions

Run this on a machine with two Docker contexts, such as Docker and Podman:

git fetch origin pull/12/head
git checkout FETCH_HEAD
DOCKER_CONTEXT=<other context> ddev start   # in any test project
plugins/ddev/skills/review/poweroff-others.sh "$(docker context show)"
# Expect "ddev is running on <other context>", with the project still running
plugins/ddev/skills/review/poweroff-others.sh --poweroff "$(docker context show)"
# Expect "powering off ddev on <other context>"

Automated Testing Overview

Claude Code ran shellcheck plugins/*/skills/*/*.sh and claude plugin validate . on Linux, and both passed. It ran poweroff-others.sh without --poweroff on a machine with four contexts and no other engine running DDEV. It printed nothing and exited 0. Nobody ran --poweroff against a running project.

Claude Code ran each phrase twice, headless, in a throwaway repository with a github.com/ddev/ remote and in one with another remote:

Phrase DDEV repository Other repository
"let's review last commit" ddev:review 2 of 2 none
"review the last commit" ddev:review 2 of 2 none
"happy now?" none none
"just read the last commit diff quickly, any bugs?" none none
"commit this" ddev:commit 2 of 2 none
"reword the commit message" ddev:commit 2 of 2 none
"this test fails, works locally but fails in CI" ddev:ci-failure 2 of 2 none
"TestFoo fails in CI on <ddev/ddev run URL>, is it intermittent?" ddev:ci-failure 2 of 2 ddev:ci-failure 2 of 2, as its description allows for a DDEV run

Nobody checked in a session that the read-only mode skips the builds.

🤖 Developed with assistance from Claude Code

## Short Summary (TL;DR)

`/ddev:review` and `/ddev:ci-failure` now ask before they power off DDEV on other local engines, and `/ddev:review` gets a read-only mode. All three skills drop one maintainer's phrases and habits, because every DDEV maintainer uses them.

## The Issue

Parts of the skills assume one maintainer's setup and voice:

- `poweroff-others.sh` powered off DDEV on every other local engine and told the user afterwards. That can stop a project someone is working in.
- Every `/ddev:review` built binaries and ran Docker, even when the user wanted only a read of the diff.
- `/ddev:ci-failure` assumed the user's machine matches the CI runner.
- The descriptions listed personal phrases, such as "happy now?", "good commit name?" and "let's fix it once and for all". `/ddev:review` used "happy" as its verdict, and the writing rules had personal style, such as no em dashes.

## How This PR Solves The Issue

| Change | Behavior |
| --- | --- |
| `poweroff-others.sh` | Lists the engines that run DDEV. It powers them off only with `--poweroff`. |
| `/ddev:review` and `/ddev:ci-failure` | List the engines before the lock, ask the user, then run `--poweroff` under the lock. The lock isn't held while the agent waits for an answer. |
| `/ddev:review` modes | A new read-only mode runs steps 1 and 3 without builds or Docker, and says that nothing ran. |
| `/ddev:review` verdict | "Ready to merge", "Not yet", or "Can't conclude yet". |
| `/ddev:ci-failure` step 2 | Says when this machine doesn't match the failing job, and asks before it builds a replica environment. |
| Descriptions | Keep only common requests, such as "commit", "review again" and "is it intermittent?". |
| Writing rules | Drop the em dash rule and "read it aloud". The ASD-STE100 rules stay, because they are for readers who are not native English speakers. |

## Manual Testing Instructions

Run this on a machine with two Docker contexts, such as Docker and Podman:

```bash
git fetch origin pull/12/head
git checkout FETCH_HEAD
DOCKER_CONTEXT=<other context> ddev start   # in any test project
plugins/ddev/skills/review/poweroff-others.sh "$(docker context show)"
# Expect "ddev is running on <other context>", with the project still running
plugins/ddev/skills/review/poweroff-others.sh --poweroff "$(docker context show)"
# Expect "powering off ddev on <other context>"
```

## Automated Testing Overview

Claude Code ran `shellcheck plugins/*/skills/*/*.sh` and `claude plugin validate .` on Linux, and both passed. It ran `poweroff-others.sh` without `--poweroff` on a machine with four contexts and no other engine running DDEV. It printed nothing and exited 0. Nobody ran `--poweroff` against a running project.

Claude Code ran each phrase twice, headless, in a throwaway repository with a `github.com/ddev/` remote and in one with another remote:

| Phrase | DDEV repository | Other repository |
| --- | --- | --- |
| "let's review last commit" | `ddev:review` 2 of 2 | none |
| "review the last commit" | `ddev:review` 2 of 2 | none |
| "happy now?" | none | none |
| "just read the last commit diff quickly, any bugs?" | none | none |
| "commit this" | `ddev:commit` 2 of 2 | none |
| "reword the commit message" | `ddev:commit` 2 of 2 | none |
| "this test fails, works locally but fails in CI" | `ddev:ci-failure` 2 of 2 | none |
| "TestFoo fails in CI on <ddev/ddev run URL>, is it intermittent?" | `ddev:ci-failure` 2 of 2 | `ddev:ci-failure` 2 of 2, as its description allows for a DDEV run |

Nobody checked in a session that the read-only mode skips the builds.

🤖 Developed with assistance from [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@stasadev
stasadev merged commit acae112 into main Oct 30, 2026
1 check passed
@stasadev
stasadev deleted the 20261030_stasadev_review_less_personal branch October 30, 2026 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant