feat: Replace codegen with my-clients #30

Now let me examine the actual repository files to verify the current state of the changed code
{
"summary": "# Review Summary\n\nThis PR replaces the in-tree Forgejo OpenAPI codegen (build.rs + contracts/forgejo.json + JRE/openapi-generator-cli) with the pre-generated my-clients crate from the private Forgejo registry, and the migration is carried through consistently: verify pipeline (JRE install removed), flake devShell, justfile codegen recipe, Nix plugin build, core/Cargo.toml feature graph, and all consumers in main.rs. The four issues from the earlier round are resolved in this diff — a 30 s timeout protects the raw diff download, the token expect panic became a PluginError, the re-export is narrowed to pub use my_clients::forgejo as forgejo_client, and the dead workspace deps (chrono, serde_repr, tokio-util, openapi-gen-build) are gone from Cargo.toml/Cargo.lock/AGENTS.md. Both token paths are now guarded (diff download via authorization_header and the primary client via validate_forgejo_token inside build_forgejo_client), and the wiremock integration tests still assert the Authorization: token <t> header plus the forbidden X-FORGEJO-OTP/Sudo/sudo headers on the new .diff endpoint. The extra_lines_count: None field is carried into the inline-review struct, and pagination / HttpClient.clone()-in-spawned-tasks semantics are unchanged.\n\nThe remaining observations below are minor and non-blocking. None block merge.",
"verdict": "comment",
"comments": [
{
"file": "Cargo.toml",
"line": 29,
"severity": "style",
"message": "my-clients = { version = \"0.1.0\", ... } uses caret semantics (any 0.1.x), yet AGENTS.md now documents that generated API signature changes require bumping my-clients and its consumers together. A future cargo update could silently resolve a 0.1.x whose generated method order/signatures differ from the positional calls in main.rs, producing a confusing compile-time break later. Consider an exact pin (version = \"=0.1.0\") to keep the pinned crate and its consumers in lockstep."
},
{
"file": "core/src/forgejo.rs",
"line": 123,
"severity": "style",
"message": "authorization_header re-validates and re-parses the exact HeaderValue that validate_forgejo_token already built and discarded, and resolve_forgejo_token + build_forgejo_client validate the same token once more on the same run — so a token may be validated up to three times. A single helper returning Result<HeaderValue, PluginError> (used by build_forgejo_client and download_forgejo_diff) would remove the duplication and keep one source of truth for the token guard."
},
{
"file": "plugins/forgejo-opencode-review/src/main.rs",
"line": 84,
"severity": "style",
"message": "The migration trades the generated client's named-parameter structs (e.g. RepoGetPullRequestFilesParams) for opaque positional arguments like (owner, repo, index, skip_to, whitespace, page, limit). Type-compatible argument-order mistakes now compile silently instead of failing at compile time, and the wiremock tests only pin the page query param (skip_to/whitespace are never exercised), so the swapped-order risk in the new thin wrapper is unverified. Since the my-clients signatures aren't inspectable here, consider a short unit test against the wrapper or at least a comment documenting the argument order of the my-clients methods."
}
]
}

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

Now let me examine the actual repository files to verify the current state of the changed code { "summary": "# Review Summary\n\nThis PR replaces the in-tree Forgejo OpenAPI codegen (`build.rs` + `contracts/forgejo.json` + JRE/`openapi-generator-cli`) with the pre-generated `my-clients` crate from the private Forgejo registry, and the migration is carried through consistently: verify pipeline (JRE install removed), flake devShell, justfile `codegen` recipe, Nix plugin build, `core/Cargo.toml` feature graph, and all consumers in `main.rs`. The four issues from the earlier round are resolved in this diff — a 30 s timeout protects the raw diff download, the token `expect` panic became a `PluginError`, the re-export is narrowed to `pub use my_clients::forgejo as forgejo_client`, and the dead workspace deps (`chrono`, `serde_repr`, `tokio-util`, `openapi-gen-build`) are gone from `Cargo.toml`/`Cargo.lock`/`AGENTS.md`. Both token paths are now guarded (diff download via `authorization_header` and the primary client via `validate_forgejo_token` inside `build_forgejo_client`), and the wiremock integration tests still assert the `Authorization: token <t>` header plus the forbidden `X-FORGEJO-OTP`/`Sudo`/`sudo` headers on the new `.diff` endpoint. The `extra_lines_count: None` field is carried into the inline-review struct, and pagination / `HttpClient.clone()`-in-spawned-tasks semantics are unchanged.\n\nThe remaining observations below are minor and non-blocking. None block merge.", "verdict": "comment", "comments": [ { "file": "Cargo.toml", "line": 29, "severity": "style", "message": "`my-clients = { version = \"0.1.0\", ... }` uses caret semantics (any 0.1.x), yet AGENTS.md now documents that generated API signature changes require bumping `my-clients` and its consumers *together*. A future `cargo update` could silently resolve a 0.1.x whose generated method order/signatures differ from the positional calls in `main.rs`, producing a confusing compile-time break later. Consider an exact pin (`version = \"=0.1.0\"`) to keep the pinned crate and its consumers in lockstep." }, { "file": "core/src/forgejo.rs", "line": 123, "severity": "style", "message": "`authorization_header` re-validates and re-parses the exact `HeaderValue` that `validate_forgejo_token` already built and discarded, and `resolve_forgejo_token` + `build_forgejo_client` validate the same token once more on the same run — so a token may be validated up to three times. A single helper returning `Result<HeaderValue, PluginError>` (used by `build_forgejo_client` and `download_forgejo_diff`) would remove the duplication and keep one source of truth for the token guard." }, { "file": "plugins/forgejo-opencode-review/src/main.rs", "line": 84, "severity": "style", "message": "The migration trades the generated client's named-parameter structs (e.g. `RepoGetPullRequestFilesParams`) for opaque positional arguments like `(owner, repo, index, skip_to, whitespace, page, limit)`. Type-compatible argument-order mistakes now compile silently instead of failing at compile time, and the wiremock tests only pin the `page` query param (`skip_to`/`whitespace` are never exercised), so the swapped-order risk in the new thin wrapper is unverified. Since the `my-clients` signatures aren't inspectable here, consider a short unit test against the wrapper or at least a comment documenting the argument order of the my-clients methods." } ] } --- *🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: **comment**

martin added 1 commit

2026-08-15 13:45:58 +00:00

fix: Comments from review

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

Details

ci/woodpecker/push/verify Pipeline was successful

Details

ci/woodpecker/push/verify-nix Pipeline was successful

Details

ad1db4f2bd

clank-bot reviewed

2026-08-15 13:48:02 +00:00

clank-bot left a comment

Code review findings from opencode.

Code review findings from opencode.

core/src/forgejo.rs Outdated

					
				@ -77,1 +77,4 @@

				    let base_path = format!("{}/api/v1", base_url.trim_end_matches('/'));

				    HttpClient::new()

				        .with_base_url(base_path)

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

[bug] The invalid-token guard was added only to `download_forgejo_diff`, but `build_forgejo_client` — the other, primary path every API call goes through — still passes the raw, untrimmed token from `resolve_forgejo_token` straight into `HttpClient::with_header(...)`. If my-clients builds a `HeaderValue` internally with an expect/unwrap, a token containing a control character (e.g. a trailing newline in `CI_FORGEJO_TOKEN`) will panic/abort rather than surface a `PluginError`; otherwise it fails as an opaque dependency error. Recommend validating/trimming the token once in `resolve_forgejo_token` (or the builder) so both paths share a single explicit guard.

[bug] The invalid-token guard was added only to `download_forgejo_diff`, but `build_forgejo_client` — the other, primary path every API call goes through — still passes the raw, untrimmed token from `resolve_forgejo_token` straight into `HttpClient::with_header(...)`. If my-clients builds a `HeaderValue` internally with an expect/unwrap, a token containing a control character (e.g. a trailing newline in `CI_FORGEJO_TOKEN`) will panic/abort rather than surface a `PluginError`; otherwise it fails as an opaque dependency error. Recommend validating/trimming the token once in `resolve_forgejo_token` (or the builder) so both paths share a single explicit guard.

martin marked this conversation as resolved

core/src/forgejo.rs Outdated

					
				@ -78,0 +92,4 @@

				    pr_number: i64,

				) -> Result<String, PluginError> {

				    let url = format!(

				        "{}/api/v1/repos/{owner}/{repo}/pulls/{pr_number}.diff",

[style] The raw download interpolates `owner`/`repo` into the URL with `format!` and no percent-encoding, unlike the generated client's path builder (which encodes path segments). Forgejo CI vars are admin-controlled identifiers so this is low risk, but the codebase already has the `url` crate available — building the path via its `path_segments_mut` (or encoding the segments) would remove the divergence and make the new endpoint as robust as the generated one.

[style] The raw download interpolates `owner`/`repo` into the URL with `format!` and no percent-encoding, unlike the generated client's path builder (which encodes path segments). Forgejo CI vars are admin-controlled identifiers so this is low risk, but the codebase already has the `url` crate available — building the path via its `path_segments_mut` (or encoding the segments) would remove the divergence and make the new endpoint as robust as the generated one.

core/src/forgejo.rs Outdated

					
				@ -82,2 +103,3 @@

				    );

				    let client = reqwest::Client::builder()

				    reqwest::Client::builder()

				        .timeout(Duration::from_secs(30))

[performance] The new 30-second timeout on `download_forgejo_diff` prevents hangs (good), but Forgejo generates `.diff` payloads lazily and a large PR or slow instance can exceed 30 s, which would turn a previously-tolerant request into a hard failure on big PRs. Consider a more generous bound (60–120 s) or making it configurable. Also note the timeout covers only this raw path — the primary `my-clients` `HttpClient` used for all JSON calls still has no timeout, so a stalled Forgejo response during e.g. `user_get_current` remains unbounded (unless the dependency exposes a way to set one).

[performance] The new 30-second timeout on `download_forgejo_diff` prevents hangs (good), but Forgejo generates `.diff` payloads lazily and a large PR or slow instance can exceed 30 s, which would turn a previously-tolerant request into a hard failure on big PRs. Consider a more generous bound (60–120 s) or making it configurable. Also note the timeout covers only this raw path — the primary `my-clients` `HttpClient` used for all JSON calls still has no timeout, so a stalled Forgejo response during e.g. `user_get_current` remains unbounded (unless the dependency exposes a way to set one).