fix(issues): enforce formatter acceptance validation - #2999
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe formatter now loads the canonical issue-format validator, repairs missing acceptance gates, validates fallback output, and reports refinement status. The consumer template also supports idempotent reuse and Original-Issue normalization. Tests cover these behaviors. ChangesIssue formatter contract
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant IssueFormatter
participant CanonicalValidator
participant LLM
participant ReuseMarker
IssueFormatter->>ReuseMarker: Check uncapped issue body
ReuseMarker-->>IssueFormatter: Return reusable or new body state
IssueFormatter->>LLM: Format non-reusable issue body
LLM-->>IssueFormatter: Return formatted issue
IssueFormatter->>CanonicalValidator: Validate fallback or generated output
CanonicalValidator-->>IssueFormatter: Return validation result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Runner dispatch state for autofix on PR #2999. Do not edit. |
🤖 Keepalive Loop StatusPR #2999 | Agent: Codex | Iteration 0/12 Current State
🔍 Failure Classification| Error type | infrastructure |
|
Keepalive Work Log (click to expand)
|
|
Runner dispatch state for codex on PR #2999. Do not edit. |
|
Autofix updated these files:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2aba350d5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "- [ ] Run `python3 -m pytest tests/scripts/test_issue_formatter.py` after formatting; " | ||
| "it passes, with the command output captured in the PR validation evidence." |
There was a problem hiding this comment.
Preserve task-specific acceptance gates
When fallback formatting handles an issue whose acceptance criteria lack a recognized gate, this adds a formatter-repository test rather than a verification step for the requested work. For example, an issue asking to update src/client.py now passes the canonical validator solely because tests/scripts/test_issue_formatter.py is named, even though that test cannot verify the client change—and the path does not exist in typical consumer repos. This fabricates readiness and can let automation treat work as complete without any relevant acceptance gate; derive a gate from the issue or leave it non-conforming for refinement instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR tightens the contract between the issue-body formatter (scripts/langchain/issue_formatter.py) and the fleet’s canonical validator (.github/scripts/issue_format.py) so the formatter cannot claim output is valid when it would be rejected by the same guard used elsewhere in the automation.
Changes:
- Dynamically loads and reuses the canonical issue-format validator module (including
GATEandvalidate()), instead of re-implementing the gate logic. - Ensures fallback-formatted Acceptance Criteria gets a verification-gate match when missing, and updates
_formatted_output_valid()to fail unlessissue_format.validate(...).okis true. - Expands formatter tests to assert canonical validation behavior and acceptance-gate behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/langchain/issue_formatter.py | Loads canonical validator; injects an acceptance gate when missing; _formatted_output_valid() now uses canonical validate().ok. |
| tests/scripts/test_issue_formatter.py | Adds coverage for acceptance-gate injection and for _formatted_output_valid() enforcing the canonical validator contract. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| acceptance_text = ( | ||
| f"{acceptance_text}\n" | ||
| "- [ ] Run `python3 -m pytest tests/scripts/test_issue_formatter.py` after formatting; " | ||
| "it passes, with the command output captured in the PR validation evidence." | ||
| ) |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/langchain/issue_formatter.py`:
- Around line 373-379: Update the formatter flow around
_issue_format_validator() to validate the complete body only after every
formatting transformation, including the acceptance-text update, and fail
explicitly when canonical validation fails rather than returning invalid
content. In tests/scripts/test_issue_formatter.py lines 135-136, assert
canonical validity after the production fix or the explicit failure for
placeholder input; at lines 248-252, assert canonical validity for the final LLM
result after raw-issue and reuse-marker processing.
In `@tests/scripts/test_issue_formatter.py`:
- Around line 71-86: Update
test_format_issue_fallback_adds_acceptance_gate_when_only_tasks_have_verify_hint
to assert that acceptance contains the source prose “Formatter preserves the
source acceptance prose.”, alongside the existing command and validation
assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 504b90bf-4ef9-496f-b623-d0b1fc52070b
📒 Files selected for processing (2)
scripts/langchain/issue_formatter.pytests/scripts/test_issue_formatter.py
|
Addressed the two current formatter findings in head |
Provider Comparison ReportProvider Summary
📋 Full Provider Details (click to expand)openai
anthropic
Agreement
Disagreement
Unique Insights
🔍 LangSmith Traces |
Closer verifier disposition (split report → close)Provider report on this merged PR is split: openai gpt-5.6-terra PASS (86%) vs anthropic claude-sonnet-5 CONCERNS (45%). Audited anthropic concerns against current
Disposition: anthropic CONCERNS are false-positive / checklist-and-path noise relative to merged code. Source issue #2992 will be closed on this PASS + audit. No bounded follow-up PR required for the stated P1 defect. |
Closes #2992
Automated Status Summary
Scope
scripts/langchain/issue_formatter.py(the generator) and its contract with.github/scripts/issue_format.py(the validator). The goal is that a body the formatter returns as valid is a body the validator accepts, so the fleet's own generator cannot file work that the format guard must reject.Context for Agent
Related Issues/PRs
Tasks
scripts/langchain/issue_formatter.py, after buildingacceptance_linesat:346, detect whether the assembled Acceptance Criteria section satisfies the same gate the validator applies. Import and reuse the single fleet definition rather than re-implementing the pattern:.github/scripts/issue_format.pyis documented in.github/sync-manifest.yml:226as the "Single fleet definition of agent-processable ... Do not fork per repo."AGENT_ISSUE_FORMAT.md's live-verification substitution, so they get a gate, not an exemption._formatted_output_validatscripts/langchain/issue_formatter.py:387-391so section presence is not the whole check: run the body through the shared validator and return False when it does not pass, so a non-conforming body cannot be returned as valid.Falseresult to the caller so the opener does not file it, instead of emitting a body that guarantees a guard loop.Acceptance criteria
pytest tests/langchain/test_issue_formatter.py::test_acceptance_criteria_always_carries_a_verification_gatepasses. The test feeds the formatter a source whose acceptance criteria are pure prose with(verify: ...)hints present only under Tasks (the exact#464shape), then asserts the formatted output satisfiesissue_format.validate(...).ok is Trueand thatGATE.search(<Acceptance Criteria section>)is truthy.acceptance_textto the rawjoin_or_placeholder(acceptance_lines, ...)pass-through atscripts/langchain/issue_formatter.py:357, confirmtest_acceptance_criteria_always_carries_a_verification_gateFAILS on theGATE.searchassertion, then revert and confirm it passes again.python3 .github/scripts/issue_format.py <formatter output>exits 0 for every fixture in the formatter test suite, including the docs-only fixture.Summary by CodeRabbit
Bug Fixes
Tests