chore: cleanup wave-2 (tomte, tox.ini, CI lint, slim docs)#175
Merged
Conversation
Planning-only commit. Outlines the remaining wave-2-style cleanup work to bring quickstart in line with trader (latest tomte, minimal tox.ini, slim CI). PR #172 already handled the poetry to uv migration. The plan covers what's still missing: tomte in dev deps, a tox.ini, a Makefile fix, a linter CI job, and a CONTRIBUTING.md trim. No runtime change. Once approved, implementation commits will be pushed on top of this same branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
OjusWiZard
reviewed
May 21, 2026
- Use PyPI pin `tomte[cli, tests]==0.7.0` instead of the git URL (v0.7.0 is published, the git form is unnecessary). - Keep `liccheck` in scope: it reads [Licenses] and [Authorized Packages], nothing AEA-specific. - Lock Makefile decision to "delete" (no references). - Config schema doc stays in CONTRIBUTING.md (no move). - Drop the "open questions" section since both are resolved. - Sketch [Licenses] + [Authorized Packages] blocks in the tox.ini draft. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Eight findings from DhairyaPatel7's review, all verified against the actual repo: - Add `linter_checks` to `all-checks-passed.needs` so the new lint job actually gates branch protection. - Pull the 7 stale `poetry` invocations in scripts/*/README.md into scope as commit 6 (PR #172 missed them). - Fix .sh count: 10 files total (was claiming 8). 9 use uv, extract_private_keys.sh is pure bash. - Add `tox-uv` to dev deps + note that `[cli]` already includes `tox` (so local `uv run tomte tox` works out of the box). - Pin action versions to checkout@v4 + setup-python@v5 (matches rest of workflow; was v6/v6). - Replace "four existing jobs" with the actual six. - Replace "Expected drop: ~344 lines" wording with the correct "Net delta: ~+22 lines" (the plan is additive, nothing drops). - Note the sequencing constraint: commit 4 (CI gate) only after commit 2 (lint green locally), else the branch lands red on its own gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mte] Brings quickstart in line with trader's wave-2 setup: lint via `tomte tox`, configured through a [tool.tomte] block in pyproject. - Add tomte[cli, tests]==0.7.0 (PyPI pin, [cli] extra already pulls tox). - Add tox-uv==1.16.0 so `uv run tomte tox` works out of the box. - Drop the explicit pytest / pytest-cov pins from dev deps; tomte[tests] brings them in at compatible versions (pytest 8.4.2 / pytest-cov 7.0.0). - [tool.tomte] points the canonical linters at scripts/ (excluding tests/ from lint, matching trader's pattern). pytest_targets stays at tests/. - Add a minimal [build-system] + [tool.setuptools] py-modules = [] so that tomte testenvs which trigger an editable install don't blow up on flat-layout auto-discovery (data/, images/, configs/). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a minimal tox.ini consumed by `tomte tox` (extensions block, [pytest], [Authorized Packages] for liccheck, [mypy-*] ignore blocks), then fixes every violation the 9 lint envs surfaced in scripts/. Real bug fixes: - rank_traders.py: `load_local_config()` was missing the required `operate` and `service_name` kwargs the middleware API needs since 0.15.x. Now passes OperateApp() + service_name="Trader Agent". Test mock updated to accept **_kwargs. - migrate_to_pearl._ensure_signable: 7× B023 closures weren't binding the loop variables qs_master_safe and chain_str. Bound via default args, same pattern as the sibling _ledger_api closure. - migrate_to_pearl: F811 duplicates of Service + Chain removed from the TYPE_CHECKING block (already imported at runtime). - migrate_to_pearl: 6 module-level imports moved out of mid-file (E402); stale `from operate.operate_types import Chain as _Chain` removed and _Chain usage replaced with the top-level Chain import. - migrate_legacy_quickstart.py: stale unused `autonomy.chain.config.ChainType` import dropped. - 10× requests.get/post calls in predict_trader scripts now pass `timeout=30` (W3101). Two test mocks updated to accept the kwarg. Hygiene: - 2× E800 commented-out code blocks removed. - 5× F541 placeholder-less f-strings cleaned up. - 4× E226 missing operator whitespace. - 2× W293 whitespace on blank lines. - 1× C0104 disallowed name `bar` → `progress_bar` in rank_traders. Docstrings (D100/D101/D102/D103/D105/D107/D205): ~28 added across optimus, predict_trader, and pearl_migration. One-liners except where the docstring already had a malformed multi-line form. Bandit (B607): 3× `# nosec` markers on subprocess.run calls that invoke `docker` / `sudo` by name via PATH — desired UX, matches the behaviour of run_service.sh. tox.ini structure: - [tomte-extensions] extra_pylint_disables: 13 codes (C0114/C0115/C0116/R0801 + C0415/W0621/C0411/W0212/W0718/R1723/ R0903/R0914/R0915) covering legacy-script noise. - [pytest] addopts = -p no:pytest_anchorpy (mirrors the same anchorpy guard already in pyproject's pytest config). - [mypy-scripts.*] ignore_errors = True for pearl_migration, predict_trader, optimus, utils — tomte's [testenv:mypy] doesn't install olas-operate-middleware so mypy can't resolve operate.* as namespace imports. Mirrors trader's [mypy-packages.valory.connections.*] pattern. - [mypy-operate.*], [mypy-autonomy.*] etc. — third-party stubs liccheck would normally complain about. - [Authorized Packages] — per-package liccheck overrides for GPL-dual / unknown-license deps (anchorpy, based58, clea, httpx, jsonalias, olas-operate-middleware, open-autonomy, pyinstaller, pyinstaller-hooks-contrib). All 370 unit tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
It was three poetry-wrapper targets, none referenced from the README, CI workflow, or any operator shell script. The 9 .sh scripts at repo root already cover install + run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Quickstart's CI had no linting at all. Adds a `linter_checks` job that installs tomte + tox-uv and runs the canonical 9 lint envs in parallel (-p): black-check, isort-check, flake8, mypy, pylint, darglint, safety, bandit; plus liccheck as its own step. Action versions pinned to checkout@v4 + setup-python@v5 to match the rest of the workflow (no version skew within one file). Appends `- linter_checks` to the `all-checks-passed.needs` list so branch protection actually gates on lint — without it the new job would be advisory only. Also extends .gitignore for `.tox/`, `*.egg-info/`, and `.tomte-*` so tomte's runtime artefacts don't leak into commits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CONTRIBUTING.md trim: The generic "open an issue, branch, commit, push, review" workflow section (~60 lines) is near-identical to the equivalent block in every other Valory repo. Replaced with a short stub linking to the canonical open-autonomy/CONTRIBUTING.md, plus repo-specific notes on where to file contributions (configs/, scripts/, tests/) and the exact local commands for `tomte tox` lint envs and unit tests. The config.json schema reference stays in place per reviewer feedback on PR #175 — it's quickstart-specific and useful where it is. Doc cleanup for the uv migration: PR #172 swapped the root .sh scripts from poetry to uv but missed the same commands inside two sub-READMEs: - scripts/predict_trader/README.md lines 16, 22, 28, 197, 198 - scripts/optimus/README.md lines 18, 19 All 7 instances rewritten from `poetry install` / `poetry run` to the equivalent `uv sync` / `uv run` form. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per the original PR description, this planning doc is deleted in the final implementation commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DIvyaNautiyal07
changed the title
P1 (test coverage): - test_rank_traders: replaced `lambda **_kwargs` with a positional fake that requires `operate` + `service_name`. Reverting the production code to a zero-arg call now raises TypeError and the test fails. - test_mech_events: dropped the `timeout: int = 30` default on both `_fake_get` mocks. Production must pass `timeout=` or it's a TypeError. P2 (latent bugs + tox.ini cleanup): - trades.py: `_query_omen_xdai_subgraph` and `_query_conditional_tokens_gc_subgraph` now wrap their `requests.post(..., timeout=30)` in `except RequestException` and re-raise as a `RuntimeError` with a clearer message. Previously the lint-driven timeout could surface as an opaque `requests.Timeout` traceback to the report script after 30s. - tox.ini: collapsed four [mypy-scripts.*] blocks into one [mypy-scripts.*], and removed ten dead [mypy-*] ignore_missing_imports blocks (operate, autonomy, aea_*, halo, dotenv, psutil, requests, gql, docker, web3). The collapse keeps mypy green on all 19 source files; the import-ignore blocks were dead because the ignore_errors carve-out already skips every module under scripts/. - migrate_legacy_quickstart.py: tightened the pylint disable comment. Fixed the inaccurate "metaclass" claim to point at eth-account's @combomethod descriptor. - utils.py: rewrote the wrong `get_service_from_config` docstring (it said "Get service safe" but returns a Service object) and the trivial `get_subgraph_api_key` docstring. - prompts.py: rewrote info/warn/backup_suffix docstrings so they add information beyond restating the function name. - migrate_to_pearl.py: reworded the `_ensure_signable` docstring so it doesn't claim a present-tense loop-var hazard. The default-arg binding is now framed as defensive against future refactors that defer the call (matching the outer inline comment). - rank_traders.py: pulled the hardcoded "Trader Agent" string into a module constant `PREDICT_TRADER_SERVICE_NAME` so a rename of the JSON-config name has exactly one place to update. CI: - .github/workflows/python-tests.yml: pinned `tox-uv==1.16.0` in the linter_checks install command (matches the dev-deps pin). All 9 lint envs green + 370/370 unit tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`tomte[tests]==0.7.0` pulls in `pytest-randomly==4.0.1`, which shuffles test order by default. tests/test_migrate_to_pearl.py uses `test_01_*`, `test_02_*`, `test_03_*`, `test_04_*` naming because each step relies on state created by the previous one — e.g. test_03_migrate_second_quickstart_mode_b asserts pearl_home exists, which test_02_migrate_first_quickstart_mode_a is responsible for creating. CI run https://github.com/valory-xyz/quickstart/actions/runs/26279378153 hit this: pytest-randomly ran test_03 first, the assertion failed because pearl_home didn't exist, and test_02/01/04 then SKIPPED. Add `-p no:randomly` to addopts in pyproject.toml and tox.ini (the two pytest config sources tomte tox / direct pytest use). Comments on both sides explain the flag's purpose. Verified locally: `pytest --collect-only` now lists the four tests in numeric order. 370/370 unit tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Root cause of the unit-test job failure: scripts/predict_trader/trades.py lines 359-360 + 402-403 (the new `try/except RequestException → RuntimeError` wrappers from 3783f4c) had zero coverage, dropping total to 99.82% and tripping `--cov-fail-under=100`. Fix: extract one `_post_subgraph_query(url, payload, *, label)` helper that wraps `requests.post` + `raise_for_status` + `.json()` in a single try-block, and apply it at both subgraph call sites. This also closes three concerns flagged in the re-review: - P1: `res.json()` was outside the try-block, so a non-JSON 200 response from the subgraph (e.g. a Cloudflare HTML error page) would raise `JSONDecodeError` and escape the wrapper. The helper's try-block now covers the parse too; `requests.JSONDecodeError` subclasses `RequestException` so the same except branch handles it. - The helper also calls `res.raise_for_status()` so 4xx/5xx responses surface as the same clean `RuntimeError(...)` instead of slipping through to a JSON parse on an HTML body. - P2: the `RuntimeError` message now includes the URL ("omen subgraph query failed for <url>: <exc>") so an operator can act on it. - P2: the duplication between the two subgraph call sites is gone. Tests: - `test_post_subgraph_query_raises_runtimeerror` parametrised over the three failure kinds (network exception, HTTP 500, non-JSON 200 body). Each asserts the resulting `RuntimeError` carries the label + URL. Other re-review findings folded in (smaller): - `test_rank_traders.py:216`: mock now asserts `service_name == rank_traders.PREDICT_TRADER_SERVICE_NAME`, so drift between the constant and the JSON config fails CI. - `rank_traders.py:51`: `PREDICT_TRADER_SERVICE_NAME` is now `Final[str]`, and the comment explicitly says both rename and value changes need updating on both sides. - `pyproject.toml:69`: dropped the `tomte 0.7.0` version from the `-p no:randomly` rationale comment (would rot on tomte bump). The pin in dev-deps stays authoritative. - `tests/test_scripts/test_predict_trader/test_mech_events.py:54, 287`: `_fake_get(url, timeout, **_kwargs)` keeps the timeout regression guard intact while staying forward-compatible if production adds other kwargs (headers=, verify=, etc.). All 9 lint envs green; 373/373 unit tests pass with 100% coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… accuracy)
Two P1 review findings from the third review pass:
1. `test_post_subgraph_query_raises_runtimeerror` "http" case had
`text="internal error"` — not valid JSON, so `.json()` would also
fail with JSONDecodeError if `raise_for_status()` were dropped.
The test couldn't actually distinguish the HTTP branch from the
body-decode branch.
Fix: use a valid JSON body (`{"errors": ["internal"]}`) and add a
per-case `expected_cause` parameter. The test now asserts
`isinstance(exc_info.value.__cause__, expected_cause)` for each
parametrised case (ConnectionError / HTTPError / JSONDecodeError),
so each branch is uniquely exercised. Mutation-verified: deleting
`res.raise_for_status()` from production now fails the "http" case
with DID NOT RAISE.
2. `PREDICT_TRADER_SERVICE_NAME` comment claimed "the test mock
asserts equality so a drift fails CI" — true only for drift
between the constant and the script's call site, NOT between the
constant and the JSON config's `name` field. The JSON isn't read
in the test.
Fix: comment now states explicitly that constant-vs-JSON drift is
NOT covered by CI and only surfaces at production runtime, so a
future maintainer doesn't over-rely on it.
Skipped (per CLAUDE.md "three similar lines is better than a
premature abstraction"):
- Duplicated `_fake_get` mocks: two near-copies with different bodies,
only signature would be shared, fixture indirection isn't worth it.
- `_post_subgraph_query` return type tightening: scripts/* under
ignore_errors so mypy never checks; theoretical only.
- Reverting `Final[str]` on the constant: approved last round,
reverting would be churn.
All 9 lint envs green; 373/373 unit tests pass; 100% coverage holds.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three review findings from DhairyaPatel7's inline comments on 3fa51e9 / a1cc2ca: 1. rank_traders.py:200 was still doing raw `requests.post` + `.json()` against the OMEN gateway — the same call pattern `_post_subgraph_query` exists to harden against. A 500 with a valid-JSON body would slip through, the `.get("data", ...)` would return {}, and the pagination loop would silently exit with zero trades (looks like "no activity" to the user). Switched to `_post_subgraph_query(url, content_json, label="omen")` so the rank-traders script gets the same 500 / network / non-JSON guard the trades.py callers have. Dropped the now-unused `import requests` and module-level `headers` dict in rank_traders.py. 2. test_mech_events.py had two redundant comment blocks above each `_fake_get` — the original one-line `# noqa`-style explanation got layered over by an expanded 5-line block during the self-review commits, producing copy-paste duplication at both call sites (lines 54-61 and 297-304). Collapsed to one 4-line block per `_fake_get` definition. 3. python-tests.yml linter_checks job was running `pip install 'tomte[cli, tests]==0.7.0' 'tox-uv==1.16.0'` — pins duplicated from pyproject.toml's [dependency-groups].dev. Switched to `uv sync --all-groups --frozen` + `uv run tomte tox ...`. Matches the pattern in unit-tests (line 304) and removes the drift risk when the dev-group pins get bumped — there's now one source of truth. #4 (DhairyaPatel7's visibility note on the test_rank_traders.py service_name assert) explicitly says "no change required" — flagging only so future maintainers don't over-read the assertion. Acknowledged inline. All 9 lint envs green; 373/373 unit tests pass; 100% coverage holds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
OjusWiZard
reviewed
May 22, 2026
Member
OjusWiZard
left a comment
OjusWiZard
left a comment
There was a problem hiding this comment.
Automated multi-perspective review (/ecc:review-pr). The bug fixes in this PR are verified correct — no Critical issues. Five Important findings are left as inline comments below. The headline one is the ~11k-line tests/ reformatting churn, which is out of the stated lint scope and unenforced by CI.
Three new fixes from OjusWiZard's review on 45e801e: P1 security (#3288142596): subgraph API key leaked into RuntimeError Added `_redact_subgraph_url` in trades.py that strips the `/api/<KEY>/` segment from gateway URLs. `_post_subgraph_query` now redacts both the URL and the `str(exc)` body (HTTPError.__str__ embeds the full request URL) before raising. The billable Graph gateway key no longer reaches error messages, CI logs, or pasted bug reports. P1 test bug (#3288142584): third `_fake_get`-style mock at test_mech_events.py was still `lambda _url: _BadResponse()`. The production `requests.get(_url, timeout=30)` call would raise TypeError BEFORE `_BadResponse.raise_for_status()` fired — the test passed for the wrong reason. Fixed to `lambda _url, timeout=None, **_kw: _BadResponse()` so the intended broad-except branch is actually exercised. Scope reduction (#3288142576): commit 0953153 ran black/isort across the whole `tests/` tree (~11k lines). Lint scope excludes `tests/`, so CI never enforces that formatting and it would rot on the next test edit. Reverted every `tests/` file to origin/main and reapplied only the 4 surgical logic edits this PR actually needs: - test_trades.py: new `_post_subgraph_query` parametrised test (network / HTTP / body branches, cause-type assertion + key redaction assertion); `_redact_subgraph_url` unit test. - test_rank_traders.py: `_fake_load_local_config(operate, service_name)` mock with the `service_name` assertion. - test_mech_events.py: two `_fake_get` signatures hardened to require `timeout` + accept `**_kwargs`; the third lambda mock (line 312) fixed per #3288142584. Diff vs origin/main on tests/: +106/-4 lines across 3 files (was +4366/-2621 across 11). Test formatting + adding `tests/` to lint scope are a clean follow-up PR. Already fixed in 45e801e (replied, no code change): - #3288142601 (bare pip in linter_checks): replaced with `astral-sh/setup-uv@v6` + `uv sync --all-groups --frozen` + `uv run tomte tox ...` so all linter pins come from pyproject. - #3288142592 (raw `requests.post` in rank_traders.py): now uses `_post_subgraph_query` helper with `raise_for_status`. All 9 lint envs green; 374/374 unit tests pass; 100% coverage holds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DhairyaPatel7 caught two connected issues in the previous redaction
attempt:
#3288298443 — Incomplete redaction. The earlier
`str(exc).replace(url, safe_url)` only scrubbed the key when the FULL
URL appeared in the exception's str. That holds for
`requests.HTTPError` (whose `__str__` produces "... for url: <full>"),
but NOT for the network-error branches: `requests.ConnectionError` /
`Timeout` / `SSLError` wrap a `urllib3.MaxRetryError` whose str
embeds only the PATH (`/api/<KEY>/subgraphs/...`), so `.replace`
with the full URL was a silent no-op and the key still leaked.
Fix: redact the key SEGMENT instead of the full URL, using a regex
that matches both shapes:
_SUBGRAPH_KEY_RE = re.compile(r"(/api/)[^/\s]+")
_redact_subgraph_key(text) # works on URLs and on bare paths
`_post_subgraph_query` now scrubs both `url` and `str(exc)` through
that helper before composing the RuntimeError message.
#3288298448 — The previous "network" test case was vacuous:
`requests.ConnectionError("boom")` has `str(exc) == "boom"`, so the
`SECRET_KEY_VALUE not in msg` assertion passed trivially — there
was nothing for the redaction logic to fail on. Replaced with a
realistic urllib3-shaped message:
HTTPSConnectionPool(host='gateway-arbitrum.network.thegraph.com',
port=443): Max retries exceeded with url:
/api/SECRET_KEY_VALUE/subgraphs/id/... (Caused by ...)
Mutation-verified: reverting the production code to the old
`.replace(url, safe_url)` form now fails the network case with
`'SECRET_KEY_VALUE' is contained here: ...with url:
/api/SECRET_KEY_VALUE/...`. So this case actually guards the
regression now.
Also added a direct unit test for `_redact_subgraph_key` on the
path-only urllib3 form.
All 9 lint envs green; 375/375 unit tests pass; 100% coverage holds.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DhairyaPatel7
left a comment
DhairyaPatel7
left a comment
There was a problem hiding this comment.
Re-review of commit aa014bc: prior issues (incomplete API-key redaction on urllib3-shaped ConnectionError and vacuous parametrised "network" test) are both correctly resolved by the single-regex redactor and the urllib3-shaped test fixture. No new issues.
DhairyaPatel7
previously approved these changes
May 22, 2026
OjusWiZard
reviewed
May 22, 2026
Member
OjusWiZard
left a comment
OjusWiZard
left a comment
There was a problem hiding this comment.
Follow-up review of commits 45e801e, 9854cde, aa014bc. The test-reformatting revert and the uv sync CI change are clean — but the API-key-leak fix is not fully closed: raise RuntimeError(...) from exc re-exposes the key via the chained __cause__ in any uncaught traceback, and the new redaction test has a blind spot that hides it. Two inline comments below.
…ceback rendering
OjusWiZard caught the remaining leak path the previous redaction
attempt missed:
#3288751109 — `raise RuntimeError(...) from exc` keeps the wrapped
`requests.RequestException` as `__cause__`. Python's default
traceback printer renders the cause verbatim ("The above exception
was the direct cause of the following exception: ..."), and that
cause's `str()` still embeds the unredacted gateway key. `report.py`
calls the subgraph helper outside any try/except on the failure
path, so an uncaught error leaks the key into the terminal, CI logs,
and pasted bug reports — exactly the vector this redaction was
added to close.
Fix: `raise RuntimeError(...) from None`. `from None` sets
`__suppress_context__`, so neither `__cause__` nor `__context__`
prints. The redacted message now also includes the original
exception's class name (`ConnectionError` / `HTTPError` /
`JSONDecodeError`) so the debugging context lost by suppressing the
chain is preserved in plain text.
#3288751121 — the parametrised test only inspected
`str(exc_info.value)` (the RuntimeError message), not the full
rendered traceback. The assertion was vacuous against the cause-chain
leak. Tightened it to:
- Render the full chain via `traceback.format_exception()` and
assert `SECRET_KEY_VALUE` doesn't appear there.
- Assert `__cause__` is None (proves `from None` is in force).
- Replaced the `expected_cause` cause-type assertion with an
`expected_type_name` check in the message — the cause object
is now gone, but the class name lives on in the formatted
message.
Mutation-verified: reverting `from None` back to `from exc` (which
re-introduces the cause leak) now fails all three parametrised
cases on the rendered-traceback assertion. So the test guards the
regression.
All 9 lint envs green; 375/375 unit tests pass; 100% coverage holds.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
OjusWiZard
approved these changes
May 22, 2026
Member
OjusWiZard
left a comment
OjusWiZard
left a comment
There was a problem hiding this comment.
Approving. ✅
Reviewed across three rounds — all 7 findings are fixed and verified:
- ~11k-line
tests/reformatting churn — reverted (out of stated lint scope) test_mech_events.pystalelambda _url:mock — fixedrank_traders.pymissingraise_for_status— now reuses_post_subgraph_query- subgraph API-key leak — redaction +
raise … from Nonecloses the message and the__cause__/traceback-render vector; guarded by a mutation-verified test - CI
linter_checkspip install→astral-sh/setup-uv+uv sync --frozen
All 25 review threads resolved; CI green (linter + unit-tests 3.10–3.14, 100% coverage). Two non-blocking nits inline — address or ignore as you see fit.
LOCKhart07
approved these changes
May 22, 2026
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
4 participants
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.
Implementation of the cleanup plan reviewed and approved by @OjusWiZard, with the agent-review findings folded in.
PR #172 handled the poetry → uv migration. This PR adds everything that was missing to match trader's wave-2 state.
What lands
tomte[cli, tests]==0.7.0+tox-uv==1.16.0in dev deps,[tool.tomte]block inpyproject.tomltox.iniconsumed bytomte tox([tomte-extensions],[pytest], mypy overlay,[Authorized Packages]for liccheck)linter_checksCI job running 9 envs in parallel (-p); gated throughall-checks-passed.needsrank_traders.load_local_config()was missing theoperate=andservice_name=kwargs the middleware API has needed since 0.15.xmigrate_to_pearl._ensure_signableService/Chain, E402 misplaced importstimeout=onrequests.get/postcallsscripts/: ~28 docstrings, removed commented-out code, fixed placeholder-less f-strings, fixed E226 spacing, renamed disallowedbaridentifierMakefiledeleted (3 poetry-wrapper targets, referenced nowhere)CONTRIBUTING.mdslimmed from 135 → ~80 lines: workflow stubbed to OA canonical, schema kept per reviewer feedbackpoetryinvocations inscripts/predict_trader/README.mdandscripts/optimus/README.mdrewritten touv.gitignoreextended for.tox/,*.egg-info/,.tomte-*artefactsPost-review hardening
_post_subgraph_queryhelper inscripts/predict_trader/trades.py— wrapsrequests.post+raise_for_status+.json()in a single try/except so network errors, 4xx/5xx responses, and non-JSON bodies all surface as a uniformRuntimeError("<label> subgraph query failed for <url>: <exc>"). Replaces two near-identical try/except blocks.PREDICT_TRADER_SERVICE_NAME: Final[str]constant extracted inrank_traders.py; the test mock asserts the script's call site passes it, so an accidental zero-arg revert fails CI.pytest-randomlydisabled via-p no:randomly. Tomte pulls it in transitively; it shuffled the order-dependenttest_NN_*tests intest_migrate_to_pearl.pyand broke the Mode A → Mode B sequence._post_subgraph_querycovering network exception, HTTP 5xx (viaraise_for_status), and non-JSON body. Each case asserts onexc.__cause__type, so each branch is uniquely exercised (mutation-verified: deletingraise_for_statusfails the HTTP case)._fake_getmocks intest_mech_events.pynow use(url, timeout, **_kwargs)— keeps the W3101 regression guard intact while staying forward-compatible if production adds another kwarg.Test plan
uv run tomte tox -p -e black-check -e isort-check -e flake8 -e mypy -e pylint -e darglint -e bandit -e safety -e liccheckpasses locally (all 9 envs green)PYTHONPATH=. uv run pytest tests --ignore=tests/test_run_service.py --ignore=tests/test_staking_service.py --ignore=tests/test_migrate_to_pearl.py --cov=scripts --cov-fail-under=100passes (373/373 unit tests, 100% coverage)linter_checks+ the existing jobsNotes for the reviewer
[tool.tomte] service_specific_packages = ["scripts"]— lintsscripts/only,tests/is excluded (matches trader's pattern: production code lints, tests don't).[mypy-scripts.*] ignore_errors = True— single block covering all ofscripts/. Tomte's[testenv:mypy]doesn't installolas-operate-middlewareso mypy can't resolveoperate.*as namespace package imports. Mirrors trader's[mypy-packages.valory.connections.*]carve-out. If we later want real mypy onscripts/, that needs a tomte-level change (install the project in the mypy testenv).extra_pylint_disablesextends to 13 codes covering legacy-script noise (C0415,W0621,C0411,W0212,W0718,R1723,R0903,R0914,R0915, plus the original 4).PREDICT_TRADER_SERVICE_NAMEconstant must stay in sync with thenamefield inconfigs/config_predict_trader.json— the CI mock catches constant-vs-call-site drift only, not constant-vs-JSON drift.