Skip to content

test: split e2e tests and add go-zero integration tests#11

Merged
spencercjh merged 8 commits intomainfrom
test/go-zero-e2e
Mar 7, 2026
Merged

test: split e2e tests and add go-zero integration tests#11
spencercjh merged 8 commits intomainfrom
test/go-zero-e2e

Conversation

@spencercjh
Copy link
Copy Markdown
Owner

Summary

  • Split e2e_test.go into framework-specific test files for better organization
  • Add go-zero framework integration tests

Test Plan

  • Run e2e tests for go-zero
  • Run e2e tests for Spring Boot (Maven/Gradle)
  • Verify error handling tests

🤖 Generated with Claude Code

- spring_maven_test.go: Maven Spring Boot tests
- spring_gradle_test.go: Gradle Spring Boot tests
- gozero_test.go: Go-zero e2e tests
- mock_provider_test.go: Shared mock provider
- error_test.go: Error handling tests
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

Signed-off-by: spencercjh <spencercjh@gmail.com>
Copilot AI review requested due to automatic review settings March 7, 2026 17:39
@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Split e2e tests into framework-specific files and add go-zero tests

🧪 Tests

Grey Divider

Walkthroughs

Description
• Split monolithic e2e test file into framework-specific test files
• Added go-zero framework integration tests with detection and generation
• Extracted error handling and mock provider into separate test files
• Improved test organization and maintainability
Diagram
flowchart LR
  A["e2e_test.go<br/>monolithic"] -->|split| B["spring_maven_test.go"]
  A -->|split| C["spring_gradle_test.go"]
  A -->|split| D["gozero_test.go"]
  A -->|split| E["error_test.go"]
  A -->|split| F["mock_provider_test.go"]
  B -->|Maven tests| G["Framework-specific<br/>test suite"]
  C -->|Gradle tests| G
  D -->|Go-zero tests| G
  E -->|Error handling| G
  F -->|Shared utilities| G
Loading

Grey Divider

File Changes

1. integration-tests/spring_maven_test.go Refactoring +0/-109

Cleaned up Maven-specific tests

• Removed Gradle Spring Boot test (moved to separate file)
• Removed error handling test (moved to error_test.go)
• Removed mock provider implementation (moved to mock_provider_test.go)
• Removed unused imports for executor and provider packages

integration-tests/spring_maven_test.go


2. integration-tests/spring_gradle_test.go 🧪 Tests +67/-0

New Gradle Spring Boot e2e tests

• New file containing Gradle Spring Boot project tests
• Implements TestE2E_GradleSpringBoot_Generate for Gradle wrapper detection
• Tests project detection, spec generation, and validation workflow
• Uses 5-minute timeout for Gradle build operations

integration-tests/spring_gradle_test.go


3. integration-tests/gozero_test.go 🧪 Tests +177/-0

New go-zero framework e2e tests

• New file with comprehensive go-zero framework integration tests
• Implements TestE2E_GoZero_Generate for full workflow testing
• Implements TestE2E_GoZero_Detect for project detection verification
• Implements TestE2E_GoZero_NoGoctl for graceful error handling

integration-tests/gozero_test.go


View more (2)
4. integration-tests/error_test.go 🧪 Tests +47/-0

New error handling e2e tests

• New file dedicated to error handling tests
• Implements TestE2E_ErrorHandling_CommandNotFound test
• Tests executor behavior when command is not found
• Verifies CommandNotFoundError type assertion

integration-tests/error_test.go


5. integration-tests/mock_provider_test.go 🧪 Tests +29/-0

New shared mock provider utility

• New file containing shared mock provider implementation
• Defines countingMockProvider struct for tracking LLM call counts
• Implements provider.Provider interface for test usage
• Provides configurable mock responses for enrichment testing

integration-tests/mock_provider_test.go


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented Mar 7, 2026

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

1. HasGoctl field missing🐞 Bug ✓ Correctness
Description
integration-tests/gozero_test.go references patchResult.HasGoctl, but gozero.PatchResult defines
GoctlAvailable instead. This is a compile-time error that prevents e2e tests from building.
Code

integration-tests/gozero_test.go[R63-72]

+	// Step 2: Patch (verify goctl availability)
+	patcher := gozero.NewPatcher()
+	patchResult, err := patcher.Patch(projectPath)
+	if err != nil {
+		t.Fatalf("Failed to patch project: %v", err)
+	}
+
+	if !patchResult.HasGoctl {
+		t.Skip("goctl not found in PATH, skipping generation test")
+	}
Evidence
The new test expects PatchResult.HasGoctl, but the implementation exposes PatchResult.GoctlAvailable
(no HasGoctl field), so the test cannot compile.

integration-tests/gozero_test.go[63-74]
internal/extractor/gozero/patcher.go[34-38]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`integration-tests/gozero_test.go` uses `patchResult.HasGoctl`, but `gozero.PatchResult` exposes `GoctlAvailable`. This causes a compilation failure under the `e2e` build tag.
## Issue Context
The patcher implementation (`internal/extractor/gozero/patcher.go`) defines:
- `PatchResult.GoctlAvailable`
- `PatchResult.GoctlVersion`
## Fix Focus Areas
- integration-tests/gozero_test.go[63-75]
- integration-tests/gozero_test.go[148-157]
- internal/extractor/gozero/patcher.go[34-38]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. No-goctl path mishandled🐞 Bug ⛯ Reliability
Description
The new go-zero e2e tests call patcher.Patch() and t.Fatalf on error, but patcher.Patch() returns
an error (nil result) when goctl is not installed. This makes the tests fail in environments without
goctl rather than skipping or asserting the expected generator error.
Code

integration-tests/gozero_test.go[R148-153]

+	// Check if goctl is available
+	patcher := gozero.NewPatcher()
+	patchResult, err := patcher.Patch(projectPath)
+	if err != nil {
+		t.Fatalf("Failed to check goctl: %v", err)
+	}
Evidence
Patcher.Patch returns nil, error when goctl isn’t found, but the tests treat any Patch error as
fatal. Also, detector already computes gozero.Info.HasGoctl, which is what generator checks to
decide whether to error.

integration-tests/gozero_test.go[148-157]
internal/extractor/gozero/patcher.go[52-68]
internal/extractor/gozero/detector.go[92-100]
internal/extractor/gozero/generator.go[148-152]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`gozero_test.go` assumes `patcher.Patch()` is a non-failing probe for goctl availability, but `Patch()` returns an error when goctl is missing. This makes e2e tests fail on machines without goctl, instead of skipping or validating the intended error behavior.
## Issue Context
- Detector already sets `gozero.Info.HasGoctl`.
- Generator checks `info.HasGoctl` and returns a clear error if false.
## Fix Focus Areas
- integration-tests/gozero_test.go[63-75]
- integration-tests/gozero_test.go[140-176]
- internal/extractor/gozero/patcher.go[43-68]
- internal/extractor/gozero/detector.go[92-100]
- internal/extractor/gozero/generator.go[148-152]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

3. Type check doesn’t fail🐞 Bug ✓ Correctness
Description
The new error-handling e2e test intends to verify CommandNotFoundError, but it only logs when the
type assertion fails. This can let regressions pass undetected.
Code

integration-tests/error_test.go[R43-46]

+	// Verify it's a CommandNotFoundError
+	if _, ok := errors.AsType[*executor.CommandNotFoundError](err); !ok {
+		t.Logf("Got error type %T: %v", err, err)
+	}
Evidence
The executor returns a specific CommandNotFoundError for missing commands, but the test doesn’t fail
if it receives a different type, reducing test effectiveness.

integration-tests/error_test.go[43-46]
internal/executor/executor.go[103-110]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The e2e error handling test logs when the returned error is not `*executor.CommandNotFoundError`, but does not fail. This makes the test pass even if the executor stops returning the expected error type.
## Issue Context
`executor.Execute` explicitly returns `*executor.CommandNotFoundError` when a command is not found.
## Fix Focus Areas
- integration-tests/error_test.go[43-46]
- internal/executor/executor.go[103-110]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Signed-off-by: spencercjh <spencercjh@gmail.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR reorganizes the integration-tests e2e suite by splitting Spring tests into separate Maven/Gradle files and introducing go-zero integration coverage.

Changes:

  • Split Spring Boot e2e tests into Maven- and Gradle-specific test files.
  • Add go-zero e2e tests covering detection, generation, and “no goctl” behavior.
  • Extract the mock LLM provider into a shared test helper file.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
integration-tests/spring_maven_test.go Removes Gradle + error-handling + mock provider code from Maven Spring test file after split.
integration-tests/spring_gradle_test.go New Gradle-specific Spring Boot generation/validation e2e test.
integration-tests/mock_provider_test.go New shared mock provider used by enrich flow tests.
integration-tests/gozero_test.go New go-zero e2e tests (detect/generate/no-goctl).
integration-tests/error_test.go New dedicated error-handling e2e test for missing command execution.

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

Comment thread integration-tests/gozero_test.go
Comment thread integration-tests/gozero_test.go Outdated
Comment thread integration-tests/gozero_test.go
Comment thread integration-tests/error_test.go Outdated
Comment thread integration-tests/gozero_test.go
Comment thread integration-tests/gozero_test.go
Signed-off-by: spencercjh <spencercjh@gmail.com>
@spencercjh spencercjh self-assigned this Mar 7, 2026
Signed-off-by: spencercjh <spencercjh@gmail.com>
Copilot AI review requested due to automatic review settings March 7, 2026 17:51
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 10 changed files in this pull request and generated 2 comments.


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

Comment thread integration-tests/README.md Outdated
Comment thread integration-tests/error_test.go Outdated
spencercjh and others added 2 commits March 8, 2026 01:56
Signed-off-by: spencercjh <spencercjh@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 7, 2026 17:58
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 11 changed files in this pull request and generated 5 comments.


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

Comment thread integration-tests/gozero_test.go
Comment thread integration-tests/gozero_test.go
Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/ci.yml
Comment thread integration-tests/README.md
- error_test.go: fail test on wrong error type instead of just logging
- error_test.go: remove unused targetDir variable
- README.md: add prerequisites for Java 25 and goctl installation

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: spencercjh <spencercjh@gmail.com>
Copilot AI review requested due to automatic review settings March 7, 2026 18:14
Signed-off-by: spencercjh <spencercjh@gmail.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 11 changed files in this pull request and generated 4 comments.


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

Comment on lines +78 to +79
result, err := gen.Generate(ctx, projectPath, info, &extractor.GenerateOptions{
Format: "yaml",
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

This e2e test writes openapi.yaml into the checked-in demo project directory (hence the added .gitignore entries). To avoid polluting the working tree and to make runs more hermetic, set GenerateOptions.OutputDir (and optionally OutputFile) to a t.TempDir() location.

Suggested change
result, err := gen.Generate(ctx, projectPath, info, &extractor.GenerateOptions{
Format: "yaml",
outputDir := t.TempDir()
result, err := gen.Generate(ctx, projectPath, info, &extractor.GenerateOptions{
Format: "yaml",
OutputDir: outputDir,
OutputFile: "openapi.yaml",

Copilot uses AI. Check for mistakes.
Comment thread .github/workflows/ci.yml
Comment on lines +93 to +95
- name: Set up goctl
run: go install github.com/zeromicro/go-zero/tools/goctl@latest

Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

Installing goctl@latest makes CI non-reproducible (a new goctl release can break the pipeline without code changes). Consider pinning to a specific version (ideally matching the go-zero version used in integration-tests/gozero-demo/go.mod) or centralizing the version in a workflow/env var.

Copilot uses AI. Check for mistakes.
Comment on lines +28 to 32
- **goctl** - Required for go-zero projects. Install with:
```bash
go install github.com/zeromicro/go-zero/tools/goctl@latest
```

Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

The docs recommend go install .../goctl@latest, which is convenient but can lead to non-reproducible e2e runs when goctl behavior changes. Consider documenting a pinned goctl version (matching the demo project's go-zero dependency) or at least noting that @latest may cause test drift over time.

Suggested change
- **goctl** - Required for go-zero projects. Install with:
```bash
go install github.com/zeromicro/go-zero/tools/goctl@latest
```
- **goctl** - Required for go-zero projects. Install with a version pinned to the go-zero dependency used in the `gozero-demo` project, for example:
```bash
go install github.com/zeromicro/go-zero/tools/goctl@vX.Y.Z

Note: Using @latest can change goctl behavior over time and may cause end-to-end tests to drift.

Copilot uses AI. Check for mistakes.
go.opentelemetry.io/otel/trace v1.24.0 // indirect
go.opentelemetry.io/proto/otlp v1.3.1 // indirect
go.uber.org/automaxprocs v1.6.0 // indirect
go.yaml.in/yaml/v2 v2.4.2 // indirect
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

The require entry on go.yaml.in/yaml/v2 introduces a likely typosquatted and potentially malicious replacement for the legitimate gopkg.in/yaml.v2 module, creating a supply-chain risk. Including this module in go.mod allows it to be fetched and built in developer or CI environments, where it could run arbitrary code with access to secrets. Remove this requirement and, if YAML parsing is needed, depend on the official gopkg.in/yaml.v2 module instead.

Copilot uses AI. Check for mistakes.
@spencercjh spencercjh merged commit 8554511 into main Mar 7, 2026
3 checks passed
@spencercjh spencercjh deleted the test/go-zero-e2e branch March 7, 2026 18:18
spencercjh added a commit that referenced this pull request Mar 30, 2026
Review feedback addressed:
- CustomProvider: use p.name instead of hardcoded CustomProviderName in error messages
- StreamWriter: optimize bytes.Contains to bytes.IndexByte for hot path
- E2E tests: handle enrich.enabled=false config, pass --custom-api-key-env for custom provider
- Design doc: fix StreamWriter file path, update Stream to *bool type, correct prefix casing

Fixes review comments #11, #12, #13, #16, #17 on PR #57

Signed-off-by: caijiahao <caijh@inesa.com>
Signed-off-by: spencercjh <spencercjh@gmail.com>
spencercjh added a commit that referenced this pull request Mar 30, 2026
* docs: add Phase 4.1 Streaming design document

Design P4.1: Real-time streaming support for LLM enrichment

Key features:
- Option pattern for Provider interface (WithStreamingFunc)
- StreamWriter for thread-safe output with batch prefix
- Concurrent processing with mutex-protected output
- CLI flag --no-stream to disable streaming (enabled by default)

Signed-off-by: spencercjh <spencercjh@gmail.com>

* docs: add P4.1 Streaming implementation plan

8 tasks with TDD approach:
1. Provider interface refactoring (Option pattern)
2. StreamWriter implementation
3. Provider streaming implementation
4. BatchProcessor streaming integration
5. Enricher streaming support
6. CLI integration (--no-stream flag)
7. Integration testing
8. Final verification

Signed-off-by: spencercjh <spencercjh@gmail.com>

* feat(provider): add Option pattern for streaming support

- Add GenerateOptions, Option, WithStreamingFunc, applyOptions
- Update Provider interface to accept ...Option
- Update all provider implementations with new signature (streaming logic in next commit)

Signed-off-by: spencercjh <spencercjh@gmail.com>

* feat(processor): add StreamWriter for thread-safe streaming output

- StreamWriter with mutex-protected writes
- WriteWithPrefix adds batch type prefix
- Tested for concurrent access safety

Signed-off-by: spencercjh <spencercjh@gmail.com>

* feat(provider): add streaming support to all providers

- Replace GenerateFromSinglePrompt with GenerateContent
- Pass StreamingFunc to langchaingo via llms.WithStreamingFunc
- Return content from response.Choices for non-streaming callers

Signed-off-by: spencercjh <spencercjh@gmail.com>

* feat(processor): integrate streaming into BatchProcessor

- Add StreamWriter field and WithStreamWriter option
- Pass streaming callback to provider with batch type prefix
- Update NewBatchProcessor to accept functional options

Task 4 of P4.1 streaming implementation

Signed-off-by: spencercjh <spencercjh@gmail.com>

* feat(enricher): add Stream option to EnrichOptions

- Add Stream (bool) and Writer (io.Writer) to EnrichOptions
- Create StreamWriter when streaming is enabled
- Default: streaming enabled to os.Stdout

Signed-off-by: spencercjh <spencercjh@gmail.com>

* feat(cli): add --no-stream flag to enrich command

- Streaming enabled by default
- Use --no-stream to disable and wait for complete response

Signed-off-by: spencercjh <spencercjh@gmail.com>

* test(enricher): add streaming integration test

- Test that streaming callback is invoked
- Test that StreamWriter output contains batch prefix [api]

Signed-off-by: spencercjh <spencercjh@gmail.com>

* style(enricher): fix whitespace alignment

Signed-off-by: spencercjh <spencercjh@gmail.com>

* fix(batch): update streaming function to ignore context parameter

Signed-off-by: spencercjh <spencercjh@gmail.com>

* fix(processor): add nil check to NewStreamWriter

Prevent runtime panic by validating writer parameter is not nil.
Add test to verify panic behavior with nil writer.

Signed-off-by: spencercjh <spencercjh@gmail.com>

* test(enricher): add test for Stream: false path

Add TestEnricher_WithStreamingDisabled to verify that no streaming
callback is passed to the provider when Stream option is disabled.

Signed-off-by: spencercjh <spencercjh@gmail.com>

* test(processor): add streaming callback verification in batch tests

Add TestBatchProcessor_ProcessBatch_WithStreaming to verify that
streaming callback is properly invoked when StreamWriter is configured.

Signed-off-by: spencercjh <spencercjh@gmail.com>

* feat(processor): add chunk buffering to StreamWriter

- Add buffer accumulation for streaming chunks
- Auto-flush on newline, buffer threshold, or prefix change
- Add WithFlushThreshold option for configurable buffering
- Add Flush method for explicit buffer clearing
- Update enricher to flush StreamWriter after processing
- Add tests for buffering behavior

Signed-off-by: spencercjh <spencercjh@gmail.com>

* feat(processor): add streaming debug logging and metrics

- Add StreamWriterMetrics struct to track streaming statistics
- Add GetMetrics() method for retrieving metrics
- Add WithDebug option for enabling debug logging
- Add WithFlushThreshold option for configurable buffering
- Track total chunks, bytes, flushes, and unique prefixes
- Debug logging shows chunk details and flush events

Signed-off-by: spencercjh <spencercjh@gmail.com>

* style: fix formatting issues from golangci-lint

Signed-off-by: spencercjh <spencercjh@gmail.com>

* docs: clarify make verify usage in CLAUDE.md

Explain that make verify checks for uncommitted changes and should only
be used after committing. Recommend individual commands (make fmt, lint,
test) before committing to avoid false failures from git diff check.

Signed-off-by: spencercjh <spencercjh@gmail.com>

* test(e2e): add conditional enrich E2E tests with streaming verification

- Add skipIfNoConfig helper to skip tests without valid E2E config
- Check for .spec-forge.e2e.local.yaml or .spec-forge.local.yaml
- Verify API key environment variable is set before running
- Test streaming output with prefix verification ([api], [schema], [param])
- Test --no-stream flag disables streaming prefixes
- Test local config file loading mechanism
- Add .spec-forge.e2e.example.yaml as configuration template
- Update .gitignore to exclude E2E local configs

Signed-off-by: spencercjh <spencercjh@gmail.com>

* test(e2e): add conditional enrich E2E tests with streaming verification

- Add loadE2EConfig helper to load config from integration-tests/.spec-forge.e2e.local.yaml
- Skip tests gracefully if no valid config found (file missing or API key env not set)
- Add TestE2E_Enrich_NoStreamFlag to verify --no-stream disables streaming prefixes
- Add TestE2E_Enrich_WithStreaming to test real LLM enrichment with streaming output
- Add TestE2E_Enrich_WithLocalConfig to test local config file loading
- Add .spec-forge.e2e.example.yaml as configuration template
- Update .gitignore to exclude integration-tests/.spec-forge.e2e.local.yaml
- Document E2E enrich test setup in integration-tests/README.md

Signed-off-by: spencercjh <spencercjh@gmail.com>

* test(e2e): fix enrich E2E tests for streaming verification

- Remove streaming prefix check in stdout (StreamWriter writes to os.Stdout directly, not Cobra's buffer)
- Remove TestE2E_Enrich_WithLocalConfig (tests Viper config loading, not enrich logic)
- Verify enrichment by checking the output spec file contains descriptions
- Update README to document only the two streaming-related tests
- All tests pass:
  - TestE2E_Enrich_Help: PASS
  - TestE2E_Enrich_MissingArgs: PASS
  - TestE2E_Enrich_NonExistentFile: PASS
  - TestE2E_Enrich_NoStreamFlag: SKIP (requires config)
  - TestE2E_Enrich_WithStreaming: PASS (with config) / SKIP (without config)

Signed-off-by: spencercjh <spencercjh@gmail.com>

* fix(provider): satisfy goconst in anthropic and custom providers

Agent-Logs-Url: https://github.com/spencercjh/spec-forge/sessions/be20b0d1-0dc2-45c4-adc4-b2bec21a9a95

Co-authored-by: spencercjh <29922079+spencercjh@users.noreply.github.com>

* fix(review): address PR review feedback for P4.1 streaming

- Change EnrichOptions.Stream to *bool (tri-state) to avoid backwards
  compatibility issues with zero value of bool (false). Now nil means
  use default (true), explicit false/true overrides.
- Add error return when LLM providers return empty choices to prevent
  silent enrichment failures (OpenAI, Anthropic, Ollama, Custom).
- Fix comment in batch.go to reflect lowercase prefix values.
- Remove stale TestE2E_Enrich_WithLocalConfig entry from README table.
- Make E2E test assertion language-agnostic (check for non-empty
  descriptions instead of specific Chinese text).

Signed-off-by: spencercjh <spencercjh@gmail.com>

* fix(review): address additional PR review feedback for P4.1 streaming

Review feedback addressed:
- CustomProvider: use p.name instead of hardcoded CustomProviderName in error messages
- StreamWriter: optimize bytes.Contains to bytes.IndexByte for hot path
- E2E tests: handle enrich.enabled=false config, pass --custom-api-key-env for custom provider
- Design doc: fix StreamWriter file path, update Stream to *bool type, correct prefix casing

Fixes review comments #11, #12, #13, #16, #17 on PR #57

Signed-off-by: caijiahao <caijh@inesa.com>
Signed-off-by: spencercjh <spencercjh@gmail.com>

* fix(review): address third round of Copilot review feedback for P4.1

Review feedback addressed:
- factory.go: use AnthropicProviderName and CustomProviderName constants instead of string literals
- design doc: update EnrichOptions summary to reflect actual API (Stream *bool, Writer io.Writer)
- design doc: fix data flow example to use lowercase prefix [api] instead of [API]
- stream_writer.go: validate WithFlushThreshold to use DefaultFlushThreshold for zero/negative values
- e2e_enrich_test.go: pass --timeout flag when configured in E2E config

Fixes review comments #21-26 on PR #57

Signed-off-by: caijiahao <caijh@inesa.com>
Signed-off-by: spencercjh <spencercjh@gmail.com>

* fix(review): address fourth round of Copilot review feedback

Review feedback addressed:
- e2e_enrich_test.go: change Enabled to *bool to distinguish between
  "not set" (nil, run tests) and "explicitly false" (skip tests)
- README.md: update skip message path to match actual output
- batch.go: add Flush() call after each LLM call to ensure buffered
  streaming output is visible for short responses

Note: #30 (streaming to os.Stdout) is intentional design for real-time
terminal feedback and not changed.

Fixes review comments #27-29 on PR #57

Signed-off-by: caijiahao <caijh@inesa.com>
Signed-off-by: spencercjh <spencercjh@gmail.com>

* fix(review): address fifth round of Copilot review feedback

Review feedback addressed:
- e2e_enrich_test.go: match API key env var logic to CLI behavior
  (OPENAI_API_KEY, ANTHROPIC_API_KEY, or LLM_API_KEY for custom)
- e2e_enrich_test.go: use YAML parsing for provider-agnostic assertions
  instead of language-specific substring matching
- design doc: update output example to show actual raw JSON streaming
  behavior instead of hypothetical human-readable text

Note: #32 (streaming writes to os.Stdout) is intentional design for
real-time terminal feedback - already addressed in #30 response.

Fixes review comments #31, #33, #34 on PR #57

Signed-off-by: caijiahao <caijh@inesa.com>
Signed-off-by: spencercjh <spencercjh@gmail.com>

* fix(enricher,stream_writer): improve error handling and metrics updates

Signed-off-by: spencercjh <spencercjh@gmail.com>

* feat(batch): add streaming support and improve processing logic

Signed-off-by: spencercjh <spencercjh@gmail.com>

* fix(review): address Copilot review feedback - streaming, concurrency, docs

- Remove redundant atomic ops in StreamWriter, use mutex-only sync
- Handle short writes in flushLocked()
- Change final Flush error to warning (streaming is ancillary)
- Streaming mode processes batches sequentially for readable output
- --no-stream enables concurrent processing (--concurrency applies)
- Update flag descriptions, config comments, design doc, CLAUDE.md

Signed-off-by: spencercjh <spencercjh@gmail.com>

---------

Signed-off-by: spencercjh <spencercjh@gmail.com>
Signed-off-by: caijiahao <caijh@inesa.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: spencercjh <29922079+spencercjh@users.noreply.github.com>
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