Skip to content

fix(ops): make scheduler policy target-driven - #347

Closed
Pigbibi wants to merge 3 commits into
mainfrom
codex/ibkr-generic-scheduler-policy
Closed

fix(ops): make scheduler policy target-driven#347
Pigbibi wants to merge 3 commits into
mainfrom
codex/ibkr-generic-scheduler-policy

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • remove the account/service-name allowlist from scheduler planning
  • derive live defaults and per-target overrides from each runtime target's JSON scheduler policy
  • keep execution_mode=live at the existing 330s safe default while leaving paper targets unchanged unless configured
  • replace deployment-specific account fixtures with generic examples

Open-source boundary

  • no account count or service name is part of the code contract
  • target cardinality is arbitrary (0/1/N)
  • no live JSON values, account identifiers, project identifiers, tokens, or secrets are added
  • no workflow, deployment, trading, order, position, or account behavior is triggered by this PR

Verification

  • focused: 80 passed
  • ruff: passed
  • compileall: passed
  • actionlint: passed
  • diff-check: passed
  • added-line sensitive-pattern scan: passed
  • full local suite: 312 passed, 5 skipped, 11 failed; failures are in untouched existing test areas and are outside this PR's scheduler/config scope

Co-Authored-By: Codex <noreply@openai.com>
@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown

🤖 Codex PR Review

🚫 Merge blocked: 2 serious issue(s) found in high-risk files

🚫 Blocking Issues

These issues must be fixed before this PR can be merged:

1. 🟠 [HIGH] Logic in scripts/build_cloud_run_env_sync_plan.py

dry_run_only is being used as a fallback proxy for execution_mode. Those flags describe different things: a paper target can legitimately have dry_run_only=false because it still submits orders to a paper account. In that case, omitting execution_mode now makes the target look "live" and injects the 330s attempt deadline, which violates the PR contract to leave paper targets unchanged unless explicitly configured. (line 505)

Suggestion: Do not infer live/paper from dry_run_only. Only apply the default live deadline when execution_mode is explicitly present and validated as "live", or add a dedicated validated runtime field that actually represents the environment type.

2. 🟠 [HIGH] Reliability in scripts/build_cloud_run_env_sync_plan.py

After removing the hard-coded service allowlist, a target only gets the 330s safety deadline if execution_mode == "live" or dry_run_only is False. Any live target that omits both fields, or provides a non-boolean/invalid value, now silently falls through with no attempt_deadline, which regresses it to Cloud Scheduler's much shorter default and can cause premature timeout/retry behavior for long-running live jobs. (line 503)

Suggestion: Fail closed when the scheduler policy omits attempt_deadline: require a valid explicit execution_mode, or require scheduler.attempt_deadline to be set. Do not silently treat missing or malformed discriminator fields as non-live.


Review by Codex PR Review bot • PR

Co-Authored-By: Codex <noreply@openai.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b38fb7ba66

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +503 to +504
if not has_attempt_deadline and str(runtime_target.get("execution_mode") or "").strip() == "live":
attempt_deadline = DEFAULT_LIVE_RUN_ATTEMPT_DEADLINE

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Derive live deadlines from dry_run_only too

For valid runtime targets that omit execution_mode, this branch never applies the 330s default even when dry_run_only is false. The pinned QuantPlatformKit resolve_runtime_target_from_env treats execution_mode as optional and derives live/paper from dry_run_only, so a live target JSON with dry_run_only: false but no execution_mode now produces no scheduler.attempt_deadline; the workflow then emits an empty deadline and skips --attempt-deadline for the live /run scheduler, reintroducing the scheduler deadline being shorter than the 300s Cloud Run timeout. Please normalize/derive the live mode from dry_run_only as well as the explicit string.

Useful? React with 👍 / 👎.

Co-Authored-By: Codex <noreply@openai.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eebac97373

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +505 to +506
is_live = execution_mode == "live" or (
not execution_mode and type(dry_run_only) is bool and not dry_run_only

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Normalize live detection before defaulting deadlines

Fresh evidence: the pinned resolve_runtime_target_from_env accepts string booleans such as "false" for dry_run_only and compares execution_mode case-insensitively. For a valid live target that omits execution_mode but has "dry_run_only":"false" (or uses "LIVE"), this check treats the target as non-live, so the plan omits scheduler.attempt_deadline and the workflow skips --attempt-deadline for the /run scheduler even though the Cloud Run timeout is 300s.

Useful? React with 👍 / 👎.

@Pigbibi

Pigbibi commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author

PERMANENT_FREEZE: the current runtime-target contract has no single validated field that safely distinguishes live from paper for scheduler deadline defaults. dry_run_only controls execution suppression and is not an environment identity; missing/invalid execution_mode also cannot safely default. Closure is exhausted, so this PR is preserved as evidence and will not be merged or resliced. A future change requires a concrete producer-owned runtime discriminator or an explicit per-target scheduler deadline contract.

@Pigbibi Pigbibi closed this Jul 16, 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.

1 participant