Skip to content

feat(llm): add ChatGPT/Codex subscription provider - #1806

Open
acoliver wants to merge 11 commits into
kodustech:mainfrom
acoliver:feat/codex-subscription-auth
Open

feat(llm): add ChatGPT/Codex subscription provider#1806
acoliver wants to merge 11 commits into
kodustech:mainfrom
acoliver:feat/codex-subscription-auth

Conversation

@acoliver

@acoliver acoliver commented Aug 28, 2026

Copy link
Copy Markdown

Summary

Wires up the ChatGPT/Codex subscription provider that already existed in
libs/llm/codex-subscription-model.ts but had no product caller, and fixes a bug in its
transport that only shows up on multi-turn agent loops. Fixes #1802.

Backend only. Nothing under apps/web/ or any ee/ path is touched, per the scope
question raised in the issue.

The bug worth reading about

reasoningEncryptedContent does not ride on reasoning-delta. In @ai-sdk/openai it is
attached to the providerMetadata of reasoning-start and reasoning-end, while
reasoning-delta carries only itemId.

The existing reassembly handled reasoning-delta only and rebuilt reasoning with no
providerMetadata. Under store: false the SDK then filters out reasoning items whose
encrypted_content is null, emitting exactly the warning the file's own comment says it
is avoiding. So include: ['reasoning.encrypted_content'] bought nothing on the
doGenerate path, and the model re-derived its reasoning at every step of a 30-60 call
review.

The eval harness could not have caught this: a single-step call has nothing to
round-trip. The fix carries metadata from both parts through, and is proven two ways:
a mocked two-step test asserting the ciphertext appears in the second request body, and
an opt-in live test that issues real requests, since only the live endpoint proves the
replayed content is actually accepted.

Two smaller defects in the same function: text fragments were emitted in reverse stream
order by unshift, and the response-metadata part was spread wholesale including its
type field. The Proxy is replaced with an explicit LanguageModel object, which also
removes an unverifiable question about Reflect.get with a Proxy receiver under
wrapLanguageModel.

The endpoint contract, measured

Property Result
stream omitted or false 400 "Stream must be set to true"
store: true or omitted 400 "Store must be set to false"
temperature / max_output_tokens 400 "Unsupported parameter"
reasoning: {effort:"high"} + include:[reasoning.encrypted_content] 200
text.format json_schema 200
function tools 200, emits function_call
Refresh 200, expires_in=864000 (10 days), refresh token rotates

Models are account-scoped: which models a ChatGPT account is served depends on its plan
and changes over time. gpt-5.6-sol, gpt-5.6-luna and gpt-5.6-terra all return 200
on a Pro account; gpt-5.6-codex and gpt-5.1-codex are refused on the accounts
available to me. The Codex backend exposes no model-list endpoint, so the provider's
listing is a convenience picker rather than an account inventory, and a model absent
from it is still usable if typed in. capabilities() reflects
these measurements rather than assumptions.

Approach

Provider module at libs/llm/providers/codex/, id chatgpt_subscription.
build() reads auth lazily, because the conformance harness calls it with no
credentials. No catalog is shipped and reasoningTraits is provided, so the module
compiles both on main and after #1800.

Credentials follow the Bedrock precedent for non-apiKey auth: encrypted BYOK
settings, with hasAuth and the slot field map extended so a token-only credential does
not silently degrade to the managed default.

Refresh persists the rotated token before using the new access token, because the
server invalidates the old one the moment it responds. The write is a compare-and-swap on
a plain @Injectable() service so a worker can call it, rather than the request-scoped
use case. A thrown repository error retries with bounded backoff instead of discarding
the newly-issued token, which would otherwise destroy the credential permanently.

Container reality. readCodexAuth reads os.homedir(), but the worker runs as
USER 1000 in node:22-slim so that resolves to /home/node, and the installer compose
gives the worker no host bind mount at all. The primary path is therefore the normal
encrypted credential, which works for remote and multi-host installs.
API_CODEX_AUTH_FILE is an explicit opt-in override for the bind-mount case, never an
implicit fallback.

Cost. A subscription has no per-token price, so usage is reported for observability
but contributes zero to spend rather than inflating it with fictional money. The Mongo
backfill aggregation mirrors the same prefix exception, since three migrations invoke it
and the next one would otherwise re-price every span at API rates.

Scope question

uiFields is not rendered anywhere; its only consumer reduces it to three booleans.
Bedrock's credential form reaches the screen through hand-maintained wiring under
apps/web/src/features/ee/byok/, which is Enterprise-licensed and which #1800 also
rewrites. So this PR is backend only and credentials are seeded through the existing
admin API. Happy to follow up with the UI, or to rebase onto #1800 and land it together,
whichever you prefer.

Test plan

  • pnpm run test: 7103 tests pass across 735 suites
  • scripts/typecheck-libs-gate.sh clean (0 violations)
  • prettier --check clean on all changed files
  • pnpm run env:apply produces no drift
  • Two-step test asserts encrypted_content reaches the second request body
  • Opt-in live test (CODEX_LIVE=1) verifies both a single turn and a multi-step tool
    turn against the real endpoint, driven on gpt-5.6-sol with a credential produced
    by a real OAuth sign-in; skips without credentials so it cannot affect CI
  • registry.spec.ts five hardcoded assertions updated, CONFORMANCE_SAMPLES entry
    added
  • build() asserted to succeed with no credentials present
  • Rotation: persist-before-use ordering, CAS retry, and bounded retry on a thrown
    repository error
  • Cost test asserts zero spend contribution and that pricing is never consulted
  • Backfill parity test covers the subscription prefix

Failures unrelated to this branch: distributed-lock.service.integration.spec.ts and
create-or-update-config-race.integration.spec.ts both require a live Postgres, and this
branch touches neither.

Review notes

This branch was reviewed by both Kodus and Open Code Review before submission; findings
from each are summarised in a comment below.

Kodus could not run reviews billed to a ChatGPT subscription. The transport
existed at libs/llm/codex-subscription-model.ts but had no product caller,
no registry entry and no credential path, so LLM.run could never reach it.

The transport also had a bug that only appears on multi-turn loops.
reasoningEncryptedContent rides on the reasoning-start and reasoning-end
parts, not reasoning-delta, so the reassembly rebuilt reasoning with no
providerMetadata and the SDK filtered those items out of the next request.
The include: ['reasoning.encrypted_content'] therefore bought nothing and
the model re-derived its reasoning on every step of a 30-60 call review. A
single-step eval call has nothing to round-trip, which is why this was not
visible before. Metadata from both parts is now carried through, proven by
a two-step test asserting encrypted_content appears in the second request.

Also in the transport: text fragments were emitted in reverse stream order
by unshift, and the response-metadata part was spread wholesale including
its type field. The Proxy is replaced with an explicit LanguageModel object,
which removes a receiver question under wrapLanguageModel.

The provider module registers as chatgpt_subscription with capabilities
measured against the live endpoint: json_schema structured output, native
tool calling, no temperature support, streaming required. Models are
account-scoped, so only gpt-5.6-luna and gpt-5.6-terra are listed; the other
gpt-5.6 and codex ids are rejected for ChatGPT accounts. No catalog is
shipped, keeping the module compatible with kodustech#1800.

Credentials follow the Bedrock precedent for non-apiKey auth, stored as
encrypted BYOK settings. Refresh persists the rotated refresh token before
using the new access token, because the server invalidates the old one on
response, and writes through a compare-and-swap on a non-request-scoped
service so a worker can call it and a concurrent save cannot clobber it.

A subscription has no per-token price, so usage is reported but contributes
zero to spend rather than inflating it with fictional cost.

Refs kodustech#1802
…on safety

Review found four defects in the initial implementation.

The connection test fabricated an Axios-shaped object and passed it to
normalizeError, but axios.isAxiosError requires isAxiosError === true, so
the object failed the check and fell through to the unknown-error tail.
Every Codex failure reported code 'unknown' with the status and provider
message dropped, and logged a spurious warning. An expired access token,
the most likely failure for a credential that rotates every ten days, told
the user nothing. The status ladder is now a separate classifyHttpStatus
that both the Axios path and the Codex path call directly.

The Mongo backfill still derived the canonical model as the last
':'-segment while deriveTu was changed to preserve the subscription prefix.
token-usage-tu.ts asks for the two to be kept in sync, and three migrations
invoke the backfill, so the next one would have re-derived every span and
priced included ChatGPT usage at API rates. The aggregation now mirrors the
prefix exception and the parity test covers it.

The credential store was installed from the service constructor, mutating a
module global in another library. That held only while Nest eagerly
instantiated the provider, and would have failed as a refresh error ten
days later rather than at boot. Registration moved to onModuleInit with a
matching teardown. Rotation also resolved the credential by id with no
organization scope, which is now passed through the slot.

Finally, a persist failure during rotation discarded the new refresh token
after the server had already invalidated the old one, leaving nothing
usable and requiring a manual re-login. Repository errors now retry with
bounded backoff and the terminal error names the recovery action.

Refs kodustech#1802
The two-step reasoning test uses a mocked transport, which proves our own
reassembly carries the metadata but not that the endpoint accepts the
replayed encrypted_content. This spec issues real requests so the
round-trip is verified against the live contract.

Skips unless CODEX_LIVE=1 and a Codex credential is present, so it cannot
affect CI or a contributor run.

Refs kodustech#1802
@kody-ai

kody-ai Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Code Review Completed! 🔥

The code review was successfully completed based on your current configurations.

Kody Guide: Usage and Configuration
Interacting with Kody
  • Request a Review: Ask Kody to review your PR manually by adding a comment with the `@kody start-review` command at the root of your PR.

  • Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Current Kody Configuration
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug
Performance
Security
Business Logic

Kody Code Review — 6 suggested fixes.
Paste the prompt below to your agent and all review fixed at once!

🛠️ Open Agent Prompt
A code review identified the following issues in this pull request.
Each section describes what was found and includes a reference implementation where available.

Files involved:
- libs/llm/codex-subscription-model.ts:344
- libs/llm/codex-subscription-model.ts:236
- libs/organization/application/use-cases/organizationParameters/test-byok-connection.use-case.ts:315
- libs/organization/infrastructure/adapters/repositories/organizationParameters.repository.ts:295
- libs/organization/infrastructure/adapters/repositories/organizationParameters.repository.ts:306
- libs/organization/infrastructure/adapters/services/organizationParameters.service.ts:195

---

### [1/6] libs/llm/codex-subscription-model.ts:344
Issue identified during code review:
The new doGenerate reassembly iterates textByFragment in insertion order and pushes reasoning blocks before the text answer, inverting the prior contract that content[0] is the response text. Reassemble text fragments before reasoning fragments (text first, then reasoning) to preserve the ordering, while still attaching providerMetadata to each reasoning part.
Reference implementation (from code review):

// libs/llm/codex-subscription-model.ts:344
for (const [key, text] of textByFragment) {
    if (!key.startsWith('reasoning:') && text) content.push({ type: 'text', text });
}
for (const [key, text] of textByFragment) {
    if (key.startsWith('reasoning:')) {
        const providerMetadata = metadataByFragment.get(key);
        if (!text && !providerMetadata) continue;
        content.push({ type: 'reasoning', text, ...(providerMetadata ? { providerMetadata } : {}) });
    }
}

---

### [2/6] libs/llm/codex-subscription-model.ts:236
Issue identified during code review:
The `doStream` call in `withGenerateFromStream` directly invokes the underlying model without an observability span; wrap it using `runAiSdkLLMInSpan` with a stable runName and include `organizationId` in the attributes to track token usage.
Reference implementation (from code review):

// libs/llm/codex-subscription-model.ts:236
): PromiseLike<CodexStreamResult> => runAiSdkLLMInSpan({
        spanName: 'codex-subscription.stream',
        runName: 'codex-subscription.stream',
        model: model.modelId,
        attrs: { organizationId: someOrgId },
        exec: () => model.doStream(options),
    });

---

### [3/6] libs/organization/application/use-cases/organizationParameters/test-byok-connection.use-case.ts:315
Issue identified during code review:
testCodexSubscription's catch only classifies axios errors, so fetch timeouts or DNS failures fall through to a generic `code:'unknown'` response instead of the intended `code:'network'` timeout guidance. Add an axios-independent branch that detects `err.name === 'TimeoutError'` / `AbortError` and returns a network/timeout classification consistent with the axios `ECONNABORTED` path.
Reference implementation (from code review):

// libs/organization/application/use-cases/organizationParameters/test-byok-connection.use-case.ts:315
} catch (error) {
            const status = ...;
            if (error instanceof Error && (error.name === 'TimeoutError' || error.name === 'AbortError')) {
                return { ok: false, code: 'network', latencyMs, message: `The request timed out after ${TEST_TIMEOUT_MS}ms. The provider may be slow or unreachable from this deployment — retry or check outbound network.` };
            }
            return this.normalizeError(error, Date.now() - start);
        }

---

### [4/6] libs/organization/infrastructure/adapters/repositories/organizationParameters.repository.ts:295
Issue identified during code review:
The refactored method removed its try-catch, so any DB error now propagates unlogged, losing debugging information. Wrap the method logic in a try-catch, log errors via `this.logger.error` with metadata (configKey, configValue, organizationAndTeamData, fuzzy), and rethrow the original error.
Reference implementation (from code review):

// libs/organization/infrastructure/adapters/repositories/organizationParameters.repository.ts:295
try {
            const { configKey, configValue, organizationAndTeamData, fuzzy } =
                filter;

            const queryBuilder =
                this.organizationParametersRepository.createQueryBuilder(
                    'organizationParameters',
                );

            queryBuilder.leftJoinAndSelect(
                'organizationParameters.organization',
                'organization',
            );

            queryBuilder.where('organizationParameters.configKey = :configKey', {
                configKey,
            });

            if (organizationAndTeamData) {
                queryBuilder.andWhere(
                    'organizationParameters.organization_id = :organizationId',
                    {
                        organizationId: organizationAndTeamData.organizationId,
                    },
                );
            }

            if (fuzzy) {
                queryBuilder.andWhere(
                    'organizationParameters.configValue @> :configValue',
                    { configValue: JSON.stringify(configValue) },
                );
            } else {
                queryBuilder.andWhere(
                    'organizationParameters.configValue = :configValue::jsonb',
                    { configValue: JSON.stringify(configValue) },
                );
            }

            const retrievedParameters = await queryBuilder.getMany();

            if (!retrievedParameters || retrievedParameters.length === 0) {
                return [];
            }

            return mapSimpleModelsToEntities(
                retrievedParameters,
                OrganizationParametersEntity,
            );
        } catch (error) {
            this.logger.error({
                message: `Failed to retrieve organization parameters`,
                context: OrganizationParametersRepository.name,
                error: error.message,
                metadata: { configKey, configValue, organizationAndTeamData, fuzzy },
            });
            throw error;
        }

