Skip to content

fix: make all skills warehouse-agnostic with best-practice improvements#16

Closed
suryaiyer95 wants to merge 7 commits into
mainfrom
fix/skill-improvements
Closed

fix: make all skills warehouse-agnostic with best-practice improvements#16
suryaiyer95 wants to merge 7 commits into
mainfrom
fix/skill-improvements

Conversation

@suryaiyer95
Copy link
Copy Markdown
Contributor

Summary

All 10 remaining skills updated to be warehouse-agnostic and follow community best practices. Research covered dbt community, Reddit, industry blogs, and official documentation.

Cross-cutting changes (all skills):

  • Add "Use when" trigger phrasing to every description for LLM auto-activation
  • Add dialect/warehouse detection as first workflow step (warehouse_list/dbt_profiles)
  • Replace Snowflake-specific defaults with auto-detection

Per-skill improvements:

  • query-optimize: Remove Snowflake default, add cross-warehouse optimization guidance
  • sql-translate: Add dialect inference from SQL syntax, common migration paths, high-risk areas
  • incremental-logic: Fix microbatch claim (cross-platform since dbt v1.9), add per-warehouse strategy references/
  • medallion-patterns: Add convention detection via dbt_manifest, resolve naming conflict with model-scaffold
  • generate-tests: Add dbt_expectations, dbt_utils, elementary packages with full test catalog in references/
  • dbt-docs: Add documentation patterns by layer, doc block guidance
  • impact-analysis: Add schema_diff and dbt_lineage tools, breaking change classification
  • lineage-diff: Add schema_diff, dbt_manifest, dbt_lineage, modified edge detection
  • model-scaffold: Add warehouse-specific materialization defaults, dbt Mesh governance patterns
  • yaml-config: Add data_tests v1.8 rename, model contracts, YAML organization patterns

New reference files (progressive disclosure):

  • generate-tests/references/test-packages.md — Full catalog of dbt_utils, dbt_expectations, elementary tests
  • generate-tests/references/coverage-strategy.md — Per-layer test priority guide
  • incremental-logic/references/{snowflake,bigquery,databricks,postgres,redshift}.md — Warehouse-specific incremental strategies

Test plan

  • Verify all 10 skills load correctly with updated frontmatter
  • Verify references/ files are discoverable when skills are activated
  • Spot-check descriptions trigger LLM auto-activation on relevant prompts

🤖 Generated with Claude Code

anandgupta42 and others added 6 commits March 1, 2026 21:14
AI-powered CLI for SQL analysis, dbt integration, and data engineering.

- TypeScript CLI with cross-platform binaries (npm, Homebrew)
- Python engine sidecar for SQL analysis and warehouse connectivity
- JSON-RPC bridge between CLI and engine
- AI-powered code review, SQL optimization, and lineage analysis
- dbt project integration with model parsing and profile management
- GitHub Actions and CI/CD integration
- MCP (Model Context Protocol) support
The root bunfig.toml prevents running tests from the repo root.
CI must use working-directory to run from packages/altimate-code/.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tests need git user.name/email for git commit in temp dirs. Also fix
github-remote test expectations that still referenced sst/opencode.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* docs: rewrite README for open-source launch

Position as opencode fork specialized for data teams. Add comparison
table, detailed feature sections, and correct badge URLs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: move opencode attribution to Acknowledgements section

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: update package name and Homebrew tap path in upgrade/uninstall logic

Internal upgrade, version check, and uninstall commands still referenced
the old package name `altimate-code-ai` and tap path `altimate/tap`.
Updated to `@altimateai/altimate-code` and `AltimateAI/tap`.

Fixes Sentry review comments on PR #14.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All 10 skills updated:
- Add "Use when" trigger phrasing to every description for LLM auto-activation
- Add dialect/warehouse detection as first workflow step in every skill
- Replace Snowflake-specific defaults with auto-detection via `warehouse_list`/`dbt_profiles`

