Skip to content

fix: Devin review bugs + prep @relayauth/server for publishing#9

Closed
khaliqgant wants to merge 1 commit intomainfrom
fix/devin-review-bugs
Closed

fix: Devin review bugs + prep @relayauth/server for publishing#9
khaliqgant wants to merge 1 commit intomainfrom
fix/devin-review-bugs

Conversation

@khaliqgant
Copy link
Copy Markdown
Member

@khaliqgant khaliqgant commented Mar 27, 2026

Bug Fixes (from Devin review of AgentWorkforce/cloud#56)

1. Webhook 4xx retry bug

File: packages/server/src/engine/audit-webhook-dispatcher.ts

The throw for 4xx status codes was caught by its own catch block, causing permanent client errors to be retried 3 times with exponential backoff. Now 4xx errors escape the retry loop immediately.

2. sponsorChain duplication

File: packages/server/src/routes/identities.ts:455

When creating a child identity, the sponsorChain was built as [...parentChain, parent.sub] which duplicates the parent. Fixed to [...parentChain, newIdentityId].

3. Test assertion

File: packages/server/src/__tests__/create-identity.test.ts

Updated the assertion to expect the new identity's ID (not the duplicated parent sub).

Package Prep

  • Renamed relayauth-server@relayauth/server
  • Version: 0.1.0
  • Removed private: true
  • Added exports map (., ./worker, ./durable-objects)
  • Ready for npm publish after merge

Next Steps

Once published, AgentWorkforce/cloud PR #56 will update packages/relayauth/ to import from @relayauth/server instead of using relative path re-exports.


Open with Devin

Bug fixes:
1. audit-webhook-dispatcher: 4xx client errors now fail immediately
   instead of being caught and retried. Only 5xx/network errors retry.

2. identities.ts: sponsorChain now appends the NEW identity ID instead
   of duplicating the parent sub. Was [...chain, auth.claims.sub],
   now [...chain, id].

3. create-identity.test.ts: Updated assertion to expect new identity ID
   in sponsorChain instead of the duplicated parent sub.

Package prep:
- Renamed from relayauth-server to @relayauth/server
- Set version to 0.1.0
- Removed private: true
- Added exports map for worker and durable-objects subpaths
- Ready for npm publish
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

View 3 additional findings in Devin Review.

Open in Devin Review

Comment on lines +61 to +63
if (error instanceof Error && error.message.includes("status 4")) {
throw error;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 HTTP 429 (Too Many Requests) incorrectly treated as permanent failure, preventing retries

The new catch block uses error.message.includes("status 4") to detect 4xx errors and fail immediately without retrying. This matches all 4xx status codes, including HTTP 429 (Too Many Requests), which is explicitly a retryable status code indicating rate limiting. Before this PR, a 429 response would be thrown at line 56, caught at line 60, stored as lastError, and then retried on the next loop iteration. After this PR, 429 triggers the includes("status 4") check and is re-thrown immediately, permanently losing the webhook delivery without any retry attempt. This is a semantic regression for a commonly encountered status code in webhook delivery scenarios.

Suggested change
if (error instanceof Error && error.message.includes("status 4")) {
throw error;
}
if (error instanceof Error && error.message.includes("status 4") && !error.message.includes("status 429")) {
throw error;
}
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

khaliqgant added a commit that referenced this pull request Mar 27, 2026
…cation)

Cherry-picked from PR #9:
1. audit-webhook-dispatcher: 4xx client errors now fail immediately
   instead of being caught and retried
2. identities.ts: sponsorChain now appends the NEW identity ID instead
   of duplicating the parent sub
3. create-identity.test.ts: Updated assertion accordingly

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@khaliqgant
Copy link
Copy Markdown
Member Author

Combined into #10 — bug fixes cherry-picked into the server-package branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant