feat(cli): draft SECURITY.md for owner review - #536
Conversation
…/codex/shared-codex-session
…/codex/security-policy-drafts
|
@codex review Please review the current head, |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review Please review the current head, |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 8c6eaccccc
Only the user who started this review can view the report in Codex.
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
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. |
|
@codex review Please review the current head, |
|
@codex security review Please review the current head, |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
kmbroai
left a comment
kmbroai
left a comment
There was a problem hiding this comment.
Reviewed head ffa18764bc4aa581b112c7c7899f66167d172503 for correctness, necessity, and simplification.
Is this feature necessary?
This is optional product work, not a prerequisite for scan correctness. The repository already ships a define-security-policy skill; this adds a separate three-turn orchestration, saved manifest and evidence lifecycle, owner-question UI, cost accumulation, SDK exports, and CLI output/error handling. That is a substantial permanent surface for drafting one owner-reviewed document.
My recommendation is to justify the three-stage workflow with representative output-quality/cost evidence against invoking the existing skill, and otherwise start with a narrower draft/preview entry point. Keep the architecture and threat-model documents as private supporting outputs, not new approval artifacts or mandatory user steps. Avoid exposing low-level target resolution and every orchestration concept as public API until there is a concrete consumer. Owner decisions must remain review notes, not inferred exclusions.
Correctness at this head
The earlier broad host-read finding is addressed in source. The policy profile uses :minimal plus explicit readable roots, disables plugins/apps/MCP/network, suppresses ambient project instructions, and rechecks the checkout/Git binding before starting the model work. Policy writes are performed by the host into an external private artifact directory; the generation API does not install the draft in the repository.
I also traced inherited-policy snapshots, strict UTF-8/size handling, custom-plugin selection, cancellation between stages, partial-document preservation, and cost aggregation across the three turns. I did not reproduce a new functional blocker. Source files other than the policy checkpoints are not frozen; the documentation correctly tells users to regenerate after relevant source changes. Do not describe the saved draft as a reproducible snapshot of the whole repository.
Simplification and verification
Share the existing runtime/authentication/observer machinery, but keep policy-specific orchestration outside the already large scan API where possible. Integrate the canonical-state work from #481 without creating another variant of path validation. Keep #489's repository-writing lifecycle separate from this draft-only feature so accepting generation does not implicitly accept the much riskier apply/recovery surface.
Ran the focused policy and API-policy suites: 71 passed, 0 failed, using cached dependencies. Those tests validate orchestration with synthetic model responses; they do not establish policy quality, source-citation accuracy, comparative three-stage value, or native sandbox behavior on every platform. I would narrow or defer the feature until that product justification is clear, even though the focused correctness checks passed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07f656b6bd
ℹ️ 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".
| const result = await options.run(stage, `${common}\n\n${instructions}`); | ||
| signal.throwIfAborted(); | ||
| const hasDocument = result.markdown.trim().length > 0; | ||
| if (hasDocument) await writePolicyArtifact(path, result.markdown, signal); |
There was a problem hiding this comment.
When the policy stage returns Markdown that exceeds the 1 MiB contract or contains an unpaired surrogate, this writes SECURITY.md before validatePolicyContent() rejects the response at line 801. The command therefore reports failure but leaves an invalid artifact behind—and invalid Unicode may already have been encoded as replacement characters—so callers can mistake the saved file for a reviewable partial result. Validate the policy-stage Markdown before calling writePolicyArtifact.
Useful? React with 👍 / 👎.
| "Git metadata changed during security-policy validation. Retry with a stable checkout.", | ||
| ); | ||
| } | ||
| return [...new Set([target.repository, ...binding.metadata])]; |
There was a problem hiding this comment.
For a linked worktree or checkout using a separate Git directory, this explicitly grants every policy turn read access to both the worktree Git directory and the common Git directory outside the selected checkout. Those directories can contain credential-bearing remote URLs and objects for deleted files, other branches, or other worktrees, so repository-controlled prompt injection can cause data outside the selected source tree to enter the inference turn. Fresh evidence beyond the earlier whole-filesystem report is that the narrowed profile still adds the common Git object/config store as a readable root; keep Git inspection host-side rather than exposing these metadata directories to the model.
AGENTS.md reference: sdk/typescript/AGENTS.md:L8-L12
Useful? React with 👍 / 👎.
2 participants
Summary
Add
codex-security policy, which drafts a source-backedSECURITY.mdwithout changing the checkout. It saves architecture, threat-model, and policy documents outside the repository and shows the exact policy diff for owner review.This PR covers generation and preview. Stacked #489 adds saved-draft review and application.
Changes
policy [repository](default: current directory), with--pathfor a component, interactive owner questions,--headless, local--dry-run, and Markdown or structured output. Existing authentication, model, knowledge-base and cost settings apply.Testing
pnpm run types,pnpm run format,pnpm run build, andgit diff --check: passed.12345: 1,996 passed, 33 skipped, one sandbox-blockedpscheck. The exact process-group test passed outside the sandbox.Final main refresh (
01bd062): 439 affected tests passed with 18 platform skips;the final configuration rerun passed 24 tests with two outer-sandbox skips.
Both native macOS scan/policy permission tests then passed outside the outer
sandbox. The policy probe reproduced a denied runtime launch before its fixture
was corrected; it now verifies permitted reads and denied writes with Python,
without widening the production permission profile.
Types/models, formatting, build, static package checks and full installed smoke
passed again. The bundle matches main's 0.1.60; the real 0.1.59 cache upgrade
matched all 118 files and preserved credentials. Native Windows/Linux and live
model-quality evaluation remain unverified. Unsupported native sandbox backends
are treated as unavailable, not proof of policy execution. CI is left for a second pass.
fd98a90): package 0.1.21 includes the MCP launcher-permission fix; feature source and bundled payload are unchanged. Types/model generation, formatting, build, 28 focused tests, static artifact verification and full installed-package smoke passed, including MCP initialization. CI was not awaited.Risk and rollout
Draft generation is opt-in and never installs a policy in the checkout. Owner approval is still required; supporting documents may contain sensitive details and need disclosure review. Policy snapshots do not freeze the rest of the source tree. Regenerate after relevant source changes.
The tests validate orchestration and local contracts with synthetic model responses, not the comparative quality or cost of three-stage generation. Existing scan defaults and the child PR's separate application lifecycle remain unchanged. No package release or PR merge is performed.
Public disclosure review
Existing history includes work-email metadata, requester-only automated review links, and an older security-review discussion. They are not reproduced here; the second attestation remains unchecked. New changes use synthetic examples.