feat: add four editorial-grade output skills#460
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a manual file registration mechanism in the Python and JavaScript executors to ensure files created outside the standard workspace are tracked. It also adds a file_id field to the FileInfo model and introduces several new editorial skills for generating styled HTML, PDF, PPTX, and XLSX reports. Feedback identifies code duplication and performance inefficiencies in the file scanning logic, suggesting a shared utility using optimized directory traversal. Additionally, a redundant error handling block in the workspace file tool was noted for simplification.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a suite of 'editorial' skills for generating high-quality HTML decks, PDF reports, PowerPoint presentations, and Excel financial reports. It also enhances the JavaScript and Python executors with a 'belt-and-braces' file registration mechanism to capture files created in working directories that diverge from the main workspace root. Additionally, the FileInfo model now includes a file_id to support clickable file links in chat. Feedback focused on optimizing the new file registration logic in the executors, specifically recommending that manual scans be skipped when the working directory is already within the workspace tree to avoid redundant filesystem operations and expensive path resolutions.
Addresses all five Gemini-code-assist review points on PR xorbitsai#460. Centralise the manual file-registration backfill that python_executor and javascript_executor were each doing inline. Both adapters now just call ``workspace.backfill_files_from_cwd(working_directory)`` around the existing ``auto_register_files()`` block. Key changes from the duplicated inline implementations: 1. **Skip-when-redundant fast path.** When the executor's cwd is already inside ``workspace_dir`` (the common case in tests and for tasks whose workspace id matches the executor's working directory), ``auto_register_files`` already covers those files on its own scan. The new helper detects this with ``wd.is_relative_to(workspace_dir)`` and yields immediately without doing any second walk — avoiding the double-scan Gemini called out. 2. **``os.walk`` with in-place dir pruning** replaces ``rglob("*")`` + post-filter. Hidden subdirs, ``__pycache__`` and ``node_modules`` are pruned from ``dirnames`` so the walk never descends into them — significantly faster on trees that contain large ``node_modules`` directories (which is exactly the case for the pptxgenjs-using JS executor). 3. **No more ``p.resolve()`` inside the snapshot loop.** ``os.walk`` already yields absolute paths when given an absolute root, so we resolve the root once outside the walk and reuse it. Each entry in the resulting set is already absolute and comparable across the before/after snapshots without further normalisation. 4. **Module-level logger.** The inline helpers were creating a per-call ``_log`` shadow of the module logger; both adapters now use the module-level ``logger`` they already declared at import time. 5. **No per-call function redefinition.** The inline ``_scan_cwd`` was being defined fresh on every executor invocation; the extracted ``_scan_user_files`` is a module-level function. Also drop the redundant ``try/except`` around ``self.workspace.get_file_id_from_path(...)`` in ``workspace_file_tool.py``: that method already swallows internal exceptions and returns ``None``, so the outer guard was dead code. Tests: 60 tests across javascript_executor / python_executor / workspace / file_analysis suites pass; ruff check + format clean.
|
Thanks @gemini-code-assist — all five points addressed in 44f13e0. Code duplication (comments 1, 2, 4, 5): Extracted into Redundant manual scans: The helper checks
Per-call function definitions / redundant logger: workspace_file_tool.py redundant try/except (comment 3): Dropped. 60 tests across the touched + adjacent suites pass; ruff check + format clean. |
|
The changes in 44f13e0 correctly address the identified issues. Centralizing the file backfill logic into |
Addresses all five Gemini-code-assist review points on PR xorbitsai#460. Centralise the manual file-registration backfill that python_executor and javascript_executor were each doing inline. Both adapters now just call ``workspace.backfill_files_from_cwd(working_directory)`` around the existing ``auto_register_files()`` block. Key changes from the duplicated inline implementations: 1. **Skip-when-redundant fast path.** When the executor's cwd is already inside ``workspace_dir`` (the common case in tests and for tasks whose workspace id matches the executor's working directory), ``auto_register_files`` already covers those files on its own scan. The new helper detects this with ``wd.is_relative_to(workspace_dir)`` and yields immediately without doing any second walk — avoiding the double-scan Gemini called out. 2. **``os.walk`` with in-place dir pruning** replaces ``rglob("*")`` + post-filter. Hidden subdirs, ``__pycache__`` and ``node_modules`` are pruned from ``dirnames`` so the walk never descends into them — significantly faster on trees that contain large ``node_modules`` directories (which is exactly the case for the pptxgenjs-using JS executor). 3. **No more ``p.resolve()`` inside the snapshot loop.** ``os.walk`` already yields absolute paths when given an absolute root, so we resolve the root once outside the walk and reuse it. Each entry in the resulting set is already absolute and comparable across the before/after snapshots without further normalisation. 4. **Module-level logger.** The inline helpers were creating a per-call ``_log`` shadow of the module logger; both adapters now use the module-level ``logger`` they already declared at import time. 5. **No per-call function redefinition.** The inline ``_scan_cwd`` was being defined fresh on every executor invocation; the extracted ``_scan_user_files`` is a module-level function. Also drop the redundant ``try/except`` around ``self.workspace.get_file_id_from_path(...)`` in ``workspace_file_tool.py``: that method already swallows internal exceptions and returns ``None``, so the outer guard was dead code. Tests: 60 tests across javascript_executor / python_executor / workspace / file_analysis suites pass; ruff check + format clean.
martinma51
force-pushed
the
editorial-skills-and-file-registration
branch
from
44f13e0 to
7566374
Compare
|
Rebased onto current upstream/main — was -8 behind, including #383 (durable file storage layer) which touches Preserved the original 3-commit history ( Local checks (against the rebased branch):
Ready for review. |
Addresses all five Gemini-code-assist review points on PR xorbitsai#460. Centralise the manual file-registration backfill that python_executor and javascript_executor were each doing inline. Both adapters now just call ``workspace.backfill_files_from_cwd(working_directory)`` around the existing ``auto_register_files()`` block. Key changes from the duplicated inline implementations: 1. **Skip-when-redundant fast path.** When the executor's cwd is already inside ``workspace_dir`` (the common case in tests and for tasks whose workspace id matches the executor's working directory), ``auto_register_files`` already covers those files on its own scan. The new helper detects this with ``wd.is_relative_to(workspace_dir)`` and yields immediately without doing any second walk — avoiding the double-scan Gemini called out. 2. **``os.walk`` with in-place dir pruning** replaces ``rglob("*")`` + post-filter. Hidden subdirs, ``__pycache__`` and ``node_modules`` are pruned from ``dirnames`` so the walk never descends into them — significantly faster on trees that contain large ``node_modules`` directories (which is exactly the case for the pptxgenjs-using JS executor). 3. **No more ``p.resolve()`` inside the snapshot loop.** ``os.walk`` already yields absolute paths when given an absolute root, so we resolve the root once outside the walk and reuse it. Each entry in the resulting set is already absolute and comparable across the before/after snapshots without further normalisation. 4. **Module-level logger.** The inline helpers were creating a per-call ``_log`` shadow of the module logger; both adapters now use the module-level ``logger`` they already declared at import time. 5. **No per-call function redefinition.** The inline ``_scan_cwd`` was being defined fresh on every executor invocation; the extracted ``_scan_user_files`` is a module-level function. Also drop the redundant ``try/except`` around ``self.workspace.get_file_id_from_path(...)`` in ``workspace_file_tool.py``: that method already swallows internal exceptions and returns ``None``, so the outer guard was dead code. Tests: 60 tests across javascript_executor / python_executor / workspace / file_analysis suites pass; ruff check + format clean.
martinma51
force-pushed
the
editorial-skills-and-file-registration
branch
from
7566374 to
2df5151
Compare
Addresses all five Gemini-code-assist review points on PR xorbitsai#460. Centralise the manual file-registration backfill that python_executor and javascript_executor were each doing inline. Both adapters now just call ``workspace.backfill_files_from_cwd(working_directory)`` around the existing ``auto_register_files()`` block. Key changes from the duplicated inline implementations: 1. **Skip-when-redundant fast path.** When the executor's cwd is already inside ``workspace_dir`` (the common case in tests and for tasks whose workspace id matches the executor's working directory), ``auto_register_files`` already covers those files on its own scan. The new helper detects this with ``wd.is_relative_to(workspace_dir)`` and yields immediately without doing any second walk — avoiding the double-scan Gemini called out. 2. **``os.walk`` with in-place dir pruning** replaces ``rglob("*")`` + post-filter. Hidden subdirs, ``__pycache__`` and ``node_modules`` are pruned from ``dirnames`` so the walk never descends into them — significantly faster on trees that contain large ``node_modules`` directories (which is exactly the case for the pptxgenjs-using JS executor). 3. **No more ``p.resolve()`` inside the snapshot loop.** ``os.walk`` already yields absolute paths when given an absolute root, so we resolve the root once outside the walk and reuse it. Each entry in the resulting set is already absolute and comparable across the before/after snapshots without further normalisation. 4. **Module-level logger.** The inline helpers were creating a per-call ``_log`` shadow of the module logger; both adapters now use the module-level ``logger`` they already declared at import time. 5. **No per-call function redefinition.** The inline ``_scan_cwd`` was being defined fresh on every executor invocation; the extracted ``_scan_user_files`` is a module-level function. Also drop the redundant ``try/except`` around ``self.workspace.get_file_id_from_path(...)`` in ``workspace_file_tool.py``: that method already swallows internal exceptions and returns ``None``, so the outer guard was dead code. Tests: 60 tests across javascript_executor / python_executor / workspace / file_analysis suites pass; ruff check + format clean.
martinma51
force-pushed
the
editorial-skills-and-file-registration
branch
2 times, most recently
from
a006318 to
9bd5f1a
Compare
martinma51
changed the title
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Leaving the validated review findings inline. I grouped related symptoms together so each thread points at one underlying contract issue.
Add four new builtin skills focused on magazine-grade visual output for "face-of-the-product" deliverables (investor pitch decks, board reviews, executive whitepapers, customer-facing dashboards). - `html-deck-editorial` — single-file HTML slide deck with inline CSS, keyboard navigation, hash sync, top progress bar; 5 palettes; 10 numbered layouts (L01 cover → L10 closing). Zero JS dependencies. - `pptx-editorial` — native .pptx via pptxgenjs (npm), same 5 palettes and 10 layouts as html-deck. Generates async-IIFE-wrapped JS through `execute_javascript_code` to avoid the Node top-level-await / CommonJS require conflict. - `xlsx-financial-report` — native .xlsx via openpyxl (already in backend .venv). Five financial-report palettes (Bloomberg Mono / Indigo Sheet / Forest Ledger / Crimson Quarterly / Slate Audit), freeze panes, alternating row bands, conditional formatting, monochrome data bars. Currency defaults to USD with explicit guidance on when to switch to CNY / EUR / GBP. Ships copy-pasteable openpyxl 3.x chart snippets to avoid the common kwarg-name footguns (`ln=` not `line=`, `w=` not `width=`, `ChartLines(spPr=...)` wrap). - `pdf-report-editorial` — editorial PDF (whitepaper / brief / memo) via HTML → Chrome headless. Same 5 palettes, A4 + print-aware CSS, drop caps, pull quotes, callout boxes, paginated footer. Each skill enforces: - One palette per output, no hex mixing - Two fonts max (display + body), all system-available - Forbidden visual junk (drop-shadow, 3D, gradients, rounded > 2px) - No fabricated data — drop the slot rather than invent - Language matches user prompt (Chinese prompt → fully Chinese output, no English template phrases leaking through) - CJK text uses `charSpacing: 0` to avoid the per-glyph gap artifact - Final answer leads with bare `[name](file:UUID)` chip link so the output is clickable in chat - Each skill includes a pre-write output checklist for the agent to self-verify before declaring success
martinma51
force-pushed
the
editorial-skills-and-file-registration
branch
from
9bd5f1a to
9f83f8a
Compare
…tsai#460 [#1] Replace `triggers:` frontmatter with `tags:` + `when_to_use:`. ``SkillParser.parse()`` only reads ``description`` / ``when_to_use`` / ``tags`` / ``files`` / ``content``, so the trigger phrases were silently ignored while the keyword-tag fallback could attach unrelated tags. The trigger phrases that aren't already in the ``description`` are folded in there; explicit ``tags:`` now declare the selection vocabulary the runtime actually consumes. [xorbitsai#2] Drop the ``get_file_info(...)`` → ``file_id`` instruction in all four skills. ``FileInfo`` does not include a ``file_id`` field, so following the previous wording yielded fabricated UUIDs and dead chips. Each skill now tells the model to read ``markdown_link`` / ``file_refs[].markdown_link`` from the producing tool's own response (the contract upstream xorbitsai#439 ships) and copy it verbatim. Calling ``get_file_info`` for chips is now an explicit anti-pattern. [xorbitsai#3] pdf-report-editorial: rewrite the PDF export step from the nonexistent ``browser_use`` dispatcher with ``navigate``/``pdf`` sub-actions to the actually-callable tools: ``browser_navigate`` (workspace path or ``file://`` URL) followed by ``browser_pdf(output_filename=..., format="A4", print_background=True)``. ``browser_pdf`` does not accept a ``margin`` object; page margins are set in CSS ``@page`` rules inside the HTML. The base64 fallback hint is dropped — the tool writes the file directly and returns ``markdown_link``. [xorbitsai#4] html-deck-editorial: resolve the "no external ``<link>``" vs "load Playfair Display + Inter from Google Fonts" contradiction by relaxing Hard Rule 1 to allow exactly one Google Fonts stylesheet ``<link>``. All other external scripts/images/CDNs remain forbidden. [xorbitsai#5] xlsx-financial-report: align the palette contract on 4 colors (``ink`` / ``paper`` / ``paper_tint`` / ``accent``) — ``paper_tint`` is required by the alternating-row band, so claiming "only 3 usable colors" while consuming the 4th was internally contradictory. All chart snippets now reference a single ``palette = {…}`` dict instead of hard-coding hex literals (including the stray ``E5E7EB`` that wasn't in any palette). [xorbitsai#6] xlsx-financial-report: document that ``execute_python_code`` 's sandbox ships only ``pandas`` / ``numpy`` / ``matplotlib`` — openpyxl is NOT preinstalled. The skill now instructs the model to pass ``packages=["openpyxl>=3.1.0"]`` on every invocation so the very first ``import openpyxl`` doesn't ModuleNotFoundError. [xorbitsai#7] pdf-report-editorial: pick one heading hierarchy. The output checklist already said "H1 only on cover; H2 for section dividers", so the typography table now labels H1 as cover-only and the ``page-break-before`` rule applies to ``section.chapter`` wrappers or the H2 section-divider rule instead of conflicting with the checklist. [xorbitsai#8] pptx-editorial: remove the contradictory "Node env has pptxgenjs preinstalled" claim. The executor only has pptxgenjs when it's supplied through ``packages``; the skill now states that explicitly and tells the model to pass it on every run. Also switch any remaining internal-name references (``python_executor`` / ``javascript_executor``) to the public tool names (``execute_python_code`` / ``execute_javascript_code``) the agent actually sees.
|
Thanks @rogercloud — all 8 issues addressed in 481c84b. The threads were marked resolved on GitHub but the SKILL.md content hadn't been updated; this commit closes them at the file level. Quick map: [1] [2] [3] PDF skill [4] HTML deck Google Fonts vs no- [5] XLSX palette 3 vs 4 colors — Aligned everything on 4 colors ( [6] openpyxl not in sandbox — Added an explicit "Required runtime packages" section: openpyxl is NOT preinstalled, the model must pass [7] PDF skill H1 vs H2 / page-break conflict — Picked the hierarchy the output checklist already enforced (H1 cover-only, H2 chapter dividers). Updated the typography table to label H1 as cover-only and moved [8] pptxgenjs "preinstalled" claim — Removed. Replaced with explicit "not preinstalled in the sandbox — supplying it via |
3 participants
Summary
Adds four magazine-grade editorial output skills (HTML deck / PPTX / XLSX / PDF) and fixes two related file-registration bugs that broke chat chip links for any agent-generated workspace file.
What's in this PR
Two commits, can be reviewed independently:
fix(tools): register raw-write files + expose file_id from get_file_info— backend file-registration bugfixfeat(skills): add four editorial-grade output skills— four new SKILL.md files undersrc/xagent/skills/builtin/The bugfix (commit 1)
Bug A —
python_executor/javascript_executoradapters miss raw-fs-IO filesworkspace.auto_register_files()scansworkspace.workspace_dir. The executor's actualworking_directory(resolved viaworkspace.resolve_path("")) can be a sibling tree — e.g. workspace id"67"→ workspace_dir isuser_1/67, but the executor cwd lands inuser_1/web_task_67/output. Files written byopenpyxl,pptxgenjs.writeFile,fs.writeFileSync, etc. land in the latter; the scan misses them entirely; they never reachuploaded_files; the frontend has no UUID for chip links.Fix: in both adapters, snapshot the executor's actual
working_directorybefore and afterexecute_code(), diff the file sets, and explicitly callworkspace.register_file()on the deltas.register_file()already dedupes via the path → file_id DB lookup, so this is a no-op when the dirs coincide.Bug B —
get_file_infodoesn't returnfile_idWorkspaceFileOperations.get_file_info()returned name / size / mtime but never the registeredfile_id. When an LLM asks "what's the UUID for foo.pptx so I can build a chip link" the tool gave it no way to answer — the agent guessed a UUID, the chip 404'd, the user saw a dead link. Same failure mode that PR #432's Rule #10 partially addresses, but here the agent has no real value to fall back on.Fix: add
file_id: Optional[str]toFileInfoand populate it fromworkspace.get_file_id_from_path()insideget_file_info().Combined effect
openpyxl-written .xlsx, pptxgenjs-written .pptx, and other raw-fs-IO files now auto-register, and the agent can retrieve the real UUID to render a clickable chip in its final answer.
Relationship to other PRs
This sits at a different layer than the recent pptxgenjs-error-handling PRs:
success: falseThese are independent. No file-level conflict with #442 (this PR touches
core/tools/adapters/vibe/*_executor.pyandcore/tools/core/workspace_file_tool.py; #442 touchescore/tools/core/javascript_executor.py).The skills (commit 2)
Four builtin skills focused on "face-of-the-product" deliverables:
html-deck-editorialpptx-editorial.pptxexecute_javascript_codexlsx-financial-report.xlsxexecute_python_codepdf-report-editorial.pdfZero new dependencies.
pptxgenjsis already in_get_deps.openpyxlis already in the backend.venv. Each skill is a singleSKILL.mdfile (markdown + YAML frontmatter), no helper code modules.What each skill enforces
charSpacing: 0to avoid the per-glyph gap artifact[name](file:UUID)chip link so the output is clickable in chatWhy these instead of extending
presentation-generatorpresentation-generatoris a defensive general-purpose generator (4 fixed themes, allows placeholder text, no language matching). These four skills take an opinionated editorial-design stance — closer to Monocle / Economist / NYT style guides — for cases where visual quality matters more than flexibility. They coexist withpresentation-generator; the skill selector picks based on description and triggers.Verification
End-to-end run of all 4 skills with Chinese prompts (Xagent fundraising deck, community growth dashboard, enterprise whitepaper, dev-community pitch):
All chips resolve to real files in
uploaded_files. Zero manual backfill required.Test plan
presentation-generatorskill still selects for generic "make slides" prompts (i.e. the new skill doesn't over-trigger)pip install-style new deps: none (only existingopenpyxl+pptxgenjs)🤖 Generated with Claude Code