Skip to content

test(cli): remove network-dependent session export case - #11012

Merged
marius-kilocode merged 1 commit into
mainfrom
fix/remove-session-export-worker-e2e
Jun 8, 2026
Merged

test(cli): remove network-dependent session export case#11012
marius-kilocode merged 1 commit into
mainfrom
fix/remove-session-export-worker-e2e

Conversation

@marius-kilocode

@marius-kilocode marius-kilocode commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

The base branch now explicitly opts this test into its custom loopback endpoint, so it no longer falls back to the production session-export service. That fixes the production traffic risk, but the worker case still tests persistence by relying on an upload failure.

Worker shutdown deliberately flushes pending events. Successful uploads and terminal responses remove their rows from SQLite. The test checks for those rows after shutdown, so it passes only because 127.0.0.1:1 refuses the upload and leaves the rows queued. Its persistence assertion is therefore coupled to a specific transport failure rather than to the storage behavior it intends to verify.

Remove only this worker case and its private fixtures. Focused tests already cover storage ordering, persisted metadata, secret scrubbing, upload outcomes, and shutdown behavior. The two workspace capture integration cases remain. This preserves the useful integration coverage without retaining a worker test that conflates persistence with failed delivery.

@kilo-code-bot

kilo-code-bot Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Incremental review against the previous pass at bd179a286d. The PR has one commit (fd4b7a27) — no new changes since the last review. Findings are unchanged.

The PR cleanly removes a network-dependent E2E test case that was flaky by design: the worker persistence assertion was coupled to a transport failure (127.0.0.1:1 refusing uploads) rather than to actual storage behaviour. The removal is well-scoped — only the problematic test, its worker init/shutdown helpers (ready(), shutdown()), its DB-read helper (rows()), and its fixture factory (events()) are deleted. The two remaining workspace-capture integration tests and all shared helpers they depend on are preserved intact.

Other Observations (not in diff)

Issues found in unchanged code that cannot receive inline comments:

File Line Issue
packages/opencode/test/kilocode/session-export/e2e.test.ts 63 The until() error message still reads "timed out waiting for worker rows" — a stale reference from the removed worker-DB path. The remaining tests use until() to wait for posted envelopes, not DB rows. Low-priority rename opportunity for a future cleanup.
Files Reviewed (1 file)
  • packages/opencode/test/kilocode/session-export/e2e.test.ts

Reviewed by claude-4.6-sonnet-20260217 · 246,693 tokens

Review guidance: REVIEW.md from base branch main

@marius-kilocode
marius-kilocode force-pushed the fix/remove-session-export-worker-e2e branch from bd179a2 to fd4b7a2 Compare June 8, 2026 15:53
@marius-kilocode
marius-kilocode enabled auto-merge June 8, 2026 16:07
@marius-kilocode
marius-kilocode merged commit 0925eba into main Jun 8, 2026
19 checks passed
@marius-kilocode
marius-kilocode deleted the fix/remove-session-export-worker-e2e branch June 8, 2026 16:12
t7tran pushed a commit to t7tran/kilocode that referenced this pull request Aug 14, 2026
…port-worker-e2e

test(cli): remove network-dependent session export case
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.

2 participants