192 lines
16 KiB
Markdown
192 lines
16 KiB
Markdown
# Decisions: session-run-trace-isolation
|
|
|
|
## sm-flow State
|
|
|
|
- Checkpoint: Apply / Phase 6 ready
|
|
- Scale: complex
|
|
- Capability source: sm-flow built-in protocol for context/proposal; grill decisions are recorded from the confirmed user discussion in the issue thread.
|
|
- Change slug: `session-run-trace-isolation`
|
|
|
|
## Entry Summary
|
|
|
|
Problem: the same `sessionId` currently represents both multi-turn conversation context and one persisted diagnosis trace. Multi-turn Chat E2E proved that Redis context behaves correctly, but MySQL trace rows from different rounds are mixed under one `session_id`.
|
|
|
|
Expected result: split session metadata from per-run execution state, expose `runId` as the official run identifier, and make trace, feedback, evaluation, demo scripts, Trace UI, and AIOps read/write by run.
|
|
|
|
Known modules: Flyway/JPA entities/repositories, `ChatService`, `ChatController`, `AiOpsService`, `DiagnosisTraceService`, `EvaluationService`, `FeedbackService`, `CaseLibraryService`, `AgentLoggingHook`, `ToolInvocationRecorder`, `SessionContextHolder`, demo scripts, static Trace UI, MVP docs.
|
|
|
|
## Context Collection
|
|
|
|
`devflow/index.md`: relevant entries found.
|
|
|
|
Relevant historical decisions:
|
|
|
|
- `session-storage`: current trace persistence is `diagnosis_session + agent_step + tool_invocation`; `sessionId` propagation uses `RunnableConfig.metadata` with `SessionContextHolder` fallback for tools; AIOps records child agents only.
|
|
- `confidence-feedback`: `FeedbackService` writes feedback, useful feedback creates `case_library`, and feedback must not change execution status.
|
|
- `mvp-demo-trace-acceptance`: Trace API is read-only and demo artifacts/scripts are part of acceptance.
|
|
- `aiops-traceable-diagnosis-entry`: `/api/ai_ops` is an SSE entry point that accepts optional alert payload and exposes `sessionId`.
|
|
- `data-model.md`: current `case_library.diagnosis_id` maps to `diagnosis_session.session_id`; this must be treated as legacy data after the change.
|
|
- `session-trace-lifecycle.md`: current docs already list run id as a future enhancement for multi-run sessions.
|
|
|
|
OpenSpec inputs that must be carried forward:
|
|
|
|
- Current specs mention `diagnosis_session` directly in trace, verifier, evidence, and demo requirements; new specs must either supersede or preserve compatibility for those requirements.
|
|
- `GET /api/diagnosis/{sessionId}/trace` must remain read-only.
|
|
- Baseline/eval checks must expect changed counts only when the change is explained by run isolation.
|
|
|
|
## Question Pool
|
|
|
|
| # | Question | Mode | Status |
|
|
|---|---|---|---|
|
|
| Q1 | Should this be an added field on existing `diagnosis_session`, or split tables? | user-interview | confirmed: split `chat_session` and `diagnosis_run`, keep existing trace detail tables |
|
|
| Q2 | What is metadata in `chat_session`? | user-interview | confirmed: session directory fields only, not full message body |
|
|
| Q3 | Is "message body" the full conversation history? | user-interview | confirmed: full conversation history stays in Redis `SessionContext.messageHistory` for now |
|
|
| Q4 | Should `runId` be an official API field? | user-interview | confirmed: yes |
|
|
| Q5 | How should trace work without `runId`? | user-interview | confirmed: default to latest run for compatibility |
|
|
| Q6 | Should Feedback without `runId` fail or fall back? | user-interview | confirmed: short-term fallback to latest run, long-term may tighten |
|
|
| Q7 | Should simple Chat Q&A create a run? | user-interview | confirmed: yes, every valid `/api/chat` execution creates a run |
|
|
| Q8 | Should AIOps be included? | user-interview | confirmed: yes, same run isolation semantics |
|
|
| Q9 | Should there be a separate trace table? | user-interview | confirmed: no, current `agent_step` and `tool_invocation` are enough for this phase |
|
|
| Q10 | What should happen to old `diagnosis_session`? | user-interview | confirmed: keep it for history/rollback, new code stops writing it after migration |
|
|
| Q11 | Does the issue need demo/Trace UI support? | user-interview | confirmed: yes, minimal `runId` support |
|
|
| Q12 | What extra risks were found by document review? | evidence-driven | reported and patched into ISS-010 |
|
|
|
|
## Evidence-Driven Findings
|
|
|
|
- Code evidence: `SessionContext` contains `messageHistory` and `getMessagePairCount()`, supporting the decision that Redis holds hot conversation history while MySQL stores auditable per-run query/answer.
|
|
- Code evidence: `CaseLibraryService.createFromSession` currently deduplicates by `session.getSessionId()` and maps answer/query from `DiagnosisSession`; this must change for new run-based data.
|
|
- Documentation evidence: `mvp/architecture/data-model.md` states `case_library.diagnosis_id = diagnosis_session.session_id`; this becomes transitional legacy semantics.
|
|
- Documentation evidence: existing trace OpenSpec requires `GET /api/diagnosis/{sessionId}/trace` to be read-only; run resolution must preserve that invariant.
|
|
- E2E evidence from ISS-010: two Chat rounds with the same `sessionId` resulted in one overwritten `diagnosis_session` row and mixed step/tool rows.
|
|
|
|
## Confirmed Decisions
|
|
|
|
- `runId` format: `run-` + full UUID.
|
|
- Latest run ordering: `diagnosis_run.created_at DESC, id DESC`, not `updated_at`.
|
|
- `chat_session` stores metadata: `session_id`, `status`, `message_pair_count`, `created_at`, `last_active_at`, `expires_at`.
|
|
- Per-run long-term audit stores `query` and `answer` in `diagnosis_run`.
|
|
- `agent_step` and `tool_invocation` retain `session_id` and add `run_id`.
|
|
- Missing feedback `runId` returns `fallbackToLatestRun=true` plus actual bound `runId`.
|
|
- Historical mixed data is not split into multiple true runs.
|
|
|
|
## OpenSpec Backwrite Log
|
|
|
|
- Created `proposal.md` with problem, scope, non-goals, context constraints, interface impact, and risks.
|
|
- ISS-010 patched to clarify phase boundary, migration order, AIOps same-change requirement, feedback fallback response, case-library transitional semantics, and Redis/MySQL TTL boundary.
|
|
- Glossary patched to include `SessionContext.messageHistory` and its persistence boundary.
|
|
- Created `design.md`, `specs/session-run-trace-isolation/spec.md`, `specs/mvp-demo-trace-acceptance/spec.md`, and `tasks.md`.
|
|
- Architecture audit found that `ToolTraceSummaryService`, `ExecutorGatekeeperService`, and `EvaluationService` also read tool rows by `sessionId`; Phase 2 tasks were updated to cover run-scoped reads.
|
|
|
|
## Cross-Artifact Alignment
|
|
|
|
| Check | Result | Evidence |
|
|
|---|---|---|
|
|
| issue/context -> proposal | aligned | proposal carries E2E problem, split-table solution, compatibility, AIOps, feedback, demo/UI, and baseline scope |
|
|
| proposal -> design | aligned | design records data model, API impact, migration plan, rollback, and key decisions |
|
|
| design -> specs | aligned | specs cover session/run split, Chat, trace, feedback, AIOps, migration, demo/UI, E2E, and baseline behavior |
|
|
| specs -> tasks | aligned | tasks implement schema, Chat write path, trace reads, feedback/case, AIOps, demo/UI/docs, and verification gates |
|
|
|
|
## Architecture Audit
|
|
|
|
Input to output chain:
|
|
|
|
```text
|
|
Chat/AIOps request
|
|
-> ChatController / AIOps endpoint
|
|
-> ChatService / AiOpsService
|
|
-> execution context(sessionId, runId)
|
|
-> AgentLoggingHook -> agent_step
|
|
-> ToolInvocationRecorder -> tool_invocation
|
|
-> verifier/gatekeeper/evaluation summary reads
|
|
-> diagnosis_run answer/self_evaluation/status/counts
|
|
-> Trace API / Feedback / CaseLibrary / Demo / Trace UI
|
|
```
|
|
|
|
Audit conclusions:
|
|
|
|
- Data ownership is clearer with `chat_session` owning conversation metadata and `diagnosis_run` owning execution state; `agent_step` and `tool_invocation` remain trace details owned by one run.
|
|
- The highest coupling risk is execution-context propagation because hooks and tools currently use both `RunnableConfig.metadata` and `SessionContextHolder`.
|
|
- The main read-path risk is missing one of the session-scoped consumers (`ToolTraceSummaryService`, `ExecutorGatekeeperService`, `EvaluationService`, trace, feedback, case creation).
|
|
- Migration is additive and rollback-friendly until constraints are tightened; historical mixed data must be treated as compatibility data.
|
|
- AIOps must complete before final archive because otherwise the system would still have one production entry point with mixed trace semantics.
|
|
|
|
## Commit Gate Preflight
|
|
|
|
- Interface impact level: L4.
|
|
- Required artifacts: proposal, design, specs, tasks.
|
|
- Strict OpenSpec validation: passed with `openspec validate session-run-trace-isolation --strict`.
|
|
- `.committed` marker: created after successful commit gate.
|
|
|
|
## Pre-apply Research
|
|
|
|
- Reference migrations: `V005__create_session_storage.sql`, `V008__add_answer_to_diagnosis_session.sql`, `V010__add_relevance_level_to_tool_invocation.sql`.
|
|
- Entity style: JPA entities use Lombok `@Data`, `@Builder`, `@NoArgsConstructor`, `@AllArgsConstructor`, `@PrePersist`, and `@PreUpdate` where timestamps need maintenance.
|
|
- Repository style: Spring Data JPA repository interfaces with derived query methods returning `Optional<T>` or `List<T>`.
|
|
- Test style: repository tests use `@DataJpaTest`, `@AutoConfigureTestDatabase(replace = NONE)`, Flyway enabled, and `ddl-auto=validate`.
|
|
|
|
## Phase 1 Apply Notes
|
|
|
|
- Implemented additive migration `V011__add_session_run_isolation.sql`.
|
|
- Implemented `ChatSession` / `DiagnosisRun` entities and repositories.
|
|
- Added nullable `runId` fields to `AgentStep` and `ToolInvocation`.
|
|
- Added run-scoped repository methods for step/tool lookup and counts.
|
|
- 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.
|
|
- Clarified document review findings before continuing Phase 2: OpenSpec task phases are authoritative over the older active issue phase sketch, and AIOps rule evaluation is stored in `diagnosis_run.self_evaluation.aiops_rule_evaluation`, not a separate table.
|
|
|
|
## Document Review Follow-up Before Phase 3 Gate
|
|
|
|
- Clarified AIOps SSE compatibility before Phase 5: keep SSE event name `message`, emit a JSON `SseMessage` with `type=metadata`, and preserve existing content message shape for report streaming.
|
|
- Clarified Feedback API before Phase 4: request `runId` is preferred, response always includes the bound `runId` and `fallbackToLatestRun`, and wrong-session run binding uses the existing failed feedback response path.
|
|
- Clarified run-list API before Phase 3 gate: `GET /api/chat/session/{sessionId}/runs` returns `ApiResponse<List<RunSummary>>`, returns an empty list for an existing session with no runs, and uses existing missing-session error behavior when no session/run data exists.
|
|
|
|
## Phase 2 Apply Notes
|
|
|
|
- Capability source: `openspec-apply-change` + sm-flow apply protocol. `codebase-retrieval` and LSP tools were not available in this session, so call-chain confirmation used OpenSpec context, `rg`, targeted file reads, compilation, focused tests, E2E, DB inspection, and logs.
|
|
- Implemented unified Chat execution context carrying `sessionId` and `runId` through `RunnableConfig.metadata` and `SessionContextHolder`.
|
|
- Switched valid Chat writes to create/update `chat_session` metadata and create one `diagnosis_run` per request.
|
|
- Switched Chat run completion, failure, self-evaluation, metrics, verifier support reads, gatekeeper validation, and evidence scoring to run-scoped data.
|
|
- Added `/api/chat` response `runId` and focused tests for valid run creation, invalid request no-run behavior, run-scoped trace consumers, and same-session multi-turn run creation.
|
|
- Phase 2 gate evidence is recorded in `phase-2-evidence.md`.
|
|
|
|
## Phase 3 Apply Notes
|
|
|
|
- Capability source: `openspec-apply-change` + sm-flow apply protocol. The committed OpenSpec remained the execution source; a document review follow-up tightened DTO/SSE/listing contracts before the Phase 3 gate.
|
|
- Implemented latest-run trace resolution using `diagnosis_run.created_at DESC, id DESC`.
|
|
- Implemented exact trace lookup for `sessionId + runId` with run/session ownership validation.
|
|
- Changed trace details to read `agent_step` and `tool_invocation` by `run_id`, while retaining a legacy `diagnosis_session` fallback for historical compatibility.
|
|
- Added trace response fields for resolved `runId`, chat session metadata, run metadata, and per-row `runId`.
|
|
- Added lightweight `GET /api/chat/session/{sessionId}/runs` backed by `diagnosis_run` summaries.
|
|
- Added focused tests for latest trace, exact first trace, exact second trace, wrong-session rejection, missing session, read-only trace behavior, run listing, and legacy fallback.
|
|
- Phase 3 E2E used Maven startup with profile `mvp-demo`; evidence is recorded in `phase-3-evidence.md`.
|
|
|
|
## Phase 4 Apply Notes
|
|
|
|
- Capability source: `openspec-apply-change` + sm-flow apply protocol. `codebase-retrieval` and LSP tools were still unavailable; call-chain confirmation used OpenSpec context, `rg`, targeted file reads, focused tests, API E2E, DB inspection, and logs.
|
|
- Added `FeedbackRequest.runId` and `FeedbackResponse.runId/fallbackToLatestRun`.
|
|
- Changed `FeedbackController` to pass `runId` through to `FeedbackService`.
|
|
- Changed `FeedbackService` so new feedback prefers exact `sessionId + runId`, validates ownership, falls back to latest run when `runId` is omitted, and writes new feedback to `diagnosis_run.feedback`.
|
|
- Preserved a legacy `DiagnosisSession` fallback only when no `diagnosis_run` exists, so old data can still receive feedback during the migration window.
|
|
- Added `CaseLibraryService.createFromRun`, using `diagnosis_run.run_id` as the new automatic `case_library.diagnosis_id`; `createFromSession` remains the legacy session-id path.
|
|
- Phase 4 evidence is recorded in `phase-4-evidence.md`.
|
|
- Document review follow-up: clarified that the legacy `DiagnosisSession` feedback path only applies when no `diagnosis_run` exists for the session. It returns no bound `runId` and is not the same as latest-run fallback.
|
|
|
|
## Phase 5 Apply Notes
|
|
|
|
- Capability source: `openspec-apply-change` + sm-flow apply protocol. `codebase-retrieval` and LSP tools remain unavailable; call-chain confirmation used OpenSpec context, `rg`, targeted file reads, dependency method inspection with `javap`, focused tests, E2E, DB inspection, and logs.
|
|
- Changed `/api/ai_ops` to allocate a `runId` before execution and emit a JSON `SseMessage` with `type=metadata`, `sessionId`, and `runId` on SSE event name `message`.
|
|
- Changed `AiOpsService` to create one `diagnosis_run` with `agent_flow=AI_OPS` for each valid execution instead of writing new execution state to `diagnosis_session`.
|
|
- Changed AIOps execution context propagation to pass `sessionId/runId` through both `RunnableConfig.metadata` and `SessionContextHolder`.
|
|
- Changed AIOps final report, metrics, status, and `aiops_rule_evaluation` persistence to update the current `diagnosis_run`.
|
|
- Preserved `persistFinalReport(sessionId, finalReport, request)` as a historical `DiagnosisSession` compatibility path.
|
|
- Added focused tests for run-scoped final report evaluation, run-scoped metrics, distinct runs under the same AIOps session, and SSE metadata shape.
|