Skip to content

fix(batch): render skill names and console fields literally - #734

Merged
rng1995 merged 21 commits into
mainfrom
yashraj/fix-batch-terminal-output-20261005
Oct 6, 2026
Merged

rng1995 merged 21 commits into
mainfrom
yashraj/fix-batch-terminal-output-20261005

Conversation

@yashrajp22

@yashrajp22 yashrajp22 commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

Untrusted skill names and error text could inject Rich markup or terminal controls into batch output. Render dynamic fields literally, preserve Unicode spaces, emoji shortcodes and trailing backslashes, and avoid parsing an already-rendered report a second time.

Validation: 152 targeted batch reporting and CLI tests passed, including Rich and plain-console warnings. The new trailing-backslash check fails on the old code. Ruff checks passed.

Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

Hi @yashrajp22, thank you for hardening the batch scanner's console output against hostile skill names and error text!

Value and readiness: This change does what it says for the contrib batch scanner. Every untrusted value that batch_scan.py and the batch terminal report print now goes through _terminal_text (non-printables removed, folded to one line) and then escape() before Rich markup. That covers directory names, manifest names, error text, source groups, language, and ledger-exception reason, path and message. The already-rendered report is printed with markup=False, highlight=False, so it is no longer parsed twice. I traced the attack inputs. On main and on the #733 base, a skill directory named [/x] aborts the whole batch: the progress-line _print raises MarkupError outside the per-future try, so no report is produced for any skill. With this PR the name prints literally. The ESC that starts \x1b[2J or an OSC 8 hyperlink is removed, along with NUL, CR and U+202E, so what remains prints as inert text. The tests cover the attack cases. This is ready for maintainer review. Merge should wait for #733 and a passing CI run on this PR; none has run yet because the base branch is not main.

