Conversation
WalkthroughA try/except was added around 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 |
github-actions
Bot
added
the
group/backend
Merging this PR will improve performance by 38.71%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_graphql_generate_schema |
516.2 ms | 372.2 ms | +38.71% |
| ⚡ | test_query_one_model |
463.7 ms | 352.9 ms | +31.39% |
| ⚡ | test_base_schema_duplicate_CoreProposedChange |
2.1 ms | 1.7 ms | +28.37% |
| ⚡ | test_get_schema |
324 ms | 250.7 ms | +29.27% |
| ⚡ | test_schemabranch_duplicate |
7.3 ms | 5.5 ms | +31.85% |
| ⚡ | test_query_rel_many |
692.2 ms | 523.3 ms | +32.29% |
| ⚡ | test_schemabranch_process |
1,027 ms | 794.5 ms | +29.27% |
| ⚡ | test_get_menu |
245 ms | 188.9 ms | +29.67% |
| ⚡ | test_query_rel_one |
662.6 ms | 500.1 ms | +32.49% |
| ⚡ | test_nodemanager_querypeers |
1.5 ms | 1.3 ms | +21.96% |
| ⚡ | test_relationshipmanager_getpeer |
156.7 µs | 133.3 µs | +17.49% |
| ⚡ | test_load_node_to_db_node_schema |
66.3 ms | 51.6 ms | +28.46% |
Comparing sb-20022026-schema-integrity-check-allow-invalid-schemas-ifc-2259 (7a0d263) with stable (43d16b8)1
Footnotes
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/tests/component/message_bus/operations/requests/test_proposed_change.py (1)
348-385: New test covers the happy-error path well; two minor observations.1.
set_diff_summary_cacheis unreachable in this test path.
Becauseprocess()raises beforeget_diff_summary_cacheis called, lines 372–374 populate cache data that is never consumed. The call is harmless but slightly misleading — readers may believe the diff summary influences the outcome here.2. Assertions on internal error-message substrings are brittle (lines 384–385).
assert conflicts[0]["kind"] == "TestPerson" assert "unique" in conflicts[0]["value"].lower()Both assertions depend on
process()raisingValueErrorwith the exact format"TestPerson: ... unique ...". Ifprocess()ever reformats its error messages (e.g., different casing, different wording), these fail silently in a misleading way. Consider asserting thepathfield (which is deterministic:f"schema/{kind}") or at least add an inline comment documenting the expected format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/component/message_bus/operations/requests/test_proposed_change.py` around lines 348 - 385, The test test_schema_integrity_process_validation_error currently populates the diff cache via set_diff_summary_cache which is never used because process() raises before get_diff_summary_cache is called; remove the set_diff_summary_cache call (or move it to after run_proposed_change_schema_integrity_check if you intended to assert cached state) to avoid misleading readers. Also make the failure assertions less brittle: instead of checking substring text in conflicts[0]["value"], assert deterministic fields like conflicts[0]["path"] == f"schema/{conflicts[0]['kind']}" or assert conflicts[0]["kind"] == "TestPerson" plus a guarded check (e.g., lower-cased contains) with an inline comment documenting the expected error format; update assertions around registry.manager.query and checks[0].conflicts accordingly.backend/infrahub/proposed_change/tasks.py (1)
434-463: No log emission whenValueErroris caught — observability gap.Every other flow in this file (
merge_proposed_change,run_proposed_change_user_tests, etc.) logs at the start or on error paths. The new except branch silently records the conflict and returns without any structured log entry, making Prefect run traces and server logs silent on these failures.♻️ Suggested logging addition
+ log = get_run_logger() try: candidate_schema.duplicate().process() except ValueError as exc: error_msg = str(exc) + log.warning(f"Schema process validation failed during integrity check: {error_msg}") parts = error_msg.split(":", 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/proposed_change/tasks.py` around lines 434 - 463, The except block catching ValueError from candidate_schema.duplicate().process() currently records a SchemaConflict but emits no log; add a structured log entry before contacting the DB to improve observability: call the module's logger (e.g. process_logger.error or logger.exception) inside the except ValueError as exc block with context including the error/exception, error_msg, kind, model.proposed_change and model.source_branch (use logger.exception to capture stack trace), then proceed to the existing ObjectConflictValidatorRecorder.record_conflicts and return. Ensure the log call is placed immediately after building error_msg/kind (before await get_database()) so failures are visible in run traces.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/infrahub/proposed_change/tasks.py`:
- Around line 434-463: The except block catching ValueError from
candidate_schema.duplicate().process() currently records a SchemaConflict but
emits no log; add a structured log entry before contacting the DB to improve
observability: call the module's logger (e.g. process_logger.error or
logger.exception) inside the except ValueError as exc block with context
including the error/exception, error_msg, kind, model.proposed_change and
model.source_branch (use logger.exception to capture stack trace), then proceed
to the existing ObjectConflictValidatorRecorder.record_conflicts and return.
Ensure the log call is placed immediately after building error_msg/kind (before
await get_database()) so failures are visible in run traces.
In
`@backend/tests/component/message_bus/operations/requests/test_proposed_change.py`:
- Around line 348-385: The test test_schema_integrity_process_validation_error
currently populates the diff cache via set_diff_summary_cache which is never
used because process() raises before get_diff_summary_cache is called; remove
the set_diff_summary_cache call (or move it to after
run_proposed_change_schema_integrity_check if you intended to assert cached
state) to avoid misleading readers. Also make the failure assertions less
brittle: instead of checking substring text in conflicts[0]["value"], assert
deterministic fields like conflicts[0]["path"] ==
f"schema/{conflicts[0]['kind']}" or assert conflicts[0]["kind"] == "TestPerson"
plus a guarded check (e.g., lower-cased contains) with an inline comment
documenting the expected error format; update assertions around
registry.manager.query and checks[0].conflicts accordingly.
| parts = error_msg.split(":", 1) | ||
| kind = parts[0].strip() if len(parts) > 1 and parts[0].strip() else "Unknown" | ||
| schema_path = f"schema/{kind}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Step 1: Find SchemaBranch class and process() method
find . -type f -name "*.py" | xargs grep -l "class SchemaBranch" | head -5Repository: opsmill/infrahub
Length of output: 272
🏁 Script executed:
# Step 2: Search for process() method in schema-related files
rg -n "def process\(" --type=py | grep -i schema | head -20Repository: opsmill/infrahub
Length of output: 170
🏁 Script executed:
# Step 3: Find all ValueError raises in the codebase to understand patterns
rg -n "raise ValueError" --type=py -A 1 | grep -A 1 "schema\|kind\|Schema" | head -40Repository: opsmill/infrahub
Length of output: 2750
🏁 Script executed:
# Read the SchemaBranch.process() method
cat -n backend/infrahub/core/schema/schema_branch.py | sed -n '620,700p'Repository: opsmill/infrahub
Length of output: 3868
🏁 Script executed:
# Search for all ValueError raises within SchemaBranch.process() and methods it calls
rg -n "raise ValueError" backend/infrahub/core/schema/schema_branch.py -A 1 -B 2Repository: opsmill/infrahub
Length of output: 15925
🏁 Script executed:
# Also check for any validators or schema processing that might raise ValueError
rg -n "ValueError" backend/infrahub/core/schema/schema_branch.py -B 2 -A 1Repository: opsmill/infrahub
Length of output: 16572
🏁 Script executed:
# Read the context around lines 438-440 in tasks.py
cat -n backend/infrahub/proposed_change/tasks.py | sed -n '430,450p'Repository: opsmill/infrahub
Length of output: 1101
🏁 Script executed:
# Search for tests related to run_proposed_change_schema_integrity_check
fd -t f "test.*\.py" | xargs grep -l "run_proposed_change_schema_integrity_check" | head -5Repository: opsmill/infrahub
Length of output: 139
🏁 Script executed:
# Look for tests of error message parsing
rg -n "TestPerson.*:" backend/tests --type=py -B 2 -A 2 | head -50Repository: opsmill/infrahub
Length of output: 4387
🏁 Script executed:
# Find tests specifically testing schema integrity check or error message parsing
rg -n "schema_integrity\|SchemaConflict" backend/tests --type=py -B 2 -A 5 | head -80Repository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Check ValueError messages in SchemaBranch.process and related validation methods
# Focus on finding messages that start with kind names
rg -n 'raise ValueError.*\{.*\.kind\}.*:' backend/infrahub/core/schema/schema_branch.py -A 0Repository: opsmill/infrahub
Length of output: 394
🏁 Script executed:
# Count unique ValueError patterns to understand the format
python3 << 'EOF'
import re
import subprocess
result = subprocess.run(
["rg", "-n", "raise ValueError", "backend/infrahub/core/schema/schema_branch.py", "-A", "0"],
capture_output=True,
text=True
)
messages = []
for line in result.stdout.split('\n'):
if 'raise ValueError' in line:
# Extract the message template
match = re.search(r'raise ValueError\((.*)\)', line)
if match:
msg_template = match.group(1)
messages.append(msg_template.strip())
# Analyze patterns
colon_separated = []
other = []
for msg in messages[:30]: # First 30
if '{' in msg and ':' in msg and 'f"' in msg:
colon_separated.append(msg)
else:
other.append(msg)
print("Messages with f-string containing ':' separator:")
for msg in colon_separated[:10]:
print(f" {msg[:100]}")
print(f"\nTotal messages with colon-separated format: {len(colon_separated)}")
EOFRepository: opsmill/infrahub
Length of output: 779
🏁 Script executed:
# Verify the actual error message patterns more comprehensively
# Look at all ValueError raises that include node.kind
rg -n 'raise ValueError\(f.*\{.*\.kind\}.*:' backend/infrahub/core/schema/schema_branch.py | wc -lRepository: opsmill/infrahub
Length of output: 61
🏁 Script executed:
# Get a sample of these messages to verify the consistent format
rg -n 'raise ValueError\(f.*\{.*\.kind\}.*:' backend/infrahub/core/schema/schema_branch.py | head -20Repository: opsmill/infrahub
Length of output: 394
🏁 Script executed:
# Check what error_header is and understand its format
rg -n "error_header\s*=" backend/infrahub/core/schema/schema_branch.py -B 2 -A 2 | head -50Repository: opsmill/infrahub
Length of output: 263
🏁 Script executed:
# Look for the complete context of how ValueError is used in schema validation
# Find all places where error_header is set
rg -n "error_header" backend/infrahub/core/schema/schema_branch.py | head -30Repository: opsmill/infrahub
Length of output: 1160
🏁 Script executed:
# Check if schema kind names (NodeSchema, GenericSchema, etc.) can contain spaces
# Look at how kinds are defined and validated
rg -n "\.kind\s*=" backend/infrahub/core/schema/ --type=py | head -20Repository: opsmill/infrahub
Length of output: 2678
🏁 Script executed:
# Search for the SchemaConflict model to understand what fields are expected
rg -n "class SchemaConflict\|id.*field\|path.*field" backend/infrahub --type=py -A 3 | head -50Repository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Check if there are any actual test cases that validate this error parsing
rg -n "run_proposed_change_schema_integrity_check\|schema/\{" backend/tests --type=py -B 3 -A 10 | head -80Repository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Verify what schematypes/kinds look like in practice
rg -n "InfrahubKind\." backend/infrahub/core/schema/ --type=py | grep -E "IPPREFIX|IPADDRESS|ORGANIZATION" | head -10Repository: opsmill/infrahub
Length of output: 1253
🏁 Script executed:
# Check if kind names contain spaces or special characters
rg -n 'self.kind\s*=' backend/infrahub/core/models.py -B 2 -A 2 | head -40Repository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Look at SchemaConflict to understand what kind of values are actually stored
fd -type f "*.py" -path "*models*" | xargs grep -l "SchemaConflict\|class.*Conflict" | head -5Repository: opsmill/infrahub
Length of output: 230
🏁 Script executed:
# Search for SchemaConflict class definition
rg -n "class SchemaConflict" backend/infrahub --type=py -A 20 | head -50Repository: opsmill/infrahub
Length of output: 1384
🏁 Script executed:
# Also check for where schema_path is used as id/name
rg -n "schema_path\s*=" backend/infrahub/proposed_change/tasks.py -B 5 -A 5Repository: opsmill/infrahub
Length of output: 635
🏁 Script executed:
# Check actual test cases for proposed changes with schema errors
rg -n "schema.*conflict\|SchemaCheck" backend/tests --type=py -i -B 2 -A 5 | head -60Repository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Check if there are actual test cases exercising the error path
rg -n "record_conflicts\|SchemaConflict" backend/tests --type=py -B 3 -A 10 | head -100Repository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Look for actual ValueError messages that might contain dots or complex formats
rg -n 'raise ValueError.*error_header.*\{' backend/infrahub/core/schema/schema_branch.py -A 0 | head -20Repository: opsmill/infrahub
Length of output: 232
🏁 Script executed:
# Examine the ObjectConflictValidatorRecorder to understand what schema_path is used for
rg -n "class ObjectConflictValidatorRecorder\|schema_path" backend/infrahub --type=py -B 2 -A 5 | head -80Repository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Look at how error_header is used in ValueError raises to see actual message formats
rg -n "error_header" backend/infrahub/core/schema/schema_branch.py | grep "raise ValueError" -A 1 -B 1Repository: opsmill/infrahub
Length of output: 755
🏁 Script executed:
# Search for actual test coverage of error message parsing in proposed_change
rg -n "error_msg\|kind\s*=" backend/tests --type=py | grep -A 5 -B 5 "proposed_change" | head -50Repository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Inspect how kind is validated - check if it needs to be a valid schema kind
rg -n "valid.*kind\|InfrahubKind\." backend/infrahub/core/diff --type=py -B 2 -A 2 | head -50Repository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Final check: look at all error messages that would actually be raised
# Count patterns to ensure they all match the "kind: detail" format
python3 << 'EOF'
import subprocess
import re
# Get all ValueError raises
result = subprocess.run(
["rg", "-n", "raise ValueError", "backend/infrahub/core/schema/schema_branch.py", "-A", "0"],
capture_output=True,
text=True
)
# Parse patterns
patterns = []
for line in result.stdout.split('\n'):
if 'raise ValueError' in line:
# Extract the message content
match = re.search(r'raise ValueError\((.*)\)', line)
if match:
msg = match.group(1).strip()
# Check if it starts with a variable or field
if msg.startswith('f"'):
# Check if the first part is {something} or contains {kind} or {error_header}
if '{node.kind}' in msg or '{error_header}' in msg:
patterns.append(('starts_with_kind', msg[:80]))
else:
patterns.append(('other', msg[:80]))
elif msg.startswith('"'):
patterns.append(('literal', msg[:80]))
print(f"Total ValueError raises: {len(patterns)}")
print(f"\nStarts with node.kind or error_header: {len([p for p in patterns if p[0] == 'starts_with_kind'])}")
print(f"Other f-strings: {len([p for p in patterns if p[0] == 'other'])}")
print(f"Literal strings: {len([p for p in patterns if p[0] == 'literal'])}")
print("\nSample of 'starts_with_kind' messages:")
for _, msg in [p for p in patterns if p[0] == 'starts_with_kind'][:5]:
print(f" {msg}")
EOFRepository: opsmill/infrahub
Length of output: 527
🏁 Script executed:
# Verify if error_header with dots (e.g., "Kind.attribute") is a valid case
rg -n 'error_header\s*\+=' backend/infrahub/core/schema/schema_branch.py -B 2 -A 2Repository: opsmill/infrahub
Length of output: 235
🏁 Script executed:
# Check if the resulting kind value (which could include dots) would be used as a valid schema kind
# Look at how the schema_path is used downstream
rg -n "schema_path\s*=" backend/infrahub/proposed_change/tasks.py -A 15 | head -40Repository: opsmill/infrahub
Length of output: 877
🏁 Script executed:
# Verify what valid kind names look like by checking actual node schema definitions
rg -n "\"kind\":\s*\"" backend/tests --type=py | head -20Repository: opsmill/infrahub
Length of output: 2496
🏁 Script executed:
# Confirm the actual error message format with dots
rg -n "error_header.*=" backend/infrahub/core/schema/schema_branch.py -A 3 | head -20Repository: opsmill/infrahub
Length of output: 295
🏁 Script executed:
# Check if there are actual test cases that exercise the dot scenario
rg -n "element_name" backend/infrahub/core/schema/schema_branch.py | wc -lRepository: opsmill/infrahub
Length of output: 62
🏁 Script executed:
# Find where validate_schema_attribute_path is called to see if element_name is typically set
rg -n "validate_schema_attribute_path" backend/infrahub/core/schema/schema_branch.py | head -10Repository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Confirm element_name is actually used in practice
rg -n "validate_schema_attribute_path.*element_name" backend/infrahub/core/schema/schema_branch.py -B 2 -A 2Repository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Check all calls to validate_schema_attribute_path to see what element_name values are passed
rg -n "validate_schema_attribute_path\(" backend/infrahub/core/schema/schema_branch.py -A 2 | head -40Repository: opsmill/infrahub
Length of output: 42
🏁 Script executed:
# Look at the function signature and calls more carefully
ast-grep --pattern 'validate_schema_attribute_path($_,$_,$_,element_name: $_)'Repository: opsmill/infrahub
Length of output: 42
Fragile error-message parsing — document or guard the assumed format.
The kind extraction assumes ValueError messages from process() follow the format "SchemaKind: detail...", but this assumption is incomplete. The error header is constructed as f"{node_schema.kind}" optionally followed by f".{element_name}" when validating attribute paths (line 760 in schema_branch.py), producing messages like "TestPerson.owner: cannot use...".
The current parsing will incorrectly extract kind = "TestPerson.owner" (including the dot), which is not a valid schema kind and silently pollutes the recorded conflict.
💡 Suggested defensive extraction
- parts = error_msg.split(":", 1)
- kind = parts[0].strip() if len(parts) > 1 and parts[0].strip() else "Unknown"
+ # process() raises ValueError with format "SchemaKind[.element]: detail" when it can
+ # pinpoint the offending schema node; fall back to "Unknown" otherwise.
+ parts = error_msg.split(":", 1)
+ raw_kind = parts[0].strip() if len(parts) > 1 else ""
+ # A valid schema kind is a simple identifier with no spaces or dots.
+ kind = raw_kind if raw_kind and " " not in raw_kind and "." not in raw_kind else "Unknown"📝 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.
| parts = error_msg.split(":", 1) | |
| kind = parts[0].strip() if len(parts) > 1 and parts[0].strip() else "Unknown" | |
| schema_path = f"schema/{kind}" | |
| # process() raises ValueError with format "SchemaKind[.element]: detail" when it can | |
| # pinpoint the offending schema node; fall back to "Unknown" otherwise. | |
| parts = error_msg.split(":", 1) | |
| raw_kind = parts[0].strip() if len(parts) > 1 else "" | |
| # A valid schema kind is a simple identifier with no spaces or dots. | |
| kind = raw_kind if raw_kind and " " not in raw_kind and "." not in raw_kind else "Unknown" | |
| schema_path = f"schema/{kind}" |
|
It would be nice if we could get this to go into the |
solababs
force-pushed
the
sb-20022026-schema-integrity-check-allow-invalid-schemas-ifc-2259
branch
from
03ae1be to
c744775
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/infrahub/proposed_change/tasks.py (1)
438-459: Consider extracting schema-conflict persistence into a shared helper.The recorder setup here is duplicated in the later schema-conflict write path. A small helper would reduce drift risk and simplify future changes.
♻️ Suggested refactor sketch
- database = await get_database() - async with database.start_transaction() as db: - object_conflict_validator_recorder = ObjectConflictValidatorRecorder( - db=db, - validator_kind=InfrahubKind.SCHEMAVALIDATOR, - validator_label="Schema Integrity", - check_schema_kind=InfrahubKind.SCHEMACHECK, - ) - await object_conflict_validator_recorder.record_conflicts( - proposed_change_id=model.proposed_change, - conflicts=[ - SchemaConflict( - name=schema_path, - type="schema.process.validation", - kind=kind, - id=schema_path, - path=schema_path, - value=error_msg, - branch=model.source_branch, - ) - ], - ) + await _record_schema_integrity_conflicts( + proposed_change_id=model.proposed_change, + conflicts=[ + SchemaConflict( + name=schema_path, + type="schema.process.validation", + kind=kind, + id=schema_path, + path=schema_path, + value=error_msg, + branch=model.source_branch, + ) + ], + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/proposed_change/tasks.py` around lines 438 - 459, Extract the duplicated recorder setup and conflict write into a shared helper (e.g., a function like persist_schema_conflict or create_and_record_schema_conflict) that accepts the transaction/db, proposed_change_id, schema_path, kind, error_msg and branch; move the ObjectConflictValidatorRecorder construction (using InfrahubKind.SCHEMAVALIDATOR, validator_label="Schema Integrity", check_schema_kind=InfrahubKind.SCHEMACHECK) and the call to record_conflicts into that helper, then replace the duplicated blocks with calls to this helper to centralize persistence logic and reduce drift risk.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/infrahub/proposed_change/tasks.py`:
- Around line 438-459: Extract the duplicated recorder setup and conflict write
into a shared helper (e.g., a function like persist_schema_conflict or
create_and_record_schema_conflict) that accepts the transaction/db,
proposed_change_id, schema_path, kind, error_msg and branch; move the
ObjectConflictValidatorRecorder construction (using
InfrahubKind.SCHEMAVALIDATOR, validator_label="Schema Integrity",
check_schema_kind=InfrahubKind.SCHEMACHECK) and the call to record_conflicts
into that helper, then replace the duplicated blocks with calls to this helper
to centralize persistence logic and reduce drift risk.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e5144ea-10bc-41fa-8f12-a23884448a55
📒 Files selected for processing (2)
backend/infrahub/proposed_change/tasks.pybackend/tests/component/message_bus/operations/requests/test_proposed_change.py
ogenstad
left a comment
ogenstad
left a comment
There was a problem hiding this comment.
Would be good with a news fragment for this that targeted the bug.
|
|
||
| try: | ||
| candidate_schema.duplicate().process() | ||
| except ValueError as exc: |
There was a problem hiding this comment.
I'm not sure if this is enough as we are raising ValidationError in a few places and also SchemaNotFoundError. Not completely sure if it would show up for these type of errors. But I think it's better to be more verbose and include those too. At some point it might be better if we only raised one specific error from this related to schema processing error or such. But until then we should include the other exceptions as well.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/infrahub/proposed_change/tasks.py (1)
434-437:⚠️ Potential issue | 🟡 MinorSplit schema kind from schema element path before recording the conflict.
Line 436 currently copies the full error header into
kind.SchemaBranch.process()can emit headers likeTestPerson.owner: ..., so the recorded conflict ends up withkind="TestPerson.owner"instead of the actual schema kind. That still blocks the change, but it makes the conflict metadata inconsistent.Suggested fix
error_msg = str(exc) parts = error_msg.split(":", 1) - kind = parts[0].strip() if len(parts) > 1 and parts[0].strip() else "Unknown" - schema_path = f"schema/{kind}" + raw_schema_path = parts[0].strip() if len(parts) > 1 and parts[0].strip() else "Unknown" + kind = raw_schema_path.split(".", 1)[0] + schema_path = f"schema/{raw_schema_path}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/proposed_change/tasks.py` around lines 434 - 437, The code that extracts the schema kind from the error header (in the block assigning error_msg, parts, kind, schema_path) can capture element paths like "TestPerson.owner" instead of the actual schema kind; change the extraction so after getting header = parts[0].strip() you further split header on the first "." and take the leftmost token as the kind (fallback to "Unknown" if header is empty), then build schema_path = f"schema/{kind}"; this ensures SchemaBranch.process() emitted headers like "TestPerson.owner: ..." record kind="TestPerson" not "TestPerson.owner".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/infrahub/proposed_change/tasks.py`:
- Around line 434-437: The code that extracts the schema kind from the error
header (in the block assigning error_msg, parts, kind, schema_path) can capture
element paths like "TestPerson.owner" instead of the actual schema kind; change
the extraction so after getting header = parts[0].strip() you further split
header on the first "." and take the leftmost token as the kind (fallback to
"Unknown" if header is empty), then build schema_path = f"schema/{kind}"; this
ensures SchemaBranch.process() emitted headers like "TestPerson.owner: ..."
record kind="TestPerson" not "TestPerson.owner".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f90b3b58-729e-4e23-b42c-f2f557925a9c
📒 Files selected for processing (2)
backend/infrahub/proposed_change/tasks.pychangelog/8355.added.md
ogenstad
left a comment
ogenstad
left a comment
There was a problem hiding this comment.
LGTM, just need to resolve some merge conflicts.
…nvalid-schemas-ifc-2259
…integrity-check-allow-invalid-schemas-ifc-2259' into sb-20022026-schema-integrity-check-allow-invalid-schemas-ifc-2259
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/infrahub/proposed_change/tasks.py (1)
437-440:⚠️ Potential issue | 🟡 MinorKind extraction is brittle for the newly caught exception types.
Line 438-Line 440 assume
"kind: detail"formatting, butValidationErrorandSchemaNotFoundErrorstring formats don’t reliably match that, so conflicts can be recorded asschema/Unknownor with non-kind headers.Suggested hardening
- error_msg = str(exc) - parts = error_msg.split(":", 1) - kind = parts[0].strip() if len(parts) > 1 and parts[0].strip() else "Unknown" + error_msg = str(exc).strip() + kind = "Unknown" + if isinstance(exc, SchemaNotFoundError): + kind = exc.identifier + elif isinstance(exc, ValueError): + header = error_msg.split(":", 1)[0].strip() + if header: + candidate_kind = header.split(".", 1)[0] + if " " not in candidate_kind: + kind = candidate_kind schema_path = f"schema/{kind}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/proposed_change/tasks.py` around lines 437 - 440, The current kind extraction from error_msg using split(...) is brittle for exceptions like ValidationError and SchemaNotFoundError; replace this fragile string parsing in the except block by deriving kind from the exception type or explicit attributes instead: prefer exc.__class__.__name__ (or a small mapping from ValidationError/SchemaNotFoundError to desired canonical kind strings) and fallback to "Unknown" only if that fails, then build schema_path from that computed kind (use the existing variables error_msg, kind, schema_path) so conflicts are recorded with a stable, meaningful kind instead of relying on error_msg formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/infrahub/proposed_change/tasks.py`:
- Around line 437-440: The current kind extraction from error_msg using
split(...) is brittle for exceptions like ValidationError and
SchemaNotFoundError; replace this fragile string parsing in the except block by
deriving kind from the exception type or explicit attributes instead: prefer
exc.__class__.__name__ (or a small mapping from
ValidationError/SchemaNotFoundError to desired canonical kind strings) and
fallback to "Unknown" only if that fails, then build schema_path from that
computed kind (use the existing variables error_msg, kind, schema_path) so
conflicts are recorded with a stable, meaningful kind instead of relying on
error_msg formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d7be553-8492-4279-87aa-86b1827e6893
📒 Files selected for processing (1)
backend/infrahub/proposed_change/tasks.py
solababs
deleted the
sb-20022026-schema-integrity-check-allow-invalid-schemas-ifc-2259
branch
3 participants
Why
When a proposed change introduces a schema modification that makes the merged schema structurally invalid (e.g., a relationship added with the same name as an existing attribute), run_proposed_change_schema_integrity_check was silently passing the invalid schema through to the schema_path_validate constraint-validation tasks. Those tasks would fail or retry unexpectedly — producing Prefect task_run_recorder UniqueViolationError noise — instead of surfacing a clear failure on the proposed change.
Goal: Catch merged-schema structural failures early via SchemaBranch.process() and record them as a SchemaConflict, so the proposed change is blocked with a visible error rather than silently misbehaving.
Non-goals: This does not change data-level constraint validation (schema_path_validate tasks) or fix Prefect internals.
Closes #8355
What changed
How to review
Focus on backend/infrahub/proposed_change/tasks.py lines 434–462 — the new try/except ValueError block.
How to test
The new test test_schema_integrity_process_validation_error creates a source branch where TestPerson has a relationship named "name" (colliding with the existing "name" attribute), runs the integrity check, and asserts exactly one SchemaConflict with type="schema.process.validation" is recorded.
Impact & rollout
Backward compatibility:
No breaking changes. The new code path only activates when process() raises, which was previously unhandled (tasks ran silently against invalid schemas).
Performance:
One extra duplicate() + process() call per schema integrity check — negligible.
Deployment notes:
Safe to deploy.
Checklist
Summary by CodeRabbit
Bug Fixes
Tests
Documentation