fix(skills): make DocumentAnalysisSkill do the work it reports (#13897)

[mrveiss]

Thinking Path

I filed this issue claiming the skill was unwired with zero callers. That was
wrong, and the truth is worse — corrected on #13897 before implementing.

SkillRegistry.discover_builtin_skills() scans skills.builtin with
pkgutil.iter_modules and registers any BaseSkill subclass it finds. There
is no textual reference to grep for; registration is by module scan. Verified by
running it:

discovered 25 builtin skills
document-analysis registered: True
execute(extract_text) -> {'success': True, 'file_path': '/tmp/x.pdf', 'format': 'markdown',
                          'status': 'queued', 'message': 'Text extraction queued for /tmp/x.pdf'}

So this was not dead code. It was a registered, routable, callable surface
returning fabricated success — three advertised tools reporting completed work
against no queue, no worker and no consumer.

That changes the closure bar. The invariant is not "the handlers now call the
extractor"; it is no handler reports success for work that did not happen.

What Changed

extract_text / analyze_document run real extraction through the canonical
extractor (#13893) and inherit its text-layer detection — a scanned document now
fails with the page numbers needing OCR instead of returning empty success.

The two config fields that changed nothing now change behaviour:

Field	Before	After
max_pages	advertised, ignored	truncates, reports truncated: true
output_format	advertised, ignored	markdown (markers) / plain (stripped) / json (structured pages)
ocr_enabled	defaulted True with no OCR anywhere	defaults False; enabling changes the reported reason rather than pretending OCR ran

summarize_document is deliberately not backed. It extracts for real, then
returns success: False with the text and an explicit statement. The alternatives
were passing extraction off as a summary, or restoring the fabricated queue —
both are the defect this PR removes. Wiring it is #14258: no builtin skill
currently reaches an LLM directly, so choosing that coupling is an architecture
decision that belongs in its own PR, not smuggled into this one.

Path validation before any filesystem access. This skill now genuinely reads
what it is handed, so the check precedes the open(). It uses
contains_dotdot_traversal, not contains_path_traversal — the latter treats /
itself as suspicious because it guards bare filenames, and rejected every absolute
path. I hit that in testing (#9670 documents the distinction).

Verification

$ pytest skills/builtin/document_analysis_test.py -q
22 passed

$ pytest skills/ -q
256 passed

$ pytest tests/test_startup_imports.py -q
352 passed

Mutation-tested:

### MUTATION 1: max_pages ignored (the config that used to change nothing)  → 1 failed
### MUTATION 2: scanned document reported as success again                  → 2 failed
### MUTATION 3: traversal guard removed                                     → 1 failed
### RESTORED                                                                → 22 passed

A fixture correction worth recording. My first fixtures wrote ~3 characters per
page, and every extraction test failed — correctly. The min_chars_per_page floor
(50) treats a 3-character page as a page-number stamp, not content (#13884). The
fixtures now write realistic page bodies. The guard was right and my test documents
were the unrealistic thing.

Scope note

tables_attempted (#13895 / PR #14233) and render_plain (#13894 / PR #14239) are
not used here — both are still unmerged, and I hit import errors reaching for
them. This branch builds against Dev_new_gui only. plain output is therefore
strip_page_markers(render_pages(...)), which is in base.

The document_uploaded trigger is left in the manifest: manifest.triggers is
surfaced only in the routing index and nothing dispatches on it for any skill.
That is a platform-level gap, not this skill's, so removing it here would only
make this one inconsistent with the other 24.

Model Used

Claude Opus 5 (1M context)

Part of #13892 — Wave 3. Does not close #13897 on its own: see #14258.

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No hardcoded values detected that have SSOT config equivalents!

[mrveiss]

Code review

Reviewed from a sweep of unreviewed PRs. No substantive issues. The work is real and reachable: SkillManager.__init__ → discover_builtin_skills() → registry.py:367's pkgutil module scan, so the skill is registered by mechanism rather than by a textual reference, and api/skills.py reaches SkillManager. The diff replaces every handler's fabricated {"success": True, "status": "queued"} with real extraction via extract_document, and _summarize now returns success: False with an explicit unimplemented-backend message instead of a fabricated success — verified in the diff, not taken from the description.

SUPPORTED_SUFFIXES is a capability declaration rather than a tunable threshold, and the config defaults are SkillConfigField entries, which is this codebase's mechanism for user-tunable skill config — not the env-var-backed constant pattern that applies to service TTLs. No violation there.

Tests are capable of failing: reportlab/Pillow are installed in CI (per the startup-import-smoke job), so the PDF fixtures are not silently skipped, and all 12 shards report passes with no failures. The names target concrete regressions (test_no_handler_fabricates_a_queued_success, test_missing_file_reports_failure_not_success) rather than tautologies.

On the failing code-quality check — it is this PR's own, and it is two missing waiver comments:

document_analysis.py:28      [extension-no-core-internals]  imports core 'utils.path_validation'
document_analysis_test.py:22 [skill-no-sibling-import]      imports sibling 'skills.builtin.document_analysis'

No autobot_shared equivalent of contains_dotdot_traversal exists, so the fix is not restructuring the import — it is the inline # nosemgrep: waiver that six sibling files in the same directory already carry (skill_router.py:23, skill_researcher.py:24, skill_router_test.py:12, skill_researcher_test.py:11, community_growth_test.py:17, autonomous_skill_development_test.py:11). Matching that precedent should clear it.

[mrveiss]

Code Quality fixed.

Root cause: Enforce extension/skill/plugin import boundaries (#7372) — a skill may import autobot_shared and its own package only, and I had reached into utils.path_validation, which is core-backend.

Two commits:

1. cdaf9eb97 — switched to autobot_shared.security.path_validator.validate_path. That is not just boundary-legal, it is the stronger check: it canonicalizes and then confines the resolved path to the allowed roots, rather than denylisting .. in the raw string. bug(config): the SSOT itself defaults base_dir, code_source_dir and audit_log_file to /opt/autobot #14050 records why the raw-string denylist is the wrong test. A path containing no .. that still points outside the allowed roots was previously accepted and is now rejected — with a test for that case.

2. afb65d33d — the boundary step scans every *.py under skills/builtin, tests included, so the test importing its own subject tripped skill-no-sibling-import. Every other skill test carries the documented waiver for exactly this; this one now matches.

Verified with the workflow's own invocation rather than the unit test:

$ find autobot-backend/middleware/builtin autobot-backend/skills/builtin plugins/core-plugins \
       -name "*.py" -print0 | xargs -0 python3 tools/lint/check_extension_import_boundaries.py
exit 0

Discovered while fixing this: the checker blocks far less than its docstring claims — membership in an explicit list is the whole test, so media, agents, tools and others are importable from a skill today with no complaint. Filed as #14329 with a decision to make; not fixed here, since inverting the rule will surface violations across other extensions and belongs in its own PR.

Test verification for this PR is CI-side. Please treat the Unit & Integration Tests job result as the gate rather than the overall tick — it is not a required check, so green does not imply it ran.