fix: PR #39 medium-priority code quality improvements (M-01 through M-09)#43
Merged
fix: PR #39 medium-priority code quality improvements (M-01 through M-09)#43
Conversation
Previously NaN/Infinity would panic in Duration::from_secs_f64 and arbitrarily large values were uncapped. Now: - finite positive values are clamped to MAX_HOOK_COLLECT_TIMEOUT_SECS (300s) - non-finite or non-positive values fall back to DEFAULT (30s)
adds 5 debug_assert! guards verifying tokio::runtime::Handle::try_current().is_ok() before each block_on call in register_kernel_service_imports(). These fire only in debug builds and document the invariant that WASM host import closures must execute inside spawn_blocking. Includes an invariant test. 🤖 Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…lt helpers Replace 18 serde_json::to_string and 8 serde_json::from_str inline unwrap_or_else+warn patterns with two shared private helpers: fn to_json_or_warn(value, label) -> String fn from_json_or_default<T>(json, label) -> T No behavior change — identical serialization paths and identical warn messages, just deduplicated. All 49 conversion tests pass.
All 6 WASM bridge files contained a nearly identical function for resolving a typed WASM Component export (try root-level first, then try nested inside an interface-exported instance). The only differences were the local function name, the INTERFACE_NAME constant used, and whether the types were generic or hardcoded. Consolidate into a single pub(crate) get_typed_func<Params, Results> in bridges/mod.rs, parameterized by func_name and interface_name. Removed functions: - wasm_tool: get_typed_func_from_instance<P, R> - wasm_provider: get_provider_func<P, R> - wasm_context: get_context_func<P, R> - wasm_hook: get_handle_func (hardcoded types) - wasm_orchestrator: get_execute_func (hardcoded types) - wasm_approval: get_request_approval_func (hardcoded types) Also removed now-unused type aliases (HandleFunc, OrchestratorExecuteFunc, RequestApprovalFunc, WasmResult<T> in context/hook/approval) and unused imports (Store, WasmState in hook/approval/provider/orchestrator). Net: -233 lines / +76 lines across 7 files. All 41 bridge tests pass.
… loaded WASM artifacts
- Add WasmPath(PathBuf) variant to ModuleArtifact enum with doc comment
explaining it is the pre-load state returned by parse_amplifier_toml
- Change parse_amplifier_toml to return WasmPath(wasm_path) instead of
WasmBytes { bytes: Vec::new(), path: wasm_path } — the invisible empty-bytes
contract is now made explicit via the type system
- Update load_module (wasm feature) to handle WasmPath by reading bytes from
disk via std::fs::read before dispatching to the appropriate wasm transport;
WasmBytes continues to work as before for already-loaded bytes
- Update all match arms on ModuleArtifact throughout the codebase:
- bindings/node/src/lib.rs: add WasmPath arm (returns path string, same as WasmBytes)
- bindings/python/src/lib.rs: add WasmPath arm (same artifact_path behavior)
- Update existing parse_toml_wasm_transport test to expect WasmPath
- Add three new tests:
- parse_toml_wasm_transport_returns_wasm_path: verifies parse_amplifier_toml
returns WasmPath, not WasmBytes with empty bytes
- wasm_path_variant_basic: WasmPath can be constructed, cloned, and compared
- load_module_wasm_path_loads_bytes_from_disk: WasmPath artifact in a manifest
causes load_module to read bytes from disk and load successfully
WasmBytes is preserved for backward compatibility (used by resolve_module
wasm auto-detection path and direct-bytes test scenarios).
…odules
- Add sha2 = { version = "0.10", optional = true } dependency, feature-gated
under the 'wasm' feature flag to avoid pulling in sha2 for non-WASM builds
- Add sha256: Option<String> field to ModuleManifest for optional integrity hash
- Parse optional 'sha256' field from [module] section in amplifier.toml
(available for all transport types, primarily useful for WASM)
- Add ModuleResolverError::IntegrityMismatch variant with descriptive message
showing path, expected hash, and actual hash
- Add verify_wasm_integrity() function (cfg(feature = "wasm")) using sha2 crate
to compute and compare SHA-256 digests; logs debug message on success
- Wire verification into load_module() for both WasmPath and WasmBytes artifacts,
executing before bytes are passed to wasmtime for compilation
- Update all ModuleManifest construction sites to include sha256: None
Tests added (TDD - written before implementation):
- parse_toml_with_sha256_field: parses sha256 from amplifier.toml into manifest
- parse_toml_without_sha256_field: missing sha256 field yields None (no verification)
- sha256_missing_field_skips_verification: None sha256 skips check, load succeeds
- sha256_matching_hash_passes: correct hash passes verification, load succeeds
- sha256_mismatched_hash_returns_error: wrong hash returns IntegrityMismatch error
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
Addresses all 9 medium-priority items identified by the 4-agent code review of PR #39 (Cross-Language SDK). The Critical and High priority items were already fixed in v1.1.0; this PR completes the remaining code quality improvements.
Changes (9 commits, ordered quickest-first)
Security Hardening
timeout_secondsto max 300s inEmitHookAndCollect— prevents thread pool exhaustion from absurdly large timeouts. Also fixes a panic on NaN/Infinity inputs. (grpc_server.rs)sha256field toamplifier.tomlfor WASM module integrity verification — detects supply chain attacks or corrupted WASM files at load time. (module_resolver.rs,Cargo.toml)Bug Fixes
unwrap()withswap_remove(0)in module resolver — eliminates a panic-capable unwrap with invisible invariant. (module_resolver.rs)debug_assert!before 5block_oncalls in orchestrator bridge — catches deadlock-inducing misuse in debug builds. (wasm_orchestrator.rs)Node Bindings Cleanup
_reasonparameter from cancellation methods — the parameter was accepted but silently discarded. (bindings/node/src/lib.rs)JsToolResult,JsToolSpec,JsSessionConfig,Role) — 4 exported types with no method that accepts or returns them. (bindings/node/src/lib.rs)Code Deduplication
to_json_or_warn/from_json_or_defaulthelpers — replaces 26 repeated serialization patterns. Net -76 lines. (generated/conversions.rs)get_typed_functobridges/mod.rs— deduplicates 6 nearly-identical WASM function lookup implementations across bridge files. Net -157 lines. (bridges/*.rs)Type Safety
WasmPath(PathBuf)variant inModuleArtifact— makes the deferred-load contract explicit in the type system instead of using an emptyVec<u8>sentinel. Also removes deadensure_wasm_bytes_loadedhelper from Python bindings. (module_resolver.rs,bindings/*/src/lib.rs)Stats
--features wasmand withoutTesting
Review Methodology
Each task was implemented via a three-agent pipeline (implementer → spec-reviewer → code-quality-reviewer) with both reviews passing before proceeding to the next task.