Skip to content

fix(sdk/go): return isolated session definitions - #1044

Open
emecii wants to merge 3 commits into
Agent-Field:mainfrom
emecii:fix/sdk-go-defensive-session-definitions
Open

emecii wants to merge 3 commits into
Agent-Field:mainfrom
emecii:fix/sdk-go-defensive-session-definitions

Conversation

@emecii

@emecii emecii commented Sep 7, 2026

Copy link
Copy Markdown

Summary

Agent.SessionDefinitions() returned copied SessionDefinition structs, but
their slice and metadata fields still shared state with the registered
definitions. This makes each returned definition an isolated snapshot,
including recursively nested metadata maps and slices, overlapping slice views,
and the TurnDetection configuration and its optional scalar pointers.
Nil, zero, and false values are preserved.

Type of change

  • Bug fix
  • New feature
  • Refactor / cleanup
  • Docs only
  • Tests only
  • CI / tooling
  • Breaking change

Test plan

Local validation by Codex on Go 1.23.12 / macOS arm64 (TMPDIR=/private/tmp):

  • cd sdk/go && go test ./... -count=1 (all eight SDK packages)
  • Focused session/turn-detection race tests repeated 20 times
  • go build ./...
  • cd sdk/go && go vet ./agent
  • gofmt -d agent/session.go agent/session_test.go (no output)
  • go mod tidy -diff (no diff)
  • The new regression fails against the previous SessionDefinitions() implementation and passes with this change.

Test coverage

  • I ran tests for the changed Go SDK surface locally.
  • New code paths are covered by tests in this PR.
  • The coverage gate is green in CI before requesting review.

Checklist

  • I have read CONTRIBUTING.md.
  • Commit follows conventional-commits style.
  • Related issue: no existing issue covers this focused bug fix.

AI assistance

This change was prepared with OpenAI Codex. The validation listed above was run locally.
Repository-wide scripts were not completed because the local Python environment lacks pytest/ruff.
Hosted checks for the updated head await maintainer workflow approval.

@emecii
emecii requested review from a team and AbirAbbas as code owners September 7, 2026 09:58
@CLAassistant

CLAassistant commented Sep 7, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@santoshkumarradha santoshkumarradha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good. The snapshot now isolates all session slices plus nested map and slice metadata, including cycles, while preserving nil values. The focused defensive-snapshot test passes locally.

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Performance

SDK Memory Δ Latency Δ Tests Status
Go 189 B -33% 0.46 µs -54%

✓ No regressions detected

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

📊 Coverage gate

Thresholds from .coverage-gate.toml: per-surface ≥ 84%, aggregate ≥ 85%, max per-surface regression ≤ 1.0 pp, max aggregate regression ≤ 0.50 pp.

Surface Current Baseline Δ
control-plane 87.80% 87.40% ↑ +0.40 pp 🟡
sdk-go 93.20% 92.00% ↑ +1.20 pp 🟢
sdk-python 94.72% 93.73% ↑ +0.99 pp 🟢
sdk-typescript 91.76% 90.42% ↑ +1.34 pp 🟢
web-ui 84.77% 84.79% ↓ -0.02 pp 🟡
aggregate 85.89% 85.75% ↑ +0.14 pp 🟡

✅ Gate passed

No surface regressed past the allowed threshold and the aggregate stayed above the floor.

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

📐 Patch coverage gate

Threshold: 80% on lines this PR touches vs origin/main (from .coverage-gate.toml:thresholds.min_patch).

Surface Touched lines Patch coverage Status
control-plane 0 ➖ no changes
sdk-go 77 84.00%
sdk-python 0 ➖ no changes
sdk-typescript 0 ➖ no changes
web-ui 0 ➖ no changes

✅ Patch gate passed

Every surface whose lines were touched by this PR has patch coverage at or above the threshold.

@santoshkumarradha

Copy link
Copy Markdown
Member

The implementation and all CI checks look good. The CLA is the only remaining step; once that’s signed, this should be good to go.

@AbirAbbas AbirAbbas 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.

I built a standalone program against this branch's sdk/go that registers session metadata with a map inside a slice inside a map, a typed []string, a map[string]string, []byte, a reference cycle and nil values, mutates every level of the SessionDefinitions() result and then re-reads the registry. On this branch every one of those stays isolated, nils keep their typed-nil-ness, the cycle stays a cycle and terminates, and the JSON of the definition is unchanged; the same program run against main leaks all of them, so the fix does what it says. go test -race ./agent/... is clean and patch coverage on the touched lines is 80%.

One real problem though, left inline: two metadata slices that share a backing array collide in the copy memo and one of them comes back with the wrong length. It's nondeterministic and silent, and it's a way for the payload to be wrong that doesn't exist on main, so I'd rather see it fixed here than shipped.

Separately, and fine to leave as is: the default: branch hands back pointers, structs with reference fields and arrays as they are, so map[string]any{"cfg": &Cfg{...}} still aliases into the registry. That's outside what this PR set out to do, but a line on cloneSessionMetadataValue saying only maps and slices are deep-copied would save the next reader the experiment.

Comment thread sdk/go/agent/session.go Outdated
@emecii

emecii commented Sep 12, 2026

Copy link
Copy Markdown
Author

CLA signed.

@santoshkumarradha santoshkumarradha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good. The updated memo key preserves overlapping slice views, and both defensive snapshot regressions pass locally under the race detector. I also approved the pending fork workflows now that the CLA is signed.

@santoshkumarradha
santoshkumarradha added this pull request to the merge queue Sep 15, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Sep 15, 2026
@emecii

emecii commented Sep 15, 2026

Copy link
Copy Markdown
Author

Merged current main (2180e30c) in 146e5fa3 to resolve the conflict with #1056. Both existing snapshot regressions and the upstream turn-detection registration test are retained unchanged, including the reviewed slice memo key fix.

The integration also needs to detach the newly introduced TurnDetection struct and its five scalar pointers. Added that copy while preserving nil, zero, and false values. The new regression reproduced aliasing before the fix and now passes for defaults, explicit zero/false, semantic VAD, and no turn detection.

Validation with Go 1.23.12 on macOS arm64 (TMPDIR=/private/tmp): full Go SDK go test ./... -count=1, 20 repeated focused race runs (Session|TurnDetection), build, vet, tidy with no diff, and formatting all pass. The repository-wide script remains unavailable locally because pytest is missing; make fmt tidy also stops on missing ruff. No unrelated formatting changes are included. The effective diff against current main remains limited to the two Go session files.

@santoshkumarradha santoshkumarradha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I rechecked the rebased head. The snapshot clone still preserves shared and cyclic metadata safely, distinguishes overlapping slice views, and now includes the current turn-detection fields from main. The focused Go tests, vet, and diff check pass locally.

@santoshkumarradha

Copy link
Copy Markdown
Member

recheck

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.

4 participants