feat: REPL improvements - persistent variables, focus tracking, tab completion

[jsavin]

Summary

• Persistent REPL variables: Variables assigned in the REPL now persist across evaluations via system.temp.FrontierREPL.variables
• Focus tracking: /jump navigation is tracked in system.temp.FrontierREPL.focus, accessible from UserTalk
• Tab completion for /list: The /list command now supports tab completion for path arguments
• Prioritized completion: Tab completion for /jump and /list prioritizes the currently focused table
• msg() prefix: Added "msg: " prefix to msg() output in REPL mode for clarity
• Bug fixes: Fixed double-free crash in variable sync callback, fixed crash when defining functions

Changes

Persistent Variables

• Created system.temp.FrontierREPL table structure with subtables for variables, commands, target, and focus
• Implemented callback hook in langpoplocalchain to sync variables from the with block's local table to persistent storage
• Variables persist across REPL evaluations without polluting the root table

Focus Tracking

• repl_set_focus() updates system.temp.FrontierREPL.focus whenever /jump navigates
• Focus is exposed to UserTalk scripts for integration with custom tooling

Tab Completion

• Extended tab completion to work with /list command arguments
• Completion prioritizes entries from the currently focused table

Bug Fixes

• Added missing exemptfromtmpstack() call after hashassign() to prevent double-free
• Skip code values (functions/scripts) in sync callback to prevent langunpacktree assertion failures

Test plan

• Unit tests pass (pre-existing callback test failures unrelated to this PR)
• Integration tests: 1441 passed, 149 skipped, 98 failed (2 new expected failures for function persistence which is intentionally deferred)
• Manual testing of variable persistence, focus tracking, and tab completion

Known Limitations

• Function definitions in REPL do not persist (intentionally deferred - requires special handling for code tree copying)

🤖 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 #371: REPL Improvements

Overview

This PR adds significant REPL functionality including persistent variables, focus tracking, and enhanced tab completion. The implementation is generally solid with good architecture, but there are several areas requiring attention before merge.

---

🔴 Critical Issues

1. Global Mutable State in Core Runtime (BLOCKING)

Location: Common/source/lang.c:29, Common/headers/langinternal.h:15-16

langmagictablecallback langmagictabledisposecallback = nil;

Problem: This introduces global mutable state into the core language runtime. Per docs/ARCHITECTURAL_ANTIPATTERNS.md, global mutable state should be eliminated before launch and replaced with thread-local or explicit context.

Impact:

• Prevents future multi-threading support
• Creates tight coupling between CLI and runtime
• Violates architectural principles

Recommendation:

• Add this callback to a thread-local context structure or pass it explicitly through the call chain
• Consider using a registration pattern where the REPL registers/unregisters the callback via a proper API
• See ADR-005 for thread-local parameter state pattern

References:

• docs/ARCHITECTURAL_ANTIPATTERNS.md - "Global mutable state"
• ADR-005 - Thread-local parameter state

---

2. Incorrect Include Path (BLOCKING)

Location: Common/source/langverbs.c:129

#include "../../frontier-cli/repl.h"  /* For repl_is_active() */

Problem: Core runtime code (Common/) should NEVER include CLI-specific headers. This creates backwards dependency and violates layering.

Impact:

• Breaks architectural boundaries
• Makes the runtime dependent on CLI implementation
• Prevents code reuse in other contexts

Recommendation:

• Move repl_is_active() check to a callback pattern similar to the variable sync
• OR: Add a global flag in the runtime that the CLI sets (e.g., langflags.interactive_mode)
• The runtime should not know about REPL specifics

---

3. Missing tmpstack Protection (CRITICAL BUG RISK)

Location: frontier-cli/repl_variables.c:648

While you correctly added exemptfromtmpstack() after hashassign(), there's a potential issue in the error path:

