Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the skycap check command to verify if recorded trajectories remained linear and to identify the causes of any forks. The implementation includes the core verification logic, CLI integration, documentation, and comprehensive tests. The review feedback provides valuable improvements for the new check.py module, including optimizing fork detection using branch points, preventing potential KeyErrors when parsing document metadata, optimizing sibling comparison to avoid redundant string allocations, and correcting the grammatical logic in the _a helper function.
|
|
Hey @kouroshHakha, thanks again for reviewing #2425! This adds |
…nd why each fork happened Reads a record directory's documents (no sidecars) and, per trajectory, reports the path count and a cause for each fork: resample, edited reply, re-rendered (token mode), different message, or tools or model. Exits 0 if every trajectory is linear, 1 if any forks, 2 if a record can't be read. record.load's graph rebuild moves into record.add_nodes so check shares it.
…t non-object records Forks are found under the roots and graph.branch_points() rather than every node. Siblings are indexed by match hash, message, and the latest (model) sibling per role, so only a client edit compares against every earlier sibling, by a change count rather than the formatted list: 5,000 replies under one node take 0.05s. A record whose document isn't a JSON object is reported unreadable, named by its file, instead of stopping the run.
check normalizes each message with MatchKey.fields instead of its own copy of it. test_check builds on test_graph's Harness and message helpers rather than repeating them; test_graph's Harness.call takes `bridged` for it.
…ad of an index _Siblings kept five indexes to find the sibling a fork is compared with, and _count_changes repeated _changes to count without formatting. One ranked min() over the earlier siblings picks the same sibling in every test. It is as fast up to hundreds of siblings under one node (0.07s at 500); only thousands of model replies under one node are slower (7s at 5,000, from 0.09s). Client edits were already quadratic under the index (15s at 5,000).
…unreadable, not fatal A document that is a JSON object but whose capture is a list raised AttributeError, which stopped the whole run instead of reporting that one record. It is now a read error like the others.
…st that already builds it
|
@greptile re-review pls - should have fixed the malformed record crash |
|
Thanks for this, and for the careful write-up (the sibling-index benchmark especially). We currently have a lot of duplicate work in skycap-viewer and I think the right place for this inspection is actually there instead of adding it to skycap:
|
Summary
skycap check <record_dir> [trajectory_ids...]reports whether each recorded trajectory stayed on one path, and why each fork happened, so a harness whose history should be append-only can be checked against a recorded run.resample(another reply to the same history),edited reply(a model reply sent back with fields changed, e.g. stripped reasoning),re-rendered(token mode: the same message rendered to different tokens),different message(compaction, subagent, rewritten turn),tools or model(the same message with another tool set or model). In token mode it also counts unbridged calls.record.loadintorecord.add_nodes, whichcheckreuses.checknormalizes messages withMatchKey.fields, andtest_checkbuilds ontest_graph'sHarnessand message helpers.How a fork's cause is found
The graph is walked only at its fork points (
graph.roots()andgraph.branch_points()). For each child after the first,checkpicks the earlier sibling the request should have matched and diffs the two. The pick is one rankedmin()over the earlier siblings: same match hash, then same message, then same role (for a harness-written node, the fewest changed fields), then a model reply over a harness-written one, then the latest.The graph can't answer this pick by itself: a fork exists only because the request's message matched no child's hash, so after the hash and the message, content is the only signal left. Following the call history instead would be wrong for the main case. Resample A, resample B, then send back A with its reasoning stripped: the latest call is B, but the edit came from A, and only a content diff finds A.
Why not an index of siblings
An earlier revision kept five indexes (by match hash, message, role, model reply per role, latest) to make the pick O(1) for most nodes. It was removed because it didn't earn its code:
min()min()n > 1(one call is one sample) and a trajectory is one agent run, so that many separate calls on the same history isn't a real workload, and identical replies already dedupe into one node.checkruns offline, after the run, never on the serving path.If very wide forks ever show up, caching the latest model reply per role brings back most of the speed without the other four indexes.
Review comments
_changesdirectly._agrammar. Done ("an unnamed", "a user")..get("id")/.get("status"). Left as is:check_dircatches theKeyErrorand reports the record as unreadable under its file name, the same contractrecord.loadhas.capturea list) also raisedAttributeErrorand stopped the run; it is now a read error too. Both are covered bytest_an_unreadable_record_is_reported_and_exits_2.Testing
cd skycap && uv run pytest: 245 passed, 1 skipped (on the branch merged with currentmain)pre-commit run --files $(git ls-files skycap): ruff, black, gitleaks cleanNote
Low Risk
Offline read-only validation on record JSON;
record.loadbehavior is preserved viaadd_nodesrefactor with no serving-path changes.Overview
Adds
skycap check, a CLI that scans a--record-dirand reports whether each trajectory stayed on a single root-to-leaf path or forked, with a labeled cause per fork (resample,edited reply,re-rendered,different message,tools or model). It reads record documents only (no token sidecars), rebuilds the message graph via new sharedrecord.add_nodes, walks branch points to compare each extra sibling against the best-matching earlier one, and prints a per-trajectory summary plus aggregate fork counts. In token mode it also reports unbridged calls. Exit codes: 0 all linear, 1 any fork, 2 unreadable record or usage error; optional trajectory id filters are supported. README documents the workflow;test_checkcovers fork kinds, token-mode behavior, CLI integration, and malformed records.Reviewed by Cursor Bugbot for commit b16fc84. Bugbot is set up for automated code reviews on this repo. Configure here.