[mvanhorn]

Why

In the CLI, a user can pick one discovered record at the prompt and then type a different host. If the authenticated probe of that host returns no ACP identity, configure fell back to the selected record's syAP and printed it as the device's syAP, even though that record describes another device. The result is a wrong label in that configure printout and in that run's device_syap telemetry. Nothing reaches the .env, deploy, doctor or flash, since TC_AIRPORT_SYAP is in ENV_FILE_OMIT_KEYS.

This is hardening for that CLI-only edge case. The macOS app sends either a selected record or a host, never both, so it cannot reach it. On current main, an ACPd that has quit already fails configure with ACPD_QUIT before identity matters, so what is left is a transient acp -q failure while ACPd is still running.

Summary

• ConfigureTargetResolution.selected_record now holds the record only when the host came from it, and is empty for a typed or saved host. The target_record property is gone, so no caller can pick up a record for another device by mistake. Both callers pass this field as ConfigureFlowRequest.selected_record, the only place configure learns about the record. run_configure_flow already used it for the ACP record and the MAC pin; it now also reads the record's syAP from it, for the unsupported-model check and for the identity fallback.
• discovered_airport_syap and selected_record_airport_syap are removed from ConfigureFlowRequest and ConfigureTargetResolution, and from both callers (cli/configure.py, app/ops/configure.py).
• observed_device_identity's argument is renamed to record_airport_syap. Its fallback source is now selected_record instead of discovered, and the source is typed as SyapSource = Literal["probed", "selected_record"]. The CLI prints it as "Using TC_AIRPORT_SYAP from the selected device". The single-use ConfigureValueChoice wrapper and its print_automatic_value_choice branches for sources nothing produces are replaced by print_syap_choice.
• Configure's finish telemetry gets device_syap_source next to device_syap, in the CLI and the app, so a report can tell the device's own syAP from the record's stand-in.
• The discovered_airport_syap debug field is dropped: both the one the CLI added whenever a record was discovered and the one on the unsupported-device failure. Telemetry keeps the value: the selected_bonjour_record debug field already carries the record's syAP, and the unsupported-device message names it.
• One case loses a label it used to get right: a host typed for the same device the record describes, such as the LAN IP of a record that advertised only link-local addresses, now gets no syAP when ACP returns no identity. An empty label is better than one that may describe another device, and the probe still reports the device's own syAP whenever ACP answers.
• The syAM/syAP mismatch diagnostic from earlier revisions is gone. The parser keeps main's behavior of leaving model and syAP empty on a mismatch.

Validation

• test_configure_record_syap_stands_in_only_for_the_records_own_host drives the CLI main(): it selects a record with syAP 119 and probes a device with no ACP identity, in two subtests.

  • Keeping the record's host: TC_AIRPORT_SYAP is 119, the output says "Using TC_AIRPORT_SYAP from the selected device: 119", and telemetry's finish gets device_syap 119, device_syap_source selected_record and device_model TimeCapsule8,119.
  • Typing root@10.0.0.2: no TC_AIRPORT_SYAP in the output, the values or finish, and no device_syap_source or device_model.

  Both subtests fail on main's src/, which prints "Using discovered TC_AIRPORT_SYAP: 119" in both cases, and pass with this change.

• test_configure_telemetry_says_where_the_syap_came_from runs the app's configure twice: with a host whose probe reads syAP 119, finish gets device_syap_source probed; with a selected record advertising 119 and a probe with no ACP identity, it gets selected_record.

• test_configure_typed_host_over_record_enables_ssh_without_the_record (renamed from test_configure_uses_discovered_airport_syap_without_prompting): a typed host with SSH closed. ACP gets no record, and the reprobe's syAP 119 is reported over the record's 113. This guards behavior main already had.

• test_configure_probed_syap_beats_record_and_invalid_existing_syap (renamed from test_configure_discovered_syap_beats_invalid_existing_syap): the reprobe's 119 wins over the record's 113 and a saved 999, and the output says "Using probed TC_AIRPORT_SYAP: 119".

• The resolver tests assert the resolution carries no record for a typed or saved host. The service and target tests now pass a record (record_advertising_syap) instead of syAP fields. A selected record advertising syAP 115 is still refused before SSH or ACP, and the app's error debug still carries the record's syAP 115. test_run_configure_flow_ignores_unsupported_syap_from_record_the_host_did_not_come_from is deleted: the resolver test covers it, since a typed host gets no record.

• Merged with main 5ab32a82: the configure, configure target, configure defaults and app API test files pass, and ruff check passes. At the head commit, the test files that import a changed module (app API, CLI configure, configure service, configure target, configure defaults, operation context, telemetry, Bonjour integration and labels) pass: 887 passed.

