Knowledge base documents: one-shot PDF upload and metadata CRUD

[mocha06]

Motivation

Fourth phase of the knowledge-base milestone (#413, depends on #412). Pipes can hold PDF documents as agent knowledge, but the API contract is a three-step dance (presign → S3 PUT → create) with no validation on the URL path — the backend's PDF/size checks only run for raw uploads, which this flow never uses. Agents and CLI users need a single safe call that does the whole pipeline and fails fast with a clear step tag instead of a late 422 or a silently unindexable document.

What ships

• create_ai_knowledge_base_document (MCP) / pipefy kb document create --file … (CLI): one-shot create from a local PDF path — resolves the pipe's organization, mints a presigned URL, PUTs the bytes, then runs createAiKnowledgeBaseDocument with the persistent download URL. Every failure carries step: file_read | presigned_url | s3_upload | kb_create. No standalone presign tool.
• get_ai_knowledge_base_document / pipefy kb document get: fetch by ID; content is the stored document URL, not extracted text.
• update_ai_knowledge_base_document / pipefy kb document update: metadata-only (name/description) in v1; no file replacement.
• delete_ai_knowledge_base_document / pipefy kb document delete: preview-then-confirm (MCP confirm=True, CLI --yes/prompt).
• SDK: KnowledgeBaseService document methods reusing the attachment pipeline's presign mutation and S3 uploader (same host allowlist, no auth header leakage to S3, body snippet capped on errors).

Proven contract (traced to the owning layer, not the schema)

• All operations are pipe-scoped by pipe UUID; create's presign step needs the organization, resolved internally from the pipe — callers never pass an org ID.
• documentUrl must be the persistent download URL from createPresignedUrl, not the single-use upload URL: the backend stores it and the async indexer fetches the PDF from it later. A pre-signed-PUT URL there would 403 at indexing time.
• Client-side checks are the only validation on this path: the backend's application/pdf content-type and 20 MiB checks are conditional on a raw upload and are skipped when the document arrives as a URL. The toolkit enforces .pdf (case-insensitive), 20 MiB, and description required 1–900 chars before any network call.
• description is required end-to-end even though the GraphQL schema marks it optional — same shared rule proven for plain texts in the previous phase. (The issue's "description ≤ 900" understated this; confirmed against the backing model and enforced as required.)
• Writes require manage_ai_agents on the pipe; the read probe (validate_knowledge_base_access) proves read access only, and its note says so.

Behavior boundaries

• Indexing is asynchronous: a created document may not be searchable by agents immediately; docs call this out.
• The create tool reads local files, so it is excluded from the remote profile (same rationale as attachment uploads); get/update/delete document tools ship in the local profile with the rest of the KB writes.
• CLI gates create/update on the access probe and requires confirmation for delete; MCP delete is preview-first (confirm=True to execute).

Docs & skills

• docs/mcp/tools/knowledge-bases.md: document tool reference, upload pipeline, step-tag table, async-indexing note.
• docs/parity.md: +4 rows, tool count 168 → 172 (drift-guard line updated).
• README.md: counts and KB area row (6 → 10 tools).
• skills/ai-agents/pipefy-ai-agents/SKILL.md: document workflow guidance.

Testing

Live verification against a test pipe through the reloaded MCP tools:

#	Scenario	Expected	Observed	Verdict
1	validate_knowledge_base_access on pipe	ok=true, count reported	ok:true, knowledge_base_count:3, read-only note	✅
2	create_ai_knowledge_base_document with a local PDF	doc created; content=persistent download URL	created; content = signed storage download URL	✅
3	get_ai_knowledge_base_document by id	name/description/content returned	exact fields returned	✅
4	get_ai_knowledge_bases lists the doc	appears with type: knowledge_base_documents	present, correct type	✅
5	update_ai_knowledge_base_document name+description	metadata updated, content URL unchanged	both updated, updatedAt bumped, content unchanged	✅
6	create with a non-PDF path	rejected with step: file_read, nothing uploaded	File must be a .pdf: …, step:file_read	✅
7	create with blank description	rejected before any I/O	description must be a non-empty string	✅
8	delete without confirm	preview only, mutation not run	requires_confirmation:true preview	✅
8b	delete with confirm=true on a throwaway	executes delete; record gone	deleted_id returned; follow-up get → 404 (classified not_found)	✅

Size-cap and per-step failure paths are proven in-process (SDK/MCP/CLI unit tests) rather than by forcing pathological inputs through the live API. Full suite: 3612 passed / 39 skipped; ruff check + format, lint-imports, version verify, skill linters, and the parity/remote-profile drift guards all green.

Note: a pre-existing, cross-cutting error-disclosure issue on the AI-chat-backed surface (Core serializing internal service details into public GraphQL errors) was found during this work and filed separately as a Core bug; it is not introduced or addressed here.

[adriannoes]

Summary

Solid fourth-phase KB documents surface: one-shot PDF create with client-side .pdf/20 MiB/description guards, download URL (not PUT URL) as documentUrl, metadata-only update, delete confirmation, CLI probe gate, and create kept off the remote profile. Local checkout ran ruff and unit tests on an earlier tip; this review is against e971e1cb after rebase onto the #427 tip. Soft write unwrap is fixed here (_unwrap_document fails closed; create tags it as kb_create). One CLI/MCP parity gap remains before merge.

What worked well

• Wire path and tests lock documentUrl to the persistent download URL and tag HTTP S3 failures with step=s3_upload plus body_snippet.
• Full SDK + MCP + CLI parity, registry, parity matrix, docs, and skills, including async-indexing callouts.
• Tip amend fail-closes null nested write payloads for documents (and brings the plain-text empty-get fix from the #427 tip).

Required before merge

• CLI kb document get must treat SDK {} as not-found (success: false, exit 1), mirroring MCP and the plain-text get pattern already on this tip, with a regression test.

Also noted

• Absolute "every pipeline failure carries step" overstates the shared S3 uploader contract. HTTP status failures from put are tagged; allowlist ValueError and transport errors still bubble bare, same as AttachmentService. Optional follow-up: wrap both call sites, or narrow the docs wording to the HTTP-scoped tag.

Review path

• Worktree review against tip e971e1cb (recheck after amend from f75253c0); earlier local ruff + sdk/mcp/cli unit suite was green on the prior tip.
• Adversarial passes + refutation kept CLI document empty-get; soft unwrap was open on the first tip and is fixed on this amend; put-exception step tagging stayed a shared-attachment summary note.
• Base is now 98c3a8d6 (#427 tip), so stack alignment is in good shape aside from mirroring empty-get onto document get.

[mocha06]

@adriannoes Thanks for the review — both findings verified and addressed, tip is now 7f997f6.

Required (CLI document get empty-result): fixed in 94d87b7, details on the inline thread.

Also noted (step-tag contract vs raising PUT): you're right on both halves — the docs claimed the absolute while transport errors and the allowlist ValueError bubbled bare, and that faithfully mirrored AttachmentService (its step="s3_upload" wrap covers _extract_storage_path, not the PUT). I took the wrap option rather than narrowing the docs: a raising PUT is genuinely an s3_upload-stage failure, so 94d87b7 wraps the call and re-raises tagged, with a raising-uploader test. The documented "every stage carries step" contract now holds instead of being worded around.

The AttachmentService twin gap is pre-existing and stays out of this stacked PR — filed as #430 (same milestone) to bring the original pipeline into parity with the same wrap + test shape.

Validation on the tip: full suite 3618 passed / 39 skipped, ruff check + format, lint-imports, CLI help golden regenerated (exits-1 help text), parity/remote-profile drift guards green.

[adriannoes]

Rechecked tip 7f997f62 after your follow-ups. The CLI document empty-get path now mirrors MCP and plain-text get (_kb_not_found, exit 1, regression test), soft write unwrap stays fail-closed, and S3 PUT exceptions are tagged step=s3_upload as well. Local KB CLI/SDK unit tests and CI are green on tip. Approving.

Thanks for turning around the review notes so cleanly, @mocha06. The shared not-found helper and the stronger S3 wrap were especially welcome.

What worked well

• Empty-get parity with a regression test that locks the contract.
• Wrapping raising S3 PUT failures so the documented step taxonomy holds beyond HTTP status errors.
• Extracting _kb_not_found before a third KB kind would have duplicated the envelope.