You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Commit 238309f
Browse filesBrowse the repository at this point in the historyBrowse files
docs: encode review corrections and fold the resource-ID RFC into main's design
## 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 `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
Copy file name to clipboardExpand all lines: .agents/skills/autoreview/lenses.md
+15-2Lines changed: 15 additions & 2 deletions
Display the source diff
Display the rich diff
Original file line number
Diff line number
Diff line change
@@ -4,7 +4,7 @@ Paste one brief to one reviewer. A brief is the whole assignment: the reviewer h
4
4
5
5
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.
6
6
7
-
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.
7
+
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.
8
8
9
9
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`.
10
10
@@ -21,6 +21,13 @@ Covers Design and Separation of Concern. Read `AGENTS.md` on versioned state, co
21
21
- 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.
22
22
- 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.
23
23
-`platform/` does not import a domain. A domain `core/` does not import a service package. An implementation lives in a subpackage of its interface.
24
+
- 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.
25
+
- A homegrown wrapper over an ecosystem type (a `Logger` interface in front of zap or pino) where the repository never swaps the implementation.
26
+
- 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.
27
+
- 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.
28
+
- Layout mirrors the Go tree in every language: wiring under `service/`, tests under `test/e2e` and `test/integration`, tooling under `tool/`. A framework default (`web/e2e`) or a package named `platform` that is really one host's wiring is a finding.
29
+
- 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 (`/<queue>/request/<id>`) 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.
30
+
- 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.
24
31
25
32
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.
26
33
@@ -32,7 +39,7 @@ Covers Correctness and Bugs. Read `AGENTS.md` on eventual consistency, idempoten
32
39
- 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.
33
40
- 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.
34
41
- 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.
35
-
- 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.
42
+
- 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.
36
43
37
44
## Simplicity and clarity
38
45
@@ -43,6 +50,8 @@ Covers Simplicity. Read `AGENTS.md` on naming, comments, and code style.
43
50
- 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.
44
51
- Value types. `(T, bool)` for absence, not a pointer. An error is a failure, not a branch of normal control flow.
45
52
- 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.
53
+
- 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.
54
+
- Names describe what a thing is, not a metaphor: `githubtestrepo` and `SQ_GITHUB_TEST_REPO`, not `sandbox`.
46
55
47
56
## Tests and hermeticness
48
57
@@ -53,6 +62,8 @@ Covers Testing and Hermeticness. Read `AGENTS.md` on testing and `doc/howto/TEST
53
62
- No `assert` or `require` on `err.Error()`. No `time.Sleep`. No timeout of the test's own.
54
63
- 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.
55
64
- 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.
65
+
- 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.
66
+
- 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.
56
67
- 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.
57
68
58
69
## Operability
@@ -70,6 +81,8 @@ Run only when documentation changed. Read `AGENTS.md` on README files and markdo
70
81
71
82
- 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.
72
83
- Terms match the code. A doc that still says `merging` where the code says `landing` is a finding.
84
+
- 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.
85
+
- 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.
73
86
- 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.
74
87
- 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.
75
88
- A new or renamed RFC is linked from `doc/rfc/index.md`, and links to the old path are updated.
Copy file name to clipboardExpand all lines: .agents/skills/autoreview/review.md
+7-6Lines changed: 7 additions & 6 deletions
Display the source diff
Display the rich diff
Original file line number
Diff line number
Diff line change
@@ -44,18 +44,18 @@ A failed check is a fact. Report the target and the first error. Do not turn a s
44
44
45
45
Read [`lenses.md`](lenses.md). Run a lens only when its files changed:
46
46
47
-
-**Architecture and boundaries** — `go`, `proto`, `sql`, `build`, or `config` is not `none`.
48
-
-**Correctness and failure modes** — `go`, `tests`, `python`, `shell`, `proto`, `sql`, or `config` is not `none`.
49
-
-**Simplicity and clarity** — `go`, `tests`, `python`, `shell`, `proto`, `sql`, `build`, `config`, or `other` is not `none`.
50
-
-**Tests and hermeticness** — `go`, `tests`, `python`, `shell`, `build`, `config`, `generated`, or `other` is not `none`.
47
+
-**Architecture and boundaries** — `go`, `web`, `proto`, `sql`, `build`, or `config` is not `none`.
48
+
-**Correctness and failure modes** — `go`, `web`, `tests`, `python`, `shell`, `proto`, `sql`, or `config` is not `none`.
49
+
-**Simplicity and clarity** — `go`, `web`, `tests`, `python`, `shell`, `proto`, `sql`, `build`, `config`, or `other` is not `none`.
50
+
-**Tests and hermeticness** — `go`, `web`, `tests`, `python`, `shell`, `build`, `config`, `generated`, or `other` is not `none`.
51
51
-**Operability** — the diff changes logging, metrics, or an error returned to a caller.
52
52
-**Docs and RFCs** — `docs` is not `none`.
53
53
54
54
Generated files (`protopb/`, `mock/`, `*_mock.go`, `*.pb.go`) are not a lens of their own. Each lens checks whether they were regenerated, and does not review them line by line.
55
55
56
56
When you can start subagents, start one per applicable lens in parallel. Give each the lens brief, the normalized intent and its source, the `review-scope` output, the diff, and nothing from the conversation that wrote the change. When you cannot, finish one lens and set it aside before reading the next brief.
57
57
58
-
Pass a lens the smells that match it: `secondary-index` and `extension-factory`to architecture; `narration-comment` to simplicity; `sleep`, `error-string-assert`, `external-test-package`, `repo-root`, and `shell-script` to tests and hermeticness; `formatted-log` to operability. The lens confirms or drops each one.
58
+
Pass a lens the smells that match it: `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. The lens confirms or drops each one.
59
59
60
60
If the diff is too large to pass, give the reviewer the file list and have it read the files. Anything unread is listed in the report, and the verdict is incomplete.
61
61
@@ -67,7 +67,8 @@ If the diff is too large to pass, give the reviewer the file list and have it re
67
67
- Severity is based on consequence: **Blocker** means reproducible merge-unsafe behavior such as data loss, incorrect state, outage, security exposure, or incompatible rollout. **Should-fix** means a reachable material defect with a bounded fix appropriate to this change. **Nit** means style, preference, or process consistency without behavioral impact. Confidence is high, medium, or low.
68
68
- A question is a missing fact, not a severity. A material unresolved question makes coverage incomplete.
69
69
- Nits stay out unless the user asked for a wider pass.
70
-
- Sweep `AGENTS.md` for rules no lens covered: layout and import paths, committed proto generation, entity field comments, string enums, int64 milliseconds, Makefile target order, and commit shape when the range has commits (conventional, subject under 70 characters, one line per paragraph, no `Co-Authored-By` / `Co-authored-by`).
70
+
- Sweep `AGENTS.md` for rules no lens covered: layout and import paths, committed proto generation, entity field comments, string enums, int64 milliseconds, Makefile target order, the Design Defaults, and commit shape when the range has commits (conventional, subject under 70 characters, one line per paragraph, no `Co-Authored-By` / `Co-authored-by`, no leftover `# Conflicts:` or rebase-editor lines).
71
+
- The repository is public. An Uber-internal name in the diff, a commit message, or the PR body is a **Should-fix** under Architecture and contracts, whether or not `internal-reference` fired; grep the commit messages yourself, since the script scans only the diff.
71
72
- Commit-shape findings must quote the offending subject or trailer from `git log <base>..HEAD` (or the named commit). Drop any claim that contradicts the message body you just read. A `Co-authored-by` trailer is a **Nit** (process/policy), never Blocker or Should-fix, and belongs under Nits only when the user asked for nits or for a commit-message pass.
72
73
- List files in scope that were not read, checks that were skipped, and lenses that did not finish.
0 commit comments