justinmclean commented on PR #250:
URL: https://github.com/apache/airflow-steward/pull/250#issuecomment-4538691406
Pre-flight self-review — PR #250
https://github.com/apache/airflow-steward/pull/250 · draft · author:
justinmclean
Base: main
Files changed: 24 (≈21 added, 3 modified)
Diff size: 2032 additions, 3 deletions
The change is ~1700 lines of markdown specs/docs/prompts plus one
executable
file, tools/spec-loop/loop.sh (the bash runner). All substantive findings
are
in loop.sh; the docs are prose.
---
Correctness
- [blocking] tools/spec-loop/loop.sh:203,223-240 — update mode is handed a
note that contradicts its own job. BUILD_ITERATION is set true for both
build
and update; when launched from a branch ≠ BASE, the runner appends a
"Tooling
source" note ending "do NOT edit specs there — the control branch owns the
specs." But PROMPT_update.md's entire purpose is "Edit specs only."
▎ 239: echo "Implement the product change on the work branch; do NOT edit
specs"
▎ Rule: a runner-injected instruction must not contradict the active
beat's
prompt — ./loop.sh update from any feature branch will likely refuse the
spec
edits that are the beat's whole point. Gate the note on build-only (e.g.
MODE
= build), not on BUILD_ITERATION.
- [advisory] tools/spec-loop/loop.sh:90-94,115,166 — a non-numeric
iteration
count silently becomes "unlimited". ./loop.sh plan foo sets
MAX_ITERATIONS="foo"; with no set -e, [ "foo" -gt 0 ] errors to stderr and
is
treated as false, so the loop runs unbounded instead of rejecting the typo.
▎ 90: MODE="plan"; ... MAX_ITERATIONS="${2:-0}"
▎ Rule: validate the plan/update/consolidate count argument is numeric
(build's $1 is already regex-checked).
- [advisory] tools/spec-loop/loop.sh:126-128 — spinner prints mojibake
under a
C/POSIX locale. The braille frames are multibyte UTF-8, but
${frames:$i:1} /
${#frames} are byte-indexed unless the locale is UTF-8 — and the loop is
meant
to run inside the clean-env sandbox, where LANG/LC_* are typically unset.
▎ 126: ... frames='⠋⠙⠹⠸⠼⠴⠦⠧⠇⠏' i=0
▎ Rule: cosmetic, but set LC_ALL=C.UTF-8 for the spinner or use ASCII
frames.
- [advisory] tools/spec-loop/loop.sh:248 — the running script deletes
itself
mid-execution. When launched from a feature branch building on main, git
checkout "$BASE" removes tools/spec-loop/loop.sh (not on main) while bash
is
executing it. It survives on Linux/macOS via unlink-keeps-inode semantics,
so
it works on the target platforms — but it's fragile and undocumented.
▎ 248: if ! checkout_out="$(git checkout "$BASE" 2>&1)"; then
▎ Rule: add a comment noting the dependency, or run the agent from a copy
outside the worktree.
Security
- [advisory] tools/spec-loop/loop.sh:269 — the defense-in-depth deny likely
doesn't match. The wildcard uses a space (Bash(git push *) / Bash(gh *)),
but
Claude Code's Bash permission syntax is colon-based (Bash(git push:*) /
Bash(gh:*)), so these rules won't match git push origin … or gh pr create —
defeating the stated "hard-deny … even with permissions skipped."
▎ 269: --disallowedTools "Bash(git push *)" "Bash(gh *)" \
▎ Rule: advisory, not an active vuln — the real protection is the OS
sandbox +
no credentials (as the header says). But fix the syntax to Bash(git
push:*) /
Bash(gh:*) so the secondary layer isn't false assurance.
Conventions
No findings. SPDX/ASF headers are present on every new file (verified below
the doctoc TOCs); loop.sh and the prompts are correctly excluded from
doctoc
in .pre-commit-config.yaml; placeholder usage is spec-loop tooling, not
skill-directive, so the placeholder convention doesn't apply.
---
Summary
Blocking findings present — fix the update-mode injected-note contradiction
before marking the draft ready; the rest are advisory hardening/robustness
items.
Blocking: 1 Advisory: 4
--
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]
