Skip to content

[test-improver] Improve tests for logger/jsonl_logger package #2915

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lpcox merged 1 commit into from
Mar 31, 2026
Merged

[test-improver] Improve tests for logger/jsonl_logger package #2915

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
178 changes: 174 additions & 4 deletions internal/logger/jsonl_logger_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,12 +438,11 @@ func TestLogRPCMessageJSONLDirectionTypes(t *testing.T) {

// Read and verify
content, err := os.ReadFile(logPath)
if err != nil {
return // File might not exist yet
}
require.NoError(err, "Failed to read log file")

var entry JSONLRPCMessage
json.Unmarshal(content, &entry)
err = json.Unmarshal(content, &entry)
require.NoError(err, "Failed to parse JSONL entry")
Comment on lines 440 to +445
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

In this subtest, require was created with the parent t (outside t.Run) and is used here. If an assertion fails, FailNow would be invoked on the parent test from the subtest goroutine, which can panic or misattribute failures. Instantiate a new require := require.New(t) inside the subtest (or use require.NoError(t, ...) with the subtest t) for these checks.

Copilot uses AI. Check for mistakes.

a.Equal(tt.expected["direction"], entry.Direction, "Direction should match")
a.Equal(tt.expected["type"], entry.Type, "Type should match")
Expand Down Expand Up @@ -621,3 +620,174 @@ func TestLogDifcFilteredItem_MultipleEntriesAuditTrail(t *testing.T) {
assert.NotEmpty(line.Reason, "entry[%d] must have Reason", i)
}
}

