chore: standardize .version contract with unified parser/writer (lega…#349
chore: standardize .version contract with unified parser/writer (lega…#349eva57gr wants to merge 3 commits intoLight-Heart-Labs:mainfrom
Conversation
Lightheartdevs
left a comment
There was a problem hiding this comment.
Review: Standardize .version contract with unified parser/writer
Good intent — centralizing version parsing is the right move. A few things need addressing before this merges.
Blocking
-
python3 hard dependency without fallback.
dream-update.shandmigrate-config.shnowexit 1ifpython3is missing. Previously these only neededjq+bash. Adding a mandatory runtime dep that wasn't required before needs either ajq-only fallback path or a clear migration note. On minimal Docker images or fresh installs this could break the update path. -
No tests for the new shared library.
version-file.shis now a critical contract — it parses both legacy plain-text and JSON formats, handles missing/empty/malformed files, and does semver comparison. The existing test infrastructure (tests/contracts/) should cover this. At minimum: round-trip write/read, legacy format parsing, malformed JSON fallback, semver comparison edge cases. -
Version source divergence with PR #350. After both merge, version lives in three places with no single source of truth:
- PR #349: reads/writes
.versionfile (JSON/plain-text) - PR #350: reads
DREAM_VERSIONfrom.env, falls back tomanifest.json
dream-update.sh checkanddream update(from dream-cli) would report different installed versions if.versionand.envdrift apart. Coordinate with #350 on which is canonical before merging either. - PR #349: reads/writes
Non-blocking
- Silent Python failures. If the heredoc Python script fails (permission error, etc.), bash functions silently return empty strings. Under
set -euo pipefail, empty version strings fed into comparisons could cause unexpected behavior. Surface errors:|| { log_error "version parse failed"; return 1; }. || trueaftershiftinversion_file_upsert_fields— technically violates CLAUDE.md conventions. Use a guard instead.- Pre-release suffix handling (
2.0.0-rc1→2.0.0, suffix silently dropped) is reasonable but undocumented. cmd_checkexit code change (exit 2for update available) is a behavioral change for any external automation. The help text documents it, but worth a note in PR description.
The version-file abstraction is solid architecture. Just needs tests, the dependency story sorted, and coordination with #350.
Lightheartdevs
left a comment
There was a problem hiding this comment.
Review — Good standardization, coordinates with #350 and #358.
What it does
Introduces scripts/version-file.sh as a unified parser/writer for the .version file, handling both legacy plain-text and JSON formats. Refactors dream-update.sh to use these helpers instead of inline jq calls.
Changes
- New
version-file.shwithversion_file_get_current(),version_file_upsert_fields(),version_file_migrate()— handles legacy text → JSON migration dream-update.shrefactored to use the new helperscmd_checkgets--jsonmode for structured output- Semver comparison refactored to use
_semver_triplet()helper emit_check_json()for structured check output
Coordination
- #350 (@buddy0323) adds
_check_version_compat()to dream-cli which reads.version— should use this PR's parser - #358 (merged) also writes to
.versionviajq— would benefit from using this PR'sversion_file_upsert_fields() - These three PRs form a cohesive "version management" story
Concern
Adds python3 as a hard dependency for dream-update.sh — was previously optional. Justified since the JSON parsing is more robust, but should be documented.
Has CHANGES_REQUESTED from prior reviewer. Address feedback, then coordinate merge order with #350.
|
Closing per maintainer request. The work here is appreciated — please reopen or create a fresh PR if you'd like to continue this effort. |
…cy + JSON)