fix: hash agent operation fingerprint — bulk tool calls overflowed varchar(256)

[thomasluizon]

Problem

Asking Astra to create 10 habits killed the whole chat turn with "Desculpe, algo deu errado". Server log:

SqlState 22001: value too long for type character varying(256)
at PendingAgentOperationStore.Create(...)
at AgentPolicyEvaluator.EvaluateConfirmationRequirement(...)

AgentOperationExecutor.EvaluatePolicy built the operation fingerprint as {operationId}:{rawArgumentsJson} — unbounded — and it lands in the OperationFingerprint varchar(256) column when a confirmation-gated mutation creates a pending operation. Any bulk payload (bulk_create_habits, bulk_log_habits, …) larger than ~256 chars made the insert throw, which propagated up through the tool-call loop and failed the entire chat command.

Fix

The fingerprint is a deterministic dedupe/confirmation key (matched on create-dedupe and TryConsumeFreshConfirmation), never displayed — so it's now the SHA-256 hex of the same source string: fixed 64 chars, same idiom as PendingAgentOperationStore.HashToken. Single construction site; both chat and MCP surfaces funnel through it, and the confirm/re-execute path recomputes it from the same inputs, so matching is unaffected.

Deploy note: pending operations created before the deploy hold raw fingerprints; a confirmation arriving after the deploy recomputes the hashed form and won't match — the user is simply asked to confirm once more. TTL is minutes, no migration needed (column bound unchanged).

Validation

Regression test: 10-habit bulk payload → fingerprint is 64-char hex, deterministic for identical args, distinct for different args. Full suite: Domain 481 ✓ / Application 2571 ✓ / Infrastructure 1368 ✓.

Refs thomasluizon/orbit-ui-mobile#382 (found during launch QA)

🤖 Generated with Claude Code

[claude]

/pr-review — PR #288 (fix/agent-fingerprint-overflow)

Scope: AgentOperationExecutor.ComputeOperationFingerprint (SHA-256 hash of {operationId}:{argumentsJson}) + regression test. CI adaptations per workflow: dotnet build/test skipped (separate required checks), cross-repo orbit-ui-mobile dimensions marked not verifiable here (repo not checked out).

Findings

[Critical] The claimed fix does not cover the MCP direct-tool-call surface — the exact same crash is still reproducible today
· dimension: Correctness (does the fix do what the PR says, across every boundary it crosses?)
· location: src/Orbit.Api/Extensions/WebApplicationExtensions.cs:370,372,376 (TryGetMcpToolCall)
· issue: The PR body claims "Single construction site; both chat and MCP surfaces funnel through it." That's true only for the execute_agent_operation_v2 MCP tool (explicitly skipped in HandleMcpToolCallAsync:146-150 and deferred to next() → AgentOperationExecutor, which is fixed). Every other MCP tool — including bulk_create_habits, bulk_log_habits, bulk_skip_habits (HabitTools.cs:349,403,423, mapped via mcpTools: [...] in AgentCatalogService.Capabilities.cs:243 to capability HabitsBulkWrite, which is isMutation: true + AgentConfirmationRequirement.FreshConfirmation) — is intercepted by UseMcpSelectiveAuth before reaching McpExecutorBridge/AgentOperationExecutor. That middleware builds its own raw, unhashed, unbounded fingerprint ($"{toolName}:{paramsElement.GetRawText()}") and passes it straight into AgentPolicyEvaluator.Evaluate → EvaluateConfirmationRequirement → PendingAgentOperationStore.Create(...) (AgentPolicyEvaluator.cs:115-122, PendingAgentOperationStore.cs:39-53), which persists it into the same varchar(256) OperationFingerprint column with no truncation.
· risk: An MCP client (e.g. Claude Desktop, or any external agent) calling bulk_create_habits directly with a 10-habit payload — the identical scenario in this PR's own regression test — never reaches the fixed code path, since FreshConfirmation means the first call (no confirmationToken yet) is always intercepted by this middleware pre-check. The raw JSON easily exceeds 256 chars, so SaveChanges() throws the same SqlState 22001 this PR set out to fix, on the MCP surface specifically — the surface explicitly named as covered.
· fix: Route the middleware's fingerprint construction through the same ComputeOperationFingerprint-style hash (ideally by extracting it to a shared location both AgentOperationExecutor and WebApplicationExtensions.TryGetMcpToolCall call, since it's currently private static on AgentOperationExecutor) so the pre-check path persists a bounded, hashed fingerprint too.
· reference: PR description's own stated scope/regression claim; orbit-api hard rule "No workarounds — root-cause every bug" (a fix that leaves the reported crash reachable via a documented, live surface is not a root-cause fix).

Other dimensions

• Dead/stale code, SOLID, comment policy, type safety, console.log, no-workaround (self-contained diff): clean. The new ComputeOperationFingerprint XML-doc documents non-obvious intent (why hashing, why this satisfies the column bound) — compliant with the comment policy.
• Security: PASS — plain SHA-256 is appropriate for a dedupe/matching key (not a MAC/secret); every lookup is additionally scoped by (UserId, CapabilityId) plus IsUsable's expiry/consumed checks, so a hash collision alone can't cross-confirm another user's/capability's pending op. No new PII exposure — hashing is strictly better than the prior raw-JSON fingerprint.
• Tests: the new regression test (AgentExecutionAndSanitizerTests.cs) correctly asserts 64-char uppercase hex, determinism, and differentiation — good coverage for the path it targets.
• Contract parity / backward-compat guard (packages/shared, orbit-ui-mobile): not verifiable in this job — sibling repo not checked out.
• Deploy note in PR body (pre-deploy pending ops need one re-confirmation): reasonable, no migration needed, consistent with the fixed path's minimal blast radius.

Recommendation

Request changes — the one Critical finding is that the fix, as scoped, leaves the reported production crash reachable through a live, documented MCP tool surface (bulk habit operations), directly contradicting the PR's own "both surfaces" claim.

[thomasluizon]

Addressed the Critical finding: fingerprint hashing extracted to Orbit.Domain.Common.AgentOperationFingerprint and the MCP selective-auth middleware (WebApplicationExtensions.TryGetMcpToolCall) now builds all three fingerprint variants through it, so direct MCP bulk tool calls are bounded too. Per-path self-consistency is preserved (each surface hashes the same source string it previously used raw). Added AgentOperationFingerprintTests (Domain) covering bound, determinism, and differentiation. Full suite: 484 + 2571 + 1368 green.

[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]

/pr-review — PR #288 (fix/agent-fingerprint-overflow)

Scope reviewed: full diff vs main — AgentOperationFingerprint.cs (new shared helper), AgentOperationExecutor.cs, WebApplicationExtensions.cs (MCP middleware), plus new/updated tests. Build/test skipped per CI adaptations (separate required checks cover that); cross-repo orbit-ui-mobile/packages/shared dimensions not verifiable in this job (sibling repo not checked out) — not applicable anyway, since no DTO/route/contract surface is touched.

Follow-up on the prior review

My previous review (on commit c8fc252) requested changes for one Critical: the MCP middleware's TryGetMcpToolCall built a raw, unhashed {toolName}:{rawJson} fingerprint that could still overflow varchar(256) for bulk MCP tool calls, leaving the reported crash reproducible on that surface.

Commit a038653 fixes this correctly: fingerprint construction is now centralized in Orbit.Domain.Common.AgentOperationFingerprint.Compute() (SHA-256 hex, fixed 64 chars), and both call sites — AgentOperationExecutor.EvaluatePolicy and WebApplicationExtensions.TryGetMcpToolCall (both branches: execute_agent_operation_v2 and legacy-named tools) — now route through it. A repo-wide grep for fingerprint/OperationFingerprint-construction sites turns up no remaining unhashed builder. The originally-reported SqlState 22001 overflow is fixed everywhere it could occur.

Findings

Medium (non-blocking, pre-existing, out of scope for this PR) — MCP middleware pre-check never threads confirmationToken, independent of this fix

While tracing the "matching is unaffected" claim end-to-end, I found that WebApplicationExtensions.cs:174-183 builds AgentPolicyEvaluationContext without a ConfirmationToken argument, so AgentPolicyEvaluator.HasFreshConfirmation always sees null there and the middleware's own pre-check always returns ConfirmationRequired for FreshConfirmation-gated legacy MCP tools (e.g. bulk_create_habits, bulk_delete_habits, delete_habit) — even on a retry that correctly carries a valid confirmationToken in arguments, since AgentOperationExecutor's token-aware evaluation is never reached (the middleware returns before calling next()).

I confirmed via git diff aecc8a8 9b5d565 -- src/Orbit.Api/Extensions/WebApplicationExtensions.cs that this PR's only change to that file is the two fingerprint lines — the AgentPolicyEvaluationContext construction is byte-for-byte identical to main. So this is a real, separate bug, but it predates this PR, isn't worsened by it, and isn't what this PR set out to fix (the varchar overflow crash). Not blocking this merge — worth a fast, separate follow-up to thread confirmationToken through the middleware's pre-check for legacy-named MCP tools.

Low/Info

• WebApplicationExtensions.cs:370-372 computes a fingerprint hash for the execute_agent_operation_v2 branch that's immediately discarded (HandleMcpToolCallAsync short-circuits to next() for that tool name before reading OperationFingerprint). Pre-existing structurally, touched by this diff's rewrite — trivial cleanup opportunity, not blocking.
• No direct test exercises TryGetMcpToolCall/the MCP middleware itself (new tests cover the shared helper and AgentOperationExecutor in isolation, which is the right unit boundary given the middleware function is private static with no test seam). Worth folding into the same follow-up above if that gap gets addressed.

Other dimensions

• Security: PASS — un-keyed SHA-256 is appropriate for a dedupe/match key (not a bearer credential); the actual secret (ConfirmationToken) is a separate CSPRNG value checked independently. No new logging, no auth-path changes.
• Tests: new AgentOperationFingerprintTests (fixed length, determinism, distinctness) and the AgentOperationExecutor regression test are well-targeted and cover the actual overflow bug.
• Comment/dead-code/no-workaround policy: compliant — the new XML-doc documents contract intent, not narration.
• Contract parity: N/A — no DTO/route/packages/shared surface touched.

Recommendation

Approve. The fix is now correct and complete for its stated scope — the reported crash is fixed on every fingerprint-construction path, verified by grep and by diffing against main. The confirmation-token-threading gap found during verification is real but pre-existing and out of scope; recommend tracking it as a fast follow-up rather than blocking this fix.