Fix the first sixty seconds: run.sh blocked every newcomer - #2
Merged
Conversation
Reviewed the published repo by cloning it and following the README verbatim, which is not something I had done before. Both commands it opens with are broken. `./run.sh --demo` crashed with "line 25: $2: unbound variable" — the script runs under `set -u` and passed "$2" through unconditionally. That is the first command in the README. `./run.sh` then refused to start at all without a config.json holding a broker account number. But config.json is needed to SYNC from a broker, not to run: the demo needs no broker, no account and no config. Anyone following the README hit a wall, and the only way through was to bypass run.sh and call app/server.py directly. A bare `./run.sh` now offers the two real choices — try the demo, or set up a broker — and starting without config.json works, saying only that broker sync is unavailable. `--help` lists --demo and --clear-demo, which it never did despite the README leading with them. CI passed through all of this because it called app/samples.py and app/server.py directly and never once invoked run.sh — testing around the entry point rather than through it. It now runs `./run.sh --help`, checks that a bare `./run.sh` points a newcomer at --demo, and runs `./run.sh --demo`, so this class of failure cannot go green again. Co-Authored-By: Claude Opus 5 <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.
Reviewed the published repo the way a stranger would — cloned it from GitHub and followed
the README verbatim. Both commands it opens with are broken.
What a newcomer hits today
./run.sh --democrashes. The script runs underset -uand passes"$2"throughunconditionally. This is the first command in the README.
./run.shthen refuses to start without aconfig.jsoncontaining a broker accountnumber. But
config.jsonis needed to sync from a broker, not to run: the demo needsno broker, no account, and no config. The only way through was to bypass
run.shand callapp/server.pydirectly — which is exactly what CI was doing, and why nobody noticed.Changes
"$2"→"${2:-}"../run.shwith no database and no config now offers the two real choices —try the demo, or set up a broker — instead of a dead end. Starting with a database
but no config now works, noting only that broker sync is unavailable.
--helplists--demoand--clear-demo, which it never did despite the READMEleading with them.
Why CI stayed green
It called
app/samples.pyandapp/server.pydirectly and never once invokedrun.sh—testing around the entry point rather than through it. This PR adds a step that runs
./run.sh --help, asserts a bare./run.shpoints a newcomer at--demo, and runs./run.sh --demo, so this class of failure cannot go green again.Verified
From a clean checkout with no config and no database:
16/16 XIRR tests,
scripts/check_clean.pyclean.