Skip to content

feat: add Participants Mode and human/AI task ownership - #169

Open
lee-to wants to merge 4 commits into
mainfrom
feature/participants-mode-local-auth-task-ownership
Open

feat: add Participants Mode and human/AI task ownership#169
lee-to wants to merge 4 commits into
mainfrom
feature/participants-mode-local-auth-task-ownership

Conversation

@lee-to

@lee-to lee-to commented Jul 31, 2026

Copy link
Copy Markdown
Owner

Summary

  • add opt-in Participants Mode with local password authentication, opaque sessions, CSRF, exact-origin CORS, RBAC, authenticated WebSockets, and separate HTTP MCP bearer authentication
  • add Human/AI task ownership, participant assignments, revision-safe handoffs, actor-aware manual transitions, immutable executor history, and audit events
  • exclude Human-owned tasks from coordinator, scheduler, auto-queue, watchdog, and runtime-budget execution paths
  • add login, participant administration, ownership/handoff controls, filters, participant-aware comments, executor timeline, assignment notifications, and synchronized Pencil designs
  • add self-service password changes that preserve the current session, revoke other sessions, and record an audit event
  • portal role selectors outside scroll containers and add consistent participant-dialog action spacing
  • document bootstrap, configuration, security policy, API/MCP contracts, password changes, proxy headers, recovery, and workspace caveats

Validation

  • format, lint, unit/integration tests, coverage, and production builds passed across all packages; every package remains above 70% coverage
  • the full web suite passed: 95 files and 723 tests
  • Playwright MCP verified admin/member account menus, participant creation spacing, role-selector portal behavior without table overflow, and the password-change flow
  • Pencil MCP contains the four participant interface states and reports no layout problems
  • Codex app-server protocol artifacts are in sync
  • npm run ai:validate reaches the perf phase; the existing perf/load harness still assumes anonymous API access and receives 401 in Participants Mode, including on an isolated clean database

Issue: #159

Closes #159

Add opt-in local participant authentication, RBAC, CSRF, WebSocket and MCP protections.

Add human and AI task ownership, revision-safe handoffs, executor history, collaboration UI, documentation, and regression coverage.

Refs #159
@lee-to
lee-to requested a review from ichinya July 31, 2026 13:17
lee-to added 3 commits July 31, 2026 17:01
Add an interactive first-admin flow, keep dropdown triggers measurable, and allow Human-owned tasks to be narrowed by assignee.

Refs #159
Flush pending loader warnings before starting interactive input so the username prompt remains at the active cursor position.

Refs #159
Add self-service password changes while preserving the current session and revoking other sessions. Portal select menus outside overflow containers and restore consistent dialog action spacing.

Refs #159
@ichinya

ichinya commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

PR #169 Review — feat: add Participants Mode and human/AI task ownership

Verdict: Request changes

PR is functionally extensive and architecturally well-thought-out: full-fledged participant auth, scrypt-hashing, RBAC, CSRF, CORS, WebSocket auth, auditing, migrations, and test coverage. However, there are three P1 blockers that need to be resolved before merging: IDOR on task mutations, MCP HTTP auth bypass, and race condition in handoff. The rest is P2/P3.


Overview

  • Scope: 156 files, +15 641 / -1 471.
  • Head: 4efb23d4.
  • Purpose: opt-in Participants Mode with local authentication, RBAC, CSRF, CORS, WebSocket auth, HTTP MCP bearer auth, Human/AI task ownership, revision-safe handoff, and audit.
  • Tests: web suite 95 files / 723 tests, coverage of all packages >70%.

