feat(cli): review and apply saved SECURITY.md drafts - #489
feat(cli): review and apply saved SECURITY.md drafts#489mldangelo-oai wants to merge 69 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.
|
@codex review Please review exact head The Unix permission-race regression failed on the preceding head and passes with this change. Local TypeScript, formatting, and combined policy, CLI, API, and configuration checks passed; exact-head native Windows package and matrix checks are pending. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ee5fe1c43
ℹ️ 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 exact head Real Unix permission-metadata and verification-retry regressions failed on the preceding head and now pass. Local TypeScript, formatting, and combined policy, CLI, API, and configuration checks passed; exact-head native Windows and macOS checks are pending. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95f168ba41
ℹ️ 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 exact head Real read-only and rollback access-control regressions, cross-filesystem recovery, retry races, existing alias verification, and atomic concurrent-writer protections pass locally. Exact-head native Windows and macOS checks are pending. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a47da34698
ℹ️ 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 the exact current head Local validation: 196 tests passed, 11 platform-specific tests skipped, and no tests failed. Fresh Linux, macOS, and Windows CI is running. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afbdd0e8ff
ℹ️ 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 exact current head Local validation: 196 tests passed, 12 platform-specific tests skipped, and no tests failed. Fresh native Windows, Linux, and macOS CI is running. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a1ed9c1c8
ℹ️ 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 temporary = join( | ||
| dirname(target.targetPath), | ||
| `.SECURITY.md.${randomUUID()}.tmp`, | ||
| ); |
There was a problem hiding this comment.
On SELinux systems with a filename-specific type transition for SECURITY.md, creating the inode under this randomized .tmp name gives it the temporary file's context, and the later hard-link or rename does not recompute the label for the final name. Because the new-policy path has no metadata reference, post-write verification never notices that the installed policy has the wrong mandatory-access context. The earlier existing-policy context preservation does not cover this branch; create or relabel the inode using the target's expected context and verify it after installation.
AGENTS.md reference: sdk/typescript/AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
|
@codex review Please review exact current head The separate open review item concerning filename-specific SELinux context transitions for a brand-new policy remains unresolved and is intentionally not changed by this Windows-only update. Local validation: 196 tests passed, 13 platform-specific tests skipped, and no tests failed. Fresh native Windows, Linux, and macOS CI is running. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f061f012d
ℹ️ 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 exact current head The separate open review item concerning filename-specific SELinux context transitions for a brand-new policy remains unresolved and is intentionally not changed by this update. Local validation: 197 tests passed, 14 platform-specific tests skipped, and no tests failed. Fresh native Windows, Linux, and macOS CI is running. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfac0391e2
ℹ️ 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 exact current head The separate open review item concerning filename-specific SELinux context transitions for a brand-new policy remains unresolved and is intentionally not changed by this update. Local validation: 198 tests passed, 14 platform-specific tests skipped, and no tests failed. Fresh native Windows, Linux, and macOS CI is running. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9b4f0678b
ℹ️ 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 exact current head The separate open review item concerning filename-specific SELinux context transitions for a brand-new policy remains unresolved and is intentionally not changed by this update. Filesystem change-generation checks do not make their final observation and the following operating-system move one atomic operation. Local validation: 199 tests passed, 14 platform-specific tests skipped, and no tests failed. Fresh native Windows, Linux, and macOS CI is running. |
|
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 255925c081
ℹ️ 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".
| await validatePolicyLinks(target); | ||
| if ((await readSecurityPolicy(target.targetPath)) !== draft.content) { |
There was a problem hiding this comment.
When applying an existing component policy, an editor or checkout operation can change a regular ancestor SECURITY.md while the potentially slow Unix metadata or Windows descriptor verification runs. This final block revalidates aliases and the installed target only, so the changed inherited-policy hash is never checked again and the operation can report written or unchanged against guidance different from the reviewed snapshot. Repeat the inherited snapshot check after permission verification before returning success.
AGENTS.md reference: sdk/typescript/AGENTS.md:L22-L23
Useful? React with 👍 / 👎.
| } finally { | ||
| // Preserve the write or recovery outcome if temporary cleanup fails. | ||
| await rm(temporary, { force: true }).catch(() => undefined); |
There was a problem hiding this comment.
When a writable new policy—or a writable existing policy on Windows—is installed through the hard-link path and removing the staging name fails, this suppression still lets application report success. The leftover .SECURITY.md.*.tmp is another name for the installed inode, so a later write through that unreported path changes the reviewed policy after all verification has passed. Treat this cleanup failure as a written-but-unverified result, or ensure installation cannot leave an unverified hard-link alias.
AGENTS.md reference: sdk/typescript/AGENTS.md:L22-L23
Useful? React with 👍 / 👎.
2 participants
Summary
Add offline review and explicit application of saved
SECURITY.mddrafts.policy --apply <artifact-directory>validates the selected repository and component, previews the exact diff, and asks before writing.--writeapplies an already-reviewed saved draft without another prompt; it cannot write an unseen model response.This PR remains stacked on #536 and includes its updated head and main.
Changes
Testing
pnpm run types,pnpm run format,pnpm run build, andgit diff --check: passed.The full repository suite was not rerun. Native Windows, SELinux and Docker validation were not performed locally. Current-head CI is left for a second pass.
Risk and rollout
Applying a draft replaces only the selected policy after explicit approval. It does not stage, commit or publish it. Retain reported recovery files until concurrent editor writes are reconciled. Filesystem generation checks do not make the final observation and following move one atomic operation.
Two follow-ups remain: filename-specific SELinux context transitions for a brand-new policy are unresolved; the Windows release-note test's null child status was not reproduced locally. The shard change addresses the observed policy-test timeout, but native timing still needs CI verification. Do not treat this first pass as merge approval.
Public disclosure review
Preexisting public history includes contributor metadata and requester-only automated review links. Those links are not reproduced here; the second attestation remains unchecked. New examples and tests are synthetic.