// TestLogRPCMessageJSONLWithTags_AgentSecrecyTags verifies that agent secrecy tags
// are written into the JSONL entry when provided, and that integrity is omitted.
func TestLogRPCMessageJSONLWithTags_AgentSecrecyTags(t *testing.T) {
require := require.New(t)
assert := assert.New(t)

tmpDir := t.TempDir()
logDir := filepath.Join(tmpDir, "logs")

err := InitJSONLLogger(logDir, "test.jsonl")
require.NoError(err, "InitJSONLLogger failed")
defer CloseJSONLLogger()

payload := []byte(`{"jsonrpc":"2.0","id":1}`)
secrecyTags := []string{"private:org/repo", "public"}

LogRPCMessageJSONLWithTags(RPCDirectionInbound, RPCMessageResponse, "github", "tools/call", payload, nil, secrecyTags, nil)
Comment on lines +637 to +640
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

The payload literal is a raw string (backticks) but still includes escaped quotes ("), which makes the JSON invalid and forces sanitize.SanitizeJSON down its "invalid JSON" wrapping path. Use a valid JSON payload (remove the backslashes or use a normal quoted string) so this test exercises the intended logging behavior.

This issue also appears in the following locations of the same file:

  • line 668
  • line 699
  • line 734
  • line 771

Copilot uses AI. Check for mistakes.
CloseJSONLLogger()
Comment on lines +635 to +641
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

This test defers CloseJSONLLogger() and also calls CloseJSONLLogger() explicitly. Since CloseJSONLLogger() returns an error, this pattern both double-closes and ignores potential close/flush errors. Prefer a single close path and assert the returned error (e.g., close once at the end with require.NoError).

This issue also appears in the following locations of the same file:

  • line 664
  • line 695
  • line 730
  • line 767
Suggested change
defer CloseJSONLLogger()
payload := []byte(`{"jsonrpc":"2.0","id":1}`)
secrecyTags := []string{"private:org/repo", "public"}
LogRPCMessageJSONLWithTags(RPCDirectionInbound, RPCMessageResponse, "github", "tools/call", payload, nil, secrecyTags, nil)
CloseJSONLLogger()
payload := []byte(`{"jsonrpc":"2.0","id":1}`)
secrecyTags := []string{"private:org/repo", "public"}
LogRPCMessageJSONLWithTags(RPCDirectionInbound, RPCMessageResponse, "github", "tools/call", payload, nil, secrecyTags, nil)
err = CloseJSONLLogger()
require.NoError(err, "CloseJSONLLogger failed")

Copilot uses AI. Check for mistakes.

logPath := filepath.Join(logDir, "test.jsonl")
content, err := os.ReadFile(logPath)
require.NoError(err, "Failed to read log file")

var entry JSONLRPCMessage
err = json.Unmarshal(content, &entry)
require.NoError(err, "Failed to parse JSONL entry")

assert.Equal(secrecyTags, entry.AgentSecrecy, "AgentSecrecy should match provided tags")
assert.Empty(entry.AgentIntegrity, "AgentIntegrity should be absent when not provided")
}

// TestLogRPCMessageJSONLWithTags_AgentIntegrityTags verifies that agent integrity tags
// are written into the JSONL entry when provided, and that secrecy is omitted.
func TestLogRPCMessageJSONLWithTags_AgentIntegrityTags(t *testing.T) {
require := require.New(t)
assert := assert.New(t)

tmpDir := t.TempDir()
logDir := filepath.Join(tmpDir, "logs")

err := InitJSONLLogger(logDir, "test.jsonl")
require.NoError(err, "InitJSONLLogger failed")
defer CloseJSONLLogger()

payload := []byte(`{"jsonrpc":"2.0","id":1}`)
integrityTags := []string{"approved:org/repo", "merged"}

LogRPCMessageJSONLWithTags(RPCDirectionOutbound, RPCMessageRequest, "github", "tools/list", payload, nil, nil, integrityTags)
CloseJSONLLogger()

logPath := filepath.Join(logDir, "test.jsonl")
content, err := os.ReadFile(logPath)
require.NoError(err, "Failed to read log file")

var entry JSONLRPCMessage
err = json.Unmarshal(content, &entry)
require.NoError(err, "Failed to parse JSONL entry")

assert.Empty(entry.AgentSecrecy, "AgentSecrecy should be absent when not provided")
assert.Equal(integrityTags, entry.AgentIntegrity, "AgentIntegrity should match provided tags")
}

// TestLogRPCMessageJSONLWithTags_BothTagTypes verifies that both agent secrecy and
// integrity tags are correctly written when both are provided in the same call.
func TestLogRPCMessageJSONLWithTags_BothTagTypes(t *testing.T) {
require := require.New(t)
assert := assert.New(t)

tmpDir := t.TempDir()
logDir := filepath.Join(tmpDir, "logs")

err := InitJSONLLogger(logDir, "test.jsonl")
require.NoError(err, "InitJSONLLogger failed")
defer CloseJSONLLogger()

payload := []byte(`{"jsonrpc":"2.0","id":2}`)
secrecyTags := []string{"private:org/repo"}
integrityTags := []string{"approved:org/repo", "merged:org/repo"}

LogRPCMessageJSONLWithTags(RPCDirectionInbound, RPCMessageResponse, "github", "tools/call", payload, nil, secrecyTags, integrityTags)
CloseJSONLLogger()

logPath := filepath.Join(logDir, "test.jsonl")
content, err := os.ReadFile(logPath)
require.NoError(err, "Failed to read log file")

var entry JSONLRPCMessage
err = json.Unmarshal(content, &entry)
require.NoError(err, "Failed to parse JSONL entry")

assert.Equal(secrecyTags, entry.AgentSecrecy, "AgentSecrecy should match")
assert.Equal(integrityTags, entry.AgentIntegrity, "AgentIntegrity should match")
assert.Equal("github", entry.ServerID)
assert.Equal("tools/call", entry.Method)
}

// TestLogRPCMessageJSONLWithTags_EmptyTagsOmitted verifies that empty (non-nil) tag
// slices are treated the same as nil — they must NOT appear in the JSON output due to
// the omitempty struct tag on AgentSecrecy and AgentIntegrity.
func TestLogRPCMessageJSONLWithTags_EmptyTagsOmitted(t *testing.T) {
require := require.New(t)
assert := assert.New(t)

tmpDir := t.TempDir()
logDir := filepath.Join(tmpDir, "logs")

err := InitJSONLLogger(logDir, "test.jsonl")
require.NoError(err, "InitJSONLLogger failed")
defer CloseJSONLLogger()

payload := []byte(`{"jsonrpc":"2.0","id":1}`)

// Pass explicitly empty (non-nil) slices.
LogRPCMessageJSONLWithTags(RPCDirectionOutbound, RPCMessageRequest, "github", "tools/list", payload, nil, []string{}, []string{})
CloseJSONLLogger()

logPath := filepath.Join(logDir, "test.jsonl")
content, err := os.ReadFile(logPath)
require.NoError(err, "Failed to read log file")

var entry JSONLRPCMessage
err = json.Unmarshal(content, &entry)
require.NoError(err, "Failed to parse JSONL entry")

// Empty slices must not be stored (len == 0 check in implementation).
assert.Empty(entry.AgentSecrecy, "AgentSecrecy must be absent for empty slice input")
assert.Empty(entry.AgentIntegrity, "AgentIntegrity must be absent for empty slice input")

// Verify the raw JSON does not contain the tag fields at all.
assert.NotContains(string(content), "agent_secrecy", "raw JSON must not contain agent_secrecy key for empty slice")
assert.NotContains(string(content), "agent_integrity", "raw JSON must not contain agent_integrity key for empty slice")
}

// TestLogRPCMessageJSONLWithTags_TagsCopied verifies that the tags stored in the log
// entry are independent copies of the caller's slice. Mutating the original after the
// call must not alter the data that was written to disk.
func TestLogRPCMessageJSONLWithTags_TagsCopied(t *testing.T) {
require := require.New(t)
assert := assert.New(t)

tmpDir := t.TempDir()
logDir := filepath.Join(tmpDir, "logs")

err := InitJSONLLogger(logDir, "test.jsonl")
require.NoError(err, "InitJSONLLogger failed")
defer CloseJSONLLogger()

payload := []byte(`{"jsonrpc":"2.0","id":1}`)
secrecyTags := []string{"private:org/repo"}
integrityTags := []string{"approved:org/repo"}

LogRPCMessageJSONLWithTags(RPCDirectionInbound, RPCMessageResponse, "github", "tools/call", payload, nil, secrecyTags, integrityTags)
CloseJSONLLogger()

// Mutate the originals after the call.
secrecyTags[0] = "MUTATED"
integrityTags[0] = "MUTATED"

logPath := filepath.Join(logDir, "test.jsonl")
content, err := os.ReadFile(logPath)
require.NoError(err, "Failed to read log file")

var entry JSONLRPCMessage
err = json.Unmarshal(content, &entry)
require.NoError(err, "Failed to parse JSONL entry")

// The logged values must reflect the originals at call time, not the mutation.
assert.Equal([]string{"private:org/repo"}, entry.AgentSecrecy, "AgentSecrecy must be an independent copy")
assert.Equal([]string{"approved:org/repo"}, entry.AgentIntegrity, "AgentIntegrity must be an independent copy")
}
Comment on lines +760 to +793
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

TestLogRPCMessageJSONLWithTags_TagsCopied doesn't currently prove that the implementation copies the tag slices: LogRPCMessageJSONLWithTags encodes the entry synchronously before returning, so mutating the caller slice after the call cannot affect the already-written file even without a defensive copy. Either remove this test or rework it to mutate concurrently while logging (and/or validate behavior under -race) so it would fail if the copy is removed.

Suggested change
func TestLogRPCMessageJSONLWithTags_TagsCopied(t *testing.T) {
require := require.New(t)
assert := assert.New(t)
tmpDir := t.TempDir()
logDir := filepath.Join(tmpDir, "logs")
err := InitJSONLLogger(logDir, "test.jsonl")
require.NoError(err, "InitJSONLLogger failed")
defer CloseJSONLLogger()
payload := []byte(`{"jsonrpc":"2.0","id":1}`)
secrecyTags := []string{"private:org/repo"}
integrityTags := []string{"approved:org/repo"}
LogRPCMessageJSONLWithTags(RPCDirectionInbound, RPCMessageResponse, "github", "tools/call", payload, nil, secrecyTags, integrityTags)
CloseJSONLLogger()
// Mutate the originals after the call.
secrecyTags[0] = "MUTATED"
integrityTags[0] = "MUTATED"
logPath := filepath.Join(logDir, "test.jsonl")
content, err := os.ReadFile(logPath)
require.NoError(err, "Failed to read log file")
var entry JSONLRPCMessage
err = json.Unmarshal(content, &entry)
require.NoError(err, "Failed to parse JSONL entry")
// The logged values must reflect the originals at call time, not the mutation.
assert.Equal([]string{"private:org/repo"}, entry.AgentSecrecy, "AgentSecrecy must be an independent copy")
assert.Equal([]string{"approved:org/repo"}, entry.AgentIntegrity, "AgentIntegrity must be an independent copy")
}

Copilot uses AI. Check for mistakes.
Loading