Skip to content

chore: clear house-rules findings - #426

Merged
LukasWodka merged 2 commits into
developfrom
chore/house-rules-clean
Jul 29, 2026
Merged

chore: clear house-rules findings#426
LukasWodka merged 2 commits into
developfrom
chore/house-rules-clean

Conversation

@LukasWodka

@LukasWodka LukasWodka commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Clears all 9 house-rules findings in this repo, measured at develop HEAD with the shared checker (tracebloc/.github scripts/house-rules.sh --all). Part of tracebloc/backend#1303.

Findings → fixes

Finding Fix
scripts/install.sh:179 curl-timeout (cosign binary) --connect-timeout 30 --max-time 300 — matches the client installer's identical cosign-bootstrap curls
scripts/install.sh:180 curl-timeout (cosign checksums) --connect-timeout 30 --max-time 300
scripts/install.sh:205 curl-timeout (HEAD tag resolve) --connect-timeout 30 --max-time 30 (tiny header-only request)
scripts/install.sh:268 curl-timeout (binary download) --connect-timeout 30 --max-time 300
scripts/install.sh:274 curl-timeout (SHA256SUMS) --connect-timeout 30 --max-time 300
scripts/install.sh:348 curl-timeout (.sig + .cert, both curls on the logical line) --connect-timeout 30 --max-time 300 on each
scripts/sync-backend-fixtures.sh:137 curl-timeout --connect-timeout 10 --max-time 60 — tighter than the installer values on purpose: this runs 18× per CI check run, so a generous per-file ceiling would compound
scripts/sync-schema.sh:116 curl-timeout --connect-timeout 10 --max-time 60 (same rationale)
scripts/tests/install-verify.sh:27 pipefail (sha256sum | awk) set -uset -uo pipefail

No waivers used — every finding fixed for real.

pipefail judgment call

install-verify.sh deliberately has no set -e (it counts pass/fail itself), so adding pipefail only changes $? propagation. I eyeballed every pipeline in the file for legs expected to fail: sha256sum|awk, shasum|awk, uname|tr, and ls|grep|grep|head (the tamper test, where the sandbox guarantees a match) — none has a leg that legitimately exits non-zero on the green path, and no pipeline's exit status feeds a set -e abort. A comment at the top documents why -e stays off.

Test plan

  • house-rules.sh --all0 findings (was 9)
  • sh -n / bash -n on all four edited scripts
  • scripts/tests/install-verify.sh12 passed, 0 failed (the harness's mock curl tolerates the new flags; full installer flow exercised end-to-end)

🤖 Generated with Claude Code


Note

Low Risk
Installer and CI fetch behavior only; no auth or release verification logic changes beyond how long curl may run.

Overview
Addresses house-rules curl-timeout findings by centralizing release/cosign downloads in install.sh behind a new dl() helper: TLS 1.2, 30s connect timeout, and stall detection (--speed-limit 1024 / --speed-time 60) instead of a fixed wall-clock cap so large binaries can finish on slow links while hung connections still abort. Cosign bootstrap, CLI binary, SHA256SUMS, and .sig/.cert fetches all use dl(); resolving latest via HEAD keeps a short --max-time 30.

sync-backend-fixtures.sh and sync-schema.sh add --connect-timeout 10 --max-time 60 on their GitHub fetches so CI cannot wedge on a stuck endpoint (tighter than the installer because many files run per job).

install-verify.sh switches to set -uo pipefail so a failing stage in pipelines like sha256sum | awk is not masked, without enabling set -e.

Reviewed by Cursor Bugbot for commit 5c22f8c. Bugbot is set up for automated code reviews on this repo. Configure here.

Fix every finding the shared org checker (tracebloc/.github
scripts/house-rules.sh) reports at develop HEAD: missing curl
timeouts/TLS floors, plus (cli) a missing pipefail. Waivers only where
the finding is a documented false positive. Part of tracebloc/backend#1303.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@LukasWodka LukasWodka self-assigned this Jul 29, 2026
@LukasWodka

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 c9ab43b. Configure here.

Comment thread scripts/install.sh
Comment thread scripts/install.sh Outdated
…(review)

--max-time 300 made the ~50MB binary fail under ~1.4 Mbps and the ~90MB
cosign bootstrap under ~2.6 Mbps -- slow-but-alive links must be allowed
to finish. dl() replaces the wall-clock cap with --speed-limit 1024
--speed-time 60 (abort only when under 1 KiB/s for 60s straight = dead
connection), keeps the TLS 1.2 floor + --connect-timeout 30, and
deduplicates the flag string across all six body fetches. The HEAD
tag-resolve keeps its 30s cap (header-only, wall-clock is right there).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@saadqbal

Copy link
Copy Markdown
Collaborator

I feel like 30s timeout is too less time. I will approve it anyways.

@LukasWodka

Copy link
Copy Markdown
Contributor Author

@saadqbal on the 30s — worth being precise about what it bounds: --connect-timeout 30 caps only the TCP+TLS handshake (reaching github.com and completing the hello), not the download. Transfer time is now completely uncapped after your earlier review — a 50MB pull on a 200 kbps line can take 35 minutes and succeeds; only a stalled transfer (<1 KiB/s for 60s straight) aborts. A handshake that hasn't completed in 30s essentially never completes — it means blackholed routing or a dead proxy, which is exactly what we want to fail fast instead of hanging the bootstrap (the pre-fix behavior). Proxy environments do their slowness in the transfer phase, which is unbounded.

That said, if you'd still rather have 60: it's now a one-line change in dl() — say the word and I'll include it in the next commit that touches this file rather than burning your approval on a value tweak.

@LukasWodka
LukasWodka merged commit d972cdc into develop Jul 29, 2026
26 checks passed
@LukasWodka
LukasWodka deleted the chore/house-rules-clean branch July 29, 2026 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants