Repository navigation
fix(daemon): preserve disabled UI on manual start - #2205
vincentkoc wants to merge 10 commits into
Conversation
Signed-off-by: Vincent Koc <25068+vincentkoc@users.noreply.github.com>
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
|
Thank you for the cold-start configuration fix. Current startup has separate loaded-configuration and explicit CLI override paths; review should prove that configured values survive a cold start while explicit flags retain precedence. Please keep the cold/warm and explicit-override cases as the acceptance boundary. This is queued for focused review. |
|
@vincentkoc, thank you for this, and for the unusually honest PR description. Calling out the clang-tidy baseline and the Windows guard failure yourself saved us real time. We've settled the contract question this raised: a bare Two small things before we merge:
Thanks again. This is a careful, well-tested fix. |
|
Thanks again for the disabled-UI fix and the clear validation limits. The earlier rebase request remains recorded; attribution of the Windows guard and the release-note follow-up are maintainer-side tasks. This sweep has not rerun that guard or changed the agreed startup semantics. |
|
Thanks again for the disabled-UI fix. The branch refresh remains the recorded contributor step; Windows-guard attribution and the release-note follow-up remain ours. Sorry for the delay on our part—I’ll follow up soon. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
@vincentkoc, thank you again for this fix and for the unusually careful validation notes, and sorry for how long it has waited. Your change does exactly what we settled on: a bare |
The cold daemon start rule this PR introduces (a bare start honours a saved ui_enabled=false; only an explicit --port=N or --open turns the UI on and persists that choice) was covered only by tests/test_daemon_open_readiness.py, a real-process guard that no CI leg runs. Move the rule into a pure helper, cbm_ui_config_daemon_start_ui() in src/ui/config.c, and call it from the cold start path in main.c; the CBM_EMBEDDED_FILE_COUNT gate stays in main.c and the behaviour is unchanged. Add config_daemon_start_honours_saved_ui_choice to the ui suite, which CI runs. It fails if the old always-configure rule comes back (a bare start with a saved disabled UI) and if --port=N or --open stop re-enabling a disabled UI. Refs DeusData#2114 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
config set now stores ui_enabled and ui_port in the shared settings database (_config.db), so the readiness guard read of config.json raised FileNotFoundError before its first assertion. Read the pair back with config get ui_enabled and config get ui_port instead; they load the same value every start path uses. The guard compares it after config set, after the first start, and, for a disabled UI, after the repeated start, status and start --open commands. Drop the now unused json import. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
@vincentkoc, thank you again for this fix. The rule you implemented is exactly the one we settled on: a bare
Locally the ui and daemon suites ( If you push to this branch again, please pull first so these commits are kept. Thanks again for the careful work and the unusually honest validation notes. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thanks for your patience, @vincentkoc: CI went green, but another change touched docs/CONFIGURATION.md on main in the meantime, so we merged current main into your branch once more (c7e7b23, a plain signed-off merge commit; your change is unchanged) to let CI cover the exact tree that will merge. It merges as soon as this run is green. Please pull before pushing again. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thanks for your patience, @vincentkoc: the index resource-worker change (#1724) merged into main last night and touched src/main.c and docs/CONFIGURATION.md next to your change, so we merged current main into your branch once more (a plain signed-off merge commit, no conflicts; your change is unchanged) to get a fresh run on the exact tree that will merge. Please pull before pushing again. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you again, @vincentkoc. Another change merged into main tonight and touches src/main.c and docs/CONFIGURATION.md, which this PR also changes, so we merged current main into your branch - a plain signed-off merge commit, your change is untouched - to get a fresh CI run on the exact tree that will merge. Nothing is needed from you: it merges as soon as that run is green. If you push again, please pull first so the merge commit is kept. |
What does this PR do?
A cold
daemon startnow preserves a savedui_enabled=falsesetting. Previously it unconditionally requested UI configuration, rewrote the preference totrue, and started the HTTP listener.An explicit cold
--portor--openrequest keeps its existing UI-enabling behavior. Repeated bare starts report the disabled UI without incorrectly saying a browser request failed, and the configuration hint points toconfig set ui_enabled true. The configuration docs describe these semantics.Fixes #2114
Validation
The new disabled-UI regression failed against the unchanged base and passed with this fix. All seven real-process readiness cases passed, covering cold and warm starts, saved enabled/disabled settings, explicit UI requests, delayed readiness, and foreign-listener rejection.
scripts/test.sh BUILD_DIR=build/full-testspassed on macOS arm64: 8,022 tests passed, 10 platform skips, plus all prescribed process guards and security/ordering checks.git diff --check, formatting, cppcheck, and the NOLINT suppression check passed.Full lint is not green:
scripts/lint.shexited 2 with 6,972 repo-wide clang-tidy diagnostics. An exact-path VFS comparison of unchanged and candidatesrc/main.cproduced 125 diagnostics each with the same diagnostic categories; none point to changed lines. The existingmain_run_daemon_ctlcognitive-complexity diagnostic increases from 82 to 89, already above the threshold of 25. Tools: Apple clang-format 21, clang-tidy 22.1.8, cppcheck 2.21.0.Hosted CI at
e0a6133293556d87de6c2ca43de674e3a643e339is not green: Windows guards failedsection_cold_stormwhen client 0 reportedsecure CLI coordination could not be created (endpoint). The same failure was observed on sibling headc1dd44151fee8cfd815c693392a7c1c72def98fd, which shares base339b3f4097aa6ede22fc382ab7fd320d93c498b8. The guard runs six concurrentcli list_projectscalls; the error occurs in local CLI endpoint creation, before the daemon-control UI path changed here. This comparison does not replace an exact-head Windows pass. A maintainer needs to rerun/investigate that guard; no assertion has been weakened.Checklist
git commit -s).AI-assisted implementation and follow-up; submitted on behalf of @vincentkoc.