feat: agent interface and source layer (Phase 0a — T0.1, T0.2, T0.2a, T0.2b)#36
Open
canonical-muhammadbassiony wants to merge 1 commit into
Open
feat: agent interface and source layer (Phase 0a — T0.1, T0.2, T0.2a, T0.2b)#36canonical-muhammadbassiony wants to merge 1 commit into
canonical-muhammadbassiony wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new abstraction layer around “AI execution” and “upstream content fetching” to decouple the orchestrator from concrete implementations (copilotcli and gdocs). It also updates configuration handling for DryRun to support tri-state behavior and adjusts wiring in the CLI/API entrypoints.
Changes:
- Added
internal/agent.Agent(plusMockAgent) and refactored the orchestrator to depend onagent.Agentrather thancopilotcli.Client. - Added
internal/source(SourceBundle,Manager) and refactored the orchestrator to fetch viasource.Managerinstead of importinggdocsdirectly. - Updated
Config.DryRunfrombool→*booland updated call sites/tests accordingly.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/workflow/workflow.go | Updates DryRun wiring to *bool and switches result access to ExtractionBundle. |
| internal/source/types.go | Introduces SourceBundle to carry extracted document/design data. |
| internal/source/source.go | Adds Adapter interface + Request type for source fetching. |
| internal/source/manager.go | Adds Manager.Fetch() which currently fetches from Google Docs. |
| internal/source/manager_test.go | Adds basic tests for manager creation and empty-request fetch behavior. |
| internal/orchestrator/orchestrator.go | Refactors orchestrator to use agent.Agent + source.Manager, adds summary handling and local chunk output type. |
| internal/copilotcli/client.go | Makes Client implement agent.Agent; changes Start signature; changes GenerateSummary to return the summary string. |
| internal/config/config.go | Changes DryRun to *bool and adds BoolPtr/BoolVal helpers. |
| internal/config/config_test.go | Updates credentials fixture JSON to satisfy gdocs.ValidateCredentialsFile. |
| internal/config/cli.go | Introduces CLIFlags and updates DryRun assignment to pointer form. |
| internal/agent/agent.go | Adds the new Agent interface definition. |
| internal/agent/mock.go | Adds MockAgent used by tests. |
| internal/agent/agent_test.go | Adds tests + compile-time interface conformance check for MockAgent. |
| cmd/bauer/main.go | Updates wiring to construct copilotcli.Client and source.Manager, then inject into orchestrator.New(...). |
| cmd/app/main.go | Updates API server wiring to use injected agent + source manager with the new orchestrator constructor. |
| docs/implementation-log.md | Marks the branch/task entry as done and documents the changes made. |
Comments suppressed due to low confidence (1)
internal/config/cli.go:100
Config.DryRunis a*bool, butdryRun := flag.Bool(...)always yields a non-nil pointer, even when the flag wasn’t provided. That makes it impossible to distinguish “unset” vs “explicit false”, which undermines the stated goal of allowing CLI flags to override config-file values during merges.
flag.Parse()
// If --config is provided, load from JSON file
if *configFile != "" {
return LoadFromJSONFile(*configFile)
}
// If no required flags are provided, show usage and exit
if *docID == "" && *credentialsPath == "" {
flag.Usage()
os.Exit(1)
}
cfg := &Config{
DocID: *docID,
CredentialsPath: *credentialsPath,
DryRun: dryRun,
ChunkSize: *chunkSize,
PageRefresh: *pageRefresh,
OutputDir: *outputDir,
Model: *model,
SummaryModel: *summaryModel,
TargetRepo: *targetRepo,
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+5
to
+10
| // SourceBundle is the normalized combined output from all source adapters. | ||
| // It is what the orchestrator operates on — not raw gdocs or figma types directly. | ||
| type SourceBundle struct { | ||
| // Document is the Google Docs extraction result. Always present when a DocID is set. | ||
| Document *gdocs.ProcessingResult `json:"document,omitempty"` | ||
| // Design holds the optional Figma normalized design output. |
Comment on lines
+10
to
+13
| // Manager holds all registered source adapters and orchestrates fetching. | ||
| type Manager struct { | ||
| credentialsPath string | ||
| } |
Comment on lines
284
to
307
| done := make(chan error, 1) | ||
| var fullOutput string | ||
|
|
||
| session.On(func(event copilot.SessionEvent) { | ||
| switch event.Type { | ||
| case "assistant.message_delta": | ||
| if event.Data.DeltaContent != nil { | ||
| fmt.Print(formatSummaryOutput(*event.Data.DeltaContent)) | ||
| fullOutput += *event.Data.DeltaContent | ||
| } | ||
|
|
||
| case "assistant.reasoning_delta": | ||
| if event.Data.DeltaContent != nil { | ||
| fmt.Print(formatCopilotDim(*event.Data.DeltaContent)) | ||
| fullOutput += *event.Data.DeltaContent | ||
| } | ||
|
|
||
| case "assistant.message": | ||
| // Print final message in yellow for summary | ||
| if event.Data.Content != nil { | ||
| fullOutput += *event.Data.Content | ||
| fmt.Println(formatSummaryOutput(*event.Data.Content)) | ||
| slog.Debug("Summary response", slog.String("content", *event.Data.Content)) | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Good catch on the potential duplication. This is pre-existing behavior in copilotcli — this PR only adjusted the package to satisfy the agent.Agent interface without modifying the streaming logic. Reworking the streaming accumulation is risky (tightly coupled to the Copilot CLI protocol) and out of scope here. Tracked as a follow-up.
Comment on lines
309
to
315
| case "assistant.reasoning": | ||
| // Print reasoning in dimmed style | ||
| if event.Data.Content != nil { | ||
| fullOutput += *event.Data.Content | ||
| fmt.Println(formatCopilotDim(*event.Data.Content)) | ||
| slog.Debug("Summary reasoning", slog.String("content", *event.Data.Content)) | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Same as above — the reasoning accumulation is pre-existing. Separating reasoning from summary output is a valid improvement but belongs in a dedicated refactor of the streaming handler.
Comment on lines
+37
to
+41
| copilotAgent, err := copilotcli.NewClient(cwd) | ||
| if err != nil { | ||
| slog.Error("failed to create Copilot client", "error", err.Error()) | ||
| return err | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Agreed this is a concern for concurrent requests. It is addressed in later branches (phase-3/phase-4) where the API handler creates a fresh agent per workflow execution. The shared agent pattern here is interim.
Comment on lines
+43
to
+45
| sources := source.NewManager(cfg.CredentialsPath) | ||
| orch := orchestrator.New(copilotAgent, sources) | ||
|
|
Collaborator
Author
There was a problem hiding this comment.
Same — later branches (phase-3, T3.2) remove per-request credentials entirely from the API request body. The Manager is initialized at startup with the service account path, and that becomes the only source. This interim state goes away.
Comment on lines
33
to
37
| // Only populated if not dry run | ||
| CopilotOutputs []copilotcli.ChunkOutput | ||
| CopilotOutputs []ChunkOutput | ||
| Summary string | ||
| CopilotDuration time.Duration | ||
| SummaryDuration time.Duration |
| output.BauerResult.ExtractionDuration = bauerResult.ExtractionDuration | ||
| output.BauerResult.PlanDuration = bauerResult.PlanDuration | ||
| output.BauerResult.CopilotDuration = bauerResult.CopilotDuration | ||
| output.BauerResult.CopilotDuration = bauerResult.AgentDuration |
Comment on lines
61
to
65
| func (o *DefaultOrchestrator) Execute(ctx context.Context, cfg *config.Config) (*OrchestrationResult, error) { | ||
| startTime := time.Now() | ||
|
|
||
| // 1. Initialize GDocs Client and extract from doc | ||
| // 1. Fetch from source (Google Docs) | ||
| extractionStart := time.Now() |
Comment on lines
+145
to
+149
| func openPRExecutionConfig(cfg *config.Config) *config.Config { | ||
| copy := *cfg | ||
| f := false | ||
| copy.DryRun = &f | ||
| return © |
Comment on lines
+65
to
+67
| if o.sources == nil { | ||
| return nil, fmt.Errorf("orchestrator: sources must not be nil") | ||
| } |
|
|
||
| // Start starts the Copilot CLI server | ||
| func (c *Client) Start() error { | ||
| func (c *Client) Start(_ context.Context) error { |
Comment on lines
+98
to
+102
| // resolveCLIConfig builds a Config from CLI flags, falling back to environment variables | ||
| // when flag values are empty. It does NOT call Validate — callers must do that separately. | ||
| func resolveCLIConfig(flags config.CLIFlags) (*config.Config, error) { | ||
| docID := flags.DocID | ||
| if docID == "" { |
- T0.1: internal/agent package with Agent interface and MockAgent - T0.2: copilotcli.Client implements agent.Agent (updated Start/GenerateSummary sigs) - T0.2a: internal/source package with SourceBundle and Manager - T0.2b: orchestrator refactored to use source.Manager and agent.Agent - Fix: Config.DryRun changed to *bool with BoolPtr/BoolVal helpers - Fix: config.CLIFlags added; resolveCLIConfig/openPRExecutionConfig added to cmd/bauer - Fix: config_test.go updated to use valid credentials JSON fixture
canonical-muhammadbassiony
force-pushed
the
feat/phase-0a-agent-source
branch
from
5dd0c4e to
aa21438
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces the
agent.Agentinterface to decouple the orchestrator fromcopilotcli.Client, and creates thesourcelayer so the orchestrator no longer importsgdocsdirectly.Tasks Implemented
internal/agent/agent.gowithAgentinterface (Start,ExecuteChunk,GenerateSummary,Stop)copilotcli.Clientimplementagent.Agent; updated orchestrator to depend on the interfaceinternal/sourcewith source adapters and normalizedSourceBundlesource.Manager.Fetch()instead ofgdocsdirectlyFiles Changed
internal/agent/agent.go—Agentinterfaceinternal/agent/mock.go—MockAgentfor testsinternal/agent/agent_test.go— compile-time check + mock testsinternal/source/source.go—Adapterinterface andRequesttypeinternal/source/types.go—SourceBundlewrapping*gdocs.ProcessingResultinternal/source/manager.go—ManagerwithFetchinternal/source/manager_test.go— testsinternal/copilotcli/client.go— implementsagent.Agentinternal/orchestrator/orchestrator.go— depends onagent.Agent+source.Managerinternal/config/config.go—DryRun→*bool;BoolPtr/BoolValhelperscmd/bauer/main.go,cmd/app/main.go— updated wiringArchitecture
graph LR subgraph cmd A[cmd/bauer] B[cmd/app] end subgraph orchestrator C[DefaultOrchestrator] end subgraph agent D[Agent interface] E[MockAgent] end subgraph copilotcli F[Client] end subgraph source G[Manager] end A --> C B --> C C --> D C --> G F -->|implements| D E -->|implements| DPart of the Bauer v2 stacked PR series (Branch 1 of 12).