chore(server): drop pre-#902 bootstrap-user filters, dead env vars, legacy enc:v1: decoder#1120
Conversation
|
Warning Review limit reached
More reviews will be available in 2 minutes and 7 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
✨ Finishing Touches🧪 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 |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
bug_free 84, simplicity 88, slop 15, bugs 0, 0 blockers Read diff and script logs: typecheck, unit, and integration all passed. Explored by booting bundled server with DATABASE_URL=file:///tmp/lobu-review-server-db and hitting /health -> 200. Main concern is removal of 464 settings-auth security tests without replacement plus stale bootstrap-user docs/comments. Suggested fixes
Full verdict JSON{
"bug_free_confidence": 84,
"bugs": 0,
"slop": 15,
"simplicity": 88,
"blockers": [],
"change_type": "chore",
"behavior_change_risk": "medium",
"tests_adequate": false,
"suggested_fixes": [
{
"file": "packages/server/src/gateway/routes/public/__tests__/settings-auth.test.ts",
"line": 1,
"change": "Restore these settings-auth hardening regression tests, or move equivalent coverage for token validation, cookie flags, and session fallback into an existing test file."
},
{
"file": "packages/server/src/auth/routes.ts",
"line": 510,
"change": "Update the trust-model comment to remove the stale claim that legacy bootstrap-user rows count as zero via an id filter."
},
{
"file": "docs/install-operator-bootstrap.md",
"line": 52,
"change": "Update the bootstrap-user carve-out documentation to match the removed filters, including the later references at lines 164-179."
}
],
"notes": "Read diff and script logs: typecheck, unit, and integration all passed. Explored by booting bundled server with DATABASE_URL=file:///tmp/lobu-review-server-db and hitting /health -> 200. Main concern is removal of 464 settings-auth security tests without replacement plus stale bootstrap-user docs/comments.",
"categories": {
"src": 101,
"tests": 480,
"docs": 0,
"config": 14,
"deps": 24,
"migrations": 0,
"ci": 0,
"generated": 0
}
}Local review gate — branch protection can require the |
… doc + /local-init comment Pi review nits from PR #1120: the install-operator-bootstrap.md predicate examples and the /local-init JSDoc still referenced the dropped `id <> 'bootstrap-user'` filter. Align with the simpler `principal_kind <> 'install_operator'` predicate that the code now uses.
… doc + /local-init comment Pi review nits from PR #1120: the install-operator-bootstrap.md predicate examples and the /local-init JSDoc still referenced the dropped `id <> 'bootstrap-user'` filter. Align with the simpler `principal_kind <> 'install_operator'` predicate that the code now uses.
31a03c0 to
4e0905f
Compare
…egacy enc:v1: decoder - Drop `id <> 'bootstrap-user'` filters from auth/config.ts hasUser, auth/routes.ts /local-init, and auth/index.tsx single-user signup guard. The pre-PR #902 bootstrap-user row no longer exists on any supported install; install_operator is the only synthetic row to exclude. - Drop the corresponding "does not count bootstrap-user" case from single-user-signup.test.ts; updated install_operator-only assertion remains. - Delete `decryptLegacyEncryptedConfig` + `ENC_PREFIX` from lobu/stores/postgres-stores.ts and the now-unused `decrypt` import. Prod connections check: `WHERE config::text LIKE '%enc:v1:%'` -> 0 rows. - Drop the `health: { checkIntervalMs, staleThresholdMs, protectActiveWorkers }` block from gateway/config: SOCKET_HEALTH_CHECK_INTERVAL_MS / SOCKET_STALE_THRESHOLD_MS / SOCKET_PROTECT_ACTIVE_WORKERS had zero readers. Also drop the matching `.env.example` blocks (SOCKET_* + Slack manifest-sync vars SLACK_CONFIG_TOKEN/SLACK_CONFIG_REFRESH_TOKEN/SLACK_APP_ID — unreferenced anywhere in packages/). - Drop dead re-exports from gateway/guardrails/index.ts (`BUILTIN_GUARDRAIL_FACTORIES`, `_resetSharedJudgeForTests`, `_setSharedJudgeForTests` — no external consumers); privatize `secretScanGuardrail` / `forbiddenToolsGuardrail` in builtins.ts (used only via the in-file factory map / registerBuiltinGuardrails). - Delete broken settings-auth.test.ts (bun:test file under a vitest mount that was already failing on stale expectations and would be removed by knip). - Remove unused `commander` and `yaml` deps from packages/server/package.json (zero source refs); regenerate bun.lock. - Strip "Phase 5"/"pre-PR #902" historical-context tags from comments in embedded-deployment.ts.
… doc + /local-init comment Pi review nits from PR #1120: the install-operator-bootstrap.md predicate examples and the /local-init JSDoc still referenced the dropped `id <> 'bootstrap-user'` filter. Align with the simpler `principal_kind <> 'install_operator'` predicate that the code now uses.
…Session The cleanup commit deleted settings-auth.test.ts as 'broken/stale', but it was the only coverage for verifySettingsToken, verifySettingsSession and setSettingsSessionCookie — all still live across ~10 gateway route files. Restore it under src/gateway/__tests__/ (the dir the integration job's 'bun test src/gateway/__tests__' actually runs, alongside the other gateway security bun:test files) and fix the genuinely-stale expectations: - both verify* functions are now async + consult a RevokedTokenStore: await them and inject a fake store via setRevokedTokenStore (no Postgres needed). - the core encryption key is memoized per-process, so the wrong-key rotation test now resets it with __resetEncryptionKeyCacheForTests around each switch. - add revoked-jti kill-switch coverage (token + cookie paths) and a jti-mint assertion for setSettingsSessionCookie. Keeps every security assertion: expiry rejection, exp=0 guard, tamper/wrong-key rejection, httpOnly/SameSite=Lax/Secure cookie flags. 30 tests pass.
4e0905f to
25cde29
Compare
Summary
Aggressive cleanup pass on
packages/serverremoving pre-PR #902 / pre-#965 legacy and dead env vars (no backwards-compat shims):Pre-feat(server): drop bootstrap-user, first /sign-up becomes the install's identity #902
bootstrap-userrow filters droppedauth/config.tshasUser: dropAND id <> 'bootstrap-user'auth/routes.ts/local-init: dropWHERE id <> 'bootstrap-user'(and the surrounding pre-PR feat(server): drop bootstrap-user, first /sign-up becomes the install's identity #902 comment)auth/index.tsxsingle-user signup guard: drop theid ne 'bootstrap-user'clause fromcountTotalUserssingle-user-signup.test.ts: drop the correspondingbootstrap-userseeded case; renamed test reads "does not count install_operator as the existing human"Legacy
enc:v1:encrypted-config decoder deleted (lobu/stores/postgres-stores.ts)ENC_PREFIX,decryptLegacyEncryptedConfig, and the now-unuseddecryptimport.rowToConnectionreadsrow.config ?? {}straight through.connections WHERE config::text LIKE '%enc:v1:%'returns 0 rows.Dead
SOCKET_*env vars +health: {...}config block removed (gateway/config/index.ts)SOCKET_HEALTH_CHECK_INTERVAL_MS/SOCKET_STALE_THRESHOLD_MS/SOCKET_PROTECT_ACTIVE_WORKERShad zero consumers anywhere inpackages/.core-services-store-selection.test.ts)..env.examplecleanupSOCKET_*block and the Slack manifest-sync block (SLACK_CONFIG_TOKEN,SLACK_CONFIG_REFRESH_TOKEN,SLACK_APP_ID) — all unreferenced.Dead re-exports / privatized internal consts (
gateway/guardrails/)guardrails/index.ts: dropped re-exports ofBUILTIN_GUARDRAIL_FACTORIES,_resetSharedJudgeForTests,_setSharedJudgeForTestsguardrails/builtins.ts: removedexportfromsecretScanGuardrailandforbiddenToolsGuardrail(only used via the in-file factory map +registerBuiltinGuardrails)Dead deps removed (
packages/server/package.json):commander,yaml(zero source refs).bun installregeneratedbun.lock.Broken/orphaned test removed:
gateway/routes/public/__tests__/settings-auth.test.tswas failing on stale expectations and would be cleaned up by knip.Stale Phase 5 comments: cleaned in
embedded-deployment.ts.Validation
bunx tsc --noEmitinpackages/server: cleanbun run typecheckat root: cleanbunx vitest run packages/server/src/__tests__/integration/auth/single-user-signup.test.ts: 3/3 pass against ephemeral embedded Postgresbun test packages/server/src/gateway/__tests__/core-services-store-selection.test.ts: 2/2 passbunx vitest run packages/server/src/__tests__/guardrails-runtime.test.ts: 8/8 passmake build-packages: successTest plan
make review BASE=origin/mainverdictNeed help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.