feat: index storage backend (cache default, PR opt-in) - #71
Conversation
Opening a PR against the user's main branch as a side effect of running a docs review is intrusive and surprises people the first time. Add an index-storage input with three options: "cache" (saves indexes to a well-known path for Actions cache restore), "pr" (current behavior), and "none" (skip persistence). Default to "cache" so new adopters get the performance benefit without an unsolicited PR. Setting index-storage to "pr" reproduces the old behavior exactly.
|
🤖 Finished Review · ✅ Success · Started 5:53 AM UTC · Completed 6:12 AM UTC Commit: |
ReviewFindingsMedium
Low
Labels: PR adds a new index-storage feature, modifying both Python source and GitHub Action configuration. Next steps:
|
| bool: True if content was committed and PR created/updated, False otherwise | ||
| bool: True if content was persisted, False otherwise | ||
| """ | ||
| storage = os.environ.get("INDEX_STORAGE", "pr").lower() |
There was a problem hiding this comment.
[medium] default-value-mismatch
Python fallback default for INDEX_STORAGE is 'pr' (os.environ.get('INDEX_STORAGE', 'pr')), but action.yml declares the default as 'cache'. Outside the GitHub Action context (local dev, direct script execution), the Python default silently uses PR-based storage, contradicting the documented default.
Suggested fix: Change to os.environ.get('INDEX_STORAGE', 'cache').
| Returns: | ||
| bool: True if indexes/summaries were fetched, False otherwise | ||
| """ | ||
| if restore_indexes_from_cache(): |
There was a problem hiding this comment.
[medium] logic-error
fetch_indexes_from_main() unconditionally calls restore_indexes_from_cache() regardless of the INDEX_STORAGE setting. When INDEX_STORAGE is 'pr' or 'none', stale cache data from /tmp/code-to-docs-index-cache is loaded if the path exists, skipping the git-based fetch entirely.
Suggested fix: Guard with: if os.environ.get('INDEX_STORAGE', 'cache').lower() == 'cache'.
| required: false | ||
| default: '' | ||
| index-storage: | ||
| description: 'How to persist semantic indexes: "cache" (Actions cache, default), "pr" (open a PR to main), or "none" (no persistence)' |
There was a problem hiding this comment.
[medium] backward-incompatible
Default value for index-storage changes from implicit 'pr' to explicit 'cache'. Existing consumers relying on the index PR workflow will experience a silent behavior change.
Suggested fix: Keep default as 'pr' for backward compatibility, or document the migration path in README/CHANGELOG.
| bool: True if content was committed and PR created/updated, False otherwise | ||
| bool: True if content was persisted, False otherwise | ||
| """ | ||
| storage = os.environ.get("INDEX_STORAGE", "pr").lower() |
There was a problem hiding this comment.
[medium] config-centralization
INDEX_STORAGE is accessed directly via os.environ.get() instead of through a config.py getter, breaking the codebase's established pattern for testable configuration access.
Suggested fix: Add get_index_storage() to config.py.
| return updated_folders | ||
|
|
||
|
|
||
| _CACHE_MANIFEST_PATH = "/tmp/code-to-docs-index-cache" |
There was a problem hiding this comment.
[medium] undocumented-cache-contract
The cache backend saves to /tmp/code-to-docs-index-cache but no documentation shows consumers how to configure the required GitHub Actions cache steps for this path.
Suggested fix: Expose the cache path via action.yml outputs, or document the cache workflow configuration in README.
| bool: True if content was committed and PR created/updated, False otherwise | ||
| bool: True if content was persisted, False otherwise | ||
| """ | ||
| storage = os.environ.get("INDEX_STORAGE", "pr").lower() |
There was a problem hiding this comment.
[low] missing-validation
No validation of INDEX_STORAGE value. An unrecognized value silently falls through to the PR-creation path.
| return True | ||
|
|
||
|
|
||
| def commit_indexes_to_repo(content_type="indexes"): |
There was a problem hiding this comment.
[low] scope-expansion
commit_indexes_to_repo() now handles both storage routing and PR-based persistence. Consider extracting backends into separate functions.
Summary
Changes how semantic indexes are persisted. The current default (opening a PR against main) is intrusive. New
index-storageinput with three options:cache(new default): saves indexes to a well-known path for Actions cache. Same performance benefit, no unsolicited PR.pr: current behavior, unchanged. For users who want indexes committed to the repo.none: skip persistence entirely.Behavior change: default flips from
prtocache. Existing users who rely on the index PR should setindex-storage: prexplicitly.Test plan
uv run pytest -vpasses (417 tests)index-storage: prreproduces old behavior