feature/automatic-pr-decision

[tmseidel]

No description provided.

[max-california]

🤖 Agentic Code Review

Summary: the backend changes for parsing/posting formal review decisions look reasonable, but the workflow configuration UI change has a blocking bug. The new JavaScript assumes each workflow section has exactly one checkbox and one textarea; in practice that will disable unrelated text fields in other workflows and can leave the wrong field greyed out or unsaveable.

Blocking

1. Generic UI selector disables the wrong textarea in workflows that happen to contain any checkbox + any textarea

File: src/main/resources/templates/system-settings/workflow-configurations/workflows.html
Lines: 187–201

The new script does:

var boolCheckbox = collapse.querySelector('input[type="checkbox"]');
var textarea = collapse.querySelector('textarea');

That is not scoped to the agentic-review fields you just added. It grabs the first checkbox and the first textarea in each workflow panel.

That means any workflow panel that contains:

• any boolean param, and
• any text/textarea param

will now get this behavior unintentionally. The textarea may be disabled based on an unrelated checkbox, which can silently prevent editing/saving valid config.

This is especially risky because the template is generic and shared by all workflows, not just agentic-review.

Suggested fix: target the specific field IDs/names for enableFormalReviewDecision and formalReviewDecisionPrompt, or render an explicit data-relationship in the HTML and bind to that. Example direction:

• select input[name="params.agentic-review.enableFormalReviewDecision"]
• select textarea[name="params.agentic-review.formalReviewDecisionPrompt"]

or better, add data-controls / data-controlled-by attributes per field instead of relying on “first checkbox / first textarea”.

Non-blocking

2. parseDecision accepts only very narrow JSON-at-end shapes

File: src/main/java/org/remus/giteabot/prworkflow/agentreview/AgentReviewService.java

Not a bug for the documented format, but the parser is intentionally brittle:

• fenced block must be the last thing in the response
• only simple object shapes are supported
• extraction is regex-based, not JSON-based

That matches the docs/tests, so I wouldn’t block on it. But if model output variability becomes a problem, using a tiny JSON parser for just the last line/object would be more maintainable than regexes.

3. setEnableNativeToolCalling is an inverted setter over a negatively named field

File: src/main/java/org/remus/giteabot/admin/AiIntegration.java

This setter is fine mechanically, but it encodes inversion (useLegacyToolCalling = !enableNativeToolCalling) and may be surprising to frameworks/readers. If this property is bound from forms/JSON anywhere, it’s worth checking that there isn’t also a conventional setter/getter pair for useLegacyToolCalling causing confusion.

What looks good

• RepositoryApiClient.postReviewAction(...) is introduced safely with a default no-op for unsupported providers.
• The workflow correctly posts the Markdown review comment first, then attempts the formal review action, and degrades gracefully on failures.
• Tests around decision parsing and workflow parameter handling cover the main happy paths.

---

Read-only agentic review by AI Git Bot

[max-california]

Changes requested by AI Git Bot (agentic review)

[max-california]

🤖 Agentic Code Review

Summary: the feature is well-scoped and mostly consistent across docs, workflow params, parsing, and provider implementations. I found one blocking correctness issue in the workflow settings UI that can silently force the new formal-review option on, plus one smaller API robustness concern.

Blocking

1. Checkbox + hidden input ordering can cause enableFormalReviewDecision to be persisted as true even when unchecked

File: src/main/resources/templates/system-settings/workflow-configurations/workflows.html:107-121

The template now renders the checkbox first and the hidden "false" input second, both with the same name:

<input class="form-check-input" type="checkbox" ... name="params.agentic-review.enableFormalReviewDecision" value="true" .../>
<input type="hidden" name="params.agentic-review.enableFormalReviewDecision" value="false"/>

When the box is checked, browsers submit both values. Many server-side binders/map parsers keep the last value for duplicate keys, which would make the saved value "false" despite the user checking the box. If your parser keeps the first value, the inverse problem appears when unchecked in some frameworks. Either way, this is fragile and depends on downstream parameter resolution behavior.

The previous ordering (hidden first, checkbox second) is the conventional safe pattern because the checked value wins when frameworks take the last value.

Why this matters here: if the persisted parameter ends up as "true" unexpectedly, this PR enables autonomous approve/request-changes actions without the operator intending to. That’s a significant behavior change.

Suggested fix: move the hidden boolean input back before the checkbox, or ensure the server-side workflow-param parser explicitly handles duplicate boolean values deterministically.

Non-blocking

2. postReviewAction failures on unsupported provider states may be noisier than intended

Files:

• src/main/java/org/remus/giteabot/github/GitHubApiClient.java:71-90
• src/main/java/org/remus/giteabot/gitea/GiteaApiClient.java:75-95

The service correctly swallows failures after posting the comment, but these provider methods assume the token has permission to approve/request changes and that the API endpoint is available for the PR state/repo type. That’s probably fine, but given this is an optional automation feature, it may be worth handling common 4xx cases with clearer logging (e.g. insufficient permissions, self-approval restrictions, provider feature unsupported).

Not blocking since the caller already degrades gracefully.

What looks good

• The decision format is explicitly appended to the prompt and stripped before posting the markdown review comment.
• RepositoryApiClient’s default no-op preserves compatibility for unsupported providers.
• GitLab support was already aligned via override, while GitHub/Gitea implementations match existing review-comment patterns.
• The parsing tests cover the important happy paths and fail-open behavior.

---

Read-only agentic review by AI Git Bot

[max-california]

Changes requested by AI Git Bot (agentic review)

[max-california]

🤖 Agentic Code Review

Summary: the feature is mostly well-implemented and documented, but there is one blocking regression in the workflow configuration form. Unchecked boolean params will no longer be submitted at all, which can make it impossible to turn off previously-enabled flags like enableFormalReviewDecision.

Blocking

1. Unchecked boolean workflow params are no longer persisted as false

File: src/main/resources/templates/system-settings/workflow-configurations/workflows.html
Lines: around 106–111

The template removed the hidden false input that used to accompany every boolean field:

<input th:if="${field.type.name() == 'BOOLEAN'}"
       type="hidden"
       th:name="|params.${row.workflowKey}.${field.name}|"
       value="false"/>

Without that hidden field, an unchecked checkbox does not submit any value. In this form, that means a previously saved boolean workflow param can become “sticky” because the server never receives a replacement false value to overwrite the stored one.

This is especially problematic for the new enableFormalReviewDecision flag:

• operator enables it once and saves
• later unchecks it and saves
• request contains no param for that field
• persisted config may remain true

I can see WorkflowParamsValidator accepts empty/missing input as false, but that only helps if a value is actually present in the submitted param map. The missing-field case needs separate handling, and the removed hidden input was providing exactly that.

Suggested fix: restore the hidden false input for boolean fields, or add explicit server-side logic that treats absent boolean fields as false during workflow-config updates.

Non-blocking

2. parseDecision is stricter than the documented format examples imply

File: src/main/java/org/remus/giteabot/prworkflow/agentreview/AgentReviewService.java

The parser only accepts:

• a fenced ```json block at EOF, or
• a bare JSON object at EOF

That’s okay, but it’s worth noting the fenced regex only strips the final block if it is literally the last content. Any trailing whitespace is fine, but any extra prose after the JSON makes the decision disappear silently. That matches the prompt, so this is not wrong — just somewhat brittle. If you want higher reliability, a tiny JSON parser or looser “last line contains JSON object” extraction would be more tolerant.

Positive

• Nice fail-open behavior: review comment is still posted even when decision parsing or provider review-action posting fails.
• Good provider abstraction via RepositoryApiClient.postReviewAction(...), with graceful default no-op.
• Tests for decision parsing cover the key happy paths and malformed output.
• The AiIntegration#setEnableNativeToolCalling setter looks correct and fixes positive-checkbox binding for the inverse persisted flag.

---

Read-only agentic review by AI Git Bot

[max-california]

Changes requested by AI Git Bot (agentic review)

[max-california]

🤖 Agentic Code Review

Summary: the feature is mostly well integrated, but I found one blocking correctness issue in the web form/controller path for workflow params. The review-action plumbing itself looks reasonable, and the parsing/tests cover the main happy paths.

Blocking

1. extractWorkflowParams can throw on empty value lists

src/main/java/org/remus/giteabot/prworkflow/config/WorkflowConfigurationController.java:197-199

List<String> values = entry.getValue();
String effective = values != null && values.contains("true") ? "true" : values.get(0);

This assumes every params.* entry has at least one value. With MultiValueMap, an entry may exist with an empty list depending on binding/input edge cases, and this will raise IndexOutOfBoundsException, turning a valid save into an error redirect.

This became more likely with the move from Map<String,String> to MultiValueMap<String,String>, since you now explicitly consume the raw list.

Suggested fix: guard for values == null || values.isEmpty() and skip that param (or treat it as blank) before calling get(0).

Non-blocking

1. UI logic only targets the first matching workflow field instance

src/main/resources/templates/system-settings/workflow-configurations/workflows.html:191-202

Using document.querySelector(...) is fine today because workflow keys are unique on the page, but this code is brittle if the form is ever reused in a modal/partial or rendered more than once. Using querySelectorAll plus iterating within a container would make it more robust.

Not a blocker for this PR, just a maintainability note.

2. Missing provider-level tests for formal review actions

The service tests cover parsing well, but I didn’t see tests for postReviewAction(...) in GitHub/Gitea/GitLab clients. Since these are provider-specific endpoints with different semantics, a couple of request-shape tests would help catch regressions.

What looks good

• RepositoryApiClient gets a safe default no-op for unsupported providers.
• AgentReviewService.parseDecision(...) strips the structured suffix before posting the visible comment.
• Fail-open behavior on unparsable output / unsupported providers is sensible.
• The workflow parameter additions and docs are coherent.
• The hidden false + checkbox true handling in the controller is a good improvement.

---

Read-only agentic review by AI Git Bot

[max-california]

Changes requested by AI Git Bot (agentic review)

[max-california]

🤖 Agentic Code Review

Summary: the feature is mostly coherent and well-covered by tests, but I found one blocking correctness issue in the new provider integrations for formal review actions.

Blocking

1. Formal review actions are posted as a second review, not as the action on the review comment just generated

Files:

• src/main/java/org/remus/giteabot/prworkflow/agentreview/AgentReviewService.java
• src/main/java/org/remus/giteabot/github/GitHubApiClient.java
• src/main/java/org/remus/giteabot/gitea/GiteaApiClient.java

The workflow currently does this:

1. postReviewComment(...) with event COMMENT
2. postReviewAction(...) with event APPROVE or REQUEST_CHANGES

On both GitHub and Gitea, POST /pulls/{n}/reviews creates a new review each time. So this change will produce two separate review entries:

• one containing the model’s actual Markdown review text
• another containing only "Approved by AI Git Bot..." / "Changes requested by AI Git Bot..."

That does not match the docs’ description of “post the markdown review comment, then forward the parsed decision”, and more importantly it disconnects the formal state from the substantive review content. Users will see an approval/request-changes review with boilerplate instead of the actual findings.

If the intent is a single formal review, the provider API call needs to submit the model review body with the final event in one request, rather than posting a comment review and then a second action review. If the intent is really two entries, the docs should say so explicitly, but I don’t think that’s desirable UX.

This is serious enough that I’d block on it before merge.

Non-blocking

2. parseDecision is stricter than the prompt wording suggests

File: src/main/java/org/remus/giteabot/prworkflow/agentreview/AgentReviewService.java

The prompt says the model must append a JSON block on the last line, and the docs say parsing works in both native and legacy modes. The implementation only accepts:

• a fenced ```json block at EOF, or
• a bare JSON object at EOF

That’s reasonable, but it will fail open for harmless variants like trailing HTML comments, extra whitespace lines with content, or a fenced block not explicitly tagged json. Since fail-open is intentional, this is fine, but if you want better robustness you could loosen the regex a bit or parse the last non-empty line more directly.

3. UI logic assumes the textarea is wrapped in .mb-3

File: src/main/resources/templates/system-settings/workflow-configurations/workflows.html

decisionTextarea.closest('.mb-3').style.opacity = ...

This will throw if the markup changes and .mb-3 is no longer present around that field. Today it probably exists, so this is not a blocker, but a null-check would make the script less brittle.

What looks good

• Good fail-open behavior when decision parsing fails or the provider doesn’t support review actions.
• MultiValueMap handling for checkbox + hidden-field forms is the right fix.
• The new workflow params and docs are internally consistent.
• Tests around parseDecision cover the main edge cases well.

---

Read-only agentic review by AI Git Bot

[max-california]

Changes requested by AI Git Bot (agentic review)

[max-california]

🤖 Agentic Code Review

Summary: the feature is well-scoped and mostly coherent, but I found one blocking behavioral mismatch between the new parsing logic and the documented/prompted output format.

Blocking

1. The parser does not accept the exact JSON format the docs/instructions tell the model to emit

In src/main/java/org/remus/giteabot/prworkflow/agentreview/AgentReviewService.java, parseDecision() only recognizes:

• a fenced json ... block at EOF, or
• a bare JSON object at EOF

However, both the new docs and the reviewer prompt say:

• “append a JSON decision block on the last line”
• and the example/fixed format is effectively a plain JSON line at the end of the response

That means a model following the instruction literally and returning:

Some review text...

{"decision": "APPROVE"}

will work, but if it follows the phrase “JSON decision block” in the common Markdown sense and emits the fenced example shown in docs, it works too. The real problem is the instruction text is internally inconsistent: it says “last line” but the fenced code block necessarily spans multiple lines. This increases the chance of provider/model-specific formatting drift and fail-open behavior.

More importantly, the implementation currently strips only the matched suffix. If the model appends explanatory prose after a fenced block, parsing silently degrades to NONE, which is acceptable by design, but the instruction/implementation contract should be tightened so the most likely output is unambiguous.

Why this is blocking: this PR’s core feature is “automatic PR decision”; if the required output format is ambiguous, formal decisions will be unreliable in practice.

Suggested fix:

• Make the prompt and docs require exactly one canonical format, preferably a single bare JSON object on the last line.
• Simplify DECISION_FORMAT_INSTRUCTION and docs to match that exactly.
• Keep fenced parsing only as a tolerant fallback if you want, but do not present it as the canonical format.

Non-blocking suggestions

2. postReview(...) can create an empty review body if parsing strips everything

reviewPullRequest() always calls:

• String reviewBody = formatReview(parsed.reviewText());
• repositoryClient.postReview(...)

If the model outputs only the decision JSON and no actual review text, parsed.reviewText() becomes empty and the bot posts a review with just the footer wrapper. That may be acceptable, but it’s probably not desirable.

Consider validating parsed.reviewText() after stripping and either:

• treating it as “no review produced”, or
• falling back to the original review text when the stripped body is blank.

3. UI logic is hard-coded to one workflow/field pair

In src/main/resources/templates/system-settings/workflow-configurations/workflows.html, the JS targets the exact names:

• params.agentic-review.enableFormalReviewDecision
• params.agentic-review.formalReviewDecisionPrompt

This is fine for now, but it couples the template to one specific workflow key and param names. If similar conditional fields are added elsewhere, this pattern will become brittle. A small data-* driven mechanism would scale better.

4. Minor wording inconsistency in docs/UI

The parameter key is formalReviewDecisionPrompt, but the UI label/docs alternate between:

• “Approval decision prompt”
• “formal review decision”
• “approve/request changes”

Not a correctness issue, just worth making consistent so operators understand that NONE is also a valid outcome.

Overall, the repository/API wiring looks sensible, and the fallback from postReview(...) to postReviewComment(...) is a good resilience measure. Once the decision output contract is made unambiguous, this looks close.

---

Read-only agentic review by AI Git Bot

[max-california]

Changes requested by AI Git Bot (agentic review)

[max-california]

🤖 Agentic Code Review

Summary: the feature is well-scoped and the provider-specific postReview(...) path looks correct, but I found one blocking regression in the workflow configuration UI/controller interaction.

Blocking

Disabled textarea means the custom decision prompt is never submitted when the checkbox starts unchecked and the user saves without re-enabling it

In src/main/resources/templates/system-settings/workflow-configurations/workflows.html:198-204, the script disables the formalReviewDecisionPrompt <textarea> whenever enableFormalReviewDecision is unchecked.

Because disabled form controls are not submitted by browsers, this interacts badly with the backend save logic in WorkflowConfigurationController.extractWorkflowParams(...):

• if a config already has a custom formalReviewDecisionPrompt persisted,
• and the operator temporarily unchecks “Enable formal review decision” and saves,
• the prompt field is omitted from the POST entirely,
• so the persisted param is likely dropped/overwritten on save.

That makes the “disable when unchecked” behavior destructive rather than merely cosmetic; re-enabling later will silently lose the operator’s custom prompt and revert to the default.

This is especially risky because the docs/UI describe the textarea as “only editable when the checkbox is checked”, not “cleared when unchecked”.

Suggested fix: keep the textarea readonly instead of disabled, or mirror its value into a hidden field when disabled, so the existing/custom prompt survives round-trips even while the feature is turned off.

Non-blocking

Good improvement: single-review submission path is aligned with provider APIs

RepositoryApiClient.postReview(...) plus the GitHub/Gitea overrides are a sensible addition, and the fallback behavior in AgentReviewService.reviewPullRequest(...) is pragmatic.

Consider adding a controller test for duplicate boolean params + disabled/omitted fields

The MultiValueMap change in WorkflowConfigurationController is good and worth protecting with tests, especially around:

• hidden false + checked true
• omitted textarea due to disabled control
• preserving/merging params for selected workflows

---

Read-only agentic review by AI Git Bot

[max-california]

🤖 Agentic Code Review

Summary: the feature is mostly well-structured and thoughtfully documented/tested, but I found one blocking correctness issue in the review-decision parser that can cause malformed output to be posted to PRs when formal decisions are enabled.

Blocking

parseDecision() fails open by posting the raw JSON/code fence when the model returns an unparseable decision value

In src/main/java/org/remus/giteabot/prworkflow/agentreview/AgentReviewService.java, parseDecision() only strips the trailing structured block if it can also parse a valid enum value.

Relevant flow:

• reviewPullRequest(...) calls parseDecision(review) when formal decisions are enabled.
• If parsing fails, ParseResult.noDecision(review) returns the original full review text.
• The docs and prompt say this should “fail-open” by posting the review with no formal decision.
• In practice, if the model ends with something like:

  {"decision":"REQUEST-CHANGES"}

  or any other malformed decision block, the bot will post that trailing JSON/code fence verbatim into the PR comment.

This is inconsistent with the documented behavior (“review is still posted with no formal decision”) and degrades the PR UX with prompt/protocol leakage.

Concrete location:

• AgentReviewService.parseDecision() around the fenced-block branch (lines ~297-306 in the diff)

Suggested fix:

• Separate stripping from decision parsing:
  • if a trailing fenced/bare JSON decision block is detected, strip it from reviewText regardless;
  • if the decision value is invalid, return action = null but keep the cleaned text.
• Add tests for malformed-but-detectable trailing blocks asserting that the JSON is removed from the posted review body.

Non-blocking

Add one test for the malformed trailing bare-JSON case

src/test/java/org/remus/giteabot/prworkflow/agentreview/AgentReviewServiceTest.java covers malformed fenced JSON, but only checks action() is null. It does not assert whether reviewText() is cleaned, and there is no equivalent malformed bare-JSON test. Given the parser’s contract, it would be good to lock down both cases.

Minor wording mismatch in docs/UI

The UI label is “Approval decision prompt”, but the feature supports APPROVE, REQUEST_CHANGES, and NONE. “Formal review decision prompt” would be a more accurate label and would better match the parameter key / docs.

---

Read-only agentic review by AI Git Bot

[max-california]

🤖 Agentic Code Review

Summary

Looks solid overall: the feature is implemented end-to-end, the provider-specific postReview(...) override avoids double-posting on GitHub/Gitea, and the new parsing / UI plumbing is reasonably well covered by tests.

I did find one blocking correctness issue in the form parameter extraction logic that can corrupt non-boolean multi-valued parameters.

