From bad585ef3fba34e505dec69a230295d8704237d1 Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Wed, 29 Jul 2026 10:38:19 +0200 Subject: [PATCH 1/7] ci: add code-quality caller workflow (advisory) (#420) Co-authored-by: Claude Fable 5 --- .github/workflows/code-quality-caller.yml | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 .github/workflows/code-quality-caller.yml diff --git a/.github/workflows/code-quality-caller.yml b/.github/workflows/code-quality-caller.yml new file mode 100644 index 0000000..584c557 --- /dev/null +++ b/.github/workflows/code-quality-caller.yml @@ -0,0 +1,22 @@ +name: Code quality + +on: + pull_request: + types: [opened, reopened, synchronize, ready_for_review] + +# Supersede the previous run when a branch is pushed again. Measured: +# workflows missing this stack ~10-minute duplicate runs per push. +concurrency: + group: code-quality-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + quality: + uses: tracebloc/.github/.github/workflows/code-quality.yml@main + with: + python: true # repos with Python + shell: true # repos with shell scripts + # soft-fail: false # flip once the backlog is clear From 97384760b0e078de9ce4a034353422f8fd1f61e5 Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Wed, 29 Jul 2026 10:38:46 +0200 Subject: [PATCH 2/7] ci: wire golangci-lint + gosec into CI (advisory); migrate config to v2 (#423) * ci: migrate .golangci.yml to v2 format and enable gosec golangci-lint v1 is EOL and cannot typecheck this module (go.mod says go 1.26.0), and v2 binaries -- including what brew ships -- refuse v1-format configs, so make lint-full was broken for fresh installs. Migrated via `golangci-lint migrate` with the narrative comments preserved; same linter set (still no SSA linters, per #6), plus gosec for insecure-pattern scanning of our own code. Test files are gosec/errcheck-exempt per convention (12 additional findings fire there, all fixed-temp-path/perms test idioms). Part of tracebloc/backend#1305 (epic #930, Layer 1). * ci: run golangci-lint + gosec in CI (advisory, not required) The config existed but nothing in CI loaded it. Official action pinned v9.3.0, golangci-lint pinned v2.12.2 (built with Go 1.26). Advisory by design: not in branch protection, and continue-on-error at the job level so the pre-existing 8-finding gosec backlog does not red-X unrelated PRs -- comes off at the required-flip after cleanup (backend#1303 pattern). Part of tracebloc/backend#1305 (epic #930, Layer 1). * ci: advisory mode via --issues-exit-code=0, not continue-on-error First run on #423 confirmed the quirk: job-level continue-on-error greens the workflow RUN but the job check run still red-Xs in the PR checks list. --issues-exit-code=0 gives the intended semantics: findings -> green job with inline annotations; infrastructure breakage (bad config, typecheck failure) -> still fails. Remove the arg at the required-flip (backend#1303 pattern). * ci: document the typecheck blind spot of advisory mode accurately Bugbot on #423: typecheck findings ride the issues exit path, so --issues-exit-code=0 greens them too -- the previous comment wrongly claimed typecheck failures still fail the job. Verified empirically (broken type: exit 0 with the flag, 1 without). Comment now states the real containment: required Test/Lint/Build jobs red a non-compiling PR, and a broken config still fails this job via the config-verify pre-step. Behavior unchanged. --- .github/workflows/golangci.yml | 83 ++++++++++++++++++++++++++++++++ .golangci.yml | 88 ++++++++++++++++++++++++---------- 2 files changed, 147 insertions(+), 24 deletions(-) create mode 100644 .github/workflows/golangci.yml diff --git a/.github/workflows/golangci.yml b/.github/workflows/golangci.yml new file mode 100644 index 0000000..b44810a --- /dev/null +++ b/.github/workflows/golangci.yml @@ -0,0 +1,83 @@ +name: golangci-lint + +# Runs golangci-lint (with the gosec security linter) against the +# repo's .golangci.yml on every PR + push to develop/main. This is the +# "one tool, one config" successor being sized up for the standalone +# lint steps in build.yml, and the first thing in this repo that scans +# our own code for insecure patterns — govulncheck (build.yml + +# vulncheck.yml) only covers known CVEs in dependencies. +# +# Why the action is safe now: the golangci-lint-action timeout story +# (#6) was the SSA linters (staticcheck, unused) choking on the +# k8s.io/* dep tree. The current .golangci.yml enables no SSA linters; +# a full run measures ~11s wall locally / ~62s on the runner (#423). +# +# Part of backend#1305 (epic #930, Layer 1). + +on: + push: + branches: [develop, main] + pull_request: + branches: [develop, main] + +permissions: + contents: read + +concurrency: + group: golangci-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +jobs: + golangci: + timeout-minutes: 10 + name: golangci-lint (advisory) + runs-on: ubuntu-latest + # ============================ ADVISORY ============================ + # DELIBERATELY not a required check, and `--issues-exit-code=0` on + # the run step keeps findings from red-Xing the job: golangci-lint + # exits non-zero on any finding by default, and the pre-existing + # gosec backlog (8 findings at introduction: 4x G204, 3x G304, + # 1x G115 — sized on #423, the PR that added this job) would + # otherwise fail every PR for issues it didn't introduce. Findings + # still surface as inline annotations and in the job log. + # + # Known blind spot while advisory: typecheck (compile) errors ride + # the same issues exit path as lint findings, so under this flag + # they exit 0 too (verified empirically on #423 — this is NOT a + # separate exit code). No real signal is lost: a non-compiling PR + # reds the required Test / Lint / Build jobs anyway, and a broken + # .golangci.yml still fails THIS job via the action's + # `golangci-lint config verify` pre-step, which --issues-exit-code + # does not touch. + # + # (Job-level `continue-on-error: true` is NOT the tool for this — + # it greens the workflow run but still shows the job itself as + # failed in the PR checks list; see the first run on #423.) + # + # REMOVE the `--issues-exit-code=0` arg when the backlog hits zero + # and this check flips to required (the advisory -> required + # pattern from backend#1303; tracked under backend#1305 / epic + # #930). A required check that can't fail is worse than no check. + # ================================================================== + steps: + - uses: actions/checkout@v7 + + - name: Set up Go + uses: actions/setup-go@v7 + with: + go-version-file: go.mod + cache: true + + # Both versions pinned for reproducibility, same policy as the + # standalone tools in build.yml (#127). golangci-lint v2.12.2 is + # built with Go 1.26 (required: go.mod says `go 1.26.0`; v1-era + # binaries can't typecheck this module). Bump deliberately, and + # keep the version in step with the format expectations noted in + # .golangci.yml. + - name: golangci-lint run (.golangci.yml) + uses: golangci/golangci-lint-action@v9.3.0 + with: + version: v2.12.2 + # ADVISORY MODE — see the block comment above (incl. the + # typecheck caveat). Remove at the required-flip. + args: --issues-exit-code=0 diff --git a/.golangci.yml b/.golangci.yml index a6d52e7..d5976f5 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -4,9 +4,24 @@ # bugs without flooding PRs with style noise. Tune up over time # rather than turning everything on day one and quarantining half of # them. +# +# Format: golangci-lint v2 (`version: "2"`). The v1-format file died +# with golangci-lint v1: v1 binaries are EOL and predate Go 1.26, so +# they can't typecheck this module at all, and v2 binaries (what +# `brew install golangci-lint` ships) refuse v1 configs. Migrated via +# `golangci-lint migrate` (backend#1305, epic #930 Layer 1); needs +# golangci-lint >= v2. +# +# CI: run by the advisory `golangci-lint` job in +# .github/workflows/golangci.yml (pinned version there; keep it in +# lockstep when the format needs a newer binary). The old +# action-times-out story (#6) was about the SSA linters (staticcheck, +# unused) on the k8s.io dep tree — this set has none, so the action +# is safe with it. + +version: "2" run: - timeout: 5m # Track go.mod's `go` directive. Go's release cadence + `go mod # tidy`'s aggressive bumping (especially when k8s.io/* deps want # newer Go) keep dragging go.mod's minimum up; pinning a stale @@ -17,42 +32,67 @@ run: go: "1.26" linters: - disable-all: true + default: none enable: # The set is trimmed to the linters that DON'T do full whole-program # SSA analysis. `staticcheck` and `unused` were in the original set # and reproducibly caused the GitHub-hosted runner to time-budget # the job (~2 min then shutdown signal) on this module — k8s.io/* # transitive deps inflate the analysis graph enough to OOM-or-stall - # the standard 4-CPU/16GB runner. Re-enabling them is a v0.2 - # follow-up that needs either a larger runner, a much narrower - # scope (e.g. only `./internal/...`), or a faster successor like - # govulncheck for the security-only subset. + # the standard 4-CPU/16GB runner (#6). staticcheck now runs + # standalone in build.yml's Lint job instead. # Cheap, per-file checks — catch real bugs without SSA. - errcheck # unchecked error returns - govet # `go vet` - ineffassign # assignments that go nowhere + # Security: insecure code patterns (G1xx-G6xx) — command injection, + # path traversal, weak crypto, world-writable files. Complements + # govulncheck (known CVEs in deps) with our-own-code checks; this is + # a customer-installed binary that shells out and writes to disk, so + # both halves matter (backend#1305, epic #930 Layer 1). + - gosec + # Style / hygiene that pays for itself in code review time. - - gofmt - - goimports - misspell - unconvert # unnecessary type conversions -linters-settings: - goimports: - # Group imports: stdlib, third-party, our own. Keeps diffs - # readable when adding new imports. - local-prefixes: github.com/tracebloc/cli - -issues: - # Default exclusions hide a lot of real findings — opt back in. - exclude-use-default: false - - exclude-rules: - # Test files often deliberately ignore err returns from - # bytes.Buffer / strings.Builder / fmt.Fprintf, which never fail. - - path: _test\.go - linters: - - errcheck + exclusions: + # v1 had `exclude-use-default: false` — the default exclusions hide + # a lot of real findings, so keep opting back in (no presets). + generated: lax + rules: + # Test files often deliberately ignore err returns from + # bytes.Buffer / strings.Builder / fmt.Fprintf, which never fail. + # gosec is likewise test-exempt per convention: tests use fixed + # temp paths, os.Setenv, and relaxed perms that G-rules flag but + # that never ship in the binary. + - path: _test\.go + linters: + - errcheck + - gosec + paths: + - third_party$ + - builtin$ + - examples$ + +# v2 moved the formatters out of `linters`. Same tools as before — +# build.yml's Lint job runs the standalone equivalents (`gofmt -s`, +# `goimports -local`). +formatters: + enable: + - gofmt + - goimports + settings: + goimports: + # Group imports: stdlib, third-party, our own. Keeps diffs + # readable when adding new imports. + local-prefixes: + - github.com/tracebloc/cli + exclusions: + generated: lax + paths: + - third_party$ + - builtin$ + - examples$ From 044b0d9d3c3761619fb9555ba6b02a327632bc6d Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Wed, 29 Jul 2026 10:39:05 +0200 Subject: [PATCH 3/7] docs: add Bugbot resolve-and-reply team norm to .cursor/BUGBOT.md (#421) * docs: add Bugbot resolve-and-reply team norm to .cursor/BUGBOT.md Part of tracebloc/backend#1308 Co-Authored-By: Claude Fable 5 * docs: point CONTRIBUTING.md at the Bugbot findings norm Part of tracebloc/backend#1308 Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Claude Fable 5 --- .cursor/BUGBOT.md | 8 ++++++++ CONTRIBUTING.md | 4 ++++ 2 files changed, 12 insertions(+) diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md index e4e67b7..df9d63c 100644 --- a/.cursor/BUGBOT.md +++ b/.cursor/BUGBOT.md @@ -125,3 +125,11 @@ consequence — what they see, and which exit code they get — not just the cod This repo is **public**: never put a customer name, internal hostname, or internal-only ticket detail in a finding. A bare `tracebloc/backend#NNNN` reference is fine. + +## Working with Bugbot findings (team norm) + +Every Bugbot review thread gets a reply, then gets resolved: +- **Fixed**: say what changed and in which commit. +- **False positive**: say why, with evidence (file/line, measured behavior). +Unresolved cursor threads HOLD release-train promotions (soft gate) — an +unaddressed finding blocks the fleet, not just this PR. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 15123e4..2fe3a99 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -105,3 +105,7 @@ Coverage says a line *ran*; mutation testing says a test would *fail* if the lin 4. File one issue per real gap, titled `test(): pin (mutation survivor)`, quoting the gremlins line (mutant type + file:line) and what behavior the missing test must pin. That issue then flows through the kanban like any other test ticket — #262, #263, #264 are the pattern. Survivors are *findings to triage*, not build failures — the workflow stays green even when mutants live, on purpose. + +## Bugbot findings + +See `.cursor/BUGBOT.md` — every thread gets a reply (fixed / false-positive-with-evidence), then resolved. From cb88ff0350adcef44c0690b5c84c368e8e9d249a Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Wed, 29 Jul 2026 11:34:30 +0200 Subject: [PATCH 4/7] feat(data delete): reap ingestor bookkeeping rows with the table (RFC-0003 I6, backend#1209) (#424) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(data delete): reap the ingestor's bookkeeping rows with the table (RFC-0003 I6, backend#1209) Dropping a table stranded its run-journal rows (tracebloc_ingest_runs) and pseudonymization-salt row (tracebloc_ingest_meta). Under per-ingestion tables (data-ingestors#408) every dataset is its own table, so every delete would leak one husk row of each kind, unbounded. Teardown now DELETEs both best-effort after the DROP — separately per bookkeeping table (either may be absent on clusters that never ran a journal-aware ingestor), never failing a teardown whose DROP succeeded (TeardownResult.BookkeepingCleaned reports it). plan.Table has passed ValidateTableName, so it cannot escape the quoted literal. Benefits legacy label tables identically. Co-Authored-By: Claude Fable 5 * fix(teardown): feed bookkeeping SQL on stdin — shell quoting ate the string literal (Bugbot, High) The DELETEs embedded a single-quoted SQL literal inside a single-quoted sh -c string: the shell stripped the inner quotes, mysql saw an unquoted identifier, and the best-effort cleanup silently no-opped forever — exactly the leak this PR exists to stop. SQL now rides stdin (the runMySQLQuery pattern), sidestepping shell quoting entirely. The recording executor now captures stdin, and the test asserts the quoted literal arrives intact AND that no DELETE ever appears as a shell argument — pinning the whole bug class, not just this instance. Co-Authored-By: Claude Fable 5 * chore: goimports grouping in teardown_test (CI lint) Co-Authored-By: Claude Fable 5 * fix(teardown): surface bookkeeping failures + reuse runMySQLQuery + pin the column contract (review) 1. Observability: TeardownResult gains BookkeepingErrs (per-table failure with mysql stderr folded in via runMySQLQuery); data delete prints a warning on incomplete cleanup and --output-json gains bookkeeping_cleaned — schema drift is now diagnosable in the field instead of collapsing into a silent false. 2. Column contract pinned in a comment against data-ingestors database.py: both bookkeeping tables key by table_name (tracebloc_ingest_runs indexed, tracebloc_ingest_meta PK). 3. The inline stdin exec is gone — DELETEs ride runMySQLQuery; its error prose neutralized to 'running mysql query' (the 'querying datasets' wording lives on only in list.go's own exec, whose test asserts it). Co-Authored-By: Claude Fable 5 * fix(overwrite): surface bookkeeping-cleanup failures on the ingest pre-clean too (Bugbot) + JSON cosmetic (review) data ingest --overwrite runs the identical teardown but discarded the result — a bookkeeping failure printed unconditional success, hiding on this path the exact schema-drift signal data delete now surfaces. The overwrite pre-clean warns the same way. Also: dry-run/declined emit bookkeeping_cleaned=false (nothing was attempted — a strict consumer must never read 'cleanup happened' out of a run that deleted nothing). Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Claude Fable 5 --- internal/cli/data.go | 10 +- internal/cli/data_delete.go | 34 ++++-- .../cli/testdata/golden/zz-all-strings.golden | 5 + internal/push/list_detailed.go | 2 +- internal/push/teardown.go | 45 ++++++++ internal/push/teardown_test.go | 102 ++++++++++++++++++ 6 files changed, 185 insertions(+), 13 deletions(-) diff --git a/internal/cli/data.go b/internal/cli/data.go index 98421b8..cc7f8b8 100644 --- a/internal/cli/data.go +++ b/internal/cli/data.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "strings" "github.com/spf13/cobra" @@ -165,7 +166,7 @@ func runDataIngest(ctx context.Context, out, errOut io.Writer, a runDataIngestAr // DROP/rm and then "succeed". plan := push.PlanTeardown(existingTable) rmSpin := a.Printer.Spinner(fmt.Sprintf("Removing the existing %q first", existingTable), "") - _, terr := push.Teardown(ctx, cs, &push.SPDYExecutor{Config: resolved.RestConfig, Client: cs}, resolved.Namespace, plan, push.PodSpecOptions{ + tres, terr := push.Teardown(ctx, cs, &push.SPDYExecutor{Config: resolved.RestConfig, Client: cs}, resolved.Namespace, plan, push.PodSpecOptions{ Namespace: resolved.Namespace, PVCClaimName: pvc.ClaimName, PVCMountPath: pvc.MountPath, @@ -185,6 +186,13 @@ func runDataIngest(ctx context.Context, out, errOut io.Writer, a runDataIngestAr "first, then re-run this ingest. Nothing new was staged. (%w)", existingTable, existingTable, terr)} } + if !tres.BookkeepingCleaned { + // Same surfacing `data delete` does (Bugbot on the PR): the + // overwrite pre-clean runs the identical teardown, and a silent + // bookkeeping failure here would hide the same schema-drift + // regression on this path. + a.Printer.Warnf("Bookkeeping cleanup incomplete — the old table is gone, but its run-journal/salt rows may remain: %s", strings.Join(tres.BookkeepingErrs, "; ")) + } a.Printer.Successf("Removed the old %q — ingesting the new data.", existingTable) } diff --git a/internal/cli/data_delete.go b/internal/cli/data_delete.go index 7b1e6d3..9961491 100644 --- a/internal/cli/data_delete.go +++ b/internal/cli/data_delete.go @@ -212,7 +212,7 @@ undone — re-ingesting the data is the only way back.`) p.Newline() p.Successf("Dry-run — nothing was deleted.") if a.OutputJSON { - writeDataDeleteJSON(a.JSONOut, "dry-run", resolved.Namespace, release.ReleaseName, plan, nil) + writeDataDeleteJSON(a.JSONOut, "dry-run", resolved.Namespace, release.ReleaseName, plan, nil, false) jsonEmitted = true } return nil @@ -234,7 +234,7 @@ undone — re-ingesting the data is the only way back.`) // exit 0. One closure so the pair can't drift apart. declined := func() error { if a.OutputJSON { - writeDataDeleteJSON(a.JSONOut, "declined", resolved.Namespace, release.ReleaseName, plan, nil) + writeDataDeleteJSON(a.JSONOut, "declined", resolved.Namespace, release.ReleaseName, plan, nil, false) jsonEmitted = true } return cleanCancel(p, "nothing was deleted.") @@ -283,9 +283,15 @@ undone — re-ingesting the data is the only way back.`) p.Newline() p.Successf("Deleted %s.%s and %d PVC path(s).", plan.Database, plan.Table, len(res.RemovedPaths)) + if !res.BookkeepingCleaned { + // Best-effort cleanup failed — say so, or a schema-drift regression + // (a renamed keying column) is indistinguishable from a legacy + // cluster without the bookkeeping tables (review, Saqlain). + p.Warnf("Bookkeeping cleanup incomplete — the table is gone, but its run-journal/salt rows may remain: %s", strings.Join(res.BookkeepingErrs, "; ")) + } p.Infof("The dataset's catalog metadata is kept as a record on tracebloc, marked unavailable — never removed.") if a.OutputJSON { - writeDataDeleteJSON(a.JSONOut, "deleted", resolved.Namespace, release.ReleaseName, plan, res.RemovedPaths) + writeDataDeleteJSON(a.JSONOut, "deleted", resolved.Namespace, release.ReleaseName, plan, res.RemovedPaths, res.BookkeepingCleaned) jsonEmitted = true } return nil @@ -302,12 +308,17 @@ type dataDeleteJSON struct { Table string `json:"table"` // the REAL (case-resolved) spelling, not the raw argument PVCPaths []string `json:"pvc_paths"` RemovedPaths []string `json:"removed_paths"` + // BookkeepingCleaned mirrors push.TeardownResult: whether the + // run-journal/salt rows were removed with the table. Always false for + // dry-run/declined — nothing was attempted, and a strict consumer must + // never read "cleanup happened" out of a run that deleted nothing. + BookkeepingCleaned bool `json:"bookkeeping_cleaned"` } // writeDataDeleteJSON serializes the delete result to w (stdout in // --output-json mode). Marshal errors are dropped: marshaling our own // struct can't fail in practice, and the exit code remains the contract. -func writeDataDeleteJSON(w io.Writer, status, namespace, release string, plan push.TeardownPlan, removed []string) { +func writeDataDeleteJSON(w io.Writer, status, namespace, release string, plan push.TeardownPlan, removed []string, bookkeepingCleaned bool) { pvcPaths := plan.PVCPaths if pvcPaths == nil { pvcPaths = []string{} // emit [] not null @@ -316,13 +327,14 @@ func writeDataDeleteJSON(w io.Writer, status, namespace, release string, plan pu removed = []string{} // emit [] not null } res := dataDeleteJSON{ - Status: status, - Namespace: namespace, - Release: release, - Database: plan.Database, - Table: plan.Table, - PVCPaths: pvcPaths, - RemovedPaths: removed, + Status: status, + Namespace: namespace, + Release: release, + Database: plan.Database, + Table: plan.Table, + PVCPaths: pvcPaths, + RemovedPaths: removed, + BookkeepingCleaned: bookkeepingCleaned, } b, err := json.MarshalIndent(res, "", " ") if err != nil { diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index f2341e5..46dd8d4 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -60,6 +60,7 @@ screen. %s/%d are runtime placeholders. "%s — %s" "%s, … and %d more" "%s/%s" +"%s: %v" "%s: %w" "%s=%s,%s=%s" "%v (policy: %v)" @@ -96,6 +97,8 @@ screen. %s/%d are runtime placeholders. "Already signed out." "Applies to your next training run; a run already going keeps its size." "Ask one of these admins (or ask them to grant you access)" +"Bookkeeping cleanup incomplete — the old table is gone, but its run-journal/salt rows may remain: %s" +"Bookkeeping cleanup incomplete — the table is gone, but its run-journal/salt rows may remain: %s" "CPU cores for one run (1–%d)" "CSV %s has no columns" "Can't reach tracebloc from here." @@ -140,6 +143,7 @@ screen. %s/%d are runtime placeholders. "Ctrl-C to cancel" "Ctrl-C to stop watching — the run keeps going on the cluster" "DB failures" +"DELETE FROM `%s`.`%s` WHERE table_name='%s'" "DROP TABLE IF EXISTS `%s`.`%s`" "Datasets in %s (0)" "Datasets in %s — %d" @@ -553,6 +557,7 @@ screen. %s/%d are runtime placeholders. "resource env" "restarted ≥%d times — check logs: %v" "root" +"running mysql query: %w%s" "scanning the cluster for tracebloc clients: %w" "schema" "schema entry %q must be col:TYPE (e.g. age:INT,price:FLOAT)" diff --git a/internal/push/list_detailed.go b/internal/push/list_detailed.go index 2e0cd4f..1ce5811 100644 --- a/internal/push/list_detailed.go +++ b/internal/push/list_detailed.go @@ -288,7 +288,7 @@ func runMySQLQuery(ctx context.Context, exec Executor, namespace, pod, container script := `mysql -uroot -p"$MYSQL_ROOT_PASSWORD" -N` if err := exec.Exec(ctx, namespace, pod, container, []string{"sh", "-c", script}, strings.NewReader(query), &stdout, &stderr); err != nil { - return "", fmt.Errorf("querying datasets: %w%s", err, stderrSuffix(&stderr)) + return "", fmt.Errorf("running mysql query: %w%s", err, stderrSuffix(&stderr)) } return stdout.String(), nil } diff --git a/internal/push/teardown.go b/internal/push/teardown.go index b917d6f..e81487f 100644 --- a/internal/push/teardown.go +++ b/internal/push/teardown.go @@ -66,6 +66,18 @@ func PlanTeardown(table string) TeardownPlan { type TeardownResult struct { DroppedTable bool RemovedPaths []string + // BookkeepingCleaned reports whether the ingestor's bookkeeping rows + // for the table (run-journal + pseudonymization salt) were deleted + // alongside it. Best-effort: false on clusters whose ingestor never + // created those tables — the teardown itself still succeeds. + BookkeepingCleaned bool + // BookkeepingErrs carries the per-table failure detail (which + // bookkeeping table, mysql's stderr folded into the error) so callers + // can SURFACE it: a silent false is indistinguishable from the + // schema-drift regression this cleanup exists to prevent — e.g. a + // renamed keying column would otherwise no-op invisibly, reopening + // the husk-row leak (review, Saqlain). + BookkeepingErrs []string } // Teardown performs the in-cluster teardown described by plan: @@ -107,6 +119,39 @@ func Teardown(ctx context.Context, cs kubernetes.Interface, exec Executor, names } res.DroppedTable = true + // 1b. Best-effort bookkeeping cleanup (RFC-0003 I6 — tracebloc/backend#1209): + // the ingestor keeps one run-journal row per ingest and one + // pseudonymization-salt row per table; dropping the table alone + // strands them. Under per-ingestion tables (data-ingestors#408) + // every dataset is its own table, so every delete would leave one + // husk row of each kind — an unbounded slow leak. plan.Table passed + // ValidateTableName ([A-Za-z_][A-Za-z0-9_]*), so it cannot escape + // the single-quoted literal. Each DELETE runs separately and + // best-effort: either bookkeeping table may be absent on clusters + // that never ran a journal-aware ingestor, and these are metadata + // rows, not data — never fail a teardown whose DROP succeeded. + // The SQL rides runMySQLQuery — stdin, never a shell -e argument: the + // string literal's single quotes would terminate a single-quoted shell + // string and mysql would see an unquoted identifier, silently no-oping + // the DELETEs forever (Bugbot on the PR). Column contract, pinned + // against data-ingestors tracebloc_ingestor/database.py: BOTH + // bookkeeping tables key these rows by `table_name` — + // RUNS_TABLE tracebloc_ingest_runs (ingestor_id PK, table_name + // indexed via ix_tracebloc_ingest_runs_table) + // SALT_TABLE tracebloc_ingest_meta (table_name PK, salt) + // Each DELETE stays a separate best-effort call: batched on one stdin, + // a missing first table would abort the second (mysql stops on error). + res.BookkeepingCleaned = true + for _, bookkeeping := range []string{ingestRunsTable, ingestMetaTable} { + cleanupSQL := fmt.Sprintf("DELETE FROM `%s`.`%s` WHERE table_name='%s'", + plan.Database, bookkeeping, plan.Table) + if _, err := runMySQLQuery(ctx, exec, namespace, mysqlPod, mysqlContainer, cleanupSQL); err != nil { + res.BookkeepingCleaned = false + res.BookkeepingErrs = append(res.BookkeepingErrs, + fmt.Sprintf("%s: %v", bookkeeping, err)) + } + } + // 2. rm the PVC dirs from an ephemeral stage-identity pod (see the // doc note above + #259). The pod owns the staging files it // deletes, so this works on hostPath and CSI. diff --git a/internal/push/teardown_test.go b/internal/push/teardown_test.go index df514ed..1b2bc50 100644 --- a/internal/push/teardown_test.go +++ b/internal/push/teardown_test.go @@ -3,6 +3,8 @@ package push import ( "context" "errors" + "fmt" + "io" "strings" "testing" @@ -183,3 +185,103 @@ func TestCleanStaging_PodCreateFailureReturnsError(t *testing.T) { t.Errorf("rm ran (%v) despite the pod never being created", fe.gotCmd) } } + +// TestTeardown_CleansBookkeepingRows pins the RFC-0003 I6 half of teardown +// (tracebloc/backend#1209): after the DROP, the ingestor's run-journal and +// salt rows for the table are deleted best-effort — and a failure there +// never fails a teardown whose DROP already succeeded. +func TestTeardown_CleansBookkeepingRows(t *testing.T) { + newCS := func() *fake.Clientset { + cs := fake.NewClientset(&corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "mysql-0", Namespace: "tracebloc"}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "mysql"}}}, + Status: corev1.PodStatus{Phase: corev1.PodRunning}, + }) + readyOnNextGet(cs) + return cs + } + opts := PodSpecOptions{ + Namespace: "tracebloc", + PVCClaimName: "client-pvc", + PVCMountPath: "/data/shared", + Table: "ds_0f2ab1de3c444e558f66778899aabbcc", + } + plan := PlanTeardown("ds_0f2ab1de3c444e558f66778899aabbcc") + + t.Run("journal and salt rows are deleted for the dropped table", func(t *testing.T) { + rec := &recordingExecutor{} + res, err := Teardown(context.Background(), newCS(), rec, "tracebloc", plan, opts) + if err != nil { + t.Fatalf("Teardown: %v", err) + } + if !res.BookkeepingCleaned { + t.Error("BookkeepingCleaned = false, want true") + } + // The SQL must arrive on STDIN with its quoted literal intact — + // never through a shell -e argument, where the literal's single + // quotes would be eaten by the shell (Bugbot: the DELETEs would + // silently no-op forever). + var journal, salt bool + for _, call := range rec.calls { + if strings.Contains(strings.Join(call.cmd, " "), "DELETE FROM") { + t.Errorf("DELETE passed as a shell argument (%q) — must be fed on stdin", call.cmd) + } + stdin := string(call.stdin) + want := "WHERE table_name='" + plan.Table + "'" + if strings.Contains(stdin, "DELETE FROM") && strings.Contains(stdin, ingestRunsTable) && strings.Contains(stdin, want) { + journal = true + } + if strings.Contains(stdin, "DELETE FROM") && strings.Contains(stdin, ingestMetaTable) && strings.Contains(stdin, want) { + salt = true + } + } + if !journal { + t.Errorf("no stdin DELETE against %s with a quoted literal for %s observed", ingestRunsTable, plan.Table) + } + if !salt { + t.Errorf("no stdin DELETE against %s with a quoted literal for %s observed", ingestMetaTable, plan.Table) + } + }) + + t.Run("bookkeeping failure never fails the teardown", func(t *testing.T) { + rec := &recordingExecutor{failWhenStdinContains: "DELETE FROM"} + res, err := Teardown(context.Background(), newCS(), rec, "tracebloc", plan, opts) + if err != nil { + t.Fatalf("Teardown should tolerate bookkeeping failures, got: %v", err) + } + if !res.DroppedTable { + t.Error("DroppedTable = false, want true") + } + if res.BookkeepingCleaned { + t.Error("BookkeepingCleaned = true, want false when the DELETEs fail") + } + if len(res.RemovedPaths) == 0 { + t.Error("PVC rm did not run — bookkeeping failure must not short-circuit step 2") + } + }) +} + +// recordingExecutor records every Exec call (command AND stdin) and can +// fail calls whose stdin matches a marker. +type recordingExecutor struct { + calls []execCall + failWhenStdinContains string +} + +type execCall struct { + pod, container string + cmd []string + stdin []byte +} + +func (r *recordingExecutor) Exec(ctx context.Context, namespace, pod, container string, cmd []string, stdin io.Reader, stdout, stderr io.Writer) error { + var in []byte + if stdin != nil { + in, _ = io.ReadAll(stdin) + } + r.calls = append(r.calls, execCall{pod: pod, container: container, cmd: cmd, stdin: in}) + if r.failWhenStdinContains != "" && strings.Contains(string(in), r.failWhenStdinContains) { + return fmt.Errorf("simulated bookkeeping failure") + } + return nil +} From cf5b010dab84f0fc988aa2c23033eb18bbe075ce Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Wed, 29 Jul 2026 14:04:32 +0200 Subject: [PATCH 5/7] chore(gosec): reviewed per-site #nosec waivers for the 8 reported findings (18 real sites) (#427) Each gosec finding got an individually reviewed inline waiver with a site-specific justification, so the advisory golangci job can go to zero and backend#1305 can later drop --issues-exit-code=0. The advertised backlog of 8 was an artifact of golangci-lint's default issues.max-same-issues=3: the 13 G304s share one message text, so only 3 surfaced per run (which 3 flapped with cache state). The real, uncapped backlog is 18: 4x G204, 13x G304, 1x G115. All 18 are waived here; waiving only the visible 8 would have been whack-a-mole. Hardening was considered per site and deliberately not bolted on: the meaningful control (symlink rejection on the dataset walk, re-checked at stream time) already exists, and filepath.Clean wrappers would silence gosec without confining anything. Comment-only change; no behavior touched. Verified with the CI-pinned golangci-lint v2.12.2 (go1.26.3): 0 issues with --max-same-issues=0 --max-issues-per-linter=0. Part of tracebloc/backend#1305 (epic #930, Layer 1). Co-authored-by: Claude Fable 5 --- internal/cli/home_local_fallback.go | 2 +- internal/cli/ingest.go | 2 +- internal/cli/installlog.go | 2 +- internal/cli/prepare_host.go | 2 +- internal/cli/update_check.go | 2 +- internal/cli/upgrade.go | 2 +- internal/config/config.go | 2 +- internal/helm/upgrade.go | 2 +- internal/nodeboot/nodeboot.go | 2 +- internal/push/detect.go | 2 +- internal/push/image_resolution.go | 2 +- internal/push/preflight.go | 6 +++--- internal/push/stream.go | 2 +- internal/push/tabular.go | 2 +- internal/push/text.go | 2 +- internal/slug/slug.go | 2 +- 16 files changed, 18 insertions(+), 18 deletions(-) diff --git a/internal/cli/home_local_fallback.go b/internal/cli/home_local_fallback.go index 405660f..0afa1f6 100644 --- a/internal/cli/home_local_fallback.go +++ b/internal/cli/home_local_fallback.go @@ -110,7 +110,7 @@ func tbAliasAvailable() bool { // different tracebloc at another path (Bugbot). Case-insensitive: .cmd is a // Windows artifact and NTFS paths are case-insensitive. func tbCmdAliasOurs(dir, exe string) bool { - b, err := os.ReadFile(filepath.Join(dir, binTB+".cmd")) + b, err := os.ReadFile(filepath.Join(dir, binTB+".cmd")) // #nosec G304 -- fixed name next to os.Executable(): inspects the install dir's own tb.cmd shim; whoever controls that dir already controls the binary. if err != nil { return false } diff --git a/internal/cli/ingest.go b/internal/cli/ingest.go index 748b827..5935407 100644 --- a/internal/cli/ingest.go +++ b/internal/cli/ingest.go @@ -75,7 +75,7 @@ Exit codes: func runIngestValidate(cmd *cobra.Command, args []string) error { path := args[0] - body, err := os.ReadFile(path) + body, err := os.ReadFile(path) // #nosec G304 -- reading the ingest.yaml the operator named as the positional arg is this command's documented job; local CLI, invoking user's privileges. if err != nil { // fileError is exit-code 3 territory. We use a sentinel // exit-coded error so cobra propagates the right code via diff --git a/internal/cli/installlog.go b/internal/cli/installlog.go index 8c45a23..c85ab75 100644 --- a/internal/cli/installlog.go +++ b/internal/cli/installlog.go @@ -34,7 +34,7 @@ func newInstallLog() (*installLog, string) { return nil, "" } path := filepath.Join(dir, "install-"+time.Now().UTC().Format("20060102-150405")+".log") - f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) + f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) // #nosec G304 -- creates (never reads) the install log at a timestamp-generated name under the CLI's own 0700 config dir, mode 0600. if err != nil { // No file was created — return an empty path so the caller never // advertises a "Full log:" location that doesn't exist (Bugbot). diff --git a/internal/cli/prepare_host.go b/internal/cli/prepare_host.go index d946aeb..cdfd805 100644 --- a/internal/cli/prepare_host.go +++ b/internal/cli/prepare_host.go @@ -110,7 +110,7 @@ func prepareHostEnv(user string) []string { // that traps signals. We rely on the default SIGKILL rather than a custom // SIGINT-only Cancel (which a privileged child could ignore, hanging Wait). func prepareHostCmd(ctx context.Context) *exec.Cmd { - c := exec.CommandContext(ctx, "bash", "-c", prepareHostInstallerCmd) + c := exec.CommandContext(ctx, "bash", "-c", prepareHostInstallerCmd) // #nosec G204 -- argv is compile-time constant: literal "bash" -c installerRunScript("prepare-host"), built only from the installerURL const; no runtime input. c.WaitDelay = 5 * time.Second return c } diff --git a/internal/cli/update_check.go b/internal/cli/update_check.go index 8f317c4..f8214bd 100644 --- a/internal/cli/update_check.go +++ b/internal/cli/update_check.go @@ -164,7 +164,7 @@ func readUpdateCache(path string) (updateCache, bool) { if path == "" { return updateCache{}, false } - raw, err := os.ReadFile(path) + raw, err := os.ReadFile(path) // #nosec G304 -- the CLI's own throttle cache: config.Dir() + the constant updateCacheFile name; contents JSON-validated before use. if err != nil { return updateCache{}, false } diff --git a/internal/cli/upgrade.go b/internal/cli/upgrade.go index 9c0d8b6..686c105 100644 --- a/internal/cli/upgrade.go +++ b/internal/cli/upgrade.go @@ -120,7 +120,7 @@ Safe to run anytime; safe to re-run.`, // Stream the installer straight to the user's terminal, and keep // stdin wired so its interactive prompts (sign-in, etc.) still work. ctx := cmd.Context() - c := exec.CommandContext(ctx, plan.name, plan.args...) + c := exec.CommandContext(ctx, plan.name, plan.args...) // #nosec G204 -- upgradePlanFor(runtime.GOOS) yields compile-time constants: "bash" -c installerRunScript(""); only the GOOS branch varies, no user input. c.Stdin, c.Stdout, c.Stderr = os.Stdin, os.Stdout, os.Stderr if err := c.Run(); err != nil { // User aborted (Ctrl-C) or the parent context was cancelled: exit diff --git a/internal/config/config.go b/internal/config/config.go index 5490ff3..1e020c7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -122,7 +122,7 @@ func Load() (*Config, error) { if err != nil { return nil, err } - data, err := os.ReadFile(path) + data, err := os.ReadFile(path) // #nosec G304 -- the CLI's own config: Dir()/config.json under ~/.tracebloc or the operator's explicit $TRACEBLOC_CONFIG_DIR override, read as the invoking user. if errors.Is(err, fs.ErrNotExist) { return &Config{Version: schemaVersion, Profiles: map[string]*Profile{}}, nil } diff --git a/internal/helm/upgrade.go b/internal/helm/upgrade.go index 67e713b..aa0f842 100644 --- a/internal/helm/upgrade.go +++ b/internal/helm/upgrade.go @@ -53,7 +53,7 @@ const ( // var so tests substitute a fake without spawning real helm. Mirrors // nodeboot.Runner exactly. var Runner = func(ctx context.Context, name string, args ...string) (string, error) { - out, err := exec.CommandContext(ctx, name, args...).CombinedOutput() + out, err := exec.CommandContext(ctx, name, args...).CombinedOutput() // #nosec G204 -- test seam: every caller passes the literal "helm"; args are the operator's own release/kubeconfig flags, exec'd as an argv array, no shell. return string(out), err } diff --git a/internal/nodeboot/nodeboot.go b/internal/nodeboot/nodeboot.go index 82599d8..db29f96 100644 --- a/internal/nodeboot/nodeboot.go +++ b/internal/nodeboot/nodeboot.go @@ -32,7 +32,7 @@ const imageReference = "ghcr.io/tracebloc/*" // Runner executes an external command and returns its combined output. A package // var so tests can substitute a fake without spawning real k3d/helm/docker. var Runner = func(ctx context.Context, name string, args ...string) (string, error) { - out, err := exec.CommandContext(ctx, name, args...).CombinedOutput() + out, err := exec.CommandContext(ctx, name, args...).CombinedOutput() // #nosec G204 -- test seam: callers pass literal tool names (k3d/helm/docker) with argv from package consts and the operator's own cluster/release names; no shell. return string(out), err } diff --git a/internal/push/detect.go b/internal/push/detect.go index cdcf544..b82031c 100644 --- a/internal/push/detect.go +++ b/internal/push/detect.go @@ -27,7 +27,7 @@ import ( // default, advising --target-size. (Since Discover only yields the // ingestor's accept-set — .jpg/.jpeg/.png — that path is defensive.) func DetectImageSize(path string) (width, height int, err error) { - f, err := os.Open(path) + f, err := os.Open(path) // #nosec G304 -- decodes the header of an image the symlink-rejecting dataset walk found under the operator-chosen root; operator's own file. if err != nil { return 0, 0, err } diff --git a/internal/push/image_resolution.go b/internal/push/image_resolution.go index e33d8af..64dcdf5 100644 --- a/internal/push/image_resolution.go +++ b/internal/push/image_resolution.go @@ -27,7 +27,7 @@ import ( func scanImageResolutions(paths []string, expectedW, expectedH, minW, minH int) (broken, tooSmall, mismatched []string) { for _, path := range paths { name := filepath.Base(path) - f, err := os.Open(path) + f, err := os.Open(path) // #nosec G304 -- paths come from the symlink-rejecting dataset walk of the operator-chosen root; mirrors the in-cluster validator on the operator's own files. if err != nil { broken = append(broken, fmt.Sprintf("%s (unreadable: %v)", name, err)) continue diff --git a/internal/push/preflight.go b/internal/push/preflight.go index 3315b27..8dc1130 100644 --- a/internal/push/preflight.go +++ b/internal/push/preflight.go @@ -42,7 +42,7 @@ var utf8BOM = []byte{0xEF, 0xBB, 0xBF} // non-EOF Read error is only ever a genuine I/O failure, so callers can treat // it as fail-closed. The caller closes the returned Closer. func openCSVReader(path string) (*csv.Reader, io.Closer, error) { - f, err := os.Open(path) + f, err := os.Open(path) // #nosec G304 -- opens the operator's own dataset CSV to mirror the ingestor's checks locally before upload; no privilege boundary crossed. if err != nil { return nil, nil, err } @@ -78,7 +78,7 @@ func matchColumnIndex(header []string, want string) int { // HasBOM reports whether the file starts with a UTF-8 BOM. func HasBOM(path string) (bool, error) { - f, err := os.Open(path) + f, err := os.Open(path) // #nosec G304 -- 3-byte BOM sniff of the same operator-supplied dataset CSV openCSVReader reads; local preflight as the invoking user. if err != nil { return false, err } @@ -802,7 +802,7 @@ func hasKnownExtension(name string) bool { // the FIRST gate validate_data runs in-cluster: the CSV must be valid UTF-8 // and free of NUL bytes, or the whole run aborts (after the upload). func CheckCSVEncoding(path string) error { - f, err := os.Open(path) + f, err := os.Open(path) // #nosec G304 -- encoding-check of the operator's own dataset CSV, size-capped by LimitReader; same local-preflight threat model as openCSVReader. if err != nil { return fmt.Errorf("reading %s: %w", filepath.Base(path), err) } diff --git a/internal/push/stream.go b/internal/push/stream.go index f26233c..7433022 100644 --- a/internal/push/stream.go +++ b/internal/push/stream.go @@ -469,7 +469,7 @@ func writeTarFile(tw *tar.Writer, src, dst string) (int64, error) { if err := tw.WriteHeader(hdr); err != nil { return 0, err } - f, err := os.Open(src) + f, err := os.Open(src) // #nosec G304 -- src came from the symlink-rejecting walk and the Lstat guard above re-rejects symlinks at stream time; reads the operator's own dataset file. if err != nil { return 0, err } diff --git a/internal/push/tabular.go b/internal/push/tabular.go index 05d52e9..319465f 100644 --- a/internal/push/tabular.go +++ b/internal/push/tabular.go @@ -388,7 +388,7 @@ type SchemaInference struct { // redeclares them. The risky cases (empty-in-sample, id-like) are returned // alongside the schema so the caller can surface them as warnings. func InferSchema(csvPath string) (*SchemaInference, error) { - f, err := os.Open(csvPath) + f, err := os.Open(csvPath) // #nosec G304 -- csvPath is the dataset CSV DiscoverTabular's symlink-rejecting walk found under the operator-chosen root; local read as the invoking user. if err != nil { return nil, err } diff --git a/internal/push/text.go b/internal/push/text.go index 6cb73e1..6cfb089 100644 --- a/internal/push/text.go +++ b/internal/push/text.go @@ -162,7 +162,7 @@ func validateTextRecords(csvPath, dirName string, files []string, rf RecordForma if path == "" { continue // manifest names a file not on disk — a missing-file check's job, not ours } - content, err := os.ReadFile(path) + content, err := os.ReadFile(path) // #nosec G304 -- path comes from byBase/byStem, keyed only by files the symlink-vetted walk found in the dataset dir; a manifest entry selects among them, it cannot point elsewhere. if err != nil { return fmt.Errorf("reading %s: %w", filepath.Join(dirName, filepath.Base(path)), err) } diff --git a/internal/slug/slug.go b/internal/slug/slug.go index fb3f8a0..17091a8 100644 --- a/internal/slug/slug.go +++ b/internal/slug/slug.go @@ -87,7 +87,7 @@ func toASCII(s string) string { var b strings.Builder for _, r := range norm.NFKD.String(s) { if r < 128 { - b.WriteByte(byte(r)) + b.WriteByte(byte(r)) // #nosec G115 -- false positive: range-over-string runes are non-negative and the r < 128 guard bounds them, so byte(r) is a lossless ASCII conversion. } } return b.String() From d972cdc8dd0dd3f987a67f2965d3ba0c33d81498 Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Wed, 29 Jul 2026 14:13:19 +0200 Subject: [PATCH 6/7] chore: clear house-rules findings (#426) * chore: clear house-rules findings 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 * fix(install): stall-based download bounding via a shared dl() helper (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 --------- Co-authored-by: Claude Fable 5 --- scripts/install.sh | 33 ++++++++++++++++++++++---------- scripts/sync-backend-fixtures.sh | 6 ++++-- scripts/sync-schema.sh | 5 +++-- scripts/tests/install-verify.sh | 5 ++++- 4 files changed, 34 insertions(+), 15 deletions(-) diff --git a/scripts/install.sh b/scripts/install.sh index 312ee66..5886c61 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -173,11 +173,13 @@ ensure_cosign() { csums="$TMP/cosign_checksums.txt" echo " cosign not found — bootstrapping pinned ${COSIGN_VERSION} to verify the signature..." - # --tlsv1.2 floor for the cosign bootstrap fetch, matching the client - # installer's curls — never negotiate below TLS 1.2 to pull the verifier we - # then trust to authenticate the release. - if ! curl -fsSL --tlsv1.2 "$cbase/$casset" -o "$cbin" 2>/dev/null; then return 1; fi - if ! curl -fsSL --tlsv1.2 "$cbase/cosign_checksums.txt" -o "$csums" 2>/dev/null; then return 1; fi + # dl() carries the TLS 1.2 floor + stall-based bounding (see the helper) — + # never negotiate below TLS 1.2 to pull the verifier we then trust to + # authenticate the release, and never let a dead endpoint wedge the + # install. No wall-clock cap: the ~90MB cosign binary must be allowed to + # finish on slow links (review #426). + if ! dl "$cbase/$casset" "$cbin" 2>/dev/null; then return 1; fi + if ! dl "$cbase/cosign_checksums.txt" "$csums" 2>/dev/null; then return 1; fi cwant="$(grep " ${casset}\$" "$csums" | awk '{print $1}' | head -1)" [ -n "$cwant" ] || return 1 @@ -202,7 +204,7 @@ resolve_tag() { # Use the redirect-trail of /releases/latest to learn the tag — # avoids hitting the rate-limited /api/repos endpoint for the # zero-auth one-liner case. - redirect_url="$(curl -fsSI --tlsv1.2 \ + redirect_url="$(curl -fsSI --tlsv1.2 --connect-timeout 30 --max-time 30 \ "https://github.com/${GITHUB_REPO}/releases/latest" \ | awk '/^[Ll]ocation:/ { print $2 }' \ | tr -d '\r')" @@ -261,17 +263,28 @@ echo "Installing tracebloc CLI $TAG ($OS/$ARCH)..." BINARY_FILE="${BINARY_NAME}-${TAG}-${OS}-${ARCH}" BASE_URL="https://github.com/${GITHUB_REPO}/releases/download/${TAG}" +# Shared download profile for every body fetch in this script (review: #426). +# Stall-based bounding instead of a wall-clock cap: --max-time 300 made the +# ~50MB binary fail under ~1.4 Mbps and the ~90MB cosign bootstrap under +# ~2.6 Mbps — links that are slow but alive must be allowed to finish, while +# a dead connection (under 1 KiB/s for 60s straight) still aborts instead of +# wedging the install. TLS floor stays 1.2. Retune here, once. +# usage: dl +dl() { + curl -fsSL --tlsv1.2 --connect-timeout 30 --speed-limit 1024 --speed-time 60 "$1" -o "$2" +} + TMP="$(mktemp -d)" trap 'rm -rf "$TMP"' EXIT INT TERM echo "Downloading binary..." -if ! curl -fsSL --tlsv1.2 "$BASE_URL/$BINARY_FILE" -o "$TMP/$BINARY_FILE"; then +if ! dl "$BASE_URL/$BINARY_FILE" "$TMP/$BINARY_FILE"; then echo "Error: failed to download $BASE_URL/$BINARY_FILE" >&2 exit 1 fi echo "Downloading SHA256SUMS..." -if ! curl -fsSL --tlsv1.2 "$BASE_URL/SHA256SUMS" -o "$TMP/SHA256SUMS"; then +if ! dl "$BASE_URL/SHA256SUMS" "$TMP/SHA256SUMS"; then echo "Error: failed to download SHA256SUMS — release may be malformed" >&2 exit 1 fi @@ -345,8 +358,8 @@ verify_cosign_signature() { fi echo "Verifying cosign signature..." - if ! curl -fsSL --tlsv1.2 "$BASE_URL/$BINARY_FILE.sig" -o "$TMP/$BINARY_FILE.sig" 2>/dev/null \ - || ! curl -fsSL --tlsv1.2 "$BASE_URL/$BINARY_FILE.cert" -o "$TMP/$BINARY_FILE.cert" 2>/dev/null; then + if ! dl "$BASE_URL/$BINARY_FILE.sig" "$TMP/$BINARY_FILE.sig" 2>/dev/null \ + || ! dl "$BASE_URL/$BINARY_FILE.cert" "$TMP/$BINARY_FILE.cert" 2>/dev/null; then if [ "$ALLOW_UNVERIFIED" = "1" ]; then echo " WARNING: .sig/.cert not published for $TAG — signature NOT verified" >&2 echo " (TRACEBLOC_ALLOW_UNVERIFIED=1)." >&2 diff --git a/scripts/sync-backend-fixtures.sh b/scripts/sync-backend-fixtures.sh index a58b476..4bf4f80 100755 --- a/scripts/sync-backend-fixtures.sh +++ b/scripts/sync-backend-fixtures.sh @@ -133,8 +133,10 @@ sync_one() { # sync_one is called as `if ! sync_one ...`, which suspends `set -e` for the # whole body — so check curl's exit explicitly and report the real fetch # failure instead of misdiagnosing an empty temp file as "not valid JSON". - # --tlsv1.2 matches every other curl in the repo (scripts/install.sh). - curl -fsSL --tlsv1.2 \ + # --tlsv1.2 matches every other curl in the repo (scripts/install.sh); the + # time bounds keep a hung GitHub API from wedging CI (18 fixtures per run, + # so a generous per-file ceiling would compound). + curl -fsSL --tlsv1.2 --connect-timeout 10 --max-time 60 \ -H "Authorization: Bearer ${TOKEN}" \ -H "Accept: application/vnd.github.raw+json" \ -H "X-GitHub-Api-Version: 2022-11-28" \ diff --git a/scripts/sync-schema.sh b/scripts/sync-schema.sh index 1fa12e5..e6ee10a 100755 --- a/scripts/sync-schema.sh +++ b/scripts/sync-schema.sh @@ -112,8 +112,9 @@ sync_one() { # whole body — so a failed curl (e.g. a 404) would otherwise fall through and # be misdiagnosed as "not valid JSON" on the empty temp file. Check curl's # exit explicitly and report the real fetch failure. --tlsv1.2 matches every - # other curl in the repo (scripts/install.sh). - curl -fsSL --tlsv1.2 "$url" -o "$tmp" + # other curl in the repo (scripts/install.sh); the time bounds keep a hung + # endpoint from wedging CI. + curl -fsSL --tlsv1.2 --connect-timeout 10 --max-time 60 "$url" -o "$tmp" local curl_rc=$? if [[ $curl_rc -ne 0 ]]; then echo "error: failed to fetch $url (curl exited $curl_rc)" >&2 diff --git a/scripts/tests/install-verify.sh b/scripts/tests/install-verify.sh index ed388ad..f499394 100755 --- a/scripts/tests/install-verify.sh +++ b/scripts/tests/install-verify.sh @@ -13,7 +13,10 @@ # served from a temp dir. No network, no real download. This harness is bash # (for arrays/locals); the script under test stays POSIX sh. # ============================================================================= -set -u +# pipefail so a failing pipeline producer (sha256sum | awk in _sha) can't be +# masked by its last stage exiting 0. Deliberately NO -e: this harness counts +# pass/fail itself and must keep running after a failed assertion. +set -uo pipefail SELF_DIR="$(cd "$(dirname "$0")" && pwd)" INSTALLER="$SELF_DIR/../install.sh" From f37dad5338b261cbd317d304b23675722bd798d0 Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Wed, 29 Jul 2026 15:09:38 +0200 Subject: [PATCH 7/7] ci(golangci): drop the advisory flag + uncap max-same-issues (#430) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ci(golangci): drop the advisory flag + uncap max-same-issues (backend#1305) Backlog is zero after the reviewed #nosec waivers (#427): findings now fail the job. max-same-issues: 0 so repeated findings can never hide behind the default cap of 3 again (the '8 findings were really 18' lesson). Branch-protection required-flip follows once this merges. Co-Authored-By: Claude Opus 4.8 * ci(golangci): drop the advisory flag + uncap max-same-issues (backend#1305) Backlog is zero after the reviewed #nosec waivers (#427): findings now fail the job. max-same-issues: 0 so repeated findings can never hide behind the default cap of 3 again (the '8 findings were really 18' lesson). Branch-protection required-flip follows once this merges. Co-Authored-By: Claude Opus 4.8 * build: make ci runs lint-full — mirror the now-failing golangci gate (Bugbot) golangci-lint fails PRs on findings since this branch; make ci skipping it broke the 'make ci mirrors CI exactly' rule (green local, red PR). lint-full's guard already gives install instructions when the tool is missing, which is correct mirroring rather than a soft skip. Co-Authored-By: Claude Opus 4.8 * build: pin lint-full to the CI golangci version via go run (Bugbot) lint-full ran whatever golangci-lint was on PATH while CI pins v2.12.2 -- with ci depending on lint-full, version drift could green a local run that reds the PR gate. Now runs the exact pinned version through the Makefile's own 'go run tool@version' pattern (like errcheck/ staticcheck/govulncheck): no PATH dependency, no brew-version drift. GOLANGCI_LINT var removed (unused); lockstep note added on both sides. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- .github/workflows/golangci.yml | 45 +++++++++++----------------------- .golangci.yml | 6 +++++ Makefile | 23 +++++++++-------- 3 files changed, 33 insertions(+), 41 deletions(-) diff --git a/.github/workflows/golangci.yml b/.github/workflows/golangci.yml index b44810a..257420c 100644 --- a/.github/workflows/golangci.yml +++ b/.github/workflows/golangci.yml @@ -30,34 +30,18 @@ concurrency: jobs: golangci: timeout-minutes: 10 - name: golangci-lint (advisory) + name: golangci-lint runs-on: ubuntu-latest - # ============================ ADVISORY ============================ - # DELIBERATELY not a required check, and `--issues-exit-code=0` on - # the run step keeps findings from red-Xing the job: golangci-lint - # exits non-zero on any finding by default, and the pre-existing - # gosec backlog (8 findings at introduction: 4x G204, 3x G304, - # 1x G115 — sized on #423, the PR that added this job) would - # otherwise fail every PR for issues it didn't introduce. Findings - # still surface as inline annotations and in the job log. - # - # Known blind spot while advisory: typecheck (compile) errors ride - # the same issues exit path as lint findings, so under this flag - # they exit 0 too (verified empirically on #423 — this is NOT a - # separate exit code). No real signal is lost: a non-compiling PR - # reds the required Test / Lint / Build jobs anyway, and a broken - # .golangci.yml still fails THIS job via the action's - # `golangci-lint config verify` pre-step, which --issues-exit-code - # does not touch. - # - # (Job-level `continue-on-error: true` is NOT the tool for this — - # it greens the workflow run but still shows the job itself as - # failed in the PR checks list; see the first run on #423.) - # - # REMOVE the `--issues-exit-code=0` arg when the backlog hits zero - # and this check flips to required (the advisory -> required - # pattern from backend#1303; tracked under backend#1305 / epic - # #930). A required check that can't fail is worse than no check. + # ============================= GATE ============================== + # Blocking by exit code since the backlog hit zero: the gosec + # findings sized on #423 were all resolved with per-site reviewed + # #nosec waivers (#427), so any finding this job reports from now + # on is NEW and fails the job — including typecheck errors, which + # ride the same exit path. History of the advisory era (the + # --issues-exit-code=0 flag, why job-level continue-on-error was + # not the tool) is in the #423/#426 discussions if you need it. + # Final step of the flip = marking this check required in branch + # protection (backend#1305 / epic #930). # ================================================================== steps: - uses: actions/checkout@v7 @@ -73,11 +57,10 @@ jobs: # built with Go 1.26 (required: go.mod says `go 1.26.0`; v1-era # binaries can't typecheck this module). Bump deliberately, and # keep the version in step with the format expectations noted in - # .golangci.yml. + # .golangci.yml AND with GOLANGCI_LINT_VERSION in the Makefile + # (make lint-full runs the same pinned version -- the local/CI + # mirror depends on the two never drifting). - name: golangci-lint run (.golangci.yml) uses: golangci/golangci-lint-action@v9.3.0 with: version: v2.12.2 - # ADVISORY MODE — see the block comment above (incl. the - # typecheck caveat). Remove at the required-flip. - args: --issues-exit-code=0 diff --git a/.golangci.yml b/.golangci.yml index d5976f5..4b12669 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -96,3 +96,9 @@ formatters: - third_party$ - builtin$ - examples$ + +issues: + # Never cap repeated findings: the default (3) hid 10 of the 13 G304s + # behind a cache-flappy sample — "8 findings" were really 18 (#427). + # A gate must see the whole backlog, every run. + max-same-issues: 0 diff --git a/Makefile b/Makefile index 54e93dc..d415185 100644 --- a/Makefile +++ b/Makefile @@ -9,11 +9,12 @@ # ---- toggles ----------------------------------------------------- GO ?= go -GOLANGCI_LINT ?= golangci-lint PKGS := ./... # Pinned lint/analysis tool versions (reproducibility — no more @latest drift). -# Keep these in lockstep with .github/workflows/build.yml. Bump deliberately. +# Keep these in lockstep with .github/workflows/build.yml — and +# GOLANGCI_LINT_VERSION with .github/workflows/golangci.yml. Bump deliberately. +GOLANGCI_LINT_VERSION ?= v2.12.2 ERRCHECK_VERSION ?= v1.20.0 INEFFASSIGN_VERSION ?= v0.2.0 MISSPELL_VERSION ?= v0.3.4 @@ -24,8 +25,11 @@ GOIMPORTS_VERSION ?= v0.48.0 # ---- top-level targets ------------------------------------------- +# ci mirrors the PR gates exactly — including golangci-lint (lint-full), +# which fails on findings since #430. A green `make ci` must imply a green +# PR; lint-full's own guard tells you how to install the tool if missing. .PHONY: ci -ci: vet test lint fmt-check schema-check vulncheck file-budget deadcode check-style +ci: vet test lint lint-full fmt-check schema-check vulncheck file-budget deadcode check-style @echo "==> ci: all green" .PHONY: build @@ -126,15 +130,14 @@ deadcode: vulncheck: $(GO) run golang.org/x/vuln/cmd/govulncheck@$(GOVULNCHECK_VERSION) ./... +# Pinned to the exact version the golangci CI job runs (see +# .github/workflows/golangci.yml), via the same `go run tool@version` +# pattern as the tools above — no PATH dependency, so a green +# `make ci` and the PR gate can never disagree on golangci version. +# First run builds from source (~1-2 min); cached afterwards. .PHONY: lint-full lint-full: - @command -v $(GOLANGCI_LINT) >/dev/null 2>&1 || { \ - echo "==> $(GOLANGCI_LINT) not on PATH"; \ - echo " install via: brew install golangci-lint"; \ - echo " or see: https://golangci-lint.run/usage/install/"; \ - exit 1; \ - } - $(GOLANGCI_LINT) run + $(GO) run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION) run .PHONY: fmt fmt: