shahar1 commented on code in PR #70437:
URL: https://github.com/apache/airflow/pull/70437#discussion_r3671578326
##########
providers/standard/src/airflow/providers/standard/decorators/bash.py:
##########
@@ -88,7 +88,6 @@ def execute(self, context: Context) -> Any:
if not isinstance(self.bash_command, str) or self.bash_command.strip()
== "":
raise TypeError("The returned value from the TaskFlow callable
must be a non-empty string.")
- self._is_inline_cmd =
self._is_inline_command(bash_command=self.bash_command)
self.render_template_fields(context)
Review Comment:
**Blocking — `@task.bash` returning a `.sh` filename now runs inline**
The deleted classification line above this call needs to be restored (also
called out in the earlier thread comment): at parse time the decorator's
`bash_command` is the non-str `SET_DURING_EXECUTION` sentinel, so no parse-time
hook can ever classify it — the deleted line was the only point where the raw
filename returned by the callable was still observable. Without it,
`self.render_template_fields(context)` substitutes the `.sh` file's rendered
contents, and `super().execute()` then classifies script *content* as inline —
same failure mode as in `operators/bash.py` (E2BIG on large scripts, `$0`
change).
*Drafted by Claude Code (Fable 5) using Apache Magpie; reviewed by shahar1
before posting.*
##########
providers/standard/tests/unit/standard/operators/test_bash.py:
##########
@@ -307,3 +307,17 @@ def test_templated_bash_script(self, dag_maker,
create_task_instance_of_operator
task = ti.render_templates(context=context)
result = task.execute(context=context)
assert result == "test_templated_fields_task"
+
+ @pytest.mark.db_test
+ def test_script_file_classification_uses_rendered_value(self,
create_task_instance_of_operator):
+ """A templated bash_command that renders to a .sh path must run as a
script file, not inline."""
+ ti = create_task_instance_of_operator(
+ BashOperator,
+ dag_id="test_bash_script_classification",
+ task_id="bash_script_test",
+ bash_command="{{ params.script_name }}",
+ params={"script_name": "sample.sh"},
+ )
+ task = ti.render_templates()
+ assert task.bash_command == "sample.sh"
+ assert BashOperator._is_inline_command(bash_command=task.bash_command)
is False
Review Comment:
**Test does not fail without the fix**
`BashOperator._is_inline_command(bash_command="sample.sh")` returns `False`
on `main` as well (verified empirically), and the render path this test
exercises is untouched by the diff — so this test passes with and without the
change, and also would not catch the regressions flagged above. Per the testing
standard in AGENTS.md: *"every test must fail without the PR's change."*
Assert on the *strategy the operator actually picks* instead — e.g. mock
`_run_inline_command` / `_run_rendered_script_file` and assert which one runs
(PR #70369 has a test in exactly that shape, worth borrowing).
*Drafted by Claude Code (Fable 5) using Apache Magpie; reviewed by shahar1
before posting.*
##########
providers/standard/src/airflow/providers/standard/operators/bash.py:
##########
@@ -215,7 +213,7 @@ def execute(self, context: Context):
raise AirflowException(f"The cwd {self.cwd} must be a
directory")
env = self.get_env(context)
- if self._is_inline_cmd:
+ if self._is_inline_command(self.bash_command):
Review Comment:
**Blocking — script-file commands now run inline**
By the time `execute()` runs, a literal `bash_command="script.sh"` no longer
holds the path — the Task SDK has already substituted the file's rendered
**contents** (parse time via `resolve_template_files()`,
[templater.py#L91](https://github.com/apache/airflow/blob/main/task-sdk/src/airflow/sdk/definitions/_internal/templater.py#L91);
render time via the `template_ext` file branch,
[templater.py#L256](https://github.com/apache/airflow/blob/main/task-sdk/src/airflow/sdk/definitions/_internal/templater.py#L256)
— and the worker always renders before execute). Script contents virtually
never end in `.sh`, so every real script-file command classifies as inline and
runs via `bash -c "<entire script>"`, defeating `_run_rendered_script_file`'s
documented purpose ("prevents 'Argument list too long' error" — Linux caps a
single argv entry at ~128 KiB) and changing `$0`. This is the same failure mode
as the comparison table earlier in this thread — this classification point is
even
*later* than the variant that table measured:
```text
this branch main
_is_inline_cmd : True False
239 KB script : OSError [Errno 7] Arg list too long OK
$0 : /usr/bin/bash tmpXXXX.sh
```
Classification has to happen **before** the contents substitution — per the
earlier comment: a `resolve_template_files()` override that records
`_is_inline_cmd` from the raw value before calling `super()`, unguarded (the
`Any` widening you added is exactly what makes that safe), with an `is None`
fallback in `execute()` for values that only materialize at runtime.
*Drafted by Claude Code (Fable 5) using Apache Magpie; reviewed by shahar1
before posting.*
##########
providers/standard/src/airflow/providers/standard/operators/bash.py:
##########
@@ -179,8 +179,6 @@ def __init__(
self.append_env = append_env
self.output_processor = output_processor
self._is_inline_cmd = None
Review Comment:
**`_is_inline_cmd` is now dead code**
This attribute is assigned `None` here and never read anywhere on this
branch — `execute()` calls the classmethod directly, and the decorator's write
was removed. Either wire it into the pre-substitution classification described
above (which uses it), or remove it. Knock-on: `decorators/test_bash.py:107`
asserts this vestigial attribute; under the pre-substitution fix it becomes
`False` and needs the one-line update mentioned in the earlier thread comment.
*Drafted by Claude Code (Fable 5) using Apache Magpie; reviewed by shahar1
before posting.*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