Findings

Blocking

• WorkflowConfigurationController.extractWorkflowParams(...) now mis-parses any duplicated non-boolean field values as the first submitted value
  • File: src/main/java/org/remus/giteabot/prworkflow/config/WorkflowConfigurationController.java:193-203
  • The new MultiValueMap handling applies this rule to all duplicated params:

    String effective = values.contains("true") ? "true" : values.get(0);

    That is only correct for the hidden+checkbox boolean pattern. For any other field type that legitimately submits multiple values with the same name, this will silently pick the first one.
  • In this form, the obvious case is ENUM, which is rendered as a radio-button group in workflows.html. Browsers normally submit only the checked radio, but if markup changes, malformed client input is sent, or another multi-valued param type is introduced later, the controller will persist the wrong value instead of the selected one.
  • More importantly, this method is generic controller infrastructure, not boolean-specific logic, so it should not encode a global “first unless true exists” rule.
  • Suggested fix: only apply the "true"-wins logic when the duplicated parameter corresponds to a known BOOLEAN field; otherwise use the last submitted value (or reject duplicates). That likely means either:
    • passing workflow schema/type info into extraction, or
    • keeping raw multi-values here and resolving boolean semantics later during validation.

Non-blocking

• No regression test for the MultiValueMap duplicate-param handling

  • File: src/main/java/org/remus/giteabot/prworkflow/config/WorkflowConfigurationController.java
  • This change fixes a real checkbox round-tripping problem, but I don’t see a controller/unit test proving:
    1. unchecked boolean => "false"
    2. checked boolean => "true" wins over hidden "false"
    3. non-boolean duplicate params don’t get coerced by the same rule
  • Adding a focused test here would make future UI parameter changes much safer.

• postReviewAction(...) and postReview(...) duplicate provider logic

  • Files: src/main/java/org/remus/giteabot/github/GitHubApiClient.java, src/main/java/org/remus/giteabot/gitea/GiteaApiClient.java
  • Not wrong, but both classes now duplicate reviewEvent(...) and the endpoint/body assembly. Since AgentReviewService now prefers postReview(...), consider whether postReviewAction(...) can delegate to it (or vice versa) to reduce drift.

---

Read-only agentic review by AI Git Bot

[max-california]

🤖 Agentic Code Review

Summary

Looks solid overall. The change is coherent end-to-end: config/UI, workflow params, provider APIs, parsing, docs, and tests all move in the same direction. I didn’t find a clear correctness bug that should block merging.

Findings

Minor / non-blocking

• AgentReviewService.parseDecision(...) is regex-based and intentionally permissive

  • File: src/main/java/org/remus/giteabot/prworkflow/agentreview/AgentReviewService.java
  • This is probably fine for the constrained output format, but it’s worth noting that the parser only supports fairly simple trailing JSON and will strip any trailing object containing "decision" plus no nested } structure. That matches the documented contract, so I don’t think it’s wrong; just a bit brittle if you later relax the output format.
  • Suggestion: keep the contract strict, or add one test covering pretty-printed multi-line bare JSON to document what is and isn’t supported.

• UI behavior depends on field names being specific to agentic-review

  • File: src/main/resources/templates/system-settings/workflow-configurations/workflows.html
  • The JS targets:
    • params.agentic-review.enableFormalReviewDecision
    • params.agentic-review.formalReviewDecisionPrompt
  • That’s fine today, but it’s workflow-specific logic embedded in the generic workflow config page.
  • Suggestion: acceptable as-is, but if more param interdependencies appear, consider rendering declarative metadata from WorkflowParamsSchema instead of hardcoding names in the template.

Positive notes

• Good fallback behavior in AgentReviewService: failed formal review submission degrades to a plain comment rather than dropping findings.
• Good handling of checkbox submission semantics via MultiValueMap and schema-aware boolean resolution.
• Nice provider abstraction with RepositoryApiClient.postReview(...), while preserving default behavior for providers that cannot combine body + action.
• Test coverage is directionally good, especially around parsing and the new provider review endpoints.

---

Read-only agentic review by AI Git Bot