feat: Real platform detection + unified version system#426
Conversation
…system - Fix headless initenvironment() to detect actual platform via compile-time checks (#ifdef __APPLE__, __linux__, _WIN32) instead of setting all flags to false. On macOS, queries real OS version via sw_vers. - Add three new system.environment flags: - isHeadless (true in CLI builds) - isPosix (true on macOS and Linux) - isLinux (true on Linux only) - Unify version system: Frontier.version() now returns product version derived from git tags (CLI major + 10), e.g. v1.0.0-alpha.6 → "11.0a6". This preserves backward compat with date.versionLessThan() checks against legacy 7.x/10.x versions. - Add Frontier.cliVersion() kernel verb returning the git-derived CLI distribution version string. - Add tools/derive_product_version.sh for Makefile version derivation. - Integration tests for all new environment flags and version verbs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0623d034b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| major=$(echo "$v" | sed -E 's/^([0-9]+).*/\1/') | ||
| rest=$(echo "$v" | sed -E 's/^[0-9]+\.//') | ||
| pmajor=$((major + 10)) |
There was a problem hiding this comment.
The derivation script assumes the input starts with digits, but frontier-cli/Makefile passes git describe --tags --always --dirty, and git describe -h documents that --always will "show abbreviated commit object as fallback" when no tag is available. In that case (for example shallow clones without tags), major becomes a hash like db0949d, pmajor=$((major + 10)) throws Illegal number, and FRONTIER_PRODUCT_VERSION_STRING ends up empty, so frontier.version() can return an empty string in non-tag builds.
Useful? React with 👍 / 👎.
| esac | ||
| fi | ||
|
|
||
| if [ "$patch" = "0" ] || [ "$patch" = "$minor" ]; then |
There was a problem hiding this comment.
The condition if [ "$patch" = "0" ] || [ "$patch" = "$minor" ] incorrectly drops the patch component whenever minor and patch are numerically equal, so valid tags like v1.2.2 are converted to 11.2 instead of 11.2.2. This silently produces wrong product versions and can break ordering/compatibility checks that rely on full semantic version precision.
Useful? React with 👍 / 👎.
- derive_product_version.sh: Add input validation for bare commit hashes and non-tag git describe output (falls back to "11.0d" safely) - derive_product_version.sh: Fix patch comparison — only omit patch when genuinely zero, not when minor == patch (v1.2.2 → 11.2.2 not 11.2) - langstartup.c: Add isHeadless/isPosix/isLinux to non-headless initenvironment() so UserTalk scripts work portably across runtimes - headless_frontier_verbs.c: Dedup version logic — frov_version case now calls frontierversion() instead of duplicating the #ifdef logic - Add osBuildNumber integration test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review: PR #426 - Real Platform Detection + Unified Version SystemI've reviewed this PR and have the following feedback: ✅ Strengths
🔍 Issues & Concerns1. Memory Management - Potential Handle Leak (langstartup.c:299-323)CRITICAL: The code creates handles but doesn't properly handle error cases. If unixshellcall() or any intermediate operation fails, execution may exit early without calling disposehandle(), causing a memory leak. Recommendation:
2. Error Handling - Silent FailuresThe code doesn't check if unixshellcall() succeeds. If sw_vers fails (non-macOS system, sandboxing issue, missing binary), the code continues silently and could result in garbage data or empty strings in system.environment fields. Recommendation:
3. Code DuplicationThe handle allocation and sw_vers calling pattern is repeated twice (lines 299-306 and 311-318). Recommendation: Extract to a helper function for better maintainability. 4. Shell Script - POSIX Compliance Issues (derive_product_version.sh)The script uses sed -E which is not POSIX-compliant (GNU/BSD extension). May fail on minimal Unix systems or busybox environments. Recommendation:
5. Shell Script - Edge Case HandlingThe regex validation may have edge cases (e.g., v0.9.0 would give major=0, product version 10.0). The fallback "11.0d" may mask valid but unexpected version formats. Recommendation:
🎯 RecommendationsPriority 1 (Must Fix)
Priority 2 (Should Fix)
Priority 3 (Nice to Have)
📋 Testing RecommendationsThe test coverage is excellent, but consider adding:
✅ Architectural ComplianceChecked against project docs:
🎬 VerdictConditionally Approve pending fixes for:
The core implementation is solid and well-tested. The issues identified are fixable and primarily concern robustness and error handling rather than fundamental design problems. Great work on the comprehensive test coverage and clear documentation! 🚀 |
- langstartup.c: Add proper error handling for sw_vers handle allocation and unixshellcall() failures. Handles are always disposed on all paths. Falls back to "unknown" on failure instead of garbage/empty strings. - langstartup.c: Add structured logging (LOG_COMP_STARTUP) for sw_vers results and failures per docs/LOGGING_STANDARDS.md. - derive_product_version.sh: Change shebang to #!/bin/bash since sed -E is not POSIX sh compliant. Document edge cases and v0.x collision note. Use bash [[ ]] for input validation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review: PR #426 - Real platform detection + unified version systemThis PR introduces real platform detection for headless builds and unifies the version system. The implementation is generally solid with good test coverage. Below are my findings: ✅ Strengths
🐛 Critical Issues1. Memory Leak in Handle Management (langstartup.c:286-364)Severity: HIGH - Memory leak on error paths Location: Issue: The handle allocation pattern has multiple error paths that don't properly clean up both handles. // Lines 286-298
Handle hcommand = nil, hreturn = nil;
boolean flgothandles;
flgothandles = newemptyhandle (&hreturn) && newtexthandle ("\psw_vers -buildVersion", &hcommand);
if (!flgothandles) {
log_warn (LOG_COMP_STARTUP, "initenvironment: failed to allocate handles for sw_vers");
if (hreturn != nil) disposehandle (hreturn);
if (hcommand != nil) disposehandle (hcommand);
// ... continues
}Problem: If Recommended Fix: Use explicit pattern from Handle hcommand = nil, hreturn = nil;
if (!newemptyhandle(&hreturn)) {
log_warn(LOG_COMP_STARTUP, "initenvironment: failed to allocate hreturn");
copyctopstring("unknown", bs);
langassignstringvalue(ht, str_osBuildNumber, bs);
langassignstringvalue(ht, str_osFullNameForDisplay, bs);
goto cleanup_done;
}
if (!newtexthandle("\psw_vers -buildVersion", &hcommand)) {
log_warn(LOG_COMP_STARTUP, "initenvironment: failed to allocate hcommand");
disposehandle(hreturn);
copyctopstring("unknown", bs);
langassignstringvalue(ht, str_osBuildNumber, bs);
langassignstringvalue(ht, str_osFullNameForDisplay, bs);
goto cleanup_done;
}
// ... use handles ...
disposehandle(hcommand);
disposehandle(hreturn);
cleanup_done:
// continue with rest of functionThis ensures that only successfully allocated handles are disposed. 2. Shell Command Injection Risk (langstartup.c:318-326)Severity: MEDIUM - Potential security issue Location: Manual handle content manipulation for second shell command sethandlesize (hreturn, 0);
copystring ("\psw_vers -productName", bs);
sethandlecontents (stringbaseaddress (bs), stringlength (bs), hcommand);
if (unixshellcall (hcommand, hreturn)) {Issue: While safe in this specific case (hardcoded string), the pattern of manually manipulating handle contents for shell commands is error-prone and deviates from established patterns. Recommendation: Use disposehandle(hcommand);
if (!newtexthandle("\psw_vers -productName", &hcommand)) {
log_warn(LOG_COMP_STARTUP, "initenvironment: sw_vers -productName allocation failed");
disposehandle(hreturn);
copyctopstring("unknown", bsos);
langassignstringvalue(ht, str_osFullNameForDisplay, bsos);
goto cleanup_done;
}
|
- Rewrite sw_vers block with fully sequential allocation (no shared &&) - Fresh newtexthandle for second command instead of sethandlecontents reuse - goto swvers_fallback/swvers_done for consolidated error paths - Use log_debug for graceful fallbacks (not log_warn) - Guard empty $patch in version script (handles "v1.0" edge case) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: Real platform detection + unified version systemOverall AssessmentThis is a well-executed PR that addresses important technical debt by replacing placeholder values with real platform detection. The implementation is clean, well-tested, and maintains backward compatibility. I have a few suggestions for improvement around error handling and code robustness. Code Quality ✅Strengths
Issues & Suggestions🔴 Critical: Resource Leak in Error PathFile: if (!newtexthandle ("\psw_vers -buildVersion", &hcommand)) {
log_debug (LOG_COMP_STARTUP, "initenvironment: hcommand alloc failed, using fallback");
disposehandle (hreturn); // ❌ hreturn disposed here
goto swvers_fallback;
}After this error, execution jumps to if (!newtexthandle ("\psw_vers -productName", &hcommand)) {
log_debug (LOG_COMP_STARTUP, "initenvironment: hcommand alloc failed for productName");
disposehandle (hreturn); // ❌ DOUBLE DISPOSE - hreturn was already disposed above
copyctopstring ("unknown", bsos);
langassignstringvalue (ht, str_osFullNameForDisplay, bsos);
goto swvers_done;
}Issue: If the first Recommendation: Set if (!newtexthandle ("\psw_vers -buildVersion", &hcommand)) {
log_debug (LOG_COMP_STARTUP, "initenvironment: hcommand alloc failed, using fallback");
if (hreturn) disposehandle (hreturn);
hreturn = nil;
goto swvers_fallback;
}
// Later...
if (!newtexthandle ("\psw_vers -productName", &hcommand)) {
log_debug (LOG_COMP_STARTUP, "initenvironment: hcommand alloc failed for productName");
if (hreturn) disposehandle (hreturn);
hreturn = nil;
// ...
}🟡 Medium: Shell Script PortabilityFile: # Uses sed -E (extended regex) which requires bash; not POSIX sh.The script correctly documents that it requires bash, but the shebang Recommendation: Consider using 🟡 Medium: Version Parsing RobustnessFile: The version parsing uses Recommendation: Add validation: if (stringlength(bsmajor) > 0) {
stringtonumber (bsmajor, &x);
langassignlongvalue (ht, str_osMajorVersion, x);
} else {
langassignlongvalue (ht, str_osMajorVersion, 0);
}🟡 Medium: Missing Error CheckFile: sethandlesize (hreturn, 0);The return value of Recommendation: Check return value or add a comment explaining why failure is acceptable here. Best Practices ✅
Test Coverage ✅The test coverage is excellent:
Minor suggestion: Consider adding a unit test for ./tools/derive_product_version.sh "v1.0.0-alpha.6" # Should output: 11.0a6
./tools/derive_product_version.sh "db0949d" # Should output: 11.0d
./tools/derive_product_version.sh "" # Should output: 11.0dPerformance Considerations ✅The Security Concerns ✅
Documentation ✅The PR description is excellent:
Suggestion: Consider adding a note to SummaryThis is a high-quality PR that properly addresses platform detection. The main issues are:
After addressing the double-dispose issue, this PR should be safe to merge. The backward compatibility handling and test coverage are exemplary. Recommendation: ✅ Approve after fixing the double-dispose issue Additional Questions
|
…ity (review round 4) - Nil out hreturn after dispose to prevent potential double-dispose - Validate nthfield() results before stringtonumber() (empty string guard) - Document sethandlesize(0) safety (shrinking to zero never fails) - Use #!/usr/bin/env bash for portability across systems Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review - PR #426: Real platform detection + unified version systemOverall AssessmentThis is a solid enhancement that addresses real deficiencies in the headless mode's environment detection. The implementation is well-structured, thoroughly tested (26 integration tests covering the new functionality), and includes good backward compatibility considerations. However, there are a few resource management concerns and minor improvement opportunities. Critical Issues1. Resource Leak in Error Path 🔴Location: When if (!newtexthandle ("\psw_vers -buildVersion", &hcommand)) {
log_debug (LOG_COMP_STARTUP, "initenvironment: hcommand alloc failed, using fallback");
disposehandle (hreturn);
hreturn = nil;
goto swvers_fallback;
}If Recommendation:
2. Missing Error Recovery for
|
| Category | Count |
|---|---|
| 🔴 Critical Issues | 1 |
| 2 | |
| 🟡 Minor Issues | 1 |
| 📋 Suggestions | 6 |
| ✅ Strengths | 5 |
Recommendation: Request changes to address the resource leak concern (#1). The test formatting (#4) should also be fixed for consistency. Other items are suggestions for improvement but not blockers.
Strengths: Well-tested, thoughtfully designed, good backward compatibility, strong documentation.
Generated by code review bot • Following Frontier development guidelines
Merge RationaleMerged after 4 rounds of review feedback, all addressed: Round 1 (Codex): Fixed bare commit hash crash in version script + patch==minor comparison bug. Round 2 (Claude + local bar-raiser): Added isHeadless/isPosix/isLinux to non-headless path, deduped frontierversion() logic, added error checking on handle allocation/unixshellcall, added osBuildNumber test. Round 3 (Claude): Rewrote sw_vers block with fully sequential handle allocation (no shared Round 4 (Claude): Nil out Remaining review suggestions not addressed (non-blocking):
All 27 integration tests passing (15 frontier_verbs + 12 system_environment). CI green. |
1 participant
Summary
initenvironment()to detect actual platform via#ifdef __APPLE__/__linux__/_WIN32instead of setting all flags to false. On macOS, queries real OS version viasw_vers.system.environmentflags:isHeadless,isPosix,isLinuxFrontier.version()returns product version derived from git tags (CLI major + 10), preserving backward compat withdate.versionLessThan()checks against legacy 7.x/10.x versionsFrontier.cliVersion()kernel verb returning the git-derived CLI distribution versiontools/derive_product_version.shfor Makefile version derivationVersion mapping examples
Frontier.version()Frontier.cliVersion()v1.0.0-alpha.611.0a6v1.0.0-alpha.6v1.0.0-beta.311.0b3v1.0.0-beta.3v1.2.311.2.3v1.2.3system.environment changes (macOS)
isMacfalsetrueisCarbonfalsetrueisHeadlesstrueisPosixtrueisLinuxfalseosMajorVersion015osVersionString0.0.015.6.1osFullNameForDisplay0.0.0macOSTest plan
Frontier.version()returns11.0a6(satisfiesdate.versionLessThanchecks)Frontier.cliVersion()returns full git tag stringsystem.environment.isMac= true,.isHeadless= true,.isPosix= true🤖 Generated with Claude Code