feat: gex-poller — scheduled MCP extraction into timeseries.db - #2
Conversation
also fix pytest import shadowing: drop tests/poller/__init__.py (it shadowed the poller package) and add pythonpath=["."] so repo-root modules import cleanly alongside the legacy from-conftest import
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds an MCP client, response normalization, SQLite persistence, polling orchestration, market-hour scheduling, operational reporting, command-line status output, launch-agent deployment, and pytest coverage. ChangesPoller foundation
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@poller/db.py`:
- Around line 27-32: Update the put_call_ratio table definition to make the
uniqueness key include source alongside ticker and ts. Modify the UNIQUE
constraint in the CREATE TABLE statement so rows from menthorq_put_call_ratio
and spotgamma_equity_put_call_ratio with the same ticker and timestamp are
retained separately, while duplicate rows from the same source remain ignored.
- Around line 72-79: Update record_call to accept a source argument and use it
in the scrape_runs INSERT instead of the hardcoded empty string. Propagate the
real source through every record_call call site, and extend the relevant
database test to assert that per-call rows retain the supplied source.
- Around line 63-69: Update begin_cycle to retrieve the inserted row’s cycle_id
by its connection-scoped inserted-row identity, rather than querying
MAX(cycle_id) after commit. Preserve the existing insert and commit behavior
while ensuring the returned ID always belongs to this cycle.
In `@poller/normalize.py`:
- Around line 53-91: Update to_rows handling for the menthorq_gamma_levels and
menthorq_dealer_positioning branches so missing required fields ticker,
frequency, timestamp, or reference_timestamp are caught and raised as
descriptive ValueError exceptions instead of propagating KeyError. Preserve the
existing row mappings and optional-field behavior.
In `@tests/poller/test_db.py`:
- Line 5: Replace the module-level pytestmark in the poller database tests with
the integration marker, so all tests calling init_db with a SQLite file are
categorized as integration tests. Align this with the marker definitions in
pyproject.toml and remove the unit classification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f54e5504-a6d2-443f-ba58-8bdfeb25381e
📒 Files selected for processing (6)
poller/__init__.pypoller/db.pypoller/normalize.pypyproject.tomltests/poller/test_db.pytests/poller/test_normalize.py
…ll source, ValueErrors)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@poller/db.py`:
- Around line 30-31: Update the database initialization/schema setup for the
put_call_ratio table so existing databases with UNIQUE (ticker, ts) are migrated
to the new UNIQUE (ticker, ts, source) constraint, or explicitly recreate the
database under a documented policy. Ensure the migration preserves existing data
and add coverage for upgrading a database created with the previous schema.
- Around line 64-70: Update begin_cycle’s scrape_runs insert to avoid the SQLite
RETURNING clause, or explicitly enforce SQLite version >=3.35.0 before executing
it; ensure older host-linked SQLite versions do not fail before recording the
cycle.
In `@poller/normalize.py`:
- Around line 41-44: Update _req to reject required fields whose values are None
or blank strings, while preserving its existing missing-field validation. In the
dealer normalization branch, retrieve ticker through _req instead of converting
a missing ticker to an empty string. Add regression coverage for null and
missing dealer tickers, ensuring both are rejected before insertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a8f5d556-7737-414b-ae12-69a9ea7bed82
📒 Files selected for processing (4)
poller/db.pypoller/normalize.pytests/poller/test_db.pytests/poller/test_normalize.py
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@poller/mcp_client.py`:
- Around line 39-41: Update the initialization flow around the first
_rpc("initialize", ...) call to inspect its response before sending
notifications/initialized. Raise McpError when the response contains an error or
its negotiated protocolVersion differs from _PROTOCOL; only call
_rpc("notifications/initialized", is_notification=True) after validation
succeeds.
- Around line 72-87: Update McpClient._rpc to read responses until the matching
request ID is found, skipping notifications, rejecting server-initiated requests
and unexpected IDs, and preserve notification behavior; ensure tools/call is
tested against an intervening notification. Update __enter__ to validate the
initialize response, propagate initialization errors, require a supported
negotiated protocolVersion, and send notifications/initialized only after
validation.
In `@poller/observe.py`:
- Around line 40-42: Update the osascript invocation in the poller flow to pass
a finite timeout, and catch subprocess.TimeoutExpired alongside OSError so a
blocked command is handled without stopping scheduled polling.
- Around line 37-40: Update notify_macos so title and message are not
interpolated into AppleScript source: use a constant on run argv handler and
pass both values as separate osascript arguments, preserving notification
behavior. Extend the notify_macos tests with quotes and AppleScript syntax in
both values, verifying they are passed as arguments rather than embedded in the
script.
- Around line 47-50: Update the cycle query in the observe logic around
conn.execute to compare timestamps using julianday(started_at) >=
julianday('now', '-1 day') instead of text datetime comparison, ensuring the
24-hour cutoff works with _utcnow()’s T separator and +00:00 offset. Add a test
covering the exact cutoff boundary and confirming records older than 24 hours
are excluded.
In `@poller/schedule.py`:
- Around line 5-13: Update poller/schedule.py lines 5-13 in in_market_window to
convert now to America/New_York, then evaluate the local weekday and 09:30 <=
time < 16:00 window so daylight-saving and standard-time offsets are handled
correctly. Update tests/poller/test_schedule.py lines 10-25 to add standard-time
weekday boundary cases for January 5, 2026 at 14:30 UTC and 21:00 UTC.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5d88d8b-5538-4005-a549-be988d024677
📒 Files selected for processing (12)
poller/mcp_client.pypoller/observe.pypoller/schedule.pytests/__init__.pytests/poller/conftest.pytests/poller/fake_mcp_server.pytests/poller/hang_mcp_server.pytests/poller/silent_mcp_server.pytests/poller/test_mcp_client.pytests/poller/test_observe.pytests/poller/test_schedule.pytests/test_positioning_artifact.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
poller/db.py (1)
120-130: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDerive insert columns per row, not from
rows[0]only.
cols = list(rows[0].keys())derives the column set from the first row only, then[r.get(c) for c in cols] for r in rows]applies that fixed column set to every row. If a later row in the same batch has extra keys absent fromrows[0], those values are silently dropped; if a later row is missing a key present inrows[0], it silently becomesNoneinstead of failing.Every current caller in poller/normalize.py builds each row in a batch from the same literal dict shape, so this does not manifest today. It is still a fragile, undocumented precondition on a general-purpose helper; a future heterogeneous batch would silently lose data instead of raising an error.
🛡️ Proposed fix: validate row shape before insert
cols = list(rows[0].keys()) + if any(set(r.keys()) != set(cols) for r in rows[1:]): + raise ValueError(f"insert_rows: heterogeneous row keys for table {table!r}") sql = f"INSERT OR IGNORE INTO {table} ({','.join(cols)}) VALUES ({','.join('?' * len(cols))})"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@poller/db.py` around lines 120 - 130, Update insert_rows to validate that every row has the same key set before constructing the INSERT statement, rather than deriving columns solely from rows[0]. Raise a clear error for heterogeneous row shapes; preserve the existing empty-batch behavior and insertion/counting flow for valid batches.poller/normalize.py (1)
75-80: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard
tickerandtimestampin the put/call-ratio branch, matching the sibling branches.
d.get("ticker", d.get("sym", ""))on Line 76 silently defaults to"", andd["timestamp"]is a direct, unguarded subscript. This repeats the exact pattern already fixed formenthorq_dealer_positioning(Line 69, now using_req), but this branch was left out of that fix.Consequences:
- A missing
timestampraises a rawKeyErrorinstead of the descriptiveValueErrorused by every other branch into_rows.- A blank or missing ticker either violates the
ticker TEXT NOT NULLconstraint input_call_ratiowith an unhandledsqlite3.IntegrityError, or two distinct sources both missing a ticker collide under the same(ticker="", ts, source)key, defeating the source-aware uniqueness constraint added in poller/db.py.No test in tests/poller/test_normalize.py covers null, missing, or blank ticker/timestamp for
menthorq_put_call_ratioorspotgamma_equity_put_call_ratio, unlike the equivalent dealer-positioning and gamma-level tests.🛡️ Proposed fix
if tool in ("menthorq_put_call_ratio", "spotgamma_equity_put_call_ratio"): - row = {"ticker": d.get("ticker", d.get("sym", "")), "ts": d["timestamp"], + row = {"ticker": _req(tool, d, "ticker") if "ticker" in d else _req(tool, d, "sym"), + "ts": _req(tool, d, "timestamp"), "volume_calls": d.get("volume_calls"), "volume_puts": d.get("volume_puts"), "ratio": d.get("put_call_ratio"), "payload": payload, "captured_at": captured_at, "source": src} return "put_call_ratio", [row]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@poller/normalize.py` around lines 75 - 80, Update the put/call-ratio branch in to_rows to validate ticker and timestamp using the existing _req helper, matching the menthorq_dealer_positioning and other sibling branches. Replace the default-empty ticker lookup and direct timestamp subscript with guarded values before constructing the row, and add coverage in the poller normalization tests for missing, null, and blank ticker or timestamp inputs for both supported tools.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@poller/mcp_client.py`:
- Around line 95-102: Update the response loop in the relevant RPC method to
validate each line and convert malformed JSON or non-dict values into McpError
instead of leaking JSONDecodeError or TypeError. Establish an absolute deadline
before the loop, compute the remaining budget before every _readline call, and
pass that remaining timeout so skipped notifications cannot extend the total
wait beyond self._read_timeout; preserve response-ID validation and notification
skipping.
In `@poller/observe.py`:
- Around line 55-58: Update the cycle query used by status_report to order
grouped rows by cycle_id descending before the results are read via cycles[0].
Add ORDER BY cycle_id DESC to ensure the newest cycle is consistently reported
as last_cycle.
---
Outside diff comments:
In `@poller/db.py`:
- Around line 120-130: Update insert_rows to validate that every row has the
same key set before constructing the INSERT statement, rather than deriving
columns solely from rows[0]. Raise a clear error for heterogeneous row shapes;
preserve the existing empty-batch behavior and insertion/counting flow for valid
batches.
In `@poller/normalize.py`:
- Around line 75-80: Update the put/call-ratio branch in to_rows to validate
ticker and timestamp using the existing _req helper, matching the
menthorq_dealer_positioning and other sibling branches. Replace the
default-empty ticker lookup and direct timestamp subscript with guarded values
before constructing the row, and add coverage in the poller normalization tests
for missing, null, and blank ticker or timestamp inputs for both supported
tools.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1302edb-fc03-499a-9ea0-0c6dd48e30cf
📒 Files selected for processing (11)
poller/db.pypoller/mcp_client.pypoller/normalize.pypoller/observe.pypoller/schedule.pytests/poller/fake_mcp_server.pytests/poller/test_db.pytests/poller/test_mcp_client.pytests/poller/test_normalize.pytests/poller/test_observe.pytests/poller/test_schedule.py
| while True: | ||
| resp = json.loads(self._readline()) | ||
| if "id" not in resp or "method" in resp: | ||
| continue # server notification; not a response | ||
| if resp["id"] != self._next_id: | ||
| raise McpError( | ||
| f"unexpected response id {resp['id']!r}, expected {self._next_id}") | ||
| return resp |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate response shape and bound the total wait across skipped notifications.
Two gaps in the new response loop:
json.loads(self._readline())on Line 96 has no guard. A malformed or non-JSON line from the subprocess raises a rawjson.JSONDecodeErrorinstead ofMcpError. A non-dict JSON value (for example a bare number) makes"id" not in respraiseTypeErrorinstead ofMcpError._readline()appliesself._read_timeoutper individual read call. Because this loop can now iterate multiple times to skip notifications, a server that emits notifications faster than the timeout keeps_rpcblocked past the nominal_read_timeoutbudget.
🛡️ Proposed fix for malformed/non-dict responses
while True:
- resp = json.loads(self._readline())
+ try:
+ resp = json.loads(self._readline())
+ except json.JSONDecodeError as e:
+ raise McpError(f"invalid JSON from server: {e}") from e
+ if not isinstance(resp, dict):
+ raise McpError(f"unexpected non-object response: {resp!r}")
if "id" not in resp or "method" in resp:
continue # server notification; not a response
if resp["id"] != self._next_id:
raise McpError(
f"unexpected response id {resp['id']!r}, expected {self._next_id}")
return respFor the timeout budget, track an absolute deadline before the loop and pass the remaining time to _readline on each iteration, so the total wait never exceeds self._read_timeout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@poller/mcp_client.py` around lines 95 - 102, Update the response loop in the
relevant RPC method to validate each line and convert malformed JSON or non-dict
values into McpError instead of leaking JSONDecodeError or TypeError. Establish
an absolute deadline before the loop, compute the remaining budget before every
_readline call, and pass that remaining timeout so skipped notifications cannot
extend the total wait beyond self._read_timeout; preserve response-ID validation
and notification skipping.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/RUNBOOK.md`:
- Around line 170-172: Update the manual run commands in the RUNBOOK section to
explicitly execute from the repository root, using a repository cd or absolute
paths. Ensure both the one-shot poll command and status command resolve the
virtual environment, poller module, database, and logs relative to the intended
repository.
In `@poller/poll.py`:
- Around line 36-44: Broaden the per-tool exception handling in the run_cycle
polling loop to catch AttributeError and TypeError in addition to the existing
exceptions, ensuring malformed tool payloads are recorded through record_call
and do not skip finish_cycle or abort remaining CAPTURE entries. Add a test in
test_poll.py covering a successful response with malformed data such as data:
None.
- Around line 62-70: Update the except McpError path in the poller cycle flow to
record setup failures before returning: call begin_cycle, record_call with a
non-null tool, the McpError, and source="poller", then call finish_cycle and
failure_streak. Preserve the existing spawn_failure log and return behavior
after recording the failed cycle.
In `@tests/poller/test_plist.py`:
- Around line 16-19: Strengthen the LaunchAgent assertions in the plist test by
comparing ProgramArguments to the exact expected list, rather than checking
membership. Replace the WorkingDirectory suffix check and log-path key-presence
checks with exact value assertions for WorkingDirectory, StandardOutPath, and
StandardErrorPath, using the expected contract values defined by the test setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d5ec61a-965c-4f3d-92ed-eb0aa06e3b07
📒 Files selected for processing (7)
docs/RUNBOOK.mdpoller/com.gexhub.poller.plistpoller/poll.pypoller/status.pytests/poller/test_plist.pytests/poller/test_poll.pytests/poller/test_status.py
| One shot: `.venv/bin/python3 -m poller.poll --once` | ||
| Status: `.venv/bin/python3 -m poller.status` | ||
| Logs: `logs/poller.jsonl` (cycles), `logs/launchagent.log` (stdout/stderr) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
State the required working directory for manual runs.
The one-shot and status commands use a relative .venv/bin/python3. poller.poll also defaults to relative timeseries.db and logs/poller.jsonl (poller/poll.py Lines 50-54). A run from another directory can fail to import poller or write a second database and log. Add an explicit repository cd or use absolute paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/RUNBOOK.md` around lines 170 - 172, Update the manual run commands in
the RUNBOOK section to explicitly execute from the repository root, using a
repository cd or absolute paths. Ensure both the one-shot poll command and
status command resolve the virtual environment, poller module, database, and
logs relative to the intended repository.
| try: | ||
| result = clients[key].call_tool(tool, args) | ||
| table, rows = to_rows(tool, result, cap) | ||
| written = insert_rows(conn, table, rows) | ||
| record_call(conn, cid, tool, result.http_status, written, None) | ||
| rows_total += written | ||
| except (McpError, ValueError, KeyError, sqlite3.Error) as e: | ||
| errors += 1 | ||
| record_call(conn, cid, tool, None, 0, str(e)[:300]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Broaden the per-tool exception handling to avoid aborting the whole cycle.
The except tuple only covers McpError, ValueError, KeyError, sqlite3.Error. to_rows (poller/normalize.py) accesses d.get(...) and iterates items with it.get(...)/_first(it, ...) without verifying d/it is a dict. A tool response that is technically successful (result.ok) but returns data: None or a list containing a non-dict item raises AttributeError or TypeError, neither of which is caught here.
When that happens, the exception escapes run_cycle, the remaining CAPTURE entries for this cycle are skipped, and finish_cycle(conn, cid) on line 45 never runs, leaving the cycle record unfinished. This also propagates up through main(), which only catches McpError (line 65), crashing the process with an unhandled traceback instead of exiting cleanly with errors >= 1.
Widen the catch so a single malformed payload cannot take down the entire polling cycle:
🐛 Proposed fix
- except (McpError, ValueError, KeyError, sqlite3.Error) as e:
+ except (McpError, ValueError, KeyError, TypeError, AttributeError, sqlite3.Error) as e:
errors += 1
record_call(conn, cid, tool, None, 0, str(e)[:300])Also consider adding a test_poll.py case (e.g. a fixture with data: None) that exercises this path, since none of the current tests cover an unexpected/malformed payload shape.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| result = clients[key].call_tool(tool, args) | |
| table, rows = to_rows(tool, result, cap) | |
| written = insert_rows(conn, table, rows) | |
| record_call(conn, cid, tool, result.http_status, written, None) | |
| rows_total += written | |
| except (McpError, ValueError, KeyError, sqlite3.Error) as e: | |
| errors += 1 | |
| record_call(conn, cid, tool, None, 0, str(e)[:300]) | |
| try: | |
| result = clients[key].call_tool(tool, args) | |
| table, rows = to_rows(tool, result, cap) | |
| written = insert_rows(conn, table, rows) | |
| record_call(conn, cid, tool, result.http_status, written, None) | |
| rows_total += written | |
| except (McpError, ValueError, KeyError, TypeError, AttributeError, sqlite3.Error) as e: | |
| errors += 1 | |
| record_call(conn, cid, tool, None, 0, str(e)[:300]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@poller/poll.py` around lines 36 - 44, Broaden the per-tool exception handling
in the run_cycle polling loop to catch AttributeError and TypeError in addition
to the existing exceptions, ensuring malformed tool payloads are recorded
through record_call and do not skip finish_cycle or abort remaining CAPTURE
entries. Add a test in test_poll.py covering a successful response with
malformed data such as data: None.
| try: | ||
| with McpClient(MENTHORQ_ARGV) as m, McpClient(SPOTGAMMA_ARGV) as sg: | ||
| rep = run_cycle({"m": m, "sg": sg}, conn, logger) | ||
| except McpError as e: | ||
| logger.log({"event": "spawn_failure", "error": str(e)[:300]}) | ||
| return 1 | ||
| if failure_streak(conn) >= 3: | ||
| notify_macos("gex-poller", "3 consecutive failed cycles — check logs/poller.jsonl") | ||
| return 0 if rep["errors"] == 0 else 1 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect failure_streak's implementation to confirm what it counts as a "failed cycle".
fd -a 'observe.py' poller
rg -n -B2 -A20 'def failure_streak' poller/observe.pyRepository: filipesalvio-code/gex-hub
Length of output: 861
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- poller files ---'
git ls-files poller
printf '%s\n' '--- poll.py ---'
cat -n poller/poll.py
printf '%s\n' '--- observe.py ---'
cat -n poller/observe.py
printf '%s\n' '--- cycle and schema definitions/usages ---'
rg -n -C 4 'def (begin_cycle|record_call|finish_cycle)|CREATE TABLE.*scrape_runs|scrape_runs|run_cycle\(' pollerRepository: filipesalvio-code/gex-hub
Length of output: 12854
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- db.py schema and cycle helpers ---'
cat -n poller/db.py | sed -n '1,125p'
printf '%s\n' '--- McpClient context-manager behavior ---'
cat -n poller/mcp_client.py | sed -n '1,180p'
printf '%s\n' '--- tests and failure-streak references ---'
git ls-files | rg '(^|/)(test|tests)/|test_.*\.py$|.*_test\.py$' || true
rg -n -C 3 'failure_streak|spawn_failure|begin_cycle|record_call|finish_cycle' --glob '*.py' .
printf '%s\n' '--- standalone SQL shape probe ---'
python3 - <<'PY'
import sqlite3
conn = sqlite3.connect(":memory:")
conn.row_factory = sqlite3.Row
conn.executescript("""
CREATE TABLE scrape_runs (
id INTEGER PRIMARY KEY AUTOINCREMENT,
cycle_id INTEGER NOT NULL,
source TEXT NOT NULL,
tool TEXT,
started_at TEXT NOT NULL,
finished_at TEXT,
http_status INTEGER,
rows_written INTEGER DEFAULT 0,
error TEXT
);
""")
def streak():
cycles = conn.execute(
"SELECT cycle_id, MAX(error IS NOT NULL) AS failed FROM scrape_runs"
" WHERE tool IS NOT NULL GROUP BY cycle_id ORDER BY cycle_id DESC"
).fetchall()
n = 0
for row in cycles:
if row["failed"]:
n += 1
else:
break
return n, [(row["cycle_id"], row["failed"]) for row in cycles]
# A run_cycle failure has a failed tool row plus its cycle marker.
conn.execute("INSERT INTO scrape_runs VALUES (1,1,'poller',NULL,'t','t',NULL,0,NULL)")
conn.execute("INSERT INTO scrape_runs VALUES (2,1,'menthorq','menthorq_x','t','t',NULL,0,'spawn/call error')")
# A successful cycle has a non-error tool row.
conn.execute("INSERT INTO scrape_runs VALUES (3,2,'poller',NULL,'t','t',NULL,0,NULL)")
conn.execute("INSERT INTO scrape_runs VALUES (4,2,'menthorq','menthorq_x','t','t',200,1,NULL)")
print("run_cycle-shaped data:", streak())
# A spawn-only cycle with no tool row is excluded.
conn.execute("INSERT INTO scrape_runs VALUES (5,3,'poller',NULL,'t','t',NULL,0,NULL)")
print("spawn-only marker:", streak())
# Adding the proposed failed tool row makes that cycle count.
conn.execute("INSERT INTO scrape_runs VALUES (6,3,'spotgamma','mcp_spawn','t','t',NULL,0,'spawn error')")
print("spawn row:", streak())
PYRepository: filipesalvio-code/gex-hub
Length of output: 21132
Record McpClient setup failures as failed cycles before returning.
Call begin_cycle, record_call with a non-null tool and error, finish_cycle, and failure_streak in the except McpError path. Pass source="poller" to record_call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@poller/poll.py` around lines 62 - 70, Update the except McpError path in the
poller cycle flow to record setup failures before returning: call begin_cycle,
record_call with a non-null tool, the McpError, and source="poller", then call
finish_cycle and failure_streak. Preserve the existing spawn_failure log and
return behavior after recording the failed cycle.
| assert d["ProgramArguments"][0] == VENV_PYTHON | ||
| assert "-m" in d["ProgramArguments"] and "poller.poll" in d["ProgramArguments"] | ||
| assert d["WorkingDirectory"].endswith("gex-hub") | ||
| assert "StandardOutPath" in d and "StandardErrorPath" in d |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete LaunchAgent contract.
Membership checks allow invalid argument order and extra arguments. The suffix and key-presence checks allow incorrect working and log paths. Assert the exact ProgramArguments list and exact values for WorkingDirectory, StandardOutPath, and StandardErrorPath.
Suggested assertions
- assert d["ProgramArguments"][0] == VENV_PYTHON
- assert "-m" in d["ProgramArguments"] and "poller.poll" in d["ProgramArguments"]
- assert d["WorkingDirectory"].endswith("gex-hub")
- assert "StandardOutPath" in d and "StandardErrorPath" in d
+ repo_root = PLIST.parents[1]
+ assert d["ProgramArguments"] == [VENV_PYTHON, "-m", "poller.poll"]
+ assert d["WorkingDirectory"] == str(repo_root)
+ assert d["StandardOutPath"] == str(repo_root / "logs" / "launchagent.log")
+ assert d["StandardErrorPath"] == str(repo_root / "logs" / "launchagent.log")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert d["ProgramArguments"][0] == VENV_PYTHON | |
| assert "-m" in d["ProgramArguments"] and "poller.poll" in d["ProgramArguments"] | |
| assert d["WorkingDirectory"].endswith("gex-hub") | |
| assert "StandardOutPath" in d and "StandardErrorPath" in d | |
| repo_root = PLIST.parents[1] | |
| assert d["ProgramArguments"] == [VENV_PYTHON, "-m", "poller.poll"] | |
| assert d["WorkingDirectory"] == str(repo_root) | |
| assert d["StandardOutPath"] == str(repo_root / "logs" / "launchagent.log") | |
| assert d["StandardErrorPath"] == str(repo_root / "logs" / "launchagent.log") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/poller/test_plist.py` around lines 16 - 19, Strengthen the LaunchAgent
assertions in the plist test by comparing ProgramArguments to the exact expected
list, rather than checking membership. Replace the WorkingDirectory suffix check
and log-path key-presence checks with exact value assertions for
WorkingDirectory, StandardOutPath, and StandardErrorPath, using the expected
contract values defined by the test setup.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if "error" in r: | ||
| raise McpError(f"{name}: {r['error'].get('message')}") | ||
| result = r.get("result", {}) | ||
| if result.get("isError"): | ||
| text = result["content"][0]["text"] if result.get("content") else "isError" | ||
| return ToolResult(name, ok=False, error=text[:300], raw=text) | ||
| text = result["content"][0]["text"] | ||
| return parse_tool_text(name, text) |
There was a problem hiding this comment.
content[0] access raises IndexError outside the catch
The success path text = result["content"][0]["text"] will raise IndexError when an MCP server returns a well-formed but empty content list ({"result": {"content": []}}), and KeyError when "content" is absent entirely. Neither exception is in run_cycle's catch tuple (McpError, ValueError, KeyError, sqlite3.Error) — IndexError escapes entirely, propagates out of run_cycle before finish_cycle is called, and the cycle row in scrape_runs is left without a finished_at, while main() receives an unhandled exception and no structured log entry is written.
Prompt To Fix With AI
This is a comment left during a code review.
Path: poller/mcp_client.py
Line: 106-113
Comment:
**Unguarded `content[0]` access raises `IndexError` outside the catch**
The success path `text = result["content"][0]["text"]` will raise `IndexError` when an MCP server returns a well-formed but empty content list (`{"result": {"content": []}}`), and `KeyError` when `"content"` is absent entirely. Neither exception is in `run_cycle`'s catch tuple `(McpError, ValueError, KeyError, sqlite3.Error)` — `IndexError` escapes entirely, propagates out of `run_cycle` before `finish_cycle` is called, and the cycle row in `scrape_runs` is left without a `finished_at`, while `main()` receives an unhandled exception and no structured log entry is written.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| if tool in ("menthorq_put_call_ratio", "spotgamma_equity_put_call_ratio"): | ||
| row = {"ticker": d.get("ticker", d.get("sym", "")), "ts": d["timestamp"], |
There was a problem hiding this comment.
d["timestamp"] raises an opaque KeyError on missing field — every other required field in to_rows uses _req() which raises an informative ValueError. The KeyError IS caught by run_cycle, but the recorded error string will be "'timestamp'" rather than "menthorq_put_call_ratio: missing required field 'timestamp'", making log-based debugging harder.
| if tool in ("menthorq_put_call_ratio", "spotgamma_equity_put_call_ratio"): | |
| row = {"ticker": d.get("ticker", d.get("sym", "")), "ts": d["timestamp"], | |
| if tool in ("menthorq_put_call_ratio", "spotgamma_equity_put_call_ratio"): | |
| row = {"ticker": d.get("ticker", d.get("sym", "")), "ts": _req(tool, d, "timestamp"), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: poller/normalize.py
Line: 75-76
Comment:
`d["timestamp"]` raises an opaque `KeyError` on missing field — every other required field in `to_rows` uses `_req()` which raises an informative `ValueError`. The `KeyError` IS caught by `run_cycle`, but the recorded error string will be `"'timestamp'"` rather than `"menthorq_put_call_ratio: missing required field 'timestamp'"`, making log-based debugging harder.
```suggestion
if tool in ("menthorq_put_call_ratio", "spotgamma_equity_put_call_ratio"):
row = {"ticker": d.get("ticker", d.get("sym", "")), "ts": _req(tool, d, "timestamp"),
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| def insert_rows(conn: sqlite3.Connection, table: str, rows: list[dict]) -> int: | ||
| if table not in DATA_TABLES: | ||
| raise ValueError(f"unknown table: {table}") | ||
| if not rows: | ||
| return 0 | ||
| cols = list(rows[0].keys()) | ||
| sql = f"INSERT OR IGNORE INTO {table} ({','.join(cols)}) VALUES ({','.join('?' * len(cols))})" |
There was a problem hiding this comment.
cols = list(rows[0].keys()) are injected verbatim into the INSERT statement: f"INSERT OR IGNORE INTO {table} ({','.join(cols)}) VALUES (...)". The table name is validated against DATA_TABLES, but the column names are not. A caller passing a row dict with a key like "gex_1, payload) SELECT 1; --" would produce malformed (or exploitable) SQL. Today all callers in normalize.py use hardcoded keys, but the lack of a column-name allowlist at the DB layer is one refactor away from a latent injection path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: poller/db.py
Line: 119-126
Comment:
**Column names interpolated into SQL without validation**
`cols = list(rows[0].keys())` are injected verbatim into the `INSERT` statement: `f"INSERT OR IGNORE INTO {table} ({','.join(cols)}) VALUES (...)"`. The table name is validated against `DATA_TABLES`, but the column names are not. A caller passing a row dict with a key like `"gex_1, payload) SELECT 1; --"` would produce malformed (or exploitable) SQL. Today all callers in `normalize.py` use hardcoded keys, but the lack of a column-name allowlist at the DB layer is one refactor away from a latent injection path.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| <?xml version="1.0" encoding="UTF-8"?> | ||
| <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> | ||
| <plist version="1.0"> | ||
| <dict> | ||
| <key>Label</key><string>com.gexhub.poller</string> | ||
| <key>ProgramArguments</key> | ||
| <array> | ||
| <string>/Users/filipesalvio/gex-hub/.venv/bin/python3</string> | ||
| <string>-m</string> | ||
| <string>poller.poll</string> | ||
| </array> | ||
| <key>WorkingDirectory</key><string>/Users/filipesalvio/gex-hub</string> | ||
| <key>StartInterval</key><integer>900</integer> | ||
| <key>StandardOutPath</key><string>/Users/filipesalvio/gex-hub/logs/launchagent.log</string> | ||
| <key>StandardErrorPath</key><string>/Users/filipesalvio/gex-hub/logs/launchagent.log</string> | ||
| </dict> | ||
| </plist> |
There was a problem hiding this comment.
/Users/filipesalvio/gex-hub is hardcoded as both the Python interpreter path and WorkingDirectory. test_plist.py also hardcodes this value in VENV_PYTHON, so anyone cloning the repo to a different machine will have a test that asserts an incorrect path. The RUNBOOK should either document how to regenerate the plist or substitute $HOME / a placeholder, and the test should assert the structural contract (e.g., path ends with .venv/bin/python3) rather than a literal user path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: poller/com.gexhub.poller.plist
Line: 1-17
Comment:
**Hardcoded user-specific paths committed to shared repo**
`/Users/filipesalvio/gex-hub` is hardcoded as both the Python interpreter path and `WorkingDirectory`. `test_plist.py` also hardcodes this value in `VENV_PYTHON`, so anyone cloning the repo to a different machine will have a test that asserts an incorrect path. The RUNBOOK should either document how to regenerate the plist or substitute `$HOME` / a placeholder, and the test should assert the structural contract (e.g., path ends with `.venv/bin/python3`) rather than a literal user path.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def _migrate_put_call_ratio(conn: sqlite3.Connection) -> None: | ||
| row = conn.execute( | ||
| "SELECT sql FROM sqlite_master WHERE type='table'" | ||
| " AND name='put_call_ratio'").fetchone() | ||
| if row is None or "UNIQUE (ticker, ts, source)" in row[0]: | ||
| return | ||
| conn.executescript( | ||
| "ALTER TABLE put_call_ratio RENAME TO put_call_ratio_old;\n" | ||
| + _PCR_NEW_SQL + "\n" | ||
| "INSERT OR IGNORE INTO put_call_ratio" | ||
| " (ticker, ts, volume_calls, volume_puts, ratio, payload, captured_at, source)" | ||
| " SELECT ticker, ts, volume_calls, volume_puts, ratio, payload, captured_at, source" | ||
| " FROM put_call_ratio_old;\n" | ||
| "DROP TABLE put_call_ratio_old;") | ||
| conn.commit() |
There was a problem hiding this comment.
Python's executescript() issues an implicit COMMIT before running the script, leaving all subsequent statements in autocommit mode — each statement commits immediately with no rollback on failure. If the process is killed after ALTER TABLE put_call_ratio RENAME TO put_call_ratio_old commits (step 1) but before CREATE TABLE put_call_ratio runs (step 2), sqlite_master no longer has a put_call_ratio entry. On the next startup _migrate_put_call_ratio evaluates row is None → return, skips the migration entirely, and conn.executescript(_SCHEMA) creates a fresh empty put_call_ratio via CREATE TABLE IF NOT EXISTS — leaving all the historical rows stranded in put_call_ratio_old with no path to recover them.
Adding BEGIN IMMEDIATE;\n at the start of the script and \nCOMMIT; at the end makes the entire migration a single SQLite transaction; a mid-flight crash rolls it back so the old schema is intact on the next startup and migration is reattempted correctly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: poller/db.py
Line: 64-78
Comment:
**Non-atomic migration can orphan historical PCR data**
Python's `executescript()` issues an implicit `COMMIT` before running the script, leaving all subsequent statements in autocommit mode — each statement commits immediately with no rollback on failure. If the process is killed after `ALTER TABLE put_call_ratio RENAME TO put_call_ratio_old` commits (step 1) but before `CREATE TABLE put_call_ratio` runs (step 2), `sqlite_master` no longer has a `put_call_ratio` entry. On the next startup `_migrate_put_call_ratio` evaluates `row is None → return`, skips the migration entirely, and `conn.executescript(_SCHEMA)` creates a fresh empty `put_call_ratio` via `CREATE TABLE IF NOT EXISTS` — leaving all the historical rows stranded in `put_call_ratio_old` with no path to recover them.
Adding `BEGIN IMMEDIATE;\n` at the start of the script and `\nCOMMIT;` at the end makes the entire migration a single SQLite transaction; a mid-flight crash rolls it back so the old schema is intact on the next startup and migration is reattempted correctly.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
1 participant
Implementation of docs/superpowers/plans/2026-08-01-gex-poller.md. CodeRabbit gate 1: tasks 1-3 (scaffold, db, normalize).
Summary by CodeRabbit
New Features
Documentation
Tests
Greptile Summary
Introduces
gex-poller, a new LaunchAgent-scheduled subsystem that spawns the MenthorQ and SpotGamma MCP servers via stdio JSON-RPC, normalises their responses, and persists time-series data into a local SQLitetimeseries.db. The implementation is well-structured with a clean separation of concerns acrossdb,normalize,mcp_client,observe,schedule, andpollmodules, and is backed by comprehensive unit and integration tests.poller/db.py: Defines the SQLite schema (6 data tables +scrape_runsaudit table), a one-time migration for theput_call_ratiounique constraint, and row-insert/cycle-audit helpers. The migration usesexecutescriptwithout a wrappingBEGIN/COMMIT, making it non-atomic and capable of orphaning data if interrupted.poller/mcp_client.py: Minimal stdio JSON-RPC client with timeout, process-death detection, and notification filtering; the success pathresult[\"content\"][0][\"text\"]on line 112 remains unguarded against an empty content list.poller/normalize.py+poller/poll.py: Response normalisation and the per-tool polling loop; theput_call_ratiobranch still accessesd[\"timestamp\"]via direct dict lookup (raising an opaqueKeyError) rather than through_req().Confidence Score: 4/5
Safe to merge for day-to-day use; the non-atomic migration is the one concrete data-loss path, but it only fires during an upgrade from the old PCR schema and only if the process is killed in the middle of four fast DDL statements.
The non-atomic migration in
_migrate_put_call_ratiocan silently drop all historicalput_call_ratiorows if the process is interrupted between the table rename and the table create. The three issues carried over from round 1 (empty-content IndexError escapingrun_cycle, opaque KeyError for a missing PCR timestamp, and unvalidated column names in INSERT) also remain open. The rest of the implementation — scheduling, observability, the MCP client, and the test suite — is solid.Files Needing Attention: poller/db.py (migration atomicity), poller/mcp_client.py (empty content list on success path), poller/poll.py (exception catch tuple), poller/normalize.py (direct dict access for PCR timestamp)
Important Files Changed
_migrate_put_call_ratiouses a non-atomicexecutescriptthat can orphan historical data if interrupted mid-flight; column names are interpolated into INSERT SQL without an allowlist.result["content"][0]["text"](line 112) raises IndexError on an empty content list — IndexError is not caught in run_cycle's exception tuple, leaving the cycle row without a finished_at.d["timestamp"]in the put_call_ratio branch (line 76) raises an opaque KeyError instead of the informative ValueError that every other required field uses via_req().pytest -qstep preceding both gates means integration tests run twice in CI, but all gates pass correctly.Sequence Diagram
sequenceDiagram participant LA as LaunchAgent (every 15 min) participant Poll as poller.poll.main() participant Sched as schedule.in_market_window() participant MCP_M as McpClient (menthorq) participant MCP_SG as McpClient (spotgamma) participant Norm as normalize.to_rows() participant DB as db (timeseries.db) participant Log as JsonlLogger LA->>Poll: spawn python -m poller.poll Poll->>Sched: in_market_window(now)? alt outside market hours Sched-->>Poll: False exit 0 else inside market hours Sched-->>Poll: True Poll->>DB: init_db() / migrate Poll->>MCP_M: enter (initialize handshake) Poll->>MCP_SG: enter (initialize handshake) Poll->>DB: begin_cycle() loop each (key, tool, args) in CAPTURE Poll->>MCP_M: call_tool(tool, args) MCP_M-->>Poll: ToolResult Poll->>Norm: to_rows(tool, result, captured_at) Norm-->>Poll: (table, rows) Poll->>DB: insert_rows(table, rows) Poll->>DB: record_call(cid, tool, ...) end Poll->>DB: finish_cycle(cid) Poll->>Log: log event cycle Poll->>DB: failure_streak() opt streak gte 3 Poll->>Poll: notify_macos(...) end Poll-->>LA: exit 0/1 endPrompt To Fix All With AI
Reviews (2): Last reviewed commit: "docs: correct gated-endpoint examples in..." | Re-trigger Greptile