feat(session): add lifecycle causality diagnostics#825
Merged
Conversation
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/utils/server.test.ts, packages/app/src/utils/server.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
Contributor
📝 WalkthroughWalkthroughThis PR adds client-action header helpers and server request-context snapshots, enriches lifecycle close actions with timestamps/affected-directory keys/origin/request, threads provenance through instance-store disposal/reload and middleware, captures provenance in run-observability summaries, extends run-incident export/sanitization with incident-chains, and updates UI flows to use action-scoped SDK clients. ChangesLifecycle Causality Diagnostics for Interrupted Runs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces a request context tracking system using AsyncLocalStorage to capture and propagate client action metadata throughout the session lifecycle. It enhances lifecycle provenance by adding timestamps, hashed directory keys for privacy, and origin information to interrupts and incidents. Additionally, it adds support for exporting incident chains in session diagnostics. Feedback was provided regarding the use of Hono's built-in request path property to improve reliability and efficiency when capturing the request path.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/session/export.ts`:
- Around line 1067-1071: The code currently returns diagnostics.incident_chains
as-is which lets fields like plain_summary/nearest_origin bypass sanitization;
always sanitize and export each incident in any chain regardless of whether
incident_chains already exists: when building incident_chains use a map over
(diagnostics.incident_chains ?? ...) and for each incident in each chain call
RunIncident.sanitize(...) then RunIncident.toExportChain(...) so every incident
passes through RunIncident.sanitize before toExportChain; update the logic that
produces incident_chains to perform this per-incident sanitization and export
using RunIncident.sanitize and RunIncident.toExportChain.
In `@packages/opencode/src/session/lifecycle-provenance.ts`:
- Around line 42-44: The lifecycle provenance payloads expose mutable
references; update createLifecycleCloseAction and lifecycleCloseActionMeta so
affectedDirectoryKeys and origin are frozen clones rather than direct
references: produce new copies (e.g., new array from Set for
affectedDirectoryKeys) and Object.freeze them, and for origin make a defensive
clone (shallow or deep as appropriate for origin's shape) then Object.freeze
that clone; ensure the returned payload uses these frozen values so downstream
mutation cannot alter prior provenance snapshots.
In `@packages/opencode/src/session/run-observability/recorder.ts`:
- Around line 395-398: When building lifecycle snapshots from the "next" object,
you are assigning references directly (next.lifecycleInitiatedAt,
next.lifecycleInitiatedMonotonicMs, next.lifecycleAffectedDirectoryKeys,
next.lifecycleOrigin) which allows later mutation to alter stored snapshots;
clone these payloads before storing/returning (e.g., copy primitive timestamps
directly, shallow-copy arrays like lifecycleAffectedDirectoryKeys with
[...next.lifecycleAffectedDirectoryKeys], and shallow/deep-clone objects like
lifecycleOrigin via {...next.lifecycleOrigin} or structuredClone/JSON roundtrip
as appropriate). Apply the same cloning change to the other occurrence that sets
those fields (the block around the second occurrence noted in the comment).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ca23af5-c5d2-4a32-ae6e-e0d2378b066c
📒 Files selected for processing (19)
packages/app/src/utils/server.test.tspackages/app/src/utils/server.tspackages/opencode/src/effect/runner.tspackages/opencode/src/project/instance-store.tspackages/opencode/src/server/request-context.tspackages/opencode/src/server/routes/instance/middleware.tspackages/opencode/src/session/export.tspackages/opencode/src/session/lifecycle-provenance.tspackages/opencode/src/session/processor.tspackages/opencode/src/session/run-incident/derive.tspackages/opencode/src/session/run-incident/index.tspackages/opencode/src/session/run-incident/types.tspackages/opencode/src/session/run-observability/recorder.tspackages/opencode/src/session/run-observability/types.tspackages/opencode/src/session/run-state.tspackages/opencode/test/server/middleware.test.tspackages/opencode/test/session/export.test.tspackages/opencode/test/session/run-observability.test.tspackages/opencode/test/session/run-state.test.ts
Perf delta summaryComparator: pass
|
b17ca66 to
49d5f61
Compare
49d5f61 to
b9fbb13
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/session/run-incident/derive.ts (1)
68-76:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve lifecycle
sourcewhen deriving incident provenance.
input.lifecycle.sourceis typed on input but currently dropped in the derivedprovenance.lifecycle, which can silently lose initiator context on paths that don’t populateorigin.💡 Proposed fix
lifecycle: { action_id: input.lifecycle.action_id, kind: input.lifecycle.kind, + source: input.lifecycle.source, reason: input.lifecycle.reason, initiated_at: input.lifecycle.initiated_at, initiated_monotonic_ms: input.lifecycle.initiated_monotonic_ms, affected_directory_keys: input.lifecycle.affected_directory_keys, origin: input.lifecycle.origin,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/session/run-incident/derive.ts` around lines 68 - 76, The derived provenance lifecycle is dropping input.lifecycle.source; update the lifecycle object construction (the block that builds provenance.lifecycle) to include source: input.lifecycle.source so the initiator context is preserved (e.g., add "source: input.lifecycle.source" alongside action_id, kind, reason, etc., in the lifecycle literal). Ensure the provenance type accepts this field or cast/extend types if necessary.
🧹 Nitpick comments (1)
packages/app/src/utils/server.test.ts (1)
39-39: ⚡ Quick winBroaden path-leak assertions beyond macOS-only patterns.
The current check only catches
"/Users/". Add Linux/Windows path markers so this privacy test stays effective across environments.💡 Suggested test update
- expect(JSON.stringify(headers)).not.toContain("/Users/") + const serialized = JSON.stringify(headers) + expect(serialized).not.toContain("/Users/") + expect(serialized).not.toContain("/home/") + expect(serialized).not.toContain("\\\\Users\\\\")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/utils/server.test.ts` at line 39, The test currently only asserts expect(JSON.stringify(headers)).not.toContain("/Users/"); update this assertion to block common absolute path patterns across OSes by checking for macOS (/Users/), Linux (/home/), and Windows drive paths (e.g. a letter + colon + backslash or forward slash like C:\ or C:/); replace the single toContain check with either a single regex-based check against JSON.stringify(headers) that matches any of these patterns or with multiple not.toContain assertions for "/Users/", "/home/", and a Windows drive-letter pattern so the privacy assertion applies on all platforms.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/opencode/src/session/run-incident/derive.ts`:
- Around line 68-76: The derived provenance lifecycle is dropping
input.lifecycle.source; update the lifecycle object construction (the block that
builds provenance.lifecycle) to include source: input.lifecycle.source so the
initiator context is preserved (e.g., add "source: input.lifecycle.source"
alongside action_id, kind, reason, etc., in the lifecycle literal). Ensure the
provenance type accepts this field or cast/extend types if necessary.
---
Nitpick comments:
In `@packages/app/src/utils/server.test.ts`:
- Line 39: The test currently only asserts
expect(JSON.stringify(headers)).not.toContain("/Users/"); update this assertion
to block common absolute path patterns across OSes by checking for macOS
(/Users/), Linux (/home/), and Windows drive paths (e.g. a letter + colon +
backslash or forward slash like C:\ or C:/); replace the single toContain check
with either a single regex-based check against JSON.stringify(headers) that
matches any of these patterns or with multiple not.toContain assertions for
"/Users/", "/home/", and a Windows drive-letter pattern so the privacy assertion
applies on all platforms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8fa13c48-5b0b-414e-afcf-80b12b3a1bf4
📒 Files selected for processing (30)
packages/app/src/components/dialog-connect-provider-source.test.tspackages/app/src/components/dialog-connect-provider.tsxpackages/app/src/components/settings-providers.tsxpackages/app/src/context/global-sync.tsxpackages/app/src/context/global-sync/client-action-source.test.tspackages/app/src/pages/layout.tsxpackages/app/src/utils/server.test.tspackages/app/src/utils/server.tspackages/opencode/src/config/config.tspackages/opencode/src/effect/runner.tspackages/opencode/src/project/instance-store.tspackages/opencode/src/server/instance/global.tspackages/opencode/src/server/instance/middleware.tspackages/opencode/src/server/request-context.tspackages/opencode/src/server/routes/instance/middleware.tspackages/opencode/src/session/export.tspackages/opencode/src/session/lifecycle-provenance.tspackages/opencode/src/session/processor.tspackages/opencode/src/session/run-incident/derive.tspackages/opencode/src/session/run-incident/index.tspackages/opencode/src/session/run-incident/sanitize.tspackages/opencode/src/session/run-incident/types.tspackages/opencode/src/session/run-observability/recorder.tspackages/opencode/src/session/run-observability/types.tspackages/opencode/src/session/run-state.tspackages/opencode/test/project/instance-store.test.tspackages/opencode/test/server/middleware.test.tspackages/opencode/test/session/export.test.tspackages/opencode/test/session/run-observability.test.tspackages/opencode/test/session/run-state.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds lifecycle causality diagnostics for interrupted runs: immutable lifecycle close snapshots, request context provenance on real instance/global lifecycle routes, renderer client-action wiring for lifecycle-triggering UI calls, config-origin attribution, structured request/client-action provenance in run incidents and incident chains, explicit load→reload context-mismatch reasons, lifecycle initiation-time ordering, and sanitized export incident chains.
Why
#802 asks for the local lifecycle causality slice of #808 so exported diagnostics can explain which local reload/dispose action interrupted a run and what provenance is complete or missing.
Related Issue
Closes #802. Part of #808.
Human Review Status
Pending
Review Focus
Please focus on whether lifecycle close provenance stays diagnostic/export-only, does not change recovery behavior, preserves lifecycle-vs-provider terminal cause ordering while retaining lifecycle provenance, carries safe request/client-action initiator fields from real renderer call paths into exports, records explicit load→reload context mismatch/config origins, and avoids leaking raw local paths or secret-like strings.
Risk Notes
Skipped conditional checks:
Platform impact was considered: the change hashes local paths before export and only adds diagnostic headers/snapshots, without changing runtime lifecycle behavior.
How To Verify
Screenshots or Recordings
Not applicable — no visible UI or copy changes.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
New Features
Bug Fixes
Tests