feat(vfs): add pixel dir, pass attachments, shader used-by routes#162
feat(vfs): add pixel dir, pass attachments, shader used-by routes#162BANANASJIM merged 4 commits intomasterfrom
Conversation
Fill three VFS route gaps: 1. /draws/<eid>/pixel/ - add to _DRAW_CHILDREN and register dir routes so `rdc ls` and `rdc tree` show the pixel directory 2. /passes/<name>/attachments/ - lazy-populate color/depth children via populate_pass_attachments(), add pass_attachment handler returning resource IDs 3. /shaders/<id>/used-by - persist EID list in shader_meta, add leaf route and handler returning which draws reference a shader
📝 WalkthroughWalkthroughAdds VFS routes and handlers to expose pass attachment targets and shaders' EID usage. Extends shader cache to record EIDs, adds dynamic pass-attachment population during VFS listing/tree population, updates CLI VFS extractors, and adds unit and e2e tests exercising these endpoints. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DaemonVFS as "Daemon: VFS Handler"
participant VfsTree as "VfsTree"
participant Adapter as "Replay Adapter"
Client->>DaemonVFS: GET /passes/{name}/attachments/{attachment}
DaemonVFS->>VfsTree: check /passes/{name} subtree
VfsTree-->>DaemonVFS: node (may be unpopulated)
DaemonVFS->>DaemonVFS: _ensure_pass_attachments_populated(request, path)
DaemonVFS->>Adapter: seek to pass.begin_eid
Adapter-->>DaemonVFS: pipeline state
DaemonVFS->>VfsTree: populate_pass_attachments(tree, name, pipe_state)
VfsTree-->>DaemonVFS: attachment leaf created
DaemonVFS-->>Client: return attachment info (resource_id / error)
sequenceDiagram
participant Client
participant ShaderHandler as "Daemon: Shader Handler"
participant State as "Daemon State"
participant Cache as "Shader Cache"
Client->>ShaderHandler: GET /shaders/{id}/used-by
ShaderHandler->>State: ensure shader cache built
State->>Cache: _build_shader_cache (collect EIDs)
Cache-->>ShaderHandler: shader meta with eids list
ShaderHandler-->>Client: return { id, eids }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||
![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 (1)
tests/unit/test_draws_events_daemon.py (1)
628-639: Strengthenshader_used_byassertions to catch duplicates/order regressions.Using sets at Line 633 and Line 639 masks duplicate EIDs. Prefer asserting the exact normalized list (or both length and content).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_draws_events_daemon.py` around lines 628 - 639, The current tests test_returns_eids and test_all_eids_correct use set(...) which hides duplicates and order regressions for the shader_used_by RPC; update the assertions to verify the exact normalized list and length instead of a set. Locate the tests using rpc_request("shader_used_by", {"id": ...}), _handle_request and _make_shader_used_by_state and replace the set-based checks with either an exact list equality assert (e.g. assert resp["result"]["eids"] == [10, 20]) or two checks asserting length (assert len(resp["result"]["eids"]) == 2) and content (assert set(resp["result"]["eids"]) == {10, 20}) so duplicates or ordering changes will fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rdc/handlers/query.py`:
- Around line 619-630: Fix the color attachment indexing so it uses the raw
output slots instead of filtering nulls: in the block that handles
attachment.startswith("color") use pipe.GetOutputTargets() directly (store the
full list, not a filtered one) and check bounds against that full list, then
verify that the selected slot (targets[idx]) is non-null (return _error_response
with -32001 if it is null) before returning _result_response with
int(targets[idx].resource); reference the variables/functions: attachment,
pipe.GetOutputTargets(), targets, idx, _error_response, _result_response,
request_id, name. Also add a regression test that constructs a sparse output
scenario (e.g., color0 present, color1 null, color2 present) and asserts color2
resolves correctly while color1 returns the not-found error.
---
Nitpick comments:
In `@tests/unit/test_draws_events_daemon.py`:
- Around line 628-639: The current tests test_returns_eids and
test_all_eids_correct use set(...) which hides duplicates and order regressions
for the shader_used_by RPC; update the assertions to verify the exact normalized
list and length instead of a set. Locate the tests using
rpc_request("shader_used_by", {"id": ...}), _handle_request and
_make_shader_used_by_state and replace the set-based checks with either an exact
list equality assert (e.g. assert resp["result"]["eids"] == [10, 20]) or two
checks asserting length (assert len(resp["result"]["eids"]) == 2) and content
(assert set(resp["result"]["eids"]) == {10, 20}) so duplicates or ordering
changes will fail.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/rdc/commands/vfs.pysrc/rdc/handlers/_helpers.pysrc/rdc/handlers/query.pysrc/rdc/handlers/shader.pysrc/rdc/handlers/vfs.pysrc/rdc/vfs/router.pysrc/rdc/vfs/tree_cache.pytests/unit/test_draws_events_daemon.pytests/unit/test_shader_preload.pytests/unit/test_vfs_daemon.pytests/unit/test_vfs_router.pytests/unit/test_vfs_tree_cache.py
Greptile SummaryThis PR systematically addresses three VFS route gaps identified in the design documentation, adding pixel history directory discovery, lazy-populated pass attachments, and shader usage reverse indexing. Key Changes:
Implementation Quality:
The implementation is production-ready with no identified issues. Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[VFS Route Request] --> B{Route Type?}
B -->|Pixel| C["pixel directory route"]
B -->|Pass Attach| D["pass attachment route"]
B -->|Shader Used| E["shader used-by route"]
C --> F[pixel_history handler]
F --> G[Return pixel modifications]
D --> H{First Access?}
H -->|Yes| I[populate_pass_attachments]
H -->|No| J[Use cached tree]
I --> K[Inspect pipeline state]
K --> L[Create color/depth nodes]
L --> M[pass_attachment handler]
J --> M
M --> N[Return resource ID]
E --> O[_build_shader_cache]
O --> P[Track shader EIDs]
P --> Q[shader_used_by handler]
Q --> R[Return EID list]
Last reviewed commit: 1f15913 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
If a draw uses the same shader resource ID in multiple stages (e.g., shader 100 in both VS and PS), |
…andler The handler compacted targets (filtering resource==0) before indexing, but populate_pass_attachments uses enumerate() to preserve original indices. This caused wrong resource lookups when empty slots exist. Use all_targets with original index and validate non-zero resource. Also add e2e tests 5.28-5.31 for new VFS routes: draw pixel listing, pass attachments, shader used-by, and pass attachment detail.
Add 6 e2e tests (5.32-5.37) covering pixel dir, pass attachment depth/invalid, shader used-by listing for both shaders 111 and 112. Update blackbox test catalog with all 10 new VFS route gap tests.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rdc/handlers/query.py`:
- Around line 596-598: The code currently defaults name and attachment to empty
strings (e.g., name = str(params.get("name", "")) and attachment =
str(params.get("attachment", ""))) which masks malformed requests as not-found
errors; change these reads to validate presence and non-empty values and return
a JSON-RPC invalid params error (-32602) when missing/empty instead of
proceeding to lookups (affecting the initial block that checks
state.vfs_tree/pass_name_map and the other occurrences where params are
defaulted). Specifically, replace the defaulting pattern with explicit checks of
params.get("name") and params.get("attachment") (or check after casting) and if
either is missing or empty, return the RPC error -32602 with a clear message
rather than performing the pass or attachment lookup.
In `@tests/e2e/blackbox_test_catalog.md`:
- Around line 279-291: The Coverage Summary table is stale and the counts (e.g.,
the "Query commands" row and NOTE counts) do not match the per-test status
markers; regenerate the summary so the "Query commands" count, NOTE counts, the
"**TOTAL**" row and the "Pass rate" line are recomputed from the actual per-test
statuses, update any mismatched cells (including the single failing/tested
counts) and ensure Pass rate (158/159 -> percentage) is correct and consistent
with the recalculated totals.
In `@tests/e2e/test_vfs.py`:
- Around line 276-277: The test currently picks the first pass via
passes_out.strip().splitlines()[0], which is brittle; instead iterate over the
pass names returned by passes_out (from rdc_ok("ls", "/passes",
session=vkcube_session)) and select a pass whose directory listing (via
rdc_ok("ls", f"/passes/{pass_name}", session=vkcube_session)) contains the
required attachment string (e.g., "color0" or "depth") before using it; update
all occurrences where pass_name is chosen (the blocks using passes_out,
pass_name) to perform this capability-based selection and fail the test if no
suitable pass is found.
| name = str(params.get("name", "")) | ||
| attachment = str(params.get("attachment", "")) | ||
| if state.vfs_tree and name in state.vfs_tree.pass_name_map: |
There was a problem hiding this comment.
Return -32602 for missing name/attachment instead of not-found errors.
Defaulting to "" makes malformed requests look like lookup failures (pass not found / unknown attachment). This should be treated as invalid parameters for clearer client behavior.
💡 Proposed fix
- name = str(params.get("name", ""))
- attachment = str(params.get("attachment", ""))
+ if "name" not in params or not str(params["name"]).strip():
+ return _error_response(request_id, -32602, "missing name"), True
+ if "attachment" not in params or not str(params["attachment"]).strip():
+ return _error_response(request_id, -32602, "missing attachment"), True
+ name = str(params["name"]).strip()
+ attachment = str(params["attachment"]).strip()Also applies to: 603-605, 632-632
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rdc/handlers/query.py` around lines 596 - 598, The code currently
defaults name and attachment to empty strings (e.g., name =
str(params.get("name", "")) and attachment = str(params.get("attachment", "")))
which masks malformed requests as not-found errors; change these reads to
validate presence and non-empty values and return a JSON-RPC invalid params
error (-32602) when missing/empty instead of proceeding to lookups (affecting
the initial block that checks state.vfs_tree/pass_name_map and the other
occurrences where params are defaulted). Specifically, replace the defaulting
pattern with explicit checks of params.get("name") and params.get("attachment")
(or check after casting) and if either is missing or empty, return the RPC error
-32602 with a clear message rather than performing the pass or attachment
lookup.
| | Query commands | 39 | 39 | 0 | 1 | | ||
| | Output formats | 10 | 10 | 0 | 0 | | ||
| | VFS navigation | 37 | 37 | 0 | 0 | | ||
| | Export commands | 14 | 14 | 0 | 0 | | ||
| | Debug commands | 7 | 6 | 1 | 0 | | ||
| | Assert/CI | 13 | 13 | 0 | 0 | | ||
| | Diff | 7 | 7 | 0 | 0 | | ||
| | Script | 2 | 2 | 0 | 0 | | ||
| | Advanced | 5 | 5 | 0 | 0 | | ||
| | Multi-fixture | 5 | 5 | 0 | 0 | | ||
| | **TOTAL** | **159** | **158** | **1** | **1** | | ||
|
|
||
| Pass rate: **99.4%** (158/159) |
There was a problem hiding this comment.
Coverage Summary counts don’t reconcile with the listed statuses.
The summary table/pass-rate appears inconsistent with the per-test status markers (notably NOTE counts in Query commands). Please regenerate/reconcile this section to avoid stale reporting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/blackbox_test_catalog.md` around lines 279 - 291, The Coverage
Summary table is stale and the counts (e.g., the "Query commands" row and NOTE
counts) do not match the per-test status markers; regenerate the summary so the
"Query commands" count, NOTE counts, the "**TOTAL**" row and the "Pass rate"
line are recomputed from the actual per-test statuses, update any mismatched
cells (including the single failing/tested counts) and ensure Pass rate (158/159
-> percentage) is correct and consistent with the recalculated totals.
| passes_out = rdc_ok("ls", "/passes", session=vkcube_session) | ||
| pass_name = passes_out.strip().splitlines()[0].strip() |
There was a problem hiding this comment.
Avoid brittle “first pass” selection for attachment tests.
Using splitlines()[0] couples these tests to pass ordering and can fail for valid captures where the first pass lacks color0 or depth. Pick a pass by capability (contains the attachment) instead.
💡 Suggested refactor
+def _pass_with_attachment(session: str, attachment: str) -> str:
+ passes_out = rdc_ok("ls", "/passes", session=session)
+ for raw in passes_out.strip().splitlines():
+ name = raw.strip()
+ if not name:
+ continue
+ out = rdc_ok("ls", f"/passes/{name}/attachments", session=session)
+ if attachment in out:
+ return name
+ pytest.fail(f"No pass exposes attachment {attachment!r}")
+
class TestLsPassAttachments:
@@
- passes_out = rdc_ok("ls", "/passes", session=vkcube_session)
- pass_name = passes_out.strip().splitlines()[0].strip()
+ pass_name = _pass_with_attachment(vkcube_session, "color0")
out = rdc_ok("ls", f"/passes/{pass_name}/attachments", session=vkcube_session)
assert "color0" in outAlso applies to: 294-295, 312-313, 322-323, 332-333
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/test_vfs.py` around lines 276 - 277, The test currently picks the
first pass via passes_out.strip().splitlines()[0], which is brittle; instead
iterate over the pass names returned by passes_out (from rdc_ok("ls", "/passes",
session=vkcube_session)) and select a pass whose directory listing (via
rdc_ok("ls", f"/passes/{pass_name}", session=vkcube_session)) contains the
required attachment string (e.g., "color0" or "depth") before using it; update
all occurrences where pass_name is chosen (the blocks using passes_out,
pass_name) to perform this capability-based selection and fail the test if no
suitable pass is found.
Additional Comments (1)
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/test_vfs.py (1)
328-339: Consider asserting error message content for consistency.Other error tests in this file (e.g.,
TestCatNotFound,TestLsNotFound) assert that"error"appears in the output. This test only verifies the exit code. For consistency and better failure diagnostics, consider adding an assertion on the output.💡 Suggested improvement
def test_invalid_attachment(self, vkcube_session: str) -> None: passes_out = rdc_ok("ls", "/passes", session=vkcube_session) pass_name = passes_out.strip().splitlines()[0].strip() - rdc_fail( + out = rdc_fail( "cat", f"/passes/{pass_name}/attachments/color99", session=vkcube_session, exit_code=1, ) + assert "error" in out.lower()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/test_vfs.py` around lines 328 - 339, The test TestCatPassAttachmentInvalid::test_invalid_attachment currently only asserts the exit code; after calling rdc_fail(...) add an assertion that the command output contains the string "error" to match other tests (use the rdc_fail return or captured output variable) so failures give consistent diagnostics; locate the calls to rdc_ok(...) and rdc_fail(...) and assert "error" in the failure output (e.g., reference variables passes_out, pass_name, and the rdc_fail invocation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/test_vfs.py`:
- Around line 328-339: The test
TestCatPassAttachmentInvalid::test_invalid_attachment currently only asserts the
exit code; after calling rdc_fail(...) add an assertion that the command output
contains the string "error" to match other tests (use the rdc_fail return or
captured output variable) so failures give consistent diagnostics; locate the
calls to rdc_ok(...) and rdc_fail(...) and assert "error" in the failure output
(e.g., reference variables passes_out, pass_name, and the rdc_fail invocation).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
pyproject.tomltests/e2e/conftest.pytests/e2e/e2e_helpers.pytests/e2e/test_advanced.pytests/e2e/test_assert.pytests/e2e/test_debug.pytests/e2e/test_diff.pytests/e2e/test_export.pytests/e2e/test_formats.pytests/e2e/test_presession.pytests/e2e/test_query.pytests/e2e/test_session.pytests/e2e/test_shader_edit.pytests/e2e/test_vfs.py
✅ Files skipped from review due to trivial changes (3)
- tests/e2e/test_query.py
- tests/e2e/test_presession.py
- tests/e2e/test_formats.py
1 participant
User description
Summary
/draws/<eid>/pixel/directory node to VFS skeleton sordc lsandrdc treecan discover pixel history routes/passes/<name>/attachments/withcolor0,color1, ...,depthleaf nodes from pipeline state on first access/shaders/<id>/used-byreverse index returning all draw EIDs that reference a shaderDetails
Three VFS route gaps between the design doc and implementation:
Pixel directory:
pixeladded to_DRAW_CHILDREN, registered as empty dir (coordinates are user-specified, not enumerable). Two directory routes added for intermediate paths.Pass attachments: New
populate_pass_attachments()function followspopulate_draw_subtreepattern._ensure_pass_attachments_populated()trigger called in bothvfs_lsandvfs_treehandlers. Newpass_attachmenthandler returns{pass, attachment, resource_id}.Shader used-by:
_build_shader_cachenow persistsshader_eidsas"eids"inshader_meta. Newshader_used_byhandler + route + tree node.Test plan
pixi run lintcleanPR Type
Enhancement
Description
Add
/draws/<eid>/pixel/directory node to VFS skeleton for pixel history route discoverabilityLazy-populate
/passes/<name>/attachments/with color and depth leaf nodes from pipeline stateAdd
/shaders/<id>/used-byreverse index returning all draw EIDs that reference a shaderImplement handlers and routes for all three VFS gaps with comprehensive test coverage
Diagram Walkthrough
File Walkthrough
7 files
Register pixel, pass attachment, and shader used-by routesAdd pixel to draw children and populate pass attachmentsBuild shader cache with EID list and populate pass attachmentsTrigger pass attachment population in vfs_ls and vfs_treeAdd pass_attachment handler returning resource IDsAdd shader_used_by handler returning EID listAdd formatters for pass_attachment and shader_used_by responses5 files
Add route resolution tests for all three VFS gapsAdd tests for pixel dir, pass attachments, and shader used-byAdd handler tests for pass_attachment and shader_used_byAdd VFS integration tests for pixel, attachments, and shader routesAdd tests verifying shader_meta contains EID listSummary by CodeRabbit