feat: a way to actually turn pairing on - #111
Merged
Merged
Conversation
Pairing shipped in #108 with no way to configure it. The config field existed, the keychain storage existed, the heartbeat loop ran on startup -- and no screen rendered an input, so the only way to pair was to hand-edit config.json and write a keychain entry. Working code nobody can reach is the same defect as code nobody wired up, which is the exact mistake this loop had just finished fixing on Android. Two fields in the existing settings form. Not the Preact rewrite (bead 806) -- that is a separate, much larger change, and gating a shipped feature's only entry point on it would leave it unreachable for however long that takes. THREE THINGS THAT NEEDED CARE: EMPTY IS MEANINGFUL HERE, unlike every other field in SaveSettings. Standalone is the supported default, so "" means unpair -- the surrounding fields all key on `!= ""`, and copying that pattern would have made unpairing impossible. REPOINTING AT A DIFFERENT SERVER DISCARDS THE PER-WORKER KEY. That credential belongs to the server that issued it; presenting it to another one fails authentication with no obvious cause, and keeping it leaves a live secret for a machine the user has stopped talking to. Saving the SAME url keeps it, so an unrelated settings save cannot silently re-enrol the machine. THE KEY GOES TO THE KEYCHAIN, not config.json, with a test asserting the token does not appear in that file. The display value round-trips like the existing fleet key rather than inventing a mask-and-detect scheme no other secret here uses -- consistency beats a bespoke convention for one field. Pairing takes effect immediately (startUpstream stops any previous loop first, so this cannot leave two loops reporting under one identity), unlike the fleet listener, which still needs a restart and says so. The test harness now builds the full app graph -- catalog, runtime, services -- because SaveSettings goes through a.ready(). Carving out a narrower helper to make the tests easy would have left the path users actually hit untested. Four negative controls fire: keeping the old worker key on a repoint, treating an empty URL as no-change, dropping the fields from the form, and never writing the key to the keychain. Frontend typechecked with the real compiler (tsc --noEmit in Docker on the build host) -- `npx tsc` here silently installs a bogus `tsc@2.0.4` package that typechecks nothing.
📝 WalkthroughWalkthroughSettings now support an upstream pairing URL and enrollment key. Saving settings trims and persists the URL, securely stores the key, handles unpairing, resets or retains worker keys as needed, and restarts upstream reporting. Frontend mappings and integration tests cover the flow. ChangesUpstream pairing settings
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #111 +/- ##
==========================================
- Coverage 73.64% 73.62% -0.02%
==========================================
Files 17 17
Lines 3612 3617 +5
==========================================
+ Hits 2660 2663 +3
- Misses 760 761 +1
- Partials 192 193 +1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@app.go`:
- Around line 702-727: Make the pairing update flow in the settings handler
failure-atomic across SetUpstreamWorkerKey, SetUpstreamEnrolmentKey, and
a.cfg.Save(cfg): stage or record the previous URL and secret values, persist
cfg, and roll back every secret/config mutation if any later operation fails.
Preserve existing clearing behavior when switching upstreamUrl and updating
upstreamKey, and add a regression test covering a.cfg.Save failure that verifies
the prior pairing state remains intact.
In `@upstream_client.go`:
- Around line 200-204: Update upstreamEnrolmentKey and the settings SaveSettings
flow to distinguish a missing pairing key from an unreadable keychain read,
using an explicit unavailable state instead of returning an empty string for
failures. Preserve the stored upstreamKey during unrelated settings saves when
the key is unavailable, and only omit or clear it when the key is intentionally
absent or secret storage is readable.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: abe92611-c02f-49bf-902d-6e616af6bdcf
📒 Files selected for processing (4)
app.gofrontend/src/main.tsupstream_client.goupstream_client_test.go
Comment on lines
+702
to
+727
| if value, present := values["upstreamUrl"]; present { | ||
| next := strings.TrimRight(strings.TrimSpace(value), "/") | ||
| if next != cfg.UpstreamURL { | ||
| // Repointing at a DIFFERENT server must discard the per-worker key | ||
| // the previous one issued. It is that server's credential and means | ||
| // nothing to the new one; presenting it would fail authentication | ||
| // with no obvious cause, and keeping it around is a stale secret for | ||
| // a machine the user has stopped talking to. | ||
| if err := config.SetUpstreamWorkerKey(a.cfg.AppDir(), ""); err != nil { | ||
| return SettingsState{}, fmt.Errorf("clearing the previous worker key: %w", err) | ||
| } | ||
| a.upstream.mu.Lock() | ||
| a.upstream.workerKey = "" | ||
| a.upstream.mu.Unlock() | ||
| } | ||
| cfg.UpstreamURL = next | ||
| } | ||
| if value, present := values["upstreamKey"]; present { | ||
| // Stored verbatim, including empty -- clearing the field is how a user | ||
| // unpairs the credential. Like the existing fleet key, the real value | ||
| // round-trips to the form (rendered type="password"); this file does not | ||
| // invent a mask-and-detect scheme that no other secret here uses. | ||
| if err := config.SetUpstreamEnrolmentKey(a.cfg.AppDir(), strings.TrimSpace(value)); err != nil { | ||
| return SettingsState{}, fmt.Errorf("storing the pairing key: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make pairing updates failure-atomic.
Lines 710 and 724 mutate secret storage before Line 751 persists cfg. If a.cfg.Save(cfg) fails, the old UpstreamURL can remain configured after the worker key was deleted or the enrolment key was replaced. The next upstream start then uses inconsistent pairing state.
Coordinate config and secret updates with rollback or staged writes. Add a regression test for a config-save failure.
🤖 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 `@app.go` around lines 702 - 727, Make the pairing update flow in the settings
handler failure-atomic across SetUpstreamWorkerKey, SetUpstreamEnrolmentKey, and
a.cfg.Save(cfg): stage or record the previous URL and secret values, persist
cfg, and roll back every secret/config mutation if any later operation fails.
Preserve existing clearing behavior when switching upstreamUrl and updating
upstreamKey, and add a regression test covering a.cfg.Save failure that verifies
the prior pairing state remains intact.
Comment on lines
+200
to
+204
| func (a *App) upstreamEnrolmentKey() string { | ||
| key, err := config.UpstreamEnrolmentKey(a.cfg.AppDir()) | ||
| if err != nil { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline frontend/src/main.ts --items all --type function
rg -n -C 10 'function saveSettingsFromForm|function renderEnvSetting|envInputName|upstreamKey|SaveSettings\(' frontend/src/main.tsRepository: GeiserX/CashPilot-Desktop
Length of output: 8499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching upstream_client.go and app.go:\n'
fd -a '^(upstream_client|app)\.go$' . | sed 's#^\./##'
printf '\nupstream_client.go outline:\n'
ast-grep outline upstream_client.go --items all --view expanded || true
printf '\nRelevant upstream_client.go sections:\n'
rg -n -C 12 'func \(a \*App\) upstreamEnrolmentKey|upstreamEnrolmentKey|SaveSettings|func .*SaveSettings' upstream_client.go
sed -n '1,280p' upstream_client.go
printf '\nRelevant app.go sections around SaveSettings and SettingsState helpers:\n'
rg -n -C 8 'func .*SaveSettings|SettingsState|CurrentSettings|currentSettings|enrolment|UpstreamEnrolmentKey|upstreamKey' app.go
sed -n '1,260p' app.goRepository: GeiserX/CashPilot-Desktop
Length of output: 27232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect config.UpstreamEnrolmentKey implementation if present.
rg -n -C 6 'func UpstreamEnrolmentKey|UpstreamEnrolmentKey|masterKey|GetCredentials|RenameCredentials|DeleteCredentials' internal/config internal/store app.go upstream_client.goRepository: GeiserX/CashPilot-Desktop
Length of output: 13601
Preserve unreadable pairing keys.
upstreamEnrolmentKey returns "" for both a missing key and keychain read failure. The settings form submits that displayed value as upstreamKey, and SaveSettings stores empty verbatim; an unrelated settings save can delete the stored pairing key.
Use an explicit unavailable state while keychain reads fail, and only omit/clear upstreamKey when the key is intentionally missing or secret storage becomes readable.
🤖 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 `@upstream_client.go` around lines 200 - 204, Update upstreamEnrolmentKey and
the settings SaveSettings flow to distinguish a missing pairing key from an
unreadable keychain read, using an explicit unavailable state instead of
returning an empty string for failures. Preserve the stored upstreamKey during
unrelated settings saves when the key is unavailable, and only omit or clear it
when the key is intentionally absent or secret storage is readable.
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.
Pairing shipped in #108 with no way to configure it. The config field existed, the keychain storage existed, the heartbeat loop ran on startup — and no screen rendered an input. The only way to pair was to hand-edit
config.jsonand write a keychain entry.Working code nobody can reach is the same defect as code nobody wired up — the exact mistake I'd just finished fixing on Android, where a whole earnings pipeline shipped unreferenced.
Two fields in the existing form, not the rewrite
Desktop-806(the 1641-line frontend → Preact) is a separate and much larger change. Gating a shipped feature's only entry point on it would leave that feature unreachable for however long the rewrite takes.Three things that needed care
Empty is meaningful here, unlike every other field in
SaveSettings. Standalone is the supported default, so""means unpair. The surrounding fields all key on!= ""— copying that pattern would have made unpairing impossible. This keys on presence instead.Repointing at a different server discards the per-worker key. That credential belongs to the server that issued it: presenting it to another one fails authentication with no obvious cause, and keeping it leaves a live secret for a machine you've stopped talking to. Saving the same URL keeps it, so an unrelated settings save cannot silently re-enrol the machine — both directions are tested.
The key goes to the keychain, not
config.json— with a test asserting the token does not appear in that file. The display value round-trips like the existing fleet key rather than inventing a mask-and-detect scheme no other secret here uses; consistency beats a bespoke convention for one field.Pairing also takes effect immediately —
startUpstreamstops any previous loop first, so this cannot leave two loops reporting under one identity. The fleet listener still needs a restart, and still says so.The harness builds the real graph
SaveSettingsgoes througha.ready(), which needs cfg, catalog, store, runtime and services. The test harness now constructs all of them, so these tests drive the real settings path. Carving out a narrower helper to make the tests easy would have left the path users actually hit untested.Evidence
Full Go suite green,
go vetandgofmtclean. Four negative controls fire:Frontend typechecked with the real compiler (
tsc --noEmitin Docker on the build host). Worth noting:npx tscin this repo silently installs a bogustsc@2.0.4package that typechecks nothing and exits 0 — my first "typecheck passed" was meaningless.Summary by CodeRabbit