feat: implement triage agent (Story 3, #126) - #279
Conversation
Site previewPreview: https://9ce2e35b-site.fullsend-ai.workers.dev Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
Multi-Agent Code Review (4 agents: Security, Quality, Gemini, Cursor)
Architecture: The three-layer pipeline (agent → schema validation → post-script) is well-designed and ADR 0022 is well-reasoned. The separation of concerns is clean and the zero-trust validation model is sound.
Critical: Post-script may run on unvalidated output
The post-script is registered as a defer in run.go and executes regardless of whether schema validation passed. When validationPassed == false, the function returns an error, but the deferred post-script fires first — meaning post-triage.sh reads the invalid triage-result.json and attempts GitHub mutations with malformed data.
This directly violates ADR 0022: "No unvalidated output is emitted. The pipeline receives a failure signal, not silently bad data."
Fix: Either move post-script execution out of defer (only run after validationPassed == true), or add a guard inside the defer closure.
Inline comments below cover the remaining findings from all 4 review agents, deduplicated and prioritized.
Full review documents:
Research/fullsend-pr279-triage-agent-security-review.mdResearch/fullsend-pr279-triage-agent-quality-review.mdResearch/fullsend-pr279-triage-agent-gemini-review.mdResearch/fullsend-pr279-triage-agent-cursor-review.md
d0f1eb9 to
c425681
Compare
- Fix ADR heading 0020→0022 and stale references in schema/scripts - Add additionalProperties: false to schema root and triage_summary per ADR 0022 zero-trust model - Guard post-script defer on validationPassed so it never runs on unvalidated agent output (critical security fix) - Prefer triage-result.json by name in validate-output-schema.sh instead of non-deterministic find ordering - Update post-triage-test.sh assertions to match labels API refactor - Add GOOS=linux GOARCH=amd64 to vendorBinaryForE2E for cross-compile Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4bc608d to
1b26cc8
Compare
- Fix ADR heading 0020→0022 and stale references in schema/scripts - Add additionalProperties: false to schema root and triage_summary per ADR 0022 zero-trust model - Guard post-script defer on validationPassed so it never runs on unvalidated agent output (critical security fix) - Prefer triage-result.json by name in validate-output-schema.sh instead of non-deterministic find ordering - Update post-triage-test.sh assertions to match labels API refactor - Add GOOS=linux GOARCH=amd64 to vendorBinaryForE2E for cross-compile Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
49b77b6 to
7c8532a
Compare
…ell safety - Post-script failure is now a hard error (propagates via named return) - Post-script defer guards on both validationPassed and runErr - Post-script requires validation_loop at harness load time (ADR 0022) - ValidateRunnerEnv accepts an expander function, removing os.Setenv - Schema: maxLength 16384 on comment field - Post-triage.sh: --body-file for shell safety, add_label fails hard - Post-triage.sh: self-duplicate guard (duplicate_of != issue number) - Pre-triage.sh: verify label state after deletion with GET - Shim workflow: concurrency group per issue (cancel-in-progress) - Agent prompt: comment content rules (no @mentions, no verbatim echo) - Agent prompt: proposed_test_case → conceptual description - VendorBinary: file size limit and path validation - Verbose flag comment about artifact upload visibility Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…agent The load-time check "post_script requires validation_loop" rejected harness/code.yaml which legitimately uses post_script without a validation_loop. The code agent's post-script handles its own output checks and doesn't need a validation loop. - Remove strict Validate() check; move the guard to runtime in run.go where it only skips post-script when validation_loop exists AND failed - Add TestHarnessesLoadAndValidate that extracts the full scaffold and loads every harness YAML through the same pipeline the runner uses - Add GOTOOLCHAIN=auto to cross-compilation env for version portability Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The upload-artifact step was skipped when the fullsend run step failed, losing the transcript needed to debug agent crashes. Adding always() ensures artifacts are uploaded regardless of the run outcome. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
waynesun09
left a comment
There was a problem hiding this comment.
Multi-Agent Review Round 5 — APPROVED
Four review agents (Security, Quality, Gemini, Cursor) all approve after verifying the hardening commit 4fc3690d and 5 supporting commits.
Hardening changes verified (13/13)
- Post-script hard error — named return propagates via defer; agent error preserved when both fail
- ValidateRunnerEnv expander —
os.Setenveliminated; custom expander function passed directly - Schema maxLength 16384 on comment field — bounds LLM output
--body-file -(stdin piping) — no temp file, no shell expansion risk- add_label fails hard —
|| trueremoved; label API failures propagate - Self-duplicate guard —
duplicate_of != issue_numberwith test coverage - Pre-triage label verification — GET after DELETE confirms removal
- Concurrency group —
triage-${{ issue.number }}with cancel-in-progress - Agent prompt content rules — no @mentions, no verbatim user input echo, no URL relay
- proposed_test_case → conceptual — better for heterogeneous repos
- VendorBinary — 100MB size limit + directory check
- Official sandbox image in triage harness
- --verbose flag for
claude --output-format stream-json
All prior fixes confirmed intact
Post-script validationPassed guard, ADR 0022 heading, additionalProperties: false, agent-result.json by name, jsonschema >= 4.18.0, category enum, labels API test assertions, GOOS=linux, issue-author re-triage, Makefile targets — all holding.
Remaining follow-ups (non-blocking)
- Bot-loop HTML marker (accepted — 3 defense layers exist)
http.DefaultClientin E2E label check (test-only)- Comment content sanitization (deferred to Story 6, #129)
The code policy enumerated specific googleapis.com subdomains but missed the regionless aiplatform.googleapis.com that Claude CLI connects to, causing a 403 policy_denied error. Use *.googleapis.com wildcard to match the triage policy and avoid future breakage from endpoint changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix ADR heading 0020→0022 and stale references in schema/scripts - Add additionalProperties: false to schema root and triage_summary per ADR 0022 zero-trust model - Guard post-script defer on validationPassed so it never runs on unvalidated agent output (critical security fix) - Prefer triage-result.json by name in validate-output-schema.sh instead of non-deterministic find ordering - Update post-triage-test.sh assertions to match labels API refactor - Add GOOS=linux GOARCH=amd64 to vendorBinaryForE2E for cross-compile Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
appdumpster/test-repo#4— agent triaged, commented via bot, post-script ranKey changes
agents/triage.md— Production triage agent prompt with clarity scoring (4 dimensions), anti-premature-resolution rule, structured JSON outputschemas/triage-result.schema.json— JSON Schema with conditional requirements (if/then) for action-specific fieldsscripts/pre-triage.sh— Strips triage labels before agent runs (label state machine reset)scripts/post-triage.sh— Deterministic GitHub mutations (comment + label + close) based on agent JSON outputscripts/validate-output-schema.sh— Generic schema validator for any agent (ADR 0020)harness/triage.yaml— Updated with pre_script, validation_loop (max 2 iterations), post_script, runner_env with FULLSEND_DIRtemplates/shim-workflow.yaml— Triage re-entry on human reply to needs-info issues (bot loop prevention)internal/cli/run.go— FULLSEND_DIR env injection for ValidateRunnerEnv, script chmod for Contents API compatibilityinternal/harness/harness.go—Scripts()method for executable permission managementinternal/forge/—ListIssueCommentsfor triage context gatheringAlso includes (cherry-picked from #277)
--vendor-fullsend-binaryflag for dev iteration without cutting releasesTest plan
make go-test— all unit tests pass (scaffold, harness, layers, forge, cli)make go-vet— cleanmake lint— cleanappdumpster/test-repo#4(run)Related
🤖 Generated with Claude Code