justinmclean commented on PR #454:
URL: https://github.com/apache/airflow-steward/pull/454#issuecomment-4629200540
The design is sound and well-scoped: conservative thresholds,
maintainer-decides framing, propose-then-confirm on every write, and an
explicit [R]eview anyway escape hatch. Cross-links and anchors resolve, the
license header matches convention, and the threshold logic is consistent across
all three files.
Issues worth fixing/looking into:
- H1 relies on files[].changeType, which the Step 2 fetch doesn't provide.
Step 2's gh pr view --json files returns path / additions / deletions only;
changeType is a GraphQL field that gh's files JSON does not expose. So "infers
new-directory status from files[].changeType (no base-ref tree lookup)" is
likely undetectable as written. The fix is easy and already in-hand: the
unified diff is cached in Step 2, and added files show new file mode / ---
/dev/null. Point H1 at the diff, not changeType.
- slop-detection.md and review-flow.md contradict each other on the
note-only output. slop-detection.md says "add [suspicious] chip to headline"
(twice, in the threshold table). But review-flow.md Step 2.5 explicitly says
"do not attempt to modify the already-displayed Step 1 headline" and emit a
separate ⚠ [suspicious] — H5, S1, ... line. Since the flow is sequential and
the Step 1 headline is already printed before Step 2.5 runs, review-flow.md is
right and the slop-detection.md "chip to headline" language is stale.
- S1 ("ticket-style title") needs the title, which isn't a Step 2 field. The
--json list in Step 2 has no title. It's available from the Step 1 /
working-list cache, so it's fine in practice, but the doc's claim that "all
signals are evaluated from the Step 2 payload" is inaccurate for S1.
- H3 + H4 are not independent, which weakens the "2 hard = nearly
conclusive" claim. Both fire from the same underlying fact: a team developed on
a shared fork and merged internal PRs before sending one upstream. A legitimate
org that works that way would trip H3+H4 together and hit early-exit. Consider
treating H3+H4 as a single signal, or requiring one of them to pair with a
different hard signal.
- Scope vs Golden rule 9. The review skill currently produces only approve /
request-changes / comment reviews; Golden rule 9 declares triage actions
(close, etc.) out of scope. The [X] action adds close + lock to this skill.
That may be the right call, but it's a real scope expansion that warrants a
deliberate decision rather than a side effect of sloppy handling.
- lock_reason=spam mislabels good-faith newcomers. A student class project
pushed upstream is misdirected, not spam, and your own contributor stance leans
against being paternalistic toward newcomers. Classifying it as spam (and
steering the maintainer to GitHub's "Spam or misleading" report) is harsh for a
first-time contributor.
- The [C] warning comment skips the AI-attribution footer. Golden rule 5
requires the footer on review bodies; this comment goes out via gh pr comment,
so it's technically outside that rule, but a bot-posted public comment with no
attribution is inconsistent with the rest of the skill's transparency posture.
Add the footer.
- No eval/fixture coverage. This adds real branching logic to a skill, and
the repo dogfoods skill evals, yet there are no cases for it. Worth at least
three: a clear-slop PR (early exit), a 1-hard-2-soft PR (note-only), and a
large legitimate refactor (silent / no false positive). I can help with these
if you want.
- Placeholder drift. The lock call uses repos/<owner>/<repo>/... while every
other recipe uses --repo <repo> (owner/name form). <owner> is never defined in
the placeholder block. Either define it or use gh api
repos/<repo>/issues/<N>/lock.
--
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]
