Repository navigation
fix(batch): render skill names and console fields literally - #734
Conversation
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
rng1995
left a comment
There was a problem hiding this comment.
[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
- [Non-blocking]
contrib/batch_scan/reports.py:296:_terminal_textdeletes 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 withc.isprintable() or c.isspace()(as #733's_markdown_plain_textdoes) and then" ".join(text.split())keeps one space. It still removes ESC, C1 and bidi controls, since none of them are whitespace. - [Non-blocking]
contrib/batch_scan/batch_scan.py:397: emoji shortcodes are still rewritten.escape()andmarkup=Falsedo not turn off Rich's emoji replacement. A directory or manifest name such ashelper:white_check_mark:prints with an emoji glyph in progress lines, the table and the final report. If fully literal output is the goal, passemoji=Falsehere and create both consoles (batch_scan.py:147,reports.py:99) withemoji=False. - [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 againstbf97e339, the #733 commit it is stacked on). #733 is reviewed separately. - Statically checked every f-string interpolation in print and
add_rowcalls inbatch_scan.pyandreports.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
isprintableandisspacesemantics 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 isaction_required. After #733 merges and this PR is retargeted tomain, CI must pass before merge. If #733 is squash-merged, rebase this branch ontomainso onlyb92e02d6remains. ruff checkandruff format --checkpass on both test files. The contrib files are outsidemake lintandformat-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>
|
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
left a comment
There was a problem hiding this comment.
[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 keepsisspace()characters and folds each whitespace run to one space, so技能\u3000名称\u00a0xprints as技能 名称 x(test attests/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:becamex😄even withmarkup=False. Withemoji=Falseit stays literal in markup strings,Textcells andmarkup=Falseprints. Tests:tests/test_batch_scan_reports.py:136(both formatters) andtests/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
Textobjects (reports.py:187-195,:260,:291), so they are never markup-escaped. Tests attests/test_batch_scan_reports.py:137and:165-174assert that no doubled backslash appears.
Material findings
- [Non-blocking]
contrib/batch_scan/batch_scan.py:332: this corrects my last review. I said everydisplay()value is followed by a closing tag, but the--no-llmnon-English warning quotes'{display(rel_name)}'. A directory namedx\still prints as'x\\'in that one stderr line. It is cosmetic. Building that line fromText, 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, where01516f68is #733's branch as merged here. Thef13e98a8merge had one conflict, intests/test_batch_scan_reports.py.git show --remerge-diffshows it was resolved by keeping both appended test blocks, with no other edits. - I checked every print and
add_rowinbatch_scan.pyandreports.pyagain. The remainingescape()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 checkandruff format --checkonsrc/andtests/pass on the full tree. Thecontrib/ruff findings all exist onmain; 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 ontomainso that only this PR's commits remain. git merge-treeis clean against #733,mainand #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)
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
…ries Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com> # Conflicts: # contrib/batch_scan/batch_scan.py
|
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. |
…20261005 fix(batch): enforce the per-skill process timeout
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.