Refs #359

[jamesyc]

Thanks for this. The code is tidy, and the new tests plus the nearby suites pass on this branch (772 passed). I'm requesting changes because the PR doesn't fix the issue it closes, and it adds a hard stop based on an unconfirmed assumption about real devices.

Blocking

1. This doesn't fix #359, so it shouldn't close it.
#359 is about a label: the CLI calls syAP 113 "Time Capsule 3rd generation", but the reporter's unit is an A1409. That label is correct. syAP 113 is the 3rd-gen A1355, and the A1409 is 116 (see the table in discussion #334).

The two-device theory is possible in principle, but in v3.1.0 (the version the reporter ran) it can't produce this report:

• Configure uses the Bonjour syAP only when the device's own ACP read over SSH comes back empty or mismatched; observed_device_identity prefers the probed compatibility.exact_syap. A healthy A1409 would answer 116, and that wins whatever discovery says. So the A1409's ACP read would have had to fail during configure, and nothing in the report suggests it did.
• Every other command reads syAP from the device itself. The flash read gets it over network ACP (services/flash.py, "Reading ACP product property syAP..."), and deploy and doctor take their label from the SSH probe's ACP read. The reporter's flash read worked (acp_match=True). If the unit answered 116, those commands would have said "4th generation", and the reporter would have seen 113 and 116 disagree instead of a steady "3rd generation".

So the unit itself most likely reports 113, which means it's really an A1355 or mislabeled. acp -q syAP and acp -q syAM output from the reporter would settle it. Keeping configure from using a different record's syAP is still reasonable hardening for an edge case, so please describe it that way and change Closes #359 to Refs #359.

2. A syAM/syAP disagreement is now a hard refusal, with no evidence it happens on real hardware.
Before, a mismatch left the probe's identity empty and configure carried on. Now run_configure_flow raises DEVICE_IDENTITY_MISMATCH, with no override. If any real unit's firmware reports a syAM and syAP that disagree, configure stops working on it. Please confirm that real devices never disagree before making this fatal. Telemetry can't settle it, because the mismatch detail was never recorded.

The user also sees the raw parser text ("AirPort identity mismatch: syAM … expects syAP 113, got 116"). And the recovery card for DEVICE_IDENTITY_MISMATCH (app/recovery.py) says the device "did not identify itself as the saved device… Refresh discovery and reconnect" and marks it retryable. That advice is wrong here: a device whose own fields disagree will fail the same way every time.

3. The new fallback test uses a state that can't occur.
test_missing_acp_identity_uses_the_selected_record_not_discovery passes discovered_airport_syap="119" with selected_record_airport_syap="113". In production, ConfigureTargetResolution.selected_record_airport_syap returns either discovered_airport_syap or None, so the two can never differ. The case this PR actually changes isn't tested: a typed host, discovered "119", selected None, and no ACP identity, which should now give identity.syap is None. The existing …ignores_unsupported_syap_from_record_the_host_did_not_come_from doesn't cover it, because its probe still returns an ACP identity.

Non-blocking

4. ConfigureFlowRequest.discovered_airport_syap is now dead weight. Its only remaining use is the new callbacks.debug(discovered_airport_syap=…) line, which duplicates existing logging: the CLI already logs that field (cli/configure.py), and the app logs the whole selected record (app/ops/configure.py). The unsupported-model branch also writes the same debug key with the selected record's value, so one key now means two things. Simpler: drop the field from the request and both callers, then rename observed_device_identity's argument to something like record_airport_syap.

5. Two ProbeResult fields where one would do. airport_identity_detail is only set when airport_identity_mismatch is true. A single airport_identity_mismatch: str | None field holding the diagnostic carries the same information.

6. Configure and deploy now disagree. Deploy and doctor still accept a mismatched device, so a hand-written .env skips the new check. That's fine if configure is meant to be the only gate, but please say so in the PR.

Checked and ruled out

• Stale saved syAP when identity comes back empty: not a risk. TC_AIRPORT_SYAP is in ENV_FILE_OMIT_KEYS, so configure never carries an old value forward.
• Effect of dropping the discovery fallback: small. Identity only feeds the CLI printout, the app result and telemetry.

[jamesyc]

Thanks for the quick turnaround. This commit takes the earlier suggestions: a syAM/syAP mismatch no longer stops configure, the two ProbeResult fields are now one, discovered_airport_syap is gone from the request, and the fallback test now builds its target through resolve_configure_target. Tests and Ruff pass locally (772 passed), and it merges cleanly with main.

I'm still requesting changes. The main new test doesn't catch the bug it describes, the new log line almost never reaches telemetry, and the PR description no longer matches the code.

Blocking

1. The headline test passes on main's code too, so it guards nothing.
I ran test_typed_host_without_acp_identity_leaves_syap_empty against main's src/ and it passed. The test calls resolve_configure_target, asserts the target's discovered_airport_syap is "119", and then never hands that value to run_configure_flow. It only passes selected_record_airport_syap=None, and an empty fallback gives no syAP on any branch.

The actual fix is the two deleted call-site lines in cli/configure.py and app/ops/configure.py, and no test exercises either call site. A real regression test has to run the CLI's main() with a selected record plus a typed host, and a probe with no ACP identity, then assert on the printed or JSON device_syap. The app path can't reach this case at all; see 4.

2. "Log mismatches instead of failing" logs to almost nowhere.
callbacks.debug(...) adds a debug field, and debug fields reach telemetry only inside a failure's error text (OperationContext.build_error, failure_debug). Configure now continues on a mismatch, so a successful run drops the diagnostic. It shows up only if configure later fails for some unrelated reason. The probe summary in that error text doesn't include the new field either.

That leaves a parser flag, a new ProbeResult field, a probe test and a configure branch, all for one line that is usually thrown away. Either send it through a channel that goes out on success, or delete all of it. callbacks.measurement(...) lands in execution on every finish, as locate.py's host_follow does. The case has never been seen on a real device, so deleting it is a fair choice.

3. The PR description is stale and still closes #359.

• It still says configure "raises a device identity mismatch and stops", which this commit reverses.
• It still says "the discovery syAP is logged for telemetry", which this commit deletes.
• Its Validation section still describes the first commit's tests.
• Closes #359 is unchanged, although point 1 of the earlier review asked for Refs #359. If you disagree with that point, please say why.
• The opening claim that a mismatched device "is still configured from that advertisement" overstates things. TC_AIRPORT_SYAP is in ENV_FILE_OMIT_KEYS, and render_env_text never writes it, so nothing gets configured from it. The new test's assertNotIn("TC_AIRPORT_SYAP", result.values) checks a dict that never reaches disk.

Non-blocking, but it changes whether the PR is worth merging

4. The bug is now very hard to reach, so please restate the case for it after rebasing on main.

• CLI only. The macOS app sends either selected_record or host, never both (OperationParams.Configure.save). In the app, a typed host never comes with a record, which leaves only the CLI case where the user picks one record at the prompt and types a different host.
• Usually stopped earlier on main now. Since this branch was cut, main gained a check that fails configure with ACPD_QUIT when ACPd has quit (services/configure.py, right after observed_device_identity). That is the realistic way to get an authenticated probe with no ACP identity, and it now fails before identity matters.
• What's left: a transient acp -q failure while ACPd is still running, or a syAM/syAP mismatch that has never been observed.
• What it costs when it happens: a wrong label in one configure printout and in that run's device_syap telemetry. Nothing reaches the .env, deploy, doctor or flash.

GitHub reports it as mergeable, but no CI has run on this PR. Please rebase and get CI green rather than rely on a textual merge.

5. Leftover naming. syap_source="discovered" now means "from the selected record". The CLI prints it as the value's source, so it should be "selected_record" or "record".

Bottom line

After this commit, the code change amounts to deleting two call-site kwargs, plus a diagnostic that rarely leaves the Mac. That's a fine small cleanup if it comes with:

• a test that drives the CLI path,
• the mismatch diagnostic either sent as a measurement or removed,
• an accurate description that says Refs #359.

[mvanhorn]

Thanks for the detailed review. 7854697 covers points 2 through 5:

• A syAM/syAP mismatch no longer stops configure. The probe leaves model and syAP empty, the diagnostic goes to debug output, and configure continues as it did before.
• The 119/113 fallback test is replaced by test_typed_host_without_acp_identity_leaves_syap_empty: a typed host built through resolve_configure_target over a discovered syAP 119 record, no ACP identity, asserting identity.syap is None.
• discovered_airport_syap is dropped from ConfigureFlowRequest and both callers, along with the duplicate debug line, and the observed_device_identity argument is now record_airport_syap.
• ProbeResult has a single airport_identity_mismatch: str | None field.

ruff check and the full test suite (5240 passed, 2 skipped) pass locally.

[mvanhorn]

Thanks for the second pass. 1faca20 is rebased on current main and replaces the earlier commits:

1. The new test test_configure_typed_host_without_acp_identity_does_not_take_selected_record_syap drives the CLI main(): it selects a record with syAP 119, types a different host, and probes a device with no ACP identity. It fails on main's src/ (printing "Using discovered TC_AIRPORT_SYAP: 119") and passes with this change.
2. The mismatch diagnostic is removed: no probe field, probe test or configure branch. The parser keeps main's behavior.
3. The description is rewritten to match the code and now says Refs #359.
4. The description now frames this as hardening for the CLI-only case where a typed host differs from the selected record, after the ACPD_QUIT check on main.
5. The fallback source is now selected_record, printed as "Using TC_AIRPORT_SYAP from the selected device".

ruff check . and the full suite (5384 passed, 2 skipped) pass locally.

[jamesyc]

Thanks, this revision resolves every blocking point from the last round:

• The test now catches the bug. I ran test_configure_typed_host_without_acp_identity_does_not_take_selected_record_syap against main's src/: it fails, printing Using discovered TC_AIRPORT_SYAP: 119, and it passes on this branch.
• The mismatch plumbing is gone: no probe field, no parser flag and no configure branch.
• The description now matches the code, scoped as CLI-only hardening behind main's ACPD_QUIT check, with Refs #359.
• It's rebased onto current main as one commit, CI is green, and the related suites pass locally (799 passed).

The code change itself is right. Accepting the record's own host still clears explicit_host in cli/configure.py, so the correct fallback still works for the normal pick-and-accept flow. What's left is test coverage and the PR title. I'll approve once 1–3 are in.

Should fix before merge

1. The renamed source and its new message have no test, and a mistake there would print nothing.
This PR changes the source label from "discovered" to "selected_record" and adds the line "Using TC_AIRPORT_SYAP from the selected device". No test asserts either one. No test checks syap_source at all, and the service test that passes selected_record_airport_syap="119" only checks the written value. print_automatic_value_choice has no else branch, so a misspelled source, or a producer still sending "discovered", would print nothing while every test still passes.

Please add the positive case: pick the record, accept its default host, probe with no ACP identity, then assert Using TC_AIRPORT_SYAP from the selected device: 119 in the output and TC_AIRPORT_SYAP == "119" in result.values. Next to the new negative test, that covers the fallback from both sides.

2. test_run_configure_flow_ignores_unsupported_syap_from_record_the_host_did_not_come_from no longer tests what its name says.
With discovered_airport_syap="115" gone, there's no unsupported syAP left in its input. It now just passes selected_record_airport_syap=None and checks the probed 119, which is the same as the None subtest of test_run_configure_flow_continues_for_supported_or_unusable_selected_record_syap right above it. The behavior it was written for is already covered at the resolver level: test_configure_target.py asserts that a typed host over a syAP-115 record gives selected_record_airport_syap is None. Please delete it, or rename it to match what it now checks.

3. The PR title still describes the first revision.
"Preserve target identity during Time Capsule configuration" overstates a change that only stops one fallback. This will be squashed and the title usually becomes the commit message, so please retitle it to the commit's own headline: "Keep the selected record's syAP off a typed host".

Nits

• The new call-site comment # selected_record_airport_syap is empty unless the host came from that record. repeats the field comment a few lines up in ConfigureFlowRequest. One of them is enough.
• Pre-existing, but confusing next to the new test: test_configure_uses_discovered_airport_syap_without_prompting uses the same record and typed host as the new test, yet asserts Using probed TC_AIRPORT_SYAP: 119. Its value comes from the probe after SSH is enabled, not from discovery. Renaming it to say "probed" would make the two tests easier to read side by side.

[mvanhorn]

Thanks. 872cf6c covers points 1 and 2 and both nits:

• The new test test_configure_selected_record_without_acp_identity_uses_record_syap picks the record, accepts its own host, and probes a device with no ACP identity. It asserts Using TC_AIRPORT_SYAP from the selected device: 119 in the output and TC_AIRPORT_SYAP == "119" in result.values.
• test_run_configure_flow_ignores_unsupported_syap_from_record_the_host_did_not_come_from is deleted.
• The call-site comment is gone, so the field comment in ConfigureFlowRequest is the only one.
• test_configure_uses_discovered_airport_syap_without_prompting is renamed to test_configure_uses_probed_airport_syap_without_prompting.

ruff check and the configure, configure target, configure defaults and locate test files (214 passed) pass locally.

[jamesyc]

Thanks for working through the review rounds. I rebased this onto main as one commit and matched the new test's style to its neighbors. The fallback is now tested from both sides, and CI is green.