fix(config): refuse mutation after an existing configuration read fails

[rudycelekli]

Summary

Provider add/remove and updateConfiguration can overwrite an existing configuration after loading it fails. A truncated JSON file, invalid field type or permission-denied read produces nil, which the mutation paths treat as a missing file or replace with cached data.

Refuse these mutations when loading failed and the configuration file still exists. The error occurs before the update closure or write. Missing files still use the existing defaults/cached fallback. The lenient loading API and explicit reset operation remain unchanged.

Native evidence

• Actual public peekaboo config provider add on owned files: three failed reads previously returned success and replaced the files; four valid JSON/JSONC, missing-file and dry-run controls passed. CLI remove has an earlier guard and is not claimed affected.
• The complete native SDK at the unchanged baseline executed 23 regression cases: add/remove/update each overwrote truncated, wrongly typed and unreadable files, and cached-then-invalid update also overwrote the file (10 failures); 13 JSON/JSONC/environment-reference/missing-file/cache controls passed.
• At signed head 0e7c77f4902493adef925f80d757d809b3a4eec9, all 23 cases pass. The ten failed reads refuse mutation, preserve independently hashed file bytes and never enter the update closure. Permission cases ran as UID 501 with owned chmod 000 fixtures restored afterward.

Verification

• Full swiftformat --lint: no files require formatting (2,089 checked).
• Full swiftlint: 112 existing warnings, zero serious violations.
• Ordinary macOS CI: all six jobs pass; complete CLI suite 1,659 tests / 198 suites passes.
• CodeQL: all three jobs pass.
• Unchanged release validation: all four groups pass, including actual pnpm run test:safe (2,266 tests / 271 suites) and execution of the new SDK owner.

Baseline expected failures were qualification evidence, not acceptance. Final acceptance ran the real complete SDK on hosted macOS; no local Swift rebuild, UI permission or hardware behavior is claimed. The two production files and unchanged regression owner received independent source/evidence review before signing. No workflows, manifests, locks or submodules change.

Public CLI AFTER proof requested in review

The proof is in a tests-only validation branch in our fork at 0b9b72286638b0e4e5e3e5febd2954a9738ac961 (owner diff). Both production files, the original SDK owner prefix, manifests, workflows, locks and submodules are byte-identical to this PR's 0e7c77f4902493adef925f80d757d809b3a4eec9 tree. The ordinary macOS workflow built the real CLI and supplied PEEKABOO_CLI_BINARY; all seven new process cases actually executed as UID 501.

Command used for each owned fixture (with --dry-run added for that control):

peekaboo config provider add owned-public-new --type openai --name 'Owned public fixture' --base-url http://127.0.0.1:9/v1 --credential-ref '${OWNED_FAKE_REFERENCE}' --no-input --json

Each process receives only an owned HOME/TMP/config directory, a whitelisted PATH, and migration-disabled setting. This config command makes no provider request. The child is waited for and the owned fixtures/permissions are restored and removed afterward.

Copied actual terminal receipts below (same built binary SHA-256 for all cases):

Public CLI provider-add truncated: uid=501 pid=44429 binary=076d30eb28497fdae2cdbc64f02d3c7f650d55270cf059f291c9330a5721a4f0 exit=1 before=e57edb2b2bf79f08365c061ec91f550e53cff053e17ac1332e205868266d82bf after=e57edb2b2bf79f08365c061ec91f550e53cff053e17ac1332e205868266d82bf
Public CLI provider-add wrongType: uid=501 pid=44433 binary=076d30eb28497fdae2cdbc64f02d3c7f650d55270cf059f291c9330a5721a4f0 exit=1 before=20a42be397db413e0bc485a46d6813391626ea03264f77272b71a3f42b52f7b6 after=20a42be397db413e0bc485a46d6813391626ea03264f77272b71a3f42b52f7b6
Public CLI provider-add json: uid=501 pid=44434 binary=076d30eb28497fdae2cdbc64f02d3c7f650d55270cf059f291c9330a5721a4f0 exit=0 before=4ab0b11a552d6f6f9303186c7df80e10f38d73e1b68b03a65e8667c5ac4976f6 after=7ba3e4d519090c0adfc41f0c12e4abbab68f822c39b2d091bbcf72b35e292879
Public CLI provider-add jsonc: uid=501 pid=44435 binary=076d30eb28497fdae2cdbc64f02d3c7f650d55270cf059f291c9330a5721a4f0 exit=0 before=fa5d05935f16dbb9fb08881fbc8a10c2dc7e07e7f1f82c5a954895b4041229be after=7ba3e4d519090c0adfc41f0c12e4abbab68f822c39b2d091bbcf72b35e292879
Public CLI provider-add absent: uid=501 pid=44451 binary=076d30eb28497fdae2cdbc64f02d3c7f650d55270cf059f291c9330a5721a4f0 exit=0 before=absent after=24158a5778c78736111926511cbb4fd6b072710a258bbaf3f455ab6c0c2071fd
Public CLI provider-add dryRun: uid=501 pid=44467 binary=076d30eb28497fdae2cdbc64f02d3c7f650d55270cf059f291c9330a5721a4f0 exit=0 before=e57edb2b2bf79f08365c061ec91f550e53cff053e17ac1332e205868266d82bf after=e57edb2b2bf79f08365c061ec91f550e53cff053e17ac1332e205868266d82bf
Public CLI provider-add unreadable: uid=501 pid=44483 binary=076d30eb28497fdae2cdbc64f02d3c7f650d55270cf059f291c9330a5721a4f0 exit=1 before=4ab0b11a552d6f6f9303186c7df80e10f38d73e1b68b03a65e8667c5ac4976f6 after=4ab0b11a552d6f6f9303186c7df80e10f38d73e1b68b03a65e8667c5ac4976f6

Actual truncated-file JSON error excerpt (only the owned temporary path is redacted):

{
  "error" : {
    "code" : "ADD_FAILED",
    "message" : "Failed to add provider: File I\/O error: Unable to read existing configuration at <owned-config>/config.json. Refusing to replace it."
  },
  "data" : null,
  "debug_logs" : [
    "[2026-10-08T06:22:35.418Z] DEBUG: Runtime host: local (in-process)"
  ],
  "success" : false
}

Truncated/wrong-type/unreadable files return failure and preserve exact bytes. Valid JSON/JSONC preserve custom/default/logging/environment-reference values while adding the provider; absence creates config; dry-run succeeds without writing. The existing 23 SDK cases pass again.

At the tests-only proof head, all unchanged mandatory groups pass: ordinary six jobs, CodeQL three jobs, and supplemental four groups including the actual full safe suite. This proof branch does not change the submitted production fix and does not create another upstream PR.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[cursor]

PR Summary

Medium Risk
Touches core config persistence for provider add/remove and updates; behavior change prevents data loss on corrupt/unreadable files but could surface new errors where mutations previously “succeeded” with a blank overwrite.

Overview
Configuration mutations no longer overwrite an on-disk config when a read fails. addCustomProvider, removeCustomProvider, and updateConfiguration now go through requireReadableConfigurationForMutation, which throws a file I/O error if config.json exists but loading returned nil (truncated/invalid JSON, permission denied, etc.)—before any update closure runs or bytes are written.

Missing files and successful reads behave as before: absent config still falls back to defaults, cached in-memory config, or empty Configuration(); valid JSON/JSONC and env references are unchanged on mutation.

Adds serialized SDK tests (ConfigurationMutationReadTests) covering refused failed reads (byte preservation, no update closure), cached-then-invalid updates, and positive paths for add/remove/update.

^{Reviewed by Cursor Bugbot for commit 0e7c77f. Bugbot is set up for automated code reviews on this repo. Configure here.}

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed October 8, 2026, 3:12 AM ET / 07:12 UTC (Revision 2).

ClawSweeper review

What this changes

The branch refuses provider and configuration mutations when an existing configuration file cannot be read, while preserving missing-file fallbacks.

Example: Add provider owned-public-new with a truncated config.json

• Before: The command succeeds and replaces the truncated configuration.
• After: The command exits 1 with ADD_FAILED and “Refusing to replace it,” preserving the original file bytes.

Review scores

Measure	Result	What it means
Overall readiness	🦞 diamond lobster (5/6)	A narrow, useful configuration-preservation repair with real CLI proof, compatibility controls, and no remaining findings.
Proof confidence	🦞 diamond lobster (5/6)	Sufficient (terminal): The copied hosted-macOS terminal receipts exercise the real provider-add CLI against production files verified identical to the submitted head: all three failed-read cases refuse publication and preserve hashes, while four compatibility controls pass. The 23 disk-backed SDK cases supplement coverage of remove/update. The previous rank-up request is satisfied; unchanged serialization and existing-state controls establish compatibility without migration.
Patch quality	🦞 diamond lobster (5/6)	No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Adding a provider or updating settings can erase an existing configuration when that file is malformed or unreadable.
Reason: Preventing destructive replacement has clear value and requires only a small guard. The repair preserves supported configuration behavior without adding a setting or public API.

Merge readiness

✅ Ready for maintainer review

This PR remains useful: current main still permits the destructive overwrite, and the added public CLI transcript resolves the previous proof blocker. No actionable patch defect remains.

Priority: P2
Reviewed head: 0e7c77f4902493adef925f80d757d809b3a4eec9

Before merge

None.

Findings

None.

Agent review details

How this fits together

Peekaboo shares saved configuration between its CLI, macOS app, and automation SDK. Provider changes and settings updates read this configuration, modify it, and write it back to disk.

flowchart TD
    A[Provider or settings update] --> B[Read saved configuration]
    B --> C{Read succeeded?}
    C -->|Yes| D[Modify loaded settings]
    C -->|No| E{File still exists?}
    E -->|Yes| F[Return error and preserve file]
    E -->|No| D
    D --> G[Save configuration]

Loading

Technical review

Best possible solution:

Keep failed-read protection inside the existing serialized mutation boundary, preserving valid settings, missing-file creation, and explicit reset behavior.

Do we have a high-confidence way to reproduce the issue?

Yes: current source returns nil on decoding or read failures, then mutation paths fall back and save replacement configuration. Contributor baseline evidence records the corresponding destructive results; this review did not execute them.

Is this the best way to solve the issue?

Yes: a shared guard before mutation is a narrow repair that retains the existing loader, locking, serialization, and missing-file behavior.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against c02c26927d77.

Provenance checked

• ConfigurationManager+CustomProviders.swift: addCustomProvider/removeCustomProvider keeps the original intent (17b6148: The recorded transaction repair serializes complete load-mutate-persist operations so concurrent updates cannot discard one another.)
• ConfigurationManager+Persistence.swift: updateConfiguration keeps the original intent (17b6148: Fresh loading and recursive transaction locking preserve disk-backed updates during concurrent configuration mutations.)

Testing

Proof path: shipped entry point. Added test files: 1.

Security

None.

Evidence

What I checked:

• Introduced production guard: The pinned introduced diff routes add, remove, and update through a shared guard before mutation or publication; missing-file fallback expressions remain intact. (Core/PeekabooCore/Sources/PeekabooAutomation/Configuration/ConfigurationManager+Persistence.swift:43, 0e7c77f49024)
• Still necessary on current main: The configuration implementation and relevant CLI command files have no changes between fetched main and the pinned base. Main therefore retains the failed-read fallback that can publish defaults or cached configuration. (Core/PeekabooCore/Sources/PeekabooAutomation/Configuration/ConfigurationManager+Persistence.swift:34, c02c26927d77)
• After-fix public CLI receipts: The fully supplied PR body, captured as sourceRevision 194347dc0701fa13c884d1bf59bd724f081d827a77260296d4f30a3e9605c88b, records seven actual macOS CLI processes as UID 501. Truncated, wrong-type, and chmod-000 files exit 1 with unchanged before/after SHA-256 hashes; JSON, JSONC, absent-file, and dry-run controls exit 0. The JSON error contains ADD_FAILED and the new refusal message. (0e7c77f49024)
• Proof source equivalence: GitHub's comparison shows the validation head adds only 122 test lines to the existing test file. Its process harness launches the real executable with the exact provider-add arguments, captures output, and checks exit status and preserved bytes; production files and other repository files are unchanged. https://github.com/rudycelekli/Peekaboo/pull/4/files. (Apps/CLI/Tests/CoreCLITests/ConfigurationMutationReadTests.swift:232, 0b9b72286638)
• Validation run identity: The ordinary macOS run is completed successfully at the tests-only proof head, and all six jobs succeeded. This supports the copied runtime receipts rather than substituting CI status for behavior proof. https://github.com/rudycelekli/Peekaboo/actions/runs/37735497625. (0b9b72286638)
• Compatibility coverage: The submitted tests cover ten refused failed-read cases and thirteen compatible cases across add/remove/update, including retained settings and credential references, JSONC, environment interpolation, absent files, and cached configuration. No serialized schema, migration, or default changes are introduced. (Apps/CLI/Tests/CoreCLITests/ConfigurationMutationReadTests.swift:100, 0e7c77f49024)

Likely related people:

• SebTardif: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Review metrics

Metric	Value	Why it matters
Production versus test growth	production +13/-3 lines; tests +230/-0 lines	Small production growth is justified by one shared guard; the larger test matrix checks byte preservation and compatibility across three mutation APIs.

Labels

Label changes:

• add proof: sufficient: Contributor real behavior proof is sufficient.
• add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
• add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.
• remove rating: 🦐 gold shrimp: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.
• remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

• P2: This is a focused configuration-preservation fix affecting mutations after malformed or unreadable file reads.
• rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.
• proof: sufficient: Contributor real behavior proof is sufficient.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only; repair and merge need explicit maintainer commands such as @clawsweeper autofix or @clawsweeper automerge.

History

Review history (1 earlier review cycle)

• reviewed 2026-10-08T05:23:23.408Z sha 0e7c77f :: needs real behavior proof before merge. :: none

[rudycelekli]

Added the requested real public CLI AFTER proof to the PR body, including copied actual terminal receipts and JSON refusal output so it does not depend on access to hosted job artifacts.

The unchanged ordinary macOS workflow built the real CLI from a tests-only validation branch at 0b9b72286638b0e4e5e3e5febd2954a9738ac961; production files/manifests/workflows/submodules are identical to this PR's 0e7c77f4902493adef925f80d757d809b3a4eec9. Seven owned process cases actually ran as UID 501 using binary SHA-256 076d30eb28497fdae2cdbc64f02d3c7f650d55270cf059f291c9330a5721a4f0. Truncated JSON, wrong field type and permission-denied reads return failure with ADD_FAILED/“Refusing to replace it” and preserve identical file hashes. Valid JSON/JSONC, absence and dry-run controls pass; the existing 23 SDK cases pass again. All six ordinary jobs, three CodeQL jobs and four supplemental groups including the actual full safe suite pass at that proof head.

No upstream production change or additional upstream PR was needed for this proof. @clawsweeper re-review

[steipete]

Landed as 81bce40. Thanks @rudycelekli!

Verification: I confirmed loadConfiguration() and updateConfiguration both read ConfigurationManager.configPath, the same path the new existence guard checks. Missing files keep the defaults/cached fallback, while a failed read of an existing file now refuses before the update closure or write. Codex autoreview (P0–P2) was scoped-clean at 0e7c77f. Hosted CI was green on that head, including the full CLI suite with the 23 ConfigurationMutationReadTests cases. I'm adding the changelog entry with credit in a follow-up.