CE-1007: baton-sql library API (NewFromYAML + LookupFunc) by btipling · Pull Request #142 · ConductorOne/baton-sql

btipling · 2026-07-25T19:07:39Z

Summary

Library embed surface for C1 agent-authored SQL connectors (CE-1007 / ds-663v):

LookupFunc for ${KEY} expansion on connect fields/DSN (nil -> os.LookupEnv for CLI)
NewWithConfig / NewFromYAML public constructors (no os.Setenv)
OfflineValidate + RejectNonV1ProductFeatures (Postgres-only, single DB, sync-only; rejects multi-db, actions, provisioning, non-postgres scheme including postgresql)
Golden fixture examples/c1-golden-postgres.yml with map-value placeholder keys documented

Non-test production changes

PR: #142
HEAD: 50e20c0b
BASE: 9f01eeb4

Library-safe constructors so C1 can open a baton-sql connector from YAML bytes and resolve ${KEY} placeholders from a map, without writing secrets into process environment. CLI New still expands from the environment. Separate offline validators encode the v1 product bar: Postgres, one database, sync-only.

1. `pkg/database/database.go`

Where: LookupFunc ~L28; ConnectOptions.Lookup ~L59-60; resolveLookup ~L63-68; updateFromLookup ~L70-90; updateFromEnv ~L94-96; extractPlaceholders ~L102+; expandDSN ~L292+; ResolveDatabaseName / buildConnectionURL / expandValue call sites ~L357-607

What changed

Placeholder expansion takes an optional lookup function instead of always calling os.LookupEnv.
ConnectOptions carries Lookup. When it is nil, helpers still use process environment.
Internal expand path for DSN, structured connect fields, and params threads that lookup through.

Why

C1 will hold secrets in EnvConfig map values. Library embeds must not call os.Setenv to make ${KEY} work.
Nil lookup keeps the existing CLI binary path unchanged.

2. `pkg/connector/connector.go`

Where: LookupFunc alias ~L108; New ~L111-118; NewWithConfig ~L122-127; NewFromYAML ~L132-138; Config ~L141-143; newConnector ~L145-177

What changed

New still loads a file and passes a nil lookup into the shared constructor.
NewWithConfig builds from an already-parsed config with env expansion.
NewFromYAML parses YAML bytes and passes the caller lookup into connect options.
Config() exposes the parsed config for host callers.
newConnector sets ConnectOptions.Lookup before opening databases.

Why

C1 twin and MCP validate/smoke need in-process open from YAML + map secrets, same shape as baton-http embeds.
File-based operators keep one entrypoint with the old env behavior.

3. `pkg/bsql/offline_validate.go`

Where: new file; OfflineValidate ~L12-37; RejectNonV1ProductFeatures ~L41-67; ValidateYAML ~L70-76; validateConnectOffline ~L78-92; resolveConnectScheme ~L94-121

What changed

Structural checks without opening a database: app name, connect shape, resource type list query, then product gates.
Rejects multi-database connect, actions, account provisioning, credential rotation.
Scheme must resolve to postgres only. The postgresql alias fails the same check.
Whole-DSN-as-single-placeholder requires an explicit scheme field offline.

Why

Editor and RPC validate need a fail-closed answer before any network call.
Agent-authored v1 is intentionally narrower than full baton-sql CLI capability.

4. `examples/c1-golden-postgres.yml`

Where: whole file ~L1-53; connect placeholders ~L12-20; user resource_type list/map ~L22-53

What changed

New golden YAML: Postgres structured connect with ${DB_*} placeholders, map-value key list in the header comment, one user list query with static member entitlement.
Documents SaaS egress allowlisting expectation for the customer DB.

Why

Gives C1 and agents a known-good copy-paste shape for the SQL catalog path.
Keeps secrets out of the YAML by construction.

Excluded from detailed notes

Path	Reason
`pkg/bsql/offline_validate_test.go`	unit tests
`pkg/connector/connector_from_yaml_test.go`	unit tests
`pkg/database/database_test.go`	unit tests
`pkg/database/lookup_test.go`	unit tests

Call graph (library path)

NewFromYAML -> bsql.Parse -> newConnector -> ConnectOptions.Lookup -> buildConnectionURL / expandDSN via resolveLookup.
CLI stays New -> newConnector(..., nil) -> os.LookupEnv.
OfflineValidate / ValidateYAML are opt-in; not wired into New.

Test plan

go test ./...
Concurrent LookupFunc expansion isolation tests
Offline validate reject cases + happy minimal Postgres YAML

Follow-up

c1 vendors this pin via local_vendor/baton-sql and builds the SQL catalog HTTP twin.

linear-code · 2026-07-25T19:07:42Z

CE-1007

github-actions · 2026-07-25T19:09:58Z

+  database: ${DB_DATABASE}
+  user: ${DB_USER}
+  password: ${DB_PASSWORD}
+  params:


🟡 Suggestion: This "golden path" fixture connects over the public internet (the header comment notes the customer DB must allowlist ConductorOne egress IPs) but sets sslmode: disable, sending credentials and query data unencrypted. Consider sslmode: require (or stronger) as the reference default so downstream agent-authored configs don't inherit an insecure transport. (confidence: medium)

github-actions · 2026-07-25T19:20:44Z

@-

github-actions

No blocking issues found.

github-actions · 2026-07-25T20:46:10Z

+  user:
+    name: User
+    description: A user row in the database
+    list:


🟡 Suggestion: This golden fixture is documented as connecting from ConductorOne SaaS across the internet to a customer DB (egress-IP allowlisting comment above), yet sslmode: disable turns off TLS entirely — DB credentials and row data would transit in plaintext. Since golden examples tend to get copied verbatim, consider defaulting to sslmode: require (or verify-full) so the canonical path is TLS-on. (confidence: medium)