if (fl) {
    exemptfromtmpstack(&valcopy);  // ✅ Good
    count++;
} else {
    disposevaluerecord(valcopy, false);  // ⚠️ Potential issue
    log_warn(LOG_COMP_GENERAL, "Failed to assign variable %.*s",
             (int)bsname[0], bsname + 1);
}

Problem: If hashassign() fails, valcopy is on tmpstack and needs disposal. However, if copyvaluerecord() added it to tmpstack, this could cause issues depending on handle ownership.

Recommendation: Add comment explaining tmpstack ownership in error path, or verify copyvaluerecord behavior.

---

⚠️ High Priority Issues

4. Function Persistence Deferred Without Integration Test

Location: frontier-cli/repl_variables.c:627-631

You skip code values with a TODO, but I don't see integration tests that verify functions DON'T persist and document this as expected behavior.

Recommendation:

• Add integration test cases in tests/integration/test_cases/repl_sessions.yaml that explicitly test:
  • Function defined in one evaluation
  • Attempt to call it in next evaluation
  • Expected: error or undefined
• Mark these tests as documenting the current limitation
• Update PR description to reference these test cases

---

5. Address Value Creation Without Context Guard

Location: frontier-cli/repl_variables.c:909, frontier-cli/repl_variables.c:718

if (!setexemptaddressvalue(htable, zerostring, &focusval)) {

Concern: Per architectural anti-patterns, address value manipulation should use context guards to prevent mode stack corruption. While you're using setexemptaddressvalue(), which is correct, there's no verification that this is being called in a safe context.

Recommendation:

• Add comments explaining why context guards aren't needed here (likely because we're not navigating)
• OR: Add defensive checks that we're not in the middle of evaluation

Reference: docs/ARCHITECTURAL_ANTIPATTERNS.md - "Address values" section

---

6. Path Completion Logic Complexity

Location: frontier-cli/repl.c:252-402

The complete_slash_command_path() function has grown to 150 lines with nested conditionals and complex path resolution logic.

Issues:

• Difficult to test in isolation
• Combines multiple responsibilities (parsing, navigation, filtering)
• Has incomplete dotted-path logic (lines 319-324 with TODO comment)

Recommendation:

• Break into smaller functions:
  • complete_single_component_path()
  • complete_dotted_path()
  • add_table_completions()
• Add unit tests for completion logic (if test infrastructure supports it)
• Complete or remove the incomplete relative dotted-path code

---

🟡 Medium Priority Issues

7. Memory Leak Risk in build_wrapped_script

Location: frontier-cli/repl_variables.c:767-799

If sethandlesize() fails after newemptyhandle(), the handle is disposed. But there's no verification that all paths properly clean up.

Recommendation: Add comments confirming cleanup paths, or use RAII-style pattern.

---

8. Logging Levels Inconsistency

Location: Various in repl_variables.c

You use log_trace for entry points (good) but log_debug for important state changes. Per docs/LOGGING_STANDARDS.md:

• log_trace: Verbose flow tracking
• log_debug: Diagnostic info for debugging
• log_info: Important state changes

Issues:

• Line 600: "Syncing variables..." should be log_info (important state change)
• Line 732: "REPL variables initialized..." is correctly log_info
• Line 857: "Evaluating script..." could be log_info

Recommendation: Review logging levels for consistency with standards.

---

9. Test Coverage Gaps

Based on reviewing tests/integration/test_cases/repl_sessions.yaml:

Missing test cases:

1. Variables persisting across REPL evaluations with errors in between
2. Focus tracking verification (no tests confirm system.temp.FrontierREPL.focus is updated)
3. Tab completion behavior (completions are UI feature, hard to integration test, but document this)
4. msg() prefix behavior in REPL vs batch mode
5. Code value (function) persistence explicitly failing

Recommendation: Add test cases for #1, #2, #4, #5. For #3, document as manual testing only.

---

10. Comments vs Documentation

Location: frontier-cli/repl_variables.h:932-1010

The header file has excellent documentation. However, the implementation file has some complex logic that could use more explanation:

