Repository navigation
refactor(layout): batch1 low-risk relocations - #591
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR reorganizes storage persistence, execution, and scheduling subsystems into dedicated modules with comprehensive durable backing. It introduces workspace archive utilities, external executor adapters with PostgreSQL persistence, scheduled-run planning with proactive signals, and owner-fenced SQLite/PostgreSQL durable stores, while converting legacy modules into backward-compatible shims. ChangesStorage, Execution, and Scheduling Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
coding-agent/src/coding_agent/stores/durable_pg.py (1)
727-738: 💤 Low valueUnused
for_updateparameter.The
for_updateparameter is accepted but immediately deleted and ignored—the query always usesFOR UPDATEregardless. Either remove the parameter or implement the conditional behavior.♻️ Proposed fix to remove unused parameter
async def _session_id_for_run( self, connection: Any, run_id: str, - *, - for_update: bool, ) -> str | None: - del for_update row = await connection.fetchrow(self._SELECT_RUN_SESSION_FOR_UPDATE_SQL, run_id) if row is None: return None return _required_str(dict(row), "session_id")Also update the caller at line 746-749:
session_id = await self._session_id_for_run( connection, run_id, - for_update=True, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/src/coding_agent/stores/durable_pg.py` around lines 727 - 738, The method _session_id_for_run currently accepts a for_update parameter but immediately deletes it and always executes _SELECT_RUN_SESSION_FOR_UPDATE_SQL (which implies FOR UPDATE); either remove the unused for_update parameter from _session_id_for_run and update all callers to stop passing that argument (and optionally rename _SELECT_RUN_SESSION_FOR_UPDATE_SQL to remove the “FOR_UPDATE” wording), or implement the conditional behavior by keeping for_update and switching the query used (e.g., choose between _SELECT_RUN_SESSION_FOR_UPDATE_SQL and a non-locking _SELECT_RUN_SESSION_SQL) so the SQL reflects the for_update value; locate and update any callers that pass the extra argument to match the chosen fix.coding-agent/src/coding_agent/executors/external.py (1)
477-485: 💤 Low valueIntentional broad exception catch – consider adding a comment.
The bare
Exceptioncatch (flagged by Ruff BLE001) is appropriate here: the runner may raise arbitrary exceptions from user code, and sanitizing these prevents leaking sensitive information. A brief comment would clarify the intent.📝 Suggested comment
- except Exception as exc: + except Exception as exc: # Catch-all: sanitize unexpected runner errors🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/src/coding_agent/executors/external.py` around lines 477 - 485, Add a short inline comment above the "except Exception as exc:" block explaining that the broad Exception catch is intentional to safely capture and sanitize any errors coming from user code (Ruff BLE001 is acceptable here) and prevent leaking sensitive details; mention that the handler returns a sanitized ExecutorResult (see the ExecutorResult(...) construction with sanitized_summary and metadata "phase": "local_runner_failed") and therefore no raw exception info is propagated.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@coding-agent/src/coding_agent/environment/archive.py`:
- Around line 36-46: The archive path checks use check-then-use filesystem calls
(path.is_symlink(), path.is_file(), path.stat(), path.read_bytes()) which are
vulnerable to TOCTOU if the workspace is mutated concurrently; fix by acquiring
the exclusive workspace lock around the entire walk-and-read sequence (or pause
the container) and, for each file, perform no-follow checks and revalidation
before IO: use lstat/no-follow semantics to detect symlinks, verify the file's
resolved realpath remains inside the expected workspace_root (e.g., compare
Path.resolve(strict=True) or os.fstat on an opened FD) immediately before read,
open or read with follow_symlinks=False (or via an FD opened with O_NOFOLLOW) to
prevent symlink races, and keep calling _raise_if_archive_too_large as before;
apply the same locking + no-follow/revalidate pattern to the analogous block at
lines 147-157.
- Around line 31-50: The archive creation loop doesn't enforce the member-count
cap (_MAX_WORKSPACE_ARCHIVE_MEMBERS), so many tiny files can produce an archive
that later fails extraction; add a member counter (e.g., member_count = 0 before
the loop), increment it each time you add a file (where total_input_bytes is
updated / before archive.addfile), and if member_count >
_MAX_WORKSPACE_ARCHIVE_MEMBERS raise the same error used on extract (or raise
ValueError) to stop creation; reference the constant
_MAX_WORKSPACE_ARCHIVE_MEMBERS and ensure the check is performed in the loop
alongside the existing total_input_bytes/_raise_if_archive_too_large check.
In `@coding-agent/src/coding_agent/runs/scheduled.py`:
- Around line 163-175: The summary field is declared as required (summary: str)
but __post_init__ calls _require_optional_display_text which allows None; change
the validation to enforce non-None by replacing the call to
_require_optional_display_text("summary", self.summary) with a required
validator (e.g., _require_display_text("summary", self.summary)) or add an
explicit check like if self.summary is None: raise ValueError(...) before
calling the existing validator so that Summary in runs/scheduled.py (inside
__post_init__) cannot be None at runtime.
- Around line 77-84: The _SECRET_VALUE_MARKERS tuple in runs/scheduled.py is
missing several markers and treats "sk-" differently from the logic used
elsewhere; update _SECRET_VALUE_MARKERS to include "bearer ", "gho_", "ghp_",
"kubeconfig", and "private key" and change the detection logic used when
scanning folded values to use substring matching (e.g., treat any marker found
via "marker in folded" rather than only folded.startswith("sk-")), so the
behavior of the _SECRET_VALUE_MARKERS constant and its usage matches the
executor path (or alternatively add a clear comment documenting the intentional
difference if you must keep distinct behavior). Ensure you modify the scanning
code that uses _SECRET_VALUE_MARKERS in runs/scheduled.py to use the same
membership check as executors/external.py (and update tests/comments
accordingly).
In `@coding-agent/src/coding_agent/stores/migration.py`:
- Around line 130-140: The checkpoint migration currently overwrites existing
checkpoints whenever existing != snapshot; update the migration logic in the
function that iterates checkpoints (references: target_store.load,
target_store.save, SQLiteCheckpointStore, variables existing and snapshot) to
respect a new boolean parameter replace_checkpoints (default false) or, if you
prefer the minimal change, emit a warning log and skip overwriting when
replace_checkpoints is false; specifically, add the replace_checkpoints
parameter to the migration call site and signature, check it before calling
await target_store.save(snapshot) (only save when replace_checkpoints is true or
existing is None), and when skipping an overwrite emit a warning with
identifying checkpoint_id so behavior matches the tape migration semantics.
---
Nitpick comments:
In `@coding-agent/src/coding_agent/executors/external.py`:
- Around line 477-485: Add a short inline comment above the "except Exception as
exc:" block explaining that the broad Exception catch is intentional to safely
capture and sanitize any errors coming from user code (Ruff BLE001 is acceptable
here) and prevent leaking sensitive details; mention that the handler returns a
sanitized ExecutorResult (see the ExecutorResult(...) construction with
sanitized_summary and metadata "phase": "local_runner_failed") and therefore no
raw exception info is propagated.
In `@coding-agent/src/coding_agent/stores/durable_pg.py`:
- Around line 727-738: The method _session_id_for_run currently accepts a
for_update parameter but immediately deletes it and always executes
_SELECT_RUN_SESSION_FOR_UPDATE_SQL (which implies FOR UPDATE); either remove the
unused for_update parameter from _session_id_for_run and update all callers to
stop passing that argument (and optionally rename
_SELECT_RUN_SESSION_FOR_UPDATE_SQL to remove the “FOR_UPDATE” wording), or
implement the conditional behavior by keeping for_update and switching the query
used (e.g., choose between _SELECT_RUN_SESSION_FOR_UPDATE_SQL and a non-locking
_SELECT_RUN_SESSION_SQL) so the SQL reflects the for_update value; locate and
update any callers that pass the extra argument to match the chosen fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 86b81e17-2231-4884-904f-2c05b4ebc9de
📒 Files selected for processing (13)
coding-agent/src/coding_agent/environment/archive.pycoding-agent/src/coding_agent/environment/docker_workspace_provider.pycoding-agent/src/coding_agent/executors/external.pycoding-agent/src/coding_agent/external_executor.pycoding-agent/src/coding_agent/local_durable_store.pycoding-agent/src/coding_agent/pg_durable_store.pycoding-agent/src/coding_agent/runs/scheduled.pycoding-agent/src/coding_agent/scheduled_runs.pycoding-agent/src/coding_agent/storage_migration.pycoding-agent/src/coding_agent/stores/durable_local.pycoding-agent/src/coding_agent/stores/durable_pg.pycoding-agent/src/coding_agent/stores/migration.pycoding-agent/src/coding_agent/workspace_archive.py
This was referenced Jun 14, 2026
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.
What
Batch 1 of the package-layout consolidation: relocate 6 low-risk (≤2 refs) top-level modules in
src/coding_agent/into their cohesive subpackages. Old import paths are preserved via deprecation shims — no call-site changes, so this is a pure move.local_durable_store.pystores/durable_local.pypg_durable_store.pystores/durable_pg.pystorage_migration.pystores/migration.pyexternal_executor.pyexecutors/external.pyworkspace_archive.pyenvironment/archive.pyscheduled_runs.pyruns/scheduled.pyCompat mechanism
Each old path is now a shim that aliases the moved module via
sys.modules[__name__] = _moved, preserving every public/private name andisinstanceidentity.from coding_agent.<old> import Xkeeps working.One eager old-path import in
environment/docker_workspace_provider.pywas repointed to.archiveto break a circular import (otherwise theworkspace_archiveshim could not use the standard alias form).Verification
uv run pytest -q: 3725 passed, 8 skipped, 20 xfailedcoding_agent.workspace_archive is coding_agent.environment.archive→Truein both import orderings.Spec / task packet:
.opencode/prompts/tasks/package-layout-consolidation-task-packet.md. This is batch 1 of 4.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores