Skip to content

fix: remove build time warning and add started process log#92

Open
Kronprinz03 wants to merge 7 commits intomainfrom
Remove-build-time-warning-and-improve-logs
Open

fix: remove build time warning and add started process log#92
Kronprinz03 wants to merge 7 commits intomainfrom
Remove-build-time-warning-and-improve-logs

Conversation

@Kronprinz03
Copy link
Copy Markdown
Contributor

@Kronprinz03 Kronprinz03 commented Apr 30, 2026

Changes

  • Added a new logs to inform the User that the process has been started
  • Removed warning, if no business-key is used

Copy link
Copy Markdown
Contributor

@hyperspace-insights hyperspace-insights Bot left a comment

Choose a reason for hiding this comment

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

The PR is straightforward and in good shape overall. The one issue flagged is a stale string literal ('not found for process start') left in the test predicate after the corresponding warning constant was deleted — it makes the assertion trivially true for the wrong reason and will mislead future readers.

PR Bot Information

Version: 1.20.37

  • File Content Strategy: Full file content
  • Event Trigger: pull_request.opened
  • LLM: anthropic--claude-4.6-sonnet
  • Correlation ID: e41a54cd-7949-455c-b933-ca1e9f62b6fe

Comment on lines +819 to +823
expect(
result.warnings.some(
(w) =>
w.msg.includes(BUSINESS_KEY) &&
w.msg.includes('not found for process start') &&
w.msg.includes('Length check will be skipped'),
(w) => w.msg.includes(BUSINESS_KEY) && w.msg.includes('not found for process start'),
),
).toBe(true);
).toBe(false);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Best Practices: Stale string literal references deleted warning message

The predicate w.msg.includes('not found for process start') references text that was part of the now-deleted WARNING_BUSINESS_KEY_NOT_FOUND constant. Because the assertion changed to .toBe(false), this filter will trivially pass regardless of what string is used — the dead literal adds no protection and will confuse future readers into thinking the old warning message could still appear.

Since the intent is simply to assert that no business-key-related warning is emitted, the filter should only check for the BUSINESS_KEY constant (or be replaced with a toHaveLength(0) check on result.warnings).

Suggested change
expect(
result.warnings.some(
(w) =>
w.msg.includes(BUSINESS_KEY) &&
w.msg.includes('not found for process start') &&
w.msg.includes('Length check will be skipped'),
(w) => w.msg.includes(BUSINESS_KEY) && w.msg.includes('not found for process start'),
),
).toBe(true);
).toBe(false);
expect(
result.warnings.some((w) => w.msg.includes(BUSINESS_KEY)),
).toBe(false);

Double-check suggestion before committing. Edit this comment for amendments.


Please provide feedback on the review comment by checking the appropriate box:

  • 🌟 Awesome comment, a human might have missed that.
  • ✅ Helpful comment
  • 🤷 Neutral
  • ❌ This comment is not helpful

@hyperspace-insights
Copy link
Copy Markdown
Contributor

Summary

The following content is AI-generated and provides a summary of the pull request:


Fix: Remove Build-Time Warning for Missing Business Key and Add Process Started Log

Bug Fix / Improvement

🐛 Addresses two logging issues when starting a process:

  1. Removes the spurious build-time WARNING logged when @bpm.process.businessKey is not defined — since many processes simply don't use a business key, this warning was unnecessary noise.
  2. Adds a success log message (Process started successfully) after a process instance is actually started, in both BTP and local process service implementations.

Changes

  • lib/build/constants.ts: Removed the WARNING_BUSINESS_KEY_NOT_FOUND constant that was used to generate the now-removed warning.
  • lib/build/validations.ts: Removed the import and usage of WARNING_BUSINESS_KEY_NOT_FOUND, so no warning is emitted when the business key annotation is absent.
  • lib/handlers/processService.ts: Updated debug log message from Starting process to Queuing process start to better reflect the handler's role.
  • srv/BTPProcessService.ts: Added LOG.info('Process started successfully', definitionId) after the workflow is started successfully.
  • srv/localProcessService.ts: Added LOG.info('Process started successfully', definitionId) after the local workflow is started successfully.
  • tests/integration/build-validation/processStart.test.ts: Updated test cases to assert that no warning is emitted when the business key annotation is missing (previously the test expected a warning).
  • CHANGELOG.md: Documented the added process start logs and the removal of the build-time business key warning under version 0.2.1.

Images and Links (ONLY add this section if the links are explicitly provided in the original content)

GitHub Issue #83: Remove build time warning and improve logs


  • 🔄 Regenerate and Update Summary
  • ✏️ Insert as PR Description (deletes this comment)
  • 🗑️ Delete comment
PR Bot Information

Version: 1.20.37

Copy link
Copy Markdown
Contributor

@hyperspace-insights hyperspace-insights Bot left a comment

Choose a reason for hiding this comment

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

Now I have enough context. Let me analyze the changes:

  1. lib/handlers/processService.ts – The log message was changed from "Starting process: ${definitionId}" to "Queuing process start: ${definitionId}". This is LOG.debug, which is consistent with the handler being a routing/queuing layer before the actual service executes the start.

  2. srv/BTPProcessService.ts and srv/localProcessService.tsLOG.info('Process started successfully', definitionId) was added. In BTPProcessService.ts this is placed after await this.workflowInstanceClient.startWorkflow(...), so it accurately reflects success. In localProcessService.ts, the localWorkflowStore.startWorkflow(...) call is not awaited — it's synchronous — and the log is placed before return;. This is fine.

  3. lib/build/validations.ts and lib/build/constants.ts – The WARNING_BUSINESS_KEY_NOT_FOUND warning is removed. When bKeyExpr is falsy, the function now silently returns instead of emitting a warning. The constant itself is removed from constants.ts.

  4. tests/integration/build-validation/processStart.test.ts – The test is correctly updated to assert the warning is NOT emitted. However, looking at lines 819–823, the updated assertion still checks w.msg.includes('not found for process start') as a negative — but since the warning is now removed entirely, there won't be any such message. The assertion is valid, but it's worth noting the test still references the old message string, which is harmless but slightly confusing. Actually, this is fine; the test correctly asserts false.

One concern: In localProcessService.ts line 27, LOG.info('Process started successfully', definitionId) is placed after calling localWorkflowStore.startWorkflow(...) which is synchronous, but the log fires unconditionally regardless of whether startWorkflow throws. If it throws, the log would not be reached (the exception propagates), so that's actually fine. No issue there.

Now let me look more carefully at srv/BTPProcessService.ts. The startWorkflow call can throw (it's awaited). The success log at line 27 is after the await, so it only logs on success. This is correct.

One genuine issue: in srv/localProcessService.ts, localWorkflowStore.startWorkflow(...) is not awaited (it appears synchronous), but the LOG.info('Process started successfully', ...) is placed before return;. This is fine as-is.

The test at line 821 references the old message string 'not found for process start' purely as a negative check — this is valid though slightly redundant since the code that would produce that message no longer exists. No actionable bug here.

Overall the changes look clean and correct. No substantive issues found that haven't already been addressed.

The PR is in good shape — the warning removal is properly reflected across the implementation, constant definitions, and tests, and the new "process started successfully" log messages are placed correctly after the actual workflow start calls in both service variants.

PR Bot Information

Version: 1.20.37

  • Correlation ID: aa511894-7dc7-4ce8-a39d-bfdeadcb4f21
  • LLM: anthropic--claude-4.6-sonnet
  • Event Trigger: pull_request.ready_for_review
  • File Content Strategy: Full file content

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants