Skip to content

Collect attached proxies without overwriting archive media - #1359

Open
sanyamsingh001 wants to merge 4 commits into
storytold:mainfrom
sanyamsingh001:fix/collect-proxies-r48
Open

sanyamsingh001 wants to merge 4 commits into
storytold:mainfrom
sanyamsingh001:fix/collect-proxies-r48

Conversation

@sanyamsingh001

@sanyamsingh001 sanyamsingh001 commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Collect Files preserves attached footage and composition proxies in the collected project, while retaining the live project's paths and proxy settings. Originals are collected before proxies with one shared destination reservation set. When choosing media filenames, Collect Files skips destinations reported as existing or readable by the service. This change does not claim atomic protection against concurrent destination writers.

This conflict repair retains the document-path APIs and upstream footage-check expectations from the selected 2d805d1 baseline. It explicitly includes prerequisite #1776 at 2de015f: the HexTile centerline correction and narrowly scoped test-service mutex allowance required by strict CI. The Collect change and its engine and lifecycle regressions touch three additional files.

The original full repository CI passed on this exact composed source: formatting, strict release all-targets Clippy, 3,536 native tests and 2 doctests passed, with 31 native tests ignored and no failures. All three Collect regressions passed. PDF backend policy, layering, assets (39 attributed), and the 32-crate Wasm compilation check passed. The Wasm check retained the non-fatal pointer_wrap::State.disabled warning; recorded Mesa diagnostics remain part of the evidence.

Validation ran on Linux with Mesa software-rendering settings and EFFECTCRAFT_REQUIRE_GPU=1, on the preserved 2d805d1-based source plus #1776. The separate source-compatibility review through f36b2b3 does not establish runtime validation of latest main. These results do not establish other-platform, other-backend, universal capability, GUI, or Wasm-runtime coverage. Earlier C2 CI56 and #1776 results remain attributed to their own sources.

AI-generated with Codex; implementation and review used GPT-6.1 Sol and GPT-6 Astra agents.

Summary by CodeRabbit

  • New Features
    • File collection now includes attached proxy footage as well as original footage, while keeping the active project and source files unchanged.
  • Bug Fixes
    • Collected files are assigned distinct destinations, avoiding overwrites when destination files already exist or sources share a name.
    • Collected projects retain their footage paths and can continue rendering after original files are removed.
    • Adjusted pixel rounding along hex-tile centerlines.

sanyamsingh001 and others added 4 commits October 14, 2026 07:54
Collect attached footage and composition proxies after ordinary footage.
Reserve occupied media destinations and atomic-write temporary paths.
Honor readable service entries when existence checks miss them.
Cover proxy inclusion and source preservation with native and memory tests.

Co-authored-by: Codex <codex@openai.com>
Reconcile collection with the current exclusive temporary-file writer while retaining scoped preservation regressions.

Co-authored-by: Codex <codex@openai.com>
Evaluate the third cube coordinate symmetrically to retain the intended centerline tie. Keep the existing pixel oracle and tolerance.

Scope the Mem fixture mutex unwrap allowance so the original strict CI can validate this base.

Co-authored-by: Codex <codex@openai.com>
Retain attached proxies, document-path APIs, shared destination reservations and existing-or-readable destination checks. Preserve the current upstream footage-check expectations.

Include published HexTile prerequisite PR1776 and its scoped test-fixture lint allowance.

Co-authored-by: Codex <codex@openai.com>
@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: f0034129-b629-4055-93c1-f2a020e1d3e2
📥 Commits

Reviewing files that changed from the base of the PR and between 2d805d1 and b99dc45.

📒 Files selected for processing: 5
  • crates/automation/tests/qa/lifecycle.rs
  • crates/engine/src/commands/file_more.rs
  • crates/engine/src/tests_proxy.rs
  • crates/gpu/src/shaders/fx_pixel2.wgsl
  • crates/script/src/tests_services.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

The pull request updates file.collectFiles to archive attached proxies and avoid occupied destinations. It adds collection and rendering tests, changes the fp2_hextile coordinate calculation, and adjusts a script test fixture’s Clippy handling.

Changes: File Collection

Layer / File(s) Summary
Copy originals and proxies
crates/engine/src/commands/file_more.rs
collect_files checks destinations for existing or readable files. It processes original footage before attached proxies, copies both, and updates the collected paths.
Validate collection behavior
crates/engine/src/tests_proxy.rs, crates/automation/tests/qa/lifecycle.rs
Regression and QA tests check proxy collection, occupied destinations, preserved project and session state, and rendering after source files are removed.

Hextile Coordinate Calculation

Layer / File(s) Summary
Calculate the third coordinate
crates/gpu/src/shaders/fx_pixel2.wgsl
fp2_hextile calculates ccy directly from u and v before rounding and cell selection.

Script Test Fixture

Layer / File(s) Summary
Document fixture mutex handling
crates/script/src/tests_services.rs
A comment documents the expected test failure for a poisoned fixture mutex, and the code allows unwrap_used.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature

Suggested reviewers: bflatastic, echelon

Merge Risk: ⚪ Minimal · up to b99dc

The collection changes have no identified merge-blocking issue. Concurrent collections targeting the same archive retain a pre-existing overwrite limitation.

Security architecture risk: 🔵 Low · up to b99dc

Collection now exports more attached media while preserving the open project. No distinct increase in filesystem privileges was established compared with the previous collection behavior, but existing export-safety limits and incomplete validation keep the risk above minimal.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — For native collection, source access is bounded by the host process’s filesystem permissions, not by media decoding. Project-specified files are copied as bytes into the chosen archive. Proxy disablement does not exclude those files from export, but the same host-readable-file scope was already reachable through original-footage collection.

Security Findings and Attack Paths

  • inferred — A project author who can get a crafted project opened and collected through confined automation can route embedded source references outside the configured read roots into an allowed archive. This condition predates the PR through original footage: collection already read those paths directly, including paths marked missing. Proxy traversal adds another reference type, but no distinct expansion of maximum filesystem authority was established.

Trust Boundaries and Controls

  • observed — Automation confinement validates the explicit collection folder as a write path, including root membership and symbolic-link resolution. Explicit file.setProxy paths are checked as reads. These parameter checks do not enumerate sources already embedded in project state, and installing confinement does not replace the session’s filesystem service.

Resilience and Maintainability Implications

  • inferred — Exclusive temporary-file creation protects individual writes, but final rename can replace a destination. The occupancy check is not an atomic reservation, so concurrent collectors can still select the same initially absent filename. This pre-existing publication limitation now applies to proxy copies too; sequential collision protection should not be interpreted as archive-wide concurrency safety.

Hardening Proposals

  • proposed — Enforce automation read authority at the actual source-read boundary, covering both original and proxy paths and every sequence member. This would address the pre-existing embedded-reference gap rather than only checking explicit command parameters.
Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 4 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly summarizes the primary change: collecting attached proxies while preventing archive media from being overwritten.
Full details: Docstring Coverage

Explanation

Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 4 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant