Close audit issues #20–#27: agent engine, worker & email, profiles, external clients, web, data layer, docs, deploy - #38
Open
ahueb wants to merge 444 commits into
Open
Close audit issues #20–#27: agent engine, worker & email, profiles, external clients, web, data layer, docs, deploy#38ahueb wants to merge 444 commits into
ahueb wants to merge 444 commits into
Conversation
…ter's new default (#23, #24) Cross-round sweep: 35c59a9 (#24 audit Minor 2) stopped parse_retry_after from returning 0.0 for a past-dated HTTP-date or a negative header — a zero backoff is a hot retry against an API that has just throttled us — so it now returns the caller's default (5.0 at the slack_client call site). These two tests asserted the old 0.0. Their actual subject is unchanged and still pinned: nothing raises out of the `except SlackApiError` block and time.sleep gets a sane float. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…I5) Neither was gitignored: .mypy_cache (58 MB / 34 files, created by the new mypy stage) is baked into every image via the Dockerfile's `COPY . .`, measured at 88% of the built image's /app tree, and uv.lock is a stray second lockfile that would be committable to a public repo alongside requirements.lock. Add both to .gitignore, add .mypy_cache to .dockerignore, and extend test_dockerignore.py's MUST_EXCLUDE list. Verified: `git check-ignore -v .mypy_cache uv.lock` now matches both (previously matched neither). `.venv-test/bin/python -m pytest tests/unit/test_dockerignore.py -q -p no:cacheprovider` — 2 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…ting a reply every turn (#20 I1) `posted is False` only logged in Phase 4 and Phase 5's three post sites, so a deterministic Slack refusal (is_archived, not_in_channel, invalid_auth, recurring msg_too_long) cost one LLM call per turn per affected thread forever, and Phase 4's thread could never reach the 12-message timeout close (has_pending_reply stayed True, message_count never advanced). Added `ThreadState.post_failure_count: int = 0` (src/agent/state.py) and mirrored the existing authorship_reject_count two-strike pattern in Phase 4's `if not posted:` branch: increment, and on the second consecutive failure clear has_pending_reply; reset to 0 on a successful post alongside the other per-thread counters. Phase 5's two reply branches (private-channel flat reply, and the general/threaded reply) attempt to post to target_post_id BEFORE any ThreadState exists for it -- one is only ever created on a SUCCESSFUL threaded reply, and never for a private-channel reply -- so there is no persistent object to hang post_failure_count on until a post lands. Added an engine-level `self._phase5_post_failure_counts: dict[str, int]` keyed by target_post_id instead, wired through a new `_note_phase5_post_failure` helper: same two-strike shape, dropping target_post_id from interesting_posts (rather than clearing has_pending_reply, since there is no ThreadState) after the 2nd consecutive failure, and popped on success or once the drop fires. Phase 5's third site (a brand-new top-level post, no target_post_id) is left as log-only -- each turn's decision to write a new post is a fresh LLM choice with no persistent object across turns to back off, and the brief's own "drop target_post_id from interesting_posts" instruction is specific to the two branches that have one. state.py check requested by the coordinator: ThreadState is never reconstructed field-by-field from persisted data anywhere in simulation.py's rebuild paths -- every real construction site is a keyword call that omits post_failure_count, so it defaults to 0 cleanly (a restart gives a failing thread a fresh two strikes, which is correct: the failure condition itself, e.g. a revoked token, would keep recurring anyway). No test constructs ThreadState positionally -- grepped every call site in src/ and tests/; all are keyword args. RED: tests/unit/test_simulation_logic.py::TestPhase4PostFailureBackoff -- both tests failed (post_failure_count stayed 0 / had no such attribute path exercised as expected). tests/unit/test_simulation_logic.py::TestPhase5PostFailureBackoff -- all three failed (AttributeError: no _phase5_post_failure_counts; target not dropped after 2 failures). GREEN: tests/unit/test_simulation_logic.py tests/unit/test_roster_sync.py tests/unit/test_thread_not_found.py tests/unit/test_service_bot_uid_probe.py tests/unit/test_funding_reply_backoff.py tests/unit/test_authorship_emit_gate.py -- 201 passed; tests/integration/test_state_rebuild.py -- 9 passed. ruff check src still 251 findings (no new ones); tests/ and state.py clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
MYPY_MAX=143 was the exact count with zero slack, on `mypy>=1.10.0` unpinned (resolves to 2.3.1 here). Any mypy release that adds a check would turn every push red for reasons unrelated to the change. Cap the dev extra at `mypy>=2.3,<2.4` — dev extras are not part of requirements.lock (verified: 1316 lines, no mypy pin; re-ran the lock-freshness check and it still agrees with the committed lock, no drift from this change) — and reword the ci.sh comment to say what the number is and how to move it deliberately (re-measure, its own commit, both numbers in the message) instead of an unconditional "never raise it". Measured via `git archive HEAD src` at commit 2170efb (the tip before this round started) + the exact ci.sh command, mypy 2.3.1: 145 findings (up from the audit's 143 due to concurrent fix rounds; 2 of the 145, in http_retry.py, are being fixed by a separate round already in flight). Set MYPY_MAX=150 — 145 measured plus 5 of slack, the same shape as SRC_LINT_MAX's 6-of-260. Both numbers (143 audit-time, 145 today) are recorded in the ci.sh comment; this ceiling is provisional pending the other rounds landing, per the controller's note. `.venv-test/bin/python -m pytest tests/unit/test_ci_gate.py tests/unit/test_dependencies_lock.py -q -p no:cacheprovider` — 12 passed. `bash -n scripts/ci.sh` clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…he new preflight check shifted (#22, #27) Round D registered a publication-duplicate check ahead of the snapshot step, renumbering the tail, so R.6's reference to "preflight check 13" would have pointed at the wrong check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…fter a DB outage (#21) Reading the #21 Critical fix (415d44f) end to end: the terminal handler is now safe against a poisoned session, but a genuine Postgres outage still fails the poller's own commit, so the S3 object is retried and each retry's migration creates a new timestamped priv-* channel rather than adopting the existing one. Bounded at three attempts per worker restart. The runbook now tells the operator how to find and archive the orphans; channel adoption is a recorded follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…27 I8) Deleting either ci.sh's stale-lock exit 1 or its mypy-ceiling exit 1 left all existing tests green — the script the project calls the whole gate had no test that would notice it stopped gating. Add one behavioural test per ratchet, invoking scripts/ci.sh in a real subprocess (same shape test_run_migration_sh.py uses for bash): - mypy: MYPY_MAX=0 must fail before pytest starts ("rose to" + nonzero exit, and "-m pytest" never printed); MYPY_MAX=99999 is the mirror-image positive control — the subprocess is killed the instant the "==> pytest" banner appears, so the control never pays pytest's ~7-minute cost. - lockfile: a symlink farm mirrors every top-level repo entry except pyproject.toml, which is a real copy with one dependency's cap lowered (anthropic <1.0.0 -> <0.100.0) so pip-compile resolves a genuinely different pin than the (symlinked, untouched) committed requirements.lock — never touches the real pyproject.toml. Skips if uv is not on PATH, the same valve ci.sh itself uses. Mutation evidence (reverted immediately after, `git diff scripts/ci.sh` clean both times): with `exit 1` replaced by `true` at ci.sh:427 (mypy) and ci.sh:399 (lockfile stale), a direct `timeout 30/60 ./scripts/ci.sh` run prints the findings/ERROR line as before but then falls through into the full pytest run instead of stopping — proving both new tests would fail (timeout or wrong exit code) against that mutant, and pass against the real script. `.venv-test/bin/python -m pytest tests/unit/test_ci_gate.py tests/unit/test_dependencies_lock.py -q -p no:cacheprovider` — 15 passed in ~25s. `ruff check` on both files: clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…a-run writes where UID 10001 can (#27 Critical 1, Minor 12/18) --via-run writes the preflight snapshot INSIDE a one-off container as UID 10001 (the image's runtime user; the prod compose sets no `user:` override on one-off `run`s), but the snapshot's default directory was $BACKUP_DIR (backups/), which is operator-owned on the prod host — R.6's very first command would BLOCK on EACCES with the app already stopped. Default the snapshot to <repo>/data/ under --via-run instead (already chowned to 10001:10001 by Part R.5 for the profiles/data bind mounts); an explicit MIGRATE_SNAPSHOT still wins in either mode, and the non---via-run default (exec into an already-running container, no UID restriction) is unchanged. Wiring the already-exported MIGRATE_STATE_DIR through preflight/postflight would have been a cleaner fix but those files belong to another round — left the env var itself untouched, just corrected the default path. Also Minor 18: Step 3's `pg_dump -U copi` is now `pg_dump -U "${POSTGRES_USER:-copi}"`, matching the rest of the compose stack. RED: the two new behavioural tests failed against the pre-fix script (snapshot defaulted under backups/, pg_dump ignored POSTGRES_USER). GREEN after the fix: `.venv-test/bin/python -m pytest tests/unit/test_run_migration_sh.py -q -p no:cacheprovider` — 5 passed (includes an existing test updated with an explicit MIGRATE_SNAPSHOT so it keeps exercising the "backup dir == snapshot dir" guard now that the --via-run default no longer follows --backup-dir). `bash -n scripts/migrate/run_migration.sh` clean; `ruff check tests/unit/test_run_migration_sh.py scripts/migrate` clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…ols TODO and the redundant lock filter (#27 Minor 13/14/16) Minor 16: req_general/req_graph/conn_perip were single zones shared across all three vhosts (${DOMAIN}, devel, blackbird), so one IP's burst against devel or blackbird consumed the primary site's whole rate-limit budget and vice versa. Give each vhost its own zone (req_general_main/_devel/_blackbird, conn_perip_main/_devel/_blackbird, and req_graph_main/_blackbird — devel has no public collaboration-graph routes so it gets none); 8 zones x 10m is 80m of shared memory, trivial for a single host. Verified with `nginx -t` against the envsubst-rendered config in a throwaway nginx:latest container with self-signed certs standing in for the real letsencrypt ones — "configuration file test is successful". Minor 13: the Dockerfile's "Follow-up: pin hashed setuptools/wheel ... (Task 27.5's owner)" comment pointed at a task that is already complete. Reworded to state the accepted trade-off (this local `pip install .` is not hash-verified, but it installs only this repo's own source, not a third-party network artifact) instead of a stale forward-reference. Minor 14: `grep -v '^#'` in ci.sh's lock diff is a no-op on today's input (--no-header means requirements.lock has zero '^#' lines, verified) but kept anyway as defensive belt-and-braces against a future pip-tools reintroducing header comments — documented the trade-off (it would also silently mask a newly-appearing "packages considered unsafe" comment block, though never an actual pin) rather than silently keeping or dropping it. `.venv-test/bin/python -m pytest tests/unit/test_ci_gate.py tests/unit/test_dependencies_lock.py tests/unit/test_dockerignore.py tests/unit/test_run_migration_sh.py tests/unit/test_nginx_config.py tests/unit/test_dockerfile_build.py tests/unit/test_deploy_compose.py -q -p no:cacheprovider` — 52 passed (15 in test_nginx_config.py, up from 13: two new zone-isolation tests, RED confirmed against the pre-fix shared zones before the nginx.conf edit). `bash -n scripts/ci.sh` clean; `ruff check tests/unit/test_nginx_config.py` clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…annot orphan a private Slack channel (#24 V5 / #20 COR-13) reopen_proposal's write block (db.add(review) through commit()) had no except IntegrityError -- the exact guard Task 24.2 added to review_proposal three hundred lines above, left off its sibling. migrate_public_thread_to_private only flushes refined_in_channel and the AgentChannel rows (it never commits), so a lost race against a concurrent insert for the same (thread_decision_id, agent_id) -- the engine's implicit rating=-1 marker, an e-mail reply, a delegate's /review -- 500s the request, get_db rolls back, and the private Slack channel already created for real survives with refined_in_channel gone: an orphan channel invisible to the app. Worse, under the D6 upsert rule a PI retry then passes the :695 != -1 guard and migrates AGAIN, minting a second priv-...-N channel. Wrap the write block in try/except IntegrityError mirroring review_proposal's guard exactly: on rollback, re-load the ThreadDecision and re-bind refined_in_channel from the value captured before the failure (no re-migration), then re-select the winning ProposalReview row and apply the same D6 three-way branch (None / rating==-1 upgrade / real review left alone) before the recovery commit. Re-migration analysis (requested by the brief): once this fix's own commit lands, refined_in_channel is durable and the winning review's rating != -1, so the :695 guard blocks a retry -- no second migration in the real (separate-transaction) case. The one residual gap, proven by the "recovers_refined_in_channel" integration test's own retry step: this repo's db_session test fixture runs the whole request in one never-truly-committed savepoint, so a competing insert staged in the SAME session cannot outlive this handler's own rollback() -- both rows (and the migration's AgentChannel row) are lost together, and a retry in that specific degenerate case DOES re-migrate. That is a harness limitation, not a change in the shipped code's behavior; the two new fake-session unit tests in test_concurrent_write_guards.py prove the upgrade-in-place recovery is correct once a winning row survives, which is the real production shape of the race. Test evidence: - RED (pre-fix code, git HEAD, called directly against a real Postgres fixture): reopen_proposal raised an uncaught sqlalchemy.exc.IntegrityError (UniqueViolationError on uq_proposal_reviews_decision_agent) instead of recovering. - GREEN: tests/integration/test_proposal_review.py::test_reopen_write_race_does_not_500_and_recovers_refined_in_channel passes (302, refined_in_channel preserved); full tests/integration/test_proposal_review.py tests/integration/test_agent_page.py -- 129 passed; tests/unit/test_concurrent_write_guards.py -- 5 passed (includes the two new reopen_proposal fake-session tests, RED-confirmed against the pre-fix module too). - ruff check src --output-format=concise --quiet | grep -c . -> 251 (unchanged). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
… imports (#27 I6) The freshness gate (step 5) proves requirements.lock MATCHES pyproject.toml; nothing proves it actually WORKS. .venv-test is Python 3.12 resolved fresh from pyproject.toml, while the Dockerfile installs requirements.lock on Python 3.11 — measured drift: anthropic 0.117.0 vs 0.125.0, fastapi 0.139.2 vs 0.141.1, alembic 1.18.5 vs 1.19.1, sqlalchemy 2.0.51 vs 2.0.52, slack-sdk 3.43.0 vs 3.44.1, uvicorn 0.51.0 vs 0.52.4, boto3 1.43.51 vs 1.43.88. With no server-side CI (D17) the prod image is the first thing that ever executes those exact pins. Add a new opt-in step (LOCK_SMOKE=1, off by default) between the lockfile freshness check and mypy: installs requirements.lock with --require-hashes into a throwaway Python 3.11 venv and imports src.main, src.worker.main and src.agent.main, then discards the venv. Reuses the freshness check's interpreter-discovery order via a new find_lock_python() helper rather than duplicating it. Off by default because a cold uv cache means downloading and hash-verifying ~80 wheels — real network time this gate should not add to every push; a one-line note explains the skip so the gap stays visible. Ran it for real (LOCK_SMOKE=1, isolated venv, warm uv cache from this session's earlier steps): 5.9s wall (`uv venv` + `uv pip install --require-hashes -r requirements.lock`), exit 0, all three modules (src.main, src.worker.main, src.agent.main) imported cleanly with no .env present. A cold cache would take materially longer (network download + hash verification of every pin) — noted in the comment as a caveat, not measured here. `.venv-test/bin/python -m pytest tests/unit/test_ci_gate.py tests/unit/test_dependencies_lock.py tests/unit/test_dockerignore.py tests/unit/test_run_migration_sh.py tests/unit/test_nginx_config.py tests/unit/test_dockerfile_build.py tests/unit/test_deploy_compose.py -q -p no:cacheprovider` — 54 passed (2 new: the step is documented/opt-in, and its default path prints a visible skip). `bash -n scripts/ci.sh` clean; `ruff check tests/unit/test_dependencies_lock.py tests/unit/test_ci_gate.py` clean; `ruff check src` unchanged at 251. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…rades it instead of returning "Already reviewed" (#24 V5-2 / D6) review_proposal's except IntegrityError arm unconditionally returned 400 "Already reviewed". The reachable race is web-vs-engine: _persist_implicit_proposal_review (a separate process, its own session) can insert the engine's implicit rating=-1 marker between this request's guard SELECT (which already treats -1 as not-yet-reviewed, per the D6 ruling) and this request's own flush. The PI was then told someone else reviewed when nobody had, and their rating/comment were silently discarded -- contradicting D6 inside the one function 24.2 hardened for exactly this constraint. Fix mirrors the vote endpoint (src/routers/public.py:1083-1099): after rollback(), re-select the winning row; if it is still rating == -1, apply the same six field assignments plus reviewed_at the happy-path insert/update would have set, retire the notification (this request DID file a real review, just via the recovery path), commit, and take the success redirect. Only when the winner is a real decision (rating != -1) does the existing 400 path still apply -- pinned by the updated control test. Test evidence: - RED (pre-fix code, git HEAD, loaded via importlib and called directly against a fake session mirroring the winning insert): raised HTTPException(400, "Already reviewed") instead of upgrading the -1 marker and returning 302. - GREEN: tests/unit/test_concurrent_write_guards.py -- 5 passed (test_review_proposal_upgrades_the_engines_implicit_marker_after_a_lost_race new; test_review_proposal_survives_a_lost_race_via_autoflush extended with a real-review 5th SELECT result as the control, still 400); full tests/integration/test_proposal_review.py tests/integration/test_agent_page.py -- 129 passed. - ruff check src --output-format=concise --quiet | grep -c . -> 251 (unchanged). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
The Dockerfile's `COPY . .` would otherwise bake a stray uv.lock (any `uv` invocation without --no-project writes one) into the image as a second, competing lockfile. Round H added it to .gitignore only, per its brief's literal wording. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…the ratio (#21 V2 COR-18c) The final gate failed here: `elapsed2 > 1.5 * elapsed` measured 0.639s vs 0.451s (ratio 1.42) with a perfectly correct ladder. Each measurement is `overhead + sleep`, and this harness's real DB work is ~0.25s — higher when the whole suite is hammering the same Postgres — so a constant overhead inflates the smaller measurement proportionally more and the ratio sags toward 1 exactly when the box is busy. The difference cancels the overhead algebraically: elapsed2 - elapsed == base for the real ladder and ~= 0 for a constant-backoff mutant. Base raised 0.2s -> 0.5s so jitter has to exceed 0.25s to produce a false failure. Mutation-verified: replacing `2 ** max(0, job.attempts - 1)` with `2 ** max(0, 0)` in src/worker/main.py fails this test (restored immediately; `git diff -- src/` clean). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…rocess env (#27 I6) It asserts the step's DEFAULT (skipped) behaviour but passed `{**os.environ, ...}`, so a gate run started as `LOCK_SMOKE=1 ./scripts/ci.sh` made the step run for real inside the subprocess and the assertion failed — observed on the final gate run, where the step itself reported "PASS requirements.lock installs on Python 3.11 and src.main/src.worker.main/src.agent.main import cleanly". Verified green now both with and without LOCK_SMOKE=1 in the ambient environment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…ls, verification, rollout Written to the path Part R's `gh pr create --body-file` line already points at. Carries: what each part closes; every "deliberately not planned" row with its reason; the 60 residual risks and known gaps the Phase 7 audits produced, re-verified against the code after the fix wave (3 dropped as fully fixed, 10 narrowed); the green gate figures at c049ec7; the rollout summary including the four runbook defects the audit found by building the image and running the real tooling; and the process notes (do not squash, the three mixed-attribution commits, the ledger's location). The PR itself is NOT opened — that waits on the branch owner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
-#27) Five late rulings: how the audit findings were routed into eight file-disjoint rounds and which three were deliberately recorded as residuals instead of fixed; why the third Phase-5 post-failure site stays log-only; why the sparsedata bot-name helper is a pinned duplicate rather than a cross-script import; that the mypy ceiling is now a measured number with slack on a capped tool; and why the lockfile install smoke test is opt-in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…s intentional deletes or an operator's extra table (#22, #25) Found by running the real 0024->0028 chain against a copy of the live production database (verified nightly dump 20260904T080003Z, sha256 confirmed, restored and neutralized locally). Both defects fire on the real deploy, inside the migration window, and both tell the operator to restore from backup — while the migration was in fact correct. Neither is reachable from the synthetic fixtures the gate uses, because those have no duplicate publications and no operator artefacts. 1. Row counts (BLOCK). `compare_row_counts` was written when nothing in the chain deleted rows ("the migration itself inserts no rows"), so it treats all shrinkage as loss. 0025 deliberately deletes duplicate (user_id, pmid) publications - 223 of them on production - and postflight reported "publications: 4,731 rows before, 4,508 after - 223 rows LOST ... Compare against the backup before doing anything else", exit 1. run_migration.sh turns that into BLOCKED and the runbook sends the operator to the rollback section, where rollback past 0019 is impossible (2,210 agent_messages rows with agent_id IS NULL), i.e. restore-from-backup after a good migration. Fix: preflight measures the surplus it already reports in check 12 and records it in the snapshot as expected_deletions; postflight accepts a shrinkage that matches EXACTLY, and still fails when more or fewer rows went than predicted. A snapshot written before this key existed keeps the strict behaviour. 2. ORM drift (BLOCK). `remove_table` sat in DRIFT_FAIL_OPS, so any table present in the database but not declared by a model was "drift the models cannot tolerate". Production has one: email_notifications_expired_bak_20260814, 40 rows, made during the 2026-08-14 deploy. An extra table cannot affect the ORM. Fix: a DRIFT_WARN_OPS bucket - named in the output with an explanation, exit 0 (postflight already runs warn_exit_code=0). Verified end to end on the production copy: preflight exit 2 (4 WARN, expected_deletions {'publications': 223}) -> alembic 0024->0028 in 1.50s, 0025 reporting 223 deletes / 0 merges -> postflight exit 0, "publications is 223 row(s) smaller, exactly the duplicates 0025 was measured to delete", 13 checks 0 FAIL 1 WARN. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…ny (#22, #25) An adversarial audit of my own previous fix (dd7e06f) found it unsound, and demonstrated it on a copy of production. A count cannot detect a concurrent deleter working INSIDE a duplicate group: every row such a process removes reduces 0025's own delete count by exactly one, so the net shrinkage always lands on the prediction. Proven end to end — 5 duplicate-group KEEPERS deleted mid-window, 0025 then deleted 218, total 223, and postflight reported "exactly the duplicates 0025 was measured to delete", exit 0, with five rows of real data destroyed. The keepers are the rows 0025 merges each group's data into, so that is the worst row to lose, and dup_publications.csv listed them as survivors — the operator's only paper trail was actively wrong. Now preflight NAMES the rows (`publication_duplicate_plan`, mirroring 0025's own `created_at ASC, id ASC` keeper choice) and records `expected_deleted_ids` / `expected_kept_ids` in the snapshot; postflight asks the database which of them are actually there. Re-ran the attack: it now BLOCKs with "5 of the 223 rows 0025 was supposed to delete are still present" AND "5 of the 223 rows 0025 was supposed to KEEP are gone — real data loss, not a dedup". The happy path still passes with "those are exactly the 223 rows preflight named". Two more findings from the same audit, both verified fixed against the copy: - The expectation was recorded even when 0025 was not in the pending set, so `--target 0024` on a 0024 database licensed 223 arbitrary deletions. Now gated on `pending_revisions()`. - The snapshot's own identity was never checked, so a snapshot from a DIFFERENT database verified happily against whatever was in front of it. postflight now refuses a snapshot whose kind, target or database differ from the run being verified. The identity checks run outside the count branch, so `--allow-row-growth` no longer skips them (verified: 5 keepers deleted plus 300 rows inserted, net growth, still BLOCKs). My first attempt at the identity query used `ANY(:ids::uuid[])`, which is a syntax error once bound; the guarded check turned that into a FAIL, i.e. it would have blocked a good deploy. Only running the attack surfaced it. Now `ANY(CAST(:ids AS uuid[]))`, chunked at 1000 ids. Regression tests: five integration tests in test_migration_tooling_chain.py (preflight names the rows; postflight passes on an exact match; the keeper-deleted attack fails, with and without --allow-row-growth; a foreign snapshot is refused; no licence when 0025 is not pending) and five unit tests for the binding predicate and the pending-set gate. 10 passed / 174 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
… health probe really is bounded (#22 COR-23, #27 I2, #29) Both found by driving the real HTTP routes against a copy of production, and both are defects in features this branch introduced. Neither was reachable from the test suite as written. 1. `POST /agent/{id}/profile/save` took `content: str = Form(...)`. An emptied textarea submits `content=`, which Starlette's form parser hands to FastAPI as a MISSING field, so the route answered a raw `422 {"type":"missing","loc":["body","content"]}` and cleared nothing — reproduced against the copy: DB text and disk file both survived. So the headline clear path of #22 COR-23 / #29 (a080900's file unlink, a20ac4a's seed clearing) only ran if the PI happened to leave whitespace in the box; select-all-and-delete, the normal gesture, just errored. The onboarding twin has always used `Form("")`. Now so does this one, verified end to end: 302, both columns NULL, file gone. tests/integration/test_private_profile_clear.py had DOCUMENTED the 422 in a docstring and worked around it with whitespace, calling it "unrelated to the fix under test" — that docstring is removed and replaced by a test that posts a truly empty value. 2. `HEALTH_PROBE_TIMEOUT_SECONDS` did not bound the failure its own comment names. `asyncio.wait_for` cannot interrupt a socket read a SQLAlchemy greenlet is already parked on. Measured against a frozen Postgres (`docker pause` — precisely "TCP open, no query response"): the probe returned only after 142s, when the server was thawed, and each hung probe held a connection from the request pool for the whole outage. nginx's `depends_on: service_healthy` means that also blocks an nginx (re)start. The probe now uses its own NullPool engine with asyncpg's connect AND command timeouts armed under the outer bound. `command_timeout` alone was not enough — a NullPool probe opens a fresh connection, and against a frozen server the hang is in the startup exchange, which is the connect timeout's job (measured: three consecutive probes still past 30s with only command_timeout). With both: 503 in 3.00s, three for three, recovering in 0.05s once thawed. Connection-refused stays fast (503 in 0.003s) and the healthy path is 0.30s. Ratchets unchanged: ruff src 251, mypy 145. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…c blockers from the closure audit (#21, #24, #26) Per-issue closure audits against the LIVE issue text found five things the issues' own "Definition of done" clauses require and the branch had not delivered. 1. #24, new defect (regression introduced by this branch's own reopen guard). `migrate_public_thread_to_private` wrote its AgentChannel / PrivateChannelMember / handover rows with `flush()` only, leaving them hostage to the caller. After 4062eeb added the IntegrityError guard, a lost race on uq_proposal_reviews_decision_agent rolls back — discarding the AgentChannel row while the Slack channel it just created stays, and restoring `refined_in_channel` so no retry can repair it. The engine discovers private channels ONLY from AgentChannel (simulation.py:2113-2126), so the outcome was a real Slack channel no bot would ever read, with the PI shown a success redirect. Pre-branch the same race 500'd and a retry re-migrated, so it was recoverable. The migration now commits as soon as its Slack side effects are irreversible, which is where the durability boundary belongs; a caller's later failure then costs only that caller's own rows. 201 tests across the migration's unit + four integration files pass. 2. #21 DoD clause 3: "V11 and V3 additionally add their prerequisites to docs/inbound-email.md's bring-up checklist." Only V11's had landed. V3's is now there: quarantine after MAX_S3_PROCESS_ATTEMPTS, the unknown-charset fallback, the int/bool rating coercion and the bounded S3 pagination, plus the two behaviours an operator will notice once the flag is on. 3. #21 V11-h: specs/local-db-conversations.md said "Five writer-slot claims" and omitted REMEDIATION_WRITER_SLOT = 99, which the issue's Fix explicitly counted. Now six, with the slot numbers and where the sixth lives. 4. #26 A9: AGENT.md called the status-overview digest "wired daily". The worker calls it from a ~300 s poll loop and each PI's cadence comes from email_notification_preferences.frequency, which defaults to weekly. 5. #26 A10: AGENT.md still said "the 8 pilot labs" at the line the issue names, and its "Pilot Lab ORCIDs" table listed 10 PIs against orcids.txt's 48. Both now point at AgentRegistry as the live roster and the table is labelled the historical snapshot it is. 6. #26 blocker 4: the CLAUDE.md and README DOC-7 paragraphs omitted `-e PYTHONPATH=/app`, contradicting production-migration.md §8/§11, run_migration.sh's own step-8 text and Part R.6b — and the omission was pinned by test_slack_ts_repair_documented.py. Command and pin corrected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…epending on the ambient DSN (#27 I2, #25 P1.4) 93a48e7 gave /api/health its own engine (it needs asyncpg connect/command timeouts that asyncio.wait_for cannot supply), so this fixture's `get_session_factory` patch no longer covered the probe: the test fell through to whatever settings.database_url pointed at and returned 503 instead of 200 on any machine without a Postgres on the default DSN. `./scripts/ci.sh` was red for this reason alone. Two independent closure audits (#23 R11 and #25) bisected it to 93a48e7. The fixture now also patches `get_health_engine`, recording through the same statement list so the "no badge queries ran for /api/health" assertion still sees everything the request issued. Verified green both with and without DATABASE_URL set in the environment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…le (#27 I4, #20) The #27 closure audit found `./scripts/ci.sh` red at HEAD for a reason no commit caused: `alembic 1.19.2` was published to PyPI today at 17:10:12Z, and the lock step compared the committed lock against a FRESH `pip-compile`, so upstream publishing anything turned the gate red and blamed "pyproject.toml changed without regenerating it" when nobody had touched it. It was not even reproducible on one machine — two back-to-back runs of the same command disagreed (alembic 1.19.1 vs 1.19.2, rich 14.3.4 vs 15.0.0) depending on which HTTP cache the ephemeral environment saw. With D17 making ci.sh the whole gate and the pre-push hook running it, that check did not protect the lock; it blocked every push on PyPI's schedule and taught everyone to reach for LOCKCHECK=none. The drift worth gating on happens inside the repository: a dependency edited in pyproject.toml without regenerating the lock. That is decided by the two files, so `scripts/check_lockfile.py` now checks it from the two files, offline and deterministically — every direct dependency present in the lock, pinned at a version its specifier allows. Verified it catches all three real shapes: a dependency added to pyproject (names `requests`), a floor raised past the pin, and a cap that excludes the pin. `LOCKCHECK=strict` keeps the fresh-resolve comparison for when someone deliberately asks "could this lock be newer?", as a NOTE that does not fail. Also closes the two remaining #27 I4 Fix clauses the audit found unimplemented: - "raise floors past known CVEs" — checked against OSV today: jinja2 <3.1.6 (6 advisories), python-multipart <0.0.31 (16), authlib <1.7.1 (22). Floors raised to the first safe releases. The lock already resolved above all three, so nothing moves today; the point is that a future resolve cannot walk back in. - "cap pre-1.0/major SDKs" — uvicorn, httpx, asyncpg, python-multipart and typer were uncapped. Now capped <1.0.0. I first over-applied this to alembic/boto3/pydantic-settings/itsdangerous/rich and reverted it: a <15 cap on rich downgraded a working 15.0.0 to 14.3.4 for nothing, and those five are not what the Fix line names. The behavioural mutation test now proves the new check fails closed and names the drifting package; `shutil`'s skip valve went with the network dependency (and its unused import was itself failing the gate's ruff step first, which is how both subprocess tests surfaced it). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…ctually applied (#22 COR-22) profile_pipeline.py wrote raw_abstracts_hash unconditionally at step 9, even on the branch where a produced synthesis was explicitly discarded to protect a better stored profile (failed validation twice, or lost evidence on a refresh). That records this run's abstracts as the last-seen input even though its output was thrown away, so change detection would treat a later run over the same still-bad input as unchanged relative to the good profile that is actually stored. The write now happens after the store decision and is skipped specifically on the discard branch (new synthesis_discarded flag); a run that never produced a synthesis at all (LLM outage, synthesized == {}) still records its hash, since there is no better output it lost to -- that case is pinned by the existing test_profile_pipeline_llm_failure_leaves_fields_unset GM snapshot, which stays green and unchanged. RED: tests/characterization/test_profile_pipeline_gm.py::test_profile_pipeline_discarded_synthesis_does_not_record_the_new_abstracts_hash failed before the fix (second.raw_abstracts_hash == the discarded run's own hash, not the version-1 run's hash it should have kept). GREEN: tests/unit/test_apply_synthesis.py tests/unit/test_validate_profile.py tests/unit/test_profile_pipeline_dedup.py tests/characterization/test_profile_pipeline_gm.py -> 53 passed, 11 snapshots passed (unchanged). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…tops re-reopening (#20 COR-13) _db_reopened_thread_ids is in-memory only and always starts empty on a fresh process, so the per-tick reopen block in _sync_proposal_reviews_from_db (keyed on thread_id, same as this seed) re-entered on the first post-restart sync for any still-open reopened thread. already_minted already stopped the duplicate persisted PI-guidance row, but the block still unconditionally overwrote both agents' ThreadState with a fresh message_count_offset — the "fresh reply budget each restart" half of the bug named in the issue, which meant a reopened thread could never reach the 12-message timeout close across restarts. Seed the set during _rebuild_agent_state from the latest ThreadDecision per thread whose reopened_at is set — the same set the function already computes (reopened_thread_ids) to keep those threads out of closed_thread_ids, so the key matches the per-tick check exactly. Residual not fixed by this change (verified, not papered over): _rebuild_agent_state's own per-agent restoration of a reopened thread's ThreadState (the "2. generic active-threads loop", ~:4939 and the _rebuild_one_agent_state mirror ~:5276) recomputes offset = msg_count fresh at every process restart, because its only guard is "already tracked THIS process" (`if thread_id in agent.state.active_threads: continue`), not across restarts. So a reopened thread's reply budget still effectively resets on every restart via that separate path -- closing that fully needs a durable "replies since reopen" counter or a stored offset next to reopened_at, which is a bigger change than "persist reopen/dedup state" (the issue's literal Fix: clause) asks for. Test evidence: RED: tests/integration/test_state_rebuild.py::test_a_rebuild_seeds_the_reopen_dedup_set_and_does_not_re_reopen failed before the fix: "assert '<ts>' in set()" (assert root_ts in eng._db_reopened_thread_ids) -- 1 failed, 1 passed (the reopened_at-IS-NULL sanity control already passed). GREEN: tests/integration/test_state_rebuild.py -q -> 11 passed. Neighbours: tests/unit/test_simulation_logic.py tests/unit/test_roster_sync.py tests/unit/test_thread_not_found.py tests/unit/test_funding_reply_backoff.py -q -> 177 passed. ruff check tests/integration/test_state_rebuild.py -q -> clean. ruff check src --output-format=concise --quiet | grep -c . -> 251 (unchanged ceiling). No prompt changes: AST-walked every ast.Constant string in src/agent/simulation.py before/after -- 730 strings both sides, sha256 identical (c11ba8b0c4b0e42588993ff933f28485bcdf7d7a8fc3d354fb42f858d4dc42b5). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
… transient failure retries instead of losing the row (#20 COR-10(3)) _poll_inbound_from_db advanced _pi_inbox_cursor and appended the row to the message log before calling _handle_pi_inbound_entry, so a raise inside the handler left the row's content in the log while the lookback re-scan deduped on that now-present entry -- permanently losing the PI-specific side effects (proposal-review clearing, reopen, pi_context, @bot tag routing) with no second chance. Reorder so the handler runs first; the log append and the cursor advance now happen only after it returns successfully. Already-known and tombstoned rows still advance the cursor unconditionally (nothing will ever process them either way). This deliberately changes the failure mode from at-most-once to at-least-once, exactly the trade issue #20's COR-10 Fix: clause asks for ("apply side effects before ... the cursor advance") and it reverses the plan's Decision D25 ("accept the single-loss") -- documented in a comment at the reordered code. Concretely: on a transient failure the row is retried on the next poll (within PI_INBOX_LOOKBACK) and _handle_pi_inbound_entry runs again for it. Its side effects are a proposal-review clear/persist (safe to re-run -- persist is already idempotent per (thread_decision, agent)), a thread reopen (_reopen_thread; guarded elsewhere against double-reopen), an in-memory pi_context/has_pi_directive set (idempotent), and @bot tag routing via handle_channel_tag, which can send a DM -- that one repeats visibly to the PI if the first attempt's side effect actually landed before some LATER step in the same handler call raised. Test evidence: RED: tests/unit/test_simulation_logic.py::TestPollInboundFromDbGuardsTheHandler (rewritten) -- 2 of 3 failed against pre-fix code: the permanently-raising case found the row already in the log (old at-most-once behavior) and the transient-retry case found the row landed after the FIRST (failing) attempt already; the happy-path case passed unchanged. GREEN: same class -> 3 passed. tests/unit/test_simulation_logic.py -q -> full file passes. tests/integration/test_state_rebuild.py -q -> 11 passed (unaffected). tests/integration/test_message_persistence.py -q -> 19 passed (this function's other real-DB coverage, not owned by this task, unaffected). Neighbours: tests/unit/test_simulation_logic.py tests/unit/test_roster_sync.py tests/unit/test_thread_not_found.py tests/unit/test_funding_reply_backoff.py -q -> 179 passed. ruff check tests/unit/test_simulation_logic.py -q -> clean. ruff check src --output-format=concise --quiet | grep -c . -> 251 (unchanged ceiling). No prompt changes: AST-walked every ast.Constant string in src/agent/simulation.py against the prior commit (ac218fb) -- identical multiset of 730 string literals both sides (order-independent sha256 c5ab15037612ba980e9da9cf81e48150780eeaf841e58cdd2b3118b7db28f53d matches); the raw concatenation hash differs only because the reorder changed AST walk order, not string content. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…ist signup, on two connections (#24 V5) Issue #24's Definition of done for V5 is explicit: "a concurrent-insert test". Every existing V5 test (tests/unit/test_concurrent_write_guards.py) drives the route function with a hand-built fake AsyncSession that raises a scripted IntegrityError at a specific call -- nothing inserts, nothing races, no real unique constraint is exercised. The plan excused this as unavoidable given the shared, savepoint-joined db_session fixture; that excuse is false -- tests/integration/test_profile_version_race.py already races two independent sessions from the session-scoped `engine` fixture with asyncio.gather, and tests/integration/test_worker.py's _race_claims proves overlap with a threading.Barrier. This adds the missing test in that same shape: two independent async_sessionmaker(engine) sessions per test (real connections, real transactions), asyncio.gather'd, racing on the real unique constraints (`waitlist_signups.email`, `uq_proposal_reviews_decision_agent`). Postgres serializes the second writer's INSERT on the unique index, so no asyncio.Barrier is needed for a deterministic race. RED against pre-fix code (git show 18ba52c:src/routers/public.py and git show 18ba52c:src/routers/agent_page.py copied into a scratch checkout, verified by printing src.routers.public.__file__ so imports actually resolved to the scratch copy, not the real tree): FAILED test_two_concurrent_first_time_waitlist_signups_race_on_email sqlalchemy.exc.IntegrityError: duplicate key value violates unique constraint "waitlist_signups_email_key" -- uncaught out of db.commit() FAILED test_two_concurrent_first_time_reviews_race_on_the_unique_constraint review_proposal raised instead of guarding the race: IntegrityError('... duplicate key value violates unique constraint "uq_proposal_reviews_decision_agent" ...') GREEN at HEAD: both tests pass (2 passed), plus the unit-level fake-session guards in tests/unit/test_concurrent_write_guards.py (5 passed, unaffected). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…stead of failing the job (#22 COR-16) Issue #22's COR-16 Note says the new uq_publications_user_pmid constraint "makes PR V5's IntegrityError discipline relevant to any writer of this table", but no publications writer had it: two overlapping pipeline runs for one user raced on the plain `await db.flush()` after the insert loop and the second job failed outright at the constraint instead of double-inserting. Added `_insert_publication_tolerating_conflict`, an `INSERT ... ON CONFLICT DO NOTHING` on (user_id, pmid) mirroring the existing `_claim_foa` pattern in src/agent/grantbot.py, and wired it into the new-publication branch of the insert loop. Chose ON CONFLICT over a per-row SAVEPOINT: a `begin_nested()` whose flush raised a real IntegrityError left the session needing an explicit top-level rollback under this app's `join_transaction_mode="create_savepoint"` test fixture, whereas ON CONFLICT DO NOTHING never raises at all. The in-run dedup from 22.4 (`_dedup_pmids` + the in-loop `existing_pubs` update) still runs first, so this only ever tolerates a genuinely concurrent OTHER session's writer, not a duplicate within one run. RED: tests/unit/test_profile_pipeline_dedup.py::test_a_concurrent_writer_inserting_the_same_pmid_does_not_raise failed with ImportError (_insert_publication_tolerating_conflict did not exist yet). GREEN: tests/unit/test_apply_synthesis.py tests/unit/test_validate_profile.py tests/unit/test_profile_pipeline_dedup.py tests/characterization/test_profile_pipeline_gm.py -> 55 passed, 11 snapshots passed (unchanged). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…ate channel (#21 COR-19.6) Issue #21's Fix for COR-19.6 is "order side effects after commit (or make them idempotent)". Commit 34d3c15 made migrate_public_thread_to_private commit its own AgentChannel/member/handover rows and thread_decisions.refined_in_channel as soon as its Slack side effects are irreversible, on the theory that this alone makes a retried inbound e-mail idempotent -- closure-21.md flagged that claim as unproven. It does not hold: drives process_inbound_email twice over the same S3 object (real committing sessions from the `engine` fixture, a stubbed Slack client counting channel creations) -- pass 1 forces the migration's own commit to land for real while the CALLER's later commit (email_inbound.py:446, which would persist the ProposalReview row and flip notification.status to "responded") is made to fail, simulating exactly the Postgres hiccup COR-19.6 describes; pass 2 retries with a fresh session and no induced failure. _handle_instruction's only idempotency guard (email_inbound.py:853-865) checks for a ProposalReview row -- it never reads refined_in_channel or origin_visibility, and origin_visibility is never flipped away from "public" by the migration. So when pass 1's outer commit failure loses the ProposalReview add and the notification-status flip, pass 2 finds notification.status still "sent" and no ProposalReview row, and re-runs migrate_public_thread_to_private end to end -- asking Slack for a second real private channel and leaving TWO AgentChannel rows with the same channel_name. Per the brief: does not force the fix. The test is committed as a strict=True xfail with the full mechanism recorded in the reason (and the two-AgentChannel-rows evidence in the docstring), so it flips to a plain passing regression pin once COR-19.6's residual is actually closed. Evidence: without the xfail marker, the test FAILED at the final assertion -- `assert sum(len(c.created_channels) for c in slack_migration_stub) == 1` -> `assert 2 == 1`, and the DB-state assertion right before it read back two AgentChannel rows (`<AgentChannel ... name=priv-agent2-agent3-channel-4>` x2, same channel_name). With the marker: `1 xfailed`. Full file: `pytest tests/integration/test_email_inbound_reply_paths.py -q` -> 23 passed, 1 xfailed. Neighbour: `pytest tests/unit/test_concurrent_write_guards.py -q` -> 5 passed (unaffected). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
… one (#22 COR-23, #29) 93a48e7 changed save_private_profile's `content` to Form("") so an emptied textarea (which submits `content=`) could clear the profile -- that stays correct and is pinned. But Form("") also means a request that OMITS `content` entirely gets the same "" and destroys both private columns and the disk file; before that change such a request 422'd instead. Not reachable from the browser (the textarea always submits the field), but a data-destroying default for any other client (#22 COR-23 residual, item 44 in the closure audit). The brief's literal suggestion of `content: str | None = Form(None)` does not work: verified with an isolated FastAPI repro that its Form() dependency resolution collapses a present-but-empty value and a genuinely omitted field to the SAME declared default no matter what that default is (None included), so no Form() parameter shape can tell them apart. The fix instead reads the raw Starlette FormData directly (`form = await request.form()`), which does distinguish them -- the same pattern save_public_profile already uses for its six profile fields just above this route -- and 400s when "content" is not a key in it, leaving both DB columns and the exported file untouched. Applied identically to the onboarding.py twin, which has always had the Form("") shape. RED (measured against the ACTUAL Form("") pre-fix code, by reverting the fix in place and re-applying it after, per IMPLEMENTER_PREAMBLE -- no git reset/stash used): tests/integration/test_private_profile_clear.py::test_omitting_content_entirely_is_rejected_and_survives and ::test_onboarding_twin_also_rejects_an_omitted_content_field both got 302 (everything cleared) instead of 400; the 3 pre-existing tests in the file, including the `content=` -> 302-clears-everything pin, stayed green throughout. GREEN: tests/unit/test_apply_synthesis.py tests/unit/test_validate_profile.py tests/unit/test_profile_pipeline_dedup.py tests/characterization/test_profile_pipeline_gm.py -> 55 passed, 11 snapshots passed (unchanged); tests/integration/test_private_profile_clear.py tests/integration/test_onboarding_flow.py tests/integration/test_agent_page.py -> 189 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Nnc7rWXX4MbGSnGNEthzx
…ss-wide event (audit 2026-09-10 O-1) The per-pool event / thread-local binding / _pool_shutdown_pending flag design (K-2, M-2, N-2) regressed in three consecutive review rounds. Replace it with a single threading.Event (SHUTDOWN_REQUESTED, SHUTTING_DOWN kept as a compat alias) that every caller checks regardless of thread or pool, is sticky for the process's life, and is never cleared by src/. slack_executor's _get_executor() no longer touches the event at all; shutdown_slack_executor() now uses cancel_futures=False so a queued run_slack_call still runs and aborts via the event instead of raising CancelledError. The agent process's shutdown handler now calls signal_shutdown() directly instead of shutdown_slack_executor(), since it doesn't own that pool's lifecycle. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ite is actually pending (audit 2026-09-10 O-2) _check_pi_proposal_review recorded a (agent_id, thread_id) pair in _deferred_implicit_reviews whenever thread_decision_id was None, even when no matching payload was ever queued in _pending_thread_decisions -- most notably with session_factory=None, where _close_thread never enqueues a ThreadDecision write at all, so the pair would leak for the rest of the run. Also drop matching pairs when the N-6 cap evicts a payload, since a dropped payload can never be replayed either. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ly exercise the guard (audit 2026-09-10 O-3) test_force_cleared_private_profile_is_not_resurrected_by_the_watcher ran with no session_factory, so N-3's DB-authoritative branch early-returned and the assertion passed regardless of whether the force-cleared guard worked at all. Stub a session confirming ResearcherProfile.private_profile_md == "" and keep the retry unlink failing, so the test exercises the actual still-un-removable branch; verified red (with the guard temporarily disabled) before restoring it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…DB resync (audit 2026-09-10 O-4) _sync_one_agent_private_profile_from_db now reports whether it reached a definitive verdict (a real AgentRegistry/ResearcherProfile row was actually consulted) vs bailing out early. The watcher's force-cleared branch in _sync_profiles_from_disk uses this to: (1) skip advancing agent_sigs[sub] when no verdict was reached, so a bump that could not be resolved (DB unreachable, no linked user, no profile row) is retried on the next tick instead of being silently treated as already-handled forever; and (2) re-stat the file after a verdict that rewrote it, instead of reusing the pre-write signature, so the helper's own write is not logged as a spurious external edit on the following tick. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…t (audit 2026-09-10 P-1) Root cause: shutdown_slack_executor() was only registered via a plain atexit.register(), and plain atexit callbacks run AFTER threading._shutdown() has already joined every non-daemon thread (including the Slack I/O pool's workers) to completion. A worker sleeping through _sleep_interruptibly at interpreter exit therefore ran out its whole sleep -- nothing had set SHUTDOWN_REQUESTED yet -- before the atexit callback ever got a chance to fire (reviewer measured a 5s sleeper completing in full). Fix: also register slack_client.signal_shutdown directly via threading._register_atexit, whose hooks run BEFORE the thread join (guarded with hasattr, falling back to atexit.register on interpreters that lack the private API). The existing atexit.register( shutdown_slack_executor) backstop is kept for actually shutting the pool down/dropping the module reference. Corrected the module docstring's claim about atexit ordering to match. Test: a real subprocess starts a 5s run_slack_call sleeper and falls off the end of __main__ (unforced interpreter exit); asserts wall time < 3s. Was red (5.19s) before the fix. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…(audit 2026-09-10 P-2) Root cause: the first SIGTERM/SIGINT only scheduled the Slack-abort event via loop.call_later(SHUTDOWN_SLACK_ABORT_GRACE_SECONDS, signal_shutdown). If sim_engine.start() returned before that timer fired -- a --max-runtime run finishing on schedule, a clean stop, or a flush simply faster than the 20s grace -- the timer was dropped when the event loop closed and SHUTDOWN_REQUESTED was never set at all, even though the process has already committed to exiting. Fix: factor the pending call_later handle onto the shutdown closure's state and add _finalize_shutdown(shutdown), called from _run_simulation's finally block AFTER sim_engine.stop()'s DB flush. It cancels the now-moot pending timer and calls signal_shutdown() unconditionally. Test: a teardown path with a live run_slack_call sleeper and zero signals received (no timer ever scheduled) now raises SlackShuttingDown once _finalize_shutdown runs; a second test pins that the pending grace timer is actually cancelled. Both were red (AttributeError: no _finalize_shutdown) before the fix. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…(audit 2026-09-10 P-3) Root cause: _call_with_retry only ever checked SHUTDOWN_REQUESTED inside the except-SlackApiError retry-sleep branch, i.e. after attempt 0 had already made a full network round trip. Queued-but-unstarted work therefore always paid for at least one HTTP call even when the process had already committed to shutting down. Fix: check SHUTDOWN_REQUESTED before attempt 0 and raise SlackShuttingDown immediately if already set. Corrected the exit-bound claims in slack_client.py's and slack_executor.py's docstrings, which stated only the retry-sleep's <=1s bound and did not account for the unavoidable first HTTP call attempt 0 used to always make; the actual bound is now "zero round trips if not yet started, else one in-flight HTTP call + <=1s". Test: with SHUTDOWN_REQUESTED already set before the call, asserts _call_with_retry raises SlackShuttingDown with zero recorded Slack calls. Was red (DID NOT RAISE) before the fix. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…audit 2026-09-10 P-4) Root cause: SHUTDOWN_REQUESTED is process-wide and sticky by design, so every test file that sets it had to clear it again itself, before AND after, or poison every later test in the same pytest process. That made each file implicitly depend on every other file's cleanup discipline (and on its own fixture/finally actually running) rather than there being one place that guarantees the invariant. Fix: add an autouse `_clear_slack_shutdown_requested` fixture to tests/conftest.py that clears the event before and after every test. Removed the now-redundant per-file fixtures in test_agent_main_finalize_shutdown.py and test_agent_main_shutdown_grace.py (their entire job was that same clearing); simplified test_slack_executor.py's fixture to drop its own now-duplicate clear call while keeping its pool-shutdown responsibility, which the new conftest fixture does not cover. Verification: ran the shutdown-related unit test files together in one process (previously order-sensitive state) — all 104 pass. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…es its thread_id (audit 2026-09-10 P-5) Root cause: the overflow purge in _enqueue_pending_thread_decision dropped a _deferred_implicit_reviews pair whenever its thread_id appeared ANYWHERE in the dropped (oldest) payloads -- even when a second, still-pending payload for that same thread_id survives the purge. That surviving payload can still legitimately flush and be matched against the deferred review, so purging it lost a real PI-engagement review for no reason. Fix: compute the set of thread_ids that still have a remaining (i.e. not dropped) payload after the purge, and only drop a pair whose thread_id is in dropped_thread_ids AND NOT in that remaining set. Test: two payloads queued for the same thread_id, one of which is dropped by the overflow purge and one of which survives -- the deferred review for that thread_id must survive too. Was red before the fix (the pair was incorrectly dropped). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ct (audit 2026-09-10 P-6) Root cause: _sync_one_agent_private_profile_from_db returned False (no verdict reached) for four distinct cases that O-4's watcher used identically to decide whether to advance its mtime signature: no session_factory, no AgentRegistry row / no linked user_id, no ResearcherProfile row, and an exception talking to the DB. The first and last are genuinely transient -- the DB may answer differently on the very next tick with no further disk change -- but "no linked user_id" and "no ResearcherProfile row" are definitive: a real query ran and conclusively found nothing, and that fact will not change without another disk edit. Treating them the same as a transient failure meant the watcher re-queried the DB on every single tick for an agent that structurally has no linked profile, forever. Fix: return True (reached a verdict) for the no-linked-user-id and no-ResearcherProfile-row branches, so the watcher advances its signature and stops re-querying until the file changes again. Only no-session_factory and the exception handler still return False. Test: replaced O-4's "no linked user_id" test (which asserted the OLD, now-wrong behaviour) with two tests -- one using a DB call that raises to pin that a genuinely transient failure still does not advance the signature (retries every tick), and a new one pinning that a no-linked-user verdict DOES advance the signature and does not re-query the DB on a second tick with no further disk change. The new test was red (2 queries instead of 1) before the fix. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…inked registry row stays transient (audit 2026-09-10 Q-1, Q-2) Q-1: the P-5 purge kept a (agent, thread) pair whenever any payload shared the thread_id, but the flush replays by (agent_id, thread_id) against agent_a/agent_b, so a different-pair payload kept a never-replayable pair alive forever. Q-2: AgentRegistry.user_id is populated later by signup/activation, so treating it as definitive froze the watcher for a live agent. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… never let finalisation skip the run-status update (audit 2026-09-10 Q-3, Q-4) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ay predicate, and finalisation always signals (audit 2026-09-10 R-1, R-3, R-4) R-1: Q-2 had made agent_reg is None transient too, re-querying the DB every tick for an agent with no registry row. R-3: the record site still used a thread_id-only predicate, leaking never-replayable pairs. R-4: a failing timer cancel could skip signal_shutdown(). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ll SIGTERM/SIGINT with signal.signal (audit 2026-09-10 S-1) Root cause: _sync_roster_from_db (roster add/remove/reconnect and the grantbot uid re-probe) and _phase1_channel_discovery called AgentSlackClient.connect()/join_channel() synchronously on the event-loop thread instead of through run_slack_call. Under a sustained Slack throttle each call can sit in its retry/backoff loop for up to RATE_LIMIT_WAIT_BUDGET_SECONDS (180s), blocking the ONE process-wide event loop for that long. Because the SIGTERM/SIGINT handler was installed with loop.add_signal_handler, whose self-pipe is only drained by the loop's own select/poll wait, a blocked loop silently swallowed the signal entirely: no request_stop(), no grace timer, no signal_shutdown(), until the blocking call happened to return on its own -- docker's SIGKILL could arrive first. Fix: - Route every connect()/join_channel() call reachable from the roster-sync path (both _sync_roster_from_db branches, _resolve_service_bot_uids' grantbot probe, and the per-turn _phase1_channel_discovery) through run_slack_call so they run on the dedicated Slack I/O thread pool instead of the event loop. - Install the SIGTERM/SIGINT handler with signal.signal instead of loop.add_signal_handler. signal.signal handlers are invoked via CPython's EINTR-retry machinery (PEP 475) and still fire while the main thread is blocked inside a synchronous call. request_stop() (a plain flag flip, already documented safe from a signal handler) runs synchronously and immediately; the loop.call_later grace-timer installation -- which mutates the loop's internal timer heap and must never run inside a true signal-handler context -- is deferred onto the loop via loop.call_soon_threadsafe. The second signal calls signal_shutdown() directly, without depending on the loop ever becoming free, since that is exactly the guarantee a blocked loop can no longer make. - Updated the docstrings/comments that claimed the abort "fits inside the 30s stop grace" to state the actual bound now achievable: one in-flight Slack HTTP call plus <=1s once the process is actually signalled. _ensure_seeded_channels' list_channels/create_channel/join_channel calls (also direct on the loop, per the grep the audit item calls for) are intentionally left out of scope: they run exactly once, before the turn loop and its signal-sensitive steady state begins, so the starvation risk they carry is far smaller and converting them would require an async signature change cascading into several Docker-only integration tests. Tests (tests/unit/test_roster_sync.py::TestRosterSyncDoesNotBlockTheLoop, tests/unit/test_agent_main_shutdown_grace.py): confirmed red against the prior synchronous connect() (0 ticks recorded during the blocking window) and green after routing through run_slack_call; added a handler test proving the second-signal path sets SHUTDOWN_REQUESTED even when shutdown() is invoked from a thread other than the event loop's own. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…_FILE with -f args (audit 2026-09-10 S-2) Root cause: docker compose ignores $COMPOSE_FILE entirely the moment ANY -f/--file flag is passed on the command line -- documented docker compose precedence, not a bug in compose itself. _known_files() unconditionally unioned $COMPOSE_FILE with every -f argument before the guard checked for both docker-compose.prod.yml and docker-compose.override.yml, so `COMPOSE_FILE=docker-compose.prod.yml:docker-compose.override.yml ./scripts/redeploy.sh -f docker-compose.prod.yml` passed the guard (the override string came from $COMPOSE_FILE) even though the compose() invocations that follow only ever pass `-f docker-compose.prod.yml` to docker compose -- the override never reaches it. Every service would come up on the prod file's bare `logging.driver: awslogs` and die immediately with AccessDeniedException (see CLAUDE.md "Compose file set"). Fix: _known_files() now falls back to $COMPOSE_FILE only when NO -f/--file argument was given at all. Once any -f is present it is the exhaustive list of files docker compose will actually use, matching docker compose's own precedence, and $COMPOSE_FILE is ignored for the guard exactly as it will be for the real invocation. Test: reproduces the exact scenario from the audit item (COMPOSE_FILE=prod:override, single `-f docker-compose.prod.yml`) — confirmed red (script proceeded, exit 0, docker log non-empty) before the fix, green (non-zero exit, empty docker log) after. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…he live roster (audit 2026-09-10 S-3) Root cause: _flush_pending_thread_decisions' `if not a: continue` guard (agent not on self.agents, e.g. deactivated since the decision was queued) skipped BOTH the pending_proposals decision-id stamp (correctly moot -- no in-memory ProposalRef to update for an absent agent) AND the deferred- implicit-review replay + removal, which needs only `aid` and the now-resolved `decision_id`, not a live Agent object -- `_persist_implicit_proposal_review` reads everything it needs from the DB. The PI engagement that queued that review was permanently lost, and the (aid, thread_id) pair stayed in `_deferred_implicit_reviews` forever, silently re-checked and no-op'd on every future flush. Fix: move the deferred-review lookup/replay/removal out from under the `a is not None` guard so it runs for every (aid, thread_id) pair regardless of whether that agent is still on the live roster. Test: TestCloseThreadDecisionWriteRetriesAndParks:: test_deferred_review_replays_even_for_an_agent_off_the_live_roster — confirmed red (pair stayed in _deferred_implicit_reviews, replay never called) before the fix, green after. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…te from PI_INBOUND_MAX_ATTEMPTS (audit 2026-09-10 S-4) Root cause: _poll_inbound_from_db's generic `except Exception as exc:` counted PiOwnershipLookupFailed (a transient DB failure resolving which agents a PI's user id owns) toward the same PI_INBOUND_MAX_ATTEMPTS == 3 budget used for a deterministically-failing handler. A 30s DB blip spanning three unlucky polls could exhaust that small cap and get a genuine PI directive permanently stamped HANDLED, indistinguishable from a row whose handler is actually broken. Fix: added a dedicated `except PiOwnershipLookupFailed` branch, ordered before the generic handler, that records the failure in a SEPARATE counter (`_pi_inbound_lookup_failures`, pruned the same way as `_pi_inbound_attempts`) against a new, much larger `PI_INBOUND_MAX_LOOKUP_FAILURES = 30` cap (src/agent/inbound_state.py). The main `_pi_inbound_attempts` counter is never charged for a lookup failure, and vice versa (both are popped on a subsequent success). Tests (tests/unit/test_simulation_logic.py:: TestPollInboundFromDbSeparatesLookupFailuresFromHandlerAttempts): confirmed red (row stamped HANDLED after PI_INBOUND_MAX_ATTEMPTS + 2 lookup failures) before the fix; green after, plus a companion test confirming the row still eventually gives up once PI_INBOUND_MAX_LOOKUP_FAILURES is reached. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…se the quote-aware segment split (audit 2026-09-10 S-5)
Root cause: _authentication_results_ok extracted the authserv-id with a
naive `header.split(";", 1)[0]` and compared it verbatim to the literal
string "amazonses.com". RFC 8601 §2.2 permits a trailing
`authres-version` token on the authserv-id (SES may stamp
"amazonses.com 1"), so any message carrying that version token was
rejected outright as "not amazonses.com" even though it genuinely
transited SES. The naive split was also not quote-aware, unlike
_split_auth_results_segments (already used for the resinfo segments
below), so a quoted identity containing a literal ";" could fabricate a
fake authserv-id boundary.
Fix: extract the authserv-id from the same quote-aware
_split_auth_results_segments()[0] the resinfo segments already use, then
split that segment on whitespace and compare only the authserv-id token
itself, ignoring an optional version suffix.
Test: test_authserv_id_with_rfc_8601_version_token_is_accepted — confirmed
red (message rejected with "amazonses.com 1" != "amazonses.com") before
the fix, green after.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… token is absent from To (audit 2026-09-10 S-6) Root cause: process_inbound_email only ever looked for the review+TOKEN@ reply address in the To header. A PI who Ccs the reply address instead of (or in addition to) To, or a forwarding rule that moves the review address out of To entirely (recorded by the downstream MTA in Delivered-To or X-Original-To instead), had their reply silently dropped with "No reply token found in To address" even though the token was present elsewhere in the message. Fix: when To carries no token, check Cc, then Delivered-To, then X-Original-To, in that order, before giving up. Tests (tests/unit/test_email_inbound_hardening.py): one per fallback header plus a negative case confirming a message with no token anywhere is still dropped without touching the db. Confirmed red (all three fallback tests failed with "DID NOT RAISE AttributeError", i.e. never reached the token-lookup db call) before the fix, green after. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ated pool (audit 2026-09-10 S-7) Root cause: src/services/email_inbound.py's _send_simple_email/ send_html_email_outcome and src/services/email_notifications.py's _send_html_email each construct a boto3 SES client and call send_email/send_raw_email synchronously. Every async call site (_notify_instruction_failure, _notify_reply_expired, _handle_instruction, _send_review_confirmation, _send_instruction_confirmation, _maybe_send_stale_token_bounce, _send_help_email, _send_paused_email, _send_status_overview, _send_new_proposal_email) invoked one of these directly, running the blocking SES network call ON the worker's single event loop and stalling every other coroutine (Slack polling, DB writes, other pending sends) for however long SES takes to respond. Fix: added src/services/io_executor.py, a dedicated bounded thread pool (IO_MAX_WORKERS=8, separate from slack_executor's 16-worker pool so a slow SES send cannot starve Slack I/O or vice versa) with run_blocking(fn, *args) -- a drop-in asyncio.to_thread replacement -- and shutdown_io_executor(), wired into src/main.py's lifespan and src/worker/main.py's shutdown path alongside shutdown_slack_executor(). Every async call site above now awaits its SES send through run_blocking; the sync helper signatures (_send_simple_email, send_html_email_outcome, _send_html_email) are unchanged. _notify_instruction_failure and _notify_reply_expired (previously sync wrappers making the same direct call) are now async so their callers can await the run_blocking call. Tests (tests/unit/test_io_executor.py, tests/unit/test_email_send_off_loop.py): confirmed red (send ran on the test's own thread) for every converted async call site before the fix; green (send ran on an "io-blocking" worker thread) after. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…le max_workers=8 (audit 2026-09-10 S-8) Appends a compact summary of S-1 through S-7 (this branch's eight items) to docs/plans/2026-09-08-audit-fixes.md, matching the existing per-round bullet style used for the Q and R sections. Also fixes D8: the R-2 write-up's "Fix"/"Tests" paragraphs still said the Slack executor shipped with max_workers=8 and pinned that value in its own tests. That was only ever true for the moment R-2 itself landed — "R-2 follow-up #2" (same day) raised it to SLACK_IO_MAX_WORKERS = 16, and the shipped module has carried 16 ever since, but the earlier paragraphs were never annotated to say so. Annotated both in place rather than rewriting history. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…fter the second signal, and defer request_stop to the loop (audit 2026-09-10 T-1..T-3) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… SIGINT restores default_int_handler; docs for the three-signal semantics (audit 2026-09-10 U-1..U-3) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ecord complete Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…s the codebase Rewrite comments and docstrings in src/, scripts/, alembic/, templates/, nginx, compose files, the Dockerfile, .dockerignore and tests/ so they explain what the code does and why, for a reader new to the repository. Removed audit/review/task labels, dates, plan and spec pointers, commit hashes, production evidence and fix-round narrative; kept every invariant and constraint the deleted text carried. Non-comment changes are limited to label removals inside log/help/assert message strings, alembic "Create Date" boilerplate, and test renames whose names embedded labels (test_dat1_*, test_p2_*, test_local_only_ci_stance_*), with one pinned assertion in tests/unit/test_ci_gate.py following the reworded ci.sh sentence. Gate: ./scripts/ci.sh green (3111 passed / 120 skipped, 80.84 % branch, ruff 248/260, mypy 139/150, head 0030). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…gated-path commands Co-Authored-By: Claude Fable 5.1 <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.
Summary
Resolves the verified-defect issues #20–#27 (opened by @ahueb, re-verified 2026-08-11) on top of
copi-prod@ 18ba52c, then hardens the result with a second adversarial audit run live against thecopi-testSlack workspace. Every fix ships with a test that fails on the pre-fix code. Migrations 0025–0030 are additive. No file underprompts/and no model-facing string changed.Issues addressed
#20 — Agent engine: turn & thread state-machine correctness
✅finalizes only the most recent:memo:and both emoji spellings are treated alike on public and private paths; dead threads are evicted, tombstoned and deduplicated in_prior_threads.@Botroute are authorized by the agents the sender owns or delegates for (agent_messages.sender_user_id, migration 0030), persisted durably with a retry-and-flush path, and keyed identically by the rebuild and per-tick readers.<@Uxxx>mentions resolved, ungrounded authorship claims rejected.#21 — Worker & background jobs
failedstatus with retry backoff; a reaper for orphanedprocessingjobs.False.#22 — Profile pipeline & write integrity
(user_id, pmid)unique constraint with data dedup (0025); PubMeditertext()so titles and abstracts are not truncated.tempfile+os.replace); an overwrite is gated on validation state; the private-profile seed is exported and the DB is authoritative over disk at startup and on roster re-add.#23 — External clients
Retry-After, a rate-limit wait budget (180 s, 8 attempts) with interruptible sleeps, Slack calls run off the event loop in a dedicated executor, nesteddisplay_name.#24 — Web request-path robustness
IntegrityErrors (waitlist, vote, PI message, review, reopen) return 4xx instead of 500, proven by a two-connection test.Retry-After; nginx gives the provisioning route its own 300 s timeout.#25 — Data layer
passive_deleteson cascaded children (0026).runningsimulation runs are reconciled at startup./staticand/api/healthbypass the session middleware.pool_pre_ping/pool_recycle/ explicitpool_timeout.#26 — Documentation
backfill_slack_ts.pydocumented as a one-time repair.#27 — Deploy, CI & coverage gate
scripts/ci.shvia the pre-push hook)./api/healthprobes the DB with a bounded deadline; amigrateone-shot gates app/worker/grantbot;scripts/redeploy.shruns stop → migrate → verify → start → nginx reload..dockerignoreexcludesbackups/,.env*,profiles/, tests and.git; the image runs as UID 10001.requirements.lockwith a freshness check and an opt-in install smoke test./api/csp-report), per-service resource limits, json-file log rotation.Second audit (2026-09-08 → 09-10)
A live adversarial audit of the branch on
copi-testfound further defects, all fixed with red-first tests and recorded indocs/plans/2026-09-08-audit-fixes.md: DB-path PI authorization by thread membership (#20 COR-5); PI messages and DMs written whileagent-runwas down losing every side effect (pi_inbound_state,pi_dm_messages.handled_at, 0030); a standing instruction whose disk write failed clobbering the DB; new posts defaulting to#generalwhen the model omitted the channel; a thread decision dropped after a blocking Slack retry sleep; graceful shutdown that could hang on a Slack retry or lose the final flush (single sticky shutdown event, interruptible sleeps, signal defaults restored only after the run status is committed); admin delete orphaning a live agent; health-probe retries exceeding the deadline;COPI_PROFILES_DIRwith a live-tier preflight check.Verification
./scripts/ci.shat 72caa26: alembic head 0030, round trip clean, ruff 248/260, mypy 139/150, 3111 passed / 120 skipped, 80.84 % branch coverage.copi-test(T0BMVSBMEC8) at 72caa26: 53/53 non-LLM, 8/8 real-LLM (docs/plans/2026-09-04-decisions/task-35.md).Closure
Closes #20, #22, #23, #24, #25. #21, #26 and #27 are closed by hand after merge with the stated carve-outs indocs/plans/2026-09-04-decisions/README.md(#21: the worker coverage clause's premise was already false; #26: needs deploy verification; #27: its definition of done is the gate itself). Residual risks and follow-ups are listed indocs/plans/2026-09-02-close-issues-20-27-pr-body.md.Migration and deploy
Schema goes 0024 → 0030 (0025 dedups
publicationsand adds a unique index; 0026 cascades; 0027 adds 20 indexes; 0028–0030 add nullable columns). The chain runs as one transaction and takes the web app down for its window, so use the gated path, not a bareup -d --build.Runbook:
docs/plans/2026-09-02-close-issues-20-27.mdPart R (steps R.0–R.11, ordered for the prod host), with the command reference indocs/production-migration.md§10.2. In outline:git pulland build the new images without recreating anything.docker stop -t 30 agent-run, thendocker compose $C stop grantbot worker, thenstop app. nginx returns 502 for the window; expected.sudo chown -R 10001:10001 profiles data(neverprompts/).DATABASE_URLexported as the in-network DSN, rehearse then apply:LockNotAvailableErrormeans a writer is still connected.scripts/backfill_slack_ts.py --applyonce, if the workspace has never had it../scripts/redeploy.sh $Crecreates app, worker and grantbot on the new image and reloads nginx.docker compose $C --profile agent build agent, then startagent-runlast.GrantBot needs its own Slack bot token before the first run. Do not squash-merge: the commit sequence is the review trail.
Closes #20, #22, #23, #24, #25
🤖 Generated with Claude Code