o1-labs / o1-labs/Archive-Node-API
Merge order and three silent conflict hazards across the production-readiness PR batch
Nobody has claimed this yet.
- 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 outbuildYoga. - #190 replaces
logging: LOG_LEVELwith an explicitYogaLoggerobject (its fix forLOG_LEVEL=fatalemitting 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:
- #194 →
node:22-alpine@sha256:16e22a55… - #189 →
node: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 error — provider 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: falsefor 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 mechanicalupdate-branchre-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
-
strictrestored totrueif it was temporarily disabled -
npm run build && npm run test:unitgreen onmainafter the final merge
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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