Merge branch 'step5' into web-dashboard
# Conflicts: # src/web/static/app.css # src/web/static/app.js
This commit is contained in:
@@ -0,0 +1,62 @@
|
||||
# Step 5 handoff — settings
|
||||
|
||||
## Landed
|
||||
|
||||
- `src/web/dashboard/settings.rs`: a schema derived by walking serialized
|
||||
default and live `Config` values, including optional/env-only fields, exact
|
||||
field kinds and enum choices, derived environment names, source tracking,
|
||||
README/example-backed help text, stable group order, and dotted-path anchors.
|
||||
- `/dashboard/settings`: reload-on-mtime with a visible load-error banner and
|
||||
last-good config retention; secret presence-only rendering, environment
|
||||
locks, typed inputs, defaults/reset controls, and weight normalization notes.
|
||||
- Typed `toml_edit` saves that preserve comments/order, collect field errors,
|
||||
write multiline string arrays, create missing tables, validate through
|
||||
`Config::load` in `<path>.tmp.<pid>`, preserve permissions, rename atomically,
|
||||
swap the live config, and record one attributed `config_changes` row per key.
|
||||
- Provider add/remove flows through the same validated writer, including name
|
||||
and kind validation, role-reference refusal, placeholder defaults, audit
|
||||
history, and redaction of a hand-written provider `api_key` from history.
|
||||
- `/dashboard/settings/history`, newest first at 100 rows per page.
|
||||
- A step 5 CSS block and reset helper in `app.js`; `/etc/daily-epub` added to
|
||||
the server unit's `ReadWritePaths`.
|
||||
- Sixteen settings tests covering every schema and writer case listed in §17,
|
||||
including router-level rendering and POST behavior through `oneshot`.
|
||||
|
||||
## Deviations and notes
|
||||
|
||||
- Shipped providers cannot be removed, even when unreferenced. They are members
|
||||
of `Config::default()`, so deleting their TOML table would immediately restore
|
||||
the built-in entry and falsely report success. They can be edited or left
|
||||
unreferenced; custom providers can be removed normally.
|
||||
- `FieldKind::Enum` owns `Vec<String>` rather than the plan sketch's static
|
||||
slice because `llm.bulk` and `llm.editor` must include provider names derived
|
||||
at runtime. The rendered behavior and validation table are unchanged.
|
||||
- Restart notices also cover `server.public_url`, `server.session_days`,
|
||||
`server.login_attempts`, and `server.login_window_minutes`, in addition to
|
||||
the plan sketch's `database_path` and `server.bind`, because those values are
|
||||
captured when the server/session/throttle layers are built.
|
||||
- An absent key whose effective value is already the submitted default remains
|
||||
absent. This reconciles “only changed fields” with the no-JavaScript form
|
||||
posting every editable field; resetting an explicit non-default file value
|
||||
still writes the default explicitly.
|
||||
- Step 6 should call `WebState::reload_if_changed` before starting each job, as
|
||||
§4.2 requires. Step 5 supplies the shared helper; this branch's jobs module is
|
||||
intentionally still the parallel-step stub.
|
||||
- The shared brief's sandbox list omits three pre-existing OpenAI fake-server
|
||||
tests that bind through the same `curate::llm::tests::serve` helper as its four
|
||||
named Anthropic tests. The step 1 handoff records those three additional
|
||||
sandbox failures; they are not settings regressions.
|
||||
|
||||
## Verification
|
||||
|
||||
- `cargo fmt`: pass.
|
||||
- `cargo clippy --all-targets -- -D warnings`: pass.
|
||||
- Focused `cargo test web::dashboard::settings -- --nocapture`: **16 passed,
|
||||
0 failed**.
|
||||
- Unfiltered `cargo test`: **379 library tests passed** before the harness
|
||||
reported the expected loopback-bind failures (the 10 named library tests plus
|
||||
the three OpenAI tests noted above); no settings test failed.
|
||||
- `cargo test` with all pre-existing loopback listener tests excluded:
|
||||
**411 passed, 0 failed, 15 filtered out** (379 library, 7 binary-unit, and 25
|
||||
integration tests passed; 13 library listener tests and both `m7_server`
|
||||
tests were filtered).
|
||||
@@ -0,0 +1,92 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user