chore: burn down the repository's measured slow, dead, and quietly broken parts - #241
Merged
Conversation
Three findings from a repository diagnostic, each reproduced before it was acted on. `scripts/tests/test_bench_event_latency.py` was never executed by CI. Seven test modules live under `scripts/tests/`; the `test` and `runtime-floor` jobs each named six. AGENTS.md carries it in the canonical pre-PR list, so the gate was weaker than the document it is measured against. It costs 0.052s. Its one real timing assertion is a lower bound on an elapsed wait, so load can only make it more true - safe on a shared runner. `.claude/worktrees/` was ignored only through `.git/info/exclude`, which is per-clone and uncommitted. Its sibling `.worktrees/` was already in the tracked file. A fresh clone therefore searches its own worktrees, and every repo-wide grep returns each hit once per checkout - measured here while chasing an unrelated symbol. `read_sidecar` in observer.py had no caller anywhere: of 254 public definitions in the runtime it was the only one with zero references outside its own definition. `write_sidecar` stays - http_api.py calls it and returns the result directly rather than reading back, which is how the reader was orphaned. `state_read_cap_bytes` keeps its consumer in lifecycle.py. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Jared Scott <jared.scott@variable.team>
The canonical pre-PR validator walked every Markdown file under `docs/`, and
most of them are not source. 56 of the 78 present here are gitignored: the
vendored Spacedock mods, the linked worktree holding the entity-state orphan
branch, and a person's local deep-dive notes.
That made `python3 scripts/validate_plugins.py` fail on clean `main` for anyone
with the mods installed, on a vendored template's unexpanded `{state-owner}`
placeholders. CI never saw it, because the directory is gitignored and is never
checked out - so the red belonged to no change and pointed at no fix.
Filtered with `check-ignore` rather than `ls-files`, so a doc that is merely new
is still validated before it is staged. Fails open: any git error validates
everything, which is exactly the behaviour it replaces.
Verified by injecting a broken link into a tracked doc and confirming the
validator still fails on it.
AGENTS.md's Quality Gate sentence described the 3.11 job as a direct-launch
smoke test. It also runs the whole suite under coverage on that floor, so an
agent reading the sentence would underestimate what has to pass there.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Jared Scott <jared.scott@variable.team>
`support.py` already seeds CARGENTO_HOME into the process environment, and the comment there explains why: unset, the suite reads the developer's real ~/.cargento and its verdict depends on what they happen to have dismissed. The same argument applies one level up and was never made. Harness store roots resolve from the user's home, not from CARGENTO_HOME (config.py:403 reads USERPROFILE on Windows and HOME everywhere else), so every test that collects without patching a store walked the developer's real ~/.claude, ~/.codex and ~/.cursor. On this machine that is 24,981 files under ~/.claude/projects alone. Measured on the same command, `unittest discover` over the dashboard tests: 59.5s before, 38.1s after. 2,005 tests, green both ways. Per module the effect tracks how much collecting each one does: test_quota 4.63s -> 1.69s, test_lifecycle 8.04s -> 4.97s, test_http_api 11.34s -> 8.96s. CI will not see that gain - a fresh runner's home holds no agent stores - so this is a local-developer and local-agent saving. The hermeticity half applies everywhere: a suite whose runtime and whose result both depend on what the developer's other agents were writing while it ran is not measuring what it claims to. A second temp directory rather than reusing STATE_HOME, because the two answer different questions: a test that redirects CARGENTO_HOME itself must still see an empty harness home rather than the state it just pointed elsewhere. This also subsumes the separate `_clean_env` fix in test_lifecycle: that helper copies os.environ, which now carries the seeded HOME before it is read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Jared Scott <jared.scott@variable.team>
…low edits `validate` was the only job in the repository with no `timeout-minutes`, so it inherited GitHub's 360-minute default. Its longest step is the same `unittest discover` that AGENTS.md records hanging on subprocess and socket waits under load, and it is a required check - so a hang held the PR pending for six hours instead of failing in ten minutes. Its checkout also lacked the `persist-credentials: false` that every other checkout in the repository sets. `plugin-compatibility` ignores prose but not workflows, so editing any workflow file installed four upstream CLIs over the network to prove that cargento's package layout had not changed. A workflow file cannot change that layout. The trade, which the filter now states: a change to the canary itself no longer runs on its own PR. The push-to-main and Monday schedule runs cover it, and it is not a required check, so nothing is gated on the gap. mypy's `files` listed three of the six entry-point scripts. The other three - the hook adapters the shipped manifests actually register - were checked only because tests/test_events_ingress.py imports them and `tests` is on the list, a guarantee lasting exactly as long as those imports. Now stated: 110 source files before, 113 after, clean either way. AGENTS.md's architecture tree named one hook, `notify_hook.py`, which no manifest registers. The three that ship - `event_hook.py` for Claude and Codex, `agy_hook.py` and `statusline_hook.py` for Antigravity - were absent, so an agent orienting on the tree would not find the adapter code that receives real harness events. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Jared Scott <jared.scott@variable.team>
…pped `spacedock.stage_names` had no caller in the runtime, the hooks, the MCP server or the scripts - only two test assertions. The one production site that needs exactly its value, spacedock.py:676, open-codes the comprehension instead. The result was a public function tested in isolation from the path it describes: a change to either could not fail the other's tests. Deleted rather than called from production, which was the other proposal. That site already holds `entries` and uses it twice, for `stages` and for `resting`; routing it through `stage_names(config, lines)` would re-read and re-parse the frontmatter to rebuild a list it is already holding. The tests now assert against `stage_entries`, which both of those methods already use a few lines away. AGENTS.md: "docs/plans/*.md | Transient plans for unshipped work. Delete a plan once its work ships." All three event-driven phase plans shipped - `scripts/bench_collect.py` for Phase 0, `cargento_runtime/snapshot.py` for 1a, `cargento_runtime/stream.py` for 1b - and 2,592 lines of them stayed. They open by instructing an agent to execute their 114 unchecked task boxes task-by-task, which is the wrong instruction to leave lying in a repository agents read first. The design owner, `event-driven-session-observation.md`, stays: it is linked from docs/design-runtime-architecture.md and docs/captures/README.md, and it already records what each phase delivered. Its one link to a deleted plan is rewritten. `native-notifications.md` stays too - still linked from two design docs and from notifications.py, and its decision is still open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Jared Scott <jared.scott@variable.team>
`scripts/bench_collect.py` crashed on its first call for anyone who ran it:
AttributeError: 'Namespace' object has no attribute 'host'
It hand-builds an `argparse.Namespace` to stand in for parsed CLI arguments and
hands it to `cli.build_runtime`. That function later grew `host`, `no_git`,
`no_dismiss` and `no_ask`; the stand-in was never extended, so the script died
on the first attribute read. Reproduced on clean `main` before changing
anything, so this is not fallout from the collector work in this branch.
CI did not catch it because `scripts/tests/test_bench_collect.py` exercises the
measurement helpers rather than `main()`, so every test passed against a script
that could not start. This matters more than a broken script usually would:
docs/plans/event-driven-session-observation.md and the comment at
observation.py:73 both point at this benchmark as the way collection cost is
measured, so the tool the repository names for answering "is this slower now?"
was the one tool that could not answer it.
Filled in with production defaults rather than benchmark-friendly ones, so the
figure still describes what a real collect pays. `no_usage=True` remains the one
deliberate difference, for the reason already written above it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Jared Scott <jared.scott@variable.team>
Three changes on the collection path, which runs on a 2.5s floor while a session is writing. Measured with scripts/bench_collect.py, repeat 5, against a real store: total collect 146.2ms -> 131.8ms (-9.9%), the Claude collector itself 123.9ms -> 110.0ms (-11.2%). The glob loop already takes `os.path.getmtime` for every transcript, then kept only the path. That made the newest-wins comparison re-stat the incumbent for a number it had just measured and dropped, and the per-prefix loop stat the winner a third time. `transcripts` now carries `(path, mtime)`, which removes both stats and the two OSError guards that existed only to cover them. `started_agent_ids` and the pending-member roster were computed one prefix above the `if not (active or show_all): continue` gate, and neither feeds `activity_sources`. `pending_members` is not read until the row is being built, several hundred lines below. So the comprehension, the sort and one `started_agent_ids` call ran for every prefix on disk to produce values discarded for all but the handful that survive the gate. Moved below it. Pure waste rather than a trade: nothing above the gate reads either name. The Cursor collector asked `os.path.exists(wal)` and then `os.path.getmtime(wal)` - two stats for one question, since `getmtime` raises when the file is absent. Worse, both sat under the same handler as the db's own stat, so a WAL that vanished between the two calls dropped the whole chat row instead of falling back to the db's mtime. A WAL is the file most likely to go while being read. Now one suppressed `getmtime`, scoped so its absence costs nothing. Deliberately not taken here: the larger restructuring of the same loop, which would skip the per-session subagent walk for prefixes outside the window. It is a real cost and a bigger change, with a behaviour trade around long-running subagents that deserves its own PR and its own measurement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Jared Scott <jared.scott@variable.team>
Five jobs in the quality gate call setup-python; four pass `cache: pip`. `runtime-floor` did not, and it installs the heaviest requirements of the five - requirements-dev.txt, which carries ruff, mypy and coverage - so it downloaded and built the whole toolchain on every run. Measured across two recent runs it took 37s and 39s, against 14-20s for the type-check job that installs the same file with a warm cache. It does not shorten the gate's wall clock: the critical path is changes -> Tests (windows-latest) at 119s -> aggregator, and runtime-floor is nowhere near it. This is runner time and cache-hit rate, not latency. Left alone deliberately: this job also runs the full suite under `coverage` and calls `coverage report`, which enforces pyproject's `fail_under` a second time on a second interpreter. There is a real argument that the threshold belongs to the `test` job alone and that a duplicate gate can fail for reasons unrelated to the 3.11 floor. But AGENTS.md says the threshold only ever ratchets up, and removing one of the two places it is enforced is a decision about how strict the gate should be rather than a defect to fix in a cleanup PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Jared Scott <jared.scott@variable.team>
The sync-docs tone check globbed `docs/plans/*.md`, which is also where per-issue deep-dive notes are kept locally and excluded one file at a time in .git/info/exclude. So the check reported drift in somebody's private scratch: three hits in a deep-dive note, none of them in a file any reviewer will read, and none fixable without editing a file that is not repository content. `git ls-files` instead of `ls`. It still covers all nineteen tracked prose docs, including the two plans that remain, and it keeps the reason the original `ls` was there: step 4 deletes plan docs, and a bare glob matching nothing stays literal and makes grep exit 2. Same shape as the validator fix earlier in this branch. Both walked `docs/` as though everything under it were source, and `docs/` has held vendored mods, orphan-branch state and local notes for a while now. With this, the tone check reads clean on a tree whose prose is already clean, which is what makes it worth running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Jared Scott <jared.scott@variable.team>
The WAL fix in the previous commit changed behaviour, so it gets a test that fails without it. Verified both ways: reverted to the pre-fix collector this asserts 1 != 0, because losing the race withdrew the whole chat row; with the fix the row survives on the db's own mtime. The race is made deterministic rather than waited for. `os.path.getmtime` raises for the `-wal` path and answers honestly for everything else, which is exactly the state the collector sees when a checkpoint removes the WAL between the existence check and the stat. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Jared Scott <jared.scott@variable.team>
Contributor
CoverageThreshold: |
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
A diagnostic pass over the repository for slow code, slow CI, and dead or inefficient code, then the fixes that survived adversarial verification.
Method. Eight finders ran one diagnostic dimension each and produced 37 findings. Every finding then faced three independent verifiers prompted to refute by default, each on a different lens: does it reproduce in the code, does the cost actually matter, and is the proposed fix safe against this repository's invariants. Majority-refute killed the finding. 15 of 37 were refuted and thrown away. The survivors were then reproduced by hand before anything was changed, which is where two more died and one was corrected.
Nothing here touches
cargento_runtime/web/, so no frontend byte pins move and this does not consume the one-PR-per-web/budget.What was measured
unittest discover, dashboard suitebench_collect, total collectbench_collect, Claude collectorThe suite figure is local-only: a CI runner's home holds no agent stores, so CI will not see it. The collect figures are on the poll path, which runs on a 2.5s floor while a session is writing.
Three things were broken and nothing said so
python3 scripts/validate_plugins.pyfailed on cleanmain. The canonical pre-PR check walked every Markdown file underdocs/, and 56 of the 78 there are gitignored: vendored Spacedock mods, the entity-state orphan-branch worktree, and local deep-dive notes. It died on a vendored template's unexpanded{state-owner}placeholders. CI never saw it because that directory is never checked out, so the red belonged to no change and pointed at no fix. Confirmed pre-existing by stashing and runningmaindirectly.scripts/bench_collect.pycrashed on startup withAttributeError: 'Namespace' object has no attribute 'host'. Its hand-builtNamespacewas never extended whencli.build_runtimegrew four arguments.test_bench_collectpasses because it exercises the helpers, notmain(). The design docs name this script as how collection cost is measured, so the one tool for answering "is this slower now?" could not start.scripts/tests/test_bench_event_latency.pywas never run by CI. Seven modules on disk; thetestandruntime-floorjobs each named six.AGENTS.mdcarries it in the canonical pre-PR list, so the gate was weaker than the document it is measured against. It costs 0.05s.The rest
HOME, so any test collecting without a store patch swept their actual~/.claude(24,981 files here). Speed was the smaller half: a suite whose result depends on what your other agents were writing while it ran is not measuring what it claims to.exists(wal)thengetmtime(wal), both under the db's error handler, so a WAL checkpointed away mid-read dropped the whole chat row. Fixed, with a regression test verified to fail against the old collector.runtime-floorwas the only one of fivesetup-pythoncalls withoutcache: pip, while installing the heaviest requirements file.validatewas the only job with notimeout-minutes, so a hang held a required check pending for six hours.observer.read_sidecar(the only one of 254 public runtime definitions with zero references anywhere) andspacedock.stage_names. Three shipped plan docs, 2,592 lines, that open by telling an agent to execute their 114 unchecked task boxes.Deliberately not done
coveragefromruntime-floor. There is a fair argument the threshold belongs to thetestjob alone. ButAGENTS.mdsays it only ratchets up, and removing one of the two places it is enforced is a decision about strictness, not a defect.discover()existence globs. Both survived verification. Codex is 4.8ms of a 131ms collect and the fix adds a cache field tostate.py, a known conflict surface; thediscover()change touches five files for under a millisecond. Filed rather than bundled.locked. Left alone: a sibling session may be mid-flight, andAGENTS.mdrecords a worktree cleanup destroying work once.Refuted, for the record
Among the 15: that the
platform-testsubuntu leg duplicates thetestjob (it does not, it alone proves the suite passes with no dev tooling installed, and removing it would save zero wall clock since Windows at 119s is the long pole); thatvalidate.ymlre-running the suite is a real cost; and thatassign_display_idsrebuilds a loop-invariant set that matters.Test plan
AGENTS.md§ Pre-PR Checks (ruff, ruff format, mypy,lint_embedded.py,validate_plugins.py,bump_version.py --current, dashboard tests under coverage via discovery) and it is clean. 2,006 tests plus 192 script tests, coverage 90.3% against a threshold of 73sync-docsskill — it found the deleted plans fully dereferenced, the command lists already linking toAGENTS.mdrather than duplicating, and one real defect of its own: its tone check globbeddocs/plans/*.mdincluding untracked local notes, reporting drift in somebody's scratch. Now scoped to tracked files.COMPATIBILITY.md's sync marker deliberately untouched, per the parallel-branch rule1 != 0) and pass afterclaude plugin validate --strictandagy plugin validateboth pass