[test-improver] Improve tests for logger/jsonl_logger package by github-actions[bot] · Pull Request #2915 · github/gh-aw-mcpg

github-actions · 2026-03-31T11:49:38Z

File Analyzed

Test File: internal/logger/jsonl_logger_test.go
Package: internal/logger
Lines of Code: 623 → 793 (+170)

Improvements Made

1. Increased Coverage — `LogRPCMessageJSONLWithTags`

LogRPCMessageJSONLWithTags is the core function responsible for writing RPC messages with agent secrecy/integrity tag snapshots to the JSONL audit log. It was never tested directly: the existing tests only called LogRPCMessageJSONL, which delegates to LogRPCMessageJSONLWithTags with both tag arguments as nil. The agent-tag code paths were completely uncovered.

Five new tests are added:

Test	What it covers
`TestLogRPCMessageJSONLWithTags_AgentSecrecyTags`	Secrecy tags appear in JSON; integrity absent
`TestLogRPCMessageJSONLWithTags_AgentIntegrityTags`	Integrity tags appear in JSON; secrecy absent
`TestLogRPCMessageJSONLWithTags_BothTagTypes`	Both tag types written correctly in the same entry
`TestLogRPCMessageJSONLWithTags_EmptyTagsOmitted`	Empty (non-nil) slices trigger `len == 0` guard; `agent_secrecy`/`agent_integrity` keys must not appear in raw JSON (`omitempty`)
`TestLogRPCMessageJSONLWithTags_TagsCopied`	Defensive `append([]string(nil), ...)` copy: mutating the caller's slice after the call must not corrupt the on-disk log entry

2. Fixed Silent Error-Swallowing in `TestLogRPCMessageJSONLDirectionTypes`

The existing subtest loop contained two issues that would let test failures pass silently:

// Before — errors silently ignored
content, err := os.ReadFile(logPath)
if err != nil {
    return  // ← silently skips assertions on read failure
}
var entry JSONLRPCMessage
json.Unmarshal(content, &entry)  // ← unmarshal error discarded

Fixed to use proper require.NoError assertions:

// After — failures are surfaced
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")

Why These Changes?

LogRPCMessageJSONLWithTags is the authoritative function for attaching agent label snapshots to the RPC audit trail — a key part of the DIFC security model. Yet it had zero direct test coverage for its tag-handling branches. Any regression in tag serialization (e.g., wrong field name, omitempty misapplication, forgotten copy) would go undetected.

The silent-return pattern in the direction-types test was a secondary quality issue: a read failure or JSON parse error would cause assertions to silently pass rather than fail the test.

Generated by Test Improver Workflow
Focuses on better patterns, increased coverage, and more stable tests

Generated by Test Improver · ◷

… coverage - Add 5 new tests covering LogRPCMessageJSONLWithTags which was previously untested (only LogRPCMessageJSONL with nil tags was exercised): - TestLogRPCMessageJSONLWithTags_AgentSecrecyTags: secrecy-only tags - TestLogRPCMessageJSONLWithTags_AgentIntegrityTags: integrity-only tags - TestLogRPCMessageJSONLWithTags_BothTagTypes: both tag types together - TestLogRPCMessageJSONLWithTags_EmptyTagsOmitted: empty slices must not produce agent_secrecy/agent_integrity JSON fields (omitempty) - TestLogRPCMessageJSONLWithTags_TagsCopied: defensive copy semantics - Fix TestLogRPCMessageJSONLDirectionTypes: replace silent error-swallowing (bare os.ReadFile check with return and bare json.Unmarshal) with proper require.NoError assertions for test correctness Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

This PR improves test coverage and correctness for the internal/logger JSONL audit logger, especially around agent secrecy/integrity tag serialization for RPC message logging.

Changes:

Adds new tests that directly exercise LogRPCMessageJSONLWithTags for secrecy-only, integrity-only, both, and empty-tag omission cases.
Fixes silent error-swallowing in TestLogRPCMessageJSONLDirectionTypes by asserting on file read and JSON unmarshal errors.

Comments suppressed due to low confidence (8)

internal/logger/jsonl_logger_test.go:673

This test defers CloseJSONLLogger() and also calls CloseJSONLLogger() explicitly, and the close error is ignored. Prefer one close (and assert its error) to avoid double-close behavior and to surface flush/write failures.

	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()

