fix: first confirmation of chat tool calls always failed — canonicalize fingerprint JSON

[thomasluizon]

Problem

Every confirmation-gated chat tool call failed its first Confirm with confirmation_required and succeeded on retry — reproduced consistently in launch QA (bulk habit creation via Astra).

Root cause: PendingAgentOperationState.ArgumentsJson is a jsonb column. PostgreSQL normalizes jsonb (reorders object keys, strips whitespace), so AiController.ExecutePendingOperation re-executes with arguments whose GetRawText() differs byte-for-byte from the original tool call. The recomputed fingerprint never matched the stored one, TryConsumeFreshConfirmation failed, and policy demanded a fresh confirmation — whose pending op, built from the already-normalized form, then matched on the second attempt. Deterministic fail-once-work-twice. This predates #288 (the raw-string comparison had the identical mismatch).

Fix

AgentOperationFingerprint.Compute now canonicalizes the arguments before hashing: parse → recursive re-serialization with ordinal-sorted object keys, compact output. Both construction sites (executor + MCP middleware) already funnel through this helper, so creation and consumption agree regardless of storage normalization or client/server whitespace. Non-JSON input hashes as opaque text.

Deploy note: pending operations in flight at deploy time mismatch once more (TTL is minutes); after that, first-confirm works everywhere.

Validation

New tests: fingerprint stable across key-reordering/whitespace (jsonb-style round-trip), opaque non-JSON handling; existing bound/determinism/differentiation tests unchanged. Full suite: Domain 486 ✓ / Application 2571 ✓ / Infrastructure 1368 ✓.

Refs #288, found in thomasluizon/orbit-ui-mobile#382 launch QA

🤖 Generated with Claude Code

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[claude]

Reviewed. No Critical/High findings — approving.

[claude]

Code Review: PR #289

Scope: PR #289 — canonicalize fingerprint JSON so jsonb round-trips match on confirm
Recommendation: APPROVE

Summary

Two-file diff: src/Orbit.Domain/Common/AgentOperationFingerprint.cs (+60/-5) and its test file (+25). AgentOperationFingerprint.Compute now canonicalizes the arguments JSON (parse → recursively re-serialize with ordinal-sorted object keys, compact whitespace, opaque fallback for non-JSON) before hashing.

Traced the full call chain to confirm this is a root-cause fix, not a symptom patch:

• The fingerprint is computed once from the client's raw tool-call JSON (execution.Arguments.GetRawText() in AgentOperationExecutor.EvaluatePolicy) when a pending operation is created.
• It's recomputed later from PendingAgentOperationState.ArgumentsJson — after a Postgres jsonb round-trip, which reorders keys and strips whitespace — when the confirmed operation is re-executed (PendingAgentOperationStore.GetExecution → ParseArguments).
• Ordinal string comparison in TryConsumeFreshConfirmation / IsUsable never matched the two on the first attempt.
• Canonicalizing both computations closes the loop. Both construction call sites (AgentOperationExecutor, and the MCP execute_agent_operation_v2 middleware in WebApplicationExtensions.cs) already funnel through this one helper, so it's a single point of fix with no risk of a third divergent call site.

Findings

Critical / High / Medium: none.

Low / Info:

• Scientific-notation numbers are a residual edge case. WriteCanonical's scalar branch calls element.WriteTo(writer), which preserves the original numeric token text verbatim (e.g. 1.500 stays 1.500). Postgres jsonb normalizes exponential notation to decimal on storage (1e2 → 100), so a tool-call argument containing a number in scientific notation would still mismatch pre/post round-trip. Likelihood is low — tool-call arguments in this codebase are titles/booleans/plain integers — so not blocking, noted for awareness only.

Security

Reviewed the canonicalization for collision and DoS risk: it only reorders object keys and normalizes whitespace — the values themselves (via JsonElement.WriteTo) fully determine the hash, so two semantically different argument payloads cannot collide onto the same fingerprint. Recursion depth is bounded by JSON nesting (small tool-call payloads, no untrusted deep-nesting vector). The JsonException fallback (hash the raw string as opaque text) matches the pre-fix behavior for malformed input — no regression. The fingerprint is used only for confirmation-matching, never as an authorization decision by itself.

Contract drift

N/A — no DTO, Controller route, or packages/shared type touched by this diff.

What's good

• Root-cause fix, matches orbit-api's "No workarounds" rule directly.
• New tests target the exact reported failure mode (jsonb-style key reordering + whitespace stability, plus non-JSON opacity), not just generic coverage.
• XML-doc comment update explains why canonicalization is needed (jsonb normalization) — a WHY note, not narration.
• PR body traces the root cause precisely and links back to the prior related fix (fix: hash agent operation fingerprint — bulk tool calls overflowed varchar(256) #288).

Validation

Build/test could not be re-run from this review session (sandbox restriction on this pass). Relying on the PR's own reported run: Domain 486 ✓ / Application 2571 ✓ / Infrastructure 1368 ✓. Recommend confirming via CI's Build/Test checks before merge.

🤖 Generated with Claude Code