This commit contains multiple fixes for temperature proxy registration,
but the core issue remains unresolved.
## What's Fixed:
1. Added config pointer and reloadFunc to TemperatureProxyHandlers
2. Added SetConfig method to keep handler in sync with router config changes
3. Added config reload after registration to prevent monitor from overwriting
4. Fixed installer port conflict detection and duplicate YAML key issues
5. Added comprehensive debug logging throughout registration flow
## What's Still Broken:
The TemperatureProxyURL, TemperatureProxyToken, and TemperatureProxyControlToken
fields are NOT persisting to nodes.enc after SaveNodesConfig is called.
Debug logs confirm:
- HandleRegister correctly updates nodesConfig.PVEInstances[matchedIndex]
- The correct data is passed to SaveNodesConfig (verified in logs)
- SaveNodesConfig completes without errors
- Config reload executes successfully
- BUT after Pulse restart, the fields are empty when loaded from disk
The bug is in SaveNodesConfig serialization or file writing logic itself.
Related files:
- internal/api/temperature_proxy.go: Registration handler
- internal/config/persistence.go: SaveNodesConfig implementation
- internal/config/config.go: PVEInstance struct definition
Fixed bug where config.yaml would end up with root:root 600 permissions
after the installer modified it, causing service startup failures with
"permission denied" errors.
Root cause: Two code paths modified config.yaml without resetting ownership:
1. ensure_control_plane_config() - used mktemp (creates root-owned file),
then mv'd it over config.yaml without chown/chmod
2. HTTP mode configuration - appended to config.yaml without resetting perms
Fix: Added chown/chmod after both modifications:
- Line 1601-1602: After control-plane config update
- Line 1860-1861: After HTTP mode config append
Now config.yaml maintains pulse-sensor-proxy:pulse-sensor-proxy 644
permissions after all modifications, allowing the service to start correctly.
This bug was discovered during repair logic testing - the service failed
to start after the installer ran, even though the fmt.Sprintf argument
alignment fix was working correctly.
Added TestPVESetupScriptArgumentAlignment to prevent future fmt.Sprintf
argument mismatch bugs in the PVE quick setup script template.
The test uses sentinel values (SENTINEL_URL, SENTINEL_HOST, deadbeef...)
to verify that critical placeholders receive the correct argument types:
✓ Repair block INSTALLER_URL uses pulseURL (not authToken)
✓ Repair --pulse-server flags use pulseURL (not authToken)
✓ Authorization headers use runtime $AUTH_TOKEN variable (not hardcoded)
✓ Token ID uses tokenName (pulse-*) (not pulseURL or authToken)
This test would have caught the bugs fixed in commits 2bb73d3c7 and
2053bc5e2, where:
- authToken appeared in --pulse-server URLs (argument shift)
- Authorization headers were hardcoded instead of using runtime variable
Recommended by Codex as a safeguard against this class of regression.
Changed Authorization headers in ssh-config and verify-temperature-ssh API
calls to use the runtime $AUTH_TOKEN variable instead of compile-time
hardcoded authToken.
This fixes a bug where users who override the auth token via:
- PULSE_SETUP_TOKEN environment variable
- Interactive prompt (when auth_token URL param omitted)
...would still send an empty Bearer token in the Authorization headers,
causing API calls to fail with 401 Unauthorized.
Changes:
- Line 4748: -H "Authorization: Bearer %s" → -H "Authorization: Bearer $AUTH_TOKEN"
- Line 4937: -H "Authorization: Bearer %s" → -H "Authorization: Bearer $AUTH_TOKEN"
- Removed 2 authToken arguments from fmt.Sprintf (lines 5059)
Now the script respects runtime token overrides in all code paths.
Identified by Codex during fmt.Sprintf argument alignment review.
Fixed critical argument mismatch bug where fmt.Sprintf arguments didn't align
with template placeholders. This caused:
- authToken being passed where pulseURL expected (curl errors)
- pulseURL being passed where authToken expected (empty Authorization headers)
- tokenName misalignment (Token ID placeholder broken)
Root cause: Template has 51 %s placeholders (54 total - 3 escaped %%s), but
argument list had wrong count and ordering.
Solution: Rebuilt argument list (lines 5049-5059) with correct mapping:
- 27 pulseURL (all installer URLs, --pulse-server flags, API endpoints)
- 11 tokenName (token creation, checks, final Token ID)
- 3 authToken (AUTH_TOKEN variable + 2 Authorization headers)
- 3 serverHost (error message rerun hints)
- 1 each: serverName, time, pulseIP, storagePerms, SSH keys, minProxyReadyVersion
Verified with go vet (passes). Mapping confirmed by walking each placeholder
in template and matching to correct argument type.
Related to #TBD (user will test)
Same turnkey UX issue as PVE quick setup: when local sensor-proxy socket
exists and validates, install-docker.sh would skip reinstallation (line 498).
This left stale control-plane tokens and URLs.
Fix: Always reinstall when LOCAL_PROXY_EXISTED_AT_START=true
- Track socket existence at script start
- Change message: 'will refresh to update tokens/config'
- Set SKIP_INSTALLATION=false to force reinstall
- Installer is idempotent (Phase 2), safe to rerun
This completes the turnkey repair fix across all entry points (PVE quick
setup and Docker installer).
Issue: When deployment type cannot be determined, error message referenced
$PROXY_INSTALLER but deleted it immediately, making instructions unusable.
Fix: Provide complete curl commands that users can copy-paste directly:
curl -fsSL $PULSE_URL/api/install/install-sensor-proxy.sh | bash -s -- ...
This ensures users have a working repair path even when auto-detection fails.
Identified by Codex final review.
Addresses all remaining issues from Codex final review:
Issue 1: SUMMARY_PROXY_INSTALLED unreliable (only set by install.sh)
Fix: Use PROXY_SOCKET_EXISTED_AT_START flag set at script start - works
for all installation methods (manual, older installers, etc.)
Issue 2: CTID detection fails when container offline/renamed
Fix: Read SUMMARY_CTID from install_summary.json as fallback. Priority:
1) Live PULSE_CTID detection
2) SUMMARY_CTID from json file
3) Standalone node detection
Issue 3: Failed repair disables working proxy (TEMPERATURE_ENABLED=false)
Fix: Keep TEMPERATURE_ENABLED=true in all failure paths. Comments explain:
proxy was working before, keep it enabled even if repair fails.
This ensures turnkey repair works reliably across all deployment scenarios
without breaking existing working proxies.
Addresses all issues found in Codex review:
1. Prevent double-install: Check SUMMARY_PROXY_INSTALLED to distinguish
between fresh installs (skip repair) vs existing installs (run repair)
2. Fix clustered node failures: Explicitly detect deployment type and bail
out with clear error message if neither --ctid nor --standalone can be
determined
3. Add health validation: Mirror main install path - verify service active,
socket exists, and fetch SSH public key after repair
4. Capture installer output: Show full diagnostics on failure (tail -20)
5. Better error messages: Provide specific manual repair commands when
deployment type cannot be auto-detected
This ensures the turnkey repair experience works reliably without regressing
fresh install UX.
The previous attempt (ed04926) was ineffective - it only set TEMPERATURE_ENABLED=true
which was redundant (already set at line 4051) and didn't trigger the auto-install block
because that block is gated by SKIP_TEMPERATURE_PROMPT != true.
This fix actually downloads and runs install-sensor-proxy.sh when an existing
socket is detected, which:
- Refreshes control plane tokens (fixes 401 errors)
- Updates control plane URL to correct Pulse instance
- Rewrites config atomically (Phase 2 installer is idempotent)
- Maintains turnkey UX - rerunning setup script now actually works
Detected by Codex final review.
When sensor-proxy socket is detected, the setup script was skipping
temperature monitoring setup with 'already configured' message. This
left stale control plane URLs/tokens, breaking temperature monitoring.
Now follows Codex recommendation: treat existing installations as
upgrade/repair opportunities. The installer is idempotent (Phase 2),
so rerunning it safely refreshes tokens, updates URLs, and ensures
turnkey operation even on hosts with existing installations.
Changes:
- Remove early return when sensor-proxy socket detected
- Set TEMPERATURE_ENABLED=true to proceed with reinstall
- Update message to clarify repair/upgrade behavior
- Maintains turnkey promise: rerun setup and it just works
- Update installer to use v4.32.0 Phase 2 binaries with file-based config
- Add automatic detection of Pulse service (systemd/hot-dev/docker)
- Add --restart-pulse flag for automatic Pulse restart in dev/test environments
- Default behavior shows clear instructions to restart Pulse manually (safe for production)
- Add prominent restart notice with command suggestions based on detected deployment
- Improve UX by making restart step impossible to miss
Related to Phase 2 sensor-proxy architecture improvements
Stop pulse-sensor-proxy service before modifying config.yaml to prevent
races with the running daemon or concurrent CLI commands.
The migration script now:
1. Stops service if running
2. Updates config atomically (temp + rename in same directory)
3. Restarts service if it was running
This achieves complete architectural isolation - ALL config file writers
are now coordinated (either through Phase 2 CLI locking or by ensuring
the service is stopped during modification).
Addresses final Codex review feedback.
Create temp file in same directory as config.yaml to ensure mv is truly
atomic (won't degrade to copy+unlink on different filesystems).
Added comments noting this is a legacy migration script with minor race
risk (no file locking) that should be deprecated once all users upgrade
to v4.32+.
1. Self-heal script: Add BINARY_PATH variable so CLI migration actually runs
- Previously logged "Binary not available" and skipped migration
2. migrate-sensor-proxy-control-plane.sh: Use atomic write (temp + rename)
- Prevents partial writes if script is interrupted
- Reduces race window with running service
These were the remaining gaps identified by Codex review.
NOTE: migrate-sensor-proxy-control-plane.sh still uses Python manipulation
instead of the Phase 2 CLI, but as a one-time migration script for upgrades
from v4.31, the atomic write provides sufficient protection. Future versions
can deprecate this script entirely.
Remove all code paths that manipulate config files without Phase 2 locking:
1. Installer: Remove ensure_allowed_nodes_file_reference() call (line 1674)
- Migration now handled exclusively by config migrate-to-file
2. Installer: Make migration failures fatal in update_allowed_nodes()
- Prevents fallback to unsafe Python manipulation
3. Daemon sanitizer: Remove os.WriteFile() call
- Now only sanitizes in-memory copy, doesn't write back to disk
- Logs warning instructing admin to run `config migrate-to-file`
4. Self-heal script: Replace 132 lines of Python with CLI call
- sanitize_allowed_nodes() now calls `config migrate-to-file`
- Eliminates uncoordinated Python-based config rewriting
All config mutations now flow exclusively through Phase 2 CLI with
atomic operations and file locking. No code paths remain that can
create duplicate allowed_nodes blocks.
Addresses Codex review feedback on Phase 2 gaps.
The migrate-to-file command now calls sanitizeDuplicateAllowedNodesBlocks
before parsing the config, allowing it to handle corrupted configs with
duplicate allowed_nodes blocks.
This ensures migration works even on hosts that were affected by the
original corruption issue.
The Codex MCP tool description was misleading - it said "Ask a question"
and "Returns a thoughtful response" which implied it was read-only.
In reality, Codex has full filesystem access and can create/modify files
directly since the MCP server executes the `codex` CLI agent which has
full implementation capabilities.
**Changes:**
- Updated description to clarify Codex can "implement changes or answer questions"
- Added note that "Codex has full filesystem access and can create/modify files directly"
- Updated inputSchema description from "question" to "question or implementation task"
- Updated comment from "for initial questions" to "for both questions and implementation"
This prevents confusion about Codex being a read-only Q&A tool when it's
actually a full implementation agent.
Final security hardening based on second Codex review:
**Lock File Permission Fix (Security)**
- Lock file now created with 0600 instead of 0644
- Prevents unprivileged users from opening lock and holding LOCK_EX
- Without this, any local user could DoS the installer/self-heal
- Added f.Chmod(0600) to fix permissions on existing lock files
**Deadlock Prevention (Future-Proofing)**
- Added documentation for future multi-file locking scenarios
- Specifies consistent lock ordering requirement (config.yaml.lock before allowed_nodes.yaml.lock)
- Prevents potential deadlocks if future commands modify multiple files
- Current implementation only locks one file, so no immediate issue
**Testing:**
✅ Lock file created as `-rw-------` (0600)
✅ Existing lock files with wrong perms get fixed
✅ Unprivileged users can no longer DoS the lock
**Codex Validation:**
- Locking is now correct (persistent .lock file, held during entire operation)
- Atomic writes complete while lock is held
- Validation honors actual config paths
- Empty lists supported for operational flexibility
- Error propagation prevents silent failures
- No remaining race conditions or security issues
Phase 2 is now complete and Codex-verified as secure.
Related to Phase 2 fixes commit 804a638ea
Fixes critical issues found by Codex code review:
**1. Fixed file locking race condition (CRITICAL)**
- Lock file was being replaced by atomic rename, invalidating the lock
- New approach: lock a separate `.lock` file that persists across renames
- Ensures concurrent writers (installer + self-heal timer) are properly serialized
- Without this fix, corruption was still possible despite Phase 2
**2. Fixed validation to honor configured allowed_nodes_file path**
- validate command now uses loadConfig() to read actual config
- Respects allowed_nodes_file setting instead of assuming default path
- Prevents false positives/negatives when path is customized
**3. Allow empty allowed_nodes lists**
- Empty lists are valid (admin may clear for security, or rely on IPC validation)
- validate no longer fails on empty lists
- set-allowed-nodes --replace with zero nodes now supported
- Critical for operational flexibility
**4. Installer error propagation**
- update_allowed_nodes failures now exit installer with error
- Prevents silent failures that leave stale allowlists
- Self-heal will abort instead of masking CLI errors
**Technical Details:**
- withLockedFile() now locks `<path>.lock` instead of target file
- Lock held for entire duration of read-modify-write-rename
- atomicWriteFile() completes while lock is still held
- Empty lists represented as `allowed_nodes: []` in YAML
**Testing:**
✅ Lock file created and persists across operations
✅ Empty list can be written with --replace
✅ Validation passes with empty lists
✅ Config path from allowed_nodes_file honored
✅ Concurrent operations properly serialized
These fixes ensure Phase 2 actually eliminates corruption by design.
Identified by Codex code review
Related to Phase 2 commit 3dc073a28
