Skip to content

fix: unedited dashboard config preview reports zero changes - #693

Closed
VijitSingh97 wants to merge 2 commits into
developfrom
claude/cranky-carson-f39bad
Closed

fix: unedited dashboard config preview reports zero changes#693
VijitSingh97 wants to merge 2 commits into
developfrom
claude/cranky-carson-f39bad

Conversation

@VijitSingh97

Copy link
Copy Markdown
Collaborator

Problem

On gouda v1.9.3, an unedited dashboard config preview (POST /api/control/preview with the config round-tripped from /api/config) reports two phantom INFO changes:

  1. CLEARNET_STATE_DIR: /srv/code/current/... → /srv/code/pithead-v1.9.3/...
  2. "Energy calculator settings (dashboard.energy) … updated" with nothing changed.

Root causes

Path spelling. pithead cd's to its own directory at startup and bash keeps the logical path, so $PWD is spelled via the current symlink from an operator shell but via the versioned install dir from the control runner's systemd unit. Every $PWD-derived path re-renders under the other spelling. Only CLEARNET_STATE_DIR was visible (generic describe_change fallthrough), but CONTROL_DIR, CADDY_LOG_DIR, and TARI_WALLET_SECRET_FILE were silently diffing too — a commit would rewrite .env to the versioned spelling and break the repoint-current rollback convention. On an all-default install the chain data dirs would flip as DEST rows.

Energy block. Not the #646 price feed (it never writes config — verified). The editor's read_config merges config.reference.json defaults UNDER the sparse live config, so an untouched save posts back every energy default spelled out while config.json stays sparse; the raw jq equality in control_preview / control_approval_gate read that as an edit.

Fix

  • preserve_path_spelling (+ physical_path): the render keeps the spelling already in .env whenever old and new resolve (pwd -P) to the same physical location, applied to all ten path keys at the parse_and_validate_config / render_env chokepoints. A real move (different physical dir) still previews and warns exactly as before.
  • migrate_dashboard_data gets the matching physical-equality guard, so a preserved spelling can't trip the "data exists at BOTH locations" stop on one directory counted twice.
  • energy_block_changed: both sides filled from the same config.reference.json defaults before comparing, shared by the preview row and the commit gate's audit key. Fails toward "changed" on an unreadable reference — never toward hiding a real edit.

Coverage (tier 1 per docs/dev/testing-strategy.md)

  • Control-channel rig: an unedited reference-merged round-trip previews changes: [], still commits, and audits no DASHBOARD_ENERGY; the existing real-edit, bundling-refusal, and smuggling tests all still pass.
  • New versioned-layout sandbox (version dir + current symlink): baseline apply through the symlink, then dry-run and apply from the versioned dir → zero porcelain rows and .env spellings intact (incl. the tari secret file); a genuine monero.data_dir move still reports DEST.
  • Revert-proven: all six behavior assertions fail on the unfixed script.

make lint and make test pass (stack suite 1514 ✓ / 0 ✗). Changelog + docs/operations.md layout note updated. Ponytail review: lean, nothing to cut (energy_block_changed dedups a previously duplicated jq expression).

Note for the deployed fleet: gouda's systemd unit still carries the versioned WorkingDirectory/ExecStart — harmless now (either spelling renders identically); the unit self-updates on the next apply cycle.

🤖 Generated with Claude Code

VijitSingh97 and others added 2 commits July 19, 2026 18:30
Two phantom rows on the versioned deploy layout. The control runner's
systemd unit invokes pithead from the version directory while .env was
rendered through the `current` symlink, so every $PWD-derived path
(CLEARNET_STATE_DIR visibly; CONTROL_DIR, CADDY_LOG_DIR and
TARI_WALLET_SECRET_FILE silently) re-rendered under the other spelling
and previewed as a change — and a commit rewrote .env to the versioned
spelling, breaking the repoint-`current` rollback convention. The render
now keeps the spelling already in .env whenever both resolve to the same
physical directory (preserve_path_spelling); a real move still previews
as before. migrate_dashboard_data gets the matching physical-equality
guard so a preserved spelling can't trip its both-locations stop on one
directory counted twice.

Separately, the editor round-trip carries the config.reference.json
default for every dashboard.energy key the live config omits
(read_config merges the reference UNDER the sparse live copy), and the
raw jq equality in control_preview/control_approval_gate read those
spelled-out defaults as an energy edit. Both sides are now filled from
the same reference defaults before comparing (energy_block_changed);
the comparison still fails toward "changed" on an unreadable reference.

Tier-1 coverage: control-rig case proving an unedited reference-merged
round-trip previews zero changes and audits no DASHBOARD_ENERGY, and a
versioned-layout sandbox (version dir + current symlink) proving a
dry-run/apply from either spelling is a no-op that keeps .env spellings,
while a genuine data-dir move still reports DEST.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ymlink-spelling case

The sandbox case seeded no dashboard data, and its unedited apply
short-circuited at "no changes" — the #455 guard was never reached, so
reverting it changed nothing the test could see. Seed a live DB marker
and apply a real change from the versioned dir: the guard must read the
`current` and versioned spellings as one physical dir (apply exits 0,
data stays put) instead of hard-stopping on data-at-both-locations.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@VijitSingh97

Copy link
Copy Markdown
Collaborator Author

Closing as superseded by #700, which fixes the same two root causes (#695's logical-vs-physical $PWD spelling flip and #696's reference-merged energy defaults firing a phantom change) with a smaller mechanism: SCRIPT_DIR is canonicalized once via pwd -P, making every render deterministic, instead of preserving whichever spelling .env happened to have. The two PRs rewrite the same jq lines and insert tests at the same points, so no merge order lands both. #700's regression tests were verified to fail on pre-fix code (34 asserts, both bugs by name). The migrate_dashboard_data double-count hazard this branch guarded against cannot arise once renders are deterministic. The physical-path ownership comparison landed separately in #692 still covers units written with the old spellings.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant