Skip to content

refine import/export of workflow configurations - #5523

Draft
abarz722 wants to merge 4 commits into
CactuseSecurity:developfrom
abarz722:abfixes4
Draft

abarz722 wants to merge 4 commits into
CactuseSecurity:developfrom
abarz722:abfixes4

Conversation

@abarz722

Copy link
Copy Markdown
Contributor

No description provided.

@abarz722 abarz722 self-assigned this Oct 18, 2026
@sonarqubecloud

Copy link
Copy Markdown

@abarz722

Copy link
Copy Markdown
Contributor Author

Findings

Number Criticality Confidence Status Subject
F1 🔴 critical 🟢 high 🔴 new Failed merges can leave an existing workflow configuration partially modified
F2 🟠 high 🟢 high 🔴 new Missing workflow states are recreated with fabricated semantics
F3 🔵 low 🟢 high 🔴 new The new transfer workflow is absent from help and What’s New content
F4 🔵 low 🟢 high 🔴 new Production method documentation is missing or stale

Recommendations

Fix in this PR

  • F1: Make merge-to-existing imports atomic, or explicitly reverse every added phase, derived-state mapping, and transition-group link when any later step fails; add failure-injection tests for each mutation boundary.
  • F2: Preserve and validate complete state definitions in the transfer format (including name and automatic_only), or retain the previous behavior of rejecting packages whose referenced states do not exist.
  • F3: Update HelpSettingsStateMatrix.cshtml and both language versions of whats_new_facts for selective export, custom filenames, and merge imports.
  • F4: Add the required XML summary for OnAfterRenderAsync and update Import so its summary covers both new-configuration and merge modes.

Create follow-up issue

  • None. Each finding was introduced by this feature and belongs within its stated scope.

Details

F1 — Failed merges can leave an existing workflow configuration partially modified

WorkflowConfigurationTransferService.cs:108-109 enters MergeIntoConfiguration, where each phase and each transition-group link is written through a separate GraphQL request (:197-208, :220-233, :248-275). If, for example, the derived-state upsert succeeds and a later link or phase insert fails, the catch block calls RollBack (:132-140), but that method only deletes newly created transition groups, visibility groups, and states (:625-665). It never removes phases, derived-state mappings, or links already added to the selected target configuration. The import therefore reports failure while leaving the existing configuration partly changed; newly created states referenced by those leftover rows can also make rollback fail with an aggregate exception. Use one transactional mutation or track and reverse every merge mutation, and test failures after an added phase, derived state, and earlier link.

F2 — Missing workflow states are recreated with fabricated semantics

CreateMissingStates inserts every referenced-but-absent ID with name = "Imported state {id}" and automaticOnly = false (WorkflowConfigurationTransferService.cs:279-290), but version 1 of the package does not carry either source attribute. This changes behavior, not just presentation: StateMatrix.cs:66 builds AutomaticOnlyStates from that flag, and StateMatrix.cs:98-110 uses it to suppress manual transitions. Importing a package whose source state was automatic-only can therefore expose that state as a manual transition on the target, while also losing its real name. Negative/reserved or otherwise invalid IDs are not rejected either. Transfer complete state definitions with validation (and a format-version update), or reject missing target states as before.

F3 — The new transfer workflow is absent from help and What’s New content

The UI now adds selectable export phases, a custom filename, and import into an existing configuration (TransferWorkflowConfiguration.razor:23-33, :60-85, :247-260), but the PR changes neither roles/ui/files/FWO.UI/Pages/Help/HelpSettingsStateMatrix.cshtml nor the German/English whats_new_facts entries in fworch-texts.sql. Repository policy requires both for a UI feature change.

F4 — Production method documentation is missing or stale

The new OnAfterRenderAsync override at TransferWorkflowConfiguration.razor:175 has no required /// summary. The existing summary at :245 now says Import always creates a new inactive configuration even though :257 can merge into an existing target. Bring both comments in line with the repository’s C# documentation rule and the implemented behavior.

Review summary

  • Round: 1 of max. 5.
  • Depth/capability: deep; fwo-review-pr two-pass review (correctness/quality plus security), followed by primary-model reconciliation and verification.
  • Scope: all 9 changed files at head 692e9a8dc63b5c6b9cc8bf5e87cd41f5b2638483 were inspected, together with the directly relevant state-matrix runtime code and changed tests. Security checks covered authorization reachability, GraphQL/input handling, stored XSS/output encoding, CSP, tenant isolation, secrets/logging, and rollback/data integrity.
  • Delegation: reduced-tier gpt-5.6-sol workers collected GitHub diff/history evidence and performed mechanical coding-guideline, localization/help, HTML/CSP, and test-presence scans. Security decisions, defect verification, ratings, numbering, and this comment were handled on the primary model. One earlier reduced-tier dispatch failed at capacity and returned no review data.
  • Validation: FWO.Services, FWO.Ui, and FWO.Test built successfully with .NET 10 and BuildProjectReferences=false; 72 targeted tests passed. git diff --check passed. A full solution/reference build could not be validated because the local MSBuild worker launcher failed without a diagnostic and then spawned runaway workers under serialized retry; those review-launched workers were stopped. This remains residual validation risk.
  • Token budget: usage could not be measured because this environment exposes no session usage/cost figure or attributable transcript counter (measurement method 4). The deep-review ceiling is 1M review tokens; proxy limits were enforced (4 dispatches, one failed without data; no repeated diff/history fetch; review limited to changed files and direct dependencies).
  • Model: OpenAI Codex, GPT-5 family. The exact serving version and primary reasoning setting are not exposed to the agent; reduced-tier scans used gpt-5.6-sol with low reasoning. No more specific model metadata is claimed.

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