Externally managed tables config by callicles · Pull Request #3459 · 514-labs/moosestack

callicles · 2026-02-05T14:56:56Z

No description provided.

linear · 2026-02-05T14:57:00Z

ENG-2039 Create externally managed tables in local dev

vercel · 2026-02-05T14:57:01Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
docs-v2	Ready	Preview, Comment	Feb 7, 2026 0:49am

coderabbitai · 2026-02-05T14:57:05Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

🔍 Trigger a full review

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch nico/eng-2039-create-externally-managed-tables-in-local-dev

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 11

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

apps/framework-cli/src/utilities/constants.rs (1)
118-146: ⚠️ Potential issue | 🟡 Minor

Add rustdoc for new public constants (and the updated prompt).
Public API constants should use /// docs; the current line comment won’t appear in rustdoc.
Proposed fix
-// Keychain keys for remote ClickHouse credentials (separate from URL)
+/// Keychain key for remote ClickHouse username (separate from URL).
 pub const KEY_REMOTE_CLICKHOUSE_USER: &str = "remote_clickhouse_user";
-// Keychain keys for remote ClickHouse credentials (separate from URL)
+/// Keychain key for remote ClickHouse password (separate from URL).
 pub const KEY_REMOTE_CLICKHOUSE_PASSWORD: &str = "remote_clickhouse_password";

+/// Prompt shown when remote ClickHouse credentials are required for dev mirroring.
 pub const STORE_CRED_PROMPT: &str = r#"You have externally managed tables in your code base.
As per coding guidelines "Document all public APIs and breaking changes".

🤖 Fix all issues with AI agents

In `@apps/framework-cli/src/cli/routines/mod.rs`:
- Around line 618-621: The parse_clickhouse_connection_string(remote_url).ok()
call swallows parsing errors and causes silent fallback; change this to handle
the Result explicitly (match or map_err) so parsing failures are logged or
warned before proceeding. Locate parse_clickhouse_connection_string and the
creation of remote_ch (used by ClickHouseRemote::from_config and
create_external_mirrors) and, on Err(e), emit a clear warning or error including
remote_url and e (using the existing logger), then pass None or bail depending
on desired behavior; ensure the log message mentions remote_url, the parse
error, and that local schema creation will be used if you keep the fallback.
- Around line 498-512: The PR adds a new config flag
project.dev.externally_managed.tables.create_local_mirrors but the dev
environment docs lack any mention; update the dev-environment.mdx reference to
document the [dev.externally_managed.tables] table and the create_local_mirrors
boolean: describe what setting create_local_mirrors = true does (creates local
mirrors for externally managed tables in dev), list default/behavior when false,
and include a short example TOML snippet showing the config and a brief note on
when to toggle it; ensure the section mentions the config key exactly as
project.dev.externally_managed.tables.create_local_mirrors so readers can map
docs to the code.

In `@apps/framework-cli/src/cli/routines/seed_data.rs`:
- Around line 792-894: The code duplicates the "check exists, optionally
drop-if-refresh" logic between create_external_tables_from_local_schema and
create_single_mirror; extract a helper (e.g., ensure_table_absent_or_skipped)
that accepts (&ClickHouseClient, &str db, &str table_name, bool
refresh_on_startup) and returns a Result<enum {Skipped, Ready}, String> (or
Result<bool, String> where true=ready) so both functions call it instead of
repeating local_client.table_exists, local_client.drop_table_if_exists and
pushing format_warning/format_error; update
create_external_tables_from_local_schema and create_single_mirror to call this
helper, handle the Skipped/Ready result, and keep existing calls to
format_warning/format_error/format_success unchanged.

In `@apps/framework-cli/src/framework/core/infrastructure_map.rs`:
- Around line 2971-2982: get_mirrorable_external_tables currently returns values
from a HashMap which is non-deterministic; collect the filtered Table references
into a Vec, sort that Vec by a stable key (e.g., Table::name or a stable id
field) using a deterministic comparator (lexicographic on name or numeric on
id), then return the sorted Vec so mirror creation/logs are reproducible; update
get_mirrorable_external_tables to perform the collect -> sort -> return sequence
while still filtering by life_cycle == LifeCycle::ExternallyManaged and
engine.supports_select().

In `@apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs`:
- Around line 295-298: The SELECT query builder uses remote_database directly in
format!("SELECT * FROM `{}`.`{}` LIMIT 0", remote_database, table_name) without
validation; add the same identifier validation/sanitization used for
local_database and table_name (e.g., the validate_identifier or
sanitize_identifier helper used elsewhere) to verify or escape remote_database
before interpolation, and return an error if it contains invalid characters so
the constructed query is safe and consistent with the checks on
local_database/table_name.
- Around line 340-343: The call building select_query in insert_from_remote uses
remote_database without validation; add the same validation used elsewhere to
check remote_database (e.g., non-empty and matches the allowed identifier
pattern) before constructing select_query and return an error if invalid; update
the insert_from_remote function to validate remote_database (and reuse any
helper or regex used for table_name validation) so select_query =
format!("SELECT * FROM `{}`.`{}` LIMIT {}", remote_database, table_name, limit)
only runs with a sanitized/validated remote_database.

