feat(policy): make deployment policy capability-aware and isolate privileged roles - #14
feat(policy): make deployment policy capability-aware and isolate privileged roles#14Nickfost wants to merge 14 commits into
Conversation
…vileged roles Declare organization.github_plan (free/team/enterprise, omitted = free, fail closed) and gate each environment's approval on a mechanism the plan can actually provide: github-environment requires team/enterprise because GitHub Free private repositories have no protected Environments; manual-external requires recorded exact-head approval_evidence that never names ordinary-CI state as its own approver. Host-group roles become logical inventory labels (deployment, persistent-testing, image-build) pending core ci-fleet #22/#23; the validator rejects routing-label reuse across pools and collisions with privileged host-group identities, and environments may target only deployment-role host groups. Schema, initializer (--github-plan), both example fleets (free/manual and team/environment paths), regression tests (16 new), and documentation are updated together per AGENTS.md. Closes #11
|
@codex review |
![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: 48201d4773
ℹ️ 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".
…olation - Include deployment-role host groups (and all declared groups) in the routing-label collision set; deployment hosts are privileged too. - Gate github-environment approval on github_plan enterprise only: required reviewers on private repositories need Enterprise, not Team. - Keep schema-v3 compatible: approval_mechanism becomes optional and the validator infers the fail-closed gate from the declared plan, so legacy environments keep validating without a data migration or version bump. - Match approval-evidence identity references as bounded hyphen-normalized phrases instead of raw substrings to stop mid-word false positives. - Type-guard pools in the evidence check so malformed runner_pools entries produce structural errors instead of an AttributeError traceback. Adds regression tests for each finding; suite is now 70 tests.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 882df10d85
ℹ️ 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".
…a optionality, and enum guards - approval_evidence self-approval check now tokenizes on every punctuation separator and fails closed when the CI identity is named beside run-output context (trusted-ci/job-log, trusted-ci: job log), while generic prose reuse of one identity word (the initializer's 'release ticket' with --runner-label release) stays valid, so init.sh no longer crashes for such options (Codex PR #14 threads). - fleet.schema.json no longer requires approval_mechanism, matching the semantic validator's schema-v3-compatible omission inference. - enum-valued fields (github_plan, controller state/lifecycle, host-group role/environment_class, approval_mechanism) are type-checked before set membership so malformed arrays/objects yield structural errors, not unhashable-type tracebacks.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f37d36d5a
ℹ️ 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".
…ties, and initializer approval locators Codex review round 3 (PR #14): - the actual pre-branch schema-v3 fleet.json omitted both approval_mechanism and approval_evidence; non-strict validation keeps that import contract while --strict still demands a locator before use, pinned by a regression test on the real e483998 fleet.json; - a CI identity that is itself a run-output word (run, job, workflow) no longer disables self-approval detection via the disjointness shortcut: exact token matches count for such identities; - init.sh writes a REPLACE-ME placeholder that --strict rejects until the operator passes --approval-evidence with a real locator, instead of a generic prose sentence strict mode silently accepted.
The strict placeholder rejection from ccbcd83 is intentional, so the workflow's happy-path initializer call now supplies a real approval locator and additionally proves that omitting one fails closed.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89ede920aa
ℹ️ 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".
…ass holes, enforce explicit manual gates in every mode Codex round-4 findings (PR #14): - marker-word CI identities no longer disable self-approval detection (complete ordered identity-phrase match replaces the disjointness shortcut and the fixed three-token window) - multi-token identities match as a whole phrase, so a single component near run-output vocabulary cannot reject legitimate external evidence - README quick-start passes --approval-evidence explicitly, keeping the documented onboarding command usable under strict validation - an explicitly selected manual-external gate requires its evidence in every validation mode; only environments omitting BOTH new fields keep the legacy schema-v3 omission contract
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f9ec52494
ℹ️ 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".
…xplicit nulls, and evidence locators - self-approval detection now also covers controller IDs and scale-set names: evidence citing their workflow logs is ordinary-CI self-approval - an explicitly null approval_mechanism fails closed instead of inheriting the schema-v3 legacy field-omission compatibility exception - approval_evidence locators must not contain host addresses or internal hostnames (IPv4, bracketed IPv6, >=3-label hostnames, private-TLD suffixes), keeping infrastructure details out of the Git-authored configuration per AGENTS.md
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6416a5f603
ℹ️ 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".
| r"(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}" | ||
| r"|\[[0-9a-f:]+\]" # bracketed IPv6 |
There was a problem hiding this comment.
When an operator records a bare IPv6 host address, such as approval recorded at 2001:db8::1 ticket RT-1042, this pattern does not match it because the IPv6 alternative requires square brackets. The configuration consequently passes validation while storing a forbidden host address; recognize unbracketed IPv6 literals as well as URL-style bracketed ones.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
| continue | ||
| host_group = environment.get("host_group") | ||
| validation.require(host_group in groups, f"{path}.host_group", "must reference a declared deployment host group") | ||
| if host_group in groups and groups[host_group].get("role") != "deployment": |
There was a problem hiding this comment.
When an environment references an existing host-group key whose value is malformed—for example, "development-apps": []—the earlier structural validation records an error, but this new .get("role") call raises AttributeError instead of returning the accumulated validation errors. Type-check the referenced group before both this role lookup and the later environment_class lookup.
Useful? React with 👍 / 👎.
| # configuration (Codex finding, PR #14). | ||
| mechanism = "github-environment" if environment_capable else "manual-external" | ||
| evidence = environment.get("approval_evidence") | ||
| validation.require(evidence is None or (isinstance(evidence, str) and bool(evidence.strip())), f"{path}.approval_evidence", "must be a logical reference to where exact-head approval is recorded, never a secret value") |
There was a problem hiding this comment.
When a non-approval environment explicitly sets approval_evidence: null, this condition accepts it and the repository's CI workflow, which invokes only validate.sh, reports no error, even though fleet.schema.json permits only strings for this field. Distinguish field omission from explicit null so the semantic validator and schema enforce the same contract.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
| EVIDENCE_RUN_OUTPUT_MARKERS = frozenset({ | ||
| "artifact", "artifacts", "console", "job", "jobs", "log", "logs", | ||
| "output", "run", "runner", "runners", "runs", "stdout", | ||
| }) |
There was a problem hiding this comment.
Fresh evidence after the earlier identity-matching fixes is that the marker set omits execution nouns such as workflow, action, and check, so production evidence like trusted-ci workflow 123 approved exact reviewed commit SHA passes validation despite citing the requesting CI workflow as its approval record. Include these CI execution forms in the self-approval detection so a manual-external gate cannot point back to the CI run under a different noun.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| # configuration (Codex finding, PR #14). | ||
| mechanism = "github-environment" if environment_capable else "manual-external" | ||
| evidence = environment.get("approval_evidence") | ||
| validation.require(evidence is None or (isinstance(evidence, str) and bool(evidence.strip())), f"{path}.approval_evidence", "must be a logical reference to where exact-head approval is recorded, never a secret value") |
There was a problem hiding this comment.
When approval evidence contains URI userinfo on a two-label host, such as approval at https://reviewer:s3cr3t@example.com/RT-1042, this free-form string passes both semantic validation and the existing high-confidence secret scan, committing the username and password to the repository. Reject credential-bearing URI syntax in this field, or constrain the locator to a structured non-secret reference.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
…dential guards Reject unbracketed IPv6 literals, type-guard malformed host groups before reading role, distinguish omitted vs explicit-null approval_evidence, extend CI execution marker vocabulary, and reject credential-bearing URI userinfo in approval evidence.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30f8c224dc
ℹ️ 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".
| evidence_present = "approval_evidence" in environment | ||
| evidence = environment.get("approval_evidence") | ||
| validation.require( | ||
| (not evidence_present and evidence is None) or (isinstance(evidence, str) and bool(evidence.strip())), |
There was a problem hiding this comment.
When an operator supplies arbitrary nonempty prose such as --approval-evidence "the exact reviewed commit SHA was approved", strict validation succeeds even though no ticket, path, or system reference identifies an approval record. Fresh evidence after the initializer-placeholder fix is that replacing the placeholder with this equally generic text bypasses the gate because this condition checks only that the string is nonempty; require a structured or otherwise verifiable locator so production cannot claim an external approval that cannot be found.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| return False | ||
|
|
||
|
|
||
| FORBIDDEN_CREDENTIAL_USERINFO = re.compile(r"//[^@/\s]+:[^@/\s]+@") |
There was a problem hiding this comment.
When approval evidence contains URI userinfo without a colon, such as https://s3cr3t@example.com/RT-1042, strict validation accepts it even though the userinfo can itself be a credential or token committed to Git. Fresh evidence after the password-style userinfo fix is that this regex requires username:password@; reject any userinfo before @, not only the colon-delimited form.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
|
|
||
|
|
||
| def evidence_contains_bare_ipv6(text: str) -> bool: | ||
| for token in text.split(): |
There was a problem hiding this comment.
When an unbracketed IPv6 address is followed or surrounded by ordinary punctuation, such as approval recorded on (2001:db8::1), ticket RT-1042, the whitespace-only tokenization leaves the punctuation attached and the hexadecimal regex does not match, so strict validation accepts the forbidden address. Fresh evidence after the bare-IPv6 fix is that both parenthesized and comma-suffixed literals bypass it; extract candidate literals independently of surrounding prose punctuation.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30f8c224dc
ℹ️ 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".
| EVIDENCE_RUN_OUTPUT_MARKERS = frozenset({ | ||
| "artifact", "artifacts", "action", "actions", "check", "checks", | ||
| "console", "job", "jobs", "log", "logs", | ||
| "output", "run", "runner", "runners", "runs", "stdout", | ||
| "workflow", "workflows", |
There was a problem hiding this comment.
When manual evidence names the requesting identity using another common execution noun, such as trusted-ci pipeline 123 approved exact reviewed commit SHA or trusted-ci build 123, this marker set does not recognize it, so the configuration accepts the CI execution as its own external production approval. Fresh evidence after the workflow/action/check fix is that the current set still omits pipeline and build; include these forms or replace the open-ended vocabulary heuristic with structured evidence.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| FORBIDDEN_EVIDENCE_ADDRESS = re.compile( | ||
| r"(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}" | ||
| r"|\[[0-9a-f:]+%?[^\]]*\]" # bracketed IPv6 with optional zone | ||
| r"|[a-z0-9-]+(?:\.[a-z0-9-]+){2,}" # >=3-label hostname |
There was a problem hiding this comment.
When an approval locator includes an ordinary semantic version, such as release 1.2.3 approved in ticket RT-1042, the generic three-label-hostname alternative matches 1.2.3 and rejects the otherwise valid evidence as an infrastructure address. Constrain hostname detection to actual hostname/URL contexts or exclude dotted numeric release versions so strict validation does not block routine release records.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
| config = json.loads( | ||
| subprocess.check_output(["git", "show", "e483998:fleet.json"], text=True) | ||
| ) |
There was a problem hiding this comment.
When a private repository is created through GitHub's “Use this template” flow, it receives a new snapshot commit without the template's ancestry, so e483998 does not exist and this git show raises CalledProcessError. The Run policy regression tests step in .github/workflows/validate.yml therefore fails for every freshly created adopter repository; construct the legacy configuration in the test or store it as a versioned fixture instead of depending on an upstream commit object.
Useful? React with 👍 / 👎.
…ndent legacy fixture - Require structured external approval locator (type:value) only for requires_approval environments so the initializer's development evidence and the committed CI init step stay valid (round-7: real external locator). - Reject token-only URI userinfo, pipeline/build self-approval markers, punctuated bare IPv6, and accept semantic versions in evidence locators. - Vendor the schema-v3 legacy fixture instead of reading e483998 from repository history, so templated adopter repos validate without the upstream ancestry (round-7: history-dependent legacy fixture). - Align the CI initializer step and README onboarding command with the structured locator requirement. Tests: 94 policy tests green; validate.sh, secret scan, syntax, and the initializer strict round-trip pass.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1298bdde9d
ℹ️ 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".
| # a production gate with no real locator must still fail (round 7). | ||
| if strict and requires_approval: | ||
| validation.require( | ||
| ":" in evidence and evidence.count(":") >= 1 and evidence.index(":") > 0 and evidence.rindex(":") < len(evidence) - 1, |
There was a problem hiding this comment.
When strict production evidence is arbitrary prose containing a colon, such as the exact reviewed commit SHA was approved: yes, this condition succeeds even though it does not identify any external approval record. The new check therefore remains bypassable after the structured-locator fix; validate an allowed prefix such as ticket:, url:, system:, or doc: and a corresponding nonempty locator.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| identities = {str(pool_name), str(pool.get("runner_group") or "")} | ||
| labels = pool.get("routing_labels") | ||
| if isinstance(labels, list): | ||
| identities.update(str(label) for label in labels if isinstance(label, str)) | ||
| hit = next((identity for identity in identities if evidence_names_ci_state(evidence, identity)), None) |
There was a problem hiding this comment.
When manual production evidence directly references the requesting GitHub Actions execution, for example url:https://github.com/acme/app/actions/runs/123, strict validation accepts it because this check requires a configured pool, label, controller, or scale-set identity to appear. A standard Actions run URL is still ordinary-CI state and cannot serve as external approval merely because it omits those local identity strings; detect known CI execution locators independently of configured identity matching.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| r"|(?!\d+(?:\.\d+){2}\b)[a-z0-9-]+(?:\.[a-z0-9-]+){2,}" # >=3-label hostname (not semantic version) | ||
| r"|[a-z0-9-]+(?:\.[a-z0-9-]+)*\.(?:internal|local|lan|corp|private|home|intranet)\b)", |
There was a problem hiding this comment.
When an approval locator contains an internal single-label host, such as url:http://ci-runner/RT-1042, strict validation accepts it because the hostname alternatives require either at least three labels or a recognized private suffix. These names are common for host-local services and expose exactly the infrastructure details this evidence scan is intended to block; parse URL hosts and reject unqualified hostnames as well.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
Validate the approval-evidence locator type (ticket:/url:/system:/doc:) instead of any colon, reject ordinary-CI execution URLs referenced directly as manual-external evidence, and reject unqualified single-label hosts. All three are bypasses of the existing structured-locator and address scan that Codex flagged at exact head 1298bdd; add regression tests for each.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 693f9abd25
ℹ️ 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".
| # on requires_approval so an initializer's non-gating evidence (the | ||
| # development environment) and legacy non-strict imports stay valid; | ||
| # a production gate with no real locator must still fail (round 7). | ||
| if strict and requires_approval: |
There was a problem hiding this comment.
When a production environment explicitly selects manual-external but supplies arbitrary prose, the strict guard skips the only check that requires a findable locator; the Validate reference configurations workflow runs lines 60–61 without --strict, and the current unstructured production evidence therefore passes CI. Explicitly declared new policy fields are not part of the legacy-omission case, so enforce the structured locator in every mode for an explicit manual gate to prevent production configuration from claiming an approval record that cannot be located.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| FORBIDDEN_EVIDENCE_ADDRESS = re.compile( | ||
| r"(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}" | ||
| r"|\[[0-9a-f:]+%?[^\]]*\]" # bracketed IPv6 with optional zone | ||
| r"|(?!\d+(?:\.\d+){2}\b)[a-z0-9-]+(?:\.[a-z0-9-]+){2,}" # >=3-label hostname (not semantic version) |
There was a problem hiding this comment.
When an external approval record uses a public three-label service such as url:https://acme.atlassian.net/browse/RT-1042, this alternative matches the hostname and strict validation rejects it as an internal infrastructure detail. This blocks the newly supported url: locator type for common external SaaS records; parse URL hosts and reject actual IP literals, single-label names, and private suffixes rather than treating every hostname with three or more labels as host-local.
AGENTS.md reference: AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
| validation.require(engine == "RandomDevelopment/ci-fleet", "$.organization.delivery_engine", "must use the fixed reviewed public engine repository") | ||
| validation.require(organization.get("workflow_ref_policy") == "immutable-commit", "$.organization.workflow_ref_policy", "must equal immutable-commit") | ||
| plan = organization.get("github_plan") | ||
| validation.require(plan is None or (isinstance(plan, str) and plan in {"free", "team", "enterprise"}), "$.organization.github_plan", "must be free, team, or enterprise; omitted means free") |
There was a problem hiding this comment.
When a configuration explicitly sets organization.github_plan to null, this condition treats it like field omission and the repository's semantic validator and CI accept it, even though fleet.schema.json permits only free, team, or enterprise. Distinguish key absence from an explicit null, as the environment approval fields already do, so the authoritative validator and schema enforce the same contract.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
Enforce the structured approval locator for an explicitly declared manual-external gate in every mode (not only --strict), so the shipped template's own prose evidence can no longer pass the non-strict reference validation; record a real ticket: locator in fleet.json accordingly. Distinguish public multi-label SaaS approval URLs from host-local infrastructure: only IP literals, single-label hosts, and private suffixes are rejected, so url:https://acme.atlassian.net/... is accepted. Reject an explicit null organization.github_plan like the approval fields already reject explicit nulls.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc06b7d944
ℹ️ 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".
| return False | ||
|
|
||
|
|
||
| FORBIDDEN_CREDENTIAL_USERINFO = re.compile(r"//[^@/\s]+(?:[:][^@/\s]+)?@") |
There was a problem hiding this comment.
When a url: locator is query-authenticated or presigned, such as url:https://approvals.example.com/RT-1042?token=s3cr3t, validation succeeds because this pattern only recognizes URI userinfo, while the generic secret scan does not inspect parameter names embedded inside a string. Fresh evidence after the userinfo fixes is that token=, password=, and signature query parameters can still commit credentials through approval_evidence; reject credential-bearing query/fragment parameters or disallow them in approval URLs.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| if token.startswith("[") and token.endswith("]"): | ||
| continue | ||
| # Strip surrounding punctuation before checking for bare IPv6 | ||
| stripped = token.strip(".,;:()[]{}<>\"'") |
There was a problem hiding this comment.
When a bare IPv6 address begins or ends with compression, such as ::1, fe80::, or 2001:db8::, this strip removes the colons that are part of the address before parsing, leaving fewer than two colons and allowing the forbidden host address through validation. Fresh evidence after the punctuated-IPv6 fix is that treating : as surrounding punctuation breaks valid leading- and trailing-compressed literals; trim only punctuation that cannot be part of IPv6 syntax.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
| # are not addresses. | ||
| EVIDENCE_PRIVATE_HOST = re.compile( | ||
| r"(?:localhost\b" | ||
| r"|\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}" |
There was a problem hiding this comment.
When approval evidence includes a four-part release or calendar version, such as ticket:RT-1042 release 2026.8.28.1 approved, this alternative rejects it as a host address even though 2026 makes it invalid IPv4. Fresh evidence after allowing three-part semantic versions is that every four-component numeric value is still classified as IPv4 without validating octet ranges or address context; parse actual IP literals rather than matching any dotted quartet.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
| if args.github_plan != "enterprise" and not args.approval_evidence: | ||
| fail("--approval-evidence is required when --github-plan is not enterprise (structured locator: type:value, e.g. ticket:RT-1042 or url:https://tracker.example/RT-1042)") |
There was a problem hiding this comment.
When the Validate configuration workflow exercises its invalid-registry case at .github/workflows/validate.yml:75-83, it omits --approval-evidence, so this new early exit rejects the invocation before BAD reaches registry validation. I confirmed that the command now fails solely with the missing-evidence error, meaning the workflow remains green even if initializer registry validation regresses; pass valid approval evidence in that negative test so it fails for the condition it claims to cover.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
Reject credential-bearing query/fragment parameters (token=/password=/sig=) in approval URLs, not only URI userinfo. Keep IPv6 compression colons during punctuation trimming so ::1 and fe80:: are still rejected. Require valid octets and digit delimiters for IPv4 so four-part calendar versions such as 2026.8.28.1 are not mistaken for addresses. Pass approval evidence in the initializer invalid-registry workflow case so it exercises registry validation instead of failing on the missing-evidence gate.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a28ead054e
ℹ️ 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".
| FORBIDDEN_CREDENTIAL_PARAM = re.compile( | ||
| r"[?&#][^=&\s]*\b(?:token|password|passwd|secret|sig|signature|key|api[_-]?key)" | ||
| r"=[^\s&]+", |
There was a problem hiding this comment.
When an approval URL uses a standard compound credential name such as ?access_token=s3cr3t, ?client_secret=s3cr3t, or ?private_token=s3cr3t, this pattern does not match because _ is a word character and the \b before token/secret cannot occur after it; validation therefore accepts and commits the credential. Fresh evidence after the prior query-parameter fix is that these common OAuth and API parameter names still pass errors_for unchanged, so parse parameter names or explicitly cover compound secret-bearing names.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
Cover compound OAuth/API credential names (access_token, client_secret, private_token) in approval URLs by allowing an optional _ or - separator before the keyword instead of a word boundary, which cannot occur after an underscore.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 986616e284
ℹ️ 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".
| # are covered by allowing an optional _ or - separator before the keyword | ||
| # (Codex round 10). | ||
| FORBIDDEN_CREDENTIAL_PARAM = re.compile( | ||
| r"[?&#][^=&\s]*(?:_|-)?(?:token|password|passwd|secret|sig|signature|key|api[_-]?key)" |
There was a problem hiding this comment.
After the compound-name fix, a manual approval locator such as url:https://approvals.example/RT?credential=s3cr3t still passes strict validation because this pattern omits credential (and the generic secret scan does not inspect parameter names embedded in strings). This commits an explicitly credential-bearing value through the new evidence field, so parse and validate decoded query parameters or cover the repository's full forbidden credential-key set.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| # evidence even when they omit the local pool/controller identity strings | ||
| # (Codex PR #14 round 8). | ||
| CI_EXECUTION_URL = re.compile( | ||
| r"https?://[^\s/]+(?:/[^\s/]+){2,}/(?:actions|runs?|jobs?|builds?|pipelines?|checks?)\b", |
There was a problem hiding this comment.
Fresh evidence after the GitHub Actions URL fix is that the regex requires at least two path segments before the execution marker, so a standard locator such as url:https://app.circleci.com/pipelines/github/acme/app/123 passes strict validation without naming a configured local identity. That lets the requesting CI pipeline serve as its own production approval record; detect known execution segments regardless of their path position.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| def evidence_mentions_single_label_host(text: str) -> bool: | ||
| # Unqualified (single-label) hosts name host-local services and leak the | ||
| # infrastructure details the evidence scan must block (Codex PR #14 round 8). | ||
| for match in re.finditer(r"https?://([^\s/@]+)", text, re.IGNORECASE): |
There was a problem hiding this comment.
Fresh evidence after the HTTP single-label-host fix is that url:ssh://ci-runner/RT-1042 passes strict validation because host extraction is limited to http and https, while the structured url: locator accepts any scheme. An operator can therefore commit an internal hostname and SSH detail through approval evidence; parse the locator's authority independently of scheme, or restrict url: locators to approved schemes.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
…eme findings Cover the credential= query parameter name. Detect ordinary-CI execution segments (actions/runs/jobs/builds/pipelines/checks) at any path depth and under any scheme, not only after two http(s) path segments. Scan every url: scheme for single-label hosts, not only http(s), so ssh://ci-runner and similar internal hosts are still blocked.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 422d3f975f
ℹ️ 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 PR #14 round 8). Detect known execution segments at any path depth | ||
| # and under any scheme (Codex round 11). | ||
| CI_EXECUTION_URL = re.compile( | ||
| r"[a-z][a-z0-9+.-]*://[^\s/]+(?:/[^/\s]+)*/(?:actions|runs?|jobs?|builds?|pipelines?|checks?)\b", |
There was a problem hiding this comment.
When a manual production gate cites the requesting CI execution through another supported locator type, such as system:circleci/pipelines/123, validation succeeds because this expression only recognizes scheme://host/... URLs and the locator does not name a configured local identity. Fresh evidence after the any-depth URL fix is that system: CI-run references still let ordinary CI serve as its own production approval record; apply execution-marker detection to every structured locator type.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
|
|
||
| # Structured external-approval locators carry a typed prefix; a bare colon in | ||
| # arbitrary prose is not a locator (Codex PR #14 round 8). | ||
| EVIDENCE_LOCATOR_RE = re.compile(r"^(?:doc|system|ticket|url):[^\s]") |
There was a problem hiding this comment.
When approval_evidence uses an authority-less scheme, such as url:data:text/plain,password=s3cr3t, strict validation accepts it because url: permits any non-whitespace payload while the later URL checks expect authority or query syntax. Fresh evidence after broadening host inspection beyond HTTP is that opaque schemes can still embed a credential directly (and file:///home/deployer/.ssh/config similarly exposes host-local details); restrict url: to approved external schemes or fully parse and validate its payload.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| r"[?&#][^=&\s]*(?:_|-)?(?:token|password|passwd|secret|sig|signature|key|api[_-]?key|credential)" | ||
| r"=[^\s&]+", |
There was a problem hiding this comment.
When an approval URL carries a credential under a standard authentication name, such as url:https://approvals.example/RT-1042?auth=s3cr3t or ?authorization=BearerSecret, validation succeeds because neither parameter name contains one of these suffixes. Fresh evidence after the credential-name fixes is that query-authenticated approval links can therefore still commit credentials; parse decoded parameter names and reject authentication-bearing parameters rather than relying on this incomplete vocabulary.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| # locator in every mode; strict gating also covers inferred gates | ||
| # and legacy non-strict imports (round 7), so a production gate with | ||
| # no real locator cannot pass CI (Codex PR #14 round 8). | ||
| if (declared_mechanism == "manual-external" and requires_approval) or (strict and requires_approval): |
There was a problem hiding this comment.
When a Free or undeclared-plan production environment omits approval_mechanism but includes unstructured approval_evidence, such as the exact reviewed commit SHA was approved: yes, normal validation succeeds because this condition requires a structured locator only for an explicitly declared manual mechanism or in strict mode. Fresh evidence after enforcing explicit manual gates is that this one-new-field shape is not the tolerated legacy omission of both fields, yet the non-strict reference-validation workflow accepts it; enforce the locator contract whenever the inferred mechanism is manual-external and evidence is present.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| controller_ids = list(controllers) if isinstance(controllers, dict) else [] | ||
| scale_sets = [ | ||
| str(controller.get("scale_set_name")) | ||
| for controller in controllers.values() |
There was a problem hiding this comment.
When controllers is structurally malformed as a non-object, such as "controllers": [], the earlier check records the expected validation error, but an environment with string approval evidence reaches this comprehension and calls .values() on the list before its isinstance(controllers, dict) filter can run. The authoritative validator therefore exits with an AttributeError instead of reporting the accumulated configuration errors; guard the collection before constructing controller and scale-set identities.
Useful? React with 👍 / 👎.
1 participant
Problem
Issue #11: the template claims production is gated by GitHub Environment approval, but GitHub Free organizations cannot use protected Environments (or Environment secrets) on private repositories. Host-group roles also carry no machine-checkable separation between ordinary CI, persistent testing, image building, and deployment, so a template adopter can declare privileged hosts with no isolation guarantee.
Approach
organization.github_plan(free/team/enterprise; omitted = free, fail closed).environments[].approval_mechanism:github-environmentvalidates only on a plan that provides protected Environments;manual-externalrequiresapproval_evidencerecording where the exact reviewed commit SHA was approved — never a secret value, never ordinary-CI state as its own approver.rolebecomes logical inventory (deployment,persistent-testing,image-build) pending core #22/#23; no invented runtime fields.--github-planand emits the matching mechanism for both paths.Tests
scripts/test_policy.pycovering capability overclaims, self-approval evidence, missing evidence, non-deployment environment targets, role vocabulary, label collisions, and cross-pool label reuse. Sabotage run against the old validator: 15/16 fail (the remaining one asserts valid configs stay valid), proving the tests bite.python3 scripts/test_policy.py→ 63 tests OK../scripts/validate.shand--config examples/multi-host/fleet.json→ OK (matches CI). Note--strictfails identically on baseline main by design (fictional example values); CI does not run strict mode.Risk / exclusions
approval_mechanism(documented in README + AGENTS hard rules).Closes #11