feat: add modular artifact format validation - #2158
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a robust format-readability validation framework for generated artifacts (such as Office documents, PDFs, CSVs, and images) to ensure they are readable before delivery. It includes backend checks run in isolated subprocesses, integration with workspace file references, and a frontend ArtifactValidation component. The review feedback identifies three key improvements: removing an invalid reset_dimensions() call on read-only worksheets in openpyxl to prevent AttributeError failures, explicitly setting binary mode for standard input on Windows to avoid binary data corruption, and logging original exception details when redacting subprocess errors to preserve debug traces.
rogercloud
left a comment
There was a problem hiding this comment.
Major
src/xagent/core/artifact_validation/formats.py:48—PdfReader(..., strict=True)promotes correctable structural quirks (xref issues, bad object offsets, missing/Length) common in real-world non-Adobe PDFs into fatalPdfReadError, mapped toInvalidArtifact; a perfectly usable PDF gets reported to the model as "INVALID. Repair and recheck." Usestrict=False(pypdf's own default) or map strict-only failures to unchecked instead of invalid.tests/core/test_artifact_validation.py— cache tests never exercise identical byte content under two different filenames/extensions. The cache key atsrc/xagent/core/artifact_validation/service.py:128is(sha256, filename, max_bytes); a regression that drops filename from the key would go undetected. Add a same-bytes-different-filename cache test.src/xagent/core/artifact_validation/office.py—check_package's five zip-bomb/path-traversal guards (absolute/..member rejection ~L30-31, duplicate-member rejection ~L30, missing-required-part ~L40, encrypted-entryflag_bits & 1check ~L46-49,max_entrieslimit ~L25) have zero test coverage despite being the preflight checks against malicious OOXML archives. Add tests for each guard.
Minor
src/xagent/core/artifact_validation/config.py:1818-1822— bareint()parse with no upper ceiling and no shared parser (unlikeXAGENT_MAX_UPLOAD_SIZE's"100M"/"1G"support); a value like"32M"throwsValueErrorthatservice.py:82-83silently swallows into permanentuncheckedwith zero log line. Reuse a shared size parser and log a warning on invalid config.src/xagent/web/api/files.py— the unauthenticatedpublic_preview_fileroute with?validation_only=trueshares the process-globalBoundedSemaphore(2)(service.py:28) with the authenticated preview path; a handful of concurrent anonymous requests can starve other validations touncheckedfor up to the timeout. No rate limiting exists. Add per-user/IP throttling on this parameter.src/xagent/core/tools/artifacts.py:336,workspace_file_tool.py:367,385passvalidate=Truebutpptx_tool.py(~L2019,2085) andimage_tool.py(~L692,832) don't for registry-supported formats — combined with_format_artifact_lines(artifacts.py:203-218) showing identical "UNCHECKED" text for "never checked" vs "checked, inconclusive", a byte-identical .pptx reads differently to the model depending on which tool produced it. Extendvalidate=Trueto these producers, or distinguish "not validated" from "validated, unchecked" in the text.frontend/src/components/file/inline-file-preview.tsx:869-878— wraps every non-audio/video attachment inArtifactValidationregardless of registered validator support, so unsupported types (.txt/.md/.json/.html) permanently show "File not checked" plus a useless "Recheck" button (confirmed viaartifact-validation.test.tsx:79-84fornotes.txt). Gate on a server-advertised supported-extension list.frontend/src/components/file/artifact-validation.tsx:57-58— HTTP 403/500, network errors, the 20s client abort-timeout, and a genuine "no validator installed" report all collapse into the same "File not checked" UI state with no distinct error state.artifact-validation.tsx:29-32,86— server-generated check messages (e.g. "PDF header is missing.") render as raw untranslated English literals even in zh locale; no message-code-to-i18n-key mapping exists.src/xagent/core/tools/artifacts.py:37— unionsGENERATED_ARTIFACT_EXTENSIONSwith the validator registry's extensions, silently causing.tsv/.bmp/.tif/.tiffto now be treated as inline-previewed generated artifacts wherever they weren't before. Call this out explicitly in the PR description as a behavior change.example.envdocumentsXAGENT_ARTIFACT_VALIDATION_MAX_BYTES/XAGENT_ARTIFACT_VALIDATION_TIMEOUT_SECONDSbut neither appears indocker-compose.yml(which does forward the siblingXAGENT_MAX_UPLOAD_SIZE) nor insrc/xagent/web/README.md's config list.
Simplification
src/xagent/core/artifact_validation/registry.py:ArtifactCheckRegistry(register/supports/extensions/validate, with duplicate-name/format guards) is instantiated exactly once indefaults.py:10-25as a static list of 5 hardcoded checks — no second instance or runtime/plugin registration point anywhere. Replace with a plain tuple ofArtifactCheckentries plus free functionssupports(checks, filename)/validate(checks, content) -> ValidationReport, preserving ordering, short-circuit, exception dispatch, and byte-budget preflight without the class/private-dict indirection.
net: -25 lines possible
Blocking: yes — recommended event: COMMENT
src/xagent/core/artifact_validation/formats.py:48, Major, valid non-strict-conformant PDFs get misreported as INVALID to the model, [new]
|
@rogercloud Addressed the review on 9ff1357 in 76bd114. Major findings
Other changes and chosen boundaries
Registry simplification not adopted: the reusable registry is an intentional extension boundary of this PR, with deterministic composition, registration guards, and independently instantiated custom-check coverage. Replacing it with a fixed tuple/free-function interface does not fix a current defect and would change the requested modular API merely to reduce a small amount of code. Validation: 689 backend tests, 1,144 widget tests with coverage thresholds, 82 CI-manifest tests, all applicable pre-commit hooks, and all three Compose configurations passed locally. CI for the new commit is separate and will rerun. This feedback was submitted as a review summary, not resolvable inline threads; the three earlier inline threads remain resolved. |
rogercloud
left a comment
There was a problem hiding this comment.
Major
src/xagent/core/artifact_validation/service.py:28,31,88-101—_public_slotsonly fencespublic=Truecallers; the authenticatedpreview_file?validation_only=truepolling path (src/xagent/web/api/files.py:1776-1894, driven by ordinary UI inline preview perfrontend/src/contexts/file-access-context.tsx:81) and tool-side validation (src/xagent/core/file_ref.py:245-247) both passpublic=Falseand share the same unprotected 2_slots, so the dominant authenticated traffic can still starve tool-side validation. Only public-vs-private contention is tested (tests/core/test_artifact_validation.py:391-420); add a test for authenticated-vs-authenticated contention or extend the slot split to cover it.src/xagent/core/artifact_validation/office.py:99-102— reader exceptions (ValueError, KeyError, IndexError, TypeError, OSError, BadZipFile) are unconditionally mapped toinvalid, with no distinction between genuine corruption and a reader quirk on a legitimate file — the same problem this PR solved for PDF viastrict=False(formats.py:48-50). Apply an analogous fail-open/lenient policy for Office reader errors, and add a test that distinguishes the two cases.src/xagent/core/artifact_validation/office.py:16-20,35-39andregistry.py:78-80— required-part paths (xl/workbook.xml,word/document.xml,ppt/presentation.xml) are hardcoded instead of resolved via[Content_Types].xml/relationships, and the registry breaks after the first non-valid check sooffice-readernever runs to correct a false rejection. A genuinely-openable.xlsxwith its main part renamed (relationships updated) is rejectedinvalideven thoughopenpyxl.load_workbookopens it fine. Resolve parts via relationships, or don't short-circuit before the real reader check runs; add a relationship-correct-but-renamed-part test.src/xagent/core/artifact_validation/formats.py:31—check_csvusescsv.reader(..., strict=True), rejecting input likea,b\n1,"he said "hi" there"\nthat bothpandas.read_csvand the non-strict defaultcsv.readerparse fine, contradicting this PR's own readability-over-conformance policy applied to PDF in the same file. Switch tostrict=False(or catch and fall back), and add a false-positive test (only an unterminated-quote case is currently tested attests/core/test_artifact_validation.py:114).src/xagent/core/artifact_validation/formats.py:56-62—check_pdf's byte-budget loop callsstream.get_data()without validating the decoded content; corrupting a PDF's FlateDecode payload (structure/xref intact) makes pypdf's decoder swallow thezlib.errorand return garbled bytes, so the check reportsstatus="valid"for a page whose content is actually corrupted, and the check's own error message is unreachable for this failure mode. Detect decode failures explicitly (re-raise or check decoded output) and add a corrupted-content-stream test.src/xagent/core/artifact_validation/service.py:56— the parser subprocess inherits the full host environment (env={**os.environ, "OPENBLAS_NUM_THREADS": "1"}) while being the pipeline's only contact point with untrusted bytes via parser libraries with CVE history (pypdf/Pillow/openpyxl/python-docx/python-pptx), with no isolation beyond anRLIMIT_AScap — weaker than this codebase's ownsandboxed_tool_wrapper.py's_build_execution_env(), which builds a minimal allowlisted env instead of inheritingos.environ. Scrub to an allowlist (PATH, locale,OPENBLAS_NUM_THREADS) to avoid exposing DB/API/OAuth/session secrets to a parser RCE.
Minor
src/xagent/core/artifact_validation/service.py(public capacity design) — the single_public_slotsslot only guards against public-vs-private starvation; a second concurrent public request is immediately turned tounchecked, and nothing stops one client from issuing unlimited sequential?validation_only=truerequests to hold that slot indefinitely, denying the feature to other anonymous users. Out of scope per the PR's own stated boundary, but worth a follow-up.src/xagent/core/artifact_validation/worker.py:14-17— theRLIMIT_AScall is unguarded; if a hardRLIMIT_ASbelow 1GiB is already in force,setrlimitraises unhandled and every validation call fails with a logged exception. Wrap intry/except (ValueError, OSError): pass.src/xagent/core/artifact_validation/formats.py:79-82— the multi-frameseek/loadloop (multi-page TIFF/animated GIF/WEBP) and the newly-advertised.bmp/.tif/.tiffformats have zero test coverage (only.pngis exercised). Add at least one multi-frame and one non-PNG raster test.src/xagent/core/file_ref.py:216-247—build_workspace_file_ref(validate=True, internal=True)silently performs no validation since the validation code is nested insideif not internal:. No current call site combines both, but thevalidatedocstring doesn't mention this interaction — add a docstring note or a guard.
Blocking: no — recommended event: COMMENT
|
Handled the review on 76bd114 in 63008c4; every inline item has an individual reply. Fixed demonstrated compatibility/reliability gaps: metadata-resolved OOXML main parts, ambiguous CSV verdicts, silently empty standalone FlateDecode recovery, and inherited Linux memory limits. Added renamed/missing-part fixtures for all three Office types and multi-frame/non-PNG coverage. 305 focused backend tests (including 75 artifact-validation tests) and applicable pre-commit checks pass on the current head. Not expanding scope to authenticated-user/tool scheduling, general throttling, security sandboxing, or wholesale Office-reader fail-open behavior. Those Major labels are not supported by a concrete deployment regression or a legitimate reader-compatible fixture. Registry short-circuiting stays: reader success cannot override a package safety or budget failure. The stated sequential-public-client fairness concern remains separate follow-up work, as the review itself acknowledges. PDF validation remains bounded reader-based checking, not proof of every decoded stream's semantics or visual fidelity. The concrete silent-empty case is addressed without rejecting successful decoder recovery or introducing a complete PDF decoder/renderer. No docs files, extra dependencies or service restarts. |
rogercloud
left a comment
There was a problem hiding this comment.
Critical
(none)
Major
src/xagent/core/tools/adapters/vibe/sandboxed_tool/sandboxed_tool_wrapper.py:498-502 MCP tool structured_content output never passes through the top-level-only validation strip, and _sanitize_tool_result_value (artifacts.py:239-252) preserves nested validation keys — a malicious MCP server can forge a "valid" verdict straight to the model. Strip validation recursively regardless of nesting, or drop it from SAFE_FILE_REF_KEYS and only re-attach from host-rebuilt refs.
src/xagent/core/artifact_validation/service.py:96,118-122 MemoryError (and other non-OSError) escapes the except OSError guard in _validate_snapshot; artifacts.py:326-334's except Exception: ... continue then silently drops the real generated file from file_refs entirely, and workspace_file_tool.py:364,382 has no guard at all so the same exception turns a successful write into a reported failure. Catch broadly (include MemoryError) and degrade to unchecked instead of dropping/failing.
src/xagent/core/artifact_validation/service.py:29-31 _slots = BoundedSemaphore(2) is a single process-wide pool shared by every authenticated preview and tool-call validation, with no per-user/route limiter (web/api/files.py:256-258); the frontend fires one validation request per attachment (artifact-validation.tsx:48-69) and artifacts.py:323-350 validates every changed file sequentially uncapped — either ordinary pattern can stall all other users' validation requests for minutes. Add a per-user/route concurrency cap ahead of the shared semaphore and bound/parallelize per-call file validation.
src/xagent/web/api/files.py:1783-1784,1851-1858,1887-1894 preview_file never passes public_validation=True even when via_stream_ticket is true, unlike public_preview_file (files.py:2109-2118). Stream-ticket traffic (a shareable capability URL) thus bypasses the public-capacity reservation and consumes private _slots. Pass public_validation=via_stream_ticket.
Minor
src/xagent/core/artifact_validation/defaults.py:14 registers office-reader unconditionally though python-docx/openpyxl/python-pptx are optional extras; registry.py:33-35,61-66 reports .xlsx/.docx/.pptx as supported on a minimal install but every check permanently returns unchecked. Feature-detect the import in supports().
docs/artifact-validation.md was added then deleted in this same PR (commit 9ff135712) with no replacement, leaving the feature undocumented despite the PR description claiming no docs changes.
src/xagent/core/tools/core/image_tool.py:692,832 and pptx_tool.py:2019,2091 don't pass validate=True for registry-supported .png/.pptx outputs, unlike artifacts.py:338/workspace_file_tool.py:364,382, so identical formats get inconsistent validation status depending on producer.
src/xagent/web/README.md:150 documents XAGENT_MAX_UPLOAD_SIZE but omits the two new XAGENT_ARTIFACT_VALIDATION_MAX_BYTES/XAGENT_ARTIFACT_VALIDATION_TIMEOUT_SECONDS vars already present in docker-compose.yml/example.env.
Simplification
L20-82: overengineered ArtifactCheckRegistry class with only one instantiation site (defaults.py:10, a fixed 5-check list). Replace with a module-level tuple of checks plus free supports()/validate() functions.
net: -15 lines possible
Blocking: yes — recommended event: COMMENT
- sandboxed_tool_wrapper.py:498-502, forged validation verdict reaches model [new]
- service.py:96,118-122, generated files silently dropped from model's view on exception [new]
- service.py:29-31, shared validation pool exhaustible by ordinary usage, cross-user stall [new]
|
Review closeout at 02923e8: Fixed nested guest validation trust, recoverable snapshot-exception isolation, and stream-ticket classification into the public reservation. New tests cover nested structured_content through the wrapper/public sanitizer (including registration failure), MemoryError/RuntimeError during file reads without losing generated attachments, and a real ticket-only validation request. 271 focused tests, Linux-target mypy, and applicable pre-commit checks pass. The expanded live-sandbox run had 272 passes and 7 failures: all seven failed before tool execution because the test sandbox image cannot import fsspec. This is a validation environment limitation, not presented as a green live-sandbox run; no image/environment changes were made for this PR. Remaining boundaries are unchanged: no authenticated-user scheduling/QoS subsystem, no runtime dependency-probing API, and no removal of the explicitly requested modular registry. Other producers remain clearly NOT RUN rather than falsely checked; that explicit-state alternative was chosen and documented in the previous review round, so adding synchronous validation to image/PPTX producers is not a new merge prerequisite. No docs/README additions: the user explicitly excluded docs. The net PR file list contains no docs changes; an earlier add-then-delete in commit history does not alter that final diff. Configuration remains documented in example.env and wired through Compose. This is not grounds to reintroduce the removed documentation. |
rogercloud
left a comment
There was a problem hiding this comment.
Major
src/xagent/core/artifact_validation/formats.py:59— PDF page-count validation is hard-capped atmin(content.limits.max_units, 500), a literal constant with no env var or config path to raise it. A legitimate PDF over 500 pages silently skips validation of pages 501+, and the cap is undocumented and untested. Exposemax_unitsvia config likemax_bytes/timeout already are, or document the ceiling.
Minor
src/xagent/core/artifact_validation/office.py:93— The except tuple omitsAttributeError, which python-docx/python-pptx raise on a well-formed-but-semantically-corrupt main part (reproduced with python-docx 1.2.0 / python-pptx 1.0.2). This falls through to the generic checker-bug path and reportsuncheckedinstead of the more preciseinvalid. AddAttributeErrorto the caught tuple.src/xagent/core/artifact_validation/formats.py:104— The PDF except tuple missespypdf.errors.PyPdfErrorsubclasses that aren'tPdfReadErrorsubclasses, e.g.LimitReachedError(reproduced on an oversized FlateDecode stream) andDependencyError. These escape to the generic checker-bug path with a logged traceback instead of a cleanuncheckedclassification. CatchPyPdfErrorbroadly, or add these subclasses explicitly.src/xagent/core/artifact_validation/formats.py:87— Content-stream decompression viapart.get_data()happens before themax_expanded_bytesbudget check; the currently-pinned pypdf 6.x has its own 75MB internal limiter masking this, butpyproject.toml:56only pinspypdf>=4.0.0with no upper bound. A future resolve to pypdf 4.x/5.x would silently remove that safety net. Pin an upper bound on pypdf, or add an explicit incremental decompression cap independent of the library.src/xagent/core/artifact_validation/formats.py:90—decoder.decompress(part._data, 1)reads a private, underscore-prefixed pypdf attribute with no public equivalent. A future pypdf release renaming/removing_datawould silently degrade all PDF validation touncheckedvia the generic exception path. Wrap this access so anAttributeErrorhere maps to an explicit "pypdf API changed" reason instead of the generic checker-bug path.src/xagent/core/artifact_validation/worker.py:32—output.write(json.dumps(...))on the rebound former-stdout handle has no explicit.flush(). Sincesys.stdoutwas reassigned earlier, CPython's exit-timeflush_std_files()no longer covers this object, so the write currently survives only via GC finalization. Add an explicitoutput.flush()after the write.src/xagent/core/artifact_validation/service.py:132— Theidentity(stat: os.stat_result)parameter name shadows the module-levelimport statused elsewhere in the file (e.g.stat.S_ISREG). No current bug, but any future line inside this function usingstat.*would silently break. Rename the parameter (e.g.stat_result).src/xagent/core/artifact_validation/service.py:145-161— No in-flight request deduplication: concurrent validation requests for the same not-yet-cached file each spawn their own worker, wasting one of only 2 total parser slots on redundant work. Self-limiting given the 2-slot cap, but worth coalescing concurrent identical requests behind one in-flight computation.frontend/src/components/file/inline-file-preview.tsx:878— A new wrapper<div data-artifact-validation=...>(notdisplay:contents) now encloses every inline preview where none existed before, with no layout regression test. This could break flex/grid sizing assumptions in parent containers. Worth a quick visual check in the affected preview layouts.src/xagent/config.py:1817-1834— The max-bytes/timeout getters have no upper bound (only positive+finite is checked), and unlikeget_max_upload_size_bytes()(which treats an empty-string env var as "use default"), an empty-stringXAGENT_ARTIFACT_VALIDATION_MAX_BYTESraisesValueErrorand disables validation entirely via a single log warning. Align empty-string handling with the upload-size getter, and consider a sane upper bound.src/xagent/core/tools/artifacts.py:220-223— The model-facing rendering collapses ~10 distinctuncheckedroot causes (unsupported format, capacity busy, byte budget exceeded, worker crash, missing dependency, etc.) into one identical status string, discarding the per-checkmessagethe API response actually carries. This makes "no validator exists" indistinguishable from "the validator crashed" to the agent. Surface the per-checkmessage(or at least a coarser reason code) in the model-facing text.
Simplification
src/xagent/core/artifact_validation/registry.py:20— yagni:ArtifactCheckRegistryis a full register/lookup class with exactly one production instantiation and one.register()caller (a hardcoded static tuple built once in defaults.py, never varied at runtime). Replace with a plain tuple/list ofArtifactCheckbuilt directly in defaults.py plus free functionssupports(checks, filename)/validate(checks, content).frontend/src/components/file/artifact-validation.tsx:16— shrink: the['valid','invalid','unchecked']list is recreated and checked via.includesat 2 separate points. Hoist a single module-levelconst STATUSES = new Set([...] as const)and reuse it.
net: -18 lines possible
Blocking: no — recommended event: APPROVE
Summary
Introduce modular, skill-independent artifact format validation, separate from tool execution success.
Scope and behavior changes
This provides bounded, reader-based format checks, not business correctness, visual fidelity or task completion. Unsupported formats, absent optional readers and exhausted budgets remain unchecked. Markdown table diagnostics and Base64-to-attachment conversion are separate PRs.
Artifact discovery now also includes
.tsv,.bmp,.tif, and.tiffthrough the validator registry. These outputs are registered/exposed as generated artifacts where previously they were not auto-discovered. This does not add a new preview engine or promise visual/business correctness.Earlier review validation
No docs files were added or modified, and no running user services were restarted.
Latest review follow-up (63008c4)