Skip to content

mcpx: CodeRabbit minor follow-ups from PR #2840 — stream the CI log formatter, separate stderr cap for hook failure text #2880

Closed

Description

@dezren39

CodeRabbit minor findings on PR #2840 — deferred follow-ups

PR #2840 (fix for #2573) was reviewed by CodeRabbit at head 831ac5dc (2026-10-30T23:17Z), which raised 3 Minor findings. One was already fixed by the next head; two are valid-but-minor and are tracked here instead of blocking the merge (Codex reviewed the final head 8db2e07b clean; the PR's own acceptance in #2573 covers the 4 Codex findings from #2541, all fixed).

1. Workflow log formatter retains every parsed event in memory — .github/workflows/mcpx.yml:187

The readable-output formatter appends every parsed go test -json event to lines before printing in two passes. Retained memory grows with the full stream. Suggested fix (from the review thread): re-read the file per pass with a generator instead of retaining events. Thread: #2840 (comment)

2. Hook failure text can be polluted by stdout — pkgs/mcpx/internal/hooks/runner.go

execute() points both cmd.Stdout and cmd.Stderr at one shared cappedWriter, and the failure text is firstLine(out.String()). A hook that fills the cap with stdout (no newline) before writing its stderr diagnostic records truncated stdout as the "error". Suggested fix: give stderr its own capped writer, build the failure text from stderr's first line, and fall back to stdout's first line when stderr is empty. Add a regression test (hook writes >cap stdout, then a one-line stderr diagnostic, exits non-zero; Stats().LastError names the diagnostic). Thread: #2840 (comment)

Already fixed (no action)

The third finding (wildcard hook's Dropped counting hook.* events, thread #2840 (comment)) was fixed in head 8db2e07b by Hook.SubFilter() (pkgs/mcpx/internal/hooks/hooks.go), which expands a wildcard to the hookable kinds so hook.* events are excluded before the bus buffers them.

Acceptance

  • Formatter streams per pass (no full event retention) with identical log output
  • stderr has its own capped writer; failure text prefers stderr, falls back to stdout; regression test above passes
  • go test ./internal/hooks/... green in CI

Acceptance criteria

  • The mcpx workflow log formatter streams parsed go test -json events without retaining the full event list in memory (or the retention is bounded and documented).
  • Each deferred CodeRabbit minor finding from PR fix(mcpx): disposition the 4 undispositioned Codex findings from PR #2541 (#2573) #2840 listed in this issue is either fixed or explicitly dispositioned as wontfix with a reason.
  • mcpx CI stays green on the change; no behavior change to the formatted output content.

Activity

  1. added
    kind/debtTechnical debt worth paying down deliberately
    priority/lowNice to have; do when convenient
    app/mcpxPackage/app: mcpx (pkgs/ or apps/ in this repo)
    effort/sSmall: expected under 2 hours of work
    source/queue-workerFiled or worked by the queue/worker pilot
    meta/enrichedIntake review done: acceptance criteria, links, and normalized labels added
    on Oct 31, 2026
  2. dezren39 commented on Oct 31, 2026

    @dezren39
    MemberAuthor

    Intake review

    Related: PR #2840 (merged work for #2573) — CodeRabbit minor findings deferred here by design. Risks: the log formatter change touches CI readability; keep output format identical. Suggested approach: stream-print events in one pass; disposition each listed finding in the PR description.

    — Fleet monitor intake enrichment (protocol §5): verified still live this run (no merged PR resolves it), labels normalized, acceptance criteria appended. Marked meta/enriched.

  3. added
    work/assignedQueue worker lifecycle state
    queue/claimedQueue worker lifecycle state
    and removed on Nov 3, 2026
  4. dezren39 commented on Nov 3, 2026

    @dezren39
    MemberAuthor

    agent-note:v1 — task 7985 picked up by lane code-k (repo-worker-code). Execution lane assigned: opencode — handed off to the opencode claimant, which will launch a bound session to triage issue #2880 at current main.

  5. dezren39 commented on Nov 3, 2026

    @dezren39
    MemberAuthor

    🤖 Agent note · handed off

  6. 13 remaining items

  7. added
    status/parkedDeliberately paused with a recorded resume condition
    and removed
    queue/claimedQueue worker lifecycle state
    on Nov 3, 2026
  8. developing-today commented on Nov 3, 2026

    @developing-today
    Contributor

    agent-note:v1
    state: triaged (coverage pass)
    Coverage pass 2026-11-03: reviewed in the fleet-wide per-issue pass (drift / overlap / already-handled / stale / blockers / ordering). Verdict: still live; queued in repo-health-code-pilot as triage at priority 30 (priority/low). If this is already handled at current main, the triage closes it with evidence; if it needs splitting, the triage files the split.
    Found via: coverage pass 2026-11-03

  9. dezren39 commented on Nov 3, 2026

    @dezren39
    MemberAuthor

    🤖 Agent note · parked (CI still pending)

    • Agent: queue worker (repo-worker-code), lane bf419306-c4ef-470f-a553-beabf93239de (OpenChamber scheduled lane), task 7985 (attempt 5).
    • Resume check 2026-11-03 ~17:20Z: PR fix(mcpx): separate hook stderr cap, stream the CI log formatter (#2880) #5004 is OPEN at 946bbf059, not draft. Mergeable state is UNKNOWN and basic: production line limit (2500) is still pending (Actions backlog — no run has started); CodeRabbit still NEUTRAL/skipping. Attempt-4's local verification (go tests ok, vet clean, formatter byte-parity) stands unrebutted, but the merge criteria (checks green) are not met — parking rather than merging on local evidence alone.
    • Expectation: xs. Resume condition: next pass checks PR fix(mcpx): separate hook stderr cap, stream the CI log formatter (#2880) #5004; if green, merge (the issue closes with the merge); if mcpx checks fail, fix forward on the same branch.
  10. dezren39 commented on Nov 3, 2026

    @dezren39
    MemberAuthor
    No description provided.
  11. dezren39 commented on Nov 3, 2026

    @dezren39
    MemberAuthor

    🤖 Agent note · picked up

  12. removed
    status/parkedDeliberately paused with a recorded resume condition
    queue/queuedHas a queued task in the Supabase agent queue
    queue/parkedQueue task is parked (waiting on CI, time, or an event)
    work/assignedQueue worker lifecycle state
    on Nov 3, 2026
  13. dezren39 commented on Nov 3, 2026

    @dezren39
    MemberAuthor

    🤖 Agent note · done

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

Metadata

Metadata

Assignees

Labels

app/mcpxPackage/app: mcpx (pkgs/ or apps/ in this repo)effort/sSmall: expected under 2 hours of workkind/debtTechnical debt worth paying down deliberatelymeta/enrichedIntake review done: acceptance criteria, links, and normalized labels addedpriority/lowNice to have; do when convenientsource/queue-workerFiled or worked by the queue/worker pilottask/triageQueue packet kind: verify, reproduce, and route this item

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions