Skip to content

Conversation

@dsherret
Copy link
Member

Changes NotCapable errors to NotFound -- not too sure about this tbh

Closes #31168

@coderabbitai
Copy link

coderabbitai bot commented Nov 20, 2025

Walkthrough

Adds an ignore_read permission across CLI flags, config deserialization, and runtime enforcement. Introduces ignore_read: Option<Vec<String>> on PermissionFlags and PermissionsOptions, parses --ignore-read and read.ignore from deno.json, and serializes --ignore-read in permission args. Read permissions are initialized with ignore descriptors via Permissions::new_unary_with_ignore; ignored paths are translated to I/O NotFound results (via a new ignored_to_not_found mapping) rather than permission denials. Removes check_partial from UnaryPermission<ReadDescriptor> and adds PermissionCheckError::Io(std::io::Error). Tests and spec fixtures for multiple ignore-read scenarios were added.

Sequence Diagram

sequenceDiagram
    actor User as User/CLI
    participant Arg as Arg Parser
    participant Config as Config Loader
    participant Flags as PermissionFlags
    participant Perms as Permissions (runtime)
    participant FS as File System

    User->>Arg: deno run --ignore-read=path main.ts
    Arg->>Flags: set ignore_read = ["path"]
    Config->>Flags: merge read.ignore from deno.json
    Flags->>Perms: build PermissionsOptions { ignore_read, allow/deny ... }
    Perms->>Perms: new_unary_with_ignore(read, ignore_read)

    FS->>Perms: request read /project/path
    alt path ∈ ignore_read
        Perms-->>FS: return Io(NotFound)   %% ignored -> NotFound
    else
        Perms->>Perms: evaluate allow/deny
        alt allowed
            Perms-->>FS: allow access
        else denied
            Perms-->>FS: return PermissionDenied -> mapped to NotFound
        end
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Focus areas:
    • runtime/permissions/lib.rs: new ignore_read field, removal of check_partial, ignored_to_not_found helper, PermissionCheckError::Io variant, and updated permission initialization/call sites.
    • cli/args/flags.rs and cli/args/mod.rs: parsing, serialization, and merging logic for --ignore-read and refactored permission arg builders.
    • libs/config/deno_json/permissions.rs: read type change to AllowDenyIgnorePermissionConfig and deserializer swap.
    • New tests and spec fixtures validating ignore-read behavior and NotFound semantics.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly summarizes the main feature addition of the --ignore-read permission flag.
Description check ✅ Passed Description relates to the changeset by explaining the mechanism and noting uncertainty about converting NotCapable to NotFound errors.
Linked Issues check ✅ Passed The PR fully addresses the linked issue #31168 by implementing --ignore-read to treat denied reads as NotFound instead of throwing NotCapable.
Out of Scope Changes check ✅ Passed All changes are in scope: CLI flag parsing, permission system integration, error handling refactoring, and test coverage for ignore-read functionality.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
cli/args/flags.rs (1)

984-1004: --ignore-read parsing and round‑tripping are inconsistent with other path‑based permissions

The new --ignore-read flag is wired end‑to‑end, but its value handling is inconsistent and likely surprising:

  • In permission_args_parse (Line 6713+), ignore-read values are collected directly:

    if let Some(read_wl) = matches.remove_many::<String>("ignore-read") {
      flags.permissions.ignore_read = Some(read_wl.collect());
    }

    Unlike allow_read/deny_read/allow_write/deny_write/allow_ffi/deny_ffi, these are not passed through flat_escape_split_commas. So --ignore-read=/etc,/var/log.txt becomes a single entry "/etc,/var/log.txt" instead of two paths.

  • In to_permission_args (Lines 995–1004), multiple ignore_read entries are encoded as a single flag:

    Some(ignorelist) => {
      let s = format!("--ignore-read={}", ignorelist.join(","));
      args.push(s);
    }

    With the current parser, that reconstructed CLI will not round‑trip back to the original Vec<String>; it will be seen as one element containing a comma.

  • The help text explicitly advertises comma‑separated usage:

    --ignore-read[=<<PATH>...>]            Ignore file system read access ...
    --ignore-read  |  --ignore-read="/etc,/var/log.txt"
    

    which matches how allow_read/deny_read work, but does not match actual parsing here.

