refactor(stella-protocol,stella-pipeline): derive the call-role family instead of hand-listing it (#1977) - #2035
Merged
Merged
Conversation
…y instead of hand-listing it `ALL_ROLES` in stella-pipeline's management-prompt witness was a hand-maintained `[ModelCallRole; 15]`, and its own doc comment conceded completeness was not compiler-checked — it delegated that to the exhaustive match in `management_system_block`. The delegation only held by luck of ordering: when #1778 added `ModelCallRole::Research`, the match did fail the build, but the array was a separate edit nothing would have caught. An array one variant short makes the #1855 shared-preamble parity witness silently test fewer roles than it claims, and the failure mode is a *passing* test. `stella-protocol` now owns the enumeration of its own variants. A `model_call_roles!` declaration feeds two expansions from one token list: `ModelCallRole::ALL`, and an exhaustive `match` over the enum. A variant added to the enum but not the declaration fails that match with E0004, so the list is provably a superset of the variants; `ALL` is built from the same list, so it is provably total. No variant count exists to fall out of date -- Rust cannot count variants on stable, and a hand-written length is the drift this replaces. Modelled on `agent_event_tags!`, which binds `KNOWN_TYPE_TAGS` to `AgentEvent` the same way in the same crate. `ModelCallRole` moves to `event/call_role.rs` rather than growing `event.rs`, which sat at 1497 of the 1500-line ratchet with no baseline entry; the parent drops to 1446. The public path `stella_protocol::ModelCallRole` is unchanged. Witness: adding a variant to the enum and not the declaration fails with "E0004: non-exhaustive patterns: `ModelCallRole::ScratchProbe` not covered" pointing at `model_call_roles!`. On the old code the same edit compiled clean once the two pipeline matches were updated, leaving ALL_ROLES at 15. Closes #1977
Moving the enum rewrote its doc comment, and that comment is the `description` field of docs/wire/agentevent.schema.json and its TypeScript twin — so the rustdoc-internal `(super)` / `(super::AgentEvent)` link syntax I introduced shipped straight into an artifact whose consumers have no Rust. The wire-schema guard caught it. Restores the published prose byte-for-byte, so the refactor now produces a zero wire diff — which is the proof it is shape-preserving. The maintainer note about the E0004 tripwire moves to a non-doc comment and to `model_call_roles!`, neither of which reaches the wire. The `AgentEvent` intra-doc link resolves from the child module via a `cfg(doc)` import, invisible to a normal build.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
Reviewer's GuideRefactors ModelCallRole so its "all roles" enumeration is derived in stella-protocol via a macro, moves the enum into its own module, exposes a compile-time exhaustiveness guard, and updates stella-pipeline tests to consume the new ModelCallRole::ALL instead of a hand-maintained array while preserving wire schema shape. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
ALL_ROLESincrates/stella-pipeline/src/management_prompt/tests.rswas ahand-maintained
[ModelCallRole; 15], and its own doc comment conceded the gap:That delegation held, but only by luck of ordering. When #1778 added
ModelCallRole::Research, the exhaustive match did fail the build — but thearray was a separate edit that nothing would have caught had the match not
happened to live in the same file. An array one variant short makes the #1855
shared-preamble parity witness silently test fewer roles than it claims to, and
the failure mode is a passing test, not a failing one.
stella-protocolnow owns the enumeration of its own variants.How it is provably total
There is no variant count anywhere — Rust cannot count enum variants on stable,
and a hand-written length is exactly the drift this replaces. Instead one
model_call_roles!token list feeds two expansions:ModelCallRole::ALL, built from the list, andmatchoverModelCallRole, checked against the list.A variant added to the enum but not the declaration fails that match with
E0004, so the list is provably a superset of the variants;ALLis built fromthe same list, so it is provably total. Neither half can drift from the other
because there is only one list.
Modelled on
agent_event_tags!, which bindsKNOWN_TYPE_TAGStoAgentEventthe same way, in the same crate — this is that idiom applied to thesibling enum, not a new pattern.
Why the enum moved file
event.rswas at 1497 of the 1500-line ratchet with no baseline entry, sothere was nowhere for the declaration to land.
ModelCallRolemoves toevent/call_role.rs;event.rsdrops to 1446. The public pathstella_protocol::ModelCallRoleis unchanged.Zero wire diff
The doc comment on this enum is the
descriptionfield ofdocs/wire/agentevent.schema.jsonand its TypeScript twin. My first passrewrote it and leaked rustdoc-internal
(super)/(super::AgentEvent)linksyntax into an artifact whose consumers have no Rust — the
wire-schemaguardcaught it (second commit). The published prose is now restored byte-for-byte,
so this refactor produces no wire diff at all, which is the proof it is
shape-preserving. Maintainer notes live in a non-doc comment and on the macro,
neither of which reaches the wire; the
AgentEventintra-doc link resolves fromthe child module via a
cfg(doc)import that a normal build never sees.Closes #1977
The witness
main, passes here), orTwo runtime tests ship in
event/call_role.rs(ALLhas no repeats and is indeclaration order; every role in
ALLround-trips through serde). But thereal witness for this issue is a compile-time one, so it cannot live in the
test tree — verified by hand:
On
mainthe same edit compiles clean once the twostella-pipelinematchesare updated, leaving
ALL_ROLESat 15 and the parity witness quietly coveringone role fewer. That difference is the whole issue.
The gate
cargo fmt --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspacemake wire-schema— clean, zero diffRUSTDOCFLAGS="-D warnings" cargo doc— cleanmake guards-fast— all green (file-size, god-files, gate-parity, …)Closes #1977appears both here and as a commit trailerNothing left behind
const ALLarrays can still drift from their enums #2031crates/stella-pipeline/src/pipeline/raw_usage.rs'smanagement_boundsisnamed in the issue as the same pairing, but it carries no array — only an
exhaustive match — so it is already safe and needed no change.
The same shape does exist in ~12 other places in the workspace (a hand-written
pub const ALL: [X; N]beside an exhaustive match that cannot force the array togrow):
stella-tuideck.rs/envelope.rs/views/engine.rs/theme.rs/deck_ui.rs,stella-diaglevel.rs,stella-graphlang.rs,stella-serveobserve/event.rs,stella-coreextensions.rs,stella-clisettings.rs. Filed as #2031rather than swept into this PR, which is deliberately one enum — that issue also
raises whether twelve near-identical macros should instead be one shared
enum_all!in a leaf crate, which is a design call, not a mechanical sweep.Ground-rule check
stella-core; no new deps (option 2 in the issue, astrumderive, was rejected —stella-protocolis types-only anddependency-light, and option 1 needs nothing)
Anything reviewers should know?
The tripwire function
every_variant_is_in_alldeliberately has an empty bodyand is never called at runtime — its exhaustive
matchis the assertion, andthe
const _: () = …item below it forces evaluation so it cannot rot into deadcode. It looks strange on purpose; the doc comment says why.
Summary by Sourcery
Move the ModelCallRole enum into its own module and introduce a macro-driven enumeration to derive the complete role family, ensuring consumers iterate a compiler-checked ALL list rather than a hand-maintained array.
Enhancements:
Tests: