feat(cli): review and apply saved SECURITY.md drafts - #489
feat(cli): review and apply saved SECURITY.md drafts#489mldangelo-oai wants to merge 84 commits into
Conversation
|
@codex review Please review the current head, |
|
Codex Review: Didn't find any major issues. 🚀 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: b1de4d9394
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4014d9fe0
ℹ️ 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".
# Conflicts: # sdk/typescript/README.md # sdk/typescript/src/cli.ts
|
@codex review Please review exact current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 967a1ebe51
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fee8290552
ℹ️ 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".
| if ((await readSecurityPolicy(candidate)) === draft.previousContent) | ||
| candidates.push(candidate); | ||
| } | ||
| if (candidates.length !== 1) { |
There was a problem hiding this comment.
When the same saved draft has been applied more than once—for example, A→B, the user restores A while reconciling an editor save, and then reapplies B—the artifact directory can legitimately contain two recovery-SECURITY-*.md files whose contents both equal draft.previousContent. Both are added as candidates, so this uniqueness check rejects a subsequent --apply ... --write verification even though the recovery reported by the latest application still exists. Persist or select the exact reported recovery path instead of treating duplicate-content snapshots as unverifiable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Current head: 866a086cfda8c322dfa59796b2140a988c39ac45. Confirmed as an unresolved retry limitation. An apply/restore/reapply sequence can leave multiple valid original snapshots, so a later exact-content retry rejects safely. Selecting by identical contents would not establish which retained inode belongs to the latest application. I have left this open for the recovery-association design rather than adding another receipt format during this simplification pass.
| if (sourceContext !== null && sourceContext !== destinationContext) | ||
| arguments_.push("--preserve=context"); |
There was a problem hiding this comment.
On SELinux Linux, when the existing SECURITY.md has no security.selinux xattr but the newly created temporary file inherits a label from its directory, sourceContext is null and destinationContext is non-null, so this branch neither passes --preserve=context nor clears the destination-only label. GNU cp -p preserves only mode, ownership, and timestamps, with context as a separate attribute (GNU cp documentation); the label therefore remains and verifyUnixSecurityMetadata rejects every update before displacement even when the account could preserve the unlabeled state. Fresh evidence beyond the earlier non-default-context finding is that the current branch only acts when a source label is present.
AGENTS.md reference: sdk/typescript/AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Current head: 866a086cfda8c322dfa59796b2140a988c39ac45. No label-removal change has been made. The current mismatch rejects before displacement and leaves the original file unchanged. I have not reproduced a native source-without-context/destination-with-context case on an enforcing SELinux filesystem. I am leaving this open rather than removing a security label based only on the synthetic null case.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fee8290552
ℹ️ 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".
| try { | ||
| signal?.throwIfAborted(); | ||
| // Check the displaced file, then install without replacing a newer save. | ||
| await rename(targetPath, recoveryPath); |
There was a problem hiding this comment.
When updating an existing policy, this rename removes SECURITY.md before content, Unix metadata, or the potentially slow Windows descriptor checks finish and before the staged inode is installed. A concurrent scan during that interval resolves no component policy (or falls back to different ancestor guidance), and an abrupt process termination leaves the checkout in that state with only a hidden recovery file. Retain a recovery reference while atomically replacing the target so readers always observe either the old or reviewed policy.
AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Current head: 866a086cfda8c322dfa59796b2140a988c39ac45. Confirmed and left open as a blocking design finding. The move/check/install sequence leaves the final path absent, including during the Windows descriptor check. The guide now warns about it, but documentation is not a fix. Simply hard-linking the original and renaming over the target can overwrite a newer save; an exchange can preserve that inode while still removing the newer save from the active path. Closing this gap requires choosing and implementing the concurrent-save contract, so I have not substituted another non-atomic verification loop.
|
@codex review @codex security review Please review current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 866a086cfd
ℹ️ 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".
# Conflicts: # sdk/typescript/README.md
|
@codex review Please review current head |
There was a problem hiding this comment.
💡 Codex Review
codex-security/sdk/typescript/src/api.ts
Lines 953 to 954 in c2d84a5
Revalidate host-resolved policy guidance
When a selected component contains a nested or linked SECURITY.md and that file changes after this one-time host resolution—or during the subsequent multi-stage generation—the model continues using the stale contents because it can no longer read or resolve those policies itself. The following requireUnchangedSecurityPolicy call validates only the selected target and its ancestor snapshot, not descendant/reporting policies from inputs.policyPaths, and there is no later inventory check, so generation can successfully return a draft based on guidance that no longer exists. Snapshot and revalidate every host-resolved policy before accepting the generated draft.
AGENTS.md reference: sdk/typescript/AGENTS.md:L22-L23
ℹ️ 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".
|
@codex review Please review current head |
|
Codex Review: Didn't find any major issues. 🎉 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". |
|
Validation for
Current-head CI is green: Node CI, container checks, and PR release validation passed. Cancelled duplicate runs are excluded. The native Windows policy shard passed 544 tests, with 18 platform skips and zero failures; its separate machine-wide PowerShell policy check also passed. This includes EFS, audit-rule, descriptor, and resource-attribute regressions. Both duplicate EFS reports are resolved with this evidence. Native SELinux and Smack enforcement are still unverified; the synthetic label tests are not reported as native coverage. The existing-policy replacement gap remains blocking: the final path can be absent while the displaced inode is checked. The ambiguous recovery association and staging-only SELinux report also remain open. The latter still needs native reproduction; current mismatch handling rejects before displacing the original. Full local suite results and their tested predecessor commits remain listed in the PR description. This pass does not approve, merge, or release the feature. |
|
@codex review Please review head Focused policy and severity-classification CLI tests: 49 passed. SDK TypeScript check, plugin bundle generation, package smoke-script syntax, changed-file formatting, and git diff --check passed. The full suite is left to CI. |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
|
@codex review Please review head All four package-smoke timeout tests passed. JavaScript syntax, changed-file formatting, and git diff checks passed. The full suite is left to CI. |
|
Codex Review: Didn't find any major issues. Hooray! 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". |
2 participants
Summary
Add offline review and explicit application of saved
SECURITY.mddrafts.policy --apply <artifact-directory>previews the saved draft and offers to install it;--writeapplies an already-reviewed draft without another confirmation. This PR remains stacked on #536 at84a4291514cfcd3f5538562373eab76fa6da03b3.Changes
--headlessor an explicit output format skips that prompt.--writerequires--apply; it cannot install an unseen model response.Testing
CI repair at
c843a876ba614158bc4d2920d43a5d5bf0d02636againstmdangelo/codex/security-policy-draftsat84a4291514cfcd3f5538562373eab76fa6da03b3:Earlier validation recorded for prior heads:
Merge conflict resolution at
238973cb37b9e8c58887c71c59f1ee5f4001ad8aagainstmdangelo/codex/security-policy-draftsat84a4291514cfcd3f5538562373eab76fa6da03b3:Earlier validation recorded for prior heads:
At
017d163f724b92f3a85d2a59eacb1212d7917f52:git diff --check: passed.866a086c, alongside descriptor, audit-rule, and resource-attribute tests. It is restricted to disposable runners because fixture encryption can create a certificate. Current-head CI is requested separately.866a086c: 2,302 SDK tests passed, 59 skipped, in both fixed and random order. Both installed-target regressions failed before their fix; the focused final-verification set then passed eight tests, with three platform skips. The combined policy/API suites at precedingc2d84a5dpassed 146 tests, with 15 platform skips. The inherited framework follow-up passed 99 focused Homebrew Python 3.14 checks and four native standalone Python 3.12 checks. Installed-package smoke and current CI/review results will be recorded in a validation comment.Risk and rollout
Application is optional and never stages, commits, or publishes the policy. Retain reported recovery files until concurrent editor writes are reconciled. EFS-encrypted targets require a tool that preserves their encryption settings. Native Windows and labeled-Linux behavior must be assessed separately from macOS testing.
Blocking design finding: updating an existing policy temporarily removes its final path while the displaced file is checked. A concurrent scan can observe missing guidance, and abrupt termination can leave only the recovery file. The guide now states this limitation; documenting it does not fix it. Closing the gap while retaining the current concurrent-save contract requires a broader replacement design.
Repeated apply/restore/apply cycles can also leave multiple matching recovery snapshots and cause a safe but unnecessary retry rejection. That recovery-association issue remains open. A proposed staging-only SELinux label removal has not been adopted without a native reproduction; the current code rejects mismatched labels before displacing the original.
The parent's comparative generation-quality/cost question and documented macOS temporary-directory sandbox limitation remain open. This pass does not approve, merge, or release the feature.
Public disclosure review
Preexisting commit metadata and access-restricted automated review links prevent a clean second attestation. They are not reproduced here, and published history has not been rewritten. New examples and fixtures are synthetic; new commits use GitHub noreply metadata.