Repository navigation
feat(checks): diagnose failures in Go - #832
tbhb-agent wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Reviewed head: 9ff9c96
PR: #832
Model: OpenAI/GPT-6 (Codex), high
Repo criteria: none: no file
Loosens: no; no owner-tier or gate configuration paths touched
Verdict: REQUEST_CHANGES
Findings:
- [blocking] internal/agentrepo/diagnose.go:174: An empty link invalidates the entire check set, although GitHub commit statuses can legitimately omit their target URL. A passing status without a URL returns 2 instead of 0; an unrelated URL-less status also prevents reporting known failures. Fix: accept empty links, preserve name/state validation, and add passing and mixed-failure regressions.
- [should-fix] packages/agents/source/skills/fix-pr/shared/scripts/diagnose.sh:11: Fallback directory resolution inherits CDPATH before the legacy script neutralizes it. With a relative invocation and CDPATH containing the current directory, Bash prints the resolved directory; command substitution captures that plus pwd, producing an invalid fallback path. Other CDPATH entries can redirect resolution. Fix: clear CDPATH for directory resolution and cover fallback under a hostile CDPATH.
- [should-fix] internal/agentrepo/diagnose.go:130: TrimSpace removes significant trailing whitespace from repository directory names. A valid checkout whose directory ends in a space becomes a different subprocess working directory and diagnosis refuses it; the frozen shell preserves that path. Fix: remove only Git’s line terminator and add a repository-path regression.
AC:
AC1: not met — valid repository paths can fail target discovery; internal/agentrepo/diagnose.go:130.
AC2: met — identity, mismatch precedence, detached checkout and dirty-path coverage; internal/agentrepo/diagnose_internal_test.go:158.
AC3: met — sequential reads without retries; refusal and log request counts asserted; internal/agentrepo/diagnose_internal_test.go:112 and internal/agentrepo/diagnose_internal_test.go:268.
AC4: not met — failure-state selection is covered, but an empty link suppresses the failure report; internal/agentrepo/diagnose.go:174.
AC5: not met — legitimate nonempty no-failure sets with empty links return 2; internal/agentrepo/diagnose.go:174.
AC6: met — ordered mappings, mixed case, overlaps, unknown names and task definitions covered; internal/agentrepo/diagnose_internal_test.go:274.
AC7: met — bounded logs, distinct/shared runs and unavailable-log notices covered; internal/agentrepo/diagnose_internal_test.go:234.
AC8: met — malformed, empty and failed reads refuse successful diagnosis; internal/agentrepo/diagnose_internal_test.go:125.
AC9: met — fixtures reject unexpected commands and parity tests compare Git state; internal/agentrepo/diagnose_internal_test.go:75 and internal/agentrepo/diagnose_unix_internal_test.go:141.
AC10: not met — Go subprocess hardening exists, but compatibility fallback remains CDPATH-sensitive; packages/agents/source/skills/fix-pr/shared/scripts/diagnose.sh:11.
AC11: met — native Linux, macOS and Windows test check-runs passed; platform-neutral regressions at internal/agentrepo/diagnose_internal_test.go:84.
AC12: not met — probe, forwarding and supported failure statuses are covered, but fallback can fail under inherited CDPATH; tests/skills/fix-pr/scripts/diagnose.bats:40 and packages/agents/source/skills/fix-pr/shared/scripts/diagnose.sh:11.
DoD: 7/10; unmet: D1, D2, D10
Rationale:
D1 remains pending merged-head verification and the AC gaps above; D2 awaits independent approval of the exact merged head.
D3: all 24 reported head check-runs succeeded, including native platform tests, coverage, lint and packaging.
D4: gitleaks passed; successful lint includes the range leak scan at .github/workflows/ci.yml:79.
D5: the sole commit has verification.verified=true.
D6: command, statuses and fallback safety difference are documented at docs/agent-skills-migration.md:163.
D7: Assisted-by trailer and provenance:agent-assisted label are present.
D8: scope remains the issue’s single diagnosis migration; 22 files include generated mirrors. No numerical PR-size limit was found.
D9: no additional deferred implementation or follow-ups identified.
D10 remains pending; the live body uses “Refs #794”, so add a closing reference before merge.
Generated wrappers and frozen fallback blobs are committed consistently across both packages and deployments.
Round: 1 of 3
9ff9c96 to
746f659
Compare
There was a problem hiding this comment.
Reviewed head: 746f659
PR: #832
Model: OpenAI/GPT-6 (Codex), high
Repo criteria: none: no file
Loosens: no; no owner-tier or gate configuration paths touched
Verdict: APPROVE
Findings: none
AC:
AC1: met — explicit/inferred targets and refusals covered at internal/agentrepo/diagnose_internal_test.go:84; literal repository paths preserved at internal/agentrepo/diagnose.go:130.
AC2: met — identity, mismatch precedence, detached checkout and dirty paths covered at internal/agentrepo/diagnose_internal_test.go:158.
AC3: met — failed reads are not retried; request counts asserted at internal/agentrepo/diagnose_internal_test.go:112 and internal/agentrepo/diagnose_internal_test.go:297.
AC4: met — exact failure-state selection and status 1 covered at internal/agentrepo/diagnose_internal_test.go:179; missing-link regressions at internal/agentrepo/diagnose_internal_test.go:234.
AC5: met — nonempty passing, skipped and unfinished sets preserve no-failure wording and check display at internal/agentrepo/diagnose_internal_test.go:221.
AC6: met — ordered mappings, mixed case, overlaps, unknown jobs and defined task contracts covered at internal/agentrepo/diagnose_internal_test.go:302.
AC7: met — bounded logs, distinct/shared runs and unavailable-log notices covered at internal/agentrepo/diagnose_internal_test.go:262.
AC8: met — missing, malformed, empty and failed responses remain inconclusive at internal/agentrepo/diagnose_internal_test.go:125 and internal/agentrepo/diagnose_internal_test.go:404.
AC9: met — fixtures reject unexpected commands at internal/agentrepo/diagnose_internal_test.go:75; parity fixtures compare Git state at internal/agentrepo/diagnose_unix_internal_test.go:145.
AC10: met — subprocess environment isolation at internal/agentrepo/diagnose.go:47; hostile-environment coverage at internal/agentrepo/diagnose_internal_test.go:377 and tests/skills/fix-pr/scripts/diagnose.bats:92.
AC11: met — platform-neutral regressions at internal/agentrepo/diagnose_internal_test.go:84; exact-head Linux, macOS and Windows test check-runs succeeded.
AC12: met — thin probe/forward/fallback wrapper at packages/agents/source/skills/fix-pr/shared/scripts/diagnose.sh:5; forwarding, frozen fallback and hostile-CDPATH coverage at tests/skills/fix-pr/scripts/diagnose.bats:40.
DoD: 7/10; unmet: D1, D2, D10
Rationale:
Round 1 finding 1 fixed: optional links accepted while name/state validation remains at internal/agentrepo/diagnose.go:174; regressions at internal/agentrepo/diagnose_internal_test.go:234.
Round 1 finding 2 fixed: fallback clears CDPATH at packages/agents/source/skills/fix-pr/shared/scripts/diagnose.sh:11; regression at tests/skills/fix-pr/scripts/diagnose.bats:92.
Round 1 finding 3 fixed: only Git’s terminating newline is removed at internal/agentrepo/diagnose.go:130; trailing-space checkout regression at internal/agentrepo/diagnose_unix_internal_test.go:39.
Reviewed the changes since round 1, accounting for the updated base; no regressions found. Generated wrappers and frozen fallback blobs are committed consistently.
D3: all 24 exact-head check-runs completed successfully, including native platform tests, lint, coverage and package validation.
D4: gitleaks succeeded; successful lint includes the repository leak scan. No exposed credentials found in the reviewed changes.
D5/D7: both commits have verification.verified=true and Assisted-by trailers; provenance:agent-assisted label is present.
D6: command, statuses and fallback safety difference match docs/agent-skills-migration.md:163.
D8/D9: focused diagnosis migration across 22 files, including mirrors; no numerical PR-size limit or deferred implementation requiring follow-ups identified.
D1/D2/D10 remain pending merge verification and approval of the exact merged head; the live body now closes #794.
Round: 2 of 3
Unreadable checks mustn't look like a clean pull request. Diagnosis needs to distinguish missing evidence from known failures whose logs are unavailable while keeping the existing report and task suggestions. Refs #794 Assisted-by: codex:gpt-6-astra Signed-off-by: Tony Burns <tony@tonyburns.net>
746f659 to
380a4d7
Compare
Summary
Adds the
agentrepo checks diagnosecommand, which ports the shell diagnosis of failing pull requests to Go. The command reads the failing checks and their logs, then points each failure at the local task that reproduces it, in the existing report format. A check with unreadable evidence shows as missing evidence rather than a clean result. The shell entry point still runs the old script. No task invokes the Go command yet.Why
Epic #588 moves shell logic to Go. Diagnosis has to tell missing evidence apart from known failures whose logs are unavailable, because an unreadable check that looks clean sends the reader to merge. The port keeps the report and task suggestions that
fix-prrelies on.A stacked follow-up adds the wrapper that prefers the Go command and keeps the shell as the N-2 fallback. Splitting the port keeps each pull request under the size limit.
Consumers don't need to act. Nothing in the published payload changes, and no task invokes the command.
Verification
TestDiagnosisTargetsAndRefusals,TestDiagnoseRefusesOutsideRepositoryandTestDiagnosisDisplayRefusalcover targets and refusals.TestDiagnosisMalformedData,TestDiagnosisChecksWithoutLinksandTestDiagnosisCheckSelectioncover unreadable data, checks without links, and which checks the report includes.TestDiagnosisLogBoundsAndDeduplication,TestDiagnosisReproducers,TestDiagnosisCheckoutContextandTestDiagnosisEnvironmentAndProbecover log limits, reproducer names, checkout context, and the environment probe.go test ./internal/agentrepo ./internal/checkspassed on the head this description covers.Risk
A misreported check could point a reader at the wrong reproducer or hide a failure. No task invokes the command yet, so a defect has no effect until the wrapper follow-up merges. Reverting the commit removes the command.
Compatibility: none
Related
Part of #794.
Part of #588.