o1-labs / o1-labs/Archive-Node-API

Merge order and three silent conflict hazards across the production-readiness PR batch

Open
#211 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

P0 production-readiness
Dominant language
TypeScript
Stars
19
Forks
9
Avg merge
14h 20m
Merged PRs (30d)
8

Description

Context

17 non-draft production-readiness PRs are approved and individually MERGEABLE/CLEAN against main @ 6b72e5b. They are not independent of each other: git merge-tree across all 18 open head refs finds 37 conflicting pairs.

Conflict degree (number of other open PRs each collides with):

#193 -> 10   #190 -> 9   #183 -> 7   #184 #185 #187 #191 -> 6
#194 #195 #196 -> 4   #186 #188 #198 -> 2   #182 #189 -> 1
#134 #192 #197 -> 0

Hub files: docs/getting-started.md, src/server/plugins.ts, src/server/server.ts, package.json / package-lock.json, README.md, .env.example.compose, src/envionment.d.ts.

Most are append-only doc/env collisions — resolve by taking both sides. Three are not, and a careless resolution is silent in all three: no test fails, no error is raised, and a defect that was just fixed comes back.

This issue exists so whoever performs the merges has the order and the hazards written down.


Hazard 1 (SILENT) — src/server/server.ts, #190 vs #195

Both edit the same createYoga options object.

  • #195 adds maskedErrors: { isDev: false } and splits out buildYoga.
  • #190 replaces logging: LOG_LEVEL with an explicit YogaLogger object (its fix for LOG_LEVEL=fatal emitting debug output).

Either one-sided resolution re-breaks the other. Dropping #195's maskedErrors is undetectable in CI: its own test suite still passes, because with NODE_ENV unset the behaviour is identical to main. The only thing that comes back is the NODE_ENV=development leak, where main puts the full Postgres connection string including the password into extensions.originalError — and nothing in CI sets NODE_ENV=development.

Resolution: merge #195 before #190. The merged options object must retain all three changes:

function buildYoga(context: GraphQLContext, plugins: Plugin[]) {
  return createYoga<GraphQLContext>({
    schema,
    logging: {                        // from #190
      debug: yogaLog('debug'), info: yogaLog('info'),
      warn:  yogaLog('warn'),  error: yogaLog('error'),
    },
    graphqlEndpoint: '/',
    landingPage: false,
    healthCheckEndpoint: '/healthcheck',
    graphiql: process.env.ENABLE_GRAPHIQL === 'true' ? true : false,
    maskedErrors: { isDev: false },   // from #195 — do not drop
    plugins,
    cors: { origin: process.env.CORS_ORIGIN ?? '*', methods: ['GET', 'POST'] },
    context,
  });
}

Acceptance check after resolving: with NODE_ENV=development, a resolver that throws an error containing a DSN must return Unexpected error. with no extensions.originalError.


Hazard 2 (SILENT) — Dockerfile, #189 vs #194

Both rewrite the same two FROM lines and disagree on the base:

  • #194node:22-alpine@sha256:16e22a55…
  • #189node:20-alpine@sha256:fb4cd12c…

Both digests are genuine multi-arch official images. But Node 20 reached EOL on 2026-04-30, and #194 also adds engines: { "node": ">=22.12.0" }, which #189's base does not satisfy. Resolving toward #189 silently enshrines an EOL runtime that contradicts the package's own declared floor.

Resolution: merge #194 first, then rebase #189 onto it. Keep #194's digest, layer #189's hardening on top. The full merged Dockerfile is in the #189 review comment. apk add --no-cache tini and BusyBox wget were both re-tested on the node:22 base and behave identically, so the combination is safe.

Acceptance check: docker run the built image, confirm ps shows 1 tini / 3 node, confirm SIGTERM reaches node, and confirm HEALTHCHECK reports healthy.


Hazard 3 (SILENT) — src/server/plugins.ts, #188's signature change

#188 changes buildPlugins() from return plugins to return { plugins, provider } so the entry point can flush OpenTelemetry spans on shutdown, and updates its caller accordingly. Six other PRs edit the same function and still return plugins.

A resolution that keeps the old form drops trace flushing on shutdown with no errorprovider is simply undefined and spans are silently discarded.

Resolution: merge #188 on its own, then rebase the remaining plugins.ts PRs onto the new signature. Do not interleave it.


Non-hazard (recorded so it is not re-litigated)

#183, #185, #190 and #191 each insert their plugin at index 0 of buildPlugins, each with a comment asserting it "runs ahead of everything else". This is a textual conflict only.

It was initially suspected that ordering #185 (rate limiter) before #191 (metrics) would re-break #191's http_requests_in_flight gauge, since the limiter short-circuits with a 429. It does not. #191's guard is:

const start = startTimes.get(request);
if (start === undefined) return;   // no inc => no dec
startTimes.delete(request);
metrics.inFlight.dec();

No-inc implies no-dec, so both orderings stay balanced. Verified in source and by execution. All four plugins are onRequest/onResponse-only or validate-phase, so the order is semantically free. Resolve textually, keeping all four insertions.


Recommended merge order

Wave PRs Note
1 #134, #192 zero conflicts — bank them
2 #182 resolve .gitignore toward the fully anchored /db/ + /data/ form
3 #194 then #189 Node 22 before the digest pin (Hazard 2)
4 #195 then #190 maskedErrors before the logger swap (Hazard 1)
5 #188 signature change alone, then rebase the rest (Hazard 3)
6 #191, #185, #183, #184, #187, #193 textual conflicts only
7 #186 (once unblocked), #196, #197, #198 docs that reference everything above

Branch-protection prerequisite

Current settings:

required status checks : Run-Tests, Linting
strict                 : TRUE
required approvals     : 1
dismiss_stale_reviews  : TRUE
enforce_admins         : FALSE

With strict: TRUE, every merge puts the remaining PRs BEHIND. The update-branch that fixes that is itself a push, and with dismiss_stale_reviews: TRUE that dismisses the approval. 16 approvals today does not produce 16 merges without re-approving each one after every preceding merge.

Two options before starting:

  • (a) Recommended — temporarily set strict: false for the duration of the merge train, keeping the two required checks and the 1-approval rule. Reversible, and it does not weaken what is actually verified.
  • (b) Use the admin bypass (enforce_admins: false) for the mechanical update-branch re-approvals.

Real content conflicts still need resolving under either option.


Acceptance criteria

  • All 17 non-draft PRs merged, or explicitly deferred with a reason
  • Hazards 1–3 each verified by their acceptance check above after resolution
  • strict restored to true if it was temporarily disabled
  • npm run build && npm run test:unit green on main after the final merge

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with src/server/server.ts, src/server/plugins.ts, Dockerfile, package.json and package-lock.json, then review the referenced PRs and their acceptance checks. Run npm run build && npm run test:unit and the stated Docker and NODE_ENV=development checks after the merge sequence. Done means all 17 PRs are merged or deferred with reasons, hazards are verified, and strict protection is restored if changed.

Written by the indexing model from the issue text.

Assessment

Tech stack
docker, git, github, node.js, typescript
Domain
backend, ci-cd, devops, release
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
20/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.