Skip to content

Add dela deny#167

Merged
aleyan merged 5 commits into
mainfrom
deny
May 23, 2026
Merged

Add dela deny#167
aleyan merged 5 commits into
mainfrom
deny

Conversation

@aleyan
Copy link
Copy Markdown
Owner

@aleyan aleyan commented May 21, 2026

Summary by CodeRabbit

  • New Features

    • Added a CLI command to deny/block specified tasks with name resolution and ambiguity handling; updates the allowlist and prints clear success/error messages.
    • Allowlist now supports task-scoped deny entries (deny applies only to listed task names when scoped).
  • Tests

    • Added unit and integration tests (including shell integration) for deny behavior, ambiguity handling, and allowlist effects; improved test environment isolation to restore working directory.
  • Documentation

    • Marked the deny task item as completed in project docs.
  • Chores

    • Updated baseline metrics/report entries.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 21, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1c30a149-c342-47c1-9ab8-1405b8bb289c

📥 Commits

Reviewing files that changed from the base of the PR and between 8647107 and 0824008.

📒 Files selected for processing (1)
  • tests/docker_noinit/test_noinit.sh

📝 Walkthrough

Walkthrough

Adds a new dela deny CLI subcommand, implements commands::deny::execute to mark matched tasks as denied in the allowlist (with clear errors for no/ambiguous matches), provides unit tests and integration coverage, updates TestEnvGuard behavior, and records metric and project-plan updates.

Changes

Implement dela deny command

Layer / File(s) Summary
CLI command definition and dispatcher wiring
src/main.rs, src/commands/mod.rs
Adds Commands::Deny { task: String } to the CLI, declares pub mod deny;, and extends run_command to dispatch to commands::deny::execute(&task).
Deny command implementation and unit tests
src/commands/deny.rs
Implements execute(task_name) to discover tasks, match by name or disambiguated variant, apply AllowScope::Deny for single matches, and return specific errors for zero or multiple matches. Includes TestEnvGuard scaffolding and tests for success, nonexistent task, uninitialized config, and ambiguous names.
TestEnvGuard working-directory restore
src/commands/allow.rs
TestEnvGuard now records the original working directory and attempts to restore it on Drop to improve test isolation around CWD changes.
Allowlist deny matching and tests
src/allowlist.rs
Deny evaluation updated to support task-scoped deny entries; allowlist_entry_for_task now populates tasks for AllowScope::Deny; unit test added for task-scoped denies.
Integration shell test for deny command
tests/docker_noinit/test_noinit.sh
Test 32 exercises dela deny end-to-end: denies a task, verifies a Deny-scoped allowlist entry, ensures dela allow-command is blocked for denied tasks, and asserts failures for nonexistent and ambiguous task inputs.
Metrics baseline and project documentation updates
cargo_crap_baseline.json, dev_docs/project_plan.md
Adjusts recorded metrics for run_command, main, and evaluate_task_against_allowlist, adds baseline metrics for commands::deny::execute, and marks DTKT-110 (dela deny) completed in the project plan.

Possibly related PRs

  • aleyan/dela#166: Both PRs wire new CLI subcommands in src/main.rs/src/commands/mod.rs and touch allowlist logic in src/allowlist.rs (including evaluate_task_against_allowlist and allowlist entry fields).
  • aleyan/dela#164: Related changes to run_command CLI dispatch and command execution wiring.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 I hopped through code and found the task,
I nudged it "Deny" and wrote it fast,
Temp dirs tidied, tests ran true,
The allowlist sighed — a rabbit's cue,
Carrots aplenty for CI's pass.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Add dela deny' directly and clearly describes the main change: introducing a new dela deny command feature across multiple files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch deny

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.

❤️ Share

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

Copy link
Copy Markdown

@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

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/commands/deny.rs`:
- Around line 52-85: TestEnvGuard currently doesn't restore the process CWD, so
capture the original current working directory when creating a TestEnvGuard
(e.g., add a field original_cwd: PathBuf set in new()/new_uninitialized()) and
then in the Drop implementation call
std::env::set_current_dir(&self.original_cwd) before calling
reset_to_real_environment() to ensure the process CWD is restored and no temp
dirs remain as the current working directory.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 91d41f27-32bb-454b-b7e3-fee80170f0c8

📥 Commits

Reviewing files that changed from the base of the PR and between 515d1cc and db3d465.

📒 Files selected for processing (6)
  • cargo_crap_baseline.json
  • dev_docs/project_plan.md
  • src/commands/deny.rs
  • src/commands/mod.rs
  • src/main.rs
  • tests/docker_noinit/test_noinit.sh

Comment thread src/commands/deny.rs
Copy link
Copy Markdown

@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: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/commands/allow.rs`:
- Line 60: The fallback for capturing the working directory uses "." which is
relative and can leak CWD across tests; replace the unwrap_or_else fallback for
original_cwd (and the similar capture at line 70) with a deterministic absolute
path (for example PathBuf::from("/") or std::env::temp_dir()) so that when
env::current_dir() fails you store a known absolute location instead of ".";
update both the original_cwd assignment and the other capture at line 70 to use
that deterministic absolute fallback.

In `@src/commands/deny.rs`:
- Line 60: The code captures original_cwd with
env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), which uses a relative
"." fallback that can be resolved later and leak the process CWD; replace that
relative fallback with a deterministic absolute fallback (e.g.,
PathBuf::from("/") ) or preferably propagate the error instead of silently
falling back; update the two occurrences (the original_cwd binding and the
similar call at the other occurrence) to use an absolute PathBuf fallback or
return Err so the captured cwd is never a relative ".".
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1c00cc66-736d-46bc-ba49-eb131b702aaa

📥 Commits

Reviewing files that changed from the base of the PR and between db3d465 and 33f3ec8.

📒 Files selected for processing (3)
  • cargo_crap_baseline.json
  • src/commands/allow.rs
  • src/commands/deny.rs

Comment thread src/commands/allow.rs Outdated
Comment thread src/commands/deny.rs Outdated
Copy link
Copy Markdown

@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

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/docker_noinit/test_noinit.sh`:
- Around line 915-933: The test currently masks command failures with "|| true"
and only greps output; remove "|| true" from both dela allow-command invocations
(the ones assigning output and output_other) and assert their exit statuses
explicitly: for "dela allow-command another-task" verify it exited non-zero AND
its combined output contains "was denied by the allowlist"; for "dela
allow-command print-args" verify it exited zero AND its output does NOT contain
that denial message. Use the exit code from the command (or "$?") after each
invocation and fail the test if the exit code/message expectations are not met.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 12846d15-e3b7-44c5-b189-620dad762185

📥 Commits

Reviewing files that changed from the base of the PR and between ff38504 and 8647107.

📒 Files selected for processing (4)
  • cargo_crap_baseline.json
  • src/allowlist.rs
  • src/commands/deny.rs
  • tests/docker_noinit/test_noinit.sh

Comment thread tests/docker_noinit/test_noinit.sh
@aleyan aleyan merged commit 03a2418 into main May 23, 2026
9 checks passed
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