Skip to content

https://github.com/GautamTalksDev/keyring/pull/new/test/audit-fork-regression - #2

Merged
GautamTalksDev merged 3 commits into
mainfrom
test/audit-fork-regression
Aug 29, 2026
Merged

GautamTalksDev merged 3 commits into
mainfrom
test/audit-fork-regression

Conversation

@GautamTalksDev

Copy link
Copy Markdown
Owner

Guards the fork bug fixed in #1. Forces 20 concurrent chained appends to share
an identical recorded_at, then asserts no duplicate prevHash values and that
the full chain verifies. Fails against the pre-seq implementation.

20 concurrent chained appends forced to share a timestamp; asserts no
duplicate prevHash and full chain verification. Fails against the
pre-seq implementation.
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add audit-chain fork regression test for identical timestamps

🧪 Tests 🕐 Less than 10 minutes

Grey Divider

AI Description

• Reproduces concurrent audit appends sharing one database-generated timestamp.
• Confirms predecessor hashes remain unique and the complete audit chain verifies.
Diagram

graph TD
  A["Vitest case"] --> B["Fixed timestamp"] --> C["20 concurrent appends"] --> D[("Audit records")] --> E["Predecessor uniqueness"] --> F["Chain verification"]
Loading
High-Level Assessment

The deterministic database-default timestamp combined with real concurrent appends is the appropriate regression strategy: it directly reproduces the former ordering ambiguity while retaining end-to-end coverage of persistence and chain verification. Mocking time or testing append serialization in isolation would provide weaker database-level assurance.

Files changed (1) +47 / -1

Tests (1) +47 / -1
audit-append-only.test.tsCover concurrent appends with identical recorded timestamps +47/-1

Cover concurrent appends with identical recorded timestamps

• Adds an integration regression test that forces 20 concurrent audit appends to share one database-generated recorded_at value. It confirms the setup, rejects duplicate prevHash usage across the ledger, and verifies the complete stored chain.

packages/server/src/db/audit-append-only.test.ts

@qodo-code-review

qodo-code-review Bot commented Aug 29, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Fixed default leaks across tests ✓ Resolved 🐞 Bug ☼ Reliability
Description
The test permanently changes audit_records.recorded_at from its normal now() default to a fixed
timestamp and never restores it. In real-PostgreSQL test mode the database is shared and teardown
only closes the client, so subsequent inserts and test suites receive the stale 2026 timestamp.
Code

packages/server/src/db/audit-append-only.test.ts[117]

+      sql`ALTER TABLE audit_records ALTER COLUMN recorded_at SET DEFAULT '2026-08-29T23:00:00.000Z'::timestamptz`,
Evidence
The schema defines recorded_at as defaultNow(), while the insert path omits recordedAt, so
changing the column default directly changes later audit data. openTestDatabase connects to the
unchanged DATABASE_URL in PostgreSQL mode, and PostgreSQL teardown only ends that client rather
than recreating or resetting the database.

packages/server/src/db/schema.ts[51-65]
packages/server/src/db/store.ts[121-137]
packages/server/src/db/test-db.ts[15-25]
packages/server/src/db/client.ts[64-72]
packages/server/src/db/audit-append-only.test.ts[35-37]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new regression test changes the persistent `audit_records.recorded_at` column default and does not restore it, leaking a fixed timestamp into later tests when using the shared PostgreSQL test backend.

## Issue Context
The production schema declares `recorded_at` with `defaultNow()`, and audit inserts omit that field. Wrap the test mutation and assertions in `try/finally`, restoring the default with `ALTER TABLE audit_records ALTER COLUMN recorded_at SET DEFAULT now()` in the `finally` block so restoration also occurs when an assertion or append fails.

## Fix Focus Areas
- packages/server/src/db/audit-append-only.test.ts[114-157]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
Review mode: ⚖️ Balanced

Grey Divider

Tip of the day
💡 Did you know, you can group findings by type and pick your Finding display, from Minimal to Full

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread packages/server/src/db/audit-append-only.test.ts Outdated
GautamTalksDev and others added 2 commits August 29, 2026 19:39
Use TypeScript project references so clean runners compile core before dependent packages, and fix the connector strict-mode parameter types.

Co-authored-by: Cursor <cursoragent@cursor.com>
Addresses Qodo review: the fixed default leaked into subsequent tests on the
shared Postgres backend and was never restored on assertion failure.

Co-authored-by: Cursor <cursoragent@cursor.com>
@GautamTalksDev
GautamTalksDev merged commit 267df4e into main Aug 29, 2026
1 check passed
@GautamTalksDev
GautamTalksDev deleted the test/audit-fork-regression branch August 29, 2026 23:46
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.

1 participant