What was tested

  • Auth / sessions / CSRF / CORS / RBAC (packages/api/src/middleware/*, packages/data/src/authSessions.ts, packages/data/src/participants.ts).
  • API routes (packages/api/src/routes/{auth,participants,tasks}.ts, packages/api/src/ws.ts, packages/api/src/index.ts).
  • Data layer (packages/data/src/taskOwnership.ts, packages/data/src/taskTransitions.ts, packages/data/src/index.ts, packages/data/src/audit.ts).
  • State machine (packages/shared/src/stateMachine.ts, packages/shared/src/types.ts).
  • MCP (packages/mcp/src/server.ts, packages/mcp/src/env.ts).
  • Schema / migrations (packages/shared/src/schema.ts, packages/shared/src/db.ts v27).
  • Docs (SECURITY.md, AGENTS.md, README.md).

[P1] IDOR: any member can mutate other members' tasks

Location: packages/api/src/routes/tasks.ts:771, :856, :994, :1044

The global participantRouteAuthorization() (packages/api/src/index.ts:37) requires authentication, but it is not a full-fledged authorization middleware for intra-resource checks. As a result, the following endpoints are accessible to any authenticated member, without checking the owner/assignee/role:

  • PUT /tasks/:id - changes title, description, plan, paused, priority, blockedReason, etc.
  • PATCH /tasks/:id/position — changes the order of any task.
  • POST /tasks/:id/sync-plan — re-reads the plan from disk for any task.
  • POST /tasks/:id/run-qa — runs the QA pipeline for any AI-owned task.
// packages/api/src/routes/tasks.ts:771
tasksRouter.put("/:id", jsonValidator(updateTaskSchema), async (c) => {
  const { id } = c.req.param();
  const body = c.req.valid("json");
  const existing = findTaskById(id);
  if (!existing) return c.json({ error: "Task not found" }, 404);
  // ... no verification: admin? assignee?
  const updated = updateTask(id, updatePayload);
**Risk:** any participant can pause (`paused=true`) AI tasks, rewrite the plan, change the priority, and restart QA, disrupting the workflow and DoS-ing the coordinator.

**Recommendation:** add an ownership/role guard to each of these routes. Minimum:
- `PUT /tasks/:id` - admin or assignee.
- `PATCH /tasks/:id/position` - admin or assignee within a project.
- `POST /tasks/:id/sync-plan` - admin or assignee.
- `POST /tasks/:id/run-qa` - admin or assignee.

`DELETE /tasks/:id` (`:875`) is protected as admin-only here via `isAdminOnlyRoute()` in `participantRouteAuthorization`, so I don't flag it separately.

---

## [P1] MCP HTTP is open if Participants Mode is off

**Location:** `packages/mcp/src/server.ts:124-148`, `packages/mcp/src/env.ts:97-105`

```ts
// packages/mcp/src/server.ts:124
if (env.participantsModeEnabled) {
  const token = bearerToken(...);
  if (!env.authToken || !tokensMatch(token, env.authToken)) {
    res.writeHead(401, ...);
    return;
  }
}
await handleMcp(req, res);
// packages/mcp/src/env.ts:97
if (env.transport === "http" && env.participantsModeEnabled && !env.authToken) {
  throw new Error("MCP_AUTH_TOKEN is required when MCP_TRANSPORT=http and PARTICIPANTS_MODE_ENABLED=true.");
}

Authentication on /mcp only works when PARTICIPANTS_MODE_ENABLED=true. If the operator enables MCP_TRANSPORT=http without Participants Mode, /mcp is accessible without a bearer token. This contradicts the stated "separate HTTP MCP bearer authentication" and grants full access to MCP tools (create/update tasks, projects, etc.).

Recommendation: require MCP_AUTH_TOKEN for MCP_TRANSPORT=http regardless of PARTICIPANTS_MODE_ENABLED; in createMcpHttpHandler, check env.authToken instead of env.participantsModeEnabled.


[P1] Race condition: handoffTaskExecution does not atomically check the lock

Location: packages/data/src/taskOwnership.ts:250-265, :428-438; packages/data/src/index.ts:1413-1426

handoffTaskExecution reads the task and checks task.lockedBy/task.lockedUntil in memory, but UPDATE tasks (:428-438) only includes ownershipRevision / executionOwner / status in the WHERE, without checking the lock:

.where(
  and(
    eq(tasks.id, task.id),
    eq(tasks.ownershipRevision, input.expectedOwnershipRevision),
    ...
  ),
)

tryStartQaRun (packages/data/src/index.ts:1413) and claimTask (packages/data/src/index.ts:2161) set lockedBy/lockedUntil without changing ownershipRevision. In a competitive claimTask between reading and UPDATE in handoffTaskExecution, a manual handoff can pass while the AI has already locked the task.

Recommendation: add the condition or(isNull(tasks.lockedBy), lte(tasks.lockedUntil, nowIso)) to the WHERE clause of handoffTaskExecution, or start the transaction with BEGIN IMMEDIATE/BEGIN EXCLUSIVE.


[P2] CSRF hostMatchesOrigin breaks cross-subdomain / proxy

Location: packages/api/src/middleware/csrf.ts:24-34

function hostMatchesOrigin(requestUrl, host, origin) {
  const originHostname = new URL(origin).hostname.toLowerCase();
  const requestHostname = new URL(requestUrl).hostname.toLowerCase();
  const headerHostname = new URL(`http://${host}`).hostname.toLowerCase();
  return headerHostname === requestHostname && headerHostname === originHostname;
}

Host is the API host, Origin is the frontend host. When cross-subdomain (app.example.comapi.example.com), the hostname is different, and the CSRF protection will reject a legitimate request. Also, when reverse proxy, c.req.url may differ from Host if Hono does not use the Host header to build the URL.

Recommendation: do not require Host === Origin. It is sufficient to check Origin/Referer against PARTICIPANT_ALLOWED_ORIGINS. Validate Host separately (for example, using an allowlist) or support X-Forwarded-Host.


[P2] internalBroadcastAuth is not timing-safe and has a dev bypass via X-Forwarded-For

Location: packages/api/src/middleware/internalBroadcastAuth.ts:25-53

if (configuredToken) {
  return { trusted: headerToken === configuredToken, ... }; // === instead of timingSafeEqual
}

if (nodeEnv === "development") {
  return {
    trusted:
      isLoopbackAddress(c.req.header("x-forwarded-for")) ||
      isLoopbackAddress(c.req.header("x-real-ip")),
    ...
  };
}
  • === instead of crypto.timingSafeEqual — timing side-channel.
  • X-Forwarded-For is controlled by the client, unless proxy rewrites; in dev env, accidentally exposed, this is a bypass.
  • NODE_ENV === "test" gives trusted: true — worth documenting.

participantAuth.ts already does a timing-safe check for internal broadcast, so this is a duplication and a footgun. It's still worth fixing.

Recommendation: replace with timingSafeEqual, remove the X-Forwarded-For bypass, or wrap it under the explicit flag AIF_DEV_INSECURE_BROADCAST=true, and document the test_bypass.


[P2] State machine: improve is not handled in resolveLegacyAction

Location: packages/shared/src/stateMachine.ts:70-128, :325-328

The new status improve is in TASK_STATUSES and is allowed for mark_plan_ready in resolveHumanOwnerAction, but:

  • HUMAN_ACTIONS_BY_STATUS.improve = [] - the UI will not offer any actions.
  • resolveLegacyAction doesn't know about improve — an AI-owned task in this status won't be able to move forward.
  • approve_done / request_changes / retry_from_blocked in the human branch are delegated to resolveLegacyAction, which will reject the request from improve.

Recommendation: either remove improve from TASK_STATUSES or add it to all necessary state machine locations.


[P2] WebSocket broadcast рассылает всё всем

Location: packages/api/src/ws.ts:239-259

export function broadcast(event: WsEvent): void {
  for (const client of clients) {
    if (client.readyState === client.OPEN) {
      client.send(JSON.stringify(event));

task:handoff, task:assignment_updated, auth:session_revoked, etc. are sent to all connected clients. If there is isolation by project/participant, this is a metadata leak.

Recommendation: routing map participantId -> Set<WebSocket> / projectId -> Set<WebSocket> with sensitive event filtering.


[P2] npm run ai:validate does not pass the perf phase in Participants Mode

Location: PR body, Validation

npm run ai:validate reaches the perf phase and gets a 401 in participant mode, even on an isolated clean DB. This means that the standard validation pipeline is not green.

Recommendation: update the perf/load harness to create a participant session and use a CSRF token, or run it with PARTICIPANTS_MODE_ENABLED=false.


[P2] SECURITY.md is outdated

Location: SECURITY.md

In SECURITY.md, it still says:

"The WebSocket endpoint has no authentication — intended for local development use"

PR adds authorizeWebSocketRequest() (packages/api/src/ws.ts:47-118) with Origin and cookie validation. The document is misleading.

Recommendation: update SECURITY.md and docs/configuration.md with the current threat model.


[P3] Missing indexes

Location: packages/shared/src/schema.ts:61-275

Drizzle-schema does not declare indexes for frequent lookups:

  • participantSessions.participantId
  • auditEvents.taskId, participantId, createdAt
  • taskExecutorHistory.taskId
  • taskAssignments.participantId

In SQLite, UNIQUE/PRIMARY KEY indexes are created, but the rest of the queries will be sequentially scanned.

Recommendation: add .index() / CREATE INDEX to the schema/migration.


Positive notes

  • Passwords: scrypt N=16384, r=8, p=1, 32-байт key, 16-байт salt + timingSafeEqual (packages/data/src/authSessions.ts:171-217).
  • Сессии: 256-bit opaque token, SHA-256 digest in the DB, CSRF token — HMAC-SHA256 from session token (packages/data/src/authSessions.ts:248-259, :350-378).
  • Cookies: httpOnly, secure (env-driven), SameSite=Lax, path: "/" (packages/api/src/routes/auth.ts:116-123).
  • Rate limiting:** /auth/login, /auth/change-password (packages/api/src/routes/auth.ts:26-57).
  • Final active admin: cannot deactivate the last admin (packages/data/src/participants.ts:242-247).
  • Audit: append-only triggers (packages/shared/src/db.ts:979-1002), snapshots instead of live links.
  • Handoff: optimistic locking by ownershipRevision (packages/data/src/taskOwnership.ts:428-438).
  • Migration v27: a single migration creates tables, backfills existing tasks as executionOwner='ai', and triggers.
  • Middleware order: CORS → Auth → Route auth → CSRF (packages/api/src/index.ts:33-38) is correct.

Summary

PR does a huge amount of work and is generally architecturally correct. Before merging, be sure to fix the following:

  1. P1: Authorization on task mutations PUT/PATCH/sync-plan/run-qa.
  2. P1: MCP HTTP auth regardless of Participants Mode.
  3. P1: race condition in handoffTaskExecution on lock.

P2 (CSRF host/origin, internalBroadcastAuth, improve state machine, broadcast scoping, ai:validate, SECURITY.md) can be either here or fast-follow. P3 (indexes) — follow-up.

@ichinya ichinya left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is functionally extensive and architecturally well-thought-out: full-fledged participant auth, scrypt hashing, RBAC, CSRF, CORS, WebSocket auth, auditing, migrations, and test coverage. However, there are three P1 blockers that need to be resolved before merging: IDOR on task mutations, MCP HTTP auth bypass, and race condition in handoff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[feature] Add Participants Mode with local auth and human/AI task ownership

2 participants