Skip to content

feat(config): timestamped config backups with a configurable folder and retention - #661

Merged
Ark0N merged 13 commits into
Ark0N:masterfrom
opticon454:feat/config-backups
Oct 22, 2026
Merged

Ark0N merged 13 commits into
Ark0N:masterfrom
opticon454:feat/config-backups

Conversation

@opticon454

Copy link
Copy Markdown
Contributor

What

Config backups: timestamped snapshots of the files that hold a user's customization, in a default but configurable folder, with configurable retention. Settings → System gains a "Config backups" group (on/off, folder, keep N backups, delete older than N days, Back up now, a list with Restore).

Why

Those files (settings.json, clis.json, custom endpoints, webhook, linked cases, accounts) are plain JSON in the data dir, rewritten by the server, the Settings UI and the CLI, and there was no copy to go back to. A single bad write (a stray test run, a hand edit, a bad restart) silently resets everything, and nothing says what it used to be.

How

  • src/config-backup.ts (pure, takes its directories): a fixed allowlist of tracked files (settings, CLI overrides, custom endpoints, webhook, intents, linked cases, remote and Docker hosts and cases, saved dashboards, push keys, accounts, .env), never "everything in the data dir"; state.json is deliberately not tracked (it is rewritten with every session event and also holds cron jobs and respawn presets); one snapshot per actual content change (a digest of the file set is compared with the latest snapshot's, so an idle server writes nothing); snapshots are built in a temp folder and renamed into place.
  • Retention: keep the newest keepCount (1-500) and, when keepDays > 0, drop older ones. It only ever deletes folders it made (name pattern and a parseable manifest), so pointing the folder at a directory holding other things cannot delete them, and the newest snapshot always survives.
  • Restore: every file is checked against the manifest's SHA-256 before anything is written, a pre-restore snapshot of the live files is taken first, and each file is replaced atomically with mode 0600. Only allowlisted names are ever written, whatever a manifest lists.
  • Snapshots are a 0700 folder of 0600 files: .env, users.json, custom-model-hosts.json and push-keys.json carry credentials. In multi-user mode all three routes are admin only.
  • Settings (synced): configBackupEnabled (absent = on), configBackupDir, configBackupKeepCount (20), configBackupKeepDays (30). The server reads them fresh, so a change applies on the next pass.
  • Routes: GET/POST /api/config-backups, POST /api/config-backups/:id/restore, GET /api/config-backups/diff?from=&to=. A 5-minute server timer plus a snapshot at startup; skipped in test mode.
  • Compare: a backup against the running config, against the backup before it, or any two backups. Each differing file lists its changed keys as before/after (plus files that appeared or vanished); a non-JSON file reports only that it differs, a file is cut at 200 changes, and the self-refreshed discovery timestamp does not count. Credential values are never returned: .env, users.json and push-keys.json show names only, and any other key that looks like a token, secret, password, credential or key is shown as changed without its value. Admin only in multi-user mode, like the other routes.

Tests

test/config-backup.test.ts (settings clamping and defaults, tracked set, no snapshot while unchanged, private modes, both retention limits and the never-delete-foreign-folders and keep-newest rules, restore ordering, checksum refusal, traversal and allowlist, and the diff: key-level changes, backup vs running config, credential redaction, volatile timestamps, non-JSON, the cap, unknown ids and a tampered snapshot) and test/routes/config-backup-routes.test.ts (list, back up now, custom folder and retention, restore with restart hints, diff with its 400/404, 404s, multi-user admin gate). The settings, i18n and docs-parity suites pass. Changeset included (minor: a new feature).

Docs

New wiki page Config-Backups (what is backed up, when, naming, retention, restore, security, troubleshooting), linked from the Home index and sidebar; Settings Reference, Troubleshooting ("my settings were reset"), Security and Core Concepts; README and CLAUDE.md; endpoint reference in docs/api-reference.md and HTTP-API.

@Ark0N

Ark0N commented Oct 18, 2026

Copy link
Copy Markdown
Owner

Thank you @opticon454, this is a really useful feature: on-by-default config backups with change detection, retention that only touches its own folders, a checksum-verified restore with a pre-restore copy, and a key-level compare, all with thorough tests and docs. Typecheck, lint, format and your 38 tests pass here, and CI is green.

A few things need changing before it can go in.

Must fix

  1. The compare endpoint returns two secrets in clear text (src/config-backup.ts:439). Redaction matches key names, and two tracked files keep secrets under keys that do not match: webhook.json stores the webhook URL under url (CLAUDE.md and docs/api-reference.md say that URL is a bearer secret that is never returned), and intents.json holds captured prompts (CLAUDE.md: "prompts carry secrets"; in multi-user mode it holds every user's prompts). Running diffConfigBackups on a changed webhook returned both full Slack URLs. Please add webhook.json and intents.json to REDACT_ALL_FILES and cover both in the "never returns a credential value" test.

  2. Synchronous file IO on the user-chosen backup folder (src/config-backup.ts:216, :276, :279, called from the timer in src/web/server.ts:3082 and from GET /api/config-backups). CLAUDE.md's fix(cases): bound path probes for linked workspaces and session creation, so an unreachable mount cannot freeze the server #516 rule says user-chosen paths are probed bounded, never with existsSync/statSync, because a network mount that stops answering blocks the event loop and freezes the whole server. The wiki page suggests a network share or synced folder, so this case is expected. When settings.dir is set, please await probePathKind(dir, { pastCap: true }) (src/utils/bounded-path-probe.ts) before each pass and in each route, skip the pass or answer OPERATION_FAILED via describeUnknownPath() on unknown, and only create a missing custom folder when its parent is present. The default folder needs no probe.

Should fix in the same round

  1. A restored intents.json is reverted (src/web/routes/config-backup-routes.ts:52). IntentStore caches the file in memory and rewrites it whole on the next captured prompt, so the restore disappears (I reproduced it: 1 prompt after restore, 4 after the next prompt). Please add a reload() to the store and call it after a restore that included the file, or list it in RESTART_REQUIRED.

  2. Two instances sharing one backup folder (src/config-backup.ts:270). The change check compares with the newest snapshot in the folder whoever wrote it, so two instances (a laptop and a desktop on one synced folder, or prod and a beta) each snapshot on every tick with no edits: 48 snapshots in two hours, history gone in about 50 minutes, and Restore offers the other machine's users.json and .env. Please record an origin (data dir plus hostname, or the instance name) in the manifest and scope the change check, prune, list and restore to it, or write into a per-instance subfolder.

Smaller things (I can fix these at merge time if you prefer)

  • docs/wiki/Config-Backups.md:136: restore does not remove files created after the backup (correctly), so "which is also what a restore of it would undo" is not true for added files; fix the sentence and name untouched files in the restore response.
  • src/config-backup.ts:441: strip userinfo and the query from URL values (baseUrl with user:pw@, web tab URLs with tokens).
  • src/web/routes/config-backup-routes.ts:52: restoring settings.json does not re-run the watcher toggles PUT /api/settings drives; say they apply after a restart.
  • src/config-backup.ts:57: remote-cases.json is missing from the list, while the CLAUDE.md paragraph says remote cases are covered.
  • src/web/public/index.html:3245: hide the group for non-admins in multi-user mode, like the Diagnostics group.
  • src/web/public/settings-ui.js:2874: an empty days field saves 0 (no age limit); fall back to 30.
  • src/web/public/settings-ui.js:1373: the MCP sync JSDoc ended up above the new block.
  • Sweep stale .tmp-cfg-* folders at startup and unlink the .restore-<pid> temp on a failed restore.

Once 1 to 4 are in, this is ready to merge. Thanks again for the care that went into the docs and tests.

opticon454 added a commit to opticon454/Codeman that referenced this pull request Oct 18, 2026
… per-instance origin, reload intents on restore

Review round on Ark0N#661: webhook.json and intents.json are redacted in compare and URL values lose userinfo/query; a custom folder is probed (bounded, pastCap) before each pass and in each route and only created when its parent exists; manifests record an origin and change check, prune, list, restore and diff are scoped to it; a restore reloads IntentStore, lists untouched files and treats settings.json as restart-required; remote-cases.json is tracked; stale .tmp-cfg-* folders are swept and a failed restore unlinks its temp; the group is hidden from non-admins; an empty days field saves 30.
@opticon454

Copy link
Copy Markdown
Contributor Author

Thanks @Ark0N, all four required items and the small ones are in ff1d5ae0, rebased on current master.

Must fix

  1. webhook.json and intents.json are now in REDACT_ALL_FILES. The "never returns a credential value" coverage has a new test that changes both files and checks neither URL nor prompt text appears in the diff.
  2. A custom folder is probed with probePathKind(dir, { pastCap: true }) before each timer pass and in each route. unknown skips the pass or answers 503 OPERATION_FAILED via describeUnknownPath(). A missing custom folder is only created when its parent is present (a test checks nothing is created otherwise). The default folder is not probed.

Should fix

  1. IntentStore.reload() is called after a restore that included intents.json. A route test records a prompt, backs up, records another, restores, records a third, and expects 2 prompts rather than the old cache plus one.
  2. Each manifest records an origin (hostname plus data dir). The change check, prune, list, restore and compare are scoped to it, so two instances on one folder do not snapshot each other's state or prune each other's history. A test with two data dirs on one folder covers this.

Smaller things

  • Compare and restore wording fixed. The restore response now has untouched (tracked files present now that the backup did not hold).
  • URL values lose userinfo, query and fragment in the compare.
  • settings.json is now listed in restartRequired, and the wiki says so.
  • remote-cases.json is tracked.
  • The group is hidden for non-admins in multi-user mode.
  • An empty days field now saves 30; an explicit 0 still means no age limit.
  • The MCP sync JSDoc is back above its function.
  • Stale .tmp-cfg-* folders are swept (only ones over an hour old, so a live pass on a shared folder is safe), and a failed restore unlinks its temp file.

Typecheck, lint and the config-backup and route tests pass locally.

@Ark0N

Ark0N commented Oct 18, 2026

Copy link
Copy Markdown
Owner

Thank you @opticon454 for the second round. This PR adds on-by-default, change-detected config backups with retention, a checksum-verified restore and a redacted compare, and every item from my first review is in ff1d5ae0: I checked each one in the code. Typecheck, lint, format, frontend syntax and the full npm test gate pass here, and CI is green.

Two things still need changing, and the first one is my fault.

Must fix

  1. The snapshot origin must not include the hostname (src/config-backup.ts:160). My round-1 suggestion of "data dir plus hostname" was wrong for Docker Compose: docker/docker-compose.yaml sets no hostname:, so the container's hostname is its container ID, and Start-Codeman.sh / Update-Codeman.sh recreate the container on every update. After an update every earlier backup disappears from the list, restore and compare (404), and since prune is origin-scoped those snapshots are never deleted, so the folder grows without bound. I reproduced it: three snapshots, host part changed, 0 listed and 3 on disk. The origin needs to survive a container recreate and still differ between two machines syncing one folder under the same $HOME path. A random id generated once and stored in a small file in the data dir does both (please not the hook secret). Please add a test that a hostname change keeps snapshots listed and prunable, and update the shared-folder test (test/config-backup.test.ts:409), the CLAUDE.md paragraph and docs/wiki/Config-Backups.md:130-132.

  2. Save after Restore writes the old settings back (src/web/public/settings-ui.js:1495). Restore runs from inside App Settings and only refreshes the backup list, so every control keeps its pre-restore value; Save or Apply then PUTs those values over the restored settings.json. When the restore included settings.json, please re-read /api/settings into the form (or close the modal and reload), with a test that the controls are repopulated.

Smaller things (I can do these at merge time if you prefer)

  • src/config-backup.ts:424: restore writes clis.json directly, while CLAUDE.md says only mutateRegistryFile() writes it. Please name restore as the exception in the CLI registry paragraph and call reloadCliRegistry() after restoring it, which also means it no longer needs a restart.
  • src/web/public/settings-ui.js:1485: a restore also rolls back users.json and .env (disabled accounts, passwords, grants). Please name the files about to be replaced in the confirm text, credential files called out.
  • docs/api-reference.md:1157: "all three routes" (four), remote-cases.json missing from the tracked list, "never content" no longer true with the diff, and the restore and POST rows miss untouched, safetySnapshot and removed. The route @fileoverview (config-backup-routes.ts:10) has the same "all three" and an incomplete redaction list.
  • src/web/routes/config-backup-routes.ts:57: users.json is re-read after one second (user-store.ts cache), so it does not need a restart.
  • CLAUDE.md:159: I will add an architecture-invariants section and pointer at merge.

Once 1 and 2 are in, this is ready to merge. Thanks again for the care in the docs and tests.

opticon454 added a commit to opticon454/Codeman that referenced this pull request Oct 18, 2026
…ter a settings restore, clis.json reloads

Review round 2 on Ark0N#661: the hostname in the origin orphaned every snapshot when a Docker container was recreated, so the origin is a random id kept in <data dir>/config-backup-id; after restoring settings.json the Settings form is re-read from /api/settings so Save cannot write the old values back; the restore confirm names the files and the credential ones; clis.json is reloaded (reloadCliRegistry) and users.json needs no restart; API reference and route docs corrected.
@opticon454

Copy link
Copy Markdown
Contributor Author

Thanks @Ark0N, and no harm done on the hostname: the Docker recreate case is a good catch. Both must-fix items and the smaller ones are in the new commit on top of ff1d5ae0.

Must fix

  1. The origin is now a random UUID made once and kept in <data dir>/config-backup-id (0600, its own file, not the hook secret, not tracked or restored). No hostname or path is involved, so it survives a container recreate and differs between two machines on one synced folder. New test/config-backup-origin.test.ts changes the hostname (mocked) and checks that snapshots stay listed, restorable and prunable; the shared-folder test is unchanged because its two data dirs now get two ids. CLAUDE.md and docs/wiki/Config-Backups.md say what the id is and that deleting the file makes the instance a new owner (old snapshots stay on disk, unlisted).
  2. After a restore that included settings.json, Settings re-reads /api/settings into the form (loadAppSettingsFromServer() then openAppSettings()), so Save cannot write the old values back. test/config-backup-restore-ui.test.ts evaluates the real settings-ui.js and checks the form is refilled for a settings.json restore and left alone otherwise.

Smaller things

  • Restore of clis.json is named as the exception in the CLI registry paragraph (CLAUDE.md and architecture-invariants.md), and the route calls reloadCliRegistry() after it. clis.json and users.json are no longer in restartRequired; the remaining ones are .env, push-keys.json and settings.json.
  • The confirm text lists the files about to be replaced and calls out credential files (.env, users.json, push-keys.json, custom-model-hosts.json, webhook.json) as rolling back passwords, accounts and keys.
  • docs/api-reference.md: "all four routes", remote-cases.json added, the content claim corrected for the diff, the origin field, and untouched, safetySnapshot, removed and the 503 on the rows. The route @fileoverview has the same fixes and the full redaction list.
  • Left the architecture-invariants section and the CLAUDE.md pointer to you, as offered.

Typecheck, lint and the config-backup, route and settings-structure tests pass locally.

@Ark0N

Ark0N commented Oct 18, 2026

Copy link
Copy Markdown
Owner

Thank you @opticon454 for the third round. This PR adds on-by-default config backups with change detection, retention, a checksum-verified restore and a redacted compare, and both items from my last review are in c855cac9: the origin is a stored random id, and Settings refills the form after a settings.json restore. Typecheck, lint, format, frontend syntax and the full npm test gate pass here, and CI is green.

Running the retention against a realistic timeline turned up two problems that my earlier reviews missed.

Must fix

  1. Age-based retention deletes the last good copy on the first change after a quiet month (src/config-backup.ts:381). Snapshots are made only on change, so a stable config has one snapshot whose createdAt keeps aging. pruneConfigBackups ages each snapshot by its own createdAt, so once that snapshot is older than keepDays (30 by default), the next change creates a new snapshot at index 0 and the same pass deletes the old one at index 1. I reproduced it with the defaults: one snapshot on day 0, 40 days unchanged, then settings.json overwritten with {}; after the next createConfigBackup plus pruneConfigBackups, the folder holds only the {} snapshot. That is the bad write the feature exists for, and the copy it needed is the one removed. A snapshot's state was live until the next snapshot was made, so please age it from its successor:

    // A snapshot's state was live until the next one was made: age it from then.
    const tooOld = cutoff !== null && all[index - 1].createdAt < cutoff;

    With that change the scenario keeps both snapshots, "drops anything older than keepDays, but never the newest" still passes, and "applies both limits together" (test/config-backup.test.ts:216) removes 1 instead of 2 (T0+1d was live until T0+2d, which sits exactly on the cutoff), so its expectation moves. Please add a test for the stable-then-changed scenario and adjust the retention text in docs/wiki/Config-Backups.md:85-94 to say the age counts from when a backup was replaced.

Should fix in the same round

  1. Three more tracked files rewrite themselves, so the history shrinks to about 95 minutes (src/config-backup.ts:198). This is the same class as the lastDiscoveredAt fix in 8fb42e88:

    • users.json: touchLastLogin() (src/user-store.ts:389, called on every Basic auth in src/web/middleware/auth.ts:591) rewrites lastLoginAt up to once a minute per user, so in multi-user mode any Basic client (TUI, codeman agent, the MCP server) changes the file every minute. I ran 30 five-minute passes where only lastLoginAt changed: 30 snapshots were created, and the 20 kept cover 95 minutes.
    • settings.json: every session start in a different case than the last one rewrites lastUsedCase (src/web/routes/session-routes.ts:4534, src/web/routes/ralph-routes.ts:479), in single-user mode too.
    • intents.json: with Read My Mind on, every captured prompt rewrites recentPrompts and updatedAt (src/intent-store.ts:146).

    Please add 'users.json': ['lastLoginAt'], 'settings.json': ['lastUsedCase'] and 'intents.json': ['recentPrompts', 'updatedAt'] to VOLATILE_KEYS (dropping intents.json from the tracked list instead is fine too, since it is rewritten per event like state.json), with a test per file like the existing discovery-timestamp one.

Smaller things (I can fix these at merge time if you prefer)

  • docs/wiki/Config-Backups.md:123-126: the Restoring paragraph says clis.json and users.json need a restart and then that they do not. Please align it with RESTART_REQUIRED (.env, push-keys.json, settings.json).
  • src/web/schemas.ts:1533 and src/web/public/index.html:3265: with "Keep this many backups" at 1, every change deletes the state before it, so a bad write can never be undone. A minimum of 2, or a sentence in the row description, closes that.
  • CLAUDE.md:307: the State Files list should name config-backup-id and backups/config/. I will fold that into the architecture-invariants section I am adding at merge.

Once 1 and 2 are in, this is ready to merge. Thanks again for the care in the docs and tests, and for turning each round around so quickly.

opticon454 added a commit to opticon454/Codeman that referenced this pull request Oct 18, 2026
…y-only rewrites, keep at least 2

Review round 3 on Ark0N#661: retention aged each snapshot by its own createdAt, so the first change after a quiet month deleted the last good copy; it now ages from when the next snapshot was made. users.json lastLoginAt, settings.json lastUsedCase and intents.json recentPrompts/updatedAt are volatile keys for the change check. The keep count has a minimum of 2. Wiki restore paragraph aligned with RESTART_REQUIRED.
@opticon454

Copy link
Copy Markdown
Contributor Author

Thanks @Ark0N, both items and the smaller ones are in the new commit on top of c855cac9.

Must fix

  1. Retention now ages a snapshot from its successor: tooOld = cutoff !== null && all[index - 1].createdAt < cutoff. New test: one snapshot, 40 days unchanged, then settings.json overwritten with {}; after the next snapshot and prune both are kept. The "both limits together" expectation moved to 1 removed, as you worked out, and the wiki's retention text now says age counts from when a backup was replaced.

Should fix

  1. VOLATILE_KEYS gains users.json: ['lastLoginAt'], settings.json: ['lastUsedCase'] and intents.json: ['recentPrompts', 'updatedAt'] (I kept intents.json tracked, since its goals are worth a copy). One parametrised test covers each file: an activity-only rewrite is unchanged, an edit next to it creates a snapshot.

Smaller things

  • The Restoring paragraph now agrees with RESTART_REQUIRED (.env, push-keys.json, settings.json).
  • Keep count has a minimum of 2 in readConfigBackupSettings, the schema, the UI clamp and the input (min="2", with a sentence in the row description), plus the wiki, CLAUDE.md and API reference.
  • Left the State Files list for your architecture-invariants section, as offered.

Typecheck, lint and the config-backup, route, settings-structure and schema tests pass locally.

@Ark0N

Ark0N commented Oct 20, 2026

Copy link
Copy Markdown
Owner

Thank you @opticon454 for the fourth round. This PR adds on-by-default config backups with change detection, retention, a checksum-verified restore and a redacted compare, and every item from my last review is in acb2fea4: retention now ages a snapshot from its successor (I walked it through origin scoping, forced snapshots and restore, and it holds), the three activity-only keys are ignored with a test each, and the keep count starts at 2. Typecheck, lint, format, frontend syntax and the config-backup tests pass here.

One more file of the same class turned up, plus some doc lines this round left behind.

Should fix

  1. docker-cases.json rewrites itself on every conversation change (src/config-backup.ts:205). persistDockerCaseClaudeSessionId() (src/docker-hosts.ts:143) writes the case's lastClaudeSessionId whenever a Docker Claude session switches conversation, which is every /clear (from hook-event-routes.ts:161 and the last-response route in session-routes.ts:2871/:2891) and every quick-start. A Docker case on a respawn cycle therefore makes one snapshot per tick: with 30 ticks each carrying a new id, 30 snapshots were created and the 20 kept covered 95 minutes, the same shrinkage as users.json last round. The fix is one line in VOLATILE_KEYS:

    'docker-cases.json': ['lastClaudeSessionId'],

    plus a row in the it.each table at test/config-backup.test.ts:150:

    ['docker-cases.json', (n: number) => JSON.stringify([{ name: 'app', hostId: 'h', lastClaudeSessionId: `c${n}` }]), 'hostId'],

    and "a Docker case's conversation id after a /clear" in the comment above the new keys.

Doc lines

  • docs/wiki/Config-Backups.md:39-43 and :159: both still name only lastDiscoveredAt as ignored. Please list the others (users.json lastLoginAt, settings.json lastUsedCase, intents.json recentPrompts/updatedAt, docker-cases.json lastClaudeSessionId) and add that a restore still puts those fields back as they were (Compare shows intents.json as unchanged when only prompts differ, yet a restore drops the prompts captured since).
  • docs/wiki/Troubleshooting.md:142-143: the restart list is clis.json, users.json, .env, push-keys.json; RESTART_REQUIRED is .env, push-keys.json, settings.json. Pointing at the response's restartRequired list keeps it from drifting again.
  • README.md:482 and docs/wiki/Settings-Reference.md:195 still give the keep count as 1 to 500.
  • src/web/public/index.html:3271-3272: "Delete backups older than (days)" now means "removed once a newer backup has existed this many days"; the row description could say so, as the wiki table does.
  • docs/wiki/Security.md:110-111: add webhook.json to the credential-bearing files, as the Config-Backups page and the restore confirm text already do.

These are small enough that I will apply them on top at merge, together with the architecture-invariants section and the State Files entries I mentioned before, unless you would rather push them yourself first. Thanks again for the care in the docs and tests, and for turning every round around so quickly.

@opticon454

Copy link
Copy Markdown
Contributor Author

Thanks @Ark0N, the Docker case file and the doc lines are in the new commit (rebased on current master first).

  • VOLATILE_KEYS gains 'docker-cases.json': ['lastClaudeSessionId'], with the comment updated and your row in the it.each table.
  • Config-Backups.md now lists every ignored key (lastDiscoveredAt, lastLoginAt, lastUsedCase, recentPrompts/updatedAt, lastClaudeSessionId), says a restore puts them back as they were, and that Compare shows intents.json unchanged when only prompts differ. The Comparing section points at that list.
  • Troubleshooting.md points at the response's restartRequired list.
  • The keep count reads 2 to 500 in README.md and Settings-Reference.md.
  • The "Delete backups older than" row description says a backup is removed once a newer one has existed that many days.
  • Security.md names webhook.json among the credential-bearing files.

Typecheck, and the config-backup and route tests pass locally. The architecture-invariants section and State Files entries stay with you, as offered.

opticon454 added a commit to opticon454/Codeman that referenced this pull request Oct 20, 2026
… per-instance origin, reload intents on restore

Review round on Ark0N#661: webhook.json and intents.json are redacted in compare and URL values lose userinfo/query; a custom folder is probed (bounded, pastCap) before each pass and in each route and only created when its parent exists; manifests record an origin and change check, prune, list, restore and diff are scoped to it; a restore reloads IntentStore, lists untouched files and treats settings.json as restart-required; remote-cases.json is tracked; stale .tmp-cfg-* folders are swept and a failed restore unlinks its temp; the group is hidden from non-admins; an empty days field saves 30.
opticon454 added a commit to opticon454/Codeman that referenced this pull request Oct 20, 2026
…ter a settings restore, clis.json reloads

Review round 2 on Ark0N#661: the hostname in the origin orphaned every snapshot when a Docker container was recreated, so the origin is a random id kept in <data dir>/config-backup-id; after restoring settings.json the Settings form is re-read from /api/settings so Save cannot write the old values back; the restore confirm names the files and the credential ones; clis.json is reloaded (reloadCliRegistry) and users.json needs no restart; API reference and route docs corrected.
opticon454 added a commit to opticon454/Codeman that referenced this pull request Oct 20, 2026
…y-only rewrites, keep at least 2

Review round 3 on Ark0N#661: retention aged each snapshot by its own createdAt, so the first change after a quiet month deleted the last good copy; it now ages from when the next snapshot was made. users.json lastLoginAt, settings.json lastUsedCase and intents.json recentPrompts/updatedAt are volatile keys for the change check. The keep count has a minimum of 2. Wiki restore paragraph aligned with RESTART_REQUIRED.
@Ark0N

Ark0N commented Oct 20, 2026

Copy link
Copy Markdown
Owner

Thank you @opticon454 for the fifth round. This PR adds on-by-default config backups with change detection, retention, a checksum-verified restore and a redacted compare, and every item from my last review is in d4de3ded: the Docker case conversation id is ignored with its test row, the wiki lists every ignored key, and the restart list, keep-count range, days description and Security page are fixed. Typecheck, lint, format, frontend syntax and the config-backup and settings tests pass here, and CI is green.

One more file of the same class turned up, plus a few smaller things.

Should fix

  1. webviews.json rewrites itself whenever a web tab is opened (src/config-backup.ts:213). POST /api/webviews/:id/open (src/web/routes/webview-routes.ts:275) writes lastOpenedAt on every call, and the browser calls it on every web-tab click, on Alt+N onto a web tab, and when a frame recovers itself. Opening a dashboard tab once per five-minute tick for 30 ticks created 30 snapshots, and the 20 kept covered 95 minutes, the same shrinkage as users.json and docker-cases.json. The fix is one line in VOLATILE_KEYS:

    'webviews.json': ['lastOpenedAt'],

    a row in the it.each table at test/config-backup.test.ts:150:

    ['webviews.json', (n: number) => JSON.stringify([{ id: 'w1', name: 'Grafana', url: 'http://localhost:3001', embedMode: 'proxy', trusted: false, createdAt: 1, lastOpenedAt: n }]), 'name'],

    and webviews.json lastOpenedAt (opening a web tab) in the ignored-keys list at docs/wiki/Config-Backups.md:40-48 and in the comment above the keys. With that line the same 30 opens make one snapshot, and an edit next to the timestamp still makes a new one.

Smaller things

  1. src/web/routes/config-backup-routes.ts:205: in multi-user mode a restore that includes users.json replaces accounts, password hashes, roles and canBypassPermissions grants, but writes nothing to admin-audit.jsonl, which src/web/admin-audit.ts says records every user-management action. Please add void appendAdminAudit({ admin: getAuthUser(req).username, action: 'config_backup_restore', target: id, ip: req.ip, detail: { restored: out.restored } }) after a successful restore when isMultiUserMode().
  2. src/config-backup.ts:440-450: restore copies a snapshot file into the data dir as 0600 whatever its own mode, and readFileSync follows a symlink. isUnsafePermissions() (src/config/cli-registry/registry.ts:95-107) refuses a clis.json with any group/world bit because that file selects the binaries Codeman spawns, and its comment says rewriting such a file as 0600 would turn it into trusted config. This module always writes snapshot files 0600, so please lstat each file (and the snapshot folder) before reading it and refuse the restore with ConfigBackupCorruptError for a symlink or a mode with 0o077 bits (POSIX only, like isUnsafePermissions). Please also add a sentence to the wiki's Security section: anyone who can write to a custom backup folder can plant a backup that Restore applies.
  3. src/config-backup.ts:465: restoring a settings.json from before a folder change also restores the old configBackupDir, so the list then shows the other folder and the pre-restore copy just made is not in it. Keeping the four configBackup* keys at their current values on a settings restore covers it; a line in the wiki's Restoring section is fine too.

I will fold these into the merge together with the architecture-invariants section and the State Files entries (config-backup-id, backups/config/) I mentioned before, unless you would rather push them yourself first. Thanks again for the care in the docs and tests, and for turning every round around so quickly.

@opticon454

Copy link
Copy Markdown
Contributor Author

Thanks @Ark0N, all four items are in the new commit (on top of current master).

  1. VOLATILE_KEYS gains 'webviews.json': ['lastOpenedAt'], with the comment updated, your row in the it.each table, and lastOpenedAt added to the ignored-keys list in Config-Backups.md.
  2. In multi-user mode a successful restore calls appendAdminAudit({ admin, action: 'config_backup_restore', target: id, ip, detail: { restored } }). New route test: an admin restore writes that line to admin-audit.jsonl.
  3. Restore now lstats the snapshot folder and each file before reading it and throws ConfigBackupCorruptError for a symlink or any 0o077 bit (POSIX only, like isUnsafePermissions), before anything is written or a pre-restore copy is taken. New tests: a 0644 file, a symlinked file and a 0777 folder are refused, and the folder restores once it is 0700 again. The wiki's Restoring section says what it refuses, and the Security section has the sentence about anyone who can write to a custom folder planting a backup.
  4. Restoring settings.json keeps the four configBackup* keys at their current values (and drops one the live file does not have). When none of the four differ the bytes are written unchanged. New test with a changed folder and retention. A line in the Restoring section says so.

The architecture-invariants section and State Files entries stay with you, as offered. Typecheck, lint and the config-backup and route tests pass locally (58).

@Ark0N

Ark0N commented Oct 20, 2026

Copy link
Copy Markdown
Owner

Thank you @opticon454 for the sixth round. This PR adds on-by-default config backups with change detection, retention, a checksum-verified restore and a redacted compare, and every item from my last review is in 8d4c42ac: webviews.json lastOpenedAt is ignored with its test row, a multi-user restore writes config_backup_restore to the admin audit log, restore refuses symlinked or loosened snapshots, and a settings restore keeps the four backup settings. Typecheck, lint, format, frontend syntax and the full npm test gate pass here, and CI is green.

Two things still need changing.

Must fix

  1. Retention falls back to the defaults when settings.json does not parse or was reset (src/web/routes/config-backup-routes.ts:104). runConfigBackup reads its settings through readJsonConfig, which answers {} for any read failure, including a syntax error and a read that lands inside the non-atomic write in PUT /api/settings (src/web/routes/system-routes.ts:1088). The pass then prunes with keepCount 20 and keepDays 30. I reproduced it: with configBackupKeepCount: 100 and configBackupKeepDays: 0, 30 snapshots, and one pass after a trailing comma was added to settings.json, 20 were left; the same happens after settings.json is reset to {}. With keepDays 0 on a long-lived install, every snapshot whose successor is older than 30 days goes in that one pass. The newest 20 survive, but this is the longer history the user asked to keep, lost on exactly the events the feature exists for. Please:

    • read settings.json directly in runConfigBackup, and when it exists but does not parse, still snapshot but skip pruneConfigBackups for that pass;
    • record keepCount and keepDays in each manifest, and when the parsed file has none of the four configBackup* keys, prune with the newest manifest's values instead of the defaults (the Settings form always sends all four, so a real change still applies);
    • add tests for both scenarios (expecting 31 kept) and one with aged manifests and keepDays: 0.
  2. Restore refuses every backup in a folder on a filesystem without POSIX modes (src/config-backup.ts:428, applied to the folder at :480). vfat and exFAT drives report 0755 for every entry with the usual mount options, CIFS shares default to 0755, and WSL /mnt/d without the metadata option reports 0777, so the 0o077 test fails on all of them and chmod cannot fix it. Snapshots still succeed there (createConfigBackup already treats mode-less filesystems as expected at :349-353), so the user finds out on the day they need Restore. The wiki suggests these places (docs/wiki/Config-Backups.md:183-185 and :219-221). Please probe whether the backup folder keeps modes before the test (write a 0600 temp file in the backup folder, lstat it, remove it), skip the 0o077 test when the mode does not stick while keeping the symlink test, add a sentence to the Restoring section (docs/wiki/Config-Backups.md:125-128), and add a test.

Smaller things (I can fix these at merge time if you prefer)

  • src/config-backup.ts:671: Compare reads snapshot files without the symlink check Restore now applies; please use the same check per file there.
  • docs/wiki/Troubleshooting.md:134: a reset settings.json also drops the backup folder setting, so say to enter a custom folder again and save before looking for the list. And after a reinstall the old snapshots are unlisted because the instance has a new config-backup-id (docs/wiki/Config-Backups.md:146-150): add that copying the origin from any of their manifest.json files into <data dir>/config-backup-id makes them visible again.
  • docs/wiki/Config-Backups.md:93 says "Three guarantees" above four bullets, and the Security list at :180-181 should name webhook.json, as Security.md and the restore confirm text do.
  • src/web/routes/config-backup-routes.ts:150: the first GET /api/config-backups creates config-backup-id through configBackupOrigin; a list route that returns an empty list when the file is absent would never write.

Once 1 and 2 are in, this is ready to merge, and I will add the architecture-invariants section and the State Files entries (config-backup-id, backups/config/) at merge as planned. Thanks again for the care in the docs and tests, and for turning every round around so quickly.

claude and others added 10 commits October 20, 2026 18:01
…nd retention

The files that hold a user's customization (settings.json, clis.json, custom-model-hosts.json,
webhook.json, intents.json, linked-cases.json, push-keys.json, users.json, .env) are plain JSON
in the data dir, rewritten by the server, the Settings UI and the CLI, with no copy to go back to.

A snapshot is taken at startup and whenever the tracked files' content changes (checked every
five minutes, and only written when a digest of the file set differs from the latest snapshot's),
into `<data dir>/backups/config` by default or any absolute folder. Retention keeps the newest
configBackupKeepCount (1-500) and, when configBackupKeepDays > 0, drops older ones; it only ever
deletes folders it made (name pattern and a parseable manifest) and never the newest.

GET/POST /api/config-backups list and back up now; POST .../:id/restore verifies every file against
the manifest's SHA-256, takes a pre-restore snapshot, then replaces files atomically with mode
0600, writing only allowlisted names. Snapshots are 0700 folders of 0600 files and all routes are
admin only in multi-user mode. Settings -> System gets the controls and a Restore list.
…remote/docker/webview files

- New wiki page Config-Backups: what is backed up (and what is not, and why), when a backup is made,
  where and under what name (cfg-YYYYMMDD-HHMMSS, manifest.json, reasons), retention and its
  guarantees, the Settings controls, restore (verify, pre-restore copy, atomic write, which files
  need a restart), security, the HTTP API and troubleshooting.
- Linked from the Home index and sidebar; Settings Reference, Troubleshooting ("my settings were
  reset"), Security and Core Concepts updated; README and CLAUDE.md entries; the api-reference
  section now states the naming convention and the full tracked list.
- Found while documenting: remote-hosts.json, docker-hosts.json, docker-cases.json and
  webviews.json are user configuration too and were not tracked; they are now. state.json stays
  out on purpose (rewritten per session event, also holds cron jobs and respawn presets) and the
  docs say so.
custom-model-hosts.json carries a lastDiscoveredAt per host that the 5-minute model
discovery poll rewrites, so the content hash never matched the latest snapshot and a
snapshot was written every tick. At the default keep count of 20 that left about 100
minutes of history and evicted every useful older copy.

The change check now looks through volatile keys per tracked file. The snapshot still
stores the file as written and restore still verifies against its own digest.
…ning config

Settings -> System -> Config backups gains a Compare view: a backup against the
running config, a backup against the one before it, or any two backups. Each
differing file lists its changed keys as before/after, plus files that appeared
or vanished. GET /api/config-backups/diff?from=&to= serves it (`to` defaults to
`current`; admin only in multi-user mode).

Credential values are never returned: .env, users.json and push-keys.json report
names only, and any other key that looks like a token, secret, password,
credential or key is reported as changed without its value. A non-JSON file
reports only that it differs, a file is cut at 200 changes, and a self-refreshed
discovery timestamp does not count as a difference.
… per-instance origin, reload intents on restore

Review round on Ark0N#661: webhook.json and intents.json are redacted in compare and URL values lose userinfo/query; a custom folder is probed (bounded, pastCap) before each pass and in each route and only created when its parent exists; manifests record an origin and change check, prune, list, restore and diff are scoped to it; a restore reloads IntentStore, lists untouched files and treats settings.json as restart-required; remote-cases.json is tracked; stale .tmp-cfg-* folders are swept and a failed restore unlinks its temp; the group is hidden from non-admins; an empty days field saves 30.
…ter a settings restore, clis.json reloads

Review round 2 on Ark0N#661: the hostname in the origin orphaned every snapshot when a Docker container was recreated, so the origin is a random id kept in <data dir>/config-backup-id; after restoring settings.json the Settings form is re-read from /api/settings so Save cannot write the old values back; the restore confirm names the files and the credential ones; clis.json is reloaded (reloadCliRegistry) and users.json needs no restart; API reference and route docs corrected.
…y-only rewrites, keep at least 2

Review round 3 on Ark0N#661: retention aged each snapshot by its own createdAt, so the first change after a quiet month deleted the last good copy; it now ages from when the next snapshot was made. users.json lastLoginAt, settings.json lastUsedCase and intents.json recentPrompts/updatedAt are volatile keys for the change check. The keep count has a minimum of 2. Wiki restore paragraph aligned with RESTART_REQUIRED.
…y ignored key, correct the retention and restart docs
… restores, refuse symlinked or loosened snapshots, keep the folder settings on restore
…s.json, restore works on mode-less filesystems, compare checks snapshots too
@opticon454

Copy link
Copy Markdown
Contributor Author

Thanks @Ark0N, both must-fix items and the smaller ones are in the new commit (rebased on current master first).

Must fix

  1. Retention. runConfigBackup now reads settings.json directly instead of through readJsonConfig. If the file exists but does not parse (or is not readable), the pass still takes its snapshot but skips pruneConfigBackups. Each manifest now records keepCount and keepDays; when the parsed file has none of the four configBackup* keys (a reset file), the pass uses the newest manifest's values instead of the defaults, for both the new snapshot and the prune. A file that has any of the keys is used as written, so a real change from the Settings form still applies. New route tests with 30 snapshots at keepCount 100 and keepDays 0: an unparseable file leaves 31 and removes none; a {} file leaves 31 (the snapshots are a year old, so the 30 day default would have dropped them); a file with keepCount 5 still prunes to 5.
  2. Mode-less filesystems. Restore and Compare probe the backup folder first (a 0600 temp file, lstat, removed) and skip the 0o077 test when the mode does not stick; the symlink test always runs. If the probe cannot write the temp file the strict test stays on. restoreConfigBackup takes a keepsModes override so the tests can stand in for vfat/CIFS: a 0644 file restores, and a symlink is still refused. The Restoring section of the wiki says so.

Smaller things

  • Compare checks the snapshot folder and each file the way Restore does, with a test.
  • Troubleshooting.md says a reset settings.json drops the custom folder setting (enter it again and save), and that after a reinstall the old backups are unlisted until the origin from one of their manifest.json files is copied into <data dir>/config-backup-id; Config-Backups.md has the same copy instruction.
  • "Four guarantees", and webhook.json in the Security list.
  • GET /api/config-backups uses a new read-only readConfigBackupOrigin, so listing never writes config-backup-id (a test removes the file, lists, and checks it is still absent). Restore and Compare use it too, so with no id they answer 404.

Typecheck, lint and the config-backup and route tests (64) pass locally.

@Ark0N

Ark0N commented Oct 20, 2026

Copy link
Copy Markdown
Owner

Thank you @opticon454 for the seventh round. This PR adds on-by-default config backups with change detection, retention, a checksum-verified restore and a redacted compare, and every item from my last review is in c982fa1c: an unparseable settings.json prunes nothing, a reset one prunes with the retention the newest backup recorded, restore and compare work on mode-less filesystems, compare checks snapshots the way restore does, and listing never writes config-backup-id. Typecheck, lint, format, frontend syntax and the full npm test gate pass here, and CI is green.

Two things still need changing.

Must fix

  1. A restore after a reset still prunes with the defaults (src/config-backup.ts:504 and :559). The reset fix holds on its own, but the next thing the Troubleshooting page tells the user to do is Restore, and that undoes it. I reproduced it: 30 backups a day apart from 2025 under configBackupKeepCount: 100 and configBackupKeepDays: 0, settings.json reset to {}, one pass (31 kept, as intended), then Restore of a pre-reset backup, then one pass: 29 removed, 4 left. Two causes:

    • keepCurrentBackupSettings deletes each configBackup* key from the restored file when the live file lacks it, and a reset file lacks all four, so the restored settings.json comes back without the retention.
    • The pre-restore snapshot is made without keepCount/keepDays, it is then the newest, and latestBackupRetention() (:406) reads only the newest manifest, so runConfigBackup (src/web/routes/config-backup-routes.ts:131) falls back to 20 copies and 30 days.

    Please (a) in keepCurrentBackupSettings, when the live file has none of the four keys (hasConfigBackupSettings(live) is false), keep the backup's configBackupKeepCount and configBackupKeepDays (the folder and on/off can stay as now, so the pre-restore copy stays listed); (b) make latestBackupRetention() return the newest manifest that recorded a retention instead of only the newest, and record the retention in force on the pre-restore snapshot too; (c) add a route test: reset, pass, restore a pre-reset backup, pass, expect nothing removed. docs/wiki/Config-Backups.md:139-141 and :192-194 then need a line each.

  2. The mode probe writes through a planted symlink (src/config-backup.ts:482, reached from GET /api/config-backups/diff at :731 and from restore at :542). writeFileSync(probe, '', { mode: 0o600 }) truncates whatever .mode-probe-<pid> points at, chmodSync then sets that target to 0600, and the finally removes the link, so nothing shows it happened. I reproduced it: a symlink named with the server's pid, pointing at a file outside the backup folder, then one Compare, and the file was empty and 0600. That is the planted-file threat the round-6 symlink refusal covers (the wiki's Security section names it), and here it needs only a Compare click, no confirm. A planted directory of that name also makes the finally throw ERR_FS_EISDIR, so every Compare and Restore fails. Please create the probe with flag: 'wx' and a random name (.mode-probe-${randomUUID()}), remove it only when the write succeeded, and add a test with a planted symlink that checks the target is untouched.

Smaller things (I can fix these at merge time if you prefer)

  • src/web/routes/config-backup-routes.ts:74 and src/web/public/settings-ui.js:557-558: after a reset, GET /api/config-backups reports keepCount: 20, keepDays: 30 while the pass prunes with the recorded 100/0, and the form fills with ?? 20 / ?? 30. Any Save after a reset (docs/wiki/Troubleshooting.md:145 tells custom-folder users to re-enter the folder and save) then writes 20/30, and the next pass prunes with them. Please resolve the settings the GET reports through the same fallback runConfigBackup uses (one shared helper), and fill the two retention inputs from it when settings.json has none of the keys.
  • src/web/routes/config-backup-routes.ts:124-126: while settings.json does not parse, the pass reads it as {}, so it snapshots into the default folder even with a custom folder set or configBackupEnabled: false. Nothing widens (same modes, inside the data dir), but the "Retention survives a bad settings.json" bullet in the wiki can say where that snapshot goes.

Once 1 and 2 are in, this is ready to merge, and I will add the architecture-invariants section and the State Files entries (config-backup-id, backups/config/) at merge as planned, including the retention recorded per manifest. Thanks again for the care in the docs and tests, and for turning every round around so quickly.

…mode probe cannot write through a planted symlink, the form shows the retention in use
@opticon454

Copy link
Copy Markdown
Contributor Author

Thanks @Ark0N, both must-fix items and the smaller ones are in the new commit.

Must fix

  1. A restore after a reset.
    • (a) keepCurrentBackupSettings no longer deletes configBackupKeepCount and configBackupKeepDays when the live file has none of the four keys (hasConfigBackupSettings(live) is false): the backup's own retention stays. The folder and on/off still follow the live file, so the pre-restore copy stays listed.
    • (b) latestBackupRetention() returns the newest manifest that recorded a retention, not just the newest manifest, and the pre-restore snapshot records the retention in force.
    • (c) New route test, run with and without the four keys in the backups: 30 backups a day apart from 2025 under 100 / 0, settings.json reset to {}, a pass (nothing removed), restore of the oldest backup, a pass (nothing removed, 32 or more kept). It also checks that GET /api/config-backups reports 100 / 0. Unit tests cover the settings merge and the recorded retention on the pre-restore copy. Two lines in Config-Backups.md (the Restoring section and the retention bullet).
  2. The mode probe. It now creates .mode-probe-${randomUUID()} with flag: 'wx' and removes it only when its own write succeeded. New test: a symlink named .mode-probe-<pid> pointing at a file outside the folder and a planted directory of another probe-style name, then a Restore and a Compare: the target keeps its content and its 0644 mode and neither call fails.

Smaller things

  • withRecordedRetention() is the one fallback used by both runConfigBackup and what GET /api/config-backups reports (via readEffectiveBackupSettings). loadConfigBackups in settings-ui.js fills the two retention inputs from the GET result when the saved settings have none of the keys (with a test), so a Save after a reset no longer writes 20 / 30.
  • The wiki's retention bullet now says that while settings.json does not parse the pass reads it as empty and snapshots into the default folder, even with a custom folder or backups switched off, and that nothing widens.

Typecheck, lint and the config-backup and route tests (71) pass locally.

@Ark0N

Ark0N commented Oct 20, 2026

Copy link
Copy Markdown
Owner

Thank you @opticon454 for the eighth round. This PR adds on-by-default config backups with change detection, retention, a checksum-verified restore and a redacted compare, and every item from my last review is in 241886d4: a restore after a reset keeps the retention, the pre-restore copy records it, the mode probe uses a random name with wx, and the GET and the form report the retention the passes use. Typecheck, lint, format, frontend syntax and the config-backup tests pass here, and CI is green.

Two edges still cost the backup history.

Should fix

  1. After a reset, re-entering a custom folder saves 20 copies and 30 days (src/web/routes/config-backup-routes.ts:78). A reset settings.json has no configBackupDir, so withRecordedRetention (src/config-backup.ts:428) looks in the default folder, which holds none of a custom-folder user's manifests (only the first post-reset snapshot, which recorded the defaults), and GET /api/config-backups reports 20 / 30. docs/wiki/Troubleshooting.md:145 tells those users to enter the folder again and save. In a browser with no stored copy of the keys (another device, cleared storage), the form fills the retention from that GET (src/web/public/settings-ui.js:1396), Save sends all four keys, and the next pass prunes the custom folder with 20 / 30. I reproduced it: 30 backups a day apart from 2025 in a custom folder under 100 / 0, settings.json reset to {}, one pass, then the folder saved with the reported retention, then one pass: 29 removed, 2 left. A later restore keeps the live 20 / 30, so it does not undo it. Please make the fallback follow the folder the user re-enters. One way: the form sends configBackupKeepCount and configBackupKeepDays only when they were saved or edited, and withRecordedRetention checks only those two keys, so the first pass in the re-entered folder finds the 100 / 0 its manifests recorded. Please add a route test with a custom folder, and adjust docs/wiki/Config-Backups.md:198 ("Settings shows those values until you save") to match.

  2. One snapshot dated ahead of the clock makes every pass write a copy (src/config-backup.ts:350). listConfigBackups orders by createdAt and the change check compares only with existing[0]. A snapshot written while the clock is ahead (a dual-boot machine whose clock reads hours ahead until NTP corrects it, a VM resumed with a stale clock) stays at index 0 after the correction, so every pass differs from it and writes a new snapshot dated below it until real time catches up. I reproduced it: 20 changes a day apart, one change snapshotted with the clock 2 hours ahead, the clock corrected, one more change, then 24 passes with nothing changing: 24 snapshots made and all 20 earlier backups removed. Dating a new snapshot no earlier than the newest one fixes it (I checked this on a patched copy: the same run makes 1 snapshot):

    const createdAt = existing[0] ? Math.max(now, existing[0].createdAt + 1) : now;

    used for createdAt and for the folder stamp, plus a test for the scenario.

Smaller things (I can fix these at merge time if you prefer)

  • docs/api-reference.md:1161 and docs/wiki/Config-Backups.md:64: the manifest field list misses hash, origin (wiki), keepCount and keepDays; the GET, diff and restore rows do not mention the 503 for a custom folder that does not answer, and the 409 also covers a symlinked or group/world-accessible backup.

Once 1 and 2 are in, this is ready to merge, and I will add the architecture-invariants section and the State Files entries (config-backup-id, backups/config/, the retention recorded per manifest) at merge as planned. Thanks again for the care in the docs and tests, and for turning every round around so quickly.

…k-ahead snapshot cannot make every pass write a copy
@opticon454

Copy link
Copy Markdown
Contributor Author

Thanks @Ark0N, both items and the manifest/API doc point are in the new commit.

  1. The fallback follows the folder.
    • withRecordedRetention now decides per retention key (configBackupKeepCount, configBackupKeepDays) and ignores the folder and on/off keys. After a reset, once the user enters the custom folder again, the first pass finds that folder's own manifests, so it uses their 100 / 0.
    • The Settings form sends a retention value only when it was saved before or edited there: saveAppSettings spreads each key in conditionally, and the two inputs get oninput="this.dataset.touched=1". The values displayed from the GET after a reset are therefore never written back.
    • The restore merge uses the same per-key rule (a live file lacking a retention key keeps the backup's value).
    • New route tests: your custom-folder scenario (30 backups in the folder under 100 / 0, reset, a pass, the folder entered again with no retention keys, GET reports 100 / 0, the next pass removes nothing) and a file with only configBackupKeepDays set, which falls back to the recorded count for the other key. The wiki bullet now says what Save sends.
  2. A snapshot dated ahead of the clock. createConfigBackup dates a new snapshot Math.max(now, existing[0].createdAt + 1), for both createdAt and the folder stamp. New test: a snapshot 2 hours ahead, the clock corrected, then 24 passes with nothing changing make no snapshots (3 in the folder at the end).

The smaller point: the manifest field lists in api-reference.md and the wiki now include hash, origin, keepCount and keepDays, and the API reference says all four routes can answer 503 for a custom folder that does not respond and diff and restore answer 409 for a symlinked or group/world-accessible backup too.

Typecheck, lint and the config-backup and route tests (74) pass locally.

@Ark0N

Ark0N commented Oct 20, 2026

Copy link
Copy Markdown
Owner

Thank you @opticon454 for the ninth round. This PR adds on-by-default config backups with change detection, retention, a checksum-verified restore and a redacted compare, and both items from my last review are in 9c0fc309: the retention fallback now follows the folder the user enters again, and a snapshot is never dated earlier than the newest one. Typecheck, lint, format, frontend syntax and the config-backup tests pass here, and CI is green.

One thing still needs changing, and it comes from the last round's fix.

Must fix

  1. Three routes read a custom backup folder before the bounded probe (src/web/routes/config-backup-routes.ts:78). readEffectiveBackupSettings() calls withRecordedRetention(). Whenever settings.json lacks configBackupKeepCount or configBackupKeepDays, that runs latestBackupRetention() → listConfigBackups(dir): a readdirSync of the custom folder plus a readFileSync per manifest. GET /api/config-backups (:161, probe at :166), the diff (:204/:205) and restore (:232/:233) all call it before probeBackupDir(). Before 9c0fc309 the "any of the four keys" check returned early for a custom-folder user. Now the check is per retention key, and the form (src/web/public/settings-ui.js:2892-2897) no longer sends retention it only displayed, so setting just a folder writes exactly the shape that triggers the read. I reproduced it: with probePathKind mocked to 'unknown' and settings.json holding only configBackupDir, each of the three routes called readdirSync on the folder once before answering 503 (zero times with the retention keys present). On a share that stopped answering, that read freezes the server, and App Settings calls the GET every time it opens (settings-ui.js:559). runConfigBackup is ordered correctly. Please:
    • probe inside readEffectiveBackupSettings before withRecordedRetention, returning whether the folder exists so the GET skips the fallback and the listing when it does not (and move that call inside the GET's try);
    • add a route test: probePathKind mocked to 'unknown', settings.json with only configBackupDir, and each of the three routes answers 503 without touching the folder (test/case-path-unreachable.test.ts has a recorder of that kind).

Smaller things (I can fix these at merge time if you prefer)

  • src/web/public/settings-ui.js:2892: nothing tests that Save leaves out a retention value that was only displayed; the route test writes settings.json directly (test/routes/config-backup-routes.test.ts:270). A case in the vm harness in test/config-backup-restore-ui.test.ts would pin it.
  • docs/wiki/Config-Backups.md:140-145: the restore keeps the backup's retention per key whenever the live file lacks it, not only when it has "none of the four". docs/api-reference.md:1161, docs/wiki/Settings-Reference.md:193-197 and CLAUDE.md:159 still give 20 and 30 as the defaults for an absent key, which now means what the newest backup in that folder recorded.
  • src/config-backup.ts:342, :405, :453: a pass lists and reads every manifest three times (24.5 ms here at 500 backups on local disk); listing once and passing the result through keeps it to one walk on a network folder.
  • src/config-backup.ts:351: after a clock set to a future year, every later backup keeps that date. A Troubleshooting line (delete the future-dated folder) covers it.
  • src/config-backup.ts:593: the pre-restore copy is forced even when it equals the newest backup, so it spends a retention slot on a duplicate; a non-forced call that reports the existing id would avoid it.

Once 1 is in, this is ready to merge, and I will add the architecture-invariants section and the State Files entries (config-backup-id, backups/config/, the retention recorded per manifest) at merge as planned. Thanks again for the care in the docs and tests, and for turning every round around so quickly.

…ck reads it, test that Save omits a displayed-only retention
@opticon454

Copy link
Copy Markdown
Contributor Author

Thanks @Ark0N, the must-fix is in the new commit, along with most of the smaller things.

Must fix. readEffectiveBackupSettings() now probes the folder with probeBackupDir() before withRecordedRetention() can list it, and returns present as well; it throws BackupDirUnavailableError for a folder that does not answer, and skips the fallback when a custom folder does not exist yet. The GET calls it inside its try, and diff and restore no longer call the probe separately since it is part of the read. New test/routes/config-backup-unreachable.test.ts mocks probePathKind to 'unknown', records readdirSync/readFileSync/statSync/lstatSync/existsSync on the folder, uses a settings.json with only configBackupDir and an existing config-backup-id, and expects GET, diff and restore to answer 503 with nothing touched. I checked that all three fail against the previous route code.

Smaller things

  • The retention keys a Save sends are now built by _configBackupRetentionPayload(prev), with tests in the vm harness: a displayed-only value is left out, a value saved before or edited is sent, and the clamping is unchanged.
  • The wiki's Restoring section says the restore keeps the backup's retention key by key; api-reference.md, Settings-Reference.md and CLAUDE.md say an absent key means what the newest backup in the folder recorded and the defaults apply only when none did.
  • Troubleshooting.md has the future-clock line (delete the future-dated folder).
  • I left the manifest being listed three times per pass and the forced pre-restore copy for you to take at merge, if you still want them; neither is a correctness issue.

Typecheck, lint and the config-backup, route and settings tests (91) pass locally.

@Ark0N

Ark0N commented Oct 20, 2026

Copy link
Copy Markdown
Owner

Thank you @opticon454 for the tenth round. This PR adds on-by-default config backups with change detection, retention that only touches its own folders, a checksum-verified restore with a pre-restore copy, and a redacted compare. The must-fix from my last review is in 8a4ff775: readEffectiveBackupSettings() now probes a custom folder before the retention fallback can list it, and the new test/routes/config-backup-unreachable.test.ts pins GET, diff and restore at 503 with nothing touched. The vm-harness cases for _configBackupRetentionPayload cover the Save point too. Typecheck, lint, format, frontend syntax and the config-backup tests pass here, and CI is green.

Nothing blocks the merge. A few small things, which I will fold in on top at merge unless you would rather push them yourself:

  • src/config-backup.ts:614: a restore that fails partway (disk full, a file the server cannot replace) leaves the files before it restored and answers a bare 500. I reproduced it with clis.json turned into a directory: settings.json came back, clis.json did not, and the response named neither them nor the pre-restore id; the Settings form was also not re-read, so a Save would write the old settings back. The fix is to return restored, failed and safetySnapshot on that path, run the reload hooks for what was written, and refill the form when settings.json is among them.
  • test/routes/config-backup-unreachable.test.ts:66: add ['POST', '/api/config-backups'] to the table, so Back up now is pinned as well (runConfigBackup already probes first).
  • From my last review, still planned: list the manifests once per pass instead of two or three times (src/config-backup.ts:342, :405, :453), make the pre-restore copy non-forced when it would equal the newest backup (:593), and add the architecture-invariants section, the CLAUDE.md pointer and the State Files entries (config-backup-id, backups/config/, the retention recorded per manifest).

I will merge with those on top. Thanks again for the care in the docs and tests, and for turning every round around so quickly.

@Ark0N
Ark0N merged commit 8a4ff77 into Ark0N:master Oct 22, 2026
2 checks passed
Ark0N pushed a commit that referenced this pull request Oct 22, 2026
…ed and what failed

Merge-time fix for #661. A restore that could not write one file (a full
disk, a directory where the file should be) used to stop there and answer a
bare 500, naming neither the files it had already put back nor the
pre-restore copy, and the Settings form was not re-read, so the next Save
wrote the old settings back over a restored settings.json.

restoreConfigBackup now writes every verified file it can, then throws
ConfigBackupPartialRestoreError with restored, failed (name and error),
untouched and safetySnapshot. The route runs the same reload hooks and audit
for the files written as a full restore does, and answers 500 with the error
envelope plus those fields (and restartRequired) in data, as the tab-layout
409 carries its layout. Settings refills the form when settings.json is among
the restored files, also on that path. Tests: a unit case where a later file
is still restored, the clis.json-as-a-directory route case, and two vm cases
for the Settings handler.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Ark0N pushed a commit that referenced this pull request Oct 22, 2026
…s not answer

Merge-time fix for #661. The unreachable-folder table covered GET, diff and
restore; POST /api/config-backups (Back up now) now has its row too.
runConfigBackup already probes before it touches the folder, and the row
fails when the temp sweep is moved ahead of the probe.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Ark0N pushed a commit that referenced this pull request Oct 22, 2026
Merge-time fix for #661. A pass listed the folder and parsed every manifest
three times: for the recorded-retention fallback, for the change check and
for the prune (and restore listed it twice more). On a network folder that
is every manifest read three times every five minutes.

runConfigBackupPass (config-backup.ts) now reads the folder once, shares the
names with the temp sweep and the manifests with withRecordedRetention, the
snapshot and the prune, and prepends the snapshot it just made (always the
newest, by the date floor) to the listing retention counts. runConfigBackup
in the route keeps the settings read and the bounded probe and calls it.
restoreConfigBackup lists once for the recorded retention and the pre-restore
snapshot. The exported createConfigBackup, latestBackupRetention,
withRecordedRetention and pruneConfigBackups keep their signatures
(latestBackupRetention and withRecordedRetention take an optional pre-read
listing). A recording node:fs test pins one
readdir and one read per manifest per pass, and that the new snapshot counts
toward retention.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Ark0N pushed a commit that referenced this pull request Oct 22, 2026
…hing changed

Merge-time fix for #661. The pre-restore copy was forced, so a restore made
while the live files equalled the newest backup spent a retention slot on a
duplicate. It now goes through the normal change check: when nothing changed
since the newest backup, no copy is made and safetySnapshot names that
backup. Two tests that relied on the forced copy now change a file before the
restore; a new one pins the reuse.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Ark0N pushed a commit that referenced this pull request Oct 22, 2026
…inter and State Files

Merge-time fix for #661, as promised in the reviews. The long Config backups
paragraph in CLAUDE.md moves into a new architecture-invariants section that
gives each rule with the reason behind it (allowlist, volatile keys, retention
by successor and the date floor, the origin id, retention after a bad or
reset settings.json, the bounded probe, one walk per pass, verify before
write, the undo copy and the partial-restore answer, the reload hooks,
redaction, admin only). CLAUDE.md keeps the short rules and a pointer, and the
State Files list names config-backup-id and backups/config/ with the
retention each manifest records.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Ark0N pushed a commit that referenced this pull request Oct 22, 2026
…h a configurable folder and retention

Conflicts in CLAUDE.md, README.md, docs/api-reference.md and
src/web/server.ts were additions beside #662's Copilot quota read and
#666's keep-warm sections; both sides kept.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Ark0N pushed a commit that referenced this pull request Oct 22, 2026
Conflict in CLAUDE.md's State Files line resolved by keeping #661's new
config-backup entries and this PR's "case default groups" in state.json.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@Ark0N

Ark0N commented Oct 22, 2026

Copy link
Copy Markdown
Owner

Merged and shipped in 1.44.0. Thank you @opticon454, ten rounds is a lot, and every one came back fast and better tested. Config backups with change detection, retention that only touches its own folders, a verified restore and a redacted compare are a real safety net for everyone's setup.

What I added at merge time, as separate commits:

  • A restore that fails partway keeps going past the file that failed, then answers 500 with restored, failed, untouched and the safetySnapshot id in data. It also runs the reload hooks for what it did write (one afterRestore() for both paths), and Settings re-reads its form when settings.json came back. There is a route test with clis.json turned into a directory.
  • Back up now is pinned in the unreachable-folder test.
  • One read per pass: runConfigBackupPass() reads the folder once and each manifest once, with a test that counts the reads.
  • The pre-restore copy reuses the newest backup when nothing changed since it. One side effect worth knowing: "nothing changed" uses the change check, which ignores the volatile keys, so a restore taken when only those differ makes no new copy of them.
  • Docs: a Config backups section in docs/architecture-invariants.md, a shorter pointer in CLAUDE.md, and the config-backup-id and backups/config/ State Files entries.

@opticon454
opticon454 deleted the feat/config-backups branch October 22, 2026 20:16
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.

3 participants