Repository navigation
fix(mysql): run the installed command, dedupe by instant, refuse out-of-range offsets - #50
Merged
Merged
Conversation
Two findings Greptile raised on the MongoDB destination that this package shares: - The entrypoint check compared `process.argv[1]` against `cli.js`, but npm installs the `bin` as a symlink named `workos-sync-mysql`, so the installed command imported `main()` and exited. Both sides are now resolved to real paths and compared. - `dedupeByKey` compared `updatedAt` as text, so a batch mixing offset and UTC forms could keep the older row. It now orders by the instant, and falls back to text for a value that is not a date, which the database refuses on the write anyway.
 [Medium impact] The PR appears safe to merge. SummaryThe PR fixes the installed MySQL command and timestamp comparisons.
No new actionable issues were found in the changes since the previous review. gjtorikian acknowledged the changed DDL and stated that nothing has shipped. Reviews (3) · Last reviewed commit: "fix(mysql): refuse an out-of-range offse..." · Reviewed by Greptile |
Date.parse reads three fractional digits and a DATETIME(6) keeps six, so two removals a microsecond apart tied and the later row in the batch won, whichever was newer. The rest of the fraction is read alongside it now.
The generated column range-checked the clock but not the offset, so `+00:99` was accepted and applied as 99 minutes. The offset's hours and minutes are now checked like the clock's. Nothing has shipped, so no recorded column DDL changes under a live deployment.
gjtorikian
changed the title
gjtorikian
added a commit
that referenced
this pull request
Nov 5, 2026
… out-of-range offsets The same three fixes Postgres (#49) and MySQL (#50) got. npm installs the bin as a symlink named after the command, which the old argv check did not recognise, so `npx workos-sync-clickhouse` imported the CLI and did nothing. Duplicate keys in a batch are now judged by the instant each timestamp names rather than by its text, so `01:00:00+01:00` no longer outranks `00:30:00Z`. And the timestamp projection range-checks the clock and the offset ahead of the best-effort parser, which read `24:00` as the next midnight and `+00:99` as a zone where the other destinations project NULL.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three bugs Greptile found in the MongoDB destination (#41) that
sync-mysqlshares, each with a test:process.argv[1]againstcli.js, but npm installs abinas a symlink namedworkos-sync-mysql, sonpx workos-sync-mysqlimportedmain()and exited.invokedDirectly()now resolves both the argv path and the module URL to real paths before comparing. The Docker image was unaffected, since it runsnode dist/cli.jsdirectly.dedupeByKeycompared timestamps as text, so a batch mixing offset and UTC forms (01:00:00+01:00vs00:30:00Z) could keep the older row on the delete, relink and unlink paths. It now orders by the instant at the precision aDATETIME(6)keeps (Date.parsereads only three fractional digits), falling back to text for a value that is not a date, which the database refuses on the write anyway.+00:99and applied them as 99 minutes. The offset's hours and minutes are now range-checked like the clock's. This changes the generated column DDL the package records in_columns, which would read as drift on an existing deployment; nothing has shipped, so there is none.