Log parameters and selectors after task is found #178
Conversation
- Log parameters when set (non-empty) to stderr - Log selectors when set (non-empty) to stderr - Ensure logging happens exactly once in Run method - Add comprehensive unit tests for logging behavior - Tests cover all scenarios: with params, selectors, resume mode, and empty cases Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>
pkg/codingcontext/context.go
Outdated
| } | ||
|
|
||
| // Log selectors if any are set | ||
| if len(cc.includes) > 0 { |
There was a problem hiding this comment.
Yes, selectors are modified after the task is found. The findTask() method adds task_name as a selector (line 107) and merges selectors from task frontmatter (line 129). Commands can also add selectors (line 227).
Fixed in 3fe95e3 - moved logging to after findTask() is called so we capture the complete selector state including task_name and any task/command frontmatter selectors.
pkg/codingcontext/context.go
Outdated
| } | ||
|
|
||
| // Log parameters if any are set | ||
| if len(cc.params) > 0 { |
There was a problem hiding this comment.
Removed the zero check in 3fe95e3 - now always logs parameters and selectors even if empty (params="" and selectors="{}").
The TestExpanderSecurityIntegration test was checking that sensitive parameter values don't appear anywhere in the output. However, with the new parameter logging, the parameter values legitimately appear in stderr logs. Updated the test to only check the actual prompt output (stdout) for sensitive data re-expansion, not the logging output (stderr). This preserves the security check while allowing legitimate parameter logging. Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>
- Move parameter/selector logging to after findTask() is called - This ensures we capture task_name and any selectors from task/command frontmatter - Remove length checks - always log even if empty (per review feedback) - Update test expectations to reflect always-logging behavior - task_name selector is always added during findTask Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds diagnostic logging to output parameters and selectors during the context assembly process. The logging occurs after the task is found to ensure all selectors from task/command frontmatter are captured.
Key changes:
- Added INFO-level logging of parameters and selectors in
context.goafter task resolution - Added comprehensive table-driven unit tests for the new logging functionality
- Updated security integration test to distinguish between parameter logging (stderr) and prompt output (stdout)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/codingcontext/context.go | Added two log statements to output parameters and selectors after task is found, ensuring all additions from frontmatter are captured |
| pkg/codingcontext/context_test.go | Added comprehensive table-driven test TestLogsParametersAndSelectors covering various scenarios including resume mode and empty parameters/selectors |
| integration_test.go | Updated TestExpanderSecurityIntegration to separate stderr (logs) from stdout (prompt) when validating that sensitive parameters don't appear in the generated prompt |
| expectParamsLog bool | ||
| expectSelectors bool // Always true since task_name is added |
There was a problem hiding this comment.
The expectParamsLog and expectSelectors fields are always set to true in all test cases, making them unnecessary. Since the PR description indicates that logging always occurs (even when empty), these boolean fields add complexity without value. Consider removing these fields and simplifying the test to always verify that exactly one log message appears for each type.
| // Verify parameters logging - always exactly once | ||
| if tt.expectParamsLog { | ||
| if paramsCount != 1 { | ||
| t.Errorf("expected exactly 1 Parameters log, got %d", paramsCount) | ||
| } | ||
| } else { | ||
| if paramsCount != 0 { | ||
| t.Errorf("expected no Parameters log, got %d", paramsCount) | ||
| } | ||
| } | ||
|
|
||
| // Verify selectors logging - always exactly once | ||
| if tt.expectSelectors { | ||
| if selectorsCount != 1 { | ||
| t.Errorf("expected exactly 1 Selectors log, got %d", selectorsCount) | ||
| } | ||
| } else { | ||
| if selectorsCount != 0 { |
There was a problem hiding this comment.
The else branches in this verification code are unreachable since expectParamsLog and expectSelectors are always true in all test cases. This is dead code that should be removed along with the boolean fields, or the test cases should be updated to actually test scenarios where logging does not occur (if such scenarios exist).
| // Verify parameters logging - always exactly once | |
| if tt.expectParamsLog { | |
| if paramsCount != 1 { | |
| t.Errorf("expected exactly 1 Parameters log, got %d", paramsCount) | |
| } | |
| } else { | |
| if paramsCount != 0 { | |
| t.Errorf("expected no Parameters log, got %d", paramsCount) | |
| } | |
| } | |
| // Verify selectors logging - always exactly once | |
| if tt.expectSelectors { | |
| if selectorsCount != 1 { | |
| t.Errorf("expected exactly 1 Selectors log, got %d", selectorsCount) | |
| } | |
| } else { | |
| if selectorsCount != 0 { | |
| // Verify parameters logging - expected count depends on test case | |
| expectedParamsCount := 0 | |
| if tt.expectParamsLog { | |
| expectedParamsCount = 1 | |
| } | |
| if paramsCount != expectedParamsCount { | |
| if expectedParamsCount == 1 { | |
| t.Errorf("expected exactly 1 Parameters log, got %d", paramsCount) | |
| } else { | |
| t.Errorf("expected no Parameters log, got %d", paramsCount) | |
| } | |
| } | |
| // Verify selectors logging - expected count depends on test case | |
| expectedSelectorsCount := 0 | |
| if tt.expectSelectors { | |
| expectedSelectorsCount = 1 | |
| } | |
| if selectorsCount != expectedSelectorsCount { | |
| if expectedSelectorsCount == 1 { | |
| t.Errorf("expected exactly 1 Selectors log, got %d", selectorsCount) | |
| } else { |
Parameters (
-p) and selectors (-s) are now logged to stderr when the CLI runs, making it easier to debug and verify what values were used.Changes
Logging: Added one-time logging of parameters and selectors in
context.Run()after the task is foundmsg=Parameters params="key=value,..."andmsg=Selectors selectors="{key=val, ...}"params=""and empty selectors show asselectors="{}"findTask()to capturetask_nameselector and any selectors from task/command frontmatterresume=true) is included when-rflag is usedTests: Added
TestLogsParametersAndSelectorscovering all scenarios (with params, selectors, resume mode, empty)Security test fix: Updated
TestExpanderSecurityIntegrationto check only stdout (prompt) for parameter re-expansion, not stderr (logs)Example
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.