Compare commits

..
Author SHA1 Message Date
logan e62e12f6c6 docs(agent): let a scheduled run merge behind an independent review
CI / e2e (push) Skipped
CI / check (push) Skipped
CI / check (pull_request) Failing after 35s
CI / e2e (pull_request) Skipped
The runs produced five green PRs and no way to land them without a person. What makes that safe to automate is not CI — the author writes the tests — but a reviewer in a fresh context: last night's caught two guards that left the whole suite green when deleted, an over-narrow sweep, and an overlay covering the artwork it was editing.

So the gate is seven conditions, and a Kind/Feature is never one of them. It can exist at all because a push to main publishes nothing here: release.yml is workflow_dispatch and the four publishers key on v* tags, so a wrong merge costs a revert rather than a package nobody can unpublish. The section says so, and says it is void if that changes.
2026-08-22 01:57:56 -04:00
4 changed files with 63 additions and 52 deletions
+63 -2
View File
@@ -133,10 +133,71 @@ status and `GET /api/v1/repos/yonlu/yellowjacket/actions/jobs/<id>/logs` for
the log — and fix it. Two consecutive failed CI runs on the same cause: stop,
comment what you know on the PR, and leave it for a human.
**Do not merge.** Comment on the issue linking the PR, leave
`Status/In Progress` on, and end the run.
**Do not merge, and do not review your own work.** Comment on the issue
linking the PR, leave `Status/In Progress` on, and end the run. Whether
this merges is decided by a reviewer that did not write it — see below.
A CI run is not a review: it proves the tests you wrote pass, which is
exactly the thing an author is worst placed to judge.
## Finally
Report in three lines: which issue you took, what state it is in
(PR open / CI green / stopped and why), and any issues you filed.
---
# The merge gate
This half is **not** run by the author. It is run against a PR by
someone who has not seen the branch before, and it decides whether the
work lands on its own or waits for a human.
A push to `main` publishes nothing here — `release.yml` is
`workflow_dispatch` only and all four publishers key on `v*` tags — so
the cost of a wrong merge is a bad commit on `main` and the time to
revert it. That is the whole reason this gate can exist. If that ever
changes, this section is void.
**Merge only when every one of these is true.** Any single no means
leave it open.
1. An independent review of the diff returns **MERGE** or **MERGE WITH
NITS**. `CHANGES NEEDED`, or a review that could not reach a verdict,
means a human looks.
2. `CI / check (pull_request)` **and** `CI / e2e (pull_request)` are
`success` on the PR's current head. Ignore the `(push)` contexts —
they are `skipped` by design and Gitea folds `skipped` into a
combined state of `pending` that never clears.
3. The PR is mergeable with no conflicts, rebased onto current `main`.
4. The diff touches **none** of: `.gitea/workflows/`, `.releaserc.yml`,
`packaging/`, `build/`, `scripts/gitea-release.sh`,
`backend/database/sql/schemas/`, `backend/database/staleshape*.go`,
`go.mod`, `go.sum`. These either publish to somewhere a mistake
cannot be taken back from, or can destroy a database that a user
cannot rebuild.
5. The issue is `Kind/Bug`, `Kind/Testing`, `Kind/Documentation` or
`Kind/Enhancement`. **A `Kind/Feature` is a design decision and is
never auto-merged**, however green it is.
6. The diff is under ~600 changed lines across under ~15 files. Past
that, "a reviewer read it" stops being a claim anyone should take on
trust.
7. The PR does not claim to have verified something no tier here can
see. A change whose evidence would have to come from a physical
device is reported, not merged.
**When it merges**, use `{"Do":"merge"}` on
`POST /api/v1/repos/yonlu/yellowjacket/pulls/<n>/merge`. Then **check
the issue actually closed** — a squash or an edited merge message drops
the `Closes` footer — and close it by hand with a comment naming the
commit if it did not. `unclaim.yml` strips `Status/In Progress` on
close; if the label is still there a minute later, strip it yourself.
**When it does not merge**, say so on the PR in one paragraph: which
condition failed and what would satisfy it. Leave the PR open, leave
the label on, and file the review's substantive findings as issues so
they are searchable rather than buried in a PR comment.
**A nit is not a blocker, and it is not free either.** A `MERGE WITH
NITS` merges, and each nit worth keeping becomes an issue. Do not fix
nits on the branch: that is a second author pass with no second review,
which is the thing this gate exists to prevent.
-20
View File
@@ -262,26 +262,6 @@ real store code. A binding carries an **ID**, not a name
`yellowjacket/backend/home.Service.GetShelves`), so the fake derives
that map from the generated tree rather than writing it down.
**A test file does not get its own origin, so `setup.ts` clears
`localStorage` between tests.** `@vitest/browser-playwright` opens one
BrowserContext per session and runs several files in it one after
another, so everything a component persists — the track list's sort and
column widths, the cover size, `now-playing`'s scroll mode — is still
there when the next file mounts the same component. Which files share a
tab, and in what order, changes run to run, so the symptom is a spec
that fails about one test in three and passes every time it is run on
its own: #138 cost three scheduled runs, one of them a PR whose diff
held no frontend code at all. Measured on the build before the fix, a
single full run started **24** tests with storage already set. Two
things follow. The clear is safe precisely because the leak is
sequential — files in a session do not overlap, so it cannot wipe
storage a concurrently-running file is in the middle of using — and it
belongs in `setup.ts` rather than in the specs that write, because the
spec that *reads* is never the one that knows. And a spec whose
assertion depends on an order still **states that order** rather than
inheriting a default, or the next change to a default is the same
mystery again.
**`frontend/bindings/` is generated by `wails3`, not `go generate`**, so
the pre-commit codegen check does not cover it. `make bindings-check`
(~3.5 s warm, ~20 s on a cold build cache, also a pre-commit hook)
@@ -161,15 +161,6 @@ describe('double-clicking a row in the track list', () => {
localStorage.removeItem('track-list-column-widths');
el = await fixture<LitElement>('track-list', { externalTracks: LIST });
// Say which order is being asserted rather than inheriting one.
// `restoreSortPreferences()` runs in `connectedCallback`, so the
// list opens in whatever sort was last persisted -- and
// `track-11` sorts before `track-3` by title, which is the shape
// of #138. The row below is the fixture's third track only while
// nothing is sorting the list.
(el as unknown as { sortField: string | null }).sortField = null;
el.style.display = 'block';
el.style.height = '600px';
await flush();
@@ -181,10 +172,6 @@ describe('double-clicking a row in the track list', () => {
const rows = shadowAll(el, '.track-row');
const row = rows.find((r) => r.getAttribute('data-index') === '3');
// Stated first, so a list that is not in the order this asserts
// fails by saying so rather than as an off-by-eight file path.
expect(row?.getAttribute('data-file-path')).toBe('/music/track-3.mp3');
dblclick(row!);
await flush();
-17
View File
@@ -72,23 +72,6 @@ document.body.style.margin = '0';
beforeEach(() => {
resetHarness();
// A test file does not get its own origin. `@vitest/browser-playwright`
// opens one BrowserContext per session and runs several files in it,
// one after another, so everything a component persists — the track
// list's sort and column widths, the cover size, `now-playing`'s
// scroll mode — is still there when the next file mounts the same
// component. That is invisible until it is intermittent, because
// which files share a tab and in what order changes run to run: it
// cost #138 three scheduled runs, one of them a PR with no frontend
// code in it at all.
//
// Clearing here rather than in the specs that write is deliberate —
// the spec that *reads* is never the one that knows. It is safe for
// the same reason the leak exists: files in a session are
// sequential, so this cannot wipe storage a concurrent file is in
// the middle of using.
localStorage.clear();
});
afterEach(() => {