Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes the copilot-usage session <prefix> command by adding a directory-name pre-filter so it can skip parsing most events.jsonl files when searching for a UUID session ID prefix, addressing #138’s O(n) full-parse behavior.
Changes:
- Add a fast pre-filter in
sessionto skip parsing UUID-shaped session directories that cannot match the requested prefix (for prefixes ≥ 4 chars). - Add a “no match” fallback pass to collect available session IDs for the error message.
- Add unit tests that validate the pre-filter behavior using monkeypatched
discover_sessions/parse_events.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/copilot_usage/cli.py |
Adds the UUID-directory pre-filter and a second-pass collection of available IDs for error output. |
tests/copilot_usage/test_cli.py |
Adds tests validating the pre-filter skip logic, short-prefix behavior, and non-UUID directory behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
microsasa
added
aw-quality-gate-approved
Contributor
|
Commit pushed:
|
…138) Add a directory-name pre-filter to the session command that skips parsing events.jsonl files for UUID-named directories that clearly cannot match the requested session ID prefix (when prefix >= 4 chars). Non-UUID directories (e.g. test fixtures like 'corrupt-session') are always parsed to preserve correctness. When no match is found, a second pass collects available session IDs for the error message. Tests added: - Verify parse_events is only called for the matching directory with >= 5 UUID-named sessions and prefix >= 4 chars - Verify short prefixes (< 4 chars) bypass the pre-filter - Verify non-UUID directory names are always parsed Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Collect available session IDs during the first loop to avoid a second full parse_events/build_session_summary pass on the error path. For UUID-shaped dirs skipped by the pre-filter, derive available from dir_name[:8] without parsing. - Remove _write_uuid_session() and extend _write_session() with use_full_uuid_dir parameter to reduce duplicated fixture setup. - Fix flaky short-prefix test by using deterministic reverse-name ordering and asserting that the non-matching cd333… dir was parsed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
microsasa
force-pushed
the
fix/138-session-prefilter-007def80bda329a3
branch
from
3ab4bee to
d66ab37
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
microsasa
added
aw-quality-gate-approved
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Quality Gate: APPROVED ✅
Code Quality: Good
- Clean pre-filter optimization in the
sessioncommand loop — skips expensiveparse_eventscalls for UUID-shaped directories that clearly don't match the prefix - Sensible guardrails: short prefixes (< 4 chars) bypass the filter, non-UUID dirs always parsed
availablelist built during the main loop avoids a second full-parse pass on the error path- Three meaningful tests cover the key scenarios (matching, short prefix, non-UUID dirs)
Impact: LOW
- Performance optimization only — no behavioral, API, or dependency changes
- 2 files changed (14 lines in
cli.py, 146 lines of tests) - Existing + new test coverage is solid
Auto-approving for merge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #138
Problem
The
sessioncommand iterates through all session paths, fully parsing everyevents.jsonland building aSessionSummaryjust to check if the session ID starts with the given prefix. With many sessions, this is O(n) parse work instead of O(1).Solution
Added a directory-name pre-filter in the
sessioncommand loop:corrupt-session,empty-session) are always parsed — their names don't correspond to session IDsTests
Three new tests:
test_session_prefilter_skips_non_matching_dirs— mocksparse_eventsand verifies it's only called once (for the matching directory) with ≥ 5 UUID-named sessions and prefix ≥ 4 charstest_session_prefilter_short_prefix_parses_all— verifies short prefixes (< 4 chars) bypass the pre-filtertest_session_prefilter_non_uuid_dirs_always_parsed— verifies non-UUID directory names are always parsedAll 410 tests pass. Coverage: 98%.
Warning
The following domain was blocked by the firewall during workflow execution:
astral.shTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.