Make imaging tasks announce the outcome they actually had - #1205
Merged
Conversation
Closes half of #1202. Three defects in one place, all in TaskQueue::checkout()'s single unconditional notify(). HOST_IMAGEUP_COMPLETE had no caller anywhere in the tree. All three bundled notification plugins register a listener for it -- slack, ntfy and pushbullet each register it alongside HOST_IMAGE_COMPLETE on the same class -- so the name was plainly meant to mean "a capture finished". Nothing ever fired it, so a capture announced itself with the deploy name and nothing listening could tell "an image finished uploading" from "a machine finished being imaged". checkout() already branches on isDeploy()/isCapture() forty lines above, so it knew which it was and threw the answer away. HOST_IMAGE_FAIL had no caller either, so its listeners have never run on any server. It now fires from checkout()'s catch, which is the one place in core that knows imaging ran and FOG then failed to record it -- the host update, the task save, the task log or the imaging log. That is not the same as "FOS could not write the disk", which the server still cannot see at all; see the issue. And the notification did not check that the task was imaging. checkout() is reached from Post_Wipe.php as well as Post_Stage2/3.php, so wiping a disk sent "This host has finished imaging." The payload keeps HostName exactly as it was -- it is the only key any current listener reads, in core, in the bundled plugins and in third-party plugins nobody here can inspect -- and adds Host, Task, Image, ImageName, TaskType, plus Reason on the failure path. Additive, so no listener needs editing; the bundled ones are updated separately in fog-plugins to use it. Gated by tests/imaging-notify-events.test.php and mutation-verified: dropping the imaging guard, the capture branch, the failure branch, the catch's call or the ImageName key each fails the suite. Co-Authored-By: Claude <noreply@anthropic.com>
mastacontrola
added a commit
to FOGProject/fog-plugins
that referenced
this pull request
Aug 19, 2026
The other half of [FOGProject/fogproject#1202](FOGProject/fogproject#1202); the core half is [FOGProject/fogproject#1205](FOGProject/fogproject#1205). Neither depends on the other landing first. ## What changed upstream Core now fires `HOST_IMAGEUP_COMPLETE` for a capture, `HOST_IMAGE_COMPLETE` for a deploy and `HOST_IMAGE_FAIL` when imaging did not finish — and it sends the image and the failure reason alongside the host name. Before that, **two of those three names had no caller anywhere in FOG**, so listeners registered for them had never run. ## What these listeners did with it Nothing, because they could not. Every one of the six registered `HOST_IMAGEUP_COMPLETE` or `HOST_IMAGE_FAIL` and then ignored which event had arrived: ```php public function onEvent($event, $data) { self::$message = 'This host has finished imaging.'; self::$shortdesc = 'Imaging Complete'; parent::onEvent($event, $data); } ``` So a finished capture said "This host has finished imaging", identically to a deploy, and a failure said "This host has failed to image" with no hint as to why. | | before | after | |---|---|---| | deploy | Host lab01 completed imaging. | Host lab01 finished deploying image Win11-Lab. | | capture | Host lab01 completed imaging. | Host lab01 finished capturing image Win11-Lab. | | failure | Host lab01 imaging failed. | Host lab01 failed imaging Win11-Lab: Failed to update host | ## Defensive about every added key A plugin release is not tied to a FOG release, so all six have to keep working against a server whose core still sends nothing but `HostName`: ```php $image = (string) ($data['ImageName'] ?? ''); if ('' === $image) { $image = _('an unnamed image'); } ``` Reading a missing key directly would turn the notification into a PHP warning — in an event that only fires when something has already gone wrong. That is also why `fog_min` is untouched: these work on any 1.6, they just say more on a server that has taken #1205. ## Translation The image name is substituted **outside** `_()`, not inside it — a msgid that mixes literal text with a variable extracts nothing and never translates, silently and permanently. Positional specifiers (`%1$s`) so a translator can reorder the sentence. The ntfy and pushbullet listeners compose the finished string themselves rather than handing their base class a bare literal, because the substitution has to happen after translation. The base's `_()` then finds no entry and passes it through unchanged. ## Verification `tests/imaging-notification-detail.test.php` — 42 checks across the six files, source-level for the same reason `group-tab-permissions.test.php` is: these classes extend the plugin's own Event base, which extends FOG's, so none of them loads without a booted FOG, a session and a database. It pins the defensive reads, the capture/deploy split, the registrations, and the gettext shape — that last one because **this repository has no gettext gate of its own**; fogproject's only scans fogproject. Mutation-verified: ``` drop the ImageName default -> reads ImageName without a default drop the capture branch -> does not distinguish a capture from a deploy interpolate inside _() -> interpolates a variable inside a double-quoted _() drop the Reason default -> does not report why imaging failed ``` Full suite: `7 passed, 0 failed`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_013mJVe4CpK3rRbi9H5GubXd
This was referenced Aug 19, 2026
mastacontrola
added a commit
that referenced
this pull request
Aug 19, 2026
The server half of #1206. The FOS half is [FOGProject/fos#152](FOGProject/fos#152) — this one is useful on its own (an older FOS just never calls it), the FOS one is not useful without this. ## The gap `handleError()` in FOS prints a banner to the console and `exit 1`s. It makes **no request to the server**, so a real imaging failure — bad image, mount failure, partition error, any of the ~40 `handleError` call sites across `fog.download`, `fog.upload` and `fog.mount` — has always been invisible to FOG. The only failure the server ever heard about was a storage node problem, through `fog.checkmount` → `blame.php`, and that re-queues rather than fails. #1205 wired `HOST_IMAGE_FAIL` to `TaskQueue::checkout()`'s catch, which covers "imaging ran and FOG then failed to *record* it". This covers the failure people actually mean. `service/taskerror.php` takes the host identity FOS already sends everywhere else, plus the error text and the script that raised it, and fires `HOST_IMAGE_FAIL` with the payload #1205 defined — the same keys from either path, so a listener behaves identically whichever failure reached it. ## Deliberately narrow, in three ways **It does not change the task's state.** `taskStates` has no Failed — the five are Queued, Checked In, In-Progress, Complete, Cancelled. Reusing Cancelled loses the difference between "an admin stopped this" and "this broke"; adding a sixth means every place that enumerates states has to learn about it or a failed task becomes invisible there. That is a decision with UI and API consequences, it stays on #1206, and making the event fire does not depend on taking it. **It refuses a task that is not imaging.** `HOST_IMAGE_FAIL` is an imaging event and this endpoint is reachable from a wipe or an inventory task too. Firing it there would be the defect #1202 just removed, pointing the other way. There is no event for a non-imaging task failing; noted on the issue. **It answers `##` with 200 on every path, including every rejection.** FOS calls this on its way out and cannot act on the reply. Answering identically also means the endpoint cannot be used to ask whether a given MAC has an active imaging task. ## The text Bounded to 500 characters and flattened to one line. This is unauthenticated in the same way every other `service/*.php` is — identified by MAC, reachable by anything that can reach the web tier — and the text lands in an administrator's Slack, ntfy or pushbullet message. An embedded newline there forges what looks like a second, separate notification, which is a better lie than anything markup could manage: ``` error=Could not mount images folder (fog.mount) Args Passed: --target /images Host imaging completed successfully ``` comes out as one line, with the forged "success" visibly part of the failure text. `\p{C}` rather than a newline-only strip, because a console-facing error string can carry terminal escapes too; `preg_replace` returns null rather than throwing on invalid UTF-8, so there is a byte-wise fallback — a machine with the wrong locale must not silently report nothing. ## Not a REST route `service/*.php` is served directly and is not in `Route::defineRoutes()`, so `OpenAPI::document()` is unaffected — same as `blame.php` and the `Post_Stage` endpoints. No schema change, so no `FOG_SCHEMA` bump. No change to `Route::$validClasses`, so no FogApi sync. ## Verification Live, against this install's real database, using a throwaway host on a locally-administered MAC (`02:00:00:00:0E:11`) that matches no hardware, so nothing could ever PXE boot into the task: | case | result | |---|---| | deploy task | `FOG: imaging failed on host … (task 62): Could not mount images folder (fog.mount) Args Passed: --target /images Host imaging completed successfully` — **newlines flattened** | | capture task | notified and logged | | wipe task | refused: `Task is not an imaging task` | | unknown MAC | refused: `Invalid Host` | | 4000-char report | cut to exactly 500 | | every one of them | HTTP 200, body `##` | `HOST_IMAGE_FAIL` now appears in `notifyEvents`, which is proof `notify()` was genuinely reached and got past its guards. Fixture and shadow tree removed afterwards; host count back to 86. `tests/task-error-report.test.php`, mutation-verified — dropping the control-character strip, the length bound, the imaging gate, the uniform ack, or adding a state change each fails the suite. Full suite: `68 passed, 0 failed`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_013mJVe4CpK3rRbi9H5GubXd
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the half of #1202 that lives in core. The plugin half is FOGProject/fog-plugins#21; neither depends on the other landing first.
Three defects, all in
TaskQueue::checkout()'s single unconditionalnotify().1.
HOST_IMAGEUP_COMPLETEhad no caller anywhere in the treeAll three bundled notification plugins register a listener for it, on the same class as
HOST_IMAGE_COMPLETE:So the name plainly means "a capture finished". Nothing ever fired it, so a capture announced itself with the deploy name and nothing listening could tell "an image finished uploading" from "a machine finished being imaged".
checkout()already branches onisDeploy()/isCapture()forty lines above — it knew which it was and threw the answer away.2.
HOST_IMAGE_FAILhad no caller eitherIts listeners have never run on any server. It now fires from
checkout()'s catch, which is the one place in core that knows imaging ran and FOG then failed to record it — the host update, the task save, the task log or the imaging log.This is not the same as "the deploy failed." FOS's
handleError()prints to the console andexit 1s; it reports nothing to the server, so a genuine imaging failure — bad image, mount failure, partition error — is still invisible to FOG and the task just sits in Progress. Closing that needs a failure-report endpoint plus a FOS change, which is a separate decision and is written up in the issue.3. The notification did not check the task was imaging
checkout()is reached fromPost_Wipe.phpas well asPost_Stage2/3.php, so wiping a disk sent "This host has finished imaging."The payload
HostNameis kept exactly as it was — it is the only key any current listener reads, in core, in the bundled plugins, and in third-party plugins nobody here can inspect.Host,Task,Image,ImageName,TaskTypeare added alongside it, plusReasonon the failure path. Additive, so no listener needs editing.No references in the payload:
notify()handsonEvent()a copy and discards the result, so&would imply a mutability it does not have. That distinction is the one ADR 0017 draws betweennotify()andprocessEvent().Blast radius
One behaviour change worth naming: a capture now fires
HOST_IMAGEUP_COMPLETEinstead ofHOST_IMAGE_COMPLETE. A third-party listener registered only on the deploy name stops hearing captures. That is the documented intent of having two names, and all three bundled plugins already register both, but it is a real change and not a pure fix.The other two changes can only reduce wrong notifications: a wipe stops claiming to be imaging, and a failure that previously notified nothing now notifies something.
No route change, so no OpenAPI change. No schema change.
Verification
tests/imaging-notify-events.test.php, no database. Mutation-verified — each of these fails the suite:Full suite:
67 passed, 0 failed.Downstream
No change to
Route::$validClasses, so no FogApi sync needed.🤖 Generated with Claude Code
https://claude.ai/code/session_013mJVe4CpK3rRbi9H5GubXd