[Audit/code-health] shared/sync + shared/mcp — 8 findings (1 medium, 7 low)

[cuttlefisch]

Code-health findings for shared/sync + shared/mcp, from the horizontal per-crate pass of the pre-v0.15
audit (epic #592).

This pass deliberately looked for what capability tracing structurally cannot see — dead code, AI-slop,
error-convention drift, module boundaries, doc drift. It was scoped away from re-reporting #569-#601.

Every claim was independently re-verified by a reviewer briefed to refute it; dead-code claims had to
survive a re-run repo-wide grep, and AI-slop was refuted aggressively as the most subjective category.
Of 96 claims across the whole pass, 18 were refuted and all 13 claimed-high were downgraded — the
serious defects had already been caught by the capability pass, so what remains here is hygiene.

8 findings — 1 medium, 7 low. Kinds: bug×3, doc-drift×2, ai-slop×1, anti-pattern×1, dead-code×1.

---

1. Hand-rolled hex::decode byte-slices a &str — an unauthenticated peer panics the PSK handshake with a non-ASCII proof

bug · medium

Why it matters. A remote, unauthenticated client can panic the connection task with a two-field JSON message. With the default unwind profile this drops that one connection rather than the process, but it is a library panic on wholly untrusted input in the authentication path — exactly what principle #14's attacker-model tests exist to catch, and the pattern that turns into a full crash under any future panic = "abort" or panic-propagating supervisor.

Evidence

shared/mcp/src/auth.rs:767-775 — pub fn decode(s: &str) -> Option<Vec<u8>> { if !s.len().is_multiple_of(2) { return None; } (0..s.len()).step_by(2).map(|i| u8::from_str_radix(&s[i..i + 2], 16).ok()).collect() }. s.len() is a BYTE length but &s[i..i+2] is a str range index, which panics unless both ends land on char boundaries. Input "aéb" is 4 bytes (61 C3 A9 62), passes the even-length check, and &s[0..2] splits the 2-byte é → byte index 2 is not a char boundary panic. Reached pre-auth: shared/mcp/src/auth.rs:317-325 parses the client's AuthResponse and calls Self::verify_with(&key.secret, &server_nonce, &client_nonce, &response.proof), and verify_with's first statement is let proof = match hex::decode(proof_hex) (auth.rs:230). response.proof is arbitrary attacker JSON. PskAuth::server_handshake runs on the daemon's collab TCP listener (daemon/src/collab_handler/mod.rs:92) and on MAE's own PSK MCP socket (shared/mcp/src/lib.rs:338), both before any authentication. The existing tests (auth.rs:1039-1041) only feed ASCII ("xyz", "abc"), so the boundary case is untested.

Verification

The code and the reachability both check out. shared/mcp/src/auth.rs:766-775: pub fn decode(s: &str) -> Option<Vec<u8>> { if !s.len().is_multiple_of(2) { return None; } (0..s.len()).step_by(2).map(|i| u8::from_str_radix(&s[i..i + 2], 16).ok()).collect() } — s.len() is bytes, &s[i..i+2] is a char-boundary-checked str index, so "aéb" (bytes 61 C3 A9 62, even length) panics at &s[0..2]. Pre-auth reachability confirmed: auth.rs:316-325 serde_json::from_str::<AuthResponse>(line.trim()) then Self::verify_with(&key.secret, &server_nonce, &client_nonce, &response.proof), and verify_with's first statement (auth.rs:230) is let proof = match hex::decode(proof_hex). response.proof is arbitrary attacker-controlled JSON. server_handshake runs before authentication on the daemon's collab TCP listener (daemon/src/main.rs:964/983 → collab_handler/mod.rs:90-94) and on MAE's PSK MCP socket (shared/mcp/src/lib.rs:338). Tests at auth.rs:1039-1041 only feed ASCII ("00ff10", "xyz", "abc"), so nothing covers the boundary case — exactly principle #14's gap.

Scope correction from verification: Downgraded from high to medium: the handshake runs inside tokio::spawned per-connection tasks (daemon/src/main.rs:889 → :964/:983), and neither workspace sets panic = "abort", so the blast radius is one dropped connection, not a process crash. The claim itself concedes this. Still a genuine library panic on wholly untrusted pre-auth input, and the one-edit fix (delete mod hex, use the already-declared hex = "0.4" crate) is the same edit as the finding below.

---

2. Local mod hex duplicates the hex crate the manifest already depends on, with a stale justification comment

ai-slop · low

Why it matters. The stated reason for the duplicate no longer holds, and the divergent return types make the panic finding above easy to miss on review (hex::decode(..) reads as the well-tested crate function). Deleting mod hex and using the dependency removes both the duplication and the panic in one edit.

Evidence

shared/mcp/src/auth.rs:760 — /// Hex encoding/decoding helpers (avoids adding \hex` crate).followed bymod hex { .. }. shared/mcp/Cargo.toml:26 declares hex = "0.4", and two sibling modules in the same crate use the real crate: shared/mcp/src/content_key_store.rs:40 hex::decode(content.trim()).ok()?(Result-returning) and shared/mcp/src/collection_store.rs:80let Ok(kb_bytes) = hex::decode(stem) else. So the crate carries two hex::decodes with *different signatures* (OptionvsResult`) resolved purely by which module you are in — and the shadowing one is the buggy one (see the panic finding above).

Verification

Verified. shared/mcp/src/auth.rs:760-761: "/// Hex encoding/decoding helpers (avoids adding hex crate)." followed by mod hex { ... } — while shared/mcp/Cargo.toml:26 declares hex = "0.4" as a direct dependency, used as the real crate in two sibling modules of the same crate (content_key_store.rs:40 hex::decode(content.trim()).ok()? — Result-returning, and collection_store.rs:80 let Ok(kb_bytes) = hex::decode(stem) else). Inside auth.rs the local module shadows the crate, so hex::decode there is the Option-returning hand-rolled one — which is the one that panics (finding 1 above). The stated justification for the duplicate is therefore factually stale, and the shadowing makes the panic easy to miss on review.

---

3. The MCP server's PSK handshake has no deadline and no read-size bound — the daemon's identical handshake has both

anti-pattern · low · already tracked in #342

Why it matters. A client that connects to MAE's PSK MCP socket and sends nothing pins a tokio task and an fd for the process lifetime; a client that sends a newline-free stream grows a single String without limit (bounded only by the daemon's 10s deadline on that listener, and by nothing at all on MAE's). The protection already exists and is documented one crate away — this is the shared component not being applied at the second call site rather than a missing design.

Evidence

shared/mcp/src/lib.rs:337-354 calls psk.server_handshake(&mut reader, &mut writer).await with no tokio::time::timeout wrapper, while the daemon wraps the same AuthProvider::server_handshake in one: daemon/src/collab_handler/mod.rs:90-94 tokio::time::timeout(Duration::from_secs(HANDSHAKE_TIMEOUT_SECS), auth.server_handshake(..)), whose constant carries the rationale verbatim — "#342: … an accepted-but-silent connection … would otherwise park a task+socket forever with nothing to reclaim it" (collab_handler/mod.rs:60-67). Separately, both handshake implementations read with unbounded reader.read_line(&mut line).await? (auth.rs:272, :316, :504, :536) with no cap, whereas the post-auth read_message on the same stream enforces MAX_HEADER_SIZE (16 KB, lib.rs:600-613) and MAX_MESSAGE_SIZE (lib.rs:639-644).

Verification

Verified. shared/mcp/src/lib.rs:337-354 calls psk.server_handshake(&mut reader, &mut writer).await bare, while daemon/src/collab_handler/mod.rs:88-94 wraps the same AuthProvider::server_handshake in tokio::time::timeout(Duration::from_secs(HANDSHAKE_TIMEOUT_SECS), ...) with the rationale spelled out at :60-67 ("#342: ... an accepted-but-silent connection ... would otherwise park a task+socket forever with nothing to reclaim it"). Both handshake bodies read with unbounded reader.read_line(&mut line).await? (auth.rs:272, :316), unlike the post-auth read_message which enforces MAX_HEADER_SIZE / MAX_MESSAGE_SIZE (lib.rs:600-644).

Scope correction from verification: Severity is inflated by treating the two listeners as equivalent. The comment immediately above the un-timed call (lib.rs:325-336) records that MAE's PSK path is "a SEPARATE, second Unix socket dedicated to first-party local-model harnesses" — a local /tmp/mae-{pid}-agent.sock, same-UID-only, not a network listener; the daemon's timed path is the TCP collab listener. So the exposure is a local same-user process holding an fd, not a remote resource-exhaustion vector. The daemon half of this (accept-loop caps) is already tracked as open issue #342. Accurate narrow finding: apply the existing HANDSHAKE_TIMEOUT_SECS pattern to shared/mcp/src/lib.rs:338 and bound the handshake read_line. Low.

---

4. write_secure writes secret files at umask permissions and only chmods afterwards — TOCTOU window on the identity key, PSK keystore and per-KB content keys

bug · low

Why it matters. A local unprivileged process that can stat the mae data dir can open the file inside the window and hold the fd past the chmod, reading the long-term identity private key or a KB content key. keystore.rs's own test only asserts the mode after the call (add_key_creates_secure_file_and_roundtrips), so the window is invisible to the suite. Fix is OpenOptions::new().mode(0o600).create(true).truncate(true) on unix (or write-to-temp-then-rename), which also makes the doc comment's claim true.

Evidence

shared/mcp/src/keystore.rs:220-223 — pub fn write_secure(path: &Path, content: &str) -> std::io::Result<()> { std::fs::write(path, content)?; set_secure_file_perms(path) }. std::fs::write creates with 0o666 & !umask (0644 under the common 022), so the secret is on disk group/world-readable until the following set_permissions(.., 0o600) lands. The doc comment two lines above claims the opposite: "On unix: chmod 0600 (a failure propagates — we never leave a secret world-readable silently)" (keystore.rs:225-227) — but on a set_permissions failure the already-written world-readable file is left in place, only the error is returned. This is the single write path for every MAE secret: the Ed25519 identity key (shared/mcp/src/identity.rs:316 write_secure(&dir.join("id_ed25519"), ..)), the authorized-keys/known-hosts files (identity.rs:414, 577, 588, 600), per-KB content keys (shared/mcp/src/content_key_store.rs:34), and the PSK (crates/mae/src/main.rs:993).

Verification

The mechanism is as described — shared/mcp/src/keystore.rs:220-223 pub fn write_secure(path: &Path, content: &str) -> std::io::Result<()> { std::fs::write(path, content)?; set_secure_file_perms(path) } — so the file exists at 0o666 & !umask until the set_permissions(.., 0o600) at keystore.rs:235 lands.

Scope correction from verification: The claimed consequence is prevented at almost every call site by an upstream gate the finding did not trace: every secret-file writer hardens the containing directory to 0700 BEFORE writing. content_key_store.rs:31-34 std::fs::create_dir_all(dir)?; secure_dir(dir); crate::keystore::write_secure(...); identity.rs:307-317 (identity key) does the same; keystore.rs:180-182 (add_key) and identity.rs:407-410 / :577-580 (known_hosts / authorized_keys) all call create_dir_all(parent) + secure_dir(parent) first, where secure_dir is set_permissions(dir, from_mode(0o700)). A non-owner cannot traverse a 0700 directory, so there is no window to open the file in. The one genuinely exposed site is the per-process PSK: crates/mae/src/main.rs:991-993 writes /tmp/mae-{pid}.psk via write_secure with no directory hardening at all — that file really is 0644 in a world-traversable directory for the duration of the chmod. Accurate narrow finding: harden the /tmp PSK write (or give write_secure an OpenOptions::mode(0o600) create on unix). Severity low.

---

5. had_full_replication_window reads the crypto-valid op set instead of the authorization-derived one, so any op-log writer can suppress the residual-replica warning

bug · low · already tracked in #449

Why it matters. ADR-067's residual-replica signal is owner-facing and informational (docs/adr/067-…:455-463), so this is not an access-control bypass — but it is the one derive in this module that reads an unvalidated op set, which is exactly the pattern ADR-026 exists to forbid ("never read as a stored verdict"), and it silences a warning rather than over-reporting. Intersecting with derive_valid_members_governed's valid set would cost nothing here.

Evidence

shared/sync/src/membership.rs:1130-1160 replication_history filters with crypto_valid(ops) — which only checks signature + fingerprint binding (membership.rs:572-577) — and then takes every Admit/SetRole in causal_order whose subject == principal, with no authorized() / valid-set intersection. had_full_replication_window (membership.rs:1170-1181) then does let (last, earlier) = history.split_last()?; if *last != ReplicationPolicy::QueryOnly { return None; }. Any principal holding any keypair can chain a self-signed SetRole{subject: victim, replication: Full} onto an existing op hash: it fails authorized so it changes no real membership, but it becomes the last entry here and the function returns None — "not restricted, nothing to report". Consumed by crates/core/src/kb_sharing.rs:239 via had_full_replication_window_self_anchored.

Verification

The derive really does read the unvalidated set. shared/sync/src/membership.rs:1130-1160 replication_history starts let crypto: Vec<&SignedMembershipOp> = crypto_valid(ops); — and crypto_valid (membership.rs:571-577) filters only on o.verify_signed() || is_recovery_signed_rebind(...), i.e. signature + fingerprint↔pubkey binding, which any freshly generated keypair satisfies. It then takes every Admit/SetRole in causal_order whose subject == principal with no intersection against derive_valid_members_governed's authorized set. had_full_replication_window (membership.rs:1170-1181) does let (last, earlier) = history.split_last()?; if *last != ReplicationPolicy::QueryOnly { return None; }, so a chained self-signed SetRole{subject: victim, replication: Full} becomes last and returns None. Consumed in production at crates/core/src/kb_sharing.rs:238-241 (residual_replica_risk, the KB Sharing buffer).

Scope correction from verification: Two things bound this to genuinely low. (1) It has no live consequence today: as this audit already established (verdicts/med4.json, tracked under open epic #449), nothing in production ever authors a SetRole or sets ReplicationPolicy::QueryOnly — ADR-067 itself says "No RPC surface exists yet to set replication on a member (out of this phase's scope)" — so the signal being suppressed never fires in the first place. (2) The function's own doc (membership.rs:1105-1130, :1163-1169) is explicit that this is a best-effort informational owner-facing bound, not an access-control decision. The accurate finding is a latent hardening gap to close before ADR-067 Phase B ships a real QueryOnly writer: intersect replication_history with the governed valid set. Belongs on epic #449, not as a standalone defect.

---

6. ClientSession idle detection is entirely dead code while three docs advertise it

dead-code · low

Why it matters. No MCP session is ever reaped for idleness — a client that connects and goes silent holds its slot indefinitely, which the docs say is handled. Three fields on an already-over-ceiling 17-field struct exist only to feed a mechanism nobody invokes. Either wire is_idle into handle_client's select loop or delete the fields and correct the two docs.

Evidence

rg -c 'is_idle' -g '!target' → shared/mcp/src/session.rs:2 only (the definition at session.rs:178 plus its own unit test at :230); nothing outside the file calls it. last_activity is written by touch() (session.rs:173, called from lib.rs:411 and the $/ping arm) and read by nothing but is_idle. messages_sent (session.rs:49) is initialised to 0 at session.rs:125 and never incremented or read anywhere — rg -c messages_sent → shared/mcp/src/session.rs:2 (declaration + init) and one unrelated hit in crates/ai/src/guardrail.rs. Meanwhile docs/MCP_ARCHITECTURE.md:118 lists "connected_at, last_activity (for idle detection)" and :212 describes session.rs as "ClientSession struct, idle tracking", and CLAUDE.md's Server-Client Architecture section claims "Heartbeat: $/ping returns "pong", idle detection via last_activity". The daemon implements its own unrelated idle close (daemon/src/main.rs:1498-1506).

Verification

Re-ran the greps repo-wide. rg -n 'is_idle' -g '!target' . → only shared/mcp/src/session.rs:178 (definition) and :232 (its own unit test) — no caller outside the file, and no trait impl or macro could reach it (it is an inherent pub fn on a plain struct). last_activity is written by touch() (session.rs:174, called from lib.rs:411 and the $/ping arm) and read only by is_idle. messages_sent is declared at session.rs:49, initialised to 0 at :125, and never incremented or read — the only other repo hits for the token are max_messages_sent in crates/ai/src/guardrail.rs, unrelated. Meanwhile docs/MCP_ARCHITECTURE.md:118 ("connected_at, last_activity (for idle detection)") and :212 ("ClientSession struct, idle tracking") and CLAUDE.md:564 ("idle detection via last_activity") all advertise the mechanism. The daemon's unrelated idle close (daemon/src/main.rs) does not cover MCP sessions.

---

7. reconcile_to's doc comment states the opposite offset kind from the code and from every other method in the file

doc-drift · low

Why it matters. reconcile_to is the CRDT-safe local-undo/redo delta path (CLAUDE.md #11). A maintainer who trusts this comment while editing offset arithmetic reintroduces exactly the non-ASCII offset-mismatch class of bug the file was changed to fix, and the corruption lands in the shared document, not just the local view.

Evidence

shared/sync/src/text.rs:531-532 — "Note: yrs uses byte offsets (OffsetKind::Bytes), so we track byte offsets alongside char offsets throughout the diff application." The doc is created with offset_kind: OffsetKind::Utf16 (text.rs:36 and :77), the body of reconcile_to tracks let mut utf16_offset: u32 = 0; and computes change.value().chars().map(|c| c.len_utf16() as u32).sum() (text.rs:547-561), and the sibling methods say the correct thing — insert/delete: "yrs is configured with OffsetKind::Utf16 (the Yjs standard)" (text.rs:222, :247). text.rs:33 even records why: "default OffsetKind::Bytes causes char↔yrs offset mismatches for non-ASCII text."

Verification

Verified verbatim. shared/sync/src/text.rs:531-532: "Note: yrs uses byte offsets (OffsetKind::Bytes), so we track byte offsets alongside char offsets throughout the diff application." The doc is constructed with offset_kind: OffsetKind::Utf16 (text.rs:36 and :77), and reconcile_to's body (text.rs:547-561) tracks let mut utf16_offset: u32 = 0; with change.value().chars().map(|c| c.len_utf16() as u32).sum() — there is no byte offset anywhere in the function. The sibling methods say the correct thing (insert at text.rs:222, delete at :247: "yrs is configured with OffsetKind::Utf16 (the Yjs standard)"), and text.rs:33 records why: "the default OffsetKind::Bytes causes char↔yrs offset mismatches for non-ASCII text." The comment is flatly, uniquely wrong.

Scope correction from verification: Confirmed as stated; severity lowered from medium to low. This is a comment with no behavioural consequence — reconcile_to is correct today, the surrounding four doc comments state the right thing, and the field it describes is set two screens away in the same file. It is a one-line delete, not a latent defect.

---

8. membership.rs's @ai-caution states a line count that is 12% stale and contradicts its own next sentence

doc-drift · low

Why it matters. This is the single largest file in the two crates and the one CLAUDE.md's debt-tagging convention points a reader at first. A marker that carries a number 457 lines out of date is precisely the drift the convention exists to prevent, and the self-contradiction means one of the two sentences was added without reading the other. Delete the figure; keep the pointer to the baseline. (For the record on the seam: test_lines is 2503 of 3912 — 64% — so the first split is extracting mod tests to membership/tests/*.rs, matching what kb/tests/ in the same crate already does, not carving up the derivation logic.)

Evidence

shared/sync/src/membership.rs:20-26 — "@ai-caution: [architecture-debt] At 3,455 lines, well over the 800-line ceiling … The line count is deliberately not repeated here: the baseline holds it and make audit-metrics-check fails if it grows." docs/AUDIT_METRICS.json reports "path": "shared/sync/src/membership.rs", "lines": 3912. The marker both quotes a number and, four lines later, says it deliberately does not.

Verification

Verified verbatim. shared/sync/src/membership.rs:20-26: "@ai-caution: [architecture-debt] At 3,455 lines, well over the 800-line ceiling ... The line count is deliberately not repeated here: the baseline holds it and make audit-metrics-check fails if it grows." wc -l = 3912, matching docs/AUDIT_METRICS.json ("lines": 3912, "test_lines": 2503) and docs/AUDIT_BASELINE.json:140. Same self-contradiction as the shared/kb/src/lib.rs marker, and the same one ROADMAP.md:554-562 explicitly says the convention exists to prevent. The file's size itself is accepted tracked debt (AUDIT_BASELINE accepted, ROADMAP.md:556); the stale number inside the marker is not.

---

Pre-v0.15 codebase audit, horizontal per-crate pass — epic #592.

[cuttlefisch]

Part of the pre-v0.15 codebase audit — tracking epic #592.