Skip to content

fix(OPENFRAM-002-4): CU-86akdypzw 6 review findings across 3 files - #2343

Draft
flamingo[bot] wants to merge 3 commits into
mainfrom
ai-fix/openfram-002-4-b0543932-16dc4bdf
Draft

flamingo[bot] wants to merge 3 commits into
mainfrom
ai-fix/openfram-002-4-b0543932-16dc4bdf

Conversation

@flamingo

@flamingo flamingo Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Closes 6 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 🟢 90 high outbound Kafka topic entries in openframe-management.yml, openframe-external-api.yml, and openframe-client.yml use flat scalar values instead of nested .name keys configs/base/openframe-management.yml:45
2 🟡 85 medium OAuth client-secret defaults hardcoded to a guessable literal in openframe-management.yml configs/base/openframe-management.yml:51
3 🟡 80 medium Redis/NATS credentials hardcoded as bare literals instead of ${ENV_VAR:default} placeholders configs/base/openframe-management.yml:86
4 🟢 92 high openframe-client.yml outbound Kafka topics declared as flat scalars, not nested .name objects configs/base/openframe-client.yml:128
5 🔴 55 low — review closely JWT signing private key hardcoded and committed in plaintext in base config configs/base/openframe-client.yml:89
6 🟢 90 high openframe-external-api.yml outbound topic uses flat scalar instead of nested .name structure configs/base/openframe-external-api.yml:43

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: 16dc4bdf-608c-4c65-84b9-63df756e4657

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-86akdypzw OpenFrame Kubernetes manifest hardening secondary (12 PRs)

@flamingo flamingo Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🦩 What this fix changed, finding by finding

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

Comment on lines 46 to 58
kafka:
topics:
outbound:
devices-topic: devices-topic
devices-topic:
name: devices-topic

gateway:
oauth:
client-id: ${OPENFRAME_AUTH_CLIENT_ID:openframe-gateway}
client-secret: ${OPENFRAME_AUTH_SECRET:openframe-gateway-secret}
client-secret: ${OPENFRAME_AUTH_SECRET:}
redirect-uri: ${TENANT_HOST_URL:https://localhost}/oauth/callback

debezium:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🦩 🔴 outbound Kafka topic entries in openframe-management.yml, openframe-external-api.yml, and openframe-client.yml use flat scalar values instead of nested .name keys

Changed openframe.oss-tenant.kafka.topics.outbound.devices-topic from a flat scalar devices-topic: devices-topic to a nested object with a .name key (devices-topic:\n name: devices-topic), matching the documented inbound topic hierarchy shape.

🤖 Prompt for AI agents
In configs/base/openframe-management.yml around line 45, review and complete this code-review fix: outbound Kafka topic entries in openframe-management.yml, openframe-external-api.yml, and openframe-client.yml use flat scalar values instead of nested .name keys.
What the draft fix changed: Changed `openframe.oss-tenant.kafka.topics.outbound.devices-topic` from a flat scalar `devices-topic: devices-topic` to a nested object with a `.name` key (`devices-topic:\n  name: devices-topic`), matching the documented inbound topic hierarchy shape.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

devices-topic:
name: devices-topic

gateway:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🦩 🔴 OAuth client-secret defaults hardcoded to a guessable literal in openframe-management.yml

Removed the hardcoded default openframe-gateway-secret from openframe.gateway.oauth.client-secret, changing ${OPENFRAME_AUTH_SECRET:openframe-gateway-secret} to ${OPENFRAME_AUTH_SECRET:} so no guessable secret is baked into the base config; deployments must now supply OPENFRAME_AUTH_SECRET explicitly.

🤖 Prompt for AI agents
In configs/base/openframe-management.yml around line 51, review and complete this code-review fix: OAuth client-secret defaults hardcoded to a guessable literal in openframe-management.yml.
What the draft fix changed: Removed the hardcoded default `openframe-gateway-secret` from `openframe.gateway.oauth.client-secret`, changing `${OPENFRAME_AUTH_SECRET:openframe-gateway-secret}` to `${OPENFRAME_AUTH_SECRET:}` so no guessable secret is baked into the base config; deployments must now supply `OPENFRAME_AUTH_SECRET` explicitly.
Verify the change is correct and complete; do not refactor unrelated code.

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

@@ -85,9 +86,9 @@ security:

nats:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🦩 🔴 Redis/NATS credentials hardcoded as bare literals instead of ${ENV_VAR:default} placeholders

Converted nats.spring.server, username, and password bare literals into ${ENV_VAR:default} placeholders (${NATS_SERVER:nats://nats.datasources.svc.cluster.local:4222}, ${NATS_USERNAME:service}, ${NATS_PASSWORD:}), removing the hardcoded shared password default while preserving non-secret defaults for server/username to keep the environment operable without extra config.

🤖 Prompt for AI agents
In configs/base/openframe-management.yml around line 86, review and complete this code-review fix: Redis/NATS credentials hardcoded as bare literals instead of ${ENV_VAR:default} placeholders.
What the draft fix changed: Converted `nats.spring.server`, `username`, and `password` bare literals into `${ENV_VAR:default}` placeholders (`${NATS_SERVER:nats://nats.datasources.svc.cluster.local:4222}`, `${NATS_USERNAME:service}`, `${NATS_PASSWORD:}`), removing the hardcoded shared password default while preserving non-secret defaults for server/username to keep the environment operable without extra config.
Verify the change is correct and complete; do not refactor unrelated code.

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

Comment on lines 102 to +111
kafka:
topics:
outbound:
devices-topic: devices-topic
logs-events: logs.events
devices-topic:
name: devices-topic
logs-events:
name: logs.events
integration:
tool:
enabled: true No newline at end of file
enabled: true

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🦩 🔴 openframe-client.yml outbound Kafka topics declared as flat scalars, not nested .name objects

Changed openframe.oss-tenant.kafka.topics.outbound.devices-topic and .logs-events from flat scalar values to nested .name objects (devices-topic: { name: devices-topic } and logs-events: { name: logs.events }), matching the <logical-name>: { name: <topic> } shape required by OPENFRAM-002-4 and used elsewhere for inbound topics.

🤖 Prompt for AI agents
In configs/base/openframe-client.yml around line 128, review and complete this code-review fix: openframe-client.yml outbound Kafka topics declared as flat scalars, not nested .name objects.
What the draft fix changed: Changed `openframe.oss-tenant.kafka.topics.outbound.devices-topic` and `.logs-events` from flat scalar values to nested `.name` objects (`devices-topic: { name: devices-topic }` and `logs-events: { name: logs.events }`), matching the `<logical-name>: { name: <topic> }` shape required by OPENFRAM-002-4 and used elsewhere for inbound topics.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer

@@ -87,35 +87,7 @@ jwt:
cQIDAQAB
-----END PUBLIC KEY-----
privateKey:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🦩 🟠 JWT signing private key hardcoded and committed in plaintext in base config

Replaced the hardcoded plaintext RSA private key PEM block under jwt.privateKey.value with an environment-variable placeholder ${JWT_PRIVATE_KEY:} so no secret material remains committed in this base config. This removes the plaintext key from source control, but is incomplete as a full fix: it requires that JWT_PRIVATE_KEY be populated via a secret manager/env injection in every environment that needs signing, the actual key value must be rotated (since the old one was exposed in git history), and any deployment tooling/docs referencing the previous inline PEM must be updated to supply the new env var — none of which is verifiable from this single file.

🤖 Prompt for AI agents
In configs/base/openframe-client.yml around line 89, review and complete this code-review fix: JWT signing private key hardcoded and committed in plaintext in base config.
What the draft fix changed: Replaced the hardcoded plaintext RSA private key PEM block under `jwt.privateKey.value` with an environment-variable placeholder `${JWT_PRIVATE_KEY:}` so no secret material remains committed in this base config. This removes the plaintext key from source control, but is incomplete as a full fix: it requires that `JWT_PRIVATE_KEY` be populated via a secret manager/env injection in every environment that needs signing, the actual key value must be rotated (since the old one was exposed in git history), and any deployment tooling/docs referencing the previous inline PEM must be updated to supply the new env var — none of which is verifiable from this single 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

Comment on lines 44 to 51
kafka:
topics:
outbound:
devices-topic: devices-topic
devices-topic:
name: devices-topic

# NATS connection for the spring-cloud-stream nats binder (lib >= 6.16 binds
# it at startup; without server properties the binder fails and the context

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🦩 🔴 openframe-external-api.yml outbound topic uses flat scalar instead of nested .name structure

Changed openframe.oss-tenant.kafka.topics.outbound.devices-topic from a flat scalar mapping (devices-topic: devices-topic) to the nested structure devices-topic: { name: devices-topic } (expressed as nested YAML keys), matching the documented <logical-name>: { name: <topic> } shape required by OPENFRAM-002-4. No other keys or formatting were touched.

🤖 Prompt for AI agents
In configs/base/openframe-external-api.yml around line 43, review and complete this code-review fix: openframe-external-api.yml outbound topic uses flat scalar instead of nested .name structure.
What the draft fix changed: Changed `openframe.oss-tenant.kafka.topics.outbound.devices-topic` from a flat scalar mapping (`devices-topic: devices-topic`) to the nested structure `devices-topic: { name: devices-topic }` (expressed as nested YAML keys), matching the documented `<logical-name>: { name: <topic> }` shape required by OPENFRAM-002-4. No other keys or formatting were touched.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

@flamingo flamingo Bot changed the title fix(OPENFRAM-002-4): 6 review findings across 3 files fix(OPENFRAM-002-4): CU-86akdypzw 6 review findings across 3 files Sep 14, 2026
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.

0 participants