Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f9e167259
ℹ️ 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".
| return buildDevinLiveCatalogEntry( | ||
| name, | ||
| prov, | ||
| id, | ||
| provider: name, | ||
| ...(liveWindow ? { contextWindow: liveWindow } : {}), | ||
| // The account catalog names the effort variants each base model has, so | ||
| // its ladder is measured rather than assumed. Without this the entry | ||
| // inherits the generic routed ladder and offers rungs the model rounds | ||
| // away, and every client that keys an effort control off this field — | ||
| // the Pi-shaped exports — renders no control at all. | ||
| ...(liveResult.efforts[id]?.length ? { reasoningEfforts: liveResult.efforts[id] } : {}), | ||
| ...catalogHintsFromProviderConfig(name, prov, id, contextCap, metadataModelIdCaseFold, captured.effectiveAlias), | ||
| } as CatalogModel; | ||
| liveWindow, | ||
| liveResult.efforts[id], |
There was a problem hiding this comment.
When a later catalog gather hits cachedDevin, the cached rows are passed through applyConfigHintsToCachedModels, whose configuredReasoningEfforts call reapplies prov.reasoningEfforts and overwrites the account ladder stored by this new builder. Consequently, only the first return after discovery preserves the measured rungs; ordinary reads during the cache TTL advertise the degraded provider-wide ladder again. The cached path needs the same Devin-specific precedence handling.
Useful? React with 👍 / 👎.
| // A per-model setting is an operator override. The provider-wide value is | ||
| // only a degraded-mode fallback, so live account evidence supersedes it. | ||
| ...(liveEfforts?.length && modelEfforts === undefined ? { reasoningEfforts: liveEfforts } : {}), |
There was a problem hiding this comment.
When a successful account catalog exposes zero or only one reasoning variant for a base model, fetchDevinUsableModels intentionally omits that model from efforts because there is no useful control. Here, an undefined/empty liveEfforts skips the final spread and leaves hinted.reasoningEfforts set to the provider-wide fallback, so the live result advertises choices the account did not expose. Successful discovery should explicitly suppress the fallback for this case.
Useful? React with 👍 / 👎.
| const modelEfforts = modelRecordValue(prov.modelReasoningEfforts, id); | ||
| return { | ||
| id, | ||
| provider: name, | ||
| ...(liveWindow ? { contextWindow: liveWindow } : {}), | ||
| ...hinted, | ||
| // A per-model setting is an operator override. The provider-wide value is | ||
| // only a degraded-mode fallback, so live account evidence supersedes it. | ||
| ...(liveEfforts?.length && modelEfforts === undefined ? { reasoningEfforts: liveEfforts } : {}), |
There was a problem hiding this comment.
Checking only modelReasoningEfforts misses the existing per-model noReasoningModels override: catalogHintsFromProviderConfig correctly produces reasoningEfforts: [] for such a model, but this final spread replaces it with the discovered ladder. A Devin model explicitly configured to reject reasoning therefore regains an effort picker after successful discovery; the previous hints-last ordering preserved that configuration. Treat this explicit disable as authoritative as well.
AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
1 participant
Motivation
Description
buildDevinLiveCatalogEntryinsrc/codex/catalog/provider-fetch.tsthat applies live-discovered context windows and measuredeffortswhile avoiding replacement by provider-wide degraded fallbacks.buildDevinLiveCatalogEntryso live account evidence is preserved.tests/providers/devin-effort-ladder.test.tsto cover the precedence: live-account rungs win over provider fallback, and explicitmodelReasoningEffortsstill override discovery.docs-site/src/content/docs/reference/adapters.md,structure/*.md) to record that Devin live-discovered ladders are authoritative and the degraded/provider defaults are only for signed-out or failed-discovery cases.Testing
bun test tests/providers/devin-effort-ladder.test.ts tests/providers/devin-adapter.test.ts, both passed (49 tests, 0 failures).bun run typecheck), structure SSOT (bun run structure:check) and privacy scan (bun run privacy:scan), all passed, and rebuilt docs (cd docs-site && bun install --frozen-lockfile && bun run build) which succeeded.git diff/format checks andbun run privacy:scanwere clean for the changeset.bun run testrun exercised the whole suite but surfaced many unrelated environment/concurrency failures and one error outside the modified area; the focused Devin regression suite is green and validates the fix.Codex Task