feat(trajectory): tester-facing bug report flow in raven trajectory - #380
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned, with two nonblocking correctness notes below.
I covered the repository rules in AGENTS.md/CLAUDE.md and the canonical trajectory terms in CONTEXT-MAP.md/CONTEXT.md; the complete target diff; the affected CLI, store, replay, pricing, sanitization, packaging, recovery, and report-list callers; relevant history; backward compatibility; and the changed tests for weakening (none found). I also checked the export/privacy, frozen-snapshot, state-machine, retry, offline-replay, and no-real-tool architecture constraints.
Verification: uv run pytest tests/test_trajectory_bugreport.py tests/test_trajectory_completeness.py tests/test_cli_trajectory_browse.py tests/test_cli_trajectory_commands.py tests/test_provider_rates.py tests/test_token_wise_pricing.py tests/test_trajectory_replay.py reported 447 passed in 16.02s, then the process exited 139 during teardown. To refute this as a branch regression, I ran the unchanged tests/test_trajectory_replay.py at the exact base commit 53532eb740d8; it likewise reported 31 passed and then exited 139, so the teardown crash is reproducible on the target baseline.
| """The required description plus the optional detail fields (design 1.2/1.3).""" | ||
| while True: | ||
| description = _ask_action(questionary.text("Describe the problem (required):", style=style, qmark=QMARK)) | ||
| if description: |
There was a problem hiding this comment.
Nonblocking: The required-description loop accepts any truthy string, so an entry containing only spaces proceeds through freeze_export and can land a report whose required description is visually blank. The scriptable command already strips before validating; the interactive path should apply the same empty-after-trimming rule.
There was a problem hiding this comment.
Fixed in 371a4e4: the interactive prompt (and freeze_export itself) now trims before the emptiness check, so a spaces-only entry reprompts; the existing reprompt test gained a spaces-only step and a data-layer test asserts freeze_export rejects whitespace-only descriptions.
There was a problem hiding this comment.
Confirmed on 371a4e4: both the interactive prompt and the data-layer guard trim before validation, and the regression tests cover spaces-only input. This resolves the finding.
| if not root.is_dir(): | ||
| return [] | ||
| out: list[tuple[Path, dict[str, Any]]] = [] | ||
| for entry in sorted(root.iterdir(), reverse=True): |
There was a problem hiding this comment.
Nonblocking: new_report_id contains only the UTC date plus a random suffix, so reverse lexicographic directory order is random among reports filed on the same day. That makes the documented/UI newest first ordering wrong in the common case of multiple same-day reports; order the validated records by created_at (with a deterministic tie-breaker) instead.
There was a problem hiding this comment.
Fixed in 371a4e4: list_reports now sorts by the recorded created_at with the report id as a deterministic tie-breaker, instead of directory-name order; covered by a test with two same-day reports whose lexicographic id order contradicts creation order.
There was a problem hiding this comment.
Confirmed on 371a4e4: list_reports orders validated records by created_at with report_id as a deterministic tie-breaker, and the contradictory same-day IDs test covers the original failure. This resolves the finding.
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this revision can merge as far as I am concerned, with the two previously posted nonblocking notes still open.
The new delta is test-only: it splits the fake PEM marker in three fixtures so the source scanner does not mistake it for a real key. I verified all three expressions still produce the exact original runtime value, and the affected blocked-classification tests still exercise that value. No production behavior, public API, package format, or architecture boundary changed. I rechecked the full target diff status and the delta against AGENTS.md/CLAUDE.md, the trajectory domain terms, affected test callers/history, backward compatibility, and test weakening; the new comment explains the necessary hook constraint in English.
Verification: uv run pre-commit run detect-private-key --files tests/test_cli_trajectory_browse.py tests/test_cli_trajectory_commands.py tests/test_trajectory_bugreport.py passed. uv run pytest tests/test_cli_trajectory_browse.py tests/test_cli_trajectory_commands.py tests/test_trajectory_bugreport.py reported 298 passed in 6.92s, then exited 139 during process teardown. This is the same teardown failure already reproduced at the exact target base in the prior review, not a new failure from this test-only delta.
…tion time Two review notes from PR #380: - the interactive description prompt (and freeze_export itself) now trims before the emptiness check, so a spaces-only entry reprompts instead of landing a report whose required description looks blank - matching the scriptable command's validation - list_reports orders by the recorded created_at (report id as the deterministic tie-breaker) instead of directory-name order, which was random among same-day reports since the id carries only the UTC date Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this revision can merge as far as I am concerned.
Both previously reported nonblocking issues are resolved: whitespace-only descriptions are rejected at the interactive and data-layer boundaries, and reports are ordered by recorded creation time with a deterministic report-id tie-breaker. I found no new issues.
I reviewed the 371a4e4 fix delta in the context of the full target-branch diff, its affected interactive and scriptable callers, report-list consumers, and relevant history. I also rechecked AGENTS.md/CLAUDE.md and the context map, backward compatibility, architecture boundaries, and that the regression tests strengthen rather than weaken existing coverage. git diff --check 08509fc1b1a9..HEAD passed. The three focused regressions passed (3 passed). The broader affected run completed all assertions (300 passed) and then exited 139 during pytest teardown, matching the pre-existing environment teardown crash established on the target baseline in earlier rounds.
|
The automatic-shrink assumption no longer holds now that #379 was squash-merged. The live |
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: this branch does not merge cleanly into current main after #379 was squash-merged; see the base-update note.
The automatic-shrink assumption is false for the retained pre-squash commit ancestry. The live comparison remains 23 commits ahead and 1 behind with 19 changed files, and git merge-tree --write-tree HEAD github/main reports conflicts in raven/cli/trajectory_browse.py and tests/test_cli_trajectory_browse.py. Under the fourth-round standard, this revision retains the ancestry that introduces the conflict relative to current main, every ordinary merge attempt reaches it, and the operator has no working merge path until the branch is updated.
I refreshed and reviewed github/main...HEAD, isolated the effective tree delta from the now-base-identical #379 UX changes, and checked the relevant history, callers, backward compatibility, tests for weakening, architecture boundaries, AGENTS.md/CLAUDE.md, and the context glossary. No new code-level findings emerged, and both earlier bug-report findings remain resolved. Both live-base diff checks passed.
Tests: the effective bug-report suites completed 302 assertions successfully, then the pytest process exited 139 during the previously established teardown crash. The UX/theme run reported 168 passed and 1 failed (test_bold_accent_renders_styled_not_bare); its source and test files are identical to current main, so that failure is pre-existing and out of scope.
Shared absolute-path parser (POSIX/Windows/UNC, URL-exempt, charset independent), text and tree sanitizers replacing paths with a [REDACTED:path] placeholder, the assertion scan that re-runs the same parser, and an order-stable tree digest that refuses symlinks. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Public resolver for the deduplicated member trace set of any attempt address shape: definitions (id/alias/member), legacy attempt.id groups, and bare single traces. attempt_members knows only definitions, so the bug report flow needs this to freeze legacy multi-trace attempts. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Bug Report Record (machine-local lifecycle json) and Bug Report Package (the only exportable artifact: canonical bugreport.json plus an embedded Trajectory Report). The whole deliverable is frozen under snapshot/export/ before confirmation - user fields go through the same redaction as the trajectory, both trees are path-sanitized and asserted clean, and packaging retries reuse the approved bytes verbatim after an export tree digest check. Persisted drafts recover to a retryable failed state (crash recovery). CONTEXT.md defines both new domain terms. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
New first action "Report a bug": snapshot + redaction with a pin disclosure, required one-line description with optional details, a frozen confirmation summary (merged redaction counts, PII note, no upload notice), needs_review second confirmation, blocked refusal for private-key hits on either side, and a "Bug reports (n)" entry to inspect and retry failed reports. Existing actions and wording are unchanged; report picks in older tests are disambiguated to the expert action. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Review fixes on the bug report pipeline: - normalize tar member headers (uid/gid/uname/gname) on both archive layers so no local username or group leaks through the package; the expert raven trajectory report command keeps its existing headers - verify export_digest immediately before and after every tar run (first packaging and retries share the check), so bytes the user never approved cannot land in a local_ready package - validate the full record structure on load (required fields, types, enums, digest shapes, report id grammar and directory match); damaged records are skipped by listings and can never turn into path operations outside the report directory - normalize expected filesystem/archive failures into PreparationError (pre-record, staging cleaned) or PackagingError with a retryable flag, so I/O errors show the designed failure blocks instead of crashing the browser Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The v1 record validator now pins each status to its legal field combination: local_ready requires a real package identity (non-empty path, 64-hex sha256, positive size), no kept snapshot, and no failure; draft/failed require empty package fields and a kept snapshot; failed requires a reason. Completeness status is limited to its enum, upload and links require their v1 subfields, member traces must be non-empty, unique, and ascending, and a landed record can be neither blocked nor unreviewed. Parametrized rejection tests cover each invariant plus a positive case per legal status. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Completeness of a bug report now describes the sanitized deliverable tree: explicit contract checks (load_recording, empty turns, the new replay.validate_recording payload validator kept next to the consuming code), one warn-mode mock run_replay probe whose fatal divergences map to unreplayable (covering control-flow exhaustion and AgentLoop empty- response recovery with zero drift), static-only defects (corrupt span lines, unsupported format version), and degraded reasons for missing comparison material including an unlocatable history cut via the new replay.diagnose_history_cut. Unrecognized probe failures degrade the evaluation to unknown instead of masquerading as evidence damage. Also: - sanitize: a missing artifact's absolute *.artifact_path becomes JSON null instead of a placeholder relative path, which replay's _artifact_path would reject - the delivered embedded trajectory now loads; the sanitized basename stays in manifest.missing_artifacts - environment summary (python, os major.minor, arch, format/schema versions, provider/channel section key names via raw JSON only) and completeness reasons go through the same known-value/pattern/residual redaction as user fields before entering the package - classify_redaction gains an explicit require_review policy signal (RAVEN_BUGREPORT_REQUIRE_REVIEW) that appends its reason unconditionally and never outranks blocked Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The confirmation summary renders the evaluated completeness status with indented reasons (same style as the needs_review block), and the bug report details screen shows the status with a reason count. Policy- driven needs_review runs through the existing second confirmation. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Review fixes on the completeness probe: - cost estimation reaches remote model catalogs (LiteLLM, OpenRouter); run_replay now suppresses pricing alongside tracing via a new pricing_suppressed() context, so a mock replay neither gates on the network nor leaks that it ran - a crash while consuming the recording inside ReplayProvider or ReplayToolRegistry becomes a fatal corrupt-recording divergence instead of an ordinary error reply the loop shrugs off, so an unanticipated payload shape can never end as complete - validate_recording covers the nested dereference paths the consumers actually take: input tools[].function(.name), message tool_calls entries and their function objects, output tool call id/name/ arguments, and thinking_blocks entries - _turn_span_count counts key presence, not truthiness - a sanitized (nulled) turn reference is exactly the loss it must surface; the generic missing-artifacts reason is settled by count against role-attributed gaps instead of a boolean that swallowed the list Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The vision-capability probe starts a background OpenRouter catalog warm that the pricing-only suppression missed. Pricing suppression is replaced by a single rates_offline() ContextVar context in raven.providers.rates, honored at every entry: token_rates answers None, warm_catalog_in_background returns before creating its thread (a ContextVar does not cross into workers), _fetch_openrouter_models delegates to the cache-only path, and vision_verdict skips the warm at the call site while keeping today's unknown verdict. run_replay enters the context alongside trace suppression; concurrent real turns keep warming and pricing normally, and the context does not leak on exit. The no-network tests now count recorded no-op calls instead of raising inside the warm's daemon thread (which swallows exceptions and made the previous assertions false positives), and the rates suite covers blocked warm/lookups inside the context, normal warm after it, and the cache-only fetch entry. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
…g cache estimate_cost_usd returns before the warning path while rates_offline() is active: an offline completeness probe otherwise logged the one-time unknown-model warning and consumed it from the process-global cache, stealing it from the next real turn. Regression test covers no warning and no cache mutation inside the context, and the normal single warning immediately after it. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The attempt table gains a 3-wide RPT column after MERGED, checked for any attempt whose frozen bug report association matches (recorded attempt id or member-trace overlap). Records are read once per snapshot scan - which also runs the persisted-draft crash recovery - never per row. Coverage includes a damaged record directory (skipped, list still renders) and reports not crossing sessions; the narrow-width fixture grows with the new column's minimum table width. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The scriptable face of the browser's Report-a-bug: an explicit attempt id and --description, optional detail flags, and the same pipeline underneath. Authorization preflight comes before any side effect: without a TTY and --yes it exits before collecting or pinning anything. --yes covers only the creation confirmation; needs_review additionally requires --accept-risk (interactive second confirmation on a TTY); blocked cannot be produced by any flag. Every pre-record exit mirrors the browser's staging cleanup. Tests cover the happy path, missing authorization before side effects (pins asserted untouched), risk gating, blocked, severity validation, traversal ids, freeze failure, and stale-at-confirmation cleanup. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
A merged attempt files as one report: the record freezes the definition id with both member traces and merged_definition=true, and the embedded trajectory carries every member's spans. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Review fixes on the non-interactive entry: - a blank --description fails in the preflight, before any collection or pin side effect (typer only enforces presence) - the TTY confirmation renders the full canonical metadata: the frozen attempt association, every populated optional field, the reporter marked as included in the package, and the completeness reasons - the user confirms what actually ships - the blocked refusal is trigger-specific again (source-trajectory hit points at the expert command; a key pasted into --description says to rerun without it), matching the frozen design wording Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The needs_review authorization is judged on evidence: the summary now renders the bounded sanitized sample block (file: sample, five at most, with an overflow note) from the canonical residual findings, plus the frozen session association with last activity and the compact trajectory evidence line - the same final selection sanity check the browser gives before confirming. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The detect-private-key pre-commit hook scans source bytes for the marker substring and cannot tell the blocked-classification fixture apart from a real key; concatenating the marker keeps the runtime value identical while the source no longer carries the contiguous substring. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
…tion time Two review notes from PR #380: - the interactive description prompt (and freeze_export itself) now trims before the emptiness check, so a spaces-only entry reprompts instead of landing a report whose required description looks blank - matching the scriptable command's validation - list_reports orders by the recorded created_at (report id as the deterministic tie-breaker) instead of directory-name order, which was random among same-day reports since the id carries only the UTC date Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
371a4e4 to
d3c802f
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The prior integration blocker is resolved: this revision is based directly on squash-merged main, git merge-tree --write-tree HEAD github/main succeeds, and its tree is byte-for-byte identical to the previously reviewed clean revision. Both earlier bug-report findings remain fixed, and I found no new issue.
I reviewed the immutable github/main...HEAD comparison (SHA-256 e45f9fcdf74f52fb2a71318fb86a1eaf1b71473a09675b65d79ab018953ef379) and rechecked the project rules, full diff, relevant callers and history, backward compatibility, architecture and confidentiality boundaries, and tests for weakening. Risk assessment: high impact if the redaction/export boundary were wrong, low regression likelihood, partial regression protection because of the known teardown crash, managed recovery, and moderate confidence; recommendation is merge with human review.
Exact-head validation in an isolated offline checkout: the three focused regressions passed. The broader affected run completed all 302 assertions, then the interpreter exited 139 during the previously established teardown crash. Diff checks and the dry-run merge passed.
Summary
Tester-facing bug report flow inside the
raven trajectorybrowser(and a scriptable
report-bugsubcommand): pick an attempt, type aone-line description, confirm once, and get a stable report id with a
local shippable package.
Key decisions, in the order the phases landed:
record.json(lifecycle:draft->
local_ready| retryablefailed, crash recovery for interrupteddrafts, 26 structural invariants on load) and the only exportable
artifact
<report-id>.tar.gz- sanitized problem metadata plus anembedded Trajectory Report in its existing layout. Everything the
package will contain is frozen under
snapshot/export/(tree digest,verified before and after every tar run) before the confirmation
screen, so the text the user approves, the first packaging run, and
any retry use the same bytes.
pattern / residual redaction as the trajectory; the merged signals
classify the report as
clean(one confirmation),needs_review(separate authorization;
--yesnever implies it), orblocked(private-key hit; no flag can bypass it). Two sanitization layers
strip absolute paths (generic POSIX/Windows/UNC token parser shared
with the pre-export assertions), tar member headers are anonymized on
both layers, and
RAVEN_BUGREPORT_REQUIRE_REVIEWadds anorganization-policy review trigger.
an offline warn-mode mock replay probe plus explicit contract checks
(
validate_recording, corrupt span lines, format version), mapping tocomplete/degraded/unreplayablewith reasons; unexplained probefailures degrade to
unknowninstead of masquerading as evidencedamage. The probe runs with a new
rates_offline()context so nopricing/catalog/vision-warm path can reach the network.
raven trajectory report(including--yes) andevery existing browser action keep their exact behavior and outputs;
the new flow writes only under
<trace-state>/bugreports/. All newfunctions (
member_traces,classify_redaction, the sanitizer set,evaluate_completeness,diagnose_history_cut,validate_recording,rates_offline) are pure additions.Two pre-existing gaps found and fixed along the way: the sanitizer's
placeholder path for missing artifacts made delivered trajectories fail
load_recording, and mock replays could fire OpenRouter/LiteLLMcatalog fetches.
Type
Verification
uv run pytest tests/test_trajectory_bugreport.py tests/test_trajectory_completeness.py tests/test_cli_trajectory_browse.py tests/test_cli_trajectory_commands.py tests/test_provider_rates.py tests/test_token_wise_pricing.py tests/test_trajectory_replay.py- all green (about 240 new tests among them)uv run pytest- 7090+ passed; the 11 failures + 20 errors are thepre-existing baseline (cron/everos/theme/tracing/config_loader),
reproduced identically on a clean worktree of the base commit
uv run ruff check .anduv run ruff format --check .- cleanmake check-large-files- cleanRelevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
(CONTEXT.md domain terms; command help texts are the user docs)
Risk
redaction classification, path sanitization, header anonymization,
pre-export assertions; expert
trajectory report --yesdeliberatelykeeps its existing semantics)
actions, and store file protocols unchanged; covered by the
pre-existing suites plus explicit regression tests)
only persistent state is the new
<trace-state>/bugreports/directory, which older builds simply ignore)
Related Issues
N/A