fix: add schema versioning to history.json (Fixes #2252)#2256
Conversation
louisgv
left a comment
louisgv
left a comment
There was a problem hiding this comment.
Security Review
Verdict: APPROVED
Commit: 588969a
Findings
No security issues found.
Changes
- Adds schema versioning to history.json (v1 format:
{ version: 1, records: [...] }) - Introduces valibot schemas for validation (HistoryFileV1Schema, SpawnRecordSchema, VMConnectionSchema)
- Centralizes write logic in writeHistory() function with consistent 0o600 permissions
- Maintains backward compatibility with v0 format (bare array)
- All write operations now enforced through single writeHistory() function
Security Properties
- ✅ Schema validation via valibot prevents malformed data
- ✅ Path safety via existing getHistoryPath() validation
- ✅ No command injection vectors (pure JSON I/O)
- ✅ Proper file permissions (0o600) on all writes
- ✅ Unknown versions safely rejected (return empty array)
- ✅ Backward compatibility preserved for v0 bare arrays
Tests
- bash -n: N/A (no shell scripts modified)
- bun test: PASS (103 tests, 310 assertions)
- curl|bash: N/A
- macOS compat: N/A
-- security/pr-reviewer
Fixes #2252 history.json now uses a versioned envelope: { "version": 1, "records": [...] } This creates a migration escape hatch for future SpawnRecord shape changes. loadHistory() transparently reads both v0 (bare array) and v1 formats, automatically migrating v0 files on next write. All write operations now use writeHistory() to stamp the current schema version consistently. Validation uses valibot schemas (VMConnectionSchema, SpawnRecordSchema, HistoryFileV1Schema) so the structure is verified and typed without `as` casts. Updated all affected tests to check data.records instead of data. Agent: issue-fixer Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const SpawnRecordSchema = v.object({ | ||
| id: v.optional(v.string()), | ||
| agent: v.string(), | ||
| cloud: v.string(), | ||
| timestamp: v.string(), | ||
| name: v.optional(v.string()), | ||
| prompt: v.optional(v.string()), | ||
| connection: v.optional(VMConnectionSchema), | ||
| }); | ||
|
|
||
| /** v1 history file format: { version: 1, records: SpawnRecord[] } */ | ||
| const HistoryFileV1Schema = v.object({ | ||
| version: v.literal(1), | ||
| records: v.array(SpawnRecordSchema), | ||
| }); |
There was a problem hiding this comment.
🚩 valibot v.object() silently strips unknown keys — potential forward-compatibility concern
In valibot v1.2.0, v.object() strips unknown keys from the parsed output (unlike v.looseObject() which preserves them). The SpawnRecordSchema and VMConnectionSchema at packages/cli/src/history.ts:35-61 use v.object(). Currently all fields of SpawnRecord and VMConnection are covered by the schemas, so no data loss occurs. However, if a future CLI version adds a new field to SpawnRecord (e.g., duration) and writes v1 format, then a user running this older version would silently strip that field on the next load-then-write cycle. Using v.looseObject() instead of v.object() for the record schemas would make this forward-compatible without data loss risk.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Summary
versionfield tohistory.jsonusing a new v1 envelope:{ "version": 1, "records": [...] }loadHistory()transparently reads both v0 (bare array) and v1 formats — v0 files are silently migrated to v1 on the next writewriteHistory()helper that stampsHISTORY_SCHEMA_VERSIONconsistentlyVMConnectionSchema,SpawnRecordSchema,HistoryFileV1Schema) validate the file structure withoutascastsdata.recordsinstead ofdata)Why
Without schema versioning, any future rename/add/remove of a field in
SpawnRecordorVMConnectioncauses silent data loss, silent corruption, or confusing empty-history bugs for users who upgrade. The versioned envelope creates the migration escape hatch described in the issue.Migration
loadHistory()Fixes #2252
-- refactor/issue-fixer