fix(utf8): eliminate 23 string byte-slicing panic vectors on ingested text - #422
Merged
Conversation
… text `&s[a..b]` on a `&str` panics when a bound lands mid-UTF-8-sequence. 4DA ingests arbitrary internet text (HN, Reddit, RSS, Mastodon, arXiv, OSV/GHSA, YouTube), so every byte index derived from arithmetic was a live crash. An audit of ALL char-boundary-panicking APIs — not just indexing, but also split_at, String::truncate, replace_range, split_off, drain, insert/remove, get_unchecked and multi-line slice forms — found 21 unguarded sites plus a distinct index-desynchronization class. Highest reachability first: - source_fetching/fetcher.rs (x2), extractors/pdf.rs, toolkit_http.rs: fixed byte caps (500 KB / 5 MB) cut mid-char on any large non-ASCII payload. These run on every fetch and on the user's own PDFs. - preemption.rs: the advisory-subject window fell back to a raw 80-byte cut; extract_advisory_id took an index from a `to_uppercase()` copy and applied it to the original, then capped an unterminated id at start+30. - monitoring_briefing.rs: the 19-byte GHSA window and the 4-byte CVE year window were length-guarded but not boundary-guarded. The index-desync class deserves calling out: `to_lowercase()`/`to_uppercase()` are Unicode-aware and CHANGE BYTE LENGTH (U+FB01 -> "FI" shrinks; U+0130 lowercases to two code points and grows). Four sites searched a case-folded copy and sliced the original with the result — a panic when the shift lands mid-char, and silently byte-shifted output when it does not. In preemption::extract_advisory_id that corrupted the cross-tier dedup key. All four now fold with the ASCII-only variants, which preserve byte length; every needle involved is pure ASCII. prompt_safety.rs already did this right. Fixes are per-class, matching each call site's actual intent rather than routing everything through one helper: floor/ceil_char_boundary for byte caps, chars().take/skip for the three key-mask sites (whose doc comments all say "chars"), str::get for fallible fixed-width windows, and byte-wise ASCII checks where only ASCII can match. Deliberately does NOT touch utils/text.rs — PR #416 owns that file and adds truncate_display, which is a *display* truncator (word-break + ellipsis) and would be wrong on a memory cap. 18 regression tests added. Each was verified to FAIL on the pre-fix code: 11 of the 12 original expressions panic on the exact inputs the new tests use, and the 12th returns an empty advisory id. cargo clippy -D warnings (default + experimental): clean cargo fmt --check: clean cargo test --lib: 4437 passed, 0 failed (was 4419) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUeKTKwNmdow8yUk3q8RB2
runyourempire
added a commit
that referenced
this pull request
Aug 14, 2026
…uracy, close enforcement gaps (#421) Remediation of the full-codebase audit (2026-08-12). **390 files changed, +3,349 / −54,041.** Every deletion was grep-verified repo-wide before removal, and every surface was re-verified after. Where a claim from the audit turned out to be wrong, it is corrected below rather than quietly dropped. ## Verification | Gate | Result | |---|---| | `cargo test --lib` | **4,267 passed, 0 failed** | | `cargo build --lib` | zero warnings | | `cargo fmt --check` | clean | | `pnpm typecheck` | 0 errors | | `pnpm test` | 1,249 passed | | `pnpm build` | ✓ | | site build | ✓ (corrections confirmed in built HTML) | | mcp-4da-server build + tests | ✓ 122 passing | | **IPC health** | **100%** — 0 ghost, 0 unregistered (was 95.4%) | | `validate-translations` | 0 errors | | `check-remove-by` | 0 expired (was ~40) | | `check-release-channel` | passes | ## What changed **Dead code.** The V1 scoring pipeline had been unreachable since 2026-03-03 (`const USE_V2: bool = true`, no override) yet was still co-maintained — `pipeline.rs` was edited in the AD-029 PR the day before this audit "to keep both pipelines aligned". ~3,100 lines removed. Plus `job_queue.rs` (564) and `reachability.rs` (195), both entirely dead; 18 ghost Tauri commands and their newly-orphaned backing modules (~5,800 lines, incl. `project_health.rs` and 8 `content_personalization` submodules); 1,138 dead lines from the published `@4da/mcp-server`; 14 dead React components; 23 orphan scripts; 9,230 dead i18n keys across 13 locales. **Correctness fixes found along the way.** - 19 curated feeds were 404ing on every cycle — repaired via RSS autodiscovery rather than dropped (SQLite moved to its GitHub releases feed, which actually matches its `release_notes` type). 124 of 125 now verified serving live items. - Dependency-snapshot expiry existed but was never called, so `dependency_snapshots` grew forever. Wired into `run_maintenance` — via a connection-taking helper, because the obvious call would have deadlocked on a mutex `run_maintenance` already holds. - Bluesky's 7 configured queries all fetched the same hardcoded URL (6 wasted requests/cycle, dead ACE shaping). The GitHub fallback parsed an HTML page as RSS and could never yield items. The audio extractor advertised 6 formats it cannot transcribe. - The site emitted **no robots.txt at all** — wrong passthrough path, and `.txt` isn't an Eleventy template format. - `site/rotate-key.sh` rotated the live Stripe key against the **dead Vercel project**; a rotation would have silently failed to change the key the live deployment uses. - Stale `_site` output was reaching production (Eleventy never cleaned it, `cf:deploy` uploaded wholesale), which is why 4da.ai served old server-side source and the internal Stripe E2E harness. **Legal / doc accuracy.** The privacy policy named Vercel as data processor in six places — including the GDPR international-transfer clause — three weeks after the Cloudflare migration. Licence validation is cached 90 days, not 24 hours. Terms were missing the $299 Lifetime plan, used USD where the operative terms use AUD, and described a Keygen machine fingerprint the code does not send. `LICENSE-ACTIVATION` documented a device-limit and deactivation flow that does not exist. Tier lists corrected: Developer DNA, NL search and AI briefings are free per AD-025/AD-026, and Score Autopsy, signal chains and channels have no gate at all. **Enforcement.** Added `check-remove-by.cjs` — the repo carried its own "REMOVE BY" cleanup contract that nothing ever checked, so ~40 markers had silently expired. Also replaced the pre-commit dead-code gate's shell implementation, which forked a `grep` **per line** of every staged annotated file (20+ minutes once a 5k-line file was staged; the hook's own comment recorded the symptom). Same work now runs in **1.17s**, negative-tested to confirm it still blocks. ## Corrections to the audit's own claims - **"9.8 MB dead STREETS payload"** — not simply dead. `scoring/context.rs` → `assemble_profile` → `assemble_playbook_progress` reads `docs/streets/` at runtime. It is *effectively* inert (the Playbook UI is gone, so `playbook_progress` can never be populated and it always computes zero), but removing the bundle is a scoring-input change and was left alone. - **"~4,800 orphaned lines behind ghost commands"** — overstated. `sovereign_developer_profile`, `content_personalization::context`, `tech_radar`, `playbook_commands` and `suns` all have live non-command callers. Only the command entry points were removable; the load-bearing helpers stayed. - **1.1.0 version bump** — recommended on semver grounds, then **reverted**: `check-release-channel` encodes `EXPECTED_DESKTOP_VERSION_LINE = '1.0'` with "do not publish 1.1.0 yet". The gate reflects a deliberate decision and wins. - **i18n purge bug (mine).** The first pass treated plural storage forms (`key_one`/`key_other`) as independent keys; they are referenced by their base key via `t('key', {count})`, so 24 live strings were deleted. Caught by checking every literal `t()` call site against the pre-purge baseline, restored, and redone plural-aware — **0 of 1,800 call sites broken**. That check also surfaced **185 keys already unresolvable before this work**, a pre-existing defect worth its own fix. ## Not done — needs a decision or its own PR 1. **The forked source-fetch pipeline** — highest-value remaining fix. The routine path lacks freshness/fallback/per-feed-error recording; the deep-scan path lacks the quality gate (`apply_source_quality_gate` has exactly one call site repo-wide). `sources/fallback.rs` is therefore effectively dead in normal operation. 2. **Two migration systems on one database** — `ace/db.rs` runs a second 469-line `migrate()` with no version tracking and error-swallowing ALTERs; whichever init runs first wins. Already caused the documented `kv_store` affinity drift. 3. **`ipc_guard.rs`** — tested SSRF/path-traversal validators with zero production callers. Wire them in or delete them. 4. **Hermetic Fresh-Clone never runs on `main`** — its path filters skip the matrix, so the workflow's green ticks on `main` are *skipped* jobs, not passes. Combined with no Visual Studio on the runner image, a transient `prebuild-install` failure (seen on this PR: `socket hang up`) falls through to `node-gyp` and hard-fails with nothing to fall back on. Wants a retry around the frozen install. ## Done since this description was first written - **`site/src/signal.njk`** rewritten — 9 cards to 6 (Blind Spots, Knowledge Gaps, Cross-Project Intelligence, Semantic Shifts, Standing Queries, Precision Ledger). Score Autopsy and signal chains are now stated as free in the FAQ rather than sold as Signal-tier. - **Site deployed** — `wrangler pages deploy --branch main`; the legal corrections are live on 4da.ai and verified against the domain, not the CLI's success line. - **Merged `origin/main`** (#422, UTF-8 panic-vector fix). Two conflicts, both resolved toward this branch's deletions after verifying the code was unreachable: `template_processor.rs` (module deleted; no callers) and `community_intelligence.rs` (peer's fix sat inside `get_community_status`, a ghost command removed here). Re-verified after merging: `cargo test --lib` 4,267 passed, `pnpm test` 1,249 passed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01WJ6BP3GX5HYrnjW1nGtrvC
This was referenced Aug 14, 2026
runyourempire
added a commit
that referenced
this pull request
Aug 14, 2026
…g background refresh (#428) ## The bug A panic anywhere in the scheduled-analysis path **permanently disabled background refresh for the rest of the process lifetime**. No error, no UI signal — the feed just quietly stopped updating. Verified mechanism: 1. `monitoring.rs:1183` / `:1335` claims the gate: `is_checking.swap(true, SeqCst)` — **before** `scheduled-analysis` is emitted. 2. `app_setup.rs:785` ran `run_scheduled_analysis(handle)` inside a bare `tauri::async_runtime::spawn`. The `JoinHandle` is dropped, so nothing ever observes a `JoinError`. 3. That awaits `fill_cache_background(&handle).await` unguarded (`app_setup.rs:1985`). 4. **Every** site that clears the gate — `app_setup.rs:2011`, `:2109`, `monitoring.rs:1348`, `monitoring_notifications.rs:69` — sits **after** the work. So one unwind skipped all four clear sites and latched `is_checking` at `true` forever. Every subsequent tick saw `is_checking == true` and silently skipped itself. This was already a known shape with no recovery path: `void_engine/heartbeat.rs:158` carries the comment *"check if monitoring is_checking stuck (simple heuristic)"*. `headless.rs:258` had the twin failure — an unwind out of a cycle propagated through the daemon loop and killed `fourda-engine` outright. ## The fix Contain the panic at the boundary so a bad cycle becomes a **skipped cycle**, not a permanent wedge. - **`src-tauri/src/task_guard.rs`** (new) — `contain()`, an `AssertUnwindSafe(..).catch_unwind()` wrapper that logs the panic payload and returns `None`. Mirrors the pattern already proven in `analysis_status.rs:59-68`. - **`app_setup.rs:1983-2025`** — `run_scheduled_cycle_contained()` wraps the cycle and releases the gate in the **recovery arm only**. Deliberately *not* an unconditional clear: clearing after a normal completion could race a tick that has already claimed the gate for the *next* cycle, trading a wedge for two concurrent scheduled analyses against shared state. The normal paths already release it; only the unwind path was unreachable. - **`headless.rs:583-599`** — the daemon loop survives a panicking cycle and retries on the next tick instead of dying. - **`headless.rs:133-144`** — `--once` exits with its documented code `1` instead of an unwind/abort, so an OS scheduler records a clean failed run. ## Proving the regression test is real A test that passes both before and after proves nothing, so this was falsified explicitly: the containment in `run_scheduled_cycle_contained` was temporarily replaced with a bare `cycle.await` (the exact unfixed shape) and the suite re-run. ``` cycle_panicking_before_first_await_releases_the_gate ......... FAILED panicking_scheduled_cycle_releases_the_in_flight_gate ........ FAILED normal_completion_leaves_the_gate_exactly_as_the_cycle_left_it ok normal_completion_preserves_the_cycles_own_gate_release ...... ok ``` Both panic tests fail against unfixed code (the unwind escapes and kills the test); both success-path tests still pass, confirming they are not just passing by accident. The containment was then restored and all four pass. The tests use a local `MonitoringState` rather than the process-wide global, so they are hermetic under parallel execution. ## `clippy::string_slice` — measured, then scoped The intent was a crate-wide `string_slice = "deny"` on the theory that residual hits would be few after #422. **Measured against this tree it fires 280 times**, so that premise did not hold: | Area | Hits | Area | Hits | |---|---|---|---| | `sources/` | 55 | `monitoring_briefing.rs` | 13 | | `ace/` | 38 | `preemption.rs` | 10 | | `scoring/` | 36 | `diagnostics.rs` | 7 | | `content_personalization/` | 25 | *(~90 others)* | — | The lint **is** the right one: the only two hits in `source_fetching/` are #422's own `floor_char_boundary` slices, which confirms it would have flagged all 23 original panic sites. But bulk-`#[allow]`ing 278 pre-existing sites — 55 of them in `sources/` adapters that parse ingested text — would risk **cementing a live panic** behind an allow that claims safety without an audit. That is the same "large churn" bar this PR was told to apply to `indexing_slicing`. So it is adopted **per-module**, starting where the panic class actually lives: - `#![deny(clippy::string_slice)]` on `src/source_fetching/mod.rs` — the module that caps scraped bodies, and where two of #422's 23 panics were. - The two flagged sites collapse into one `cap_on_char_boundary()` helper carrying the single char-boundary proof, removing the duplication. - `fetcher_tests.rs` previously **re-implemented** the capping logic inline (`content[..CONTENT_CAP]`), so it asserted against a copy of the logic and could never have caught the original bug. It now calls the real function, plus new multi-byte (3-byte `あ` across every cap 1..=8) and emoji (4-byte) boundary cases. - `Cargo.toml` carries the measurement and the promotion path for whoever audits the backlog. ## Verification | Check | Result | |---|---| | `cargo clippy -- -D warnings` (default) | clean | | `cargo clippy --features experimental -- -D warnings` | clean | | `cargo fmt --check` | clean | | `cargo test --lib` | **4446 passed**, 1 pre-existing flake | The one failure is `db::migrations::recovery_tests::locked_db_returns_recovery_failed_without_quarantine`, a timing assertion (*"locked DB recovery must return quickly, elapsed 32.7s"*). It **passes in 2.03s in isolation** — starved by parallel load on a busy machine, unrelated to this diff, which touches no DB-migration code. `cargo clippy --all-features` is pre-existing broken on main (~56 errors in `team_sync`/`webhooks`, `chacha20poly1305::generate_nonce` gone from the locked `aead 0.6.1`) and was not touched. This diff cannot affect it: `source_fetching` contains **zero** `cfg(feature)` code, so the scoped deny behaves identically under every feature combination. ## Follow-ups deliberately left out - **`clippy::indexing_slicing`** — not added, as instructed. Not separately measured; expected to exceed the 280 `string_slice` count since it covers slices and index ops on all sequence types. - **The 278-site `string_slice` backlog** — needs a real per-site audit, best done module-by-module (`sources/` 55 first — highest ingested-text exposure). Some of those may be live panics of exactly the #422 kind. - **The other ~54 unguarded `spawn` sites** — scoped out per instructions. `contain()` is now available for them. No `spawn_guarded` wrapper was added: both call sites here need custom recovery (a gate release / an exit code), so a generic spawner would have had zero users and tripped the dead-code doctrine. - **`crash_guard` interaction (pre-existing, worth knowing)** — `crash_guard.rs` installs a panic *hook* that zeroizes API keys on **any** unwind, including ones caught by `catch_unwind`. That already applied to the existing guards in `analysis_status.rs` and `extractors/pdf.rs`; this PR does not change it. But it means a *survived* scheduled panic leaves the process running with zeroized LLM keys. std's hook API cannot distinguish caught from fatal panics, so fixing it is a separate design question. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01AUeKTKwNmdow8yUk3q8RB2 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
runyourempire
added a commit
that referenced
this pull request
Aug 16, 2026
…on the user's own tech stack #422 fixed 17 byte-slicing sites. It could not fix seven it did not know about: the same word-boundary helper had been written **eight times** across the tree, and exactly one copy — `scoring/utils.rs` — was UTF-8 safe. The other seven advanced their search cursor with search_from = abs + 1; // next iteration: text[search_from..] `abs` is the START of a match, so `abs + 1` is a char boundary only when the needle's first char is one byte. And the advance is reached ONLY when the word-boundary test FAILS — i.e. when the match abuts an alphanumeric char. Both conditions must hold, which is exactly why every ASCII test in the tree passed over this for as long as it existed. ("cafe2" is safe; "eclair2" is not, because the term's first char is two bytes.) **The P0 is `signals.rs`.** Verified chain: `scoring/pipeline_v2.rs` -> `clf.classify(..., &ctx.declared_tech, ...)` -> `has_word_boundary(&title_lower, &t)`. `ctx.declared_tech` is the tech stack the user typed at onboarding, and this runs on every item of every scoring pass. One non-ASCII stack entry plus a title where that term abuts an alphanumeric char aborted the whole pipeline. `signals.rs` also had no empty-term guard (N3/N5/N6 did), so an empty stack entry walked the string one byte at a time and panicked at the first ASCII-letter-followed-by-multibyte position. Fixed sites: signals.rs, scoring/dependencies.rs, package_ambiguity.rs, knowledge_decay.rs, dep_linker.rs, preemption.rs, competing_tech.rs, stacks/scoring.rs. The last two take their terms from const tables today, so they were latent — one non-ASCII entry away from live. **The duplication is the actual defect**, so it is gone. The correct implementation is promoted to `utils/text.rs` — not `scoring/utils.rs`, because six of the eight call sites live outside `scoring/` and should not take a dependency on scoring internals for a plain string primitive, and because `utils/text.rs` already houses `truncate_utf8` whose doc comment states the identical rationale. `scoring::utils` now re-exports it, so the ~30 `super::utils::has_word_boundary_match` call sites are untouched. The predicates genuinely disagree and collapsing them would have been a behaviour change, not a refactor, so the shared API is parameterised: - `has_word_boundary_match` — alphanumeric boundaries (5 sites) - `has_word_boundary_match_with_ext` — plus `.js`/`.ts`/`.rs` as a right boundary, for the package-name matchers ("next.js" IS `next`) - `has_bounded_match(.., is_word_char)` — dep_linker and stacks/scoring treat `-`/`_`/`.`/`@` as name-internal - `match_offsets` + `char_before`/`char_at` — for preemption's asymmetric rule (left must be non-alphanumeric, right need only not be a hyphen) and dependencies' `.js`-suffix logic Boundary tests now run on CHARS, not bytes, everywhere. `as_bytes()[i-1] .is_ascii_alphanumeric()` is false for every UTF-8 continuation byte, so a non-ASCII letter glued to the term ("иgo") read as a word boundary and "go" matched — bug E, previously fixed in one copy only. Also fixes a false documented invariant in the same family (`scoring/dependencies.rs`). `has_adjacent_version_literal` took one `name_len` for every position, commented "every accepted form equals the normalized single-token name, so `name_len` is uniform". It is not: `normalize_package_name` strips a leading `@`, so `@foo` yields forms of length 3 and 4. Every `@foo` hit started its version scan one byte INSIDE the name — and did so with unsnapped arithmetic, which panics when that lands mid-char. `package_name_positions` now returns `(offset, form_len)` pairs, so the arithmetic is correct by construction rather than by a premise the next edit would have trusted. Every fix has a test that fails without it — a non-ASCII term (multi-byte FIRST char) abutting an alphanumeric char, asserting both no-panic and the correct boolean — plus one end-to-end test per live entry point (`SignalClassifier::classify` with a non-ASCII declared stack, `compute_competing_penalty` with a non-ASCII primary stack). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
runyourempire
added a commit
that referenced
this pull request
Aug 17, 2026
…on the user's own tech stack (#471) Closes the UTF-8 panic vectors that #422's sweep did not know about, plus the INV-003 violation and the unvalidated numeric IPC boundary. `cargo fmt --check` clean · `cargo clippy -- -D warnings` (the repo's gate) clean · `cargo test --lib` **4,415 passed, 0 failed** · file-size gate clean. ## Eight copies of one helper, seven of them broken `has_word_boundary_match` exists in eight places. Exactly one was correct. The other seven advanced the search cursor with `search_from = abs + 1`, where `abs` is the **start** of a match — which is a character boundary only when the needle's first character is one byte. The worst copy, `signals.rs`, takes **the user's own onboarding tech stack** as its needle and runs on every item in every scoring pass, reached from `pipeline_v2.rs` → `clf.classify(..., &ctx.declared_tech, ...)`. It also had no empty-term guard, which three of its siblings did have, so an empty stack entry walked the string byte by byte. ### The brief I was working from was wrong about the trigger, and it mattered I was told a non-ASCII term abutting an alphanumeric character panics. That is too loose: the term's **first character** must be multi-byte. `"café"` is safe — `c` is one byte, so `abs + 1` lands cleanly. `"éclair"` is not. My first pass of tests used `café`. Every one of them passed against the unfixed code. I caught it before committing and rewrote all of them with `éclair` / `привет` / `我们` / `🦀`. Flagging it because a suite of tests that cannot fail is precisely the defect this audit was about, and I nearly shipped one into the fix for it. ### Two of the seven are not copies `preemption::is_compound_prefix_match` and `scoring::dependencies::package_name_positions` share the defective cursor but implement genuinely different boundary rules — the first is asymmetric (left must be non-alphanumeric, right need only not be a hyphen), the second handles `.js`/`.ts`/`.rs` suffixes and sentence periods. They cannot collapse into one boolean helper without changing behaviour, so they take the new `match_offsets` / `char_before` / `char_at` primitives instead, with the divergence documented in place. ### Where the shared helper went `utils/text.rs`, not `scoring/utils.rs`. Six of the eight call sites live outside `scoring/`, and making `preemption`, `dep_linker`, `competing_tech` and `stacks` depend on scoring internals for a plain string primitive is the wrong direction. `utils/text.rs` already houses `truncate_utf8`, whose doc comment states the same rationale. `scoring::utils` re-exports it, so roughly 30 existing call sites are untouched. ## Four more defects in the same family **A truncated model response panicked the judge.** `llm_judge.rs` did `&response[start..=end]` from `find('[')` and `rfind(']')` with no ordering guard, so a response whose last `]` precedes its first `[` panics. Its sibling in `blind_spots.rs` already had the guard and a doc comment promising "never a panic". The test that should have caught it **re-implemented the unguarded expression inline** instead of calling the function, so the suite reproduced the bug rather than detecting it. Both fixed. **Streaming destroyed every multi-byte character straddling a network chunk.** All three transports (Anthropic SSE, OpenAI SSE, Ollama NDJSON) called `String::from_utf8_lossy` per chunk, so a character split across a packet boundary became U+FFFD in both halves — silent corruption of user-visible output, worst on non-English text and emoji. Now decoded at line boundaries. **A false invariant in the version-literal check.** `has_adjacent_version_literal` computed an unsnapped offset while trusting a comment claiming all accepted forms share one length. They do not: `normalize_package_name` strips a leading `@`, so `@foo` yields forms of length 3 and 4. **An unvalidated number from the frontend could abort the process.** `taste_test_respond` passed `item_slot: usize` straight from IPC into `assert!(item_slot < NUM_ITEMS)` — `assert!`, not `debug_assert!`, so live in release. `ipc_guard.rs` validated strings, URLs and paths but had no numeric validator at all. It does now. Because the command is `async`, its panic also left the frontend's `invoke()` promise unsettled until the 30-second timeout — so this was a process abort *and* a UI hang. The assert is downgraded to `debug_assert!` with a release backstop that returns rather than indexing out of bounds. ## INV-003: the file watcher could die and nothing could tell `cb(changes)` ran unguarded inside the watcher thread, so a panic anywhere in the extractor chain killed it. What made it silent is worse than the panic: the `running` flag it would have set is **private with no getter and read by nothing**, and the only health surface counted `file_signals` rows in the last hour — so a dead thread reported Healthy for an hour and then looked exactly like an idle developer. The callback is now contained, `running` is cleared on panic and logged at error level, and `check_watcher` is driven by **liveness** rather than row counts. `check_all_components` takes `watcher_alive` as a parameter rather than fetching it internally, because the one production caller already holds the ACE read guard and the connection lock — fetching inside risked re-entrancy. ## Verification I reverted **every** fix in one pass, keeping all the new tests, and ran the suite: **20 tests went red.** One did not, and that is the useful part. `compute_competing_penalty_survives_non_ascii_stack` passed in both states, because `compute_competing_penalty` only reaches the helper when the stack entry is a **key of the `COMPETING_TECH` const table** — a non-ASCII entry can never get there, so my end-to-end test was vacuous. It is replaced (`975ecdf6`) with a test of the actual premise: every key and competitor in that table is ASCII, which fails the day that stops being true. ## Lint adoption `#![deny(clippy::string_slice)]` now covers **14 modules**, up from one. Eleven remaining slices inside them carry `#[allow]` with a stated boundary proof. The tree-wide count went 246 → 221 after the fixes → **210** after annotations. The `Cargo.toml` comment quoted a stale 280; it is corrected and now says to re-measure before quoting. No crate-wide deny — the remaining backlog (`ace/scanner.rs` 30, `monitoring_briefing.rs` 13, `sources/*` ~45) is real, and blanket-allowing it would cement whatever live panic is hiding in it. ## Two caveats `cargo clippy --all-targets -- -D warnings` is **not** clean on this tree — 1,004 findings, pre-existing and toolchain-driven, with `tests/victauri_dogfood.rs` alone accounting for 579. I verified this branch adds none by intersecting the finding list against its own changed files: the six overlapping files carry only pre-existing findings. The two this branch did introduce were removed before commit. `utils/text.rs` (703) and `llm_stream.rs` (710) crossed the 700-line **warn** threshold. No errors and no gate breakage, but they belong on the split-candidates list. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Fq96xWyPQjx2bCCzWtsnC9 --------- Co-authored-by: Claude Opus 4.8 <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.
What
&s[a..b]on a&strpanics when a bound lands mid-UTF-8-sequence. 4DA ingests arbitrary internet text (HN, Reddit, RSS, Mastodon, arXiv, OSV/GHSA, YouTube), so every byte index derived from arithmetic was a live crash waiting on the right title.This audits every char-boundary-panicking API — not just indexing — and fixes all 23 unguarded sites.
Audit coverage
&s[a..b]str::split_at&[u8], len-guarded)String::truncateVec, zeroStringString::replace_rangefind-derived, ASCII delimiters)String::split_off/drainVec)insert/remove/get_unchecked/.get(..).unwrap()rg -U)is_char_boundarywalk-downsHighest reachability first
source_fetching/fetcher.rs(x2),extractors/pdf.rs,toolkit_http.rs— fixed byte caps (500 KB / 5 MB) cut mid-char on any large non-ASCII payload. Runs on every fetch, on user PDFs, and on any URL the user probes.preemption.rs— the advisory-subject window fell back to a raw 80-byte cut;extract_advisory_idhad two stacked bugs (desync +start + 30).monitoring_briefing.rs— the 19-byte GHSA window and 4-byte CVE year window were length-guarded but not boundary-guarded.The index-desync class
to_lowercase()/to_uppercase()are Unicode-aware and change byte length (U+FB01 ->FIshrinks; U+0130 lowercases to two code points and grows). Four sites searched a case-folded copy and sliced the original with the result — a panic when the shift lands mid-char, and silently byte-shifted output when it does not. Inpreemption::extract_advisory_idthat corrupted the cross-tier dedup key, so alerts stopped collapsing.All four now use the ASCII-only folding variants, which preserve byte length; every needle involved is pure ASCII.
prompt_safety.rsalready did this correctly and was the model.Design notes
Fixes are per-class, matching each call site's intent rather than routing everything through one helper:
floor_char_boundary/ceil_char_boundaryfor byte caps (also subsumes the old.min(len)clamps)chars().take/skipfor the three key-mask sites — their doc comments all say "chars", and the old byte-length star count rendered one*per bytestr::getfor fallible fixed-width windowsDeliberately does not touch
utils/text.rs. #416 owns that file and addstruncate_display, which is a display truncator (word-break + ellipsis) — semantically wrong for a memory cap, where an ellipsis would corrupt the payload. Zero file overlap with #416, #418, #419, #420, #421.Verification
18 regression tests added, and each was verified to fail on the pre-fix code: 11 of the 12 original expressions panic on the exact inputs the new tests use; the 12th returns an empty advisory id.
cargo clippy -- -D warnings(default +--features experimental): cleancargo fmt --check: cleancargo test --lib: 4437 passed, 0 failed (was 4419)Pre-existing, not addressed here
cargo clippy --all-featuresfails onmainwith 56 errors inteam_sync_commands/webhooks/team_sync_crypto— including dependency-API drift (chacha20poly1305::generate_nonceno longer exists, deprecatedsha2from_slice). CI builds onlydefaultandexperimental, so this is not gating, but it is worth a separate issue.