Conversation
…ted: Include System Implementation Files Changed/Created: - compiler/src/ast.rs: Added SourceLocation, Include enum, updated Program and WordDef - compiler/src/parser.rs: Added parse_include() to handle include std:name and include "path" - compiler/src/resolver.rs: New module with include resolution and collision detection - compiler/src/lib.rs: Integrated resolver into compilation pipeline - docs/INCLUDE_SYSTEM.md: Design document Features: 1. Standard library includes: include std:http loads from stdlib/http.seq 2. Relative includes: include "my-utils" loads ./my-utils.seq relative to current file 3. Collision detection: Clear error when same word is defined in multiple files 4. Double-include prevention: Files included multiple times are silently deduplicated 5. Recursive includes: Included files can include other files Tested: - include std:stack-utils - used 2dup from stdlib, output: 6 - include "my-utils" - used double and triple from local file, output: 10, 15 - Collision detection - clear error message showing both file locations Known Issues (Pre-existing): - stdlib/math.seq has parameter names (min, max) that the parser interprets as type names - stdlib/http.seq has some type checking issues when functions are combined All 144+ tests pass and there are no clippy warnings.
⏺ Write(examples/include_demo/http_simple.seq)
⎿ Wrote 56 lines to examples/include_demo/http_simple.seq
# Simple HTTP Server using stdlib
#
# This example demonstrates using the std:http library to build
# a minimal HTTP server with less boilerplate.
#
# Usage:
# seqc --output http_simple http_simple.seq
# ./http_simple
# curl http://localhost:8080/
… +46 lines (ctrl+o to expand)
⏺ Bash(./target/release/seqc --output /tmp/http_simple /Users/navicore/git/navicore/patch-seq/examples/include_demo/http_simple.seq 2>&1)
⎿ Compiled /Users/navicore/git/navicore/patch-seq/examples/include_demo/http_simple.seq -> /tmp/http_simple
⏺ Both examples compile. Created examples/include_demo/ with:
- main.seq - Demonstrates include std:math and include std:stack-utils with various function calls
- http_simple.seq - A minimal HTTP server using include std:http, showing http-ok, http-not-found, and http-response
The HTTP example also uses the fixed http-response function to show custom status codes (201 Created).
Pull Request Review: Modules SystemOverviewThis PR implements a module/include system for Seq, adding the ability to import code from the standard library and relative file paths. The implementation is solid and follows the design documented in ✅ Strengths1. Well-Designed Architecture
2. Good Code Quality
3. Thoughtful Features
🐛 Potential Issues1. Line Number Tracking Not Implemented (Critical for UX)Location: Without line numbers, collision errors will show Recommendation: Track line numbers during parsing or document this limitation prominently. 2. Test Coverage: Missing Integration TestsCurrent State: Only unit tests in Missing Coverage:
Recommendation: Add integration tests that create temp files and run the compiler. 3. Parser Edge CasesLocation: The
4. Security: Path TraversalLocation: No validation against path traversal attacks like if rel_path.contains("..") {
return Err("Include paths cannot contain ..".to_string());
}5. Error MessagesLocation: Multiple places using Error messages like "Failed to canonicalize" may be unclear to users. Consider user-friendly wording like "Include file not found or inaccessible". 6. Test Name MismatchLocation:
🎯 Performance Considerations
🔒 Security AssessmentOverall: Low Risk - This is a compiler feature on trusted code. Main concern is path traversal (see issue #4). 📝 Documentation & ExamplesStrengths:
Suggestions:
🧪 Testing RecommendationsHigh Priority:
Medium Priority:
✅ What Works Well
🎯 RecommendationConditional Approval - This is a solid implementation of an important feature. Main gaps: Before Merge (High Priority):
After Merge (Can be follow-up PRs):
🎉 ConclusionThis is a well-designed and thoughtfully implemented feature. The code quality is good, error handling is solid, and it enables important functionality. With integration tests and a few security improvements, this is ready to merge. Great work! The include system makes the standard library much more practical. 🚀 Review by Claude Code |
Pull Request Review: Module/Include SystemSummaryThis PR introduces a module system for Seq with ✅ Strengths1. Clean Architecture
2. Good Error Handling
3. Documentation
4. Test Coverage
🔍 Issues & RecommendationsCRITICAL: Path Traversal Security RiskLocation: Include::Relative(rel_path) => {
let path = source_dir.join(format!("{}.seq", rel_path));
if !path.exists() {
return Err(format!(
"Include file '{}' not found at {}",
rel_path,
path.display()
));
}
Ok(path)
}Issue: The code doesn't prevent path traversal attacks. A malicious While these specific files won't parse as Seq code, this could:
Recommendation: Add path validation before resolving: Include::Relative(rel_path) => {
// Prevent path traversal
if rel_path.contains("..") || rel_path.starts_with('/') {
return Err(format!(
"Invalid include path '{}': relative paths cannot contain '..' or start with '/'",
rel_path
));
}
let path = source_dir.join(format!("{}.seq", rel_path));
// Ensure the resolved path is still within source_dir or a subdirectory
let canonical = path.canonicalize().map_err(|e| {
format!("Failed to resolve include path {}: {}", path.display(), e)
})?;
let canonical_source = source_dir.canonicalize().map_err(|e| {
format!("Failed to canonicalize source directory: {}", e)
})?;
if !canonical.starts_with(&canonical_source) {
return Err(format!(
"Include path '{}' is outside the source directory",
rel_path
));
}
if !canonical.exists() {
return Err(format!(
"Include file '{}' not found at {}",
rel_path,
canonical.display()
));
}
Ok(canonical)
}Bug: Missing Line Number TrackingLocation: word.source = Some(SourceLocation {
file: source_path.clone(),
line: 0, // TODO: Track actual line numbers
});Issue: The TODO indicates line numbers aren't being tracked, which defeats much of the purpose of the Impact: Makes debugging collisions significantly harder. Recommendation:
Alternatively, if this is a known limitation for the initial release, document it clearly in the PR description. Code Quality Issues1. Inconsistent Naming ConventionLocation: The function uses if let Ok(exe_path) = std::env::current_exe()
&& let Some(exe_dir) = exe_path.parent()
{Issue: This will fail to compile on stable Rust. The Recommendation: Rewrite using nested if let Ok(exe_path) = std::env::current_exe() {
if let Some(exe_dir) = exe_path.parent() {
// ...
}
}2. Unused Test Case LogicLocation: The test #[test]
fn test_collision_detection_same_file_allowed() {
// Same word in same file is fine (parser should catch true duplicates)
// ...
// This IS a collision - same name in same file on different lines
// (though typically parser would catch this as a parse error)
let result = check_collisions(&words);
assert!(result.is_err());
}Issue: The test name suggests same-file collisions are allowed, but the test expects an error. The comments are contradictory. Recommendation: Either:
Performance Considerations1. Redundant CanonicalizationLocation: The code canonicalizes paths multiple times: let source_path = source_path.canonicalize()?; // Line 36
// ...
let canonical = included_path.canonicalize()?; // Line 64Impact: File system calls on every include. For deep include trees, this could add up. Recommendation: Consider caching canonicalized paths in the 2. Multiple String Allocations in Error MessagesLocation: for (name, locations) in definitions {
if locations.len() > 1 {
let mut msg = format!("Word '{}' is defined multiple times:\n", name);
for loc in &locations {
msg.push_str(&format!(" - {}\n", loc));
}
msg.push_str("\nHint: Rename one of the definitions to avoid collision.");
errors.push(msg);
}
}Recommendation: Pre-allocate string capacity or use a more efficient string builder approach for large numbers of collisions. Test Coverage GapsMissing Test Cases:
Recommendation: Add integration tests in a #[test]
fn test_include_stdlib_module() {
// Create temp stdlib, compile file that includes it
}
#[test]
fn test_include_relative_path() {
// Create temp project with multiple files
}
#[test]
fn test_circular_include_error() {
// Should detect and report circular dependencies
}Documentation Issues1. Outdated Comments in stdlibLocation: Both files still reference "cem3" instead of "Seq": And say: Issue: This PR literally adds the module system, making these comments incorrect. Recommendation: Update to: 2. Missing Documentation in ASTLocation: The new Recommendation: Add doc comments: /// Source location for error reporting and collision detection.
/// Used to track where word definitions originate when merging
/// multiple files via the include system.
#[derive(Debug, Clone, PartialEq)]
pub struct SourceLocation {
/// Source file path
pub file: PathBuf,
/// Line number in source file (0-indexed)
pub line: usize,
}🎯 Additional ObservationsPositive Design Choices:
Future Considerations:Per
These are wisely deferred for now. Get the basics right first. 📋 Action ItemsMust Fix (Blocking):
Should Fix (Pre-merge):
Nice to Have (Post-merge):
🎉 ConclusionThis is a solid implementation of a module system that follows good software engineering practices. The architecture is clean, the design is well-documented, and the feature works as advertised. However, the path traversal security issue is critical and must be fixed before merge. The unstable Rust feature should also be addressed for compatibility. With those fixes, this PR will be a valuable addition to Seq. Nice work! 🚀 Overall Assessment: Approve with required changes (security fix + stable Rust compatibility) |
From the review, here's what I addressed:
| Issue | Status | Notes
|
|--------------------------------|------------------|-------------------------------------------------------------------------------------------------
--------|
| Path traversal security | ✅ Fixed | Added validation to reject .. and absolute paths starting with /
|
| let_chains unstable syntax | ❌ Not an issue | let_chains was stabilized in Rust 1.88, we're on 1.91.1
|
| stdlib comments "cem3" → "Seq" | ✅ Fixed | Updated stdlib/math.seq and stdlib/stack-utils.seq
|
| Misleading test name | ✅ Fixed | Renamed test_collision_detection_same_file_allowed →
test_collision_detection_same_file_different_lines |
| Line number tracking | Known limitation | Already has TODO in code, not blocking
|
| Integration tests | Future work | Good suggestion but not blocking for merge
|
Files Modified:
- compiler/src/resolver.rs - Path traversal validation + test name fix
- stdlib/math.seq - Updated comments to reference Seq and include syntax
- stdlib/stack-utils.seq - Updated comments to reference Seq and include syntax
Verified:
- All tests pass
- Path traversal blocked: include "../etc/passwd" → "paths cannot contain '..'"
- Examples still compile and run correctly
PR Review: Module System ImplementationThis PR implements a module/include system for Seq, adding support for include std:name and include "path". The implementation adds 774 lines. Strengths
Critical Issues1. SECURITY: Path Traversal Bypass (HIGH PRIORITY)Location: resolver.rs:112 The current check only looks for literal ".." substring which can be bypassed:
Recommendation: After constructing the path, verify it is still within allowed directory using canonical paths and starts_with check. 2. Windows Path Handling (MEDIUM PRIORITY)Location: resolver.rs:118 The check for absolute paths only catches Unix-style paths starting with /. On Windows, absolute paths can start with C:\ or \. Recommendation: Use Path::is_absolute() instead of manual string checking. 3. Missing Line Number TrackingLocation: ast.rs:51-53, resolver.rs:50-52 Line numbers are set to 0 (TODO comment). Collision errors will show file.seq:0 instead of actual line numbers. 4. Missing TestsNo tests for recursive includes, circular includes, invalid stdlib modules, file I/O errors, Windows paths, or deduplication. Security Summary
Checklist Before Merge
Overall AssessmentThis is solid foundational work with good architecture and error handling. The primary concern is the path traversal security check needs hardening before merge. Recommendation: Approve with required changes - fix the security issues, then merge. Great work on collision detection and stdlib location logic! |
The updated resolver.rs now has three layers of protection:
1. Early rejection of .. (line 112-116)
if rel_path.contains("..") {
return Err("paths cannot contain '..'");
}
2. Cross-platform absolute path detection (line 119-126)
let rel_as_path = std::path::Path::new(rel_path);
if rel_as_path.is_absolute() { // Works on Unix AND Windows
return Err("paths cannot be absolute");
}
3. Canonical path verification (line 137-151)
let canonical_path = path.canonicalize()?;
let canonical_source = source_dir.canonicalize()?;
if !canonical_path.starts_with(&canonical_source) {
return Err("resolves outside the source directory");
}
This third layer is the defense-in-depth the review asked for - it catches any bypass attempts (symlinks, encoded paths, multiple slashes, etc.) by
verifying the final resolved path is actually within the allowed directory.
- All tests pass
- Include demo works
- Path traversal blocked
Code Review - PR #24: Module SystemSummaryThis PR implements a module/include system for the Seq language, allowing code reuse via StrengthsArchitecture & Design
Code Quality
Issues & Concerns1. Missing Line Number Tracking (Medium Priority)Issue: Source locations are tracked but always set to line 0 (resolver.rs:53), making collision error messages less helpful. 2. Test Coverage Gap (High Priority)Issue: No integration tests for the include system. Missing test cases for include resolution, error handling, and parser edge cases. 3. Include Cycle Prevention (Verified Working)The code DOES handle cycles correctly by marking files as included BEFORE processing their dependencies. 4. Parser String Handling (Low Priority)The parser doesn't handle escaped quotes within include paths (parser.rs:86-88). 5. Stdlib Location Search (Low Priority)Using current directory as fallback could be surprising if a random directory has a stdlib/ folder. Best Practices Observed
Recommendations SummaryMust Fix (before merge)
Should Fix (near-term)
Nice to Have (future)
Suggested Test CasesAdd to compiler/tests/include_tests.rs:
ConclusionThis is a solid implementation that follows the design spec well. The architecture is clean, security is good, and code quality is high. Main gaps:
Once tests are added, this should be safe to merge. Great work on security and error handling! |
PR Review: Modules/Include SystemOverviewThis PR implements a solid foundation for an include/module system for the Seq language. The implementation is well-structured and follows good design principles. Here's my detailed review: ✅ StrengthsCode Quality & Design
Documentation
🔍 Issues & Suggestions1. Missing Line Number Tracking (Minor)Location: line: 0, // TODO: Track actual line numbersImpact: Error messages won't show exact line numbers for collision detection 2. Parser State Management (Minor)Location: The Current code: if token == "std" {
if !self.consume(":") { ... }
let name = self.advance()...
}Consideration: This works but couples the parser to tokenizer behavior. Document this assumption or consider handling 3. Incomplete Test Coverage (Moderate)Location: The unit tests cover:
Missing test cases:
Suggestion: Add integration tests that actually exercise the file system operations. The current tests only verify collision detection logic. 4. Windows Path Handling (Minor Security Consideration)Location: let rel_as_path = std::path::Path::new(rel_path);
if rel_as_path.is_absolute() {
return Err(...);
}Issue: On Windows, paths like Suggestion: Add a test case for Windows absolute paths (even if running on Linux, you can construct PathBuf instances to test the logic). 5. Stdlib Discovery Robustness (Moderate)Location: The Issue: If if let Ok(path) = std::env::var("SEQ_STDLIB") {
let path = PathBuf::from(path);
if path.is_dir() {
return Ok(path);
}
return Err(...); // ❌ Should this be a hard error or warning?
}Consideration: Should an invalid 6. Parser Include Placement (Minor Design)Location: Includes can appear anywhere in the source file (between word definitions). This might lead to confusing code. Current behavior: Allows: Consideration: Consider requiring all includes at the top of the file for consistency. The spec in 7. let-else Syntax (Compatibility Warning)Location: if let Ok(exe_path) = std::env::current_exe()
&& let Some(exe_dir) = exe_path.parent()
{This uses 🔒 Security AnalysisOverall: Good security practices ✅ Path traversal protection: Multiple layers
✅ No arbitrary file system access: Limited to source directory and stdlib
🧪 Test Coverage AssessmentCurrent state: Basic unit tests exist, but no integration tests Recommendations:
📊 Performance ConsiderationsCurrent implementation: Reads and parses each file synchronously Observations:
Future optimization: Could parallelize parsing of independent includes, but not needed yet. 🎯 Recommendations SummaryMust Fix (Blockers)None - this PR is ready to merge Should Fix (Before Merge)
Could Fix (Follow-up PRs)
✨ Overall AssessmentThis is high-quality work that:
The main gap is test coverage - adding integration tests would make this excellent. Recommendation: ✅ Approve with minor suggestions Great job on the implementation! The security considerations and error handling show careful thought about edge cases. |
1 participant
No description provided.