Skip to content

feat: plan and report skill PR evaluations with explicit authorization - #118

Open
Dong1017 wants to merge 7 commits into
mainfrom
codex/issue101-pr-skill-eval
Open

Dong1017 wants to merge 7 commits into
mainfrom
codex/issue101-pr-skill-eval

Conversation

@Dong1017

@Dong1017 Dong1017 commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Refs #101 · Parent #88

Adds a report-only PR integration around the merged #100 comparison contract. The evaluator and original pilot remain unchanged.

flowchart LR
    A["PR Git objects"] --> B["Trusted mapping + frozen plan"]
    B --> C["Operator approves versions, hash and budget"]
    C --> D["Existing executor; no publication credentials"]
    D --> E["Retained results: 30 days"]
    E --> F["Trusted publisher → one PR summary"]
Loading
  • The small mapping selects absent/new for additions and old/new for updates. Unmapped paths, deletions and unsupported snapshots remain visible coverage gaps.
  • Execution requires the authenticated operator's current repository write permission plus the exact canonical PR-plan hash, implementation/base/head revisions and complete budget. New inputs invalidate old approval/results.
  • Per-PR non-cancelling Actions concurrency, durable execution-run history and an exclusive local claim prevent duplicate execution. Failed/cancelled/uncertain attempts cannot automatically restart paid work. Authorization now requires a stable integer total_count below GitHub's filtered-search cap and exactly that many distinct run IDs. Capped, short, changing or overlapping histories fail before approval is saved.
  • A separate publisher reconstructs trusted plans, validates the source run and bounded result archive, regenerates reports and validates accepted grades with trusted code. It executes no artifact scripts and never invokes the model. Lost publication responses retry the single marked summary using retained results.
  • Counts preserve fixes, regressions, both unchanged categories, inconclusive/missing pairs and uncovered/unsupported changes. Artifacts request 30-day retention; expired results are unverified.

Usage and trust boundaries · Offline summary example

Current review snapshot: head f1c422c14792a335f7c29275b71a403b2052ccc0, base 7218d48657a84f02cd2a60d1fdbbc794cfa6d49f.

  • Main integration retains the same 10 integration files. The evaluator and original pilot remain unchanged.
  • The maintained map now includes the prospective audit-fixed-workload-claim addition and the existing inference-benchmark-design update, both using the unchanged audit-threshold holdout case. The new owner is not added by this PR. Its mapping selects nothing until a PR actually adds that skill. The two new planning controls use the real nested paths, existing canonical skill index, maintained mapping and existing case; each selects one comparison and two estimated calls, with no execution. Canonical index changes stay explicitly uncovered. Selecting both owners would require a four-call budget.
  • Local Ruff and both maintained-map planning tests passed on Windows / Python 3.13.5. The integration suite now has 47 controls. Current-head Ready CI passed all four jobs: Python 3.10/3.11 each 799 passed, 9 skipped, 70 subtests passed, Ruff and both clean installed-wheel jobs passed. Tested merge 2132ad8 contains the stated head and main base.
  • Historical Ready CI at 2ad3e4f: all four jobs passed; Python 3.10/3.11 each 797 passed, 9 skipped, 70 subtests passed, plus Ruff and both installed-wheel jobs. These are carried results for the preceding head, not new-head test counts.
  • Review of the preceding head found no major issues after the file-only artifact fix. The adapter still recreates the executor's empty results directories and preserves exclusive claims and fresh-results checks. Current-head re-review found no major issues. A subsequent software integration review explicitly gave LGTM for this exact head and found no new blocker in the reviewed increment. The fixed artifact-directory thread is resolved; real-agent acceptance and merge authorization remain separate. The earlier history-completeness review cleared that unchanged guard; none of these reviews grants real-agent acceptance or merge permission.
  • Fixed-output clients, simulated GitHub and preparation-only controls: zero real evaluation/model calls. Current-head model-access provisioning and real addition/update PR flows remain unrun.

Review fix evidence: the omitted-older-execution fixture, with reported totals of 1,000 and 1,001 but only the newest 1,000 runs returned, reproduced incorrect authorization before the fix. Both cases now reject authorization before creating approval.json; complete multi-page history still finds the older duplicate. This is an offline completeness regression, not a reproduced live duplicate-spending incident. GitHub API limit.

Real-agent trial proposal — not authorized:

Trial Fixed implementation / case source Model / effort Proposed execution limits
Skill addition Implementation candidate f1c422c14792a335f7c29275b71a403b2052ccc0; audit-threshold at the same revision gpt-6.1-sol / xhigh 2 task calls, 1 repeat, 180 s/trial, 600 s/comparison, observed-token stop 20,000
Skill update Same implementation and case; old/new skill revisions must be fixed by its actual PR gpt-6.1-sol / xhigh Same limits, separately authorized

Case-source SHA-256: aaaa1d12a88b0706355be69f8996af7dcf33bcd4f1eaf3ae0d8002b38b6075be. Default-grader source SHA-256: a11ef015770706aa33e4c801dcf31c0bb08870c80dc8aa5b8a713d049f16b7ff. The observation threshold is not a provider hard token or monetary cap; cost remains unknown.

Unfinished real-trial setup: there are no two actual acceptance PRs or frozen compared skill base/head/plan hashes yet. This PR includes the prospective audit-fixed-workload-claim mapping; it becomes trusted only after maintainer integration on main, before that skill's actual addition PR is planned. The workflow also needs to be available on main, and the current hosted jobs have no existing Codex/model provisioning. This change adds no runner, credential or repository security setting. It fails real-client preflight when unavailable and never substitutes fixed outputs. After those prerequisites are authorized and ready, regenerate both exact plans against the actual main implementation and obtain separate execution approval. The draft implementation pin above is not authorization for a future merge SHA.

Implementation assistance: Codex, GPT-6 family; exact session model ID and reasoning effort are unavailable and are not inferred from the evaluation model. Commits retain DCO and Agent-assisted-by attribution; no AI co-author trailer.

#101/#88 remain open. Final acceptance still requires real-agent addition/update flows and automatic PR feedback; neutral or negative results are valid. Ready transition is authorized for independent review. The user has authorized a conditional squash merge of #118/#145 after genuine current-head Dong1017 LGTM and all applicable checks pass. Issue closure remains unauthorized.

Current integration blocker (2026-10-25): the latest knowledge-model-policy run records history-unavailable for this head: its status-history scan exceeded the shared sweep budget. Ready CI remains green, but the latest policy status is failure, so this PR is not eligible to merge. The existing bounded-scanning repair is tracked separately in #140 / PR #146; this PR does not duplicate or bypass it. Reassess all current checks after that dependency is resolved.

Dong1017 and others added 2 commits October 23, 2026 18:12
Add revision-bound planning, explicit operator approval and retained-result PR summaries around the existing comparison tool.

Agent-assisted-by: Codex / unknown [effort=unknown] (implementation)
Signed-off-by: GUOGUO <xwdong1998@163.com>
Reject malformed input-integrity flags and count unsupported snapshot changes as coverage gaps. Remove transient configuration paths from retained plans and include a fixed-output summary excerpt.

Agent-assisted-by: Codex / unknown [effort=unknown] (implementation)
Signed-off-by: GUOGUO <xwdong1998@163.com>

@Dong1017 Dong1017 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Draft review at 6b93c54: the implementation stays within the intended thin integration layer. Inspected the workflow, adapter, existing comparison/report code, mapping and offline controls. No confirmed credential/execution-boundary bypass was found: trusted checkout, explicit operator/plan approval, isolated publication credentials, artifact path restrictions and trusted grader rechecking are present.

One duplicate-execution boundary needs qualification before paid activation: tools/skill_eval_pr.py:309–319 and the event-filtered run-history query used by authorize assume a short/empty last page proves complete history. GitHub documents a 1,000-result limit for filtered workflow-run searches. The adapter ignores total_count.

A deterministic mock with total_count=1001, only the newest 1000 returned, and an omitted older same-plan execution causes the exact pages()/claim() functions to accept a duplicate. This is a conditional completeness failure, not a reproduced live GitHub duplicate-spending incident: if GitHub instead errors beyond its cap, the current path fails closed. Please reject demonstrably incomplete/uncertain history and add that fixture, or document and verify a complete bounded retrieval method. A local completeness guard is sufficient; no new registry is requested.

Ready CI is green across full checks and clean-wheel lanes. Independent checks here were source inspection and isolated deterministic function probes, not a fresh full suite, hosted workflow or real-model exercise.

Keep the PR's draft/acceptance boundaries explicit: model provisioning and the actual addition/update-to-PR trials remain unrun. #101/#88 should remain open until their real end-to-end criteria are met with separately authorized plans and budgets. Observed-token stopping is not a hard monetary cap. No unconditional end-to-end LGTM or execution authorization is given by this review.

Reject capped, inconsistent or overlapping execution history before emitting approval. Cover the omitted older execution through the authorization entry point.

Agent-assisted-by: Codex / unknown [effort=unknown] (implementation)
Signed-off-by: GUOGUO <xwdong1998@163.com>

@Dong1017 Dong1017 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Incremental draft review at 57dffe4c7ee0e0ab7b56ea9b993177f707fb0ad5, compared with 6b93c54b82e9dbd51d48ea0729f8d59413c9cc70: the prior workflow-history completeness concern is cleared. GitHub.pages() now requires stable integer totals below the API cap, unique IDs and exact record counts; capped, short, overlapping or changing history fails before saving approval.json. Both authorization callers use the guard, and regression fixtures cover complete-history older duplicates and rejection without an approval file. No new blocker found in this bounded delta.

The four Ready jobs and current policy check are green. I inspected source/delta/callers/tests and CI status; I did not run tests or a live duplicate-spending experiment.

This is blocker clearance, not unconditional end-to-end LGTM: the PR remains draft; separately authorized real-agent addition/update flows and automatic PR feedback remain outside this offline review. No budget, merge, closure or #101/#88 acceptance is granted.

@Dong1017 Dong1017 added the area:knowledge Knowledge line: packs, skill library, knowledge admission label Oct 24, 2026 — with ChatGPT Codex Connector
Integrate the current main branch without changing the evaluation adapter, comparison contract or workflow.

Agent-assisted-by: Codex / unknown [effort=unknown] (integration)
Signed-off-by: GUOGUO <xwdong1998@163.com>
@Dong1017
Dong1017 marked this pull request as ready for review October 24, 2026 13:50
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-24T14:42:13.481768Z f1c422c Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eba5942db6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread tools/skill_eval_pr.py
Recreate empty result directories in the PR adapter before invoking the unchanged executor. Cover addition and update through a file-only artifact round trip while retaining duplicate-attempt rejection.

Agent-assisted-by: Codex / unknown [effort=unknown] (implementation)
Signed-off-by: GUOGUO <xwdong1998@163.com>

Copy link
Copy Markdown
Collaborator Author

@codex review

Please re-review 2ad3e4f, especially the file-only plan-artifact round trip and retained duplicate-execution safeguards. Both addition/update reproductions now pass with the existing fixed-output client; the focused suite has 3 passing tests and Ruff passed. Current-head Ready CI is running. Real-agent addition/update execution and automatic feedback remain blocked on trusted-main activation, the addition mapping, and an authorized real-model environment; no model budget or merge is authorized.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: 2ad3e4fdd8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Reserve the fixed-workload audit owner in the existing PR mapping. Verify the maintained nested paths select absent/new or old/new without model execution and keep canonical index changes uncovered.

Agent-assisted-by: Codex / unknown [effort=unknown] (implementation)
Signed-off-by: GUOGUO <xwdong1998@163.com>
State the combined budget when both mapped owners change.

Agent-assisted-by: Codex / unknown [effort=unknown] (documentation)
Signed-off-by: GUOGUO <xwdong1998@163.com>

Copy link
Copy Markdown
Collaborator Author

@codex review

Please review f1c422c, specifically the bounded map increment for #88's planned addition acceptance. It reuses the existing audit-threshold case, model settings, limits, adapter and workflow. Both actual nested-path planning controls pass: absent/new for the prospective owner and old/new for inference-benchmark-design; each schedules two calls without execution, and canonical index changes remain uncovered. Ruff passed. Current-head Ready CI is pending: https://github.com/InferMatrixLab/InferRSI/actions/runs/40103515211. This does not add the skill, dispatch evaluation, provision model access or authorize a merge.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: f1c422c147

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@Dong1017 Dong1017 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

LGTM for the software integration increment at f1c422c14792a335f7c29275b71a403b2052ccc0, compared with previously inspected 57dffe4c7ee0e0ab7b56ea9b993177f707fb0ad5; reported base 7218d48657a84f02cd2a60d1fdbbc794cfa6d49f.

The file-only artifact round-trip fix restores missing results directories before the existing executor, while exclusive execution claims and nonempty-results rejection remain intact. Addition/update regression fixtures cover this boundary and duplicate execution. The prospective addition mapping reuses the existing case, reports two calls per comparison (four if both owners are selected), and does not execute or add a skill. The previously cleared history-completeness guard is unchanged. No new blocker found in this delta and affected callers/tests.

All four exact-head Ready jobs pass; the per-head knowledge-model-policy status is success / Not applicable. Verification was source/delta and CI-status inspection, not independently executed tests or model calls.

This is software review only. #101/#88 still require separately authorized real-agent addition/update workflows and automatic live PR feedback. This LGTM grants neither model budget nor merge/closure/goal acceptance.

@Dong1017 Dong1017 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Readiness delta at unchanged head f1c422c14792a335f7c29275b71a403b2052ccc0 (base 7218d48657a84f02cd2a60d1fdbbc794cfa6d49f): the earlier software-scope LGTM remains, but its then-green policy observation is no longer current.

The latest exact-head knowledge-model-policy status is failure, Status history could not be read, in run 40199569985, rechecked immediately before posting. Current merge readiness is blocked until the per-head prerequisite is verified again. This is not a new declaration-noncompliance finding; scanner availability/progress is tracked by #140/#146. Do not bypass or treat this as green because software CI passed.

No source re-review or resolved concern reopened on this unchanged head. Real #101/#88 addition/update acceptance remains separate and ungranted by this software review. No merge, workflow dispatch or model execution performed.

Reviewer: dot / OpenAI assistant (review); exact model identifier is not exposed in this runtime.

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

Labels

area:knowledge Knowledge line: packs, skill library, knowledge admission

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant