From cb26534db7a7298dda950fe0851e5eaaff773afa Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Wed, 7 Oct 2026 19:45:21 -0700 Subject: [PATCH] docs: encode review corrections and fold the resource-ID RFC into main's design MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? Recent sessions kept getting the same review corrections: speculative packages and adapters, homegrown wrappers, host code in the library, framework layouts, non-Bazel tooling, re-encoded IDs, internal names in a public repo, separate CI workflows, and RFCs that describe discussion rather than main. Autoreview also never ran its architecture or correctness lenses on TypeScript, because `.ts`/`.tsx` files classified as `other`. One such RFC already exists: queue-scoped decimal IDs shipped (#770, #804), but the decision still lived in a standalone "current vs proposed" RFC. ### What? - `AGENTS.md`: new **Design Defaults** section, aligned with the web UI RFC (#810); CI credential and workflow rules; naming, RFCs-describe-main, and concise-docs bullets; ask before splitting a large change into a stack; local run steps in the Test Plan. - Autoreview `review-scope.py`: new `web` kind (`.ts`, `.tsx`, `.js`, `.css`; `*.test.*` as tests), and `internal-reference` and `new-workflow` smells. - Autoreview dispatch: the parent runs a planner, one lens reviewer per assignment in parallel, and a consolidator, because subagents cannot start their own. Committed targets are reviewed in a clean detached worktree so local edits never skip checks, and every readable changed file must appear in some reviewer's Read list or the report marks it unreviewed. Coverage is counted from Read/Partial/Unread buckets rather than judged, and every finding must be re-opened at its cited line before it is admitted, with a recheck log in the report. - Autoreview `review.md` / `lenses.md`: route `web` to all code lenses; treat unneeded added surface as should-fix; add checks for each design default and for RFCs that diverge from main; sweep commit messages for internal names; skip data-migration findings for OSS deployments. - RFCs: add a **Resource IDs** section to `doc/rfc/submitqueue/workflow.md`, delete `doc/rfc/scoped-resource-ids.md` and its index entry, fix stale ID text in the history and Stovepipe workflow RFCs, delete the superseded SubmitQueue `list-api.md` proposal (shipped as `status-list-api.md`), and describe the implemented Stovepipe List API as current rather than proposed. ## Test Plan ✅ `bazel test //.agents/skills/autoreview/scripts:all` ✅ `bazel test //tool/docsite:site_test` (strict link check) ✅ `make fmt` leaves the tree unchanged --- .agents/skills/autoreview/SKILL.md | 56 +++- .agents/skills/autoreview/lenses.md | 19 +- .agents/skills/autoreview/review.md | 142 +++++++--- .../skills/autoreview/scripts/review-scope.py | 34 ++- .../autoreview/scripts/review_scope_test.py | 43 +++ AGENTS.md | 22 ++ doc/rfc/index.md | 5 +- doc/rfc/scoped-resource-ids.md | 69 ----- doc/rfc/stovepipe/list-api.md | 4 +- doc/rfc/stovepipe/request-history-api.md | 2 +- doc/rfc/stovepipe/workflow.md | 2 +- doc/rfc/submitqueue/history-api.md | 2 +- doc/rfc/submitqueue/list-api.md | 263 ------------------ doc/rfc/submitqueue/status-list-api.md | 2 +- doc/rfc/submitqueue/workflow.md | 16 ++ 15 files changed, 280 insertions(+), 401 deletions(-) delete mode 100644 doc/rfc/scoped-resource-ids.md delete mode 100644 doc/rfc/submitqueue/list-api.md diff --git a/.agents/skills/autoreview/SKILL.md b/.agents/skills/autoreview/SKILL.md index e2fbac1b7..24baf4d3c 100644 --- a/.agents/skills/autoreview/SKILL.md +++ b/.agents/skills/autoreview/SKILL.md @@ -8,28 +8,56 @@ description: >- # Autoreview dispatcher -Always run the review in a dedicated subagent. The parent context only dispatches the work and returns the result. +The review runs in subagents. Subagents cannot start subagents of their own, so this parent context coordinates three phases and relays their outputs verbatim. It never reads the diff, runs checks, opens changed files, or writes findings itself. -## Dispatch +Every subagent is a general-purpose subagent on the inherited model, run in the foreground. Give each one only what its prompt below names: nothing from the conversation that wrote the change. -1. Launch exactly one `generalPurpose` subagent with `run_in_background: false` and the inherited model. -2. Give it only: - - the absolute repository path, - - the target exactly as the user named it, or `current branch changes including uncommitted work` when no target was named, - - the user's review instruction. -3. Use this worker prompt: +## 1. Plan + +Launch one planner: ```text -You are the autoreview worker. Execute the review yourself; do not delegate the whole review. +You are the autoreview planner. Repository: -Target: +Target: Request: -Read and execute `.agents/skills/autoreview/review.md` completely. Read `lenses.md` only as directed there. Do not use conversation context about how the change was authored. Return only the final autoreview report. +Read `/.agents/skills/autoreview/review.md` and execute its "Plan" section only. Return only the plan block it specifies. +``` + +## 2. Lenses + +For every assignment in the plan, launch one lens reviewer. Launch them all in one message so they run in parallel: + +```text +You are an autoreview lens reviewer. + +Plan header: + + +Assignment: + + +Read `/.agents/skills/autoreview/review.md` and execute its "Lens" section for this assignment only. Return only the lens result it specifies. +``` + +## 3. Consolidate + +Launch one consolidator: + +```text +You are the autoreview consolidator. + +Plan: + + +Lens results: + + +Read `/.agents/skills/autoreview/review.md` and execute its "Consolidate" and "Report" sections. Return only the final report. ``` -4. Do not inspect the diff, run mechanical checks, open changed files, or produce findings in the parent context. -5. Return the worker's final report unchanged. Do not weaken, expand, or reinterpret its findings. +Return the consolidator's report unchanged. Do not weaken, expand, or reinterpret its findings. -If a subagent cannot be launched, report the review as incomplete and explain that isolated execution was unavailable. Do not fall back to reviewing in the parent context. +If a subagent cannot be launched or fails, do not review in the parent context and do not drop its part silently. Pass the failure to the consolidator as that assignment's result, so it lands under Not reviewed. If the planner or consolidator fails, report the review as incomplete and name the phase. diff --git a/.agents/skills/autoreview/lenses.md b/.agents/skills/autoreview/lenses.md index c03a59155..9209ca322 100644 --- a/.agents/skills/autoreview/lenses.md +++ b/.agents/skills/autoreview/lenses.md @@ -2,9 +2,9 @@ Paste one brief to one reviewer. A brief is the whole assignment: the reviewer has not seen the conversation that wrote the change, and it does not edit files. -Generated files (`protopb/`, `mock/`, `*_mock.go`, `*.pb.go`, `*.pb.yarpc.go`) are not reviewed line by line. Say only whether they look stale relative to their source. +Generated files (`protopb/`, `mock/`, `*_mock.go`, `*.pb.go`, `*.pb.yarpc.go`, `*_pb.ts`) are not reviewed line by line. Say only whether they look stale relative to their source. -Report a finding only when the changed code creates a reachable material impact. For each item give severity (`blocker`, `should-fix`, or `nit` when requested), confidence (`high`, `medium`, or `low`), changed `path:line`, observable impact, the shortest reproducing scenario or static caller trace, supporting locations, the smallest safe fix in this change, and a specific verification step. Cite the `AGENTS.md` section when it explains the engineering constraint, but a rule violation without behavioral, contract, verification, or maintenance impact is a nit and is omitted unless requested. +Report a finding only when the changed code creates a reachable material impact. Added surface counts as material: a package, interface, adapter, file, dependency, workflow, or call the stated intent does not need is a should-fix, because it is code every later change has to carry and the first thing a human reviewer asks about. Say what breaks if it is deleted; if nothing does, the fix is to delete it. For each item give severity (`blocker`, `should-fix`, or `nit` when requested), confidence (`high`, `medium`, or `low`), changed `path:line`, observable impact, the shortest reproducing scenario or static caller trace, supporting locations, the smallest safe fix in this change, and a specific verification step. Cite the `AGENTS.md` section when it explains the engineering constraint, but a rule violation without behavioral, contract, verification, or maintenance impact is a nit and is omitted unless requested. When impact or reachability cannot be established, report a question with the exact missing fact. A material question makes review coverage incomplete. Inspect the surrounding implementation and tests needed to support the claim; the changed line alone is not evidence of a system-level defect. Do not restate a mechanical failure (formatting, gazelle, mocks, lint, or a failed test). If nothing is wrong, answer `No findings`. @@ -21,6 +21,13 @@ Covers Design and Separation of Concern. Read `AGENTS.md` on versioned state, co - A decision or action extension takes the thin reference entity (`entity.Request`, `entity.Batch`) and resolves changes, diffs, and targets itself. `conflict.Analyzer` is the reference shape. - Factories and per-queue routing live in service wiring. `NewFactory` under an `extension/` package violates that ownership when it couples the extension to topology or implementation selection. A service-scoped aggregate, implementation, mocks, and schema live with the service that resolves them; the behavioral contracts stay shared. - `platform/` does not import a domain. A domain `core/` does not import a service package. An implementation lives in a subpackage of its interface. +- Built for consumers in scope. A shared package, an extension point, an adapter, or a second supported path (a fallback format, a host that "can't" do the standard thing) that exists for a consumer the change does not ship is speculative. Extraction waits for the second real consumer. +- A homegrown wrapper over an ecosystem type (a `Logger` interface in front of zap or pino) where the repository never swaps the implementation. +- Host and framework details stay in the host; the library owns the complete behaviour. In web code, a framework (Next.js), generated-binding, or transport import under `web/platform/` or the domain module is a finding, and so is product layout, styling, or page assembly in the host. The module depends on the structural shape of the gateway client; only the host picks implementations. +- A new or moved boundary is enforced by Bazel `visibility` or a forbidden-import test, not only stated in a README or RFC. Generated bindings with several in-repo consumers stay with the contract, private to those consumers. +- Layout mirrors the Go tree in every language. A non-Go workspace repeats the root layout inside itself (`web/service`, `web/test/e2e`, `web/tool`); placing it there instead of the repo-level `test/` or `tool/` is correct, not a finding. A framework default (`web/e2e`) or a package named `platform` that is really one host's wiring is a finding. +- Identifiers are opaque. Re-encoding an ID for a URL or UI (base64, hashes), or packing auxiliary data into one (file lists in a change URI), is a finding. The path follows the ID's scope (`//request/`) or mirrors the URI it names. Timestamps or a fixed window in a page URL, so refresh does not show live data, are a finding; an older page is an opaque cursor. +- Public-repository hygiene. Confirm every `internal-reference` smell: an Uber-internal system, host, repository, library, or service name in code, tests, or docs is should-fix. Describing an internal consumer by its properties is fine. Trace both sides of a boundary before reporting it: producer and consumer for queue contracts, caller and implementation for interfaces, controller and store for versioned writes. State the concrete coupling, backend incompatibility, race, or rollout failure the boundary violation creates. @@ -32,7 +39,7 @@ Covers Correctness and Bugs. Read `AGENTS.md` on eventual consistency, idempoten - Extensions return plain errors. A classifier decides retry. A failed publish is not wrapped as retryable just so the handler runs again. `storage.ErrNotFound` is a user error only at a call site that knows the user asked for a missing resource. - Races, lost updates, a wrong version guard, and a publish that can run before its write. Give the concrete interleaving. Context cancellation is material only when the call can block or leak work after cancellation. - A concrete defect in the changed lines: nil or empty identity, a mishandled `""` enum sentinel, an off-by-one, an error that is dropped. A design disagreement is not a bug. -- Rolling compatibility. Proto field numbers stay stable and evolution stays additive. For schema or message findings, name the incompatible old/new binary combination and deployment order; do not infer a rollout failure from `NOT NULL` alone. +- Rolling compatibility. Proto field numbers stay stable and evolution stays additive. For schema or message findings, name the incompatible old/new binary combination and deployment order; do not infer a rollout failure from `NOT NULL` alone. Deployments run from this repository (local, demo, CI) hold no data worth migrating, so do not raise a backfill of existing rows for them. When a change alters a persisted format, ask one question about downstream deployments that consume this repository instead of reporting a finding. ## Simplicity and clarity @@ -43,6 +50,8 @@ Covers Simplicity. Read `AGENTS.md` on naming, comments, and code style. - A false comment that changes a reader's understanding of behavior is should-fix. Narration and comment-budget violations are nits. Prefer a clearer name over a comment. - Value types. `(T, bool)` for absence, not a pointer. An error is a failure, not a branch of normal control flow. - Dead code that expands the maintenance or behavior surface. Treat an intent mismatch as a question unless it creates review risk or changes behavior outside the stated scope. +- Work the behavior does not need: an upfront pass over every item when each item is checked before it is used, an RPC whose result does not reach the output, a config file that restates defaults, a shell function or wrapper the program could absorb as a flag. Each adds a failure mode for nothing. +- Names describe what a thing is, not a metaphor: `githubtestrepo` and `SQ_GITHUB_TEST_REPO`, not `sandbox`. ## Tests and hermeticness @@ -53,6 +62,8 @@ Covers Testing and Hermeticness. Read `AGENTS.md` on testing and `doc/howto/TEST - No `assert` or `require` on `err.Error()`. No `time.Sleep`. No timeout of the test's own. - Integration and e2e inputs come from runfiles: `testutil.Runfile`, `testutil.WithBuildContext`, and Bazel `data`. Resolving the repo root (`show-toplevel`, `FindRepoRoot`) is a finding. A compose context is `{category}-{domain}-{name}`. Docker or network access has the `integration` and `requires-network` tags. - Repository automation and validation run through Bazel with pinned toolchains and declared `srcs`/`data`; Make targets are thin entry points, not a second build graph. Report host-installed tools, undeclared files, runtime downloads, or direct source-tree assumptions when they make local, CI, or remote execution diverge. +- A new language or tool is built by Bazel end to end. A side-car `go.mod`, a virtualenv, or a package-manager-only build next to the Bazel graph is a finding. A local server binds a free port the way the Compose services do, not a hard-coded one. +- CI work is a job in `.github/workflows/ci.yml`, not a new workflow or a nightly schedule; confirm every `new-workflow` smell. A test that needs a credential reads a repository secret plus a repository variable naming its target, never a GitHub environment (which records deployments) or a hard-coded repository. It skips only when the repository has not opted in and fails when it has opted in but the credential is missing, so a deleted or expired secret cannot turn CI silently green. - Nontrivial scripting is a Python `py_binary`/`py_test` or a Go binary like the repository linters, not Bash. A shell file is limited to an unavoidable `exec`-style launcher with no parsing, branching workflow, or domain logic. For a finding, identify the portability, quoting, dependency, or hermetic-execution failure and name the Python or Go target that should own the logic. ## Operability @@ -70,6 +81,8 @@ Run only when documentation changed. Read `AGENTS.md` on README files and markdo - A decision names material alternatives and why one was chosen when that context is needed to evaluate or safely change it. Missing alternatives are otherwise a question or nit. - Terms match the code. A doc that still says `merging` where the code says `landing` is a finding. +- An RFC reflects main. A finding is any of: "proposed" or "before/after" wording for behavior that has shipped; a standalone discussion or superseded RFC kept next to the RFC that owns the topic; or a description that contradicts the code it names. The fix is to fold the decision into the owning RFC, update references, and delete the rest. +- Concise. A UX section is a mock image and a one-line description. A section that restates the code, or that a reader has to scroll through to reach the decision, is a finding with the cut named. - A README describes behavior in prose. It does not paste an interface or a type definition. A short example is fine only where the doc already says to include one. - Prose is one line per paragraph and one line per list item. Hard wrapping is a nit unless it breaks rendering or generated processing. Code blocks, tables, and diagrams keep their own line breaks. - A new or renamed RFC is linked from `doc/rfc/index.md`, and links to the old path are updated. diff --git a/.agents/skills/autoreview/review.md b/.agents/skills/autoreview/review.md index f6e85221c..1902839da 100644 --- a/.agents/skills/autoreview/review.md +++ b/.agents/skills/autoreview/review.md @@ -1,31 +1,36 @@ -# Autoreview worker +# Autoreview phases -Review the change. Do not edit it, do not commit, and do not post to the pull request. Findings are advice. A clean verdict covers only the target below, and a review that did not finish is not clean. +Review the change. Do not edit it, do not commit, and do not post to the pull request. Findings are advice. A clean verdict covers only the planned target, and a review that did not finish is not clean. -Run the four stages in order. `AGENTS.md` is the source of truth; `CLAUDE.md` is a copy, so do not read it separately. +`AGENTS.md` in the repository is the source of truth; `CLAUDE.md` is a copy, so do not read it separately. Execute only the section your prompt names. -## 1. Scope and intent +## Plan -Run `bazel run //.agents/skills/autoreview/scripts:review-scope --` from the repository, followed by any arguments below. The tool is read-only and does not fetch. Its source is [`scripts/review-scope.py`](scripts/review-scope.py). +### Review tree + +Checks and reading run against a tree that is exactly the reviewed target, so local edits never block or skew them. + +- When the target includes uncommitted work (the default, or `--uncommitted`), the review tree is the repository working tree. +- Otherwise resolve the head SHA: `headRefOid` for a pull request, the named commit for `--commit`, `HEAD` for `--range`. Then create a detached tree outside the repository: `work=$(mktemp -d -t autoreview)` and `git -C worktree add --detach "$work/tree" `. Bazel gives the new tree a fresh output base, so its first build is slow; that is expected. +- For uncommitted targets, still create `work=$(mktemp -d -t autoreview)` for the plan's files. + +### Scope and intent + +Run the repository's copy of the scope script from inside the review tree: `python3 -I /.agents/skills/autoreview/scripts/review-scope.py > "$work/scope.txt"`. It is read-only and does not fetch. - By default it reviews committed changes since the merge-base with `origin/main`, plus staged, unstaged, and untracked files. - `--base REF` when the user names a different base. -- `--range REF` when the target is committed work only; dirty work is excluded. -- `--commit REV` when the user names one commit. Dirty work is excluded. +- `--range REF` when the target is committed work only. +- `--commit REV` when the user names one commit. - `--uncommitted` when the user wants only uncommitted work. -- When the user names a pull request, read it with `gh pr view N --json baseRefName,baseRefOid,headRefOid,title,body`. Verify local `HEAD` equals `headRefOid` and `baseRefOid` exists locally, then pass `--range ` so unrelated dirty work is excluded. Do not fetch. If either check fails, stop and report the review as incomplete rather than reviewing a different revision. +- When the user names a pull request, read it with `gh pr view N --json baseRefName,baseRefOid,headRefOid,title,body`. Confirm both `headRefOid` and `baseRefOid` exist locally (`git cat-file -e`), create the review tree at `headRefOid`, and pass `--range `. Do not fetch. If either object is missing, stop and return a plan that marks the review incomplete rather than reviewing a different revision. -Read the full commit messages (`git log ..HEAD`) or the pull request body, not just the subjects the script prints. Write a one-line intent and record whether it came from the PR body, commit messages, or the user. Use `unspecified` when none states an intent; do not invent one. Hold the change to that intent, including changes that do not belong in it. +Read the full commit messages (`git log ..`) or the pull request body, not just the subjects the script prints. Write a one-line intent and record whether it came from the PR body, commit messages, or the user. Use `unspecified` when none states an intent; do not invent one. -The script's `## smells` lines are leads. A hit is not a finding until a lens confirms it in the code. `## hints` means an interface declaration or mock-generation source changed. +### Mechanical checks -Read context from the reviewed snapshot. Branch and uncommitted targets use the working tree. A range target uses `git show HEAD:` and its merge-base for before-state; a commit target uses `git show :` and its parent. Do not open the working-tree copy for a committed-only target when dirty work is excluded or the named commit is not `HEAD`. If required context cannot be read from the Git objects, mark coverage incomplete. +Run only the checks that match the file list, from the review tree, and write each result to `$work/checks.txt`. -## 2. Mechanical checks - -Run only the checks that match the file list. - -- Run checks only when the filesystem materializes the reviewed target. A branch or uncommitted review uses the current tree. For `--range`, any dirty work means all checks are skipped. For `--commit`, all checks are skipped unless the named commit equals `HEAD` and the tree is clean. Record `skipped: reviewed target is not materialized` and mark verification limited; never report results from a different tree. - `make check-gazelle` when `go` or `build` is not `none`. - `make check-mocks` when `## hints` is not `none`, or a file under `mock/` changed. - `make check-tidy` when `MODULE.bazel`, `go.mod`, or `go.sum` changed. @@ -34,46 +39,96 @@ Run only the checks that match the file list. - `bazel build` (or `./tool/bazel build` when `bazel` is not on `PATH`) on each package under `## packages`: append `:all`, so `//` becomes `//:all` and `//pkg` becomes `//pkg:all`. - For each affected package, query `tests(:all)`. Run `bazel test` only on the returned labels. An empty query is `not applicable`, not a failure. Exclude `integration` and `e2e` tags unless a changed file is an integration or e2e test. Do not test `//...`. -`make lint`, `make check-gazelle`, `make check-mocks`, `make check-tidy`, and `make proto` rewrite files and then require a clean tree. Skip them when `dirty: true`, record `skipped: dirty tree`, and mark verification limited. When the tree is clean, record `git status --porcelain` first and compare it after every command. A successful command that leaves changes is a failed stale-output check. Restore the tracked paths with `git restore` and remove only untracked paths that were not in the earlier status, whether the command failed or merely left edits. Do not leave the rewrite in the worktree. - -`bazel test` does not edit source. Run it on a dirty tree too. - -A failed check is a fact. Report the target and the first error. Do not turn a smell into a failed check. - -## 3. Independent lenses - -Read [`lenses.md`](lenses.md). Run a lens only when its files changed: +`make lint`, `make check-gazelle`, `make check-mocks`, `make check-tidy`, and `make proto` rewrite files and then require a clean tree. A detached review tree is always clean. In the repository working tree, skip them when `dirty: true`, record `skipped: dirty tree`, and mark verification limited. When the tree is clean, record `git status --porcelain` first and compare it after every command. A successful command that leaves changes is a failed stale-output check. Restore the tracked paths with `git restore` and remove only untracked paths that were not in the earlier status. Do not leave the rewrite in the tree. + +`bazel test` does not edit source; run it on a dirty tree too. A failed check is a fact: record the target and the first error. Do not turn a smell into a failed check. + +### Assignments + +Every changed file a reviewer must read lands in exactly one assignment, and each assignment is small enough for one reviewer to read completely. + +- Readable files are the changed files except generated ones (kind `generated`, plus `*_pb.ts`) and lockfiles (`pnpm-lock.yaml`, `go.sum`, `MODULE.bazel.lock`, `*requirements_lock.txt`). Generated files are judged only for staleness, by the checks above or by the reviewer of their source. +- Group readable files by Bazel package (the nearest `BUILD.bazel`), and docs by directory. Merge small groups in the same top-level area; split a group above 15 files or about 1,200 changed lines. +- Give each group the lenses whose kinds it contains, by this table: + - **Architecture and boundaries**: `go`, `web`, `proto`, `sql`, `build`, or `config`. + - **Correctness and failure modes**: `go`, `web`, `tests`, `python`, `shell`, `proto`, `sql`, or `config`. + - **Simplicity and clarity**: `go`, `web`, `tests`, `python`, `shell`, `proto`, `sql`, `build`, `config`, or `other`. + - **Tests and hermeticness**: `go`, `web`, `tests`, `python`, `shell`, `build`, `config`, `generated`, or `other`. + - **Operability**: the group's diff changes logging, metrics, or an error returned to a caller. + - **Docs and RFCs**: `docs`. +- Give each group the smells for its files that match its lenses: `secondary-index`, `extension-factory`, and `internal-reference` to architecture; `narration-comment` to simplicity; `sleep`, `error-string-assert`, `external-test-package`, `repo-root`, `shell-script`, and `new-workflow` to tests and hermeticness; `formatted-log` to operability. +- When the change spans more than one package, add one cross-cutting architecture assignment. It reviews the seams between groups (import direction, Bazel visibility, layering, layout against `AGENTS.md`) from `BUILD.bazel` files, package entry points, and imports, not every line. +- Aim for at most 12 assignments. Past that, raise the group size; never drop a file. + +### Plan block + +Return exactly this, with the header ending at the `Excluded` line: + +```text +# Autoreview plan +Repository: +Review tree: ( | repository working tree) +Work dir: (scope.txt, checks.txt) +Base: Head: [ plus working tree] +Target: +Intent: () +Checks: passed ; failed ; skipped +Excluded from reading: + +## Assignment A1 +Lenses: +Smells: