Skip to content

Supply WENDY_JETPACK_MAJOR as docker build arg - #1144

Merged
Joannis merged 1 commit into
mainfrom
thombles/jetpack-major
Jun 25, 2026
Merged

Joannis merged 1 commit into
mainfrom
thombles/jetpack-major

Conversation

@thombles

Copy link
Copy Markdown
Contributor

For some apps it's useful to select a base image based on whether we're dealing with Jetpack 6 vs Jetpack 7. While we already have WENDY_JETPACK_VERSION, this is a value like 7.2 which is not easily used for conditional selection within a Dockerfile. If we provide WENDY_JETPACK_MAJOR (7) then this is straightforward.

Example usage in paired PR: wendylabsinc/samples#14

@thombles thombles self-assigned this Jun 23, 2026
@thombles
thombles requested a review from Joannis as a code owner June 23, 2026 23:58
Copilot AI review requested due to automatic review settings June 23, 2026 23:58
@thombles
thombles requested a review from EBro912 as a code owner June 23, 2026 23:58

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Integration test review — automated suggestions from Claude. Apply, adapt, or dismiss as needed.

NO_CHANGES_NEEDED

The PR introduces WENDY_JETPACK_MAJOR as a new docker build arg derived from WENDY_JETPACK_VERSION. This is a build-time argument injection feature specific to Jetson hardware — it is already covered by a new Go unit test (TestApplyDeviceBuildArgHints_DerivesJetpackMajor) in device_build_args_test.go that exercises both the happy path ("7.2" → "7") and the L4T fallback omission case. An integration test for this feature would require a physical Jetson device with a specific JetPack version, and the feature itself has no observable runtime behavior (it's a build arg consumed at image-build time, not at container runtime). There is no new entitlement, deployment mode, or CLI command introduced. No integration test additions are warranted.

@github-actions

Copy link
Copy Markdown
Contributor

AI Security Review


Security & Compliance Review — PR #1144: Supply WENDY_JETPACK_MAJOR as docker build arg


1. Executive Summary

This pull request introduces a minor, well-scoped feature: deriving a coarse WENDY_JETPACK_MAJOR integer string (e.g., "7") from the existing WENDY_JETPACK_VERSION field and injecting it as a Docker build argument, alongside documentation and comment updates. The implementation is conservative — it reuses the existing setHint / validateBuildArgPair guard path, applies a narrow numeric-only validation in jetpackMajor(), and omits the value rather than erroring when the version string is not a clean dotted numeric. No new network calls, secrets, PII, credentials, or third-party dependencies are introduced. The diff presents a very low security risk profile overall, with only minor informational observations worth noting for completeness.


2. Findings Table

Severity Standards File Line(s) Title
LOW [SOC2-CC7] [ISO27001-A.12] go/internal/cli/commands/docker.go ~309–315 jetpackMajor silently drops malformed version strings without logging
LOW [ISO27001-A.8] [NIST-SI] go/internal/cli/commands/docker.go ~307 Device-reported version string flows into build args without an explicit length cap
INFORMATIONAL [SOC2-CC8] go/internal/cli/commands/device_build_args_test.go ~62–73 Test coverage gap: no test for boundary/edge inputs to jetpackMajor directly
INFORMATIONAL [ISO27001-A.8] go/internal/cli/commands/docker.go ~309 strings.Cut on an empty string yields ("", "", false) — benign but implicit

3. Detailed Findings


Finding 1 — jetpackMajor silently drops malformed version strings without logging

Severity: LOW
Standards: [SOC2-CC7] [ISO27001-A.12]

Description:
When jetpackMajor receives a version string that does not parse as a clean <major>.<rest> numeric form (e.g., the documented "L4T 39.2.0" fallback, or any future unexpected format), it silently returns "". The caller setHint in turn silently skips the key when the value is empty. The existing setHint path does emit a warning when validateBuildArgPair rejects a value, but jetpackMajor short-circuits before that validation path is ever reached for bad inputs. This means operator visibility of unusual agent-reported version strings is reduced — a device reporting a novel or corrupted version string will simply omit the build arg with no diagnostic output.

Relevant snippet:

func jetpackMajor(version string) string {
    major, _, _ := strings.Cut(version, ".")
    if _, err := strconv.Atoi(major); err != nil {
        return "" // empty, or an unmapped "L4T 39.2.0" fallback — not a clean major
    }
    return major
}

Remediation:
Add a log.Debugf or equivalent trace-level log statement in the err != nil branch when version is non-empty (i.e., the agent did report something, but it wasn't parseable). This preserves the silent-omit behaviour for build correctness while giving operators visibility. Example:

func jetpackMajor(version string) string {
    if version == "" {
        return ""
    }
    major, _, _ := strings.Cut(version, ".")
    if _, err := strconv.Atoi(major); err != nil {
        log.Debugf("jetpackMajor: skipping non-numeric JetPack version %q", version)
        return ""
    }
    return major
}

Controls violated: SOC2 CC7.2 (anomaly detection / operational visibility), ISO 27001 A.12.4 (event logging).


Finding 2 — Device-reported version string flows into build args without an explicit length cap

Severity: LOW
Standards: [ISO27001-A.8] [NIST-SI]

Description:
versionResp.GetJetpackVersion() is a device-reported string. It is passed first to jetpackMajor, which calls strings.Cut and strconv.Atoi on a potentially unbounded input, and the result is subsequently passed to setHint → validateBuildArgPair for regex validation. While strconv.Atoi and strings.Cut are not vulnerable to catastrophic behaviour on large inputs in Go, the existing validBuildArgValueRe regex (not shown in this diff but presumably anchored) is the primary guard. There is no explicit upper-bound check on the raw version string length before processing begins.

For a defence-in-depth posture — particularly given the comment that these values are "device-reported and feed straight into a builder CLI" — an explicit length guard would eliminate any theoretical DoS or regex-amplification risk if the regex is ever modified.

Relevant snippet:

setHint("WENDY_JETPACK_MAJOR", jetpackMajor(versionResp.GetJetpackVersion()))

Remediation:
Add a length cap early in jetpackMajor (or centrally in setHint) before any string processing:

func jetpackMajor(version string) string {
    if len(version) > 64 { // generous cap; real versions are < 10 chars
        return ""
    }
    // ... existing logic
}

Alternatively, enforce a maximum length inside setHint or validateBuildArgPair that applies uniformly to all hints.

Controls violated: ISO 27001 A.8.28 (secure coding — input validation), NIST SP 800-53 SI-10 (information input validation).


Finding 3 — Test coverage gap: no direct unit tests for jetpackMajor edge cases

Severity: INFORMATIONAL
Standards: [SOC2-CC8]

Description:
The new test TestApplyDeviceBuildArgHints_DerivesJetpackMajor exercises two cases via the higher-level applyDeviceBuildArgHints wrapper: a clean "7.2" input and the "L4T 39.2.0" fallback. However, jetpackMajor itself is not tested directly for edge cases such as: empty string "", a bare integer with no dot "7", a multi-segment version "7.2.1", a version with leading zeroes "07.2", or an excessively long/malformed string. Lack of direct tests for the helper makes future refactors riskier.

Relevant snippet:

func TestApplyDeviceBuildArgHints_DerivesJetpackMajor(t *testing.T) {
    clean := map[string]string{}
    applyDeviceBuildArgHints(clean, &agentpb.GetAgentVersionResponse{JetpackVersion: strptr("7.2")})
    // ...
    fallback := map[string]string{}
    applyDeviceBuildArgHints(fallback, &agentpb.GetAgentVersionResponse{JetpackVersion: strptr("L4T 39.2.0")})
    // ...
}

Remediation:
Export or add a package-level test for jetpackMajor directly (or via a table-driven test within the _test.go file using //go:build unexported access), covering: empty string, no-dot input, multi-segment input, leading-zero input, and an oversized string if a cap is added.

Controls violated: SOC2 CC8.1 (change management — adequate testing prior to deployment).


Finding 4 — strings.Cut on empty string is implicitly safe but undocumented

Severity: INFORMATIONAL
Standards: [ISO27001-A.8]

Description:
When versionResp.GetJetpackVersion() returns "" (agent did not report a JetPack version), jetpackMajor("") calls strings.Cut("", "."), which returns ("", "", false). Then strconv.Atoi("") returns an error, so "" is returned — which setHint correctly skips. This is the desired behaviour, but it is reached via the error-path of Atoi rather than an explicit empty-string guard. A future reader might find this non-obvious.

Relevant snippet:

func jetpackMajor(version string) string {
    major, _, _ := strings.Cut(version, ".")
    if _, err := strconv.Atoi(major); err != nil {
        return ""
    }
    return major
}

Remediation:
Add an explicit early-return for the empty-string case (as shown in Finding 1's remediation) for clarity and to make the intent self-documenting. No functional change required.

Controls violated: ISO 27001 A.8.28 (secure coding practices — code clarity/maintainability).


4. Compliance Summary

Framework Checked Violations / Risks Found
SOC 2 (TSC) ✅ LOW: CC7 (reduced operational visibility on malformed inputs); INFORMATIONAL: CC8 (test coverage)
ISO/IEC 27001:2022 ✅ LOW: A.8 (input validation, length cap); INFORMATIONAL: A.8 (code clarity)
PCI DSS v4.0 N/A Diff does not touch payment or card data flows — not assessed
GDPR / Privacy ✅ No personal data involved — no violations found
HIPAA N/A Diff does not touch health or medical data — not assessed
NIST SP 800-53 / CSF 2.0 ✅ LOW: SI-10 (input validation on device-reported string)

Overall assessment: This is a low-risk, narrowly scoped change. No critical or high severity issues were identified. The two LOW findings (missing debug log on parse failure, missing length cap on device-reported input) are defence-in-depth recommendations rather than exploitable vulnerabilities in typical deployment contexts. Addressing Finding 1 (adding a debug-level log) is the most actionable improvement before merge.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a coarse JetPack selector build arg (WENDY_JETPACK_MAJOR) to make it easier for Dockerfiles/Containerfiles to choose base images by JetPack generation (e.g., 6 vs 7) while preserving the existing fine-grained version args.

Changes:

  • Inject WENDY_JETPACK_MAJOR as a validated build-arg hint derived from WENDY_JETPACK_VERSION.
  • Add unit coverage ensuring the major is derived for clean versions and omitted for unmapped L4T ... fallbacks.
  • Document the new build arg for both wendy run and wendy build.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
go/internal/cli/commands/run.go Updates Jetson platform-selection guidance to mention WENDY_JETPACK_MAJOR.
go/internal/cli/commands/docker.go Derives and injects WENDY_JETPACK_MAJOR during build-arg hint application.
go/internal/cli/commands/device_build_args_test.go Adds tests for WENDY_JETPACK_MAJOR derivation/omission behavior.
go/internal/cli/assets/docs/clients/wendy-cli/commands/run.md Documents WENDY_JETPACK_MAJOR in wendy run build-arg table.
go/internal/cli/assets/docs/clients/wendy-cli/commands/build.md Documents WENDY_JETPACK_MAJOR in wendy build build-arg table.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 281 to +285
// applyDeviceBuildArgHints injects the optional device/GPU build-arg hints the
// agent reports (WENDY_DEVICE_TYPE, WENDY_HAS_GPU, WENDY_GPU_VENDOR,
// WENDY_JETPACK_VERSION, WENDY_CUDA_VERSION) into buildArgs. Each hint is only
// set when the agent reports it, so Dockerfiles keep their own ARG defaults on
// older agents. These values are device-reported and feed straight into a
// builder CLI, so any hint that fails build-arg validation is skipped with a
// warning rather than failing the whole deploy — e.g. a Jetson running an L4T
// release the agent's JetPack table doesn't map reports a fallback like
// "L4T 38.2.0", whose space is rejected by validBuildArgValueRe.
// WENDY_JETPACK_VERSION, WENDY_JETPACK_MAJOR, WENDY_CUDA_VERSION) into
// buildArgs. Each hint is only set when the agent reports it, so Dockerfiles
// keep their own ARG defaults on older agents. These values are device-reported
@github-actions

Copy link
Copy Markdown
Contributor

Docs preview

Preview this PR's docs at: https://docs.wendy.dev/branch-thombles-jetpack-major-da086b6fc508d573158446e1c0ed23e0a6309a83/

This comment is updated automatically when the docs preview is redeployed.

@Joannis
Joannis merged commit 94220ef into main Jun 25, 2026
14 checks passed
@Joannis
Joannis deleted the thombles/jetpack-major branch June 25, 2026 12:13

This branch was successfully deployed

1 active deployment
docs — da086b6f Deployed Jun 23, 2026 by thombles via Deploy docs #517
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants