Adds two opt-in tests (build tag "browsertest", run via `make
test-browser`) that drive a real headless Chrome instead of only
asserting on the raw HTML/JS source:
- TestPlayerRendersNatively confirms the shipped page still renders
normally and never injects es-module-shims on a browser with native
import map support.
- TestPlayerRendersUnderForcedShimMode forces es-module-shims into its
own shimMode (importmap-shim/module-shim, per the library's docs),
routing the real app.js and vendored dependencies through the
library's actual polyfill resolution. This exercises the old-Safari
code path directly in CI/local headless Chrome, without needing
physical iPadOS 15 hardware.
Not wired into `test`/`check`/CI yet, since chromedp has not
previously been exercised as part of this repo's test suite (only in
the standalone doc-screenshot tool) and needs a Chrome/Chromium binary
available to the runner.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Loading es-module-shims unconditionally would charge every browser an
~80KB uncompressed download on every page view, including the vast
majority that already support import maps natively -- the static
asset server here applies no compression. Feature-detect
HTMLScriptElement.supports('importmap') instead, so only a browser
that actually lacks support ever fetches it.
Insert the script via the DOM with async = false rather than
document.write: document.write is deprecated and subject to browser
interventions that can silently drop externally-sourced scripts it
injects, where DOM insertion with async explicitly disabled gives the
same before-the-deferred-module-script execution guarantee without
that risk.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Restores the import map and bare-specifier imports across all
components, and instead polyfills import map support for Safari on
iPadOS 15 (which has ES modules but not import maps) via
es-module-shims, loaded unconditionally since it detects native
support and no-ops there.
The previous approach converted every component to relative imports
through a dependencies.js facade and permanently sed-patched the
vendored preact-hooks build, baking the compatibility workaround into
the whole codebase instead of keeping it encapsulated in index.html.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The handler counted every request regardless of path, so the background
status-update goroutine AddDeviceByHost spawns after a successful probe
could land before the assertion and be mistaken for a second concurrent
seed probe, occasionally failing with request count 2 instead of 1. Filter
by path like the existing TestDiscoverDevicesRetriesConfiguredHosts test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fixes correctness issues found reviewing the bounded device-seed retry
loop before merging: a datastore read failure could make the readiness
check trivially pass; stale-host pruning only considered hosts inserted
in the current attempt and only ran inside the retry loop, not the
plain SeedExtraDevices path; the retry loop and a devices-changed-hook
seed could probe the same offline host concurrently; and a zero-change
startup window silently dropped the previously-unconditional device-list
broadcast. Also makes the retry interval/window configurable instead of
hardcoded, following the existing discovery-interval flag pattern.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
NewTestServer registers its own t.Cleanup(Close) instead of needing a
manual defer, and fails the test on a handler panic instead of just
logging it. It defaults to an in-memory transport reachable only via
Server.Client(), which wouldn't work here since our production
client.NewClient dials a real address rather than using that client --
calling Start() instead of Client() opts back into a real loopback
listener, identical to the old NewServer, confirmed by reading the
actual go1.27.0 source (server.go's Start implementation).
This is a proactive adoption of a new stdlib idiom, not one of the
review findings from the previous commit; it doesn't change the
goroutine-drain fix from that commit, which is a separate concern
Close()'s "wait for outstanding requests" guarantee doesn't fully
cover (a goroutine that hasn't started its request yet at Close() time
isn't "outstanding").
Verified: 10x -count re-run under -race, full suite + lint clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Five findings from reviewing gesellix/Bose-SoundTouch#644
(Retry configured player devices during discovery):
1. Source-labeling used exact string equality
(device.DiscoveryMethod == "Configuration"), which breaks once a
configured host is also found via mDNS/UPnP in the same sweep:
mergeDeviceData concatenates methods into e.g.
"Configuration+mDNS/Bonjour", so the check silently failed and the
device got labeled "discovered" instead of "manual". Extracted the
decision into classifySource() and switched to a substring match.
Added TestClassifySource, which fails against the old exact-equality
logic on exactly the composite-string cases (verified) and would
have caught this before merge -- the PR's own test disables
mDNS/UPnP, so it never exercised this path.
2. Manually configured devices no longer registered immediately at
startup -- they now wait for the full mDNS/UPnP sweep (up to the
10s discovery timeout) to complete, since the PR removed the
synchronous registration loop and relies entirely on
PreferredDevices. Restored the immediate loop alongside (not
instead of) folding manualHosts into PreferredDevices, so a
currently-online configured device registers immediately as
before, while an offline one still gets retried on every
subsequent discovery pass -- the actual value this PR adds.
3. The new PreferredDevices-seeding loop didn't dedupe against hosts
already loaded from PREFERRED_DEVICES, so setting both for the same
host produced duplicate entries. Currently harmless (absorbed by
AddDeviceByHost's fast path) but fragile. Added dedup by host.
4. NewDiscoveryService's doc comment didn't mention the new
configuredHosts parameter or its retry-on-every-sweep behavior.
Documented.
5. The new test's second DiscoverDevices call spawns a one-shot
status-update goroutine and a 30s-ticker poll loop with no
guaranteed drain before the deferred server.Close(), risking
benign but real -race/CI flakiness. Added a bounded settle delay
after RemoveDevice.
Verified: full build/vet/race test suite/lint clean; the new
TestClassifySource fails against the pre-fix logic and passes with
it; TestDiscoverDevicesRetriesConfiguredHosts re-run 20x under -race
with no flakiness.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
syncSources never reported how many sources it actually saved, so the
Admin UI's success message always said the meaningless "sources:
synced" regardless of outcome. syncSources now returns the count saved
(-1 if the fetch failed), threaded through SyncResult.SourcesCount.
Also replaces the single run-on results string (which visually mashed
presets/recents/sources together with no separator) with a real <ul>
list, one <li> per resource, matching the presets/recents diff lines.
Built via DOM APIs rather than innerHTML string concatenation, since
preset/recent names ultimately come from user-editable station names
on the speaker.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
startSync() used to POST once and, on any 2xx, render a hardcoded
"Presets: OK / Recents: OK / Sources: OK" regardless of what the
response actually said -- exactly why a silent partial data loss (see
the previous commit) would have looked like success to the user.
Now: on a 409 (destructive) response, build a specific confirm message
from the diff (e.g. "presets: 6 -> 5: Ici Roussillon") and gate via
window.confirm(), matching the existing QuickFix confirm UX; on
confirm, retry with ?confirmed=true. On success, render the actual
per-resource counts from the response body instead of a canned string.
Adds an HTTP-level regression test
(TestHandleInitialSync_DestructiveSyncReturns409ThenAppliesWhenConfirmed)
covering the same refuse-then-confirm flow through the real handler and
router, complementing the lower-level setup package test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
SyncDeviceData's syncPresets/syncRecents unconditionally overwrote the
datastore with whatever the speaker's live :8090 API returned at that
instant, with no check against what's already stored. If the speaker's
own local cache was stale or incomplete at that moment (e.g. right
after a burst of preset writes, or shortly after a reboot before the
speaker resyncs with Marge), Sync would silently persist that bad
snapshot over good data. A reporter's fresh #614 repro showed the
account's /full response dropping from 6 to 5 presets right after a
Sync click, consistent with this mechanism.
SyncDeviceData now diffs a fresh live fetch against what's stored
before writing anything; if applying would shrink either list, it
returns the diff (via the new SyncResourceDiff/SyncResult types)
without writing unless the caller passes confirmed=true.
HandleInitialSync surfaces this as a 409 with the diff JSON; every call
(confirmed or not) re-fetches live from the speaker, so a confirmed
retry re-checks reality rather than replaying a stale snapshot. Sources
sync is left unconditional, as before -- lower risk in practice and
out of scope for this fix.
fetchLivePresets/fetchLiveRecents are extracted pure-fetch helpers;
syncPresets/syncRecents keep their unconditional-apply behavior (used
directly by existing tests) since the button-driven path now goes
through the diff/confirm guard instead.
Adds TestSyncDeviceData_DestructiveSyncRequiresConfirmation covering
both the refusal and the confirmed-retry path.
Frontend wiring (script.js's startSync + real per-resource result
rendering, replacing the current hardcoded "OK" text) is a follow-up
commit on this branch.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Converts the remaining GetX-then-SaveX call sites (UpdatePreset,
RemovePreset, AddRecent's recent + learned-source persistence, AddSource)
to the new datastore.Mutate{Presets,Recents,ConfiguredSources} helpers,
closing the lost-update race for good on the actual write path the
speaker hits on every preset/recent store.
Adds a regression test that fires 6 concurrent UpdatePreset calls (same
shape as #614's rapid-fire repro) and asserts none are lost. Verified it
reliably fails against the pre-fix code (consistently drops presets
across repeated runs) and passes reliably with the fix, including under
-race.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GetX-then-SaveX call sites did an unguarded read-modify-write: two
concurrent callers could each read the same starting list, mutate
different entries, and the second writer's Save clobber the first's
update. This is exactly what dropped a preset during #614's rapid-fire
preset-programming repro (overlapping PUT .../preset/N requests).
Add MutatePresets/MutateRecents/MutateConfiguredSources, each holding a
single write lock across the whole read-mutate-write cycle, and switch
resolvePresetSource's auto-add-canonical-source path (the same race,
for sources) to use the new MutateConfiguredSources.
Part of the #614 follow-up; more call sites (UpdatePreset's own preset
write, recents, other sources writers) still need converting.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Speakers paired via non-AfterTouch tooling (e.g. the USB-stick SSH-enable
method) can report a margeAccountUUID that isn't Bose's own 7-digit
numeric format, such as "stick@local". Discovery persisted this value
unvalidated, and the datastore's identifier check rejected it outright,
so the device was silently never saved.
Widens datastore.IsSafeIdentifier to accept any identifier that's safe
as a path component, XML value, and telnet-command token (still
excluding whitespace, control characters, and HTML/XML/shell
metacharacters), and makes it the single account-ID validator,
replacing setup's separate, stricter 7-digit-only IsValidAccountID.
Also closes related gaps found while widening the validator:
- postSetMargeAccount now XML-escapes the account ID instead of raw
string interpolation.
- SaveAccountInfo/HandleMargeCreateAccount now validate the account ID
the same way SaveDeviceInfo already did.
- handlers_export.go URL-escapes account/device IDs before building
outbound diagnostic-fetch URLs.
- pkg/service/health gained the sanitizeLog helper every other package
already has, applied to log lines carrying speaker-reported values.
- The admin web UI (script.js) renders account/device IDs via DOM APIs
instead of innerHTML/inline event-handler string interpolation,
closing a stored-XSS path, and a duplicate escape helper was
consolidated into one.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
readPresetsLocked's os.IsNotExist branch and GetRecents' equivalent
branch silently returned an empty result with no log line at all,
unlike their sibling 0-byte/malformed-XML branches which already log.
When a reporter's speaker got served an empty preset list at reboot
despite an intact on-disk Presets.xml, there was no durable record of
it anywhere except a live capture at the exact moment.
Also log the per-device preset count going into every /full response
in CreateAccountDevice, distinguishing a disk read that came back
empty from one where source-mapping silently dropped presets
afterward.
Diagnostic only, no behavior change - the actual trigger for the
empty response is still open.
A speaker can be reachable, named, and already account-paired yet still
report SOUNDTOUCH_NOT_CONFIGURED, leaving the "install the Bose app"
prompt on screen (reported for ST30 Series II/III in #615). Only a full
pass through the WebSocket setup state machine clears it, but running
that unconditionally risks re-running the bracket on speakers that
don't need or support it.
Add Manager.PreflightInitPlan: checks /supportedURLs for
/setMargeAccount, then requires /soundTouchConfigurationStatus to read
exactly SOUNDTOUCH_NOT_CONFIGURED before ExecuteInitPlan runs.
Already-configured devices are a no-op; an unsupported route or an
unrecognised status value aborts instead of guessing.
The default Suggested Plan (both XML-over-SSH and Telnet) migrates the
speaker over plain HTTP and never touches CA/TLS at all, but the CA/TLS
precondition always showed a red not-installed marker and the HTTPS
Connection Test panel was always rendered, regardless of whether the
current Target URL actually needs HTTPS. Both read as mandatory steps
even when nothing needed doing.
CA/TLS and HTTPS only matter when the Target URL is https:// or the
Customize form's DNS-interception method is chosen (that one always
targets https://*.bose.com).
- caVerdict() now takes whether the Target URL is HTTPS: shows a
neutral marker with a "not needed" note for HTTP targets, keeps the
red marker with a sharper "required" note for HTTPS targets.
- The HTTPS Connection Test panel gets a small note under its heading
("Optional for your current plan (HTTP)" / "Required ... (HTTPS)"),
computed from the same check. Stays visible either way so someone can
still run it if they want.
Frontend-only — showSummary already had the Target URL in scope.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Follow-up on #621: the Reboot Speaker button (plus Revert to Defaults,
Enable SSH, Disable SSH) was reachable only after expanding the
collapsed "Customize this migration" section and scrolling past three
fieldsets and the XML/telnet diff panes. Meanwhile every other real
action elsewhere in the admin UI (Save Settings, Apply Suggested Plan,
Start Sync, ...) is visible by default.
- Move Revert to Defaults and Reboot Speaker into an always-visible
"Speaker controls" row directly under the Migration State card.
- Move Enable/Disable SSH into the Preconditions table, inline with the
SSH (remote_services) status row, sized like the existing "Trust CA
Now" button next to the CA/TLS row. script.js now only rewrites the
inner status span on re-render (matching the CA/TLS pattern) so the
buttons survive summary refreshes.
- Add shared .btn-primary/.btn-danger CSS classes so button color
consistently means the same thing everywhere (primary = confirm,
danger = destructive) instead of ad-hoc inline colors; applied to
Save Settings, Apply Suggested/Custom Plan, Enable/Disable SSH,
Revert to Defaults, and Trust CA Now. Removed decorative gray from
Reboot Speaker and the connection/DNS test buttons.
- Replace the "Cancel" button (which only hid the whole summary panel,
not any of the actions it sat beside) with a "✕ Hide" control next
to the "Migration Summary for <device>" heading, alongside a new
"↻ Reload" shortcut for refreshSummary().
- Remove the now-unneeded force-open-the-details hack in migrate()
since Reboot no longer lives inside any collapsed container.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- setup: resync all four boseurls (not just marge/swUpdate) over telnet
after an SSH-XML migration. `envswitch boseurls set` persists whatever
is currently in the runtime layer, so leaving stats/bmx untouched froze
their stale pre-migration values into the persistence layer permanently
-- surviving reboot and previously requiring a factory reset to clear.
- admin-ui: Migrate tab's Target Domain edits now propagate into the four
service URL fields (tracked via a dataset.autofilled flag so real manual
edits still aren't clobbered), closing the gap where changing Target
Domain to a new value left the four fields pointed at a stale default.
- install.sh: prune stale binary backups before the download too, not
only after a successful install, so a backup left by a previously
aborted (out-of-space) run gets cleaned up instead of compounding.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
validateURL() unconditionally rejected the hostnames "localhost" and
"127.0.0.1" for the four Migrate-tab plan URL fields, with no awareness
of deployment mode. Since the Suggested Plan's URLs are derived from the
page's own configured Target URL, a fresh on-device install (whose
server_url is now correctly http://localhost:8000, since #546) loaded
the Migrate tab with "Apply Suggested Plan" and "Pre-flight" disabled
by default, before the user touched anything -- directly contradicting
the on-device docs' "Migrate -> accept the suggested plan -> apply"
instructions.
Found while investigating why a #614 reporter used the non-standard
"localhost.localdomain" as a workaround, and why a #621 reporter got
stuck with "Migration Status: Migrated (URL mismatch)" trying to follow
the (correct) on-device localhost guidance.
Fix: a loopback URL is only flagged when it doesn't match the plan's
own Target URL origin. A field that's exactly what the service itself
is already configured to answer as (the on-device case) is accepted;
a stray "localhost" typed into one field while Target URL is a real LAN
address (the external-host mistake the check exists to catch) is still
flagged, since the origins differ.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Confirmed on real hardware (192.168.178.28): RevertMigration's full call
graph (revertXMLConfig/revertHosts/revertResolvConf/revertAftertouchHook/
removeRcLocalHooks/revertCACert) makes 17 separate client.Run() calls, and
pkg/ssh.Client.Run/UploadContent each dialed a brand-new SSH connection
per call with no reuse. Hitting a resource-constrained speaker with 17
rapid reconnects overwhelmed it -- confirmed via a follow-up plain SSH
command timing out at the TCP level, and the speaker going visibly
unresponsive.
Gives pkg/ssh.Client an opt-in persistent connection: Connect() dials
once and caches it, Close() releases it, and a shared dial() helper makes
Run/UploadContent reuse the cached connection when one's open, falling
back to today's per-call dial otherwise. RevertMigration now calls
Connect() once and defer Close(), collapsing 17 connections into 1. The
other ~21 m.NewSSH() call sites in pkg/service/setup never call Connect,
so their behavior is completely unchanged -- this only touches the one
function that was actually causing real-world problems.
SSHClient interface gained Connect()/Close(); both test mocks
(pkg/service/setup/setup_test.go, pkg/service/handlers/handlers_setup_test.go)
got no-op stubs. Added TestClose_NoOpWithoutConnect and
TestConnect_DialFailureLeavesConnNil in pkg/ssh/ssh_test.go -- these don't
prove connection reuse against a real server (Client.Run hardcodes :22,
no configurable port for a test listener), so that specific behavior is
verified by code review (a single `if c.conn != nil` branch) plus the
real-hardware confirmation above, not an automated integration test.
Also fixes the web UI's "Revert to Defaults" button, which calls the same
RevertMigration code path.
Adds a second QuickFix to the speaker_presets_count warning, reusing
the existing postSourcesUpdated fix (checks_refresh_sources.go). It
nudges the speaker to re-fetch /full, which is confirmed (both from
marge.AccountFullToXML and a genuine captured Bose-cloud response) to
carry presets alongside sources.
Whether firmware actually re-applies /full's preset section back onto
its own local table is unconfirmed — issue253_regression_test.go
already flags that exact link as untested. So this is offered as a
free, non-destructive thing to try first, with the guaranteed
restore_presets_to_speaker push as the fallback. Gives both directions
(pull-style nudge, direct push) rather than only the one.
Refs #614
When the speaker shows 0 preset slots while the service's Presets.xml
has entries (the #614 pattern), replays each stored preset onto the
speaker via :8090/storePreset (client.StorePreset), one slot at a
time. Doesn't require a reboot and doesn't need the content playing
first, unlike a physical preset-button save.
Sync only ever reads from the speaker; this is the missing write
direction, and lets a reporter try recovering presets without
re-entering all 6 by hand.
Refs #614
The speaker_presets_count check told users a power-cycle "usually
re-syncs" missing presets. #614 shows a power-cycle is itself one of
the two reported triggers for the speaker wiping its own presets, so
that advice was actively harmful for this failure mode.
Also fixes the comment's citation: it claimed this was a known pattern
from discussion #295 and #235, but neither actually discusses preset
loss (#295 is a cloud-hosting question, #235 a closed Spotify
preset-save bug). That reference was wrong from the original commit
(7d46ae2); #614 is the first confirmed instance.
Refs #614
The update-check interval field just got an info-toggle explaining Go
duration syntax; Discovery Interval takes the exact same syntax and
had no such help, which would read as inconsistent on the same
Settings page. Pre-existing gap, unrelated to #591 itself, but small
enough to fix alongside it while the pattern is fresh.
Reuses the existing info-toggle/info-details pattern (already used for
the HTTPS override, TLS extra hosts, and DNS upstream fields) rather
than inventing a new affordance, so users aren't left guessing at Go's
duration syntax when typing a custom interval.
Follow-up to #591: UpdateCheckEnabled/UpdateCheckInterval are now
persisted, live-reloaded Settings fields (mirroring the discovery
enabled/interval pattern), editable from the admin Settings page
without a restart. The env var/CLI flag remains the seed value for a
fresh install with no settings.json yet.
The background goroutine now always runs and polls the live settings
every minute (updateCheckPollTick), instead of being started only if
enabled at process launch, so flipping the toggle takes effect within
a minute rather than requiring a restart.
htm/Preact template literals insert text as a DOM text node rather than
parsing it as HTML, so the × entity was never decoded and showed
up literally in the player UI's announcement banner. The admin UI's
equivalent button is unaffected because it's built as an HTML string
inserted via innerHTML, where the browser does decode entities.
Fixes the player-UI regression noted in #591.
Comparing against JRpersonal/streborn#587 surfaced two gaps: no test
pinned that a newly added source type renders the same element shape
as a known-good default (the firmware rejects the whole account
document if one source entry omits an expected element), and our DLNA
discovery only swept SSDP from the service host, missing servers only
visible from a paired speaker's own LAN segment.
Adds TestSourceXMLShapeConsistencyAcrossTypes in pkg/service/marge,
and has HandleDiscoverLibraryServers merge results from each paired
speaker's own /listMediaServers alongside the existing SSDP sweep,
deduped by UDN, with unreachable speakers skipped silently.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses recommendations 4 and 7 from #515 comment 5231931569: a
green-looking getpdo readback only confirms the sys configuration
writes were accepted, not that they'll survive a reboot (that's what
the envswitch-persisted layer decides). Labels the getpdo line in both
migrateViaTelnet and runTelnetInjection's CLI/log output accordingly,
softens migrateViaTelnet's "succeeded" wording to "accepted", and adds
the same one-line caveat to TELNET-MIGRATION-METHOD.md #2.3 (previously
only in TELNET-COMMAND-REFERENCE.md).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Community hardware testing (bitranox, JRpersonal) on 2026-08-09 retracted
the earlier "inter-command delay is necessary" theory and established that
envswitch boseurls set commits the whole runtime layer (not just its two
arguments), has no read form, and doesn't ack with "OK". Corrects
TELNET-MIGRATION-METHOD.md and TELNET-COMMAND-REFERENCE.md accordingly,
retracts the stale "confirmed necessary" command-delay claim in
enable_ssh.go/cmd_setup.go, and lowers DefaultTelnetCommandDelay 5s -> 3s
as a smaller hedge now that the delay itself is known not to be the
mechanism.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ErrorsResponse.Error() only returned the <error> element's text body,
dropping the name attribute entirely. Some speaker error responses
have a Message that just restates Value as text (e.g. a bare "1047"
for SOURCE_ALREADY_REMOVED), so callers only ever saw the useless
numeric string. Found while live-debugging a Deezer account
add/remove cycle on real hardware, where the raw XML
(<error value="1047" name="SOURCE_ALREADY_REMOVED">1047</error>)
carried real information only in the name attribute.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Guards against whitespace-only stationID/podcastID/encodedName path
segments and tightens the existing empty-string checks on the search
q/cursor query params. Spotted while reviewing stalkerquatre-oss's
fork diff for TuneIn handling improvements; their s0/Radio fallback
defaults were skipped as unprecedented invented values that would
mask malformed requests instead of erroring.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Follow-up to the 3s default from earlier in #515: the reporter agreed
5s is a better trade-off (issue comment 5230881285) — more headroom
than the original guess, still comfortably under the ~7s gap their
manual A/B test used.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
#515 comment 5230833551: on a genuinely unpaired (factory-reset) device,
margeServerUrl is reportedly never polled at all, so the boseurls
SSH-enable injection has no read cycle to fire on regardless of any
command delay. enable-ssh now checks /info first and, if
margeAccountUUID is empty, pairs the device via the existing
PairAccount helper (HTTP /setMargeAccount, telnet fallback) before
running the injection.
Adds setup.Manager.EnsureMargeAccountPaired plus --no-auto-pair (skip
entirely) and --account (use a specific 7-digit ID instead of a
generated one, e.g. to match one already in the datastore) flags on
enable-ssh. Pairing failure is a warning, not fatal, since the claim
is unconfirmed on this specific hardware and existing working flows
must not regress.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Per #515 comment 5228449448: on a real Lifestyle console, the same 6
commands (5 sys configuration/envswitch + reboot) sent back-to-back left
sshd down after reboot, but succeeded sent one at a time with ~7s gaps —
same commands, same order, same device, minutes apart. Sending fast may
not let the device fully process one command before the next arrives.
Adds --command-delay (setup.DefaultTelnetCommandDelay, 3s), threaded
through EnableSSHViaTelnetFullConfig/runTelnetInjection (pause after each
of the 5 commands) and runEnableSSHInjection (one more pause before the
reboot). 0 restores the old back-to-back behavior. The reporter didn't
try to find the true minimum, just confirmed ~7s works and speculated
"a second or two may well be enough" — 3s is a middle ground, tunable via
the flag if a specific device needs more.
Also prints an approximate total for the injection phase up front (6
steps x delay, ~18s at the default) so the command doesn't look hung —
separate from the existing --wait message for sshd coming up after
reboot, which can take much longer.
Refs #515
Follow-up to #591, prompted by the update-check notice showing a raw
https:// URL as plain text instead of a clickable link. Made it general
rather than a one-off fix, since future announcements may also want to
link to docs.
Added Announcement.LinkText/LinkURL (+ LinkURLFunc, the dynamic
counterpart, for the update-check entry's per-release URL) alongside the
existing Message/MessageFunc pair. Both frontends render it as a real
<a> element now: the admin UI (innerHTML) escapes Message/LinkText/LinkURL
via the existing escapeHtml() before composing the markup — previously
Message went into innerHTML unescaped, which this incidentally hardens;
the player (Preact/htm) templates an actual <a> rather than interpolating
a string, since Preact escapes string children by default and a raw
<a href=...> string would otherwise render as literal text, not a link.
Rephrased the #419 admin-gate announcement to use the new field too (was
a plain "See issue #419 for details." text mention).
Bug found while wiring this up: UpdateCheckState never persisted the
release URL, only the version — so after a restart, the announcement
would show a correct message but a broken/empty link until the next live
check completed (which can be up to a full interval away, since a fresh
check is skipped when the persisted last-check is still recent). Fixed by
adding UpdateCheckState.LastReleaseURL and threading it through
Checker.persist/NewChecker's seeding path, with a test
(TestNewChecker_SeedsFromPersistedState) that would have caught it.
Also fixed two gocritic rangeValCopy findings in
handlers_announcements.go (switched to index-based iteration) surfaced by
the Announcement struct growing with the new fields.
Refs #591
Addresses the Semgrep finding on PR #599
(go.lang.security.audit.crypto.math_random.math-random-used) on the
update-check jitter delay, and applies the same treatment to the #419
activity-log filename suffix, which has the same non-security shape but
predates this PR's diff so it wasn't flagged.
Neither value is ever compared, kept secret, or otherwise security-
sensitive (a sleep duration and a filename-uniqueness suffix), so
crypto/rand would only add error-handling overhead for no real benefit.
Suppressed with the same // nosemgrep: <rule-id> pattern already used in
the mock-amazon/mock-spotify/mock-tunein servers, mirroring the existing
//nolint:gosec on the same lines.
Refs #591
Fifth and final piece of #591's initial implementation. Extends
/api/setup/version with update_available/latest_version/
latest_release_url (nil-safe via Server.UpdateCheckResult, defaults to
Available: false when the check was never enabled). Response switched from
map[string]string to map[string]interface{} to carry the new bool field;
updated the one existing test that decoded into the old stricter type.
Documents UPDATE_CHECK_ENABLED/UPDATE_CHECK_INTERVAL in the Configuration
Options reference table, explicit that this is the only network call
AfterTouch makes beyond speaker/provider traffic when enabled, and that it
defaults off.
This closes out the initial #591 implementation per the design doc
(_/i591/design-update-check.md): UpdateCheckState persistence, the
updatecheck.Checker package, background goroutine wiring with jitter/
backoff, reusing #419's Announcements mechanism instead of a second notice
UI, and this version-info exposure. `make check` passes end to end
(including the Docker HTTP integration suite).
Refs #591
Fourth piece of #591 — the "minimal and future-proof at once" move from
the design doc: no new notice UI, just one new entry in the #419
announcements list, which is already rendered in both the admin UI and the
player and already has per-ID dismissal.
Added Announcement.MessageFunc/DismissKeyFunc (nil = use the static
Message/ID, as before, so the existing #419 entry is unaffected) since
this entry's text names a specific version and its dismissal must be
per-version — dismissing the notice for v1.2.0 must not suppress a later
notice for v1.3.0. HandleListAnnouncements/HandleDismissAnnouncement now
compute the effective key through Announcement.dismissKey(s) rather than
reading the static ID field directly.
Refs #591
Third piece of #591. --update-check-enabled/--update-check-interval
(UPDATE_CHECK_ENABLED/UPDATE_CHECK_INTERVAL), default off/24h, following
the same local main.go flag pattern as discovery-enabled — not pkg/config,
which soundtouch-service doesn't import at all (correction to the issue's
proposed location, see the design doc).
Background goroutine modeled on startDeviceDiscovery: startup jitter
(0-5min), skips the immediate check if the persisted last-check is still
fresh, backs off retries to no sooner than 1h after a failure, logs once
per newly-detected version. The decision logic (shouldCheckImmediately,
shouldSkipDueToBackoff, logUpdateIfNewlyAvailable) is split into pure,
directly-testable functions rather than living inline in the goroutine.
Server gets a SetUpdateChecker/UpdateCheckResult pair (nil-safe) so the
next two pieces (announcement, /api/setup/version) can read the current
state without importing updatecheck's construction details.
Manually verified against a running instance: enabled via flags, no panic,
service stays responsive (jitter means the actual first check can take up
to 5 minutes to fire, so this only confirms the wiring, not a live
GitHub response — that's covered by the previous commit's httptest-backed
unit tests).
Refs #591
Second piece of #591. Standalone package (pkg/service/updatecheck):
GitHub releases API client, golang.org/x/mod/semver comparison (promoted
from indirect to direct dependency), persisted state via datastore's
UpdateCheckState. Dev/(devel)/dirty current versions skip the comparison
entirely rather than guessing; prereleases are excluded even though
GitHub's /releases/latest endpoint shouldn't return one anyway (defensive).
Deliberately decoupled from handlers.Server/main.go: repo and current
version are constructor arguments, not hardcoded, so a future CLI-side
check could reuse this as a plain import rather than a rewrite (open
question 2 in the design doc).
Not wired into the service yet — nothing calls NewChecker/CheckNow outside
tests.
Refs #591
First piece of #591 (opt-in periodic update check). A small persisted
state (last_checked_at, last_seen_version) under update-check.json,
mirroring Settings' Get/Save shape — separate from Settings itself since
this is runtime state, not operator-editable config.
Not wired to anything yet.
Refs #591
