Skip to content

fix: route k8q sum through pipeline writer, accumulate filter errors - #2

Draft
tobiash wants to merge 3 commits into
mainfrom
forge/cyc-4-k8q-sum-filter
Draft

fix: route k8q sum through pipeline writer, accumulate filter errors#2
tobiash wants to merge 3 commits into
mainfrom
forge/cyc-4-k8q-sum-filter

Conversation

@tobiash

@tobiash tobiash commented Aug 18, 2026

Copy link
Copy Markdown
Owner

Summary

Cycles: cyc-4 (c5e3f0)
Implementation task: t_e1f181d1
Review task: t_0b102a3d (verdict: ACCEPT, round 2)

This PR fixes k8q sum so that:

  1. The summary output is routed through the kyaml pipeline writer instead of being printed directly to stdout via fmt.Println/fmt.Printf. This restores composability with downstream filters.
  2. Missing-resource errors are accumulated and returned through the pipeline as a normal filter error (%w-wrapped), surfaced by the CLI's standard error path — no more direct os.Stderr writes inside the filter.

The black-box six-line summary output (Requests:\n CPU: …\n Memory: …\nLimits:\n CPU: …\n Memory: …) is byte-for-byte identical to the previous behavior (SHA-256 verified live against a 2-replica 100m-CPU Deployment: e9fee323…).

Acceptance evidence (from t_0b102a3d ACCEPT verdict)

  • gofmt -d . clean
  • go vet ./... clean
  • go test -count=1 ./pkg/engine/... PASS (5/5 sub-tests including new TestSumFilter and TestSumJSON)
  • go test -count=1 ./... PASS for all packages
  • Live CLI byte-check: stdout SHA-256 e9fee3232658a2e508779e87d859e7b6c879c45912629fcc159077d5143b1351 (88 bytes, 0 bytes stderr)
  • Live error path: --require-limits on a manifest missing limits → exit 1, single stderr line resource requirements check failed for 1 resources: Deployment/example: container "app" missing resources.limits, 0 bytes stdout
  • Scope exactly 4 files: REVIEW.md, pkg/engine/engine.go, pkg/engine/sum.go, pkg/engine/sum_test.go

Files changed

 REVIEW.md              | 12 +++---
 pkg/engine/engine.go   | 22 +++++++++--
 pkg/engine/sum.go      | 22 +++++-----
 pkg/engine/sum_test.go | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 137 insertions(+), 18 deletions(-)

Commits

  • 452595a fix: preserve sum output alignment
  • adbe92c fix: route sum output through pipeline

Two small linear commits because the first review found an output-spacing regression in adbe92c; the follow-up 452595a corrects it. No merge commits, no upstream tracking, working tree clean.

Limitations

  • This is a draft PR awaiting human review (defaults.agent_policy.merge_prs: false, human_review_required: true).
  • No CI/release/dependency/secrets/infrastructure files were touched.

@tobiash

tobiash commented Aug 18, 2026

Copy link
Copy Markdown
Owner Author

No terminal and no GitHub tools are exposed in this session — I cannot run gh pr diff. I should not fabricate a review.

Need you: this webhook session has no terminal or GitHub tool available, so I can't fetch PR #2's diff to review.

Action: none on my side — please run the review in a session that has shell/gh access, or expose a terminal / mcp__github__* tool to the github-pr-review profile.

Reply: skipping review for PR #2 to avoid fabricating findings.

(◕‿◕)✧

@tobiash

tobiash commented Aug 18, 2026

Copy link
Copy Markdown
Owner Author

I can't fetch the PR diff — no shell/HTTP/GitHub tool is available in this webhook-only session. Reporting the blocker honestly rather than fabricating a review.


Review skipped — unable to fetch diff

Need you: this webhook session has no shell/HTTP tool to run gh pr diff or fetch the patch directly.
Action: either re-deliver via a session with shell access, or paste the diff and I'll review it.
Reply: n/a (I'll comment on PR #2 once the diff is available).

Preliminary notes from the PR title alone (no code seen yet):

  • "route k8q sum through pipeline writer" — looks like a streaming/encoding fix; should verify the writer isn't double-buffered or missing a flush/close on error path.
  • "accumulate filter errors" — ensure errors don't short-circuit remaining filters when downstream might still want partial results, and that returned errors wrap enough context (filter name + index) for debugging.
  • Watch for: goroutine leaks if filter pipeline runs concurrently, and test coverage on the new error-aggregation path.

Send the diff (or trigger this from a shell-capable session) and I'll produce the actual review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant