@@ -12,6 +12,10 @@ session + CSRF, or the agent-token allowlist in `auth_middleware.py`) and a
1212test asserting an unauthenticated or cross-principal caller gets 401/403.
1313Example: PR #2036 added ` GET /api/secrets/agent/{name}/github ` with no guard,
1414leaking installation IDs and repo names to any caller.
15+ For project-scoped routes, copy the house guard verbatim:
16+ ` if not user.is_admin and user.user_id != p["user_id"] ` followed by a masked
17+ 404 (see routes/projects.py). A bare 403 without the admin bypass both locks
18+ out admins and leaks resource existence to other users (#2042 ).
1519
1620** 2. Bind both directions on authenticated channels.**
1721When a message or envelope arrives over an authenticated channel, verify BOTH
@@ -37,6 +41,16 @@ and strips it. Tokens we only VERIFY are stored hashed; tokens we must PRESENT
3741outbound are the only ones stored recoverable. Example: PR #2009 moved the
3842GitHub App RSA key out of plaintext config.yaml.
3943
44+ ** 16. Never commit runtime state or key material.**
45+ Anything a store or subsystem writes under ` data/ ` at runtime is state, not
46+ source. A PR that introduces a component writing under ` data/ ` must add that
47+ directory to ` .gitignore ` in the SAME PR, and ` git status ` must be checked for
48+ runtime artifacts before every commit. Any private key that reaches a pushed
49+ commit is burned: regenerate it, and keep it out of the target branch's history
50+ (squash merge, or rewrite the branch). Example: ` data/hub/identity.json ` with
51+ live signing/encryption keys entered one branch's history and was then
52+ re-committed by a second PR (#2043 history, #2042 ).
53+
4054## Correctness
4155
4256** 5. Never take element ` [0] ` of a collection that can hold more than one.**
@@ -74,6 +88,12 @@ share flow), state it in the PR body and file a follow-up issue. Example: PR
7488permissions. Wire the real value or do not add the parameter yet. Example:
7589PR #2036 ` handleSaveGrants ` .
7690
91+ ** 17. A new view must be wired into every surface it has: desktop AND mobile.**
92+ The desktop tab list and the mobile tab order are separate registries; updating
93+ one and not the other ships a view that is unreachable on phones (#2042 :
94+ ` TABS ` updated, ` mobileTabOrder ` missed). Grep for every registry the sibling
95+ views appear in and update all of them.
96+
7797## Store and schema
7898
7999** 11. SCHEMA is the frozen v1; new columns and their indexes go in MIGRATIONS.**
@@ -87,6 +107,18 @@ Running a data migration twice must be a no-op (existence checks, INSERT OR
87107IGNORE, migrated-from markers) and there must be a test proving the second run
88108changes nothing. Example done right: PR #2028 ` test_migrate_idempotent ` .
89109
110+ ** 18. Retrofitting a column onto an already-shipped store needs a guarded
111+ ALTER, not just a MIGRATIONS entry.**
112+ The migration runner baselines pre-existing databases at the latest version
113+ WITHOUT executing the migrations (FOOTGUN #2 in ` db_migrations.py ` 's own
114+ docstring), so a plain ` MIGRATIONS = [(1, "ALTER TABLE ...")] ` on a store that
115+ already shipped is a silent no-op on every upgraded install: fresh DBs work,
116+ upgraded DBs lose the feature at runtime. Use the guarded ` _post_init ` pattern
117+ (PRAGMA table_info, ALTER only when the column is absent - see
118+ ` knowledge_store._migration_v1_add_user_id ` ) and add an upgrade test that
119+ builds the PRE-change schema first. Fresh-DB tests are structurally blind to
120+ this class (#2043 : ` peer_fingerprint ` ).
121+
90122## Process
91123
92124** 13. A fix and its test belong in one PR.**
@@ -110,3 +142,87 @@ Findings are gated on severity of content, not review state. Address each one
110142(fix it or rebut it concretely in the thread); never merge past an open
111143finding, and never let a "pass" verdict from a stale commit stand in for the
112144current head.
145+ A finding is closed only when it is ANSWERED IN-THREAD: reply to each numbered
146+ item with the commit that addresses it or a concrete rebuttal. Pushing code
147+ without replies leaves the finding open - the reviewer re-verifies blind and
148+ the verdict stays HOLD (#2043 round two).
149+
150+ ** 19. State scope deltas against the issue explicitly.**
151+ If a PR ships less than its issue scopes - a subfeature dropped, owner-only
152+ routes where the issue says peer-serving, a "live" view that fetches once -
153+ say so in the PR body and file the follow-up issue in the same push. Never let
154+ a partial slice close the parent issue. Silent shortfalls read as done, get
155+ caught in review anyway, and cost a full extra round (#2042 : community chat
156+ and peer access absent with no deferral note).
157+
158+ ** 20. A tolerance is not a test of an invariant.**
159+ An assertion like ` assert abs(after - before) <= 2 ` cannot distinguish correct
160+ behaviour from catastrophic failure. In PR #2062 that exact assertion ran green
161+ while reprocess destroyed the user's original uploaded file: the observed values
162+ were ` before=2, after=0 ` , which the tolerance accepted, and the same tolerance
163+ would equally have accepted a doubling to 4. If the property is "the count does
164+ not change", assert equality and assert the resulting status, so the test fails
165+ loudly on both loss and duplication. Reserve tolerances for genuinely
166+ approximate quantities such as timings, and even then bound them tightly.
167+
168+ ** 21. Shell snippets inside template literals must escape ` ${ ` .**
169+ A bash or PowerShell snippet stored in a JavaScript template literal collides
170+ with the language's own interpolation: ` ${VAR:-default} ` is parsed as JS, not
171+ shell. In PR #2077 this produced 225 TypeScript syntax errors from a single
172+ cause, in one of four otherwise-clean files, and read like incoherent output
173+ rather than one mechanical mistake. Escape as ` \${ ` , or keep snippets in plain
174+ non-template strings or separate asset files. The class generalises to any
175+ language sharing ` ${...} ` with the shell, and it is invisible to anything that
176+ does not actually compile the file, which is why the frontend typecheck gate
177+ exists before a PR is opened.
178+
179+ ** 22. Conflict resolution is a decision, not a mechanical act.**
180+ Taking the wrong side of a hunk silently reverts fixes that were just made. When
181+ a base branch has moved substantially under a long-lived branch, read what
182+ changed underneath before resolving: the four defects fixed in Library P1
183+ (nested response envelope, the guard preventing reprocess from deleting the
184+ source upload, the compare-and-swap status transition, exact artifact-count
185+ assertions) are all reintroducible by a plausible-looking resolution. Rebase
186+ rather than merging the base in, so the diff stays reviewable and each conflict
187+ is seen individually.
188+
189+ ** 23. Mocks of EXTERNAL service contracts need a real source, not a guess.**
190+ Mocking our own internals or injecting errors you cannot produce on demand (a
191+ 500, a timeout, an ImportError) is fine and unavoidable. The dangerous class is
192+ narrow: a mock of a service we do not control, whose fixture was hand-written
193+ from what the calling code expects. That fixture encodes a BELIEF about someone
194+ else's API, and when the belief is wrong the test proves nothing while going
195+ green. This happened three times in one PR cycle (#2062 ): an invented ` dbPath `
196+ request shape, an un-nested poll response, and a tolerance assertion over both.
197+
198+ First, split the class by whether the contract has a reachable OWNER.
199+
200+ ** Sibling services (taOSmd, taOS website): ASK. Do not guess.** These are not
201+ third parties. Their maintainer is on the A2A bus, and asking costs one message.
202+ Every failure in the #2062 cycle came from inferring a contract that was free to
203+ obtain: the builder guessed a request shape, and the reviewer verified against
204+ source rather than asking the owner. When we finally asked, we got the envelope
205+ documented, a wrong stats key list corrected by its own author, and a
206+ ` /version ` capabilities endpoint built to make the contract machine-checkable.
207+ Reverse-engineering a sibling's API from its source is a smell, not diligence:
208+ source tells you what it does today, the owner tells you what it guarantees.
209+ Contributors without bus access (external collaborators) ask in the PR, and the
210+ lead relays.
211+
212+ ** Genuinely third-party (GitHub, OpenRouter, Reddit): capture, do not compose.**
213+ There is no one to ask, so call the real service once and commit its response as
214+ the fixture. A recorded response cannot encode a wrong belief.
215+
216+ For both, then:
217+
218+ 1 . ** Keep one real integration test per external contract.** Have it feature
219+ detect and skip when the service is unreachable, so CI stays green offline
220+ but drift surfaces the moment anyone runs it against a live instance. For
221+ taosmd, ` GET /version ` returns a capabilities list for exactly this.
222+ 2 . ** If that is impossible, mark the mock provisional IN CODE** with the
223+ contract source and the date it was verified. A follow-up issue is not
224+ sufficient: it detaches the caveat from the code and, in a tracker with
225+ hundreds of open items, functions as indefinite deferral.
226+
227+ Reviewer's job: ask where the fixture came from. A green test over a fictional
228+ contract is worse than no test, because it certifies the bug.
0 commit comments