fix(cli): implement --execution-env-priority on project context set #1135

2026-03-23T13:17:41Z

hamza.khyari commented

2026-03-23 13:17:41 +00:00

Summary

Implement the missing --execution-env-priority flag on project context set per issue #1079 / spec WF17.

Changes

project.py: Added execution_env_priority: str | None field to ContextConfig domain model
project_context.py: Added --execution-env-priority CLI flag with fallback/override validation against ExecutionEnvPriority enum. Persists in context_policy_json blob alongside execution_environment. Added display in project context show output.

Behavior

project context set local/myproj --execution-env-priority override validates and persists
project context set local/myproj --execution-env-priority invalid rejects with error
Setting priority preserves previously set execution_environment (merged blob)
project context show displays both environment and priority in a new section

Quality Gates

Session	Result
`nox -s typecheck`	PASS (0 errors)
`nox -s lint`	PASS
New tests	9 scenarios, 26 steps PASS

Closes #1079

## Summary Implement the missing `--execution-env-priority` flag on `project context set` per issue #1079 / spec WF17. ### Changes - **`project.py`**: Added `execution_env_priority: str | None` field to `ContextConfig` domain model - **`project_context.py`**: Added `--execution-env-priority` CLI flag with `fallback`/`override` validation against `ExecutionEnvPriority` enum. Persists in `context_policy_json` blob alongside `execution_environment`. Added display in `project context show` output. ### Behavior - `project context set local/myproj --execution-env-priority override` validates and persists - `project context set local/myproj --execution-env-priority invalid` rejects with error - Setting priority preserves previously set `execution_environment` (merged blob) - `project context show` displays both environment and priority in a new section ### Quality Gates | Session | Result | |---|---| | `nox -s typecheck` | PASS (0 errors) | | `nox -s lint` | PASS | | New tests | 9 scenarios, 26 steps PASS | Closes #1079

hamza.khyari added the

Type

Bug

label 2026-03-23 13:33:59 +00:00

hamza.khyari self-assigned this 2026-03-23 13:34:43 +00:00

hamza.khyari added the

State

In Review

label 2026-03-23 14:21:04 +00:00

freemo approved these changes 2026-03-24 15:25:32 +00:00

Dismissed

freemo left a comment

Review: APPROVED with Comments

Good implementation that correctly addresses the core issue — the critical blob-merge fix (loading existing context_policy_json before writing) prevents previously-set fields from being silently dropped. The PR follows existing patterns, has a clean CHANGELOG entry, and includes 9 BDD scenarios. Approving, but please note the following items.

Issues to Address

No milestone assigned. The branch is prefixed bugfix/m8-*, suggesting it belongs to Milestone 8. Per CONTRIBUTING.md requirement #11, a milestone is mandatory. Please assign before merge.
Merge conflicts. mergeable: false — needs a rebase against master.
Test fidelity gap. The BDD step step_call_set_with_priority re-implements the enum validation and persistence logic inline rather than calling the actual context_set CLI function. If the real CLI code diverges from this simulation, the tests will still pass. Consider invoking the actual CLI code path (via CliRunner) for at least one scenario to ensure the wiring is correct.
Loose domain typing. execution_env_priority: str | None in ContextConfig (project.py) should be ExecutionEnvPriority | None to enforce invariants at the model level. Currently, validation is only at the CLI boundary, meaning any code that constructs ContextConfig directly can set arbitrary strings.

Minor Notes

No test for context_show output. The new display section added to the show command is untested.
The merge dict ordering {**existing, **policy.model_dump(), **extra_fields} means policy.model_dump() defaults could overwrite previously persisted non-default settings. This is pre-existing behavior (not a regression from this PR), but worth noting for a future hardening pass.

## Review: APPROVED with Comments Good implementation that correctly addresses the core issue — the critical blob-merge fix (loading existing `context_policy_json` before writing) prevents previously-set fields from being silently dropped. The PR follows existing patterns, has a clean CHANGELOG entry, and includes 9 BDD scenarios. Approving, but please note the following items. ### Issues to Address 1. **No milestone assigned.** The branch is prefixed `bugfix/m8-*`, suggesting it belongs to Milestone 8. Per CONTRIBUTING.md requirement #11, a milestone is mandatory. Please assign before merge. 2. **Merge conflicts.** `mergeable: false` — needs a rebase against master. 3. **Test fidelity gap.** The BDD step `step_call_set_with_priority` re-implements the enum validation and persistence logic inline rather than calling the actual `context_set` CLI function. If the real CLI code diverges from this simulation, the tests will still pass. Consider invoking the actual CLI code path (via `CliRunner`) for at least one scenario to ensure the wiring is correct. 4. **Loose domain typing.** `execution_env_priority: str | None` in `ContextConfig` (project.py) should be `ExecutionEnvPriority | None` to enforce invariants at the model level. Currently, validation is only at the CLI boundary, meaning any code that constructs `ContextConfig` directly can set arbitrary strings. ### Minor Notes 5. No test for `context_show` output. The new display section added to the show command is untested. 6. The merge dict ordering `{**existing, **policy.model_dump(), **extra_fields}` means `policy.model_dump()` defaults could overwrite previously persisted non-default settings. This is pre-existing behavior (not a regression from this PR), but worth noting for a future hardening pass.

