Skip to content

docs: clarify env vars in quick start are for local dev; warn against .env for runtime secrets - #55

Merged
heskew merged 12 commits into
mainfrom
claude/fix-32-env-var-docs
Apr 23, 2026
Merged

docs: clarify env vars in quick start are for local dev; warn against .env for runtime secrets#55
heskew merged 12 commits into
mainfrom
claude/fix-32-env-var-docs

Conversation

@claude

@claude claude Bot commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds For **local development** label to the export command block in the Quick Start
  • Adds a note explaining that Harper reads from the process environment directly (does not load .env automatically)
  • Warns that Harper's .env convention is for dotenv-cli deploy CLI credentials, not runtime OAuth secrets
  • Directs Harper Fabric users to inject secrets via CI/CD pipeline (e.g., GitHub repository secrets)

Closes #32

Test plan

  • Doc-only change: npm run format:check passes for the changed file
  • npm run lint reports no new errors

github-actions Bot and others added 8 commits April 21, 2026 19:47
Harper does not auto-load .env files; secrets must be injected
via the process environment (systemd, Kubernetes, cloud secrets
managers, etc.). Add a note to both quick start sections so
users are not misled into using a .env file in production.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two changes rolled together — both needed to get reliable reviews:

1. Layered review-scopes integration. Clones HarperFast/ai-review-log
   at the merge of #19 (ef8d994), composes universal + harper/common +
   harper/v5 + repo-type/plugin layers into a single prompt block, and
   replaces the previous monolithic inline prompt's "what to ignore",
   "review scope and style", and "output format" sections with the
   composed layers. OAuth-specific bullets (CSRF state, provider-of-
   record, redirect/path validation, session-field preservation across
   refresh) stack on top. Review guidance for other HarperFast repos
   (core, pro, manager, apps) now lives centrally; bumping
   REVIEW_LAYERS is the mechanism to opt each repo in.

2. Timeout 10→15. A recent run on PR #48 cancelled at 10m18s with a
   partial tool_result logged at 30s and then 9+ minutes of silence.
   The stall was inside a single Claude API call, not a max-turns
   loop. 15 gives headroom without removing the backstop; max-turns=24
   remains the real cost ceiling.

Clone uses AI_REVIEW_LOG_TOKEN (already configured in repo secrets).
Compose step fails the job if every layer is missing (hard error), but
tolerates individual missing layers with a warning.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per-repo copy of the layer files (copied from
HarperFast/ai-review-log/review-scopes/ at merge SHA ef8d994) while we
evaluate the layered-scope approach across a few repos. Compose step
reads from .github/review-scopes/; the external clone step is removed.

See .github/review-scopes/README.md for provenance and the plan for
eventually sharing these across repos.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Keep .github/review-scopes/ byte-identical to upstream so future syncs
against HarperFast/ai-review-log don't have to account for local
formatting drift.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Caught in external review: the REVIEW_LAYERS comment still referenced
HarperFast/ai-review-log/review-scopes/README.md, but the vendor commit
moved the files (and the authoritative pointer) local. Point at
.github/review-scopes/README.md instead.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The layered universal.md was telling reviewers to check "New public APIs
have tests / Failure-path coverage matches severity / etc." without
distinguishing between the PR's own new code and pre-existing gaps in
code the PR merely touches. Without a definition of "blocker-severity"
in the Output discipline section, reviewers treated every Testing bullet
as blocker-worthy — which scales poorly on repos that are still catching
up on test coverage.

Two changes:

1. Testing section now explicitly scopes each check to what the PR
   ITSELF introduces. A lead-in line and a closing line both state that
   pre-existing gaps are out of scope. Each bullet tags the check as a
   blocker so the gradation is explicit.

2. Output discipline grows an explicit "What counts as a blocker" list
   (correctness, security, broken contracts, PR-introduced test gaps,
   misleading docs) and a "What is NOT a blocker" list (pre-existing
   coverage, style, readability suggestions, edge-case tests when
   happy-path and primary failure are covered, prose polish,
   speculative architecture).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nst .env for runtime secrets

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-docs

# Conflicts:
#	.github/review-scopes/universal.md
Comment thread README.md Outdated
Comment thread docs/getting-started.md Outdated
@claude

claude Bot commented Apr 23, 2026

Copy link
Copy Markdown
Contributor Author

Review: docs: clarify env vars in quick start

2 blockers found. Both are in the documentation itself — no code changes in this PR.


