Skip to content

fix: resolve duplicate migration 0043 numbering#402

Closed
0xSolace wants to merge 4 commits intodevfrom
fix/migration-numbering
Closed

fix: resolve duplicate migration 0043 numbering#402
0xSolace wants to merge 4 commits intodevfrom
fix/migration-numbering

Conversation

@0xSolace
Copy link
Copy Markdown
Collaborator

Problem

Two migration files share the number 0043:

  • 0043_add_missing_referral_context_columns.sql
  • 0043_seed_chain_data_pricing.sql

This blocks clean migration execution for dev→prod promotion.

Fix

Renumbered the second 0043 to 0044 and cascaded all subsequent migrations (+1). Final range: 00430053 (sequential, no gaps, no duplicates).

Verification

  • No TypeScript code references migration numbers directly
  • Migration content unchanged, only filenames renumbered

- Add isAuthenticationError() helper for proper error classification
- Improve try/catch coverage in agent creation and pairing flows
- Update tests for new error handling paths
The second 0043 (seed_chain_data_pricing) is renumbered to 0044,
and all subsequent migrations are cascaded +1.

Before: two files numbered 0043
After: sequential 0043-0053 (no gaps, no duplicates)
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 22, 2026

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2804355e-7560-4d79-b8ff-6febf5856f80

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/migration-numbering

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

CodeRabbit can approve the review once all CodeRabbit's comments are resolved.

Enable the reviews.request_changes_workflow setting to automatically approve the review once all CodeRabbit's comments are resolved.

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 22, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
eliza-cloud-v2 Ready Ready Preview, Comment Mar 22, 2026 6:06pm

@claude
Copy link
Copy Markdown

claude Bot commented Mar 22, 2026

Code Review

Critical: _journal.json not updated

The PR renames SQL files (0043_seed → 0044, cascading to 0053) but does not update packages/db/migrations/meta/_journal.json. Drizzle tracks applied migrations by their filename tag in the journal, not by file content.

The journal currently contains both:

  • idx 42: 0043_seed_chain_data_pricing
  • idx 43: 0043_add_missing_referral_context_columns

After renaming the SQL files, Drizzle will:

  1. Still see both old 0043_* tags as "applied" in the journal
  2. See 0044_seed_chain_data_pricing, 0045_add_whatsapp_identity_columns, ..., 0053_add_milady_billing_columns as new, unapplied migrations
  3. Attempt to re-apply all of them on any environment that has already run past 0043

For seed data migrations this means duplicate rows. For structural migrations (0047_docker_nodes, etc.) this means CREATE TABLE / ALTER TABLE failures unless they have IF NOT EXISTS guards. This can break production on next deploy.

The journal entries need to be updated to use the new tags. Drizzle doesn't currently have a first-class rename command — the approach is to manually edit _journal.json to replace the old tags with the new ones, or to generate a no-op custom migration that re-establishes the sequence.


Bug: isAuthenticationError() string-matching is fragile (app/api/v1/milady/agents/route.ts)

function isAuthenticationError(message: string): boolean {
  return (
    message.includes("Unauthorized") || ...
  );
}

Any downstream error (DB query, sandbox service) whose message happens to contain "Unauthorized" will return a 401 instead of a 500. This masks real errors and makes debugging harder. Auth errors thrown by requireAuthOrApiKeyWithOrg should be typed/subclassed, not identified by string content.

Also, the 500 branch returns the raw error.message to the caller:

{ success: false, error: authError ? "Unauthorized" : errorMessage }

Internal error messages (stack details, DB query fragments, etc.) shouldn't be surfaced to API consumers. Return a generic message and log the detail server-side only.


Bug: Incomplete dot-escaping in domain alias regex (packages/lib/services/pairing-token.ts)

url.hostname.replace(new RegExp(`${a.replace(".", "\\.")}$`), b)

String.prototype.replace(string, replacement) only replaces the first occurrence. For a = ".waifu.fun":

  • a.replace(".", "\\.")"\\.waifu.fun" (only the leading dot is escaped)
  • The . in .fun remains unescaped in the regex, matching any character

Use a.replace(/\./g, "\\.") to escape all dots:

url.hostname.replace(new RegExp(`${a.replace(/\./g, "\\.")}$`), b)

Minor: Missing test for POST auth error handling

The new test suite covers GET returning 401 on auth failure, but the POST handler has the same try/catch pattern and isn't tested. Add a parallel test for POST to match.


Observation: Python3 dependency for Steward registration

registerAgentWithSteward executes an inline Python3 heredoc over SSH. This is functional but ties the provisioning path to Python3 being available on every Docker node. Worth documenting this requirement in docs/steward-container-provisioning.md or verifying it's guaranteed by the node image.


Migration renaming: valid approach, wrong execution

The intent (fix duplicate 0043 numbering) is correct and necessary. The execution needs the journal update to be safe. Suggest either:

  1. Edit _journal.json to rename the duplicate 0043_seed_chain_data_pricing tag to 0044_seed_chain_data_pricing (and cascade 0044→0045 through 0052→0053), and verify no environment has already applied the old numbered files as new entries
  2. Alternatively, only rename the duplicate 0043_seed_chain_data_pricing → 0044_seed_chain_data_pricing and leave all subsequent files untouched (the gap doesn't affect Drizzle execution)

@0xSolace
Copy link
Copy Markdown
Collaborator Author

Combined into #403 (fix/steward-security-migrations)

@0xSolace 0xSolace closed this Mar 22, 2026
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