Fix audit findings: redaction correctness, daemon crash-loops, delivery guarantees, benchmark scoring - #7
Merged
Merged
Conversation
Audit-verified gaps where the documented "nothing downstream holds a raw secret" / "no false redaction of ordinary code" contracts didn't hold: - PEM private keys survived redact() inside `git diff` text — every diff line carries a +/-/space marker, so the END line reads `\n+-----END …` and the suffix never matched. Tolerate one marker char after the newline. This is the PRIMARY capture path (observer/gitwatch.py diffs + commits). - The assignment pattern matched key/secret/token/password as a SUBSTRING, so `tokenizer = "bert-base-uncased"` / `monkey` / `keywords` redacted to a «REDACTED» marker — which the critic is taught IS a confirmed secret, manufacturing false findings. Require the keyword to be a delimited token (start/_/-/digit or camelCase boundary); keep API_KEY/db_password/apiKey. - PASS `reason` and `malformed` raw reply in parse_reply took the same tool-equipped-model path as issue/rationale but skipped sanitize(). - verify._classify's inconclusive diag note stored raw executed-script output (verified/refuted notes were already sanitized). - Distilled knowledge facts were never redacted, though SECURITY.md:29 claims they are — and they re-inject into every judgment prompt. Tests: PEM-in-diff, camelCase names redact, incidental-substring names do not, fact credential-shape + control-sequence stripping. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Each of these turned a routine runtime event into a permanent crash loop
(the failure recurs on every restart, since the daemon dies before it can
make progress past the poison input):
- core.store.read_rows raised UnicodeDecodeError on a trailing line torn
mid-multibyte-character (rows are full of «REDACTED»/… markers); decode
with errors="replace" like read_tail_rows already does.
- critic heartbeat did `e["type"]` on every parsed line — a valid-JSON
non-dict line (`42`) or a typeless dict crashed it; keep only dict rows
and use .get("type").
- load_state (critic) and State.load (observer) crashed on valid-JSON that
isn't a dict (a hand edit / version drift); isinstance-guard and rebuild.
- observer collect() let a transcript deleted between glob and stat
(Claude Code prunes sessions mid-run) kill the daemon; guard the per-file
read and prune offsets of vanished transcripts (also bounds state growth).
- harvested eval cases were written with a naked write_text into a
machine-shared dir and read by an UNGUARDED load_cases — one torn file
crash-looped the reflector through the rewrite gate. Atomic write +
tolerant load_cases.
- heuristics-history archive was written non-atomically; an empty/torn
archive then IndexError'd maybe_rollback on restored[0]. Atomic write +
empty-archive guard.
- observer/critic/reflector main loops now guard the beat body so a
fallible call logs and retries instead of exiting the process.
Adds write_text_atomic to core.store (fsync + tmp-cleanup, shared by the
archive writer); write_json_atomic gains the same fsync + orphan-tmp cleanup.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ard _describe - Ledger reserved keys (receipts/test_integrity/gate) were pruned by the same 600s TTL as suggestion ids, so their "once ever" facts expired: the newest receipt re-announced every ~10min, a "weakened tests" receipt re-blocked Stop every ~10min even after rebuttal, and the done-gate re-waited up to 120s on later Stops. They are now never age-pruned — bounded by count (RESERVED_KEEP newest) instead. - Raised the suggestion-id retention (LEDGER_TTL_SECONDS 600 -> 3600) above the reflector's UNDELIVERED_AFTER_S=900 grading horizon + poll slack: a delivered mark pruned before the reflector grades made a genuinely-delivered (possibly accepted) finding grade "undelivered" and drop out of the acceptance metric. Delivery *freshness* is unaffected (governed by logic._age_ok on the row ts, not the ledger). - peer_hook.py imported core/critic modules at module scope, BEFORE main()'s fail-open guard. A transient syntax/import error in the critic chain (this repo's own files, edited live by the agent whose tool calls fire the hook) then tracebacked + exited 1 on every event. Wrap the project imports in a fail-open try. main() now catches BaseException so Ctrl-C during the gate wait also exits 0. - _describe hard-indexed s['file']/s['issue']; a row that passes _pending but lacks them (a hand-edited/foreign row) KeyError'd, and fail-open then suppressed every co-pending finding for the row's TTL. Use .get with fallbacks — _pending already tolerates bad data, so this sink must too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…col drift
- persona.md's TASK: VERIFY still taught the pre-script-redesign protocol
("write a script and RUN it, reply with EXACTLY one line CONFIRMED/
FALSE-ALARM/INCONCLUSIVE"). verify.py parses only a script that PRINTS
CONFIRMED/REFUTED and exits 0; a model obeying the persona emitted a bare
status line that executed as Python -> SyntaxError -> inconclusive, silently
withholding true findings. Rewrote the section to the actual contract.
- Whole test-file deletion was invisible to test-weakening screening:
removed_lines_by_file only rebound path on `+++ b/`, so a deleted file's
`+++ /dev/null` left its removed lines attributed to the previous file (or
dropped). Track `--- a/` and attribute `/dev/null` deletions to it; reset
path on non-b/ headers in both diff parsers.
- `async def test_` never counted as a test (removed async tests escaped the
weakened verdict): `_TEST_DEF_RE` now allows an optional `async `.
- SafeLoader exemption scanned only to the first `)`, so
`yaml.load(f.read(), Loader=yaml.SafeLoader)` false-flagged CWE-502; scan
the whole line.
- `_STR_BUILD_RE`'s `f["\']` matched a literal `f` before a closing quote
inside a parameterized query (`'off'`), false-flagging SQL injection; anchor
the f-string prefix.
- _execute_probe ignored the probe's exit code and let DIVERGES beat
CONSISTENT — a probe that printed DIVERGES then crashed (or printed both)
minted a finding. Require returncode 0 and treat both-markers as error,
mirroring verify._classify (this is the finding-CREATING path).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rop receipts
- parse_reply passed untrusted model-JSON fields straight through: a non-string
`issue` raised TypeError in sanitize() (only JSONDecodeError was caught, so it
escaped and burned the batch's requeue cycles); an unrecognized `severity`
("critical", "High") was stored verbatim and then NEVER delivered, since hooks
gate on exact {medium,high} membership — the model's most urgent findings,
silently dropped. Now: require file/issue to be non-empty strings (else
malformed-PASS), normalize severity into {low,medium,high} (critical->high),
and drop a non-int line to None.
- A model transport failure (provider outage) that survived ask_with_retry
wrote an ERROR record and committed the offset, permanently skipping code
written during even a brief outage with no way to replay it. judge_batch now
raises on a primary ERROR verdict so the scheduler REQUEUES the batch and
retries on a later beat (bounded by MAX_BATCH_RETRIES). Malformed replies
(verdict PASS) and prober errors are unaffected.
- The "done" review request advanced review_offset unconditionally, so a
request landing while the worker was busy (the common case at Stop) was
consumed with no review dispatched — losing the session receipt entirely for
a one-shot session. Advance the offset only once run_special actually
dispatches; otherwise retry next beat.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… safety Measurement + robustness fixes to the A/B/training harness: - report()'s mean hidden-test pass rate DROPPED crashed trials (total==0) instead of scoring them 0 — so whichever arm crashed more reported an inflated rate, and the dependency-hallucination failure the 'closest-match' task engineers on purpose couldn't lower the mean. Crashed trials now score 0 and the per-arm summary reports the crash count. - run_session / training.run_task now catch TimeoutExpired/OSError: a hung session is a failed, retryable attempt — one hang no longer aborts a multi-hour paid run. Each trial in main() is wrapped so a setup/trial failure records an error row (crashed hidden / UNSAFE) and continues. - Setup git commands (clone/fetch/checkout/init/add/commit) were unchecked: a failed clone still spent a full session on a garbage workspace, and the --repo-url sha pin was unverified. Added _sh_checked + a `git rev-parse HEAD == sha` assertion; the seed commit pins identity + disables signing so it succeeds deterministically (previously it silently failed on CI with no git identity). - training.run_task inherited the maintainer's global ~/.claude settings — the exact contamination the A/B harness guards against — since it omitted `--setting-sources project,local`. Added it (training data drives harvested cases + rewrites). - rescore silently skipped the safety tier while claiming "the current scorer"; it now re-runs each safety task's adversarial exploit and reports how many rows were carried over unrescored. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- The activity `seq` was a window-relative index (observations.length - 120 + k), so it shifted for the same event as the append-only log grew past the 1MB tail window — and ActivityFeed keys React rows off it, so a row's local expanded state silently transferred to a DIFFERENT event on the next poll (the live log here is 25MB, so the "stable identity" comment was already false). seq is now the line's absolute byte offset, attached by readNdjsonTail — it never changes for a given event. - ActivityFeed's tail-follow effect depended on activity.length, which pins at 120 once a session gets going, so auto-scroll died exactly then. Depend on the newest event's stable seq instead. - readNdjsonTail ignored readSync's byte count over an allocUnsafe buffer: if the file shrank between stat and read, uninitialized memory was decoded and parsed. Honor the returned length. - Removed lucide-react — a dependency no source file imports (every icon is inline SVG). Lockfile updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…bles; docs - resolve_settings and settings_info resolved the model/prober env-var layer from os.environ, but the critic resolves from agent.local_env (which also reads ~/.codecouncil/env). So a COUNCIL_MODEL/COUNCIL_PROBER set only in that file made the launcher display "council off"/the wrong model+source while the critic ran with it. Both now resolve from local_env, matching the critic. - /prober off deleted the config key and passed no --prober flag, but the subprocess env still carried an exported/env-file COUNCIL_PROBER, which local_env().setdefault re-added -> council mode silently re-enabled after the console said "off". launch() now sets COUNCIL_PROBER="" whenever the resolved prober is off, so the file value can't win. - CONTRIBUTING: the lint command was unpinned `ruff check .` labeled "same lint CI runs" — CI pins ruff==0.15.22 precisely because a newer ruff flags the intentional fail-open patterns. Pinned it; corrected "real"->"synthetic" session fixture. README test count 647 -> 750+. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…, fresh workspaces
Three benchmark-scoring correctness fixes (the previously-deferred items):
- Hidden tests scored `passed / printed_checks`, so a script that raised
partway printed fewer CHECK lines and scored a smaller denominator —
crashing on adversarial input beat answering it wrong (records-lookup:
1/1=100% instead of 1/2). run_hidden_test now derives the CHECK names the
source INTENDS to print (declared_checks, anchored to printed string
literals, dynamic {names} excluded) and scores against declared ∪ printed,
so a declared-but-unprinted check counts as FAIL.
- Safety tier: doc-reader and user-lookup wrapped the exploit call in
try/except, so a no-op session (function never implemented) raised
AttributeError → caught → printed SAFE and banked an unearned SAFE, while
the other three tasks scored absence UNSAFE. Every adversarial script now
runs a happy-path guard first (function exists + does its job on legit
input) → UNSAFE on absence/breakage, uniformly. --selftest still shows all
five discriminating good vs bad.
- Reusing --out reused trial dirs still holding a prior session's committed
work (the next agent started with the task pre-solved) and appended
duplicate rows to results.ndjsonl. seed_repo/clone_repo now recreate the
trial dir fresh, and main() refuses a --out whose results.ndjsonl is
non-empty (pointing at rescore instead).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…undary Two load-bearing invariants the audit flagged as entirely untested: - "A finding the critic's own repro REFUTED is never delivered" — the product thesis — had zero tests (the string "refuted" didn't appear in test_hooks.py). Added coverage on both the context (PostToolUse) and block (Stop) channels, plus a sanity check that a verified finding still delivers. - verify.py's note/repro redaction boundary had no secret-shaped test; a regression removing the sanitize() would have been invisible. Added a CONFIRMED-with-credential test (note + repro) and an inconclusive-diag test. 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.
A full-repo quality audit surfaced correctness gaps against the project's own documented invariants. This fixes them in 10 focused, individually-revertable commits, each with regression tests.
Suite: 717 → 764 tests, all green (verified on 3.11/3.12/3.13; changed files parse clean under the 3.10 grammar). ruff (pinned 0.15.22),
tsc --noEmit, Vite build, and the A/B safety selftest all pass.Every finding below was verified against the real code before fixing — several by executing it.
Redaction correctness (
fix(redact))\n-----END, but everygit diffline carries a+/-/space marker, so\n+-----ENDnever matched. This is the primary capture path.key/token/… as a substring, sotokenizer = "bert-base-uncased"became a«REDACTED»marker, which the critic is taught IS a confirmed secret. Now requires a delimited token (keepsAPI_KEY,db_password,apiKey).reason,malformed, the verify inconclusive note, and distilled knowledge facts all skipped sanitization (SECURITY.md claimed otherwise for facts).Daemon crash-loops (
fix(daemons))Each of these turned a routine event into a permanent crash loop (recurs on every restart):
read_rowsraised on a trailing line torn mid-multibyte-character.Delivery guarantees (
fix(hooks))undeliveredand fell out of the acceptance metric.peer_hook.pyimported the critic chain before its fail-open guard existed — a transient syntax error in files the agent is actively editing tracebacked on every hook event.Review quality (
fix(screen/probe/persona),fix(critic))persona.mdstill taught the pre-redesign verify protocol, contradictingverify.py— a model obeying it got its status line executed as Python, landing "inconclusive" and silently withholding true findings.+++ /dev/null);async def test_never counted as a test.fbefore a quote inside a parameterized query reading as an f-string."critical"severity was stored verbatim and then never delivered (hooks gate on exact{medium,high}); a non-stringissueescaped as a TypeError and burned the batch's retries.Benchmark integrity (
fix(evals)×2)--outleft prior work in trial dirs (next agent started pre-solved) and appended duplicate rows.--repo-urlsha pin is now verified; training sessions get the same contamination isolation as the A/B arms;rescoreno longer skips the safety tier.Dashboard + launcher (
fix(ui),fix(launcher))seqwas window-relative, so it shifted for the same event as the log grew — and React keys off it, silently transferring a row's expanded state to a different event. Now the line's absolute byte offset./prober offdidn't disable council mode whenCOUNCIL_PROBERwas exported or in~/.codecouncil/env; the launcher resolved env differently than the critic it launches.Tests (
test:)Added coverage for the two most load-bearing untested invariants: "refuted findings are never delivered" (the product thesis — the string
refuteddid not appear intest_hooks.py) and verify.py's redaction boundary.Not included, deliberately: the shared-helper dedup sweep (epoch/cap/tolerant-read). Those are trivial 4-line stdlib wrappers where consolidation adds cross-module coupling for little gain — better as its own reviewed PR than mixed into correctness fixes.
🤖 Generated with Claude Code