feat(driver): add project driver v1 protocol and graph provenance - #165
feat(driver): add project driver v1 protocol and graph provenance#165joeykchen wants to merge 1 commit into
Conversation
9fb832b to
ffc0265
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #165 +/- ##
==========================================
+ Coverage 81.34% 87.43% +6.09%
==========================================
Files 10 21 +11
Lines 906 1640 +734
==========================================
+ Hits 737 1434 +697
- Misses 150 169 +19
- Partials 19 37 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ffc0265 to
f60c71c
Compare
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
Solid, defense-minded addition. The Encode/Parse round trip is carefully symmetric, SHA-256 identity validation is thorough (length + hex + lowercase-canonical + byte re-hash), and the module-cache split-source validation is rigorous about symlinks and cache layout. Findings below are correctness/maintainability polish — none are blocking.
Notable items (see inline comments):
- Dead
if p.infos == nilguard inimportClass. - Unreachable
filepath.IsAbs && !isAbsoluteCleanPathbranch invalidateResolvedModuleSyntax. - Undocumented positional-ordering contract on the exported
ResolvedClassGraph.ClassModules.
Additional (no reliable inline location):
-
README does not document the new
driverdirective.README.md:14describes the internaldriverprotocolpackage andImportClassesResolved, but the user-facing gox.mod authoring surface added here —driver <protocol> <package>, thev[1-9][0-9]*protocol format, and the "nearest preceding project / no block form / no duplicates" scoping rules (modfile/rule.go:406-442) — is undocumented. Authors can't learn how to write the directive this PR enables. Also, README:14 ("drivers verify identity-bearing paths before use") reads as the opposite emphasis fromprotocol.go:18("Validation is structural; consumers verify identity-bearing paths") — worth aligning the two. -
driverprotocolnever verifies the declaration digest against file bytes.Request.Validate()(request_validate.go:73-80) is filesystem-free by design and only checks thatdeclaration-sha256is well-formed. A driver thatParses argv and then trustsrequest.Declarationas a verified path+digest pair is relying on a digest that this package never checks against the file (byte-vs-digest verification lives only in the separatexgomodflow). Consider documenting onParse/Requestthat consumers MUST re-hash the declaration before trusting it. -
Silent last-wins on non-driver extension collisions. In
registerProject(xgomod/classfile_provenance.go:160-171), two distinct non-driver projects claiming the same extension fall through toprojects[ext] = ...; infos[ext] = ...with no diagnostic, while driver-backed collisions are rejected. This matches the legacyimportClassbehavior so may be intentional, but the asymmetry warrants a brief comment. -
Minor perf (low):
ImportClassesResolvedre-canonicalizes the target modfile atclassfile_provenance.go:57(Stat+EvalSymlinks+Abs) althoughgraph.validate()already canonicalized and read it viavalidateFileIdentity. Threading the canonicalized path/digest out ofvalidate()would remove the duplicate syscalls. Acceptable in one-shot validation code. -
Maintainability: option names are spelled independently in
Encode(argv.go),singularOptions, andcommonRequiredOptions(argv_options.go). A single source of truth (name constants) would prevent a three-list drift; currently only round-trip tests catch a mismatch.
| for _, w := range c.Works { | ||
| p.projs[w.Ext] = c | ||
| info := &ProjectInfo{Project: c} | ||
| if p.infos == nil { |
There was a problem hiding this comment.
Dead code: if p.infos == nil guard is unreachable. Every path into importClass originates from ImportClasses, which unconditionally sets p.infos = make(...) before any importClass/importClassFrom call. The guard also gives a false impression that importClass is safe with an uninitialized receiver, yet p.projs[ext] = c below would panic if p.projs were nil in that hypothetical. Suggest removing the guard and relying on ImportClasses initialization (consistent with how p.projs is handled).
| return fmt.Errorf("replacement: %w", err) | ||
| } | ||
| } | ||
| if filepath.IsAbs(m.Replace.Path) && !isAbsoluteCleanPath(m.Replace.Path) { |
There was a problem hiding this comment.
Unreachable condition. By the time execution reaches this check, the preceding if/else-if/else (the Version == "" local branch enforces isAbsoluteCleanPath, the versioned+absolute branch already returns, and the versioned+non-absolute branch leaves filepath.IsAbs false) has exhausted the cases, so filepath.IsAbs(m.Replace.Path) && !isAbsoluteCleanPath(...) is always false. Suggest removing it to avoid implying a case the earlier branches don't already cover.
|
|
||
| // ResolvedClassGraph is XGo's resolved graph snapshot; it is not rediscovered. | ||
| type ResolvedClassGraph struct { | ||
| Target ResolvedModule |
There was a problem hiding this comment.
Undocumented positional-ordering contract on ClassModules. ResolvedClassGraph.validate() requires ClassModules[i].Selected.Path == markerPaths[i], where markerPaths comes from the order of class-marked require directives in the target modfile. This makes the order of this exported slice a strict part of the API — a caller building the graph in a natural order (map iteration, sorted-by-path) will fail with "class module %d has logical path ... want marker ..." even though the set is correct. Suggest either documenting the required ordering on this field, or relaxing validate() to a set-membership check (the seenMarkers/seenModules maps are already built).
f60c71c to
00938f9
Compare
There was a problem hiding this comment.
FennoAI Review
Solid, well-tested change. The new driverprotocol package (argv codec + validation) and xgomod resolved-module provenance layer are idiomatic Go with descriptive, namespaced error messages and strong security-minded validation (canonical paths, NUL rejection, SHA-256 digest verification, module-cache layout checks). Test coverage is extensive.
No blocking issues found. Inline comments below cover minor correctness/maintainability cleanups. A few package-level notes that don't map to a single line:
- Duplicated option catalog (
driverprotocol/argv_options.go):singularOptionsandcommonRequiredOptionshand-enumerate large overlapping option-name lists that must stay in sync withEncode(argv.go) andParse. Consider centralizing the option catalog so adding a field doesn't require touching four sites. - Duplicated SHA-256 validation: near-identical length/hex/lowercase checks exist in both
driverprotocol/validation.go(validateSHA256) andxgomod/resolved_identity.go(validateFileIdentity), with different error styles. Sincedriverprotocolalready importsxgomod, a single shared validator would keep the rules identical. - Repeated canonicalization (perf, cold path): in
ImportClassesResolved, the same paths (targetgo.mod, effectiveDir) are run throughcanonicalPath(stat +EvalSymlinks+Abs) and read/hashed multiple times acrossvalidateFileIdentity,graph.validate(), andimportResolvedModule. Threading already-derived canonical paths / read bytes through would remove redundant stat/read work. Minor since imports are infrequent. - README: the new user-facing
driver <protocol> <package>gox.mod/gop.mod directive (added inmodfile/rule.go) isn't mentioned in the README; a one-line note would improve completeness.
| return fmt.Errorf("replacement: %w", err) | ||
| } | ||
| } | ||
| if filepath.IsAbs(m.Replace.Path) && !isAbsoluteCleanPath(m.Replace.Path) { |
There was a problem hiding this comment.
This check is unreachable. The preceding if/else-if/else fully partitions the cases: the Version == "" branch already enforces isAbsoluteCleanPath(m.Replace.Path), the filepath.IsAbs branch already returns an error, and the final else handles non-absolute versioned paths. Any absolute Replace.Path reaching this line has already been validated clean, so !isAbsoluteCleanPath(...) is always false. Consider removing it, or add a comment justifying it as defensive.
| for _, w := range c.Works { | ||
| p.projs[w.Ext] = c | ||
| info := &ProjectInfo{Project: c} | ||
| if p.infos == nil { |
There was a problem hiding this comment.
This lazy p.infos == nil guard is dead defensive code: the sole caller ImportClasses now initializes p.infos at line 99 before invoking importClass. Keeping both the eager init and this guard is redundant — consider dropping the guard.
| option("driver-package", request.DriverPackage), | ||
| option("selected-path", request.DriverOrigin.Selected.Path), | ||
| option("selected-version", request.DriverOrigin.Selected.Version), | ||
| option("origin-main", fmt.Sprint(request.DriverOrigin.Main)), |
There was a problem hiding this comment.
fmt.Sprint on a known bool triggers the reflection-based formatting path plus an allocation. strconv.FormatBool(request.DriverOrigin.Main) is more direct. Cosmetic.
| type Action string | ||
|
|
||
| const ( | ||
| ActionRun Action = "run" |
There was a problem hiding this comment.
ActionRun and ActionBuild are exported but lack godoc comments, unlike every other exported symbol in this package (Version1, Action, etc.). golint/revive will flag this — a one-line comment on the block or each constant would match the file's style.
| import "github.com/goplus/mod/xgomod" | ||
|
|
||
| const ( | ||
| // Version1 is the gox.mod driver protocol value. |
There was a problem hiding this comment.
"the gox.mod driver protocol value" is imprecise. This constant is the v1 protocol-version string shared by the driver directive (validated by driverProtocolRE = ^v[1-9][0-9]*$) and encoded requests; it is not literally read from gox.mod by this package. Consider rewording to "the v1 driver protocol version string."
| return *m.Replace == *other.Replace | ||
| } | ||
|
|
||
| // IsLocal reports whether the module uses filesystem source. |
There was a problem hiding this comment.
Two small nits: (1) the doc says "uses filesystem source," but a versioned replacement also resolves to a filesystem Dir/GoMod (module cache) yet returns false — the intended meaning is "local (unversioned) filesystem source"; (2) m.Main || m.Replace != nil && m.Replace.Version == "" is correct given Go precedence but reads more clearly as m.Main || (m.Replace != nil && m.Replace.Version == "").
00938f9 to
ad4bb1a
Compare
1 participant
Summary
Supersedes goplus/mod#164, which GitHub closed when its fork head branch was renamed.
This PR adds the
goplus/modlanguage and contract layer for the XGo Project Driver v1 design tracked by goplus/spx#1741. It defines driver metadata, resolved graph provenance, and a driver-neutral request protocol without adding XGo dispatch or SPX-specific runtime policy.What changed
driver <protocol> <driver-package>metadata scoped to the nearest preceding project, with strict protocol and import-path validation.modfile.Project.Driverwithout aruntimealias or any compatibility-specific parser branch.driverprotocolwith thexgo-driver-v1preamble, typedrun/buildrequests, deterministic argv encoding, andDriverPackage/DriverOriginprovenance.go.modandgox.mod/gop.modsnapshots consumed during resolution.Compatibility and boundaries
Projects without a driver retain their existing behavior. Built-in projects remain provenance-free, and the existing class import path remains available.
The driver protocol floor and a module's general
xgominimum are independent requirements; consumers enforce whichever minimum is higher. Filesystem checks that need live driver inputs remain the dispatcher's or driver's responsibility.Verification
GOWORK=off go test -race ./... GOWORK=off go vet ./... git diff --checkRelease order
This is the first repository in the coordinated release order. Mod must land and be released before XGo and SPX update their
github.com/goplus/modrequirements; the current publishedv0.21.2does not containdriverprotocol.Part of goplus/spx#1741.