Skip to content

feat(web): add environment management workflow#4050

Open
RSO wants to merge 22 commits into
mainfrom
feat/web-env-management
Open

feat(web): add environment management workflow#4050
RSO wants to merge 22 commits into
mainfrom
feat/web-env-management

Conversation

@RSO

@RSO RSO commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add one idempotent pnpm web:env set command that asks whether a variable is sensitive, then updates both Vercel projects across Development, Staging, and Production.
  • Mirror sensitive Production values to the shared 1Password vault while leaving Development exportable for local setup.
  • Update tracked dotenv defaults, highlight matching remote values without blocking intentional reuse, and run contributor setup smoke checks when relevant environment templates change.

Verification

  • Confirmed the sensitivity prompt rejects sensitive NEXT_PUBLIC_* values before accessing remote providers.
  • Confirmed local argument validation, pinned Vercel CLI invocation, and 1Password create/update template behavior.
  • Did not run the main command against live Vercel resources during implementation.

Visual Changes

CleanShot 2026-06-17 at 09 06 50@2x CleanShot 2026-06-17 at 09 06 32@2x

Reviewer Notes

  • The implementation is intentionally small and idempotent: partial failures are recovered by rerunning the same command rather than maintaining transaction or resume state.
  • Multiline values use --development-file, --staging-file, and --production-file; normal values use hidden terminal prompts.
  • Matching tracked and remote values produce a highlighted warning but remain allowed.

Comment thread scripts/web-env/index.ts Fixed
@RSO RSO marked this pull request as ready for review June 17, 2026 15:57
@kilo-code-bot

kilo-code-bot Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

Adds a well-structured pnpm web:env set workflow for managing environment variables across Vercel projects and 1Password, with trackable dotenv defaults and CI smoke checks — no new code issues beyond the pre-existing CodeQL flag about op error output.

Files Reviewed (10 files)
  • .github/workflows/ci.yml — Added env file paths to kilocode_backend change filter
  • .github/workflows/setup-smoke.yml — Added PR trigger for env template changes, gated notify on non-PR events
  • DEVELOPMENT.md — Added section c documenting the new env management workflow
  • package.json — Added web:env script and @types/node devDep
  • pnpm-lock.yaml — Lockfile updates for @types/node version pinning
  • scripts/lint-all.sh — Added scripts/web-env to lint scope
  • scripts/typecheck-all.sh — Added scripts/web-env tsconfig to typecheck pass
  • scripts/web-env/index.ts — Main CLI entry point: arg parsing, sensitivity prompt, value collection, plan/apply loop
  • scripts/web-env/shared.ts — Shared utilities: process execution, Vercel/1Password integration, env file management
  • scripts/web-env/tsconfig.json — Minimal tsconfig for the scripts directory

Reviewed by deepseek-v4-pro-20260423 · 299,689 tokens

Review guidance: REVIEW.md from base branch main

@RSO

RSO commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

@kilocode-bot please review the changes in this PR

@kilo-code-bot

kilo-code-bot Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Code Review: PR #4050feat(web): add environment management workflow

Scope: +621/−7 across 10 files. The substance is two new scripts: scripts/web-env/index.ts (CLI/orchestration) and scripts/web-env/shared.ts (Vercel + 1Password + dotenv helpers). The rest is CI wiring, docs, a package.json script, a tsconfig, and a @types/node catalog pin.

Overall this is well-structured, security-conscious code: array-based spawnSync (no shell injection), strict variable-name validation, redacted Vercel error output, hidden secret prompts with TTY enforcement, and an idempotent upsert design. The issues below are mostly correctness/robustness concerns, with a couple worth resolving before merge.


1. Correctness & potential bugs

🔴 High — VERCEL_PROJECT_ID is set to the project name, not the project ID

shared.ts:484-489:

VERCEL_ORG_ID: context.orgId,
VERCEL_PROJECT_ID: context.project,   // 'kilocode-app' | 'kilocode-global-app'

context.project comes from the PROJECTS literal (shared.ts:404), which holds project names. Vercel's VERCEL_PROJECT_ID env var is documented to be the project ID (prj_…). Since the commands run in an empty temp dir (--cwd tempDirectory) with no .vercel/project.json link, the CLI relies entirely on these env vars. If the CLI resolves the project strictly by ID, every env add will fail with a project-not-found error. The author explicitly notes they "did not run the main command against live Vercel resources," so this is unverified. Please verify against the pinned CLI (vercel@53.3.1) that a project name/slug is accepted in VERCEL_PROJECT_ID; if not, you need to resolve each name to its prj_… ID first (e.g. via vercel project ls --format=json or the API). This is the most likely break in the whole PR.

🟡 Medium — staging is not a standard Vercel environment

shared.ts:405 and setVariable (shared.ts:506-527) call vercel env add NAME staging. Vercel's built-in targets are production, preview, and development. staging only works if a custom environment named staging exists in both projects. This is a reasonable assumption for this org, but it's load-bearing and unverified — worth a one-line confirmation in the PR description or a clearer error path if the environment is missing.

🟡 Medium — whoami --format=json team field is version-dependent

