Skip to content

env var updates#3061

Merged
akshaydeo merged 4 commits intomainfrom
04-26-env_var_updates
Apr 27, 2026
Merged

env var updates#3061
akshaydeo merged 4 commits intomainfrom
04-26-env_var_updates

Conversation

@akshaydeo
Copy link
Copy Markdown
Contributor

Summary

Briefly explain the purpose of this PR and the problem it solves.

Changes

  • What was changed and why
  • Any notable design decisions or trade-offs

Type of change

  • Bug fix
  • Feature
  • Refactor
  • Documentation
  • Chore/CI

Affected areas

  • Core (Go)
  • Transports (HTTP)
  • Providers/Integrations
  • Plugins
  • UI (React)
  • Docs

How to test

Describe the steps to validate this change. Include commands and expected outcomes.

# Core/Transports
go version
go test ./...

# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build

If adding new configs or environment variables, document them here.

Screenshots/Recordings

If UI changes, add before/after screenshots or short clips.

Breaking changes

  • Yes
  • No

If yes, describe impact and migration instructions.

Related issues

Link related issues and discussions. Example: Closes #123

Security considerations

Note any security implications (auth, secrets, PII, sandboxing, etc.).

Checklist

  • I read docs/contributing/README.md and followed the guidelines
  • I added/updated tests where appropriate
  • I updated documentation where needed
  • I verified builds succeed (Go and UI)
  • I verified the CI pipeline passes locally if applicable

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 26, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 44501a83-6858-4ba0-bd92-67bbc305b9d8

📥 Commits

Reviewing files that changed from the base of the PR and between 880f504 and fdcb08f.

📒 Files selected for processing (2)
  • core/schemas/envvar.go
  • plugins/semanticcache/main.go
✅ Files skipped from review due to trivial changes (1)
  • plugins/semanticcache/main.go

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Improved environment variable handling to better determine when stored configurations should be preserved or overwritten based on source and content.
  • Chores

    • Updated internal code patterns and type definitions for consistency.

Walkthrough

Adds ShouldPreserveStored() to EnvVar to decide preservation of stored values based on source and redaction/emptiness, and adjusts types/initialization in the semantic cache plugin (StreamAccumulator.Metadata to map[string]any and boolean pointer defaults).

Changes

Cohort / File(s) Summary
Environment Variable Storage Control
core/schemas/envvar.go
Added func (e *EnvVar) ShouldPreserveStored() bool implementing overwrite/preservation logic: returns true for nil receiver, false if value is from environment, and true when a non-env value is empty or redacted.
Semantic Cache Plugin Updates
plugins/semanticcache/main.go
Changed StreamAccumulator.Metadata from map[string]interface{} to map[string]any. Replaced bifrost.Ptr(true) defaults with new(true) for Config.CacheByModel and Config.CacheByProvider initialization.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 I nibble bytes beneath the moonlit log,
I guard the hush of redacted cog,
"Keep what's quiet, skip what's from the air," I sing—
Lighter maps and safer things, I spring,
Hoppity-hop, a tiny change, a jaunty jog. 🥕

🚥 Pre-merge checks | ✅ 1 | ❌ 4

❌ Failed checks (1 warning, 3 inconclusive)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is entirely a blank template with no actual content filled in—no summary, changes, type, affected areas, or other details are provided. Fill in all required sections with concrete information about the changes, including summary, specific changes made, type of change, affected areas, and testing approach.
Title check ❓ Inconclusive The title 'env var updates' is vague and generic, not describing the specific changes or their purpose. Provide a more specific title that describes the main change, such as 'Add ShouldPreserveStored method to EnvVar' or similar.
Linked Issues check ❓ Inconclusive The linked issue #123 requests File APIs support for providers, but the PR changes only add ShouldPreserveStored() to EnvVar and update StreamAccumulator metadata—unclear if these changes address that issue's requirements. Clarify the relationship between the PR changes and issue #123, or link the correct issue if this PR is unrelated to Files API support.
Out of Scope Changes check ❓ Inconclusive The PR modifies files in core schemas and plugins, but without a clear problem statement or issue requirements, it is difficult to determine if all changes are in scope. Provide a detailed PR description linking specific changes to issue requirements to clarify scope.
✅ Passed checks (1 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 04-26-env_var_updates

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.11.4)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Comment @coderabbitai help to get the list of available commands and usage tips.

@akshaydeo akshaydeo marked this pull request as ready for review April 26, 2026 17:33
Copy link
Copy Markdown
Contributor Author

akshaydeo commented Apr 26, 2026

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 26, 2026

Confidence Score: 2/5

Not safe to merge — the plugin file contains a compile error that prevents the package from building.

An unresolved P0 compile error (new(true) in semanticcache/main.go) caps the score at 2/5. The envvar.go change is clean and correct on its own.

plugins/semanticcache/main.go — the new(true) calls on lines 311 and 314 must be reverted to bifrost.Ptr(true) before this can build.

Important Files Changed

Filename Overview
core/schemas/envvar.go Adds ShouldPreserveStored() helper on *EnvVar — nil-safe, correctly returns true for empty/redacted non-env values and false for env-var references or plain new values. Logic and doc comment are consistent.
plugins/semanticcache/main.go Renames map[string]interface{} to map[string]any (fine alias change), but replaces bifrost.Ptr(true) with new(true) — invalid Go syntax that will fail to compile (previously flagged).

Reviews (7): Last reviewed commit: "env var updates" | Re-trigger Greptile

Comment thread plugins/semanticcache/main.go
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
plugins/semanticcache/main.go (1)

311-314: Use bifrost.Ptr(true) instead of new(true) on lines 311 and 314 for consistency.

The codebase throughout semanticcache consistently uses bifrost.Ptr() for pointer initialization, including for the same CacheByModel and CacheByProvider fields in test files. Lines 311 and 314 are the only instances in the plugin that use new(true).

Suggested change
 	if config.CacheByModel == nil {
-		config.CacheByModel = new(true)
+		config.CacheByModel = bifrost.Ptr(true)
 	}
 	if config.CacheByProvider == nil {
-		config.CacheByProvider = new(true)
+		config.CacheByProvider = bifrost.Ptr(true)
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/semanticcache/main.go` around lines 311 - 314, Replace the ad-hoc
pointer creation using new(true) with the project's helper function
bifrost.Ptr(true) for consistency: change any occurrences where
config.CacheByModel and config.CacheByProvider are set via new(true) to use
bifrost.Ptr(true) instead, and ensure the bifrost package is imported/available
in the file so the calls compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@plugins/semanticcache/main.go`:
- Around line 311-314: Replace the ad-hoc pointer creation using new(true) with
the project's helper function bifrost.Ptr(true) for consistency: change any
occurrences where config.CacheByModel and config.CacheByProvider are set via
new(true) to use bifrost.Ptr(true) instead, and ensure the bifrost package is
imported/available in the file so the calls compile.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a9a7c1d4-b34e-43ef-8d3e-ecc58cdca026

📥 Commits

Reviewing files that changed from the base of the PR and between 1de1e53 and e7a62e3.

📒 Files selected for processing (2)
  • core/schemas/envvar.go
  • plugins/semanticcache/main.go

coderabbitai[bot]
coderabbitai Bot previously approved these changes Apr 26, 2026
@akshaydeo akshaydeo mentioned this pull request Apr 26, 2026
18 tasks
@akshaydeo akshaydeo force-pushed the 04-26-vertex_anthropic_fixes branch from 1de1e53 to 650b3a2 Compare April 27, 2026 08:23
@akshaydeo akshaydeo force-pushed the 04-26-env_var_updates branch 2 times, most recently from 91a8e27 to b55ebc8 Compare April 27, 2026 08:41
@akshaydeo akshaydeo force-pushed the 04-26-vertex_anthropic_fixes branch from 650b3a2 to db5d8b1 Compare April 27, 2026 08:41
@akshaydeo akshaydeo force-pushed the 04-26-env_var_updates branch from b55ebc8 to a9dd3d6 Compare April 27, 2026 08:58
@akshaydeo akshaydeo force-pushed the 04-26-vertex_anthropic_fixes branch from db5d8b1 to 792ecff Compare April 27, 2026 08:58
@akshaydeo akshaydeo force-pushed the 04-26-vertex_anthropic_fixes branch from 792ecff to 0e71c35 Compare April 27, 2026 09:08
@akshaydeo akshaydeo force-pushed the 04-26-env_var_updates branch from a9dd3d6 to 4775cb0 Compare April 27, 2026 09:08
@akshaydeo akshaydeo force-pushed the 04-26-vertex_anthropic_fixes branch from 0e71c35 to 9516aff Compare April 27, 2026 11:24
@akshaydeo akshaydeo force-pushed the 04-26-env_var_updates branch from 4775cb0 to 880f504 Compare April 27, 2026 11:24
@akshaydeo akshaydeo force-pushed the 04-26-vertex_anthropic_fixes branch from 9516aff to 663292e Compare April 27, 2026 12:07
@akshaydeo akshaydeo force-pushed the 04-26-env_var_updates branch from 880f504 to fdcb08f Compare April 27, 2026 12:07
Copy link
Copy Markdown
Contributor Author

akshaydeo commented Apr 27, 2026

Merge activity

  • Apr 27, 8:08 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Apr 27, 8:12 PM UTC: @akshaydeo merged this pull request with Graphite.

@akshaydeo akshaydeo changed the base branch from 04-26-vertex_anthropic_fixes to graphite-base/3061 April 27, 2026 20:11
@akshaydeo akshaydeo changed the base branch from graphite-base/3061 to main April 27, 2026 20:11
@akshaydeo akshaydeo dismissed coderabbitai[bot]’s stale review April 27, 2026 20:11

The base branch was changed.

@akshaydeo akshaydeo merged commit ce38ad8 into main Apr 27, 2026
14 of 16 checks passed
@akshaydeo akshaydeo deleted the 04-26-env_var_updates branch April 27, 2026 20:12
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.

Files API Support

2 participants