diff --git a/openspec/changes/session-run-trace-isolation/decisions.md b/openspec/changes/session-run-trace-isolation/decisions.md index 56778ac..8671c58 100644 --- a/openspec/changes/session-run-trace-isolation/decisions.md +++ b/openspec/changes/session-run-trace-isolation/decisions.md @@ -133,3 +133,11 @@ Audit conclusions: - Added `DiagnosisRunRepositoryTest`. - Verification evidence is recorded in `phase-1-evidence.md`. - Historical DB inspection found one orphan `tool_invocation` row without a matching `diagnosis_session`; it remains `run_id = NULL` because no reliable compatibility run can be inferred. + +## Document Review Follow-up + +- Clarified that Chat creates runs for the effective resolved `sessionId`, including requests where the server generates a session id. +- Tightened legacy feedback fallback so `fallbackToLatestRun=true` and the actual bound `runId` are response fields, not log-only evidence. +- Clarified AIOps SSE compatibility: emit a metadata message containing `sessionId` and `runId` before report content while preserving the existing content stream shape. +- Clarified that run/session ownership is enforced by service-layer validation and indexed lookup in this change; database foreign keys are intentionally deferred to preserve compatibility with historical orphan detail rows and rollback. +- Clarified that `chat_session.expires_at` is nullable directory metadata / best-effort TTL snapshot, not mandatory persisted conversation history. diff --git a/openspec/changes/session-run-trace-isolation/design.md b/openspec/changes/session-run-trace-isolation/design.md index 13f8edf..8babd54 100644 --- a/openspec/changes/session-run-trace-isolation/design.md +++ b/openspec/changes/session-run-trace-isolation/design.md @@ -46,6 +46,7 @@ Constraints from existing work: | Feedback fallback | Missing `runId` binds latest run and returns fallback metadata | Reject missing `runId` immediately | Short-term compatibility is needed for old clients; explicit fallback keeps ambiguity observable. | | Case library provenance | New automatic cases store `diagnosis_id = run_id` | Add a new case-library column now | Existing column name can carry transitional provenance; docs and query logic must recognize old `session_id` and new `run_id`. | | AIOps phase | Implement after Chat but before overall archive | Leave AIOps for a follow-up issue | AIOps is already a trace entry point; leaving it old-model would preserve the same bug on another endpoint. | +| Ownership integrity | Validate run/session ownership in application services; do not add database foreign keys in this change | Add foreign keys from `diagnosis_run`, `agent_step`, and `tool_invocation` | Existing historical/orphan compatibility data and rollback needs make additive, application-level validation safer for this release. | ## Data Model @@ -87,7 +88,9 @@ tool_invocation ... ``` -`chat_session.expires_at` is MySQL directory metadata. Redis TTL can expire `SessionContext.messageHistory`; persisted `diagnosis_run`, `agent_step`, and `tool_invocation` remain audit records. +`chat_session.expires_at` is nullable MySQL directory metadata and may be a best-effort Redis TTL snapshot when known. Redis TTL can expire `SessionContext.messageHistory`; persisted `diagnosis_run`, `agent_step`, and `tool_invocation` remain audit records. + +Ownership between `chat_session`, `diagnosis_run`, `agent_step`, and `tool_invocation` is enforced by service-layer validation and indexed lookup in this change. The migration intentionally does not add database foreign keys so historical orphan trace detail rows and rollback paths remain compatible. ## API / Interface Impact @@ -95,7 +98,7 @@ Interface level: L4. - Database contract changes: new tables, new columns, backfill, indexes, and later non-null expectations for new writes. - `/api/chat` response adds `runId`. -- `/api/ai_ops` SSE emits the resolved `runId`. +- `/api/ai_ops` SSE emits a compatible metadata message before report content. The metadata payload includes `sessionId` and `runId`; report content continues to stream through the existing content message shape. - Trace API accepts optional `runId`. - Feedback request accepts preferred `runId` and returns fallback binding metadata when omitted. - New run summary API: `GET /api/chat/session/{sessionId}/runs`. @@ -136,4 +139,3 @@ Rollback: ## Open Questions None blocking. Long-term tightening of missing feedback `runId` remains a follow-up decision after clients migrate. - diff --git a/openspec/changes/session-run-trace-isolation/specs/session-run-trace-isolation/spec.md b/openspec/changes/session-run-trace-isolation/specs/session-run-trace-isolation/spec.md index 9a488ba..e68683b 100644 --- a/openspec/changes/session-run-trace-isolation/specs/session-run-trace-isolation/spec.md +++ b/openspec/changes/session-run-trace-isolation/specs/session-run-trace-isolation/spec.md @@ -4,10 +4,10 @@ The system SHALL persist multi-turn conversation metadata in `chat_session` and one execution's auditable state in `diagnosis_run`. #### Scenario: Valid Chat execution creates session metadata and a run -- **WHEN** a valid `/api/chat` request enters the Chat execution path with a `sessionId` -- **THEN** the system SHALL ensure a `chat_session` row exists for that `sessionId` +- **WHEN** a valid `/api/chat` request enters the Chat execution path and the service resolves an effective `sessionId` +- **THEN** the system SHALL ensure a `chat_session` row exists for the effective `sessionId` - **AND** it SHALL create a new `diagnosis_run` row with a unique `run_id` -- **AND** the `diagnosis_run.session_id` SHALL equal the request `sessionId` +- **AND** the `diagnosis_run.session_id` SHALL equal the effective `sessionId` #### Scenario: Invalid Chat request does not create a run - **WHEN** a `/api/chat` request fails parameter validation before execution @@ -15,7 +15,7 @@ The system SHALL persist multi-turn conversation metadata in `chat_session` and #### Scenario: Chat session stores metadata only - **WHEN** a Chat request completes -- **THEN** `chat_session` SHALL store metadata such as status, message pair count, created time, last active time, and expiration time +- **THEN** `chat_session` SHALL store metadata such as status, message pair count, created time, last active time, and optional expiration time - **AND** it SHALL NOT store full conversation message history ### Requirement: Chat responses SHALL expose run identity @@ -86,8 +86,8 @@ The system SHALL bind new feedback to a diagnosis run rather than an ambiguous m #### Scenario: Feedback without runId falls back observably - **WHEN** a legacy feedback request includes `sessionId` but omits `runId` - **THEN** the system SHALL bind feedback to the latest run for that session -- **AND** the response or log SHALL include `fallbackToLatestRun=true` -- **AND** the response or log SHALL include the actual bound `runId` +- **AND** the response SHALL include `fallbackToLatestRun=true` +- **AND** the response SHALL include the actual bound `runId` #### Scenario: Useful feedback creates case from run - **WHEN** feedback for a run is `useful` @@ -104,8 +104,9 @@ The system SHALL create and expose a diagnosis run for every valid `/api/ai_ops` #### Scenario: AIOps SSE exposes runId - **WHEN** `/api/ai_ops` streams response metadata to the caller -- **THEN** the stream SHALL expose the resolved `sessionId` -- **AND** it SHALL expose the created `runId` +- **THEN** the stream SHALL send a compatible metadata message before report content +- **AND** the metadata payload SHALL expose the resolved `sessionId` +- **AND** the metadata payload SHALL expose the created `runId` ### Requirement: Migration SHALL preserve historical trace access The system SHALL migrate historical diagnosis data into compatibility runs without deleting the old `diagnosis_session` table. @@ -144,4 +145,3 @@ The change SHALL verify both runtime behavior and evaluation baseline impact. - **WHEN** verification is complete - **THEN** the project SHALL run or explicitly evaluate the relevant baseline diff command - **AND** any drift caused by run isolation SHALL be documented as expected or investigated as a regression -