Skip to content

fix(cli): harden project resolution (stacked on #137) - #138

Open
juangaitanv wants to merge 4 commits into
juan/cor-1577-resolve-by-repofrom
juan/cor-1577-harden-resolution
Open

fix(cli): harden project resolution (stacked on #137)#138
juangaitanv wants to merge 4 commits into
juan/cor-1577-resolve-by-repofrom
juan/cor-1577-harden-resolution

Conversation

@juangaitanv

Copy link
Copy Markdown
Contributor

Stacked on #137 — review that first; this diff is against it, not main.

#137 fixes COR-1577 with the smallest thing that works: one page of /projects, a whole-path re-check, and the legacy name as fallback. This PR is everything that came out of review afterwards — the ways that resolver can still answer with the wrong project, plus the argument and test-harness cleanups. None of it is needed for the ticket, which is why it is separate.

Four commits, each independently revertible

1aa8227 — argument structs. wait::run took five positional params with repo and project_id adjacent and same-typed, so transposing them at the scan.rs call sites compiled silently and changed resolution. list::run took nine behind #[allow(clippy::too_many_arguments)]. Now ProjectSelector { name, repo } plus WaitArgs/ListArgs; the allow is gone rather than institutionalized.

0453070 — the resolver can answer wrong in three ways. Each ends in the legacy-name fallback listing someone else's scans:

  • /projects is @paginated(default_page_size=20, max_page_size=50) over a repo_url__icontains filter ordered -created_at. Enough acme/api-* siblings pushed the exact acme/api off page 1. Now page_size=50 and a walk to the last page; stopping early at the ceiling is an error, not a miss, because the search was truncated.
  • A 200 that does not parse, or omits projects entirely ({"status":"error"}), read as "no matches". @paginated emits that key on every 200 including the empty case, so both now fail the parse.
  • Two projects can share a path across forges. The host settles it — as a tie-breaker, never a gate: a lone path match is accepted whatever its host, so an SSH-config alias origin still resolves. (This repository's own remote is git@github.com-corgea:Corgea/cli.git; a host gate would have broken exactly the setups the ticket targets.) Several matches with none on our host errors and names the competing URLs.

1bcdb74 — three user-facing defects.

  • get_scan_issues interpolated the project name into the query string, so --project-name 'foo&bar' sent ?project=foo&bar&page=1 and the server read foo, returning another project's issues under the name asked for. Built with query now.
  • --project-id exposes the id wait already took internally from an upload response; paired with a scan id it skips resolution entirely, which is what a CI job passing the id between steps wants. clap requires the scan id — without one the scan is still chosen by name, so a lone --project-id would only relabel the link.
  • --project-name/--repo were accepted with --sca-issues and silently ignored. list_sca_issues does take project, so the resolved name is threaded in. Only an explicit selector scopes it; unflagged --sca-issues still returns the company-wide latest scan, because narrowing that default is a separate decision.

d277030 — shared e2e stub harness. One common::Routes table replaces the near-duplicate stubs in both suites. Scaffolding only; no assertion changed.

Related

COR-1729 asks the backend for project_id and exact repo_url matching on the list endpoints. If it lands, the CLI can delete both the pagination walk and the path re-check.

Verification

cargo build, cargo clippy --all-targets (clean), cargo fmt --check, full suite under CI=1 GITHUB_ACTIONS=true — 548 tests.

Test added 4 commits July 29, 2026 15:48
`wait::run` took five positional params, four of them Option<String>;
`repo` and `project_id` were adjacent and same-typed, so transposing them
at the scan.rs call sites compiled silently and changed resolution.
`list::run` took nine and carried #[allow(clippy::too_many_arguments)].

- `ProjectSelector { name, repo }` replaces the two Option<&str> resolver
  params, so the pair travels as one value.
- `WaitArgs`/`ListArgs` name every argument at the call sites; the
  too_many_arguments allow is gone rather than institutionalized.
- list::run now owns its arguments, so two `is_some()` + `unwrap()` pairs
  clippy started flagging become `if let Some(id)`.
Three ways the single-page resolver could quietly answer with the wrong
project, all of which end in the legacy-name fallback listing someone
else's scans:

- `/projects` is `@paginated(default_page_size=20, max_page_size=50)` over
  a `repo_url__icontains` filter ordered `-created_at` (doghouse
  api/views/core.py:1640-1683, api/decorators.py:158). Enough `acme/api-*`
  siblings pushed the exact `acme/api` off page 1. Now requests
  page_size=50 and walks until an exact match or the last page; stopping
  early at PROJECTS_MAX_PAGES is an error, not a miss, since the search was
  truncated.
- A 200 whose body does not parse, or which omits `projects` entirely
  (`{"status":"error"}`), read as "no matches". `@paginated` emits the key
  on every 200 including the empty case, so both now fail the parse.
- Two projects can share a path across forges. The host settles it — but as
  a tie-breaker, never a gate: a lone path match is accepted whatever its
  host, so an SSH-config alias origin (`corp-github:org/repo`, the shape
  this repository's own remote uses) still resolves. Several matches with
  none on our host errors and names the competing URLs.
… selector

- `get_scan_issues` interpolated the project name straight into the query
  string, so `--project-name 'foo&bar'` sent `?project=foo&bar&page=1` and
  the server read the project as `foo` — returning another project's issues
  under the name the caller asked for. Built with `query` now, like
  `query_scan_list` and `get_sca_issues`.

- `--project-id` exposes the id `wait` already accepted internally from an
  upload response. Paired with a scan id it skips resolution entirely,
  which is what a CI job passing the id between steps wants. clap requires
  the scan id: without one the scan is still picked by the resolved name,
  so a lone --project-id would only relabel the link.

- `--project-name`/`--repo` were accepted with `--sca-issues` and then
  ignored: the SCA branch returns before any resolution and
  `get_sca_issues` sent only pagination, so a script asking for one project
  silently got company-wide findings. `list_sca_issues` does take `project`
  (doghouse api/views/core.py:1179-1195), so the resolved name is threaded
  in. Only an explicit selector scopes it — unflagged `--sca-issues` keeps
  returning the company-wide latest scan, since narrowing that default is a
  separate decision.
`spawn_stub` was a near-duplicate across list_resolution.rs and
wait_resolution.rs, and the two files hand-rolled the same route matching.

`common::Routes` is one table for every endpoint the two suites stub; an
unset field 404s, so "this endpoint is never dialed" tests just leave it
out. Tests needing an arm outside the table (blocking rules, the scan
upload) match it first and delegate to `Routes::answer`.

Scaffolding only: no assertion changed.

@cursor cursor 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.

No actionable findings. I verified the merge-base diff against the stacked base and traced the changed list/wait/scan call sites, /projects pagination and fail-closed parsing, host/path disambiguation, query encoding, SCA selector behavior, --project-id clap/runtime semantics, and the shared E2E stubs. The focused resolver tests cover the new wrong-project and invalid-response paths, and the PR's rust-tests plus all platform build checks pass. I found no concrete reason to block this merge.

Open in Web View Automation 

Sent by Cursor Automation: pr-flow

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