feat: add new dbt artifacts support #89
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 9358e35 in 26 seconds. Click for details.
- Reviewed
770lines of code in19files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/vendor/dbt_artifacts_parser/parser.py:254
- Draft comment:
The error message in parse_run_results is misleading; it says 'Not a manifest.json' when it should indicate run-results.json. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. src/vendor/dbt_artifacts_parser/parser.py:324
- Draft comment:
The error message in parse_sources is incorrect; it raises 'Not a manifest.json' instead of 'Not a sources.json'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_eK1ZtMY6XRJXBtYu
You can customize 
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
Comment on lines
+112
to
+114
| run_results: RunResults = parse_run_results(run_results_dict) | ||
| except ValueError as e: | ||
| raise AltimateInvalidManifestError(f"Invalid run results file: {run_results_path}. Error: {e}") from e |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 6d2034f in 45 seconds. Click for details.
- Reviewed
366lines of code in12files - Skipped
0files when reviewing. - Skipped posting
12draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/clients/altimate/constants.py:9
- Draft comment:
Removed 'semantic_manifest' from SUPPORTED_ARTIFACT_TYPES. Ensure this breaking change is documented. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/datapilot/core/platforms/dbt/cli/cli.py:21
- Draft comment:
Removed import of load_semantic_manifest. All references to semantic_manifest are cleaned up. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. src/datapilot/core/platforms/dbt/cli/cli.py:160
- Draft comment:
Removed '--semantic-manifest-path' option and related validation from the onboard command. Confirm this removal aligns with overall artifact support changes. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. src/datapilot/core/platforms/dbt/schemas/semantic_manifest.py:1
- Draft comment:
Deleted the semantic_manifest schema file. Ensure downstream consumers are updated, as this removal is breaking. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. src/datapilot/core/platforms/dbt/utils.py:132
- Draft comment:
Removed the load_semantic_manifest() function. This cleanup is consistent with the removal in CLI and schema. Confirm no external dependency remains. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. src/vendor/dbt_artifacts_parser/parser.py:346
- Draft comment:
Removed parse_semantic_manifest function. Verify that no external code depends on this parser. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. src/vendor/dbt_artifacts_parser/parsers/semantic_manifest/__init__.py:1
- Draft comment:
Removed the semantic_manifest init file from the parsers. Ensure that no module import errors occur post-deletion. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. src/vendor/dbt_artifacts_parser/parsers/semantic_manifest/semantic_manifest_v1.py:1
- Draft comment:
Removed the semantic_manifest_v1 parser implementation. Confirm that full semantic manifest support is no longer required. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. src/vendor/dbt_artifacts_parser/parsers/version_map.py:78
- Draft comment:
Removed the SEMANTIC_MANIFEST_V1 mapping from ArtifactTypes. This is consistent with the removal of semantic manifest support. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
10. tests/clients/altimate/test_utils.py:8
- Draft comment:
Updated expected artifact types in tests to reflect removal of semantic_manifest. Test coverage seems adjusted accordingly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
11. tests/core/platform/dbt/test_artifact_loaders.py:35
- Draft comment:
Removed tests for load_semantic_manifest. Ensure that removal does not affect backward compatibility if needed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
12. tests/core/platform/dbt/test_cli.py:125
- Draft comment:
Adjusted CLI help test: '--semantic-manifest-path' is no longer expected. Verify that help output remains clear for supported options. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_P90yRDDddxLPfvJA
You can customize
Collaborator
|
@gaurpulkit : fix error message to be more generic, def load_json(file_path: str) -> Dict: |
mdesmet
reviewed
Dec 29, 2025
Collaborator
mdesmet
left a comment
mdesmet
left a comment
There was a problem hiding this comment.
Please readd semantic manifest, we can already collect it in backend
| "manifest", | ||
| "catalog", | ||
| "run_results", | ||
| "sources", |
Collaborator
There was a problem hiding this comment.
also add semantic manifest, we can already collect it, we should not validate it.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 9a34999 in 31 seconds. Click for details.
- Reviewed
69lines of code in4files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/clients/altimate/constants.py:9
- Draft comment:
Added 'semantic_manifest' to SUPPORTED_ARTIFACT_TYPES. Ensure documentation is updated if needed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/datapilot/core/platforms/dbt/cli/cli.py:162
- Draft comment:
New '--semantic-manifest-path' option added. The option is consistent with the other artifact paths. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. src/datapilot/core/platforms/dbt/cli/cli.py:275
- Draft comment:
Handles onboarding for 'semantic_manifest'. Consider adding file validation (if applicable) similar to catalog, run_results, and sources for consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. src/datapilot/core/platforms/dbt/cli/cli.py:247
- Draft comment:
The repeated logic for onboarding different artifacts (catalog, run_results, sources, semantic_manifest) might be refactored into a shared function or loop to reduce duplication. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. tests/clients/altimate/test_utils.py:8
- Draft comment:
Test updated: expected artifact types now include 'semantic_manifest'. This update is consistent with the constants. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. tests/core/platform/dbt/test_cli.py:129
- Draft comment:
The onboarding help test now asserts '--semantic-manifest-path' is shown. This verifies that the new option is exposed in CLI help. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_YOkMtfNZAGhnrZNh
You can customize
Comment on lines
+275
to
+283
| if semantic_manifest_path: | ||
| response = onboard_file( | ||
| token, instance_name, dbt_integration_id, dbt_integration_environment, "semantic_manifest", semantic_manifest_path, backend_url | ||
| ) | ||
| if response["ok"]: | ||
| click.echo("Semantic manifest onboarded successfully!") | ||
| artifacts_uploaded.append("semantic_manifest") | ||
| else: | ||
| click.echo(f"{response['message']}") |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ba68036 in 1 minute and 36 seconds. Click for details.
- Reviewed
906lines of code in14files - Skipped
0files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/clients/altimate/utils.py:107
- Draft comment:
Nice use of an f-string here. Ensure that 'file_type' is always provided to avoid a blank error message. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. src/datapilot/core/platforms/dbt/schemas/catalog.py:29
- Draft comment:
Avoid using double Optional (Optional[Optional[str]]); use Optional[str] instead. - Reason this comment was not posted:
Comment was on unchanged code.
3. src/vendor/dbt_artifacts_parser/parsers/run_results/run_results_v1.py:18
- Draft comment:
Switching 'extra' from 'forbid' to 'allow' improves artifact compatibility but relaxes validation. Confirm this trade-off. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. src/vendor/dbt_artifacts_parser/parsers/sources/sources_v1.py:28
- Draft comment:
Changing extra validation to 'allow' permits unexpected fields; ensure this is intended for new artifact versions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. Multiple Files (e.g., manifest.py, nodes.py, run_results_v* and sources_v*):1
- Draft comment:
Consistently adding model_config = ConfigDict(extra="allow") aids forward compatibility but reduces strict type validation. Verify if this is acceptable. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. src/datapilot/core/platforms/dbt/schemas/catalog.py:21
- Draft comment:
Typographical note: The type hint forinvocation_idis written asOptional[Optional[str]], which appears to be a redundant nesting ofOptional. Consider changing it toOptional[str]. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. src/datapilot/core/platforms/dbt/schemas/catalog.py:70
- Draft comment:
Typographical note: It looks like the type annotation for ‘errors’ is using a double Optional (Optional[Optional[List[str]]]). This may be unintentional; consider using Optional[List[str]] for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. src/datapilot/core/platforms/dbt/schemas/manifest.py:70
- Draft comment:
Typographical Issue: The type hint for 'data_type' in AltimateManifestColumnInfo is declared as Optional[Optional[str]]. This appears to be a duplicate of Optional; please correct it to Optional[str] if that was the intent. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_9RFp8ujlyDWuOpAk
You can customize
mdesmet
reviewed
Jan 6, 2026
Collaborator
mdesmet
left a comment
mdesmet
left a comment
There was a problem hiding this comment.
Remove model extra allow everywhere except in vendor
|
|
||
|
|
||
| class InsightResult(BaseModel): | ||
| model_config = ConfigDict(extra="allow") |
|
|
||
|
|
||
| class InsightResponse(BaseModel): | ||
| model_config = ConfigDict(extra="allow") |
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive support for new dbt artifacts beyond the existing manifest and catalog support. The changes enable onboarding and validation of run_results, sources, and semantic_manifest artifacts, along with necessary schema definitions and test coverage.
- Introduces loader functions for
run_resultsandsourcesartifacts with validation - Updates the CLI
onboardcommand to accept and process multiple artifact types - Adds schema wrappers and updates vendor parsers to allow extra fields for forward compatibility
Reviewed changes
Copilot reviewed 15 out of 26 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/datapilot/clients/altimate/constants.py |
Defines the set of supported artifact types for onboarding |
src/datapilot/clients/altimate/utils.py |
Adds validation for supported artifact types and improves documentation |
src/datapilot/core/platforms/dbt/cli/cli.py |
Extends onboard command with new artifact options and validation workflow |
src/datapilot/core/platforms/dbt/utils.py |
Implements load_run_results() and load_sources() loader functions |
src/datapilot/core/platforms/dbt/schemas/run_results.py |
New schema wrappers for run_results artifacts (v1-v6) |
src/datapilot/core/platforms/dbt/schemas/sources.py |
New schema wrappers for sources artifacts (v1-v3) |
src/datapilot/core/platforms/dbt/schemas/manifest.py |
Adds extra="allow" config to all models for flexibility |
src/datapilot/core/platforms/dbt/schemas/catalog.py |
Adds extra="allow" config to all catalog models |
src/datapilot/schemas/nodes.py |
Adds extra="allow" config to ModelNode and SourceNode |
src/datapilot/core/insights/schema.py |
Adds extra="allow" config to insight models |
src/vendor/dbt_artifacts_parser/parsers/run_results/*.py |
Updates all run_results parsers to allow extra fields |
src/vendor/dbt_artifacts_parser/parsers/sources/*.py |
Updates all sources parsers to allow extra fields |
tests/clients/altimate/test_utils.py |
Tests for artifact type validation in onboarding |
tests/core/platform/dbt/test_artifact_loaders.py |
Tests for new loader functions with valid and invalid inputs |
tests/core/platform/dbt/test_cli.py |
Tests CLI help output includes all artifact options |
tests/data/run_results_v6.json |
Test fixture for run_results artifact |
tests/data/sources_v3.json |
Test fixture for sources artifact |
tests/clients/altimate/__init__.py |
Empty init file for test module |
tests/clients/__init__.py |
Empty init file for test module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+275
to
+283
| if semantic_manifest_path: | ||
| response = onboard_file( | ||
| token, instance_name, dbt_integration_id, dbt_integration_environment, "semantic_manifest", semantic_manifest_path, backend_url | ||
| ) | ||
| if response["ok"]: | ||
| click.echo("Semantic manifest onboarded successfully!") | ||
| artifacts_uploaded.append("semantic_manifest") | ||
| else: | ||
| click.echo(f"{response['message']}") |
There was a problem hiding this comment.
The semantic_manifest_path artifact is uploaded without validation, unlike the other artifacts (manifest, catalog, run_results, sources). This inconsistency could lead to invalid files being uploaded. Consider adding validation for semantic_manifest before uploading, similar to lines 217-236.
Contributor
Author
There was a problem hiding this comment.
This is for semantic_manifest only. dbt-artifacts-library doesn't have a parser for it, so are storing it as-it-is.
Comment on lines
+60
to
+74
| """ | ||
| Upload a dbt artifact file to the Altimate backend. | ||
| Args: | ||
| api_token: API authentication token | ||
| tenant: Tenant/instance name | ||
| dbt_core_integration_id: ID of the dbt integration | ||
| dbt_core_integration_environment: Environment type (e.g., PROD) | ||
| file_type: Type of artifact - one of: manifest, catalog, run_results, sources, semantic_manifest | ||
| file_path: Path to the artifact file | ||
| backend_url: URL of the Altimate backend | ||
| Returns: | ||
| Dict with 'ok' boolean and optional 'message' on failure | ||
| """ |
There was a problem hiding this comment.
The documentation added for the onboard_file function is helpful and clear. However, it should note that validation is the caller's responsibility. Currently, semantic_manifest files are not validated before being uploaded, which differs from other artifact types.
Comment on lines
13
to
34
| class RunResultsV1(BaseRunResultsV1): | ||
| model_config = ConfigDict(extra="allow") | ||
|
|
||
|
|
||
| class RunResultsV2(BaseRunResultsV2): | ||
| model_config = ConfigDict(extra="allow") | ||
|
|
||
|
|
||
| class RunResultsV3(BaseRunResultsV3): | ||
| model_config = ConfigDict(extra="allow") | ||
|
|
||
|
|
||
| class RunResultsV4(BaseRunResultsV4): | ||
| model_config = ConfigDict(extra="allow") | ||
|
|
||
|
|
||
| class RunResultsV5(BaseRunResultsV5): | ||
| model_config = ConfigDict(extra="allow") | ||
|
|
||
|
|
||
| class RunResultsV6(BaseRunResultsV6): | ||
| model_config = ConfigDict(extra="allow") |
There was a problem hiding this comment.
The model_config = ConfigDict(extra="allow") is redundant here since the base classes (BaseRunResultsV1 through BaseRunResultsV6) already have this configuration set. While this doesn't cause issues, it adds unnecessary repetition. Consider removing these redundant configurations unless there's a specific need to override them.
Comment on lines
3
to
19
| from pydantic import ConfigDict | ||
|
|
||
| from vendor.dbt_artifacts_parser.parsers.sources.sources_v1 import SourcesV1 as BaseSourcesV1 | ||
| from vendor.dbt_artifacts_parser.parsers.sources.sources_v2 import SourcesV2 as BaseSourcesV2 | ||
| from vendor.dbt_artifacts_parser.parsers.sources.sources_v3 import SourcesV3 as BaseSourcesV3 | ||
|
|
||
|
|
||
| class SourcesV1(BaseSourcesV1): | ||
| model_config = ConfigDict(extra="allow") | ||
|
|
||
|
|
||
| class SourcesV2(BaseSourcesV2): | ||
| model_config = ConfigDict(extra="allow") | ||
|
|
||
|
|
||
| class SourcesV3(BaseSourcesV3): | ||
| model_config = ConfigDict(extra="allow") |
There was a problem hiding this comment.
The model_config = ConfigDict(extra="allow") is redundant here since the base classes (BaseSourcesV1 through BaseSourcesV3) already have this configuration set. While this doesn't cause issues, it adds unnecessary repetition. Consider removing these redundant configurations unless there's a specific need to override them.
Suggested change
| from pydantic import ConfigDict | |
| from vendor.dbt_artifacts_parser.parsers.sources.sources_v1 import SourcesV1 as BaseSourcesV1 | |
| from vendor.dbt_artifacts_parser.parsers.sources.sources_v2 import SourcesV2 as BaseSourcesV2 | |
| from vendor.dbt_artifacts_parser.parsers.sources.sources_v3 import SourcesV3 as BaseSourcesV3 | |
| class SourcesV1(BaseSourcesV1): | |
| model_config = ConfigDict(extra="allow") | |
| class SourcesV2(BaseSourcesV2): | |
| model_config = ConfigDict(extra="allow") | |
| class SourcesV3(BaseSourcesV3): | |
| model_config = ConfigDict(extra="allow") | |
| from vendor.dbt_artifacts_parser.parsers.sources.sources_v1 import SourcesV1 as BaseSourcesV1 | |
| from vendor.dbt_artifacts_parser.parsers.sources.sources_v2 import SourcesV2 as BaseSourcesV2 | |
| from vendor.dbt_artifacts_parser.parsers.sources.sources_v3 import SourcesV3 as BaseSourcesV3 | |
| class SourcesV1(BaseSourcesV1): | |
| ... | |
| class SourcesV2(BaseSourcesV2): | |
| ... | |
| class SourcesV3(BaseSourcesV3): | |
| ... |
| try: | ||
| run_results: RunResults = parse_run_results(run_results_dict) | ||
| except ValueError as e: | ||
| raise AltimateInvalidManifestError(f"Invalid run results file: {run_results_path}. Error: {e}") from e |
There was a problem hiding this comment.
The exception type AltimateInvalidManifestError is misleading when used for non-manifest files like run_results. Consider creating a more generic exception type such as AltimateInvalidArtifactError or using specific exceptions for each artifact type.
| try: | ||
| sources: Sources = parse_sources(sources_dict) | ||
| except ValueError as e: | ||
| raise AltimateInvalidManifestError(f"Invalid sources file: {sources_path}. Error: {e}") from e |
There was a problem hiding this comment.
The exception type AltimateInvalidManifestError is misleading when used for non-manifest files like sources. Consider creating a more generic exception type such as AltimateInvalidArtifactError or using specific exceptions for each artifact type.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 752cf7c in 1 minute and 37 seconds. Click for details.
- Reviewed
433lines of code in6files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/core/insights/schema.py:2
- Draft comment:
Removed unused ConfigDict import & model_config lines—ensure strict validation of extra fields is intended. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. src/datapilot/core/platforms/dbt/schemas/catalog.py:15
- Draft comment:
Consistently removed model_config from AltimateCatalog models; verify that extra fields are now handled as desired. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify that extra fields are handled as desired after removingmodel_config. This falls under asking the author to ensure behavior is intended, which is against the rules.
3. src/datapilot/core/platforms/dbt/schemas/manifest.py:24
- Draft comment:
Removal of model_config in manifest models simplifies schema; confirm that no extra field handling is now needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm that no extra field handling is needed after the removal ofmodel_config. This is against the rules as it asks for confirmation of intention. The comment does not provide a specific code suggestion or ask for a test to be written.
4. src/datapilot/core/platforms/dbt/schemas/run_results.py:1
- Draft comment:
Directly importing run_results classes cleans up the code; ensure removed extended classes don't affect behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure that a change doesn't affect behavior, which violates the rule against asking the author to ensure behavior is intended or tested. It doesn't provide a specific suggestion or ask for a specific test to be written.
5. src/datapilot/core/platforms/dbt/schemas/sources.py:1
- Draft comment:
Removed model_config from Sources classes; verify that extra fields no longer need to be accepted. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify that extra fields no longer need to be accepted after removingmodel_config. This is asking for confirmation of intention, which is against the rules.
6. src/datapilot/schemas/nodes.py:4
- Draft comment:
Removed model_config lines from ModelNode and SourceNode; ensure that rejecting extra fields is intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_kQf6aMX1chQstHOG
You can customize
Comment on lines
+275
to
+283
| if semantic_manifest_path: | ||
| response = onboard_file( | ||
| token, instance_name, dbt_integration_id, dbt_integration_environment, "semantic_manifest", semantic_manifest_path, backend_url | ||
| ) | ||
| if response["ok"]: | ||
| click.echo("Semantic manifest onboarded successfully!") | ||
| artifacts_uploaded.append("semantic_manifest") | ||
| else: | ||
| click.echo(f"{response['message']}") |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
mdesmet
approved these changes
Jan 6, 2026
jontsai
approved these changes
Jan 6, 2026
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 32d52c7 in 1 minute and 51 seconds. Click for details.
- Reviewed
71lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/vendor/dbt_artifacts_parser/parsers/run_results/run_results_v6.py:64
- Draft comment:
New AdapterResponse model added. Consider adding a docstring or explicit fields if known. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. src/vendor/dbt_artifacts_parser/parsers/run_results/run_results_v6.py:80
- Draft comment:
Changed adapter_response field type from dict to AdapterResponse. Ensure downstream usage handles the new model appropriately. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that downstream usage handles the new model appropriately, which is a form of asking the author to double-check their work. This violates the rule against asking the author to ensure behavior is intended or tested.
3. src/vendor/dbt_artifacts_parser/parsers/run_results/run_results_v6.py:88
- Draft comment:
New Args model introduced and used in RunResultsV6. If known, consider defining expected fields for clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. src/vendor/dbt_artifacts_parser/parsers/sources/sources_v3.py:88
- Draft comment:
New AdapterResponse model is added here too. Consider centralizing this model to avoid duplication across modules. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is about potential duplication "across modules", which inherently requires cross-file context to verify. The rules explicitly state to "Ignore cross-file issues. Only think about the file you are reviewing." Additionally, this appears to be generated code (based on the header comment), so refactoring suggestions may not be appropriate. The comment is also a code quality suggestion that would require seeing other files to verify if duplication actually exists. Without being able to see other modules, I cannot confirm there is duplication. The rules state I need "STRONG EVIDENCE that the comment is correct" and should "assume that the comment is incorrect or not useful" by default. Perhaps the reviewer has knowledge of other files in the codebase where AdapterResponse is duplicated, and this is a valid refactoring suggestion. The comment could be helpful for maintaining DRY principles across the codebase. While the reviewer may have broader context, the rules explicitly state to ignore cross-file issues and only focus on the file being reviewed. Without being able to verify duplication exists in other modules from this diff alone, I cannot confirm this comment is correct. Additionally, this is generated code, which makes refactoring suggestions less actionable. This comment should be deleted because it references cross-file issues ("across modules") which requires context beyond this single file, and the rules explicitly state to ignore cross-file issues. There is no strong evidence within this diff alone that duplication exists.
5. src/vendor/dbt_artifacts_parser/parsers/sources/sources_v3.py:101
- Draft comment:
Changed adapter_response field in Results1 to use AdapterResponse model. Verify that this change aligns with upstream data expectations. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify alignment with upstream data expectations, which is against the rules. It doesn't provide a specific suggestion or point out a clear issue with the code.
Workflow ID: wflow_KAr64mhSt5fua4DI
You can customize
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
This PR adds support for new dbt artifacts
run_resultsandsources, updates the CLI for onboarding these artifacts, and includes new schemas and tests.run_resultsandsourcesinonboard_file()inutils.py.onboardcommand incli.pyto handle new artifact paths and validate them.manifest,catalog,run_results,sources, andsemantic_manifest.run_results.pyandsources.pyfor new artifact schemas.test_utils.pyto verify supported artifact types and error handling for unsupported types.test_artifact_loaders.pyfor loadingrun_resultsandsources.test_cli.pyto check CLI help for new artifact options.run_results_v6.jsonandsources_v3.json.This description was created by
for 32d52c7. You can customize this summary. It will automatically update as commits are pushed.