This is a real behavioral discrepancy vs both other permission flags and the documented example.

Concrete fix that keeps behavior aligned with existing path‑based flags:

  • Parse ignore-read the same way as read/write/ffi:
fn permission_args_parse(
   flags: &mut Flags,
   matches: &mut ArgMatches,
 ) -> clap::error::Result<()> {
@@
-  if let Some(read_wl) = matches.remove_many::<String>("ignore-read") {
-    flags.permissions.ignore_read = Some(read_wl.collect());
-    debug!("read ignorelist: {:#?}", &flags.permissions.ignore_read);
-  }
+  if let Some(read_wl) = matches.remove_many::<String>("ignore-read") {
+    let read_wl = read_wl
+      .flat_map(flat_escape_split_commas)
+      .collect::<Result<Vec<_>, _>>()?;
+    flags.permissions.ignore_read = Some(read_wl);
+    debug!("read ignorelist: {:#?}", &flags.permissions.ignore_read);
+  }

Optionally (but recommended for clean round‑trip) you could also mirror the “one‐flag‐per‑path” style instead of joining:

-    match &self.permissions.ignore_read {
-      Some(ignorelist) if ignorelist.is_empty() => {
-        args.push("--ignore-read".to_string());
-      }
-      Some(ignorelist) => {
-        let s = format!("--ignore-read={}", ignorelist.join(","));
-        args.push(s);
-      }
-      _ => {}
-    }
+    match &self.permissions.ignore_read {
+      Some(ignorelist) if ignorelist.is_empty() => {
+        args.push("--ignore-read".to_string());
+      }
+      Some(ignorelist) => {
+        for path in ignorelist {
+          args.push(format!("--ignore-read={path}"));
+        }
+      }
+      _ => {}
+    }

This would make --ignore-read behave like --deny-read/--allow-write et al, and align with the documented "/etc,/var/log.txt" form when combined with flat_escape_split_commas.

Also applies to: 4237-4250, 6713-6733

🧹 Nitpick comments (3)
tests/specs/permission/ignore_read/flags/__test__.jsonc (1)

1-20: Flag scenarios accurately cover ignore/allow/deny interactions

The expected outputs for all, some, some_allow_env, and deny_env line up with the intended semantics: an ignored ./deno.json is surfaced as NotFound (true), while data.txt reflects whether it’s globally allowed or still denied.

If you touch this again, consider renaming the some_allow_env / deny_env keys to *_read for consistency with the feature under test, but this is non-blocking.

cli/args/flags.rs (2)

4292-4295: New --ignore-read help text is clear and aligned with the feature

The added help entries under “Permission options” for --ignore-env and --ignore-read are concise and match the intended semantics (undefined for env, NotFound error for read). Once the parsing behavior is fixed as noted above, this documentation will match runtime behavior.

Consider also mentioning in docs (outside this file) that ignore-read may change some NotCapable errors to NotFound, to make the behavioral shift explicit.


9215-9262: ignore_read tests look good; consider adding a comma‑separated case

The two new tests:

  • ignore_read_ignorelist
  • ignore_read_ignorelist_multiple

correctly cover the single‑flag and multi‑flag cases and ensure PermissionFlags.ignore_read is populated as expected.

Given the help text shows --ignore-read="/etc,/var/log.txt", it would be worthwhile to add a test for a comma‑separated invocation once the parser is fixed, to guard against regressions:

+  #[test]
+  fn ignore_read_comma_separated() {
+    let r = flags_from_vec(svec![
+      "deno",
+      "run",
+      "--ignore-read=/etc,/var/log.txt",
+      "script.ts"
+    ]);
+    assert_eq!(
+      r.unwrap().permissions.ignore_read,
+      Some(svec!["/etc", "/var/log.txt"])
+    );
+  }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e59d37f and 57344f7.

📒 Files selected for processing (9)
  • cli/args/flags.rs (8 hunks)
  • cli/args/mod.rs (4 hunks)
  • libs/config/deno_json/permissions.rs (6 hunks)
  • runtime/permissions/lib.rs (9 hunks)
  • tests/specs/permission/ignore_read/config/__test__.jsonc (1 hunks)
  • tests/specs/permission/ignore_read/config/deno.json (1 hunks)
  • tests/specs/permission/ignore_read/config/main.ts (1 hunks)
  • tests/specs/permission/ignore_read/flags/__test__.jsonc (1 hunks)
  • tests/specs/permission/ignore_read/flags/main.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
runtime/permissions/lib.rs (3)
runtime/permissions/prompter.rs (1)
  • set_prompter (87-89)
runtime/ops/permissions.rs (1)
  • from (36-47)
ext/fs/ops.rs (3)
  • from (89-99)
  • from (163-174)
  • from (2063-2096)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: test debug macos-aarch64
  • GitHub Check: test debug linux-x86_64
  • GitHub Check: test release linux-x86_64
  • GitHub Check: test debug linux-aarch64
  • GitHub Check: test debug windows-x86_64
  • GitHub Check: test debug macos-x86_64
  • GitHub Check: lint debug macos-x86_64
  • GitHub Check: lint debug linux-x86_64
  • GitHub Check: lint debug windows-x86_64
🔇 Additional comments (9)
tests/specs/permission/ignore_read/config/deno.json (1)

1-9: Config shape matches new read.ignore semantics

permissions.default.read.ignore: true cleanly maps to the new AllowDenyIgnorePermissionConfig and will mark all reads in this set as ignored (treated as NotFound) without touching allow/deny. Looks consistent with the intended behavior for the tests.

tests/specs/permission/ignore_read/config/main.ts (1)

1-6: Targeted probe for NotFound mapping looks correct

This script is minimal and does exactly what the config test needs: distinguish a successful read from a denied-but-ignored read via err instanceof Deno.errors.NotFound. The behavior lines up with the read.ignore semantics.

tests/specs/permission/ignore_read/config/__test__.jsonc (1)

1-4: Harness correctly validates the config-based ignore behavior

The args and expected output match the script: under the configured read.ignore default, the read of ./deno.json should hit the NotFound branch and print true, which this fixture asserts.

tests/specs/permission/ignore_read/flags/main.ts (1)

1-13: Two-read layout cleanly exercises per-path ignore semantics

Splitting the reads into separate try/catch blocks with NotFound checks makes it easy for the JSONC tests to assert different behaviors for ./deno.json vs data.txt under varying --ignore-read / --allow-read / --deny-read combinations.

libs/config/deno_json/permissions.rs (1)

185-187: read schema now matches env-style allow/deny/ignore behavior with good coverage

Switching PermissionsObject.read to AllowDenyIgnorePermissionConfig with deserialize_allow_deny_ignore brings read permissions in line with env: existing boolean and list forms still deserialize to allow=All/Some with deny/ignore=None, while object forms can now express an explicit ignore set.

The updated tests exercise:

  • read: true / ["test"] happy paths (ignore stays None),
  • object read with allow+deny+ignore lists,
  • and an object using a boolean allow with deny+ignore lists,

which together give solid confidence that existing configs keep working and new ignore forms are accepted as intended.

Also applies to: 296-300, 347-351, 399-407, 431-438

cli/args/flags.rs (1)

815-852: PermissionFlags.ignore_read wiring looks consistent with existing flags

Adding ignore_read: Option<Vec<String>> and including it in has_permission() mirrors the existing ignore_env behavior and fits the overall permission model. No issues here.

runtime/permissions/lib.rs (3)

3367-3388: ignore_read plumbing and new_unary_with_ignore look consistent with existing env-ignore semantics

  • Adding ignore_read to PermissionsOptions and routing it through Permissions::new_unary_with_ignore keeps read permissions aligned with the existing allow/deny/ignore model used for env.
  • Capacity calculation and global flag handling in new_unary_with_ignore mirror the previous new_unary behavior, extended to support ignore descriptors.
  • Using parse_maybe_vec(opts.ignore_read.as_deref(), parser.parse_read_descriptor) ensures ignore_read gets the same normalization as allow/deny lists, and the constructor wiring is correct.

I don't see correctness or API issues here; this is a straightforward, well-scoped extension.

Also applies to: 3406-3447, 3537-3549


6593-6682: test_read_ignore appropriately exercises allow/deny/ignore ordering for read paths

  • The test covers both simple (distinct allowed, ignored, denied) and overlapping prefix cases, confirming that more specific allow/ignore entries win over broader denies and that PermissionState::Ignored is produced where expected.
  • This directly validates the new read-ignore behavior at the UnaryPermission<ReadDescriptor>::query level and matches the intended semantics.

No additional cases seem strictly necessary here given other ordering tests already present in this module.


3682-3753: Verified: All exhaustive matches properly handle the new Io variant

The verification confirms the review approval was correct:

  • The two exhaustive match statements (kind() and into_io_error()) in the snippet already include PermissionCheckError::Io(e)
  • The additional line ranges (3894-3903, 3992-3995, 4029-4035) don't contain exhaustive matches over PermissionCheckError—they all use .map_err(ignored_to_not_found) to transform errors
  • Pattern matches in ext/os/lib.rs use guarded patterns and don't require exhaustive coverage

No missing cases found. The implementation correctly handles the new variant.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
runtime/permissions/lib.rs (1)

3682-4043: PermissionCheckError::Io and ignored_to_not_found are correctly scoped but only applied to read paths

The new PermissionCheckError::Io(std::io::Error) plus ignored_to_not_found cleanly re-map only PermissionDeniedError with state == PermissionState::Ignored to a platform-appropriate NotFound I/O error, while leaving all other permission states as PermissionDenied. kind() and into_io_error() continue to behave as callers would expect for existing variants, and Io just propagates the underlying error.

Hooking ignored_to_not_found into:

  • PermissionsContainer::check_specifier for file: URLs,
  • the read side of check_open_with_requested, and
  • check_read_all,

ensures that the main runtime entry points for read permission checks will surface ignored reads as NotFound instead of NotCapable, matching the PR’s goal, while still reporting Ignored as “denied” through the PermissionState/permissions ops path. Given PermissionsContainer is the public surface here and inner is private, this should cover all runtime read checks.

One subtle behavioral change is that these synthesized NotFound errors don’t include the original path in the std::io::Error (they’re created from raw OS errors or a generic message on wasm). That does slightly differ from real FS NotFound errors, but also helps avoid leaking ignored paths. If retaining the path in messages is important for diagnostics, a follow-up could thread a redacted or normalized path into the constructed std::io::Error.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b659308 and 774c594.

📒 Files selected for processing (1)
  • runtime/permissions/lib.rs (9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
runtime/permissions/lib.rs (4)
cli/tsc/dts/lib.deno.ns.d.ts (2)
  • Permissions (4285-4367)
  • PermissionState (4062-4062)
runtime/permissions/prompter.rs (1)
  • set_prompter (87-89)
runtime/ops/permissions.rs (1)
  • from (36-47)
ext/fs/ops.rs (3)
  • from (89-99)
  • from (163-174)
  • from (2063-2096)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: test debug linux-x86_64
  • GitHub Check: test debug linux-aarch64
  • GitHub Check: test release linux-x86_64
  • GitHub Check: test debug macos-aarch64
  • GitHub Check: test debug windows-x86_64
  • GitHub Check: test debug macos-x86_64
  • GitHub Check: lint debug linux-x86_64
  • GitHub Check: lint debug windows-x86_64
  • GitHub Check: lint debug macos-x86_64
  • GitHub Check: build libs
🔇 Additional comments (2)
runtime/permissions/lib.rs (2)

3367-3446: ignore_read option and new_unary_with_ignore wiring look consistent with existing env ignore

Adding ignore_read to PermissionsOptions and routing read permissions through Permissions::new_unary_with_ignore mirrors the existing ignore_env pattern, including global vs per-descriptor handling via global_from_option and UnaryPermissionDesc::FlagIgnored. Capacity preallocation and the way descriptors are inserted keep ordering semantics (specific-before-general, deny/ignore precedence) intact. I don’t see behavioral regressions here.


6601-6690: test_read_ignore covers core ignore/deny/allow interactions for read descriptors

The new test_read_ignore mirrors the existing env-ignore tests and validates both the simple (allowed/ignored/denied siblings) and overlapping-prefix cases, confirming that FlagIgnored descriptors win over broader denies and that allows still work when nested under a denied prefix. This gives good confidence in the ignore_read descriptor ordering and PermissionState::Ignored behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file became really hard to review :(

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but let's get more eyes on this

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
cli/args/flags.rs (2)

985-1005: to_permission_args correctly serializes --ignore-read, but consider aligning path handling

The new ignore_read branch mirrors ignore_env and will correctly round‑trip simple values. However, unlike allow-read/deny-read, it doesn't use join_paths, so comma-containing paths would not be escaped consistently.

If you want ignore-read to support the same comma‑escaping semantics as the read allow/deny lists, consider:

-    match &self.permissions.ignore_read {
-      Some(ignorelist) if ignorelist.is_empty() => {
-        args.push("--ignore-read".to_string());
-      }
-      Some(ignorelist) => {
-        let s = format!("--ignore-read={}", ignorelist.join(","));
-        args.push(s);
-      }
-      _ => {}
-    }
+    match &self.permissions.ignore_read {
+      Some(ignorelist) if ignorelist.is_empty() => {
+        args.push("--ignore-read".to_string());
+      }
+      Some(ignorelist) => {
+        let s = format!("--ignore-read={}", join_paths(ignorelist, ","));
+        args.push(s);
+      }
+      _ => {}
+    }

Not blocking, but this keeps ignore-read behavior aligned with other path‑based permissions.


9235-9282: ignore_read tests are good; consider adding a comma‑separated case

The new ignore_read_ignorelist and ignore_read_ignorelist_multiple tests cover single and repeated --ignore-read flags and validate the PermissionFlags wiring, which is great.

Once permission_args_parse is updated to split comma‑separated values, it would be worth adding a test like:

+  #[test]
+  fn ignore_read_ignorelist_commas() {
+    let r = flags_from_vec(svec![
+      "deno",
+      "run",
+      "--ignore-read=foo,bar",
+      "script.ts"
+    ]);
+    assert_eq!(
+      r.unwrap(),
+      Flags {
+        subcommand: DenoSubcommand::Run(RunFlags::new_default(
+          "script.ts".to_string(),
+        )),
+        permissions: PermissionFlags {
+          ignore_read: Some(svec!["foo", "bar"]),
+          ..Default::default()
+        },
+        code_cache_enabled: true,
+        ..Flags::default()
+      }
+    );
+  }

This will lock in the expected multi‑path behavior that the help text advertises.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 774c594 and 805186e.

📒 Files selected for processing (2)
  • cli/args/flags.rs (8 hunks)
  • cli/args/mod.rs (4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
cli/args/flags.rs

📄 CodeRabbit inference engine (CLAUDE.md)

CLI flag parsing should be defined in cli/args/flags.rs

Files:

  • cli/args/flags.rs
**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or use lldb directly
Use eprintln!() or dbg!() macros for debug prints in Rust code

Files:

  • cli/args/flags.rs
  • cli/args/mod.rs
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to runtime/permissions.rs : Permission system is implemented in `runtime/permissions.rs`
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to runtime/permissions.rs : Permission system is implemented in `runtime/permissions.rs`

Applied to files:

  • cli/args/flags.rs
  • cli/args/mod.rs
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to cli/args/flags.rs : CLI flag parsing should be defined in `cli/args/flags.rs`

Applied to files:

  • cli/args/flags.rs
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Use `DENO_LOG=debug` or `DENO_LOG=<module>=debug` environment variable for verbose logging during development

Applied to files:

  • cli/args/flags.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: test debug macos-x86_64
  • GitHub Check: test debug linux-aarch64
  • GitHub Check: test debug macos-aarch64
  • GitHub Check: test release macos-x86_64
  • GitHub Check: lint debug linux-x86_64
  • GitHub Check: test debug linux-x86_64
  • GitHub Check: test debug windows-x86_64
  • GitHub Check: lint debug macos-x86_64
  • GitHub Check: build libs
  • GitHub Check: lint debug windows-x86_64
🔇 Additional comments (3)
cli/args/flags.rs (1)

816-853: New ignore_read flag plumbing in PermissionFlags looks consistent

Adding ignore_read: Option<Vec<String>> and including it in PermissionFlags::has_permission() mirrors how ignore_env is handled and integrates cleanly with Flags::has_permission(). No functional issues spotted here.

cli/args/mod.rs (2)

1680-1684: ignore_read wiring matches existing fs permission semantics

Using handle_deny_or_ignore plus &make_fs_config_value_absolute for ignore_read mirrors deny_read/allow_read behavior and ensures config-relative paths are resolved against the config file directory. This is the right place and shape for threading --ignore-read and read.ignore into PermissionsOptions.


1818-2045: Tests correctly exercise read.ignore config and ignore_read defaults

test_flags_to_permission_options now:

  • Builds PermissionsObject.read as AllowDenyIgnorePermissionConfig with an ignore entry, and
  • Asserts PermissionsOptions.ignore_read contains the config-dir‑absolute read-ignore path in the first case, and stays None when allow_read is supplied alongside permissions.all: true in the second.

This gives good coverage for both config path resolution and flag‑overrides behavior for ignore_read.

Comment on lines +4239 to +4252
let make_deny_ignore_read_arg = |arg: Arg| {
let mut arg = arg
.num_args(0..)
.action(ArgAction::Append)
.require_equals(true)
.value_name("PATH")
.long_help("false")
.value_hint(ValueHint::AnyPath)
.hide(true);
if let Some(requires) = requires {
arg = arg.requires(requires)
}
arg
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

--ignore-read parsing doesn’t split comma‑separated paths (behavior mismatch with docs and other read flags)

Right now ignore-read values are collected as raw strings:

  • deny-read goes through flat_escape_split_commas(...) to support --deny-read=/etc,/var/log.txt with the same escaping rules as --allow-read.
  • ignore-read uses the same Arg shape (make_deny_ignore_read_arg) but in permission_args_parse it just does read_wl.collect(), so --ignore-read=/etc,/var/log.txt becomes a single entry "/etc,/var/log.txt".

Given the help text and example:

  • --ignore-read[=<<PATH>...]]
  • --ignore-read="/etc,/var/log.txt"

this is surprising and can lead to ignore-read silently not matching what users expect.

To align semantics with the other read permissions and the documented example, I recommend:

 fn permission_args_parse(
   flags: &mut Flags,
   matches: &mut ArgMatches,
 ) -> clap::error::Result<()> {
@@
   if let Some(read_wl) = matches.remove_many::<String>("deny-read") {
     let read_wl = read_wl
       .flat_map(flat_escape_split_commas)
       .collect::<Result<Vec<_>, _>>()?;
     flags.permissions.deny_read = Some(read_wl);
   }
 
-
-  if let Some(read_wl) = matches.remove_many::<String>("ignore-read") {
-    flags.permissions.ignore_read = Some(read_wl.collect());
-    debug!("read ignorelist: {:#?}", &flags.permissions.ignore_read);
-  }
+  if let Some(read_wl) = matches.remove_many::<String>("ignore-read") {
+    let read_wl = read_wl
+      .flat_map(flat_escape_split_commas)
+      .collect::<Result<Vec<_>, _>>()?;
+    flags.permissions.ignore_read = Some(read_wl);
+    debug!("read ignorelist: {:#?}", &flags.permissions.ignore_read);
+  }

With this, --ignore-read=/etc,/var/log.txt behaves like the help suggests and matches how allow-read/deny-read are parsed.

Also, if you adopt this, combining it with the join_paths tweak in to_permission_args() will keep round‑tripping consistent.

Also applies to: 4294-4297, 4346-4347, 6744-6747

🤖 Prompt for AI Agents
In cli/args/flags.rs around lines 4239-4252 (and similarly at 4294-4297,
4346-4347, 6744-6747), the --ignore-read flag values are being collected as raw
strings which preserves comma-separated lists as a single entry; update the
parsing to use the same comma-splitting logic as deny-read by piping the
iterator through flat_escape_split_commas(...) (or otherwise splitting with the
same escaping rules used for allow-/deny-read) before collecting so that
`--ignore-read=/etc,/var/log.txt` produces two paths; apply the same change at
the other cited locations and ensure any downstream code that expects joined
paths is kept consistent (e.g., keep the join_paths tweak in
to_permission_args()).

@dsherret dsherret merged commit 99a8dbf into denoland:main Dec 1, 2025
20 checks passed
@dsherret dsherret deleted the feat_ignore_read branch December 1, 2025 18:58
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.

Allow Processing Continuation with 'Not Found' Status Instead of Throwing NotCapable

2 participants