-
Notifications
You must be signed in to change notification settings - Fork 15
Description
Summary
The PR reviewer currently appears to load only the repository-root AGENTS.md (plus top-level project skills), not the closest package-specific AGENTS.md files for changed code.
That seems to explain why it approved a PR that introduced a breaking REST API contract change in software-agent-sdk without flagging the package-specific policy that should have applied.
Answer to the question
Why didn't it cry on the ACP PR? Which AGENTS.md did it get?
From the current implementation and the historical workflow logs, the reviewer got the repo-root AGENTS.md as the agents skill, but did not get openhands-agent-server/AGENTS.md.
So it had generic repo-wide backward-compatibility guidance, but it likely did not have the package-specific agent-server REST API compatibility/deprecation policy in context when reviewing the ACP PR.
Evidence
1) Current/future reviewer plugin in extensions runs from repo root and loads project skills from there
plugins/pr-review/action.yml:134-137cd pr-repopython ../extensions/plugins/pr-review/scripts/agent_script.py
plugins/pr-review/scripts/agent_script.py:747-755- calls
load_project_skills(cwd)
- calls
2) load_project_skills() only loads third-party files from the working dir and git repo root
In openhands-sdk/openhands/sdk/context/skills/skill.py:793-868, load_project_skills(work_dir):
- loads third-party files like
AGENTS.mdfromwork_dir - if different, also from the git repo root
- does not walk changed-file paths and load the nearest package-level
AGENTS.md
So when the reviewer runs from repo root, it picks up:
- root
AGENTS.md - top-level
.agents/skills/...
But not something like:
openhands-agent-server/AGENTS.md
3) Historical PR-review log for the ACP PR confirms only root-level agents was loaded
Historical review run for software-agent-sdk PR #2190 (Enable ACPAgent on RemoteRuntime API):
- Run:
22486322088 - Log line shows:
Loaded 6 project skills: ['agents', 'run-eval', 'custom-codereview-guide', 'write-behavior-test', 'debug-test-examples-workflow', 'feature-release-rollout']
There is no package-specific skill corresponding to openhands-agent-server/AGENTS.md.
4) The current reviewer still behaves the same way
Current PR-review run for software-agent-sdk PR #2433:
- Run:
23075160009 - Log line shows:
Loaded 7 project skills: ['agents', 'design-principles', 'run-eval', 'custom-codereview-guide', 'write-behavior-test', 'debug-test-examples-workflow', 'feature-release-rollout']
Again, only root-level agents is visible in the loaded skill list.
Why this matters
In the ACP case, the specific policy lived in openhands-agent-server/AGENTS.md, not only in the repo root.
That package-specific file spelled out the REST API compatibility/deprecation expectations. If the reviewer never loaded that file, it had no strong repo-native policy signal telling it to object to the in-place breaking REST contract change.
Suggestions to fix
Option 1: Load the closest AGENTS.md files for changed paths
When reviewing a PR, compute the changed files and load:
- the repo-root
AGENTS.md - plus the nearest ancestor
AGENTS.mdfor each changed file
Example:
- if a PR changes
openhands-agent-server/openhands/agent_server/api.py, include:AGENTS.mdopenhands-agent-server/AGENTS.md
This seems like the highest-value fix.
Option 2: Load all ancestor AGENTS.md files along each changed-file path
If we want nested overrides to compose naturally, load all ancestor AGENTS files from repo root down to the closest one, with deeper files taking precedence.
Option 3: Feed CI/check summaries into review context before approving
The ACP PR later failed the OpenAPI breakage check. The reviewer should probably see:
- failed check names
- bot summary comments for API/OpenAPI breakage
- relevant workflow conclusions
At minimum, a reviewer should not issue an approval if there is already a matching API breakage signal on the PR.
Option 4: Add a pre-approval guard
If any of these are true, avoid APPROVE and downgrade to COMMENT/REQUEST_CHANGES:
- API/OpenAPI breakage checks failed
- package-specific AGENTS policy indicates a required deprecation/migration path
- breaking API change detected in diff + no evidence of migration/versioning
Option 5: Add a regression test for nested AGENTS loading
Add a test around the PR review context builder that verifies:
- root
AGENTS.mdis loaded - closest package-level
AGENTS.mdis also loaded for changed files - the deeper package file is present in the final prompt/context
Notes
The current software-agent-sdk in-repo reviewer appears to have the same limitation, but the plugin in extensions is the right place to fix if it is becoming the canonical reviewer.