REVIEW-OF-RECORD — REQUEST CHANGES (#682, head 6e37ab00b99ca76b4bcb2fe0565d016b5051b18b). BLOCKER: notes list can leak other users' mission_tasks.notes for team-scoped missions. In FederationListQueryService.listNotes(), when filter.teamIds is non-empty, the query adds missionTasks.missionId IN (team mission ids) as an OR branch without also constraining missionTasks.userId to filter.subjectUserId. The schema explicitly treats mission_tasks as user-scoped for multi-tenant RBAC isolation, and the normal gateway mission-task path filters by mission AND user. As written, a grant with include_teams for notes can return every user's notes on accessible team missions, widening beyond the subject user. Please keep missionTasks.userId = subjectUserId on all missionTasks note rows (or otherwise prove/document a distinct team-owned notes model) and add regression tests for: (1) team-scoped notes do not include another user's missionTasks rows, and (2) include_personal:false does not return subject personal notes unless an explicitly allowed team-owned note model exists.
Reviewed positives: controller route/auth metadata is correct; request limit/cursor validation happens before scope/query; ScopeService deny maps to federation error envelope and does not query storage; row cap/pagination uses limit+1 and opaque keyset cursors; _source: local tagging is applied; sensitive credentials/api_keys are denied by native RBAC for M3. Reviewer verification: scoped tests list.controller.spec.ts list-query.service.spec.ts pass 9/9; gateway typecheck pass; gateway lint pass; prettier check pass. CI may continue running, but merge should wait on the notes isolation fix.
REVIEW-OF-RECORD — REQUEST CHANGES (#682, head 6e37ab00b99ca76b4bcb2fe0565d016b5051b18b). BLOCKER: `notes` list can leak other users' `mission_tasks.notes` for team-scoped missions. In `FederationListQueryService.listNotes()`, when `filter.teamIds` is non-empty, the query adds `missionTasks.missionId IN (team mission ids)` as an OR branch without also constraining `missionTasks.userId` to `filter.subjectUserId`. The schema explicitly treats `mission_tasks` as user-scoped for multi-tenant RBAC isolation, and the normal gateway mission-task path filters by mission AND user. As written, a grant with `include_teams` for notes can return every user's notes on accessible team missions, widening beyond the subject user. Please keep `missionTasks.userId = subjectUserId` on all missionTasks note rows (or otherwise prove/document a distinct team-owned notes model) and add regression tests for: (1) team-scoped notes do not include another user's missionTasks rows, and (2) `include_personal:false` does not return subject personal notes unless an explicitly allowed team-owned note model exists.
Reviewed positives: controller route/auth metadata is correct; request `limit`/`cursor` validation happens before scope/query; ScopeService deny maps to federation error envelope and does not query storage; row cap/pagination uses limit+1 and opaque keyset cursors; `_source: local` tagging is applied; sensitive `credentials`/`api_keys` are denied by native RBAC for M3. Reviewer verification: scoped tests `list.controller.spec.ts list-query.service.spec.ts` pass 9/9; gateway typecheck pass; gateway lint pass; prettier check pass. CI may continue running, but merge should wait on the notes isolation fix.
INDEPENDENT REVIEW-OF-RECORD — REQUEST-CHANGES (reviewed by an independent code-review agent dispatched by Mos; this review applies to commit 6e37ab00 — note a newer commit b38b9846 has since been pushed and needs the same scrutiny / may already address some of these).
This PR has BLOCKING issues; CI being green does not make the diff sound. Do not merge until resolved.
CRITICAL (must fix):
Cross-scope data leak in listNotes (list-query.service.ts) — when includePersonal is true, the personal clause eq(missionTasks.userId, subjectUserId) is OR'd with the mission-scoped inArray(missionTasks.missionId, missionIds). The OR means "all notes created by the subject ANYWHERE, OR all notes in authorized missions" — a federation peer can enumerate notes the subject created in missions outside the authorized grant scope. The personal clause must be INTERSECTED with the authorized mission set (AND), not OR'd — or removed entirely in favor of project/mission scope only.
listMemory pagination is incorrect across pages — two parallel sub-queries each apply the same keyset cursor against separate tables, merged + re-sorted in memory; the emitted cursor cannot be deterministically resumed, so rows are duplicated/skipped at page boundaries (equal timestamps interleave). Either paginate the two resources separately, or return a single deterministic capped block.
Missing notes test coverage — the resource with the most complex scoping (and the leak above) has ZERO tests. Add specs proving unauthorized-mission notes are excluded and includePersonal does not over-share.
IMPORTANT (fix concurrently):
4. list.controller.ts missing-context path throws a plain Error → 500 leaking internal message; use a federation HttpException envelope (FederationUnauthorizedError/InvalidRequest).
5. encodeCursor returns undefined for non-Date createdAt while hasMore is true → silent pagination truncation (peer sees _truncated with no nextCursor, cannot continue). Throw instead.
6. listAllRows switch has no default → returns undefined for unknown resource → paginate(undefined) crash. Add default: throw.
The federation auth guard, scope delegation (per #672), and sensitive-resource (credentials/api_keys) denial are all sound. The blocker is the notes scope leak. Please address #1–#3 (and ideally #4–#6) and re-push; I'll re-review and shepherd on a clean solo green CI run.
INDEPENDENT REVIEW-OF-RECORD — REQUEST-CHANGES (reviewed by an independent code-review agent dispatched by Mos; this review applies to commit 6e37ab00 — note a newer commit b38b9846 has since been pushed and needs the same scrutiny / may already address some of these).
This PR has BLOCKING issues; CI being green does not make the diff sound. Do not merge until resolved.
CRITICAL (must fix):
1. **Cross-scope data leak in `listNotes` (list-query.service.ts)** — when `includePersonal` is true, the personal clause `eq(missionTasks.userId, subjectUserId)` is OR'd with the mission-scoped `inArray(missionTasks.missionId, missionIds)`. The OR means "all notes created by the subject ANYWHERE, OR all notes in authorized missions" — a federation peer can enumerate notes the subject created in missions outside the authorized grant scope. The personal clause must be INTERSECTED with the authorized mission set (AND), not OR'd — or removed entirely in favor of project/mission scope only.
2. **`listMemory` pagination is incorrect across pages** — two parallel sub-queries each apply the same keyset cursor against separate tables, merged + re-sorted in memory; the emitted cursor cannot be deterministically resumed, so rows are duplicated/skipped at page boundaries (equal timestamps interleave). Either paginate the two resources separately, or return a single deterministic capped block.
3. **Missing `notes` test coverage** — the resource with the most complex scoping (and the leak above) has ZERO tests. Add specs proving unauthorized-mission notes are excluded and `includePersonal` does not over-share.
IMPORTANT (fix concurrently):
4. `list.controller.ts` missing-context path throws a plain `Error` → 500 leaking internal message; use a federation HttpException envelope (FederationUnauthorizedError/InvalidRequest).
5. `encodeCursor` returns `undefined` for non-Date createdAt while `hasMore` is true → silent pagination truncation (peer sees _truncated with no nextCursor, cannot continue). Throw instead.
6. `listAllRows` switch has no `default` → returns `undefined` for unknown resource → `paginate(undefined)` crash. Add `default: throw`.
The federation auth guard, scope delegation (per #672), and sensitive-resource (credentials/api_keys) denial are all sound. The blocker is the notes scope leak. Please address #1–#3 (and ideally #4–#6) and re-push; I'll re-review and shepherd on a clean solo green CI run.
REVIEW-OF-RECORD — APPROVE (#682 FED-M3-05 list verb, head b38b9846c170da1fb64eb49feebd6acacd2b2117). Re-review confirms the prior notes isolation blocker is closed: listNotes() now constrains every mission_tasks note row by missionTasks.userId = subjectUserId AND accessible mission IDs, so team scope narrows allowed missions but does not widen to other users' mission task notes. Added PGlite regressions cover both another-user team mission note exclusion and includePersonal:false excluding subject personal mission notes.
Trust-boundary review: route is POST /api/federation/v1/list/:resource under FederationAuthGuard; request limit/cursor validation happens before storage access; FederationScopeService remains the allow/deny gate with query service as native RBAC evaluator; scope deny returns federation error envelopes and does not query storage; sensitive credentials/api_keys are denied in M3 native RBAC; rows are capped/paginated with opaque keyset cursor and tagged _source: local; no request/response persistence or audit writes (M4-deferred by plan). Reviewer verification on head b38b9846: scoped list tests 11/11 pass; gateway typecheck pass; gateway lint pass; prettier check pass. APPROVE.
REVIEW-OF-RECORD — APPROVE (#682 FED-M3-05 list verb, head b38b9846c170da1fb64eb49feebd6acacd2b2117). Re-review confirms the prior notes isolation blocker is closed: `listNotes()` now constrains every mission_tasks note row by `missionTasks.userId = subjectUserId` AND accessible mission IDs, so team scope narrows allowed missions but does not widen to other users' mission task notes. Added PGlite regressions cover both another-user team mission note exclusion and `includePersonal:false` excluding subject personal mission notes.
Trust-boundary review: route is POST `/api/federation/v1/list/:resource` under `FederationAuthGuard`; request limit/cursor validation happens before storage access; `FederationScopeService` remains the allow/deny gate with query service as native RBAC evaluator; scope deny returns federation error envelopes and does not query storage; sensitive `credentials`/`api_keys` are denied in M3 native RBAC; rows are capped/paginated with opaque keyset cursor and tagged `_source: local`; no request/response persistence or audit writes (M4-deferred by plan). Reviewer verification on head b38b9846: scoped list tests 11/11 pass; gateway typecheck pass; gateway lint pass; prettier check pass. APPROVE.
Process: this PR is NOT mergeable. Author fixes #1-#6 locally; re-push is HELD — the shared Woodpecker runner is OOM-serialized (every push to an open-PR branch fires 2 pipelines that OOM-kill ci-postgres), so re-push happens only on a clean SOLO run once the orchestrator confirms the queue is idle. Will ping for re-review on the new head.
ORCHESTRATOR — re-review delta + status (head 6e37ab00 -> b38b9846)
Reconciling the independent review (15945, vs 6e37ab00) against the newer head b38b9846 so you can re-review only the delta:
Already changed in b38b9846 (author-reported, addresses original 15940):
- listNotes team branch now constrains `missionTasks.userId = subjectUserId AND missionId IN (accessible mission ids)`.
- +2 regression tests: (a) team-scoped notes exclude another user's missionTasks rows; (b) `includePersonal:false` excludes subject personal mission note.
Still OUTSTANDING from 15945 (author addressing locally now):
- CRITICAL #1 — listNotes `includePersonal` personal clause OR'd with mission scope -> intersect (AND) or drop.
- CRITICAL #2 — listMemory two-table merged keyset pagination non-deterministic -> paginate separately or single capped block.
- IMPORTANT #4 — controller missing-fed-context plain Error -> HttpException envelope.
- IMPORTANT #5 — encodeCursor silent-undefined truncation -> throw.
- IMPORTANT #6 — listAllRows switch missing `default` -> add default throw.
Process: this PR is NOT mergeable. Author fixes #1-#6 locally; re-push is HELD — the shared Woodpecker runner is OOM-serialized (every push to an open-PR branch fires 2 pipelines that OOM-kill ci-postgres), so re-push happens only on a clean SOLO run once the orchestrator confirms the queue is idle. Will ping for re-review on the new head.
REVIEW-OF-RECORD — APPROVE (#682, head e483d976e4). Re-review confirms the prior blockers are resolved.
Trust boundary: listNotes now constrains note rows with missionTasks.userId = subjectUserId AND missionTasks.missionId in authorized missionIds; there is no remaining OR branch that widens team visibility into another user's notes or into missions outside the grant-visible project/mission set. The new PGlite coverage includes both another-user notes and subject notes on an unauthorized mission, and the scoped tests prove those rows are excluded.
Pagination: listMemory now uses deterministic table-block pagination (insights first, preferences second) and encodes the cursor source for memory rows, so page boundaries do not reapply one table's keyset cursor to the other table. The regression with equal timestamps across insights/preferences returns two non-overlapping pages with no duplicate/skip. encodeCursor also now fails closed on non-Date rows instead of silently omitting nextCursor.
Other hardening verified: missing federation context returns a structured 401 unauthorized federation envelope; unsupported resources hit an explicit default throw; request validation still prevents malformed cursor/limit before storage access.
Reviewer verification on head e483d976: scoped gateway list tests 16/16 pass; gateway typecheck pass; gateway lint pass; prettier check pass for changed list/module/scratchpad files. APPROVE.
REVIEW-OF-RECORD — APPROVE (#682, head e483d976e40b8f35eae173f7e342af6d870f6e51). Re-review confirms the prior blockers are resolved.
Trust boundary: listNotes now constrains note rows with `missionTasks.userId = subjectUserId` AND `missionTasks.missionId in authorized missionIds`; there is no remaining OR branch that widens team visibility into another user's notes or into missions outside the grant-visible project/mission set. The new PGlite coverage includes both another-user notes and subject notes on an unauthorized mission, and the scoped tests prove those rows are excluded.
Pagination: listMemory now uses deterministic table-block pagination (insights first, preferences second) and encodes the cursor source for memory rows, so page boundaries do not reapply one table's keyset cursor to the other table. The regression with equal timestamps across insights/preferences returns two non-overlapping pages with no duplicate/skip. encodeCursor also now fails closed on non-Date rows instead of silently omitting nextCursor.
Other hardening verified: missing federation context returns a structured 401 unauthorized federation envelope; unsupported resources hit an explicit default throw; request validation still prevents malformed cursor/limit before storage access.
Reviewer verification on head e483d976: scoped gateway list tests 16/16 pass; gateway typecheck pass; gateway lint pass; prettier check pass for changed list/module/scratchpad files. APPROVE.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Summary
POST /api/federation/v1/list/:resourceFederationAuthGuard→ merged M3-04FederationScopeService→ read-only list query layer_source: "local"tagsDepends on merged M3-04 scope service (#672).
Refs #462
Verification
pnpm typecheck— PASS (41/41)pnpm lint— PASS (23/23)pnpm format:check— PASSpnpm --filter @mosaicstack/gateway test -- list.controller.spec.ts list-query.service.spec.ts— PASS (9 tests)Notes
localhost:5433; scoped FED-M3-05 tests pass and repo gates are green.REVIEW-OF-RECORD — REQUEST CHANGES (#682, head 6e37ab00b99ca76b4bcb2fe0565d016b5051b18b). BLOCKER:
noteslist can leak other users'mission_tasks.notesfor team-scoped missions. InFederationListQueryService.listNotes(), whenfilter.teamIdsis non-empty, the query addsmissionTasks.missionId IN (team mission ids)as an OR branch without also constrainingmissionTasks.userIdtofilter.subjectUserId. The schema explicitly treatsmission_tasksas user-scoped for multi-tenant RBAC isolation, and the normal gateway mission-task path filters by mission AND user. As written, a grant withinclude_teamsfor notes can return every user's notes on accessible team missions, widening beyond the subject user. Please keepmissionTasks.userId = subjectUserIdon all missionTasks note rows (or otherwise prove/document a distinct team-owned notes model) and add regression tests for: (1) team-scoped notes do not include another user's missionTasks rows, and (2)include_personal:falsedoes not return subject personal notes unless an explicitly allowed team-owned note model exists.Reviewed positives: controller route/auth metadata is correct; request
limit/cursorvalidation happens before scope/query; ScopeService deny maps to federation error envelope and does not query storage; row cap/pagination uses limit+1 and opaque keyset cursors;_source: localtagging is applied; sensitivecredentials/api_keysare denied by native RBAC for M3. Reviewer verification: scoped testslist.controller.spec.ts list-query.service.spec.tspass 9/9; gateway typecheck pass; gateway lint pass; prettier check pass. CI may continue running, but merge should wait on the notes isolation fix.6e37ab00b9tob38b9846c1INDEPENDENT REVIEW-OF-RECORD — REQUEST-CHANGES (reviewed by an independent code-review agent dispatched by Mos; this review applies to commit 6e37ab00 — note a newer commit b38b9846 has since been pushed and needs the same scrutiny / may already address some of these).
This PR has BLOCKING issues; CI being green does not make the diff sound. Do not merge until resolved.
CRITICAL (must fix):
listNotes(list-query.service.ts) — whenincludePersonalis true, the personal clauseeq(missionTasks.userId, subjectUserId)is OR'd with the mission-scopedinArray(missionTasks.missionId, missionIds). The OR means "all notes created by the subject ANYWHERE, OR all notes in authorized missions" — a federation peer can enumerate notes the subject created in missions outside the authorized grant scope. The personal clause must be INTERSECTED with the authorized mission set (AND), not OR'd — or removed entirely in favor of project/mission scope only.listMemorypagination is incorrect across pages — two parallel sub-queries each apply the same keyset cursor against separate tables, merged + re-sorted in memory; the emitted cursor cannot be deterministically resumed, so rows are duplicated/skipped at page boundaries (equal timestamps interleave). Either paginate the two resources separately, or return a single deterministic capped block.notestest coverage — the resource with the most complex scoping (and the leak above) has ZERO tests. Add specs proving unauthorized-mission notes are excluded andincludePersonaldoes not over-share.IMPORTANT (fix concurrently):
4.
list.controller.tsmissing-context path throws a plainError→ 500 leaking internal message; use a federation HttpException envelope (FederationUnauthorizedError/InvalidRequest).5.
encodeCursorreturnsundefinedfor non-Date createdAt whilehasMoreis true → silent pagination truncation (peer sees _truncated with no nextCursor, cannot continue). Throw instead.6.
listAllRowsswitch has nodefault→ returnsundefinedfor unknown resource →paginate(undefined)crash. Adddefault: throw.The federation auth guard, scope delegation (per #672), and sensitive-resource (credentials/api_keys) denial are all sound. The blocker is the notes scope leak. Please address #1–#3 (and ideally #4–#6) and re-push; I'll re-review and shepherd on a clean solo green CI run.
REVIEW-OF-RECORD — APPROVE (#682 FED-M3-05 list verb, head b38b9846c170da1fb64eb49feebd6acacd2b2117). Re-review confirms the prior notes isolation blocker is closed:
listNotes()now constrains every mission_tasks note row bymissionTasks.userId = subjectUserIdAND accessible mission IDs, so team scope narrows allowed missions but does not widen to other users' mission task notes. Added PGlite regressions cover both another-user team mission note exclusion andincludePersonal:falseexcluding subject personal mission notes.Trust-boundary review: route is POST
/api/federation/v1/list/:resourceunderFederationAuthGuard; request limit/cursor validation happens before storage access;FederationScopeServiceremains the allow/deny gate with query service as native RBAC evaluator; scope deny returns federation error envelopes and does not query storage; sensitivecredentials/api_keysare denied in M3 native RBAC; rows are capped/paginated with opaque keyset cursor and tagged_source: local; no request/response persistence or audit writes (M4-deferred by plan). Reviewer verification on head b38b9846: scoped list tests 11/11 pass; gateway typecheck pass; gateway lint pass; prettier check pass. APPROVE.ORCHESTRATOR — re-review delta + status (head 6e37ab00 -> b38b9846)
Reconciling the independent review (15945, vs 6e37ab00) against the newer head b38b9846 so you can re-review only the delta:
Already changed in b38b9846 (author-reported, addresses original 15940):
missionTasks.userId = subjectUserId AND missionId IN (accessible mission ids).includePersonal:falseexcludes subject personal mission note.Still OUTSTANDING from 15945 (author addressing locally now):
includePersonalpersonal clause OR'd with mission scope -> intersect (AND) or drop.default-> add default throw.Process: this PR is NOT mergeable. Author fixes #1-#6 locally; re-push is HELD — the shared Woodpecker runner is OOM-serialized (every push to an open-PR branch fires 2 pipelines that OOM-kill ci-postgres), so re-push happens only on a clean SOLO run once the orchestrator confirms the queue is idle. Will ping for re-review on the new head.
b38b9846c1toe483d976e4REVIEW-OF-RECORD — APPROVE (#682, head
e483d976e4). Re-review confirms the prior blockers are resolved.Trust boundary: listNotes now constrains note rows with
missionTasks.userId = subjectUserIdANDmissionTasks.missionId in authorized missionIds; there is no remaining OR branch that widens team visibility into another user's notes or into missions outside the grant-visible project/mission set. The new PGlite coverage includes both another-user notes and subject notes on an unauthorized mission, and the scoped tests prove those rows are excluded.Pagination: listMemory now uses deterministic table-block pagination (insights first, preferences second) and encodes the cursor source for memory rows, so page boundaries do not reapply one table's keyset cursor to the other table. The regression with equal timestamps across insights/preferences returns two non-overlapping pages with no duplicate/skip. encodeCursor also now fails closed on non-Date rows instead of silently omitting nextCursor.
Other hardening verified: missing federation context returns a structured 401 unauthorized federation envelope; unsupported resources hit an explicit default throw; request validation still prevents malformed cursor/limit before storage access.
Reviewer verification on head
e483d976: scoped gateway list tests 16/16 pass; gateway typecheck pass; gateway lint pass; prettier check pass for changed list/module/scratchpad files. APPROVE.