[DO NOT MERGE] #2212 on oss before #2356 — snapshot for feature tenant #3330 - #2358
semen-flamingo wants to merge 6 commits into
Conversation
…g and agent-version gate Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnYUpYgKvuMw6hB5VZSimY
…sion gate Review follow-up (#2212): - machine.{id}.delivery.result carries ACKED/DONE/FAILED with dispatchId for every type; InstalledAgentService and ScriptExecutionAcknowledgeListener are back to main - offline rows are postponed to the next sweep tick instead of parked and woken by a device event - per-type flag only; the agent-version gate and AgentVersion are gone - ToolInstallationDeliverySeed is a top-level class Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnYUpYgKvuMw6hB5VZSimY
An incomplete, unknown or malformed delivery result is an agent contract violation: error log + openframe.delivery.result.rejected{reason} for alerting, then acked so it cannot poison the consumer.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnYUpYgKvuMw6hB5VZSimY
… back
DeliveryRef {type, targetId, dispatchId} replaces the bare dispatchId in the command body; the result message carries the same block, so the agent reports without knowing types or targets.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnYUpYgKvuMw6hB5VZSimY
🦩 Flamingo Code Review5 finding(s) — 2 action required · 3 recommended · 0 informational Mode: advisory · 5 defect(s) outside any rule Inline comments: 5 new Need another pass? Commits pushed after this review are not reviewed automatically.
Prefer typing? Comment React 👍/👎 on inline comments to teach the reviewer. Started 2026-09-23 21:12 UTC · updated 2026-09-23 21:12 UTC · workflow run |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -26,9 +28,6 @@ public class DeliveryRecorder { | |||
| private final ObjectMapper objectMapper; | |||
|
|
|||
| public void record(DeliveryRequest<?> request) { | |||
There was a problem hiding this comment.
🦩 🔴 [error/action_required] DeliveryRecorder no longer checks properties.isEnabled(), removing the kill-switch for delivery recording
The previous record() method returned early when !properties.isEnabled(), providing an operator kill-switch to stop new delivery rows being written. This early-return has been deleted along with the isEnabled() no-arg check, and DeliveryProperties#isEnabled has been changed to take a DeliveryType parameter (per-type enablement) elsewhere in this PR (per the cross-repo facts, isEnabled signature changed). However DeliveryRecorder.record() does not call the new per-type isEnabled(type) at all — it unconditionally upserts the pending row regardless of whether the type is enabled. This means an admin disabling a delivery type (e.g. via properties.setEnabled(Map.of(TOOL_INSTALLATION, false))) will not actually stop the recorder from writing rows for that type; only the dispatcher's behavior would need to check this, but there's no evidence in the diff that DeliveryDispatcher.dispatch() checks isEnabled(type) either before recording/publishing. This looks like a functional regression: the enable/disable feature flag is defined and tested (see DeliveryPropertiesTest) but never consulted by dispatch/record.
Evidence
public void record(DeliveryRequest<?> request) {
MachineDelivery delivery = pendingRow(request);
repository.upsertPending(delivery);
log.info("Delivery recorded: type={} targetId={} machineId={}",
request.getType(), request.getTargetId(), request.getMachineId());
}
🤖 Prompt for AI agents
In openframe-machine-delivery/src/main/java/com/openframe/delivery/dispatch/DeliveryRecorder.java around lines 30-35, address this code-review finding: DeliveryRecorder no longer checks properties.isEnabled(), removing the kill-switch for delivery recording.
The previous `record()` method returned early when `!properties.isEnabled()`, providing an operator kill-switch to stop new delivery rows being written. This early-return has been deleted along with the `isEnabled()` no-arg check, and `DeliveryProperties#isEnabled` has been changed to take a `DeliveryType` parameter (per-type enablement) elsewhere in this PR (per the cross-repo facts, `isEnabled` signature changed). However `DeliveryRecorder.record()` does not call the new per-type `isEnabled(type)` at all — it unconditionally upserts the pending row regardless of whether the type is enabled. This means an admin disabling a delivery type (e.g. via `properties.setEnabled(Map.of(TOOL_INSTALLATION, false))`) will not actually stop the recorder from writing rows for that type; only the dispatcher's behavior would need to check this, but there's no evidence in the diff that `DeliveryDispatcher.dispatch()` checks `isEnabled(type)` either before recording/publishing. This looks like a functional regression: the enable/disable feature flag is defined and tested (see DeliveryPropertiesTest) but never consulted by dispatch/record.
The flagged code:
```
public void record(DeliveryRequest<?> request) {
MachineDelivery delivery = pendingRow(request);
repository.upsertPending(delivery);
log.info("Delivery recorded: type={} targetId={} machineId={}",
request.getType(), request.getTargetId(), request.getMachineId());
}
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 55 — react 👍/👎 to teach the reviewer
| @Component | ||
| @RequiredArgsConstructor | ||
| @ConditionalOnProperty(name = {"openframe.delivery.enabled", "openframe.delivery.sweep.enabled"}, havingValue = "true") | ||
| @ConditionalOnProperty(name = "openframe.delivery.sweep.enabled", havingValue = "true") |
There was a problem hiding this comment.
🦩 🔴 [warn/action_required] DeliverySweepScheduler/Service/Watchdog/MachineOnlineStatus lost the top-level openframe.delivery.enabled gate
Previously these beans were conditional on BOTH openframe.delivery.enabled and openframe.delivery.sweep.enabled (an array of property names, which Spring's @ConditionalOnProperty AND's together). The diff narrows the condition to only openframe.delivery.sweep.enabled. If an operator sets openframe.delivery.enabled=false (to fully disable the delivery subsystem) but leaves openframe.delivery.sweep.enabled=true (e.g. a stale/default value), the sweep scheduler, sweep service, watchdog, and machine-online-status beans will now activate even though delivery dispatch and recording (which also lost its isEnabled() guard in DeliveryRecorder) may still run un-gated. This silently changes the activation semantics of four beans across the module and should be called out explicitly, since DeliveryProperties#isEnabled also changed from a global boolean to a per-type map with no obvious single 'delivery enabled' switch anymore — verify this is an intentional design change (per-type enablement) and that the sweep components no longer need the coarse gate.
Evidence
@ConditionalOnProperty(name = "openframe.delivery.sweep.enabled", havingValue = "true")
🤖 Prompt for AI agents
In openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliverySweepScheduler.java around line 14, address this code-review finding: DeliverySweepScheduler/Service/Watchdog/MachineOnlineStatus lost the top-level openframe.delivery.enabled gate.
Previously these beans were conditional on BOTH `openframe.delivery.enabled` and `openframe.delivery.sweep.enabled` (an array of property names, which Spring's @ConditionalOnProperty AND's together). The diff narrows the condition to only `openframe.delivery.sweep.enabled`. If an operator sets `openframe.delivery.enabled=false` (to fully disable the delivery subsystem) but leaves `openframe.delivery.sweep.enabled=true` (e.g. a stale/default value), the sweep scheduler, sweep service, watchdog, and machine-online-status beans will now activate even though delivery dispatch and recording (which also lost its `isEnabled()` guard in DeliveryRecorder) may still run un-gated. This silently changes the activation semantics of four beans across the module and should be called out explicitly, since `DeliveryProperties#isEnabled` also changed from a global boolean to a per-type map with no obvious single 'delivery enabled' switch anymore — verify this is an intentional design change (per-type enablement) and that the sweep components no longer need the coarse gate.
The flagged code:
```
@ConditionalOnProperty(name = "openframe.delivery.sweep.enabled", havingValue = "true")
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 45 — react 👍/👎 to teach the reviewer
| Sweep sweep = properties.getSweep(); | ||
| long recheckMillis = sweep.getInterval(); | ||
| Instant recheckAt = now.plusMillis(recheckMillis); | ||
| Instant dueAt = earliest(windowEnd, recheckAt); | ||
| String id = delivery.getId(); |
There was a problem hiding this comment.
🦩 🟠 [warn/recommended] DeliverySweepService now interprets sweep interval as milliseconds instead of seconds without renaming or documenting the unit change
Previously recheckAt = now.plusSeconds(policy.getMaxRetryIntervalSeconds()). The new code reads sweep.getInterval() and calls now.plusMillis(recheckMillis). This silently changes both the source of the recheck interval (from a per-type Policy field to the global Sweep config) and its unit (seconds -> millis). If Sweep.getInterval() is reused elsewhere (e.g. the @Scheduled(fixedDelayString = "${openframe.delivery.sweep.interval}") in DeliverySweepScheduler, which is a Spring duration string typically in ms), conflating the sweep tick interval with the machine-recheck-when-offline interval is a semantic change: a delivery whose machine is offline will now be rechecked after the scheduler's own tick interval rather than after the type-specific maxRetryIntervalSeconds policy value. This drops per-type backoff tuning for offline-machine parking and should be confirmed intentional.
Evidence
Sweep sweep = properties.getSweep();
long recheckMillis = sweep.getInterval();
Instant recheckAt = now.plusMillis(recheckMillis);
Instant dueAt = earliest(windowEnd, recheckAt);
String id = delivery.getId();
🤖 Prompt for AI agents
In openframe-machine-delivery/src/main/java/com/openframe/delivery/sweep/DeliverySweepService.java around lines 97-101, address this code-review finding: DeliverySweepService now interprets sweep interval as milliseconds instead of seconds without renaming or documenting the unit change.
Previously `recheckAt = now.plusSeconds(policy.getMaxRetryIntervalSeconds())`. The new code reads `sweep.getInterval()` and calls `now.plusMillis(recheckMillis)`. This silently changes both the source of the recheck interval (from a per-type Policy field to the global Sweep config) and its unit (seconds -> millis). If `Sweep.getInterval()` is reused elsewhere (e.g. the `@Scheduled(fixedDelayString = "${openframe.delivery.sweep.interval}")` in DeliverySweepScheduler, which is a Spring duration string typically in ms), conflating the sweep tick interval with the machine-recheck-when-offline interval is a semantic change: a delivery whose machine is offline will now be rechecked after the scheduler's own tick interval rather than after the type-specific `maxRetryIntervalSeconds` policy value. This drops per-type backoff tuning for offline-machine parking and should be confirmed intentional.
The flagged code:
```
Sweep sweep = properties.getSweep();
long recheckMillis = sweep.getInterval();
Instant recheckAt = now.plusMillis(recheckMillis);
Instant dueAt = earliest(windowEnd, recheckAt);
String id = delivery.getId();
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 40 — react 👍/👎 to teach the reviewer
| @Override | ||
| public boolean park(String id, Set<DeliveryStatus> from, Instant dispatchedAt, Instant dueAt) { | ||
| Update update = new Update() | ||
| .set(FIELD_DUE_AT, dueAt) | ||
| .set(FIELD_PARKED, true); | ||
| return updateOne(sameDispatch(id, from, dispatchedAt), update); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean markAcked(String id, Set<DeliveryStatus> from, Instant ackedAt, Instant dueAt) { | ||
| public boolean markAcked(String id, String dispatchId, Set<DeliveryStatus> from, Instant ackedAt, Instant dueAt) { | ||
| Update update = new Update() | ||
| .set(FIELD_STATUS, DeliveryStatus.ACKED) | ||
| .set(FIELD_ACKED_AT, ackedAt) | ||
| .set(FIELD_DUE_AT, dueAt) | ||
| .set(FIELD_PARKED, false); | ||
| return updateOne(stillIn(id, from), update); | ||
| .set(FIELD_DUE_AT, dueAt); | ||
| return updateOne(thisDispatch(id, from, dispatchId), update); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean markDone(String id, Set<DeliveryStatus> from, Instant finishedAt, Instant expiresAt) { | ||
| public boolean markDone(String id, String dispatchId, Set<DeliveryStatus> from, Instant finishedAt, Instant expiresAt) { | ||
| Update update = closed(DeliveryStatus.DONE, finishedAt, expiresAt); | ||
| return updateOne(stillIn(id, from), update); | ||
| return updateOne(thisDispatch(id, from, dispatchId), update); | ||
| } |
There was a problem hiding this comment.
🦩 🟠 [warn/recommended] markAcked/markDone dropped the parked/wake mechanism without replacing the 'unpark on ack' path
The old markAcked() reset 'parked' to false and updated dueAt, and a wake() query would clear parked rows for a machine that had come back online. Both park, wake, and the parked field are removed in this diff, but nothing in the new markAcked/markDone/markFailed logic replaces the 'wake all parked deliveries for a machine' behavior — deliveries that were parked due to an offline machine now have no mechanism shown in this diff to be resumed when the machine reconnects, unless replaced elsewhere not shown in the diff. If the offline/reconnect handling was intentionally moved to a different mechanism (e.g., no longer parking but always retrying via findDue), that's fine, but if DeliveryOfflineBehavior/reconnect window semantics still depend on 'parked' state anywhere else in the codebase, this removal could silently break the reconnect-driven redelivery flow.
Evidence
@Override
public boolean markAcked(String id, String dispatchId, Set<DeliveryStatus> from, Instant ackedAt, Instant dueAt) {
Update update = new Update()
.set(FIELD_STATUS, DeliveryStatus.ACKED)
.set(FIELD_ACKED_AT, ackedAt)
.set(FIELD_DUE_AT, dueAt);
return updateOne(thisDispatch(id, from, dispatchId), update);
}
@Override
public boolean markDone(String id, String dispatchId, Set<DeliveryStatus> from, Instant finishedAt, Instant expiresAt) {
Update update = closed(DeliveryStatus.DONE, finishedAt, expiresAt);
🤖 Prompt for AI agents
In openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImpl.java around lines 93-106, address this code-review finding: markAcked/markDone dropped the parked/wake mechanism without replacing the 'unpark on ack' path.
The old markAcked() reset 'parked' to false and updated dueAt, and a wake() query would clear parked rows for a machine that had come back online. Both `park`, `wake`, and the `parked` field are removed in this diff, but nothing in the new markAcked/markDone/markFailed logic replaces the 'wake all parked deliveries for a machine' behavior — deliveries that were parked due to an offline machine now have no mechanism shown in this diff to be resumed when the machine reconnects, unless replaced elsewhere not shown in the diff. If the offline/reconnect handling was intentionally moved to a different mechanism (e.g., no longer parking but always retrying via findDue), that's fine, but if `DeliveryOfflineBehavior`/reconnect window semantics still depend on 'parked' state anywhere else in the codebase, this removal could silently break the reconnect-driven redelivery flow.
The flagged code:
```
@Override
public boolean markAcked(String id, String dispatchId, Set<DeliveryStatus> from, Instant ackedAt, Instant dueAt) {
Update update = new Update()
.set(FIELD_STATUS, DeliveryStatus.ACKED)
.set(FIELD_ACKED_AT, ackedAt)
.set(FIELD_DUE_AT, dueAt);
return updateOne(thisDispatch(id, from, dispatchId), update);
}
@Override
public boolean markDone(String id, String dispatchId, Set<DeliveryStatus> from, Instant finishedAt, Instant expiresAt) {
Update update = closed(DeliveryStatus.DONE, finishedAt, expiresAt);
return updateOne(thisDispatch(id, from, dispatchId), update);
}
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 30 — react 👍/👎 to teach the reviewer
| @Override | ||
| public long wake(String machineId, Set<DeliveryStatus> from, Instant dueAt) { | ||
| Criteria parkedRowsOfMachine = Criteria.where(FIELD_MACHINE_ID).is(machineId) | ||
| .and(FIELD_STATUS).in(from) | ||
| .and(FIELD_PARKED).is(true); | ||
| Query query = new Query(parkedRowsOfMachine); | ||
| Update update = new Update() | ||
| .set(FIELD_DUE_AT, dueAt) | ||
| .set(FIELD_PARKED, false); | ||
| UpdateResult result = mongoTemplate.updateMulti(query, update, MachineDelivery.class); | ||
| return result.getModifiedCount(); | ||
| public boolean markFailed(String id, String dispatchId, Set<DeliveryStatus> from, DeliveryFailure failure, String error, Instant finishedAt, Instant expiresAt) { | ||
| Update update = closed(DeliveryStatus.FAILED, finishedAt, expiresAt) | ||
| .set(FIELD_FAILURE, failure) | ||
| .set(FIELD_ERROR, error); | ||
| return updateOne(thisDispatch(id, from, dispatchId), update); | ||
| } |
There was a problem hiding this comment.
🦩 🟠 [warn/recommended] markFailed no longer clears payloadJson but test asserts $unset
The test markFailed_openRowOfThisDispatch_agentErrorWrittenPayloadDropped asserts that the update object contains "$unset" (implying payloadJson is dropped on failure), but the markFailed implementation only sets FIELD_FAILURE and FIELD_ERROR on top of closed(...). Unless closed() (unchanged in this diff, not shown) already unsets payloadJson, this test will fail, or worse, the payload is being retained un-intentionally when it should be dropped after a terminal failure. Verify that closed() includes an $unset for payloadJson, otherwise the implementation does not match its own test's stated intent.
Evidence
@Override
public boolean markFailed(String id, String dispatchId, Set<DeliveryStatus> from, DeliveryFailure failure, String error, Instant finishedAt, Instant expiresAt) {
Update update = closed(DeliveryStatus.FAILED, finishedAt, expiresAt)
.set(FIELD_FAILURE, failure)
.set(FIELD_ERROR, error);
return updateOne(thisDispatch(id, from, dispatchId), update);
}
🤖 Prompt for AI agents
In openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/delivery/impl/CustomMachineDeliveryRepositoryImpl.java around lines 127-133, address this code-review finding: markFailed no longer clears payloadJson but test asserts $unset.
The test markFailed_openRowOfThisDispatch_agentErrorWrittenPayloadDropped asserts that the update object contains "$unset" (implying payloadJson is dropped on failure), but the markFailed implementation only sets FIELD_FAILURE and FIELD_ERROR on top of `closed(...)`. Unless `closed()` (unchanged in this diff, not shown) already unsets payloadJson, this test will fail, or worse, the payload is being retained un-intentionally when it should be dropped after a terminal failure. Verify that `closed()` includes an $unset for payloadJson, otherwise the implementation does not match its own test's stated intent.
The flagged code:
```
@Override
public boolean markFailed(String id, String dispatchId, Set<DeliveryStatus> from, DeliveryFailure failure, String error, Instant finishedAt, Instant expiresAt) {
Update update = closed(DeliveryStatus.FAILED, finishedAt, expiresAt)
.set(FIELD_FAILURE, failure)
.set(FIELD_ERROR, error);
return updateOne(thisDispatch(id, from, dispatchId), update);
}
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 25 — react 👍/👎 to teach the reviewer
1 participant
Temporary: the #2212 commits on top of main right before #2356 (ticket status rework the tenant has not adopted yet), only to get a PR-suffixed snapshot for the feature tenant test in openframe-saas-tenant#3330. Will be closed after the test; review happens on #2212.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SnaktPhqs3aeuuUHdXCAjX