Implement test run by andrii-flamingo · Pull Request #2218 · flamingo-stack/openframe-oss-lib

andrii-flamingo · 2026-09-16T22:11:17Z

Closes https://app.clickup.com/t/9013925967/86ajehrju

github-actions · 2026-09-16T22:11:34Z

🦩 Flamingo Code Review

6 finding(s) — 3 action required · 1 recommended · 2 informational

Mode: advisory · Rules cited: OFJAVA-011, OPENFRAM-002-21, CODEWIKI-008, OFJAVA-006 · 1 defect(s) outside any rule

Inline comments: 6 new

Need another pass? Commits pushed after this review are not reviewed automatically.

Review the new commits — the commits added since this review
Review the whole diff again — ignoring what was already reviewed

Prefer typing? Comment @flamingo-review, or @flamingo-review full. To review every push on this pull request, add the flamingo-review-always label.

React 👍/👎 on inline comments to teach the reviewer.

Started 2026-09-16 22:11 UTC · updated 2026-09-16 22:12 UTC · workflow run

github-actions · 2026-09-16T22:12:23Z

+    @Value("${openframe.rmm.test-mode.enabled}")
+    private boolean testModeEnabled;


🦩 🔴 [error/action_required] OFJAVA-011 @value property has no default in CustomScheduleScriptExecutionRepositoryImpl, inconsistent with all other injection sites

All other classes in this diff inject openframe.rmm.test-mode.enabled with a default of :false (ScriptMapper, ScriptScheduleMapper, CommandDispatchService, ScriptDispatchService, ScheduleFireDispatcher). This one site omits the default (${openframe.rmm.test-mode.enabled} instead of ${openframe.rmm.test-mode.enabled:false}), which will cause Spring context startup failure (fail-fast, per OFJAVA-011's own no-inline-defaults principle) in any environment where the property is not explicitly set — while every other consumer of the same property will silently default to false. This is an internal inconsistency within the same feature/PR: either all sites should fail fast (no default anywhere) or all should share the same default. As written, this asymmetry means the app will crash on this class's bean creation in environments that omit the property, even though the rest of the code assumes a safe default exists.

Evidence

@Value("${openframe.rmm.test-mode.enabled}") private boolean testModeEnabled;

📝 Committable suggestion

Suggested change

@Value("${openframe.rmm.test-mode.enabled}")

private boolean testModeEnabled;

@Value("${openframe.rmm.test-mode.enabled:false}")

private boolean testModeEnabled;

🤖 Prompt for AI agents

In openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/CustomScheduleScriptExecutionRepositoryImpl.java around lines 64-65, address this code-review finding: @Value property has no default in CustomScheduleScriptExecutionRepositoryImpl, inconsistent with all other injection sites. All other classes in this diff inject `openframe.rmm.test-mode.enabled` with a default of `:false` (ScriptMapper, ScriptScheduleMapper, CommandDispatchService, ScriptDispatchService, ScheduleFireDispatcher). This one site omits the default (`${openframe.rmm.test-mode.enabled}` instead of `${openframe.rmm.test-mode.enabled:false}`), which will cause Spring context startup failure (fail-fast, per OFJAVA-011's own no-inline-defaults principle) in any environment where the property is not explicitly set — while every other consumer of the same property will silently default to false. This is an internal inconsistency within the same feature/PR: either all sites should fail fast (no default anywhere) or all should share the same default. As written, this asymmetry means the app will crash on this class's bean creation in environments that omit the property, even though the rest of the code assumes a safe default exists. The flagged code: ``` @Value("${openframe.rmm.test-mode.enabled}") private boolean testModeEnabled; ``` A suggested replacement for the flagged range: @Value("${openframe.rmm.test-mode.enabled:false}") private boolean testModeEnabled; Make the minimal change that resolves the finding; do not refactor unrelated code.

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

github-actions · 2026-09-16T22:12:24Z

+    @Value("${openframe.rmm.test-mode.enabled}")
+    private boolean testModeEnabled;


🦩 🔴 [error/action_required] OFJAVA-011 @value property missing default in CustomScriptExecutionRepositoryImpl breaks startup fail-fast convention inconsistently with sibling repositories

CustomScriptExecutionRepositoryImpl declares @Value("${openframe.rmm.test-mode.enabled}") with no inline default, while the two sibling repository implementations changed in this same PR (CustomScriptRepositoryImpl, CustomScriptScheduleRepositoryImpl) both use @Value("${openframe.rmm.test-mode.enabled:false}") with an inline default of false. Per OFJAVA-011, @Value properties must have NO inline default — every property must be explicit per environment, and a missing property must fail fast on startup. Here we have the opposite inconsistency: two of the three repos silently default to false (violating OFJAVA-011) while this one will fail startup if the property is not defined (compliant with OFJAVA-011, but inconsistent with its siblings). This split behavior means that if openframe.rmm.test-mode.enabled is not set in a given environment's config, CustomScriptExecutionRepositoryImpl's bean creation will fail while CustomScriptRepositoryImpl and CustomScriptScheduleRepositoryImpl will silently default to disabled — an inconsistent and confusing failure mode across three classes touched by the very same PR for the very same feature flag. Standardize on one approach (per OFJAVA-011: no inline default, explicit declaration in every environment's config) across all three files.

Evidence

@Value("${openframe.rmm.test-mode.enabled}") private boolean testModeEnabled;

🤖 Prompt for AI agents

In openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/CustomScriptExecutionRepositoryImpl.java around lines 77-78, address this code-review finding: @Value property missing default in CustomScriptExecutionRepositoryImpl breaks startup fail-fast convention inconsistently with sibling repositories. CustomScriptExecutionRepositoryImpl declares `@Value("${openframe.rmm.test-mode.enabled}")` with no inline default, while the two sibling repository implementations changed in this same PR (CustomScriptRepositoryImpl, CustomScriptScheduleRepositoryImpl) both use `@Value("${openframe.rmm.test-mode.enabled:false}")` with an inline default of false. Per OFJAVA-011, `@Value` properties must have NO inline default — every property must be explicit per environment, and a missing property must fail fast on startup. Here we have the opposite inconsistency: two of the three repos silently default to `false` (violating OFJAVA-011) while this one will fail startup if the property is not defined (compliant with OFJAVA-011, but inconsistent with its siblings). This split behavior means that if `openframe.rmm.test-mode.enabled` is not set in a given environment's config, `CustomScriptExecutionRepositoryImpl`'s bean creation will fail while `CustomScriptRepositoryImpl` and `CustomScriptScheduleRepositoryImpl` will silently default to disabled — an inconsistent and confusing failure mode across three classes touched by the very same PR for the very same feature flag. Standardize on one approach (per OFJAVA-011: no inline default, explicit declaration in every environment's config) across all three files. The flagged code: ``` @Value("${openframe.rmm.test-mode.enabled}") private boolean testModeEnabled; ``` Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: 62 — react 👍/👎 to teach the reviewer

github-actions · 2026-09-16T22:12:24Z

+    private Criteria baseCriteria(String tenantId, String scriptScheduleId, ScheduleRunQueryFilter filter) {
        Criteria criteria = Criteria.where(FIELD_TENANT_ID).is(tenantId)
                .and(FIELD_SCRIPT_SCHEDULE_ID).is(scriptScheduleId);
+        if (testModeEnabled) {
+            criteria.and(FIELD_TEST_SCRIPT).ne(true);
+        }
        if (filter == null) {
            return criteria;
        }


🦩 🔴 [warn/action_required] Test-mode filter hides schedule-run rows only when test mode is currently enabled, not when the row was created as a test fixture

The new filter if (testModeEnabled) { criteria.and(FIELD_TEST_SCRIPT).ne(true); } inverts the intended visibility rule described elsewhere in the diff (GraphQL schema doc: "Dispatch still runs, user-facing reads hide it" — meaning test rows should ALWAYS be hidden from normal reads, regardless of the current state of the toggle). As written, if testModeEnabled is later turned back to false after some rows were stamped testScript=true during a test window, this filter no longer applies at all and those test-fixture execution rows become visible in the schedule-run history for users — exactly the leak the feature is meant to prevent. The filter should instead unconditionally exclude testScript=true rows from user-facing reads (or be based on a separate 'hide test rows' flag), not gate on the live value of the write-side toggle.

Evidence

private Criteria baseCriteria(String tenantId, String scriptScheduleId, ScheduleRunQueryFilter filter) { Criteria criteria = Criteria.where(FIELD_TENANT_ID).is(tenantId) .and(FIELD_SCRIPT_SCHEDULE_ID).is(scriptScheduleId); if (testModeEnabled) { criteria.and(FIELD_TEST_SCRIPT).ne(true); } if (filter == null) { return criteria; }

📝 Committable suggestion

Suggested change

private Criteria baseCriteria(String tenantId, String scriptScheduleId, ScheduleRunQueryFilter filter) {

Criteria criteria = Criteria.where(FIELD_TENANT_ID).is(tenantId)

.and(FIELD_SCRIPT_SCHEDULE_ID).is(scriptScheduleId);

if (testModeEnabled) {

criteria.and(FIELD_TEST_SCRIPT).ne(true);

}

if (filter == null) {

return criteria;

}

private Criteria baseCriteria(String tenantId, String scriptScheduleId, ScheduleRunQueryFilter filter) {

Criteria criteria = Criteria.where(FIELD_TENANT_ID).is(tenantId)

.and(FIELD_SCRIPT_SCHEDULE_ID).is(scriptScheduleId)

.and(FIELD_TEST_SCRIPT).ne(true);

if (filter == null) {

return criteria;

}

🤖 Prompt for AI agents

In openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/CustomScheduleScriptExecutionRepositoryImpl.java around lines 212-220, address this code-review finding: Test-mode filter hides schedule-run rows only when test mode is currently enabled, not when the row was created as a test fixture. The new filter `if (testModeEnabled) { criteria.and(FIELD_TEST_SCRIPT).ne(true); }` inverts the intended visibility rule described elsewhere in the diff (GraphQL schema doc: "Dispatch still runs, user-facing reads hide it" — meaning test rows should ALWAYS be hidden from normal reads, regardless of the current state of the toggle). As written, if `testModeEnabled` is later turned back to `false` after some rows were stamped `testScript=true` during a test window, this filter no longer applies at all and those test-fixture execution rows become visible in the schedule-run history for users — exactly the leak the feature is meant to prevent. The filter should instead unconditionally exclude `testScript=true` rows from user-facing reads (or be based on a separate 'hide test rows' flag), not gate on the live value of the write-side toggle. The flagged code: ``` private Criteria baseCriteria(String tenantId, String scriptScheduleId, ScheduleRunQueryFilter filter) { Criteria criteria = Criteria.where(FIELD_TENANT_ID).is(tenantId) .and(FIELD_SCRIPT_SCHEDULE_ID).is(scriptScheduleId); if (testModeEnabled) { criteria.and(FIELD_TEST_SCRIPT).ne(true); } if (filter == null) { return criteria; } ``` A suggested replacement for the flagged range: private Criteria baseCriteria(String tenantId, String scriptScheduleId, ScheduleRunQueryFilter filter) { Criteria criteria = Criteria.where(FIELD_TENANT_ID).is(tenantId) .and(FIELD_SCRIPT_SCHEDULE_ID).is(scriptScheduleId) .and(FIELD_TEST_SCRIPT).ne(true); if (filter == null) { return criteria; } Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: 55 — react 👍/👎 to teach the reviewer

github-actions · 2026-09-16T22:12:24Z

+    /**
+     * Tenant-instance test-mode switch. When {@code true}, every newly-created Script is
+     * stamped {@code testScript = true} and hidden from user-facing GraphQL reads by the
+     * repository shield. Default {@code false} — everything is visible.
+     */
+    @Value("${openframe.rmm.test-mode.enabled:false}")
+    private boolean testModeEnabled;


🦩 🟠 [warn/recommended] OPENFRAM-002-21 openframe.rmm.test-mode.enabled duplicated as standalone @value in five separate classes instead of a shared @ConfigurationProperties bean

The same boolean flag openframe.rmm.test-mode.enabled is injected via a standalone @Value field independently in ScriptMapper, ScriptScheduleMapper, CommandDispatchService, ScriptDispatchService, ScheduleFireDispatcher, and CustomScheduleScriptExecutionRepositoryImpl. Per OPENFRAM-002-21, a single boolean toggle used repeatedly across the codebase (not just once) should be centralized — duplicating the same @value binding six times is exactly the kind of scatter this rule intends to prevent, and it already caused a defect (the missing default in CustomScheduleScriptExecutionRepositoryImpl). Introducing a single shared bean/service (e.g., RmmTestModeProperties or a TestModeGate component) would remove the duplication and the drift risk.

Evidence

/** * Tenant-instance test-mode switch. When {@code true}, every newly-created Script is * stamped {@code testScript = true} and hidden from user-facing GraphQL reads by the * repository shield. Default {@code false} — everything is visible. */ @Value("${openframe.rmm.test-mode.enabled:false}") private boolean testModeEnabled;

🤖 Prompt for AI agents

In openframe-api-lib/src/main/java/com/openframe/api/mapper/ScriptMapper.java around lines 19-25, address this code-review finding: openframe.rmm.test-mode.enabled duplicated as standalone @Value in five separate classes instead of a shared @ConfigurationProperties bean. The same boolean flag `openframe.rmm.test-mode.enabled` is injected via a standalone `@Value` field independently in ScriptMapper, ScriptScheduleMapper, CommandDispatchService, ScriptDispatchService, ScheduleFireDispatcher, and CustomScheduleScriptExecutionRepositoryImpl. Per OPENFRAM-002-21, a single boolean toggle used repeatedly across the codebase (not just once) should be centralized — duplicating the same @Value binding six times is exactly the kind of scatter this rule intends to prevent, and it already caused a defect (the missing default in CustomScheduleScriptExecutionRepositoryImpl). Introducing a single shared bean/service (e.g., `RmmTestModeProperties` or a `TestModeGate` component) would remove the duplication and the drift risk. The flagged code: ``` /** * Tenant-instance test-mode switch. When {@code true}, every newly-created Script is * stamped {@code testScript = true} and hidden from user-facing GraphQL reads by the * repository shield. Default {@code false} — everything is visible. */ @Value("${openframe.rmm.test-mode.enabled:false}") private boolean testModeEnabled; ``` Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: 55 — react 👍/👎 to teach the reviewer

github-actions · 2026-09-16T22:12:24Z

+    private Criteria baseCriteria(String tenantId, String scriptScheduleId, ScheduleRunQueryFilter filter) {
        Criteria criteria = Criteria.where(FIELD_TENANT_ID).is(tenantId)
                .and(FIELD_SCRIPT_SCHEDULE_ID).is(scriptScheduleId);
+        if (testModeEnabled) {
+            criteria.and(FIELD_TEST_SCRIPT).ne(true);
+        }
        if (filter == null) {
            return criteria;
        }


🦩 🔵 [info/informational] CODEWIKI-008 baseCriteria changed from static to instance method solely to reach the injected flag, increasing coupling for a query-builder helper

baseCriteria was a private static helper and is now a non-static instance method purely so it can read the newly injected testModeEnabled field. This couples what was a pure, stateless query-building utility to instance state, making it harder to unit test in isolation and inconsistent with the otherwise stateless nature of the other private static criteria builders likely present in this class. Consider passing testModeEnabled as an explicit parameter instead of converting the method to an instance method, preserving testability and locality of the toggle behavior.

Evidence

private Criteria baseCriteria(String tenantId, String scriptScheduleId, ScheduleRunQueryFilter filter) { Criteria criteria = Criteria.where(FIELD_TENANT_ID).is(tenantId) .and(FIELD_SCRIPT_SCHEDULE_ID).is(scriptScheduleId); if (testModeEnabled) { criteria.and(FIELD_TEST_SCRIPT).ne(true); } if (filter == null) { return criteria; }

🤖 Prompt for AI agents

In openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/CustomScheduleScriptExecutionRepositoryImpl.java around lines 212-220, address this code-review finding: baseCriteria changed from static to instance method solely to reach the injected flag, increasing coupling for a query-builder helper. `baseCriteria` was a `private static` helper and is now a non-static instance method purely so it can read the newly injected `testModeEnabled` field. This couples what was a pure, stateless query-building utility to instance state, making it harder to unit test in isolation and inconsistent with the otherwise stateless nature of the other private static criteria builders likely present in this class. Consider passing `testModeEnabled` as an explicit parameter instead of converting the method to an instance method, preserving testability and locality of the toggle behavior. The flagged code: ``` private Criteria baseCriteria(String tenantId, String scriptScheduleId, ScheduleRunQueryFilter filter) { Criteria criteria = Criteria.where(FIELD_TENANT_ID).is(tenantId) .and(FIELD_SCRIPT_SCHEDULE_ID).is(scriptScheduleId); if (testModeEnabled) { criteria.and(FIELD_TEST_SCRIPT).ne(true); } if (filter == null) { return criteria; } ``` Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: 40 — react 👍/👎 to teach the reviewer

github-actions · 2026-09-16T22:12:24Z

+    private void applyManagedScriptShield(Criteria criteria) {
        criteria.and(FIELD_TYPE).nin(ScriptType.SYSTEM, ScriptType.SOFTWARE);
+        if (testModeEnabled) {
+            criteria.and(FIELD_TEST_SCRIPT).ne(true);
+        }
    }


🦩 🔵 [info/informational] OFJAVA-006 Test-mode shield logic duplicated identically across three repository implementations instead of a shared helper

The exact same three-line pattern (if (testModeEnabled) { criteria.and(FIELD_TEST_SCRIPT).ne(true); }) is copy-pasted across CustomScriptExecutionRepositoryImpl, CustomScriptRepositoryImpl, and CustomScriptScheduleRepositoryImpl, each with its own @Value-injected testModeEnabled field and its own FIELD_TEST_SCRIPT constant. Per OFJAVA-009 (refactor when duplication appears, not later), this cross-cutting concern (hiding test scripts/executions/schedules when test-mode is enabled) is a good candidate for extraction into a shared component (e.g., a TestScriptShieldSupport bean injected into all three repositories) so the flag and field name are defined once and the behavior stays consistent as it evolves (as already evidenced by the default-value inconsistency flagged separately).

Evidence

private void applyManagedScriptShield(Criteria criteria) { criteria.and(FIELD_TYPE).nin(ScriptType.SYSTEM, ScriptType.SOFTWARE); if (testModeEnabled) { criteria.and(FIELD_TEST_SCRIPT).ne(true); } }

🤖 Prompt for AI agents

In openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/rmm/CustomScriptRepositoryImpl.java around lines 193-198, address this code-review finding: Test-mode shield logic duplicated identically across three repository implementations instead of a shared helper. The exact same three-line pattern (`if (testModeEnabled) { criteria.and(FIELD_TEST_SCRIPT).ne(true); }`) is copy-pasted across CustomScriptExecutionRepositoryImpl, CustomScriptRepositoryImpl, and CustomScriptScheduleRepositoryImpl, each with its own `@Value`-injected `testModeEnabled` field and its own `FIELD_TEST_SCRIPT` constant. Per OFJAVA-009 (refactor when duplication appears, not later), this cross-cutting concern (hiding test scripts/executions/schedules when test-mode is enabled) is a good candidate for extraction into a shared component (e.g., a `TestScriptShieldSupport` bean injected into all three repositories) so the flag and field name are defined once and the behavior stays consistent as it evolves (as already evidenced by the default-value inconsistency flagged separately). The flagged code: ``` private void applyManagedScriptShield(Criteria criteria) { criteria.and(FIELD_TYPE).nin(ScriptType.SYSTEM, ScriptType.SOFTWARE); if (testModeEnabled) { criteria.and(FIELD_TEST_SCRIPT).ne(true); } } ``` Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: 40 — react 👍/👎 to teach the reviewer

…hotfix/implement-test-run

Implemented test run

a12068e

andrii-flamingo requested a review from aliaska-varieva September 16, 2026 22:11

github-actions Bot reviewed Sep 16, 2026

View reviewed changes

andrii-flamingo added 3 commits September 17, 2026 01:14

Merge branch 'main' into hotfix/implement-test-run

c9209d3

Removed default variable. Fail fast

f20c600

Merge remote-tracking branch 'origin/hotfix/implement-test-run' into …

5f79196

…hotfix/implement-test-run

Navigation Menu

Implement test run - #2218

Implement test run#2218
andrii-flamingo wants to merge 4 commits into
mainfrom
hotfix/implement-test-run

andrii-flamingo commented Sep 16, 2026

Uh oh!

github-actions Bot commented Sep 16, 2026 •

edited

Loading

Uh oh!

github-actions Bot Sep 16, 2026

Uh oh!

github-actions Bot Sep 16, 2026

Uh oh!

github-actions Bot Sep 16, 2026

Uh oh!

github-actions Bot Sep 16, 2026

Uh oh!

github-actions Bot Sep 16, 2026

Uh oh!

github-actions Bot Sep 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		@Value("${openframe.rmm.test-mode.enabled}")
		private boolean testModeEnabled;

Conversation

andrii-flamingo commented Sep 16, 2026

Uh oh!

github-actions Bot commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦩 Flamingo Code Review

Uh oh!

github-actions Bot Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

github-actions Bot commented Sep 16, 2026 •

edited

Loading