feat: NDJSON protocol mode + parallel test execution

[jsavin]

Summary

• NDJSON protocol mode: Long-lived frontier-cli process reads JSON commands from stdin, executes them, and writes JSON responses to stdout. Eliminates per-test process spawning overhead for integration tests.
• Parallel test runner: Python integration test runner supports --batch mode with -j N parallel workers, each communicating with a persistent CLI process via NDJSON protocol.
• State leak fix: Reset langerrordisable, langerrorlogdisable, and fllangerror in handle_clear_context() to prevent error-suppression state from leaking between evaluations in batch mode. Reduces sequential batch failures from 139 to 79.
• TCP socket timeouts: Replace blocking connect() with non-blocking connect + select() (10s timeout). Add SO_RCVTIMEO/SO_SNDTIMEO (30s) on connected sockets. Add langbackgroundtask() yield in write loop for cancellation support.

Test plan

• make -C frontier-cli builds cleanly (universal binary)
• make test-integration passes (63 failures, down from 76 pre-fix in parallel mode)
• Batch sequential (--batch -j 1) drops from 139 to 79 failures
• Unit test segfault is pre-existing on develop, not introduced by this branch
• Verify tcp.openNameStream("198.51.100.1", 80) times out in ~10s (TEST-NET-2 address) — confirmed 10s
• Verify localhost TCP tests still pass — client tests pass, server-side listenStream failures are pre-existing

🤖 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: NDJSON Protocol Mode + Parallel Test Execution

Summary

This PR introduces a well-architected NDJSON protocol mode for frontier-cli and parallel test execution infrastructure. The implementation is generally solid with good attention to detail, but there are several issues that should be addressed before merging.

Critical Issues

1. Protocol Handler - DoS Risk from Unbounded String Allocation

File: frontier-cli/protocol_handler.c:json_extract_string()

The function malloc's unbounded memory based on JSON input. A malicious client could send large operation names causing excessive heap allocation.

Recommendation: Add length validation (e.g., MAX_JSON_STRING_LENGTH 8192) and free+return NULL if exceeded.

2. Protocol Handler - Global State Mutation Thread Safety

File: frontier-cli/protocol_handler.c:handle_clear_context()

Direct mutation of global error state (langerrordisable, langerrorlogdisable, fllangerror). Is the GIL held for the entire protocol request lifecycle? If yes, add a comment confirming this is safe. If no, these need mutex protection.

3. TCP Socket Timeouts - Missing Error Handling

File: Common/source/tcpverbs.c:tcp_set_socket_timeouts()

Return values from setsockopt() are silently ignored. Recommend logging failures at log_debug level or returning a boolean.

High-Priority Issues

4. Python Test Runner - Missing Worker Temp Directory Cleanup

File: tests/integration/runner.py:_run_file_worker()

Worker temp directories are created but never cleaned up, potentially causing flaky tests or disk space exhaustion. Add shutil.rmtree(worker_tmp, ignore_errors=True) in a finally block.

5. Integration Test Checklist Incomplete

From the PR description, these items are not checked:

• Verify tcp.openNameStream times out in ~10s
• Verify localhost TCP tests still pass

Please complete these manual test steps before merging.

Medium-Priority Issues

6. TCP Write Loop - Inefficient GIL Yielding

The GIL yield happens on every write() iteration. For large writes, recommend yielding every 64KB instead of every iteration.

7. stdout Redirection to /dev/null - Lost Diagnostics

Consider redirecting stdout to stderr instead of /dev/null to preserve diagnostics from verb implementations.

Test Coverage

8. Missing Protocol Edge Case Tests

Consider adding tests for: oversized JSON lines, malformed JSON, unknown operation codes, protocol process crash recovery.

Positive Observations

1. Excellent state leak fix - Resetting error globals in handle_clear_context() is exactly right
2. Proper UTF-8 handling in JSON parser with surrogate pair support
3. Graceful shutdown protocol
4. Worker isolation with dedicated protocol processes
5. Backward compatibility via --no-batch flag
6. Clean code organization
7. Database integrity verification via checksums

Overall Assessment

This is a high-quality PR that delivers significant value. The core architecture is sound with good attention to detail. The critical issues are addressable and don't require major refactoring.

