Slice 3/6: install renames + dashboard styling + healthcheck channels/self-mode#260
Merged
Conversation
…al-state outcome) The 'Daemon status' screen was an orphan flat NoticeInfo listing key:value lines, with no colored outcome - inconsistent with the other diagnostics. Restyle it to match: it now computes the daemon's REAL combined state (systemd existence refined onto the heartbeat via health.RefineWithPresence - the SAME verdict the run and healthcheck checks use) and renders it as a severity-colored notice: a state keyword in the colored title (green 'running' only when the daemon is alive and beating; yellow 'running, not reporting' / 'not running' / 'not installed' / 'stale' otherwise) plus a one-line explanation and the scheduler details in the body. Still read-only and automatic on entry (no check button), consistent with how the status screen already behaved. Sharing health's state vocabulary means this screen and the healthcheck checks can never disagree about the daemon by construction.
Add a dashboard 'Check for updates' screen styled like the other checks: a colored Status keyword + explanation, with two buttons (Check upgrade / Back). Check runs the same update-availability probe proxsave already does every run (checkForUpdates); if a newer release exists the button becomes 'Run upgrade' and the Status shows the available version, else 'no upgrade'. Run upgrade drives the real --upgrade flow (runUpgrade with UpgradeAutoYes) entirely in-session via RunTask, muting the CLI logger AND os.Stdout/err so the bootstrap output never corrupts the alternate screen, then shows a success/failure notice. The bubbletea program captured stdout at session start, so the mute only silences runUpgrade's own writes, not the spinner. Only --upgrade (not the borderline --upgrade-config). The GitHub 'latest' version is scrubbed through an allowlist (upgradeSafeToken) before it reaches the non-sanitizing styled-prompt path, closing a terminal-injection vector. The new menu item shifts the diagnostics group, so the row-order navigation tests are updated. Seams (dashboardUpgradeCheck/Run/Version/Mute) make the screen unit-testable without real network/systemctl; a new test drives Check -> Run -> Back and pins the sanitization.
Rework the upgrade screen's prompt per request: rename the 'Status:' label to 'Last available release:', drop the redundant explanation sub-line, and under it show a summary of what the latest release contains. The summary is the CodeRabbit 'Release Notes' block extracted from the GitHub release body (between its two auto-generated- comment markers). fetchLatestRelease now also returns the release body (bounded by a 512KB LimitReader); UpdateInfo gains a Notes field populated by extractReleaseNotes on the up-to-date and new-version paths. The screen renders the notes via renderReleaseNotes, which - since the notes are remote-controlled and the styled-prompt path is not sanitized by the component - scrubs every line of ANSI/control bytes (sanitizeNotesLine), drops bold markers, emphasizes markdown headers, and caps both line count (16 + ellipsis) and per-line width (100 + ellipsis) so a hostile body can never inject escapes or push the menu off-screen. Tests: extraction (present/absent/unterminated), rendering (control-byte scrubbing, bold strip, count + width caps), and the in-screen render of the notes below the version.
Under 'Last available release: <version>' show, after one blank line, the release page URL (then the release notes). UpdateInfo gains a Tag field (populated by checkForUpdates from the release tag); the screen builds the link via releaseTagURL = a constant base (https://github.com/<githubRepo>/releases/tag/) + the tag scrubbed through the same allowlist as the version, so a spoofed remote tag stays a single safe path segment and cannot inject control bytes into the styled (non-sanitized) prompt. Shown only when a release was read (update-available and up-to-date), cleared on a failed check.
… no symbols
The three 'press Check' screens rendered the pre-check status inconsistently: Telegram
white, Healthchecks yellow WITH a triangle, Upgrade yellow with no symbol. Make Telegram
and Healthchecks match the Upgrade screen: the status is a COLORED keyword with NO symbol
(the ANSI glyphs are dropped), and the pre-check state is yellow ('NOT CHECKED'), turning
green on a positive result, yellow on a warning, red on an error/failure. Telegram keeps a
distinct blue for the 'action needed' state (start bot / send ID). The Phase-7 run section
(a run-log surface, not a check screen) is untouched. Tests updated + no-symbol guards.
…, yellow unchecked
Correct the previous over-removal (no symbols anywhere) into the intended model, applied
uniformly across the three 'press Check' screens (Telegram, Healthchecks, Upgrade):
- pre-check: yellow 'NOT CHECKED', NO symbol (yellow-without-triangle);
- results: green ✓ positive, yellow ⚠ warning/attention/retryable, red ✗ error/fatal.
Exactly three color-symbol pairings; same semantic state renders the same color on every
screen (e.g. 'server unreachable' is yellow ⚠ on both Telegram and Healthchecks; only a
hard fatal is red ✗).
Healthchecks gains a HealthcheckSetupLevelNeutral (front-end-only) so the pre-check has no
symbol without string-sniffing. Telegram's 'action needed' and 'server unreachable' move to
yellow ⚠ (retryable, not blue/red). Upgrade keywords are uppercased ('NOT CHECKED', 'NO
UPGRADE', 'UPGRADED', 'FAILED', ...) and gain the ✓/⚠/✗ symbols on results. Tests updated.
…k screen (R1)
Phase 1 (client). Adds a third healthchecks sensor and surfaces all sensors on the
dashboard check screen.
R2 - updates sensor: the daemon checks GitHub for a newer release every ~5 min and pings
a dedicated check - up-to-date -> /0 (green), update-available -> /1 (fail/red, so the
panel alerts). health.Reporter gains ReportUpdate + updatesURL (self: HEALTHCHECK_UPDATES_
URL/_ID; centralized: CentralizedConfig.Checks["updates"], additive/optional - the
completeness check still requires only alive+backup, so old servers/clients keep working).
The verdict is stored orthogonally to transmission: health.UpdateRecord{Ping, Available,
Latest} + RecordUpdate; a new daemon updateCheckLoop mirrors heartbeatLoop, reuses
checkForUpdates with a quiet logger, and throttles the 'new version' warning to once per
transition. LoadStatus stays tolerant of an old status file (no 'update' key -> nil).
R1 - sensor list: new leaf helper internal/health/sensors.go (SensorLevel + SensorRows,
reusing HumanizeAge) keeps health import-cycle-free; the check screen (buildHealthcheckPrompt)
renders one colored line per sensor (green/yellow/red, unified style) read from the status
file already loaded by CheckHealthcheckConnection (RawStatus/HaveStatus, no extra I/O).
R3: confirmed Healthchecks already dispatches last (extensions.go).
Server-side provisioning of proxsave-updates and the per-notification-channel sensors (R4)
are Phase 2. 3 refuters HOLD; build/vet/gofmt/-race + leaf/no-tview green.
…check + tests Re-check of ae73364 found a real flapping bug and coverage gaps. BUG (updateTick): checkForUpdates collapses 'GitHub unreachable' / rate-limited / empty-latest into UpdateInfo{NewVersion:false, Latest:""}, so a transient GitHub error made the daemon ping /0 (green='up to date') and CLEAR a live /1 (update available). While an update stays uninstalled the check sat DOWN, so every intermittent GitHub failure fired a spurious 'recovered' then 'down' again = alert flapping. A genuine 'up to date' always carries a non-empty Latest, so an empty Latest now marks the check INCONCLUSIVE: updateTick re-affirms the last persisted verdict (or skips the ping when there is no prior verdict) instead of forcing green. Also: introduce a daemonEvaluateUpdate test seam so updateTick is drivable without network; in centralized mode only swap in a rebuilt reporter when it actually HasUpdatesURL (avoid churning the pointer); make the test fakeReporter mirror the real Reporter (ReportUpdate returns ErrNoUpdatesURL when no URL is resolved) and stop snapshot() dropping the update fields (which would have let future update assertions pass vacuously). Tests: daemon_update_test.go drives updateTick through up-to-date/available/still/cleared + the two inconclusive paths (re-affirm, skip) + the no-url swallow; daemon_update_helpers_test.go covers recordUpdate/orUnknownVersion/quietUpdateLogger; healthcheck_setup_rawstatus_test.go asserts RawStatus/HaveStatus. 3 refuters HOLD (mutation-proven); build/vet/gofmt/-race + leaf/no-tview green.
Generalize the healthchecks client plumbing from fixed alive/backup/updates URL fields + fixed Status fields to two dynamic maps, so N per-notification-channel checks (Fase 2B) fit without further churn. No behavior change for the existing sensors. - Reporter: urls map[checkName]baseURL behind a generic Ping(name,suffix,rid,body,label) + HasCheck(name); named wrappers (Heartbeat/RunStarted/RunFinished/RunHang/ReportUpdate) keep their exact sentinels atop the shared pingCheck. Config drops UpdatesURL and gains Checks; NewReporter seeds alive/backup then folds Checks in. Add ErrNoPingURL + IsNoURLErr. - Status: Records map[kind]*PingRecord (keyed by KindHeartbeat/start/finish/hang + notify-<ch>) replaces the 4 fixed fields; Update stays a sidecar. Custom UnmarshalJSON migrates the legacy on-disk format (top-level heartbeat/run_started/run_finished/run_hang -> Records[Kind]) so an in-place daemon upgrade never drops the last outcome on its first read-modify-write. RecordPing default-stores any non-empty kind (dynamic notify kinds now valid). PingRecord gains Down (the /1 signal, omitempty for byte-compat on downgrade). - config.go: shared check-key vocabulary (CheckKeyAlive/Backup/Updates/NotifyPrefix + CheckKeyNotify). - sensors.go: appends a sorted proxsave-notify-<ch> row per notify Records key, Down-aware. - daemon: backupReporter interface += Ping/HasCheck; buildReporter/fetchCentralized/selfURLs thread the Checks map. Backward-compat: completeness check + wire keys frozen; legacy status file migrates on read; Down omitempty keeps update.ping byte-identical on downgrade. 10 new health tests incl. the legacy-shim RMW data-loss guard; build/vet/gofmt/-race + leaf/no-tview green.
…n handoff (Fase 2B / R4)
One healthchecks check per ENABLED notification channel (email/telegram/webhook/gotify),
reporting each channel's send outcome. The child SENDS the notifications (Phase-7) and the
daemon PINGS, so the outcomes cross the process boundary via a per-run handoff file.
Child (internal/orchestrator): capture every channel's outcome into BackupStats.NotifyResults
(display name -> severity) - recordNotifierStatus now records all 4 (Gotify/Webhook were
dropped before), and an enabled-but-unconfigured channel records "error". At the end of
dispatch, persistNotifyResults writes <baseDir>/identity/.notify_results.json {rid,ts,results},
env-gated on PROXSAVE_RUN_ID so only the daemon's supervised child writes (a bare --backup
leaves no file).
Handoff (internal/health, leaf): notify_results.go (NotifyResults + Write/Load, atomic via the
shared writeJSONAtomic extracted from writeStatus) + EnvRunID const (one source, both packages)
+ RecordNotifyPing (stores PingRecord.Down = the /1 signal, orthogonal to OK).
Daemon (cmd/proxsave): buildBackupCmd injects PROXSAVE_RUN_ID (preserving os.Environ()); after
the child's finish ping, reportNotifyOutcomes reads the file, and - DRIVEN BY THE FILE's channel
set, not the daemon's cached config (so a channel toggled off without a restart never false-DOWNs)
- pings one check per channel: ok->/0, warning|error->/1 (any imperfection alerts, per the chosen
policy), disabled->skip. A results file whose rid != this run (stale / crash-before-Phase-7) is
rejected (no ping, no flap); an unresolved check records a no_url trace; nil reporter is guarded;
the centralized re-resolve is hoisted to at most once per run.
Backward-compat: old-daemon/new-child and new-daemon/old-child both degrade cleanly (env absent /
file absent -> rid guard skips). health stays a leaf. 3 refuters HOLD (mutation-proven);
build/vet/gofmt/-race + leaf/no-tview green.
…nnel checks (Fase 2C client) Client half of Fase 2C: the daemon tells the server which notification channels are enabled so the server can provision one check per channel (the server half lands in the proxsave_server repo). - internal/health/config.go: FetchCentralizedConfigWithChannels - a SEPARATE daemon-only variant that sends the authoritative enabled-notification set as ?channels (empty -> the explicit "none" sentinel, never an empty value a proxy could drop). Plain FetchCentralizedConfig is unchanged and sends nothing, so the install-wizard / Phase-7 callers structurally cannot send ?channels. - cmd/proxsave/daemon.go: fetchCentralized derives the set via enabledNotifyChannels(cfg) (sorted email/telegram/gotify/webhook from cfg.*Enabled; metrics excluded; always non-nil). selfURLs resolves per-channel notify checks into checks[notify-<ch>] from HEALTHCHECK_NOTIFY_<CH>_URL/ID (self mode). - internal/config: the 8 HEALTHCHECK_NOTIFY_<CH>_URL/_ID fields. Backward-compat: an old server ignores the unknown ?channels arg and returns no notify checks; the daemon then records a benign no_url per channel (Fase 2B). build/vet/gofmt/-race + leaf/no-tview green; 3 refuters HOLD.
… the daemon
A manual `proxsave --backup` (or the dashboard "run now") now updates the proxsave-backup
sensor - but WITHOUT breaking the single-pinger invariant. The DAEMON stays the only thing that
talks to healthchecks: the standalone run hands its outcome to the daemon and wakes it with
SIGUSR1; the daemon pings + records through its existing finish path. If no daemon is running,
nothing pings (coherent: without the daemon healthchecks transmits nothing at all).
- internal/health: a per-run handoff file identity/.manual_backup_outcome.json {rid,ts,exit_code}
(WriteManualOutcome/LoadManualOutcome/RemoveManualOutcome, atomic, tolerant) + a daemon pidfile
identity/.daemon.pid (WriteDaemonPID/ReadDaemonPID/RemoveDaemonPID) so a run can discover the daemon.
- daemon: publishes its pid + installs a SIGUSR1 handler at startup; on the signal,
processManualOutcome reads the handoff, drops it if stale (>15m), else pings the backup check with
the run's exit code through the SAME finishPing + recordPing (KindRunFinished, statusMu-serialized)
a supervised run uses, then removes the handoff (processed-once). Signal ordering is safe in BOTH
directions: signal.Notify precedes publishing the pid (Go's default SIGUSR1 action is terminate),
and on shutdown the pid is removed before signal.Stop reverts the disposition.
- run side (standalone only, PROXSAVE_RUN_ID unset): if a backup was attempted and a LIVE proxsave
--daemon is found (pid alive + /proc/<pid>/cmdline verified, so a recycled pid is never signalled),
write the handoff + SIGUSR1 the daemon. The daemon's own supervised child (env set) hands off
nothing - the daemon already pings that run in runOnce. No double ping, no cross-process status write.
3 refuters HOLD (found + fixed a startup default-terminate window and a symmetric shutdown one);
build/vet/gofmt/-race + leaf/no-tview green.
…ection (block 1)
Foundation for a single daemon-state check reused by the dashboard AND post-install/upgrade
verification. Detects whether the running daemon is ALIGNED with the on-disk binary or 'behind'
(rebuilt/upgraded without a restart) via SHA256.
- internal/health/binary_identity.go: BinaryIdentity{Path,SHA256,Size,MTime} + ComputeBinaryIdentity
(streaming sha256 + stat) + Aligned (SHA256-primary, content not path; an empty hash never aligns).
cmd/proxsave/runtime_helpers.go executableHash() now delegates here (single sha256 impl).
- internal/health/daemon_info.go: the daemon records its binary identity at startup in a COMPANION
file identity/.daemon_info.json {pid,exec_path,binary,version,commit,start_ts} (WriteDaemonInfo via
the shared writeJSONAtomic; ReadDaemonInfo tolerant; RemoveDaemonInfo idempotent). .daemon.pid (the
SIGUSR1 handoff contract) is left untouched. daemon.go writes it best-effort next to the pidfile at
startup and removes it on shutdown.
- internal/health/daemon_state.go: CheckDaemonState(DaemonStateInput) DaemonState composes existing
primitives (LoadStatus -> Diagnose -> RefineWithPresence) + pid-liveness + the aligned check, with
INJECTED seams (systemd Presence + ProcAlive) so health stays a leaf (no systemctl, no cmd import).
The 'behind' verdict requires a recorded info (HaveInfo); no record = alignment UNKNOWN, never a false
'behind'. The effective PID (pidfile, else the info record) is resolved before the liveness probe.
3 refuters HOLD (one found + fixed a PID/ProcessAlive ordering inconsistency); build/vet/gofmt/-race +
leaf/no-tview green. Next: dashboard render + consolidate the two inline pipelines; then restart+verify
on upgrade/install.
…k pipelines (block 2) Consumes the Stage-1 CheckDaemonState from both daemon-check sites and surfaces binary alignment. - Consolidate the two DUPLICATE inline daemon-diagnosis pipelines (LoadStatus->Diagnose-> RefineWithPresence) onto health.CheckDaemonState: cmd/proxsave/dashboard.go runDashboardDaemonStatus (Presence=daemonPresenceProbe, ProcAlive=probeProxsaveDaemonAlive) and internal/orchestrator/ healthcheck_setup_check.go CheckHealthcheckConnection (ProcAlive=nil, the leaf pidAlive). The dead healthcheckSetupLoadStatus seam is removed; HaveStatus becomes content-based (a missing status file is now honestly 'unknown' rather than 'read OK' - benign, no supported host changes since systemd is always probed there). - Render a DISTINCT 'behind - restart needed' state (dashboard daemonStatusStyle + the post-install classify), separate from the heartbeat-derived 'running, not reporting' (TxRunningNoReport). The dashboard body also shows the running version + 'aligned/BEHIND'. The run-phase self-report is left unchanged (a daemon reporting on itself is aligned by definition). - Robustness (from review): a new DaemonState.AlignChecked distinguishes a KNOWN binary mismatch (real 'behind') from UNKNOWN (no record, empty recorded hash, or an unreadable on-disk binary). Both render gates require AlignChecked, so an undeterminable alignment is never a false 'behind'. 3 refuters HOLD (verified: behavior-preserving diagnosis, ExecPath == the upgrade's dest path so the alignment compares the right binary, no false/missed behind) + hardened the unknown-vs-behind edge. build/vet/gofmt/-race + leaf/no-tview green. Next: restart+verify on upgrade/install (Stage 3).
…install (block 3) Closes the gap where 'proxsave --upgrade' replaced the binary but left the daemon running the old/deleted inode until a manual restart. Now upgrade (CLI + dashboard), install, and the dashboard 'Restart daemon' button restart the daemon and verify it comes back ALIGNED, waiting for any running backup first so a restart never kills one. - cmd/proxsave/daemon_restart_verify.go: restartAndVerifyDaemon(ctx,baseDir,lockFilePath,interval) - (0) if a backup is running, WAIT (bounded ~4m) for it to finish, else defer (BackupWaitTimedOut, no restart); (1) snapshot the daemon's pre-restart StartTS; (2) restartDaemonService; (3) poll CheckDaemonState (~30s) until ProcessAlive && Aligned && AlignChecked && StartTS>pre (a genuinely fresh, aligned, live daemon). verifyDaemonAligned (poll-only) for install; daemonIsActive gate. - internal/checks/backup_probe.go: BackupInProgress - a READ-ONLY probe reusing the existing O_EXCL+PID lock detection (stat + read + signal-0), never creating/holding/removing the lock. Uses the REAL lock path (cfg.LockPath/.backup.lock), so a custom LOCK_PATH host is honored (a review caught a baseDir-only derivation that would have skipped the wait and killed a backup). - Integration: --upgrade (footer 'daemon restarted, now aligned (vX)' / deferred / warning - exit code NEVER changed by the restart), dashboard upgrade (auto, single restart via the upgradeRestartsDaemon gate), install reconcile/applyDaemonMode (best-effort verify), and the 'Restart daemon' button. 3 refuters HOLD (one found + fixed the LOCK_PATH probe hole; freshness gate now mutation-tested); build/vet/gofmt/-race + leaf/no-tview green. Completes the daemon-state/alignment + restart-verify feature.
…the hash machinery Simplify the daemon binary-alignment detection. Linux forbids overwriting a running executable (ETXTBSY), so a rebuilt/upgraded binary is ALWAYS replaced by rename/unlink and shows the '(deleted)' suffix in /proc/<pid>/exe. That single readlink is a complete, record-independent, hash-free 'behind' signal - and it fixes the real gap the recorded-SHA256 approach had: a daemon predating the identity record (or the first deploy) was shown GREEN even while running a stale binary. - cmd/proxsave/proc_binary_stale.go: procBinaryStale(pid) = readlink /proc/<pid>/exe, '(deleted)' suffix -> stale. Injected as the ProcStale seam into CheckDaemonState from the dashboard, the restart-verify, and the orchestrator (DaemonProcStale, wired in main.go). health stays a leaf. - internal/health/daemon_state.go: alignment now comes SOLELY from the injected ProcStale seam (AlignChecked/Aligned/StaleReason); the recorded-hash comparison is gone. 'behind' render gates are AlignChecked && !Aligned (no HaveInfo requirement), so a record-less-but-stale daemon shows behind. - internal/health/daemon_info.go: the record is kept ONLY for version/commit display + the restart-verify freshness gate (StartTS); the Binary hash field is removed and the daemon no longer hashes itself at startup. - Deleted internal/health/binary_identity.go; reverted cmd/proxsave/runtime_helpers.go executableHash to its original local sha256 (buildSignature unchanged). No ComputeBinaryIdentity/BinaryIdentity references remain. 3 refuters HOLD (the record-less old daemon now correctly reads 'behind'; clean removal; version + restart-verify intact). build/vet/gofmt/-race + leaf/no-tview green.
…ealthchecks checks The daemon status screen used a components.Notice box (keyword in the title, plain body lines), visually inconsistent with the Telegram and healthchecks check screens which use a styled prompt above a selector. Rework it to the SAME look: a 'Status: <colored keyword + symbol>' header (green check / yellow warning), a subtle explanation, a Details block, presented via components.NewSelector with WithSelectorPromptStyled and Check/Back items. - daemonStatusStyle now returns an orchestrator.HealthcheckSetupLevel (Ok for running, Warn for the behind/gap states) instead of a components.NoticeKind; keywords + explanations are unchanged. - renderDaemonStatusLevel mirrors the install package's renderHealthcheckLevel palette exactly (theme.SuccessText/WarningText/ErrorText + SymbolSuccess/Warning/Error). - runDashboardDaemonStatus is a selector loop: Check re-runs CheckDaemonState (so the screen flips to aligned/running after an external restart), Back returns. State logic (CheckDaemonState inputs, the behind gate, the detail lines) is IDENTICAL: presentation-only. 2 refuters HOLD (visual parity with the reference; presentation-only, no state-logic change). build/vet/gofmt/-race + leaf/no-tview green.
The daemon menu's action-RESULT screens (Restart daemon, install/revert, the post-upgrade restart, and the upgrade success/failure) used components.Notice boxes, inconsistent with the just-restyled daemon STATUS screen. Render them all with the same styled 'Status: <colored keyword + symbol>' look via a single-Back selector, so the whole daemon menu and the upgrade flow read consistently. - showDaemonResultScreen + buildDaemonResultPrompt: the shared styled result renderer (mirrors buildDaemonStatusPrompt's Status block + renderDaemonStatusLevel palette), presented via components.NewSelector + WithSelectorPromptStyled + WithSelectorBack. - restartVerifyNotice -> restartVerifyStatus: returns an orchestrator.HealthcheckSetupLevel + short keyword + explanation instead of a NoticeKind; the outcome mapping (aligned / deferred / not-confirmed / failed) and all wording are unchanged (Success->Ok, Warning->Warn, Error->Error, no arm flipped). - Converted: runDashboardDaemonRestart, runDashboardDaemonAdmin (install/revert success + both errors), dashboardUpgradeRestartDaemon (active + inactive branches), and the 'Upgrade failed' result. - The RunTask spinners + all logic (restartAndVerifyDaemon, the backup-wait, apply ops, logger muting, the double-restart gate) are untouched: presentation only. Non-daemon Notices left as-is. 3 refuters HOLD (visual parity with the status screen; correct outcome mapping; presentation-only). build/vet/gofmt/-race + leaf/no-tview green.
…only on problems
Per feedback: the restart success screen showed a long redundant line ('The resident daemon
(proxsave-daemon.service) was restarted and is now running the current binary (vX).') that just
repeated the keyword. On a successful outcome the styled result now shows ONLY the 'Status:' line
(the keyword 'restarted, aligned (vX)' already says everything); a what-to-do suggestion appears
only on a problem outcome (deferred / not-confirmed / failed), shortened to one actionable line,
and buildDaemonResultPrompt now puts a blank line between the Status line and that suggestion.
…wizard The install 'configuration' screen had no Healthchecks choice: HEALTHCHECK_ENABLED was set implicitly for daemon mode, HEALTHCHECK_MODE was never written (default centralized), and SELF mode (the user's own healthchecks instance) was unreachable from the wizard (bootstrap skipped it). Now the user picks the mode, and self mode is fully set up from the wizard. - Config form (internal/ui/flows/install/install.go): new 'Healthchecks' select - Off / Centralizzato (nostro) / Server personale (manuale) - active only in daemon mode. ApplyInstallData (internal/installer/install_data.go) now writes HEALTHCHECK_ENABLED + HEALTHCHECK_MODE from the choice (empty+daemon keeps the backward-compat centralized-on default), and resets the ALIVE/BACKUP URL cache symmetrically on every mode switch so a stale self URL never lingers as the centralized cache. - Self params screen (new internal/ui/flows/install/healthcheck_selfparams.go): when 'self' is chosen, a FormGrid collects the FULL ping URLs (alive+backup required, updates + per-notify email/telegram/ gotify/webhook optional; http(s) validated) and writes them before the check. Flow: centralized = Next->check ; self = Next->params->check, in both TUI and CLI. - Self check: bootstrap gains HealthcheckSetupEligibleSelf; CheckHealthcheckSelfConnection + ClassifyHealthcheckSelfResult verify the user's instance is REACHABLE (state-neutral ping to the alive URL via the existing Reporter.TestPing seam) - no server fetch, no magic-link. RunHealthcheckSetup + the CLI setup are self-aware; the centralized path is unchanged. selfURLs() already supported full URLs. - Template (internal/config/templates/backup.env): added the missing self keys (UPDATES_URL/ID, UPDATE_INTERVAL, NOTIFY_*_URL/ID) so the existing template-driven upgrade merge injects them with SAFE empty defaults (assertSafeDaemonDefaults extended); fixed the ALIVE/BACKUP URL comments for self dual use. Built via a build->adversarial-verify workflow: 3 refuters HOLD (config/template-injection; bootstrap+ self-check+no-centralized-regression; flow+CLI+safety) + a LOW parity fix. go build/vet/test/-race + leaf/no-tview green; install characterization goldens regenerated.
…nly)
The healthchecks self-mode wizard shipped with Italian UI labels/descriptions by mistake, and a sweep
found some pre-existing Italian comments/strings too. The repo/code/UI must be English (only user-facing
CHAT is Italian).
- internal/ui/flows/install/healthcheck_selfparams.go + install.go: the self-params field descriptions,
the form title, and the Healthchecks mode select labels are now English ('Off' / 'Centralized (ours)' /
'Your own server'; the config VALUES off/centralized/self are unchanged).
- Pre-existing Italian translated: internal/config/templates/backup.env (FS_IO_TIMEOUT, PVESH_TIMEOUT
comments), internal/config/config.go, internal/orchestrator/restore_zfs.go log line, and a few Italian
comments/fixtures in test files. Non-ASCII test fixtures that the sanitization tests deliberately assert
on (caffé, the emoji sample body) are left as-is.
- Install characterization goldens regenerated for the two translated backup.env comment lines.
go build/vet/test/-race + leaf/no-tview green; a token sweep over cmd/ + internal/ shows no Italian prose.
…'ours') 'Centralized (ours)' was ambiguous - a user downloading proxsave could read 'ours' as their own server. Name the proxsave-hosted option explicitly 'ProxSave HC Server' in the config-form select and its description, and mirror the wording in the CLI mode prompt. The 'Your own server' (self) label and the config VALUES (off/centralized/self) are unchanged. Characterization goldens regenerated.
'Resident daemon (recommended)' read as informal filler. Name it plainly 'ProxSave local daemon' in the scheduler-engine select. The config VALUES (daemon/cron) and the daemon-default behavior are unchanged; the CLI prompt (keyed on the typed 'daemon'/'cron') is already clear and untouched.
…eens
The post-install audit ('Unused components') reported its 5 outcomes as components.Notice boxes,
inconsistent with the unified Telegram/Healthchecks/Daemon check screens. Render them with the same
styled 'Status: <colored keyword + symbol>' + subtle explanation via a selector (new showAuditResult
helper reusing renderHealthcheckLevel), one 'Post-install check' title: no unused components / no
changes / updated (with the disabled list) / check failed / update failed. Success shows just the
Status line (per the daemon-result convention); a blank line separates the explanation. Audit logic
(dry-run, MultiSelect, ApplyAuditDisables, PostInstallAuditResult, non-blocking semantics) unchanged.
1 refuter HOLD (visual parity + presentation-only). build/vet/test/-race + no-tview/leaf green.
…all flow (not 'Back') The restyled audit outcome screen hardcoded a 'Back' leave item, which read wrong in the install/edit flow (you continue forward to the next step, not go back). Thread backToMenu into showAuditResult and label the leave item exactly like the Telegram check that follows: 'Continue' in the install/edit flow, 'Back' (return to the dashboard menu) only when opened from the dashboard diagnostics.
…' console prefix The install finalization mixed two console formats: BootstrapLogger.Info/Warning/Error printed the raw message (no prefix) while the main Logger printed '[2026-07-07 20:55:04] INFO ...'. Now the bootstrap semantic lines carry the SAME prefix. Extracted a shared FormatConsoleLogLine/assembleConsoleLine/ levelColorCode in the logging package (single source of truth, main Logger output byte-identical incl. PHASE/STEP/SKIP labels + color overrides). BootstrapLogger.Info/Warning/Error format the console line via it (stdout for Info, stderr for Warning/Error), TTY+NO_COLOR-aware color, timestamp/padding matching the main Logger. Banners (Println/Printf) stay raw; the recorded entry stays raw so Flush adds exactly one prefix (no double). 1 refuter HOLD (no double prefix, banners raw, byte-identical main logger, correct stream TTY check). build/vet/test/-race + leaf/no-tview green.
After 'Daemon mode enabled: ...' the install polled (up to 30s) for the just-started daemon to come up
process-alive and aligned with the installed binary, with NO console line - a silent pause. Log
'Verifying the daemon started and is aligned with the installed binary...' before the poll (same
progress style as 'Checking dependencies...'), so the user sees what is happening during the wait. The
outcome line ('Daemon verified: ...' or the not-confirmed warning) already follows.
…aemon alignment...'
…-verify warnings The dashboard exists so the user does not run manual commands; the daemon alignment-verify warnings pushed 'check proxsave --daemon-status' / 'restart it manually'. Make them plain outcome statements: install -> 'Daemon started but alignment check timeout' / 'Daemon alignment could not be confirmed'; CLI --upgrade + the restart-verify footer -> the same, without the manual-command suggestion.
…(not a timeout)
Two bugs behind the always-timing-out post-install alignment check:
1. reconcileSchedulerAfterInstall used installDaemonService ('enable --now'), which does NOT restart an
already-running daemon, so a reinstall/reconfigure (or a rebuilt binary) left it on the OLD/deleted
inode. Now it restartDaemonService()s so the running process is the freshly installed binary.
2. verifyDaemonAligned polled until 'ProcessAlive && Aligned && AlignChecked' - so a daemon that is up
but BEHIND (never aligned) polled the full 30s and printed a bare 'alignment check timeout'. It now
polls until 'ProcessAlive && AlignChecked' (the state is ASSESSABLE) and reports the real verdict -
the SAME as 'proxsave --daemon-status': aligned / BEHIND / not-running. A timeout now means only that
the daemon never came up. verifyDaemonAlignedBestEffort logs the three distinct outcomes.
Diagnosed live on the test PVE (/proc/<pid>/exe -> build/proxsave (deleted), NRestarts=0: the daemon was
never restarted after the rebuild). 1 refuter HOLD (no old-daemon race - systemctl restart is synchronous;
restartAndVerifyDaemon untouched; the new test proves behind returns immediately not a timeout).
build/vet/test/-race + leaf/no-tview green.
Shorten the three post-install daemon-verify lines to concise, parallel forms: - 'Daemon verified: running and aligned (vX).' (was '... aligned with the installed binary (vX).') - 'Daemon running but not aligned (behind).' (was '... BEHIND: on an older binary than the one on disk.') - 'Daemon not running.' (was 'Daemon not confirmed running after install.')
…g progress + outcome The install wizard closed the graphical session and ran the finalization (symlinks/cron, daemon install+restart+verify, permissions) as plain CLI log lines. Now those progress lines STREAM inside the graphics and the outcome is shown below, then Continue. Built as reusable BRICKS (install is the first consumer; --upgrade/backup/restore can reuse them): - Brick A (generic UI, internal/ui/components/streamtask.go): StreamTask + RunStreamTask - run a job, stream its lines (bounded tail), show a pre-styled outcome below, wait for Continue. Mirrors the Task plumbing (token + s.Send + shell.Ask + spinner); Task is untouched. - Brick B (generic log capture, internal/logging/capture.go): NewLineWriter (io.Writer -> per-line callback, ANSI-stripped, keeps the [ts] LEVEL prefix) + CaptureConsole(bootstrap, w) -> restore() - the single place that redirects the default + bootstrap loggers to a writer. Local loggers (the temp one in fixPermissionsAfterInstall) accept an io.Writer param (CLI passes nil, unchanged). - Install (cmd/proxsave/install_tui.go): keeps the session open, presents RunStreamTask, captures all three finalization loggers to the stream, computes the outcome (buildInstallOutcomePrompt reusing restartVerifyStatus + renderDaemonStatusLevel + a Permissions line), then Close. The CLI runInstall is unchanged (the new returns are discarded at statement call sites; the permissions param is nil). The footer (printInstallFooter) still prints after Close as the persistent scrollback record. Built via a 3-brick build -> 3-refuter workflow: all HOLD (2 verdicts mislabeled REFUTED but their text says 'no real holes'; findings are LOW/cosmetic). build/vet/test/-race + leaf/no-tview green.
Address the two actionable cosmetic findings from review: StreamTask no longer appends blank/whitespace lines (no empty rows in the stream), and esc while running shows a '(cancelling...)' indicator in the header for parity with the Task component. Tests added.
…other checks
The post-install check opened with a Yes/No Confirm ('Run check (default)' / Skip), with buttons
positioned differently from the Telegram/Healthchecks checks. Replace it with the SAME selector pattern:
a styled prompt ('Status: NOT CHECKED' + intro) above a 'Check' + Skip/Back selector (Back only from the
dashboard). Same layout, same 'Check' label, same pre-check look as the sibling check screens.
Drop the 'Choose Check to run the dry-run...' explanation line and shorten the intro to 'Detect unused backup components.'
… confirmed' The graphical install outcome ran the poll-only verify result through restartVerifyStatus, whose success arm needs Restarted/FreshInfo that verifyDaemonAligned never sets - so it ALWAYS printed 'Daemon: restarted, not confirmed' + 'Open Daemon status to confirm', even when the daemon came up aligned. Extract the shared installVerifyVerdict(rv) -> (level, keyword) mapping (aligned / behind / not-running, the SAME as --daemon-status) and use it in BOTH the log line (verifyDaemonAlignedBestEffort) and the outcome (buildInstallOutcomePrompt), so they can never diverge. No 'open X to confirm' pointer.
…s above The streaming task showed its title (and, once done, the outcome) at the TOP with the log lines below. The app's recaps/summaries live at the BOTTOM: stream the log lines from the top and pin the spinner/title + outcome + Continue hint as a summary block at the bottom, separated by a rule.
…the graphical summary Two consistency fixes on the graphical Finalizing-installation summary: - Duplicate 'enter continue': StreamTask.View rendered the hint in its body AND the frame renders Help() in its help bar (router.go:218), like every other screen -> two hints. Remove the body hint; Help() now returns 'esc cancel' while running / 'enter continue' when done, shown only by the frame (parity with the Task component). The done summary shows the caller's outcome verbatim (no body title or injected symbol - the caller owns styling). - Completion banner: the summary lacked the colored completion banner the CLI footer shows. Extract the shared installBanner(installErr) -> (title, level) from printInstallFooter (the single completed/aborted/ failed extraction point) and reuse it in BOTH the CLI footer (ANSI box) and the graphical summary (renderInstallBanner, themed). The graphical finalization is reached only on the success path, so it shows the green 'Go-based installation completed' banner above the Daemon/Permissions lines; failures/ aborts return early and the footer prints the red/magenta banner. So the two never drift. Tests: body must NOT render 'enter continue' (frame does); banner appears in the outcome; footer variants green. build/vet + components + cmd/proxsave green (3x stable).
…el, not debug CaptureConsole created the bootstrap mirror at LogLevelDebug, and BootstrapLogger.Debug forwards to the mirror UNCONDITIONALLY (no minLevel gate), so every DebugStepBootstrap line leaked into the graphical finalization stream even on a standard run. Create the mirror at the bootstrap's OWN level (New(bootstrap.levelValue(), false)) via a new lock-safe levelValue() getter: a standard run (INFO) filters debug out; a debug run would stream it. The default logger already filters its own logging.Debug by level. Test: bootstrap+default at INFO -> debug lines dropped, info lines kept.
…trings
'Go-based'/'Go Version' distinguished the Go rewrite from the old Bash tool during the transition; it means
nothing to a user now. Rename the completion banners ('Go-based installation/upgrade X' -> 'Installation/
Upgrade X', shared installBanner + printUpgradeFooter), the start-banner subtitle ('ProxSave - Go Version' ->
'ProxSave', 4 sites) and the orchestration log line. Internal code comments keep 'Go-based' (accurate, not
user-facing). Tests updated.
…imeouts The Charm/bubbletea driver tests poll a render buffer for a screen/line with fixed wall-clock deadlines (5-60s). Under the race detector the event loop runs ~an order of magnitude slower, so those deadlines could fire spuriously (false timeouts) even though the logic is correct. Add internal/uitest.Deadline(base) - identity for normal runs, x8 under the -race build tag - and wrap every UI render-poll deadline (waitScreen/waitFor/waitOutput loops + select arms waiting for a UI driver goroutine to resolve) across the 13 Charm driver-test files. Poll TICKS (time.Sleep/NewTicker at ms) and semantic operation-timeout assertions are deliberately left untouched. Since these polls return as soon as the condition is met, the wider deadline is free on the success path and only adds headroom before a genuine hang is reported. Verified: build/vet (normal + -race) clean; the previously-flaky TestDashboardDaemonInstallInSession and the StreamTask driver tests pass under -race in ~2s. (The dominant real-world cause was CPU saturation from running overlapping -race commands - a process-discipline issue, not the tests.)
The dashboard offered two backup runs, a normal one and a debug variant that forced --log-level debug. Drop the debug variant: remove the ActionBackupDebug menu item + constant and its dashboard dispatch case (debug logging stays available via the --log-level flag). Re-navigate the by-row-count driver tests, whose down-counts all shifted up by one once the debug row was gone (TestDashboardActions cases, the in-session Telegram/healthcheck/post-install/restore/daemon navigations, and the daemon restart button test).
…ce, detached Exit Reorganize the dashboard menu groups: - new 'Tools' group holding Restore + Decrypt (moved out of Maintenance); - Maintenance now holds New encryption key, Reconfigure, and 'Updates' (renamed from 'Check for updates') moved up from Diagnostics; - Diagnostics keeps the three check screens; - Exit gets its own divider so it no longer reads as part of the Daemon group. Re-navigate the by-row-count driver tests whose selectable order shifted (Updates now precedes the diagnostics rows) and reorder TestMenuRowOrder's expected list. Row values/dispatch unchanged.
Descriptions were appended right after each label, so they started at a different column per row (following the command-name length). Pad every label to the widest selectable label so all descriptions line up in a single column. Applies to every selector screen (dashboard menu, Telegram/healthcheck checks, etc.).
… Confirm, hung on the Selector When the post-install audit screen became a Selector (Check/Skip items) instead of a Yes/No Confirm, this test kept sending 'tab enter' - Confirm-button navigation. On the Selector 'tab' is a no-op and 'enter' selects the FIRST item (Check), which launched the dry-run against the fake exec path and blocked on the result screen waiting for input the test never sent -> a flaky hang (tab is non-deterministic, so it passed under some runs and hung under others, which is what made the driver test runs slow and inconsistent). Use 'down enter' to select Skip (the 2nd item). Full install+menu packages now run green in ~2s.
…op 'Check' from its items The group already conveys these are checks, so the per-item 'Check' prefix was redundant: Diagnostics -> Diagnostic Checks; 'Check Telegram' -> 'Telegram'; 'Check healthchecks' -> 'Healthchecks'; 'Post-install check' -> 'Post-install'. Labels only (action values, dispatch, and the audit screen title 'Post-install check' are unchanged).
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @tis24dev, your pull request is larger than the review limit of 150000 diff characters
📝 WalkthroughWalkthroughThis PR adds daemon health-state persistence, self and centralized healthcheck flows, update and notification reporting, restart verification, dashboard upgrade/status screens, streamed installation finalization, and supporting UI/logging infrastructure. ChangesHealth and daemon runtime
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant HealthcheckSetup
participant ResidentDaemon
participant HealthReporter
Installer->>HealthcheckSetup: collect mode and ping URLs
HealthcheckSetup->>HealthReporter: test configured reachability
Installer->>ResidentDaemon: install or restart daemon
ResidentDaemon->>HealthReporter: publish heartbeat, update, and notification outcomes
HealthReporter-->>HealthcheckSetup: return healthcheck verdicts
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+126
to
+131
| if configResult.HealthcheckMode == "self" { | ||
| logging.DebugStepBootstrap(bootstrap, "install workflow (cli)", "healthcheck self params") | ||
| if err := runHealthcheckSelfParamsCLI(ctx, reader, baseDir, configPath, bootstrap); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
When the CLI user selects self mode and then skips or aborts the self-parameter prompts, this path continues after writing HEALTHCHECK_ENABLED=true and HEALTHCHECK_MODE=self. The next setup step sees no self URLs and skips, so the daemon can start in self mode with no ping URLs and never report healthchecks.
| // replaces that disposition; only after it returns is it safe for the pid to become | ||
| // discoverable/signallable. SIGUSR1 wakes the daemon to ping a standalone run's handed-off outcome | ||
| // (processManualOutcome). Buffered(1) so a wake is not lost while we are mid-process. | ||
| usr1 := make(chan os.Signal, 1) |
There was a problem hiding this comment.
The manual-backup wake channel only buffers one SIGUSR1, and Go coalesces repeated signals while the buffer is full. If two standalone backups finish close together, both can write a manual outcome but only one wake is delivered; the second outcome is left until a later signal or the stale-window drop, so that backup result is reported late or not at all.
Comment on lines
+793
to
+838
| func (d *daemon) reportNotifyOutcomes(ctx context.Context, r backupReporter, rid string) { | ||
| if !d.cfg.HealthcheckEnabled { | ||
| return | ||
| } | ||
| nr, err := health.LoadNotifyResults(d.cfg.BaseDir) | ||
| if err != nil { | ||
| logging.Debug("daemon: read notify results failed: %v", err) | ||
| return | ||
| } | ||
| if nr.RID != rid || len(nr.Results) == 0 { | ||
| return // stale/missing file, or the child recorded nothing to report | ||
| } | ||
|
|
||
| // Resolve the reporter at most ONCE for the whole run: in centralized mode, if any | ||
| // channel still lacks a resolved check URL, try a single rebuild (mirrors beat's single | ||
| // re-resolve; never a per-channel re-fetch storm). | ||
| if d.cfg.HealthcheckMode == "centralized" && needsNotifyResolve(r, nr.Results) { | ||
| if newR := d.buildReporter(ctx); newR != nil { | ||
| d.setReporter(newR) | ||
| r = newR | ||
| } | ||
| } | ||
|
|
||
| names := make([]string, 0, len(nr.Results)) | ||
| for name := range nr.Results { | ||
| names = append(names, name) | ||
| } | ||
| sort.Strings(names) // deterministic ping/record order | ||
| for _, name := range names { | ||
| suffix, down, skip := severityToSuffix(nr.Results[name]) | ||
| if skip { | ||
| continue // "disabled"/unknown: the child did not really send this channel | ||
| } | ||
| key := health.CheckKeyNotify(name) | ||
| var perr error | ||
| if r == nil || !r.HasCheck(key) { | ||
| perr = health.ErrNoPingURL // not provisioned yet (self/old server) -> no_url trace | ||
| } else { | ||
| perr = r.Ping(ctx, key, suffix, rid, "", key) | ||
| } | ||
| if perr != nil && !health.IsNoURLErr(perr) { | ||
| logging.Debug("daemon: notify ping %s failed: %v", key, perr) | ||
| } | ||
| d.recordNotifyPing(key, down, perr) | ||
| } | ||
| } |
There was a problem hiding this comment.
This reporting path intentionally uses the child's results file instead of the daemon's current enabled-channel config. If a channel is disabled after the reporter was resolved but before a run finishes, the child can still report that channel and this loop pings the cached check URL, keeping a disabled notification check alive or sending a down ping for a channel the operator just turned off.
Comment on lines
+210
to
+229
| func waitForBackupIdle(ctx context.Context, lockFilePath string) bool { | ||
| if !restartVerifyBackupRunning(lockFilePath) { | ||
| return false | ||
| } | ||
| waitCtx, cancel := context.WithTimeout(ctx, backupWaitTimeout) | ||
| defer cancel() | ||
| ticker := time.NewTicker(backupWaitPollInterval) | ||
| defer ticker.Stop() | ||
| for { | ||
| select { | ||
| case <-waitCtx.Done(): | ||
| // Final check: the backup may have freed the lock right at the deadline. | ||
| return restartVerifyBackupRunning(lockFilePath) | ||
| case <-ticker.C: | ||
| if !restartVerifyBackupRunning(lockFilePath) { | ||
| return false | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Restart verification trusts the lock path passed in by the dashboard or upgrade caller. When config loading fails for an install that uses a custom LOCK_PATH, those callers fall back to the default lock file, this wait loop sees no backup in progress, and systemctl restart can kill an active backup that is holding the custom lock.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (17)
internal/logging/bootstrap.go-182-184 (1)
182-184: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winExplicitly discard console-write results.
These unchecked
fmt.Fprintreturns triggererrcheck; console logging can explicitly ignore both results.Proposed fix
- fmt.Fprint(os.Stdout, line) + _, _ = fmt.Fprint(os.Stdout, line) ... - fmt.Fprint(os.Stderr, line) + _, _ = fmt.Fprint(os.Stderr, line) ... - fmt.Fprint(os.Stderr, line) + _, _ = fmt.Fprint(os.Stderr, line)Also applies to: 198-200, 214-216
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/logging/bootstrap.go` around lines 182 - 184, Update the console-writing calls in the affected logging paths, including the blocks around FormatConsoleLogLine, to explicitly discard both return values from fmt.Fprint (bytes written and error). Apply the same change to all noted occurrences while preserving the existing output behavior.Source: Linters/SAST tools
cmd/proxsave/main_update.go-42-46 (1)
42-46: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire the closing marker before returning release notes.
When the end marker is absent, this returns the entire remaining release body rather than a valid marked block.
Proposed fix
rest := body[i+len(coderabbitNotesStart):] - if j := strings.Index(rest, coderabbitNotesEnd); j >= 0 { - rest = rest[:j] + j := strings.Index(rest, coderabbitNotesEnd) + if j < 0 { + return "" } - return strings.TrimSpace(rest) + return strings.TrimSpace(rest[:j])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/proxsave/main_update.go` around lines 42 - 46, Update the release-notes extraction logic around the coderabbitNotesStart and coderabbitNotesEnd markers so it only returns trimmed content when the closing marker is found. If coderabbitNotesEnd is absent, return the established empty or invalid-block result instead of the entire remaining body.cmd/proxsave/daemon_setup.go-108-116 (1)
108-116: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the ineffectual
aligninitialization.Every switch arm overwrites
"unknown", triggering the suppliedineffassignfinding.Proposed fix
- align := "unknown" + var align string🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/proxsave/daemon_setup.go` around lines 108 - 116, Remove the redundant initial assignment to align before the switch in the daemon setup logic. Keep all existing switch cases and their assignments unchanged, since each branch already sets the final value.Source: Linters/SAST tools
internal/orchestrator/restore_zfs.go-107-107 (1)
107-107: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPrint two complete
journalctlcommands.The embedded
ormakes this troubleshooting command invalid when copied verbatim.Proposed fix
- logger.Info(" - journalctl -u zfs-import@<pool-name>.service or import@<pool-name>.service") + logger.Info(" - journalctl -u zfs-import@<pool-name>.service") + logger.Info(" or: journalctl -u import@<pool-name>.service")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/orchestrator/restore_zfs.go` at line 107, Update the informational command output in the restore guidance to print two separate, complete journalctl commands—one for zfs-import@<pool-name>.service and one for import@<pool-name>.service—instead of combining them with “or” on a single command. Preserve the existing logger context and formatting.cmd/proxsave/dashboard_upgrade.go-237-238 (1)
237-238: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTell users to relaunch the dashboard after upgrade.
Restarting the daemon does not replace this running dashboard process, so it continues using the old binary and may offer the same upgrade again.
Proposed fix
level, keyword, explanation := restartVerifyStatus(rv) + explanation += " Relaunch proxsave to use the new version in this dashboard." showDaemonResultScreen(ctx, session, "Daemon restart", level, keyword, explanation)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/proxsave/dashboard_upgrade.go` around lines 237 - 238, Update the upgrade completion flow around restartVerifyStatus and showDaemonResultScreen to explicitly instruct users to relaunch the dashboard after a successful daemon restart, ensuring the message explains that the running dashboard still uses the old binary.cmd/proxsave/daemon_setup.go-187-199 (1)
187-199: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not report an alive daemon as “not running.”
ProcessAlive=truewithAlignChecked=falsecurrently reaches the default branch. Report alignment as unconfirmed instead.Proposed fix
case rv.ProcessAlive && rv.State.AlignChecked: return orchestrator.HealthcheckSetupLevelWarn, "running but not aligned (behind)" + case rv.ProcessAlive: + return orchestrator.HealthcheckSetupLevelWarn, "running, alignment not confirmed" default: return orchestrator.HealthcheckSetupLevelWarn, "not running"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/proxsave/daemon_setup.go` around lines 187 - 199, Update installVerifyVerdict so any ProcessAlive=true result is not reported as “not running”; add an alive-daemon branch for AlignChecked=false that returns a warning with wording indicating alignment is unconfirmed, while preserving the existing aligned and behind outcomes.cmd/proxsave/daemon_restart_verify_test.go-467-473 (1)
467-473: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse the race-aware UI deadline here too.
This dashboard interaction retains a fixed 60-second timeout while the related UI tests use
uitest.Deadline. Scale this wait as well to avoid race-build flakes.-case <-time.After(60 * time.Second): +case <-time.After(uitest.Deadline(60 * time.Second)):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/proxsave/daemon_restart_verify_test.go` around lines 467 - 473, Update the dashboard interaction wait in the test’s select block to use the race-aware uitest.Deadline instead of a fixed 60-second timeout. Preserve the existing handled assertion and timeout failure behavior while scaling the deadline consistently with the related UI tests.internal/logging/capture.go-72-102 (1)
72-102: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore the previous bootstrap mirror instead of always clearing it.
A nested
CaptureConsoleoverwrites the outer mirror, then its restore sets the mirror tonil. Subsequent bootstrap lines bypass the still-active outer capture. Save and restore the previous mirror, matching the default logger’s stack behavior.Proposed approach
var ( + prevMirror *Logger prevQuiet bool - wiredMirror bool wiredQuietOK bool ) ... - bootstrap.SetMirrorLogger(mirror) + prevMirror = bootstrap.SwapMirrorLogger(mirror) ... - if wiredMirror { - bootstrap.SetMirrorLogger(nil) - } + bootstrap.SwapMirrorLogger(prevMirror)Add a lock-protected
SwapMirrorLoggermethod and a nested-capture regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/logging/capture.go` around lines 72 - 102, Update CaptureConsole to preserve and restore the bootstrap logger’s existing mirror during nested captures instead of always calling SetMirrorLogger(nil). Add a lock-protected SwapMirrorLogger method on the bootstrap logger, use it to save the previous mirror when wiring the capture and restore that exact mirror during cleanup, and add a regression test covering nested CaptureConsole behavior.internal/checks/backup_probe_test.go-59-64 (1)
59-64: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuarantee that the test lock belongs to a foreign host.
A runner named
some-other-hosttakes the local-host PID branch instead of testing age fallback. Derive the value from the actual hostname.Proposed fix
dir := t.TempDir() - p := writeLock(t, dir, "pid=4242\nhost=some-other-host\ntime=now\n") + host, err := os.Hostname() + if err != nil { + t.Fatalf("hostname: %v", err) + } + p := writeLock(t, dir, fmt.Sprintf("pid=4242\nhost=%s-foreign\ntime=now\n", host))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/checks/backup_probe_test.go` around lines 59 - 64, Update TestBackupInProgressForeignHostAgeFallback and its writeLock fixture to derive the lock’s host value from the actual local hostname, then ensure it is different from that hostname so the foreign-host age-fallback path is exercised rather than the local-host PID branch.internal/ui/flows/install/audit.go-127-131 (1)
127-131: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn the configuration update failure to the caller.
The failure is displayed but returned as
(result, nil), sorunInstallTUIlogs “none disabled” instead of its existing non-blocking error warning.Proposed fix
- if err := installer.ApplyAuditDisables(configPath, keys); err != nil { + if applyErr := installer.ApplyAuditDisables(configPath, keys); applyErr != nil { showAuditResult(ctx, session, "Post-install check", orchestrator.HealthcheckSetupLevelError, - "update failed", err.Error(), backToMenu) - return result, nil + "update failed", applyErr.Error(), backToMenu) + return result, applyErr }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ui/flows/install/audit.go` around lines 127 - 131, The installer audit update failure is currently swallowed after display. In the ApplyAuditDisables error branch within the install flow, return the existing result together with err instead of nil so runInstallTUI receives and logs the non-blocking error warning.internal/orchestrator/healthcheck_setup_check.go-30-43 (1)
30-43: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate these field comments to match the new daemon-state contract.
DaemonReadnow means systemd was probed or readable status content exists.DaemonAlignCheckednow means the/procprobe returned a verdict; it no longer requires an identity record or hash comparison.Proposed documentation fix
- DaemonRead bool // false only if the status file existed but could not be read + DaemonRead bool // true when systemd was probed or readable status content exists ... - // on disk?". DaemonAligned is a real comparison only when DaemonAlignChecked is true (a record - // was found, its recorded hash was non-empty, AND the on-disk binary re-hashed). When + // on disk?". DaemonAligned is meaningful only when DaemonAlignChecked is true, + // indicating that the /proc staleness probe returned a verdict. When🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/orchestrator/healthcheck_setup_check.go` around lines 30 - 43, The comments for DaemonRead and DaemonAlignChecked in the healthcheck result structure describe the old contract. Update them to state that DaemonRead is true when systemd was probed or readable status content exists, and DaemonAlignChecked is true when the /proc probe returned a verdict, without requiring an identity record or hash comparison.internal/health/daemon_pid.go-55-60 (1)
55-60: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject non-positive PID values.
Atoiaccepts0and negative integers, allowing malformed numeric contents into the daemon-signaling path. Treat these as corrupt PID files.Proposed fix
pid, err := strconv.Atoi(s) if err != nil { return 0, fmt.Errorf("parse daemon pid %q: %w", s, err) } + if pid <= 0 { + return 0, fmt.Errorf("parse daemon pid %q: pid must be positive", s) + } return pid, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/health/daemon_pid.go` around lines 55 - 60, Update the PID parsing flow after strconv.Atoi in the daemon PID reader to reject values less than or equal to zero as corrupt, returning an error consistent with the existing parse failure handling; only positive PIDs should reach the successful return path.cmd/proxsave/daemon_notify_test.go-81-83 (1)
81-83: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck the raw ping count before indexing by name.
pingByNamecollapses duplicate channel pings, so this test can miss duplicate transmissions despite claiming one ping per channel.Proposed fix
- pings := pingByName(rep.snapshot().pings) - if len(pings) != 3 { - t.Fatalf("want 3 transmitted pings, got %d: %#v", len(pings), pings) + got := rep.snapshot().pings + if len(got) != 3 { + t.Fatalf("want 3 transmitted pings, got %d: %#v", len(got), got) } + pings := pingByName(got)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/proxsave/daemon_notify_test.go` around lines 81 - 83, Update the test around the ping collection to validate the raw transmitted ping count before calling pingByName, asserting the expected total of three. Keep the existing per-channel assertions after this check, using pingByName only for channel-based validation so duplicate transmissions cannot be hidden.cmd/proxsave/dashboard.go-525-534 (1)
525-534: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the ineffectual
alignassignment.Every switch branch overwrites the initial value, triggering
ineffassign.Proposed fix
- align := "unknown" - switch { - case !ds.AlignChecked: - align = "unknown" - case ds.Aligned: - align = "aligned" - default: - align = "BEHIND (restart needed)" + align := "unknown" + if ds.AlignChecked { + if ds.Aligned { + align = "aligned" + } else { + align = "BEHIND (restart needed)" + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/proxsave/dashboard.go` around lines 525 - 534, Remove the initial align assignment before the switch in the dashboard alignment logic, since every branch already assigns a value. Preserve the existing assignments in the !ds.AlignChecked, ds.Aligned, and default branches.Source: Linters/SAST tools
cmd/proxsave/daemon_notify_test.go-211-214 (1)
211-214: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
exec.CommandContexthere. The callback already receives acontext.Context, so propagating it keeps cancellation and avoids thenoctxissue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/proxsave/daemon_notify_test.go` around lines 211 - 214, Update the command factory callback in TestBuildBackupCmdSetsRunIDEnv to construct the command with exec.CommandContext using its provided context instead of exec.Command, preserving the existing "true" command and ensuring cancellation is propagated.Source: Linters/SAST tools
cmd/proxsave/dashboard_test.go-140-155 (1)
140-155: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that each dashboard action sets only its intended mode.
These cases pass if the expected flag and one or more competing flags are all set, masking ambiguous downstream dispatch.
Proposed approach
{"restore", "down enter", false, func(t *testing.T, args *cli.Args) { - if !args.Restore { - t.Fatal("restore flag not set") + if !args.Restore || args.Decrypt || args.ForceNewKey || args.Install { + t.Fatalf("restore mode flags are not exclusive: %+v", args) } }},Apply the equivalent exclusivity check to decrypt, newkey, and reconfigure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/proxsave/dashboard_test.go` around lines 140 - 155, Update the dashboard action assertions in the test cases for decrypt, newkey, and reconfigure so each verifies its intended flag is set and all competing mode flags remain unset. Match the exclusivity validation already used by the restore case, preserving the existing action inputs and expected mode checks.cmd/proxsave/upgrade.go-748-753 (1)
748-753: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude configuration migration failure in the footer status.
A failed configuration upgrade returns a failure exit code but still renders a green “Upgrade completed” banner. Include
cfgUpgradeErrin this condition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/proxsave/upgrade.go` around lines 748 - 753, Update the footer status condition around upgradeErr so cfgUpgradeErr also selects the red “Upgrade failed” title and color. Preserve the existing success banner only when both upgradeErr and cfgUpgradeErr are nil.
🧹 Nitpick comments (3)
internal/config/config.go (1)
257-270: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTypo in comments: "Fase" instead of "Phase".
Since Italian comments are actively being migrated to English (as noted at line 988), consider updating "Fase" to "Phase" here as well.
📝 Proposed fix
- // Fase 1 updates sensor (daemon): whether a newer release is available. + // Phase 1 updates sensor (daemon): whether a newer release is available. HealthcheckUpdatesURL string // self mode: full updates check ping URL (optional) HealthcheckUpdatesID string // self mode: UUID or slug of the updates check HealthcheckUpdateInterval time.Duration // updates-check cadence (default 5m) - // Fase 2 per-notification-channel sensors (self mode only): full URL or check ID per + // Phase 2 per-notification-channel sensors (self mode only): full URL or check ID per // channel. Centralized mode resolves these from the server, not from config. HealthcheckNotifyEmailURL string🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/config.go` around lines 257 - 270, Update the comments above the healthcheck update and notification-channel fields to use “Phase” instead of “Fase,” keeping the existing English wording and field declarations unchanged.internal/health/daemon_pid_test.go (1)
40-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix copy-paste artifact in error message.
The failure message specifies "mkdir identity dir", but this block creates the directory for the PID file.
📝 Proposed fix
path := DaemonPIDPath(base) if err := os.MkdirAll(filepath.Dir(path), 0o750); err != nil { - t.Fatalf("mkdir identity dir: %v", err) + t.Fatalf("mkdir pid dir: %v", err) } if err := os.WriteFile(path, []byte(content), 0o600); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/health/daemon_pid_test.go` around lines 40 - 53, Correct the mkdir failure message in the ReadDaemonPID test setup to refer to creating the PID file’s directory instead of an identity directory. Update only the t.Fatalf message associated with os.MkdirAll; keep the test logic unchanged.cmd/proxsave/daemon_restart_verify_test.go (1)
285-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the production restart decision instead of restating it.
This test only evaluates
upgradeRestartsDaemon && daemonIsActive(...)locally, so it still passes if the upgrade path stops using that gate. Drive the production upgrade decision through its seams or extract the predicate into a production helper and test that helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/proxsave/daemon_restart_verify_test.go` around lines 285 - 291, Replace the local boolean assertions in the restart test with an invocation of the production upgrade/restart decision path, using its existing seams to configure daemon activity and dashboard suppression. Alternatively, extract the predicate into a production helper and test that helper for both enabled and suppressed upgrade cases, ensuring the test fails if production stops applying the restart gate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/proxsave/backup_healthcheck.go`:
- Around line 54-64: The manual backup handoff currently probes and signals a
PID separately, allowing PID reuse and accepting unrelated processes whose
command line merely contains “proxsave.” Update the handoff logic around
daemonAliveProbe and syscall.Kill to validate the daemon identity using argv[0]
or /proc/<pid>/exe, then open and use a pidfd for signaling so the probe and
signal remain bound to the same process.
In `@cmd/proxsave/daemon_restart_verify.go`:
- Around line 137-149: Update both polling loops in the restart verification
flow to pass the bounded pollCtx, rather than the parent ctx, to
daemonPresenceProbe when constructing health.DaemonStateInput. Preserve the
existing timeout and cancellation behavior established by context.WithTimeout.
In `@cmd/proxsave/daemon.go`:
- Around line 809-813: Update the reporter replacement logic in the
centralized-healthcheck branch around needsNotifyResolve and buildReporter so it
compares the rebuilt reporter’s required-check coverage with the existing
reporter before calling setReporter. Preserve r when the refresh reduces
coverage; only assign newR when coverage increases or remains sufficient,
preventing valid channels from being recorded as no_url.
- Around line 728-731: Update the URL resolution logic in the daemon healthcheck
configuration method so HealthcheckAliveURL and HealthcheckBackupURL are
resolved independently: return each configured full URL verbatim when present,
otherwise build that endpoint from its corresponding ID. Preserve the existing
checks result and avoid using a single condition that bypasses resolution for
both endpoints.
In `@cmd/proxsave/dashboard.go`:
- Around line 217-229: The runDashboardDaemonRestart setup must not continue
with the fallback lock path when daemonStatusLoadConfig fails or returns no
configuration. Handle that configuration error by reporting it and returning
before the restart/idle-check flow; only derive interval, baseDir, and lockPath
from a successfully loaded config via backupLockFilePath.
- Around line 417-446: Update the daemon-state check in the
health.CheckDaemonState call to use the successfully loaded cfg.BaseDir when it
is configured, rather than always passing the detected baseDir. Preserve the
existing detected-directory fallback when no valid configuration is loaded, and
leave the remaining DaemonStateInput fields unchanged.
In `@cmd/proxsave/install_outcome.go`:
- Around line 34-44: Replace the boolean-based branch in the install outcome
rendering with an explicit scheduler outcome that distinguishes cron scheduling
from daemon-install failure and unreadable verification context. Update the
surrounding caller and relevant outcome symbols so each unverified case renders
its actual scheduler status or failure message, while preserving
installVerifyVerdict for verified daemon outcomes.
In `@cmd/proxsave/install.go`:
- Around line 529-550: The healthcheck mode switch must preserve existing self
endpoints and explicitly persist off mode. In the self branch of the hcMode
switch, stop clearing HEALTHCHECK_ALIVE_URL and HEALTHCHECK_BACKUP_URL so
runHealthcheckSelfParamsCLI can prefill and retain them, including on abort;
keep centralized clearing both URLs. In the default/off branch, also set
HEALTHCHECK_MODE to "off" alongside disabling healthchecks and clearing the
URLs.
In `@cmd/proxsave/upgrade.go`:
- Around line 245-258: Include cfgUpgradeErr in the upgrade success gating: in
the daemon migration/restart flow around maybeAutoMigrateDaemon and
restartAndVerifyDaemon, proceed only when both upgradeErr and cfgUpgradeErr are
nil. In cmd/proxsave/upgrade.go lines 748-753, update the footer title/color
selection so either non-nil error produces the failed completion state.
- Around line 75-86: Update upgradeBackupLockPath to fail closed when
config.LoadConfigWithBaseDir cannot load the configuration: return the loading
error alongside the path, or reuse a previously successful configuration. Adjust
the daemon-restart backup-wait flow to skip the restart when the lock path
cannot be resolved, and remove the fallback to backupLockFilePath with nil.
In `@internal/checks/backup_probe.go`:
- Around line 50-52: Update the lock inspection error handling around osStat in
the backup probe so only os.IsNotExist(err) returns false for a missing lock;
treat all other errors as busy, allowing the existing bounded wait to defer
restart rather than proceeding on inconclusive inspection failures.
In `@internal/health/config.go`:
- Around line 90-97: Update FetchCentralizedConfigWithChannels so a nil channels
slice is normalized to a non-nil empty slice before calling fetchConfig, causing
the daemon-specific API to send the “none” sentinel. Preserve nil-as-omission
behavior in the plain FetchCentralizedConfig API.
In `@internal/health/daemon_state.go`:
- Around line 146-158: The pidAlive function currently treats every non-nil
signal(0) error as dead, but EPERM indicates the process is alive and merely not
signalable. Update pidAlive to return true for syscall.EPERM while preserving
nil as alive and other errors as dead, adding the errors import needed to
identify the condition.
In `@internal/health/reporter.go`:
- Around line 107-110: Update the loop over c.Checks in the health reporter to
ignore or reject the reserved keys "alive" and "backup" before adding normalized
URLs to urls. Preserve processing for all other documented “OTHER” checks,
ensuring the dedicated core ping URLs cannot be overridden.
In `@internal/health/status.go`:
- Around line 71-100: The Status serialization must preserve downgrade
compatibility by emitting the legacy top-level heartbeat, run_started,
run_finished, and run_hang fields alongside records and update. Update
Status.MarshalJSON to derive those fields from Records while retaining the new
schema; otherwise explicitly remove the legacy migration compatibility if
downgrades are not supported.
In `@internal/installer/install_data.go`:
- Around line 246-280: Update the healthcheck mode handling around
normalizeHealthcheckMode and the hcMode switch to track the previous effective
mode, preserve all URL values when the mode is unchanged, and clear every
HEALTHCHECK_*_URL field only when transitioning between modes or disabling
healthchecks. Ensure self-to-self edits retain URLs for
RunHealthcheckSelfParams, while leaving self clears required and optional URLs;
add regression tests covering both behaviors.
In `@internal/orchestrator/decrypt_charm_e2e_test.go`:
- Line 290: Update the workflow context timeout near the decrypt charm E2E
test’s watchdog to use the same race-scaled duration as the
time.After(uitest.Deadline(3 * time.Minute)) branch. Ensure the context deadline
remains aligned with the scaled outer wait so race runs do not cancel
prematurely.
In `@internal/ui/components/streamtask.go`:
- Around line 98-123: Preserve cancellation errors across all StreamTask
finalization paths: in internal/ui/components/streamtask.go lines 98-123, keep
Esc cancellation when the Resolve callback returns nil; in lines 195-200, return
the shell abort when runErr is nil. In cmd/proxsave/install_tui.go lines
321-359, return taskCtx.Err() and map cancellation to an aborted install rather
than only logging it, so stream errors cannot be discarded as success.
---
Minor comments:
In `@cmd/proxsave/daemon_notify_test.go`:
- Around line 81-83: Update the test around the ping collection to validate the
raw transmitted ping count before calling pingByName, asserting the expected
total of three. Keep the existing per-channel assertions after this check, using
pingByName only for channel-based validation so duplicate transmissions cannot
be hidden.
- Around line 211-214: Update the command factory callback in
TestBuildBackupCmdSetsRunIDEnv to construct the command with exec.CommandContext
using its provided context instead of exec.Command, preserving the existing
"true" command and ensuring cancellation is propagated.
In `@cmd/proxsave/daemon_restart_verify_test.go`:
- Around line 467-473: Update the dashboard interaction wait in the test’s
select block to use the race-aware uitest.Deadline instead of a fixed 60-second
timeout. Preserve the existing handled assertion and timeout failure behavior
while scaling the deadline consistently with the related UI tests.
In `@cmd/proxsave/daemon_setup.go`:
- Around line 108-116: Remove the redundant initial assignment to align before
the switch in the daemon setup logic. Keep all existing switch cases and their
assignments unchanged, since each branch already sets the final value.
- Around line 187-199: Update installVerifyVerdict so any ProcessAlive=true
result is not reported as “not running”; add an alive-daemon branch for
AlignChecked=false that returns a warning with wording indicating alignment is
unconfirmed, while preserving the existing aligned and behind outcomes.
In `@cmd/proxsave/dashboard_test.go`:
- Around line 140-155: Update the dashboard action assertions in the test cases
for decrypt, newkey, and reconfigure so each verifies its intended flag is set
and all competing mode flags remain unset. Match the exclusivity validation
already used by the restore case, preserving the existing action inputs and
expected mode checks.
In `@cmd/proxsave/dashboard_upgrade.go`:
- Around line 237-238: Update the upgrade completion flow around
restartVerifyStatus and showDaemonResultScreen to explicitly instruct users to
relaunch the dashboard after a successful daemon restart, ensuring the message
explains that the running dashboard still uses the old binary.
In `@cmd/proxsave/dashboard.go`:
- Around line 525-534: Remove the initial align assignment before the switch in
the dashboard alignment logic, since every branch already assigns a value.
Preserve the existing assignments in the !ds.AlignChecked, ds.Aligned, and
default branches.
In `@cmd/proxsave/main_update.go`:
- Around line 42-46: Update the release-notes extraction logic around the
coderabbitNotesStart and coderabbitNotesEnd markers so it only returns trimmed
content when the closing marker is found. If coderabbitNotesEnd is absent,
return the established empty or invalid-block result instead of the entire
remaining body.
In `@cmd/proxsave/upgrade.go`:
- Around line 748-753: Update the footer status condition around upgradeErr so
cfgUpgradeErr also selects the red “Upgrade failed” title and color. Preserve
the existing success banner only when both upgradeErr and cfgUpgradeErr are nil.
In `@internal/checks/backup_probe_test.go`:
- Around line 59-64: Update TestBackupInProgressForeignHostAgeFallback and its
writeLock fixture to derive the lock’s host value from the actual local
hostname, then ensure it is different from that hostname so the foreign-host
age-fallback path is exercised rather than the local-host PID branch.
In `@internal/health/daemon_pid.go`:
- Around line 55-60: Update the PID parsing flow after strconv.Atoi in the
daemon PID reader to reject values less than or equal to zero as corrupt,
returning an error consistent with the existing parse failure handling; only
positive PIDs should reach the successful return path.
In `@internal/logging/bootstrap.go`:
- Around line 182-184: Update the console-writing calls in the affected logging
paths, including the blocks around FormatConsoleLogLine, to explicitly discard
both return values from fmt.Fprint (bytes written and error). Apply the same
change to all noted occurrences while preserving the existing output behavior.
In `@internal/logging/capture.go`:
- Around line 72-102: Update CaptureConsole to preserve and restore the
bootstrap logger’s existing mirror during nested captures instead of always
calling SetMirrorLogger(nil). Add a lock-protected SwapMirrorLogger method on
the bootstrap logger, use it to save the previous mirror when wiring the capture
and restore that exact mirror during cleanup, and add a regression test covering
nested CaptureConsole behavior.
In `@internal/orchestrator/healthcheck_setup_check.go`:
- Around line 30-43: The comments for DaemonRead and DaemonAlignChecked in the
healthcheck result structure describe the old contract. Update them to state
that DaemonRead is true when systemd was probed or readable status content
exists, and DaemonAlignChecked is true when the /proc probe returned a verdict,
without requiring an identity record or hash comparison.
In `@internal/orchestrator/restore_zfs.go`:
- Line 107: Update the informational command output in the restore guidance to
print two separate, complete journalctl commands—one for
zfs-import@<pool-name>.service and one for import@<pool-name>.service—instead of
combining them with “or” on a single command. Preserve the existing logger
context and formatting.
In `@internal/ui/flows/install/audit.go`:
- Around line 127-131: The installer audit update failure is currently swallowed
after display. In the ApplyAuditDisables error branch within the install flow,
return the existing result together with err instead of nil so runInstallTUI
receives and logs the non-blocking error warning.
---
Nitpick comments:
In `@cmd/proxsave/daemon_restart_verify_test.go`:
- Around line 285-291: Replace the local boolean assertions in the restart test
with an invocation of the production upgrade/restart decision path, using its
existing seams to configure daemon activity and dashboard suppression.
Alternatively, extract the predicate into a production helper and test that
helper for both enabled and suppressed upgrade cases, ensuring the test fails if
production stops applying the restart gate.
In `@internal/config/config.go`:
- Around line 257-270: Update the comments above the healthcheck update and
notification-channel fields to use “Phase” instead of “Fase,” keeping the
existing English wording and field declarations unchanged.
In `@internal/health/daemon_pid_test.go`:
- Around line 40-53: Correct the mkdir failure message in the ReadDaemonPID test
setup to refer to creating the PID file’s directory instead of an identity
directory. Update only the t.Fatalf message associated with os.MkdirAll; keep
the test logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1da4fe9f-2d12-4e06-ba12-dc365901f08d
📒 Files selected for processing (115)
cmd/proxsave/backup_healthcheck.gocmd/proxsave/backup_healthcheck_test.gocmd/proxsave/backup_mode.gocmd/proxsave/daemon.gocmd/proxsave/daemon_manual_outcome_test.gocmd/proxsave/daemon_notify_channels_test.gocmd/proxsave/daemon_notify_test.gocmd/proxsave/daemon_restart_verify.gocmd/proxsave/daemon_restart_verify_test.gocmd/proxsave/daemon_setup.gocmd/proxsave/daemon_setup_test.gocmd/proxsave/daemon_test.gocmd/proxsave/daemon_update_helpers_test.gocmd/proxsave/daemon_update_test.gocmd/proxsave/dashboard.gocmd/proxsave/dashboard_test.gocmd/proxsave/dashboard_upgrade.gocmd/proxsave/dashboard_upgrade_test.gocmd/proxsave/decrypt_cmd.gocmd/proxsave/healthcheck_setup_cli.gocmd/proxsave/healthcheck_setup_cli_test.gocmd/proxsave/install.gocmd/proxsave/install_characterization_test.gocmd/proxsave/install_finalize_stream_test.gocmd/proxsave/install_outcome.gocmd/proxsave/install_test.gocmd/proxsave/install_tui.gocmd/proxsave/main.gocmd/proxsave/main_modes.gocmd/proxsave/main_runtime.gocmd/proxsave/main_update.gocmd/proxsave/newkey_charm_test.gocmd/proxsave/permissions.gocmd/proxsave/proc_binary_stale.gocmd/proxsave/proc_binary_stale_test.gocmd/proxsave/testdata/install_characterization/edit_noop.envcmd/proxsave/testdata/install_characterization/fresh_decline_all.envcmd/proxsave/testdata/install_characterization/fresh_decline_all.transcriptcmd/proxsave/testdata/install_characterization/fresh_enable_all.envcmd/proxsave/testdata/install_characterization/fresh_enable_all.transcriptcmd/proxsave/upgrade.gointernal/checks/backup_probe.gointernal/checks/backup_probe_test.gointernal/cli/args.gointernal/config/config.gointernal/config/scheduler_healthcheck_test.gointernal/config/templates/backup.envinternal/health/config.gointernal/health/config_checks_test.gointernal/health/daemon_info.gointernal/health/daemon_info_test.gointernal/health/daemon_pid.gointernal/health/daemon_pid_test.gointernal/health/daemon_state.gointernal/health/daemon_state_test.gointernal/health/diagnose.gointernal/health/diagnose_test.gointernal/health/manual_outcome.gointernal/health/manual_outcome_test.gointernal/health/notify_results.gointernal/health/notify_results_test.gointernal/health/reporter.gointernal/health/reporter_checks_test.gointernal/health/reporter_test.gointernal/health/reporter_update_test.gointernal/health/sensors.gointernal/health/sensors_test.gointernal/health/status.gointernal/health/status_migration_test.gointernal/health/status_test.gointernal/health/update_status_test.gointernal/identity/identity_test.gointernal/installer/install_data.gointernal/installer/install_data_healthcheck_test.gointernal/logging/bootstrap.gointernal/logging/capture.gointernal/logging/capture_test.gointernal/logging/format_console_test.gointernal/logging/logger.gointernal/notify/email_delivery_methods_test.gointernal/orchestrator/decrypt_charm_e2e_test.gointernal/orchestrator/directory_recreation_test.gointernal/orchestrator/extensions.gointernal/orchestrator/healthcheck_section_test.gointernal/orchestrator/healthcheck_setup_bootstrap.gointernal/orchestrator/healthcheck_setup_check.gointernal/orchestrator/healthcheck_setup_classify.gointernal/orchestrator/healthcheck_setup_rawstatus_test.gointernal/orchestrator/healthcheck_setup_self_test.gointernal/orchestrator/healthcheck_setup_test.gointernal/orchestrator/notification_adapter.gointernal/orchestrator/notify_results_handoff_test.gointernal/orchestrator/orchestrator.gointernal/orchestrator/restore_zfs.gointernal/orchestrator/telegram_setup_classify.gointernal/ui/components/selector.gointernal/ui/components/streamtask.gointernal/ui/components/streamtask_test.gointernal/ui/components/taskview_test.gointernal/ui/flows/agesetup/agesetup_test.gointernal/ui/flows/install/audit.gointernal/ui/flows/install/healthcheck.gointernal/ui/flows/install/healthcheck_selfparams.gointernal/ui/flows/install/healthcheck_selfparams_test.gointernal/ui/flows/install/healthcheck_sensors_test.gointernal/ui/flows/install/healthcheck_test.gointernal/ui/flows/install/install.gointernal/ui/flows/install/install_test.gointernal/ui/flows/install/telegram.gointernal/ui/flows/menu/menu.gointernal/ui/flows/menu/menu_test.gointernal/ui/shell/ask_test.gointernal/uitest/deadline.gointernal/uitest/deadline_norace.gointernal/uitest/deadline_race.go
Comment on lines
+54
to
+64
| if pid <= 0 || !daemonAliveProbe(pid) { | ||
| logging.Debug("manual backup handoff: no live proxsave daemon to signal (pid=%d)", pid) | ||
| return | ||
| } | ||
|
|
||
| if err := health.WriteManualOutcome(baseDir, health.NewRunID(), time.Now().Unix(), res.exitCode); err != nil { | ||
| logging.Debug("manual backup handoff: write outcome failed: %v", err) | ||
| return | ||
| } | ||
| if err := syscall.Kill(pid, syscall.SIGUSR1); err != nil { | ||
| logging.Debug("manual backup handoff: signal daemon pid=%d failed: %v", pid, err) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
# Inspect the declared Go toolchain and existing pidfd support/dependencies.
fd -a 'go.mod|go.work|go.work.sum' . -x sh -c 'echo "### $1"; sed -n "1,160p" "$1"' sh {}
rg -n -C3 '\b(PidfdOpen|PidfdSendSignal|pidfd_open|pidfd_send_signal)\b' .
rg -n -C3 'golang\.org/x/sys/unix' --glob '*.go' --glob 'go.mod'Repository: tis24dev/proxsave
Length of output: 1704
🏁 Script executed:
#!/bin/bash
set -eu
# Inspect the relevant file and the helper used to identify the daemon process.
ast-grep outline cmd/proxsave/backup_healthcheck.go --view expanded || true
sed -n '1,220p' cmd/proxsave/backup_healthcheck.go | cat -n
printf '\n### references to daemonAliveProbe and process matching\n'
rg -n -C4 'daemonAliveProbe|proxsave|strings\.Contains|--daemon|argv|exe|proc/' cmd/proxsave -g '*.go'
printf '\n### pidfd support in installed x/sys references\n'
rg -n -C2 '\bPidfd(Open|SendSignal)\b|\bPidfd(Open|SendSignal)\b' .Repository: tis24dev/proxsave
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
ast-grep outline cmd/proxsave/backup_healthcheck.go --view expanded || true
sed -n '1,220p' cmd/proxsave/backup_healthcheck.go | cat -n
printf '\n### references to daemonAliveProbe and process matching\n'
rg -n -C4 'daemonAliveProbe|proxsave|strings\.Contains|--daemon|argv|exe|proc/' cmd/proxsave -g '*.go'
printf '\n### pidfd support in installed x/sys references\n'
rg -n -C2 '\bPidfd(Open|SendSignal)\b|\bPidfd(Open|SendSignal)\b' .Repository: tis24dev/proxsave
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '1,220p' cmd/proxsave/backup_healthcheck.go | cat -nRepository: tis24dev/proxsave
Length of output: 5418
Bind the handoff to the same process. probeProxsaveDaemonAlive still accepts any cmdline arg containing proxsave, and the separate PID probe leaves a reuse window before syscall.Kill. Use a pidfd for signaling and narrow the identity check to argv[0] or /proc/<pid>/exe.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/proxsave/backup_healthcheck.go` around lines 54 - 64, The manual backup
handoff currently probes and signals a PID separately, allowing PID reuse and
accepting unrelated processes whose command line merely contains “proxsave.”
Update the handoff logic around daemonAliveProbe and syscall.Kill to validate
the daemon identity using argv[0] or /proc/<pid>/exe, then open and use a pidfd
for signaling so the probe and signal remain bound to the same process.
Comment on lines
+137
to
+149
| pollCtx, cancel := context.WithTimeout(ctx, restartVerifyTimeout) | ||
| defer cancel() | ||
| ticker := time.NewTicker(restartVerifyTick) | ||
| defer ticker.Stop() | ||
| for { | ||
| st := restartVerifyDaemonState(health.DaemonStateInput{ | ||
| BaseDir: baseDir, | ||
| HeartbeatInterval: interval, | ||
| Now: time.Now(), | ||
| Presence: daemonPresenceProbe(ctx), | ||
| ProcAlive: probeProxsaveDaemonAlive, | ||
| ProcStale: procBinaryStaleProbe, | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use the bounded polling context for presence probes.
Both loops pass the parent ctx to daemonPresenceProbe, so a slow systemctl call can block beyond restartVerifyTimeout. Pass pollCtx to preserve the stated timeout guarantee.
Proposed fix
- Presence: daemonPresenceProbe(ctx),
+ Presence: daemonPresenceProbe(pollCtx),Apply this change in both polling loops.
Also applies to: 177-189
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/proxsave/daemon_restart_verify.go` around lines 137 - 149, Update both
polling loops in the restart verification flow to pass the bounded pollCtx,
rather than the parent ctx, to daemonPresenceProbe when constructing
health.DaemonStateInput. Preserve the existing timeout and cancellation behavior
established by context.WithTimeout.
Comment on lines
+728
to
+731
| if d.cfg.HealthcheckAliveURL != "" || d.cfg.HealthcheckBackupURL != "" { | ||
| return d.cfg.HealthcheckAliveURL, d.cfg.HealthcheckBackupURL, checks | ||
| } | ||
| return build(d.cfg.HealthcheckAliveID), build(d.cfg.HealthcheckBackupID), checks |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve alive and backup URLs independently.
When either full URL is present, both configured URL fields are returned verbatim. A valid mixed configuration such as HEALTHCHECK_ALIVE_URL plus HEALTHCHECK_BACKUP_ID therefore leaves the backup endpoint unresolved and records no_url.
Proposed fix
- if d.cfg.HealthcheckAliveURL != "" || d.cfg.HealthcheckBackupURL != "" {
- return d.cfg.HealthcheckAliveURL, d.cfg.HealthcheckBackupURL, checks
- }
- return build(d.cfg.HealthcheckAliveID), build(d.cfg.HealthcheckBackupID), checks
+ alive := strings.TrimSpace(d.cfg.HealthcheckAliveURL)
+ if alive == "" {
+ alive = build(d.cfg.HealthcheckAliveID)
+ }
+ backup := strings.TrimSpace(d.cfg.HealthcheckBackupURL)
+ if backup == "" {
+ backup = build(d.cfg.HealthcheckBackupID)
+ }
+ return alive, backup, checks📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if d.cfg.HealthcheckAliveURL != "" || d.cfg.HealthcheckBackupURL != "" { | |
| return d.cfg.HealthcheckAliveURL, d.cfg.HealthcheckBackupURL, checks | |
| } | |
| return build(d.cfg.HealthcheckAliveID), build(d.cfg.HealthcheckBackupID), checks | |
| alive := strings.TrimSpace(d.cfg.HealthcheckAliveURL) | |
| if alive == "" { | |
| alive = build(d.cfg.HealthcheckAliveID) | |
| } | |
| backup := strings.TrimSpace(d.cfg.HealthcheckBackupURL) | |
| if backup == "" { | |
| backup = build(d.cfg.HealthcheckBackupID) | |
| } | |
| return alive, backup, checks |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/proxsave/daemon.go` around lines 728 - 731, Update the URL resolution
logic in the daemon healthcheck configuration method so HealthcheckAliveURL and
HealthcheckBackupURL are resolved independently: return each configured full URL
verbatim when present, otherwise build that endpoint from its corresponding ID.
Preserve the existing checks result and avoid using a single condition that
bypasses resolution for both endpoints.
Comment on lines
+809
to
+813
| if d.cfg.HealthcheckMode == "centralized" && needsNotifyResolve(r, nr.Results) { | ||
| if newR := d.buildReporter(ctx); newR != nil { | ||
| d.setReporter(newR) | ||
| r = newR | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve the reporter when a refresh reduces check coverage.
On a centralized fetch failure, buildReporter returns an alive/backup-only fallback. This block then replaces the existing reporter, discarding previously resolved notification checks and recording otherwise valid channels as no_url. Only adopt the rebuilt reporter when it increases or preserves required-check coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/proxsave/daemon.go` around lines 809 - 813, Update the reporter
replacement logic in the centralized-healthcheck branch around
needsNotifyResolve and buildReporter so it compares the rebuilt reporter’s
required-check coverage with the existing reporter before calling setReporter.
Preserve r when the refresh reduces coverage; only assign newR when coverage
increases or remains sufficient, preventing valid channels from being recorded
as no_url.
Comment on lines
+217
to
+229
| func runDashboardDaemonRestart(ctx context.Context, session *shell.Session, configPath, baseDir string) { | ||
| interval := time.Duration(0) | ||
| // Fallback lock path (base-dir default) used only when the config is unreadable; the | ||
| // normal path resolves the REAL <cfg.LockPath>/.backup.lock so the backup-wait probe | ||
| // inspects the same lock the orchestrator acquires even under a custom LOCK_PATH. | ||
| lockPath := backupLockFilePath(nil, baseDir) | ||
| if cfg, err := daemonStatusLoadConfig(configPath, baseDir); err == nil && cfg != nil { | ||
| interval = cfg.HealthcheckHeartbeatInterval | ||
| if strings.TrimSpace(cfg.BaseDir) != "" { | ||
| baseDir = cfg.BaseDir | ||
| } | ||
| lockPath = backupLockFilePath(cfg, baseDir) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Do not restart when the real backup lock path is unknown.
When configuration loading fails, this proceeds with the default lock path. On hosts using custom LOCK_PATH, the idle check can miss an active backup and restart the daemon mid-run. Show a configuration error and abort instead.
Proposed direction
- lockPath := backupLockFilePath(nil, baseDir)
- if cfg, err := daemonStatusLoadConfig(configPath, baseDir); err == nil && cfg != nil {
- interval = cfg.HealthcheckHeartbeatInterval
- if strings.TrimSpace(cfg.BaseDir) != "" {
- baseDir = cfg.BaseDir
- }
- lockPath = backupLockFilePath(cfg, baseDir)
+ cfg, err := daemonStatusLoadConfig(configPath, baseDir)
+ if err != nil || cfg == nil {
+ showDaemonResultScreen(ctx, session, "Daemon restart",
+ orchestrator.HealthcheckSetupLevelError, "config unreadable",
+ "Could not determine the active backup lock; restart was not attempted.")
+ return
}
+ interval = cfg.HealthcheckHeartbeatInterval
+ if strings.TrimSpace(cfg.BaseDir) != "" {
+ baseDir = cfg.BaseDir
+ }
+ lockPath := backupLockFilePath(cfg, baseDir)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func runDashboardDaemonRestart(ctx context.Context, session *shell.Session, configPath, baseDir string) { | |
| interval := time.Duration(0) | |
| // Fallback lock path (base-dir default) used only when the config is unreadable; the | |
| // normal path resolves the REAL <cfg.LockPath>/.backup.lock so the backup-wait probe | |
| // inspects the same lock the orchestrator acquires even under a custom LOCK_PATH. | |
| lockPath := backupLockFilePath(nil, baseDir) | |
| if cfg, err := daemonStatusLoadConfig(configPath, baseDir); err == nil && cfg != nil { | |
| interval = cfg.HealthcheckHeartbeatInterval | |
| if strings.TrimSpace(cfg.BaseDir) != "" { | |
| baseDir = cfg.BaseDir | |
| } | |
| lockPath = backupLockFilePath(cfg, baseDir) | |
| } | |
| func runDashboardDaemonRestart(ctx context.Context, session *shell.Session, configPath, baseDir string) { | |
| interval := time.Duration(0) | |
| // Fallback lock path (base-dir default) used only when the config is unreadable; the | |
| // normal path resolves the REAL <cfg.LockPath>/.backup.lock so the backup-wait probe | |
| // inspects the same lock the orchestrator acquires even under a custom LOCK_PATH. | |
| cfg, err := daemonStatusLoadConfig(configPath, baseDir) | |
| if err != nil || cfg == nil { | |
| showDaemonResultScreen(ctx, session, "Daemon restart", | |
| orchestrator.HealthcheckSetupLevelError, "config unreadable", | |
| "Could not determine the active backup lock; restart was not attempted.") | |
| return | |
| } | |
| interval = cfg.HealthcheckHeartbeatInterval | |
| if strings.TrimSpace(cfg.BaseDir) != "" { | |
| baseDir = cfg.BaseDir | |
| } | |
| lockPath := backupLockFilePath(cfg, baseDir) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/proxsave/dashboard.go` around lines 217 - 229, The
runDashboardDaemonRestart setup must not continue with the fallback lock path
when daemonStatusLoadConfig fails or returns no configuration. Handle that
configuration error by reporting it and returning before the restart/idle-check
flow; only derive interval, baseDir, and lockPath from a successfully loaded
config via backupLockFilePath.
Comment on lines
+107
to
+110
| for name, u := range c.Checks { | ||
| if nu := normalizeURL(u); nu != "" { | ||
| urls[name] = nu | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prevent dynamic checks from overriding reserved URLs.
Checks is documented for “OTHER” checks, but this loop permits Checks["alive"] and Checks["backup"] to replace the dedicated fields. Skip or reject reserved keys so core pings cannot be silently redirected.
Proposed fix
for name, u := range c.Checks {
+ if name == CheckKeyAlive || name == CheckKeyBackup {
+ continue
+ }
if nu := normalizeURL(u); nu != "" {
urls[name] = nu
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for name, u := range c.Checks { | |
| if nu := normalizeURL(u); nu != "" { | |
| urls[name] = nu | |
| } | |
| for name, u := range c.Checks { | |
| if name == CheckKeyAlive || name == CheckKeyBackup { | |
| continue | |
| } | |
| if nu := normalizeURL(u); nu != "" { | |
| urls[name] = nu | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/health/reporter.go` around lines 107 - 110, Update the loop over
c.Checks in the health reporter to ignore or reject the reserved keys "alive"
and "backup" before adding normalized URLs to urls. Preserve processing for all
other documented “OTHER” checks, ensuring the dedicated core ping URLs cannot be
overridden.
Comment on lines
+71
to
+100
| // Status is the last-known outcome of each ping the daemon sends, keyed by KIND in a | ||
| // dynamic map so a variable set of per-notification-channel checks (notify-<ch>) fits | ||
| // alongside the fixed heartbeat/start/finish/hang kinds. A missing key means "never | ||
| // attempted"; omitempty keeps a fresh file minimal. | ||
| type Status struct { | ||
| // Mode records the healthcheck mode in effect at the last write ("centralized" | ||
| // / "self"); the section uses it to phrase its output. | ||
| Mode string `json:"mode,omitempty"` | ||
| Heartbeat *PingRecord `json:"heartbeat,omitempty"` | ||
| RunStarted *PingRecord `json:"run_started,omitempty"` | ||
| RunFinished *PingRecord `json:"run_finished,omitempty"` | ||
| RunHang *PingRecord `json:"run_hang,omitempty"` | ||
| Mode string `json:"mode,omitempty"` | ||
| // Records maps a ping KIND (KindHeartbeat/KindRunStarted="start"/KindRunFinished= | ||
| // "finish"/KindRunHang="hang", or "notify-<ch>") to its last outcome. See the custom | ||
| // UnmarshalJSON below for the legacy-format migration. | ||
| Records map[string]*PingRecord `json:"records,omitempty"` | ||
| // Update is the last update-check + report-ping outcome. It is a dedicated record | ||
| // (not a bare PingRecord) because the /0-vs-/1 SIGNAL (Available) is orthogonal to | ||
| // whether the ping transmitted (Ping.OK). Nil until the first update check runs; | ||
| // omitempty so an old status file that predates it round-trips unchanged. | ||
| Update *UpdateRecord `json:"update,omitempty"` | ||
| } | ||
|
|
||
| // Record returns the last PingRecord for kind, or nil if never attempted. Nil-safe on a | ||
| // zero Status (nil map read yields nil). | ||
| func (s Status) Record(kind string) *PingRecord { return s.Records[kind] } | ||
|
|
||
| // UnmarshalJSON migrates the pre-Fase-2 status file in place: that format stored the | ||
| // fixed kinds as top-level keys "heartbeat"/"run_started"/"run_finished"/"run_hang" | ||
| // (note the json tag "run_started" mapped to Kind "start", etc.). We decode BOTH the new | ||
| // "records" map AND those legacy keys, then fold each legacy record into Records under its | ||
| // KIND, but only if the new map does not already carry it (new format wins). This is | ||
| // load-bearing: an in-place daemon upgrade whose FIRST write is a heartbeat read-modify- | ||
| // write would otherwise drop the last backup outcome + update verdict permanently. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant area.
wc -l internal/health/status.go
cat -n internal/health/status.go | sed -n '1,220p'
# Look for JSON marshal/unmarshal hooks and any references to legacy keys.
rg -n "MarshalJSON|UnmarshalJSON|records|run_started|run_finished|run_hang|heartbeat|update" internal/healthRepository: tis24dev/proxsave
Length of output: 32974
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the full file with line numbers if it's small enough, otherwise a focused slice.
if [ "$(wc -l < internal/health/status.go)" -le 500 ]; then
cat -n internal/health/status.go
else
cat -n internal/health/status.go | sed -n '1,260p'
fi
# Search for a custom marshal path or any manual JSON construction for Status.
rg -n "func .*MarshalJSON|json\\.Marshal|json\\.MarshalIndent|Status\\b" internal/healthRepository: tis24dev/proxsave
Length of output: 25241
Keep the legacy top-level keys in the on-disk schema if downgrades are supported. records and update are ignored by older binaries, so a downgrade read-modify-write will erase the migrated heartbeat/run history and update verdict from disk. Emit the legacy fields alongside the new shape in MarshalJSON, or drop downgrade compatibility entirely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/health/status.go` around lines 71 - 100, The Status serialization
must preserve downgrade compatibility by emitting the legacy top-level
heartbeat, run_started, run_finished, and run_hang fields alongside records and
update. Update Status.MarshalJSON to derive those fields from Records while
retaining the new schema; otherwise explicitly remove the legacy migration
compatibility if downgrades are not supported.
Comment on lines
+246
to
+280
| // Apply the healthchecks connector mode. Healthchecks require the daemon (the | ||
| // sole pinger), so in cron mode the connector is always off. In daemon mode the | ||
| // user's explicit choice wins; an EMPTY choice with the daemon keeps the previous | ||
| // behaviour (centralized-on out of the box) so pre-UI callers still work. | ||
| hcMode := normalizeHealthcheckMode(data.HealthcheckMode) | ||
| if mode != "daemon" { | ||
| hcMode = "off" | ||
| } else if hcMode == "" { | ||
| hcMode = "centralized" | ||
| } | ||
| // HEALTHCHECK_ALIVE_URL/BACKUP_URL are dual-purpose (self: the user's full ping | ||
| // URLs; centralized: a cache the server auto-fills). Reset them symmetrically on | ||
| // every mode switch so a leftover self URL never lingers as the centralized cache | ||
| // (which the daemon would ping if the server fetch fails). Self rewrites them via | ||
| // the params screen right after; centralized/off leave them blank (the server | ||
| // repopulates the centralized cache on next fetch). | ||
| switch hcMode { | ||
| case "off": | ||
| template = setEnvValue(template, "HEALTHCHECK_ENABLED", "false") | ||
| template = setEnvValue(template, "HEALTHCHECK_ALIVE_URL", "") | ||
| template = setEnvValue(template, "HEALTHCHECK_BACKUP_URL", "") | ||
| case "self": | ||
| template = setEnvValue(template, "HEALTHCHECK_ENABLED", "true") | ||
| template = setEnvValue(template, "HEALTHCHECK_MODE", "self") | ||
| // Clear the centralized cache so a switch to self does not ping a stale | ||
| // server-minted URL before the params screen writes the user's own URLs. | ||
| template = setEnvValue(template, "HEALTHCHECK_ALIVE_URL", "") | ||
| template = setEnvValue(template, "HEALTHCHECK_BACKUP_URL", "") | ||
| default: // centralized | ||
| template = setEnvValue(template, "HEALTHCHECK_ENABLED", "true") | ||
| template = setEnvValue(template, "HEALTHCHECK_MODE", "centralized") | ||
| // Clear any leftover self URL so it can't be pinged as the centralized | ||
| // cache; the server repopulates this cache on the next fetch. | ||
| template = setEnvValue(template, "HEALTHCHECK_ALIVE_URL", "") | ||
| template = setEnvValue(template, "HEALTHCHECK_BACKUP_URL", "") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clear URLs only on an actual mode transition—and clear all seven.
A self-to-self edit currently blanks ALIVE/BACKUP before RunHealthcheckSelfParams derives its prefill, forcing users to re-enter saved required URLs. Conversely, optional self URLs are never cleared when leaving self mode.
Track the previous effective mode, preserve URLs when it is unchanged, and clear all HEALTHCHECK_*_URL fields when crossing modes or disabling healthchecks. Add regression tests for self-to-self preservation and optional URL cleanup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/installer/install_data.go` around lines 246 - 280, Update the
healthcheck mode handling around normalizeHealthcheckMode and the hcMode switch
to track the previous effective mode, preserve all URL values when the mode is
unchanged, and clear every HEALTHCHECK_*_URL field only when transitioning
between modes or disabling healthchecks. Ensure self-to-self edits retain URLs
for RunHealthcheckSelfParams, while leaving self clears required and optional
URLs; add regression tests covering both behaviors.
| case err := <-errCh: | ||
| return err | ||
| case <-time.After(3 * time.Minute): | ||
| case <-time.After(uitest.Deadline(3 * time.Minute)): |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Scale the workflow context deadline too.
The outer wait is race-scaled, but the context at Line 306 still cancels after three minutes. A slow race run can therefore return context deadline exceeded long before this watchdog expires.
Proposed fix
- ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute)
+ ctx, cancel := context.WithTimeout(
+ context.Background(),
+ uitest.Deadline(3*time.Minute),
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case <-time.After(uitest.Deadline(3 * time.Minute)): | |
| ctx, cancel := context.WithTimeout( | |
| context.Background(), | |
| uitest.Deadline(3*time.Minute), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/orchestrator/decrypt_charm_e2e_test.go` at line 290, Update the
workflow context timeout near the decrypt charm E2E test’s watchdog to use the
same race-scaled duration as the time.After(uitest.Deadline(3 * time.Minute))
branch. Ensure the context deadline remains aligned with the scaled outer wait
so race runs do not cancel prematurely.
Comment on lines
+98
to
+123
| case StreamDoneMsg: | ||
| if msg.Token == t.token { | ||
| t.outcome = msg.Outcome | ||
| t.err = msg.Err | ||
| t.done = true | ||
| } | ||
| return t, nil | ||
| case spinner.TickMsg: | ||
| if t.done { | ||
| return t, nil | ||
| } | ||
| var cmd tea.Cmd | ||
| t.spin, cmd = t.spin.Update(msg) | ||
| return t, cmd | ||
| case tea.KeyPressMsg: | ||
| switch msg.String() { | ||
| case "enter", "space", "return": | ||
| if t.done { | ||
| return t, t.Resolve(StreamResult{Err: t.err}, nil) | ||
| } | ||
| case "esc": | ||
| if !t.done && !t.cancelling && t.cancel != nil { | ||
| t.cancelling = true | ||
| t.cancel() | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not turn finalization cancellation into install success.
Both StreamTask cancellation paths can return nil, and the installer additionally discards stream errors. This can leave symlink, scheduler, or permission finalization incomplete while reporting success.
internal/ui/components/streamtask.go#L98-L123: preserve Esc cancellation when the callback returns nil.internal/ui/components/streamtask.go#L195-L200: return the shell abort whenrunErris nil.cmd/proxsave/install_tui.go#L321-L359: returntaskCtx.Err()and map cancellation to an aborted install instead of debug-only logging it.
📍 Affects 2 files
internal/ui/components/streamtask.go#L98-L123(this comment)internal/ui/components/streamtask.go#L195-L200cmd/proxsave/install_tui.go#L321-L359
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/ui/components/streamtask.go` around lines 98 - 123, Preserve
cancellation errors across all StreamTask finalization paths: in
internal/ui/components/streamtask.go lines 98-123, keep Esc cancellation when
the Resolve callback returns nil; in lines 195-200, return the shell abort when
runErr is nil. In cmd/proxsave/install_tui.go lines 321-359, return
taskCtx.Err() and map cancellation to an aborted install rather than only
logging it, so stream errors cannot be discarded as success.
This was referenced Jul 15, 2026
Merged
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
1 participant
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.
Slice 3 of 6 — progressive rebuild of dev (base=dev), cherry-picked checkpoint range, tree matches
1813528,go build ./...verified. ~115 files.Content: install scheduler renames, dashboard styling/labels alignment, healthcheck enabled-channels + self-mode per-channel checks (Fase 2C client).
Merges into dev (no release/tag). Trigger review with
@coderabbitai review.Greptile Summary
This PR expands install, daemon, dashboard, and healthcheck behavior. The main changes are:
Confidence Score: 4/5
The changed install, handoff, and restart paths can misconfigure healthchecks, lose backup outcome reporting, or restart during an active backup.
cmd/proxsave/install.go, internal/config/config.go, cmd/proxsave/daemon.go, cmd/proxsave/daemon_restart_verify.go
Important Files Changed
Comments Outside Diff (1)
internal/config/config.go, line 832-837 (link)This parser only reads
SCHEDULER_TIME, so an existing config that still carries the renamed schedule-time key falls back to02:00on load. After upgrade, that silently changes the backup run time instead of preserving the user's configured schedule until the config is rewritten.Reviews (1): Last reviewed commit: "style(dashboard): rename Diagnostics gro..." | Re-trigger Greptile
Summary by CodeRabbit