Skip to content

fix(llm,storage): harden Windows extraction paths#287

Merged
yyiilluu merged 3 commits into
ReflexioAI:mainfrom
wenchanghan:codex/windows-extraction-fixes
Jul 4, 2026
Merged

fix(llm,storage): harden Windows extraction paths#287
yyiilluu merged 3 commits into
ReflexioAI:mainfrom
wenchanghan:codex/windows-extraction-fixes

Conversation

@wenchanghan

@wenchanghan wenchanghan commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Why

Windows local extraction can fail before Reflexio gets a usable model response:

  • Windows code pages can break non-ASCII subprocess input unless the local CLI bridge is pinned to UTF-8.
  • Large extraction schemas can exceed Windows command-line length when sent through --append-system-prompt.
  • Extensionless CLI override paths can miss executable shims commonly installed on Windows.
  • Some storage callers can hand timestamp values that need safe normalization without breaking existing open-bound sentinels.

What Changed

  • Local CLI provider
    • Pins Claude and Codex subprocess text encoding to UTF-8.
    • Uses replacement decoding only on Windows; non-Windows stays strict.
    • Keeps small Claude prompts on --append-system-prompt.
    • Moves only large Windows Claude system prompts into stdin, without Codex-specific ## Task framing.
    • Resolves extensionless CLAUDE_SMART_CLI_PATH overrides through PATHEXT plus .cmd/.exe/.bat Windows shims.
    • Skips .ps1 shim auto-resolution because PowerShell scripts require an explicit PowerShell host.
  • SQLite timestamp helper
    • Normalizes contemporary millisecond epochs to seconds before ISO conversion.
    • Preserves documented open-bound sentinel behavior such as to_ts=10**12.
  • Tests
    • Covers non-ASCII subprocess input, Windows large prompt fallback, large structured-output schemas, .cmd/.exe/.bat override shims, .ps1 exclusion, and seconds/milliseconds/sentinel timestamp cases.

Compatibility Notes

  • Public API timestamp contracts remain Unix seconds; no public docs update is needed for this PR.
  • The timestamp tolerance is intentionally bounded so historical open-bound sentinels remain in seconds-space.
  • The Windows prompt fallback is only used when the system prompt is large enough to risk argv limits.

Validation

  • uv run pytest --no-cov tests/server/llm/test_claude_code_provider.py tests/server/services/storage/test_sqlite_storage.py tests/server/api_endpoints/test_evaluation_overview_api.py -q - 112 passed.
  • uv run pytest --no-cov tests/lib/test_interactions_unit.py tests/e2e_tests/test_playbook_workflows.py -q - 39 passed, 16 skipped.
  • uv run ruff check reflexio/server/llm/providers/claude_code_provider.py reflexio/server/services/storage/sqlite_storage/_base.py tests/server/llm/test_claude_code_provider.py tests/server/services/storage/test_sqlite_storage.py tests/server/api_endpoints/test_evaluation_overview_api.py - passed.
  • uv run ruff format --check reflexio/server/llm/providers/claude_code_provider.py reflexio/server/services/storage/sqlite_storage/_base.py tests/server/llm/test_claude_code_provider.py tests/server/services/storage/test_sqlite_storage.py tests/server/api_endpoints/test_evaluation_overview_api.py - passed.
  • uv run pyright reflexio/server/llm/providers/claude_code_provider.py reflexio/server/services/storage/sqlite_storage/_base.py tests/server/llm/test_claude_code_provider.py tests/server/services/storage/test_sqlite_storage.py tests/server/api_endpoints/test_evaluation_overview_api.py - passed.
  • git diff --check - passed.
  • Earlier full uv run pytest -q was attempted on this branch: coverage passed at 81.78%, but unrelated benchmark/eval/optimizer tests failed locally because OPENAI_API_KEY is not set.

@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 81076737-6c70-4ae9-8941-46e5cae7e789

📥 Commits

Reviewing files that changed from the base of the PR and between d20964f and 4762d12.

📒 Files selected for processing (2)
  • reflexio/server/llm/providers/claude_code_provider.py
  • tests/server/llm/test_claude_code_provider.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/server/llm/test_claude_code_provider.py

📝 Walkthrough

Walkthrough

This PR improves Windows handling in the Claude Code provider by normalizing overridden CLI paths, inlining oversized system prompts into stdin, and setting explicit UTF-8 subprocess decoding. It also normalizes plausible millisecond epochs in _epoch_to_iso and adds tests for both behaviors.

Changes

Claude Code Windows compatibility

Layer / File(s) Summary
Windows CLI path resolution
reflexio/server/llm/providers/claude_code_provider.py, tests/server/llm/test_claude_code_provider.py
Adds Windows suffix normalization for CLI overrides and updates _resolve_cli_path to validate the normalized path before returning it; tests cover adjacent .cmd selection and executable shim probing.
System prompt inlining and subprocess decoding
reflexio/server/llm/providers/claude_code_provider.py, tests/server/llm/test_claude_code_provider.py
Adds the Windows prompt length threshold and stdin formatter, switches _run_claude_stream between --append-system-prompt and stdin inlining, and sets explicit UTF-8 subprocess decoding for claude calls; tests cover stdin payloads and Windows-specific invocation details.
Codex host subprocess behavior
reflexio/server/llm/providers/claude_code_provider.py, tests/server/llm/test_claude_code_provider.py
Updates the Codex-host subprocess invocation to use UTF-8 decoding, Windows error handling, new stdin payload text, and CLAUDE_SMART_HOST environment coverage in tests.

Estimated code review effort: 4 (Complex) | ~40 minutes

Epoch to ISO Millisecond Normalization

Layer / File(s) Summary
Millisecond epoch normalization
reflexio/server/services/storage/sqlite_storage/_base.py, tests/server/services/storage/test_sqlite_storage.py
Adds a lower-bound constant for plausible millisecond epochs and updates _epoch_to_iso to normalize those values before clamping and conversion; tests cover second-level, millisecond, and sentinel epoch handling.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately reflects the Windows hardening work across llm and storage.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

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 `@reflexio/server/llm/providers/claude_code_provider.py`:
- Around line 120-129: The Windows override lookup in
_windows_executable_cli_path() only tries a .cmd suffix, so extensionless
CLAUDE_SMART_CLI_PATH values can miss common shims like .exe and .bat and fall
back to PATH. Update the logic in _windows_executable_cli_path() to resolve
Windows executable extensions more broadly, either by honoring PATHEXT or by
probing the usual extensions in addition to .cmd, while keeping the non-Windows
and already-qualified-path behavior unchanged.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d3835acb-2552-4055-8816-f9c83555768a

📥 Commits

Reviewing files that changed from the base of the PR and between b6d4813 and 829b67b.

📒 Files selected for processing (4)
  • reflexio/server/llm/providers/claude_code_provider.py
  • reflexio/server/services/storage/sqlite_storage/_base.py
  • tests/server/llm/test_claude_code_provider.py
  • tests/server/services/storage/test_sqlite_storage.py

Comment thread reflexio/server/llm/providers/claude_code_provider.py Outdated
Comment thread reflexio/server/llm/providers/claude_code_provider.py Outdated
Comment thread reflexio/server/llm/providers/claude_code_provider.py Outdated
Comment thread reflexio/server/llm/providers/claude_code_provider.py Outdated
@wenchanghan

Copy link
Copy Markdown
Contributor Author

Code review

Found 1 issue:

  1. _epoch_to_iso's new millisecond-detection floor exactly equals the documented "no upper limit" sentinel, silently corrupting it (bug: _MIN_PLAUSIBLE_MILLISECOND_EPOCH_TS = 1_000_000_000_000 is exactly 10**12, the value the adjacent comment says callers pass as an open upper bound)

# Bounds that ``datetime.fromtimestamp(tz=UTC)`` can represent (year 1..9999).
# Callers pass sentinel "open" bounds — e.g. ``to_ts=10**12`` for "no upper
# limit" or ``0`` for "from the beginning" — which would otherwise overflow
# ``datetime.fromtimestamp`` with a ``ValueError``. Clamping to these bounds
# yields the same query semantics (the ISO string still sorts before/after every
# stored row) with a valid value.
_MAX_SAFE_EPOCH_TS = 253_402_300_799 # 9999-12-31T23:59:59Z
_MIN_SAFE_EPOCH_TS = 0 # 1970-01-01T00:00:00Z
_MIN_PLAUSIBLE_MILLISECOND_EPOCH_TS = 1_000_000_000_000
def _epoch_to_iso(ts: int) -> str:
"""Convert a Unix timestamp (seconds) to an ISO 8601 string.
Plausible millisecond timestamps are normalized to seconds. Out-of-range
sentinel bounds are clamped to the representable range so callers passing
"open" window bounds never trigger platform-specific datetime errors.
"""
if _MIN_PLAUSIBLE_MILLISECOND_EPOCH_TS <= ts <= _MAX_SAFE_EPOCH_TS * 1000:
ts = ts // 1000
clamped = max(_MIN_SAFE_EPOCH_TS, min(ts, _MAX_SAFE_EPOCH_TS))
return datetime.fromtimestamp(clamped, tz=UTC).isoformat()

_epoch_to_iso(10**12) now returns 2001-09-09T01:46:40+00:00 instead of the pre-PR 9999-12-31T23:59:59+00:00, because 10**12 falls inside the new inclusive range [_MIN_PLAUSIBLE_MILLISECOND_EPOCH_TS, _MAX_SAFE_EPOCH_TS * 1000] and gets divided by 1000. This sentinel is live — tests/server/api_endpoints/test_evaluation_overview_api.py#L15 passes to_ts=1_000_000_000_000 — and is the exact value #172 clamped after it 500'd the evaluation-overview endpoint. This PR reintroduces that failure mode, just silently instead of as a crash: an "open" upper-bound query now resolves to a 2001 cutoff and would exclude nearly all real rows. The PR's new tests cover 10**18 but not 10**12, so this isn't caught.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@wenchanghan wenchanghan changed the title fix(llm): harden Windows local CLI extraction fix(llm,storage): harden Windows extraction paths Jul 4, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

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 `@reflexio/server/llm/providers/claude_code_provider.py`:
- Around line 140-153: _skip .ps1 from Windows CLI override resolution or launch
it through PowerShell._ The _resolve_cli_override_path() helper can currently
resolve a PowerShell script path that _run_claude_stream() cannot execute
directly, which leads to an OSError instead of the intended fallback. Update
_resolve_cli_override_path() to avoid returning .ps1 candidates, or adjust
_run_claude_stream() to detect and invoke PowerShell scripts via PowerShell, so
the existing fallback behavior continues to work.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e13e2f52-ecaf-45b9-8dc6-360b12a3aa7f

📥 Commits

Reviewing files that changed from the base of the PR and between 829b67b and d20964f.

📒 Files selected for processing (4)
  • reflexio/server/llm/providers/claude_code_provider.py
  • reflexio/server/services/storage/sqlite_storage/_base.py
  • tests/server/llm/test_claude_code_provider.py
  • tests/server/services/storage/test_sqlite_storage.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/server/llm/test_claude_code_provider.py
  • reflexio/server/services/storage/sqlite_storage/_base.py

Comment thread reflexio/server/llm/providers/claude_code_provider.py
@yyiilluu yyiilluu merged commit 0256326 into ReflexioAI:main Jul 4, 2026
1 check 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.

2 participants