Skip to content

refactor(gateway): split policy_engine::scope into per-responsibility modules - #27

Open
marcorivm wants to merge 1 commit into
feat/org-switchingfrom
refactor/scope-modules
Open

refactor(gateway): split policy_engine::scope into per-responsibility modules#27
marcorivm wants to merge 1 commit into
feat/org-switchingfrom
refactor/scope-modules

Conversation

@marcorivm

Copy link
Copy Markdown
Member

Splits the 1,195-line policy_engine/scope.rs monolith introduced in #17. Pure reorganisation — no behaviour change.

Layout

Follows the repo's existing foo.rs + foo/ convention.

file lines seam
scope.rs 416 entry point: the pub(crate) surface, ScopeVerdict, evaluate_scope. The only place that decides which provider handles a request.
scope/session_policy.rs 125 stored JSON → typed scope. No verdict logic, no request knowledge.
scope/path_safety.rs 23 the one security predicate both extractors need.
scope/github.rs 222 URL-only extraction + exact repo match.
scope/dropbox.rs 471 body/header JSON extraction + prefix-nesting match.
scope/test_support.rs 12 the one fixture shared by two test modules.

1,195 → 1,269 lines; the +74 is entirely module docs and use statements. Largest file is now 471.

This is a move, not a rewrite — verified mechanically

I normalised both sides (strip blanks, comments, use/mod lines, visibility keywords), sorted, and diffed old against the concatenation of new:

in OLD but not NEW:  (nothing)
in NEW but not OLD:  3 closing braces, 4 #[cfg(test)] attributes

Zero logical lines were removed or altered. Not one expression, condition, or assertion changed. Tests moved between files; no assertion was touched.

The five invariants, re-checked

  1. Fail-closed — every Indeterminate return site moved verbatim; apply_resource_scope still maps both OutOfScope and Indeterminate to Blocked.
  2. Empty list → None (// empty list = all) — both arms intact, comment attached.
  3. Path traversal incl. percent-encoded — same percent_decode_str(...).decode_utf8_lossy(), with the "the forwarding layer collapses these afterwards" reasoning kept next to it.
  4. Closed provider allowlist — still a literal six-line match (provider, scope) with _ => Indeterminate. Grepped for trait/dyn/Box<: none.
  5. is_non_resource_rpc — same four matches! literals plus ends_with("/continue"), verbatim.

Restraint — what was deliberately NOT abstracted

This mattered as much as the split:

  • No trait ResourceProvider, registry, or dispatch table. Two providers behind an explicit match is right at this size, and a trait would obscure invariant 4.
  • Repo matching and folder matching left separate. repo_in_scope is exact equality after lowercasing; folder_in_scope is a segment-boundary prefix match with a root case. Superficially similar, semantically different — merging them would be a bug factory.
  • ListShape stayed private rather than being promoted to a shared string-list utility; it has one caller.
  • The query/fragment strip is still duplicated in github_repo_ref and is_non_resource_rpc — one expression, two provider modules, and hoisting it would create a cross-provider dependency worth less than the two lines saved.

Only two things were genuinely deduplicated: has_traversal/is_dot_segment (already shared, now with an obvious home) and one test fixture.

Public API

Untouched. apply_resource_scope and needs_body keep pub(crate) and their exact signatures. policy_engine.rs:42, forward.rs and websocket.rs compile unmodified — the diff against the base touches only the six scope files.

Why it's stacked here rather than folded into #17

scope.rs is touched by exactly one commit in the whole stack (#17's own), and upstream v1.45.0 doesn't touch it — so this rebased onto the tip conflict-free, and #17#26 stay exactly as reviewed. It also keeps "is this logic correct?" and "is this file organised well?" as two separate reads.

Verification

cargo test -p onecli-gateway              # 615 passed, 0 failed
cargo clippy --all-targets -- -D warnings # clean
cargo fmt -- --check                      # clean

… modules

`scope.rs` had grown to 1,195 lines covering parsing, path safety, two
provider extractors and 31 tests. Split it by responsibility, keeping
`scope.rs` as the entry point (the `pub(crate)` surface, `ScopeVerdict`,
and the closed provider dispatch) with the parts beside it:

  scope/session_policy.rs  stored session_policy -> ResourceScope
  scope/path_safety.rs     dot-segment rejection (shared by both extractors)
  scope/github.rs          repository extractor + exact matcher
  scope/dropbox.rs         folder extractor + nesting matcher
  scope/test_support.rs    the one test fixture used by two test modules

Pure move: every non-test and test code line is identical to the original
modulo `use` statements and `pub(super)` visibility (verified by
normalized diff). No trait, registry or generic abstraction was
introduced — the provider dispatch stays a literal two-arm `match` so the
closed allowlist remains visible in one place, and the repo (exact) and
folder (prefix-nesting) matchers stay separate.

Public API unchanged: `apply_resource_scope` and `needs_body` keep their
signatures, so `policy_engine.rs`, `gateway/forward.rs` and
`gateway/websocket.rs` are untouched. All 31 scope tests (585 crate-wide)
pass; clippy -D warnings and fmt clean.

Claude-Session: https://claude.ai/code/session_01VfmDPUTkYRs6NFpN89ezpM
@marcorivm
marcorivm force-pushed the refactor/scope-modules branch from ccf3b91 to 95ba251 Compare August 8, 2026 19:35
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