fix: reliable multi-device discovery and chat --device flag

[moskovich]

Summary

• mDNS discovery on Linux: switches to avahi-browse subprocess, which reliably finds all devices. bonjour-service was silently missing devices on Linux due to multicast handling. Falls back to bonjour on macOS/Windows or if avahi isn't installed.
• Bonjour timeout: 2s → 5s for better reliability on slower networks
• device add: removed apiKey/topicID prompts — CLI is LAN-only for now; those fields aren't needed and users have no way to obtain them
• upsertDevice: no longer writes empty apiKey/topicID strings to new device entries
• chat --device: adds -d, --device <name> flag for parity with play, send, and ssh
• Docs: removes stale apiKey/topicID from config example; adds --device to chat options; adds missing device and ssh subcommands to public CLI docs

Risk zone

Yellow — cross-surface changes to device discovery and multi-device routing. Anh and Brandon to review.

Test plan

• ff1 device add discovers both devices on Linux
• ff1 device list shows both devices cleanly (no empty apiKey/topicID fields)
• ff1 chat --device kitchen "..." routes to the correct device
• ff1 send playlist.json -d office still works
• All 21 unit tests pass (npm test)

Related

• Relayer auth architecture issue filed for future work: feral-file/ff-cloud-command-service#14

🤖 Generated with Claude Code

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 0, request-changes: 3).

1. chat --device is still not implemented on the main AI chat path, which is the headline feature of this PR. index.ts correctly accepts and forwards options.device into buildPlaylist(...), but in src/main.ts:187-258 that value is only consumed by the special send ... shortcut at src/main.ts:205-249. For the normal intent-parser / orchestrator flow, defaultDeviceName is never merged into params.playlistSettings.deviceName before the request is handed off, so commands like ff1 chat -d kitchen "build something and display it" can still target the wrong device unless the prompt text itself names the device. This was independently raised by all 3 reviewers, and the current code still supports that concern.

2. The new docs promise a non-interactive ff1 device add --host ... --name ... flow, but the implementation still always opens readline and always prompts for the name. In index.ts:1367-1416, even when both --host and --name are already provided, the command still calls ask(namePrompt). That conflicts with docs/CONFIGURATION.md and docs/README.md, and it means scripted or non-TTY usage can block on stdin instead of completing directly. This was also raised repeatedly across the reports and remains valid in the current diff.

3. device remove introduces device-name matching semantics that conflict with the rest of the multi-device feature. resolveConfiguredDevice() uses exact, case-sensitive name matching in src/utilities/ff1-compatibility.ts:61-79, and the new tests explicitly preserve Kitchen and kitchen as distinct devices in tests/multi-device.test.ts:90-99. But index.ts:1456-1458 lowercases both sides and removes the first match, so ff1 device remove kitchen can delete the wrong configured device in a setup the rest of the CLI can distinguish. This was only raised by one reviewer, but re-review shows it is a real correctness issue introduced by this PR.

I also checked the “missing tests” points. Those are valid as supporting context, but they are downstream of the issues above rather than separate review blockers: there is still no coverage for chat --device on the AI-driven path, and no coverage for the documented non-interactive device add --host --name behavior.

[feralfile-bot]

Automated human review request for PR #30.

Risk zone: yellow
Responsibility area: FF1 device discovery and selection
Why this needs human review: This PR changes the operator-facing device discovery path with a new Linux-specific subprocess dependency and alters multi-device routing behavior; that is narrow but meaningful enough to benefit from a steward check.

This PR needs targeted human review on the scoped areas below, not a full second pass of the whole PR.

• @lpopo0856: please review Linux avahi-browse discovery, parser fallback, and device-selection semantics. Context: runtime reliability (Brandon Yeh).

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 0, request-changes: 3)

• Critical: chat --device is only honored in the literal send... shortcut. The normal chat path still reaches src/main.ts without any device override, so ff1 chat --device office "..." can still fall back to the first configured device. This was independently flagged by reviewers 2 and 3, and it affects the primary new behavior.
• Medium: device add --host --name is still interactive. The command always opens readline and prompts for a device name in index.ts, so the documented non-interactive flow blocks waiting for stdin even when both flags are supplied. Also reported by reviewers 2 and 3.
• Medium: Updating an existing host can silently rename the device to ff1 if the name prompt is left blank. In the overwrite path in index.ts, the fallback does not preserve the stored label, so a previously named device can be clobbered during an edit.
• Medium: The new Linux discovery branch ignores the timeout contract. src/utilities/ff1-discovery.ts hard-codes an avahi-browse timeout and never forwards options.timeoutMs into that path, so callers cannot bound discovery duration on Linux.
• Medium: Name-based routing can still hit stale duplicates. upsertDevice() in index.ts dedupes only by host, so re-adding the same logical device under a new host can leave multiple entries with the same name; later selection in src/utilities/ff1-compatibility.ts will return the first match, which may be the stale device.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 0, request-changes: 3)

Critical issues

• Duplicate device names are still accepted, but device lookup is exact-name plus first-match. That makes ff1 send -d kitchen, ssh -d kitchen, and chat-driven sends ambiguous and can route to the wrong device with no error. I re-checked the new device add flow and it only warns at write time (index.ts), while resolution still returns the first matching name (ff1-compatibility.ts). This is a real correctness bug, not just a UX issue.

Medium issues

• chat --device is only honored by the literal send ... shortcut. The CLI flag is passed into buildPlaylist() (index.ts), but the main AI-driven send_playlist path ignores it and sends sendParams.deviceName from the model instead (src/main.ts). Reviewers 1 and 3 both flagged this, and it will still target the wrong/default device on the common path.
• Updating an existing device by host can silently lose the stored device name. In both setup and ff1 device add, the overwrite default is derived from discoveredName or --name instead of the current config entry, so pressing Enter after Update this device? relabels it to ff1 or the discovered mDNS name. Reviewers 2 and 3 both called this out, and it breaks later name-based selection.
• The setup/status check still treats only ff1Devices.devices[0] as meaningful (index.ts). In a multi-device config, a bad first entry can still report the whole config as missing even if later devices are valid, so the new multi-device flow is still carrying a single-device assumption.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 0, request-changes: 3)

Critical issues

• The chat --device override is still dropped on the direct send path. [src/main.ts](/Users/ffci/codex/repo/feral-file_ff1-cli-pr30-QamzMC/repo/src/main.ts#L328) only copies the CLI fallback into playlistSettings.deviceName, but the send_playlist branch reads sendParams.deviceName at [src/main.ts](/Users/ffci/codex/repo/feral-file_ff1-cli-pr30-QamzMC/repo/src/main.ts#L333). That means a chat request resolved as send_playlist can ignore --device and route to the default device instead. This is the same root cause all three reviewers independently called out, so it should be treated as the main blocker. Add a regression test for the send_playlist intent path with the CLI device override.

Medium issues

• Rediscovering an already configured host can silently rename the stored device entry. In [index.ts](/Users/ffci/codex/repo/feral-file_ff1-cli-pr30-QamzMC/repo/index.ts#L1446), the prompt default prefers discoveredName over the existing saved name, so pressing Enter after rediscovery writes the discovery label back into config. That mutates user-managed labels and can break exact-match routing for send and ssh. Preserve the existing name when the host is already configured, and add a regression test for the blank-response case.

Overall: the routing regression is a merge blocker, and the label-clobbering behavior is also worth fixing before merge.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 0, request-changes: 3)

Medium issues

• src/utilities/ff1-discovery.ts:104 and src/utilities/ff1-discovery.ts:115 make the Linux path lossy: Avahi headers are split on whitespace and the service name is lowercased before it is stored. That can truncate multi-word instance names and rewrite mixed-case labels, but src/utilities/ff1-compatibility.ts:78 does an exact name lookup, so a Linux-discovered device may be impossible to target later under the label the operator saw. This needs regression coverage for mixed-case and multi-word discovery output.
• src/utilities/device-upsert.ts:26 removes a same-name device and appends the replacement to the end of the array. Because the CLI defaults to devices[0] when --device is omitted, re-adding or rediscovering a moved device can silently change the implicit target for play, send, and ssh. That is user-visible routing drift; either preserve array position or lock down the new defaulting rule with a test.

I rechecked the reported same-host metadata-loss concern and did not carry it forward: the host-match branch in upsertDevice() spreads the existing entry, so apiKey and topicID survive.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 0, request-changes: 3)

• Critical: src/utilities/device-upsert.ts:30 still replaces same-name, different-host entries with { ...newDevice }, which drops any existing apiKey and topicID. That is real config data loss, and it matters because sendPlaylistToDevice() and sendSshAccessCommand() still consume those fields. This was the one issue all three reviewers independently converged on, so it should stay active; it also needs a regression test for re-adding an existing device after a host change.
• Critical: src/main.ts:334 copies the CLI --device flag into playlistSettings.deviceName before the intent result is known, and src/utilities/index.js:526 sends whenever that field is defined. In practice, ff1 chat --device ... now turns a build-only chat into an implicit send to hardware, which is a user-visible behavior regression with a network side effect. Two reviewers flagged the same path; I rechecked it and the concern is valid. Add a regression test that a build-only chat request with --device does not dispatch unless the intent parser selected send_playlist.
• Medium: the setup flow at index.ts:528 still defaults rediscovered devices to selection.discoveredName || 'ff1', so pressing Enter can overwrite an already curated label when the same host is seen again. The device add path now preserves the existing name default, so this is limited to setup, but it is still enough to make exact-name targeting flaky. This should get a small regression test around rediscovery of an existing host.
• Medium: src/utilities/ff1-discovery.ts:188 treats any non-empty avahi-browse stdout as a successful Linux discovery, even when the subprocess exits non-zero, so Bonjour fallback is bypassed in partial-failure cases. That weakens the reliability fix the PR is aiming for because Linux discovery can still come back partial instead of recovering through the fallback path. A test should cover the non-zero-with-stdout branch, or the code should explicitly document that behavior if partial results are intentional.

Overall verdict: request changes.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 1, request-changes: 2)

• src/utilities/ff1-discovery.ts:181-199 still drops every avahi-browse result on any non-zero exit and falls back to Bonjour, even when stdout may already contain valid resolved devices. That weakens the main Linux reliability fix this PR is trying to land, and it was the recurring concern across reports, so I’d keep it as the highest-priority issue. Add a regression test for the “usable stdout + non-zero exit” case so the fallback only happens when the avahi output is actually unusable.
• index.ts:620-623 only validates ff1Devices.devices[0].host while advertising the full device count in ff1 status. In a multi-device config, a malformed second or later entry can still show the whole block as OK, which makes the new plural-device summary misleading. Validate the full array, or fail status when any configured device is malformed.

I rechecked the reported chat --device precedence and raw "null" override concerns and did not raise them: confirmPlaylistForSending() already normalizes "null" and "" to undefined, and the direct send shortcut does not go through the auto-select path described in the reports.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 1, request-changes: 2)

Critical issues

• index.ts:527 and index.ts:1422 still regress re-adding a device whose host changed. Setup and device add only preserve the stored friendly name on an exact host match, so when a previously configured FF1 comes back on a new IP the blank prompt falls back to the raw mDNS label and upsertDevice() appends a duplicate row instead of updating the existing one. That leaves the stale device in slot 0, so play/send/ssh can keep targeting the wrong host. This is the same issue independently raised by reviewers 1 and 3.

Missing tests

• tests/chat-device-routing.test.ts and src/main.ts:334 still leave the --device routing path under-tested. The current tests validate extracted helpers, but not the actual buildPlaylist send shortcut and send_playlist fallback branches that decide whether defaultDeviceName is honored. A regression test that drives those branches directly would better protect the user-facing chat --device flow.
• src/utilities/ff1-discovery.ts:181 adds a new avahi-browse non-zero-exit fallback path, but the current discovery tests only cover parseAvahiBrowseOutput. A targeted test for the execFile error branch would make the Linux reliability change much safer.

[feralfile-bot]

Reviewers: 3 (approve: 0, request-changes: 3)

1. src/utilities/device-lookup.ts still does not reconcile a stored IP host with a rediscovered .local name, and index.ts still marks already configured by exact host equality. That leaves the setup and device add flows able to treat the same FF1 as new after an address change, default to the raw discovery label, and create duplicate entries instead of preserving the saved friendly name. This is the same host-change regression raised by reviewers 1 and 2, and there is still no regression test covering the IP↔.local case or the discovery-list state.

I did not keep the chat --device routing concern from reviewer 3: the explicit send_playlist path in src/main.ts already falls back to the CLI flag, and I did not find a separate orchestrator regression introduced by this diff.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 0, request-changes: 3)

• src/utilities/device-lookup.ts and the write path in index.ts still do not make reconciliation end-to-end. findExistingDeviceEntry() checks exact host before id, so a stale configured row can win over the stable device identity, and upsertDevice() still dedupes only by host/name, so when a rediscovered device gets a new friendly label it is appended instead of updating the matched entry. That breaks the PR’s core host-change reconciliation goal and can silently reorder the default device list.
• src/utilities/device-upsert.ts is still vulnerable to erasing stored identity metadata: callers pass id: discoveredId unconditionally, so any same-host/same-name update that does not have a discovery id overwrites a previously persisted id with undefined. This was independently raised by multiple reviewers, and it removes the metadata the new matching logic depends on.
• src/utilities/ff1-discovery.ts still assumes _ff1._tcp will appear in the resolved avahi-browse -r header line when extracting the service name. Real Avahi resolved output does not expose the type token that way, so multi-word device names can be truncated to the first word and later exact-name matching will fail on Linux.
• index.ts makes the selection prompt wording more permissive than the matcher actually is. The prompt says enter ID/host, but the comparison logic only checks the raw lowercased answer against bare host forms, so a pasted URL-form host like http://ff1-hh9jsnoc.local:1111 will not select the device unless the user strips the scheme first.

Overall verdict: request changes.

[feralfile-bot]

Reviewers: 3 (approve: 2, request-changes: 1, comment: 0)

Critical issues

• src/utilities/device-lookup.ts and index.ts still leave a legacy gap for pre-id configs: if a stored device has a curated name like kitchen but no id, and rediscovery returns a .local host whose avahi label is just the raw service label or hostname-derived id, findExistingDeviceEntry() falls through and upsertDevice() appends a duplicate entry instead of updating in place. That means IP-to-.local migration is still broken for an important class of existing configs. The current coverage in tests/device-lookup.test.ts exercises stored-id and TXT-name fallback paths, but not this no-id + renamed-host regression.

[feralfile-bot]

Reviewers: 3 (approve: 1, request-changes: 2)

• Medium: src/utilities/ff1-discovery.ts only keeps one resolved address per service, but src/utilities/device-lookup.ts depends on the full address set to bridge IP ↔ .local migrations. On dual-stack or otherwise multi-address FF1s, avahi can emit more than one resolved IP; if the stored IP is not the one that survives parsing, findExistingDeviceEntry() misses the match and device add/setup can append a duplicate instead of updating in place. This is the same defect both reviewers 1 and 3 raised, and the current tests only cover a single address line, so the regression remains unguarded.

I’m not elevating the updated: false log wording from reviewer 3; that looks like a UX nit rather than a functional bug.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 1, request-changes: 2)

Critical issues

• None.

Medium issues

• src/utilities/device-lookup.ts: findExistingDeviceEntry() only does IP↔.local reconciliation when discoveredAddresses is present. The new device add --host <ip> path calls it with discoveredAddresses unset, so an existing .local row will not match and the command can append a duplicate device instead of updating in place.
• src/utilities/ff1-discovery.ts: when avahi emits a later partial record for the same host:port key, the merge preserves only addresses and overwrites name, id, and txt with the later record. That can clobber a previously complete device entry and drop TXT metadata needed for stable lookup/routing.
• src/main.ts: the direct send_playlist branch still trusts sendParams.deviceName verbatim. confirmPlaylistForSending() already normalizes sentinel values on the shortcut path, but this path does not, so a parser-emitted literal "null" can override the CLI --device fallback and misroute or fail the send.

Missing tests

• No focused regression covers the manual device add --host case where an existing .local entry should be recognized without discoveredAddresses.
• No test asserts that merged avahi records preserve the earlier name/txt fields instead of being overwritten by a later partial record.
• No test exercises the parser-emitted "null" case on the send_playlist path.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 0, request-changes: 3)

• IPv6 reverse lookup still never runs because findExistingDeviceEntry() only treats dotted-quad hosts as IPs at src/utilities/device-lookup.ts. Since discovery now persists IPv6 addresses, ff1 device add --host [fe80::1] can miss an existing .local entry and create a duplicate on dual-stack or IPv6-only networks. This was raised by all three reviewers, and tests/device-lookup.test.ts should add an IPv6 regression for the stored-address fallback.
• The discovery merge still prefers existing?.txt unconditionally at src/utilities/ff1-discovery.ts, so a partial record that has txt = [] leaves {} behind and blocks a later complete TXT payload from replacing it. That conflicts with the merge intent in the surrounding comment and can leave the device name/id stuck on incomplete data. tests/avahi-parse.test.ts needs a partial-then-complete regression here.
• upsertDevice() still overwrites addresses wholesale on every update at src/utilities/device-upsert.ts. If a later discovery only reports a subset of previously observed addresses, the stored list shrinks and the new reverse-lookup path can stop matching the device again, reintroducing duplicates. tests/device-upsert.test.ts should cover address preservation or merging on update.

I did not raise the chat --device end-to-end wiring test gap separately because the helper coverage and the direct option pass-through in index.ts make it lower risk than the parser/lookup regressions above.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 2, request-changes: 1)

Critical issues

• src/utilities/device-upsert.ts and src/utilities/device-lookup.ts together leave stale IPs in addresses across host changes, and the reverse lookup still trusts that list for --host <ip>. Re-reviewing this confirms the reviewer 1 finding: a device that moved can continue to match an old IP and be resolved as the wrong configured FF1. That is a real correctness/routing bug, not just a cosmetic issue.

Missing tests

• The coverage gap around the discovery-to-upsert flow is still real and was raised by more than one reviewer: there is no command-level regression test for ff1 device add / setup that exercises the Linux avahi-browse branch plus the selection-to-upsert wiring end to end. The current tests cover parsing and helpers in isolation, but not the top-level CLI path.
• There is still no end-to-end regression test for chat --device through src/main.ts and src/main.ts, so the send_playlist fallback when the intent parser omits a device remains unverified outside the helper tests. This was also called out in review and is worth closing before merge.

Overall, this is not merge-ready yet because the stale-address lookup bug is user-visible and the new CLI routing paths still lack end-to-end coverage.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 1, request-changes: 2)

Critical issues

• device add still allows duplicate friendly names, and the new upsert path resolves them by first-match overwrite. The command only warns, then index.ts calls upsertDevice which replaces the first row with that name. In a multi-device config that can silently clobber the wrong device entry and make the previous one unreachable. This needs either hard duplicate-name rejection or explicit update semantics; there is no regression test for the collision case.

Medium issues

• device remove only matches d.name, so unnamed legacy/manual entries can be used elsewhere in the CLI but cannot be removed. See index.ts. Add coverage for the unnamed-device shape, or teach the command how to target the implicit first device when no name is present.

[feralfile-bot]

Reviewers: 3 (approve: 1, request-changes: 2, comment: 0)

Critical

• The same name-collision overwrite bug is still reachable in both setup and device add: findExistingDeviceEntry() can fall back to discoveredName, so a newly discovered device that advertises a friendly name already used by another row is resolved to the wrong existing entry, and upsertDevice() then replaces that other device instead of rejecting the add. The duplicate-name guard only runs after lookup and only protects the already-matched row, so it does not prevent this overwrite. This was raised independently by two reviewers, so it should be treated as confirmed rather than speculative.

Missing tests

• Add end-to-end coverage for the collision path: one test where two distinct devices share the same friendly/TXT name but have different ids/hosts and the lookup must not collapse them into one row, plus one setup/device-add test proving the flow rejects or reprompts instead of overwriting the other device.

[feralfile-bot]

Reviewers: 3 (approve: 0, request-changes: 3)

• Critical: setup still writes through upsertDevice without the same-name collision guard that device add now uses (index.ts vs index.ts). A new device that advertises a friendly name already owned by another row can silently replace the wrong entry and make the original device disappear. Reviewers 1 and 3 both flagged this, and the setup path still lacks a regression test of its own.

• Medium: findExistingDeviceEntry now excludes id-bearing rows from both TXT-name fallback and the resolved-address fallback (src/utilities/device-lookup.ts and src/utilities/device-lookup.ts). That means a partial avahi result or a manual ff1 device add --host ... run with no discovery metadata can fail to land on an already-configured device and instead trip the name-collision rejection. Reviewers 2 and 3 reached the same root cause from different angles.

• Medium: normalizeDeviceHost still does not canonicalize bare IPv6 input (index.ts), so ff1 device add --host fe80::1 can still produce an invalid http://fe80::1 form instead of a bracketed URL. The discovery path is now IPv6-aware; the manual path needs the same normalization.

[feralfile-bot]

Reviewers: 3 (approve: 0, request-changes: 3)

Critical issues

• None after re-review.

Medium issues

• src/utilities/device-upsert.ts:45 and src/utilities/device-lookup.ts:93: the new addresses cache is merged on same-host updates and then treated as a valid reverse-lookup signal, so a device that keeps the same .local host but gets a new DHCP lease can retain stale IPs indefinitely. That can make ff1 device add --host <old-ip> resolve the wrong saved row later. The current tests cover host-change replacement, but not same-host lease churn.
• index.ts:171 and index.ts:309: normalizeDeviceIdToHost() is case-sensitive for both scheme detection and the ff1- prefix. Uppercase pasted input like HTTP://192.168.1.10:1111 or FF1-HH9JSNOC can be rewritten into malformed hosts instead of matching cleanly, which breaks the manual-entry fallback and the discovered-device picker. The new tests do not cover uppercase/pasted input.

Missing tests

• tests/device-upsert.test.ts, tests/device-lookup.test.ts, and tests/chat-device-routing.test.ts: add regressions for same-host IP churn and uppercase device/URL input so these paths are exercised directly.

Overall, the PR is not merge-ready yet. I re-checked reviewer 3's chat --device concern and did not keep it: the send_playlist action is resolved through resolveSendPlaylistDeviceName() before dispatch, and the CLI flag is intentionally not merged into build-only playlist settings.

[feralfile-bot]

Reviewers: 3 (approve: 1, request-changes: 2).

• High: src/utilities/device-upsert.ts:29 preserves existing.addresses whenever the incoming patch omits addresses, and that leaves the stale-IP bug alive on two paths the reviewers independently flagged: a partial Linux avahi timeout result with host/id but no addresses, and the manual ff1 device add --host <ip> update path. In both cases, later reverse-IP matching in src/utilities/device-lookup.ts:93 can continue to bind an old IP to the wrong FF1 after DHCP churn or host migration, so send/ssh can target the wrong device. This recurrence across reviewers makes it the merge-blocking issue here; it needs regression coverage for both partial avahi output and the existing-device --host update path.

[feralfile-bot]

Reviewers: 3 (approve: 0, request-changes: 3)

1. src/utilities/device-lookup.ts / index.ts: findExistingDeviceEntry() still suppresses TXT-name fallback for any stored row with an id, so a rediscovery that only has the host/name but no discoveredId or addresses cannot reconcile an already-configured device. In the device add/setup flow that turns a legitimate re-add or host migration into either a duplicate append or a false name-collision error instead of an in-place update. Two reviewers independently hit this same gap; please add a regression for partial discovery with an id-bearing stored row.

2. index.ts / src/utilities/device-upsert.ts: if lookup finds the existing physical device by hostname/IP but the user chooses a new friendly name, upsertDevice() has no path to update that matched row unless the name also matches an existing entry. For id-less migrated devices this falls through to the append path and silently creates a duplicate config row. Add a workflow test for rename-on-rediscovery so the matched row is updated in place.

3. src/intent-parser/index.ts / src/main.ts: the confirm_send_playlist path still calls confirmPlaylistForSending() without the CLI --device fallback, so ff1 chat --device kitchen ... can still prompt for device selection or use the parser's omission instead of the CLI default. That leaves the new --device parity incomplete on the send path; add a regression covering confirm_send_playlist with no model-provided deviceName.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 1, request-changes: 2)

Medium issues

• src/main.ts:449 still calls buildPlaylistWithAI(...) without forwarding the CLI --device value, and the orchestrator only auto-sends when params.playlistSettings.deviceName is already present. In a ff1 chat --device <name> build-and-send flow, the operator-selected device can still be dropped unless the model independently re-emits it.
• src/utilities/ff1-discovery.ts:288 still treats avahi errors with no parsed stdout, including timeout cases, as a hard fallback to Bonjour. That leaves the Linux discovery path exposed on slow or busy scans, which is the same reliability gap this PR is trying to close.

Missing tests

• The new chat-routing coverage is still helper-level only; there is no end-to-end test for the real buildPlaylist -> processIntentParserRequest -> buildPlaylistWithAI send flow, so the device propagation issue above could regress unnoticed.
• There is no regression test for discoverFF1Devices() when avahi times out or returns no stdout, so the Linux fallback behavior is still unpinned.

[feralfile-bot]

Reviewers: 3 (approve: 1, comment: 1, request-changes: 1)
Missing tests

• src/utilities/ff1-discovery.ts still lacks an end-to-end test for the Linux process.platform === 'linux' branch in discoverFF1Devices() and the Bonjour fallback decision. The new tests cover parseAvahiBrowseOutput() and resolveAvahiResult(), but not the actual branch that chooses avahi vs. Bonjour, so a regression in the platform gate or fallback wiring would still slip through.

I rechecked the reported setup retry leak and did not keep it: the finally block closes rl.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 1, request-changes: 2)

Critical issues

• src/utilities/ff1-discovery.ts still returns null when avahi-browse exits non-zero and stdout parses to zero devices, which sends Linux back to Bonjour. That contradicts the contract above resolveAvahiResult() and can reintroduce the original missing-devices behavior on partial avahi output. No regression test covers the non-zero-exit / usable-stdout / zero-devices branch.

Medium issues

• src/utilities/device-lookup.ts disables TXT-name fallback for any stored row that already has an id, even when the new discovery has neither discoveredId nor addresses. In the partial-avahi path, that prevents re-associating an existing device after a host change and pushes device add toward duplicate entries or name-collision prompts. There is no test for the partial-avahi / no-id / no-addresses case.
• index.ts plus src/utilities/device-lookup.ts still cannot update an existing IP-stored device when the user runs ff1 device add --host <.local-url> and discovery data is unavailable. Because the new host is not an IP, the reverse-address lookup never runs, so the name-collision guard treats the existing row as a different device instead of updating it. No test covers the --host migration path.

I did not keep the IPv6 host:port concatenation concern because the current avahi/Bonjour discovery paths feed hostnames into those call sites, so I could not verify a live failure from this diff.

[lpopo0856]

The avahi parser itself is mostly in decent shape. It handles multi-record merges, dual-stack addresses, partial TXT, and timeout behavior better than before. I do not think the parser is the main review risk anymore.

The weak point is still identity reconciliation between “the device I just found” and “the row already in config.” The PR is trying to make device identity stable across host changes, but the final write path still does not preserve the specific matched row when the user changes the friendly name. That means the code is close, but not yet internally consistent.

• if the CLI correctly recognizes an existing device, but the user renames it during re-add/setup, it can still create a duplicate instead of updating the matched row

• manual migration from an IP-stored device to a .local host without discovery metadata can still fail to reconcile

[feralfile-bot]

Reviewers: 3 (approve: 0, request-changes: 3)

• Critical: index.ts now skips the duplicate-name guard whenever findExistingDeviceEntry() returns no match, and that is broader than the intended migration-only exception. In both setup and device add, a brand-new device that happens to reuse an existing friendly name can now fall through to upsertDevice() case 3 and overwrite the already-configured row in place. That is a data-loss regression, and it is the same underlying identity-reconciliation risk called out in the PR discussion and independently by all three reviewer reports.
• Missing regression coverage: there is still no test that exercises the real CLI collision path where existingIndex === -1 and the chosen name matches another configured device. The current workflow tests document the old guard behavior, but they do not protect this newly introduced bypass, so the overwrite regression remains unguarded.

Overall verdict remains request changes until the collision guard is narrowed or restored and the missing regression test is added.