feat: Replace codegen with library #29

This PR replaces the in-repo Forgejo API codegen pipeline (32,700-line contracts/forgejo.json, crates/core, openapi-gen-build, the openapi-generator-cli jar + JRE in CI/dev-shell) with the pinned my-clients crate from the private Forgejo registry, collapsing the workspace to a single member and re-wiring src/integration/forgejo_integration.rs to the new client API.

Strengths

Massive net deletion (~32,700 lines) and consistent removal of the codegen toolchain from .nix/pkgs.nix, flake.nix dev shell, and .woodpecker/verify.yaml — a clear build/maintainability win. The dev shell, CI, and Nix derivations are all mutually consistent; no stale references to crates/ or contracts/ remain.
create_client faithfully ported URL construction, token injection, and owner extraction from create_configuration; the retained wiremock suite (owner-vs-author, subpath mounting, base64 config decode, empty-body 404 → Ok(None)) still exercises the real HTTP paths.
The orphaned workspace deps the earlier inline review flagged (serde_repr, tokio-util, chrono, reqwest) were in fact removed in this diff — that cleanup is done.
Lockfile pruning (reqwest feature reduction drops h2, mime, encoding_rs, system-configuration, etc.) shrinks the build graph as a side benefit.

The items already covered by earlier review (no timeout on HttpClient::new(), private-registry/Renovate blind spot, AppError::Client(String) loosing structured error detail) remain valid but non-blocking and are not repeated here.

New, non-blocking observations

map_client_error/is_not_found now key off ApiOpError::api() and the ApiError.status field, coupling 404 semantics to my-clients internals. The retained tests only exercise 404/500 responses with empty/non-JSON bodies, so the contract that api() is populated from the raw HTTP status (the case that makes a missing woodpecker.toml stay benign) is not locked in by a JSON-bodied 404 test. Worth one added test.
From<HttpError> for AppError has no call site (both methods consume ApiOpError<E>); it is dead weight that will not trigger a lint since it's a trait impl.
A fresh HttpClient (and thus a fresh reqwest connection pool + TLS handshake) is constructed per API call — two per extension request under tokio::try_join!. This predates the PR, but the new create_client seam makes caching trivial.

Verdict: approve — well-scoped refactor; the findings above are minor cleanup/verification items, not blockers.

*🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: approve

# Review Summary This PR replaces the in-repo Forgejo API codegen pipeline (32,700-line `contracts/forgejo.json`, `crates/core`, `openapi-gen-build`, the `openapi-generator-cli` jar + JRE in CI/dev-shell) with the pinned `my-clients` crate from the private Forgejo registry, collapsing the workspace to a single member and re-wiring `src/integration/forgejo_integration.rs` to the new client API. **Strengths** - Massive net deletion (~32,700 lines) and consistent removal of the codegen toolchain from `.nix/pkgs.nix`, `flake.nix` dev shell, and `.woodpecker/verify.yaml` — a clear build/maintainability win. The dev shell, CI, and Nix derivations are all mutually consistent; no stale references to `crates/` or `contracts/` remain. - `create_client` faithfully ported URL construction, token injection, and owner extraction from `create_configuration`; the retained wiremock suite (owner-vs-author, subpath mounting, base64 config decode, empty-body 404 → `Ok(None)`) still exercises the real HTTP paths. - The orphaned workspace deps the earlier inline review flagged (`serde_repr`, `tokio-util`, `chrono`, `reqwest`) were in fact removed in this diff — that cleanup is done. - Lockfile pruning (reqwest feature reduction drops `h2`, `mime`, `encoding_rs`, `system-configuration`, etc.) shrinks the build graph as a side benefit. The items already covered by earlier review (no timeout on `HttpClient::new()`, private-registry/Renovate blind spot, `AppError::Client(String)` loosing structured error detail) remain valid but non-blocking and are not repeated here. **New, non-blocking observations** - `map_client_error`/`is_not_found` now key off `ApiOpError::api()` and the `ApiError.status` field, coupling 404 semantics to `my-clients` internals. The retained tests only exercise 404/500 responses with **empty/non-JSON bodies**, so the contract that `api()` is populated from the raw HTTP status (the case that makes a missing `woodpecker.toml` stay benign) is not locked in by a JSON-bodied 404 test. Worth one added test. - `From<HttpError> for AppError` has no call site (both methods consume `ApiOpError<E>`); it is dead weight that will not trigger a lint since it's a trait impl. - A fresh `HttpClient` (and thus a fresh reqwest connection pool + TLS handshake) is constructed per API call — two per extension request under `tokio::try_join!`. This predates the PR, but the new `create_client` seam makes caching trivial. Verdict: **approve** — well-scoped refactor; the findings above are minor cleanup/verification items, not blockers. --- *🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: **approve**

clank-bot reviewed

2026-08-13 20:00:39 +00:00

clank-bot left a comment

Code review findings from opencode.

Code review findings from opencode.

Cargo.toml Outdated

[style] With the workspace collapsed to `members = ["."]` and `crates/core` deleted (its only consumer), the `[workspace.dependencies]` entries `serde_repr` (line 39), `tokio-util` (line 42), and `chrono` (line 46) now have no consumers anywhere in the tree — they are never referenced in `[dependencies]`, `[dev-dependencies]`, or source. `reqwest` in `[dependencies]` (line 73) also appears unused directly now that HTTP is handled by `my-clients`. Consider removing these orphaned entries as part of this cleanup to keep the manifest honest and reduce lock noise.

[style] With the workspace collapsed to `members = ["."]` and `crates/core` deleted (its only consumer), the `[workspace.dependencies]` entries `serde_repr` (line 39), `tokio-util` (line 42), and `chrono` (line 46) now have no consumers anywhere in the tree — they are never referenced in `[dependencies]`, `[dev-dependencies]`, or source. `reqwest` in `[dependencies]` (line 73) also appears unused directly now that HTTP is handled by `my-clients`. Consider removing these orphaned entries as part of this cleanup to keep the manifest honest and reduce lock noise.

Will do

src/integration/forgejo_integration.rs Outdated

					
				@ -49,0 +65,4 @@

				}

				fn is_not_found<E: std::fmt::Debug>(error: &my_clients::forgejo::client::ApiOpError<E>) -> bool {

				    error.api().is_some_and(|response| response.status == 404)

[style] `is_not_found` now detects 404 via `error.api().is_some_and(|r| r.status == 404)`, which depends on `my-clients` populating `api()` for the response. The previous code matched `response.status == StatusCode::NOT_FOUND` directly, so a 404 with an unparseable or empty error body (e.g. an HTML error page from a reverse proxy in front of Forgejo) still mapped to `Ok(None)`. If `my-clients` only fills `api()` when the body deserializes into its typed error model, a benign missing `woodpecker.toml` could surface as a 500 and fail the whole extension request. Worth verifying `api()` is populated from the raw HTTP status rather than requiring a parseable body.

[style] `is_not_found` now detects 404 via `error.api().is_some_and(|r| r.status == 404)`, which depends on `my-clients` populating `api()` for the response. The previous code matched `response.status == StatusCode::NOT_FOUND` directly, so a 404 with an unparseable or empty error body (e.g. an HTML error page from a reverse proxy in front of Forgejo) still mapped to `Ok(None)`. If `my-clients` only fills `api()` when the body deserializes into its typed error model, a benign missing `woodpecker.toml` could surface as a 500 and fail the whole extension request. Worth verifying `api()` is populated from the raw HTTP status rather than requiring a parseable body.

martin added 1 commit

2026-08-14 20:07:27 +00:00

Remove unused dependencies

ci/woodpecker/pr/pr-review Pipeline was successful

Details

ci/woodpecker/push/verify Pipeline was successful

Details

7378a415d6

clank-bot reviewed

2026-08-14 20:11:17 +00:00

clank-bot left a comment

Code review findings from opencode.

Code review findings from opencode.

src/integration/forgejo_integration.rs

					
				@ -49,0 +57,4 @@

				        format!("https://{machine}/api/v1")

				    };

				    Ok((

				        HttpClient::new()

[performance] `create_client` builds a fresh `HttpClient` (hence a new reqwest connection pool / TCP + TLS handshake) on every invocation. `get_workflows_for_endpoint` calls it twice per extension request via `tokio::try_join!`, and under `#[derive(Clone, Default)]`-style stateless usage there is zero connection reuse across pipelines or repos. This behavior predates the PR, but since `create_client` is now the single clear construction seam, consider hoisting the client into `ForgejoClient` (or server `Context`) so it's constructed once — it eliminates a handshake per API call with no API surface change.

[performance] `create_client` builds a fresh `HttpClient` (hence a new reqwest connection pool / TCP + TLS handshake) on every invocation. `get_workflows_for_endpoint` calls it twice per extension request via `tokio::try_join!`, and under `#[derive(Clone, Default)]`-style stateless usage there is zero connection reuse across pipelines or repos. This behavior predates the PR, but since `create_client` is now the single clear construction seam, consider hoisting the client into `ForgejoClient` (or server `Context`) so it's constructed once — it eliminates a handshake per API call with no API surface change.

src/integration/forgejo_integration.rs

					
				@ -49,0 +64,4 @@

				    ))

				}

				fn is_not_found<E: std::fmt::Debug>(error: &my_clients::forgejo::client::ApiOpError<E>) -> bool {

[style] The 404-swallowing path now depends entirely on `ApiOpError::api()` returning `Some(ApiError { status: 404, .. })`, which is an internal contract of the private-registry `my-clients` crate. The existing tests only mock 404/500 with empty/non-JSON bodies, so neither the "real" Forgejo JSON error body nor the raw-status fallback path is pinned by a test. Consider adding a JSON-bodied 404 mock (e.g. `{"message":"..."}`) to `test_missing_repository_config_returns_no_config` so a future library behavior change (e.g. `api()` only populated when the body deserializes) fails loudly instead of turning a missing `woodpecker.toml` into a 500.

[style] The 404-swallowing path now depends entirely on `ApiOpError::api()` returning `Some(ApiError { status: 404, .. })`, which is an internal contract of the private-registry `my-clients` crate. The existing tests only mock 404/500 with empty/non-JSON bodies, so neither the "real" Forgejo JSON error body nor the raw-status fallback path is pinned by a test. Consider adding a JSON-bodied 404 mock (e.g. `{"message":"..."}`) to `test_missing_repository_config_returns_no_config` so a future library behavior change (e.g. `api()` only populated when the body deserializes) fails loudly instead of turning a missing `woodpecker.toml` into a 500.

src/integration/forgejo_integration.rs

					
				@ -49,0 +68,4 @@

				    matches!(error.api(), Some(ApiError { status: 404, .. }))

				}

				impl From<my_clients::forgejo::client::HttpError> for AppError {

[style] `From for AppError` appears to be dead code: both call sites (`list_repo_contents`, `get_repo_config`) consume `ApiOpError` via the dedicated `From` impls, and `map_client_error` already converts non-`api()` errors to `AppError::Client(error.to_string())`. No path in this file produces a bare `HttpError`, and clippy won't warn on unused trait impls. Since the codegen scaffolding is being torn out, removing this impl keeps the error mapping surface exactly as wide as it is used.

[style] `From<HttpError> for AppError` appears to be dead code: both call sites (`list_repo_contents`, `get_repo_config`) consume `ApiOpError<E>` via the dedicated `From` impls, and `map_client_error` already converts non-`api()` errors to `AppError::Client(error.to_string())`. No path in this file produces a bare `HttpError`, and clippy won't warn on unused trait impls. Since the codegen scaffolding is being torn out, removing this impl keeps the error mapping surface exactly as wide as it is used.