Breadway/breadcrumbs
1
0
Fork 0
Code Issues 2 Pull requests 1 Projects Releases Packages Wiki Activity Actions

fix: breadcrumbs audit batch (CRUMB-25, 28, 32, 35, 36, 37, 44) #46

Merged
Breadway merged 9 commits from fix/crumb-audit-batch into main 2026-10-20 18:31:40 +08:00
Conversation 4 Commits 9 Files changed 8 +626 -45
Breadway commented 2026-10-20 15:35:16 +08:00
Owner
Copy link

Batch of breadcrumbs audit findings. Each card is one commit on this branch with a test that covers it.

CRUMB-25: detect --apply --json emitted more than one JSON object

  • How found: audit of detect --apply --json, which printed the apply result and the detection result as separate JSON objects, so a consumer parsing one object failed.
  • Fix: emit exactly one JSON object (f66fca8, src/app.rs).
  • Test: unit coverage in src/app.rs for the single-object output.

CRUMB-28: connect did not propagate hidden=false to the saved profile

  • How found: audit of connect. A network un-hidden in config kept 802-11-wireless.hidden=true in the NetworkManager profile, because the rewrite only ran for credential changes or hidden=true.
  • Fix: profile_needs_update compares the saved hidden flag in both directions, and treats a missing key as false (d4e313a, src/nm.rs).
  • Test: unhiding_a_network_triggers_a_profile_update_and_clears_the_flag and hidden_flag_matching_the_saved_profile_does_not_force_a_rewrite in src/nm.rs.

CRUMB-32: DNS pin left DHCP DNS on IPv6 when the profile had no ipv6 section

  • How found: audit of apply_dns. The pin created the ipv4 section but only touched ipv6 if it already existed, so a profile without one kept auto DNS around the pin.
  • Fix: the pin creates the ipv6 section the same way as ipv4, with method=auto when creating a missing IP section; existing methods are preserved (rework d4db5b8). Unpin still only touches existing sections (4b5f017, src/nm.rs).
  • Tests: apply_dns_pin_creates_ipv6_section_when_profile_has_none (fails before the fix), apply_dns_pin_v4_only_still_clears_ipv6_auto_dns_on_created_section (fails before the fix), apply_dns_unpin_does_not_invent_ipv6_section, apply_dns_unpin_without_ipv6_section_leaves_no_ipv6_when_pinned_before.

CRUMB-35: a blank password answer could wipe a network created concurrently

  • How found: audit of cmd_add. existing was computed from the config loaded at startup, before the interactive prompt. The locked Config::update closure reloads the config, so a network created in the meantime was treated as new and got its password set to none.
  • Fix: the existence check runs inside the locked closure against the fresh config. The keep-or-write decision is the pure function password_to_write. The prompt stays outside the lock (c3889ea, src/app.rs).
  • Tests: blank_prompt_keeps_password_when_network_exists_in_fresh_config, blank_prompt_sets_no_password_for_a_new_network, non_blank_prompt_sets_password_for_existing_and_new_networks, explicit_password_always_replaces_even_when_empty.

CRUMB-36: a non-204 HTTP answer with a blocked ping was not treated as a portal

  • How found: audit of the connectivity check. A captive portal that answered the HTTP probe with a non-204 status, while blocking ping, was reported as online.
  • Fix: a non-204 HTTP answer together with a blocked ping is classified as a portal (a2e3e2c, src/status.rs).
  • Tests: unit coverage in src/status.rs plus production connectivity() coverage in tests/flow_watch.rs for 403/404 with blocked ping, 000/500 with blocked ping, and 403 with working ping (rework 2dff10b).

CRUMB-44: a configured profile key with surrounding spaces could not be selected from the bus

  • How found: audit of the set_profile bus command. The payload was trimmed before the config lookup, so a configured key such as " home " could never match. A whitespace-only payload also reported missing string "profile", which was misleading.
  • Fix: the payload is forwarded untrimmed. On apply, an exact configured key wins, and the trimmed name is the fallback. A whitespace-only payload reports an empty-profile error (319e473, src/bread_events.rs).
  • Tests: src/bread_events.rs unit tests (exact_profile_key_with_spaces_resolves_to_itself, padded_payload_falls_back_to_trimmed_key, exact_key_wins_over_trimmed_key_when_both_exist, whitespace_only_payload_reports_empty_profile_not_missing_field, absent_or_non_string_payload_reports_missing_field). The integration test set_profile_command_trims_name_before_applying is renamed to set_profile_command_falls_back_to_trimmed_name_when_applying and now asserts the new contract. The tests use config fixtures only and do not touch the bus.

CRUMB-37: two release workflows published the same tag

  • How found: audit of the release path. .github/workflows/release.yml and .forgejo/workflows/release.yml both ran on v* tag pushes, and both wrote /srv/breadway-dl/breadcrumbs/<version> and ran gh release upload --clobber. Forgejo is authoritative, since GitHub auto-mirrors from origin.
  • Fix: the GitHub release workflow is workflow_dispatch only (e44b642). The Forgejo release path is unchanged.
  • Test: tests/release_workflows.rs, which asserts the GitHub workflow does not trigger on tag pushes and the Forgejo one still triggers on v*.
  • Note: CLAUDE.md is not tracked in this repo (removed in 7a58d4a and listed in .gitignore as local-only), so there is no tracked file to rewrite. This card's CLAUDE.md part is therefore not applicable here.

Verified locally: cargo fmt --all -- --check, cargo clippy --all-targets -- -D warnings, cargo test --release.

Refs CRUMB-25 CRUMB-28 CRUMB-32 CRUMB-35 CRUMB-36 CRUMB-37 CRUMB-44

Batch of breadcrumbs audit findings. Each card is one commit on this branch with a test that covers it. ## CRUMB-25: detect --apply --json emitted more than one JSON object - How found: audit of `detect --apply --json`, which printed the apply result and the detection result as separate JSON objects, so a consumer parsing one object failed. - Fix: emit exactly one JSON object (f66fca8, src/app.rs). - Test: unit coverage in src/app.rs for the single-object output. ## CRUMB-28: connect did not propagate hidden=false to the saved profile - How found: audit of connect. A network un-hidden in config kept `802-11-wireless.hidden=true` in the NetworkManager profile, because the rewrite only ran for credential changes or hidden=true. - Fix: `profile_needs_update` compares the saved hidden flag in both directions, and treats a missing key as false (d4e313a, src/nm.rs). - Test: `unhiding_a_network_triggers_a_profile_update_and_clears_the_flag` and `hidden_flag_matching_the_saved_profile_does_not_force_a_rewrite` in src/nm.rs. ## CRUMB-32: DNS pin left DHCP DNS on IPv6 when the profile had no ipv6 section - How found: audit of `apply_dns`. The pin created the ipv4 section but only touched ipv6 if it already existed, so a profile without one kept auto DNS around the pin. - Fix: the pin creates the ipv6 section the same way as ipv4, with `method=auto` when creating a missing IP section; existing methods are preserved (rework d4db5b8). Unpin still only touches existing sections (4b5f017, src/nm.rs). - Tests: `apply_dns_pin_creates_ipv6_section_when_profile_has_none` (fails before the fix), `apply_dns_pin_v4_only_still_clears_ipv6_auto_dns_on_created_section` (fails before the fix), `apply_dns_unpin_does_not_invent_ipv6_section`, `apply_dns_unpin_without_ipv6_section_leaves_no_ipv6_when_pinned_before`. ## CRUMB-35: a blank password answer could wipe a network created concurrently - How found: audit of `cmd_add`. `existing` was computed from the config loaded at startup, before the interactive prompt. The locked `Config::update` closure reloads the config, so a network created in the meantime was treated as new and got its password set to none. - Fix: the existence check runs inside the locked closure against the fresh config. The keep-or-write decision is the pure function `password_to_write`. The prompt stays outside the lock (c3889ea, src/app.rs). - Tests: `blank_prompt_keeps_password_when_network_exists_in_fresh_config`, `blank_prompt_sets_no_password_for_a_new_network`, `non_blank_prompt_sets_password_for_existing_and_new_networks`, `explicit_password_always_replaces_even_when_empty`. ## CRUMB-36: a non-204 HTTP answer with a blocked ping was not treated as a portal - How found: audit of the connectivity check. A captive portal that answered the HTTP probe with a non-204 status, while blocking ping, was reported as online. - Fix: a non-204 HTTP answer together with a blocked ping is classified as a portal (a2e3e2c, src/status.rs). - Tests: unit coverage in src/status.rs plus production connectivity() coverage in tests/flow_watch.rs for 403/404 with blocked ping, 000/500 with blocked ping, and 403 with working ping (rework 2dff10b). ## CRUMB-44: a configured profile key with surrounding spaces could not be selected from the bus - How found: audit of the `set_profile` bus command. The payload was trimmed before the config lookup, so a configured key such as `" home "` could never match. A whitespace-only payload also reported `missing string "profile"`, which was misleading. - Fix: the payload is forwarded untrimmed. On apply, an exact configured key wins, and the trimmed name is the fallback. A whitespace-only payload reports an empty-profile error (319e473, src/bread_events.rs). - Tests: src/bread_events.rs unit tests (`exact_profile_key_with_spaces_resolves_to_itself`, `padded_payload_falls_back_to_trimmed_key`, `exact_key_wins_over_trimmed_key_when_both_exist`, `whitespace_only_payload_reports_empty_profile_not_missing_field`, `absent_or_non_string_payload_reports_missing_field`). The integration test `set_profile_command_trims_name_before_applying` is renamed to `set_profile_command_falls_back_to_trimmed_name_when_applying` and now asserts the new contract. The tests use config fixtures only and do not touch the bus. ## CRUMB-37: two release workflows published the same tag - How found: audit of the release path. `.github/workflows/release.yml` and `.forgejo/workflows/release.yml` both ran on `v*` tag pushes, and both wrote `/srv/breadway-dl/breadcrumbs/<version>` and ran `gh release upload --clobber`. Forgejo is authoritative, since GitHub auto-mirrors from origin. - Fix: the GitHub release workflow is `workflow_dispatch` only (e44b642). The Forgejo release path is unchanged. - Test: `tests/release_workflows.rs`, which asserts the GitHub workflow does not trigger on tag pushes and the Forgejo one still triggers on `v*`. - Note: `CLAUDE.md` is not tracked in this repo (removed in 7a58d4a and listed in `.gitignore` as local-only), so there is no tracked file to rewrite. This card's CLAUDE.md part is therefore not applicable here. Verified locally: `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, `cargo test --release`. Refs CRUMB-25 CRUMB-28 CRUMB-32 CRUMB-35 CRUMB-36 CRUMB-37 CRUMB-44
apply_dns created the ipv4 section for the pin but only touched ipv6 if
it already existed. A profile without an ipv6 section kept DHCP/RA DNS
around the pin. The pin now creates the ipv6 section the same way, with
only DNS keys (no ipv6 method, so NetworkManager keeps its default).
Unpin still only touches sections that exist.
cmd_add computed whether the network existed from the startup config,
before the interactive prompt. The locked Config::update closure reloads
the config, so a network created in the meantime could have its password
wiped by a blank answer. The existence check now runs inside the locked
closure against the fresh config, and the keep-or-write choice is a pure
function (password_to_write) with unit tests for existing vs new and
blank vs non-blank input. The prompt stays outside the lock.
A set_profile payload was trimmed before the config lookup, so a
configured profile key such as " home " could never be selected. The
payload is now forwarded untrimmed; apply resolves an exact configured
key first and falls back to the trimmed name. A whitespace-only payload
now reports an empty-profile error instead of a missing-field error.

The integration test that asserted the old trim-on-receive behaviour now
asserts the new contract.
ci(release): publish tags only from Forgejo (CRUMB-37)
All checks were successful
check / check (push) Successful in 6m30s
Details
e44b642c3d 
.github/workflows/release.yml and .forgejo/workflows/release.yml both
ran on v* tag pushes. Both write /srv/breadway-dl/breadcrumbs/<version>
and run 'gh release upload --clobber', so one tag push raced two
publishers. Forgejo is authoritative (GitHub auto-mirrors from origin),
so the GitHub copy is now workflow_dispatch only. The file is kept rather
than deleted so a manual run stays possible; it is the one trigger left.

Not changed: the Forgejo release path, rc-release and dev-release.
CLAUDE.md is not tracked in this repo (removed in 7a58d4a and listed in
.gitignore as local-only), so there is no tracked file here to rewrite.

Adds tests/release_workflows.rs to pin the trigger shape: the GitHub
release workflow must not run on tag pushes, and the Forgejo one must
still run on v*.
Breadway commented 2026-10-20 17:27:59 +08:00
Author
Owner
Copy link

Review of PR #46 at head e44b642c3d (base main 939d9edc). Head re-read from Forgejo before and after review: unchanged.

PR VERDICT: CHANGES REQUESTED (blocking: CRUMB-32; test gap: CRUMB-36). Five cards pass.

Checks on this head: Forgejo "check / check (push)" = success. Independently in a clean worktree: cargo fmt --check rc 0, cargo clippy --all-targets -D warnings rc 0, cargo test rc 0 (113 lib, 54, 56, 2 release_workflows, 0 failures). Existing reviews: none. Branch protection is not readable anonymously, so required-check configuration is unverified.

PER CARD

  • CRUMB-25: PASS. detect --apply --json now prints one object {profile, outcome, ok} (src/app.rs cmd_detect, detect_apply_json); the human line and the bare {"profile"} line are suppressed under --json. Superset of the old key, exit code unchanged. Unit test asserts structure and single-line serialization. It tests the builder, not cmd_detect stdout; acceptable.
  • CRUMB-28: PASS. profile_needs_update (src/nm.rs ~506) compares net.hidden to the saved flag both ways, missing key = false. Tests cover un-hide, no-op when equal, hidden, password. Non-blocking: connect_verbose now reads GetSettings on every connect to an existing profile (nm.rs ~939-942); previously only when an update was needed, so a GetSettings failure now aborts connects that used to succeed.
  • CRUMB-32: CHANGES REQUESTED. See below.
  • CRUMB-35: PASS. Existence is recomputed inside the Config::update closure from the fresh config (app.rs, network_exists then password_to_write); the prompt stays outside the lock; explicit --password/--password-stdin semantics preserved. Four tests pin the decision table. The closure wiring itself is verified by reading, not by a test.
  • CRUMB-36: CHANGES REQUESTED (test only; the code is correct). src/status.rs:62 (else if answered => Portal) is not covered: I replaced it with else if false && answered and the whole suite stayed green. The only new test covers the helper is_unexpected_http_answer, not connectivity(). tests/flow_watch.rs already has a FakeRunner harness (internet_ok_requires_204_and_falls_back_to_ping); add a case: curl answers 403/404, ping fails, expect Portal; curl 000 and 500 with ping failing, expect NoNet; 403 with ping succeeding, expect Online. Nit: the Connectivity doc comment still lists only 200/redirect/511.
  • CRUMB-37: PASS. .github/workflows/release.yml is workflow_dispatch only; .forgejo release.yml (v*, non-rc) and rc-release.yml (v*, rc only) are mutually exclusive. tests/release_workflows.rs asserts both sides. CLAUDE.md: untracked at base (git cat-file fails at 939d9edc; removed in 7a58d4a; listed in .gitignore), so that half is correctly not applicable. Non-blocking hazard: the GitHub workflow still derives VERSION from GITHUB_REF_NAME, so a manual dispatch from a branch writes /srv/breadway-dl/breadcrumbs/ and repoints the "latest" symlink at it. Consider deleting that workflow or guarding the job with if: startsWith(github.ref, 'refs/tags/v').
  • CRUMB-44: PASS. Payload forwarded untrimmed; exact configured key wins, trimmed is the fallback (resolve_profile_key); whitespace-only reports "empty profile" instead of "missing string". Unit tests plus the renamed integration test assert the new contract. Nit: the bread.crumbs.set_profile.done event (bread_events.rs:147) echoes the raw payload, not the resolved key.

CRUMB-32 (blocking)
Change: apply_dns (src/nm.rs:857-858) now creates an ipv6 section containing only ignore-auto-dns and dns, with no method key. Tests assert the absence of method on purpose (nm.rs:1226 and :1237).
Evidence that this is invalid to NetworkManager:

  1. libnm source (NetworkManager main, nm-setting-ip-config.c verify): if (!priv->method) fail with "ipv6.method: property is missing". The method property has no default (g_param_spec_string default NULL), and nm-setting-ip6-config.c init sets none. The documented "auto" default applies only when the whole ipv6 setting is absent (nm-connection.c _normalize_ip_config adds ipv6 method=auto, may-fail=TRUE). A present-but-methodless section is not defaulted.
  2. Update2 parses with STRICT|NORMALIZE (nm-settings-connection.c settings_connection_update), and the keyfile reader also normalizes, so the invalid profile is rejected, not repaired.
  3. Reproduced in-process with the host libnm 1.46.0 (no daemon, no D-Bus, no NMClient): ipv6 {ignore-auto-dns, dns} without method => "ipv6.method: property is missing"; the same dict with method=auto verifies and normalizes. Same failure for the pre-existing ipv4 shape without method.
  4. A profile with no [ipv6] in keyfile, loaded and normalized, comes back with ipv6.method=auto and an ipv6 section in GetSettings-shaped output. So on a live system GetSettings should essentially always include ipv6 with a method, which makes the leak scenario in the card largely theoretical.
    Consequence: if the hypothetical profile ever does occur, enforce_dns's Update2 fails, the error is only logged ("dns: could not apply DNS setting"), and the v4 pin that worked before this change is lost too. That is a regression in the one case the change targets.
    The fake does not catch it: tests/common/fake_nm.rs validate_settings (line 206) checks dns wire types but not that ip sections carry a method, so the tests mirror the code.
    Requested: when creating a missing ipv6 section, also set method=auto (as the create path at nm.rs:697-699 does); flip the two assertions to expect method == "auto"; teach validate_settings to reject ipv4/ipv6 sections without method, so a regression fails in the fake.
    Does pinning stop v6 auto DNS? With method=auto, per nm-settings docs ipv6.ignore-auto-dns=TRUE ignores RA/DHCPv6 name servers and search domains, and only dns/dns-search are used; so yes for this profile's own DNS. Not covered: dns-priority is never set, so other active connections (VPN, second NIC) can still contribute DNS; the card does not claim that. Separate pre-existing issue, not for this PR: with ipv6.method ignore, disabled or link-local, libnm rejects a non-empty ipv6.dns, so a pin with a v6 server on such a profile fails the same way (reproduced in-process).
    Confidence: high that methodless ipv6 is rejected (source plus libnm 1.46.0 run); medium-high that the no-ipv6-section case rarely occurs live. Only provable on a live system: the running daemon's behavior (same libnm code, but I did not run NetworkManager), real RA/RDNSS and DHCPv6 DNS being dropped after Reapply, resulting resolv.conf or systemd-resolved state, and interaction with Tailscale DNS. None of that was touched.

Scope: seven files, all attributable to the seven cards; no scope creep. Added lines and commit messages contain no em or en dashes and no attribution.
Not merged. Cards not moved.

Review of PR #46 at head e44b642c3d8129f5d06eb9df1c57a046977212b9 (base main 939d9edc). Head re-read from Forgejo before and after review: unchanged. PR VERDICT: CHANGES REQUESTED (blocking: CRUMB-32; test gap: CRUMB-36). Five cards pass. Checks on this head: Forgejo "check / check (push)" = success. Independently in a clean worktree: cargo fmt --check rc 0, cargo clippy --all-targets -D warnings rc 0, cargo test rc 0 (113 lib, 54, 56, 2 release_workflows, 0 failures). Existing reviews: none. Branch protection is not readable anonymously, so required-check configuration is unverified. PER CARD - CRUMB-25: PASS. detect --apply --json now prints one object {profile, outcome, ok} (src/app.rs cmd_detect, detect_apply_json); the human line and the bare {"profile"} line are suppressed under --json. Superset of the old key, exit code unchanged. Unit test asserts structure and single-line serialization. It tests the builder, not cmd_detect stdout; acceptable. - CRUMB-28: PASS. profile_needs_update (src/nm.rs ~506) compares net.hidden to the saved flag both ways, missing key = false. Tests cover un-hide, no-op when equal, hidden, password. Non-blocking: connect_verbose now reads GetSettings on every connect to an existing profile (nm.rs ~939-942); previously only when an update was needed, so a GetSettings failure now aborts connects that used to succeed. - CRUMB-32: CHANGES REQUESTED. See below. - CRUMB-35: PASS. Existence is recomputed inside the Config::update closure from the fresh config (app.rs, network_exists then password_to_write); the prompt stays outside the lock; explicit --password/--password-stdin semantics preserved. Four tests pin the decision table. The closure wiring itself is verified by reading, not by a test. - CRUMB-36: CHANGES REQUESTED (test only; the code is correct). src/status.rs:62 (else if answered => Portal) is not covered: I replaced it with else if false && answered and the whole suite stayed green. The only new test covers the helper is_unexpected_http_answer, not connectivity(). tests/flow_watch.rs already has a FakeRunner harness (internet_ok_requires_204_and_falls_back_to_ping); add a case: curl answers 403/404, ping fails, expect Portal; curl 000 and 500 with ping failing, expect NoNet; 403 with ping succeeding, expect Online. Nit: the Connectivity doc comment still lists only 200/redirect/511. - CRUMB-37: PASS. .github/workflows/release.yml is workflow_dispatch only; .forgejo release.yml (v*, non-rc) and rc-release.yml (v*, rc only) are mutually exclusive. tests/release_workflows.rs asserts both sides. CLAUDE.md: untracked at base (git cat-file fails at 939d9edc; removed in 7a58d4a; listed in .gitignore), so that half is correctly not applicable. Non-blocking hazard: the GitHub workflow still derives VERSION from GITHUB_REF_NAME, so a manual dispatch from a branch writes /srv/breadway-dl/breadcrumbs/<branch> and repoints the "latest" symlink at it. Consider deleting that workflow or guarding the job with if: startsWith(github.ref, 'refs/tags/v'). - CRUMB-44: PASS. Payload forwarded untrimmed; exact configured key wins, trimmed is the fallback (resolve_profile_key); whitespace-only reports "empty profile" instead of "missing string". Unit tests plus the renamed integration test assert the new contract. Nit: the bread.crumbs.set_profile.done event (bread_events.rs:147) echoes the raw payload, not the resolved key. CRUMB-32 (blocking) Change: apply_dns (src/nm.rs:857-858) now creates an ipv6 section containing only ignore-auto-dns and dns, with no method key. Tests assert the absence of method on purpose (nm.rs:1226 and :1237). Evidence that this is invalid to NetworkManager: 1. libnm source (NetworkManager main, nm-setting-ip-config.c verify): if (!priv->method) fail with "ipv6.method: property is missing". The method property has no default (g_param_spec_string default NULL), and nm-setting-ip6-config.c init sets none. The documented "auto" default applies only when the whole ipv6 setting is absent (nm-connection.c _normalize_ip_config adds ipv6 method=auto, may-fail=TRUE). A present-but-methodless section is not defaulted. 2. Update2 parses with STRICT|NORMALIZE (nm-settings-connection.c settings_connection_update), and the keyfile reader also normalizes, so the invalid profile is rejected, not repaired. 3. Reproduced in-process with the host libnm 1.46.0 (no daemon, no D-Bus, no NMClient): ipv6 {ignore-auto-dns, dns} without method => "ipv6.method: property is missing"; the same dict with method=auto verifies and normalizes. Same failure for the pre-existing ipv4 shape without method. 4. A profile with no [ipv6] in keyfile, loaded and normalized, comes back with ipv6.method=auto and an ipv6 section in GetSettings-shaped output. So on a live system GetSettings should essentially always include ipv6 with a method, which makes the leak scenario in the card largely theoretical. Consequence: if the hypothetical profile ever does occur, enforce_dns's Update2 fails, the error is only logged ("dns: could not apply DNS setting"), and the v4 pin that worked before this change is lost too. That is a regression in the one case the change targets. The fake does not catch it: tests/common/fake_nm.rs validate_settings (line 206) checks dns wire types but not that ip sections carry a method, so the tests mirror the code. Requested: when creating a missing ipv6 section, also set method=auto (as the create path at nm.rs:697-699 does); flip the two assertions to expect method == "auto"; teach validate_settings to reject ipv4/ipv6 sections without method, so a regression fails in the fake. Does pinning stop v6 auto DNS? With method=auto, per nm-settings docs ipv6.ignore-auto-dns=TRUE ignores RA/DHCPv6 name servers and search domains, and only dns/dns-search are used; so yes for this profile's own DNS. Not covered: dns-priority is never set, so other active connections (VPN, second NIC) can still contribute DNS; the card does not claim that. Separate pre-existing issue, not for this PR: with ipv6.method ignore, disabled or link-local, libnm rejects a non-empty ipv6.dns, so a pin with a v6 server on such a profile fails the same way (reproduced in-process). Confidence: high that methodless ipv6 is rejected (source plus libnm 1.46.0 run); medium-high that the no-ipv6-section case rarely occurs live. Only provable on a live system: the running daemon's behavior (same libnm code, but I did not run NetworkManager), real RA/RDNSS and DHCPv6 DNS being dropped after Reapply, resulting resolv.conf or systemd-resolved state, and interaction with Tailscale DNS. None of that was touched. Scope: seven files, all attributable to the seven cards; no scope creep. Added lines and commit messages contain no em or en dashes and no attribution. Not merged. Cards not moved.
NetworkManager rejects an ipv4 or ipv6 section that has no method
("ipv6.method: property is missing"), so the DNS pin's created sections
made verify and Update2 fail and the pin never took effect. Created
sections now carry method = auto, the default NM applies to a profile
with no section at all. Existing sections keep their method.

The fake NM now rejects a present ipv4/ipv6 section without a method,
so a client that builds an invalid shape fails in tests the way it
would against real NetworkManager.
test(status): cover the non-204 HTTP answer with a blocked ping (CRUMB-36)
All checks were successful
check / check (push) Successful in 5m37s
Details
2dff10b59c 
Pin the connectivity verdicts through the FakeRunner harness: a 403 or
404 with ping blocked is a portal, 000 or 500 with ping blocked is no
network, and a 403 with ping working is online. Breaking the portal
branch in status.rs fails the 403/404 test.
Breadway commented 2026-10-20 17:44:13 +08:00
Author
Owner
Copy link

Rework for the independent review (comment 5672), on the same branch.

  • d4db5b8 fix(nm): give a created ipv4/ipv6 section method auto (CRUMB-32). The DNS pin now creates a missing ipv6 section with method = auto. The same missing-method path existed for ipv4, so that is fixed too. Existing sections keep their method. The assertions at the two previously flagged spots now expect auto. The fake NM's validate_settings rejects a present ipv4/ipv6 section with no method, with unit tests for reject and accept.
  • 2dff10b test(status): cover the non-204 HTTP answer with a blocked ping (CRUMB-36). Tests of connectivity() through FakeRunner: 403/404 with ping blocked gives Portal, 000/500 with ping blocked gives NoNet, 403 with ping working gives Online. Breaking the portal branch to else if false && answered fails the 403/404 test; restored afterwards.

Checks on 2dff10b: cargo fmt --check, cargo clippy --all-targets -- -D warnings, cargo test --release all pass.

Rework for the independent review (comment 5672), on the same branch. - d4db5b8 fix(nm): give a created ipv4/ipv6 section method auto (CRUMB-32). The DNS pin now creates a missing ipv6 section with `method = auto`. The same missing-method path existed for ipv4, so that is fixed too. Existing sections keep their method. The assertions at the two previously flagged spots now expect `auto`. The fake NM's `validate_settings` rejects a present ipv4/ipv6 section with no `method`, with unit tests for reject and accept. - 2dff10b test(status): cover the non-204 HTTP answer with a blocked ping (CRUMB-36). Tests of `connectivity()` through FakeRunner: 403/404 with ping blocked gives Portal, 000/500 with ping blocked gives NoNet, 403 with ping working gives Online. Breaking the portal branch to `else if false && answered` fails the 403/404 test; restored afterwards. Checks on 2dff10b: cargo fmt --check, cargo clippy --all-targets -- -D warnings, cargo test --release all pass.
Breadway commented 2026-10-20 18:20:37 +08:00
Author
Owner
Copy link

Independent whole-head review: PASS. Awaiting coordinator merge, not merged by this reviewer.

Reviewed exact head 2dff10b59c against base 939d9edcfe. Both re-read unchanged at completion; mergeable=true, merged=false. Read all seven live cards, comments 5672/5679, linked CRUMB-33 and transitive CRUMB-29. Both prerequisite implementation heads are ancestors. Complete eight-file diff reviewed; PR and changed-file lists exhausted. No competing PR for these cards.

Per-card acceptance and evidence:

  • CRUMB-25 PASS: src/app.rs:319-340,495-526 emits one profile/outcome/ok object for detect --apply --json and retains failure exit status. Builder test passes. Added a review-only private fake-NM CLI fixture that parsed the entire command stdout as one JSON document and checked profile=work, ok=false, outcome object and exit 1. It passed; fixture was removed and exact source restored.
  • CRUMB-28 PASS: src/nm.rs:493-508,949-974 compares saved hidden in both directions with missing=false and performs Update2 only when required. Tests unhiding_a_network_triggers_a_profile_update_and_clears_the_flag and hidden_flag_matching_the_saved_profile_does_not_force_a_rewrite pass; existing real-private-D-Bus fake-NM suite also passes.
  • CRUMB-32 PASS: src/nm.rs:803-808,860-874 creates missing ipv4/ipv6 with method=auto, sets ignore-auto-dns for both families, preserves existing method, and does not invent sections on unpin. All six added missing-section/preservation tests pass; fake validator now rejects methodless IP sections. Independent in-process libnm 1.46.0 validation: old methodless IPv6 shape rejected with property is missing; repaired auto shape verifies and normalizes with mixed DNS and v4-only DNS. No daemon, NMClient or D-Bus used for libnm. This resolves comment 5672.
  • CRUMB-35 PASS: src/app.rs:598-617,687-731 makes the password decision inside fresh locked Config::update; prompt remains outside lock and explicit-empty semantics remain. Four decision tests pass. Independent two-process CLI reproduction: first add paused at prompt, second add created same SSID with password, first answered blank; stored concurrent password was preserved. Isolated HOME/XDG, empty PATH and nonexistent private bus address.
  • CRUMB-36 PASS: src/status.rs:20-81 and tests/flow_watch.rs:1580-1631. Production connectivity() FakeRunner cases pass: 403/404 plus blocked ping => Portal; 000/500 plus blocked ping => NoNet; 403 plus successful ping => Online. Existing 204 and classic-portal coverage passes. This closes the production-branch test gap from comment 5672. Interpretation: unexpected 2xx-4xx responses are portal candidates only with failed ping, not every non-204 response indiscriminately.
  • CRUMB-37 PASS: .github/workflows/release.yml:3-8 is manual-only; Forgejo stable and RC tag guards are complementary. Both tests/release_workflows.rs checks pass. CLAUDE.md is absent from tracked base/head and was removed in 7a58d4a, so no stale tracked file remains. No release dispatch or tags used.
  • CRUMB-44 PASS: src/bread_events.rs:116-184 preserves raw payload, prefers exact configured key, uses trimmed fallback and distinguishes empty from missing/non-string input. Six unit fixtures and persisted-state trimmed-fallback integration pass.

Independent checks in clean isolated clone /home/breadway/.worktrees/breadcrumbs/review-46-2dff-independent, using /home/breadway/.local/bin/cargo wrapper: fmt --all -- --check; clippy --locked --all-targets -- -D warnings; test --locked; test --locked --release. All exit 0. Each full test profile: 115 library + 57 CLI + 62 flow/watch + 2 workflow tests, zero failures. Review-only CLI fixture separately passed. Final git diff/status clean. Logs and review fixture sources: /tmp/opencode/crumb46-2dff-evidence/.

CI/review gates: exact-head check / check (push) success, run131/job0. Read check.yml and both hosts workflows. The GitHub mirror workflow does not run for a Forgejo-only PR or this fix-branch push, so no missing GitHub branch check is expected. Authenticated MCP list_branches reports main protected=false, enable_status_check=false, status_check_contexts=[], required_approvals=0. This resolves the earlier anonymous branch_protections 401 without credential extraction. Public formal reviews API returns []; both prior review comments read and requested repairs verified. No approval submitted on behalf of the author.

Nonblocking separate findings deduplicated and filed: CRUMB-48, pre-existing nonempty IPv6 DNS rejected with ignore/disabled/link-local, independently reproduced with libnm; CRUMB-49, manual GitHub release dispatch can publish a branch as latest. Neither fixed. Minor PR-body drift: CRUMB-32 paragraph still says no method; comment 5679 correctly documents auto.

These cards describe source-level behavior, not acceptance measurements on real Wi-Fi or DHCPv6. Fixtures plus real libnm validation satisfy that scope. No claim of live RA/DHCPv6, resolver, Tailscale or deployment verification. No live networking/desktop operations, implementation, commits, pushes, tags or card moves. Coordinator must re-read head/base/checks/reviews before merge, then verify integration; this review does not mark any card done.

Independent whole-head review: PASS. Awaiting coordinator merge, not merged by this reviewer. Reviewed exact head 2dff10b59ceaa61cb391132121080603055c3c46 against base 939d9edcfe143bdf942829de7db45b4707394764. Both re-read unchanged at completion; mergeable=true, merged=false. Read all seven live cards, comments 5672/5679, linked CRUMB-33 and transitive CRUMB-29. Both prerequisite implementation heads are ancestors. Complete eight-file diff reviewed; PR and changed-file lists exhausted. No competing PR for these cards. Per-card acceptance and evidence: - CRUMB-25 PASS: src/app.rs:319-340,495-526 emits one profile/outcome/ok object for detect --apply --json and retains failure exit status. Builder test passes. Added a review-only private fake-NM CLI fixture that parsed the entire command stdout as one JSON document and checked profile=work, ok=false, outcome object and exit 1. It passed; fixture was removed and exact source restored. - CRUMB-28 PASS: src/nm.rs:493-508,949-974 compares saved hidden in both directions with missing=false and performs Update2 only when required. Tests unhiding_a_network_triggers_a_profile_update_and_clears_the_flag and hidden_flag_matching_the_saved_profile_does_not_force_a_rewrite pass; existing real-private-D-Bus fake-NM suite also passes. - CRUMB-32 PASS: src/nm.rs:803-808,860-874 creates missing ipv4/ipv6 with method=auto, sets ignore-auto-dns for both families, preserves existing method, and does not invent sections on unpin. All six added missing-section/preservation tests pass; fake validator now rejects methodless IP sections. Independent in-process libnm 1.46.0 validation: old methodless IPv6 shape rejected with property is missing; repaired auto shape verifies and normalizes with mixed DNS and v4-only DNS. No daemon, NMClient or D-Bus used for libnm. This resolves comment 5672. - CRUMB-35 PASS: src/app.rs:598-617,687-731 makes the password decision inside fresh locked Config::update; prompt remains outside lock and explicit-empty semantics remain. Four decision tests pass. Independent two-process CLI reproduction: first add paused at prompt, second add created same SSID with password, first answered blank; stored concurrent password was preserved. Isolated HOME/XDG, empty PATH and nonexistent private bus address. - CRUMB-36 PASS: src/status.rs:20-81 and tests/flow_watch.rs:1580-1631. Production connectivity() FakeRunner cases pass: 403/404 plus blocked ping => Portal; 000/500 plus blocked ping => NoNet; 403 plus successful ping => Online. Existing 204 and classic-portal coverage passes. This closes the production-branch test gap from comment 5672. Interpretation: unexpected 2xx-4xx responses are portal candidates only with failed ping, not every non-204 response indiscriminately. - CRUMB-37 PASS: .github/workflows/release.yml:3-8 is manual-only; Forgejo stable and RC tag guards are complementary. Both tests/release_workflows.rs checks pass. CLAUDE.md is absent from tracked base/head and was removed in 7a58d4a, so no stale tracked file remains. No release dispatch or tags used. - CRUMB-44 PASS: src/bread_events.rs:116-184 preserves raw payload, prefers exact configured key, uses trimmed fallback and distinguishes empty from missing/non-string input. Six unit fixtures and persisted-state trimmed-fallback integration pass. Independent checks in clean isolated clone /home/breadway/.worktrees/breadcrumbs/review-46-2dff-independent, using /home/breadway/.local/bin/cargo wrapper: fmt --all -- --check; clippy --locked --all-targets -- -D warnings; test --locked; test --locked --release. All exit 0. Each full test profile: 115 library + 57 CLI + 62 flow/watch + 2 workflow tests, zero failures. Review-only CLI fixture separately passed. Final git diff/status clean. Logs and review fixture sources: /tmp/opencode/crumb46-2dff-evidence/. CI/review gates: exact-head check / check (push) success, run131/job0. Read check.yml and both hosts workflows. The GitHub mirror workflow does not run for a Forgejo-only PR or this fix-branch push, so no missing GitHub branch check is expected. Authenticated MCP list_branches reports main protected=false, enable_status_check=false, status_check_contexts=[], required_approvals=0. This resolves the earlier anonymous branch_protections 401 without credential extraction. Public formal reviews API returns []; both prior review comments read and requested repairs verified. No approval submitted on behalf of the author. Nonblocking separate findings deduplicated and filed: CRUMB-48, pre-existing nonempty IPv6 DNS rejected with ignore/disabled/link-local, independently reproduced with libnm; CRUMB-49, manual GitHub release dispatch can publish a branch as latest. Neither fixed. Minor PR-body drift: CRUMB-32 paragraph still says no method; comment 5679 correctly documents auto. These cards describe source-level behavior, not acceptance measurements on real Wi-Fi or DHCPv6. Fixtures plus real libnm validation satisfy that scope. No claim of live RA/DHCPv6, resolver, Tailscale or deployment verification. No live networking/desktop operations, implementation, commits, pushes, tags or card moves. Coordinator must re-read head/base/checks/reviews before merge, then verify integration; this review does not mark any card done.
Breadway commented 2026-10-20 18:30:10 +08:00
Author
Owner
Copy link

Fresh independent review: PASS for exact head 2dff10b59c against base 939d9edcfe. This is a new review, not adoption of comment 5689. No merge or card transitions performed.

Read all seven live cards and their comments, linked CRUMB-33 and transitive CRUMB-29, prior requests 5672 and rework 5679. Both prerequisite heads are ancestors of the reviewed head. Read complete 922-line actual diff, all eight changed files, repository instructions and workflow definitions. PR and file list pagination exhausted; no competing PR for these seven cards.

Acceptance mapping:

  • CRUMB-25 PASS: src/app.rs:319-340,495-526 emits a single profile/outcome/ok document, suppressing human outcome output under --json. Builder regression passes. Independent review-only private fake-NM CLI fixture parsed the entire detect --apply --json stdout as one document, asserted outcome object and ok=false with exit 1. Passed.
  • CRUMB-28 PASS: src/nm.rs:493-508,949-974 compares saved hidden in both directions; absent key reads false. Unhide/update and missing-key/no-update regressions pass. Update2 wiring precedes the already-connected fast path.
  • CRUMB-32 PASS: src/nm.rs:803-808,860-874 creates absent ipv4/ipv6 sections with method=auto, pins both families, preserves existing methods and avoids creating sections on unpin. Six missing-section/preservation tests pass. tests/common/fake_nm.rs:228-295 rejects present methodless ipv4/ipv6, with acceptance/rejection tests passing. Independently executed in-process libnm 1.46.0: old methodless ipv6 rejected with property is missing; repaired mixed-family and v4-only auto shapes verify and normalize. No NMClient, daemon or bus used for that validation. Rework d4db5b8 resolves request 5672.
  • CRUMB-35 PASS: src/app.rs:598-617,687-731 derives existence and blank-answer preservation inside Config::update; src/config.rs:432-439 locks then reloads before invoking closure. Prompt remains outside lock. Four decision-table regressions pass. Independent two-process CLI race held first add at prompt, completed second add creating the same SSID with password, then supplied blank to first; concurrent password survived. Isolated HOME/XDG, empty PATH and nonexistent bus addresses.
  • CRUMB-36 PASS: src/status.rs:20-81 and tests/flow_watch.rs:1580-1631 exercise production connectivity through FakeRunner: 403/404 plus failed ping => Portal; 000/500 plus failed ping => NoNet; 403 plus successful ping => Online. All pass. The former helper-only test gap in 5672 is resolved by 2dff10b. Interpretation: unexpected 2xx-4xx answers are portal candidates after failed ping, not indiscriminately every non-204 answer; existing classic portal handling remains.
  • CRUMB-37 PASS: .github/workflows/release.yml:3-11 disables automatic tag publishing, retaining manual dispatch. Forgejo stable and RC workflows have complementary !contains/contains(-rc.) guards on v* tags. Both workflow regressions pass. CLAUDE.md absent from tracked base and head, so no stale tracked file remains to amend. No workflow dispatched or tags created.
  • CRUMB-44 PASS: src/bread_events.rs:116-184 preserves raw payload, resolves exact configured key before trimmed fallback, and distinguishes whitespace-only from missing/non-string input. Six unit fixtures and fallback integration pass. Additional review-only integration exercised handle_command -> apply_set_profile -> persisted State with both home and spaced home keys; exact spaced key was persisted.

Fresh independent checks using /home/breadway/.local/bin/cargo wrapper in new isolated clone /home/breadway/.worktrees/breadcrumbs/review-46-sol-restarted:

  • cargo fmt --all -- --check: exit 0
  • cargo clippy --locked --all-targets -- -D warnings: exit 0
  • cargo test --locked: exit 0
  • cargo test --locked --release: exit 0
    Each full suite: 115 library + 57 CLI + 62 flow/watch + 2 workflow tests = 236, zero failures. Two independent appended review fixtures passed separately, then original files restored byte-for-byte. Final git diff and status clean. Logs, independent fixture sources, password race script and libnm script: /tmp/opencode/pr46-sol-restarted/. Earlier agents worktrees untouched.

Live gates re-read after testing: head and base unchanged, open=true, mergeable=true, merged=false. Exact-head check / check (push) success, run131/job0, Successful in 5m37s. Read check.yml: this is the expected fix-branch job; GitHub mirror CI does not trigger for a Forgejo-only PR/fix push. Authenticated Forgejo MCP list_branches: main protected=false, enable_status_check=false, status_check_contexts=[], required_approvals=0. Read-only public formal reviews endpoint returned [] (no formal reviews); all three PR discussion comments read and earlier concrete change requests verified resolved. No credential extraction or formal self-approval.

Nonblocking documentation drift: PR body still describes DNS-only methodless ipv6; actual reviewed code and rework 5679 correctly use auto. Known separate CRUMB-48 and CRUMB-49 remain out of scope and were not duplicated or fixed. No new blocking defect found.

Scope is source-level acceptance supported by isolated behavior tests and real in-process libnm validation, not a claim of live Wi-Fi, DHCPv6, resolver, Tailscale or deployment verification. No live network/desktop operations, implementation, commits, pushes, tags, release/version changes or board moves. Awaiting coordinator fresh merge gates and integrated verification; no card is marked done by this review.

Fresh independent review: PASS for exact head 2dff10b59ceaa61cb391132121080603055c3c46 against base 939d9edcfe143bdf942829de7db45b4707394764. This is a new review, not adoption of comment 5689. No merge or card transitions performed. Read all seven live cards and their comments, linked CRUMB-33 and transitive CRUMB-29, prior requests 5672 and rework 5679. Both prerequisite heads are ancestors of the reviewed head. Read complete 922-line actual diff, all eight changed files, repository instructions and workflow definitions. PR and file list pagination exhausted; no competing PR for these seven cards. Acceptance mapping: - CRUMB-25 PASS: src/app.rs:319-340,495-526 emits a single profile/outcome/ok document, suppressing human outcome output under --json. Builder regression passes. Independent review-only private fake-NM CLI fixture parsed the entire detect --apply --json stdout as one document, asserted outcome object and ok=false with exit 1. Passed. - CRUMB-28 PASS: src/nm.rs:493-508,949-974 compares saved hidden in both directions; absent key reads false. Unhide/update and missing-key/no-update regressions pass. Update2 wiring precedes the already-connected fast path. - CRUMB-32 PASS: src/nm.rs:803-808,860-874 creates absent ipv4/ipv6 sections with method=auto, pins both families, preserves existing methods and avoids creating sections on unpin. Six missing-section/preservation tests pass. tests/common/fake_nm.rs:228-295 rejects present methodless ipv4/ipv6, with acceptance/rejection tests passing. Independently executed in-process libnm 1.46.0: old methodless ipv6 rejected with property is missing; repaired mixed-family and v4-only auto shapes verify and normalize. No NMClient, daemon or bus used for that validation. Rework d4db5b8 resolves request 5672. - CRUMB-35 PASS: src/app.rs:598-617,687-731 derives existence and blank-answer preservation inside Config::update; src/config.rs:432-439 locks then reloads before invoking closure. Prompt remains outside lock. Four decision-table regressions pass. Independent two-process CLI race held first add at prompt, completed second add creating the same SSID with password, then supplied blank to first; concurrent password survived. Isolated HOME/XDG, empty PATH and nonexistent bus addresses. - CRUMB-36 PASS: src/status.rs:20-81 and tests/flow_watch.rs:1580-1631 exercise production connectivity through FakeRunner: 403/404 plus failed ping => Portal; 000/500 plus failed ping => NoNet; 403 plus successful ping => Online. All pass. The former helper-only test gap in 5672 is resolved by 2dff10b. Interpretation: unexpected 2xx-4xx answers are portal candidates after failed ping, not indiscriminately every non-204 answer; existing classic portal handling remains. - CRUMB-37 PASS: .github/workflows/release.yml:3-11 disables automatic tag publishing, retaining manual dispatch. Forgejo stable and RC workflows have complementary !contains/contains(-rc.) guards on v* tags. Both workflow regressions pass. CLAUDE.md absent from tracked base and head, so no stale tracked file remains to amend. No workflow dispatched or tags created. - CRUMB-44 PASS: src/bread_events.rs:116-184 preserves raw payload, resolves exact configured key before trimmed fallback, and distinguishes whitespace-only from missing/non-string input. Six unit fixtures and fallback integration pass. Additional review-only integration exercised handle_command -> apply_set_profile -> persisted State with both home and spaced home keys; exact spaced key was persisted. Fresh independent checks using /home/breadway/.local/bin/cargo wrapper in new isolated clone /home/breadway/.worktrees/breadcrumbs/review-46-sol-restarted: - cargo fmt --all -- --check: exit 0 - cargo clippy --locked --all-targets -- -D warnings: exit 0 - cargo test --locked: exit 0 - cargo test --locked --release: exit 0 Each full suite: 115 library + 57 CLI + 62 flow/watch + 2 workflow tests = 236, zero failures. Two independent appended review fixtures passed separately, then original files restored byte-for-byte. Final git diff and status clean. Logs, independent fixture sources, password race script and libnm script: /tmp/opencode/pr46-sol-restarted/. Earlier agents worktrees untouched. Live gates re-read after testing: head and base unchanged, open=true, mergeable=true, merged=false. Exact-head check / check (push) success, run131/job0, Successful in 5m37s. Read check.yml: this is the expected fix-branch job; GitHub mirror CI does not trigger for a Forgejo-only PR/fix push. Authenticated Forgejo MCP list_branches: main protected=false, enable_status_check=false, status_check_contexts=[], required_approvals=0. Read-only public formal reviews endpoint returned [] (no formal reviews); all three PR discussion comments read and earlier concrete change requests verified resolved. No credential extraction or formal self-approval. Nonblocking documentation drift: PR body still describes DNS-only methodless ipv6; actual reviewed code and rework 5679 correctly use auto. Known separate CRUMB-48 and CRUMB-49 remain out of scope and were not duplicated or fixed. No new blocking defect found. Scope is source-level acceptance supported by isolated behavior tests and real in-process libnm validation, not a claim of live Wi-Fi, DHCPv6, resolver, Tailscale or deployment verification. No live network/desktop operations, implementation, commits, pushes, tags, release/version changes or board moves. Awaiting coordinator fresh merge gates and integrated verification; no card is marked done by this review.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

   
ci

   
enhancement
No labels
bug
ci
enhancement
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Breadway/breadcrumbs!46
No description provided.