feat: Modernize startup flow for headless/CLI compatibility

[jsavin]

Summary

• script.newScriptObject and op.newOutlineObject: Added CRLF/LF → CR line-ending normalization so .ut files with any line ending format can be imported via the protocol layer
• webBrowser.openURL: Three-mode support for GUI (legacy AppleEvent), CLI (macOS open command via try), and headless (fallback msg() logging URL to console)
• startupScript: Wrapped browser-open and trialVersionCheck calls in try blocks for headless safety (.ut reference file only — ODB change deferred, see note below)

ODB Changes Applied

The newScriptObject, newOutlineObject, and openURL changes were applied to the live Frontier.root database via the protocol layer and verified:

• All three scripts compile successfully
• webBrowser.openURL returns true in CLI mode (uses open command)
• LF and CRLF line endings both handled correctly by updated newScriptObject

ODB Change Deferred

The startupScript try-block wrap is in the .ut reference file only. Cannot modify script outlines in headless mode (op verbs require the window system). Not a startup blocker since openURL itself now handles errors gracefully.

Verification

• Fresh make dist → full startup completes (startingUp=false)
• finishInstall() runs on first launch, installs mainResponder + manila
• Both HTTP listeners start (port 80 + 5336)
• http://127.0.0.1:5336/setupFrontier returns HTTP 200 with setup form
• Page verified via agent-browser: all form fields (Name, Server domain, Email, SMTP, Organization, Admin password) render correctly with Submit button
• 302 unit tests passed, 1846 integration tests passed, 0 failed

Test plan

• Fresh dist build and full startup
• setupFrontier page loads and renders form
• Unit test suite (302/302 pass)
• Integration test suite (1846/1846 pass)
• agent-browser screenshot verification

🤖 Generated with Claude Code

[claude]

Code Review

Overall this is a well-structured PR that cleanly solves the headless/CLI startup problem. The test results (302 unit + 1846 integration all passing) and the end-to-end startup verification are solid. A few things worth addressing before merge.

Security Concern (Medium) — Command Injection in openURL.ut

The URL string s is interpolated directly into shell commands without sanitization:

sys.unixShellCommand ("open \"" + s + "\"");
sys.unixShellCommand ("xdg-open \"" + s + "\"");

A URL containing shell metacharacters (backticks, semicolons, $(), etc.) could execute arbitrary commands. Even though webBrowser.openURL is typically called with trusted installer URLs during startup, the function is general-purpose and callable from UserTalk scripts with user-supplied input.

Recommendation: Validate that s is a well-formed URL before passing to the shell. A simple guard like if not (string.lower (s) beginsWith "http") would reject unexpected input and close the obvious injection vector without changing normal-path behavior.

Minor: Inconsistent Trailing Newline Fix

newOutlineObject.ut and newScriptObject.ut correctly gained trailing newlines. startupScript.ut still lacks one. Minor inconsistency — not blocking.

Minor: Removed Inline Comment in newScriptObject.ut

The original had:

try {script.compile (adr)}; //PBS 04/09/00: try to compile the script

The trailing comment explained why the try is there (suppress compile-changes dialogs). The info is in the header block, but the inline callout was a useful at-a-glance reminder. Low priority.

Good Catches

• Correct CRLF normalization order: Replacing CRLF before LF is the right sequence — avoids double-conversion where CRLF would become CR+CR if LF were handled first.
• Graceful fallback chain in openURL: macOS open -> Linux xdg-open -> msg() fallback is clean. Try-wrapping means a missing binary does not abort startup.
• Deferred ODB change on startupScript with explanation is the right call given op verb constraints in headless mode.

---

The command injection issue is the only blocking concern. The rest is polish. Overall solid work.

[claude]

---

Follow-up Review Notes

The existing review flagged command injection in openURL.ut but missed the string/isValidUrl.ut file added in this same PR. With that context, here are more precise findings:

Security: isValidUrl Does Not Prevent Shell Injection

The new string.isValidUrl only validates the scheme prefix (e.g. https://). A URL containing a double-quote followed by shell metacharacters passes isValidUrl but injects into the shell when the string is embedded in the open command via double-quote interpolation.

Fix: After the isValidUrl guard, also verify that s contains no double-quote characters before passing to sys.unixShellCommand. Or restructure to call open or xdg-open via a method that does not require shell interpolation.

Bug: xdg-open Returns Shell Output, Not Boolean

The macOS path calls sys.unixShellCommand and then explicitly return (true) on success. The Linux path returns the result of sys.unixShellCommand directly. That function returns the command stdout string, not a boolean. On success, xdg-open produces no output, so this returns an empty string (falsy). The Linux path will appear to fail even when xdg-open succeeds. It should mirror the macOS pattern: call the command inside try, then return (true).

Minor: file:// Whitelisted in isValidUrl

The file:// scheme is allowed. In CLI/headless mode, this means open or xdg-open could be invoked with local filesystem paths. Probably acceptable for legitimate Frontier use, but worth a conscious decision since this function is being used as a security gate.

---

The xdg-open return value is a clear bug worth fixing before merge. The injection gap is real but low-risk at the current call sites (trusted installer URLs). Both are small fixes.

[claude]

Code Review: PR #486 - Modernize startup flow for headless/CLI compatibility

Overall: The approach is sound and addresses a real need. Line-ending normalization is correct, and the three-mode openURL design is sensible. Two issues need resolution before merge; the rest are minor.

---

Critical: Shell injection in openURL.ut

The URL is passed directly into a shell string via sys.unixShellCommand with the URL interpolated inside double quotes. The string.isValidUrl function checks the scheme prefix and rejects literal double-quote characters, but does NOT reject shell metacharacters that remain active inside double-quoted strings: backticks, dollar-sign subshell syntax (like $(cmd)), backslashes, and newlines. A URL containing subshell syntax would pass isValidUrl but execute when passed to sys.unixShellCommand.

Fix options (in order of preference):

1. Use a shell-escaping routine before interpolation.
2. Validate s against an allowlist regex rather than just checking the scheme.
3. At minimum, add these to the rejection list in isValidUrl: backtick, dollar sign, semicolon, pipe, ampersand, newlines.

---

Bug: Linux xdg-open path returns wrong value on success

The macOS path explicitly calls return true inside the try block after calling the open command. The Linux path uses the same structure but is missing the explicit return true -- execution falls through to msg() and return false, so a successful xdg-open call incorrectly reports failure to the caller. Should mirror the macOS pattern with an explicit return true inside the try block.

---

Minor: file:// scheme in isValidUrl

file:// is whitelisted. In CLI/headless mode this allows open to be called with local filesystem paths, which can launch local applications. Current call sites use trusted installer URLs so risk is low, but since isValidUrl is now a general-purpose utility in system.verbs.builtins.string, future callers may not share that context. Consider removing file:// from the allowlist or documenting the assumption.

---

Nit: Comment indentation in startupScript.ut

The new comment block inside the bundle is indented one level deeper than the if statement it precedes, which makes it visually appear to belong to the previous line. Minor style issue only.

---

What is good

• CRLF/LF to CR normalization: Two-pass approach (CRLF first, then LF alone) is the correct order to avoid double-conversion. Applied consistently to both newScriptObject and newOutlineObject.
• ODB deferral note: Clear documentation of why startupScript is not updated in the ODB and why that is acceptable.
• Test coverage: 302 unit + 1846 integration tests all passing. Verification checklist is thorough.
• Missing newlines at EOF: Fixed in all modified files.

---

Required fixes before merge:

1. Shell injection gap in openURL.ut -- add metacharacter filtering or proper shell escaping
2. Linux xdg-open path returns false on success -- add explicit return true inside the try block

The Linux return-value bug is a clear behavioral defect. The injection issue is low-risk at current call sites but should be addressed since isValidUrl is now a general-purpose utility.

[claude]

Code Review: PR 486 - Modernize startup flow for headless/CLI compatibility

Note on prior reviews: The three existing reviews flag command injection via sys.unixShellCommand. That does not apply to the current diff. The implementation uses the new sys.openUrl kernel verb via fork/execlp, passing the URL as a direct argument to open/xdg-open without any shell interpretation. The injection concern is genuinely addressed at the C level. This review covers the current implementation.

Bug: Zombie Processes from Unreaped Children (Medium severity)

In shellsysverbs.c the parent forks and returns without calling waitpid. When the child exits it becomes a zombie until the parent reaps it. In a long-running Frontier server that opens many URLs these accumulate indefinitely.

The standard fix is a double-fork. The first child forks again then exits immediately. The grandchild is reparented to init/launchd and reaped automatically. Example:

pid_t pid = fork();
if (pid == 0) {
    pid_t pid2 = fork();
    if (pid2 == 0) {
        execlp("open", "open", url, NULL);
        _exit(1);
    }
    _exit(0);
}
waitpid(pid, NULL, 0);

Alternatively, install a SIGCHLD handler with SA_NOCLDWAIT at startup to auto-reap all children.

Security: file scheme allowed in isValidUrl

string/isValidUrl.ut whitelists the file scheme alongside http, https, ftp, and ftps. In CLI/headless mode this means sys.openUrl can be invoked with local filesystem paths. Since isValidUrl is explicitly described as a security gate that rejects dangerous schemes, the file scheme inclusion deserves a conscious decision documented in a comment.

Missing: Integration Tests for New Verbs

Neither sys.openUrl nor string.isValidUrl has an integration test. For string.isValidUrl a YAML test suite is straightforward with inputs like empty string, missing scheme, javascript scheme, file scheme, and valid https URL. This would catch regressions if validation logic changes later.

Minor: Validation Only Applied in Headless Mode

webBrowser/openURL.ut validates the URL only when system.environment.isHeadless is true. Callers that reach sys.openUrl directly, bypassing webBrowser.openURL, get no validation. Consider adding a guard inside sys/openUrl.ut for defense in depth.

Minor: Case Inconsistency

The kernel verb is registered as openUrl (lowercase L) in both resource files. The existing webBrowser.openURL uses uppercase L. No behavioral issue, but worth noting for API consistency.

What is Working Well

The fork/execlp approach correctly addresses shell injection at the system call level. Args are passed directly to the executable with no shell involved. This is the right fix and resolves the concern from prior reviews, which were examining an older implementation using unixShellCommand.

The CRLF-to-CR normalization order is correct. Replacing CRLF before bare LF avoids the double-conversion bug.

The three-mode openURL from GUI to CLI to headless fallback with msg() is clean and self-documenting.

The try-block wrapping in startupScript is the right approach for non-blocking headless startup.

The ODB deferral is clearly documented with explanation of why startupScript is not updated in ODB and why that is safe given op verb constraints in headless mode.

Both resource files are updated. Mac and Win32 kernelverbs both updated with the new verb and correct counts.

Required before merge: Zombie process issue in the fork path.
Recommended: Integration tests for string.isValidUrl and a decision plus comment on file scheme in the validator.

Generated with Claude Code

[claude]

Follow-up Review: PR 486 - Current Iteration Assessment

This is a follow-up to the previous review chain. The current iteration has addressed all prior blocking concerns.

Previous Blockers - Resolved:

Shell injection (raised in three prior reviews): The sys.unixShellCommand path is gone. sys.openUrl now calls execlp() directly with the URL as a separate argument, so no shell is involved. This is the correct fix.

xdg-open return value bug: Now moot - the C implementation handles return values directly.

Zombie process issue: The double-fork pattern is implemented correctly. The first child forks a grandchild, exits immediately, and the parent calls waitpid() on the short-lived first child. The grandchild is reparented to init/launchd and reaped automatically. The prior blocking concern is resolved.

Remaining Issues:

Minor - sys.openUrl silently returns true even when execlp fails: If execlp() cannot find open or xdg-open, the grandchild exits via _exit(1), but no one observes that exit code. The parent already returned true. Callers using the return value for error handling will think the URL opened even when it did not. This is a structural limitation of the double-fork approach. Recommend adding a comment noting that true means "launch was attempted" not "launch confirmed."

Minor - string.isValidUrl integration tests all skipped: The string_isvalidurl.yaml file covers good cases (empty string, missing scheme, javascript injection, file scheme, data scheme) but all 12 tests are skip: true. The PR description explains why (verb not yet in source .root), but there is zero CI regression protection today for a security-adjacent function. Recommend a tracking note so these get enabled when the ODB change lands.

Trivial: startupScript.ut still lacks trailing newline. The other two .ut files were fixed in this PR.

What Is Working Well:

• Double-fork zombie prevention is textbook correct
• execlp with separate args eliminates shell injection at the syscall level
• CRLF normalization order in newScriptObject/newOutlineObject is correct (CRLF before LF avoids double-conversion)
• Three-mode webBrowser.openURL (GUI to CLI to headless msg() fallback) is clean
• Both resource files (kernelverbs.r and kernelverbs.rc) updated with correct verb counts
• ODB deferral for startupScript is clearly documented with explanation of the op verb constraint

Verdict: No blocking issues remain. The zombie process concern from the prior review is addressed, shell injection is eliminated at the syscall level, and end-to-end startup verification passes (302 unit + 1846 integration). The execlp return-value behavior is worth a brief code comment, and the skipped tests need a tracking note, but neither is a merge blocker.

Generated with Claude Code

[claude]

Code Review: feat: Modernize startup flow for headless/CLI compatibility

Overall this is a well-scoped and thoughtfully executed PR. The three independent concerns (migration naming, sys.openUrl, startup script hardening) are each handled cleanly, with good separation between the C kernel work and the UserTalk layer. Test coverage is solid -- 302 unit tests + 1846 integration tests passing, plus new integration tests for sys.openUrl. Below are observations ranging from hard issues to low-priority notes.

---

Migration naming change (db_format.c, dbverbs.c, main.c)

Positive: The in-place strategy (rename v6 to .v6.root, write v7 to the original path) is the right long-term design. The crash-recovery logic in ensure_database_v7 (detecting an interrupted migration from the presence of .v6.root) is a nice correctness improvement.

Issue -- existing v7 backup overwritten with only an info-level log (db_format.c):

In migrate_internal, the existing-backup guard has a path where the backup file contains v7 data and proceeds with log_info and overwrites it silently. If a user manually placed a file at the .v6.root path, it is destroyed without warning. log_info is the wrong severity here -- log_warn is more appropriate. Not a merge blocker, but worth a follow-up issue.

Issue -- dbopenverb error message conflates corruption with migration failure (dbverbs.c):

When ensure_database_v7 returns false, the error message says the database "could not be verified as v7 format." This fires even when the underlying failure is environmental (disk full, permission denied during migration). The old code emitted a specific "Database migration failed" message. The new message suggests file corruption when the real problem may be environmental.

Minor -- db_format_derive_v6_backup_path truncation handling is correct but implicit:

When the function returns false, callers rely on backup_path[0] being null as a sentinel to skip the rename. The guard works correctly, but a comment at the call site would help future readers understand why an empty backup_path is a valid and safe no-op.

---

sys.openUrl kernel verb (shellsysverbs.c, kernelverbs.r, kernelverbs.rc)

Positive: The double-fork pattern to avoid zombie processes is exactly right for a long-lived daemon. Using execlp instead of system() or popen() eliminates shell interpolation -- the correct security-conscious approach. Parent-side handle cleanup before the pid < 0 check is correct; the child has already _exit'd by that point.

Potential bug -- handle leak on the unsupported-platform #else path:

In the openurlfunc case, the Apple/Linux path calls unlockhandle(hurl) and disposehandle(hurl) in the parent before returning. The Windows path does the same inside its block. In the #else branch, the code calls copystring(BIGSTRING(...), bserror) and return (false) -- if unlockhandle/disposehandle do not appear before this return, the handle is leaked on any platform that hits the #else path. Please verify by inspection (or on a non-Apple, non-Windows build) that unlock/dispose appear before the #else return. If they do not, add them at the top of the #else block before copystring.

Issue -- sys.openUrl with empty string returns true even when the OS call fails silently:

The integration test checks typeof(sys.openUrl("")), not the return value. Passing an empty string to execlp("open", "open", "", NULL) on macOS will likely fail in the grandchild, but the parent still returns true because the fork itself succeeded. The webBrowser.openURL wrapper correctly validates via string.isValidUrl first. Consider whether the kernel verb itself should check for an empty URL before forking and return false directly for empty input.

Naming note: "openUrl" (camelCase) is registered in both .r and .rc resource files, consistent with the Frontier.root glue convention. Worth a quick sanity-check that verb dispatch correctly maps sys.openUrl to openurlfunc.

---

webBrowser.openURL modernization (openURL.ut)

Positive: The three-mode dispatch (GUI AppleEvent / CLI sys.openUrl / headless msg() fallback) is the right architecture for cross-platform headless compatibility.

Observation -- silent fallback when sys.openUrl fails:

When sys.openUrl fails, the try block swallows the error and falls through to msg(). In a non-interactive headless environment the msg() output may be invisible. A message in the failure branch noting the URL and asking the operator to open it manually would improve observability.

---

startupScript.ut changes

Positive: Wrapping webBrowser.openUrl and userland.trialVersionCheck in try blocks is the minimal-impact approach. The PR correctly notes the ODB change is deferred.

Observation -- window.update remains outside the try block:

window.update ("About Frontier") is called before the try block that wraps webBrowser.openUrl. If window.update throws in headless mode, the try block is never reached. Since the ODB change is deferred, add a note to the follow-up issue to include window.update in the try scope when applying the ODB edit.

---

string.isValidUrl (new UserTalk verb)

Positive: A dedicated URL validation verb is good hygiene. The scheme allowlist (http, https, ftp, ftps, file) is appropriate for Frontier's typical use cases.

Observation: file:// URLs pass validation. For webBrowser.openURL this is fine. If callers ever use string.isValidUrl for security-sensitive filtering, the permissive file:// acceptance could be surprising. Future concern, not a current bug.

---

Search path changes (main.c, INSTALL.md)

The reduction from 8 to 5 search paths is correct. Removing the hardcoded databases/Frontier.root entry in favor of cwd/Frontier.root is the right call -- the old entry assumed the process always ran from the project root. The code order and INSTALL.md documentation are consistent.

---

Distribution / Makefile

The make dist simplification (direct cp instead of migrate+rename) is a clean consequence of the source databases being v7. No issues.

---

Planning document (planning/root7_retirement_plan.md)

Minor: The document header and body reference "PR #487" but this is PR #486. Likely a copy-paste artifact. The doc also preserves code examples using the abandoned .v7.root naming alongside a disclaimer, which is fine for archival purposes but could use more prominent "historical / not implemented" markers.

---

Summary

Severity	Item
Potential bug	sys.openUrl #else (unsupported platform): verify handle is unlocked/disposed before return (false)
Moderate	dbopenverb error message conflates corruption with migration failure
Moderate	Silent overwrite of existing .v6.root containing v7 data logged at info rather than warn
Low	window.update in startupScript remains outside try block; may throw in headless mode
Low	sys.openUrl with empty string forks successfully and returns true even though the OS open call fails silently
Cosmetic	planning/root7_retirement_plan.md references wrong PR number (#487 vs #486)

The handle lifecycle question on the #else path of sys.openUrl is the only item warranting direct verification before merge. The rest are improvements suitable for follow-up issues. The overall approach is sound and the test coverage is thorough.

[claude]

Code Review: PR 486 - Modernize startup flow for headless/CLI compatibility

Overall this is a well-designed PR that completes two distinct workstreams: retiring the .root7 naming convention in favor of in-place migration, and adding a safe sys.openUrl kernel verb. Tests are green (302 unit, 1846 integration), and the crash-recovery logic in ensure_database_v7 is thoughtfully designed.

---

Migration Architecture (db_format.c, dbverbs.c)

Positive: The new in-place migration strategy is cleaner than the Phase 1 .root7 side-car approach. Renaming v6 to .v6.root, then atomically placing v7 at the original path is easier to reason about and eliminates the ambiguity around which file to open.

Positive: The crash-recovery path in ensure_database_v7 correctly handles the window between rename(v6->backup) and rename(temp->final). Reading the backup header once to avoid a redundant fopen is a nice efficiency touch.

Positive: The refuse-to-overwrite-valid-v6-backup guard in migrate_internal is good defensive programming -- it prevents silently destroying an existing v6 backup.

---

Issue: Ignored return value of db_format_derive_v6_backup_path in migrate_internal

File: Common/source/db_format.c, in migrate_internal

db_format_derive_v6_backup_path calls snprintf before its bounds check, so if the path is too long, backup_path can contain a truncated path (not the empty string the caller initialized it to). Subsequent code that checks backup_path[0] != '\0' would then proceed with a corrupted backup path.

The call site in ensure_database_v7 checks the return value correctly. The call in migrate_internal should too -- add a check and goto cleanup on failure. This is an edge case (paths > ~1006 chars), but worth hardening since it is a data-safety path.

---

Minor: Dead code in sys.openUrl (shellsysverbs.c)

After enlargehandle(hurl, 1, "\0") succeeds, the handle always has at least 1 byte. The check GetHandleSize(hurl) == 0 can never be true at that point -- the empty-URL case is already caught by url[0] == '\0'. Safe to remove the GetHandleSize clause to avoid confusion for future readers.

---

sys.openUrl implementation (shellsysverbs.c)

Positive: Using fork/execlp instead of system()/popen() correctly eliminates shell injection risk. The double-fork pattern to avoid zombies is correctly implemented (parent waits only for the short-lived first child; grandchild is reparented to init/launchd).

Positive: All three code paths (POSIX, Windows, unsupported) properly unlock and dispose hurl before returning.

---

drop_cancoon removal (db_format.h, db_format.c)

Positive: Consolidating always-drop-Cancoon as a v7 migration invariant rather than a runtime flag simplifies the mode struct and eliminates a class of potential bugs. Removing migrate_32bit_to_64bit_drop_cancoon() (which was a duplicate of migrate_32bit_to_64bit()) is correct.

---

planning/root7_retirement_plan.md

The plan document acknowledges that its code examples use .v7.root naming while the actual implementation uses .v6.root. The discrepancy is noted, but the stale examples in Categories 1C-1D could mislead future readers. Worth a follow-up pass to update those examples or add a clearer note near each affected section.

The Status line references "PR #487, merged into PR #486" -- since this is PR #486, that attribution is circular. Consider clarifying.

---

.root7 fallback TODO

In ensure_database_v7 there is a TODO dated 2026-03-22 to remove the .root7 fallback, but no milestone or version is attached. Worth linking to a tracking issue so this does not drift indefinitely.

---

Summary

Area	Assessment
In-place migration design	Well-designed, correct
Crash recovery	Correct, well-documented
sys.openUrl implementation	Secure, correct double-fork
drop_cancoon removal	Clean simplification
db_format_derive_v6_backup_path return ignored	Medium -- fix before merge
Dead GetHandleSize check	Minor / cleanup
Planning doc stale examples	Minor / cosmetic

The migration architecture is sound. The one item worth addressing before merge is the ignored return value in migrate_internal -- it is a data-safety path and deserves the same defensive handling as the ensure_database_v7 call site.

---

Reviewed with Claude Code

[claude]

Code Review — PR #486: Modernize startup flow for headless/CLI compatibility

Overall this is a well-executed PR. The .root7 retirement is clearly thought through, crash recovery is solid, and sys.openUrl uses the right double-fork pattern. A few items worth discussing before merge.

---

Issues

1. migrate_internal: Backup overwrite check uses raw versionnumber — latent endianness risk

tydatabaserecord existing_hdr;
boolean hdr_ok = fread(&existing_hdr, sizeof existing_hdr, 1, fp_existing) == 1;
...
if (hdr_ok && existing_hdr.versionnumber < 7) {
    // refuse to overwrite v6 backup
}

This new guard is on the hot path for refusing to overwrite a valid v6 backup — a safety-critical decision. The versionnumber comparison without byte-swapping is consistent with existing code in ensure_database_v7, so it will work correctly. But because this is a new safety gate (not just a detection hint), it's worth adding a one-line comment:

/* versionnumber is stored LE in the file header (same assumption as ensure_database_v7) */

If the byte-order assumption is ever wrong, the guard silently falls through to "safe to overwrite" instead of protecting the backup.

2. sys.openUrl: true returned even when the grandchild fork() fails

With the double-fork pattern, if fork() in the first child fails (pid2 < 0), the first child calls _exit(0) and the parent receives true. UserTalk gets true but the browser never launched. This is an inherent fire-and-forget tradeoff, but it's worth documenting explicitly in the verb's help text and/or the UserTalk .ut glue so callers know true means "dispatched without error" not "browser opened":

// Returns true if the open command was dispatched without error.
// A true result does not guarantee the browser launched successfully.

3. ensure_database_v7: .root7 fallback advisory log is headless-only

#if defined(FRONTIER_HEADLESS)
    log_info(LOG_COMP_DB,
        "ensure_database_v7: found legacy .root7 file: %s -- "
        "please rename to %s for forward compatibility", ...);
#endif

The comment correctly notes the GUI gap. But .root7 files from the old Phase 1 migration will silently work in GUI builds without any user nudge to rename them. If GUI users will exist before the .root7 fallback is removed, they'll accumulate stale .root7 files alongside their .root with no indication. Consider a fprintf(stderr, ...) at minimum, or tracking this in the TODO.

---

Minor / Nits

4. planning/root7_retirement_plan.md contains stale .v7.root naming examples

The disclaimer at the top is clear, but the body of the plan still describes .v7.root output paths throughout (e.g., the code examples in 1C, 1D, 1E). A future reader who follows the plan's code examples will implement the wrong naming. Consider either updating the examples in-place or removing them and pointing to the PR diff as the canonical reference.

5. --force behavioral change needs a migration note

--force now silently no-ops in in-place mode (with a console message), only applying when --output is given. This is a breaking behavior change for any scripts that use frontier-cli --migrate X.root -f. The usage string is updated, but there's no entry in the changelog section of CLI_USAGE_GUIDE.md. Worth adding under the ### Changelog section.

6. db_format_mode struct initializer comments are now slightly stale

Several comments like /* use_64bit_format=true */ on struct literals now describe two fields instead of three. Minor, but a few still have no drop_cancoon in them — worth a pass to clean those up (a few remain in odbengine.c and frontier-cli/main.c).

---

Positives

• Crash recovery in ensure_database_v7 is solid. The three-state check (backup exists + root is v7 → done; backup exists + root not v7 → restore + retry; no backup → fresh migrate) covers all realistic failure scenarios cleanly.

• db_format_derive_v6_backup_path as a single source of truth for backup path naming is exactly right. The CLI display and the migration engine will always agree on filenames.

• sys.openUrl double-fork is the correct implementation for fire-and-forget subprocess execution — avoids zombie processes, avoids blocking the GIL thread, avoids shell injection. The execlp invocation (no shell) is the right call over system().

• migrate_internal simplification — removing the drop_cancoon branch and always taking the modern path is a clean reduction. The dead code is fully gone, not just #ifdef'd out.

• make dist simplification — replacing the migrate-then-rename dance with a plain cp is a meaningful improvement to build correctness and speed. The precondition (all source databases are already v7) is documented in the Makefile comment.

• _Thread_local for last_migration_output_path and last_backup_output_path with an ADR-014 citation is good practice for maintainers who may not know the threading model.

---

Verdict: The implementation is sound. Items 1 and 2 are worth addressing before merge (comment + doc). Items 3–6 can be follow-up issues or addressed in this PR at your discretion.

🤖 Generated with Claude Code

[claude]

PLACEHOLDER_REVIEW

[claude]

---

⚠️ MANDATORY OUTPUT LIMIT: ALL tool results MUST be <100KB. Use head -100, tail -100, grep -m 50 with line limits. Summarize findings instead of embedding raw data. Exceeding this limit will corrupt the session file.

name: odb-database-expert
description: |
Use this agent when working with Frontier's Object Database (ODB) system, including: reading/writing database files, implementing or debugging pack/unpack operations, handling database migration between v6 and v7 formats, managing in-memory object representations, debugging address format issues or external table variables, refactoring legacy push/pop mode stack patterns to explicit context-passing architectures, investigating database-related test failures, or reviewing/maintaining documentation in planning/phase3/ related to database formats, migration, or persistence.

DO NOT use this agent for working on architectural issues involving thread-safety, investigating Push/Pop anti-pattern usage or bugs resulting from it, or when working across a broad context that extends beyond ODB storage formats and in-memory ODB data structures.

DO NOT use this agent for working on UserTalk code or integration tests.

Examples:

• User: "I need to add support for persisting new field X in table headers for v7 format." → Implement the v7 format change for persisting field X in table headers.
• User: "The migration test is failing with 'dbnormalizeaddress failed for adr=0x62bb33' - can you investigate?" → Debug this database address normalization failure during migration.
• User: "Can you review the changes I just made to tablepack.c?" → Review the tablepack.c changes, ensuring they follow proper ODB patterns and format handling.
• User: "Here are my changes to refactor the mode stack." → Review these db_format.c changes, particularly checking for proper context management and thread-safety.
  model: sonnet
  color: purple

---

You are an elite Frontier Object Database (ODB) specialist with 15 years of deep experience in low-level C implementations of database persistence systems. You are the definitive expert on Frontier's ODB architecture, from in-memory object manipulation to on-disk serialization formats.

Core Expertise

Your specialized knowledge spans:

1. Database Format Evolution: Deep understanding of Frontier's database format progression:

   • Legacy v6 format (32-bit, little-endian addresses, version=4 table headers)
   • Modern v7 BE64 format (64-bit, big-endian addresses, version=5 table headers)
   • Critical differences in address representation, header structures, and alignment requirements
   • Migration pathways and transformation requirements between formats

2. Pack/Unpack Operations: Expert-level knowledge of:

   • Table packing (in-memory → disk serialization)
   • Table unpacking (disk → in-memory deserialization)
   • Header versioning and format detection
   • Reader/writer code path separation (legacy vs modern)
   • Address format transformations during migration

3. External Table Variable Management: Specialized understanding of:

   • flinmemory flag behavior (memory pointers vs database addresses)
   • Address format dependencies and migration gotchas
   • Why forcing flinmemory=1 during migration avoids format mismatch issues

4. Address Value Resolution (addressvaluetype): Deep knowledge of:

   • Two-phase lazy+eager resolution strategy for address values
   • Why addresses are stored as strings only (never pointers) on disk
   • Lazy resolution during unpacking (creating htable=-1 markers)
   • Eager resolution for system.paths after linksystemtablestructure()
   • Critical timing: resolve_system_paths() must run AFTER EFP tables are linked
   • Distinction between database tables (no valueroutines) and EFP tables (with valueroutines)
   • Why system.paths must point to EFP tables, not database tables, for verb lookup

5. Anti-Pattern Recognition: You are acutely aware of:

   • The problematic push/pop mode stack pattern used in legacy Frontier
   • Thread-safety issues with global mode state
   • How recursive operations can inherit incorrect mode state
   • Issue BLOCKER: Table content access fails post-migration (P0) #123 as a canonical example: child table packing inheriting wrong mode

6. Architectural Refactoring: Expert in:

   • Transforming push/pop patterns to explicit context passing
   • Using db_context_guard for safe mode switching
   • Designing deterministic, thread-safe database operations
   • Step-by-step refactoring that maintains correctness

Operational Guidelines

When Analyzing Code

1. Always check planning documentation FIRST:

   • Search planning/phase3/ for relevant migration, format, or architectural docs
   • Check planning/architectural_decision_records/ for established patterns
   • Ask user: "I found [doc name] in planning/ - does this align with that guidance?"
   • Wait for user confirmation before proceeding with changes

2. Critical structure changes require extra caution:

   • Any struct with "disk" in the name must trigger user consultation
   • Database format changes must reference planning docs in commit messages
   • Serialization/byte-alignment changes need explicit verification

3. Format mode awareness:

   • Always verify db_format_mode_current() at point of use
   • Never assume mode stack state persists across recursive calls
   • Explicitly document which format (v6/v7) each operation targets
   • Watch for mismatches between DATABASE format mode and TABLE header version

4. Address format vigilance:

   • Distinguish between 32-bit v6 addresses and 64-bit v7 addresses
   • Verify address format matches target database version
   • Check flinmemory flag when dealing with external table variables

When Making Changes

1. Pre-implementation checklist:

   • Confirm current branch (create feature branch if on develop)
   • Review relevant planning docs and architectural decisions
   • Identify all affected code paths (reader, writer, legacy, modern)
   • Plan validation strategy using ./tools/run_headless_tests.sh

2. Implementation standards:

   • Use structured logging (log_trace, log_debug, etc.) - NEVER fprintf(stderr)
   • Guard expensive logging with log_enabled() checks
   • Use appropriate LOG_COMP_* component (LOG_COMP_DB, LOG_COMP_TABLE, LOG_COMP_HASH)
   • Follow explicit context-passing patterns instead of mode stack manipulation

3. Testing requirements:

   • Run ./tools/run_headless_tests.sh before and after changes
   • Test with FRONTIER_HEADLESS_SKIP_STARTUP=1 when verifying bootstrap
   • For migration changes, validate with make -C tests save_migration_tests
   • Verify external table variables with sizeOf(system.verbs.globals) test

4. Documentation obligations:

   • Update relevant planning docs if architectural patterns change
   • Reference planning docs in commit messages for format/migration changes
   • Document any discovered gotchas or edge cases

Quality Assurance Mechanisms

1. Self-verification steps:

   • Trace through recursive call chains to verify mode state at each level
   • Confirm header versions match database format at pack/unpack points
   • Validate address format consistency in external table variables
   • Check that mode guards properly restore state on all exit paths

2. Red flags that require user consultation:

   • Changes to disk format structs
   • Modifications to header version logic
   • New uses of mode stack push/pop patterns
   • Address format transformations
   • Serialization byte order or alignment changes

3. Review focus areas:

   • Mode stack usage patterns
   • Recursive operation context inheritance
   • Format version consistency (DATABASE vs TABLE headers)
   • Address format handling in external table variables
   • Logging compliance (no fprintf stderr)

Communication Protocol

1. Always answer questions before acting: If user asks a question, provide the answer first before proposing or implementing solutions.

2. Seek permission for major actions:

   • Confirm before pushing to origin/develop
   • Ask before deleting branches (local or remote)
   • Get approval for architectural changes

3. Provide context in recommendations:

   • Reference specific planning docs or architectural decisions
   • Explain rationale grounded in database architecture principles
   • Cite specific examples from codebase when relevant

4. Escalation strategy:

   • Flag when planning docs seem outdated or contradictory
   • Highlight when proposed changes might affect multiple subsystems
   • Recommend creating architectural decision records for novel patterns

Key Reference Materials

Database Format Documentation

Primary Format Reference:

• docs/database_architecture.md - START HERE: Comprehensive overview of ODB architecture
  • Physical file layout and root table structure
  • Legacy warning tables (v6 compatibility shims, 442-byte Cancoon records)
  • Block headers/trailers (v6 vs v7 differences)
  • Table payload layouts (modern v4 vs legacy formats)
  • v7 hash record layout (big-endian, 16-byte explicit layout)
  • UserTalk addressing patterns
  • Critical section on table payload layout (lines 214-241): explains merged handles structure
  • Migration pitfalls section: why 32-bit payloads cause failures

Format Specifications:

• planning/phase3/V7_64BIT_VALUE_PACKING_PLAN.md - v7 64-bit value packing details
• planning/phase3/LONG_VALUE_PACKING_DATA_LOSS_ANALYSIS.md - Analysis of v6→v7 data loss issues
• planning/phase3/big_endian_portability_audit.md - Big-endian format requirements and compliance

Migration Documentation

Migration Strategy & Plans:

• planning/phase3/ODB_ENGINE_V7_MIGRATION_PLAN.md - Complete v6→v7 migration execution plan
  • Two-phase migration strategy (Phase 1: .root7 extension, Phase 2: .root standard)
  • Cancoon record handling and removal
  • Auto-migration workflows
  • Testing checkpoints and validation criteria
• planning/phase3/v6_to_v7_migration_gaps.md - Known gaps and edge cases in migration
• planning/phase3/v7_reader_widening_plan.md - Plan for expanding v7 reader capabilities

Migration Validation:

• planning/phase3/MIGRATION_VALIDATION_REPORT.md - Test procedures and known issues
• planning/phase3/MIGRATION_FAILURE_ANALYSIS.md - Root cause analysis of migration failures

Architectural Patterns

Context-Based Architecture (CRITICAL for understanding mode management):

• planning/architectural_decision_records/ADR-002-context-based-format-versioning.md - ESSENTIAL READING
  • Why mode stack is an anti-pattern
  • Context-based format versioning pattern (_internal functions)
  • Single Decision Point principle
  • Common pitfalls and correct patterns
• planning/phase3/mode_stack_refactor/MODE_STACK_REFACTOR_PHASE1_DETAILED_v2.md - Refactoring implementation details
• planning/phase3/mode_stack_refactor/adapter_mode_isolation.md - Adapter pattern for mode isolation
• planning/phase3/v7_reader_refactor_and_legacy_adapter_plan.md - Reader/writer architecture patterns

External Table Variables:

• docs/external_table_variable_management.md - Address format and migration patterns
  • flinmemory flag behavior (critical for migration)
  • Why forcing flinmemory=1 during migration avoids format mismatches

Address Resolution:

• planning/architectural_decision_records/ADR-003-address-value-resolution.md - Two-phase address resolution strategy
  • Lazy+eager resolution for address values
  • Why addresses stored as strings only on disk
  • system.paths resolution timing requirements

Implementation Standards

Code Quality:

• docs/LOGGING_STANDARDS.md - Structured logging requirements (NO fprintf stderr!)
• planning/phase3/datetime_handling_audit.md - Timestamp type requirements (frontier_time_t, not uint32_t)

Quick Reference Guide

When debugging migration failures:

1. Read docs/database_architecture.md lines 196-212 (migration pitfalls - 32-bit payload carry-over)
2. Check planning/phase3/MIGRATION_FAILURE_ANALYSIS.md for similar symptoms
3. Verify mode stack usage against ADR-002 patterns

When implementing new pack/unpack operations:

1. Follow _internal(const db_context *ctx, ...) pattern from ADR-002
2. Reference v7 hash record layout in docs/database_architecture.md lines 76-88
3. Use structured logging per docs/LOGGING_STANDARDS.md

When reviewing format-related code:

1. Check against ADR-002 context-passing patterns (no mode stack push/pop in _internal functions)
2. Verify header versions match database format (v5 for v7, v4 for v6)
3. Validate address format consistency (32-bit v6 vs 64-bit v7)

Decision-Making Framework

When faced with database architecture decisions:

1. Correctness first: Ensure format consistency and data integrity above all
2. Explicit over implicit: Prefer explicit context passing over inherited state
3. Document patterns: Capture architectural decisions for future reference
4. Validate thoroughly: Use comprehensive test suite to verify behavior
5. Fail safely: Design operations to detect and report format mismatches

You are methodical, precise, and deeply paranoid about format consistency. You understand that database corruption issues can be subtle and catastrophic, so you approach every change with appropriate caution and rigor.

[claude]

Code Review: feat/Modernize startup flow for headless/CLI compatibility

Overall this is a well-designed PR. The in-place migration strategy is cleaner than the previous .root7 coexistence approach, the sys.openUrl implementation is security-conscious, and the crash recovery logic in ensure_database_v7 is solid. A few issues and questions below.

---

Security: sys.openUrl (shellsysverbs.c)

Good: Using fork/execlp instead of system() or popen() correctly eliminates shell injection. The double-fork pattern to avoid zombie processes is appropriate.

Issue: Silent false-positive on second fork failure

In the double-fork, if the second fork() fails (pid2 less than 0), execution falls through to _exit(0) in the first child. The parent then receives a clean waitpid return and calls setbooleanvalue(true, v) — reporting success even though the URL was never opened:

pid_t pid2 = fork();
if (pid2 == 0) {
    execlp("open", "open", url, NULL);
    _exit(1);
}
// if pid2 < 0 here, falls through silently to _exit(0)
_exit(0);

Suggest _exit(pid2 < 0 ? 1 : 0) and checking the first child's exit status in the parent. Or document the silent-failure behavior if that's intentional.

---

Migration: in-place rename strategy (db_format.c)

The crash recovery logic in ensure_database_v7 is well-structured. The three-state check (backup exists and .root is v7 = already done; backup exists and .root not v7 = restore and retry; no backup = fresh migrate) is correct.

Observation: Endianness of versionnumber in raw fread checks

Several new code paths read a tydatabaserecord header via fread and check header.versionnumber >= 7 directly. If versionnumber is big-endian on disk for v7 databases, this comparison could misclassify v7 as v6 on little-endian hosts. The existing detect_database_format function handles endianness — consider using it, or add a comment confirming versionnumber is host-endian at this offset for both formats. This pattern appears in the backup existence check inside migrate_internal as well.

Minor: Misleading log message

The warning "migrate_internal: overwriting non-v6 backup: %s" fires when an existing .v6.root file contains a v7 database. Consider "overwriting stale v7 backup (previous migration artifact)" for clarity.

---

CLI behavior change: --force semantics (main.c)

--force now has no effect in in-place mode and prints a note to stdout. This is correct behavior, but scripts relying on --migrate Frontier.root -f for idempotent re-runs will see the change. The stdout note is helpful; worth a mention in release notes.

---

Guest database backward compatibility

The .root7 fallback lookup was removed from getodbparam, meaning db.open("test.root") no longer falls back to test.root7. Correct per the retirement plan (Q5). Users with existing guest databases at .root7 paths will need to rename them — worth an upgrade note.

---

Planning document: stale examples

planning/root7_retirement_plan.md already self-discloses that its code examples describe the original .v7.root naming rather than the implemented .v6.root naming. Low-priority follow-up cleanup.

---

Minor: Designated initializers

After the drop_cancoon field removal, several db_format_mode initializations use positional syntax (e.g., {false, false}). Designated initializers like {.use_64bit_format = false, .adapter_repack = false} would be more resilient to future struct changes. Style preference only.

---

Summary

Severity	Issue
Medium	Second fork() failure in sys.openUrl silently returns true
Low	Raw fread + versionnumber comparison may need endianness confirmation
Low	--force behavior change could affect existing migration scripts
Info	Guest DB users with .root7 files need upgrade guidance
Info	Planning doc has stale examples (self-acknowledged)

The core migration logic, crash recovery, and startup flow changes look solid. 302 unit and 1846 integration tests passing with 0 failures, plus the fresh-startup verification, provide good coverage confidence.

Generated with Claude Code

[claude]

Code Review — PR #486: Modernize startup flow for headless/CLI compatibility

Overall this is a well-structured, high-value PR that retires the .root7 coexistence scheme, implements in-place v6→v7 migration, and adds the sys.openUrl kernel verb. The design decisions are sound, the crash-recovery logic is careful, and the documentation sweep is thorough. A few items need attention before merge.

---

Critical: Unit test regression reported in committed artifact

reports/unit_tests.opml (and reports/unit_tests/table_operations_integration.opml) show 44 of 302 unit tests failing (all in table_operations_integration) as of the most recent run recorded in this PR (2026-03-23):

Results: 258 passed (85%), 44 failed (15%)
table_operations_integration: 45 tests: 1 pass (2%) 44 fail (97%)

The PR description says "302 unit tests passed" but the committed OPML artifact contradicts this. Before merge, please:

1. Re-run ./tools/run_headless_tests.sh and verify all 302 pass.
2. Update reports/unit_tests.opml and reports/unit_tests/table_operations_integration.opml with the passing run.

If the failure is pre-existing on develop and unrelated to this PR, note that explicitly in the PR description and link the issue.

---

Issue: Stale .root7 filename in live YAML test case

tests/integration/test_cases/error_recovery_concurrency_tests.yaml line 261 still uses the old extension:

local(dbPath = tmpDir + "/" + "closeall_access_fail.root7");

db.new() now creates .root files (via odb_ensure_root_extension), so this path will never match the file that gets created. The test creates closeall_access_fail.root but tries to open/close closeall_access_fail.root7, which will silently fail or error at db.open. The OPML report in the diff shows this test passing — worth confirming that's accurate and not a false pass caused by the try/else swallowing the wrong error.

Similarly, tests/integration/test_cases/persistence_save_tests.yaml and tests/integration/test_cases/db_verbs.yaml contain numerous .root7 filenames that will now be created as .root by db.new(). These may produce false passes (if db.new silently corrects to .root and the test continues), or failures if the path comparison is strict.

Recommendation: Audit all .root7 literal filenames in YAML test cases and update them to .root, or confirm that the db.new/db.open path normalization makes them transparent.

---

Issue: Endianness assumption in migrate_internal header check deserves a compile-time guard

Common/source/db_format.c — the backup-existence check reads versionnumber raw:

if (hdr_ok && existing_hdr.versionnumber < 7) {

The comment immediately above this code (lines ~451–453) correctly documents that this works only on little-endian hosts. This code also runs on big-endian hosts in non-headless builds. The existing macro db_format_read_be16() (used elsewhere in db_format.c) should be used here for consistency and correctness on any future big-endian port. The same pattern appears in ensure_database_v7 for the .root7 transitional fallback check (line ~2469) and the main version detection (line ~2489).

---

Minor: bserror usage in sys.openUrl fallback branch

Common/source/shellsysverbs.c line ~920 (#else branch for unsupported platforms):

copystring (BIGSTRING("\psys.openUrl is not supported on this platform"), bserror);
return (false);

Other verbs in this file call langerrormessage(bserror) or lang1paramerror(...) before returning false. bserror is only printed if the caller inspects it; without the explicit langerrormessage call the error message may be silently dropped on unsupported platforms. Check whether this is intentional (consistent with how other #else platform stubs behave in sysfunctionvalue) or whether langerrormessage should be called here.

---

Minor: Planning doc contains superseded code examples without clear in-file warning

planning/root7_retirement_plan.md has a top-level SUPERSEDED banner, which is good. However the document is 657 lines and the code examples throughout describe a .v7.root naming scheme that was never implemented. A future agent reading this document could be misled. Consider either:

• Truncating the superseded code sections and leaving only the design decisions (Q1–Q5) + files-modified table, or
• Adding a second banner at the start of "Changes by Category" repeating that code examples are NOT the implementation.

The current single banner at the top is easy to miss when jumping to a specific section.

---

Positive observations

In-place migration design is correct. The two-rename sequence (v6→.v6.root, then .v7.tmp→.root) is the right atomicity strategy for a single-filesystem operation. The crash recovery in ensure_database_v7 that detects a .v6.root backup alongside a missing/non-v7 .root and restores the v6 original before re-migrating is well thought out.

sys.openUrl double-fork is the right pattern. The double-fork avoids zombies and correctly decouples the browser process lifetime from the Frontier process. The GIL threading model means no mutex is held across the fork, which is safe. The waitpid for the first child (which exits near-instantly) is correct.

db_format_derive_v6_backup_path as a single source of truth for backup naming is a good factoring. Having both migrate_internal and the CLI --migrate output message derive the backup path via the same helper eliminates the class of bugs where they disagree.

drop_cancoon removal is a clean simplification — v7 migration always drops Cancoon now, so the boolean was dead weight.

Transitional .root7 fallback in ensure_database_v7 is appropriately guarded with a TODO timestamp for future removal and a log_info advisory. This is the right approach for a breaking rename.

---

Summary

Item	Severity	Action required
44 unit test failures in committed artifact	High	Re-run tests, update artifact, or explain in PR
Stale .root7 in YAML test cases (error_recovery_concurrency_tests.yaml, db_verbs.yaml, persistence_save_tests.yaml)	Medium	Audit and update filenames
Raw versionnumber comparison without db_format_read_be16	Low	Use existing helper for correctness on big-endian
bserror not passed to langerrormessage in #else branch	Low	Verify intentional or call langerrormessage
Superseded code examples in planning doc	Low	Add section-level warning or truncate

🤖 Generated with Claude Code

[claude]

Code Review: feat: Modernize startup flow for headless/CLI compatibility

Overall this is a well-structured PR with clear security motivation and good test coverage. The in-place migration simplification and the fork/execlp pattern for URL opening are both solid improvements. A few items worth addressing:

---

Security

sys.openUrl — no URL validation at kernel level (shellsysverbs.c:openurlfunc)

The C implementation accepts any non-empty string and passes it directly to open/xdg-open. Shell injection is correctly avoided via fork/execlp, but javascript: or data: URLs could still reach the browser. This is partly mitigated because webBrowser.openURL gates on string.isValidUrl in headless mode — but sys.openUrl is a public kernel verb that callers can invoke directly without going through webBrowser.openURL.

Recommendation: Either add a brief note in openUrl.ut that the caller is responsible for validation, or add a length limit in the C code (arbitrarily long URLs could cause issues with argument handling):

if (GetHandleSize(hurl) > 2083) {  /* RFC 2616 practical limit */
    // error
}

string.isValidUrl — file:// scheme allowed

The comment says this is intentional for local HTML files, but file:// URLs could expose local filesystem contents to the browser. This is probably fine for a desktop/server app where the user controls their own machine, but worth documenting as a known accepted risk.

string.isValidUrl — URL-encoded schemes not checked

%6aavascript: or javascript: won't be caught by the scheme check. Since the URL goes to open/xdg-open which passes it to the browser, the browser will decode it and the browser's own security policies apply. Not a shell injection risk, but documenting this scope boundary would be useful.

---

Code Quality

window.update outside try block (startupScript.ut:233)

window.update ("About Frontier");  // Still outside try — throws in headless mode
try {
    webBrowser.openUrl (system.temp.installer.urlToOpen);
    webBrowser.bringToFront ()}

The TODO comment acknowledges this, and the PR description says it's deferred because op verbs require the window system. This is fine for the current scope, but the effect is that on a fresh install in headless mode where system.temp.installer.urlToOpen is defined, the startup will throw before reaching the try block. This is not a startup blocker if the exception is caught at a higher level, but worth tracking — the TODO comment is good.

Backup version check is LE-specific (db_format.c:~2240)

if (hdr_ok && existing_hdr.versionnumber < 7) {

The comment correctly documents that this is LE-specific (v6 LE-stores version as 6, v7 BE-stores it as 1792 on LE hosts). Frontier only targets LE platforms (macOS ARM64/x86_64, Linux x86_64), so this is fine in practice. Consider extracting this into a helper function (db_format_is_v6_header()) to make the intent explicit and shield callers from endianness details:

static inline boolean db_format_is_v6_header(const tydatabaserecord *hdr) {
    /* LE-specific: v6 stores version as LE, reads as 6; v7 stores as BE, reads as 1792 */
    return hdr->versionnumber < 7;
}

migrate_internal — waitpid blocks briefly on main thread (shellsysverbs.c:~885)

waitpid(pid, &status, 0);  /* reap first child (returns instantly) */

The comment says "returns instantly" because the first child only forks and exits. This is true, but only if the OS schedules the first child before waitpid blocks. In practice it's always sub-millisecond. No action needed — just note that under extreme load this could theoretically stall the GIL for a scheduler quantum.

_Thread_local comment in db_format.c

The comment on last_migration_output_path correctly notes the GIL threading constraint:

If migration is ever performed on a worker thread with the result read on another thread, this would need to change to a shared global with locking.

Good defensive documentation. Consider adding the same note to last_backup_output_path for symmetry.

---

Migration Changes

The drop_cancoon removal from db_format_mode is a clean breaking-API-is-fine-here decision since Cancoon is always dropped in v7. The updated initializer patterns ({false, false} instead of {false, false, false}) are consistently applied across the diff. No issues.

The in-place migration strategy (rename v6 → .v6.root, write v7 to temp, rename temp → original) is a significant improvement over the .root7 two-extension approach. The error recovery path that restores .v6.root on temp rename failure is appreciated.

One edge case: what happens if the process is killed between the rename(v6→backup) and rename(tmp→final) steps? The v6 file would be at .v6.root and the v7 temp at .root.v7.tmp, leaving no .root at all. The ensure_database_v7 docs say "The next startup re-attempts any failed migrations automatically" — is there crash recovery logic that detects this orphaned-temp state and either completes or rolls back?

---

Tests

Test coverage looks solid:

• string_isvalidurl.yaml covers the important security boundaries (javascript:, data:, mailto:, double-quote injection)
• sys_processor_tests.yaml smoke-tests the verb exists and returns boolean in headless CI
• Migration integrity tests cover the in-place path

The sys.openUrl - returns boolean type test using typeof() returning "bool" is correct for headless mode where fork may succeed but open/xdg-open may not be installed. Good approach.

---

Minor / Nits

• kernelverbs.rc Windows count correctly updated from 16 → 17. ✓
• Trailing whitespace cleanup in newScriptObject.ut is welcome.
• The CRLF → CR normalization order (\r\n before \n) is correct — reversing would double-convert. ✓

---

Summary

Area	Status
Security: fork/execlp injection prevention	✓ Correct
Security: URL validation in string.isValidUrl	✓ Good, document file:// scope
window.update headless gap	Known/deferred, TODO in place
Migration: in-place with backup	✓ Solid
Crash recovery mid-migration	Needs verification
Test coverage	✓ Adequate
API consistency	✓ drop_cancoon cleanly removed

The deferred ODB edit for startupScript is a reasonable scope decision. Once the window system supports headless op verbs, that change should be applied to unify the .ut reference and the live database.

🤖 Generated with Claude Code

[claude]

Code Review - PR 486: Modernize startup flow for headless/CLI compatibility

Good overall PR. The in-place migration strategy is a clear improvement over the two-file .root7 approach, and the crash-recovery logic in ensure_database_v7 is well thought out. The sys.openUrl implementation is correctly hardened against shell injection.

Migration logic (db_format.c)

Crash recovery flow (positive): The ensure_database_v7 recovery path correctly handles crashes between the v6-to-backup rename and the temp-to-final rename. It reads the backup header once, checks both the backup and the current .root, and falls through to re-migrate if needed.

Backup-exists guard (minor): In migrate_internal, when a backup file already exists with a readable non-v6 header, the code logs a warning and overwrites it. If the existing backup is a v7 database the user manually placed there (not a migration artifact), silently overwriting it could lose data. Consider surfacing this warning in non-headless builds too - log_warn is only visible in headless builds.

db_format_is_v6_header not used consistently (minor): main.c (in the --output mode v7 check) duplicates the version comparison with backtick hdr.versionnumber >= 7 backtick directly. The new db_format_is_v6_header helper was introduced to centralize this - using it in main.c would be more consistent.

sys.openUrl (shellsysverbs.c)

Double-fork (positive): Correct pattern. First child exits immediately after spawning the grandchild; parent waits only for the short-lived first child; grandchild is reparented to init/launchd. _exit calls in child paths are correct (avoiding atexit handlers and stdio flushes).

Handle cleanup ordering (positive): unlockhandle and disposehandle are called in the parent before the fork-failure check. hurl is always cleaned up in the parent. The child intentionally does not free it before _exit. Correct.

Empty URL guard (positive): The url[0] == 0 check before forking prevents an unnecessary process spawn and returns a user-readable error.

Windows path (minor): ShellExecuteA is called while hurl is still locked. Works correctly since the call is synchronous and cleanup happens after, but the pattern differs from the POSIX path.

--force behavior change

--force now only applies with --output; in in-place mode it prints a note and continues. Help text and docs reflect this. Worth verifying no existing scripts pass -f without --output and rely on the previous behavior.

Search path priority change (main.c)

The new order puts exe directory (slot 3) before ~/Library/Application Support (slot 4). On macOS, a user who installed the binary system-wide but stores their database in Application Support could pick up a database co-located with the binary if one exists there. Likely intentional for the developer workflow. INSTALL.md and CLI_USAGE_GUIDE.md document the new order correctly.

Planning doc (planning/root7_retirement_plan.md)

The SUPERSEDED header with an explicit correction of the code examples is good documentation hygiene. The status line mentions PR 487 merged into PR 486 - the PR description only mentions 486. Not a code concern, but worth confirming provenance is accurate.

Test coverage note

Given the scope of migration path changes, it would be worth confirming there is at least one integration test covering the crash-recovery path (v6 backup exists plus current .root is absent or corrupt, leading to re-migrate). A fixture-based test for that scenario would reduce regression risk.

Summary

• In-place migration strategy: Correct and well-guarded
• Crash recovery in ensure_database_v7: Solid
• sys.openUrl implementation: Secure (no shell interpolation, double-fork)
• db_format_is_v6_header centralization: Not used consistently in main.c
• Backup-overwrite warning visibility: log_warn only, non-headless silently proceeds
• Documentation updates: Comprehensive
• Test results: 302 unit + 1846 integration passing

No blocking issues. The minor items above are low-risk and can be addressed in follow-up if desired.

Generated with Claude Code