Repository navigation
fix(labels): exempt outside-app labels from the drift report - #343
zengchuang-hw merged 4 commits into
Conversation
The nightly safety-net job has failed since 2026-11-02 because the live `bot-reviewed` label is missing from `.github/labels.json`. That label belongs to an outside GitHub App: nothing in this repository references it, no workflow applies it, and renaming or deleting it only makes the app recreate it. Its name also cannot enter `labels.json`, whose entries must be `group:value`. Record such labels in `.github/external-labels.json` with the app that owns each one, and exclude those names from the "in the repository, missing from labels.json" report. A template or `skill_eval_*_config.json` reference to one still fails, because this repository must not depend on a label it does not own. Signed-off-by: Dong1017 <55723162+Dong1017@users.noreply.github.com> Agent-assisted-by: Claude Code / claude-opus-5 (author)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Dong1017
left a comment
There was a problem hiding this comment.
Implementation review: no blocking code finding at 4a0a17d352a7ebd683f7a7268b5567e4b3851210. The external-label policy disposition remains pending in #284.
Reviewed all four changed files against main 663f3b327998526a13aa31e72ad729025c18983c, the checker’s callers, relevant regression tests and hosted CI.
- The exemption affects only unexpected live labels. Missing managed labels, managed-description drift and unlisted template/config references still fail. The separate list is checked for duplicate names and overlap with the managed manifest before its names reach
drift(). - Tests cover external-label presence and absence, malformed declared entries, overlap/duplicates and rejection of a template reference to an external label. The nightly invocation still uses the same read-only CLI; this change performs no label mutation.
- Refreshed the head, conversation and review threads before posting. No existing inline findings require resolution; the automated review is complete on this head.
All six Ready jobs and the declaration check passed. The inspected Python 3.11 log reports Ruff success and 3,552 passed / 13 skipped / 284 subtests. Tested merge c60ee680 has no file differences from this head.
Remaining disposition: #284’s recorded owner or Issue-line delegate still needs to decide how this exception satisfies the accepted naming/live-label criteria. This review does not make that policy decision or clear the Issue’s other completion criteria.
Static inspection and existing hosted evidence only: I did not rerun tests, replay the author’s live-label snapshot or run a natural nightly. This is a non-voting COMMENT, not formal approval, merge authorization or Issue acceptance.
|
Review at head
|
Review found that `external_problems` reported a non-string `name` and then still fed it to the duplicate and overlap sets, so a list or dict name raised `TypeError: unhashable type` instead of printing a drift line. Only a usable name now joins those comparisons, and three negative cases cover it. Also state in the docs that `owner` is required but informational: no check verifies it and none can, because GitHub does not report which app created a label. Readers should confirm from the label's event history before relying on an entry. Signed-off-by: Dong1017 <55723162+Dong1017@users.noreply.github.com> Agent-assisted-by: Claude Code / claude-opus-5 (author)
|
Thanks — finding 3 was a real defect. Pushed 3. Unhashable name (fixed). It did crash. Reproduced against the previous head:
if isinstance(name, str) and name:
names.append(name)
else:
problems.append(f"{name}: name must be a nonempty string")
2. 1. Policy disposition. Correct that it is outstanding, and I will not self-clear it. The requester decided the approach on 2026-11-04 — exempt the label rather than rename or delete it — on this reasoning, which I verified before building the change:
I will record that disposition in #284 against its naming and live-label criteria, and link it here, before asking anyone to approve. Holding this PR open until that link exists is the right order — it is the same reason I have not treated the existing review as merge clearance. Re-verified at |
|
Incremental review Resolved in this delta
Still open
Optional nits (not blocking)
Static review of the delta plus hosted CI status; I did not run anything locally. |
Review noted that `name` accepted `" "` while `owner` was already checked with `.strip()`. A whitespace-only name can never match a GitHub label, so it was silently admitted as an exemption that exempts nothing. Check it the same way. Signed-off-by: Dong1017 <55723162+Dong1017@users.noreply.github.com> Agent-assisted-by: Claude Code / claude-opus-5 (author)
|
Incremental review Resolved in this delta
Still open before I can approve
Optional nit (still not blocking)
Static review of the delta plus hosted check status; I did not run anything locally. |
|
Policy disposition recorded. Your item 1 is answered: #284 issuecomment-6631265140 records the disposition against that Issue's criteria 1 and 2, and #284's status block now points at it with In short: the Your two nits. One taken, one I think rests on a wrong premise. Whitespace-only name: fixed in A whitespace-only name cannot match a GitHub label, so it was being admitted as an exemption that exempts nothing. Non-dict entry: I would rather leave it. The premise does not hold — So the two files are already symmetric, and guarding only the new one would break that. Both manifests are tracked files that change through review, so a bare string in either is a malformed-file problem rather than a drift case; if it is worth reporting, it is worth reporting for both, which is a change to existing behaviour and belongs in its own PR rather than this one. Happy to do it either way if you prefer — say which and I will follow. Re-verified at |
|
Item 1 is resolved: CI finished at
Item 2: the delegation is not recorded in a repository artifact. I would rather say that plainly than point you at something that only resembles one. What does exist:
Your concern holds regardless: this account covers both the decision owner and the agents acting for them, so nothing posted from it can be the independent acceptance you are asking for. @zengchuang-hw — you are already a requested reviewer here and a repository admin. Would you accept or reject the policy reading recorded in #284 issuecomment-6631265140? The question is narrow: does If that independent acceptance does not come, the right outcome is to close this PR rather than merge it, and to take the alternative the disposition names. The optional nit stays unchanged, for the symmetry reason in my previous comment: |
|
@david6666666 — requesting your review here as well. What the PR does: State: head The part that needs someone other than me. The policy reading behind this PR is recorded in #284 issuecomment-6631265140: that Either form of help is useful: accept or reject that policy reading, or review the code on its own terms. If the reading is rejected, the right outcome is to close this PR and instead remove the app's label-write access or have it adopt a |
|
@amy-why-3459 @zengchuang-hw can you review again? |
|
Re-review at Resolved
Before I can recommend approval
This is a non-voting re-review of the checker, its tests, and the linked records. The post-merge nightly verification remains part of the Issue follow-up. |
Validate objects before field access in both label manifests, and keep unusable names out of duplicate and overlap comparisons. Add symmetric validation and CLI regressions. Agent-assisted-by: dot (implementation) Signed-off-by: Codex <codex@openai.com>
|
@amy-why-3459 @zengchuang-hw please re-review new head
The independent-maintainer policy confirmation requested in zengchuang's review is still pending. Please include an independent acceptance/rejection and link it from #284's disposition if you can provide that decision. This code fix does not clear that blocker or complete #284; post-merge nightly verification remains outstanding. |
|
Incremental review Resolved in this delta
Still open before I can approve
Static review of the delta plus hosted check status; I did not run the suite locally. |
|
Incremental review 本轮已解决
批准前必做(Before I can recommend approval)
非阻塞问题或建议 No additional finding in this delta. Checks and limits Validation ran on Windows with Python 3.12.1, pytest 9.1.1 and Ruff 0.15.15 at the head above. Targeted pytest, Ruff and the delta whitespace check passed. I also checked the CLI callers and the target advance through Codex performed this non-voting incremental review through |
|
@zengchuang-hw can you review again, since |
|
PR review verdict — PASS for the bounded software change at Same-head re-review: Correction: I withdraw the extra requirement for a separate independent policy-confirmation comment. #284's recorded delegation and the authorized disposition already provide the scope for this change. I had incorrectly turned a request for additional clarification into a mandatory approval gate. 本轮已解决
批准前必做(Before I can approve)None. No unresolved blocking finding remains for this PR's content. No additional policy vote, clarification comment or test rerun is required by this review. 非阻塞问题或建议No additional finding. Post-merge nightly observation and whole-Issue completion remain the existing Issue follow-up. Actual checks and limitsRechecked the exact head, existing diff/review evidence, current checks, original authorization and disposition. A separate Codex agent independently reassessed that authority evidence. Reused the recorded tests because the code head is unchanged; no new local tests or live-label mutations were performed. The account holder has now directly authorized formal approval of reviewed PR content when no unresolved blocker remains. The formal review will be bound to this exact head. |
zengchuang-hw
left a comment
There was a problem hiding this comment.
LGTM
PR review verdict — PASS
Reviewed head 992eae4210f1a03ff5e134b7a80f2b008a3691d4 targeting main at e843b9d31cb86e1fd12d00890c89798b0bedd1fe. Same-head follow-up to the preceding review; the corrected re-review withdraws the extra policy-confirmation gate.
本轮已解决
- The scoped external-label exception follows the recorded Issue-line delegation and authorized disposition.
- The checker keeps managed naming/description, unknown-drift and template/config-reference enforcement. Both manifests report malformed entries before field access; invalid names do not crash duplicate/overlap checks.
- The exact-head technical review recorded 40 passing label-checker tests and 14 regressions that fail against the parent implementation, plus Ruff and whitespace checks. Current-head Ready CI and knowledge policy are successful.
批准前必做(Before I can approve)
None. No unresolved blocking finding remains in the reviewed PR content.
非阻塞问题或建议
No additional finding. Post-merge natural-nightly observation and whole-Issue completion remain the recorded Issue follow-up.
Actual checks and limits
Rechecked the unchanged head, target, original authority/disposition, earlier diff/caller/test evidence and current checks. A separate Codex agent independently reassessed the authority evidence. No new local test run, live-label mutation or nightly replay was performed for this approval.
Delegation and execution: directly delegated by the account holder zengchuang-hw, who authorized content-based formal approval when no unresolved blocker remains. Executor: Codex, acting as independent PR reviewer through the connected GitHub account zengchuang-hw. Scope: this PR's software content and supporting evidence at the exact head above. Authorized action exercised: formal Approve, bound with commit_id to that head. Merge, label changes and Issue closure are separate actions.
Problem
The nightly
safety-netjob has failed on main since 2026-11-02, on its read-only label-drift step only:Runs 41466562686 (11-02) and 41646825739 (11-03) report the same line. Both
frozen-bytesjobs pass, and the 11-03 run passed lint, 3,544 tests and the evidence checks, so this is not a test regression.bot-reviewedbelongs to an outside GitHub App:agent-ci-infra-bot[bot]created it and applies it; it is on 24 Issues and PRs.Neither obvious fix works. Renaming or deleting it makes the app recreate
bot-reviewedon its next review, so the drift returns the following night. Adding it tolabels.jsonfails that file's own contract:manifest_problemsrequires every entry to be<group>:<value>with a matchinggroupfield (tools/check_labels.py:26-31), andbot-reviewedis ungrouped.docs/agent-collaboration.mdseparately forbids duplicating review or check states with labels, which is exactly what this label records.Change
Record outside-owned labels in a separate file and exempt those names from the one report they cannot satisfy.
.github/external-labels.json(new): each entry is{"name", "owner"}, naming the app that owns the label.labels.jsonkeeps its structure and its rules unchanged.tools/check_labels.py:external_problems()validates the new file — exact fields, nonempty name, an owner that names an app, no duplicates, and no overlap withlabels.json.drift()takes the external names and excludes them from "in the repository, missing from labels.json".docs/agent-collaboration.md: states what the file is for, that these names are exempt fromgroup:valueand from the drift report because this repository neither creates them nor decides their wording, that label maintenance leaves them alone, and that they carry no coordination meaning here.Three properties are deliberately preserved:
group:valuenaminglabels.json; external labels are declared outside it, not admitted into itbot-reviewedis not one of themAn Issue template or
tools/skill_eval_*_config.jsonreference to an external label still fails, because this repository must not depend on a label it does not own.test_drift_still_rejects_a_template_reference_to_an_external_labelcovers that.Validation
Run against this branch's files, with the repository's real 22 labels fetched from the labels API:
pytest tests/test_check_labels.pyruff check tools/check_labels.py tests/test_check_labels.pybot-reviewedabsentstray-labelThe five new tests cover a valid external list, malformed entries, duplicates, overlap with
labels.json, an external label present or absent in the repository, and the template-reference rejection above.Not run: the nightly workflow itself, and no label was created, renamed or deleted. This changes no labels, automation settings or acceptance criteria.
Scope
This fixes the drift report. It records no disposition for
bot-reviewedbeyond "an outside app owns it", and it does not by itself complete the label-consistency criterion in #284; the acceptance decision stays with that Issue's owner.