Fix audit findings: redaction correctness, daemon crash-loops, delivery guarantees, benchmark scoring

CONTRIBUTING.md

@@ -75,9 +75,9 @@ They exist because each one was earned by a real failure — see
 No venv, no pip install — the loops are stdlib-only Python 3.10+.
 
 ```sh
-python3 -m unittest discover -s tests     # everything (~10s)
+python3 -m unittest discover -s tests     # everything
 python3 -m unittest tests.test_critic     # one slice
-pipx run ruff check .                     # same lint CI runs
+pipx run --spec 'ruff==0.15.22' ruff check .   # the exact lint CI runs (pinned)
 ```
 
 Model calls are stubbed in tests via `CRITIC_CMD` — an executable invoked
@@ -105,7 +105,7 @@ Where things live: `observer/` tails transcripts + git into
 `critic/verify.py` runs repros in a staging dir); `hooks/` delivers
 (`logic.py` is pure — test it without I/O); `reflector/` grades and
 rewrites `heuristics.md`; `core/` is the only shared code. Tests mirror
-this: one `tests/test_<thing>.py` per concern, real session transcript in
+this: one `tests/test_<thing>.py` per concern, a synthetic session transcript in
 `tests/fixtures/session.jsonl`.
 
 ## Architecture map

README.md

@@ -250,7 +250,7 @@ here, and the critic reviews its builders):
 | Catch-to-delivery into the agent's context | **~2 minutes** |
 | Caught in its own redaction code | A real secret-leak bug **two independent reviewers had approved** |
 | Model bake-off | 12 candidates × 7 frozen cases, latency + format discipline measured — [docs/benchmarks/](docs/benchmarks/) |
-| Tests | **647** (`python3 -m unittest discover -s tests`) · CI on 3.10/3.12 + UI build + lint + installer smoke test + bench-selftest |
+| Tests | **750+** (`python3 -m unittest discover -s tests`) · CI on 3.10/3.12 + UI build + lint + installer smoke test + bench-selftest |
 
 The bench-selftest means the A/B harness's safety scorers prove they
 discriminate good from bad — zero API spend — before anyone trusts a live

codecouncil/main.py

@@ -96,7 +96,12 @@ def resolve_settings(args, console_set: frozenset | set = frozenset()
     persisted it to config.json, so the launch-time flag and any exported env
     var must stop outranking it — that knob resolves from the config file only."""
     from core import config as cfg
-    env = dict(os.environ)
+    from critic import agent
+    # local_env(), not os.environ: the critic resolves from ~/.codecouncil/env
+    # too (agent.local_env), so the launcher must consult the same layers or a
+    # model/prober set only in that file resolves differently here than in the
+    # critic it launches.
+    env = agent.local_env()
 
     def one(knob: str, flag: str | None, env_name: str, key: str) -> str | None:
         if knob in console_set:
@@ -141,6 +146,13 @@ def launch(name: str) -> None:
         env = os.environ.copy()
         if m:
             env["COUNCIL_MODEL"] = m
+        if not p:
+            # /prober off must win over an exported COUNCIL_PROBER or one in
+            # ~/.codecouncil/env: set it empty so the critic's
+            # local_env().setdefault can't re-add the file value (resolve_prober
+            # treats "" as off). When p is set it goes on the --prober flag
+            # below, which outranks env anyway.
+            env["COUNCIL_PROBER"] = ""
         extra = {"observer": ["--wait"],
                  "critic": ["--prober", p] if p else [],
                  "reflector": []}[name]
@@ -190,12 +202,14 @@ def settings_info() -> dict:
         _resolve_model), and the console should show that truthfully."""
         from core import config as cfg
         env_file = agent.local_env()   # includes ~/.codecouncil/env keys
-        env = dict(os.environ)
 
         def one(knob, flag, env_name, key):
             if knob in console_set:
                 return cfg.resolve_with_source(None, env_name, key, {})
-            return cfg.resolve_with_source(flag, env_name, key, env)
+            # resolve against local_env (incl. ~/.codecouncil/env), matching the
+            # critic — else a COUNCIL_MODEL in that file is missed here and the
+            # display wrongly falls through to the auto:<KEY> default.
+            return cfg.resolve_with_source(flag, env_name, key, env_file)
 
         m, msrc = one("model", args.model, "COUNCIL_MODEL", "model")
         if m is None:

core/knowledge.py

@@ -23,6 +23,8 @@
 import tempfile
 from pathlib import Path
 
+from core.redact import sanitize
+
 KNOWLEDGE_MAX_FACTS = 30
 MAX_FACT_CHARS = 240
 
@@ -135,7 +137,11 @@ def parse_fact(raw: str) -> str | None:
             or IMPERATIVE_RE.search(text) or NEVER_VALID_RE.search(text)
             or SUPPRESSION_RE.search(text) or SECURITY_EXEMPTION_RE.search(text)):
         return None
-    return text
+    # The fact is model-authored and gets re-injected into every future
+    # judgment prompt (and written to knowledge.md), so it is a redaction sink
+    # like every other stored model text — SECURITY.md states distilled facts
+    # are "redacted and capped again at parse time"; make that true.
+    return sanitize(text)
 
 
 def _normalize(fact: str) -> str:

core/redact.py

@@ -18,13 +18,36 @@
 # like "changeme" or a short test fixture.
 ASSIGNMENT_MIN_VALUE_LEN = 16
 
+# Credential words for the "assignment" pattern. Each must appear as a
+# DELIMITED token in the variable name — bounded by name start/end, `_`/`-`,
+# a digit, or a camelCase case transition — never as a substring of a longer
+# alphabetic word. That is the line that keeps API_KEY / db_password /
+# access_token / apiKey while leaving tokenizer / keywords / monkey / turkey
+# alone (all of which embed a keyword but are ordinary identifiers). Cost of
+# the stricter boundary: a bare all-lowercase run with no delimiter
+# (`apikey`, `mytoken`) is missed — accepted per this module's stated rule
+# that a false redaction of ordinary code is worse than a miss.
+_CRED_WORDS = ("key", "secret", "token", "password", "passwd")
+_CRED_CAMEL = "|".join(w.capitalize() for w in _CRED_WORDS) + "|" + \
+    "|".join(w.upper() for w in _CRED_WORDS)
+_ASSIGN_KEYWORD = (
+    r"(?:"
+    r"(?<![A-Za-z])(?i:" + "|".join(_CRED_WORDS) + r")"   # start-of-name or after _/-/digit
+    r"|(?<=[a-z])(?:" + _CRED_CAMEL + r")"                # camelCase: apiKey, dbSECRET
+    r")(?:s|S)?(?![a-z])"                                 # optional plural, never mid-word
+)
+
 PATTERNS: list[tuple[str, re.Pattern]] = [
     (
         "private-key",
         re.compile(
             r"(?P<prefix>-----BEGIN [A-Z ]*PRIVATE KEY-----\r?\n)"
             r".*?"
-            r"(?P<suffix>\r?\n-----END [A-Z ]*PRIVATE KEY-----)",
+            # In a `git diff`, every line carries a +/-/space marker, so the
+            # END line reads `\n+-----END …`; tolerate one marker char after
+            # the newline or a PEM inside diff text slips through unredacted
+            # (the primary capture path).
+            r"(?P<suffix>\r?\n[+\- ]?-----END [A-Z ]*PRIVATE KEY-----)",
             re.DOTALL,
         ),
     ),
@@ -64,11 +87,12 @@
     (
         "assignment",
         re.compile(
-            # The keyword only needs to appear *somewhere* in the name, so
             # SECRET_KEY / DB_PASSWORD / CLIENT_SECRET / JWT_SECRET /
-            # AUTH_TOKEN — the common prefixed/compound env-var shapes — all
-            # trigger redaction, not just a bare `key`/`secret`/... name.
-            r"(?i)(?P<prefix>\b[A-Za-z0-9_-]*(?:key|secret|token|password|passwd)"
+            # AUTH_TOKEN / apiKey — the common prefixed/compound/camelCase
+            # env-var shapes — all trigger redaction via the delimited-token
+            # match in `_ASSIGN_KEYWORD`; tokenizer / keywords / monkey do
+            # not (the keyword there is a substring of a longer word).
+            r"(?P<prefix>\b[A-Za-z0-9_-]*" + _ASSIGN_KEYWORD +
             r"[A-Za-z0-9_-]*\s*[=:]\s*['\"]?)"
             # The 16+ charset run is what qualifies the value as "high
             # entropy enough" to redact; once qualified, also consume any

core/store.py

@@ -19,7 +19,13 @@ def read_rows(path: Path) -> list[dict]:
         return []
     rows = []
     try:
-        lines = path.read_text(encoding="utf-8").splitlines()
+        # decode with errors="replace" (not read_text, which raises
+        # UnicodeDecodeError): the file is appended mid-write, so the trailing
+        # line can be torn mid-multibyte-character — and this repo's rows are
+        # full of multibyte content (every «REDACTED:…» marker, every
+        # `… [N chars total]`). read_tail_rows already decodes this way; a
+        # torn byte must skip a line, never crash the reflector.
+        lines = path.read_bytes().decode("utf-8", errors="replace").splitlines()
     except OSError:
         return []
     for line in lines:
@@ -78,9 +84,44 @@ def write_json_atomic(path: Path, obj, *, indent: int | None = None) -> None:
     Code settings.json) that wants pretty-printed JSON instead."""
     path.parent.mkdir(parents=True, exist_ok=True)
     fd, tmp = tempfile.mkstemp(dir=path.parent, suffix=".tmp")
-    with os.fdopen(fd, "w", encoding="utf-8") as f:
-        json.dump(obj, f, ensure_ascii=False, indent=indent)
-    os.replace(tmp, path)
+    try:
+        with os.fdopen(fd, "w", encoding="utf-8") as f:
+            json.dump(obj, f, ensure_ascii=False, indent=indent)
+            f.flush()
+            os.fsync(f.fileno())  # durability: survive power loss, not just process crash
+        os.replace(tmp, path)
+        tmp = None  # replaced — nothing to clean up
+    finally:
+        # if json.dump raised (unserializable obj) the tmp file was never
+        # replaced; unlink it so failures don't litter `.codecouncil/` with
+        # orphaned tmp*.tmp files next to the state they failed to write.
+        if tmp is not None:
+            try:
+                os.unlink(tmp)
+            except OSError:
+                pass
+
+
+def write_text_atomic(path: Path, text: str) -> None:
+    """Write `text` to `path` atomically (tmp in same dir + os.replace + fsync),
+    the text sibling of write_json_atomic. For files a crash mid-write could
+    corrupt that another process reads whole — heuristics-history archives
+    (the rollback restore source), knowledge.md, receipts."""
+    path.parent.mkdir(parents=True, exist_ok=True)
+    fd, tmp = tempfile.mkstemp(dir=path.parent, suffix=".tmp")
+    try:
+        with os.fdopen(fd, "w", encoding="utf-8") as f:
+            f.write(text)
+            f.flush()
+            os.fsync(f.fileno())
+        os.replace(tmp, path)
+        tmp = None
+    finally:
+        if tmp is not None:
+            try:
+                os.unlink(tmp)
+            except OSError:
+                pass
 
 
 def wait_for(path: Path, message: str, once: bool, poll_s: float = 2.0) -> bool:

critic/main.py

@@ -60,7 +60,14 @@ def load_state(path: Path) -> dict:
             state = json.loads(path.read_text(encoding="utf-8"))
         except (json.JSONDecodeError, OSError):
             state = None
-        if state is not None:
+        # valid JSON that isn't a dict (a hand edit, a version-drifted file) is
+        # discarded and rebuilt, not fatal: `state["committed_offset"] = …`
+        # below would TypeError on a list/str and crash the daemon on every
+        # restart. Also backfill the required keys so a dict missing beat/offset
+        # can't KeyError inside heartbeat.
+        if isinstance(state, dict):
+            state.setdefault("offset", 0)
+            state.setdefault("beat", 0)
             # committed_offset: how far batches have DURABLY landed (their
             # record appended to suggestions.ndjsonl). Legacy state files
             # predate this field — default it to offset so upgrading never
@@ -277,6 +284,15 @@ def judge_batch(events: list[dict], ctx: dict) -> None:
     }
     save_prompt(suggestions_file.parent / "prompts", record["id"], text)
     primary_parsed = ask_with_retry(text, ctx)
+    if primary_parsed.get("verdict") == "ERROR":
+        # A transport failure (provider outage/gateway error) that survived
+        # ask_with_retry's in-turn retries. Raise so the SCHEDULER requeues the
+        # batch and retries it on a later beat (up to MAX_BATCH_RETRIES) rather
+        # than writing an ERROR record and committing the offset — which
+        # permanently skipped judging code written during even a brief outage,
+        # with no way to replay it. Malformed replies degrade to PASS (not
+        # ERROR) and are unaffected; the prober's own ERROR is handled below.
+        raise agent.AgentError(primary_parsed.get("error", "model turn failed"))
     if ctx.get("prober") and ctx.get("verify", True):
         # Council mode: ask a second, recall-oriented model the SAME prompt.
         # Measured basis (docs/benchmarks/): the primary (precision anchor)
@@ -794,11 +810,18 @@ def heartbeat(obs_file: Path, state: dict, scheduler: TurnScheduler, ctx: dict)
     events = []
     for line in lines:
         try:
-            events.append(json.loads(line))
+            parsed = json.loads(line)
         except json.JSONDecodeError:
             continue
-
-    diffs = [e for e in events if e["type"] == "diff"]
+        # "skip unparseable lines rather than crash" also covers a line that
+        # parses to valid JSON but isn't an event dict (a bare scalar `42`, a
+        # list): `e["type"]` below would TypeError/KeyError and — because the
+        # crash precedes the state save while offset already advanced past the
+        # line — re-read and re-crash on every restart (a permanent loop).
+        if isinstance(parsed, dict):
+            events.append(parsed)
+
+    diffs = [e for e in events if e.get("type") == "diff"]
     if diffs:
         state["latest_diff"] = diffs[-1]
 
@@ -830,17 +853,24 @@ def heartbeat(obs_file: Path, state: dict, scheduler: TurnScheduler, ctx: dict)
            "probed_keys": state.get("probed_keys", [])}
     status = scheduler.submit(events, ctx)
 
-    # the coding agent declared itself done: consider a task-level claim review
+    # the coding agent declared itself done: consider a task-level claim review.
+    # Read the pending "done" requests WITHOUT consuming them — advance
+    # review_offset only once a review actually dispatches. A request that lands
+    # while the worker is busy (common at Stop: the end-of-task edit flurry
+    # usually still has a judgment turn in flight) is then retried next beat
+    # instead of silently swallowed. For a one-shot session Stop is the
+    # receipt's only channel, so a swallowed request loses the receipt entirely.
     review_file = ctx["suggestions_file"].parent / "review-requests.ndjsonl"
     if review_file.exists():
-        req_lines, state["review_offset"] = tail_new_lines(
+        req_lines, new_review_offset = tail_new_lines(
             review_file, state.get("review_offset", 0))
         if should_task_review(state, len(req_lines), time.time(),
                               cooldown=ctx.get("task_review_cooldown", TASK_REVIEW_COOLDOWN_S)):
             since = state.get("last_task_review", time.time() - 3600)
             if scheduler.run_special(
                 lambda: task_review(obs_file, ctx, since_epoch=since)
             ):
+                state["review_offset"] = new_review_offset  # consume only on dispatch
                 state["last_task_review"] = time.time()
                 state["material_since_review"] = False
                 render_status(beat, ts, "task review dispatched — agent claimed done")
@@ -932,14 +962,21 @@ def _on_committed(offset: int) -> None:
     state["interval"] = args.interval
     try:
         while True:
-            heartbeat(obs_file, state, scheduler, ctx)
-            if args.once:
-                # drain first so a clean --once exit persists the
-                # committed_offset the drained batch actually reached,
-                # rather than a stale one that would replay it needlessly.
-                scheduler.drain({**ctx, "beat": state["beat"], "ts": now_iso(),
-                                 "latest_diff": state.get("latest_diff"),
-                                 "offset_now": state["offset"]})
+            try:
+                heartbeat(obs_file, state, scheduler, ctx)
+                if args.once:
+                    # drain first so a clean --once exit persists the
+                    # committed_offset the drained batch actually reached,
+                    # rather than a stale one that would replay it needlessly.
+                    scheduler.drain({**ctx, "beat": state["beat"], "ts": now_iso(),
+                                     "latest_diff": state.get("latest_diff"),
+                                     "offset_now": state["offset"]})
+            except Exception as e:
+                # Daemons never die: an unexpected beat error (disk-full write,
+                # an observations.ndjsonl deletion racing tail_new_lines' stat)
+                # logs and retries next tick. Offset only advances after a
+                # durable append, so a mid-beat failure replays safely.
+                print(f"critic: beat error, retrying — {e}", file=sys.stderr)
             write_json_atomic(
                 state_path,
                 {k: state[k] for k in PERSISTED_STATE_KEYS if k in state},

critic/persona.md

@@ -61,18 +61,25 @@ resolve.
 
 Some messages begin with `TASK: VERIFY`. You are given one finding you
 previously made and a staged copy of the file under review, already placed at
-the path named in the message. Your job: prove it or kill it.
-
-- Write a minimal script in your working directory that exercises the flagged
-  code (import the file, call the function, reproduce the failure) and RUN it.
-- Judge only from what actually happened when the code ran.
-- Then reply with EXACTLY one line, nothing else. The label is about the
-  FINDING, not about the code's claims:
-  - `CONFIRMED: <observed proof>` — the problem is REAL; you reproduced the
-    bad behavior (e.g. "shipping_cost(-5) returned -25, no ValueError").
-  - `FALSE-ALARM: <why>` — the code actually behaves correctly; the finding
-    was wrong.
-  - `INCONCLUSIVE: <why>` — cannot be tested in isolation.
+the path named in the message. Your job: prove it or kill it — with a script
+the harness runs, NOT by running anything yourself.
+
+- Reply with ONLY a single Python script — no explanation, no status line,
+  just code (a `​```python` fenced block or bare source). Do NOT use any tool
+  and do NOT try to run it: the harness executes your script for you, with the
+  staged file present and the script's own directory as the working directory.
+- The script must exercise the flagged code (import the file, call the
+  function) and then print exactly ONE final line AND exit with code 0:
+  - `CONFIRMED: <one-line evidence>` — running it reproduces the claimed
+    problem (e.g. "shipping_cost(-5) returned -25, no ValueError").
+  - `REFUTED: <one-line evidence>` — running it demonstrates the code actually
+    behaves correctly, so the finding was wrong.
+- If the finding can't be tested this way, print NEITHER line and exit 0.
+- If your script hits an UNEXPECTED error (wrong signature, import failure,
+  your own bug), print NEITHER marker — a broken script is not evidence. Wrap
+  only the specific call you are probing; never print REFUTED from a general
+  `except`. The label is about the FINDING, judged from what actually happened
+  when your script ran.
 
 ## TASK: PROBE