Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
WalkthroughThis PR extends the deployment infrastructure to support a testnet environment as an intermediate stage between development and production. Changes include new GitHub Actions jobs for testnet deployment and lifecycle verification, updated release procedures, refactored environment checks into centralized helper functions, and corresponding test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions
participant Terraform as Terraform
participant API as API Server
participant S3 as S3 Lifecycle
GHA->>Terraform: deploy_otr_testnet (with testnet workspace)
Terraform->>API: Deploy API with testnet config
GHA->>API: test_lifecycle_testnet: GET /healthcheck
API-->>GHA: 200 OK (ready)
GHA->>API: POST /api/v2/assets/test/lifecycle
API->>S3: Renew lifecycle records
S3-->>API: Renewal complete
API-->>GHA: 200 + success: true
GHA->>API: verify-testnet: GET /api/v2/assets/test/lifecycle-status
API-->>GHA: 200 + status: healthy
GHA->>GHA: Write summary with lifecycle metrics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @neekolas's task —— View job PR Review: Add testnet supportI've completed a comprehensive review of this PR. Overall, the implementation is solid and follows good practices, but there are several important issues that should be addressed. ✅ Strengths
🔴 Critical Issues1. Production deployment not gated on testnet ancestry (.github/workflows/deploy-aws.yml:124-147)Issue: The Impact: High - violates the stated deployment policy Recommendation: Add a guard job that verifies the commit exists on verify_prod_commit_on_testnet:
name: Verify prod commit exists on otr-testnet
runs-on: ubuntu-latest
if: (inputs.ref == 'otr-prod') || (inputs.environment == 'otr-prod') || (github.ref == 'refs/heads/otr-prod' && !inputs.ref && !inputs.environment) || (github.event_name == 'repository_dispatch' && github.event.client_payload.ref == 'otr-prod')
steps:
- name: Checkout target ref
uses: actions/checkout@v4
with:
fetch-depth: 0
ref: ${{ github.event_name == 'repository_dispatch' && github.event.client_payload.ref || github.event_name == 'workflow_dispatch' && inputs.environment || github.event_name == 'workflow_call' && inputs.ref || github.ref }}
- name: Verify ancestry from otr-testnet
run: |
git fetch origin otr-testnet --depth=1
candidate_sha="$(git rev-parse HEAD)"
if ! git merge-base --is-ancestor "$candidate_sha" "origin/otr-testnet"; then
echo "::error::Commit $candidate_sha is not present on otr-testnet"
exit 1
fi
deploy_otr_prod:
name: Deploy to OTR Production
runs-on: ubuntu-latest
needs: [push_to_registry, verify_prod_commit_on_testnet] # Add the guard2. Prettier formatting violation (src/api/v2/agents/handlers/join.ts:86-88)Issue: The Current code: const forceError = shouldAllowForcedErrors()
? req.headers["x-force-error"]
: undefined;Should be: const forceError = shouldAllowForcedErrors()
? req.headers["x-force-error"]
: undefined;Impact: Medium - will fail CI linting checks 🟡 Important Issues3. Type safety issue with domain mapping (src/api/v2/agents/handlers/join.ts:40-47)Issue: The Current code: const domainByEnv: Record<
Extract<XmtpEnv, "production" | "testnet">,
string
> = {
production: "popup.convos.org",
testnet: "testnet.convos.org",
};
const domain = xmtpEnv === "dev" ? "dev.convos.org" : domainByEnv[xmtpEnv];Recommendation: Simplify to a safer pattern: const domainByEnv: Record<XmtpEnv, string> = {
production: "popup.convos.org",
testnet: "testnet.convos.org",
dev: "dev.convos.org",
};
const domain = domainByEnv[xmtpEnv];This is more maintainable and type-safe. 4. Missing test coverage for workflow conditionsIssue: While the new helper functions have unit tests, there are no tests verifying the actual workflow behavior (e.g., that testnet uses Recommendation: Add integration or E2E tests that verify:
💡 Suggestions (Nice to Have)5. Workflow duplication could be reducedIssue: The three lifecycle test jobs ( Recommendation: Consider using GitHub Actions matrix strategy to reduce duplication: test_lifecycle:
name: Test S3 Lifecycle Renewal
runs-on: ubuntu-latest
needs: ${{ matrix.deploy_job }}
strategy:
matrix:
include:
- env: dev
api_url_var: DEV_API_URL
deploy_job: deploy_otr_dev
- env: testnet
api_url_var: TESTNET_API_URL
deploy_job: deploy_otr_testnet
- env: prod
api_url_var: PROD_API_URL
deploy_job: deploy_otr_prod
steps:
# ... use ${{ vars[matrix.api_url_var] }}This would make future changes easier to maintain. 6. Consider making helper functions use object parametersNote: CodeRabbit suggested this, and while it's not strictly necessary, it aligns with modern TypeScript best practices. Current: export function isXmtpProduction(xmtpEnv: XmtpEnv = XMTP_ENV): booleanSuggested: export function isXmtpProduction({ xmtpEnv = XMTP_ENV }: { xmtpEnv?: XmtpEnv } = {})This pattern makes it easier to add optional parameters in the future and improves call-site clarity. However, for simple functions with one parameter, the current approach is acceptable. 🔒 SecurityNo security concerns identified. The testnet environment correctly inherits dev-like behavior for testing features (like forced errors) while maintaining production's restrictions. ⚡ PerformanceNo performance concerns. The environment checks are simple boolean comparisons with cached environment variables. 📊 Summary
Recommendation: Address the critical issues (#1 and #2) before merging. The other suggestions can be addressed in follow-up PRs if desired. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
RELEASE.md (1)
1-1: Rename this file to kebab-case for repository consistency.Line 1 is inside
RELEASE.md, but the filename itself is uppercase and does not follow kebab-case (for example,release.md).As per coding guidelines, "Follow consistent file naming (kebab-case)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@RELEASE.md` at line 1, The file name RELEASE.md is not in kebab-case; rename RELEASE.md to release.md to comply with repository naming conventions, use git mv to preserve history, and update any references to "RELEASE.md" in docs, READMEs, CI configs, or build scripts (search for the string RELEASE.md) so they point to "release.md"; commit the rename with a clear message like "rename RELEASE.md to release.md" to complete the change..github/workflows/deploy-aws.yml (1)
201-236: Lifecycle test jobs are a good candidate for matrix refactor.
test_lifecycle_dev,test_lifecycle_testnet, andtest_lifecycle_prodare near-identical; converting to a matrix (env + URL + needs) would simplify future changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy-aws.yml around lines 201 - 236, Replace the three near-identical jobs test_lifecycle_dev, test_lifecycle_testnet, and test_lifecycle_prod with a single matrix job (e.g., job name test_lifecycle) that iterates over environments; define matrix.include entries for each environment containing the environment label (dev/testnet/prod), the corresponding API URL variable (e.g., TESTNET_API_URL), the required needs value (deploy_otr_dev/testnet/prod) and the matching secret token name (LIFECYCLE_DEV_TOKEN / LIFECYCLE_TEST_TOKEN / LIFECYCLE_PROD_TOKEN); update the job to reference matrix.env and matrix.url (and matrix.token via secrets) and set needs: matrix.needs so each matrix axis waits for its corresponding deployment job, keeping the existing healthcheck and curl steps unchanged but parameterized by the matrix values (replace explicit references to test_lifecycle_testnet, ${ { vars.TESTNET_API_URL } }, and ${ { secrets.LIFECYCLE_TEST_TOKEN } } with matrix.url and secrets[matrix.token])..github/workflows/s3-lifecycle-verify.yml (1)
81-151: Consider deduplicating the env-specific verification jobs.This block is almost identical to dev/prod. A matrix job (env + API URL + title) would reduce maintenance drift risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/s3-lifecycle-verify.yml around lines 81 - 151, The workflow duplicates nearly identical jobs (verify-testnet, verify-dev, verify-prod) — replace them with a single matrix job that iterates over environments and injects the env-specific values (e.g., TESTNET_API_URL/secrets.LIFECYCLE_TEST_TOKEN) so maintenance is centralized; create a matrix entry (env, api_url, token_secret, title) and update the step that posts to "${{ vars.TESTNET_API_URL }}/api/..." to use the matrix api_url/token values, keep the step logic that parses response and writes to GITHUB_STEP_SUMMARY unchanged, and remove the separate verify-testnet job so the new matrix job (e.g., job name verify-lifecycle) covers all environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/deploy-aws.yml:
- Around line 143-147: The production deploy job (deploy_otr_prod) is still only
depending on push_to_registry, allowing prod to run without testnet ancestry;
update the workflow so deploy_otr_prod depends on the testnet gate job
(deploy_otr_testnet or a new guard job) instead of or in addition to
push_to_registry and ensure the guard job's condition verifies the
ref/environment ancestry (use the existing deploy_otr_testnet job name or create
a guard job and add it to deploy_otr_prod's needs field) so production deploys
cannot run unless the testnet gate passes.
In `@src/api/v2/agents/handlers/join.ts`:
- Around line 79-80: The ternary assignment to forceError violates Prettier
formatting; reformat the declaration so the ternary is on one line and properly
spaced: assign forceError using the result of shouldAllowForcedErrors() ?
req.headers["x-force-error"] : undefined, referencing the existing symbols
forceError, shouldAllowForcedErrors, and req.headers["x-force-error"] to locate
and fix the statement so it passes the project's Prettier checks.
- Around line 35-46: Refactor buildInviteUrl and shouldAllowForcedErrors to use
object parameters and rely on TypeScript's inferred return types: change
buildInviteUrl to buildInviteUrl({ slug, xmtpEnv = XMTP_ENV }) and change
shouldAllowForcedErrors to shouldAllowForcedErrors({ xmtpEnv = XMTP_ENV } = {}),
but when invoking shouldUseDevBehavior inside shouldAllowForcedErrors call it
positionally as shouldUseDevBehavior(xmtpEnv) (not shouldUseDevBehavior({
xmtpEnv })). Update all call sites and tests to call buildInviteUrl({ slug })
and shouldAllowForcedErrors() (or pass an object when providing xmtpEnv).
In `@src/config.ts`:
- Around line 40-46: Refactor isXmtpProduction and shouldUseDevBehavior to
accept a single object parameter (e.g., { xmtpEnv = XMTP_ENV }) and remove
explicit boolean return annotations so TypeScript infers the return type; update
all call sites (including tests and src/api/v2/agents/handlers/join.ts) to pass
an object with the xmtpEnv property instead of a positional argument, and ensure
shouldUseDevBehavior calls the refactored isXmtpProduction with the same object
parameter shape.
---
Nitpick comments:
In @.github/workflows/deploy-aws.yml:
- Around line 201-236: Replace the three near-identical jobs test_lifecycle_dev,
test_lifecycle_testnet, and test_lifecycle_prod with a single matrix job (e.g.,
job name test_lifecycle) that iterates over environments; define matrix.include
entries for each environment containing the environment label
(dev/testnet/prod), the corresponding API URL variable (e.g., TESTNET_API_URL),
the required needs value (deploy_otr_dev/testnet/prod) and the matching secret
token name (LIFECYCLE_DEV_TOKEN / LIFECYCLE_TEST_TOKEN / LIFECYCLE_PROD_TOKEN);
update the job to reference matrix.env and matrix.url (and matrix.token via
secrets) and set needs: matrix.needs so each matrix axis waits for its
corresponding deployment job, keeping the existing healthcheck and curl steps
unchanged but parameterized by the matrix values (replace explicit references to
test_lifecycle_testnet, ${ { vars.TESTNET_API_URL } }, and ${ {
secrets.LIFECYCLE_TEST_TOKEN } } with matrix.url and secrets[matrix.token]).
In @.github/workflows/s3-lifecycle-verify.yml:
- Around line 81-151: The workflow duplicates nearly identical jobs
(verify-testnet, verify-dev, verify-prod) — replace them with a single matrix
job that iterates over environments and injects the env-specific values (e.g.,
TESTNET_API_URL/secrets.LIFECYCLE_TEST_TOKEN) so maintenance is centralized;
create a matrix entry (env, api_url, token_secret, title) and update the step
that posts to "${{ vars.TESTNET_API_URL }}/api/..." to use the matrix
api_url/token values, keep the step logic that parses response and writes to
GITHUB_STEP_SUMMARY unchanged, and remove the separate verify-testnet job so the
new matrix job (e.g., job name verify-lifecycle) covers all environments.
In `@RELEASE.md`:
- Line 1: The file name RELEASE.md is not in kebab-case; rename RELEASE.md to
release.md to comply with repository naming conventions, use git mv to preserve
history, and update any references to "RELEASE.md" in docs, READMEs, CI configs,
or build scripts (search for the string RELEASE.md) so they point to
"release.md"; commit the rename with a clear message like "rename RELEASE.md to
release.md" to complete the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83371547-fde9-404e-8e5c-0d9520c5a3b0
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.github/workflows/deploy-aws.yml.github/workflows/s3-lifecycle-verify.ymlRELEASE.mdsrc/api/v2/agents/handlers/join.tssrc/api/v2/index.tssrc/api/v2/notifications/handlers/webhook.tssrc/config.tstests/agent-join.test.tstests/config-environment.test.ts
neekolas
force-pushed
the
04-15-otr-testnet
branch
from
8132287 to
f109dd7
Compare
1 participant
Note
Add testnet environment support to deployment pipeline and invite URL routing
otr-testnetbranch,workflow_dispatchoption, anddeploy-otr-testnetrepository dispatch type to deploy-aws.yml, deploying to theconvos-otr-testnetTerraform workspace.verify-testnetCI job in s3-lifecycle-verify.yml that checks S3 lifecycle canaries againstTESTNET_API_URL.buildInviteUrlin join.ts to mapXMTP_ENV=testnettohttps://testnet.convos.orginstead of the dev domain.isXmtpProduction()andshouldUseDevBehavior()helpers in config.ts, replacing scatteredprocess.env.XMTP_ENVcomparisons.otr-testnetbefore reachingotr-prod.Macroscope summarized 8132287. (Automatic summaries will resume when PR exits draft mode or review begins).
Summary by CodeRabbit
Release Notes
New Features
Chores