|
| 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 | +``` |
0 commit comments