• Line 574-663: The callback function would benefit from a flow diagram in comments
• Line 767-799: Explain why we can't just eval with direct table push
• Line 882-925: Explain the target.set() crash issue more thoroughly

---

✅ Positive Observations

1. Excellent tmpstack hygiene: The exemptfromtmpstack() fix shows deep understanding of memory management
2. Good separation of concerns: Split into repl_variables.c rather than bloating repl.c
3. Comprehensive logging: Good use of structured logging throughout
4. Careful error handling: Most error paths are properly handled
5. Documentation: Header file documentation is exemplary
6. Integration tests exist: Good coverage of basic REPL functionality

---

🔧 Minor Issues / Style

11. C++ style comments: Mix of // and /**/ comments. Frontier codebase uses /**/ style.

    • frontier-cli/repl.c:174-175

12. Magic numbers: sizeof(completion) - 1 appears multiple times

    • Consider #define COMPLETION_BUF_SIZE 1024 for clarity

13. Long function: repl_sync_variables_callback() is 90 lines

    • Consider extracting "should skip variable" logic to helper

14. Whitespace changes: Several files have trailing whitespace changes unrelated to functionality

    • Common/source/langcallbacks.c has many whitespace-only changes

---

📋 Pre-Merge Checklist

Before merging, please address:

• BLOCKING: Remove global callback, use thread-local or registration pattern
• BLOCKING: Remove #include "../../frontier-cli/repl.h" from langverbs.c
• Add integration tests for function non-persistence
• Add integration tests for focus tracking
• Add integration tests for msg() prefix
• Document why context guards aren't needed for setexemptaddressvalue calls
• Complete or remove incomplete dotted-path completion code
• Review and adjust logging levels
• Add flow diagram comments for callback mechanism
• Convert // comments to /* */ style

---

🎯 Architecture Assessment

The overall approach is sound:

• Using callback for variable sync is creative
• Storage in system.temp.FrontierREPL is appropriate
• Focus tracking via address values is correct

However, the global callback violates architectural principles and must be fixed before merge.

---

📚 References

• docs/ARCHITECTURAL_ANTIPATTERNS.md - Global mutable state, Address values
• docs/LOGGING_STANDARDS.md - Logging level guidelines
• docs/TESTING_GUIDE.md - Integration test requirements
• ADR-005 - Thread-local parameter state pattern
• ADR-009 - Hash table stack restoration (relevant to function persistence issue)

---

Summary

This is solid work that adds valuable REPL functionality. The implementation shows good understanding of Frontier internals (tmpstack, hash tables, address values). However, the global callback introduces architectural debt that must be resolved before merge. With the blocking issues fixed, this will be a strong contribution.

Recommendation: Request changes, blocking on issues #1 and #2.

🤖 Generated with Claude Code - Bar Raiser Review

[jsavin]

Thanks for the thorough review!

Re: Issue #1 (Global Mutable State)

I agree this is architectural debt that should be addressed. However, converting the global callback to a thread-local or registration pattern is a larger refactoring effort that would expand the scope of this PR significantly. Could you please create a P0 issue to track this work? That way we can address it properly in a focused follow-up rather than rushing it as part of this PR.

Re: Issue #2 (Incorrect Include Path)

You're right - I'll fix this by moving the interactive mode check to a runtime flag that the CLI sets, removing the dependency on CLI headers from Common/.

Re: Other Issues

I'll address:

• Add integration tests for function non-persistence, focus tracking, and msg() prefix
• Add context guard documentation comments
• Fix logging levels
• Improve comments in repl_variables.c

Will push fixes shortly.

[claude]

Code Review - PR #371: REPL improvements

This PR introduces a well-designed persistent variables system for the REPL with focus tracking and improved tab completion. The implementation is clean and the architecture is sound. I have some recommendations for further improvements.

---

✅ Strengths

1. Excellent Architecture: The callback-based approach using langmagictabledisposecallback is clean and non-invasive. The flow diagram in repl_variables.c (lines 76-87) is exemplary documentation.

