3523dcf93e
plans/ held 58 documents and exactly one said whether it was open. The rest
mixed finished work, reviews of shipped work, parked specs and genuinely
pending ones, with nothing distinguishing them, so "how many plans are in
the queue" had no answer short of reading all 58.
Now `grep -H '^Status:' plans/*.md` is the answer:
50 done 3 in progress 2 planned 2 reference 1 parked
Statuses were derived rather than guessed: CLAUDE.md's own built list and
"What's NOT built yet" section, plus checking the subject exists in the
code. A review of work that shipped counts as done -- it records what was
found, it is not a request for anything. `reference` separates the two docs
that are conventions rather than work items (admin-design-standards,
admin-work-framework), which otherwise read as permanently-open plans.
The vocabulary is deliberately five words. A larger one invites "mostly
done" and "blocked-ish", which is how the directory became unreadable.
test_plans_declare_status.py keeps it from rotting: a new plan without a
marker fails, as does an unknown status, one buried below the eighth line,
or an open status with no reason -- "planned" alone is the state that rots,
since nobody can tell later whether it waits on a decision, a dependency,
or just nobody's turn.
Also updates the sweep plan with what landed and what did not, including
that #9 was not a defect.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VRQXz5SYZYVWscxS1QqF6U
195 lines
8.6 KiB
Markdown
195 lines
8.6 KiB
Markdown
# Admin profile CRUD should write to the overlay
|
|
|
|
Status: done -- profile CRUD writes config.local.yaml
|
|
|
|
**Status: FINAL — decision-complete.** Written 2026-09-04 against
|
|
`feat/config-local-overlay` at `0570123` (PR #26). Depends on that PR landing.
|
|
|
|
## Why this exists
|
|
|
|
`plans/config-local-overlay.md` states the rule without qualification:
|
|
|
|
> **The admin portal writes to `config/config.local.yaml`. It never writes
|
|
> `config/config.yaml`.**
|
|
|
|
The implementation splits it. Allowlisted scalars go to the overlay; **profile
|
|
CRUD writes the base file directly**, documented at `src/admin.py:670`.
|
|
|
|
This is not a considered exception. It is chronological accident: profile CRUD
|
|
shipped in PR #25 (`340453e`), and the overlay decision was made afterwards
|
|
(`e5ee92b`). Plan 8 then implemented the new rule for the scalar path it
|
|
touched and left the profile path where it was.
|
|
|
|
## The reasoning is the same one the user already endorsed
|
|
|
|
Plan 8's justification for the scalar path:
|
|
|
|
> An operator changing a knob in a loopback-only admin portal is making a
|
|
> **local operational decision**, not a project decision. Someone changing a
|
|
> project default edits `config/config.yaml` in the repo and commits it,
|
|
> deliberately, through git.
|
|
|
|
Creating `onlycheaps` in the portal is that same act. Nothing about the
|
|
argument depends on the value being a scalar rather than a nested object — the
|
|
only reason the code diverges is the ordering above.
|
|
|
|
## What it costs to leave it
|
|
|
|
The tariff no longer lives in `config/config.yaml`, so the blast radius is far
|
|
smaller than the six clobberings Plan 8 was written for. The residual harm is
|
|
real but narrower:
|
|
|
|
- Using the profiles UI **re-dirties a tracked file**. `git pull --rebase`
|
|
refuses on a dirty tree — observed, and listed in Plan 8's problem statement.
|
|
- A `git switch` or `git checkout` silently discards a profile the operator
|
|
just created, with nothing indicating why.
|
|
- `commit -am` sweeps profiles into unrelated commits.
|
|
|
|
The sharpest version: **a partial guarantee is worse than none.** An operator
|
|
who has internalized "the portal writes to the overlay, `config.yaml` stays
|
|
clean" stops checking. The failure then arrives against an expectation this
|
|
project's own fix created.
|
|
|
|
## Two defects that exist TODAY, independent of the rule
|
|
|
|
Both were found by reading the code on 2026-09-04, and the first was confirmed
|
|
live. They are the reason this is a bug fix and not only a consistency tidy.
|
|
|
|
### 1. An overlay-defined profile is live in routing and invisible to the portal
|
|
|
|
`_persisted_profiles()` (`src/admin.py:~789`) reads **base only**:
|
|
|
|
```python
|
|
store = load_config_store_safe(config_path) or {}
|
|
return store.get("profiles") or {}
|
|
```
|
|
|
|
But `load_config` deep-merges the overlay, so a `profiles:` block in
|
|
`config/config.local.yaml` **is** live in `cfg.profiles`. Confirmed by writing
|
|
a temporary `ghosttest` profile into the overlay:
|
|
|
|
| reader | sees |
|
|
|---|---|
|
|
| `load_config` (what routing uses) | `['ghosttest']` |
|
|
| `_persisted_profiles()` (what the portal lists) | `[]` |
|
|
|
|
So the portal's profile list can already disagree with what the router will
|
|
actually accept as `auto:<name>`. That is the same class of failure as the
|
|
vision-ceiling incident — a real state, computed correctly somewhere, never
|
|
surfaced.
|
|
|
|
Note this is reachable **today** by hand-editing the overlay, before any of
|
|
this plan lands. It is not created by the redirect; the redirect is what fixes
|
|
it, because the portal will finally be reading the file it writes.
|
|
|
|
### 2. Delete reads merged config but writes the base file
|
|
|
|
`admin_profile_delete` deliberately reads the merged store so the
|
|
`default_profile` guard is correct regardless of which file set it:
|
|
|
|
```python
|
|
merged = _load_merged_config_store(config_path, config_local_path)
|
|
persisted_profiles = merged.get("profiles") or {}
|
|
```
|
|
|
|
then deletes from base:
|
|
|
|
```python
|
|
_persist_config_block(config_path, ("profiles", name), None, delete=True)
|
|
```
|
|
|
|
For an overlay-defined profile that is found-then-not-deletable: the existence
|
|
check passes on merged, the write raises `KeyError`, and the operator gets a
|
|
**404 for a profile the portal just confirmed exists**. The endpoint is already
|
|
half-overlay-aware, which is a good sign the split was never intentional.
|
|
|
|
## The rule
|
|
|
|
| operation | destination |
|
|
|---|---|
|
|
| create | `config/config.local.yaml` |
|
|
| update | `config/config.local.yaml` |
|
|
| delete | overlay-defined profiles only |
|
|
| base-defined profile | **read-only**, alongside built-ins |
|
|
|
|
## The deletion wrinkle, and why it resolves rather than blocks
|
|
|
|
Deep merge can add and override. It cannot express *remove*. A profile defined
|
|
in base `config/config.yaml` cannot be deleted from the overlay without a
|
|
tombstone, and tombstones are exactly the config-framework machinery Plan 8's
|
|
non-goals rule out ("resist the config-framework instinct").
|
|
|
|
**Resolve it by classification, not mechanism.** A profile someone committed to
|
|
`config/config.yaml` *is* a project artifact — the same category as a built-in.
|
|
Render it read-only, refuse edit and delete, and say why in a message that
|
|
points at the repo.
|
|
|
|
This is cheap because the refusal path already exists at three sites, each
|
|
guarding on `name in BUILTIN_PROFILES`:
|
|
|
|
- `src/admin.py:902` — create
|
|
- `src/admin.py:936` — update
|
|
- `src/admin.py:960` — delete
|
|
|
|
The change is a widened predicate and a distinct message, not new machinery.
|
|
Keep the two refusals **distinguishable**: a built-in cannot be changed at all,
|
|
while a base-defined profile can be changed by editing the repo. Collapsing
|
|
both into "read-only" would tell the operator less than the code knows.
|
|
|
|
It also gives the provenance display Plan 8 already built a real job: base vs
|
|
overlay is precisely what explains why a given profile is not editable.
|
|
|
|
## Interactions to get right
|
|
|
|
- **`routing.default_profile` may name a profile from either file.** The
|
|
existing delete guard already reads merged config for this; keep it. Deleting
|
|
the current default stays refused (422), and that check must run against the
|
|
merged view, not the overlay alone.
|
|
- **A name collision between base and overlay** must resolve to the overlay
|
|
(standard merge precedence) and be **visible as such** in the listing, not
|
|
silently deduplicated. Two definitions for one name is the ambiguity Plan 7
|
|
refused to accept for built-ins; the same reasoning applies here.
|
|
- **Built-in name collisions stay rejected at config load**, unchanged.
|
|
- **`_profile_probe` / `_probe_candidate_zero_admit` do not change.** Admission
|
|
is still computed via `routing.select_candidates`; this plan moves a write
|
|
destination and a visibility source, not any routing logic.
|
|
- **The zero-admit warning still fires** on save, unchanged.
|
|
|
|
## Non-goals
|
|
|
|
- No tombstone or delete-marker syntax in the overlay. If a base-defined
|
|
profile must go, it goes through git.
|
|
- Do not mirror profile writes into both files. Plan 8 settled that: one value,
|
|
one home.
|
|
- Do not migrate existing base-defined profiles into the overlay automatically.
|
|
Same reasoning as Plan 8's Migration section — this plan does not decide
|
|
where someone else's committed config lives.
|
|
- No changes to `RoutingProfile`'s fields, to built-in definitions, or to how
|
|
`auto:<name>` resolves.
|
|
- Do not widen `_CONFIG_ALLOWLIST`.
|
|
|
|
## Success criteria
|
|
|
|
- Creating a profile through the portal writes `config/config.local.yaml` and
|
|
leaves `config/config.yaml` **byte-identical** — asserted directly, since the
|
|
existing `test_profile_create_preserves_comments_and_does_not_create_overlay`
|
|
asserts the opposite and must be inverted rather than deleted.
|
|
- Updating an overlay-defined profile writes the overlay only.
|
|
- Deleting an overlay-defined profile removes it from the overlay only.
|
|
- A base-defined profile returns 403 on update and delete, with a message
|
|
distinct from the built-in message and naming `config/config.yaml`.
|
|
- **The portal lists overlay-defined profiles.** A test writes a profile into
|
|
the overlay and asserts `GET /admin/api/profiles/` returns it — this is the
|
|
regression test for defect #1 and must fail on current `main`.
|
|
- A base/overlay name collision resolves to the overlay and is labelled with
|
|
its source in the listing.
|
|
- Deleting the profile named by `routing.default_profile` is still refused when
|
|
that setting comes from **either** file.
|
|
- The overlay is created on first profile write when absent, with the same
|
|
header comment the scalar path writes.
|
|
- Backup-before-write and validation-of-merged-config still hold on the profile
|
|
path.
|
|
- Full suite green with `local_energy.enabled` both true and false.
|
|
- The user's `config/config.local.yaml` values are untouched: tariff `0.159`
|
|
and `enabled: true` verbatim, verified after the test run, not before.
|