Repository navigation
Conversation
…no block An empty block says the review found nothing, so a refusal such as "Unable to review." read as a clean review and passed a clean-diff control. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
A finding body with a fenced sample put a literal fence in the JSON, which last_json() read as the block's end: the review graded as unparseable. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
…s cwd A relative tool path was taken from the workspace root, so a reach scenario started in sub/ recorded AGENTS.md for sub/AGENTS.md; shell reads already used cwd. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 6 selected item(s). Review details
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #322 +/- ##
==========================================
+ Coverage 94.00% 94.04% +0.03%
==========================================
Files 292 297 +5
Lines 37823 38027 +204
Branches 6399 6425 +26
==========================================
+ Hits 35557 35763 +206
+ Misses 1458 1454 -4
- Partials 808 810 +2
🚀 New features to boost your workflow:
|
… path Address review: posixpath kept ..\AGENTS.md as one name, so repo reach missed the guidance read. Thread: PRRT_kwDOVBg7g86yOVMM Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
pr-review, re-review at fc6c5b2 (new since 74ab336: one commit, What the PR is for: follow-ups to #116. A refusal sentence no longer reads as a clean review, a code fence in a finding body no longer drops the block, and Copilot Findings (this round)
No blocking findings. Probes: the new test fails on the 74ab336 source and passes on fc6c5b2 ( Coverage: read the one-commit delta and its callers, ran the cli_traces tests and ruff, probed edge paths on head and the parent. Not checked: the rest of the PR (reviewed at 74ab336), the full test suite. |
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at fc6c5b2. The fence escape and the Copilot cwd join are right. The "no finding and no clean verdict gets no block" rule needs two changes before it goes in: the wider verdict regex now also takes back earlier findings, and an earlier clean verdict before a "Review finished." no longer counts. Details are in the line comments.
Agreed with @napetrov: a drive-less rooted \AGENTS.md with cwd sub gives /AGENTS.md (I see the same for \ws\a.py); a POSIX name with a literal backslash is split into directories (a\b.txt gives sub/a/b.txt); the test should pin dir\f.py.
What I ran, at fc6c5b2:
make ci-local --offline: 0 failed, 19 passed, 13 skipped (network-only steps); the test suite 11,642 passed, 58 skipped.convert_answeron the same message lists at the base (1f95568) and at this head: a P1 followed by"Overall: looks good."or"Tests pass. No regressions."keeps the P1 on the base and gives[]here;["No issues found.", "Review finished."]gives[]on the base and no block here; a refusal gives[]on the base and no block here, as intended._from_cwdon~/…,C:foo,\ws\a.py,a\b.txt,dir\f.pywith cwdsub.
Checked, no issues
- Fenced bodies: the escaped block round-trips through
findings_of, and ``` decodes back to the original backtick in the message. - Copilot cwd:
view,editandapply_patchall go through_from_cwd; absolute POSIX and drive paths are left alone, and..is normalized beforerelative_path. - Docs and changelog: the contract and the (still unreleased) #116 fragment describe the new no-block case.
| + r"|(?:i )?(?:did not|didn't|could not|couldn't) (?:find|identify|spot) any " + _NOUN + _WHERE | ||
| + r"|(?:the )?(?:patch|change|commit|diff) (?:is|looks) (?:correct|good)|lgtm|looks good(?: to me)?)" | ||
| r"[ \t]*(?:[.!\n]|$)", re.I) |
There was a problem hiding this comment.
Blocking: The wider clean-verdict regex also widens the take-back at line 127, so a final "looks good" or "No regressions" now drops the findings of earlier messages.
messages=["- [P1] Leak — /w/a.py:3\n body", "Overall: looks good."] and [..., "Tests pass. No regressions."] now end in an empty ```json [] block, so a review that reported a P1 is graded as clean. On the base (1f95568) both keep the P1 finding: looks good and regressions did not match before.
Suggestion: keep the narrow regex for the take-back (a final message that says the earlier findings are gone) and use the wider one only to tell a clean review from a missing one at line 147. A test with a P1 message followed by "Overall: looks good." would catch it.
| len(findings) + unparsed, len(findings)) | ||
| return answer | ||
| if not findings and not answer.strip(): # an empty final answer is a missing review, as with one message | ||
| if not findings and not _CLEAN_VERDICT.search(texts[-1]): # an empty block is a claim the review made |
There was a problem hiding this comment.
When the final message has neither a finding nor a verdict, every message is read, but the clean-verdict check still looks only at texts[-1], which is known to hold none.
messages=["No issues found.", "Review finished."] now gets no block, so a clean review is graded as missing; on the base it got []. The docstring says such a final message "only points back at the review, so the block is built from every message".
Suggestion: if not findings and not any(_CLEAN_VERDICT.search(t) for t in texts): and a case for it in test_convert_answer_a_clean_verdict_gets_an_empty_block.
| if not findings and not _CLEAN_VERDICT.search(texts[-1]): # an empty block is a claim the review made | ||
| return answer |
There was a problem hiding this comment.
A clean answer that the verdict phrase list does not know now silently reads as a missing review.
These all get no block (checked): "I did not find any discrete, actionable bugs in this commit.", "No issues found in the changes.", "No major issues found.", "The changes look correct.". On the base each got []. Missing is the safe side, but nothing in the log says why the arm has no review, unlike the unparsed-marker case above.
Suggestion: log.warning("%s: no finding and no clean verdict in the answer; no findings block", fmt) before this return, so phrase-list gaps show up in a run.
|
|
||
| def _from_cwd(path: str, cwd: str) -> str: | ||
| """A tool's relative ``path`` joined to ``cwd``, the workspace-relative directory the CLI started in.""" | ||
| if not cwd or path.startswith("/") or PureWindowsPath(path).is_absolute(): |
There was a problem hiding this comment.
_from_cwd joins a home-relative path to the cwd.
A Copilot view of ~/.copilot/config.json with cwd sub is recorded as a read of sub/~/.copilot/config.json, a workspace path that does not exist; without a cwd it stays ~/.copilot/config.json.
Suggestion: also return path unchanged when it starts with ~.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at fc6c5b2. The fence escape and the Copilot cwd join hold up, and the refusal case now gets no block. The merge waits on points that @vlad-perevezentsev raised and that still hold at this head, and on questions to the author.
Points still open that I confirm:
I agree with @vlad-perevezentsev's four points, which still hold at this head:
- The widened clean-verdict regex also widens the take-back: a P1 followed by
"Overall: looks good."or"Tests pass. No regressions."ends in an empty block. - Only
texts[-1]is checked, so["No issues found.", "Review finished."]gets no block. - Clean answers outside the phrase list read as a missing review. New evidence:
"**No issues found.**","No actionable issues found."and"Nothing to report."each get no block. - A home-relative path is joined to the cwd:
_from_cwd("~/x", "sub")returnssub/~/x.
@napetrov's questions on \AGENTS.md (a rooted drive-less path gives /AGENTS.md) and on a POSIX name with a literal backslash (a\b.txt gives sub/a/b.txt) wait for the author, as does the request to pin dir\f.py in the test. New evidence: _from_cwd("C:foo", "sub") returns sub/C:foo, so a drive-relative path needs the same decision.
What I ran, on a Linux review host at fc6c5b2:
make ci-local: 11,641 tests passed, 59 skipped; changed-line coverage 100% (13 lines).- Reproductions of
convert_answerand_from_cwdfor the points above, with the LLM transport mocked. - A mutation run on the new code: the no-verdict check, the fence escape, the Windows conversion and the
apply_patch/viewuse are all caught. Dropping theis_absolutecheck or thestartswith("/")check in_from_cwdpasses the new test, because no case combines a Windows absolute path with a cwd.
Checked, no issues
- Fenced bodies: a finding with a fenced sample round-trips through
findings_of; at the base it returnsNone. - Refusal:
"Unable to review..."gets no block here and[]at the base. - Commits: all four carry
Signed-off-by. - Docs: the contract and the changelog fragment describe the no-block case.
Follows up #116 with fixes for the three automated review findings left on its last head.
convert_answer("codex-review", "Unable to review because the repository is unavailable.")->[]appended -> expected: no block, i.e. a missing review (contract §6: no block means no review, not a clean one). Fix: an empty block is appended only after a clean-verdict sentence (No issues found.,I did not find any issues in this change.,Looks good to me.); otherwise the answer is returned unchanged.findings_of()returnsNone-> expected: the finding. Fix: the block's JSON escapes each fence as\u0060\u0060\u0060, as the review oracles already do.viewofAGENTS.mdwith cwdsub->AGENTS.md-> expected:sub/AGENTS.md. Fix: relativeview,editandapply_patchpaths are joined to cwd, as shell reads already were.Each fix comes with a test that fails without it.