Enrich the CLI experience with usecases to quickly try out ThunderID#3279
Merged
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a Go-based CLI with install/upgrade/sample orchestration and an interactive REPL, introduces cross-platform build scripts and Make targets for tools, replaces legacy npx-thunderid with a new tools/npx launcher/package, updates CI to release specific tool packages, and gates Wayfinder AI UI via an env flag. ChangesGo CLI implementation and npm launcher migration
Sequence Diagram(s)sequenceDiagram
participant CLI as cmd/thunderid/main
participant Root as cli.Run
participant Release as services/release
participant Setup as services/setup
participant Health as services/health
participant REPL as ui.RunREPL
CLI->>Root: cli.Run(verbose, forceSetup)
Root->>Release: FetchLatestVersion()
Release-->>Root: version
Root->>Release: Download(version, installPath)
Release-->>Root: extracted files
Root->>Setup: RunSetup(installPath)
Setup-->>Root: setup complete
Root->>Setup: StartBackground(installPath)
Setup-->>Root: started process
Root->>Health: ResolveBaseURL(port)
Health-->>Root: baseURL
Root->>REPL: RunREPL(version, proc, installPath, baseURL)
REPL-->>Root: upgradeRequested boolean
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/cli/go.mod (1)
1-46:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffNew module with ~30 dependencies requires explicit approval.
This PR introduces a new Go module with the Charm UI stack and numerous transitive dependencies. As per coding guidelines, new dependencies and changes to go.mod require explicit approval before merging.
Please confirm that the addition of these dependencies has been reviewed and approved:
- Direct:
bubbles,bubbletea,huh,huh/spinner,lipgloss,testify- Indirect: ~24 additional packages for terminal UI, clipboard, text processing, etc.
#!/bin/bash # List all new dependencies being introduced echo "=== Direct dependencies ===" grep -A 100 '^require (' tools/cli/go.mod | grep -v '^)' | grep -v '^require' | grep -v '//' | head -6 echo -e "\n=== Total dependency count ===" grep -c 'github.com\|golang.org\|gopkg.in' tools/cli/go.mod🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/cli/go.mod` around lines 1 - 46, New Go module tools/cli (module github.com/thunder-id/thunderid/tools/cli) adds several direct deps (bubbles, bubbletea, huh, huh/spinner, lipgloss, testify) and ~24 indirect packages and requires explicit approval; update the PR by adding an approvals note and dependency review: confirm security/maintenance review and a reviewer sign-off in the PR description, list the direct dependencies and count of total deps, and attach any rationale for choosing the Charm UI stack; also run go mod tidy and include generated go.sum changes in the commit so CI sees a clean module state.
🟡 Minor comments (9)
tools/cli/internal/cli/root.go-93-94 (1)
93-94:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRecompute first-run state after changing
runVersion.
isFirstRunis derived before the fallback branch can replace a missing active install withlatestVersion. When that happens, the REPL gets onboarding state for the old version instead of the version that was actually downloaded and started.Also applies to: 135-139, 171-171
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/cli/internal/cli/root.go` around lines 93 - 94, Recompute the onboarding check after any code path that mutates runVersion: move or re-evaluate isFirstRun = !config.IsOnboardingDone(runVersion) immediately after the fallback that replaces a missing active install with latestVersion (the branch that assigns latestVersion to runVersion/active install). Update the same pattern at the other occurrences you noted (the blocks around the earlier fallback logic near the code that sets runVersion and the one at 171) so that isFirstRun always reflects the final runVersion used; reference the variables isFirstRun, runVersion, latestVersion and the call config.IsOnboardingDone to locate and fix each site.build.ps1-534-538 (1)
534-538:⚠️ Potential issue | 🟡 MinorGuard/document
bashdependency forBuild-CLI(tools/cli/scripts/build.sh).
build.ps1’sBuild-CLIalways runs& bash "$PSScriptRoot/tools/cli/scripts/build.sh"(lines 534-538) and there’s nobash-availability check/fallback;tools/cli/scripts/build.shis a#!/bin/bashscript. Add aGet-Command bash-style check with a clear error (or document the required environment like Git Bash/WSL) for Windows users.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build.ps1` around lines 534 - 538, The Build-CLI function currently calls bash unconditionally; update the Build-CLI function to first verify bash is available (e.g., use Get-Command bash -ErrorAction SilentlyContinue) and if not present emit a clear error and exit (or print a short guidance message describing required environments like Git Bash/WSL) before attempting to run & bash "$PSScriptRoot/tools/cli/scripts/build.sh"; ensure the message references the script path tools/cli/scripts/build.sh so Windows users know what to install or which shell to use.build.ps1-575-597 (1)
575-597:⚠️ Potential issue | 🟡 MinorVerify golangci-lint installation before linting tools.
build.ps1’sLint-CLIandLint-I18n-Extractordirectly run$PSScriptRoot/backend/bin/tools/golangci-lint.exewithout anyTest-Path/bootstrap step first. Add a preflight existence check (and/or invoke the Makefilegolangci-linttarget) or document that prerequisite explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build.ps1` around lines 575 - 597, Add a preflight check in both Lint-CLI and Lint-I18n-Extractor to verify the $golangciLint path exists before invoking it: use Test-Path on the $golangciLint variable at the start of each function (Lint-CLI and Lint-I18n-Extractor) and if missing either call the bootstrap/Makefile target to install golangci-lint or write a clear error via Write-Host/Write-Error and exit with a non-zero code; ensure the check runs before Push-Location and before any invocation of & $golangciLint run ./....tools/npx/bin/thunderid.js-42-44 (1)
42-44:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDev fallback points to the wrong CLI directory.
At Line 42,
path.join(__dirname, '..', 'cli')resolves totools/npx/cli, so thego.modprobe at Line 43 misses the actual module intools/cli.Suggested fix
- const cliDir = path.join(__dirname, '..', 'cli'); + const cliDir = path.resolve(__dirname, '..', '..', 'cli');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/npx/bin/thunderid.js` around lines 42 - 44, The dev fallback is probing the wrong directory: the variable cliDir (set via path.join(__dirname, '..', 'cli')) points to tools/npx/cli instead of the real module in tools/cli; update the cliDir computation (used where cliDir is defined and where you call fs.existsSync(path.join(cliDir, 'go.mod')) and spawnSync('go', ['run', '.', ...])) to point one level up (e.g., join __dirname with '..', '..', 'cli' or otherwise resolve to the repo-level tools/cli) so the go.mod check and subsequent spawnSync execute against the actual CLI module.tools/cli/internal/ui/onboarding.go-114-114 (1)
114-114:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle onboarding state persistence failures explicitly.
At Line 114,
config.MarkOnboardingDoneerrors are discarded. If save fails, users may be repeatedly shown onboarding with no signal.Suggested fix
- _ = config.MarkOnboardingDone(m.version) + if err := config.MarkOnboardingDone(m.version); err != nil { + m.messages = append(m.messages, Yellow("⚠")+" Could not persist onboarding state: "+err.Error()) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/cli/internal/ui/onboarding.go` at line 114, The call to config.MarkOnboardingDone(m.version) currently discards errors; change it to capture the returned error and handle it explicitly: if it returns an error, log a clear message including m.version via the component's logger (e.g., m.logger or existing UI logger) and either surface/return the error from the current onboarding function so the caller can react or show a user-facing message indicating the save failed; do not ignore the error so onboarding state persistence failures are detectable and recoverable.tools/cli/internal/services/release/release.go-162-165 (1)
162-165:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace hardcoded "Thunder" with
product.Name.Same issue as above—use the constant instead of the hardcoded string.
♻️ Proposed fix
if total > 0 && onProgress != nil { pct := int(float64(received) / float64(total) * 100) - onProgress(pct, fmt.Sprintf("Downloading Thunder v%s", version)) + onProgress(pct, fmt.Sprintf("Downloading %s v%s", product.Name, version)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/cli/internal/services/release/release.go` around lines 162 - 165, Replace the hardcoded product name in the progress message with the product constant: change the string "Downloading Thunder v%s" used in the onProgress call inside the block that computes pct (the if total > 0 && onProgress != nil branch) to use product.Name (e.g., fmt.Sprintf("Downloading %s v%s", product.Name, version) or equivalent), ensuring you reference the existing product variable in this scope so the progress message uses the product.Name constant instead of the literal "Thunder".Source: Coding guidelines
tools/cli/internal/services/release/release.go-156-158 (1)
156-158:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace hardcoded "Thunder" with
product.Name.The progress message uses a hardcoded "Thunder" string literal instead of the
product.Nameconstant defined in the same package. This breaks the branding abstraction.♻️ Proposed fix
if onProgress != nil { - onProgress(-1, fmt.Sprintf("Downloading Thunder v%s for %s/%s", version, runtime.GOOS, runtime.GOARCH)) + onProgress(-1, fmt.Sprintf("Downloading %s v%s for %s/%s", product.Name, version, runtime.GOOS, runtime.GOARCH)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/cli/internal/services/release/release.go` around lines 156 - 158, Replace the hardcoded "Thunder" literal in the progress message with the package-level product.Name constant: update the fmt.Sprintf call where onProgress is invoked (the block using onProgress(-1, fmt.Sprintf("Downloading Thunder v%s for %s/%s", ...))) to use product.Name instead of "Thunder" so the message reads Downloading <product.Name> v%s for %s/%s; keep the existing formatting and arguments (version, runtime.GOOS, runtime.GOARCH) and ensure onProgress is still guarded by the same nil check.Source: Coding guidelines
tools/cli/internal/services/health/health_test.go-68-80 (1)
68-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis test doesn’t exercise
ResolveBaseURLsuccess path.Line 68 names a
ResolveBaseURLtest, but Line 79 assertsCheckReadyonly. Please parse the test server port and callResolveBaseURL(port, timeout)directly so the intended behavior is actually covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/cli/internal/services/health/health_test.go` around lines 68 - 80, The test TestResolveBaseURL_ReturnsURLWhenReady currently only calls health.CheckReady(srv.URL) and doesn't exercise ResolveBaseURL; update the test to extract the port from srv.URL (parse srv.URL to get the port or use srv.Listener.Addr()), then call ResolveBaseURL(port, timeout) and assert it returns the expected base URL (or non-nil URL and nil error) instead of only asserting CheckReady; keep or optionally still assert CheckReady(srv.URL) for coverage but ensure ResolveBaseURL(port, timeout) is invoked and its results are asserted.tools/cli/internal/services/setup/setup_test.go-83-87 (1)
83-87:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFixed port
19999makes this test flaky.Line 86 depends on a “likely free” port. If occupied on CI/host, this test fails nondeterministically. Use an OS-assigned ephemeral port (
:0), close it, then assertWaitForPortFreeon that released port.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/cli/internal/services/setup/setup_test.go` around lines 83 - 87, The test TestWaitForPortFree_UnoccupiedPort is flakey because it uses a hard-coded port 19999; replace that with an OS-assigned ephemeral port: in the test open a temporary listener with net.Listen("tcp", ":0"), read the assigned port from the listener's Addr(), close the listener to free the port, then call setup.WaitForPortFree on that released port (with the same timeout) and assert it returns true; this uses the runtime-assigned port instead of a fixed one to avoid CI conflicts.
🧹 Nitpick comments (1)
tools/cli/internal/product/product.go (1)
29-30: 💤 Low valueIs the hardcoded brand name in these URLs intentional?
Both
ReleasesURLandGitHubAPIembed "thunderid" or "thunder-id" in their endpoint paths. If these URLs might change or if the tool should support different branding, consider parameterizing the repository/organization identifiers or sourcing them from runtime configuration.Current:
ReleasesURL = "https://brionmario.github.io/thunderid/data/releases.json" GitHubAPI = "https://api.github.com/repos/thunder-id/thunderid/releases/latest"If these endpoints are fixed and brand-specific by design, this is acceptable. Otherwise, consider making the repository owner/name configurable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/cli/internal/product/product.go` around lines 29 - 30, ReleasesURL and GitHubAPI are hardcoded to the "thunderid"/"thunder-id" branding; make the repo owner/name configurable instead of fixed values by introducing parameters (e.g., RepoOwner and RepoName or a Config struct) and construct ReleasesURL and GitHubAPI dynamically from those values at runtime (or read from env/flags). Update usages that reference ReleasesURL and GitHubAPI to use the constructed URLs so different org/repo combos can be supplied without changing the constants.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 138-146: This change adds three Makefile targets (build_tools,
test_tools, lint_tools) that call build.sh and thus requires explicit approval
per repository policy; before merging, obtain and record that approval by
requesting review from the repo owners/code-owners or a designated approver and
add an approval comment or an "approved" label/sign-off on the PR that
references these targets and build.sh so there is an explicit audit trail; do
not merge or proceed with CI promotion until that approval is present.
In `@tools/cli/cmd/thunderid/main.go`:
- Around line 34-124: The PR added user-facing CLI behavior in main (commands:
upgrade, try, integrate) and flags parsed by parseFlags/parseUpgradeFlags and
printUsage but lacks documentation; add a new guide under docs/content/guides/
that documents the default CLI flow and each command (upgrade, try <usecase>,
integrate <tech>), global flags (--verbose, --setup, --help), upgrade modes
(direct vs side-by-side), staging-port and cutover flow, the sample workflow for
try wayfinder (prereqs, ports/URLs, startup steps), and AI-enabled vs non-AI
sample behavior including required env/config inputs; ensure the new docs
reference the functions/commands (main, parseFlags, parseUpgradeFlags,
printUsage, sample.Run, upgrade.Run) and include links/examples, then update the
PR to include these files before merging.
- Around line 50-58: The current check in main.go uses
release.FetchLatestVersion() and compares it only against
config.ReadActiveVersion(), which blocks "try" when the latest is installed but
not active or when an older active install should remain until upgrade; update
the logic so that after fetching version := release.FetchLatestVersion() and
activeVersion := config.ReadActiveVersion() you first check whether that latest
version is already installed (use the existing helper such as
config.IsVersionInstalled/versionExists or inspect
config.ReadInstalledVersions()), and if it is installed but not active, either
activate it or redirect the user into the upgrade flow (call the existing
upgrade/activate routine) instead of fataling; only show the "npx …" install
hint when the latest is neither installed nor active.
In `@tools/cli/internal/commands/sample/sample.go`:
- Around line 185-200: The sample cache is reused regardless of the fetched
version; modify the caching logic so the cache key includes the fetched version
(use version from release.FetchLatestVersion()) or persist/read a sample-version
metadata file and compare it before reusing; update the cacheDir calculation
(currently derived from installPath and sampleName) to include the version
(e.g., join version into the path) or, if using metadata, read a version file in
cacheDir and call release.DownloadSample(sampleName, version, cacheDir,
onDownload) when versions differ; adjust the progress messages accordingly to
reference the version when downloaded or when using an existing version.
In `@tools/cli/internal/commands/upgrade/upgrade.go`:
- Around line 127-142: The code currently calls
config.WriteActiveVersion(newVersion) before starting the replacement instance
(setup.StartBackground), which risks marking a non-running version active if the
start fails; move the call to config.WriteActiveVersion(newVersion) so it runs
only after setup.StartBackground succeeds and the process is confirmed ready
(i.e., after proc is returned and any readiness check or ui.RunREPL handoff
succeeds), and apply the same change for the other occurrence referenced around
the second block (the lines around ui.RunREPL/setup.LogDir); update references
to newVersion/newPath/verbose/product.Name accordingly so the activeVersion is
written only on successful start/readiness.
- Around line 120-130: The upgrade flow downloads and sets up newVersion at
newPath but never persists that install location, so subsequent runs still think
the version is missing; after runSetupWithPort and before/after
config.WriteActiveVersion, resolve the absolute path of newPath (use
filepath.Abs or equivalent) and persist it for the version using the same config
API used by the default install flow (e.g. call
config.WriteInstallPath(newVersion, absPath) or the project’s equivalent),
ensuring config.ReadInstallPath(newVersion) will return the on-disk path for
future launches; apply the same change for the other upgrade block around lines
146-152.
In `@tools/cli/internal/services/config/config.go`:
- Around line 30-33: StateDir currently ignores the error from os.UserHomeDir
causing invalid paths; change StateDir to return (string, error) instead of just
string, check and return any error from os.UserHomeDir, and update all callers
(e.g., statePath, load, save) to handle the returned error (propagate or wrap
with context) so failures are surfaced rather than producing paths like
"/.thunderid". Ensure callers cease assuming a non-empty home value and handle
the new error return accordingly.
In `@tools/cli/internal/services/health/health.go`:
- Around line 34-44: ResolveBaseURL currently ignores the caller timeout because
each CheckReady call uses a fixed internal 2s probe; change ResolveBaseURL to
enforce the overall deadline by computing the remaining time before each probe
(deadline - time.Now()), aborting when remaining <= 0, and passing that
remaining duration into CheckReady (or a new CheckReadyWithTimeout that accepts
a timeout param) instead of relying on a hardcoded 2s; apply the same change to
the other similar function in the file (the block around lines 49-57) so every
probe uses the remaining budget and the function never overruns the caller
timeout.
In `@tools/cli/internal/services/setup/setup_test.go`:
- Around line 33-36: The test TestLogDir and related cases assume Unix paths and
hardcoded script names; change the assertions and test setup to be OS-agnostic
by using filepath.Join to construct expected paths when calling setup.LogDir,
and pick the script filename based on runtime.GOOS (use "setup.ps1" on windows,
"setup.sh" otherwise) when creating test files used by FindThunderRoot; update
references in the tests around TestLogDir and the cases at lines ~56-74 to use
filepath.Join and conditional script name selection so the tests pass on Windows
and Unix CI runners.
In `@tools/cli/internal/services/setup/setup.go`:
- Around line 130-134: Replace the hardcoded credentials and unconditional
security disable in the env construction (the block that appends
"ADMIN_USERNAME=admin", "ADMIN_PASSWORD=admin", "THUNDER_SKIP_SECURITY=true")
with a secure default flow: add an explicit opt-in flag like --insecure-dev-mode
to the setup command handler, generate a random password (e.g., via a
randomPassword helper) when insecure mode is requested, set
"THUNDER_SKIP_SECURITY=false" by default and only set it true when the opt-in
flag is present, populate ADMIN_PASSWORD using the generated password, and print
a prominent warning and the generated admin password to stdout after setup
succeeds so developers see the credentials. Ensure the changes touch the env
assembly code and the command setup/RunSetupOnPort command handler to wire the
flag, generation, and user output.
In `@tools/cli/internal/ui/repl.go`:
- Around line 888-893: After handling sampleErrMsg (the case that resets
m.tryingOut, m.trySampleStatus, m.sampleProgressCh and appends to m.messages),
restore the REPL input focus so the UI becomes interactive again; add a call to
restore focus (e.g., m.setInputFocused(true) or the equivalent method/property
your UI uses) immediately after those resets so the input regains focus after a
sample failure.
- Around line 739-742: The loop that advances m.ucStep currently skips inputs if
a key exists in m.ucValues (using presence-only check), which incorrectly treats
empty persisted values as configured; change the condition in the loop that
inspects m.ucValues[m.ucInputs[m.ucStep].Key] to verify the stored value is
non-empty (e.g., retrieve the value, ensure it's not nil/zero/empty string)
before skipping — only increment m.ucStep when the key exists AND the value is
non-empty/valid; references: m.ucStep, m.ucInputs, m.ucValues.
In `@tools/cli/internal/ui/usecases.go`:
- Around line 63-79: The PR adds new CLI onboarding inputs (ConfigInput entries
LLM_PROVIDER and LLM_API_KEY) and new UX flows (SampleEnvTarget "ai-agent" and
SampleFeatures "ai") but lacks documentation updates; add/update docs in
docs/content/guides/ to cover the first-run picker behavior, use-case selection
walkthrough, and the new slash commands (/open-console, /status, /logs,
/upgrade, /cutover), and add a section describing the new required config keys
LLM_PROVIDER (accepted values like "anthropic", "gemini") and LLM_API_KEY
(storage/secret handling and where it is written), then cross-link these docs
from docs/content/apis.mdx so the CLI/API reference indexes the changes.
In `@tools/npx/bin/thunderid.js`:
- Around line 24-70: Add documentation covering the NPX launcher behavior and
the CLI distribution/build flow: describe platform/architecture resolution
(PLATFORM_MAP/ARCH_MAP and how platform/arch are chosen), binary lookup and
fallback behavior (binaryName, binaryPath, the go run dev fallback using
spawnSync), error flows/messages (unsupported platform/arch and missing binary
errors), and how dist artifacts are produced for releases (how tools produce
files expected in dist). Create/update guides under docs/content (a CLI
install/run guide explaining runtime semantics and error messages, and a
tooling/release page describing how tools/npx/dist artifacts are built and
released).
In `@tools/npx/package.json`:
- Around line 1-35: This new tools/npx/package.json addition requires explicit
approval before merging; update the PR to request and capture that approval by
(1) adding a clear approval note in the PR description referencing the new
package name "thunderid", the bin entry "./bin/thunderid.js", and the scripts
"build" and "clean" (which map to tools/npx/scripts/build.js and
tools/npx/scripts/clean.js), and (2) obtaining an explicit approval from the
repository maintainers or the CODEOWNERS team responsible for tooling; include
the approver's GitHub username or an approval checklist item in the PR so it’s
recorded before merging.
In `@tools/npx/scripts/clean.js`:
- Around line 19-21: rmSync('dist', ...) is CWD-dependent; require('path') and
compute an absolute, script-relative path before deleting (e.g. const target =
path.resolve(__dirname, '..', '..', '..', 'dist')) and then call rmSync(target,
{ recursive: true, force: true }); update imports to include const path =
require('path') and replace the string 'dist' with the computed target so the
cleanup always targets the intended directory relative to this script.
---
Outside diff comments:
In `@tools/cli/go.mod`:
- Around line 1-46: New Go module tools/cli (module
github.com/thunder-id/thunderid/tools/cli) adds several direct deps (bubbles,
bubbletea, huh, huh/spinner, lipgloss, testify) and ~24 indirect packages and
requires explicit approval; update the PR by adding an approvals note and
dependency review: confirm security/maintenance review and a reviewer sign-off
in the PR description, list the direct dependencies and count of total deps, and
attach any rationale for choosing the Charm UI stack; also run go mod tidy and
include generated go.sum changes in the commit so CI sees a clean module state.
---
Minor comments:
In `@build.ps1`:
- Around line 534-538: The Build-CLI function currently calls bash
unconditionally; update the Build-CLI function to first verify bash is available
(e.g., use Get-Command bash -ErrorAction SilentlyContinue) and if not present
emit a clear error and exit (or print a short guidance message describing
required environments like Git Bash/WSL) before attempting to run & bash
"$PSScriptRoot/tools/cli/scripts/build.sh"; ensure the message references the
script path tools/cli/scripts/build.sh so Windows users know what to install or
which shell to use.
- Around line 575-597: Add a preflight check in both Lint-CLI and
Lint-I18n-Extractor to verify the $golangciLint path exists before invoking it:
use Test-Path on the $golangciLint variable at the start of each function
(Lint-CLI and Lint-I18n-Extractor) and if missing either call the
bootstrap/Makefile target to install golangci-lint or write a clear error via
Write-Host/Write-Error and exit with a non-zero code; ensure the check runs
before Push-Location and before any invocation of & $golangciLint run ./....
In `@tools/cli/internal/cli/root.go`:
- Around line 93-94: Recompute the onboarding check after any code path that
mutates runVersion: move or re-evaluate isFirstRun =
!config.IsOnboardingDone(runVersion) immediately after the fallback that
replaces a missing active install with latestVersion (the branch that assigns
latestVersion to runVersion/active install). Update the same pattern at the
other occurrences you noted (the blocks around the earlier fallback logic near
the code that sets runVersion and the one at 171) so that isFirstRun always
reflects the final runVersion used; reference the variables isFirstRun,
runVersion, latestVersion and the call config.IsOnboardingDone to locate and fix
each site.
In `@tools/cli/internal/services/health/health_test.go`:
- Around line 68-80: The test TestResolveBaseURL_ReturnsURLWhenReady currently
only calls health.CheckReady(srv.URL) and doesn't exercise ResolveBaseURL;
update the test to extract the port from srv.URL (parse srv.URL to get the port
or use srv.Listener.Addr()), then call ResolveBaseURL(port, timeout) and assert
it returns the expected base URL (or non-nil URL and nil error) instead of only
asserting CheckReady; keep or optionally still assert CheckReady(srv.URL) for
coverage but ensure ResolveBaseURL(port, timeout) is invoked and its results are
asserted.
In `@tools/cli/internal/services/release/release.go`:
- Around line 162-165: Replace the hardcoded product name in the progress
message with the product constant: change the string "Downloading Thunder v%s"
used in the onProgress call inside the block that computes pct (the if total > 0
&& onProgress != nil branch) to use product.Name (e.g., fmt.Sprintf("Downloading
%s v%s", product.Name, version) or equivalent), ensuring you reference the
existing product variable in this scope so the progress message uses the
product.Name constant instead of the literal "Thunder".
- Around line 156-158: Replace the hardcoded "Thunder" literal in the progress
message with the package-level product.Name constant: update the fmt.Sprintf
call where onProgress is invoked (the block using onProgress(-1,
fmt.Sprintf("Downloading Thunder v%s for %s/%s", ...))) to use product.Name
instead of "Thunder" so the message reads Downloading <product.Name> v%s for
%s/%s; keep the existing formatting and arguments (version, runtime.GOOS,
runtime.GOARCH) and ensure onProgress is still guarded by the same nil check.
In `@tools/cli/internal/services/setup/setup_test.go`:
- Around line 83-87: The test TestWaitForPortFree_UnoccupiedPort is flakey
because it uses a hard-coded port 19999; replace that with an OS-assigned
ephemeral port: in the test open a temporary listener with net.Listen("tcp",
":0"), read the assigned port from the listener's Addr(), close the listener to
free the port, then call setup.WaitForPortFree on that released port (with the
same timeout) and assert it returns true; this uses the runtime-assigned port
instead of a fixed one to avoid CI conflicts.
In `@tools/cli/internal/ui/onboarding.go`:
- Line 114: The call to config.MarkOnboardingDone(m.version) currently discards
errors; change it to capture the returned error and handle it explicitly: if it
returns an error, log a clear message including m.version via the component's
logger (e.g., m.logger or existing UI logger) and either surface/return the
error from the current onboarding function so the caller can react or show a
user-facing message indicating the save failed; do not ignore the error so
onboarding state persistence failures are detectable and recoverable.
In `@tools/npx/bin/thunderid.js`:
- Around line 42-44: The dev fallback is probing the wrong directory: the
variable cliDir (set via path.join(__dirname, '..', 'cli')) points to
tools/npx/cli instead of the real module in tools/cli; update the cliDir
computation (used where cliDir is defined and where you call
fs.existsSync(path.join(cliDir, 'go.mod')) and spawnSync('go', ['run', '.',
...])) to point one level up (e.g., join __dirname with '..', '..', 'cli' or
otherwise resolve to the repo-level tools/cli) so the go.mod check and
subsequent spawnSync execute against the actual CLI module.
---
Nitpick comments:
In `@tools/cli/internal/product/product.go`:
- Around line 29-30: ReleasesURL and GitHubAPI are hardcoded to the
"thunderid"/"thunder-id" branding; make the repo owner/name configurable instead
of fixed values by introducing parameters (e.g., RepoOwner and RepoName or a
Config struct) and construct ReleasesURL and GitHubAPI dynamically from those
values at runtime (or read from env/flags). Update usages that reference
ReleasesURL and GitHubAPI to use the constructed URLs so different org/repo
combos can be supplied without changing the constants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f182082a-61ec-43bc-a993-e5cb05ab779f
⛔ Files ignored due to path filters (3)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltools/cli/go.sumis excluded by!**/*.sumtools/npx-thunderid/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (64)
.github/actions/release-tool/npm/action.yml.github/workflows/release-tools.ymlMakefilebuild.ps1build.shsamples/apps/wayfinder-sample/frontend/.env.examplesamples/apps/wayfinder-sample/frontend/src/App.jsxsamples/apps/wayfinder-sample/frontend/src/main.jsxsamples/apps/wayfinder-sample/package.jsontools/cli/.golangci.ymltools/cli/cmd/thunderid/main.gotools/cli/go.modtools/cli/internal/cli/root.gotools/cli/internal/commands/sample/sample.gotools/cli/internal/commands/upgrade/upgrade.gotools/cli/internal/product/product.gotools/cli/internal/product/product_test.gotools/cli/internal/services/config/config.gotools/cli/internal/services/config/config_test.gotools/cli/internal/services/health/health.gotools/cli/internal/services/health/health_test.gotools/cli/internal/services/release/release.gotools/cli/internal/services/release/release_test.gotools/cli/internal/services/setup/setup.gotools/cli/internal/services/setup/setup_test.gotools/cli/internal/ui/banner.gotools/cli/internal/ui/onboarding.gotools/cli/internal/ui/repl.gotools/cli/internal/ui/spinner/spinner.gotools/cli/internal/ui/spinner/spinner_test.gotools/cli/internal/ui/usecases.gotools/cli/internal/utils/browser.gotools/cli/scripts/build.ps1tools/cli/scripts/build.shtools/npx-thunderid/bin/thunderid.jstools/npx-thunderid/eslint.config.jstools/npx-thunderid/package.jsontools/npx-thunderid/rolldown.config.jstools/npx-thunderid/src/constants/Product.tstools/npx-thunderid/src/constants/ThunderRepo.tstools/npx-thunderid/src/deploy/index.tstools/npx-thunderid/src/deploy/recipes/fly.tstools/npx-thunderid/src/deploy/recipes/index.tstools/npx-thunderid/src/deploy/recipes/railway.tstools/npx-thunderid/src/deploy/recipes/render.tstools/npx-thunderid/src/download.tstools/npx-thunderid/src/index.tstools/npx-thunderid/src/models/db.tstools/npx-thunderid/src/models/deploy.tstools/npx-thunderid/src/setup.tstools/npx-thunderid/src/state.tstools/npx-thunderid/tsconfig.eslint.jsontools/npx-thunderid/tsconfig.jsontools/npx-thunderid/tsconfig.lib.jsontools/npx-thunderid/tsconfig.spec.jsontools/npx-thunderid/vitest.config.tstools/npx/.editorconfigtools/npx/.gitignoretools/npx/.prettierignoretools/npx/README.mdtools/npx/bin/thunderid.jstools/npx/package.jsontools/npx/scripts/build.jstools/npx/scripts/clean.js
💤 Files with no reviewable changes (22)
- tools/npx-thunderid/tsconfig.json
- tools/npx-thunderid/src/deploy/recipes/index.ts
- tools/npx-thunderid/bin/thunderid.js
- tools/npx-thunderid/src/models/db.ts
- tools/npx-thunderid/tsconfig.spec.json
- tools/npx-thunderid/src/index.ts
- tools/npx-thunderid/src/download.ts
- tools/npx-thunderid/src/constants/ThunderRepo.ts
- tools/npx-thunderid/vitest.config.ts
- tools/npx-thunderid/package.json
- tools/npx-thunderid/src/state.ts
- tools/npx-thunderid/eslint.config.js
- tools/npx-thunderid/src/deploy/recipes/render.ts
- tools/npx-thunderid/src/constants/Product.ts
- tools/npx-thunderid/src/setup.ts
- tools/npx-thunderid/rolldown.config.js
- tools/npx-thunderid/src/deploy/index.ts
- tools/npx-thunderid/src/deploy/recipes/fly.ts
- tools/npx-thunderid/tsconfig.eslint.json
- tools/npx-thunderid/tsconfig.lib.json
- tools/npx-thunderid/src/deploy/recipes/railway.ts
- tools/npx-thunderid/src/models/deploy.ts
Comment on lines
+34
to
+124
| func main() { | ||
| args := os.Args[1:] | ||
|
|
||
| // upgrade [--direct] — explicit upgrade with optional blue/green staging. | ||
| if len(args) > 0 && args[0] == "upgrade" { | ||
| verbose, direct := parseUpgradeFlags(args[1:]) | ||
| if err := upgrade.Run(cli.BaseDir(), upgrade.Opts{Direct: direct, Verbose: verbose}); err != nil { | ||
| os.Exit(1) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| // try <usecase> — download and launch a use-case sample app. | ||
| if len(args) >= 2 && args[0] == "try" { | ||
| usecase := args[1] | ||
| verbose, _ := parseFlags(args[2:]) | ||
| version, err := release.FetchLatestVersion() | ||
| if err != nil { | ||
| ui.Fatal("Could not fetch latest version: " + err.Error()) | ||
| os.Exit(1) | ||
| } | ||
| activeVersion := config.ReadActiveVersion() | ||
| if activeVersion != version { | ||
| ui.Fatal(fmt.Sprintf("%s v%s is not installed. Run `npx %s` first.", product.Name, version, product.Slug)) | ||
| os.Exit(1) | ||
| } | ||
| path := cli.VersionedInstallPath(activeVersion) | ||
| if err := sample.Run(usecase, path, verbose, sample.Options{}); err != nil { | ||
| ui.Fatal(err.Error()) | ||
| os.Exit(1) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| // integrate <technology> — configure a technology integration (future). | ||
| if len(args) >= 2 && args[0] == "integrate" { | ||
| ui.Fatal(fmt.Sprintf("`integrate %s` is not yet implemented.", args[1])) | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| if len(args) > 0 && (args[0] == "--help" || args[0] == "-h") { | ||
| printUsage() | ||
| return | ||
| } | ||
|
|
||
| verbose, forceSetup := parseFlags(args) | ||
| cli.Run(verbose, forceSetup) | ||
| } | ||
|
|
||
| func printUsage() { | ||
| fmt.Printf(`Usage: %s [command] [flags] | ||
|
|
||
| Commands: | ||
| (none) Install and start %s | ||
| upgrade Upgrade to the latest release (side-by-side by default) | ||
| try <usecase> Download and launch a use-case sample app | ||
| integrate <tech> Configure a technology integration (coming soon) | ||
|
|
||
| Flags: | ||
| --verbose, -v Show detailed output | ||
| --setup Force re-run setup | ||
| --help, -h Show this help message | ||
|
|
||
| Upgrade flags: | ||
| --direct Upgrade in-place (stop current, upgrade, restart) | ||
| `, product.Slug, product.Name) | ||
| } | ||
|
|
||
| func parseFlags(args []string) (verbose, forceSetup bool) { | ||
| for _, a := range args { | ||
| switch a { | ||
| case "--verbose", "-v": | ||
| verbose = true | ||
| case "--setup": | ||
| forceSetup = true | ||
| } | ||
| } | ||
| return | ||
| } | ||
|
|
||
| func parseUpgradeFlags(args []string) (verbose, direct bool) { | ||
| for _, a := range args { | ||
| switch a { | ||
| case "--verbose", "-v": | ||
| verbose = true | ||
| case "--direct": | ||
| direct = true | ||
| } | ||
| } | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates under docs/.
Please update the relevant documentation before merging.
Missing documentation:
- CLI command usage: document the new default CLI flow plus
upgrade,try <usecase>,integrate <tech>, and the new global flags (--verbose,--setup,--help) in a guide underdocs/content/guides/. - Upgrade behavior: document direct vs side-by-side upgrade modes, the staging-port behavior, and the cutover flow in
docs/content/guides/. - Sample workflow: document
try wayfinder, its local prerequisites, expected ports/URLs, and the sample startup flow indocs/content/guides/. - AI-enabled sample mode: document the AI-vs-non-AI sample behavior and any required env/config inputs for the generated service/frontend setup in
docs/content/guides/.
As per coding guidelines, "If ANY of the above are detected and the PR does NOT include corresponding updates under docs/ ... post a single consolidated PR-level comment."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/cli/cmd/thunderid/main.go` around lines 34 - 124, The PR added
user-facing CLI behavior in main (commands: upgrade, try, integrate) and flags
parsed by parseFlags/parseUpgradeFlags and printUsage but lacks documentation;
add a new guide under docs/content/guides/ that documents the default CLI flow
and each command (upgrade, try <usecase>, integrate <tech>), global flags
(--verbose, --setup, --help), upgrade modes (direct vs side-by-side),
staging-port and cutover flow, the sample workflow for try wayfinder (prereqs,
ports/URLs, startup steps), and AI-enabled vs non-AI sample behavior including
required env/config inputs; ensure the new docs reference the functions/commands
(main, parseFlags, parseUpgradeFlags, printUsage, sample.Run, upgrade.Run) and
include links/examples, then update the PR to include these files before
merging.
Source: Coding guidelines
Comment on lines
+63
to
+79
| RequiredConfigs: []ConfigInput{ | ||
| { | ||
| Key: "LLM_PROVIDER", | ||
| Label: "LLM provider for the AI concierge", | ||
| Choices: []Choice{ | ||
| {Value: "anthropic", Label: "Anthropic (Claude)"}, | ||
| {Value: "gemini", Label: "Gemini"}, | ||
| }, | ||
| }, | ||
| { | ||
| Key: "LLM_API_KEY", | ||
| Label: "API key", | ||
| Secret: true, | ||
| }, | ||
| }, | ||
| SampleEnvTarget: "ai-agent", | ||
| SampleFeatures: []string{"ai"}, |
Contributor
There was a problem hiding this comment.
🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates under docs/.
Please update the relevant documentation before merging.
Missing documentation:
- CLI onboarding and new interactive flows: document first-run picker behavior, use-case selection, and walkthrough UX in
docs/content/guides/(new or updated CLI guide page). - New slash commands and behavior: document
/open-console,/status,/logs,/upgrade(and staging/cutover) command semantics and outcomes indocs/content/guides/. - New required sample configuration inputs: document
LLM_PROVIDERandLLM_API_KEY(accepted values, where written, and security handling expectations) indocs/content/guides/and cross-link fromdocs/content/apis.mdxif this is part of the CLI/API reference index.
As per coding guidelines, “If ANY of the above are detected and the PR does NOT include corresponding updates under docs/, post a single consolidated PR-level comment.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/cli/internal/ui/usecases.go` around lines 63 - 79, The PR adds new CLI
onboarding inputs (ConfigInput entries LLM_PROVIDER and LLM_API_KEY) and new UX
flows (SampleEnvTarget "ai-agent" and SampleFeatures "ai") but lacks
documentation updates; add/update docs in docs/content/guides/ to cover the
first-run picker behavior, use-case selection walkthrough, and the new slash
commands (/open-console, /status, /logs, /upgrade, /cutover), and add a section
describing the new required config keys LLM_PROVIDER (accepted values like
"anthropic", "gemini") and LLM_API_KEY (storage/secret handling and where it is
written), then cross-link these docs from docs/content/apis.mdx so the CLI/API
reference indexes the changes.
Source: Coding guidelines
Comment on lines
+24
to
+70
| const PRODUCT_NAME_LOWERCASE = 'thunderid'; | ||
| const PLATFORM_MAP = { darwin: 'darwin', linux: 'linux', win32: 'win' }; | ||
| const ARCH_MAP = { x64: 'x64', arm64: 'arm64' }; | ||
|
|
||
| const platform = PLATFORM_MAP[process.platform]; | ||
| const arch = ARCH_MAP[process.arch]; | ||
|
|
||
| if (!platform || !arch) { | ||
| process.stderr.write(`Unsupported platform: ${process.platform}/${process.arch}\n`); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const ext = process.platform === 'win32' ? '.exe' : ''; | ||
| const binaryName = `${PRODUCT_NAME_LOWERCASE}-${platform}-${arch}${ext}`; | ||
| const binaryPath = path.join(__dirname, '..', 'dist', binaryName); | ||
|
|
||
| if (!fs.existsSync(binaryPath)) { | ||
| // Dev fallback: run via `go run` if the pre-built binary is absent. | ||
| const cliDir = path.join(__dirname, '..', 'cli'); | ||
| if (fs.existsSync(path.join(cliDir, 'go.mod'))) { | ||
| const result = spawnSync('go', ['run', '.', ...process.argv.slice(2)], { | ||
| cwd: cliDir, | ||
| stdio: 'inherit', | ||
| env: process.env, | ||
| }); | ||
| process.exit(result.status ?? 1); | ||
| } | ||
| process.stderr.write( | ||
| `Binary not found: ${binaryPath}\nRun "pnpm build" to compile the Go CLI.\n`, | ||
| ); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| if (process.platform !== 'win32') { | ||
| try { | ||
| fs.chmodSync(binaryPath, 0o755); | ||
| } catch { | ||
| // ignore — may already be executable | ||
| } | ||
| } | ||
|
|
||
| const result = spawnSync(binaryPath, process.argv.slice(2), { | ||
| stdio: 'inherit', | ||
| env: process.env, | ||
| }); | ||
|
|
||
| process.exit(result.status ?? 1); |
Contributor
There was a problem hiding this comment.
🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates under docs/.
Please update the relevant documentation before merging.
Missing documentation:
- NPX launcher runtime behavior: document platform/architecture resolution, binary lookup behavior, and error flows in
docs/content/guides/(add/update a CLI install/run guide). - CLI distribution/build flow for the NPX package: document how
tools/npx/distartifacts are produced and expected for releases indocs/content/guides/(or a tooling/release page underdocs/content/).
As per coding guidelines, “If ANY of the above are detected and the PR does NOT include corresponding updates under docs/ … post a single consolidated PR-level comment.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/npx/bin/thunderid.js` around lines 24 - 70, Add documentation covering
the NPX launcher behavior and the CLI distribution/build flow: describe
platform/architecture resolution (PLATFORM_MAP/ARCH_MAP and how platform/arch
are chosen), binary lookup and fallback behavior (binaryName, binaryPath, the go
run dev fallback using spawnSync), error flows/messages (unsupported
platform/arch and missing binary errors), and how dist artifacts are produced
for releases (how tools produce files expected in dist). Create/update guides
under docs/content (a CLI install/run guide explaining runtime semantics and
error messages, and a tooling/release page describing how tools/npx/dist
artifacts are built and released).
Source: Coding guidelines
brionmario
force-pushed
the
feat-mobile-sdks
branch
from
cd4c495 to
95b275c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
brionmario
force-pushed
the
feat-mobile-sdks
branch
from
95b275c to
f2ecd1a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
brionmario
changed the title
brionmario
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
brionmario
added a commit
to brionmario/thunderid
that referenced
this pull request
Jun 11, 2026
- config: fall back to OS temp dir when UserHomeDir fails instead of silently producing an invalid path - health: cap each ResolveBaseURL probe to the remaining deadline so the function never overruns its timeout budget - upgrade: write install path and active version only after StartBackground succeeds (both runDirect and performCutover), preventing corrupt state on launch failure - sample: invalidate the sample cache when the fetched release version differs from the cached version - main: allow `try` with any active install, not just when active == latest; drop unused release import - setup_test: use t.TempDir() and the platform-correct setup script to fix Windows CI failures - setup: respect THUNDER_ADMIN_USERNAME/THUNDER_ADMIN_PASSWORD env overrides so credentials are configurable - repl: skip persisted config values that are empty strings so required prompts are not bypassed - repl: restore input focus after sampleErrMsg when the server is still ready - clean.js: use __dirname-relative path for dist so the script is safe to invoke from any CWD Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
brionmario
added a commit
to brionmario/thunderid
that referenced
this pull request
Jun 11, 2026
- config: fall back to OS temp dir when UserHomeDir fails instead of silently producing an invalid path - health: cap each ResolveBaseURL probe to the remaining deadline so the function never overruns its timeout budget - upgrade: write install path and active version only after StartBackground succeeds (both runDirect and performCutover), preventing corrupt state on launch failure - sample: invalidate the sample cache when the fetched release version differs from the cached version - main: allow `try` with any active install, not just when active == latest; drop unused release import - setup_test: use t.TempDir() and the platform-correct setup script to fix Windows CI failures - setup: respect THUNDER_ADMIN_USERNAME/THUNDER_ADMIN_PASSWORD env overrides so credentials are configurable - repl: skip persisted config values that are empty strings so required prompts are not bypassed - repl: restore input focus after sampleErrMsg when the server is still ready - clean.js: use __dirname-relative path for dist so the script is safe to invoke from any CWD Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
brionmario
force-pushed
the
feat-mobile-sdks
branch
from
fe4897f to
d4f4057
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/cli/cmd/thunderid/main.go (1)
54-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate that the install path exists before calling
sample.Run.If the active version is configured but the install directory has been deleted, the current code will fail deeper in
sample.Runwith a potentially confusing error. Add a check to verify the path exists on disk and provide a clear error message if it doesn't.🛡️ Proposed fix to validate install path
path := cli.VersionedInstallPath(activeVersion) + if _, err := os.Stat(path); err != nil { + ui.Fatal(fmt.Sprintf("Active %s v%s install not found at %s. Run `npx %s --setup` to reinstall.", + product.Name, activeVersion, path, product.Slug)) + os.Exit(1) + } if err := sample.Run(usecase, path, verbose, sample.Options{}); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/cli/cmd/thunderid/main.go` around lines 54 - 55, The code calls cli.VersionedInstallPath(activeVersion) and immediately passes the resulting path into sample.Run(usecase, path, verbose, sample.Options{}), but it doesn’t verify the path exists; add a filesystem existence check (os.Stat or similar) after obtaining path and before calling sample.Run, and if the directory is missing return a clear error message like “install path for version X not found: <path>” (or log and exit) so sample.Run is not invoked on a non-existent install directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tools/cli/cmd/thunderid/main.go`:
- Around line 54-55: The code calls cli.VersionedInstallPath(activeVersion) and
immediately passes the resulting path into sample.Run(usecase, path, verbose,
sample.Options{}), but it doesn’t verify the path exists; add a filesystem
existence check (os.Stat or similar) after obtaining path and before calling
sample.Run, and if the directory is missing return a clear error message like
“install path for version X not found: <path>” (or log and exit) so sample.Run
is not invoked on a non-existent install directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e1ed2031-d40f-44c7-b79a-5e57dbcdc132
📒 Files selected for processing (9)
tools/cli/cmd/thunderid/main.gotools/cli/internal/commands/sample/sample.gotools/cli/internal/commands/upgrade/upgrade.gotools/cli/internal/services/config/config.gotools/cli/internal/services/health/health.gotools/cli/internal/services/setup/setup.gotools/cli/internal/services/setup/setup_test.gotools/cli/internal/ui/repl.gotools/npx/scripts/clean.js
🚧 Files skipped from review as they are similar to previous changes (7)
- tools/cli/internal/services/health/health.go
- tools/cli/internal/services/setup/setup_test.go
- tools/cli/internal/services/config/config.go
- tools/cli/internal/services/setup/setup.go
- tools/cli/internal/commands/upgrade/upgrade.go
- tools/cli/internal/commands/sample/sample.go
- tools/cli/internal/ui/repl.go
brionmario
force-pushed
the
feat-mobile-sdks
branch
from
d4f4057 to
20fee95
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
tools/npx/scripts/build.js (1)
27-36: ⚡ Quick winStatic analysis false positive, but consider defensive improvement.
The static analysis tool flagged potential command injection, but this is a false positive since all path components are derived from
__dirnameand hardcoded strings with no user input. However, for defense-in-depth and to silence linter warnings, consider usingspawnSyncwith an argument array instead ofexecSyncwith a shell string.🛡️ Defensive refactor using spawnSync
-const { execSync } = require('child_process'); +const { spawnSync } = require('child_process'); const fs = require('fs'); const path = require('path'); const cliDir = path.resolve(__dirname, '../../cli'); const cliDist = path.join(cliDir, 'dist'); const npxDist = path.resolve(__dirname, '../dist'); +let result; if (process.platform === 'win32') { - execSync( - `powershell.exe -ExecutionPolicy Bypass -File "${path.join(cliDir, 'scripts', 'build.ps1')}"`, - { stdio: 'inherit' }, - ); + result = spawnSync('powershell.exe', [ + '-ExecutionPolicy', 'Bypass', + '-File', path.join(cliDir, 'scripts', 'build.ps1') + ], { stdio: 'inherit' }); } else { - execSync(`bash "${path.join(cliDir, 'scripts', 'build.sh')}"`, { - stdio: 'inherit', - }); + result = spawnSync('bash', [ + path.join(cliDir, 'scripts', 'build.sh') + ], { stdio: 'inherit' }); +} + +if (result.status !== 0) { + process.stderr.write(`Build failed with exit code ${result.status}\n`); + process.exit(result.status ?? 1); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/npx/scripts/build.js` around lines 27 - 36, The current build.js uses execSync with shell command strings (see the execSync calls inside the process.platform === 'win32' conditional), which triggers command-injection warnings; replace these with child_process.spawnSync calls passing the executable and an argument array (e.g., 'powershell.exe' with ['-ExecutionPolicy','Bypass','-File', path.join(cliDir,'scripts','build.ps1')] for Windows and 'bash' with [path.join(cliDir,'scripts','build.sh')] for non-Windows), preserve { stdio: 'inherit' } and error handling, and remove reliance on shell interpolation so linters no longer warn while keeping identical behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build.ps1`:
- Around line 551-562: Build-I18n-Extractor, Lint-CLI and Lint-I18n-Extractor
currently hardcode Windows extensions (i18n-extractor.exe and
golangci-lint.exe); change them to compute the binary name like Build-Backend
does by checking $GO_OS and appending ".exe" only when $GO_OS -eq "windows"
(e.g., set a local $exeSuffix or build $binaryName = "i18n-extractor" + ($GO_OS
-eq "windows" ? ".exe" : "") and use that in & go build and lint invocations),
update references in Build-I18n-Extractor, Lint-CLI, and Lint-I18n-Extractor so
cross-platform builds/linting succeed.
In `@tools/cli/cmd/thunderid/main.go`:
- Around line 45-66: The "try" and "integrate" branches currently only trigger
when len(args) >= 2 so calls like "thunderid try" or "thunderid integrate" fall
through to cli.Run; add explicit operand validation for args[0] == "try" and
args[0] == "integrate": if args length is 1 (missing operand) call ui.Fatal with
a concise usage/error message (e.g., "usage: thunderid try <usecase>" or "usage:
thunderid integrate <technology>") and os.Exit(1), and keep the existing
behavior when the operand is present (continue to parseFlags/sample.Run or the
not-implemented fatal message). Ensure these checks occur before falling through
to cli.Run so invalid subcommands do not start the default server flow (refer to
symbols args, parseFlags, sample.Run, ui.Fatal, cli.Run).
In `@tools/cli/internal/commands/sample/sample.go`:
- Around line 487-532: startSampleServices currently starts npm via exec.Command
and drops the *exec.Cmd, preventing shutdown control; change startSampleServices
(the function) to return the started *exec.Cmd (or otherwise expose it)
alongside error, so callers can track and stop the process: create and start cmd
as now but return cmd instead of nil on success, and ensure callers (where
sample.Run is invoked) store the returned cmd on the ReplModel (e.g.,
ReplModel.sampleCmd) and update ReplModel.killThunder() to also kill/Wait the
sampleCmd (calling Process.Kill/Signal and Wait) to reliably stop the sample
stack on Ctrl+C; keep the existing immediate-failure detection logic (done
channel and tailLog) but do not discard the cmd.
- Around line 395-405: The filename derived from sample YAML (when
reID.FindStringSubmatch(doc) sets filename = idM[1]+".yaml") is not sanitized
and may contain path separators or "..", allowing directory traversal; fix by
validating and sanitizing idM[1] before using it: extract a safe base (e.g. use
filepath.Base or clean and compare to detect traversal), reject or replace any
path separators or ".." segments, and fall back to the existing
fmt.Sprintf("%s_%04d.yaml", resourceType, i) if the id is invalid; ensure the
sanitized filename is then used in the existing target/ filepath.Join(...) and
os.WriteFile calls so files cannot escape repository/resources/<type>.
- Around line 220-225: The WaitForPortFree calls after killing the product and
consent server are currently ignored; update the stop sequence around
progress("Stopping "+product.Name+"...") to handle errors returned by
setup.WaitForPortFree(health.DefaultPort, 15*time.Second) and
setup.WaitForPortFree(consentServerPort, 15*time.Second) (and any errors from
setup.KillPort if applicable), and abort/return a non‑nil error (or call
os.Exit(1)) when either call times out or fails so the workflow does not
continue against stale listeners; reference the functions setup.KillPort,
setup.WaitForPortFree, and the variables health.DefaultPort and
consentServerPort to locate and fix the logic.
- Around line 254-269: The install and seed steps use exec.Command("npm", ...)
which fails on Windows; change them to use a platform-aware npm executable
(e.g., determine npmExe := "npm" or "npm.cmd" when runtime.GOOS == "windows")
and pass that npmExe as the command name for both installCmd and seedCmd so the
calls in the block that creates installCmd and seedCmd use the Windows-safe
executable; ensure you reuse the same logic that startSampleServices uses (or
centralize into a helper like npmExecutable) and update both places referencing
installCmd and seedCmd to use that executable.
In `@tools/cli/internal/services/health/health.go`:
- Around line 37-52: ResolveBaseURL overruns its deadline because it always
sleeps a fixed 500ms between probes; cap that sleep to the remaining time so the
function respects the overall timeout. Inside ResolveBaseURL (the loop that
iterates schemes and calls checkReadyIn), recompute remaining :=
time.Until(deadline) before sleeping and replace time.Sleep(500 *
time.Millisecond) with sleeping for the minimum of 500ms and remaining (and if
remaining <= 0 return "", false immediately). This ensures probeTimeout logic
with checkReadyIn remains unchanged but prevents a final fixed 500ms sleep from
exceeding the deadline.
---
Nitpick comments:
In `@tools/npx/scripts/build.js`:
- Around line 27-36: The current build.js uses execSync with shell command
strings (see the execSync calls inside the process.platform === 'win32'
conditional), which triggers command-injection warnings; replace these with
child_process.spawnSync calls passing the executable and an argument array
(e.g., 'powershell.exe' with ['-ExecutionPolicy','Bypass','-File',
path.join(cliDir,'scripts','build.ps1')] for Windows and 'bash' with
[path.join(cliDir,'scripts','build.sh')] for non-Windows), preserve { stdio:
'inherit' } and error handling, and remove reliance on shell interpolation so
linters no longer warn while keeping identical behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e36b7c2b-5cc9-41dd-a73b-335802c287b6
⛔ Files ignored due to path filters (3)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltools/cli/go.sumis excluded by!**/*.sumtools/npx-thunderid/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (65)
.github/actions/release-tool/npm/action.yml.github/workflows/release-tools.ymlMakefilebuild.ps1build.shsamples/apps/wayfinder-sample/frontend/.env.examplesamples/apps/wayfinder-sample/frontend/src/App.jsxsamples/apps/wayfinder-sample/frontend/src/main.jsxsamples/apps/wayfinder-sample/package.jsontools/cli/.golangci.ymltools/cli/cmd/thunderid/main.gotools/cli/go.modtools/cli/internal/cli/root.gotools/cli/internal/commands/sample/sample.gotools/cli/internal/commands/upgrade/upgrade.gotools/cli/internal/product/product.gotools/cli/internal/product/product_test.gotools/cli/internal/services/config/config.gotools/cli/internal/services/config/config_test.gotools/cli/internal/services/health/health.gotools/cli/internal/services/health/health_test.gotools/cli/internal/services/release/release.gotools/cli/internal/services/release/release_test.gotools/cli/internal/services/setup/setup.gotools/cli/internal/services/setup/setup_test.gotools/cli/internal/ui/banner.gotools/cli/internal/ui/onboarding.gotools/cli/internal/ui/repl.gotools/cli/internal/ui/spinner/spinner.gotools/cli/internal/ui/spinner/spinner_test.gotools/cli/internal/ui/usecases.gotools/cli/internal/utils/browser.gotools/cli/scripts/build.ps1tools/cli/scripts/build.shtools/npx-thunderid/README.mdtools/npx-thunderid/bin/thunderid.jstools/npx-thunderid/eslint.config.jstools/npx-thunderid/package.jsontools/npx-thunderid/prettier.config.jstools/npx-thunderid/rolldown.config.jstools/npx-thunderid/src/constants/ThunderRepo.tstools/npx-thunderid/src/deploy/index.tstools/npx-thunderid/src/deploy/recipes/fly.tstools/npx-thunderid/src/deploy/recipes/index.tstools/npx-thunderid/src/deploy/recipes/railway.tstools/npx-thunderid/src/deploy/recipes/render.tstools/npx-thunderid/src/download.tstools/npx-thunderid/src/index.tstools/npx-thunderid/src/models/db.tstools/npx-thunderid/src/models/deploy.tstools/npx-thunderid/src/setup.tstools/npx-thunderid/src/state.tstools/npx-thunderid/tsconfig.eslint.jsontools/npx-thunderid/tsconfig.jsontools/npx-thunderid/tsconfig.lib.jsontools/npx-thunderid/tsconfig.spec.jsontools/npx-thunderid/vitest.config.tstools/npx/.editorconfigtools/npx/.gitignoretools/npx/.prettierignoretools/npx/README.mdtools/npx/bin/thunderid.jstools/npx/package.jsontools/npx/scripts/build.jstools/npx/scripts/clean.js
💤 Files with no reviewable changes (23)
- tools/npx-thunderid/tsconfig.lib.json
- tools/npx-thunderid/README.md
- tools/npx-thunderid/tsconfig.json
- tools/npx-thunderid/src/constants/ThunderRepo.ts
- tools/npx-thunderid/prettier.config.js
- tools/npx-thunderid/src/deploy/index.ts
- tools/npx-thunderid/src/deploy/recipes/railway.ts
- tools/npx-thunderid/tsconfig.spec.json
- tools/npx-thunderid/tsconfig.eslint.json
- tools/npx-thunderid/src/setup.ts
- tools/npx-thunderid/src/download.ts
- tools/npx-thunderid/rolldown.config.js
- tools/npx-thunderid/src/models/db.ts
- tools/npx-thunderid/src/models/deploy.ts
- tools/npx-thunderid/src/deploy/recipes/index.ts
- tools/npx-thunderid/bin/thunderid.js
- tools/npx-thunderid/src/deploy/recipes/fly.ts
- tools/npx-thunderid/vitest.config.ts
- tools/npx-thunderid/eslint.config.js
- tools/npx-thunderid/src/deploy/recipes/render.ts
- tools/npx-thunderid/src/index.ts
- tools/npx-thunderid/package.json
- tools/npx-thunderid/src/state.ts
✅ Files skipped from review due to trivial changes (2)
- samples/apps/wayfinder-sample/frontend/.env.example
- tools/npx/package.json
🚧 Files skipped from review as they are similar to previous changes (28)
- tools/cli/internal/ui/spinner/spinner_test.go
- tools/cli/internal/product/product.go
- tools/cli/internal/ui/spinner/spinner.go
- samples/apps/wayfinder-sample/frontend/src/main.jsx
- .github/actions/release-tool/npm/action.yml
- samples/apps/wayfinder-sample/package.json
- .github/workflows/release-tools.yml
- tools/cli/internal/services/release/release_test.go
- tools/cli/scripts/build.ps1
- samples/apps/wayfinder-sample/frontend/src/App.jsx
- Makefile
- tools/cli/.golangci.yml
- tools/cli/internal/ui/onboarding.go
- tools/npx/scripts/clean.js
- build.sh
- tools/cli/go.mod
- tools/cli/internal/services/config/config_test.go
- tools/cli/internal/utils/browser.go
- tools/cli/internal/services/setup/setup_test.go
- tools/npx/bin/thunderid.js
- tools/cli/internal/ui/banner.go
- tools/cli/internal/commands/upgrade/upgrade.go
- tools/cli/scripts/build.sh
- tools/cli/internal/product/product_test.go
- tools/cli/internal/services/config/config.go
- tools/cli/internal/cli/root.go
- tools/cli/internal/services/release/release.go
- tools/cli/internal/services/setup/setup.go
Comment on lines
+551
to
+562
| function Build-I18n-Extractor { | ||
| $toolBin = Join-Path $PSScriptRoot "backend/bin/tools" | ||
| New-Item -ItemType Directory -Force -Path $toolBin | Out-Null | ||
| Write-Host "Building i18n-extractor..." | ||
| Push-Location "$PSScriptRoot/tools/i18n-extractor" | ||
| try { | ||
| & go build -o "$toolBin/i18n-extractor.exe" . | ||
| if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } | ||
| } finally { | ||
| Pop-Location | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Platform-conditional binary extensions needed in build.ps1.
Build-I18n-Extractor (L551-562) hardcodes i18n-extractor.exe, and both Lint-CLI (L575-585) and Lint-I18n-Extractor (L587-597) hardcode golangci-lint.exe regardless of target OS. Since PowerShell 7 is cross-platform and the script detects $GO_OS, these functions should conditionally append .exe only when $GO_OS -eq "windows", matching the pattern used in Build-Backend (lines 372-374). Without this, builds and linting will fail on Linux and macOS.
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] Missing BOM encoding for non-ASCII encoded file 'build.ps1'
(PSUseBOMForUnicodeEncodedFile)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@build.ps1` around lines 551 - 562, Build-I18n-Extractor, Lint-CLI and
Lint-I18n-Extractor currently hardcode Windows extensions (i18n-extractor.exe
and golangci-lint.exe); change them to compute the binary name like
Build-Backend does by checking $GO_OS and appending ".exe" only when $GO_OS -eq
"windows" (e.g., set a local $exeSuffix or build $binaryName = "i18n-extractor"
+ ($GO_OS -eq "windows" ? ".exe" : "") and use that in & go build and lint
invocations), update references in Build-I18n-Extractor, Lint-CLI, and
Lint-I18n-Extractor so cross-platform builds/linting succeed.
Comment on lines
+45
to
+66
| // try <usecase> — download and launch a use-case sample app. | ||
| if len(args) >= 2 && args[0] == "try" { | ||
| usecase := args[1] | ||
| verbose, _ := parseFlags(args[2:]) | ||
| activeVersion := config.ReadActiveVersion() | ||
| if activeVersion == "" { | ||
| ui.Fatal(fmt.Sprintf("No active %s install found. Run `npx %s` first.", product.Name, product.Slug)) | ||
| os.Exit(1) | ||
| } | ||
| path := cli.VersionedInstallPath(activeVersion) | ||
| if err := sample.Run(usecase, path, verbose, sample.Options{}); err != nil { | ||
| ui.Fatal(err.Error()) | ||
| os.Exit(1) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| // integrate <technology> — configure a technology integration (future). | ||
| if len(args) >= 2 && args[0] == "integrate" { | ||
| ui.Fatal(fmt.Sprintf("`integrate %s` is not yet implemented.", args[1])) | ||
| os.Exit(1) | ||
| } |
Contributor
There was a problem hiding this comment.
Reject incomplete subcommands instead of falling through to startup.
thunderid try and thunderid integrate currently bypass these branches when the required operand is missing, then drop into cli.Run(...). That starts the default server flow for an invalid command instead of surfacing usage or an error.
Suggested fix
- // try <usecase> — download and launch a use-case sample app.
- if len(args) >= 2 && args[0] == "try" {
+ // try <usecase> — download and launch a use-case sample app.
+ if len(args) > 0 && args[0] == "try" {
+ if len(args) < 2 {
+ ui.Fatal("Missing use case. Usage: thunderid try <usecase>")
+ os.Exit(1)
+ }
usecase := args[1]
verbose, _ := parseFlags(args[2:])
activeVersion := config.ReadActiveVersion()
if activeVersion == "" {
ui.Fatal(fmt.Sprintf("No active %s install found. Run `npx %s` first.", product.Name, product.Slug))
@@
- // integrate <technology> — configure a technology integration (future).
- if len(args) >= 2 && args[0] == "integrate" {
+ // integrate <technology> — configure a technology integration (future).
+ if len(args) > 0 && args[0] == "integrate" {
+ if len(args) < 2 {
+ ui.Fatal("Missing technology. Usage: thunderid integrate <tech>")
+ os.Exit(1)
+ }
ui.Fatal(fmt.Sprintf("`integrate %s` is not yet implemented.", args[1]))
os.Exit(1)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/cli/cmd/thunderid/main.go` around lines 45 - 66, The "try" and
"integrate" branches currently only trigger when len(args) >= 2 so calls like
"thunderid try" or "thunderid integrate" fall through to cli.Run; add explicit
operand validation for args[0] == "try" and args[0] == "integrate": if args
length is 1 (missing operand) call ui.Fatal with a concise usage/error message
(e.g., "usage: thunderid try <usecase>" or "usage: thunderid integrate
<technology>") and os.Exit(1), and keep the existing behavior when the operand
is present (continue to parseFlags/sample.Run or the not-implemented fatal
message). Ensure these checks occur before falling through to cli.Run so invalid
subcommands do not start the default server flow (refer to symbols args,
parseFlags, sample.Run, ui.Fatal, cli.Run).
Comment on lines
+220
to
+225
| // Stop the product and the consent server (port 9090). | ||
| progress("Stopping " + product.Name + "...") | ||
| setup.KillPort(health.DefaultPort) | ||
| setup.KillPort(consentServerPort) | ||
| setup.WaitForPortFree(health.DefaultPort, 15*time.Second) | ||
| setup.WaitForPortFree(consentServerPort, 15*time.Second) |
Contributor
There was a problem hiding this comment.
Abort if the old listeners never go down.
Both WaitForPortFree calls are ignored. If either port stays occupied, the rest of the workflow can restart against a stale process or fail later with a much less actionable error.
Suggested fix
progress("Stopping " + product.Name + "...")
setup.KillPort(health.DefaultPort)
setup.KillPort(consentServerPort)
- setup.WaitForPortFree(health.DefaultPort, 15*time.Second)
- setup.WaitForPortFree(consentServerPort, 15*time.Second)
+ if !setup.WaitForPortFree(health.DefaultPort, 15*time.Second) {
+ return nil, "", "", fmt.Errorf("port %d did not stop in time", health.DefaultPort)
+ }
+ if !setup.WaitForPortFree(consentServerPort, 15*time.Second) {
+ return nil, "", "", fmt.Errorf("port %d did not stop in time", consentServerPort)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Stop the product and the consent server (port 9090). | |
| progress("Stopping " + product.Name + "...") | |
| setup.KillPort(health.DefaultPort) | |
| setup.KillPort(consentServerPort) | |
| setup.WaitForPortFree(health.DefaultPort, 15*time.Second) | |
| setup.WaitForPortFree(consentServerPort, 15*time.Second) | |
| // Stop the product and the consent server (port 9090). | |
| progress("Stopping " + product.Name + "...") | |
| setup.KillPort(health.DefaultPort) | |
| setup.KillPort(consentServerPort) | |
| if !setup.WaitForPortFree(health.DefaultPort, 15*time.Second) { | |
| return nil, "", "", fmt.Errorf("port %d did not stop in time", health.DefaultPort) | |
| } | |
| if !setup.WaitForPortFree(consentServerPort, 15*time.Second) { | |
| return nil, "", "", fmt.Errorf("port %d did not stop in time", consentServerPort) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/cli/internal/commands/sample/sample.go` around lines 220 - 225, The
WaitForPortFree calls after killing the product and consent server are currently
ignored; update the stop sequence around progress("Stopping
"+product.Name+"...") to handle errors returned by
setup.WaitForPortFree(health.DefaultPort, 15*time.Second) and
setup.WaitForPortFree(consentServerPort, 15*time.Second) (and any errors from
setup.KillPort if applicable), and abort/return a non‑nil error (or call
os.Exit(1)) when either call times out or fails so the workflow does not
continue against stale listeners; reference the functions setup.KillPort,
setup.WaitForPortFree, and the variables health.DefaultPort and
consentServerPort to locate and fix the logic.
Comment on lines
+254
to
+269
| if _, err := os.Stat(filepath.Join(sampleDir, "node_modules")); os.IsNotExist(err) { | ||
| progress("Installing dependencies...") | ||
| installCmd := exec.Command("npm", "install", "--silent") | ||
| installCmd.Dir = sampleDir | ||
| if out, installErr := installCmd.CombinedOutput(); installErr != nil { | ||
| return proc, meta.sampleURL, serverURL, | ||
| fmt.Errorf("npm install failed: %w\n%s", installErr, out) | ||
| } | ||
| } | ||
|
|
||
| // Seed database on first run. | ||
| if _, err := os.Stat(filepath.Join(sampleDir, "backend", "wayfinder.sqlite")); os.IsNotExist(err) { | ||
| progress("Seeding database...") | ||
| seedCmd := exec.Command("npm", "run", "seed") | ||
| seedCmd.Dir = filepath.Join(sampleDir, "backend") | ||
| if out, seedErr := seedCmd.CombinedOutput(); seedErr != nil { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section around the reported lines
sed -n '220,310p' tools/cli/internal/commands/sample/sample.go | nl -ba
# Find all uses of "npm" / "npm.cmd" in the file (excluding comments)
rg -n --hidden --no-ignore -S '"npm(\.cmd)?"' tools/cli/internal/commands/sample/sample.go
# Locate startSampleServices and show it (or nearby) to confirm existing Windows switching
rg -n "startSampleServices" tools/cli/internal/commands/sample/sample.go
start_line=$(rg -n "startSampleServices" tools/cli/internal/commands/sample/sample.go | head -n1 | cut -d: -f1)
sed -n "$((start_line-80)),$((start_line+120))p" tools/cli/internal/commands/sample/sample.go | nl -baRepository: thunder-id/thunderid
Length of output: 107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Relevant section around the reported lines
sed -n '220,310p' tools/cli/internal/commands/sample/sample.go
# Find all uses of "npm" / "npm.cmd" in the file
rg -n --hidden --no-ignore -S '"npm(\.cmd)?"' tools/cli/internal/commands/sample/sample.go
# Locate startSampleServices and show nearby code for windows switching
rg -n "startSampleServices" tools/cli/internal/commands/sample/sample.go
start_line=$(rg -n "startSampleServices" tools/cli/internal/commands/sample/sample.go | head -n1 | cut -d: -f1)
sed -n "$((start_line-80)),$((start_line+120))p" tools/cli/internal/commands/sample/sample.goRepository: thunder-id/thunderid
Length of output: 11049
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tools/cli/internal/commands/sample/sample.go"
# Inspect around the npmExe logic (around where npmExe is defined/used)
sed -n '470,540p' "$FILE" | nl -ba || sed -n '470,540p' "$FILE"
# Re-run without nl if nl missing
sed -n '470,540p' "$FILE"
# List exact occurrences of npm-related commands
rg -n --hidden --no-ignore '"npm(\.cmd)?"' "$FILE"
# Scan for hardcoded string literals "Thunder" / "ThunderID"
rg -n --hidden --no-ignore '"Thunder(ID)?"' "$FILE"
# Also separately show "Thunder" and "ThunderID" matches with context
rg -n --hidden --no-ignore --context 2 '"Thunder"' "$FILE" || true
rg -n --hidden --no-ignore --context 2 '"ThunderID"' "$FILE" || trueRepository: thunder-id/thunderid
Length of output: 4538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tools/cli/internal/commands/sample/sample.go"
# Inspect around the npmExe logic
sed -n '470,540p' "$FILE"
# List exact occurrences of npm-related commands
rg -n --hidden --no-ignore '"npm(\.cmd)?"' "$FILE"
# Scan for hardcoded string literals "Thunder" / "ThunderID"
rg -n --hidden --no-ignore --context 2 '"Thunder"' "$FILE" || true
rg -n --hidden --no-ignore --context 2 '"ThunderID"' "$FILE" || trueRepository: thunder-id/thunderid
Length of output: 2352
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tools/cli/internal/commands/sample/sample.go"
# Inspect around the npmExe logic
sed -n '470,540p' "$FILE"
# List exact occurrences of npm-related commands
rg -n --hidden --no-ignore '"npm(\.cmd)?"' "$FILE"
# Scan for hardcoded string literals "Thunder" / "ThunderID"
rg -n --hidden --no-ignore --context 2 '"Thunder"' "$FILE" || true
rg -n --hidden --no-ignore --context 2 '"ThunderID"' "$FILE" || trueRepository: thunder-id/thunderid
Length of output: 2352
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tools/cli/internal/commands/sample/sample.go"
# Find exact string literals (no comments filtering; we'll inspect matches in output)
rg -n --hidden --no-ignore '"Thunder(ID)?"' "$FILE" || true
# Also show "ThunderID" and "Thunder" separately with a couple context lines
rg -n --hidden --no-ignore --context 2 '"Thunder"' "$FILE" || true
rg -n --hidden --no-ignore --context 2 '"ThunderID"' "$FILE" || trueRepository: thunder-id/thunderid
Length of output: 46
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tools/cli/internal/commands/sample/sample.go"
# Show import block
rg -n "import \\(" "$FILE"
start=$(rg -n "import \\(" "$FILE" | head -n1 | cut -d: -f1)
# print a small window after import start
sed -n "$((start)),$((start+60))p" "$FILE"
# Confirm runtime is imported/used
rg -n "\bruntime\." "$FILE" | head -n20Repository: thunder-id/thunderid
Length of output: 2076
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tools/cli/internal/commands/sample/sample.go"
# Show a bit earlier to find the exact start of startSampleServices and surrounding package-scope funcs
sed -n '430,520p' "$FILE"Repository: thunder-id/thunderid
Length of output: 3020
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tools/cli/internal/commands/sample/sample.go"
rg -n --hidden --no-ignore "\bnpmExecutable\b|\bnpmExe\b|npm\.cmd" "$FILE"Repository: thunder-id/thunderid
Length of output: 158
Use Windows-safe npm.cmd for first-run install + seed
startSampleServices already switches to npm.cmd on Windows for npm run ..., but the initial dependency install and database seed still call exec.Command("npm", ...) and will fail on Windows before the service startup path runs.
Suggested fix
+func npmExecutable() string {
+ if runtime.GOOS == "windows" {
+ return "npm.cmd"
+ }
+ return "npm"
+}
+
// Install workspace dependencies if not already present.
if _, err := os.Stat(filepath.Join(sampleDir, "node_modules")); os.IsNotExist(err) {
progress("Installing dependencies...")
- installCmd := exec.Command("npm", "install", "--silent")
+ installCmd := exec.Command(npmExecutable(), "install", "--silent")
installCmd.Dir = sampleDir
if out, installErr := installCmd.CombinedOutput(); installErr != nil {
return proc, meta.sampleURL, serverURL,
fmt.Errorf("npm install failed: %w\n%s", installErr, out)
}
@@
if _, err := os.Stat(filepath.Join(sampleDir, "backend", "wayfinder.sqlite")); os.IsNotExist(err) {
progress("Seeding database...")
- seedCmd := exec.Command("npm", "run", "seed")
+ seedCmd := exec.Command(npmExecutable(), "run", "seed")
seedCmd.Dir = filepath.Join(sampleDir, "backend")
if out, seedErr := seedCmd.CombinedOutput(); seedErr != nil {
return proc, meta.sampleURL, serverURL,
fmt.Errorf("seed failed: %w\n%s", seedErr, out)
}
@@
- npmExe := "npm"
- if runtime.GOOS == "windows" {
- npmExe = "npm.cmd"
- }
- logPath := filepath.Join(logsDir, "sample.log")
- cmd := exec.Command(npmExe, "run", script)
+ logPath := filepath.Join(logsDir, "sample.log")
+ cmd := exec.Command(npmExecutable(), "run", script)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/cli/internal/commands/sample/sample.go` around lines 254 - 269, The
install and seed steps use exec.Command("npm", ...) which fails on Windows;
change them to use a platform-aware npm executable (e.g., determine npmExe :=
"npm" or "npm.cmd" when runtime.GOOS == "windows") and pass that npmExe as the
command name for both installCmd and seedCmd so the calls in the block that
creates installCmd and seedCmd use the Windows-safe executable; ensure you reuse
the same logic that startSampleServices uses (or centralize into a helper like
npmExecutable) and update both places referencing installCmd and seedCmd to use
that executable.
Comment on lines
+395
to
+405
| if idM := reID.FindStringSubmatch(doc); idM != nil { | ||
| filename = idM[1] + ".yaml" | ||
| } else { | ||
| filename = fmt.Sprintf("%s_%04d.yaml", resourceType, i) | ||
| } | ||
|
|
||
| target := filepath.Join(thunderRoot, "repository", "resources", dir) | ||
| if err := os.MkdirAll(target, 0o755); err != nil { | ||
| return err | ||
| } | ||
| if err := os.WriteFile(filepath.Join(target, filename), []byte(doc+"\n"), 0o644); err != nil { |
Contributor
There was a problem hiding this comment.
Sanitize resource filenames derived from sample YAML.
id / handle comes from downloaded sample content and is joined directly into the destination path. A value containing path separators or .. can escape repository/resources/<type> and overwrite arbitrary files under the install tree.
Suggested fix
var filename string
if idM := reID.FindStringSubmatch(doc); idM != nil {
- filename = idM[1] + ".yaml"
+ id := idM[1]
+ if id != filepath.Base(id) || strings.ContainsRune(id, filepath.Separator) {
+ return fmt.Errorf("invalid resource id %q", id)
+ }
+ filename = id + ".yaml"
} else {
filename = fmt.Sprintf("%s_%04d.yaml", resourceType, i)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if idM := reID.FindStringSubmatch(doc); idM != nil { | |
| filename = idM[1] + ".yaml" | |
| } else { | |
| filename = fmt.Sprintf("%s_%04d.yaml", resourceType, i) | |
| } | |
| target := filepath.Join(thunderRoot, "repository", "resources", dir) | |
| if err := os.MkdirAll(target, 0o755); err != nil { | |
| return err | |
| } | |
| if err := os.WriteFile(filepath.Join(target, filename), []byte(doc+"\n"), 0o644); err != nil { | |
| if idM := reID.FindStringSubmatch(doc); idM != nil { | |
| id := idM[1] | |
| if id != filepath.Base(id) || strings.ContainsRune(id, filepath.Separator) { | |
| return fmt.Errorf("invalid resource id %q", id) | |
| } | |
| filename = id + ".yaml" | |
| } else { | |
| filename = fmt.Sprintf("%s_%04d.yaml", resourceType, i) | |
| } | |
| target := filepath.Join(thunderRoot, "repository", "resources", dir) | |
| if err := os.MkdirAll(target, 0o755); err != nil { | |
| return err | |
| } | |
| if err := os.WriteFile(filepath.Join(target, filename), []byte(doc+"\n"), 0o644); err != nil { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/cli/internal/commands/sample/sample.go` around lines 395 - 405, The
filename derived from sample YAML (when reID.FindStringSubmatch(doc) sets
filename = idM[1]+".yaml") is not sanitized and may contain path separators or
"..", allowing directory traversal; fix by validating and sanitizing idM[1]
before using it: extract a safe base (e.g. use filepath.Base or clean and
compare to detect traversal), reject or replace any path separators or ".."
segments, and fall back to the existing fmt.Sprintf("%s_%04d.yaml",
resourceType, i) if the id is invalid; ensure the sanitized filename is then
used in the existing target/ filepath.Join(...) and os.WriteFile calls so files
cannot escape repository/resources/<type>.
Comment on lines
+487
to
+532
| // startSampleServices launches the sample services in the background via npm. | ||
| // For AgentID mode (aiEnabled=true) it runs `npm run dev` (all three services); | ||
| // otherwise it runs `npm run dev:b2c` (backend + frontend only). | ||
| func startSampleServices(sampleDir string, aiEnabled bool) error { | ||
| logsDir := filepath.Join(sampleDir, "logs") | ||
| if err := os.MkdirAll(logsDir, 0o755); err != nil { | ||
| return err | ||
| } | ||
| logFile, err := os.OpenFile(filepath.Join(logsDir, "sample.log"), | ||
| os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) | ||
| if err != nil { | ||
| logFile, _ = os.OpenFile(os.DevNull, os.O_WRONLY, 0) | ||
| } | ||
|
|
||
| script := "dev:b2c" | ||
| if aiEnabled { | ||
| script = "dev" | ||
| } | ||
|
|
||
| npmExe := "npm" | ||
| if runtime.GOOS == "windows" { | ||
| npmExe = "npm.cmd" | ||
| } | ||
| logPath := filepath.Join(logsDir, "sample.log") | ||
| cmd := exec.Command(npmExe, "run", script) | ||
| cmd.Dir = sampleDir | ||
| cmd.Stdout = logFile | ||
| cmd.Stderr = logFile // never write to os.Stderr — it corrupts the Bubble Tea display | ||
| cmd.Stdin = nil | ||
| if err := cmd.Start(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Detect immediate failures (e.g. missing npm script) before returning. | ||
| done := make(chan error, 1) | ||
| go func() { done <- cmd.Wait() }() | ||
| select { | ||
| case err := <-done: | ||
| tail := tailLog(logPath, 10) | ||
| if err != nil { | ||
| return fmt.Errorf("sample services failed to start:\n%s", tail) | ||
| } | ||
| case <-time.After(2 * time.Second): | ||
| // Still running — startup succeeded. | ||
| } | ||
| return nil |
Contributor
There was a problem hiding this comment.
Track the sample-service process instead of starting it fire-and-forget.
startSampleServices launches npm run ..., waits two seconds, and then discards the *exec.Cmd. That leaves the CLI with no way to stop the sample stack it just started: main.go returns immediately after sample.Run(...), and ReplModel.killThunder() only stops the Thunder process. The current "Press Ctrl+C to stop." contract is therefore false for the sample services.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/cli/internal/commands/sample/sample.go` around lines 487 - 532,
startSampleServices currently starts npm via exec.Command and drops the
*exec.Cmd, preventing shutdown control; change startSampleServices (the
function) to return the started *exec.Cmd (or otherwise expose it) alongside
error, so callers can track and stop the process: create and start cmd as now
but return cmd instead of nil on success, and ensure callers (where sample.Run
is invoked) store the returned cmd on the ReplModel (e.g., ReplModel.sampleCmd)
and update ReplModel.killThunder() to also kill/Wait the sampleCmd (calling
Process.Kill/Signal and Wait) to reliably stop the sample stack on Ctrl+C; keep
the existing immediate-failure detection logic (done channel and tailLog) but do
not discard the cmd.
Comment on lines
+37
to
+52
| for time.Now().Before(deadline) { | ||
| for _, scheme := range []string{"https", "http"} { | ||
| remaining := time.Until(deadline) | ||
| if remaining <= 0 { | ||
| return "", false | ||
| } | ||
| probeTimeout := remaining | ||
| if probeTimeout > 2*time.Second { | ||
| probeTimeout = 2 * time.Second | ||
| } | ||
| base := fmt.Sprintf("%s://localhost:%d", scheme, port) | ||
| if checkReadyIn(base, probeTimeout) { | ||
| return base, true | ||
| } | ||
| } | ||
| time.Sleep(500 * time.Millisecond) |
Contributor
There was a problem hiding this comment.
Cap the polling sleep to the remaining deadline.
ResolveBaseURL still overruns timeout on short budgets because it always sleeps 500ms after the last failed probe before re-checking deadline. Callers like sample.runWithResult therefore do not get the timeout guarantee this helper documents.
Suggested fix
for time.Now().Before(deadline) {
for _, scheme := range []string{"https", "http"} {
remaining := time.Until(deadline)
if remaining <= 0 {
return "", false
}
probeTimeout := remaining
if probeTimeout > 2*time.Second {
probeTimeout = 2 * time.Second
}
base := fmt.Sprintf("%s://localhost:%d", scheme, port)
if checkReadyIn(base, probeTimeout) {
return base, true
}
}
- time.Sleep(500 * time.Millisecond)
+ sleepFor := 500 * time.Millisecond
+ if remaining := time.Until(deadline); remaining <= 0 {
+ return "", false
+ } else if remaining < sleepFor {
+ sleepFor = remaining
+ }
+ time.Sleep(sleepFor)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for time.Now().Before(deadline) { | |
| for _, scheme := range []string{"https", "http"} { | |
| remaining := time.Until(deadline) | |
| if remaining <= 0 { | |
| return "", false | |
| } | |
| probeTimeout := remaining | |
| if probeTimeout > 2*time.Second { | |
| probeTimeout = 2 * time.Second | |
| } | |
| base := fmt.Sprintf("%s://localhost:%d", scheme, port) | |
| if checkReadyIn(base, probeTimeout) { | |
| return base, true | |
| } | |
| } | |
| time.Sleep(500 * time.Millisecond) | |
| for time.Now().Before(deadline) { | |
| for _, scheme := range []string{"https", "http"} { | |
| remaining := time.Until(deadline) | |
| if remaining <= 0 { | |
| return "", false | |
| } | |
| probeTimeout := remaining | |
| if probeTimeout > 2*time.Second { | |
| probeTimeout = 2 * time.Second | |
| } | |
| base := fmt.Sprintf("%s://localhost:%d", scheme, port) | |
| if checkReadyIn(base, probeTimeout) { | |
| return base, true | |
| } | |
| } | |
| sleepFor := 500 * time.Millisecond | |
| if remaining := time.Until(deadline); remaining <= 0 { | |
| return "", false | |
| } else if remaining < sleepFor { | |
| sleepFor = remaining | |
| } | |
| time.Sleep(sleepFor) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/cli/internal/services/health/health.go` around lines 37 - 52,
ResolveBaseURL overruns its deadline because it always sleeps a fixed 500ms
between probes; cap that sleep to the remaining time so the function respects
the overall timeout. Inside ResolveBaseURL (the loop that iterates schemes and
calls checkReadyIn), recompute remaining := time.Until(deadline) before sleeping
and replace time.Sleep(500 * time.Millisecond) with sleeping for the minimum of
500ms and remaining (and if remaining <= 0 return "", false immediately). This
ensures probeTimeout logic with checkReadyIn remains unchanged but prevents a
final fixed 500ms sleep from exceeding the deadline.
20fee95 to
e60636f
Compare
Dependency Validation ResultsDependency name: github.com/charmbracelet/bubbles Dependency name: github.com/charmbracelet/bubbletea Dependency name: github.com/charmbracelet/huh Dependency name: github.com/charmbracelet/huh/spinner Dependency name: github.com/charmbracelet/lipgloss Dependency name: github.com/stretchr/testify |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
brionmario
force-pushed
the
feat-mobile-sdks
branch
from
e60636f to
24dcac5
Compare
Dependency Validation ResultsDependency name: github.com/charmbracelet/bubbles Dependency name: github.com/charmbracelet/bubbletea Dependency name: github.com/charmbracelet/huh Dependency name: github.com/charmbracelet/huh/spinner Dependency name: github.com/charmbracelet/lipgloss Dependency name: github.com/stretchr/testify |
JayaShakthi97
approved these changes
Jun 18, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
The current ThunderID CLI only setup the server and the experience ends after.
Approach
Improve the DX by setting up usecases to quickly try out to experience the product a bit better.
Related Issues
thunderidnpx CLI package zero dependencies #3312Related PRs
charmbraceletpackages used in ThunderID CLI wso2/engineering-governance#14Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Build & Infrastructure