feat(codex): add Codex CLI governance hooks package - #3409
feat(codex): add Codex CLI governance hooks package#3409Josh (thatjosh) wants to merge 26 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
🤖 AI Agent: docs-sync-checker — Docs Sync
Docs SyncDocumentation is in sync. |
🤖 AI Agent: security-scanner — View details
No security issues found. |
🤖 AI Agent: breaking-change-detector — API Compatibility
API CompatibilityNo breaking changes detected. |
🤖 AI Agent: contributor-guide — View details
Welcome, and thank you for your contribution! Great job on providing detailed documentation and tests for the new Before merging, please address the following:
For guidance, please refer to CONTRIBUTING.md. Let us know if you need any help! |
🤖 AI Agent: code-reviewer — View details
TL;DR: 0 blockers, 1 warning. The change introduces a new governance package with reused logic, but lacks explicit validation for Codex-specific edge cases.
Action items:
Warnings are fine as follow-up PRs. |
🤖 AI Agent: test-generator — View details
Test coverage looks good. No gaps identified. |
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
|
@microsoft-github-policy-service agree |
dc1e617 to
f36e748
Compare
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- Hardening sequencing: this package is a byte-faithful copy of main's claude-code governance core, which means it re-introduces the pre-#3251 fail-open rm option-cluster allow-list and pre-#3295 secret-read patterns in a fifth location that no open fix covers (#3251/#3295 predate this package and won't patch it). Add agent-governance-codex-cli to #3251's scope, rebase onto the hardened patterns, or get explicit maintainer ordering.
- audit.mjs carries the pre-#3250 rollover-brick bug: MAX_ENTRIES not exported, no seam verification, slice(-MAX_ENTRIES) truncates the chain head from GENESIS so verification fails permanently after 10k entries and fail-closed appends deny every subsequent decision. Adopt the #3250 seam-based fix.
- server/agt-mcp.mjs (233L) ships unreachable: installer never wires it into Codex config, README never says how to connect it, no mcp-server.test.mjs (claude-code has one). Also default-policy.json allowlists mcp__agt_governance__* which is Claude Code's MCP tool-naming convention, unverified for Codex.
- Confirm Codex 0.144.x emits Claude-style tool_name values (Bash/Read/WebFetch/MultiEdit); non-matching names silently downgrade the Bash-specific deny rules to generic review.
- .gitignore: add !agent-governance-codex-cli/bin/* negation per opencode/antigravity precedent (**/[Bb]in/* matches; current files tracked but future bin/ files silently ignored).
- Fix 'Codex shim' -> 'Claude Code shim' comment slips in the three hook files; 'anchor to CODEX_HOME rather than ~/.codex' is self-contradictory (default IS ~/.codex).
- (follow-up ok) registration-surface per #2658 precedent: root README row, docs page, dependency-audit doc for the new lockfile, package AGENTS.md.
- (maintainer) 18 action_required; reserve @microsoft/agent-governance-codex-cli on npm before merge (name now asserted in REGISTERED_NPM_PACKAGES); decide whether RFC #3408 acceptance precedes the reference implementation.
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- Remaining from round 1 (asks on unwired MCP server, .gitignore negation, and comment slips are addressed at 10779a9 — thanks): (1) hardening sequencing vs #3251/#3295 — this package still copies the pre-#3251 fail-open rm option-cluster allow-list and pre-#3295 secret-read patterns; add the package to #3251's scope, rebase onto the hardened patterns, or get explicit maintainer ordering.
- (2) audit.mjs still carries the pre-#3250 rollover-brick bug (verification fails permanently after 10k entries; fail-closed appends then deny everything). Adopt the #3250 seam-based fix or track it explicitly for this package.
- (3) confirm Codex 0.144.x emits Claude-style tool_name values (Bash/Read/WebFetch/MultiEdit); non-matching names silently downgrade Bash-specific deny rules to generic review.
- (maintainer) 18 action_required; npm name reservation before merge; RFC #3408 ordering decision.
|
(1) and (2) addressed via 23f3bc6, rebasing on the changes in the open PRs. Working on (3). |
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- Round-3 verified: hardening adoption is faithful and complete — policy.mjs shared body byte-identical to #3251's opencode head incl. normalizeCommandNameToken, letters-only cluster, recursive-without-force deny; #3295's dropCopyDestinationToken + readTokens + all 4 default-policy.json secret-read hunks verbatim; audit.mjs + rollover test byte-identical to #3250's head incl. the limit+2 e2e test. 18/18 package tests pass locally. Remaining: (1) Codex tool_name vocabulary evidence (author self-declares in progress); (2) CI node-test job has not executed on 23f3bc6 (needs workflow approval); (3) re-sync guard: #3250/#3251/#3295 still open — commit to re-syncing if their merged form differs, or sequence after they land.
- (minor, non-blocking) port the broader #3251/#3295 test matrices (PowerShell remove-item, backtick/brace invocation, .env template-copy allow, source/redirect denies) to the codex test file.
Signed-off-by: thatjosh <learningcs1711@gmail.com>
…emove unwired MCP server Signed-off-by: thatjosh <learningcs1711@gmail.com>
…over, microsoft#3251 recursive-delete, microsoft#3295 secret-read) Signed-off-by: thatjosh <learningcs1711@gmail.com>
…_patch/webrun, view_image) Signed-off-by: thatjosh <learningcs1711@gmail.com>
…erShell/quoted rm, source/redirect secret reads, .env template allow) Signed-off-by: thatjosh <learningcs1711@gmail.com>
7125037 to
534ef2e
Compare
|
Amended commits to include DCO signoff. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 42 changed files in this pull request and generated 1 comment.
Files not reviewed (5)
- agent-governance-antigravity-cli/package-lock.json: Generated file
- agent-governance-claude-code/package-lock.json: Generated file
- agent-governance-codex-cli/package-lock.json: Generated file
- agent-governance-copilot-cli/package-lock.json: Generated file
- agent-governance-opencode/package-lock.json: Generated file
Suppressed comments (1)
agent-governance-codex-cli/lib/policy.mjs:1202
isSafeEnvTemplateReadCommand()relies oncontainsCommandControlOperator()to decide when the.env.*template bypass is allowed, butcontainsCommandControlOperator()currently ignores I/O redirection and single pipelines. This enables a bypass likecat .env.example < ~/.ssh/id_rsa: the secret-read rule matchesid_rsa, but the bypass returns true because it only checks for.envtokens and doesn’t treat<as a control operator, so the deny is skipped.
function containsCommandControlOperator(commandText) {
return /(?:&&|\|\||[;`]|[\r\n])/.test(commandText);
}
Comment-staleness pass over the round-5 changes. Codex keeps listing a known marketplace plugin as "not installed" after removal, and the read-back gates on the status column, so comments saying "still lists the plugin" overstated the failure condition: - uninstall() JSDoc and the matching installer test title now say "still reports the plugin as installed", matching the gate and its error message. - policy test isolation comment now states loadPolicy's actual lookup order (explicit policyPath, then $AGT_CODEX_POLICY_PATH, then ~/.codex/agt/policy.json, then the bundled default) instead of the abbreviated form, and the bundled-failure test reuses isolatedPolicy() rather than hand-rolling the same path. Signed-off-by: thatjosh <learningcs1711@gmail.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 42 changed files in this pull request and generated no new comments.
Files not reviewed (5)
- agent-governance-antigravity-cli/package-lock.json: Generated file
- agent-governance-claude-code/package-lock.json: Generated file
- agent-governance-codex-cli/package-lock.json: Generated file
- agent-governance-copilot-cli/package-lock.json: Generated file
- agent-governance-opencode/package-lock.json: Generated file
Suppressed comments (2)
.github/dependabot.yml:125
- The new npm Dependabot entry for
agent-governance-codex-cliis missing thecommit-messageprefix configuration that the other npm entries use. Without it, Dependabot will likely open PRs with default titles (e.g. "Bump …"), which can fail the repo’s PR-title/conventional-commit checks.
- package-ecosystem: "npm"
directory: "/agent-governance-codex-cli"
schedule:
interval: "weekly"
open-pull-requests-limit: 5
labels:
- "dependencies"
agent-governance-codex-cli/test/policy.test.mjs:21
- This comment overstates
loadPolicy’s precedence: if$AGT_CODEX_POLICY_PATHis set (even to a missing path),loadPolicydoes not fall back to~/.codex/agt/policy.json; it falls back to the bundled default. Please adjust the comment so future maintainers don’t infer an unsupported fallback behavior.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 42 changed files in this pull request and generated no new comments.
Files not reviewed (5)
- agent-governance-antigravity-cli/package-lock.json: Generated file
- agent-governance-claude-code/package-lock.json: Generated file
- agent-governance-codex-cli/package-lock.json: Generated file
- agent-governance-copilot-cli/package-lock.json: Generated file
- agent-governance-opencode/package-lock.json: Generated file
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 42 changed files in this pull request and generated no new comments.
Files not reviewed (5)
- agent-governance-antigravity-cli/package-lock.json: Generated file
- agent-governance-claude-code/package-lock.json: Generated file
- agent-governance-codex-cli/package-lock.json: Generated file
- agent-governance-copilot-cli/package-lock.json: Generated file
- agent-governance-opencode/package-lock.json: Generated file
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 42 changed files in this pull request and generated no new comments.
Files not reviewed (5)
- agent-governance-antigravity-cli/package-lock.json: Generated file
- agent-governance-claude-code/package-lock.json: Generated file
- agent-governance-codex-cli/package-lock.json: Generated file
- agent-governance-copilot-cli/package-lock.json: Generated file
- agent-governance-opencode/package-lock.json: Generated file
Suppressed comments (1)
agent-governance-codex-cli/lib/audit.mjs:110
- Use American English spelling in user-facing error messages for consistency ("recognized" instead of "recognised").
throw new Error(`Audit log at ${auditPath} is not a recognised audit format.`);
|
liamcrumm and MohammadHaroonAbuomar, could you take another look? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 42 changed files in this pull request and generated no new comments.
Files not reviewed (5)
- agent-governance-antigravity-cli/package-lock.json: Generated file
- agent-governance-claude-code/package-lock.json: Generated file
- agent-governance-codex-cli/package-lock.json: Generated file
- agent-governance-copilot-cli/package-lock.json: Generated file
- agent-governance-opencode/package-lock.json: Generated file
Suppressed comments (1)
.github/dependabot.yml:125
- The new Dependabot config for
agent-governance-codex-cliis missing thecommit-messageprefixes that all the other npm entries use, which can cause generated PR titles/commits to fail the repo’s conventional-commit / PR-title checks. Mirror the existing npm entries by addingcommit-message.prefixandcommit-message.prefix-development.
- package-ecosystem: "npm"
directory: "/agent-governance-codex-cli"
schedule:
interval: "weekly"
open-pull-requests-limit: 5
|
liamcrumm and MohammadHaroonAbuomar, could you take another look? |
Description
Adds
agent-governance-codex-cli, a new package bringing AGT governance to the OpenAI Codex CLI. It's a thin host adapter over the existing Claude Code governance core: Codex's hooks (SessionStart, UserPromptSubmit, PreToolUse) mirror Claude Code's contract, so the governance logic is reused unchanged and only host wiring is new approach used for the OpenCode integration #2658. Tested with sandbox and live.Enforcement map
SessionStartUserPromptSubmitPreToolUseProposed via RFC #3408.
Type of Change
Package(s) Affected
Checklist
Attribution & Prior Art
Prior art / related projects:
Adapted from the in-repo
agent-governance-claude-codepackage (credited in the README), following the same copy-approach used for the OpenCode CLI integration #2658 and the Antigravity CLI integration #2554. Codex's hook contract mirrors Claude Code's.AI Assistance
If AI tools materially shaped this change, briefly note what was used:
IP, Patents, and Licensing
Related Issues
Implements RFC #3408