Skip to content

feat(context): bind a connection profile to a project#2251

Open
PaulChen79 wants to merge 21 commits intomainfrom
feat/proj-to-profile-structure
Open

feat(context): bind a connection profile to a project#2251
PaulChen79 wants to merge 21 commits intomainfrom
feat/proj-to-profile-structure

Conversation

@PaulChen79
Copy link
Copy Markdown
Contributor

@PaulChen79 PaulChen79 commented May 8, 2026

Summary

  • New wren context set-profile <name> command writes both profile: and data_source: into wren_project.yml, locking a project to a specific connection profile
  • dry-plan / dry-run now consult the project's pinned profile before falling back to the global active one — multi-project setups stop accidentally hitting the wrong DB
  • wren context validate hints when no profile is bound and warns when the bound profile has been removed
  • onboarding skill + connect / quickstart / installation docs reordered to profile-first so users leave onboarding with a fully bound project

Why

Today, the CLI's only profile mechanism is the global active: field in ~/.wren/profiles.yml. That's fine for one-project-per-machine, but it breaks the moment you have two Wren projects pointing at different databases — wren profile switch becomes a footgun and the SDK already supports profile: in wren_project.yml while the CLI ignores it. This PR closes that gap with a single new command (set-profile) and a small resolver helper, so the project↔profile mapping is now an explicit first-class fact in the project file.

The SDK's existing 3-tier resolution (kwarg → project pin → active) is already aligned with the new CLI behavior — no SDK changes needed.

Design decisions (locked during planning)

  • Minimal surface area: only set-profile is new. init, profile add, and profile switch are unchanged.
  • set-profile semantics: always overwrites data_source from the bound profile (single source of truth at bind time). Mismatched MDL is surfaced later by validate / build rather than blocking at bind.
  • Onboarding ordering: mkdir → fill .env → profile add → context init → set-profile → MDL gen — profile-first so the new step slots in cleanly after profile validation.
  • validate hint tone: info (not warning) when no profile bound, so error output stays uncluttered for projects with real problems.

Commit breakdown

Commit Purpose
feat(profile): add resolve_profile_for_project helper Pure addition — 3-tier resolver, no call sites
refactor(cli): honor project-pinned profile in dry-plan and dry-run Wire the resolver into existing CLI sites; behavior-equivalent for unpinned projects
feat(context): add wren context set-profile command The new command + save_project_config() helper
feat(context): hint about set-profile in validate when no profile bound UX nudge + missing-pin warning
docs(skill): reorder wren-onboarding to profile-first + add set-profile step Skill version 2.0 → 2.1
docs: document project↔profile binding in connect.md New Step 5 in the connect guide
docs: align installation and quickstart with profile-first onboarding Loose ends in the other onboarding-adjacent docs

Test plan

  • 22 new unit tests covering: resolver fallback, pinned-profile-missing error, set-profile first-bind, set-profile rebind, set-profile preserves other fields, validate hints, validate strict on missing pin
  • pytest core/wren/tests/ — 364 passed, 1 pre-existing failure (test_validate_strict_warns) unrelated to this PR
  • ruff check clean on every changed source/test file
  • Manually walked the new onboarding flow in an isolated WREN_HOME (set-profile binds, validate goes silent, dry-plan honors the pin)
  • Reviewer: skim the connect.md / quickstart.md diffs for tone/clarity
  • Reviewer: confirm the SKILL.md ordering reads well end-to-end (Step 2 → 3 → 3.5 → 3.6 → 4)

Out of scope (deliberate)

  • init --profile X flag — onboarding flow now creates the profile before init runs, so the flag has no use case
  • profile add cwd detection — set-profile already covers post-add binding without polluting the global command
  • profile switch warning — keeps the global command pure
  • SDK changes — the SDK was already reading profile: correctly
  • Eager connection validation in WrenToolkit.from_project() — separate SDK improvement, tracked separately

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added wren context set-profile to bind a connection profile to a project; CLI now prefers a project-bound profile for project-aware commands.
  • Bug Fixes / Improvements

    • CLI commands (including dry-plan) use project-bound profiles when present.
    • Validation now warns on missing/mismatched pinned profiles.
    • Stable, round-trip-friendly saving of project config.
  • Documentation

    • Updated connect, installation, and quickstart docs; onboarding skill bumped to v2.1 and expanded to cover .env and binding.
  • Tests

    • Added unit and integration tests for profile resolution and binding behaviors.

PaulChen79 added 7 commits May 8, 2026 12:23
3-tier resolver for project-aware profile lookup: read the project's
`profile:` field from wren_project.yml first, fall back to the global
active profile when unset. Raises SystemExit when the project pins a
profile that no longer exists, so users get an actionable error
instead of silent fallback.

No call sites yet — pure addition. Wires up in the next commit.
Both commands now consult the project's `profile:` field (via the new
resolve_profile_for_project helper) before falling back to the global
active profile. Behavior is unchanged for projects without the field.

The new _resolve_engine_profile helper handles both --mdl-derived and
cwd-discovered project paths, so users get consistent project-aware
resolution regardless of how they invoke the command.
New subcommand binds a connection profile to a project: writes
`profile: <name>` plus `data_source: <profile.datasource>` into
wren_project.yml. Both first-bind and rebind always overwrite the
data_source from the profile (single source of truth at bind time);
mismatched MDL is surfaced later by validate / build.

Adds save_project_config() to wren.context for stable, ordered yaml
writeback. Comments are dropped (we use PyYAML, not ruamel) — the
project file's only comments are init-template copy and aren't worth
a new dep.
validate now reports two new conditions:
- pinned profile that no longer exists → warning (and error in --strict)
- no profile pin at all → friendly info pointing to set-profile

The no-pin info only fires when validation otherwise passes so error
output stays uncluttered for projects with real problems.
…le step

Step 2 now stops at workspace + .env (no project scaffolding yet);
Step 3 creates the profile (unchanged); new Step 3.5 scaffolds the
project; new Step 3.6 binds the profile via the new set-profile
command, deriving data_source from the validated profile so users
never edit wren_project.yml by hand.

Bumps skill version 2.0 -> 2.1.
Adds a new Step 5 walking through `wren context set-profile`,
explaining why explicit binding matters (multi-project safety) and
when re-binding requires MDL regeneration. Renumbers downstream
steps (Generate MDL, Index memory) to fit.
installation.md described the wren-onboarding skill as
"project scaffolding, profile setup" — pre-change ordering. Updated
both the skill summary and narrative to reflect the new flow:
.env → profile → project → bind.

quickstart.md Step 5 now includes the `wren context set-profile`
binding step so users following the tutorial leave with a fully
bound project, not one that silently falls back to global active.
@github-actions github-actions Bot added documentation Improvements or additions to documentation python Pull requests that update Python code core skills labels May 8, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 8, 2026

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

This PR adds project-scoped profile binding: new resolver honors wren_project.yml's pinned profile, a wren context set-profile command writes profile and data_source into the project file, CLI engine construction and dry-plan use project-aware resolution, validation checks and save helper added, tests and docs updated.

Changes

Project Profile Binding

Layer / File(s) Summary
Project Config Persistence
core/wren/src/wren/context.py
Adds _PROJECT_FIELD_ORDER constant and save_project_config() to write wren_project.yml with stable, human-friendly key ordering.
Profile Resolution Strategy
core/wren/src/wren/profile.py
Adds resolve_profile_for_project(project_path) that prefers a project-pinned profile: in wren_project.yml, falls back to the global active profile, returns (None,{}) when unset, and raises SystemExit when a pinned profile name is missing.
CLI Profile Resolution Integration
core/wren/src/wren/cli.py
Adds _resolve_engine_profile(mdl) helper and updates _build_engine() and dry-plan to use project-aware profile resolution when no explicit connection flags provided; preserves datasource expansion and existing error paths.
Set-Profile Binding Command
core/wren/src/wren/context_cli.py
New wren context set-profile <name> subcommand: validates profile exists and has datasource, updates wren_project.yml (profile, data_source), and reports old→new datasource transition.
Validation Profile Checks
core/wren/src/wren/context_cli.py
Enhanced wren context validate: loads project config, warns when pinned profile name is missing from local profiles, and prints a set-profile hint only when validation is otherwise clean and no profile is bound.
Profile Resolution Tests
core/wren/tests/test_profile.py
Adds _write_project() helper and tests for resolve_profile_for_project: pinned profile, active fallback, missing-pin error, unset behavior, and empty-string pin handling.
CLI Profile Resolution Tests
core/wren/tests/unit/test_cli_profile_resolve.py
New unit tests for _resolve_engine_profile: mdl path resolution, CWD discovery, fallback to active profile, non-file mdl handling, and SystemExit on missing pinned profile; includes profile isolation fixture.
Set-Profile & Validation Integration Tests
core/wren/tests/unit/test_context_cli.py
Integration tests for set-profile (binding, overwriting placeholders, rebinding, error cases, field preservation, datasource transition output) and enhanced context validate behaviors (hints, warnings, strict mode).
Documentation & Onboarding
docs/core/get_started/connect.md, docs/core/get_started/installation.md, docs/core/get_started/quickstart.md, skills/wren-onboarding/SKILL.md, skills/index.json, skills/versions.json
Adds explicit "Bind the profile to your project" step and guidance; updates quickstart and installation text; bumps onboarding skill version 2.0 → 2.1 and expands workflow text to include binding and .env handling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • goldmedal
  • douenergy

🐰 I hopped through configs, tidy and spry,
I pinned a profile so projects won't sigh,
Each wren_project.yml now keeps its key,
Bound connections snug, stable as can be,
Hooray — happy builds for you and me!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.59% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main feature: adding a command to bind a connection profile to a project.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/proj-to-profile-structure

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.

CI's skills/check-versions.sh enforces parity between SKILL.md
frontmatter, versions.json, and index.json. The earlier commit only
bumped SKILL.md; sync the registries and refresh the index.json
description to match the new profile-first flow.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

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

⚠️ Outside diff range comments (1)
core/wren/src/wren/cli.py (1)

466-468: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update datasource-missing error text to reflect project-pinned resolution.

At Line 467, the message says “active profile,” but this branch can also be using a project-pinned profile. The wording is now misleading during troubleshooting.

Suggested fix
-                typer.echo("Error: no datasource in active profile.", err=True)
+                typer.echo(
+                    "Error: no datasource in resolved profile (project-pinned or active).",
+                    err=True,
+                )
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/wren/src/wren/cli.py` around lines 466 - 468, The error message in the
prof_ds is None branch (the block using prof_ds, typer.echo, and raise
typer.Exit) is misleading because it only mentions "active profile" even when a
project-pinned profile may be in use; update the typer.echo string to reference
the current profile more accurately (e.g., "Error: no datasource in the current
profile (active or project-pinned)." or similar) so the message reflects both
active and project-pinned profile contexts while leaving the rest of the branch
(raise typer.Exit(1)) unchanged.
🧹 Nitpick comments (1)
core/wren/tests/unit/test_context_cli.py (1)

431-461: ⚡ Quick win

Consider adding a test for a profile that exists but has no datasource field

The error path at context_cli.py lines 595–601 (profile found, datasource key absent) has no test coverage. This is the third validation gate in set_profile and the only one not exercised.

✅ Suggested test
def test_set_profile_errors_when_profile_has_no_datasource(tmp_path, monkeypatch):
    """Profile exists but has no datasource field → exit non-zero, message printed."""
    import wren.profile as profile_mod  # noqa: PLC0415

    _isolate_profiles(tmp_path / "wren-home", monkeypatch)
    # Add a profile that is deliberately missing the datasource key
    profile_mod.add_profile("nodatasource", {})

    proj = tmp_path / "myproj"
    runner.invoke(app, ["context", "init", "--empty", "--path", str(proj)])

    result = runner.invoke(
        app, ["context", "set-profile", "nodatasource", "--path", str(proj)]
    )
    assert result.exit_code != 0
    assert "datasource" in result.output
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/wren/tests/unit/test_context_cli.py` around lines 431 - 461, Add a new
unit test named test_set_profile_errors_when_profile_has_no_datasource in
core/wren/tests/unit/test_context_cli.py that creates an isolated profile store
via _isolate_profiles and uses profile_mod.add_profile("nodatasource", {}) to
register a profile missing the datasource key, initializes an empty project
(runner.invoke(app, ["context", "init", "--empty", "--path", str(proj)])), then
calls the CLI set-profile command for "nodatasource" and asserts the command
exits non‑zero and that the output contains the word "datasource" to exercise
the branch in set_profile (context_cli.py) that handles profiles without a
datasource.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@core/wren/src/wren/context_cli.py`:
- Around line 310-317: The call to list_profiles() in the validate logic can
raise (YAML/IO) and is currently unguarded; wrap the call to
wren.profile.list_profiles() in a try/except around the block that checks
profile_pin so exceptions are caught, and on failure append a user-friendly
semantic warning to sem_warnings (or skip the profile existence check) instead
of letting the exception propagate; reference the validate code path where
profile_pin is checked and the list_profiles() call so you locate and surround
it with try/except and include the caught exception message in the sem_warnings
entry.

In `@core/wren/src/wren/profile.py`:
- Around line 234-244: The YAML parse error is currently swallowed (except
yaml.YAMLError sets config = {}) causing silent fallback to
get_active_profile(); change this to fail-fast by re-raising or throwing a
descriptive exception when yaml.safe_load(project_yml.read_text()) raises
yaml.YAMLError. In practice, replace the except block around yaml.safe_load with
code that raises a RuntimeError or a custom exception that includes the
project_yml name/content and the original error, so the caller (and any use of
pinned_name) cannot silently proceed on malformed wren_project.yml.

---

Outside diff comments:
In `@core/wren/src/wren/cli.py`:
- Around line 466-468: The error message in the prof_ds is None branch (the
block using prof_ds, typer.echo, and raise typer.Exit) is misleading because it
only mentions "active profile" even when a project-pinned profile may be in use;
update the typer.echo string to reference the current profile more accurately
(e.g., "Error: no datasource in the current profile (active or project-pinned)."
or similar) so the message reflects both active and project-pinned profile
contexts while leaving the rest of the branch (raise typer.Exit(1)) unchanged.

---

Nitpick comments:
In `@core/wren/tests/unit/test_context_cli.py`:
- Around line 431-461: Add a new unit test named
test_set_profile_errors_when_profile_has_no_datasource in
core/wren/tests/unit/test_context_cli.py that creates an isolated profile store
via _isolate_profiles and uses profile_mod.add_profile("nodatasource", {}) to
register a profile missing the datasource key, initializes an empty project
(runner.invoke(app, ["context", "init", "--empty", "--path", str(proj)])), then
calls the CLI set-profile command for "nodatasource" and asserts the command
exits non‑zero and that the output contains the word "datasource" to exercise
the branch in set_profile (context_cli.py) that handles profiles without a
datasource.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 8415e045-6777-4314-8a7a-89893f39307a

📥 Commits

Reviewing files that changed from the base of the PR and between 628cf5f and 7afcd1b.

📒 Files selected for processing (11)
  • core/wren/src/wren/cli.py
  • core/wren/src/wren/context.py
  • core/wren/src/wren/context_cli.py
  • core/wren/src/wren/profile.py
  • core/wren/tests/test_profile.py
  • core/wren/tests/unit/test_cli_profile_resolve.py
  • core/wren/tests/unit/test_context_cli.py
  • docs/core/get_started/connect.md
  • docs/core/get_started/installation.md
  • docs/core/get_started/quickstart.md
  • skills/wren-onboarding/SKILL.md

Comment thread core/wren/src/wren/context_cli.py Outdated
Comment thread core/wren/src/wren/profile.py
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@skills/index.json`:
- Line 11: The "description" field in skills/index.json references the stale
docs/get_started/connect.md path; update that string to
docs/core/get_started/connect.md so the onboarding description points to the
correct file (edit the "description" value in skills/index.json to replace
docs/get_started/connect.md with docs/core/get_started/connect.md).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 7f33715b-0640-4e5c-814a-6c5ad7135275

📥 Commits

Reviewing files that changed from the base of the PR and between 7afcd1b and 9a0a3e5.

📒 Files selected for processing (2)
  • skills/index.json
  • skills/versions.json
✅ Files skipped from review due to trivial changes (1)
  • skills/versions.json

Comment thread skills/index.json Outdated
PaulChen79 added 6 commits May 8, 2026 14:20
resolve_profile_for_project() was silently treating a YAML parse
failure as missing config and falling back to the global active
profile, defeating the whole point of an explicit project-pin.
Raise SystemExit with the offending path and parser message instead
so users can fix the typo deterministically.

Addresses CodeRabbit finding on profile.py:240.
…rors

When ~/.wren/profiles.yml is unreadable (permissions, malformed YAML
that escaped _load_raw, etc.), list_profiles() can raise and crash
validate with a Python traceback. Wrap the lookup so we surface the
failure as a warning and let the rest of validate continue running.

Addresses CodeRabbit finding on context_cli.py:317.
Since dry-plan / dry-run now consult the project pin first, the
'no datasource in active profile' message is misleading when a
pinned profile is missing the datasource field. Update copy to
'resolved profile (project-pinned or active)'.

Addresses CodeRabbit finding on cli.py:467.
The onboarding skill description pointed at docs/get_started/connect.md
but the actual file lives under docs/core/get_started/connect.md.

Addresses CodeRabbit finding on index.json:11.
…field

The third validation gate in set_profile (profile exists but has no
datasource) had no test. Code already handles it correctly; this
test locks the behavior.

Addresses CodeRabbit nitpick on test_context_cli.py:431-461.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@core/wren/src/wren/context_cli.py`:
- Around line 594-618: The set_profile flow calls list_profiles() and
save_project_config(project_path, config) without protection; wrap the
list_profiles() call in a try/except mirroring validate (catch yaml.YAMLError
and IOError/OSError) and on failure emit a user-friendly typer.echo error and
raise typer.Exit(1); likewise wrap save_project_config(...) in a try/except
catching OSError/IOError (and any write-related exceptions), emit a clear
typer.echo error message and raise typer.Exit(1) so malformed
~/.wren/profiles.yml and disk/write errors produce clean CLI errors instead of
raw tracebacks.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 817735af-8f32-40c3-8bdf-e2e622d96058

📥 Commits

Reviewing files that changed from the base of the PR and between 9a0a3e5 and e2a0882.

📒 Files selected for processing (6)
  • core/wren/src/wren/cli.py
  • core/wren/src/wren/context_cli.py
  • core/wren/src/wren/profile.py
  • core/wren/tests/test_profile.py
  • core/wren/tests/unit/test_context_cli.py
  • skills/index.json
🚧 Files skipped from review as they are similar to previous changes (5)
  • core/wren/src/wren/profile.py
  • skills/index.json
  • core/wren/tests/test_profile.py
  • core/wren/src/wren/cli.py
  • core/wren/tests/unit/test_context_cli.py

Comment thread core/wren/src/wren/context_cli.py Outdated
set_profile mirrored the exact pattern validate had — bare list_profiles()
and an unguarded save_project_config() — so a malformed profiles.yml or a
write failure (disk full, permissions) would leak as a Python traceback.
Wrap both in try/except and surface as clean CLI errors.

Addresses CodeRabbit finding on context_cli.py:594-618.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@core/wren/tests/unit/test_context_cli.py`:
- Around line 647-658: The test currently asserts no OSError was raised and that
user-visible output exists but doesn't lock the CLI exit contract; update the
assertion for the result produced by runner.invoke(app, ["context", "validate",
"--path", str(tmp_path)]) to assert a deterministic non-zero exit code (e.g.,
assert result.exit_code == 1) so changes to error handling will fail the test;
modify the existing test near the result variable and the list_profiles
binding-check expectation to include this explicit exit-code assertion.
- Around line 371-372: The test performs multiple setup invocations using
runner.invoke(app, ["context", "init", ...]) and similar init/rebind calls
without asserting their results; change each unasserted runner.invoke call
(e.g., the initial runner.invoke(app, ["context", "init", "--empty", "--path",
str(proj)]) and other context init/rebind calls) to capture the result into a
variable and assert the invocation succeeded (for example assert
result.exit_code == 0 and/or assert expected output in result.output) so setup
failures are surfaced immediately instead of masking later test failures; update
all occurrences noted in the review (the initial init and the first rebind plus
the other listed invocations) to follow this pattern.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: a952ad1d-70ff-48c0-bfec-ed2136cb5ec9

📥 Commits

Reviewing files that changed from the base of the PR and between 7411301 and 219a2bb.

📒 Files selected for processing (2)
  • core/wren/src/wren/context_cli.py
  • core/wren/tests/unit/test_context_cli.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • core/wren/src/wren/context_cli.py

Comment thread core/wren/tests/unit/test_context_cli.py Outdated
Comment thread core/wren/tests/unit/test_context_cli.py
- Add _invoke_ok() helper that asserts exit_code 0; route the 9
  context-init / setup-rebind invocations through it so a failed
  scaffold step surfaces immediately instead of masking the real
  assertion failure further down.
- Lock the exit-code contract for test_validate_handles_list_profiles_exception:
  warning-only path stays exit 0; --strict makes the same probe failure
  exit 1. Pinning both ends prevents either direction silently flipping.

Addresses CodeRabbit findings on test_context_cli.py:372 and :658.
Copy link
Copy Markdown
Collaborator

@goldmedal goldmedal left a comment

Choose a reason for hiding this comment

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

Solid PR overall — the project-pin design closes a real footgun in multi-project setups, and CodeRabbit's earlier findings are all addressed in commits 9–15. CI is green across lint/unit/memory/mysql/postgres/skill-version-parity. A few notes before merge, one of which I'd consider a follow-up worth pursuing.

1. _resolve_engine_profile couples --mdl parsing with project detection (medium)

core/wren/src/wren/cli.py:148-161: the resolver uses --mdl's path as the primary project-detection signal and only walks cwd when --mdl is absent. This re-introduces the same class of footgun the PR is trying to close.

Current behavior matrix:

--mdl cwd in project A Result
/tmp/external.json (not in any project) yes global active ⚠️ — A's pin silently bypassed
<base64-string> yes global active ⚠️ — same bypass
/other-proj/target/mdl.json yes other-proj pin
(omitted) yes A's pin

The first two rows are exactly the kind of "wrong DB silently chosen" problem this PR is designed to eliminate — just triggered by --mdl shape instead of by a stale global active.

Suggested rewrite — decouple, then fall back only when no project context exists at all:

def _resolve_engine_profile(mdl: str | None) -> tuple[str | None, dict]:
    project_path = _discover_project_for_engine(mdl)
    if project_path is None:
        return get_active_profile()
    return resolve_profile_for_project(project_path)


def _discover_project_for_engine(mdl: str | None) -> Path | None:
    # 1. If --mdl is a real file, walk up looking for wren_project.yml.
    #    Don't hard-code parent.parent — `target/mdl.json` is the build
    #    default, not a contract. Users may run `--mdl <project>/build/mdl.json`.
    if mdl is not None:
        mdl_path = Path(mdl).expanduser()
        if mdl_path.exists() and mdl_path.is_file():
            for parent in mdl_path.resolve().parents:
                if (parent / "wren_project.yml").exists():
                    return parent
                if parent == Path.home() or parent == parent.parent:
                    break
    # 2. Always try cwd discovery, regardless of whether --mdl was given.
    try:
        return discover_project_path()
    except SystemExit:
        return None

Two changes in one:

  • Walk up from --mdl instead of hard-coding parent.parent — the <project>/target/mdl.json layout is a build default, not a contract.
  • Always fall through to cwd discovery when --mdl doesn't carry project context. --mdl becomes a pure "which MDL artifact to load" override; project (and therefore profile) is determined independently. Active profile is reserved for "no project context anywhere" — its actual fallback role.

This also enables a coherent use case the current code blocks: running wren dry-plan --mdl /tmp/experiment.json from inside project A to test an external MDL against A's connection, without wren profile switch first.

If you'd rather keep the current "self-contained --mdl" semantics, please at least print a stderr line when falling back to active despite cwd being in a project — silent bypass is the worst of both worlds.

2. No bind-time hint when re-binding overwrites data_source and MDL is already built (low/medium)

