Strip Excel-illegal control characters from inventory report cells (PP-5246)

[dbernstein]

Description

Strips characters that Excel disallows in worksheet cells from report values before they are written.

The stripping happens in _cell_value, the shared conversion point used by both the CSV and Excel writers, so the two outputs stay identical — which _write_reports already documents as a guarantee. It uses openpyxl's own ILLEGAL_CHARACTERS_RE, the same regex its check_string validation checks against, so anything that survives sanitization is guaranteed writable and the two can't drift apart as openpyxl changes. Tab, newline and carriage return are not matched by that regex, so legitimate whitespace is preserved.

Motivation and Context

Inventory report generation was failing outright for at least one library. Imported metadata contained a stray U+001F (unit separator) control character embedded in a contributor name, and openpyxl raises IllegalCharacterError rather than writing it:

IllegalCharacterError: <long contributor list, one name containing \x1f> cannot be used in worksheets.

(The affected value is a contributor list; the names are omitted here, and the tests use a generic placeholder rather than the real data.)

That propagated out of _write_excel_rows and failed the whole generate_inventory_and_hold_reports Celery task, so a single bad character in one row meant no report at all.

How Has This Been Tested?

Two new tests in tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py cover the Excel and CSV paths with control characters in the data, including an assertion that tab and newline survive sanitization.

• All 20 tests in tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py pass.
• mypy is clean on the changed module.
• pre-commit passes on both changed files.

Note on how the tests were run: tox -e py312-docker currently fails in my local environment while building the MinIO image (unauthorized: access to the requested resource is not authorized), which is the known broken-images issue and unrelated to this change. I ran pytest directly against a throwaway postgres:16 container with the PALACE_TEST_DATABASE_URL_* variables tox would have set. These tests only need Postgres, so coverage is equivalent, but the full docker test environment was not exercised locally — CI will cover that.

Checklist

• I have updated the documentation accordingly.
• All new and existing tests passed.

🤖 Generated with Claude Code

[claude]

Claude finished @dbernstein's task in 1m 3s —— View job

---

Summary

No issues found; this looks good to merge. Every path in _cell_value that can return an untrusted string now goes through _sanitize_cell_value: the stringified-column branch and the new plain-str branch. The branches that skip it only return strings the code builds itself: enum .values, and strftime output for timezone-aware datetimes. Other value types (int, Decimal, bool, naive datetime) never reach openpyxl's check_string, so they can't raise IllegalCharacterError. _cell_value is only called by _write_csv_rows and _write_excel_rows, so both outputs get the same sanitization. Reusing openpyxl's own ILLEGAL_CHARACTERS_RE means the sanitizer can't drift from the check openpyxl applies when writing. The enum feedback from @tdilauro is handled in 017a88f. The formula-injection thread was already settled as pre-existing and is tracked in #3769, so I haven't raised it again. The new tests cover the stringified-column and plain-string branches for both writers, plus preservation of tab and newline.

[greptile-apps]

Confidence Score: 5/5

[Medium risk] Strips control characters from inventory report output.

The PR appears safe to merge based on the current changes and resolved previous thread.

Summary

The PR strips Excel-illegal control characters at the shared report-cell conversion point so CSV and XLSX receive the same sanitized values.

• Adds Excel and CSV tests for control-character removal and preservation of legal whitespace.
• The changes since the previous review introduce no new changes to this PR’s files.

_{Reviews (7) · Last reviewed commit: "Don't sanitize enum values"}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.72%. Comparing base (a87d4c7) to head (017a88f).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3768      +/-   ##
==========================================
- Coverage   93.72%   93.72%   -0.01%     
==========================================
  Files         510      510              
  Lines       46510    46515       +5     
  Branches     6313     6314       +1     
==========================================
+ Hits        43590    43594       +4     
- Misses       1886     1887       +1     
  Partials     1034     1034              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tdilauro]

Looks good! 🎸🤘🏽

It seems like this PR should not make it into a release without #3769 (see the thread from greptile).

One minor comment below.

[tdilauro]

Do we need to sanitize our own enum values?