Summary
Phase B warning classes are produced in one place and consumed in three, and every consumer keeps its own hardcoded list of the class names. Nothing derives from the producer, so a class added to the daemon and forgotten in a consumer is silently dropped, and the test meant to catch that is itself a third hardcoded list.
Producer, crates/travsr-daemon/src/lib.rs (write_phase_b_results), writes ten classes as format!("{class}:{lang}"):
crashed version_mismatch needs_approval needs_consent no_references
zero_nodes skipped_unregistered skipped_no_analyzer skipped_no_compdb
untrusted_corpus
Consumers:
crates/travsr-cli/src/status.rs, handles all ten.
crates/travsr-mcp/src/observability.rs (decode_phase_b_warnings), a hand-written match per class.
- the pinned-name list in
observability.rs's test module, which asserts the two agree.
Why it matters
An unhandled class does not merely lose its wording. It falls through to the availability ladder and can surface as a terminal done, so the user is told the language succeeded. The test's own doc comment describes this as the failure it exists to prevent, and cites untrusted_corpus as a case it was missed on.
The guard cannot actually prevent it. A class missing from both decode_phase_b_warnings and the pinned list is invisible to the test, because both lists are hand-maintained and there is nothing to disagree with. That is not hypothetical: zero_nodes and needs_consent sat in exactly that hole and were only found by reading the producer against the consumers during the #752 review. They were fixed in cc27300; the hole that let them sit there is unchanged.
Suggested fix
Derive the expected set from the producer rather than restating it. The write sites in write_phase_b_results are uniform format!("{class}:{lang}") calls, so a single exported const slice of class names (or a small enum with a tag(), matching LangStatus::tag() in phase_b/status.rs) can be the one source, with the daemon formatting from it and the test iterating it to assert every class is handled.
This is the same fix that was applied to is_native_phase_b in #752: that test compared the function against two arrays restating the names the function matched on, so it asserted the function agreed with itself. It was replaced with a table over the real decision. The warning-class guard is the same shape and wants the same treatment. Done once, it would have caught zero_nodes and needs_consent when they were introduced, and catches the next one for free.
Provenance
Raised in the #752 review and deferred there by agreement; the author's cc27300 commit message says so explicitly:
The deeper point stands and is not fixed here: this is a hardcoded list that has to agree with another hardcoded list [...] Deriving it from the format!("{class}:...") sites in write_phase_b_results would have caught these two years ago and would catch the next for free. That is a follow-up, as the review suggests.
Filing so the deferral is tracked. #752 is unaffected: it closed the two live gaps, and this is the structural change that keeps them closed.
Summary
Phase B warning classes are produced in one place and consumed in three, and every consumer keeps its own hardcoded list of the class names. Nothing derives from the producer, so a class added to the daemon and forgotten in a consumer is silently dropped, and the test meant to catch that is itself a third hardcoded list.
Producer,
crates/travsr-daemon/src/lib.rs(write_phase_b_results), writes ten classes asformat!("{class}:{lang}"):Consumers:
crates/travsr-cli/src/status.rs, handles all ten.crates/travsr-mcp/src/observability.rs(decode_phase_b_warnings), a hand-writtenmatchper class.observability.rs's test module, which asserts the two agree.Why it matters
An unhandled class does not merely lose its wording. It falls through to the availability ladder and can surface as a terminal
done, so the user is told the language succeeded. The test's own doc comment describes this as the failure it exists to prevent, and citesuntrusted_corpusas a case it was missed on.The guard cannot actually prevent it. A class missing from both
decode_phase_b_warningsand the pinned list is invisible to the test, because both lists are hand-maintained and there is nothing to disagree with. That is not hypothetical:zero_nodesandneeds_consentsat in exactly that hole and were only found by reading the producer against the consumers during the #752 review. They were fixed in cc27300; the hole that let them sit there is unchanged.Suggested fix
Derive the expected set from the producer rather than restating it. The write sites in
write_phase_b_resultsare uniformformat!("{class}:{lang}")calls, so a single exportedconstslice of class names (or a small enum with atag(), matchingLangStatus::tag()inphase_b/status.rs) can be the one source, with the daemon formatting from it and the test iterating it to assert every class is handled.This is the same fix that was applied to
is_native_phase_bin #752: that test compared the function against two arrays restating the names the function matched on, so it asserted the function agreed with itself. It was replaced with a table over the real decision. The warning-class guard is the same shape and wants the same treatment. Done once, it would have caughtzero_nodesandneeds_consentwhen they were introduced, and catches the next one for free.Provenance
Raised in the #752 review and deferred there by agreement; the author's cc27300 commit message says so explicitly:
Filing so the deferral is tracked. #752 is unaffected: it closed the two live gaps, and this is the structural change that keeps them closed.