Material findings

  1. [Non-blocking] contrib/batch_scan/reports.py:296: _terminal_text deletes non-ASCII spaces instead of folding them. str.isprintable() is False for U+3000 (ideographic space) and U+00A0, and neither is in "\r\n\t", so 技能 名称 renders as 技能名称. This tool targets zh/ja/ko skills. Filtering with c.isprintable() or c.isspace() (as #733's _markdown_plain_text does) and then " ".join(text.split()) keeps one space. It still removes ESC, C1 and bidi controls, since none of them are whitespace.
  2. [Non-blocking] contrib/batch_scan/batch_scan.py:397: emoji shortcodes are still rewritten. escape() and markup=False do not turn off Rich's emoji replacement. A directory or manifest name such as helper:white_check_mark: prints with an emoji glyph in progress lines, the table and the final report. If fully literal output is the goal, pass emoji=False here and create both consoles (batch_scan.py:147, reports.py:99) with emoji=False.
  3. [Non-blocking] contrib/batch_scan/reports.py:188: escape() doubles a trailing backslash, and Rich only consumes the extra one when a tag follows. Whole-cell values (name, language, the source-breakdown group at :258) that end in \ display as \\. Progress lines and the ledger header are fine because a closing tag follows. Cosmetic.

PIC tradeoffs: This PR (like #733 and _sanitize_finding) deletes hidden characters. Core cli._visible_terminal_text renders them visibly as \xNN. Deletion means an operator cannot tell that a skill name contained control or bidi characters. That is acceptable for this contrib tool, but one project-wide choice would make batch, Markdown and terminal output consistent.

Verification and gaps:

  • Reviewed only this PR's own commit b92e02d6 (diff against bf97e339, the #733 commit it is stacked on). #733 is reviewed separately.
  • Statically checked every f-string interpolation in print and add_row calls in batch_scan.py and reports.py. The only unwrapped values left are internal counts, scores, colors, the version and the literal "en" branch.
  • Probed Rich 15.0.0 (library only) for escape, emoji and control-character handling, and checked Python's isprintable and isspace semantics for the edge characters above.
  • Read the tests statically. They cover both formatters with closing-tag, balanced-tag and control payloads, in all four exception fields; the source and language breakdowns; and CLI progress and error lines with and without Rich, including the second-parse case. They would fail on the base. I did not run them locally, per policy.
  • CI: no checks. The CI workflow runs only for PRs into main, and #733's own run is action_required. After #733 merges and this PR is retargeted to main, CI must pass before merge. If #733 is squash-merged, rebase this branch onto main so only b92e02d6 remains.
  • ruff check and ruff format --check pass on both test files. The contrib files are outside make lint and format-check, and were already unformatted on the base.
  • Merge: clean with #733, main, #725, #735, #723, and #743 (which is stacked on this head).
  • Out of scope: contrib and core logger.warning(..., batch.file_label) lines still write raw file paths to stderr through a plain logging handler.

Decision: Approved (reviewed head b92e02d6d014155f88f8f67bc18a9a5173dc180c)

Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>

# Conflicts:
#	tests/test_batch_scan_reports.py
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
@yashrajp22

Copy link
Copy Markdown
Collaborator Author

Thanks for checking these edge cases! I now preserve Unicode spaces, disable emoji shortcode conversion, and use literal Rich text for table cells and breakdown labels so trailing backslashes stay unchanged. Fixed in 2c3e9ab; all 149 targeted report and CLI tests pass. This still needs to land after #733. I also brought in the reviewed changes from #733.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

Hi @yashrajp22, thank you for following up on all three console edge cases and adding a test for each one!

Value and readiness: My earlier approval was for b92e02d6, so I re-reviewed the new commits on this head. The only change to this PR's own code is 2c3e9ab5. It closes the three non-blocking points from my last review without weakening the escaping. Hostile directory names, manifest names and error text still print literally, and that now includes emoji shortcodes, trailing backslashes and non-ASCII spaces. f13e98a8 only brings in #733's reviewed commits. This is ready for maintainer review. It still has to land after #733, and it needs a CI run once it targets main.

Previous findings:

  • Resolved (non-ASCII spaces were deleted). _terminal_text (contrib/batch_scan/reports.py:297) now keeps isspace() characters and folds each whitespace run to one space, so 技能\u3000名称\u00a0x prints as 技能 名称 x (test at tests/test_batch_scan_reports.py:135). ESC, U+009B and the bidi controls are still removed because they are not whitespace. Whitespace-like controls such as NEL and U+2028 become one space, which is also safe.
  • Resolved (emoji shortcodes were rewritten). Both consoles are now created with emoji=False (contrib/batch_scan/batch_scan.py:147, reports.py:101). I checked this with Rich 15.0 (library only). With the default setting, x:smile: became x😄 even with markup=False. With emoji=False it stays literal in markup strings, Text cells and markup=False prints. Tests: tests/test_batch_scan_reports.py:136 (both formatters) and tests/test_batch_scan_security.py:232-251 (CLI progress lines, with and without Rich).
  • Resolved (trailing backslash doubled in whole-cell values). The name and language cells and both breakdown labels are now Rich Text objects (reports.py:187-195, :260, :291), so they are never markup-escaped. Tests at tests/test_batch_scan_reports.py:137 and :165-174 assert that no doubled backslash appears.

Material findings

  1. [Non-blocking] contrib/batch_scan/batch_scan.py:332: this corrects my last review. I said every display() value is followed by a closing tag, but the --no-llm non-English warning quotes '{display(rel_name)}'. A directory named x\ still prints as 'x\\' in that one stderr line. It is cosmetic. Building that line from Text, as the table now does, would make it consistent.

PIC tradeoffs: Unchanged. This tool deletes hidden characters, while core cli._visible_terminal_text shows them as \xNN.

Verification and gaps:

  • This PR's own diff is 01516f68..f13e98a8, where 01516f68 is #733's branch as merged here. The f13e98a8 merge had one conflict, in tests/test_batch_scan_reports.py. git show --remerge-diff shows it was resolved by keeping both appended test blocks, with no other edits.
  • I checked every print and add_row in batch_scan.py and reports.py again. The remaining escape() uses (severity in the table, the ledger header, progress lines) are each followed by a closing tag.
  • The new test cases would fail on b92e02d6: the emoji, trailing-backslash and U+3000 cases all render differently there.
  • ruff check and ruff format --check on src/ and tests/ pass on the full tree. The contrib/ ruff findings all exist on main; this PR adds none.
  • No CI checks: CI only runs for PRs into main. After #733 merges and this PR is retargeted, CI must pass before merge. If #733 is squash-merged, rebase onto main so that only this PR's commits remain.
  • git merge-tree is clean against #733, main and #743, which is stacked on this head.
  • Landing order: #733, then this PR, then #743.
  • I did not run the tests locally, per policy.

Decision: Approved (reviewed head f13e98a89998d9564b473bdf7b65f1034b75e775)

Base automatically changed from yashraj/fix-batch-markdown-output-20261005 to main October 5, 2026 18:29
github-actions Bot and others added 5 commits October 5, 2026 18:29
@yashrajp22

Copy link
Copy Markdown
Collaborator Author

Thanks for spotting the remaining warning case! The warning now uses literal text, so a trailing backslash stays unchanged with or without Rich. Fixed in 2cefeac; all 152 targeted batch and report tests pass, and the new check fails on the old code.

@rng1995
rng1995 merged commit 95528a7 into main Oct 6, 2026
@rng1995
rng1995 deleted the yashraj/fix-batch-terminal-output-20261005 branch October 6, 2026 09:22
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