internal/logger/jsonl_logger_test.go:671

The payload raw string includes escaped quotes ("..."), which is not valid JSON. This causes sanitize.SanitizeJSON to wrap the payload as an error object instead of preserving the JSON. Use a valid JSON payload in the test data.

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

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

internal/logger/jsonl_logger_test.go:705

This test defers CloseJSONLLogger() but also calls it explicitly and ignores the returned error. Close once and assert the error so the test fails if the logger cannot flush/sync.

	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()

internal/logger/jsonl_logger_test.go:703

The payload raw string is written as \"-escaped JSON inside backticks, which is invalid JSON input. Using valid JSON here avoids unintentionally testing the sanitize "invalid JSON" fallback path.

	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)

internal/logger/jsonl_logger_test.go:739

This test defers CloseJSONLLogger() and also closes explicitly without checking the returned error. Prefer a single CloseJSONLLogger() call and assert its error to ensure the log entry is flushed and written successfully.

	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()

internal/logger/jsonl_logger_test.go:738

The payload raw string includes escaped quotes ("), which makes it invalid JSON and changes what gets logged in payload. Use a valid JSON literal for the payload to keep the test focused on tag serialization.

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

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

internal/logger/jsonl_logger_test.go:777

This test defers CloseJSONLLogger() and also closes explicitly while ignoring the returned error. Close once and assert the error so failures to sync/write the file don't get hidden.

	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()

internal/logger/jsonl_logger_test.go:776

The payload raw string is not valid JSON because it contains \"-escaped quotes inside a backtick literal. Use a valid JSON payload to avoid triggering the sanitize invalid-JSON wrapper behavior.

	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()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-31T18:10:35Z

internal/logger/jsonl_logger_test.go

 			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")


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 · 2026-03-31T18:10:35Z

internal/logger/jsonl_logger_test.go

+	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()


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 · 2026-03-31T18:10:35Z

internal/logger/jsonl_logger_test.go

+	payload := []byte(`{"jsonrpc":"2.0","id":1}`)
+	secrecyTags := []string{"private:org/repo", "public"}
+
+	LogRPCMessageJSONLWithTags(RPCDirectionInbound, RPCMessageResponse, "github", "tools/call", payload, nil, secrecyTags, nil)


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 · 2026-03-31T18:10:36Z

internal/logger/jsonl_logger_test.go

+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")
+}


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")

}

github-actions bot added automation improvement testing labels Mar 31, 2026

lpcox marked this pull request as ready for review March 31, 2026 18:06

Copilot AI review requested due to automatic review settings March 31, 2026 18:06

lpcox merged commit 006f1cc into main Mar 31, 2026
3 checks passed

lpcox deleted the test-improver/jsonl-logger-coverage-0eddd3fafbe4db87 branch March 31, 2026 18:06

Copilot started reviewing on behalf of lpcox March 31, 2026 18:07 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

This was referenced Mar 31, 2026

Smoke Test: Copilot - 23812425834 #2942

Closed

Smoke Test: Copilot - 23813405954 #2944

Closed

[ca] Update CLI versions: MCP Gateway v0.2.11, Playwright Browser v1.59.0, Playwright MCP 0.0.70 github/gh-aw#23820

Closed

Copilot AI mentioned this pull request Apr 1, 2026

feat: bump MCP Gateway v0.2.11, Playwright Browser v1.59.0, Playwright MCP 0.0.70, Firewall v0.25.6 github/gh-aw#23821

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

[test-improver] Improve tests for logger/jsonl_logger package#2915
lpcox merged 1 commit intomainfrom
test-improver/jsonl-logger-coverage-0eddd3fafbe4db87

github-actions bot commented Mar 31, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

github-actions bot commented Mar 31, 2026

File Analyzed

Improvements Made

1. Increased Coverage — LogRPCMessageJSONLWithTags

2. Fixed Silent Error-Swallowing in TestLogRPCMessageJSONLDirectionTypes

Why These Changes?

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

1. Increased Coverage — `LogRPCMessageJSONLWithTags`

2. Fixed Silent Error-Swallowing in `TestLogRPCMessageJSONLDirectionTypes`