docs(app): record the AUD-16 and AUD-19 decisions (P0-08) - #61
Merged
Merged
Conversation
Both looked like inconsistencies and both are deliberate; this writes that down at the bind sites, in ARCHITECTURE.md, and in a test. AUD-16 - rollback stays user-facing. ADR-0006 scopes developer tooling to producing a model: training, repo-path config, train-from-export, the CLI copy surface. Recovering from a bad deployed model is the user's half of that line, so rollback_classifier_model and, for the same reason, retry_model_deployment_cleanup stay ungated. A user whose classifier was ruined by a deployment must not need SNAPBACK_DEV_TRAINING or a Debug build to get back to a working model. AUD-19 - no-session predictions are an intended live preview, so the health field was the thing that was wrong. Of its four values only private_mode and idle actually stop compute_event before predict(); without a session the engine scores normally and only persistence is skipped, since persist() early-returns on an empty session_id. The value is now 'not_recorded' rather than 'no_session', which claimed a suppression that never happened. 647/647 ctest and the frontend suite pass.
There was a problem hiding this comment.
🟡 Changes recommended
The demo diagnostics contract remains inconsistent, with documentation and test follow-ups also unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Records the AUD-16 and AUD-19 decisions for P0-08, clarifying user-facing model recovery and no-session preview diagnostics.
Changes:
- Documents recovery command access decisions.
- Renames
no_sessiontonot_recordedand adds regression coverage. - Updates architecture and roadmap documentation.
File summaries
| File | Summary and review notes |
|---|---|
tests/test_app_state.cpp |
Adds preview coverage. Nit (2 votes): Assert prediction_history(10) is empty to verify it is not persisted. |
src/app/state.cpp |
Updates prediction status semantics and explanatory comments. |
src/app/commands.hpp |
Documents user-facing recovery commands. |
frontend/src/useDiagnostics.ts |
Updates the diagnostics default. Moderate (1 vote): Update the demo backend, which still returns none after a session ends. |
docs/ROADMAP.md |
Marks P0-08 complete. Nit (3 votes): Update or mark historical the remaining no_session descriptions. |
docs/ARCHITECTURE.md |
Records both decisions and their rationale. |
Review details
Suppressed comments (1)
frontend/src/useDiagnostics.ts:17
- The browser demo is another producer of this diagnostics contract, but
frontend/demo/backend.ts:225-237still hard-codespredictionSuppressionReason: "none"even afteractiveSessionIdbecomes null. The real backend now returnsnot_recorded, so stopping a demo session leaves Diagnostics inconsistent with the documented/API value; update the demo health response too.
predictionSuppressionReason: "not_recorded",
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+203
to
+204
| predictions are a deliberate live preview, so the health field was renamed | ||
| `no_session` → `not_recorded` to stop claiming a suppression that never happened. |
| const auto latest = state->latest_prediction(); | ||
| REQUIRE(latest.has_value()); | ||
| // ...but it carries no session, which is what keeps persist() from writing it. | ||
| CHECK(latest->session_id.empty()); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes P0-08, the last remaining code task in Phase 0. Both AUD-16 and AUD-19 were flagged as decisions rather than bugs, so this records the decisions where the next reader will hit them.
AUD-16 — rollback stays user-facing
get_training_deploy_status,set_training_repo_path, andtrain_from_exportall requiredeveloper_tools_enabled().rollback_classifier_modelandretry_model_deployment_cleanupdo not, and that asymmetry read as an oversight.It isn't. ADR-0006 scopes developer tooling to producing a model — "training, repo-path configuration, in-app train-from-export, and the CLI copy surface." Recovering from a bad one is the user's half of that line: someone whose classifier was ruined by a deployment must not need
SNAPBACK_DEV_TRAININGor a Debug build to get back to a working model. Comments at both bind sites now say so.AUD-19 — no-session predictions are an intended preview; the health field was wrong
The audit asked whether the live preview is intended or the suppression reason is lying. Tracing it, the field is the part that's wrong — and more narrowly than the ticket suggests.
prediction_suppression_reasonhas four values, and only two mean no prediction was computed:private_modestate.cpp:2134idlepredict()state.cpp:2212no_sessionclassifier_.predict()runs atstate.cpp:2262noneWithout a session the engine scores normally, sets
prediction_dirty_so the tick emits it, and updateslast_prediction_at_ms_. What the missing session costs is persistence —persist()early-returns on an emptysession_id(state.cpp:2292).That's intended: the Now surface previews live scores so the untracked-work nudge has something to react to before the user hits record. So the value is now
not_recorded, which describes what actually happens.Worth noting the old string was nearly unobservable anyway:
DiagnosticsCard.tsx:50renders the reason only whenlastPredictionAgeSecs == null, but the no-session path sets that timestamp on every prediction — so the card showed "1.3s old", never "none (no_session)". Surfacing "predicting but not recording" in the UI is a real improvement but it's a product change, not P0-08; left alone deliberately.Changes
rollback_classifier_modelandretry_model_deployment_cleanupbind sitesprediction_suppression_reasonassignment explaining which values mean whatno_session→not_recorded(C++ producer, theuseDiagnostics.tsdefault, two test assertions)ARCHITECTURE.mdsubsection under IPC: "Two decisions the command surface encodes"session_id, which is what keepspersist()from writing itP0-08checked off inROADMAP.mdwith the decisions recorded inlineVerification
ctest— 647/647 passed, including the newAppState predicts without a session but does not record itnpm run test:components— 31 files, 165 tests passednpm run typecheck— exit 0prettier --checkon the one changed frontend file — clean (the repo-wide advisory warnings are pre-existing; the format job only gates added files, and this adds none)Phase 0 now has one item left: P0-09, the manual Windows Release soak.