Repository navigation
Conversation
Birfy
requested changes
Nov 10, 2026
Birfy
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
Tip SHA: 900f69a
Ran (on a trial merge of this tip into main 1aaf1bd):
python3 .github/scripts/risk_gate.py --repo ThinkFlowLab/agentdescent --pr 925→ T3 (pathagentdescent/runstore.py)pytest -q tests/test_run_events.py tests/test_run_panel.py→ 18 passedpytest -q(full suite) → no failures through ~81% before my 600 s local timeout; CItest (3.9–3.13)still running at review time
Not run: a browserEventSourceclient; the full suite to completion locally.
Blocking
- Unrelated
CNAMEfile at the repo root (agentdescent.hugoxk.online). It isn't part of this feature and isn't in the commit900f69a; it comes in from the fork's history (theMerge branch 'Birfy:main' into mainmerge on the fork'smain). Merged into this repo, a rootCNAMEcan repoint the project's GitHub Pages to a personal domain. Please drop it from the PR. The cleanest way is to branch from currentThinkFlowLab/agentdescentmain(not the fork'smain) and cherry-pick900f69a. - PR body still says
Tip SHA: PENDINGandpytest -q → PENDING. Please fill in the real tip and full-suite result once (1) is fixed.
Non-blocking (correctness/design)
- A stalled run streams forever. If the run process dies without writing a terminal status,
statestaysrunningand the handler thread loops until the client disconnects. A cheap guard would be to emit a terminalstatus(or close) whenstatus.json'spidis no longer alive, the same waystatus/list_runsdetect a stale run, or to cap the stream with a max idle time so the client reconnects. - Each poll re-parses the whole
rounds.jsonl. That's fine for tens of rounds, but tracking the byte offset (or at least only parsing lines past the last-seen count) would keep long runs O(new lines) per tick. - Status change and the rounds that caused it are emitted in status-then-rounds order. On the terminal tick, the final rounds are emitted after the status was read but before return, which is right. On a non-terminal state change, though, a client sees
statusbefore the rounds that preceded it. Consider reading rounds first, then status, so event order matches what happened. Last-Event-IDwith a negative or garbage value silently falls back to replay-all. That's reasonable, but a one-line test would pin it.- Keep-alive interval (15 s) is hard-coded while the poll interval is configurable; making it a module constant next to
EVENT_POLL_SECONDSwould let tests cover the keep-alive branch, which no test exercises today. _eventsduplicates the run-id traversal check indo_GET; hoisting it into one helper would keep the two endpoints from drifting.
Good
The read-only, loopback-only CORS boundary matches the JSON endpoints. X-Accel-Buffering: no, retry: and the round-index ids are the right SSE details, and the tests drive a real server on an ephemeral port with a small event_poll. The docs change in docs/plugins.md merges cleanly onto main as a small paragraph, so it's fine as is.
Risk tier T3: this PR needs Danyang's explicit merge click even after a PASS; it won't be auto-merged.
This branch has not been deployed
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
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.
Fixes #922
What changed
Adds
GET /api/runs/<run_id>/eventsto the existing read-only run-storeserver. It is a Server-Sent Events stream:
statusevent fromstatus.json;roundevent per newrounds.jsonlline, with the round index as theevent id, so a reconnect can send
Last-Event-IDand resume after the lastround it saw;
statusevent when the run reachesdone,failedorcancelled,then the stream closes;
retry: 1000and periodic: keep-alivecomments;endpoints, and the same run-id traversal checks.
The stream polls
rounds.jsonl(default everyEVENT_POLL_SECONDS = 0.5) anddoes not add a write path, an auth change or a background thread to the run
itself.
serve_http(..., event_poll=...)is a wall-clock knob, used by thetests to keep them fast.
Verification at the tip
Tip SHA: 900f69a
Ran:
pytest -q tests/test_run_events.py tests/test_run_panel.py→ 18 passedpython -m tools.gen_api_docs --check→ up to datepytest -q→ 5 failed, all reproduced on the base1aaf1bd0in this macOSenvironment before the change:
test_dynamic_cheatsheet::test_the_shared_runner_really_is_ungated...,test_era_algotune::test_compilation_is_not_charged_wherever_the_author_put_it,test_gskill::test_a_memory_hungry_candidate_costs_its_score_not_the_run,test_integrations::test_the_launcher_install_writes_ignores_a_shadowing_cwd,test_porous_molecules::test_every_heavy_atom_gets_a_position...(CI runs Linux; these are environment-sensitive, not touched by this diff.)
Not run:
EventSourceclient — the offline tests read the stream withhttp.client; the protocol is covered at the transport level, no browser isneeded.