awslabs / awslabs/cli-agent-orchestrator

A settings write over an unparseable settings.json silently deletes every setting

Open
#737 1 comment 0 reactions 1 assignee Claimed by @fanhongy View on GitHub
Dominant language
Python
Stars
1.3k
Forks
267
Avg merge
1d 23h
Merged PRs (30d)
70

Description

A single syntax error in `~/.aws/cli-agent-orchestrator/settings.json` plus any subsequent settings write **silently deletes every setting in the file**. The write reports success. There is no backup.

Found while checking the vault configuration surface (#722), but this is not vault-specific — it affects every section of `settings.json` on `main` today.

**This blocks #722.** That issue proposes `cao config set memory.vault ''`, which would route through the same read-modify-write pair described below. The users who reach for a CLI setter are disproportionately those whose hand-edit just went wrong — exactly the state in which that pair destroys the file. Today they get a validation error and an intact file; with the setter added before this fix, they would get a truncated one. Measured with a working vault config plus one trailing comma, using `memory.enabled` as a stand-in for the proposed setter since both share the pair:

```
BEFORE: 345 bytes, "vaults" block present
$ cao config set memory.enabled true
{"enabled": true, "flush_threshold": 0.85, ...} # success, exit 0
AFTER: 41 bytes, "vaults" block gone
{ "memory": { "enabled": true } }
```

### Reproduction

```bash
export CAO_HOME_DIR=$(mktemp -d)
cat > "$CAO_HOME_DIR/settings.json" <<'EOF'
{
"terminal": {"backend": "tmux", "herdr_session": "cao"},
"logging": {"level": "DEBUG"},
"agents": {"extra_dirs": ["/Users/me/important-agent-store"]},
"memory": {"enabled": true, "flush_threshold": 0.5,}
}
EOF
# note the single trailing comma on the memory line ---------------^

cao config set logging.level INFO
cat "$CAO_HOME_DIR/settings.json"
```

Observed — 216 bytes in, 42 bytes out:

```
Failed to read /…/settings.json: Expecting property name enclosed in double quotes: line 5 column 54
"INFO"
```

```json
{
"logging": {
"level": "INFO"
}
}
```

`terminal`, `agents.extra_dirs` and `memory` are gone. The command exited 0 and echoed `"INFO"` as if it had succeeded; the only hint is a `logger.warning` that is easy to miss and not visible at every log level.

### Cause

Both settings loaders swallow every exception and return an empty dict:

- `services/settings_service.py:32-37` — `except Exception as e: logger.warning(f"Failed to read settings: {e}")` then `return {}`
- `services/config_service.py:274` — same shape

Each is a read-modify-write pair: the loader returns `{}`, the caller mutates that empty dict, and `_save`/`_save_raw` writes it over the original file. There is no backup, no atomic temp-and-rename, and no distinction between "file absent" (where `{}` is correct) and "file present but unparseable" (where `{}` discards user data). Eight call sites across the two services write through those loaders — `settings_service.py` lines 94, 161, 674, 716, 769 and `config_service.py` lines 287, 433, 442 — so this is reachable from far more than `cao config set`; any code path that persists a setting will do it.

### Why the current behaviour looks deliberate but isn't quite

Lenient loading is reasonable for a *read* — the tool starting with defaults beats refusing to start. It is not reasonable for a read that will be written back, because the fallback value is then persisted as the new truth.

The suite does not pin the current behaviour for the primary file. The only malformed-file test is `test_config_service.py:93 test_malformed_legacy_file_falls_back_to_default`, and that one is about `LEGACY_CONFIG_FILE` — a migration-only input where silent fallback is defensible and can stay. Nothing asserts that a malformed *primary* `settings.json` yields `{}`, and nothing covers the read-then-write path, so a fix is unblocked.

### Proposed remedy — refuse to write on unparseable input

Stating this as a concrete proposal rather than a menu, so there is something specific to disagree with before any code exists.

1. **Distinguish absent from unparseable.** Return `{}` for a missing file; raise for a file that exists and does not parse. `{}` is the correct answer to "no file"; it is a wrong answer to "file I could not read".
2. **Refuse to write over a file that failed to parse.** Any setter reached with an unparseable file exits non-zero with the path, the parse position, and an explicit "no settings were changed". This is the load-bearing change — the others are hardening.
3. **Make the failure loud on read too.** Read-only commands warn on stderr and continue with exit 0; anything that would write exits non-zero. A parse error is a misconfiguration the user has to fix, and a warning buried in logs means the tool silently runs on defaults. This also fixes the misleading downstream message noted in #722, where a trailing comma surfaces as `Error: memory vault is disabled by configuration` — pointing at a flag that is in fact `true`.
4. **Write atomically** — `mkstemp` plus `os.replace` in the destination directory — so an interrupted or failing write cannot leave a truncated file either. Standard library only; no new dependency for two chokepoint functions.
5. **Back up before the first write of a session** to `settings.json.bak`, making any future instance of this class recoverable. (Retention policy — one rolling backup vs timestamped — is deliberately left open.)

Both loaders route through one primitive rather than being fixed twice in parallel, and the regression tests exercise `config_service._load_raw` directly, not only `set()` — the read-triggered path is reachable without any user write at all, so a test that only drives the setter would miss it.

### Alternative considered and not chosen — lenient-with-backup

The obvious cheaper option is to keep loading leniently but write a `.bak` first, so the data is recoverable rather than preserved. It is not proposed, for three reasons:

- It keeps a **success exit code on a data-losing operation**. The user is told the write worked, and only discovers otherwise later.
- Recovery requires the user to know a backup exists, notice the loss, and act before the next write rolls the backup forward. Every one of those is a chance to lose the data permanently.
- It leaves the actual bug — a fallback value being persisted as the new truth — in place, and mitigates its consequences instead.

Refuse-to-write costs the user one clear error and an intact file. Lenient-with-backup costs them a silent truncation and a recovery procedure. That trade seems clearly worth making, but it is a judgement call about CAO's tolerance for hard failures on misconfiguration, and it is a maintainer's call to overrule.

### Before I implement this

I intend to send a PR implementing the remedy above, separately from and ahead of the #722 vault work, targeting `main`. If the refuse-to-write stance is wrong for CAO — or if you would rather this were fixed differently, or scoped differently — saying so on this issue now is much cheaper than saying so at PR review. I will give it a short window for objections and then proceed on the proposal as written; I am not asking anyone to commit to a review timeline.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.