Conversation
📝 WalkthroughWalkthroughThis PR introduces a signed viewer assertion token system to replace legacy Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Web as Web App<br/>(Apollo)
participant Server as Server<br/>(Auth)
participant DB as Database
Client->>Web: Request with viewer context
Web->>Web: Read assertion from env/<br/>localStorage
Web->>Server: GraphQL query with<br/>x-involute-viewer-assertion header
Server->>Server: Extract assertion &<br/>VIEWER_ASSERTION_SECRET
Server->>Server: verifyViewerAssertion()<br/>validate signature & expiry
alt Assertion Valid
Server->>Server: Decode claims<br/>(sub, subType)
alt subType === 'email'
Server->>DB: Query user by email
else subType === 'id'
Server->>DB: Query user by id
end
DB-->>Server: User record
Server->>Server: Create GraphQLContext<br/>with viewer
else Assertion Invalid/Missing
Server->>Server: Default to admin viewer
end
Server-->>Web: GraphQL response
Web-->>Client: Rendered UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces raw user headers with a signed 'viewer assertion' mechanism using HMAC for trusted impersonation across the CLI, server, and web packages. It also optimizes the server's GraphQL issues query by conditionally including related data based on the requested fields and introduces a performance test suite. I have no feedback to provide.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/environment.ts (1)
31-39:⚠️ Potential issue | 🟠 MajorFail fast when
VIEWER_ASSERTION_SECRETis missing.Line 38 collapses a missing/blank secret to
null, andpackages/server/src/auth.tsthen treats that the same as an invalid assertion and falls back toDEFAULT_ADMIN_EMAIL. A typoed env var therefore silently disables signed viewer assertions while authorized requests still resolve as the default admin.🔧 Suggested hardening
export interface ServerEnvironment { databaseUrl: string; authToken: string; port: number; - viewerAssertionSecret: string | null; + viewerAssertionSecret: string; } export function getServerEnvironment(env: NodeJS.ProcessEnv = process.env): ServerEnvironment { const port = Number(env.PORT ?? DEFAULT_PORT); + const viewerAssertionSecret = env.VIEWER_ASSERTION_SECRET?.trim(); + + if (!viewerAssertionSecret) { + throw new Error('VIEWER_ASSERTION_SECRET is required'); + } return { databaseUrl: env.DATABASE_URL ?? '', authToken: env.AUTH_TOKEN ?? '', port: Number.isFinite(port) && port > 0 ? port : DEFAULT_PORT, - viewerAssertionSecret: env.VIEWER_ASSERTION_SECRET?.trim() || null, + viewerAssertionSecret, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/environment.ts` around lines 31 - 39, The function getServerEnvironment should fail fast when VIEWER_ASSERTION_SECRET is missing or blank: in getServerEnvironment validate env.VIEWER_ASSERTION_SECRET (trimmed) and if it's undefined/empty throw a clear Error instead of returning null for viewerAssertionSecret; update the returned ServerEnvironment to expect a non-null viewerAssertionSecret and ensure callers rely on that invariant (references: getServerEnvironment, viewerAssertionSecret, ServerEnvironment, VIEWER_ASSERTION_SECRET).
🧹 Nitpick comments (3)
packages/server/src/schema.performance.test.ts (1)
87-88: Avoid hard-coding fixture totals in this perf test.
19and38tightly couple this to current seed contents. Prefer asserting the delta added by this test fixture.Suggested assertion tweak
- expect(baseline.issueCount).toBe(19); - expect(expanded.issueCount).toBe(38); + expect(expanded.issueCount - baseline.issueCount).toBe(19);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/schema.performance.test.ts` around lines 87 - 88, The test currently hard-codes seed totals using baseline.issueCount and expanded.issueCount; change it to assert the delta introduced by this test fixture instead—compute the difference (expanded.issueCount - baseline.issueCount) and assert that equals the expected number of issues your fixture adds (use a named constant like expectedAddedIssues for clarity) rather than comparing to literal totals..github/workflows/ci.yml (1)
1-15: Declare least-privilege workflow permissions explicitly.This job only checks out code and runs local install/test/build steps, so inheriting the repo default
GITHUB_TOKENpermissions is broader than necessary for a merge gate. Adding an explicit read-onlypermissionsblock makes that boundary unambiguous.🔒 Suggested change
name: CI + +permissions: + contents: read + on:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 1 - 15, The workflow currently inherits default GITHUB_TOKEN permissions; add an explicit least-privilege permissions block at the top of the workflow (e.g., under the existing "name: CI" or before "concurrency") to limit token scope to read-only repository contents — for example set permissions: contents: read — so the verify job (and the overall workflow) only has the minimal required access for checkout and local test/build steps.packages/cli/src/index.ts (1)
373-373: Consider documenting the security implications of--secret.Passing secrets via command-line flags exposes them in shell history and process listings (e.g.,
ps aux). While the environment variableINVOLUTE_VIEWER_ASSERTION_SECRETis provided as the primary method, users might still use--secretfor convenience.Consider adding a note to the command description or documentation warning users to prefer the environment variable approach, or to clear shell history after use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/index.ts` at line 373, Update the CLI option description for the '--secret <secret>' flag in packages/cli/src/index.ts to warn about security implications: explicitly note that passing secrets via the command line exposes them to shell history and process listings and recommend using the environment variable INVOLUTE_VIEWER_ASSERTION_SECRET instead (or clearing shell history/avoiding process lists) so users see the preferred, safer approach when they run or view help for the command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/schema.performance.test.ts`:
- Around line 12-72: The hardcoded counts and threshold in the schema
performance test are brittle; replace assertions that use fixed numbers (e.g.,
the toBe(19)/toBe(38) checks and the toBeLessThanOrEqual(20) query threshold)
with dynamic values derived from the seeded data or parameters so they adapt to
seed changes: compute expected counts by querying the prisma fixtures after
seedDatabase()/seedBoardFixture() (using prisma.issue.count(),
prisma.comment.count() or capturing returned totals from seedBoardFixture) and
then assert against those computed totals and a relative/per-item query budget
(e.g., queries per issue) instead of fixed integers; update references in this
file to executedQueries, seedDatabase, seedBoardFixture, and any count
assertions to use the derived values.
In `@packages/server/src/son-validation-restore.test.ts`:
- Around line 33-35: Tests reassign exportDir in beforeEach but only remove the
last directory in afterAll; change cleanup to run afterEach so each created temp
directory is removed: add an afterEach that deletes the directory created by
exportDir (guarding against undefined and using recursive removal) and remove
the existing afterAll cleanup block; reference the beforeEach that calls mkdtemp
and writeSonExportFixture, the exportDir variable, and the afterAll cleanup to
locate where to update.
In `@packages/shared/src/viewer-assertion.ts`:
- Around line 57-60: The verification currently accepts empty or whitespace-only
subjects; update the verification in the function handling ViewerAssertionClaims
(the code that uses the parsedPayload -> const claims = parsedPayload as
ViewerAssertionClaims) to reject blank subjects by checking that claims.sub is a
string and claims.sub.trim().length > 0 (and return null if not). Ensure this
check is applied alongside the existing exp validation so blank/whitespace-only
claims.sub values do not proceed to { id } / { email } lookups.
In `@README.md`:
- Around line 114-122: Update the local validation snippet so it matches CI by
replacing the current commands `pnpm typecheck`, `pnpm test`, `pnpm lint` with
the CI-equivalent sequence: include `pnpm build` and run tests with `pnpm test
--run` (i.e., `pnpm typecheck`, `pnpm test --run`, `pnpm lint`, `pnpm build`) so
following the README executes the same gates as GitHub Actions.
---
Outside diff comments:
In `@packages/server/src/environment.ts`:
- Around line 31-39: The function getServerEnvironment should fail fast when
VIEWER_ASSERTION_SECRET is missing or blank: in getServerEnvironment validate
env.VIEWER_ASSERTION_SECRET (trimmed) and if it's undefined/empty throw a clear
Error instead of returning null for viewerAssertionSecret; update the returned
ServerEnvironment to expect a non-null viewerAssertionSecret and ensure callers
rely on that invariant (references: getServerEnvironment, viewerAssertionSecret,
ServerEnvironment, VIEWER_ASSERTION_SECRET).
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 1-15: The workflow currently inherits default GITHUB_TOKEN
permissions; add an explicit least-privilege permissions block at the top of the
workflow (e.g., under the existing "name: CI" or before "concurrency") to limit
token scope to read-only repository contents — for example set permissions:
contents: read — so the verify job (and the overall workflow) only has the
minimal required access for checkout and local test/build steps.
In `@packages/cli/src/index.ts`:
- Line 373: Update the CLI option description for the '--secret <secret>' flag
in packages/cli/src/index.ts to warn about security implications: explicitly
note that passing secrets via the command line exposes them to shell history and
process listings and recommend using the environment variable
INVOLUTE_VIEWER_ASSERTION_SECRET instead (or clearing shell history/avoiding
process lists) so users see the preferred, safer approach when they run or view
help for the command.
In `@packages/server/src/schema.performance.test.ts`:
- Around line 87-88: The test currently hard-codes seed totals using
baseline.issueCount and expanded.issueCount; change it to assert the delta
introduced by this test fixture instead—compute the difference
(expanded.issueCount - baseline.issueCount) and assert that equals the expected
number of issues your fixture adds (use a named constant like
expectedAddedIssues for clarity) rather than comparing to literal totals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3d08802-78f7-4f76-8b21-fac026e98ebb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
.env.example.github/workflows/ci.yml.gitignoreREADME.mdpackages/cli/package.jsonpackages/cli/src/index.tspackages/server/package.jsonpackages/server/prisma/seed.test.tspackages/server/src/auth.test.tspackages/server/src/auth.tspackages/server/src/environment.tspackages/server/src/index.tspackages/server/src/schema.performance.test.tspackages/server/src/schema.tspackages/server/src/son-validation-restore.test.tspackages/shared/package.jsonpackages/shared/src/index.test.tspackages/shared/src/index.tspackages/shared/src/viewer-assertion.tspackages/web/package.jsonpackages/web/src/App.drag-utils.test.tsxpackages/web/src/lib/apollo.tsx
Comment on lines
+12
to
+72
| describe('schema query performance', () => { | ||
| const prisma = new PrismaClientConstructor({ | ||
| log: [{ emit: 'event', level: 'query' }], | ||
| }); | ||
| const executedQueries: string[] = []; | ||
| let server: StartedServer; | ||
| let adminId: string; | ||
| let defaultTeamId: string; | ||
| let readyStateId: string; | ||
|
|
||
| beforeAll(async () => { | ||
| prisma.$on('query', (event) => { | ||
| executedQueries.push(event.query); | ||
| }); | ||
|
|
||
| await prisma.$connect(); | ||
| server = await startServer({ | ||
| authToken: TEST_AUTH_TOKEN, | ||
| port: 0, | ||
| prisma, | ||
| }); | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| await server.stop(); | ||
| await prisma.$disconnect(); | ||
| }); | ||
|
|
||
| beforeEach(async () => { | ||
| executedQueries.length = 0; | ||
|
|
||
| await prisma.comment.deleteMany(); | ||
| await prisma.issue.deleteMany(); | ||
| await prisma.workflowState.deleteMany(); | ||
| await prisma.team.deleteMany(); | ||
| await prisma.issueLabel.deleteMany(); | ||
| await prisma.user.deleteMany(); | ||
| await prisma.legacyLinearMapping.deleteMany(); | ||
|
|
||
| await seedDatabase(prisma); | ||
|
|
||
| const admin = await prisma.user.findFirstOrThrow(); | ||
| const team = await prisma.team.findFirstOrThrow(); | ||
| const readyState = await prisma.workflowState.findFirstOrThrow({ | ||
| where: { | ||
| name: 'Ready', | ||
| teamId: team.id, | ||
| }, | ||
| }); | ||
|
|
||
| adminId = admin.id; | ||
| defaultTeamId = team.id; | ||
| readyStateId = readyState.id; | ||
|
|
||
| await seedBoardFixture(prisma, { | ||
| adminId, | ||
| readyStateId, | ||
| teamId: defaultTeamId, | ||
| }); | ||
| executedQueries.length = 0; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Vitest/Vite config files =="
fd -HI 'vitest(\.workspace)?\.(config\.)?(ts|js|mts|cts|mjs|cjs)$|vite\.config\.(ts|js|mts|cts|mjs|cjs)$'
echo
echo "== Parallelism/isolation-related config =="
rg -n -C2 'threads|pool|maxWorkers|minWorkers|isolate|fileParallelism|sequence|test\s*:' \
$(fd -HI 'vitest(\.workspace)?\.(config\.)?(ts|js|mts|cts|mjs|cjs)$|vite\.config\.(ts|js|mts|cts|mjs|cjs)$') || true
echo
echo "== Other DB-mutating tests =="
rg -n -C1 'deleteMany\(|seedDatabase\(|startServer\(' --iglob '*test*.ts'Repository: fakechris/Involute
Length of output: 21696
🏁 Script executed:
cat packages/server/vitest.config.tsRepository: fakechris/Involute
Length of output: 237
🏁 Script executed:
sed -n '72,$p' packages/server/src/schema.performance.test.ts | head -50Repository: fakechris/Involute
Length of output: 1655
Make count assertions resilient to seed data evolution.
The test uses absolute issue counts (toBe(19), toBe(38)) and query threshold (toBeLessThanOrEqual(20)) that depend on the exact output of seedDatabase() and seedBoardFixture(). If seed data changes in the future, these brittle assertions will fail even though the underlying query performance may be sound. Consider either:
- Capturing baseline/expanded counts dynamically from seed output rather than hardcoding
- Parameterizing the fixture totals so they adapt to seed changes
Note: File parallelism is already disabled in packages/server/vitest.config.ts, so cross-test interference is not a concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server/src/schema.performance.test.ts` around lines 12 - 72, The
hardcoded counts and threshold in the schema performance test are brittle;
replace assertions that use fixed numbers (e.g., the toBe(19)/toBe(38) checks
and the toBeLessThanOrEqual(20) query threshold) with dynamic values derived
from the seeded data or parameters so they adapt to seed changes: compute
expected counts by querying the prisma fixtures after
seedDatabase()/seedBoardFixture() (using prisma.issue.count(),
prisma.comment.count() or capturing returned totals from seedBoardFixture) and
then assert against those computed totals and a relative/per-item query budget
(e.g., queries per issue) instead of fixed integers; update references in this
file to executedQueries, seedDatabase, seedBoardFixture, and any count
assertions to use the derived values.
Comment on lines
+33
to
+35
|
|
||
| exportDir = await mkdtemp(join(tmpdir(), 'involute-son-restore-')); | ||
| await writeSonExportFixture(exportDir); |
There was a problem hiding this comment.
Temp directory cleanup only removes the last created directory.
exportDir is reassigned in each beforeEach run, but cleanup in afterAll only removes the final value. If there are multiple test cases (currently 2), only the last temp directory gets cleaned up.
Consider using afterEach for cleanup instead:
Proposed fix
beforeEach(async () => {
await prisma.legacyLinearMapping.deleteMany();
await prisma.comment.deleteMany();
await prisma.issue.deleteMany();
await prisma.workflowState.deleteMany();
await prisma.issueLabel.deleteMany();
await prisma.user.deleteMany();
await prisma.team.deleteMany();
exportDir = await mkdtemp(join(tmpdir(), 'involute-son-restore-'));
await writeSonExportFixture(exportDir);
});
+
+ afterEach(async () => {
+ if (exportDir) {
+ await rm(exportDir, { force: true, recursive: true }).catch(() => {});
+ }
+ });And remove the afterAll cleanup block at lines 71-73.
Also applies to: 71-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server/src/son-validation-restore.test.ts` around lines 33 - 35,
Tests reassign exportDir in beforeEach but only remove the last directory in
afterAll; change cleanup to run afterEach so each created temp directory is
removed: add an afterEach that deletes the directory created by exportDir
(guarding against undefined and using recursive removal) and remove the existing
afterAll cleanup block; reference the beforeEach that calls mkdtemp and
writeSonExportFixture, the exportDir variable, and the afterAll cleanup to
locate where to update.
Comment on lines
+57
to
+60
| const claims = parsedPayload as ViewerAssertionClaims; | ||
|
|
||
| if (!Number.isInteger(claims.exp) || claims.exp <= Math.floor(now.getTime() / 1000)) { | ||
| return null; |
There was a problem hiding this comment.
Reject blank sub values during verification.
Right now any string passes, so '' and whitespace-only subjects are accepted and forwarded into { id } / { email } lookups. Those assertions should fail validation instead of producing meaningless unique queries.
🔧 Suggested validation tweak
const claims = parsedPayload as ViewerAssertionClaims;
+ if (!claims.sub.trim()) {
+ return null;
+ }
+
if (!Number.isInteger(claims.exp) || claims.exp <= Math.floor(now.getTime() / 1000)) {
return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const claims = parsedPayload as ViewerAssertionClaims; | |
| if (!Number.isInteger(claims.exp) || claims.exp <= Math.floor(now.getTime() / 1000)) { | |
| return null; | |
| const claims = parsedPayload as ViewerAssertionClaims; | |
| if (!claims.sub.trim()) { | |
| return null; | |
| } | |
| if (!Number.isInteger(claims.exp) || claims.exp <= Math.floor(now.getTime() / 1000)) { | |
| return null; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared/src/viewer-assertion.ts` around lines 57 - 60, The
verification currently accepts empty or whitespace-only subjects; update the
verification in the function handling ViewerAssertionClaims (the code that uses
the parsedPayload -> const claims = parsedPayload as ViewerAssertionClaims) to
reject blank subjects by checking that claims.sub is a string and
claims.sub.trim().length > 0 (and return null if not). Ensure this check is
applied alongside the existing exp validation so blank/whitespace-only
claims.sub values do not proceed to { id } / { email } lookups.
Comment on lines
+114
to
+122
| Run workspace checks from the repository root: | ||
|
|
||
| ```bash | ||
| pnpm typecheck | ||
| pnpm test | ||
| pnpm lint | ||
| ``` | ||
|
|
||
| GitHub Actions now runs the same `typecheck`, `lint`, `test`, and `build` gates on pull requests and pushes to `main`. |
There was a problem hiding this comment.
Make the local validation snippet match CI.
This section omits pnpm build and uses pnpm test instead of pnpm test --run, so following the README does not exercise the same merge gates and may leave Vitest in watch mode.
📝 Suggested docs update
pnpm typecheck
-pnpm test
pnpm lint
+pnpm test --run
+pnpm build📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Run workspace checks from the repository root: | |
| ```bash | |
| pnpm typecheck | |
| pnpm test | |
| pnpm lint | |
| ``` | |
| GitHub Actions now runs the same `typecheck`, `lint`, `test`, and `build` gates on pull requests and pushes to `main`. | |
| pnpm typecheck | |
| pnpm lint | |
| pnpm test --run | |
| pnpm build |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 114 - 122, Update the local validation snippet so it
matches CI by replacing the current commands `pnpm typecheck`, `pnpm test`,
`pnpm lint` with the CI-equivalent sequence: include `pnpm build` and run tests
with `pnpm test --run` (i.e., `pnpm typecheck`, `pnpm test --run`, `pnpm lint`,
`pnpm build`) so following the README executes the same gates as GitHub Actions.
This was referenced Apr 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
.gitignoreto exclude compiled and generated artifacts