feat(#82): Enhance refactoring logs and add error handling in Critic agent (#107)

Author: volodya-lombrozo

internal/client/refrax_client.go

@@ -138,7 +138,7 @@ func (c *RefraxClient) Refactor(proj domain.Project) (domain.Project, error) {
 	<-rvwr.Ready()
 
 	log.Info("all servers are ready: facilitator %d, critic %d, fixer %d, reviewer %d", facilitatorPort, criticPort, fixerPort, reviewerPort)
-	log.Info("begin refactoring")
+	log.Info("begin refactoring for project %s with %d classes", proj, len(classes))
 	ch := make(chan refactoring, len(classes))
 	go refactor(fclttor, proj, c.params.MaxSize, ch)
 	for range len(classes) {
@@ -184,6 +184,7 @@ func refactor(f domain.Facilitator, p domain.Project, size int, ch chan<- refact
 	}
 	artifacts, err := f.Refactor(&job)
 	if err != nil {
+		log.Error("failed to refactor project %s: %v", p, err)
 		ch <- refactoring{err: fmt.Errorf("failed to refactor project %s: %w", p, err)}
 		close(ch)
 		return

internal/client/refrax_client_test.go

@@ -36,7 +36,7 @@ func TestRefraxClient_PrintsStatsIfEnabled(t *testing.T) {
 	out := bytes.Buffer{}
 	params.Log = NewSyncWriter(io.Writer(&out))
 	client := NewRefraxClient(params)
-	_, err := client.Refactor(domain.NewInMemory(domain.NewClass("Foo.java", ".", "abstract class Foo {}")))
+	_, err := client.Refactor(domain.NewInMemory(domain.NewInMemoryClass("Foo.java", ".", "abstract class Foo {}")))
 	assert.NoError(t, err)
 	assert.Contains(t, out.String(), "Total LLM messages asked", "Expected total messages asked to be logged")
 }

internal/critic/agent.go

@@ -0,0 +1,98 @@
+package critic
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/cqfn/refrax/internal/brain"
+	"github.com/cqfn/refrax/internal/domain"
+	"github.com/cqfn/refrax/internal/log"
+	"github.com/cqfn/refrax/internal/tool"
+)
+
+type agent struct {
+	brain brain.Brain
+	log   log.Logger
+	tools []tool.Tool
+}
+
+const notFound = "No suggestions found"
+
+const prompt = `Analyze the following Java code:
+
+{{code}}
+
+Identify possible improvements or flaws such as:
+
+* grammar and spelling mistakes in comments,
+* variables that can be inlined or removed without changing functionality,
+* unnecessary comments inside methods,
+* redundant code.
+
+Don't suggest any changes that would alter the functionality of the code.
+Don't suggest any changes that would require moving code parts between files (like extract class or extract an interface).
+Don't suggest method renaming or class renaming.
+
+Keep in mind the following imperfections with Java code, identified by automated static analysis system:
+
+{{imperfections}}
+
+Respond with a few most relevant and important suggestion for improvement, formatted as a few lines of text. 
+If there are no suggestions or they are insignificant, respond with "{{not-found}}".
+Do not include any explanations, summaries, or extra text.
+`
+
+// Review sends the provided Java class to the Critic for analysis and returns suggested improvements.
+func (c *agent) Review(job *domain.Job) (*domain.Artifacts, error) {
+	class := job.Classes[0]
+	c.log.Info("received class %q for analysis", class.Name())
+	replacer := strings.NewReplacer(
+		"{{code}}", class.Content(),
+		"{{imperfections}}", tool.NewCombined(c.tools...).Imperfections(),
+		"{{not-found}}", notFound,
+	)
+	answer, err := c.brain.Ask(replacer.Replace(prompt))
+	if err != nil {
+		return nil, fmt.Errorf("failed to get answer from brain: %w", err)
+	}
+	suggestions := c.associated(parseAnswer(answer), class.Path())
+	logSuggestions(c.log, suggestions)
+	artifacts := domain.Artifacts{
+		Descr: &domain.Description{
+			Text: fmt.Sprintf("Critique for class %s", class.Name()),
+		},
+		Suggestions: suggestions,
+	}
+	return &artifacts, nil
+}
+
+func logSuggestions(logger log.Logger, suggestions []domain.Suggestion) {
+	for i, suggestion := range suggestions {
+		logger.Info("#%d: %s", i+1, suggestion)
+	}
+}
+
+func parseAnswer(answer string) []string {
+	lines := strings.Split(strings.TrimSpace(answer), "\n")
+	var suggestions []string
+	for _, line := range lines {
+		suggestion := strings.TrimSpace(line)
+		if suggestion != "" {
+			suggestions = append(suggestions, suggestion)
+		}
+	}
+	return suggestions
+}
+
+func (a *agent) associated(suggestions []string, class string) []domain.Suggestion {
+	res := make([]domain.Suggestion, 0)
+	for i, suggestion := range suggestions {
+		if strings.EqualFold(suggestion, notFound) {
+			a.log.Info("no suggestions found for the class #%d: %s", i+1, class)
+		} else {
+			res = append(res, *domain.NewSuggestion(suggestion, class))
+		}
+	}
+	a.log.Info("total suggestions associated with class %s: %d", class, len(res))
+	return res
+}

internal/critic/server.go

@@ -5,7 +5,6 @@ import (
 	"context"
 	"fmt"
 	"net/http"
-	"strings"
 
 	"github.com/cqfn/refrax/internal/brain"
 	"github.com/cqfn/refrax/internal/domain"
@@ -17,48 +16,20 @@ import (
 // Critic represents the main struct responsible for analyzing code critiques.
 type Critic struct {
 	server protocol.Server
-	brain  brain.Brain
 	log    log.Logger
 	port   int
-	tools  []tool.Tool
+	agent  *agent
 }
 
-const notFound = "No suggestions found"
-
-const prompt = `Analyze the following Java code:
-
-{{code}}
-
-Identify possible improvements or flaws such as:
-
-* grammar and spelling mistakes in comments,
-* variables that can be inlined or removed without changing functionality,
-* unnecessary comments inside methods,
-* redundant code.
-
-Don't suggest any changes that would alter the functionality of the code.
-Don't suggest any changes that would require moving code parts between files (like extract class or extract an interface).
-Don't suggest method renaming or class renaming.
-
-Keep in mind the following imperfections with Java code, identified by automated static analysis system:
-
-{{imperfections}}
-
-Respond with a few most relevant and important suggestion for improvement, formatted as a few lines of text. 
-If there are no suggestions or they are insignificant, respond with "{{not-found}}".
-Do not include any explanations, summaries, or extra text.
-`
-
 // NewCritic creates and initializes a new instance of Critic.
 func NewCritic(ai brain.Brain, port int, tools ...tool.Tool) *Critic {
 	logger := log.NewPrefixed("critic", log.NewColored(log.Default(), log.Cyan))
 	server := protocol.NewServer(agentCard(port), port)
 	critic := &Critic{
 		server: server,
-		brain:  ai,
 		log:    logger,
 		port:   port,
-		tools:  tools,
+		agent:  &agent{brain: ai, log: logger, tools: tools},
 	}
 	server.MsgHandler(critic.think)
 	critic.log.Debug("preparing the Critic server on port %d with ai provider %s", port, ai)
@@ -122,62 +93,8 @@ func (c *Critic) thinkLong(m *protocol.Message) (*protocol.Message, error) {
 	if err != nil {
 		return nil, fmt.Errorf("failed to parse task from message: %w", err)
 	}
-	class := tsk.Classes[0]
-	c.log.Info("received class %q for analysis", class.Name())
-	replacer := strings.NewReplacer(
-		"{{code}}", class.Content(),
-		"{{imperfections}}", tool.NewCombined(c.tools...).Imperfections(),
-		"{{not-found}}", notFound,
-	)
-	answer, err := c.brain.Ask(replacer.Replace(prompt))
-	if err != nil {
-		return nil, fmt.Errorf("failed to get answer from brain: %w", err)
-	}
-	suggestions := parseAnswer(answer)
-	suggestions = associated(suggestions, class.Path())
-	logSuggestions(c.log, suggestions)
-	all := make([]domain.Suggestion, 0, len(suggestions))
-	for _, suggestion := range suggestions {
-		if strings.EqualFold(suggestion, notFound) {
-			c.log.Info("no suggestions found for class %s", class.Name())
-			continue
-		}
-		s := domain.NewSuggestion(suggestion)
-		all = append(all, s)
-	}
-	artifacts := domain.Artifacts{
-		Descr: &domain.Description{
-			Text: fmt.Sprintf("Critique for class %s", class.Name()),
-		},
-
-		Suggestions: all,
-	}
-	return artifacts.Marshal().Message, nil
-}
-
-func logSuggestions(logger log.Logger, suggestions []string) {
-	for i, suggestion := range suggestions {
-		logger.Info("suggestion #%d: %s", i+1, suggestion)
-	}
-}
-
-func parseAnswer(answer string) []string {
-	lines := strings.Split(strings.TrimSpace(answer), "\n")
-	var suggestions []string
-	for _, line := range lines {
-		suggestion := strings.TrimSpace(line)
-		if suggestion != "" {
-			suggestions = append(suggestions, suggestion)
-		}
-	}
-	return suggestions
-}
-
-func associated(suggestions []string, classPath string) []string {
-	for i, suggestion := range suggestions {
-		suggestions[i] = fmt.Sprintf("%s: %s", classPath, suggestion)
-	}
-	return suggestions
+	artifacts, err := c.agent.Review(tsk)
+	return artifacts.Marshal().Message, err
 }
 
 func agentCard(port int) *protocol.AgentCard {

internal/critic/server_test.go

@@ -61,7 +61,7 @@ func TestNewCritic_Success(t *testing.T) {
 
 	critic := NewCritic(ai, 18081)
 	require.NotNil(t, critic)
-	assert.Equal(t, ai, critic.brain)
+	assert.Equal(t, ai, critic.agent.brain)
 }
 
 func TestCriticStart_Success(t *testing.T) {

internal/domain/a2a.go

@@ -41,7 +41,7 @@ func UnmarshalArtifacts(msg *protocol.Message) (*Artifacts, error) {
 			if err != nil {
 				return nil, fmt.Errorf("failed to unmarshal suggestion: %w", err)
 			}
-			suggestions = append(suggestions, s)
+			suggestions = append(suggestions, *s)
 		default:
 			return nil, fmt.Errorf("unknown part type %s", t)
 		}
@@ -85,7 +85,7 @@ func UnmarshalJob(msg *protocol.Message) (*Job, error) {
 			if err != nil {
 				return nil, fmt.Errorf("failed to unmarshal suggestion: %w", err)
 			}
-			suggestions = append(suggestions, s)
+			suggestions = append(suggestions, *s)
 		default:
 			return nil, fmt.Errorf("unknown part type %s", t)
 		}
@@ -96,9 +96,26 @@ func UnmarshalJob(msg *protocol.Message) (*Job, error) {
 	return job, nil
 }
 
-func UnmarshalSuggestion(part protocol.Part) (Suggestion, error) {
+func UnmarshalOldSuggestion(part protocol.Part) (OldSuggestion, error) {
 	if part.PartKind() == protocol.PartKindText {
-		suggestion := NewSuggestion(part.(*protocol.TextPart).Text)
+		suggestion := NewOldSuggestion(part.(*protocol.TextPart).Text)
+		return suggestion, nil
+	}
+	return nil, fmt.Errorf("expected text part for suggestion, got %s", part.PartKind())
+}
+
+func UnmarshalSuggestion(part protocol.Part) (*Suggestion, error) {
+	if part.PartKind() == protocol.PartKindText {
+		tp := part.(*protocol.TextPart)
+		text := tp.Text
+		path, ok := tp.Metadata()["class-path"].(string)
+		if !ok {
+			return nil, fmt.Errorf("missing or invalid class-path metadata in suggestion part")
+		}
+		suggestion := &Suggestion{
+			ClassPath: path,
+			Text:      text,
+		}
 		return suggestion, nil
 	}
 	return nil, fmt.Errorf("expected text part for suggestion, got %s", part.PartKind())
@@ -114,7 +131,7 @@ func UnmarshalClass(part protocol.Part) (Class, error) {
 		return nil, fmt.Errorf("failed to decode file part: %w", err)
 	}
 	meta := f.Metadata()
-	return NewClass(
+	return NewInMemoryClass(
 		fmt.Sprintf("%v", meta["class-name"]),
 		fmt.Sprintf("%v", meta["class-path"]),
 		decoded,
@@ -148,10 +165,7 @@ func (j *Job) Marshal() *protocol.MessageSendParams {
 	}
 	if len(j.Suggestions) > 0 {
 		for _, suggestion := range j.Suggestions {
-			if suggestion == nil {
-				continue
-			}
-			msg.AddPart(MarshalSuggestion(suggestion))
+			msg.AddPart(suggestion.Marshal())
 		}
 	}
 	if len(j.Examples) > 0 {
@@ -180,10 +194,7 @@ func (a *Artifacts) Marshal() *protocol.MessageSendParams {
 	}
 	if len(a.Suggestions) > 0 {
 		for _, suggestion := range a.Suggestions {
-			if suggestion == nil {
-				continue
-			}
-			msg.AddPart(MarshalSuggestion(suggestion))
+			msg.AddPart(suggestion.Marshal())
 		}
 	}
 	return protocol.NewMessageSendParams().WithMessage(msg)
@@ -197,13 +208,20 @@ func (d *Description) Marshal() protocol.Part {
 	return part
 }
 
+func (s *Suggestion) Marshal() protocol.Part {
+	part := protocol.NewText(s.Text).
+		WithMetadata("class-path", s.ClassPath).
+		WithMetadata("type", typeSuggestion)
+	return part
+}
+
 func MarshalClass(c Class, t string) protocol.Part {
 	return protocol.NewFileBytes([]byte(c.Content())).
 		WithMetadata("type", t).
 		WithMetadata("class-name", c.Name()).
 		WithMetadata("class-path", c.Path())
 }
 
-func MarshalSuggestion(s Suggestion) protocol.Part {
+func MarshalSuggestion(s OldSuggestion) protocol.Part {
 	return protocol.NewText(s.Text()).WithMetadata("type", typeSuggestion)
 }

internal/domain/a2a_test.go

@@ -14,16 +14,16 @@ func TestMarshalAndUnmarshalJob(t *testing.T) {
 			Meta: map[string]any{"key": "value"},
 		},
 		Classes: []Class{
-			NewClass("TestClass", "test/path/TestClass.java", "public class TestClass {}"),
-			NewClass("AnotherClass", "test/path/AnotherClass.java", "public class AnotherClass {}"),
+			NewInMemoryClass("TestClass", "test/path/TestClass.java", "public class TestClass {}"),
+			NewInMemoryClass("AnotherClass", "test/path/AnotherClass.java", "public class AnotherClass {}"),
 		},
 		Examples: []Class{
-			NewClass("ExampleClass", "test/path/ExampleClass.java", "public class ExampleClass {}"),
+			NewInMemoryClass("ExampleClass", "test/path/ExampleClass.java", "public class ExampleClass {}"),
 		},
 		Suggestions: []Suggestion{
-			NewSuggestion("Improve TestClass"),
-			NewSuggestion("Add documentation to AnotherClass"),
-			NewSuggestion("Refactor ExampleClass"),
+			*NewSuggestion("Improve TestClass", "test/path/TestClass.java"),
+			*NewSuggestion("Add documentation to AnotherClass", "test/path/AnotherClass.java"),
+			*NewSuggestion("Refactor ExampleClass", "test/path/ExampleClass.java"),
 		},
 	}
 	after, err := UnmarshalJob(before.Marshal().Message)