fix: model fallback to config.llm when resolveAgentPrimaryModelRef returns undefined (PR #618 continuation)#713
Conversation
![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: b76f5afa6c
ℹ️ 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".
| ?? (llmConfig?.model as string | undefined); | ||
| const split = modelRef ? splitProviderModel(modelRef) : { provider: undefined, model: undefined }; |
There was a problem hiding this comment.
generateReflectionText now pulls llmConfig?.model through a type cast and then calls splitProviderModel(modelRef) without runtime type checks. If a user config provides any non-string truthy llm.model (for example a number/object), splitProviderModel will call .trim() on it and throw, which makes the embedded reflection path fail and forces CLI/fallback execution. Please guard this fallback with typeof === "string"/asNonEmptyString before splitting.
Useful? React with 👍 / 👎.
…romBaseURL - Add inferProviderFromBaseURL() using hostname.endsWith() to prevent subdomain spoofing - Modify generateReflectionText to fallback to config.llm.model when resolveAgentPrimaryModelRef returns undefined - Add regression test to CI manifest
jlin53882
force-pushed
the
fix/pr618-clean-v2
branch
from
b76f5af to
ea52a90
Compare
…iderModel Address Codex review feedback: - Guard modelRefFromConfig with typeof === 'string' to prevent runtime errors when user config provides non-string values (number/object)
…icts Conflict resolution: - scripts/ci-test-manifest.mjs: keep both to-import-specifier-windows.test.mjs (removed in master f194d79 rebase, preserved here) and infer-provider-from-baseurl.test.mjs - scripts/verify-ci-test-manifest.mjs: same resolution Both test entries belong in core-regression group.
…oduction in test, add fallback comments - Export inferProviderFromBaseURL for unit testing (matches other exported utilities) - Rewrite test to jiti-import the actual function instead of duplicating logic - Add inline comments explaining model/provider resolution chain in generateReflectionText - Fix test header: PR CortexReach#618 -> PR CortexReach#713 - Add URL path variation test cases (deep path, no path)
Conflicts resolved: - index.ts: combine HEAD model fallback + inferProviderFromBaseURL with master passing params.api - ci-test-manifest.mjs: take master additions - verify-ci-test-manifest.mjs: take master additions
…onMode + PendingRecall hooks), sync CI manifest, add issue606 to verify baseline
jlin53882
force-pushed
the
fix/pr618-clean-v2
branch
from
ca596f4 to
be4ba28
Compare
…ncy on chunker.ts (PR CortexReach#713)
1 participant
Summary
This PR continues the work from PR #618 with a clean diff.
Changes
index.ts - Add
inferProviderFromBaseURL()function:.endsWith(".suffix")to prevent subdomain spoofinggenerateReflectionText() - Model fallback:
resolveAgentPrimaryModelRefreturns undefined, fallback toconfig.llm.modelconfig.llm.baseURLCI manifest - Add regression test entry
Fixes from Original PR #618
Original PR
Please close PR #618 as this PR supersedes it.
/cc @app3apps