workspace: fix archive count and partial copy cleanup - #250
Conversation
Include strikethrough items in the JSON archived count so it matches the pointer text. Clean up partial domain directories on copytree failure to prevent stale workspace domains. Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fonta-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
WalkthroughThe workspace scripts now include strikethrough items in archive totals and clean up partial destinations after failed copy-on-write operations. Workspace plugin metadata is updated to version 0.1.2. ChangesWorkspace updates
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@plugins/workspace/scripts/consolidate-project.py`:
- Line 221: Unify the archived-item count used by the consolidation flow so both
JSON/CLAUDE.md pointers and build_archive_block() report checked items plus
strikethrough items. Update build_archive_block() and the surrounding
archive-list construction to reuse one shared count or list, then add positive
coverage for sections with strikethroughs and negative coverage for sections
without them.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e8c53820-82bf-4e53-ae09-08ddffcb2a04
📒 Files selected for processing (2)
plugins/workspace/scripts/consolidate-project.pyplugins/workspace/scripts/domain-info.py
Accepted after review: - consolidate-project.py:152: unify archive block header count to include strikethroughs Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
coderabbitai
Bot
added
the
ready-for-human-review
lucaconsalvi
left a comment
lucaconsalvi
left a comment
There was a problem hiding this comment.
Thanks for the quick follow-up, Pablo!
Both fixes look correct and minimal — the archive count now agrees across all three sites (archive block header, CLAUDE.md pointer, JSON output), and the partial-copy cleanup addresses a real downstream problem where find_domain_dir would pick up a broken directory on the next run.
One suggestion inline on the cleanup path — everything else looks good to merge.
| shutil.copytree(domain_dir, dest) | ||
| except (OSError, shutil.Error) as exc: | ||
| if dest.exists(): | ||
| shutil.rmtree(dest, ignore_errors=True) |
There was a problem hiding this comment.
Nit: ignore_errors=True is the right instinct here (cleanup should not mask the original error), but if rmtree fails, the partial directory stays behind — and find_domain_dir will discover it on the next run and treat it as a valid domain.
Worth reporting the cleanup failure so the user knows to remove it manually:
except (OSError, shutil.Error) as exc:
cleanup_ok = True
if dest.exists():
try:
shutil.rmtree(dest)
except OSError:
cleanup_ok = False
msg = f"Failed to copy domain to workspace: {exc}"
if not cleanup_ok:
msg += (f" (warning: partial directory at {dest} could not be"
" removed — delete it manually before retrying)")
print(json.dumps({"status": "error", "error": msg}))
returnNot blocking — the current version works for the common case.
There was a problem hiding this comment.
Good catch — applied in 2deb127. Now catches rmtree failure and appends a manual-cleanup warning to the error message.
When rmtree fails to clean up a partial domain directory after a copytree failure, warn the user to delete it manually. Bump plugin version to 0.1.2. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/workspace/scripts/domain-info.py (1)
240-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd copy-on-write cleanup failure tests. Cover
shutil.copytreefailure with a partial destination, successful cleanup, and failed cleanup with the manual-delete warning. Assert the JSON error and destination state for each case.🤖 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 `@plugins/workspace/scripts/domain-info.py` around lines 240 - 250, Extend the tests covering the domain-copy error path around shutil.copytree to simulate failures with a partial destination. Add cases for successful cleanup and cleanup failure, asserting the emitted JSON error and whether the destination is removed or remains with the manual-delete warning; also cover the copy failure with a partial destination state.
🤖 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 `@plugins/workspace/scripts/domain-info.py`:
- Around line 240-245: Update the domain copy flow around find_domain_dir() and
copytree() to copy into a uniquely created staging directory, then remove only
that staging directory on failure instead of deleting dest. Add a negative-path
test covering a destination created before copy failure and verify that
destination remains intact.
---
Nitpick comments:
In `@plugins/workspace/scripts/domain-info.py`:
- Around line 240-250: Extend the tests covering the domain-copy error path
around shutil.copytree to simulate failures with a partial destination. Add
cases for successful cleanup and cleanup failure, asserting the emitted JSON
error and whether the destination is removed or remains with the manual-delete
warning; also cover the copy failure with a partial destination state.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b406333f-41ea-47c0-9b7a-93b218faccdd
📒 Files selected for processing (3)
.claude-plugin/marketplace.jsonplugins/workspace/.claude-plugin/plugin.jsonplugins/workspace/scripts/domain-info.py
|
Re: CodeRabbit review-body nitpick — add copy-on-write cleanup tests Won't fix — generic "add tests" suggestion. The error paths are straightforward and the existing test suite covers the domain-info happy paths. Not adding speculative coverage for a race condition that isn't reachable (see inline reply). |
|
/lgtm |
2 participants
Summary
sections[*].archivedcount so it matches the pointer text written to CLAUDE.mdshutil.copytreefailure to prevent stale workspace domains on subsequent runsFollow-up to #249 — addresses CodeRabbit findings that landed after merge.
Test plan
python3 -m pytest plugins/workspace/tests/— 50 passedbash plugins/workspace/tests/test_setup.sh— 84 passed🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores