Skip to content

fix(cli): match intercept args after global options - #1344

Merged
SequeI merged 2 commits into
nolabs-ai:mainfrom
dpersek:codex/nono-1328-intercept-argv-matcher
Jul 6, 2026
Merged

fix(cli): match intercept args after global options#1344
SequeI merged 2 commits into
nolabs-ai:mainfrom
dpersek:codex/nono-1328-intercept-argv-matcher

Conversation

@dpersek

@dpersek dpersek commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Linked Issue

Closes #1328

Summary

  • Match intercept rule args as a contiguous sequence within argv[1..], so command-global options before a subcommand do not bypass a configured intercept action.
  • Add regression coverage for git -c foo=bar push --force against an intercept rule for ["push", "--force"].
  • Update the Rust docs, profile schema descriptions, and Tool Sandbox guide from strict-prefix wording to contiguous argument-sequence wording.

Agent Disclosure

This PR was prepared with assistance from an AI coding agent and reviewed by the human contributor before submission.

Relevant files and sections consulted:

  • AGENTS.md Coding Agent Contribution Policy, coding standards, and security considerations.
  • NOGENT.md tool-sandbox/security review guidance.
  • SECURITY.md responsible disclosure guidance.
  • .github/pull_request_template.md PR and agent-compliance requirements.
  • crates/nono-cli/src/tool-sandbox/policy.rs intercept and invocation policy matching code.
  • crates/nono-cli/src/command_policy.rs InterceptRuleConfig docs and validation context.
  • docs/cli/features/tool-sandbox.mdx Tool Sandbox intercept documentation.
  • crates/nono-cli/data/nono-profile.schema.json profile schema descriptions.

Compliance confirmation: this change is scoped to the existing public issue, follows the repository's coding and security requirements for the affected area, avoids forbidden unwrap/expect patterns, and does not change path handling or NonoError behavior.

Test Plan

  • cargo test -p nono-cli resolve_intercept_action
  • cargo fmt --all -- --check
  • make test-cli
    • Note: a managed-sandbox run failed in unrelated PTY/proxy tests with Unix-socket Operation not permitted; the same command passed when rerun outside that sandbox.
  • make clippy
  • git diff --check

Scope / Caveat

This changes intercept args matching from strict index-0 prefix matching to command-agnostic contiguous sequence matching within argv[1..]. It intentionally does not add command-specific option parsing.

Checklist

  • An issue exists and is linked above
  • All commits are signed-off, using DCO
  • All new code follows the project's coding standards (CLAUDE.md) and is covered by tests
  • Public-facing changes are paired with documentation updates
  • Release note has been added to CHANGELOG.md if needed

Agent Compliance Check (Required for AI/Automated PRs)

  • I am not prohibited from contributing under this policy
  • An issue already exists
  • I disclosed that I am an agent in the issue discussion
  • I described my intent and approach in the issue discussion
  • I reviewed repository coding and security rules for the affected area
  • I provided required attribution for reused or adapted code
  • I did not use forbidden patterns such as unwrap/expect
  • I used NonoError where required
  • I validated and canonicalized all relevant paths
  • This PR matches the approved or disclosed issue scope

Signed-off-by: Dustin Persek <dustin.persek@protonmail.com>
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

PR Review Summary

Size

Metric Value
Lines added +71
Lines removed -12
Total changed 83
Classification Medium (50–300 lines)

Affected crates

  • crates/nono-cli — CLI changes. Verify argument parsing, flag documentation, and UX behaviour across supported platforms.

Blast radius — Moderate

This PR touches: source code,configuration / policy files


Updated automatically on each push to this PR.

@dpersek dpersek changed the title Harden intercept arg matching after command-global options fix(cli): match intercept args after global options Jul 6, 2026
@dpersek
dpersek marked this pull request as ready for review July 6, 2026 02:56
@github-actions github-actions Bot added the bug Something isn't working label Jul 6, 2026

@nogent-nolabs-ai nogent-nolabs-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nogent code review

No blocking issues; 0 findings.

Findings: none flagged in scope.

Automated code + security review. CI already covers clippy, rustfmt, tests, cargo-audit and commit-lint.

@SequeI SequeI left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left some review comments, thank you for the PR!

Comment thread crates/nono-cli/src/tool-sandbox/policy.rs Outdated
Comment thread docs/cli/features/tool-sandbox.mdx Outdated
Signed-off-by: Dustin Persek <dustin.persek@protonmail.com>

@SequeI SequeI left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, thank you! Welcome to the community :)

@SequeI
SequeI merged commit 4216162 into nolabs-ai:main Jul 6, 2026
15 checks passed
@kipz

kipz commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Nice fix for the -c foo=bar push --force bypass — contiguous-window matching does close that specific gap.

args is a literal-string list matched against argv, and the space of argv shapes that produce the "same" effective invocation while dodging that literal comparison is large — global options before the subcommand (this bug), --flag=value vs --flag value, short vs. long flag aliases (-f vs --force), repeated/duplicate flags, flag order permutations, etc. Each of these is a plausible follow-up bug report against whatever positional/contiguity assumption the matcher currently makes. Patching the comparator bypass-by-bypass will keep finding new gaps.

There's also a whole separate class around environment variables. Two different shapes:

  • Out-of-band env vars that change behavior without appearing in argv at all — e.g. GIT_SSH_COMMAND, or a bare FOO=bar git push prefix assignment the shell strips before exec. A rule keyed only on argv has no visibility into these.
  • Env var references inside an argv token — e.g. git $FOO push where $FOO expands to --force. If the matcher inspects argv before shell expansion happens, it sees the literal string $FOO, not --force, so a rule looking for --force never fires even though that's exactly what gets executed. The matcher's view of the command and what actually runs can diverge.

Might be worth treating this as "the matcher needs richer predicate matching" (and possibly needs to observe post-expansion argv, not pre-expansion) rather than a series of point fixes — each gap keeps showing up as its own bypass report otherwise.

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

Labels

bug Something isn't working nono-cli size/medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Intercept rule matching uses hand-rolled strict-prefix argv comparison, bypassable via global flags before subcommand

3 participants