Skip to content

build: bump logos-module-builder past 0.2.6 and take the typed records - #65

Open
dlipicar wants to merge 2 commits into
masterfrom
feat/bump-builder-typed-records
Open

build: bump logos-module-builder past 0.2.6 and take the typed records#65
dlipicar wants to merge 2 commits into
masterfrom
feat/bump-builder-typed-records

Conversation

@dlipicar

@dlipicar dlipicar commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

0.2.6's logos-rust-sdk generated the provider trait with serde_json::Value wherever chat_module.lidl declares a record. The newer SDK generates the records themselves, so the trait now reads the way the contract does:

fn init(&mut self, config: ChatConfig) -> Result<Value, String>
fn list_conversations(&mut self) -> Vec<Conversation>
fn get_messages(&mut self, convo_id: String) -> Vec<Message>
fn list_group_members(&mut self, convo_id: String) -> Vec<GroupMember>
fn status(&mut self) -> Status

Nothing here was broken before this. chat_module builds green on 0.2.6. The bump is what the Windows target needs — mkLogosModule only grew an x86_64-windows target after 0.2.6 (#194) — and this migration is its cost. Windows support itself is a separate PR on top of this one.

What the migration deletes

Most of the diff is removal, and it is the interesting part:

  • The hand-written #[derive(Serialize)] mirrors — ConversationSummary, StatusView, GroupMemberRow — existed only to be serialised into the shape the .lidl declares. The generated records are that shape, so the getters build them directly. The mirrors are gone, and with them the chance of one drifting from the contract. (GroupMemberRow gaining pending in feat: add display_name field for module #33 was exactly that kind of two-sided edit.)
  • chat_config/config_field decoded init's config out of an untyped Value, including the case where logoscore call could only send the record's JSON text — "a record parameter reaches a module as a string or not at all". A typed parameter makes decoding the SDK's job, so both helpers go.

About the tests

The four tests over that decoding covered deleted code and went with it. But they also happened to pin down one thing that is still this module's own — the logos.test default, introduced deliberately in a05d511. Rather than let that coverage evaporate, the defaulting is now delivery_preset() with three tests of its own (named / absent / empty).

Mechanical notes

persistence::DisplayMessage stays the stored shape and is mapped to the Message record at the boundary. Message counts and millisecond timestamps widen to the contract's signed 64-bit numbers.

Cargo.lock moves the logos-rust-sdk path dependency 0.2.0 → 0.3.0 — a one-line change, but a required one, because CI runs cargo test --locked.

The builder is pinned to a rev rather than a tag only because no release carries the typed-record codegen yet; it should go back to a tag ref once one is cut.

Verification

  • nix build .#packages.x86_64-linux.default — green, no warnings.
  • cargo test31 passed, 0 failed, including the three new ones.

🤖 Generated with Claude Code


Update — re-pinned to module-builder master

The pin moved again, from d256a42 to master (e45caf1). d256a42 was picked
only because it was the first commit carrying the Windows cross fixes; there is
no reason to sit between it and master.

Beyond the cross fixes this branch already relied on, e45caf1 brings:

  • protocol 0.6 — the caller-identity wave — and a protocol version that
    fails to parse now failing loudly rather than silently vanishing (48cfdbe);
  • the SDK chain relocked onto master, including logos-rust-sdk's teardown
    exports (bdc9804) and the lossless Qt type mapping (ba1b0aa);
  • ui_qml glue generated by logos-view-generator rather than
    logos-qt-generator (fe4f26c), and the view teardown hook restored along
    with the deduplicated qt-sdk (bc72ce3) — neither reaches this module, but
    logos-chat-ui follows this pin and both reach it there;
  • platform-keyed overlays in metadata.json (ad1899d), and an ABI test that
    reads the module-impl exports off the built plugin per backend
    (26a886b).

Verification

On aarch64-darwin, against this lock:

check result
nix run .#generate provider backend: 19 methods, 7 events, 1 dep client
cargo fmt --check clean
cargo clippy --locked --all-targets -- -D warnings clean
cargo test --locked 31 passed, 0 failed
nix build .#default .#lgx-portable green, darwin-arm64 variant

The Windows target in #66 (which stacks on this) still evaluates end to end on
the new pin: nix build --dry-run walks the whole cross closure, and from an
empty chroot store with only the public cache it reports 54 derivations to
build — comfortably inside the cold-derivation-budget: 120 set there.

The one downstream consequence worth naming: the lossless Qt mapping means
optional record fields are now std::optional<QString> rather than QVariant.
logos-co/logos-chat-ui#57 carries that migration and is pinned to this branch's
head.

Copilot AI lite review requested due to automatic review settings August 14, 2026 18:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

0.2.6's logos-rust-sdk generated the provider trait with
`serde_json::Value` wherever chat_module.lidl declares a record. The
newer SDK generates the records themselves, so the trait now reads the
way the contract does:

    fn init(&mut self, config: ChatConfig) -> Result<Value, String>
    fn list_conversations(&mut self) -> Vec<Conversation>
    fn get_messages(&mut self, convo_id: String) -> Vec<Message>
    fn list_group_members(&mut self, convo_id: String) -> Vec<GroupMember>
    fn status(&mut self) -> Status

Nothing here was broken before this: chat_module built green on 0.2.6.
The bump is what the Windows target needs (mkLogosModule only grew an
x86_64-windows target after 0.2.6), and the migration is its cost.

What the migration removes is worth calling out, because it is most of
the diff:

- The hand-written `#[derive(Serialize)]` mirrors -- `ConversationSummary`,
  `StatusView`, `GroupMemberRow` -- existed only to be serialised into the
  shape the .lidl declares. The generated records ARE that shape, so the
  getters build them directly and the mirrors (and the chance of one
  drifting from the contract) are gone.
- `chat_config`/`config_field` decoded `init`'s config out of an untyped
  `Value`, including the case where `logoscore call` could only send the
  record's JSON *text*. A typed parameter makes decoding the SDK's job.

The four tests over that decoding went with it; they covered deleted
code. The defaulting they also happened to pin down did not go with it --
it is this module's own behaviour (a05d511 introduced it deliberately),
so it is now `delivery_preset()` with three tests of its own.

`persistence::DisplayMessage` stays the stored shape and is mapped to the
`Message` record at the boundary; the counts and timestamps it and
`Conversation` carry widen to the contract's signed 64-bit numbers.

Cargo.lock moves 0.2.0 -> 0.3.0 for the logos-rust-sdk path dependency,
which CI needs because it runs `cargo test --locked`.

Verified: nix build green, and `cargo test` 31 passed / 0 failed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@dlipicar
dlipicar force-pushed the feat/bump-builder-typed-records branch from 418a148 to 9c0b7a8 Compare August 14, 2026 18:25
@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown

Doc-test report

Headless chat_module instances built against this commit, driven through the 1:1 round-trip and the three-instance group conversation, rendered alongside the commands actually run and their output (updated each run, commit deafa92):

Pages can take a minute to update after the run finishes.

@osmaczko osmaczko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks

Master, not the cross-fix rev this branch first landed on. `d256a42` was
picked because it was the first commit carrying the Windows cross fixes; master
has since moved a long way past it and there is no reason to sit between the
two.

`e45caf1` brings, beyond the cross fixes already relied on here:

  * protocol 0.6 -- the caller-identity wave (logos-module-builder e45caf1),
    and a protocol version that fails to parse now fails loudly instead of
    silently vanishing (48cfdbe);
  * the SDK chain relocked onto master, including logos-rust-sdk's teardown
    exports (bdc9804) and the lossless Qt type mapping (ba1b0aa);
  * ui_qml glue generated by logos-view-generator rather than
    logos-qt-generator (fe4f26c), and the view teardown hook restored along
    with the deduplicated qt-sdk (bc72ce3) -- neither reaches this module, but
    logos-chat-ui follows this pin and both reach it there;
  * platform-keyed overlays in metadata.json (ad1899d), and an ABI test that
    reads the module-impl exports off the BUILT plugin per backend (26a886b).

Verified on aarch64-darwin against this lock: `nix run .#generate` emits the
provider backend (19 methods, 7 events, 1 dep client), `cargo fmt --check` and
`cargo clippy --all-targets -- -D warnings` are clean, `cargo test --locked`
is 31 passed / 0 failed, and `nix build .#default .#lgx-portable` produces the
plugin and a `darwin-arm64` .lgx.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants