Skip to content

fix(store): refuse non-string models.yml header values at the door (RIG-3134) - #2224

Merged
mattwilkinsonn merged 1 commit into
mainfrom
compass-comms/rig-3134-models-header-shapes
Oct 12, 2026
Merged

mattwilkinsonn merged 1 commit into
mainfrom
compass-comms/rig-3134-models-header-shapes

Conversation

@rigel-mintaka

@rigel-mintaka rigel-mintaka commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

This PR is part of a stack containing 2 PRs:

  1. main
  2. "fix(store): refuse non-string models.yml header values at the door (RIG-3134)" (this PR)
  3. fix(store): check model-level headers at the models.yml door (RIG-5287) #2227

What

rejectCredentialModels (go/internal/store/agent_config.go) skipped a providers.<name>.headers value that was not a string, so isEnvIndirection never checked it. It also skipped a headers node that was not a mapping.

It now refuses both shapes, and the error names the offending path:

  • A header value that is not a string (mapping, list, number, null). The SDK schema (@oh-my-pi/pi-coding-agent@18.0.11, { "[string]": "string" }) rejects all of these, so no valid models.yml is affected.
  • A headers node that is a list. The SDK schema accepts it. createLiveConfigHeaders then sends each item under its index (probe: ["Bearer sk-live-123"] → {"0":"Bearer sk-live-123"}), so a literal secret in a list reached the request. This was a real gap.
  • headers: {} and headers: ~ still pass. New accept cases pin this, because the guard relies on yamlMapEntries returning a non-nil empty slice.
  • headers: [] is now refused even though it sends nothing. A list is never a valid header map, so I chose this on purpose.

Tests

  • Red first: 5 new cases in TestValidateConfigBundleRejectsCredentialKeys failed on main and pass now.
  • Mutations go red: dropping the non-mapping guard fails models headers list; skipping non-string values again fails the 4 value-shape cases; refusing an empty mapping fails models.yml empty headers mapping.
  • go test ./internal/store/ passes. golangci-lint reports 0 issues. go vet is clean.

Out of scope

models[].headers and modelOverrides.<id>.headers pass the door entirely. The SDK sends them the same way. Filed as RIG-5287.

Spec-impact: none
Ledger-impact: none

Closes RIG-3134

Co-authored-by: Matt Wilkinson matt@rigel.build

…IG-3134)

rejectCredentialModels skipped a providers.<name>.headers value that was not
a string, so a mapping, list, number or null value was never checked by
isEnvIndirection. It also skipped a headers node that was a list.

The installed SDK schema types every header value as a string, so none of
these shapes is a valid models.yml. A headers list is worse: the SDK's
createLiveConfigHeaders sends each list item under its index as a header
name, so a literal secret in a list reaches the request. Both shapes are
now refused with the offending path named.

Co-authored-by: Matt Wilkinson <matt@rigel.build>
@trunk-io

trunk-io Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

😎 This pull request was merged.

@rigel-mintaka

Copy link
Copy Markdown
Contributor Author

Review at head e5f669f6: high 0, medium 0, low 0, nit 0

  • Scope: the whole diff, all lenses, 1 round. Round 1 found 2 low: (1) the empty-mapping contract was unpinned, so I added accept cases for headers: {} and headers: ~; the mutation goes red. (2) Refusing headers: [] is stricter than the SDK; kept on purpose and noted in the body.
  • Checks: go test ./internal/store/ passes, golangci-lint reports 0 issues, vet is clean. The red-first cases and 3 mutations all go red.

@linear-code

linear-code Bot commented Oct 12, 2026

Copy link
Copy Markdown

RIG-3134

@github-actions

Copy link
Copy Markdown

Compass engineering docs preview: https://compass-comms-rig-3134-model.compass-eng-docs.pages.dev

Deployed from compass-comms/rig-3134-models-header-shapes at e5f669f.

@mattwilkinsonn
mattwilkinsonn merged commit be9a90e into main Oct 12, 2026
21 checks passed
@mattwilkinsonn
mattwilkinsonn deleted the compass-comms/rig-3134-models-header-shapes branch October 12, 2026 19:18
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