Parse wire arrays positionally and reject narrowing or invalid channel values

[mkovero]

Source: from-claude-to-codex.md, verified at a8b80db9. Includes carried-over AUDIT-003.

Independent filter_map parsing can pair a frequency with the wrong gain when invalid mic-curve elements occur at different indices. Invalid generate channels can collapse to an empty list and fall back to the configured output, emitting on a channel the request did not name. Monitor has analogous fallback/mis-measurement behavior. Several wire values are narrowed with unchecked u64 as u32 casts in generate, monitor, transfer pairs, and setup.

Required outcome: arrays are validated positionally and rejected as a whole on any invalid element; an explicitly supplied invalid channel list never falls back to defaults or emits audio. Every wire-to-u32 conversion is checked and rejects overflow. Cover mismatched invalid indices, invalid-only channel lists, mixed valid/invalid lists, and values above u32::MAX.

Parent epic: #423

[mkovero]

spec

type: bug

problem statement
Lenient element filtering can shift positional mic-curve pairs, turn explicitly invalid channel lists into default routing, and narrow oversized wire integers into different channel values. A malformed request can therefore persist wrong calibration data, measure the wrong input, or emit on an output that was not requested.

acceptance criteria

• Positional arrays are rejected as a whole when any element is invalid or corresponding lengths differ
• An explicitly supplied invalid channel list never falls back to configured defaults
• Invalid generate requests emit no audio
• Every wire channel conversion rejects values greater than 4294967295 — provenance: derived
• Regressions cover invalid elements at different indices, invalid-only and mixed channel lists, and the u32 overflow boundary

out of scope

• Changing fallback behavior when the channel field is omitted
• Changing valid channel numbering

files likely affected

• ac-rs/ac-daemon/src/handlers/calibrate/mic_curve.rs — positional calibration arrays
• ac-rs/ac-daemon/src/handlers/generate.rs — output channels
• ac-rs/ac-daemon/src/handlers/monitor/mod.rs — monitor inputs
• ac-rs/ac-daemon/src/handlers/transfer/request.rs — transfer pairs
• ac-rs/ac-daemon/src/handlers/admin.rs — setup channel fields

needs architect review
no

estimated complexity
medium: 2–8h

[mkovero]

what this output must communicate

1. The request was rejected as a whole and no fallback routing or stimulus occurred.
2. The exact field, index, offending value, and required type/range.

what to remove

• Silent element dropping: it destroys positional meaning.
• Default-channel wording after an explicitly invalid channel list: fallback did not occur.
• Generic invalid request: it withholds the repairable location.

proposed output

Mic-curve pair:

  error: mic curve not saved — gain_db[0] must be a finite number
         paired field  freqs_hz[0] = 20.000 Hz
         data          existing curve unchanged

Generate channel:

  error: generate not started — channels[0] must be an integer
         received  "bad"
         stimulus  silent

Overflow:

  error: setup rejected — output_channel is outside 0–4294967295
         received  4294967296
         config    unchanged

field justifications

• command not started/not saved/rejected: states whole-request atomicity.
• indexed field: preserves positional context.
• paired field: shows why independent filtering is unsafe.
• received: records the offending wire value.
• required type/range: gives a direct correction.
• stimulus silent: confirms drive safety.
• existing curve/config unchanged: confirms persistence atomicity.

before / after

before:

  generate accepted

after:

  error: generate not started — channels[0] must be an integer
         received  "bad"
         stimulus  silent

removed: fallback success after explicit malformed input.
added: field index, received value, expected type/range, and state outcome.
changed: arrays fail atomically instead of being filtered.

open questions

none.

[mkovero]

design decision

Checked against origin/main @ 9869d495. The triage file paths are out of date. The crates are under ac-rs/crates/, and generate.rs and monitor/ are under handlers/audio/. The manifest below uses the real paths. Two findings are outside triage's file list and change scope:

• The CLI has the same defect. ac-cli parse/generate.rs stores the channel token as a raw string. commands/generate.rs::resolve_channels then calls parse_channels(spec).unwrap_or_else(|_| vec![0]). is_channel_spec accepts 4294967296, 3- and 1,,2, and parse_channels rejects all three, so ac generate sine 4294967296 plays on channel 0. 5-3 parses to an empty list, sends channels: [], and plays on the configured output. A daemon-only fix cannot meet criteria 2–4 for these, because the daemon receives a valid [0] or [].
• More daemon narrowing sites. Beyond the sites triage named, the daemon also narrows in calibrate/mod.rs::channels_from, used by calibrate, calibrate_spl and calibrate_mic_curve. It does the same in admin.rs::get_calibration, in fft_n in both monitor_spectrum and set_monitor_params, and in set_ioct_bpo. Two of these accept a wrong value today:
  • fft_n: 4294967552 wraps to 256 and passes the power-of-two check.
  • bpo: 4294967299 wraps to 3 and is accepted.

core question
Where does the rule "a supplied wire value is either exactly valid or refuses the whole request" live? And what is the one meaning of absent, null, empty and invalid for channel fields?

option A — one daemon-local wire-value module
Add ac-daemon/src/handlers/wire.rs. It holds pure functions over &Value and has no ServerState, following the style of transfer/request.rs. It exposes:

• An optional u32 field reader. Absent gives none. A present non-integer, negative, or > 4294967295 value is an error.
• A nullable variant for reference_channel and reference_output_channel: absent means keep, null means clear, a u32 means set, and anything else is an error.
• An optional u32 array reader. If it is present and not an array, that is an error. Any bad element is an error that names field[i].
• An optional finite-f32 array reader. A value that is finite as f64 but overflows f32 is an error at that index.

These return a small WireError holding the field path, what was wrong, and the value received. They also provide one renderer that produces ux's refusal layout. Every handler in the manifest takes its values from this module. ac-cli gets the parse-time fix separately (see notes). The CLI is a different process with a different error channel, so it does not share this module.
tradeoffs: this gives one definition of "invalid", one error format, and one set of boundary tests. The cost is a new module plus a signature change to channels_from, which touches three handlers.

option B — fix each site in place
Replace each as u32 with u32::try_from and each filter_map with an indexed loop, inline, with the error text built at each site.
tradeoffs: smaller diff per file, no new module. But about 12 copies of "must be an integer / is outside 0–4294967295 / received …" would have to agree with each other and with ux's layout, and nothing would enforce that. The next handler that reads a channel copies whichever version it finds nearest. This is the coupled-copies pattern (#206 fixed port resolution once in handlers/mod.rs for the same reason).

recommendation
Option A. handlers/mod.rs already owns channel→port resolution as a single refuse-don't-fabricate path (#206). Wire→channel parsing is the step just before that, and it has the same failure mode. It belongs in one place next to it, not in twelve.

affected modules

• ac-daemon handlers/wire.rs (new) — value readers, WireError, the refusal renderer, and unit tests for the boundary.
• ac-daemon handlers/mod.rs — mod wire; only.
• ac-daemon handlers/audio/generate.rs — resolve_channels uses the array reader. Serves both generate and generate_pink.
• ac-daemon handlers/audio/monitor/mod.rs — channels, fft_n, and fake_tones, which becomes whole-array rejection.
• ac-daemon handlers/calibrate/mod.rs — channels_from becomes fallible; calibrate uses it.
• ac-daemon handlers/calibrate/spl.rs — follows the channels_from signature change.
• ac-daemon handlers/calibrate/mic_curve.rs — positional freqs_hz / gain_db, and channels_from.
• ac-daemon handlers/transfer/request.rs — range check for pairs[i][j], meas_channel and ref_channel.
• ac-daemon handlers/admin.rs — setup channel fields, get_calibration, set_monitor_params fft_n, set_ioct_bpo.
• ac-cli parse/mod.rs, parse/generate.rs, commands/generate.rs — parse the channel spec at parse time and refuse a bad or empty spec.
• ac-rs/ZMQ.md — wire-value rule and per-command pointers.

file manifest

ac-rs/crates/ac-daemon/src/handlers/wire.rs
ac-rs/crates/ac-daemon/src/handlers/mod.rs
ac-rs/crates/ac-daemon/src/handlers/audio/generate.rs
ac-rs/crates/ac-daemon/src/handlers/audio/monitor/mod.rs
ac-rs/crates/ac-daemon/src/handlers/calibrate/mod.rs
ac-rs/crates/ac-daemon/src/handlers/calibrate/spl.rs
ac-rs/crates/ac-daemon/src/handlers/calibrate/mic_curve.rs
ac-rs/crates/ac-daemon/src/handlers/transfer/request.rs
ac-rs/crates/ac-daemon/src/handlers/transfer/request/tests.rs
ac-rs/crates/ac-daemon/src/handlers/admin.rs
ac-rs/crates/ac-daemon/tests/it_protocol/wire_values.rs
ac-rs/crates/ac-daemon/tests/it_protocol/main.rs
ac-rs/crates/ac-cli/src/parse/mod.rs
ac-rs/crates/ac-cli/src/parse/generate.rs
ac-rs/crates/ac-cli/src/commands/generate.rs
ac-rs/ZMQ.md

Deliberately not in scope:

• handlers/audio/plot.rs bpo: it goes u64→usize, which is lossless, and as u32 only affects the echoed payload. The real defect there is that bpo has no range check at all. That needs its own issue, and a checked conversion alone would not fix it.
• gpio.rs:396 reads back the daemon's own u32 config, so it cannot exceed the range.
• probe does not parse wire channels.

interface changes

• Semantics for channel fields, applied to every field in the manifest:

  wire value	meaning
  field absent	configured default (unchanged; triage out-of-scope)
  channels: null or []	configured default (unchanged — ac-cli plot.rs::run_tui_fallback sends channels: [] to mean "default")
  reference_channel / reference_output_channel: null	clear (unchanged)
  integer in 0–4294967295	that channel
  anything else present — string, float, negative, > 4294967295, null on a non-nullable scalar, non-array channels, any bad array element	whole request refused; no default, no worker, no write, no config change

  Assumption: criterion 2 names channel lists. I apply the same rule to scalar channel fields such as output_channel and input_channel in calibrate and setup. Today {"cmd":"calibrate_mic_curve","input_channel":"x"} silently keys the curve under the configured channel. That is the same fallback defect, and triage's out-of-scope line only protects the omitted case.

• fft_n and bpo: a value above u32 range is refused with the existing documented domain error: fft_n must be power of 2 in [256, 131072], or invalid bpo …. Those domains are narrower than u32, so a new message would add nothing.

• Daemon error strings follow ux's template: "<headline> — <field> <problem>\n <label> <value>…". It has a 9-space continuation indent, the same one Make setup persistence atomic and return failure when config cannot be saved #430 uses, so check_ack prints ux's block exactly. <problem> is must be an integer, is outside 0–4294967295, must be a finite number, or must be an array. The first trailer line is always received <JSON text of the value>. Headlines and state lines:

  command	headline	state line
  generate, generate_pink	<cmd> not started	stimulus silent
  transfer_stream	transfer_stream not started	stimulus silent
  calibrate	calibration not started	stimulus silent
  calibrate_spl	SPL calibration not started	—
  calibrate_mic_curve	mic curve not saved / mic curve not cleared	data existing curve unchanged
  monitor_spectrum	monitor not started	—
  setup	setup rejected	config unchanged
  get_calibration	calibration lookup rejected	—

  ux specified generate, setup and the mic curve. The other rows are that same template applied to the other commands, not new output design. calibrate_mic_curve also adds paired field <other>[i] = <value> when the partner element at that index is valid, as in ux's example.

• ac-cli: CommandKind::GenerateSine / GeneratePink change channels: Option<String> to Option<Vec<u32>>. parse_channels now also refuses a spec that expands to no channels, such as 5-3. No flag changes. Cargo features: none.

ZMQ protocol impact
Yes. This changes behaviour on CTRL replies only. Requests that were accepted before (malformed values that used to be filtered, defaulted or wrapped) now get {"ok": false, "error": …}. No success reply gains or loses a key, and PUB/DATA frames do not change.

• ac-cli: always sends integer u32 channels, [], or omits the field. Every one of those keeps its current meaning, so a correct CLI sees no change. Refusals are printed by the existing check_ack.
  • Mixed versions: an old CLI with a new daemon still plays ac generate sine 4294967296 on channel 0. The old CLI rewrites the value to [0] before sending, and the daemon cannot see that. Only the CLI half of this change closes that case.
• ac-view: sends none of these fields; grep finds no channels / fft_n / bpo / pairs in ac-view or ac-scene. Unaffected.
• gpio.rs: sends setup {}. Unaffected.
• it_zmq_doc_parity.rs: checks the command roster and read-only reply keys. No keys change.

implementation notes for developer

wire.rs:

• Keep it pure. The unit tests there are the boundary proof: 4294967295 accepted, 4294967296 refused (inherited criterion, provenance: derived), plus -1, 1.5, "3", absent, null, [], ["bad"], [2,"bad"], and 1e39 in the f32 reader.
• Render received with Value::to_string(), which quotes strings the way ux shows. Truncate it to 64 characters with … so a hostile array cannot echo megabytes into the reply — provenance: assumed.

generate.rs:

• resolve_channels returns the rendered refusal. [] / null / absent all go to resolve_output.
• The refusal must happen before make_engine_for_state and spawn_worker. It already does, because resolve_channels runs first; keep that order.

monitor/mod.rs:

• The same rule applies to channels: remove .filter(|v| !v.is_empty()) only for the case where an element was invalid. Empty still means default.
• fft_n: use a checked conversion, then the existing range check.
• fake_tones: refuse the whole request when any element is missing freq_hz or level_dbfs. Name it fake_tones[i].freq_hz. This is in scope because a silently dropped tone makes the display-truth harness test fewer tones than it states.

calibrate/mod.rs:

• channels_from returns a Result. Callers are calibrate (in mod.rs), spl.rs and mic_curve.rs.
• In mic_curve.rs it must run before Calibration::load_or_new.

mic_curve.rs set validation order:

1. Both fields present and arrays.
2. Equal lengths, which keeps missing/mismatched behaviour but should name both lengths.
3. Elements by ascending index, checking freqs_hz[i] before gain_db[i], using the finite-f32 reader.
4. The existing count bounds and monotonicity checks, unchanged.

Doing lengths before elements guarantees the paired-field line always has a partner.

transfer/request.rs:

• Keep the Result<_, String> signature so parse_params stays as it is. Build the message with wire.rs.
• The existing pairs[{i}][0] field paths stay.

admin.rs::setup:

• Parse all four channel fields into locals before taking the lock or touching cfg. Refuse before any mutation, which also gives criterion "config unchanged" for {input_channel: 2, output_channel: 4294967296}.
• This placement works whether Make setup persistence atomic and return failure when config cannot be saved #430 lands before or after this issue. Make setup persistence atomic and return failure when config cannot be saved #430 turns the body into a clone-then-commit, and a parse block ahead of it rebases cleanly.
• get_calibration: same readers.
• set_ioct_bpo: checked conversion, then the existing set check.

ac-cli:

• Copy parse/monitor.rs:36, which already does Some(parse_channels(&args.remove(0))?). parse/generate.rs does the same for sine and pink.
• commands/generate.rs::resolve_channels uses the parsed Vec and deletes unwrap_or_else(|_| vec![0]).
• parse_channels refuses an empty expansion, e.g. bad channel range: "5-3".
• Update the existing Some("0-11".into()) assertion in parse/generate.rs tests to the new type.

Tests. Each must fail on today's code, and the "fails today because" line is what to check. Put the integration tests in a new it_protocol/wire_values.rs (register it in main.rs) so they do not conflict with #430's setup.rs edits. Use out_of_range.rs as the model, including its status.busy == false check.

1. generate channels: ["bad"] → ok:false, error contains channels[0] and stimulus silent, busy false. Fails today because filtered to [] → default output → ok:true.
2. generate channels: [2, "bad"] → ok:false, busy false. Fails today because it plays on 2.
3. generate channels: [4294967296] → ok:false. Fails today because the value wraps to 0, which exists on the fake backend, so the tone starts.
4. generate channels: [] → ok:true, then stop. Guards the CLI contract. This test passes today, and that is intended.
5. monitor_spectrum channels set to ["bad"], [1,"bad"] and [4294967297] → all ok:false. Fails today because these become default, [1] and [1] respectively.
6. monitor_spectrum fft_n: 4294967552 → ok:false with the fft_n message. set_monitor_params fft_n: 4294967552 with no monitor running → the error is the fft_n message, not no active monitor. Fails today because the value wraps to 256 and passes.
7. Mic curve at different indices. Set a valid 32-point curve on input_channel: 1. Then send set with freqs_hz[3] = null and gain_db[7] = "x", keeping lengths equal → ok:false, error names freqs_hz[3], and get_calibration returns the original 32-point curve byte-for-byte. Fails today because both filter_maps drop one element, 31 == 31 passes the length check, the frequencies are still increasing, and a shifted curve is saved ok:true. This is the case the old length check structurally cannot catch, so assert the stored curve, not just ok.
8. calibrate_mic_curve input_channel: 4294967297 → ok:false, and no entry for in1 is created. Fails today because the value wraps to 1.
9. setup {output_channel: 4294967296} → ok:false, and a following setup {} shows the seeded value. setup {input_channel: 2, output_channel: 4294967296} → input_channel also unchanged. setup {reference_channel: "x"} → ok:false. Fails today because these wrap to 0, apply the input change, and ignore the bad value, respectively.
10. set_ioct_bpo bpo: 4294967299 → ok:false. Fails today because the value wraps to 3 and is accepted.
11. request/tests.rs: pairs: [[4294967296, 3]] is an error naming pairs[0][0]. Legacy meas_channel: 4294967296 is an error. Fails today because both return (0, 3).
12. ac-cli parse unit tests: parse_channels refuses "4294967296", "5-3", "1,,2" and "3-". ac generate sine 4294967296 → parse Err. Fails today because parse_generate stores the raw string and returns Ok.

ZMQ.md:

• Add a Wire values subsection under Error handling covering the semantics table, whole-request refusal, and the error layout.
• Add a one-line pointer to it in the request blocks of generate, monitor_spectrum, setup, calibrate_mic_curve and transfer_stream.
• In the calibrate_mic_curve Validation paragraph, add that any invalid element rejects the whole upload.

scope
scope-none — matches triage.

risks

• Merge conflicts. Open PR fix: cancel and bound plot_ir work #437 touches admin.rs + ZMQ.md, open PR fix: gate τ on the peak's own pre-impulse SNR, not captured level #384 touches calibrate/mod.rs + ZMQ.md, and Make setup persistence atomic and return failure when config cannot be saved #430, which is ready-to-implement with no PR yet, rewrites admin.rs::setup. Mitigation: the new code sits in wire.rs, the integration tests are in a new file, and the setup parse block goes ahead of the mutation. Whichever PR lands second gets a mechanical rebase.
• What this does not fix: setup partial mutation on a non-channel error. {output_channel: 5, backend: "bogus"} still changes output_channel in memory before backend refuses. Mitigation: Make setup persistence atomic and return failure when config cannot be saved #430's clone-then-commit fixes that; do not duplicate it here.
• Not fixed: other silently ignored setup fields. dbu_ref_vrms, server_idle_timeout_secs, snapshot_ring_s and temperature_c are still skipped when present with the wrong type. They are not channels or u32. Mitigation: this is the same silent-config defect class; file it separately.
• Not fixed: CLI default when the daemon reply lacks config. commands/generate.rs::resolve_channels still uses .unwrap_or(0) when setup {} returns no config. That is the omitted path, which triage puts out of scope. Mitigation: file it separately.
• Not fixed: huge CLI ranges. ac generate sine 0-4294967295 is valid and expands to 4 billion channels in a BTreeSet. Mitigation: out of scope; file it separately, together with the missing bpo range check in plot.rs.
• Scalar-field extension beyond criterion 2's wording. Mitigation: stated as an assumption above. It only refuses values ac-cli never sends, so no working client can break.