Enhances project version synchronization processes - #165
Conversation
- Cargo.lock was stuck at version 25.9.1 while Cargo.toml was at 25.9.3 - Ran cargo update to regenerate Cargo.lock with correct version - This resolves version synchronization issue between build files
- Added update_cargo_lock() function to run 'cargo update --package wfl' - Integrated Cargo.lock update into --update-all workflow - This prevents version drift between Cargo.toml and Cargo.lock - Includes proper error handling and subprocess management - Resolves root cause of version synchronization issues
Provides a comprehensive guide on how version synchronization works across project files. Explains the causes of version drift, details automated and manual synchronization processes, and outlines prevention strategies. Includes troubleshooting tips and best practices to ensure consistency and prevent build or deployment issues.
- Updated package.metadata.bundle version from 25.8.15 to 25.9.3 to match main package version - Fixed regex pattern in bump_version.py that was failing to match bundle version field - Changed from [^\[]* to .*? with DOTALL flag for proper multiline matching - This resolves the remaining version synchronization discrepancy in Cargo.toml - All version fields in Cargo.toml now consistently show 25.9.3
- Document the dual version fields in Cargo.toml (package + bundle metadata) - Add verification commands for checking both version fields - Include troubleshooting for bundle version synchronization issues - Update common issues section with regex pattern problems
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughBumps crate version in Cargo.toml, adds version-synchronization docs, extends the bump script to update/verify Cargo.lock, adds file-flush durability in the interpreter, and introduces tests and a WFL test program to validate file-flush/race behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant Script as bump_version.py
participant FS as Filesystem
participant Cargo as cargo
Dev->>Script: Run --update-all NEW_VERSION
Script->>FS: update `Cargo.toml` (root & metadata.bundle)
Script->>Cargo: run `cargo update` (update_cargo_lock)
Note over Script,Cargo: extract/verify version in `Cargo.lock`
alt version matches
Script->>FS: mark `Cargo.lock` modified (append to MODIFIED_FILES)
Script-->>Dev: success
else mismatch / error
Script-->>Dev: exit non-zero with error
end
sequenceDiagram
autonumber
participant WFL as Interpreter
participant FS as Filesystem
participant Test as TestHarness
Test->>WFL: request write_file(path, data)
WFL->>FS: open & write
WFL->>FS: flush file handle
alt flush success
WFL->>FS: best-effort sync_all (non-fatal)
WFL-->>Test: return success
else flush failure
WFL-->>Test: return error "Failed to flush file: ..."
end
Note right of WFL: append_file and close_file follow same flush-then-sync pattern
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
Claude finished @logbie's task —— View job PR Review - Version Synchronization EnhancementsReview Checklist
🎯 Overall AssessmentThis PR significantly improves the project's version management infrastructure. The changes are well-thought-out and address a critical need for version synchronization across multiple files. I recommend approval with minor suggestions below. 📋 Detailed Review✅ Cargo.toml Changes (
|
There was a problem hiding this comment.
Pull Request Overview
This pull request enhances version synchronization processes across the WFL project by fixing the automated version bump script, adding comprehensive documentation, and updating the core package version.
- Updates the version bump script to properly synchronize both
Cargo.tomlversion fields and automatically updateCargo.lock - Adds detailed documentation explaining version synchronization mechanisms and best practices
- Updates WFL core package version from
25.8.15to25.9.3
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| scripts/bump_version.py | Fixes regex pattern for bundle metadata version matching and adds automatic Cargo.lock synchronization |
| Docs/development/version-synchronization.md | Introduces comprehensive guide for version management and troubleshooting |
| Cargo.toml | Updates bundle metadata version from 25.8.15 to 25.9.3 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if not os.path.exists(CARGO_TOML): | ||
| print(f"Warning: {CARGO_TOML} not found, skipping Cargo.lock update") | ||
| return False |
There was a problem hiding this comment.
The function checks for CARGO_TOML existence but CARGO_TOML is not defined in this function scope. This should check os.path.exists('Cargo.toml') or define CARGO_TOML locally.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
scripts/bump_version.py (1)
116-153: Harden Cargo.lock update: remove unused var, avoid S607, prefer generate-lockfile
- F841: variable “result” is unused.
- S607: invoke cargo via absolute path (shutil.which) to avoid partial-path exec concerns.
- Prefer
cargo generate-lockfileto update the lock without unexpectedly refreshing dependency versions; fall back tocargo update -p wflif needed.Apply this diff within update_cargo_lock:
def update_cargo_lock(): """Update Cargo.lock to match Cargo.toml version by running cargo update.""" - import subprocess - CARGO_LOCK = "Cargo.lock" if not os.path.exists(CARGO_TOML): print(f"Warning: {CARGO_TOML} not found, skipping Cargo.lock update") return False print("Updating Cargo.lock to match Cargo.toml version...") try: - # Run cargo update to regenerate Cargo.lock with new version - result = subprocess.run( - ["cargo", "update", "--package", "wfl"], - capture_output=True, - text=True, - check=True - ) + # Resolve cargo binary and regenerate lockfile without bumping deps + import shutil + cargo_bin = shutil.which("cargo") + if not cargo_bin: + print("Error: cargo command not found. Make sure Rust/Cargo is installed.") + return False + # Safer: just regenerate lockfile to pick up root version change + subprocess.run([cargo_bin, "generate-lockfile"], capture_output=True, text=True, check=True) + # If Cargo.lock still missing (older toolchains), fall back to targeted update + if not os.path.exists(CARGO_LOCK): + subprocess.run([cargo_bin, "update", "--package", "wfl"], capture_output=True, text=True, check=True) if os.path.exists(CARGO_LOCK): MODIFIED_FILES.append(CARGO_LOCK) print("Successfully updated Cargo.lock") return True else: print(f"Warning: {CARGO_LOCK} not found after cargo update") return False except subprocess.CalledProcessError as e: print(f"Error running cargo update: {e}") print(f"stdout: {e.stdout}") print(f"stderr: {e.stderr}") return False - except FileNotFoundError: - print("Error: cargo command not found. Make sure Rust/Cargo is installed.") - return FalseAdditionally, at the top of the file, ensure
import shutilis present:import subprocess +import shutilDocs/development/version-synchronization.md (3)
57-64: Prefer generate-lockfile first; reserve update -p for fallbackRecommend documenting
cargo generate-lockfileas the primary way to refresh Cargo.lock after a version bump, then suggestcargo update -p wflas a fallback. This avoids unintended dependency refreshes.Proposed snippet:
# Refresh lockfile to pick up the new root version without updating deps cargo generate-lockfile # If the lockfile isn't created/updated (older toolchains), then: cargo update -p wfl
100-110: Make the pre-commit check more resilientLimit the grep window strictly to the bundle section to avoid false matches, and handle missing entries gracefully.
#!/bin/bash # .git/hooks/pre-commit set -euo pipefail CARGO_VERSION=$(grep -nP '^\s*version\s*=\s*"' Cargo.toml | head -1 | sed 's/.*"\(.*\)".*/\1/') BUNDLE_VERSION=$(awk '/^\[package\.metadata\.bundle\]/{f=1;next} /^\[/{f=0} f && /version *=/ {print; exit}' Cargo.toml | sed 's/.*"\(.*\)".*/\1/') LOCK_VERSION=$(grep -A1 'name = "wfl"' Cargo.lock | grep -oP '"\K[^"]+(?=")' | head -1 || true) if [ -z "${BUNDLE_VERSION:-}" ] || [ -z "${LOCK_VERSION:-}" ] || [ "$CARGO_VERSION" != "$BUNDLE_VERSION" ] || [ "$CARGO_VERSION" != "$LOCK_VERSION" ]; then echo "Version mismatch or missing version fields." echo "Run: cargo generate-lockfile || cargo update -p wfl" exit 1 fi
148-159: Great verification section — add a quick cargo checkAdd
cargo --versionandrustc --versionto help diagnose PATH/toolchain issues when the script fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.toml(1 hunks)Docs/development/version-synchronization.md(1 hunks)scripts/bump_version.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.13.1)
scripts/bump_version.py
130-130: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
131-131: Starting a process with a partial executable path
(S607)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Integration Tests (ubuntu-latest)
- GitHub Check: Integration Tests (windows-latest)
- GitHub Check: Build, Test, Clippy
- GitHub Check: claude-review
🔇 Additional comments (2)
scripts/bump_version.py (1)
106-107: Regex fix for bundle section is correct — DOTALL is appropriateUsing DOTALL to span the bundle section is the right call and prevents partial matches across newlines.
Please test against a Cargo.toml where bundle keys are split by comments/blank lines to ensure only the intended version field is touched.
Cargo.toml (1)
13-13: LGTM — versions are in sync. Consider adding a CI check to enforce thatpackage.versionandpackage.metadata.bundle.versionremain aligned.
…version validation - Replace silent failures with sys.exit(1) to ensure CI failure on cargo errors - Add comprehensive version extraction and validation from both Cargo.toml and Cargo.lock - Use cross-platform Python regex instead of grep/sed for Windows compatibility - Extract WFL-specific version from Cargo.lock (not first dependency version) - Verify exact version match between Cargo.toml and Cargo.lock after cargo update - Add detailed error messages for debugging version synchronization issues - Ensures CI will fail if cargo update doesn't properly synchronize versions
- Document that version bump script now fails CI on cargo errors - Add troubleshooting for script exit code 1 scenarios - Update best practices to expect CI failures on synchronization issues - Clarify that sys.exit(1) is intended behavior for error conditions
|
Claude finished @logbie's task —— View job PR Review - Version Synchronization EnhancementsReview Checklist
🎯 Overall AssessmentThis PR significantly improves the project's version management infrastructure. The changes are well-thought-out and address a critical need for version synchronization across multiple files. I recommend approval with minor suggestions below. 📋 Detailed Review✅ Cargo.toml Changes (
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
Docs/development/version-synchronization.md (3)
71-83: Make Cargo.lock version check resilient to field ordering
grep -A1 'name = "wfl"' Cargo.lockassumesversionis the very next line, which can break if Cargo reorders or inserts fields. Suggest scanning the [[package]] block for wfl and then extracting its version.Suggested replacement for “Verify Synchronization”:
# Cargo.lock version (robust) python - <<'PY' import re, sys lock = open("Cargo.lock", "r", encoding="utf-8").read() for blk in lock.split("[[package]]"): if re.search(r'(?m)^\s*name\s*=\s*"wfl"\s*$', blk): m = re.search(r'(?m)^\s*version\s*=\s*"([^"]+)"', blk) if not m: print("Could not find version for wfl in Cargo.lock", file=sys.stderr); sys.exit(1) print(m.group(1)); sys.exit(0) print("wfl package not found in Cargo.lock", file=sys.stderr); sys.exit(1) PY
102-113: Harden the pre-commit hook against Cargo.lock field reorderingSame assumption as above. Replace the
grep -A1approach with a small Python snippet inside the hook to parse the wfl block robustly; keeps false positives low.Minimal drop-in:
# inside .git/hooks/pre-commit CARGO_VERSION="$(awk 'BEGIN{pkg=0} /^\[package\]/{pkg=1;next} /^\[/{if(pkg) exit} pkg && /^\s*version\s*=/ {gsub(/.*"|"|.*/,""); print; exit}' Cargo.toml)" LOCK_VERSION="$(python - <<'PY' import re, sys lock = open("Cargo.lock","r",encoding="utf-8").read() for blk in lock.split("[[package]]"): if re.search(r'(?m)^\s*name\s*=\s*"wfl"\s*$', blk): m = re.search(r'(?m)^\s*version\s*=\s*"([^"]+)"', blk) print(m.group(1) if m else ""); sys.exit(0) print(""); sys.exit(0) PY )"
119-127: Strengthen CI validation stepFor the same reason, prefer a resilient extraction of the wfl version from Cargo.lock in CI. Example:
- name: Validate version synchronization run: | CARGO_VERSION="$(awk 'BEGIN{pkg=0} /^\[package\]/{pkg=1;next} /^\[/{if(pkg) exit} pkg && /^\s*version\s*=/ {gsub(/.*"|"|.*/,""); print; exit}' Cargo.toml)" LOCK_VERSION="$(python - <<'PY' import re, sys lock = open("Cargo.lock","r",encoding="utf-8").read() for blk in lock.split("[[package]]"): if re.search(r'(?m)^\s*name\s*=\s*"wfl"\s*$', blk): m = re.search(r'(?m)^\s*version\s*=\s*"([^"]+)"', blk) print(m.group(1) if m else ""); sys.exit(0) print(""); sys.exit(0) PY )" test -n "$LOCK_VERSION" test "$CARGO_VERSION" = "$LOCK_VERSION"scripts/bump_version.py (4)
121-121: Drop redundant local import
subprocessis already imported at module top. Remove the innerimport subprocessto reduce shadowing/confusion.- import subprocess
179-186: Make Cargo.lock parsing resilient to field orderingThe regex requires
versionimmediately aftername; it can fail if other keys appear in between. Scan the[[package]]block instead.- # Find WFL package version specifically (equivalent to grep -A1 'name = "wfl"') - wfl_package_match = re.search(r'\[\[package\]\]\s*name = "wfl"\s*version = "([^"]+)"', cargo_lock_content, re.DOTALL) - if not wfl_package_match: - print("Error: Could not find WFL package version in Cargo.lock") - sys.exit(1) - - actual_version = wfl_package_match.group(1) + # Locate the [[package]] block for wfl and extract its version + blocks = cargo_lock_content.split("[[package]]") + wfl_block = next((b for b in blocks if re.search(r'(?m)^\s*name\s*=\s*"wfl"\s*$', b)), None) + if not wfl_block: + print("Error: Could not find WFL package block in Cargo.lock") + sys.exit(1) + m = re.search(r'(?m)^\s*version\s*=\s*"([^"]+)"', wfl_block) + if not m: + print("Error: Could not extract version for wfl in Cargo.lock") + sys.exit(1) + actual_version = m.group(1)
146-149: Avoid broadexcept Exception; catch specific errorsCatching everything obscures actionable failures and trips linters (BLE001). Narrow to I/O/encoding/regex errors.
- except Exception as e: + except (OSError, UnicodeDecodeError) as e: print(f"Error reading Cargo.toml: {e}") sys.exit(1) @@ - except Exception as e: - print(f"Error validating Cargo.lock: {e}") + except (OSError, UnicodeDecodeError, re.error) as e: + print(f"Error validating Cargo.lock: {e}") sys.exit(1)Also applies to: 199-201
18-26: Optional: wire--verboseto control noisy outputMany prints are unconditional; consider guarding with
args.verboseor a simple logger to keep CI logs focused.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Docs/development/version-synchronization.md(1 hunks)scripts/bump_version.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.13.1)
scripts/bump_version.py
146-146: Do not catch blind exception: Exception
(BLE001)
152-152: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
153-153: Starting a process with a partial executable path
(S607)
190-190: f-string without any placeholders
Remove extraneous f prefix
(F541)
199-199: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Integration Tests (ubuntu-latest)
- GitHub Check: Build, Test, Clippy
- GitHub Check: Integration Tests (windows-latest)
🔇 Additional comments (3)
scripts/bump_version.py (3)
106-107: Good: DOTALL for bundle blockEnabling DOTALL makes the bundle-version replacement robust across multi-line sections. LGTM.
304-305: Nice: lockfile update integrated into --update-allUpdating Cargo.lock right after Cargo.toml reduces drift windows. LGTM.
116-202: The proposed “one-shot” script still invokescargo update, so Cargo must be installed. Either ensure Cargo is available in your CI/dev environment or refactor the check to compare versions directly fromCargo.tomlandCargo.lockwithout callingcargo.Likely an incorrect or invalid review comment.
| result = subprocess.run( | ||
| ["cargo", "update", "--package", "wfl"], | ||
| capture_output=True, | ||
| text=True, | ||
| check=True | ||
| ) | ||
| print("Cargo update completed successfully") | ||
|
|
There was a problem hiding this comment.
Remove unused variable and keep check=True
result isn’t used. Minor cleanup.
- result = subprocess.run(
+ subprocess.run(
["cargo", "update", "--package", "wfl"],
capture_output=True,
text=True,
check=True
)📝 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.
| result = subprocess.run( | |
| ["cargo", "update", "--package", "wfl"], | |
| capture_output=True, | |
| text=True, | |
| check=True | |
| ) | |
| print("Cargo update completed successfully") | |
| subprocess.run( | |
| ["cargo", "update", "--package", "wfl"], | |
| capture_output=True, | |
| text=True, | |
| check=True | |
| ) | |
| print("Cargo update completed successfully") |
🧰 Tools
🪛 Ruff (0.13.1)
152-152: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
153-153: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
In scripts/bump_version.py around lines 152 to 159, the subprocess.run call
assigns its result to an unused variable `result`; remove the unused assignment
and call subprocess.run(...) directly while keeping check=True (and retaining
capture_output/text if desired) so the call still raises on failure, then keep
the subsequent print statement.
| # Verify versions match | ||
| if expected_version != actual_version: | ||
| print(f"Error: Version mismatch!") | ||
| print(f" Cargo.toml version: {expected_version}") | ||
| print(f" Cargo.lock version: {actual_version}") | ||
| print("Cargo.lock was not properly synchronized") | ||
| sys.exit(1) | ||
|
|
||
| print(f"✓ Version synchronization verified: {expected_version}") | ||
| MODIFIED_FILES.append(CARGO_LOCK) |
There was a problem hiding this comment.
Tiny nit: remove superfluous f-string; improve mismatch message
- if expected_version != actual_version:
- print(f"Error: Version mismatch!")
+ if expected_version != actual_version:
+ print("Error: Version mismatch!")
print(f" Cargo.toml version: {expected_version}")
print(f" Cargo.lock version: {actual_version}")
print("Cargo.lock was not properly synchronized")
sys.exit(1)📝 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.
| # Verify versions match | |
| if expected_version != actual_version: | |
| print(f"Error: Version mismatch!") | |
| print(f" Cargo.toml version: {expected_version}") | |
| print(f" Cargo.lock version: {actual_version}") | |
| print("Cargo.lock was not properly synchronized") | |
| sys.exit(1) | |
| print(f"✓ Version synchronization verified: {expected_version}") | |
| MODIFIED_FILES.append(CARGO_LOCK) | |
| # Verify versions match | |
| if expected_version != actual_version: | |
| print("Error: Version mismatch!") | |
| print(f" Cargo.toml version: {expected_version}") | |
| print(f" Cargo.lock version: {actual_version}") | |
| print("Cargo.lock was not properly synchronized") | |
| sys.exit(1) | |
| print(f"✓ Version synchronization verified: {expected_version}") | |
| MODIFIED_FILES.append(CARGO_LOCK) |
🧰 Tools
🪛 Ruff (0.13.1)
190-190: f-string without any placeholders
Remove extraneous f prefix
(F541)
Ensures the package version is extracted specifically from the `[package]` section of `Cargo.toml`. This prevents potential misidentification if a `version` field exists elsewhere in the file, making the version bumping process more robust. Removes an unused variable assignment in the `cargo update` subprocess call for cleaner code.
|
Claude finished @logbie's task —— View job PR Review - Version Synchronization EnhancementsReview Checklist
🎯 Overall AssessmentThis PR significantly improves the project's version management infrastructure. The changes are well-thought-out and address a critical need for version synchronization across multiple files. I recommend approval with minor suggestions below. 📋 Detailed Review✅ Cargo.toml Changes (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
scripts/bump_version.py (6)
106-107: Anchor bundle-version update to the exact table/key to avoid false matchesHarden the regex so it only touches the
versionkey inside[package.metadata.bundle]and ignores comments/other tables.- new_content = re.sub(r'(\[package\.metadata\.bundle\].*?version = )"([^"]*)"', - f'\\1"{semver_version}"', new_content, flags=re.DOTALL) + new_content = re.sub( + r'(^\s*\[package\.metadata\.bundle\][\s\S]*?^\s*version\s*=\s*)"([^"]*)"', + f'\\1"{semver_version}"', + new_content, + flags=re.MULTILINE, + )
121-121: Drop redundant import
subprocessis already imported at module scope; the local import is unnecessary.- import subprocess
140-151: Make[package]extraction andversionread resilient (indentation, anchoring)Allow leading whitespace and anchor to the table; also accept optional spaces around
version =.- package_match = re.search(r'\[package\](.*?)(?:\n\[|$)', cargo_toml_content, re.DOTALL) + package_match = re.search( + r'^\s*\[package\]\s*(?:#.*)?\n([\s\S]*?)(?=^\s*\[|\Z)', + cargo_toml_content, + re.MULTILINE, + ) @@ - version_match = re.search(r'^version = "([^"]+)"', package_section, re.MULTILINE) + version_match = re.search(r'^\s*version\s*=\s*"([^"]+)"', package_section, re.MULTILINE)
156-159: Avoid catching bareExceptionhere; narrow the scopeLimit to file/regex errors so unexpected issues still surface clearly. As per static analysis (BLE001).
- except Exception as e: - print(f"Error reading Cargo.toml: {e}") - sys.exit(1) + except (OSError, UnicodeDecodeError, re.error) as e: + print(f"Error reading/parsing Cargo.toml: {e}") + sys.exit(1)
190-197: Lockfile regex can miss when other keys sit betweennameandversionMake the search tolerant to intervening lines/fields (or parse TOML). The current pattern assumes
versiondirectly followsname.- wfl_package_match = re.search(r'\[\[package\]\]\s*name = "wfl"\s*version = "([^"]+)"', cargo_lock_content, re.DOTALL) + wfl_package_match = re.search( + r'^\s*\[\[package\]\]\s*[\r\n]+[\s\S]*?^\s*name\s*=\s*"wfl"[\s\S]*?^\s*version\s*=\s*"([^"]+)"', + cargo_lock_content, + re.MULTILINE, + )Optional (clearer and robust): parse as TOML using Python 3.11’s
tomllibortomlifallback.import sys try: import tomllib except ModuleNotFoundError: import tomli as tomllib data = tomllib.loads(cargo_lock_content) pkg = next((p for p in data.get("package", []) if p.get("name") == "wfl"), None) if not pkg: print("Error: Could not find WFL package in Cargo.lock"); sys.exit(1) actual_version = pkg["version"]
209-212: Avoid catching bareExceptionduring lock validationNarrow to I/O/decoding/regex errors. As per static analysis (BLE001).
- except Exception as e: - print(f"Error validating Cargo.lock: {e}") - sys.exit(1) + except (OSError, UnicodeDecodeError, re.error) as e: + print(f"Error validating Cargo.lock: {e}") + sys.exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/bump_version.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.13.1)
scripts/bump_version.py
156-156: Do not catch blind exception: Exception
(BLE001)
163-163: Starting a process with a partial executable path
(S607)
200-200: f-string without any placeholders
Remove extraneous f prefix
(F541)
209-209: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Integration Tests (windows-latest)
🔇 Additional comments (2)
scripts/bump_version.py (2)
200-204: Remove extraneous f-string (already flagged earlier)Minor nit; print literal without
f. As per static analysis (F541).- print(f"Error: Version mismatch!") + print("Error: Version mismatch!")
314-315: Approve lock sync integration
Cargo.toml and Cargo.lock versions are synchronized (25.9.3). Wiring and fail-fast behavior viaSystemExitare correct. LGTM.
Addresses potential data loss and race conditions in file I/O operations. Explicitly flushes file buffers to the operating system after write and append operations, making data immediately available for subsequent reads. Attempts to sync file data to disk before closing to enhance data durability, gracefully handling platforms where this is not supported. Adds dedicated WFL and Rust integration tests to validate correct flushing behavior across various scenarios, including rapid write-close cycles and append operations.
|
Claude finished @logbie's task —— View job PR Review - Version Synchronization EnhancementsReview Checklist
🎯 Overall AssessmentThis PR significantly improves the project's version management infrastructure. The changes are well-thought-out and address a critical need for version synchronization across multiple files. I recommend approval with minor suggestions below. 📋 Detailed Review✅ Cargo.toml Changes (
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/file_io_concurrent_test.rs (1)
252-290: Fix mismatch between declared and actual file count
The DSL comment and test description say “Create 10 files concurrently,” but only files 0–4 are opened, written, and verified. Either extend the open/write/close blocks and assertions to cover files 5–9 or update the comment and verification loop to reflect 5 files:- // Create 10 files concurrently + // Create 5 files concurrently
🧹 Nitpick comments (3)
tests/file_io_concurrent_test.rs (2)
320-358: LGTM: good coverage of write→close→immediate read race.This validates the interpreter’s durability path after close. Consider also asserting file size > 0 before reading for an extra safety net.
360-442: LGTM: rapid write/close cycles exercise flush/sync paths.Nice end-to-end validation. If you want stricter concurrency, consider running multiple such programs with
tokio::join!at the Rust level; the current WFL snippet executes sequentially.TestPrograms/file_flush_test.wfl (1)
1-102: Helpful demo; consider optional cleanup at end.To keep the workspace tidy when run locally/CI, optionally delete the test files at the end.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
TestPrograms/file_flush_test.wfl(1 hunks)src/interpreter/mod.rs(3 hunks)tests/file_io_concurrent_test.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T17:35:32.346Z
Learnt from: logbie
PR: WebFirstLanguage/wfl#150
File: tests/file_io_modes_test.rs:0-0
Timestamp: 2025-08-12T17:35:32.346Z
Learning: In the WFL project tests, file_io_execution_test.rs contains tests that actually execute WFL code through the interpreter and create real files, while file_io_modes_test.rs contains parser-only tests that don't execute code or create files during parsing.
Applied to files:
tests/file_io_concurrent_test.rsTestPrograms/file_flush_test.wfl
📚 Learning: 2025-08-11T05:10:43.202Z
Learnt from: logbie
PR: WebFirstLanguage/wfl#137
File: TestPrograms/test.wfl:1-2
Timestamp: 2025-08-11T05:10:43.202Z
Learning: Some test scripts in TestPrograms/ are deliberately designed to trigger errors as negative test cases to validate error handling and detection mechanisms. The file TestPrograms/test.wfl specifically tests variable redefinition error detection by attempting to use `store` twice on the same variable.
Applied to files:
TestPrograms/file_flush_test.wfl
🧬 Code graph analysis (1)
tests/file_io_concurrent_test.rs (2)
tests/file_io_execution_test.rs (2)
cleanup_test_files(12-16)execute_wfl_code(18-38)src/interpreter/mod.rs (2)
new(207-213)new(500-525)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
| Ok(_) => { | ||
| // Flush the data to ensure it's written to disk | ||
| match file_clone.flush().await { | ||
| Ok(_) => { | ||
| // Try to sync to disk for durability, but don't fail if it's not supported | ||
| let _ = file_clone.sync_all().await; | ||
| Ok(()) | ||
| } | ||
| Err(e) => Err(format!("Failed to flush file: {e}")), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Durability: don’t rely on flush(); propagate sync errors (optionally fallback).
flush() on tokio::fs::File doesn’t ensure persistence; the durability guarantee comes from sync_all/sync_data. Currently sync_all errors are swallowed, so a failed fsync still reports success. Recommend syncing and propagating errors (with optional fallback to sync_data).
- Ok(_) => {
- // Flush the data to ensure it's written to disk
- match file_clone.flush().await {
- Ok(_) => {
- // Try to sync to disk for durability, but don't fail if it's not supported
- let _ = file_clone.sync_all().await;
- Ok(())
- }
- Err(e) => Err(format!("Failed to flush file: {e}")),
- }
- }
+ Ok(_) => {
+ // Ensure durability: sync to disk; flush on File is a no‑op
+ if let Err(e) = file_clone.sync_all().await {
+ // Fallback for filesystems where full metadata sync isn’t supported
+ if let Err(e2) = file_clone.sync_data().await {
+ return Err(format!(
+ "Failed to persist file to disk: {e}; fallback also failed: {e2}"
+ ));
+ }
+ }
+ Ok(())
+ }📝 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.
| Ok(_) => { | |
| // Flush the data to ensure it's written to disk | |
| match file_clone.flush().await { | |
| Ok(_) => { | |
| // Try to sync to disk for durability, but don't fail if it's not supported | |
| let _ = file_clone.sync_all().await; | |
| Ok(()) | |
| } | |
| Err(e) => Err(format!("Failed to flush file: {e}")), | |
| } | |
| } | |
| Ok(_) => { | |
| // Ensure durability: sync to disk; flush on File is a no-op | |
| if let Err(e) = file_clone.sync_all().await { | |
| // Fallback for filesystems where full metadata sync isn’t supported | |
| if let Err(e2) = file_clone.sync_data().await { | |
| return Err(format!( | |
| "Failed to persist file to disk: {e}; fallback also failed: {e2}" | |
| )); | |
| } | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In src/interpreter/mod.rs around lines 386 to 396, the current code swallows
errors from file_clone.sync_all().await so a failed fsync still returns Ok;
change this to call sync_all().await and if it returns Err attempt a fallback to
sync_data().await, and if that also fails propagate an Err with the error
details (e.g. Err(format!("Failed to sync file: {}", err))). Ensure any
successful sync returns Ok(()) and do not ignore sync errors.
There was a problem hiding this comment.
Please open an issue for this
There was a problem hiding this comment.
🧩 Analysis chain
I'll create a GitHub issue for the file synchronization durability improvement.
@logbie I've created a GitHub issue to track the file synchronization durability improvement. The issue captures the specific problem with silently ignoring sync errors and includes the proposed solution with proper error handling and fallback to sync_data() when sync_all() fails.
🏁 Script executed:
#!/bin/bash
# Create GitHub issue for file durability improvement
gh issue create \
--title "Improve file write durability by properly handling sync errors" \
--body "## Issue Description
Currently in \`src/interpreter/mod.rs\`, the \`write_file\` method in \`IoClient\` has a durability issue where \`sync_all()\` errors are silently ignored, potentially leading to data loss if the sync fails.
## Current Problem
```rust
// Try to sync to disk for durability, but don't fail if it's not supported
let _ = file_clone.sync_all().await;
```
The current code swallows \`sync_all\` errors, so a failed fsync still reports success to the caller.
## Proposed Solution
Replace the silent error handling with proper error propagation and fallback:
```rust
// Ensure durability: sync to disk; flush on File is a no-op
if let Err(e) = file_clone.sync_all().await {
// Fallback for filesystems where full metadata sync isn't supported
if let Err(e2) = file_clone.sync_data().await {
return Err(format!(
\"Failed to persist file to disk: {e}; fallback also failed: {e2}\"
));
}
}
```
## Context
This issue was identified during code review of PR #165 which enhanced file I/O durability features.
**Related:**
- PR: https://github.com/WebFirstLanguage/wfl/pull/165
- Comment: https://github.com/WebFirstLanguage/wfl/pull/165#discussion_r2387503333
**Impact:** Data integrity issue where file writes may not be persisted to disk despite appearing successful.
**Files affected:** \`src/interpreter/mod.rs\` (write_file, append_file, close_file methods)" \
--assignee logbie
Length of output: 585
Perfect! I've successfully created GitHub issue #168 to track the file synchronization durability improvement. The issue has been assigned to you and includes:
- Clear description of the current problem with silent sync error handling
- The proposed solution with proper error propagation and fallback
- Context linking back to this PR and comment thread
- Impact assessment regarding data integrity
The bash syntax errors in the output were just the shell trying to interpret the Rust code blocks in the issue body, but the GitHub CLI successfully created the issue. You can view it at: #168
🧠 Learnings used
Learnt from: logbie
PR: WebFirstLanguage/wfl#150
File: tests/file_io_modes_test.rs:0-0
Timestamp: 2025-08-12T17:35:32.346Z
Learning: In the WFL project tests, file_io_execution_test.rs contains tests that actually execute WFL code through the interpreter and create real files, while file_io_modes_test.rs contains parser-only tests that don't execute code or create files during parsing.
| // Get the file handle before removing it | ||
| if let Some((_, mut file)) = file_handles.remove(handle_id) { | ||
| // Flush the file before closing to ensure all data is written to disk | ||
| match file.flush().await { | ||
| Ok(_) => { | ||
| // Try to sync to disk for durability, but don't fail if it's not supported | ||
| let _ = file.sync_all().await; | ||
| Ok(()) | ||
| } | ||
| Err(e) => Err(format!("Failed to flush file during close: {e}")), | ||
| } | ||
| } else { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Avoid holding the file_handles mutex across await; sync on close outside the lock.
The guard is held while awaiting flush()/sync_all(), which serializes all I/O through this mutex and can cause contention. Also, flush() on File is a no‑op; prefer syncing and propagate errors.
- // Get the file handle before removing it
- if let Some((_, mut file)) = file_handles.remove(handle_id) {
- // Flush the file before closing to ensure all data is written to disk
- match file.flush().await {
- Ok(_) => {
- // Try to sync to disk for durability, but don't fail if it's not supported
- let _ = file.sync_all().await;
- Ok(())
- }
- Err(e) => Err(format!("Failed to flush file during close: {e}")),
- }
- } else {
- Ok(())
- }
+ // Remove the handle, then drop the lock before any awaits
+ let file_opt = file_handles.remove(handle_id);
+ drop(file_handles);
+ if let Some((_, mut file)) = file_opt {
+ if let Err(e) = file.sync_all().await {
+ if let Err(e2) = file.sync_data().await {
+ return Err(format!(
+ "Failed to sync file during close: {e}; fallback also failed: {e2}"
+ ));
+ }
+ }
+ Ok(())
+ } else {
+ Ok(())
+ }📝 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.
| // Get the file handle before removing it | |
| if let Some((_, mut file)) = file_handles.remove(handle_id) { | |
| // Flush the file before closing to ensure all data is written to disk | |
| match file.flush().await { | |
| Ok(_) => { | |
| // Try to sync to disk for durability, but don't fail if it's not supported | |
| let _ = file.sync_all().await; | |
| Ok(()) | |
| } | |
| Err(e) => Err(format!("Failed to flush file during close: {e}")), | |
| } | |
| } else { | |
| Ok(()) | |
| } | |
| // Remove the handle, then drop the lock before any awaits | |
| let file_opt = file_handles.remove(handle_id); | |
| drop(file_handles); | |
| if let Some((_, mut file)) = file_opt { | |
| // Sync all metadata and data; on failure fall back to data-only sync | |
| if let Err(e) = file.sync_all().await { | |
| if let Err(e2) = file.sync_data().await { | |
| return Err(format!( | |
| "Failed to sync file during close: {e}; fallback also failed: {e2}" | |
| )); | |
| } | |
| } | |
| Ok(()) | |
| } else { | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In src/interpreter/mod.rs around lines 416-429, the code currently holds the
file_handles mutex while awaiting file.flush()/file.sync_all(), which serializes
async I/O and causes contention; instead, remove the entry from the map and take
ownership of the File while still holding the lock, immediately drop the lock,
then perform the async sync_all() outside the mutex and return any error to the
caller (omit or stop relying on flush() as it’s effectively a no-op and prefer
sync_all() for durability); ensure the mutex scope only covers removal and not
the await calls so other tasks can access file_handles concurrently.
There was a problem hiding this comment.
Please open an issue for this
There was a problem hiding this comment.
🧩 Analysis chain
I'll create a GitHub issue to track this mutex contention problem.
@logbie I've created issue for tracking the mutex contention problem in the close_file method. The issue includes technical details about the problem, the proposed solution, and references back to this PR and comment for context.
🏁 Script executed:
#!/bin/bash
# Create GitHub issue for the file_handles mutex contention problem
gh issue create \
--title "Fix file_handles mutex contention in IoClient::close_file method" \
--body "## Problem
The \`close_file\` method in \`src/interpreter/mod.rs\` currently holds the \`file_handles\` mutex while awaiting \`flush()\` and \`sync_all()\` operations, which serializes all I/O through this mutex and causes contention.
## Current Code Location
- **File**: \`src/interpreter/mod.rs\`
- **Lines**: Around 416-429
- **Method**: \`IoClient::close_file\`
## Issue Details
1. The mutex guard is held while awaiting async file operations
2. This prevents concurrent access to \`file_handles\` by other tasks
3. \`flush()\` on \`tokio::fs::File\` is effectively a no-op
4. Should prefer \`sync_all()\` for durability and propagate errors properly
## Proposed Solution
- Remove the file handle from the HashMap while holding the lock
- Drop the mutex guard immediately after removal
- Perform async sync operations outside the mutex scope
- Use \`sync_all()\` with fallback to \`sync_data()\` for proper error handling
## References
- **PR**: #165
- **Comment**: https://github.com/WebFirstLanguage/wfl/pull/165#discussion_r2387503347
- **Reported by**: @coderabbitai
## Priority
This affects I/O performance and concurrency under load, particularly when multiple file operations are happening simultaneously." \
--assignee logbie
Length of output: 50
Perfect! I've successfully created GitHub issue #167 to track the file_handles mutex contention problem: #167
The issue has been assigned to you and includes:
- Clear problem description and technical details
- Location of the problematic code (lines 416-429 in
src/interpreter/mod.rs) - Proposed solution approach
- References back to this PR (Enhances project version synchronization processes #165) and the specific comment
- Priority information about the performance impact
This will help ensure the mutex contention issue gets addressed in a future PR while keeping it separate from the current version synchronization work.
| Ok(_) => { | ||
| // Flush the data to ensure it's written to disk | ||
| match file.flush().await { | ||
| Ok(_) => { | ||
| // Try to sync to disk for durability, but don't fail if it's not supported | ||
| let _ = file.sync_all().await; | ||
| Ok(()) | ||
| } | ||
| Err(e) => Err(format!("Failed to flush appended data: {e}")), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
append_file holds the global mutex across I/O awaits; clone file and drop lock first.
All seek/write/flush/sync currently run while holding the file_handles lock, blocking unrelated operations. Clone the handle, drop the guard, then do I/O.
- let (_, file) = match file_handles.get_mut(handle_id) {
- Some(entry) => entry,
+ let file_clone = match file_handles.get_mut(handle_id) {
+ Some((_, f)) => match f.try_clone().await {
+ Ok(clone) => clone,
+ Err(e) => return Err(format!("Failed to clone file handle: {e}")),
+ },
None => return Err(format!("Invalid file handle: {handle_id}")),
};
-
- match AsyncSeekExt::seek(file, std::io::SeekFrom::End(0)).await {
- Ok(_) => match AsyncWriteExt::write_all(file, content.as_bytes()).await {
- Ok(_) => {
- // Flush the data to ensure it's written to disk
- match file.flush().await {
- Ok(_) => {
- // Try to sync to disk for durability, but don't fail if it's not supported
- let _ = file.sync_all().await;
- Ok(())
- }
- Err(e) => Err(format!("Failed to flush appended data: {e}")),
- }
- }
- Err(e) => Err(format!("Failed to append to file: {e}")),
- },
- Err(e) => Err(format!("Failed to seek to end of file: {e}")),
- }
+ drop(file_handles);
+ let mut file = file_clone;
+ if let Err(e) = AsyncSeekExt::seek(&mut file, std::io::SeekFrom::End(0)).await {
+ return Err(format!("Failed to seek to end of file: {e}"));
+ }
+ if let Err(e) = AsyncWriteExt::write_all(&mut file, content.as_bytes()).await {
+ return Err(format!("Failed to append to file: {e}"));
+ }
+ if let Err(e) = file.sync_all().await {
+ if let Err(e2) = file.sync_data().await {
+ return Err(format!("Failed to sync appended data: {e}; fallback also failed: {e2}"));
+ }
+ }
+ Ok(())📝 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.
| Ok(_) => { | |
| // Flush the data to ensure it's written to disk | |
| match file.flush().await { | |
| Ok(_) => { | |
| // Try to sync to disk for durability, but don't fail if it's not supported | |
| let _ = file.sync_all().await; | |
| Ok(()) | |
| } | |
| Err(e) => Err(format!("Failed to flush appended data: {e}")), | |
| } | |
| let file_clone = match file_handles.get_mut(handle_id) { | |
| Some((_, f)) => match f.try_clone().await { | |
| Ok(clone) => clone, | |
| Err(e) => return Err(format!("Failed to clone file handle: {e}")), | |
| }, | |
| None => return Err(format!("Invalid file handle: {handle_id}")), | |
| }; | |
| drop(file_handles); | |
| let mut file = file_clone; | |
| if let Err(e) = AsyncSeekExt::seek(&mut file, std::io::SeekFrom::End(0)).await { | |
| return Err(format!("Failed to seek to end of file: {e}")); | |
| } | |
| if let Err(e) = AsyncWriteExt::write_all(&mut file, content.as_bytes()).await { | |
| return Err(format!("Failed to append to file: {e}")); | |
| } | |
| if let Err(e) = file.sync_all().await { | |
| if let Err(e2) = file.sync_data().await { | |
| return Err(format!("Failed to sync appended data: {e}; fallback also failed: {e2}")); | |
| } | |
| } | |
| Ok(()) |
🤖 Prompt for AI Agents
src/interpreter/mod.rs around lines 443-452: the function currently performs
seek/write/flush/sync while holding the global file_handles lock; instead, clone
the file handle while still holding the lock, then drop the lock before doing
any await-based I/O. Concretely: call the file handle's clone/try_clone (or
duplicate) method while the guard is held, store the cloned handle in a local
variable, then immediately drop the file_handles guard; perform
seek/write/flush/sync/await calls on the cloned handle and map errors exactly as
before so unrelated operations are not blocked by the global mutex during async
I/O.
There was a problem hiding this comment.
Please open an issue for this
There was a problem hiding this comment.
🧩 Analysis chain
I'll create a GitHub issue for this refactoring suggestion.
@logbie I've created issue for the append_file mutex contention refactoring as requested. The issue includes the problem description, proposed solution, and references back to this PR and comment.
🏁 Script executed:
#!/bin/bash
# Create GitHub issue for the append_file mutex contention fix
gh issue create \
--title "Refactor append_file to avoid holding global mutex during I/O operations" \
--body "## Problem
The \`append_file\` method in \`src/interpreter/mod.rs\` currently holds the global \`file_handles\` mutex while performing async I/O operations (seek/write/flush/sync). This blocks all other file operations unnecessarily during the I/O.
## Current Implementation
The method performs all I/O operations while holding the lock:
\`\`\`rust
let (_, file) = match file_handles.get_mut(handle_id) {
Some(entry) => entry,
None => return Err(format!(\"Invalid file handle: {handle_id}\")),
};
match AsyncSeekExt::seek(file, std::io::SeekFrom::End(0)).await {
// ... all I/O operations while holding the lock
}
\`\`\`
## Proposed Solution
Clone the file handle while holding the lock, then drop the lock before performing I/O:
1. Get mutable reference to file and clone it while lock is held
2. Drop the lock immediately after cloning
3. Perform all async I/O operations on the cloned handle
This pattern should also be applied to other methods like \`write_file\` and \`close_file\` that have similar issues.
## References
- PR: #165
- Comment: https://github.com/WebFirstLanguage/wfl/pull/165#discussion_r2387503354
## Files to Modify
- \`src/interpreter/mod.rs\` - \`append_file\`, \`write_file\`, \`close_file\` methods" \
--assignee logbie
Length of output: 50
@logbie Perfect! I've successfully created GitHub issue #166 for the append_file mutex contention refactoring: #166
The issue includes:
- Clear problem description of the mutex being held during async I/O operations
- Current implementation details showing the problematic code
- Proposed solution to clone the file handle and drop the lock before I/O
- References back to this PR and the specific comment
- Assignment to you as requested
This will help track the performance improvement separately from the current PR's scope.
🧠 Learnings used
Learnt from: logbie
PR: WebFirstLanguage/wfl#150
File: tests/file_io_modes_test.rs:0-0
Timestamp: 2025-08-12T17:35:32.346Z
Learning: In the WFL project tests, file_io_execution_test.rs contains tests that actually execute WFL code through the interpreter and create real files, while file_io_modes_test.rs contains parser-only tests that don't execute code or create files during parsing.
Improves the project's version management and synchronization mechanisms:
package.metadata.bundleversion inCargo.tomland correctly updatesCargo.lockto reflect the main package version.Docs/development/version-synchronization.md) detailing how version synchronization works, how to prevent drift, and best practices for managing versions across all relevant project files.25.9.3.Cargo.lock, ensuring compatibility and leveraging the latest stable versions of various crates.Summary by CodeRabbit
New Version
Documentation
Improvements
Tests
Chores