fix(update-system): include all language modes and current system scripts#366
fix(update-system): include all language modes and current system scripts#366FReptar0 wants to merge 1 commit intosantifer:mainfrom
Conversation
📝 WalkthroughWalkthroughExpanded the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
update-system.mjs (1)
48-67:⚠️ Potential issue | 🟠 MajorMake rollback delete paths that did not exist in the backup branch.
These newly tracked modes/scripts can be introduced by
apply()on older installs, butrollback()currently swallows checkout failures when a path was absent from the backup branch. That leaves update-added files/directories behind instead of restoring the backup state.🐛 Proposed rollback fix
// Checkout system files from backup branch + const pathsToStage = []; for (const path of SYSTEM_PATHS) { try { git('checkout', latest, '--', path); - } catch { - // File may not have existed in backup + pathsToStage.push(path); + } catch (err) { + const pathspec = path.endsWith('/') ? path.slice(0, -1) : path; + let existedInBackup = true; + try { + git('cat-file', '-e', `${latest}:${pathspec}`); + } catch { + existedInBackup = false; + } + + if (existedInBackup) throw err; + + // Path did not exist in backup; remove the update-added path. + git('rm', '-r', '-f', '--ignore-unmatch', '--', pathspec); } } - addPaths(SYSTEM_PATHS); + addPaths(pathsToStage);Also applies to: 302-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@update-system.mjs` around lines 48 - 67, rollback() currently swallows failures when checking out paths from the backup branch, leaving files/dirs introduced by apply() behind; update rollback() so that when the git checkout (or equivalent checkoutPath) for a given path fails because the path does not exist in the backup branch, you delete that local path instead (use fs.promises.rm / recursive delete) and still treat other errors as real failures; apply the same behavior to the block referenced around lines 302-310. Locate the rollback() function and the path-checkout logic (and any variable like paths, checkoutPath, or the loop that iterates over the file list) and add explicit handling: on checkout error detect "path not found" and remove the path, otherwise rethrow/log the error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@update-system.mjs`:
- Around line 48-67: rollback() currently swallows failures when checking out
paths from the backup branch, leaving files/dirs introduced by apply() behind;
update rollback() so that when the git checkout (or equivalent checkoutPath) for
a given path fails because the path does not exist in the backup branch, you
delete that local path instead (use fs.promises.rm / recursive delete) and still
treat other errors as real failures; apply the same behavior to the block
referenced around lines 302-310. Locate the rollback() function and the
path-checkout logic (and any variable like paths, checkoutPath, or the loop that
iterates over the file list) and add explicit handling: on checkout error detect
"path not found" and remove the path, otherwise rethrow/log the error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 02f6e3a7-f19a-4201-bf82-8bf968a3007c
📒 Files selected for processing (1)
update-system.mjs
update-system.mjs hardcodes the set of paths it refreshes from upstream. The list had fallen behind the repo in several ways: Missing language-mode directories: - Only `modes/de/` was listed. `modes/fr/`, `modes/ja/`, `modes/pt/`, `modes/ru/` existed in the tree but were silently skipped. Missing system-layer scripts at repo root: - scan.mjs, doctor.mjs, check-liveness.mjs, liveness-core.mjs, analyze-patterns.mjs, followup-cadence.mjs, test-all.mjs. Missing recently-merged system files: - modes/latex.md (santifer#362) - generate-latex.mjs (santifer#362) - gemini-eval.mjs (santifer#349) - GEMINI.md (santifer#349) - .gemini/commands/ (santifer#349) As a result, users running `/career-ops update` (or `node update-system.mjs apply`) would believe they were fully up to date while large parts of the system layer drifted silently — especially after the recent LaTeX and Gemini CLI additions. Add every missing entry to SYSTEM_PATHS. None of the new entries overlap with USER_PATHS, so the existing safety check is unaffected. Fixes santifer#337
FReptar0
force-pushed
the
fix/system-paths-missing-modes-scripts
branch
from
22fb9bb to
1b8cb81
Compare
|
Re: CodeRabbit's rollback finding — valid point, but intentionally out of scope for this PR. The Happy to open a follow-up PR addressing #337's sibling issue — let me know if you'd prefer it bundled here instead. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@update-system.mjs`:
- Around line 47-78: The PR adds many paths to the hard-coded SYSTEM_PATHS array
but the long-term fix is to derive SYSTEM_PATHS from a single source of truth to
avoid drift; update update-system.mjs to generate SYSTEM_PATHS at runtime (or at
build) by reading a committed manifest (e.g., system-manifest.json) or running a
filtered git ls-tree and excluding USER_PATHS, then have apply() and rollback()
consume that generated list instead of the hand-maintained array; ensure
functions that reference SYSTEM_PATHS (apply(), rollback(), any validation
logic) still receive the same shape (array of paths) and add fallback behavior
to preserve current hard-coded entries if the manifest/git scan fails.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a8d08c60-deca-46b1-998c-0108f8ef15ec
📒 Files selected for processing (1)
update-system.mjs
| 'modes/latex.md', | ||
| 'modes/de/', | ||
| 'modes/fr/', | ||
| 'modes/ja/', | ||
| 'modes/pt/', | ||
| 'modes/ru/', | ||
| 'CLAUDE.md', | ||
| 'AGENTS.md', | ||
| 'GEMINI.md', | ||
| 'generate-pdf.mjs', | ||
| 'generate-latex.mjs', | ||
| 'merge-tracker.mjs', | ||
| 'verify-pipeline.mjs', | ||
| 'dedup-tracker.mjs', | ||
| 'normalize-statuses.mjs', | ||
| 'cv-sync-check.mjs', | ||
| 'update-system.mjs', | ||
| 'scan.mjs', | ||
| 'doctor.mjs', | ||
| 'check-liveness.mjs', | ||
| 'liveness-core.mjs', | ||
| 'analyze-patterns.mjs', | ||
| 'followup-cadence.mjs', | ||
| 'gemini-eval.mjs', | ||
| 'test-all.mjs', | ||
| 'batch/batch-prompt.md', | ||
| 'batch/batch-runner.sh', | ||
| 'dashboard/', | ||
| 'templates/', | ||
| 'fonts/', | ||
| '.claude/skills/', | ||
| '.gemini/commands/', |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -u
paths=(
"modes/latex.md"
"modes/fr" "modes/ja" "modes/pt" "modes/ru"
"GEMINI.md"
"generate-latex.mjs" "scan.mjs" "doctor.mjs"
"check-liveness.mjs" "liveness-core.mjs"
"analyze-patterns.mjs" "followup-cadence.mjs"
"gemini-eval.mjs" "test-all.mjs"
".gemini/commands"
)
for p in "${paths[@]}"; do
if [ -e "$p" ]; then
echo "OK $p"
else
echo "MISSING $p"
fi
doneRepository: santifer/career-ops
Length of output: 415
Additions look correct; consider eliminating drift by deriving SYSTEM_PATHS from a manifest.
All newly added entries are system-layer paths with no overlap with USER_PATHS, so the safety check in apply() is unaffected and the additions are safe. All entries have been verified to exist on the repository. However, this PR only closes the current gap — the root cause called out in issue #337 (hand-maintained list drifts from what's actually checked in) remains. A follow-up that generates SYSTEM_PATHS from a single source of truth (e.g., a committed system-manifest.json, or a git ls-tree filtered by a USER_PATHS blocklist) would prevent the next round of missing files (e.g., future modes/<lang>/ directories or new root scripts) from silently being skipped by apply()/rollback().
Per the coding guidelines, script files like update-system.mjs are auto-updatable system files with no user-specific content — the added entries correctly belong in the system allowlist.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@update-system.mjs` around lines 47 - 78, The PR adds many paths to the
hard-coded SYSTEM_PATHS array but the long-term fix is to derive SYSTEM_PATHS
from a single source of truth to avoid drift; update update-system.mjs to
generate SYSTEM_PATHS at runtime (or at build) by reading a committed manifest
(e.g., system-manifest.json) or running a filtered git ls-tree and excluding
USER_PATHS, then have apply() and rollback() consume that generated list instead
of the hand-maintained array; ensure functions that reference SYSTEM_PATHS
(apply(), rollback(), any validation logic) still receive the same shape (array
of paths) and add fallback behavior to preserve current hard-coded entries if
the manifest/git scan fails.
1 participant
Summary
Fixes #337.
`update-system.mjs` hardcodes the set of paths it refreshes from upstream. The list had drifted from the actual repo:
Missing language-mode directories:
Missing system-layer scripts:
As a result, users running `/career-ops update` (or `node update-system.mjs apply`) saw a "✅ Updated" confirmation while large parts of the system layer silently drifted behind upstream.
Fix
Add every missing language-mode directory and system script to `SYSTEM_PATHS`. Minimum-scope fix that matches the issue description ("at minimum, it should include..."). No refactor to a generated manifest in this PR — that would be a separate discussion.
1 file changed, 11 insertions.
Safety
None of the 11 new entries overlap with `USER_PATHS`:
```js
const USER_PATHS = [
'cv.md', 'config/profile.yml', 'modes/_profile.md',
'portals.yml', 'article-digest.md',
'interview-prep/story-bank.md',
'data/', 'reports/', 'output/', 'jds/',
];
```
The existing safety-violation check in `apply()` is therefore unaffected.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit