Skip to content
BranchesTags

Folders and files

NameName
Last commit message
Last commit date

Latest commit

 

History

14 Commits
README.md
README.md
 
 

Repository files navigation

github-contribution-log

Contribution [1]: [Adapter: OpenClaw]

Contribution Number: 1
Student: Darin Andoh-Mensah
Issue: orthogonalhq/nous-core #299
Status: ✅ Complete — PR #402 merged (2026-07-05)

Why I Chose This Issue

I chose this issue because it deals directly with building out clean, fast, and highly efficient backend systems, which aligns perfectly with my goal to expand my backend expertise. Implementing an engine adapter for a personal-AI framework requires a deep understanding of data flows, data validation, and minimal-overhead processing.

Because this task involves mapping complex, real-time lifecycle functions without introducing execution latency, it gives me a great opportunity to learn how to write robust, production-ready server-side logic that remains lightweight and responsive.

Understanding the Issue

Scope correction (per the 2026-06-18 maintainer update on #299): This issue was originally written against the legacy AgentAdapter integration path under self/subcortex/coding-agents/. That path has been superseded and must not be used. OpenClaw is now to be implemented as a CLI provider leaf under self/subcortex/providers/src/providers/<vendor>/, against the integration branch feat/contributor-friendly-inference-provider-surface. The sections below describe the issue in terms of that current contract.

Problem Description

nous-core talks to external AI models and agents through certified provider leaves — small, self-contained modules that live under self/subcortex/providers/src/providers/ and conform to a shared contract so the platform can discover, construct, and route to them uniformly. The platform ships leaves for anthropic, openai, ollama, and a command-line agent (codex-cli), but no leaf for OpenClaw. Without one, there is no supported way for the platform to run OpenClaw as a model provider.

OpenClaw is a command-line (CLI) agent, so it belongs to the same family as codex-cli: it is driven by the agent-cli protocol (the platform spawns a local process, sends a prompt, and reads the response back from the process output) rather than by an HTTP API.

Expected Behavior

The provider catalog should include a production-grade openclaw leaf that:

  • Declares its metadata via ProviderDefinitionLeaf (vendor key, protocol, capabilities, and a declared executionCapabilityProfile), with its built-in provider ID derived from the vendorKey rather than hand-authored.
  • Speaks the agent-cli protocol: builds a non-interactive CLI invocation, delivers the prompt, and returns the model output — supporting both single-shot invoke() and streaming.
  • Is auto-discovered by the provider code generator and resolvable through the shared registry/adapter resolver, exactly like the existing codex-cli leaf.

Current Behavior

On the integration branch, self/subcortex/providers/src/providers/ contains anthropic, codex-cli, ollama, and openai — and no openclaw directory. As a result, openclaw is absent from the generated provider catalog (provider-definitions.ts / provider-adapters.ts / provider-factories.ts), so the registry has no factory to construct an OpenClaw provider and no adapter key to resolve. The capability simply does not exist yet — this is a missing-feature gap, not a runtime crash.

Affected Components

  • New provider leaf: self/subcortex/providers/src/providers/openclaw/ — definition.ts, adapter.ts, implementation.ts, provider.ts, index.ts.
  • Shared contracts (read-only references): the agent-cli protocol under self/subcortex/providers/src/protocols/agent-cli/, the ProviderDefinitionLeaf schema in self/subcortex/providers/src/schemas/provider-definition.ts, and the vendorKey → providerId derivation in self/subcortex/providers/src/provider-identity.ts.
  • Generated catalog (regenerated, not hand-edited): provider-definitions.ts, provider-adapters.ts, provider-factories.ts.
  • Wiring + tests: the codegen's per-vendor extra-exports map in scripts/generate-provider-aggregates.mjs, the package entry point src/index.ts, the new test file under src/__tests__/providers/, and the existing roster assertions that enumerate the certified providers.

Reproduction Process

Environment Setup

Per CONTRIBUTING.md, the repo requires Node 22+ and pnpm 10+ (npm-based installs are not supported for this workspace). Setup:

  1. Clone the fork and add the upstream remote (orthogonalhq/nous-core).
  2. Fetch and base the work on the active integration branch: feat/contributor-friendly-inference-provider-surface — this is where the CLI-provider machinery (agent-cli protocol, cli-session-manager, provider-identity, and the reference codex-cli leaf) lives. None of it exists on main.
  3. pnpm install then pnpm build from the provider package to confirm a clean baseline.

Steps to Reproduce (the missing-feature gap)

  1. Check out the integration branch and list the provider leaves: ls self/subcortex/providers/src/providers/.
  2. Observe the directory contains only anthropic, codex-cli, ollama, and openai — there is no openclaw leaf.
  3. Inspect the generated catalog (provider-definitions.ts) and confirm openclaw is not among the registered vendor keys.
  4. Observed result: the platform has no way to construct or route to an OpenClaw provider — the integration the issue asks for is simply absent.

Reproduction Evidence

  • Working branch: feature/openclaw-adapter (based on upstream feat/contributor-friendly-inference-provider-surface).
  • Findings: Confirmed via directory inspection and the generated catalog that no openclaw leaf or vendor key exists on the integration branch, while codex-cli provides a complete, working template for a CLI-based provider leaf to model the implementation on.

Solution Approach

Analysis

The gap is that no openclaw provider leaf exists. The platform's provider catalog is auto-generated by scanning self/subcortex/providers/src/providers/ for leaves that supply the four required files (definition.ts, adapter.ts, provider.ts, index.ts). So the fix is to add a correctly-shaped openclaw leaf and let the generator register it — no hand-editing of the generated catalog. The existing codex-cli leaf is the closest analog (it is also a CLI agent on the agent-cli protocol) and serves as the reference pattern.

Proposed Solution

Implement an openclaw CLI provider leaf that mirrors codex-cli's structure but uses OpenClaw's own clean CLI contract: a one-shot, non-interactive openclaw run --headless --no-color invocation, the prompt delivered over stdin, and the final response read back from stdout. The leaf declares executionCapabilityProfile: 'session_bound_command' (a one-shot exec, not a long-lived process), supports an optional --model flag, honors executable overrides via environment variables, handles abort signals, streams stdout transcript chunks, and maps CLI failures to typed NousErrors. After adding the leaf, regenerate the catalog so it is discovered, and update the codegen's extra-exports map and the roster assertions that enumerate certified providers.

Implementation Plan

Using the UMPIRE framework (adapted):

Understand: nous-core has no OpenClaw provider leaf, so it cannot run OpenClaw as a model provider. The current contract is a CLI provider leaf on the agent-cli protocol, not the deprecated AgentAdapter.

Match: Use the existing codex-cli leaf as the reference pattern — it is the certified example of a CLI agent on the same protocol — and follow the ProviderDefinitionLeaf schema and vendorKey-derived ID convention.

Plan:

  1. Rebase the working branch onto the integration branch feat/contributor-friendly-inference-provider-surface.
  2. Create the five-file leaf under self/subcortex/providers/src/providers/openclaw/.
  3. Implement the agent-cli invocation (stdin prompt → stdout response), streaming, executable resolution, and error mapping.
  4. Run pnpm run generate:providers to register the leaf in the generated catalog, and add the OpenClaw helpers to the codegen extras map + src/index.ts.
  5. Update the existing roster assertions to include openclaw.
  6. Write a unit suite (modeled on codex-cli's) using an injected fake runner, then run pnpm build and the full provider test suite.

Implement: Branch Link

Review: Verify compliance with CONTRIBUTING.md — strict typing, no fetch/process.env in the definition (metadata-only), and a clean generate:providers --check.

Evaluate: Run pnpm build (clean typecheck/compile), the new openclaw.test.ts suite, and the full provider package suite to confirm no regressions. (Results recorded in the Testing Strategy section below.)

Testing Strategy

Testing follows the existing provider-leaf conventions in the repo. The reference leaf (codex-cli) is mirrored: all CLI execution is driven through an injected fake runner (createFakeAgentCliRunner) so the unit suite never shells out to a real binary. The full provider package suite was run to prove the new leaf integrates without regressing the existing roster.

Unit Tests

New suite: self/subcortex/providers/src/__tests__/providers/openclaw.test.ts — 9 cases, all passing:

  • Definition metadata — OPENCLAW_PROVIDER_DEFINITION declares vendorKey: 'openclaw', protocol: 'agent-cli', adapterKey: 'openclaw', providerClass: 'local_text', isLocal: true, executionCapabilityProfile: 'session_bound_command', capabilities.streaming: true; the agentCli block validates cleanly through AgentCliProviderMetadataSchema; defaultArgs equal ['run', '--headless', '--no-color'].
  • Prompt rendering — renderOpenClawPrompt flattens system prompt + gateway context frames + tool definitions into a single prompt string.
  • ProviderAdapter contract — executionCapabilityProfile is exposed, capabilities.streaming is true, formatRequest produces the expected prompt, and parseResponse falls back to a text-safe response for non-JSON output instead of throwing.
  • invoke() happy path — with an injected runner, stdout is returned as output (trimmed), usage.computeMs is derived from the runner timing, and the invocation carries the correct executable, env (NO_COLOR: '1'), timeout, metadata, and CLI args (--model claw-pro appended).
  • Default-model path — when modelId is the synthetic default, no --model flag is passed; messages[] input is rendered to the stdin prompt (user: Summarize this.).
  • Error mapping — a non-zero CLI exit is mapped to a thrown typed NousError carrying the stderr tail.
  • Streaming — stream() yields each stdout transcript chunk as a content delta, then a terminal { content: '', done: true } chunk.
  • Executable resolution — selectOpenClawExecutable honors precedence: explicit option → NOUS_OPENCLAW_CLI_BIN → OPENCLAW_CLI_BIN → 'openclaw'.
  • Live-runner construction — the provider constructs with the default (real) process runner without spawning anything during tests.

Worked examples (input → expected → actual, captured from a live run):

Case Input Expected Actual
invoke() output { prompt: "Build the provider leaf." }, runner stdout "openclaw saw: …" output: "openclaw saw: Build the provider leaf.", usage.computeMs: 80 identical ✅
Default-model args modelId: "openclaw/default" ["run","--headless","--no-color"] identical ✅
Custom-model args modelId: "claw-pro" ["run","--headless","--no-color","--model","claw-pro"] identical ✅
messages[] → stdin [{ role:"user", content:"Summarize this." }] "user: Summarize this." identical ✅
Non-zero exit exitCode: 1, stderr "openclaw: model not found" throws NousError containing stderr threw: Agent CLI exited with code 1. openclaw: model not found ✅
Streaming CLI emits "Hello ", "world" [{content:"Hello ",done:false},{content:"world",done:false},{content:"",done:true}] identical ✅

Integration Tests

The leaf is auto-discovered by the provider codegen and flows through the shared catalog/registry. Existing integration suites were extended to include openclaw and re-run:

  • provider-codegen.test.ts — --list output and the checked-in generated aggregates (provider-definitions.ts, provider-adapters.ts, provider-factories.ts) stay in sync after adding the leaf.
  • provider-definitions.test.ts / provider-definition-types.test.ts — openclaw is in the validated vendor roster, derives its wellKnownProviderId from vendorKey, and the definition source stays metadata-only (no fetch / process.env / wellKnownProviderId).
  • adapter-resolver.test.ts — the openclaw adapter module resolves through the canonical resolver.
  • provider-pipeline-integration.test.ts — openclaw aggregates by vendor key, and the registry constructs an OpenClawProvider end-to-end (definition → factory → adapter → registry).

Manual Testing / Validation

Run from self/subcortex/providers:

  • pnpm run check:generated → generated catalogs in sync (exit 0).
  • pnpm run build (check:generated + tsc --build --force) → clean typecheck/compile, no errors.
  • npx vitest run src/__tests__/providers/openclaw.test.ts → 9/9 passing.
  • npx vitest run (full package suite) → 314 passing, 2 skipped (the 2 skipped are the pre-existing codex-cli.live.test.ts cases that require a real CLI binary). No regressions introduced by the new leaf.

Implementation Notes

Progress Summary

Pivot to the current contract. A maintainer update (2026-06-18) redirected #299 from the deprecated AgentAdapter / coding-agents path to the new CLI provider leaf contract on feat/contributor-friendly-inference-provider-surface. First step was rebasing the working branch onto that integration branch (the CLI-provider machinery — agent-cli protocol, cli-session-manager, provider-identity, and the reference codex-cli leaf — lives there, not on main).

Studied the reference leaf. Read codex-cli end-to-end (definition / adapter / implementation / provider / index), the agent-cli protocol (adapter.ts, runner.ts), the ProviderDefinitionLeaf schema, provider-identity (the vendorKey → wellKnownProviderId derivation), and the codegen that auto-generates the provider aggregates.

Built the OpenClaw leaf. Implemented the five-file leaf mirroring codex-cli's shape but with OpenClaw's own clean CLI contract: a one-shot openclaw run --headless --no-color invocation, prompt delivered over stdin, final response read back from stdout, optional --model flag, env-var executable overrides, abort-signal handling, streaming via stdout transcript chunks, and typed NousError failure mapping. Wired it into the catalog by regenerating the aggregates and updated the codegen's per-vendor extras map so the package root re-exports the OpenClaw helpers.

Challenges Faced

  • Stale issue scope. The issue text and the original branch pointed at a superseded subsystem. Resolved by following the maintainer note, switching to the integration branch, and treating codex-cli as the source-of-truth pattern.
  • Catalog is generated, not hand-edited. provider-definitions.ts / provider-adapters.ts / provider-factories.ts carry a "do not edit by hand" header and are produced by generate-provider-aggregates.mjs. The leaf is auto-discovered from the directory, so the fix was to add the four required files + run generate:providers, then keep the codegen --check green.
  • Roster-coupled tests. Several suites assert the exact vendor roster (sorted) and exact type unions (ProviderVendorKey). Adding a leaf intentionally breaks these as a tripwire; updated each assertion to include openclaw in the correct alphabetical position (openclaw sorts after openai).
  • Scoping the execution profile. Chose session_bound_command (matching codex-cli) because OpenClaw runs as a one-shot headless exec, not a long-lived persistent_process — so persistent-chat surfaces correctly reject it via capability guardrails. Documented in the definition caveats.

Code Changes

Branch: feature/openclaw-adapter (based on upstream feat/contributor-friendly-inference-provider-surface).

  • New files (the leaf): self/subcortex/providers/src/providers/openclaw/{definition,adapter,implementation,provider,index}.ts
  • New tests: self/subcortex/providers/src/__tests__/providers/openclaw.test.ts
  • Generated (regenerated, not hand-edited): provider-definitions.ts, provider-adapters.ts, provider-factories.ts
  • Hand-edited: scripts/generate-provider-aggregates.mjs (added openclaw to the per-vendor extra-exports map); src/index.ts (export OpenClawProvider); roster assertions in adapter-resolver.test.ts, provider-codegen.test.ts, provider-definition-types.test.ts, provider-definitions.test.ts, provider-pipeline-integration.test.ts
  • Approach decisions:
    • Mirror codex-cli, don't invent a new pattern — keeps the contribution review-friendly and consistent with the certified-leaf shape reviewers expect.
    • stdin prompt + stdout response, transcript format text — simplest honest contract for a headless CLI; fully exercisable through the fake runner with no real binary.
    • Inject the runner everywhere — the live spawn-based runner is only constructed lazily; all unit tests use createFakeAgentCliRunner, so the suite is deterministic and offline.

Pull Request

PR Link: orthogonalhq/nous-core #402 (against feat/contributor-friendly-inference-provider-surface)

PR Description:

Add an OpenClaw CLI provider leaf

What & why. nous-core routes to external models and agents through certified provider leaves under self/subcortex/providers/src/providers/. The catalog ships leaves for anthropic, openai, ollama, and the codex-cli command-line agent, but there is no leaf for OpenClaw — so the platform has no supported way to run OpenClaw as a provider (issue #299). OpenClaw is a CLI agent, so it belongs to the same family as codex-cli and uses the agent-cli protocol (spawn a local process, send a prompt, read the response) rather than an HTTP API.

What this PR adds. A production-grade openclaw leaf modeled on the codex-cli reference:

  • Five-file leaf under self/subcortex/providers/src/providers/openclaw/ (definition, adapter, implementation, provider, index).
  • Declares metadata via ProviderDefinitionLeaf with the provider ID derived from vendorKey, protocol: 'agent-cli', providerClass: 'local_text', isLocal: true, and executionCapabilityProfile: 'session_bound_command' (a one-shot headless exec, not a long-lived process).
  • Speaks the agent-cli contract: a non-interactive openclaw run --headless --no-color invocation, prompt delivered over stdin, response read back from stdout, with an optional --model flag, env-var executable overrides (NOUS_OPENCLAW_CLI_BIN → OPENCLAW_CLI_BIN → openclaw), streaming via stdout transcript chunks, and CLI failures mapped to typed NousErrors.
  • Auto-discovered by the provider codegen; the generated aggregates (provider-definitions.ts / provider-adapters.ts / provider-factories.ts) are regenerated, not hand-edited.

Testing. A focused suite (src/__tests__/providers/openclaw.test.ts) drives all CLI execution through an injected fake runner (createFakeAgentCliRunner) so unit tests never shell out, plus live-process tests that exercise the real spawn path. pnpm build is clean and the full provider package suite passes with no regressions.

Scope note. Built against feat/contributor-friendly-inference-provider-surface (the CLI-provider machinery does not exist on main). Per the 2026-06-18 maintainer update on #299, this intentionally targets the new provider-leaf contract rather than the superseded AgentAdapter / coding-agents path.

Maintainer Feedback:

  • 2026-06-22 — Changes requested (early-access provider integration review). The maintainer confirmed the overall provider-leaf shape is correct (agent-cli path, right metadata, executionCapabilityProfile: 'session_bound_command', generated catalog updates, focused fake-runner tests) and requested two changes before merge:

    1. Windows command-injection risk. The process runner passed the user-controllable config.modelId into CLI args (['--model', this.config.modelId]) and spawned with shell: platform === 'win32'. On Windows that routes user-controlled values through cmd.exe, where shell metacharacters in a model id could be interpreted instead of treated as a literal argument. Requested fix: avoid shell: true and spawn with literal argv semantics; handle any Windows .cmd resolution without letting model-id/prompt-derived values reach the shell.
    2. Abort handling only worked pre-start. The implementation snapshotted the abort state ({ aborted: request.abortSignal.aborted }) before spawn and only checked it before launching. Once openclaw was running, later cancellation did not kill the child, so a canceled request could run to completion or timeout — contradicting the PR's claim that abort signals are supported. Requested fix: either wire the abort signal to terminate the child after spawn, or narrow the claim/tests to pre-start-only.
    • (Non-blocking, maintainer-side follow-up they flagged: some persistent-chat guardrails still look Codex-specific and should become generic for agent-cli providers. They are handling this as provider-surface work; nothing required from this PR.)

  • 2026-06-28 — Addressed both requested changes.

    1. Shell-safe spawning. Removed shell: platform === 'win32'; the process runner now always spawns with shell: false and passes arguments as a literal argv array, so the model id and prompt-derived values can never be interpreted by a shell. Added planOpenClawSpawn, which resolves the executable per platform: POSIX and native Windows binaries are spawned directly, while a Windows .cmd/.bat shim (which Node refuses to spawn without a shell) is routed through cmd.exe /d /s /c with every argument explicitly escaped (windowsVerbatimArguments: true) rather than relying on the shell to join the command line. On Windows the bare openclaw name is resolved via where.exe, preferring a native .exe/.com over a .cmd shim. New tests assert literal argv passthrough for a metacharacter-laden model id and cover the .cmd escaping and .exe-preference branches.
    2. Live abort. The provider now forwards the real AbortSignal to the runner (alongside the pre-start snapshot the shared contract expects). After spawn the runner registers an abort listener that kills the child with SIGTERM and resolves the run as a failure, with the listener and timeout cleaned up on settle. New live-process tests confirm a post-start abort terminates the child promptly (not at timeout) and that a pre-start abort still short-circuits without spawning. The "abort supported" claim is now accurate for both pre- and post-start cancellation.
    • Verification: pnpm build clean; full provider suite 321 passing / 2 skipped; OpenClaw suite 16/16.

  • 2026-07-05 — Merged (initial early-access OpenClaw provider integration). The maintainer confirmed the final version lands OpenClaw as an agent-cli provider leaf with the expected session_bound_command metadata, generated provider-catalog wiring, package exports, focused fake-runner coverage, the Windows spawn hardening, and live abort handling. They resolved the final merge conflict on their side as maintainer-side provider-roster/catalog churn — additive conflict resolution from other provider leaves landing on the integration branch, not a contributor-side issue with this PR. The remaining generic agent-cli / persistent-chat guardrail cleanup is tracked separately so this contribution stays focused.

Status: ✅ Merged — 2026-07-05 (PR #402)

Learnings & Reflections

Technical Skills Gained

  • Building to a plugin contract instead of a bespoke integration. The biggest shift was learning to implement a certified provider leaf — a small, self-contained module that conforms to a shared contract (ProviderDefinitionLeaf, the agent-cli protocol, a vendorKey-derived provider ID, a declared executionCapabilityProfile) so the platform can discover, construct, and route to it uniformly. Modeling the leaf on the existing codex-cli reference taught me how to read a certified pattern and reproduce its shape rather than inventing my own.
  • Code-generated catalogs and "do not hand-edit" boundaries. The provider catalog (provider-definitions.ts / provider-adapters.ts / provider-factories.ts) is produced by a generator that scans the providers directory. I learned to add the leaf's four required files and run generate:providers to register it, keeping --check green, instead of editing generated output — and why roster-coupled tests intentionally break as a tripwire when a new vendor is added.
  • The agent-cli process model. Driving a CLI agent as a provider: a non-interactive openclaw run --headless --no-color invocation, prompt over stdin, response over stdout, streaming via transcript chunks, env-var executable resolution, and mapping CLI failures to typed NousErrors.
  • Secure child-process spawning. The review taught me the concrete mechanics of shell injection on Windows: how shell: true joins arguments into a single cmd.exe command line where metacharacters get interpreted, and how to avoid it by spawning with shell: false and a literal argv array, resolving the executable myself, and routing .cmd/.bat shims through cmd.exe with each argument explicitly escaped.
  • Correct cancellation semantics. The difference between a snapshot of an abort state (checked once before spawn) and a live AbortSignal wired to actually kill the running child — including cleaning up the listener and timeout on settle so nothing leaks.
  • Deterministic, offline testing of process code. Using an injected fake runner (createFakeAgentCliRunner) for the unit suite so tests never shell out, plus a few live-process tests (spawning real node) to prove literal-argv passthrough and post-start abort.

Challenges Overcome

  • A stale issue scope. Issue #299 was originally written against the deprecated AgentAdapter / coding-agents path. A maintainer update redirected it to the new CLI provider-leaf contract on the feat/contributor-friendly-inference-provider-surface integration branch. I resolved this by following the maintainer note, rebasing onto the integration branch (where all the CLI-provider machinery lives — none of it exists on main), and treating codex-cli as the source-of-truth pattern.
  • Security review feedback. My first version inherited two flaws from the reference pattern: a Windows shell-injection vector (user-controlled modelId passed through shell: true) and abort handling that only worked before the process started. Rather than doing the minimum, I fully wired live abort and rebuilt the spawn path to be shell-safe on every platform, then added focused tests for each so the "abort supported" claim became accurate.
  • Working in a busy, generated area. The provider catalog and roster assertions are touched by every new provider leaf, so the final merge hit conflicts from other leaves landing on the integration branch. The maintainer handled that as additive roster/catalog churn on their side — a reminder that generated, roster-coupled surfaces are naturally conflict-prone and that keeping the leaf's own files clean and regenerating the catalog is what keeps a contribution mergeable.

What I'd Do Differently Next Time

  • Read the reference pattern and its known weaknesses first. I copied codex-cli's spawn/abort shape faithfully — which also copied its latent shell-injection and pre-start-only abort issues. Next time I'll treat "the reference does it this way" as a starting point, not a guarantee, and run a security lens over child-process and user-input handling before review.
  • Verify environment assumptions earlier. I lost a little time to node's -e argument parsing (--model being read as a node flag) while writing the live-process tests. Confirming how the harness passes argv up front would have saved a debug cycle.
  • Confirm the target contract before writing code. The scope pivot from AgentAdapter to the provider-leaf contract could have cost a lot more if caught late; checking issue comments for maintainer updates before starting is now part of my process.

Resources Used
  • The codex-cli provider leaf (self/subcortex/providers/src/providers/codex-cli/) — the certified reference pattern this contribution was modeled on.
  • In-repo contracts — the agent-cli protocol (src/protocols/agent-cli/), the ProviderDefinitionLeaf schema (src/schemas/provider-definition.ts), and the vendorKey → providerId derivation (src/provider-identity.ts).
  • orthogonalhq/nous-core #299 and the maintainer's scope-pivot comment redirecting the work to the provider-leaf contract on the integration branch.
  • CONTRIBUTING.md — Node 22+ / pnpm 10+ toolchain, strict-typing and metadata-only definition rules, and the generate:providers --check requirement.
  • Node.js child_process docs — spawn options (shell, windowsVerbatimArguments, windowsHide) and the Windows .cmd/.bat spawning restrictions that informed the shell-injection fix.
  • PR #402 review thread — the maintainer's two requested changes (Windows spawn hardening, live abort) and the merge confirmation.

Contribution [2]: [AI Provider type safety — replace `any` with `LanguageModel`]

Contribution Number: 2
Student: Darin Andoh-Mensah
Issue: Vets-Who-Code/vets-who-code-app #879
Status: 🔨 Phase III In Progress — PR #1242 submitted, awaiting review

Why I Chose This Issue

After a heavy, architecture-first contribution (the OpenClaw provider leaf in #299), I wanted a focused, self-contained issue in a domain I care about — a well-scoped TypeScript type-safety task in a real production Next.js codebase. This issue is labeled beginner / good first issue / type-safety / typescript, which made it a good fit for learning a new repo's conventions quickly without a large surface area. It also supports a mission I connect with: #VetsWhoCode builds software to help military veterans and spouses learn to code.

More concretely, I chose it because "replace any with a proper type" is deceptively educational: doing it correctly means researching the real type the AI SDK expects, understanding how the model instances flow through the code, and proving the change compiles without weakening the public API — exactly the kind of careful, type-driven reasoning I want to get faster at.

Understanding the Issue

Problem Description

src/lib/ai-provider.ts is a small utility that initializes the project's AI providers (Google Gemini, Azure OpenAI, OpenAI, and Phi-3) and exposes helpers to pick a primary provider and run an operation with automatic fallback. It uses the any type in three places, which turns off TypeScript's type checking exactly where the model object is passed around — so mistakes using the model would not be caught at compile time, and editors give no autocomplete.

Expected Behavior

The three any types are replaced with the proper type from the AI SDK, so the provider instance and the fallback helpers are fully type-checked, and TypeScript still compiles.

Current Behavior

Three untyped any values (line numbers on master at the time of work):

  • Line 10 — instance: any; in the ProviderConfig interface (the constructed model object).
  • Line 134 — model: any; in the return type of getAIModelWithFallback().
  • Line 164 — operation: (model: any) => Promise<T> — the callback parameter of tryProvidersWithFallback<T>().

All three refer to the same underlying thing: the model instance returned by google("…"), azureProvider("…"), openai("…"), i.e. the value stored in ProviderConfig.instance and later handed to the caller's operation.

Affected Components

  • Only file changed: src/lib/ai-provider.ts (three any sites + one import).
  • Type source (read-only): the ai package's LanguageModel type (AI SDK v5) and, transitively, @ai-sdk/provider's LanguageModelV2.
  • Providers referenced: @ai-sdk/openai, @ai-sdk/azure, @ai-sdk/google — the factories whose return value is being typed.

Reproduction Process

Environment Setup

  • Toolchain: Node 24, npm (the repo uses package-lock.json; scripts typecheck = tsc -p tsconfig.json and lint = biome lint).
  • Setup: forked Vets-Who-Code/vets-who-code-app, cloned the fork, and ran npm install (~1,800 packages).
  • Challenge solved: the machine's global npm cache had root-owned files (EACCES), so npm install initially failed. Worked around it by pointing npm at a project-local cache dir (npm install --cache <local-cache>) rather than needing sudo.

Steps to Reproduce (the type-safety gap)

  1. Open src/lib/ai-provider.ts on master.
  2. grep -n ": any" src/lib/ai-provider.ts → three hits (lines 10, 134, 164).
  3. Observed result: the model instance flows through ProviderConfig.instance → getAIModelWithFallback() → tryProvidersWithFallback() entirely as any, so no type checking or editor assistance exists on the model object anywhere in the file.

Reproduction Evidence

  • Fork: DAmensah27/vets-who-code-app
  • Findings: confirmed all three any sites describe the same model type, so a single imported type applied in three places resolves the whole issue — no runtime code paths change.

Solution Approach

Analysis

The root cause is simply that the model object's type was never declared. The @ai-sdk/* factories return a concrete model instance; in AI SDK v5 (ai@^5.0.93, @ai-sdk/openai@^2.0.67) openai("gpt-4-turbo") returns a LanguageModelV2 (verified in @ai-sdk/openai's .d.ts: (modelId): LanguageModelV2). So all three anys should be that model type.

Research finding (a subtlety in the issue text): the issue suggested importing LanguageModel from @ai-sdk/provider. In this repo's installed v5, @ai-sdk/provider actually exports LanguageModelV2 (not LanguageModel), and it is only a transitive dependency — importing from it would be fragile. The ai package is a direct dependency and exports LanguageModel = string | LanguageModelV2, which the concrete LanguageModelV2 instances satisfy, and which is exactly the type AI SDK functions like generateText({ model }) consume. So the idiomatic, robust choice is LanguageModel from ai.

Proposed Solution

Add import type { LanguageModel } from "ai"; and replace all three anys with LanguageModel. Type-only change; no runtime behavior changes.

Implementation Plan

Using the UMPIRE framework (adapted):

Understand: three any sites in ai-provider.ts all describe the AI SDK model instance; typing them restores compile-time safety with no behavior change.

Match: follow the AI SDK's own public typing — LanguageModel from ai is the type the SDK exposes for a model, so consumers should use it rather than reaching into the transitive @ai-sdk/provider.

Plan:

  1. Fork + clone + npm install; establish a clean npm run typecheck baseline.
  2. Import LanguageModel from ai.
  3. Replace the three anys (ProviderConfig.instance, the getAIModelWithFallback() return model, the tryProvidersWithFallback() operation param).
  4. Run npm run typecheck and npx biome lint on the file.
  5. Commit on a topic branch, push to the fork, open a PR to upstream master.

Implement: fix/879-ai-provider-types — commit 394e5c4.

Review: self-checked against the acceptance criteria — correct @ai-sdk type researched, all three anys replaced, tsc compiles, no runtime paths touched; commit message conforms to the repo's commitlint config (refactor type, sentence-case subject, ≤72-char header).

Evaluate: npm run typecheck exits 0 with no errors; biome lint on the file is clean apart from a pre-existing, unrelated warning. (Results in Testing Strategy below.)

Testing Strategy

This is a type-only change (the LanguageModel annotation is erased at compile time), so there is no new runtime behavior to unit-test; correctness is proven by the type checker and linter.

Verification

  • grep -n ": any" src/lib/ai-provider.ts → no matches remaining.
  • npm run typecheck (tsc -p tsconfig.json) → exit 0, no errors.
  • npx biome lint src/lib/ai-provider.ts → clean, except one pre-existing, unrelated warning (await inside a loop at line 177 of the original file — not introduced by this change).
  • Diff review → exactly one added import + three any → LanguageModel replacements; no runtime code paths altered.

Manual Testing

Confirmed the three usage sites still typecheck against how the value is used: the concrete LanguageModelV2 instances (from openai()/google()/azure()) are assignable to LanguageModel, and the value continues to flow unchanged through getAIModelWithFallback() and tryProvidersWithFallback().

Implementation Notes

Progress Summary

  • Researched the real type. Read the installed .d.ts files to confirm the v5 return types and where LanguageModel vs LanguageModelV2 actually live, rather than taking the issue's suggested import path at face value.
  • Made the minimal correct change. One import + three annotations; verified with typecheck + lint.
  • Committed and pushed to the fork on branch fix/879-ai-provider-types (commit 394e5c4), then opened PR #1242 against upstream master.

Challenges Faced

  • The issue's suggested type was slightly off for this version. It pointed at LanguageModel from @ai-sdk/provider; in v5 that package exports LanguageModelV2 and is only transitive. Resolved by using LanguageModel from the ai direct dependency (documented above).
  • npm cache permissions. Root-owned files in the global npm cache blocked npm install; solved with a project-local --cache dir instead of sudo.
  • Commit hooks. The repo enforces commitlint via husky; the first commit was rejected for a non-sentence-case subject. Rewrote the message to conform (refactor(...), sentence-case, ≤72 chars).

Code Changes

  • File modified: src/lib/ai-provider.ts — +import type { LanguageModel } from "ai"; and three any → LanguageModel replacements.
  • Branch: fix/879-ai-provider-types (commit 394e5c4).
  • Approach decision: use the AI SDK's public LanguageModel type from a direct dependency instead of the more specific LanguageModelV2 from a transitive one — matches how the SDK is meant to be consumed and keeps the import robust to dependency changes.

Pull Request

PR Link: Vets-Who-Code/vets-who-code-app #1242 (against master)

PR Description (draft):

Replace any with LanguageModel in the AI provider utility (#879)

What & why. src/lib/ai-provider.ts used any in three places that all describe the same thing — the AI SDK model instance returned by openai()/google()/azure(). This PR types them with LanguageModel from the ai package (AI SDK v5), restoring compile-time safety and editor autocomplete with no runtime change.

Changes. Added import type { LanguageModel } from "ai"; and replaced the any on ProviderConfig.instance, the getAIModelWithFallback() return model, and the tryProvidersWithFallback() operation parameter.

Note on the type. The issue suggested LanguageModel from @ai-sdk/provider, but in the installed v5 that package exports LanguageModelV2 and is only a transitive dependency. ai (a direct dependency) exports LanguageModel = string | LanguageModelV2, which the concrete model instances satisfy and which is the type the SDK's own APIs consume — so I imported from ai.

Testing. Type-only change; npm run typecheck passes with no errors and biome lint is clean on the file.

Maintainer Feedback:

  • (none yet — awaiting first review)

Status: Awaiting review

Learnings & Reflections

Technical Skills Gained

  • Verifying types from source, not docs. Reading the installed .d.ts files (@ai-sdk/openai, ai, @ai-sdk/provider) to confirm what openai() actually returns and where each type is exported — and catching that the issue's suggested import didn't match the installed version.
  • Direct vs. transitive dependencies. Why importing a type from a transitive package is fragile, and how to prefer the equivalent type re-exported by a direct dependency.
  • A new repo's guardrails. Getting a clean tsc/biome baseline and satisfying a commitlint + husky commit-message policy.

Challenges Overcome

  • Diagnosed the EACCES npm-cache issue and worked around it without elevated permissions.
  • Reconciled the issue's suggested type with the repo's actual installed SDK version, choosing the correct, robust import.

What I'd Do Differently Next Time

  • Check the installed package versions and their exported types before accepting an issue's suggested import path — it saves a round-trip when the issue predates a dependency bump.

Resources Used
  • Vets-Who-Code/vets-who-code-app #879 — the issue, acceptance criteria, and the affected lines.
  • Vercel AI SDK v5 — the LanguageModel / LanguageModelV2 types (ai and @ai-sdk/provider type declarations) and the @ai-sdk/openai/azure/google provider factory return types.
  • The repo's own config — tsconfig.json, biome lint config, and commitlint.config.js (conventional-commits rules) that shaped the fix and the commit.

Contribution [3]: [opendot — regression test for the `--api-base` local-server path]

Contribution Number: 3
Student: Darin Andoh-Mensah
Issue: vedaant00/opendot #36
Status: 🔨 Phase III In Progress — PR #37 reviewed; requested change addressed, awaiting merge

Why I Chose This Issue

After a type-safety fix, I wanted to practice the other half of code quality: tests that lock in behavior. opendot is a terminal AI agent ("any model, every action reversible") built on LiteLLM, and this issue (enhancement, good first issue) is a self-contained ask — add regression coverage for a documented, user-visible feature that currently has none. Writing a good test here meant actually understanding how a config value threads through the agent loop into the model calls, which is exactly the kind of read-the-codebase-carefully skill I want to sharpen.

It also appealed to me because "the feature works but is untested" is one of the most common real-world contribution types, and doing it well (hermetic, mocked, mirroring existing patterns) is a transferable skill.

Understanding the Issue

Problem Description

opendot can point at a local, OpenAI-compatible server — llama.cpp's llama-server, vLLM, or LM Studio — via the --api-base flag. That flag becomes AgentConfig.api_base and is passed into the LiteLLM completion calls. The feature is documented and user-facing, but a search of tests/ finds no reference to api_base, so it has no regression protection and could break silently.

Expected Behavior / Acceptance Criteria

  1. A test verifying that when api_base is configured, it propagates into the model calls.
  2. Confirm that --api-base bypasses provider auto-detection (a local server needs no API key).
  3. Stub/mock the completion calls so no real server is needed.
  4. Follow patterns from the existing loop/usage tests.

Current Behavior

Feature works as documented, but "operates without regression protection" — zero tests reference api_base.

Affected Components

  • Only new file: tests/test_api_base.py (no source changes).
  • The path under test: AgentConfig.api_base (src/opendot/agent/config.py) → Agent._stream_turn / Agent._nonstream_turn in src/opendot/agent/loop.py, both of which pass api_base=self.config.api_base into litellm.acompletion(...) → the auto-detect bypass guard (if not api_base:) in _build_agent (src/opendot/cli.py).

Reproduction Process

Environment Setup

  • Toolchain: Python (repo requires ≥3.10; I used a local venv), pip install -e ".[dev]" (pulls pytest, pytest-asyncio, and the runtime deps). Test config lives in pyproject.toml (asyncio_mode = "auto", testpaths = ["tests"]).
  • Baseline: pytest → all green (105 pre-existing tests) before adding anything.

Steps to Reproduce (the coverage gap)

  1. grep -rn "api_base" tests/ → no matches.
  2. Read loop.py: both acompletion calls pass api_base=self.config.api_base; read cli.py: _build_agent skips provider auto-detection when api_base is set.
  3. Observed result: the whole --api-base path — propagation into both completion calls and the auto-detect bypass — is exercised by no test.

Reproduction Evidence

  • Fork: DAmensah27/opendot
  • Findings: the value threads through two acompletion sites (streaming + non-streaming fallback) and one CLI branch — so the test needs three assertions: stream propagation, non-stream propagation, and the auto-detect bypass (plus a default-None check and a contrast test that proves auto-detect still fires without --api-base).

Solution Approach

Analysis

Nothing is broken; the gap is missing tests. The cleanest coverage stubs LiteLLM entirely (the repo's tests/test_usage.py already does this with a "bare agent" + fake-LiteLLM pattern), so the tests are hermetic and never contact a server. The two behaviors worth pinning are (a) api_base reaching both acompletion calls, and (b) the CLI's if not api_base: guard skipping provider auto-detection.

Proposed Solution

Add tests/test_api_base.py with a _RecordingLiteLLM fake whose acompletion records its kwargs and returns a minimal stream / response, plus monkeypatched provider functions for the bypass test — all mirroring existing test conventions.

Implementation Plan (UMPIRE, adapted)

Understand: the --api-base path is untested; pin propagation into both completion calls and the auto-detect bypass. Match: reuse test_usage.py's bare-agent + fake-LiteLLM approach; use pytest monkeypatch/tmp_path fixtures (already used in test_catalog.py/test_office.py). Plan:

  1. Fork/clone; pip install -e ".[dev]"; establish a green baseline.
  2. Fake LiteLLM that records acompletion kwargs (stream + non-stream).
  3. Assert api_base propagates in _stream_turn and _nonstream_turn, and defaults to None.
  4. Test _build_agent with monkeypatched provider functions: bypass when api_base set; auto-switch when unset.
  5. Keep it hermetic (monkeypatch MCP config to None); run pytest, ruff check, ruff format --check. Implement: test/api-base-local-server — commit de15912. Review: conventional-commit (test:), mirrors existing patterns, no source touched, no real server contacted. Evaluate: pytest → 110 passed (5 new); ruff clean. (Details below.)

Testing Strategy

Unit Tests — tests/test_api_base.py (6 cases, all passing)

  • Streaming propagation — _stream_turn calls acompletion once with api_base == the configured URL, model == gpt-4o, stream == True.
  • Non-streaming propagation — _nonstream_turn calls acompletion with the configured api_base and stream == False.
  • Default is None (non-streaming) — with no --api-base, api_base=None reaches acompletion (provider default).
  • Default is None (streaming) — the same default holds on the streaming path (_stream_turn). (Added in review — see Maintainer Feedback.)
  • Auto-detect bypass — with api_base set, _build_agent keeps the chosen model and never consults env_var_for / model_for_available_key.
  • Contrast (bypass is meaningful) — without api_base, a missing key for the chosen model does trigger the auto-switch.

Manual Testing / Validation

  • pytest (full suite) → 110 passed (105 pre-existing + 5 new), no regressions.
  • ruff check tests/test_api_base.py → All checks passed; ruff format --check → already formatted.
  • Completion calls are stubbed via a fake LiteLLM (_RecordingLiteLLM) — no local server or network involved; the bypass tests monkeypatch MCP config to stay hermetic.

Implementation Notes

Progress Summary

  • Traced the api_base path end-to-end (config → loop → acompletion, plus the CLI bypass) so the tests assert real integration points, not incidental details.
  • Wrote 5 hermetic tests mirroring test_usage.py; verified with the full suite + ruff; committed on test/api-base-local-server (de15912) and opened PR #37.

Challenges Faced

  • Where the value actually flows. It's passed to acompletion in two places (streaming turn + non-streaming fallback); covering only one would leave half the feature unprotected — so the test hits both.
  • Keeping _build_agent hermetic. It loads MCP config and builds a real Agent; I monkeypatched the provider lookups and the MCP config loader so the bypass test is deterministic and offline.

Code Changes

  • New file: tests/test_api_base.py (no source changes).
  • Branch: test/api-base-local-server — commit de15912.
  • Approach decision: stub LiteLLM rather than spin up a real local server — matches the repo's existing test style and makes the suite fast and offline.

Pull Request

PR Link: vedaant00/opendot #37 (against main)

PR Description (summary): Adds tests/test_api_base.py — proves api_base propagates into both litellm.acompletion() calls and that --api-base bypasses provider auto-detection, with completion calls stubbed (no real server). pytest → 110 passed; ruff clean.

Maintainer Feedback:

  • 2026-07-31 — Approved with one small change requested. The maintainer (Vedaant Singh) called the coverage thorough and the auto-detect-bypass contrast test "a good touch," and flagged one gap Copilot caught: the api_base=None default was tested on the non-streaming path but not the streaming one — "then it's good to merge."
  • 2026-07-31 — Addressed. Renamed the existing case to test_api_base_defaults_to_none_nonstreaming and added test_api_base_defaults_to_none_streaming, which drives _stream_turn with the same assertion. Full suite 111 passed, ruff clean; pushed to the PR branch and replied on the thread.

Status: Change requested → addressed; awaiting merge

Learnings & Reflections

Technical Skills Gained

  • Testing async agent code deterministically — a fake LiteLLM with an async acompletion returning an async generator, driven through pytest-asyncio's auto mode.
  • Recording-mock assertions — capturing call kwargs to prove a config value reaches an external call, rather than asserting on observable side effects.
  • Hermetic tests around CLI wiring — monkeypatching provider lookups + MCP config so a function that builds a real object stays offline and deterministic.

Challenges Overcome

  • Made sure both completion sites were covered, and added a contrast test so the "bypass" assertion actually proves something.

What I'd Do Differently Next Time

  • Skim for every call site of the value under test before writing assertions — I caught the second acompletion early, but a grep-first habit would guarantee it.

Resources Used
  • vedaant00/opendot #36 — the issue and acceptance criteria.
  • tests/test_usage.py — the bare-agent + fake-LiteLLM pattern this test mirrors.
  • src/opendot/agent/loop.py, agent/config.py, cli.py — the api_base propagation and auto-detect-bypass code under test.
  • CONTRIBUTING.md + pyproject.toml — the pip install -e ".[dev]" / pytest workflow, conventional-commit convention, and ruff config.

Contribution [4]: [BetterDev Protocol — "City Organizers" footer link on the meetup page]

Contribution Number: 4
Student: Darin Andoh-Mensah
Issue: BetterDevOrg/Protocol #41
Status: ✅ Complete — PR #42 merged (2026-07-31)

Why I Chose This Issue

I wanted a quick, high-signal front-end contribution in a modern stack (Next.js App Router + Tailwind) to round out the batch. This good first issue is small and well-defined — add one missing navigation link — but doing it right still means matching the existing component's styling conventions, preserving the responsive layout, and verifying the build. It's a good exercise in "make the change indistinguishable from the surrounding code."

Understanding the Issue

Problem Description

The footer navigation on the /meetup (BetterDev Passport) page links Contact, Partners, Careers (and Docs) but omits a link to the organizers directory.

Expected Behavior / Acceptance Criteria

  1. A "City Organizers" link appears in the footer nav on /meetup.
  2. Clicking it routes to /organizers.
  3. Hover styling matches the sibling links.
  4. Mobile layout is preserved.
  5. npm run build completes.

Current Behavior

The footer nav has no organizers link.

Affected Components

  • File: src/app/meetup/page.tsx — the footer <nav> block.
  • Target route: /organizers (already exists at src/app/organizers/page.tsx).

Reproduction Process

Environment Setup

  • Toolchain: Node + npm (package-lock.json); Next.js 15.5. npm install (~720 packages).
  • Steps: open src/app/meetup/page.tsx; the footer <nav> renders Contact (/contact), Partners (/partnership), Careers (/careers), and Docs — no organizers link. Confirmed /organizers exists as a route.

Reproduction Evidence

  • Fork: DAmensah27/Protocol
  • Findings: siblings use <Link href="..." className="transition hover:text-white">; the <nav> uses flex flex-wrap ... justify-center gap-6 (already responsive), so adding one more <Link> reflows cleanly on mobile.

Solution Approach

Analysis

Purely additive: insert one <Link> matching the sibling pattern. No new component, no styling changes, no layout rework needed (flex-wrap handles reflow).

Proposed Solution / Plan (UMPIRE, adapted)

Understand: add a missing footer link to /organizers. Match: copy the exact <Link className="transition hover:text-white"> pattern used by Contact/Partners/Careers. Plan: (1) fork/clone/npm install; (2) add <Link href="/organizers">City Organizers</Link> in the footer <nav>; (3) next lint the file + next build. Implement: feat/meetup-footer-organizers-link — commit d48f063. Review: single-file, additive, styling matched, responsive layout preserved. Evaluate: file lints clean; build compiles successfully (see note below).

Testing Strategy

Manual Testing / Verification

  • Added <Link href="/organizers">City Organizers</Link> to the footer nav, styled identically to siblings (transition hover:text-white).
  • next lint --file src/app/meetup/page.tsx → No ESLint warnings or errors.
  • next build → ✓ Compiled successfully (types + compilation pass).
  • Responsive check by inspection: the nav's flex-wrap means the extra link wraps on narrow viewports without breaking layout.

Build note (honest): next build runs ESLint over the whole app and currently fails on pre-existing react/no-unescaped-entities errors in unrelated files (e.g. src/app/meetup/[meetupId]/page.tsx, which had You're RSVP'd on main before my change). My change is one isolated, lint-clean file and does not introduce or touch those errors — this is flagged transparently in the PR so a maintainer can decide whether to address the pre-existing lint debt separately.

Implementation Notes

Progress Summary

Added the single footer <Link> mirroring the sibling styling, verified the changed file lints clean and compiles, and confirmed the responsive layout is preserved. Committed on feat/meetup-footer-organizers-link (d48f063) and opened PR #42.

Challenges Faced

  • A failing whole-repo build that isn't mine. Diagnosed that next build failed on pre-existing lint errors in unrelated files; verified via git show HEAD:... that those errors predate my change, and that my file passes lint. Reported it transparently instead of silently "fixing" out-of-scope files.

Code Changes

  • File modified: src/app/meetup/page.tsx — +3 lines (one <Link> to /organizers).
  • Branch: feat/meetup-footer-organizers-link — commit d48f063.
  • Approach decision: keep the change minimal and out-of-scope-free — do not touch the unrelated pre-existing lint errors; surface them in the PR instead.

Pull Request

PR Link: BetterDevOrg/Protocol #42 (against main)

PR Description (summary): Adds a "City Organizers" → /organizers link to the /meetup footer nav, styled to match its siblings; responsive layout preserved via the existing flex-wrap. Changed file lints clean and compiles; the PR transparently notes that next build's lint step fails on pre-existing, unrelated no-unescaped-entities errors.

Maintainer Feedback:

  • 2026-07-31 — Merged. The maintainer (FrankezeCode) merged the PR into main (merge commit cc4f749) and closed issue #41 as completed — no changes requested. The transparently-flagged pre-existing build lint errors were accepted as out of scope for this change.

Status: ✅ Merged — 2026-07-31 (PR #42)

Learnings & Reflections

Technical Skills Gained

  • Matching existing UI conventions — reading a component to copy its exact link/hover pattern and Tailwind classes so the addition is seamless.
  • Reasoning about responsive layout without a browser — recognizing flex-wrap guarantees clean reflow of an added nav item.
  • Isolating my change in a repo with pre-existing build failures — proving (via git) that a red build is not caused by my diff.

Challenges Overcome

  • Resisted scope creep: the tempting "fix" was to clean up the unrelated lint errors so the build goes green, but that would bloat a one-line footer PR. Surfacing them for the maintainer was the more honest, reviewable choice.

What I'd Do Differently Next Time

  • Run the repo's build on the untouched base first, so "is this failure mine?" is answered before I even start.

Resources Used
  • BetterDevOrg/Protocol #41 — the issue and acceptance criteria.
  • src/app/meetup/page.tsx — the footer <nav> and the sibling <Link> styling pattern I mirrored.
  • Next.js next/link + Tailwind — the routing component and utility classes used for the link and hover state.

Contribution [5]: [Stet — surface the API's error message in the typed content client]

Contribution Number: 5
Student: Darin Andoh-Mensah
Issue: jamiedavenport/stet #33
Status: 🔨 Phase III In Progress — PR #55 reviewed; requested refactor addressed, awaiting merge

Why I Chose This Issue

I wanted the last one to be backend/API work in TypeScript, which is my favourite stack right now. This issue is a clean slice of API-client design: the typed client for Stet's REST API (an oRPC contract) was throwing away the descriptive error bodies the server sends and surfacing a bare HTTP status instead. Fixing it meant reading a real API contract, understanding how errors travel over the wire, and improving developer experience at exactly the moment a typed client should be most helpful — a small change with real "make the tool trustworthy" value.

It also sits in a genuinely well-engineered monorepo (pnpm workspaces, strict typing, thorough tests), so it was a good place to practise matching a high bar: the change had to be typed, formatted, and covered like the code around it.

Understanding the Issue

Problem Description

In published/client/src/content.ts, createContentClient's request throws on any non-ok response using only the status code:

throw new Error(`Stet request ${path} failed with status ${response.status}.`);

But the API answers failures with a described error body: oRPC serializes a thrown error to JSON carrying a human message, and internal/api/src/contract.ts writes a specific one for each case — UNAUTHORIZED ("Authentication required. Pass an organization API key in the x-api-key header."), plus QUOTA_EXCEEDED, RATE_LIMITED, NOT_FOUND. None of that reached the developer.

Expected Behavior

On a failed request, read the body, pull out the message, and include it in the thrown error — so a revoked key, an exhausted quota, and a misspelled slug each say what actually happened. Fall back to the status-only message when there's nothing usable to read.

Current Behavior

Every failure — auth, quota, rate-limit, not-found — surfaces as a bare number.

Affected Components

  • Changed: published/client/src/content.ts (the request path) and published/client/src/content.test.ts (new error-path tests).
  • Read-only reference: internal/api/src/contract.ts — the declared error messages (authErrors, rateLimitError) whose message fields the client now surfaces.

Reproduction Process

Environment Setup

  • Toolchain: Node + pnpm workspace using the catalog: protocol; the @stetcms/client package tests run via vp test run (vitest under vite-plus), typecheck via tsc --noEmit, and format/lint via vp check.
  • Scoped install: pnpm install --filter "@stetcms/client..." — installs just the client package and its workspace deps rather than the whole heavy monorepo.
  • Baseline: pnpm --filter @stetcms/client test → 14 passed before any change.

Reproduction Evidence

  • Fork: DAmensah27/stet
  • Findings: the throw site discards response's body entirely; the contract declares rich messages right there in contract.ts, so the fix is purely on the client — read the body it already receives.

Solution Approach

Analysis

The server already sends everything needed; the client just never reads it. The fix is a small, well-contained addition to the error branch of request, plus a helper that safely extracts a message from an unknown body without assuming a shape (the body could be a proxy's HTML error page).

Proposed Solution / Plan (UMPIRE, adapted)

Understand: the typed client drops the API's descriptive error body and shows only a status code. Match: the file's existing style — module-level helper functions (assetUrl, resolveAssetUrls) with heavy doc comments; tests use a stubFetch helper. Plan: (1) scoped install + green baseline; (2) add an errorMessage(response) helper that returns the body's message string or undefined; (3) append it to the thrown error in request, falling back to the status-only text; (4) extend content.test.ts with an errorFetch helper + four cases; (5) run tests, tc, and vp check. Implement: fix/client-surface-api-error-message — commit 4dfe916. Review: typed (no any), formatted, and covered to match the surrounding code. Evaluate: 18 tests pass; tsc and vp check clean. (Details below.)

Testing Strategy

Unit Tests — published/client/src/content.test.ts (4 new cases)

  • Message surfaced on get() — a 401 body { code: 'UNAUTHORIZED', message: '…' } throws an error whose message contains both 401 and the API's text.
  • Message surfaced on list() — a 429 RATE_LIMITED body's message reaches the thrown error from the collection-list path too.
  • Fallback for a non-JSON body — a 502 with an HTML body throws the status-only message without crashing.
  • Fallback for a body with no message — a 404 { code: 'NOT_FOUND' } falls back to exactly …failed with status 404..

Uses a new errorFetch(status, body, kind) helper mirroring the existing stubFetch pattern — no network involved.

Manual Testing / Validation

  • pnpm --filter @stetcms/client test → 18 passed (14 existing + 4 new), no regressions.
  • pnpm --filter @stetcms/client tc (tsc --noEmit) → clean.
  • vp check (client package) → all files correctly formatted; no lint or type errors.

Implementation Notes

Progress Summary

Added a typed errorMessage helper and wired it into request's error branch (append the API message; fall back to status-only). Extended the test suite with four error-path cases. Verified with the package's tests, typecheck, and vp check; committed on fix/client-surface-api-error-message (4dfe916) and opened PR #55.

Challenges Faced

  • A catalog:/workspace: pnpm monorepo. First time working in one; learned to do a scoped install (--filter "@stetcms/client...") so I didn't have to build the entire app (web, drizzle, playwright, …) just to touch one published package.
  • Parsing an unknown body safely. The error body isn't guaranteed to be JSON (a gateway could return HTML), so the helper try/catches response.json() and narrows the shape before reading message, returning undefined to trigger the fallback rather than throwing a second error.

Code Changes

  • Files: published/client/src/content.ts (+errorMessage helper, error branch surfaces it) and published/client/src/content.test.ts (+errorFetch + 4 cases).
  • Branch: fix/client-surface-api-error-message — commit 4dfe916.
  • Approach decision: module-level helper with a doc comment (matching assetUrl/resolveAssetUrls) rather than an inline block, so the error-parsing intent is documented and testable in isolation.

Pull Request

PR Link: jamiedavenport/stet #55 (against main)

PR Description (summary): request now reads the failed response body and appends the API's message to the thrown error (falling back to the status-only message for empty/non-JSON/message-less bodies), turning bare status codes into descriptive errors. Adds an errorMessage helper and four error-path tests. 18 tests pass; tsc and vp check clean.

Maintainer Feedback:

  • 2026-07-31 — Refactor requested (on the errorMessage helper). The reviewer pointed me at oRPC's own createORPCErrorFromJson and isORPCErrorJson: rather than hand-reading .message off the body, use the library's canonical helpers.
  • 2026-07-31 — Addressed. Replaced the manual shape check with isORPCErrorJson(body) ? createORPCErrorFromJson(body).message : undefined (from @orpc/client, already a direct dependency). This validates the full oRPC error shape (defined/code/status/message, no stray keys) and reuses oRPC's code-based message fallback instead of reimplementing it. Updated the tests to the real oRPC error wire shape (added an orpcError helper) and made the fallback case cover a JSON body that isn't an oRPC error. 18 tests pass; tsc and vp check clean; pushed to the PR branch and replied on the thread.

Status: Refactor requested → addressed; awaiting merge

Learnings & Reflections

Technical Skills Gained

  • Using a library's own error helpers instead of reinventing them. The review taught me to reach for oRPC's isORPCErrorJson / createORPCErrorFromJson rather than hand-parsing the error body — the shape validation and code-based message fallback then stay in lockstep with the server instead of drifting.
  • Reading an API contract to improve a client. Tracing how oRPC error messages declared in contract.ts travel over HTTP and surfacing them where a developer actually sees them.
  • Defensive parsing of an unknown response body — try/catch around .json() plus shape-narrowing, with a clean undefined fallback instead of a second throw.
  • Working in a pnpm catalog: monorepo — scoped installs and per-package test / tc / check scripts (vite-plus).

Challenges Overcome

  • Kept the change strictly typed and formatted to a high-bar codebase's standards, verified by the repo's own vp check rather than guessing at style.

What I'd Do Differently Next Time

  • Nothing major — scoping the install early and leaning on the repo's own check/test scripts made this smooth; I'll reuse that "find the package's own scripts first" habit in future monorepos.

Resources Used
  • jamiedavenport/stet #33 — the issue and the exact throw site it names.
  • published/client/src/content.ts + content.test.ts — the code changed and the stubFetch test pattern mirrored.
  • internal/api/src/contract.ts — the declared error messages (UNAUTHORIZED, QUOTA_EXCEEDED, RATE_LIMITED, NOT_FOUND) the client now surfaces.
  • pnpm workspaces + vite-plus — scoped --filter installs and the vp test / vp check tooling used to verify.

About

No description, website, or topics provided.

Resources

Readme
Activity

Stars

0 stars

Watchers

0 watching

Forks

0 forks
Report repository

Releases

Packages

Contributors