2. Proper Memory Management: Correct use of exemptfromtmpstack() after hashassign() (line 686) - this fixes the double-free crash mentioned in the PR description.

3. Good Defensive Programming:

   • Guards against syncing code values (functions) that can't yet be properly copied (lines 665-669)
   • Only syncs when g_repl_eval_active is true to avoid affecting non-REPL code
   • Skips "with" reference entries during sync (lines 654-660)

4. Focus Tracking Context Guard Analysis: The comment in repl_set_focus() (lines 932-940) correctly identifies why context guards aren't needed - this demonstrates understanding of architectural anti-patterns.

5. Comprehensive Test Coverage: Added 17 new integration tests covering variable persistence, focus tracking, msg() prefix, and error handling scenarios.

6. Logging Standards Compliance: All diagnostic output uses structured logging macros (log_trace, log_debug, log_info, etc.) - no fprintf(stderr) violations.

---

🔍 Issues & Recommendations

1. CRITICAL: Global Mutable State Violation

Location: Common/headers/langinternal.h:9, Common/source/langerror.c:105

extern boolean flreplmode;  /* Global flag */

Issue: This introduces global mutable state into the language runtime, which violates the architectural principle documented in docs/ARCHITECTURAL_ANTIPATTERNS.md. The document states:

"Global mutable state: Eliminate before launch, use thread-local or explicit context"

Why This Matters:

• Frontier is moving toward thread-safety and potential multi-threaded execution
• Global state prevents running multiple interpreters or REPL instances
• This flag is checked in langverbs.c:1152-1154 for every msg() call

Recommended Fix:
The proper solution is to add a field to an existing context structure. Looking at the codebase:

// Option 1: Add to existing TLS structure (if one exists)
// Option 2: Add to langcallbacks structure
// Option 3: Check if repl_is_active() directly from langverbs.c

// Preferred approach - call the REPL module directly:
extern boolean repl_is_active(void);  // Already exists in repl.h

// In langverbs.c:
if (repl_is_active()) {
    fputs("msg: ", stdout);
}

This keeps the flag encapsulated in the REPL module where it belongs, rather than leaking into the core runtime.

---

2. MEDIUM: Missing Documentation of Callback Contract

Location: Common/headers/langinternal.h:18-26

Issue: The callback typedef and comments are good, but the contract isn't fully specified.

Recommendations:

• Document that the callback is called with currenthashtable which will be disposed immediately after
• Specify that the callback must not modify the hash table structure (adding/removing nodes)
• Clarify that it's safe to read values and copy them elsewhere
• Document the relationship between hmagictable in langpushlocalchain (currently unused) vs. the callback in langpoplocalchain

---

3. MEDIUM: Tab Completion Code Complexity

Location: frontier-cli/repl.c:274-423

Issue: The complete_slash_command_path() function is 150 lines with deep nesting. The relative path resolution logic (lines 307-356) is particularly complex.

Recommendations:

• Extract helper functions:
  • complete_dotted_path()
  • complete_single_component()
  • try_relative_completion()
• Add comments explaining the search priority (currently only in function comment)
• Consider simplifying the nested path navigation logic

---

4. LOW: Inconsistent Error Handling

Location: repl_variables.c:741-750, 753-767

Issue: Initialization of target and focus have different error handling:

• target init: logs warning on failure, continues
• focus init: logs warning on failure, continues
• But if setaddressvalue() fails for focus, the value isn't properly cleaned up

Recommendation: Make error handling consistent and ensure proper cleanup in all paths.

---

5. LOW: Potential Memory Leak on Focus Assignment Failure

Location: repl_variables.c:963-972

Issue: If hashassign() fails, the code correctly disposes focusval. However, in the success case, there's no call to exemptfromtmpstack(), which is inconsistent with the pattern in repl_sync_variables_callback() line 686.

Question: Is setexemptaddressvalue() already handling tmpstack exemption internally? If so, add a comment. If not, this may leak tmpstack entries.

---

6. LOW: Magic Numbers in String Literals

Location: repl_variables.c:34-39

Issue: Pascal string length bytes are manually calculated:

static byte namerepltable[] = "\x0c" "FrontierREPL";  /* 12 chars */

Risk: If the string changes, the length byte could become incorrect.

Recommendation: Use a macro or compile-time assertion:

#define PSTRING(s) "\x" #s s
// Or use stringlength() assertions in init

---

🧪 Testing Observations

1. Good Coverage: 17 new tests cover the main scenarios well
2. Proper Handling of Known Limitations: Function persistence tests are properly marked as skipped with clear explanations
3. Error Resilience: Tests verify variables persist across syntax and runtime errors - excellent
4. Missing Edge Cases:
   • What happens if system.temp is deleted while REPL is running?
   • What if variables table is manually modified by user during REPL session?
   • Stress test: 1000+ variables in persistence

---

📊 Performance Considerations

Variable Sync Performance:
The sync callback (lines 642-696) walks the entire local hash table on every REPL evaluation. This is fine for typical REPL usage (< 100 variables), but could be noticeable with hundreds of variables.

Optimization Opportunity (low priority):

• Track which variables are new/modified instead of copying all
• Or use a dirty flag approach

Not critical for current use case, but worth noting for future optimization.

---

🔒 Security

No security concerns identified. The implementation:

• Properly validates table types before navigation
• Uses safe string operations
• Doesn't expose new attack surfaces

---

📝 Documentation

Excellent:

• Flow diagram in callback (repl_variables.c:76-87)
• Clear comments explaining "why" not just "what"
• Test descriptions document expected behavior

Could Improve:

• Add entry to docs/CLI_USAGE_GUIDE.md explaining persistent variables
• Document system.temp.FrontierREPL structure for users
• Mention in release notes that this is a breaking change if anyone was relying on non-persistent REPL behavior

---

✅ Verdict

This is high-quality work with a solid architectural foundation. The main issue is the global flreplmode flag which violates architectural guidelines.

Recommendation:

• MUST FIX before merge: Replace global flreplmode with module-local state (call repl_is_active() from langverbs.c)
• SHOULD FIX: Document the callback contract more thoroughly
• NICE TO HAVE: Refactor tab completion complexity

Once the global state issue is resolved, this PR will be ready to merge.

---

🎯 Suggested Next Steps

1. Remove flreplmode global and use repl_is_active() directly
2. Add documentation for users about the system.temp.FrontierREPL structure
3. Consider extracting tab completion helpers in a follow-up PR
4. Run full test suite to confirm no regressions

Great work overall! The persistent variables implementation is exactly what the REPL needed, and the code quality is very good. Just needs the architectural fix for global state.

[jsavin]

Response to Second Review

The second review recommends replacing flreplmode global with a call to repl_is_active() from langverbs.c.

However, this contradicts the first review's blocking issue #2:

BLOCKING: Incorrect Include Path - Core runtime code (Common/) should NEVER include CLI-specific headers. This creates backwards dependency and violates layering.

The original code had #include "../../frontier-cli/repl.h" in langverbs.c which was flagged as a blocking issue. The flreplmode flag was specifically added to fix this by:

1. Keeping the state in the runtime layer (Common/)
2. Having the CLI set the flag on REPL start/exit
3. Avoiding any CLI header includes in Common/

This is the correct architectural approach because:

• Runtime code should not depend on CLI implementation
• The flag pattern allows other frontends (GUI, etc.) to also set the mode
• It follows the same pattern as other runtime flags like fllangerror

The global state concern from the first review (issue #1 about langmagictabledisposecallback) is a separate issue that was acknowledged to need a P0 ticket for proper resolution. The flreplmode flag is a simple boolean that doesn't have the same threading concerns as the callback.

Recommendation: Keep the current flreplmode implementation as it resolves the blocking layering issue from review #1 while following established patterns in the codebase.