Skip to content

fix(nodejs): check every sqlx version and read persons through the replica url - #130062

Merged
trunk-io[bot] merged 1 commit into
masterfrom
fix/devex-node-schema-check-sqlx
Nov 6, 2026
Merged

trunk-io[bot] merged 1 commit into
masterfrom
fix/devex-node-schema-check-sqlx

Conversation

@webjunkie

@webjunkie webjunkie commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Changes

  • The manifest lists every sqlx version, and the check names each missing one.
  • Persons falls back to the read-only URL. Replica lag only adds wait.
  • Env keys are typed against the Node config, so a rename fails to compile.
  • sqlx stays in shadow mode until the logs show every production database recorded all versions.

How did you test this code?

Agent-authored. Not run against a production database.

Test rationale: new rows in existing tables: a late lower version (passes under the old rule), a replica-only persons URL, an empty versions list.

  • CI report comment density (4 comments): each states a reason the code cannot show, the version ordering or the typed keys.

Release status

  • No feature flag controls this change

Docs update

  • None.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Opus 5.5 (claude-opus-5-5)

Skills: /writing-tests, /writing-code-comments, /writing-pr-descriptions, /stacking-prs

More context for AI agents
  • Decisions: found by an adversarial review of the schema check; reviewed by a second Opus reviewer and Codex before the first push.
  • Follow-ups: enforce cyclotron-node and behavioral-cohorts once shadow logs show complete version rows; persons stays in shadow mode (long concurrent index builds); fix the alert noise of held deploys in charts first.
  • Read closely: resolveSqlxDatabaseUrl in cli.ts, evaluateSqlxRows in check.ts.

…plica url

sqlx versions are the timestamps in the migration file names, so a PR that
merges late can add a version below the highest one, and sqlx migrate run
still applies it. Checking only the highest version passed while such a
migration was missing. persons_migrations had two cases (66b4dd0,
1333170). The manifest now lists every version per database, and the
check reads them all in one query.

Most ingestion apps get only PERSONS_READONLY_DATABASE_URL, so the persons
check skipped on them. The CLI now falls back to the read-only URL. Replica
lag can only make it wait longer.

The env keys are typed against the Node config, so a rename there fails to
compile instead of turning the check into a silent skip.

The sqlx check stays in shadow mode. The first deploys show whether every
production database recorded all versions before it can block.
@trunk-io

trunk-io Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

😎 This pull request was merged.

@webjunkie
webjunkie added this pull request to stack #130064 November 5, 2026 13:25
@webjunkie webjunkie self-assigned this Nov 5, 2026
@webjunkie webjunkie changed the title fix/devex node schema check sqlx fix(nodejs): check every sqlx version and read persons through the replica url Nov 5, 2026
@github-actions

github-actions Bot commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

✅ Trunk lane — non-backend lane (node:ingestion)

This PR is assigned to the non-backend lane (node:ingestion). It does not run backend Python tests and may merge in parallel with PRs in other lanes.

✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

⚠️ Comment density — 6% of added code lines are comments (4 of 71)

This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.

Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.

Files with the most added comment lines:

File Comment lines Added lines
nodejs/src/schema-check/cli.ts 2 15
nodejs/src/schema-check/manifest.ts 2 11

This check does not block merging. It updates on every push and clears when the share drops.

@webjunkie
webjunkie marked this pull request as ready for review November 5, 2026 13:36
@parameterai

parameterai Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

Risk: No findings

This PR hardens the Node deploy schema check: sqlx requirements now list every migration version instead of only the highest, manifest parsing rejects empty or malformed version lists, and the persons check falls back to the read-only database URL for ingestion apps. All database access stays parameterized and read-only, and no URLs or credentials reach the log output, so the change introduces no security risk.

Sentinel reviewed fc3f9de · Review settings

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved.

Deployment tooling is risky territory, but current-head agent review and the author's STRONG familiarity provide assurance. No unresolved substantive concerns or other refusal grounds were found.

  • Author wrote 100% of the modified lines and has 0 merged PRs in these paths (familiarity STRONG).
  • PostHog Review (Standard) reviewed the current head: no findings.
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 65L, 5F substantive, 97L/7F incl. docs/generated/snapshots — within ceiling
tier ✓ T1-agent / T1c-medium (97L, 7F, single-area, fix)
stamphog 2.7.0 .stamphog/policy.yml @ fc3f9de · reviewed head fc3f9de

@trunk-io
trunk-io Bot merged commit efe6fbc into master Nov 6, 2026
322 of 323 checks passed
@trunk-io
trunk-io Bot deleted the fix/devex-node-schema-check-sqlx branch November 6, 2026 07:49
@trunk-io

trunk-io Bot commented Nov 6, 2026

Copy link
Copy Markdown

This pull request was merged into master as part of stacked PR 130063.

@deployment-status-posthog

deployment-status-posthog Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-11-06 08:15 UTC Run
prod-us ✅ Deployed 2026-11-06 08:37 UTC Run
prod-eu ✅ Deployed 2026-11-06 08:41 UTC Run

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