Retire .root7 extension from runtime and docs

[jsavin]

Summary

• Retire the .root7 file extension from the Frontier runtime, tests, and documentation
• Migration now renames v6 to .v6.root and writes v7 to the original .root path (crash-safe: temp file → atomic renames)
• Simplify dbopenverb from ~90 to ~40 lines by removing scattered .root7 fallback logic
• .root7 still recognized for backward compatibility (positional args, transitional fallback with dated TODO)
• Update 17 documentation files to reflect new behavior

Depends on: PR #486 (feature/setup-frontier-startup) — that PR migrates source databases to v7 format; this PR retires .root7 from the runtime

Changes

Runtime (3 files):

• db_format.c: migrate_internal uses rename-to-backup + temp-write + rename-to-final; ensure_database_v7 checks .v6.root sibling with v7 header verification + transitional .root7 fallback
• dbverbs.c: Remove odb_ensure_root7_extension, odb_check_root7_exists, ROOT7_EXTENSION_LEN; new odb_ensure_root_extension; simplified dbopenverb and getodbparam
• main.c: Collapse 8 search paths to 4; update --migrate defaults; --output error handling with strerror(errno)

Tests/tools (14 files):

• All Frontier.root7 references → Frontier.root
• Migration tests pass with new naming (17/17)
• Bisect scripts clean up both legacy artifacts

Docs (17 files):

• CLI_USAGE_GUIDE, TESTING_GUIDE, GETTING_STARTED, DEBUGGING_GUIDE, GUEST_DATABASES, etc.
• CLAUDE.md, agent definitions, test READMEs

Test plan

• make -C frontier-cli — builds clean (6 pre-existing warnings)
• make -C tests test — all pass (table_operations_integration failures pre-existing)
• make -C tests test-migrate-flag — 17/17 pass
• grep -rn "root7" verification — only backward-compat references remain
• cd tests && make test-integration — integration tests
• Manual: ./frontier-cli --migrate tests/fixtures/v6/Frontier.root produces .v6.root backup + v7 at .root

🤖 Generated with Claude Code

[claude]

Code Review: Retire .root7 Extension from Runtime and Docs

Overall this is a well-structured and carefully executed refactor. The migration strategy (rename v6 to .v6.root, write v7 to the original .root path) is the right mental model — the output is the canonical thing, not the backup. The crash-safety design in db_format.c is sound. Several issues worth discussing below.

---

Concern 1: Unit test regression in committed reports (significant)

reports/unit_tests.opml shows the test run captured for this PR has 152 tests, 108 pass (71%), 44 fail — down from 302 tests / 100% pass in the prior commit. The table_operations_integration suite went from 45/45 passing to 1/45. 13 test executables are missing entirely from the report (headless_selection_tests, headless_thread_registry_tests, menu_v7_refcon_tests, op_context_tests, refcon tests, table_context_tests, tcp_phase1a_unit_tests, test_callback_infrastructure, test_stringerrorlist, test_sys_shell_command_verbs, time_portability_test).

The PR description acknowledges table_operations_integration failures are pre-existing, but the report committed here shows a much worse picture than before. It's not clear whether these failures/missing tests are a worktree/environment artifact or a real regression introduced by this branch. Before merging, the committed test reports should reflect a clean full run (302 tests, 100% pass) or the failures need to be explicitly diagnosed and documented.

---

Concern 2: --output flag leaves the filesystem in a surprising intermediate state (medium)

In frontier-cli/main.c, the --output path does three filesystem operations after migration:

1. ensure_database_v7 runs: renames v6 to .v6.root, writes v7 to .root
2. The code copies v7 to --output path
3. Removes the v7 at the original path and renames .v6.root back to the original

Steps 2→3 are not atomic. If the copy succeeds but step 3's remove() fails (permissions, NFS, etc.), you end up with the v7 database at both the original path AND the --output path, with the v6 backup gone. The error message at that point ("Cannot remove migrated file") will confuse users who just wanted to write to a specific output location.

A simpler approach for --output: call migrate_internal with an explicit output path rather than doing in-place migration + copy + restore. The --output path already passes through ensure_database_v7, which delegates to migrate_internal. It might be worth adding an explicit output parameter to that call chain rather than the post-hoc copy/restore dance.

---

Concern 3: ensure_database_v7 crash-recovery heuristic has a gap (medium)

In Common/source/db_format.c, ensure_database_v7 detects a previous migration by checking whether .v6.root exists. If it does, it reads the header from .root to confirm it's v7. The comment correctly notes: "A crash between the v6 rename and temp-to-final rename could leave .root as v6 or missing."

The gap: if .root is missing (crash after rename(v6->backup) but before rename(tmp->final), and tmp was cleaned up by OS), fopen(db_path, "rb") returns NULL, fp_check is NULL, header_ok is false, and the code falls through to re-migrate. But re-migration calls migrate_internal(db_path, ...) which will try to read the now-missing .root — that will fail downstream with a less-than-obvious error.

The code should explicitly handle the case where .v6.root exists but .root does not: offer to rename .v6.root back to .root (restore from backup) and then re-migrate. A log message here would help operators diagnose the situation.

---

Concern 4: last_backup_path is set to output_path not backup_path (minor bug)

In Common/source/db_format.c around line 2122–2124:

strncpy(last_backup_path, output_path, sizeof last_backup_path);

This is the old code tracking the output (v7) path under the name last_backup_path. After the rename-based strategy, the semantically meaningful path to remember is backup_path (the .v6.root file), not output_path. If last_backup_path is surfaced anywhere in error messages or test assertions, this could be confusing. Worth verifying what last_backup_path is used for and whether it should now store backup_path.

---

Concern 5: sys.openUrl — unlockhandle placement after fork (minor)

In Common/source/shellsysverbs.c, after pid = fork(), both the parent branch and the error branch call unlockhandle(hurl) and disposehandle(hurl). But if pid == 0 (the first child), neither of those cleanup calls is reached before _exit(0). The handle is leaked in the child process before the child exits. This is harmless on modern Unix (the child's address space is torn down at _exit), but it's worth noting as a non-idiomatic pattern. A comment acknowledging this would prevent future confusion.

Also: the Windows path uses (int)(intptr_t) cast for the ShellExecuteA return value. ShellExecuteA returns HINSTANCE, which is a pointer-sized type on Win64. The cast to intptr_t then int is correct for checking the > 32 threshold but could silently truncate on 64-bit Windows if not reviewed carefully. A (uintptr_t) intermediate cast would be safer: (int)((uintptr_t) ShellExecuteA(...)).

---

Concern 6: Integration test completion status (minor)

The PR description marks cd tests && make test-integration as unchecked ([ ]). The string_isvalidurl.yaml tests were previously skipped (pending v6 retirement) and are now enabled, which is good. But given the PR touches migration paths that affect how test fixtures are found, the integration test run is important. The PR should either complete this check or document why it was deferred.

---

Positive observations

• The crash-safe migration sequence in migrate_internal (write temp → rename v6 → rename temp) is well-designed and the best-effort restore on failure is a nice touch.
• Removing the TOCTOU double-check in dbopenverb is correct: the new in-place strategy makes the race moot since there's no separate .root7 file to contend over.
• v6 fixture preservation in tests/fixtures/v6/ is the right call — migration tests need a pristine v6 source.
• The planning/root7_retirement_plan.md document is thorough; interesting that the final implementation diverged from the plan (plan described .v7.root output, implementation uses .v6.root backup + in-place v7). The plan doc could be updated to reflect what was actually built, or noted as superseded.
• Simplifying dbopenverb from ~90 to ~40 lines is a clear win. The old TOCTOU handling was complex and the new ensure_database_v7 delegation is much easier to reason about.
• sys.openUrl using fork/execlp with double-fork rather than shell string interpolation is the correct security approach. Defense-in-depth with string.isValidUrl remaining in place is appropriate.

---

Summary

The core design is solid. The main blocker before merge is clarifying the unit test regression visible in the committed reports — whether those 44 failures and 13 missing test executables are a build environment issue on this branch or a real regression needs to be confirmed. The other concerns are medium/minor and could be addressed in follow-up PRs if desired, but the --output intermediate state issue and the crash-recovery gap in ensure_database_v7 are worth tracking.

[claude]

Code Review: Retire .root7 Extension (#487)

This is a well-scoped, well-documented PR that completes a clean naming convention transition. The core design is sound — placing the v7 database at the original .root path and backing up the v6 original to .v6.root is cleaner than the Phase 1 .root7 convention. The planning doc (planning/root7_retirement_plan.md) is detailed and the implementation closely follows the design decisions laid out there.

Overall this is solid work. A few issues need attention before merge, ranging from a stale test file with the old convention to a couple of correctness concerns in the new code.

---

Issues That Need Attention

1. db_migration.yaml still references .root7 convention (test staleness)

tests/integration/test_cases/db_migration.yaml lines 17 and 38 still reference .root7 paths, and the comments at the bottom of the file (lines 115-129) still describe the old behavior (test.root7 was created, Delete v7 file steps). The skipped tests reference SYSTEM_ROOT_V6 and .root7 destinations.

This is the kind of stale reference that will confuse the next person who tries to enable those skipped tests. The file-level comment at line 7 also says "Does NOT modify v6 source file" which is now the opposite of the in-place migration behavior (the v6 file IS renamed).

Recommendation: Update this file to reflect the new behavior — the v6 file is renamed to .v6.root, and the v7 database occupies the original path.

2. Missing integration test for sys.openUrl

This PR adds a new UserTalk verb sys.openUrl to the kernel (new enum value, new case in sysfunctionvalue, registration in both kernelverbs.r and kernelverbs.rc). However, tests/integration/test_cases/sys_processor_tests.yaml has no tests for it.

The existing sys tests have good coverage patterns — at minimum there should be:

• A test that sys.openUrl is recognized/callable (smoke test, even if it returns false in headless)
• A test for error handling with a non-string argument
• A test that validates the return type

In headless CI, launching a browser isn't feasible, but the verb can still be tested structurally. For example, calling it with a file:// or about:blank URL and verifying it returns a boolean (it will call fork/execlp on Linux and return true since the fork will succeed even if xdg-open isn't installed). If the behavior on Linux headless is to return false or error, that should be documented and tested.

Per docs/AI_SHARED_GUIDELINES.md: "New or modified behavior is covered by tests."

3. planning/root7_retirement_plan.md is not indexed

planning/INDEX.md does not reference the new planning doc. Since the project uses planning/INDEX.md as navigation, this doc won't be discoverable.

Recommendation: Add a brief entry to planning/INDEX.md pointing at this retirement plan document.

---

Code-Level Observations

4. Path length check off-by-one in ensure_database_v7

// db_format.c line 2349
if (strlen(db_path) >= sizeof(v6_backup_path) - 9) { /* 8 chars for ".v6.root" + 1 for null terminator */

The comment says "8 chars for .v6.root" but .v6.root is 8 characters, and sizeof(v6_backup_path) - 9 leaves room for 8 chars + null. That math is correct for an input with .root extension. However, for the else branch (inputs without .root), the backup path is %s.v6 — only 3 extra characters. The check using -9 is overly conservative in that case but still safe. Consider factoring this into a shared helper or at minimum adding a comment that the length guard is conservative for non-.root inputs.

5. sys.openUrl — potential handle leak on fork failure path

// shellsysverbs.c — openurlfunc case
pid_t pid = fork();

if (pid == 0) { /* first child */
    pid_t pid2 = fork();
    if (pid2 == 0) { /* grandchild */
        execlp(...);
        _exit(1);
    }
    _exit(0);
}

unlockhandle(hurl);
disposehandle(hurl);

if (pid < 0) { /* fork failed */
    return (setbooleanvalue(false, v));
}

When fork() fails (pid < 0), the code returns setbooleanvalue(false, v) — but hurl has already been unlocked and disposed at that point (those calls happen before the pid < 0 check). So there is no actual leak here. However, this is easy to misread: the unlockhandle/disposehandle appear to be "unconditional" but they're actually only reached by the parent after the child has _exit'd. The fork-failure case is handled correctly because the dispose already happened.

The comment explaining the double-fork pattern is good. One small addition that would help readers: a brief note that hurl is intentionally disposed in the parent before the pid < 0 check, not inside the if (pid < 0) block.

6. migrate_32bit_to_64bit_drop_cancoon still identical to migrate_32bit_to_64bit

After this PR, both functions call migrate_internal(db_path, true, NULL):

boolean migrate_32bit_to_64bit(const char *db_path) {
    return migrate_internal(db_path, true, NULL);
}

boolean migrate_32bit_to_64bit_drop_cancoon(const char *db_path) {
    return migrate_internal(db_path, true, NULL);
}

This was the case before the PR too, but since the PR is already simplifying this file, it would be a good time to either (a) remove the redundant function and update callers, or (b) add a comment explaining why both exist with the same body. As it stands it looks like a copy-paste mistake.

7. dbopenverb silent migration failure fallthrough

if (ensure_database_v7(cpath, &migrated, output_path, sizeof(output_path))) {
    /* ... */
}
/* If ensure_database_v7 fails, fall through and try to open the original */

The comment documents the intent but the original code returned an error on migration failure. Silently falling through to attempt opening a v6 file as v7 could lead to confusing downstream errors. The behavior change is intentional per the PR, but it should be explicitly noted in the commit message and the trade-off documented. A log_warn on failure would also help operators diagnose issues without being disruptive.

---

Minor / Nits

• The planning/root7_retirement_plan.md doc describes a .v7.root intermediate naming convention in several places (Categories 1C, 2A, etc.) — but the actual implementation uses .v6.root for the backup. The plan was written before the final design decision. The doc is useful for understanding the decision process, but the discrepancy between "plan" and "implementation" could confuse readers who open it expecting it to describe current behavior.

• INSTALL.md still shows the search order doc ending with "Note: .root7 paths are also checked for backward compatibility but are deprecated" — this is accurate but the .root7 backward-compat check only exists in ensure_database_v7, not in get_system_root_search_paths. The note could be more specific about where that fallback applies (auto-migration, not startup search paths).

---

Positive Highlights

• The atomic-ish rename sequence in migrate_internal (write temp, rename v6 to backup, rename temp to final, restore on failure) is a solid crash-safety improvement over the old approach.
• The crash recovery logic in ensure_database_v7 (detecting .v6.root backup + missing/truncated v7, then restoring) is thoughtful and correct.
• Removing the TOCTOU double-check in dbopenverb is the right call — ensure_database_v7 centralizes that logic.
• dbopenverb going from ~90 to ~40 lines is a genuine readability win.
• The dated TODO on the .root7 transitional fallback (TODO (2026-03-22)) gives future maintainers a clear signal.
• The sys.openUrl double-fork pattern to avoid zombie processes is correct and well-commented.

---

Summary

The main asks before merge:

1. Update tests/integration/test_cases/db_migration.yaml to remove stale .root7 references and correct the behavior description
2. Add at minimum smoke-test coverage for sys.openUrl in sys_processor_tests.yaml
3. Add planning/root7_retirement_plan.md to planning/INDEX.md

Items 4–7 are lower priority but worth addressing before this code goes into heavy rotation.

🤖 Generated with Claude Code

[claude]

PR 487 Review: Retire .root7 Extension

Overall this is a well-structured, well-documented cleanup. The atomic rename strategy for in-place migration is correct, the crash-recovery path in ensure_database_v7 is thorough, and the code reduction in dbopenverb (~90 to ~40 lines) is a clear improvement. A few issues to address:

---

Bug: In-place migration prints misleading output message

In frontier-cli/main.c, the --migrate in-place path calls:

printf("Migrated: %s -> %s\n", input, migration_output);

For in-place migration, migration_output equals input (both are the original .root path). The output reads "Migrated: Frontier.root -> Frontier.root" which looks like a no-op. The backup path is derived separately on a subsequent line.

Suggested fix: derive v6_backup first and make it the primary message:

printf("Migrated in-place: %s\n  v6 backed up to: %s\n", input, v6_backup);

---

sys.openUrl naming inconsistency

All other sys.* verbs are lowercase (osversion, systemtask, memavail, etc.). openUrl uses camelCase. If intentional, document this in the kernelverbs comment and docs. If not, consider openurl for consistency.

---

sys.openUrl missing integration test

The test plan item is unchecked. sys.openUrl has no integration test. A new verb that forks external processes needs at least a minimal smoke test (verify it returns boolean, does not crash with a well-formed URL) before merging.

---

ensure_database_v7: .root7 advisory log only fires in headless mode

The advisory log message encouraging users to rename legacy .root7 files is guarded by #if defined(FRONTIER_HEADLESS). Non-headless users with a lingering .root7 file will get the legacy path silently used with no guidance. If retiring .root7 is a goal, this warning should be unconditional or user-visible.

---

TESTING_GUIDE.md clean migration workflow is a no-op on v7 databases

The guide now says to run ./frontier-cli/frontier-cli --migrate databases/Frontier.root as the clean migration step. But databases/Frontier.root is already v7 (committed in this PR), so --migrate prints "Already v7 format" and does nothing. The guide should reference tests/fixtures/v6/Frontier.root as the v6 source, or clarify that the committed database is already v7 and no migration step is needed.

---

Minor: backup path re-derived in main.c instead of using the API

After ensure_database_v7 fills migration_output, main.c re-derives the .v6.root backup path using inline strrchr logic that duplicates derivation already inside migrate_internal. If the naming convention changes in db_format.c, main.c will not track it automatically. Consider a second output parameter or a cross-reference comment.

---

Things done well

• Atomic rename sequence in migrate_internal: write to .v7.tmp, rename v6 to .v6.root, rename temp to final. Best-effort restore on temp-rename failure is correct.
• Crash recovery in ensure_database_v7: detects incomplete migration and restores cleanly before re-migrating.
• dbopenverb simplification: removing the TOCTOU double-check and interleaved .root7 fallback makes the code significantly more readable.
• Makefile dist target: replacing migrate-then-rename with direct cp is correct now that source databases are already v7.
• .root7 backward compat: recognizing but not promoting .root7 with a dated TODO is the right balance.
• Double-fork for sys.openUrl: correct zombie-avoidance pattern; safe in the GIL context since only async-signal-safe functions (fork, execlp, _exit) are called between fork and exec/exit.

---

Verdict: The migration runtime work (db_format.c, dbverbs.c, main.c) is solid and can merge with the in-place output message bug fixed. sys.openUrl needs at least one integration test before merging. The doc/guide issues should be addressed in this PR since it is explicitly a doc cleanup.

[claude]

PR #487 Code Review: Retire .root7 Extension

Overall this is a well-structured, clearly-motivated cleanup. The atomic rename strategy for in-place migration is crash-safe and the crash recovery logic in ensure_database_v7 is solid. A few items to address before merge:

---

Issues

1. Scope creep: sys.openUrl and string.isValidUrl are unrelated features

This PR adds a new kernel verb (sys.openUrl) registered in kernelverbs.r / kernelverbs.rc, plus string.isValidUrl tests. Neither has anything to do with retiring the .root7 extension. These should ship in their own PRs so:

• The .root7 retirement stays reviewable on its own merits
• The new verbs get proper independent test sign-off
• Bisect is easier if a new verb causes a regression

Recommendation: Extract sys.openUrl and string.isValidUrl into a separate branch and PR before merging this one.

2. Integration tests not verified (test plan checkbox unchecked)

The PR test plan leaves cd tests && make test-integration unchecked. Given the number of files touched (31 test/tool files), this is the most important checkbox. The table_operations_integration failures are noted as pre-existing, but other integration tests should be confirmed green before merge.

3. drop_cancoon parameter is now dead code

After this PR, migrate_internal is called with drop_cancoon = true in all three call sites:

boolean migrate_32bit_to_64bit(const char *db_path) {
    return migrate_internal(db_path, true, NULL);   // always true
}
boolean migrate_32bit_to_64bit_to_output(const char *db_path, const char *output) {
    return migrate_internal(db_path, true, output); // always true
}
// ensure_database_v7 also calls migrate_internal(db_path, true, NULL)

The false path is dead. Consider removing the drop_cancoon parameter entirely (or at minimum adding a /* TODO: remove dead parameter */ comment) so it doesn't mislead future readers.

4. db_format_clear_last_migration_output_path — verify callers updated

db_format_clear_last_backup_path was renamed to db_format_clear_last_migration_output_path in the header and implementation, but I don't see a corresponding grep in the test plan. If any caller site uses the old name and wasn't caught by the diff (e.g., an out-of-tree tool, a test file not shown), it would silently compile if the old symbol is still exported somewhere or fail to link. Worth running grep -rn "db_format_clear_last_backup_path" across the full repo to confirm no stale callers.

5. last_migration_output_path is global, not thread-local

The existing last_backup_path (now last_migration_output_path) is a plain static char[]. If migration is always single-threaded this is fine — but given the GIL-based threading model (ADR-014), it would be cleaner as _Thread_local to match g_db_format_runtime_initialized. Low risk, but worth noting for consistency.

6. ensure_database_v7: restore branch doesn't verify the v6 backup is actually v6

In the crash-recovery path:

// .root is missing or not valid v7 — restore v6 backup
if (rename(v6_backup_path, db_path) == 0) {
    /* Fall through to re-migrate below */
}

If somehow .v6.root contains v7 data (e.g., user renamed files manually), the re-migration below will fail (or silently produce garbage). A cheap guard would be reading the version byte from v6_backup_path before restoring it. Low risk in practice but easy to add.

---

Minor / Nits

Headless-only advisory log for .root7 fallback:

#if defined(FRONTIER_HEADLESS)
    log_info(LOG_COMP_DB, "ensure_database_v7: found legacy .root7 file...");
#endif

The TODO comment acknowledges this, but GUI users hitting a .root7 file get no feedback at all. This is acceptable given the deprecation window, but the TODO should include a tracking issue number so it doesn't get forgotten.

backup_path[0] != '\0' && strcmp(output_path, db_path) == 0 condition:
This guard is correct and the comment explains it well. One edge case: if db_path and output_path are the same path string but differ in canonical form (e.g., symlinks, trailing slash), strcmp could miss the match. In practice this won't happen with --migrate inputs, but worth acknowledging.

Test 8 in test_migrate_flag.sh:
The test now correctly hashes the .v6.root backup rather than the original path. Good fix. The variable names (src_hash_before / src_hash_after) are slightly confusing since they no longer refer to the same file — consider renaming to original_hash / backup_hash for clarity.

MAX_SEARCH_PATHS reduced from 8 to 5:
The constant is 5, but the diff shows 4 search paths in get_system_root_search_paths. Either update the constant to 4 or add the fifth path to match.

---

Strengths

• The two-phase atomic rename (v6→backup, temp→final) with rollback on failure is the right approach for crash safety.
• db_format_derive_v6_backup_path as a single source of truth for backup naming is a good abstraction.
• Crash recovery in ensure_database_v7 (detect incomplete migration via .v6.root + header check) is thorough and well-commented.
• dbopenverb simplification (90→40 lines) is a real improvement.
• Bisect scripts now clean up both legacy artifacts (.root7 and .v6.root). Good defensive hygiene.
• The transitional .root7 fallback in ensure_database_v7 is appropriately dated with a removal TODO.

🤖 Generated with Claude Code

[claude]

Code Review: PR #487 — Retire .root7 extension

Overall this is a clean, well-motivated simplification. The in-place atomic rename strategy is correct, the dbopenverb simplification is a real improvement, and the drop_cancoon field removal eliminates dead state. A few things worth considering:

---

Behavioral change in dbopenverb (worth confirming intent)

Before: migration failure blocked the open (return false).
After: ensure_database_v7 failure logs a warning and falls through to attempt opening as-is.

} else {
    log_warn(LOG_COMP_DB, "dbopenverb: ensure_database_v7 failed for %s, attempting to open as-is", cpath);
}

If ensure_database_v7 fails mid-migration (e.g. disk full, crash), the v6 file may have already been renamed to .v6.root and the .root path may not exist or be corrupt. Opening as-is in that state could produce confusing errors downstream rather than a clean failure message. Is this intentional leniency, or should a migration failure still block the open?

---

--output mode bypasses ensure_database_v7 with duplicated v7-check logic

In main.c, the --output path inlines its own v7-check:

FILE *fp_check = fopen(input, "rb");
tydatabaserecord hdr;
boolean hdr_ok = fread(&hdr, sizeof hdr, 1, fp_check) == 1;
if (hdr_ok && hdr.versionnumber >= 7) {
    printf("Already v7 format: %s\n", input);
    return 0;
}

This is the same logic that ensure_database_v7 already handles. The parallel code path is minor, but it means the --output mode skips the transitional .root7 fallback, skips crash recovery, and skips the .v6.root detection that ensure_database_v7 does. Also worth noting: versionnumber in legacy headers is big-endian; a direct >= 7 comparison is fragile on little-endian hosts unless this field happens to be at an offset where the byte ordering doesn't matter. The pre-existing code in ensure_database_v7 has the same pattern, so this may be a known accepted issue — worth confirming.

---

Comment inaccuracy in ensure_database_v7

/* Conservative check: reserves 9 bytes for ".v6.root\0" (the longest
 * suffix we append). */
if (strlen(db_path) >= sizeof(v6_backup_path) - 9) {

.v6.root is 7 chars + 1 null = 8 bytes. The reserved margin of 9 is slightly over (harmless), but the comment should say 8 to avoid confusion. The actual check uses - 9 which is fine as a conservative guard, just the comment is off by one.

---

Crash recovery in ensure_database_v7 opens a subtle TOCTOU window

The recovery logic is:

1. Detect .v6.root backup exists
2. Check if .root is valid v7
3. If not: check if backup is valid v6
4. If yes: restore backup → fall through to migrate

Between steps 2 and 4, another process could write to .root. This is a very narrow window and likely not a real concern in practice, but worth acknowledging. More practically: if the process crashes between the v6→backup rename and the temp→final rename (leaving .root absent), step 2 fails (fopen returns NULL), and the code falls through to restore correctly. Good edge case coverage.

---

Restore failure silently returns false without cleanup note

In the recovery path:

if (rename(v6_backup_path, db_path) == 0) {
    // fall through to re-migrate
} else {
    log_error(...);
    return false;
}

When rename(backup → db_path) fails, the function returns false leaving the backup at .v6.root and .root absent or invalid. The caller (dbopenverb) logs a warning and tries to open as-is. There's no user-visible message explaining that the database is in a partially-migrated state and how to recover. A more explicit error message here (or in dbopenverb) would help users who hit this path.

---

Minor: migration_output buffer in main.c is never used when migrated == false

char migration_output[CLI_MAX_PATH_LENGTH + 16];
...
if (!ensure_database_v7(input, &migrated, migration_output, sizeof(migration_output))) { ... }
if (!migrated) {
    printf("Already v7 format: %s\n", input);
    return 0;
}
{
    char v6_backup[CLI_MAX_PATH_LENGTH + 16];
    db_format_derive_v6_backup_path(input, v6_backup, sizeof(v6_backup));
    printf("Migrated in-place: %s\n", input);
    printf("  v6 backed up to: %s\n", v6_backup);
}

migration_output is passed to ensure_database_v7 but its value is never actually read/printed. The print derives the backup path independently via db_format_derive_v6_backup_path. This is fine (the paths agree), but the migration_output buffer is dead — consider removing it or using it directly in the output message.

---

Test coverage gap

The crash recovery path in ensure_database_v7 (.v6.root exists but .root is absent or not-yet-v7) isn't covered by any of the migration tests referenced in the test plan. The 17 migration tests confirm the happy path; a test that simulates an interrupted migration (rename v6→backup, leave temp) would give confidence in the recovery logic.

---

What's working well

• Atomic rename sequence in migrate_internal is correct — write to .v7.tmp, rename v6→backup, rename temp→final, with best-effort rollback on final-rename failure.
• db_format_derive_v6_backup_path as single source of truth for backup naming is good.
• dbopenverb simplification is a genuine readability win.
• drop_cancoon removal eliminates a boolean that was always true on the active code path.
• The dated TODO on the .root7 transitional fallback is the right approach.

🤖 Generated with Claude Code

[claude]

Code Review: Retire .root7 Extension

This is a well-executed cleanup. The overall direction is right and the crash-safe rename strategy is solid. A few things worth addressing before merge.

ISSUES

1. Silent overwrite of existing .v6.root backup

In migrate_internal, the rename(db_path, backup_path) call on POSIX silently overwrites an existing .v6.root file. If something caused a partial state where ensure_database_v7 falls through to migrate_internal with a pre-existing .v6.root, the original backup is silently destroyed. Consider logging a warning or refusing unless --force is set.

2. ~/.frontier/Frontier.root dropped from search paths

The old 8-path search list included ~/.frontier/Frontier.root. The new 4-path list drops it. This is a silent breaking change for Linux users who followed previous GETTING_STARTED instructions pointing at ~/.frontier/. The INSTALL.md uninstallation section still references this location, but the runtime no longer discovers it automatically.

3. --force flag silently ignored in in-place mode

The --force check in main.c only applies when output_path is not NULL. In in-place mode (no --output), --force does nothing but no warning is issued. Worth documenting in print_usage.

4. Integration tests not verified

The test plan leaves "cd tests && make test-integration" unchecked. Since this PR touches dbopenverb auto-migration, getodbparam, and db.open/db.new codepaths, integration tests are the primary verb-level coverage. Worth completing before merge.

OBSERVATIONS (no action needed)

Crash recovery logic is well-designed. The ensure_database_v7 recovery path (detect partial migration, verify backup is valid v6, restore, re-migrate) is thoughtful. The three-file pattern (write temp, rename v6 to backup, rename temp to final) is correctly implemented.

dbopenverb simplification is excellent. Went from 90 to 40 lines by removing the fragile TOCTOU double-check and scattered .root7 fallback code. The old TOCTOU comment acknowledged a race but the fix was incomplete anyway; delegating to ensure_database_v7 is the right call.

drop_cancoon field removal is clean. Since all migrations now unconditionally drop Cancoon, the boolean field was dead weight. Removing it simplifies struct initializers throughout.

Single source of truth for backup path naming. db_format_derive_v6_backup_path as a shared helper is correct -- the old code had the naming logic duplicated across migrate_internal and callers.

TODO comment for .root7 fallback has a date, which is good. Would be stronger with a linked issue.

SUMMARY

The core migration strategy is sound and the dbopenverb simplification is a real improvement. The silent backup overwrite risk, dropped ~/.frontier search path, and incomplete integration test verification are the items worth resolving before merge.

Generated with Claude Code

[claude]

---

Code Review: PR 487 - Retire .root7 Extension

Overall this is a clean, well-structured change. The atomic-ish rename sequence (write temp -> rename v6 -> rename temp-to-final) is a sound approach, and the crash-recovery path in ensure_database_v7 is a nice defensive addition. The simplification to dbopenverb (~90 to ~40 lines) is a real improvement in readability.

A few issues worth addressing before merge:

---

Issue 1: ensure_database_v7 duplicates backup path derivation (DRY violation)

db_format_derive_v6_backup_path is documented as "the single source of truth for backup naming", but ensure_database_v7 derives the backup path inline using its own strrchr/snprintf instead of calling the helper. The logic happens to match, but having two independent derivations contradicts the stated intent and risks divergence. ensure_database_v7 should call db_format_derive_v6_backup_path directly.

---

Issue 2: _Thread_local for last_migration_output_path - subtle threading concern

The variable was previously a plain global. Making it _Thread_local means the migration result is only readable from the thread that set it. Under the GIL model this is safe - current callers all read the value immediately after migrating in the same call stack. But if future code ever calls migration on a worker thread and reads the result on the main thread, it will silently get an empty string. A brief comment explaining the GIL-based reasoning would prevent the next reader from re-questioning this.

---

Issue 3: Backup file opened twice in ensure_database_v7

v6_backup_path is fopened once just to test existence (fclose immediately), then re-opened later to read the header. Reading the header on the first open would eliminate the redundant syscall and the existence-then-open TOCTOU window.

---

Issue 4: Missing strerror(errno) when v6->backup rename fails

In migrate_internal, the rename error code is discarded when rename(db_path, backup_path) fails. The cleanup block logs fail_step but not errno. If the rename fails (cross-device link, permissions, etc.) the user gets no diagnostic. A log_error with strerror(errno) before the goto cleanup would help.

---

Issue 5: No recovery guidance when restore-after-failure also fails

When rename(temp_path, output_path) fails and the best-effort rename(backup_path, db_path) also fails, the user is left with Frontier.v6.root (the v6 backup) and Frontier.root.v7.tmp (the completed v7 file), but no message explaining the state or how to recover. The ensure_database_v7 crash-recovery path has good diagnostics; this failure path deserves at least a log_error with manual recovery guidance.

---

Minor: planning doc status and naming are stale

planning/root7_retirement_plan.md is marked "Status: Planning" but implementation is complete. The doc also describes the output extension as .v7.root throughout, while the actual implementation uses .v6.root for the backup and .root for the v7 output - the inverse of what the doc describes. Correcting the status and naming would prevent confusion for future readers.

---

Positive callouts:

• dbopenverb simplification is clean and correct - delegating fully to ensure_database_v7 eliminates the old TOCTOU complexity entirely.
• The incremental rename sequence with restore-on-failure is well thought out and the crash-recovery path in ensure_database_v7 is explicitly tested.
• Removing the drop_cancoon field from db_format_mode is good cleanup - the struct was carrying dead weight.
• odb_ensure_root_extension correctly handles .root7 -> .root conversion for backward compat without promoting the old naming.
• The --force help text update (clarifying it only applies with --output) is the right level of transparency for a behavioral change.

Generated with Claude Code

[claude]

test comment to verify posting works

[claude]

Code Review: PR 487

Overall: Well-scoped and technically solid. The in-place rename strategy (v6 backed up to .v6.root, v7 at original .root path) is the right call. The crash-safe atomic rename sequence is a meaningful improvement over what was there before.

---

Correctness Issues (confirm before merge)

1. migrate_internal CRITICAL path / temp file cleanup (db_format.c) -- When the v6 restore fails after a failed final rename, the CRITICAL branch does not explicitly call remove(temp_path). It relies on falling through to goto cleanup. Confirm this path reaches that cleanup block; if not, add an explicit remove(temp_path). The CRITICAL log message should also include the temp file path.

2. ensure_database_v7 control flow ordering (db_format.c) -- After crash recovery (restoring .root v6 from .v6.root backup), the code falls through to the legacy .root7 fallback check before the fresh migration. If a .root7 file happens to exist, the function returns true pointing at .root7 while a freshly-restored v6 .root is also on disk awaiting re-migration. Consider skipping the .root7 fallback after crash recovery, or add a comment explaining this ordering is intentional.

3. dbopenverb error message inaccuracy (dbverbs.c) -- ensure_database_v7 can fail for reasons unrelated to migration (unreadable header, permission error on .v6.root sibling, etc.). The new code shows 'migration to v7 failed' even when the database was already v7 and the failure was a header read error. The log message should distinguish 'migration failed' from 'version check failed'.

---

Design Observations (lower risk)

4. Naming mismatch: create_root_backup writes last_migration_output_path (db_format.c) -- create_root_backup writes the backup path to last_migration_output_path, while migrate_internal writes the v7 output path to the same variable. These are semantically different. A clarifying comment or rename would reduce confusion.

5. migrate_32bit_to_64bit is now destructive (db_format.h) -- This function previously wrote .root7 without touching the input. It now renames the v6 original to .v6.root. The callers in odbengine.c (odbOpenFile) and db_migrate_reopen_if_legacy in dbverbs.c should be audited; the odbOpenFile path appears correct but deserves a comment in the header noting the in-place behavior.

6. db_format_derive_v6_backup_path can silently truncate near 1024-byte path limit -- snprintf return value is not checked against backup_size. A path near 1024 bytes produces a silently truncated backup path and the function still returns true. Check the snprintf return value and return false on overflow.

---

Minor

7. planning/root7_retirement_plan.md -- The note at the top acknowledges the naming divergence from .v7.root to .v6.root backup strategy. However, the BEFORE/AFTER code blocks in Categories 1C, 1E, 1F, and 2A still show .v7.root. Mark those sections as superseded or update them to match what was actually implemented.

8. --force without --output prints to stderr (frontier-cli/main.c) -- 'Note: --force has no effect in in-place migration mode' goes to stderr. Scripts checking stderr for errors may treat this as a warning. Consider stdout or silent no-op.

9. tests/fixtures/v6/Frontier.root binary -- Adding a genuine v6 fixture is correct. The test updates in save_migration_tests.c and test_migrate_flag.sh look right.

---

Positive observations

• Atomic-ish rename sequence (write temp, rename v6 to backup, rename temp to final, rollback on failure) is correct and much safer than before.
• ensure_database_v7 crash recovery logic is thorough.
• db_format_derive_v6_backup_path as single source of truth for backup naming is the right pattern.
• Removing drop_cancoon from db_format_mode is the right simplification.
• dbopenverb is genuinely cleaner (~40 lines vs ~90 lines).
• Tests moved to use tests/fixtures/v6/Frontier.root instead of live databases/Frontier.root -- the old approach was a latent correctness issue.
• shellsysverbs.c comments on why hurl is not freed in child process are a welcome addition.

Summary: Core design and crash-safety reasoning are correct. Items 1-3 are worth confirming before merge. Items 4-6 are lower risk. Item 7 is planning-doc housekeeping.

Generated with Claude Code

[claude]

Code Review: PR #487 — Retire .root7 Extension

Overall

This is a well-executed simplification. Moving from a two-phase .root7 transitional scheme to in-place migration is the right long-term decision, and the crash recovery logic (check for .v6.root + .root pair on startup) is thorough. The dbopenverb simplification from ~90 to ~40 lines is a clear win. A few things worth discussing:

---

Issues

1. last_migration_output_path serves two distinct roles — naming is misleading

File: Common/source/db_format.c

The comment on the variable explicitly acknowledges this:

create_root_backup() stores the backup path; migrate_internal() stores the v7 output path. Callers should read it immediately after the operation that set it.

This is a race-prone design. After create_root_backup() runs, the variable holds a backup path — but db_format_last_migration_output_path() is the accessor, which doesn't communicate that at all. The renamed API (db_format_last_migration_output_path vs. old db_format_last_backup_path) actually makes this more confusing because create_root_backup() still writes to it with semantically different content.

In test_backup_creation:

TEST_ASSERT(db_format_last_migration_output_path(backup_path, sizeof backup_path), "Backup path should be recorded");

This test retrieves the backup path via a function named for "migration output" — the test message says "Backup path should be recorded" which contradicts the function name. It works, but it signals that the abstraction isn't clean.

Suggestion: Either (a) keep two separate variables (last_backup_path for create_root_backup, last_migration_output_path for migrate_internal), or (b) remove create_root_backup from public API and ensure only migrate_internal writes to the shared slot. The dual-write is a footgun if these functions are ever called in sequence by a future caller.

---

2. Unreadable backup header treated as "safe to overwrite"

File: Common/source/db_format.c, migrate_internal()

FILE *fp_existing = fopen(backup_path, "rb");
if (fp_existing) {
    tydatabaserecord existing_hdr;
    boolean hdr_ok = fread(&existing_hdr, sizeof existing_hdr, 1, fp_existing) == 1;
    fclose(fp_existing);
    if (hdr_ok && existing_hdr.versionnumber < 7) {
        log_error(..., "refusing to overwrite existing v6 backup: %s", backup_path);
        ...
        goto cleanup;
    }
    /* If existing backup is v7 or unreadable, overwrite is safe */
    log_info(..., "overwriting non-v6 backup: %s", backup_path);
}

If hdr_ok is false (file exists but header can't be read — e.g., zero-byte file, I/O error, truncated write from a previous crash), the code logs "overwriting non-v6 backup" and proceeds. This silently destroys a potentially valid-but-unreadable v6 backup.

A safer approach: treat an unreadable backup header as an ambiguous state and refuse to proceed (or at minimum log a warning-level message rather than info-level).

if (!hdr_ok) {
    log_error(LOG_COMP_DB, "migrate_internal: cannot read backup header at %s — refusing to overwrite", backup_path);
    ok = false;
    goto cleanup;
}

---

3. ensure_database_v7 control flow — double-brace block

File: Common/source/db_format.c

if (!did_crash_recovery) {
/* Transitional fallback: check for legacy .root7 file ... */
{
    char legacy_root7_path[1024];
    ...
}
} /* !did_crash_recovery */

The double-brace construct (outer if + inner scope block) suggests this was wrapped around existing code. The inner {} block's scope doesn't add anything since legacy_root7_path could just live in the outer if block. This is cosmetic but worth cleaning up since the control flow here is already complex with the nested backup-check logic above.

---

4. Integration tests unchecked in the test plan

The PR test plan has two unchecked items:

• cd tests && make test-integration — integration tests
• Manual: ./frontier-cli --migrate tests/fixtures/v6/Frontier.root produces .v6.root backup + v7 at .root

The second one is testable via make test-migrate-flag (which passes 17/17), but the first is left unverified. Given that dbopenverb and getodbparam were significantly refactored, it's worth confirming the integration tests pass before merging, even if the table_operations_integration failures are pre-existing.

---

5. Minor: DEFAULT_SYSTEM_ROOT defined but not used in search paths

File: frontier-cli/main.c

#define DEFAULT_SYSTEM_ROOT "databases/Frontier.root"

This constant isn't referenced in get_system_root_search_paths() — the search paths are hardcoded inline (lines like snprintf(paths[count], ..., "%s/Frontier.root", cwd)). The old DEFAULT_SYSTEM_ROOT_V7 was similarly unused inline. Consider either removing the constant or actually using it in the search path construction to avoid drift.

---

Positive Notes

• The atomic-ish rename sequence (rename v6→backup, rename temp→final, with best-effort rollback) is the right approach for crash safety. The error message in the double-failure case (both temp→final AND backup restore fail) is particularly good — it tells the operator exactly what to do manually.
• db_format_derive_v6_backup_path() as a single source of truth for backup naming is correct — having this in both migrate_internal() and main.c previously would have been a maintenance hazard.
• Removing the TOCTOU race mitigation code from dbopenverb (the double-check for .root7 between detection and migration) is correct — the in-place approach eliminates the race class entirely since there's no new file name being introduced.
• The .root7 transitional fallback in ensure_database_v7 with a dated TODO is the right level of backward compatibility.

🤖 Generated with Claude Code

[claude]

PR 487 Review: Retire .root7 Extension

Good PR overall - the migration logic is substantially cleaner and the crash-safety design is solid.

Strengths

Atomic-ish rename sequence in migrate_internal: The three-step sequence (write to .v7.tmp, rename v6 to .v6.root, rename temp to final) is the right approach. The rollback path (restore .v6.root on temp-to-final failure) is correctly implemented. The CRITICAL log message in the dual-failure case includes explicit recovery instructions.

Single source of truth for backup naming: db_format_derive_v6_backup_path eliminates the previous pattern of computing the same name in multiple places. The bounds-checking (written >= backup_size) is correct.

dbopenverb simplification: Reducing ~90 lines to ~40 by delegating to ensure_database_v7 is a real improvement. The TOCTOU double-check present before is now irrelevant under the in-place model, and removing it is correct.

drop_cancoon removal: Always dropping Cancoon in v7 migration is the right call now that Phase 1 is complete. Removing the conditional branch cleans up a long-standing complexity.

Issues to Address

1. dbopenverb failure path behavioral change (medium)

Previously, if odb_detect_database_version failed (e.g., file does not exist), dbopenverb logged and continued without returning an error -- the open attempt would simply fail later. Now, ensure_database_v7 returns false for any unreadable file, and dbopenverb immediately returns false with langerrormessage.

This changes behavior for scripts that call db.open(path) on a path that may not exist yet and handle the failure differently. Worth verifying this does not break any existing UserTalk scripts that relied on the old failure mode. A grep for integration tests calling db.open on non-existent paths would confirm.

2. Crash recovery fallthrough when backup is v7 (minor)

In ensure_database_v7, when .v6.root exists but has versionnumber >= 7, the fallthrough reaches fopen(db_path). But db_path may not exist (the original v6 was renamed to backup and the v7 write never completed). Migration will fail gracefully but the error log will not mention the .v6.root context, making diagnosis harder. Consider logging the backup path before falling through.

3. Integration tests unchecked in test plan

The integration test item is unchecked in the test plan. This is the primary test for UserTalk behavior including db.open and db.new. The PR should not merge with this unchecked given that both dbopenverb and dbnewverb changed. Please complete and confirm.

4. --force semantics with in-place migration

The note about --force having no effect in in-place mode prints to stdout (not stderr). If a user passes --force expecting it to overwrite an existing .v6.root backup, they will be surprised. Worth documenting in --help output that --force only applies when --output is specified.

5. Error message in dbopenverb is overly generic

The "Can't open the database because it could not be verified as v7 format" message fires for any ensure_database_v7 failure: file not found, header unreadable, migration failed, etc. The previous code had more specific messages per branch. Not a blocker, but worth noting for future UX improvement.

Minor Notes

• The dated TODO (2026-03-22) for removing .root7 fallback is good practice. Consider tracking in a GitHub issue so it does not get lost.
• The _Thread_local comment on last_migration_output_path correctly references ADR-014 and explains the threading assumption. Good documentation practice.
• The .root7 fallback removal in getodbparam is correct under the new model. Given this PR depends on PR 486 migrating source databases first, there should be no in-memory .root7 handles at this point.
• Binary database files committed -- presumably pre-migrated v7 databases from PR 486. Worth confirming they were produced by the migration tool rather than hand-edited.

Summary

The design is sound and the implementation is careful. The main asks before merging:

1. Run and confirm integration tests pass
2. Verify dbopenverb failure path change does not break existing UserTalk scripts that handle open failures

Generated with Claude Code