Skip to content

Inconsistent validation across file-reading APIs #9

Open

Description

@cblanquera

Timing: Follow-up after trial/3 merges into main; defer implementation until then.

Review baseline: Local trial/3 at a094418baf6388e3eeca23821df481a7f0a8751c. This branch is not yet on the remote. Source locations below refer to that snapshot and should be rechecked after merge.

Problem

The public read_source convenience helper bypasses file-type validation performed by ProjectLoader. The same source path therefore has materially different behavior depending on the API used: one rejects a special file while the other can block opening it.

Evidence and reproduction

  • crates/idea/idea-transformer/src/lib.rs:94-103: read_source opens the supplied path directly.
  • crates/idea/idea-transformer/src/source/native.rs:108-113: ProjectLoader::load rejects special files before opening.
  • A controlled Unix probe passed the same FIFO to both APIs. ProjectLoader returned schema source is not a regular file; read_source blocked until a 2-second watchdog terminated its process.

Acceptance criteria

  • Establish consistent supported-file and bounded-acquisition policy for source-reading entry points.
  • Reject unsupported special files before potentially blocking open/read operations, and retain post-open validation where needed.
  • Preserve intended path/symlink semantics, finite byte ceilings, UTF-8 validation, and retained IO causes.
  • Verify FIFO/device and regular-file cases using bounded, platform-appropriate checks.
  • Review the bootstrap reader as a related entry point while keeping this issue focused on source API consistency.

Code examples and suggested direction

A basic cooperative-filesystem guard would look like this:

fn open_regular(path: &std::path::Path) -> std::io::Result<std::fs::File> {
    if !std::fs::metadata(path)?.is_file() {
        return Err(std::io::Error::other("source is not a regular file"));
    }
    let file = std::fs::File::open(path)?;
    if !file.metadata()?.is_file() {
        return Err(std::io::Error::other("opened source is not a regular file"));
    }
    Ok(file)
}

This is only a baseline sketch. A path can be replaced between metadata and open, so these checks do not establish race-free acquisition. Investigate supported platform-specific nonblocking/open-handle options if concurrent replacement is in scope. Preserve each caller's intended symlink policy.

Activity

  1. cblanquera commented on Oct 26, 2026

    @cblanquera
    MemberAuthor

    Suggested investigation and fix notes

    Deferred until trial/3 is merged into main. Recheck the merged source and tests first. The snippets are examples/proposals, not an approved patch or a change to frozen contracts.

    1. Compare read_source, ProjectLoader::load/read, the CLI bootstrap reader, and the output writer's read_regular. Identify shared acquisition rules without mixing source-resolution policy with output ownership policy.
    2. Decide explicitly whether the contract assumes cooperative filesystem writers or must reject a file swapped to a FIFO/device during acquisition. The description's metadata-before/open/metadata-after sketch does not close that race.
    3. Put the strongest selected policy at one clear platform boundary and reuse it where contracts match. Keep unsafe_code = forbid; assess safe platform APIs rather than inserting unchecked native calls.
    4. Preserve source-relative resolution, symlink behavior, original IO/UTF-8 causes, empty-file handling, exact byte-boundary acceptance, and one-extra-byte oversize detection.
    5. Keep Unix FIFO tests in a child process with a watchdog. Use cfg gates and platform equivalents where available; do not run an unbounded open in the main test process.

    Useful regular-file boundary example, using current APIs:

    let directory = tempfile::tempdir()?;
    let path = directory.path().join("sample.idea");
    std::fs::write(&path, "abc")?;
    assert_eq!(stackpress_idea_transformer::read_source(&path, 3)?, "abc");
    assert!(stackpress_idea_transformer::read_source(&path, 2).is_err());

    Extend tests/source_loaders.rs and coordinate bootstrap cases with #8. Start with cargo test -p stackpress-idea-transformer --test source_loaders --locked. Document any remaining filesystem-race or stalled-storage limitations honestly.

    Implementation workflow: After the merge, follow .agents/workflows/rust-programming.md: prove the affected behavior, complete the three test-verified review rounds, and record relevant formatting/lints/docs, feature/platform scope, and applicable development coverage. Preserve unrelated changes.

  2. cblanquera commented on Oct 26, 2026

    @cblanquera
    MemberAuthor

    Fixed on trial/3 in 407b4d6 + df443b7 (merged as ff7a251, task 00010 lane 2; not yet on main, so leaving open).

    • read_source now applies the same special-file policy as ProjectLoader; FIFOs and devices are rejected before a potentially blocking open, with post-open validation retained.
    • Regression failed first (the old reader accepted /dev/null and blocked on a FIFO until a 2 s watchdog killed it), now passes.

    Coordinator-verified on the combined code: fmt, Clippy -D warnings, Rustdoc -D warnings, cargo +1.96.0 check, 466 workspace tests and 857 doctests pass (467 tests on merged trial/3). Coverage: server 91.61%, CLI 93.15%, transformer 93.62%.

  3. added a commit that references this issue on Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions