fix(OFJAVA-013): CU-86akqmm95 3 review findings across 3 files

[flamingo]

Closes 3 review findings across 3 files.

Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.

#	Fix confidence	Finding	Location
1	🟡 75 medium	findByRegistrationId returns null instead of throwing or using Optional pipeline	openframe-authorization-service-core/src/main/java/com/openframe/authz/config/DynamicClientRegistrationRepository.java:26
2	🟡 70 medium	linkTempAttachmentsToArticle silently drops attachments on move failure instead of surfacing a real error	openframe-api-lib/src/main/java/com/openframe/api/service/knowledgebase/KnowledgeBaseTempAttachmentService.java:104
3	🔴 55 low — review closely	resolveBaseUrl documented to return null instead of Optional	openframe-stream-service-core/src/main/java/com/openframe/stream/service/FleetBaseUrlResolver.java:12

What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.

---

Run: https://product-hub.flamingo.so/admin/code-review
Run id: 9a25c19f-a499-4571-bb37-da3cba9b8394

Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.

ClickUp task: CU-86akqmm95 OpenFrame lib batch review findings sweep (15 PRs) (part 1)

[flamingo]

🦩 What this fix changed, finding by finding

3 finding(s) fixed in this draft — 3 explained inline on the diff; 1 low-confidence hunk(s) need close review before merging.

[flamingo]

🦩 🔴 findByRegistrationId returns null instead of throwing or using Optional pipeline

In findByRegistrationId, replaced the null-returning tenant resolution with resolveTenantId() now returning Optional<String> and using .orElseThrow(...) to throw an IllegalStateException when the tenant cannot be resolved, instead of returning null. The catch (IllegalArgumentException ex) branch around dynamic.loadClient(...) now rethrows as IllegalStateException (wrapping the original exception) instead of returning null. resolveTenantId() was changed to return Optional<String> (via Optional.of/Optional.empty) instead of a nullable String, so absence is modeled through the Optional pipeline as required. Note: this changes the runtime behavior from a silent null return (which the caller/Spring OAuth2 machinery may not have handled) to a thrown exception; a global exception handler mapping these to a 400/404 was not added since it is outside this file's scope, so downstream behavior on this exception being uncaught should be verified.

🤖 Prompt for AI agents

In openframe-authorization-service-core/src/main/java/com/openframe/authz/config/DynamicClientRegistrationRepository.java around line 26, review and complete this code-review fix: findByRegistrationId returns null instead of throwing or using Optional pipeline.
What the draft fix changed: In `findByRegistrationId`, replaced the null-returning tenant resolution with `resolveTenantId()` now returning `Optional<String>` and using `.orElseThrow(...)` to throw an `IllegalStateException` when the tenant cannot be resolved, instead of returning null. The `catch (IllegalArgumentException ex)` branch around `dynamic.loadClient(...)` now rethrows as `IllegalStateException` (wrapping the original exception) instead of returning null. `resolveTenantId()` was changed to return `Optional<String>` (via `Optional.of`/`Optional.empty`) instead of a nullable `String`, so absence is modeled through the Optional pipeline as required. Note: this changes the runtime behavior from a silent null return (which the caller/Spring OAuth2 machinery may not have handled) to a thrown exception; a global exception handler mapping these to a 400/404 was not added since it is outside this file's scope, so downstream behavior on this exception being uncaught should be verified.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 linkTempAttachmentsToArticle silently drops attachments on move failure instead of surfacing a real error

In moveToArticle, the catch (Exception e) block no longer swallows the failure by returning null; it now throws a new IllegalStateException wrapping the original exception, identifying the failed temp attachment id and target article id. Since the method runs inside the @Transactional linkTempAttachmentsToArticle, this propagates out of the stream in that method (the .filter(Objects::nonNull) and now-unused Objects import were removed accordingly), causing the transaction to roll back rather than silently returning a shorter attachment list. This surfaces the error to the caller instead of hiding it, per OFJAVA-013. A more complete fix might introduce a dedicated checked/unchecked exception type or a result object listing failed temp ids for partial-success reporting, but throwing here correctly stops silent data loss and is a minimal, safe change within this file.

🤖 Prompt for AI agents

In openframe-api-lib/src/main/java/com/openframe/api/service/knowledgebase/KnowledgeBaseTempAttachmentService.java around line 104, review and complete this code-review fix: linkTempAttachmentsToArticle silently drops attachments on move failure instead of surfacing a real error.
What the draft fix changed: In `moveToArticle`, the `catch (Exception e)` block no longer swallows the failure by returning `null`; it now throws a new `IllegalStateException` wrapping the original exception, identifying the failed temp attachment id and target article id. Since the method runs inside the `@Transactional` `linkTempAttachmentsToArticle`, this propagates out of the stream in that method (the `.filter(Objects::nonNull)` and now-unused `Objects` import were removed accordingly), causing the transaction to roll back rather than silently returning a shorter attachment list. This surfaces the error to the caller instead of hiding it, per OFJAVA-013. A more complete fix might introduce a dedicated checked/unchecked exception type or a result object listing failed temp ids for partial-success reporting, but throwing here correctly stops silent data loss and is a minimal, safe change within this file.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 resolveBaseUrl documented to return null instead of Optional

Changed FleetBaseUrlResolver#resolveBaseUrl return type from String to Optional<String> and updated the Javadoc to describe returning Optional.empty() instead of null. This models expected absence per OFJAVA-013/OFJAVA-003. Risk: this is a public interface, so any existing callers in this codebase or the openframe-saas-shared implementation (not visible here) will fail to compile until updated to match the new signature; a complete fix requires updating those call sites/implementations as well, which are outside this file.

🤖 Prompt for AI agents

In openframe-stream-service-core/src/main/java/com/openframe/stream/service/FleetBaseUrlResolver.java around line 12, review and complete this code-review fix: resolveBaseUrl documented to return null instead of Optional.
What the draft fix changed: Changed FleetBaseUrlResolver#resolveBaseUrl return type from `String` to `Optional<String>` and updated the Javadoc to describe returning `Optional.empty()` instead of null. This models expected absence per OFJAVA-013/OFJAVA-003. Risk: this is a public interface, so any existing callers in this codebase or the openframe-saas-shared implementation (not visible here) will fail to compile until updated to match the new signature; a complete fix requires updating those call sites/implementations as well, which are outside this file.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer