Uh oh!
There was an error while loading. Please reload this page.
docs(bugbot): add .cursor/BUGBOT.md project context - #399
Merged
Conversation
Cursor Bugbot reviews this repo with zero project context today — no repo in the org has a BUGBOT.md. On `client` Bugbot writes ~21.6x the human inline review volume, so it is effectively the whole code review here; every house rule it has to re-derive is a pass it doesn't spend on a hard finding. Encodes the invariants that recurring findings actually cluster on, each with the reason and a real reference in this repo: the `$CURL_SECURE` TLS floor being a constant rather than a wrapper (so new curl calls lose it silently), kubectl/helm/curl timeout conventions, version-before-URL validation, fail-closed guards (`detect_installed_client`), never reporting success you haven't verified (SEAL-CHECK), Helm nil-guards on new values keys across both `--reuse-values` and `--reset-then-reuse-values`, the `resource-policy: keep` stored-manifest trap that cost a PVC set, digest pinning incl. the prod-overlay CI blind spot, and the R8 manifest regeneration step. Also records verified non-issues so Bugbot stops re-reporting them — most importantly that `scripts/lib/*.sh` omit `set -euo pipefail` by design (sourced after install-k8s.sh sets it), the deliberate `set -uo pipefail` guard scripts, and the known cross-file SC2034 false positive. Item 4 of tracebloc/backend#930; improves on RFC 0001's Appendix A draft. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LukasWodka
commented
Jul 25, 2026
ContributorAuthor
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit fce3cd5. Configure here.
21 tasks
LukasWodka pushed a commit
that referenced
this pull request
Jul 27, 2026
… (backend#1252) `CURL_SECURE` was a bare constant every call site had to splice in by hand, so call sites kept losing it: seven live `curl` invocations ran with no minimum TLS version, including the POST in `verify_credentials()` that carries the client's password. These installs run on customer-managed hosts and behind TLS-inspecting proxies, which negotiate down to whatever the client permits — the reason this repo adopted an explicit floor instead of trusting curl's defaults. Add `curl_secure()` in `scripts/lib/common.sh` and route every fetch in `scripts/lib/*.sh` through it (18 call sites). The wrapper always passes `--tlsv1.2` and supplies default `--connect-timeout 30` / `--max-time 300`. Defaults are injected before `"$@"`, so a call site that wants a tighter bound still wins (curl honours the last occurrence), and a transfer that bounds itself with `--speed-limit`/`--speed-time` gets no injected `--max-time` — a hard deadline would fail a slow-but-healthy link on a large binary download. Every existing site therefore keeps its effective behaviour; seven gain the floor and nine previously unbounded ones gain a deadline. Also fixed while here: - `gpu-amd.sh` had the least-bounded curl usage in the repo — no TLS floor, no timeout, no retry. Both calls now go through the wrapper; the `.deb` download is retry-wrapped. The listing scrape deliberately is not: `retry()` reports attempts on stdout, which is that function's return value. - `install-k8s.ps1`'s WSL2 here-string had the same two nvidia-container-toolkit fetches bare. It cannot source `common.sh`, so it spells the flags out inline the way the bootstrap does. `scripts/install.sh` keeps its seven hardcoded literals: it is the trust root that fetches `common.sh`, so it cannot source the wrapper. `CURL_SECURE` stays defined and unchanged for out-of-tree callers, but nothing in the repo reads it now — the wrapper names the flag itself, so the constant can never silently reshape every fetch in the installer. Enforcement: an INTERIM third check in `scripts/check-style.sh` fails on a bare `curl`. tracebloc/.github#65 already implements this properly (a shell-aware lexer, not a grep) in a shared reusable workflow, but that workflow is not on `main` yet and cannot be referenced from here until it is. The check is marked for retirement the moment this repo adds that caller. Regenerated `scripts/manifest.sha256` (R8 supply-chain gate). Found in #399. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LukasWodka added a commit
that referenced
this pull request
Jul 27, 2026
… (backend#1252) (#400) * fix(installer): make the curl TLS floor structural, not per-call-site (backend#1252) `CURL_SECURE` was a bare constant every call site had to splice in by hand, so call sites kept losing it: seven live `curl` invocations ran with no minimum TLS version, including the POST in `verify_credentials()` that carries the client's password. These installs run on customer-managed hosts and behind TLS-inspecting proxies, which negotiate down to whatever the client permits — the reason this repo adopted an explicit floor instead of trusting curl's defaults. Add `curl_secure()` in `scripts/lib/common.sh` and route every fetch in `scripts/lib/*.sh` through it (18 call sites). The wrapper always passes `--tlsv1.2` and supplies default `--connect-timeout 30` / `--max-time 300`. Defaults are injected before `"$@"`, so a call site that wants a tighter bound still wins (curl honours the last occurrence), and a transfer that bounds itself with `--speed-limit`/`--speed-time` gets no injected `--max-time` — a hard deadline would fail a slow-but-healthy link on a large binary download. Every existing site therefore keeps its effective behaviour; seven gain the floor and nine previously unbounded ones gain a deadline. Also fixed while here: - `gpu-amd.sh` had the least-bounded curl usage in the repo — no TLS floor, no timeout, no retry. Both calls now go through the wrapper; the `.deb` download is retry-wrapped. The listing scrape deliberately is not: `retry()` reports attempts on stdout, which is that function's return value. - `install-k8s.ps1`'s WSL2 here-string had the same two nvidia-container-toolkit fetches bare. It cannot source `common.sh`, so it spells the flags out inline the way the bootstrap does. `scripts/install.sh` keeps its seven hardcoded literals: it is the trust root that fetches `common.sh`, so it cannot source the wrapper. `CURL_SECURE` stays defined and unchanged for out-of-tree callers, but nothing in the repo reads it now — the wrapper names the flag itself, so the constant can never silently reshape every fetch in the installer. Enforcement: an INTERIM third check in `scripts/check-style.sh` fails on a bare `curl`. tracebloc/.github#65 already implements this properly (a shell-aware lexer, not a grep) in a shared reusable workflow, but that workflow is not on `main` yet and cannot be referenced from here until it is. The check is marked for retirement the moment this repo adds that caller. Regenerated `scripts/manifest.sha256` (R8 supply-chain gate). Found in #399. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(installer): stall-bound the kubectl fetch, don't give it a deadline (Bugbot) `_fetch_kubectl` had no time bound at all, so routing it through `curl_secure` handed it the wrapper's default `--max-time 300`. kubectl is a ~50 MB binary, and this repo already documents (at `_fetch_k3d_release`, same file) that a fixed ceiling fails a slow-but-healthy link at that size — so the wrapper would have made every retry fail where the fetch previously completed. Give both fetches the same `--connect-timeout 15 --speed-limit 1024 --speed-time 60` as the k3d pair. That is also how `curl_secure` knows to skip its default deadline, and it is strictly better than before: the fetch was previously unbounded in both directions, so a mid-stream stall hung the step indefinitely. Audited the other 7 sites that now inherit the 300s default — get.docker.com, get-helm-3, the Homebrew script, stable.txt, the DMG checksum, the device-plugin manifest and the amdgpu-install package are all small text/script payloads. The only large downloads in the repo are kubectl, k3d and the Docker Desktop DMG; the latter two were already stall-bounded. Adds a bats test pinning it, since nothing covered `_fetch_kubectl` before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
LukasWodka added a commit
that referenced
this pull request
Jul 27, 2026
…ler (#401) * fix(installer): make the curl TLS floor structural, not per-call-site (backend#1252) `CURL_SECURE` was a bare constant every call site had to splice in by hand, so call sites kept losing it: seven live `curl` invocations ran with no minimum TLS version, including the POST in `verify_credentials()` that carries the client's password. These installs run on customer-managed hosts and behind TLS-inspecting proxies, which negotiate down to whatever the client permits — the reason this repo adopted an explicit floor instead of trusting curl's defaults. Add `curl_secure()` in `scripts/lib/common.sh` and route every fetch in `scripts/lib/*.sh` through it (18 call sites). The wrapper always passes `--tlsv1.2` and supplies default `--connect-timeout 30` / `--max-time 300`. Defaults are injected before `"$@"`, so a call site that wants a tighter bound still wins (curl honours the last occurrence), and a transfer that bounds itself with `--speed-limit`/`--speed-time` gets no injected `--max-time` — a hard deadline would fail a slow-but-healthy link on a large binary download. Every existing site therefore keeps its effective behaviour; seven gain the floor and nine previously unbounded ones gain a deadline. Also fixed while here: - `gpu-amd.sh` had the least-bounded curl usage in the repo — no TLS floor, no timeout, no retry. Both calls now go through the wrapper; the `.deb` download is retry-wrapped. The listing scrape deliberately is not: `retry()` reports attempts on stdout, which is that function's return value. - `install-k8s.ps1`'s WSL2 here-string had the same two nvidia-container-toolkit fetches bare. It cannot source `common.sh`, so it spells the flags out inline the way the bootstrap does. `scripts/install.sh` keeps its seven hardcoded literals: it is the trust root that fetches `common.sh`, so it cannot source the wrapper. `CURL_SECURE` stays defined and unchanged for out-of-tree callers, but nothing in the repo reads it now — the wrapper names the flag itself, so the constant can never silently reshape every fetch in the installer. Enforcement: an INTERIM third check in `scripts/check-style.sh` fails on a bare `curl`. tracebloc/.github#65 already implements this properly (a shell-aware lexer, not a grep) in a shared reusable workflow, but that workflow is not on `main` yet and cannot be referenced from here until it is. The check is marked for retirement the moment this repo adds that caller. Regenerated `scripts/manifest.sha256` (R8 supply-chain gate). Found in #399. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(installer): stall-bound the kubectl fetch, don't give it a deadline (Bugbot) `_fetch_kubectl` had no time bound at all, so routing it through `curl_secure` handed it the wrapper's default `--max-time 300`. kubectl is a ~50 MB binary, and this repo already documents (at `_fetch_k3d_release`, same file) that a fixed ceiling fails a slow-but-healthy link at that size — so the wrapper would have made every retry fail where the fetch previously completed. Give both fetches the same `--connect-timeout 15 --speed-limit 1024 --speed-time 60` as the k3d pair. That is also how `curl_secure` knows to skip its default deadline, and it is strictly better than before: the fetch was previously unbounded in both directions, so a mid-stream stall hung the step indefinitely. Audited the other 7 sites that now inherit the 300s default — get.docker.com, get-helm-3, the Homebrew script, stable.txt, the DMG checksum, the device-plugin manifest and the amdgpu-install package are all small text/script payloads. The only large downloads in the repo are kubectl, k3d and the Docker Desktop DMG; the latter two were already stall-bounded. Adds a bats test pinning it, since nothing covered `_fetch_kubectl` before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(installer): stop the ROCm package lookup from aborting the installer `_find_package_name` ran `curl … | grep … | head -1` as a single pipeline and returned its status. `install-k8s.sh` sources this lib under `set -euo pipefail`, so that pipeline could kill the installer two different ways: 1. A failed fetch (404, timeout, proxy block) made the command substitution non-zero, the caller's assignment inherited it, and `set -e` aborted BEFORE the friendly `[[ -z "$name" ]] && error "No amdgpu-install …"` on the next line could run. The user got a silent abort mid-GPU-step instead of an actionable message, and the RHEL major-version fallback was unreachable for the same reason. 2. `head -1` can close the pipe while grep is still writing, so grep takes SIGPIPE (141) and `pipefail` propagates that as a pipeline failure even though a filename WAS found. It only triggers when the directory index exceeds the pipe buffer, so it fails on large mirrors only. Capture the fetch and the match separately and let neither fail the function, then take the first match with `${var%%…}` so `head` leaves the pipeline entirely. The contract is unchanged — filename on stdout, nothing when not found — so emptiness remains the single signal all three callers already test. Adds scripts/tests/gpu-amd.bats (first coverage for this lib): the contract, both hazards, and a caller-shaped regression test under `set -euo pipefail`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: re-trigger standard-checks after base retarget to develop (#401) Retargeting the PR base from #400's merged branch to develop doesn't fire a pull_request event, so standard-checks (Unit tests + Lint) never ran on this head. Empty commit fires synchronize so the required checks run against develop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
saadqbal
approved these changes
Jul 27, 2026
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Summary
Adds
.cursor/BUGBOT.md— project context for the Cursor Bugbot reviewer. Zero repos in the org have one today, so Bugbot reviews every repo blind. This is item 4 of tracebloc/backend#930 and RFC 0001's own highest-value change.On this repo it matters most: Bugbot writes roughly 21.6x the human inline review volume here, so it is effectively the entire code review. Every house rule it re-derives from scratch is a pass not spent on a genuinely hard finding.
What's in it
Rules are grounded in this repo, not generic security advice — each carries the reason and a real reference:
$CURL_SECUREis a constant, not a wrapper (scripts/lib/common.sh:10), so a newcurlcall silently loses the--tlsv1.2floor. Lists the sites already missing it.kubectl --request-timeout=5s;helmhas none, so gate it behind a boundedcluster-infoprobe;curlneeds connect + max-time or the stall pair.detect_installed_client()setsINSTALLED_CLIENT_UNKNOWN=1rather than "no client here", because failing open lets a re-install overwrite a live client.values.yamlkeys, and why both--reuse-valuesand--reset-then-reuse-valuespaths have to survive.helm.sh/resource-policy: keepstored-manifest trap that cost a production PVC set.ingestor-multiarchdoes not read the prod overlay.Plus a verified known-non-issues section so Bugbot stops re-reporting false positives — chiefly that
scripts/lib/*.shomitset -euo pipefailby design (they are sourced afterinstall-k8s.sh:44sets it), the four guard scripts that deliberately drop-e, and the cross-file SC2034 "unused variable" class that CI already treats as advisory.Type
Docs / DevEx. No code or chart changes.
Test plan
client_loggervsprint_and_logwas dropped after confirming those symbols do not exist in this repo.Checklist
develop.cursor/BUGBOT.mdin this PR🤖 Generated with Claude Code
Note
Low Risk
Documentation-only; no runtime, chart, or installer behavior changes.
Overview
Adds
.cursor/BUGBOT.md, a Cursor Bugbot playbook for this repo so automated reviews stop re-deriving house rules from scratch.The doc frames the product (Helm charts + verified
curl | bashinstaller on customer-operated clusters) and tells Bugbot to prioritize operator-visible failures over style. Always flag covers installer/chart pitfalls with file:line anchors: entry-pointset -euo pipefailand the[[ -f x ]] && sourcetrap, per-callCURL_SECURE/ boundedkubectl/curl/helmprobes, ref validation before URL interpolation, fail-closed guards, no false “ready” without verification, nil-safe newvalues.yamlkeys across reuse vs reset upgrade paths, theresource-policy: keepstored-manifest gotcha, digest pinning and prod-overlay CI blind spots, credential hygiene,manifest.sha256regeneration, andChart.yamlversion/appVersionpairing.A known non-issues section suppresses repeat noise (sourced libs without their own
set -e, guard scripts without-e, SC2034 cross-file, generated artifacts,clientIdin diagnose bundles). Tone rules keep findings concrete and public-safe.Reviewed by Cursor Bugbot for commit fce3cd5. Bugbot is set up for automated code reviews on this repo. Configure here.