Skip to content

feat: add TQQQ present package output root gate#45

Closed
Pigbibi wants to merge 2 commits into
mainfrom
codex/t2b2-qsp-output-root-v2-20260720
Closed

feat: add TQQQ present package output root gate#45
Pigbibi wants to merge 2 commits into
mainfrom
codex/t2b2-qsp-output-root-v2-20260720

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Scope

Adds only the frozen T2B2 QSP PRESENT package producer boundary and its focused tests.

  • Canonicalizes and admits output roots only when outside the checkout or Git proves an in-checkout directory ignored before side effects.
  • Preserves strict empty source-status checks without private-stage or prior-package allowlists.
  • Rejects symlink, non-directory, Git-admin, unignored, and prepublication-drift roots before final publication.

Tests-first evidence

  • RED (test-only delta): the new focused suite initially collected with ModuleNotFoundError because the new module was absent.
  • GREEN: uv run --extra test pytest -q tests/test_tqqq_market_regime_control_present.py12 passed.
  • GREEN: uv run --extra test pytest -q tests/test_strategy_plugin_runner.py tests/test_tqqq_market_regime_control_present.py47 passed.

Validation

The acceptance contract corrected its pytest invocations to the frozen module form. The newly required full command was run exactly once:

  • uv run --frozen --extra test python -m pytest -q105 passed in 9.11s.
  • uv run --extra test ruff check src/quant_strategy_plugins/tqqq_market_regime_control_present.py tests/test_tqqq_market_regime_control_present.py — passed.
  • uv run --extra test python -m compileall -q src/quant_strategy_plugins/tqqq_market_regime_control_present.py — passed.
  • git diff --check — passed before staging.

No real package artifact was generated. File scope is exactly the two authorized paths.

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: 9aa858a472

ℹ️ 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 thread src/quant_strategy_plugins/tqqq_market_regime_control_present.py Outdated
Comment thread src/quant_strategy_plugins/tqqq_market_regime_control_present.py
Comment thread src/quant_strategy_plugins/tqqq_market_regime_control_present.py
@github-actions

github-actions Bot commented Jul 19, 2026

Copy link
Copy Markdown

🤖 Codex PR Review

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

🚫 Blocking Issues

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

1. 🟠 [HIGH] Security in src/quant_strategy_plugins/tqqq_market_regime_control_present.py

_sensitive_key() only rejects a small substring blacklist (token, secret, password, cookie, jwt, api_key). Any credential stored under another common name such as auth, authorization, credentials, or bearer passes validation, and _package_config() then serializes that value into config.value for the published artifact. Because main() accepts an arbitrary TOML config and this PR writes the package to disk, a reachable caller can leak production secrets simply by using an unlisted key name. (line 213)

Suggestion: Do not publish arbitrary flattened config keys. Replace the blacklist with a positive allowlist of fields that are safe to package, or drop the settings subtree entirely from config.value unless each key is explicitly approved.

ℹ️ Other Findings

1. 🟡 [MEDIUM] Logic in src/quant_strategy_plugins/tqqq_market_regime_control_present.py

A falsey but present external_context value (for example "") is treated as ABSENT here, but the original key is still preserved in selected and later copied into both runner_config and config.value. That means the plugin still receives external_context="", while the published package simultaneously says inputs.external_context.status == "ABSENT" and config.value.external_context == "@input:external_context". This reachable input produces self-contradictory package metadata and can also make the producer call the plugin with an invalid path. (line 252)

Suggestion: Normalize external_context once after parsing: either reject falsey-but-present values as invalid input, or remove the key from selected whenever it is treated as absent before building runner_config and the package config.

2. 🟡 [MEDIUM] Reliability in src/quant_strategy_plugins/tqqq_market_regime_control_present.py

The staging directory is only deleted when published is false, so every successful run leaves .tqqq-market-regime-control-present-* behind under the output root. Those directories still contain the copied prices.csv and optional external_context.csv, so repeated successful publishes leak extra input copies and accumulate disk usage indefinitely. (line 405)

Suggestion: Clean up stage after a successful os.replace(), or use a temporary-directory helper that always removes the staging tree once the final package has been moved into place.


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: 72a876cf73

ℹ️ 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".

return final, digest
except PresentError:
raise
except (OSError, ValueError, subprocess.SubprocessError):

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 Convert malformed runner settings into a producer error

A valid TOML configuration can supply a non-scalar runner setting such as taco_opportunity_size_scalar = []; _select_config admits it, and run_market_regime_control_plugin eventually calls float() on that value. That raises TypeError, but this handler catches only OSError, ValueError, and SubprocessError, so main emits an uncaught traceback and exits 1 rather than returning a controlled PresentError/producer error code. Catch TypeError here (or validate runner-facing settings before invoking it).

Useful? React with 👍 / 👎.

"log_record", "mode", "notification", "plugin", "position_control", "profile", "schema_version",
"strategy", "strategy_policy", "suggested_action", "target_type", "would_trade_if_enabled",
}
SENSITIVE_KEY_PARTS = ("token", "secret", "password", "cookie", "jwt", "api_key")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Reject private-key credential settings before packaging

Even with recursive traversal, the denylist does not recognize common credential names such as private_key, access_key, or credential: for example, [strategy_plugins.settings.broker] private_key = "..." passes _contains_sensitive_key and is serialized verbatim by _package_config into the published package. This leaks broker/service credentials whenever those conventional TOML names are used; expand the check or, preferably, allowlist the packageable configuration fields.

Useful? React with 👍 / 👎.

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