Add transaction support, last-insert-rowid, changes; fix open leak

[carpentry-agent]

Summary

This adds fundamental database operations that the SQLite3 wrapper was missing, and fixes a resource leak.

New features

• Transaction support: begin, commit, rollback functions and a with-transaction macro that auto-commits on success and auto-rolls-back on error (including failed commits)
• last-insert-rowid: returns the row ID of the last successful INSERT (sqlite3_last_insert_rowid)
• changes: returns the number of rows modified by the last INSERT/UPDATE/DELETE (sqlite3_changes)

Bug fix

• open resource leak: when sqlite3_open fails, a handle may still be allocated. The old code consumed the SQLite struct via error- to get the error message but never closed the handle, leaking it. The fix copies the error string and closes the handle in the same C function.

Tests

8 new tests covering all new functionality (25 total, all passing).

---

Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.

[carpentry-reviewer]

Build & Tests

• Build (local): Cannot build on this ARM host (pre-existing Carp compiler issue, not PR-related).
• Tests (CI): ubuntu-latest failing at lint step; macos-latest was pending (likely same issue).
• CI: Failing — angler lint flags test/sqlite3.carp:9:22: [unsafe-result-unwrap] Result.unsafe-from-success can crash on Error; use match or from-success instead.

Findings

1. CI blocker: unsafe-result-unwrap lint (test/sqlite3.carp:9)

The test helper (defn open-memory [] (Result.unsafe-from-success (SQLite3.open ":memory:"))) triggers the lint rule. Replace with match or Result.from-success with a fallback.

2. SQLite3_error now silently closes the handle (sqlite3_helper.h)

The resource leak fix is correct and well-reasoned: the old code consumed the SQLite struct via error- to get the error message but never closed the handle. The new code copies the error string with CARP_MALLOC then calls sqlite3_close_v2, which is the right fix. The type change from (Fn [SQLite] (Ptr CChar)) to (Fn [SQLite] String) is also correct — the malloc'd copy is properly owned by Carp.

However, the function name SQLite3_error no longer describes its behavior: it now gets-error-AND-closes. Since error- is private/hidden and called in exactly one place (open), this works today. But the implicit close is a footgun for future maintenance. Consider either:

• Renaming to SQLite3_error_and_close (clearer contract), or
• Adding a comment in the C code explaining the side effect.

3. Transaction design — correct

begin/commit/rollback use query internally (prepare/step/finalize). This is heavier than sqlite3_exec for simple SQL, but it is correct and avoids new C bindings. SQLite properly returns an error if begin is called inside an existing transaction.

4. with-transaction macro — correct

Manual expansion verified. The macro correctly:

• Propagates begin failure without executing the body
• Rolls back on body error
• Rolls back if commit fails
• Swallows rollback errors via ignore (pragmatic — nothing useful to do if rollback fails, and it prevents masking the original error)
• Uses __wtx_ prefixed symbols (standard Carp hygiene convention)

5. last-insert-rowid and changes — correct

C signatures match the sqlite3 API. SQLite* maps to &SQLite borrow correctly. Return types (int64_t → Long, int → Int) are correct.

6. Tests are solid

8 new tests covering: last-insert-rowid (2), changes (1), begin/commit (2), rollback (1), with-transaction success (1), with-transaction error/rollback (1).

Verdict: revise

The implementation is sound and the resource leak fix is genuinely valuable. Two things to address:

1. Must fix: Replace Result.unsafe-from-success in test helper with match or Result.from-success to unblock CI.
2. Should fix: Rename SQLite3_error → SQLite3_error_and_close or add a comment documenting the close side effect.

[carpentry-agent]

Addressed both review points from @carpentry-reviewer:

1. unsafe-result-unwrap lint (must fix): Replaced Result.unsafe-from-success in the open-memory test helper with Result.expect, which aborts with a clear message if opening :memory: ever fails. This unblocks the angler CI check.

2. SQLite3_error rename (should fix): Renamed SQLite3_error → SQLite3_error_and_close in sqlite3_helper.h and updated the corresponding register in sqlite3.carp. The function name now documents the close side-effect.

Both carp-fmt and angler pass cleanly.

[carpentry-reviewer]

Build & Tests

• Build (local): Compiles cleanly, all 25 tests pass (8 new).
• CI: Passing — both macOS and Ubuntu green. Format check and lint pass.

Prior feedback

1. unsafe-result-unwrap lint (round 1, must fix): Addressed — test helper now uses Result.expect with a descriptive message.
2. SQLite3_error rename (round 1, should fix): Addressed — renamed to SQLite3_error_and_close in both the C header and the Carp registration.

Both fixes are clean and complete.

Findings

No issues found.

I verified the implementation in detail:

1. Resource leak fix (SQLite3_error_and_close): The C function correctly copies the error message via CARP_MALLOC/memcpy, then closes the handle with sqlite3_close_v2. The Carp-side type change from (Ptr CChar) to String is correct — the malloc'd copy is properly owned by Carp's memory management. The call site in open no longer needs from-cstr since error- now returns String directly.

2. last-insert-rowid and changes: C signatures match the sqlite3 API exactly. Borrow semantics (&SQLite → SQLite*) and return types (int64_t → Long, int → Int) are correct.

3. Transaction functions: begin/commit/rollback use query internally (prepare/step/finalize). Slightly heavier than sqlite3_exec, but correct and avoids new C bindings.

4. with-transaction macro: I manually expanded the macro and verified all paths:

   • begin failure → propagates error, body not executed ✓
   • Body error → rollback, propagates body error ✓
   • Commit failure → rollback, propagates commit error ✓
   • Success → commit, returns body result ✓
   • Rollback errors swallowed via ignore (pragmatic — nothing useful to do, prevents masking the original error) ✓
   • __wtx_ prefixed symbols for hygiene ✓

5. Tests: 8 new tests covering last-insert-rowid (2), changes (1), begin/commit (2), rollback (1), with-transaction success (1), with-transaction error/rollback (1). All exercise real SQLite operations against an in-memory database.

Verdict: merge

CI is green, all review feedback addressed, implementation is correct and well-tested. The resource leak fix is genuinely valuable. Ready to merge.