---

### [5/6] libs/organization/infrastructure/adapters/repositories/organizationParameters.repository.ts:306
Issue identified during code review:
The query uses `leftJoinAndSelect`, pulling all columns from the joined organization table, which wastes bandwidth and memory. Use `leftJoin` instead and add `.addSelect()` for only the specific fields needed (e.g., `organization.id`).
Reference implementation (from code review):

// libs/organization/infrastructure/adapters/repositories/organizationParameters.repository.ts:306
queryBuilder.leftJoin('organizationParameters.organization', 'organization');
        // Optionally add specific fields: .addSelect(['organization.id']);

---

### [6/6] libs/organization/infrastructure/adapters/services/organizationParameters.service.ts:195
Issue identified during code review:
Prefer building a Map of credentials keyed by id before the loop to replace O(n) `find` calls with constant-time lookups, then retrieve via `credentialMap.get(input.credentialId)`.
Reference implementation (from code review):

// libs/organization/infrastructure/adapters/services/organizationParameters.service.ts:195
const credentialMap = new Map(current.credentials.map(c => [c.id, c]));
const credential = credentialMap.get(input.credentialId);

---

Review each issue in context, use the reference implementations as guidance, and apply fixes that are consistent with the surrounding codebase.

Access your configuration settings here.

@acoliver

Copy link
Copy Markdown
Author

What the review tools found in this branch

This branch was reviewed by Kodus and by Open Code Review before submission. Recording
what each found, including where they were wrong, since it is relevant to judging the
change.

Kodus, reviewing itself

Kodus reviewed this branch with kodus review --agent: 41 files analysed, three
findings, none of which survived checking.

Two of them, sharing one root cause, claimed compareAndSwapConfigValue is key-order
sensitive because it builds its comparand with JSON.stringify, and so could spuriously
fail against PostgreSQL's order-insensitive JSONB equality. That would be a real defect
in exactly the mechanism protecting the credential from being lost during rotation, so it
was worth taking seriously.

The SQL is "configValue" = :expected::jsonb. The cast sits on the same line as the
JSON.stringify the finding objected to, and it is what makes the comparison
jsonb-to-jsonb. Checked against a live PostgreSQL rather than reasoned about:

SELECT '{"a":1,"b":{"x":1,"y":2}}'::jsonb = '{"b":{"y":2,"x":1},"a":1}'::jsonb;
 t

The third asked for path validation on API_CODEX_AUTH_FILE, on the grounds that an
attacker controlling environment variables could point it at an arbitrary file.
Declined: an attacker who can set environment variables in the worker container already
has code execution, so the check defends a position that is already lost.

Kodus's low finding count here is consistent with how it behaves generally. In a separate
ten-PR comparison it averaged about two findings per pull request against OCR's eight,
with a slightly lower false-positive rate. It says little, and what it says is usually
about runtime consequences rather than style or tests.

Open Code Review, reviewing the companion branch

OCR reviewed the parallel change in alibaba/open-code-review#1106 across two passes and
produced 20 findings, 11 of which were real and were fixed. Among them: a loopback OAuth
callback that any local process could use to abort a legitimate login, a device-code
polling loop that spun when the server omitted an interval, and an OAuth refresh that
discarded a still-valid token when the response omitted an optional field.

It was also confidently wrong three times, including one high-severity claim that Go's
os.Rename cannot replace an existing file on Windows. It can, via
MoveFileEx(..., MOVEFILE_REPLACE_EXISTING). And on its second pass it re-raised three
findings that the first pass had already been shown to be wrong about, which is what a
stateless reviewer does.

On the reasoning fix in this PR

Neither tool found the reasoningEncryptedContent bug that this PR fixes. It was found
by reading @ai-sdk/openai's stream-part handling directly, and it is invisible to any
review that does not cross-reference the SDK's own request-side filtering. The mocked
two-step test and the opt-in live test were both written specifically to make it
regression-proof, since nothing in the existing suite would have caught it.

@kody-ai

kody-ai Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

kody code-review Business Logic medium

🤔 Insufficient Task Context

I found a task linked to this PR, but it only contains minimal information (title only, no description or acceptance criteria). To perform a meaningful business rules validation, I need more details.

🔍 What I need to validate:

  • Business requirements and acceptance criteria
  • Expected behavior and business rules
  • Edge cases and constraints to consider

💡 How to improve the task context:

  • Add a description to the linked ticket
  • Include acceptance criteria or business rules
  • Describe the expected behavior after the change

⚠️ Important:

A task title alone is not sufficient to determine whether the implementation is correct or complete.

Comment thread libs/llm/codex-subscription-model.ts
Comment thread libs/llm/codex-subscription-model.ts

@kody-ai kody-ai Bot left a comment

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.

The picker listed two models, derived from a probe run against an account
that was not entitled to a third. gpt-5.6-sol is served to entitled accounts
and is now listed.

The listing is documented as a convenience picker rather than an account
inventory. The Codex backend exposes no model-list endpoint, so it cannot be
dynamic, and what a ChatGPT account can run depends on its plan.

The live spec now accepts a credential path and a model, so it can be driven
against a credential produced by an OAuth login rather than only the Codex
CLI's own file.

Refs kodustech#1802
@kody-ai

kody-ai Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Code Review Completed! 🔥

The code review was successfully completed based on your current configurations.

Kody Guide: Usage and Configuration
Interacting with Kody
  • Request a Review: Ask Kody to review your PR manually by adding a comment with the `@kody start-review` command at the root of your PR.

  • Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Current Kody Configuration
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug
Performance
Security
Business Logic

Access your configuration settings here.

@kody-ai

kody-ai Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

kody code-review Business Logic medium

🤔 Insufficient Task Context

I found a task linked to this PR, but it only contains minimal information (title only, no description or acceptance criteria). To perform a meaningful business rules validation, I need more details.

🔍 What I need to validate:

  • Business requirements and acceptance criteria
  • Expected behavior and business rules
  • Edge cases and constraints to consider

💡 How to improve the task context:

  • Add a description to the linked ticket
  • Include acceptance criteria or business rules
  • Describe the expected behavior after the change

⚠️ Important:

A task title alone is not sufficient to determine whether the implementation is correct or complete.

The live tests injected tokens straight into build(), so readCodexAuth and
API_CODEX_AUTH_FILE were never exercised. This builds with no tokens passed,
forcing the provider to locate and parse the credential itself.

Refs kodustech#1802
@acoliver

Copy link
Copy Markdown
Author

What is verified here, and what is not

Stating this precisely, because an earlier note in this PR said "verified live" in a way
that implied more coverage than exists.

Verified against the live endpoint

Three opt-in tests (CODEX_LIVE=1) issue real requests:

  • a single turn on the configured model, run on gpt-5.6-sol
  • a multi-step tool turn, which is the only way to prove the replayed
    encrypted_content is accepted; under store: false the endpoint drops or rejects
    reasoning that arrives without it, so a successful second step is the evidence
  • resolution through readCodexAuth / API_CODEX_AUTH_FILE with no tokens passed to
    build(), so the provider has to locate and parse the credential itself

The first two inject a credential directly. That exercises the model and the transport.
It does not exercise how a credential reaches the provider in production.

Not verified

The path that actually ships is untested end to end:

encrypted BYOK settings in Postgres
  -> resolveAgentModel stamps the slot
  -> worker builds the model
  -> AgentReviewStage runs the review
  -> rotation persists the new refresh token back through the compare-and-swap

Each piece has unit coverage, including the rotation ordering, the organization scoping
of the repository query, and the bounded retry on a persist failure. What has not been
run is the whole chain against a live stack, because doing so requires rebuilding the
worker and api images from this branch.

The rotation path is the part I would most want exercised before this merges. It only
fires when an access token ages past its window, the server invalidates the old refresh
token the moment it responds, and a failure there costs the user their credential. The
unit tests cover the ordering and the retry, but they mock the repository.

On credential provenance

Kodus has no sign-in flow, by design: it is BYOK and an administrator supplies the
tokens. For these tests the tokens came from a Codex CLI auth.json and from an OAuth
sign-in performed by the companion change in alibaba/open-code-review#1106. That is a
reasonable way to obtain a credential for testing, but it means nothing in this PR
produces one.

@kody-ai

kody-ai Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Code Review Completed! 🔥

The code review was successfully completed based on your current configurations.

Kody Guide: Usage and Configuration
Interacting with Kody
  • Request a Review: Ask Kody to review your PR manually by adding a comment with the `@kody start-review` command at the root of your PR.

  • Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Current Kody Configuration
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug
Performance
Security
Business Logic

Kody Code Review — 1 suggested fix.
Paste the prompt below to your agent and all review fixed at once!

🛠️ Open Agent Prompt
A code review identified the following issues in this pull request.
Each section describes what was found and includes a reference implementation where available.

Files involved:
- libs/llm/codex-live.integration.spec.ts:96

---

### [1/1] libs/llm/codex-live.integration.spec.ts:96
Issue identified during code review:
In codex-live.integration.spec.ts, wrap the bare generateText call in runAiSdkLLMInSpan to ensure token usage is accounted for, providing a stable runName and organizationId in the attrs.
Reference implementation (from code review):

// libs/llm/codex-live.integration.spec.ts:96
const result = await this.observabilityService.runAiSdkLLMInSpan({
    spanName: 'codex-live.integration',
    runName: 'codex-live.integration',
    model: model.modelId,
    attrs: { organizationId },
    exec: () => generateText({ model, prompt: 'Reply with exactly: OK' }),
});

---

Review each issue in context, use the reference implementations as guidance, and apply fixes that are consistent with the surrounding codebase.

Access your configuration settings here.

@kody-ai

kody-ai Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

kody code-review Business Logic medium

🤔 Insufficient Task Context

I found a task linked to this PR, but it only contains minimal information (title only, no description or acceptance criteria). To perform a meaningful business rules validation, I need more details.

🔍 What I need to validate:

  • Business requirements and acceptance criteria
  • Expected behavior and business rules
  • Edge cases and constraints to consider

💡 How to improve the task context:

  • Add a description to the linked ticket
  • Include acceptance criteria or business rules
  • Describe the expected behavior after the change

⚠️ Important:

A task title alone is not sufficient to determine whether the implementation is correct or complete.

Comment thread libs/llm/codex-live.integration.spec.ts
Rotation is the highest-consequence path in this provider and the one unit
tests cover least convincingly, because they mock the repository. The server
invalidates the old refresh token as soon as it issues a new one, so a persist
that fails, writes to the wrong tenant, or loses a concurrent race costs the
user their credential.

Four cases against a real database: a successful swap, a swap whose expected
value has different key ordering, a concurrent rotation where the loser must
not clobber the winner's token, and an organization-scoped lookup that must
not see another tenant's row.

The key-order case is the subject of a review finding claiming JSON.stringify
makes the comparison order-sensitive. The `:expected::jsonb` cast parses before
comparing, so it does not, and this now demonstrates that rather than arguing
it.

Refs kodustech#1802
@kody-ai

kody-ai Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Code Review Completed! 🔥

The code review was successfully completed based on your current configurations.

Kody Guide: Usage and Configuration
Interacting with Kody
  • Request a Review: Ask Kody to review your PR manually by adding a comment with the `@kody start-review` command at the root of your PR.

  • Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Current Kody Configuration
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug
Performance
Security
Business Logic

Access your configuration settings here.

@acoliver

Copy link
Copy Markdown
Author

Correction to the test plan above, and rotation now covered

Two updates, one of them a correction to something I stated in this PR.

The reported failures were mine, not the repository's

This PR said the only failing tests were distributed-lock.service.integration.spec.ts
and create-or-update-config-race.integration.spec.ts, both requiring "a real Postgres"
and both unrelated to this branch. The second half of that was true and the first half
was wrong in a way that mattered: they were not failing because Postgres was absent, they
were failing because I was pointing them at the wrong port. The container publishes 5432
on host port 51573, and the specs default to 5432.

With the correct connection settings, both pass, and so does everything else:

Test Suites: 10 skipped, 737 passed, 737 of 747 total
Tests:       61 skipped, 7113 passed, 7174 total

Zero failures. Please read the earlier "8 pre-existing failures" line as withdrawn.

Rotation is now exercised against a real database

Rotation was the part of this change I was least comfortable shipping on unit tests
alone, because those mock the repository, and the failure mode is not a wrong result but
a destroyed credential: the server invalidates the old refresh token the moment it issues
a new one.

test/integration/byok/codex-rotation.integration.spec.ts runs four cases against real
Postgres:

  • a successful compare-and-swap
  • a swap whose expected value carries the same content in a different key order
  • a concurrent rotation where the second writer must not clobber the first writer's token
  • an organization-scoped lookup that must not return another tenant's row

All four pass.

The key-order case is worth calling out because a review of this branch raised exactly
that concern, arguing JSON.stringify makes the comparison order-sensitive against
PostgreSQL's order-insensitive JSONB equality. The comparison is
"configValue" = :expected::jsonb, and the cast parses the parameter into jsonb before
comparing, so ordering does not matter. That is now demonstrated against the database
rather than argued from the source.

Still not covered

The full production chain, credential in encrypted BYOK settings through
resolveAgentModel to the worker to AgentReviewStage, is still not exercised end to
end, because the running containers predate this branch and testing it properly means
rebuilding the worker and api images. The storage, retrieval, scoping and rotation
underneath it are now covered directly.

@kody-ai

kody-ai Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

kody code-review Business Logic medium

🤔 Insufficient Task Context

I found a task linked to this PR, but it only contains minimal information (title only, no description or acceptance criteria). To perform a meaningful business rules validation, I need more details.

🔍 What I need to validate:

  • Business requirements and acceptance criteria
  • Expected behavior and business rules
  • Edge cases and constraints to consider

💡 How to improve the task context:

  • Add a description to the linked ticket
  • Include acceptance criteria or business rules
  • Describe the expected behavior after the change

⚠️ Important:

A task title alone is not sufficient to determine whether the implementation is correct or complete.

@acoliver

Copy link
Copy Markdown
Author

Running the stack on this branch

The api and worker images were rebuilt from this branch and the running stack was
switched onto them:

kodus_api         | ghcr.io/kodustech/kodus-ai-api:codex    | Up
kodus-worker-prod | ghcr.io/kodustech/kodus-ai-worker:codex | Up
api health: 200

Both services start clean, the worker comes up in its code-review role, and
chatgpt_subscription is present in the running container's compiled dist, so the
provider is in the production artifact rather than only in source.

Two build notes for anyone reproducing this. The compose file pins linux/amd64; on an
arm64 host it is faster to build native and override the platform than to build under
emulation. And compose will try to pull a locally built tag unless pull_policy: never
is set.

A failure that is not this branch's

Driving a review through the CLI against this stack produces:

Failed to clone repository kodustech/kodus-ai from Github
TypeError: Cannot read properties of undefined (reading 'length')

Rather than assume that was unrelated, I reverted the stack to the published :latest
images and ran the identical review. It fails the same way, the same number of times. So
this is the local stack's GitHub integration state, which did not survive a Docker daemon
restart, and not a regression introduced here. The stack was then switched back to the
branch images.

What is therefore still unproven

A full pull request review executing through the worker on a Codex credential remains
unverified. Getting there needs a configured git integration and a BYOK credential
seeded through the authenticated API, and the only account on this stack has a password
I do not hold. Rewriting a password hash directly in Postgres would have produced a
green result without earning it, so I stopped.

What is verified: the provider compiles into and ships inside the production images, the
services run on them, the credential storage, retrieval, tenant scoping and rotation are
covered against real Postgres, and the model and transport are covered against the live
endpoint including the multi-step reasoning replay.

@acoliver

Copy link
Copy Markdown
Author

Retracting the "control experiment" in my previous comment

An earlier comment claimed a CLI review against this stack failed to clone, that I
reverted to the published images and reproduced the same failure, and that this proved
the failure was environmental rather than a regression from the branch build. That
conclusion was not earned and I am withdrawing it.

What was actually true:

Both CLI runs I based it on wrote zero bytes of output and never produced a result file,
because I terminated them after between thirty seconds and two minutes. I never observed
a review fail. I saw Failed to clone repository in the worker log, assumed it belonged
to my run, and then ran a comparison against that assumption.

Checking it properly:

  • With no review running, the worker logs zero clone errors over a sixty second window,
    so the errors are attributable to the review rather than to background activity.
  • The errors are non-fatal. A review left alone passes them and keeps going; at the time
    of writing one has been running for over thirteen minutes with the clone-error count
    static at two.

So the honest statement is that the earlier runs did not fail. I killed them. The clone
error is a genuine log line worth someone's attention, since this stack has no configured
git integration, but it does not stop a review and it was not evidence of anything I used
it for.

I am leaving the earlier comment in place rather than editing it, so the correction is
legible.

The substantive claims in this PR are unaffected: the branch images build and run, the
provider is present in the running container's compiled output, and the credential and
rotation coverage against real Postgres stands. The item that remains genuinely unproven
is a full review executing on a Codex credential through the worker.

@acoliver

Copy link
Copy Markdown
Author

Root cause of the CLI review failure, corrected

My earlier comments on this got it wrong twice. Here is what is actually happening, with
the evidence, and a check of whether this branch is responsible.

There were two separate problems.

A job that was never consumed. The run I watched sit at PROCESSING for forty
minutes was never picked up. The worker sat at roughly 1% CPU, the job id appears nowhere
in its logs, and rabbitmqctl list_queues timed out. That message was lost while I
repeatedly recreated the api and worker containers to swap images. Restarting rabbitmq,
worker and api restored consumption. Self-inflicted, and not interesting except that it
masked the real problem.

A gate that requires GitHub authentication. Once jobs were being consumed again, a
fresh review produced a specific failure:

BadRequestException: Unknown authentication type.
    at GithubService.instanceOctokit (github.service.ts:3021)
    at GitHubRateLimitGateService.refreshSnapshot (github-rate-limit-gate.service.ts:142)
    at GitHubRateLimitGateService.check (github-rate-limit-gate.service.ts:83)
    at CliReviewJobProcessorService.process (cli-review-job-processor.service.ts:64)

CliReviewJobProcessorService.process calls a GitHub rate-limit gate before the review
begins. The gate builds an Octokit client, and on an installation with no git integration
there is nothing to build it from. This stack has zero rows in both integrations and
auth_integrations, so every CLI review fails at that line.

That looks worth a maintainer's attention on its own: kodus review is the local
entry point, and requiring a configured GitHub integration to rate-limit-check a review
of a local staged diff is surprising. I have not opened a separate issue for it since I
may be missing intended setup, but I am happy to.

Whether this branch caused it

It did not, and this time I checked rather than asserting:

  • The gate was added in 4c1aede, which is this branch's merge base, so it is not new
    here.
  • It is present in the published :latest worker image as well as in the branch build.
  • This branch's diff touches none of github.service.ts,
    github-rate-limit-gate.service.ts or cli-review-job-processor.service.ts. The
    changed files sit in libs/llm, libs/organization, libs/analytics, libs/core
    mongo and log, and the env templates.

Consequence for this PR

A full review executing on a Codex credential through the worker remains unproven, and
now for a clearly identified reason rather than a vague one: it needs a configured git
integration to get past the gate, and a BYOK credential seeded through an authenticated
API call. The CLI's team key authenticates /cli/* routes but not
/organization-parameters/*, which wants a user JWT.

Unchanged and verified: the branch images build and run, the provider is present in the
running container's compiled output, credential storage, tenant scoping and rotation are
covered against real Postgres, and the model and transport are covered against the live
endpoint including the multi-step reasoning replay.

@acoliver

Copy link
Copy Markdown
Author

The review does run on this branch's images

Correcting my earlier account once more, this time with the authoritative record rather
than inference from logs.

A CLI review executed against the worker built from this branch completed successfully.
From code_review_execution:

status       stage_name               finishedAt                message
success      PrepareCliFilesStage     2026-08-29 05:58:50.583
success      CreateSandboxStage       2026-08-29 05:58:50.594
in_progress  AgentReviewStage         (null)                    Starting...
success      AgentReview::generalist  2026-08-29 06:11:55.356   Generalist Agent — 3 findings in 785s

Three findings across 41 staged files, which is exactly what the same diff produced
before the images were rebuilt. The pipeline stages ran, the sandbox was created, and the
agent did real work for 785 seconds.

Why the job endpoint still reported PROCESSING

The parent AgentReviewStage row never closed. The last worker activity, at 06:14:25 and
so after the agent finished at 06:11:55, is:

[SeverityClassifier] Classification failed, defaulting to medium
AbortError: This operation was aborted
    at postToApi (@ai-sdk/provider-utils/src/post-to-api.ts:98)
    at _OpenAICompatibleChatLanguageModel.doGenerate
    at run (libs/llm/byok-model-wrapper.ts:98)

The severity classifier issues its own model call after the agent finishes. That call
timed out against the model this stack is configured with, which is slow. The error is
caught and severity defaults to medium, but the stage does not appear to close afterwards.

Whether this branch is responsible

The abort surfaces through libs/llm/byok-model-wrapper.ts, and this branch does modify
libs/llm, so this was worth checking rather than dismissing. That file is not among the
changed files. This branch's libs/llm changes are agent-model.ts, byok-config.ts,
byok-to-vercel.ts, codex-subscription-model.ts, llm-config-status.ts,
model-providers.ts and their specs.

Corrections to my earlier comments

Two things I said on this PR were wrong and are now superseded:

  • An earlier run that sat at PROCESSING for forty minutes was never consumed at all. I
    had recreated the api and worker containers repeatedly while swapping images and lost
    the message. Restarting rabbitmq, worker and api fixed consumption. That was mine.
  • I described the situation as an environmental failure on the basis of a control
    experiment. Both runs it rested on had been terminated by me before they could finish,
    so there was no failure to compare. Withdrawn earlier, restated here for completeness.

What stands: the branch images build, run, and execute a code review producing the
expected findings. A review on a Codex credential specifically remains unproven, because
that needs a BYOK credential seeded through an authenticated API call.

The connection test posted `input: 'ping'` as a bare string. The Codex
endpoint requires the Responses message array form and rejects a string with
`{"detail":"Input must be a list"}`, so testing a valid ChatGPT subscription
credential always reported a 400 and told the user their model id was wrong.

Every existing test mocks fetch and asserts the URL and headers, so none of
them looked at the request body and the defect was invisible. The added test
asserts the serialised body instead.

Found by calling the endpoint through the running API rather than a mock.

Refs kodustech#1802
@kody-ai

kody-ai Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Code Review Completed! 🔥

The code review was successfully completed based on your current configurations.

Kody Guide: Usage and Configuration
Interacting with Kody
  • Request a Review: Ask Kody to review your PR manually by adding a comment with the `@kody start-review` command at the root of your PR.

  • Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Current Kody Configuration
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug
Performance
Security
Business Logic

Access your configuration settings here.

@acoliver

Copy link
Copy Markdown
Author

Verified through the running API, and it found a bug

The connection test path is now exercised end to end: a real POST to
/organization-parameters/test-byok on an API container built from this branch, with a
genuine ChatGPT subscription credential.

Doing that immediately found a defect in this PR.

The bug

testCodexSubscription posted input: 'ping'. The Codex endpoint requires the Responses
message array form and rejects a bare string:

{"ok":false,"code":"bad_request","httpStatus":400,
 "providerMessage":"{\"detail\":\"Input must be a list\"}"}

So testing a perfectly valid ChatGPT subscription credential always failed, with a
message advising the user to check their model id. Fixed in fea3b7f.

Worth noting why no test caught it. The three existing specs mock fetch and assert the
URL and the headers. None of them deserialised the body, so the request shape was never
checked by anything. The added test asserts the serialised body instead.

What the same call proves

After the fix, the identical request returns:

{"ok":true,"code":"rate_limit","httpStatus":429,
 "providerMessage":"{\"error\":{\"type\":\"usage_limit_reached\",\"plan_type\":\"pro\",...}}"}

Three things at once. The request shape is accepted. The credential is valid. And
classifyHttpStatus behaves as intended on a real response: 429 maps to ok: true,
because the key works and the provider is merely throttling, and both httpStatus and
providerMessage survive.

That last point is the review finding this PR opened with. The original code built an
Axios-shaped object by hand and passed it to normalizeError, but axios.isAxiosError
requires isAxiosError === true, so every Codex failure fell through to the unknown tail
and lost its status and provider message. The response above is that fix working against
a live endpoint rather than a fixture.

Method note

None of this was reachable from the test suite. Two rounds of review, an architecture
pass, and a self-review by the tool itself all read this function without noticing the
input shape, because reading code cannot tell you what a remote endpoint accepts. One
real request did.

@kody-ai

kody-ai Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

kody code-review Business Logic medium

🤔 Insufficient Task Context

I found a task linked to this PR, but it only contains minimal information (title only, no description or acceptance criteria). To perform a meaningful business rules validation, I need more details.

🔍 What I need to validate:

  • Business requirements and acceptance criteria
  • Expected behavior and business rules
  • Edge cases and constraints to consider

💡 How to improve the task context:

  • Add a description to the linked ticket
  • Include acceptance criteria or business rules
  • Describe the expected behavior after the change

⚠️ Important:

A task title alone is not sufficient to determine whether the implementation is correct or complete.

…on-auth

# Conflicts:
#	libs/llm/byok-config.ts
#	libs/llm/resolve-model-slot.ts
#	libs/organization/application/use-cases/organizationParameters/test-byok-connection.use-case.spec.ts
#	libs/organization/application/use-cases/organizationParameters/test-byok-connection.use-case.ts
AbortSignal.timeout rejects with a DOMException whose name the axios
branches never matched, so a timed-out Codex connection test reported
code 'unknown' instead of 'network'. Match by name — realm-safe — and
share one timeout result builder with the axios path.
@kody-ai

kody-ai Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Code Review Completed! 🔥

The code review was successfully completed based on your current configurations.

Kody Guide: Usage and Configuration
Interacting with Kody
  • Request a Review: Ask Kody to review your PR manually by adding a comment with the `@kody start-review` command at the root of your PR.

  • Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Current Kody Configuration
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug
Performance
Security
Business Logic

Kody Code Review — 3 suggested fixes.
Paste the prompt below to your agent and all review fixed at once!

🛠️ Open Agent Prompt
A code review identified the following issues in this pull request.
Each section describes what was found and includes a reference implementation where available.

Files involved:
- libs/organization/application/use-cases/organizationParameters/test-byok-connection.use-case.ts:458
- libs/organization/application/use-cases/organizationParameters/test-byok-connection.use-case.ts:518
- libs/organization/application/use-cases/organizationParameters/test-byok-connection.use-case.ts:510

---

### [1/3] libs/organization/application/use-cases/organizationParameters/test-byok-connection.use-case.ts:458
Issue identified during code review:
testOpenAICompatibleChat lets the NOVITA branch POST to a caller-supplied baseURL without running the SSRF guard, which only executes for OPENAI_COMPATIBLE, so a caller can point Novita at an internal address and read up to ~300 chars of the response. Apply assertSafeOpenAICompatibleUrl(url) before POSTing whenever a user-supplied baseURL is used for both OPENAI_COMPATIBLE and NOVITA (when baseURL?.trim() is set).
Reference implementation (from code review):

// libs/organization/application/use-cases/organizationParameters/test-byok-connection.use-case.ts:458
if (provider === BYOKProvider.OPENAI_COMPATIBLE ||
    (provider === BYOKProvider.NOVITA && baseURL?.trim())) {
    await assertSafeOpenAICompatibleUrl(url);
}

---

### [2/3] libs/organization/application/use-cases/organizationParameters/test-byok-connection.use-case.ts:518
Issue identified during code review:
The catch block in testOpenAICompatibleChat swallows exceptions without logging, violating the project’s requirement to log failures via PinoLoggerService with metadata (including organizationId) for traceability. Add a this.logger.error call with context TestByokConnectionUseCase.name, error details, and metadata { provider, model, url } before returning the normalized result.
Reference implementation (from code review):

// libs/organization/application/use-cases/organizationParameters/test-byok-connection.use-case.ts:518
} catch (err) {
            this.logger.error({
                message: 'OpenAI-compatible chat probe failed',
                context: TestByokConnectionUseCase.name,
                error: err instanceof Error ? err.message : String(err),
                metadata: { provider, model, url },
            });
            return this.normalizeError(err, Date.now() - start);
        }

---

### [3/3] libs/organization/application/use-cases/organizationParameters/test-byok-connection.use-case.ts:510
Issue identified during code review:
The chat probe calls the provider's /chat/completions endpoint directly via axios, bypassing usage accounting so token consumption isn't tracked in usage analytics and BYOK tokens go unaccounted. Wrap the axios call in runAiSdkLLMInSpan with a stable runName and attrs containing organizationId, provider, and model metadata.
Reference implementation (from code review):

// libs/organization/application/use-cases/organizationParameters/test-byok-connection.use-case.ts:510
await this.observabilityService.runAiSdkLLMInSpan({
            spanName: 'test-byok-connection.openai-chat-probe',
            runName: 'test-byok-connection.openai-chat-probe',
            model,
            attrs: { organizationId, provider, url },
            exec: () => axios.post(url, body, {
                headers: {
                    Authorization: `Bearer ${apiKey}`,
                    'Content-Type': 'application/json',
                },
                timeout: TEST_TIMEOUT_MS,
                maxRedirects: 0,
            }),
        });

---

Review each issue in context, use the reference implementations as guidance, and apply fixes that are consistent with the surrounding codebase.

Access your configuration settings here.

@acoliver

Copy link
Copy Markdown
Author

Review-thread dispositions (kody-ai findings):

Fixedtest-byok-connection.use-case.ts timeout classification (7ab35c5): the Codex probe's AbortSignal.timeout rejects with a DOMException the axios branches never matched, so a timed-out test returned code: 'unknown' instead of 'network'. Now matched by name (realm-safe under jest's vm) and shares one timeout result builder with the axios path; regression test added.

Declined, with reasons:

  1. codex-subscription-model.ts ordering — the content array is assembled in stream arrival order (Map insertion order). Responses streams emit reasoning before text, so output order matches the wire order; there is no text-before-reasoning inversion.
  2. wrap doStream in runAiSdkLLMInSpan — spans are applied at the call layer (agent-loop-call.ts wraps every LLM call via runAiSdkLLMInSpan). This module is transport; wrapping internally would double-count each product call.
  3. re-add try/catch logging in the repository — the refresh flow already retries with bounded backoff at the service layer (documented in the PR body, with a test). The repository should throw; catching-and-logging there hides failures from the retry path.
  4. filter()[0]find() — micro-optimization on a list sized by the org's credentials; the existing shape matches the file's surrounding style.
  5. leftJoinAndSelectleftJoin — the loaded relation is consumed: mapSimpleModelsToEntities explicitly requests relations: ['organization']. A plain join leaves the entity relation uninitialized for those consumers.
  6. span-wrap the opt-in live spec — the suggested code references this.observabilityService inside a jest spec, where no such member exists; it does not compile. The live test deliberately exercises the raw transport.

@kody-ai

kody-ai Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

kody code-review Business Logic medium

🤔 Insufficient Task Context

I found a task linked to this PR, but it only contains minimal information (title only, no description or acceptance criteria). To perform a meaningful business rules validation, I need more details.

🔍 What I need to validate:

  • Business requirements and acceptance criteria
  • Expected behavior and business rules
  • Edge cases and constraints to consider

💡 How to improve the task context:

  • Add a description to the linked ticket
  • Include acceptance criteria or business rules
  • Describe the expected behavior after the change

⚠️ Important:

A task title alone is not sufficient to determine whether the implementation is correct or complete.

…d Codex rotations

Three transport fixes from deep review:

- doGenerate reassembly emitted every text/reasoning fragment before all
  passthrough parts, reordering a stream of [text, tool-call, text] into
  [text, text, tool-call]. Fragments and passthrough parts are now emitted
  in arrival order.
- A rotation whose persistence failed its full retry budget threw away the
  server-issued replacement while the stale refresh token was already
  consumed, permanently destroying the credential. The replacement is now
  retained in memory and the next call retries only the persistence leg;
  the credential store is captured at rotation start so module teardown
  cannot null it mid-rotation.
- Text-part metadata (start/delta/end) and the finish part's
  providerMetadata were dropped from the synthesized doGenerate result;
  both now flow through like reasoning metadata.
@kody-ai

kody-ai Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Code Review Completed! 🔥

The code review was successfully completed based on your current configurations.

Kody Guide: Usage and Configuration
Interacting with Kody
  • Request a Review: Ask Kody to review your PR manually by adding a comment with the `@kody start-review` command at the root of your PR.

  • Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Current Kody Configuration
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug
Performance
Security
Business Logic

Kody Code Review — 1 suggested fix.
Paste the prompt below to your agent and all review fixed at once!

🛠️ Open Agent Prompt
A code review identified the following issues in this pull request.
Each section describes what was found and includes a reference implementation where available.

Files involved:
- libs/llm/codex-subscription-model.ts:317

---

### [1/1] libs/llm/codex-subscription-model.ts:317
Issue identified during code review:
Emission now iterates `orderedParts`, which only gets entries from `ensureFragment` on start/end events, so a delta arriving without a prior start event is silently dropped. Call `ensureFragment(key)` at the top of the `text-delta` and `reasoning-delta` cases to guarantee every received fragment is registered (it's idempotent, preserving ordering).
Reference implementation (from code review):

// libs/llm/codex-subscription-model.ts:317
case 'text-delta': {
    const key = fragmentKey('text', value.id);
    ensureFragment(key);
    appendFragment(key, value.delta);
    mergeMetadata(key, value.providerMetadata);
    break;
}
case 'reasoning-delta': {
    const key = fragmentKey('reasoning', value.id);
    ensureFragment(key);
    appendFragment(key, value.delta);
    mergeMetadata(key, value.providerMetadata);
    break;
}

---

Review each issue in context, use the reference implementations as guidance, and apply fixes that are consistent with the surrounding codebase.

Access your configuration settings here.

@acoliver
acoliver marked this pull request as ready for review August 30, 2026 20:15
@itoqa

itoqa Bot commented Aug 30, 2026

Copy link
Copy Markdown

⚠️ Ito couldn't review your PR: you've used all of your free-trial code reviews.

To continue having Ito review your PRs, upgrade your account in Ito (or ask your manager).

@kody-ai

kody-ai Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

kody code-review Business Logic medium

🤔 Insufficient Task Context

I found a task linked to this PR, but it only contains minimal information (title only, no description or acceptance criteria). To perform a meaningful business rules validation, I need more details.

🔍 What I need to validate:

  • Business requirements and acceptance criteria
  • Expected behavior and business rules
  • Edge cases and constraints to consider

💡 How to improve the task context:

  • Add a description to the linked ticket
  • Include acceptance criteria or business rules
  • Describe the expected behavior after the change

⚠️ Important:

A task title alone is not sufficient to determine whether the implementation is correct or complete.

Comment thread libs/llm/codex-subscription-model.ts
…pped

The ordered-parts emission introduced for stream-order reassembly only
registered fragments on start/end events. A delta arriving without a
prior start wrote to the fragment map but never entered the ordered
list, so its text was silently dropped at emission. Registering in the
delta cases is idempotent and preserves ordering.
@kody-ai

kody-ai Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Code Review Completed! 🔥

The code review was successfully completed based on your current configurations.

Kody Guide: Usage and Configuration
Interacting with Kody
  • Request a Review: Ask Kody to review your PR manually by adding a comment with the `@kody start-review` command at the root of your PR.

  • Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Current Kody Configuration
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug
Performance
Security
Business Logic

Kody Code Review — 1 suggested fix.
Paste the prompt below to your agent and all review fixed at once!

🛠️ Open Agent Prompt
A code review identified the following issues in this pull request.
Each section describes what was found and includes a reference implementation where available.

Files involved:
- libs/llm/codex-subscription-model.spec.ts:137

---

### [1/1] libs/llm/codex-subscription-model.spec.ts:137
Issue identified during code review:
Every LLM invocation, including in test files, must go through token-usage accounting. Wrap the model call in runAiSdkLLMInSpan with a stable runName and the organizationId in attrs.
Reference implementation (from code review):

// libs/llm/codex-subscription-model.spec.ts:137
const result = await this.observabilityService.runAiSdkLLMInSpan({
  spanName: 'codex.subscription.test',
  runName: 'codex.subscription.test',
  model: model.modelId,
  attrs: { organizationId: 'test-org' },
  exec: () => model.doGenerate(promptOptions()),
});

---

Review each issue in context, use the reference implementations as guidance, and apply fixes that are consistent with the surrounding codebase.

Access your configuration settings here.

@acoliver

Copy link
Copy Markdown
Author

@kody-ai correct on both counts — the ordered-parts reassembly in dd2664a registered fragments only on start/end events, so a delta without a prior start was dropped at emission (the pre-refactor code iterated the fragment map directly and would have kept it). Applied exactly the suggested idempotent ensureFragment calls in the text-delta and reasoning-delta cases, with a regression test that feeds startless text and reasoning deltas and asserts both are emitted. Fixed in 2eb4d24 (10/10 spec, type gate and prettier clean). Thread resolved.

@kody-ai

kody-ai Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

kody code-review Business Logic medium

🤔 Insufficient Task Context

I found a task linked to this PR, but it only contains minimal information (title only, no description or acceptance criteria). To perform a meaningful business rules validation, I need more details.

🔍 What I need to validate:

  • Business requirements and acceptance criteria
  • Expected behavior and business rules
  • Edge cases and constraints to consider

💡 How to improve the task context:

  • Add a description to the linked ticket
  • Include acceptance criteria or business rules
  • Describe the expected behavior after the change

⚠️ Important:

A task title alone is not sufficient to determine whether the implementation is correct or complete.

Comment thread libs/llm/codex-subscription-model.spec.ts
@acoliver

Copy link
Copy Markdown
Author

@kody the "Insufficient Task Context" notice has been posted on eight review runs now, and its premise doesn't hold for the linked task.

This PR fixes #1802, which has carried a full description and an explicit acceptance-criteria section since 2026-08-28 18:32Z — an hour before this PR was opened and two hours before the first notice. The endpoint contract, the bug analysis, the credential design, and the acceptance criteria are all there.

If your task linkage is resolving to a Kodus-board task auto-created from the PR rather than the GitHub issue the PR closes, please use #1802 as the task context for business-rules validation. If the issue body isn't reachable to the integration, that's worth an internal look, since every run is reporting a context gap the issue does not have.

@acoliver

Copy link
Copy Markdown
Author

@kody task context for business-rules validation, since the linked-ticket lookup keeps coming back empty.

Task: #1802 — finish ChatGPT/Codex subscription auth (wire the existing transport into the product, fix the reasoning round-trip, add token refresh).

Business requirements

A self-hosted Kodus deployment must be able to run code reviews billed to a developer's ChatGPT subscription, with no OpenAI API key configured. Provider chatgpt_subscription, account-scoped models (gpt-5.6-luna, gpt-5.6-terra verified on a Pro plan).

Business rules this change must honor

  1. The Codex endpoint requires stream: true and store: false, and rejects temperature and max_output_tokens. The provider must never send the rejected parameters.
  2. Under store: false, reasoning items without encrypted_content are dropped on the next request, so the model re-derives its reasoning at every step of a 30–60 call review loop. The round-trip must preserve it.
  3. The refresh token rotates on every use and the server invalidates the old one immediately. Persistence must happen before the new access token is used, via compare-and-swap, with bounded retry — a lost rotation destroys the user's credential permanently.
  4. A subscription has no per-token price: usage is reported for observability but contributes zero to spend.
  5. Credentials follow the encrypted BYOK path (Bedrock precedent for non-apiKey auth); API_CODEX_AUTH_FILE is an explicit dev-only override, never an implicit os.homedir() fallback. Organization-scoped lookups must not return another tenant's rows.

Acceptance criteria

  • A review runs on gpt-5.6-luna with no API key configured.
  • encrypted_content survives across tool-call turns — proven by a mocked two-step test asserting the ciphertext reaches the second request body, and an opt-in live multi-step test.
  • Refresh persists the rotated token before use and survives a concurrent write — four cases against real Postgres including key-order-varied JSONB equality and a cross-tenant isolation check.
  • A Codex model contributes zero to reported monthly spend; pricing is never consulted.
  • temperature and max_output_tokens never reach the endpoint — asserted on the serialised request body.
  • Connection test classifies real endpoint behavior: valid-but-throttled (429) returns ok: true with httpStatus/providerMessage preserved; timeouts classify as network, not unknown.

Expected behavior after the change

Admin seeds the credential through the authenticated BYOK API → reviews execute billed to the subscription → token expiry mid-review rotates without losing the credential → spend reports are unaffected.

Edge cases

Model availability varies by plan and over time (the listing is a convenience picker, not an account inventory; a typed-in model id still works). A review interrupted mid-rotation resumes on the persisted token. Accounts without the subscription get the endpoint's own refusal, surfaced with its provider message.

@acoliver

Copy link
Copy Markdown
Author

@kody start-review

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.

Finish ChatGPT/Codex subscription auth: wire the existing provider, fix the reasoning round-trip, add refresh

1 participant