Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 387a647355
ℹ️ 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".
| isinstance(node.func, ast.Attribute) | ||
| and node.func.attr in python_path_filesystem_read_methods | ||
| and python_path_receiver_expression( | ||
| node.func.value, tree, parents | ||
| ) |
There was a problem hiding this comment.
The new receiver guard skips validation entirely when a Path constructor is aliased: factory = Path; print(factory("synthetic-private").read_text()) now returns no scanner violation, whereas the parent scanner rejected it. Any unrecognized factory or helper returning a Path similarly bypasses all filesystem-reader checks and can expose private files through packet output, so unresolved receivers using these reader names must remain fail-closed.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| elif isinstance(node, (ast.For, ast.AsyncFor, ast.comprehension)): | ||
| # Values yielded from an environment iterator remain sensitive in | ||
| # both loop bodies and comprehension elements. | ||
| assignments.append((node.target, node.iter)) |
There was a problem hiding this comment.
When an environment iterator is wrapped, recording only the outer iterable expression loses the taint: for _, value in enumerate(os.environ.values()): print(value) and the analogous zip(...) case both pass this scanner. Those snippets still print inherited values, including credentials, so iterable taint must recurse through ordinary iterator-wrapper calls rather than recognizing only a direct os.environ.values() call.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| if python_call_derived_command_alias(value, modules): | ||
| return True | ||
| if isinstance( |
There was a problem hiding this comment.
The new helper traverses literal containers but falls through on ordinary calls, so launchers = {"x": subprocess.run}; for launch in launchers.values(): launch(["gh", "workflow", "run", "ci.yml"]) is accepted. This leaves the iterable-alias finding open for dictionary views and container-conversion calls, allowing a hidden workflow replay or other process launch to pass packet certification; the iterable analysis needs to follow reviewed view/wrapper calls back to their source containers.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
| for statement in module.body: | ||
| if isinstance(statement, (ast.Import, ast.ImportFrom)): | ||
| exec(compile(ast.Module(body=[statement], type_ignores=[]), "<packet-scanner-import>", "exec"), namespace) | ||
| elif isinstance(statement, (ast.FunctionDef, ast.AsyncFunctionDef)): | ||
| exec(compile(ast.Module(body=[statement], type_ignores=[]), "<packet-scanner-function>", "exec"), namespace) |
There was a problem hiding this comment.
When the Markdown scanner fence is modified, this setup executes every extracted import and function definition with full builtins before any specimen is inspected. An added import, decorator, or default-argument expression can therefore run arbitrary code merely by invoking the advertised offline regression harness, contradicting its non-executing contract; validate and whitelist these AST nodes without evaluating packet-controlled imports or definition-time expressions.
Useful? React with 👍 / 👎.
| def config_violation(assignment): | ||
| key = assignment.split("=", 1)[0].lower() | ||
| if git_config_include_key(key): | ||
| return "Git configuration includes are not allowed before read-only commands" |
There was a problem hiding this comment.
The new check covers include keys supplied as -c, --config-env, or git config operands but still accepts git config --includes --list and command-prefix injection such as GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=include.path GIT_CONFIG_VALUE_0=/tmp/inc git status. git config -h describes --[no-]includes as “respect include directives on lookup,” and a local probe confirmed the environment form loads values from the referenced file, so these forms can still read sensitive configuration or activate included Git behavior after passing certification; reject include-enabling flags and inspect Git configuration environment assignments before wrapper stripping.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| @@ -0,0 +1,582 @@ | |||
| """Offline, non-executing regression probes for the seven issue #79 findings. | |||
There was a problem hiding this comment.
This commit introduces a Python implementation without an ADR or evidence-backed exception, even though the repository requires Go unless an ADR changes that decision. Reimplement the harness using the repository's Go tooling or add the required accepted ADR before introducing this non-Go maintenance surface.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
| executable = executable_basename(tokens[0]) | ||
| if executable == "set": |
There was a problem hiding this comment.
The environment-dump check handles set and export but still accepts bare env and forms such as env -0 or env -u NAME; the wrapper parser then strips env, finds no child executable, and certifies the command. env --help explicitly states, “If no COMMAND, print the resulting environment,” so an executable shell fence can still emit every inherited credential despite this hardening; reject env whenever wrapper parsing reaches the no-command case.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
|
Triage checkpoint: Codex completed review of 387a647 with seven P1 findings. All are being treated as unresolved premerge blockers; current quick-check success does not override them. Independent contract/security reviews also reproduced overlapping reader/iterator gaps. A single correction batch with focused red/green evidence is in progress; no merge until final-head independent delta review, a fresh @codex review completion and quick checks. No live operations or workflow replay are authorized by this fix batch. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 387a647355
ℹ️ 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".
| ) | ||
| ): | ||
| raise SystemExit("Git packet blob query was outside the reviewed revision/path") | ||
| command = git_query(["show", blob_spec]) |
There was a problem hiding this comment.
When the local repository contains refs/replace/<HEAD>, this git show resolves the replacement object, while rev-parse HEAD and the remote ls-remote comparison retain the original commit ID. I reproduced a clean worktree where the original HEAD contained one packet, a replacement commit contained different bytes, and both this blob query and the worktree returned the replacement bytes while HEAD still matched the original remote SHA. git --help lists the global [--no-replace-objects] option; use it for the exact-head queries or explicitly reject replacement refs, otherwise the template can certify content that is absent from the reviewed remote head.
AGENTS.md reference: AGENTS.md:L16-L16
Useful? React with 👍 / 👎.
| guarded = any( | ||
| isinstance(call, ast.Call) | ||
| and python_dotted_name(call.func) == "package_dir.relative_to" | ||
| and len(call.args) == 1 | ||
| and python_path_division_names(call.args[0]) == expected_root |
There was a problem hiding this comment.
When an executable fence defines source_fuzz_guard with these expected variable names but calls it with arbitrary roots, this check only proves that package_dir is below the caller-supplied go_repo_root / module_dir. I loaded the scanner and it accepted a helper invoked as source_fuzz_guard(Path("/"), Path("etc"), Path("")) that prints package_dir.glob("*"), allowing filesystem enumeration to enter packet output. Unlike the unresolved-receiver finding, this uses a directly recognized Path receiver and passes specifically through the new exemption; require provenance for the root or validate every call site rather than trusting the helper's name and parameter spelling.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dac58b4add
ℹ️ 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".
| if isinstance(node.func, ast.Name): | ||
| local_helpers = python_local_function_candidates( | ||
| node.func.id, node, tree, parents |
There was a problem hiding this comment.
Restricting helper resolution to calls whose target is an ast.Name lets other local callables launder inherited credentials. For example, a class method that returns dict(os.environ), followed by secret = Snapshot().read(); print(secret), receives no scanner violation and emits the environment; a lambda assigned to a name has the same result because it is absent from the function index. Resolve method/lambda bodies too, or fail closed when their return provenance is unknown.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| elif isinstance(node, (ast.For, ast.AsyncFor, ast.comprehension)): | ||
| # Values yielded from an environment iterator remain sensitive in | ||
| # both loop bodies and comprehension elements. | ||
| assignments.append((node.target, node.iter)) |
There was a problem hiding this comment.
When an outer loop consumes a comprehension, this new binding records the whole comprehension as the target's source, but that expression is not recognized as sensitive when its iterator is the call os.environ.values(). The fresh case for secret in [value for value in os.environ.values()]: print(secret) still passes after the enumerate/zip fix and prints inherited credentials; list, set, dict, and generator comprehensions need to propagate taint from their elements and generators.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| local_functions = {} | ||
| for node in ast.walk(tree): | ||
| if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): | ||
| local_functions.setdefault(node.name, []).append(node) |
There was a problem hiding this comment.
Indexing only FunctionDef/AsyncFunctionDef nodes leaves other local factories outside launcher provenance. I checked factory = lambda: subprocess.run; launch = factory(); launch(["gh", "workflow", "run", "ci.yml"]) and the equivalent class-method factory, and both receive no scanner violation even though they execute a workflow dispatch. Include lambdas and callable methods in return analysis, or reject aliases produced by unresolved local calls.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
| current = node | ||
| while current in parents: | ||
| parent = parents[current] | ||
| if isinstance(parent, ast.If): | ||
| condition = condition_value(parent.test) |
There was a problem hiding this comment.
The reachability evaluator treats unrecognized constant conditions as unknown, so wrapping the containment try in if 1 == 1: pass; else: ... is accepted even though the check can never run; I applied that mutation to the canonical source-fuzz body and inspect_python_heredoc returned no violation. With an escaping package argument, execution continues to package_dir.glob without proving containment, allowing files outside the reviewed module to be inspected. Require the containment try in the direct reachable function body instead of accepting it beneath partially evaluated branches.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| def _verification_function(module: ast.Module, name: str) -> ast.FunctionDef: | ||
| for statement in module.body: | ||
| if isinstance(statement, ast.FunctionDef) and statement.name == name: | ||
| return statement |
There was a problem hiding this comment.
Returning the first matching function lets the harness validate a safe definition while Python executes a later override. I inserted a second require_packet_head_parity that simply returns immediately before its call: the packet scanner reported no violation, _verification_function still selected the original verifier, but the live template would bind and invoke the no-op definition, allowing mismatched packet bytes or intent bits to pass. Require exactly one top-level helper with this name before compiling it.
AGENTS.md reference: AGENTS.md:L16-L16
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b79709bf4c
ℹ️ 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".
| if ( | ||
| isinstance(value, ast.Attribute) | ||
| and value.attr in python_path_filesystem_read_methods | ||
| and not python_known_non_path_reader_call(value, tree) | ||
| ): | ||
| receiver = value.value | ||
| elif isinstance(value, ast.Name) and value.id in aliases: | ||
| receiver = aliases[value.id] |
There was a problem hiding this comment.
Fresh evidence after the direct-receiver fix is def factory(): return Path("synthetic-private/file").read_text; reader = factory(); print(reader()): the current scanner returns no violation and the snippet emits arbitrary file contents. This alias analysis follows only direct attributes and existing names, not call results, even though local return analysis already exists elsewhere; propagate returned Path reader callables or fail closed on the unresolved alias.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| if ( | ||
| isinstance(value.func, ast.Attribute) | ||
| and value.func.attr in {"items", "keys", "values"} | ||
| ): |
There was a problem hiding this comment.
Fresh evidence after the dictionary-view fix is launchers = {"x": subprocess.run}; launch = launchers.pop("x"); launch(["gh", "workflow", "run", "ci.yml"]), which the scanner accepts. I checked gh 2.96.0's workflow run --help, which states that this command creates a workflow_dispatch event and triggers the workflow, so limiting attribute-call propagation to items/keys/values still permits an automatic replay through pop; mapping element-producing methods must preserve launcher provenance.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
| for node in ast.walk(tree): | ||
| if not isinstance(node, ast.Call) or not isinstance(node.func, ast.Name): | ||
| continue | ||
| function = local_functions.get(node.func.id) | ||
| if function is None: | ||
| continue | ||
| for parameter, argument in call_arguments(node, function): | ||
| if python_sensitive_value_expression( | ||
| argument, sensitive_names, tree, parents | ||
| ) and parameter not in sensitive_names: | ||
| sensitive_names.add(parameter) | ||
| changed = True | ||
| for function in python_local_function_candidates( | ||
| node.func.id, node, tree, parents | ||
| ): |
There was a problem hiding this comment.
Fresh evidence exercises argument flow rather than the previously fixed return flow: class C: def emit(self, payload): print(payload); C().emit(os.environ) receives no violation and prints the inherited environment; an assigned lambda with the same body also passes. The call-site taint pass considers only calls whose target is an ast.Name and resolves only function definitions, so method and lambda parameters never become sensitive; bind arguments for those local callable forms as well.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| packet_path = Path("docs/evidence/g01-recovery-packet.md") | ||
| intent_result = run_bounded_git_query( | ||
| git_query(["ls-files", "-v", "-z", "--", packet_path.as_posix()]), | ||
| cwd=Path.cwd(), | ||
| env=git_environment, |
There was a problem hiding this comment.
When scripts/evidence_packet/issue79_regression_test.py is marked assume-unchanged or skip-worktree and then edited to weaken or skip its tests, this packet-only intent/blob check still succeeds and the later git status remains empty, so test output from modified code can be paired with the reviewed remote HEAD. I checked the installed git update-index -h, which describes these flags as marking files “not changing” or “index-only”; reject such intent bits and compare HEAD bytes for every changed evidence source, not only the Markdown packet.
AGENTS.md reference: AGENTS.md:L16-L16
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9233241cbd
ℹ️ 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".
| if isinstance(value, ast.Call) and isinstance(value.func, ast.Name): | ||
| class_names.add(value.func.id) | ||
| for returned in python_local_call_return_values(value, tree, parents): |
There was a problem hiding this comment.
Fresh evidence after the direct method-return correction is Alias = Snapshot; print(Alias().read()), where Snapshot.read returns dict(os.environ): the scanner reports no violation and prints the inherited environment. This resolver records the called name Alias as the class name without following its assignment to Snapshot, so both sensitive method returns and parameter flows can still be laundered through a class alias; resolve constructor aliases before looking up the method.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| isinstance(value.func, ast.Attribute) | ||
| and value.func.attr in {"items", "keys", "values", "pop"} | ||
| ): |
There was a problem hiding this comment.
Fresh evidence after the .pop correction is launchers = {"x": subprocess.run}; launch = launchers.get("x"); launch(["gh", "workflow", "run", "ci.yml"]), which the scanner accepts because this mapping-method list omits get. I checked gh 2.96.0's workflow run --help, which says it creates a workflow_dispatch event and triggers the workflow, so mapping lookup can still hide an automatic workflow replay; preserve launcher provenance through get as well.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
| if dotted not in path_preserving_calls and not ( | ||
| isinstance(node.func, ast.Attribute) | ||
| and node.func.attr in {"as_posix", "as_uri"} | ||
| ): | ||
| return False |
There was a problem hiding this comment.
When a resolved path is converted with the built-in formatter, value = format(Path.cwd().resolve()); print(value) receives no scanner violation and emits the local worktree path. Because an unrecognized call returns False here instead of propagating path provenance from its arguments, the new output-sink protection can be bypassed by format (and similarly ascii or an explicit __str__ call); include these path-preserving conversions or conservatively trace their arguments.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| The harness must use only the standard library and explicitly selected local | ||
| Git fixture operations. Invoke it with `python3 -B` to avoid bytecode artifacts | ||
| and record the actual interpreter and test results. Adding dependencies, |
There was a problem hiding this comment.
When the maintainer environment has PYTHONPATH, a user-site sitecustomize, or another Python startup customization, the prescribed python3 -B command executes that code before the harness's import and definition-time validation. I reproduced this with a synthetic PYTHONPATH containing ast.py, which executed immediately on harness startup; Python's local --help confirms that -B only disables bytecode while -I isolates Python from the user environment. Invoke the harness with isolated mode so external startup/import code cannot access credentials or corrupt the recorded regression result.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a61c35fb8a
ℹ️ 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".
| for statement in candidate.body | ||
| for call in ast.walk(statement) |
There was a problem hiding this comment.
When the Markdown link check nests path.relative_to(repository_root) under if False, this broad ast.walk predicate still marks the path as guarded; I applied that mutation to the canonical link-check heredoc and inspect_python_heredoc returned no violation. A repository Markdown target such as ../../private/file#x can then reach path.is_file() and anchors(path) without containment, causing certification to read outside the reviewed repository; require the containment call to be a direct reachable statement in the guarded try.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| candidates = tuple(assignments.get(value.id, ())) + tuple( | ||
| iterable_bindings.get(value.id, ()) | ||
| ) |
There was a problem hiding this comment.
When a function parameter shadows a module-level regex-match variable, this global name-based lookup still classifies the parameter as a regex result. I verified that match = re.match("x", "x"); def expose(match): print(match.group()); expose(Path("/etc/passwd")) receives no scanner violation, even though runtime dispatches Path.group() and performs an unreviewed filesystem metadata read. Scope assignment and parameter bindings before exempting .group() as a regex API.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c755af9dc
ℹ️ 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".
| if dotted == "dict" and any( | ||
| isinstance(argument, ast.Attribute) | ||
| and python_dotted_name(argument) == "os.environ" | ||
| for argument in node.args | ||
| python_sensitive_value_expression( | ||
| value, sensitive_names, tree, parents, seen.copy() | ||
| ) | ||
| for value in list(node.args) |
There was a problem hiding this comment.
In the packet's isolated Python heredocs, aliasing a preserving constructor bypasses this name-based check: maker = dict; value = maker(os.environ); print(value) receives no scanner violation and prints the inherited environment. Resolve aliases of the listed container/string constructors before testing the call target, otherwise packet certification can emit credentials despite the new taint propagation.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| python_imported_function_alias_is_stable( | ||
| node.func.id, "sys", "exit", tree | ||
| ) | ||
| or python_assigned_callable_alias(node.func.id, "sys.exit", tree) | ||
| ) |
There was a problem hiding this comment.
The name-based alias branch only resolves sys.exit, so a packet heredoc such as emit = print; emit(os.environ) receives no scanner violation and writes the entire inherited environment; emit = warnings.warn is affected similarly. Extend alias resolution to all sinks recognized by this function so known credential taint cannot be laundered merely by assigning the sink to another name.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| assignments_by_name, | ||
| seen.copy(), | ||
| ): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
In the packet's isolated Python heredocs, print(Path.home()), print(Path("~").expanduser()), and print(os.path.expanduser("~")) all receive no scanner violation while emitting the maintainer's personal home path. This provenance check recognizes Path.cwd() but returns false for these other standard local-path constructors, so include home/expansion operations among resolved local path sources before output-sink certification.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| or value not in reviewed_shell_export_assignments | ||
| for value in assignments | ||
| ): | ||
| return "shell export requires explicit reviewed assignments" | ||
| return None |
There was a problem hiding this comment.
The new environment-output gate accepts every non-set/export command, but the shell allowlist includes processors with direct environment access: awk 'BEGIN { print ENVIRON["GH_TOKEN"] }' and jq -n env both receive no scanner violation, and local synthetic probes confirmed that each prints the supplied variable. Inspect or reject the environment primitives of these allowlisted processors so executable packet fences cannot emit inherited credentials.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| def _top_level_assignment(module: ast.Module, name: str) -> ast.Assign | ast.AnnAssign: | ||
| for statement in module.body: | ||
| if isinstance(statement, ast.Assign) and any( | ||
| isinstance(target, ast.Name) and target.id == name | ||
| for target in statement.targets | ||
| ): | ||
| return statement | ||
| if isinstance(statement, ast.AnnAssign) and isinstance(statement.target, ast.Name) and statement.target.id == name: |
There was a problem hiding this comment.
Fresh evidence beyond the duplicate-helper correction is a second top-level issue79_reviewed_evidence_paths assignment after the canonical one: this helper returns the first assignment and the regression assertion still sees both reviewed files, while Python executes the later value and the live parity loop can omit the harness entirely. Requiring each security-relevant template assignment to be unique prevents an assume-unchanged modified harness from being paired with the reviewed remote head after tests validated a shadowed value.
AGENTS.md reference: AGENTS.md:L16-L16
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
https://github.com/1XP-AI/gh-runnerd/blob/1f5f89bc80f09393dbc44f45d20f0141e747005a/docs/evidence/g01-recovery-packet.md#L11329-L11330
Fresh evidence after the launcher-alias corrections is import subprocess; (launch := subprocess.run)(["gh", "workflow", "run", "ci.yml"]), which the current scanner accepts. The installed gh 2.96.0 help states that workflow run creates a workflow_dispatch event and triggers the workflow, so limiting NamedExpr handling here to conditional values still permits an automatic replay when the assignment expression itself is the call target; resolve that target or fail closed on such callable expressions.
AGENTS.md reference: AGENTS.md:L8-L8
ℹ️ 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".
| "traceback.print_exception", | ||
| }: | ||
| return True | ||
| if isinstance(node.func, ast.Name) and tree is not None: |
There was a problem hiding this comment.
Fresh evidence after the assigned/import-alias fix is import os; (emit := print)(os.environ): the current scanner returns no violation, while Python immediately invokes print and emits the entire inherited environment. This branch only resolves sink aliases when the callable is an ast.Name, so a NamedExpr used directly as the call target bypasses the new protection; resolve callable expressions here or fail closed on unresolved targets.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| for value in assignments | ||
| ): | ||
| return "shell export requires explicit reviewed assignments" | ||
| return None |
There was a problem hiding this comment.
In an executable Bash fence, name=GH_TOKEN followed by printf '%s\n' "${!name}" receives no scanner violation and prints the inherited token; I confirmed this with a synthetic GH_TOKEN. The new environment-output gate accepts every command other than the special set/export cases, while the earlier parameter regex does not recognize Bash's ${!name} indirect expansion, so an allowlisted printf can still place credentials in packet output; reject indirect expansion or resolve its referenced variable before allowlisting the command.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e277abf1c
ℹ️ 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".
| sensitive_receiver = python_sensitive_value_expression( | ||
| node.func.value, sensitive_names, tree, parents, seen.copy() | ||
| ) | ||
| return sensitive_receiver |
There was a problem hiding this comment.
In an executable Python heredoc, secret = os.environ; print(urllib.parse.urlencode(secret)) receives no scanner violation, although executing it emits every inherited environment key and value, including credentials. This generic attribute-call branch propagates taint only from the receiver (urllib.parse) and discards the sensitive argument; inspect arguments for value-producing transformations such as urlencode, or otherwise fail closed so encoded credentials cannot enter packet output.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| git_config_bounded_regexp_queries = {r"^filter\."} | ||
| git_config_reviewed_query_keys = { | ||
| "--get": {"core.repositoryformatversion"}, | ||
| "--get-all": {"remote.origin.url"}, |
There was a problem hiding this comment.
When a maintainer checkout stores an authenticated HTTPS origin such as https://user:token@github.com/... (or a personal file:// path), an executable fence containing git config --local --get-all remote.origin.url receives no scanner violation and prints that sensitive value. The installed git config -h confirms that --get-all returns all values for the requested key, so allowlisting the key alone does not make its output public-safe; restrict this query to the captured-and-compared verification helper or validate/redact its value before permitting it in shell output.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| def target_names(target): | ||
| if isinstance(target, ast.Name): | ||
| return [target.id] | ||
| if isinstance(target, ast.Starred): | ||
| return target_names(target.value) |
There was a problem hiding this comment.
Fresh evidence after the name/parameter taint fixes is box.payload = os.environ; print(box.payload) (and equivalently box["payload"] = os.environ): the scanner returns no violation while execution prints the inherited environment. This target extractor records only names, starred names, and destructuring targets, so storing a sensitive value on an object or in a container erases its provenance; track these assignment targets or reject their later use at output sinks.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| for token in tokens: | ||
| if not assignment.fullmatch(token): | ||
| continue |
There was a problem hiding this comment.
Fresh evidence after the direct shell-assignment correction is an executable fence containing secret=${GH_TOKEN#x} followed by printf '%s\n' "$secret": the scanner returns no violation, while Bash prints the token whenever it does not begin with x. The recorder ignores assignment tokens that fail the narrow assignment.fullmatch, and the assignment-only line is then discarded, so parameter-removal/default operators can launder a credential into an ordinary variable; parse these expansions conservatively or reject unsupported assignment forms.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
https://github.com/1XP-AI/gh-runnerd/blob/a55fb9d1c9402bc65c0f40daa6673d6f447700f4/docs/evidence/g01-recovery-packet.md#L8035-L8038
An executable fence containing /tmp/git status --porcelain=v1 or ./git status --porcelain=v1 receives no scanner violation because the allowlist discards the executable's directory and classifies it as trusted git. The canonical PATH preflight does not protect explicit paths, so a repository-controlled or temporary binary can perform arbitrary credential access, runner mutation, or workflow replay while presenting reviewed Git arguments; require approved absolute locations or bare names resolved through the reviewed PATH.
AGENTS.md reference: AGENTS.md:L8-L9
https://github.com/1XP-AI/gh-runnerd/blob/a55fb9d1c9402bc65c0f40daa6673d6f447700f4/docs/evidence/g01-recovery-packet.md#L7604-L7605
After the required preflight exports GIT_CONFIG_COUNT and GIT_CONFIG_KEY_*, assignment-only lines can replace them with GIT_CONFIG_COUNT=1, GIT_CONFIG_KEY_0=diff.external, and GIT_CONFIG_VALUE_0=/tmp/reviewed-hook; this branch discards those commands, and the following git diff HEAD^ HEAD is accepted. I reproduced with Git 2.43.0 that the reassigned exported configuration invokes the synthetic external-diff hook, so a certified fence can delegate to arbitrary code despite the per-command diff.external checks; retain and validate security-relevant environment state across the fence.
AGENTS.md reference: AGENTS.md:L8-L8
https://github.com/1XP-AI/gh-runnerd/blob/a55fb9d1c9402bc65c0f40daa6673d6f447700f4/docs/evidence/g01-recovery-packet.md#L8356-L8357
With Git 2.43.0, git show --output=AGENTS.md --format=oneline -s HEAD opens and overwrites the named file, but the scanner accepts it because output-path validation returns immediately for every subcommand except diff while show is globally classified as read-only. A certified executable fence can therefore corrupt any writable reviewed source or runner file before a later cleanliness check notices; reject --output for show or apply the same reviewed-path policy to all diff-producing commands.
https://github.com/1XP-AI/gh-runnerd/blob/a55fb9d1c9402bc65c0f40daa6673d6f447700f4/docs/evidence/g01-recovery-packet.md#L8845-L8847
A fence containing diff --from-file=$HOME/.netrc docs/EXECUTION.md receives no violation because every option-shaped token is skipped before path validation. The installed diff --help states that --from-file=FILE1 compares that file to all operands, so executing the certified command reads the maintainer's private file and can copy its contents into captured output; parse filename-bearing options such as --from-file and --to-file and apply the same reviewed-path restriction to their values.
AGENTS.md reference: AGENTS.md:L9-L9
ℹ️ 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".
| if python_dotted_name(node.value) == "os.environ": | ||
| key = node.slice.value if isinstance(node.slice, ast.Constant) else None | ||
| return key is None or not isinstance(key, str) or credential_environment_name(key) |
There was a problem hiding this comment.
Fresh evidence beyond the Path.home()/expanduser() cases is home = os.environ["HOME"]; print(home): the scanner accepts it because this branch marks only credential-shaped environment keys as sensitive, and assigning the value first avoids the later descendant check for os.environ. When HOME is present, executing the certified heredoc emits the maintainer's personal machine path; taint path-valued environment keys such as HOME, or conservatively taint all inherited environment values.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| issue79_reviewed_evidence_paths = ( | ||
| "docs/evidence/g01-recovery-packet.md", | ||
| "scripts/evidence_packet/issue79_regression_test.py", | ||
| ) |
There was a problem hiding this comment.
Fresh evidence after the earlier two-file intent-bit fix is that this commit also adds docs/decisions/0004-offline-python-ast-regression-tooling.md, but this tuple omits it. I verified that marking the ADR assume-unchanged and editing it leaves git status --porcelain=v1 --untracked-files=all empty (the installed git update-index -h describes the flag as marking files “not changing”), while the verifier compares bytes only for these two listed paths, so modified invocation or trust policy can be paired with the reviewed remote HEAD; include every changed evidence source or reject intent bits across the tracked tree.
AGENTS.md reference: AGENTS.md:L16-L16
Useful? React with 👍 / 👎.
| if _safe_assignment_expression(value, namespace): | ||
| safe_statement = statement |
There was a problem hiding this comment.
The loader accepts probe = os.system("gh workflow run ci.yml") as a validated statement, then silently skips it because the value is not considered safe. The regression harness can consequently test the remaining scanner definitions without executing or rejecting this statement, while running the actual packet heredoc evaluates the assignment before the scanner can inspect itself and launches the command; fail closed on every unsupported top-level expression instead of omitting it from the harness namespace.
AGENTS.md reference: AGENTS.md:L8-L9
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3c335bd40
ℹ️ 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".
| continue | ||
| elif isinstance(statement, (ast.FunctionDef, ast.AsyncFunctionDef)): | ||
| exec(compile(ast.Module(body=[statement], type_ignores=[]), "<packet-scanner-function>", "exec"), namespace) | ||
| elif isinstance(statement, (ast.Assign, ast.AnnAssign)): |
There was a problem hiding this comment.
Fresh evidence after the unsupported-assignment correction is a bare top-level print(subprocess.os.environ) added to the scanner fence: _scanner_namespace() accepts and silently skips this statement, while inspect_python_heredoc also reports no violation for it; executing the actual packet heredoc therefore dumps the inherited environment before scanning begins. Add a fail-closed branch for every unsupported top-level statement kind rather than allowing non-assignment expressions to fall through.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| if not dotted or not dotted.startswith("os."): | ||
| continue |
There was a problem hiding this comment.
Under the CPython environment used by this packet, an executable heredoc containing import subprocess; subprocess.os.remove("/tmp/maintainer-owned") receives no scanner violation and deletes the referenced file. The fail-closed OS check only recognizes dotted names beginning with os., while the filesystem mutator check recognizes remove only as the exact name os.remove, so the os module re-exported through the allowlisted subprocess module bypasses both protections; resolve such module aliases before classifying calls.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
| assignments = [ | ||
| node for node in ast.walk(tree) | ||
| if isinstance(node, (ast.Assign, ast.AnnAssign, ast.AugAssign, ast.NamedExpr)) | ||
| ] |
There was a problem hiding this comment.
An executable heredoc defining def deco(f): return subprocess.run, decorating launch with @deco, and then calling launch(["gh", "workflow", "run", "ci.yml"]) receives no scanner violation, because launcher binding considers assignments but not the runtime replacement performed by decorators. I checked the installed gh workflow run --help, which states that it creates a workflow_dispatch event and triggers the workflow, so reject command-capable decorator returns or resolve the decorated binding before approving calls.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f3f8c8b54
ℹ️ 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".
| targets = ( | ||
| "print", | ||
| "builtins.print", | ||
| "sys.exit", | ||
| "warnings.warn", |
There was a problem hiding this comment.
Fresh evidence beyond the direct/imported sink-alias fix is import os, sys; emit = sys.stdout.write; emit(str(os.environ)): inspect_python_heredoc returns no violation, although execution writes the inherited environment. The new alias target list covers only fixed dotted functions, while .write is recognized only when it remains an attribute call, so assigning any recognized method sink to a name launders credentials; resolve aliases of python_sensitive_sink_methods as well or reject unresolved callable aliases.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| isinstance(value, ast.Attribute) | ||
| and value.attr in python_filesystem_mutating_methods | ||
| ): | ||
| receiver = value.value | ||
| method = value.attr |
There was a problem hiding this comment.
In an executable Python heredoc, import os; actions = {"delete": os.remove}; actions["delete"]("/tmp/maintainer-owned") receives no scanner violation, although executing it deletes the referenced file. This new alias check recognizes a mutator only when the entire assignment value is an attribute/getattr expression, so placing the same callable in a dict, list, attribute, or subscript erases its provenance before the computed call; propagate filesystem-mutator bindings through these containers or reject unresolved computed call targets.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31c2e6018b
ℹ️ 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".
| literal_dicts = { | ||
| target.id: node.value |
There was a problem hiding this comment.
Fresh evidence beyond the subprocess re-export correction is import os; alias = os; alias.remove("/tmp/maintainer-owned"): the scanner returns no violation, although executing it deletes the file. This guard tracks aliases that may refer to subprocess but not assignments of the os module itself, so filesystem-mutator and unknown-OS checks no longer recognize the call; propagate direct OS-module aliases before classifying calls.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
| for value in python_join_assignment_index(tree).get(node.id, ()) | ||
| ) | ||
| return False | ||
| if isinstance(node, ast.Attribute) and python_dotted_name(node) == "os.environ": |
There was a problem hiding this comment.
In an executable Python heredoc, from os import environ as inherited; print(inherited) receives no scanner violation and prints the entire inherited environment, including credentials. This exact-name branch recognizes only os.environ, while the alias helper handles assignments rather than ImportFrom bindings; resolve imported environment aliases before output-sink certification.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| if "${!" in token: | ||
| return "indirect shell parameter expansion is not allowed" |
There was a problem hiding this comment.
With Bash 5.2.21, an executable fence containing printf -v payload '%s%s' '$' 'GH_TOKEN'; printf '%s\n' "${payload@P}" receives no scanner violation but prints the inherited token. The first command constructs $GH_TOKEN without exposing a sensitive expansion to the scanner, and Bash's @P transformation then evaluates it as prompt text; reject this transformation or conservatively track values assigned through printf -v.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| elif not shell_reviewed_reader_path(token): | ||
| return "reader path is not reviewed or packet-owned" |
There was a problem hiding this comment.
When an ignored credential file such as maintainer.pem exists at the checkout root, awk '{print}' maintainer.pem receives no scanner violation and copies the private key into packet output. The rewritten operand loop trusts shell_reviewed_reader_path, whose fallback accepts every non-dot relative name, so the repository's *.pem ignore rule also keeps this file out of the later status check; require a tracked/reviewed path or an explicitly approved prefix instead.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| exception_values = ( | ||
| list(node.exc.args) + [keyword.value for keyword in node.exc.keywords] | ||
| if isinstance(node.exc, ast.Call) else [node.exc] |
There was a problem hiding this comment.
An executable heredoc containing import os; raise RuntimeError(os.environ) receives no scanner violation, while the unhandled exception renders the complete environment to stderr. The new exception_values extraction applies only the local-path check, and the credential check tests the outer RuntimeError(...) call without traversing its arguments; apply sensitive-value analysis to these extracted arguments as well. 【F:AGENTS.md†L9-L9��
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b902f7df1d
ℹ️ 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".
| if isinstance(node, ast.Attribute) and python_dotted_name(node) == "os.environ": | ||
| return True | ||
| if isinstance(node, ast.Subscript): | ||
| if not ( | ||
| isinstance(node.value, ast.Name) | ||
| and node.value.id in reviewed_python_compile_slice_bases | ||
| and node.value.id in source_provenance | ||
| and isinstance(node.slice, ast.Slice) | ||
| if python_dotted_name(node.value) == "os.environ": | ||
| key = node.slice.value if isinstance(node.slice, ast.Constant) else None | ||
| return key is None or not isinstance(key, str) or key == "HOME" or credential_environment_name(key) |
There was a problem hiding this comment.
Fresh evidence beyond the from-import and assigned-module fixes is import os; print(vars(os)["environ"]): inspect_python_heredoc returns no violation, although the expression retrieves and prints the inherited environment mapping, including credentials. This branch recognizes only the exact os.environ attribute spelling, so module-dictionary access bypasses the new taint propagation; resolve vars(os)/os.__dict__ accesses or fail closed.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| if isinstance(node, ast.Import) and any( | ||
| alias.name in {"os", "shutil"} and alias.asname is not None | ||
| for alias in node.names | ||
| ): | ||
| return "Python heredoc aliases a filesystem-capable module" |
There was a problem hiding this comment.
Fresh evidence beyond the assigned os-module correction is import shutil; alias = shutil; alias.rmtree("/tmp/maintainer-owned"): the scanner returns no violation, even though the call recursively deletes the target. The import guard rejects only import shutil as alias, while the mutator classifier recognizes only the canonical shutil.rmtree dotted name, so assignment aliases still bypass the destructive-operation boundary; propagate module aliases or reject them.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
| elif executable == "tr" or not expression_consumed: | ||
| expression_consumed = True | ||
| elif not shell_reviewed_reader_path(token): | ||
| return "reader path is not reviewed or packet-owned" |
There was a problem hiding this comment.
Fresh evidence after the reader-operand correction is awk 'BEGIN { ARGV[1]="maintainer.pem" } {print}' docs/EXECUTION.md: the scanner accepts the reviewed visible operand, but GNU Awk 5.2.1 executes the program against maintainer.pem instead; I reproduced the private-file output in a temporary fixture, and awk --help identifies the invocation form as awk ... 'program' file .... Because the program token can rewrite ARGV, checking only subsequent shell operands still lets an ignored credential file enter packet output; reject ARGV mutation or parse the effective inputs conservatively.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a31f9942d
ℹ️ 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".
| @@ -12009,6 +15842,35 @@ def python_sensitive_read_violation(tree, parents): | |||
| "Python credential/environment value is sent to an exception " | |||
| f"on line {node.lineno}" | |||
| ) | |||
| exception_values = [] | |||
There was a problem hiding this comment.
Fresh evidence beyond the raise RuntimeError(...) case is import os; assert False, os.environ: inspect_python_heredoc returns None, while the unhandled assertion renders the complete inherited environment to stderr. The exception-value pass only examines ast.Raise, so include ast.Assert.msg in sensitive-value checks before certifying executable heredocs.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| "replace", | ||
| }: | ||
| path_arguments.extend(node.args[:1]) | ||
| elif dotted in python_filesystem_mutating_functions: |
There was a problem hiding this comment.
On CPython 3.13, import shutil; shutil._rmtree_unsafe("/tmp/maintainer-owned", None, lambda *args: None) receives no scanner violation and recursively deletes the target. The classifier treats only explicitly enumerated public names as mutations even though the entire shutil module is importable, so unreviewed private mutators bypass the live-runner safety boundary; reject unknown shutil calls rather than assuming unlisted entry points are harmless.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
| import_launcher_violation = python_import_launcher_violation(node) | ||
| if import_launcher_violation: | ||
| return f"{import_launcher_violation} on line {node.lineno}" | ||
| dotted = python_dotted_name(node.func) |
There was a problem hiding this comment.
An executable heredoc containing from os import getppid, kill; kill(getppid(), 9) receives no scanner violation, although executing it kills the packet process's invoking parent. The signal check compares only the call's literal dotted spelling, so both from-import and assigned aliases of os.kill, os.killpg, and the signal mutators bypass the owned-process check; resolve these aliases before deciding that a call is not signal-capable.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
| """Reject jq filters that read inherited environment values.""" | ||
| if not tokens or executable_basename(tokens[0]).casefold() != "jq": | ||
| return None | ||
| for token in tokens[1:]: |
There was a problem hiding this comment.
An executable fence containing jq -n -L/tmp 'include "evil"; leak' receives no scanner violation; with /tmp/evil.jq defining def leak: env;, jq 1.7 prints inherited credentials. The installed jq --help describes -L directory as searching modules from that directory, but this check examines only the visible filter text, so environment access hidden in a loaded module bypasses it; reject -L and external module imports or inspect the loaded source.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
|
@codex review |
|
@codex review |
|
@codex review |
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2390a54e25
ℹ️ 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".
| if isinstance(target, ast.Name) and isinstance(value, ast.Name): | ||
| if value.id in names and target.id not in names: | ||
| names.add(target.id) | ||
| return True | ||
| if isinstance(target, (ast.Tuple, ast.List)) and isinstance(value, (ast.Tuple, ast.List)): |
There was a problem hiding this comment.
Fresh evidence beyond the direct assigned-module and stored-callable fixes is import os; mods = [os]; print(mods[0].environ): the exact-head scanner returns no violation, although execution dumps the inherited environment; replacing the final line with mods[0].system(...) likewise bypasses command classification. This binder only follows name-to-name assignments and positional literal destructuring, so modules recovered from lists, dictionaries, helpers, or iterator operations lose their security identity; propagate or reject those derived module values before certifying the heredoc.
AGENTS.md reference: AGENTS.md:L8-L9
Useful? React with 👍 / 👎.
|
|
||
| def temporary_path_component_safe(val |
There was a problem hiding this comment.
An executable heredoc containing import os, warnings; warnings.showwarning(os.environ, UserWarning, "x", 1) receives no scanner violation, while CPython writes the complete environment mapping to stderr. The allowlist recognizes warnings.warn and warn_explicit but omits the directly callable showwarning sink, allowing credentials to enter packet output; classify it and its aliases as sensitive sinks.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| changed = True | ||
| return bindings | ||
|
|
||
|
|
||
| def temporary_path_component_safe(val |
There was a problem hiding this comment.
Fresh evidence beyond the Path.cwd() and .resolve() cases is from pathlib import Path; print(Path(".").absolute()): the exact-head scanner returns no violation, while execution emits the invocation worktree's absolute path. This path analysis treats every .resolve() call as sensitive but omits the equivalent current-directory resolution performed by .absolute(), so a certified heredoc can place a maintainer's personal checkout path into packet output; classify .absolute() and its aliases as resolved local-path sources.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| if isinstance(node, ast.Attribute) and python_dotted_name(node) == "os.environ": | ||
| return True |
There was a problem hiding this comment.
On the POSIX/macOS environment targeted by this packet, import os; print(os.environb) receives no scanner violation and emits every inherited environment key and value as bytes, including credentials; from os import environb; print(environb) and an imported getenvb alias are accepted as well. The taint source check recognizes only the text os.environ spelling, so include environb and its from-import accessors in the inherited-environment boundary.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
1 participant
Goal and scope
Refs #79. Harden the G01 evidence packet and offline regression harness against reviewed credential, filesystem, command-delegation, and reader gaps. Current head: 2390a54. The earlier ADR 0004 delta remains in this PR; no production Go or live runner boundary changed. G01/G02 live evidence gates remain open.
TDD and offline verification
The packet ledger records immutable review URLs, inert RED witnesses, GREEN corrections, safe controls, and rollback. Exact-head Codex review 5343868697 on 9a31f99 had four P1 inline findings, all corrected. Four independent GPT-6-Luna/max follow-ups found thirteen adjacent P1 bypasses on subsequent heads; all were RED-reproduced and corrected. The clean full rerun passed 116 tests in 160.740s. A separate post-ledger packet scan passed in 72.672s: 331 shell commands, 95 Python heredocs, zero violations. Diff whitespace and added-line credential/private-path scans passed. Unsafe specimens were never executed.
Premerge gates
Hosted Go quick check passed on this exact head: https://github.com/1XP-AI/gh-runnerd/actions/runs/36481731698/job/109128752028. Fresh @codex review was requested at issue comment 5878235554 and acknowledged by the bot, but remains pending. Do not merge until exact-head review completes and all blocking findings are triaged. No trusted/live test, workflow dispatch/replay, runner/SDK/Scale Set/App operation, real-credential operation, or browser operation was performed. Rollback is a reviewed reversal of the #79 packet, harness, and ADR changes while preserving unrelated work and manually installed runners.