#1885 fix: run every pending migration in version order and record it - #1886
#1885 fix: run every pending migration in version order and record it#1886RAprogramm wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMigration execution now processes all unapplied scripts in version order and persists successful applications. Dot-metafile validation distinguishes remote sources from local paths and applies stricter source, type, containment, and existence checks. ChangesMigration tracking
Metafile validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant MigrationRunner as run_pending_migrations
participant MigrationScript
participant StateFile
Installer->>MigrationRunner: provide migration directory and state file
MigrationRunner->>MigrationScript: execute each unapplied script in version order
MigrationScript-->>MigrationRunner: success or failure
MigrationRunner->>StateFile: record successful filename
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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.
Actionable comments posted: 3
🤖 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 `@Scripts/global_fn.sh`:
- Line 211: Update the migration status logic around the applied counter so the
“No outstanding migrations” message is based on whether any unrecorded migration
was encountered, not whether applied remains zero. Track that encounter
separately while processing pending migrations, and preserve the message only
when none were found, including when encountered migrations fail.
In `@tests/python/check_dots.py`:
- Around line 155-157: Update the path validation logic in the check_dots test
around inside and has_glob to expand each glob pattern and validate every
resolved match with inside(root, match), rejecting matches outside source_root.
Preserve the existing existence-check exemption only for patterns with no
matches; literal paths and unmatched globs must retain their current handling.
In `@tests/test_migrations.sh`:
- Around line 49-53: Extend the migration test around run_pending_migrations to
make v26.11.0.sh write a failure marker, then invoke the migration runner again
and assert the order log contains two entries, proving the failed migration is
retried. Also verify v26.11.0 remains absent from the state file after the
second invocation, while preserving the existing check that successful
migrations are not rerun.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 56dcd812-1609-4417-85eb-036db3c6083c
📒 Files selected for processing (6)
CHANGELOG.mdScripts/global_fn.shScripts/install.shtests/README.mdtests/python/check_dots.pytests/test_migrations.sh
| if not inside(root, relative): | ||
| fail(f"{where} points outside its source_root with path {relative!r}") | ||
| elif not has_glob(relative) and not (root / relative).exists(): |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate resolved glob matches, not just the literal pattern.
inside(root, relative) resolves * literally. A path such as linked/*, where linked is a symlink outside source_root, passes Line 155 and then bypasses Line 157’s existence check. Expand matched globs and apply containment validation to every match; only unmatched-glob existence checks should remain exempt.
Proposed fix
- elif not has_glob(relative) and not (root / relative).exists():
+ elif has_glob(relative):
+ for match in root.glob(relative):
+ if not inside(root, str(match.relative_to(root))):
+ fail(f"{where} glob escapes its source_root with path {relative!r}")
+ elif not (root / relative).exists():
fail(f"{where} points at a missing path {relative!r}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not inside(root, relative): | |
| fail(f"{where} points outside its source_root with path {relative!r}") | |
| elif not has_glob(relative) and not (root / relative).exists(): | |
| if not inside(root, relative): | |
| fail(f"{where} points outside its source_root with path {relative!r}") | |
| if has_glob(relative): | |
| for match in root.glob(relative): | |
| if not inside(root, str(match.relative_to(root))): | |
| fail(f"{where} glob escapes its source_root with path {relative!r}") | |
| elif not (root / relative).exists(): | |
| fail(f"{where} points at a missing path {relative!r}") |
🤖 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 `@tests/python/check_dots.py` around lines 155 - 157, Update the path
validation logic in the check_dots test around inside and has_glob to expand
each glob pattern and validate every resolved match with inside(root, match),
rejecting matches outside source_root. Preserve the existing existence-check
exemption only for patterns with no matches; literal paths and unmatched globs
must retain their current handling.
| : >"$order_log" | ||
| run_pending_migrations "$migration_dir" "$state_file" >/dev/null 2>&1 | ||
|
|
||
| [ -s "$order_log" ] && | ||
| fail "already applied migrations ran a second time: $(tr '\n' ' ' <"$order_log")" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert that failed migrations are retried.
The second invocation only verifies successful migrations did not rerun. Make v26.11.0.sh write a failure marker and assert two entries after the second invocation; also recheck that it remains absent from the state file.
🤖 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 `@tests/test_migrations.sh` around lines 49 - 53, Extend the migration test
around run_pending_migrations to make v26.11.0.sh write a failure marker, then
invoke the migration runner again and assert the order log contains two entries,
proving the failed migration is retried. Also verify v26.11.0 remains absent
from the state file after the second invocation, while preserving the existing
check that successful migrations are not rerun.
|
Three follow-ups in 5d42806, from re-reviewing the runner. Migrations no longer inherit the loop's stdin. Honest reporting. The "no outstanding migrations" line was keyed on the count of successes, so a run where a migration was found and failed printed both the failure warning and "no outstanding migrations". It now keys on whether anything was pending, and the test asserts that message cannot appear alongside a pending migration. Test diagnostics. Sourcing Suite: 9 cases, 0 failed. |
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 `@tests/test_migrations.sh`:
- Around line 66-67: Strengthen the assertion in the migration test around
run_pending_migrations so it verifies an actual invocation from
Scripts/install.sh rather than any matching text. Match the command in an
executable position with appropriate shell syntax, or run the installer using a
stubbed migration runner and assert that the runner is called.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b337e0c5-42e1-415b-bf3f-7c92589f2e80
📒 Files selected for processing (2)
Scripts/global_fn.shtests/test_migrations.sh
…ler really calls the runner
|
Both test findings addressed. Failed migrations are asserted to retry. The installer check is no longer a substring match. It now requires The first finding — reporting "no outstanding" when every pending migration failed — was already fixed in 5d42806 before this review landed: the message is keyed on whether anything was pending, not on the success count, and the test asserts the two cannot coexist. Suite: 9 cases, 0 failed. |
1 participant
Closes #1885
Problem
The installer ran exactly one migration — the lexicographically highest filename — and kept no record of what had been applied. Three consequences:
Scripts/migrations/holdsv25.8.2.sh,v25.9.1.sh,v26.4.3.sh,v26.7.4.sh, and only the last one ever executed, so anyone skipping a release never got the ones in between.sort -ris lexicographic, sov26.10.1would have sorted belowv26.4.3and the newest migration would have been the one skipped.v26.8.x.shretiresv26.7.4.shfor everyone — including users who have not upgraded yet and still need it to stop the superseded shell helpers from shadowing their replacements.Change
The runner moved out of
install.shintorun_pending_migrationsinScripts/global_fn.sh, which takes the migration directory and a record file. It runs every migration absent from that record, ordered withsort -V, and appends a name only after that migration exits zero. A failure stays a warning, as before, but is not recorded, so it is retried on the next run.The record lives at
${XDG_STATE_HOME}/hyde/migration/applied.Existing migrations are all safe to re-run — three only print notices or reload state, and
v26.7.4.shskips any file whose backup already exists — so an installed base with no record simply replays them once.Tests
tests/test_migrations.sh(new) covers version ordering withv26.10.1afterv26.4.3, recording of successes, a non-zero migration staying unrecorded, no re-run on a second pass, and silence on a missing directory.Full suite: 9 cases, 0 failed.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation