Skip to content

fix(v0.1.10): resolve open review comments from PRs #147/#149 + release prep - #150

Closed
db-tycoon-stephen wants to merge 7 commits into
v0.1.10from
fix/v0.1.10-review-followups
Closed

fix(v0.1.10): resolve open review comments from PRs #147/#149 + release prep#150
db-tycoon-stephen wants to merge 7 commits into
v0.1.10from
fix/v0.1.10-review-followups

Conversation

@db-tycoon-stephen

@db-tycoon-stephen db-tycoon-stephen commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Addresses every unresolved review comment from PR #147 (my review) and PR #149 (Gemini), plus the v0.1.10 release-prep machinery, targeting the release branch.

Review fixes (PR #147)

  • Stale AI-agent hints removed from live pathsai_hint() (printed tycoon ask chat … on every failed ingestion / dbt test) deleted along with its 6 call sites; data observability scaffold no longer suggests tycoon ask sync; dead nao_dir config property, scaffolded .gitignore nao entries, and stale Nao comments/docstrings dropped
  • Stale-string sentinel updated_STALE_SUBSTRINGS now guards tycoon ask / register llm / nao / Nao, so this class of drift fails tests from now on
  • pandas resolved empirically — removing it broke tests/test_e2e_demo_arc.py: dlt's on-demand filesystem source needs it for CSV reads. Kept, pinned ==3.0.3 to match the exact-pin convention, with a comment naming the real consumer (extra treatment stays tracked in feat: make pandas an optional extra with proper CI matrix #148)
  • Coverage floor corrected 55 → 68 — real coverage after the deletion is ~73% (up from ~65%), so the floor moves up, keeping the same 5-point drift headroom as the old 65/60 split
  • tycoon start shutdown catches (OSError, subprocess.TimeoutExpired) instead of blanket Exception (PR fix(v0.1.9): address PR #59 review feedback (robustness polish) #69 convention)
  • quack asserted in the PORTS-keys test

Review fixes (PR #149)

  • ingestion-architecture.md command names aligned to the tycoon data sources namespace (upgrade, catalog) per Gemini's consistency comments

Release prep

Test plan

  • uv run pytest → 611 passed, 2 skipped (includes new sentinel entries)
  • uv run pytest --cov → 73.3% ≥ new 68% floor
  • uvx ruff check src tests → clean
  • uv run --extra docs mkdocs build --strict → clean
  • uv run tycoon --version → 0.1.10
  • grep -rniE 'tycoon ask|register llm|dagster|nao' README.md docs/ (excl. releases/proposals) → only generic-English hits

🤖 Generated with Claude Code


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.

db-tycoon-stephen and others added 6 commits July 15, 2026 18:41
…view)

ai_hint() pointed every failed ingestion and dbt test at the removed
'tycoon ask chat'; observability scaffold suggested 'tycoon ask sync'.
Also drops the dead nao_dir config property, the scaffolded .gitignore
nao entries, and stale Nao mentions in comments/docstrings.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…Exception (PR #147 review)

Same convention as the PR #69 fixes: specific exception types so real
bugs aren't silently swallowed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sert quack port (PR #147 review)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…coverage floor to 68

- pandas==3.0.3, pinned like every other core dep; nothing in src/
  imports it but dlt's on-demand filesystem source needs it for CSV
  reads (tests/test_e2e_demo_arc.py proves it). Extra treatment: #148.
- Coverage floor: deleting the untested server/dagster code lifted
  real coverage to ~73%, so the floor moves UP to 68 (5-point drift
  headroom, same margin as the old 65/60 split) — not down to 55.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cope-doc status note

- CHANGELOG [0.1.10] UNRELEASED section + docs/releases/v0.1.10.md
  (Released: TBD — date flips at ship time per release procedure)
- docs/commands/ask/ and recipes/lm-studio-local-llm.md deleted with
  the feature; mkdocs nav updated
- ingestion-architecture.md: align command names to the
  'tycoon data sources' namespace (PR #149 review)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…user-facing pages

Every page describing the removed surface now documents the actual
v0.1.10 CLI: 4-step wizard, two servers (rill + quack), no extras
beyond [docs]. Verified against src (cli.py command tree, start.py
_SERVER_NAMES, constants.PORTS, project.py StackConfig, pyproject
extras). docs/releases/ and docs/proposals/ untouched (historical
record). mkdocs build --strict clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@db-tycoon-stephen

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request transitions the database-tycoon CLI to version 0.1.10, focusing on streamlining the codebase by completely removing the FastAPI web UI, Dagster orchestration, and Nao/ask AI agent integrations. It introduces a typed MetadataBackend protocol with a DuckDB file backend for run history, pins pandas as a core dependency, and re-baselines the test coverage floor to 68%. The review feedback correctly suggests adding 'dagster' and 'Dagster' to the stale substring sentinel checks in tests to prevent accidental re-introduction of these terms.

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.

Comment thread tests/test_cli_surface.py
Comment on lines +45 to +48
("nao",
"removed in v0.1.10 — the Nao agent integration was dropped"),
("Nao",
"removed in v0.1.10 — the Nao agent integration was dropped"),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Since the Dagster integration has been completely removed in this release (along with the [dagster] extra and its associated commands), we should also add "dagster" and "Dagster" to the _STALE_SUBSTRINGS sentinel. This will ensure that any accidental re-introduction of Dagster-related user-facing strings or commands in the codebase will be caught by the tests.

Suggested change
("nao",
"removed in v0.1.10 — the Nao agent integration was dropped"),
("Nao",
"removed in v0.1.10 — the Nao agent integration was dropped"),
("nao",
"removed in v0.1.10 — the Nao agent integration was dropped"),
("Nao",
"removed in v0.1.10 — the Nao agent integration was dropped"),
("dagster",
"removed in v0.1.10 — the Dagster integration was dropped entirely"),
("Dagster",
"removed in v0.1.10 — the Dagster integration was dropped entirely"),

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request implements a major cleanup and refocusing of the codebase for version 0.1.10, referred to as "the great slimming." It removes the FastAPI web UI, Dagster orchestration, and Nao/ask AI agent integrations entirely, along with their respective optional dependency groups ([server], [dagster], and [ask]). Concurrently, it introduces the first milestone of the ingestion rewrite, adding a typed MetadataBackend protocol and a DuckDB file backend to handle run history. It also pins pandas as a core dependency, updates the test coverage floor to 68%, and cleans up all related documentation, CLI commands, and helper utilities. I have no feedback to provide as there are no review comments.

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.

#62/#64-#68/#47/#31/#53/#54)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@db-tycoon-stephen

Copy link
Copy Markdown
Contributor Author

Split per review feedback: docs-only sweep → #151, code fixes + release prep → #152. Branch kept for reference until both land.

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.

1 participant