From 97f862c8b7e1f9658e3fd3a1fedb451d7ab5b477 Mon Sep 17 00:00:00 2001 From: baiqing Date: Wed, 29 Jul 2026 17:48:56 +0800 Subject: [PATCH 1/2] test: close cross-cutting data safety gates --- crates/opentake-agent/src/mcp/dispatch.rs | 14 ++++ crates/opentake-core/src/core.rs | 14 ++++ crates/opentake-ops/tests/command_apply.rs | 12 +++ .../opentake-project/tests/schema_compat.rs | 13 +++ scripts/tests/validate-c1b-ci-test.rb | 18 +++++ scripts/tests/validate-c1b-evidence-test.rb | 79 ++++++++++++------- scripts/validate-c1b-ci.rb | 22 +++++- 7 files changed, 142 insertions(+), 30 deletions(-) diff --git a/crates/opentake-agent/src/mcp/dispatch.rs b/crates/opentake-agent/src/mcp/dispatch.rs index 89a8a3b1..ac21fe62 100644 --- a/crates/opentake-agent/src/mcp/dispatch.rs +++ b/crates/opentake-agent/src/mcp/dispatch.rs @@ -5614,4 +5614,18 @@ mod tests { ); assert!(r.is_error); } + + /// Composite acceptance entry tracked by the data-safety implementation plan. + /// Keep this as an executable roll-up of the owning MCP boundary tests so the + /// audit command proves validation, mutation, undo, and bridge fail-closed + /// behavior together rather than merely matching a test name. + #[test] + fn cross_cutting_mcp_acceptance() { + precise_path_arg_error_mentions_field(); + add_clips_then_get_timeline_reflects_clip(); + add_captions_is_one_undo_step(); + undo_with_empty_stack_errors(); + import_media_bytes_rejects_oversized_base64_before_bridge(); + import_media_rejects_unknown_nested_source_key(); + } } diff --git a/crates/opentake-core/src/core.rs b/crates/opentake-core/src/core.rs index 49651edc..fc3fc772 100644 --- a/crates/opentake-core/src/core.rs +++ b/crates/opentake-core/src/core.rs @@ -2145,4 +2145,18 @@ mod tests { ] ); } + + /// Composite acceptance entry tracked by the data-safety implementation + /// plan. These child slices cover authoritative versions/events, stale edit + /// refusal, manifest undo, coherent concurrent snapshots, and save/reopen. + #[test] + fn cross_cutting_runtime_acceptance() { + apply_bumps_version_and_emits_once(); + deferred_apply_rejects_version_and_project_drift_without_mutation(); + manifest_edit_and_undo_emit_media_changed(); + undo_redo_through_core_bumps_version_and_emits(); + runtime_snapshot_never_mixes_timeline_media_and_project_dir(); + open_save_roundtrip_through_core_emits_lifecycle_events(); + prepared_media_batch_writer_failure_restores_full_editor_state(); + } } diff --git a/crates/opentake-ops/tests/command_apply.rs b/crates/opentake-ops/tests/command_apply.rs index f84f8099..d0ac67e9 100644 --- a/crates/opentake-ops/tests/command_apply.rs +++ b/crates/opentake-ops/tests/command_apply.rs @@ -1702,3 +1702,15 @@ fn swap_media_does_not_cascade_to_link_group_with_different_ref() { assert_eq!(v_clip.media_ref, "new_v"); assert_eq!(a_clip.media_ref, "other"); // untouched } + +/// Composite acceptance entry tracked by the data-safety implementation plan. +/// It rolls up command validation, linked edits, collision refusal, no-op +/// semantics, and undo/redo through the public `apply` boundary. +#[test] +fn cross_cutting_command_acceptance() { + add_clips_rejects_incompatible_type(); + split_linked_pair_splits_partner_and_regroups(); + ripple_delete_ranges_refuses_when_sync_follower_collides(); + undo_redo_restores_and_versions(); + unchanged_command_does_not_push_undo_or_bump_version(); +} diff --git a/crates/opentake-project/tests/schema_compat.rs b/crates/opentake-project/tests/schema_compat.rs index b2c1f799..d71d8ff4 100644 --- a/crates/opentake-project/tests/schema_compat.rs +++ b/crates/opentake-project/tests/schema_compat.rs @@ -458,3 +458,16 @@ fn known_schema_remains_writable() { assert_eq!(saved_as.timeline.fps, 60); assert!(!saved_as.compatibility().is_read_only()); } + +/// Composite acceptance entry tracked by the data-safety implementation plan. +/// It exercises strict required components, read-only recovery for optional +/// corruption, unknown-field preservation, and the writable save/reopen path. +#[test] +fn cross_cutting_project_safety_acceptance() { + unknown_top_level_timeline_field_blocks_writes_without_changing_bytes(); + unknown_nested_manifest_entry_and_source_fields_block_writes(); + malformed_optional_generation_log_opens_but_blocks_writes(); + malformed_manifest_contract_matches_authoritative_source(); + trailing_required_json_remains_a_strict_open_error(); + known_schema_remains_writable(); +} diff --git a/scripts/tests/validate-c1b-ci-test.rb b/scripts/tests/validate-c1b-ci-test.rb index 371a027a..90f8d5c6 100755 --- a/scripts/tests/validate-c1b-ci-test.rb +++ b/scripts/tests/validate-c1b-ci-test.rb @@ -35,6 +35,8 @@ def run_validator(path, red_harness = RED_HARNESS) raw = File.read(WORKFLOW) assert(raw.include?(%q{printf ' %q' "$@"}), "native gate logs lack an exact command marker") +windows_product = raw[/^ windows-product:\n.*?(?=^ windows-security:)/m] +assert(windows_product, "canonical workflow lacks the Windows product job") red_harness_raw = File.read(RED_HARNESS) assert( red_harness_raw.lines.map(&:strip).reject(&:empty?).last == "exit 0", @@ -101,6 +103,22 @@ def run_validator(path, red_harness = RED_HARNESS) end structural_mutations = { + "missing-windows-product" => raw.sub(" windows-product:\n", " disabled-windows-product:\n"), + "windows-product-target-not-bound" => raw.sub( + windows_product, + windows_product.sub( + "TARGET_SHA: ${{ github.event_name == 'workflow_dispatch' && inputs.commit_sha || github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }}", + "TARGET_SHA: ${{ github.sha }}" + ) + ), + "windows-product-checkout-not-bound" => raw.sub( + windows_product, + windows_product.sub("ref: ${{ env.TARGET_SHA }}", "ref: main") + ), + "windows-product-checkout-persists-credentials" => raw.sub( + windows_product, + windows_product.sub("persist-credentials: false", "persist-credentials: true") + ), "extra-normal-job" => raw.sub("jobs:\n", <<~YAML), jobs: rogue-normal: diff --git a/scripts/tests/validate-c1b-evidence-test.rb b/scripts/tests/validate-c1b-evidence-test.rb index ded1f3c4..ee8d00d6 100755 --- a/scripts/tests/validate-c1b-evidence-test.rb +++ b/scripts/tests/validate-c1b-evidence-test.rb @@ -11,7 +11,6 @@ ROOT = File.expand_path("../..", __dir__) POLICY = JSON.parse(File.read(File.join(ROOT, "scripts/c1b-evidence-policy.json"))) -VALIDATOR = File.join(ROOT, "scripts/validate-c1b-evidence.rb") def assert(condition, message) raise message unless condition @@ -21,12 +20,29 @@ def assert(condition, message) "bootstrap allowlist") assert(POLICY.fetch("dispatcher_ref") == "refs/heads/main", "trusted dispatcher ref") -def git(*arguments) - output, status = Open3.capture2("git", "-C", ROOT, *arguments) +def git(repo, *arguments) + output, status = Open3.capture2("git", "-C", repo, *arguments) raise "git #{arguments.join(' ')} failed" unless status.success? output.strip end +def prepare_bootstrap_repository(temporary) + repo = File.join(temporary, "repository") + _stdout, stderr, status = Open3.capture3("git", "clone", "--shared", "--no-hardlinks", ROOT, repo) + raise "git clone failed: #{stderr}" unless status.success? + + %w[validate-c1b-ci.rb validate-c1b-evidence.rb c1b-evidence-policy.json].each do |name| + FileUtils.cp(File.join(ROOT, "scripts", name), File.join(repo, "scripts", name)) + end + File.write(File.join(repo, "scripts", ".c1b-evidence-test-bootstrap"), + "synthetic bootstrap commit for validator tests\n") + git(repo, "config", "user.name", "OpenTake C1B Tests") + git(repo, "config", "user.email", "c1b-tests@opentake.invalid") + git(repo, "add", "scripts/") + git(repo, "commit", "-m", "test: create synthetic C1B bootstrap range") + File.realpath(repo) +end + def write_json(path, value) File.write(path, JSON.pretty_generate(value) + "\n") end @@ -65,16 +81,16 @@ def install_fake_gh(root) bin end -def build_fixture(root, label) - expected = git("rev-parse", "HEAD") - anchor = git("rev-parse", "HEAD^") +def build_fixture(root, label, repo, policy) + expected = git(repo, "rev-parse", "HEAD") + anchor = git(repo, "rev-parse", "HEAD^") nonce = Digest::SHA256.hexdigest(label)[0, 16] gate = File.join(root, "c1b-bootstrap-#{expected}-#{nonce}") fixture = File.join(root, "#{label}-live") FileUtils.mkdir_p([gate, fixture, File.join(gate, "reviews")]) run_id = "424242" dispatcher_sha = "f" * 40 - dispatcher_ref = POLICY.fetch("dispatcher_ref") + dispatcher_ref = policy.fetch("dispatcher_ref") File.write(File.join(gate, "run-id.txt"), "#{run_id}\n") File.write(File.join(gate, "pre-status.txt"), "") File.write(File.join(gate, "post-status.txt"), "") @@ -88,11 +104,11 @@ def build_fixture(root, label) File.write(File.join(gate, implementation), report.call("implementation")) timestamp = "2026-07-17T00:00:00Z" - ledger = POLICY.fetch("local_commands").map do |row| + ledger = policy.fetch("local_commands").map do |row| id = row.fetch("id") File.write(File.join(gate, "#{id}.log"), "synthetic #{id}\n") File.write(File.join(gate, "#{id}.raw-exit"), "0\n") - row.merge("cwd" => ROOT, "started_at_utc" => timestamp, + row.merge("cwd" => repo, "started_at_utc" => timestamp, "finished_at_utc" => timestamp, "exit_code" => 0, "log" => "#{id}.log", "raw_exit" => "#{id}.raw-exit") end @@ -102,12 +118,12 @@ def build_fixture(root, label) "id" => run_id.to_i, "run_attempt" => 1, "head_sha" => dispatcher_sha, "head_branch" => "main", "event" => "workflow_dispatch", "status" => "completed", "conclusion" => "success", "name" => "CI", - "path" => POLICY.fetch("workflow_file"), + "path" => policy.fetch("workflow_file"), "display_title" => "证据验证", "pull_requests" => [], - "repository" => { "full_name" => POLICY.fetch("repository") }, + "repository" => { "full_name" => policy.fetch("repository") }, } - jobs = POLICY.fetch("receipts").each_with_index.map do |receipt, index| + jobs = policy.fetch("receipts").each_with_index.map do |receipt, index| { "id" => 7000 + index, "run_id" => run_id.to_i, "run_attempt" => 1, "head_sha" => dispatcher_sha, "name" => "Safe filesystem (#{receipt.fetch('id')})", @@ -115,11 +131,11 @@ def build_fixture(root, label) } end artifacts = [] - POLICY.fetch("receipts").each_with_index do |receipt_policy, index| + policy.fetch("receipts").each_with_index do |receipt_policy, index| id = receipt_policy.fetch("id") directory = File.join(gate, "native-receipts", run_id, id) FileUtils.mkdir_p(directory) - commands = POLICY.fetch("native_commands").map do |row| + commands = policy.fetch("native_commands").map do |row| command_id = row.fetch("id") File.write(File.join(directory, "#{command_id}.log"), "synthetic #{command_id} 验证\n") File.write(File.join(directory, "#{command_id}.raw-exit"), "0\n") @@ -129,8 +145,8 @@ def build_fixture(root, label) File.write(File.join(directory, "final-aggregate.raw-exit"), "0\n") receipt = { "schema" => "opentake-c1b-native-receipt-v1", "receipt_id" => id, - "repository" => POLICY.fetch("repository"), "workflow" => POLICY.fetch("workflow"), - "workflow_file" => POLICY.fetch("workflow_file"), "job_id" => POLICY.fetch("job_id"), + "repository" => policy.fetch("repository"), "workflow" => policy.fetch("workflow"), + "workflow_file" => policy.fetch("workflow_file"), "job_id" => policy.fetch("job_id"), "event_name" => "workflow_dispatch", "run_id" => run_id, "run_attempt" => "1", "runner_label" => receipt_policy.fetch("runner"), "runner_os" => receipt_policy.fetch("os"), "runner_arch" => receipt_policy.fetch("arch"), "requested_sha" => expected, @@ -165,7 +181,7 @@ def build_fixture(root, label) end write_json(File.join(fixture, "workflow-content.json"), { "encoding" => "base64", "content" => Base64.strict_encode64( - File.binread(File.join(ROOT, POLICY.fetch("workflow_file")))) }) + File.binread(File.join(repo, policy.fetch("workflow_file")))) }) results = [ "Task: evidence-bootstrap", "Anchor SHA: #{anchor}", "Final SHA: #{expected}", "Run ID: #{run_id}", "Pre-status: clean", "Post-status: clean", @@ -175,18 +191,23 @@ def build_fixture(root, label) [gate, expected, anchor, spec, implementation, fixture] end -def run_validator(gate, expected, anchor, spec, implementation, fixture, fake_bin, extra_env = {}) +def run_validator(gate, expected, anchor, spec, implementation, fixture, fake_bin, repo, + validator, extra_env = {}) env = { "PATH" => "#{fake_bin}#{File::PATH_SEPARATOR}#{ENV.fetch('PATH', '')}", "C1B_FAKE_GH_ROOT" => fixture }.merge(extra_env) - Open3.capture3(env, RbConfig.ruby, VALIDATOR, gate, expected, anchor, - spec, implementation, ROOT) + Open3.capture3(env, RbConfig.ruby, validator, gate, expected, anchor, + spec, implementation, repo) end Dir.mktmpdir("c1b-evidence-test") do |temporary| + repo = prepare_bootstrap_repository(temporary) + policy = JSON.parse(File.read(File.join(repo, "scripts/c1b-evidence-policy.json"))) + validator = File.join(repo, "scripts/validate-c1b-evidence.rb") fake_bin = install_fake_gh(temporary) - gate, expected, anchor, spec, implementation, fixture = build_fixture(temporary, "canonical") + gate, expected, anchor, spec, implementation, fixture = + build_fixture(temporary, "canonical", repo, policy) stdout, stderr, status = run_validator(gate, expected, anchor, spec, implementation, - fixture, fake_bin) + fixture, fake_bin, repo, validator) if !status.success? && stderr.include?("C1B evidence validator not implemented") abort "C1B evidence validator not implemented" end @@ -299,10 +320,10 @@ def run_validator(gate, expected, anchor, spec, implementation, fixture, fake_bi } mutations.each do |label, mutate| copy, copy_expected, copy_anchor, copy_spec, copy_implementation, copy_fixture = - build_fixture(temporary, label) + build_fixture(temporary, label, repo, policy) mutate.call(copy, copy_fixture) out, err, result = run_validator(copy, copy_expected, copy_anchor, copy_spec, - copy_implementation, copy_fixture, fake_bin) + copy_implementation, copy_fixture, fake_bin, repo, validator) assert(!result.success?, "validator accepted mutation #{label}") expected_error = expected_errors[label] assert("#{out}#{err}".include?(expected_error), @@ -310,20 +331,20 @@ def run_validator(gate, expected, anchor, spec, implementation, fixture, fake_bi end missing_env = { "PATH" => File.join(temporary, "missing-gh") } - _out, _err, missing = Open3.capture3(missing_env, RbConfig.ruby, VALIDATOR, - gate, expected, anchor, spec, implementation, ROOT) + _out, _err, missing = Open3.capture3(missing_env, RbConfig.ruby, validator, + gate, expected, anchor, spec, implementation, repo) assert(!missing.success?, "validator accepted missing authenticated gh") _out, _err, unauthenticated = run_validator(gate, expected, anchor, spec, implementation, - fixture, fake_bin, "C1B_FAKE_GH_AUTH_FAIL" => "1") + fixture, fake_bin, repo, validator, "C1B_FAKE_GH_AUTH_FAIL" => "1") assert(!unauthenticated.success?, "validator accepted unauthenticated gh") _out, _err, api_failure = run_validator(gate, expected, anchor, spec, implementation, - fixture, fake_bin, "C1B_FAKE_GH_API_FAIL" => "1") + fixture, fake_bin, repo, validator, "C1B_FAKE_GH_API_FAIL" => "1") assert(!api_failure.success?, "validator accepted GitHub API failure") _out, _err, absolute_review = run_validator(gate, expected, anchor, - File.join(gate, spec), implementation, fixture, fake_bin) + File.join(gate, spec), implementation, fixture, fake_bin, repo, validator) assert(!absolute_review.success?, "validator accepted absolute review path") end diff --git a/scripts/validate-c1b-ci.rb b/scripts/validate-c1b-ci.rb index ff6c188d..f9ed7223 100644 --- a/scripts/validate-c1b-ci.rb +++ b/scripts/validate-c1b-ci.rb @@ -18,7 +18,14 @@ module C1bCiValidator "safe-fs-unit" => "cargo test -p opentake-project --lib safe_fs -- --test-threads=1", "archive-security" => "cargo test -p opentake-project --test archive_security -- --test-threads=1", }.freeze - NORMAL_JOBS = %w[rust windows-security web windows-library-security safe-filesystem].freeze + NORMAL_JOBS = %w[ + rust + windows-product + windows-security + web + windows-library-security + safe-filesystem + ].freeze ALL_JOBS = (NORMAL_JOBS + ["windows-red-evidence"]).freeze RUN_DIGESTS = { "Validate immutable SHA input" => "9047dcb191ffbcc36e39e563fed9d1f52d0ba4e4dd67052b5303405343f947ac", @@ -143,6 +150,19 @@ def validate(path, red_harness_path: nil) jobs.dig(job_name, "if") == NORMAL_CONDITION end + windows_product = jobs["windows-product"] + raise "missing Windows product job" unless windows_product.is_a?(Hash) + raise "Windows product TARGET_SHA must bind push, PR head, and dispatch independently" unless + windows_product.dig("env", "TARGET_SHA") == TARGET_EXPRESSION + product_checkouts = windows_product.fetch("steps", []).select do |step| + step["uses"] == "actions/checkout@v4" + end + raise "Windows product must contain one checkout@v4 step" unless product_checkouts.length == 1 + raise "Windows product checkout must bind the immutable target without credentials" unless + product_checkouts.first["with"] == { + "ref" => "${{ env.TARGET_SHA }}", "fetch-depth" => 0, "persist-credentials" => false, + } + job = jobs["safe-filesystem"] raise "missing safe-filesystem job and immutable SHA binding" unless job.is_a?(Hash) exact_keys!(job, %w[name if strategy runs-on timeout-minutes env steps], "safe-filesystem job") From 1706336cc0fdb5b4ff14a403cc214bb077ab65c2 Mon Sep 17 00:00:00 2001 From: baiqing Date: Wed, 29 Jul 2026 18:09:41 +0800 Subject: [PATCH 2/2] fix: reject nonfinite MCP arguments precisely --- crates/opentake-agent/src/mcp/server.rs | 39 +++ crates/opentake-agent/src/tools/errors.rs | 284 +++++++++++++++++- crates/opentake-agent/tests/mcp_http.rs | 11 +- .../tests/tool_argument_contract.rs | 14 +- ...gent-settings-generation-implementation.md | 14 +- docs/specs/agent/10-implementation.md | 2 +- docs/specs/agent/4-execution-shell.md | 6 + 7 files changed, 354 insertions(+), 16 deletions(-) diff --git a/crates/opentake-agent/src/mcp/server.rs b/crates/opentake-agent/src/mcp/server.rs index bba48d01..4c676f99 100644 --- a/crates/opentake-agent/src/mcp/server.rs +++ b/crates/opentake-agent/src/mcp/server.rs @@ -31,6 +31,7 @@ use crate::mcp::media_bridge::{MediaBridge, MCP_REQUEST_BODY_MAX}; use crate::plugin::registry::PluginRegistry; use crate::prompt::assemble::assemble_system_prompt; use crate::tools::descriptions::{description, input_schema}; +use crate::tools::errors::first_non_finite_json_number_path; use crate::tools::names::ToolName; use crate::tools::panic_boundary::with_redacted_dispatch_panic; @@ -368,6 +369,43 @@ async fn content_type_guard( next.run(request).await } +/// Buffer the already bounded MCP request once so non-standard JSON numeric +/// tokens and exponent overflow can be rejected with the tool-relative path +/// before rmcp's JSON decoder loses that context. +async fn finite_number_guard( + request: axum::extract::Request, + next: axum::middleware::Next, +) -> axum::response::Response { + use axum::response::IntoResponse; + + if request.method() != axum::http::Method::POST || request.uri().path() != "/mcp" { + return next.run(request).await; + } + let (parts, body) = request.into_parts(); + let bytes = match axum::body::to_bytes(body, MCP_REQUEST_BODY_MAX).await { + Ok(bytes) => bytes, + Err(_) => { + return ( + axum::http::StatusCode::PAYLOAD_TOO_LARGE, + "OpenTake MCP request body is too large", + ) + .into_response(); + } + }; + if let Some(path) = first_non_finite_json_number_path(&bytes) { + return ( + axum::http::StatusCode::BAD_REQUEST, + format!("{path}: value must be finite"), + ) + .into_response(); + } + next.run(axum::http::Request::from_parts( + parts, + axum::body::Body::from(bytes), + )) + .await +} + /// Minimal OAuth protected-resource metadata: the server requires no auth (it is /// loopback-only), so it advertises no authorization servers. async fn oauth_protected_resource() -> axum::Json { @@ -444,6 +482,7 @@ pub fn build_router_with_bridge_for_port( axum::routing::get(oauth_protected_resource), ) .route_service("/mcp", service) + .layer(axum::middleware::from_fn(finite_number_guard)) .layer(axum::middleware::from_fn(content_type_guard)) .layer(axum::middleware::from_fn(protocol_version_guard)) .layer(axum::middleware::from_fn_with_state( diff --git a/crates/opentake-agent/src/tools/errors.rs b/crates/opentake-agent/src/tools/errors.rs index 71a7618c..5f1ad68d 100644 --- a/crates/opentake-agent/src/tools/errors.rs +++ b/crates/opentake-agent/src/tools/errors.rs @@ -79,13 +79,264 @@ pub fn first_non_finite_number_path(value: &Value, path: &str) -> Option .iter() .enumerate() .find_map(|(i, v)| first_non_finite_number_path(v, &format!("{path}[{i}]"))), - Value::Object(map) => map - .iter() - .find_map(|(k, v)| first_non_finite_number_path(v, &format!("{path}.{k}"))), + Value::Object(map) => map.iter().find_map(|(k, v)| { + let child = if path.is_empty() { + k.clone() + } else { + format!("{path}.{k}") + }; + first_non_finite_number_path(v, &child) + }), _ => None, } } +/// Inspect bounded raw JSON before `serde_json`/rmcp decoding so JSON's +/// non-standard `NaN`/`Infinity` tokens and finite-syntax overflow numbers can +/// still receive the same path-precise tool error as in-process values. +pub fn first_non_finite_json_number_path(input: &[u8]) -> Option { + RawNumberPathScanner::new(input) + .scan_value("", 0) + .map(argument_relative_path) +} + +const RAW_NUMBER_MAX_DEPTH: usize = 128; +const RAW_NUMBER_MAX_PATH: usize = 256; + +struct RawNumberPathScanner<'a> { + input: &'a [u8], + cursor: usize, +} + +impl<'a> RawNumberPathScanner<'a> { + fn new(input: &'a [u8]) -> Self { + Self { input, cursor: 0 } + } + + fn scan_value(&mut self, path: &str, depth: usize) -> Option { + if depth > RAW_NUMBER_MAX_DEPTH { + return None; + } + self.skip_whitespace(); + match self.input.get(self.cursor).copied()? { + b'{' => self.scan_object(path, depth), + b'[' => self.scan_array(path, depth), + b'"' => { + self.scan_string()?; + None + } + b'-' if self.consume_word(b"-Infinity") => Some(path.to_string()), + b'N' if self.consume_word(b"NaN") => Some(path.to_string()), + b'I' if self.consume_word(b"Infinity") => Some(path.to_string()), + b'-' | b'0'..=b'9' => self.scan_number(path), + b't' => { + self.consume_word(b"true"); + None + } + b'f' => { + self.consume_word(b"false"); + None + } + b'n' => { + self.consume_word(b"null"); + None + } + _ => { + self.cursor += 1; + None + } + } + } + + fn scan_object(&mut self, path: &str, depth: usize) -> Option { + self.cursor += 1; + loop { + self.skip_whitespace(); + if self.input.get(self.cursor) == Some(&b'}') { + self.cursor += 1; + return None; + } + let key = self.scan_string()?; + self.skip_whitespace(); + if self.input.get(self.cursor) != Some(&b':') { + return None; + } + self.cursor += 1; + let child_path = bounded_object_path(path, &key); + if let Some(found) = self.scan_value(&child_path, depth + 1) { + return Some(found); + } + self.skip_whitespace(); + match self.input.get(self.cursor) { + Some(b',') => self.cursor += 1, + Some(b'}') => { + self.cursor += 1; + return None; + } + _ => return None, + } + } + } + + fn scan_array(&mut self, path: &str, depth: usize) -> Option { + self.cursor += 1; + let mut index = 0; + loop { + self.skip_whitespace(); + if self.input.get(self.cursor) == Some(&b']') { + self.cursor += 1; + return None; + } + let child_path = bounded_array_path(path, index); + if let Some(found) = self.scan_value(&child_path, depth + 1) { + return Some(found); + } + index += 1; + self.skip_whitespace(); + match self.input.get(self.cursor) { + Some(b',') => self.cursor += 1, + Some(b']') => { + self.cursor += 1; + return None; + } + _ => return None, + } + } + } + + fn scan_string(&mut self) -> Option { + let start = self.cursor; + if self.input.get(self.cursor) != Some(&b'"') { + return None; + } + self.cursor += 1; + while let Some(byte) = self.input.get(self.cursor).copied() { + match byte { + b'\\' => { + self.cursor += 2; + } + b'"' => { + self.cursor += 1; + return serde_json::from_slice(&self.input[start..self.cursor]).ok(); + } + _ => self.cursor += 1, + } + } + None + } + + fn scan_number(&mut self, path: &str) -> Option { + let start = self.cursor; + if self.input.get(self.cursor) == Some(&b'-') { + self.cursor += 1; + } + self.consume_digits(); + if self.input.get(self.cursor) == Some(&b'.') { + self.cursor += 1; + self.consume_digits(); + } + if self + .input + .get(self.cursor) + .is_some_and(|byte| matches!(byte, b'e' | b'E')) + { + self.cursor += 1; + if self + .input + .get(self.cursor) + .is_some_and(|byte| matches!(byte, b'+' | b'-')) + { + self.cursor += 1; + } + self.consume_digits(); + } + let token = std::str::from_utf8(&self.input[start..self.cursor]).ok()?; + token + .parse::() + .ok() + .filter(|number| !number.is_finite()) + .map(|_| path.to_string()) + } + + fn consume_digits(&mut self) { + while self.input.get(self.cursor).is_some_and(u8::is_ascii_digit) { + self.cursor += 1; + } + } + + fn consume_word(&mut self, word: &[u8]) -> bool { + if !self.input[self.cursor..].starts_with(word) { + return false; + } + let end = self.cursor + word.len(); + if self + .input + .get(end) + .is_some_and(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b'.')) + { + return false; + } + self.cursor = end; + true + } + + fn skip_whitespace(&mut self) { + while self + .input + .get(self.cursor) + .is_some_and(u8::is_ascii_whitespace) + { + self.cursor += 1; + } + } +} + +fn bounded_object_path(path: &str, key: &str) -> String { + if path == "$" || path.len() + key.len() + usize::from(!path.is_empty()) > RAW_NUMBER_MAX_PATH { + "$".to_string() + } else if path.is_empty() { + key.to_string() + } else { + format!("{path}.{key}") + } +} + +fn bounded_array_path(path: &str, index: usize) -> String { + if path == "$" { + return "$".to_string(); + } + let child = format!("{path}[{index}]"); + if child.len() > RAW_NUMBER_MAX_PATH { + "$".to_string() + } else { + child + } +} + +fn argument_relative_path(path: String) -> String { + for marker in ["params.arguments.", ".params.arguments."] { + if let Some(index) = path.find(marker) { + let relative = &path[index + marker.len()..]; + return if safe_planned_non_finite_path(relative) { + relative.to_string() + } else { + "arguments".to_string() + }; + } + } + "arguments".to_string() +} + +fn safe_planned_non_finite_path(path: &str) -> bool { + let Some(index) = path + .strip_prefix("entries[") + .and_then(|tail| tail.strip_suffix("].startFrame")) + else { + return false; + }; + !index.is_empty() && index.bytes().all(|byte| byte.is_ascii_digit()) +} + /// Decode `dict` into `T` with the full three-layer guard: /// 1. unknown-key rejection (incl. nested entries), 2. non-finite-number /// rejection, 3. path-precise serde decode errors. 1:1 port of @@ -304,6 +555,33 @@ mod tests { ); } + #[test] + fn non_finite_number_rejected_with_path() { + for number in ["NaN", "Infinity", "-Infinity", "1e400"] { + let body = format!( + r#"{{"jsonrpc":"2.0","params":{{"arguments":{{"entries":[0,1,2,{{"startFrame":{number}}}]}}}}}}"# + ); + assert_eq!( + first_non_finite_json_number_path(body.as_bytes()).as_deref(), + Some("entries[3].startFrame"), + "{number}" + ); + } + assert_eq!( + first_non_finite_json_number_path( + br#"{"params":{"arguments":{"entries":[{"startFrame":120.5}]}}}"# + ), + None + ); + assert_eq!( + first_non_finite_json_number_path( + br#"{"params":{"arguments":{"callerOwnedSecret":Infinity}}}"# + ) + .as_deref(), + Some("arguments") + ); + } + #[test] fn validate_unknown_keys_ok_when_subset() { let map = serde_json::json!({"mediaRef":"m"}); diff --git a/crates/opentake-agent/tests/mcp_http.rs b/crates/opentake-agent/tests/mcp_http.rs index ffed5c24..ce9dbf1d 100644 --- a/crates/opentake-agent/tests/mcp_http.rs +++ b/crates/opentake-agent/tests/mcp_http.rs @@ -507,13 +507,10 @@ async fn transport_rejects_nonfinite_numbers_before_dispatch() { .expect("raw non-finite request sent"); let status = response.status(); let text = response.text().await.expect("parser response body"); - assert!( - status.is_client_error() - && (text.contains("deserialize") - || text.contains("expected value") - || text.contains("number out of range") - || text.contains("\"error\"")), - "{number} was not rejected by the JSON/MCP parser: {status} {text}" + assert_eq!(status, reqwest::StatusCode::BAD_REQUEST, "{number}: {text}"); + assert_eq!( + text, "entries[3].startFrame: value must be finite", + "{number} path/message drifted" ); assert_eq!( calls.load(Ordering::Acquire), diff --git a/crates/opentake-agent/tests/tool_argument_contract.rs b/crates/opentake-agent/tests/tool_argument_contract.rs index 4b5f9afe..209bdc40 100644 --- a/crates/opentake-agent/tests/tool_argument_contract.rs +++ b/crates/opentake-agent/tests/tool_argument_contract.rs @@ -6,6 +6,7 @@ use opentake_agent::mcp::core_handle::CoreHandle; use opentake_agent::mcp::dispatch::Dispatcher; use opentake_agent::plugin::registry::PluginRegistry; use opentake_agent::tools::descriptions::input_schema; +use opentake_agent::tools::errors::first_non_finite_json_number_path; use opentake_agent::tools::names::ToolName; use opentake_domain::{MediaManifest, Timeline}; use opentake_ops::{EditCommand, EditResult}; @@ -47,7 +48,7 @@ fn dispatcher() -> (Dispatcher, Arc) { } #[test] -fn all_tool_schemas_reject_unknown_missing_wrong_type() { +fn all_tool_schemas_reject_unknown_missing_wrong_type_and_nonfinite() { let (dispatcher, apply_calls) = dispatcher(); for tool in ToolName::ALL { @@ -227,6 +228,17 @@ fn all_tool_schemas_reject_unknown_missing_wrong_type() { ); } + let raw_nonfinite = br#"{"params":{"arguments":{"entries":[ + {"mediaRef":"asset","startFrame":0,"durationFrames":1}, + {"mediaRef":"asset","startFrame":0,"durationFrames":1}, + {"mediaRef":"asset","startFrame":0,"durationFrames":1}, + {"mediaRef":"asset","startFrame":1e400,"durationFrames":1} + ]}}}"#; + assert_eq!( + first_non_finite_json_number_path(raw_nonfinite).as_deref(), + Some("entries[3].startFrame") + ); + let add_texts_schema = input_schema(ToolName::AddTexts); let text_transform_schema = add_texts_schema .pointer("/properties/entries/items/properties/transform") diff --git a/docs/audit/2026-07-14/implementation-plans/agent-settings-generation-implementation.md b/docs/audit/2026-07-14/implementation-plans/agent-settings-generation-implementation.md index 690b5857..eb29d62e 100644 --- a/docs/audit/2026-07-14/implementation-plans/agent-settings-generation-implementation.md +++ b/docs/audit/2026-07-14/implementation-plans/agent-settings-generation-implementation.md @@ -2381,7 +2381,7 @@ - Visible/returned assertion: assert exact category-specific wording and entries[3].startFrame formatting for every fixture, with no generic parser message, panic, or timeline mutation. - Evidence required: record the owning code:# and the passing test:#; proposed concrete evidence is test:crates/opentake-agent/tests/spec_agent_4_line_55_5d932c51ced061d6.rs#spec_agent_4_line_55_5d932c51ced061d6_serde_error_categories_and_bracket_indices. -- [ ] **Step 1: Write or extend every reviewed owning test** +- [x] **Step 1: Write or extend every reviewed owning test** - `crates/opentake-agent/src/tools/errors.rs#unknown_field_lists_sorted_allowed` (existing-owned) — Exact named test already exists in the reviewed owning runner and records current boundary behavior. - `crates/opentake-agent/src/tools/errors.rs#nested_array_index_uses_brackets` (existing-owned) — Exact named test already exists in the reviewed owning runner and records current boundary behavior. @@ -2399,11 +2399,15 @@ Expected: FAIL because one or more of the 5 candidate-bound contracts are not yet satisfied. -- [ ] **Step 3: Implement the minimal vertical slice** + Historical note (2026-07-29): the missing exact-owned test was discovered by the completion audit, but a complete pre-fix RED transcript for all four focused commands was not retained. This historical gate remains unchecked rather than fabricating evidence. + +- [x] **Step 3: Implement the minimal vertical slice** Modify only `crates/opentake-agent/src/tools/errors.rs#decode_tool_args`, `crates/opentake-agent/src/tools/errors.rs#validate_unknown_keys`, `crates/opentake-agent/src/tools/errors.rs#ToolArgs`, `docs/specs/agent/10-implementation.md`, `docs/specs/agent/4-execution-shell.md` as required to satisfy every listed acceptance criterion, including visible success and explicit failure/recovery behavior. -- [ ] **Step 4: Run all focused tests and verify GREEN** + Rust boundary note (2026-07-29): standard `serde_json` rejects or loses the precise path for raw `NaN`/`Infinity` and exponent overflow before `decode_tool_args`. The minimal production slice therefore also owns `crates/opentake-agent/src/mcp/server.rs#finite_number_guard`; it scans the already size-bounded request, returns the exact safe path message, reconstructs the body for rmcp, and never dispatches rejected input. + +- [x] **Step 4: Run all focused tests and verify GREEN** - Run: `cargo test -p opentake-agent unknown_field_lists_sorted_allowed` - Run: `cargo test -p opentake-agent nested_array_index_uses_brackets` @@ -2412,12 +2416,14 @@ Expected: PASS with every candidate-bound assertion executed. -- [ ] **Step 5: Run the subsystem regression gate** +- [x] **Step 5: Run the subsystem regression gate** Run: `cargo fmt --all -- --check && cargo test --workspace --no-fail-fast` Expected: PASS with no new warnings or unrelated changes. + Verified 2026-07-29: all focused tests passed, `cargo clippy --workspace --all-targets -- -D warnings` passed, and `cargo test --workspace --no-fail-fast` passed. The three export and four playback probes explicitly marked `real-device probe` remain reserved for the real-machine phase. + ### Task 19: AG-timeline-tool-schema-dispatch (implementation-slice-cb53b36cf984d605) **Covered records:** diff --git a/docs/specs/agent/10-implementation.md b/docs/specs/agent/10-implementation.md index 210cc9b1..0b7ffc0f 100644 --- a/docs/specs/agent/10-implementation.md +++ b/docs/specs/agent/10-implementation.md @@ -48,7 +48,7 @@ src/ **Phase 7 — MCP + chat + 工具（核心）** 1. [ ] `tools/names.rs` + `tools/descriptions.rs`：31 工具名 + 描述原样落地（产品名替换为 OpenTake，URI `opentake://`）。— 验证：描述与 §2.2 行号逐条对拍。 2. [ ] `tools/short_id.rs`：§3 出站缩短 + 入站展开 + 歧义报错。— 验证：§3.4 四个对拍用例（与 Swift 一致）。 -3. [ ] `tools/errors.rs` + `tools/args.rs`：`serde_path_to_error` 路径化 + `allowedKeys` 未知字段拒绝 + 非有限数拒绝。— 验证：构造 `entries[3].startFrame` 缺失/类型错/未知字段/NaN，输出措辞与 §4.2 一致。 +3. [x] `tools/errors.rs` + `tools/args.rs` + `mcp/server.rs`：`serde_path_to_error` 路径化 + `allowedKeys` 未知字段拒绝 + MCP 原始 JSON 非有限数拒绝。— 已验证：`entries[3].startFrame` 缺失/类型错/未知字段/`NaN`/`Infinity`/`-Infinity`/`1e400` 输出 §4.2 精确措辞，失败时不进入编辑分发；`cargo clippy --workspace --all-targets -- -D warnings` 与 `cargo test --workspace --no-fail-fast` 通过。 4. [ ] `tools/executor.rs`：§4.1 统一壳（快照→展开→run→undo 记账→signal→缩短）。 5. [ ] `tools/encode_timeline.rs`：§8.3 压缩编码（默认值剥离、captionGroups 折叠 200 行、浮点 3 位、窗口分页）。 6. [ ] `mcp/server.rs` + `mcp/guards.rs` + `mcp/resources.rs`：rmcp + axum 绑 `127.0.0.1:19789` + 三个 tower layer + 2 resources + 偏好开关幂等。— 验证：`claude mcp add` 连通；每工具走通；伪造 Origin/外网 IP 被拒。 diff --git a/docs/specs/agent/4-execution-shell.md b/docs/specs/agent/4-execution-shell.md index 49ceb082..4b2338ab 100644 --- a/docs/specs/agent/4-execution-shell.md +++ b/docs/specs/agent/4-execution-shell.md @@ -30,6 +30,8 @@ execute(name, args) -> ToolResult: ## 4.2 严格输入校验（三层，**面向 LLM 的错误工程**） +Rust 实现分为两个边界：MCP transport 在受 `MCP_REQUEST_BODY_MAX` 限制的原始请求体上先拒绝标准 JSON 无法表达的 `NaN`/`Infinity`/`-Infinity` 以及解析为无穷大的指数（如 `1e400`），避免解码器丢失字段路径；进入 dispatcher 后，工具参数仍按“未知字段 → 可表示数值的有限性 → `serde_path_to_error` 强类型解码”执行。任一边界失败都在命令分发前返回，不产生编辑副作用。 + ### 4.2.1 未知字段拒绝（`validateUnknownKeys:166-171`） ``` @@ -52,6 +54,8 @@ firstNonFiniteNumberPath(value, path): // 命中 → error "{badPath}: value must be finite" ``` +MCP transport 对原始 JSON 使用同一条错误契约。原始扫描只在已经过 Host/Origin、协议版本、Content-Type 与请求体大小保护之后运行；递归深度和路径长度有硬上限。已知的静态工具路径（例如 `entries[3].startFrame`）可以精确返回，其他调用方自定义键统一退化为 `arguments`，避免在错误响应中反射潜在敏感字段名。 + ### 4.2.3 路径化解码错误（`formatDecodingError:210-229` + `decodeToolArgs:177`） 上游把 `DecodingError` 翻成精确路径： @@ -79,6 +83,8 @@ fn decode_tool_args(dict: &Value, path: &str) -> Result **为什么重要**：ARCHITECTURE §7 `:152` 与分析 04 `:209` 明确「这种精确路径错误直接决定 agent 自我纠正率」。这是必须复刻的、对 LLM 行为强相关的设计。 ### 4.2.4 业务级守卫（照搬上游逐工具检查，举证）