Skip to content

fix: #3174 count valid encrypted session items for limits#3175

Merged
seratch merged 2 commits intoopenai:mainfrom
Aphroq:fix/encrypted-session-limit-valid-items
May 7, 2026
Merged

fix: #3174 count valid encrypted session items for limits#3175
seratch merged 2 commits intoopenai:mainfrom
Aphroq:fix/encrypted-session-limit-valid-items

Conversation

@Aphroq
Copy link
Copy Markdown
Contributor

@Aphroq Aphroq commented May 7, 2026

Summary

  • Make positive effective EncryptedSession.get_items() limits count valid decrypted items instead of raw encrypted envelopes, whether the limit is passed explicitly or comes from the wrapped session's SessionSettings.
  • Expand the underlying fetch window when invalid, expired, or undecryptable envelopes are encountered, stopping once enough valid items are found or the underlying history is exhausted.
  • Add regression coverage for explicit limits, SessionSettings(limit=...), invalid latest envelopes, and mixed valid/invalid ordering.

This keeps limit=None with no configured limit and limit<=0 on the previous direct delegation path. Negative limit semantics are a separate cross-backend session-limit consistency issue and are intentionally out of scope here.

Test plan

  • uv run pytest tests/extensions/memory/test_encrypt_session.py -k "session_settings_limit_skips_invalid_envelopes or limit_skips_invalid_latest_envelope or latest_valid_items_after_invalids or get_items_limit"
  • uv run pytest tests/extensions/memory/test_encrypt_session.py
  • bash .agents/skills/code-change-verification/scripts/run.sh

Issue number

Closes #3174

Checks

  • I've added new tests (if relevant)
  • I've added/updated the relevant documentation
  • I've run make lint and make format
  • I've made sure tests pass

@github-actions github-actions Bot added bug Something isn't working feature:extensions labels May 7, 2026
Copy link
Copy Markdown
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the explicit get_items(limit=...) path. I think the same issue still remains when the limit comes from the underlying session's SessionSettings.

For example, if the underlying session is configured with SessionSettings(limit=3), EncryptedSession.get_items() currently calls underlying_session.get_items(None), which delegates to the underlying limit and returns only the latest three encrypted envelopes. If one of those latest envelopes is invalid, an older valid item can still be hidden.

I reproduced this with:

  • valid 0
  • valid 1
  • valid 2
  • invalid encrypted envelope

With SessionSettings(limit=3), await session.get_items() returns ["valid 1", "valid 2"], but it should return ["valid 0", "valid 1", "valid 2"].

Could you update the implementation to resolve the effective limit from both the explicit limit argument and self.session_settings, then use the same expanding-window logic for that effective limit? A regression test for the SessionSettings(limit=...) path would cover this.

@Aphroq Aphroq changed the title fix: count valid encrypted session items for limits fix: #3174 count valid encrypted session items for limits May 7, 2026
Copy link
Copy Markdown

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

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: 57630d310f

ℹ️ 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/agents/extensions/memory/encrypt_session.py Outdated
@Aphroq Aphroq force-pushed the fix/encrypted-session-limit-valid-items branch from 57630d3 to 5f08b17 Compare May 7, 2026 05:36
Copy link
Copy Markdown
Contributor Author

Aphroq commented May 7, 2026

Thanks for the review. I updated this PR in 5f08b17a to cover the configured-limit path as well:

  • EncryptedSession.get_items() now resolves the effective limit from both the explicit argument and the wrapped session's SessionSettings.
  • Positive effective limits use the same expanding-window logic, so invalid encrypted envelopes no longer count against either explicit or configured limits.
  • Added a regression test for SessionSettings(limit=3) with valid 0, valid 1, valid 2, and an invalid latest envelope.

@seratch seratch added this to the 0.16.x milestone May 7, 2026
@seratch seratch merged commit a67d95f into openai:main May 7, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working feature:extensions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EncryptedSession limit can return fewer valid items when latest envelopes are invalid

2 participants