Skip to content

[CLI] Track: var-of-func seam on chatHistorySuppressionNotice is not safe for concurrent callers #174

Description

@charlesgreen

Context

Surfaced during the code review of PR fix/cli-chat-cleanup (#164/#165/#167/#168).

`cmd/aixgo/cmd/chat.go` exposes `chatHistorySuppressionNotice` as a package-level `var` of function type so tests can stub the stderr emission. Gating is a package-level `sync.Once`.

Current status: Accepted Risk

This is deliberately tracked rather than fixed. Justification:

  • The interactive chat loop is single-threaded per invocation.
  • No production code path spawns goroutines that touch this seam.
  • `go test -race ./cmd/aixgo/...` is clean.
  • A future test that stubs the hook while also running concurrent writers would be the correct trigger to revisit.

When to act

Revisit if any of these become true:

  1. The chat loop gains concurrent worker goroutines (e.g. async tool calls that log to stderr from multiple goroutines).
  2. Tests are added that stub `chatHistorySuppressionNotice` while spawning goroutines — at that point the right fix is to introduce a `chatSession` struct type and move the hook onto it, not to patch the current seam.
  3. Race detector starts complaining on any code path touching the var.

Suggested fix (when actioned)

  • Introduce a `chatSession` struct owning the `Notice func`, `sync.Once`, and `liner.State`.
  • `runChat` instantiates one per invocation; tests construct their own without touching package state.
  • Delete the package-level `chatHistorySuppressionNotice` var.

References

  • cmd/aixgo/cmd/chat.go (search for `chatHistorySuppressionNotice`)
  • Filed during: fix/cli-chat-cleanup review

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions