Conversation
Add StorageHook trait, task-local context bridge, and hooked storage wrappers that intercept all storage operations with before/after hooks. What changed: - data_connector/src/hooks.rs: new StorageHook trait with before()/after() methods, StorageOperation enum (16 variants), BeforeHookResult (Continue/Reject), ExtraColumns type alias, HookError - data_connector/src/context.rs: RequestContext (per-request key-value bag) and ExtraColumns task-local bridge via tokio::task_local!; exposes with_request_context/current_request_context and with_extra_columns/current_extra_columns - data_connector/src/hooked.rs: HookedConversationStorage, HookedResponseStorage, HookedConversationItemStorage decorator wrappers; write operations scope ExtraColumns via task-local around inner backend calls; hook errors are non-fatal (logged, operation continues) unless explicitly Rejected - data_connector/src/factory.rs: StorageFactoryConfig gains hook: Option<Arc<dyn StorageHook>>; create_storage() wraps backends in Hooked*Storage when hook is provided - data_connector/src/lib.rs: module declarations and re-exports for context, hooks, hooked modules - model_gateway/src/app_context.rs: pass hook: None to StorageFactoryConfig Why: Storage hooks allow injecting custom logic (audit logging, multi-tenancy, PII redaction, validation) before and after storage operations without forking the codebase. The task-local bridge avoids changing any public storage trait signatures while still passing hook-provided data to backends. How: Decorator pattern wraps inner backends. Task-local storage bridges ExtraColumns from hooks to backends without trait signature changes. Hook errors use warn!+continue semantics to prevent hooks from accidentally breaking storage operations (explicit Reject required). Refs: #526 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
…ll backends Add ColumnDef, extra_columns, skip_columns to SchemaConfig and wire them through DDL, INSERT, and SELECT paths in Postgres, Oracle, and Redis. What changed: - data_connector/src/schema.rs: add ColumnDef (sql_type + default_value), extra_columns: HashMap<String, ColumnDef> and skip_columns: HashSet on TableConfig; add is_skipped() method; validate_sql_type() with allowlisted chars to prevent SQL injection; uppercase_for_oracle() handles extra/skip column names; 18 new tests - data_connector/src/common.rs: add extra_column_defs(), sorted_extra_column_names(), value_to_sql_string(), resolve_extra_column_values() helpers; build_response_select_base() respects skip_columns; 10 new tests - data_connector/src/postgres.rs: DDL conditionally omits skipped columns and appends extra column defs for all 4 tables (conversations, items, links, responses); INSERT dynamically builds column/param lists respecting skip_columns and appending extra columns from current_extra_columns(); SELECT and row parsing use is_skipped() guards with sensible defaults - data_connector/src/oracle.rs: same DDL/INSERT/SELECT changes as Postgres; captures current_extra_columns() before spawn_blocking since task-locals don't propagate to blocking threads - data_connector/src/redis.rs: write paths skip hset for skipped columns and append extra columns from hooks; read paths (build_item_from_map, build_response_from_map, get_conversation) use defaults for skipped fields; HashMap import consolidated Why: extra_columns enables hooks to persist custom fields (tenant ID, audit trail) alongside core data without schema changes. skip_columns enables adapting to existing schemas that lack certain standard columns. Together they make the storage layer flexible enough to fit into enterprise environments without forking. How: DDL: filter out skipped core columns, append extra column defs. INSERT: build column/param lists dynamically, append extra column values resolved via hook > default > NULL priority chain. SELECT: omit skipped columns from queries, use defaults in row parsing. Oracle: capture task-local extra columns before spawn_blocking boundary. Redis: skip hset/hget for skipped fields, append extra as hash fields. Refs: #526 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Add WIT interface, wasmtime bindings, and WasmStorageHook bridge for running storage hooks as sandboxed WASM components. Includes a working guest example and updated data_connector README. What changed: - wasm/src/interface/storage/storage-hooks.wit: WIT interface defining storage-hook-types (ContextEntry, Operation enum, ExtraColumn, BeforeResult variant), storage-hook-before and storage-hook-after export interfaces, and storage-hook world - wasm/src/storage_spec.rs: wasmtime component::bindgen! for the storage-hook world with async imports/exports - wasm/src/storage_hook.rs: WasmStorageHook struct implementing StorageHook trait; compiles WASM component once at construction, instantiates per call with 10MB memory limit; type conversions between Rust StorageOperation/ExtraColumns and WIT types; 5 unit tests - wasm/Cargo.toml: add storage-hooks feature flag gating smg-data-connector, async-trait, serde_json as optional deps - wasm/src/lib.rs: feature-gated module declarations and re-export of WasmStorageHook - examples/wasm/wasm-guest-storage-hook/: complete guest example demonstrating multi-tenancy (tenant_id from context) and audit trail (created_by) extra columns; includes Cargo.toml, src/lib.rs, build.sh, README.md - data_connector/README.md: document StorageHook trait, wiring, request context, extra columns (with YAML config example and resolution order), skip columns, and WASM storage hooks Why: WASM storage hooks enable sandboxed, language-agnostic hook implementations for teams that cannot or prefer not to write Rust. The Component Model provides strong isolation with memory limits. The example guest demonstrates the primary use cases (multi-tenancy, audit trail). How: Separate WIT world (storage-hook) avoids polluting the existing smg middleware world. Feature-gated behind storage-hooks to keep the WASM crate lean by default. Each before()/after() call creates a fresh WASM store instance for isolation. Type conversions bridge between Rust domain types and WIT flat types. Refs: #526 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
… storage hooks Proves that schema config + hooks replace the need to fork storage backends (e.g. temp_oracle.rs) for custom columns, tenancy, or naming. What changed: - data_connector/src/common.rs: 4 unit tests proving SchemaConfig generates Oracle-compatible SQL, adds extra columns, skips columns, and remaps column names — all without code changes - data_connector/src/hooked.rs: 2 integration tests proving the full hook → task-local → backend pipeline (TenantHook + CapturingStorage and RequestContext → ContextDrivenHook → CapturingConvStorage) - wasm/tests/storage_hook_integration.rs: 5 WASM integration tests loading pre-built guest fixtures and verifying host↔guest round-trip - wasm/tests/fixtures/build_fixtures.sh: script to build WASM test fixtures from examples/wasm/ guest sources - examples/wasm/wasm-guest-storage-hook-passthrough/: minimal WASM guest that never rejects, adds HOOK_ACTIVE marker on writes - model_gateway: --storage-hook-wasm-path CLI arg + gateway wiring to load WASM hooks from disk (types.rs, builder.rs, main.rs, app_context.rs, Cargo.toml enables storage-hooks feature) - e2e_test/responses/test_storage_hooks.py: 3 E2E tests verifying Responses API works with a WASM storage hook active - .gitignore: ignore compiled WASM fixtures (built from source) - .pre-commit-config.yaml: add wit/WIT/implementors to codespell ignore list (valid technical terms) - examples/wasm/README.md, wasm/README.md: document storage hook examples Why: Phase 2b validates the claim that schema config + hooks make backend forking unnecessary. Tests at three layers (unit, WASM integration, E2E) prove extra columns, skip columns, column remapping, and hook- driven tenancy all work without modifying backend code. Signed-off-by: Simolin <simolin@users.noreply.github.com> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
requested review from
CatherineSue,
XinyueZhang369,
key4ng and
tonyluj
as code owners
February 25, 2026 18:32
github-actions
Bot
added
documentation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a storage hook subsystem: hook trait/types, per-request context and task-local extra columns, schema support for extra/skip columns, hooked storage wrappers, WASM host and guest implementations/examples, gateway wiring to load a WASM hook, and tests/fixtures. No breaking public API removals. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Gateway
participant HookedStorage
participant WasmStorageHook
participant InnerBackend
Client->>Gateway: API request (store/get)
Gateway->>HookedStorage: call operation(payload, context)
HookedStorage->>WasmStorageHook: before(operation, context, payload)
WasmStorageHook->>WasmStorageHook: instantiate component & call before
alt before -> Continue(extra)
WasmStorageHook-->>HookedStorage: Continue(extra)
HookedStorage->>HookedStorage: set task-local extra columns
HookedStorage->>InnerBackend: execute operation(with extras)
InnerBackend-->>HookedStorage: result
HookedStorage->>WasmStorageHook: after(operation, context, payload, result, extra)
WasmStorageHook->>WasmStorageHook: instantiate & call after
WasmStorageHook-->>HookedStorage: final_extra
HookedStorage-->>Gateway: return result
else before -> Reject(reason)
WasmStorageHook-->>HookedStorage: Reject(reason)
HookedStorage-->>Gateway: return StorageError
end
sequenceDiagram
participant App
participant AppContext
participant WasmStorageHook
participant WasmComponent
App->>AppContext: with_storage(wasm_path)
AppContext->>AppContext: read wasm bytes
AppContext->>WasmStorageHook: WasmStorageHook::new(wasm_bytes)
WasmStorageHook->>WasmComponent: compile/validate component
AppContext->>StorageFactory: build with hook
StorageFactory->>HookedStorage: wrap backends with hook
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Summary of ChangesHello @slin1237, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the data connector's extensibility by introducing a robust storage hook system, a flexible schema configuration, and a WebAssembly bridge. These changes eliminate the need to fork storage backends for common customizations like adding tenant IDs, audit trails, or schema naming changes. The new architecture allows for dynamic modification of storage operations and data persistence through configuration and sandboxed WASM modules, making deployments more maintainable and adaptable. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and well-designed storage hook system, a significant feature that enhances extensibility by allowing custom logic for storage operations without forking backends. The changes are extensive, spanning the data_connector, model_gateway, and wasm crates, and include a flexible StorageHook trait, schema configuration for extra and skippable columns, and a WASM bridge for sandboxed hooks. The implementation is robust, with thorough testing at unit, integration, and E2E levels. My review includes a couple of suggestions, informed by established repository rules, to improve code clarity and maintainability in the database backend implementations.
![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: b3b2c6f387
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/wasm/README.md (1)
98-98:⚠️ Potential issue | 🟡 Minor"All three modules" is stale now that the directory has five examples.
The two new storage-hook examples use a different deployment path (CLI flag
--storage-hook-wasm-pathrather than the/wasmPOST endpoint), so the deployment section intentionally covers only the three HTTP middleware examples. The prose should clarify this rather than using a bare count.📝 Proposed fix
-You can deploy all three modules together: +You can deploy the three HTTP middleware modules together:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/wasm/README.md` at line 98, Update the sentence "You can deploy all three modules together:" to clarify it only refers to the three HTTP middleware examples (not the two storage-hook examples), and explicitly mention that the repository now contains five examples and that the storage-hook examples are deployed via the CLI flag --storage-hook-wasm-path rather than the /wasm POST endpoint; locate this text in examples/wasm/README.md (look for the exact phrase "You can deploy all three modules together:"), reword to something like "You can deploy the three HTTP middleware modules together; note there are five examples total and the two storage-hook examples use --storage-hook-wasm-path instead of the /wasm POST endpoint." Ensure the prose contrasts the two deployment methods clearly.data_connector/src/postgres.rs (1)
807-836:⚠️ Potential issue | 🔴 CriticalResponse skip-columns can break startup and identifier queries.
This DDL can omit
safety_identifier/created_at, but startup still creates a safety index and list/delete identifier paths still filter/order by those columns. Skip-enabled configs can fail at init or query time.🔧 Suggested guard pattern
- ddl.push_str(&format!( - "\nCREATE INDEX IF NOT EXISTS responses_safety_idx ON {table} ({});", - s.col("safety_identifier") - )); + if !s.is_skipped("safety_identifier") { + ddl.push_str(&format!( + "\nCREATE INDEX IF NOT EXISTS responses_safety_idx ON {table} ({});", + s.col("safety_identifier") + )); + }
♻️ Duplicate comments (2)
examples/wasm/wasm-guest-storage-hook/Cargo.toml (1)
12-12: Samewit-bindgenversion verification concern as in the passthrough crate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/wasm/wasm-guest-storage-hook/Cargo.toml` at line 12, The wit-bindgen dependency in this crate is not the same version as the passthrough crate; update the wit-bindgen entry so it matches the version used by the passthrough crate (or centralize it via the workspace by using the same version or workspace = true), e.g., set the version string to the exact same value used in the passthrough crate's Cargo.toml to ensure consistency across crates.examples/wasm/wasm-guest-storage-hook/build.sh (1)
50-50: Same\sportability issue as in the passthrough variant.
grep -q "^(\s*component"has the same non-portable\spattern. Apply the same fix: replace\swith[[:space:]].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/wasm/wasm-guest-storage-hook/build.sh` at line 50, The grep pattern used in the conditional (grep -q "^(\s*component") uses the non-portable \s class; update that grep invocation used with wasm-tools print (the `if wasm-tools print "$WASM_MODULE" ... | grep -q "^(\s*component"` conditional) to use a POSIX character class by replacing \s with [[:space:]] so the pattern becomes grep -q "^([[:space:]]*component" and ensure the rest of the conditional remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data_connector/README.md`:
- Around line 225-230: The docs currently downplay that before() returning
Err(_) is treated as non-fatal; update the README to add a prominent warning
block (e.g., "⚠️ Warning") immediately above the bullets that clearly states
that Err(_) from the before() hook is silently ignored and can bypass policy
enforcement, and then add documentation for a new StorageFactoryConfig boolean
flag strict_mode which, when true, causes the system to treat Err(_) from
before() as a hard Reject (failing the operation) instead of continuing;
reference the before(), after(), Err(_), Continue(extra_columns), Reject(reason)
symbols and the StorageFactoryConfig name so readers can find the related code
paths.
In `@data_connector/src/factory.rs`:
- Around line 141-149: Add a unit test for create_storage that sets config.hook
= Some(...) and asserts the factory returns HookedResponseStorage,
HookedConversationStorage, and HookedConversationItemStorage wrappers (not the
raw resp/conv/items), and verifies these wrappers delegate calls to the
underlying storages (e.g., call a simple get/insert method on each wrapper and
confirm the underlying mock/storage received the call and hook was invoked).
Target the create_storage function and the types HookedResponseStorage,
HookedConversationStorage, HookedConversationItemStorage to ensure the
hook-enabled branch wiring is exercised and guarded against regressions.
In `@data_connector/src/hooked.rs`:
- Around line 31-46: Document in the StorageHook trait that errors returned from
StorageHook::before are treated as soft failures (they are logged and run_before
returns empty ExtraColumns) and that authors who require hard failure should
return Ok(BeforeHookResult::Reject(...)) instead of Err; reference the
run_before function, StorageHook::before, BeforeHookResult, ExtraColumns and the
current_request_context behavior so maintainers and hook authors understand that
Err from before() does not abort the storage operation but is logged and
substituted with ExtraColumns::new().
- Around line 49-60: run_after currently drops the Ok(ExtraColumns) from
StorageHook::after; change run_after to return Option<ExtraColumns> (or
Result<ExtraColumns, HookError> if you want to propagate errors) and update all
callers to accept and merge the returned ExtraColumns. Specifically, update the
signature of run_after to async fn run_after(...) -> Option<ExtraColumns>, call
hook.after(...).await and on Ok(cols) return Some(cols) (on Err(e) keep the
warn!("after hook error...") and return None), and then modify callers that
invoked run_after(...) to handle the Option<ExtraColumns> (merge or forward the
columns into API responses or higher-level ExtraColumns aggregation) so
hook-produced data is not discarded; reference symbols: run_after,
StorageHook::after, ExtraColumns, HookError.
In `@data_connector/src/oracle.rs`:
- Around line 301-307: The update flow currently assumes created_at and metadata
exist; modify the update_conversation SQL builder to respect the schema's
skip_columns: only include the "metadata = :metadata" assignment and its bind
parameter when !s.is_skipped("metadata"), and only include "RETURNING
<created_at_column>" in the RETURNING clause (and handle scanning the returned
value) when !s.is_skipped("created_at"); also ensure the positional/named bind
arguments and result scanning match the conditional inclusion. Apply the same
conditional logic to the related update SQL construction in the block referenced
around lines 402-429 so both metadata updates and returning created_at are
omitted when schema skips those columns.
- Around line 1116-1135: The conditional omission of core columns via core_cols
and s.is_skipped causes downstream code (index creation and identifier queries
that reference previous_response_id, safety_identifier, created_at) to break;
either ensure those specific columns are always added to col_defs (e.g., after
the for loop unconditionally push formatted definitions for
previous_response_id, safety_identifier, and created_at so functions that expect
them find them) or update the downstream logic that uses
previous_response_id/safety_identifier/created_at (index creation and identifier
query code paths) to check s.is_skipped before referencing them; locate symbols
col_defs, core_cols, s.is_skipped, extra_column_defs and the index/identifier
query logic and apply one of these two fixes consistently.
In `@data_connector/src/postgres.rs`:
- Around line 78-85: The CREATE TABLE logic conditionally omits
created_at/metadata using s.is_skipped, but update_conversation still
unconditionally sets metadata and returns created_at; update the SQL builder in
the update_conversation function to conditionally include the "SET metadata =
$N" clause only when !s.is_skipped("metadata") and to include "RETURNING
created_at" only when !s.is_skipped("created_at"), adjusting parameter indexes
accordingly; use the same s.is_skipped checks (and the helper
col/extra_column_defs references) to keep column handling consistent (also apply
the same conditional inclusion in the other update_conversation occurrence
around lines 182-193).
In `@data_connector/src/redis.rs`:
- Around line 848-851: The safety_identifier branch currently only writes the
hash when present (pipe.hset in the sr.is_skipped("safety_identifier") block),
which lets the sorted-set index become stale when the column is skipped later;
modify the logic so that when sr.is_skipped("safety_identifier") is true you:
(1) read the existing safety identifier from the response hash (HGET on the same
key/field) and if present issue a ZREM on the safety sorted-set for that
identifier, and (2) remove the hash field (HDEL) so delete_response cannot rely
on it—apply the same fix to the other occurrence around lines 862-868; reference
sr.is_skipped, response.safety_identifier, pipe.hset and delete_response to
locate the places to add the HGET/ZREM/HDEL operations.
- Around line 171-191: The update_conversation path still unconditionally
reads/writes created_at and metadata which can cause runtime errors when these
columns are skipped; modify the update_conversation implementation to mirror
create_conversation/get_conversation by checking s.is_skipped("created_at") and
s.is_skipped("metadata") before performing HGET/HSET on s.col("created_at") and
s.col("metadata"), use Utc::now() (or leave existing value) when created_at is
skipped and treat metadata as None (or preserve existing) when metadata is
skipped, and reuse Self::parse_metadata to decode Option<String> only when a
value was actually fetched; update error handling to return
ConversationStorageError::StorageError as currently used.
In `@data_connector/src/schema.rs`:
- Around line 152-158: The current uppercasing loop that mutates
tc.extra_columns can silently clobber entries when keys differ only by case
(e.g., "tenant_id" vs "TENANT_ID"); change validation so that before mutating
tc.extra_columns you scan its keys (tc.extra_columns.keys()) and detect any
pairs that would collide when uppercased (map each key ->
key.to_ascii_uppercase() and check for duplicates), and if any duplicates are
found return an error/validation failure instead of performing the
remove/insert; apply the same check to the other similar block referenced (the
200-210 region) so both places reject case-colliding extra_columns entries.
- Around line 111-113: The skip check is broken because uppercase_for_oracle()
normalizes skip_columns to uppercase but is_skipped(&self, field: &str) does a
case-sensitive lookup against skip_columns, so callers passing lowercase names
fail to match; fix by normalizing the queried field the same way skip_columns
are normalized (call uppercase_for_oracle(field) or otherwise
uppercase/normalize the input) before checking self.skip_columns.contains(...)
and apply the same normalization approach to other similar checks (the other
occurrences referenced around the blocks noted) so storage and lookup use the
same casing/normalization.
- Around line 255-270: In validate_sql_type, reject inputs that are only
whitespace by trimming first and validating the trimmed value: add let trimmed =
sql_type.trim(); if trimmed.is_empty() return Err(...) with a clear message;
then use trimmed for the length check against MAX_SQL_TYPE_LEN and for the
character validation (replace sql_type.len() and sql_type.chars() uses with
trimmed.len() and trimmed.chars()). Keep the existing allowed-character check
but apply it to trimmed instead of the original string.
In `@e2e_test/responses/test_storage_hooks.py`:
- Around line 24-80: These tests only assert API status and not that the WASM
storage hook actually ran; update one or more tests (e.g.,
test_create_and_get_response_with_hook,
test_conversation_with_previous_response_with_hook, or
test_input_items_list_with_hook) to assert a concrete side‑effect from the hook
(for example switch from PASSTHROUGH_HOOK_PATH to a test hook that writes a
visible marker or rejects and assert that marker/rejection occurred, or assert a
reserved metadata field/item added by the hook after client.responses.create or
client.responses.retrieve); ensure you reference the hook fixture/path
(PASSTHROUGH_HOOK_PATH or the rejecting fixture you add) and add an assertion
that inspects the response, retrieved response, input_items, or gateway/log
output to prove the hook executed.
In `@examples/wasm/wasm-guest-storage-hook-passthrough/build.sh`:
- Around line 1-74: The two nearly-identical scripts
(wasm-guest-storage-hook-passthrough/build.sh and
wasm-guest-storage-hook/build.sh) should be consolidated: create a shared script
(e.g., examples/wasm/build_hook.sh) that accepts parameters for the crate base
name (wasm_guest_storage_hook_passthrough or wasm_guest_storage_hook) and the
usage message, move the common logic (target checks, wasm-tools checks, cargo
build, component wrapping using wasm-tools and file checks) into build_hook.sh,
and update both build.sh files to simply call build_hook.sh with the appropriate
crate name and usage text (or source it); also add a short comment in each
wrapper build.sh referencing the sibling so maintainers know they defer to the
shared script and to update parameters if needed.
- Line 50: The grep pattern using “\s” is not portable to BSD/macOS grep; update
the component-detection grep in the build scripts so it uses a POSIX character
class instead (replace the pattern grep -q "^(\s*component" with one using
[[:space:]] such as grep -q "^\([[:space:]]*component" or equivalent), and apply
the same change in the other build.sh variant; this preserves the intended
whitespace matching while remaining portable.
- Around line 63-74: Remove the redundant outer existence check for
$WASM_COMPONENT and replace it with an unconditional success block: after the
inner guard that runs `wasm-tools component new` and exits on failure (the code
that calls `wasm-tools component new` and does `exit 1` on error), delete the
surrounding `if [ -f "$WASM_COMPONENT" ]; then ... else ... fi` and simply emit
the success messages that reference $WASM_MODULE and $WASM_COMPONENT along with
the usage hint; keep the inner guard behavior unchanged so failures still exit,
but avoid the unreachable outer branch.
In `@examples/wasm/wasm-guest-storage-hook/README.md`:
- Around line 44-60: The example imports the wrong crate name; replace the `use
data_connector::factory::StorageFactoryConfig;` import with the correct crate
`smg_data_connector` (crate name uses underscore for the package
`smg-data-connector`) so the example compiles; update the import that references
StorageFactoryConfig (and any other `data_connector::...` symbols) to
`smg_data_connector::factory::StorageFactoryConfig` to match the actual crate,
leaving WasmStorageHook::new, create_storage, and StorageFactoryConfig usage
intact.
- Around line 37-40: Add a blank line before the fenced code block that contains
the compiled component path and change the opening fence to include a language
specifier (for example use ```text or ```bash) so the block follows MD031/MD040;
locate the block that currently shows
"target/wasm32-wasip2/release/wasm_guest_storage_hook.component.wasm" in the
README and insert an empty line above it and update the fence from ``` to
```text (or ```bash).
In `@model_gateway/src/app_context.rs`:
- Line 449: The error mapping that turns a WASM compile error into a string
currently omits the source file path; update the closure that produces the error
message (the ".map_err(|e| format!(\"failed to compile WASM storage hook:
{e}\"))?" expression in app_context.rs) to include the path (e.g., via path or
path.display()) so the log reads like "failed to compile WASM storage hook for
<path>: <error>" to make failures actionable.
- Around line 448-449: WasmStorageHook::new is a CPU-bound, blocking call
(wasmtime JIT/AOT) and must not run directly on an async executor thread; wrap
the call in tokio::task::spawn_blocking and await its JoinHandle inside the
async context to offload compilation to a blocking thread, then map any error
into the existing format ("failed to compile WASM storage hook: {e}") before
returning—replace the direct call to smg_wasm::WasmStorageHook::new(&bytes) with
a spawn_blocking invocation that performs the new(...) call and returns the
created WasmStorageHook or error.
In `@model_gateway/src/config/builder.rs`:
- Around line 356-359: Change the signature of maybe_storage_hook_wasm_path to
accept Option<impl Into<String>> (or a generic P: Into<String>) instead of
Option<&String>, and map the Option to String when assigning: replace
self.config.storage_hook_wasm_path = path.cloned() with
self.config.storage_hook_wasm_path = path.map(|p| p.into()); keep function name
maybe_storage_hook_wasm_path and the field storage_hook_wasm_path unchanged so
existing call sites (like passing self.storage_hook_wasm_path.as_ref()) continue
to compile.
In `@wasm/src/storage_hook.rs`:
- Around line 61-64: Enable and enforce a hard execution budget on the WASM
store: update the Config setup where Config::new(), config.async_support(true),
and config.wasm_component_model(true) are called to also enable interruption and
fuel (e.g., config.epoch_interruption(true) and config.consume_fuel(true)), then
when creating the Store/Engine ensure you add a bounded amount of fuel
(Store::add_fuel(...)) and/or set epoch deadlines before invoking guest code;
specifically add fuel and/or schedule epoch-based interruption at the guest call
sites (the "before hook" and "after hook" invocation points referenced around
lines 160–178 and 182–228) so the guest is precharged with a finite fuel budget
and will be interrupted if it exceeds that budget.
In `@wasm/src/storage_spec.rs`:
- Around line 1-4: The file-level doc comment is misleading because this file
invokes the bindgen! macro rather than containing generated code; update the
header comment in storage_spec.rs to state that the module invokes the bindgen!
macro to generate WebAssembly interface bindings for storage-hooks.wit at
compile time (mention bindgen! and the source interface storage-hooks.wit), e.g.
replace or remove the "Generated from ..." phrasing so it clearly indicates the
macro invocation generates the bindings rather than the file being the generated
output.
---
Outside diff comments:
In `@examples/wasm/README.md`:
- Line 98: Update the sentence "You can deploy all three modules together:" to
clarify it only refers to the three HTTP middleware examples (not the two
storage-hook examples), and explicitly mention that the repository now contains
five examples and that the storage-hook examples are deployed via the CLI flag
--storage-hook-wasm-path rather than the /wasm POST endpoint; locate this text
in examples/wasm/README.md (look for the exact phrase "You can deploy all three
modules together:"), reword to something like "You can deploy the three HTTP
middleware modules together; note there are five examples total and the two
storage-hook examples use --storage-hook-wasm-path instead of the /wasm POST
endpoint." Ensure the prose contrasts the two deployment methods clearly.
---
Duplicate comments:
In `@examples/wasm/wasm-guest-storage-hook/build.sh`:
- Line 50: The grep pattern used in the conditional (grep -q "^(\s*component")
uses the non-portable \s class; update that grep invocation used with wasm-tools
print (the `if wasm-tools print "$WASM_MODULE" ... | grep -q "^(\s*component"`
conditional) to use a POSIX character class by replacing \s with [[:space:]] so
the pattern becomes grep -q "^([[:space:]]*component" and ensure the rest of the
conditional remains unchanged.
In `@examples/wasm/wasm-guest-storage-hook/Cargo.toml`:
- Line 12: The wit-bindgen dependency in this crate is not the same version as
the passthrough crate; update the wit-bindgen entry so it matches the version
used by the passthrough crate (or centralize it via the workspace by using the
same version or workspace = true), e.g., set the version string to the exact
same value used in the passthrough crate's Cargo.toml to ensure consistency
across crates.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (35)
.gitignore.pre-commit-config.yamldata_connector/README.mddata_connector/src/common.rsdata_connector/src/context.rsdata_connector/src/factory.rsdata_connector/src/hooked.rsdata_connector/src/hooks.rsdata_connector/src/lib.rsdata_connector/src/oracle.rsdata_connector/src/postgres.rsdata_connector/src/redis.rsdata_connector/src/schema.rse2e_test/responses/test_storage_hooks.pyexamples/wasm/README.mdexamples/wasm/wasm-guest-storage-hook-passthrough/Cargo.tomlexamples/wasm/wasm-guest-storage-hook-passthrough/build.shexamples/wasm/wasm-guest-storage-hook-passthrough/src/lib.rsexamples/wasm/wasm-guest-storage-hook/Cargo.tomlexamples/wasm/wasm-guest-storage-hook/README.mdexamples/wasm/wasm-guest-storage-hook/build.shexamples/wasm/wasm-guest-storage-hook/src/lib.rsmodel_gateway/Cargo.tomlmodel_gateway/src/app_context.rsmodel_gateway/src/config/builder.rsmodel_gateway/src/config/types.rsmodel_gateway/src/main.rswasm/Cargo.tomlwasm/README.mdwasm/src/interface/storage/storage-hooks.witwasm/src/lib.rswasm/src/storage_hook.rswasm/src/storage_spec.rswasm/tests/fixtures/build_fixtures.shwasm/tests/storage_hook_integration.rs
What changed: - data_connector/src/schema.rs: stop uppercasing skip_columns in uppercase_for_oracle() — they are logical field names used in case-sensitive is_skipped() checks; add whitespace-only sql_type validation; fix test to assert skip_columns stay lowercase - data_connector/src/postgres.rs: guard safety_identifier index creation with is_skipped(); handle skipped metadata/created_at in update_conversation(); simplify unzip patterns to direct iteration - data_connector/src/oracle.rs: guard previous_response_id and safety_identifier index creation with is_skipped(); handle skipped metadata/created_at in update_conversation(); simplify unzip patterns to direct iteration - data_connector/src/redis.rs: guard metadata write and created_at read in update_conversation() with is_skipped() checks - model_gateway/src/app_context.rs: include file path in WASM compilation error message - examples/wasm/wasm-guest-storage-hook/README.md: fix crate name from data_connector to smg_data_connector - examples/wasm/*/build.sh: replace \s with [[:space:]] in grep for macOS portability Why: PR #541 review identified that skip_columns uppercasing broke the case-sensitive HashSet lookup in is_skipped(), unconditional index creation would fail when the indexed column was skipped, and update_conversation in all three backends referenced metadata and created_at without skip guards. How: Each backend's update_conversation now branches on whether metadata and created_at are skipped: if metadata is skipped there is nothing to update (just verify row exists); if created_at is skipped, use Utc::now() instead of RETURNING. Index DDL is wrapped in is_skipped guards. The unzip+indexed-loop pattern is replaced with a collected Vec iterated in two passes (names then values) to satisfy borrow checker constraints on the params vector. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3667cc3145
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
data_connector/src/postgres.rs (1)
1130-1153:⚠️ Potential issue | 🟠 MajorGuard identifier queries when required columns are skipped.
These methods unconditionally use
safety_identifier(and listing usescreated_atfor ordering), so skip-enabled schemas can produce invalid SQL at runtime.🛡️ Proposed guard
async fn list_identifier_responses( &self, identifier: &str, limit: Option<usize>, ) -> ResponseResult<Vec<StoredResponse>> { let s = &self.store.schema.responses; + if s.is_skipped("safety_identifier") || s.is_skipped("created_at") { + return Err(ResponseStorageError::StorageError( + "responses.skip_columns cannot include safety_identifier/created_at for identifier listing" + .to_string(), + )); + } let col_safety = s.col("safety_identifier"); let col_created = s.col("created_at"); @@ async fn delete_identifier_responses(&self, identifier: &str) -> ResponseResult<usize> { let s = &self.store.schema.responses; + if s.is_skipped("safety_identifier") { + return Err(ResponseStorageError::StorageError( + "responses.skip_columns cannot include safety_identifier for identifier deletion" + .to_string(), + )); + } let table = s.qualified_table(self.store.schema.owner.as_deref()); let col_safety = s.col("safety_identifier");Also applies to: 1173-1178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/postgres.rs` around lines 1130 - 1153, The code builds SQL using the responses schema columns (col("safety_identifier") and col("created_at")) without checking that those columns are present, which causes invalid SQL for skip-enabled schemas; update the methods that use self.select_base (the response retrieval and listing blocks around where col_safety and col_created are used) to first verify the responses schema contains "safety_identifier" (and "created_at" when ordering) and return a clear ResponseStorageError (e.g., ResponseStorageError::StorageError or a new InvalidSchema variant) if they are missing instead of constructing the SQL; apply the same guard to the other occurrence noted (around the 1173–1178 block) so both query paths validate schema columns before formatting SQL.
♻️ Duplicate comments (5)
model_gateway/src/app_context.rs (1)
449-450:⚠️ Potential issue | 🟠 MajorCompile the WASM hook off the async executor thread.
WasmStorageHook::newis still called inline in async startup code; this CPU-heavy compile path can block Tokio workers.🔧 Proposed fix (use
spawn_blocking)- let wasm_hook = smg_wasm::WasmStorageHook::new(&bytes) - .map_err(|e| format!("failed to compile WASM storage hook at {path}: {e}"))?; + let path_for_err = path.clone(); + let wasm_hook = tokio::task::spawn_blocking(move || { + smg_wasm::WasmStorageHook::new(&bytes).map_err(|e| { + format!("failed to compile WASM storage hook at {path_for_err}: {e}") + }) + }) + .await + .map_err(|e| format!("WASM storage hook compilation task failed for {path}: {e}"))??;#!/bin/bash # Verify direct WasmStorageHook::new usage and whether it's wrapped in spawn_blocking. rg -n --type=rust -C4 'WasmStorageHook::new\(' rg -n --type=rust -C2 'spawn_blocking'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/app_context.rs` around lines 449 - 450, The call to smg_wasm::WasmStorageHook::new is executed inline on the async startup path and may block the Tokio executor; change the code to run the CPU-heavy compilation off the async thread by invoking tokio::task::spawn_blocking (or equivalent) to perform WasmStorageHook::new(&bytes) and await its JoinHandle result, mapping errors the same way; update the code that assigns wasm_hook to use the spawn_blocking result and preserve the current error mapping (format!("failed to compile WASM storage hook at {path}: {e}")) so the rest of app_context startup uses the non-blocking value.examples/wasm/wasm-guest-storage-hook/README.md (1)
37-40: Fix fenced-block formatting in the compiled-path section.This block still misses a blank line before the fence and the opening fence has no language tag.
📝 Proposed fix
The compiled component will be at: -``` + +```text target/wasm32-wasip2/release/wasm_guest_storage_hook.component.wasm</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@examples/wasm/wasm-guest-storage-hook/README.mdaround lines 37 - 40, Add a
blank line before the fenced code block and give the opening fence a language
tag (e.g.,text) so the compiled component path is rendered correctly; update the block containing "target/wasm32-wasip2/release/wasm_guest_storage_hook.component.wasm" to be preceded by an empty line and start withtext and end with ``` to fix
formatting.</details> </blockquote></details> <details> <summary>data_connector/src/schema.rs (1)</summary><blockquote> `152-158`: _⚠️ Potential issue_ | _🟠 Major_ **Reject case-colliding `extra_columns` keys before Oracle uppercasing.** Uppercasing keys here can silently overwrite one definition when keys differ only by case. <details> <summary>🛡️ Proposed validation guard</summary> ```diff fn validate_table(label: &str, tc: &TableConfig) -> Result<(), String> { + let mut folded_extra = HashSet::new(); + for name in tc.extra_columns.keys() { + let folded = name.to_ascii_uppercase(); + if !folded_extra.insert(folded.clone()) { + return Err(format!( + "{label}.extra_columns: duplicate keys differ only by case (conflict on '{folded}')" + )); + } + } + for (name, def) in &tc.extra_columns { validate_identifier(name) .map_err(|e| format!("{label}.extra_columns key '{name}': {e}"))?; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@data_connector/src/schema.rs` around lines 152 - 158, Before you uppercase extra column keys, validate tc.extra_columns for case-insensitive duplicates: iterate over tc.extra_columns.keys(), map each key to key.to_ascii_uppercase(), and if any uppercase key is produced more than once, return an error (or a Config validation error) indicating the conflicting original keys. Implement this check just before the existing uppercasing loop that uses tc.extra_columns.remove/insert and reference tc.extra_columns and to_ascii_uppercase to locate the code to change. ``` </details> </blockquote></details> <details> <summary>data_connector/src/oracle.rs (1)</summary><blockquote> `1531-1542`: _⚠️ Potential issue_ | _🟠 Major_ **Identifier operations still assume skipped columns are present.** `list_identifier_responses` and `delete_identifier_responses` still hard-reference `safety_identifier` (and listing also orders by `created_at`), so valid skip configs can fail at runtime. <details> <summary>🧭 Suggested fail-fast guard</summary> ```diff async fn list_identifier_responses( &self, identifier: &str, limit: Option<usize>, ) -> Result<Vec<StoredResponse>, ResponseStorageError> { let identifier = identifier.to_string(); let select_base = self.select_base.clone(); let schema = self.store.schema.clone(); self.store .execute(move |conn| { let s = &schema.responses; + if s.is_skipped("safety_identifier") || s.is_skipped("created_at") { + return Err( + "responses.skip_columns cannot include safety_identifier/created_at for identifier listing" + .to_string(), + ); + } let col_safety = s.col("safety_identifier"); let col_created = s.col("created_at"); @@ async fn delete_identifier_responses( &self, identifier: &str, ) -> Result<usize, ResponseStorageError> { @@ .execute(move |conn| { let s = &schema.responses; + if s.is_skipped("safety_identifier") { + return Err( + "responses.skip_columns cannot include safety_identifier for identifier deletion" + .to_string(), + ); + } let table = s.qualified_table(schema.owner.as_deref()); let col_safety = s.col("safety_identifier"); ``` </details> Also applies to: 1569-1575 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@data_connector/src/oracle.rs` around lines 1531 - 1542, In list_identifier_responses and delete_identifier_responses the code unconditionally uses s.col("safety_identifier") (and list also uses s.col("created_at")), which will panic when those columns are skipped; add a fail-fast guard that checks for the columns before building the SQL (e.g., use a safe lookup like s.col_opt or s.has_col and return a clear error/Result if missing), and only include ORDER BY created_at when that column exists; update the blocks that build select_base/sql (and the same logic around the symbols col_safety, col_created, and the formatted SQL) to avoid hard-referencing absent columns. ``` </details> </blockquote></details> <details> <summary>data_connector/src/redis.rs (1)</summary><blockquote> `854-857`: _⚠️ Potential issue_ | _🟠 Major_ **Prevent safety-index drift when `safety_identifier` is skipped.** The hash field write is skip-aware, but the sorted-set index write is still unconditional. That can leave orphaned safety-index entries because `delete_response` resolves the identifier from the hash field before `ZREM`. <details> <summary>🔧 Proposed fix</summary> ```diff - // Index by safety identifier if present - if let Some(safety) = &response.safety_identifier { - let safety_key = self.safety_key(safety); - let score = response.created_at.timestamp_millis() as f64; - conn.zadd::<_, _, _, ()>(safety_key, response_id_str, score) - .await - .map_err(|e| ResponseStorageError::StorageError(e.to_string()))?; - } + // Index by safety identifier only when the column is persisted + if !sr.is_skipped("safety_identifier") { + if let Some(safety) = &response.safety_identifier { + let safety_key = self.safety_key(safety); + let score = response.created_at.timestamp_millis() as f64; + conn.zadd::<_, _, _, ()>(safety_key, response_id_str, score) + .await + .map_err(|e| ResponseStorageError::StorageError(e.to_string()))?; + } + } ``` </details> Also applies to: 885-891 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@data_connector/src/redis.rs` around lines 854 - 857, The sorted-set index update for safety_identifier must be made skip-aware like the hash field to avoid orphaned index entries; wrap any pipe.zadd/pipe.zrem calls that touch the safety index with the same guard used for the hash (if !sr.is_skipped("safety_identifier")) so you only update or remove the sorted-set when the safety_identifier column is not skipped—apply this change both where pipe.hset is guarded (the block using response.safety_identifier, key, sr.col("safety_identifier")) and the other occurrence around the zadd/zrem logic (lines shown ~885-891). ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@data_connector/src/oracle.rs:
- Around line 460-466: The metadata-skipped path currently uses
s.is_skipped("metadata") and runs conn.query_row_as on "SELECT 1 FROM {table}
WHERE {col_id} = :1", which raises an error when no row exists; change this to
perform a query that returns an Option (e.g., use a query_row_optional /
query_opt style API or run a query and map None) and when no row is found return
Ok(None) instead of propagating an error (preserve map_oracle_error for other
errors and keep using id_str and the same SQL string).
Outside diff comments:
In@data_connector/src/postgres.rs:
- Around line 1130-1153: The code builds SQL using the responses schema columns
(col("safety_identifier") and col("created_at")) without checking that those
columns are present, which causes invalid SQL for skip-enabled schemas; update
the methods that use self.select_base (the response retrieval and listing blocks
around where col_safety and col_created are used) to first verify the responses
schema contains "safety_identifier" (and "created_at" when ordering) and return
a clear ResponseStorageError (e.g., ResponseStorageError::StorageError or a new
InvalidSchema variant) if they are missing instead of constructing the SQL;
apply the same guard to the other occurrence noted (around the 1173–1178 block)
so both query paths validate schema columns before formatting SQL.
Duplicate comments:
In@data_connector/src/oracle.rs:
- Around line 1531-1542: In list_identifier_responses and
delete_identifier_responses the code unconditionally uses
s.col("safety_identifier") (and list also uses s.col("created_at")), which will
panic when those columns are skipped; add a fail-fast guard that checks for the
columns before building the SQL (e.g., use a safe lookup like s.col_opt or
s.has_col and return a clear error/Result if missing), and only include ORDER BY
created_at when that column exists; update the blocks that build select_base/sql
(and the same logic around the symbols col_safety, col_created, and the
formatted SQL) to avoid hard-referencing absent columns.In
@data_connector/src/redis.rs:
- Around line 854-857: The sorted-set index update for safety_identifier must be
made skip-aware like the hash field to avoid orphaned index entries; wrap any
pipe.zadd/pipe.zrem calls that touch the safety index with the same guard used
for the hash (if !sr.is_skipped("safety_identifier")) so you only update or
remove the sorted-set when the safety_identifier column is not skipped—apply
this change both where pipe.hset is guarded (the block using
response.safety_identifier, key, sr.col("safety_identifier")) and the other
occurrence around the zadd/zrem logic (lines shown ~885-891).In
@data_connector/src/schema.rs:
- Around line 152-158: Before you uppercase extra column keys, validate
tc.extra_columns for case-insensitive duplicates: iterate over
tc.extra_columns.keys(), map each key to key.to_ascii_uppercase(), and if any
uppercase key is produced more than once, return an error (or a Config
validation error) indicating the conflicting original keys. Implement this check
just before the existing uppercasing loop that uses
tc.extra_columns.remove/insert and reference tc.extra_columns and
to_ascii_uppercase to locate the code to change.In
@examples/wasm/wasm-guest-storage-hook/README.md:
- Around line 37-40: Add a blank line before the fenced code block and give the
opening fence a language tag (e.g.,text) so the compiled component path is rendered correctly; update the block containing "target/wasm32-wasip2/release/wasm_guest_storage_hook.component.wasm" to be preceded by an empty line and start withtext and end with ``` to fix
formatting.In
@model_gateway/src/app_context.rs:
- Around line 449-450: The call to smg_wasm::WasmStorageHook::new is executed
inline on the async startup path and may block the Tokio executor; change the
code to run the CPU-heavy compilation off the async thread by invoking
tokio::task::spawn_blocking (or equivalent) to perform
WasmStorageHook::new(&bytes) and await its JoinHandle result, mapping errors the
same way; update the code that assigns wasm_hook to use the spawn_blocking
result and preserve the current error mapping (format!("failed to compile WASM
storage hook at {path}: {e}")) so the rest of app_context startup uses the
non-blocking value.</details> --- <details> <summary>ℹ️ Review info</summary> **Configuration used**: Organization UI **Review profile**: ASSERTIVE **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between b3b2c6f3877b12c372c75d7c347b9a27d3276402 and 3667cc31454a40913d3b13e21dfdf509fc6ede1d. </details> <details> <summary>📒 Files selected for processing (9)</summary> * `data_connector/src/oracle.rs` * `data_connector/src/postgres.rs` * `data_connector/src/redis.rs` * `data_connector/src/schema.rs` * `examples/wasm/wasm-guest-storage-hook-passthrough/build.sh` * `examples/wasm/wasm-guest-storage-hook/README.md` * `examples/wasm/wasm-guest-storage-hook/build.sh` * `model_gateway/src/app_context.rs` * `wasm/tests/storage_hook_integration.rs` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
…overage Backend fixes (Postgres, Oracle, Redis): - Add early-return guards in list/delete_identifier_responses when safety_identifier is skipped, preventing queries on missing columns - Guard ORDER BY created_at with skip check in Postgres/Oracle - Guard alter_safety_identifier_column with skip check in Oracle - Fix Oracle update_conversation metadata-skipped path: SELECT COUNT(*) instead of SELECT 1 to avoid ORA-00904 - Simplify extra column appending from two-loop to single-loop pattern across all 4 INSERT paths in Postgres and Oracle - Remove unused sql binding in Postgres store_response - Restructure Redis delete_response to branch on skip for atomic hget+del+zrem vs simple del - Flip if/else branches in Redis delete_response to satisfy clippy if_not_else lint Schema validation: - Add case-insensitive collision detection for extra_columns names against built-in column names WASM storage hooks: - Enable epoch_interruption on wasmtime Engine config - Set epoch_deadline(1) on each fresh Store - Add background epoch ticker (5s timeout) in before() and after() with #[expect(clippy::disallowed_methods)] for tokio::spawn - Fix doc comments in storage_spec.rs bindgen invocation Gateway wiring: - Change maybe_storage_hook_wasm_path from Option<&String> to Option<&str> in RouterConfigBuilder - Update call site in main.rs from .as_ref() to .as_deref() Documentation: - Fix crate name in wasm-guest-storage-hook README New tests (14 total): - common.rs: skip+extra together, remap+skip+extra, INSERT column list, value_to_sql_string conversions - hooked.rs: response store/get round-trip, list/delete identifier responses with hooks, conversation update hooks, item link+list hooks, accumulated multi-operation hook calls - schema.rs: case-colliding extra column rejection - factory.rs: create_storage_with_hook wiring test - storage_hook_integration.rs: extra columns match schema config, multiple operation types, after receives before extras, rejection reason propagation Refs: #541 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e4b243f27
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
examples/wasm/wasm-guest-storage-hook/README.md (1)
37-40:⚠️ Potential issue | 🟡 MinorFix fenced block formatting for markdownlint compliance.
Line 38 should use a language-tagged fence and be preceded by a blank line.
📝 Proposed fix
The compiled component will be at: -``` + +```text target/wasm32-wasip2/release/wasm_guest_storage_hook.component.wasm</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@examples/wasm/wasm-guest-storage-hook/README.mdaround lines 37 - 40, Add a
blank line before the fenced code block and replace the existing fence with a
language-tagged fence (e.g., ```text) so the snippet containing
"target/wasm32-wasip2/release/wasm_guest_storage_hook.component.wasm" is in a
properly formatted fenced block; update README.md to ensure the block is
preceded by a blank line and uses the language-tagged fence.</details> </blockquote></details> <details> <summary>data_connector/src/schema.rs (2)</summary><blockquote> `224-230`: _⚠️ Potential issue_ | _🟡 Minor_ **Primary-key skip guard should be case-insensitive.** Line 226 only rejects `"id"`. `"ID"` currently passes validation even though this table-level rule is meant to ban skipping the primary key. <details> <summary>✅ Minimal fix</summary> ```diff - if name == "id" { + if name.eq_ignore_ascii_case("id") { return Err(format!( "{label}.skip_columns: cannot skip 'id' — it is the primary key" )); } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@data_connector/src/schema.rs` around lines 224 - 230, The check preventing skipping the primary key in the loop over tc.skip_columns currently only rejects the exact string "id"; update the comparison in that loop (where validate_identifier is called for each name) to perform a case-insensitive match (e.g., use an ASCII case-insensitive comparison like eq_ignore_ascii_case or normalize to lowercase) against "id" so "ID", "Id", etc. are also rejected and still return the same formatted error using label. ``` </details> --- `152-158`: _⚠️ Potential issue_ | _🟠 Major_ **Case-colliding `extra_columns` can still be silently overwritten in the Oracle path.** Line 152-Line 158 uppercases keys destructively, but the collision guard at Line 211-Line 221 runs later. In the Oracle init flow, normalization happens before validation, so one entry can be lost before the check ever runs. <details> <summary>🐛 Suggested fix (apply in Oracle init flow)</summary> ```diff - let mut schema = config.schema.clone().unwrap_or_default(); - schema.uppercase_for_oracle(); - schema.validate()?; + let mut schema = config.schema.clone().unwrap_or_default(); + // Validate pre-normalized names first so case-collisions are caught + // before uppercase mutation can clobber entries. + schema.validate()?; + schema.uppercase_for_oracle(); + // Optional second pass to validate the normalized shape. + schema.validate()?; ``` </details> Also applies to: 211-221 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@data_connector/src/schema.rs` around lines 152 - 158, The Oracle path uppercases tc.extra_columns keys in-place which can silently overwrite entries before the collision guard later runs; fix by first scanning tc.extra_columns to detect any case-colliding keys (compute key.to_ascii_uppercase() for each original key and record duplicates), return an error (or otherwise fail initialization) if any two distinct original keys map to the same uppercased key, and only then perform the destructive rename/insertion; reference tc.extra_columns and the to_ascii_uppercase() normalization used in the current loop so you can locate and replace the logic in the Oracle init flow. ``` </details> </blockquote></details> <details> <summary>data_connector/src/redis.rs (1)</summary><blockquote> `935-955`: _⚠️ Potential issue_ | _🟠 Major_ **`delete_response` skip-branch can leave stale safety index members.** At Line 935-Line 939, the skip path deletes only the hash. If an older row still has a safety index entry, it remains orphaned and can pollute later identifier reads/limits. <details> <summary>🧹 Suggested cleanup approach</summary> ```diff - if sr.is_skipped("safety_identifier") { - conn.del::<_, ()>(&key) - .await - .map_err(|e| ResponseStorageError::StorageError(e.to_string()))?; - } else { + if sr.is_skipped("safety_identifier") { + // Best-effort cleanup for legacy rows that still have a safety field/index entry. + let col_safety = sr.col("safety_identifier"); + let (safety, ()): (Option<String>, ()) = redis::pipe() + .atomic() + .hget(&key, col_safety) + .del(&key) + .query_async(&mut conn) + .await + .map_err(|e| ResponseStorageError::StorageError(e.to_string()))?; + if let Some(s) = safety { + conn.zrem::<_, _, ()>(self.safety_key(&s), id) + .await + .map_err(|e| ResponseStorageError::StorageError(e.to_string()))?; + } + } else { let col_safety = sr.col("safety_identifier"); ... } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@data_connector/src/redis.rs` around lines 935 - 955, In delete_response's skip branch (where sr.is_skipped("safety_identifier")), you currently only call conn.del(&key) which can leave an orphaned member in the safety ZSET; instead, fetch the existing safety value with conn.hget(&key, col_safety) (or conn.hget::<_, _, Option<String>>), if Some(s) then call conn.zrem(self.safety_key(&s), id) to remove the index entry, and only then delete the hash with conn.del(&key); use the same symbols present in this diff (sr.is_skipped("safety_identifier"), col_safety, key, id, self.safety_key(&s), conn.hget/conn.zrem/conn.del) and propagate the same ResponseStorageError mapping on errors. ``` </details> </blockquote></details> <details> <summary>data_connector/src/hooked.rs (1)</summary><blockquote> `49-60`: _⚠️ Potential issue_ | _🟠 Major_ **`run_after` drops the `ExtraColumns` returned by `hook.after()`.** Line 57 only checks `Err` and discards the successful `ExtraColumns` value, so post-hook enrichment cannot propagate beyond the hook call despite the trait return type supporting it. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@data_connector/src/hooked.rs` around lines 49 - 60, run_after currently ignores the Ok(ExtraColumns) returned by hook.after() so any enrichment is dropped; modify run_after to either accept extra as mutable (e.g., &mut ExtraColumns) or change its return type to propagate ExtraColumns, then call hook.after(op, ctx.as_ref(), payload, result, extra).await and on Ok(received) merge/extend the received ExtraColumns into the caller's ExtraColumns (or return the received ExtraColumns) instead of discarding it; reference run_after, StorageHook::after, ExtraColumns, and current_request_context when updating the signature and merging logic. ``` </details> </blockquote></details> <details> <summary>data_connector/src/factory.rs (1)</summary><blockquote> `319-394`: _⚠️ Potential issue_ | _🟡 Minor_ **`test_create_storage_with_hook` does not prove the hook path is exercised.** This test currently verifies storage behavior only; it would still pass if the hook-wrapping branch regressed and raw memory storages were returned. Please assert actual hook invocation (e.g., before/after counters in the hook implementation) so the `Some(hook)` branch is truly covered. </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@data_connector/src/postgres.rs:
- Around line 130-156: Extract the repeated column/parameter/placeholder
assembly into a small internal helper (e.g., build_insert_parts) and replace the
duplicated blocks (the one that creates col_names, params, applies s.is_skipped
checks, appends hook_extra via current_extra_columns() and
resolve_extra_column_values, and builds placeholders and sql) with calls to it;
the helper should accept the schema descriptor s plus required base params
(id_str, created_at, metadata_json or their signed references) and return
(Vec<&str> column_names, Vec<&(dyn tokio_postgres::types::ToSql + Sync)> params,
String placeholders) so callers can join column_names and format the final
INSERT SQL consistently across the write paths (ensure it references the same
helpers: s.col, s.is_skipped, current_extra_columns,
resolve_extra_column_values).In
@wasm/src/storage_hook.rs:
- Around line 194-200: The spawned ticker task is not guaranteed to be aborted
when the WASM call returns an error; update the before and after call paths
(e.g., the call to bindings.smg_storage_storage_hook_before().call_before and
the analogous after call) to capture the Result into a variable (e.g., let
result = ... .await.map_err(...) ), then unconditionally call ticker.abort()
before returning or propagating the Result (e.g., ticker.abort(); result?).
Ensure you apply the same pattern for the second block (the after hook) so
HookError::Internal paths still abort the ticker before propagating the error.- Around line 98-99: The current use of store.set_epoch_deadline(1) (and the
ticker tasks that call engine.increment_epoch()) causes different requests
sharing the same Engine to cancel each other when one ticker fires; instead,
remove per-request calls to set_epoch_deadline(1) and the per-request ticker
spawning and either (A) wire this code to a single shared epoch driver that
increments the Engine at fixed intervals and leaves per-store deadline checks to
compare against that shared epoch, or (B) switch to per-store fuel/budget
tracking so each Store instance enforces its own execution quota without
touching Engine::increment_epoch(); update references to
store.set_epoch_deadline, engine.increment_epoch, and the ticker task creation
sites so deadlines are only evaluated against a shared epoch driver (or replaced
by fuel accounting) and ensure no per-request background task calls
engine.increment_epoch().
Duplicate comments:
In@data_connector/src/hooked.rs:
- Around line 49-60: run_after currently ignores the Ok(ExtraColumns) returned
by hook.after() so any enrichment is dropped; modify run_after to either accept
extra as mutable (e.g., &mut ExtraColumns) or change its return type to
propagate ExtraColumns, then call hook.after(op, ctx.as_ref(), payload, result,
extra).await and on Ok(received) merge/extend the received ExtraColumns into the
caller's ExtraColumns (or return the received ExtraColumns) instead of
discarding it; reference run_after, StorageHook::after, ExtraColumns, and
current_request_context when updating the signature and merging logic.In
@data_connector/src/redis.rs:
- Around line 935-955: In delete_response's skip branch (where
sr.is_skipped("safety_identifier")), you currently only call conn.del(&key)
which can leave an orphaned member in the safety ZSET; instead, fetch the
existing safety value with conn.hget(&key, col_safety) (or conn.hget::<_, _,
Option>), if Some(s) then call conn.zrem(self.safety_key(&s), id) to
remove the index entry, and only then delete the hash with conn.del(&key); use
the same symbols present in this diff (sr.is_skipped("safety_identifier"),
col_safety, key, id, self.safety_key(&s), conn.hget/conn.zrem/conn.del) and
propagate the same ResponseStorageError mapping on errors.In
@data_connector/src/schema.rs:
- Around line 224-230: The check preventing skipping the primary key in the loop
over tc.skip_columns currently only rejects the exact string "id"; update the
comparison in that loop (where validate_identifier is called for each name) to
perform a case-insensitive match (e.g., use an ASCII case-insensitive comparison
like eq_ignore_ascii_case or normalize to lowercase) against "id" so "ID", "Id",
etc. are also rejected and still return the same formatted error using label.- Around line 152-158: The Oracle path uppercases tc.extra_columns keys in-place
which can silently overwrite entries before the collision guard later runs; fix
by first scanning tc.extra_columns to detect any case-colliding keys (compute
key.to_ascii_uppercase() for each original key and record duplicates), return an
error (or otherwise fail initialization) if any two distinct original keys map
to the same uppercased key, and only then perform the destructive
rename/insertion; reference tc.extra_columns and the to_ascii_uppercase()
normalization used in the current loop so you can locate and replace the logic
in the Oracle init flow.In
@examples/wasm/wasm-guest-storage-hook/README.md:
- Around line 37-40: Add a blank line before the fenced code block and replace
the existing fence with a language-tagged fence (e.g., ```text) so the snippet
containing "target/wasm32-wasip2/release/wasm_guest_storage_hook.component.wasm"
is in a properly formatted fenced block; update README.md to ensure the block is
preceded by a blank line and uses the language-tagged fence.</details> --- <details> <summary>ℹ️ Review info</summary> **Configuration used**: Organization UI **Review profile**: ASSERTIVE **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 3667cc31454a40913d3b13e21dfdf509fc6ede1d and 6e4b243f272780e7aff6c5988e484a157c3ca984. </details> <details> <summary>📒 Files selected for processing (13)</summary> * `data_connector/src/common.rs` * `data_connector/src/factory.rs` * `data_connector/src/hooked.rs` * `data_connector/src/oracle.rs` * `data_connector/src/postgres.rs` * `data_connector/src/redis.rs` * `data_connector/src/schema.rs` * `examples/wasm/wasm-guest-storage-hook/README.md` * `model_gateway/src/config/builder.rs` * `model_gateway/src/main.rs` * `wasm/src/storage_hook.rs` * `wasm/src/storage_spec.rs` * `wasm/tests/storage_hook_integration.rs` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
WASM epoch ticker leak on error paths (comments #33, #38): - Move `ticker.abort()` before the `?` operator in both `before()` and `after()` methods. Previously, if `call_before` or `call_after` returned an error, the `?` would return early and skip the abort, leaving the ticker task alive to increment the engine epoch 5s later — potentially affecting subsequent WASM calls on the same engine. - Fix: capture the raw Result, abort the ticker unconditionally, then propagate the error with `?`. Schema validation — extra columns shadowing core names (comment #34): - Add `core_columns_for()` helper returning the logical column names for each table (conversations, responses, conversation_items, conversation_item_links). - Cross-check extra_columns keys against core column names (case-insensitive) during `validate()`. Rejects configs like `extra_columns: { CREATED_AT: ... }` that would collide with built-in columns. - Add test `validate_rejects_extra_column_shadowing_core_column`. WASM compilation off async thread (comment #24): - Wrap `WasmStorageHook::new()` in `tokio::task::spawn_blocking` in `app_context.rs`. JIT/AOT compilation is CPU-intensive and should not block the async executor, even though it only runs once at startup. Refs: #541 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
wasm/src/storage_hook.rs (1)
184-192:⚠️ Potential issue | 🟠 MajorConcurrent requests sharing the same
Enginecan prematurely interrupt each other via the shared epoch counter.Each
before()/after()call spawns its own ticker that callsengine.increment_epoch()after 5 seconds. Since the epoch counter is engine-global and shared across all stores from the same engine, a ticker firing for request A also satisfies the deadline for request B's store—potentially interrupting B prematurely (or letting it run longer than intended if B started much later).Consider either:
- A single shared periodic epoch driver (e.g., one background task that increments every 1 s, with
set_epoch_deadline(5)per store), or- Per-store fuel budgets for full isolation.
Also applies to: 233-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wasm/src/storage_hook.rs` around lines 184 - 192, The current per-request ticker spawned in before()/after() (the tokio::spawn block that sleeps 5s then calls epoch_engine.increment_epoch()) causes cross-request interference because increment_epoch() is engine-global; replace this pattern with a single shared epoch driver or per-store fuel budgeting: implement one background task that periodically calls Engine::increment_epoch() (e.g., every 1s) while stores call set_epoch_deadline(5s) or similar to enforce per-call deadlines, or alternatively implement per-store fuel counters checked by the engine so each store has isolated budgets; remove the per-request tokio::spawn ticker in before()/after() and wire those calls to use the shared driver or the per-store budget API instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data_connector/src/schema.rs`:
- Around line 249-280: core_columns_for currently matches on raw string labels
and falls back to &_[] for unknown names, which silently disables the shadowing
check; update this by replacing the fragile string-match behavior with a safer
approach: either switch callers to use a dedicated enum (e.g., TableLabel) and
match on that enum in core_columns_for, or keep string inputs but change the
default arm to surface unexpected labels (e.g., call debug_assert!(false, ...)
or panic!() in tests and log a warning in non-test builds) so renames/additions
are caught; refer to core_columns_for and the match arms
("conversations","responses","conversation_items","conversation_item_links")
when making the change.
- Around line 108-113: Add an INVARIANT doc-comment to the is_skipped(&self,
field: &str) -> bool method documenting that skip_columns contains only
lowercase names and callers must pass lowercase field names; mention the
lowercase-only contract and that the method performs an exact case-sensitive
lookup against the skip_columns set (reference: skip_columns and is_skipped) so
future maintainers know this assumption and avoid regressions.
- Around line 236-243: The loop validating tc.skip_columns only checks
identifier syntax and forbids the literal "id" but doesn't verify that the name
is an actual core column; update the loop to (1) call core_columns_for(&tc) to
get the set/list of valid core column names, (2) after validate_identifier(name)
check that name exists in that core set and if not return Err with a clear
message including label and the invalid name, and (3) keep the existing
special-case error for "id" (or replace it by the general existence check) so
mis-typed names like "safty_identifier" are rejected; use the existing symbols
validate_identifier, core_columns_for, tc.skip_columns and label in the error
text for precise localization.
In `@wasm/src/storage_hook.rs`:
- Around line 270-294: The test operation_conversion_round_trips currently only
ensures to_wit_operation(op) doesn't panic; change it to assert correctness by
comparing the returned WIT enum against the expected WIT variant for each
StorageOperation variant. Update the test to iterate through pairs of
(StorageOperation::..., ExpectedWit::...) and use assertions (e.g., assert_eq!)
to verify to_wit_operation returns the exact expected WIT variant so mapping
errors in to_wit_operation are caught; reference the test function
operation_conversion_round_trips and the conversion function to_wit_operation
when making the change.
- Around line 150-154: from_wit_extra_columns currently coerces every
ExtraColumn.value into Value::String, losing numeric/boolean/null types; update
from_wit_extra_columns to attempt restoring original JSON types by parsing the
WIT string back into serde_json::Value (e.g., try serde_json::from_str and fall
back to Value::String on error) or, if you prefer a more robust solution, change
the WIT ExtraColumn to carry a type tag and use that tag to reconstruct
Value::Number/Value::Bool/Value::Null appropriately; locate the
from_wit_extra_columns function and ExtraColumn type to implement either the
string-to-JSON parse fallback or the type-tagged reconstruction so
numeric/boolean values round-trip without being forced to strings.
---
Duplicate comments:
In `@wasm/src/storage_hook.rs`:
- Around line 184-192: The current per-request ticker spawned in
before()/after() (the tokio::spawn block that sleeps 5s then calls
epoch_engine.increment_epoch()) causes cross-request interference because
increment_epoch() is engine-global; replace this pattern with a single shared
epoch driver or per-store fuel budgeting: implement one background task that
periodically calls Engine::increment_epoch() (e.g., every 1s) while stores call
set_epoch_deadline(5s) or similar to enforce per-call deadlines, or
alternatively implement per-store fuel counters checked by the engine so each
store has isolated budgets; remove the per-request tokio::spawn ticker in
before()/after() and wire those calls to use the shared driver or the per-store
budget API instead.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
data_connector/src/schema.rsmodel_gateway/src/app_context.rswasm/src/storage_hook.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7c2abd445
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Reject unrecognized skip_columns entries during validate(). Previously a typo like "safty_identifier" would pass validation silently and have no effect at runtime, making misconfiguration hard to diagnose. Each skip_columns entry is now checked against core_columns_for() for the table. Unknown entries produce a clear error listing the valid column names. Add test validate_rejects_unknown_skip_column. Refs: #541 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
data_connector/src/schema.rs (1)
318-330:⚠️ Potential issue | 🟡 MinorNormalize
sql_typeonce and validate the normalized value consistently.Line 319 trims only for emptiness, but Lines 322-329 validate length/chars on the untrimmed string. This can produce avoidable padded-value inconsistencies.
✅ Proposed fix
fn validate_sql_type(sql_type: &str) -> Result<(), String> { - if sql_type.trim().is_empty() { + let sql_type = sql_type.trim(); + if sql_type.is_empty() { return Err("sql_type must not be whitespace-only".to_string()); } if sql_type.len() > MAX_SQL_TYPE_LEN { return Err(format!( "sql_type '{sql_type}' exceeds maximum length of {MAX_SQL_TYPE_LEN} characters"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/schema.rs` around lines 318 - 330, In validate_sql_type, normalize the input once (e.g., let normalized = sql_type.trim()) and run all validations against that normalized value: check emptiness, check length against MAX_SQL_TYPE_LEN, and check allowed characters using normalized.chars(), so the length and character checks are consistent with the trimmed value; update all references in validate_sql_type to use the normalized variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data_connector/src/schema.rs`:
- Around line 211-221: The validation in core_columns_for + loop over
tc.extra_columns only checks logical core names, so remapped core physical names
(via the tc.columns mapping) can be shadowed; update the check to compute the
set of core physical names by taking core_columns_for(label) and applying
tc.columns remapping (use the mapping from logical -> physical name in the
TableConfig, falling back to the logical name when no remap exists), uppercase
them for case-insensitive comparison, then reject any tc.extra_columns key whose
uppercased name equals any uppercased physical core name; reference
core_columns_for, tc.extra_columns and the tc.columns mapping when implementing
this change.
---
Duplicate comments:
In `@data_connector/src/schema.rs`:
- Around line 318-330: In validate_sql_type, normalize the input once (e.g., let
normalized = sql_type.trim()) and run all validations against that normalized
value: check emptiness, check length against MAX_SQL_TYPE_LEN, and check allowed
characters using normalized.chars(), so the length and character checks are
consistent with the trimmed value; update all references in validate_sql_type to
use the normalized variable.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 644fc9015e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Replace `include_bytes!` (compile-time embed) with `std::fs::read` (runtime load) for the WASM guest fixture in integration tests. When the fixture is absent (e.g. CI where wasm32-wasip2 guests are not built), tests skip gracefully instead of failing compilation. Add `require_hook!()` macro that early-returns `Ok(())` when the fixture file does not exist. Also includes skip_columns typo detection from previous fix (schema.rs was staged together). Refs: #541 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b63ad3c48
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
slin1237
added a commit
that referenced
this pull request
Feb 26, 2026
Add a "Build WASM test fixtures" step to the unit-tests job that runs build_fixtures.sh before clippy and cargo test. This compiles the wasm-guest-storage-hook examples to wasm32-wasip2 so the integration tests in wasm/tests/storage_hook_integration.rs have the fixture files available. The build script handles installing the wasm32-wasip2 target via rustup if not already present. Refs: #541 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9696f5988
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Add a "Build WASM test fixtures" step to the unit-tests job that runs build_fixtures.sh before clippy and cargo test. This compiles the wasm-guest-storage-hook examples to wasm32-wasip2 so the integration tests in wasm/tests/storage_hook_integration.rs have the fixture files available. The build script handles installing the wasm32-wasip2 target via rustup if not already present. Refs: #541 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 750c7296ad
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The storage-hooks feature increased the future size of test helper async functions (AppTestContext::new, new_with_config, create_test_context, etc.) past clippy's 16KB threshold (~19KB). Fix by converting all 5 public async helpers in common/mod.rs to return Pin<Box<dyn Future<...>>> so callers get a pointer-sized future on the stack. Also allow clippy::large_futures within the module since large stack futures are harmless in test setup code. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1bb401ca7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The agentic-apis E2E test passes --storage-hook-wasm-path to the gateway via the Python launcher, but the argument was not recognized because: 1. RouterArgs dataclass in router_args.py lacked the field and CLI arg 2. The Rust PyO3 Router binding did not accept or forward the parameter 3. The WASM fixture was not built or available in the gateway-e2e CI job Changes: - bindings/python/src/smg/router_args.py: add storage_hook_wasm_path field and --storage-hook-wasm-path CLI argument in the Backend group - bindings/python/src/lib.rs: add storage_hook_wasm_path to PyO3 Router struct, constructor, signature defaults, and to_router_config() call - .github/workflows/pr-test-rust.yml: build WASM fixtures in build-wheel job, upload as artifact, download in gateway-e2e before running tests; add examples/wasm/** to cache key and wasm fixtures to cache paths Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 344eb3c53d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+478
to
+482
| let mut col_names: Vec<&str> = vec![ | ||
| sl.col("conversation_id"), | ||
| sl.col("item_id"), | ||
| sl.col("added_at"), | ||
| ]; |
There was a problem hiding this comment.
skip_columns can be configured on conversation_item_links, but this insert path still unconditionally writes conversation_id, item_id, and added_at, and list_items also depends on added_at for cursor/order logic. With a config that skips one of these fields (a valid config per current schema validation), the backend still emits SQL against the skipped column and fails at runtime on legacy schemas that omit it. Please either make link-table queries truly skip-aware or reject these skip values during validation.
Useful? React with 👍 / 👎.
Comment on lines
+154
to
+157
| let keys: Vec<String> = tc.extra_columns.keys().cloned().collect(); | ||
| for key in keys { | ||
| if let Some(def) = tc.extra_columns.remove(&key) { | ||
| tc.extra_columns.insert(key.to_ascii_uppercase(), def); |
There was a problem hiding this comment.
Oracle normalization rewrites extra_columns keys in place, so case-variant entries like tenant_id and TENANT_ID collide during this loop and one definition is silently overwritten based on HashMap iteration order. Because OracleStore::new validates after this rewrite, the case-insensitive collision check never sees both keys, so startup can pick different SQL/default definitions across runs instead of failing fast. Validate collisions before rewriting (or error on overwrite while rewriting).
Useful? React with 👍 / 👎.
Merged
2 of 3 tasks
3 of 4 tasks
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
Adds a complete storage hook system that eliminates the need to fork storage backends for custom columns, multi-tenancy, audit trails, or schema naming changes. Closes the extensibility gap where deployments like
temp_oracle.rsrequired duplicating entire backend implementations just to add aTENANT_IDcolumn.StorageHooktrait withbefore()/after()interceptors for all storage operationsSchemaConfigwith extra columns, skip columns, and column name remappingHookedResponseStorage/HookedConversationStorage/HookedConversationItemStoragedecorator wrappersRequestContext+ExtraColumnsvia tokio task-local storage for per-request data flowWasmStorageHookbridge for sandboxed WASM Component Model hooks--storage-hook-wasm-pathCLI arg for loading WASM hooks at gateway startupWhat changed
data_connector/(core infrastructure)hooks.rs:StorageHooktrait,BeforeHookResult(Continue/Reject),StorageOperationenum,ExtraColumnstype aliashooked.rs:HookedResponseStorage,HookedConversationStorage,HookedConversationItemStoragewrappers that scopeExtraColumnsvia task-local during inner storage callscontext.rs:RequestContextper-request key-value bag +ExtraColumnstask-local storage withwith_request_context()/with_extra_columns()scopingschema.rs:TableConfig.extra_columns,TableConfig.skip_columns,ColumnDef(sql_type + default_value), validationcommon.rs:resolve_extra_column_values(),extra_column_defs(),build_response_select_base()— all respect skip/extra columnsfactory.rs:StorageFactoryConfig.hookfield, wraps backends in hooked storage when hook is presentoracle.rs,postgres.rs,redis.rs: DDL, INSERT, SELECT all respect skip_columns and extra_columns from schema configwasm/(WASM bridge)src/interface/storage/storage-hooks.wit: WIT world definingstorage-hook-beforeandstorage-hook-afterexportssrc/storage_hook.rs:WasmStorageHookimplementingStorageHooktrait via wasmtime Component Modelsrc/storage_spec.rs: Type conversions between Rust and WIT typestests/storage_hook_integration.rs: 5 integration tests loading pre-built WASM guest fixturestests/fixtures/build_fixtures.sh: Script to build WASM test fixtures from sourcemodel_gateway/(gateway wiring)config/types.rs:storage_hook_wasm_path: Option<String>onRouterConfigconfig/builder.rs:maybe_storage_hook_wasm_path()builder methodmain.rs:--storage-hook-wasm-pathCLI argapp_context.rs: Loads WASM hook from disk and injects intoStorageFactoryConfigCargo.toml: Enablesstorage-hooksfeature onsmg-wasmexamples/wasm/(guest examples)wasm-guest-storage-hook/: Multi-tenant hook requiringtenant_idin context, addsTENANT_ID/STORED_BY/CREATED_BYextra columnswasm-guest-storage-hook-passthrough/: Minimal hook that never rejects, addsHOOK_ACTIVE=truemarker on writese2e_test/responses/test_storage_hooks.py: 3 E2E tests verifying Responses API works with a WASM storage hook active (create+get, multi-turn conversation, input items listing)Other
.gitignore: Ignore compiled WASM test fixtures.pre-commit-config.yaml: Addwit/WIT/implementorsto codespell ignore listexamples/wasm/README.md,wasm/README.md,data_connector/README.md: Document storage hooks, schema config, WASM bridge, and new examplesWhy
Before this change, customizing storage (adding tenant columns, renaming tables, skipping unused columns) required forking the entire backend — see
temp_oracle.rs(1139 lines of hardcoded Oracle). This made deployments fragile, hard to maintain, and impossible to upstream.Now:
SchemaConfighandles naming/columns,extra_columnshandles enrichment,skip_columnshandles omission, andStorageHookhandles per-request logic — all without touching backend code. WASM hooks add sandboxed execution for untrusted or third-party hook logic.Test plan
cargo test -p data-connector— 177 tests pass (unit tests for schema config, hooks, hooked storage, extra columns, all backends)cargo test --features storage-hooks --test storage_hook_integration— 5 WASM integration tests passcargo clippy -p data-connector -p smg-wasm --all-targets --all-features -- -D warnings— cleancargo +nightly fmt --all -- --check— cleancargo build -p smg— gateway builds with WASM hook wiringcd e2e_test && pytest responses/test_storage_hooks.py -v— requires OpenAI API key and running gatewaySummary by CodeRabbit
New Features
Documentation
Tests
Chores