github-actions · 2026-07-25T20:46:27Z

 		varName := match[2 : len(match)-1]

-		value, exists := os.LookupEnv(varName)
+		value, exists := lookup(varName)


🟡 Suggestion: Now that LookupFunc backs this expansion, a map-based embedder (the new NewFromYAML path) that omits a key still gets environment variable %s is not set (line 81), which misdescribes the source as an env var. Consider a source-neutral message like placeholder ${%s} could not be resolved so library-embed errors read correctly. (confidence: medium)

github-actions · 2026-07-25T20:51:38Z

Connector PR Review: CE-1007: baton-sql library API (NewFromYAML + LookupFunc)

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 9f01eeb49ca5.
Review mode: full
View review run

Review Summary

Full PR diff scanned for security and correctness. This adds library-safe constructors (NewWithConfig, NewFromYAML) and threads an optional LookupFunc through DSN/connect-field expansion so embedders resolve ${KEY} from a map instead of process env, plus offline validators (OfflineValidate / RejectNonV1ProductFeatures) encoding the Postgres single-DB sync-only v1 bar. The lookup is threaded through every expansion call site (expandDSN, expandValue, extractPlaceholders) and both Connect/ConnectMany paths, nil correctly falls back to os.LookupEnv, Parse performs no env mutation, and missing-key errors name the key not the value. Tests cover happy path, missing key, concurrency (no cross-contamination), env isolation, and each v1-rejection case. No blocking issues found.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

examples/c1-golden-postgres.yml:22 — sslmode=require encrypts but does not verify the server certificate; consider verify-full for a SaaS-facing golden template customers copy (low confidence, may be an intentional tradeoff).

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `examples/c1-golden-postgres.yml`:
- Around line 22: The golden Postgres template sets `sslmode: require`, which encrypts the
  connection but does not authenticate the server certificate (no protection against MITM
  for public-internet customer DBs). Since this file is a template customers copy, consider
  defaulting to `sslmode: verify-full` (or `verify-ca`) and documenting `require` only for
  local non-TLS labs. Leave as-is if root-cert distribution to customers is not feasible and
  `require` is the intended product default.

github-actions

No blocking issues found.

Add injectable LookupFunc for ${KEY} expansion (no process env mutation), NewWithConfig/NewFromYAML for library embeds, OfflineValidate and RejectNonV1ProductFeatures for editor/agent offline checks, and a C1 golden Postgres fixture with externalized map-value secrets. CE-1007 / ds-663v Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>

github-actions

No blocking issues found.

C1 SaaS / public-internet customer DBs should use TLS by default. disable remains documented only for non-TLS local lab DBs. ds-kjkw Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>

github-actions · 2026-07-25T22:45:00Z

+      map:
+        id: ".username"
+        display_name: ".username"
+        description: ".email"


🟡 Suggestion: The query selects a stable u.id but the resource id maps to .username (a mutable field). If a user is renamed, C1 will see it as a delete + re-create, breaking grant continuity. Since this is the canonical golden fixture, consider mapping id: ".id" (keep display_name/login on .username) so the example demonstrates stable-ID practice. (confidence: medium)

Resource renames would otherwise surface as delete+create and break grant continuity. Keep display_name/login on .username. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>

github-actions · 2026-07-25T22:51:12Z

+  params:
+    # require: default for C1 SaaS / public-internet customer DBs (TLS).
+    # disable: only for non-TLS local lab DBs — not the golden path.
+    sslmode: require


🟡 Suggestion (low confidence): sslmode=require encrypts the connection but does not verify the server certificate, so it does not protect against MITM on the path to a public-internet customer DB. Since this is a golden template customers will copy, consider verify-full (or verify-ca) for the SaaS default and reserving require for local labs. Non-blocking — this may be an intentional tradeoff if customers can't reliably provide root certs.

github-actions

No blocking issues found.

btipling requested a review from a team July 25, 2026 19:07

github-actions Bot reviewed Jul 25, 2026

View reviewed changes

btipling force-pushed the bt/ce-1007-library-api branch from 6a6ee3b to 50e20c0 Compare July 25, 2026 21:58

github-actions Bot reviewed Jul 25, 2026

View reviewed changes

fix: golden Postgres fixture defaults sslmode to require

65e311f

C1 SaaS / public-internet customer DBs should use TLS by default. disable remains documented only for non-TLS local lab DBs. ds-kjkw Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>

github-actions Bot reviewed Jul 25, 2026

View reviewed changes

fix: golden map.id uses stable u.id not username

8f181e1

Resource renames would otherwise surface as delete+create and break grant continuity. Keep display_name/login on .username. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>

github-actions Bot reviewed Jul 25, 2026

View reviewed changes

btipling merged commit 9e81038 into main Jul 25, 2026
9 checks passed

btipling deleted the bt/ce-1007-library-api branch July 25, 2026 23:02

Uh oh!

Conversation

btipling commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Non-test production changes

1. pkg/database/database.go

2. pkg/connector/connector.go

3. pkg/bsql/offline_validate.go

4. examples/c1-golden-postgres.yml

Excluded from detailed notes

Call graph (library path)

Test plan

Follow-up

Uh oh!

linear-code Bot commented Jul 25, 2026

Uh oh!

github-actions Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 25, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Connector PR Review: CE-1007: baton-sql library API (NewFromYAML + LookupFunc)

Review Summary

Security Issues

Correctness Issues

Suggestions

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

btipling commented Jul 25, 2026 •

edited

Loading

1. `pkg/database/database.go`

2. `pkg/connector/connector.go`

3. `pkg/bsql/offline_validate.go`

4. `examples/c1-golden-postgres.yml`

github-actions Bot commented Jul 25, 2026 •

edited

Loading