Add RBAC authorization roles

[cass-clearly]

What changed

Added persisted RBAC roles (viewer, commenter, resolver, admin), server-side permission enforcement, ownership checks for comment edits, role management endpoints, and UI permission gating.

Why

Closes #276. Block Security requires authorization boundaries for read/create/edit/resolve/delete/admin actions.

New/Changed Endpoints

Method	Endpoint	Description
GET	/me/permissions	Returns current actor role and permissions for UI gating.
PUT	/users/:id/role	Admin-only persisted role assignment.

How to verify

• npm run check
• Use X-Remarq-User with users assigned different roles and verify unauthorized actions return 403.

Manual testing checklist

• Server starts without errors (npm run start)
• Existing tests pass (npm test)
• Tested in browser (annotations, sidebar, highlights work)
• Tested API changes with curl (include example commands above)
• No console errors in browser DevTools

[cass-clearly]

The Marketing Guru — Round 1 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_marketing-guru_r1 npm test

I do not recommend a totally different implementation path from a marketing/docs lens. I do recommend blocking this until the public-facing docs stop making readers guess.

Required fixes:

1. Add the RBAC change to CHANGELOG.md under [Unreleased].

   • This is required for every PR. A security/RBAC feature is release-note material. Omitting it violates the changelog requirement and hides a major enterprise-facing capability.

2. Make docs/api.md a real API reference for the new endpoints.

   • The new GET /me/permissions and PUT /users/:id/role sections are one-line stubs. The file promises request/response schemas, status codes, error codes, and curl examples. Add concrete examples for both endpoints, response bodies, 403 behavior, valid roles, and the identity/role headers involved.

3. Update the OpenAPI spec or stop claiming the API docs/spec are complete.

   • README.md says every endpoint appears through /openapi.json and the API reference is complete. These two new public endpoints are implemented but absent from server/openapi.js. That breaks agent/tooling expectations and makes the docs untrustworthy.

4. Rewrite docs/rbac.md around the actual threat model.

   • X-Remarq-User is a trusted identity header, not authentication. The doc must say it must only be accepted behind a trusted auth proxy. Right now it reads like clients can self-identify by sending a header, which is dangerous for a security feature.
   • Remove or qualify the Okta language unless Okta integration exists in this PR. Public docs should not imply shipped support that is not in the code.
   • Explain the no-header behavior explicitly: unauthenticated local requests resolve as admin. That is a major caveat, not a footnote.

5. Fix the verification section so it is copy-pasteable and proves the feature.

   • The current POST example has no body and does not show role assignment. Add a minimal sequence: check viewer permissions, attempt forbidden create, admin assigns role, retry create, commenter forbidden from deleting or editing someone else's comment.

This PR changes access control. Slow is smooth: security docs have to be more precise than normal feature docs, not less.

[cass-clearly]

The Craftsperson — Round 1 Review

Verdict: REQUEST_CHANGES

Test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_craftsperson_r1 npm test.

I read gh pr diff 285, git diff origin/main...HEAD, and the changed files.

I do not recommend a totally different path. Persisted roles plus route-level authorization is a workable shape. The implementation is not reviewable as shipped because the behavior is not tested.

Required fixes:

1. Add integration tests for the RBAC behavior, not just the permission map. server/test.mjs:107-122 only proves permissionsForRole() returns arrays. It does not prove any protected endpoint is protected. Add tests for:

   • GET /me/permissions for anonymous admin-compatible mode, unknown header user as viewer, and persisted role lookup.
   • PUT /users/:id/role allowed for admin and rejected for non-admin.
   • GET /comments allowed for viewer and create/update/delete rejected where appropriate.
   • POST /comments rejected for viewer and allowed for commenter/resolver/admin.
   • PATCH /comments/:id edit-own allowed, edit-other rejected, resolve requiring resolver/admin.
   • DELETE /comments/:id requiring admin.

2. Add tests around the new ownership rule in server/index.js:447-453. This is the riskiest part of the feature and currently only the old anonymous-admin tests exercise it. Those tests pass because anonymous requests become admin, so they provide no confidence that real authenticated users get the right 403/200 behavior.

3. Add client-side permission tests for the changed UI gates. feedback-layer/src/index.js:134-270 and feedback-layer/src/sidebar.js:58-67,568-573 introduce create/edit/resolve/delete hiding with no test coverage. At minimum test that viewer permissions hide mutation controls, commenter permissions show reply/create and only own edit, resolver shows resolve, admin shows delete/edit-any. Also test the fallback behavior deliberately.

4. Fix or explicitly test the inconsistent fallback. fetchPermissions().catch(() => ({ permissions: [] })) passes an empty list into the sidebar, where empty permissions mean legacy allow-edit/reply, but onSelectionChange() requires comments:create, so failed permission fetch disables new comment creation while leaving some mutation UI in legacy mode. Either make fallback consistently read-only or consistently legacy-compatible, and cover it with a test.

This skipped the refactor/test step. A security boundary cannot ship with only pure helper tests while the actual Express middleware, route composition, DB role persistence, and UI gating are effectively untested.

[cass-clearly]

The Minimalist — Round 1 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_minimalist_r1 npm test.

I do not recommend a totally different path. A small authz.js plus direct route guards is the right shape. The implementation still adds avoidable flexibility and leaves the ownership model unclear.

Required changes:

1. Delete the X-Remarq-Role fallback in resolveUser(). Persisted roles are the requirement. A client-supplied role header is an extra control plane and makes bypass trivial for any unpersisted user. If there is no persisted row for X-Remarq-User, default to viewer.

2. Stop treating invalid role assignment as viewer. PUT /users/:id/role should reject unknown roles with 400. Silent downgrading is surprising API behavior and makes bad admin calls look successful.

3. Make ownership one source of truth. The API currently allows X-Remarq-User: alice to create a comment with author: bob, while edit ownership compares the stored author to req.user.id. Either derive author from the authenticated user when a user header exists, or reject mismatches. Do not make callers remember to keep two identity fields aligned.

4. Add focused API tests for the actual behavior, not just the permission map: viewer can read but not create, commenter can edit own but not other/delete/resolve, resolver can resolve, admin can assign roles, invalid role returns 400. Keep it small; this should not become a policy framework test matrix.

The skipped plan review shows here: the extra role header and split identity model are complexity that should have been questioned before implementation.

[cass-clearly]

The Steward — Round 1 Review

Verdict: REQUEST_CHANGES

Test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_steward_r1 npm test.

I read gh pr diff 285, git diff origin/main...HEAD, and the changed files.

I do not recommend a totally different path for the feature. Persisted RBAC plus server-side enforcement is the right shape. I do recommend a different path for the trust boundary: do not accept roles from client-controlled request headers. Identity must come from a trusted auth/proxy boundary; roles must come from persisted server state or an explicit dev-only bypass.

Blocking issues:

1. X-Remarq-Role is a privilege-escalation hole. server/authz.js:47 lets any request with X-Remarq-User and no persisted row set X-Remarq-Role: admin, then pass requirePermission("users:manage") on PUT /users/:id/role. Remove role headers from authorization, or gate them behind an explicit trusted/dev config that cannot be enabled accidentally.

2. RBAC is bypassable through existing write endpoints. DELETE /documents/:id still has no permission check and cascades comment deletion (server/index.js:265-272), so a viewer can delete comments by deleting the document. Reaction create/delete are also unauthz writes (server/index.js:515-547), and the UI still always renders the reaction add button (feedback-layer/src/sidebar.js:654-664). Apply the same server-side permissions to every endpoint that mutates protected resources; UI gating is not a control.

3. PUT /users/:id/role silently turns invalid input into viewer (server/authz.js:16-17, 58-65). That makes typos destructive and makes the API hard to operate. Invalid or missing role must return 400 with a clear error. Normalization-to-viewer is acceptable for reading stored legacy data, not for admin writes.

4. The OpenAPI contract is stale. The PR adds GET /me/permissions and PUT /users/:id/role, but server/openapi.js has neither path. This project says the API spec drives consumers/CLI; new endpoints must be in the spec with request/response schemas and 403/400 errors.

5. Browser fallback is inconsistent and breaks consumer expectations. If /me/permissions fails, sidebar.js treats empty permissions as legacy allow for edit/reply, but index.js:270 hides the selection tooltip unless comments:create is present. A transient permissions failure or older server makes comment creation disappear client-side. Pick a coherent compatibility policy and implement it consistently.

Add API-level tests for these cases before re-review: spoofed X-Remarq-Role cannot self-promote, viewer cannot delete documents/comments or mutate reactions, invalid role assignment returns 400, and the new endpoints are present in /openapi.json.

[cass-clearly]

The Architect — Round 1 Review

Verdict: REQUEST_CHANGES

Tests passed with the exact required command: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_architect_r1 npm test.

This implementation is not coherent enough to ship. It adds an RBAC-shaped layer, but the security boundary is porous and the ownership model is wrong. I do recommend a different path for the identity/ownership seam: do not use the user-editable comment author string as the authorization principal. Use a server-derived actor identity (actor_id or equivalent), or in the minimal version make authenticated create/update paths consistently use req.user.id. The UI can still display a name, but display name and auth principal cannot be the same mutable field.

Required fixes:

1. Remove the client-controlled role bypass. server/authz.js trusts X-Remarq-Role for any known user without a persisted row. A caller can send X-Remarq-User: bob plus X-Remarq-Role: admin and become admin. Roles must come from persisted state or a trusted deployment integration that is explicitly configured and protected. Default unknown authenticated users to viewer.

2. Close the document-delete bypass. DELETE /documents/:id is still unguarded and cascades comment deletion. A viewer can delete all comments for a document without comments:delete. Protect document mutating endpoints, or remove/disable destructive document operations for non-admins.

3. Fix ownership. POST /comments persists author from request body, while PATCH /comments/:id checks rows[0].author !== req.user.id. The UI still writes author from localStorage. Under Okta/trusted-header auth, users will not reliably be able to edit their own comments, and request-body author spoofing pollutes the ownership model. Make the server own the authorization identity and test it.

4. Add real RBAC API tests. Current tests only exercise pure helper functions. They do not prove unauthorized requests are rejected. Add integration tests for viewer/commenter/resolver/admin behavior, own-vs-other edit, delete denial, role management, invalid roles, and the document-delete bypass.

5. Invalid role assignment must be a 400. setUserRole() silently normalizes unknown roles to viewer. A typo in an admin call should not demote a user silently.

6. Update the OpenAPI spec. The README says /openapi.json is the API reference, but the new /me/permissions and /users/:id/role endpoints and 403 responses are not in server/openapi.js.

This should have gone through plan review. The missed plan review shows up in the final shape: RBAC, identity, API docs, and tests are not aligned.

[cass-clearly]

The Minimalist — Round 2 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_minimalist_r2 npm test.

I read the current PR diff and changed files. I still do not recommend a totally different path: authz.js plus direct route guards is the smallest viable shape.

One material issue remains:

1. Ownership is still split in the browser. The server now stores req.user.id as author when X-Remarq-User is present (good), but the sidebar decides whether to show the edit button by comparing comment.author to getCommenter() from localStorage (feedback-layer/src/sidebar.js:563, feedback-layer/src/utils/permissions.js:9-14). In the actual RBAC path, /me/permissions returns the actor id, then feedback-layer/src/index.js:134-139 discards it. A commenter can create a comment as alice@example.com server-side and then not see their own edit control unless they also typed exactly alice@example.com into the name field. That keeps two identity fields aligned by convention, not by design.

Fix this without adding a framework: keep the actor id returned by /me/permissions, pass it to the sidebar, and use it for own-edit checks when permissions are present. Fall back to getCommenter() only in legacy/null-permissions mode. Add the small permission/UI utility test for that behavior.

[cass-clearly]

The Craftsperson — Round 2 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_craftsperson_r2 npm test.

I read the current PR diff, changed files, and Round 1 feedback.

I do not recommend a totally different path. The Round 2 changes moved this from helper-only confidence to behavior confidence: there are now API-level RBAC tests for default viewer denial, role-header spoofing, invalid role rejection, own-vs-other edit, resolver-only status changes, and document-delete denial. The author/ownership seam is now server-owned when an actor header is present, and the client fallback was refactored into explicit null legacy-compatible permission semantics.

From the Craftsperson lens, the important refactor was extracting browser permission decisions into feedback-layer/src/utils/permissions.js and testing those decisions directly. That is the right testing level for this codebase's current client test harness: behavior rules are covered without coupling tests to sidebar DOM structure.

Non-blocking cleanup for the next pass: split the broad RBAC integration test into smaller role-focused it cases if it grows again, and add DOM-level sidebar coverage when the client harness supports it. Not required for this PR.

[cass-clearly]

The Marketing Guru — Round 2 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_marketing-guru_r2 npm test.

I read the current PR diff and changed files. I do not recommend a totally different implementation path from a marketing/docs lens. The changelog entry and OpenAPI coverage are now present. Two public-doc issues remain material for an access-control PR:

1. docs/api.md still has stubs for the new public endpoints. The README sends readers there for request/response schemas, error codes, and curl examples. GET /me/permissions and PUT /users/:id/role currently get three sentences total. Add the same level of reference as other endpoints: headers/trusted identity behavior, request body for role assignment, success response body, 400 invalid role, 403 forbidden, valid roles, and copy-paste curl examples.

2. docs/rbac.md verification is still not copy-pasteable and does not prove the feature. The POST example has no JSON body, no role assignment step, and no concrete localhost flow. Add a minimal sequence that shows: unknown X-Remarq-User is viewer, create is forbidden, local/admin assigns commenter, create succeeds with server-owned author, and commenter cannot delete/resolve/admin-manage. Also make the trust boundary explicit: X-Remarq-User is trusted only when set by a trusted auth proxy; deployments must strip/ignore client-supplied identity headers before they reach Remarq.

Security docs are part of the security boundary. Readers should not have to infer how to use or verify RBAC from code.

[cass-clearly]

The Steward — Round 2 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_steward_r2 npm test.

I read the current PR diff, git diff --name-only origin/main...HEAD, and the changed files. I do not recommend a totally different implementation path now; persisted roles plus route-level enforcement is still the right shape, and Round 2 closed the role-header spoofing, invalid-role, author-spoofing, document-delete, reaction-write, and new-endpoint spec gaps.

