Skip to content

fix(scripts): detect globalThis-cast and severity-threshold env reads#8632

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
rsnetworkinginc:fix-env-reference-posthog-vars-8627
Jul 25, 2026
Merged

fix(scripts): detect globalThis-cast and severity-threshold env reads#8632
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
rsnetworkinginc:fix-env-reference-posthog-vars-8627

Conversation

@rsnetworkinginc

Copy link
Copy Markdown
Contributor

Summary

The selfhost-env-reference generator's AST visitor only recognized .env accesses whose base is a bare process/c identifier, plus helper calls carrying an env var name in one fixed argument slot. src/selfhost/posthog.ts (#8287) reads three real, operator-facing config vars through shapes outside that set, so all three were silently absent from the generated reference operators rely on:

  • POSTHOG_SERVER_NAME (src/selfhost/posthog.ts:179) — read via (globalThis as unknown as { process?: { env?: ... } }).process?.env?.POSTHOG_SERVER_NAME. The base of .env is a property access on a cast expression, not an identifier, so isEnvContainer never matched it. This is the migration regression: the Sentry-era equivalent (env.SENTRY_SERVER_NAME) WAS detected and listed.
  • POSTHOG_MIN_SEVERITY / POSTHOG_REPO_MIN_SEVERITY (src/selfhost/posthog.ts:81) — passed as string-literal arguments 2 and 3 to resolveSeverityThreshold(...), which reads env[globalVarName]/env[repoMapVarName] internally via computed access that is AST-invisible at the read site. The helper was not in ENV_NAME_LITERAL_ARG_HELPERS (pre-existing gap, per the issue).

Changes

scripts/gen-selfhost-env-reference.ts

  • isEnvContainer now delegates its .env-base check to a new isProcessExpression, which accepts (a) the existing bare process/c identifiers and (b) a .process property access whose base resolves to the globalThis identifier after unwrapping parentheses and as casts (via the existing unwrapEnvExpression, so chained as unknown as {...} casts work the same way they already did for env itself, docs(selfhost): fill config/env doc gaps and reconcile checkRunDetailLevel default mismatch #2907). No var name is hardcoded — any future var read through the same globalThis-cast shape is caught, matching how selfhost-env-reference generator misses AI review-pipeline env vars #6993 generalized rather than special-cased.
  • ENV_NAME_LITERAL_ARG_HELPERS entries now carry a list of argument indexes instead of a single index, and resolveSeverityThreshold is registered at indexes [2, 3]; the visitor adds every string-literal name argument present at a registered index.

test/unit/selfhost-env-reference-script.test.ts — four new fixture tests (new describe block for this issue): a fixture using the exact (globalThis as unknown as {...}).process?.env?.FOO shape from posthog.ts:179 asserting the var is returned through collectSelfHostEnvVars; a deliberate non-match with the same cast silhouette but a non-globalThis base; a fixture using the exact resolveSeverityThreshold(env as unknown as Env, repo, "VAR_A", "VAR_B") call asserting both names are returned; and a deliberate non-match calling a same-arity helper outside the allowlist. None stubbed.

apps/loopover-ui/src/lib/selfhost-env-reference.ts — regenerated via npm run selfhost:env-reference and committed in the same PR. The diff is additions only: POSTHOG_MIN_SEVERITY, POSTHOG_REPO_MIN_SEVERITY, and POSTHOG_SERVER_NAME, each attributed to src/selfhost/posthog.ts.

Note: two additional rows the generalized fix surfaces

The regenerated reference also gains PAGERDUTY_MIN_SEVERITY and PAGERDUTY_REPO_MIN_SEVERITY, attributed to src/services/notify-pagerduty.ts — that file is already in DEFAULT_SOURCE_ROOTS and calls the same resolveSeverityThreshold(env, repo, "PAGERDUTY_MIN_SEVERITY", "PAGERDUTY_REPO_MIN_SEVERITY", DEFAULT_MIN_SEVERITY) shape at line 103 (#5119). This is the identical pre-existing blind-spot instance the issue documents for the deleted Sentry-era vars: both are real, operator-facing severity knobs that were silently missing from the reference for the same reason. Excluding them would require exactly the file-scoped special-casing the issue forbids ("the fix must generalize the AST visitor's pattern recognition so any future var read the same way is also caught"), so they are included rather than gerrymandered out. Beyond these strictly-additive rows, nothing else in the generated output changes.

Verification

  • npx vitest run test/unit/selfhost-env-reference-script.test.ts — 10 tests pass (6 existing + 4 new).
  • npm run selfhost:env-reference re-run; output committed.
  • npm run selfhost:env-reference -- --check passes clean against the committed output (the drift check CI runs in test:ci).
  • npm run typecheck and npm run ui:typecheck pass.
  • git diff --check clean.

Closes #8627

The selfhost-env-reference generator only recognized .env accesses whose
base is a bare process/c identifier, and helper calls carrying an env var
name in a single fixed argument slot. src/selfhost/posthog.ts reads
POSTHOG_SERVER_NAME through (globalThis as unknown as {...}).process?.env
and POSTHOG_MIN_SEVERITY/POSTHOG_REPO_MIN_SEVERITY as string-literal
arguments 2 and 3 of resolveSeverityThreshold(), so all three real
operator-facing vars were silently absent from the generated reference.

- isEnvContainer now accepts a .env base that resolves to
  globalThis.process through casts (new isProcessExpression), not just a
  bare process/c identifier.
- ENV_NAME_LITERAL_ARG_HELPERS entries now carry a list of argument
  indexes, and resolveSeverityThreshold is registered at indexes 2 and 3.
- Regenerated apps/loopover-ui/src/lib/selfhost-env-reference.ts: adds
  POSTHOG_MIN_SEVERITY, POSTHOG_REPO_MIN_SEVERITY, POSTHOG_SERVER_NAME
  (src/selfhost/posthog.ts) plus PAGERDUTY_MIN_SEVERITY and
  PAGERDUTY_REPO_MIN_SEVERITY (src/services/notify-pagerduty.ts:103,
  the same resolveSeverityThreshold blind spot in an already-scanned
  root, surfaced by the same generalized fix).
- Four new fixture tests (match + deliberate non-match for each new
  branch) in test/unit/selfhost-env-reference-script.test.ts.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.77%. Comparing base (b471ee1) to head (24675dd).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8632   +/-   ##
=======================================
  Coverage   92.77%   92.77%           
=======================================
  Files         798      798           
  Lines       79464    79464           
  Branches    24078    24078           
=======================================
  Hits        73725    73725           
  Misses       4592     4592           
  Partials     1147     1147           
Flag Coverage Δ
backend 93.65% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 25, 2026
@loopover-orb

loopover-orb Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-25 16:13:49 UTC

3 files · 1 AI reviewer · no blockers · readiness 88/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR fixes a real regression in the selfhost-env-reference AST visitor: `isProcessExpression` now recognizes `globalThis.process` reached through a cast chain (via the existing `unwrapEnvExpression`), and `ENV_NAME_LITERAL_ARG_HELPERS` now supports multiple argument indexes so `resolveSeverityThreshold`'s two literal-name arguments (indexes 2 and 3) are both captured. The generated reference file and tests are correctly regenerated/added to match, including negative tests for a different `.process` base and a same-arity non-allowlisted call. This is a narrow, well-targeted fix with good test coverage of both the new branch and its negative cases.

Nits — 5 non-blocking
  • The 'PostHog self-host env reads invisible to the visitor' describe block in test/unit/selfhost-env-reference-script.test.ts is missing a closing bracket check — actually the diff shows an extra blank line before the next describe block; verify formatting/lint passes cleanly given the +49/-0 hunk boundary.
  • scripts/gen-selfhost-env-reference.ts:131 introduces the literal `3` implicitly via the `[2, 3]` array for resolveSeverityThreshold — the external brief flags this as a magic number, but in context (explicit argument indexes tied to a documented call signature) this is self-explanatory and not worth a named constant.
  • The size-smell flag on apps/loopover-ui/src/lib/selfhost-env-reference.ts (686 lines) is pre-existing generated-file growth from a data table, not something this diff should be penalized for.
  • Consider a one-line comment in isProcessExpression noting it deliberately does not require optional-chaining (?.) on the `.process` access, to make clear that both `x.process.env` and `x?.process?.env` shapes are handled equivalently by the AST check.
  • If more helpers accumulate multiple literal-arg indexes in the future, the loop in collectEnvReads (scripts/gen-selfhost-env-reference.ts) is already generic enough to scale — no further generalization needed now.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8627
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ❌ 8/20 High review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 52 registered-repo PR(s), 21 merged, 3 issue(s).
Contributor context ✅ Confirmed Gittensor contributor rsnetworkinginc; Gittensor profile; 52 PR(s), 3 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff generalizes isEnvContainer to recognize the globalThis-cast .process.env shape via a new isProcessExpression helper, and extends ENV_NAME_LITERAL_ARG_HELPERS to support multiple argument indexes so resolveSeverityThreshold's args 2 and 3 are captured, matching both requirements without hardcoding specific var names. The regenerated selfhost-env-reference.ts adds POSTHOG_MIN_SEVERITY, POST

Review context
  • Author: rsnetworkinginc
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: TypeScript
  • Official Gittensor activity: 52 PR(s), 3 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Add a concise scope and risk note.
  • Then work through the remaining 1 step in the Signals table above.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit bacfcdf into JSONbored:main Jul 25, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

selfhost-env-reference generator misses POSTHOG_SERVER_NAME/POSTHOG_MIN_SEVERITY/POSTHOG_REPO_MIN_SEVERITY

1 participant