chore: Sync Core - #719
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| "stream-chain": "2.2.5", | ||
| "stream-json": "1.9.1", | ||
| "structon": "^1.0.7", | ||
| "structon": "1.0.7", |
There was a problem hiding this comment.
High: pinning structon at exactly 1.0.7 permanently forecloses the harper#1441 fix that shipped in structon@1.0.8
To be precise about what this PR does and does not do: this is not a downgrade. main's lockfile already resolves structon@1.0.7, so the exposure below is pre-existing. What changes here is the escape route — ^1.0.7 would let any future lock regen or fresh install float up to 1.0.8; exact 1.0.7 removes that. After this merges, only a deliberate manual bump can pick the fix up.
Why it matters — the typed-struct CAS in RecordEncoder is inert with 1.0.7, not merely weakened.
harper-pro compiles the core encoder into its own dist (tsconfig.json includes core/resources/**/*) against a single root node_modules, so core/resources/RecordEncoder.ts resolves structon from this line. RecordEncoder extends StructonEncoder and installs the RocksDB override at core/resources/RecordEncoder.ts:311-354:
this.saveStructures = function (structures, isCompatible) {
if (typeof isCompatible == 'function') { if (!isCompatible(existingStructures)) return false; }
else if (existingStructures && existingStructures.length !== isCompatible) return false;
txn.putSync(sharedStructuresKey, structures);structon 1.0.7 builds the compatibility closure (prepareStructures attaches structures.isCompatible, dist/node.cjs:1252) but never hands it to the consumer — _saveTypedStructures() at dist/node.cjs:1426-1429 calls this.saveStructures(structures) with one argument and discards the return value.
So isCompatible arrives as undefined. The function branch is skipped, and the numeric guard compares existingStructures.length !== undefined. Once typed structs exist, prepareStructures persists the {named, typed} Map form (dist/node.cjs:1245-1250), and a Map (or its mapsAsObjects plain-object decode) has no .length — so undefined !== undefined is false, the guard does not fire, and txn.putSync runs unconditionally and returns true. No compatibility check executes at all on the typed path.
structon 1.0.8 fixes exactly this, and names the ticket in its own source:
// Forward isCompatible as the second arg, matching msgpackr's own pack call site ...
// without forwarding it, a saveStructures implementation that runs an optimistic CAS on the
// parameter (e.g. Harper's RocksDB override) sees `undefined` and a concurrent same-length
// save silently clobbers the previously persisted struct. See HarperFast/harper#1441.
return this.saveStructures(structures, structures.isCompatible);1.0.8 also adds a declined-save retry in _structonEncode (so a declined save stops returning bytes that reference an unpersisted structure id) and cbor-x SharedData handling in onLoadedStructures — without which, per its own comment, the object branch reads neither named nor typed and "silently clears the whole dictionary."
harper#1441 — "Typed-struct CAS clobber loses concurrent structure updates" is closed, but only the receiver half of its fix (the typeof isCompatible == 'function' branch) is deployed; the sender half is structon@1.0.8. Its production repro was Harper 5.1.6 on RocksDB on the Akamai v4→v5 stage cluster, and its stated impact is that reads surface as Data read, but end of buffer not reached, which RecordEncoder.decode's catch block logs and returns null for — laundering corruption as a missing record.
structon@1.0.8 published 2026-08-09, nine days before this PR. I searched harper and harper-pro: no open ticket tracks the bump.
Suggested fix: bump structon to 1.0.8 in harper (sync-core.sh copies core's dependency block wholesale, so that is the source of truth for this line) and re-run Sync Core. If the bump needs its own soak, that is fine — but file the follow-up before merging this, because the caret that could have caught it accidentally is gone afterwards.
—
Generated by Barber AI
| "dependencies": { | ||
| "@aws-sdk/client-s3": "^3.1012.0", | ||
| "@aws-sdk/lib-storage": "3.1101.0", | ||
| "@aws-sdk/lib-storage": "3.1106.0", |
There was a problem hiding this comment.
Medium: harper-pro inherits harper#2179's pins but not the check that enforces them
The pin sweep in this hunk is correct — I verified every pinned spec against both lockfiles and none sits below its resolved version, so the harper#2179 → harper#2195 mistake is not repeated here. The gap is elsewhere.
harper#2179 did two things: convert the ranges to exact pins, and add build-tools/check-shrinkwrap-pins.mjs (plus a 407-line unit test) to enforce them. Per that PR: Docker smoke "requires the root msgpackr and @harperfast/extended-iterable specs to be exact and byte-equal to rocksdb-js's requirements, verifies their installed root versions, and rejects a nested copy under rocksdb-js," so "a rocksdb-js automation or Renovate PR that moves only one side will therefore fail before it can merge."
Only the first half crosses over. build-tools/sync-core.sh copies core/package-lock.json to the root and merges core's dependency fields — it does not sync build-tools/. At this head, harper-pro's build-tools/ has prune-shrinkwrap-dev.mjs but no check-shrinkwrap-pins.mjs, and a repo-wide grep for check-shrinkwrap-pins across *.sh/*.yaml/*.yml/*.mjs returns nothing. harper-pro also generates its published shrinkwrap on its own path (build-tools/build-pro.sh:46-50 — npm shrinkwrap, then prune-shrinkwrap-dev.mjs), which the harper-side guard never observes.
So in the repo that actually ships the container, these pins are documentation rather than an enforced invariant: the module-identity guarantee harper#2179 was written to protect (one msgpackr, one @harperfast/extended-iterable, matching rocksdb-js's requirements, no nested copy) has no failing check behind it here.
Suggested fix: port check-shrinkwrap-pins.mjs into harper-pro's build-tools/ and wire it into build-pro.sh or a CI job — or extend sync-core.sh to sync that script alongside the dependency fields it already copies.
—
Generated by Barber AI
| "mocha": "^11.7.5", | ||
| "mqtt": "^5.15.1", | ||
| "openai": "^6.45.0", | ||
| "openai": "^7.0.0", |
There was a problem hiding this comment.
Low (no action needed): the openai major bump ^6.45.0 -> ^7.0.0 is safe — recording the check so nobody has to redo it
A major version auto-merging unreviewed is worth a second look, so here is the answer rather than a flag.
openai v7.0.0 has exactly one breaking change, per its release notes: "require Node.js 22 and codify version support" (openai/openai-node#2026). No API surface changed between 6.49.0 and 7.4.0. The installed 7.4.0 declares engines.node: ">=22.0.0"; harper-pro declares engines.node: "^22.22.2 || ^24.15.0 || >=26.0.0", pins .node-version to 24, and runs unit tests on 22/24/26. Every supported runtime already satisfies it.
Consumers: searched harper-pro root and the core submodule — the SDK has exactly one importer, core/integrationTests/server/v1-gateway.test.ts:281:
const { OpenAI } = (await import('openai')) as { OpenAI: new (opts: object) => any };
const client = new OpenAI({ apiKey: operation_token, baseURL: `${ctx.harper.httpURL}/v1` });
const stream = client.chat.completions.stream({ model: 'default', messages: [...] });
for await (const chunk of stream) { ... }baseURL points at Harper's own /v1 gateway, so this tests Harper's SSE compatibility rather than calling OpenAI. Production code does not use the SDK: Harper's own OpenAI backend is core/components/openai/index.ts, whose header states "Native fetch is used directly — no SDK dependency."
One thing worth knowing for future syncs, independent of this bump: harper-pro's checks do not execute any test under core/. test:unit is mocha ... 'unitTests/**/*.test.mjs' (harper-pro's own tree, .mjs only) and test:integration:all globs integrationTests/** (analytics, cloneNode, cluster, licensing, security, stress). Neither descends into the submodule. So core-side test coverage never gates a Sync Core PR — it gates the upstream harper PR instead (here, harper#2190, whose test:integration:all does glob integrationTests/**/*.test.ts and does exercise this file).
Correction (edited): an earlier revision of this comment described the test as calling stream.finalChatCompletion(). It does not — it iterates the stream with for await, as shown above. The conclusion is unchanged.
—
Generated by Barber AI
a467119 to
f740ab5
Compare
Sync core submodule and update package.json dependencies.
This PR was automatically created by the Sync Core workflow.
Auto-merge is enabled: this PR will merge automatically once
required status checks pass. If checks fail, the PR stays open
for manual review.