Two material issues remain from the Steward lens:

1. The OpenAPI contract is still incomplete for the RBAC behavior added to existing endpoints. server/openapi.js documents DELETE /documents/{id}, POST /comments, PATCH /comments/{id}, DELETE /comments/{id}, POST /comments/{id}/reactions, and DELETE /comments/{id}/reactions/{emoji} without the 403 responses these routes can now return. /openapi.json is the machine contract for consumers and CLI/tooling; if authenticated viewers/non-admins can receive 403, the spec must say so with the existing error schema.

2. The browser ownership gate uses the local display name instead of the server actor identity. /me/permissions returns id, but feedback-layer/src/index.js discards it and feedback-layer/src/sidebar.js checks canEditComment(..., getCommenter()). In a trusted-header deployment the server stores author as req.user.id; a user who types Alice while the proxy identity is alice@example.com will not see edit-own controls for their own comments. Use the actor id from /me/permissions for permission ownership, falling back to the local name only in legacy permissions === null mode, and cover it with a focused client test.

Non-blocking but should be fixed while touching the UI: the add-reaction button is still rendered for viewers even though the server now rejects reaction writes. Hiding unavailable actions keeps the right path obvious and avoids turning expected authorization into user-visible errors.

[cass-clearly]

The Architect — Round 2 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_architect_r2 npm test.

I read the current PR diff and changed files. A totally different path is no longer warranted: the Round 2 fixes moved the implementation into the right shape. X-Remarq-Role is gone, unknown authenticated users default to viewer, invalid role writes return 400, document deletion is guarded, and authenticated comment creation now uses the server-derived actor as the stored author.

One material coherence issue remains:

1. The UI still uses the local display-name field as the authorization principal for edit-own. The server now correctly stores req.user.id as comment.author when X-Remarq-User/X-Forwarded-User is present, but feedback-layer/src/sidebar.js checks canEditComment(_permissions, ann, getCommenter()). In an authenticated deployment, the proxy identity may be alice@example.com while the sidebar name field is Alice. The server would allow Alice to edit her own comment; the UI hides the edit action. That is the same identity/display-name seam from Round 1, just moved client-side.

Fix this by carrying the actor id returned from GET /me/permissions into the sidebar permission checks and using it for own-edit gating, with getCommenter() only as the legacy unauthenticated fallback/display name. Add a focused client test for authenticated commenter own-edit with actor id different from the local name.

After that, ship it. The remaining gaps are follow-up material, not reasons to redesign the PR.

[cass-clearly]

The Marketing Guru — Round 3 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_marketing-guru_r3 npm test

I read the current PR diff and changed files. A totally different implementation path is not warranted from the docs/marketing lens. The public docs are still not safe enough for an access-control feature.

Blocking issues:

1. docs/rbac.md still has the same non-copy-paste verification stub. It uses https://remarq.internal, the POST has no JSON body, and it never shows role assignment or a successful retry. Replace it with a minimal localhost flow that proves the feature: unknown X-Remarq-User is viewer, create returns 403, local/admin assigns commenter, create succeeds with server-owned author, and commenter cannot delete/resolve/admin-manage.

2. The trusted-header boundary is still too easy to misunderstand. Say explicitly that X-Remarq-User / X-Forwarded-User must be set only by a trusted auth proxy and that deployments must strip or ignore client-supplied identity headers before requests reach Remarq. Do not let the RBAC doc read like browsers or API clients can self-identify safely.

3. docs/api.md examples are not fully self-consistent. The GET /me/permissions example shows alice@example.com as a commenter without first assigning that role, so a copied local call will return viewer. The PUT /users/:id/role section also needs the actual success response body to match the README promise that the API reference includes response schemas.

Security docs are part of the security boundary. The right path has to be obvious and copy-pasteable.

[cass-clearly]

The Craftsperson — Round 3 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_craftsperson_r3 npm test.

I read the current PR diff and changed files for Round 3.

I still do not recommend a totally different path. The implementation is now coherent enough from the Craftsperson lens: the authorization rules live in small named helpers, the server-side behavior has integration coverage, and the Round 3 actor-id change removes the last identity/display-name coupling in the sidebar without adding a larger framework.

The new currentUser plumbing in feedback-layer/src/index.js → feedback-layer/src/sidebar.js is the right small refactor: edit-own decisions now use the server actor identity when present and preserve the legacy display-name fallback only where compatibility needs it.

Non-blocking cleanup for a future PR: reset the new users table in the API test beforeEach, and add DOM-level sidebar permission tests once the client harness supports them. Those are maintainability improvements, not material blockers for this round.

[cass-clearly]

The Steward — Round 3 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_steward_r3 npm test.

I read the current PR diff and changed files. A totally different path is not warranted; persisted roles plus route-level enforcement is the right shape, and the Round 3 actor-id pass-through closes the browser ownership issue from my last review.

One material API-contract issue remains:

1. /openapi.json still does not describe the 403 responses now returned by existing protected write endpoints. The server can return 403 for DELETE /documents/:id (server/index.js:269-272), POST /comments (server/index.js:355-358), PATCH /comments/:id (server/index.js:452-458), DELETE /comments/:id (server/index.js:504-507), and reaction create/delete (server/index.js:520-523, server/index.js:551-553). The corresponding OpenAPI responses only list 200/201/400/404 (server/openapi.js:283-286, 379-383, 455-459, 473-476, 515-518, 554-557). This violates API stability: /openapi.json is the machine contract for consumers and CLI/tooling, and generated clients should not discover authorization failures only at runtime.

Fix: add 403 responses with the existing error schema for every endpoint that can now return Forbidden, and add an OpenAPI test that asserts the protected operations include 403 so this contract does not regress.

[cass-clearly]

The Minimalist — Round 3 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_minimalist_r3 npm test.

I read the current PR diff and changed files. A totally different path is not warranted: a small authz.js plus direct route guards is still the simplest viable RBAC shape.

One material issue remains:

1. The Round 2 ownership fix was applied to source, but not to the shipped browser file. server/index.js serves server/serve/feedback-layer.js, and server/package.json packages serve/. That minified file still contains no currentUser path and still checks edit-own with the local commenter name (SO(qn,e,Ne())). So the actual script users load still has the split identity bug: authenticated actor id from /me/permissions is discarded for edit-own UI gating.

Do the small thing: rebuild/copy the client artifact (npm run build) and commit the updated server/serve/feedback-layer.js so the served script matches feedback-layer/src/index.js and feedback-layer/src/sidebar.js. No new abstraction, no redesign.

[cass-clearly]

The Architect — Round 3 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_architect_r3 npm test.

A totally different path is still not warranted. The core shape is now coherent: persisted roles, no client role header, server-owned author for authenticated requests, guarded comment/document destructive paths, and the browser now carries the actor id into edit-own checks.

One material system-level gap remains: webhook management is outside the RBAC boundary. registerWebhookRoutes() still exposes GET/POST/PATCH/DELETE /webhooks to any authenticated actor, including a default viewer. That means a viewer can create outbound subscriptions, alter or delete integrations, and route comment events to arbitrary URLs. For an access-control PR, that is not a follow-up polish item; it leaves a privileged control plane unprotected while the docs present role-based enforcement for internal deployments.

Required fix:

1. Put webhook management behind an admin-only permission check, with focused tests proving an authenticated viewer cannot create/update/delete webhooks and anonymous local/admin-compatible mode still behaves as existing tests expect. Keep it simple; do not build a policy framework. Reuse the existing RBAC shape and update the OpenAPI/docs only as needed for any new 403 behavior.

After that, ship it. The identity/ownership concern from Round 2 is resolved.

[cass-clearly]

The Marketing Guru — Round 4 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_marketing-guru_r4 npm test.

I fetched/read the current PR diff and changed files. A totally different implementation path is not warranted from the docs/marketing lens; the RBAC docs are now much clearer and the verification flow is copy-pasteable.

One material docs issue remains:

1. PUT /users/:id/role still does not document its success response in either docs/api.md or server/openapi.js.
   • The server returns { "object": "user", "id": "...", "role": "..." }.
   • README.md tells readers that docs/api.md has request/response schemas and that /openapi.json is the machine-readable API contract. A new public role-management endpoint with an undocumented 200 body breaks that promise and makes agent/tooling output less trustworthy.
   • Fix: add the 200 OK response body example/schema to docs/api.md, and add matching content: jsonContent(...) for the 200 response in server/openapi.js.

While touching docs/api.md, make the GET /me/permissions example self-consistent: either show the preceding role assignment or say the example assumes alice@example.com already has the commenter role. As written, a copied local request for a new user returns viewer, not the shown commenter response.

[cass-clearly]

The Minimalist — Round 4 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_minimalist_r4 npm test.

I read the current PR diff and changed files. A totally different path is not warranted. The shape is still small enough: one authz.js, direct route guards, and simple browser permission helpers. The earlier avoidable complexity is gone: no client role header, invalid role writes fail, server-owned actor identity is the ownership source, document/webhook destructive paths are guarded, and the shipped server/serve/feedback-layer.js now includes the currentUser path instead of the stale local-name edit check.

From the Minimalist lens, this is now the simplest viable RBAC implementation for the stated requirement. Ship it.

[cass-clearly]

The Craftsperson — Round 4 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_craftsperson_r4 npm test.

I read the current PR diff, changed files, and prior Round 4-relevant changes from this worktree.

A totally different path is not warranted. The implementation remains in the right small shape: authorization rules are isolated in named helpers, route enforcement is direct, and behavior is covered with API-level tests instead of only permission-map tests.

The Round 4 changes address the material implementation/test concerns from the current loop: the served browser artifact now carries the actor id through to edit-own checks, and webhook management is guarded at the route prefix with a negative authenticated-user test while the existing anonymous/admin-compatible webhook tests continue to cover the happy paths.

Non-blocking cleanup if this RBAC test section grows again: split the broad integration case into role-focused subtests and reset the users table in beforeEach for stronger isolation. Not a blocker for this PR.

[cass-clearly]

The Steward — Round 4 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_steward_r4 npm test.

I read the current PR diff and changed files. A totally different implementation path is not warranted; persisted roles with direct route guards is still the right shape, and the webhook control-plane gap from Round 3 is now closed server-side.

One material API-contract issue still blocks shipping:

1. /openapi.json still does not describe the 403 Forbidden responses this PR adds to existing endpoints. The server can now return 403 for DELETE /documents/:id, POST /comments, PATCH /comments/:id, DELETE /comments/:id, reaction create/delete, and all /webhooks routes (server/index.js:269-584). The corresponding OpenAPI operations still list only success/400/404 responses (server/openapi.js:283-285, 379-382, 455-458, 473-475, 515-518, 554-557), and webhook routes are not represented in the spec at all despite this PR changing their authorization behavior.

Fix: add 403 responses with the existing error schema for every protected operation exposed in /openapi.json, cover that with an OpenAPI regression test, and either add webhook routes to the spec with their new admin-only 403 behavior or explicitly align the API contract strategy for webhooks. This is not docs polish: the README says /openapi.json is the machine contract for consumers/CLI, and generated clients should not discover authorization failures only at runtime.

[cass-clearly]

The Architect — Round 4 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_architect_r4 npm test.

I fetched/read the current PR diff and changed files. A totally different path is not warranted. The implementation shape now holds together: persisted roles, trusted-header identity, no client role override, server-owned author under authenticated requests, guarded comment/document writes, actor-id UI gating, rebuilt served asset, and webhook management now sits behind admin permission.

One material contract gap remains:

1. /openapi.json still does not describe the 403 responses this PR adds to protected existing endpoints. The server can now return 403 for DELETE /documents/:id, POST /comments, PATCH /comments/:id, DELETE /comments/:id, POST /comments/:id/reactions, and DELETE /comments/:id/reactions/:emoji, but server/openapi.js still lists only the old success/400/404 responses for those operations. This is not polish; /openapi.json is the machine contract for CLI/tooling and API consumers. Generated clients should not discover RBAC failures only at runtime.

Required fix: add 403 responses with the existing error schema to every operation that is now permission-gated, and add a focused OpenAPI test asserting those protected operations include 403. While touching docs, make sure the webhook API reference states webhook management is admin-only/403, since this round changed that behavior.

After that, ship it. Do not redesign the RBAC path.

[cass-clearly]

The Craftsperson — Round 5 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_craftsperson_r5 npm test.

I fetched/read the current PR diff and changed files, including the Round 5 OpenAPI/docs/test follow-up.

A totally different path is not warranted. The implementation is still the right small shape: authorization rules are isolated in authz.js, route-level checks are direct, browser permission decisions are factored into a testable utility, and the served client artifact now matches the source path for actor-owned edit checks.

From the Craftsperson lens, the Round 5 changes are appropriately covered: the OpenAPI contract now has a regression test for protected operations' 403 responses, and the role-management response schema is asserted. The remaining nits I see are cleanup-level only: split the broad RBAC integration case if it grows again, and reset users in the integration-test beforeEach for stronger isolation. Neither is material enough to block this PR.

[cass-clearly]

The Minimalist — Round 5 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_minimalist_r5 npm test.

I fetched/read the current PR diff and changed files. A totally different path is not warranted. The implementation is still the small viable shape: one authz.js, direct route guards, persisted roles, server-owned actor identity for ownership, and minimal browser permission helpers.

The Round 5 contract/doc additions do not turn this into a framework or add speculative flexibility. They close the API-contract gap without changing the core design. From the Minimalist lens, this is enough RBAC for the stated requirement. Ship it.

[cass-clearly]

The Steward — Round 5 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_steward_r5 npm test.

I fetched/read the current PR diff and changed files from /home/cass/remarq-276-rbac.

A totally different path is not warranted. The implementation now holds the operational contract together: roles are persisted server-side, client-controlled role escalation is gone, authenticated author ownership is server-derived, destructive comment/document/reaction/webhook paths are permission-gated, and /openapi.json now documents the new 403 responses for protected operations including webhooks.

From the Steward lens, the prior blocking API-contract issue is resolved. The remaining trusted-header deployment caveat is documented in docs/rbac.md; production deployments still need the auth proxy to set/strip identity headers correctly, but that is an operational deployment requirement rather than a blocker in this PR.

[cass-clearly]

The Marketing Guru — Round 5 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_marketing-guru_r5 npm test.

I fetched/read the current PR diff and changed files. A totally different implementation path is not warranted; the RBAC shape and the OpenAPI contract are now aligned enough to ship once the human API reference stops contradicting the runtime.

One material docs issue remains:

1. docs/api.md still omits the new 403 Forbidden behavior from protected existing endpoints.
   • This PR now returns 403 for DELETE /documents/:id, POST /comments, PATCH /comments/:id, DELETE /comments/:id, and reaction create/delete.
   • The corresponding sections in docs/api.md still list only the old success/400/404 status codes, and the common status-code list also omits 403.
   • README.md promises that docs/api.md contains request/response schemas, error codes, and curl examples. Leaving the human reference stale makes agents and developers discover authorization failures only after copy-pasting calls.

Fix: add 403 Forbidden to the status-code lists for every protected endpoint documented in docs/api.md, with one sentence tying it to insufficient RBAC permissions. While touching the users section, make the GET /me/permissions example self-consistent by saying it assumes alice@example.com already has the commenter role, or show the preceding role-assignment call.

[cass-clearly]

The Architect — Round 5 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_architect_r5 npm test.

I fetched/read the current PR diff and changed files.

A totally different path is not warranted. The implementation now holds together: persisted roles, trusted-header identity, server-owned authorship for authenticated comment/reaction writes, direct route guards, UI permission gates using the actor id for ownership, rebuilt served asset, admin-only webhook management, and docs that name the trusted-header boundary.

The Round 4 material contract gap is closed. /openapi.json now documents 403 for the protected comment/document/reaction operations, includes webhook routes with admin-only 403 behavior, documents the PUT /users/:id/role 200 body, and has a regression test asserting the protected operations include 403.

Remaining nits are follow-up material, not blockers. Ship it.

[cass-clearly]

The Steward — Round 6 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_steward_r6 npm test.

I fetched/read the current PR diff and changed files from /home/cass/remarq-276-rbac.

A totally different path is not warranted. The current shape remains operationally sound: roles are persisted server-side, client-supplied role escalation is ignored, authenticated authorship is server-derived for ownership checks, protected comment/document/reaction/webhook operations return 403, and /openapi.json documents those authorization failures for generated clients/tooling.

The Round 6 docs-only change does not weaken the API contract. From the Steward lens, the remaining trusted-header requirement is correctly called out as a deployment boundary: production must put Remarq behind a proxy that sets and strips identity headers. That is an operational requirement, not a blocker for this PR.

[cass-clearly]

The Minimalist — Round 6 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_minimalist_r6 npm test.

I fetched/read the current PR diff and changed files. A totally different path is not warranted. The design remains the small viable shape for #276: one authz.js, direct route guards, persisted roles, server-owned actor identity for ownership, and minimal browser permission helpers.

The Round 6 change is docs-only and just makes existing 403 behavior explicit. No added abstraction, no speculative flexibility, no new moving parts. From the Minimalist lens, this is enough RBAC for the requirement. Ship it.

[cass-clearly]

The Marketing Guru — Round 6 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_marketing-guru_r6 npm test.

I fetched/read the current PR diff and changed files. A totally different implementation path is not warranted; the RBAC shape, changelog, OpenAPI contract, and dedicated RBAC doc are close. The remaining issues are in the human API reference, and they still matter because this is access-control documentation.

Blocking fixes:

1. docs/api.md still omits 403 Forbidden from Common Status Codes.

   • The protected endpoint sections now mention 403, but the summary list still jumps from 400 to 404.
   • Add a common entry such as: 403 Forbidden — Current actor does not have the required RBAC permission.
   • This is the section humans and agents scan when diagnosing authorization failures.

2. docs/api.md still has a non-copy-pasteable GET /me/permissions example.

   • The response shows alice@example.com as commenter, but the example request does not assign that role first. A copied local request for a new user returns viewer.
   • Either say the example assumes alice@example.com already has the commenter role, or show the preceding role-assignment call.

Principles: right thing should be the easy thing; slow is smooth. Security docs need to make the expected RBAC behavior obvious without readers inferring setup steps from code.

[cass-clearly]

The Craftsperson — Round 6 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_craftsperson_r6 npm test.

I fetched/read the current PR diff, changed-file list, and the changed source/test/docs files from /home/cass/remarq-276-rbac.

A totally different path is not warranted. The implementation remains in the right small shape: RBAC decisions are isolated in server/authz.js, route guards are direct, ownership is server-derived for authenticated requests, browser permission decisions are factored into a small tested utility, and the served browser artifact carries the same actor-id edit-own path as the source.

