feat(security): add HDT risk guardrails, deterministic evidence outputs, and drift monitoring#22280
Conversation
BrianCLong
added
the
codex
ChatGPT Codex Connector
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 23 minutes and 16 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive Human Digital Twin (HDT) risk control framework, including policy definitions, a risk gate evaluation script, and enhanced log scanning to prevent the exposure of sensitive persona or intimacy data. The review identified a critical logic error in the risk gate where the 'deny-by-default' setting was effectively bypassed. Additionally, feedback was provided regarding the fragility of the manual YAML parser, missing error handling for fixture parsing, and potential security gaps in the log scanner's delimiter-based filtering.
| } | ||
| } | ||
|
|
||
| const decision = violations.length > 0 ? 'deny' : policy.default === 'deny' ? 'allow' : 'allow'; |
There was a problem hiding this comment.
The decision logic contains a bug where policy.default is effectively ignored when no violations are found. The ternary expression policy.default === 'deny' ? 'allow' : 'allow' always evaluates to 'allow', which contradicts the "deny-by-default" requirement. If no rules match and the default is deny, the decision should be deny. Conversely, if a rule matches and is satisfied (no violations), it should be allow.
| const decision = violations.length > 0 ? 'deny' : policy.default === 'deny' ? 'allow' : 'allow'; | |
| const decision = violations.length > 0 ? 'deny' : (policy.rules.some((r) => r.match?.any?.some((cap) => capabilities.has(cap))) ? 'allow' : policy.default); |
| const loadPolicy = (): Policy => { | ||
| const raw = readFileSync(path.join(process.cwd(), '.github/policies/hdt-risk-controls.yml'), 'utf8'); | ||
| const lines = raw.split(/\r?\n/); | ||
| const policy: Policy = { id: 'hdt-risk-controls-v1', default: 'deny', rules: [] }; | ||
| let current: Rule | null = null; | ||
| let section: 'match' | 'require' | null = null; | ||
| for (const line of lines) { | ||
| const t = line.trim(); | ||
| if (!t || t.startsWith('#')) continue; | ||
| if (t.startsWith('id:') && !current) policy.id = t.slice(3).trim(); | ||
| if (t.startsWith('rulepack_version:')) policy.rulepack_version = t.slice('rulepack_version:'.length).trim(); | ||
| if (t.startsWith('default:')) policy.default = t.slice(8).trim() as 'deny' | 'allow'; | ||
| if (t.startsWith('- id:')) { | ||
| if (current) policy.rules.push(current); | ||
| current = { id: t.slice(5).trim() }; | ||
| section = null; | ||
| continue; | ||
| } | ||
| if (!current) continue; | ||
| if (t.startsWith('match:')) { | ||
| current.match = { any: [] }; | ||
| section = 'match'; | ||
| continue; | ||
| } | ||
| if (t.startsWith('require:')) { | ||
| current.require = []; | ||
| section = 'require'; | ||
| continue; | ||
| } | ||
| if (t.startsWith('deny_if_missing:')) { | ||
| current.deny_if_missing = t.endsWith('true'); | ||
| continue; | ||
| } | ||
| if (t.startsWith('action:')) { | ||
| current.action = t.slice(7).trim() as 'deny' | 'allow'; | ||
| continue; | ||
| } | ||
| if (t.startsWith('- ') && section === 'match') { | ||
| current.match?.any?.push(t.slice(2).trim()); | ||
| } | ||
| if (t.startsWith('- ') && section === 'require') { | ||
| current.require?.push(t.slice(2).trim()); | ||
| } | ||
| } | ||
| if (current) policy.rules.push(current); | ||
| return policy; | ||
| }; |
There was a problem hiding this comment.
The manual YAML parsing implementation is fragile and highly dependent on specific formatting (indentation, line prefixes). This approach is prone to errors or bypasses if the policy file structure changes slightly or includes comments in unexpected places. For a security-critical gate, using a robust, standard YAML library like js-yaml or yaml is strongly recommended to ensure consistent and secure policy evaluation.
| const started = process.hrtime.bigint(); | ||
| const policy = loadPolicy(); | ||
| const fixtureRaw = readFileSync(fixturePath, 'utf8'); | ||
| const fixture = JSON.parse(fixtureRaw) as Fixture; |
There was a problem hiding this comment.
Parsing the fixture JSON without error handling will cause the script to crash with a generic stack trace if the input file is malformed. Adding a try...catch block allows for a cleaner exit and a more descriptive error message.
let fixture: Fixture;
try {
fixture = JSON.parse(fixtureRaw);
} catch (err) {
console.error(`Invalid JSON in fixture: ${fixturePath}`);
process.exit(1);
}| const content = readFileSync(fullPath, "utf-8"); | ||
| for (const pattern of BLACKLIST) { | ||
| if (pattern.test(content)) { | ||
| if (content.includes('":') || content.includes("=") || content.includes(": ")) { |
There was a problem hiding this comment.
The check for structured log markers (":, =, or : ) may cause the scanner to miss sensitive patterns in unstructured log formats, such as plain text or CSV. While this logic aims to reduce false positives, it creates a potential bypass for the "never-log" requirement if sensitive HDT data (like raw_chat_transcript) is logged without these specific delimiters. Consider if these patterns should be blocked regardless of the surrounding characters.
![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: 9421a6136a
ℹ️ 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 decision = violations.length > 0 ? 'deny' : policy.default === 'deny' ? 'allow' : 'allow'; |
There was a problem hiding this comment.
The decision logic ignores the policy default because both branches return "allow" when there are no violations. With default: deny in .github/policies/hdt-risk-controls.yml, fixtures that introduce new/unmodeled capabilities (or provide no capabilities) are currently accepted instead of denied, which bypasses the stated deny-by-default guardrail.
Useful? React with 👍 / 👎.
|
|
||
| - run: pnpm install --frozen-lockfile | ||
|
|
||
| - run: pnpm exec tsx scripts/monitoring/human-digital-twins-risk-controls-drift.ts --baseline scripts/monitoring/fixtures/hdt-risk-controls/baseline.json --candidate scripts/monitoring/fixtures/hdt-risk-controls/drifted.json --out artifacts/hdt-risk-drift |
There was a problem hiding this comment.
This scheduled workflow compares the baseline against the intentionally drifted fixture on every run, so the drift script will continuously report high severity and exit non-zero even when no real policy drift occurred. That makes the monitor permanently noisy and prevents it from signaling actual regressions in repository policy state.
Useful? React with 👍 / 👎.
## Summary Rebuilds the golden-main merge train from a clean `main` base and converges the currently mergeable PR set into one replacement branch. This branch absorbs: - #22296 - #22279 - #22281 - #22282 - #22283 - #22284 - #22285 - #22295 - #22297 - #22286 - #22291 - #22280 - #22277 - unique non-conflicting surfaces from #22241 - the admissibility/CACert/failure-demo runtime lane from #22314 This branch supersedes: - #22298 as the contaminated/conflicting convergence branch - #22277 as a standalone merge vehicle - #22241 as the broad mixed-purpose convergence vehicle once remaining review is complete - #22314 as the standalone admissibility lane now folded into the golden path This branch intentionally excludes: - #22292 because it targets `merge-surge/staging`, not `main` ## Conflict policy used while absorbing #22241 When merging `#22241` on top of the cleaned train, the following files conflicted and were resolved in favor of the current train versions so the newer focused CI/governance repairs remain authoritative: - `.github/ci/required-checks.json` - `.github/workflows/drift-sentinel.yml` - `.github/workflows/pr-gate.yml` - `docs/ci/REQUIRED_CHECKS_POLICY.yml` - `pnpm-lock.yaml` - `scripts/ci/check_branch_protection_drift.mjs` - `scripts/ci/validate_workflows.mjs` All other `#22241` changes merged on top of the train. ## Mapping Change Summary This convergence branch updates workflow, schema, and governance contracts that control merge eligibility, admissibility evidence, and deterministic trust artifacts. ## Diff - Added admissibility/evidence/CACert surfaces including `packages/evidence/schemas/decision_trace.schema.json` - Tightened golden-lane workflow policy and drift handling in `.github/workflows/_policy-enforcer.yml`, `.github/workflows/execution-graph-reconciliation.yml`, `.github/workflows/post-ga-hardening-enforcement.yml`, `.github/workflows/merge-surge.yml`, `.github/workflows/control-plane-drift.yml` - Realigned governance state in `governance/pilot-ci-policy.json` and `governance/branch-protection.json` - Repaired deterministic reconciliation verification in `scripts/ci/verify_execution_graph_reconciliation.mjs` and `scripts/ci/drift-sentinel.mjs` ## Justification The repo needed one mergeable replacement lane that restores deterministic governance checks, folds the admissibility implementation into the golden path, and suppresses broken optional PR workflows that were blocking convergence without being canonical required checks. ## Impact - Canonical pilot checks remain `pr-gate / gate` and `drift-sentinel / enforce` - Merge-train branches no longer fail ordinary small-PR enforcement gates by construction - Optional broken workflows are narrowed to their owned surfaces so they stop contaminating this convergence lane and the immediate post-merge main push - Execution-graph reconciliation now accepts the repo’s canonical snake_case trust bundle fields ## Rollback Plan Revert commit `ce32b96c0f` from `merge-train/golden-main-20260331-final`, then rerun the prior golden-lane checks and restore the previous PR body. ## Backfill Plan After the lane is green, backfill the same workflow scoping and governance-contract repairs into any surviving PRs that still touch `.github/workflows/**` or governance surfaces, then close superseded PRs against `#22309`. ## Validation Evidence Local validation completed: - `node scripts/ci/drift-sentinel.mjs` - `ruby -e 'require "yaml"; ... YAML.load_file(...)'` over all edited workflow files - `jq . governance/pilot-ci-policy.json` - `jq . governance/branch-protection.json` - merge-marker scan over all edited files returned clean ## Notes - Live GitHub PR checks on the open PR set are being converged through this single branch instead of salvaging each broken lane independently. - I did not run the full local verification matrix in this session; this PR is intended to give the repo one clean convergence lane for CI and human review. - After this PR lands, the absorbed PRs should be closed as superseded. --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: Bot <bot@summit.ai> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Gemini CLI <gemini-cli@google.com>
|
Superseded by #22309, which is now merged into main. |
…ts, and drift monitoring (rebase on main)
BrianCLong
force-pushed
the
codex/establish-human-digital-twins-risk-controls
branch
from
9421a61 to
3488c12
Compare
1 participant
Motivation
Description
.github/policies/hdt-risk-controls.ymldefining rulesno-intimate-hdt-without-consentandno_raw_sensitive_chat_logswith required controls (consent, purpose, disclosure, retention)..github/scripts/hdt-risk-gate.tsthat evaluates fixture inputs, enforces artifact size/runtime ceilings, and emitsreport.json,metrics.json, andstamp.json(stable key ordering, no wall-clock timestamps in deterministic outputs)..github/scripts/check-never-log.tsto include HDT-sensitive patterns and a--fixturescanning mode for CI/fixtures..github/workflows/hdt-risk-guardrails.yml, add a scheduled drift workflow.github/workflows/hdt-risk-drift.yml, a deterministic drift detectorscripts/monitoring/human-digital-twins-risk-controls-drift.ts, integration fixtures/tests underSECURITY/__tests__/integration/fixtures/hdt-risk/, and operator/docs underdocs/standards/,docs/security/data-handling/, anddocs/ops/runbooks/.{"agent":"antigravity","confidence":0.86,"mode":"warn-only"}
Testing
pnpm exec tsx .github/scripts/hdt-risk-gate.ts --fixture SECURITY/__tests__/integration/fixtures/hdt-risk/forbidden-no-consent.json --out tmp/hdt-riskand observed a non-zero (deny) exit and the expected ruleno-intimate-hdt-without-consentinreport.json(passed).pnpm exec tsx .github/scripts/hdt-risk-gate.ts --fixture SECURITY/__tests__/integration/fixtures/hdt-risk/allowed-research-with-disclosure.json --out tmp/hdt-riskand observed a zero exit anddecision: "allow"inreport.json(passed).report.json,metrics.json, andstamp.jsonbyte-for-byte (commands shown in PR CI); outputs were identical (passed).pnpm exec tsx .github/scripts/check-never-log.ts --fixture SECURITY/__tests__/integration/fixtures/hdt-risk/forbidden-log-sink.jsonand the scanner reported the expected forbidden pattern and exited non-zero (passed).pnpm exec tsx scripts/monitoring/human-digital-twins-risk-controls-drift.ts --baseline scripts/monitoring/fixtures/hdt-risk-controls/baseline.json --candidate scripts/monitoring/fixtures/hdt-risk-controls/drifted.json --out tmp/hdt-risk-driftand observedseverity: highwith a non-zero exit per design (passed).node .github/scripts/validate-evidence-schemas.mjsfailed in the current environment due to an AJV draft/schema resolution error (no schema with key or ref "https://json-schema.org/draft/2020-12/schema") and is marked as failed; this appears to be an environment/schema compatibility issue rather than the new gate output format.All automated tests above were executed locally in the branch and the only failing automated check was the AJV/schema resolution issue during evidence-schema validation.
Codex Task