test: add reflect upstream and ixgo type round trips - #13
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: reflect round-trip test suite
Solid, well-structured addition. The tests thoughtfully verify that host-created reflect objects survive the guest sandbox boundary with before/during/after-Run assertions, dynamic discovery of upstream reflect tests via go list + AST parsing, and a completed write-back guard proving the guest closure actually ran. Documentation/citations were verified accurate (upstream test names, license paths, even a faithfully reproduced upstream typo). No performance concerns: the reflect test binary is built once and reused, with correctly layered guest (2m) / host (3m) timeouts.
Two findings below; both are non-blocking. All command execution and temp-file handling in upstream_test.go is safe (fixed arg slices, 0600/0755 perms, regexp.QuoteMeta applied).
| field := types[10].Field(0) | ||
| if field.Name != "Count" || field.Tag.Get("json") != "count" || field.Type != types[0] { | ||
| panic("StructOf field metadata changed") | ||
| } | ||
| if !types[12].IsVariadic() || types[12].In(1) != types[5] || types[12].Out(0) != types[0] { | ||
| panic("FuncOf signature changed") |
There was a problem hiding this comment.
[P2] Brittle hardcoded indices couple assertions to constructor order
types[10] (StructOf) and types[12] (variadic FuncOf), plus types[0]/types[5], reach into the slice from publicReflectTypes() using magic indices that must stay manually in sync with the constructor's element order. They are correct today, but inserting or reordering a type (e.g. a new ChanOf variant) would silently retarget these checks at the wrong element without any compile-time or test-time signal. Consider binding the interesting types to named locals and asserting against the names, or at minimum add a comment mapping each index to its constructor line.
| {Type: "bind", Source: "/", Target: "/", Options: []string{"ro"}}, | ||
| {Type: "tmpfs", Target: "/tmp"}, | ||
| {Type: "proc", Target: "/proc"}, |
There was a problem hiding this comment.
[P2] Full-root / and /proc mounts widen the guest trust boundary
The sandbox bind-mounts all of host / (read-only) and mounts a full proc. Even read-only, this lets sandboxed test code read any file the process uid can (~/.ssh, cloud creds, CI tokens) and, via /proc/*/environ and /proc/*/cmdline, secrets from sibling processes. For a harness that only needs the Go toolchain and the compiled test binary, this is broader than necessary. If this only ever runs in an ephemeral, secret-free CI container it is acceptable, but consider narrowing the bind to runtime.GOROOT() (plus required dirs) and dropping/masking /proc if the reflect tests don't need it.
6a17ced to
4d7c18f
Compare
4d7c18f to
82b08c3
Compare
1 participant
No description provided.