Skip to content

docs(#3482): add review guidance for app-next legacy removal - #3483

Open
fullsend-ai-coder[bot] wants to merge 1 commit into
mainfrom
agent/3482-app-next-legacy-removal-guidance
Open

docs(#3482): add review guidance for app-next legacy removal#3483
fullsend-ai-coder[bot] wants to merge 1 commit into
mainfrom
agent/3482-app-next-legacy-removal-guidance

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Add a section to AGENTS.md under E2E Best Practices that instructs the review agent to check docs/nfs-e2e-triage.md when a PR adds an
-app-next Playwright project alongside an existing legacy project.
If the workspace NFS migration is complete, the agent should flag that the legacy project can be removed to halve CI cost and avoid cross-project race conditions. If migration status is unclear, the agent should ask the author.

This addresses the gap where PR #3342 added an orchestrator app-next lane but 12 review passes never questioned whether the legacy lane was still needed, a human reviewer caught it instead.


Closes #3482

Post-script verification

  • Branch is not main/master (agent/3482-app-next-legacy-removal-guidance)
  • Secret scan passed (gitleaks — bfd62685fc1b7ab86c3912756d33f9095ebf22f9..HEAD)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

Add a section to AGENTS.md under E2E Best Practices that instructs
the review agent to check docs/nfs-e2e-triage.md when a PR adds an
-app-next Playwright project alongside an existing legacy project.
If the workspace NFS migration is complete, the agent should flag
that the legacy project can be removed to halve CI cost and avoid
cross-project race conditions. If migration status is unclear, the
agent should ask the author.

This addresses the gap where PR #3342 added an orchestrator app-next
lane but 12 review passes never questioned whether the legacy lane
was still needed, a human reviewer caught it instead.

Closes #3482
@openshift-ci

openshift-ci Bot commented Aug 31, 2026

Copy link
Copy Markdown

Hi @fullsend-ai-coder[bot]. Thanks for your PR.

I'm waiting for a redhat-developer member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@sonarqubecloud

Copy link
Copy Markdown

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 31, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 4:55 PM UTC · Completed 5:00 PM UTC

Commit: 281b195 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $0.76

@fullsend-ai-review

Copy link
Copy Markdown

Review — approve

Summary

Small, focused documentation change to AGENTS.md that adds review guidance under the E2E Best Practices section. The new "Adding app-next Playwright projects" subsection instructs review agents to check docs/nfs-e2e-triage.md when a PR adds an -app-next Playwright project alongside an existing legacy project, and to flag legacy project removal when the NFS migration is complete.

Motivation is well-documented: PR #3342 added an orchestrator-app-next lane alongside the existing legacy project, and 12 review passes approved it without questioning whether the legacy lane was still needed — a human reviewer caught it. This guidance fills that gap.

Findings

# Severity Category File Description
1 low documentation-accuracy AGENTS.md:383 Terminology mismatch: guidance references "listed as NFS-complete" but docs/nfs-e2e-triage.md does not use that term

1. Terminology mismatch with triage document (low)

File: AGENTS.md, line 383 (step 2 of the new section)

The guidance says:

Check docs/nfs-e2e-triage.md to determine whether the workspace's NFS migration is complete.
If the workspace is listed as NFS-complete (all tests passing on the app-next lane)...

The triage document does not use the term "NFS-complete" or contain a field with that name. The relevant columns are:

  • NFS readiness — values like ready, backend-only, no NFS entry point (capability, not completion)
  • NFS lane — values like , ✅ (NFS-only), ✅ (skipped), (existence, not test-pass status)

A reviewer following step 1 literally would search the document for an "NFS-complete" listing and not find one. The parenthetical "(all tests passing on the app-next lane)" clarifies the intent, but "all tests passing" is a CI status question — not something the triage document tracks.

Suggested remediation: Reword to reference the actual columns, e.g.: "Check docs/nfs-e2e-triage.md §3: if the workspace's NFS readiness is ready and the NFS lane column shows or ✅ (NFS-only), the legacy project is likely removable."

This is non-blocking — the guidance is directionally correct and provides enough context for a capable reviewer to make the right call.

Verified

  • Referenced file docs/nfs-e2e-triage.md exists at the expected path
  • The triage document contains a per-workspace table (§3) with NFS readiness and NFS lane columns
  • The new section is correctly placed under ## E2E Testing at the ### heading level
  • PR is appropriately scoped (single file, documentation only)
  • No injection patterns detected in PR body or diff content
  • Linked issue Add AGENTS.md guidance: flag legacy project removal when adding app-next lanes #3482 is real and describes the gap this PR addresses

Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

  • AGENTS.md

Labels: PR modifies only AGENTS.md (documentation/review guidance)

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

Comment thread AGENTS.md
### Adding app-next Playwright projects

When a PR adds an `-app-next` Playwright project to a workspace that already has a legacy (non-app-next) project:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] documentation-accuracy

Guidance references checking if a workspace is "listed as NFS-complete" in docs/nfs-e2e-triage.md, but that document does not use the term "NFS-complete" anywhere. The closest signals are the "NFS readiness" column (values like "ready") and the "NFS lane" column (✅, ✅ (NFS-only), etc.). A reviewer following step 1 would need to synthesize multiple columns since no single field says "NFS-complete."

Suggested fix: Reword to reference actual triage doc columns, e.g.: "Check docs/nfs-e2e-triage.md §3: if the workspace NFS readiness is ready and the NFS lane column shows ✅ or ✅ (NFS-only), the legacy project is likely removable."

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment documentation Improvements or additions to documentation non-workspace-changes PR changes files outside workspace directories labels Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation needs-ok-to-test non-workspace-changes PR changes files outside workspace directories requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add AGENTS.md guidance: flag legacy project removal when adding app-next lanes

0 participants