justinmclean commented on PR #543:
URL: https://github.com/apache/airflow-steward/pull/543#issuecomment-4763644084
## Correctness
[advisory] tools/skill-evals/src/skill_evals/runner.py:707 — negate is read
with a truthiness check, not a strict boolean.
if spec.get("negate") and holds is not None:
Rule: a stray non-bool value (e.g. "negate": "false") would silently invert.
load_assertions validates type but not negate. Low risk given fixtures are the
only source, but a bool guard in load_assertions would fail typos loudly,
consistent with how unknown predicate types are already rejected.
[advisory] step-3-staging fixtures — the dist/release/ denylist check is
vacuous. staging_url_not_dist_release and proposed_true lack a has_/mention_
prefix, so they route to the field-aware comparator and are
intersection-skipped (the model output has no field of those names). The
injection case therefore passes without actually verifying the security
property. Rename to has_-prefixed keys (now that negate works) to exercise it.
## Security
No findings in the changed code. The skill is read-only by design (golden
rules 1 to 5: no local execution, no key handling, SHA-512/256 only,
proposal-gated writes, dist/release/ denylist), and the runner change is
grading-only. The negate refactor preserves fail-loud behaviour: a spec error
returns None and is passed through un-inverted (covered by
test_assert_negate_passes_spec_error_through), and a real --passphrase leak
still evaluates to False (covered by test_assert_negate_inverts_regex_result).
## Conventions
No blocking findings. SPDX headers present on new .md files; JSON fixtures
are comment-incapable and correctly exempt. The negate feature is documented in
both the module docstring and the eval README.
--
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]