hamza.khyari force-pushed bugfix/m8-project-exec-env-priority from d9c61e28ce to 46951e7243

2026-03-27 14:29:50 +00:00

Compare

hamza.khyari dismissed freemo's review 2026-03-27 14:29:50 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

hamza.khyari force-pushed bugfix/m8-project-exec-env-priority from 46951e7243 to 411a80c643

2026-03-27 16:05:35 +00:00

Compare

hamza.khyari force-pushed bugfix/m8-project-exec-env-priority from 411a80c643 to 8a8a949106

2026-03-27 16:23:27 +00:00

Compare

freemo approved these changes 2026-03-27 17:10:14 +00:00

Dismissed

freemo left a comment

Review: fix(cli): implement --execution-env-priority on project context set

Approved with comments. Solid implementation with decent test coverage.

Issues to Address

1. No milestone assigned (Medium)
Per CONTRIBUTING.md, every PR must be assigned to a milestone.

2. Unrelated merge commits (Medium)
Branch contains 10 unrelated merge commits from master. Should be rebased.

3. BDD tests don't invoke the actual CLI (Medium)
The test steps inline the validation logic (ExecutionEnvPriority(priority.lower())) and directly manipulate the DB rather than using CliRunner to invoke the actual context_set Typer command. This means the command wiring is untested — a regression in argument binding would be missed. At least one scenario should use CliRunner.

4. Type safety gap (Low)
ContextConfig.execution_env_priority is str | None instead of ExecutionEnvPriority | None. The CLI validates via the enum, but the domain model accepts any string.

What's Good

Well-structured implementation: new --execution-env-priority option with clear help text.
Validates against ExecutionEnvPriority enum with proper error message and typer.Exit(1).
_load_policy_json null-coalescing (or {}) prevents NoneType errors on first use.
existing = _load_policy_json(...) preserves previously set fields — critical and correctly implemented.
context_show updated to display the new field.
9 BDD scenarios covering domain model, enum, CLI validation, and persistence round-trip.

## Review: fix(cli): implement --execution-env-priority on project context set **Approved with comments.** Solid implementation with decent test coverage. ### Issues to Address **1. No milestone assigned (Medium)** Per CONTRIBUTING.md, every PR must be assigned to a milestone. **2. Unrelated merge commits (Medium)** Branch contains **10 unrelated merge commits** from master. Should be rebased. **3. BDD tests don't invoke the actual CLI (Medium)** The test steps inline the validation logic (`ExecutionEnvPriority(priority.lower())`) and directly manipulate the DB rather than using `CliRunner` to invoke the actual `context_set` Typer command. This means the command wiring is untested — a regression in argument binding would be missed. At least one scenario should use `CliRunner`. **4. Type safety gap (Low)** `ContextConfig.execution_env_priority` is `str | None` instead of `ExecutionEnvPriority | None`. The CLI validates via the enum, but the domain model accepts any string. ### What's Good - Well-structured implementation: new `--execution-env-priority` option with clear help text. - Validates against `ExecutionEnvPriority` enum with proper error message and `typer.Exit(1)`. - `_load_policy_json` null-coalescing (`or {}`) prevents NoneType errors on first use. - `existing = _load_policy_json(...)` preserves previously set fields — critical and correctly implemented. - `context_show` updated to display the new field. - 9 BDD scenarios covering domain model, enum, CLI validation, and persistence round-trip.

freemo requested review from hurui200320 2026-03-28 21:28:06 +00:00

freemo requested changes 2026-03-28 23:21:52 +00:00

Dismissed

freemo left a comment

Day 48 Planning Review — Bug Fix PR for #1079

The implementation adds --execution-env-priority to project context set with 9 BDD scenarios. The single-commit structure and Conventional Changelog format are correct.

Blocking issues:

Merge conflicts (mergeable: false) — rebase onto master required.
No milestone assigned — CONTRIBUTING.md requires a milestone on all PRs. The branch name says m8- but the linked bug #1079 is in v3.5.0. Set the PR milestone to match the issue milestone.
Test fidelity gap — As @freemo noted in reviews #2709 and #2788, the BDD steps re-implement validation logic inline instead of invoking the actual CLI via CliRunner. This means a wiring regression (e.g., the CLI flag not being passed through to the service) would not be caught. At minimum, add one end-to-end scenario that exercises the real CLI path.
Domain typing — ContextConfig.execution_env_priority is str | None instead of ExecutionEnvPriority | None. Per the spec and CONTRIBUTING.md's type safety requirements, the domain model should use the enum type. CLI boundary validation is correct but the domain should enforce it too.
No @tdd_expected_fail removal in diff — Verify whether bug #1079 has a TDD counterpart with an expected-fail test. If so, it must be removed in this PR. If no TDD test exists, that's a separate concern.

Requested changes: Rebase, set milestone, add CLI integration test, address typing concern.

**Day 48 Planning Review — Bug Fix PR for #1079** The implementation adds `--execution-env-priority` to `project context set` with 9 BDD scenarios. The single-commit structure and Conventional Changelog format are correct. **Blocking issues:** 1. **Merge conflicts** (`mergeable: false`) — rebase onto master required. 2. **No milestone assigned** — CONTRIBUTING.md requires a milestone on all PRs. The branch name says `m8-` but the linked bug #1079 is in v3.5.0. Set the PR milestone to match the issue milestone. 3. **Test fidelity gap** — As @freemo noted in reviews #2709 and #2788, the BDD steps re-implement validation logic inline instead of invoking the actual CLI via `CliRunner`. This means a wiring regression (e.g., the CLI flag not being passed through to the service) would not be caught. At minimum, add one end-to-end scenario that exercises the real CLI path. 4. **Domain typing** — `ContextConfig.execution_env_priority` is `str | None` instead of `ExecutionEnvPriority | None`. Per the spec and CONTRIBUTING.md's type safety requirements, the domain model should use the enum type. CLI boundary validation is correct but the domain should enforce it too. 5. **No `@tdd_expected_fail` removal in diff** — Verify whether bug #1079 has a TDD counterpart with an expected-fail test. If so, it must be removed in this PR. If no TDD test exists, that's a separate concern. **Requested changes:** Rebase, set milestone, add CLI integration test, address typing concern.

hurui200320 requested changes 2026-03-30 04:03:25 +00:00

hurui200320 left a comment

