Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
11 changes: 11 additions & 0 deletions .compozy/tasks/ext-parity/reviews-001/_meta.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
provider: coderabbit
pr: "25"
round: 1
created_at: 2026-04-16T16:42:11.902095Z
---

## Summary
- Total: 46
- Resolved: 46
- Unresolved: 0
202 changes: 202 additions & 0 deletions .compozy/tasks/ext-parity/reviews-001/issue_001.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,202 @@
---
status: resolved
file: extensions/bridges/discord/provider.go
line: 2351
author: coderabbitai[bot]
provider_ref: thread:PRRT_kwDOR5y4QM57fTPp,comment:PRRC_kwDOR5y4QM64dqFo
---

# Issue 001: _⚠️ Potential issue_ | _🟠 Major_
## Review Comment

_⚠️ Potential issue_ | _🟠 Major_

<details>
<summary>🧩 Analysis chain</summary>

🏁 Script executed:

```shell
#!/bin/bash
set -euo pipefail

echo "Check where not-initialized RPC errors are produced:"
rg -nP --type=go 'NewRPCError\(\s*(rpcCodeNotInitialized|-32003)\b'

echo
echo "Check for string-based matching of initialization errors:"
rg -nP --type=go 'Not initialized'
```

Repository: compozy/agh

Length of output: 5929

---

🏁 Script executed:

```shell
# Read the isNotInitializedRPCError function from discord/provider.go
sed -n '2320,2360p' extensions/bridges/discord/provider.go
```

Repository: compozy/agh

Length of output: 969

---

🏁 Script executed:

```shell
# Check if there are tests for isNotInitializedRPCError in discord
rg -n 'isNotInitializedRPCError' extensions/bridges/discord/
```

Repository: compozy/agh

Length of output: 668

---

🏁 Script executed:

```shell
# Verify if the function signature and behavior are tested
rg -B5 -A10 'isNotInitializedRPCError' extensions/bridges/discord/provider_test.go | head -50
```

Repository: compozy/agh

Length of output: 718

---

🏁 Script executed:

```shell
# Find all test usages of isNotInitializedRPCError in discord tests
rg -B2 -A2 'isNotInitializedRPCError' extensions/bridges/discord/provider_test.go
```

Repository: compozy/agh

Length of output: 379

---

🏁 Script executed:

```shell
# Check if there are any references to rpcCodeErr type definition in discord tests
rg -B5 'type rpcCodeErr' extensions/bridges/discord/provider_test.go
```

Repository: compozy/agh

Length of output: 131

---

🏁 Script executed:

```shell
# Get the full rpcCodeErr type definition including its Code() method
rg -A5 'type rpcCodeErr struct' extensions/bridges/discord/provider_test.go
```

Repository: compozy/agh

Length of output: 206

---



</details>

**Remove string-based RPC error matching fallback in `isNotInitializedRPCError`.**

The function currently falls back to string comparison (`strings.EqualFold(rpcErr.Message, "Not initialized")`), which is brittle and violates the coding guideline: "never compare error strings." Production code consistently uses the error code (`rpcCodeNotInitialized`), and multiple bridges use different message variants ("Not initialized", "not ready", etc.), making the message fallback unreliable. Keep matching strictly code-based via the `Code()` interface check.

<details>
<summary>🔧 Proposed fix</summary>

```diff
func isNotInitializedRPCError(err error) bool {
if err == nil {
return false
}
type rpcCodeError interface {
Code() int
}
var codeErr rpcCodeError
if errors.As(err, &codeErr) && codeErr.Code() == rpcCodeNotInitialized {
return true
}
var rpcErr *subprocess.RPCError
- if !errors.As(err, &rpcErr) {
- return false
- }
- return rpcErr.Code == rpcCodeNotInitialized ||
- strings.EqualFold(strings.TrimSpace(rpcErr.Message), "Not initialized")
+ return errors.As(err, &rpcErr) && rpcErr.Code == rpcCodeNotInitialized
}
```
</details>

<!-- suggestion_start -->

<details>
<summary>📝 Committable suggestion</summary>

> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

```suggestion
if err == nil {
return false
}
type rpcCodeError interface {
Code() int
}
var codeErr rpcCodeError
if errors.As(err, &codeErr) && codeErr.Code() == rpcCodeNotInitialized {
return true
}
var rpcErr *subprocess.RPCError
return errors.As(err, &rpcErr) && rpcErr.Code == rpcCodeNotInitialized
```

</details>

<!-- suggestion_end -->

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@extensions/bridges/discord/provider.go` around lines 2336 - 2351, The
function isNotInitializedRPCError currently falls back to brittle string
matching on subprocess.RPCError.Message; remove that string-based check and
return only based on the code-based checks: keep the errors.As check for the
Code() interface (rpcCodeError) and the check against rpcCodeNotInitialized, and
if errors.As into *subprocess.RPCError, only use rpcErr.Code ==
rpcCodeNotInitialized to decide true/false; delete the strings.EqualFold(...
"Not initialized") fallback and ensure the function returns false when no code
match is found (no other error-string comparisons).
```

</details>

<!-- fingerprinting:phantom:poseidon:hawk -->

<!-- This is an auto-generated comment by CodeRabbit -->

## Triage

- Decision: `VALID`
- Notes: `isNotInitializedRPCError` still falls back to matching `subprocess.RPCError.Message == "Not initialized"`, which is brittle and inconsistent with the code-based path already present in the same helper. The fix is to remove the string fallback and tighten the discord tests so only RPC code-based matches succeed.
21 changes: 21 additions & 0 deletions .compozy/tasks/ext-parity/reviews-001/issue_002.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
---
status: resolved
file: extensions/bridges/github/provider_test.go
line: 1050
severity: nitpick
author: coderabbitai[bot]
provider_ref: review:4122628916,nitpick_hash:e4870173171b
review_hash: e4870173171b
source_review_id: "4122628916"
source_review_submitted_at: "2026-04-16T16:31:31Z"
---

# Issue 002: Test name no longer matches behavior
## Review Comment

The test is still named `TestGitHubProviderReconcileRejectsSharedWebhookPaths`, but it now asserts shared paths are accepted. Please rename it to reflect the new contract (e.g., `...AllowsSharedWebhookPaths`) to avoid future misreads.

## Triage

- Decision: `VALID`
- Notes: The test name still says shared webhook paths are rejected, but the assertions explicitly require both configs to be accepted. That mismatch is real and will be corrected while fixing the shared-path signature-routing bug covered by issue 003.
108 changes: 108 additions & 0 deletions .compozy/tasks/ext-parity/reviews-001/issue_003.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
---
status: resolved
file: extensions/bridges/github/provider_test.go
line: 1054
author: coderabbitai[bot]
provider_ref: thread:PRRT_kwDOR5y4QM57fTP2,comment:PRRC_kwDOR5y4QM64dqF6
---

# Issue 003: _⚠️ Potential issue_ | _🟠 Major_
## Review Comment

_⚠️ Potential issue_ | _🟠 Major_

<details>
<summary>🧩 Analysis chain</summary>

🏁 Script executed:

```shell
#!/bin/bash
set -euo pipefail

echo "Inspect webhook signature + routing flow in provider implementation..."
rg -n -C6 'func verifyGitHubWebhookSignature|func selectGitHubIssueConfig|func \(.*\) handleWebhookRequest|webhookSecret|X-Hub-Signature-256' extensions/bridges/github/provider.go

echo
echo "Look for any-candidate signature acceptance loops:"
rg -n -C5 'for _, .* := range candidates|ConstantTimeCompare|hmac|sha256' extensions/bridges/github/provider.go
```

Repository: compozy/agh

Length of output: 5390

---

🏁 Script executed:

```shell
#!/bin/bash
set -euo pipefail

echo "=== Examine handleWebhookRequest and how signature verification is used ==="
rg -n -C10 'handleWebhookRequest' extensions/bridges/github/provider.go | head -80

echo
echo "=== Look for where verifyGitHubWebhookSignature is called ==="
rg -n -B5 -A10 'verifyGitHubWebhookSignature' extensions/bridges/github/provider.go | grep -A15 'verifyGitHubWebhookSignature('

echo
echo "=== Examine the test context around line 1050 ==="
sed -n '1010,1080p' extensions/bridges/github/provider_test.go
```

Repository: compozy/agh

Length of output: 4636

---



</details>

**Fix test name-behavior mismatch and address signature-validation binding gap**

The test name `TestGitHubProviderReconcileRejectsSharedWebhookPaths` contradicts the assertions (line 1050-1054), which now expect `configError == nil`—accepting shared paths.

More critically, the current signature validation (line 1648 in `verifyGitHubWebhookSignature`) accepts ANY candidate's secret, but routing (line 1562 in `selectGitHubIssueConfig`) selects by repository name from the webhook payload. This means a payload signed with Instance A's secret can be routed to Instance B if the `repository.full_name` field matches Instance B—Instance B's secret is never verified against the signature.

Either:
1. Rename the test to reflect acceptance of shared paths, OR
2. Restore rejection of shared webhook paths, OR
3. Change signature verification to re-validate against the selected instance's secret after routing

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@extensions/bridges/github/provider_test.go` around lines 1050 - 1054, The
test name TestGitHubProviderReconcileRejectsSharedWebhookPaths is inconsistent
with assertions that expect configs[i].configError == nil and there is a real
security gap: verifyGitHubWebhookSignature currently accepts any candidate
secret but selectGitHubIssueConfig routes by repository.full_name, so an event
signed with Instance A's secret can be delivered to Instance B without
re-checking its secret. Fix by (preferably) updating the webhook handling flow:
after selectGitHubIssueConfig selects the target config (by
repository.full_name), re-run signature verification against that selected
config's secret (use the same verification logic in verifyGitHubWebhookSignature
but with the selectedConfig.Secret) before accepting/routing the payload; also
update tests (either rename TestGitHubProviderReconcileRejectsSharedWebhookPaths
to reflect acceptance, or change tests to assert rejection if you restore
shared-path rejection) so they match the new behavior and add a unit test that
ensures a payload signed with A's secret is rejected for B.
```

</details>

<!-- fingerprinting:phantom:poseidon:hawk -->

<!-- This is an auto-generated comment by CodeRabbit -->

## Triage

- Decision: `VALID`
- Notes: The production routing gap is real. `serveWebhookHTTP` verifies the signature against any config sharing the path, then `selectGitHubIssueConfig`/`selectGitHubReviewConfig` routes by payload repository name, so a payload signed with one instance secret can be routed to another instance on the same webhook path. Fix requires a minimal production change in `extensions/bridges/github/provider.go` to re-verify the selected config's secret after routing, plus test updates in `provider_test.go`.
21 changes: 21 additions & 0 deletions .compozy/tasks/ext-parity/reviews-001/issue_004.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
---
status: resolved
file: extensions/bridges/teams/provider_test.go
line: 1255
severity: nitpick
author: coderabbitai[bot]
provider_ref: review:4122628916,nitpick_hash:bcca49a3ba22
review_hash: bcca49a3ba22
source_review_id: "4122628916"
source_review_submitted_at: "2026-04-16T16:31:31Z"
---

# Issue 004: Wait for ready state in the polling predicate to reduce flakiness.
## Review Comment

This currently waits for “any state” and asserts readiness afterward. If an intermediate non-ready state is written first, the test can fail intermittently.

## Triage

- Decision: `VALID`
- Notes: The current predicate returns as soon as any state marker exists, but the assertion checks readiness afterward. If the file first contains a non-ready marker and later appends a ready marker, this test can observe the intermediate state and fail. The fix is to wait for a ready marker in the predicate.
21 changes: 21 additions & 0 deletions .compozy/tasks/ext-parity/reviews-001/issue_005.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
---
status: resolved
file: go.mod
line: 77
severity: nitpick
author: coderabbitai[bot]
provider_ref: review:4122628916,nitpick_hash:c42d9123128f
review_hash: c42d9123128f
source_review_id: "4122628916"
source_review_submitted_at: "2026-04-16T16:31:31Z"
---

# Issue 005: Consider updating the daytona SDK if gorilla/websocket pseudo-version drift is a concern.
## Review Comment

The `gorilla/websocket` pseudo-version is an indirect dependency pulled transitively through `github.com/daytonaio/daytona/libs/sdk-go/pkg/daytona` with no direct imports in this codebase. The version constraint is controlled by the daytona SDK, not by this project. If supply-chain risk from the pseudo-version is a concern, you would need to update the daytona SDK version or explicitly pin `gorilla/websocket` at the expense of potentially conflicting with daytona's requirements.

## Triage

- Decision: `INVALID`
- Notes: This is dependency-management advice, not a defect in the scoped change. `github.com/gorilla/websocket` is only an indirect dependency here and the review comment does not identify a concrete incompatibility, failing build, or exploitable issue caused by the current graph. Updating the upstream Daytona SDK is outside this remediation batch.
Loading