Repository navigation
Preserve MCP log continuation cursors and episode coverage - #70856
Conversation
Surface per-target resume cursors and partial or unknown episode evidence in the logs file response without changing retained report data. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The adapter can silently discard valid continuation cursors when an episode coverage field is malformed or evolves incompatibly: the single unmarshal returns an error and the function returns an empty response. Preserve cursor extraction independently from optional coverage parsing, and add a regression test. Static review only; no commands/tests were run.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 2.23 AIC · ⌖ 0.544 AIC · ⊞ 1.6K
Comment /review to run again
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (195 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
🟢 Approval recommended
The adapter matches the report structures and regression cases; only non-blocking documentation clarifications remain.
No open issues 🎉
What changed in this PR
Updates the MCP logs adapter to expose collection boundaries and episode coverage without altering cached reports.
Changes:
- Preserves single-target and per-target continuation cursors.
- Surfaces episode coverage status and deduplicated reasons.
- Adds regression tests and updates caller guidance.
| File | Description |
|---|---|
pkg/cli/mcp_tools_privileged.go |
Clarifies continuation and coverage guidance. |
pkg/cli/mcp_logs_guardrail.go |
Preserves cursors and summarizes collection state. |
pkg/cli/mcp_logs_guardrail_test.go |
Tests cursors, coverage gaps, and unchanged report bytes. |
docs/src/content/docs/reference/gh-aw-as-mcp-server.md |
Documents response fields and recovery behavior. |
Review effort: Max
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design (skill selection used the fallback heuristic — pr-triage was not invoked for this review; classification was done manually against the diff and change type was determined to be a feature/refactor touching MCP logs guardrail response shaping). Two non-blocking findings posted inline; overall the PR is solid.
📋 Key Themes & Highlights
Key Themes
- Untested error path:
extractLogsCollectionStatesilently swallows JSON parse failures (debug-log only), returning a zero-value response indistinguishable from 'fully collected, no gaps'. Verified locally that malformed/truncated CLI output yieldsPartial:false, CoverageStatus:""— worth either an explicit regression test or an"unknown"coverage status on parse failure. - Minor duplication: the
"retrieval_" + reasonprefixing convention is reimplemented inline rather than reusing the pattern already established inlogs_episode_retrieval.go. Low risk today, but a seam that could drift if the reason vocabulary changes.
Positive Highlights
- ✅ 9 new table-driven subtests (
TestBuildLogsFileResponse_CollectionCoverage) give solid coverage of the happy-path cursor/coverage-status transitions, truncation flag, and reason dedup/sort behavior. - ✅
(nolint/redacted):largefuncon the now-109-linebuildLogsFileResponseis consistent with existing repo conventions (same pattern used inzizmor.go,grype.go,mcp_inspect.go). - ✅ Reuse of
EpisodeCoverage(rather than reinventing coverage semantics) keeps the new response shape aligned with the existing episode-coverage model. - ✅ Doc and MCP tool-description updates accurately describe the new
continuations/coverage_status/coverage_reasonsfields for both human and MCP-client readers.
Nothing here blocks merge; the missing error-path test is the one item I'd actually want addressed before the gap compounds further.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 147.8 AIC · ⌖ 15.1 AIC · ⊞ 10.2K
Comment /matt to run again
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 5ef3e08
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Expose unknown collection state on invalid reports, share retrieval reason naming, and finalize ADR-70856. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Regarding Test Quality Sentinel's report: no code change is needed for this informational no-action result. I did not treat it as proof of completeness; the batch adds explicit malformed-output regressions and has its own validation. |
|
Regarding PR Code Quality Reviewer's completion notice: the actionable inline feedback is accepted. Cursor sections now decode independently from optional coverage, with regression cases for malformed coverage and preserved resume paths. |
|
Regarding Ponytail Reviewer's no-action report: no implementation change is requested by this status notice. I did not infer that the PR is fully verified from the no-op result and validated the review fixes separately. |
|
Regarding Matt Pocock Skills Reviewer's completion notice: both related inline suggestions are accepted. Parse failures now expose unknown collection state, and retrieval coverage reasons share a helper while retaining named retrieval types. |
|
Regarding the generated ADR commit: reviewed and finalized the ADR's rationale, alternatives, and consequences. Its status is Proposed rather than implying human approval, and the PR body links it. |
|
Regarding the saved review theme: accepted the cursor-preservation finding and added independent parsing plus malformed-section regressions. The managed comment-memory block is unchanged. |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…elikhan-didactic-eureka # Conflicts: # pkg/cli/logs_episode_retrieval.go # pkg/cli/mcp_logs_guardrail.go # pkg/cli/mcp_logs_guardrail_test.go Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Summary
Fix the MCP logs response wrapper so collection boundaries already recorded by the CLI remain visible to callers:
coverage_status(partialorunknown) and deduplicatedcoverage_reasonsfor episode gaps and parse uncertainty. Malformed reports exposeinvalid_report_json; malformed sections never erase valid independent cursors.The cached report remains byte-for-byte unchanged, including intentional-failure markers. No permissions, artifact selection, filtering, raw stderr retention, or workflow sources are changed.
ADR: ADR-70856: Surface Logs Collection State in the MCP Logs Response
Evidence and scope
Bounded collection and episode retrieval already exist in current source. The remaining adapter defect inspected only singular
continuation, hiding multi-target cursors and episode gaps. This follows the collection limitations in discussion #70014, without claiming that the adapter caused every historical timeout or establishes fleet-wide coverage.Review follow-up commit
3dc9202eccmakes cursor/coverage parsing independent, explicitly reports unknown parse state, adds malformed-input regression cases, and finalizes the generated ADR as Proposed without implying human approval.Validation
GOMAXPROCS=2 go test ./pkg/cli -run 'Test(BuildLogsFileResponse|ExtractLogsCollectionState|NewLogsTool|LogsTool)' -count=1.make agent-report-progress; the follow-up changed-file custom linter check passed after extracting the decoder helpers.make fmtcompleted Go formatting but its unrelated JavaScript stage lacks Prettier in this worktree. Changed Go files are formatted andgit diff --checkpasses.TestEpisodeRetrievalTimeoutAndCancellationfixture. The same failure was reproduced using a Go overlay of both committed pre-fix production files, without changing the workspace. That pre-existing test is not modified by this batch.