Skip to content

Conversation

@bolinfest
Copy link
Collaborator

@bolinfest bolinfest commented Dec 3, 2025

I noticed that features: Features was defined on struct SessionConfiguration, which is commonly owned by SessionState, which is in turn owned by Session.

Though I do not believe that Features should be allowed to be modified over the course of a session (if the feature state is not invariant, it makes it harder to reason about), which argues that it should live on Session rather than SessionState or SessionConfiguration.

This PR moves Features to Session and updates all call sites. It appears the only place we were mutating Features was:

  • in tests
  • the sub-agent config for a review task:

sub_agent_config
.features
.disable(crate::features::Feature::WebSearchRequest)
.disable(crate::features::Feature::ViewImageTool);

Note this change also means it is no longer an async call to check the state of a feature, eliminating the possibility of a TOCTTOU error between checking the state of a feature and acting on it:

pub async fn enabled(&self, feature: Feature) -> bool {
self.state
.lock()
.await
.session_configuration
.features
.enabled(feature)
}

@bolinfest bolinfest merged commit 1cfc967 into main Dec 4, 2025
71 of 73 checks passed
@bolinfest bolinfest deleted the pr7540 branch December 4, 2025 00:12
@github-actions github-actions bot locked and limited conversation to collaborators Dec 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants