feat: email the operator when Igor requests his review #451

Merged

joshtronic merged 4 commits from feat/439-email-on-review-request into master 2026-07-29 00:31:25 +00:00

Conversation 6 Commits 4 Files changed 7 +618 -1

igor commented 2026-07-28 23:06:34 +00:00

Collaborator

Copy link

Part of #439 — deliberately does NOT close it (see below).

Forgejo's own notifications are off — it mails on everything the bot does, and the volume buried the useful mail. So there's currently no signal when a PR lands on your desk. This sends one, deliberately, at the moment that means something: when Igor requests your review.

Hooked at forgejo_request_review, not at its callers. Ten call sites — terminal verdicts, unvalidated repo, escalation after N rework rounds, rework that produced no commits — all mean the same thing, and hooking callers means remembering to hook the eleventh.

Deduped on repo#number + head sha. forgejo_request_review is idempotent by design, so callers fire it freely; mailing on every call would rebuild the flood you turned Forgejo off to escape. Same head → quiet. New head → code you haven't seen, second mail. Recorded only after a send lands, so a failed send retries instead of marking you notified about mail you never got.

Scope — why this is Part of, not Closes

#439 specifies the whole needs-you set: PRs, issues gaining Status/Blocked/Need More Info, CEO proposals awaiting a greenlight, failed systemd units, wait times, a weekly still-waiting summary. It was marked Status/Blocked for scope (792 lines, over the per-issue cap) before any of this landed.

This PR implements one source of that set — "Igor requested your review on a PR" — which is the slice you asked for directly. #449 (merged) built the detection half and only logs. Merging this with a Closes keyword would auto-close #439 and silently take the unbuilt remainder with it. Hence Part of.

Two rounds of shadow review found four real defects in the original commit

1. The hook was per-process, not per-call-site. site-work-block.sh and ideation-pipeline.sh request your review from child processes that never sourced the notifier — they'd have landed the request and mailed nothing. Both now source it, and check-sync.sh walks each entry point's transitive source set and fails if one can reach forgejo_request_review/forgejo_open_pr without the notifier, so the design claim enforces itself instead of relying on memory.
2. Fail-open had no cap. A persistently failing PR fetch recorded nothing, so ~10 call sites × every tick = an email per call. Now keys on an hour bucket: a stuck fetch degrades to one an hour, a recovered one still sends on an unseen sha.
3. The PR title — the one string on this path Igor didn't write — is now escaped before entering the HTML part.
4. mkdir -p the state dir (a fresh host would have run with dedup permanently off behind one warning), and mktemp beside the state file so the mv is a rename, not a cross-mount copy.

Tests

bin/test-reviewnotify.sh, picked up automatically by check-sync.sh. Mutation-checked against hook-unwired, dedup-disabled, and record-before-send — each caught. The wiring block drives the real forgejo_request_review with only its documented HTTP seam stubbed, because every other assertion would pass just as happily if the hook were never wired.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QfDyMfn2Go73gw21a92wNB

**Part of #439** — deliberately does NOT close it (see below). Forgejo's own notifications are off — it mails on everything the bot does, and the volume buried the useful mail. So there's currently no signal when a PR lands on your desk. This sends one, deliberately, at the moment that means something: when Igor requests your review. **Hooked at `forgejo_request_review`, not at its callers.** Ten call sites — terminal verdicts, unvalidated repo, escalation after N rework rounds, rework that produced no commits — all mean the same thing, and hooking callers means remembering to hook the eleventh. **Deduped on `repo#number` + head sha.** `forgejo_request_review` is idempotent by design, so callers fire it freely; mailing on every call would rebuild the flood you turned Forgejo off to escape. Same head → quiet. New head → code you haven't seen, second mail. Recorded only after a send lands, so a failed send retries instead of marking you notified about mail you never got. ### Scope — why this is `Part of`, not `Closes` #439 specifies the whole needs-you set: PRs, issues gaining `Status/Blocked`/`Need More Info`, CEO proposals awaiting a greenlight, failed systemd units, wait times, a weekly still-waiting summary. It was marked `Status/Blocked` for scope (792 lines, over the per-issue cap) before any of this landed. This PR implements one source of that set — "Igor requested your review on a PR" — which is the slice you asked for directly. #449 (merged) built the detection half and only logs. Merging this with a `Closes` keyword would auto-close #439 and silently take the unbuilt remainder with it. Hence `Part of`. ### Two rounds of shadow review found four real defects in the original commit 1. **The hook was per-process, not per-call-site.** `site-work-block.sh` and `ideation-pipeline.sh` request your review from child processes that never sourced the notifier — they'd have landed the request and mailed nothing. Both now source it, and `check-sync.sh` walks each entry point's transitive source set and fails if one can reach `forgejo_request_review`/`forgejo_open_pr` without the notifier, so the design claim enforces itself instead of relying on memory. 2. **Fail-open had no cap.** A persistently failing PR fetch recorded nothing, so ~10 call sites × every tick = an email per call. Now keys on an hour bucket: a stuck fetch degrades to one an hour, a recovered one still sends on an unseen sha. 3. The PR title — the one string on this path Igor didn't write — is now escaped before entering the HTML part. 4. `mkdir -p` the state dir (a fresh host would have run with dedup permanently off behind one warning), and `mktemp` beside the state file so the `mv` is a rename, not a cross-mount copy. ### Tests `bin/test-reviewnotify.sh`, picked up automatically by `check-sync.sh`. Mutation-checked against hook-unwired, dedup-disabled, and record-before-send — each caught. The wiring block drives the real `forgejo_request_review` with only its documented HTTP seam stubbed, because every other assertion would pass just as happily if the hook were never wired. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01QfDyMfn2Go73gw21a92wNB

igor added 1 commit 2026-07-28 23:06:34 +00:00

feat: email the operator when Igor requests his review ... 96ccaab630

Closes #439

Forgejo's own notifications are off, because it mails on everything the
bot does and the volume buried the useful mail. That leaves no signal
when a PR lands on the operator's desk. This sends one, deliberately, at
the moment that actually means something: when Igor requests his review.

Hooked at forgejo_request_review, not at any of its ten callers. Terminal
verdicts, an unvalidated repo, escalation after N rework rounds, a rework
that produced no commits -- they all mean the same thing, and hooking
callers means remembering to hook the eleventh. lib/forgejo.sh stays a
pure API wrapper: it fires the notifier by name only if one is defined,
best-effort, so a notification failure can't turn a landed request into a
failed one, and bin/agent-*.sh keep working without it.

Deduped on repo#number + head sha. forgejo_request_review is idempotent
by design, so callers fire it freely -- mailing on every call would
rebuild exactly the flood that got Forgejo's notifications turned off.
Same head, already mailed, stays quiet; a head that moved is code the
operator hasn't seen and earns a second mail. The record is written only
after a send lands, so a failed send retries instead of marking him
notified about mail he never got.

Dedup fails OPEN, unlike cascade's starvation check: no state, corrupt
state, or a failed PR fetch all mean send. A duplicate is an annoyance; a
swallowed one leaves him unaware the loop is blocked on him, which is the
whole failure this fixes.

Tests in bin/test-reviewnotify.sh, mutation-checked against three breaks
(hook unwired, dedup disabled, record-before-send). The last block drives
the real forgejo_request_review with only its documented HTTP seam
stubbed -- every other assertion would pass just as happily if the hook
were never wired at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QfDyMfn2Go73gw21a92wNB

igor commented 2026-07-28 23:09:26 +00:00

Author

Collaborator

Copy link

🤖 Review — REQUEST_CHANGES (automated)

CI for 96ccaab6: success

Adds lib/reviewnotify.sh + a hook in forgejo_request_review's 2xx branch to email the operator once per PR-head when Igor hands a PR over. The shell is careful, the test file is genuinely adversarial (it drives the real forgejo_request_review and guards its own guards), CI is green, and the diff is ~350 lines with no drive-by refactors. Two things block.

Blocking

1. Closes #439 overclaims — this implements roughly one bullet of five.
Issue #439 defines the needs-you set as PRs plus issues gaining Status/Blocked / Status/Need More Info, CEO proposals/questions awaiting greenlight, and failed systemd units; it also requires grouping by repo and wait time in the content, and names four required tests (first entry notifies, unchanged set silent, removal silent, second distinct item notifies). This PR covers only "Igor requested your review on a PR", with no wait time, no grouping, and no removal semantics. The issue was explicitly blocked on 2026-07-28 with "Split this into smaller issues" — so merging this with Closes #439 auto-closes the tracking issue and silently drops the other four sources plus the wait-time requirement.
Fix: change the description to Part of #439 (or reference the specific split-out sub-issue this actually closes), and make sure the remaining slices exist as issues before #439 disappears.

2. The hook only fires in processes that source lib/reviewnotify.sh — and only bin/tick.sh does.
lib/forgejo.sh:289 gates on declare -F review_notify_human, and the only new . line in the diff is in bin/tick.sh. The description's central claim ("ten call sites … hooking the function means you can't forget the eleventh") holds only for call sites that execute inside the tick process. Any forgejo_request_review reached from a bin/agent-*.sh helper run as a subprocess silently no-ops — the same "forgot to hook the caller" failure mode the design is arguing against, just moved to process boundaries. The diff alone doesn't let me verify where the ten call sites live.
Fix: either state (and ideally assert) that all ten call sites are in lib/* sourced by tick.sh, or source reviewnotify.sh wherever forgejo.sh is sourced. A one-line check in bin/check-sync.sh (every file calling forgejo_request_review transitively sources reviewnotify.sh) would make the claim self-enforcing.

Non-blocking, worth addressing

• HTML injection into the email body — lib/reviewnotify.sh:118: email_send "$subject" "<pre>${body}</pre>" "$body" ... interpolates the PR title (from the Forgejo API, not necessarily authored by Igor) into HTML unescaped. Low severity for a mail client, but it's the one untrusted string on the path; escaping & < > costs nothing.
• Flood risk on the fail-open path — review_notify_human deliberately sends and records nothing when forgejo_get_pr fails. With ~10 call sites and a persistently failing fetch (bad scope, endpoint change), that's an unbounded email per call per tick — precisely the flood the dedup exists to prevent. Consider a coarse fallback key (repo#number with no sha, or a timestamp floor) so "no head" degrades to at-most-one-per-hour rather than unbounded.
• State dir not created — lib/reviewnotify.sh:104: [ -f "$f" ] || echo '{}' > "$f" fails silently if AGENT_STATE_DIR doesn't exist yet, after which every write fails and dedup is permanently off (only a warning log). mkdir -p "$(dirname "$f")" first.
• mktemp + mv is cross-filesystem — lib/reviewnotify.sh:129-131: mktemp defaults to /tmp, so the mv may be copy+unlink rather than an atomic rename onto the state file. Use mktemp in the state dir (other repo code may already do this — worth matching).
• No reviewer check — the hook fires on any successful review request regardless of who $3 is; if a non-operator reviewer is ever requested, the operator still gets "Needs you". Fine today, fragile later.

Test coverage

Coverage of what's implemented is good — dedup key, fail-open cases, record-only-after-send, retry-after-failure, config guards (with an explicit guard-the-guard), and a real-function wiring block. The subshell-swallowed-assertion fix called out in the description is visibly in the diff. Missing relative to #439: nothing for removal-is-silent or non-PR sources, which is consistent with the reduced scope but not with Closes #439.

---

_{Independent review by the harness on claude-opus-5 (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `REQUEST_CHANGES` _(automated)_ CI for `96ccaab6`: **success** Adds `lib/reviewnotify.sh` + a hook in `forgejo_request_review`'s 2xx branch to email the operator once per PR-head when Igor hands a PR over. The shell is careful, the test file is genuinely adversarial (it drives the real `forgejo_request_review` and guards its own guards), CI is green, and the diff is ~350 lines with no drive-by refactors. Two things block. ## Blocking **1. `Closes #439` overclaims — this implements roughly one bullet of five.** Issue #439 defines the needs-you set as PRs *plus* issues gaining `Status/Blocked` / `Status/Need More Info`, CEO proposals/questions awaiting greenlight, and failed systemd units; it also requires grouping by repo and **wait time** in the content, and names four required tests (first entry notifies, unchanged set silent, **removal silent**, second distinct item notifies). This PR covers only "Igor requested your review on a PR", with no wait time, no grouping, and no removal semantics. The issue was explicitly blocked on 2026-07-28 with "Split this into smaller issues" — so merging this with `Closes #439` auto-closes the tracking issue and silently drops the other four sources plus the wait-time requirement. *Fix:* change the description to `Part of #439` (or reference the specific split-out sub-issue this actually closes), and make sure the remaining slices exist as issues before #439 disappears. **2. The hook only fires in processes that source `lib/reviewnotify.sh` — and only `bin/tick.sh` does.** `lib/forgejo.sh:289` gates on `declare -F review_notify_human`, and the only new `.` line in the diff is in `bin/tick.sh`. The description's central claim ("ten call sites … hooking the function means you can't forget the eleventh") holds only for call sites that execute *inside the tick process*. Any `forgejo_request_review` reached from a `bin/agent-*.sh` helper run as a subprocess silently no-ops — the same "forgot to hook the caller" failure mode the design is arguing against, just moved to process boundaries. The diff alone doesn't let me verify where the ten call sites live. *Fix:* either state (and ideally assert) that all ten call sites are in `lib/*` sourced by `tick.sh`, or source `reviewnotify.sh` wherever `forgejo.sh` is sourced. A one-line check in `bin/check-sync.sh` (every file calling `forgejo_request_review` transitively sources `reviewnotify.sh`) would make the claim self-enforcing. ## Non-blocking, worth addressing - **HTML injection into the email body** — `lib/reviewnotify.sh:118`: `email_send "$subject" "<pre>${body}</pre>" "$body" ...` interpolates the PR title (from the Forgejo API, not necessarily authored by Igor) into HTML unescaped. Low severity for a mail client, but it's the one untrusted string on the path; escaping `& < >` costs nothing. - **Flood risk on the fail-open path** — `review_notify_human` deliberately sends and records *nothing* when `forgejo_get_pr` fails. With ~10 call sites and a persistently failing fetch (bad scope, endpoint change), that's an unbounded email per call per tick — precisely the flood the dedup exists to prevent. Consider a coarse fallback key (`repo#number` with no sha, or a timestamp floor) so "no head" degrades to at-most-one-per-hour rather than unbounded. - **State dir not created** — `lib/reviewnotify.sh:104`: `[ -f "$f" ] || echo '{}' > "$f"` fails silently if `AGENT_STATE_DIR` doesn't exist yet, after which every write fails and dedup is permanently off (only a warning log). `mkdir -p "$(dirname "$f")"` first. - **`mktemp` + `mv` is cross-filesystem** — `lib/reviewnotify.sh:129-131`: `mktemp` defaults to `/tmp`, so the `mv` may be copy+unlink rather than an atomic rename onto the state file. Use `mktemp` in the state dir (other repo code may already do this — worth matching). - **No reviewer check** — the hook fires on any successful review request regardless of who `$3` is; if a non-operator reviewer is ever requested, the operator still gets "Needs you". Fine today, fragile later. ## Test coverage Coverage of what's implemented is good — dedup key, fail-open cases, record-only-after-send, retry-after-failure, config guards (with an explicit guard-the-guard), and a real-function wiring block. The subshell-swallowed-assertion fix called out in the description is visibly in the diff. Missing relative to #439: nothing for removal-is-silent or non-PR sources, which is consistent with the reduced scope but not with `Closes #439`. --- _{Independent review by the harness on `claude-opus-5` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.} <!-- review sha=96ccaab63062db48667f7cda9af9511ce79f422a verdict=REQUEST_CHANGES ci=success -->

igor added spent time 2026-07-28 23:09:26 +00:00

1 minute 47 seconds

igor self-assigned this 2026-07-28 23:09:27 +00:00

igor referenced this pull request 2026-07-28 23:22:35 +00:00

PR #451 says Closes #439 but only covers one slice -- split it? #452

igor commented 2026-07-28 23:27:26 +00:00

Author

Collaborator

Copy link

🔧 Rework — round 1 (automated)

Addressed the review on claude-opus-5 at effort high — 2 new commit(s).

### 🔧 Rework — round 1 _(automated)_ Addressed the review on `claude-opus-5` at **effort high** — 2 new commit(s). <!-- audit:rework round=1 effort=high -->

igor removed their assignment 2026-07-28 23:27:56 +00:00

igor added spent time 2026-07-28 23:27:56 +00:00

17 minutes 9 seconds

igor referenced this pull request from a commit 2026-07-28 23:27:57 +00:00

fix: address review on the review-request notifier

igor added 2 commits 2026-07-28 23:27:57 +00:00

fix: address review on the review-request notifier ... e7a5aad687

Two fixes from the shadow review of #451.

The hook was a per-PROCESS property, not a per-call-site one. lib/forgejo.sh
fires review_notify_human only if something defined it, and only bin/tick.sh
sourced lib/reviewnotify.sh -- so site-work-block.sh and ideation-pipeline.sh,
both of which open a PR and request the operator's review as child processes,
landed the request and told him nothing. That is the same "forgot to hook the
caller" failure the design argued against, moved down a level. Both now source
it, and bin/check-sync.sh walks each entry point's transitive source set and
fails if one can reach forgejo_request_review / forgejo_open_pr without the
notifier, so the claim enforces itself.

Also from the review, on lib/reviewnotify.sh:

- The PR title is the one string on this path Igor did not write; it is now
  escaped before it goes into the HTML part.
- The fail-open path recorded nothing without a head, which capped nothing:
  ~10 call sites against a persistently broken fetch is an email per call per
  tick. It keys on an hour bucket instead, so a stuck fetch degrades to one an
  hour and a recovered one still sends on a sha nobody has seen.
- mkdir -p the state dir; without it a first run on a fresh host turns dedup
  off permanently behind a single warning line.
- mktemp next to the state file, so the mv is a rename and not copy+unlink
  across a mount.
- Only a request FOR FORGEJO_REVIEWER emails him. Fails open when either side
  is unset, like the rest of the module.

Not fixed here: the PR description still says "Closes #439" and should say
"Part of #439". The rework path does not regenerate the PR body and I have no
sanctioned way to edit it, so that one needs a human.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

fix: harden the notifier-wiring check and its own test ... c63ed41e45

Follow-ups on the previous commit, all from re-reading it:

- check-sync's source-walk ended its loop body on `[ -n "$next" ] && ...`,
  which returns 1 for every file that sources nothing -- the common case, and
  the shape errexit trips on. Made it an `if`.
- The wiring test stubbed the hook with a bare `$3`. Under `set -u` a dropped
  reviewer aborted the suite instead of failing an assertion, so the mutation
  that removes the pass-through produced no output at all rather than a named
  failure. `${3:-}` now reports it.
- The success log read "(head )" when the fetch failed and the mail went out on
  the hour bucket. It logs the key instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

igor commented 2026-07-28 23:31:26 +00:00

Author

Collaborator

Copy link

🤖 Review — REQUEST_CHANGES (automated)

CI for c63ed41e: success

Adds lib/reviewnotify.sh + a best-effort hook in forgejo_request_review so the operator gets one email per PR-per-head when Igor hands a PR over. The core module is careful, well-commented, and genuinely well tested (the "drive the real forgejo_request_review with only the HTTP seam stubbed" block is the right instinct). CI is green. Blocking on the issue-closure claim and two verification gaps, not on the design.

Blocking

• Closes #439 over-claims the work. Issue #439 defines the needs-you set as four sources: PRs awaiting the human, issues gaining Status/Blocked / Status/Need More Info, CEO proposals/questions awaiting greenlight, and failed systemd units — plus "group by repo" and "include how long it has been waiting". This PR implements exactly one source (PR review requests), no wait time, no grouping. The issue's own blocked note says to split into smaller issues, which is what this appears to be — but merging it with Closes will auto-close #439 and silently drop the other three sources and the wait-time/grouping requirements. Change the description to Part of #439 (or Refs #439) and file the follow-up issues for the remaining sources before this lands. This is the one item the human would otherwise have to catch by hand.

• Two new entry points source lib/email.sh with no runtime coverage. bin/site-work-block.sh:101 and bin/ideation-pipeline.sh:102 now source lib/email.sh and lib/reviewnotify.sh. Neither script is exercised by check-sync.sh, and both run under set -u-style strictness elsewhere in the repo. If lib/email.sh has any top-level code that touches unset config (SMTP2GO_*, recipient lists), these two entry points now abort at startup — a regression in the PR-opening path itself, which is far worse than a missed email. Please confirm lib/email.sh is side-effect-free at source time (a one-line note in the PR is enough if so).

• The delivery seam is never exercised, even indirectly. review_notify_human calls recipients_with_primary "" and email_send "$subject" "$html" "$text" "$recipients"; bin/test-reviewnotify.sh stubs both. Nothing in the diff or the tests pins those signatures to the real ones in lib/email.sh, so an argument-order mismatch ships green and produces mail that is wrong (or no mail at all) — invisible, because the whole module is best-effort and returns 0 on failure. Either add a cheap assertion against the real email_send's arity/param order, or state in the PR that you verified it against lib/email.sh (which isn't in the diff, so I can't).

Non-blocking findings

• Cross-writer clobbering of discretionary-state.json. review_notify_human does its own read-modify-write (jq … "$state_file" > "$tmp"; mv). If any other writer — tick.sh, the .review recorder — snapshots the file in memory earlier in the tick and rewrites it wholesale at the end, the .review_notified entry written mid-tick is lost, dedup never sticks, and you get the per-tick flood this feature exists to prevent. Worth confirming every writer is read-modify-write-at-write-time. No test covers the interaction.
• Diff size. ~540 added lines (190 lib + 263 test + 55 check-sync + entry points) against the ~400 budget, on an issue that was already blocked once for scope. Nothing was deleted to fit, so this isn't the bad kind of overrun, but the check-sync.sh wiring guard (55 lines, a new repo-wide invariant over every bin/*.sh) is arguably its own change.
• Description omits ~70 lines of the diff. The body explains the hook and the dedup but never mentions the check-sync.sh guard or the two entry-point source additions. The check-sync change is the one a human most needs told about: it now fails the build for any bin/*.sh that can reach forgejo_request_review/forgejo_open_pr without sourcing reviewnotify.sh. That's a new global constraint on future scripts and it isn't in the description.
• mktemp "${state_file}.XXXXXX" + mv will reset the state file's mode to 0600 on every successful record. Fine if it's single-user; a problem if anything else reads it.
• reviewnotify_dedup_head's hour bucket is a good fix for unbounded fail-open, but note that a persistently broken forgejo_get_pr still yields one mail/hour/PR with no title and no URL — i.e. an alert the operator can't act on. Consider saying "PR details unavailable" in the body for that path.

Test coverage

Good. The dedup matrix, fail-open cases, record-only-on-success, HTML escaping, reviewer mismatch, and the fresh-state-dir path are all real assertions, and the two subshell-swallowed guards being pulled in-shell is exactly the kind of self-check I'd want. The gap is everything downstream of email_send (above) and the two new entry points that source email.sh.

---

_{Independent review by the harness on claude-opus-5 (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `REQUEST_CHANGES` _(automated)_ CI for `c63ed41e`: **success** Adds `lib/reviewnotify.sh` + a best-effort hook in `forgejo_request_review` so the operator gets one email per PR-per-head when Igor hands a PR over. The core module is careful, well-commented, and genuinely well tested (the "drive the real `forgejo_request_review` with only the HTTP seam stubbed" block is the right instinct). CI is green. Blocking on the issue-closure claim and two verification gaps, not on the design. ## Blocking - **`Closes #439` over-claims the work.** Issue #439 defines the needs-you set as four sources: PRs awaiting the human, issues gaining `Status/Blocked` / `Status/Need More Info`, CEO proposals/questions awaiting greenlight, and failed systemd units — plus "group by repo" and "include how long it has been waiting". This PR implements exactly one source (PR review requests), no wait time, no grouping. The issue's own blocked note says to *split into smaller issues*, which is what this appears to be — but merging it with `Closes` will auto-close #439 and silently drop the other three sources and the wait-time/grouping requirements. Change the description to `Part of #439` (or `Refs #439`) and file the follow-up issues for the remaining sources before this lands. This is the one item the human would otherwise have to catch by hand. - **Two new entry points source `lib/email.sh` with no runtime coverage.** `bin/site-work-block.sh:101` and `bin/ideation-pipeline.sh:102` now source `lib/email.sh` and `lib/reviewnotify.sh`. Neither script is exercised by `check-sync.sh`, and both run under `set -u`-style strictness elsewhere in the repo. If `lib/email.sh` has any top-level code that touches unset config (`SMTP2GO_*`, recipient lists), these two entry points now abort at startup — a regression in the PR-opening path itself, which is far worse than a missed email. Please confirm `lib/email.sh` is side-effect-free at source time (a one-line note in the PR is enough if so). - **The delivery seam is never exercised, even indirectly.** `review_notify_human` calls `recipients_with_primary ""` and `email_send "$subject" "$html" "$text" "$recipients"`; `bin/test-reviewnotify.sh` stubs *both*. Nothing in the diff or the tests pins those signatures to the real ones in `lib/email.sh`, so an argument-order mismatch ships green and produces mail that is wrong (or no mail at all) — invisible, because the whole module is best-effort and returns 0 on failure. Either add a cheap assertion against the real `email_send`'s arity/param order, or state in the PR that you verified it against `lib/email.sh` (which isn't in the diff, so I can't). ## Non-blocking findings - **Cross-writer clobbering of `discretionary-state.json`.** `review_notify_human` does its own read-modify-write (`jq … "$state_file" > "$tmp"; mv`). If any other writer — `tick.sh`, the `.review` recorder — snapshots the file in memory earlier in the tick and rewrites it wholesale at the end, the `.review_notified` entry written mid-tick is lost, dedup never sticks, and you get the per-tick flood this feature exists to prevent. Worth confirming every writer is read-modify-write-at-write-time. No test covers the interaction. - **Diff size.** ~540 added lines (190 lib + 263 test + 55 check-sync + entry points) against the ~400 budget, on an issue that was *already* blocked once for scope. Nothing was deleted to fit, so this isn't the bad kind of overrun, but the `check-sync.sh` wiring guard (55 lines, a new repo-wide invariant over every `bin/*.sh`) is arguably its own change. - **Description omits ~70 lines of the diff.** The body explains the hook and the dedup but never mentions the `check-sync.sh` guard or the two entry-point source additions. The check-sync change is the one a human most needs told about: it now *fails the build* for any `bin/*.sh` that can reach `forgejo_request_review`/`forgejo_open_pr` without sourcing `reviewnotify.sh`. That's a new global constraint on future scripts and it isn't in the description. - `mktemp "${state_file}.XXXXXX"` + `mv` will reset the state file's mode to 0600 on every successful record. Fine if it's single-user; a problem if anything else reads it. - `reviewnotify_dedup_head`'s hour bucket is a good fix for unbounded fail-open, but note that a persistently broken `forgejo_get_pr` still yields one mail/hour/PR with no title and no URL — i.e. an alert the operator can't act on. Consider saying "PR details unavailable" in the body for that path. ## Test coverage Good. The dedup matrix, fail-open cases, record-only-on-success, HTML escaping, reviewer mismatch, and the fresh-state-dir path are all real assertions, and the two subshell-swallowed guards being pulled in-shell is exactly the kind of self-check I'd want. The gap is everything downstream of `email_send` (above) and the two new entry points that source `email.sh`. --- _{Independent review by the harness on `claude-opus-5` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.} <!-- review sha=c63ed41e4565e4ab63bd1c3c7d2ea8713afe636f verdict=REQUEST_CHANGES ci=success -->

igor added spent time 2026-07-28 23:31:27 +00:00

2 minutes 10 seconds

igor self-assigned this 2026-07-28 23:31:27 +00:00

igor referenced this pull request 2026-07-28 23:43:57 +00:00

PR #451 says Closes #439 but only covers one slice -- split it? #452

igor commented 2026-07-28 23:45:03 +00:00

Author

Collaborator

Copy link

🔧 Rework — round 2 (automated)

Addressed the review on claude-opus-5 at effort xhigh — 1 new commit(s).

### 🔧 Rework — round 2 _(automated)_ Addressed the review on `claude-opus-5` at **effort xhigh** — 1 new commit(s). <!-- audit:rework round=2 effort=xhigh -->

igor removed their assignment 2026-07-28 23:45:33 +00:00

igor added spent time 2026-07-28 23:45:33 +00:00

12 minutes 45 seconds

igor referenced this pull request from a commit 2026-07-28 23:45:34 +00:00

fix: pin the notifier's delivery seam and cover the new sources

igor added 1 commit 2026-07-28 23:45:34 +00:00

fix: pin the notifier's delivery seam and cover the new sources ... df468d9dcd

Addresses the shadow review on #451.

Blocking, both verification gaps:

- The delivery seam is now driven for real. Every assertion stubbed
  BOTH recipients_with_primary and email_send, so an argument-order
  drift against lib/email.sh (which is not in this diff) would ship
  green and mail garbage -- invisibly, since the module is
  best-effort and returns 0 either way. The new block sources the
  real lib/email.sh and replaces only curl, then asserts each of the
  four arguments lands in the right SMTP2GO field. Mutation-checked
  by swapping the html/text arguments: caught. The payload is
  captured to a FILE, not a variable -- email_send reads curl through
  a command substitution, so a stub assigning a variable is discarded
  in the subshell and the block would have asserted against "" and
  passed on a notifier that sent nothing.

- lib/email.sh is side-effect-free at source time: one conditional
  `log` fallback and one constant, no read of SMTP2GO_* or any
  recipient list. That is now asserted rather than claimed -- the new
  block sources email.sh + reviewnotify.sh in a clean `bash -u` with
  that config unset, which is the shape site-work-block.sh and
  ideation-pipeline.sh now run. Mutation-checked by adding a
  top-level unset-variable read: caught.

Non-blocking:

- A failed PR fetch now says so in the body ("Forgejo did not return
  the PR's details -- open <repo> and look for #<n>") instead of a
  titleless alert with a blank line where the link goes.

- Cross-writer clobbering does not happen, and the comment now says
  why: every writer of discretionary-state.json re-reads the file
  inside the jq that produces its replacement (tick.sh's slot /
  weekly / seo / sports / review / cascade helpers, needsyou, ceo,
  feedback, automerge, deferred, logwatch, claude). None snapshots it
  early and rewrites it wholesale. Checked by grepping every
  mktemp+mv site for a jq fed from a variable instead of the file:
  none.

- mktemp's 0600 is left alone deliberately. Every other mktemp+mv
  writer of that file resets the mode the same way, so preserving it
  here would only mean the next slot_mark_done undoes it.

Two things I could NOT do from a rework tick, both for the human:

- The body still reads `Closes #439`. The harness rewrites a PR body
  only on the issue-work path (pr_body_ensure_closes); PR-review
  rework never touches it, and there is no helper for it. It needs a
  one-word edit to `Part of #439` before merge, or the merge closes
  #439 and drops the other three sources. #452 is already open and
  assigned, asking for exactly that split -- agent-ask declined a
  second one, correctly.

- The description also omits the check-sync.sh guard, which is the
  part a reviewer most needs told: CI now FAILS for any bin/*.sh that
  can reach forgejo_request_review or forgejo_open_pr without
  sourcing lib/reviewnotify.sh. That is a new repo-wide constraint on
  future entry points, and it is not in the PR body.

igor commented 2026-07-28 23:49:57 +00:00

Author

Collaborator

Copy link

🤖 Review — COMMENT (automated)

CI for df468d9d: success

Adds lib/reviewnotify.sh (email the operator when Igor requests his review), hooks it into forgejo_request_review, wires three entry points, and adds a check-sync.sh guard plus a 323-line test file. CI is green and I found no defect I'd block on — but the diff is well over the size budget and a few behaviors are worth a human glance, so this is a COMMENT rather than an approve.

Checklist / description honesty

Verified — every claim in the description maps to something real in the diff:

• Hook at forgejo_request_review only (lib/forgejo.sh:289-298), fired by name via declare -F, || true so a notifier failure can't fail a landed request. ✔
• Dedup on repo#number + head sha, recorded only after a successful send (lib/reviewnotify.sh:180-206). ✔
• All four "shadow review" fixes are present: entry-point sourcing (bin/site-work-block.sh, bin/ideation-pipeline.sh) + the transitive check-sync.sh guard; hour-bucket cap in reviewnotify_dedup_head; reviewnotify_escape_html on the title; mkdir -p + mktemp beside the state file. ✔
• Part of #439 (not Closes) is the correct call — #439 specifies issues/CEO proposals/systemd/wait-times/weekly summary, none of which are here.

Findings (non-blocking, but I'd want eyes on these)

• Size: ~618 added lines, over the ~400 budget. lib/reviewnotify.sh is +208 (roughly half comment prose) and bin/test-reviewnotify.sh is +323. Nothing was trimmed to fit — the opposite — so this isn't the "shrunk by deleting tests" failure, but it's the same cap that got #439 blocked at 792 lines. If the repo enforces the budget, the natural split is (a) lib/reviewnotify.sh + hook + tests, then (b) the check-sync.sh wiring guard as a follow-up.

• A transient PR-fetch failure costs two extra emails, not one (lib/reviewnotify.sh:79-88, 170-176). Sequence: mailed at abc → fetch fails → key becomes nohead-N, differs from abc, sends and overwrites the recorded sha → fetch recovers, head is still abc, which now differs from nohead-N → sends again. Consistent with the deliberate fail-open design, but the module docs describe it as "costs nothing once the fetch recovers," which isn't quite true. Recording the bucket under a separate key (or keeping the last-known real sha alongside) would make it exactly one.

• New unlocked writers to discretionary-state.json from child processes. review_notify_human does read-modify-write + mv (atomic rename, so no corruption), but bin/site-work-block.sh and bin/ideation-pipeline.sh now write this file too. If either can overlap with bin/tick.sh, a lost update clobbers whatever other subsystem wrote between the jq read and the mv — not just a duplicate email. The in-code comment addresses in-memory snapshots, which is a different hazard. Worth confirming these are strictly sequential children of the tick.

• The check-sync.sh guard is weaker than "the design claim enforces itself." sourced_libs only recognises . "$AGENT_HOME/lib/x.sh" (bin/check-sync.sh:120). An entry point that sources via a different variable ($HERE, a relative path, a computed path) yields an empty reach set and hits the [ -n "${callers// /}" ] || continue early-out — i.e. it passes silently rather than failing. Same for any future entry point outside bin/*.sh. Good guard, but it's a lint-grade heuristic, not the invariant the comment claims.

• Subject line isn't escaped/sanitised, only the HTML body (reviewnotify_subject, used raw at lib/reviewnotify.sh:170). Low risk — SMTP2GO's JSON API sets headers, so no CRLF header injection, and mail subjects render as text — but the title is the one attacker-influenceable string on this path and the asymmetry is worth being deliberate about.

• log "reviewnotify: emailed ${recipients} ..." writes the operator's address to the log. Not a secret; flagging only in case logs are shipped anywhere.

What I couldn't verify from the diff

• lib/email.sh and lib/review.sh aren't in the diff, so I can't independently confirm email_send's 4-arg signature or recipients_with_primary's. The final test block sources the real lib/email.sh and stubs only curl, which is exactly the right way to pin this — I'm relying on green CI for it.
• Whether any other bin/*.sh (e.g. agent-*.sh) textually reaches forgejo_request_review/forgejo_open_pr and would now be forced to source the notifier. Green CI says none do today, but the new guard makes that a standing constraint on every future entry point.

Test coverage

Genuinely good, and unusually honest about its own blind spots: the "hook is actually wired" block drives the real forgejo_request_review with only _forgejo_post_reviewers stubbed, the delivery-seam block sources the real email.sh and writes the curl payload to a file (correctly noting the command-substitution subshell would swallow a variable), and the set -u source-cleanliness check guards the two entry points this PR newly made depend on email.sh. The mutation-testing claim ("hook-unwired, dedup-disabled, record-before-send each caught") is a process assertion I can't check from the diff, but the corresponding assertions do exist.

---

_{Independent review by the harness on claude-opus-5 (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `COMMENT` _(automated)_ CI for `df468d9d`: **success** Adds `lib/reviewnotify.sh` (email the operator when Igor requests his review), hooks it into `forgejo_request_review`, wires three entry points, and adds a `check-sync.sh` guard plus a 323-line test file. CI is green and I found no defect I'd block on — but the diff is well over the size budget and a few behaviors are worth a human glance, so this is a **COMMENT** rather than an approve. ## Checklist / description honesty Verified — every claim in the description maps to something real in the diff: - Hook at `forgejo_request_review` only (`lib/forgejo.sh:289-298`), fired by name via `declare -F`, `|| true` so a notifier failure can't fail a landed request. ✔ - Dedup on `repo#number` + head sha, recorded only after a successful send (`lib/reviewnotify.sh:180-206`). ✔ - All four "shadow review" fixes are present: entry-point sourcing (`bin/site-work-block.sh`, `bin/ideation-pipeline.sh`) + the transitive `check-sync.sh` guard; hour-bucket cap in `reviewnotify_dedup_head`; `reviewnotify_escape_html` on the title; `mkdir -p` + `mktemp` beside the state file. ✔ - `Part of #439` (not `Closes`) is the correct call — #439 specifies issues/CEO proposals/systemd/wait-times/weekly summary, none of which are here. ## Findings (non-blocking, but I'd want eyes on these) - **Size: ~618 added lines, over the ~400 budget.** `lib/reviewnotify.sh` is +208 (roughly half comment prose) and `bin/test-reviewnotify.sh` is +323. Nothing was trimmed to fit — the opposite — so this isn't the "shrunk by deleting tests" failure, but it's the same cap that got #439 blocked at 792 lines. If the repo enforces the budget, the natural split is (a) `lib/reviewnotify.sh` + hook + tests, then (b) the `check-sync.sh` wiring guard as a follow-up. - **A transient PR-fetch failure costs two extra emails, not one** (`lib/reviewnotify.sh:79-88`, `170-176`). Sequence: mailed at `abc` → fetch fails → key becomes `nohead-N`, differs from `abc`, sends and *overwrites* the recorded sha → fetch recovers, head is still `abc`, which now differs from `nohead-N` → sends again. Consistent with the deliberate fail-open design, but the module docs describe it as "costs nothing once the fetch recovers," which isn't quite true. Recording the bucket under a separate key (or keeping the last-known real sha alongside) would make it exactly one. - **New unlocked writers to `discretionary-state.json` from child processes.** `review_notify_human` does read-modify-write + `mv` (atomic rename, so no corruption), but `bin/site-work-block.sh` and `bin/ideation-pipeline.sh` now write this file too. If either can overlap with `bin/tick.sh`, a lost update clobbers whatever *other* subsystem wrote between the `jq` read and the `mv` — not just a duplicate email. The in-code comment addresses in-memory snapshots, which is a different hazard. Worth confirming these are strictly sequential children of the tick. - **The `check-sync.sh` guard is weaker than "the design claim enforces itself."** `sourced_libs` only recognises `. "$AGENT_HOME/lib/x.sh"` (`bin/check-sync.sh:120`). An entry point that sources via a different variable (`$HERE`, a relative path, a computed path) yields an empty reach set and hits the `[ -n "${callers// /}" ] || continue` early-out — i.e. it passes silently rather than failing. Same for any future entry point outside `bin/*.sh`. Good guard, but it's a lint-grade heuristic, not the invariant the comment claims. - **Subject line isn't escaped/sanitised**, only the HTML body (`reviewnotify_subject`, used raw at `lib/reviewnotify.sh:170`). Low risk — SMTP2GO's JSON API sets headers, so no CRLF header injection, and mail subjects render as text — but the title is the one attacker-influenceable string on this path and the asymmetry is worth being deliberate about. - `log "reviewnotify: emailed ${recipients} ..."` writes the operator's address to the log. Not a secret; flagging only in case logs are shipped anywhere. ## What I couldn't verify from the diff - `lib/email.sh` and `lib/review.sh` aren't in the diff, so I can't independently confirm `email_send`'s 4-arg signature or `recipients_with_primary`'s. The final test block sources the real `lib/email.sh` and stubs only `curl`, which is exactly the right way to pin this — I'm relying on green CI for it. - Whether any other `bin/*.sh` (e.g. `agent-*.sh`) textually reaches `forgejo_request_review`/`forgejo_open_pr` and would now be forced to source the notifier. Green CI says none do today, but the new guard makes that a standing constraint on every future entry point. ## Test coverage Genuinely good, and unusually honest about its own blind spots: the "hook is actually wired" block drives the real `forgejo_request_review` with only `_forgejo_post_reviewers` stubbed, the delivery-seam block sources the real `email.sh` and writes the curl payload to a *file* (correctly noting the command-substitution subshell would swallow a variable), and the `set -u` source-cleanliness check guards the two entry points this PR newly made depend on `email.sh`. The mutation-testing claim ("hook-unwired, dedup-disabled, record-before-send each caught") is a process assertion I can't check from the diff, but the corresponding assertions do exist. --- _{Independent review by the harness on `claude-opus-5` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.} <!-- review sha=df468d9dcddb83c3e1c0a97e658f9232c147ed72 verdict=COMMENT ci=success -->

igor added spent time 2026-07-28 23:49:57 +00:00

3 minutes 16 seconds

igor requested review from joshtronic 2026-07-28 23:49:58 +00:00

joshtronic approved these changes 2026-07-29 00:31:21 +00:00

joshtronic merged commit 0d87da82de into master 2026-07-29 00:31:25 +00:00

joshtronic deleted branch feat/439-email-on-review-request 2026-07-29 00:31:25 +00:00

joshtronic referenced this pull request from a commit 2026-07-29 00:31:27 +00:00

Merge pull request 'feat: email the operator when Igor requests his review' (#451) from feat/439-email-on-review-request into master

Sign in to join this conversation.

Reviewers

No reviewers

joshtronic

Labels

Clear labels   

Agent

  

Compat/Breaking

Breaking change that won't be backward compatible

  

Kind/Bug

Something is not working

  

Kind/Documentation

Documentation changes

  

Kind/Enhancement

Improve existing functionality

  

Kind/Feature

New functionality

  

Kind/Security

This is security issue

  

Kind/Testing

Issue or pull request related to testing

  

Priority

Critical

The priority is critical

  

Priority

High

The priority is high

  

Priority

Low

The priority is low

  

Priority

Medium

The priority is medium

  

Reviewed

Confirmed

Issue has been confirmed

  

Reviewed

Duplicate

This issue or pull request already exists

  

Reviewed

Invalid

Invalid issue

  

Reviewed

Won't Fix

This issue won't be fixed

  

Status

Abandoned

Somebody has started to work on this but abandoned work

  

Status

Blocked

Something is blocking this issue or pull request

  

Status

Need More Info

Feedback is required to reproduce issue or to continue work

No labels

Agent

Compat/Breaking

Kind/Bug

Kind/Documentation

Kind/Enhancement

Kind/Feature

Kind/Security

Kind/Testing

Priority

Critical

Priority

High

Priority

Low

Priority

Medium

Reviewed

Confirmed

Reviewed

Duplicate

Reviewed

Invalid

Reviewed

Won't Fix

Status

Abandoned

Status

Blocked

Status

Need More Info

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Total time spent: 37 minutes 7 seconds

igor

37 minutes 7 seconds

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

joshtronic/igor!451

No description provided.