From the Craftsperson lens, Round 6 only adds the remaining human API-reference status-code documentation. It does not disturb the tested implementation. The behavior-sensitive pieces still have coverage at the useful levels: API integration tests for RBAC enforcement and OpenAPI contract regression, plus focused client utility tests for permission decisions.

Remaining cleanup stays non-blocking: split the broad RBAC integration test if it grows again, and reset the users table in API test setup for stronger isolation. Neither is material enough to block this PR.

[cass-clearly]

The Architect — Round 6 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_architect_r6 npm test.

I fetched/read the current PR diff and changed files from /home/cass/remarq-276-rbac.

A totally different path is not warranted. The implementation is now coherent enough to ship: persisted server-side roles, trusted-header identity, no client-controlled role escalation, server-owned authorship for authenticated writes, direct route guards on protected comment/document/reaction/webhook mutations, actor-id UI ownership checks, rebuilt served asset, OpenAPI 403 coverage, and API docs updated for the protected document/comment paths.

The remaining imperfections are follow-up material, not round-6 blockers. docs/api.md could still be tightened around the common 403 status listing and a fuller reactions reference, and the viewer reaction UI can be made less error-prone later. Those do not change the security boundary or justify another loop.

Ship it.

[cass-clearly]

The Minimalist — Round 7 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_minimalist_r7 npm test.

I fetched/read the current PR diff and changed files from /home/cass/remarq-276-rbac.

A totally different path is still not warranted. The implementation remains the smallest viable RBAC shape for this codebase: one authz.js, direct route guards, persisted roles, server-owned actor identity for ownership, and small browser permission helpers. The Round 7 docs change only fixes example clarity; it does not add another layer, option, or speculative mechanism.

From the Minimalist lens: enough. Ship it.

[cass-clearly]

The Craftsperson — Round 7 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_craftsperson_r7 npm test.

I fetched/read the current PR diff, changed-file list, prior Craftsperson feedback, and the changed source/test/docs files from /home/cass/remarq-276-rbac.

A totally different path is still not warranted. The implementation keeps the right small shape: RBAC rules live in server/authz.js, enforcement is direct at the route boundary, authenticated ownership uses the server-derived actor id, and browser permission decisions are isolated in feedback-layer/src/utils/permissions.js rather than spread through DOM code.

From the Craftsperson lens, the behavior-sensitive pieces remain covered at useful levels: API integration tests exercise viewer denial, header spoofing denial, invalid roles, own-vs-other edit, resolver-only status changes, delete/webhook denial, plus OpenAPI 403 regression checks; the client permission utility has focused tests for legacy fallback and role-gated actions. Round 7 only clarifies docs examples and does not disturb the implementation.

Non-blocking cleanup remains the same: if the RBAC integration case grows, split it into smaller role-focused tests; reset users in beforeEach for stronger future isolation. Neither is material enough to block this PR.

[cass-clearly]

The Architect — Round 7 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_architect_r7 npm test.

I fetched/read the current PR diff and changed files from /home/cass/remarq-276-rbac. I also checked PR CI; current checks are green.

A totally different path is not warranted. The implementation remains coherent: persisted server-side roles, trusted-header actor identity, no client-controlled role escalation, server-owned authorship for authenticated writes, direct route guards, browser permission gates based on /me/permissions, OpenAPI coverage for protected operations, and docs that now explain the viewer default and role-assignment flow without contradicting runtime behavior.

Round 7 is a docs clarity fix. It reduces operational ambiguity; it does not add scope or introduce a new pattern. We are past material architecture risk and into review fatigue territory. Under Principles 7, 10, and 11: ship it.

[cass-clearly]

The Steward — Round 7 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_steward_r7 npm test.

I fetched/read the current PR diff and changed files from /home/cass/remarq-276-rbac.

A totally different path is not warranted. The current implementation remains operationally coherent: roles are persisted server-side, client-supplied role escalation is ignored, authenticated authorship is server-derived for ownership checks, protected comment/document/reaction/webhook operations return 403, and /openapi.json plus the human API docs now describe those authorization failures.

The Round 7 docs fixes close the remaining consumer-experience gap: GET /me/permissions is copy-pasteable/self-consistent, PUT /users/:id/role documents its response, and common status codes include the RBAC 403. From the Steward lens, the remaining trusted-header requirement is a documented deployment boundary, not a blocker for this PR.

[cass-clearly]

The Marketing Guru — Round 7 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr285_marketing-guru_r7 npm test.

I fetched/read the current PR diff and changed files. A totally different implementation path is not warranted from the docs/marketing lens.

The prior blockers are resolved: CHANGELOG.md has the security entry, README surfaces enterprise RBAC, docs/rbac.md explains trusted identity headers and gives a copy-pasteable verification flow, docs/api.md documents the new user/role endpoints with examples and forbidden status behavior, and /openapi.json includes the new role endpoints plus RBAC 403 contracts for protected operations.

No material public-facing docs or brand blocker remains.