Recommendation: Request changes to address the DoS risk, thread safety clarification, worker cleanup, and complete the test checklist.

Estimated effort to address critical issues: 2-3 hours

Great work overall!

[claude]

Code Review: NDJSON Protocol Mode + Parallel Test Execution

This is an impressive performance optimization that delivers a 4x speedup on integration tests while also fixing critical state leak bugs and adding TCP timeout support. The implementation is well-structured and aligns nicely with the future GUI protocol.

---

🎯 Summary

Strengths:

• Major performance win: ~6.5min → ~1.5min test runtime (4x improvement)
• Well-architected protocol design: NDJSON stdio protocol aligns with future GUI WebSocket protocol
• Critical bug fix: Error state leak fix (reduces batch failures from 139 → 79)
• Production-ready TCP timeouts: Prevents indefinite hangs on network operations
• Excellent documentation: STDIO_PROTOCOL.md is thorough and well-structured

Areas for improvement:

• Memory leak risk in JSON parsing
• Missing error handling in timeout helpers
• Protocol executor restart logic could be more robust
• Test coverage gaps for new protocol mode

---

🔍 Detailed Findings

1. C Implementation (protocol_handler.c) ⭐

Critical: Memory Leak in json_extract_string

Location: protocol_handler.c:88-184

The json_extract_string function allocates memory with malloc/realloc and returns it to the caller. If the caller doesn't free this memory, it leaks.

char *expression = json_extract_string(line_buf, "expression");
if (expression == NULL) {
    write_eval_error(id, "Missing 'expression' in params");
    return;  // ❌ No free needed here, but pattern is fragile
}
// ... later ...
free(expression);  // ✅ Good, but easy to miss

Risk: In protocol_main (line 429-457), if a malformed op is extracted and then an error occurs before free(op), memory leaks.

Recommendation:

• Add a comment to json_extract_string documenting the caller's responsibility to free
• Consider using a small fixed-size buffer for op since it's always short ("script/eval", "shutdown")
• Add __attribute__((warn_unused_result)) if using GCC/Clang

Good: Surrogate Pair Handling

The UTF-16 surrogate pair decoding (lines 116-127) is correctly implemented. This is non-trivial and handles edge cases well.

Minor: Missing NULL checks

tcp_set_socket_timeouts (line 569) doesn't check setsockopt return values. While this is typically best-effort, logging failures would help debugging.

---

2. TCP Timeout Implementation (tcpverbs.c) ⭐⭐

Excellent: Non-blocking connect with timeout

The tcp_connect_with_timeout implementation (lines 517-566) is well-done:

• Correctly handles EINPROGRESS
• Uses select() for timeout
• Checks SO_ERROR to detect connection failure
• Restores blocking mode

Critical: langbackgroundtask() in write loop

Location: tcpverbs.c:112-119

Adding langbackgroundtask() to tcp_write_stream is excellent for GIL cooperation and cancellation support. However:

if (!langbackgroundtask(false)) {
    /* User cancelled */
    unlockhandle(hdata);
    tcp_stream_release(stream);
    tcp_set_error(TCP_ERR_SOCKET_ERROR, "Write cancelled");
    return false;
}

Question: Does tcp_stream_release properly close the socket? If the socket is left open after cancellation, it could leak file descriptors.

Recommendation: Verify that cancellation properly cleans up the socket (looks okay from context, but worth confirming in testing).

---

3. Python Test Runner (runner.py) ⭐

Good: Process pool parallelization

The parallel execution architecture is clean:

• Sequential vs parallel bucketing (lines 924-931)
• Per-worker temp directories to avoid conflicts (line 465)
• Graceful degradation when protocol executor fails (lines 904-906)

Issue: Protocol executor restart logic

Location: runner.py:295-304

def _restart(self):
    try:
        if self._proc is not None:
            self._proc.kill()
            self._proc.wait(timeout=2)
    except Exception:
        pass  # ❌ Silently swallows all exceptions
    self._proc = None
    self.start()

Problems:

1. self.start() can raise exceptions, but caller doesn't expect it (see line 328)
2. If kill() fails (e.g., process already dead), wait() may hang
3. Broad except Exception hides real issues

Recommendation:

def _restart(self):
    try:
        if self._proc is not None:
            try:
                self._proc.kill()
            except ProcessLookupError:
                pass  # Already dead
            try:
                self._proc.wait(timeout=2)
            except subprocess.TimeoutExpired:
                pass  # Force killed, can't wait
    finally:
        self._proc = None
    
    try:
        self.start()
    except Exception as e:
        # Log but don't raise - allow graceful degradation
        print(f"Warning: Failed to restart protocol executor: {e}", file=sys.stderr)

Minor: Race condition in is_alive

Location: runner.py:393-394

@property
def is_alive(self) -> bool:
    return self._proc is not None and self._proc.poll() is None

Between the is_alive check (line 696) and execute() call (line 698), the process could die. The execute() method handles this (line 325-338), but worth noting.

---

4. Error State Fix ⭐⭐⭐

Excellent: Root cause analysis

The error state leak fix (protocol_handler.c:365-369) is exactly right:

langerrordisable = 0;
langerrorlogdisable = 0;
fllangerror = false;

Impact: Reduces batch failures from 139 → 79 (43% reduction).

Question: Are there other global state variables that should be reset? What about:

• langcallbacks stack depth?
• langcanceldialog flag?
• Mode stack (pushmode/popmode)?

Recommendation:

• Audit other global state that could leak between evaluations
• Document in STDIO_PROTOCOL.md which globals are reset vs preserved
• Consider adding a comprehensive reset_runtime_state() function

---

5. Documentation ⭐⭐⭐

Excellent: STDIO_PROTOCOL.md

The protocol documentation is thorough, well-organized, and includes:

• Design rationale
• Example sessions
• Error state management explanation
• Future GUI integration path

Minor suggestions:

• Section 5.2: Add note about potential for other state leaks (see above)
• Section 2.3: Mention that stderr goes to DEVNULL in Python client (line 253) but is logged in C code
• Add troubleshooting section for common issues (protocol desync, process crashes)

---

🐛 Potential Bugs

1. JSON Parser: Unescaped quotes in keys

If a JSON key contains a quote (malformed input), json_extract_string could match the wrong field:

{"op\"\"":"fake","op":"script/eval"}

The simple strstr search would find the first "op" match.

Severity: Low (input is machine-generated)
Recommendation: Add comment noting this limitation

2. Protocol Executor: Timeout in reset()

def reset(self):
    if self._proc is None or self._proc.poll() is not None:
        return
    try:
        self._send_recv({'op': 'script/clearContext'}, timeout=5.0)
    except Exception:
        pass  # Best effort

If clearContext hangs, the timeout will raise an exception, but the process is left in an unknown state. Subsequent execute() calls might fail.

Recommendation: After timeout in reset(), call _restart() to ensure clean state.

3. TCP Timeout: fcntl error handling

In tcp_connect_with_timeout, if fcntl(F_SETFL) fails when restoring flags, we return -1 but leave the socket in non-blocking mode.

Severity: Low (rare failure mode)
Recommendation: Log error if fcntl restoration fails

---

🧪 Test Coverage

What's Tested:

✅ Basic protocol operations (script/eval, clearContext, shutdown)
✅ Parallel execution across workers
✅ Error state reset between evaluations
✅ TCP connection to localhost

Gaps:

❌ TCP connect timeout (TEST-NET address test in checklist is unchecked)
❌ Protocol executor restart after process death
❌ Malformed JSON handling in protocol mode
❌ Socket send/receive timeout enforcement
❌ Protocol desync recovery (extra newlines, binary data)

Recommendation:

• Add integration test for TCP timeout (connect to 192.0.2.1:80 from test plan)
• Add unit test for json_extract_string edge cases
• Add test for protocol recovery after unexpected process exit

---

🔒 Security Considerations

1. Stdout isolation

✅ Excellent: Redirecting stdout to /dev/null prevents protocol contamination
✅ Logging goes to stderr, separate from protocol stream

2. Input validation

⚠️ Limited: Protocol handler trusts JSON structure from client

• No input size limits (64KB line buffer, but no per-field limits)
• No validation of expression content (relies on UserTalk sandbox)

Recommendation: Since this is currently test-only, security is acceptable. For future GUI use, add:

• Expression length limits
• Rate limiting on requests
• Authentication/authorization

---

📊 Performance

Measurements:

• Before: ~6.5 minutes (per-process spawning)
• After: ~1.5 minutes (persistent process + parallelization)
• Speedup: 4.3x

What's contributing:

1. Process reuse: Eliminates ~210ms startup per test
2. Parallelization: N workers for N CPU cores (capped at 8)
3. Reduced I/O: Single database open per worker

Minor concern:
Python's ProcessPoolExecutor uses multiprocessing, which has significant overhead on macOS (fork+exec). On a large test suite, consider using concurrent.futures.ThreadPoolExecutor with async I/O for even better performance.

---

✅ Best Practices

Excellent:

• ✅ Separation of concerns (protocol_handler.c, cli_json_output.c)
• ✅ Backward compatibility (--no-batch -j 1 preserves old behavior)
• ✅ Database integrity verification (checksum before/after tests)
• ✅ Graceful degradation (falls back to per-process if protocol fails)
• ✅ Clean shutdown protocol (shutdown operation)

Good:

• ✅ Error handling in most paths
• ✅ Timeout configuration per-test
• ✅ Verbose mode for debugging
• ✅ Worker temp directories to avoid conflicts

---

🎯 Recommendations

Must Fix:

1. Document memory ownership in json_extract_string to prevent leaks
2. Improve restart logic in ProtocolExecutor._restart() (see detailed note above)
3. Add TCP timeout test (unchecked item in test plan)

Should Fix:

4. Audit global state reset beyond error flags (see Section 4 above)
5. Add protocol error recovery tests (malformed JSON, process crash)
6. Check tcp_stream_release cleanup on cancellation

Nice to Have:

7. Add troubleshooting section to STDIO_PROTOCOL.md
8. Consider fixed-size buffer for op field to avoid allocation
9. Log setsockopt failures in tcp_set_socket_timeouts
10. Add metrics (operations/sec, average latency) to test output

---

📝 Commit Quality

✅ Excellent commit messages:

• Clear feature description
• Performance metrics
• Known issues documented (68 batch-only failures)
• Co-authored attribution

✅ Logical commit breakdown:

1. Core protocol + parallel runner
2. State leak fix + TCP timeouts
3. Documentation

---

Final Verdict: ⭐⭐⭐⭐ (4/5 stars)

This is high-quality work that delivers significant value:

• ✅ Major performance improvement
• ✅ Critical bug fixes
• ✅ Well-documented and maintainable
• ✅ Future-proof architecture

Recommendation: Approve with minor changes

Address the "Must Fix" items (especially memory leak documentation and restart logic), add the TCP timeout test, and this is ready to merge. The remaining issues are low-severity and can be addressed in follow-up PRs.

Excellent work on this optimization! The attention to detail in the protocol design and error state debugging is impressive.

---

Review generated by Claude Code - checked against CLAUDE.md conventions

[jsavin]

Review Feedback Addressed

Thanks for the thorough review! All 7 items addressed in commit 3ce9e45:

Critical Issues — Fixed

1. DoS risk in json_extract_string() — Added PROTOCOL_MAX_STRING (8KB) cap with length checks on single-byte and all multi-byte UTF-8 paths. Returns NULL if exceeded.

2. Thread safety of error state reset — Added comment confirming protocol_main() holds the GIL for the entire request lifecycle. No mutex needed.

3. setsockopt() return values — Now logged at log_debug level with errno on failure.

High-Priority Issues — Fixed

4. Worker temp directory cleanup — Wrapped _run_file_worker() body in try/finally with shutil.rmtree(worker_tmp, ignore_errors=True).

5. Incomplete test checklist — Verified manually:

   • tcp.openNameStream("198.51.100.1", 80) (TEST-NET-2) times out in exactly 10 seconds ✅
   • Localhost TCP client tests pass ✅ (server-side listenStream failures are pre-existing)

Medium-Priority Issues — Fixed

6. GIL yield frequency — Changed from every send() to every 64KB boundary crossing: (total_written % 65536) < bytes_written.

