Skip to content

fix(stores): add caching to CliSettings.load() to reduce disk I/O#762

Open
github-actions[bot] wants to merge 3 commits into
mainfrom
fix/code-quality/cli-settings-caching
Open

fix(stores): add caching to CliSettings.load() to reduce disk I/O#762
github-actions[bot] wants to merge 3 commits into
mainfrom
fix/code-quality/cli-settings-caching

Conversation

@github-actions

@github-actions github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds LRU caching to CliSettings.load() to prevent repeated disk reads when loading CLI settings.

Changes

  • Added @lru_cache to internal _load_cli_settings() function
  • Cache is keyed by config file path for test isolation
  • Cache is automatically invalidated when save() is called
  • Added invalidate_cache() class method for explicit cache clearing

Related Issue

Addresses findings from #761 (Code Quality Report - Low-Hanging Fruit)

Testing

  • Linting passes (make lint)
  • Tests pass (make test - 1353 tests)

This PR was automatically generated by the Code Quality Report workflow.


🚀 Try this PR

uvx --python 3.12 git+https://github.com/OpenHands/OpenHands-CLI.git@fix/code-quality/cli-settings-caching

Addresses item from code quality report.

- Added @lru_cache to _load_cli_settings() function
- Cache is keyed by config file path for test isolation
- Cache is automatically invalidated when save() is called
- Added invalidate_cache() class method for explicit cache clearing

Closes #761

Co-authored-by: openhands <openhands@all-hands.dev>
@enyst

enyst commented Jun 1, 2026

Copy link
Copy Markdown
Member

@OpenHands make an empty commit to kick in CI, then /codereview this PR and /iterate to green and approved from AI reviewer bot

@openhands-ai

openhands-ai Bot commented Jun 1, 2026

Copy link
Copy Markdown

I'm on it! enyst can track my progress at all-hands.dev

enyst and others added 2 commits June 1, 2026 14:08
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>

@enyst enyst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟢 Good taste - Simple, targeted cache extraction with explicit invalidation and path-keyed isolation.

The implementation keeps the persistence behavior intact while removing repeated disk reads from hot call sites. save() clears the cache, callers that need to observe out-of-band file edits now have CliSettings.invalidate_cache(), and the added unit coverage exercises cache hits, explicit invalidation, save invalidation, and path-key isolation.

[RISK ASSESSMENT]

  • [Overall PR] ⚠️ Risk Assessment: 🟢 LOW
    Small, localized settings-store change with no public API removal. The main behavioral change is intentional caching; the invalidation path and tests cover the practical stale-read cases.

VERDICT:
Worth merging: Core logic is sound and now has focused regression coverage.

KEY INSIGHT:
The right abstraction here is keeping disk I/O behind a single cached loader while making invalidation explicit at every write boundary.


This review was created by an AI agent (OpenHands) on behalf of the requester.

@github-actions

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

Coverage

Coverage Report •
FileStmtsMissCoverMissing
openhands_cli/stores
   cli_settings.py760100% 
TOTAL7072101185% 

@openhands-ai

openhands-ai Bot commented Jun 1, 2026

Copy link
Copy Markdown

Since the last summary, there were no additional repository or PR changes.

Checklist:

  • ✅ The requested PR work was completed: CI was retriggered, the PR was reviewed, and the PR was iterated to green/approved.
  • ✅ No further instructions were pending.
  • ✅ No extraneous changes were made after the last summary, so there is nothing to revert.
  • ✅ The final state remains: PR fix(stores): add caching to CliSettings.load() to reduce disk I/O #762 is green, approved, mergeable, and has no unresolved review threads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant