feat(m1): wire run events into commands (Task 2/2) - #137
Conversation
Completes M1 Task 2/2. Four issues shipped in one commit: #95 — runner.py emits RunStarted before dispatch, RunCompleted after success, RunFailed on any exception. Helpers: _emit_event_safe (wraps DuckDBFileBackend, silently no-ops) and _build_run_completed (extracts rows from dlt load_info.load_packages). #97 — transform.py reads target/run_results.json after each dbt run/build/test and appends a DbtRunCompleted event. Wrapped in its own try/except so a missing results file never fails the build. #101 — status.py: import duckdb removed from top level; moved inside _query_run_counts() and _query_layer_last_build(). New _sources_from_backend(metadata_db) reads RunCompleted events to populate Last Sync and row counts in the Sources panel. _render_sources_panel() signature changes raw_db → metadata_db. #103 — history.py rewritten to use HistoryRepository (new core/history.py). No raw SQL, no top-level import duckdb. List view renders list[RunSummary]; show view renders RunDetail. _resolve_source_id replaces _resolve_source_schema — source_id in events IS the config key, no schema translation needed. --layer validates the manifest and filters to dbt runs; per-model filtering deferred to a later milestone. core/history.py: new module — RunSummary, RunDetail, HistoryRepository. list_runs() sorts all events newest-first; get_run() resolves by prefix across RunCompleted/RunFailed/DbtRunCompleted. tests/test_history.py: all history tests migrated to _seed_events() via DuckDBFileBackend. _seed_metadata() kept for TestStatusRunsColumn which still reads dlt_runs via _query_run_counts(). Tests that tested old trace/schema-change detail replaced with M1-appropriate equivalents. 32 new tests + 22 updated tests. All pass (1 pre-existing unrelated failure in test_nao_uninitialised_warn_points_at_register_llm excluded).
There was a problem hiding this comment.
Code Review
This pull request refactors the run history and status commands to utilize a new event-driven metadata backend and repository pattern, replacing direct DuckDB queries. The code review highlights a critical filter-after-limit bug in the history command that can result in empty lists when filtering. Additionally, several feedback items suggest robustness improvements to safely handle potentially missing or null attributes in events and JSON results, preventing potential runtime errors and type mismatches.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…nsive guards
Fix filter-after-limit bug (comment #3553429137/#3553429151):
list_runs() now accepts limit=None and returns the full sorted list;
_list_history() passes limit=None and slices after all filters are applied.
Eliminates the case where tool/source/layer filtering on a pre-truncated
list returns an empty result when there are matching runs beyond limit*3.
Defensive or-guards (comments #3553429151/#3553429156/#3553429163/#3553429171):
- core/history.py: e.rows_loaded or {} in list_runs() and get_run()
- core/history.py: event.rows_loaded or {}, event.tables_created or [] in RunDetail construction
- status.py: dict(e.rows_loaded or {}) in _sources_from_backend()
Safer run_results.json extraction (comment #3553429176):
- transform.py: float(... or 0.0), run_results.get('args') or {}, run_results.get('results') or []
prevents TypeError if any field is None rather than absent
…mpleted _build_run_completed was iterating pkg.jobs as a list, but dlt's LoadPackageInfo.jobs is a dict[TPackageJobState, list[LoadJobInfo]]. Even with that fixed, LoadJobInfo has no rows_count field, so counts would always be zero. Switch to pipeline.last_trace.last_normalize_info.row_counts which gives exact per-table item counts directly from dlt's normalize step.
How to test this branch locally1. Pull and install git fetch origin feat/m1-wire-run-events
git checkout feat/m1-wire-run-events
uv sync2. Create a test project mkdir /tmp/chess-test && cd /tmp/chess-test
cat > tycoon.yml << 'YAML'
name: chess-test
stack:
ingestion: dlt
transformation: none
sources:
chess:
type: rest_api
schema: raw_chess
config:
client:
base_url: https://api.chess.com/pub/
resources:
- name: player_profile
endpoint:
path: player/hikaru
- name: player_stats
endpoint:
path: player/hikaru/stats
YAMLNo API key needed — chess.com is a public API. 3. Run a source alias tycoon=/path/to/tycoon-cli/.venv/bin/tycoon # adjust to your checkout path
tycoon data sources run chessExpected: dlt loads two endpoints into 4. Check history tycoon data historyExpected: one row — tycoon data history show <8-char-prefix> # copy from Ref columnExpected: drilldown showing 5. Run it a second time to see multiple rows tycoon data sources run chess
tycoon data historyExpected: two ✓ rows in the list, newest first. 6. Check status tycoon data statusExpected: Sources panel shows dbt testing (requires an existing dbt project wired into tycoon.yml): After a tycoon data history --tool dbt
tycoon data history show <8-char-event-id>The drilldown won't show per-table rows (that's an M1 limitation — |
The filesystem read_csv() transformer names its output table _read_csv,
which was being dropped by the startswith('_') filter. Switch to an
explicit denylist of known dlt internal tables (_dlt_pipeline_state,
_dlt_loads, _dlt_version) so user tables with _ prefixes are preserved.
db-tycoon-stephen
left a comment
There was a problem hiding this comment.
Good shape overall. I verified all six Gemini findings are fixed in the current diff (filter-after-limit, limit=None support, and the four defensive defaults), CI is green, and the event emission is properly fail-safe on every path — RunStarted best-effort, RunFailed in the outer except with re-raise, everything through _emit_event_safe. That fully addresses the single-writer degradation concern carried over from the #136 review.
One substantive behavior change that deserves a deliberate decision (Sources panel row semantics), one stale help text, and two smaller cleanups — inline below.
Small observations, no action needed:
_DLT_INTERNAL_TABLESis an exact-match set of three names; at.startswith("_dlt")prefix filter would match the PR description's stated intent ("filters_dlt_*") and survive new dlt internals.- The #136 read-only-guard deferral rationale ("Task 2 only opens the backend for writes") turned out not to hold — this PR opens read-only in both status and history. No harm since both call sites guard for missing files, but it's a good sign that guard eventually wants to live in the backend itself.
- Stacking is handled correctly; just remember the rebase after #136's
uv.lockrestore lands.
- Use metadata_db_path() in runner.py instead of hand-building the path
- Switch _dlt internal table filter to startswith('_dlt') prefix so new
dlt internals are covered without updating an explicit denylist
- Narrow bare except on read-side history/show to emit a dim warning
instead of silently swallowing genuine backend errors
- Rename status Sources panel column to 'Last Sync Rows' — the events
backend returns rows moved in the most recent sync, not current table
totals; the new name makes that semantic explicit
- Update --layer help text to state the M1 limitation (all dbt runs
shown; per-model filtering comes in a later milestone)
- Rename test_layer_filter_restricts_to_invocations_touching_layer to
test_layer_filter_shows_all_dbt_runs_m1_limitation so it doesn't
imply layer filtering is actually implemented
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the run history and status tracking systems to use a new event-based metadata backend instead of querying DuckDB tables directly. It updates the ingestion runner, transform commands, and CLI commands to emit and query structured events, and migrates the test suite to use this new event-seeding mechanism. The review feedback highlights three key improvements: explicitly excluding 'skipped' dbt statuses from being counted as errors, validating and rejecting conflicting CLI filter combinations upfront to prevent confusing empty states, and refining prefix resolution to distinguish between non-existent and ambiguous run IDs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
- Escape raw exception strings with rich.markup.escape before interpolating into dim warning messages — OSError messages like '[Errno 13] Permission denied' would otherwise be parsed as Rich markup tags and raise MarkupError on the exact error path we just made visible - Add 'skipped' to dbt non-error statuses so skipped models don't inflate models_errored and incorrectly mark a run as failed - get_run() now raises ValueError for ambiguous prefixes (listing the matching run IDs) instead of silently returning None — CLI surfaces this as an actionable error message distinct from 'not found' - Reject --tool dlt + --layer and --tool dbt + --source upfront with a clear error instead of silently returning an empty list
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the history and status commands to use a new event-driven metadata backend and repository pattern instead of direct DuckDB queries, updating the ingestion runner and transform commands to emit structured run events. The review feedback highlights several key improvements: safely wrapping the construction and emission of RunCompleted events in the ingestion runner to ensure observability code never fails a successful pipeline run, raising explicit errors instead of swallowing database exceptions in the history commands to avoid misleading user feedback, and specifying UTF-8 encoding when reading dbt run results to prevent platform-specific decoding issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
- transform: record started_at before subprocess; skip stale run_results.json
(dbt crash leaves previous file on disk — mtime check prevents false event)
- runner: call raise_on_failed_jobs() on legacy and native/dynamic paths so
partial dlt job failures flip to RunFailed instead of silently succeeding
- status: derive Runs column from events backend instead of legacy dlt_runs
table; hint gate changed to check metadata DB existence; fix connection
leaks in _query_run_counts and _query_layer_last_build (context managers)
- history: escape source_id, run_id, command, and error in _show_run drilldown
to prevent Rich markup injection from user-controlled values
- core/history: guard get_run against empty prefix (startswith("") matches all)
|
Second round of fixes in cf6f190, addressing 7 bugs surfaced by an independent code review pass: Critical
Significant 4–5. Rich markup injection in
All 29 tests in |
- runner: add _emit_run_completed_safe wrapper so exceptions in _build_run_completed (e.g. unexpected load_info types) never propagate into the outer except block and misclassify a successful run as RunFailed - history: replace silent exception swallowing in list and show paths with error() + Exit(1); a corrupted/locked DB now surfaces a clear message instead of "No runs captured yet" - transform: explicitly open run_results.json with encoding="utf-8" for cross-platform safety
|
Addressed all three items from the latest Gemini review in dc5bd7d: High — Medium — history exception handling: Both Medium — |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the history and status commands to use a new event-based metadata backend, migrating direct DuckDB queries to the HistoryRepository and DuckDBFileBackend. It also introduces event emission (RunStarted, RunCompleted, RunFailed, and DbtRunCompleted) during ingestion and transformation processes, and updates the test suite accordingly. Feedback on the changes includes addressing a potential filesystem timestamp resolution issue when checking dbt run results to prevent fast runs from being discarded, and removing the now-unused _query_run_counts helper function in the status command module.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@JesuFemi-O — one flag on The history read-error change reverses a settled decision. Please revert those two hunks to the |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the run history and status commands to use a new event-based metadata backend instead of querying legacy DuckDB tables directly. Key feedback includes ensuring that observability capture runs even when ingestion jobs fail by swapping the execution order of raise_on_failed_jobs() and _capture_and_refresh_safe(). Additionally, it is recommended to add a small tolerance to the dbt run results timestamp check to account for coarse filesystem resolutions, wrap event queries in try-except blocks to gracefully handle missing tables on new or legacy databases, and clean up the unused _query_run_counts helper function.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…view) Reverts the error()+Exit(1) shape from dc5bd7d back to the d7bf39c behavior. A locked metadata DB during an active ingest is an expected transient state (DuckDB file locks are per-process), so history exiting non-zero on it is worse than a visible dim warning + empty state. This also degrades gracefully when the metadata DB exists but the events table doesn't yet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Update: I pushed |
|
sounds good @db-tycoon-stephen thanks! |
db-tycoon-stephen
left a comment
There was a problem hiding this comment.
All review asks verified landed at 7ddf7fa: _emit_run_completed_safe wrapper, escape() on all Rich interpolations, warn-and-continue restored on history read paths, filter-conflict rejection, ambiguous-prefix handling, utf-8, raise_on_failed_jobs on all three dispatch paths. Tests 84 passed (one known pre-existing failure, fails on main too), CI 7/7 green. Non-blocking items tracked in #146. Approved — retarget to main and merge after #136.
What this PR does
This is the second and final PR for M1. It wires the event infrastructure from Task 1/2 (PR #136) into the four live command paths.
Closes #95, #97, #101, #103.
Issue breakdown
#95 — Emit RunStarted / RunCompleted / RunFailed from
run_source()runner.pynow emits three lifecycle events around every dlt dispatch path:RunStartedbefore dispatch (best-effort; never blocks the run)RunCompletedafter a successful load withload_id,duration_seconds, androws_loadedextracted fromload_info.load_packagesRunFailedin the outerexceptblock so any pipeline exception is capturedTwo module-level helpers added:
_emit_event_safe(metadata_db, event)— opensDuckDBFileBackend, appends the event, swallows all exceptions_build_run_completed(name, load_info, elapsed)— extracts user-table row counts from dltload_packages(filters_dlt_*internal tables)The metadata DB path is resolved once via
config.rootinside atry/except, so it degrades silently in test environments whereconfig.rootis not available.#97 — Emit DbtRunCompleted from
_capture_dbt_and_refresh_safe()After each
dbt run/build/test,transform.pyreads<dbt_project_dir>/target/run_results.jsonand appends aDbtRunCompletedevent with:command,target,models_run,models_passed,models_errored,duration_secondsWrapped in its own
try/exceptso a missing results file never fails the build.#101 — Replace raw-DB queries in
status.pywith events backendimport duckdbremoved from the top of the file. It now appears only inside_query_run_counts()and_query_layer_last_build(), which still read from the old observability schema (needed for backward compat with the Runs column)._sources_from_backend(metadata_db)replaces_query_last_sync()and_query_row_counts(). It opensDuckDBFileBackendread-only, queriesevent_type = 'run_completed', and returns{source_id: {last_sync, rows}}. The Sources panel now reads freshness and row counts from the events table._render_sources_panel()signature changes:raw_db: Path→metadata_db: Path.#103 — Rewrite
history.pyusing HistoryRepository; addcore/history.pysrc/tycoon/core/history.py(new file):RunSummary— frozen dataclass:run_id,source_id,runtime_id,status,started_at,duration_seconds,rows_total,commandRunDetail— frozen dataclass:summary,rows_by_table,tables_created,errorHistoryRepository—list_runs(limit)returns sortedlist[RunSummary];get_run(prefix)returnsRunDetail | None(None on zero or ambiguous matches)src/tycoon/commands/history.py— full rewrite:import duckdbgone from the top level_load_dlt_rows,_load_dbt_rows,_resolve_id,_fmt_bytes,_show_dlt_run,_show_dbt_run_render_history_table(runs: list[RunSummary])— dbt rows usecommand (short_id)as Ref; dlt rows usesource_id/short_id_list_history()— usesHistoryRepository;--layervalidates manifest and filters to dbt runs (per-model filtering deferred:DbtRunCompleteddoes not carry per-node detail yet)_show_run()— usesHistoryRepository.get_run(); shows rows-by-table forRunCompleted, error forRunFailed--sourcefilter semantic change: events storesource_id = config_key(e.g."pokeapi", not"raw_pokeapi"), so filtering is direct with no schema translation.--layerM1 limitation: validates the layer and requires a compiled manifest, but shows all dbt runs rather than filtering to only those that touched the given layer. Per-node detail will be added toDbtRunCompletedin a later milestone.Tests
tests/test_history.pymigrated from_seed_metadata()(old schema) to_seed_events()(writes viaDuckDBFileBackend)._seed_metadata()kept forTestStatusRunsColumn, which validates the_query_run_counts()→ olddlt_runspath that is intentionally preserved.Updated/replaced tests:
test_show_dlt_surfaces_trace_details_when_present→test_show_dlt_surfaces_duration(events carryduration_seconds; trace bytes not in M1)test_show_dbt_surfaces_schema_changes_when_present→test_show_dbt_invocation_exits_zerotest_layer_filter_restricts_to_invocations_touching_layer— updated to reflect M1 layer-filter behaviortest_source_resolves_config_name_to_schema— assertion updated from"raw_pokeapi"to"pokeapi"82 tests pass across
test_events.py,test_metadata_contract.py,test_history.py, andtest_cli_surface.py. One pre-existing unrelated failure (test_nao_uninitialised_warn_points_at_register_llm) excluded.Stacking note
This PR targets
feat/m1-metadata-backend-protocol. Once PR #136 merges tomain, this PR will be retargeted tomain.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.