7. stdout → stderr — Replaced /dev/null redirect with dup2(STDERR_FILENO, STDOUT_FILENO). Diagnostic output from verb implementations now visible on stderr.

Item 8: Protocol edge case tests — Deferred

The YAML test framework sends all tests through script/eval automatically — there's no mechanism to send raw protocol messages (unknown ops, malformed JSON, oversized lines). Options for future:

• Extend runner.py with a protocol_raw test type
• Standalone Python script that communicates directly with --protocol subprocess
• C unit tests for protocol handler functions

Filed as future work rather than forcing tests into a framework that can't express them.

[claude]

Code Review: NDJSON Protocol Mode + Parallel Test Execution

This is a well-architected PR that introduces significant performance improvements to the test infrastructure. The code quality is generally excellent with thoughtful design decisions. Below are my findings organized by category.

---

🎯 Strengths

Architecture & Design

• Clean protocol abstraction: The NDJSON protocol handler is well-isolated with clear separation of concerns
• Stdout isolation pattern: Duplicating stdout fd and redirecting the C library's stdout to stderr is an elegant solution to prevent protocol stream contamination
• Minimal JSON parser: Intentionally simple string scanning is appropriate for machine-generated input with a fixed schema
• UTF-8 handling: Comprehensive support for escape sequences including surrogate pairs (lines 109-172 in protocol_handler.c)
• Error state management: Resetting langerrordisable, langerrorlogdisable, and fllangerror in clearContext fixes a real state leak issue

Implementation Quality

• Thread safety awareness: GIL comment at protocol_handler.c:385 shows understanding of the threading model
• Memory management: Proper cleanup with dynamic reallocation for JSON string parsing
• Timeout handling: Non-blocking connect with select() for TCP operations is properly implemented
• Test infrastructure: Parallel execution with worker pools is well-designed with per-worker temp directories

---

⚠️ Issues Found

1. Resource Leak in JSON Parsing (protocol_handler.c:187-189, 614-615)

if (len + 1 >= PROTOCOL_MAX_STRING) {
    free(result);
    return NULL;
}

Problem: This check happens AFTER UTF-8 encoding has potentially written beyond PROTOCOL_MAX_STRING (lines 143-170). The bounds check should occur before writing.

Impact: Potential buffer overflow with carefully crafted input.

Recommendation: Check len + need >= PROTOCOL_MAX_STRING before each UTF-8 encoding operation (already done at line 143, but missing consistent pattern).

---

2. Missing Null Termination Check (protocol_handler.c:201)

result[len] = '\0';
return result;

Problem: If the loop exits because *pos == '"', the closing quote is never consumed. The caller might process subsequent JSON incorrectly.

Impact: Malformed JSON could cause subtle parsing errors.

Recommendation: Add if (*pos == '"') pos++; before returning, or document that callers must handle this.

---

3. TCP Timeout Interaction (tcpverbs.c:106-115)

if ((total_written % 65536) < bytes_written && !langbackgroundtask(false)) {
    /* User cancelled */
    ...
}

Problem: The yield happens every 64KB, but SO_SNDTIMEO is set to 30s. If a write blocks, the 30s timeout applies to the entire write syscall, not the 64KB chunk. This could cause unexpected timeout behavior on slow connections.

Impact: Tests with large writes might timeout unexpectedly.

Recommendation: Consider lowering TCP_SOCKET_TIMEOUT_SECS or documenting the interaction between write loop yields and socket timeouts.

---

4. Missing Error Propagation (protocol_handler.c:366-367)

if (result.valuetype == stringvaluetype && result.data.stringvalue != nil) {
    disposehandle(result.data.stringvalue);
}

Problem: Only disposes string values, but other value types (binary, list, record) may also allocate handles. This could leak memory if coercetostring() allocates intermediate handles.

Impact: Potential memory leak for non-string result types.

Recommendation: Call disposevalue(&result) instead, or document why only strings need cleanup.

---

5. Race Condition in Worker Cleanup (runner.py:502)

finally:
    shutil.rmtree(worker_tmp, ignore_errors=True)

Problem: If the protocol executor is still running when the worker exits, temp files might still be open (database locks, etc.). ignore_errors=True silently suppresses these failures.

Impact: Incomplete cleanup could cause test pollution.

Recommendation: Call executor.stop() in a try/finally block before shutil.rmtree(), or at least log ignored errors.

---

6. Incomplete Restart Logic (runner.py:296-305)

def _restart(self):
    try:
        if self._proc is not None:
            self._proc.kill()
            self._proc.wait(timeout=2)
    except Exception:
        pass
    self._proc = None
    self.start()

Problem: If start() fails after killing the old process, the executor is left in an inconsistent state (_proc = None but is_alive was true before restart).

Impact: Subsequent tests might fail with confusing errors.

Recommendation: Wrap start() in try/except and raise a clear error if restart fails.

---

🔧 Minor Issues & Improvements

Code Quality

1. Magic number: PROTOCOL_LINE_MAX 65536 (protocol_handler.c:39) should be documented - why 64KB? Is this based on stdin buffer limits?

2. Inconsistent error handling: json_extract_int() returns -1 on error, but -1 could be a valid ID. Consider using a sentinel like LONG_MIN or an out-parameter for errors.

3. Missing validation: tcp_connect_with_timeout() doesn't validate that sockfd is a valid socket before calling fcntl().

4. Type coercion assumption: protocol_handler.c:344 assumes result_type is always "string" for protocol responses, but this might confuse tests expecting type fidelity. Consider preserving original type names.

Testing

1. Test plan incomplete: The PR description lists unchecked items:

   • [ ] Verify tcp.openNameStream("192.0.2.1", 80) times out in ~10s (TEST-NET address)
   • [ ] Verify localhost TCP tests still pass

2. Hardcoded worker limit: min(multiprocessing.cpu_count(), 8) (runner.py:877) caps at 8 workers. On a 64-core CI machine, this might be unnecessarily limiting.

Documentation

1. STDIO_PROTOCOL.md line 1022: "redirects stdout to /dev/null" should say "redirects stdout to stderr" (matches implementation).

2. Missing example: The protocol spec doesn't show how to handle server-initiated events (mentioned in section 7.1 for future GUI use).

---

🛡️ Security Considerations

1. Input validation: The JSON parser is "intentionally simple" but doesn't validate schema. Malformed JSON (missing quotes, infinite nesting) could cause undefined behavior. Consider adding basic sanity checks (max nesting depth, balanced braces).

2. Resource exhaustion: A malicious client could send very long lines (up to 64KB) repeatedly. Consider rate limiting or max request size.

3. File descriptor leak: If protocol_main() crashes before fclose(g_protocol_out), the dup'd fd leaks. Consider using RAII patterns or cleanup handlers.

---

📊 Performance Considerations

Wins

• ✅ Eliminating per-test process spawning saves ~210ms per test
• ✅ Parallel execution with -j N workers scales well
• ✅ Non-blocking TCP connect prevents hangs on unreachable hosts

Potential Bottlenecks

• ⚠️ Line-buffered protocol output (setvbuf(..., _IOLBF, 0)) flushes on every newline. For high-throughput tests, this might cause syscall overhead.
• ⚠️ Python's select.select() on a single fd (runner.py:277) is fine for now, but won't scale if future versions need to multiplex multiple streams.

---

✅ Test Coverage Assessment

The PR improves test infrastructure but doesn't add new test cases for the protocol itself. Recommendations:

1. Protocol unit tests: Test malformed JSON handling (missing fields, invalid UTF-8, oversized strings)
2. Timeout tests: Verify that tcp.openNameStream() actually times out in 10s (as noted in test plan)
3. State leak regression test: Add a test that verifies langerrordisable is reset between evaluations
4. Parallel safety: Add a test that runs multiple workers against the same database to verify no corruption

---

🎓 Best Practices Compliance

✅ Following Repository Guidelines

• Uses logging standards (LOG_COMP_GENERAL, LOG_COMP_LANG)
• Includes comprehensive documentation (STDIO_PROTOCOL.md)
• Follows GIL threading model requirements
• PR description includes test plan

❌ Minor Deviations

• New files lack copyright year update (still shows 1992-2004)
• Some functions exceed 80 characters per line (cli_json_write_escaped_buffer signature)

---

📝 Recommendations

Must Fix (Before Merge)

1. Fix bounds checking in JSON UTF-8 encoding (issue Repo Modernization: 64‑bit/ARM + DB Header Rev; Headless/Portable Runtime; Headless EFP Routing for file.* and Tests #1)
2. Complete the test plan checklist items
3. Add proper cleanup to _run_file_worker finally block (issue planning: reorganize phase structure and refresh docs #5)

Should Fix (High Priority)

4. Improve error handling in _restart() (issue docs: add Frontier code patterns catalog #6)
5. Fix memory leak in handle_script_eval() (issue planning: relocate UTF-8 plan and add phase directories #4)
6. Add protocol-level error handling tests

Nice to Have (Low Priority)

7. Document magic numbers (PROTOCOL_LINE_MAX, worker limit of 8)
8. Add schema validation to JSON parser
9. Update copyright years in new files

---

🎉 Summary

This is high-quality work that meaningfully improves test execution performance. The architecture is sound, the code is readable, and the documentation is thorough. The issues identified are primarily edge cases and defensive programming opportunities rather than fundamental flaws.

Recommendation: Approve with requested changes (fix issues #1, #4, #5, and complete test plan).

Estimated Risk: Low — changes are well-isolated to test infrastructure and protocol handling, with minimal impact on production runtime code (only TCP timeout additions).

---

Great work on this PR! The parallel test execution is a significant quality-of-life improvement for development velocity. 🚀

[jsavin]

Review Round 2 Feedback Addressed

All actionable items from the second review addressed in commit dcda7a3:

Must Fix

#1 — UTF-8 bounds checking: Already addressed in round 1 — the PROTOCOL_MAX_STRING checks before 2/3/4-byte UTF-8 writes were already in place. Verified correct.

#2 — Closing quote not consumed: Added documentation comment explaining this is intentional (callers extract values independently).

#4 — Memory leak for non-string results: Replaced string-only disposehandle() with disposevaluerecord(result, false) which handles all value types (string, binary, list, record, external, etc.).

#5 — Worker cleanup ordering: Now calls executor.stop() before shutil.rmtree() in the finally block, with try/except guard so cleanup always proceeds.

Should Fix

#6 — Restart error handling: Wrapped self.start() in try/except that raises RuntimeError with clear message and exception chaining.

Documentation

STDIO_PROTOCOL.md: Updated all references from "/dev/null" to "stderr" to match the implementation change from round 1.

Test Plan Items (verified in round 1)

• tcp.openNameStream("198.51.100.1", 80) times out in exactly 10s ✅
• Localhost TCP client tests pass ✅

Advisory Items (not addressed — acceptable as-is)

• docs: UTF-8 transition plan #3 (TCP timeout interaction): SO_SNDTIMEO and yield frequency are independent safety nets; 30s timeout on individual syscalls + 64KB yield is reasonable.
• json_extract_int returning -1: Protocol IDs are always positive, so -1 is a safe sentinel.
• Copyright years: Follows existing codebase convention (original copyright range preserved).
• Protocol edge case tests: Deferred — requires test framework extension (documented in round 1 response).

[claude]

PR Review: NDJSON Protocol Mode + Parallel Test Execution

Summary

This PR introduces a significant performance improvement to the integration test infrastructure through:

1. NDJSON stdio protocol mode for long-lived frontier-cli processes
2. Parallel test execution with worker pools
3. TCP timeout fixes to prevent hanging tests
4. Error state cleanup to prevent cross-test contamination

Impact: ~4x test runtime improvement (6.5 min → 1.5 min)

---

Code Quality Assessment: EXCELLENT ✅

Strengths

1. Well-architected protocol design

   • Clean separation of concerns (JSON parsing, protocol handling, execution)
   • Proper stdout isolation to prevent protocol contamination
   • Aligns with future GUI protocol spec for reusability
   • Comprehensive documentation in STDIO_PROTOCOL.md

2. Robust error handling

   • Memory allocation failure paths covered
   • Graceful process death recovery with auto-restart
   • Timeout handling with select() for non-blocking I/O
   • Proper resource cleanup in all paths

3. Memory safety

   • DoS protection: 8KB cap on extracted JSON strings (PROTOCOL_MAX_STRING)
   • Proper disposal of tyvaluerecord via disposevaluerecord() (all handle types)
   • Dynamic reallocation with overflow checks in JSON parser
   • Cleanup of temp directories in worker processes

4. Code reuse

   • JSON escaping extracted to shared cli_json_output.c
   • UTF-16 surrogate pair handling is thorough and correct
   • Protocol executor abstraction allows fallback to per-process mode

5. Thread safety awareness

   • GIL documentation in error state reset comment (protocol_handler.c:388-389)
   • Proper understanding of Frontier's threading model

---

Security: SECURE ✅

No security issues found

• Input validation: JSON parser handles malformed input gracefully
• DoS protection: PROTOCOL_MAX_STRING cap prevents unbounded allocation
• No injection risks: Scripts passed via stdin protocol, not shell
• Resource limits: Worker count capped, temp dirs cleaned up

---

Performance: EXCELLENT ✅

1. 4x speedup from eliminating per-test process spawning
2. Parallel execution with auto-scaling worker count
3. TCP timeouts prevent test hangs from blocking forever
4. Smart routing: Protocol-incompatible tests fall back to per-process execution

---

Issues & Recommendations

Medium Priority

1. Incomplete Test Plan

Issue: PR description shows 2 test plan items unchecked:

• Verify tcp.openNameStream("192.0.2.1", 80) times out in ~10s
• Verify localhost TCP tests still pass

Recommendation: Complete these manual tests before merging to verify TCP timeout implementation works as expected.

2. JSON Parser Bounds Check Ordering (protocol_handler.c:187-197)

Location: json_extract_string() single-byte character path

The bounds check at line 187 happens before character append at line 197, which is correct. However, for consistency with the multi-byte UTF-8 path (lines 143-172), consider checking PROTOCOL_MAX_STRING immediately before the append operation rather than before the capacity check.

Current code is safe, but the ordering could be slightly clearer.

Low Priority

3. Worker Temp Cleanup Duplication (runner.py:498, 507)

Executor is stopped twice (once before finally, once in finally). Harmless but could be simplified to only stop in the finally block.

---

Best Practices: EXCELLENT ✅

Frontier Standards Compliance

• ✅ Logging Standards: Uses structured logging
• ✅ Memory Management: Proper handle disposal
• ✅ Error Handling: Consistent error reporting
• ✅ Documentation: Comprehensive protocol spec
• ✅ Testing Strategy: Maintains backward compatibility

Python Code Quality

• ✅ Type hints used appropriately
• ✅ Proper subprocess management
• ✅ Graceful degradation when protocol executor fails
• ✅ Resource cleanup with try/finally

---

Documentation: EXCELLENT ✅

1. STDIO_PROTOCOL.md: Comprehensive protocol spec with examples
2. Code comments: Clear explanations of design decisions
3. Commit messages: Detailed, explain the "why" not just "what"
4. Test plan: Well-structured

---

Test Coverage: GOOD ✅

Tested

• ✅ Protocol message parsing and dispatch
• ✅ Error state cleanup (139 → 79 failures)
• ✅ Worker process isolation
• ✅ Fallback to per-process mode

Missing

• ⚠️ Manual TCP timeout verification (per test plan)
• ⚠️ Protocol edge case unit tests (malformed JSON, DoS cap)

---

Migration/Compatibility: EXCELLENT ✅

• ✅ Backward compatible with --no-batch
• ✅ Graceful degradation for incompatible tests
• ✅ Database checksum verification

---

Recommendations

Before Merge

1. Complete test plan items (TCP timeout verification)
2. Consider the bounds check ordering clarification (optional)

Follow-up Work (Separate PRs OK)

1. Add protocol-specific unit tests for edge cases
2. Add structured logging for protocol errors

---

Final Verdict: APPROVE WITH MINOR RESERVATIONS ✅

This is high-quality work that significantly improves test infrastructure. The code is well-designed, properly documented, and follows best practices. The minor issues noted are edge cases that don't affect core functionality.

Recommendation:

• Complete the manual TCP timeout tests from the test plan
• Approved for merge after test plan completion

Great work on this 4x performance improvement! 🚀