Send stable installation identity when pairing#137
Merged
Conversation
📝 WalkthroughWalkthroughAdds persistent InstallationID support: new field in Config, generated via NewInstallationID/EnsureInstallationID, included in PairRequest and persisted on successful pairing, and validated by an added doctor check that reports InstallationID using shortID formatting. Changes
Sequence DiagramsequenceDiagram
participant User
participant Login
participant Config
participant PairAPI
User->>Login: start login
activate Login
Login->>Config: Load() (may be os.ErrNotExist)
Config-->>Login: config or error
alt config missing
Login->>Login: NewInstallationID()
else config exists
Login->>Config: EnsureInstallationID()
Config-->>Login: InstallationID (existing or newly generated)
end
Login->>PairAPI: POST /api/daemon/pair (includes installationId)
activate PairAPI
PairAPI-->>Login: success (pair response)
deactivate PairAPI
Login->>Config: Save config (persist InstallationID, MachineID, token)
Config-->>Login: saved
deactivate Login
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/login_test.go (1)
97-114: ⚡ Quick winAdd a regression test for malformed config load errors.
Please add a case where
$HOME/.gsd-cloud/config.jsoncontains invalid JSON and assertbuildPairRequestreturns an error. This will lock in the intended behavior for non-NotExist load failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/login_test.go` around lines 97 - 114, Add a new test (e.g., TestBuildPairRequestErrorsOnMalformedConfig) that sets HOME to t.TempDir(), creates a malformed config file in the same place the production loader expects, then calls buildPairRequest("ABC234","test-host") and asserts it returns a non-nil error (and no valid request). This mirrors TestBuildPairRequestMintsInstallationIDForLegacyConfig setup (use t.Setenv("HOME", t.TempDir())), but instead of using config.Save(), write invalid JSON to the config file location so buildPairRequest triggers a JSON decode/load failure and your test locks in the expected error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/login.go`:
- Around line 123-126: The buildPairRequest function is swallowing all errors
from config.Load() by returning (req, nil); change it to propagate the actual
load error instead: when cfg, err := config.Load() returns a non-nil err, return
the current req value and that err (or wrap it with context) so callers can
handle malformed/permission/load failures instead of proceeding as if
successful; update any callers of buildPairRequest if they expect only a nil
error.
---
Nitpick comments:
In `@cmd/login_test.go`:
- Around line 97-114: Add a new test (e.g.,
TestBuildPairRequestErrorsOnMalformedConfig) that sets HOME to t.TempDir(),
creates a malformed config file in the same place the production loader expects,
then calls buildPairRequest("ABC234","test-host") and asserts it returns a
non-nil error (and no valid request). This mirrors
TestBuildPairRequestMintsInstallationIDForLegacyConfig setup (use
t.Setenv("HOME", t.TempDir())), but instead of using config.Save(), write
invalid JSON to the config file location so buildPairRequest triggers a JSON
decode/load failure and your test locks in the expected error behavior.
🪄 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 Plus
Run ID: 51c42a92-d842-4c77-8c52-3a6db70640f3
📒 Files selected for processing (7)
cmd/doctor.gocmd/login.gocmd/login_test.gointernal/api/pair.gointernal/api/pair_test.gointernal/config/config.gointernal/config/config_test.go
glittercowboy
force-pushed
the
codex/stable-machine-identity
branch
from
83aa10b to
d1fe223
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/login.go (1)
59-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist
InstallationIDbefore callingPair().Right now the ID is only written to disk after
client.Pair(req)succeeds. If the request reaches the server but the response is lost, the process exits, or this later save fails, the next retry will mint/send a differentInstallationID, which defeats the stable identity / idempotent re-pair goal of this change. Save the ensured/generated ID before the network call so retries reuse the same value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/login.go` around lines 59 - 67, The InstallationID must be persisted before making the network Pair() call: create or update a config.Config with the ensured/generated InstallationID (e.g., set cfg.InstallationID = req.InstallationID or create cfg var) and call config.Save(cfg) prior to invoking client.Pair(req), so retries reuse the same InstallationID; after successful Pair() you can then update the remaining fields (AuthToken, TokenExpiresAt, MachineID, RelayURL, ServerURL) and save again with config.Save(cfg).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/login.go`:
- Around line 59-67: The InstallationID must be persisted before making the
network Pair() call: create or update a config.Config with the ensured/generated
InstallationID (e.g., set cfg.InstallationID = req.InstallationID or create cfg
var) and call config.Save(cfg) prior to invoking client.Pair(req), so retries
reuse the same InstallationID; after successful Pair() you can then update the
remaining fields (AuthToken, TokenExpiresAt, MachineID, RelayURL, ServerURL) and
save again with config.Save(cfg).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dd42c3e5-2ffd-4663-b17c-0f7c9e435677
📒 Files selected for processing (7)
cmd/doctor.gocmd/login.gocmd/login_test.gointernal/api/pair.gointernal/api/pair_test.gointernal/config/config.gointernal/config/config_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/api/pair.go
- internal/api/pair_test.go
- cmd/doctor.go
- internal/config/config.go
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.
Summary
installationIdduring pairing so cloud pairing can restore the same machine row.gsd-cloud doctorand surfaces malformed config load errors.Local Feedback
go test ./cmd ./internal/api ./internal/configgo test ./...Merge / Release Notes
installationId; full idempotent re-pairing is active when the cloud migration/API are deployed.