PR Review: !1135 (Ticket #1079)

Verdict: Request Changes

The PR adds the --execution-env-priority flag to project context set with validation, persistence, display in context_show, and propagation to plan use. The commit structure (single atomic commit, Conventional Changelog format) is clean and the core idea is correct. However, the review uncovered 2 novel critical correctness bugs, several unaddressed issues from previous reviews, and significant test quality gaps that prevent merging.

Critical Issues

C1. Data loss in context_set: _write_policy() clobbers previously stored fields before merge read

File: src/cleveragents/cli/commands/project_context.py, lines 643 → 676–688
Problem: Line 643 calls _write_policy(session_factory, project, policy, acms), which completely replaces the JSON blob with policy.model_dump() + acms_config, stripping any previously stored top-level keys like execution_environment and execution_env_priority. Then on line 678, existing = _load_policy_json(...) reads the already-clobbered blob, so the "merge" on line 679 has nothing to merge with.
Concrete scenario: User runs project context set local/proj --execution-environment container, then later project context set local/proj --execution-env-priority override. The second call clobbers execution_environment at line 643 before the merge logic on line 678 can preserve it. Any context set call without --execution-environment silently loses a previously-set environment.
Note: This bug is masked by the BDD test "Setting priority does not clear execution_environment" (scenario on line 56) because that test bypasses the actual CLI code path and directly manipulates the DB.
Recommendation: Read existing = _load_policy_json(...) before line 643's _write_policy() call, or consolidate into a single write that builds the full blob (policy + acms + extra_fields + preserved existing keys) before writing once.

C2. Propagated project-level priority is overwritten by fallback default in plan.py

File: src/cleveragents/cli/commands/plan.py, lines 1704–1734 (propagation) vs. lines 1791–1794 (fallback)
Problem: The propagation block (lines 1724–1732) correctly sets plan.execution_env_priority to the project-level value (e.g., OVERRIDE). However, the existing fallback logic on lines 1791–1794 then unconditionally overwrites it:
```
elif execution_environment:
    plan.execution_env_priority = ExecutionEnvPriority.FALLBACK
```
The condition checks the CLI parameter execution_environment, not the plan's current value. Since the user provided --execution-environment but not --execution-env-priority, the elif fires and replaces the propagated OVERRIDE with FALLBACK.
Recommendation: Guard the fallback default: elif execution_environment and plan.execution_env_priority is None:. Or move the propagation block to after the existing CLI-override handling so it only fills gaps.

C3. BDD and Robot E2E tests do not exercise actual CLI code paths

File: features/steps/project_exec_env_priority_steps.py, lines 137–147; robot/helper_wf17_project_exec_env_priority.py, lines 16–60
Problem: The BDD step step_call_set_with_priority re-implements the enum validation and direct DB persistence inline rather than invoking the actual context_set Typer command. The Robot E2E helpers only validate ExecutionEnvPriority enum construction and JSON round-trips in-memory. Neither test suite exercises the actual CLI wiring, argument parsing, persistence path, or display logic. This means bugs C1 and C2 above are not caught by any test.
Note: This was raised by @freemo in reviews #2709, #2788, and #2892, and remains unaddressed. The project has 884+ existing uses of CliRunner in BDD steps — this PR deviates from the established testing convention.
Recommendation: Refactor CLI-facing BDD scenarios to use CliRunner to invoke the actual context_set command (with a mocked DI container), consistent with established patterns. Rewrite Robot helpers to invoke the actual CLI binary via subprocess.

Major Issues

M1. Plan propagation logic (AC3) is completely untested

File: features/project_exec_env_priority.feature, lines 60–63
Problem: The scenario "Project-level priority propagates to plan when no plan-level override" only checks blob persistence — identical to what other persistence scenarios already test. The actual propagation code in plan.py (lines 1704–1734) that reads project-level settings and applies them to plan objects is never exercised. AC3 of ticket #1079 has zero test coverage.
Recommendation: Add a scenario that sets up a project with execution_env_priority = "override", then invokes plan use (or its internals) without --execution-env-priority, and asserts the plan object has the propagated value.

M2. context_show omits execution environment fields in non-rich output formats

File: src/cleveragents/cli/commands/project_context.py, lines 749–756 vs. 792–805
Problem: The data dict used for JSON/YAML/plain output does not include execution_environment or execution_env_priority. Only the rich output path reads these from the raw blob. project context show --format json will not display the new fields, partially violating AC2 ("retrievable via project context show").
Recommendation: Add these fields to the data dict by reading the raw blob before the format branching.

M3. Domain type safety gap: ContextConfig.execution_env_priority is str | None

File: src/cleveragents/domain/models/core/project.py, line 272
Problem: The Plan model correctly uses ExecutionEnvPriority | None (plan.py:699), but ContextConfig uses str | None. Any code constructing ContextConfig directly can set arbitrary strings. Per CONTRIBUTING.md's type safety requirements and the project's Pyright strict mode, the domain model should enforce the enum constraint.
Note: Raised in previous reviews, unaddressed.
Recommendation: Change to execution_env_priority: ExecutionEnvPriority | None = Field(default=None, ...).

M4. Bare except Exception: pass violates error handling guidelines

File: src/cleveragents/cli/commands/plan.py, lines 1733–1734
Problem: Silently swallows all exceptions including AttributeError, TypeError, database corruption, and import failures. Creates a partial-state risk: if plan.execution_environment = proj_env succeeds but ExecutionEnvPriority(proj_priority) fails, the plan has an environment set but no priority. Per CONTRIBUTING.md: "Do not suppress errors" and "No Silent Failures."
Recommendation: Narrow to expected exceptions and log: except (ImportError, KeyError, ValueError) as exc: logger.debug("Project context propagation skipped: %s", exc).

M5. execution_environment not validated during propagation

File: src/cleveragents/cli/commands/plan.py, lines 1721–1723
Problem: proj_env is read from the JSON blob and assigned directly to plan.execution_environment without validating against the ExecutionEnvironment enum. In contrast, proj_priority IS validated on line 1729. If the blob contains a corrupted or invalid environment string, it propagates an invalid value to the plan.
Recommendation: Validate proj_env through ExecutionEnvironment(proj_env) before assignment.

M6. No milestone assigned on PR

File: PR metadata
Problem: PR milestone is null but ticket #1079 is in milestone v3.5.0. Per CONTRIBUTING.md: "Every PR must be assigned to the same milestone as its linked issue(s)."
Note: Raised in previous reviews, unaddressed.
Recommendation: Assign milestone v3.5.0.

M7. Merge conflicts — branch needs rebase

File: PR metadata (mergeable: false)
Problem: Branch has diverged from master and cannot be merged cleanly. Per CONTRIBUTING.md: branches must be aligned via rebase, never merge.
Note: Raised in previous reviews, unaddressed.
Recommendation: Rebase onto latest master.

M8. context_show execution environment display is untested

File: src/cleveragents/cli/commands/project_context.py, lines 792–805
Problem: The new code displaying the "Execution Environment" section in rich output has no BDD or Robot test coverage.
Recommendation: Add a scenario that invokes context_show via CliRunner and asserts the output contains the priority value.

M9. No @tdd_issue_1079 test exists in codebase

File: N/A (searched entire codebase — zero matches)
Problem: Ticket subtask says "Remove @tdd_expected_fail from WF17 project-level override test case" (checked off), but no tdd_issue_1079 tag exists anywhere. Per CONTRIBUTING.md, a bug fix PR without a corresponding TDD test may be blocked by CI.
Recommendation: Verify whether a TDD test was expected. If it never existed, add a note in the PR explaining why.

Minor Issues

m1. Double-write + redundant DB read in context_set when extra_fields present

File: src/cleveragents/cli/commands/project_context.py, lines 643, 676–688
Problem: When --execution-environment or --execution-env-priority is provided, the function writes the blob twice (line 643 and line 679) with 3 reads + 2 writes and 5 separate sessions. The first write is immediately overwritten.
Recommendation: Consolidate into a single write. (Resolving C1 naturally fixes this.)

m2. Triple DB read in context_show

File: src/cleveragents/cli/commands/project_context.py, lines 739, 740, 794
Problem: _read_policy, _read_acms_config, and the new _load_policy_json each independently read the same row, creating 3 sessions. Pre-existing pattern worsened by this PR.
Recommendation: Read the blob once and derive all three results from it.

m3. Silent exception swallow in context_show for execution env display

File: src/cleveragents/cli/commands/project_context.py, lines 793–796
Problem: except Exception: raw_blob = None silently hides DB errors when loading the execution environment section. Inconsistent with earlier calls in the same function (lines 739–740) that don't use try/except.
Recommendation: Narrow exception type or log at DEBUG level.

m4. Cross-module import of private function _load_policy_json

File: src/cleveragents/cli/commands/plan.py, line 1710
Problem: plan.py imports _load_policy_json (underscore-prefixed private function) from project_context.py, creating tight coupling.
Recommendation: Either rename to load_policy_json (public API) or extract to a shared service layer.

m5. Lazy imports of domain enums inside function bodies

File: src/cleveragents/cli/commands/project_context.py, lines 648, 663
Problem: ExecutionEnvironment and ExecutionEnvPriority are imported inside the context_set function body. Per CONTRIBUTING.md: "Ensure all imports are at the top of the Python file." These enums have no circular dependency risk.
Recommendation: Move to top-level imports.

m6. Propagation only uses first project link

File: src/cleveragents/cli/commands/plan.py, line 1716
Problem: proj_name = project_links[0].project_name — multi-project plans only inherit from the first project. If the first project has no settings but the second does, propagation silently fails.
Recommendation: Document as known limitation or implement first-with-settings-wins resolution.

Nits

n1. Misleading BDD scenario title: Feature file line 60 — "Project-level priority propagates to plan when no plan-level override" only tests blob persistence, not propagation. Rename to reflect actual behavior.

n2. Missing case-sensitivity test: No scenario tests mixed-case input (e.g., "OVERRIDE") which the CLI normalizes via .lower().

n3. Missing error path test: No scenario tests setting priority on a nonexistent project.

n4. Temp DB files not cleaned up in steps: features/steps/project_exec_env_priority_steps.py line 59 — tempfile.mkstemp files are never explicitly deleted. Use context.add_cleanup.

Summary

The PR achieves the core functional intent — adding --execution-env-priority to project context set — but has significant correctness and quality issues:

Two novel critical bugs (C1: data-loss on _write_policy clobber; C2: propagated priority overwritten by fallback default) directly undermine the PR's stated purpose. These bugs are undetected because the tests bypass the actual CLI code.
Test quality is the root cause — all BDD and E2E tests simulate logic inline rather than exercising real code paths, hiding the correctness bugs. This was flagged across 3 prior reviews and remains unaddressed.
4 issues from previous reviews remain unaddressed: missing milestone, merge conflicts, domain type safety, and BDD test fidelity.
The commit structure, format, and branch naming are correct. The CHANGELOG entry is appropriate.

Recommended action: Fix the two critical correctness bugs, rewrite tests to use CliRunner, rebase onto master, set milestone, and address the type safety gap.

## PR Review: !1135 (Ticket #1079) ### Verdict: Request Changes The PR adds the `--execution-env-priority` flag to `project context set` with validation, persistence, display in `context_show`, and propagation to `plan use`. The commit structure (single atomic commit, Conventional Changelog format) is clean and the core idea is correct. However, the review uncovered **2 novel critical correctness bugs**, **several unaddressed issues from previous reviews**, and **significant test quality gaps** that prevent merging. --- ### Critical Issues **C1. Data loss in `context_set`: `_write_policy()` clobbers previously stored fields before merge read** - **File:** `src/cleveragents/cli/commands/project_context.py`, lines 643 → 676–688 - **Problem:** Line 643 calls `_write_policy(session_factory, project, policy, acms)`, which completely replaces the JSON blob with `policy.model_dump() + acms_config`, **stripping** any previously stored top-level keys like `execution_environment` and `execution_env_priority`. Then on line 678, `existing = _load_policy_json(...)` reads the already-clobbered blob, so the "merge" on line 679 has nothing to merge with. - **Concrete scenario:** User runs `project context set local/proj --execution-environment container`, then later `project context set local/proj --execution-env-priority override`. The second call clobbers `execution_environment` at line 643 before the merge logic on line 678 can preserve it. Any `context set` call without `--execution-environment` silently loses a previously-set environment. - **Note:** This bug is masked by the BDD test "Setting priority does not clear execution_environment" (scenario on line 56) because that test bypasses the actual CLI code path and directly manipulates the DB. - **Recommendation:** Read `existing = _load_policy_json(...)` **before** line 643's `_write_policy()` call, or consolidate into a single write that builds the full blob (policy + acms + extra_fields + preserved existing keys) before writing once. **C2. Propagated project-level priority is overwritten by fallback default in `plan.py`** - **File:** `src/cleveragents/cli/commands/plan.py`, lines 1704–1734 (propagation) vs. lines 1791–1794 (fallback) - **Problem:** The propagation block (lines 1724–1732) correctly sets `plan.execution_env_priority` to the project-level value (e.g., `OVERRIDE`). However, the existing fallback logic on lines 1791–1794 then unconditionally overwrites it: ```python elif execution_environment: plan.execution_env_priority = ExecutionEnvPriority.FALLBACK ``` The condition checks the CLI parameter `execution_environment`, not the plan's current value. Since the user provided `--execution-environment` but not `--execution-env-priority`, the `elif` fires and replaces the propagated `OVERRIDE` with `FALLBACK`. - **Recommendation:** Guard the fallback default: `elif execution_environment and plan.execution_env_priority is None:`. Or move the propagation block to after the existing CLI-override handling so it only fills gaps. **C3. BDD and Robot E2E tests do not exercise actual CLI code paths** - **File:** `features/steps/project_exec_env_priority_steps.py`, lines 137–147; `robot/helper_wf17_project_exec_env_priority.py`, lines 16–60 - **Problem:** The BDD step `step_call_set_with_priority` re-implements the enum validation and direct DB persistence inline rather than invoking the actual `context_set` Typer command. The Robot E2E helpers only validate `ExecutionEnvPriority` enum construction and JSON round-trips in-memory. Neither test suite exercises the actual CLI wiring, argument parsing, persistence path, or display logic. This means **bugs C1 and C2 above are not caught by any test**. - **Note:** This was raised by @freemo in reviews #2709, #2788, and #2892, and remains unaddressed. The project has 884+ existing uses of `CliRunner` in BDD steps — this PR deviates from the established testing convention. - **Recommendation:** Refactor CLI-facing BDD scenarios to use `CliRunner` to invoke the actual `context_set` command (with a mocked DI container), consistent with established patterns. Rewrite Robot helpers to invoke the actual CLI binary via subprocess. --- ### Major Issues **M1. Plan propagation logic (AC3) is completely untested** - **File:** `features/project_exec_env_priority.feature`, lines 60–63 - **Problem:** The scenario "Project-level priority propagates to plan when no plan-level override" only checks blob persistence — identical to what other persistence scenarios already test. The actual propagation code in `plan.py` (lines 1704–1734) that reads project-level settings and applies them to plan objects is never exercised. AC3 of ticket #1079 has zero test coverage. - **Recommendation:** Add a scenario that sets up a project with `execution_env_priority = "override"`, then invokes `plan use` (or its internals) without `--execution-env-priority`, and asserts the plan object has the propagated value. **M2. `context_show` omits execution environment fields in non-rich output formats** - **File:** `src/cleveragents/cli/commands/project_context.py`, lines 749–756 vs. 792–805 - **Problem:** The `data` dict used for JSON/YAML/plain output does not include `execution_environment` or `execution_env_priority`. Only the rich output path reads these from the raw blob. `project context show --format json` will not display the new fields, partially violating AC2 ("retrievable via project context show"). - **Recommendation:** Add these fields to the `data` dict by reading the raw blob before the format branching. **M3. Domain type safety gap: `ContextConfig.execution_env_priority` is `str | None`** - **File:** `src/cleveragents/domain/models/core/project.py`, line 272 - **Problem:** The Plan model correctly uses `ExecutionEnvPriority | None` (plan.py:699), but `ContextConfig` uses `str | None`. Any code constructing `ContextConfig` directly can set arbitrary strings. Per CONTRIBUTING.md's type safety requirements and the project's Pyright strict mode, the domain model should enforce the enum constraint. - **Note:** Raised in previous reviews, unaddressed. - **Recommendation:** Change to `execution_env_priority: ExecutionEnvPriority | None = Field(default=None, ...)`. **M4. Bare `except Exception: pass` violates error handling guidelines** - **File:** `src/cleveragents/cli/commands/plan.py`, lines 1733–1734 - **Problem:** Silently swallows all exceptions including `AttributeError`, `TypeError`, database corruption, and import failures. Creates a partial-state risk: if `plan.execution_environment = proj_env` succeeds but `ExecutionEnvPriority(proj_priority)` fails, the plan has an environment set but no priority. Per CONTRIBUTING.md: "Do not suppress errors" and "No Silent Failures." - **Recommendation:** Narrow to expected exceptions and log: `except (ImportError, KeyError, ValueError) as exc: logger.debug("Project context propagation skipped: %s", exc)`. **M5. `execution_environment` not validated during propagation** - **File:** `src/cleveragents/cli/commands/plan.py`, lines 1721–1723 - **Problem:** `proj_env` is read from the JSON blob and assigned directly to `plan.execution_environment` without validating against the `ExecutionEnvironment` enum. In contrast, `proj_priority` IS validated on line 1729. If the blob contains a corrupted or invalid environment string, it propagates an invalid value to the plan. - **Recommendation:** Validate `proj_env` through `ExecutionEnvironment(proj_env)` before assignment. **M6. No milestone assigned on PR** - **File:** PR metadata - **Problem:** PR `milestone` is `null` but ticket #1079 is in milestone **v3.5.0**. Per CONTRIBUTING.md: "Every PR must be assigned to the same milestone as its linked issue(s)." - **Note:** Raised in previous reviews, unaddressed. - **Recommendation:** Assign milestone v3.5.0. **M7. Merge conflicts — branch needs rebase** - **File:** PR metadata (`mergeable: false`) - **Problem:** Branch has diverged from master and cannot be merged cleanly. Per CONTRIBUTING.md: branches must be aligned via rebase, never merge. - **Note:** Raised in previous reviews, unaddressed. - **Recommendation:** Rebase onto latest `master`. **M8. `context_show` execution environment display is untested** - **File:** `src/cleveragents/cli/commands/project_context.py`, lines 792–805 - **Problem:** The new code displaying the "Execution Environment" section in rich output has no BDD or Robot test coverage. - **Recommendation:** Add a scenario that invokes `context_show` via CliRunner and asserts the output contains the priority value. **M9. No `@tdd_issue_1079` test exists in codebase** - **File:** N/A (searched entire codebase — zero matches) - **Problem:** Ticket subtask says "Remove `@tdd_expected_fail` from WF17 project-level override test case" (checked off), but no `tdd_issue_1079` tag exists anywhere. Per CONTRIBUTING.md, a bug fix PR without a corresponding TDD test may be blocked by CI. - **Recommendation:** Verify whether a TDD test was expected. If it never existed, add a note in the PR explaining why. --- ### Minor Issues **m1. Double-write + redundant DB read in `context_set` when extra_fields present** - **File:** `src/cleveragents/cli/commands/project_context.py`, lines 643, 676–688 - **Problem:** When `--execution-environment` or `--execution-env-priority` is provided, the function writes the blob twice (line 643 and line 679) with 3 reads + 2 writes and 5 separate sessions. The first write is immediately overwritten. - **Recommendation:** Consolidate into a single write. (Resolving C1 naturally fixes this.) **m2. Triple DB read in `context_show`** - **File:** `src/cleveragents/cli/commands/project_context.py`, lines 739, 740, 794 - **Problem:** `_read_policy`, `_read_acms_config`, and the new `_load_policy_json` each independently read the same row, creating 3 sessions. Pre-existing pattern worsened by this PR. - **Recommendation:** Read the blob once and derive all three results from it. **m3. Silent exception swallow in `context_show` for execution env display** - **File:** `src/cleveragents/cli/commands/project_context.py`, lines 793–796 - **Problem:** `except Exception: raw_blob = None` silently hides DB errors when loading the execution environment section. Inconsistent with earlier calls in the same function (lines 739–740) that don't use try/except. - **Recommendation:** Narrow exception type or log at DEBUG level. **m4. Cross-module import of private function `_load_policy_json`** - **File:** `src/cleveragents/cli/commands/plan.py`, line 1710 - **Problem:** `plan.py` imports `_load_policy_json` (underscore-prefixed private function) from `project_context.py`, creating tight coupling. - **Recommendation:** Either rename to `load_policy_json` (public API) or extract to a shared service layer. **m5. Lazy imports of domain enums inside function bodies** - **File:** `src/cleveragents/cli/commands/project_context.py`, lines 648, 663 - **Problem:** `ExecutionEnvironment` and `ExecutionEnvPriority` are imported inside the `context_set` function body. Per CONTRIBUTING.md: "Ensure all imports are at the top of the Python file." These enums have no circular dependency risk. - **Recommendation:** Move to top-level imports. **m6. Propagation only uses first project link** - **File:** `src/cleveragents/cli/commands/plan.py`, line 1716 - **Problem:** `proj_name = project_links[0].project_name` — multi-project plans only inherit from the first project. If the first project has no settings but the second does, propagation silently fails. - **Recommendation:** Document as known limitation or implement first-with-settings-wins resolution. --- ### Nits **n1. Misleading BDD scenario title**: Feature file line 60 — "Project-level priority propagates to plan when no plan-level override" only tests blob persistence, not propagation. Rename to reflect actual behavior. **n2. Missing case-sensitivity test**: No scenario tests mixed-case input (e.g., "OVERRIDE") which the CLI normalizes via `.lower()`. **n3. Missing error path test**: No scenario tests setting priority on a nonexistent project. **n4. Temp DB files not cleaned up in steps**: `features/steps/project_exec_env_priority_steps.py` line 59 — `tempfile.mkstemp` files are never explicitly deleted. Use `context.add_cleanup`. --- ### Summary The PR achieves the **core functional intent** — adding `--execution-env-priority` to `project context set` — but has significant correctness and quality issues: 1. **Two novel critical bugs** (C1: data-loss on `_write_policy` clobber; C2: propagated priority overwritten by fallback default) directly undermine the PR's stated purpose. These bugs are undetected because the tests bypass the actual CLI code. 2. **Test quality is the root cause** — all BDD and E2E tests simulate logic inline rather than exercising real code paths, hiding the correctness bugs. This was flagged across 3 prior reviews and remains unaddressed. 3. **4 issues from previous reviews remain unaddressed**: missing milestone, merge conflicts, domain type safety, and BDD test fidelity. 4. The commit structure, format, and branch naming are correct. The CHANGELOG entry is appropriate. **Recommended action:** Fix the two critical correctness bugs, rewrite tests to use CliRunner, rebase onto master, set milestone, and address the type safety gap.

freemo requested changes 2026-03-30 04:17:09 +00:00

freemo left a comment

Review: REQUEST CHANGES

Critical: Merge Conflict Markers in CHANGELOG.md

The diff shows unresolved merge conflict markers in CHANGELOG.md:

<<<<<<< HEAD

This will break the build. The conflict must be resolved before this PR can be merged.

Missing Milestone

Per CONTRIBUTING.md §Pull Request Process item 11: "Every PR must be assigned to the correct milestone." This PR has no milestone assigned. Since it closes #1079 and the companion PR #1136 is assigned to v3.5.0, this should likely be assigned to the same milestone.

Code Review Notes

The implementation is otherwise well-structured:

ContextConfig.execution_env_priority field added correctly with proper description.
ExecutionEnvPriority enum validation in context_set is clean — validates and rejects invalid values.
Merged blob persistence — extra_fields pattern correctly preserves existing fields when setting priority alone.
context show display — Properly renders the execution environment section.
BDD tests — 9 scenarios with 26 steps. Good coverage of domain model, enum, CLI validation, persistence round-trip, and propagation.

Minor Issues

Silent exception suppression in plan.py (use_action): The except Exception: pass block at the project context propagation silently swallows all errors. Per CONTRIBUTING.md §Exception Propagation: "Do not suppress errors." At minimum, log at debug level.
Cross-module import in CLI layer: from cleveragents.cli.commands.project_context import _load_policy_json creates a dependency between CLI command modules. Consider moving _load_policy_json to a shared service or utility module.
E2E Robot test is not a true E2E test: helper_wf17_project_exec_env_priority.py only tests enum validation and JSON round-trip — it doesn't actually invoke the CLI. Consider adding a true CLI invocation test.

## Review: REQUEST CHANGES ### Critical: Merge Conflict Markers in CHANGELOG.md The diff shows **unresolved merge conflict markers** in `CHANGELOG.md`: ``` <<<<<<< HEAD ``` This will break the build. The conflict must be resolved before this PR can be merged. ### Missing Milestone Per CONTRIBUTING.md §Pull Request Process item 11: "Every PR must be assigned to the correct milestone." This PR has no milestone assigned. Since it closes #1079 and the companion PR #1136 is assigned to v3.5.0, this should likely be assigned to the same milestone. ### Code Review Notes The implementation is otherwise well-structured: 1. **`ContextConfig.execution_env_priority`** field added correctly with proper description. 2. **`ExecutionEnvPriority` enum validation** in `context_set` is clean — validates and rejects invalid values. 3. **Merged blob persistence** — `extra_fields` pattern correctly preserves existing fields when setting priority alone. 4. **`context show` display** — Properly renders the execution environment section. 5. **BDD tests** — 9 scenarios with 26 steps. Good coverage of domain model, enum, CLI validation, persistence round-trip, and propagation. ### Minor Issues 1. **Silent exception suppression in `plan.py` (use_action)**: The `except Exception: pass` block at the project context propagation silently swallows all errors. Per CONTRIBUTING.md §Exception Propagation: "Do not suppress errors." At minimum, log at `debug` level. 2. **Cross-module import in CLI layer**: `from cleveragents.cli.commands.project_context import _load_policy_json` creates a dependency between CLI command modules. Consider moving `_load_policy_json` to a shared service or utility module. 3. **E2E Robot test is not a true E2E test**: `helper_wf17_project_exec_env_priority.py` only tests enum validation and JSON round-trip — it doesn't actually invoke the CLI. Consider adding a true CLI invocation test.

hamza.khyari referenced this issue from a commit

2026-03-30 11:04:03 +00:00

fix(cli): honour project-level execution-env-priority in resolution

hamza.khyari referenced this pull request

2026-03-30 11:04:21 +00:00

fix(cli): honour project-level execution-env-priority in resolution #1136

hamza.khyari referenced this issue from a commit

2026-03-30 11:38:15 +00:00

fix(cli): honour project-level execution-env-priority in resolution

hamza.khyari referenced this issue from a commit

2026-03-30 12:11:33 +00:00

fix(cli): honour project-level execution-env-priority in resolution