Skip to content

fix: honour PR_SPLIT_* env vars for strategy and priority options - #52

Merged
vitali87 merged 3 commits into
mainfrom
fix/strategy-envvars
Aug 30, 2026
Merged

fix: honour PR_SPLIT_* env vars for strategy and priority options#52
vitali87 merged 3 commits into
mainfrom
fix/strategy-envvars

Conversation

@vitali87

Copy link
Copy Markdown
Owner

Summary

The README's Configuration table documents PR_SPLIT_PRIORITY, PR_SPLIT_CHUNK_STRATEGY and PR_SPLIT_PARTITION_STRATEGY, but the split options had no envvar= binding. Typer passed the explicit default into Settings(...), which overrides pydantic-settings' env lookup, so the variables were silently ignored (verified: PR_SPLIT_PARTITION_STRATEGY=graph still produced llm).

  • Bind envvar on --priority, --chunk-strategy, --partition-strategy, and --cp-sat-timeout (PR_SPLIT_CP_SAT_TIMEOUT)
  • Document --cp-sat-timeout / PR_SPLIT_CP_SAT_TIMEOUT, which were missing from both README tables

Test plan

  • New test_split_uses_strategy_envvars covering all four variables
  • uv run ruff check / ruff format --check clean
  • uv run pytest -q — 445 passed

@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 19 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 30670c5d-45df-44a4-b123-0d10e8047aff

📥 Commits

Reviewing files that changed from the base of the PR and between db793ec and 1221e85.

📒 Files selected for processing (3)
  • README.md
  • pr_split/cli.py
  • tests/test_cli_new_features.py

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.

@greptile-apps

greptile-apps Bot commented Aug 30, 2026

Copy link
Copy Markdown

Greptile Summary

This change makes the documented PR_SPLIT_PRIORITY, PR_SPLIT_CHUNK_STRATEGY, PR_SPLIT_PARTITION_STRATEGY, and PR_SPLIT_CP_SAT_TIMEOUT settings effective for pr-split split, and documents the CP-SAT timeout setting. The environment-binding failure hypothesis was disproved by exercising split --dry-run with all four values and confirming that correctly parsed settings reach planning; explicit command-line options also override environment values.

Confidence Score: 5/5

Safe to merge: the changed CLI path correctly applies the documented environment settings while preserving explicit command-line overrides.

The command was exercised with all four environment values, enum and float conversion was confirmed at the planner boundary, explicit option precedence was checked, and focused CLI tests passed.

Files Needing Attention: No files need further attention.

T-Rex T-Rex Logs

What T-Rex did

  • Reviewed the general-contract-validation-proof to compare pre-change default environment bindings with post-change explicit options for the split command.
  • Validated the env-bindings behavior by running the env-bindings validation script, which exercises the Typer command and mocks external boundaries after parsing.
  • Inspected the before-and-after command output logs to confirm the exact values produced by the split command across the change boundary.
  • Checked the targeted tests log to verify test coverage for the new env-bindings validation behavior.

View all artifacts

T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "fix: honour PR_SPLIT_* env vars for stra..." | Re-trigger Greptile

README documents PR_SPLIT_PRIORITY, PR_SPLIT_CHUNK_STRATEGY and
PR_SPLIT_PARTITION_STRATEGY, but the split command's options had no
envvar binding, so their explicit defaults always overrode the
environment. Bind them (plus the undocumented --cp-sat-timeout) and
document the flag and its variable.
@vitali87
vitali87 force-pushed the fix/strategy-envvars branch from 6dc1b98 to ad8e59e Compare August 30, 2026 12:25
@vitali87
vitali87 merged commit 111463e into main Aug 30, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant