diff --git a/AGENTS.md b/AGENTS.md index 96ae6df1..bf61e5a7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -12,14 +12,31 @@ Instructions for AI coding agents that work on this repository. Human contributo One issue → one branch → one PR → CI `tests-ok` → auto-merge (squash). -Grok Bot is the single entry point for humans; it schedules OpenCode (implements) and the owner's own reviewer Bot (reviews, acting as `Birfy`; owner decision 2026-10-19 15:31). Codex is not used for review: do not request `@codex review` and do not wait for a Codex review. +A person talks to the **Chief** (the Main Bot). The Chief hands the work to the stationary roles — **Triage**, **Dev**, **Reviewer** — each a Bot in the same tree, and each step below names the role that does it. OpenCode and Codex are gone (owner decision 2026-10-22): nothing requests `@codex review`, nothing waits for a Codex review, and no model route is prescribed here. + +**An `@`-mention must produce one reply.** The member takes the work, points to who has it in a line, or says it cannot; silence is a defect, not "nothing to add" — the round record already keeps `passed` and `missed` apart, and #913 tracks the summary that merges them. 1. **Issue.** Work one issue per session and per git worktree. Skim LESSONS and code-standards first. Security debt first: [#9](https://github.com/ThinkFlowLab/FlyBot/issues/9) → [#10](https://github.com/ThinkFlowLab/FlyBot/issues/10) → [#8](https://github.com/ThinkFlowLab/FlyBot/issues/8). -2. **Branch** from latest `origin/main`. Write a test that fails without the change, then fix the class of bug. +2. **Branch from the live base, and prove it.** `git fetch origin && git switch -c origin/main` — never from a local `main` you did not just fetch. Prove the base before you write anything: + + ```sh + git diff origin/main...HEAD --stat + ``` + + That output must list only files you changed. **Any file you did not touch means the branch sits on an older `main`: delete the branch and start again from the fetched tip.** Never stack the change on a stale base and never resolve the ghost files by hand. A stale base merges green and silently reverts what landed meanwhile: on #887 the branch restored an old `main` and removed the 84 test lines #869 had just merged, while the pull request page showed every check green (#864, #876, #887). + `Dev` then writes a test that fails without the change, and fixes the class of bug. 3. **Verify locally:** `npm run verify` at the repo root (Node 22+, see Commands). 4. **PR** to `main`. Imperative English title, `Fixes #N` in the body, and what you ran and did **not** run at the tip SHA. A user-visible change adds one fragment `changelog.d/..md` ([changelog.d/README.md](changelog.d/README.md)); never edit `CHANGELOG.md` in a feature PR. Squash is the only merge method; the PR title and body become the commit message, so keep the body short. 5. **CI.** The required check is `tests-ok`. It is green only when `test`, `plugins (tree)`, `plugins (social)` and `package` all pass. Never merge around a red check. -6. **Review and merge.** Merge by tier ([`risk-gate.mjs`](.github/scripts/risk-gate.mjs): `node .github/scripts/risk-gate.mjs --pr `). **T0 and T1** (owner decision 2026-10-19 11:45): an agent may enqueue the PR once `tests-ok` is green at its tip, it is mergeable, and it conforms to the design rules. No PASS is needed. **T2 and T3** need a PASS at the tip SHA from [`review-pr`](.claude/skills/review-pr/SKILL.md) (PASS / FAIL / INC) with green checks, and then **the agent that gated the PR enqueues it itself**: since 2026-10-21 the owner's authorisation is the [self-gate](#owner-decision-the-bot-gates-and-merges-its-own-prs-2026-10-21-owner-authorized), not a click in the UI. **An implementer does not review its own pull request** ([Merge gates](#merge-gates)): the PASS comes from the reviewer Bot, and because every Bot shares the `Birfy` account, a PASS by the owner's reviewer Bot on a PR another Bot opened still counts (owner decision 2026-10-19 11:43). A tip move (any new commit) voids the PASS. Neither the PASS nor green CI replaces the other. Enqueue with `gh pr merge --auto --squash --match-head-commit `, and run `node .github/scripts/enqueue-guard.mjs --pr ` immediately before it: the guard re-reads the live head and refuses (exit 1) on a `hold`, on a newest conclusion that is not a PASS, and on a report marked "record only — do not merge", so a PASS is never a queue permit on its own (#602). `main` requires a merge queue: that command enqueues the PR (gh may answer `The merge strategy for main is set by the merge queue` or `already queued`), the queue performs the squash merge, and a plain `--squash` merge is refused; do not retry ([merge plumbing](docs/autonomous-dev.md#merge-plumbing-merge_group-merge-queue-auto-merge)). Never admin-merge and never bypass a gate. Exception: a mechanical release PR under the [standing approval](#standing-approval-mechanical-release-prs) below. [`fix-issue`](.claude/skills/fix-issue/SKILL.md) walks all of this, with the environment gotchas. +6. **Gate, then queue.** Five checks, in this order. Each one can refuse. + + 1. **Tier.** `node .github/scripts/risk-gate.mjs --pr `, or quote the `risk-tier:` line of the job summary. **T0/T1** need green `tests-ok` at the tip, a mergeable pull request and design-conformance — no PASS. **T2/T3** need a **PASS at the tip SHA** written by the Reviewer ([`review-pr`](.claude/skills/review-pr/SKILL.md): PASS / FAIL / INC), never by the implementer ([Merge gates](#merge-gates)). Neither a PASS nor green CI replaces the other. + 2. **One conclusion per tip.** A tip carries at most one `Verdict:` block. A second conclusion at the same tip is an incident, not a better answer; a new commit voids the PASS; a correction reads `Correction from : …` and is not a verdict. + 3. **Enqueue.** `node .github/scripts/enqueue-guard.mjs --pr `, then `gh pr merge --auto --squash --match-head-commit `. The guard re-reads the live head and refuses (exit 1) on a `hold`, on a newest conclusion that is not a PASS, and on a report marked "record only — do not merge", so a PASS is never a queue permit on its own (#602). `main` requires a merge queue: the command enqueues the pull request (gh may answer `The merge strategy for main is set by the merge queue` or `already queued`), the queue performs the squash merge, and a plain `--squash` merge is refused — do not retry ([merge plumbing](docs/autonomous-dev.md#merge-plumbing-merge_group-merge-queue-auto-merge)). + 4. **Judge on the merge tree.** Whether the pull request can merge is decided on the queue entry — the pull request merged onto the current `main`, built and tested again — never on the pull request page. The page's green is the head alone: #759 was 9/9 green, was kicked out of the queue twice, and went red on `main` at `core/test/follow-through.test.js:264`. + 5. **Never bypass.** No admin-merge, no bypass, no ruleset change. A `Docs-not-needed:` line goes in the PR body as well as in the branch commit, and you check both at the tip ([why, and how](#docs-not-needed-why-both-places)). Run `verify` with `TZ=UTC`. + + Exception: a mechanical release PR under the [standing approval](#standing-approval-mechanical-release-prs) below. [`fix-issue`](.claude/skills/fix-issue/SKILL.md) walks all of this, with the environment gotchas. 7. **Stop** and hand over to the maintainer on a security-sensitive change, an unresolved maintainer decision, or when you cannot get a green tip. When a class of bug repeats, date a lesson in LESSONS.md (`Seen N×`); after **≥3 days** promote the durable rule into `docs/code-standards.md` / CONTRIBUTING. @@ -71,15 +88,21 @@ If `topics.json` conflicts, keep both sides' entries (one patch per edited upstr ## Agent operating model -Decided by the owner (Danyang, GitHub `Birfy`) on 2026-10-18/19. Four scheduled routines run on the Grok Bot box, each with one job. A routine does only its own job. Every write a routine makes carries the `Written-by:` line ([rule](#every-state-changing-write-says-who-wrote-it-627)). +Decided by the owner (Danyang, GitHub `Birfy`) on 2026-10-18/19; what runs today is two routines on a timer and the rest in rounds. A routine has one job and does only that job. Every write a routine makes carries the `Written-by:` line ([rule](#every-state-changing-write-says-who-wrote-it-627)). ### Routines -| Routine | Schedule | Job | +| Routine | Runs | Job | | --- | --- | --- | -| Product self-test | every 12 h | Uses FlyBot and Bot Social as a user and **only files issues**: no PRs, no pushed branches, no code changes. Each issue states the expected behavior and the change scope. Search for duplicates first. | -| Repo maintenance | every 2 h, FlyBot and Bot Social separately | The **only** routine that opens fix PRs. Each run: review open PRs against the design rules; triage every open issue (fixed, duplicate or obsolete: close with a comment; still valid: readiness check below); label every open issue and PR; develop only `agent-ready` issues; merge ready PRs through the merge queue within the gates below. Never deploys. Never edits `CHANGELOG.md` or a version number. | -| Daily release | about 02:00 Asia/Shanghai | Publishes a real semver GitHub release (tag = `core/package.json` version, patch bump per day, skipped when there are no changelog fragments) through a mechanical release PR. It owns `CHANGELOG.md` and the version. The files and the approval are in [Standing approval](#standing-approval-mechanical-release-prs). The fork's release is followed by the upstream fast-forward and upstream's own release, in one window per version: [docs/release-runbook.md](docs/release-runbook.md). | +| Product self-test | timer, 09:00 and 21:00 Asia/Shanghai | Uses FlyBot and Bot Social as a user and **only files issues**: no PRs, no pushed branches, no code changes. Each issue states the expected behavior and the change scope. Search for duplicates first. | +| Repo maintenance | a round in the delivery group, no timer | The **only** routine that opens fix PRs. Each run: review open PRs against the design rules; triage every open issue (fixed, duplicate or obsolete: close with a comment; still valid: readiness check below); label every open issue and PR; develop only `agent-ready` issues; merge ready PRs through the merge queue within the gates below. Never deploys. Never edits `CHANGELOG.md` or a version number. | +| Daily release | timer, about 02:00 Asia/Shanghai | Publishes a real semver GitHub release (tag = `core/package.json` version, patch bump per day, skipped when there are no changelog fragments) through a mechanical release PR. It owns `CHANGELOG.md` and the version. The files and the approval are in [Standing approval](#standing-approval-mechanical-release-prs). The fork's release is followed by the upstream fast-forward and upstream's own release, in one window per version: [docs/release-runbook.md](docs/release-runbook.md). | + +**Only the two rows marked *timer* self-trigger.** Maintenance, triage and review run as rounds in the delivery group — Triage lists what is open, Dev claims one item and pushes a tip, Reviewer judges that tip, the Bot that gated it enqueues — and hold no cron entry. So a quiet period means no round ran, not that a routine is broken. A timer belongs to a Session, not to a Bot (#628): read `bot_schedules` before saying anything about which timers a Bot has. + +### Read the board once per round (#796) + +One read of the open state per round — issues, pull requests, checks — is the budget, not a step to repeat. Whoever reads it writes the facts down where the next Bot will look: the pull request body or a comment, as text. Nobody else re-pulls the same list, restates it in a group chat, or pastes more than five lines of a log; they read the comment. A second opinion needs a second question, not a second fetch. #796 measures what a round actually costs. #### A schedule is per session, not per Bot (#628) @@ -134,7 +157,7 @@ What this buys is presence and a name from a closed list, not truth: with one sh - **T2 / T3:** a PASS at the head commit from a reviewer in [`.github/agent/review-policy.json`](.github/agent/review-policy.json), with green CI, and the agent that gated it then enqueues it itself ([self-gate](#owner-decision-the-bot-gates-and-merges-its-own-prs-2026-10-21-owner-authorized)). **T2 and T3 need no maintainer approve** (user decision 2026-10-22): no human `APPROVED` review is read or required, and `merge-gate` re-judges the head on the queue entry. **Self-PASS across Bots counts:** a PR opened by the owner's Bot/agent (`Birfy`) may be PASSed by the owner's own reviewer Bot/agent (the policy's `selfReview` list) — the shared account is why that list exists, not a licence for an implementer to review its own pull request. Neither the PASS nor green CI replaces the other. - **A PASS must list what it ran.** The `Ran:` line (or a `Ran …:` heading over `$ ` commands) carrying at least one real command is part of the gate, not a nicety: `merge-gate` refuses a PASS that lists none (#681, #574). When a PASS at a tip cannot be read for that reason, the reviewer may post **one replacement PASS at the same tip**: its body keeps the original conclusion, adds the `Ran:` lines of what was actually run, and carries `Replaces: the PASS at merge-gate rejected for having no Ran: line`. That is a replacement, not a second conclusion — apart from it, one tip still carries one. - Any new commit voids a PASS. CI green, the merge queue and the no-bypass rule apply to every tier. -- Agents never bypass a gate and never admin-merge (the routines run as the maintainer account `Birfy`, but use it only for the actions above, never for admin merge, bypass or changing protection rules). The merge queue squashes the PR title and body, so a `Docs-not-needed:` line goes in the PR **body** and also in a branch commit message (`npm run arch-docs` reads only the branch's commits). Run `verify` with `TZ=UTC`. +- Agents never bypass a gate and never admin-merge (the routines run as the maintainer account `Birfy`, but use it only for the actions above, never for admin merge, bypass or changing protection rules). Run `verify` with `TZ=UTC`. ### Merge gates @@ -157,6 +180,29 @@ Four rules decide what a queue entry must pass. The mechanical judge runs where A review's `state` is not read at all: approvals are not a gate any more (user decision 2026-10-22), and the account cannot approve its own pull requests, so its reviews stay `COMMENTED` even for an INC — the INC on #610 was a comment rather than a review. +**One conclusion per tip.** A tip carries at most one conclusion: one `Verdict:` block naming one SHA. Two conclusions at the same tip is an incident, not a tie to be broken — nobody re-verdicts to win, the Reviewer that wrote the later one retracts it in a comment, and the pull request waits. A correction reads `Correction from : …` and carries no `Verdict:` line, so it never competes with the standing conclusion; the replacement PASS for a missing `Ran:` ([above](#gates)) is the only other body allowed at one tip. + +**Judged on the merge tree, never on the pull request page.** The page answers "does this commit pass alone"; the queue answers "does this commit pass merged onto the current `main`", and the second one is the merge decision. #759 was green on every check, was kicked out of the queue twice, and went red on `main` at `core/test/follow-through.test.js:264`. A red queue entry that names files you did not touch is the stale base of [step 2](#pipeline-contract), not bad luck: fix the base, do not re-enqueue blindly. + +**A merged pull request's body is not a control.** GitHub evaluates `Fixes #N` once, when the merge lands. Editing the body of a merged pull request afterwards closes nothing and triggers no guard; it only makes the record disagree with what merged. If the issue is still open, close it with a comment naming the pull request — and if the merge itself was wrong, say so on the issue. + +#### Docs-not-needed: why both places + +This is not a new rule, and it does not fail because it is unclear — it fails when nobody reads it at the tip. What the two places are for: + +- [`check-arch-docs.mjs`](core/scripts/check-arch-docs.mjs) reads `git log ..HEAD --pretty=%B` — the branch commit messages, plus the pull request title and body on the queue entry. +- The merge queue squashes the pull request title and body into the one commit that lands on `main`, so that commit carries the body and none of the branch messages. `arch-docs` runs again on the push to `main`. +- A trailer written only into a branch commit is therefore gone exactly when `main` is judged. + +**Self-check before you push, and again after any new commit:** the same line appears in both places. + +```sh +git log -1 --format=%B # the branch commit message +gh pr view --json body --jq .body # the pull request body +``` + +If either lacks it, add it there and push. No trailer and no subsystem-doc update fails `arch-docs` on the branch; a trailer in the wrong place fails only after the squash, when the branch is gone. + **The T0/T1 exemption is computed, never labelled.** `merge-gate.mjs` runs the base copy of [`risk-gate.mjs`](.github/scripts/risk-gate.mjs) over the changed files and `.github/agent/risk-rules.json` to get the tier; a `risk:T0`/`risk:T1` label does nothing. Every Bot shares one account and a label carries no signature (#627), so a label-based exemption would let whoever can label turn a T3 into "no review needed" — the gate itself. A mechanical release PR is recognised by its files as before, and `hold` still refuses whatever the tier. The standing "file no new issues" rule covers design proposals only: a reproducible code defect is still filed as an issue, with its reproduction. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7e5065ab..eb3292cf 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -12,13 +12,15 @@ Security reports: use the repository **Security** tab, not a public PR descripti ## Maintainer / agent loop -For agent-driven work on this repository (Grok Bot as the human’s unified entry): +A person talks to the **Chief** (the Main Bot); the Chief hands work to **Triage**, **Dev** and **Reviewer**, each a Bot in the same tree. OpenCode and Codex are gone (owner decision 2026-10-22). + +The rules that decide whether a change may merge — tiers and the PASS, the enqueue guard, the merge queue, one conclusion per tip — live in [AGENTS.md](AGENTS.md#pipeline-contract) and [AGENTS.md → Merge gates](AGENTS.md#merge-gates). This section says only who does what, so the two files cannot drift apart: 1. Read [LESSONS.md](LESSONS.md) and [docs/code-standards.md](docs/code-standards.md) before coding. -2. **OpenCode** implements (default model `opencode/step-5-preview-free`); the **owner's reviewer Bot** (acting as `Birfy`, self-review) reviews the tip with [`review-pr`](.claude/skills/review-pr/SKILL.md); Codex is not used for review (owner decision 2026-10-19 15:31), so nothing requests or waits for it. T2/T3 need a PASS at the tip (self-PASS counts), and with green CI the gating agent enqueues it itself; T0/T1 merge on green CI without a PASS (see AGENTS.md step 6). -3. Security follow-ups from PR #2 first when open: issues #9 → #10 → #8, then other review bugs. -4. One worktree / session per issue. Promote LESSONS into code-standards after ≥3 days when the rule stays true. -5. Reviewers use [`.claude/skills/review-pr/SKILL.md`](.claude/skills/review-pr/SKILL.md). +2. **Dev** implements on a branch proven against the live `origin/main` ([AGENTS.md step 2](AGENTS.md#pipeline-contract)): a test that fails without the change comes first. +3. **Reviewer** writes the conclusion at the tip with [`review-pr`](.claude/skills/review-pr/SKILL.md); the implementer never writes it. T2/T3 need that PASS, T0/T1 need none, and neither replaces green CI. +4. **The Bot that gated the pull request enqueues it** ([AGENTS.md → Merge gates](AGENTS.md#merge-gates)); no human approve is read. +5. One worktree / session per issue. Security follow-ups from PR #2 first when open: issues #9 → #10 → #8. Promote a lesson into code-standards after ≥3 days when it stays true. ### Verifier checklist (every agent or maintainer status report) diff --git a/LESSONS.md b/LESSONS.md index cb5e7ed3..fb3bcb3b 100644 --- a/LESSONS.md +++ b/LESSONS.md @@ -13,6 +13,34 @@ Dated, short lessons from real FlyBot work. Agents read this **before coding**. ## Open lessons +### 2026-10-24 — A stale base reverts merged work while the PR page stays green + +- **Seen:** 4× (#864, #876, #887; #907 re-uploaded an old copy of its branch twice, at `d6e143a8` and `b8b582bd`) +- **Trigger:** branching from a local `main`, or any `main` not fetched in this run, and opening a pull request from it. +- **Wrong:** reading the green pull request page as "the branch is fine"; the ghost files in the diff are the tell, and the branch quietly restores an old `main`. #887 removed the 84 test lines #869 had just merged, with every check green. #907 dropped #909's whole fix from both old tips (no `verifiedAuthor`, no `not-inviter` refusal) while its page stayed green. +- **Right:** `git diff origin/main...HEAD --stat` must list only your files. Anything else means a stale base: delete the branch and start again from the fetched tip. See [AGENTS.md step 2](AGENTS.md#pipeline-contract). + +### 2026-10-24 — The merge decision is made on the merge tree + +- **Seen:** 1× (#759) +- **Trigger:** deciding a pull request can merge from its own page. +- **Wrong:** 9/9 green on the head read as mergeable; the entry was kicked out of the queue twice and went red on `main` at `core/test/follow-through.test.js:264`. +- **Right:** read the queue entry — the pull request merged onto the current `main`. A red entry naming files you did not touch is the stale base above, not bad luck. + +### 2026-10-24 — One conclusion per tip + +- **Seen:** recurring; the team's highest incident rate, so no single issue to point at. +- **Trigger:** a Bot judging a tip that already carries a `Verdict:`. +- **Wrong:** posting a second `Verdict:` at the same SHA to "settle" the first, or editing the first when the answer changed. +- **Right:** one `Verdict:` per tip. A new commit voids the old one; a correction reads `Correction from : …` and carries no `Verdict:`. + +### 2026-10-24 — A `Docs-not-needed:` trailer only in a branch commit is gone after the squash + +- **Seen:** 1× (the commit on `main` carries the pull request title and body, nothing else) +- **Trigger:** excusing an owned-path change with a trailer in a branch commit only. +- **Wrong:** green on the pull request and on the queue entry, then `arch-docs` fails on the push to `main`, where the branch commits no longer exist. +- **Right:** write the trailer in the pull request **body** as well (`check-arch-docs.mjs` reads `git log ..HEAD`). + ### 2026-10-14 — Untrusted fence escape and remote display names - **Seen:** 2× (from PR #2 review → issue #9; #98 non-face room ingest) diff --git a/docs/autonomous-dev.md b/docs/autonomous-dev.md index b1c5d707..bbdd4dcf 100644 --- a/docs/autonomous-dev.md +++ b/docs/autonomous-dev.md @@ -11,9 +11,9 @@ Decisions by the maintainer, and what follows from them: 1. **No GitHub Apps.** The deterministic controller runs in GitHub Actions on the built-in `GITHUB_TOKEN` with the least permissions per job. It has no LLM and no secrets. 2. **No Cursor GitHub App on ThinkFlowLab**, so no Cursor cloud agent on this repo. LLM agents - (triage, implement, review) run **outside Actions**, as Grok Bot routines or agents on the Grok Bot box. - The review is done by the owner's own reviewer Bot (acting as `Birfy`, self-review); Codex is not used for review - (owner decision 2026-10-19 15:31), and no workflow, routine or gate requests or waits for it. + (triage, implement, review) run **outside Actions**, as the Chief's workstations — Triage, Dev and Reviewer — on + the team's own box. The review is done by the Reviewer Bot (acting as `Birfy`, self-review); OpenCode and Codex are + gone (owner decision 2026-10-22), and no workflow, routine or gate requests or waits for them. 3. **Agents act only on issues labeled `agent-ready`**. That label is set by a maintainer, or by the owner's maintenance routine after triage (the routine runs as the maintainer account `Birfy`, so the controller sees a maintainer's label). No LLM develops other issues, and nothing outside text sets the label. @@ -32,7 +32,7 @@ issue (agent-task form) --maintainer or maintenance routine adds agent-ready--> | Role | Runs where | Credential | Can do | | --- | --- | --- | --- | | Controller: tier, labels, enable auto-merge, watchdog | Actions, base-branch code, no PR code run | `GITHUB_TOKEN`, per-job least permissions | label, comment, `gh pr merge --auto --squash` | -| Agents: implement, review | Grok Bot routines on the box (the owner's maintenance routine runs as `Birfy`) | the maintainer account's own `gh` login; a narrower credential remains the goal | triage and set `agent-ready`, push a branch or fork, open a PR, comment, review, enqueue a PR that may merge | +| Agents: implement, review | the Chief's workstations on the box — Triage, Dev, Reviewer (the maintenance routine runs as `Birfy`) | the maintainer account's own `gh` login; a narrower credential remains the goal | triage and set `agent-ready`, push a branch or fork, open a PR, comment, review, enqueue a PR that may merge | | Merge | the required GitHub merge queue on `main` (ruleset `main merge queue`) | none: the queue merges when the required checks pass on its entry | squash only (set by the queue) | | Maintainers (`Birfy`, `hsliuustc0106`) | GitHub | own accounts | `hold` (human-only), set `agent-ready`, pause; no T2/T3 approve | diff --git a/docs/code-standards.md b/docs/code-standards.md index 5503999c..489c0646 100644 --- a/docs/code-standards.md +++ b/docs/code-standards.md @@ -18,12 +18,22 @@ When you report status on a change: - A new regression test (or assertion) must **fail on the unbroken tip** before the fix lands, then pass with the fix. - Prefer a minimal fixture that shows the class of bug (fence reconstitution, forged header), not a one-off sample that only matches one string. +- **Red evidence is one of two kinds, and the report says which.** A test that is red on `base` (the unbroken tip) and green with the fix is a **defect test**: it reproduces the reported bug. A test that is green on `base` as well is a **guardrail**: it pins behaviour a later change must not break. Both belong in a pull request, but a claimed fix needs a defect test — a guardrail is never counted as the fail-without-change evidence, and the two read identically in a diff, so state which one the red run showed (`base` ref, command, before/after counts). ## Fix the class, not the sample - If review or a test finds one escape path, fix the shared helper and cover sibling markers (`untrusted-message`, `untrusted-file`, trusted headers). - Same-class reds tighten LESSONS / this file; do not paper over with a special case. +## List sync: leave softly, replay idempotently + +A change that keeps a membership, roster or presence list in sync has the same four traps every time; a pull request that reworks one states how it handles each. + +- **Leaving marks, it does not remove.** A departure keeps its record and sets the leaving sequence (`leftSeq`/`leftAt` in `core/src/host/room-membership.js`; `currentParticipants()` filters them out for the live view), so history still names who was there. Never `filter()` the entry out of the array: the next sync cannot tell it apart from someone who was never in the list, and a rejoin has nothing to land on. +- **A rejoin clears the mark instead of adding a record.** Match the returning member to its old record, drop the stray `left` line, clear `leftSeq`/`leftAt`, and refresh `joinedSeq`. Two records for one Bot is the bug, not the fix. +- **The same delivery lands once.** A repeated message or event must not add a second line or a second membership change: the room log keys remote events by id (`seenEvents`, monotone `eventCursor`) and returns before writing again. Idempotency is checked by replaying the fixture twice, not by reasoning about it. +- **A missing field is not a cleared field.** An update diffs only the lists and fields the message explicitly carries: a roster it does not include leaves the current one alone, and an absent `notice` is not an empty notice. Clearing is always an explicit value in the payload. + ## Silent catches - A `catch` that swallows the error says why in a comment inside the block (or handles it); `check-empty-catch` in `.github/scripts/` fails the build on a bare one (#205). Text inside comments, strings, templates and regex literals is not code, so a `catch {}` written there is never reported (#213). @@ -38,17 +48,17 @@ When you report status on a change: ## Merge only a reviewed tip -- By tier (owner decisions 2026-10-19; see AGENTS.md step 6): - - **T0/T1:** merge through the queue when required checks are green, the PR is mergeable and design-conformant. No PASS needed. - - **T2/T3:** a **PASS** at the **same tip SHA** (a self-PASS by the owner's reviewer Bot/agent counts; no independent reviewer required). With green required checks and a mergeable branch, the agent that gated the PR enqueues it itself: the owner's authorisation is the self-gate (AGENTS.md, owner decision 2026-10-21, "the Bot gates and merges its own PRs"), not a click in the UI. - - Required checks in CONTRIBUTING (`npm test`, client `--check`, `npm run lint`; smoke when host changes) are green or honestly listed as not run with a maintainer waiver. -- Any new commit voids a PASS: re-run the Verifier on the new tip. Never admin-merge or bypass a gate. +- By tier: **T0/T1** merge on green required checks; **T2/T3** need a **PASS at the same tip SHA** from the Reviewer ([`review-pr`](../.claude/skills/review-pr/SKILL.md)), and neither the PASS nor green CI replaces the other. The authoritative wording, the enqueue guard and the queue are in [AGENTS.md → Merge gates](../AGENTS.md#merge-gates); this page keeps only what a change must satisfy. +- **One conclusion per tip.** A second `Verdict:` at the same SHA is an incident; a new commit voids the PASS; a correction reads `Correction from : …` and is not a verdict. +- **Judge the merge tree.** Green on the pull request page is the head alone; the queue rebuilds the merge with the current `main`, and that run decides (#759). +- Required checks ([CONTRIBUTING → Test a change](../CONTRIBUTING.md#test-a-change)) are green, or honestly listed as not run with a maintainer waiver: `npm test`, the client `--check`, `npm run lint`, plus the smoke bench when `index.js` or `src/host/` changes. - **One pull request per issue.** An issue that already has a PR keeps it: re-base the branch, use `update-branch`, or push to its head, and put new tests there. Never open a second PR and close the first — including on a contributor's fork while maintainer edits are allowed. A branch that cannot be pushed is the only exception, and the issue must say why. +- **A merged pull request's body is not a control.** `Fixes #N` is evaluated once, at merge; editing the body afterwards closes nothing. Close the issue with a comment instead. ## Architecture docs and owned paths - Each subsystem under [docs/subsystems/](subsystems/README.md) declares `owned_paths` in YAML front-matter. -- Changing an owned file without updating that subsystem doc fails `npm run arch-docs` (also run at the end of `npm run lint`). Excuse with a `Docs-not-needed:` trailer in the commit message only when the change truly needs no doc update. +- Changing an owned file without updating that subsystem doc fails `npm run arch-docs` (also run at the end of `npm run lint`). Excuse it with a `Docs-not-needed:` trailer only when the change truly needs no doc update — and write it in the pull request body **and** in the branch commit, then check both at the tip: the queue squashes the body into the commit on `main`, where the check runs again ([why](../AGENTS.md#docs-not-needed-why-both-places)). - Reviewers: FAIL if an owned-path change ships without a subsystem doc update **and** without a `Docs-not-needed:` trailer. Load the latest `origin/main` rules before each review (`git fetch` + `git show origin/main:.claude/skills/review-pr/SKILL.md` or the equivalent), not a stale local copy. ## Team authority contract (planned)