From cdef24ceaec968833da7522d6a4683265fa91b4f Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Sat, 24 Oct 2026 11:00:34 +0800 Subject: [PATCH] Say which routines run on a timer, and make the practice rules checkable The routines table still said maintenance runs every 2 h. Two routines self-trigger now: the product self-test at 09:00 and 21:00 Asia/Shanghai and the daily release at about 02:00; maintenance, triage and review run as rounds in the delivery group. Both timer rows say "timer" so the table and the sentence under it agree, and the sentence states the current shape instead of dating it: on 2026-10-21 there were three timers, not two. Docs-not-needed stops being asserted three times and becomes why plus a check: check-arch-docs reads git log ..HEAD, the queue squashes the pull request body into the commit on main, so the line goes in both places and is compared at the tip. An @-mention that produces no reply is a contract defect (#913). The code standards gain the list-sync rules and the two kinds of red evidence, and the stale-base lesson counts its fourth occurrences (#907 at d6e143a8 and b8b582bd). The review-pr link in docs/ now climbs one directory, so it lands on the file instead of a 404. Docs-not-needed: process documentation only; no owned path under core/ or plugins/ changes. --- AGENTS.md | 64 ++++++++++++++++++++++++++++++++++++------ CONTRIBUTING.md | 12 ++++---- LESSONS.md | 28 ++++++++++++++++++ docs/autonomous-dev.md | 8 +++--- docs/code-standards.md | 22 +++++++++++---- 5 files changed, 110 insertions(+), 24 deletions(-) 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)