Skip to content

fix(daemon): preserve disabled UI on manual start - #2205

Open
vincentkoc wants to merge 2 commits into
DeusData:mainfrom
vincentkoc:fix/daemon-preserve-ui-setting-20260914
Open

vincentkoc wants to merge 2 commits into
DeusData:mainfrom
vincentkoc:fix/daemon-preserve-ui-setting-20260914

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Sep 14, 2026 •

Copy link
Copy Markdown

What does this PR do?

A cold daemon start now preserves a saved ui_enabled=false setting. Previously it unconditionally requested UI configuration, rewrote the preference to true, and started the HTTP listener.

An explicit cold --port or --open request 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 to config 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-tests passed 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.sh exited 2 with 6,972 repo-wide clang-tidy diagnostics. An exact-path VFS comparison of unchanged and candidate src/main.c produced 125 diagnostics each with the same diagnostic categories; none point to changed lines. The existing main_run_daemon_ctl cognitive-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 e0a6133293556d87de6c2ca43de674e3a643e339 is not green: Windows guards failed section_cold_storm when client 0 reported secure CLI coordination could not be created (endpoint). The same failure was observed on sibling head c1dd44151fee8cfd815c693392a7c1c72def98fd, which shares base 339b3f4097aa6ede22fc382ab7fd320d93c498b8. The guard runs six concurrent cli list_projects calls; 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

  • Every commit is signed off (git commit -s).
  • Tests pass locally.
  • Full lint passes (the existing clang-tidy debt is described above).
  • New behavior is covered by a test, reproduced before the fix.

AI-assisted implementation and follow-up; submitted on behalf of @vincentkoc.

Signed-off-by: Vincent Koc <25068+vincentkoc@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

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. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

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.

@vincentkoc
vincentkoc marked this pull request as ready for review September 14, 2026 09:07
@DeusData DeusData added bug Something isn't working ux/behavior Display bugs, docs, adoption UX priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Sep 19, 2026
@DeusData

Copy link
Copy Markdown
Owner

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.

@DeusData

Copy link
Copy Markdown
Owner

@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 daemon start honours the saved ui_enabled value, and only an explicit --port/--open turns the UI back on and saves that choice. That is exactly what your change does. It brings the manual and auto-spawn start paths into agreement, and it fixes #2114 as @0x4nur4g described it. The corrected hint pointing to config set ui_enabled true is a nice catch too.

Two small things before we merge:

  • Could you rebase onto current main? It merges cleanly today but is about 180 commits behind, so CI should run on the exact head.
  • We'll look at the section_cold_storm Windows guard on our side. Your comparison with the sibling head points to something that already existed, and you were right not to weaken the assertion. We'll also add a release-notes line saying a bare daemon start no longer re-enables a disabled UI.

Thanks again. This is a careful, well-tested fix.

@DeusData

Copy link
Copy Markdown
Owner

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.

@DeusData

Copy link
Copy Markdown
Owner

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>
@DeusData

Copy link
Copy Markdown
Owner

@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 daemon start honours the saved ui_enabled value, and only an explicit --port or --open turns the UI back on and saves that choice. Since the branch allows maintainer edits, we are doing the remaining steps ourselves instead of asking you for the rebase. The first is done: we merged current main into the branch (one signed-off merge commit, 95fc0d3; your commit is untouched). About the Windows guard: you were right, the section_cold_storm failure came from a race when several first-run processes create the runtime directory at the same moment; it was fixed on main in #2275 after your branch was cut, and the branch now includes that fix. Next we will add a small unit test for the start rule (the readiness script is a manual guard that CI does not run) and explain it here. If you push to this branch again, please pull first so our commits are kept.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. ux/behavior Display bugs, docs, adoption UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

daemon start silently overrides persisted ui_enabled=false and re-enables the graph UI listener

2 participants