In `@apps/framework-cli/src/infrastructure/olap/clickhouse/config_resolver.rs`:
- Around line 1-14: Add unit tests for the public resolver behavior: write tests
exercising resolve_remote_clickhouse (or the public function that resolves
config) using ClickHouseRemote/RemoteClickHouseConfig inputs to cover (1) when
no config exists (defaults applied), (2) when config exists and is used, (3)
rejection of empty passwords (simulate prompt_password/KeyringSecretRepository
returning empty and assert failure), and (4) port defaulting logic (when
protocol is Protocol::Http vs ClickHouseProtocol ensure port defaults match
expected values). Mock or stub SecretRepository/KeyringSecretRepository and
prompt_user/prompt_password to control inputs and assertions.
- Around line 108-139: store_credentials currently can leave the username saved
if storing the password fails; update store_credentials to perform a rollback on
partial success: after repo.store(project_name, KEY_REMOTE_CLICKHOUSE_USER,
user) succeeds but repo.store(...KEY_REMOTE_CLICKHOUSE_PASSWORD...) returns Err,
call the repository deletion method (e.g., repo.delete or repo.remove) to remove
KEY_REMOTE_CLICKHOUSE_USER for that project, map any errors to RoutineFailure
similarly, and then return the original error; reference the function
store_credentials and the constants KEY_REMOTE_CLICKHOUSE_USER /
KEY_REMOTE_CLICKHOUSE_PASSWORD and the repo.store call to locate where to add
the rollback.

In `@apps/framework-cli/src/project.rs`:
- Around line 271-276: Add a Rust doc comment for the public field `tables` on
the `DevExternallyManagedTablesConfig` struct: describe that it holds the
configuration for externally managed tables used in development mode, note that
it defaults via serde (#[serde(default)]), and mention the contained type
`DevExternallyManagedConfig` so users know what configuration is expected and
how it is used.
- Around line 288-307: Update the doc comment on the
RemoteClickHouseConfig::port field to clarify that the numeric defaults are not
set on the struct but are applied later during resolution in config_resolver.rs;
specifically change the comment to something like "Optional port; resolved to
8443 for SSL or 8123 for non-SSL during config resolution" so readers know the
defaults are applied by the config resolver rather than by the struct itself.
- Around line 310-321: DevConfig exposes two user-facing options that are
undocumented: externally_managed and remote_clickhouse; update the
dev-environment documentation to add entries for dev.externally_managed (explain
it maps to DevConfig.externally_managed / DevExternallyManagedTablesConfig, what
it controls, default behavior and an example of enabling externally managed
tables) and dev.remote_clickhouse (explain it maps to
DevConfig.remote_clickhouse / RemoteClickHouseConfig, describe that it is an
optional read-only ClickHouse connection, what credentials are expected to be
provided externally, defaults, and an example config snippet). Ensure both
entries mention types, defaults, and short examples so users know how to set
them in their config.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 6f4eab1 and dd00c42.

📒 Files selected for processing (11)

apps/framework-cli/src/cli/routines/mod.rs
apps/framework-cli/src/cli/routines/seed_data.rs
apps/framework-cli/src/framework/core/infra_reality_checker.rs
apps/framework-cli/src/framework/core/infrastructure_map.rs
apps/framework-cli/src/framework/core/plan.rs
apps/framework-cli/src/framework/core/plan_validator.rs
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
apps/framework-cli/src/infrastructure/olap/clickhouse/config_resolver.rs
apps/framework-cli/src/infrastructure/olap/clickhouse/mod.rs
apps/framework-cli/src/project.rs
apps/framework-cli/src/utilities/constants.rs

🧰 Additional context used

📓 Path-based instructions (4)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Run cargo clippy to ensure Rust code passes Clippy's linting standards before each commit

**/*.rs: Use thiserror with #[derive(thiserror::Error)] for error handling in Rust; define errors near the fallibility unit, never use anyhow::Result
Use snake_case for functions and variables, PascalCase for types and traits, SCREAMING_SNAKE_CASE for constants in Rust
Place constants in constants.rs at the appropriate module level in Rust
Use tuple structs with validation constructors (newtypes) for Rust type safety (e.g., struct UserId(String))
Write inline tests with #[cfg(test)] modules in Rust
Documentation is required for all public APIs in Rust
Run cargo clippy --all-targets -- -D warnings before committing Rust code; no warnings allowed

Files:

apps/framework-cli/src/infrastructure/olap/clickhouse/mod.rs
apps/framework-cli/src/framework/core/infra_reality_checker.rs
apps/framework-cli/src/framework/core/plan_validator.rs
apps/framework-cli/src/utilities/constants.rs
apps/framework-cli/src/cli/routines/seed_data.rs
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
apps/framework-cli/src/cli/routines/mod.rs
apps/framework-cli/src/infrastructure/olap/clickhouse/config_resolver.rs
apps/framework-cli/src/framework/core/infrastructure_map.rs
apps/framework-cli/src/framework/core/plan.rs
apps/framework-cli/src/project.rs

apps/framework-cli/**/*.rs

📄 CodeRabbit inference engine (apps/framework-cli/AGENTS.md)

apps/framework-cli/**/*.rs: Always run cargo clippy --all-targets -- -D warnings before commits; fix all warnings - no Clippy warnings may remain (treat warnings as errors)
Use rustfmt --edition 2021 for consistent formatting
Write meaningful names for functions, variables, and types
Document all public APIs and breaking changes
Use thiserror crate instead of anyhow::Result for error handling
Define errors near their unit of fallibility (no global Error types)
Use #[derive(thiserror::Error)] with #[error()] messages for error definitions
Define newtypes as tuple structs: struct UserId(u64);
Add validation constructors for newtypes: UserId::new(id: u64) -> Result<Self, Error>
Derive standard traits for newtypes: #[derive(Debug, Clone, PartialEq)]
Implement From/TryFrom for newtype conversions
Use derive_more or nutype to reduce newtype boilerplate
Use const for static values (prefer over static)
Use UPPER_SNAKE_CASE naming for constants
Scope constant visibility with preference: pub(crate) > pub(super) > pub
Group related constants together
Write unit tests for all public functions
Test error conditions and edge cases

Files:

apps/framework-cli/src/infrastructure/olap/clickhouse/mod.rs
apps/framework-cli/src/framework/core/infra_reality_checker.rs
apps/framework-cli/src/framework/core/plan_validator.rs
apps/framework-cli/src/utilities/constants.rs
apps/framework-cli/src/cli/routines/seed_data.rs
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
apps/framework-cli/src/cli/routines/mod.rs
apps/framework-cli/src/infrastructure/olap/clickhouse/config_resolver.rs
apps/framework-cli/src/framework/core/infrastructure_map.rs
apps/framework-cli/src/framework/core/plan.rs
apps/framework-cli/src/project.rs

**/framework-cli/src/**

⚙️ CodeRabbit configuration file

**/framework-cli/src/**: When reviewing changes to Moose CLI:

Check if any user-facing changes were made (commands, flags, configs, apis, etc)

If yes, verify the documentation for THAT SPECIFIC feature is updated in apps/framework-docs-v2/content

If docs for that feature doesn't exist yet, it should be added. If the change removes public apis, the documentation for those should also be removed. Changing unrelated docs doesn't satisfy this requirement.

In addition to reviewing for docs discrepancies, you should also review the code as per usual guidelines.

Files:

apps/framework-cli/src/infrastructure/olap/clickhouse/mod.rs
apps/framework-cli/src/framework/core/infra_reality_checker.rs
apps/framework-cli/src/framework/core/plan_validator.rs
apps/framework-cli/src/utilities/constants.rs
apps/framework-cli/src/cli/routines/seed_data.rs
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
apps/framework-cli/src/cli/routines/mod.rs
apps/framework-cli/src/infrastructure/olap/clickhouse/config_resolver.rs
apps/framework-cli/src/framework/core/infrastructure_map.rs
apps/framework-cli/src/framework/core/plan.rs
apps/framework-cli/src/project.rs

apps/framework-cli/**/constants.rs

📄 CodeRabbit inference engine (apps/framework-cli/AGENTS.md)

Place constants in constants.rs at deepest common module level

Files:

apps/framework-cli/src/utilities/constants.rs

🧠 Learnings (9)

📓 Common learnings

Learnt from: callicles
Repo: 514-labs/moosestack PR: 3143
File: apps/framework-cli/src/cli/routines/seed_data.rs:614-641
Timestamp: 2025-12-17T23:48:30.295Z
Learning: In apps/framework-cli/src/cli/routines/seed_data.rs, the create_mirror_schema function uses CREATE TABLE ... AS SELECT ... LIMIT 0 to create dev mirrors for EXTERNALLY_MANAGED tables. This intentionally does not preserve column codecs, TTL expressions, or materialized column definitions from the remote table, which is acceptable for local dev-only mirrors where matching column types is sufficient for materialized view sources.