feat: Add support for local config #24

This PR adds repository-label-based rule matching: repos can place a root-level woodpecker.toml defining labels, which the extension fetches from the Forgejo Contents API (at the pipeline ref) and AND-combines with the existing glob-pattern rules via new match_labels / match_not_labels fields. Overall the feature is well designed and documented: the rule engine changes are clean, the concurrency via tokio::try_join! is good, label conditions have unit tests, and the 404 → no labels path degrades gracefully.

Key issues

Misleading inverted variable names in forgejo_integration.rs (most important). create_configuration returns (Configuration, String) built as Ok((config, owner)), but both call sites destructure it as let (owner, config) = ..., so the local owner actually holds the Configuration and config holds the owner string. The code still works because the two swaps cancel out when passing (&owner, &config) to the generated repo_get_contents* functions (which expect (configuration, owner, ...)). This is a correctness trap for future maintainers and should be aligned.
Unconditional extra Forgejo API call. fetch_repo_labels runs for every pipeline request even when no rule on the endpoint uses labels, adding a (usually 404) request per invocation. It should be gated on whether any referenced rule has label constraints.

Minor notes

Malformed repo-controlled woodpecker.toml maps to 400 Bad Request (documented, but misattributes the fault to the request; 502 or treat-as-empty might be better).
base64 decode only strips \n, not \r; CRLF content would fail.
Adds a second TOML parser (toml 0.8) when toml 1.1.3 is already in the tree via config-rs.
flake.nix tooltip = "defaultToolchain" is unrelated to this feature (drive-by).
Adding match_labels to the bun-astro example rule silently changes example semantics for anyone copying it.
Test gaps: no test for the 404 → None path in get_repo_config, and no end-to-end test exercising label matching through the full handler; the new evaluate_rules integration tests only use empty labels.

Tests

Existing tests remain valid because wiremock's unmatched woodpecker.toml requests fall through to a 404 which is treated as "no labels".

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

# Review Summary This PR adds repository-label-based rule matching: repos can place a root-level `woodpecker.toml` defining `labels`, which the extension fetches from the Forgejo Contents API (at the pipeline ref) and AND-combines with the existing glob-pattern rules via new `match_labels` / `match_not_labels` fields. Overall the feature is well designed and documented: the rule engine changes are clean, the concurrency via `tokio::try_join!` is good, label conditions have unit tests, and the `404 → no labels` path degrades gracefully. ## Key issues 1. **Misleading inverted variable names in `forgejo_integration.rs` (most important).** `create_configuration` returns `(Configuration, String)` built as `Ok((config, owner))`, but both call sites destructure it as `let (owner, config) = ...`, so the local `owner` actually holds the `Configuration` and `config` holds the owner string. The code still works because the two swaps cancel out when passing `(&owner, &config)` to the generated `repo_get_contents*` functions (which expect `(configuration, owner, ...)`). This is a correctness trap for future maintainers and should be aligned. 2. **Unconditional extra Forgejo API call.** `fetch_repo_labels` runs for every pipeline request even when no rule on the endpoint uses labels, adding a (usually 404) request per invocation. It should be gated on whether any referenced rule has label constraints. ## Minor notes - Malformed repo-controlled `woodpecker.toml` maps to 400 Bad Request (documented, but misattributes the fault to the request; 502 or treat-as-empty might be better). - base64 decode only strips `\n`, not `\r`; CRLF content would fail. - Adds a second TOML parser (`toml 0.8`) when `toml 1.1.3` is already in the tree via config-rs. - `flake.nix` `tooltip = "defaultToolchain"` is unrelated to this feature (drive-by). - Adding `match_labels` to the `bun-astro` example rule silently changes example semantics for anyone copying it. - Test gaps: no test for the `404 → None` path in `get_repo_config`, and no end-to-end test exercising label matching through the full handler; the new `evaluate_rules` integration tests only use empty labels. ## Tests Existing tests remain valid because wiremock's unmatched `woodpecker.toml` requests fall through to a 404 which is treated as "no labels". --- *🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: **changes**

clank-bot reviewed

2026-08-02 19:10:01 +00:00

clank-bot left a comment

Code review findings from opencode.

Cargo.toml Outdated

					
				@ -44,2 +44,4 @@

				url = {version = "2.5", features = ["serde"]}

				globset = {version = "0.4"}

				base64 = {version = "0.22"}

				toml = {version = "0.8"}

[style] toml = { version = "0.8" } adds a second TOML parser stack — the tree already contains toml 1.1.3 via config-rs (see Cargo.lock). The 0.8 line also predates TOML 1.1 spec support. Prefer reusing the existing version or document why the split is needed.

[style] `toml = { version = "0.8" }` adds a second TOML parser stack — the tree already contains `toml 1.1.3` via config-rs (see Cargo.lock). The 0.8 line also predates TOML 1.1 spec support. Prefer reusing the existing version or document why the split is needed.

config.example.toml

					
				@ -39,1 +39,4 @@

				match_patterns = ["bun.lock", "package.json"]

				# All labels listed here must be present in the repository's root-level

				# woodpecker.toml file.

				match_labels = ["destination:cluster"]

[style] Adding match_labels = ["destination:cluster"] to the existing bun-astro example rule silently changes its semantics — anyone copying the example now needs a repo-root woodpecker.toml with that label for the rule to match. Consider a separate commented-out label example or a dedicated rule instead.

[style] Adding `match_labels = ["destination:cluster"]` to the existing `bun-astro` example rule silently changes its semantics — anyone copying the example now needs a repo-root `woodpecker.toml` with that label for the rule to match. Consider a separate commented-out label example or a dedicated rule instead.

flake.nix

					
				@ -62,6 +62,7 @@

				              ;

				          };

				        devShells.default = mkDevShell {

				          tooltip = "defaultToolchain";

[style] tooltip = "defaultToolchain" appears unrelated to this PR's feature. Confirm it is intentional (e.g. required by the mkDevShell helper) or drop it to keep the change scoped.

[style] `tooltip = "defaultToolchain"` appears unrelated to this PR's feature. Confirm it is intentional (e.g. required by the `mkDevShell` helper) or drop it to keep the change scoped.

src/integration/forgejo_integration.rs

					
				@ -84,0 +118,4 @@

				fn create_configuration(

				    forge_url: &Option<Nullable<String>>,

				    netrc: &Option<Netrc>,

				) -> Result<(Configuration, String), AppError> {

[style] create_configuration returns Ok((config, owner)) typed as (Configuration, String), but both call sites destructure it as let (owner, config) = ... (lines 67 and 85). This makes the local owner a Configuration and config the owner string. It only works because the double swap aligns with the generated repo_get_contents* signatures (configuration first, owner second). This is a latent correctness hazard for anyone refactoring the calls. Return/destructure in a consistent order (e.g. (owner, config) where owner is the String), or restructure the helper to make the binding obvious.

[style] `create_configuration` returns `Ok((config, owner))` typed as `(Configuration, String)`, but both call sites destructure it as `let (owner, config) = ...` (lines 67 and 85). This makes the local `owner` a `Configuration` and `config` the owner string. It only works because the double swap aligns with the generated `repo_get_contents*` signatures (configuration first, owner second). This is a latent correctness hazard for anyone refactoring the calls. Return/destructure in a consistent order (e.g. `(owner, config)` where owner is the `String`), or restructure the helper to make the binding obvious.

src/integration/forgejo_integration.rs

					
				@ -84,0 +106,4 @@

				            )));

				        };

				        let decoded = base64::engine::general_purpose::STANDARD

				            .decode(content.replace('\n', ""))

[style] content.replace('\n', "") only strips LF; CRLF-encoded content would leave \r and fail base64 decoding. Strip all whitespace (e.g. content.chars().filter(|c| !c.is_whitespace()).collect::<String>()) for robustness.

[style] `content.replace('\n', "")` only strips LF; CRLF-encoded content would leave `\r` and fail base64 decoding. Strip all whitespace (e.g. `content.chars().filter(|c| !c.is_whitespace()).collect::<String>()`) for robustness.

src/server.rs Outdated

					
				@ -91,2 +91,4 @@

				                )));

				            }

				            AppError::InvalidRepoConfig(message) => {

				                return ProblemDetail::new_bad_request(Some(&message));

[style] A malformed woodpecker.toml (content owned by the repository, not the requester) is mapped to 400 Bad Request, which tells the Woodpecker server its request was bad. This is documented in the README as deliberate, but a 502 or treating malformed labels as 'no labels' would be less surprising for a repo-controlled artifact.

[style] A malformed `woodpecker.toml` (content owned by the repository, not the requester) is mapped to 400 Bad Request, which tells the Woodpecker server its request was bad. This is documented in the README as deliberate, but a 502 or treating malformed labels as 'no labels' would be less surprising for a repo-controlled artifact.

src/workflows.rs Outdated

					
				@ -27,1 +27,3 @@

				    let file_names = fetch_root_file_names(&client.forgejo, request).await?;

				    let (file_names, labels) = tokio::try_join!(

				        fetch_root_file_names(&client.forgejo, request),

				        fetch_repo_labels(&client.forgejo, request)

[performance] fetch_repo_labels is awaited unconditionally for every pipeline request, even when none of the endpoint's rules reference match_labels/match_not_labels. Since most repos have no woodpecker.toml, this adds a 404 round-trip to every invocation. Consider only fetching labels when at least one referenced rule defines label constraints.

[performance] `fetch_repo_labels` is awaited unconditionally for every pipeline request, even when none of the endpoint's rules reference `match_labels`/`match_not_labels`. Since most repos have no `woodpecker.toml`, this adds a 404 round-trip to every invocation. Consider only fetching labels when at least one referenced rule defines label constraints.

martin added 1 commit

2026-08-02 19:29:08 +00:00

fix: Issues and inconcistencies

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

Details

ci/woodpecker/push/verify Pipeline was successful

Details

3d5cb123fd

clank-bot commented

2026-08-02 19:33:39 +00:00

Review Summary

This PR adds repository-label based rule matching (match_labels / match_not_labels), reading an optional root-level woodpecker.toml from the repo at the pipeline ref. Overall the change is solid and well-executed: it is fully documented in the README, config examples are updated, OpenAPI documents the new 502 response, and there is good unit + wiremock test coverage for rule matching, the Forgejo client, the endpoint behavior, and the 502 path.

Strong points:

Label fetching is skipped entirely when no rule on the endpoint references labels (labels_required gate in get_workflows_for_endpoint), and the two Forgejo calls are parallelized with tokio::try_join!.
A missing woodpecker.toml (404) is treated as an empty label list; matching keeps its existing AND semantics.
The refactor of netrc/config creation into create_configuration/extract_netrc reduces duplication.

Issues worth addressing:

The base64 decode strips \n but not \r (or other whitespace), so CRLF-wrapped content from some Forgejo deployments would fail STANDARD decoding and turn an otherwise-fine repo into a permanent 502.
A single malformed woodpecker.toml hard-fails the entire extension request (502) for that repo on any endpoint that has any label rule, even when no rule would have matched. This is documented as intentional, but treating unparseable TOML as empty labels + a warning log would be more resilient.
Repo-controlled labels are not a security boundary — any contributor with push access can add labels to enable gated workflows. This should be called out so operators do not rely on match_labels for access control.
Minor: the flake.nix tooltip = "defaultToolchain" change is unrelated to this feature, and the TODO comment on create_configuration is now stale/ambiguous.

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

# Review Summary This PR adds repository-label based rule matching (`match_labels` / `match_not_labels`), reading an optional root-level `woodpecker.toml` from the repo at the pipeline ref. Overall the change is solid and well-executed: it is fully documented in the README, config examples are updated, OpenAPI documents the new 502 response, and there is good unit + wiremock test coverage for rule matching, the Forgejo client, the endpoint behavior, and the 502 path. Strong points: - Label fetching is skipped entirely when no rule on the endpoint references labels (`labels_required` gate in `get_workflows_for_endpoint`), and the two Forgejo calls are parallelized with `tokio::try_join!`. - A missing `woodpecker.toml` (404) is treated as an empty label list; matching keeps its existing AND semantics. - The refactor of netrc/config creation into `create_configuration`/`extract_netrc` reduces duplication. Issues worth addressing: - The base64 decode strips `\n` but not `\r` (or other whitespace), so CRLF-wrapped content from some Forgejo deployments would fail `STANDARD` decoding and turn an otherwise-fine repo into a permanent 502. - A single malformed `woodpecker.toml` hard-fails the entire extension request (502) for that repo on any endpoint that has any label rule, even when no rule would have matched. This is documented as intentional, but treating unparseable TOML as empty labels + a warning log would be more resilient. - Repo-controlled labels are not a security boundary — any contributor with push access can add labels to enable gated workflows. This should be called out so operators do not rely on `match_labels` for access control. - Minor: the `flake.nix` `tooltip = "defaultToolchain"` change is unrelated to this feature, and the TODO comment on `create_configuration` is now stale/ambiguous. --- *🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: **comment**

clank-bot reviewed

2026-08-02 19:33:40 +00:00

clank-bot left a comment

Code review findings from opencode.

flake.nix

					
				@ -62,6 +62,7 @@

				              ;

				          };

[style] The tooltip = "defaultToolchain" addition to the devShell appears unrelated to the repository-labels feature in this PR. Consider splitting it into a separate commit/PR to keep changes focused and reviewable.

[style] The `tooltip = "defaultToolchain"` addition to the devShell appears unrelated to the repository-labels feature in this PR. Consider splitting it into a separate commit/PR to keep changes focused and reviewable.

src/config/config.rs

					
				@ -163,1 +163,4 @@

				    pub match_not_patterns: Vec<String>,

				    /// Labels that must all be present in the repository's `woodpecker.toml`.

				    #[serde(default)]

				    pub match_labels: Vec<String>,

[security] match_labels/match_not_labels are derived from repository-controlled content that any contributor with push access can modify (they can simply add labels to woodpecker.toml). This means label conditions provide no access control — they are pure opt-in routing. Worth an explicit note in the README so operators do not use label-gated rules (e.g. a "deploy to production" workflow gated only on match_labels) as a security boundary.

[security] `match_labels`/`match_not_labels` are derived from repository-controlled content that any contributor with push access can modify (they can simply add labels to `woodpecker.toml`). This means label conditions provide no access control — they are pure opt-in routing. Worth an explicit note in the README so operators do not use label-gated rules (e.g. a "deploy to production" workflow gated only on `match_labels`) as a security boundary.

src/integration/forgejo_integration.rs

					
				@ -84,0 +114,4 @@

				    }

				}

				// TODO this should be refactored into a shared utility

