Replace the closed Harness enum with the open ProviderId identity - #100
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce09e41283
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// the wire form is unchanged (bare snake_case strings such as "codex" and | ||
| /// "claude_code"), and sessions persisted without the field still default to | ||
| /// codex via the serde defaults on the structs below. | ||
| pub type Harness = ProviderId; |
There was a problem hiding this comment.
When a valid new provider ID such as future_provider reaches a Session—which this alias and its new deserializer now allow—the IPC payload contains that string, but src/lib/types.ts:4 still declares Harness as only 'codex' | 'claude_code'. The frontend performs no runtime validation and routes non-Codex values through Claude/non-Codex branches such as src/lib/sessionGrid.ts:36, so an unknown provider is silently mislabeled rather than handled generically. Update the frontend mirror in this change, or keep the serialized backend identity closed until the descriptor-driven frontend lands.
AGENTS.md reference: AGENTS.md:L22-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this here, with reasoning rather than dismissal: no data path in this build can put a non-builtin id into a Session. The only writers are the two builtin adapters, which hard-set their own ids; deserialization (history store, scan cache) re-reads only what previous builds wrote, which is exclusively "codex"/"claude_code". The window this comment worries about — persisted data carrying a third id while the TS union is closed — requires a build that writes such ids, and that build (#40's provider wave) is sequenced behind the descriptor-driven frontend: #101 (stacked on this PR) already ships list_providers, and PR-2b replaces the closed TS union with descriptor-driven surfaces before any new provider can exist. Widening the TS type today would discard exhaustiveness checking across ~268 sites for a state no build can produce, in exchange for generic handling nothing can exercise.
ProviderId gains serde as the bare id string, wire- and storage-compatible with the snake_case strings the closed enum always persisted. model.rs aliases Harness to ProviderId during the migration, defaults deserialization to codex where the enum's Default previously did, and storage_id_for_session preserves every persisted id byte-for-byte with a generic segment for non-codex providers. Co-Authored-By: Claude Fable 5 <[email protected]>
Sweeps every remaining Harness::Codex/Harness::ClaudeCode reference left by the foundation commit that replaced the closed two-variant enum with `pub type Harness = ProviderId`. Construction and comparison sites now call codex_provider_id()/claude_code_provider_id(); matches on the open ProviderId are rewritten as explicit if/else chains that stay exhaustive about the two builtins and fail safe (or pick a documented neutral default) for any future provider. Copy-loss move errors are fixed by borrowing (&Harness) or cloning the cheap Arc<str> where an owned value is genuinely needed. provider.rs's now-tautological accepts_cached_session comparisons are simplified to direct ProviderId equality. cargo check --all-targets, cargo fmt --check, and cargo test are all clean (210 passed, 2 ignored on Windows; 7 additional store.rs tests are gated to non-Windows CI and were audited by hand). npm run check and npm test remain green and untouched, confirming the TS wire format is unaffected. Co-Authored-By: Claude Fable 5 <[email protected]>
The subscription-usage selection and the scan-cache shape fixture landed on main using the enum's Copy and variant syntax; convert them to ProviderId clones and constructors. Co-Authored-By: Claude Fable 5 <[email protected]>
ce09e41 to
65cadda
Compare
1 participant
Part of #37 (PR 1 of 2). Advances the roadmap's provider-contract seam by making the crate-wide session identity open rather than a two-variant enum. PR 2 will add the versioned provider-keyed config migration, the descriptor IPC command, and the descriptor-driven frontend.
Approach. The enum always serialized as bare snake_case strings, so
ProviderId(the validatedArc<str>newtype PR #82 introduced) gains serde with the identical wire form — every persisted session, scan-cache row, history-store blob, and the entire TypeScript wire contract are unchanged;src/is untouched.Harnessremains as a type alias during the migration. Deserialization of sessions without the field still defaults to codex, andstorage_id_for_sessionpreserves every persisted storage id byte-for-byte.Unknown-provider policy (the substance of the audit #37 asks for): each former
matchis now explicit about ids it doesn't know. Behavior branches fail safe — turn-receipt parsing errors, transcript-path validation trusts no roots, receipt hooks stay disabled. Pure-data branches get neutral values — per-provider status files derive collision-free names from the id (the id charset is filesystem-safe by construction), guidance text falls back to non-branded wording, and two label matches that provably reconstructedas_str()were simplified to it. Every such site carries a comment and was individually reviewed.Validation: cargo test --all-targets 210 passed / fmt clean; vitest 62 + node tests 23; svelte-check 0 errors. The one site the Windows sandbox could not compile (
store.rs'scfg(not(windows))test module) is a trivial constructor swap that this PR's Linux CI does compile. Foundation commit implemented directly; the 81-error mechanical sweep was executed by a subagent under a written conversion policy, with its full fallback-site inventory reviewed hunk-by-hunk and all suites re-run independently.Merge note: expect small conflicts with #98/#99 in
commands.rs/scan_cache.rs(both add code usingCopysemantics of the old enum); this branch rebases after they land.🤖 Generated with Claude Code