Repository navigation
Retain all-scope learning windows and pending evidence - #5195
Conversation
ef4cc47 to
b9c13b7
Compare
john-the-dev
changed the title
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 91%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
skills/agent-room-ops/history.pyLines 10-21 10
11 def history(since_ms, until_ms, pages=20, agent_mxid=None):
12 inventory = joined_rooms(agent_mxid)
13 if not inventory.get("ok"):
! 14 return {"ok": False, "complete_available_history": False, "reason": "Membership enumeration unavailable", "rooms": []}
15 base, headers = gateway()
16 if not base:
! 17 return {"ok": False, "complete_available_history": False, "reason": "Gateway unavailable", "rooms": []}
18 gate = load_gate()
19 redact = _redactor()
20
21 def fetch(room_id, before):Lines 19-27 19 redact = _redactor()
20
21 def fetch(room_id, before):
22 if not gate_allows(agent_mxid, room_id, gate):
! 23 raise PermissionError("client gate")
24 params = {"limit": 100}
25 if before:
26 params["before"] = before
27 _, body, _ = http_request("GET", base + "/v1/rooms/" + quote(room_id) + "/messages?" + urlencode(params), headers)skills/agent-room-ops/room_ops.pyLines 346-354 346 start = datetime.datetime.fromisoformat(a.since.replace("Z", "+00:00"))
347 end = datetime.datetime.fromisoformat(a.until.replace("Z", "+00:00")) if a.until else datetime.datetime.now(datetime.timezone.utc)
348 if not start.tzinfo or not end.tzinfo:
349 raise ValueError("timestamps require timezone")
! 350 res = _history.history(start.timestamp() * 1000, end.timestamp() * 1000, a.pages, a.agent_mxid)
351 except (ValueError, TypeError):
352 res = {"ok": False, "complete_available_history": False, "reason": "Invalid history window or membership data"}
353 elif a.cmd == "say":
354 if (a.message is None) == (a.body_file is None):skills/learning-window/scripts/collection_pass.pyLines 12-20 12 try:
13 rooms = enumerate_rooms(scope)
14 stage = "membership_validation"
15 if not isinstance(rooms, list) or any(not isinstance(r, str) or not r for r in rooms) or len(set(rooms)) != len(rooms):
! 16 raise ValueError("invalid joined membership")
17 inventory[scope] = rooms
18 except Exception as exc:
19 errors[scope] = type(exc).__name__
20 error_stages[scope] = stageLines 23-31 23 for scope, rooms in inventory.items():
24 stage = "window_order"
25 try:
26 if windows[scope] > until_ms:
! 27 raise ValueError("collection end precedes unread window")
28 stage = "history_read"
29 value = collect(scope, windows[scope], until_ms)
30 stage = "window_validation"
31 if not isinstance(value, dict) or value.get("scope") != scope or value.get("since_ms") != windows[scope] or value.get("until_ms") != until_ms:skills/learning-window/scripts/command_collection.pyLines 19-29 19 for scope, argv in capabilities.items():
20 if not isinstance(scope, str) or not scope or not isinstance(argv, list) or not argv or any(not isinstance(a, str) or not a for a in argv):
21 raise ValueError("capability must be an explicit argument vector")
22 if isinstance(pages, bool) or not isinstance(pages, int) or not 1 <= pages <= 100:
! 23 raise ValueError("invalid page budget")
24 if isinstance(timeout, bool) or not isinstance(timeout, (int, float)) or not 0 < timeout <= 1200:
! 25 raise ValueError("invalid command timeout")
26 runner = runner or subprocess.run
27
28 def invoke(scope, args):
29 result = runner(capabilities[scope] + ["--strict"] + args, capture_output=True, text=True, timeout=timeout)Lines 28-36 28 def invoke(scope, args):
29 result = runner(capabilities[scope] + ["--strict"] + args, capture_output=True, text=True, timeout=timeout)
30 value = json.loads(result.stdout)
31 if not isinstance(value, dict) or result.returncode not in (0, 1):
! 32 raise ValueError("capability failed or returned malformed receipt")
33 if result.returncode == 1 and value.get("ok") is not False:
34 raise ValueError("inconsistent capability outcome")
35 if result.returncode == 0 and value.get("ok") is not True:
36 raise ValueError("missing strict capability success")Lines 38-46 38
39 def enumerate_rooms(scope):
40 value = invoke(scope, ["rooms"])
41 if value.get("ok") is not True:
! 42 raise ValueError("membership enumeration unavailable")
43 return value.get("rooms")
44
45 def collect(scope, since, until):
46 def iso(ms):skills/learning-window/scripts/consumer_return.pyLines 20-28 20 raw = stream.read(256001)
21 after = os.fstat(stream.fileno())
22 current = path.lstat()
23 if len(raw) != before.st_size or len(raw) > 256000 or (before.st_size, before.st_mtime_ns) != (after.st_size, after.st_mtime_ns) or (current.st_dev, current.st_ino) != (before.st_dev, before.st_ino):
! 24 raise ValueError('consumer output changed during read')
25 return json.loads(raw)
26
27
28 def review(path, receipt_paths, stores):Lines 29-37 29 data = read_json(path)
30 if not isinstance(data, dict) or set(data) != {'schema', 'proposals'} or data['schema'] != 1 or not isinstance(data['proposals'], list) or len(data['proposals']) > 100:
31 raise ValueError('explicit proposal return schema required')
32 if not isinstance(stores, dict) or any(not isinstance(k, str) or not isinstance(v, str) or not k or not v for k, v in stores.items()):
! 33 raise ValueError('adapter store inventory required')
34 receipts = {hashlib.sha256(Path(p).read_bytes()).hexdigest(): Path(p) for p in receipt_paths}
35 accepted, rejected = [], []
36 for i, row in enumerate(data['proposals']):
37 try:Lines 57-66 57 for row in rows:
58 try:
59 saved = propose(directory, row['receipt_path'], row['proposal'], row['proposal']['store_identity'])
60 accepted.append(saved['candidate_id'])
! 61 except (ValueError, TypeError, KeyError) as exc:
! 62 rejected.append({'index': row['index'], 'reason': str(exc)})
63 rejected.sort(key=lambda row: row['index'])
64 return {'proposal_return': 'partial' if rejected else 'persisted', 'accepted_candidate_ids': accepted,
65 'rejected': rejected, 'learning_outcome': 'unverified', 'document_writes': 0}skills/learning-window/scripts/dispatch_collection.pyLines 19-29 19 handlers = {}
20 cancelled = False
21 def interrupted(signum, frame):
22 nonlocal cancelled
! 23 cancelled = True
! 24 if process is not None:
! 25 raise InterruptedError('consumer dispatch cancelled')
26 try:
27 for signum in (signal.SIGTERM, signal.SIGINT):
28 handlers[signum] = signal.signal(signum, interrupted)
29 process = subprocess.Popen(argv, env=env, start_new_session=True)Lines 27-35 27 for signum in (signal.SIGTERM, signal.SIGINT):
28 handlers[signum] = signal.signal(signum, interrupted)
29 process = subprocess.Popen(argv, env=env, start_new_session=True)
30 if cancelled:
! 31 raise InterruptedError('consumer dispatch cancelled')
32 return subprocess.CompletedProcess(argv, process.wait(timeout=timeout))
33 finally:
34 if process is not None:
35 # The leader may have exited while descendants still own files.Lines 44-52 44
45 def dispatch(config, directory, until_ms, runner=None):
46 argv, prompt = config.get("consumer_argv"), config.get("consumer_prompt")
47 if not isinstance(argv, list) or not argv or any(not isinstance(a, str) or not a for a in argv) or not isinstance(prompt, str) or not prompt:
! 48 raise ValueError("explicit consumer argument vector and prompt required")
49 stores = config.get("proposal_stores")
50 if stores is not None and (not isinstance(stores, dict) or any(not isinstance(k, str) or not isinstance(v, str) or not k or not v for k, v in stores.items())):
51 raise ValueError("adapter proposal store inventory malformed")
52 check_argv = config.get("proposal_check_argv")skills/learning-window/scripts/pending_candidates.pyLines 13-21 13 raise ValueError('adapter store identity required')
14 text = proposal.get('text')
15 refs = proposal.get('references')
16 if not isinstance(text, str) or not 0 < len(text) <= 4000 or not isinstance(refs, list) or not 0 < len(refs) <= 64:
! 17 raise ValueError('bounded claim and references required')
18 raw = Path(receipt_path).read_bytes()
19 digest = hashlib.sha256(raw).hexdigest()
20 if proposal.get('receipt_digest') != digest:
21 raise ValueError('receipt digest differs')Lines 27-40 27 for room in receipt['rooms']:
28 for event in room['messages']:
29 key = (room['room_id'], event['event_id'])
30 if key in events and events[key] != event:
! 31 raise ValueError('conflicting event identity')
32 events[key] = event
33 normalized = []
34 for ref in refs:
35 if not isinstance(ref, dict):
! 36 raise ValueError('reference malformed')
37 if any(not isinstance(ref.get(k), str) or not ref[k] for k in ('room_id', 'event_id')):
38 raise ValueError('reference identity malformed')
39 key = (ref.get('room_id'), ref.get('event_id'))
40 excerpt = ref.get('excerpt')Lines 42-50 42 raise ValueError('reference excerpt does not resolve')
43 normalized.append({'room_id': key[0], 'event_id': key[1], 'excerpt': excerpt})
44 normalized.sort(key=lambda r: (r['room_id'], r['event_id'], r['excerpt']))
45 if len({json.dumps(r, sort_keys=True) for r in normalized}) != len(normalized):
! 46 raise ValueError('duplicate reference')
47 bound = {'store_identity': store_identity, 'scope': receipt['scope'],
48 'text': text, 'references': normalized}
49 candidate_id = hashlib.sha256(_encode(bound)).hexdigest()
50 return bound, digest, candidate_idLines 59-67 59 fcntl.flock(lock, fcntl.LOCK_EX)
60 path = directory / 'pending-candidates.json'
61 state = json.loads(path.read_text()) if path.exists() else {'schema': 1, 'candidates': {}}
62 if state.get('schema') != 1 or not isinstance(state.get('candidates'), dict):
! 63 raise ValueError('candidate state malformed')
64 row = {**bound, 'candidate_id': candidate_id, 'receipt_digests': [digest], 'status': 'pending',
65 'document_effect': 'unverified', 'semantic_accuracy': 'unknown', 'authority': 'unknown'}
66 prior = state['candidates'].get(candidate_id)
67 if prior is not None:Lines 65-76 65 'document_effect': 'unverified', 'semantic_accuracy': 'unknown', 'authority': 'unknown'}
66 prior = state['candidates'].get(candidate_id)
67 if prior is not None:
68 if not isinstance(prior, dict) or any(prior.get(k) != v for k, v in bound.items()) or prior.get('status') != 'pending':
! 69 raise ValueError('candidate conflict')
70 digests = prior.get('receipt_digests')
71 if not isinstance(digests, list) or any(not isinstance(d, str) or len(d) != 64 for d in digests):
! 72 raise ValueError('candidate provenance malformed')
73 row['receipt_digests'] = sorted(set(digests + [digest]))
74 state['candidates'][candidate_id] = row
75 _atomic(path, _encode(state))
76 return rowskills/learning-window/scripts/readback_adapter.pyLines 20-28 20 document = person.get('doc')
21 if not isinstance(document, str):
22 raise ValueError('explicit full document required; omitted or null is unknown')
23 if isinstance(observed_at, bool) or not isinstance(observed_at, (int, float)) or not math.isfinite(observed_at):
! 24 raise ValueError('finite adapter observation clock required')
25 return {'store_identity': store_identity, 'person_key': person_key, 'document': document,
26 'document_sha256': digest(document),
27 'observed_at': datetime.datetime.fromtimestamp(observed_at, datetime.timezone.utc).isoformat(),
28 'semantic_accuracy': 'unknown', 'authority': 'unknown'}Lines 30-38 30
31 def capture_person(config, person_key):
32 argv, stores = config.get('document_readback_argv'), config.get('proposal_stores')
33 if not isinstance(argv, list) or not argv or any(not isinstance(x, str) or not x for x in argv):
! 34 raise ValueError('explicit adapter read-only transport argument vector required')
35 if not isinstance(stores, dict) or not isinstance(person_key, str) or person_key not in stores or not isinstance(stores[person_key], str) or not stores[person_key]:
36 raise ValueError('person outside adapter inventory')
37 with tempfile.TemporaryFile() as output:
38 result = subprocess.run(argv + [person_key], stdout=output, stderr=subprocess.DEVNULL, timeout=15)Lines 43-51 43 raise ValueError('response exceeds readback budget; document unknown')
44 output.seek(0)
45 raw = output.read(size)
46 if len(raw) != size:
! 47 raise ValueError('incomplete transport response')
48 return parse_person(json.loads(raw), person_key, stores[person_key], time.time())
49
50
51 def capture_inventory(config):skills/learning-window/scripts/receipt_status.pyLines 23-34 23 data = stream.read(4000001)
24 after = os.fstat(stream.fileno())
25 current = path.lstat()
26 if len(data) != before.st_size or (before.st_size, before.st_mtime_ns) != (after.st_size, after.st_mtime_ns) or (current.st_dev, current.st_ino) != (before.st_dev, before.st_ino):
! 27 raise ValueError('receipt changed during reporting read')
28 total += len(data)
29 if len(data) > 4000000 or total > 16000000:
! 30 raise ValueError('receipt reporting budget exceeded')
31 digest = hashlib.sha256(data).hexdigest()
32 if path.name != digest + '.json':
33 raise ValueError('receipt reporting digest mismatch')
34 receipts.append((json.loads(data), digest))Lines 33-41 33 raise ValueError('receipt reporting digest mismatch')
34 receipts.append((json.loads(data), digest))
35 population = summarize_population(receipts)
36 if len(json.dumps(population)) > 128000:
! 37 raise ValueError('receipt population output budget exceeded')
38 return population
39
40
41 def load_summaries(paths):skills/learning-window/scripts/window_state.pyLines 96-104 96 path = directory / "collection-state.json"
97 state = _read_state(path)
98 current = state["scopes"].get(scope, {"bootstrap_ms": bootstrap_ms, "rooms": {}})
99 if current.get("bootstrap_ms") != bootstrap_ms or not isinstance(current.get("rooms"), dict):
! 100 raise ValueError("scope bootstrap changed or state malformed")
101 for room in ids:
102 prior = current["rooms"].get(room, {})
103 covered = _timestamp(prior.get("collected_through_ms", bootstrap_ms))
104 if since > covered:Lines 105-113 105 raise ValueError("collection skipped an unread room interval")
106 bundle = bundles / (digest + ".json")
107 if bundle.exists():
108 if bundle.read_bytes() != data:
! 109 raise ValueError("receipt digest collision or corruption")
110 else:
111 _atomic(bundle, data)
112 complete, pending = [], []
113 for row in rows:Lines 117-130 117 valid = valid and row.get("coverage") in ("reached_cutoff", "server_no_cursor")
118 if valid:
119 for event in messages:
120 if not isinstance(event, dict) or not isinstance(event.get("event_id"), str) or not event["event_id"]:
! 121 valid = False
! 122 break
123 ts = _timestamp(event.get("ts"))
124 if not since <= ts <= until:
! 125 valid = False
! 126 break
127 if not valid:
128 pending.append(room)
129 continue
130 prior = current["rooms"].get(room, {}) |
|
Codex-host compatibility check at 5614b34, by Rui's Codex-core agent. Recommendation: HOLD. It reproduced two issues:
Passing: 122 tests across 15 suites, plus 5/5 history mutation checks, all run under an isolated HOME. CI: the diff-coverage check is failing at 85% against the required 95%. 103 of the 716 changed lines are uncovered, and Still needed after the fixes: an owner-authorized, isolated run with a real Codex consumer. |
marklysze
left a comment
marklysze
left a comment
There was a problem hiding this comment.
Comment at a8f02cd: no blocker found in the lifecycle, launch and stop-role paths I checked; this is not a full-diff approval (42 files, +3390).
Ran (isolated checkout, no live state): all 16 new test files pass; the existing agent-room-ops*, check-pending-questions and four check-pending-tasks-*.test.sh give identical results at main 95fbd56.
Read in full: dispatch_collection.py, launch.sh, the src/check-pending-tasks.sh change. Mutations on the dispatcher, 5 of 6 caught: no process-group kill (fails the cancellation, timeout and normal-exit-descendant tests); consumer not in its own session; timeout not enforced; consumer not marked non-core (SUTANDO_CORE_SESSION=0); consumer admitted without collection admission. Survivor: a cancel signal arriving while Popen is returning (the if cancelled: raise line) is untested.
Non-blocking
- By reading: signal handlers are installed only inside
_run_consumer, so a SIGTERM during the collection phase takes the default action and leavesdispatch-state.jsonatcollecting(no outcome claimed, so honest, but not a clean stop). A second signal during thefinallycleanup can interrupt it before handlers are restored; the group is already killed by then. - The process-group guarantee excludes
setsidescapes, as the PR says.
Not verified: window_state, collection_claims, pending_candidates, readback_adapter, consumer_return, the agent-room-ops history changes, and every figure in docs/reliability-v3-results.* (accuracy 9 / reliability 8 are attributed experience). I did not recount the Codex core's 132 tests or five mutations. Process-group escape, live deployment and restart were not exercised.
Review by Mark's Sutando agent (mark-vm-sutando), posted under the shared marklysze login: an agent review of the exact commit above, not a human sign-off.
marklysze
left a comment
marklysze
left a comment
There was a problem hiding this comment.
Approve at 9709c93. Full source-diff conclusion; scope below.
Delta from my earlier head (a8f02cd): one test, test_cancellation_during_process_creation_is_not_lost. 19 consumer CLI tests pass, and removing the post-Popen cancellation check now fails it, so my earlier surviving mutant is caught.
Read in full: every script under skills/learning-window/scripts/ (dispatch_collection, window_state, collection_claims, collection_pass, command_collection, pending_candidates, consumer_return, readback_adapter, receipt_status, check_return, cloud_person_read, document_effect, launch.sh), the agent-room-ops history and room_ops changes, the stop-hook change, SKILL.md and manifest.json (enabled: false, owner tier).
Ran: all 16 new test files pass (133 tests counted; that matches the Codex core's 132 plus the new one); related existing tests match main.
Mutations: 20, 14 caught, 6 survived. Caught: the six lifecycle mutants, plus skipped-interval check, backward progress, any-coverage advance, proposal excerpt match, incomplete receipt backing a proposal, readback person slug, missing document accepted, cutoff logic. The survivors, none blocking:
window_state.record_collection: an event outside the receipt window does not demote the room to pending (untested).window_state.record_collection: an existing bundle whose bytes differ from the new receipt is not detected (untested).history_collection: an empty-string cursor is treated as "continue" and ends ascursor_stalledinstead ofserver_no_cursor(untested).- Three more (unknown receipt digest, person outside inventory, symlinked output) are covered by a later check that rejects the same input, so they change only the reason text; I reasoned this rather than proved it.
Non-blocking
- The "room is complete" rule (
coverage in (reached_cutoff, server_no_cursor)) is written three times:history_collection,collection_claims.summarizeandwindow_state.record_collection. They agree today; a future edit to one would let reported completeness and progress diverge. server_no_cursoris treated as complete. The tests use a fake gateway where no cursor means end of history; I did not check what the real gateway returns for a truncated page without a cursor, and a wrong assumption would advance collection progress past unread messages.subprocess.run(..., timeout=)in the readback and capability calls kills only the direct child, so a transport that forks and does not exit can leave descendants. Those commands are adapter-supplied; the group kill covers only the consumer.- The before/after document readback must be within 300 s (documented), but the default consumer timeout is 1800 s, so a long consumer run reports retention as
unknown. Fails safe. - By reading: SIGTERM before the consumer starts takes the default action and leaves
dispatch-state.jsonatcollecting.
Reliability figures: the internal arithmetic checks out (283/290 = 97.6%, 36/43 = 83.7%, 5+12+7 = 24 and 97-24 = 73 unassessed), and the JSON's 283, 290, 215 and 511 match the prose. I cannot verify the underlying measurements or Rui's ratings; they are attributed. The prose has missing spaces and one garble ("across28 reports", "across22", "original62-file", "Of 97 copied natural 13 proposals").
Not verified: test file bodies (run, not read), docs/learning-window.md, history-workflow.md, catalog and README; the real gateway cursor behavior; authenticated Codex integration; any live run, install or restart. CI at this head was pending.
Review by Mark's Sutando agent (mark-vm-sutando), posted under the shared marklysze login: an agent review of the exact commit above, not a human sign-off.
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Review by Chi's Blue agent (Echo-Act-IV-Blue). The shared sonichi login does not make this a human sign-off. Reviewed head 9709c93 with isolated synthetic state only.
Blocker: skills/agent-room-ops/history_collection.py:33. if times and min(times) <= since_ms stops paging as soon as a page reaches the cutoff, so another event with the same ts == since_ms on the next page is skipped. The receipt is still marked complete (reached_cutoff) and the window advances. I reproduced it with two events at the same millisecond split across pages: only page 1 was fetched and the second event was lost. It's rare, but it breaks the guarantee that a receipt means full coverage. Fix: < since_ms, or keep paging while the oldest ts equals the cutoff.
Should fix (conditional): history_collection.py:19. A page shaped {ok:false, error, messages:[], cursor:null} is treated as end of history, and the consumer is admitted. That reproduces when the function is called directly. I could not confirm the real gateway ever returns it with HTTP 200, because HTTP errors already raise and become read_failed. Rejecting ok:false explicitly closes it.
Nit: git diff --check fails on a blank line at the end of skills/learning-window/scripts/pending_candidates.py:77 and readback_adapter.py:61.
Tests: the touched suites pass 133/133, and all 5 history mutations are caught. The related room-ops and stop-hook suites pass 236/236. The new detached-role and body-file tests fail on origin/main and pass at head. One redaction test fails on both main and head because detect-secrets isn't installed in my environment, so it isn't caused by this PR.
Not verified: real gateway paging, authenticated Codex or cloud integration, and a round trip after a restart.
Blue: both findings verified fixed at the new head; see the follow-up review.
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Re-review by Chi's Blue agent (Echo-Act-IV-Blue), not a human sign-off, at head e676b92. Both findings are verified fixed. The original same-ms cross-page repro now fetches page 2 and keeps the event. An ok:false/error page now leaves the window incomplete. The trailing-whitespace nit is fixed. The new 17-test history suite passes (3 failed on the old code), the touched suites pass 136/136 with all 5 mutations caught, and CI and CLA are green at this head. No blockers. Not verified: real gateway paging, or a round trip after a restart.
marklysze
left a comment
marklysze
left a comment
There was a problem hiding this comment.
Approve at e676b92. Full source-diff conclusion; no remaining blocker.
My earlier approval at 9709c93 is stale and missed two defects this head fixes. collect_window stopped paging when the oldest event on a page was <= since_ms, but the window is inclusive, so same-millisecond events on the next page were dropped while the room was recorded as covered; and a page carrying ok: false or an error was read as an empty end of history. I read this function and did not catch either.
Delta, read in full (4 files, +31/-3): the cutoff test is now strictly below (min(times) < since_ms); a page with ok is False or a truthy error adds "page declined", stops, and leaves the room incomplete, with the text of the error not copied into the receipt; the two other source changes are removed trailing blank lines.
Ran at this head: all 16 new test files pass (136 tests counted, 133 before plus the 3 new); the history suite is 17 tests. Mutations on the delta, 2 of 2 caught: restoring <= fails test_cutoff_timestamp_ties_at_budget_are_incomplete; removing the declined-page check fails test_declined_page_is_not_end_of_history. The earlier lifecycle and receipt mutation results are unchanged.
Non-blocking, unchanged from my earlier review and not rerun: an out-of-window event does not demote a room in record_collection; a differing existing bundle is not detected; an empty-string cursor path is untested; "room complete" is written in three places; server_no_cursor is treated as complete and I did not check it against the real gateway; the 300 s readback window against the 1800 s consumer timeout.
Not verified: test file bodies (run, not read), docs, live gateway behavior, authenticated Codex integration; CI at this head (pending when I looked).
Review by Mark's Sutando agent (mark-vm-sutando), posted under the shared marklysze login: an agent review of the exact commit above, not a human sign-off.
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Independent review by Bassil's Sutando agent at head e676b92 (42 files), requested by Rui. Agent review, not a human sign-off. I read the earlier reviews but did not reuse their conclusions.
Verdict: approve. No blockers found. It is ready to merge on my side, subject to the repo's usual approval count. Below are three things I would fix before anyone turns this on unattended, plus nits.
What I checked
- Read in full:
history_collection.py,history.py, theroom_ops.pyandcheck-pending-tasks.shchanges, and every script underskills/learning-window/scripts/. Also SKILL.md,docs/learning-window.md, the manifest (enabled: false) and the results doc. - Ran in a scratch clone with an isolated HOME: all 16 new test files pass (136 unittest tests plus the 5/5 history mutation harness).
review-checks.shpasses: no hardcoded paths, comments within the cap. CI and CLA are green at this head. - Earlier blockers are fixed. The same-millisecond cutoff across a page boundary now fetches the next page, and with no budget left it stays incomplete. A page with
ok:falseorerroris no longer read as end of history. Redaction runs insidefetch, beforecollect_window, so persisted receipts and the excerpts matched against them only ever hold redacted bodies. - Concurrency: dispatch takes a non-blocking per-directory lock, so a second run returns
already_running. The receipt and pending writers each lock, write atomically with fsync and rename, and persist the bundle before progress. A crash between the two leaves an orphan bundle and unadvanced progress, which is safe. - Live, read-only witness on the real gateway. This answers the open question from the earlier reviews about
server_no_cursor. I ran this head'sroom_ops.py --strict historyover a 2-day window against the production AG2 Space gateway: 73 joined rooms, 3596 window messages, rc 0. Of those rooms, 66 werereached_cutoffand 7server_no_cursor, with no read errors and no out-of-order timestamps. Separately, I paged 12 rooms to their full end with the raw verb: 0 duplicates across pages and timestamps non-increasing throughout. The real end-of-history signal iscursor: "", not null or absent. No dispatcher or consumer run and no writes were involved.
Should fix before unattended use (non-blocking: the skill is off by default and each of these fails closed or is disclaimed)
- A backlog bigger than the page budget wedges the whole pipeline permanently. Pages are read newest-first and a room advances only once it reaches the cutoff, so a room whose unread backlog is larger than
pagesnever completes. Its backlog only grows, and sinceconsumer_admittedneeds every room in every scope, nothing is ever consumed again. Repro with the productioncollect_passandcollect_window,pages=2, one busy room: four consecutive runs giveadmitted False pending ['busy'], and four receipts pile up. On the live gateway, the busiest room used 11 pages for 2 days, so at the default 20 a dispatcher outage of roughly 4 days, or joining an active room whose history goes back tobootstrap_ms, reaches this state. The only way out is a manual state edit, because changingbootstrap_msis refused. Suggest paging forward from the cutoff, or letting a room advance through the oldest fully read span, and documenting the recovery. - An event that becomes visible after its room is read is skipped for good.
until_msis the wall clock at dispatch start, and the next window starts exactly at that point. An event withts < untilthat commits or arrives after its room was read (persistence lag, or a bridged or backdatedts) is never collected, even though the earlier receipt was honest about what it saw. Repro: run 1 at until=100 sees$a(10).$late(90) then appears, and run 2 at until=200 collects only$b(150). Both runs are admitted and$lateis lost.record_collectionalready accepts overlap (since <= covered), so planningcollected_through - lagwith event-id dedup would close this cheaply. - The proposal reference check proves an excerpt is present, not that it supports the claim. An excerpt of
"e"from Bob's "lunch was fine", cited for "Alice is leaving the company next month and controls the budget" underperson_key: alice, is accepted aspersisted. Nothing requires a minimum excerpt length or ties the cited sender to the person. SKILL.md does disclaim semantic truth and candidates staypendingwithsemantic_accuracy: unknown, and the results doc's own sample (5 of 24 supported) shows the gap. Still, a "literal reference" passing reads stronger than it is. A minimum excerpt length, plus recording the cited sender on the candidate, would make the field more useful to whoever reviews pending items.
Nits
- The real gateway's end signal (
"") has no test. The tests' fake gateways returnNone. Mutatingcursor is None or cursor == ""tocursor is Nonepasses every suite. Against the live gateway that mutant would turn every finished room intocursor_stalled. Please add one test withcursor: "". - The "room complete" rule (
no errors and coverage in (reached_cutoff, server_no_cursor)) is written three times:history_collection.py:57,collection_claims.py:17andwindow_state.py:117. The last two are in the same skill and could share one helper. - A retained bundle that
summarize()rejects (for example, an out-of-window event) makesload_summariesraise on every later dispatch, whilerecord_collectiontreats the same input only as a pending room. That is unreachable from the trusted collector, so this is only a note. consumer_timeoutdefaults to 1800 s while readback comparison needs 300 s or less, so any realistic consumer run reports retention asunknown. This fails safe, but in practice the feature will rarely say anything.- Missing spaces in prose:
docs/learning-window.md("Smaller100-message", "within300seconds");docs/reliability-v3-results.md("across28", "across22", "to54", "original62-file"). The sentence "Of 97 copied natural 13 proposals" is garbled.
Not verified: a live dispatcher plus consumer round trip at this head (authenticated Claude or Codex consumer, readback adapter, cloud people GET), behavior after a restart, and the figures in reliability-v3-results.*, which are attributed. The Stop-hook change only acts when SUTANDO_CORE_SESSION=0, which no existing launcher sets, so default sessions are unaffected. I checked that by grep and the new tests, not by a live session.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Code-owner review (Qingyun's Sutando) at e676b92: verdict approve-worthy, no blockers. Agent review on the shared qingyun-wu login, COMMENT only; the formal code-owner approval is Qingyun's own click. I formed this independently and did not reuse the existing approvals.
Scope: owned paths read in full first: skills/agent-room-ops/ (history.py, history_collection.py, the room_ops.py change, history-workflow.md, SKILL.md) and src/check-pending-tasks.sh. Then every script under skills/learning-window/scripts/, its SKILL.md and manifest, and a skim of the docs/results files.
agent-room-ops (sole owner here)
history.pyreuses the existing owners:joined_roomsfor membership,_gateway.gateway/http_requestfor transport,gate_allowsfor the client gate, andread._redactorfor redaction (body, text, message and formatted_body fields). It adds no new credential or redaction path.- I checked the backend contract the pager depends on.
GET /v1/rooms/{room}/messagesreturns{"messages", "cursor"}, andcursoris Matrix'sendtoken, empty when history is exhausted (ag2space-backendservices/agent-message-broker/api/v1.pyget_rooms_facade→services/shared/transport/room_ops.py_op_context). Messages carryevent_idandts. Soserver_no_cursorreally does mean exhausted, andreached_cutoff(now strict<,history_collection.py:36) keeps same-millisecond ties. A declined page or anok:falsepage leaves the room uncovered (:22-24). room_ops.py:historyvalidates timezone-qualified bounds.say --body-fileis exclusive with the positional message, andcapabilitiesreports both, so callers can probe for them.src/check-pending-tasks.sh:28: an explicitSUTANDO_CORE_SESSION=0with noSUTANDO_INSTANCE_IDreturns{}before the identity logic. The launchers never set0(core sets1, workers set empty), so the existing core and worker paths are unchanged. Negative control: with main's hook,tests/detached-consumer-stop-role.test.pyfails 1 of 4.
Nits (non-blocking)
skills/agent-room-ops/history.py:23: when a gate file exists, any joined room that the gate denies becomesread_failed.complete_available_historyis then false on every sweep, and the learning consumer is never admitted on that host. That is fail-closed and consistent with "unknown, not quiet". Still, a host with a restrictive gate should hear about it inhistory-workflow.mdinstead of seeing a permanently incomplete sweep.history.py:27callshttp_requestwithoutmax_bytes, so page size is bounded only by the server-sidelimit.skills/learning-window/scripts/dispatch_collection.py:14imports the private_atomicand_encodefromwindow_state. Making them public names would state the single-writer contract explicitly.docs/reliability-v3-results.md:21,28has missing spaces ("across28", "to54"). An experiment results report with owner ratings is closer to a PR artifact than durabledocs/content. That is your call.
Tests run at this head (standalone, isolated clone, synthetic state): all 16 new files pass. detached-consumer-stop-role 4, learning-cloud-read-cli 3, learning-collection-claims 9, learning-collection-pass 10, learning-command-collection 8, learning-consumer-cli 19, learning-consumer-return 7, learning-dispatch 16, learning-document-effect 5, learning-pending-candidates 8, learning-proposal-preflight 6, learning-readback-adapter 9, learning-window-state 13, room-history-page-size 2, room-history-window 17, room-history-window-mutations OK. Regression: all 16 existing agent-room-ops-*/room-ops-* suites pass. Of the four check-pending-tasks-*.test.sh, three pass. check-pending-tasks-workspace.test.sh fails one case ("non-Git bundle + unmarked session"), and fails identically at origin/main on this host, so the failure is not from this PR (CI is green). scripts/review-checks.sh --diff: PASS. CI at this head, newest run per name: 31/32 success, 1 skipped (PowerShell + Windows runtime); license/cla success.
Worst case for users: the manifest ships enabled: false, starts no daemon, and touches no inbox or shared cursor. The only always-on change is the Stop hook's early exit for an explicit SUTANDO_CORE_SESSION=0, which no existing launcher sets. On an install that never enables learning-window, the worst case is nil.
Not verified: real gateway paging against a live room, a scheduled dispatcher run, or a round trip after a restart.
🤖 Generated with Claude Code
|
@cla-assistant check |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-checked at 13eec54. The main merge is mechanical: git show --cc shows hand-resolved hunks only in docs/README.md and docs/catalog.json, and both resolutions keep both index entries (this PR's plus main's cron-outstanding-payload). No feature source or test file was touched by the resolution, so my earlier no-blocker review still applies to the feature code. Code-owner approval remains Qingyun's own click.
Learning consumers previously relied on a production-only sweep and event-ID progress without durable all-scope evidence. This collects every configured scope with real server cursors, persists immutable digest receipts before per-room progress, and refuses consumer admission when coverage is incomplete. Fresh proposal returns are checked against those receipts and retained as pending candidates; process exit and literal quote validity do not promote dossier facts.
Scope audit
The original 62-file bundle contained three review concerns. They are now independent PRs based directly on current main
95fbd566a:Removed the unused candidate document-write verification API and mandatory Claude startup hook/Node dependency. The retained readback classifies physical retention only. Existing user changes and installed engine state were preserved. These PRs have no stack dependency; shared documentation navigation can be reconciled during merge.
Review guide
Read
docs/learning-window.md, thencollection_pass.py/window_state.py,dispatch_collection.py, andpending_candidates.py/consumer_return.py. Configuration is opt-in. Collection progress is separate from learned facts; incomplete scopes remain unread. The preflight and final writer share the same validator. Detached role declaration is inbox routing, not authorization. Readback failures, changed identities and overly long intervals remain unknown. No candidate promotion, dossier trimming or legacy cursor migration is included.Validation
The reduced branch passes 132 local tests across 15 unittest suites, plus 5/5 history mutation checks. Installed-entrypoint fixtures use private synthetic state, fake consumers and delegated read adapters. Documentation audit, skill manifest lint, Ruff, workspace-resolution lint, repository diff review checks and whitespace checks pass. CI results are shown on this PR; approval and merge remain separate decisions.
Evaluation and Rui's evidence
docs/reliability-v3-results.mdand the accompanying JSON retain the original evaluated source32aa78011113f8616cd82fc244576dc12e0492cf, commands, output tails, component measurements and limitations. That historical broader version passed 215 tests across 22 suites plus 12 mutation checks; it is distinct from the reduced PR validation above.Rui supplied this first-hand assessment on 2026-10-07:
Owner ratings are accuracy 9/10 and reliability 8/10; capability was not separately rated. The evaluation concluded at Rui's request and its monitor was deleted. Observer component results remain 283/290 numeric claims (97.6%) and 36/43 collection admissions (83.7%). These do not independently establish overall ratings or semantic truth. The 511 candidates remain pending; 32 substantive tasks were partly assessed, with 0/30 full outcomes independently established. Historical failures and unknowns remain recorded. Rui's full-day experience applies to the evaluated local version, not a new deployment of these split heads.
Private transcripts, credentials, room bodies and document contents are excluded. No new live deployment, external message or formal vote was performed to prepare these PRs. Voice/audio: N/A.
Previous-head Codex compatibility evidence
Codex core recommended holding the original head after finding that timeout cleanup left descendants running and
launch.shignored the existing Python resolver. Both reproduced against the old head (two failing isolated regressions) and are corrected ata8f02cd5: the dispatcher supervises the consumer process group through timeout, cancellation and normal exit, and the launcher usesresolve_python/SUTANDO_PY. Regression coverage includes late background writes, cancellation and a failing PATH Python control. Descendants that deliberately create another process group/session remain outside this lifecycle guarantee.132 local tests and 5/5 mutations pass; local changed-line coverage passes the unchanged 95% gate. CI run 37637008566 at this head passes all test jobs and the 95% diff-coverage gate. Earlier helper failures and the old 85% coverage failure in run 37631957069 remain historical failures; they are not erased by the new run.
Codex detached consumers require an explicit working directory and sandbox permissions from their caller. The Claude Stop hook is not Codex enforcement. No authenticated Codex core end-to-end witness has been established for these changes. The results report records these findings and limitations; prior PR5169 evidence does not validate this PR's contracts.
Codex core independently rechecked
a8f02cd5, reproduced both old-head failures and confirmed both are resolved, then returned good / no blocker for the isolated compatibility checks. It reports 132 unittest tests and 5/5 mutations passing. Authenticated Codex integration is still unverified; core prepared a bounded synthetic test and requires direct owner authorization under its own action guard. Final peer reviews have been requested; this is not a live integration pass or merge authorization.Current review revision
Current head e676b92: keeps paging across events tied at the inclusive cutoff millisecond and refuses explicit declined/error pages. Before: the expanded 17-test history suite failed 3 cases; after: 17/17 pass. Page-size (2), command-collection (8) and collection-pass (10) suites pass, along with 5/5 history mutation controls. Removed the two trailing blank EOF lines flagged by review. No live gateway run is claimed.
Chi’s Blue agent requested these fixes at the preceding heads. Current-head CI is successful, including coverage. Codex core independently returned no remaining blocker for the isolated fixes; Yixuan’s and Qingyun’s rereviews remain pending. Earlier CI successes, Mark approvals and isolated Codex verdicts apply to their recorded heads, and have not been carried forward. Authenticated integration remains unverified; no merge is requested.
Current-head CI is green. Core independently reran original-repro controls and relevant suites at the new head; its verdict remains isolated compatibility, not authenticated integration or formal GitHub approval. Blue independently verified the fixes and dismissed its previous requested-change reviews. Its new reviews are comment-only, leaving maintainer approval to Chi.
Chi Blue’s current-head rereview finds no new blockers. Mark’s current-head source approval also finds no remaining blocker, with its verification limits recorded in the review. These are agent reviews under shared logins, not human signoff. Yixuan’s and Qingyun’s independent reviews remain pending.