Skip to content

fix: reflect habit operation reversibility in previews - #701

Merged
thomasluizon merged 4 commits into
redesign/mainfrom
fix/ticket-1216-operation-reversibility
Oct 5, 2026
Merged

thomasluizon merged 4 commits into
redesign/mainfrom
fix/ticket-1216-operation-reversibility

Conversation

@thomasluizon

@thomasluizon thomasluizon commented Oct 5, 2026 •

Copy link
Copy Markdown
Owner

Bulk habit creation, logging, skipping, updating and rescheduling inherited a destructive classification, so their approval previews could imply data deletion. These operations now use low risk in the catalog. Habit deletion keeps its destructive classification and fresh confirmation requirement. The existing chat write hold still requires approval for every applicable Astra write.

AgentCatalogService.Capabilities.cs corrects the shared bulk write capability, which supplies both chat and MCP operation metadata. AgentContracts.cs appends optional nullable removesData to PendingOperationItem. PendingOperationChangePreviewer.cs and AgentArgumentPreview.cs report false for the scoped habit writes and true for habit deletes. DestructiveOperationPreviewer.cs also marks its existing deletion rows, while calendar settings and sync actions report false. The generated src/Orbit.Api/openapi.json adds only this field.

The signal belongs to each preview item so a batch can distinguish deletions without deriving them from internal risk classes. Existing response fields retain their names and types. Unit tests cover catalog metadata, actual executor approval responses, mixed preview items, legacy deserialization and the generated optional contract. Policy and pending-store tests distinguish static catalog confirmation from the chat approval hold.

Closes thomasluizon/orbit-tickets#1216

Deploy the API before the consuming UI ticket updates its shared Zod schema and presentation.

Assumptions

  • The optional field is removesData on each PendingOperationItem, rather than a batch-level flag; it is nullable to preserve an unspecified value for unrelated generic previews instead of guessing from their risk class.
  • A mixed batch uses the existing collection of pending operations, rather than introducing a mixed-operation endpoint; the regression combines real create, log, skip and delete previews and checks exactly which items remove data.
  • Kept the existing boolean display strings (True and False) rather than changing existing fields to lowercase.
  • Reused the preview loader with a removal flag rather than duplicating the suggestion loader.

Test evidence

Before implementation, unchanged catalog and preview tests passed with the defect present:

env -u LANG LC_ALL=en_US.UTF-8 dotnet test --filter 'FullyQualifiedName~AgentCatalogServiceTests|FullyQualifiedName~PendingOperationChangePreviewerTests' --verbosity quiet

Observed: 14 infrastructure and 21 application tests passed. Unchanged approval, destructive-preview and argument-preview tests also passed with:

env -u LANG LC_ALL=en_US.UTF-8 dotnet test --no-build --filter 'FullyQualifiedName~AgentChatWriteHoldTests|FullyQualifiedName~DestructiveOperationPreviewerTests|FullyQualifiedName~AgentArgumentPreviewTests' --verbosity quiet

Observed: 54 infrastructure and 14 application tests passed.
Before fixing the catalog, these strengthened tests failed:

env -u LANG LC_ALL=en_US.UTF-8 dotnet test tests/Orbit.Infrastructure.Tests --filter 'FullyQualifiedName~AgentCatalogServiceTests|FullyQualifiedName~AgentChatWriteHoldTests' --verbosity quiet

Observed: 10 failures. HabitOperations_ClassifyReversibility and ExecuteAsync_EveryChatWrite_WaitsForApproval found destructive risk where low risk was expected for each of the five bulk writes. After the catalog fix, the same command with --no-build passed all 73 tests.
Before adding the preview field:

env -u LANG LC_ALL=en_US.UTF-8 dotnet test tests/Orbit.Application.Tests --filter 'FullyQualifiedName~PendingOperationChangePreviewerTests' --verbosity quiet

Observed: seven failures, including strengthened PreviewAsync_Create_UsesStableInputIndexes and PreviewAsync_BulkDelete_ReturnsTargetPreview, because serialized items lacked removesData. After the fix, all 26 preview tests passed within the 139-test focused preview, contract, revision, catalog and executor run. The updated policy, pending-store and approval tests then passed all 116 focused cases.
The first broader run exposed four stale tests that expected bulk writes to remain statically destructive or confirmation-gated. Their replacement assertions retain deletion gates and verify that chat approval preserves low risk and existing action keys.
Final full solution checks:

env -u LANG dotnet build Orbit.slnx --verbosity quiet
env -u LANG dotnet test --verbosity quiet
env -u LANG LC_ALL=en_US.UTF-8 dotnet build Orbit.slnx --verbosity quiet
env -u LANG LC_ALL=en_US.UTF-8 dotnet test --verbosity quiet

Both builds completed with zero errors. Both full test runs passed 9,375 tests: 32 analyzer, 661 domain, 5,426 application and 3,256 infrastructure tests. Existing dependency and obsolete API warnings remain. All three commits passed the repository pre-commit hooks; the working tree is clean.

  • env -u LANG dotnet test tests/Orbit.Infrastructure.Tests --filter FullyQualifiedName~DestructiveOperationPreviewerTests: the 2 existing tests passed with the defect present; with the new tests and no fix, 5 passed and 1 failed (DismissCalendarSuggestion_PreviewsOwnedSuggestionWithoutRemovingDataAndDetectsStateChange, RemovesData was true); after the fix all 6 passed.
  • env -u LANG dotnet build Orbit.slnx and env LC_ALL=en_US.UTF-8 dotnet build Orbit.slnx: exit 0, zero errors.
  • Infrastructure tests under both locale settings: exit 0, 3,260 passed each; a coverage run recorded hits on all eight lines SonarCloud flagged.
  • env -u LANG dotnet test and env LC_ALL=en_US.UTF-8 dotnet test: exit 0, 9,379 passed each, zero failures or skips.

Manual steps

None.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes Reviewed the habit reversibility metadata, preview contract, approval paths, and regression tests across all three commits, including compatibility with the consuming client.

  • Risk classification: Bulk create, log, skip, update, and reschedule now use low risk; habit deletion retains destructive classification and fresh confirmation.
  • Preview contract: Optional nullable removesData is appended to preview items and OpenAPI, with explicit flags for the scoped habit writes and deletion previews.
  • Approval preservation: Chat write holds still require fresh confirmation independently of catalog risk, including pending execution and clarification resolution.
  • Regression coverage: Tests assert exact preview flags, legacy compatibility, catalog metadata, action keys, and actual executor approval responses. The focused suite passed 183 tests; the full suite passed all 9,375 tests, and the solution build completed with zero errors.
  • Consumer compatibility: Checked orbit-ui-mobile on both main and redesign/main. Existing schemas accept the changed risk value and approval controls remain available; consumption of the additive item flag can follow the API-first rollout described in the PR.

Pullfrog  | View workflow run | Using gpt-6.1-sol | 𝕏

@sonarqubecloud

sonarqubecloud Bot commented Oct 5, 2026

Copy link
Copy Markdown

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes Reviewed the calendar preview changes in b6d2f969 since the prior review at c265a122, with the full PR diff as context.

  • Corrected dismissal metadata: Marked calendar suggestion dismissal as non-removing while retaining the shared loader's deletion defaults, ownership filtering, and state fingerprints; verified that execution retains the suggestion and sets its dismissal timestamp.
  • Added calendar regression coverage: Tested owned versus foreign suggestions, fingerprint changes, auto-sync current and proposed values, run-sync action fields, and unknown-action handling.
  • Verified regressions: Passed all 68 focused preview, contract, and chat approval tests and all 9,379 tests in the full suite. The solution build succeeded with zero errors after restoring missing project assets; existing dependency warnings remain.

Pullfrog  | View workflow run | Using gpt-6.1-sol | 𝕏

@thomasluizon
thomasluizon merged commit f44cf27 into redesign/main Oct 5, 2026
26 checks passed
@thomasluizon
thomasluizon deleted the fix/ticket-1216-operation-reversibility branch October 5, 2026 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant