Add plan to implement INFP-504#885
Conversation
WalkthroughThis PR adds comprehensive specifications and documentation for INFP-504 "Artifact Content Composition." It introduces new Jinja2 filter contracts for async artifact/file content retrieval, synchronous JSON/YAML parsing, and a context-aware execution model. Key additions include: new 🚥 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 |
| ```python | ||
| class InfrahubFilters: | ||
| def __init__(self, client: InfrahubClient) -> None: | ||
| self.client = client |
There was a problem hiding this comment.
I'm not sure that it's correct, but the way I saw this was that a client param here would be optional. That way we can initialize a Template class the same way and then if we specifically want to use these filters we'd use some setter afterwards to add the client. However this might not be required depending on how we end up populating these filters into the template class in various circumstances.
Deploying infrahub-sdk-python with
|
| Latest commit: |
4e2de5e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bb6164fb.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://gma-20260323-infp504-plan.infrahub-sdk-python.pages.dev |
56476d3 to
4e2de5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/infp-504-artifact-composition/contracts/filter-interfaces.md`:
- Around line 101-106: The validation matrix contradicts the WORKER-only policy
for artifact_content/file_object_content: update the table so "Worker filters"
are allowed only under the WORKER context and blocked in CORE and LOCAL;
specifically change the LOCAL row entry for the "Worker filters" column from
"allowed" to "blocked" (and verify CORE remains "blocked" and WORKER remains
"allowed") and ensure any accompanying text referring to
`artifact_content`/`file_object_content` reflects the WORKER-only rule.
In `@dev/specs/infp-504-artifact-composition/plan.md`:
- Line 26: The plan incorrectly asks to add enable_async=True to the file-based
environment even though _get_file_based_environment already sets
enable_async=True; update the requirement text to state "verify existing
behavior" (or similar) instead of "add", and reference the existing
implementation in _get_file_based_environment within the template module so
reviewers know to confirm the current enable_async=True setting is correct.
In `@dev/specs/infp-504-artifact-composition/quickstart.md`:
- Line 1: Multiple markdownlint MD013 (line-length) violations exist in the
INFP-504 spec files (e.g., dev/specs/infp-504-artifact-composition/quickstart.md
lines ~9,88–90 and many lines across spec.md, tasks.md, plan.md, etc.); run
markdownlint locally and break any lines longer than 80 characters by inserting
sensible hard line breaks or rewrapping sentences, update affected files
(quickstart.md, spec.md, tasks.md, plan.md, data-model.md,
contracts/filter-interfaces.md, checklists/requirements.md, research.md) to
conform to the 80-char limit, and re-run markdownlint until there are no MD013
errors before committing.
In `@dev/specs/infp-504-artifact-composition/research.md`:
- Around line 13-14: The caveat is outdated: the function
_get_file_based_environment already sets enable_async=True (see the template
module's __init__), so update
dev/specs/infp-504-artifact-composition/research.md to remove or correct the
statement that file-based environment lacks enable_async=True and replace it
with a note confirming async is already enabled by _get_file_based_environment
(or point to the exact function that sets enable_async=True).
In `@dev/specs/infp-504-artifact-composition/tasks.md`:
- Line 43: T005's test expects artifact_content to be "LOCAL (allowed)" which
contradicts the spec's WORKER-only execution policy; update the task/test to
require LOCAL access be blocked: change the test expectation for task T005
(artifact_content) from allowed to blocked and ensure any test harness or
validation logic that checks execution_policy (WORKER-only) enforces that LOCAL
access is denied for artifact_content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c72764e5-cee8-4e4b-8085-dc37e34cdb57
📒 Files selected for processing (7)
dev/specs/infp-504-artifact-composition/contracts/filter-interfaces.mddev/specs/infp-504-artifact-composition/data-model.mddev/specs/infp-504-artifact-composition/plan.mddev/specs/infp-504-artifact-composition/quickstart.mddev/specs/infp-504-artifact-composition/research.mddev/specs/infp-504-artifact-composition/spec.mddev/specs/infp-504-artifact-composition/tasks.md
| | Context | Trusted filters | Worker filters | Untrusted filters | | ||
| | ------- | :-: | :-: | :-: | | ||
| | `CORE` | allowed | blocked | blocked | | ||
| | `WORKER` | allowed | allowed | blocked | | ||
| | `LOCAL` | allowed | allowed | allowed | | ||
|
|
There was a problem hiding this comment.
Validation matrix contradicts WORKER-only filter policy.
This table allows “Worker filters” in LOCAL, which conflicts with the spec’s WORKER-only rule for artifact_content/file_object_content. Please align the matrix to one policy across docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/specs/infp-504-artifact-composition/contracts/filter-interfaces.md`
around lines 101 - 106, The validation matrix contradicts the WORKER-only policy
for artifact_content/file_object_content: update the table so "Worker filters"
are allowed only under the WORKER context and blocked in CORE and LOCAL;
specifically change the LOCAL row entry for the "Worker filters" column from
"allowed" to "blocked" (and verify CORE remains "blocked" and WORKER remains
"allowed") and ensure any accompanying text referring to
`artifact_content`/`file_object_content` reflects the WORKER-only rule.
|
|
||
| The `SandboxedEnvironment` already uses `enable_async=True`. Jinja2's `auto_await` automatically awaits filter return values during `render_async()`. The new content-fetching filters can be `async def` — no bridging needed. | ||
|
|
||
| **Required change**: Add `enable_async=True` to the file-based environment (`_get_file_based_environment()`) so async filters work for file-based templates too. |
There was a problem hiding this comment.
This “required change” appears already implemented in the current SDK.
_get_file_based_environment() already includes enable_async=True (infrahub_sdk/template/__init__.py:134-150), so this step should be reworded as “verify existing behavior” rather than “add.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/specs/infp-504-artifact-composition/plan.md` at line 26, The plan
incorrectly asks to add enable_async=True to the file-based environment even
though _get_file_based_environment already sets enable_async=True; update the
requirement text to state "verify existing behavior" (or similar) instead of
"add", and reference the existing implementation in _get_file_based_environment
within the template module so reviewers know to confirm the current
enable_async=True setting is correct.
| @@ -0,0 +1,90 @@ | |||
| # Quickstart: Artifact Content Composition | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify markdownlint on the INFP-504 spec docs
fd -e md dev/specs/infp-504-artifact-composition \
| xargs -r npx --yes markdownlint-cli2Repository: opsmill/infrahub-sdk-python
Length of output: 532
🏁 Script executed:
#!/bin/bash
# Correct approach: find markdown files in the target directory and run markdownlint
fd . dev/specs/infp-504-artifact-composition -e md -type fRepository: opsmill/infrahub-sdk-python
Length of output: 241
🏁 Script executed:
#!/bin/bash
# Find markdown files in the target directory
fd . dev/specs/infp-504-artifact-composition --type f -e mdRepository: opsmill/infrahub-sdk-python
Length of output: 513
🏁 Script executed:
#!/bin/bash
# Run markdownlint on the found markdown files
npx --yes markdownlint-cli2 \
dev/specs/infp-504-artifact-composition/checklists/requirements.md \
dev/specs/infp-504-artifact-composition/contracts/filter-interfaces.md \
dev/specs/infp-504-artifact-composition/data-model.md \
dev/specs/infp-504-artifact-composition/plan.md \
dev/specs/infp-504-artifact-composition/quickstart.md \
dev/specs/infp-504-artifact-composition/research.md \
dev/specs/infp-504-artifact-composition/spec.md \
dev/specs/infp-504-artifact-composition/tasks.mdRepository: opsmill/infrahub-sdk-python
Length of output: 51074
Fix markdownlint violations in INFP-504 spec files before committing.
Markdownlint found 705 errors across the repository. The INFP-504 spec files specifically contain line-length violations (MD013) that exceed the 80-character limit, including in quickstart.md (lines 9, 88–90). Per coding guidelines, run markdownlint and resolve violations before committing markdown changes.
Markdownlint violations in INFP-504 spec files
dev/specs/infp-504-artifact-composition/quickstart.md:9:81 error MD013/line-length
dev/specs/infp-504-artifact-composition/quickstart.md:88:81 error MD013/line-length
dev/specs/infp-504-artifact-composition/quickstart.md:89:81 error MD013/line-length
dev/specs/infp-504-artifact-composition/quickstart.md:90:81 error MD013/line-length
dev/specs/infp-504-artifact-composition/checklists/requirements.md:3, 34, 36 errors
dev/specs/infp-504-artifact-composition/contracts/filter-interfaces.md:16–20, 33–38, 52, 66, 78 errors
dev/specs/infp-504-artifact-composition/data-model.md:19, 42–43, 51, 105, 112, 118, 126, 136, 145 errors
dev/specs/infp-504-artifact-composition/plan.md:8, 13, 18, 24, 26, 30, 34, 58–59, 92–93, 105–108 errors
dev/specs/infp-504-artifact-composition/research.md:9, 11, 13, 17–18, 21, 27, 29, 33, 35, 37, 43, 63, 69, 75 errors
dev/specs/infp-504-artifact-composition/spec.md:10, 16, 18, 20, 22, 26–29, 33, 35, 37, 39, 43–45, 51, 53, 55, 59–61, 65, 67, 69, 71, 73, 77–79, 85–90, 96–109, 113–117, 123–128, 132–137, 142, 146, 150–152, 158–162 errors
dev/specs/infp-504-artifact-composition/tasks.md:9, 25–26, 28, 34, 36, 40–42, 44, 50, 52, 56–58, 66, 68, 72–74, 76, 84, 86–88, 99, 104–106 errors
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/specs/infp-504-artifact-composition/quickstart.md` at line 1, Multiple
markdownlint MD013 (line-length) violations exist in the INFP-504 spec files
(e.g., dev/specs/infp-504-artifact-composition/quickstart.md lines ~9,88–90 and
many lines across spec.md, tasks.md, plan.md, etc.); run markdownlint locally
and break any lines longer than 80 characters by inserting sensible hard line
breaks or rewrapping sentences, update affected files (quickstart.md, spec.md,
tasks.md, plan.md, data-model.md, contracts/filter-interfaces.md,
checklists/requirements.md, research.md) to conform to the 80-char limit, and
re-run markdownlint until there are no MD013 errors before committing.
| **Caveat**: The file-based environment (`_get_file_based_environment()` at line 140) does NOT currently set `enable_async=True`. This must be added for async filters to work with file-based templates. | ||
|
|
There was a problem hiding this comment.
R-001 caveat is factually stale and conflicts with current code.
Line 13 says file-based environment lacks enable_async=True, but infrahub_sdk/template/__init__.py:134-150 already sets it. This should be corrected to avoid planning redundant work.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/specs/infp-504-artifact-composition/research.md` around lines 13 - 14,
The caveat is outdated: the function _get_file_based_environment already sets
enable_async=True (see the template module's __init__), so update
dev/specs/infp-504-artifact-composition/research.md to remove or correct the
statement that file-based environment lacks enable_async=True and replace it
with a note confirming async is already enabled by _get_file_based_environment
(or point to the exact function that sets enable_async=True).
| - [ ] T003 [US1] Create `InfrahubFilters` class in `infrahub_sdk/template/infrahub_filters.py` — new file. Class holds `InfrahubClient` reference, exposes async filter methods. Methods are `async def` (Jinja2 `auto_await` handles them in async render mode per R-001). Raises `JinjaFilterError` when called without a client. Include unit tests for instantiation with/without client. (IFC-2371) | ||
| - [ ] T004 [US1] Implement `artifact_content` async method on `InfrahubFilters` in `infrahub_sdk/template/infrahub_filters.py` — uses `self.client.object_store.get(identifier=storage_id)`. Raises `JinjaFilterError` on: null/empty storage_id, retrieval failure, permission denied (catch `AuthenticationError` per R-006). Artifacts are always text (no binary check needed per R-003). Include unit tests: happy path (mocked ObjectStore), null, empty, not-found, network error, permission denied, no-client error with descriptive message. (IFC-2372) | ||
| - [ ] T005 [US1] [US4] Add `client` parameter to `Jinja2Template.__init__` and wire up filter registration in `infrahub_sdk/template/__init__.py` — add `client: InfrahubClient | None = None` param. When client provided: instantiate `InfrahubFilters`, register `artifact_content` into Jinja2 env filter map. Add `enable_async=True` to `_get_file_based_environment()` (per R-001 caveat). Register `artifact_content` in `FilterDefinition` registry with `allowed_contexts=ExecutionContext.WORKER`. Include unit tests: render with client (mocked), render without client (error), validation in CORE (blocked), WORKER (allowed), LOCAL (allowed). Verify existing untrusted filters like `safe` remain blocked in WORKER context (US4 AC3). (IFC-2375 partial + IFC-2376 partial) | ||
|
|
There was a problem hiding this comment.
T005 test expectation conflicts with the intended security model.
“LOCAL (allowed)” for artifact_content is inconsistent with the WORKER-only execution policy defined in the spec/quickstart. This task should require LOCAL to be blocked.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/specs/infp-504-artifact-composition/tasks.md` at line 43, T005's test
expects artifact_content to be "LOCAL (allowed)" which contradicts the spec's
WORKER-only execution policy; update the task/test to require LOCAL access be
blocked: change the test expectation for task T005 (artifact_content) from
allowed to blocked and ensure any test harness or validation logic that checks
execution_policy (WORKER-only) enforces that LOCAL access is denied for
artifact_content.
This PR only provides the implementation plan for INFP-504 - Artifact composition. It proposes a way to implement Jinja2 filters in order to be able to include the content of an artifact or a file object within another one.
Summary by CodeRabbit
Release Notes