RFC-003: WiFi apply design (review only, do not merge) - #482
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an RFC for WiFi network management, defining the catalog/intent/reality model, versioned apply semantics, reconciliation and safety rules, UI sync behavior, open questions, and a phased rollout plan. ChangesWiFi Apply RFC
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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
🤖 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 `@decisions/003-wifi-apply-rfc.md`:
- Around line 77-88: D3/D4 currently conflict on rotation identity: D3 assumes
old and new PSKs can coexist during staged rotation, but D4 reconciles only by
SSID and mutates one profile in place. Update the model around ApplyNetworks and
the reconciliation rules in D4 so rotation has an explicit identity separate
from SSID, or clearly state that duplicate SSIDs are supported during rotation
and how smithd tracks them. Make the apply/delete behavior in D4 align with that
identity so the staged old+new window is preserved without ambiguity.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40e112b6-df36-4d07-8b00-6dee62505b0e
📒 Files selected for processing (1)
decisions/003-wifi-apply-rfc.md
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 (2)
decisions/003-wifi-apply-rfc.md (2)
57-60: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftGuard the report path against stale versions.
device.observed_intent_versionanddevice.network_conditionsneed a monotonic update rule here; otherwise a delayed or duplicated report with an olderapplied_versioncan overwrite newer state. Ignore or reject staleapplied_versions before writing these fields.🤖 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 `@decisions/003-wifi-apply-rfc.md` around lines 57 - 60, The report-handling flow for `device.observed_intent_version` and `device.network_conditions` needs a monotonic update rule. Update the logic that processes profile reports so stale or duplicated `applied_version` values are ignored or rejected before those fields are written, and only accept reports that advance the version in the device state.
88-92: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNarrow the adoption wording. D4 already allows in-place modification of same-SSID profiles regardless of who created them, so “profiles smithd never applied are never touched” is too broad. Reword it so only profiles outside the intent SSID set are guaranteed untouched; keep deletion limited to the previous applied list.
🤖 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 `@decisions/003-wifi-apply-rfc.md` around lines 88 - 92, Narrow the adoption language in the RFC text: the current wording in the apply/reconciliation section overstates that profiles smithd never applied are never touched, which conflicts with the same-SSID in-place modification behavior described elsewhere. Update the prose around the intended SSID reconciliation rules to say that only profiles outside the intent SSID set are guaranteed untouched, while deletion remains limited to entries in smithd’s previous applied list; keep the discussion consistent with the apply logic and the adoption lifecycle section.
🤖 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.
Outside diff comments:
In `@decisions/003-wifi-apply-rfc.md`:
- Around line 57-60: The report-handling flow for
`device.observed_intent_version` and `device.network_conditions` needs a
monotonic update rule. Update the logic that processes profile reports so stale
or duplicated `applied_version` values are ignored or rejected before those
fields are written, and only accept reports that advance the version in the
device state.
- Around line 88-92: Narrow the adoption language in the RFC text: the current
wording in the apply/reconciliation section overstates that profiles smithd
never applied are never touched, which conflicts with the same-SSID in-place
modification behavior described elsewhere. Update the prose around the intended
SSID reconciliation rules to say that only profiles outside the intent SSID set
are guaranteed untouched, while deletion remains limited to entries in smithd’s
previous applied list; keep the discussion consistent with the apply logic and
the adoption lifecycle section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a6fd48b-e2f1-4c17-8ba8-5932a0d368da
📒 Files selected for processing (1)
decisions/003-wifi-apply-rfc.md
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
decisions/003-wifi-apply-rfc.md (1)
88-100: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRoll back the temporary profile on failed rotation.
The success path is covered, but the failure path leaves the newly created same-SSID profile unaccounted for. Because D4 only deletes entries from the previous applied list, a failed apply can strand a broken duplicate SSID on disk and make the next reconciliation ambiguous.
🤖 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 `@decisions/003-wifi-apply-rfc.md` around lines 88 - 100, The failed same-SSID rotation path leaves the temporary profile behind, so update the apply/adoption lifecycle to clean up any profile created during a failed connect attempt before returning failure. In the D4/D5 flow, ensure the temporary profile is only persisted into the applied-record after successful connection, and explicitly delete or roll back that profile on failure so the next reconciliation does not see a stranded duplicate SSID.
🤖 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 `@decisions/003-wifi-apply-rfc.md`:
- Around line 60-61: The reset-to-NULL path in the device sync-state handling
breaks the monotonic behavior described for report application. Update the logic
around the API write of `device.observed_intent_version` and
`device.network_conditions` so that unversioned reports do not overwrite newer
stored state when a versioned report has already been applied; only allow the
NULL reset for true first-seen or state-loss cases. Use the existing
apply/version comparison flow and the dashboard state derivation rules to
preserve `Synced` from regressing to `Unknown`.
---
Outside diff comments:
In `@decisions/003-wifi-apply-rfc.md`:
- Around line 88-100: The failed same-SSID rotation path leaves the temporary
profile behind, so update the apply/adoption lifecycle to clean up any profile
created during a failed connect attempt before returning failure. In the D4/D5
flow, ensure the temporary profile is only persisted into the applied-record
after successful connection, and explicitly delete or roll back that profile on
failure so the next reconciliation does not see a stranded duplicate SSID.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33b228eb-5894-4a96-a50c-6e2293a437bd
📒 Files selected for processing (1)
decisions/003-wifi-apply-rfc.md
… rotation failure
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 (2)
decisions/003-wifi-apply-rfc.md (2)
100-104: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winApply the rollback cleanup to every failure mode.
D5 only names
Failed: WrongPSK, but D6 also definesNotInRangeandNmcliError. As written, non-password failures could leave the temporary profile behind and violate the “no stranded duplicate” guarantee.🤖 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 `@decisions/003-wifi-apply-rfc.md` around lines 100 - 104, The rollback cleanup in the same-SSID rotation flow is only described for the WrongPSK path, but the same temporary NM profile teardown must happen for every failure in the apply flow. Update the apply logic that creates and connects the temporary profile so that any non-success exit, including NotInRange and NmcliError, deletes the temporary profile before returning a Failed condition, while preserving the existing success path where the surviving profile is recorded.
113-115: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDefine what advances the verification timestamp.
This section derives freshness from
last_seen/ heartbeats, but the earlier problem statement says stable devices emit no reports. If there is no periodic signal, “verified 30 seconds ago” will never advance for a healthy device after the last edit. Please clarify which event refreshes verification, or the trust signal will look stale on unchanged networks.🤖 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 `@decisions/003-wifi-apply-rfc.md` around lines 113 - 115, Clarify the freshness source for verification in the Wi‑Fi picker logic: the current `verified_at`/`last_seen` wording in the verification section suggests stable devices may never advance, so define exactly which signal updates trust for a healthy network. Update the `Verification` policy text and related picker behavior so the timestamp is refreshed by a concrete recurring heartbeat or equivalent read-time derivation, rather than only by rare event-driven reports, and make this consistent with the `last_seen` / `verified_at` rules described here.
🤖 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.
Outside diff comments:
In `@decisions/003-wifi-apply-rfc.md`:
- Around line 100-104: The rollback cleanup in the same-SSID rotation flow is
only described for the WrongPSK path, but the same temporary NM profile teardown
must happen for every failure in the apply flow. Update the apply logic that
creates and connects the temporary profile so that any non-success exit,
including NotInRange and NmcliError, deletes the temporary profile before
returning a Failed condition, while preserving the existing success path where
the surviving profile is recorded.
- Around line 113-115: Clarify the freshness source for verification in the
Wi‑Fi picker logic: the current `verified_at`/`last_seen` wording in the
verification section suggests stable devices may never advance, so define
exactly which signal updates trust for a healthy network. Update the
`Verification` policy text and related picker behavior so the timestamp is
refreshed by a concrete recurring heartbeat or equivalent read-time derivation,
rather than only by rare event-driven reports, and make this consistent with the
`last_seen` / `verified_at` rules described here.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 363311b2-6c90-42bd-832b-ab73c8311bb8
📒 Files selected for processing (1)
decisions/003-wifi-apply-rfc.md
LudeeD
left a comment
LudeeD
left a comment
There was a problem hiding this comment.
overall ambituous, but feels the right direction. the intent/actual split its definetely the direction we want to go in.
Since we are tackling this from first principles, it would be worth think on how this would potentially support network types that are not just ssid and psk
for example: connections that are used at some sites today: with peap and mschapv2 auth
nmcli connection add type wifi con-name DEMO ssid 'ssid' autoconnect yes save yes 802-11-wireless.powersave 2 802-11-wireless-security.key-mgmt wpa-eap 802-1x.eap peap 802-1x.phase2-auth mschapv2 802-1x.identity 'identity' 802-1x.password 'password'
| Adoption is what fixes the stranded device: the wrong-PSK BLE profile has the intended SSID, so apply corrects it in place instead of racing a parallel profile for the same SSID (NetworkManager autoconnect picks one profile per SSID; leaving a broken higher-priority twin means flapping). | ||
| *Rejected alternative 1:* never touch profiles Smith didn't create. Makes the stranded-device case permanently unfixable remotely, which is the single most valuable case. | ||
| *Rejected alternative 2:* key reconciliation on NM profile *name* instead of SSID. Profile names are arbitrary local identity; SSID is what the network actually is, and same-SSID conflicts are the real hazard. | ||
| *Input wanted on:* the adoption lifecycle. Once adopted, a profile is smith-managed forever, including deletion when removed from intent. Half-management ("we fix your password but never remove you") seemed worse; disagree if you see a case. |
There was a problem hiding this comment.
i like this. smith manage it all, and the sync is enforced
|
|
||
| *Input wanted on two things:* | ||
| 1. **The precedence rule.** Is "operator beats department default" right, or do you see cases where fleet uniformity should win over a manual override (e.g. an operator's forgotten debugging assignment blocking a site-wide migration)? | ||
| 2. **How department defaults are stored.** Option (a): the App API writes plain rows into `device_network_intent` per device (simple, but 500 devices × 2 networks = 1000 rows that a sync job must keep consistent). Option (b): a department template stored once, with each device's effective intent computed as template + device overrides (the Flight Control fleet model: more machinery, no sync drift, scales to fleet-wide changes in one write). This RFC only commits to the `managed_by` column, which works under both; the a/b choice can wait for phase 2 but early opinions steer the schema. |
There was a problem hiding this comment.
sync job across devices, there's no department concept in fleet
| In phase 2 the App API starts assigning department networks to devices automatically, which creates the possibility of conflict: the department default says network A, but an operator deliberately set device X to network B (a repeater, a wired-only corner, a debugging session). Proposed rule: **operator rows win on conflict, and the app-api writer may only add or remove rows it created**, so an automated sync can never silently undo a human's fix. | ||
|
|
||
| *Input wanted on two things:* | ||
| 1. **The precedence rule.** Is "operator beats department default" right, or do you see cases where fleet uniformity should win over a manual override (e.g. an operator's forgotten debugging assignment blocking a site-wide migration)? |
There was a problem hiding this comment.
sure, I would need to see what this means in practice, but doesnt sound bad
d0be525 to
566d759
Compare
2 participants
What
Adds
decisions/003-wifi-apply-rfc.md: the design proposal for WiFi network management in Smith (device intent, declarative apply, catalog trust).This PR will not be merged. It exists so the RFC can be reviewed with inline comments; the document lives on the branch regardless. Rendered version: decisions/003-wifi-apply-rfc.md
Why
Since #477 and #480 we can observe a device's WiFi state, but we still cannot change it: a BLE-provisioned device with a wrong password is stranded until someone physically plugs a 4G dongle, and the network catalog silently rots (create-only sync from the App API, see #481). The RFC proposes the apply mechanism and a rollout that avoids any flag-day deploy.
How to review
[settled unless objected]ship as described unless you push back by the deadline in the header (2026-07-10).[input wanted](D4 adoption lifecycle, D8 auto-heal policy, D10 precedence + storage model) and the four Open Questions need actual answers.Approach
Three-layer model (catalog / intent / reality) with disjoint writers; sync state via version echo (Kubernetes
generation/observedGenerationstyle) instead of list diffing; one declarativeApplyNetworkscommand reconciled by SSID on-device; three phases (manual apply → catalog trust → App API department defaults). Ten decisions, each with rejected alternatives, grounded in prod data gathered this week.Testing
Checklist