Skip to content

docs: require every change to ship via PR with a bot-comment ceremony - #1

Merged
doublegate merged 2 commits into
mainfrom
docs/pr-workflow-convention
Jul 20, 2026
Merged

docs: require every change to ship via PR with a bot-comment ceremony#1
doublegate merged 2 commits into
mainfrom
docs/pr-workflow-convention

Conversation

@doublegate

Copy link
Copy Markdown
Owner

Motivation

Establishes the shipping workflow as a durable convention. Until now changes were pushed straight to main, which skips independent review entirely — the Copilot and Gemini Code Assist bots only run against pull requests.

Changes

  • AGENTS.md gains a Shipping: every change goes through a PR section with the full ceremony.
  • The rule that matters most: every bot comment gets adjudicated — adopt, reject, or adopt-with-modification — replied to individually with real reasoning, and marked resolved. A rejection needs a citation, not "won't fix".
  • Notes explicitly that several bot suggestions will conflict with deliberate ADR decisions (the reverse-order pipeline cascade in ADR 0007, derive-don't-increment in ADR 0006, the intentionally reproduced VR4300 errata). Those get rejected with the citation.

Repo settings verified

Setting State Note
Squash merge enabled the merge mode this workflow uses
main protection none not required; the convention is the gate
Auto-delete-on-merge off, deliberately so the post-merge verification cannot be skipped

Checks run

Docs-only change. markdownlint at the pinned v0.49.1 passes. This PR is also the first exercise of the workflow itself.

Records the shipping workflow as a durable convention rather than a habit.

Nothing pushes to `main` directly any more. Every change branches, opens a PR,
and waits for CI *and* the review bots (Copilot, Gemini Code Assist), which only
run against pull requests and comment asynchronously -- so the first green is not
the signal to merge.

The ceremony every PR goes through:

- Adjudicate EVERY bot comment: adopt, reject, or adopt-with-modification. None
  are silently ignored.
- Apply adopted changes as follow-up commits on the branch.
- Reply to each comment individually with the decision and real reasoning. A
  rejection needs a citation -- "contradicts ADR 0006 because ..." rather than
  "won't fix" -- because the reasoning is what a human reviewer reads later, and
  because several bot suggestions will conflict with decisions the ADRs made
  deliberately (the reverse-order pipeline cascade, derive-don't-increment, the
  intentionally reproduced VR4300 errata).
- Mark each comment resolved once answered.
- Re-run to FINAL green, then squash-merge.
- Verify the change is actually on `main` before deleting the branch.

Repo settings checked: squash merge is enabled, `main` is unprotected, and
auto-delete-on-merge is deliberately left OFF so the post-merge verification step
cannot be skipped by automation.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 20, 2026 06:23

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds a new section to AGENTS.md detailing the pull request and shipping workflow, including a step-by-step ceremony for handling bot comments. The reviewer suggested updating the squash-merge step to explicitly require human approval as defined in CONTRIBUTING.md to prevent bypassing human review requirements.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread AGENTS.md Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates AGENTS.md to formalize a documented shipping workflow that requires all changes to land via pull request, ensuring automated review bots can participate and that their feedback is consistently adjudicated.

Changes:

  • Added a “Shipping: every change goes through a PR” section describing the end-to-end PR workflow.
  • Documented a required bot-comment adjudication ceremony (adopt/reject/adopt-with-modification with individual replies and resolution).
  • Clarified expectations around rejecting bot suggestions that conflict with existing ADR decisions (with citations).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread AGENTS.md Outdated
Adopts both bot review comments on PR #1.

Gemini flagged that the new AGENTS.md shipping section said nothing about human
approval while main is unprotected. Checking its premise, the concern was real
and more specific than the comment stated: CONTRIBUTING.md 'Code review' already
requires one reviewer minimum (two for docs/architecture.md or cross-subsystem
refactors), so the two documents would have contradicted each other.

Not adopted verbatim -- the literal suggestion cited 'the required human
reviewer(s)' as though a reviewer pool exists. It does not; this is a
single-maintainer private repo, and writing that would have made the requirement
ceremonial. Instead both documents are reconciled honestly: the repo owner is the
reviewer of record, agent authorization to run the ceremony and squash-merge is
standing rather than per-PR, and six explicit conditions require stopping to ask
instead of merging (unexplained CI failure, an unadjudicable bot comment, changes
to docs/architecture.md or cross-subsystem refactors, anything breaking
byte-identity/save-state/determinism, superseding or contradicting an ADR, or
anything needing a force-push or history rewrite). Also records why branch
protection is deliberately absent, and what would change that.

CONTRIBUTING.md now cross-references AGENTS.md so the two cannot drift apart
again silently -- which is how this inconsistency arose in the first place.

Copilot's article fix ('lands via a pull request') adopted as-is.

Added to the bot-suggestion guidance: check the bot's PREMISE before rejecting on
wording. A suggestion that reads generic may rest on a real inconsistency, which
is worth fixing even when the proposed text is not.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@doublegate
doublegate merged commit 4cb248e into main Jul 20, 2026
6 checks passed
@doublegate
doublegate deleted the docs/pr-workflow-convention branch July 20, 2026 06:29
doublegate added a commit that referenced this pull request Jul 20, 2026
Adopts Gemini's comment on PR #10. AGENTS.md is an operational instruction file,
and "as of 2026-07-20" does not help an agent operate -- it is temporal noise in
a document read for rules, not history. The parenthetical now states the rule
directly.

The date is deliberately KEPT in pages.yml, where the context is different: there
it explains why a paid-plan note exists at all and warns that reverting to
private would silently reintroduce a 404 on deploy. That is operationally
relevant to whoever next edits that workflow.

On the cross-reference point: CONTRIBUTING.md already points at AGENTS.md
Shipping for merge authority (added in #1, for exactly this drift reason), and
the two do not contradict -- CONTRIBUTING states the one-reviewer-minimum rule
and AGENTS explains who that reviewer is and when to stop and ask.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
doublegate added a commit that referenced this pull request Jul 22, 2026
Adjudicating the PR #56 bot review:

- **Wait for the whole command** (CodeRabbit #2, Antigravity blocking #2):
  `Rdp::tick` now returns without advancing when `DPC_END - DPC_CURRENT`
  is less than the decoded length. The rdpq microcode advances `DPC_END`
  incrementally as it fills the buffer, so it can land mid-command;
  consuming a partial multi-word primitive would decode unwritten RDRAM.
  New test drives a 22-word triangle with `DPC_END` first at word 10
  (stalls) then at word 22 (consumed whole). Mutation-checked.

- **XBUS stalls the decoder** (CodeRabbit #3): with `DPC_STATUS.XBUS` set
  the command source is DMEM, which is not yet wired, so the decoder must
  not fall back to reading RDRAM (that would decode parameter data as
  opcodes and desync). New test asserts no advance under XBUS.
  Mutation-checked.

- **Independent decoder test** (CodeRabbit #1): the FIFO-walk fixture now
  states each command's word count explicitly rather than calling
  `command_len_words`, so the walk is a genuine check of the decoder
  against the N64brew map, not a tautology built from it.

Rejected: Antigravity blocking #1 (texture rectangle is 3 words). The
N64brew command map shows 0x24/0x25 as two 64-bit words (Word 0 coords +
Word 1 s/t/dsdx/dtdy); there is no third dsdy/dtdx word — texrect is
axis-aligned and carries only dsdx and dtdy. The decoder's 2 stands.

Docs: docs/rdp.md gains the two stall conditions; CHANGELOG updated.
Gates: fmt, clippy --workspace -D warnings, cargo test --workspace,
rustdoc -D warnings, thumbv7em no_std, markdownlint — all green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.

2 participants