feat: interactive trajectory browser with attempt merge and split - #370
Conversation
Attempt grouping moves out of the append-only span logs into a mutable sidecar (attempts.json): an attempt id equals the trace id unless a definition maps a minted att-* id to its member traces plus the historical ids it absorbed (aliases). merge_attempts creates a definition and split_attempt deletes one, so grouping stays editable while span logs are never rewritten. - Validation: per-entry schema checks drop bad entries; definition ids, aliases, and member traces must form three globally disjoint address sets or the whole file reads empty. Merge validates its inputs and the constructed state with the same checks before writing anything, and refuses cross-session members, unknown or empty ids, unsafe legacy ids, and single-group calls. - Pin safety: member pins migrate up on merge and down on split inside the attempts lock (attempts before pins lock order), with compare-and-delete cleanup, commit-failure rollback, and best-effort no-throw behavior after the commit point. New pin_attempt resolves the current owner under the attempts lock so pinning an attempt address cannot race a concurrent merge or split into a dangling pin; the pin CLI and the bundle auto-pin use it (the bundle falls back to pinning the packed traces if the address vanished mid-pack). - Read paths: resolve_attempt_id, iter_spans, is_pinned, and unpin_attempt prefer definitions (alias and member aware) and keep the legacy attempt.id fallback for old logs; legacy groups can merge but not split. read_verdicts gains a mutually exclusive attempt_ids filter so verdicts recorded under absorbed or member ids stay visible; bundles collect them and raise LookupError when all members were purged. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
attempts.json is now the sole source of attempt grouping; the span-level mechanism (introduced by #362, zero production callers, never released) is removed: - build_span drops the attempt_id parameter and no longer writes the attempt.id attribute; attempt.id is a reserved key stripped from caller-supplied attributes so it cannot be injected. - TraceCtx loses its attempt field; the begin_attempt / end_attempt / current_attempt trio and the zero-caller turn_scope helper are deleted. - Read paths keep resolving the legacy span-level attempt.id attribute in old logs. - Docs and CONTEXT.md describe the final model: at read time an attempt id equals the trace id unless an attempts.json definition groups several traces under one minted id. - Test helpers default to the new span format (explicit attempt_id= still builds legacy fixtures); real-tracer E2E tests address attempts by trace id or via merge_attempts. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
CLI adaptation for attempt definitions (attempts.json is the sole source of attempt grouping since the write-side mechanism was removed): - list folds definition members into one row (grouping precedence: definition owner > legacy attempt.id > traceId, so merged legacy members do not surface twice) and picks the verdict through the alias set (definition id, absorbed aliases, member traces; latest file order wins). Definitions are read once up front, never per span. - new merge subcommand: thin wrapper over merge_attempts; data-layer ValueError (unknown id, cross-session, too few distinct groups) exits 1. Success prints only the new definition id - no re-read of the mutable definition after commit. - new split subcommand: None (pure legacy or unknown id) exits 1 with a neutral message; success reports restored member trace count (never an attempt count) without claiming the addressed input was the deleted definition. - unpin goes through unpin_attempt (clears definition id, absorbed aliases, member-level pins, and the literal id in one transaction). - every dynamic value rendered through Rich (user input, resolved or manifest ids, session keys, verdict text, exception messages, paths, table cells) is escaped first: ids like "x[/red]y" are legal and used to crash markup parsing. The empty-list notice escapes the --session filter too. - list tolerates malformed span attributes: non-string attempt.id falls back to the trace id, non-string session keys and timestamps degrade per-field instead of killing the listing. Tests: 37 -> 60 cases in test_cli_trajectory_commands.py (merge/split round trip, legacy expansion, alias verdict surfacing, member-level unpin, markup safety across success/error/list paths, malformed attribute tolerance). Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
A JSON-legal but type-broken record (hand-edited or corrupted history) could crash whole commands instead of degrading per record/field. Now: - span and verdict logs decode per line (read_bytes + per-line UTF-8): one invalid byte hides one record, not every record in the file. - read_verdicts validates required fields (attempt_id/status/source/ts must be non-empty strings) before constructing a Verdict: a list attempt_id used to blow up the set lookup inside read_verdicts (bundle path) and the dict keys in list's verdict column. - non-string traceId/attempt.id/session.key degrade to absent via a shared _str_value helper across _expand_group, merge's session scan, is_pinned, resolve_attempt_id, and all three iter_spans filter paths (the definition-member set lookup crashed on unhashable ids even when the bad record was unrelated to the target). - a truthy non-object attributes container degrades to no attributes (shared _span_attrs; also the bundle collector and the list command). - minimize reads the manifest through one controlled path: broken JSON, invalid UTF-8, or a non-object top level exit 1 with a readable error; a non-string attempt_id falls back to the bundle directory name before the default-path join. - the empty-list notice escapes the --session filter (markup-bearing input crashed it); table cells escape str() of unvalidated values. Regression tests added across the store, bundle, and CLI suites for each crash shape (58 -> 68 CLI cases; store and bundle suites extended). Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
A bare `raven trajectory` on a TTY now opens a three-screen questionary flow (sessions -> attempts -> actions: save / report / minimize / verdict / pin|unpin / split, plus multi-select merge), completing the trajectory feature: definitions (data layer), CLI merge/split, and now the human face. Menus, labels, and action messages never show a session key, trace id, or attempt id - only artifact paths carry ids. raven/cli/trajectory_browse.py (new): the browser module. - Control flow: every prompt cancel unwinds through one internal exception to a clean exit; once an action runs - normally, refused, or failing controlled - the browser rescans everything (report bundles and auto-pins before confirming, so even an aborted action changed state). typer.Exit is swallowed silently (the controlled abort already printed); data-layer errors surface as one fixed id-free message with the original logged at debug level. - Sentinel protocol: split success wording requires the member tuple (None reads as a stale race), unpin requires True. - Aggregation: one snapshot per refresh (definitions, pins, verdicts read exactly once; no per-row store reads), logical-span dedup keyed by (traceId, spanId) so a checkpoint+final pair counts once, the list command's grouping precedence, per-field degradation for malformed records, and session titles from metadata with a channel-attribute (or key-prefix) fallback, all normalized to one plain-text menu line (questionary renders no Rich markup). - The chosen workspace propagates to every packing path (save, report, minimize), and RAVEN_STYLE loads only after questionary resolves. raven/cli/trajectory_commands.py: report workflow extracted into _report_attempt with injectable confirm and progress note (the browser passes an id-free variant; errors propagate to each frontend's own boundary); _default_cassette_dir shared with minimize; the bare-group callback gates on subcommand first, then TTY (exit 2), then locally imports the browser so the module cycle breaks and questionary stays lazy. Tests: tests/test_cli_trajectory_browse.py (new, 49 cases) covers id-free labels, fallback/degradation matrices, logical-span dedup, snapshot discipline (spies plus forbidden-call sentinels), full merge -> verdict -> save -> split cycle asserting each menu reflects rescanned state, failure and sentinel paths for every action, empty states, a cancellation matrix, prompt chrome (QMARK/POINTER, checkbox validate), artifact-path-only id output, and dependency/import smoke via a subprocess. test_cli_trajectory_commands.py adds the bare-call TTY gate cases. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the released tracing attempt API is removed without a compatibility path; see the inline note.
I reviewed the full diff, surrounding callers, commit and release history, backward compatibility, AGENTS.md/CLAUDE.md, CONTEXT-MAP.md and CONTEXT.md terminology, and the changed tests. I found no evidence that tests were weakened merely to obtain a green result.
Verification: uv run pytest tests/test_cli_trajectory_browse.py tests/test_cli_trajectory_commands.py tests/test_trajectory_store.py tests/test_trajectory_bundle.py tests/test_tracing_api.py -q completed all assertions with 241 passed, then the Python 3.13.2 process exited 139 during interpreter shutdown. The same command's implicated file on github/main completed 36 passed and then exited 139, so I treated that shutdown crash as pre-existing rather than a finding on this PR.
| return _ctx.current() | ||
|
|
||
|
|
||
| def begin_attempt(session_key: str, attempt_id: str | None = None) -> str: |
There was a problem hiding this comment.
v0.1.13 already published trace.begin_attempt, trace.end_attempt, and trace.current_attempt: commit 0d2f2c25 is an ancestor of the v0.1.13 tag, and that tag's CONTEXT.md documents trace.begin_attempt(session_key). Removing these functions means an integration that works on v0.1.13 now raises AttributeError immediately after upgrading; the associated removal of Span.attempt_id and emitted attempt.id also changes the released tracing contract. Please preserve a compatibility/deprecation path or handle this explicitly as a versioned breaking change instead of relying on the PR's never released premise.
There was a problem hiding this comment.
Confirmed - you are right, and the PR's premise was wrong. Verified locally: 0d2f2c25 is an ancestor of the v0.1.13 tag, that tag ships all three functions in raven/tracing/trace.py, and its CONTEXT.md documents trace.begin_attempt(session_key). The "never released" claim only checked in-repo callers, not the release history.
We are handling this explicitly as a versioned breaking change rather than a compatibility shim, for one main reason: the old semantics cannot actually be preserved. New spans no longer carry attempt.id (that is the point of this PR), so a shim could not make begin_attempt's returned id address anything - a single-turn attempt cannot even be represented (definitions require >= 2 member traces), and a near-miss shim would leave callers' verdicts/pins silently dangling on ids that resolve to nothing. An immediate AttributeError with a clear migration path (group after recording via merge_attempts() or raven trajectory merge) is diagnosable in minutes; the silent variant is not.
The PR description is updated accordingly: the wrong "unreleased" wording is corrected in Summary/Risk/Related Issues, and a BREAKING CHANGE: footer now declares the removal with the migration path (the squash commit body inherits it). Logs already carrying span-level attempt.id remain addressable, mergeable, and listable through the reader fallback - no data migration.
There was a problem hiding this comment.
Accepted. This resolves my finding: the PR now accurately identifies the v0.1.13 surface as released, explicitly treats its removal as breaking, provides the concrete post-recording migration, and places the BREAKING CHANGE: footer in the squash body. I agree that a compatibility shim which returns an unresolvable id would be worse than the explicit versioned break. No blocker remains from this thread.
Verification rerun: the relevant suite completed 241 assertions successfully, then the Python 3.13.2 process exited 139 during the same pre-existing interpreter-shutdown crash already reproduced on github/main.
There was a problem hiding this comment.
Verification rerun: the relevant suite completed 241 assertions successfully, then the Python 3.13.2 process exited 139 during the same pre-existing interpreter-shutdown crash already reproduced on github/main.
so my work is done and no more blocking issues, correct?
There was a problem hiding this comment.
Correct. As far as my review is concerned, the versioned-breaking-change documentation and migration path resolve the only blocker I raised, and no blocking issue remains on this unchanged revision.
I reran the relevant suite at 1cb24a4b8dba: all 241 tests passed, after which Python 3.13.2 exited 139 in the interpreter-shutdown crash already reproduced on github/main; that baseline-only exit does not block this PR.
3 participants
Summary
raven trajectoryhad eight id-oriented subcommands; using them meantcopying ids out of
listby hand. This PR changes the attempt data modeland adds the human face, in four phases:
attempts.jsonsidecar): an attempt id equalsthe trace id unless a definition groups several traces under one minted
id. Merge creates a definition (absorbing prior definitions and legacy
groups as aliases, so verdicts/pins recorded under old ids stay
visible); split deletes it, migrating pins down to members - naturally
undoable, span logs stay append-only. Pin migration is linearized under
a fixed lock order (attempts > pins); every failure and concurrency
interleaving over-protects, never under-protects. Legacy logs
(span-level
attempt.id) stay addressable and mergeable, but cannot besplit (their grouping lives in append-only spans).
begin/end/current_attempt trio and the
attempt.idspan attribute,shipped in v0.1.13) is deleted;
attempt.idis now a reservedattribute key stripped from caller input, so
attempts.jsonis thesole grouping source by mechanism. Readers keep resolving legacy logs;
zero data migration. This is a breaking API change, declared in the
BREAKING CHANGE footer below.
listfolds definitionmembers into one row and surfaces verdicts through the alias set; new
merge/splitsubcommands wrap the data layer thinly;unpinclearsdefinition, aliases, members, and the literal id in one transaction.
Two whole crash surfaces are closed along the way: Rich markup
injection (a legal id may contain
[/red]; every dynamic value isescaped), and JSON-legal but type-broken records (per-line UTF-8
decoding, required-field string validation, non-object attribute
containers) now degrade per record/field instead of killing commands.
raven trajectoryon a TTY opens athree-screen questionary flow (sessions -> attempts -> actions: save /
report / minimize / verdict / pin|unpin / split, plus multi-select
merge). Menus and action messages never show a session key, trace id,
or attempt id - only artifact paths carry ids. Every action triggers a
full rescan (report bundles and auto-pins before confirming, so even an
aborted action changed state); data-layer errors surface as one fixed
id-free message; aggregation reads one snapshot per refresh and
deduplicates records into logical spans keyed by (traceId, spanId), so
a root turn's checkpoint+final pair counts once.
CONTEXT.md gains the Attempt Definition term and the final-state Attempt
wording.
Type
Verification
uv run pytest tests/test_cli_trajectory_browse.py tests/test_cli_trajectory_commands.py tests/test_trajectory_store.py tests/test_trajectory_bundle.py tests/test_tracing_api.py -q-> 241 passeduv run pytest tests/ -q --ignore=tests/integration-> 6838 passed; the 11 failures + 20 errors match main's pre-existing environment-specific set node-id for node-id (cron timezone cases, everos/config root-permission cases, viewer probe, theme), compared as sorted FAILED/ERROR sets against a baseline recorded before this branchuv run ruff check .anduv run ruff format --check .-> cleanRelevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed (CONTEXT.md terms)
Risk
Bare
raven trajectorychanges behavior: it opens the browser on a TTY(previously the help page) and exits 2 with a hint when non-interactive.
Scripts are unaffected: the subcommand check runs before the TTY gate.
New spans no longer carry
attempt.id, andtrace.begin_attempt/trace.end_attempt/trace.current_attemptare removed. These shippedin v0.1.13, so an integration calling them raises AttributeError right
after upgrading - an explicit, immediately visible failure rather than a
silent one. Migration: group attempts after recording with
merge_attempts()(library) orraven trajectory merge(CLI); readerfallback keeps existing logs addressable with zero data migration.
attempts.jsonis the only new mutable state; deleting it reverts everyattempt to single-turn semantics. Span logs remain append-only.
Rollback: revert the squash commit; no data migration either way. The one
known crash window (process death between merge's two file writes) leaves
a harmless over-protective pin, documented in the store module.
Security impact considered (markup-injection and path-escape surfaces
closed; ids treated as untrusted in every renderer)
Backward compatibility considered (legacy span-attribute logs stay
addressable, mergeable, and listable)
Rollback path is clear for risky changes
Related Issues
#362 (reference only: this PR removes the write-side attempt mechanism
introduced there and shipped in v0.1.13). No issue is closed.
BREAKING CHANGE: trace.begin_attempt, trace.end_attempt, and
trace.current_attempt (shipped in v0.1.13) and the span-level attempt.id
attribute are removed. Group attempts after recording instead:
merge_attempts() in raven.trajectory, or
raven trajectory mergein theCLI. Existing logs carrying a span-level attempt.id stay addressable
through the reader fallback; no data migration is needed.