[style] The // TODO this should be refactored into a shared utility comment is now stale/misleading: this PR already extracted the shared create_configuration/extract_netrc helpers used by both methods. Either drop the TODO or reword it to describe the actual remaining duplication.

[style] The `// TODO this should be refactored into a shared utility` comment is now stale/misleading: this PR already extracted the shared `create_configuration`/`extract_netrc` helpers used by both methods. Either drop the TODO or reword it to describe the actual remaining duplication.

src/integration/forgejo_integration.rs

					
				@ -84,0 +106,4 @@

				            )));

				        };

				        let decoded = base64::engine::general_purpose::STANDARD

				            .decode(content.replace('\n', ""))

[bug] content.replace('\n', "") only strips LF, so CRLF-wrapped base64 (which some Forgejo/Gitea deployments emit, e.g. MIME-style 76-char wrapping with \r\n) still contains \r characters. The STANDARD engine rejects any whitespace, so this turns into InvalidRepoConfig → 502 for an otherwise-valid file. Consider stripping all ASCII whitespace instead, e.g. content.chars().filter(|c| !c.is_ascii_whitespace()).collect::<String>().

[bug] `content.replace('\n', "")` only strips LF, so CRLF-wrapped base64 (which some Forgejo/Gitea deployments emit, e.g. MIME-style 76-char wrapping with `\r\n`) still contains `\r` characters. The `STANDARD` engine rejects any whitespace, so this turns into `InvalidRepoConfig` → 502 for an otherwise-valid file. Consider stripping all ASCII whitespace instead, e.g. `content.chars().filter(|c| !c.is_ascii_whitespace()).collect::<String>()`.

src/workflows.rs

					
				@ -143,0 +174,4 @@

				        return Ok(Vec::new());

				    };

				    let config: RepoConfig =

				        toml::from_str(&content).map_err(|error| AppError::InvalidRepoConfig(error.to_string()))?;

[bug] A malformed woodpecker.toml aborts the whole request with 502 even when no rule would have matched for this repo, and it fails every pipeline of that repo on any endpoint with a label rule (the parse happens before rule evaluation and cannot be avoided by the repo operator). This is documented as intended, but a more resilient approach would be to log a warning and treat unparseable TOML as empty labels — especially since the file is repository-controlled upstream data that a broken push can introduce at any time.

[bug] A malformed `woodpecker.toml` aborts the whole request with 502 even when no rule would have matched for this repo, and it fails every pipeline of that repo on any endpoint with a label rule (the parse happens before rule evaluation and cannot be avoided by the repo operator). This is documented as intended, but a more resilient approach would be to log a warning and treat unparseable TOML as empty labels — especially since the file is repository-controlled upstream data that a broken push can introduce at any time.