Conversation
Adds a new connector that ingests KCP knowledge.yaml manifests into Aurora's Weaviate-backed knowledge base, preserving structured metadata (intent, triggers, audience, temporal validity, dependency chains) that is lost in flat document uploads. The connector consists of three modules: - manifest.py: YAML parser handling KCP 0.6+ and 0.20+ manifests, resolves unit paths relative to the manifest directory, produces typed KCPUnit/KCPManifest dataclasses. - ingest.py: Orchestrator that reads each unit's content file, runs it through Aurora's existing DocumentProcessor for chunking, enriches chunks with KCP metadata in the heading_context field, and uploads via Weaviate. Falls back to standard insert_chunks when the extended schema is not deployed. Includes CLI entry point (python -m connectors.kcp_connector). - weaviate_ext.py: Idempotent schema migration adding 13 KCP-specific properties to KnowledgeBaseChunk, plus insert_chunks_with_metadata that stores them. Design decisions: - uuid5(project:unit_id) for stable document IDs (re-ingest replaces). - Intent prepended to heading_context for immediate searchability in existing knowledge_base_search and search_runbooks results. - Graceful degradation: works against unmodified Aurora by falling back to standard insert_chunks which ignores unknown keys. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 21 minutes and 9 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughA new ChangesKCP Connector: Manifest → Weaviate Ingestion Pipeline
Sequence Diagram(s)sequenceDiagram
participant CLI as python -m kcp_connector
participant ingest as ingest_kcp_manifest
participant manifest as parse_manifest
participant proc as DocumentProcessor
participant ext as insert_chunks_with_metadata
participant weaviate as Weaviate KnowledgeBaseChunk
CLI->>ingest: manifest_path, user_id, org_id, dry_run
ingest->>manifest: load and validate knowledge.yaml
manifest-->>ingest: KCPManifest + KCPUnit list
loop for each KCPUnit
ingest->>proc: chunk content by mapped file type
proc-->>ingest: chunk dicts
ingest->>ingest: enrich chunks with kcp_* metadata and heading_context
ingest->>ext: upload enriched chunks
ext->>weaviate: ensure_kcp_properties idempotent schema update
ext->>weaviate: batch insert objects with deterministic UUIDs
weaviate-->>ext: success or per-chunk errors
ext-->>ingest: inserted count
end
ingest-->>CLI: IngestResult with counts and errors
CLI->>CLI: print summary and exit 0 or 1
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- ingest.py: except Exception → except Exception as e so the error message is included in the warning log - weaviate_ext.py: rename _client → _ to make the intentional discard explicit (SonarQube S1481 dead-store) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/connectors/kcp_connector/ingest.py`:
- Around line 134-135: Before calling _kcp_format_to_aurora_type with
unit.format and unit.path on line 134, add validation and normalization of these
inputs to ensure they are non-null strings. This prevents downstream failures in
the function at line 201 where lower() is called on format and at line 207 where
Path(path) is instantiated, which will raise exceptions if the values are null
or non-string types from the manifest data. Validate and normalize both
unit.format and unit.path before the _kcp_format_to_aurora_type call to catch
invalid data early.
- Around line 166-177: The code currently increments result.units_ingested
unconditionally even when _upload_chunks returns 0, which silently treats failed
uploads as successful and can produce false-zero-error exit codes. After calling
_upload_chunks, check if the returned inserted value equals 0 or is less than
the number of chunks being uploaded (indicating a failure or partial insert).
Only increment result.units_ingested when the insert is completely successful;
otherwise, add an error entry (with unit.id and relevant details about the
failed chunks) to a result.errors list to properly surface the failure instead
of silently marking it as ingested.
In `@server/connectors/kcp_connector/manifest.py`:
- Around line 108-131: The code has two issues: First, the KCPUnit is created
with potentially empty id and path values that should be validated as non-empty
strings to prevent issues downstream with document ID generation. Add validation
after the KCPUnit instantiation to check that both unit.id and unit.path are
non-empty, and skip processing or raise an error if they are. Second, the path
resolution logic does not sanitize unit.path against path traversal attacks
(e.g., ../../etc/passwd). Before constructing the resolved path with
manifest_dir / unit.path, normalize the path and verify that the resulting
resolved path is within the manifest directory hierarchy. You can use pathlib's
resolve() method and check that the resolved path starts with the manifest
directory to ensure no escape is possible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3503e1f-cd17-4e93-b056-fa44226e0b3e
📒 Files selected for processing (5)
server/connectors/kcp_connector/__init__.pyserver/connectors/kcp_connector/__main__.pyserver/connectors/kcp_connector/ingest.pyserver/connectors/kcp_connector/manifest.pyserver/connectors/kcp_connector/weaviate_ext.py
| file_type = _kcp_format_to_aurora_type(unit.format, unit.path) | ||
|
|
There was a problem hiding this comment.
Normalize format/path inputs before file-type mapping.
Line 134 calls _kcp_format_to_aurora_type before chunking error handling. If manifest data contains null/non-string values, Line 201 (lower) or Line 207 (Path(path)) can raise and abort the whole run.
Proposed fix
-def _kcp_format_to_aurora_type(kcp_format: str, path: str) -> str:
+def _kcp_format_to_aurora_type(kcp_format: str | None, path: str | None) -> str:
"""Map KCP ``format`` field to Aurora's file_type enum."""
- fmt = kcp_format.lower()
+ fmt = str(kcp_format or "").lower()
if fmt in ("markdown", "md"):
return "markdown"
if fmt == "pdf":
return "pdf"
# Fallback: derive from file extension
- ext = Path(path).suffix.lower().lstrip(".")
+ ext = Path(str(path or "")).suffix.lower().lstrip(".")
if ext == "md":
return "markdown"
if ext == "pdf":
return "pdf"
return "plaintext"Also applies to: 199-212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/connectors/kcp_connector/ingest.py` around lines 134 - 135, Before
calling _kcp_format_to_aurora_type with unit.format and unit.path on line 134,
add validation and normalization of these inputs to ensure they are non-null
strings. This prevents downstream failures in the function at line 201 where
lower() is called on format and at line 207 where Path(path) is instantiated,
which will raise exceptions if the values are null or non-string types from the
manifest data. Validate and normalize both unit.format and unit.path before the
_kcp_format_to_aurora_type call to catch invalid data early.
| inserted = _upload_chunks( | ||
| user_id=user_id, | ||
| document_id=doc_id, | ||
| source_filename=unit.path, | ||
| chunks=chunks, | ||
| org_id=org_id, | ||
| ) | ||
| result.total_chunks += inserted | ||
| result.units_ingested += 1 | ||
| logger.info( | ||
| "%s Unit '%s': inserted %d chunks", _LOG_PREFIX, unit.id, inserted, | ||
| ) |
There was a problem hiding this comment.
Handle zero/partial inserts as ingestion failures.
Line 174 increments units_ingested even when the uploader returns 0 (no exception). That silently reports success for failed uploads and can produce a false-zero-error CLI exit. Also, partial inserts are not surfaced in errors.
Proposed fix
try:
inserted = _upload_chunks(
user_id=user_id,
document_id=doc_id,
source_filename=unit.path,
chunks=chunks,
org_id=org_id,
)
+ if inserted <= 0:
+ msg = f"Unit '{unit.id}': upload inserted 0/{len(chunks)} chunks"
+ result.errors.append(msg)
+ result.units_skipped += 1
+ logger.error("%s %s", _LOG_PREFIX, msg)
+ continue
+ if inserted < len(chunks):
+ msg = f"Unit '{unit.id}': partial upload ({inserted}/{len(chunks)} chunks)"
+ result.errors.append(msg)
+ logger.warning("%s %s", _LOG_PREFIX, msg)
result.total_chunks += inserted
result.units_ingested += 1
logger.info(
"%s Unit '%s': inserted %d chunks", _LOG_PREFIX, unit.id, inserted,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| inserted = _upload_chunks( | |
| user_id=user_id, | |
| document_id=doc_id, | |
| source_filename=unit.path, | |
| chunks=chunks, | |
| org_id=org_id, | |
| ) | |
| result.total_chunks += inserted | |
| result.units_ingested += 1 | |
| logger.info( | |
| "%s Unit '%s': inserted %d chunks", _LOG_PREFIX, unit.id, inserted, | |
| ) | |
| inserted = _upload_chunks( | |
| user_id=user_id, | |
| document_id=doc_id, | |
| source_filename=unit.path, | |
| chunks=chunks, | |
| org_id=org_id, | |
| ) | |
| if inserted <= 0: | |
| msg = f"Unit '{unit.id}': upload inserted 0/{len(chunks)} chunks" | |
| result.errors.append(msg) | |
| result.units_skipped += 1 | |
| logger.error("%s %s", _LOG_PREFIX, msg) | |
| continue | |
| if inserted < len(chunks): | |
| msg = f"Unit '{unit.id}': partial upload ({inserted}/{len(chunks)} chunks)" | |
| result.errors.append(msg) | |
| logger.warning("%s %s", _LOG_PREFIX, msg) | |
| result.total_chunks += inserted | |
| result.units_ingested += 1 | |
| logger.info( | |
| "%s Unit '%s': inserted %d chunks", _LOG_PREFIX, unit.id, inserted, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/connectors/kcp_connector/ingest.py` around lines 166 - 177, The code
currently increments result.units_ingested unconditionally even when
_upload_chunks returns 0, which silently treats failed uploads as successful and
can produce false-zero-error exit codes. After calling _upload_chunks, check if
the returned inserted value equals 0 or is less than the number of chunks being
uploaded (indicating a failure or partial insert). Only increment
result.units_ingested when the insert is completely successful; otherwise, add
an error entry (with unit.id and relevant details about the failed chunks) to a
result.errors list to properly surface the failure instead of silently marking
it as ingested.
| unit = KCPUnit( | ||
| id=u.get("id", ""), | ||
| path=u.get("path", ""), | ||
| intent=u.get("intent", ""), | ||
| triggers=_as_list(u.get("triggers")), | ||
| audience=_as_list(u.get("audience")), | ||
| not_for=_as_list(u.get("not_for")), | ||
| scope=u.get("scope", ""), | ||
| kind=u.get("kind", "knowledge"), | ||
| format=u.get("format", "markdown"), | ||
| depends_on=_as_list(u.get("depends_on")), | ||
| validated=u.get("validated"), | ||
| temporal=temporal, | ||
| ) | ||
|
|
||
| # Resolve the content file path relative to the manifest directory | ||
| if unit.path: | ||
| resolved = manifest_dir / unit.path | ||
| if resolved.is_file(): | ||
| unit.resolved_path = resolved | ||
| else: | ||
| logger.warning( | ||
| "[KCP] Unit '%s' path does not exist: %s", unit.id, resolved, | ||
| ) |
There was a problem hiding this comment.
Missing validation for required unit fields and potential path traversal.
-
Empty
idorpath: Units with emptyidvalues will pass through, causing issues downstream when generating deterministic document IDs (uuid5(project:unit_id)). Consider validating thatidandpathare non-empty strings. -
Path traversal: The path resolution at line 125 doesn't sanitize
unit.path. A malicious manifest could use../../etc/passwdto read files outside the manifest directory. While the downstream only reads content, this could leak sensitive data.
Proposed fix
unit = KCPUnit(
- id=u.get("id", ""),
- path=u.get("path", ""),
+ id=u.get("id", "") or "",
+ path=u.get("path", "") or "",
intent=u.get("intent", ""),
triggers=_as_list(u.get("triggers")),
audience=_as_list(u.get("audience")),
not_for=_as_list(u.get("not_for")),
scope=u.get("scope", ""),
kind=u.get("kind", "knowledge"),
format=u.get("format", "markdown"),
depends_on=_as_list(u.get("depends_on")),
validated=u.get("validated"),
temporal=temporal,
)
+ if not unit.id:
+ logger.warning("[KCP] Skipping unit with missing 'id' field")
+ continue
+
+ if not unit.path:
+ logger.warning("[KCP] Unit '%s' has no 'path' field", unit.id)
+ units.append(unit)
+ continue
+
# Resolve the content file path relative to the manifest directory
if unit.path:
resolved = manifest_dir / unit.path
+ # Guard against path traversal
+ try:
+ resolved = resolved.resolve()
+ if not resolved.is_relative_to(manifest_dir.resolve()):
+ logger.warning(
+ "[KCP] Unit '%s' path escapes manifest directory: %s",
+ unit.id, unit.path,
+ )
+ units.append(unit)
+ continue
+ except (ValueError, OSError):
+ pass
+
if resolved.is_file():
unit.resolved_path = resolved
else:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/connectors/kcp_connector/manifest.py` around lines 108 - 131, The code
has two issues: First, the KCPUnit is created with potentially empty id and path
values that should be validated as non-empty strings to prevent issues
downstream with document ID generation. Add validation after the KCPUnit
instantiation to check that both unit.id and unit.path are non-empty, and skip
processing or raise an error if they are. Second, the path resolution logic does
not sanitize unit.path against path traversal attacks (e.g., ../../etc/passwd).
Before constructing the resolved path with manifest_dir / unit.path, normalize
the path and verify that the resulting resolved path is within the manifest
directory hierarchy. You can use pathlib's resolve() method and check that the
resolved path starts with the manifest directory to ensure no escape is
possible.
…lock S3776: extract per-unit loop body into _ingest_unit() helper, reducing ingest_kcp_manifest complexity from 20 to ~5 (max allowed: 15) S8572: replace logger.error() with logger.exception() in the Weaviate batch except block so the full traceback is captured automatically Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
![aurora-test-app1[bot]](https://avatars.githubusercontent.com/in/3646679?s=60&v=4){kind=small}
There was a problem hiding this comment.
Aurora Risk Review
Verdict: RISKY
This PR adds a CLI-only connector with no changes to existing code paths, which significantly limits blast radius. However, weaviate_ext.py performs a live DDL schema migration (adding 13 properties) against the shared, single-replica KnowledgeBaseChunk collection the first time the CLI is invoked — with no operator warning, no dry-run guard, and no rollback path. On the deployed Weaviate 1.27.6 instance, property additions briefly lock the collection, which can cause concurrent knowledge-base searches and ingestion calls from the running server to fail or time out. A second medium-severity issue is that the schema migration fires unconditionally inside insert_chunks_with_metadata rather than being a separate, explicit operator step, making it easy to accidentally mutate production schema during a routine ingestion run.
Findings
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | HIGH | server/connectors/kcp_connector/weaviate_ext.py:100 |
Live DDL schema migration fires automatically on first CLI invocation, locking the shared production Weaviate collection |
| 2 | MEDIUM | server/connectors/kcp_connector/weaviate_ext.py:110 |
Schema migration is not guarded by --dry-run, so a dry-run validation pass still mutates the production Weaviate schema |
| 3 | MEDIUM | server/connectors/kcp_connector/ingest.py:258 |
Broad except-and-fallback in _upload_chunks silently swallows extended-schema errors, making failures invisible in production |
Aurora reviews PRs for incident prevention. This is advisory only and does not block merge.
| """Insert chunks with KCP metadata into Weaviate. | ||
|
|
||
| Works like ``weaviate_client.insert_chunks`` but also stores the | ||
| ``kcp_*`` keys present in each chunk dict. Automatically extends |
There was a problem hiding this comment.
[HIGH] Live DDL schema migration fires automatically on first CLI invocation, locking the shared production Weaviate collection
ensure_kcp_properties() calls collection.config.add_property() on the live KnowledgeBaseChunk collection — the same single-replica Weaviate instance (weaviate:1.27.6, 50Gi PVC) that serves all knowledge-base searches and document ingestion for every user. This DDL operation is triggered automatically inside insert_chunks_with_metadata() with no operator confirmation, no --migrate flag, and no dry-run guard. In Weaviate v1.27.x, adding properties to an existing collection briefly acquires a schema lock; any concurrent search_knowledge_base() or insert_chunks() call in the running server pods will receive a 'schema locked' error or timeout during this window. Because the existing _get_weaviate_client() returns a module-level singleton _collection object, the schema mutation is immediately visible to all in-flight server requests. The fix is to make schema migration an explicit, separate operator step (e.g. python -m connectors.kcp_connector --migrate-schema) that is documented and run during a maintenance window, not silently embedded in the ingestion hot path.
| from weaviate.util import generate_uuid5 | ||
|
|
||
| # Ensure schema has KCP properties (idempotent) | ||
| try: |
There was a problem hiding this comment.
[MEDIUM] Schema migration is not guarded by --dry-run, so a dry-run validation pass still mutates the production Weaviate schema
ingest.py passes dry_run=True to skip Weaviate writes, but the dry_run check in _ingest_unit() (line 155 of ingest.py) returns before calling _upload_chunks(), which means ensure_kcp_properties() is never reached in dry-run mode — this part is actually safe. However, the risk is the inverse: there is no --migrate-schema gate, so any operator who runs the CLI without --dry-run will trigger the schema DDL on the first unit, even if they intended only a small test ingestion. The schema change is irreversible (Weaviate does not support dropping properties without recreating the collection), so a mistaken run against production permanently alters the shared schema. Operators should be warned prominently in the CLI help text, and the migration should require an explicit flag.
| chunk["kcp_not_for"] = unit.not_for | ||
| chunk["kcp_scope"] = unit.scope | ||
| chunk["kcp_kind"] = unit.kind | ||
| chunk["kcp_depends_on"] = unit.depends_on |
There was a problem hiding this comment.
[MEDIUM] Broad except-and-fallback in _upload_chunks silently swallows extended-schema errors, making failures invisible in production
The except Exception fallback at line 270 catches any runtime error from insert_chunks_with_metadata (not just ImportError) and silently retries with the standard insert_chunks, logging only a WARNING. If ensure_kcp_properties() fails due to a Weaviate connection error, a schema conflict, or a permissions issue, the ingestion will appear to succeed (chunks are inserted without KCP metadata) and the operator will not know the extended schema was never applied. In a multi-tenant SaaS environment where the KB is shared, this silent degradation means KCP metadata is permanently absent from chunks that were supposed to carry it, with no alert and no way to detect the discrepancy after the fact without re-querying Weaviate.
|
Hi! Thanks for your contribution. Before we can merge this, we need you to sign our Contributor License Agreement (CLA) for legal purposes. This is a one-time requirement for external contributors — it ensures that contributions are properly licensed and that both parties are protected. I'll send the document separately. Once signed, we're good to go on this and any future PRs. |
This branch has not been deployed
2 participants
Summary
Aurora's current KB ingestion flattens all documents into unstructured Weaviate chunks, losing the structured metadata that makes operational knowledge navigable: why a runbook exists, who it's for, when it's valid, and what it depends on.
This adds a KCP connector (
server/connectors/kcp_connector/) that ingests KCP knowledge.yaml manifests into Aurora's KB, preserving that structure:What's included
manifest.py— KCP YAML parser (handles 0.6+ and 0.20+ formats), resolves unit paths, produces typed dataclassesingest.py— Orchestrator: chunks via Aurora's existingDocumentProcessor, enriches with KCP metadata, uploads to Weaviate. CLI:python -m connectors.kcp_connector --manifest knowledge.yaml --user-id <id>weaviate_ext.py— Idempotent schema migration adding 13 KCP-specific properties +insert_chunks_with_metadataDesign decisions
uuid5(project:unit_id)for stable document IDs — re-ingesting a manifest replaces rather than duplicatesheading_context— appears in existingknowledge_base_search/search_runbooksresults immediately, no search pipeline changes neededinsert_chunkswhich ignores unknown keysTest plan
kcp_versionfield)versionfield,entityblock)heading_contextincludes[KCP:unit-id] intentprefix in search resultskcp_*properties toKnowledgeBaseChunkinsert_chunkswhenweaviate_extschema migration is skippedSummary by CodeRabbit
New Features