Repository navigation
feat(build_go): port the build tool to Go for standalone static binaries - #1166
Conversation
…efile Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…!env, logging Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pers Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…aming Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… build command CONFIG/MODULES/FLAGS modes with Python-parity validation, multi-arch contexts, fail-fast module pipeline with Slack notifications, and the full 21-module registry (placeholders swapped as ports land). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…gure) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eplace/string-replace) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tches modules Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…llelism Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…image+deb) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… build orchestration Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- platform: probe native machine arch (Rosetta/emulation safe) - release: normalize GitHub tag/title to MAJOR.MINOR.BUILD like Python - buildctx: ReleasePath always uses semantic version (parity) - string_replaces: continue past per-file failures, aggregate error - upload: build_date matches Python isoformat byte format - fsx.RemoveAll: clear read-only bits and retry (Windows clean parity) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…, NormalizeVersion test) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Greptile (@greptileai) review |
|
Claude (@claude) review |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished Nikhil (@shadowfax92)'s task in 4m 44s —— View job PR Review: feat(build_go): port the build tool to Go for standalone static binariesTasks
This is a well-structured port. The design decisions are sound — Bugs1. Silent failure in Sparkle DMG signing — When for _, dmgPath := range dmgFiles {
sig, length, err := sparkle.SignFileWithEnv(dmgPath)
if err != nil {
logx.Error(fmt.Sprintf("Error signing %s: %v", name, err))
continue // ← pipeline continues with missing signatures
}
...
}
logx.Success(...)
return nil // ← always returns nilThe fix should either fail-fast or collect errors and return after the loop. At a minimum, 2. resp, err := c.do(http.MethodPut, key, nil, headers, strings.NewReader(string(data)), int64(len(data)), sha256Hex(data))
3. The func (c *Context) ReleasePath(plat string) string {
return fmt.Sprintf("releases/%s/%s/", c.SemanticVersion, plat)
}If any caller sets Security4. Default CDN base URL uses HTTP — func R2CDNBaseURL() string {
if v := os.Getenv("R2_CDN_BASE_URL"); v != "" {
return v
}
return "http://cdn.browseros.com" // ← plain HTTP
}Artifact URLs embedded in Repository Hygiene5. Compiled binary committed to the repo — The PR includes a built binary ( git rm --cached packages/browseros/build_go/browserosThen commit the removal. The Minor / Robustness6. filepath.WalkDir(patchesDir, func(...) error { ... })
// return value ignoredIf 7. Memory spike for large DMGs in data, err := os.ReadFile(path) // loads the full DMG into RAMDMG files are often 250–400 MB. Ed25519 in the Go stdlib doesn't stream, so this is unavoidable with 8. Go 1.25 is very recent (August 2025). A comment in the Positive observations
|
Greptile SummaryThis PR ports the full Python build-tool CLI (
Confidence Score: 3/5Not ready to merge as-is: the committed binary, the HTTP default for artifact URLs, and the shell-injection risk in Windows signing all need to be resolved first. Three distinct defects on the changed paths need fixing before this lands. The pre-built binary is explicitly excluded by the project's own .gitignore. The http:// default for R2CDNBaseURL causes every auto-update payload URL to be unencrypted. The Windows eSigner integration embeds the password via Go's %q (not shell-safe) into a shell command string. The rest of the port is well-structured with thorough test coverage. packages/browseros/build_go/browseros (binary should be removed), packages/browseros/build_go/internal/envx/envx.go (http default), packages/browseros/build_go/internal/modules/sign/windows.go (shell quoting), packages/browseros/build_go/internal/modules/sign/sparklesign.go (silent signing failures)
|
| Filename | Overview |
|---|---|
| packages/browseros/build_go/browseros | Pre-compiled 15 MB binary committed despite being explicitly excluded by .gitignore — should be removed from source control. |
| packages/browseros/build_go/internal/envx/envx.go | Centralizes all env-var access; default R2CDNBaseURL uses http:// instead of https://, causing update artifact URLs to be unencrypted. |
| packages/browseros/build_go/internal/modules/sign/windows.go | Windows eSigner integration passes password via fmt.Sprintf("%q") which is not safe shell-quoting and will break on passwords containing shell meta-characters. |
| packages/browseros/build_go/internal/modules/sign/sparklesign.go | Signing errors are logged but swallowed; Execute always returns nil, allowing unsigned DMGs to proceed to upload. |
| packages/browseros/build_go/internal/r2/r2.go | Minimal R2/S3-compatible client with SigV4; PutBytes uses an unnecessary string round-trip instead of bytes.NewReader. |
| packages/browseros/build_go/internal/r2/sigv4.go | Standalone AWS SigV4 implementation; clean, well-structured, and described as verified against AWS published test vectors. |
| packages/browseros/build_go/internal/sparkle/sparkle.go | Ed25519 Sparkle signing using stdlib crypto/ed25519; handles 32-byte seed and 64-byte (seed+pub) key formats correctly. |
| packages/browseros/build_go/internal/otasrv/otasrv.go | OTA server release flow — download, sign, zip, sparkle-sign, upload, appcast generation; overall logic is sound with temp-dir cleanup. |
| packages/browseros/build_go/internal/resolver/resolver.go | Config/Direct resolution mode port with clean precedence logic (CLI > YAML/Env > default); matches Python semantics. |
| packages/browseros/build_go/internal/buildctx/buildctx.go | Central build context with all path derivations and version parsing; faithful port of Python Context with clean options pattern. |
| packages/browseros/build_go/internal/release/release.go | Release list/appcast/publish/download/GitHub operations; version comparison and metadata merge look correct. |
| packages/browseros/build_go/internal/modules/storage/upload.go | Upload module detects artifacts by platform glob, builds release.json with Sparkle metadata, and merges Linux x64/arm64 entries correctly. |
| packages/browseros/build_go/internal/pipeline/pipeline.go | Pipeline executor with fail-fast validation, per-module timing, and selective Slack notifications; clean and straightforward. |
| packages/browseros/build_go/go.mod | Go module with minimal deps (cobra, lipgloss, yaml.v3, x/sys); declares go 1.25.0 which is not yet released and may cause toolchain compatibility issues. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
CLI["browseros build / dev / release / ota / upload"]
Resolver["resolver.go\nCONFIG mode (YAML) or DIRECT mode (flags/env)"]
Context["buildctx.Context\nversions paths platform runner"]
Pipeline["pipeline.Execute\nfail-fast Slack notify"]
Modules["Module Registry 21 modules\nclean git_setup sparkle_setup configure\npatches chromium_replace string_replaces resources\nbundled_extensions download_resources\ncompile universal_build\nsign_macos sign_windows sign_linux sparkle_sign\npackage_macos package_windows package_linux\nupload"]
R2["r2.Client\nhand-rolled SigV4\nPut Get List Copy Delete"]
Sparkle["sparkle.SignFileWithEnv\ncrypto/ed25519"]
Notify["notify.go\nSlack webhooks async"]
CLI --> Resolver
Resolver --> Context
Context --> Pipeline
Pipeline --> Modules
Modules --> R2
Modules --> Sparkle
Pipeline --> Notify
Comments Outside Diff (1)
-
packages/browseros/build_go/browseros, line 1 (link)Compiled binary committed to source control
A 15 MB pre-built
browserosbinary is committed here, yetbuild_go/.gitignoreexplicitly lists/browserosto prevent exactly this. Binaries checked into git are opaque to code review, bloat history permanently, and create a supply-chain risk if the build environment producing the binary is ever compromised. The repo already ships aMakefilewithmake build— consumers should build from source rather than use a pre-compiled blob.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/browseros/build_go/browseros Line: 1 Comment: **Compiled binary committed to source control** A 15 MB pre-built `browseros` binary is committed here, yet `build_go/.gitignore` explicitly lists `/browseros` to prevent exactly this. Binaries checked into git are opaque to code review, bloat history permanently, and create a supply-chain risk if the build environment producing the binary is ever compromised. The repo already ships a `Makefile` with `make build` — consumers should build from source rather than use a pre-compiled blob. How can I resolve this? If you propose a fix, please make it concise.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Fix the following 5 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 5
packages/browseros/build_go/browseros:1
**Compiled binary committed to source control**
A 15 MB pre-built `browseros` binary is committed here, yet `build_go/.gitignore` explicitly lists `/browseros` to prevent exactly this. Binaries checked into git are opaque to code review, bloat history permanently, and create a supply-chain risk if the build environment producing the binary is ever compromised. The repo already ships a `Makefile` with `make build` — consumers should build from source rather than use a pre-compiled blob.
### Issue 2 of 5
packages/browseros/build_go/internal/modules/sign/windows.go:116-120
**Go `%q` verb is not shell-safe quoting**
`fmt.Sprintf("%q", envx.ESignerPassword())` wraps the password in Go-style double-quotes, which is not equivalent to shell quoting. A password containing `$`, backtick, `!`, or `\` will be shell-expanded when the resulting string is passed to `sh -c` / `cmd /c`. For example, a password like `P@$$123` produces `"P@$$123"` — the shell then replaces `$$` with the current PID and authentication fails silently. Prefer passing credentials via separate exec arguments (no shell intermediary) or use a proper shell-escaping helper.
### Issue 3 of 5
packages/browseros/build_go/internal/modules/sign/sparklesign.go:46-55
**Signing errors are silently dropped; function returns nil on partial failure**
When `sparkle.SignFileWithEnv` fails for a DMG, the error is logged but the loop continues and `Execute` returns `nil` at the end regardless of how many files failed. This means a build with unsigned DMGs can proceed to the upload phase, publishing artifacts that Sparkle auto-update clients will reject at verification time. The Python original (`sign/sparkle.py`) raises on any signing failure — the port should mirror that behavior.
### Issue 4 of 5
packages/browseros/build_go/internal/envx/envx.go:117-123
The default CDN base URL uses `http://` (plain-text) rather than `https://`. Every artifact URL written into `release.json` and appcast XML files inherits this, meaning auto-update clients on end-user machines will fetch updates over an unencrypted connection unless `R2_CDN_BASE_URL` is explicitly overridden.
```suggestion
// R2CDNBaseURL defaults to the public CDN.
func R2CDNBaseURL() string {
if v := os.Getenv("R2_CDN_BASE_URL"); v != "" {
return v
}
return "https://cdn.browseros.com"
}
```
### Issue 5 of 5
packages/browseros/build_go/internal/r2/r2.go:161
`strings.NewReader(string(data))` copies the byte slice to a string and then wraps it. For large in-memory objects (appcast XML, release JSON) this is an unnecessary allocation. `bytes.NewReader` avoids both the copy and the `strings` import.
```suggestion
resp, err := c.do(http.MethodPut, key, nil, headers, bytes.NewReader(data), int64(len(data)), sha256Hex(data))
```
Reviews (1): Last reviewed commit: "chore(build_go): stop tracking the compi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| if _, err := os.Stat(toolPath); err != nil { | ||
| return fmt.Errorf("CodeSignTool not found at: %s", toolPath) | ||
| } | ||
| if envx.ESignerUsername() == "" || envx.ESignerPassword() == "" || envx.ESignerTOTPSecret() == "" { |
There was a problem hiding this comment.
%q verb is not shell-safe quoting
fmt.Sprintf("%q", envx.ESignerPassword()) wraps the password in Go-style double-quotes, which is not equivalent to shell quoting. A password containing $, backtick, !, or \ will be shell-expanded when the resulting string is passed to sh -c / cmd /c. For example, a password like P@$$123 produces "P@$$123" — the shell then replaces $$ with the current PID and authentication fails silently. Prefer passing credentials via separate exec arguments (no shell intermediary) or use a proper shell-escaping helper.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/build_go/internal/modules/sign/windows.go
Line: 116-120
Comment:
**Go `%q` verb is not shell-safe quoting**
`fmt.Sprintf("%q", envx.ESignerPassword())` wraps the password in Go-style double-quotes, which is not equivalent to shell quoting. A password containing `$`, backtick, `!`, or `\` will be shell-expanded when the resulting string is passed to `sh -c` / `cmd /c`. For example, a password like `P@$$123` produces `"P@$$123"` — the shell then replaces `$$` with the current PID and authentication fails silently. Prefer passing credentials via separate exec arguments (no shell intermediary) or use a proper shell-escaping helper.
How can I resolve this? If you propose a fix, please make it concise.| signed := 0 | ||
| for _, dmgPath := range dmgFiles { | ||
| name := filepath.Base(dmgPath) | ||
| logx.Info(fmt.Sprintf("🔐 Signing %s...", name)) | ||
| sig, length, err := sparkle.SignFileWithEnv(dmgPath) | ||
| if err != nil { | ||
| logx.Error(fmt.Sprintf("Error signing %s: %v", name, err)) | ||
| continue | ||
| } | ||
| ctx.SparkleSignatures[name] = buildctx.SparkleSig{Signature: sig, Length: length} |
There was a problem hiding this comment.
When sparkle.SignFileWithEnv fails for a DMG, the error is logged but the loop continues and Execute returns nil at the end regardless of how many files failed. This means a build with unsigned DMGs can proceed to the upload phase, publishing artifacts that Sparkle auto-update clients will reject at verification time. The Python original (sign/sparkle.py) raises on any signing failure — the port should mirror that behavior.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/build_go/internal/modules/sign/sparklesign.go
Line: 46-55
Comment:
**Signing errors are silently dropped; function returns nil on partial failure**
When `sparkle.SignFileWithEnv` fails for a DMG, the error is logged but the loop continues and `Execute` returns `nil` at the end regardless of how many files failed. This means a build with unsigned DMGs can proceed to the upload phase, publishing artifacts that Sparkle auto-update clients will reject at verification time. The Python original (`sign/sparkle.py`) raises on any signing failure — the port should mirror that behavior.
How can I resolve this? If you propose a fix, please make it concise.| // R2CDNBaseURL defaults to the public CDN. | ||
| func R2CDNBaseURL() string { | ||
| if v := os.Getenv("R2_CDN_BASE_URL"); v != "" { | ||
| return v | ||
| } | ||
| return "http://cdn.browseros.com" | ||
| } |
There was a problem hiding this comment.
http:// (plain-text) rather than https://. Every artifact URL written into release.json and appcast XML files inherits this, meaning auto-update clients on end-user machines will fetch updates over an unencrypted connection unless R2_CDN_BASE_URL is explicitly overridden.
| // R2CDNBaseURL defaults to the public CDN. | |
| func R2CDNBaseURL() string { | |
| if v := os.Getenv("R2_CDN_BASE_URL"); v != "" { | |
| return v | |
| } | |
| return "http://cdn.browseros.com" | |
| } | |
| // R2CDNBaseURL defaults to the public CDN. | |
| func R2CDNBaseURL() string { | |
| if v := os.Getenv("R2_CDN_BASE_URL"); v != "" { | |
| return v | |
| } | |
| return "https://cdn.browseros.com" | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/build_go/internal/envx/envx.go
Line: 117-123
Comment:
The default CDN base URL uses `http://` (plain-text) rather than `https://`. Every artifact URL written into `release.json` and appcast XML files inherits this, meaning auto-update clients on end-user machines will fetch updates over an unencrypted connection unless `R2_CDN_BASE_URL` is explicitly overridden.
```suggestion
// R2CDNBaseURL defaults to the public CDN.
func R2CDNBaseURL() string {
if v := os.Getenv("R2_CDN_BASE_URL"); v != "" {
return v
}
return "https://cdn.browseros.com"
}
```
How can I resolve this? If you propose a fix, please make it concise.
Greptile SummaryThis PR ports the Python build tool (
Confidence Score: 3/5Do not merge until the committed binary is removed, the HTTP CDN default is fixed to HTTPS, and the Sparkle signing error-swallowing is resolved — these three issues together mean the tool could silently ship unsigned updates over plain HTTP. Three distinct defects need fixing before this tool is production-safe: a pre-built binary in git history, all generated artifact URLs defaulting to HTTP, and the Sparkle signing loop silently continuing on failure so unsigned DMGs can reach upload. There is also a likely compile error in otasrv.go due to a variable name shadowing the platform() helper. The bulk of the implementation — SigV4, patchwork engine, pipeline executor, context derivation — is solid and well-tested. packages/browseros/build_go/browseros (committed binary), packages/browseros/build_go/internal/envx/envx.go (HTTP default), packages/browseros/build_go/internal/modules/sign/sparklesign.go (silent signing failures), packages/browseros/build_go/internal/otasrv/otasrv.go (platform variable shadow)
|
| Filename | Overview |
|---|---|
| packages/browseros/build_go/browseros | Pre-built darwin/arm64 binary (~15 MB) committed to the repository; .gitignore entry has no effect on already-tracked files and this binary will persist in history. |
| packages/browseros/build_go/internal/modules/sign/sparklesign.go | SparkleSign.Execute silently swallows per-DMG signing errors and returns nil even if all DMGs failed to sign, allowing unsigned DMGs to reach upload. |
| packages/browseros/build_go/internal/envx/envx.go | R2CDNBaseURL defaults to HTTP instead of HTTPS; all generated artifact download URLs in release.json and appcasts will use plain HTTP when the env var is unset. |
| packages/browseros/build_go/internal/otasrv/otasrv.go | Server OTA flow looks solid overall; package-level platform() helper is shadowed by a for-range loop variable in GenerateServerAppcast — verify this compiles. |
| packages/browseros/build_go/internal/r2/sigv4.go | Clean hand-rolled SigV4 implementation; canonical request, string-to-sign, and signing key derivation all look correct and match AWS spec structure. |
| packages/browseros/build_go/internal/r2/r2.go | Minimal R2/S3 client with PutFile (UNSIGNED-PAYLOAD), PutBytes, GetFile, GetObject, Exists, Delete, Copy, List, and ListPrefixes; uses 30-minute HTTP timeout for large uploads. |
| packages/browseros/build_go/internal/sparkle/sparkle.go | Ed25519 Sparkle signing using stdlib crypto/ed25519; handles 32-byte seed, 64-byte seed+pubkey, and base64 of either — looks correct. |
| packages/browseros/build_go/internal/modules/sign/macos.go | Faithful port of macOS codesign+notarize flow; signing order (XPC → nested apps → executables → dylibs → helpers → frameworks → main → bundle) matches the Python original. |
| packages/browseros/build_go/internal/pipeline/pipeline.go | Clean fail-fast pipeline executor with Slack notification hooks and per-module timing; faithful port of the Python pipeline. |
| packages/browseros/build_go/internal/buildctx/buildctx.go | Context object with correct BrowserOSChromiumVersion derivation (BUILD+offset math), all path getters, and artifact registry — faithful parity with the Python original. |
| packages/browseros/build_go/internal/execx/execx.go | mergedEnv appends extra vars to os.Environ() without deduplication; duplicate keys work in practice on most OSes (last wins) but is non-standard. |
| packages/browseros/build_go/internal/modules/storage/upload.go | UploadReleaseArtifacts detects platform artifacts, uploads to R2, and generates/merges release.json; Linux x64/arm64 artifact merging handled correctly. |
| packages/browseros/build_go/internal/vendorup/vendorup.go | Third-party vendor upload (Lima, Bun, Codex, Claude Code) with SHA256 verification; extraction targets caller-supplied dest paths so path traversal risk is low. |
| packages/browseros/build_go/internal/patchwork/apply.go | Patch application engine (standard → 3-way → reject fallback) with interactive prompting and force mode; clean port of the Python apply modules. |
Sequence Diagram
sequenceDiagram
participant CLI as browseros CLI (cobra)
participant Resolver as resolver
participant Pipeline as pipeline.Execute
participant Module as Module (validate/execute)
participant R2 as r2.Client (SigV4)
participant Sparkle as sparkle (Ed25519)
participant Notify as notify (Slack)
CLI->>Resolver: ResolveContexts(CLIArgs, yamlCfg)
Resolver-->>CLI: "[]*buildctx.Context (one per arch)"
loop each Context
CLI->>Pipeline: Execute(ctx, modules, registry)
Pipeline->>Notify: PipelineStart
loop each module
Pipeline->>Module: Validate(ctx)
Module-->>Pipeline: "error | nil"
Pipeline->>Module: Execute(ctx)
note over Module: compile / sign / package / upload
Module->>R2: PutFile / GetFile / PutBytes
R2-->>Module: HTTP response (SigV4 signed)
Module->>Sparkle: SignFileWithEnv(path)
Sparkle-->>Module: (base64sig, length)
Pipeline->>Notify: ModuleCompletion
end
Pipeline->>Notify: PipelineEnd / PipelineError
Pipeline->>Notify: Flush(2s)
end
Comments Outside Diff (1)
-
packages/browseros/build_go/browseros, line 1 (link)The
browserosdarwin/arm64 binary (~15 MB) is tracked in git. The.gitignoreentry/browserosonly prevents future adds — once a file is tracked,.gitignorehas no effect on it, so this binary will persist in every clone and in git history. Committing built binaries raises supply-chain concerns (reviewers cannot verify the binary was produced from the committed source), inflates repo size, and adds noise to everygit statuson macOS. Recommend removing it withgit rm --cached packages/browseros/build_go/browserosand ensuring CI builds from source instead.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/browseros/build_go/browseros Line: 1 Comment: **Pre-built binary committed to the repo** The `browseros` darwin/arm64 binary (~15 MB) is tracked in git. The `.gitignore` entry `/browseros` only prevents future adds — once a file is tracked, `.gitignore` has no effect on it, so this binary will persist in every clone and in git history. Committing built binaries raises supply-chain concerns (reviewers cannot verify the binary was produced from the committed source), inflates repo size, and adds noise to every `git status` on macOS. Recommend removing it with `git rm --cached packages/browseros/build_go/browseros` and ensuring CI builds from source instead. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 5 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 5
packages/browseros/build_go/browseros:1
**Pre-built binary committed to the repo**
The `browseros` darwin/arm64 binary (~15 MB) is tracked in git. The `.gitignore` entry `/browseros` only prevents future adds — once a file is tracked, `.gitignore` has no effect on it, so this binary will persist in every clone and in git history. Committing built binaries raises supply-chain concerns (reviewers cannot verify the binary was produced from the committed source), inflates repo size, and adds noise to every `git status` on macOS. Recommend removing it with `git rm --cached packages/browseros/build_go/browseros` and ensuring CI builds from source instead.
### Issue 2 of 5
packages/browseros/build_go/internal/modules/sign/sparklesign.go:46-60
**Signing errors silently swallowed — DMGs can reach upload unsigned**
When `sparkle.SignFileWithEnv` returns an error for a DMG, the loop logs the error and `continue`s, so `Execute` returns `nil` even if every DMG failed to sign. The downstream `upload` module will then upload unsigned DMGs, generating an invalid appcast. The Python equivalent raises an exception on failure, stopping the pipeline. This should return an error on any signing failure, or at minimum check that `signed > 0` before returning.
```suggestion
signed := 0
for _, dmgPath := range dmgFiles {
name := filepath.Base(dmgPath)
logx.Info(fmt.Sprintf("🔐 Signing %s...", name))
sig, length, err := sparkle.SignFileWithEnv(dmgPath)
if err != nil {
return fmt.Errorf("failed to sign %s: %w", name, err)
}
ctx.SparkleSignatures[name] = buildctx.SparkleSig{Signature: sig, Length: length}
logx.Info(fmt.Sprintf(" %s: sig=%.20s... length=%d", name, sig, length))
signed++
}
if signed == 0 {
return fmt.Errorf("no DMG files were signed")
}
logx.Success(fmt.Sprintf("✅ Signed %d DMG(s) with Sparkle", signed))
return nil
```
### Issue 3 of 5
packages/browseros/build_go/internal/envx/envx.go:117-123
**`R2CDNBaseURL` default uses HTTP, not HTTPS**
The default `"http://cdn.browseros.com"` is used to construct public artifact URLs in `release.json` and appcasts. When no override is set, every published download URL will use plain HTTP, which exposes users to downgrade/MITM attacks during update downloads. The default should be `"https://cdn.browseros.com"`.
```suggestion
// R2CDNBaseURL defaults to the public CDN.
func R2CDNBaseURL() string {
if v := os.Getenv("R2_CDN_BASE_URL"); v != "" {
return v
}
return "https://cdn.browseros.com"
}
```
### Issue 4 of 5
packages/browseros/build_go/internal/otasrv/otasrv.go:155-175
**`platform` loop variable shadows the package-level `platform()` helper — likely compile error**
Inside `GenerateServerAppcast`, the first `for platform, artifact := range existing.Artifacts` loop introduces `platform` as a `string` variable. The subsequent `for _, artifact := range artifacts` loop then calls `merged[platform(artifact)] = artifact`, but at that point `platform` is still in scope as a `string`, not the `func platform(a SignedArtifact) string` helper. Go will resolve `platform(artifact)` as calling the string variable, which is not callable — this is a compile-time type error. Rename the helper to `platformKey` (or rename the loop variable) to remove the shadowing.
### Issue 5 of 5
packages/browseros/build_go/internal/execx/execx.go:109-115
**Extra env vars appended without deduplication**
`mergedEnv` appends key=value pairs to `os.Environ()` without removing pre-existing entries for the same key. On most platforms `exec.Command` uses the last occurrence, so overrides work in practice, but the behaviour is OS-specific and produces a bloated environment. Consider building a deduplicated map before serialising.
Reviews (2): Last reviewed commit: "chore(build_go): stop tracking the compi..." | Re-trigger Greptile
| signed := 0 | ||
| for _, dmgPath := range dmgFiles { | ||
| name := filepath.Base(dmgPath) | ||
| logx.Info(fmt.Sprintf("🔐 Signing %s...", name)) | ||
| sig, length, err := sparkle.SignFileWithEnv(dmgPath) | ||
| if err != nil { | ||
| logx.Error(fmt.Sprintf("Error signing %s: %v", name, err)) | ||
| continue | ||
| } | ||
| ctx.SparkleSignatures[name] = buildctx.SparkleSig{Signature: sig, Length: length} | ||
| logx.Info(fmt.Sprintf(" %s: sig=%.20s... length=%d", name, sig, length)) | ||
| signed++ | ||
| } | ||
| logx.Success(fmt.Sprintf("✅ Signed %d DMG(s) with Sparkle", signed)) | ||
| return nil |
There was a problem hiding this comment.
When sparkle.SignFileWithEnv returns an error for a DMG, the loop logs the error and continues, so Execute returns nil even if every DMG failed to sign. The downstream upload module will then upload unsigned DMGs, generating an invalid appcast. The Python equivalent raises an exception on failure, stopping the pipeline. This should return an error on any signing failure, or at minimum check that signed > 0 before returning.
| signed := 0 | |
| for _, dmgPath := range dmgFiles { | |
| name := filepath.Base(dmgPath) | |
| logx.Info(fmt.Sprintf("🔐 Signing %s...", name)) | |
| sig, length, err := sparkle.SignFileWithEnv(dmgPath) | |
| if err != nil { | |
| logx.Error(fmt.Sprintf("Error signing %s: %v", name, err)) | |
| continue | |
| } | |
| ctx.SparkleSignatures[name] = buildctx.SparkleSig{Signature: sig, Length: length} | |
| logx.Info(fmt.Sprintf(" %s: sig=%.20s... length=%d", name, sig, length)) | |
| signed++ | |
| } | |
| logx.Success(fmt.Sprintf("✅ Signed %d DMG(s) with Sparkle", signed)) | |
| return nil | |
| signed := 0 | |
| for _, dmgPath := range dmgFiles { | |
| name := filepath.Base(dmgPath) | |
| logx.Info(fmt.Sprintf("🔐 Signing %s...", name)) | |
| sig, length, err := sparkle.SignFileWithEnv(dmgPath) | |
| if err != nil { | |
| return fmt.Errorf("failed to sign %s: %w", name, err) | |
| } | |
| ctx.SparkleSignatures[name] = buildctx.SparkleSig{Signature: sig, Length: length} | |
| logx.Info(fmt.Sprintf(" %s: sig=%.20s... length=%d", name, sig, length)) | |
| signed++ | |
| } | |
| if signed == 0 { | |
| return fmt.Errorf("no DMG files were signed") | |
| } | |
| logx.Success(fmt.Sprintf("✅ Signed %d DMG(s) with Sparkle", signed)) | |
| return nil |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/build_go/internal/modules/sign/sparklesign.go
Line: 46-60
Comment:
**Signing errors silently swallowed — DMGs can reach upload unsigned**
When `sparkle.SignFileWithEnv` returns an error for a DMG, the loop logs the error and `continue`s, so `Execute` returns `nil` even if every DMG failed to sign. The downstream `upload` module will then upload unsigned DMGs, generating an invalid appcast. The Python equivalent raises an exception on failure, stopping the pipeline. This should return an error on any signing failure, or at minimum check that `signed > 0` before returning.
```suggestion
signed := 0
for _, dmgPath := range dmgFiles {
name := filepath.Base(dmgPath)
logx.Info(fmt.Sprintf("🔐 Signing %s...", name))
sig, length, err := sparkle.SignFileWithEnv(dmgPath)
if err != nil {
return fmt.Errorf("failed to sign %s: %w", name, err)
}
ctx.SparkleSignatures[name] = buildctx.SparkleSig{Signature: sig, Length: length}
logx.Info(fmt.Sprintf(" %s: sig=%.20s... length=%d", name, sig, length))
signed++
}
if signed == 0 {
return fmt.Errorf("no DMG files were signed")
}
logx.Success(fmt.Sprintf("✅ Signed %d DMG(s) with Sparkle", signed))
return nil
```
How can I resolve this? If you propose a fix, please make it concise.| // R2CDNBaseURL defaults to the public CDN. | ||
| func R2CDNBaseURL() string { | ||
| if v := os.Getenv("R2_CDN_BASE_URL"); v != "" { | ||
| return v | ||
| } | ||
| return "http://cdn.browseros.com" | ||
| } |
There was a problem hiding this comment.
R2CDNBaseURL default uses HTTP, not HTTPS
The default "http://cdn.browseros.com" is used to construct public artifact URLs in release.json and appcasts. When no override is set, every published download URL will use plain HTTP, which exposes users to downgrade/MITM attacks during update downloads. The default should be "https://cdn.browseros.com".
| // R2CDNBaseURL defaults to the public CDN. | |
| func R2CDNBaseURL() string { | |
| if v := os.Getenv("R2_CDN_BASE_URL"); v != "" { | |
| return v | |
| } | |
| return "http://cdn.browseros.com" | |
| } | |
| // R2CDNBaseURL defaults to the public CDN. | |
| func R2CDNBaseURL() string { | |
| if v := os.Getenv("R2_CDN_BASE_URL"); v != "" { | |
| return v | |
| } | |
| return "https://cdn.browseros.com" | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/build_go/internal/envx/envx.go
Line: 117-123
Comment:
**`R2CDNBaseURL` default uses HTTP, not HTTPS**
The default `"http://cdn.browseros.com"` is used to construct public artifact URLs in `release.json` and appcasts. When no override is set, every published download URL will use plain HTTP, which exposes users to downgrade/MITM attacks during update downloads. The default should be `"https://cdn.browseros.com"`.
```suggestion
// R2CDNBaseURL defaults to the public CDN.
func R2CDNBaseURL() string {
if v := os.Getenv("R2_CDN_BASE_URL"); v != "" {
return v
}
return "https://cdn.browseros.com"
}
```
How can I resolve this? If you propose a fix, please make it concise.| func mergedEnv(extra map[string]string) []string { | ||
| env := os.Environ() | ||
| for k, v := range extra { | ||
| env = append(env, k+"="+v) | ||
| } | ||
| return env | ||
| } |
There was a problem hiding this comment.
mergedEnv appends key=value pairs to os.Environ() without removing pre-existing entries for the same key. On most platforms exec.Command uses the last occurrence, so overrides work in practice, but the behaviour is OS-specific and produces a bloated environment. Consider building a deduplicated map before serialising.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/build_go/internal/execx/execx.go
Line: 109-115
Comment:
**Extra env vars appended without deduplication**
`mergedEnv` appends key=value pairs to `os.Environ()` without removing pre-existing entries for the same key. On most platforms `exec.Command` uses the last occurrence, so overrides work in practice, but the behaviour is OS-specific and produces a bloated environment. Consider building a deduplicated map before serialising.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…one in four
The question three passes deferred: does the narrowing point at the right place?
Measured against the regions a human named on delegations that succeeded:
browseros-ai#1156 handleWorkerFinished named the same hit
browseros-ai#1158 reset(), lines 66-365 named autoAccept... miss
browseros-ai#1165 chooseNextOpenIssue named requestReviewer miss
browseros-ai#1166 reset(), lines 66-365 named startAfterChoos miss
One in four, and both misses landed in reset() near the top of the file - the
original "narrowed to the header" defect, one function down. Density picks where
common words are thickest, which is wherever there are most of them.
The single hit was the one issue whose body names the function, so a matching
declaration name now beats any density. Re-measured: browseros-ai#1156 still right, browseros-ai#1158
moves to a neighbour its body names, browseros-ai#1165 and browseros-ai#1166 unchanged.
The contract asked for three of four. It was not met, and saying so is the point
of having set it. What the numbers show is that the narrowing is bounded by
whether the spec names the function - a defect in how the Queen writes specs,
not in how she reads files.
Refs Trinity-S-AI-DNA/trios#1173
…taken back
The measurement said the narrowing lands right once in four and named the
reason: the spec must name the function. The obvious next evidence was the log
event name - queen.review.verdicts is unique and sits as a literal at line 4595,
inside exactly the function a human had named by hand - and the localiser blanks
literals along with comments, so it could not see it.
Keeping the comment masking and dropping the literal masking measured worse:
browseros-ai#1156 reset() was handleWorkerFinished worse
browseros-ai#1158 acceptanceBlockReason… unchanged
browseros-ai#1165 reset() was chooseNextOpenIssue worse
browseros-ai#1166 reset() unchanged
Three of four in reset(). Literals carry a great many ordinary words, so
unmasking fed the density rule the noise that masking comments had been added to
remove. Sound reasoning, worse result - which is what measuring is for.
Restored to the committed version and re-measured to confirm: browseros-ai#1156 is
handleWorkerFinished again. The score stands at one clean hit and one useful
neighbour out of four.
Refs Trinity-S-AI-DNA/trios#1174
…hing
Four rules, one score:
density 1 of 4, two misses into reset()
declaration name beats density 1 of 4 plus a neighbour
unmask string literals 1 of 4, three misses into reset()
dotted names only 1 of 4, and browseros-ai#1158 regressed
Each traded one hit for another. Density points at whichever function holds the
most ordinary words, which in a 6,000-line file is a large early one every time.
The rule is now the only thing that ever worked - a declaration whose name the
spec actually writes - and nil otherwise:
browseros-ai#1156 silent browseros-ai#1158 acceptanceBlockReason… (its body names it)
browseros-ai#1165 silent browseros-ai#1166 silent
One right range, three silences, no wrong ranges. A brief without a range is
what worked before; a brief with a wrong range walks the bee somewhere
confidently wrong.
The silence on browseros-ai#1156 explains the earlier scores: its body never names
handleWorkerFinished - that name was in the delegation text I wrote by hand. The
narrowing had been reading my instructions rather than her spec.
Closes Trinity-S-AI-DNA/trios#1175
Closes Trinity-S-AI-DNA/trios#1174
…ros-ai#1176) (#307) Second pass. The first pass put the self-check in and then watched it change nothing - and said so - because the check compared a range's first line against the name extracted from that same line. Tautology: rule 1's window is clamped to the declaration line by construction, so the guard could never fire and removing it could not break anything. Criterion 4 was unmeetable as wired. The re-measurement found what actually produces wrong ranges on today's file, and it is not rule 1: 1. maskComments opened block comments inside string literals. A branch glob quoted at line 6864 - "No empty queen/* branch ..." - blanked the literal view from there to the end of the file: 6 726 lines of evidence invisible. Corroboration saw no mentions past the glob, browseros-ai#1158's guard and neighbour tied at zero, rule 1 went silent, and rule 4 answered with 6005-6304 - the middle of handleWorkerFinished, a function that spec never named. The confident wrong range browseros-ai#1176 is about, produced by the wrong rule through a blind view. maskComments tracks strings now, using the same model maskCommentsAndStrings always had. 2. The self-check moved to where ranges are born: every rule's answer passes through one finishing step - the first line of the returned range must declare a function, and the name rule must land on the very name it matched. A window that capping slid into the middle of a body begins nowhere in particular, and beginning nowhere in particular is silence. The check is live: queen.review.verdicts sits 322 lines into the 376-line requestReviewerVerdicts, its window would start at 7508, and the check silences it. Delete the two guards in finished() and that replay row goes red - verified from both sides; that is the criterion-4 property. The re-recording (identifiers unchanged; the boundary file moved to 13 590 lines): browseros-ai#1158 points at autoAcceptIfUnambiguous - the neighbour corroborates 0, the subject 2, measured not assumed; browseros-ai#1117 points at requestReviewerVerdicts (7432-7731, beginning at its declaration); browseros-ai#1156's clue moved upstream into settleCharacterCountVerdicts and the recording follows the clue, not the old address; browseros-ai#1166 unchanged at chooseNextOpenIssue. Expected gains declarationOrSilence - the issue's own acceptance, verbatim: point at the named function, or say nothing; a range that begins anywhere else is a FAIL. A returned range now counts as ok only when it BEGINS at the named function's declaration line and stays inside its extent - "somewhere inside" was the loophole the mid-body ranges walked through. One follow-up outside this task's boundary: the Linux module copy at agent-server/queen-core/Sources/QueenCore/QueenLocalisation.swift is byte-compared by make queen-core-sync and now differs until someone copies rings/SR-00 over it. The boundary names one file, so this task did not touch it. Co-authored-by: Trinity Bee <bee@trinity.local>
Summary
packages/browseros/build_go/(binary:browseros) porting the full Python build-tool CLI —build,dev,release,ota,upload— with flag/default/semantics parity, so any macOS/Linux/Windows machine can run the build tool with zero Python/uv setup.build/config/*.yamlincl. the!envtag,CHROMIUM_VERSION,resources/BROWSEROS_VERSION,build/config/BROWSEROS_BUILD_OFFSET,BASE_COMMIT,build/features.yaml) — no config duplication, no drift.Design
Hybrid faithful port: Python-shaped where parity matters (resolver CONFIG/DIRECT modes, Context derivations incl. BUILD+offset math, the 21-module registry keyed by the exact YAML module names, fail-fast pipeline with Slack notify hooks) and patch-tool-idiomatic where the repo already has Go conventions (cobra
cmd/, lipglossinternal/ui, injectedexecx.Runner). Subprocess-heavy modules are tested as recorded command-sequence assertions against the Python originals (codesign/notarytool order, gn/autoninja incl. the Windows RAM-capped-j, gclient, pkg-dmg, appimagetool/dpkg-deb, gh). Pure logic is reimplemented natively: Sparkle Ed25519 viacrypto/ed25519, a minimal R2/S3 client with hand-rolled SigV4 (pinned to AWS's published test vectors), appcast XML generation/merge, release.json generation. Direct deps stay minimal: cobra, lipgloss, yaml.v3, x/sys.Test plan
cd packages/browseros/build_go && make build && make test— 23 packages green (resolver parity pinned against the realbuild/config/*.yaml; patch engine exercised against real temp git repos; SigV4 against AWS vectors; Sparkle round-trip verified)make dist— static binaries for darwin/{arm64,amd64}, linux/{amd64,arm64}, windows/amd64 (CGO_ENABLED=0)./browseros build --listmatches the Python module registry; full--helpsweep over all five command trees🤖 Generated with Claude Code