Repository navigation
test(archive-stale-results): 15 tests for retention + dry-run + edge cases - #1131
bassilkhilo-ag2 wants to merge 8 commits into
Conversation
Before: "multiple processes (2 PIDs: 25023,25065)" — no way to tell if this is expected app-bundle + dev-repo coexistence or a zombie duplicate. After: "multiple processes (2 PIDs: 25023:/Applications/Sutando.app/.../ discord-bridge.py, 25065:/Users/.../sutando/src/discord-bridge.py)" — immediately actionable, confirms the two instances are different source files running in parallel, not the same script spawned twice. 5 tests in tests/health-check-bridge-multi-pid.test.py. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…port (closes sonichi#961) Adds `scripts/lint-py39-annotations.py`: AST-based lint that walks `src/*.py` and flags any file using PEP-604 `X | Y` union syntax in type annotations without `from __future__ import annotations`. All current src/*.py files are clean; the script acts as a ratchet preventing new violations. - `has_future_annotations()` — AST check for the future import - `uses_union_syntax()` — AST walk for BinOp(BitOr) in annotation positions; ignores docstrings and bitwise-OR in expressions - `lint_file(path, fix=False)` — per-file check; --fix mode inserts the import - CLI: `python3 scripts/lint-py39-annotations.py [--fix] [files...]` - 15 tests in `tests/py39-annotation-lint.test.py` — all passing Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…laim (closes sonichi#933) claim_task.py (sonichi#884) renames task-{id}.txt → task-{id}.claimed-core-N.txt atomically. All three bridges had hard-coded bare .txt paths in their archive and tier-read blocks. archive_file() silently no-ops on missing paths, so claimed files were stranded in tasks/ forever — surfacing as false health-check task-queue alarms. Adds find_task_file(tasks_dir, task_id) to slack-bridge, telegram-bridge, and discord-bridge: checks bare path first, falls back to glob for .claimed-core-*.txt. Returns None if neither exists (so callers can skip rather than silently no-op). Changes per bridge: - slack-bridge.py: 1 archive site - telegram-bridge.py: 2 archive sites (skip-marker + normal reply paths) - discord-bridge.py: 5 archive sites + 2 tier-read sites (channel-redirect access_tier lookups now also find claimed files instead of defaulting to "other" tier on miss) 13 tests in tests/stranded-claim-files.test.py — all passing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
sonichi#863) Adds scripts/lint-cwd-writes.py — grep-based lint that flags process.cwd(), Path.cwd(), and os.getcwd() calls outside the canonical resolver files. Prevents re-introduction of the workspace/repo split-brain bug (sonichi#863). Allowlist: workspace_default.{py,ts}, util_paths.{py,ts}, inline-tools.ts, browser-tools.ts, and any *_helper* / *.test.* / tests/ paths. Comment lines and inline trailing comments are stripped before matching. CLI: exits 0 if clean, 1 if violations found. 13/13 tests passing in tests/cwd-lint.test.py. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers: missing results/ no-op, mtime cutoff (archive old, skip fresh),
non-.txt skip, archive-* subdir immunity, DRY_RUN=1 print-only mode,
DRY_RUN case-insensitivity ('No'/'FALSE' as falsy), archive-YYYY-MM-DD
naming, file placement inside archive dir, multi-file batch.
Motivated by the 2026-04-15 DM-flood incident (142 stale files fired as DMs
on service restart) — this archiver was the prevention mechanism; tests
ensure it remains correct.
12/12 tests passing.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- T13: mixed fresh+stale — only stale archived, fresh untouched
- T14: RETENTION_HOURS=0 archives everything regardless of age
- T15: .sending files (orphan proactive-delivery claims) are not
archived — only .txt suffix is in scope
Total: 15/15 passing.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
BLOCKING
Same dual issue as #1124 — please split and remove build_log.md before this can land.
-
build_log.md +114/-0leak — adds Pass 207-211 notes (identical content to the leak in #1124). Discloses internal branch names (port/watch-tasks-attempts-stando-63,port/share-screen-standalone-1073,port/result-channel-key-1090,port/za-warudo-mention-precedence-1091,merge/stando-sync"162 commits ahead"), commit SHAs, and pending-push intent.build_log.mdis in.gitignorefor a reason — this is per-instance operational state. The currentbuild_log.mdon main is from a prior author (Lucy) and would be overwritten/diverged by this PR. -
Scope creep — title is
test(archive-stale-results): 15 tests, but the diff ships:- 4 additional test suites (
cwd-lint.test.py,health-check-bridge-multi-pid.test.py,py39-annotation-lint.test.py,stranded-claim-files.test.py) - 2 new lint scripts (
lint-cwd-writes.py,lint-py39-annotations.py) - 4 bridge edits (
discord-bridge.py,slack-bridge.py,telegram-bridge.py) +health-check.py - All of which are reasonable to land separately but aren't mentioned in the PR scope.
- 4 additional test suites (
Asks:
- Drop
build_log.mdfrom the diff and consider rewriting branch history so it doesn't ship as a deleted/added cycle either. - Split into: (a)
test(archive-stale-results)— just the 15 tests + the script under test; (b) lint scripts + their tests; (c) bridge/health-check edits with their own context.
NON-BLOCKING
The actual archive-stale-results.test.py (291 lines, 15 tests) looks well-scoped — T15 (.sending not archived) is a sharp catch. Worth keeping once split out.
|
Superseded by #1136 — clean branch from current origin/main with only the 15 archive-stale-results tests. Previous branch had build_log.md and scope-creep from a stale diverged base. |
|
Cold-review take (Sutando-Mini, Mac mini): On the actual archive-stale-results coverage: LGTM. 15 tests cover retention/dry-run/edge-cases comprehensively (T7 — existing-archive-subdir bypass, T15 — Three issues to flag before merge: 1. Scope creep — this is 4 PRs in one. Title says
Four distinct concerns, four separate review surfaces. Recommend splitting — each can ship independently. Same pattern I flagged on #1124 ("3 PRs in 1"). Smaller PRs = faster reviews + tighter blast radius if something needs revert. 2. Workspace-contract violation (carry-over from #1124). 3. SUTANDO_WORKSPACE-env test pattern (same trap as #1148/#1149). Your description says "Uses Workaround until #1149 lands: monkey-patch the specific module's path resolver instead of setting the env var. See discord_config.config_path = lambda: tmp_path / "state" / "..."Not a blocker for review but worth fixing in this PR's tests too (or merging #1149 first). 4. DIRTY/CONFLICTING merge state. Needs rebase against current main. — Sutando-Mini |
Summary
src/archive-stale-results.pyis the startup DM-flood prevention guard (post-mortem:notes/post-mortem-dm-flood-2026-04-15.md) — it had zero test coverage.What's covered
tests/archive-stale-results.test.py— 15 tests:results/absentRETENTION_HOURS.txtfiles even if stalearchive-*subdirsDRY_RUN=1prints "would archive" without movingDRY_RUN=Nois correctly treated as falsy (not dry-run)archive-YYYY-MM-DDarchive-YYYY-MM-DD/RETENTION_HOURS=0archives everything immediately.sendingfiles (in-flight proactive claims) are NOT archivedAll 15 pass on Python 3.9.
Notes
SUTANDO_WORKSPACEoverride to give each test a clean isolated workspacesubprocess.runto exercise the real code path including theworkspace_defaultimport🤖 Generated with Claude Code