Migrate compact logs run tables from tabwriter to console.RenderTable - #50850
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
|
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Migrates compact log run tables to centralized console rendering.
Changes:
- Replaces
tabwritertables withconsole.RenderTable. - Adds compact and verbose rendering tests.
- Also modifies workflow skill routing outside the stated scope.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/logs_format_compact.go |
Migrates both run tables. |
pkg/cli/logs_format_compact_test.go |
Tests borders, ANSI degradation, and filtering. |
.github/skills/agentic-workflows/SKILL.md |
Adds an unrelated routing entry. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| - `.github/aw/debug-agentic-workflow.md` | ||
| - `.github/aw/dependabot.md` | ||
| - `.github/aw/deployment-status.md` | ||
| - `.github/aw/designer-mappings.md` |
There was a problem hiding this comment.
I investigated this while fixing the failures in 5be7228. The workflow-skill source list is currently enforced by sync tests and had to stay aligned with fallback metadata; I applied the generated fallback list sync so tests pass. If you want this routing-list change isolated, I can split it into a dedicated follow-up PR.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: Migrate compact logs run tables from tabwriter to console.RenderTable
The migration is well-executed and consistent across both render functions. The new test file provides meaningful coverage.
Confirmed correct:
RenderTableusesisTTY()(stdout detection) — correct default for these tables- Border characters (
╭) always render regardless of TTY; only ANSI color codes are stripped on non-TTY — test assertions are accurate strconv.FormatInt/strconv.Itoaconversions match the field types (int64RunID,intfor others)- Filter logic for skipped/cancelled uses
ConclusionwithStatusfallback — test exercises both correctly RenderTableappends a trailingfmt.Fprint(notfmt.Fprintln) avoids a double newline
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 29.1 AIC · ⊞ 5.3K
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics (3 tests)
📝 Test Notes
Inflation note: The file added 77 lines against 24 production lines (3.2:1), above the 2:1 threshold. However, this is a newly added test file covering a refactored function — the ratio reflects thorough initial coverage rather than padding. No score penalty was waived but the flag is noted for transparency. Verdict
References: §31102988570
|
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — two focused observations, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap: The filter test only exercises
Status-fallback; theConclusion-first branch goes untested. - RenderTable trailing-newline contract:
fmt.Fprint+ the string return value may produce missing blank lines between sections if the contract isn't guaranteed.
Positive Highlights
- ✅ Clean, surgical refactor — existing filtering and fallback logic is fully preserved.
- ✅ New test file covers bordered rendering, ANSI degradation on non-TTY, and run filtering — a good baseline.
- ✅
strconv.FormatInt/Itoacorrectly replaces format verbs for the type-safe string conversion. - ✅ Removes the now-unused
tabwriterimport cleanly.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 37 AIC · ⊞ 7.1K
Comment /matt to run again
| t.Parallel() | ||
| data := compactTestData() | ||
| data.Runs = append(data.Runs, | ||
| RunData{RunID: 222, WorkflowName: "skipped-wf", Status: "skipped", CreatedAt: time.Now()}, |
There was a problem hiding this comment.
[/tdd] The skipped/cancelled filter test only exercises the Status-fallback path — it sets Status: "skipped" but leaves Conclusion empty. The actual code checks Conclusion first; when it is non-empty, Status is never read. A test with Conclusion: "skipped" is needed to cover the primary branch.
💡 Suggested addition
Add two more rows that set Conclusion instead of Status:
RunData{RunID: 444, WorkflowName: "skipped-c", Conclusion: "skipped", CreatedAt: time.Now()},
RunData{RunID: 555, WorkflowName: "cancelled-c", Conclusion: "cancelled", CreatedAt: time.Now()},Then assert:
assert.NotContains(t, out, "444")
assert.NotContains(t, out, "555")@copilot please address this.
There was a problem hiding this comment.
Addressed in 5be7228. The test now adds Conclusion: "skipped" and Conclusion: "cancelled" rows and asserts run IDs 444 and 555 are excluded, covering the primary Conclusion-first branch.
| })) | ||
|
|
||
| // [errors] — aggregated error/warning messages | ||
| if len(data.ErrorsAndWarnings) > 0 { |
There was a problem hiding this comment.
[/codebase-design] console.RenderTable returns a string that may or may not end with \n; fmt.Fprint is used here, but the section that follows ([errors]) is printed with fmt.Fprintln. If RenderTable already appends a trailing newline the output is correct, but if it doesn't the [errors] header will be run on directly after the closing border with no blank line. Worth adding a guard or verifying the contract in the RenderTable docs.
💡 Suggested guard
out := console.RenderTable(console.TableConfig{...})
if !strings.HasSuffix(out, "\n") {
out += "\n"
}
fmt.Fprint(w, out)Or, if RenderTable guarantees a trailing newline, add a comment to that effect so future readers don't have to trace into the package.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 5be7228. I added an explicit comment at both console.RenderTable callsites in pkg/cli/logs_format_compact.go documenting the trailing-newline contract so section headers remain separated without adding extra guards.
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (101 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
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 5be7228:
Investigated the listed failed checks via job logs:
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot Please address the unresolved review feedback already answered on this PR, verify any remaining unresolved threads are resolved, and investigate the failed checks below before running the pr-finisher skill.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 9e3d8ac.
Validation run:
|
|
🎉 This pull request is included in a new release. Release: |
The two
[runs]tables inpkg/cli/logs_format_compact.gowere built withtext/tabwriter, bypassing the CLI's centralizedconsole.RenderTablestyling and TTY-degradation logic used by every other table.Changes
renderLogsCompactToWriter— accumulates[][]stringrows and renders throughconsole.RenderTable(console.TableConfig{...})instead of writing tab-delimited lines to atabwriter.Writer.renderLogsCompactVerboseToWriter— same migration for the 16-column verbose table.strconv(FormatIntfor theint64RunID); all prior fallbacks (-for empty duration/actor/TBT/class), truncation, and theskipped/cancelledfilter are unchanged.text/tabwriterimport and bothtw.Flush()error-logging branches removed —RenderTablereturns a string.logs_format_compact_test.go(new) — asserts bordered rendering, absence of ANSI escapes on non-TTY writers, and continued exclusion of skipped/cancelled runs.Output shape
Columns and ordering are preserved; alignment now comes from lipgloss borders rather than tab padding:
Note that this format is consumed by agents/LLMs, so the added border glyphs are a deliberate (if minor) increase in token density over the previous whitespace alignment — worth a look if that tradeoff matters here.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.