SuperBizAgent-java/openspec/changes/session-run-trace-isolation/decisions.md

# Decisions: session-run-trace-isolation

## sm-flow State

- Checkpoint: Discover
- 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.