core/wren/src/wren/context_cli.py:619-630: when old_ds != new_ds, the success line shows data_source: postgres -> duckdb, but there's no warning that an existing target/mdl.json was built for the old dialect. docs/core/get_started/connect.md does call this out, but the user only sees the doc if they read it; the CLI is silent.

Cheap UX win: when old_ds != new_ds AND (project_path / "target" / "mdl.json").exists(), append:

⚠ MDL was built for <old_ds>. Run `wren context build` to regenerate before querying.

3. set_profile hardcodes the project filename (nit)

core/wren/src/wren/context_cli.py:524 / :589: the literal "wren_project.yml" is duplicated instead of using wren.context._PROJECT_FILE. Either expose the constant (drop the underscore) or accept the duplication — current state is mildly inconsistent with the rest of context.py.

4. save_project_config round-trip not tested with custom fields (nit)

core/wren/src/wren/context.py:432-449: the field-reorder logic correctly appends unknown keys at the end, but test_set_profile_preserves_other_fields only verifies the standard fields (name, catalog, schema, schema_version). One round-trip with a custom field (e.g. tags: [foo, bar]) would lock the "extra keys are preserved" contract — cheap insurance against a future re-shuffle of _PROJECT_FIELD_ORDER accidentally dropping fields.

5. No-pin info hint suppressed when warnings exist (intentional, but worth confirming)

core/wren/src/wren/context_cli.py:344-359: the "no profile bound" hint only fires when validation has zero errors AND zero warnings. The comment says this is to keep error output uncluttered — fine if the goal is "don't pile noise on warnings," but it means a project with description warnings + no profile pin never sees the hint. Tests don't catch this because they assert on clean projects.

Worth a deliberate call: should the hint be unconditional once the structural check passes? Or always after the warning summary, on its own line so it's clearly scoped? Whichever you pick, please add a one-line comment so the next reader doesn't think it's an oversight.


Items #1 and #2 I'd address before merge; #3#5 are fine as follow-ups.

PaulChen79 added 4 commits May 8, 2026 18:57
The previous resolver only consulted cwd when --mdl was absent, so
`--mdl <base64>` or `--mdl /external.json` from inside a pinned project
silently bypassed the pin and fell back to global active — exactly the
silent-mismatch class this PR set out to close, just triggered by --mdl
shape instead of by a stale active.

- Extract _discover_project_for_engine() so --mdl becomes a pure MDL
  artifact override; project context is determined by walking up from
  the MDL path AND from cwd, in that order.
- Walk up directory tree from --mdl rather than hard-coding parent.parent
  — <project>/target/mdl.json is a build default, not a contract.
- Active profile fires only when neither discovery finds a project.

Addresses goldmedal review #1.
When re-binding to a profile whose datasource differs from the
existing one AND target/mdl.json is already built, the manifest was
emitted for the previous dialect and queries will fail until the
user rebuilds. The doc mentioned this; the CLI was silent. Now the
success summary includes a one-line warning pointing at
`wren context build`.

Addresses goldmedal review #2.
Drop the leading underscore on context.PROJECT_FILE so context_cli
(in the same package) can reference it without the underscore-private
signal. Replace 3 hardcoded "wren_project.yml" literals — init,
set-profile presence check, and set-profile error message — with the
constant. Comment / help-text mentions stay as plain strings.

Addresses goldmedal review #3.
save_project_config's _PROJECT_FIELD_ORDER appends unknown keys at
the end after the known fields. Existing test_set_profile_preserves_other_fields
only checked the standard keys (name, catalog, schema, schema_version);
this new case round-trips a custom `tags` list and `owner` string so a
future re-shuffle of the field order can't silently drop user metadata.

Addresses goldmedal review #4.
…an projects

Previously the no-pin hint only fired when validation had zero errors
AND zero warnings — so anyone working through description warnings
never saw the binding nudge. Move the hint out of the success block
into a separate post-warning section, gated only on no hard errors.
Placed before the exit raise so --strict (which converts warnings to
exit 1) still surfaces the hint.

Adds two tests: hint visible under warning state, hint suppressed
under hard error.

Addresses goldmedal review #5.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core documentation Improvements or additions to documentation python Pull requests that update Python code skills

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants