Skip to content

[Follow-up] Update the chatgpt-codex-connector codebase to add (PR #1387) #1402

Description

@stranske

Fix .agents/** exclusion: enforce connector filtering and per-comment dismissal

Why

PR #1387 addressed issue #1385, but verification returned CONCERNS because .agents/** is not fully excluded from bot review comment generation and the dismissal workflow/script integration is incomplete. This follow-up issue closes the remaining gaps by (1) enforcing .agents/** filtering at the point where review comments are created, and (2) ensuring the reusable bot comment handler dismisses only the individual comments on ignored paths (with correct pattern matching, age filtering, and structured logging), while keeping the diff tightly scoped.

Source

Tasks

Connector Configuration

  • Add .agents/** pattern to the connector's ignored_paths configuration file or settings object
  • Implement filter logic in the file-selection code path that runs before review comment construction
  • Apply the ignored_paths filter to exclude matching files from the review comment generation pipeline
  • Write unit tests that verify .agents/** files are excluded from the file selection results

Dismissal Script Enhancement

  • Replace string prefix checks with glob pattern matching using minimatch or equivalent library in bot-comment-dismiss.js
  • Implement pattern matching logic that correctly handles nested paths like .agents/a/b/c.yml
  • Add unit tests verifying glob patterns match nested paths under .agents/ correctly
  • Add negative test cases confirming non-matching paths are not incorrectly dismissed

Per-Comment Dismissal Logic

  • Modify dismissal logic to iterate through individual review comments rather than dismissing entire reviews
  • Implement path filtering that checks each comment's path field against ignored_paths patterns
  • Add logic to skip dismissal for comments whose paths do not match ignored patterns
  • Write integration tests for mixed-path reviews with both ignored and non-ignored file comments

Structured Logging

  • Add structured logging for each dismissed review comment by invoking formatDismissLog() for every dismissal
  • Ensure the log includes bot name and file path for each dismissed comment

Scope Cleanup

  • Identify and revert changes related to verify-compare evaluation logic modifications
  • Remove changes related to chain depth tracking functionality additions
  • Revert ledger validation caching implementation changes
  • Remove dependency version bumps unrelated to the connector filtering feature

Deferred Tasks (Requires Human)

The following tasks require modification of protected workflow files and must be completed by a maintainer with appropriate permissions:

  • MANUAL: Remove inline dismissal code blocks from .github/workflows/agents-bot-comment-handler.yml
  • MANUAL: Add a workflow step that invokes bot-comment-dismiss.js script using node
  • MANUAL: Pass maxAgeSeconds as an explicit argument or environment variable to the dismissal script
  • MANUAL: Configure the script invocation to target only individual review comments on ignored paths

Implementation guide for workflow changes:

# In .github/workflows/agents-bot-comment-handler.yml
# Replace inline dismissal blocks with:
- name: Dismiss ignored path comments
  run: |
    node .github/scripts/bot-comment-dismiss.js \
      --maxAgeSeconds=${{ inputs.maxAgeSeconds }} \
      --ignoredPaths=".agents/**" \
      --pr=${{ github.event.pull_request.number }}

Acceptance Criteria

Connector Filtering

  • The connector's ignored-paths configuration includes an entry that matches all files under .agents/ (e.g., .agents/** or equivalent supported pattern)
  • The filter is applied in the code path that selects files for review comment generation (i.e., before any review comment is constructed/posted)
  • Given an input file list containing .agents/issue-test-ledger.yml and src/app.ts, the connector's file-selection logic returns src/app.ts and excludes .agents/issue-test-ledger.yml

Script Integration

  • The script .github/scripts/bot-comment-dismiss.js accepts maxAgeSeconds as an explicit input argument
  • When maxAgeSeconds is provided, the dismissal logic only dismisses individual review comments whose created_at timestamp is newer than now - maxAgeSeconds
  • Older comments are left unchanged when maxAgeSeconds filtering is active

Pattern Matching

  • Ignored-path matching supports patterns that match nested paths under .agents/ (e.g., .agents/** matches .agents/a/b/c.yml)
  • Pattern matching does not rely on simple prefix-only string checks
  • Unit tests verify that .agents/nested/deep/file.yml matches .agents/** pattern
  • Unit tests verify that src/agents/file.ts does NOT match .agents/** pattern

Per-Comment Dismissal

  • For a mixed-path GitHub review containing at least two review comments—one on an ignored path .agents/issue-test-ledger.yml and one on a non-ignored path src/app.ts—the script dismisses only the ignored-path comment
  • The script does not dismiss the entire review object when only some comments match ignored paths
  • The script does not dismiss non-ignored comments in mixed reviews

Logging

  • Each dismissed review comment produces exactly one structured log entry via formatDismissLog()
  • Each log entry includes (a) the bot identity and (b) the exact file path of the dismissed comment

End-to-End Validation

  • Given a test PR with changes to .agents/issue-test-ledger.yml, the dismissal script successfully dismisses all matching review comments when invoked
  • Querying the GitHub API for remaining non-dismissed comments on .agents/** paths returns zero results after script execution

Scope Control

  • The PR modifies only the following files: (1) files under chatgpt-codex-connector/ related to ignored_paths filtering, (2) .github/scripts/bot-comment-dismiss.js, and (3) test files with names matching **/test/**/ignore* or **/test/**/dismiss*
  • No other files are modified

Implementation Notes

Connector Filtering

  • Add .agents/** (or equivalent supported pattern) to the connector's ignored-paths configuration
  • Ensure the ignore filter is applied at the review comment generation file-selection step (not only PR context gathering)
  • The filter must run before comment payloads are constructed or any review/comment creation is attempted
  • Add/extend connector unit tests to stub the changed-files list and assert .agents/** is excluded while normal source files remain included

Dismissal Script Wiring

  • Ensure maxAgeSeconds is explicitly accepted as a command-line argument or environment variable in the script
  • The script must use it to filter eligible comment dismissals by created_at

Matching + Per-Comment Dismissal Behavior

  • Implement ignored path matching using proper glob/regex matching supported by the script's dependencies (e.g., minimatch) rather than string prefix checks
  • Dismiss only the individual review comment(s) whose path matches the ignored patterns
  • Do not dismiss entire reviews, and do not dismiss non-ignored comments in mixed reviews
  • Add tests for:
    • glob matching correctness for .agents/** (including nested paths and negative cases)
    • age window behavior via maxAgeSeconds
    • mixed-path reviews to ensure only targeted comment IDs are dismissed
    • structured logging: formatDismissLog() invoked once per dismissal and includes bot login + exact file path

Scope Control

  • Revert/remove unrelated changes from the prior attempt (evaluation tweaks, caching, dependency bumps, etc.)
  • The follow-up PR should only touch:
    • chatgpt-codex-connector/**
    • .github/scripts/bot-comment-dismiss.js
    • plus directly-related test files only
Background (previous attempt context)
  • Reliance on guidance text in copilot-instructions.md for Copilot exclusion failed because textual guidance does not enforce configuration and does not prevent Copilot from generating review comments. Implement an enforced connector filter and/or a fallback auto-dismiss mechanism so any comments produced are removed quickly.
  • Not wiring bot-comment-dismiss.js into the workflow failed because, although the script exists, it wasn't integrated into the reusable bot comment handler. Ensure the workflow calls the script and uses maxAgeSeconds to dismiss individual review comments with proper logging.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions