thalladaandClaude Fable 5.1
572056ce3b
Schema derived from Config::default() and the live config, help text for every shipped key, env locks and presence-only secrets, typed saves that preserve comments and validate through Config::load before an atomic rename, provider add/remove, the change history, and config reload on mtime. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHyYupFdBiR4VfoUM7NjSM
93 lines
4.6 KiB
Markdown
93 lines
4.6 KiB
Markdown
# Web dashboard step 5 implementation review
|
|
|
|
The final step 5 working tree implements the settings schema, editor, provider
|
|
operations, history page, reload-on-mtime behavior, and systemd write access
|
|
described by the plan. The implementation is well covered by behavior-focused
|
|
unit and router tests. The review found one security issue in the inherited
|
|
work—removing a provider could have copied a hand-written `api_key` into
|
|
`config_changes`—and fixed it with redaction plus a regression assertion.
|
|
|
|
## Critical
|
|
|
|
None.
|
|
|
|
## High
|
|
|
|
None. The provider-history secret exposure found during review is fixed in
|
|
`provider_literal`: `api_key` is removed before a whole-provider literal is
|
|
stored or rendered by the history page.
|
|
|
|
## Medium
|
|
|
|
None.
|
|
|
|
## Low
|
|
|
|
### Shipped provider removal differs from the literal plan wording
|
|
|
|
- Evidence: `ProviderCard::removable` and `remove_provider` refuse entries in
|
|
`default_providers()`, even when no `[llm]` role references them.
|
|
- Plan difference: §13.3 only explicitly requires refusal when an `[llm]` role
|
|
names the provider.
|
|
- Reason and recommendation: deleting a shipped provider's file table cannot
|
|
remove it from the effective config because `Config::default()` supplies it
|
|
again; pretending otherwise would reset it while reporting removal. Keep the
|
|
explicit refusal unless config loading later gains a provider tombstone or
|
|
replacement-map semantic.
|
|
|
|
### Absent defaults are not materialized by an otherwise unchanged form
|
|
|
|
- Evidence: `plan_changes` compares an absent file value through its effective
|
|
default before deciding whether to write.
|
|
- Plan difference: §13.2's “explicit beats implicit” sentence can be read as
|
|
requiring an absent default-valued key to be written whenever posted.
|
|
- Reason and recommendation: the same section says the form carries every
|
|
editable field and only changed fields are written. Materializing every
|
|
absent default on any no-JavaScript save would violate that behavior. Keep
|
|
the effective-value comparison; a reset from an explicit non-default value
|
|
still writes the default explicitly. Clarify this sentence in a future plan
|
|
revision if touched-vs-untouched browser state becomes a requirement.
|
|
|
|
## Nits
|
|
|
|
None.
|
|
|
|
## Plan Coverage
|
|
|
|
| Requirement | Status | Evidence |
|
|
|---|---|---|
|
|
| Derived schema, kinds, sources, help, group order and anchors | Implemented as planned | `schema`, `schema_with_env`, `walk`, `kind_for`, `SETTINGS_HELP` |
|
|
| Secret redaction and environment locks | Implemented as planned | `source_for`, secret field rendering, provider-literal redaction |
|
|
| Typed `toml_edit` save with collected errors | Implemented as planned | `plan_changes`, `apply_change` |
|
|
| Temp-file validation, permission preservation, rename and live swap | Implemented as planned | `write_validated`, `install` |
|
|
| One attributed history row per changed key | Implemented as planned | `record_changes`, `config_changes` |
|
|
| Provider add/remove and referenced-provider refusal | Implemented with the shipped-provider qualification above | `add_provider`, `remove_provider` |
|
|
| Reload on mtime with previous config retained after an error | Implemented as planned | `WebState::reload_if_changed`, settings GET banner |
|
|
| Settings history page, newest first, 100 per page | Implemented as planned | `history`, `config_changes`, `settings_history.html` |
|
|
| `/etc/daily-epub` writable in the server unit | Implemented as planned | `systemd/daily-epub.service` |
|
|
|
|
## Testing Assessment
|
|
|
|
Existing tests are meaningful: they compare the schema with serialized
|
|
`Config::default()`, compare shipped TOML leaves with the help table, exercise
|
|
all enum options through deserialization and `validate()`, prove secrets carry
|
|
no value, mutate and restore a real environment variable, compare untouched
|
|
configuration lines byte-for-byte, verify multiline arrays and new tables,
|
|
exercise validation failure and permission preservation, inspect persisted
|
|
history rows, add/remove/refuse providers, and drive the settings routes through
|
|
`Router::oneshot`. The provider test also proves a file-sourced API key does not
|
|
reach the audit table.
|
|
|
|
No weak or missing test from the step 5 list remains. The only environmental
|
|
suite limitation is the repository's pre-existing listener tests, which cannot
|
|
bind loopback in the sandbox.
|
|
|
|
## Open Questions
|
|
|
|
- Should the plan eventually define a tombstone for removing shipped providers,
|
|
or should the current “leave built-ins unreferenced” behavior become the
|
|
documented contract?
|
|
- The shared brief lists four Anthropic listener tests as sandbox-bound, while
|
|
the step 1 handoff also identifies three OpenAI tests using the same loopback
|
|
fake server. The latter fail with the identical sandbox permission error.
|