Add pinned Codex deny-floor adapter - #1457
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Chris0Jeky
added
hardening
Priority I
security
testing
labels
ChatGPT Codex Connector
Independent adversarial review recordExact reviewed head: Two context-isolated reviewers used different lenses (correctness/documentation and security/trust-boundary). All findings, including lower severity, were resolved or tracked:
Both original reviewers re-reviewed the fixed exact head. The correctness reviewer reported the prior finding fixed with no new findings. The security reviewer confirmed all three findings were either fixed in-scope or precisely tracked and reported no remaining finding for this narrow canonical Bash adapter. Fix and verification evidence
|
|
@codex review Please perform a fresh adversarial review of exact head |
|
Comment triage: the Gemini Code Assist sunset notice is informational and contains no review finding or requested change. No commit is associated with that notice. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
|
Review triage: Codex reviewed exact head |
Exact-head verification updateHead remains
No PR head change occurred after the local reviews or CI. The sole remaining merge gate is human T4 activation evidence: fresh interactive Codex |
Metadata review finding
Expected resolution: replace those placeholders with explicit PowerShell variables ( |
Metadata finding resolutionResolved the LOW rendered-command finding in PR/issue metadata:
This was metadata-only: PR head remains |
Project-metadata review finding
Tracked substrate fix: #1458. Before this PR can merge, set the project Priority for #1456 and #1457 to |
Runtime trust gate result: FAILEDFresh maintainer evidence from the guarded #1457 worktree:
Classification: project-hook discovery/trust failed. Green parser/smoke/CI evidence does not substitute for runtime activation. Do not mark ready or merge. No tracked Taskdeck mutation is permitted until a fresh session discovers the adapter, the maintainer reviews/trusts it through Current diagnosis: Project Priority finding resolutionThe previously empty project Priority fields were repaired with targeted project-item updates and directly re-read:
This resolves the PR-specific metadata finding without changing the PR head. Issue #1458 remains open for the audit script’s 1,000-item completeness defect. |
Fresh consolidation review ? blocked on authoritative hook source and floor driftExact Taskdeck head reviewed:
Disposition: return this PR to draft. Do not merge or rerun the force-push canary. First settle and merge the chosen agent-harness floor, sync that exact reviewed version globally, refresh this adapter pin and the source-location/trust documentation, then rerun authoritative doctor/audit, exact-head reviews and CI. Live proof must finally show at least one installed/trusted active hook before the allow command and denied non-writing canary. This comment tracks the Taskdeck-side blocker under linked issue #1456; the cross-repo defects are already tracked by the upstream issues above. No Taskdeck commit is made in this containment step. |
Containment evidencePR #1457 is now back in draft. Head remains unchanged at The two findings above remain open and are intentionally not ?fixed? by the draft transition. Resolution requires a reviewed final agent-harness floor, exact global sync, a refreshed adapter pin/source-location contract, new exact-head verification, and actual |
docs: track the #1457 /hooks gate and merged-worktree pruning as human-only items
Not merging — and there is now a second, harder blocker than the human gateReviewed 2026-07-25 while clearing the PR queue. This one should stay a draft, and the reason has grown since it was opened. 1. The pinned hash no longer matches the installed dispatcher — verified
The installed dispatcher is now Three minor floor versions have shipped since the pin was taken (1.5.2 → 1.6.5; I watched it move 1.6.4 → 1.6.5 mid-session today). This PR's own rule already settles what that means. From
So merging as-is would ship an adapter whose audit-time declaration is stale on arrival, and 2. The human gate is structural, not a formalityThe PR states it plainly, and I agree with it:
That is a live-activation proof in an interactive Codex session. No agent can perform it, and no amount of review substitutes for it. It is tracked in 3. What is still good hereNone of this is a criticism of the work. The adapter is a single pinned Recommended path
Related, from today's floor work: field evidence added to agent-harness #12, #21, #41, #45, and a new #56 ( |
The adapter pinned 9f38e906... (floor v1.5.2). The installed dispatcher is now 1.6.5, whose hash under the harness's own normalization (harness.py:2478 normalized_text_sha256 -- UTF-8, CRLF->LF, CR->LF, sha256) is 91088f9b... Both the POSIX command and commandWindows carried the stale value; both updated. Nothing else in the branch referenced it. By this PR's own rule -- 'any global dispatcher change requires an adapter pin refresh, reviewed doctor/audit, and fresh /hooks trust' -- the stale pin also invalidated the recorded doctor/audit evidence and the trust step. This commit addresses only the pin; the doctor/audit rerun and the interactive trust session remain, and the trust session is maintainer-only by design. Caveat worth stating: the floor moved 1.6.4 -> 1.6.5 during the session in which this was written, so an audit-time hash pin against a fast-moving upstream will go stale again. Re-verify at trust time rather than assuming this value holds.
Pin refreshed — here is exactly what you need to doYou asked for the parts I can't do to come with precise instructions. The pin is fixed in What I changed
Both the POSIX Your stepsStep 1 — Confirm the pin is still current (30 seconds)The floor moved 1.6.4 → 1.6.5 during the session this was written in, so verify rather than trust: python -c "import hashlib,pathlib; t=pathlib.Path.home().joinpath('.claude/hooks/dispatch.py').read_text(encoding='utf-8').replace(chr(13)+chr(10),chr(10)).replace(chr(13),chr(10)); print(hashlib.sha256(t.encode()).hexdigest())"Expected: If it differs, the floor moved again — tell me and I'll re-pin before you continue. Step 2 — Rerun doctor and auditThe stale pin invalidated the recorded evidence, so this has to be redone: cd C:\Users\jekyt\source\agent-harness
python harness.py doctor --repo C:\Users\jekyt\source\Taskdeck\.worktrees\codex-1456-codex-deny-floor-adapter
python harness.py audit --json C:\Users\jekyt\source\Taskdeck\.worktrees\codex-1456-codex-deny-floor-adapterLooking for: the project Codex floor reports one project handler, one candidate, one current pinned handler, and no adapter finding. Both commands are expected to still exit non-zero for pre-existing unrelated reasons (a missing user-skills check; repo hygiene tracked by #1138/#1291) — that is fine, it is the adapter section that must be clean. Paste the adapter section here and I'll read it. Step 3 — The trust session
|
Fresh exact-head adversarial audit - do not merge or run canaries yetAudited draft head CRITICAL / HIGH
MEDIUM / evidence drift
Required recovery order
The human gate should not be attempted against the current head. Findings remain tracked by #1456/#1457; no duplicate issue was created. |
|
Codex couldn't complete this request. Try again later. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63df07804b
ℹ️ 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".
| "hooks": [ | ||
| { | ||
| "type": "command", | ||
| "command": "expected=91088f9b8cf6a56a6eb6ef49903defe902da152e4ed835e83e59db66298f9a33; python3 \"$HOME/.claude/hooks/dispatch.py\" --event pre --runtime codex", |
There was a problem hiding this comment.
In a trusted Linux or macOS checkout, this command cannot resolve the intended global dispatcher: the applicable .codex/config.toml unconditionally replaces PATH with Windows paths and HOME with C:\Users\jekyt\source\Taskdeck\.runtime-codex\home at lines 34-35. Consequently the hook either cannot find python3 or targets a Windows-literal .../.claude/hooks/dispatch.py, leaving the deny floor inactive or blocking shell use depending on hook-error handling. Scope these environment overrides to Windows or resolve the POSIX interpreter and account home independently.
Useful? React with 👍 / 👎.
|
|
||
| Note on the pin: the `expected` hash named in the adapter is an audit-time declaration — harness doctor checks it against the installed dispatcher's normalized bytes. The hook performs no runtime byte verification of the dispatcher; runtime enforcement design is tracked at https://github.com/Chris0Jeky/agent-harness/issues/18. When the global dispatcher changes, refresh the adapter pin, re-run reviewed doctor/audit, and re-trust the hook via `/hooks`. | ||
|
|
||
| These static checks prove structure and pin declaration only — they are insufficient to prove the hook is live. Actual activation additionally requires, in a fresh Codex session: re-trusting the hook definitions via `/hooks`, then observing one allowed command pass and one dangerous command denied (use a non-writing force-push dry run such as `git push --dry-run --force`). Report activation as NOT verified unless that live check was performed in the current session. |
There was a problem hiding this comment.
When Codex system policy or another permission layer rejects force-pushes independently, merely observing git push --dry-run --force being denied does not prove that .codex/hooks.json loaded or ran; the same change explicitly identifies those layers as compensating controls. This procedure can therefore certify an inactive security hook. Require hook-specific evidence, such as confirming that the denial is reported by this PreToolUse handler.
Useful? React with 👍 / 👎.
| "hooks": [ | ||
| { | ||
| "type": "command", | ||
| "command": "expected=91088f9b8cf6a56a6eb6ef49903defe902da152e4ed835e83e59db66298f9a33; python3 \"$HOME/.claude/hooks/dispatch.py\" --event pre --runtime codex", |
There was a problem hiding this comment.
The committed adapter declares 91088f9…, but the commit's recorded doctor/audit, reviews, and exact-head CI refer to a57b64a, whose .codex/hooks.json still declares 9f38e906…; the commit message likewise describes the reviewed dispatcher as v1.5.2 with that older hash. Because doctor is the check intended to compare this audit-time declaration with the installed dispatcher, those results do not validate the configuration being reviewed. Re-run and record doctor/audit for 91088f9… before treating this adapter as verified.
Useful? React with 👍 / 👎.
| $TaskdeckRoot = (Resolve-Path .).Path | ||
| py -3 "$HarnessRoot\harness.py" doctor --repo $TaskdeckRoot | ||
| py -3 "$HarnessRoot\harness.py" audit --json $TaskdeckRoot | ||
| py -3 "$env:USERPROFILE\.claude\hooks\smoke_test.py" |
There was a problem hiding this comment.
Although this section requires the checks to come from the harness worktree detached at the reviewed commit, this command instead executes an independently installed file under the user's global .claude directory. That copy can be stale or newer than $HarnessRoot, so an unreviewed test matrix can pass while appearing to satisfy the reviewed-harness gate; doctor validates the dispatcher pin, not the provenance of this smoke-test program. Invoke the suite from $HarnessRoot or verify the installed suite against the reviewed version before running it.
Useful? React with 👍 / 👎.
| py -3 "$HarnessRoot\harness.py" doctor --repo $TaskdeckRoot | ||
| py -3 "$HarnessRoot\harness.py" audit --json $TaskdeckRoot | ||
| py -3 "$env:USERPROFILE\.claude\hooks\smoke_test.py" |
There was a problem hiding this comment.
When this block is pasted into PowerShell or automation, a nonzero exit from doctor or audit does not terminate the sequence, and a successful final smoke command overwrites $LASTEXITCODE with zero. The commit already records that both earlier commands can return nonzero, so this can report the security-verification block green without establishing that the adapter-specific results passed. Add explicit adapter assertions and $LASTEXITCODE handling after each external command rather than relying on the final process status.
AGENTS.md reference: AGENTS.md:L78-L78
Useful? React with 👍 / 👎.
Merge-gate checkpoint — 2026-07-27The maintainer has authorized agent merges for all PR types once their actual gates pass, so the former type-based hold is lifted. PR #1457 is still not technically merge-ready and must not be merged yet:
Safe recovery remains a new signed replacement branch/PR from final Taskdeck |
|
Superseded by #1538, the signed replacement cut from current |
T4 gate
Draft — exact-head CI and reviews are green, but do not merge until a maintainer has reviewed and trusted the project hook in a fresh interactive Codex
/hookssession and recorded the real-engine allow/deny canaries. This PR stages a security boundary; it does not self-ratify or self-merge it.Summary
PreToolUseadapter forBash9f38e906d49dd84de196bf22fc6388a5551494cf9b303610014ee6539783ebbb)/hookstrust procedureNo product behavior, API, database, frontend,
docs/STATUS.md, ordocs/IMPLEMENTATION_MASTERPLAN.mdchanges are included.Verification
For the reviewed harness commands below,
$TaskdeckRootmeans the absolute #1457 PR-worktree path.node scripts/check-docs-governance.mjsnode scripts/check-golden-principles.mjsgit diff --check origin/main...HEADPreToolUseentry,^Bash$, one command, timeout 5, no async/background, both platform commands contain the canonical pin and--event pre --runtime codexpy -3 -B scripts\agent_hooks\smoke_test.pywith the realpy -3interpreter directory prepended toPATH: hook behavior smoke matrix passed72639b8781e41c4872e700291d865afbf2110f19: installed canonicalsmoke_test.pyreported 1604/1604 passedharness.py doctor --repo $TaskdeckRoot: project Codex floor reported one project handler, one candidate, one current pinned handler; overall command remained nonzero only for the pre-existing missing user-skills checkharness.py audit --json $TaskdeckRoot: no adapter finding; overall command remained nonzero for pre-existing repository hygiene tracked by Docs truth & rotation: fix STATUS.md drift/bloat, repair broken links, add a link checker #1138/Adopt agent-harness T3 profile: harness right-sizing plan #1291dotnet test backend/Taskdeck.sln -c Release— not run; no backend/product code changed/hooksreview plus real-engine allow/deny canaries — maintainer gate30063860671passed all 15 jobs (including E2E Smoke) and CodeQL run30063859670passed all 4 jobs ona57b64aff4cf6f55057e163ba81c06dd5e4b31c8Two independent adversarial reviews examined the implementation. Their findings and resolutions are recorded in a PR comment; both re-reviewed exact head
a57b64aff4cf6f55057e163ba81c06dd5e4b31c8after the documentation fix and reported no remaining findings. A fresh external Codex review then examined the same head and posted no inline thread or actionable finding.Documentation
docs/STATUS.mdupdated — not applicable; shipped product reality did not changedocs/IMPLEMENTATION_MASTERPLAN.mdupdated — not applicable; roadmap did not changedocs/TESTING_GUIDE.mdupdated for the new verification flow.codex/README.mdanddocs/agentic/AGENT_TOOL_PARITY.mdupdated with trust boundariesTracking
Review; PR/issue priorities are populated; priority audit scanned 1000 items withneedsUpdate: 0CI Workflow Validation
.github/workflows/,deploy/,scripts/, or*.csprojfiles changed; CI Extended path trigger is not expectedci-required.ymlrun30063860671and CodeQL run30063859670are green on exact heada57b64afRisk Notes
GIT_CONFIG_GLOBALfalse denials; agent-harness Sandbox mode bypass #18 tracks enforceable-vs-audit-only hash semantics.