Logging improvement: detect.policy.check.fail.on.names (IDETECT-3941)

[shantyk]

Please see confluence page linked under IDETECT-3941 for examples of what the logs output looks like + initial spike that was groomed.
(+ copilot's MR summary below is spot on)

[Copilot]

Pull Request Overview

This PR improves logging and handling of policy check failures by name, introduces a new exit code type for name-based policy violations, refactors violation collection/logging in PolicyChecker, and enhances the exit code manager to attach dynamic reasons.

• Added FAILURE_POLICY_NAME_VIOLATION exit code and updated DetectStatusLogger to treat it like other failures without advice.
• Refactored PolicyChecker to collect fatal and non-fatal violations in a unified manner and added richer logging.
• Enhanced ExitCodeManager and ExitCodeType to carry dynamic descriptions for specific exit codes.
• Non-fatal policy violations are reported at DEBUG level.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/test/java/com/blackduck/integration/detect/workflow/blackduck/developer/RapidModeLogReportOperationTest.java	Added a TODO placeholder indicating missing test cases for new behavior.
src/main/java/com/blackduck/integration/detect/workflow/status/DetectStatusLogger.java	Included the new exit code type in the “no advice required” list.
src/main/java/com/blackduck/integration/detect/workflow/blackduck/policy/PolicyChecker.java	Refactored how policy violations are collected and logged, splitting fatal vs. non-fatal.
src/main/java/com/blackduck/integration/detect/lifecycle/shutdown/ExitCodeManager.java	Updated logic to track the winning exit code request and attach dynamic reasons.
src/main/java/com/blackduck/integration/detect/configuration/enumeration/ExitCodeType.java	Extended the enum to support a dynamic description flag and setter.

Comments suppressed due to low confidence (1)

src/test/java/com/blackduck/integration/detect/workflow/blackduck/developer/RapidModeLogReportOperationTest.java:60

• There are no tests for the newly introduced FAILURE_POLICY_NAME_VIOLATION scenario; add unit tests to verify exitCodePublisher publishes the correct exit code and reason when policy names trigger a failure.

    // TODO add test cases here

[Copilot]

[nitpick] Mutating an enum instance at runtime can lead to shared mutable state and thread-safety issues; consider returning a wrapper or result object that carries both the exit code and its reason instead of modifying the enum value directly.

[shantyk]

Since multiple threads are a non-issue in Detect (with the exception of certain non-buildless Detectors potentially), modifying the enums state in this way felt like a balance between simplicity + future usability without compromising thread safety as warned here. Did others have thoughts? I know the dynamic description flag approach is a bit weird and maybe others have better ideas on how we could achieve the same thing but in a better way

[zahidblackduck]

So, the above two comments got me interested and I spent some time on java enum behavior. It looks like that modifying an enum field is highly discouraged for various reasons. Mutability, thread-safety, side effects. Enum fields are advised to be kept final. So, I was wondering whether we could avoid this approach of setting enum field and still achieve the same result that you are trying.

[andrian-sevastyanov]

Copilot had a decent suggestion above:

consider returning a simple data object containing the exit code and reason instead of changing the enum's description field

Building on that, we already have such a data class: it's the ExitCodeRequest.

Since ExitCodeManager.getWinningExitCode() isn't used in many places it should be pretty straightforward to modify it to instead return ExitCodeRequest.

The ExitCodeRequest could then have a getDescription() method which by default will return the plain ExitCodeType description and in special cases would return a modified one.

The added bonus of this idea is that it could allow us to avoid having to make any kind of changes to the ExitCodeType enum.

[shantyk]

@andrian-sevastyanov @zahidblackduck thanks for your comments. Indeed ExitCodeRequest contains reason + ExitCodeType which is what we need to easily carry the info to the end but throughout the code I saw multiple uses of ExitCodeRequest with a reason that was "swallowed" during the execution and never showed up in output (neither logs nor status.json) and the static description from the enum was favoured. To make sure only the description for FAILURE_POLICY_NAME_VIOLATION is updated for the detect.policy.check.fail.on.names use case and all other exit codes remain as is, we would need to update all instances of ExitCodeRequest and replace the "reason" string to null to return the appropriate ExitCodeType's static description as before.

Alternatively, we can leave the "reason" strings as is, and now they would be present in the output unlike before.

Thoughts?

Example: A container scan related failure

Before:
2025-07-14 14:09:56 MDT INFO [main] --- ======== Detect Status ========
2025-07-14 14:09:56 MDT INFO [main] ---
2025-07-14 14:09:56 MDT ERROR [main] --- CONTAINER_SCAN: FAILURE
2025-07-14 14:09:56 MDT ERROR [main] --- Overall Status: FAILURE_BLACKDUCK_FEATURE_ERROR - Detect encountered an error while attempting an operation on Black Duck. Ensure that your Black Duck version is compatible with this version of Detect, and that your Black Duck user account has the required roles.
2025-07-14 14:09:56 MDT INFO [main] ---
2025-07-14 14:09:56 MDT INFO [main] --- ===============================

After:
2025-07-14 14:05:18 MDT INFO [main] --- ======== Detect Status ========
2025-07-14 14:05:18 MDT INFO [main] ---
2025-07-14 14:05:18 MDT ERROR [main] --- CONTAINER_SCAN: FAILURE
2025-07-14 14:05:18 MDT ERROR [main] --- Overall Status: FAILURE_BLACKDUCK_FEATURE_ERROR - insert dynamic/custom reason/description here
2025-07-14 14:05:18 MDT INFO [main] ---
2025-07-14 14:05:18 MDT INFO [main] --- ===============================
2025-07-14 14:05:18 MDT INFO [main] ---
2025-07-14 14:05:18 MDT INFO [main] --- Detect duration: 00h 00m 05s 757ms
2025-07-14 14:05:18 MDT ERROR [main] --- Exiting with code 11 - FAILURE_BLACKDUCK_FEATURE_ERROR

[shantyk]

In short: I have updated getWinningExitCode() to return ExitCodeRequest as suggested but not sure if I should modify existing calls that pass in a reason, which was previously "swallowed", to now surface in the output + status.json or to instead pass in null and keep behaviour the same @devmehtabd @andrian-sevastyanov @zahidblackduck

[devmehtabd]

I would say keeping the behavior same makes the most sense to me (since we have only week of QA left). I do see we have existing constructor which takes in only the ExitCodeType and not reason, may be updating the calls which pass in reason will be the right approach. Also, we should take QA's opinion on it as well if we decide to change the behavior.

[andrian-sevastyanov]

I also vote to not change existing behaviour unless we are willing to comb through all reasons that are currently being passed in.
Another thing we could do is have something like class ExitCodeRequestWithCustomDescription extends ExitCodeRequest which we could utilize when we want to explicitly override the default behaviour.

[shantyk]

For the implementation of the ExitCodeRequestWithCustomDescription suggestion please see 680056b

[andrian-sevastyanov]

I'd say we don't need this class after all, for the following reasons:

• it seems you cleaned up all pre-existing exit code publishing to not pass reason where it was not used
• this class currently does not override any methods so whether someone uses this or the parent class will not make any difference

[shantyk]

After giving your suggestion a try, I realized it would be great to keep because I believe it will prevent future errors from devs inputting a custom reason that never shows up in the output. As I understand it, given we have these two constructors:

    public void publishExitCode(ExitCodeType exitCodeType) {
        eventSystem.publishEvent(Event.ExitCode, new ExitCodeRequest(exitCodeType, null));
    }

    public void publishExitCode(ExitCodeRequestWithCustomDescription exitCodeRequest) {
        eventSystem.publishEvent(Event.ExitCode, exitCodeRequest);
    }

(I removed the one accepting ExitCodeRequest in latest commit)

It would not be possible for someone to pass in a reason without explicitly creating a ExitCodeRequestWithCustomDescription first -- which I hope will be enough to prevent this situation by giving a compile error. So no more "swallowed" reasons?

[andrian-sevastyanov]

Got it, thanks for the clarification. I didn't realize that we pass null to ExitCodeRequest now.
This will work.
I would however try to design this in a way that ensures that we don't pass around null values.

[shantyk]

Please see a3cea77 for some refactoring to prevent null values/clean up regular and custom ExitCodeRequests (found some more spots where an exit code was being published in a way that was not using the ExitCodePublisher class as they should)