Open
Conversation
WalkthroughA new specification directory structure is introduced containing documentation for artifact content composition via Jinja2 filters. The implementation adds a formal specification document outlining feature requirements, security models, functional changes, and success criteria for enabling inline artifact content and file object content through new Jinja2 filters. Accompanying this is a specification quality checklist document that tracks completion status against defined quality and readiness criteria. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Deploying infrahub-sdk-python with
|
| Latest commit: |
f9ccc45
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7f27c33a.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pog-artifact-composition-spe.infrahub-sdk-python.pages.dev |
ogenstad
force-pushed
the
pog-artifact-composition-spec
branch
from
13cefaa to
f9ccc45
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dev/specs/001-artifact-composition/checklists/requirements.md`:
- Around line 9-11: The checklist wrongly marks items as complete even though
the spec contains implementation details and is written for SDK implementers;
update the checklist in requirements.md by unchecking the four items at lines
shown (the items about "No implementation details", "Written for non-technical
stakeholders", "Success criteria are technology-agnostic", and "No
implementation details leak") and add a short note stating this is a technical
spec (not a pure PRD) calling out examples such as FR-001/006/010 naming
InfrahubClient/InfrahubClientSync, FR-012 file path
infrahub_sdk/template/exceptions.py, FR-013
client.object_store.get(identifier=storage_id), FR-002/007 naming
FilterDefinition/InfrahubFilters/Jinja2Template, and SC-005 referencing uv run
pytest tests/unit/ so reviewers understand the intentional deviation.
- Around line 34-36: The Markdown file ends with a Notes list but lacks a
trailing blank line; update
dev/specs/001-artifact-composition/checklists/requirements.md by adding a single
blank line after the final list item (the lines starting with "One open question
remains..." / "Ordering guarantee..." / "`from_json`/`from_yaml` existence...")
so there is an empty line at EOF, conforming to the guideline to add blank lines
before and after lists.
In `@dev/specs/001-artifact-composition/spec.md`:
- Around line 157-161: The Clarifications list at the end of the file lacks a
trailing blank line; update the final list in the "Clarifications" section (the
bullet list ending at the end of file) to include a blank line after the list so
the Markdown convention "Add blank lines before and after lists" is satisfied —
ensure there's exactly one empty line following the final list item and that any
preceding list start also has a blank line above it if missing.
- Around line 100-102: FR-007 is inconsistent with the three-context validation
model: registering artifact_content and file_object_content as trusted=False
will equate them with existing filters like safe and break the intended
Prefect-worker allowance; update the spec wording to mark FR-007 as provisional
and add a note that FilterDefinition must be extended (e.g., a new enum or flag)
to distinguish "worker-allowed" filters from general trusted/untrusted, and
reference artifact_content, file_object_content, FilterDefinition,
validate(restricted=True), safe, and JinjaTemplateOperationViolationError so
implementers know to avoid simply copying safe's trusted=False registration and
instead await the enum migration described in Open Questions.
Comment on lines
+9
to
+11
| - [x] No implementation details (languages, frameworks, APIs) | ||
| - [x] Focused on user value and business needs | ||
| - [x] Written for non-technical stakeholders |
There was a problem hiding this comment.
Four checklist items are marked complete but contradict the spec's content.
| Line | Item | Counter-evidence in spec.md |
|---|---|---|
| 9 | [x] No implementation details (languages, frameworks, APIs) |
FR-001/006/010 name InfrahubClient/InfrahubClientSync; FR-012 names file path infrahub_sdk/template/exceptions.py; FR-013 names client.object_store.get(identifier=storage_id); FR-002/007 name FilterDefinition, InfrahubFilters, Jinja2Template. |
| 11 | [x] Written for non-technical stakeholders |
The spec is addressed entirely to SDK implementers, with Python class names, method signatures, and SDK module references throughout. |
| 19 | [x] Success criteria are technology-agnostic (no implementation details) |
SC-005 names uv run pytest tests/unit/; SC-004 names InfrahubClient; SC-002 names validate(restricted=True). |
| 30 | [x] No implementation details leak into specification |
Same evidence as line 9. |
If this spec intentionally blends product requirements with technical design (a "technical spec" rather than a pure PRD), update the checklist criteria to reflect that, or uncheck these items and add a note explaining the intentional deviation.
Also applies to: 19-19, 29-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/specs/001-artifact-composition/checklists/requirements.md` around lines 9
- 11, The checklist wrongly marks items as complete even though the spec
contains implementation details and is written for SDK implementers; update the
checklist in requirements.md by unchecking the four items at lines shown (the
items about "No implementation details", "Written for non-technical
stakeholders", "Success criteria are technology-agnostic", and "No
implementation details leak") and add a short note stating this is a technical
spec (not a pure PRD) calling out examples such as FR-001/006/010 naming
InfrahubClient/InfrahubClientSync, FR-012 file path
infrahub_sdk/template/exceptions.py, FR-013
client.object_store.get(identifier=storage_id), FR-002/007 naming
FilterDefinition/InfrahubFilters/Jinja2Template, and SC-005 referencing uv run
pytest tests/unit/ so reviewers understand the intentional deviation.
Comment on lines
+34
to
+36
| - One open question remains intentionally: whether to add a Python transform convenience SDK method (FR scope question flagged in Open Questions section, documented for planning phase). | ||
| - Ordering guarantee is explicitly out of scope and documented as a known limitation. | ||
| - `from_json`/`from_yaml` existence in the current filter set is flagged as an assumption to verify during planning. |
There was a problem hiding this comment.
Missing blank line after the Notes list.
The Notes list ends at line 36 (end of file) with no trailing blank line, violating the Markdown convention.
📝 Proposed fix
- `from_json`/`from_yaml` existence in the current filter set is flagged as an assumption to verify during planning.
+As per coding guidelines: "Add blank lines before and after lists in Markdown files."
📝 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.
Suggested change
| - One open question remains intentionally: whether to add a Python transform convenience SDK method (FR scope question flagged in Open Questions section, documented for planning phase). | |
| - Ordering guarantee is explicitly out of scope and documented as a known limitation. | |
| - `from_json`/`from_yaml` existence in the current filter set is flagged as an assumption to verify during planning. | |
| - One open question remains intentionally: whether to add a Python transform convenience SDK method (FR scope question flagged in Open Questions section, documented for planning phase). | |
| - Ordering guarantee is explicitly out of scope and documented as a known limitation. | |
| - `from_json`/`from_yaml` existence in the current filter set is flagged as an assumption to verify during planning. | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/specs/001-artifact-composition/checklists/requirements.md` around lines
34 - 36, The Markdown file ends with a Notes list but lacks a trailing blank
line; update dev/specs/001-artifact-composition/checklists/requirements.md by
adding a single blank line after the final list item (the lines starting with
"One open question remains..." / "Ordering guarantee..." /
"`from_json`/`from_yaml` existence...") so there is an empty line at EOF,
conforming to the guideline to add blank lines before and after lists.
Comment on lines
+100
to
+102
| - **FR-005**: Both `artifact_content` and `file_object_content` MUST raise `JinjaFilterError` when the input `storage_id` is null or empty, or when the object store cannot retrieve the content for any reason (not found, network failure, auth failure). | ||
| - **FR-006**: Both `artifact_content` and `file_object_content` MUST raise `JinjaFilterError` when invoked and no `InfrahubClient` was supplied to `Jinja2Template` at construction time. The error message MUST name the filter and explain that an `InfrahubClient` is required. | ||
| - **FR-007**: Both `artifact_content` and `file_object_content` MUST be registered with `trusted=False` in the `FilterDefinition` registry so that `validate(restricted=True)` blocks them in the computed attributes execution context (Infrahub API server). They are only permitted to execute on Prefect workers, where an `InfrahubClient` is available. |
There was a problem hiding this comment.
FR-007 is internally inconsistent with the three-context validation model — mark as provisional.
FR-007 registers both new filters as trusted=False, but this puts them in the same bucket as safe (and similar currently-untrusted filters). User Story 4, Acceptance Scenario 3 explicitly requires that other trusted=False filters still raise JinjaTemplateOperationViolationError in the Prefect-worker context. With the current binary validate(restricted: bool), there is no way to simultaneously:
- block
artifact_contentin the computed-attributes context ✓ - allow
artifact_contentin the Prefect-worker context ✓ - continue blocking
safein the Prefect-worker context ✓
The Open Questions section (line 151) correctly identifies the enum migration needed, but FR-007 reads as a final requirement rather than a provisional one. An implementer reading FR-007 in isolation could register the new filters identically to safe and still satisfy the wording — which would break US4/SC-002.
📝 Suggested wording to make FR-007 provisional
-- **FR-007**: Both `artifact_content` and `file_object_content` MUST be registered with `trusted=False` in the `FilterDefinition` registry so that `validate(restricted=True)` blocks them in the computed attributes execution context (Infrahub API server). They are only permitted to execute on Prefect workers, where an `InfrahubClient` is available.
+- **FR-007**: Both `artifact_content` and `file_object_content` MUST be blocked in the computed attributes execution context (Infrahub API server) and permitted in the Prefect-worker execution context. *Provisional implementation*: register with `trusted=False` so that the existing `validate(restricted=True)` blocks them initially. **This requirement depends on resolution of the "Validation level model" open question**, which must introduce a dedicated worker-context tag to distinguish these filters from currently-untrusted filters (e.g., `safe`) that must remain blocked even in the Prefect-worker context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/specs/001-artifact-composition/spec.md` around lines 100 - 102, FR-007 is
inconsistent with the three-context validation model: registering
artifact_content and file_object_content as trusted=False will equate them with
existing filters like safe and break the intended Prefect-worker allowance;
update the spec wording to mark FR-007 as provisional and add a note that
FilterDefinition must be extended (e.g., a new enum or flag) to distinguish
"worker-allowed" filters from general trusted/untrusted, and reference
artifact_content, file_object_content, FilterDefinition,
validate(restricted=True), safe, and JinjaTemplateOperationViolationError so
implementers know to avoid simply copying safe's trusted=False registration and
instead await the enum migration described in Open Questions.
Comment on lines
+157
to
+161
| - Q: Are `artifact_content` and `file_object_content` identical at the storage API level, or do they use different API paths / metadata handling? → A: Different implementations — `file_object_content` uses a different API path or carries different metadata handling than `artifact_content`. | ||
| - Q: Where are these filters permitted to execute, and what mechanism enforces the boundary? → A: Blocked in computed attributes (executed locally in the Infrahub API server, which uses `validate(restricted=True)`); permitted on Prefect workers, which have access to an `InfrahubClient`. The `trusted=False` registration enforces this boundary via the existing restricted-mode validation. | ||
| - Q: What exception class should filter-level errors (no client, retrieval failure) raise? → A: A new `JinjaFilterError` class that is a child of the existing `JinjaTemplateError` base class. | ||
| - Q: Should the SDK expose a convenience method for artifact content retrieval in Python transforms? → A: No new method — document `client.object_store.get(identifier=storage_id)` directly. | ||
| - Q: What should `from_json`/`from_yaml` do on malformed input? → A: Raise `JinjaFilterError` on malformed JSON or YAML input. |
There was a problem hiding this comment.
Missing blank line after the final list.
The Clarifications list ends at line 161 (end of file) without a trailing blank line, violating the project's Markdown convention.
📝 Proposed fix
- Q: What should `from_json`/`from_yaml` do on malformed input? → A: Raise `JinjaFilterError` on malformed JSON or YAML input.
+As per coding guidelines: "Add blank lines before and after lists in Markdown files."
📝 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.
Suggested change
| - Q: Are `artifact_content` and `file_object_content` identical at the storage API level, or do they use different API paths / metadata handling? → A: Different implementations — `file_object_content` uses a different API path or carries different metadata handling than `artifact_content`. | |
| - Q: Where are these filters permitted to execute, and what mechanism enforces the boundary? → A: Blocked in computed attributes (executed locally in the Infrahub API server, which uses `validate(restricted=True)`); permitted on Prefect workers, which have access to an `InfrahubClient`. The `trusted=False` registration enforces this boundary via the existing restricted-mode validation. | |
| - Q: What exception class should filter-level errors (no client, retrieval failure) raise? → A: A new `JinjaFilterError` class that is a child of the existing `JinjaTemplateError` base class. | |
| - Q: Should the SDK expose a convenience method for artifact content retrieval in Python transforms? → A: No new method — document `client.object_store.get(identifier=storage_id)` directly. | |
| - Q: What should `from_json`/`from_yaml` do on malformed input? → A: Raise `JinjaFilterError` on malformed JSON or YAML input. | |
| - Q: Are `artifact_content` and `file_object_content` identical at the storage API level, or do they use different API paths / metadata handling? → A: Different implementations — `file_object_content` uses a different API path or carries different metadata handling than `artifact_content`. | |
| - Q: Where are these filters permitted to execute, and what mechanism enforces the boundary? → A: Blocked in computed attributes (executed locally in the Infrahub API server, which uses `validate(restricted=True)`); permitted on Prefect workers, which have access to an `InfrahubClient`. The `trusted=False` registration enforces this boundary via the existing restricted-mode validation. | |
| - Q: What exception class should filter-level errors (no client, retrieval failure) raise? → A: A new `JinjaFilterError` class that is a child of the existing `JinjaTemplateError` base class. | |
| - Q: Should the SDK expose a convenience method for artifact content retrieval in Python transforms? → A: No new method — document `client.object_store.get(identifier=storage_id)` directly. | |
| - Q: What should `from_json`/`from_yaml` do on malformed input? → A: Raise `JinjaFilterError` on malformed JSON or YAML input. | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/specs/001-artifact-composition/spec.md` around lines 157 - 161, The
Clarifications list at the end of the file lacks a trailing blank line; update
the final list in the "Clarifications" section (the bullet list ending at the
end of file) to include a blank line after the list so the Markdown convention
"Add blank lines before and after lists" is satisfied — ensure there's exactly
one empty line following the final list item and that any preceding list start
also has a blank line above it if missing.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Why
Provide specifications for INFP-504
What changed
Add Spec Kit specifications
Summary by CodeRabbit