1. README.md — wrong deployment platform guidance

File: README.md:42
What: The new note recommends systemd unit file, Kubernetes secrets, and cloud provider secrets manager as the go-to production options.
Why it matters: Harper's default deployment target is Fabric. The review scope for this repo explicitly prohibits recommending generic non-Fabric patterns unless the user has opted out of Fabric. A developer reading this will reach for the wrong tooling. Ironically, docs/getting-started.md (same PR) gets the Fabric-first framing right — the README note should match it.
Suggested fix: See inline suggestion — replace the generic list with the same Fabric / CI/CD guidance used in getting-started.md.


2. docs/getting-started.md — .env advice is incorrect

File: docs/getting-started.md:35-36
What: The note tells users "Do not store OAuth secrets in a .env file" and implies .env is only for CLI_TARGET deploy credentials.
Why it matters: The standard npm create harper template scaffolds dotenv-cli into npm run dev (dotenv -- npm run dev:component). That script loads .env before the Harper runtime starts, making OAuth secrets placed there (OAUTH_GITHUB_CLIENT_ID, etc.) correctly available to the process — the exact same mechanism as CLI_TARGET. The distinction the note draws is artificial and contradicts the official Harper toolchain. A developer following the standard template will be told not to do the thing that actually works.
Suggested fix: See inline suggestion — explain that .env is fine for local dev (OAuth secrets included), warn only against committing it, then give the correct Fabric guidance for production.


Surfaces verified:

  • README.md diff: new note at line 42 — fails Fabric-first check
  • docs/getting-started.md diff: new note at lines 35–37 — incorrect .env restriction; Fabric guidance (line 37) is correct
  • No code changes in this PR; architecture, security, and test layers not applicable

@heskew

heskew commented Apr 23, 2026

Copy link
Copy Markdown
Member

@claude address both blockers in claude[bot]'s review above. For README.md, mirror the Fabric/CI-secrets framing already in docs/getting-started.md. For docs/getting-started.md, loosen the .env restriction — .env via dotenv-cli is the correct local-dev pattern for OAuth secrets; only warn against committing it. Keep the Fabric production guidance.

…DME with Fabric/CI framing

- README.md: replace generic deployment platform language (systemd, Kubernetes, cloud secrets)
  with Fabric/CI-secrets framing that mirrors docs/getting-started.md
- docs/getting-started.md: loosen .env restriction — .env via dotenv-cli is the correct
  local-dev pattern for OAuth secrets; only warn against committing it

Addresses review comments on #55.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread docs/getting-started.md Outdated
Comment thread README.md Outdated
@claude

claude Bot commented Apr 23, 2026

Copy link
Copy Markdown
Contributor Author

Review: PR #55 — docs: clarify env vars for local dev / Fabric

1. Fabric runtime secrets guidance conflates deploy credentials with runtime app configuration

Files: README.md:42, docs/getting-started.md:37

What: Both added notes tell Harper Fabric users to inject OAuth secrets (e.g. OAUTH_GITHUB_CLIENT_ID) via GitHub repository secrets passed through the CI/CD env: block in their deploy workflow.

Why it matters: The Harper v5 toolchain draws a hard line between two categories of secrets:

  • Deploy credentials (CLI_TARGET, CLI_TARGET_USERNAME, CLI_TARGET_PASSWORD) — passed in the GitHub Actions env: block so the harperdb deploy_component CLI can authenticate with the cluster. The deploying-to-harper-fabric.md skill shows exactly this pattern.
  • Runtime app configuration (OAuth client ID/secret) — described by the v5 conventions as "runtime configuration injected by the platform." This is a distinct mechanism; the platform (Fabric) is responsible for making these vars available to the running Harper component.

The env: block in a deploy workflow step scopes those vars to the npm run deploy process. There is no documented mechanism by which they become environment variables in the live Fabric environment where Harper actually processes OAuth requests. A user who follows the PR's guidance exactly could end up with correctly-deployed code that fails every OAuth flow at runtime because OAUTH_GITHUB_CLIENT_ID resolves to undefined.

Suggested fix: Determine how Fabric surfaces runtime app secrets to a deployed component (e.g., Fabric-level env var config, config.yaml interpolation with Fabric-managed secrets, etc.) and document that mechanism. If the CI/CD env: block pattern genuinely does propagate to the Fabric runtime (e.g., the deploy command persists them as cluster-level env vars), add a sentence making that explicit so users understand why the pattern works, not just that it works.


What the PR gets right

  • Local-dev guidance (export + .env with dotenv-cli) aligns correctly with the authoritative creating-harper-apps.md and deploying-to-harper-fabric.md skills.
  • "Don't commit it to source control" warning is appropriate and well-placed.
  • Both files are consistent with each other.
  • No code-path changes; purely additive doc clarification.

@heskew

heskew commented Apr 23, 2026

Copy link
Copy Markdown
Member

@claude please revise the two added notes to narrow their scope — this repo's docs shouldn't re-explain Harper/Fabric deployment mechanics, and in the current wording we're overreaching into territory that belongs in Harper's own docs.

Keep and preserve:

  • The local-dev framing (export / .env + dotenv-cli — OAuth-specific because the env-var names are ours)
  • The "don't commit .env to source control" warning

Change:

  • Remove the specific "inject via GitHub repo secrets / CI/CD env: block" claim. That pattern is documented for deploy-CLI credentials (CLI_TARGET_*), but it's not the mechanism for runtime app secrets, and mixing the two in our docs misleads users.
  • For production guidance, replace the current Fabric paragraph with a short pointer: something like "For Harper Fabric deployments, your app-root .env is deployed alongside your component, so the same .env you use locally works in production — see the Harper Fabric documentation for managing runtime environment variables at https://docs.harperdb.io/docs/fabric/managing-applications (or the current authoritative URL in the Harper docs)."

The goal is: this PR's docs stay OAuth-specific (which env vars to set, don't commit them). Anything about HOW Harper/Fabric loads env vars gets deferred to Harper's own documentation, for easier maintenance.

Scope this to README.md and docs/getting-started.md. Two-line commit, no other files.

…Harper docs

Remove CI/CD injection guidance (belongs with deploy-CLI credentials, not
runtime secrets). Replace with a pointer to Harper Fabric documentation for
managing runtime env vars. Preserve local-dev framing and .env warning.

Addresses review feedback on PR #55.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude

claude Bot commented Apr 23, 2026

Copy link
Copy Markdown
Contributor Author

Done — pushed a two-line commit (68dc649) that:

  • README.md: replaces the CI/CD injection sentence with a pointer to the Harper Fabric docs for managing runtime environment variables.
  • docs/getting-started.md: same removal, replaced with the Fabric-specific note that the app-root .env is deployed alongside the component (so local and production use the same file), plus the docs link.

Local-dev framing and the "don't commit .env" warning are preserved as-is. No other files touched.

Comment thread docs/getting-started.md
@claude

claude Bot commented Apr 23, 2026

Copy link
Copy Markdown
Contributor Author

Review: docs: clarify env vars in quick start (#55)

1. Incorrect Fabric guidance in docs/getting-started.md

File: docs/getting-started.md:37
What: The note states "your app-root .env is deployed alongside your component, so the same .env you use locally works in production." This is factually incorrect per Harper's Fabric deployment model.
Why it matters: Per the deploying-to-harper-fabric.md skill, .env in the Fabric context holds CLI deploy credentials (CLI_TARGET*) loaded by dotenv-cli — not runtime application secrets. The review scope for this repo explicitly states: "Runtime configuration is injected by the platform." Pointing users at .env as the production secret-management path for OAuth credentials directly contradicts the platform's actual mechanism and could lead integrators to either commit secrets to source control or be confused when CI/CD deployments (which start from a clean checkout) don't have the file. The note also contradicts itself: "just don't commit it to source control" and "the same .env you use locally works in production" can't both be true in a CI/CD pipeline.
Suggested fix: Match README.md, which handles this correctly by simply deferring: "For Harper Fabric deployments, runtime secrets are injected by the platform — see the Harper Fabric documentation for managing environment variables." (Inline suggestion posted.)


What I traced

  • README.md change (line 42): correctly defers Fabric env-var mechanics to Fabric docs — no issue.
  • docs/getting-started.md local-dev additions (lines 28, 35): correctly scoped to local dev, dotenv-cli note accurate — no issue.
  • docs/getting-started.md Fabric note (lines 36–37): incorrect claim about .env deployment — blocker above.
  • No code changes; no test, security, or API-contract surface to audit beyond the doc content.

@heskew
heskew merged commit 41fea19 into main Apr 23, 2026
7 checks passed
@heskew
heskew deleted the claude/fix-32-env-var-docs branch April 23, 2026 15:35
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.

Improve documentation around environment variables

1 participant