Skip to content

feat: repoint Rust client and CLI project commands to core#1328

Merged
geoffjay merged 6 commits into
epic-1307from
issue-1311
Jun 22, 2026
Merged

feat: repoint Rust client and CLI project commands to core#1328
geoffjay merged 6 commits into
epic-1307from
issue-1311

Conversation

@geoffjay

@geoffjay geoffjay commented Jun 21, 2026

Copy link
Copy Markdown
Owner

Route all project CRUD operations (list, create, get, update, delete) in the Rust OrchestratorClient and CLI project commands to the core service, leaving association operations (add/remove agent/workflow) on the orchestrator.

Changes:

  • OrchestratorClient: add core_base_url field and with_core_url() builder; replace core_url(path) helper with url_for(path, use_core: bool); extract URL-based HTTP implementations (get_url, post_url, put_url, delete_url, delete_with_response_url) to eliminate duplication between *_core and regular wrappers; add client-side association pre-check in delete_project before deleting from core (since core doesn't enforce this constraint server-side); add unit tests for url_for fallback logic
  • CLI main.rs: wire with_core_url() for the project command arm
  • CLI project.rs: update show_project for the new Project return type (no agent_count/workflow_count)

Closes #1311

@geoffjay geoffjay added the review-agent Used to invoke a review by an agent tracking this label label Jun 21, 2026

@geoffjay geoffjay left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Review: feat: repoint Rust client and CLI project commands to core

Stack position: This PR (issue-1311) sits on top of epic-1307feature/projects-to-core (#1325). #1325 is still open, so this branch cannot merge until that parent stack is landed. issue-1312 is stacked on top of this PR and will need a restack after this merges.


⚠️ Needs attention: delete_project docstring is now incorrect — and the missing guard is a correctness concern

The method comment still says:

/// Delete a project (fails if agents or workflows are still associated).
pub async fn delete_project(&self, id: &Uuid) -> Result<()> {

But the core handler (DELETE /api/v1/projects/{id}) explicitly does not enforce this constraint:

// crates/core/src/api/projects.rs
/// Deletes the project. Core only checks its own constraints — there is no
/// cross-service delete-guard at this layer. Returns `204 No Content`.

After this PR, a project record in core can be deleted while the orchestrator still holds agent and workflow associations pointing at that project UUID — leaving orphaned references with no corresponding project.

At minimum, please fix the docstring so it accurately describes what the new endpoint does. Beyond that, the PR should either:

  • Note explicitly (here and/or in the linked issue) that the association guard is intentionally deferred, and describe where/when it will be enforced, or
  • Add a client-side pre-check in delete_project that queries list_project_agents / list_project_workflows (both still on the orchestrator) before sending the delete to core.

Non-blocking: *_core helpers duplicate ~60 lines of the existing * helpers

get_core, post_core, put_core, delete_core differ from get, post, put, delete in only one way — they call self.core_url(path) instead of format!("{}{}", self.base_url, path). Everything else (bearer-auth wiring, handle_response, error context strings) is copy-pasted.

A lightweight cleanup would be a private url_for helper:

fn url_for(&self, path: &str, use_core: bool) -> String {
    let base = if use_core {
        self.core_base_url.as_deref().unwrap_or(&self.base_url)
    } else {
        &self.base_url
    };
    format!("{base}{path}")
}

Then the private helpers each take use_core: bool and the duplication collapses. Not required for this PR, but worth a follow-up if this pattern expands to more endpoints.


Non-blocking: no unit tests for with_core_url / core_url resolution

The existing tests in client.rs only exercise base_url string conversion. The new fallback logic — "use core_base_url when set, otherwise fall back to base_url" — has no test coverage. Consider adding:

#[test]
fn test_core_url_falls_back_to_base_when_unset() {
    let c = OrchestratorClient::new("http://orch");
    assert_eq!(c.core_url("/projects"), "http://orch/projects");
}

#[test]
fn test_core_url_uses_core_base_when_set() {
    let c = OrchestratorClient::new("http://orch").with_core_url("http://core/api/v1");
    assert_eq!(c.core_url("/projects"), "http://core/api/v1/projects");
}

Looks good

  • AppState::new(storage) fix is correct. AppState has three fields (storage, pam_config, pam_verifier); the old AppState { storage } struct literal was a compile error waiting to happen once the struct gained those extra fields. Using ::new() with fail-closed defaults is right.
  • Type compatibility between core's ProjectResponse (String ids/timestamps) and the client's Project (Uuid/DateTime<Utc>) is fine — core stores UUIDs in RFC4122 format and timestamps as RFC3339 strings, both of which serde deserializes into the typed fields transparently.
  • Removing agent_count/workflow_count from show_project is consistent with get_project now returning Project instead of ProjectResponse.
  • URL wiring in main.rs is correct: orch_url routes via gateway_url("orchestrator"){core_url}/api/v1/orchestrator, while core_api_url directly targets {core_url}/api/v1. Token is correctly shared across both paths.

@geoffjay geoffjay added needs-rework PR has review feedback that must be addressed before merging and removed review-agent Used to invoke a review by an agent tracking this label labels Jun 21, 2026
@geoffjay geoffjay linked an issue Jun 21, 2026 that may be closed by this pull request
10 tasks
geoffjay and others added 2 commits June 22, 2026 09:58
feat: repoint MCP tools and UI project calls to core
- Fix delete_project docstring: core does not enforce association
  constraints server-side, so add a client-side pre-check that queries
  list_project_agents and list_project_workflows before deleting
- Replace core_url(path) helper with url_for(path, use_core: bool) to
  reduce duplication; extract URL-based implementations (get_url,
  post_url, put_url, delete_url, delete_with_response_url) that both
  *_core and regular wrappers delegate to
- Add unit tests for url_for fallback logic using a struct-literal
  url_only_client helper to avoid reqwest TLS init in test threads

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@geoffjay geoffjay added review-agent Used to invoke a review by an agent tracking this label and removed needs-rework PR has review feedback that must be addressed before merging labels Jun 22, 2026
@geoffjay geoffjay merged commit 5ff6d34 into epic-1307 Jun 22, 2026
@geoffjay geoffjay deleted the issue-1311 branch June 22, 2026 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-agent Used to invoke a review by an agent tracking this label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Repoint Rust client and CLI project commands to core

1 participant