Skip to content

fix(persistence): address review comments on migration safety, transcript docs, and keychain clarity#14

Draft
Copilot wants to merge 2 commits into006-persistence-jsonl-sqlite-secrets-and-audit-logfrom
copilot/sub-pr-13
Draft

fix(persistence): address review comments on migration safety, transcript docs, and keychain clarity#14
Copilot wants to merge 2 commits into006-persistence-jsonl-sqlite-secrets-and-audit-logfrom
copilot/sub-pr-13

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 6, 2026

Five issues flagged in code review on the persistence PR: unsafe migration rename on partial failure, force-cast validation bypassing actionable errors, mismatched fsync doc, redundant schema_version migration, and misleading keychain backend contract.

Migration safety (migrate-devices.ts)

Rename gating — source file now only renamed to .migrated when errors.length === 0; any failure leaves it in place for manual remediation and retry.

Up-front type validation — replaced blind as string/as boolean casts with explicit checks before calling registerDevice. Invalid records push a descriptive error and continue; no more relying on SQLite constraint violations for input feedback:

// Previously — silent cast, confusing SQLite error on bad data
deviceId: device.deviceId as string,
role: (device.role as "admin" | "node" | "client") ?? "client",

// Now — validated against ROLES constant with actionable error messages
if (typeof device.deviceId !== "string" || device.deviceId.length === 0) {
    errors.push(`Skipping record: 'deviceId' must be a non-empty string ...`);
    continue;
}
if (!ROLES.includes(rawRole as (typeof ROLES)[number])) {
    errors.push(`Skipping record ${deviceId}: 'role' must be one of ${ROLES.join(", ")} ...`);
    continue;
}
// approved uses Boolean() to handle legacy numeric 1/0 from SQLite exports
approved: Boolean(device.approved),

Schema migrations (migrations.ts)

Removed the redundant v3 migration that created schema_version. runMigrations() in OperationalStore already creates this table unconditionally before iterating MIGRATIONS, making v3 a dead no-op that muddied the migration history.

Transcript writer (transcript-writer.ts)

Fixed JSDoc on the fsync option: said fd.datasync(), implementation uses fsyncSync().

Keychain store (keychain-secret-store.ts)

Added class-level JSDoc and updated the constructor comment to clearly state that OS keychain availability is detected (exposed via hasKeychainAccess) but all reads/writes go through EncryptedFileSecretStore. Operators using secretsBackend: "keychain" are explicitly warned secrets land in the vault file, not the OS keychain. The serviceName option is documented as reserved for future keychain integration.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

…, and keychain store

Co-authored-by: aaronolds <1485194+aaronolds@users.noreply.github.com>
Copilot AI changed the title [WIP] Add JSONL transcripts and SQLite store with audit enhancements fix(persistence): address review comments on migration safety, transcript docs, and keychain clarity Mar 6, 2026
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.

2 participants