Fix/audit jan 2026#3
Merged
Merged
Conversation
added 11 commits
March 6, 2026 14:45
map.get(field).cloned().unwrap() → unwrap_or(Value::Null) The preceding is_some() guard makes this structurally safe, but the bare unwrap() was a panic footgun if object state changed mid-eval. Closes audit finding #4.
…..") direct_result.unwrap() → expect() with diagnostic message. The two branches (needs_alloc / direct_result) are mutually exclusive so this is behaviorally safe, but panics should always explain themselves. Closes audit finding #4 (VM side).
Three loop-stack pop sites in While/Loop/For compile paths panicked if the loop stack was unexpectedly empty (compiler bug or malformed AST). Replace with proper CompileError returns to maintain the Result contract. Closes audit finding #12.
- int(bool): true→1, false→0 (was an error in --vm mode)
- sort(): add string comparison and custom comparator support
(was silently wrong for strings, custom fn not supported at all)
- keys({}): return [] for empty objects instead of VMError
- split(str, ""): split into individual chars when delimiter is empty
- is_some() / is_none(): implement real ADT-aware logic;
were stubs that always returned false, breaking all Option<T> checks
- satisfies(): keep as stub but add TODO(M3) comment and arity check
All 626 tests pass. Resolves audit findings #1, #5, #6, #7, #8.
…housekeeping CHANGELOG.md: - Fill in completely missing v0.4.0, v0.4.1, v0.4.2 entries - Add [Unreleased] entries for this PR's fixes - Follow Keep-a-Changelog format with correct categories CLAUDE.md: - Fix stale test count (577 → 626) - Add rule #9: VM parity is your responsibility - Add rules #13/#14: CHANGELOG and version bump discipline - Add CHANGELOG format reference - Document new learnings: GC borrow-in-closure pitfall, VM TryCatch/Destructure status, parity gap history
… pattern + silent failure)
Lowercase ok()/err() aliases that exist in the interpreter were dead code
in the VM because "Ok" | "Some" and "Err" appeared earlier in the match.
Fix: move lowercase aliases BEFORE the capitalized arms.
Also in this commit:
- float(str): add string-to-float parsing (parity with interpreter)
- entries({}): return [] for empty objects, not Null
- remove dead duplicate ok|Ok / err|Err arms at end of call_native
- find/flat_map: native implementations (drop the full Interpreter spawn)
- assert_ne, any, all, unique, sum, min_of, max_of: add all 7 missing builtins
All 626 tests pass.
The previous pattern:
block_in_place(|| handle.block_on(async { PG_CLIENT.with(|..| rt.block_on(query) }))
calls rt.block_on() inside an async block already being driven by
handle.block_on() — this is undefined behaviour / deadlock in Tokio.
Fix: extract a raw pointer to the client before entering block_in_place,
then await the query directly in the outer async block (no inner block_on).
Safety invariant: block_in_place suspends the current thread; the TLS
PG_CLIENT slot cannot be swapped while the raw pointer is live.
- interpreter/builtins.rs: sus() panic on no-arg call args.into_iter().next().unwrap() -> unwrap_or(Value::Null) - interpreter/mod.rs: 8x Mutex::lock().unwrap() in Environment Replace with poison-recovery: lock().unwrap_or_else(|p| p.into_inner()) Prevents panic propagation when a spawned thread panics mid-lock. - parser/parser.rs: decorators.pop().unwrap() in multi-decorator path Replace with ok_or_else(|| self.error(...)) to return ParseError instead of panicking on unexpected empty decorator list.
…ure) All unused functions in runtime.rs (rt_int_add, encode_value, decode_value, get_tag, etc.) are intentional — they are NaN-boxing JIT bridge functions prepared for Milestone 2. Add #![allow(dead_code)] at file top with comment.
…in registry Critical missing step from previous commit: vm/builtins.rs had the implementations but vm/machine.rs:register_builtins() is the actual registry that exposes names to Forge code. Without registration, all 7 builtins returned 'undefined variable' at runtime. Verified by running actual Forge programs with --vm: - assert_ne(1, 2) -> PASS - any([1..5], n>3) -> true - all([1..5], n>0) -> true - unique([1,2,2,3]) -> len 3 - sum([1..5]) -> 15 - min_of([3,1,4]) -> 1 - max_of([3,1,4]) -> 5 626 tests still pass.
Adds all fixes from second audit pass to [Unreleased]:
- ok/err lowercase alias shadowing fix
- float(str) parity
- entries({}) null vs []
- find/flat_map native implementations
- any/all/unique/sum/min_of/max_of/assert_ne builtins + critical registry fix
- pg.query/pg.execute nested block_on deadlock
- sus() no-arg panic
- parser decorators.pop().unwrap()
- 8x Mutex lock() poison recovery
- JIT dead_code allow note
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Lines 1308 to 1310 in 5067108
Enforce object type before producing entries output
This branch converts any non-object Value::Obj input (for example an array ref) into vec![], so entries(...) returns [] instead of an error. Because only non-Obj values hit the final Err, the function no longer enforces its own entries() requires an object contract for many invalid inputs, and it diverges from the interpreter entries behavior.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+349
to
+353
| (Value::Int(a), Value::Int(b)) => b < a, | ||
| (Value::Float(a), Value::Float(b)) => b < a, | ||
| (Value::Int(a), Value::Float(b)) => b < &(*a as f64), | ||
| (Value::Float(a), Value::Int(b)) => (*b as f64) < *a, | ||
| _ => false, |
There was a problem hiding this comment.
The new min_of/max_of logic falls back to _ => false when an element is not numeric, which means inputs like [1, "x"] silently return a value instead of failing. In the interpreter implementation (src/interpreter/builtins.rs, min_of/max_of), non-number elements raise a runtime error, so VM mode now masks invalid data and can produce incorrect aggregate results.
Useful? React with 👍 / 👎.
| if let ObjKind::Array(a) = &obj.kind { a.clone() } | ||
| else { return Err(VMError::new("min_of() requires an array")); } | ||
| } else { return Err(VMError::new("null array")); }; | ||
| if items.is_empty() { return Ok(Value::Null); } |
There was a problem hiding this comment.
Returning Null for an empty array here changes failure semantics in VM mode: min_of([]) (and the mirrored max_of([]) path) now succeeds with Null instead of signaling invalid input. The interpreter version requires a non-empty array and errors, so this introduces a VM/interpreter behavior split that can hide bad inputs and alter downstream control flow.
Useful? React with 👍 / 👎.
3 tasks
This was referenced Apr 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What does this PR do?
Changes
Testing
cargo testpassesforge testpassesRelated Issues
Closes #