Follow-up to #136: tests + type/env polish for SH_MODEL_CUSTOM#144
Merged
moonlight16 merged 1 commit intoJul 21, 2026
Merged
Conversation
Applies the non-blocking review suggestions from rossoctl#136 that should have been in that PR (my omission — I merged rossoctl#136 before addressing them): - Add unit tests for the SH_MODEL_CUSTOM path in harness/test/run-turn-model.test.ts: (1) requireModel throws the clear error when SH_MODEL_CUSTOM=1 but ANTHROPIC_BASE_URL is unset; (2) with both set it synthesizes a model whose id/name = SH_MODEL, baseUrl = ANTHROPIC_BASE_URL, provider defaults to anthropic; (3) SH_MODEL_PROVIDER / SH_MODEL_CONTEXT_WINDOW / SH_MODEL_MAX_TOKENS overrides are honored. - Type the synthesized object as Model<"anthropic-messages"> instead of `as ReturnType<typeof getModel> & object`, so tsc checks the shape (a future required field on pi-ai's Model type now fails to compile rather than being silently omitted). - Thread env into requireModel(provider, modelId, env = process.env) — matches its sibling resolveModelSelection(config, env) and lets the new tests run hermetically without mutating process.env. Existing 2-arg callers (run-leaf.ts) are unaffected (param is optional). Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Jeremy Cohn <Jeremy.Cohn@ibm.com>
mrsabath
approved these changes
Jul 21, 2026
mrsabath
left a comment
There was a problem hiding this comment.
Small, high-quality follow-up to #136 — verified the full diff.
- Type tightening (
run-turn.ts): replaces theas ReturnType<typeof getModel> & objectcast with a properModel<"anthropic-messages">annotation, sotscnow checks the object shape — exactly the "fails to compile instead of silently omitting a field" safety the comment claims. Theprovidercast is narrowed fromas nevertoModel<...>["provider"]. - Testability:
requireModelgains an injectableenvparam defaulting toprocess.env— clean and backward-compatible. - Tests: three assertive tests cover the error path (missing
ANTHROPIC_BASE_URL), the synthesis path, and the overrides path (SH_MODEL_PROVIDER/SH_MODEL_CONTEXT_WINDOW/SH_MODEL_MAX_TOKENS). No hidden skips.
Verdict: APPROVE — no issues.
Areas reviewed: TypeScript, tests. Commits: 1, signed-off (DCO green). CI: passing (11/11).
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
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.
Applies the non-blocking review suggestions from #136 (
SH_MODEL_CUSTOM) that should have been included in that PR — they were requested during review but I merged #136 before addressing them. This follow-up closes that gap.Addresses the four review points from #136:
Unit tests (
harness/test/run-turn-model.test.ts) — extends the existing model test file:requireModelthrows the clear error whenSH_MODEL_CUSTOM=1butANTHROPIC_BASE_URLis unset.id/name=SH_MODEL,baseUrl=ANTHROPIC_BASE_URL,api=anthropic-messages,providerdefaults toanthropic, and context/token defaults are applied.SH_MODEL_PROVIDER/SH_MODEL_CONTEXT_WINDOW/SH_MODEL_MAX_TOKENSoverrides are honored.Tighten the cast — the synthesized object is now typed
Model<"anthropic-messages">instead ofas ReturnType<typeof getModel> & object. If pi-ai’sModeltype later gains a required field, this fails to compile instead of silently omitting it.Thread
envintorequireModel—requireModel(provider, modelId, env = process.env), matching its siblingresolveModelSelection(config, env). This also lets the new tests run hermetically without mutatingprocess.env. The param is optional, so existing 2-arg callers (run-leaf.ts) are unchanged.Env var docs — the four
SH_MODEL_CUSTOM/SH_MODEL_CONTEXT_WINDOW/SH_MODEL_MAX_TOKENS/SH_MODEL_PROVIDERvars are documented inREADME-k8s.md(added in the separate generic-k8s PR Add setup-k8s.sh: generic Kubernetes install path #142), so operators can discover the opt-in.No behavior change to the default path; purely tests + type-safety + testability.