Skip to content

Feat configurable agent as lib#6

Open
valdis wants to merge 7 commits into
mainfrom
feat-configurable-agent-as-lib
Open

Feat configurable agent as lib#6
valdis wants to merge 7 commits into
mainfrom
feat-configurable-agent-as-lib

Conversation

@valdis

@valdis valdis commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Allows configurable agent to be added as a package dependency to other projects

valdis added 4 commits June 11, 2026 18:15
…t usage

- Rename package to @eggai/configurable-agent and add exports["./lib"]
- Add src/lib/index.ts as the public library surface (runAgent, prepareMessages, types)
- Enable declaration emit in tsconfig.build.json for TypeScript consumers
- Add openai-compatible provider to ModelProvider enum and buildModel()
- Add apiKey field to model config schema
- Accumulate inputTokens/outputTokens across steps and emit in final event
- Add @vitest/coverage-v8 for coverage reporting
…nsumers

- Add "default" condition to ./lib exports map so CJS loaders (qualops)
  can resolve the package without ERR_PACKAGE_PATH_NOT_EXPORTED
- Re-export jsonSchema from 'ai' via lib/index.ts so consumers don't
  need a direct ai dependency to wrap plain JSON Schema objects
Captures x-ratelimit-* headers from the finish-step stream part and emits
a rate_limit_tokens error event when remaining tokens fall below 5% of the
limit. Passes any partial response text as partialContent so callers can
attempt recovery. Also propagates partial content on stream_error.
Extract step-completion error detection into a pure exported function
`diagnoseStep` so new error conditions can be added in one place without
touching the loop control flow.

Add `finishReason === 'length'` detection as `max_tokens_reached` error
alongside the existing TPM rate-limit check. Both conditions attach
`partialContent` when text was accumulated before the failure so callers
can attempt partial recovery.

Restructure tests: five fast `diagnoseStep` unit tests (no mock model)
plus three integration tests that exercise the full loop path.
@github-actions

github-actions Bot commented Jun 12, 2026

Copy link
Copy Markdown

QualOps Code Quality Analysis

Status: ✅ PASSED - No issues found

Summary

  • Total Issues: 0
  • Critical: 0 🔴
  • High: 0 🟠
  • Medium: 0 🟡
  • Low: 0 🟢
  • Files Analyzed: 0

No issues found in the analyzed code.

📊 Full Report

View detailed report


Powered by QualOps

@valdis valdis force-pushed the feat-configurable-agent-as-lib branch from 5588f9a to 64e57d4 Compare June 12, 2026 10:54
@valdis valdis force-pushed the feat-configurable-agent-as-lib branch from 9eae18e to 14631e0 Compare June 12, 2026 11:07
- Remove private:true to allow npm publishing
- Add npm-publish.yml: provenance-based publish (no token needed) triggered
  by merging release/vX.Y.Z branches, matching qualops release process
- Add create-release-pr.yml: bumps version, updates CHANGELOG, opens release PR
- Add changelog job to CI: required for PRs touching src/, tests/, package.json
- Add CHANGELOG.md with initial 0.1.0 entry
Comment thread src/agent/events.ts
stopReason: string;
steps: number;
truncated: boolean;
usage?: { inputTokens: number; outputTokens: number };

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.

Great idea. Should this be a mandatory field?

Comment thread src/config/schema.ts
provider: ModelProvider,
name: z.string().min(1),
baseUrl: z.string().url().optional(),
apiKey: z.string().optional(),

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.

I don't think the api key should be part of the config, because that would make the whole config a secret.

Comment thread package.json
"@biomejs/biome": "^1.9.0",
"@types/node": "^22.10.0",
"@types/shell-quote": "^1.7.5",
"@vitest/coverage-v8": "^4.1.4",

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.

👍

@sebastianwessel

Copy link
Copy Markdown

Review: provider-specific handling in diagnoseStep

My main concern is that diagnoseStep (src/agent/loop.ts) is OpenAI-only by construction, even though it lives in the provider-agnostic loop.

Core problem: hard-coded OpenAI headers

It reads only OpenAI's header names:

ctx.responseHeaders['x-ratelimit-remaining-tokens']
ctx.responseHeaders['x-ratelimit-limit-tokens']
ctx.responseHeaders['x-ratelimit-reset-tokens']

But buildModel supports five providers. For three of them the rate-limit branch silently never fires:

Provider Token rate-limit headers Result
OpenAI x-ratelimit-{limit,remaining,reset}-tokens works
Anthropic anthropic-ratelimit-tokens-{limit,remaining,reset} (+ separate input-tokens/output-tokens buckets); reset is an RFC-3339 timestamp, not "59s" never matches
Google none (429 + RetryInfo) never matches
Ollama none never matches
openai-compatible (Mistral/Groq/Together/…) header names vary per backend mostly never matches

diagnoseStep isn't even passed the provider, so it can't branch. The message text is OpenAI-semantics-specific too: "input consumed X/Y tokens, leaving only Z for output" assumes OpenAI's single TPM bucket — Anthropic tracks input and output limits separately, so that framing is wrong there, and resets in ${resetTokens} would print a raw ISO timestamp.

Correctness issue (provider-independent)

Even on OpenAI, the rate-limit branch fires at the wrong time and severity. The headers reflect remaining budget after a successful response. So when remaining/limit < 0.05, the loop emits an error and returns — aborting the loop and discarding a completed, successful step. Being near the TPM ceiling isn't a failure of this request; it's a warning about the next one. The added integration test encodes this: a successful stream (finishReason: stop) gets turned into a fatal rate_limit_tokens error. The 5% threshold is also arbitrary and a false-positive risk for small buckets.

The provider-agnostic place to detect rate limiting is the actual 429, which the AI SDK surfaces uniformly as APICallError (.statusCode, .responseHeaders, .isRetryable). Right now it just falls into the catch as agent_failed.

What's good

  • finishReason === 'length'max_tokens_reached is correct and genuinely provider-agnostic (the SDK normalizes every provider's stop reason). Keep this.
  • partialContent on stream errors and usage accumulation are provider-neutral — good additions.
  • openai-compatible provider + apiKey schema are reasonable. Minor: requiredEnvVarFor returns null so a key-requiring endpoint gets no validation, and apiKey in the config file invites plaintext secrets (env fallback already exists).

Suggested direction

  1. Preferred: drop proactive header parsing for rate limits; keep the finishReason==='length' diagnosis; handle real rate limits by catching APICallError/429 from the SDK and emitting a typed rate_limit error there — uniform across all five providers, fires only on actual failure.
  2. If you want pre-emptive "budget low" signaling: thread config.model.provider into diagnoseStep, add a per-provider header map (incl. Anthropic's two-bucket scheme), normalize the reset format, and emit it as a non-fatal warning that doesn't abort a successful step.
  3. At minimum, name it for what it is (OpenAI) so it doesn't read as general rate-limit handling.

The tests would have caught this if any rate-limit case used Anthropic-shaped headers; currently they all use the OpenAI names, so the gap is invisible in CI.

@valdis valdis force-pushed the feat-configurable-agent-as-lib branch from 27682b3 to fb8aa9c Compare June 12, 2026 12:16
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