Repository navigation
ops(OMN-18415): let the rolling fleet deploy come back for a busy runner - #3618
Conversation
A runner busy through every retry pass is reported and left alone, which is correct. Without a way to return to it, clearing that residual meant a hand-typed compose call, i.e. the exact recipe this mode exists to replace. --only=NAME converges one named fleet service and fails closed on a name the compose file does not declare, because rolling nothing and printing a completed count reads exactly like a converged fleet.
|
| Surface | Meaning | Blocks merge? |
|---|---|---|
| Review threads | Per-finding, posted by the reviewer | No (informational) |
Hostile Review Thread Gate |
Deterministic: unresolved hostile-reviewer threads exist | Fails until resolved (not yet a required context) |
degraded verdict |
Fewer than 2 models succeeded (infra) | No |
Powered by omniintelligence.review_pairing.cli_review — multi-model adversarial review: qwen3-review, qwen3-review-b, glm-review (OMN-8468/OMN-8524/OMN-17492)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: glm-review
Models failed: codex
New finding threads: 1
Deduped (already posted on this PR): 0
Nit-level findings suppressed: 1
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Findings not anchored to a changed file
-
[MAJOR] hostile-reviewer (glm-review)
Fallback after err() assumes err exits | The guard relies on err() terminating the script; otherwise execution falls through and pending is overwritten with the unknown ROLL_ONLY anyway, recreating a nonexistent service (or matching nothing and reporting 0/1, the exact ambiguity the guard exists to prevent). The diff does not show err's definition; if err is print-and-return, this is a silent logic inversion of the fail-closed intent. Even if err does exit today, the coupling is implicit and undocumented. |
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
No handling of empty pending before --only filtering | If the compose services list is empty (or all services were already excluded upstream of this hunk), the found=false loop correctly errors, but the error message claims the name is 'not a fleet service declared in COMPOSE_FILE' when the actual problem is an empty or unreadable service list. Diagnostics misdirect the operator. Additionally, --only combined with --limit is undefined: --limit=0 with --only is harmless (single target), but no documentation
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Short usage string not updated for --only | The header comment and the long --help branch both document --only, but the concise usage line inside the --help case handler was not updated. The header usage line was updated; the runtime usage line was not, so the two now disagree. | Evidence: echo "Usage: $0 [--dry-run] [--skip-build] [--soft] [--rolling]" | Fix: Update the runtime usage line to include [--only=NAME].
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Unvalidated ROLL_ONLY passed into downstream commands | ROLL_ONLY is taken verbatim from argv with no character validation. Bash quoting in the diff looks correct (pending=("${ROLL_ONLY}") is a single literal element), but the value eventually flows into whatever per-runner command construction exists below the diff (ssh/compose invocations). If any downstream site is unquoted or builds command strings by interpolation, an attacker or careless operator with shell access can shape the value into additional a
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
Findings demoted from threads (anchor rejected)
-
[MAJOR] hostile-reviewer (glm-review)
New tests assert source strings, not behavior | Both new tests grep the script's own text rather than executing it. This is a tautology: the tests pass if the exact strings exist, fail on any refactoring that preserves semantics (different quoting, reordered flags, renamed local), and pass even if the logic is inverted elsewhere. They verify no behavior: not that an unknown --only aborts, not that pending is filtered, not that total is set to 1. The existing test suite presumably executes or extracts the ro
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
#9780) * evidence: OCC companion pass 1 for OmniNode-ai/omnibase_infra#3618 * evidence: OCC companion self-bind for #9780 --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai>
…3618) A runner busy through every retry pass is reported and left alone, which is correct. Without a way to return to it, clearing that residual meant a hand-typed compose call, i.e. the exact recipe this mode exists to replace. --only=NAME converges one named fleet service and fails closed on a name the compose file does not declare, because rolling nothing and printing a completed count reads exactly like a converged fleet.
OMN-18415 — the roll needs a way to come back for a busy runner
The rolling fleet deploy skips any runner executing a job and retries it in later
passes. That is the correct behaviour and it held in practice: across two full rolls of
the 60-runner fleet, one runner was executing a long job through every pass and was
reported, not killed.
But there was no way to return to it. Clearing that residual meant a hand-typed
docker compose up -d --force-recreate --no-deps <service>on the host — the exactrecipe the rolling mode exists to replace, and the one whose busy check is subtly
fail-open when typed by hand.
What changed
--only=NAMEconverges exactly one fleet service through the same path: sametwo-signal busy check, same fail-closed unknown state, same wait-for-online, same
refusal to sweep orphans.
It fails closed on a name the compose file does not declare. Rolling nothing and
printing a completed count reads exactly like a converged fleet, which is the failure
mode worth refusing.
Default is unchanged: no
--onlymeans the whole fleet.Exercised live against the runner that stayed busy: the run targeted that one service,
found it busy on both passes, refused to touch it, and reported it by name.
uv run pytest tests/unit/scripts/test_deploy_runners_rolling_mode_omn18415.py→ 17 passed, including the two new assertions and their controls.
bash -nand shellcheck clean apart from one pre-existing finding elsewhere in thefile.
Evidence-Ticket: OMN-18415
Evidence-Source: OCC#9780