Repository navigation
Corporate-action guard for overnight paper holds (audit gap #8) - #222
Merged
seathatflowsinourveins merged 12 commits intoSep 25, 2026
Merged
Conversation
held_overnight trials could hold a position across a session boundary without any adjustment for a split, reverse split, cash/stock dividend, spin-off, merger or name change landing in that window. corporate_actions.py adds a fail-closed guard: entries are blocked and an existing position is force-flattened (ranked above the D5 gap-risk stop, below force_exit/STOP) for any symbol whose corporate action falls inside the possible overnight hold horizon, or whose Alpaca CorporateActionsClient lookup failed or was ambiguous. Wired into native_strategy.AdaptiveStrategy.rebalance() and runner.py's paper trial loop via the existing --env-file credential path; decisions are recorded in the paper/live event stream without values. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…split, AAPL dividend, TSLA none) AlpacaCorporateActionsSource.fetch returned NVDA forward_split 2024-06-10 and AAPL cash_dividend 2024-08-12 (both the correct ex-dates) and nothing for TSLA in its window, using this host's dedicated paper keys through runner.credentials. Read-only; no orders. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…review findings Merges origin/main (which carried #198's order-contract boundary and its own runner.py/source-hashes.json changes) into the paper-lane corporate-action guard branch, resolving the runner.py/source-hashes.json/ manifests/evidence.json conflicts, and folds in the independent review's fix round for the guard itself: - CRITICAL (1): widen the corporate-actions fetch window to anchor-7d.. anchor+90d (Alpaca filters by process/payable date, not the ex_date this guard checks) and filter locally in evaluate_guard; treat a result at the page/limit cap as ambiguous; native re-check recorded in corporate-actions-native-check-20260924.json. - HIGH (2): run the corporate-action refresh off the asyncio event loop in a worker thread, bounded by wait_for and an in-flight flag; pin a request timeout and cut retries on the underlying alpaca-py client; do the first fetch before run_native's tick loop starts. - HIGH (3): a failed/stale refresh no longer overwrites a previously confirmed action with "lookup failed" -- the last good result is kept, with needs_attention added, so a confirmed flatten is never cancelled by a later failure. - MEDIUM (4): evaluate_guard's date check is now an inclusive today<=action_date<=next_session_date range, catching a weekend/holiday governing date. - MEDIUM (5): the run-outcome corporate-action guard summary (pending_must_flatten) is read by _honest_overnight_hold, so a symbol still flagged must_flatten at the run boundary forces needs_attention/ recovery instead of a false held_overnight. - MEDIUM (6): added the 2027 NYSE holiday/early-close calendar to sessions.py; a session-calendar failure now blocks-and-flags only, never force-flattens every holding. - MEDIUM (7): fixed silent pass-throughs -- an omitted symbol defaults to lookup-failed; the refresh throttle is now keyed on the fetch window too; an unmapped corporate-action type fails closed instead of being dropped; unit-split alternate_symbol is mapped. - MEDIUM (8): the guard is wired into the strategy only when overnight_holds is enabled; failures retry on a short ~60s interval instead of the full refresh interval; a guard summary is added to the run outcome. - LOW (9): a symbol flagged by both the corporate-action guard and the gap-risk stop now gets exactly one sell action. - LOW (10): a resting buy order for a symbol newly blocked this tick is cancelled. - NIT (11): the decision event only carries the corporate_action_* keys when the guard is actually wired in (restores the byte-identical-when-off claim); a guard event is only re-emitted when the decision changes. Adds targeted tests for each finding (corporate_actions.py's CorporateActionMonitor/evaluate_guard/AlpacaCorporateActionsSource, native_strategy.py's rebalance() wiring, and runner-level tests of the fetch window, wiring decision, and threaded/bounded refresh scheduling). Recomputed blueprints/us-equities/adaptive-paper/source-hashes.json and the matching manifests/evidence.json entries programmatically from the changed files' actual on-disk content. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…nge test hermetic - sessions.py: HOLIDAYS_2027/EARLY_CLOSES_2027 now cite the NYSE hours-calendars page (fetched 2026-09-24; lists 2026-2028) and the same-day exchange_calendars 4.13.2 XNYS cross-check, which agrees exactly (10 weekday closures, early close 2027-11-26). exchange_calendars_agrees(year) generalizes the 2026 check with padded bounds (Jan 1 is not a session). - tests: add the 2027 agreement test; test_year_outside_calendar_refused now disables the exchange_calendars fallback, since CI installs that package and the test previously passed only where it was absent. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…guard branch Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Both independent reviews (Claude/rev-ca2 and Codex) found round 2 "not ready". Addresses every item: - HIGH (1): overlapping refreshes could let an older, slower attempt overwrite a newer confirmed action. CorporateActionMonitor now tracks a generation counter under a lock; refresh_timed_out() (called on a wait_for timeout) bumps it and treats the timeout as an immediate failure (short retry_seconds interval); _apply_success/_apply_failure drop any write from a superseded generation. The preflight fetch and tick-loop refresh now share one ca_refresh_state dict (in_flight + tasks), and run_native's shutdown path (_shutdown_corporate_action_tasks) cancels/joins every still-pending refresh task. - HIGH (2): the SDK's CorporateActionsSet parser (13 branches, no else/default) silently dropped reorganization/partial_call/capital_ gains_distribution buckets before this wrapper ever saw them, evading both unknown-type rejection and the cap check. AlpacaCorporateActionsSource now constructs with raw_data=True and parses the raw response directly via the private _get_marketdata, so every bucket (mapped or not) reaches this wrapper's own fail-closed logic. New tests fake the HTTP transport (not a pre-parsed response) so the real SDK code path runs. - HIGH (3): the request now asks data_quality=all (the documented default, complete, excludes incomplete records); a record missing its governing date marks only its own symbol(s) LOOKUP_AMBIGUOUS, not the whole fetch. - MEDIUM (4): the guard's summary now tracks pending_needs_attention_held (every currently-held symbol not positively verified clear); _honest_overnight_hold refuses held_overnight while it is non-empty, not just while pending_must_flatten is non-empty. - MEDIUM (5): the guard now evaluates every symbol with a still-resting pending BUY order, not just decision.targets.keys(), so a corporate action discovered after a signal disappears still cancels that resting buy. The fake guard test fixture now filters to the requested held|candidate set instead of echoing every scripted decision. - MEDIUM (6): the 90-day forward pad is now explicitly documented as an evidence-backed assumption (3 recorded lag observations, sourced), not a proven bound. - LOW (7): a symbol omitted from an otherwise-successful fetch is now degraded (last good record preserved), never overwritten with LOOKUP_FAILED. - LOW (8): corporate-actions-native-check-20260924.json is corrected (the 2024 window is explicitly labeled a manual approximation since sessions.py has no 2024 calendar; the real _corporate_action_fetch_window output is recorded separately; the AAPL dividend's lag is corrected to the measured 3 days, replacing the unmeasured "1-3 weeks" claim) and registered in manifests/evidence.json. - NIT (9): the "bounded fetch"/pagination comments are corrected (a requests timeout bounds each read, not the whole paginated call; the SDK does paginate); a symbol that was previously flagged and resolves now emits an explicit "cleared" event. - NIT (10): the session test's stale "CI installs that package" comment is corrected. Re-ran the review's own mutate_rev.py unmodified (just retargeted): all 24 mutations now kill (22 directly; F2b/F5a/F5b's exact pre-fix text no longer exists after this round's refactor into named, independently testable functions -- their equivalents are covered by this round's own mutation probe, also all killed). Added a matching native re-check (raw-record lag query, data_quality=all) using the same credential-loading recipe as before. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… found
Both independent reviews confirmed round 3 mostly holds (all 24 earlier
mutations still killed) but independently found the same new regression:
- HIGH: ambiguity erased a confirmed action. AlpacaCorporateActionsSource.
fetch() used to overwrite a symbol's whole record list with
LOOKUP_AMBIGUOUS whenever ANY row for that symbol lacked a governing
date, discarding valid records collected in the SAME response (e.g. a
confirmed split plus an undated dividend for the same symbol).
CorporateActionMonitor._apply_success also overwrote a previously
confirmed record with a later per-symbol LOOKUP_AMBIGUOUS/LOOKUP_FAILED
result. Fixed at both layers: fetch() only ever falls back to the
sentinel for a symbol with ZERO valid records of its own in that
response; _apply_success degrades (keeping the last good record)
instead of overwriting whenever the new per-symbol result is a
sentinel.
- MEDIUM: raw rows with no identifiable symbol used to silently count as
clear. A row (mapped or unmapped bucket) naming no symbol at all now
marks every requested symbol ambiguous.
- MEDIUM: uncertainty discovered after the final rebalance() tick could
still end held_overnight. The outcome's corporate_action_guard summary
is now recomputed fresh (_final_corporate_action_guard_summary) against
the guard's current evaluate() and the truly final held positions,
immediately before the outcome is built, instead of trusting the
strategy's last-tick snapshot.
- LOW: shutdown errors could bypass refresh-task cleanup -- session.stop()/
port.stop()/the task wait are now in their own nested try, with
_shutdown_corporate_action_tasks in its own guaranteed finally.
- LOW: the private _get_marketdata call is replaced with the public
client.get() and this wrapper's own explicit next_page_token pagination
(matching blueprints/us-equities/alpaca-historical's own bridge pattern
for this endpoint), with region=us and a page-count ceiling that fails
closed if a continuation token survives it.
- LOW: one unmapped-type row used to fail the fetch for every requested
symbol. An unmapped bucket now degrades only the symbol(s) its own rows
actually name (Alpaca API reference, corporateactions-1, fetched
2026-09-24: partial_call/reorganization/capital_gains_distribution
documented as additional types; data_quality accepts complete/all).
- LOW (measured): a hung refresh thread used to delay run_native's own
result-save/account-lock release by the hang's full duration, because
asyncio.run()'s cleanup joins every asyncio.to_thread/default-executor
thread. The worker now runs on a daemon thread bridged back via
loop.call_soon_threadsafe; asyncio.run() never waits for it (measured:
a 4s hang delayed exit by ~4s before, ~0.1s after).
- NIT: a timeout processed immediately after a successful commit for the
same generation is now ignored (a new _committed_generation check),
instead of degrading an already-fresh result for the retry interval.
- Wording: corrected the "never overlapped"/"bounded fetch" claims to
state exactly what actually bounds each thing, and the native-check
record's "2026 is the only calendar year" claim (2027 is also covered).
Adds targeted tests for every finding above, including tests that go
through the real HTTP-mocked SDK pagination path (a two-page response
with a real next_page_token, and the cap enforced across pages) and a
real run_native drive proving the shutdown path actually cancels a
hanging task and invalidates it via refresh_timed_out.
Re-ran the review's own mutate_rev3.py (retargeted, unmodified
mutations): the two mutations flagged as regressions this round (G9, G10)
and G5 are now killed. G6 ("run_native never shuts down refresh tasks")
still survives -- verified redundant, not a real gap: run_native is only
ever invoked via asyncio.run() (both directly and through main()'s own
asyncio.run(execute())), and asyncio.run()'s own Runner.close() calls
_cancel_all_tasks() before shutting down the loop, cancelling every
outstanding task regardless of this module's own explicit cleanup. The
explicit _shutdown_corporate_action_tasks call remains valuable for the
exception-path case (finding, LOW #7, now fixed via the nested finally)
and for a hypothetical caller that reuses an already-running loop, not for
this survivor's literal scenario.
Recomputed source-hashes.json and the matching manifests/evidence.json
entries programmatically from the changed files' actual on-disk content;
registered the native-check record's own row updates.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…cancellation race, fix the network-leaking pagination test Both independent reviews converged on one regression: fetch() collapsed per-symbol uncertainty into a LOOKUP_AMBIGUOUS sentinel that only applied when a symbol had zero valid records, so a benchmark symbol's routine dated dividend (SPY/QQQ/IWM/DIA almost always have one in the 97-day window) silently hid an unrelated undated/unmapped/symbol-less row for that same symbol. AlpacaCorporateActionsSource.fetch() now returns (records, ambiguous_symbols) as two independent values; CorporateAction Monitor.complete_attempt keeps the records and separately flags the symbol degraded when it is also ambiguous, so a confirmed action still flattens while residual uncertainty still raises needs_attention. Codex additionally found a genuine cancellation race: the monitor's generation was allocated lazily inside refresh() itself, which only ran once a launched worker thread actually started executing -- a thread still merely queued by the OS at shutdown time was invisible to the generation-based invalidation check, letting a delayed worker commit a stale result after shutdown. CorporateActionMonitor now exposes two- phase begin_attempt/run_attempt/fail_attempt/invalidate_attempt/ refresh_timed_out primitives; the generation is allocated on the calling coroutine before the worker thread is even created. Cancellation now calls the non-degrading invalidate_attempt instead of the degrading refresh_timed_out, so a routine in-flight refresh cancelled at shutdown no longer forces an otherwise-clean cached result into needs_attention. Both reviews also found the page-ceiling pagination test constructed the real AlpacaCorporateActionsSource before entering its own HTTP patch context, so it silently issued ~58 real requests to Alpaca's servers and passed on the resulting 401s without ever exercising its own scripted fixture; the source is now built inside the patch, the exact page count served is asserted, and both corporate-action test modules gained a module-level real-network guard. Also: an empty-string next_page_token no longer counts as a continuation; Thread.start() now sits inside the try/finally that resets in_flight; the "prevent overlap" comment was corrected to "reject stale writes"; the shutdown/final-summary/hold tests were strengthened so they fail on the exact regressions they target rather than passing on asyncio.run()'s own unrelated auto-cancellation or an in-memory stale snapshot. New tests cover region=us/data_quality=all on every page, the worker thread being a daemon, in_flight recovering from a Thread.start() failure, and a run_native-driven end-to-end refusal of held_overnight when a refresh degrades after the final rebalance tick. Re-ran: the full adaptive suite (368 tests, network-blocked pagination tests), test_native_faults_min, test_order_throughput, test_order_contract, test_trading_gates, all six validators (validate.py, validate_catalogs.py, landscape.py, evidence_manifest.py, verdict_review_gate.py, trading_gates.py), and both reviewers' mutation scripts retargeted at this worktree -- all 54 reviewer mutations plus 8 new round-5 mutations killed (two long-documented redundant survivors from round 4 unchanged: asyncio.run()'s own task cancellation already covers G6's exact scenario). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…resolve every allocated generation, and harden the warm-monitor preflight Four low-severity items from the final independent review, plus the test gaps two of them exposed: 1. An ambiguous-only response (a previously confirmed record whose row loses its governing date on a later fetch) wrote an empty list over the last good records in complete_attempt, turning a confirmed must_flatten back into mere block-and-flag and losing an already-live intraday exit signal. Fixed: an ambiguous symbol with zero valid records in the new response now degrades (preserving the last good records) instead of overwriting them with []. 2. Mutation G4 (drop the scheduler's refresh_timed_out call for a two-phase guard) survived because every existing short-timeout test drove a legacy single-arg guard, never the real CorporateActionMonitor's own generation-aware timeout path. Added a test driving the real monitor through a short-timeout scenario, asserting the symbol is flagged and the next attempt is allowed after retry_seconds (60s), not throttled for the ordinary refresh_seconds (900s). 3. A pre-allocated generation was left permanently unresolved if Thread.start() itself raised, or if an unexpected exception escaped the worker's own run_attempt call -- refreshes would then be silently throttled with nothing ever flagged for up to 900s. Both paths now call fail_attempt on the pre-allocated generation before propagating. 4. _preflight_corporate_action_refresh unconditionally awaited _schedule_corporate_action_refresh's return value, which is None (not a task) when the two-phase begin_attempt itself reports a throttled no-op -- a second preflight call on an already-warmed monitor raised TypeError from awaiting None. Now only awaits when a task was actually created. The review's own mutation script also caught two gaps in the round-5 fixes themselves once the two-phase API was exercised more directly: G5 (a cancellation mutated to a no-op) survived because the existing test only checked that a clean end state stayed clean, which held regardless of whether invalidate_attempt ran; strengthened to assert the generation counter is actually bumped past the cancelled attempt. A generation- specific refresh_timed_out check reverted to a round-4-era coarse heuristic (comparing only _generation to _committed_generation) also survived; added a test where a stale timeout for an already-superseded (but not yet committed) generation must be a complete no-op. Merged origin/main (added #206 and #218 since the round-5 push); source-hashes.json and manifests/evidence.json were recomputed from current file content for every changed path, not resolved by picking a side. No overlap between main's changes and this branch's files beyond manifests/evidence.json, which merged cleanly. Re-ran: the full adaptive suite (373 tests) with network-blocked pagination tests, test_native_faults_min, test_order_throughput, test_order_contract, test_trading_gates, all six validators (validate.py, validate_catalogs.py, landscape.py, evidence_manifest.py, verdict_review_gate.py, trading_gates.py -- all passed), and the reviewer's own mutate_rev5.py retargeted at this worktree (65 mutations, all killed except one PATTERN_COUNT=0 entry already superseded by an equivalent round-5 mutation of my own). Ran gitleaks git history scan (509 commits, 1.12 GB) with the pinned binary and repo config: all 76,054 findings are pre-existing sourcegraph-access-token/generic-api-key pattern matches inside the generated docs/ecosystem/index.html artifact; zero findings in any file this branch touches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ction guard branch - runner.py: kept both import lines (corporate_actions, credential_guard); the two changes touch different regions. - source-hashes.json: took the union of both sides' keys and recomputed every digest from the merged tree. The #219 manifest test passes. - evidence.json: rebuilt from main's version. Re-registered this branch's native-check record, registered corporate_actions.py and its tests, and refreshed the six stale digests surgically. Checks, with the network blocked: 986 adaptive tests OK; native-faults, order-contract and credential-tools 56 OK; order-throughput, trading-gates, evidence-manifest and validate 163 OK; all six validators pass. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…azily CI's validate jobs run the suite without alpaca-py, and 12 runner tests failed with ModuleNotFoundError: main() built AlpacaCorporateActionsSource, and so imported alpaca-py's data client, on every run, before its gate checks. That also contradicted the design (the guard acts only with overnight holds). - runner.main builds the monitor only when session_policy["overnight_holds"] is enabled; otherwise the guard is None. - AlpacaCorporateActionsSource imports and builds its alpaca-py client on first use (the _client property). A refused run, or an environment without alpaca-py, never needs the SDK. The session timeout and retry setup are unchanged and are applied when the client is built. Checks: CI-like system python gives 363 OK (runner, corporate actions, sessions). The pinned runtime with the network blocked gives 248 OK (corporate actions, runner) and 986 in the full suite before the manifest refresh. The manifest test and the six validators pass. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
seathatflowsinourveins
deleted the
claude/paper-corporate-action-guard-20260924
branch
September 25, 2026 01:33
seathatflowsinourveins
pushed a commit
that referenced
this pull request
Sep 25, 2026
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
seathatflowsinourveins
added a commit
that referenced
this pull request
Sep 25, 2026
…-09-24/25 merges (#225) Updates four checkpoint entries: the paper lane, the simulation lane, alpaca-paper-operational and corporate-action-data. They now reflect #198, #199, #212, #218, #219 and #222. tests.test_grand_dashboard passes, and all state strings satisfy progress.py's public-token rule. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
seathatflowsinourveins
pushed a commit
that referenced
this pull request
Sep 25, 2026
…porate-action and credential guards Main moved 19 commits since 5ed8538. The conflicts were resolved by keeping both sides: - native_strategy.py: AdaptiveStrategy takes both halted= (E4) and corporate_action_guard= (#222). The constructor keeps the halt and callback-fault state and the corporate-action guard state. cancel_expired keeps the halted-exit skip (E4) and gains #222's symbols= immediate-cancel set. - runner.py: the strategy gets both halted=controller.is_halted and the corporate-action guard. The shutdown runs stop_halt_seed() inside #222's try/finally, so the corporate-action refresh cleanup still runs if the halt-seed stop raises. - source-hashes.json: the union of both sides' keys, every digest recomputed from the merged tree. - manifests/evidence.json: main's registry, plus the branch's new engine-nautilus probe, with all stale digests refreshed. The branch's entries for the renamed CI lock (#233) and the renamed grype fixture (#224) are dropped. Checks on the merged tree: - pinned rc5 runtime, 23 paper modules: 1,320 tests OK (2 skipped); - full python3 -m unittest: 4,944 tests OK (600 skipped); - validate.py, evidence_manifest.py --check, and the source-hashes manifest test: pass. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
This adds a corporate-action guard for adaptive paper runs that hold positions overnight (trading-lane audit gap #8). It is wired in only when
session_policy.overnight_holdsis enabled, and no shipped config enables that today. For each held, targeted or pending-buy symbol it does the following:needs_attentionso recovery takes over, when any held symbol could not be verified clear.Source: Alpaca
GET /v1/corporate-actions, called through the public alpaca-pyget()with our ownnext_page_tokenpagination and a page ceiling.region=usanddata_quality=all, which includes incomplete records.Runtime.
Native read-only check (
corporate-actions-native-check-20260924.json, market data only, no orders):Review
Six rounds.
data_qualitydefault, a test that was really calling Alpaca, and more.Codex limit. The Codex CLI hit its usage limit (it resets 2026-09-30 23:50), so rounds 5–6 had only the independent Claude review. Its final verdict at 65ee7fb was ready. The one surviving mutant is an untested worker-exception branch that only runs if the monitor itself raises.
Checks (merged tree, a3b9a23, network blocked)
Tests:
Validators: all six pass.
Secret scans: the gitleaks history and working-tree scans are clean.
Not covered
run_nativewiring with fake ports) plus the one native read-only source check.🤖 Generated with Claude Code