Validate edits for all held Astra chat writes (#980)

[thomasluizon]

Held Astra writes now accept edits only after the parameter schema and the write tool's own argument check accept the complete revised arguments. Accepted edits remain in the hold and reach the ordinary execute parser; rejected edits return invalid_revision without replacing the held arguments.

The checks live in src/Orbit.Application/Chat/Tools/Implementations/. MediatR tools reuse their execute parser and send a read-only CheckChatCommandQuery, which runs the existing FluentValidation validators and checks referenced owners and domain guards. Habit command builders and the Habit and Goal guards are shared with execution. Account deletion checks inspect the challenge without consuming its code or recording failed attempts.

AgentArgumentSchema, PendingOperationRevisionService and the preview builders keep schema validation first, validate bulk item edits through the tool, preserve server preview IDs across repeated edits, and keep ignored action arguments fixed. Regression coverage lives in HeldWriteRevisionTests, HeldWriteArgumentCheckTests, their shared context, and the existing preview tests.

Client compatibility: the existing PendingOperationChange.IsEditable contract carries the edit control, as it already does for create_habit and update_habit. Consumed fields backed by the new checks now emit that flag. No client contract change is required. Browser verification was excluded by the work order.

Refs thomasluizon/orbit-tickets#980

Test evidence

• Existing tests with defects present: env -u LANG dotnet test tests/Orbit.Application.Tests --no-build --filter 'FullyQualifiedName~TagToolTests|FullyQualifiedName~ProfileNotificationCalendarToolTests|FullyQualifiedName~BulkCreateHabitsToolTests' passed all 77 unchanged tests.
• Stronger production-path tests: env -u LANG dotnet test tests/Orbit.Application.Tests --filter 'FullyQualifiedName~HeldWriteRevisionTests' initially failed five cases: the blank bulk-create child in InvalidValue_ReturnsInvalidRevisionAndPreservesHold, both DuplicateTagName_ReturnsInvalidRevision cases, DeleteSelectedNotifications_ChecksEveryOwnerWithoutDeleting, and the ignored suggestion argument in IgnoredArgument_RemainsFixed. These failures were observed before the fixes. The revised tests use real tool parsers, command validators, repository predicates and domain entities.
• env -u LANG dotnet test tests/Orbit.Application.Tests --filter 'FullyQualifiedName~HeldWriteRevisionTests.InvalidValue' additionally failed the empty goal unit case before its check stopped accepting the tool's fallback value.
• With the new tools' IArgumentCheckTool declarations temporarily removed and their exact source restored afterward, env -u LANG dotnet test tests/Orbit.Application.Tests --filter 'FullyQualifiedName~HeldWriteRevisionTests.ValidEdit_ExecutesStoredRevisedValue' failed all 35 cases. After restoration, the expanded tests accept edits, execute the stored values, and reject invalid revisions.
• The first full run caught two unchanged move-parent handler tests failing after the shared-check refactor. Execution now supplies its loaded habit and parent to the same guard used by argument checking.
• dotnet build Orbit.slnx: exit 0, zero errors.
• env -u LANG dotnet test tests/Orbit.Application.Tests --no-build --filter 'FullyQualifiedName~Chat': 1,100 passed. env -u LANG dotnet test tests/Orbit.Application.Tests --no-build --filter 'FullyQualifiedName~MoveHabitParentCommandHandlerTests|FullyQualifiedName~HeldWrite': 156 passed after the final refactor.
• env -u LANG dotnet test: exit 0, 8,453 passed across all four test projects.
• env -u LANG LC_ALL=en_US.UTF-8 dotnet test: exit 0, the same 8,453 passed.
• Architecture generation, formatting, dash, timeless, root allowlist, suppression allowlist and whitespace checks passed. Generated architecture maps remain ignored under the repository convention.
  The change introduces no HTTP response fields or external CLI response reads in production code. The installed MediatR and FluentValidation calls are exercised by the focused tests with the real argument-check handler and validators; dispatch returns the repository-owned Result.
• Baseline: dotnet test tests/Orbit.Application.Tests --filter FullyQualifiedName~HeldWriteRevisionTests passed all 123 existing tests with both defects present.
• Before fixes, the same command failed 10 new regression cases:
  • AccountDeletionRevision_SharesLockoutWithOrdinaryConfirmation: three cases accepted the correct code after exhausted attempts.
  • ChallengeCheck_WrongCodesExhaustConfirmationBudget: both operations failed to decrement attempts.
  • BulkObjectListEdit_ExecutesStoredRevisedValue: five supported edits returned invalid_revision.
• After fixes, dotnet test tests/Orbit.Application.Tests --filter 'FullyQualifiedName~HeldWriteRevisionTests|FullyQualifiedName~ConfirmAccountDeletionCommandHandlerTests|FullyQualifiedName~ApiKeyCreationChallengeFlowTests' passed all 164 tests. Malformed-entry and code-preservation guards passed before and after.
• env -u LANG dotnet test: exit 0, 8,474 passed.
• env LC_ALL=en_US.UTF-8 dotnet test: exit 0, 8,474 passed.
• Formatting, repository guards, and whitespace checks passed. Configured pre-commit checks were run directly because the hook was absent.

Assumptions

• Reused each MediatR tool's parser with a check-only dispatch rather than maintaining a second parser and validator set per tool.
• Preserved the existing fixed target IDs and action fields rather than allowing revisions to retarget a held operation; nested position lists still check every referenced owner.
  None.

Manual steps

None.
None beyond normal API deployment. No new configuration, secrets, migrations, backfills, or client changes.

[pullfrog]

Important

Two high-priority issues need correction: deletion-code revisions bypass the challenge attempt budget, and valid bulk checklist/reminder edits are rejected.

Reviewed changes Reviewed all changed files across the three commits, tracing revision validation, tool execution, ownership checks, and domain guards.

• Held-write validation: Tools now provide read-only argument checks using their execution parsers, existing validators, and entity guards.
• Preview and schema rules: Revisions validate bulk item schemas, retain server preview IDs, and restrict edits to consumed arguments.
• Shared domain behavior: Habit command builders and goal/habit guards are reused between checking and execution.
• Regression coverage: The focused revision/preview suite passes all 173 tests; isolated reproductions confirm both inline findings despite that coverage.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using openai/gpt-6.1-sol ｜ 𝕏}

[pullfrog]

No new issues found.

Reviewed changes Reviewed 36a43541 since the prior Pullfrog review, checked both resolved discussions against the current code, and read the full PR diff for context.

• Restored shared lockout: Incorrect revision code checks now consume the same failed-attempt budget as ordinary confirmation, while correct checks leave the code available for execution.
• Corrected nested schemas: Bulk checklist and scheduled-reminder edits now declare their supported fields without bypassing recursive schema validation.
• Added regression coverage: Tests assert mixed-path lockout, correct-code preservation, exact executed list values, and malformed-edit rejection. The focused held-write, preview, and challenge suite passed all 214 tests.

  ^{｜ View workflow run ｜ Using openai/gpt-6.1-sol ｜ 𝕏}