fix(sessions): accept base SessionSettings overrides on subclasses - #4820
fix(sessions): accept base SessionSettings overrides on subclasses#4820rioyu123 wants to merge 2 commits into
Conversation
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. |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
isinstance(value, SessionSettings) looks too broad here. It fixes the base-class override, but it also lets an unrelated sibling subclass bypass settings_type validation. resolve() then copies any same-named fields via getattr, so _TenantSessionSettings.resolve(_OtherSessionSettings(...)) can silently apply settings from the wrong type instead of rejecting them. Could this accept only the exact base SessionSettings or an instance of settings_type? A sibling-subclass regression would pin the boundary.
|
Thanks — confirmed. I narrowed the fast path to the exact base SessionSettings or an instance of settings_type, so unrelated sibling subclasses still raise TypeError. I also added direct and Runner-level sibling regressions, including asserting that the model is not invoked. |
SessionSettings.resolve() rejected the exact base SessionSettings override when the stored settings object was a subclass. RunConfig also normalizes dictionary overrides to the base type, so both public override forms failed through Runner.run. Accept the exact base type or an instance compatible with the requested settings type, while preserving validation for unrelated sibling subclasses. Treat missing subclass-only override fields as unset so the stored subtype and its extra fields survive. Add direct and Runner-level regressions for typed and dictionary overrides, sibling rejection, and failure before model invocation.
414c7a0 to
6f69d5c
Compare
![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: 6f69d5cb49
ℹ️ 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".
| if type(value) is SessionSettings or isinstance(value, settings_type): | ||
| return value |
There was a problem hiding this comment.
When an application subclasses RunConfig and narrows session_settings to a custom SessionSettings subtype, RunConfig.__post_init__ passes that declared subtype to this shared helper. The new exact-base exception returns a plain SessionSettings unchanged, so the field no longer has its declared normalized type and downstream provider code can fail when accessing subtype fields; previously coerce_dataclass_config rejected this value. Restrict the base-instance exception to the SessionSettings.resolve() overlay path rather than changing constructor coercion globally.
AGENTS.md reference: AGENTS.md:L145-L148
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks — good catch. I moved the exact-base SessionSettings exception into SessionSettings.resolve() only and restored _coerce_session_settings() to strict declared-type coercion. I also added RunConfig subclass regressions proving that a compatible dict normalizes to the declared settings subtype while an incompatible base instance is rejected. The focused session/RunConfig suite now passes 107 tests.
2 participants
Summary
SessionSettings.resolve()rejected a baseSessionSettingsoverride when the stored settings object was a subclass. This also broke the publicRunner.run(..., run_config=RunConfig(session_settings=...))path because a dict override is first normalized to the base class.This change preserves the concrete session-settings subtype while allowing the exact base
SessionSettingsoverride specifically at theresolve()overlay boundary. It also keeps the existing construction boundary intact: aRunConfigfield narrowed to a custom settings subtype still normalizes dictionaries to that subtype and rejects incompatible base or sibling instances before session history is read or the model is invoked.Value
SessionSettingssubclasses.RunConfigsubclasses.Implementation
SessionSettingsobject only insideSessionSettings.resolve()before subtype coercion._coerce_session_settings()delegated tocoerce_dataclass_config, preserving strict declared-type normalization everywhere else.None, so the stored subclass value is retained.Test plan
RunConfigsubclass coverage proving dictionaries normalize to the declared settings subtype and incompatible base instances are rejected.107 passed.8329 passed, 81 skipped, 19 failed; the 19 failures match the untouched baseline on this host. Two locale-dependent failures pass withPYTHONUTF8=1; the remaining 17 require Windows symbolic-link privileges unavailable to this session.Issue number
Closes #4819
Checks
.agents/skills/code-change-verification/scripts/run.sh(GNUmakeis unavailable on this Windows host; I ran the equivalent Makefile commands directly)/reviewbefore submitting this PR