fix(server): shorten default audit retention - #74
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 31 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAudit retention now defaults to 2 days instead of 90 days. The minimum allowed value is 1 day instead of 7 days. SQLite migration, runtime configuration, documentation, and retention tests reflect these values. ChangesAudit retention defaults
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/server/src/__tests__/storage-sqlite.test.ts (1)
804-820: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a migration-preservation regression test.
This test verifies only that a new row receives
2. Add a pre-0006 fixture with a non-default override, such as30, and assert that the value remains unchanged after migration 0006.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/__tests__/storage-sqlite.test.ts` around lines 804 - 820, Add a pre-0006 audit_retention_config fixture with a non-default retention_days value such as 30, then run the migration path and assert the existing row remains 30. Extend TestSqliteAuditRetentionDefaultsToTwoDays or add a focused regression test while preserving the current assertion that newly inserted rows default to 2.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server/src/__tests__/audit-retention.test.ts`:
- Around line 471-473: Update the rejection assertion for
retention.setRetentionConfig with a zero value to match the specific
minimum-retention error, rather than the broad pattern that accepts any message
containing “retention.” Keep the test focused on proving that the lower-bound
validation rejects 0.
In `@packages/server/src/db/migrations/0006_audit_retention_default.sql`:
- Around line 1-15: Update the table-rebuild migration around
audit_retention_config_v2 so existing indexes from 0001_local_bootstrap.sql are
recreated after the renamed table is restored. Add matching CREATE INDEX
statements within the migration’s transaction, preserving the original index
definitions and ensuring they target the final audit_retention_config table.
---
Nitpick comments:
In `@packages/server/src/__tests__/storage-sqlite.test.ts`:
- Around line 804-820: Add a pre-0006 audit_retention_config fixture with a
non-default retention_days value such as 30, then run the migration path and
assert the existing row remains 30. Extend
TestSqliteAuditRetentionDefaultsToTwoDays or add a focused regression test while
preserving the current assertion that newly inserted rows default to 2.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 74acca40-a20f-46b4-90cc-97a3bafac928
📒 Files selected for processing (7)
docs/audit-design.mdpackages/server/src/__tests__/audit-retention.test.tspackages/server/src/__tests__/retention-gc.test.tspackages/server/src/__tests__/storage-sqlite.test.tspackages/server/src/db/migrations/0006_audit_retention_default.sqlpackages/server/src/engine/audit-retention.tspackages/server/src/engine/retention-gc.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/audit-design.md">
<violation number="1" location="docs/audit-design.md:125">
P3: The repository now exposes conflicting retention contracts: this design document says 2/1, but `specs/audit.md` and the retention workflow still require 90/7. Updating the canonical spec and generated workflow (or clearly marking them obsolete) would prevent future implementations and verification from restoring the old behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| | Setting | Default | Minimum | Maximum | | ||
| |--------------------|---------|---------|----------| | ||
| | `retentionDays` | 90 | 7 | 365 | | ||
| | `retentionDays` | 2 | 1 | 365 | |
There was a problem hiding this comment.
P3: The repository now exposes conflicting retention contracts: this design document says 2/1, but specs/audit.md and the retention workflow still require 90/7. Updating the canonical spec and generated workflow (or clearly marking them obsolete) would prevent future implementations and verification from restoring the old behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/audit-design.md, line 125:
<comment>The repository now exposes conflicting retention contracts: this design document says 2/1, but `specs/audit.md` and the retention workflow still require 90/7. Updating the canonical spec and generated workflow (or clearly marking them obsolete) would prevent future implementations and verification from restoring the old behavior.</comment>
<file context>
@@ -122,13 +122,13 @@ Retention is configurable per organization.
| Setting | Default | Minimum | Maximum |
|--------------------|---------|---------|----------|
-| `retentionDays` | 90 | 7 | 365 |
+| `retentionDays` | 2 | 1 | 365 |
### Configuration
</file context>
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/server/src/__tests__/audit-retention.test.ts">
<violation number="1" location="packages/server/src/__tests__/audit-retention.test.ts:475">
P3: Error message grammar: "1 days" should be "1 day" when the value is 1. The template `retentionDays must be at least ${MIN_RETENTION_DAYS} days` in audit-retention.ts:123 always appends "days" regardless of plurality. Consider a conditional or a separate message for the minimum case.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| async () => retention.setRetentionConfig(db, "org_test", 0), | ||
| (error: unknown) => | ||
| error instanceof Error && | ||
| error.message === "retentionDays must be at least 1 days", |
There was a problem hiding this comment.
P3: Error message grammar: "1 days" should be "1 day" when the value is 1. The template retentionDays must be at least ${MIN_RETENTION_DAYS} days in audit-retention.ts:123 always appends "days" regardless of plurality. Consider a conditional or a separate message for the minimum case.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/server/src/__tests__/audit-retention.test.ts, line 475:
<comment>Error message grammar: "1 days" should be "1 day" when the value is 1. The template `retentionDays must be at least ${MIN_RETENTION_DAYS} days` in audit-retention.ts:123 always appends "days" regardless of plurality. Consider a conditional or a separate message for the minimum case.</comment>
<file context>
@@ -470,7 +470,9 @@ test("retention minimum is 1 day and lower values are rejected", async () => {
- /1|minimum|retention/i,
+ (error: unknown) =>
+ error instanceof Error &&
+ error.message === "retentionDays must be at least 1 days",
);
});
</file context>
1 participant
Sets default RelayAuth audit retention to 2 days and permits a 1-day minimum. Adds migration 0006 to preserve existing overrides while changing the SQLite column default.\n\nVerification:\n- server tests: 458/458 passed\n- root typecheck: 11/11 passed\n- server build passed\n- Veto diff review: PASS, no findings\n\nDownstream Cloud must explicitly admit/rehearse migration 0006 and change the temporary production 7-day overrides to 2 only after this server version is deployed.