Skip to content

Commit 311d3a1

Browse files
justin808claude
andauthored
Break CI circular dependency with non-docs change (#2065)
## Summary This PR breaks the circular dependency created by previous master commits that failed the CI safety check, and adds comprehensive documentation to prevent similar issues in the future. ## Problem After PR #2062 merged, master is still in a broken state because: 1. Commit `beb70f009` and earlier commits have workflows that failed due to the circular dependency bug 2. When we re-run those workflows, they use the **action code from that commit** (with the old bug), not from current master 3. GitHub Actions always checks out workflow files from the commit being tested, not from master 4. This means the reruns still fail with the same circular dependency issue 5. The new safety check (now fixed) correctly detects these failures and blocks subsequent docs-only commits ## Solution This PR includes two components: ### 1. Cycle-Breaking Change Makes a trivial non-docs change that will: - Trigger **full CI** (not be detected as docs-only) - Use the **fixed version** of the ensure-master-docs-safety action from PR #2062 - Pass all tests (since the fix is now in place) - Become the new "previous commit" for future safety checks - Break the circular dependency cycle ### 2. Comprehensive Documentation Updates Adds modular documentation (using @ references) to prevent similar issues: **New Documentation Files:** - `.claude/docs/testing-build-scripts.md` - Mandatory testing after package.json/package-scripts.yml changes - Manual yalc publish testing requirements - Real example of what went wrong (7-week silent failure) - `.claude/docs/master-health-monitoring.md` - Immediate actions after PR merges - How to handle broken master - Understanding workflow reruns and circular dependencies - `.claude/docs/managing-file-paths.md` - Path verification checklist before committing - Post-refactor validation steps - Real example of the package-scripts.yml bug **Enhanced Existing Content:** - Updated Merge Conflict Resolution Workflow with path verification steps - Added critical testing requirements after resolving build config conflicts **Supporting Documentation:** - `CLAUDE_MD_UPDATES.md` - Detailed implementation guide for applying these updates - `claude-md-improvements.md` - Original root cause analysis ## Why This Approach We **cannot** fix the old commits by re-running their workflows because: - Workflow reruns use the action code from the original commit - The bug fix in PR #2062 only helps commits that come AFTER it - We need a new commit with passing tests to establish a clean baseline ## Changes ### Code Changes - Updated action description for clarity (minor wording improvement) ### Documentation Changes - Added 3 new modular documentation files using @ references - Enhanced merge conflict resolution workflow - Included comprehensive guides to prevent future issues ## Root Cause Addressed The documentation specifically addresses the failures that led to this issue: - ❌ No manual testing of yalc publish → Now required after build config changes - ❌ No path verification after structure changes → Now has checklist - ❌ No master health monitoring → Now has immediate action plan - ❌ Silent failures went unnoticed for weeks → Now has detection strategies ## Test Plan - [x] Change is non-docs (triggers full CI) - [ ] CI passes with the fixed action - [ ] Future docs-only commits will check this commit and find it passed - [ ] Circular dependency is broken - [ ] Documentation @ references work correctly ## Related - Fixes the remaining issues from PR #2062 - Implements the fix for the circular dependency bug - Adds preventive documentation based on root cause analysis 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified CI action description to better reflect behavior for docs-only commits. * Added new guides on managing file paths, testing build/package scripts, and monitoring master branch health. * Expanded CLAUDE.md with conflict-resolution, testing, and architecture guidance; included an incident write-up and validation/implementation plans. * **Chores** * Added comprehensive operational checklists and workflows to reduce CI breakage during large refactors and build/config changes. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude <[email protected]>
1 parent 50f3baf commit 311d3a1

File tree

8 files changed

+836
-5
lines changed

8 files changed

+836
-5
lines changed
Lines changed: 346 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,346 @@
1+
# Concrete CLAUDE.md Updates Based on CI Breakage Analysis
2+
3+
## Table of Contents
4+
5+
- [Executive Summary of What Went Wrong](#executive-summary-of-what-went-wrong)
6+
- [High Priority Updates](#high-priority-add-these-3-sections-immediately)
7+
- [Section 1: Testing Build and Package Scripts](#section-1-testing-build-and-package-scripts)
8+
- [Section 2: Master Branch Health Monitoring](#section-2-master-branch-health-monitoring)
9+
- [Section 3: Managing File Paths in Configuration](#section-3-managing-file-paths-in-configuration)
10+
- [Medium Priority Updates](#medium-priority-enhanced-merge-conflict-resolution)
11+
- [Implementation Plan](#implementation-plan)
12+
- [Validation](#validation)
13+
- [Key Takeaways](#key-takeaways-for-claudemd-philosophy)
14+
15+
## Executive Summary of What Went Wrong
16+
17+
**Root Cause Chain:**
18+
19+
1. PR #1830 (Sep 29) moved `node_package/``packages/react-on-rails/`
20+
2. Path in `package-scripts.yml` was updated to `packages/react-on-rails/lib/ReactOnRails.full.js`
21+
3. Later, structure was partially reverted to `lib/` at root, but `package-scripts.yml` wasn't updated
22+
4. This broke `yalc publish` silently for ~7 weeks
23+
5. When fixed in PR #2054, the CI safety check created a circular dependency
24+
6. The safety check had a bug: checked `run.conclusion` which never updates after reruns
25+
26+
**Key Failures:**
27+
28+
- ❌ No verification that paths in package-scripts.yml were correct after structure change
29+
- ❌ No manual testing of yalc publish to catch the breakage
30+
- ❌ No monitoring of master health - failure went unnoticed for weeks
31+
- ❌ CI safety mechanism created circular dependency instead of helping
32+
33+
---
34+
35+
## HIGH PRIORITY: Add These 3 Sections Immediately
36+
37+
### Section 1: Testing Build and Package Scripts
38+
39+
Add this new section to CLAUDE.md (both root and .conductor/london-v1):
40+
41+
````markdown
42+
## Testing Build and Package Scripts
43+
44+
**CRITICAL: Build scripts are infrastructure code that MUST be tested manually:**
45+
46+
### Why This Matters
47+
48+
- The `prepack`/`prepare` scripts in package.json/package-scripts.yml run during:
49+
- `npm install` / `yarn install` (for git dependencies)
50+
- `yalc publish` (critical for local development)
51+
- `npm publish`
52+
- Package manager prepare phase
53+
- If these fail, users can't install or use the package
54+
- Failures are often silent - they don't show up in normal CI
55+
56+
### Mandatory Testing After ANY Changes
57+
58+
**If you modify package.json, package-scripts.yml, or build configs:**
59+
60+
1. **Test the prepack script:**
61+
```bash
62+
yarn run prepack
63+
# Should succeed without errors
64+
```
65+
````
66+
67+
2. **Test yalc publish (CRITICAL):**
68+
69+
```bash
70+
yarn run yalc.publish
71+
# Should publish successfully
72+
```
73+
74+
3. **Verify build artifacts exist at expected paths:**
75+
76+
```bash
77+
# Check the path referenced in package-scripts.yml
78+
ls -la lib/ReactOnRails.full.js
79+
80+
# If package-scripts.yml references packages/*, check that too
81+
ls -la packages/*/lib/*.js
82+
```
83+
84+
4. **Test clean install:**
85+
```bash
86+
rm -rf node_modules
87+
yarn install
88+
# Should install without errors
89+
```
90+
91+
### When Directory Structure Changes
92+
93+
If you rename/move directories that contain build artifacts:
94+
95+
1. **Update ALL path references in package-scripts.yml**
96+
2. **Test yalc publish BEFORE pushing**
97+
3. **Test in a fresh clone to ensure no local assumptions**
98+
4. **Consider adding a CI job to validate artifact paths**
99+
100+
### Real Example: What Went Wrong
101+
102+
In Sep 2024, we moved `node_package/``packages/react-on-rails/`. The path in
103+
package-scripts.yml was updated to `packages/react-on-rails/lib/ReactOnRails.full.js`.
104+
Later, the structure was partially reverted to `lib/` at root, but package-scripts.yml
105+
wasn't updated. This broke yalc publish silently for 7 weeks. Manual testing of
106+
`yarn run yalc.publish` would have caught this immediately.
107+
108+
````
109+
110+
### Section 2: Master Branch Health Monitoring
111+
112+
Add this new section to CLAUDE.md (both versions):
113+
114+
```markdown
115+
## Master Branch Health Monitoring
116+
117+
**CRITICAL: Master staying broken affects the entire team. Don't let it persist.**
118+
119+
### Immediate Actions After Your PR Merges
120+
121+
Within 30 minutes of your PR merging to master:
122+
123+
1. **Check CI status:**
124+
```bash
125+
# View the merged PR's CI status
126+
gh pr view <your-pr-number> --json statusCheckRollup
127+
128+
# Or check recent master runs
129+
gh run list --branch master --limit 5
130+
````
131+
132+
2. **If you see failures:**
133+
- Investigate IMMEDIATELY
134+
- Don't assume "someone else will fix it"
135+
- You are responsible for ensuring your PR doesn't break master
136+
137+
### When You Discover Master is Broken
138+
139+
1. **Determine if it's from your PR:**
140+
141+
```bash
142+
gh run list --branch master --limit 10
143+
```
144+
145+
2. **Take immediate action:**
146+
- If your PR broke it: Submit a fix PR within the hour, OR revert and resubmit
147+
- If unsure: Investigate and communicate with team
148+
- Never leave master broken overnight
149+
150+
### Silent Failures are Most Dangerous
151+
152+
Some failures don't show up in standard CI:
153+
154+
- yalc publish failures
155+
- Build artifact path issues
156+
- Package installation problems
157+
158+
**Always manually test critical workflows:**
159+
160+
- If you changed package structure → test `yarn run yalc.publish`
161+
- If you changed build configs → test `yarn build && ls -la lib/`
162+
- If you changed generators → test `rake run_rspec:example_basic`
163+
164+
### Understanding Workflow Reruns
165+
166+
**Important limitation:**
167+
168+
- Re-running a workflow does NOT change its `conclusion` in the GitHub API
169+
- GitHub marks a run as "failed" even if a manual rerun succeeds
170+
- Our CI safety checks (as of PR #2062) now handle this correctly
171+
- But be aware: old commits with failed reruns may still block docs-only commits
172+
173+
**What this means:**
174+
175+
- If master workflows fail, reruns alone won't fix the circular dependency
176+
- You need a new commit that passes to establish a clean baseline
177+
- See PR #2065 for an example of breaking the cycle
178+
179+
````
180+
181+
### Section 3: Managing File Paths in Configuration
182+
183+
Add this new section to CLAUDE.md (both versions):
184+
185+
```markdown
186+
## Managing File Paths in Configuration Files
187+
188+
**CRITICAL: Hardcoded paths are a major source of bugs, especially after refactors.**
189+
190+
### Before Committing Path Changes
191+
192+
1. **Verify the path actually exists:**
193+
```bash
194+
ls -la <the-path-you-just-added>
195+
````
196+
197+
2. **Test operations that use the path:**
198+
199+
```bash
200+
# If it's a build artifact path in package-scripts.yml:
201+
yarn run prepack
202+
yarn run yalc.publish
203+
204+
# If it's a webpack output path:
205+
yarn build && ls -la <output-path>
206+
```
207+
208+
3. **Search for ALL references to old paths if renaming:**
209+
```bash
210+
# Example: if renaming node_package/ to packages/
211+
grep -r "node_package" . --exclude-dir=node_modules --exclude-dir=.git
212+
grep -r "packages/react-on-rails" . --exclude-dir=node_modules
213+
```
214+
215+
### Files That Commonly Have Path References
216+
217+
**Always check these after directory structure changes:**
218+
219+
- `package-scripts.yml` - build artifact paths in prepack/prepare scripts
220+
- `package.json` - "files", "main", "types", "exports" fields
221+
- `webpack.config.js` / `config/webpack/*` - output.path, resolve.modules
222+
- `.github/workflows/*.yml` - cache paths, artifact paths, working directories
223+
- `lib/generators/**/templates/**` - paths in generated code
224+
- Documentation files - example paths and installation instructions
225+
226+
### Post-Refactor Validation Checklist
227+
228+
After any directory structure change, run this checklist:
229+
230+
```bash
231+
# 1. Find any lingering references to old paths
232+
grep -r "old/path/name" . --exclude-dir=node_modules --exclude-dir=.git
233+
234+
# 2. Verify build artifacts are in expected locations
235+
yarn build
236+
find . -name "ReactOnRails.full.js" -type f
237+
find . -name "package.json" -type f
238+
239+
# 3. Test package scripts
240+
yarn run prepack
241+
yarn run yalc.publish
242+
243+
# 4. Test clean install
244+
rm -rf node_modules && yarn install
245+
246+
# 5. Run full test suite
247+
rake
248+
```
249+
250+
### Real Example: The package-scripts.yml Bug
251+
252+
**What happened:**
253+
254+
- Path was changed from `node_package/lib/` to `packages/react-on-rails/lib/`
255+
- Later, the actual directory structure was reverted to `lib/` at root
256+
- But package-scripts.yml still referenced `packages/react-on-rails/lib/`
257+
- This caused yalc publish to fail silently for 7 weeks
258+
259+
**How to prevent:**
260+
261+
1. After changing directory structure, search for ALL references to old paths
262+
2. Always run `yarn run yalc.publish` manually to verify it works
263+
3. Check that paths in package-scripts.yml match actual file locations
264+
4. Use `ls -la <path>` to verify paths exist before committing
265+
266+
````
267+
268+
---
269+
270+
## MEDIUM PRIORITY: Enhanced Merge Conflict Resolution
271+
272+
Update the existing "Merge Conflict Resolution Workflow" section with this addition:
273+
274+
```markdown
275+
### Merge Conflict Resolution Workflow
276+
**CRITICAL**: When resolving merge conflicts, follow this exact sequence:
277+
278+
1. **Resolve logical conflicts only** - don't worry about formatting
279+
2. **VERIFY FILE PATHS** - if the conflict involved directory structure:
280+
- Check if any hardcoded paths need updating
281+
- Run: `grep -r "old/path" . --exclude-dir=node_modules`
282+
- Pay special attention to package-scripts.yml, webpack configs, package.json
283+
- **Test affected scripts:** If package-scripts.yml changed, run `yarn run prepack`
284+
3. **Add resolved files**: `git add .` (or specific files)
285+
4. **Auto-fix everything**: `rake autofix`
286+
5. **Add any formatting changes**: `git add .`
287+
6. **Continue rebase/merge**: `git rebase --continue` or `git commit`
288+
7. **TEST CRITICAL SCRIPTS if build configs changed:**
289+
```bash
290+
yarn run prepack # Test prepack script
291+
yarn run yalc.publish # Test yalc publish if package structure changed
292+
rake run_rspec:gem # Run relevant test suites
293+
````
294+
295+
**❌ NEVER manually format during conflict resolution** - this causes formatting wars.
296+
**❌ NEVER blindly accept path changes** - verify they're correct for current structure.
297+
**❌ NEVER skip testing after resolving conflicts in build configs** - silent failures are dangerous.
298+
299+
```
300+
301+
---
302+
303+
## Implementation Plan
304+
305+
### Week 1 (Immediate)
306+
- [ ] Add Section 1: Testing Build and Package Scripts to both CLAUDE.md files
307+
- [ ] Add Section 2: Master Branch Health Monitoring to both CLAUDE.md files
308+
- [ ] Add Section 3: Managing File Paths in Configuration to both CLAUDE.md files
309+
310+
### Week 2
311+
- [ ] Update merge conflict resolution section in both CLAUDE.md files
312+
- [ ] Add checklist for large refactors (optional - can wait)
313+
314+
### Future Improvements
315+
- [ ] Consider adding a CI job that validates build artifact paths
316+
- [ ] Consider pre-commit hook to validate package-scripts.yml paths exist
317+
- [ ] Document this incident in team knowledge base
318+
319+
---
320+
321+
## Validation
322+
323+
After adding these sections, verify they work by:
324+
325+
1. Having a team member review the new sections
326+
2. Testing that the instructions are clear and actionable
327+
3. Referencing these sections during next PR that touches build configs
328+
4. Updating based on feedback after first real-world use
329+
330+
---
331+
332+
## Key Takeaways for CLAUDE.md Philosophy
333+
334+
**What makes good CLAUDE.md content:**
335+
- ✅ Specific commands to run, not vague advice
336+
- ✅ Real examples of what went wrong and how to prevent it
337+
- ✅ Checklists and step-by-step instructions
338+
- ✅ Clear "why this matters" context
339+
- ✅ Mandatory testing after specific types of changes
340+
341+
**What to avoid:**
342+
- ❌ Generic advice like "be careful with paths"
343+
- ❌ Unclear responsibilities ("someone should check")
344+
- ❌ Assumptions that CI will catch everything
345+
- ❌ Missing the "silent failure" scenarios
346+
```

.claude/docs/analysis/README.md

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
# Analysis Documentation
2+
3+
This directory contains root cause analysis and implementation guides from past incidents.
4+
5+
## Files
6+
7+
### CLAUDE_MD_UPDATES.md
8+
9+
Concrete implementation guide created after the Nov 2024 CI circular dependency incident. Contains copy-paste ready sections that were applied to CLAUDE.md to prevent similar issues.
10+
11+
### claude-md-improvements.md
12+
13+
Original root cause analysis of the CI breakage, including timeline of events, what went wrong, and comprehensive recommendations for prevention.
14+
15+
## Purpose
16+
17+
These documents serve as:
18+
19+
- Historical record of incidents and how they were resolved
20+
- Templates for future documentation improvements
21+
- Learning resources for understanding project-specific failure modes
22+
- Reference for similar issues in the future
23+
24+
## Related
25+
26+
The analysis in these files led to the creation of:
27+
28+
- `.claude/docs/testing-build-scripts.md`
29+
- `.claude/docs/master-health-monitoring.md`
30+
- `.claude/docs/managing-file-paths.md`
31+
32+
See also PR #2062 and PR #2065 for the full context.

0 commit comments

Comments
 (0)