feat(api-sync): deterministic spec-driven sync patcher, replacing the AI pipeline - #36
Merged
Merged
Conversation
… AI pipeline Adds scripts/api-sync.php, a dependency-free PHP patcher that reconciles this SDK against the OpenAPI spec by state (spec property/enum presence vs the SDK class that models it), not by diffing old vs new spec revisions. State reconciliation catches drift regardless of when it appeared; the old-vs-new diff is used only for removal detection (hard fail) and version-bump classification. New curated files: - .api-sync/spec-map.json: spec schema/enum -> SDK class mapping, including the bank-account (10 rails) and customer (3 KYC/KYB variants) discriminator fan-outs, verified by hand against src/. - .api-sync/unmodeled.json: every spec property on a mapped schema that is currently absent from the SDK, each with a specific reason and owner, so --check stays green without silently masking real gaps. - .api-sync/known-divergences.json: enum value mismatches where the SDK's case value doesn't match the spec's (e.g. BankAccountType's 'savings' vs the spec's 'saving', EstimatedAnnualRevenue's extra-digit typo) -- recorded rather than blindly patched, since the fix is correcting the existing case, not adding a near-duplicate one. Applies the pending drift found by state reconciliation against the current spec: refund_wallet_address on the quote-create input, plus 3 enum gaps (BankingPartner.portage, Currency.EUR, BusinessIndustry NAICS 446120) that predate this change and were invisible to the old AI pipeline's diff-only approach. Version bumped 3.0.0 -> 3.1.0 (enum additions require minor). CI: new api-sync-check job in main.yaml (state check, map validity, determinism proof, non-blocking coverage report). api-sync.yml rewritten to run the patcher instead of claude-code-action, with CLAUDE_CODE_OAUTH_TOKEN removed entirely. api-sync-merged.yml gains an auto-tag-release job that tags and pushes v$VERSION after an api-sync PR merges, guarded against double-tagging and against tagging when VERSION didn't change. Claude-Session: https://claude.ai/code/session_01F1stiNzuNtJXoXtiW9ZCbs
…audit-types Three defects found by cross-repo review (the same patcher design ported to python/swift hit them too): - The snapshot refresh re-serialized the spec via json_decode/json_encode instead of copying bytes verbatim, which is semantically a no-op but rewrites indentation/escaping/key order -- every future sync PR would have carried an ~86k-line unreviewable snapshot diff. Now a raw file copy; regression-tested. - Property TYPE changes were not detected at all: a spec property could silently change from string to integer, or lose/gain nullability, with --check exit 0 and --apply reporting no changes. Adds a spec-declared-type vs SDK-declared-PHP-type comparison (string/integer/number/boolean/array, object for class/enum types, nullability from both the `["x","null"]` and `?T` forms), restricted to unambiguous 1:1 schema-to-class mappings (discriminator fan-outs are exempted: a flattened schema's nullability genuinely differs per rail). Nullability-only drift is checked old-vs-new (this codebase has pervasive, pre-existing nullable-vs-non-nullable divergence that predates every snapshot on record and isn't this patcher's job to re-litigate); base-category drift (string vs integer, enum constraint dropped to a bare string, etc.) is checked as current state, regardless of history. 3 genuine pre-existing mismatches found this way are recorded in known-divergences.json rather than silently accepted or blindly auto-fixed. - New optional constructor properties are appended strictly after every existing parameter (never inserted near related fields), since positional callers exist in PHP and a promoted property without a default following a defaulted one is a fatal error. Was already correct; added an explicit regression test asserting parameter order. Also adds `--audit-types`: a non-blocking, always-exit-0 mode doing the full state comparison (not just forward drift) of every mapped property's current spec type against its SDK type, wired into main.yaml next to the coverage report, so pre-existing type debt stays visible without gating CI. Findings triaged in this PR's report; none fixed here (Phase C). Claude-Session: https://claude.ai/code/session_01F1stiNzuNtJXoXtiW9ZCbs
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
BernardoSM
reviewed
Aug 3, 2026
…findings Snyk Code flagged 21 CWE-23 Path Traversal findings on scripts/api-sync.php, 10 shown inline on the PR. All 10 visible ones traced through a variable named $path used for two unrelated things in this file: an actual filesystem path in some functions, and a dotted JSON schema property path (e.g. "tracking_payment") in reconcileTypes()/computeStructuralDiff()/auditTypes() and their nestedProps()/nestedPropSchema() helpers. Renamed every non-filesystem occurrence to $schemaPath (and the URL-path lookup key in operationEnumValues() to $urlPath), so a same-name variable can no longer look like it flows into a file operation when it never does. For the genuine filesystem-path flows, added real validation rather than suppression: - resolveReadablePath()/resolveWritablePath(): --spec and --report are resolved via realpath() and rejected on a NUL byte or a missing file/ directory, right where they enter the program via parseArgs(), instead of trusted as raw strings at each later read/write call site. --report is deliberately NOT restricted to the repository root: CI writes it to /tmp and the determinism proof writes into scratch copies elsewhere on disk, both legitimate, required uses. - resolveWithinRoot(): every path built from a spec-map.json `file` entry (enum/class file edits, the VERSION file, the snapshot copy destination) is resolved and verified to stay inside the repository root, refusing to write outside it. spec-map.json is a committed, human-reviewed config file, not runtime input, but a corrupted or malicious entry should still never be able to direct a write outside the repo. Added tests for every new validation path (NUL byte rejection, missing file/directory, root containment, and that --report outside the root is still accepted since that's required functionality) and re-verified the determinism proof and full gate suite stay green. Also, from an independent review: added PaginationMetadata to spec-map.json (previously an unmapped coverage gap) and recorded two more LIVE, VERIFIED runtime defects in known-divergences.json -- next_page/prev_page are nullable string cursors on the wire but typed `int` here, so PaginationMetadata::fromArray() throws TypeError on any populated cursor (reproduced by execution, not theoretical). Strengthened the existing PayinOut.billing_fee_amount entry's wording the same way: reproduced the TypeError directly rather than describing it as a possibility. None fixed in this PR; a follow-up will address the underlying type changes. Claude-Session: https://claude.ai/code/session_01F1stiNzuNtJXoXtiW9ZCbs
Regression from the previous commit's path hardening: --spec is now resolved
to its canonical absolute form via resolveReadablePath() (realpath()) before
use, which is correct for security, but that path is also echoed into the
--report JSON's "spec" field. Writing report.json INSIDE each of the two
scratch copies meant the reports differed by the copies' own absolute
directory, failing the api-sync-check job's determinism diff even though the
actually-applied source code was identical.
Reports now write to sibling paths outside /tmp/api-sync-determinism/{a,b},
matching how this was exercised locally throughout development. The
determinism guarantee is about the applied code, not an invocation-specific
diagnostic that will always vary by which directory you happened to run in.
Claude-Session: https://claude.ai/code/session_01F1stiNzuNtJXoXtiW9ZCbs
…etic name is REQUIRED on CreateWalletIn (required: [network, name]), but CreateCustodialWalletInput's constructor only accepts customerId and network. Every custodial-wallet-creation call through this SDK omits a required field and the API rejects it -- custodial wallet creation is completely broken through this SDK today, not a minor gap. node, go and swift all send name; python and php do not. Reworded the unmodeled.json entry to say so plainly. Not fixed here: adding a required (non-defaulted) constructor parameter is a breaking API-shape change to CreateCustodialWalletInput, not a mechanical optional-field add. Claude-Session: https://claude.ai/code/session_01F1stiNzuNtJXoXtiW9ZCbs
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.
Design
Replaces the AI-driven
api-sync.yml(claude-code-action) with a deterministic patcher,scripts/api-sync.php. Full context:sdk-deterministic-sync-design.md/sdk-deterministic-sync-requirements.md(shared across all 5 SDK repos).State reconciliation, not event diffing. The patcher's primary pass checks, against the current spec only: every property of every mapped schema is present in its SDK class (or explained in
.api-sync/unmodeled.json), every mapped enum's spec members all exist as SDK case values (or explained in.api-sync/known-divergences.json), and every already-modeled property's SDK type is representationally compatible with its current spec type. This is deliberate: an old-vs-new diff would have missed every gap this PR found, because all of them (thesaving/savingstypo, the missingportage/EUR/NAICS446120enum members,refund_wallet_address) already existed identically in the previously-committedspec-snapshot.json; none are "new" in this spec delivery. The old-vs-new diff is still computed, but only for what state can't tell you: removal detection (always a hard fail), version-bump classification, and whether a nullability relaxation is genuinely new (see Type-mismatch detection below).Three curated files:
.api-sync/spec-map.json: spec schema/enum to SDK class, including the bank-account (10 rails) and customer (3 KYC/KYB) discriminator fan-outs, and the payout/payintracking_paymentnested-object case (no dedicated component schema exists for it in the spec).ignore.schemasdocumentsLedgerOperation(zero$refanywhere, an orphan), theRfifamily andupload/analyze(real, known, Phase-C gaps)..api-sync/unmodeled.json: every spec property on a mapped schema currently absent from the SDK, each with a specific reason and owner ([email protected]), so--checkis green without masking real gaps. Notably includes 14 payout-only and 3 payin-onlytracking_paymentfields (provider_reference, coelsa_id, etc.) that are deliberately deferred to a cross-SDK reviewed PR rather than patched here..api-sync/known-divergences.json: two kinds of recorded, reasoned, owned exceptions, both keyed by{schema, field}. First, enum members where the SDK's case value doesn't match the spec's (BankAccountType: spec issaving, SDK case value issavings, a live defect, not fixed here, since the fix is correcting the existing case rather than adding a near-duplicateSAVINGcase; same shape of bug onEstimatedAnnualRevenue, an extra digit). Second, genuine type-representation mismatches on already-modeled fields (see below).Reachability is structural, not list-based. The patcher computes the set of schemas transitively reachable from
paths+webhooksvia$reffor whatever spec it's given, and simply skips anything unreachable, so a schema that drops out of the public surface in some future spec revision stops generating work automatically, without needing anignore.schemasentry. 39 schemas in the current spec are unreferenced from any operation (billing/instance/invoice backoffice-only leftovers); none of them show up anywhere in this PR's reconciliation.Applicable vs needs-human vs known-divergence. Enum case additions and optional-field additions are mechanical and auto-applied. Field additions are always three coordinated edits at most: a promoted constructor property appended strictly after every existing parameter (never inserted near related fields, since positional callers are real in PHP and this codebase already has enough of them that reordering would be a breaking change in disguise; regression-tested), a
fromArraynamed-argument line (position-independent), and atoArrayline only when the class already has one, honoring its existing conditional-vs-literal include style. Removals, required-ness/type changes, new operations, new schemas, and unresolvable map anchors hard-fail with a precise message. Enum value mismatches and type-representation mismatches are a third category: recorded, not auto-patched, since blindly "fixing" either would create a confusing duplicate case or require a judgment call about which type is actually correct.Type-mismatch detection. Comparing the spec's declared JSON Schema type against the SDK's declared PHP property type: string to string, integer to int, number to float, boolean to bool, array to array, and object/enum to a named DTO or backed-enum class, plus nullability from both the
["x","null"]and?Tforms. Deliberately conservative: anything not positively known to be safe is a mismatch. A few representation pairs are recognized as genuinely safe rather than flagged: PHP's own int-to-float widening (safe even understrict_types), a spec date-time string decoded intoDateTimeImmutable(this SDK's established pattern), a specobjectmodeled as a PHParray(howjson_decode(..., true)represents it throughout this codebase), and a spec enum modeled as either a backed-enum class or a plain scalar (this SDK does both deliberately). Restricted to unambiguous 1:1 schema-to-class mappings: a discriminator fan-out's flattened schema has per-rail requiredness that genuinely diverges from the union of its classes, so type-checking a fan-out would be noise, not signal (structurally skipped, not silently ignored).Base-category drift (string vs integer, an enum constraint dropped to a bare unconstrained string, etc.) is checked as current state, regardless of history, the same principle as the rest of the design. Nullability-only drift is checked old-vs-new, deliberately narrower: this codebase has pervasive, long-standing
spec nullable / SDK non-nullabledivergence on plain scalars (see the audit below) that predates every snapshot on record, so re-litigating that entire backlog as a blocking gate on every run isn't what this patcher is for. A newly introduced nullability relaxation is a real, actionable signal though, and does hard-fail.--audit-types(new, non-blocking, always exit 0). A full state comparison, not just forward drift, of every already-modeled property's current spec type against its SDK type, for every 1:1-mapped class, wired into CI next to the coverage report. This exists because type-mismatch detection alone only catches drift going forward from a synced baseline; it doesn't tell you whether the SDK's current type surface already disagrees with the spec, which is exactly the blindness state reconciliation was built to remove for field/enum presence. Ran it against the current spec: 78 findings, all pre-existing (identical between the last two spec snapshots). Triage:QuoteOut.expires_atandCreatePayinQuoteOut.expires_at(specnumber, SDKint, no cast on the first one, so a genuinely fractional or null timestamp would throw), andPayinOut.billing_fee_amount(specnumber, SDK?stringwith no cast, so a present value would throw a TypeError today).created_at/updated_attoDateTimeImmutable, monetary amount fields declared as non-nullablefloat, nested DTOs and enum-backed properties declared non-nullable, plain strings/bools). This reads as a systemic, non-breaking spec-formalization pass (the same pattern already found forcreated_at/updated_atspecifically) rather than 75 independent new bugs. None fixed in this PR; recording each individually would be scope creep this task doesn't call for, and the audit output itself is the visibility mechanism. Good Phase C worklist: decide per-DTO whether to widen the PHP type or accept the (currently theoretical) risk.CreateBankAccountIn,CreateCustomerIn,CreateBlockchainWalletIn,PayoutOnEvmIn,PayoutOnEvmOut,CustomerOut) are reported as skipped, not silently omitted.What this PR applies
State reconciliation against the current public spec found 4 items of genuine pending drift (all pre-existing, none new to this spec delivery):
QuoteIn.refund_wallet_addresstoCreateQuoteInput(constructor +toArray; this class has nofromArray)BankingPartner::PORTAGE = 'portage'Currency::EUR = 'EUR'BusinessIndustry::NAICS_446120 = '446120'(inserted following the spec's own ordering, betweenNAICS_456110andNAICS_541511, matching the file's existing convention)Version bumped
3.0.0to3.1.0(enum additions require minor).CI
main.yamlgains anapi-sync-checkjob: state check against the committed snapshot (self-consistency check, since.api-sync/spec-current.jsonis deliberately not committed and only fetched transiently byapi-sync.yml), map validity, a determinism proof (apply into two scratch copies, diff byte-identical), a non-blocking coverage report of reachable-but-unmapped schemas, and the non-blocking--audit-typesreport.api-sync.ymlrewritten: keeps therepository_dispatch: [api-sync]trigger, dropsclaude-code-actionandCLAUDE_CODE_OAUTH_TOKENentirely, reads.api-sync/spec-current.jsonfromorigin/api-sync-data, runs the patcher, exits quietly on no changes, fails loudly on needs-human, else commits + refreshes the snapshot (a verbatim byte copy of the source spec file, never ajson_decode/json_encoderound-trip, since that would rewrite indentation/escaping/key order and turn every future sync PR into an ~86k-line unreviewable diff; regression-tested) + bumpsVERSIONon theapi-syncbranch, and opens/updates the PR with auto-merge enabled (assumes squash-merge; adjust if the repo's default differs).api-sync-merged.ymlgainsauto-tag-release: readsVERSIONfromsrc/BlindPay.phpafter anapi-sync-labeled PR merges, and tags+pushesv$VERSIONif the tag doesn't already exist and this merge actually changedVERSION(comparesHEAD~1, valid because the merge is a single squash commit).release.yamlalready triggers onv*.*.*tags, so this makes releases zero-touch.Security (Snyk Code)
code/snyk (blindpay)flagged 21 findings, all Path Traversal (CWE-23), all Medium severity, all inscripts/api-sync.php. All 21 fixed with real code changes; no suppression was used (Snyk Code doesn't support inline-comment or.snyk-file suppression the way Snyk Open Source does; narrow ignores go throughsnyk ignore createagainst the org's Snyk account, which I don't have credentials for on this repo, so a suppression wasn't an option here even for a genuine false positive).The 10 findings visible as inline PR comments (GitHub's review-comments API didn't return the other 11, but Snyk's own severity table confirmed all 21 are this same rule) split into two groups by root cause:
$pathwas used in this file for two unrelated things: an actual filesystem path in some functions, and a dotted JSON schema property path (e.g."tracking_payment") inreconcileTypes()/computeStructuralDiff()/auditTypes()and theirnestedProps()/nestedPropSchema()helpers, which never touch the filesystem at all. Snyk's data-flow model was following the identifier$pathacross these unrelated function scopes and reporting the schema-path usages as if they were the tainted filesystem path fromloadJson(). Renamed every non-filesystem occurrence to$schemaPath(and a similarly-named URL-path lookup key inoperationEnumValues()to$urlPath) so no variable name is shared between a real path and something that only looks like one. Fixing this alone would have addressed 9 of the 10 visible findings; the remaining one is covered by point 2.scanClassesDetailed()'sfile_get_contents, reachable from spec-map.json's per-classfileentries) and the other real filesystem-path sinks (--spec,--report, the VERSION file, the snapshot-copy destination) got real validation:resolveReadablePath():--specis resolved viarealpath()and rejected on a NUL byte or a missing file, once, right where it enters the program viaparseArgs()-- not re-trusted as a raw string at each later read.resolveWritablePath():--report's parent directory is resolved viarealpath()and must already exist. Deliberately not restricted to the repository root: CI writes it to/tmpand the determinism proof writes into scratch copies elsewhere on disk, both legitimate, required uses, not a vulnerability.resolveWithinRoot(): every path built from aspec-map.jsonfileentry (enum/class-file edits, the VERSION file, the snapshot-copy destination) is resolved and verified to stay inside the repository root, hard-failing otherwise.spec-map.jsonis committed and human-reviewed, not runtime input, but a corrupted or malicious entry should still never be able to write outside the repo -- andvalidateMap()independently catches the same scenario a different way (the named class must actually be declared at the claimed file), so this is defense in depth, not the only guard.Result:
code/snyk (blindpay)now reports "No new Code Analysis issues found." Tests added for every new validation path (NUL-byte rejection, missing file/directory, root-containment refusal, and that a--reportdestination outside the root is still accepted).One regression this surfaced and also fixed:
resolveReadablePath()'s canonical (absolute) form of--specgets echoed into--report's diagnostic JSON, so writing that report inside each of the determinism proof's two scratch copies made the reports differ by the copies' own directory even though the applied source code was identical. Fixed by havingmain.yaml's determinism step write both reports to sibling paths outside the compared trees, matching how this was exercised locally throughout development.Proof
composer install (local PHP is 8.5, package targets
^8.2, used--ignore-platform-req=php):composer run lint:check:
composer run test (34 new tests in
tests/ApiSync/ApiSyncTest.php, covering enum insertion and spec-order anchoring, the three-part field insertion, the two-part case for classes withouttoArray, literal-vs-conditionaltoArraystyle detection,unmodeled.json/known-divergences.jsonhonoring, idempotency, every NEEDS_HUMAN classification including type mismatches (string to integer, nullable to non-nullable, an enum-backed property degrading to a bare string) and one deliberately-compatible pairing (int to float), that a new constructor parameter always lands last with every existing parameter's order unchanged, map validity, bump classification,--audit-typesreporting a pre-existing mismatch--checkwould not flag while marking a recorded divergence rather than hiding it, and that the refreshed snapshot is a byte-identical copy of the source spec, never a re-serialization):(the 77 "deprecated" are pre-existing PHP-8.5
ReflectionProperty::setAccessible()notices in the existing test suite, unrelated to this change)php scripts/contract-check.php:
php scripts/api-sync.php --check (against the committed snapshot, post-apply):
Determinism proof (apply into two independent scratch copies of the pre-apply tree):
Also verified idempotent (running
--applyagain on an already-applied tree makes zero further changes) and that the refreshed snapshot is byte-for-byte identical to the source spec file passed via--spec.https://claude.ai/code/session_01F1stiNzuNtJXoXtiW9ZCbs