Skip to content

fix: isolate local command input and clarify audit aggregation - #145

Merged
Dong1017 merged 1 commit into
mainfrom
codex/skill-eval-local-followup
Oct 26, 2026
Merged

Dong1017 merged 1 commit into
mainfrom
codex/skill-eval-local-followup

Conversation

@Dong1017

@Dong1017 Dong1017 commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

Local commands currently inherit the caller's standard input. A scripted client can consume that stream as additional prompt text even though the executor supplies its intended inputs through arguments and files. Close standard input for these noninteractive commands, and document the contract.

The comparison example also leaves aggregation unspecified while the existing grader uses the median of per-trial request means. State that rule explicitly, including every request in each trial and reduction relative to the baseline aggregate. This matches the original pilot; report values, thresholds and grader behavior are unchanged. Previously frozen inputs and results remain historical evidence and must not be regraded with this clarification.

Validation:

  • A real parent/child subprocess regression reproduces inherited input before the fix and passes after it. No model client is simulated.
  • The new regression passes locally on Windows / Python 3.13.5; Ruff, JSON parsing and whitespace checks pass.
  • Current-head Ready CI passed all four jobs: Python 3.10 and 3.11 each report 753 passed, 9 skipped, 70 subtests passed; Ruff and both clean installed-wheel checks passed. Tested merge b743d8a combines head 5af85e3a7a614a1d153468c5f5a78b0a7e8076b4 with main 65147a03e465dc838770474736a620e04d72337e.
  • Current-head automated review completed with no findings; there are no inline review threads. This receipt is not merge permission or real-flow acceptance.
  • No real model calls were used to verify this patch.

Refs #88 and #101. This bounded follow-up uses the existing executor and comparison example. No workflow, runner, credential, execution budget or approval policy is added or changed.

Implementation assistance: Codex; exact session model ID and reasoning effort are unavailable. The commit retains DCO and truthful Agent-assisted-by attribution, with no AI co-author trailer.

Current integration status (2026-10-25): Independent Dong1017 reviewer-role software LGTM covers the exact head/base above. The user authorized conditional squash merge after independent current-head LGTM and all applicable CI passes. The per-PR knowledge-model-policy verdict is success / Not applicable, but associated declarations workflow jobs remain failed because the shared history sweep exceeds its budget (failed run). The existing repair is #140 / PR #146. Merge is held until the applicable workflow/check results pass; no checks are bypassed. #101/#88 remain open and this authorization grants no model budget.

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

chatgpt-codex-connector Bot commented Oct 25, 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-25T13:02:31.140677Z 5af85e3 PR opened
ℹ️ 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.

Copy link
Copy Markdown
Collaborator Author

@Dong1017 — independent approval-agent review requested for this bounded #101 follow-up.

  • Head: 5af85e3a7a614a1d153468c5f5a78b0a7e8076b4
  • Base: 65147a03e465dc838770474736a620e04d72337e (main)
  • Full Ready CI: all four jobs passed; Python 3.10/3.11 each 753 passed, 9 skipped, 70 subtests passed; Ruff and installed-wheel checks passed. Tested merge b743d8a.
  • Automated review reports no findings; this is not a substitute for the independent approval requested here.

Please inspect the noninteractive stdin contract across the local executor's model, verifier and external-command consumers, the real subprocess regression, and whether the example's stated aggregation matches the unchanged grader and original pilot. Preserve frozen historical inputs/results.

Please state your independent reviewer role and the checked scope. If you approve this exact head, use LGTM 5af85e3a7a614a1d153468c5f5a78b0a7e8076b4 in the review record. The Issue owner does not provide its own approval. Remaining Issue-level plan.

This is a software review request. Real addition/update acceptance, new model budgets, merge and Issue closure remain separate and on hold under the existing scope.

@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 this bounded software fix at head 5af85e3a7a614a1d153468c5f5a78b0a7e8076b4, against base 65147a03e465dc838770474736a620e04d72337e.

Reviewed all four changed files, shared execute_command callers for model/verifier/external-command execution, subprocess regression coverage, and the unchanged grader against the corrected aggregation wording. Supplying DEVNULL makes the shared command path noninteractive rather than inheriting the parent's stdin. The wording matches the existing median-of-trial-means behavior without rewriting historical graders. No actionable correctness finding in this scope; existing review discussion was checked.

Verification: Ready run 40195652657 has all four full-check/installed-package jobs green. Exact-head knowledge-model-policy reports success / Not applicable in run 40199569985. Head/base freshly re-read before posting. CI was inspected; I did not locally execute PR code or rerun model/subprocess experiments.

Limits: software-only readiness, not real addition/update E2E acceptance for #101/#88, not acceptance of exposure experiments, and not merge authorization. No model/GPU calls, workflow dispatch, merge or issue closure performed.

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

@Dong1017 Dong1017 added the area:workbench CLI workbench: profile, A/B loop, quality gates, evidence reports label Oct 26, 2026 — with ChatGPT Codex Connector
@Dong1017
Dong1017 merged commit ea7ab7d into main Oct 26, 2026
6 of 9 checks passed

Copy link
Copy Markdown
Collaborator Author

Squash-merged under the owner's explicit batch instruction. Head 5af85e3; merge ea7ab7d.

Evidence verified before merge:

The current human merge instruction proceeds on scoped review, Ready tests and the actual required policy, not an assertion that every historical declarations job is green. Software/knowledge integration only; no Issue/campaign acceptance or model/hardware efficacy claim.

@Dong1017
Dong1017 deleted the codex/skill-eval-local-followup branch October 27, 2026 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:workbench CLI workbench: profile, A/B loop, quality gates, evidence reports

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant