Pr Cycle

PR Cycle (otari)

Drive an issue from code to a reviewed, ready PR without hand-holding. One issue = one branch = one PR. For several issues, fan out (see Multi-issue orchestration).

Compose with the sibling skills rather than repeating them: review for the self-review, frontend-standards before any web/ edit (its testing.md is the dashboard's test guidance), and backend-standards before any src/gateway/ edit (its "Before you finish" section carries the backend test bar). AGENTS.md owns the repo-wide facts this skill leans on: the two runtime modes, the test notes, and the generated artifacts.

The cycle (one PR)

1. Branch off origin/main (git fetch origin main first). Name it <type>/<slug> (fix/…, perf/…, refactor/…, docs/…). Never push to main.
2. Understand the issue. Read it and the exact files and lines it names, and verify current signatures before editing rather than trusting the issue's snippet. Read AGENTS.md plus the scoped one for the directory you are in (web/, src/gateway/). A backend edit respects the layer rules scripts/check_architecture.py enforces; a management route is standalone-only, so check register_routers() placement.
3. Implement. For a refactor, keep it strictly behavior-preserving: no change to a public API, a route, a query key, or an invalidation. Correctness over speed.
4. Tests. Happy path and error path, next to the behavior they cover (unit for pure logic, integration for route or database behavior). Mock the dashboard at the apiFetch boundary, not the hooks. Never weaken coverage, and never add a global pytest rerun policy: a genuinely flaky test carries @pytest.mark.flaky(reruns=...) and a stated reason.
5. Checks must be green before opening (see Running checks), and regenerate whatever the change made stale (see Generated artifacts).
6. Commit. Conventional Commits. End every message with a Co-Authored-By: trailer naming the model that wrote it (Co-Authored-By: <your model> <noreply@anthropic.com>). Fill in your own identity; do not copy a model name out of an example or another commit.
7. Open the PR against mozilla-ai/otari with Fixes #<n>. The title must be a Conventional Commit (otari-pr-title.yml gates it; accepted types are feat, fix, perf, security, revert, chore, build, ci, docs, style, refactor, test), because the repo squash-merges and git-cliff parses that title into the changelog. Keep the template's ## PR Type, ## Checklist and ## AI Usage sections: pr-template-check.yml fails and labels the PR missing-template if any of the three is absent. Fill in AI Usage honestly, including the AI-agent checkbox. Two further sections are expected on every PR and are gated by nobody, which makes them the ones an agent drops: ## Description in plain English for a reader with no context on the area (what changes for someone using Otari, and why, no file paths), and ## How to test it locally with the steps a reviewer runs plus the automated checks that already cover it. No labels are required here. No em dashes in the description (repo prose rule). Default to opening ready for review; open a draft only if the user asked to see it first (confirm which if unsure).
8. Self-review. Invoke the review skill on your own PR before anyone else reads it. Apply what is valid and push; skip nits that fight the repo's conventions, and say why.
9. Request reviewers (see Requesting reviewers).
10. Wait for the review, then fix (see Handling the review). Leave the PR ready (or draft, per step 7). Never merge unless told to, and note that main is protected as well (see Merging), so a merge needs a human approval on top of your instruction.

Running checks

From the repo root:

• make lint: the architecture check, then Ruff. Ruff alone is not equivalent. A layer violation fails here with a clean ruff check.
• make typecheck: mypy.
• make test: tests/unit and tests/integration. make test-unit and make test-integration split it while iterating.

Integration tests need PostgreSQL: TEST_DATABASE_URL when set, otherwise a Testcontainers postgres:17. Whichever it is, it is a server URL: each xdist worker creates a database of its own on it (postgres becomes postgres_gw0, and so on) and drops it at the end of the session, so the credentials need CREATE DATABASE and a postgres database to connect through. With no Docker, point TEST_DATABASE_URL at any reachable instance; SQLite is not a fallback, because none of that is available there. Two tests make a real outbound call and report a status mismatch with no network egress (test_error_detail_leakage.py::test_provider_error_does_not_leak_details, test_streaming_error_event.py::test_streaming_creation_error_returns_http_error): that is environment noise, not a regression, so confirm the change against the rest of the suite.

A change to the app, the migrations, or dependency resolution also owes the OSS-edition smoke gate: uv run --frozen --no-dev python scripts/oss_edition_smoke.py. It defaults to a throwaway SQLite file, so it needs no Docker.

The dashboard has its own, which make lint does not touch: pnpm --dir web run lint, pnpm --dir web run typecheck, pnpm --dir web test. Screenshot baselines are gitignored and that suite runs on demand, so a PR that moves a page owes no PNGs; a PR that adds a page owes a screenshot entry so the page is covered when the suite becomes a gate.

What CI runs on a PR, and the two ways it silently does not

Every substantive workflow but one is declared pull_request: branches: [ main ] (otari-dashboard.yml, otari-dashboard-parity.yml, otari-design-system.yml, otari-tests.yml), and that filter is on the base branch. The exception is otari-sdk-codegen-check.yml, which declares pull_request with a paths filter and no branches key at all, so it matches any base: a stacked PR touching docs/public/openapi.json or scripts/sdk_codegen/** runs its four-language generate matrix and can go red where this section otherwise promises nothing. Two situations therefore leave a PR with almost no CI, and neither of them reports anything:

• A base that is not main. A PR stacked on another feature branch matches no workflow, so it gets only the pull_request_target checks. Retargeting a stacked PR onto its parent branch is what usually causes this, and the trade is invisible in the direction you are looking: the diff gets smaller and the check count drops from seven to one or two. Both ends of that range depend on when you look, and it is worth knowing why: otari-pr-title.yml lists synchronize, so Lint PR title re-runs on every push and is attached to the current head, while pr-template-check.yml lists only opened and edited, so check-template stays attached to the sha the PR was opened (or last edited) at. So the ceiling is seven on that sha and six on every head after a push, and the floor is two and then one. Measured on four stacked heads: one row each, Lint PR title, with no check-template on any of them. Keep a stacked PR on base main and live with the inherited diff until its parent merges.
• A CONFLICTING PR. A pull_request workflow builds the PR's merge ref, and a conflicting PR has none, so nothing runs until the conflict is resolved. The tell is mergeStateStatus: DIRTY beside a check count that has stopped growing.

A child PR hits the second case as soon as its parent is squash-merged, because its own unsquashed commits and the squash on main are the same content twice. git rebase --onto origin/main <the parent's old head> drops the absorbed commits and clears it.

"Green" means the full expected set is present and none of it is pending, which is not the same as nothing being pending. Both situations above produce a gh pr checks that lists one or two passing rows, and a passing row reads as a clean result to anyone who does not already know what the set should be. Check the names rather than only the buckets: on a dashboard change the substantive ones are build, dashboard, e2e, catalog and serving, and a run without them has covered nothing.

A small set is not always one of the two faults above, though. Every one of these workflows also carries a paths filter, so a change that touches nothing they watch correctly runs almost nothing: a PR editing only .github/skills/, or a docs/ file other than dashboard.md and public/openapi.json, gets the title and template checks and no more, and that is the right answer rather than a symptom. Those two docs/ paths are the exception, watched by otari-dashboard.yml, otari-dashboard-parity.yml, otari-dashboard-serving.yml and otari-docker-build.yml because the dashboard bundles the guide and generates its client from the spec, so a one-line edit to either runs dashboard, e2e, serving and build. The question to ask is whether the set matches the change, not whether the set is large.

Generated artifacts a PR can owe

• A route, a schema, or a route docstring: run uv run python scripts/generate_openapi.py, then make postman, and commit both. A single CI job runs make openapi-check and make postman-check, so regenerating only the spec still fails.
• web/src/client/schema.ts and web/src/routeTree.gen.ts are committed and drift-checked. The dashboard bundle (src/gateway/static/dashboard/) is not committed, so a web/src change sometimes leaves a file to commit and never leaves a bundle to commit.
• Never hand-edit CHANGELOG.md. The release workflows regenerate it from the squashed titles.

Requesting reviewers

One bot reviews this repo. CodeRabbit reviews automatically, without being requested, and leaves inline comments of its own; address its threads the way you would a person's (reply, then resolve). Nothing has to be requested to get it.

Copilot is gone. The org gave up its access in September 2026, so do not request it. The attempt fails silently rather than erroring: POST /pulls/<n>/requested_reviewers returns 201 Created and GraphQL requestReviews returns success, both while adding no reviewer, so a poll for its review waits out the timeout on a bot that was never coming.

• Team. gh pr edit <n> --add-reviewer mozilla-ai/otari-team. CODEOWNERS auto-requests that team only on the open-core guardrail paths (ARCHITECTURE.md, scripts/check_architecture.py, .github/CODEOWNERS), so every other PR needs the request made explicitly.

Handling the review

1. Wait for CodeRabbit to finish. It appears under /pulls/<n>/reviews with state COMMENTED once done. It is not instant, and its summary comment can land before the inline threads do, so a review that looks empty may not be finished.

2. Read the comments. The summary body is in /pulls/<n>/reviews. Inline comments can be missing from the REST /pulls/<n>/comments endpoint for bot reviews even when the summary claims N comments, so fetch the threads over GraphQL:

   gh api graphql -f query='{ repository(owner:"mozilla-ai",name:"otari"){
     pullRequest(number:<n>){ reviewThreads(first:50){ nodes{
       isResolved isOutdated id
       comments(first:10){ nodes{ databaseId author{login} path line originalLine body } } } } } } }'
   

3. Triage. Apply every valid finding; skip a nit that contradicts an established convention here and say so. Verify against current source before "re-fixing" anything: GitHub re-anchors comments to HEAD, so an addressed comment can look like it came back.

   When a finding is about a claim, grep the repo for the claim, not the file. The habit worth correcting here is fixing the place the problem was noticed rather than every place it lives. One review caught the same stale statement in three files, a role whose color was checked and whose metrics were not, and one of two identical hand-rolled values in a file already being edited. Each fix was right and each search was one file too narrow, so after applying a finding, search for the pattern rather than re-reading the diff.

4. Push the fixes. On each addressed thread, reply with one line on how it was addressed, then resolve it:

   # reply: databaseId comes from the query above
   gh api -X POST repos/mozilla-ai/otari/pulls/<n>/comments/<databaseId>/replies \
     -f body='Addressed in <sha>: <what changed>.'
   # resolve: GraphQL only, with the thread's node id (there is no REST endpoint)
   gh api graphql -f query='mutation{ resolveReviewThread(input:{threadId:"<threadId>"}){ thread{ isResolved } } }'
   

   A reply to a resolved thread does not reopen it. Keep replies terse and free of em dashes: it is a poor look to trip the prose rule in the same breath as answering a review.

5. Resolve your own self-review threads too. required_review_thread_resolution counts every thread, including the ones you opened on your own diff in step 8. Explanatory comments that need no action still block the merge until resolved, which is a non-obvious cost of doing the self-review properly. Resolve them once they have been read, the same way as a reviewer's.

6. Leave the PR ready (or draft, per the original decision).

Multi-issue orchestration

To turn a batch of issues into PRs at once, run one worktree-isolated agent per issue (Agent tool, isolation: "worktree"), each executing the cycle above.

• Group into waves by file independence. Issues touching disjoint files run in parallel; issues sharing a file are sequenced, with the later ones rebasing once the earlier lands.
• Order within a wave: small and low-risk first, large refactors later, so the rebase surface stays small.
• A stacked PR loses three protections, so stack deliberately. Basing a PR on a topic branch instead of main buys a clean diff (only your own commits, not the parent's) and costs the things that would otherwise catch a mistake. CI is the third and it has its own section above ("What CI runs on a PR"); the other two: protect-main applies to the default branch only, so the child reports mergeStateStatus: CLEAN and can be merged into its base with no approval and no thread resolution, silently folding two reviews into one. And .coderabbit.yaml sets base_branches: ["main"], so CodeRabbit skips the child entirely (it posts a "Review skipped" comment saying so); trigger it with a @coderabbitai review comment. Neither is a reason not to stack, and both are reasons to say in the PR body that it is stacked and what has to happen before the parent merges.
• Branch shape. Squash and rebase merges are enabled and merge commits are disabled, so a branch collapses to one commit on main and its internal shape never lands. Still update with git rebase origin/main and git push --force-with-lease rather than merging main in: the PR diff and CI stay about your change.
• Poll for reviews centrally, not inside each agent, since an agent idling on a review burns its run for nothing. Once a wave's PRs are open, poll until each has a completed CodeRabbit review, then route the fixes back to the original agents with SendMessage (their worktree and context are intact) rather than starting fresh ones.
• Resource note. Each agent running the integration suite boots its own Testcontainers Postgres. Stagger them rather than sharing one TEST_DATABASE_URL: two suites running at once against one server pick the same worker database names and drop each other's database out from under the run. Sharing a server is only safe if each agent gets a distinct database in the URL.

Merging (when asked to merge)

• Squash is the button (merge commits are disabled). The PR title becomes the commit on main and the changelog line, so it has to be the sentence you want released.

• Auto-merge is enabled on the repo, so gh pr merge --squash --auto works while CI runs.

• main is protected, by a ruleset rather than by legacy branch protection. This matters because of how it looks from the API: gh api repos/mozilla-ai/otari/branches/main/protection returns 404 "Branch not protected", which is not evidence of no protection. It only means the rules are not the legacy kind. Read gh api repos/mozilla-ai/otari/rulesets instead, or gh api repos/mozilla-ai/otari/branches/main -q .protected, which reports true. An earlier version of this file claimed main was unprotected on the strength of that 404.

  The protect-main ruleset requires, on the default branch:

  • one approving review, and a bot's COMMENTED review does not satisfy it. CodeRabbit comments rather than approves, so a PR with its threads resolved and green CI still reports mergeStateStatus: BLOCKED and reviewDecision: REVIEW_REQUIRED.
  • every review thread resolved (required_review_thread_resolution). See the warning in Handling the review about your own self-review counting here.
  • an extra approval for unattributed changes (require_extra_approval_for_unattributed_changes). It keys on whether GitHub can attribute every commit in the PR to a user account, so a commit whose author email is not linked to one trips it. That matters here more than anywhere else in this file, because the subject is agents pushing commits: it presents as "green CI, one approval, still BLOCKED", which sends the reader back to the API. Avoid it by committing with an email linked to the account, rather than discovering it.
  • squash or rebase only, plus deletion and non_fast_forward rules on the branch.

  Repository admins are bypass actors, so an admin can merge through all of it. That makes the bar the one you hold yourself, unchanged: green CI, the review addressed, a human approval, and an explicit instruction to merge.

• Branches are deleted on merge, which makes the stacked-PR trap real. Merging parent A with its branch deleted closes child B permanently: B's base is gone and GitHub refuses to reopen a PR whose base branch was deleted (422 "state cannot be changed"), even if you recreate the branch. So gh pr edit B --base main while A's branch still exists, then merge A. Base main is also what gets B any CI at all (see What CI runs on a PR), so it is where a stacked PR should have been sitting all along. If B already closed this way, the only recovery is a fresh PR from B's head branch, linking the old one for its review history.

Non-negotiables

• Never push to main; never merge unless told; confirm before any other outward-facing action, including posting a review or commenting on somebody else's PR.
• Every PR: Fixes #<n>, a Conventional-Commit title, the three template sections, and commit messages carrying a Co-Authored-By: trailer for the model that wrote them.
• A behavior-preserving refactor changes structure, not observable behavior. That is the bar.
• make lint before declaring done, not ruff check: the architecture check runs first and is the half that catches a layer violation.