Per-skill improvements:
- query-optimize: remove Snowflake default, add cross-warehouse optimization guidance
- sql-translate: add dialect inference, common migration paths, high-risk translation areas
- incremental-logic: fix microbatch (not Snowflake-only since dbt v1.9), add per-warehouse strategy references
- medallion-patterns: add convention detection via `dbt_manifest`, resolve naming conflict with model-scaffold
- generate-tests: add `dbt_expectations`, `dbt_utils`, `elementary` packages, add test coverage strategy reference
- dbt-docs: add documentation patterns by layer, doc block guidance
- impact-analysis: add `schema_diff` and `dbt_lineage` tools, breaking change classification
- lineage-diff: add `schema_diff`, `dbt_manifest`, `dbt_lineage`, modified edge detection
- model-scaffold: add warehouse-specific materialization defaults, dbt Mesh governance
- yaml-config: add `data_tests` v1.8 rename, model contracts, YAML organization patterns

New reference files:
- generate-tests/references/test-packages.md (dbt_utils, dbt_expectations, elementary catalog)
- generate-tests/references/coverage-strategy.md (per-layer test priorities)
- incremental-logic/references/{snowflake,bigquery,databricks,postgres,redshift}.md

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
```sql
{{ config(
materialized='incremental',
unique_key='order_id',

This comment was marked as outdated.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines +18 to +22
2. **Discover existing tests** -- Use `dbt_manifest` to load the project manifest. Extract:
- All tests already defined for the target model (avoid duplicates)
- Which test packages are installed (`dbt_utils`, `dbt_expectations`, `elementary`)
- Upstream/downstream model dependencies
3. **Find the model file** -- Use `glob` to locate the model SQL file and any existing schema YAML (`schema.yml`, `_schema.yml`, `_<model>__models.yml`) in the same directory.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The dbt_manifest tool does not return installed package information as claimed in the generate-tests/SKILL.md documentation, preventing the agent from detecting available test packages.
Severity: HIGH

Suggested Fix

Enhance the dbt_manifest tool to parse the package_name property for resources within the manifest.json. Aggregate this data to create a list of installed packages and include it in the tool's return value. Alternatively, update the skill documentation to use a different method for detecting packages, such as reading packages.yml directly.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: .altimate-code/skills/generate-tests/SKILL.md#L18-L22

Potential issue: The documentation in `generate-tests/SKILL.md` instructs the AI agent
to use the `dbt_manifest` tool to identify installed test packages like `dbt_utils`.
However, the tool's implementation in `dbt/manifest.py` does not extract or return this
information. The `parse_manifest()` function's result object lacks any field for
installed packages. This mismatch will cause the agent to fail silently when trying to
follow the documented workflow, as it cannot determine which test packages are
available. This leads to generating suboptimal tests or tests that fail at runtime
because they reference unavailable packages.

- Call `warehouse_list` to check for configured connections (returns name, type, database)
- If no connections found, call `dbt_profiles` to discover warehouse type from dbt configuration
- If neither yields a result, ask the user which warehouse they are using
- Pass the detected dialect to all subsequent tool calls via the `dialect` parameter
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The sql.analyze and sql.optimize tools accept a dialect parameter but their implementations silently ignore it, leading to incorrect, non-dialect-specific analysis and optimizations.
Severity: HIGH

Suggested Fix

Update the sql.analyze and sql.optimize tool implementations in server.py. Pass the dialect from the params_obj to the underlying function calls, such as guard_rewrite_sql(..., dialect=params_obj.dialect) and guard_lint(..., dialect=params_obj.dialect). This will ensure the dialect is used for SQL analysis and rewriting as intended.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: .altimate-code/skills/query-optimize/SKILL.md#L22

Potential issue: The documentation in `query-optimize/SKILL.md` states that the detected
SQL `dialect` should be passed to tools like `sql.analyze` and `sql.optimize`. While
these tools accept a `dialect` parameter in their respective `SqlAnalyzeParams` and
`SqlOptimizeParams` models, the implementation silently ignores it. The underlying
functions (`guard_lint`, `guard_check_semantics`, `guard_rewrite_sql`) are called
without the `dialect` argument. This causes the tools to default to generic or
Snowflake-specific logic, providing incorrect or suboptimal analysis and optimizations
for other database dialects like Postgres or BigQuery.

@kulvirgit kulvirgit closed this Mar 2, 2026
sahrizvi pushed a commit that referenced this pull request Jun 5, 2026
…uncation constant

Three follow-ups from the multi-LLM consensus review of PR #895, codex-validated:

* Worker user-text branch now skips parts with `synthetic` or `ignored`
  set (Major #1 in the review, codex-confirmed). `Session.createUserMessage`
  in prompt.ts attaches many synthetic text parts to the user messageID for
  MCP resource banners, decoded file contents, retry/reminder text,
  plan-mode reminders, and agent-handoff tags. Without the gate they pass
  the `sessionUserMsgIds.has(messageID)` check, `metadata.prompt` ends up
  holding the LAST synthetic part (typically a file blob), and the chat tab
  renders one fake "▶ You" bubble per synthetic span — defeating the two
  display surfaces this PR fixes. Gated on an `isAuthoredText` predicate
  so the symmetric assistant-text branch is also protected. Continue via
  predicate rather than `continue` keyword so the outer event loop still
  forwards the event downstream via `Rpc.emit`.

* `Trace.rehydrateFromFile` now marks any open generation span as
  interrupted (Major #2). The transient state `logStepStart` populated
  (`currentGenerationSpanId`, `generationText`, `generationToolCalls`,
  `pendingToolResults`) is memory-only and can't be reconstructed from
  disk. If we leave open spans open, the next `step-finish` for that turn
  drops at the `!this.currentGenerationSpanId` guard and follow-up
  `logToolCall` mis-parents tool spans to the root, silently degrading the
  trace shape. Closing the span with `status: "error"` and a
  `statusMessage` describing the interruption preserves the partial data
  and makes the boundary visible in the viewer.

* Extracted the 4000-char truncation cap as `USER_MESSAGE_INPUT_MAX_CHARS`
  exported from tracing.ts (new cubic P3 on viewer.ts:1331). The viewer's
  chat-tab dedupe now interpolates the same constant so the two sides can't
  drift if the truncation cap ever changes. Also reused a single
  `Date.now()` call for the user-message span's start/end timestamps
  (cosmetic, addresses review nit #16).

Skipped from the review:
- Major #3 (no per-messageID dedupe in logUserMessage): codex confirmed
  user text doesn't stream/chunk — message.part.delta is assistant-only
  — so the symptom the review described is subsumed by Major #1's
  synthetic gate. No separate fix needed.
- Major #5 (Path A `setTitle(text, text)` couples title and prompt):
  codex grep-verified that no in-repo code path populates
  `userMsg.summary.title` or `summary.body`; the branch is inert.
  Cleanup risk only, tracked in #896.

Tests
- New behavioral test in tracing-rehydrate.test.ts asserts that an open
  generation span (no `step-finish` before reconstruction) ends up
  with `endTime` set, `status: "error"`, and a statusMessage matching
  `/interrupted/i` after rehydrate + a snapshot-triggering call.
- New source-grep test in worker-trace-clearing.test.ts locks both
  the synthetic-gate literal (`!part.synthetic && !part.ignored`) and
  the requirement that both `trace.setPrompt` and `trace.logUserMessage`
  sit inside the `isAuthoredText &&` guard.

42 affected tests pass; typecheck clean.
anandgupta42 pushed a commit that referenced this pull request Jun 6, 2026
…ta after each agent turn (#895)

* fix(tracing): waterfall view collapses to system-prompt span on agent-finish

Symptom: opening `/traces` mid-session and watching the waterfall view
during agent execution shows the rich trace populating correctly, but
the moment the agent finishes its turn the view collapses to a single
"system-prompt" span — and the data is genuinely lost on disk, not
just from the viewer.

Chain (verified by reading worker.ts + routes/session/index.tsx + tracing.ts):

  1. `routes/session/index.tsx` has
     `createEffect(() => session()?.workspaceID && sdk.setWorkspace(...))`.
     SolidJS dirty-tracks any signal read inside the effect — so it
     re-runs on EVERY `session()` change (message count, status, parts
     updates), including the cascade at agent-finish.
  2. Each fire calls `worker.setWorkspace` via RPC.
  3. `worker.setWorkspace` unconditionally calls `startEventStream`.
  4. `startEventStream`:
        for (const [, trace] of sessionTraces) {
          void trace.endTrace().catch(() => {})   // fire-and-forget
        }
        sessionTraces.clear()
  5. On the next event for the same session, `getOrCreateTrace` hits a
     cache miss, calls `Trace.create()` + `startTrace(sessionID, {})`,
     which pushes a single root span into a freshly-empty `this.spans`
     and calls `this.snapshot()`. The snapshot path is derived purely
     from sessionID (`tracing.ts:836`), so it overwrites the rich
     `ses_<id>.json` with a 1-span file.

Distinct from #867 — that PR fixed intra-Trace-instance concurrency
(snapshot debounce M2; FileExporter ↔ flushSync race M3). This bug is
at the worker-level cache lifecycle: a new Trace instance gets
constructed and its near-empty initial state clobbers the previous
instance's rich state on disk.

Two minimal fixes lock the contract:

* `worker.ts` — make `setWorkspace` idempotent. Track
  `currentWorkspaceID` at module scope; early-return when the incoming
  value matches. Spurious calls from the UI no longer destroy traces.
* `routes/session/index.tsx` — switch the workspaceID effect to
  `createEffect(on(() => session()?.workspaceID, ...))`. The `on()`
  projector restricts SolidJS dirty-tracking to that one field, so the
  effect only fires when workspaceID actually changes — defense in
  depth at the upstream trigger.

Regression test in `test/cli/tui/worker-trace-clearing.test.ts`
locks both contracts via source-grep (the worker-import side has
top-level side effects that make in-process unit testing awkward).

Out of scope (follow-up): `getOrCreateTrace` on cache miss does not
rehydrate `this.spans` from the existing `ses_<id>.json` file. After
the two fixes above, this matters only on worker restart or
MAX_TRACES=100 eviction — both uncommon. Worth tracking as defense in
depth so the disk file is always authoritative.

Typecheck clean. 152 TUI tests pass; 35 existing tracing tests pass
unchanged.

* fix(tracing): real root cause — session.status=idle was destroying the Trace every turn

Previous commit on this branch was correct but not load-bearing. The
actual hot path that collapsed the on-disk trace after every agent
turn is in `worker.ts`:

    if (event.type === "session.status") {
      if (status === "idle" && sid) {
        const trace = sessionTraces.get(sid)
        if (trace) {
          void trace.endTrace().catch(() => {})
          sessionTraces.delete(sid)            // ← every turn
          sessionUserMsgIds.delete(sid)
        }
      }
    }

`session.status === "idle"` fires after every busy→idle transition,
which happens once per turn — not once per session. Each fire ended
the trace AND deleted the cache entry. The next event for the same
session in the next turn hit a cache miss, constructed a fresh
`Trace.create()`, called `startTrace(sessionID, {})` (which pushes a
single root span into empty `this.spans`), and the immediate
`snapshot()` clobbered the rich on-disk `ses_<id>.json` with a 1-span
file.

This also explains the "What was asked / No prompt recorded" symptom:
`metadata.prompt` was captured on the now-destroyed first instance and
never persisted into the replacement.

Fixes in this commit:

* `worker.ts`: removed the destructive `session.status === "idle"`
  handler. Sessions in altimate-code are long-lived; the Trace lives
  as long as the worker has the session in cache. Finalization
  happens on `shutdown` and on MAX_TRACES eviction only — both
  already correct.

* `tracing.ts`: new `Trace.rehydrateFromFile(sessionId)` that reads
  the existing on-disk file and restores `this.spans`, `this.metadata`,
  `this.rootSpanId`, `this.startTime`, counters, and clears the root's
  endTime so the trace renders as still-in-progress. Returns true on
  success; false on missing/mismatched/malformed file.

* `worker.ts:getOrCreateTrace`: on cache miss, calls
  `rehydrateFromFile` before falling back to `startTrace`. Defense in
  depth for the worker-restart / MAX_TRACES-eviction paths — even if
  some future path destroys the in-memory instance, the new instance
  loads disk state instead of overwriting.

Verification

* Behavioral test (`test/altimate/tracing-rehydrate.test.ts`, 4 cases):
  proves end-to-end that rehydrate preserves spans+metadata across
  Trace-instance reconstruction, returns false on missing/mismatched
  files, and clears the root endTime so re-opened traces accept new
  events.
* Source-grep regression tests for worker.ts continue to lock the
  no-idle-clobber and rehydrate-before-startTrace contracts.
* 449 pass / 0 fail across the full tracing + TUI suites (1 network
  flake in tracing-adversarial-2 unrelated to this change, passes on
  re-run).

What I traced before declaring done

* Inventoried every `sessionTraces.delete`, `sessionTraces.clear`,
  `endTrace()`, `Trace.create()`, `Trace.withExporters()`, and direct
  trace-file write across the whole repo.
* Confirmed `this.spans` is never reassigned mid-instance (only
  pushed) except by the new `rehydrateFromFile`.
* Confirmed no external callers of `trace.snapshot()` outside
  `tracing.ts`.

Post-fix, the only ways a new `Trace` instance can replace an existing
session's in-memory Trace are: worker boot (once), `setWorkspace` with
an actually-changed workspaceID (rare, also idempotency-guarded), and
MAX_TRACES eviction (uncommon at 100 sessions). All three now go
through `rehydrateFromFile` first.

* fix(tracing): capture user prompt via setPrompt; drop time.end gate

Per codex review (2026-06-05): the previous user-text branch in worker.ts
gated on `part.time?.end` (which user-input parts never have set) AND
called `trace.setTitle(text, text)` which would have regressed the
auto-generated session title from Path C (`session.updated`) back to
the raw user input ('Greeting' → 'hi') if the ordering went sideways.

* New `Trace.setPrompt(prompt)` method that only mutates `metadata.prompt`.
  Decouples prompt capture from title mutation.
* Path B in worker.ts now: (a) accepts user text parts without
  `part.time?.end` (it's an assistant-side concept only); (b) calls
  `setPrompt(text)` only — never `setTitle` — for user-identified
  messages. Assistant text still requires `time.end` for `logText`.
* Source-grep regression tests lock both contracts: no more
  `part.time?.end` on the user branch, no `setTitle` from the user
  branch, `setPrompt` exists and doesn't touch title.

* fix(tracing): record user messages as spans so chat tab renders multi-turn

The chat tab in the trace viewer renders 'metadata.prompt' as a single
'You' bubble at the top, then iterates 'kind: generation' spans for
assistant replies. There's no place for any user message beyond the
first to land — `setPrompt` overwrites on every call, and the viewer
only reads the latest value. Symptom: a 3-turn session shows only the
LAST user prompt followed by all earlier assistant responses, with the
older user messages dropped.

Fix splits the data and the rendering:

* New `Trace.logUserMessage(text)` pushes a `kind: 'user-message'`
  span with the user text as input. Snapshots immediately like other
  log* methods.
* New 'user-message' variant added to the TraceSpan.kind union.
* worker.ts Path B: now also calls `logUserMessage(text)` alongside
  `setPrompt(text)` for user-identified text parts (the first one
  populates metadata.prompt for the Summary tab; all of them populate
  per-turn spans for the Chat tab).
* viewer.ts chat-view: builds a chronologically sorted list of
  user-message + generation spans and walks it in startTime order.
  Older traces without user-message spans fall back to rendering
  metadata.prompt as before.
* Behavioral test in tracing-rehydrate.test.ts proves the spans are
  written, ordered chronologically, and preserve the user text.

10 unit tests in worker-trace-clearing + 8 in tracing-rehydrate pass;
35 existing tracing tests pass unchanged; typecheck clean.

* docs: trace-bugs followup to #867 — what this branch actually fixes

* fix(tracing): address PR #895 bot review feedback

* Trace.rehydrateFromFile (cubic P2): restore traceId from the on-disk file
  so post-rehydrate snapshots/exports preserve trace identity across instance
  lifetimes. Without this, every snapshot after rehydration writes a fresh
  random traceId.
* Trace.rehydrateFromFile (cubic P2): normalize sessionId via the same
  sanitization buildTraceFile applies before comparing to trace.sessionId.
  Previously, sessions with /, \, ., or : in the id would be falsely
  rejected and recreated.
* viewer chat-view (cubic P2): always render metadata.prompt at the top
  unless an existing user-message span carries the same text. Pre-fix
  traces (only metadata.prompt) and mixed traces (rehydrated pre-fix data
  + new turns) now render the first user turn correctly. Previously, the
  fallback was gated on userMsgs.length === 0 and dropped the legacy
  first turn in mixed traces.
* worker-trace-clearing.test.ts (CodeRabbit, cubic P3): broaden the
  negative regression guards to catch all three flagged bug spellings —
  inline expression, inline ternary, block body with if — for the
  workspaceID effect; and to reject part.time?.end nested inside the
  user-text branch (identified by sessionUserMsgIds.get(...).has(...)).
* routes/session/index.tsx: paraphrased the bug-shape literal that was
  in a comment so the broadened test regex doesn't catch our own
  documentation as the bug.
* tracing-rehydrate.test.ts: behavioral tests for the traceId
  preservation and sanitized-sessionId match.

Skipped CodeRabbit's tmpdir-fixture-style suggestion — the convention
isn't followed by the existing tracing tests (tracing-display-crash,
tracing-rename-race) so changing this one file alone would be inconsistent
and out of scope for this PR.

48 affected tests pass; typecheck clean.

* test(tracing): migrate tracing-rehydrate to tmpdir() fixture

CodeRabbit pointed out that `packages/opencode/test/AGENTS.md` documents
`tmpdir()` from `fixture/fixture.ts` as the project convention. My
earlier reply skipped the migration on the grounds that sibling tracing
tests don't follow the convention — but for code I'm adding fresh, the
right move is to follow the documented standard. The sibling tests
predate it and should be swept in a separate cleanup PR.

Replaces the manual `os.tmpdir() + beforeEach/afterEach` pattern with
`await using tmp = await tmpdir()` per test. Helpers `makeTrace` and
`readTraceFile` now take `dir` as a first parameter so each test
threads its own tmp directory through. 46 affected tests pass.

* fix(viewer): dedupe metadata.prompt against truncated user-message input

cubic flagged that `Trace.logUserMessage` slices user text at 4000 chars
when persisting to the span, while `metadata.prompt` keeps the full
string. The strict equality check in viewer chat-view (`u.input ===
t.metadata.prompt`) misses the dedupe for prompts longer than 4000 chars
and renders the same text twice — once as the top-level "You" fallback
bubble, once as the first user-message span.

Match against both the full and the truncated form. Same fix shape cubic
suggested.

* docs: remove redundant spec/trace-bugs-followup-867.md

The PR description already carries the bug-by-bug origin table and the
explanation of why #867's scope was disjoint from these bugs. Keeping a
295-line spec file in the repo to say the same thing again is bloat —
the PR is the right place for this content.

* fix(tracing): synthetic-part gate, in-flight gen interrupt, shared truncation constant

Three follow-ups from the multi-LLM consensus review of PR #895, codex-validated:

* Worker user-text branch now skips parts with `synthetic` or `ignored`
  set (Major #1 in the review, codex-confirmed). `Session.createUserMessage`
  in prompt.ts attaches many synthetic text parts to the user messageID for
  MCP resource banners, decoded file contents, retry/reminder text,
  plan-mode reminders, and agent-handoff tags. Without the gate they pass
  the `sessionUserMsgIds.has(messageID)` check, `metadata.prompt` ends up
  holding the LAST synthetic part (typically a file blob), and the chat tab
  renders one fake "▶ You" bubble per synthetic span — defeating the two
  display surfaces this PR fixes. Gated on an `isAuthoredText` predicate
  so the symmetric assistant-text branch is also protected. Continue via
  predicate rather than `continue` keyword so the outer event loop still
  forwards the event downstream via `Rpc.emit`.

* `Trace.rehydrateFromFile` now marks any open generation span as
  interrupted (Major #2). The transient state `logStepStart` populated
  (`currentGenerationSpanId`, `generationText`, `generationToolCalls`,
  `pendingToolResults`) is memory-only and can't be reconstructed from
  disk. If we leave open spans open, the next `step-finish` for that turn
  drops at the `!this.currentGenerationSpanId` guard and follow-up
  `logToolCall` mis-parents tool spans to the root, silently degrading the
  trace shape. Closing the span with `status: "error"` and a
  `statusMessage` describing the interruption preserves the partial data
  and makes the boundary visible in the viewer.

* Extracted the 4000-char truncation cap as `USER_MESSAGE_INPUT_MAX_CHARS`
  exported from tracing.ts (new cubic P3 on viewer.ts:1331). The viewer's
  chat-tab dedupe now interpolates the same constant so the two sides can't
  drift if the truncation cap ever changes. Also reused a single
  `Date.now()` call for the user-message span's start/end timestamps
  (cosmetic, addresses review nit #16).

Skipped from the review:
- Major #3 (no per-messageID dedupe in logUserMessage): codex confirmed
  user text doesn't stream/chunk — message.part.delta is assistant-only
  — so the symptom the review described is subsumed by Major #1's
  synthetic gate. No separate fix needed.
- Major #5 (Path A `setTitle(text, text)` couples title and prompt):
  codex grep-verified that no in-repo code path populates
  `userMsg.summary.title` or `summary.body`; the branch is inert.
  Cleanup risk only, tracked in #896.

Tests
- New behavioral test in tracing-rehydrate.test.ts asserts that an open
  generation span (no `step-finish` before reconstruction) ends up
  with `endTime` set, `status: "error"`, and a statusMessage matching
  `/interrupted/i` after rehydrate + a snapshot-triggering call.
- New source-grep test in worker-trace-clearing.test.ts locks both
  the synthetic-gate literal (`!part.synthetic && !part.ignored`) and
  the requirement that both `trace.setPrompt` and `trace.logUserMessage`
  sit inside the `isAuthoredText &&` guard.

42 affected tests pass; typecheck clean.

* test: tighten worker-trace-clearing regex to scope-bounded match (CodeRabbit)

CodeRabbit flagged that the previous source-grep assertions matched
across the entire `worker.ts` file, so unrelated code positioning
could satisfy them without the synthetic-gate fix being present in
the actual user-text branch.

* The `isAuthoredText` declaration check now asserts the const is
  built from BOTH flags (`!part.synthetic && !part.ignored`), not
  just that the literal exists somewhere.
* The two write-path checks now require `trace.setPrompt` and
  `trace.logUserMessage` to sit inside the same `if (text) { ... }`
  body within the `sessionUserMsgIds...has(part.messageID)` branch.
  The `[^{}]` bounds on the inner spans prevent the match from
  extending past the closing brace of that body, so calls elsewhere
  in the file can't false-green the assertion.

* fix(tracing): make rehydrateFromFile async to unblock the event-loop hot path

cubic flagged that `fsSync.readFileSync` in `rehydrateFromFile` blocks
the worker event loop during a trace cache miss. The hot path is
bounded (cache miss only fires on worker restart, MAX_TRACES eviction,
or initial-boot resumption of a stale session), but a multi-MB trace
file makes the pause visible.

* `Trace.rehydrateFromFile` now returns `Promise<boolean>` and uses
  the async `fs.readFile`.
* `getOrCreateTrace` in worker.ts becomes `async` and the three call
  sites in the event-stream loop now `await` it. The loop body was
  already async (`for await (const event of events.stream)`), so the
  conversion is local.
* Behavioural tests in `tracing-rehydrate.test.ts` converted to
  `await` the now-async method (7 call sites).
* The source-grep contract test for `getOrCreateTrace`'s
  rehydrate-before-startTrace shape now matches the `await` form.

Not addressed in this commit (will reply on PR):
- cubic also flagged a theoretical event-ordering race where
  `message.part.updated` could arrive before `message.updated`,
  leaving `sessionUserMsgIds` empty when the part handler runs. The
  producer (`Session.createUserMessage` → `updateMessage` THEN
  `updatePart` for each part) emits in order, and the consumer reads
  the event stream sequentially. The race is theoretical given the
  current producer; if we ever reorder, the defensive fix is to buffer
  unrouted parts by messageID. Out of scope for this PR.

---------

Co-authored-by: Haider <haider@altimate.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants