Repository navigation
Classify SDK dependencies by their pubspec source, not a name list - #72
Conversation
|
@greptileai review |
…nd sdkPackages Review follow-ups on STU-368. readSourceDirective returned the first own-key, but YAML mapping order is not significant: 'version:' written above 'hosted:' read as a plain constraint and the manifest resolved the package from pub.dev rather than the project's host. Every own-key is now read and any source key wins over 'version:'. extractPackageImports matched only 'import'. A class exporting a git or path package was compiled against a manifest missing it and the analyzer refused the deploy over an unresolvable URI, instead of the class being skipped with a warning. 'export' directives now count. The runner's sdkPackages array had no count bound after the allowlist was removed; it now shares maxDependenciesPerRequest, and package names in every manifest field are capped at pub's 64-character limit. Co-Authored-By: bot_apk <apk@cognition.ai>
|
@greptileai review |
Review follow-up on STU-368. Per-class scoping assumed an unreproducible package only affects classes that name it directly. That holds for a git/path entry in dependencies - a published package cannot depend on an unpublished one - but not for dependency_overrides: an override rewrites resolution for every package in the graph that depends on the overridden name. A class importing an ordinary pub.dev package could then be analysed against different transitive code than the project compiles, and only pubspec.lock - which a deploy never sees - records whether it does. Unrepresentable overrides now skip any class that imports a package: URI at all; dart:-only classes still verify. Regular unreproducible dependencies keep their per-class scoping. Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
Review follow-ups on STU-368.
A dependency written as a flow mapping - 'flutter_web_plugins: {sdk:
flutter}' - has a nonempty value, so it read as a scalar constraint and
the braces were forwarded to the runner, which rejects them and refuses
the whole deploy. The parser now reads a flow mapping's own keys with the
same source-over-version precedence as block form, and an unreadable
mapping is unrepresentable rather than a constraint.
Import/export directives can select a different file per platform with
'if (dart.library.x)': every URI literal in the directive is code the
class needs. The package scan now reads them all, so a conditional
import of an unreproducible dependency leaves the class uncompiled and
reported instead of refused on a missing URI.
Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
|
@greptileai review |
|
Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
… correctly Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
…cation Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
|
E2E deploy-pipeline verification — drove the real app UI ( Run A — unreproducible Exactly 4 "Not verified before deploying" warnings with correct discriminating reasons:
Run B — representable override (
Captured payloads (details)Run A Not exercised: real |
…n shift the graph Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
…arison literals in conditions Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
…r raw prefix Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
…version scalars Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
…lose Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
…ontent Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
…ed member rewrite as skipped Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
…n't match Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com>
|
Duplicate of the resolved thread — fixed in 43a1a02: entries record the name line as |
…ui-ux' into fix/stu-368-sdk-source-classification Co-Authored-By: Stuart Gardoll <sgardoll@gmail.com> # Conflicts: # dist/index.html # src/customCodeVerification.js # src/customCodeVerification.test.js # src/pubspecSync.js
b69afe7
into
milestone/stu-implement-updated-ui-ux
Closes STU-368. Ends the trade the a1238f0 revert re-opened: after this, an unreproducible dependency no longer refuses the deploy — the classes it can affect are reported unverified and the rest still compile for real.
The bug
Which packages the Flutter SDK supplies was decided by a hand-maintained name list kept in three places.
flutter_web_pluginswas in none of them, so every project declaring it — every web-enabled project with a plugin — read it as a dependency that could not be reproduced, and the deploy was refused for a package most of its classes never imported.Changes
The pubspec already held the answer. The parser read each block-form dependency's own keys and threw them away. It now returns them as
sourceKey/sourceValue:sdk:version:git:/path:/ nestedhosted:SDK_DEPENDENCIESis deleted. Nothing classifies by package name any more, so the next SDK package Flutter ships needs no change here.The runner's
allowedSdkPackagesis deleted too. It independently refused the same deploy withUnsupported SDK package: .... One safety note: that allowlist was also the only validation of ansdkPackagesentry, so the package-name pattern now does that job. Without the swap, a caller-supplied name would reach the generated pubspec unchecked — a YAML-injection path. The pattern was already there and already the guard on dependency keys; it is now the guard here too. The runner still writessdk: flutteritself (hardcoded), so that value never comes from a caller.Skip is scoped by reachability — at the graph level, not the import level. A class naming an unreproducible entry directly gets the precise "it imports X" reason. But review rounds established that per-import scoping is unsound: a git/path dep's own pubspec still constrains every package it shares with a class's imports, and a
dependency_overridesentry rewrites resolution for every dependent — so any unreproducible entry skips every class importing apackage:URI.dart:-only classes resolve nothing through pub and still verify. Which packages drift is only recorded in the project'spubspec.lock, never pushed, so no narrower rule can be trusted.Unchanged, deliberately: the accept/warn split already worked. A real analyzer error refuses with HTTP 422; a class that could not be compiled is pushed with its warning logged before the provisioning request. STU-368's original acceptance text said such a class "is refused" — that described the a1238f0 behaviour we reverted, so the issue is updated to match the agreed policy rather than the code being bent back to it.
Doc fix:
DEPLOYMENT.mdstill described the scaffolding-closure compilation that revertinga1238f0removed, and still claimed "nothing unverified is deployed". Corrected.Review follow-ups
Five review rounds (Devin Review + Greptile) landed further fixes on top of the classification change:
readSourceDirectiveread only the first own-key, but YAML mapping order is not significant —version:written abovehosted:classified a custom-hosted package as a pub.dev constraint. Every own-key is now read and a source key wins overversion:.extractPackageImports/extractImportUrismatched only the first URI of animport, missingexportdirectives andif (...)conditional alternatives. Both now scan the whole directive to its;and read every URI literal.;inside a quoted URI (src/a;b.dart) and escaped quotes ('a\'b.dart',"it's.dart") each truncated or dropped the directive — losing the URI and sending an unverifiable class to the analyzer as a missing-file error. Literals are now stepped over whole, escape-aware, and each quote type only forbids its own delimiter.sdkPackageslist was unbounded once the allowlist went away. It is now capped atmaxDependenciesPerRequest, names are limited tomaxPackageNameLength, and dedup uses a set.name: {sdk: flutter}) read as scalar constraints and were forwarded to the runner — whose constraint validator rejects braces — refusing the whole deploy.parseFlowSourceDirectivereads their top-level keys with the same source-over-version:precedence, and an unreadable mapping is unrepresentable rather than a constraint.sdk:-sourced overrides merged intosdkPackagesand were emitted underdependencies:— colliding with a same-named scalar dep as a duplicate YAML key. They now ride a separatesdkOverrideschannel and are emitted underdependency_overrides:. (This also surfaced that scalar overrides were sent asoverrideswhile the runner readdependencyOverrides— the field name now matches the wire contract.)extractPackageImportskept filtering SDK names (flutter,flutter_localizations, ...) even in verification — so a project forking an SDK package from git hid it from the unreproducible check. The extractor now returns SDK names to every caller; a name absent from the project's pubspec becomes an{sdk: flutter}addition rather than a hosted constraint, so an undeclaredflutter_web_pluginsimport still deploys.server.dartchanged, so the runner must go first. The current live runner still enforces the old allowlist, and this client will send it package names that allowlist rejects — deploying the web app alone would re-break exactly what we just fixed. The new runner is backward compatible with the old client's narrower list, so runner-first is safe.Verified
npm testnpm run test:deploy-guardnpm run test:proxiesnpm run builddart analyze(runner)New tests cover:
flutter_web_pluginsreproduced with no list (block and inline flow form); agit:dependency's transitive-constraint poison skipping every package-importing class whiledart:-only classes still verify; the reason naming a class's own direct import; a nestedversion:read as a constraint; a class importing an SDK package not being flagged as "missing"; the parser boundary case where a following sibling's key must not be read as the previous entry's source; anexportof an unreproducible package being skipped; aversion:-before-hosted:entry still classified hosted; an unreproducible override skipping all package-importing classes; a conditionalif (...)import of an unreproducible dependency;;-in-URI and escaped-quote and opposite-quote URIs surviving extraction; ansdk:override riding its own channel; a git-forked SDK name caught by verification; and inline{sdk:}/{git:}/{version:}/unreadable flow mappings classified by their real source.Link to Devin session: https://app.devin.ai/sessions/616e9d5ba84144cfb7168de3eed8da26
Open in Devin Desktop: https://app.devin.ai/desktop/session/616e9d5ba84144cfb7168de3eed8da26?variant=devin
Requested by: @sgardoll