os phase 2

[navicore]

⏺ Phase 2 implementation complete. Summary of changes:

Runtime (crates/runtime/src/os.rs):

• patch_seq_path_exists - check if path exists
• patch_seq_path_is_file - check if path is a regular file
• patch_seq_path_is_dir - check if path is a directory

Compiler updates:

• ast.rs - added intrinsics: path-exists, path-is-file, path-is-dir
• builtins.rs - added stack effects: ( String -- Int ) for all three
• codegen.rs - added mappings and extern declarations (both locations)
• lib.rs - added exports for all OS functions (including Phase 1 exports that were missing)

Demo updated (examples/os/os-demo.seq):

• Tests all three new functions with ., a non-existent path, and home directory

All 489 tests pass, and the demo output shows:
Path operations:
'.' exists: yes '.' is directory: yes '.' is file: no

Non-existent path exists: no

Testing home dir: /Users/navicore
  exists: yes
  is directory: yes
  is file: no

[claude]

PR Review: OS Phase 2 Implementation

Summary

This PR implements three new path-checking intrinsics (path-exists, path-is-file, path-is-dir) as part of Phase 2 of the OS module. The implementation is clean, well-structured, and follows established patterns in the codebase.

---

✅ Code Quality & Best Practices

Strengths:

• Consistent implementation pattern: All three functions follow the same structure as existing OS functions (getenv, home-dir, current-dir), maintaining codebase consistency
• Proper documentation: Each function has clear rustdoc comments with stack effects documented
• Type safety: Proper error handling with panic on type mismatches
• Complete integration: All necessary touchpoints updated:
  • Runtime implementation (crates/runtime/src/os.rs:100-173)
  • AST intrinsics list (crates/compiler/src/ast.rs:390-392)
  • Stack effect signatures (crates/compiler/src/builtins.rs:688-716)
  • LLVM codegen mappings (crates/compiler/src/codegen.rs:110-112)
  • Extern declarations in two locations (crates/compiler/src/codegen.rs:664-666, 992-994)
  • Public exports (crates/runtime/src/lib.rs:122-125)

Minor observations:

• The unsafe blocks nest redundantly (outer function + inner block both marked unsafe). This is valid but the inner unsafe on line 110, 135, 160 could be removed since the entire function body is already in an unsafe context. Not a bug, just a style note.

---

✅ Potential Bugs or Issues

No critical bugs found. The implementation is sound:

• ✅ Proper stack manipulation with pop and push
• ✅ Type checking with descriptive panic messages
• ✅ Returns boolean values as Int (0 or 1) consistently
• ✅ Uses standard library Path::exists(), Path::is_file(), Path::is_dir() methods

Edge case consideration (informational):

• Symbolic links: Path::is_file() and Path::is_dir() follow symlinks and check the target. This is usually desired behavior, but worth documenting if users need symlink-aware checks in the future.
• The functions don't check for permission errors separately - if a path exists but isn't readable, exists() may still return true. This matches common filesystem API behavior.

---

✅ Performance Considerations

No performance concerns:

• ✅ Direct use of Rust standard library filesystem calls (minimal overhead)
• ✅ No unnecessary allocations
• ✅ Simple boolean checks with immediate return
• ✅ String handling uses existing Value::String infrastructure

These are lightweight syscall wrappers with negligible performance impact.

---

✅ Security Concerns

No security issues identified:

• ✅ No path traversal vulnerabilities (functions only check paths, don't modify filesystem)
• ✅ No command injection risks
• ✅ Input is type-checked (must be String)
• ✅ No buffer overflows or memory safety issues (safe Rust stdlib usage)
• ✅ Read-only operations - no potential for destructive actions

Note: These functions expose filesystem state, which is expected for OS intrinsics. The Seq language user is responsible for handling paths securely in their application code.

---

⚠️ Test Coverage

Good integration testing via demo, but missing unit tests:

What's tested:

• ✅ Demo program (examples/os/os-demo.seq:70-121) exercises all three functions
• ✅ Tests with current directory (".")
• ✅ Tests with non-existent paths
• ✅ Tests with home directory
• ✅ Validates all 489 existing tests still pass

What's missing:

• ⚠️ No dedicated unit tests in crates/runtime/src/os.rs or crates/runtime/tests/
• ⚠️ No integration tests in tests/integration/src/ (other OS functions like getenv, home-dir, current-dir also lack tests there)

Recommendation:
Consider adding unit tests for edge cases:

• Empty string paths
• Paths with special characters
• Very long path names
• Paths with null bytes (should panic gracefully)
• File vs directory distinction (test with actual temp files)

This would bring the test coverage in line with other modules that have unit tests (like file.rs, string_ops.rs, tcp.rs).

---

📝 Additional Notes

Good choices:

• ✅ Fixed missing Phase 1 exports in lib.rs (lines 122-125) - this was a cleanup bonus
• ✅ Demo updated with comprehensive examples
• ✅ Consistent naming: path-exists (not path-exists?) follows the convention of returning Int instead of using Scheme-style predicate naming

Overall assessment: This is a solid, production-ready implementation. The code is clean, follows established patterns, and all 489 tests pass. The only improvement would be adding dedicated unit/integration tests.

---

Recommendation

Approve with suggestion: The implementation is excellent. Consider adding unit tests in a follow-up PR to match the test coverage of similar runtime modules.

Great work! 🎉