feat: Startup scripts and path-based file verbs

[jsavin]

Summary

This PR enables startup scripts to run by default in headless mode and refactors file verbs to use path-based semantics matching legacy Frontier behavior.

Key Changes

• Startup scripts run by default - system.startup scripts now execute automatically in headless mode
• --skip-startup flag - Added CLI flag to disable startup script execution for debugging
• Path-based file API - Refactored file.open, file.read, file.write, file.close etc. to use path-based lookups instead of refnum-based (matching legacy Frontier semantics)
• CWD system root discovery - CLI now defaults to looking for system root in the current working directory
• 128KB copy buffers - Increased file copy buffer from 8KB to 128KB for better I/O performance

File Verb Semantics Change

Legacy Frontier uses path-based file handles:

file.open("/path/to/file")
local(data = file.read("/path/to/file", 100))  // path, not refnum
file.close("/path/to/file")

This PR implements path-based lookups in the kernel to match this behavior.

Test Results

File verb integration tests improved from 7 passed (66 failed) to 71 passed (7 failed) - a 10x improvement.

Test plan

• make -C frontier-cli builds successfully
• File verb integration tests pass (71/81, was 7/81)
• Startup scripts execute with --system-root dist/Frontier.root7
• --skip-startup flag works correctly

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[claude]

Code Review - PR #382

Critical Issues ⚠️

1. BLOCKER: Corrupted parameter in file.read() return value (portable/fileverbs_portable.c:1530)

return setbinaryvalue(hdata, '\?\?\?\?', vreturned);

Problem: The second parameter to setbinaryvalue() is corrupted/malformed. This should be an OSType (4-character code) or a size value.

Expected: Based on readwholefilefunc at line 1667 and the function signature boolean setbinaryvalue(Handle, OSType, tyvaluerecord *), this should likely be:

return setbinaryvalue(hdata, bytesread, vreturned);

Impact: This will cause compilation errors or runtime crashes. Tests cannot pass with this bug.

Action Required: Fix immediately before merge.

---

2. C89 Compliance Issue: Mixed declarations and code

Multiple instances of variables declared after statements (C99 style), which violates C89 standard used by this project:

• fileverbs_portable.c:1137: boolean success = open_file_by_path(path); comes after if statements
• fileverbs_portable.c:1158: boolean success = close_file_by_path(path); comes after if statements

Problem: The codebase appears to use C89 standard. These will fail compilation on C89-strict compilers.

Fix: Move all variable declarations to the top of their scope blocks:

case openfilefunc: {
    tyfilespec fs;
    char path[4096];
    boolean success;  /* Declare at top */
    
    flnextparamislast = true;
    
    if (!getfilespecvalue(hparam1, 1, &fs))
        return false;
    
    if (!filespec_to_cstring(&fs, path, sizeof(path)))
        return false;
    
    success = open_file_by_path(path);  /* Assign after control flow */
    return setbooleanvalue(success, vreturned);
}

---

Code Quality Issues

3. Documentation Removal Without Justification

The PR removes critical documentation about macOS sandbox /tmp restrictions from both CLAUDE.md and docs/TEST_STATUS_SUMMARY.md.

Concern: This appears unrelated to the PR's stated purpose (startup scripts and file verbs). If sandbox restrictions have been lifted, this should be:

1. Explicitly mentioned in the PR description
2. Explained in commit messages
3. Verified that all test infrastructure still works

Question: Why were these restrictions removed? Has the sandbox behavior changed?

---

4. Thread Safety in Path-Based File Operations

The refactored file table uses path-based lookups with case-insensitive comparison:

static void normalize_path(const char *src, char *dst, size_t dstsize) {
    size_t i;
    for (i = 0; i < dstsize - 1 && src[i]; i++) {
        dst[i] = tolower((unsigned char)src[i]);
    }
    dst[i] = '\0';
}

Observations:

• ✅ Good: Uses (unsigned char) cast to avoid undefined behavior with negative chars
• ⚠️ Concern: Path normalization happens multiple times per operation (open, read, write, close). Consider caching normalized paths in the file table entry
• ⚠️ Race condition potential: In open_file_by_path() (lines 368-435), there's a lock release between checking for free slots and opening the file, then re-acquiring the lock. Another thread could steal the slot. The code does handle this with a re-check, but this is complex and error-prone.

Suggestion: Consider holding the lock across the entire operation or using a reservation mechanism.

---

5. Buffer Size Increase Without Justification

char buffer[131072];  /* 128KB - matches modern copy utilities */

Changed from 8KB to 128KB for file copy operations.

Concern:

• 128KB stack allocation is substantial
• Could cause stack overflow on systems with small stack limits
• No benchmarks or justification provided for 16x increase

Recommendation:

• Either provide benchmarks showing improvement
• Or use heap allocation for large buffers
• Or add a comment explaining the rationale beyond "modern copy utilities"

---

6. file.read() Infinity Handling

/* Handle infinity (or very large count) - read remaining bytes in file */
if (count >= 0x7FFFFFFF) {

Issue: The comment says "infinity" but checks against 0x7FFFFFFF (max int32). In UserTalk, infinity might be represented differently.

Questions:

• What is the actual representation of infinity in UserTalk?
• Is it LONG_MAX, LLONG_MAX, or a special value?
• Should this use a defined constant like LONG_MAX instead of magic number?

---

Positive Aspects ✅

1. Excellent test coverage: Integration tests improved from 7/81 to 71/81 - this is outstanding
2. Good error messages: "File not open - call file.open first" provides clear guidance
3. Proper path-based semantics: Matches legacy Frontier behavior as documented
4. Startup script default change is well-documented: The PR clearly explains the new default behavior and provides the --skip-startup flag
5. Reference counting: The refcount mechanism prevents race conditions where FILE* is closed while in use

---

Security Considerations

1. Path traversal: Path-based file access should validate paths don't escape intended directories (though this might be handled elsewhere)
2. File handle exhaustion: MAX_OPEN_FILES limit exists, but no rate limiting or per-user quotas
3. Large file safety: 500MB limit on file.read() is reasonable for DoS prevention

---

Test Coverage

The integration tests look comprehensive and well-structured:

• ✅ Sequential reads maintain position
• ✅ Infinity reads remaining bytes
• ✅ Error cases (unopened files)
• ✅ Multiple line ending formats (LF, CR, CRLF)

Missing: Tests for thread safety and concurrent file operations (may not be in scope)

---

Summary

Cannot merge until critical issue #1 (corrupted setbinaryvalue parameter) is fixed.

Should address C89 compliance issues (#2) and clarify documentation removal (#3) before merge.

Recommend addressing buffer size justification (#5) and infinity constant (#6) but not blockers.

Overall architecture and approach are sound. The path-based semantics refactor is well-executed and the test improvements are excellent.

---

Test Plan Verification Needed:

• Verify PR passes all tests after fixing issue Repo Modernization: 64‑bit/ARM + DB Header Rev; Headless/Portable Runtime; Headless EFP Routing for file.* and Tests #1
• Confirm C89 compliance with strict compiler flags
• Validate startup scripts behavior on actual databases

cc: @jsavin

[claude]

Code Review: PR #382 - Startup scripts and path-based file verbs

This PR makes two major changes: enabling startup scripts by default and refactoring file verbs to use path-based semantics. The implementation is generally solid with good test coverage, but there are several concerns that should be addressed.

---

🟢 Strengths

1. Excellent Test Coverage

• Integration tests improved from 7/81 (9%) to 71/81 (88%) passing - a 10x improvement
• New test cases cover path-based API thoroughly:
  • Sequential reads maintaining file position
  • infinity parameter for reading remaining bytes
  • Error cases (file not open)
  • Edge cases (empty files, large counts)

2. Good Documentation

• Comments in fileverbs_portable.c reference DocServer paths (docs/usertalk/docserver/file/open.txt)
• PR description clearly explains the semantic change
• Test cases include descriptive comments

3. System Root Discovery Enhancement

• CWD-first search order is sensible and well-documented
• Platform-specific executable directory detection (macOS/Linux)
• Clear fallback chain with priority ordering

4. Buffer Size Optimization

• 128KB copy buffers (up from 8KB) will reduce syscall overhead
• Comment explains reasoning (macOS default stack is 8MB)

---

🟡 Concerns & Issues

1. Thread Safety - Path-Based File Table ⚠️ CRITICAL

Location: portable/fileverbs_portable.c:118-185 (open_file_by_path)

Issue: Race condition in slot allocation:

pthread_mutex_unlock(&filetable_mutex);  // Line 150 - UNLOCK

/* Open file outside of lock to avoid blocking */
fp = fopen(path, "r+b");  // Line 153 - I/O without lock

pthread_mutex_lock(&filetable_mutex);  // Line 162 - RELOCK

/* Re-check slot is still free (another thread may have used it) */
if (filetable[i].inuse) {  // Line 165 - slot may be taken!

Problem:

1. Thread A finds slot 3 free at line 143
2. Thread A unlocks mutex at line 150
3. Thread B finds slot 3 free (still marked inuse=false)
4. Thread B opens file and marks slot 3 inuse=true
5. Thread A opens file and tries to use slot 3
6. Thread A's re-check at line 165 finds slot taken, searches for another slot
7. If all slots full, Thread A leaks the FILE* opened at line 153 (no fclose in this path)

Fix Required:

• Store the slot index before unlocking
• If slot is taken after relock, close the FILE* and retry from scratch
• Or simplify: use a two-phase approach with a "reserved" flag

Example Fix:

if (filetable[i].inuse) {
    fclose(fp);  // ← ADD THIS to prevent leak
    // Find another free slot...

---

2. Path Normalization - Case Sensitivity ⚠️ MODERATE

Location: portable/fileverbs_portable.c:105-111 (normalize_path)

Issue: Forces lowercase comparison on case-sensitive filesystems:

dst[i] = tolower((unsigned char)src[i]);

Problem:

• On Linux/Unix, /tmp/File.txt and /tmp/file.txt are different files
• Current implementation treats them as the same file (both normalize to /tmp/file.txt)
• Opening /tmp/File.txt then trying to read from /tmp/file.txt will succeed (wrong file!)

Impact:

• Silent data corruption if scripts use case-sensitive paths
• Breaks compatibility with legacy Frontier scripts on case-sensitive systems

Fix Required:

• Use platform-specific normalization:
  • macOS: lowercase (case-insensitive filesystem)
  • Linux: preserve case (case-sensitive filesystem)

#ifdef __APPLE__
    dst[i] = tolower((unsigned char)src[i]);
#else
    dst[i] = src[i];  // Preserve case on Linux
#endif

---

3. Startup Script Behavior Change ⚠️ MODERATE

Location: Multiple files (main.c:356, headless_scripts_loader.c:107)

Issue: Default behavior changed without migration path:

Before: Startup scripts skipped by default (opt-in with FRONTIER_HEADLESS_RUN_STARTUP=1)
After: Startup scripts run by default (opt-out with --skip-startup)

Impact:

• Existing automation scripts may break if startup scripts have side effects
• No deprecation period or warning in migration guide
• CLAUDE.md sections removed without explanation (lines 9-15 of diff)

Recommendation:

• Add migration note to docs/CLI_USAGE_GUIDE.md
• Consider a transition period with warnings
• Document why the default changed

---

4. File Handle Leak on Error ⚠️ MODERATE

Location: portable/fileverbs_portable.c:1513-1523 (readfunc)

Issue: Handle leak if fread returns 0 bytes:

if (!newhandle(bytes_to_read, &hdata)) {
    release_file_by_path(path);
    copyctopstring("Out of memory", bserror);
    return false;
}

lockhandle(hdata);
buffer = (unsigned char *)*hdata;
bytesread = fread(buffer, 1, bytes_to_read, fp);
unlockhandle(hdata);
release_file_by_path(path);  // ← Released BEFORE checking bytesread

if (bytesread == 0) {
    disposehandle(hdata);
    return setstringvalue(BIGSTRING("\x00"), vreturned);
}

Problem:

• If fread returns 0 (EOF or error), the path is already released
• But the FILE* might still be in use if another thread acquired it between fread and the check

Fix: Move release_file_by_path after all cleanup logic

---

5. Documentation Removal Without Explanation

Location: CLAUDE.md lines 219-228, docs/TEST_STATUS_SUMMARY.md lines 238-244

Issue: Removed macOS sandbox documentation:

• "macOS Sandbox /tmp Restriction" section deleted
• No explanation in PR description or commit message

Questions:

• Was the sandbox restriction lifted?
• Is this safe for production use?
• Should this be documented elsewhere?

---

6. Missing Error Handling

Location: frontier-cli/main.c:389-427 (get_system_root_search_paths)

Issue: getcwd and _NSGetExecutablePath failures set empty string but don't log:

if (getcwd(cwd, sizeof(cwd)) == NULL) {
    cwd[0] = '\0';  // ← Silent failure
}

Recommendation: Add debug logging to help troubleshoot auto-discovery failures

---

🔵 Minor Issues

7. Magic Number - USERTALK_INFINITY

Location: portable/fileverbs_portable.c:88

#define USERTALK_INFINITY 0x7FFFFFFF

Issue: Hardcoded to 32-bit max, but code uses long long (64-bit):

long long count;  // Line 1447
if (count >= USERTALK_INFINITY) {  // Line 1485

Impact: file.read(path, 0x7FFFFFFF + 1) will trigger infinity logic (probably intentional, but unclear)

Recommendation: Document why 32-bit threshold is used for 64-bit variable

---

8. Unused Variable Warnings

Location: portable/fileverbs_portable.c:1796

(void)bsext;  /* Reserved for future extension parsing */

Recommendation: Remove unused variable instead of silencing compiler

---

📋 Testing Recommendations

High Priority

1. Thread safety test: Concurrent file.open() calls to same path
2. Case sensitivity test: Open /tmp/File.txt and /tmp/file.txt on Linux
3. Startup script side effects: Ensure no regressions in existing automation

Medium Priority

4. Large file test: Read 500MB+ file with infinity parameter
5. Cross-volume move: Verify copy+delete fallback works
6. System root discovery: Test all search path branches

---

✅ Approval Recommendations

Do NOT merge until:

1. ✅ Thread safety race condition fixed (FILE* leak in open_file_by_path)
2. ✅ Path normalization uses platform-specific case handling
3. ✅ Migration guide updated for startup script behavior change

Nice to have:

• Add thread safety tests to integration suite
• Document USERTALK_INFINITY threshold choice
• Add debug logging for getcwd failures

---

🎯 Summary

Overall Assessment: Good implementation with two critical issues that must be fixed before merge.

Risk Level: MODERATE-HIGH

• Thread safety bug could cause file handle leaks in production
• Path normalization bug could cause silent data corruption on Linux

Recommendation: Request changes, re-review after fixes.

---

cc @user - Please address the thread safety and path normalization issues before merging. Happy to re-review once those are resolved!

[claude]

Code Review: PR #382 - Startup Scripts and Path-Based File Verbs

Summary

This PR makes two significant architectural changes: (1) enabling startup scripts by default in headless mode, and (2) refactoring file verbs to use path-based semantics instead of refnum-based handles. The implementation is well-executed with strong test coverage improvements (7→71 passing file verb tests).

✅ Strengths

1. Excellent Path Normalization Strategy

The path-based file API includes platform-aware path normalization (lines 407-414 in fileverbs_portable.c):

• macOS: case-insensitive comparison (matching APFS/HFS+ default behavior)
• Linux: case-sensitive comparison

This is exactly the right approach for cross-platform compatibility.

2. Strong Thread Safety

The refactored file table maintains proper reference counting and mutex protection:

• get_file_by_path() increments refcount under lock
• release_file_by_path() decrements refcount under lock
• FILE* closes are deferred outside the mutex to avoid I/O under locks

3. Clear Backward Compatibility Path

The CLI flag --skip-startup and FRONTIER_HEADLESS_RUN_STARTUP=0 provide escape hatches for users who need the old behavior during migration.

4. Comprehensive Test Coverage

The integration tests now cover:

• Sequential reads maintaining position
• Infinity constant for read remaining bytes
• Error cases (file not open)
• All major file I/O operations

[claude]

⚠️ Issues & Concerns

CRITICAL: Race Condition in open_file_by_path() 🔴

Location: fileverbs_portable.c:421-498

The function has a time-of-check to time-of-use (TOCTOU) vulnerability:

The code releases the mutex before calling fopen(), then reacquires it. Between these operations, another thread could:

1. Open the same file at the same path
2. Register it in the table
3. Start using it

When the first thread regains the lock, it will register the file a second time, creating:

• Duplicate FILE handles* for the same path
• Undefined behavior when both handles write to the file
• Data corruption risk

Fix Required: Reserve the slot BEFORE doing I/O (mark inuse=true, fp=NULL), so other threads see it as occupied even before fp is valid. Then open the file outside the lock, and finally install the FILE* handle under lock.

---

MEDIUM: Missing Range Check for Normalized Path 🟡

Location: fileverbs_portable.c:408

The normalize_path() function checks bounds but does not indicate when truncation occurs.

Recommendation: Add logging or return status when src is truncated to detect potential path corruption issues.

---

MEDIUM: Infinity Threshold is Too Restrictive 🟡

Location: fileverbs_portable.c:1150-1152

The threshold USERTALK_INFINITY is defined as LLONG_MAX (0x7FFFFFFFFFFFFFFF), but the code comment says: "we use a 32-bit threshold since any count >= 2GB effectively means read the rest of the file"

Issue: The code does not match the comment. A user passing INT_MAX (2GB) as count would NOT trigger infinity behavior.

Recommendation: Use a more practical threshold like 2GB: USERTALK_INFINITY_THRESHOLD = 2LL * 1024 * 1024 * 1024

[claude]

LOW: Startup Script Default Change Needs Clearer Migration Path 🟢

The PR changes the default behavior for all existing CLI users. Scripts that previously did not run will now run automatically.

Mitigations Already in Place:

• ✅ --skip-startup flag
• ✅ FRONTIER_HEADLESS_RUN_STARTUP=0 env var
• ✅ Documentation updates in CLI_USAGE_GUIDE.md

Recommendation: Consider adding a deprecation notice in the next release notes or a one-time warning message when startup scripts run (suppressible via env var).

---

LOW: Buffer Size Increase Lacks Justification 🟢

Location: fileverbs_portable.c:676, 684

128KB buffer is used with the comment "reduces syscall overhead; macOS default stack is 8MB"

Questions:

1. Why 128KB specifically? (Is this based on profiling?)
2. Stack allocation of 128KB is fine on macOS (8MB stack), but what about other platforms?
3. Have you tested this on systems with smaller default stack sizes?

Recommendation: Either add a comment citing performance benchmarks, use heap allocation for large buffers, or add a compile-time stack size check.

---

🔍 Code Quality Observations

Positive:

1. Error handling is thorough - all fopen(), fseek(), ftell() failures are caught and reported
2. Logging is excellent - clear debug traces for path lookup, file opening, refcount changes
3. Comments are informative - e.g., explaining why flnextparamislast is reset (line 444)

Minor Nitpicks:

1. Magic number 4096 appears many times for path buffers - consider defining MAX_PATH_LENGTH
2. Some legacy error messages reference refnums (though most are updated)

[claude]

📋 Testing

What is Tested:

• ✅ Path-based file open/close
• ✅ Sequential reads maintaining position
• ✅ Infinity reads
• ✅ Error cases (file not open)
• ✅ All major file I/O operations

What is Missing:

• ❌ Concurrent access tests - Multiple threads opening the same file
• ❌ Path normalization tests - Case sensitivity on macOS vs Linux
• ❌ Edge cases - Paths exactly 4096 bytes, paths with special characters

Recommendation: Add a concurrency test to verify the race condition fix once implemented.

---

🎯 Verdict

Approve with Required Changes

The PR accomplishes its goals and includes excellent test coverage (7→71 passing file verb tests), but the TOCTOU race condition in open_file_by_path() is a blocking issue that must be fixed before merge.

Action Items:

1. REQUIRED: Fix race condition in open_file_by_path() (reserve slot before I/O)
2. REQUIRED: Add return status to normalize_path() to detect truncation
3. RECOMMENDED: Lower infinity threshold to 2GB (matching code comment)
4. RECOMMENDED: Add concurrency tests
5. NICE TO HAVE: Document 128KB buffer size choice

---

📚 References

• Thread safety patterns: docs/ARCHITECTURAL_ANTIPATTERNS.md (global mutable state section)
• File verb semantics: docs/usertalk/docserver/file/
• TOCTOU vulnerabilities: CWE-367

---

Review conducted per: docs/TESTING_GUIDE.md, docs/ARCHITECTURAL_ANTIPATTERNS.md, CLAUDE.md

[claude]

Code Review Summary

This PR makes significant changes to file I/O semantics and startup behavior. Implementation is solid overall, but critical issues need addressing.

---

CRITICAL ISSUES

1. API Breaking Change (portable/fileverbs_portable.c)

• Removed refnum-based API entirely (no deprecation path)
• Old: file.open() returns refnum, file.read(refnum, count)
• New: file.open() returns boolean, file.read(path, count)
• Affects 14 verbs with no migration guide
• RECOMMENDATION: Document as breaking change, add migration guide

2. Thread Safety Race (portable/fileverbs_portable.c:136-199)

• Race condition between mutex unlock/relock in open_file_by_path()
• Thread B can see reserved slot (inuse=true, fp=NULL) while Thread A opens file
• Could lead to NULL pointer access or premature close
• RECOMMENDATION: Add state enum or skip NULL fp in get/close functions

3. Platform Inconsistency (portable/fileverbs_portable.c:109-126)

• Case-insensitive on macOS, case-sensitive on Linux
• Scripts behave differently across platforms
• RECOMMENDATION: Document differences, add file.isCaseSensitive() verb

---

MAJOR CONCERNS

4. Startup Scripts Default Changed

• Now run by default (was: skip by default)
• May break existing automation/CI
• RECOMMENDATION: Document prominently in release notes

5. Infinity Magic Number (line 90)

• Uses 2GB threshold instead of actual infinity constant
• RECOMMENDATION: Use LLONG_MAX or document UserTalk infinity value

6. 128KB Stack Buffer (lines 854, 965)

• Large for platforms with small stacks (embedded, WASM)
• RECOMMENDATION: Reduce to 64KB or heap-allocate

---

CODE QUALITY

7. CWD Search Priority (main.c:450-460)

• CWD checked before standard locations
• Test databases may override system root
• RECOMMENDATION: Move CWD lower or add warning

8. Missing Leak Info (line 329)

• Doesn't log which files leaked
• RECOMMENDATION: Include paths in warning

9. Performance: Linear search O(n) and repeated normalization acceptable for now

---

POSITIVES

Test Coverage: EXCELLENT

• 71/81 tests pass (was 7/81) - 10x improvement
• Covers sequential reads, infinity, error cases
• Outstanding work!

---

BEFORE MERGE

CRITICAL:

1. Fix thread safety race
2. Document breaking API change

HIGH:
3. Add migration guide
4. Document case-sensitivity

MEDIUM:
5. Reduce stack buffer
6. Add paths to leaks

---

Verdict: Request changes for thread safety + docs, then approve

Great work on test coverage!

[claude]

Code Review: PR #382 - Startup scripts and path-based file verbs

Summary

This PR makes two significant architectural changes:

1. Startup scripts run by default (with --skip-startup flag to disable)
2. File I/O refactored from refnum-based to path-based API

The implementation is well-structured and the test improvement (7→71 passing tests) is impressive. However, there are several critical issues that need attention before merge.

---

🔴 Critical Issues

1. Thread-Safety Race Condition in open_file_by_path()

Location: portable/fileverbs_portable.c:432-504

Problem: Thread A checks if file is open (not found), unlocks mutex, then thread B opens same file, then thread A also opens same file → same file opened twice.

Fix: Move the duplicate check after reservation, or hold the lock longer during initial checks.

---

2. Case-Sensitivity Inconsistency in normalize_path()

Location: portable/fileverbs_portable.c:408-424

Problems:

1. APFS can be case-sensitive - code assumes all macOS is case-insensitive
2. Symlinks not resolved - /tmp/file and /tmp/link-to-file treated as different
3. Relative vs absolute paths - ./file vs /full/path/file won't match

Fix: Use realpath() to canonicalize paths before comparison.

---

3. Path Truncation Silently Degrades Security

Location: portable/fileverbs_portable.c:416-422

Problem: Many callers ignore the return value when paths are truncated, causing file handle confusion.

Fix: Make normalize_path() propagate errors properly or fail fatally.

---

4. Leaked File Handles When refcount > 0

Location: portable/fileverbs_portable.c:597-649

Problem: If refcount > 0 when file.close() is called, the file is never closed.

Fix: Add a pending_close flag to mark files that should be closed when refcount reaches 0.

---

🟡 Design Concerns

5. Linear Search O(n) for Every File Operation

Every file.read(), file.write(), etc. does linear search through 64 entries with string comparison.

Recommendation: Add hash table for O(1) lookups (not critical but measurable for I/O-heavy scripts).

---

6. File Mode Ignored (Always Opens r+b/rb)

The PR removed the readonly parameter. Files are always opened read-write (or read-only if that fails).

Question: Is this intentional to match legacy Frontier? If so, document it.

---

7. Memory Allocation Risk

500MB per-file limit is good, but 100 concurrent reads = 50GB. Consider global memory quota or document limitation.

---

🟢 Positive Aspects

✅ Well-Structured Refactoring - Clean separation, good reference counting, I/O outside mutex
✅ 10x Test Improvement - 7→71 passing tests with comprehensive edge cases
✅ Buffer Optimization - 8KB→128KB copy buffer
✅ Documentation Updates - CLI usage guide properly updated

---

📋 Recommendations

Before Merge (Critical):

1. Fix race condition in open_file_by_path() duplicate check
2. Replace normalize_path() with realpath() for proper canonicalization
3. Handle path truncation errors properly
4. Add pending_close flag for refcount > 0 case

Consider for Follow-up:

5. Add hash table for O(1) path lookups
6. Document file mode behavior
7. Document memory quota limitations
8. Improve env var parsing for startup flag

---

Verdict

Status: ⚠️ Needs Changes

The refactoring is architecturally sound, but thread-safety issues and path normalization bugs need to be addressed before merge. These could cause production issues with race conditions and file handle leaks.

Once critical issues are fixed, this will be a solid improvement.

---

Reviewed using: CLAUDE.md conventions, focusing on architectural anti-patterns (global mutable state, thread safety, proper error handling)