Skip to content

🤖 feat: route skills to model classes (Settings-managed large/medium/small) - #3849

Open
asm wants to merge 1 commit into
coder:mainfrom
asm:skill-model-classes
Open

🤖 feat: route skills to model classes (Settings-managed large/medium/small)#3849
asm wants to merge 1 commit into
coder:mainfrom
asm:skill-model-classes

Conversation

@asm

@asm asm commented Aug 14, 2026

Copy link
Copy Markdown

Summary

Skills can now be routed to user-defined model size classes so mechanical skills (wrap-up chores, formatting passes, routine repo tasks) don't consume frontier-model tokens. Classes map a name to a model[+thinking] value (one-shot syntax) and are edited in Settings → Models → Model Classes; skills bind to a class via the spec-standard frontmatter metadata: model-class: small or a local skillModelClasses config table. The class model applies to that invocation only — the workspace model is untouched. One-shot overrides also compose with skill invocations now (/haiku+0 /deep-review), and an explicit one-shot always beats class routing.

Background

Models churn constantly, so per-skill bindings shouldn't name concrete models — they name a class (large/medium/small), and only the class map names models. Updating one class re-routes every bound skill.

  • 🤖 feat: add /<model> one-shot model override syntax #2142 introduced one-shot model overrides; this reuses that exact plumbing (per-send model) and extends it to compose with skill slash invocations.
  • Agent definition ai.model is not consulted when spawning sub-agent tasks #3038 describes the kindred gap for agent definitions (ai.model parsed but not consulted); this PR takes the same position for skills — a declared model preference should be honored — while keeping it strictly opt-in.
  • Portability: the binding uses the Agent Skills spec's metadata map, which other harnesses ignore. Frontmatter bindings to a class the user never defined are deliberately inert, so skills shipping metadata: model-class can never break users who haven't opted in. The config table exists for routing skills the user doesn't own — and because the table is the user's own explicit intent, a dangling table entry (naming a class that was deleted) fails loudly instead of silently unrouting.

Implementation

  • Config: modelClasses and skillModelClasses records (schema, load normalization, saveConfig whitelist, config.updateModelClasses route). Maps are stored verbatim — entries this build can't parse are preserved, not dropped, so edits from an older/newer build never destroy classes they don't understand. Validity is judged lazily at send time by the resolver.
  • Shared resolver (src/common/utils/ai/skillModelClasses.ts): binding resolution as a discriminated union (unbound / unknown-class / invalid-value / resolved), plus isModelServableWithProvidersConfig (modelAvailability.ts) wrapping the routing layer's isModelAvailable with the same exported provider/gateway predicates useRouting consumes — so a model reachable only via a configured gateway (e.g. OpenRouter) correctly counts as available, route-priority membership is honored, and the editor warning cannot drift from the send-time gate.
  • Send path (AgentSession.sendMessage): the override is resolved before the pricing gate, PDF-support preflight, and any history mutation, so those gates evaluate the model that will actually stream and a broken binding errors before persisting side effects. Routing is gated by a dedicated skipSkillModelRouting send option (set by explicit one-shot composition and compaction retries) rather than overloading skipAiSettingsPersistence. Bound-but-broken mappings (dangling table entry, invalid value, no configured route for the model) fail the send with an actionable error naming the fix and the one-shot bypass; unbound skills take a null fast-path and infrastructure failures (unreadable skill/config, providers state unavailable) fail open.
  • Compaction interplay: the auto-compaction threshold is computed against the routed model's context window, while the compaction request itself and its follow-up resume options carry the pre-routing model/thinking (the compaction model must fit the uncompacted history, and the user's model choice must survive the round-trip). Mid-stream forced compaction during a routed turn threads the same pre-routing options through the stream context. Routed sends only auto-compact when the history is within ROUTED_SEND_COMPACTION_HEADROOM_PERCENT (10 points) of the routed model's window — headroom for the pending turn, while still far above the workspace threshold so a small-context class model can't trigger surprise compaction of a history the workspace model handles fine.
  • Settings UI: a "Model Classes" section under Settings → Models with fixed canonical slots (large/medium/small — a shared vocabulary keeps skill frontmatter portable across machines), model + thinking selects per class, custom hand-edited classes preserved on save and listed read-only (unparseable raw values shown in a tooltip), and an inline "no configured route can serve this model" warning using the same predicate as the send-time check. Edits are disabled until config and routing state finish loading, so an early click can't clobber persisted classes; thinking suffixes carry across model swaps only when the target model's policy supports them.
  • Composer: parseCommandWithSkillInvocation composes a leading one-shot with a skill invocation by re-running parseCommand on the one-shot's message — registered commands and nested one-shots stay out of skill resolution, mirroring direct-invocation semantics exactly. Composed sends record the full command prefix (model /skill) in message metadata so transcript badges render what was actually typed. Numeric one-shot thinking is model-relative, so a thinking-only composed send (/+0 /skill) also passes the raw index (oneShotThinkingIndex) for the backend to re-resolve against the routed model's ladder — +0 means the class model's lowest level, not the workspace model's. Compact-and-retry rebuilds re-derive the one-shot's model and thinking from the original text (with skipAiSettingsPersistence, so a re-dispatch never persists one-shot values as new workspace defaults), and prepareCompactionMessage keeps carried one-shot fields from being clobbered by ambient stored options.
  • Attribution: when routing applies, the persisted user-message metadata is re-stamped with the routed model (requestedModel), so the pending-turn label and history consumers see the model that actually streams.

Review-round hardening

Sixteen Codex review rounds tightened the edges (all threads resolved):

  • Send-path ordering: routing resolves before the pricing gate, PDF preflight, and any history mutation; rejected manual/queued sends persist a visible error (never for edits, which return bare and restore the draft); queued PDF rejections surface instead of vanishing.
  • Compaction interplay: routed sends compact within a headroom of the routed window (pre-send AND mid-stream); the compaction request runs on whichever of the user/routed model has the larger usable window; the routed policy survives same-session retries, compact-and-retry rebuilds (model, thinking, prefix, skipAiSettingsPersistence), and process relaunch (durable compactionBaseOptions in retrySendOptions, honored even in child task workspaces).
  • Availability truth: the shared servability predicate and ProviderModelFactory.resolveModelRoute both apply model-aware OpenAI credential rules (Codex-OAuth-only serves the OAuth set; API keys attempt anything; custom openai-compatible providers shadowing the openai id are exempt).
  • Telemetry attribution: the accepted-send payload reports routedModel + post-floor routedThinkingLevel; persisted metadata re-stamps requestedModel. Queued-send event attribution is documented as a follow-up (needs backend-side event capture).
  • Editor integrity: class edits persist before publishing (no split-brain with a fast follow-up send), rows lock while their write is in flight, custom classes survive verbatim, and CI snapshots the wrapping layout at a pinned phone viewport.

Validation

  • ~70 tests across the feature: resolver statuses (frontmatter-inert vs table-loud, blank table entries, the opt-in guard), availability predicate (route-priority membership, disabled providers), config round-trip through the saveConfig whitelist (including preservation of unknown classes), end-to-end AgentSession routing and error paths via the session harness (gate ordering, skipSkillModelRouting exemption, thinking-only bindings, compaction follow-up model), composition parser cases, and editor UI behavior (clear preserves custom classes; load gating; warning states).
  • Full bun test src failure set is identical to main's on the same machine (pre-existing env-sensitive tests only).
  • Verified live in Storybook (ModelsSection stories now seed classes, including one pointing at an unconfigured provider to exercise the warning; row layout wraps at mobile widths) and in a packaged build used for daily work.

Risks

The sensitive area is the insertion in AgentSession.sendMessage. Scope is tightly bounded: only sends carrying agent-skill metadata without skipSkillModelRouting are considered, and workspaces with no modelClasses/table binding hit an early return before any skill read — no behavior change for anyone who hasn't opted in. Compaction interplay (threshold on the routed model, compaction request and mid-stream forced compaction on the user's model, follow-up resume options) is covered by tests. One known asymmetry, documented at the helper: the shared servability predicate mirrors the routing layer's gateway/priority gates but not per-request policy checks, so an editor warning can under-report in exotic policy setups — the send-time error remains authoritative.


🤖 Generated with Claude Code

@asm
asm marked this pull request as draft August 14, 2026 00:08
@asm
asm marked this pull request as ready for review August 14, 2026 03:20
@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

@asm

asm commented Aug 14, 2026

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 60f19ad5e5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/browser/features/ChatInput/index.tsx Outdated
Comment thread src/browser/hooks/useCompactAndRetry.ts Outdated
Comment thread src/node/services/agentSession.ts
@asm

asm commented Aug 14, 2026

Copy link
Copy Markdown
Author

@codex review

All three findings addressed in b1b0bf8:

  • Numeric thinking vs routed model: the frontend now passes the raw index (oneShotThinkingIndex send option) alongside the workspace-resolved level, and AgentSession re-resolves it against the routed class model when routing applies — /+0 /skill means the routed model's lowest allowed level. Covered by a routing test where the pre-resolved level ("medium") and the routed ladder ("off") differ.
  • One-shot thinking across compact-and-retry: the rebuilt follow-up carries the parsed thinking (named as-is; numeric resolved against the explicit model, or kept as a raw index for routed re-resolution) plus skipAiSettingsPersistence, and prepareCompactionMessage no longer lets ambient stored options clobber carried one-shot fields. This also fixes a latent issue: without the persistence flag, the re-dispatch would have persisted the one-shot model as the new workspace default.
  • requestedModel: when routing applies, the persisted user-message metadata is re-stamped with the routed model, so the incoming user event and history consumers attribute the send correctly. One deliberate limit: the frontend's fire-and-forget messageSent telemetry event still reports the requested model — threading the routed model through the send result would widen Result<void> across ~15 return sites, which felt too invasive here; happy to do it as a follow-up if maintainers prefer.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b1b0bf8591

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentSession.ts
@asm

asm commented Aug 14, 2026

Copy link
Copy Markdown
Author

@codex review

Round-2 finding addressed in 6c4903d: routed sends now compact within ROUTED_SEND_COMPACTION_HEADROOM_PERCENT (10 points) of the routed model's window instead of requiring a full 100% — headroom for the pending message, attachments, and skill snapshot that the recorded usage doesn't include, while still staying far above the workspace threshold so a cheap skill invocation can't force an unrequested compaction of a history that fits.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6c4903deb0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentSession.ts
Comment thread src/node/services/agentSession.ts Outdated
@asm

asm commented Aug 14, 2026

Copy link
Copy Markdown
Author

@codex review

Both round-3 findings addressed in 297b210:

  • Mid-stream routed policy: checkMidStream now accepts a force-threshold override, and routed turns (identified by the stream context's compaction base options) pass the same routed-send headroom bar — a usage update during a routed turn no longer forces compaction at the workspace threshold+buffer against the smaller routed window. Monitor test covers the override at 75% (no trigger) and 92% (trigger).
  • Compaction model fit: on-send and mid-stream compaction now run with whichever of the user's / routed model has the larger usable context window (getEffectiveContextLimit comparison) — normally still the user's model, but a class routing UP past the user's window no longer summarizes on a model that can't read the history. The deferred follow-up keeps pre-routing options and re-routes at dispatch either way.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 297b210330

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/common/utils/ai/modelAvailability.ts Outdated
@asm

asm commented Aug 14, 2026

Copy link
Copy Markdown
Author

@codex review

Round-4 finding addressed in 50b68ee: added canDirectOpenAIServeModel (colocated with the existing Codex OAuth routing mirrors) reflecting the factory's credential selection — OAuth-required models need stored tokens even with an API key, and OAuth-only configs serve only the allowed model set — and the shared servability predicate now consults it for direct-OpenAI routes. An OAuth-ineligible class model no longer passes the preflight on a Codex-OAuth-only config; a later gateway in routePriority can win, or the user gets the actionable class error. Tests cover OAuth-only vs API-key vs OAuth-required combinations.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 50b68ee8fc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/common/utils/ai/modelAvailability.ts Outdated
Comment thread src/node/services/agentSession.ts
Comment thread src/node/services/agentSession.ts
@asm

asm commented Aug 14, 2026

Copy link
Copy Markdown
Author

@codex review

All three round-5 findings addressed in 6452b8a:

  • Factory route selection: resolveModelRoute (both call sites) now passes the canonical model into isProviderAvailableForRouting, which rejects direct OpenAI when tokens-only credentials can't serve the model — a usable gateway later in routePriority wins, matching the shared predicate. I also realigned canDirectOpenAIServeModel with the factory's actual fallback semantics (an API key attempts any model, including OAuth-preferred ones; tokens-only serves only the allowed set) and updated the tests accordingly.
  • PDF preflight: the client-side check now defers to the backend's routed-model gate whenever a routable skill invocation is present (skillInvocation && !modelOverride) — the backend validates against the class model and rejects with a persisted, visible error, so a PDF-capable class model bound to a skill can actually receive PDFs.
  • Send telemetry: sendMessage now returns SendMessageAccepted { routedModel } through AgentSession → WorkspaceService → router → wire schema, and ChatInput attributes messageSent telemetry to result.data?.routedModel ?? effectiveModel. Queued sends report no routed model (dispatch happens later) and fall back to the requested model, as documented on the schema. Routing tests assert the payload for both the routed and skip-flag cases.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6452b8a491

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/browser/hooks/useModelClasses.ts Outdated
Comment thread src/browser/features/Settings/Sections/ModelClassesEditor.tsx
@asm

asm commented Aug 14, 2026

Copy link
Copy Markdown
Author

@codex review

Both round-6 findings addressed in 47afc04:

  • Manual memoization removed: the subscription's fetch now lives inside the effect and the write-failure revert reaches it through a ref — no useCallback, no exhaustive-deps suppressions. (Note: useModelFallbacks, which this hook was modeled on, uses the same pre-existing useCallback pattern upstream; left untouched here as out of scope.)
  • Pinned phone snapshot: ModelsConfiguredPhone pins a Pixel phone matrix variant mirrored with globals.viewport, so CI snapshots the Model Classes rows at the width their wrapping layout exists for. Verified live at 375px: label/select wrap, inline no-route warning on the unconfigured row, no right-edge overflow.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 47afc04612

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/browser/hooks/useModelClasses.ts Outdated
Comment thread src/browser/features/ChatInput/index.tsx Outdated
Comment thread src/browser/features/ChatInput/index.tsx Outdated
@asm

asm commented Aug 14, 2026

Copy link
Copy Markdown
Author

@codex review

Round-7 findings in 44facc0:

  • Split-brain class edits (fixed): useModelClasses now serializes writes and publishes state only on the write's ack — the editor can no longer advertise a mapping the backend doesn't have. Rapid edits build on the newest pending intent (no lost updates), and failures still revert via refetch.
  • Routed thinking telemetry (fixed): the accepted-send payload gains routedThinkingLevel (class suffix or re-resolved numeric one-shot), and messageSent attributes to it. Covered by a routing test asserting both payload fields.
  • Queued-send attribution (descoped as follow-up, with rationale): routing resolves at dispatch, not at queue-accept, so correct attribution for queued sends requires emitting the event backend-side at dispatch — and message_sent carries frontend-only provenance (frontendPlatform, runtime context) that a backend emitter would misreport. The durable record is already attributed correctly at dispatch via the persisted requestedModel stamp; only the fire-and-forget analytics event stays approximate for the queued minority. Happy to build dispatch-time backend capture as a follow-up if maintainers want it — it needs its own provenance design rather than a bolt-on here.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 44facc03ea

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentSession.ts Outdated
Comment thread src/node/services/agentSession.ts
Comment thread src/browser/features/ChatInput/index.tsx Outdated
@asm

asm commented Aug 14, 2026

Copy link
Copy Markdown
Author

@codex review

All three round-8 findings addressed in 79fb8e0:

  • Queued PDF rejections preserved: both PDF gate branches (unsupported input, size limit) now persist and surface the rejection through preserveRejectedManualSend — same contract as the pricing and model-class gates — so a queued skill send whose PDF the routed model rejects leaves a visible transcript error instead of silently discarding the user's text and attachment.
  • Post-policy routed thinking: the per-model floor resolution + clamping now live in one shared method (resolveThinkingFloorForModel / enforceThinkingFloorsForModel) used by both the stream request build and the accepted-send payload — routedThinkingLevel reports the clamped level the stream actually runs at.
  • Named one-shot fallback: messageSent falls back to the send's actual sendOptions.thinkingLevel (which carries a composed one-shot's thinking) rather than the ambient workspace setting.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 79fb8e040b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentSession.ts Outdated
@asm

asm commented Aug 14, 2026

Copy link
Copy Markdown
Author

@codex review

Round-9 finding addressed in 6284377: routedThinkingLevel now reports the effective level for every routed send — whatever optionsForStream carries (class suffix, re-resolved numeric one-shot, or a named/ambient level riding through), clamped by the shared per-model floor enforcement. A /+off /skill routed onto a floor-medium model reports medium. Test covers the ride-through case.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 62843778d0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentSession.ts Outdated
Comment thread src/browser/hooks/useCompactAndRetry.ts Outdated
@asm
asm force-pushed the skill-model-classes branch from 6284377 to 3d6ffbd Compare August 14, 2026 17:57
@asm

asm commented Aug 14, 2026

Copy link
Copy Markdown
Author

@codex review

Both round-10 findings addressed, and the branch is rebased onto latest main (the #3844 conflict in agentSession.ts resolved by adopting the new gateway-preserving lookupMinThinkingLevelOverride inside the shared floor helper):

  • Routed compaction context across retries: the auto-retry resume state now carries compactionBaseOptions, resumeStream threads it through to streamWithHistory, and the post-compaction context-exceeded retry reads it from the captured stream context — same-session restarts keep both the routed force threshold and the larger-window compaction model selection.
  • Leading-whitespace one-shots: the compact-retry reparse guard now trims before checking, matching parseCommand's own tolerance.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3d6ffbd18d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/browser/hooks/useCompactAndRetry.ts Outdated
@asm

asm commented Aug 14, 2026

Copy link
Copy Markdown
Author

@codex review

Round-11 finding addressed: the compact-and-retry metadata rebuild now carries source.commandPrefix into buildAgentSkillMetadata, so recovered composed invocations keep their command badge.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f9eb115404

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/common/utils/ai/modelAvailability.ts Outdated
@asm

asm commented Aug 14, 2026

Copy link
Copy Markdown
Author

@codex review

Round-12 finding addressed: canDirectOpenAIServeModel now recognizes a custom openai-compatible provider shadowing the openai id (via the existing isCustomOpenAICompatibleProviderConfig detector) and exempts it from built-in OpenAI credential rules — custom endpoints authenticate on their own terms, so availability falls back to the ordinary isConfigured gate. Test covers a keyless shadowing provider serving an OAuth-ineligible model.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2ecc3f4b18

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentSession.ts
Comment thread src/node/services/agentSession.ts Outdated
Comment thread src/browser/features/Settings/Sections/ModelClassesEditor.tsx

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1a7edc80c0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentSession.ts Outdated
Comment thread src/node/services/agentSession.ts Outdated
@asm

asm commented Aug 31, 2026

Copy link
Copy Markdown
Author

@codex review — wave-15 addressed: edits materialize trust-sensitive snapshots before the destructive truncation, and the final revalidation fires for deduped routed invocations too. Head is 052fe45.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 052fe45012

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentSession.ts
Comment thread src/browser/hooks/useCompactAndRetry.ts Outdated
@asm

asm commented Aug 31, 2026

Copy link
Copy Markdown
Author

@codex review — wave-16 addressed: a final non-destructive consent check immediately before provider dispatch (rows durable by then), and staged attached-files notices survive the compact-and-retry text rebuild. Head is aa9787e.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aa9787ede8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentSession.ts Outdated
Comment thread src/node/services/agentSession.ts Outdated
Comment thread src/node/services/agentSession.ts Outdated
Comment thread src/browser/features/ChatInput/stagedAttachments.ts Outdated
Comment thread src/node/services/agentSession.ts
Comment thread src/node/services/agentSession.ts
Comment thread src/node/services/agentSession.ts Outdated
Comment thread src/common/types/message.ts
@asm

asm commented Aug 31, 2026

Copy link
Copy Markdown
Author

@codex review — wave-17 addressed: consent checked at the provider-dispatch boundary via a shared rejection closure with accepted-failure semantics and durable non-retryable marking; project-scope consent tracks content (inline + deduped refs) not just the invocation; compaction-model pricing gate; deferred routed telemetry across on-send compaction; preStreamRejected survives RLM tail copies; generated-only notice extraction. Head is 3b9ba9e.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3b9ba9ee03

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentSession.ts Outdated
Comment thread src/node/services/agentSession.ts
Comment thread src/node/services/agentSession.ts
Comment thread src/node/services/agentSession.ts Outdated
Comment thread src/node/services/agentSession.ts Outdated
Comment thread src/browser/hooks/useModelClasses.ts
@asm

asm commented Aug 31, 2026

Copy link
Copy Markdown
Author

@codex review — wave-18 addressed: preDispatchConsentGate inside AIService right before startStream, request-history project-snapshot scanning, durable row-level rejection stamps via HistoryService, consent re-verification on every resumed dispatch, dispatch-time telemetry for compaction-deferred routed skills, and live-subscription fetch retries in the editor hook. Head is 35282db.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 35282db0d9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/aiService.ts Outdated
Comment thread src/node/services/agentSession.ts
Comment thread src/node/services/agentSession.ts
Comment thread src/node/services/agentSession.ts
Comment thread src/node/services/agentSession.ts
Comment thread src/browser/features/ChatInput/index.tsx Outdated
Comment thread src/node/services/agentSession.ts
@asm

asm commented Aug 31, 2026

Copy link
Copy Markdown
Author

@codex review — wave-19 addressed: the consent gate now executes inside StreamManager's stream-start critical section (nothing awaitable remains before provider dispatch), resumed dispatches carry the same gate with request scanning, rejected-row stamping is fail-closed with in-memory quarantine + recovery re-stamp and includes @file snapshots, accepted-no-stream outcomes drain the queue, and queued skill dispatches emit backend attribution. Head is 5b7babf.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5b7babf9e1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentSession.ts Outdated
Comment thread src/node/services/agentSession.ts Outdated
Comment thread src/node/services/branchSummary.ts
Comment thread src/node/services/agentSession.ts Outdated
Comment thread src/node/services/agentSession.ts
Comment thread src/node/services/agentSession.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit: 5b7babf9e1

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Comment thread src/node/services/streamManager.ts
@asm

asm commented Aug 31, 2026

Copy link
Copy Markdown
Author

@codex review — wave-20 addressed: per-step consent in prepareStep (fallback/retry recreations included), untrusted historical snapshots filtered from routed requests instead of deterministic rejection, non-retryable consent refusals, preference-independent whole-turn quarantine repair, quarantine coverage for refine and edit summarization, and deferred-dispatch attribution dedup. Head is 3f74763.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3f74763617

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/streamManager.ts
Comment thread src/node/services/streamManager.ts
Comment thread src/node/services/refinement/refineService.ts
Comment thread src/node/services/agentSession.ts
Comment thread src/browser/features/ChatInput/index.tsx Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit: 3f74763617

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Comment thread src/node/services/streamManager.ts
@asm

asm commented Sep 1, 2026

Copy link
Copy Markdown
Author

@codex review — wave-21 addressed: fallback requests carry the per-step consent gate, consent refusals keep their non-retryable classification through the stream error pipeline, the quarantine repair walks the full epoch, refine's recheck applies the quarantine filter, and queued skill attribution is fully deferred to the backend dispatch capture. Head is 82ba4d1.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 82ba4d1aa0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/streamManager.ts
@asm

asm commented Sep 1, 2026

Copy link
Copy Markdown
Author

@codex review — wave-22 addressed: a mid-turn consent rejection now removes the in-flight assistant partial with the rejected turn (post-error-path deletion, quarantine fallback). Head is d58879e.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d58879e43f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentSession.ts
@asm

asm commented Sep 1, 2026

Copy link
Copy Markdown
Author

@codex review — wave-23 addressed: the rejected turn's surviving partial is removed during startup repair, closing the restart gap in the assistant quarantine. Head is 5f9b4c8.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5f9b4c8e80

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentSession.ts
Comment thread src/node/services/agentSession.ts
@asm

asm commented Sep 1, 2026

Copy link
Copy Markdown
Author

@codex review — wave-24 addressed: request-time quarantine repair (closing the send-vs-startup-recovery race) and the edit path's final pre-truncation consent recheck on the widened content flag. Head is b57128a.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit: 5f9b4c8e80

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Comment thread src/node/services/agentSession.ts
@asm

asm commented Sep 1, 2026

Copy link
Copy Markdown
Author

@codex review — wave-25 addressed: rejected rows (stamped + quarantined) are filtered at the memory-harvest boundary before the dream model sees the epoch. Head is 63c0b5f.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: 63c0b5ffed

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Squashed, review-hardened branch (25 Codex waves), rebased onto the
Effect refactors (config semaphore pipeline, streamManager resource
seams, memoryConsolidation, oRPC Effect Stream) and the Fable 5.1
promotion (test fixtures reference anthropic:claude-fable-5-1).

Per-skill model routing: skills bind to a class (frontmatter
metadata.model-class, or the skillModelClasses table in config.json which
wins over frontmatter), classes map to models in Settings → Models. Routing
applies to the slash invocation's send only; explicit one-shot models
bypass it, thinking-only one-shots layer on top with model-relative numeric
indices re-resolved against the routed model.

Security: project-scope skills get no class routing (frontmatter or
name-keyed table entry) in untrusted projects — repo-controlled content
must not reroute the transcript to another configured provider, and a repo
shadow must not inherit table consent given to the user's own skill.
Authoritative scope resolves from the on-disk package. Provider-selection
consent is re-verified at every irreversible step through the per-step
provider-call boundary (prepareStep, inherited by fallback/retry
recreations); revoked turns are durably stamped provider-ineligible with
in-memory quarantine plus startup/request-time repair, covering request
assembly, refine, branch summaries, and memory harvesting.

Model-class writes are per-entry atomic in the backend config transaction
(updateModelClass); the Settings editor guards freshness across client
swaps, subscription death, superseding fetches, and transient IPC errors.
Queued/deferred skill dispatches attribute telemetry at dispatch time via
the backend capture.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@asm

asm commented Sep 1, 2026

Copy link
Copy Markdown
Author

@codex review — rebased onto latest main (145003e: Fable 5.1 promotion + the Effect refactors for config semaphore pipeline, streamManager resource seams, memoryConsolidation, and oRPC streams); conflicts resolved by re-porting this branch's changes onto the refactored structures. No behavioral changes beyond the rebase.

@asm
asm force-pushed the skill-model-classes branch from 63c0b5f to af06984 Compare September 1, 2026 20:17

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: af06984ddf

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

// original send reported { queued: true } (the renderer's messageSent
// deliberately skipped), so the routed turn is attributed here, when it
// actually streams.
if (sendResult.data?.routedModel != null) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Attribute deferred skills that become unbound

When a routed skill triggers on-send compaction and its class binding is removed while compaction runs, the follow-up intentionally re-enters sendMessage() and successfully dispatches unbound on options.model. The initial { queued: true } made ChatInput suppress renderer telemetry, but this condition emits backend attribution only if the follow-up is still routed, so that successful ambient-model dispatch produces no message_sent event. Fresh evidence beyond the prior attribution fixes is that routing is deliberately re-resolved after compaction and may disappear; attribute every successful deferred skill dispatch with routedModel ?? options.model (and the corresponding thinking fallback).

Useful? React with 👍 / 👎.

Comment on lines +5886 to +5887
await this.loadAutoRetryEnabledPreference();
await this.repairUnstampedRejectedTurn();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Repair rejection state before committing partials

When a restart follows failed rejection stamping or a failed deletePartial(), a manual send can race startup recovery: acceptance clears startupAutoRetryAbandon before entering this method, and commitPartial() already ran at line 5844 before this marker-gated repair. The repair therefore either sees no marker or sees no remaining partial, allowing the unstamped rejected prompt/snapshots and committed assistant content into the new provider request. Fresh evidence after the claimed request-time fix is this rebased ordering; perform the repair before acceptance clears the marker and before commitPartial().

AGENTS.md reference: AGENTS.md:L110-L112

Useful? React with 👍 / 👎.

// A rejected row's copy must stay provider-ineligible: the boundary
// hides the original row, so a marker-less copy would re-send the
// very prompt whose pre-stream gate rejection preserved it.
...(source?.preStreamRejected === true ? { preStreamRejected: true } : {}),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Filter rejected rows before deriving compaction carryover

When a late-rejected project-skill turn is followed by compaction, performCompaction() derives pending state from raw history: preparePendingStateFromMessages() extracts the rejected synthetic skill snapshot into cachedLoadedSkills, persists it in post-compaction.json, and the next turn injects it into a provider request. This marker propagation protects only RLM tail copies and does not stop that carryover path; failed stamps have a second gap because copied rows receive new IDs absent from the in-memory quarantine. Filter both stamped and quarantined rows before any pending-state extraction or tail copying.

AGENTS.md reference: AGENTS.md:L110-L112

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit: af06984ddf

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Comment on lines +6019 to +6021
let requestCarriesProjectContent = requestMessages.some(
(msg) => msg.metadata?.agentSkillSnapshot?.scope === "project"
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Security: Scan post-compaction skill attachments for project scope

Requires a prior project-skill load and compaction, revoked Project Trust, and a later global/built-in class-routed skill. postCompactionAttachments retains each loaded skill's scope and body and is injected after requestMessages is scanned, so this history-only check returns false once the original snapshot is behind the compaction boundary; the class provider receives project instructions without a consent gate. Fresh evidence beyond the resolved history-snapshot thread is this separate attachment channel. Include project-scoped loaded skills in the gate or drop them when untrusted.

Useful? React with 👍 / 👎.

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.

1 participant