feat(audit-trail): record the id a primary key moved from [PRD-1321]

[bexchauveto]

Closes PRD-1321. The Ruby twin is agent-ruby#394, already merged.

What was lossy

An update carries two states but is filed under one id. When a primary key is both writable (so the capture records it) and redacted, both sides hold the placeholder, the packed id is the only value left — and it speaks for the side the row was filed under, not the other one.

#1909 chose to withhold rather than answer the previous side from the id the record ended up with. That is correct, and it costs a caller squarely in scope the values they should have seen: /state loses the most, since it rebuilds from a delete row carrying the whole writable column set.

What this does

previous_record_id on the audit row, written on every confirmed update, and withhold.ts judges the previous side against it.

Written whether or not the key moved — this is the one place it departs from agent-ruby#394. That table had never shipped, so a null there can only mean the key held still. This one has: a null has to keep meaning written before this column existed, or a row from an older agent whose key did move would be judged by the id it moved to, which is exactly the leak #1909 closed. For the same reason the column arrives as 002-add-previous-record-id rather than an edit to 001, which deployed databases have already recorded as applied.

Ported from Ruby unchanged:

• a pending update's new side answers only with what it captured — the row is filed under the id the record had before the write, which says nothing about the state it was moving to;
• the column never reaches a client. Both response paths strip it.

Tests

• the previous side of a moved key is now released to a scope that covers the id it came from, and the new side withheld — the case that was lost;
• a pending update's new side stays withheld;
• the capture records the previous id on a move and on a no-move alike;
• the column is absent from the served payload;
• migration 002 applies, is idempotent, and the schema assertions cover the new column.

442 tests pass across test/audit-trail and test/routes/access.

Two things the existing tests caught while I wrote this, both worth knowing: an absent column read as known until I normalised undefined to null (which released a previous side it should have withheld), and decoding per side rather than per row made a bad id warn twice.

🤖 Generated with Claude Code

Note

Record the previous primary-key id in audit-trail update entries

Confirmed update audit entries now store the packed record id from before the write, alongside the resulting id. This applies to both key-changing and key-stable updates.

• Adds a nullable previous_record_id field to AuditRecord, persisted via a new migration 002 that adds the column to existing audit tables and skips if already present; rollback removes it (migrations.ts, sql-store.ts, types.ts).
• Value permission checks in withhold.ts now evaluate the previous snapshot against the pre-update identity and the resulting snapshot against the post-update identity; a pending update's new side is withheld when the stored identity cannot establish that scope.
• The internal previous id is stripped from all served HTTP responses in audit-trail.ts, including the batched served-match path.
• Risk: audit-history consumers see no new field, but the migration adds a column to existing audit tables; rows written before the column existed return null for the previous id.

📊 Macroscope summarized 05cba83. 7 files reviewed, 3 issues evaluated, 1 issue filtered, 2 comments posted

🗂️ Filtered Issues

packages/agent/src/audit-trail/withhold.ts — 0 comments posted, 1 evaluated, 1 filtered

• line 169: The code does not actually decode once per distinct id. Every confirmed update now has a non-null previousRecordId, including a no-op primary key update where it equals recordId; lines 168 and 169-172 call decodePrimaryKeys twice for that same malformed/unpackable id. This emits two identical warning logs for each such audit row, despite the intended single warning per id. [ Out of scope (post-validation triage) ]

[linear-code]

PRD-1321

[qltysh]

1 new issue

Tool	Category	Rule	Count
qlty	Structure	Function with many returns (count = 4): handleHistory	1

[macroscopeapp]

🟠 High audit-trail/migrations.ts:218

Concurrent upgrades on SQLite/MySQL/MariaDB/MSSQL can fail agent startup: two processes can both observe previous_record_id as absent, then the loser’s unguarded addColumn rejects with a duplicate-column error and Umzug marks migration 002 as failed. Make the column addition tolerate that dialect-specific duplicate-column race (or otherwise serialize the migration) so the losing process treats it as already applied.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @packages/agent/src/audit-trail/migrations.ts around line 218:

Concurrent upgrades on SQLite/MySQL/MariaDB/MSSQL can fail agent startup: two processes can both observe `previous_record_id` as absent, then the loser’s unguarded `addColumn` rejects with a duplicate-column error and Umzug marks migration `002` as failed. Make the column addition tolerate that dialect-specific duplicate-column race (or otherwise serialize the migration) so the losing process treats it as already applied.

[macroscopeapp]

🟡 Medium audit-trail/sql-store.ts:42

SQL persistence drops PendingAuditRecord.previousRecordId, so a subsequent confirm that omits the optional field leaves the row NULL; withholdOutsidePermissionScope then treats the confirmed update as pre-migration and withholds its previous side. Add previousRecordId to the toRow serialization so the SQL store preserves it like the in-memory store.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @packages/agent/src/audit-trail/sql-store.ts around line 42:

SQL persistence drops `PendingAuditRecord.previousRecordId`, so a subsequent `confirm` that omits the optional field leaves the row `NULL`; `withholdOutsidePermissionScope` then treats the confirmed update as pre-migration and withholds its previous side. Add `previousRecordId` to the `toRow` serialization so the SQL store preserves it like the in-memory store.

[qltysh]

Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (3)

Rating	File	% Diff	Uncovered Line #s
	packages/agent/src/audit-trail/withhold.ts	100.0%
	packages/agent/src/audit-trail/migrations.ts	100.0%
	packages/agent/src/routes/access/audit-trail.ts	100.0%
	Total	100.0%

🚦 See full report on Qlty Cloud »

🛟 Help

• Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

• Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

• File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

  • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.