Add transactions, AEAD encryption, file modes, and TOML support - #676
Conversation
Red evidence for #664, #665, #666 and #667. Tests only — no implementation. Every suite fails because the feature does not exist: database_transaction_test parse error on `in transaction on db:` crypto_seal_test Undefined variable 'seal' filesystem_mode_test Undefined variable 'file_mode' / 'set_file_mode' toml_test Undefined variable 'parse_toml' The transaction suite is deliberately file-backed rather than in-memory: `open database` hands out a five-connection pool, and in-memory SQLite is special-cased to a single connection, which is exactly what hides #664 from a test suite. Adds the two dependencies the implementation will need (chacha20poly1305 for XChaCha20-Poly1305, toml for the parser) so the Red run compiles. Refs #664, #665, #666, #667 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL
Closes the gap in #665: the crypto stdlib could mint a key with `secure_random_bytes of 32` but had nothing to encrypt with it, so a program holding an API token that must be sent upstream later had only two options — store it in plaintext, or not store it. Hashing cannot stand in, because a stored credential has to be recoverable. store sealed as seal of token and key store token as unseal of sealed and key XChaCha20-Poly1305 rather than a 96-bit-nonce AEAD: the 192-bit nonce is wide enough that a fresh random nonce per message is safe with no counter and no caller-visible state, so the implementation owns nonce handling outright. Nonce reuse is the sharpest edge in an AEAD API and a natural-language surface is the last place to expose it. An optional third argument supplies associated data, binding a ciphertext to its context: `seal of token and key and "project:acme/api_key"`. The two- and three-argument forms are the same call, so the simple form is a strict subset of the expert one. Sealed values are `wflseal1:` plus hex of nonce, ciphertext and tag — self-describing, versioned so the algorithm can change later, and hex to match the key that `secure_random_bytes` hands back. `unseal` fails closed and reports every failure identically, so it cannot be used as an oracle to tell a wrong key from a modified byte from a mismatched context. Closes #665 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL
Closes #666. A program that writes a secret could not restrict the file, and — the half that matters just as much — could not verify it was restricted. The best available mitigation was external, a UMask in a service unit, which is invisible to the program and unverifiable from inside it. That made the common check "refuse to start if this config is group- or world-readable" impossible to express. store mode as file_mode of "config.toml" // "0600" store ok as set_file_mode of "config.toml" and "0600" Unix gets real POSIX semantics. Mode strings are parsed strictly: three or four octal digits, and anything else is an error rather than something quietly masked into a mode the author did not intend. Windows has no equivalent — modes do not map onto ACLs — so `file_mode` returns a documented approximation from the read-only attribute and `set_file_mode` raises an explicit unsupported error. A loud refusal beats a silent no-op that leaves the caller believing a file is protected. Closes #666 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL
Closes #667. WFL could read JSON but not TOML, which is what a large share of config files are actually written in. The reporter's options were to hand-roll a TOML subset in WFL — fragile to own, and progressively more wrong as it meets real TOML — or to change the file format; they changed the format and wrote down the deviation from spec. The surface mirrors the existing JSON one rather than the `to_toml` spelling the issue sketched, so the two formats read the same way: store config as parse_toml of file_contents store out as stringify_toml of config Two places TOML is not JSON, handled explicitly rather than fudged: * A TOML document is always a table, so `stringify_toml` accepts only an object and says so, instead of emitting something that will not parse back. * TOML has no null. Absence is a missing key, so a `nothing` value is omitted when serializing a table. Inside an array there is no way to leave a hole, so that is an error rather than a silent change of length. Whole numbers serialize as TOML integers, so a round-tripped config reads `port = 8080` and not `port = 8080.0`. Closes #667 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL
…gh execute Closes #664, which was a silent data-integrity failure rather than a missing feature. `open database` returns a five-connection pool and every query/execute takes whichever connection is free, so a hand-written `BEGIN` … `ROLLBACK` through `execute` ran each statement on a *different* connection: the rollback undid nothing, the writes survived, and nothing errored. In-memory SQLite is capped at one connection, so a program could pass its tests and lose data in production. Two halves, both needed. A real construct, which holds one connection for the whole block: in transaction on db: execute db with "INSERT ..." execute db with "UPDATE ..." end transaction It commits when the block finishes and rolls back if anything inside it fails. Reads inside the block run on the same connection, so they see the block's own uncommitted writes. `break`, `continue` and `return` are ordinary exits and commit — only an error rolls back. Nesting on one handle, and closing a database mid-transaction, are refused with an explanation rather than quietly doing something surprising. And a loud failure for the old workaround: `BEGIN`, `COMMIT`, `ROLLBACK`, `START TRANSACTION`, `SAVEPOINT` and `RELEASE` through query/execute now raise an error naming the block. Only the leading statement keyword is inspected, so a column named `begin_at` or a value containing the word "commit" still runs. This converts silent corruption into a message; no working program depended on the old behavior, because the old behavior did not work. `transaction` is deliberately not made a lexer keyword. It is recognized positionally, after a leading `in` and after `end`, so programs already using `transaction` as a variable name keep working. A statement beginning with `in` was previously always a parse error, so the syntax claims nothing that was valid before. Also registers the six new builtins from #665/#666/#667 in the two catalogs in builtins.rs and the static contracts in stdlib/typechecker.rs, which the runtime-inventory test holds to the runtime registrations. Closes #664 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL
Ships the documentation and end-to-end programs for #664, #665, #666 and #667 alongside the implementations. Docs: - Docs/04-advanced-features/databases.md gains a Transactions section covering the block, what it guarantees, the nesting and close-during restrictions, and why BEGIN/COMMIT through execute is now refused. - Docs/05-standard-library/crypto-module.md documents seal and unseal, leading with the distinction the issue turned on: hashing verifies a value you are handed, and cannot stand in for a credential you must send upstream later. - Docs/05-standard-library/filesystem-module.md documents file_mode and set_file_mode, including the refuse-to-start check that motivated them and the Windows behavior stated outright rather than implied. - Docs/05-standard-library/toml-module.md is new; the index and overview gain it and the module counts move from 11 to 12. - Both keyword references list `transaction` as a positional marker word that is never reserved, so the keyword total correctly stays at 181. Every example in these pages was run against the release binary rather than written from memory. Three claims did not survive that and were corrected: the databases examples had used a bare `execute db with ...` statement, which the parser reads as subprocess execution, so they now use the `store <name> as execute ...` form the language actually has; and the TOML page had shown output in the wrong key order. Writing TOML sorts keys alphabetically and drops comments, which is deterministic but not faithful to a hand-written file, so the page now says so. TestPrograms: - database_transaction_test.wfl, crypto_seal_test.wfl, file_mode_test.wfl and toml_test.wfl, run by the gated integration runner (it detects `describe` and adds --test, so a failed assertion exits nonzero). - The transaction program uses a file-backed SQLite database on purpose; in-memory SQLite is capped at one connection and cannot observe #664 at all. - The file-mode program runs on Windows too, where set_file_mode refuses by design, so it probes for support once and asserts whichever contract applies. Adds a dev diary entry and CHANGELOG entries for all four issues. Refs #664, #665, #666, #667 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL
…ncryption-permissions-toml-cpsmw9
![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
Here are some automated review suggestions for this pull request.
Reviewed commit: affd7eb82e
ℹ️ 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".
| if let Some(tx) = transactions.get_mut(handle_id) { | ||
| return database::run_query(database::DbTarget::Transaction(tx), sql, params).await; |
There was a problem hiding this comment.
Under main loop concurrently, all handler futures share this IoClient, so while one handler has a transaction open for a global database handle, an unrelated handler's ordinary query on that handle is automatically executed inside the first handler's transaction. Its writes can therefore be committed or rolled back by the wrong request; transaction ownership must include the handler/execution context rather than only handle_id.
AGENTS.md reference: AGENTS.md:L148-L151
Useful? React with 👍 / 👎.
| let mut transactions = self.db_transactions.lock().await; | ||
| if let Some(tx) = transactions.get_mut(handle_id) { | ||
| return database::run_query(database::DbTarget::Transaction(tx), sql, params).await; |
There was a problem hiding this comment.
When a transactional query is slow or stalled, this awaits the entire database operation while holding db_transactions; every concurrent db_query, db_execute, begin, commit, rollback, and close—including operations on unrelated handles—must wait for it. In a concurrent web loop, one slow database can therefore wedge otherwise independent requests, so transactions need per-handle synchronization or ownership that does not retain the global map lock across I/O.
AGENTS.md reference: AGENTS.md:L148-L151
Useful? React with 👍 / 👎.
| self.db_transactions | ||
| .lock() | ||
| .await | ||
| .insert(handle_id.to_string(), tx); |
There was a problem hiding this comment.
When two concurrent handlers enter a transaction on the same handle, both can pass the earlier contains_key check while their asynchronous database::begin calls are in flight, after which the second insertion replaces and drops the first transaction. Subsequent statements can then run on the wrong transaction, and one handler's commit leaves the other with "No transaction is open"; reserve the handle under one atomic state transition before awaiting the database.
AGENTS.md reference: AGENTS.md:L138-L144
Useful? React with 👍 / 👎.
| let first_word: String = sql | ||
| .trim_start() | ||
| .chars() | ||
| .take_while(|c| c.is_ascii_alphabetic()) | ||
| .collect(); | ||
|
|
||
| if first_word.is_empty() { | ||
| return Ok(()); |
There was a problem hiding this comment.
If transaction-control SQL begins with a normal SQL comment, such as -- start transaction\nBEGIN or /* transaction */ BEGIN, first_word is empty and this returns success. The BEGIN then goes through the pool—the exact path this guard is intended to prevent—so later statements may use different connections and writes intended for rollback can survive; inspect the first SQL token after whitespace and comments.
Useful? React with 👍 / 👎.
| ::toml::Value::Integer(i) => Value::Number(i as f64), | ||
| ::toml::Value::Float(f) => Value::Number(f), | ||
| ::toml::Value::Boolean(b) => Value::Bool(b), | ||
| ::toml::Value::Datetime(dt) => Value::Text(Arc::from(dt.to_string())), |
There was a problem hiding this comment.
For any TOML date, time, or datetime, parsing converts the value to ordinary WFL text, and wfl_to_toml later serializes every text value as a quoted TOML string. Thus parsing and rewriting released = 2026-07-31 produces released = "2026-07-31", silently changing the field's TOML type; retain enough type information to emit a TOML datetime again.
Useful? React with 👍 / 👎.
| fn toml_to_wfl(value: ::toml::Value) -> Value { | ||
| match value { | ||
| ::toml::Value::String(s) => Value::Text(Arc::from(s)), | ||
| ::toml::Value::Integer(i) => Value::Number(i as f64), |
There was a problem hiding this comment.
Valid TOML integers above 2^53 are not exactly representable as WFL's f64 number, so this cast silently changes values—for example, 9007199254740993 becomes 9007199254740992. A parse/edit/stringify cycle can therefore corrupt IDs or configuration values; preserve large integers in an exact representation or reject them instead of rounding.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| fn hex_to_bytes(hex: &str) -> Option<Vec<u8>> { | ||
| if !hex.len().is_multiple_of(2) { | ||
| return None; | ||
| } | ||
| (0..hex.len()) | ||
| .step_by(2) | ||
| .map(|i| u8::from_str_radix(&hex[i..i + 2], 16).ok()) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
🔴 Programs crash outright when a non-English character is used in an encryption key or sealed value
The key and sealed text are chopped into two-byte pieces (&hex[i..i + 2] at src/stdlib/crypto.rs:1043) without checking they are plain ASCII, so any value containing an accented or non-Latin character makes the whole program crash instead of reporting a bad key.
Impact: A user-supplied or misconfigured key/sealed string containing a multi-byte character aborts the running program (and any server it is hosting) rather than raising a catchable error.
Mechanism: byte-index slicing of a UTF-8 string at a non-character boundary
hex_to_bytes only validates that hex.len() (a byte count) is even, then slices &hex[i..i + 2] for each even byte offset. For a string such as "€a" the length is 4 bytes, so the loop runs, but byte offset 2 lands in the middle of the 3-byte €, and Rust's str indexing panics with "byte index 2 is not a char boundary".
Both entry points are reachable from WFL source: parse_seal_key (src/stdlib/crypto.rs:1050) passes the caller's key straight in, and native_unseal (src/stdlib/crypto.rs:1165) passes the hex body of the sealed value. Every other malformed input is handled by returning None → a clean RuntimeError, so this is the one path that escapes the module's own fail-closed contract.
| fn hex_to_bytes(hex: &str) -> Option<Vec<u8>> { | |
| if !hex.len().is_multiple_of(2) { | |
| return None; | |
| } | |
| (0..hex.len()) | |
| .step_by(2) | |
| .map(|i| u8::from_str_radix(&hex[i..i + 2], 16).ok()) | |
| .collect() | |
| } | |
| fn hex_to_bytes(hex: &str) -> Option<Vec<u8>> { | |
| let bytes = hex.as_bytes(); | |
| if !bytes.len().is_multiple_of(2) { | |
| return None; | |
| } | |
| bytes | |
| .chunks_exact(2) | |
| .map(|pair| { | |
| let text = std::str::from_utf8(pair).ok()?; | |
| u8::from_str_radix(text, 16).ok() | |
| }) | |
| .collect() | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| { | ||
| let mut transactions = self.db_transactions.lock().await; | ||
| if let Some(tx) = transactions.get_mut(handle_id) { | ||
| return database::run_query(database::DbTarget::Transaction(tx), sql, params).await; | ||
| } | ||
| } | ||
| let pool = self.get_database(handle_id).await?; | ||
| database::run_query(database::DbTarget::Pool(&pool), sql, params).await | ||
| } |
There was a problem hiding this comment.
🔴 A slow database statement inside a transaction freezes every other database operation in the program
The shared record of open transactions is locked for the entire time a statement runs (self.db_transactions.lock().await at src/interpreter/mod.rs:2549 and src/interpreter/mod.rs:2567), so while one slow statement inside a transaction is in flight, every other database read or write in the program — even on a completely different database — has to wait.
Impact: Under concurrent request handling one slow transactional query stalls unrelated handlers that touch any database.
Mechanism: tokio mutex guard held across the SQL await
In db_query/db_execute the guard returned by db_transactions.lock().await is still alive when database::run_query(...).await / run_execute(...).await is entered, because the return happens inside the if let Some(tx) = transactions.get_mut(handle_id) borrow. Every other call to db_query/db_execute, and begin_transaction/commit_transaction/rollback_transaction/close_database (src/interpreter/mod.rs:2478, 2500, 2522, 2533), must first acquire that same mutex, so they all block for the duration of the in-transaction statement.
The non-transactional path is unaffected (the guard is dropped at the end of the block before get_database), so the stall is specific to the new transaction routing. Docs/04-advanced-features/databases.md and tests/database_transaction_test.rs::transaction_does_not_block_other_handles_on_the_same_file assert that another handle stays usable, but that test is sequential and cannot observe the contention.
A fix is to keep the map lock short: take the transaction out of the map (or wrap each DbTransaction in its own per-handle mutex/Rc<RefCell<...>>) so the shared map is not held while awaiting SQL.
Prompt for agents
In src/interpreter/mod.rs, IoClient::db_query and IoClient::db_execute route statements to an open transaction by locking the shared `db_transactions` tokio Mutex and then awaiting the SQL call while still holding the guard. Because every other database entry point (db_query, db_execute, begin_transaction, commit_transaction, rollback_transaction, close_database) locks the same map, a long-running statement inside a transaction serializes all database activity in the interpreter, including work on unrelated handles. Under `main loop concurrently:` this means one slow handler blocks unrelated handlers, which the repository's testing policy (§11.3) explicitly targets. Restructure so the map lock is only held long enough to look up/borrow the per-handle transaction — e.g. temporarily remove the DbTransaction from the map for the duration of the statement and put it back afterwards (taking care to restore it on the error path), or store each transaction behind its own per-handle lock so the shared map guard is released before awaiting.
Was this helpful? React with 👍 or 👎 to provide feedback.
| Statement::TransactionStatement { db, body, .. } => { | ||
| self.mark_used_in_expression(db, usages); | ||
| for statement in body { | ||
| self.mark_used_variables(statement, usages); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Static analysis silently skips everything written inside a transaction block
Two of the analyzer's tree walks were never taught about the new transaction block (src/analyzer/static_analyzer.rs:1133 adds only the usage walk), so declarations and function calls written inside such a block are invisible to the unused-variable check and to the insecure-random-seeding security lint.
Impact: A call to the unsafe random-seeding builtin placed inside a transaction block escapes the security warning that would otherwise block it.
Mechanism: two sibling walkers missing the new statement arm
mark_used_variables gained a Statement::TransactionStatement arm, but:
collect_calls_in_statement(src/analyzer/static_analyzer.rs:199-300) has no arm, sorng_security_ingredients/check_insecure_rng_seedingnever seerandom_seedor the security-sensitive crypto builtins called insidein transaction on db: ... end transaction.collect_variable_declarations(src/analyzer/static_analyzer.rs:880-930) has no arm either, even though its comment says it deliberately mirrorsmark_used_variables; variables declared inside the block are therefore never tracked for the unused-variable diagnostic.
Both are false negatives (no spurious warnings), but the first is a security lint bypass.
Prompt for agents
src/analyzer/static_analyzer.rs has three AST walkers that must stay in sync for block-bearing statements: collect_calls_in_statement (used by rng_security_ingredients / the insecure-RNG-seeding lint), collect_variable_declarations (unused-variable analysis), and mark_used_variables. This PR added a Statement::TransactionStatement arm only to mark_used_variables, so calls and declarations inside `in transaction on db: ... end transaction` are invisible to the other two — notably letting a `random_seed` call inside a transaction block evade the security lint. Add matching recursion into the transaction body in the other two walkers.
Was this helpful? React with 👍 or 👎 to provide feedback.
| pub fn reject_transaction_control_sql(sql: &str) -> Result<(), String> { | ||
| let first_word: String = sql | ||
| .trim_start() | ||
| .chars() | ||
| .take_while(|c| c.is_ascii_alphabetic()) | ||
| .collect(); | ||
|
|
||
| if first_word.is_empty() { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let lowered = first_word.to_ascii_lowercase(); | ||
| if TRANSACTION_CONTROL_KEYWORDS.contains(&lowered.as_str()) { | ||
| return Err(format!( | ||
| "'{first_word}' controls a transaction, and sending it through query/execute does \ | ||
| not work: each statement runs on its own pooled connection, so the transaction \ | ||
| would not cover the statements you meant it to. Use a transaction block instead:\n\ | ||
| \n in transaction on db:\n execute db with \"...\"\n end transaction\n\ | ||
| \nThe block holds one connection for its whole body, commits when it finishes, \ | ||
| and rolls back if anything inside it fails." | ||
| )); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🟡 Existing programs that manage transactions with SQL text now fail where they previously worked
Statements whose first word is one of the transaction-control words are now refused outright (reject_transaction_control_sql at src/interpreter/database.rs:180), which breaks programs that used this pattern successfully against an in-memory database, with no deprecation period.
Impact: A working program that wraps writes in hand-written transaction statements stops running after upgrading, instead of being warned first.
Why the "no working program could depend on it" argument is incomplete
The PR justifies the hard rejection on the grounds that pooled connections made the old pattern a no-op. That is true for pooled backends, but connect caps in-memory SQLite at a single connection (src/interpreter/database.rs:99-103), so for sqlite::memory: — the configuration most WFL tests and examples use — BEGIN/COMMIT/ROLLBACK through execute genuinely worked. Those programs now raise a runtime error.
AGENTS.md/CLAUDE.md state that backward compatibility is sacred and that existing WFL programs must not break "without the documented deprecation path" in GOVERNANCE.md; the change ships as a straight rejection with no deprecation window. Consider warning (or accepting on single-connection SQLite) for a release before erroring, or documenting the break through the governance deprecation process.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Authenticated encryption | ||
| env.define_native("seal", native_seal); | ||
| env.define_native("unseal", native_unseal); |
There was a problem hiding this comment.
🔍 New security-relevant builtins are not in the insecure-RNG lint's sensitive list
seal/unseal are added to the crypto module but not to SECURITY_SENSITIVE_BUILTINS (src/analyzer/static_analyzer.rs:117-134), which drives the "program seeds the RNG and then does crypto" lint. In practice the nonce comes from rand::rng() (the OS CSPRNG), exactly like secure_random_bytes, so random_seed does not weaken it — but secure_random_bytes and hmac_sha256 are on that list for the same reason, so the omission is an inconsistency worth deciding on deliberately.
Was this helpful? React with 👍 or 👎 to provide feedback.
| match outcome { | ||
| Ok((value, control_flow)) => { | ||
| // The block finished without an error. `break`, `continue` | ||
| // and `return` are ordinary exits, not failures, so they | ||
| // commit too — the work inside completed. | ||
| self.io_client | ||
| .commit_transaction(&handle) | ||
| .await | ||
| .map_err(|e| RuntimeError::new(e, *line, *column))?; | ||
| Ok((value, control_flow)) | ||
| } |
There was a problem hiding this comment.
🔍 Control-flow exits from a transaction block commit, including exit
The commit path takes any Ok((value, control_flow)) (src/interpreter/mod.rs:7322-7331), which the docs describe for break/continue/return. Statement::ExitStatement also propagates as a non-error control flow, so exit inside a transaction block commits the partial work before the program stops. That may be the intended reading of "only errors roll back", but it is not covered by the docs or tests and is the one exit where a user is most likely to expect abandonment rather than commit.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn hex_to_bytes(hex: &str) -> Option<Vec<u8>> { | ||
| if !hex.len().is_multiple_of(2) { | ||
| return None; | ||
| } | ||
| (0..hex.len()) | ||
| .step_by(2) | ||
| .map(|i| u8::from_str_radix(&hex[i..i + 2], 16).ok()) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
🟥 Malformed hex input to seal/unseal panics the interpreter instead of failing closed
hex_to_bytes (src/stdlib/crypto.rs:1037-1045) validates only that the byte length is even and then slices the string at fixed two-byte offsets. Untrusted text — a key from configuration, or a sealed value read from a file, database, or HTTP request — containing a multi-byte UTF-8 character causes a panic ("byte index is not a char boundary") rather than the documented uniform unseal error. Both parse_seal_key (src/stdlib/crypto.rs:1050) and native_unseal (src/stdlib/crypto.rs:1165) feed caller-controlled text into it, so an attacker who controls a stored/sealed blob can crash the process.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Authenticated encryption (implemented in stdlib/crypto.rs) | ||
| "seal", | ||
| "unseal", |
There was a problem hiding this comment.
🟨 New encryption builtins are not covered by the insecure-RNG-seeding security lint
SECURITY_SENSITIVE_BUILTINS (src/analyzer/static_analyzer.rs:117-134) drives the lint that blocks a program which both calls random_seed and performs a cryptographic operation. The new seal/unseal builtins were added to src/builtins.rs and src/stdlib/typechecker.rs but not to this list, so a program that seeds the general RNG and then performs authenticated encryption is no longer flagged. Additionally, collect_calls_in_statement was not taught about the new transaction block, so any security-sensitive call written inside in transaction on db: ... end transaction escapes the lint entirely.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThis PR adds scoped database transactions, authenticated encryption, file-mode operations, and TOML parsing and serialization. It updates parser, interpreter, standard-library registration, typechecking, documentation, and tests for these features. ChangesDatabase transactions
Authenticated encryption
Filesystem modes
TOML support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…nd lint gaps Addresses the automated review on #676. Two findings were more serious than anything the original change caught, and one contradicted a claim I had made. **`seal`/`unseal` panicked on non-ASCII input.** `hex_to_bytes` checked that the byte length was even and then sliced the `str` at fixed two-byte offsets, which lands inside a multi-byte character and panics. Keys and sealed values are exactly the untrusted text this module promises to fail closed on — they arrive from config files, database rows and HTTP requests — so one accented character in a stored blob could abort a running server. Decoded over bytes now. **A transaction was owned by a database handle, not by the handler that opened it.** Under `main loop concurrently:` handlers share one IoClient and can name the same global handle, so an unrelated request running an ordinary `execute` was silently enrolled in another request's transaction and had its write rolled back with it. Transactions are keyed by `(scope, handle)`, where the scope is handler-local state swapped per poll by `InstalledRunState` alongside the existing count-loop and call-stack isolation. `tokio::task_local!` would not work here: handlers are futures in a `FuturesUnordered` on one thread, not separate tasks. The new test drives two real requests over a socket and asserts the bystander's row survives; removing the scope key fails it with both rows gone. Also: - The transaction map's lock was held across the SQL await, so one slow statement serialized every database operation in the program. Transactions now sit behind their own per-handle lock and the map's lock is held only long enough to clone an Arc. - `begin` had a check-then-insert race in which a second concurrent begin could replace and drop a live transaction. The handle is reserved under one atomic step before the round-trip, and the reservation is removed if begin fails. - A leading SQL comment defeated the transaction-control guard outright: `-- go\nBEGIN` read as an empty first word and went to the pool. Comments are skipped before the first token is read. - Two of the analyzer's three AST walkers had no arm for the new statement, so a `random_seed` call inside a transaction block escaped the insecure-RNG security lint and declarations inside one were invisible to unused-variable analysis. `seal`/`unseal` join SECURITY_SENSITIVE_BUILTINS. - `exit` inside a transaction committed its partial work, contradicting the rule that an abrupt stop discards it. It now rolls back, like a program that ends with a transaction still open. `break`/`continue`/`return` still commit. - The transaction arm is awaited behind a `Box::pin`. `execute_statement` is a plain async fn, so every await in every arm enlarges the one state machine each level of statement recursion keeps on the stack. - `fuzz/Cargo.lock` regenerated for the new dependencies; the fuzz job builds with `--locked`. Documentation corrections, all verified against the binary rather than asserted: - I wrote that rejecting hand-written transaction SQL broke nothing "because the old behavior did not work". That is wrong. In-memory SQLite is capped at one connection, so under `sqlite::memory:` the pattern genuinely worked — I confirmed it by building main and watching a rollback discard its row. It still ships as a hard error, deliberately, because the same program silently loses writes against any pooled backend; the changelog now records it under Removed and the docs say so outright instead of implying nothing is lost. - TOML dates round-trip as strings, not dates, and integers above 2^53 are rounded by WFL's f64 numbers. The page had claimed the round trip was exact. Both are now stated and pinned with tests. Refs #664, #665, #666, #667 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL
|
Pushed Fixed
On backward compatibility — this one I got wrong and the review was right. I'd claimed the rejection broke nothing "because the old behavior did not work". In-memory SQLite is capped at one connection, so under Documented rather than fixed (both now stated plainly and pinned with tests, replacing a doc claim that the round trip was exact):
Also: regenerated Verification: 2051 tests passing (0 failed), clippy clean at Still watching the Windows Generated by Claude Code |
|
CI status on Green: Build/Test/Clippy, Run WFL Programs (both OSes), Database Tests, Fuzz targets compile, Repository Hygiene (both OSes), Check formatting, CodeQL, config-lint, claude-review. Red: Integration Tests (Linux) — Why I'm not fixing it here. It's flaky, and the evidence is on this same commit: It's also untouched by this PR — no database, crypto, TOML or filesystem code is on that path. My changes to The likely mechanism, for whoever picks it up: the handler does Happy to fix it properly in a separate change if wanted. I don't have permission to re-run failed jobs from here, so re-running this one needs a maintainer. For the record on the earlier Windows failure I flagged: Generated by Claude Code |
…ncryption-permissions-toml-cpsmw9 # Conflicts: # Cargo.toml # fuzz/Cargo.lock
The merge with main combined this branch's `toml` dependency with #679's TLS change (warp's legacy `tls` feature dropped in favour of tokio-rustls 0.26). `fuzz/` is a separate workspace with its own lockfile and the fuzz CI job builds it with `--locked`, so it has to carry both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL
|
Rebased onto current Two conflicts, both in dependency files:
Re-verified against the merged tree rather than assuming the merge was harmless, since #679 touches the web server this branch also runs tests against:
Generated by Claude Code |
|
It isn't one. Reproducing the Windows condition on Linux with
Same threshold at every size. The mechanism is that Filed as #681 with the full reproduction and two suggested directions (box the remaining large arms; set Happy to take #681 next if you want it prioritised — it's the thing currently making the Windows gate flap. Generated by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (7)
src/stdlib/crypto.rs (2)
1167-1170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the extra indentation from the error text.
The continuation line inside the format string carries five extra spaces. Those spaces reach the user in the rendered message. Align the continuation with the other error strings in this module.
✏️ Proposed change
format!( "unseal: this is not a sealed value. Expected text beginning with \ - '{SEAL_V1_PREFIX}', as produced by `seal`." + '{SEAL_V1_PREFIX}', as produced by `seal`." ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stdlib/crypto.rs` around lines 1167 - 1170, Update the unseal error message in the format! call to remove the extra leading spaces from the continued string literal, so the rendered error text has no unintended indentation and matches the alignment used by other error strings in the module.
1111-1114: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueAn absent context and an empty context are the same value.
context.as_deref().unwrap_or("")mapsNoneandSome("")to the same associated data. A value sealed byseal of secret and keytherefore opens underunseal of sealed and key and "", and a value sealed with""opens with the context omitted. The documentation presents the context as a binding, so the equivalence should be explicit.Two options:
- Document the equivalence in
Docs/05-standard-library/crypto-module.mdand add a test that asserts it.- Distinguish the two cases by prefixing the AAD with a presence marker. That changes the sealed-value contract, so it needs a format version bump.
Keep the current behavior if it is intentional. State it either way.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stdlib/crypto.rs` around lines 1111 - 1114, Decide and document the intended equivalence between absent and empty context in the sealing/unsealing API. If intentional, document it in crypto-module.md and add a test covering both directions; otherwise, distinguish None from Some("") using a versioned sealed-value format and update corresponding seal/unseal handling.src/builtins.rs (1)
517-520: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive the TOML arities their own section header.
Line 520 registers the TOML functions under the
=== JSON FUNCTIONS ===header. Every other family in this match has its own header. Add one so the grouping stays readable.♻️ Proposed change
// === JSON FUNCTIONS === // Single argument functions "parse_json" | "stringify_json" | "stringify_json_pretty" => 1, + + // === TOML FUNCTIONS === + // Single argument functions "parse_toml" | "stringify_toml" | "stringify_toml_pretty" => 1,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/builtins.rs` around lines 517 - 520, In the arity match near the JSON function registrations, add a dedicated TOML functions section header before the parse_toml/stringify_toml arm, keeping the existing JSON header associated only with the JSON functions.tests/crypto_seal_test.rs (1)
204-237: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd one test that asserts the failure messages are identical.
Lines 3-6 state that every failure must be "rejected the same way", and
src/stdlib/crypto.rsfunnels hex, length, nonce, decrypt, and UTF-8 failures into a singlefailed()message. No test asserts that. A later change that splits those messages would still pass every test here.
expect_unseal_failurealready returns the error string, so the check is cheap.💚 Proposed additional test
/// The uniform-failure property itself: a tampered ciphertext, a tampered tag, /// and a truncated blob must be indistinguishable from each other. #[tokio::test] async fn every_unseal_failure_reports_the_same_message() { let sealed = seal_once("top secret").await; let body_len = sealed.split_once(':').unwrap().1.len(); let tampered_ciphertext = flip_hex_digit(&sealed, 60); let tampered_tag = flip_hex_digit(&sealed, body_len - 1); let truncated = sealed[..sealed.len() - 8].to_string(); let first = expect_unseal_failure(&tampered_ciphertext, "must fail").await; for (blob, what) in [ (tampered_tag, "tampered tag"), (truncated, "truncated blob"), ] { let other = expect_unseal_failure(&blob, "must fail").await; assert_eq!( first, other, "a {what} must be indistinguishable from a tampered ciphertext, \ or `unseal` becomes an oracle" ); } }As per coding guidelines: "Classify behavioral-change risk before testing; concurrency, cancellation, lifecycle, streaming, untrusted input, crypto/secrets, and backward compatibility are R3 and require negative or failure-path tests plus risk-triggered coverage."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/crypto_seal_test.rs` around lines 204 - 237, Add a Tokio test alongside the existing tampering tests that compares the error strings returned by expect_unseal_failure for tampered ciphertext, tampered tag, and a truncated sealed blob. Assert every failure message is identical, preserving the uniform failed() behavior across untrusted-input failure paths.Source: Coding guidelines
tests/transaction_handler_scope_test.rs (3)
130-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe interleaving depends on sleep margins.
The transaction holds for 600 ms and the client waits 200 ms before it sends
/plain. On a loaded CI machine,/plaincan arrive after the transaction has already rolled back. Both assertions still pass in that case, so the test silently stops exercising the concurrent window that the module comment describes.Consider a readiness signal instead of fixed sleeps, for example a
/tx-readymarker row that the harness polls before it sends/plain, or a longer hold inside the transaction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/transaction_handler_scope_test.rs` at line 130, Replace the fixed sleep-based coordination around the transaction hold in the concurrent test with a deterministic readiness signal, such as polling for a `/tx-ready` marker emitted while the transaction is active, before sending `/plain`. Ensure the marker is produced only after the transaction has acquired the intended state, preserving the test’s concurrent interleaving assertions.
126-135: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a response on the transaction success path.
The
/txhandler responds only in thewhen error:arm. If the transaction ever completes without an error, the handler sends no response. The client at Line 150 usesreqwest, which applies no request timeout by default, sotx.awaitat Line 173 would block instead of failing. A regression in transaction failure handling then appears as a hang, not as a test failure.Respond after
end transactionas well, and assert the failure text as today.♻️ Proposed success-path response
try: in transaction on db: store a as execute db with "INSERT INTO writes (tag) VALUES ('rolled-back')" wait for 600 milliseconds store boom as execute db with "INSERT INTO no_such_table (x) VALUES (1)" end transaction + respond to req with "tx-committed" when error: respond to req with "tx-rolled-back" end try🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/transaction_handler_scope_test.rs` around lines 126 - 135, Update the /tx handler’s transaction flow so it responds after end transaction on the success path, while retaining the existing "tx-rolled-back" response in the when error arm. Keep the test assertion validating the failure response text.
89-99: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the server join with a timeout.
shutdowndiscards the result of the/shutdownrequest and then joins the server thread without a bound. If the request fails or the WFL loop does not exit,server.join()blocks forever. The test then hangs until the harness kills it, which gives a weak failure signal.Wrap the join in
tokio::time::timeoutand fail with a clear message.♻️ Proposed bounded shutdown
async fn shutdown(port: u16, server: std::thread::JoinHandle<()>) { let _ = reqwest::Client::new() .get(format!("http://127.0.0.1:{port}/shutdown")) .send() .await; - match tokio::task::spawn_blocking(move || server.join()).await { - Ok(Ok(())) => {} - Ok(Err(panic)) => std::panic::resume_unwind(panic), - Err(join_err) => panic!("server join task failed: {join_err}"), + let join = tokio::task::spawn_blocking(move || server.join()); + match tokio::time::timeout(Duration::from_secs(30), join).await { + Ok(Ok(Ok(()))) => {} + Ok(Ok(Err(panic))) => std::panic::resume_unwind(panic), + Ok(Err(join_err)) => panic!("server join task failed: {join_err}"), + Err(_) => panic!("server thread did not exit within 30s after /shutdown"), } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/transaction_handler_scope_test.rs` around lines 89 - 99, Update shutdown to check the /shutdown request result and wrap the spawn_blocking server.join() operation in tokio::time::timeout with a suitable duration. Preserve panic propagation for thread and blocking-task failures, and panic with a clear timeout message when the server does not terminate promptly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Docs/04-advanced-features/databases.md`:
- Around line 163-181: Update the “Do not send BEGIN or COMMIT through execute”
section to state that the transaction-control guard rejects both query and
execute APIs. Add a query example or otherwise explicitly document query
rejection, while preserving the existing explanation of pooled connections and
accepted SQL containing these words.
In `@Docs/05-standard-library/crypto-module.md`:
- Around line 583-587: Update the WFL example around seal/unseal to initialize
project_key and api_token before sealing, retain the successful unseal
demonstration, and wrap the wrong-context unseal in a try/when error block so
validation exits successfully while showing the refusal. Add the displayed token
and refusal message as demonstrated, then validate the example with the required
WFL tools and docs-example tracking workflow.
In `@src/analyzer/static_analyzer.rs`:
- Around line 237-244: Update collect_calls_in_statement to handle
Statement::DatabaseQueryStatement by traversing its sql and parameters
expressions, including the corresponding execute operands, so calls are detected
both directly and inside Statement::TransactionStatement bodies. Add a
regression test covering random_seed and seal/unseal calls in these database
expressions.
In `@src/interpreter/mod.rs`:
- Around line 2486-2503: Update close_database to reject closing a handle when
any entry in db_transactions has the same handle_id, regardless of scope,
instead of only checking the (scope, handle_id) key. Preserve scope for
diagnostics if needed and report the scope holding the open transaction while
retaining the existing error behavior.
- Around line 6467-6483: Update the rollback-failure branch in the transaction
error handling to append the rollback message while preserving the original
err.kind, rather than constructing a RuntimeError with the default General kind.
Keep the original line and column and ensure Cancelled, Timeout, ResourceLimit,
and other error kinds continue driving handler classification correctly.
- Around line 6435-6442: The transaction registry must be drained when handlers
or interpreter runs are abandoned. Add a scope-synchronous cleanup helper that
removes entries from db_transactions and rolls back each live DbTransaction,
then invoke it from IsolatedHandler::drop, OutboundStreamCleanup::drop, and
interpret_inner’s pre-run cleanup while preserving normal commit/rollback paths.
In `@src/stdlib/toml.rs`:
- Around line 118-193: Update the TOML function registrations in register_toml
so stringify_toml and stringify_toml_pretty require map(Text, Any) inputs. Keep
parse_toml unchanged, and align both stringification overloads with
wfl_to_toml_document’s required top-level table shape so non-map values are
rejected during type checking.
In `@src/stdlib/typechecker.rs`:
- Around line 349-370: Update register_toml so stringify_toml and
stringify_toml_pretty are registered only with map(Type::Text, Type::Any) as
their argument type, matching wfl_to_toml_document’s top-level table
requirement. Retain Nothing, Boolean, Number, Text, and list(Type::Any) only as
nested TOML value types in the descriptive logic, not as function overloads.
In `@src/typechecker/mod.rs`:
- Around line 5853-5874: Update the TransactionStatement arm in
check_statement_types to call check_statement_block_with_completion(body) and
assign its result to self.current_statement_completion. Preserve the existing
database-type validation and shared-scope behavior, while ensuring the
transaction statement reports the body’s completion type instead of retaining
Type::Nothing.
In `@TestPrograms/crypto_seal_test.wfl`:
- Around line 24-28: Update the test assertion in “the sealed value does not
contain the plaintext” so it verifies that sealed does not contain the plaintext
string “correct-horse-battery-staple”; preserve the existing prefix assertion if
desired, but ensure the test’s stated property is explicitly checked.
In `@tests/database_transaction_test.rs`:
- Around line 593-612: Update the comment above the formatted test program to
remove the claim that `main loop concurrently` runs interleaved bodies, since
this test executes top-level statements serially. Retain the explanation that
the unrelated query must progress while the transaction remains open, which
would otherwise cause a timeout or deadlock, and leave the test code unchanged.
In `@tests/transaction_analyzer_walk_test.rs`:
- Around line 11-14: Remove the unused StaticAnalyzer symbol from the
static_analyzer import in transaction_analyzer_walk_test.rs, retaining
rng_security_ingredients and all other imports unchanged.
- Around line 60-75: Update calls_in_the_transaction_header_are_collected so the
transaction header expression itself contains one of the tracked calls,
replacing the bare db reference with an appropriate call expression while
retaining a separate tracked call in the body or following statement. Adjust
assertions as needed so the test specifically fails when the header expression
is not walked.
---
Nitpick comments:
In `@src/builtins.rs`:
- Around line 517-520: In the arity match near the JSON function registrations,
add a dedicated TOML functions section header before the
parse_toml/stringify_toml arm, keeping the existing JSON header associated only
with the JSON functions.
In `@src/stdlib/crypto.rs`:
- Around line 1167-1170: Update the unseal error message in the format! call to
remove the extra leading spaces from the continued string literal, so the
rendered error text has no unintended indentation and matches the alignment used
by other error strings in the module.
- Around line 1111-1114: Decide and document the intended equivalence between
absent and empty context in the sealing/unsealing API. If intentional, document
it in crypto-module.md and add a test covering both directions; otherwise,
distinguish None from Some("") using a versioned sealed-value format and update
corresponding seal/unseal handling.
In `@tests/crypto_seal_test.rs`:
- Around line 204-237: Add a Tokio test alongside the existing tampering tests
that compares the error strings returned by expect_unseal_failure for tampered
ciphertext, tampered tag, and a truncated sealed blob. Assert every failure
message is identical, preserving the uniform failed() behavior across
untrusted-input failure paths.
In `@tests/transaction_handler_scope_test.rs`:
- Line 130: Replace the fixed sleep-based coordination around the transaction
hold in the concurrent test with a deterministic readiness signal, such as
polling for a `/tx-ready` marker emitted while the transaction is active, before
sending `/plain`. Ensure the marker is produced only after the transaction has
acquired the intended state, preserving the test’s concurrent interleaving
assertions.
- Around line 126-135: Update the /tx handler’s transaction flow so it responds
after end transaction on the success path, while retaining the existing
"tx-rolled-back" response in the when error arm. Keep the test assertion
validating the failure response text.
- Around line 89-99: Update shutdown to check the /shutdown request result and
wrap the spawn_blocking server.join() operation in tokio::time::timeout with a
suitable duration. Preserve panic propagation for thread and blocking-task
failures, and panic with a clear timeout message when the server does not
terminate promptly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 89172486-68af-41cf-ab8c-6918549280a8
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockfuzz/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
CHANGELOG.mdCargo.tomlDocs/04-advanced-features/databases.mdDocs/05-standard-library/crypto-module.mdDocs/05-standard-library/filesystem-module.mdDocs/05-standard-library/index.mdDocs/05-standard-library/overview.mdDocs/05-standard-library/toml-module.mdDocs/reference/keyword-reference.mdDocs/reference/reserved-keywords.mdEngineering/evidence/red-664-667-transactions-aead-modes-toml.txtHistory/dev-diary/2026/2026-07-31-transactions-aead-file-modes-toml.mdTestPrograms/crypto_seal_test.wflTestPrograms/database_transaction_test.wflTestPrograms/file_mode_test.wflTestPrograms/toml_test.wflsrc/analyzer/mod.rssrc/analyzer/static_analyzer.rssrc/builtins.rssrc/interpreter/database.rssrc/interpreter/mod.rssrc/linter/mod.rssrc/parser/ast.rssrc/parser/mod.rssrc/parser/stmt/database.rssrc/parser/stmt/mod.rssrc/stdlib/crypto.rssrc/stdlib/filesystem.rssrc/stdlib/mod.rssrc/stdlib/toml.rssrc/stdlib/typechecker.rssrc/typechecker/mod.rstests/crypto_seal_test.rstests/database_transaction_test.rstests/filesystem_mode_test.rstests/toml_test.rstests/transaction_analyzer_walk_test.rstests/transaction_handler_scope_test.rs
| self.io_client | ||
| .begin_transaction(self.tx_scope.get(), &handle) | ||
| .await | ||
| .map_err(|e| RuntimeError::new(e, line, column))?; | ||
|
|
||
| // Statements inside share the enclosing scope, like `try:`, so a | ||
| // variable set inside the block is still readable after it. | ||
| let outcome = self.execute_block(body, Rc::clone(&env)).await; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: find every reader/writer of the transaction registry and any scope-based cleanup.
set -euo pipefail
rg -n -C 6 'db_transactions' --type=rust
rg -n -C 4 'tx_scope' --type=rust
# Cleanup sites that already reclaim handler-owned resources.
rg -n -C 6 'fn drop' --type=rust -g 'src/interpreter/mod.rs'Repository: WebFirstLanguage/wfl
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "tracked interpreter files:"
git ls-files | rg '(^|/)interpreter/mod\.rs$|src/interpreter' || true
echo
echo "search db_transactions:"
rg -n -C 8 'db_transactions' . || true
echo
echo "search tx_scope:"
rg -n -C 4 'tx_scope' . || true
echo
echo "search begin/commit/rollback_transaction:"
rg -n -C 6 'begin_transaction|commit_transaction|rollback_transaction' . || true
echo
echo "grep cleanup/drop in src/interpreter/mod.rs if present:"
if [ -f src/interpreter/mod.rs ]; then
rg -n -C 8 'fn drop|impl .*Drop|cleanup|reclaim|drain|isolate|handler' src/interpreter/mod.rs || true
fiRepository: WebFirstLanguage/wfl
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "IsolatedHandler and interpreter lifecycle context:"
sed -n '830,1030p' src/interpreter/mod.rs
sed -n '6080,6165p' src/interpreter/mod.rs
echo
echo "transaction implementation context:"
sed -n '2514,2610p' src/interpreter/mod.rs
sed -n '6430,6490p' src/interpreter/mod.rs
sed -n '7450,7465p' src/interpreter/mod.rs
echo
echo "all close_database callsites:"
rg -n -C 3 'close_database\(self\.tx_scope\.get\(\)' src/interpreter/mod.rsRepository: WebFirstLanguage/wfl
Length of output: 22488
Roll back abandoned transactions when the handler disappears.
IoClient::begin_transaction already reserves (scope, handle) before awaiting the handler; normal completion paths commit or rollback, but IsolatedHandler::drop, OutboundStreamCleanup::drop, and interpret_inner's pre-run cleanup do not drain db_transactions. Add a scope-synchronous rollback step that removes those entries and rolls back any live DbTransaction, so cancelled or dropped handlers do not leave pinned pool connections reachable from the registry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/interpreter/mod.rs` around lines 6435 - 6442, The transaction registry
must be drained when handlers or interpreter runs are abandoned. Add a
scope-synchronous cleanup helper that removes entries from db_transactions and
rolls back each live DbTransaction, then invoke it from IsolatedHandler::drop,
OutboundStreamCleanup::drop, and interpret_inner’s pre-run cleanup while
preserving normal commit/rollback paths.
| use wfl::analyzer::Analyzer; | ||
| use wfl::analyzer::static_analyzer::{StaticAnalyzer, rng_security_ingredients}; | ||
| use wfl::lexer::lex_wfl_with_positions; | ||
| use wfl::parser::Parser; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the public surface used by this test.
fd -t f 'static_analyzer.rs' src | while IFS= read -r f; do
echo "=== $f ==="
rg -n 'pub fn rng_security_ingredients|pub struct|seed_site|security_site' "$f"
done
# Check whether StaticAnalyzer is referenced anywhere in this test file.
rg -n 'StaticAnalyzer' tests/transaction_analyzer_walk_test.rs
# Check the check_unused_variables signature.
rg -n --type=rust -C2 'fn check_unused_variables' srcRepository: WebFirstLanguage/wfl
Length of output: 1578
Remove the unused StaticAnalyzer import.
StaticAnalyzer is imported in tests/transaction_analyzer_walk_test.rs, but this file only uses rng_security_ingredients. Since imports are checked by Clippy with unused_imports, keep only the symbol required by this test.
The other static-analyzer symbols are available under src/analyzer/static_analyzer.rs.
🧹 Proposed import fix
-use wfl::analyzer::static_analyzer::{StaticAnalyzer, rng_security_ingredients};
+use wfl::analyzer::static_analyzer::rng_security_ingredients;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use wfl::analyzer::Analyzer; | |
| use wfl::analyzer::static_analyzer::{StaticAnalyzer, rng_security_ingredients}; | |
| use wfl::lexer::lex_wfl_with_positions; | |
| use wfl::parser::Parser; | |
| use wfl::analyzer::Analyzer; | |
| use wfl::analyzer::static_analyzer::rng_security_ingredients; | |
| use wfl::lexer::lex_wfl_with_positions; | |
| use wfl::parser::Parser; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/transaction_analyzer_walk_test.rs` around lines 11 - 14, Remove the
unused StaticAnalyzer symbol from the static_analyzer import in
transaction_analyzer_walk_test.rs, retaining rng_security_ingredients and all
other imports unchanged.
Source: Coding guidelines
…s, lint gaps CodeRabbit reviewed the merged branch. Four findings were bugs I introduced, two of them fallout from the transaction-scoping fix in the previous round. **`close_database` only checked the caller's scope.** Keying transactions by `(scope, handle)` meant the guard that refuses to close a pool with a live transaction no longer saw transactions belonging to other scopes — so a handler could close the pool out from under another handler's transaction, which is the outcome the guard exists to prevent. It now matches the handle across all scopes; the scope parameter is gone from `close_database` since it was only misleading. **The transaction block never reported its completion type.** It used `check_statement_block`, which discards it, where `TryStatement` — the construct this block explicitly mirrors — uses `check_statement_block_with_completion`. An action whose body ends in a transaction was therefore inferred to return nothing, making `store n as call row_count` / `n times 2` a spurious "Multiply on Nothing and Number" error on a program that runs correctly. Reproduced before fixing; `tests/transaction_completion_type_test.rs` pins it with the `try:` shape alongside as a baseline. (The review also predicted a false error from a declared return type — not reachable, the parser always sets `return_type: None`.) **Static contracts for `stringify_toml` contradicted the runtime and the docs.** The JSON value set was registered, so `stringify_toml of 42` type-checked and then failed at runtime; `wfl_to_toml_document` accepts only a table, which the TOML page states and a test already asserted. Narrowed to `map(Text, Any)`. **A failed rollback flattened the error kind.** `RuntimeError::new` yields `General`, discarding `Cancelled`/`Timeout`/`ResourceLimit` — kinds that select `when` clauses and drive concurrent-handler classification, so a client disconnect inside a transaction with a failing rollback would have been counted as a structural handler failure. Uses `with_kind` and keeps `err.kind`. Also: - `collect_calls_in_statement` ignored `DatabaseQueryStatement`, so a `random_seed` or `seal` call in a SQL string or bound parameter escaped the security lint, transaction or not. - Docs said the transaction-control guard applies to `execute`; it applies to `query` too, now stated and shown. - The crypto context example could not have passed docs validation — undefined `api_token`, and a deliberately-failing call outside `try:`. Rewritten and run against the binary. - An empty context is now refused rather than being a synonym for "no context". Both mapped to the same associated data, so a context built from a missing config key produced an unbound ciphertext that looked bound. The review suggested documenting the equivalence; erroring matches how the rest of this module treats probably-mistaken input and removes the ambiguity instead. - Added the uniform-failure test the review asked for: tampered ciphertext, tampered tag and truncated blob must be indistinguishable, or `unseal` is an oracle. - Test quality: bounded the server join in the handler-scope test, added its missing success-path response, corrected a comment claiming concurrency a serial test does not exercise, and made the "does not contain the plaintext" TestPrograms case assert that property. Not changed: the review flagged the `StaticAnalyzer` import in `tests/transaction_analyzer_walk_test.rs` as unused. It provides `check_unused_variables`, and clippy runs `-D warnings`. Refs #664, #665, #666, #667 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL
|
Addressed the CodeRabbit round in Fixed
One decided differently. Absent and empty context were the same associated data ( One skipped. The Two scoped out, both worth their own change rather than being folded in here: rolling back transactions abandoned by a dropped handler ( Verification: 2064 tests passing (0 failed), clippy clean at Generated by Claude Code |
CodeRabbit's finding that `calls_in_the_transaction_header_are_collected` did not actually test the header was auto-marked resolved by e90c10d, but that commit *replaced* the weak test with two about database statement operands rather than strengthening it. Net effect: the header walk in `collect_calls_in_statement` lost its only test, weak as it was. The header takes a full primary expression — `in transaction on call get_db:` and `in transaction on get_db of 1:` both parse — so a security-sensitive call really can sit there, and the line is reachable rather than defensive. The new test puts the only crypto call in the header and keeps `random_seed` outside it, so it fails if the walk stops descending. Verified as a genuine Red by removing `collect_calls_in_expression(db, out)` and watching it fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL
2 participants
Summary
This PR closes four related issues (#664–#667) filed by a user building a configuration subsystem in WFL. Together they address critical gaps: atomic database operations, secret encryption, file permission control, and configuration file parsing.
Key Changes
#664: Database Transactions
BEGIN/ROLLBACKsent as SQL statements landed on different pooled connections, silently failing to roll back writes. In-memory SQLite (single connection) masked the bug; production databases exposed it.in transaction on db: ... end transactionblock syntax that pins a single connection for the entire blockBEGIN,COMMIT,ROLLBACK, etc.) sent throughquery/executewith a clear error messageDbTransactionenum anddb_transactionsmap toIoClientto track open transactions per handletests/database_transaction_test.rsandTestPrograms/database_transaction_test.wflusing file-backed SQLite (not in-memory) to catch the original defect#665: Authenticated Encryption (AEAD)
secure_random_bytes of 32minted keys with no way to use them for storing secrets that must be recovered laterseal of <plaintext> and <key>— XChaCha20-Poly1305 encryption with automatic nonce handlingunseal of <sealed> and <key>— decryption with authentication tag verificationwflseal1:prefix) for future algorithm changestests/crypto_seal_test.rsandTestPrograms/crypto_seal_test.wflcovering round-trip, tampering detection, and oracle resistance#666: File Permissions
0600or verify it was enforcedfile_mode of <path>— read permissions as 4-character octal string ("0600","0644", etc.)set_file_mode of <path> and <mode>— set permissions with strict octal parsingtests/filesystem_mode_test.rsandTestPrograms/file_mode_test.wflwith platform-aware assertions#667: TOML Support
parse_toml of <text>— read TOML into WFL objects/lists/scalarsstringify_toml of <object>— write WFL object back to TOMLstringify_toml_pretty of <object>— formatted outputparse_json,stringify_json,stringify_json_pretty) for consistencynothing/nullvalues omitted from tables (TOML has no null); error in arrays (no way to leave holes)src/stdlib/toml.rsmodule with full type mappingtests/toml_test.rsandTestPrograms/toml_test.wflImplementation Details
DbTransactionenum mirrorsDbPool(Postgres/MySql/Sqlite variants);DbTargetenum routes statements to pool or transactionhttps://claude.ai/code/session_015mrGwj9ef5QPoDfodampdL
Summary by CodeRabbit