Repository navigation
Fix 14 code-review findings (request auth, Slack loop, scheduler, state files, Yahoo, .env migration); docs-only PRs skip tests - #35
Merged
Conversation
…required checks Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVEkZofdPjQ4jtXFKipeUX
robdevops
enabled auto-merge
October 13, 2026 09:39
Capture the changed-file list first so a git error fails the step, require a non-empty all-docs list (exact names, --no-renames) to skip, gate on docs_only != 'true', pass base_ref via env, re-run on base change, name and log the step, pin ubuntu-24.04, compare the full HEAD sha in the version test, and document the skip in CLAUDE.md. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVEkZofdPjQ4jtXFKipeUX
…egister the Telegram webhook only at start, ignore SIGHUP while starting - Requests are routed on the path first and Telegram's secret header is checked before the body is read. A body that is not a JSON object gets 400, a failed check 403, an unknown path 404. A failure before authentication is only logged, so a stranger can no longer post error lines into the chats; a failure after it is reported once and answered 200, so Telegram and Slack do not retry it into the chat again. The expected tokens are no longer printed on a failed check. - Slack: events not written by a person (bot_message and the other subtypes, anything from a bot, the bot's own replies, events without a user) are ignored, so a webhook report or a reply can no longer set off a loop. A Slack request no longer overwrites the Telegram bot name. - lib/telegram.py no longer unregisters/registers the webhook and calls getMe on import, which every job process did. bot.py calls telegram.register() once as it starts (interactive mode only) and refuses to start, with the token redacted, if it fails. - SIGHUP is ignored from the first line of bot.py until startup is done, so a reload sent while the bot starts cannot kill it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVEkZofdPjQ4jtXFKipeUX
Telegram parses messages as HTML and refused error lines containing a literal '<', such as the '<redacted>' placeholder or "'<' not supported", so those reports never arrived. The line is now HTML-escaped (& < >) for Telegram and Slack, which asks for the same three escapes; Discord gets it unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVEkZofdPjQ4jtXFKipeUX
… as text Slack retired files.upload, so the Slack replies that came as charts (.beta, .buy, .sell, .peg, .pie, price, performance and shorts lists, .history, .compare) arrived as nothing. webhook.takes_charts() now decides where a chart can go (Telegram only): no chart is drawn for Slack, and webhook.sendPhoto sends the report as text for any other service, or when Telegram refuses the photo (not after a read timeout, when it may have arrived after all). The README no longer asks for the files:write scope. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVEkZofdPjQ4jtXFKipeUX
…e since the previous close - price_history hands fetch_chart_json's (error, None) back to its callers, whose skip-this-ticker checks could never fire before: it went on into chart_json_to_df and raised TypeError, so one failed ticker took down .price 7d, the price-list buttons, the scheduled price.py N runs and performance (via its SPY/QQQ benchmarks). Now the price list leaves the ticker out (scheduled runs too), a single asked-for ticker gets a one-line error instead of a raw URL plus sys.exit in a worker, a benchmark without history says so in its line, and a profile goes out without its history lines. - The 1D figure fell through to the 5D change. It is now measured against the previous session's close (the last close of an earlier day in the exchange's time zone). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVEkZofdPjQ4jtXFKipeUX
…a corrupt jobs state file no longer stops the bot json_write and write_binary_cache truncated the file and wrote it in place under os.umask(0), then set the umask to a hard-coded 022, and left it at 0 when the write raised. Both now go through util.write_file: a temp file beside the target, renamed over it (state files are also fsynced), mode 0640, umask untouched. A full disk or a kill mid-write leaves the old file as it was. Scheduler.load treated an unreadable finbot_jobs.json as fatal (JSONDecodeError at start, and a SIGHUP then killed the bot). It now reports it once and carries on without a record of past runs; the next run writes the record afresh. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVEkZofdPjQ4jtXFKipeUX
parse_schedule accepted any expression croniter.is_valid() accepts, including dates that never come (30 February, 31 April): the line loaded, then next_fire raised on every tick, so no job ran again and an error was posted every 30 seconds. Such a line, and a time that is not valid at all (minute 61), is now an ImpossibleTime: finbot refuses to start with it (ERROR schedule: on stderr, nothing posted, so a restart loop does not flood the chats), and a reload with it is refused and reported while the running schedule carries on. Other bad lines are still reported and skipped; @reboot is one of those. bot.py now reads the schedule before it registers the Telegram webhook or starts anything else. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVEkZofdPjQ4jtXFKipeUX
next_fire handed croniter a zoneinfo-aware time. croniter up to 6.0, which the distro packages are (Ubuntu 24.04 1.3.15, Debian trixie 2.0.7), gets zoneinfo's DST changes wrong: with CRON_TZ=Australia/Melbourne a 09:00 daily job fired at 08:00 and 09:00 on the day the clocks go forward and at 10:00 on the day they go back; on 6.2 a frequent job could even step back in time during the repeated hour. croniter now works on the naive local time and the zone is put back after, compared by the instant. A repeated time fires once (the first time), a skipped one an hour later by the clock. Checked on 1.3.15, 2.0.7, 6.0.0, 6.2.4. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVEkZofdPjQ4jtXFKipeUX
A command that raises before typing.stop() leaves its indicator thread waiting; a later test that yields to gevent (a subprocess wait, a sleep) let it wake after the fixture's patches were gone, and it printed KeyError: 'telegram' tracebacks after the run. The fixture now stops every indicator a test started when the test ends. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVEkZofdPjQ4jtXFKipeUX
… why is reported The reload command the shipped files gave, systemctl kill -s HUP finbot, signals every process of the unit, so a job process running at that moment died of SIGHUP; the bot took any non-zero exit as already reported, so the run was lost silently. - lib/jobrun.py ignores SIGHUP from its first line, and reports any failure itself, a script that cannot be imported included, exiting with the status REPORTED (3). - The scheduler reports a job that ended any other way: "killed by SIGHUP" (or SIGKILL, an OOM kill, ...) or "exited with status N without saying why". - finbot.service and preferences.conf now give systemctl reload finbot, which signals the bot alone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVEkZofdPjQ4jtXFKipeUX
keep_warm was started only if cache was on at startup and never looked at it again. After a reload set cache=False nothing rewrote the cache files, so everything was always due and the loop fetched the token, the portfolio list and every holdings report every 5 seconds, competing with chat requests for the single Sharesight slot. It now checks the preference on every pass and idles (a look every minute) while it is off; bot.py starts it whenever Sharesight is configured, so a reload that turns cache on starts the warming too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVEkZofdPjQ4jtXFKipeUX
…it_env.py does the whole migration finbot reads only the environment now (python-dotenv is gone), so an install upgraded from the single .env, whose old unit had no EnvironmentFile=, started with no credentials or webhooks and ran silently deaf. While <checkout>/.env exists the bot now refuses to start, saying to run util/split_env.py and where the two files it writes go. util/split_env.py now reads the old dotenv format itself (export, spaces around =, quotes, trailing comments), so util/migrate_env.py is gone. With no argument it splits the checkout's .env, and it says what to do next. *.new (its output, which holds the credentials) is git-ignored. The README's upgrade note and by-hand run example no longer point at ./.env. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVEkZofdPjQ4jtXFKipeUX
…efusal stops the start Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVEkZofdPjQ4jtXFKipeUX
…0z2q # Conflicts: # README.md
robdevops
changed the title
robdevops
pushed a commit
that referenced
this pull request
Oct 14, 2026
… push #35 was opened for a CI tweak, took in the 14 review fixes pushed to the work branch afterwards, and merged under its old title. The new section: before each push, check for an open PR from the work branch and update its title and description to cover every commit it holds; never make such a PR mergeable while they are out of date. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVEkZofdPjQ4jtXFKipeUX
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
2 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.
Upgrade notes
.envin the checkout: finbot refuses to start while<checkout>/.envexists, because nothing reads it any more.util/split_env.py. It now reads the old dotenv format itself;util/migrate_env.pyis gone.finbot.envwhere your launcher loads it andpreferences.confin the data directory, then delete.env.@rebootlines are skipped and reported, like other unsupported lines.telegramOutgoingWebhook, the webhook is left alone.systemctl reload finbot, which signals the bot only. Job processes now ignore SIGHUP anyway.files.uploadis retired. Chart uploads came back in Slack chart replies via Slack's current upload API; a failed command stops its typing indicator #36, which needs thefiles:writeandfiles:readscopes.Fixes (numbered as in the review)
.holdingsreply loop. A Slack request also no longer overwrites the Telegram bot name.lib.telegrammakes no network calls, so job processes and tests no longer touch the webhook.<redacted>,'<' not supported…and similar.price.py Nruns. A single.price TICKER 7dgets a one-line error instead of a raw URL andsys.exitin a worker. A benchmark without history says so in its line of the performance report. A profile goes out without its history lines.finbot_jobs.jsonis reported once and no longer stops the bot.finbot.serviceandpreferences.confnow givesystemctl reload finbot.cachepreference across reloads; it no longer fetches every 5 seconds once cache is off. It now starts whenever Sharesight is configured..envrefusal and the mergedutil/split_env.py(dotenv format; splits the checkout's.envwhen run with no argument).*.newis git-ignored, since that output holds credentials. The README's upgrade note and by-hand example no longer point at./.env.Also included: README notes on the Telegram webhook (aa2364e), a test-fixture fix for typing-indicator threads a failed command left running (d4d332d), and the merge of the README edits from #33 and #34 (ecb094e).
CI (the change this PR was opened for)
PRs that change only
README.md,CLAUDE.mdorimg/*.pngskip install, compile and tests. The required checks still report success, and any git error fails the gate closed (8e06623, 010c0a5).Tests
226 → 255 tests, passing on Python 3.x and 3.13.5.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JVEkZofdPjQ4jtXFKipeUX