Fix inconsistent RepoManager base path in RepositoriesResource (Closes #640)#652
Conversation
WalkthroughRepoManager constructor signature was updated to accept a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
potpie/resources/repositories.py (1)
120-123:⚠️ Potential issue | 🟠 MajorExpose
_get_repo_local_pathas a public method or avoid the private API access.The fallback accesses
RepoManager._get_repo_local_path(), a private method with no public equivalent inIRepoManager. If this method is renamed or its return type changes, the fallback will silently fail or raiseAttributeError(which may be swallowed by exception handlers). Either add a public method toIRepoManagerfor this lookup (this method is legitimately needed bysync_helper.pyandparsing_helper.pyas well), or drop this fallback and returnNone(the tworm.get_repo_path(...)calls above already handle the primary use cases).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@potpie/resources/repositories.py` around lines 120 - 123, The fallback is calling the private RepoManager._get_repo_local_path(repo_name); either add a public accessor on IRepoManager (e.g., get_repo_local_path or repo_local_path) and replace rm._get_repo_local_path(...) with rm.get_repo_local_path(repo_name) (also update callers in sync_helper.py and parsing_helper.py), or remove the fallback entirely and return None so we rely on the existing rm.get_repo_path(...) semantics; ensure the chosen public method signature/return type matches other usages and update interface and implementations accordingly.
🧹 Nitpick comments (3)
potpie/resources/repositories.py (3)
48-72:_get_repo_managerre-evaluates the env var on every call when disabled.
self._repo_manageris only cached on successful initialization. WhenREPO_MANAGER_ENABLEDis false (or initialization fails), the env var is parsed from scratch on everycreate_worktreecall.♻️ Cache the disabled state with a sentinel
+_REPO_MANAGER_DISABLED = object() # sentinel def _get_repo_manager(self): """Get RepoManager instance if enabled.""" - if self._repo_manager is not None: + if self._repo_manager is _REPO_MANAGER_DISABLED: + return None + if self._repo_manager is not None: return self._repo_manager enabled = os.getenv("REPO_MANAGER_ENABLED", "false").lower() in ( "true", "1", "yes", "y", ) if not enabled: + self._repo_manager = _REPO_MANAGER_DISABLED return None try: from app.modules.repo_manager import RepoManager self._repo_manager = RepoManager( repos_base_path=self._config.repos_base_path ) logger.info("RepositoriesResource: RepoManager initialized") return self._repo_manager except Exception as e: logger.warning( "RepositoriesResource: Failed to initialize RepoManager: %s", e ) + self._repo_manager = _REPO_MANAGER_DISABLED return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@potpie/resources/repositories.py` around lines 48 - 72, The _get_repo_manager currently re-parses REPO_MANAGER_ENABLED every call because self._repo_manager is only set on success; change it to cache a disabled sentinel so disabled or failed initialization is remembered: set self._repo_manager to a sentinel value (e.g., False or a dedicated object) when enabled is false or when initialization fails, and update callers (e.g., create_worktree) to treat that sentinel as "disabled" and return None without re-checking the env var; keep successful initialization storing the RepoManager instance in self._repo_manager as before.
257-258: Replaceasyncio.get_event_loop()withasyncio.get_running_loop()inside a coroutine.
asyncio.get_event_loop()when called from a coroutine returns the running event loop, so this doesn't break today. However,asyncio.get_event_loop()is deprecated since Python 3.10 when called without a running loop, and is planned to become an alias forget_running_loop()in the future. Sincecreate_worktreeisasync,asyncio.get_running_loop()is the semantically correct and forward-compatible replacement.♻️ Proposed fix
- loop = asyncio.get_event_loop() + loop = asyncio.get_running_loop()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@potpie/resources/repositories.py` around lines 257 - 258, In the async function create_worktree replace the deprecated call asyncio.get_event_loop() with asyncio.get_running_loop() before calling loop.run_in_executor so the coroutine obtains the current running loop; update the symbol usage in the create_worktree function (where loop is used with run_in_executor to compute path) to call asyncio.get_running_loop() instead of asyncio.get_event_loop().
109-113: Extract the repeated commit-SHA heuristic into a helper.The pattern
len(ref) >= 7 and all(c in "0123456789abcdefABCDEF" for c in ref[:7])appears identically at lines 109–113, 180–182, and 269–271. A single module-level helper removes the duplication and makes future refinements (e.g., checking full SHA length) a one-line change.♻️ Proposed refactor
+def _is_commit_sha(ref: str) -> bool: + """Heuristic: treat ref as a commit SHA if its first 7 chars are all hex digits.""" + return len(ref) >= 7 and all(c in "0123456789abcdefABCDEF" for c in ref[:7]) + + class RepositoriesResource(BaseResource): ...Then replace each inline occurrence:
- is_commit = len(ref) >= 7 and all( - c in "0123456789abcdefABCDEF" for c in ref[:7] - ) + is_commit = _is_commit_sha(ref)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@potpie/resources/repositories.py` around lines 109 - 113, Extract the repeated SHA-guessing logic into a single module-level helper (e.g., is_commit_sha(ref) or looks_like_commit_sha) that returns the boolean result of the existing heuristic, then replace each inline occurrence that sets is_commit (the expressions in the blocks that compute `is_commit = len(ref) >= 7 and all(c in "0123456789abcdefABCDEF" for c in ref[:7])`) with a call to that helper and adjust subsequent uses of branch/commit_id accordingly; update all three places where that exact expression appears so future changes to the heuristic are made in one location.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@potpie/resources/repositories.py`:
- Around line 120-123: The fallback is calling the private
RepoManager._get_repo_local_path(repo_name); either add a public accessor on
IRepoManager (e.g., get_repo_local_path or repo_local_path) and replace
rm._get_repo_local_path(...) with rm.get_repo_local_path(repo_name) (also update
callers in sync_helper.py and parsing_helper.py), or remove the fallback
entirely and return None so we rely on the existing rm.get_repo_path(...)
semantics; ensure the chosen public method signature/return type matches other
usages and update interface and implementations accordingly.
---
Nitpick comments:
In `@potpie/resources/repositories.py`:
- Around line 48-72: The _get_repo_manager currently re-parses
REPO_MANAGER_ENABLED every call because self._repo_manager is only set on
success; change it to cache a disabled sentinel so disabled or failed
initialization is remembered: set self._repo_manager to a sentinel value (e.g.,
False or a dedicated object) when enabled is false or when initialization fails,
and update callers (e.g., create_worktree) to treat that sentinel as "disabled"
and return None without re-checking the env var; keep successful initialization
storing the RepoManager instance in self._repo_manager as before.
- Around line 257-258: In the async function create_worktree replace the
deprecated call asyncio.get_event_loop() with asyncio.get_running_loop() before
calling loop.run_in_executor so the coroutine obtains the current running loop;
update the symbol usage in the create_worktree function (where loop is used with
run_in_executor to compute path) to call asyncio.get_running_loop() instead of
asyncio.get_event_loop().
- Around line 109-113: Extract the repeated SHA-guessing logic into a single
module-level helper (e.g., is_commit_sha(ref) or looks_like_commit_sha) that
returns the boolean result of the existing heuristic, then replace each inline
occurrence that sets is_commit (the expressions in the blocks that compute
`is_commit = len(ref) >= 7 and all(c in "0123456789abcdefABCDEF" for c in
ref[:7])`) with a call to that helper and adjust subsequent uses of
branch/commit_id accordingly; update all three places where that exact
expression appears so future changes to the heuristic are made in one location.
2 participants
RepositoriesResource initialized RepoManager without passing
RuntimeConfig.repos_base_path, causing worktree creation to use
a different repo directory than parsing/runtime flows.
This change wires repos_base_path into RepoManager initialization
so all runtime components use the same repository base path.
Closes #640
Summary by CodeRabbit