Conversation
Notebook changes currently merge without anything having executed the code in them. This adds a `Run Changed Notebooks` workflow that converts every notebook whose *code cells* changed into a Python script (reusing the same nbconvert config as the docs conversion), submits it as a Wherobots job run on a `tiny` runtime, and fails the check if the run does not complete. Details worth calling out: - The diff is on code cell source only, so prose, heading and image edits do not spend compute. - Failures are reported as a sticky PR comment carrying the job run id and the notebook's own traceback -- JVM frames and the `run_submit.py` harness traceback are filtered out so the author sees their error, not Spark's. - A superseded push cancels the in-flight workflow, and the cancel step stops any job runs already submitted so they stop billing. - `scala/` notebooks are excluded; they run on a different kernel. Requires a `WHEROBOTS_API_KEY` repository secret before the gate can pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review these changes at https://app.gitnotebooks.com/wherobots/wherobots-examples/pull/206 |
Notebook job runsThe notebook gate did not complete (job status: Workflow run · commit |
![salty-hambot[bot]](https://avatars.githubusercontent.com/in/2952321?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed by Salty Hambot 🤖🧂 — rubric mode
Verdict: ❌ fail
📋 Requested evidence
- Confirm whether wherobots-cli publishes a SHA256 or signature for its release assets so the download can be verified.
- Clarify the intended flow for fork-based community PRs given pull_request withholds secrets — is a maintainer-approved trusted run expected?
| Dimension | Verdict | Notes |
|---|---|---|
| correctness | ❌ fail | Comment-ownership PATCH 403s on foreign markers and rename-only notebooks wrongly execute. |
| security | ❌ fail | Unauthenticated CLI binary is chmod'd and later run with the API key — supply-chain gap. |
| privacy | ✅ pass | No PII handled; only run ids and job logs surfaced in PR comments. |
| reliability | Fork PRs fail the gate on empty secrets; silent poll failures masquerade as TIMED_OUT. | |
| scalability | Rename-only PRs trigger needless compute; otherwise parallelism and paging are sensible. | |
| observability | Swallowed poll errors leave undiagnosable timeouts with no stderr breadcrumb. | |
| clarity/maintainability | ✅ pass | Well-documented script and CONTRIBUTING.md; intent is clear throughout. |
| test quality | No automated tests for the diff/parsing logic; comment script isn't in the workflow trigger paths. |
Solid notebook-gate plumbing, but two blockers will bite: the sticky-comment lookup PATCHes any comment carrying the marker (403 on a foreign author) and fork PRs die at empty secrets despite the 'please contribute' welcome mat. Plus an unverified CLI binary handed your API key, and a rename that burns compute it shouldn't.
7 finding(s) posted.
💰 Review cost: $1.6559 · 510.8k in / 19.9k out tokens · ⏱️ 4m3s
💬 To request a re-review, comment @salty-hambot review
|
|
||
| # Nothing changed that needed a run, and no prior comment to update: stay quiet. | ||
| existing=$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR}/comments" --paginate \ | ||
| --jq "[.[] | select(.body | contains(\"$MARKER\")) | .id] | first // empty") |
There was a problem hiding this comment.
issue (blocking): Any PR commenter can preempt the sticky marker, causing the workflow to PATCH a comment it does not own and fail with a 403.
The --jq filter selects any comment whose body contains the marker, regardless of author. Filter the lookup to comments authored by github-actions[bot] (or the token's actual bot identity); otherwise one <!-- notebook-job-run-gate --> in a user comment turns a successful notebook run red. Tiny HTML comment, surprisingly large blast radius.
| wherobots --version | ||
|
|
||
| - name: Run changed notebooks | ||
| env: |
There was a problem hiding this comment.
issue (blocking): Notebook PRs from forks cannot pass this gate because pull_request workflows do not receive repository secrets.
GitHub supplies an empty WHEROBOTS_API_KEY for forked PRs, so the script exits 2 before inspecting the diff, and the comment step also lacks write permission on those PRs. Since CONTRIBUTING.md explicitly invites community contributions, provide a safe maintainer-approved trusted-run path rather than executing unreviewed fork code via pull_request_target.
| - name: Install Wherobots CLI | ||
| run: | | ||
| curl -fsSL -o /usr/local/bin/wherobots \ | ||
| "https://github.com/wherobots/wherobots-cli/releases/download/${WHEROBOTS_CLI_VERSION}/wherobots_linux_amd64" |
There was a problem hiding this comment.
issue (blocking, security): The workflow downloads and executes an unauthenticated CLI binary via curl before later invoking it with WHEROBOTS_API_KEY.
There's no checksum or signature check before the binary is chmod +x'd and run. A replaced or compromised release asset can install a wrapper that captures the secret in the next step. Verify a pinned SHA256 or a release signature before chmod +x; otherwise this is supply-chain roulette with production credentials as the ante.
| def code_changed(path: str, base: str) -> bool: | ||
| """True if the notebook is new, or if any code cell's source differs from base.""" | ||
| before = run(["git", "show", f"{base}:{path}"], cwd=REPO_ROOT) | ||
| if before.returncode != 0: |
There was a problem hiding this comment.
issue: A markdown-only notebook rename is treated as a new file and submitted for execution.
git show ${base}:${path} looks up the destination path, which does not exist in the base commit, so a pure rename returns True even when every code cell is identical. Parse rename source/destination from git diff --name-status -M and compare against the source path to preserve the stated "only code changes cost compute" behavior.
| time.sleep(interval) | ||
| proc = wb("api", "runs", "get-job-run", "--run-id", run_id) | ||
| if proc.returncode != 0: | ||
| continue # transient API error; keep polling until the deadline |
There was a problem hiding this comment.
suggestion (non-blocking): Transient polling failures are swallowed with no log line (continue # transient API error). If the Wherobots API starts erroring consistently (expired token, outage), this polls silently for up to timeout + 600 seconds and then reports a plain TIMED_OUT — indistinguishable from a genuinely slow job, and the following log-fetch step will likely fail the same way, leaving the author staring at "(no logs were returned for this run)" with zero clue why. A print(..., file=sys.stderr) on each failed poll (or at least the last error) would turn a mystery into a one-line diagnosis.
| pull_request: | ||
| paths: | ||
| - '**/*.ipynb' | ||
| - '.github/workflows/test-changed-notebooks.yml' |
There was a problem hiding this comment.
nitpick: The paths trigger list doesn't include .github/workflows/scripts/comment_notebook_results.sh. A PR that only touches the commenting script won't trigger this workflow to exercise it — Schrödinger's shell script, untested until a notebook change happens to ride along.
| # The CLI prints upload progress before the JSON payload, so take the last | ||
| # line that parses as an object carrying an id. | ||
| for line in reversed(proc.stdout.splitlines()): | ||
| line = line.strip() |
There was a problem hiding this comment.
question: submit() finds the run id by scanning stdout in reverse for the first line that parses as JSON with a truthy id field. If the CLI's progress output (or a future version of it) ever emits an unrelated object carrying an id key before the real payload, this would silently grab the wrong run id and start polling something that isn't the job you submitted. Is there a more structured way to get the run id (e.g. --output json guaranteed to be the only stdout, or a dedicated --quiet flag)?
|
note that this is a public repo and we plan to make it private soon. We already have staging and prod integration tests on almost every notebook. it runs every day or every time when we push something to staging |
Okay, closing then |
2 participants
What this does
Notebook changes currently merge without anything having executed the code in them. This adds a Run Changed Notebooks workflow that actually runs them.
For each notebook whose code cells changed, the workflow:
.github/workflows/config/nbconvert_config.py).tinyruntime.A notebook merges only after the code in it has been executed.
Files
.github/workflows/test-changed-notebooks.yml.github/workflows/scripts/run_changed_notebooks.py.github/workflows/scripts/comment_notebook_results.shCONTRIBUTING.mdDetails worth reviewing
run_submit.pyharness reporting a non-zero exit. The report prefers the traceback naming the converted script, drops interleaved JVM frames, and truncates at the exception line, then appends the harness's exit-code line.concurrency. Submitted run ids are recorded torun_ids.txtas they are created, and a step gated oncancelled() || failure()cancels any that are stillPENDING/RUNNING.SedonaKepler,SedonaPyDeck,create_map,gdf.plotetc.; a notebook that converts to no executable statements is reported as skipped.scala/notebooks are excluded — different kernel, submitted as JARs rather than scripts.Before this can pass
The repo needs a
WHEROBOTS_API_KEYsecret. Please use a service principal key rather than a personal one, so the gate doesn't break when someone leaves the team. Until the secret exists the script exits 2 with a clear message.Reproducing locally
export WHEROBOTS_API_KEY=... python .github/workflows/scripts/run_changed_notebooks.py --base-sha main --runtime tinyTesting notes
Scripts are syntax-clean and the workflow YAML parses; the end-to-end path can't be exercised until the secret is in place. This PR touches no notebooks, so the new gate will report "no job runs were needed" on itself — worth merging a trivial notebook change afterward to see it fire for real.
🤖 Generated with Claude Code