feat: Replace codegen with my-clients #22

martin · 2026-08-15T20:23:51Z

martin commented

2026-08-15 20:23:51 +00:00

No description provided.

martin self-assigned this

2026-08-15 20:23:51 +00:00

martin added 1 commit

2026-08-15 20:23:53 +00:00

feat: Replace codegen with my-clients

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

Details

ci/woodpecker/push/verify Pipeline was successful

Details

d5b4f2f08a

martin requested review from clank-bot

2026-08-15 20:23:53 +00:00

clank-bot commented

2026-08-15 20:26:34 +00:00

The diff is mostly the deleted contracts JSON. Let me search for the meaningful
The bash/Grep tools are unavailable in this environment, so I'll continue reading the diff with the Read tool. Let me skip past the deleted JSON contract to the code
{
"summary": "# Review Summary\n\nThis revision of PR #22 replaces the build-time OpenAPI codegen (build.rs/openapi-gen-build), the committed contracts/forgejo.json, and the JRE/openapi-generator-cli install steps with a call into the shared my-clients Forgejo client. The cleanup is consistent across Cargo.{toml,lock}, CI, the Nix flake, and docs.\n\nBoth non-blocking items from the previous review round have been addressed:\n- reqwest is no longer a direct dependency of my-forge.\n- The Authorization header change (Bearer token {token} -> token {token}, which also fixes a latent bug since Forgejo documents the token scheme) is now exercised: the new configure_sets_forgejo_token_authorization_header test in crates/my-forge/src/integration/forgejo.rs:43 wires a mock that requires authorization: token test-token and runs repo_search through configure().\n\nOne previously-flagged concern remains open: the 17-positional-argument repo_search call (forgejo.rs:14-32) is fragile — an insert/reorder in the my-clients signature compiles cleanly but silently maps Updated/Desc/50 onto the wrong query params. The existing lib.rs tests still assert sort=updated&order=desc&limit=50 via query_param, and the new auth-header test additionally only matches method/path/header, so the mapping is only cross-checked by that one specific combination.\n\nNo security issues found; the auth-scheme fix aligns with Forgejo's documented contract. One new minor suggestion about pinning the my-clients version exactly is flagged below.",
"verdict": "changes",
"comments": [
{
"file": "Cargo.toml",
"line": 26,
"severity": "style",
"message": "version = \"0.1.0\" has caret semantics, so cargo update or a fresh lock-less build can silently resolve a newer 0.1.x whose repo_search signature differs. Because crates/my-forge/src/integration/forgejo.rs:14 calls it positionally, a parameter insert/reorder in my-clients would not fail to compile and would silently misroute sort/order/limit. Consider pinning exactly (version = \"=0.1.0\") and bumping the pin in lockstep with call sites (as AGENTS.md already advises), since the lockfile only protects --locked builds."
}
]
}

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

The diff is mostly the deleted contracts JSON. Let me search for the meaningful The bash/Grep tools are unavailable in this environment, so I'll continue reading the diff with the Read tool. Let me skip past the deleted JSON contract to the code { "summary": "# Review Summary\n\nThis revision of PR #22 replaces the build-time OpenAPI codegen (build.rs/openapi-gen-build), the committed `contracts/forgejo.json`, and the JRE/openapi-generator-cli install steps with a call into the shared `my-clients` Forgejo client. The cleanup is consistent across Cargo.{toml,lock}, CI, the Nix flake, and docs.\n\nBoth non-blocking items from the previous review round have been addressed:\n- `reqwest` is no longer a direct dependency of `my-forge`.\n- The `Authorization` header change (`Bearer token {token}` -> `token {token}`, which also fixes a latent bug since Forgejo documents the `token` scheme) is now exercised: the new `configure_sets_forgejo_token_authorization_header` test in `crates/my-forge/src/integration/forgejo.rs:43` wires a mock that requires `authorization: token test-token` and runs `repo_search` through `configure()`.\n\nOne previously-flagged concern remains open: the 17-positional-argument `repo_search` call (`forgejo.rs:14-32`) is fragile — an insert/reorder in the `my-clients` signature compiles cleanly but silently maps `Updated`/`Desc`/`50` onto the wrong query params. The existing `lib.rs` tests still assert `sort=updated&order=desc&limit=50` via `query_param`, and the new auth-header test additionally only matches method/path/header, so the mapping is only cross-checked by that one specific combination.\n\nNo security issues found; the auth-scheme fix aligns with Forgejo's documented contract. One new minor suggestion about pinning the `my-clients` version exactly is flagged below.", "verdict": "changes", "comments": [ { "file": "Cargo.toml", "line": 26, "severity": "style", "message": "`version = \"0.1.0\"` has caret semantics, so `cargo update` or a fresh lock-less build can silently resolve a newer 0.1.x whose `repo_search` signature differs. Because `crates/my-forge/src/integration/forgejo.rs:14` calls it positionally, a parameter insert/reorder in `my-clients` would not fail to compile and would silently misroute `sort`/`order`/`limit`. Consider pinning exactly (`version = \"=0.1.0\"`) and bumping the pin in lockstep with call sites (as AGENTS.md already advises), since the lockfile only protects `--locked` builds." } ] } --- *🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: **comment**

clank-bot reviewed

2026-08-15 20:26:35 +00:00

clank-bot left a comment

Code review findings from opencode.

Code review findings from opencode.

crates/my-forge/Cargo.toml Outdated

[style] `reqwest` is no longer used anywhere in `my-forge` after this migration (previously used in `configure()` for the header map and in tests via `reqwest::Client::new()`). Since `unused_crate_dependencies` is not part of the workspace lints, clippy won't flag it — consider dropping the dependency to keep the crate's deps honest.

[style] `reqwest` is no longer used anywhere in `my-forge` after this migration (previously used in `configure()` for the header map and in tests via `reqwest::Client::new()`). Since `unused_crate_dependencies` is not part of the workspace lints, clippy won't flag it — consider dropping the dependency to keep the crate's deps honest.

martin marked this conversation as resolved

crates/my-forge/src/integration/forgejo.rs

					
				@ -24,1 +8,3 @@

				    }

				    HttpClient::new()

				        .with_base_url(base_path)

				        .with_header("Authorization", format!("token {token}"))

[style] The auth header change (`Bearer token {token}` -> `token {token}`) is the core behavior change of this PR but is never exercised: all wiremock tests construct `HttpClient::new()` directly and bypass `configure()`. Note the old scheme also violated Forgejo's documented scheme ("API tokens must be prepended with `token` followed by a space"), so the new header is a genuine fix — but a test that goes through `configure()` and asserts the `Authorization` header (or has the mock require it) would protect against the shared client later defaulting to a conflicting scheme or dropping the override.

[style] The auth header change (`Bearer token {token}` -> `token {token}`) is the core behavior change of this PR but is never exercised: all wiremock tests construct `HttpClient::new()` directly and bypass `configure()`. Note the old scheme also violated Forgejo's documented scheme ("API tokens must be prepended with `token` followed by a space"), so the new header is a genuine fix — but a test that goes through `configure()` and asserts the `Authorization` header (or has the mock require it) would protect against the shared client later defaulting to a conflicting scheme or dropping the override.

martin marked this conversation as resolved

crates/my-forge/src/integration/forgejo.rs

					
				@ -100,2 +13,2 @@

				            page: None,

				        },

				pub async fn repo_search(cfg: &HttpClient) -> anyhow::Result<my_clients::forgejo::SearchResults> {

				    cfg.repo_search(

[style] 17 positional arguments (mostly `None:`) with `Some(...)` values buried at fixed indices are fragile: a reorder or insertion in the `my-clients` `repo_search` signature compiles cleanly but silently maps `RepoSearchSort::Updated`, `RepoSearchOrder::Desc`, and `Some(50)` onto the wrong query parameters (the wiremock test only catches the specific `updated/desc/50` combination). Consider pushing a params struct/builder (or named args) up into `my-clients` so call sites remain readable and signature-stable.

[style] 17 positional arguments (mostly `None:`) with `Some(...)` values buried at fixed indices are fragile: a reorder or insertion in the `my-clients` `repo_search` signature compiles cleanly but silently maps `RepoSearchSort::Updated`, `RepoSearchOrder::Desc`, and `Some(50)` onto the wrong query parameters (the wiremock test only catches the specific `updated/desc/50` combination). Consider pushing a params struct/builder (or named args) up into `my-clients` so call sites remain readable and signature-stable.

martin added 1 commit

2026-08-15 20:42:22 +00:00

Remove unused dep and tests

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

Details

ci/woodpecker/push/verify Pipeline was successful

Details

de6fdfff42