ci: provision PR preview environments in Railway#2681
Conversation
|
|
Error agent completed without reporting progress |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(8) Total Issues | Risk: Medium
🟠⚠️ Major (6) 🟠⚠️
Inline Comments:
- 🟠 Major:
preview-environments.yml:544Actionactions/checkout@v6not pinned to SHA - 🟠 Major:
preview-environments.yml:213Docker imageghcr.io/railwayapp/cli:latestuses mutable tag (also lines 438, 839) - 🟠 Major:
preview-environments.yml:357-367Secrets written to GITHUB_OUTPUT before masking - 🟠 Major:
preview-environments.yml:514-519upsert_env discards error body and lacks timeout - 🟠 Major:
preview-environments.yml:780-782Vercel runtime logs may expose secrets in error traces - 🟠 Major:
preview-environments.yml:848-862Teardown lacks error handling and verification
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
preview-environments.yml:91-97GitHub API curl call missing explicit timeout - 🟡 Minor:
preview-environments.yml:327-342Retry loops use fixed delays instead of exponential backoff (duplicated at lines 447-478, 697-740)
💭 Consider (3) 💭
Inline Comments:
- 💭 Consider:
preview-environments.yml:564Vercel CLI installed without version pinning (also line 776)
💭 1) jobs Add timeout-minutes to all jobs
Issue: None of the jobs specify timeout-minutes. This workflow involves external API calls, polling loops with up to 30 attempts (60+ seconds), and deployment operations.
Why: Without timeouts, hung jobs could consume runner minutes indefinitely. Other workflows in this repo (ci.yml, cypress.yml) specify explicit timeouts.
Fix: Add timeout-minutes: 20 (or appropriate value) to each job definition, especially provision-tier1, configure-vercel-preview, smoke-preview, and teardown-tier1.
Refs:
💭 2) ANTHROPIC_API_KEY Add explicit masking for defense-in-depth
Issue: ANTHROPIC_API_KEY is passed via environment and used in upsert_env but is never explicitly masked with ::add-mask::. While GitHub Actions auto-masks ${{ secrets.* }} references, the masking applies to exact string values.
Why: If the secret appears in a slightly different context (URL-encoded, in error messages with context, or truncated), it may not be masked. Explicit masking provides defense-in-depth.
Fix: Add at the start of the step that uses it:
echo "::add-mask::${ANTHROPIC_API_KEY}"✅ Good Practices Observed
The workflow demonstrates several security and operational best practices:
- Least-privilege permissions with
contents: readandpull-requests: write - Internal PR gating — preview environments only provision for internal PRs (lines 106-112)
- PR number/branch validation with regex guards (lines 83-86, 201-204, 420-423, 827-830)
- Proper secret masking for database URLs (lines 361-363, 491-493)
- Safe environment naming with strict
^pr-[0-9]+$pattern enforcement - No use of dangerous triggers like
pull_request_target - Comprehensive diagnostics on smoke test failure (lines 745-782)
🚫 REQUEST CHANGES
Summary: This is a well-structured preview environment workflow with good security fundamentals. The main concerns are supply chain risks (unpinned action/container tags), secret handling timing (mask-before-use), and operational reliability (timeouts, error handling, backoff strategies). The action SHA pinning and secret masking order are the most important items to address before merge. The other findings are improvements that can be addressed now or in follow-up.
Discarded (7)
| Location | Issue | Reason Discarded |
|---|---|---|
json_get_var() |
"Empty parameter assignment" | False positive — code correctly uses local json="$1" |
ensure_runtime_var_seeded() |
"Empty parameter assignment" | False positive — code correctly uses local key="$1" |
extract_runtime_var() |
"Empty parameter assignment" | False positive — code correctly uses local key="$1" |
debug_key() |
"Empty parameter assignment" | False positive — code correctly uses local key="$1" |
upsert_env() |
"Empty parameter assignment" | False positive — code correctly uses local project_id="$1" |
wait_for_success_url() |
"Empty parameter assignment" | False positive — code correctly uses local url="$1" |
line 420 |
"Branch regex may reject valid names" | Valid observation but intentionally restrictive for safety |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
8 | 0 | 2 | 0 | 2 | 0 | 4 |
pr-review-sre |
10 | 0 | 0 | 0 | 3 | 0 | 7 |
pr-review-security-iam |
4 | 0 | 1 | 0 | 3 | 0 | 0 |
pr-review-errors |
6 | 0 | 0 | 0 | 2 | 0 | 4 |
pr-review-standards |
7 | 0 | 0 | 0 | 0 | 0 | 7 |
| Total | 35 | 0 | 3 | 0 | 10 | 0 | 22 |
Note: pr-review-standards hallucinated multiple CRITICAL findings about "empty parameter assignments" in bash functions — all were false positives upon verification.
| fi | ||
|
|
||
| - name: Checkout | ||
| uses: actions/checkout@v6 |
There was a problem hiding this comment.
🟠 MAJOR: Action not pinned to SHA
Issue: actions/checkout@v6 uses a mutable tag instead of a full SHA. All other workflows in this repository consistently use SHA pinning.
Why: Mutable tags can be updated by upstream maintainers, creating a supply chain risk where malicious code could be injected into workflows.
Fix:
| uses: actions/checkout@v6 | |
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 |
Refs:
| -v "${RAILWAY_HOME_DIR}:/root/.railway" \ | ||
| -v "$PWD:/workspace" \ | ||
| -w /workspace \ | ||
| ghcr.io/railwayapp/cli:latest railway "$@" |
There was a problem hiding this comment.
🟠 MAJOR: Docker image uses mutable :latest tag
Issue: The Railway CLI Docker image uses the :latest tag at lines 213, 438, and 839. Mutable container tags are a supply chain risk — the image contents can change between workflow runs.
Why: This can introduce breaking changes, vulnerabilities, or malicious code without any visible change to the workflow file. It's functionally equivalent to using mutable action tags.
Fix: Pin to a specific version tag or digest. Check Railway's container registry for available version tags:
# Example (replace with actual stable version):
ghcr.io/railwayapp/cli:3.x.x
# or using digest:
ghcr.io/railwayapp/cli@sha256:<digest>Refs:
| MANAGE_DB_URL="$(extract_runtime_var "${RAILWAY_MANAGE_DB_URL_KEY}")" | ||
| RUN_DB_URL="$(extract_runtime_var "${RAILWAY_RUN_DB_URL_KEY}")" | ||
| SPICEDB_ENDPOINT="$(extract_runtime_var "${RAILWAY_SPICEDB_ENDPOINT_KEY}")" | ||
|
|
||
| echo "::add-mask::${MANAGE_DB_URL}" | ||
| echo "::add-mask::${RUN_DB_URL}" | ||
| echo "::add-mask::${SPICEDB_ENDPOINT}" | ||
|
|
||
| echo "manage_db_url=${MANAGE_DB_URL}" >> "$GITHUB_OUTPUT" | ||
| echo "run_db_url=${RUN_DB_URL}" >> "$GITHUB_OUTPUT" | ||
| echo "spicedb_endpoint=${SPICEDB_ENDPOINT}" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
🟠 MAJOR: Mask secrets immediately after extraction
Issue: Database URLs are extracted and stored in variables (lines 357-359), then masking is applied (lines 361-363), and finally written to outputs (lines 365-367). If extraction fails mid-way or an error occurs between extraction and masking, secret values may appear unmasked in logs.
Why: The ::add-mask:: directive should be called immediately after each secret is obtained, before any other code runs that could fail and expose the value.
Fix: Restructure to mask each secret immediately after extraction:
MANAGE_DB_URL="$(extract_runtime_var "${RAILWAY_MANAGE_DB_URL_KEY}")"
echo "::add-mask::${MANAGE_DB_URL}"
RUN_DB_URL="$(extract_runtime_var "${RAILWAY_RUN_DB_URL_KEY}")"
echo "::add-mask::${RUN_DB_URL}"
SPICEDB_ENDPOINT="$(extract_runtime_var "${RAILWAY_SPICEDB_ENDPOINT_KEY}")"
echo "::add-mask::${SPICEDB_ENDPOINT}"
echo "manage_db_url=${MANAGE_DB_URL}" >> "$GITHUB_OUTPUT"
echo "run_db_url=${RUN_DB_URL}" >> "$GITHUB_OUTPUT"
echo "spicedb_endpoint=${SPICEDB_ENDPOINT}" >> "$GITHUB_OUTPUT"Refs:
| curl --fail-with-body -sS \ | ||
| -X POST \ | ||
| -H "Authorization: Bearer ${VERCEL_TOKEN}" \ | ||
| -H "Content-Type: application/json" \ | ||
| "https://api.vercel.com/v10/projects/${project_id}/env?upsert=true&teamId=${VERCEL_ORG_ID}" \ | ||
| --data "${payload}" >/dev/null |
There was a problem hiding this comment.
🟠 MAJOR: Error output discarded and missing timeout
Issue: The upsert_env() function uses --fail-with-body but redirects output to /dev/null, losing the Vercel API error body. Additionally, there's no --connect-timeout or --max-time, so a hung request could block the workflow indefinitely.
Why: When any of the 9 sequential upsert_env calls fail (lines 523-541), operators see only a generic curl error with no indication of which env var failed or why. The --fail-with-body flag is designed to preserve error details, but discarding stdout defeats this.
Fix: Add timeouts and preserve error output:
upsert_env() {
local project_id="$1"
local key="$2"
local value="$3"
local payload response
payload="$(jq -n \
--arg key "$key" \
--arg value "$value" \
--arg branch "$PR_BRANCH" \
'{key:$key, value:$value, type:"encrypted", target:["preview"], gitBranch:$branch}')"
if ! response=$(curl --fail-with-body -sS \
--connect-timeout 10 --max-time 60 \
-X POST \
-H "Authorization: Bearer ${VERCEL_TOKEN}" \
-H "Content-Type: application/json" \
"https://api.vercel.com/v10/projects/${project_id}/env?upsert=true&teamId=${VERCEL_ORG_ID}" \
--data "${payload}" 2>&1); then
echo "Failed to upsert env var ${key} for project ${project_id}:" >&2
echo "${response}" >&2
return 1
fi
}Refs:
| vercel inspect "${TARGET_DEPLOYMENT}" --token="${VERCEL_TOKEN}" --scope="${VERCEL_ORG_ID}" 2>&1 | tee /tmp/vercel-inspect.txt | ||
| echo "## Vercel Runtime Logs (API)" >> "$GITHUB_STEP_SUMMARY" | ||
| vercel logs "${TARGET_DEPLOYMENT}" --token="${VERCEL_TOKEN}" --scope="${VERCEL_ORG_ID}" --since=1h --no-follow 2>&1 | tail -n 200 |
There was a problem hiding this comment.
🟠 MAJOR: Runtime logs may expose secrets
Issue: The diagnostic step fetches Vercel runtime logs and outputs them to the workflow console and step summary. If the application crashes during startup due to configuration issues, stack traces may include environment variable values (especially database connection strings in driver errors).
Why: This is noted as a known issue in the PR description (500 FUNCTION_INVOCATION_FAILED). When debugging such failures, logs containing connection strings could be written to the publicly-viewable workflow summary.
Fix: Filter or redact the logs before outputting, or keep them as workflow artifacts with restricted access:
vercel logs "${TARGET_DEPLOYMENT}" --token="${VERCEL_TOKEN}" --scope="${VERCEL_ORG_ID}" --since=1h --no-follow 2>&1 \
| sed -E 's/(postgres(ql)?:\/\/[^@]+@[^:]+:[^\/]+)/[REDACTED]/gi' \
| sed -E 's/(ANTHROPIC_API_KEY|SPICEDB_PRESHARED_KEY|BETTER_AUTH_SECRET)=[^[:space:]]*/\1=[REDACTED]/g' \
| tail -n 200Refs:
| railway project list --json > /tmp/railway-projects.json | ||
| ENV_EXISTS="$( | ||
| jq -r \ | ||
| --arg project_id "${RAILWAY_PROJECT_ID}" \ | ||
| --arg name "${RAILWAY_ENV_NAME}" \ | ||
| '[.[] | select(.id == $project_id) | .environments.edges[].node | select(.name == $name)] | length' \ | ||
| /tmp/railway-projects.json | ||
| )" | ||
|
|
||
| if [ "${ENV_EXISTS}" = "0" ]; then | ||
| echo "Railway environment ${RAILWAY_ENV_NAME} does not exist; nothing to teardown." | ||
| exit 0 | ||
| fi | ||
|
|
||
| railway environment delete "${RAILWAY_ENV_NAME}" --yes |
There was a problem hiding this comment.
🟠 MAJOR: Teardown lacks error handling and verification
Issue: The teardown job doesn't check if railway project list succeeded (line 848), and doesn't verify that deletion actually completed (line 862). Unlike the provision job which wraps this in an if block (lines 222-232), the teardown assumes the command succeeds.
Why: If railway project list fails (network issues, auth expired), subsequent jq may return incorrect results, causing silent failures. If deletion fails, Railway environments leak and accumulate, increasing costs. The set -euo pipefail should catch complete failures but partial/malformed output could slip through.
Fix: Add explicit error handling consistent with the provision job:
if ! railway project list --json > /tmp/railway-projects.json; then
echo "Failed to list Railway environments for project ${RAILWAY_PROJECT_ID}" >&2
echo "Unable to verify teardown state; exiting to prevent orphaning."
exit 1
fi
# ... existing jq logic ...
if ! railway environment delete "${RAILWAY_ENV_NAME}" --yes; then
echo "Failed to delete Railway environment ${RAILWAY_ENV_NAME}." >&2
echo "Manual cleanup may be required via Railway dashboard or CLI." >&2
exit 1
fi
# Verify deletion
railway project list --json > /tmp/railway-projects-post.json
POST_EXISTS="$(jq -r --arg project_id "${RAILWAY_PROJECT_ID}" --arg name "${RAILWAY_ENV_NAME}" \
'[.[] | select(.id == $project_id) | .environments.edges[].node | select(.name == $name)] | length' \
/tmp/railway-projects-post.json)"
if [ "${POST_EXISTS}" != "0" ]; then
echo "Warning: Environment ${RAILWAY_ENV_NAME} may not have been fully deleted" >&2
fiRefs:
| PR_BRANCH="$( | ||
| curl -fsS \ | ||
| -H "Authorization: Bearer ${GH_TOKEN}" \ | ||
| -H "Accept: application/vnd.github+json" \ | ||
| "${GITHUB_API_URL}/repos/${WORKFLOW_REPO_FULL_NAME}/pulls/${PR_NUMBER}" | | ||
| jq -r '.head.ref // empty' | ||
| )" |
There was a problem hiding this comment.
🟡 Minor: Add explicit timeout to curl call
Issue: The curl call to GitHub API uses -fsS but no --connect-timeout or --max-time. If the API is slow or unresponsive, this could hang indefinitely until the job times out.
Why: Fast-fail behavior helps surface issues quickly rather than wasting runner time.
Fix: Add explicit timeouts:
| PR_BRANCH="$( | |
| curl -fsS \ | |
| -H "Authorization: Bearer ${GH_TOKEN}" \ | |
| -H "Accept: application/vnd.github+json" \ | |
| "${GITHUB_API_URL}/repos/${WORKFLOW_REPO_FULL_NAME}/pulls/${PR_NUMBER}" | | |
| jq -r '.head.ref // empty' | |
| )" | |
| PR_BRANCH="$( | |
| curl --connect-timeout 10 --max-time 30 -fsS \ | |
| -H "Authorization: Bearer ${GH_TOKEN}" \ | |
| -H "Accept: application/vnd.github+json" \ | |
| "${GITHUB_API_URL}/repos/${WORKFLOW_REPO_FULL_NAME}/pulls/${PR_NUMBER}" | | |
| jq -r '.head.ref // empty' | |
| )" |
Refs:
| for attempt in $(seq 1 "${max_attempts}"); do | ||
| line="$(printf '%s\n' "${SERVICE_ENV_DUMP}" | grep -m1 "^${key}=" || true)" | ||
| if [ -n "${line}" ]; then | ||
| value="${line#*=}" | ||
| if ! printf '%s' "${value}" | grep -q '\$[{][{]'; then | ||
| printf '%s' "${value}" | ||
| return 0 | ||
| fi | ||
| fi | ||
|
|
||
| if [ "${attempt}" -lt "${max_attempts}" ]; then | ||
| sleep "${sleep_seconds}" | ||
| refresh_service_env_dump | ||
| continue | ||
| fi | ||
| done |
There was a problem hiding this comment.
🟡 Minor: Consider exponential backoff for retry loop
Issue: The extract_runtime_var() function uses fixed 2-second delays (30 attempts × 2s = 60s total). When Railway experiences issues, concurrent workflow runs will retry at the same intervals, creating thundering herd patterns.
Why: Exponential backoff with jitter spreads retry traffic and reduces load on external services during incidents. This pattern is also duplicated in the configure-vercel-preview job (lines 447-478).
Fix: Consider adding exponential backoff:
# Inside the retry loop, replace fixed sleep with:
local delay=$((sleep_seconds * (2 ** (attempt / 5)))) # double every 5 attempts
delay=$((delay > 30 ? 30 : delay)) # cap at 30s
sleep $((delay + RANDOM % 3)) # add jitterRefs:
| echo "PR branch is required for preview deploy targeting." | ||
| exit 1 | ||
| fi | ||
| npm install -g vercel |
There was a problem hiding this comment.
💭 Consider: Pin Vercel CLI version
Issue: The Vercel CLI is installed via npm install -g vercel without version pinning (also at line 776).
Why: While the existing vercel-production.yml workflow uses the same pattern, unpinned global installs can lead to non-deterministic behavior if Vercel releases breaking changes.
Fix: Consider pinning to a specific version for reproducibility:
| npm install -g vercel | |
| npm install -g vercel@39 |
Refs:
- vercel-production.yml — uses same unpinned pattern
There was a problem hiding this comment.
PR Review Summary
(10) Total Issues | Risk: Medium
Delta Re-Review: This review covers only the 52 new lines added since the prior review. The prior review identified 8 issues that remain pending.
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
preview-environments.yml:673tsxdependency unavailable at runtime — probe will always fail
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
preview-environments.yml:661-667Cleanup not guaranteed on early exit
💭 Consider (1) 💭
💭 1) probe_api_env_schema Clarify probe_status usage intent
Issue: The probe_status variable is captured on line 686 (probe_status=$?) but is only used in the diagnostic echo. The probe function returns successfully regardless of whether schema validation passed or failed.
Why: It's unclear whether this is intentional "informational-only" behavior or a forgotten TODO. Future maintainers may not understand whether probe failure should block deployment.
Fix: Either:
- If intentionally non-blocking, add a comment:
# Note: probe failure is diagnostic only; schema errors caught at runtime - If it should block, add:
if [ "${probe_status}" -ne 0 ]; then exit 1; fi
Refs:
🕐 Pending Recommendations (8)
Issues from the prior review that remain unaddressed:
- 🟠
preview-environments.yml:636Actionactions/checkout@v6not pinned to SHA - 🟠
preview-environments.yml:305Docker imageghcr.io/railwayapp/cli:latestuses mutable tag - 🟠
preview-environments.yml:453-459Secrets written to GITHUB_OUTPUT before masking - 🟠
preview-environments.yml:606-612upsert_env discards error body and lacks timeout - 🟠
preview-environments.yml:924-926Vercel runtime logs may expose secrets in error traces - 🟠
preview-environments.yml:992-1006Teardown lacks error handling and verification - 🟡
preview-environments.yml:183-188GitHub API curl call missing explicit timeout - 💭
jobsAdd timeout-minutes to all jobs
🚫 REQUEST CHANGES
Summary: The new probe_api_env_schema() diagnostic function has a blocking issue: it uses tsx which isn't installed in this job. The probe will always fail, defeating its diagnostic purpose. Address the tsx dependency before merge. The prior review's 8 issues also remain pending — particularly the action SHA pinning and secret masking order.
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
657-658 |
mktemp/cp operations don't verify success | Low-probability edge case (tmp disk full); impact is minimal in ephemeral CI |
probe_status |
Variable captured but not returned | Reclassified as Consider — it's unclear whether this is intentional |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-errors |
3 | 0 | 1 | 0 | 1 | 0 | 1 |
pr-review-devops |
2 | 0 | 0 | 0 | 1 | 0 | 0 |
| Total | 5 | 0 | 1 | 0 | 2 | 0 | 1 |
Note: Delta review scope — only reviewed the 52 new lines in probe_api_env_schema(). Prior review findings are carried forward as Pending Recommendations.
|
|
||
| { | ||
| echo "## API Env Schema Probe" | ||
| if node --import tsx/esm -e ' |
There was a problem hiding this comment.
🟠 MAJOR: tsx dependency unavailable at runtime
Issue: The probe_api_env_schema() function uses node --import tsx/esm to validate env schemas, but tsx is a devDependency that requires pnpm install to be available. This job only runs npm install -g vercel (line 564) and does not install project dependencies.
Why: The probe will fail immediately with a module resolution error:
Error: Cannot find package 'tsx' imported from...
This defeats the diagnostic purpose — the probe will always report failure even when the env schema is valid, making it impossible to distinguish between actual schema issues and missing dependencies.
Fix: Either:
- Add pnpm setup and install (recommended if you want full TypeScript validation):
- name: Setup pnpm
uses: pnpm/action-setup@v4
- name: Setup Node.js
uses: actions/setup-node@v4
with:
node-version-file: '.nvmrc'
cache: 'pnpm'
- name: Install dependencies
run: pnpm install --frozen-lockfile- Use a simpler validation that doesn't require tsx (if you want to keep the job lightweight):
# Just check the files exist and are valid JS/TS syntax
node --check packages/agents-core/src/env.ts || echo "Syntax error in env.ts"Refs:
| cleanup_probe_env() { | ||
| if [ -n "${backup_env}" ] && [ -f "${backup_env}" ]; then | ||
| mv "${backup_env}" ".env" | ||
| else | ||
| rm -f ".env" | ||
| fi | ||
| } |
There was a problem hiding this comment.
🟡 Minor: Cleanup not guaranteed on early exit
Issue: The cleanup_probe_env() function is defined but not registered as a trap handler. If the script exits unexpectedly between line 669 (cp "${env_file}" ".env") and line 691 (cleanup_probe_env), the original .env file won't be restored.
Why: With set -euo pipefail, any unexpected error (e.g., tee failing on disk full) would exit before cleanup runs, leaving .env in an inconsistent state. While the CI environment is ephemeral, this could cause confusing failures if subsequent steps in the same job depend on .env.
Fix: Add a trap to ensure cleanup runs on exit:
cleanup_probe_env() {
if [ -n "${backup_env}" ] && [ -f "${backup_env}" ]; then
mv "${backup_env}" ".env"
else
rm -f ".env"
fi
}
trap cleanup_probe_env EXIT # Add this after defining the function
cp "${env_file}" ".env"
# ... rest of probe logic ...
cleanup_probe_env
trap - EXIT # Clear the trap after successful cleanupRefs:
There was a problem hiding this comment.
PR Review Summary
(8) Total Issues | Risk: Medium
Delta Re-Review: This review covers the changes since the prior review (commit
8ede9a12fa62→c4e58e57d). The delta removes the problematicprobe_api_env_schema()function (addressing the tsx dependency issue), adds production env defaults, and introduces alogEnvIssues()helper to 3 env.ts files.
✅ Delta Changes Look Good
The new commit addresses the blocking issue from the last review:
- Removed
probe_api_env_schema()— Fixes the MAJOR issue wheretsxwas unavailable at runtime - Added
NODE_ENV,ENVIRONMENT,LOG_LEVEL— Reasonable production defaults for Vercel preview - Added
logEnvIssues()helper — Improves error visibility when env validation fails - Extended debug keys — More comprehensive env snapshot for diagnostics
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
agents-api/src/env.ts:347-352DuplicatedlogEnvIssues()helper across three env.ts files
🕐 Pending Recommendations (8)
Issues from prior reviews that remain unaddressed:
- 🟠
preview-environments.yml:547Actionactions/checkout@v6not pinned to SHA - 🟠
preview-environments.yml:213Docker imageghcr.io/railwayapp/cli:latestuses mutable tag (also lines 438, 839) - 🟠
preview-environments.yml:357-367Secrets written to GITHUB_OUTPUT before masking - 🟠
preview-environments.yml:514-519upsert_env discards error body and lacks timeout - 🟠
preview-environments.yml:780-782Vercel runtime logs may expose secrets in error traces - 🟠
preview-environments.yml:848-862Teardown lacks error handling and verification - 🟡
preview-environments.yml:91-97GitHub API curl call missing explicit timeout - 💭
jobsAdd timeout-minutes to all jobs
🚫 REQUEST CHANGES
Summary: The delta changes are clean and address the prior tsx dependency issue. However, the 6 MAJOR and 2 other issues from the original review remain pending. The action SHA pinning and secret masking order are the most important items to address before merge.
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
logEnvIssues() |
Using console.error instead of structured logger |
Acceptable for bootstrap-phase code where logger may not be initialized yet. Matches existing pattern in env.ts files. |
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
2 | 0 | 0 | 0 | 1 | 0 | 1 |
| Total | 2 | 0 | 0 | 0 | 1 | 0 | 1 |
Note: Delta review scope — only reviewed the changes in commit c4e58e57d. Prior review findings (8 items) are carried forward as Pending Recommendations.
| const logEnvIssues = (scope: string, error: z.ZodError) => { | ||
| for (const issue of error.issues) { | ||
| const key = issue.path.length > 0 ? issue.path.join('.') : '<root>'; | ||
| console.error(`[${scope}] ${key}: ${issue.message}`); | ||
| } | ||
| }; |
There was a problem hiding this comment.
💭 Consider: Duplicated helper across three files
Issue: This identical logEnvIssues() function is added to three separate env.ts files (agents-api, agents-core, agents-work-apps).
Why: Creates maintenance burden — if the logging format needs to change, three files must be updated. The codebase already has a similar formatZodErrors() helper in agents-cli/src/utils/profiles/profile-manager.ts.
Fix: Consider extracting to a shared location in @inkeep/agents-core (e.g., packages/agents-core/src/utils/zod-helpers.ts). Alternatively, since this is small bootstrap-phase code that's unlikely to change, the duplication may be acceptable — just documenting the pattern.
Refs:
There was a problem hiding this comment.
PR Review Summary
(8) Total Issues | Risk: Medium
Delta Re-Review: This review covers the changes since the prior review (commit
c4e58e57d→5e47c0933). The delta adds git metadata flags to Vercel deployments (21 lines changed).
✅ Delta Changes Look Good
The new commit is a clean improvement:
- Added
PR_HEAD_SHA— Correctly sources fromgithub.event.pull_request.head.sha || github.sha - Added SHA validation — Guards against missing SHA before proceeding (lines 668-671)
- Added git metadata flags — Uses
-mflags to attach deployment metadata to Vercel:githubDeployment=1githubCommitRef,githubCommitShagithubOrg,githubRepo,githubCommitOrg,githubCommitRepo
This improves deployment traceability in Vercel's dashboard by linking deployments back to their source commits.
🕐 Pending Recommendations (8)
Issues from prior reviews that remain unaddressed:
- 🟠
preview-environments.yml:547Actionactions/checkout@v6not pinned to SHA — other workflows use SHA pinning (e.g.,actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd) - 🟠
preview-environments.yml:213Docker imageghcr.io/railwayapp/cli:latestuses mutable tag (also lines 438, 867) - 🟠
preview-environments.yml:357-367Secrets written to GITHUB_OUTPUT before masking — mask immediately after extraction for defense-in-depth - 🟠
preview-environments.yml:514-519upsert_envdiscards error body and lacks timeout - 🟠
preview-environments.yml:809-810Vercel runtime logs may expose secrets in error traces — consider filtering/redacting before output - 🟠
preview-environments.yml:876-890Teardown lacks error handling forrailway project listfailure - 🟡
preview-environments.yml:91-97GitHub API curl call missing explicit timeout - 💭
jobsAddtimeout-minutesto all jobs — other workflows (ci.yml) specify explicit timeouts
💭 Consider (1) 💭
💭 1) agents-api/src/env.ts:347-352 Duplicated logEnvIssues() helper
Issue: Identical logEnvIssues() function added to three env.ts files. This was noted in the prior review and remains — consider extracting to @inkeep/agents-core if this helper is expected to evolve.
Refs:
- agents-api/src/env.ts:347-352
- packages/agents-core/src/env.ts:110-115
- packages/agents-work-apps/src/env.ts:79-84
✅ Good Practices Observed (Cumulative)
The workflow demonstrates several security and operational best practices:
- Least-privilege permissions with
contents: readandpull-requests: write - Internal PR gating — preview environments only provision for internal PRs
- PR number/branch validation with regex guards
- Proper secret masking for database URLs (lines 361-363, 491-493)
- Safe environment naming with strict
^pr-[0-9]+$pattern enforcement - No use of dangerous triggers like
pull_request_target - Comprehensive diagnostics on smoke test failure
- Git metadata attachment (new in this delta) for deployment traceability
🚫 REQUEST CHANGES
Summary: The delta changes are clean and improve deployment traceability. However, the 6 MAJOR and 2 other issues from prior reviews remain pending. The most important items to address before merge are:
- Action SHA pinning (
actions/checkout@v6→ use SHA like other workflows) - Docker image tag pinning (
ghcr.io/railwayapp/cli:latest→ use specific version) - Secret masking order (mask immediately after extraction, before any other code runs)
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
1 | 0 | 1 | 0 | 0 | 8 | 0 |
| Total | 1 | 0 | 1 | 0 | 0 | 8 | 0 |
Note: Delta review scope — only reviewed the 21 new lines in commit 5e47c0933. Prior review findings (8 items) are carried forward as Pending Recommendations.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Re-Review: This review covers the changes since the prior review (commit
3ffb380f0→36e9eb458). The delta is a 2-line change that addresses the prior review's Minor finding.
✅ Prior Issue Addressed
The commit 36e9eb458 (ci: broaden preview log redaction) correctly addresses the Minor finding from the prior review:
| Prior Finding | How Addressed |
|---|---|
Incomplete secret coverage in redact_preview_logs() |
Replaced hardcoded list with generic regex: [A-Z_]*(SECRET|KEY|TOKEN|PASSWORD)[A-Z_]* |
Before:
-e 's#((ANTHROPIC_API_KEY|SPICEDB_PRESHARED_KEY|BETTER_AUTH_SECRET|...)[:=])[^\r\n[:space:]]+#\1[REDACTED]#g'After:
-e 's#([A-Z_]*(SECRET|KEY|TOKEN|PASSWORD)[A-Z_]*[:=])[^\r\n[:space:]]+#\1[REDACTED]#g'Why this is better:
- Comprehensive: Catches any uppercase variable containing SECRET, KEY, TOKEN, or PASSWORD
- Future-proof: No manual updates needed when new secrets are added
- Exact match to recommendation: Implements the suggestion from the prior review
✅ Good Practices Summary (Cumulative)
This workflow now demonstrates excellent security and operational practices:
- ✅ SHA-pinned actions —
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd - ✅ Version-pinned CLIs —
@railway/cli@4.31.0,vercel@50.32.5 - ✅ Mask-before-use pattern — Secrets masked immediately after extraction
- ✅ Comprehensive timeouts — All jobs have
timeout-minutes, all curl calls have--connect-timeoutand--max-time - ✅ Error handling —
upsert_env()captures response, teardown has verification loop - ✅ Secret redaction — Generic pattern covers all SECRET/KEY/TOKEN/PASSWORD variables + Bearer tokens + postgres URLs
- ✅ Least-privilege permissions —
contents: read,pull-requests: write - ✅ Internal PR gating — Preview environments only for internal PRs
- ✅ Safe naming patterns — Strict
^pr-[0-9]+$environment name validation
💭 Consider (1) 💭
💭 1) agents-api/src/env.ts:347-352 Duplicated logEnvIssues() helper
Issue: The identical logEnvIssues() function is present in three env.ts files.
Why: Minor maintenance overhead — but since this is small bootstrap-phase code unlikely to change, the duplication is acceptable. Flagged in prior review for awareness.
Refs:
- agents-api/src/env.ts:347-352
- packages/agents-core/src/env.ts:110-115
- packages/agents-work-apps/src/env.ts:79-84
✅ APPROVE
Summary: This PR is ready to merge. All 9 issues from prior reviews have been addressed. The workflow now has comprehensive security hardening (SHA-pinned actions, version-pinned CLIs, immediate secret masking, generic secret redaction), proper timeouts at both job and request levels, robust error handling with verification, and safe environment naming. The logEnvIssues() duplication is a minor consideration for future cleanup but does not block merge.
Note: I'm unable to submit a formal GitHub approval due to permission limitations, but this review recommends approval.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
1 | 0 | 1 | 0 | 0 | 0 | 0 |
| Total | 1 | 0 | 1 | 0 | 0 | 0 | 0 |
Note: Delta review — only reviewed the 2-line change in commit 36e9eb458. No subagent reviewers dispatched for this trivial delta.
Ito Test Report ✅13 test cases ran. 13 passed. ✅ Verification focused on included smoke, edge, and adversarial checks and found stable behavior for preview routing, auth challenges, and API resilience under expected conditions. ✅ Passed (13)
📋 View Recording |
There was a problem hiding this comment.
PR Review Summary
(5) Total Issues | Risk: Medium
Delta Re-Review: This review covers the changes since the prior approved review (commit
36e9eb458→39eac86c8). The delta extracts ~640 lines of inline bash from the workflow into 8 modular scripts under.github/scripts/preview/.
🟠⚠️ Major (1) 🟠⚠️
🟠 1) common.sh:29-40 Regression: Lost error handling for railway project list
Issue: The prior approved version had explicit error handling:
if railway project list --json > /tmp/railway-projects.json; then
...
else
echo "Failed to list Railway environments for project ${RAILWAY_PROJECT_ID}"
exit 1
fiThe extracted railway_env_exists_count() function lost this — it now silently proceeds if the Railway API call fails.
Why: When Railway authentication fails or the API is down, the error will be a generic "jq: parse error" rather than the informative message from the approved version. This is a functional regression.
Fix: Restore the error handling:
if ! railway project list --json > "${output_path}"; then
echo "Failed to list Railway environments for project ${project_id}" >&2
return 1
fiRefs:
Inline Comments:
- 🟠 Major:
common.sh:29-40Lost error handling for railway project list (regression)
🟡 Minor (3) 🟡
Inline Comments:
- 🟡 Minor:
common.sh:1-3Missingset -euo pipefailfor consistency - 🟡 Minor:
deploy-vercel-preview.sh:4-12Duplicatedrequire_env_vars()— consider sourcingcommon.sh - 🟡 Minor:
provision-railway.sh:22-26railway linkoutput discarded — harder to debug auth failures
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
fetch-vercel-runtime-logs.sh:2Add comment explainingset +eusage
💭 Prior Review Items (Still Valid) 💭
The following item from prior reviews remains applicable:
💭 1) agents-api/src/env.ts:347-352 Duplicated logEnvIssues() helper
Issue: The identical logEnvIssues() function is present in three env.ts files.
Why: Minor maintenance overhead — but since this is small bootstrap-phase code unlikely to change, the duplication is acceptable. Flagged in prior review for awareness.
Refs:
- agents-api/src/env.ts:347-352
- packages/agents-core/src/env.ts:110-115
- packages/agents-work-apps/src/env.ts:79-84
✅ Good Practices Observed
The script extraction maintains excellent patterns from prior reviews:
- ✅ SHA-pinned actions —
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd - ✅ Version-pinned CLIs —
@railway/cli@4.31.0,vercel@50.32.5 - ✅ Mask-before-use pattern — Secrets masked immediately after extraction
- ✅ Comprehensive timeouts — All jobs have
timeout-minutes, all curl calls have--connect-timeoutand--max-time - ✅ Secret redaction — Generic pattern covers all SECRET/KEY/TOKEN/PASSWORD variables
- ✅ Strict mode —
set -euo pipefailin all main scripts - ✅ Modular design — Clean separation of concerns into focused scripts
- ✅ shellcheck directives — Proper source annotations for static analysis
🚫 REQUEST CHANGES
Summary: The script extraction is well-structured and maintains most good practices from prior reviews. However, there is one regression: the error handling for railway project list was lost during extraction. This must be restored before merge. The other findings (missing strict mode in common.sh, duplicated require_env_vars, discarded railway link output) are minor improvements.
Note: Unable to submit formal REQUEST_CHANGES due to permission limitations, but this review identifies a blocking regression.
Discarded (8)
| Location | Issue | Reason Discarded |
|---|---|---|
provision-railway.sh:119-133 |
Retry loop uses fixed delay without jitter | Pre-existing pattern from approved commit; not introduced by this delta |
upsert-vercel-preview-env.sh:43-75 |
Retry loop uses fixed delay without jitter | Pre-existing pattern; not introduced by this delta |
teardown-railway.sh:36-45 |
Verification loop uses fixed delay | Pre-existing pattern; not introduced by this delta |
smoke-preview.sh:22-30 |
wait_for_success_url discards curl output | Pre-existing pattern; would require larger refactor |
deploy-vercel-preview.sh:121-132 |
Vercel deploy lacks timeout | Pre-existing; job timeout provides outer bound |
provision-railway.sh:22-26 |
Railway CLI lacks timeout | Pre-existing; job timeout provides outer bound |
provision-railway.sh:101-105 |
railway variable set output discarded | Pre-existing pattern; set -e will catch failures |
smoke-preview.sh:4-12 |
Duplicated require_env_vars | Covered by deploy-vercel-preview.sh finding |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
4 | 0 | 0 | 0 | 2 | 0 | 1 |
pr-review-sre |
6 | 0 | 0 | 0 | 0 | 0 | 6 |
pr-review-errors |
7 | 1 | 0 | 0 | 2 | 0 | 0 |
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 17 | 1 | 0 | 0 | 4 | 0 | 7 |
Note: Most SRE findings were discarded because they related to pre-existing patterns that were already present in the approved commit. The security and standards reviewers found no new issues in the delta. The key finding (railway_env_exists_count regression) was identified by the errors reviewer.
| railway_env_exists_count() { | ||
| local project_id="$1" | ||
| local env_name="$2" | ||
| local output_path="${3:-/tmp/railway-projects.json}" | ||
|
|
||
| railway project list --json > "${output_path}" | ||
| jq -r \ | ||
| --arg project_id "${project_id}" \ | ||
| --arg name "${env_name}" \ | ||
| '[.[] | select(.id == $project_id) | .environments.edges[].node | select(.name == $name)] | length' \ | ||
| "${output_path}" | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR: Regression — lost error handling for railway project list
Issue: The prior approved version (commit 36e9eb458) had explicit error handling:
if railway project list --json > /tmp/railway-projects.json; then
...
else
echo "Failed to list Railway environments for project ${RAILWAY_PROJECT_ID}"
exit 1
fiThis extracted version silently proceeds if the Railway API call fails. With set -euo pipefail in calling scripts, the failure should propagate, but the error message ("Failed to list Railway environments...") is lost, making debugging much harder.
Why: When Railway authentication fails or the API is down, the error will be a generic "jq: parse error" rather than the informative message that existed before. This is a functional regression from the approved version.
Fix:
| railway_env_exists_count() { | |
| local project_id="$1" | |
| local env_name="$2" | |
| local output_path="${3:-/tmp/railway-projects.json}" | |
| railway project list --json > "${output_path}" | |
| jq -r \ | |
| --arg project_id "${project_id}" \ | |
| --arg name "${env_name}" \ | |
| '[.[] | select(.id == $project_id) | .environments.edges[].node | select(.name == $name)] | length' \ | |
| "${output_path}" | |
| } | |
| railway_env_exists_count() { | |
| local project_id="$1" | |
| local env_name="$2" | |
| local output_path="${3:-/tmp/railway-projects.json}" | |
| if ! railway project list --json > "${output_path}"; then | |
| echo "Failed to list Railway environments for project ${project_id}" >&2 | |
| return 1 | |
| fi | |
| jq -r \ | |
| --arg project_id "${project_id}" \ | |
| --arg name "${env_name}" \ | |
| '[.[] | select(.id == $project_id) | .environments.edges[].node | select(.name == $name)] | length' \ | |
| "${output_path}" | |
| } |
Refs:
| #!/usr/bin/env bash | ||
|
|
||
| require_env_vars() { |
There was a problem hiding this comment.
🟡 Minor: Missing set -euo pipefail for consistency
Issue: All other scripts in this directory use set -euo pipefail on line 2, but common.sh omits it. While scripts that source it set strict mode themselves, the inconsistency creates a risk if common.sh is ever sourced in a different context.
Why: Defense-in-depth — ensures common.sh functions fail fast even if sourced before the calling script's strict mode takes effect.
Fix: Add after the shebang:
| #!/usr/bin/env bash | |
| require_env_vars() { | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| require_env_vars() { |
Refs:
| require_env_vars() { | ||
| local required | ||
| for required in "$@"; do | ||
| if [ -z "${!required:-}" ]; then | ||
| echo "Missing required configuration: ${required}" >&2 | ||
| exit 1 | ||
| fi | ||
| done | ||
| } |
There was a problem hiding this comment.
🟡 Minor: Duplicated require_env_vars() — consider sourcing common.sh
Issue: This script defines its own require_env_vars() function (lines 4-12), duplicating the implementation in common.sh. Three scripts (deploy-vercel-preview.sh, smoke-preview.sh, capture-preview-failure-diagnostics.sh) have this duplication, while four others correctly source common.sh.
Why: Duplicated utility functions can diverge over time. Centralizing in common.sh ensures consistent behavior and reduces maintenance burden.
Fix: Replace the local definition with:
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
# shellcheck source=.github/scripts/preview/common.sh
source "${SCRIPT_DIR}/common.sh"Refs:
| railway link \ | ||
| --project "${RAILWAY_PROJECT_ID}" \ | ||
| --service "${RAILWAY_OUTPUT_SERVICE}" \ | ||
| --environment "${RAILWAY_TEMPLATE_ENVIRONMENT}" \ | ||
| >/dev/null |
There was a problem hiding this comment.
🟡 Minor: railway link output discarded — harder to debug auth failures
Issue: The output of railway link is redirected to /dev/null. If Railway authentication or linking fails, subsequent commands will fail with misleading "missing variable" or "environment not found" errors rather than surfacing the actual auth/link failure.
Why: When railway link fails, the error is hidden and cascading failures produce confusing messages like "no project linked" without showing the root cause (e.g., invalid token, network error).
Fix: Remove stdout suppression and add explicit error handling:
echo "Linking Railway project ${RAILWAY_PROJECT_ID}..."
if ! railway link \
--project "${RAILWAY_PROJECT_ID}" \
--service "${RAILWAY_OUTPUT_SERVICE}" \
--environment "${RAILWAY_TEMPLATE_ENVIRONMENT}"; then
echo "Failed to link Railway project" >&2
exit 1
fiRefs:
- Same pattern appears in
teardown-railway.sh:18-22andupsert-vercel-preview-env.sh:37-41
| @@ -0,0 +1,22 @@ | |||
| #!/usr/bin/env bash | |||
| set +e | |||
There was a problem hiding this comment.
💭 Consider: Add comment explaining set +e usage
Issue: This script uses set +e (line 2) rather than set -euo pipefail like all other scripts in this directory. This appears intentional for diagnostic collection where partial output is valuable.
Why: The inconsistency may confuse future maintainers. A brief comment explaining the design choice would help.
Fix: Add explanatory comment:
| set +e | |
| #!/usr/bin/env bash | |
| # Diagnostic script - use set +e to allow partial output collection even if some commands fail | |
| set +e |
Refs:
Ito Test Report ✅18 test cases ran. 18 passed. ✅ Verification found the preview workflow behaviors that were exercised are consistent with the implementation, including input guards, protected UI status handling, diagnostics execution, and redaction behavior. ✅ Passed (18)
📋 View Recording |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Re-Review: This review covers the changes since the prior review (commit
39eac86c8→d4fb50c64). The delta is a single commit (ci: harden preview script extraction) with +31/-34 lines across 8 bash scripts.
✅ All Prior Issues Addressed
The commit d4fb50c64 correctly addresses all 4 findings from the prior review:
| Prior Finding | How Addressed |
|---|---|
🟠 Major: Lost error handling for railway project list |
✅ Added error handling to railway_env_exists_count() in common.sh:35-38 |
🟡 Minor: railway link output discarded |
✅ Added explicit error handling in provision-railway.sh:22-29, teardown-railway.sh:18-25, upsert-vercel-preview-env.sh:37-44 |
🟡 Minor: Duplicated require_env_vars() |
✅ Scripts now source common.sh instead of duplicating the function |
💭 Consider: Add comment explaining set +e |
✅ Added explanatory comment in fetch-vercel-runtime-logs.sh:2 |
✅ Good Practices Summary (Cumulative)
This workflow now demonstrates excellent security and operational practices:
- ✅ SHA-pinned actions —
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd - ✅ Version-pinned CLIs —
@railway/cli@4.31.0,vercel@50.32.5 - ✅ Mask-before-use pattern — Secrets masked immediately after extraction
- ✅ Comprehensive timeouts — All jobs have
timeout-minutes, all curl calls have--connect-timeoutand--max-time - ✅ Error handling —
upsert_env()captures response,railway_env_exists_count()fails explicitly,railway linkchecked in all scripts - ✅ Secret redaction — Generic pattern covers all SECRET/KEY/TOKEN/PASSWORD variables + Bearer tokens + postgres URLs
- ✅ Least-privilege permissions —
contents: read,pull-requests: write - ✅ Internal PR gating — Preview environments only for internal PRs
- ✅ Safe naming patterns — Strict
^pr-[0-9]+$environment name validation - ✅ Code consolidation — Shared helpers in
common.sh, no duplicated code
💭 Consider (1) 💭
💭 1) agents-api/src/env.ts:347-352 Duplicated logEnvIssues() helper
Issue: The identical logEnvIssues() function is present in three env.ts files (agents-api, agents-core, agents-work-apps).
Why: Minor maintenance overhead — but since this is small bootstrap-phase code unlikely to change, the duplication is acceptable. Flagged in prior reviews for awareness.
Refs:
- agents-api/src/env.ts:347-352
- packages/agents-core/src/env.ts:110-115
- packages/agents-work-apps/src/env.ts:79-84
✅ APPROVE
Summary: All 4 issues from the prior review have been correctly addressed. The workflow now has comprehensive security hardening, proper error handling throughout, and clean code organization with shared helpers in common.sh. Ready to merge. 🚀
Note: Unable to submit formal GitHub approval due to permission limitations, but this review recommends approval.
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
4 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-errors |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 4 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: All reviewers confirmed prior issues are correctly addressed. No new issues identified in the delta.
Ito Test Report ✅15 test cases ran. 15 passed. ✅ Verification completed with a code-first review of the non-passed outcomes; the included results are stable reachability, security, stress, and CI-path checks that remained consistent with the inspected local code paths. ✅ Passed (15)
📋 View Recording |
vnv-varun
changed the title
| const logEnvIssues = (scope: string, error: z.ZodError) => { | ||
| for (const issue of error.issues) { | ||
| const key = issue.path.length > 0 ? issue.path.join('.') : '<root>'; | ||
| console.error(`[${scope}] ${key}: ${issue.message}`); |
There was a problem hiding this comment.
@pullfrog Should we be using the logger utility instead of console error?
There was a problem hiding this comment.
console.error is intentional here — logEnvIssues runs at module load time during bootstrap, before the structured logger is initialized (the logger itself depends on env vars like LOG_LEVEL being parsed from this same schema). Using the logger would create a circular dependency. This matches the existing pattern across all three env.ts files (agents-api, agents-core, agents-work-apps).
|
Addressed review comment from @amikofalvy on Question: Should Answer: No code change needed. |
* ci: add preview env diagnostics * ci: probe preview env schema before deploy * ci: probe preview env schema before deploy * ci: harden preview api env defaults * ci: attach git metadata to preview deploys * ci: harden preview workflow operations * ci: broaden preview log redaction * ci: extract preview workflow scripts * ci: harden preview script extraction
2 participants
Summary
preview-environments.ymlworkflow that provisions a per-PR Railway environment frompreview-base, injects the resolved backend values into Vercel preview deployments, and aliases stable PR preview domainspr-<number>, and PR close tears that Railway environment down.github/scripts/preview/Key decisions
vercel deployso live CLI preview deployments receive the PR branch-scoped env vars they need at runtimeExpected behavior
pr-<PR number>cloned frompreview-basehttps://pr-<PR number>-api.preview.inkeep.comhttps://pr-<PR number>-ui.preview.inkeep.compr-<PR number>How we tested
d4fb50c64) passed end to end, includingProvision Tier 1 (Railway),Inject Vercel Preview Env Vars, andSmoke Test Preview URLsworkflow_dispatchrun23125912084also passedhttps://pr-2681-api.preview.inkeep.com/healthhttps://pr-2681-ui.preview.inkeep.comFuture considerations
Supersedes