Skip to content

fix(tools): use consistent configurable file encodings 🤖🤖🤖 - #298

Draft
Sampoornnagpal wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
Sampoornnagpal:fix/shell-file-encoding
Draft

fix(tools): use consistent configurable file encodings 🤖🤖🤖#298
Sampoornnagpal wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
Sampoornnagpal:fix/shell-file-encoding

Conversation

@Sampoornnagpal

Copy link
Copy Markdown
Contributor

What does this PR do?

ShellTools currently uses the platform default encoding for file reads, writes, replacements, and search anchors. Under a cp1252 default, writing emoji or Chinese text raises UnicodeEncodeError; reading an existing UTF-8 file produces mojibake, including the text attached to editable search matches.

Use a consistent, configurable file encoding across these operations. The proposed default is UTF-8. Callers handling legacy files can choose a codec such as encoding="cp1252", or encoding=None to retain the previous platform-default behavior. Shell command-output decoding is unchanged.

Draft / coordination: this changes the default file-encoding policy and overlaps the bounded file-I/O refactor in #94. That refactor explicitly preserves locale-default encoding. Please agree on the encoding policy and implementation ordering before marking this ready; I can adapt this change to the bounded helpers if #94 lands first. This is not a replacement for #94's bounded/atomic I/O work.

Validation

Three regressions fail against the original implementation with a simulated cp1252 default: Unicode writes, reading existing UTF-8 text, and harvesting Unicode search anchors. Two further cases verify explicit cp1252 and None compatibility. Coverage exercises both path-based and Match-based edits, including an edit from a harvested anchor. The search session is mocked; no Bash process is required.

uv run pytest -q tests/tools/test_shell_file_encoding.py tests/tools/test_shell_tools_modern_behavior.py -k 'not run_persists and not run_reports and not cwd_change and not close_terminates and not timeout_flag_preserves' — 13 passed, 5 deselected.

The deselections are four Bash-dependent cases and one existing POSIX-absolute-path fixture incompatible with Windows. Validation used Windows/Python 3.12 with external import-only helpers for #84/#85 (fcntl/SIGUSR2); those helpers are not included and do not validate POSIX locking/signals. Real grep/Bash integration and a full Linux suite have not been run here.

Ruff lint and formatting pass. Targeted Pyright reports one diagnostic in the unchanged shell-output callback (str versus Literal["stdout", "stderr"]); this PR does not modify that callback.

Related issues

Related overlapping implementation: #94. The skill-body UTF-8 fix in #281 is separate and does not affect ShellTools.

Checklist

Signed-off-by: Sampoorn Nagpal <nagpalsampoorn@gmail.com>
@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Comment @coderabbitai help to get the list of available commands.

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