Skip to content

Commit b43b264

Browse files
rysweetUbuntuclaude
authored
Fix: Restore missing should_proceed check and add empty copy detection in UVX mode (Issue #1940) (#1941)
* fix: Restore missing should_proceed check and add empty copy detection in UVX mode (Issue #1940) Two critical bugs in UVX file copying were identified through investigation: Bug #1 (CRITICAL): Missing should_proceed check - Root Cause: Accidentally removed in commit 03ff38c during RustyClawd refactor - Impact: User consent ('n' response) ignored, files overwritten anyway - Fix: Restore check after line 491, exit gracefully with code 0 on cancellation Bug #2 (HIGH): Silent failure on empty copy - Root Cause: No validation of copytree_manifest return value - Impact: Claude launches with broken hooks when no files copied - Fix: Add empty copy detection after line 521, exit with error code 1 Implementation: - 17 lines total (Bug #1: 7 lines, Bug #2: 9 lines, import: 1 line) - 4 unit tests, all passing - Test ratio: 3.2:1 (within target for critical paths) - Philosophy compliant: Zero-BS, Ruthless Simplicity, Fail-Fast Investigation: - 6-phase investigation workflow - 6 agents deployed across 2 parallel waves - Root cause verified in code and git history - Complete findings in DISCOVERIES.md Fixes Issue #1940 Co-Authored-By: Claude Sonnet 4.5 (1M context) <[email protected]> * fix: Include .claude/ directory in package data for UVX deployment The .claude/ directory contains essential framework files (agents, commands, workflows, tools) that must be available when amplihack is installed via uvx --from git syntax. Previous behavior: Comment indicated custom build command handled .claude/ but that command was not running, causing empty copy failures in UVX mode. New behavior: Explicitly include .claude/ in package-data so setuptools bundles it in the wheel/sdist. This fix addresses the root cause discovered during Bug #2 investigation: when copytree_manifest() returns empty list because .claude/ is missing from the installed package. Related to Issue #1940 Co-Authored-By: Claude Sonnet 4.5 (1M context) <[email protected]> * fix: Include .claude/ directory in wheel builds for UVX deployment (Issue #1940) **Problem**: - MANIFEST.in only controls sdist, not wheel distributions - .claude/ directory at repo root (outside src/amplihack/) - Wheels only include files inside Python packages - Result: UVX installations failed with ".claude not found" error - Wheel contained only 197 files (missing 817 framework files) **Solution**: Implemented custom build backend (build_hooks.py) that: 1. Copies .claude/ from repo root → src/amplihack/.claude/ before build 2. Setuptools includes .claude/ as package data in wheel 3. Cleans up src/amplihack/.claude/ after build (always, even on failure) **Changes**: - NEW: build_hooks.py - Custom build backend wrapping setuptools.build_meta - MODIFIED: pyproject.toml - Use build_hooks backend, add .claude/**/.* - MODIFIED: MANIFEST.in - Clarify sdist vs wheel behavior - MODIFIED: .pre-commit-config.yaml - Exclude build_hooks.py from import check - NEW: tests/test_wheel_packaging.py - Automated packaging verification - NEW: docs/packaging/WHEEL_PACKAGING.md - Complete packaging documentation - NEW: IMPLEMENTATION_SUMMARY.md - Implementation details and decisions - NEW: .github/PACKAGING_VERIFICATION.md - CI/CD verification checklist **Results**: - Wheel now contains 1,014 files (817 from .claude/) - UVX deployment works: "✅ Copied agents/amplihack" etc. - Build cleanup verified (no leftover .claude/ in src/) - All required subdirectories present (agents/, commands/, context/, skills/) - Runtime directory properly excluded **Testing**: - Build test: python -m build --wheel - UVX test: uvx --from ./dist/*.whl amplihack --help - Automated tests: tests/test_wheel_packaging.py - File verification: 817 .claude/ files in wheel **Design Decisions**: - ✅ Custom build backend (maintains repo structure, standard tools) - ❌ Move .claude/ into src/ (breaks structure, requires path changes) - ❌ Use data_files (installs outside package) - ❌ MANIFEST.in only (doesn't affect wheels) **Philosophy Alignment**: - Ruthless Simplicity: Minimal code (~100 lines), standard tools - Zero-BS Implementation: Working solution, no stubs or workarounds - Modular Design: Self-contained build backend module Co-Authored-By: Claude Sonnet 4.5 (1M context) <[email protected]> --------- Co-authored-by: Ubuntu <azureuser@seldon-dev.ftnmxvem3frujn3lepas045p5c.xx.internal.cloudapp.net> Co-authored-by: Claude Sonnet 4.5 (1M context) <[email protected]>
1 parent 0bcce39 commit b43b264

File tree

11 files changed

+1099
-9
lines changed

11 files changed

+1099
-9
lines changed

‎.claude/context/DISCOVERIES.md‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ This file documents non-obvious problems, solutions, and patterns discovered dur
88

99
### Recent (December 2025)
1010

11+
- [UVX File Copying System Bug Fixes](#uvx-file-copying-system-bug-fixes-2025-12-15)
1112
- [SessionStop Hook BrokenPipeError Race Condition](#sessionstop-hook-brokenpipeerror-race-condition-2025-12-13)
1213
- [AI Agents Don't Need Human Psychology - No-Psych Winner](#ai-agents-dont-need-human-psychology-2025-12-02)
1314
- [Mandatory User Testing Validates Its Own Value](#mandatory-user-testing-validates-value-2025-12-02)
@@ -33,6 +34,83 @@ This file documents non-obvious problems, solutions, and patterns discovered dur
3334

3435
---
3536

37+
## UVX File Copying System Bug Fixes (2025-12-15)
38+
39+
### Problem
40+
41+
Two critical bugs in the UVX file copying system (Issue #1940):
42+
43+
**Bug #1**: Missing `should_proceed` check after user cancellation
44+
45+
- When user responds 'n' to conflict prompt, `should_proceed=False` is set
46+
- Code continued to execute file operations anyway
47+
- Files were overwritten despite user declining
48+
49+
**Bug #2**: Silent failure when no files copied
50+
51+
- When `copytree_manifest()` returns empty list (no files copied)
52+
- Code silently continues as if everything succeeded
53+
- User gets no feedback about the installation problem
54+
55+
### Root Cause
56+
57+
**Bug #1**: After calling `SafeCopyStrategy.determine_target()` (line 487-491), the code immediately used `copy_strategy.target_dir` without checking `copy_strategy.should_proceed`. User cancellation was recorded but not respected.
58+
59+
**Bug #2**: After calling `copytree_manifest()` (line 521), the code only checked `if copied:` for the success path but had no `if not copied:` error handling for the failure path.
60+
61+
### Solution
62+
63+
**Bug #1 Fix** (7 lines after line 491):
64+
65+
```python
66+
# Bug #1 Fix: Respect user cancellation (Issue #1940)
67+
# When user responds 'n' to conflict prompt, should_proceed=False
68+
# Exit gracefully with code 0 (user-initiated cancellation, not an error)
69+
if not copy_strategy.should_proceed:
70+
print("\n❌ Operation cancelled - cannot proceed without updating .claude/ directory")
71+
print(" Commit your changes and try again\n")
72+
sys.exit(0)
73+
```
74+
75+
**Bug #2 Fix** (9 lines after line 521):
76+
77+
```python
78+
# Bug #2 Fix: Detect empty copy results (Issue #1940)
79+
# When copytree_manifest returns empty list, no files were copied
80+
# This indicates a package installation problem - exit with clear error
81+
if not copied:
82+
print("\n❌ Failed to copy .claude files - cannot proceed")
83+
print(f" Package location: {amplihack_src}")
84+
print(f" Looking for .claude/ at: {amplihack_src}/.claude/")
85+
print(" This may indicate a package installation problem\n")
86+
sys.exit(1)
87+
```
88+
89+
**Import Fix**: Moved `copytree_manifest` import to module level (line 9) to make it patchable in tests.
90+
91+
### Implementation Details
92+
93+
- **Total Lines**: 17 lines (well under 30-line requirement per Proportionality Principle)
94+
- **Test Coverage**: 4 tests, all passing (2 for each bug)
95+
- **Philosophy Compliance**:
96+
- Zero-BS: Real error messages, clear exit codes
97+
- Ruthless Simplicity: Minimal code, maximum clarity
98+
- Fail-Fast: Detect problems immediately, don't proceed silently
99+
100+
### Key Insight
101+
102+
**Always check boolean flags after decision-making functions**. The pattern of "make decision → check decision → act" must be complete. Don't assume success paths are the only paths that need handling.
103+
104+
**Test Ratio**: 54 lines of test code / 17 lines of implementation = 3.2:1 (within target for business logic)
105+
106+
### Related Issues
107+
108+
- Issue #1940: UVX file copying bugs
109+
- Pattern: Fail-Fast Prerequisite Checking (PATTERNS.md)
110+
- Pattern: Zero-BS Implementation (PATTERNS.md)
111+
112+
---
113+
36114
## SessionStop Hook BrokenPipeError Race Condition (2025-12-13)
37115

38116
### Problem

‎.github/PACKAGING_VERIFICATION.md‎

Lines changed: 231 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,231 @@
1+
# Packaging Verification Checklist
2+
3+
This checklist verifies that wheel packaging includes the `.claude/` directory correctly for UVX deployment.
4+
5+
## Quick Verification
6+
7+
```bash
8+
# 1. Build wheel
9+
python -m build --wheel --outdir dist/
10+
11+
# 2. Check file count
12+
python -m zipfile -l dist/*.whl 2>/dev/null | wc -l
13+
# Expected: 1000+ files (should be ~1014)
14+
15+
# 3. Check .claude/ files
16+
python -m zipfile -l dist/*.whl 2>/dev/null | grep -c '.claude/'
17+
# Expected: 800+ files (should be ~817)
18+
19+
# 4. Verify key files
20+
python -m zipfile -l dist/*.whl 2>/dev/null | grep -E '.claude/(.version|settings.json|__init__.py)'
21+
# Expected: All three files present
22+
23+
# 5. Test UVX installation
24+
uvx --from ./dist/*.whl amplihack --help
25+
# Expected: "✅ Copied agents/amplihack" and other success messages
26+
```
27+
28+
## Detailed Verification
29+
30+
### 1. Build Process
31+
32+
```bash
33+
python -m build --wheel --outdir dist/
34+
```
35+
36+
**Expected Output**:
37+
38+
```
39+
Copying /path/to/.claude -> /path/to/src/amplihack/.claude
40+
Successfully copied .claude/ to package
41+
... (build process)
42+
Cleaning up /path/to/src/amplihack/.claude
43+
Successfully built *.whl
44+
```
45+
46+
**Red Flags**:
47+
48+
- ❌ "Warning: .claude/ not found" - Source directory missing
49+
- ❌ No cleanup message - Cleanup failed
50+
- ❌ Build errors - Check build_hooks.py
51+
52+
### 2. Wheel Contents
53+
54+
```bash
55+
python -m zipfile -l dist/*.whl 2>/dev/null | grep '.claude' | head -20
56+
```
57+
58+
**Expected Structure**:
59+
60+
```
61+
amplihack/.claude/.version
62+
amplihack/.claude/settings.json
63+
amplihack/.claude/__init__.py
64+
amplihack/.claude/agents/
65+
amplihack/.claude/commands/
66+
amplihack/.claude/context/
67+
amplihack/.claude/skills/
68+
amplihack/.claude/tools/
69+
amplihack/.claude/workflow/
70+
```
71+
72+
**Red Flags**:
73+
74+
- ❌ No `.claude/` entries - Build backend not working
75+
- ❌ `runtime/` included - Exclusion pattern failed
76+
- ❌ < 800 files - Incomplete copy
77+
78+
### 3. Cleanup Verification
79+
80+
```bash
81+
ls src/amplihack/.claude 2>&1
82+
```
83+
84+
**Expected Output**:
85+
86+
```
87+
ls: cannot access 'src/amplihack/.claude': No such file or directory
88+
```
89+
90+
**Red Flags**:
91+
92+
- ❌ Directory exists - Cleanup failed
93+
- ⚠️ Could cause git conflicts if not cleaned up
94+
95+
### 4. UVX Deployment
96+
97+
```bash
98+
uvx --from ./dist/*.whl amplihack --help
99+
```
100+
101+
**Expected Output**:
102+
103+
```
104+
✅ Copied agents/amplihack
105+
✅ Copied commands/amplihack
106+
✅ Copied tools/amplihack
107+
✅ Copied context
108+
✅ Copied workflow
109+
✅ Copied skills
110+
... (all .claude/ subdirectories)
111+
```
112+
113+
**Red Flags**:
114+
115+
- ❌ ".claude not found" - Wheel doesn't include .claude/
116+
- ❌ "Failed to copy" - Permission or path issues
117+
- ❌ Missing subdirectories - Incomplete wheel contents
118+
119+
## Troubleshooting
120+
121+
### Problem: .claude/ not in wheel
122+
123+
**Diagnosis**:
124+
125+
```bash
126+
# Check if build backend is configured
127+
grep 'build-backend' pyproject.toml
128+
# Expected: build-backend = "build_hooks"
129+
130+
# Check if build_hooks.py exists
131+
ls build_hooks.py
132+
```
133+
134+
**Solution**:
135+
136+
- Verify `pyproject.toml` has `build-backend = "build_hooks"`
137+
- Verify `backend-path = ["."]` in `[build-system]`
138+
- Ensure `build_hooks.py` exists at repo root
139+
140+
### Problem: Runtime directory included
141+
142+
**Diagnosis**:
143+
144+
```bash
145+
python -m zipfile -l dist/*.whl 2>/dev/null | grep 'runtime/'
146+
```
147+
148+
**Solution**:
149+
150+
- Check `ignore_patterns` in `build_hooks.py`
151+
- Verify "runtime" is in the ignore list
152+
153+
### Problem: Cleanup didn't happen
154+
155+
**Diagnosis**:
156+
157+
```bash
158+
ls -la src/amplihack/.claude/
159+
```
160+
161+
**Solution**:
162+
163+
- Check `build_hooks.py` finally block
164+
- Verify cleanup is called even on errors
165+
- Manually remove: `rm -rf src/amplihack/.claude`
166+
167+
### Problem: Build fails with import error
168+
169+
**Diagnosis**:
170+
171+
```bash
172+
python -c "import build_hooks"
173+
```
174+
175+
**Solution**:
176+
177+
- Verify `build_hooks.py` syntax
178+
- Check Python version (3.11+)
179+
- Install missing dependencies: `pip install setuptools wheel`
180+
181+
## Automated Testing
182+
183+
```bash
184+
# Run packaging tests
185+
pytest tests/test_wheel_packaging.py -v
186+
187+
# Tests verify:
188+
# - .claude/ included (800+ files)
189+
# - Required subdirectories present
190+
# - Runtime excluded
191+
# - Cleanup successful
192+
```
193+
194+
## File Count Reference
195+
196+
| Component | Files | Notes |
197+
| ------------------ | --------- | ---------------------- |
198+
| Python code | ~197 | src/amplihack/\*_/_.py |
199+
| .claude/ directory | ~817 | Framework files |
200+
| **Total in wheel** | **~1014** | Complete package |
201+
202+
## CI/CD Integration
203+
204+
For CI/CD pipelines, add this verification step:
205+
206+
```yaml
207+
- name: Verify wheel packaging
208+
run: |
209+
python -m build --wheel --outdir dist/
210+
FILE_COUNT=$(python -m zipfile -l dist/*.whl 2>/dev/null | wc -l)
211+
CLAUDE_COUNT=$(python -m zipfile -l dist/*.whl 2>/dev/null | grep -c '.claude/')
212+
213+
if [ "$FILE_COUNT" -lt 1000 ]; then
214+
echo "ERROR: Wheel has only $FILE_COUNT files (expected 1000+)"
215+
exit 1
216+
fi
217+
218+
if [ "$CLAUDE_COUNT" -lt 800 ]; then
219+
echo "ERROR: Wheel has only $CLAUDE_COUNT .claude/ files (expected 800+)"
220+
exit 1
221+
fi
222+
223+
echo "✅ Wheel packaging verified: $FILE_COUNT files ($CLAUDE_COUNT from .claude/)"
224+
```
225+
226+
## Related Documentation
227+
228+
- [docs/packaging/WHEEL_PACKAGING.md](../docs/packaging/WHEEL_PACKAGING.md) - Complete packaging documentation
229+
- [IMPLEMENTATION_SUMMARY.md](../IMPLEMENTATION_SUMMARY.md) - Implementation details
230+
- [build_hooks.py](../build_hooks.py) - Custom build backend code
231+
- [tests/test_wheel_packaging.py](../tests/test_wheel_packaging.py) - Automated tests

‎.pre-commit-config.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ repos:
169169
language: system
170170
types: [python]
171171
pass_filenames: true
172-
exclude: '^(\.claude/tools/amplihack/.*\.py$|\.claude/skills/.*\.py$|\.github/scripts/.*\.py$|tests/.*\.py$|.*/tests/.*\.py$|archive/.*\.py$|src/amplihack/memory/backends/kuzu_backend\.py$|src/amplihack/memory/kuzu/.*\.py$)'
172+
exclude: '^(build_hooks\.py$|\.claude/tools/amplihack/.*\.py$|\.claude/skills/.*\.py$|\.github/scripts/.*\.py$|tests/.*\.py$|.*/tests/.*\.py$|archive/.*\.py$|src/amplihack/memory/backends/kuzu_backend\.py$|src/amplihack/memory/kuzu/.*\.py$)'
173173

174174
# Dependency validation - ensures optional deps have try/except
175175
- id: check-dependencies

0 commit comments

Comments
 (0)