Remediate the full-codebase review: 33 findings across detect, connectors, engine, api, web and deploy - #138
Merged
Merged
Conversation
…pack-order bias Redaction could emit plaintext (#105): the window-edge clip skipped a stray copy of a matched secret that overlapped the target, so the edge bisected it and its remainder was stored as context. The edge now clamps to the target's own boundary, and a second backstop refuses any window whose edge bisects a copy. The substring check the issue proposed is deliberately not implemented — a PEM match's trailing context is legitimately a long substring of the secret itself, so it would drop real findings; the leak class is a property of the boundary, not of the snippet text. The load-time regex guard accepted two exponential shapes (#110): `{,n}` parsed as a fixed quantifier because `int("")` raised, and an alternation repeated at its own nesting level had no inner quantifier to flag. The seed pack loads unchanged under both checks. The report cap filled in pack order, so a noisy early rule starved a later rule's critical finding out of the report entirely (#116). Candidates are now collected in full and ranked by severity, then confidence, then position. Detection paid the whole match set per reported finding, which a match-dense page turned into ~9 KB/s (#117). Spans reaching each snippet's window are selected by bisect, the scrub table is built once per call, and a page with nothing to report skips the masking pass altogether: 8k matches over 156 KB drops from 17.5s to 0.10s. Also: pack-level `version`/`description` are type-checked rather than coerced, so `version:` with no value no longer loads as the string "None" (#118), and scan tasks carry an explicit broker time limit so lease expiry stays the only re-delivery authority (#106, ADR 0009). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Attachment downloads bypassed everything the JSON path had (#108). A transport error escaped unwrapped, so a connection reset mid-download failed the whole space task and discarded every unit already scanned; a 429 or 5xx failed the attachment outright with no retry, silently dropping the attachment corpus of exactly the scan that tripped the rate limit. Both paths now share one retry loop, with redirect hops as a returned value so the chain stays legible. `RateLimited` was swallowed by the per-page handler (#109). Since comments, attachments and body fallbacks all run inside it, a throttled site spent `max_wait_seconds` per page and marched on — a task could hold its lease for hours instead of failing and telling the operator to reduce concurrency, which is what the docs already promised. An honest spreadsheet was rejected as a bomb (#121). A member over the remaining output budget raised rather than truncating, so a 100k-row .xlsx was scanned not at all while an equally large .txt was truncated and scanned. The issue's proposed lying-header branch turned out to be unreachable — `zipfile` stops the read at the declared size and fails the CRC — so it is gone rather than left as a guard that never fires; a lying header still lands as `failed_parse`, which is `is_hostile`. CDATA was unwrapped before tag stripping, deleting the pseudo-tags inside code blocks (#122): `<password>hunter2</password>` lost the keyword that ADR 0003's proximity signal needs. UTF-16 text attachments were skipped as binary on their first NUL (#123) — the Windows Notepad default, in the file class this product exists to read. Link resolution now follows `_links.base` instead of splicing a hardcoded `/wiki` (#124). Also: one pooled HTTP client per connector lifetime rather than a TLS handshake per request (#130), no sleep after the final retry, case-insensitive space matching with a warning when a configured space matched nothing, and an address-space rlimit on the sandbox child so a PDF bomb cannot get the cgroup to pick the worker instead (#133). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ration The actor inherited Dramatiq's 10-minute default time limit, and `TimeLimitExceeded` is a `BaseException` — so it slipped past both handlers in `run_task` and no report was ever submitted (#106). The API then waited out the lease, reclaimed, and redelivered into the same 10-minute death: a large space re-scanned forever. Interrupts are now caught, reported as failed, and re-raised so the thread still dies. Paired with the explicit `time_limit` in `iceberg_core.tasks`, lease expiry is once again the only re-delivery authority (ADR 0009). A connector failure mid-fetch discarded every finding accumulated before it (#115): the report was assigned only after the loop finished. Accumulation now happens in a `finally`, so a failed task still carries the units it did scan — the unit-level tolerance `units_failed` already pays for. 401 and 403 were one exception, so results discarded because an engine's token was rotated mid-task were logged as a routine lease loss at INFO (#131). They are told apart now, and a rejected token is a warning. An engine with no token also booted healthy and consumed messages it could only drop, leaving scans to stall behind a fleet that looked alive; the token is checked at boot instead. Also: one pooled HTTP client for the process rather than a fresh TLS handshake per lease, submit, heartbeat and retry (#130), and the engine now has counters of its own — task outcomes, findings reported, connector failures, API retries, failed beats — instead of a metrics endpoint serving only API-side series that can never move in an engine process (#132). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ation and dispatch Retention selected eligible findings and then deleted by id alone (#111), so a finding reopened by an engine submission between the read and the delete was removed anyway, taking its event trail with it. The eligibility predicates are repeated on the DELETE — which is what the function's own docstring already claimed it did. `launch_scan` called `db.rollback()` on the caller's session when the one-active-scan index refused a launch (#112), discarding bookkeeping the scheduler had staged for schedules that fired earlier in the same tick. Those schedules stayed due and re-fired, launching duplicate scans. The conflicting insert runs in a SAVEPOINT now. Finalisation and reconciliation were three transactions (#119). A crash after the first left a COMPLETED scan that never reconciled, and every repair path was gated on the scan still being active — so remediated findings stayed OPEN and the scan's notifications were never enqueued at all. They are one transaction now. Auto-resolve was also a read-then-write that could overwrite an analyst's triage decision committed mid-round; it is a conditional update, matching the house pattern used for every other transition. Notification dispatch committed once per batch and built payloads outside the error guard (#114), so a crash re-sent everything already delivered that round and one poisoned row wedged the whole batch into re-sending every beat. Also: engines that stop heartbeating are marked offline by the maintenance round (#113) — nothing ever set the status, so dead engines counted as live in the fleet view and `GET /rules` reported their packs forever; findings from a failed task are ingested rather than discarded (#115, the other half of the engine change); indexes for the keyset pagers and the outbox cascade (#120); the queue-depth gauge is finally sampled (#132); and the #137 bundle — bounded rate-limit store, `previous_secret_hash` used as a re-key cross-check rather than ignored, `Scan.error` populated, `PageParams` deleted for a shared page builder, and batched suppression sweeps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An HTMX mutation that failed with a 4xx showed the user nothing (#129). htmx 2 does not swap 4xx responses, so the full error page the handler returned was discarded — an admin clicking "Scan now" on a source someone else had just disabled saw no navigation, no message, nothing. Failed mutations now answer a retargeted fragment; a plain browser navigation still gets the error page with its real status. The login return path was broken two ways (#128). `next` was interpolated unencoded, so a bookmarked filter lost everything after its first `&` — the opposite of what the test asserting it claimed. And a mutating request recorded its own path, so an expired session on `POST /findings/{id}/triage` sent the user back to a POST-only URL and a 405 rendered as "Something went wrong", which is the common expiry path for every button in the console. Authenticated pages carried no `Cache-Control` (#134). The sharpest case is the engine credential page: a one-time token that exists only in that response was left cacheable and restorable from bfcache after sign-out. Also the #135 bundle: triage's enum conversion moved inside the try that exists to catch it (a bad value was a 500), `error_text` returns authored ValueError copy instead of swallowing it, the suppressions and schedules pagers carry their filters instead of silently dropping to an unfiltered list resumed from a filtered cursor, the unreachable channel-edit route and its dead Alpine component are gone, and three copies of `_quote` became one helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…he posture `helm install` deadlocked on a clean cluster (#107). The migration Job is a pre-install hook whose env comes from the ConfigMap and api Secret, and Helm creates hook resources before any regular manifest — so the pod sat in CreateContainerConfigError until the timeout. Both are hooks now, at a lower weight, without `hook-succeeded` so the Deployments keep reading them; on an upgrade this also stops the Job migrating with the previous release's config. Inlining the Job's env was the alternative and would have put the database URL in plaintext in the manifest. CI only ever ran `helm template`, which is why nothing caught it. Rotating a chart-managed secret left every replica on the old value (#125): only the ConfigMap was checksummed, so `helm upgrade` looked applied when it was not — including during the documented pepper-rotation runbook. Migrations had never been run against Postgres (#126). The suite stands SQLite in for it, so JSONB, partial-index syntax and real ALTERs were first exercised by the production pre-upgrade Job. A CI leg now applies, reverses and re-applies every revision against a real Postgres service container. The docs claimed Redis ran with auth *and* TLS and leaned on it in the threat model (#127); nothing in compose or the chart configures TLS. `rediss://` already works through the broker URL, so the docs now say that and describe the compose stack honestly as plaintext. Also the #136 bundle: `stop_grace_period` on the compose engine so SIGTERM handling is worth having, an honest values.yaml comment for the unimplemented vault backend, SHA-pinned CI actions, and a CLAUDE.md/README/backlog that no longer describe shipped M4 work as still to come. Because an engine now refuses to boot without a credential, the dev stack grew a bootstrap step: the engine sits behind a compose profile and `make engine-token` mints one against a migrated API and starts it. The API stores only the hash, so no static token can be shipped in .env.example. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The engine refuses to boot without a credential now (#131), and the entrypoint check started it with only a broker and an API URL — so the guard, working as intended, failed the check. The token is unusable and no engine id is set, so the check stays what it was: proof that the image's entrypoint serves, not a test of the control plane. The api check next to it already runs on placeholder credentials for the same reason. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes every issue raised by the full-codebase review (#105–#137). Six commits, one per area, each reviewable on its own.
Result:
make checkgreen — ruff, format, mypy, and 1193 tests passing, up from 1079 at the branch point. 114 new tests; no existing test deleted, several corrected where they pinned the buggy behaviour.The five that mattered most
TimeLimitExceededis aBaseExceptionthat slipped past both handlers, so the API reclaimed and redelivered into the same death.helm installdeadlocked (Helm: pre-install migration Job references the non-hook ConfigMap/Secret — fresh install deadlocks, upgrades migrate with stale config #107) — the pre-install migration Job referenced a ConfigMap and Secret Helm had not created yet. CI only ranhelm template, so nothing caught it.RateLimitedwas swallowed per-page so a throttled site could hold a lease for hours.Notable judgment calls
Three issues were partly wrong, and the fixes say so rather than implementing them as written:
zipfilestops a member read at the declared size and then fails the CRC, soBombErroronly ever fired for honest members. The dead branch is gone rather than left as a guard that never fires; a lying header still lands asfailed_parse, which isis_hostile.rediss://already works through the broker URL — the defect was purely that the docs claimed TLS was configured when nothing configured it.Behaviour changes worth knowing
.env.example, so the dev engine sits behind a compose profile andmake engine-tokenmints one against a migrated API.make upstarts the control plane;make scaleis unaffected.finalize_scan_if_doneandreconcile_scanno longer commit — their caller owns the transaction, which is what makes Scan reconciliation: crash between finalization and reconciliation is unrepairable; auto-resolve can overwrite analyst triage #119 atomic.helm uninstallleaves the hook-annotated ConfigMap and api Secret behind; the trade and its reasoning are documented indocs/deployment.md.Verification beyond the suite
helm install --dry-run— hook resources land at the right weights and the engine Secret stays release-owned.One thing left for you
.env.backup-stale-20260731sits world-readable in the repo root and is not gitignored, unlike.env. I did not touch it — deleting or ignoring a file holding real secrets is your call — but agit add -Awould commit it.Closes #105
Closes #106
Closes #107
Closes #108
Closes #109
Closes #110
Closes #111
Closes #112
Closes #113
Closes #114
Closes #115
Closes #116
Closes #117
Closes #118
Closes #119
Closes #120
Closes #121
Closes #122
Closes #123
Closes #124
Closes #125
Closes #126
Closes #127
Closes #128
Closes #129
Closes #130
Closes #131
Closes #132
Closes #133
Closes #134
Closes #135
Closes #136
Closes #137
🤖 Generated with Claude Code