Add contract and task-test workflows with tasks #8
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduces new contract approval and document subflow workflows, including associated tasks and C# mapping scripts. Adds test workflows and tasks for Dapr, HTTP, and workflow communication. Updates account opening workflow state types and minor config fixes. Removes obsolete OAuth workflow scripts and adds .gitignore entry for ai-docs/.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 23400359 | Triggered | Username Password | bce9141 | core/Workflows/oauth/oauth-authentication-workflow.http | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Reviewer's GuideIntroduces new contract approval and task-test workflows (with supporting tasks, rules, and C# mapping scripts), wires them to Mockoon/Dapr for integration-style tests, adjusts existing account-opening/payment workflows, and removes obsolete OAuth workflow artifacts while tightening repo ignores. Sequence diagram for contract approval workflow and document subflow interactionsequenceDiagram
participant Client as Client
participant WorkflowEngine as WorkflowEngine
participant ContractApprovalWorkflow as ContractApprovalWorkflow
participant ContractDocumentSubflow as ContractDocumentSubflow
participant MockoonApi as MockoonApi
Client->>WorkflowEngine: Start contract_approval_workflow (groupCode)
WorkflowEngine->>ContractApprovalWorkflow: Initialize instance
ContractApprovalWorkflow->>InitContractApprovalMapping: OutputHandler
InitContractApprovalMapping-->>ContractApprovalWorkflow: init-success (groupCode, counters)
ContractApprovalWorkflow->>GetContractDocumentsMapping: InputHandler
GetContractDocumentsMapping-->>ContractApprovalWorkflow: HTTP body, headers
ContractApprovalWorkflow->>MockoonApi: POST /contracts (groupCode)
MockoonApi-->>ContractApprovalWorkflow: contracts list, totalContracts
ContractApprovalWorkflow->>GetContractDocumentsMapping: OutputHandler
GetContractDocumentsMapping-->>ContractApprovalWorkflow: documents[], totalDocuments, currentDocumentIndex=0
loop For each document while HasMoreDocumentsRule==true
ContractApprovalWorkflow->>StartDocumentSubprocessMapping: InputHandler
StartDocumentSubprocessMapping-->>ContractApprovalWorkflow: subprocess key, body (document, indices)
ContractApprovalWorkflow->>ContractDocumentSubflow: Start subprocess (document, parent ids)
ContractDocumentSubflow-->>ContractApprovalWorkflow: subprocess-started via StartDocumentSubprocessMapping.OutputHandler
ContractApprovalWorkflow->>AllDocumentsReadyRule: Handler
AllDocumentsReadyRule-->>ContractApprovalWorkflow: currentDocumentIndex>=totalDocuments?
end
ContractApprovalWorkflow->>MoreApprovalsPendingRule: Handler
MoreApprovalsPendingRule-->>ContractApprovalWorkflow: approvedCount<totalDocuments?
alt AllApprovedRule==true
ContractApprovalWorkflow->>AllApprovedRule: Handler
AllApprovedRule-->>ContractApprovalWorkflow: true
ContractApprovalWorkflow-->>Client: Workflow completed (all documents approved)
else Pending approvals
ContractApprovalWorkflow-->>Client: Waiting for document approval transitions
end
Sequence diagram for contract document subflow rendering and parent notificationsequenceDiagram
participant ContractApprovalWorkflow as ContractApprovalWorkflow
participant ContractDocumentSubflow as ContractDocumentSubflow
participant MockoonApi as MockoonApi
participant NotifyReadyMapping as NotifyDocumentReadyMapping
participant NotifyApprovedMapping as NotifyDocumentApprovedMapping
participant NotifyRejectedMapping as NotifyDocumentRejectedMapping
ContractApprovalWorkflow->>ContractDocumentSubflow: Start subprocess (document, parent ids)
ContractDocumentSubflow->>InitDocumentMapping: OutputHandler
InitDocumentMapping-->>ContractDocumentSubflow: init-document-success (renderStatus=pending)
ContractDocumentSubflow->>RenderDocumentMapping: InputHandler
RenderDocumentMapping-->>ContractDocumentSubflow: HTTP body, headers
ContractDocumentSubflow->>MockoonApi: POST /render (document)
MockoonApi-->>ContractDocumentSubflow: render response
ContractDocumentSubflow->>RenderDocumentMapping: OutputHandler
RenderDocumentMapping-->>ContractDocumentSubflow: render-success or render-failed
alt RenderSuccessRule==true
ContractDocumentSubflow->>RenderSuccessRule: Handler
RenderSuccessRule-->>ContractDocumentSubflow: true
ContractDocumentSubflow->>NotifyReadyMapping: InputHandler
NotifyReadyMapping-->>ContractDocumentSubflow: configured DirectTriggerTask to parent
ContractDocumentSubflow->>ContractApprovalWorkflow: Direct transition document-ready
ContractApprovalWorkflow->>IncrementReadyCountMapping: OutputHandler
IncrementReadyCountMapping-->>ContractApprovalWorkflow: ready-count-incremented
else RenderFailedRule==true
ContractDocumentSubflow->>RenderFailedRule: Handler
RenderFailedRule-->>ContractDocumentSubflow: true
ContractDocumentSubflow-->>ContractApprovalWorkflow: Error path or compensation
end
opt User approves document in subprocess
ContractDocumentSubflow->>NotifyApprovedMapping: InputHandler
NotifyApprovedMapping-->>ContractDocumentSubflow: DirectTriggerTask configured
ContractDocumentSubflow->>ContractApprovalWorkflow: Direct transition document-approved
ContractApprovalWorkflow->>IncrementApprovedCountMapping: OutputHandler
IncrementApprovedCountMapping-->>ContractApprovalWorkflow: approved-count-incremented
end
opt User rejects document in subprocess
ContractDocumentSubflow->>NotifyRejectedMapping: InputHandler
NotifyRejectedMapping-->>ContractDocumentSubflow: DirectTriggerTask configured
ContractDocumentSubflow->>ContractApprovalWorkflow: Direct transition document-rejected
end
Class diagram for new contract workflow mappings and rulesclassDiagram
class ScriptContext
class WorkflowTask
class HttpTask
class SubProcessTask
class DirectTriggerTask
class ScriptResponse
class IMapping {
<<interface>>
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class IConditionMapping {
<<interface>>
+Task~bool~ Handler(ScriptContext context)
}
class InitContractApprovalMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class GetContractDocumentsMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class StartDocumentSubprocessMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class InitDocumentMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class RenderDocumentMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class NotifyDocumentReadyMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class NotifyDocumentApprovedMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class NotifyDocumentRejectedMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class IncrementReadyCountMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class IncrementApprovedCountMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class DocumentsLoadedRule {
+Task~bool~ Handler(ScriptContext context)
}
class NoDocumentsRule {
+Task~bool~ Handler(ScriptContext context)
}
class HasMoreDocumentsRule {
+Task~bool~ Handler(ScriptContext context)
}
class AllDocumentsReadyRule {
+Task~bool~ Handler(ScriptContext context)
}
class RenderSuccessRule {
+Task~bool~ Handler(ScriptContext context)
}
class RenderFailedRule {
+Task~bool~ Handler(ScriptContext context)
}
class MoreApprovalsPendingRule {
+Task~bool~ Handler(ScriptContext context)
}
class AllApprovedRule {
+Task~bool~ Handler(ScriptContext context)
}
class AlwaysTrueRule {
+Task~bool~ Handler(ScriptContext context)
}
IMapping <|.. InitContractApprovalMapping
IMapping <|.. GetContractDocumentsMapping
IMapping <|.. StartDocumentSubprocessMapping
IMapping <|.. InitDocumentMapping
IMapping <|.. RenderDocumentMapping
IMapping <|.. NotifyDocumentReadyMapping
IMapping <|.. NotifyDocumentApprovedMapping
IMapping <|.. NotifyDocumentRejectedMapping
IMapping <|.. IncrementReadyCountMapping
IMapping <|.. IncrementApprovedCountMapping
IConditionMapping <|.. DocumentsLoadedRule
IConditionMapping <|.. NoDocumentsRule
IConditionMapping <|.. HasMoreDocumentsRule
IConditionMapping <|.. AllDocumentsReadyRule
IConditionMapping <|.. RenderSuccessRule
IConditionMapping <|.. RenderFailedRule
IConditionMapping <|.. MoreApprovalsPendingRule
IConditionMapping <|.. AllApprovedRule
IConditionMapping <|.. AlwaysTrueRule
WorkflowTask <|-- HttpTask
WorkflowTask <|-- SubProcessTask
WorkflowTask <|-- DirectTriggerTask
InitContractApprovalMapping ..> ScriptContext
InitContractApprovalMapping ..> ScriptResponse
GetContractDocumentsMapping ..> HttpTask
GetContractDocumentsMapping ..> ScriptContext
StartDocumentSubprocessMapping ..> SubProcessTask
RenderDocumentMapping ..> HttpTask
NotifyDocumentReadyMapping ..> DirectTriggerTask
NotifyDocumentApprovedMapping ..> DirectTriggerTask
NotifyDocumentRejectedMapping ..> DirectTriggerTask
IncrementReadyCountMapping ..> ScriptContext
IncrementApprovedCountMapping ..> ScriptContext
DocumentsLoadedRule ..> ScriptContext
NoDocumentsRule ..> ScriptContext
HasMoreDocumentsRule ..> ScriptContext
AllDocumentsReadyRule ..> ScriptContext
RenderSuccessRule ..> ScriptContext
RenderFailedRule ..> ScriptContext
MoreApprovalsPendingRule ..> ScriptContext
AllApprovedRule ..> ScriptContext
AlwaysTrueRule ..> ScriptContext
Flow diagram for contract approval workflow decision rulesflowchart TD
Start["Start contract-approval-workflow"] --> Init["InitContractApprovalMapping OutputHandler\nset counters, groupCode"]
Init --> GetDocs["GetContractDocumentsMapping\nHTTP to Mockoon contracts API"]
GetDocs --> CheckLoaded{DocumentsLoadedRule}
CheckLoaded -- true --> HasDocs["totalDocuments > 0"]
CheckLoaded -- false --> NoDocs["NoDocumentsRule\ncomplete with no documents"]
HasDocs --> LoopCheck{HasMoreDocumentsRule}
LoopCheck -- true --> StartSub["StartDocumentSubprocessMapping\nstart contract-document-subflow"]
StartSub --> IncIndex["Update currentDocumentIndex"]
IncIndex --> LoopCheck
LoopCheck -- false --> ReadyCheck{AllDocumentsReadyRule}
ReadyCheck -- true --> ApprovalsPending{MoreApprovalsPendingRule}
ReadyCheck -- false --> ErrorPath["Unexpected state or wait"]
ApprovalsPending -- true --> WaitTransitions["Wait for document-approved or document-rejected transitions"]
WaitTransitions --> ApprovalsPending
ApprovalsPending -- false --> AllApproved{AllApprovedRule}
AllApproved -- true --> CompleteSuccess["All contracts approved\nworkflow success"]
AllApproved -- false --> MixedOutcome["Some rejected or failed"]
NoDocs --> EndNoDocs["End: no documents"]
ErrorPath --> EndError["End: error or manual intervention"]
CompleteSuccess --> EndSuccess["End: success"]
MixedOutcome --> EndMixed["End: mixed result"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. WalkthroughThis PR introduces a comprehensive contract approval workflow system featuring multilingual state machines, document processing subflows, and conditional rule-based transitions. It updates existing account-opening and payment workflows with state type modifications, adds extensive task test configurations across multiple task types (HTTP, Dapr, scripting), and updates mock API endpoints and test collections. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client
participant ContractApprovalWF as Contract Approval<br/>Workflow
participant DocStore as Document<br/>API
participant DocSubflow as Document<br/>Subflow
participant Approver as Approver<br/>System
User->>Client: Start contract approval
Client->>ContractApprovalWF: POST /start
ContractApprovalWF->>ContractApprovalWF: Initialize state,<br/>counters
ContractApprovalWF->>DocStore: GET documents
DocStore-->>ContractApprovalWF: Return documents[]
ContractApprovalWF->>ContractApprovalWF: Set totalDocuments,<br/>currentIndex=0
loop For each document
ContractApprovalWF->>DocSubflow: Start subprocess
Note over DocSubflow: Document Instance
DocSubflow->>DocStore: POST render-document
DocStore-->>DocSubflow: Return URL, metadata
DocSubflow->>DocSubflow: Update renderStatus
DocSubflow->>ContractApprovalWF: Notify document-ready
ContractApprovalWF->>ContractApprovalWF: Increment readyCount
Approver->>DocSubflow: Approve/Reject document
alt Document Approved
DocSubflow->>ContractApprovalWF: Notify document-approved
ContractApprovalWF->>ContractApprovalWF: Increment approvedCount
DocSubflow->>DocSubflow: End (completed)
else Document Rejected
DocSubflow->>ContractApprovalWF: Notify document-rejected
ContractApprovalWF->>ContractApprovalWF: Increment rejectedCount
DocSubflow->>DocSubflow: End (rejected)
end
ContractApprovalWF->>ContractApprovalWF: Increment currentIndex
end
alt All approved
ContractApprovalWF->>ContractApprovalWF: State → contract-completed
else Some rejected
ContractApprovalWF->>ContractApprovalWF: State → contract-rejected
end
ContractApprovalWF-->>Client: Return final state
Client-->>User: Show result
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Possibly Related PRs
Suggested Reviewers
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (72)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @yilmaztayfun, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the system's workflow capabilities by introducing a new contract approval process and a robust testing framework for various task types. The contract workflow streamlines document management, while the task-test workflow provides valuable examples and validation for integration with Dapr, HTTP services, and inter-workflow communication. These additions enhance both business process automation and the maintainability of the workflow system. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Several mappings (e.g. GetContractDocumentsMapping, StartDocumentSubprocessMapping, IncrementApprovedCountMapping, etc.) cast dynamic values with
(int)(context.Instance?.Data?... ?? 0), which will throw if the underlying value is not exactly an int; consider usingConvert.ToInt32with null/format checks or pattern matching to make these conversions more robust. - Some condition rules and mappings swallow exceptions by returning a default boolean (e.g. AllApprovedRule returns true on error, NoDocumentsRule returns true on error); you may want to either log these failures or choose a safer default (typically false) so that unexpected data does not silently drive transitions.
- A few handlers are declared
asyncbut neverawait(e.g. manyOutputHandlerimplementations), and there is duplicated HTTP/Dapr setup logic between mappings (HttpTaskMapping vs HttpTaskItemsMapping, several DirectTriggerTask usages); consider either removingasyncwhere unnecessary or extracting shared helpers to reduce duplication and make the behavior easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several mappings (e.g. GetContractDocumentsMapping, StartDocumentSubprocessMapping, IncrementApprovedCountMapping, etc.) cast dynamic values with `(int)(context.Instance?.Data?... ?? 0)`, which will throw if the underlying value is not exactly an int; consider using `Convert.ToInt32` with null/format checks or pattern matching to make these conversions more robust.
- Some condition rules and mappings swallow exceptions by returning a default boolean (e.g. AllApprovedRule returns true on error, NoDocumentsRule returns true on error); you may want to either log these failures or choose a safer default (typically false) so that unexpected data does not silently drive transitions.
- A few handlers are declared `async` but never `await` (e.g. many `OutputHandler` implementations), and there is duplicated HTTP/Dapr setup logic between mappings (HttpTaskMapping vs HttpTaskItemsMapping, several DirectTriggerTask usages); consider either removing `async` where unnecessary or extracting shared helpers to reduce duplication and make the behavior easier to maintain.
## Individual Comments
### Comment 1
<location> `core/Workflows/contract/src/GetContractDocumentsMapping.csx:63-72` </location>
<code_context>
+ var contracts = response?.data?.data?.contracts ?? new object[] { };
</code_context>
<issue_to_address>
**issue (bug_risk):** Casting totalContracts to int directly can throw at runtime if the API returns a non-int numeric type.
Given `totalContracts` comes from `response?.data?.data?.totalContracts`, its runtime type depends on the Mockoon response. If it’s `long`, `double`, or a string, `(int)totalContracts` will raise `InvalidCastException` and hit the exception handler. Please switch to a safer approach (e.g., `Convert.ToInt32`, pattern matching, or a type check) and apply the same hardening to similar casts in the rules (e.g., `DocumentsLoadedRule`, `AllApprovedRule`).
</issue_to_address>
### Comment 2
<location> `core/Workflows/contract/src/DocumentsLoadedRule.csx:13-14` </location>
<code_context>
+ {
+ try
+ {
+ var totalDocuments = context.Instance?.Data?.totalDocuments ?? 0;
+ return (int)totalDocuments > 0;
+ }
+ catch (Exception)
</code_context>
<issue_to_address>
**issue (bug_risk):** Direct cast of totalDocuments to int in a dynamic context may cause exceptions and force the rule’s fallback behavior.
Since `context.Instance?.Data` is effectively dynamic, `totalDocuments` may not actually be an `int` at runtime (it could be `long`, `string`, etc.). The direct `(int)totalDocuments` cast would then throw and force the `catch` to return `false`. To avoid this unintended fallback, consider normalizing the value with something like `Convert.ToInt32` (and handling invalid formats) instead of a direct cast.
</issue_to_address>
### Comment 3
<location> `core/Workflows/contract/src/AllApprovedRule.csx:11-22` </location>
<code_context>
+ {
+ try
+ {
+ var approvedCount = (int)(context.Instance?.Data?.approvedCount ?? 0);
+ var totalDocuments = (int)(context.Instance?.Data?.totalDocuments ?? 0);
+
+ // All approved when approved count equals total documents
+ return approvedCount >= totalDocuments;
+ }
+ catch (Exception)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The rule returns true on any exception, which may prematurely signal that all documents are approved.
Because the `catch` block returns `true`, any data or casting issue (e.g., when reading `approvedCount` / `totalDocuments`) will incorrectly treat the state as "all approved". For a critical transition, consider a safer default (e.g., `false`) or explicitly distinguishing data errors from the valid "all approved" case.
```suggestion
try
{
var approvedCount = (int)(context.Instance?.Data?.approvedCount ?? 0);
var totalDocuments = (int)(context.Instance?.Data?.totalDocuments ?? 0);
// All approved when approved count equals total documents
// NOTE: If there's any data/casting issue, we fall back to "not all approved" in the catch block.
return approvedCount >= totalDocuments;
}
catch (Exception)
{
// Safer default: on any data or casting error, do NOT treat as "all approved"
return false;
}
```
</issue_to_address>
### Comment 4
<location> `core/Workflows/task-test/src/GetConfigValueMapping.csx:62` </location>
<code_context>
+ Key = "get-config-success",
+ Data = new
+ {
+ getConfigResult = new
+ {
+ success = true,
</code_context>
<issue_to_address>
**issue (testing):** Config test output does not reflect whether the config value was actually retrieved
Here `success` is always set to `true`, even though the input handler derives a `configRetrieved` flag from `apiBaseUrl`. This means tests will pass even when the config is missing or invalid. Consider propagating `configRetrieved` into the instance data (or tags) and using it to set `success` and choose the response key (`get-config-success` vs `get-config-failed`). Optionally expose `configRetrieved` in the output payload so callers can assert on it directly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+63
to
+72
| var contracts = response?.data?.data?.contracts ?? new object[] { }; | ||
| var contractList = new List<object>(); | ||
|
|
||
| if (contracts != null) | ||
| { | ||
| foreach (var contract in contracts) | ||
| { | ||
| contractList.Add(contract); | ||
| } | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): Casting totalContracts to int directly can throw at runtime if the API returns a non-int numeric type.
Given totalContracts comes from response?.data?.data?.totalContracts, its runtime type depends on the Mockoon response. If it’s long, double, or a string, (int)totalContracts will raise InvalidCastException and hit the exception handler. Please switch to a safer approach (e.g., Convert.ToInt32, pattern matching, or a type check) and apply the same hardening to similar casts in the rules (e.g., DocumentsLoadedRule, AllApprovedRule).
Comment on lines
+13
to
+14
| var totalDocuments = context.Instance?.Data?.totalDocuments ?? 0; | ||
| return (int)totalDocuments > 0; |
There was a problem hiding this comment.
issue (bug_risk): Direct cast of totalDocuments to int in a dynamic context may cause exceptions and force the rule’s fallback behavior.
Since context.Instance?.Data is effectively dynamic, totalDocuments may not actually be an int at runtime (it could be long, string, etc.). The direct (int)totalDocuments cast would then throw and force the catch to return false. To avoid this unintended fallback, consider normalizing the value with something like Convert.ToInt32 (and handling invalid formats) instead of a direct cast.
Comment on lines
+11
to
+22
| try | ||
| { | ||
| var approvedCount = (int)(context.Instance?.Data?.approvedCount ?? 0); | ||
| var totalDocuments = (int)(context.Instance?.Data?.totalDocuments ?? 0); | ||
|
|
||
| // All approved when approved count equals total documents | ||
| return approvedCount >= totalDocuments; | ||
| } | ||
| catch (Exception) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): The rule returns true on any exception, which may prematurely signal that all documents are approved.
Because the catch block returns true, any data or casting issue (e.g., when reading approvedCount / totalDocuments) will incorrectly treat the state as "all approved". For a critical transition, consider a safer default (e.g., false) or explicitly distinguishing data errors from the valid "all approved" case.
Suggested change
| try | |
| { | |
| var approvedCount = (int)(context.Instance?.Data?.approvedCount ?? 0); | |
| var totalDocuments = (int)(context.Instance?.Data?.totalDocuments ?? 0); | |
| // All approved when approved count equals total documents | |
| return approvedCount >= totalDocuments; | |
| } | |
| catch (Exception) | |
| { | |
| return true; | |
| } | |
| try | |
| { | |
| var approvedCount = (int)(context.Instance?.Data?.approvedCount ?? 0); | |
| var totalDocuments = (int)(context.Instance?.Data?.totalDocuments ?? 0); | |
| // All approved when approved count equals total documents | |
| // NOTE: If there's any data/casting issue, we fall back to "not all approved" in the catch block. | |
| return approvedCount >= totalDocuments; | |
| } | |
| catch (Exception) | |
| { | |
| // Safer default: on any data or casting error, do NOT treat as "all approved" | |
| return false; | |
| } |
| Key = "get-config-success", | ||
| Data = new | ||
| { | ||
| getConfigResult = new |
There was a problem hiding this comment.
issue (testing): Config test output does not reflect whether the config value was actually retrieved
Here success is always set to true, even though the input handler derives a configRetrieved flag from apiBaseUrl. This means tests will pass even when the config is missing or invalid. Consider propagating configRetrieved into the instance data (or tags) and using it to set success and choose the response key (get-config-success vs get-config-failed). Optionally expose configRetrieved in the output payload so callers can assert on it directly.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality with contract approval and task-testing workflows. The changes are extensive and well-structured. My review focuses on improving configuration management, enhancing code clarity and maintainability, and fixing a few potential bugs related to state management, error handling, and script encoding. I've identified several instances of hardcoded configuration values that should be externalized, a critical issue with an incorrectly encoded C# script, and some unsafe error handling in rule scripts. Addressing these points will improve the robustness and flexibility of the new workflows.
Comment on lines
+590
to
+594
| "mapping": { | ||
| "location": "./src/StartWorkflowMapping.csx", | ||
| "code": "using System;\nusing System.Threading.Tasks;\nusing BBT.Workflow.Definitions;\nusing BBT.Workflow.Scripting;\n\n/// <summary>\n/// Start Workflow Task Mapping - Starts a new workflow instance\n/// Test case: Verify starting new workflow instances\n/// </summary>\npublic class StartWorkflowMapping : IMapping\n{\n public Task<ScriptResponse> InputHandler(WorkflowTask task, ScriptContext context)\n {\n try\n {\n var startTask = task as StartTask;\n if (startTask == null)\n {\n throw new InvalidOperationException(\"Task must be a StartTask\");\n }\n\n // Configure target workflow\n startTask.SetDomain(\"core\");\n startTask.SetFlow(\"task-test-subflow\");\n startTask.SetSync(true);\n startTask.SetVersion(\"1.0.0\");\n startTask.SetKey(Guid.NewGuid().ToString());\n startTask.SetTags(new[] { \"task-test\", \"start-workflow\", \"success\" });\n \n // Prepare initialization body\n var initBody = new\n {\n parentInstanceId = context.Instance?.Id,\n parentWorkflowId = context.Workflow?.Key,\n testId = context.Instance?.Data?.testId ?? Guid.NewGuid().ToString(),\n startedAt = DateTime.UtcNow,\n startedBy = \"task-test-workflow\",\n message = \"StartTask test - new instance created\"\n };\n startTask.SetBody(initBody);\n\n return Task.FromResult(new ScriptResponse());\n }\n catch (Exception ex)\n {\n return Task.FromResult(new ScriptResponse\n {\n Key = \"start-workflow-input-error\",\n Data = new { error = ex.Message }\n });\n }\n }\n\n public async Task<ScriptResponse> OutputHandler(ScriptContext context)\n {\n try\n {\n var response = context.Body;\n\n if (response?.isSuccess == true)\n {\n return new ScriptResponse\n {\n Key = \"start-workflow-success\",\n Data = new\n {\n startWorkflowResult = new\n {\n success = true,\n newInstanceId = response?.data?.id,\n status = response?.data?.status,\n startedAt = DateTime.UtcNow,\n taskType = \"Start\"\n },\n // Store instance ID for later use\n subflowInstanceId = response?.data?.id\n },\n Tags = new[] { \"task-test\", \"start-workflow\", \"success\" }\n };\n }\n\n return new ScriptResponse\n {\n Key = \"start-workflow-failed\",\n Data = new\n {\n startWorkflowResult = new\n {\n success = false,\n error = response?.errorMessage ?? \"Failed to start workflow\",\n failedAt = DateTime.UtcNow,\n taskType = \"Start\"\n }\n },\n Tags = new[] { \"task-test\", \"start-workflow\", \"failed\" }\n };\n }\n catch (Exception ex)\n {\n return new ScriptResponse\n {\n Key = \"start-workflow-exception\",\n Data = new\n {\n startWorkflowResult = new\n {\n success = false,\n error = ex.Message,\n taskType = \"Start\"\n }\n }\n };\n }\n }\n}", | ||
| "encoding": "NAT" | ||
| } |
There was a problem hiding this comment.
The code for StartWorkflowMapping.csx is not Base64 encoded, and an encoding: "NAT" property has been added. This is inconsistent with all other script mappings in the project and will cause a parsing error at runtime. The C# code must be Base64 encoded and assigned to the code property, and the encoding property should be removed.
Comment on lines
+19
to
+22
| catch (Exception) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The catch block swallows any exception and returns true. This is dangerous because it can cause the workflow to proceed incorrectly if there's an issue reading the instance data (e.g., a typo in a property name). The rule should fail safe by returning false.
catch (Exception)
{
return false;
}
| { | ||
| "key": "account-details-input", | ||
| "stateType": 1, | ||
| "stateType": 2, |
There was a problem hiding this comment.
The stateType for account-details-input has been changed to 2 (System), but this state has a view defined and requires user interaction. A state with user interaction should have stateType: 1 (Interaction). This change will likely prevent the UI from being displayed for this step.
Suggested change
| "stateType": 2, | |
| "stateType": 1, |
| { | ||
| "key": "account-confirmation", | ||
| "stateType": 1, | ||
| "stateType": 2, |
There was a problem hiding this comment.
The stateType for account-confirmation has been changed to 2 (System). Since this state has a view and requires user confirmation, it should be an Interaction state (stateType: 1).
Suggested change
| "stateType": 2, | |
| "stateType": 1, |
Comment on lines
+19
to
+22
| catch (Exception) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The catch block returns true on any exception. This could cause the workflow to prematurely move to the next state, assuming all documents are ready when they might not be. It should return false to be safe.
catch (Exception)
{
return false;
}
| { | ||
| "order": 1, | ||
| "task": { | ||
| "key": "notify-document-approved", |
There was a problem hiding this comment.
The task with key notify-document-approved is being reused in the notify-parent-rejected state. While the mapping script correctly sets the transition name to document-rejected, reusing a task with a name indicating approval for a rejection action is confusing and harms maintainability. A dedicated task definition for rejection, e.g., notify-document-rejected, should be created for clarity.
| "attributes": { | ||
| "type": "6", | ||
| "config": { | ||
| "url": "http://localhost:3001/api/contract/get-documents", |
There was a problem hiding this comment.
The URL is hardcoded to localhost. This is not suitable for different environments (development, staging, production). It's better to retrieve this URL from a configuration source to allow for environment-specific endpoints.
| var startTask = task as SubProcessTask; | ||
| if (startTask == null) | ||
| { | ||
| throw new InvalidOperationException("Task must be a StartTask"); |
There was a problem hiding this comment.
The exception message is incorrect. The code checks for SubProcessTask but the error message refers to StartTask. This can be misleading during debugging.
throw new InvalidOperationException("Task must be a SubProcessTask");
Comment on lines
+1
to
+119
| using System; | ||
| using System.Threading.Tasks; | ||
| using BBT.Workflow.Definitions; | ||
| using BBT.Workflow.Scripting; | ||
|
|
||
| /// <summary> | ||
| /// HTTP Task Mapping - Makes external API calls | ||
| /// Test case: Verify HTTP requests to external services via Mockoon | ||
| /// </summary> | ||
| public class HttpTaskItemsMapping : IMapping | ||
| { | ||
| public Task<ScriptResponse> InputHandler(WorkflowTask task, ScriptContext context) | ||
| { | ||
| try | ||
| { | ||
| var httpTask = task as HttpTask; | ||
| if (httpTask == null) | ||
| { | ||
| throw new InvalidOperationException("Task must be an HttpTask"); | ||
| } | ||
|
|
||
| // Prepare request body | ||
| var requestBody = new | ||
| { | ||
| testId = context.Instance?.Data?.testId ?? Guid.NewGuid().ToString(), | ||
| workflowId = context.Workflow.Key, | ||
| instanceId = context.Instance?.Id, | ||
| timestamp = DateTime.UtcNow, | ||
| action = "http-task-test" | ||
| }; | ||
|
|
||
| httpTask.SetBody(requestBody); | ||
|
|
||
| // Set headers | ||
| var headers = new Dictionary<string, string?> | ||
| { | ||
| ["Content-Type"] = "application/json", | ||
| ["X-Test-Id"] = context.Instance?.Data?.testId?.ToString(), | ||
| ["X-Request-Id"] = Guid.NewGuid().ToString(), | ||
| ["X-Correlation-Id"] = context.Instance.Id.ToString() | ||
| }; | ||
| httpTask.SetHeaders(headers); | ||
|
|
||
| return Task.FromResult(new ScriptResponse()); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| return Task.FromResult(new ScriptResponse | ||
| { | ||
| Key = "http-task-input-error", | ||
| Data = new { error = ex.Message } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| public async Task<ScriptResponse> OutputHandler(ScriptContext context) | ||
| { | ||
| try | ||
| { | ||
| var response = context.Body; | ||
| var statusCode = response?.statusCode ?? 500; | ||
|
|
||
| if (statusCode >= 200 && statusCode < 300) | ||
| { | ||
| return new ScriptResponse | ||
| { | ||
| Key = "http-task-success", | ||
| Data = new | ||
| { | ||
| httpTaskItemsResult = new | ||
| { | ||
| success = true, | ||
| statusCode = statusCode, | ||
| data = response?.data, | ||
| executionTime = response?.executionDurationMs, | ||
| processedAt = DateTime.UtcNow, | ||
| taskType = "HttpTask" | ||
| } | ||
| }, | ||
| Tags = new[] { "task-test", "http-task", "success" } | ||
| }; | ||
| } | ||
|
|
||
| return new ScriptResponse | ||
| { | ||
| Key = "http-task-failed", | ||
| Data = new | ||
| { | ||
| httpTaskItemsResult = new | ||
| { | ||
| success = false, | ||
| statusCode = statusCode, | ||
| error = response?.errorMessage ?? "HTTP request failed", | ||
| failedAt = DateTime.UtcNow, | ||
| taskType = "HttpTask" | ||
| } | ||
| }, | ||
| Tags = new[] { "task-test", "http-task", "failed" } | ||
| }; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| return new ScriptResponse | ||
| { | ||
| Key = "http-task-exception", | ||
| Data = new | ||
| { | ||
| httpTaskItemsResult = new | ||
| { | ||
| success = false, | ||
| error = ex.Message, | ||
| taskType = "HttpTask" | ||
| } | ||
| } | ||
| }; | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This mapping script is almost identical to HttpTaskMapping.csx. The only significant difference is the key used in the output data (httpTaskItemsResult vs httpTaskResult). This code duplication makes maintenance harder. Consider creating a single, more generic mapping script that can be used for both HTTP tasks.
| "attributes": { | ||
| "type": "6", | ||
| "config": { | ||
| "url": "http://localhost:3001/api/contract/render-document", |
There was a problem hiding this comment.
The URL is hardcoded to localhost. This should be made configurable to support different environments. Hardcoding URLs makes it difficult to deploy and test the application in different stages.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduces new contract approval and document subflow workflows, including associated tasks and C# mapping scripts. Adds test workflows and tasks for Dapr, HTTP, and workflow communication. Updates account opening workflow state types and minor config fixes. Removes obsolete OAuth workflow scripts and adds .gitignore entry for ai-docs/.
Summary by Sourcery
Introduce new contract approval and document subflow workflows alongside a comprehensive task-testing workflow suite, and clean up obsolete OAuth workflow assets.
New Features:
Enhancements:
Chores:
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.