Repository navigation
[limen RESOLVE-organvm-i-theoria-rules-system-bound-8] resolve rules-system-bound#8 (UNSTABLE) - #9
Conversation
Reviewer's GuideAdds a typed environment configuration coercion helper and corresponding tests, introduces a fully featured CLI (with activation and demo commands) and main entry point, and wires the package up as a console script with pytest configuration and project metadata updates. Sequence diagram for the new CLI command handling in mainsequenceDiagram
actor User
participant Console as rules-system-bound
participant Module as __main__
participant CLI as main
participant Parser as build_parser
participant Activation as build_activation_report
participant Demo as build_demo_report
User->>Console: invoke (demo|audit|version|no command)
Console->>Module: python -m rules_system_bound
Module->>CLI: main(argv)
CLI->>Parser: build_parser()
Parser-->>CLI: argparse.ArgumentParser
CLI->>CLI: parse_args(argv)
alt command == version
CLI->>CLI: print(__version__)
CLI-->>Console: exit 0
else command == audit
CLI->>Activation: build_activation_report()
Activation-->>CLI: activation_report
CLI->>CLI: _print_activation_report(activation_report, args.json)
CLI-->>Console: exit 0
else command in {None, demo}
CLI->>Demo: build_demo_report()
Demo-->>CLI: demo_report
CLI->>CLI: _print_report(demo_report, args.json)
CLI-->>Console: exit 0
else unknown command
CLI->>CLI: parser.error(...)
CLI-->>Console: exit 2
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
More reviews will be available in 57 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
Config.from_env,_coerce_env_valuecan raiseValueErrorwithout indicating which environment variable was problematic; consider catching and re-raising with the env var name included to make misconfiguration easier to diagnose in deployment. - In
cli.main, thereturn 2afterparser.error(...)is unreachable becauseparser.errorexits viaSystemExit; you can drop the explicit return (or raise directly) to make the control flow clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Config.from_env`, `_coerce_env_value` can raise `ValueError` without indicating which environment variable was problematic; consider catching and re-raising with the env var name included to make misconfiguration easier to diagnose in deployment.
- In `cli.main`, the `return 2` after `parser.error(...)` is unreachable because `parser.error` exits via `SystemExit`; you can drop the explicit return (or raise directly) to make the control flow clearer.
## Individual Comments
### Comment 1
<location path="src/rules_system_bound/cli.py" line_range="188-192" />
<code_context>
+ _print_activation_report(build_activation_report(), args.json)
+ return 0
+
+ if args.command in {None, "demo"}:
+ _print_report(build_demo_report(), getattr(args, "json", False))
+ return 0
+
+ parser.error(f"unknown command: {args.command}")
+ return 2
</code_context>
<issue_to_address>
**suggestion:** The `parser.error` call already exits, so the subsequent `return 2` is dead code and the explicit unknown-command branch may never be hit in normal CLI use.
Given that `argparse` already treats unknown subcommands as `invalid choice` before reaching this point, this branch only matters if `main()` is called programmatically with a custom `Namespace`. If that’s not a supported scenario, you can remove the branch entirely. If it is, consider replacing `parser.error(...)` with a direct `print(..., file=sys.stderr)` plus `return 2` to make the behavior explicit and avoid additional `SystemExit` handling.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if args.command in {None, "demo"}: | ||
| _print_report(build_demo_report(), getattr(args, "json", False)) | ||
| return 0 | ||
|
|
||
| parser.error(f"unknown command: {args.command}") |
There was a problem hiding this comment.
suggestion: The parser.error call already exits, so the subsequent return 2 is dead code and the explicit unknown-command branch may never be hit in normal CLI use.
Given that argparse already treats unknown subcommands as invalid choice before reaching this point, this branch only matters if main() is called programmatically with a custom Namespace. If that’s not a supported scenario, you can remove the branch entirely. If it is, consider replacing parser.error(...) with a direct print(..., file=sys.stderr) plus return 2 to make the behavior explicit and avoid additional SystemExit handling.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a command-line interface for the package, adds module execution entry points, and implements environment variable type coercion for configuration loading. The review feedback suggests improving the robustness of environment variable parsing by gracefully handling empty or whitespace-only values, expanding test coverage to verify this fallback behavior, and simplifying the CLI's command dispatch logic by removing redundant checks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| value = os.getenv(env_key) | ||
| config[key] = default if value is None else _coerce_env_value(value, default) |
There was a problem hiding this comment.
When loading configuration from environment variables, empty or whitespace-only values (e.g., KEY=) can cause _coerce_env_value to raise a ValueError (for example, when trying to parse an empty string as a float or boolean). Gracefully falling back to the default value when the environment variable is empty or whitespace-only improves robustness, especially in containerized or CI/CD environments where variables are often defined but left empty.
| value = os.getenv(env_key) | |
| config[key] = default if value is None else _coerce_env_value(value, default) | |
| value = os.getenv(env_key) | |
| if value is None or value.strip() == "": | |
| config[key] = default | |
| else: | |
| config[key] = _coerce_env_value(value, default) |
| def test_config_from_env_preserves_types(self, monkeypatch): | ||
| """Environment values are coerced into config field types.""" | ||
| monkeypatch.setenv("CONTAINMENT_FUNCTION_WEIGHT", "0.7") | ||
| monkeypatch.setenv("LEVEL_NAMES_EMERGE", "false") | ||
|
|
||
| config = Config.from_env() | ||
|
|
||
| assert config.containment_function_weight == 0.7 | ||
| assert config.level_names_emerge is False |
There was a problem hiding this comment.
Expand the environment configuration test to verify that empty or whitespace-only environment variables are ignored and correctly fall back to their default values.
| def test_config_from_env_preserves_types(self, monkeypatch): | |
| """Environment values are coerced into config field types.""" | |
| monkeypatch.setenv("CONTAINMENT_FUNCTION_WEIGHT", "0.7") | |
| monkeypatch.setenv("LEVEL_NAMES_EMERGE", "false") | |
| config = Config.from_env() | |
| assert config.containment_function_weight == 0.7 | |
| assert config.level_names_emerge is False | |
| def test_config_from_env_preserves_types(self, monkeypatch): | |
| """Environment values are coerced into config field types.""" | |
| monkeypatch.setenv("CONTAINMENT_FUNCTION_WEIGHT", "0.7") | |
| monkeypatch.setenv("LEVEL_NAMES_EMERGE", "false") | |
| monkeypatch.setenv("CONTAINER_FUNCTION", " ") | |
| config = Config.from_env() | |
| assert config.containment_function_weight == 0.7 | |
| assert config.level_names_emerge is False | |
| assert config.container_function == "function" |
| if args.command in {None, "demo"}: | ||
| _print_report(build_demo_report(), getattr(args, "json", False)) | ||
| return 0 | ||
|
|
||
| parser.error(f"unknown command: {args.command}") | ||
| return 2 |
There was a problem hiding this comment.
Since argparse subparsers restrict the command choices to only the registered subparsers, args.command is guaranteed to be either "version", "audit", "demo", or None. Since "version" and "audit" are already handled and return early, any execution reaching this point is guaranteed to be "demo" or None. We can simplify this block and remove the redundant check and unreachable code.
_print_report(build_demo_report(), getattr(args, "json", False))
return 0![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes.
This PR introduces a critical launch blocker: the CLI imports __version__ from the package's __init__.py, but that module does not define it, causing an ImportError at runtime for source‑tree execution. Additionally, there are minor maintainability issues in environment variable coercion.
📄 Documentation Diagram
This diagram documents the new CLI activation smoke path, exercising containment, interaction, and idealization flows.
sequenceDiagram
actor Developer
participant CLI as rules_system_bound.cli
participant Containers as Container/ContainmentRules
participant Interactions as InteractionEmergence
participant Idealization as IdealizationManager
participant Config as Config
Developer->>CLI: main(["--json"])
CLI->>CLI: build_parser()
CLI->>CLI: parser.parse_args(argv)
CLI->>CLI: activation_report()
activate CLI
CLI->>Containers: ContainmentRules.compute_containment_score(root, bounded)
CLI->>Containers: root.can_contain(bounded, rules)
CLI->>Containers: root.add_child(bounded)
CLI->>Interactions: InteractionEmergence.register_system(processor)
CLI->>Interactions: InteractionEmergence.register_system(store)
CLI->>Interactions: InteractionEmergence.discover_interactions(processor, store)
CLI->>Idealization: IdealizationManager.create_form(form_id, form_type, name, real_system)
CLI->>Idealization: IdealizationManager.relate(ideal, root)
CLI->>Config: Config()
deactivate CLI
CLI-->>CLI: report
CLI->>CLI: json.dumps(report, indent=2, sort_keys=True)
CLI-->>Developer: JSON activation evidence
note over CLI: PR introduces deterministic CLI smoke path
| Priority | File | Category | Impact Summary (≤12 words) | Anchors |
|---|---|---|---|---|
| P0 | src/rules_system_bound/cli.py | Bug | CLI crashes at import due to missing __version__. |
path:src/rules_system_bound/init.py |
| P2 | src/rules_system_bound/config.py | Maintainability | Bare ValueError on env coercion lacks context. |
method:Config.from_env |
| P2 | src/rules_system_bound/config.py | Maintainability | Env variable name not passed to coerce function. | - |
🔍 Notable Themes
- Config error handling: Both P2 findings relate to environment variable coercion—errors are unhelpful and hard to debug. A unified fix (passing the key and wrapping exceptions) would strengthen the configuration module.
📈 Risk Diagram
This diagram illustrates the risk that the CLI crashes at import due to missing version in init.py.
sequenceDiagram
actor Developer
participant CLI as rules_system_bound.cli
participant Init as rules_system_bound.__init__
Developer->>CLI: python -m rules_system_bound demo
activate CLI
CLI->>Init: from . import __version__
Init-->>CLI: ImportError: name '__version__' not defined
deactivate CLI
note over CLI,Init: R1(P0): Missing __version__ definition causes ImportError at import time
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| from . import __version__ | ||
| from .container import Container, ContainmentRules |
There was a problem hiding this comment.
P0 | Confidence: High
The new cli.py imports __version__ from the parent package (from . import __version__). However, __init__.py does not define __version__. Hatchling does not automatically inject it for source imports, so ImportError is raised at import time. This blocks the entire CLI and any imports of cli.py. Fix by defining __version__ in __init__.py or using importlib.metadata.
Code Suggestion:
__version__ = "0.1.0"Evidence: path:src/rules_system_bound/init.py
| } | ||
|
|
||
|
|
||
| def _coerce_env_value(value: str, default: Any) -> Any: |
There was a problem hiding this comment.
P2 | Confidence: High
When an environment variable is present but cannot be coerced to the expected type, _coerce_env_value raises a bare ValueError. This propagates uncaught out of Config.from_env(), causing a non‑descriptive stack trace. Consider catching the exception and re‑raising with a more informative message, or falling back to the default with a warning.
| def _coerce_env_value(value: str, default: Any) -> Any: | |
| def _coerce_env_value(key: str, value: str, default: Any) -> Any: | |
| try: | |
| if isinstance(default, bool): | |
| normalized = value.strip().lower() | |
| if normalized in {"1", "true", "yes", "on"}: | |
| return True | |
| if normalized in {"0", "false", "no", "off"}: | |
| return False | |
| raise ValueError(f"Invalid boolean value: {value!r}") | |
| if isinstance(default, float): | |
| return float(value) | |
| if isinstance(default, int): | |
| return int(value) | |
| return value | |
| except ValueError as exc: | |
| raise ConfigError( | |
| f"Environment variable '{key.upper()}' = {value!r} " | |
| f"cannot be coerced to type {type(default).__name__}. " | |
| f"Expected a {type(default).__name__} value." | |
| ) from exc |
Evidence: method:Config.from_env
| value = os.getenv(env_key) | ||
| config[key] = default if value is None else _coerce_env_value(value, default) |
There was a problem hiding this comment.
P2 | Confidence: Medium
The _coerce_env_value function currently receives only the string value and the default; it does not receive the environment variable name. When coercion fails, the error message cannot mention which variable caused the problem. Pass the key as well (as suggested in the previous finding) and add a test for the error path.
Code Suggestion:
def test_config_env_coercion_failure_raises_meaningful_error(monkeypatch):
monkeypatch.setenv("LEVEL_NAMES_EMERGE", "not_a_bool")
with pytest.raises(ValueError, match="LEVEL_NAMES_EMERGE"):
Config.from_env()
limen task RESOLVE-organvm-i-theoria-rules-system-bound-8
Adds .github/workflows/ci-python.yml with test job matrix (3.10/3.11/3.12). Required branch protection checks are test(3.10), test(3.11), test(3.12). Actions pinned to commit SHAs per org policy: actions/checkout@v4 -> 11bd71901bbe5b1630ceea73d27597364c9af683 (v4.2.2) actions/setup-python@v5 -> a26af69be951a213d495a4c3e4e4022e16d87065 (v5.6.0)
14039af to
6ceb92d
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Autonomous limen dispatch of task
RESOLVE-organvm-i-theoria-rules-system-bound-8.Resolve blocked PR #8 ('[limen GH-organvm-i-theoria-rules-system-bound-2] ACTIVATION'), state=UNSTABLE. Branch=limen/gh-organvm-i-theoria-rules-system-bound-2-26b9, base=main. In the worktree:
git fetch origin limen/gh-organvm-i-theoria-rules-system-bound-2-26b9 main; git checkout -B limen/gh-organvm-i-theoria-rules-system-bound-2-26b9 origin/limen/gh-organvm-i-theoria-rules-system-bound-2-26b9; git rebase origin/mainthen address the failing checks / unresolved review threads; run the build/tests;git push --force-with-lease origin limen/gh-organvm-i-theoria-rules-system-bound-2-26b9. If the branch is unrecoverable, instead REBUILD the same feature cleanly off origin/main as a fresh PR. Goal: make it mergeable.Produced in an isolated worktree off origin — review before merge.
Summary by Sourcery
Introduce a runnable command-line interface and improve configuration handling for the rules-system-bound package.
New Features:
Bug Fixes:
Enhancements:
Tests: