Skip to content

[travsr-daemon][travsr-mcp] Phase B warning classes are hardcoded in every consumer, so a missing class is silently dropped and the guard cannot catch it #760

Description

@raj-rkv

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingrepo:travsrOriginates from the travsr repo

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions