Skip to content

Follow-up to #136: tests + type/env polish for SH_MODEL_CUSTOM#144

Merged
moonlight16 merged 1 commit into
rossoctl:mainfrom
moonlight16:feat/sh-model-custom-followups
Jul 21, 2026
Merged

Follow-up to #136: tests + type/env polish for SH_MODEL_CUSTOM#144
moonlight16 merged 1 commit into
rossoctl:mainfrom
moonlight16:feat/sh-model-custom-followups

Conversation

@moonlight16

Copy link
Copy Markdown
Member

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:

  1. Unit tests (harness/test/run-turn-model.test.ts) — extends the existing model test file:

    • requireModel throws the clear error when SH_MODEL_CUSTOM=1 but ANTHROPIC_BASE_URL is unset.
    • With both set, it synthesizes a model whose id/name = SH_MODEL, baseUrl = ANTHROPIC_BASE_URL, api = anthropic-messages, provider defaults to anthropic, and context/token defaults are applied.
    • SH_MODEL_PROVIDER / SH_MODEL_CONTEXT_WINDOW / SH_MODEL_MAX_TOKENS overrides are honored.
  2. Tighten the cast — the synthesized object is now typed Model<"anthropic-messages"> instead of as ReturnType<typeof getModel> & object. If pi-ai’s Model type later gains a required field, this fails to compile instead of silently omitting it.

  3. Thread env into requireModelrequireModel(provider, modelId, env = process.env), matching its sibling resolveModelSelection(config, env). This also lets the new tests run hermetically without mutating process.env. The param is optional, so existing 2-arg callers (run-leaf.ts) are unchanged.

  4. Env var docs — the four SH_MODEL_CUSTOM / SH_MODEL_CONTEXT_WINDOW / SH_MODEL_MAX_TOKENS / SH_MODEL_PROVIDER vars are documented in README-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.

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 mrsabath 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.

Small, high-quality follow-up to #136 — verified the full diff.

  • Type tightening (run-turn.ts): replaces the as ReturnType<typeof getModel> & object cast with a proper Model<"anthropic-messages"> annotation, so tsc now checks the object shape — exactly the "fails to compile instead of silently omitting a field" safety the comment claims. The provider cast is narrowed from as never to Model<...>["provider"].
  • Testability: requireModel gains an injectable env param defaulting to process.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).

@moonlight16
moonlight16 merged commit 557f02b into rossoctl:main Jul 21, 2026
10 checks passed
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.

2 participants