Conversation
✅ Deploy Preview for golemcloud canceled.
|
|
📖 Docs preview: https://docs-9z7gjjgji-golem-cloud.vercel.app Built from commit |
vigoo
left a comment
vigoo
left a comment
There was a problem hiding this comment.
Review of the registry/CLI middleware support. Overall the model looks right (merge-mode semantics, chain order, pinning, WIT copies and protobuf round-trips all check out), but there are a few things to address before this leaves WIP — see the inline comments:
- manifest JSON schema
compatibilityModeenum doesn't match the Rust enum - server never returns middleware state in deployment plans/summaries
- nominal compatibility mode is names-only, not the spec'd semantics
- structural error compatibility ignores
kind/exit_code; strict-equality compares docs/version - structural input projection: emit warnings for discarded inputs
synthesize_effective_definition: remark on error propagation / cross-graphRefcomparison- explicit reserved-name error for a tool named
middleware middlewareMergeModeshould be agent-only- MoonBit: custom-error names lost,
expectedshould be optional - reduce the duplicated release/grant stacks to a minimum
- DB migrations: dead columns in 036, opaque blobs in 037
- dedicated API error codes for middleware
- test gaps (end-to-end chain compile, snapshot round-trip, compat variance)
- license headers on new files
- move inline
mod testsinto separatetests.rsfiles
CI is also red on 4 jobs (clippy large_enum_variant on ToolMiddlewareScope::Monomorphic, stale golem-rust-macro fixtures using ToolInvokeError<TypedSchemaValue>, the bug_finder_guest_custom_error_wire_value_matches_declared_case_payload expectation, and prettier on middlewareRuntime.ts); the integration/worker suites never ran because of that.
| "compatibilityMode": { | ||
| "enum": ["nominal", "structural"] | ||
| }, |
There was a problem hiding this comment.
The JSON schema enum here (nominal, structural) does not match the Rust ToolCompatibilityMode enum in golem-schema/src/schema/tool/compatibility.rs, which serializes (kebab-case) as strict-equality, structural-subtype and nominal.
Consequences: the default value structural-subtype fails schema validation in editors, and the structural value accepted by the schema is rejected by serde. Please align this to ["strict-equality", "structural-subtype", "nominal"] (ideally generated from / tested against the Rust enum so they can't drift again).
| remote_tool_middlewares: Vec::new(), | ||
| published_tool_middlewares: Vec::new(), |
There was a problem hiding this comment.
The server never populates middleware state for plans/summaries: these are hard-coded to empty here, and the same happens in DeploymentIdentity::into_plan (remote_tool_middleware_deployments, published_tool_middlewares, universal_tool_middlewares, tool_compatibility_mode, environment_tool_middleware_bindings, agent_tool_middleware_bindings all Default::default() a few lines below) and in TryFrom<DeployedDeploymentIdentity> for DeploymentSummary. The DeploymentPlan/DeploymentSummary API types also cannot carry the universal chain, compatibility mode or bindings at all.
Effect on the CLI: the quick "already up to date" check still works because it uses the server-stored hash, but the detailed current-vs-local diff, diff_stage and is_stage_same_as_current can't see middleware, so whenever anything else changes every middleware binding shows up as newly "added". DeploymentPlan.remote_tool_middlewares / published_tool_middlewares are effectively dead fields right now.
The stored snapshot (DeploymentToolMiddlewareSnapshotRecord) has the data; please surface it through the plan/summary conversions and extend the API types accordingly, plus a round-trip test (see the test-gaps comment).
| // Nominal compatibility is deliberately names-only. Do not validate or | ||
| // inspect unrelated command and schema details in this mode. | ||
| if mode == ToolCompatibilityMode::Nominal { | ||
| if expected.name() != inner.name() { | ||
| return Err(vec![err("name", "tool names differ")]); | ||
| } | ||
| return Ok(CompiledToolCompatibility { | ||
| mode, | ||
| commands: Vec::new(), | ||
| }); |
There was a problem hiding this comment.
Nominal mode is implemented as a root-name-only check. GOL-39 specifies nominal compatibility with its own variance rules over the command tree, type tree and error cases, and explicitly says the validator must implement the mode semantics rather than just dispatch by name. With this implementation a nominal chain accepts an inner tool with a completely different command tree and produces an empty commands projection.
Please implement the nominal rules from the spec (or, if names-only is the intended interpretation, update the ticket/docs so they agree — but the current comment "deliberately names-only" contradicts the ticket as written).
| fn compile_errors( | ||
| expected: &Tool, | ||
| eb: &CommandBody, | ||
| inner: &Tool, | ||
| nb: &CommandBody, | ||
| mode: ToolCompatibilityMode, | ||
| path: &[String], | ||
| errors: &mut Vec<ToolCompatibilityError>, | ||
| ) -> Vec<ErrorProjection> { | ||
| if mode == ToolCompatibilityMode::StrictEquality | ||
| && eb | ||
| .errors | ||
| .iter() | ||
| .map(|e| &e.name) | ||
| .ne(nb.errors.iter().map(|e| &e.name)) | ||
| { | ||
| errors.push(err( | ||
| format!("{}.errors", format_path(path)), | ||
| "declared error cases differ", | ||
| )); | ||
| return Vec::new(); | ||
| } | ||
| if mode == ToolCompatibilityMode::StructuralSubtype { | ||
| for expected_error in &eb.errors { | ||
| if !nb | ||
| .errors | ||
| .iter() | ||
| .any(|inner_error| inner_error.name == expected_error.name) | ||
| { | ||
| errors.push(err( | ||
| format!("{}.error.{}", format_path(path), expected_error.name), | ||
| "inner tool lacks an expected error case", | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| nb.errors | ||
| .iter() | ||
| .enumerate() | ||
| .map(|(i, n)| { | ||
| let found = eb.errors.iter().enumerate().find(|(_, e)| e.name == n.name); | ||
| let (expected_index, payload) = match found { | ||
| Some((j, e)) => ( | ||
| Some(j), | ||
| compile_error_payload(inner, n, expected, e, mode, path, errors), | ||
| ), | ||
| None => (None, None), | ||
| }; | ||
| ErrorProjection { | ||
| name: n.name.clone(), | ||
| inner_index: i, | ||
| expected_index, | ||
| payload, | ||
| } | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
Structural error compatibility only matches error cases by name and then compares payloads. ErrorCase.kind and exit_code are ignored entirely, so an inner tool that keeps the name but changes the kind/exit code passes as compatible. Please include those in the structural check (and in the strict-equality comparison).
Related, for strict-equality: strictly_equal currently also compares documentation and version strings, so a doc-comment edit on the inner tool breaks a strict chain. That seems too strict — docs at least should not participate in compatibility. Also validate_tool_middleware in validation.rs never validates version.
| for (target, default) in nf.iter().zip(defaults) { | ||
| if let Some((source_index, source)) = | ||
| ef.iter().enumerate().find(|(_, f)| f.name == target.name) | ||
| { | ||
| retained.insert(source_index); | ||
| let plan = compiler.compile( | ||
| &source.type_, | ||
| &target.type_, | ||
| &format!("{}.input.{}", format_path(path), target.name), | ||
| errors, | ||
| )?; | ||
| fields.push(RecordFieldProjection { | ||
| target_name: target.name.clone(), | ||
| source_index: Some(source_index), | ||
| plan: Some(plan), | ||
| default: None, | ||
| }); | ||
| } else if let Some(default) = default.or_else(|| option_none(&target.type_, &ngraph)) { | ||
| fields.push(RecordFieldProjection { | ||
| target_name: target.name.clone(), | ||
| source_index: None, | ||
| plan: None, | ||
| default: Some(default), | ||
| }); | ||
| } else { | ||
| errors.push(err( | ||
| format!("{}.input.{}", format_path(path), target.name), | ||
| "inner-only input has no representable default", | ||
| )); | ||
| return None; | ||
| } | ||
| } | ||
| let discard = (0..ef.len()).filter(|i| !retained.contains(i)).collect(); |
There was a problem hiding this comment.
Inputs that exist on expected but not on the inner tool end up in discard silently. This is type-correct under record width subtyping, but semantically a middleware that injects e.g. a --token/retries input towards a tool that has no such input gets it thrown away with no signal at all.
Let's emit warnings for every discarded input (path + name) so they reach the deploy diagnostics, rather than dropping them silently. The CompiledToolCompatibility result could carry a warnings: Vec<...> alongside the projections.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Please move this inline mod tests into a separate tests.rs: promote the module to a directory (<name>/mod.rs + <name>/tests.rs, or <name>.rs + <name>/tests.rs) so the implementation and its tests live in two files.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Please move this inline mod tests into a separate tests.rs: promote the module to a directory (<name>/mod.rs + <name>/tests.rs, or <name>.rs + <name>/tests.rs) so the implementation and its tests live in two files.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Please move this inline mod tests into a separate tests.rs: promote the module to a directory (<name>/mod.rs + <name>/tests.rs, or <name>.rs + <name>/tests.rs) so the implementation and its tests live in two files.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Please move this inline mod tests into a separate tests.rs: promote the module to a directory (<name>/mod.rs + <name>/tests.rs, or <name>.rs + <name>/tests.rs) so the implementation and its tests live in two files. (this PR introduces the inline test module here)
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Please move this inline mod tests into a separate tests.rs: promote the module to a directory (<name>/mod.rs + <name>/tests.rs, or <name>.rs + <name>/tests.rs) so the implementation and its tests live in two files. (this PR introduces the inline test module here)
1 participant
Resolves GOL-39
WIP