resolveVercelContexts (shared.ts:496-503) parses whoami.team.id/.slug. Vercel only added team to whoami --format=json around April 2026 (vercel/vercel#16051). The script pins vercel@53.3.1. Confirm that pinned version actually emits team in JSON — if it predates that change, team is always undefined and the command dies at 'Sign in to the kilocode Vercel team…' even when correctly logged in. (The error message would also be misleading.)

🟢 Low — parseOptions mishandles empty inline value

index.ts:217-224: for --production-file= (empty after =), match[2] is '', so match[2] || nextArgument consumes the next arg as the file path, while if (!match[2]) skips the index increment. The result is silent arg misalignment rather than a clean usage(). Minor, but match[2] !== undefined would be more correct than the truthy check.

🟢 Low — assignmentValue vs. defaults comparison asymmetry

In rejectMatchingTrackedValues (index.ts:314-330), the tracked value is either a freshly-entered default (raw string) or assignmentValue() (which JSON-parses quoted values, index.ts:302-312). Meanwhile setEnvDefault always writes JSON.stringify(value). The matching logic compares raw values (remote) against possibly-unwrapped file values — generally fine, but the two code paths normalize quoting differently, which could let an equivalent-but-differently-quoted secret slip past the "matches remote" guard. Consider normalizing both sides identically.


2. Security

  • ✅ No shell interpolation anywhere; all process calls use array args + validated name. Good.
  • ✅ Vercel failures are redacted (shared.ts:463); secrets are read via hidden raw-mode prompt with TTY enforcement (shared.ts:703-708).
  • 🟡 op error output is not redacted (shared.ts:456-462). The comment justifies surfacing op stdout/stderr for sign-in UX, but op item get/create/edit --format=json emit the secret value on stdout on success — and on a partial failure the captured stdout could contain secret material that then lands in the thrown error message and CI logs. Given the rest of the code is careful to redact, consider redacting op stdout too (keep stderr, which carries the human-facing auth/permission messages) or at least not concatenating result.stdout into op errors.
  • 🟢 The 1Password audit note embeds os.userInfo().username + os.hostname() (shared.ts:548). This is intended audit metadata in a shared vault, not PII in a user-facing DB, so it's fine — but worth being aware it writes developer machine identifiers into the shared vault.
  • NEXT_PUBLIC_* sensitivity guard (index.ts:242-244) correctly prevents marking browser-exposed vars as sensitive.

3. Error handling & edge cases

  • ✅ Empty-value handling: file values (index.ts:263) and interactive values (index.ts:268-272) both reject empties and reprompt.
  • ✅ Post-write verification of 1Password persistence (shared.ts:607-609, 637-639) is a nice integrity check.
  • 🟡 Ordering / partial-failure window (index.ts:366-379): local dotenv files are written before any remote provider call. If Vercel/1Password then fails, the working tree is already mutated while remotes are not. The "rerun the same command" recovery story works, but a dev could commit a default for a var that never made it to Vercel. Consider doing remote writes first, or at least documenting this ordering explicitly.
  • 🟢 readSecret (shared.ts:703-739) handles Ctrl-C, CR/LF, and backspace, but processes input char-by-char without handling multi-byte UTF-8 boundaries or escape sequences (they'd be inserted literally). Acceptable for secrets, just noting.
  • 🟢 findRepoRoot (shared.ts:642-653) reads/parses every ancestor package.json; a malformed one throws an unhelpful error mid-walk. Minor.

4. Maintainability & best practices

  • ✅ Clear separation (index.ts orchestration vs. shared.ts providers), small focused functions, descriptive names, as const literals, and good use of isRecord/stringValue narrowing helpers instead of broad casts — matches the repo's as/! avoidance rules.
  • 🟢 setEnvDefault always emits NAME="value" via JSON.stringify (shared.ts:678), forcing quotes even for simple values. That's valid dotenv but may produce noisy diffs against existing unquoted entries in tracked files. Consider only quoting when the value needs it.
  • 🟢 The @types/node change downgrades from ~25.5.2 → 24.12.4 across the lockfile (snapshots for @slack/*). This is intentional (catalog pin to match Node 24), but it's a repo-wide type surface change riding in an env-tooling PR — worth calling out explicitly in the summary so reviewers know it's deliberate.
  • ✅ CI changes are sensible: the setup-smoke PR trigger suppresses the failure-notification step on PRs (setup-smoke.yml:160), and the ci.yml backend filter additions correctly include the new tracked env files.
  • 🟢 scripts/web-env/tsconfig.json is standalone (doesn't extend a base config). Fine for an isolated script, but it won't inherit shared strict/lib settings if those evolve.

Recommendation: Request Changes ⚠️

The design, security posture, and code quality are solid and clearly above average for a tooling script. However, two unverified, load-bearing Vercel assumptions could make the command fail on first real use:

  1. VERCEL_PROJECT_ID set to a project name rather than ID (shared.ts:485) — most likely an outright break.
  2. staging custom environment and whoami --format=json team support in vercel@53.3.1 — both need confirmation.

Since the author states the command was never run against live Vercel resources, please validate items 1–3 against the pinned CLI (a single end-to-end test against one variable would surface all of them), and address the op error-redaction gap (§2). The remaining points are low-severity polish. Once the Vercel integration is verified working, this is close to an approve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants