feat(outbox): sync PR results to per-task Mattermost channels and Focalboard cards - #1
Merged
Merged
Conversation
* fix(teardown): make landed-check robust when no pr= was ever recorded fm-teardown.sh's squash-merge landed-check already falls back to discovering a merged PR by branch name when state/<id>.meta has no recorded pr=, but nothing guaranteed pr=/pr_head= actually got recorded on a yolo-authorized merge - the "checks green" trigger that normally runs fm-pr-check.sh never fires on repos with no PR CI, so a merge done via a bare `gh-axi pr merge` silently skips it. Add bin/fm-pr-merge.sh as the one path for merging a task's PR: it always runs fm-pr-check.sh first, so pr=/pr_head= land in meta as part of the merge itself regardless of any CI signal. Document both the existing branch-name discovery fallback and the new merge path in AGENTS.md, and add regression coverage for the no-pr=-recorded landed scenario and for fm-pr-merge.sh's record-then-merge behavior. * no-mistakes(review): Guard PR merges on task metadata * no-mistakes(document): Document PR merge wrapper * no-mistakes: apply CI fixes
* Fix fm-pr-merge.sh to parse PR URLs for gh-axi gh-axi pr merge expects a PR number and --repo, not a full GitHub URL. Parse the URL, default to --squash when no merge method is passed, and fail fast on malformed URLs. Tests cover parsing, defaults, and refusal. * no-mistakes(review): Harden PR merge validation * no-mistakes(review): Harden PR merge URL guards * no-mistakes(document): Document PR merge URL handling * no-mistakes(lint): Clean shell lint
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
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.
Intent
The developer wanted to enhance the Mattermost outbox watcher script so that each outbox result entry could optionally post to a task-specific Mattermost channel and interact with a specific Focalboard card, rather than always posting to a single hardcoded "SG AI Coordination" thread. They extended the outbox JSON schema with four new optional fields: target_channel_id, board_id, card_id, and new_status. When present, the watcher should post the PR summary and risk to the specified channel, add a Focalboard comment to the given card, and move the card's status to the new_status value. The developer required backward compatibility so that old outbox entries without these fields continue posting to the existing destination unchanged. They also required idempotency to prevent double-posting on rescan, Focalboard credentials sourced exclusively from environment variables (FM_FOCALBOARD_URL and FM_FOCALBOARD_TOKEN) with a graceful warning when missing, and delivery through the no-mistakes CI pipeline with bash tests covering parsing, fallback, Focalboard API call construction, and duplicate suppression.
What Changed
bin/fm-mattermost-outbox-watch.sh) to read four new optional fields per result entry (target_channel_id,board_id,card_id,new_status) and, when present, post the PR summary to a task-specific Mattermost channel, add a comment to the matching Focalboard card, and move that card's status - falling back to the existing hardcoded destination when the fields are absent.flock-based locking (replacing a mkdir spin-wait),FM_FOCALBOARD_TOKENdelivered via curl stdin config instead of a shell argument, logged Focalboard HTTP error bodies, non-fatal Focalboard errors in--watchmode, and a newbin/fm-pr-merge.shhelper that recordspr=andpr_head=in task meta before merging.tests/fm-mattermost-outbox-watch.test.sh,tests/fm-pr-merge.test.sh, extendedtests/fm-teardown.test.sh) and documentation (docs/configuration.md,docs/architecture.md) covering the new schema fields, Focalboard credentials, idempotency markers, andfm-pr-mergeusage.Risk Assessment
✅ Low: All prior findings are resolved, the Focalboard error-absorption is correctly scoped to post_pr without touching the Mattermost failure path, and the new fm-pr-merge.sh is well-tested with appropriate guards.
Testing
Ran the 9-case mattermost outbox watcher test suite plus the two other changed test files (teardown, pr-merge); all pass. Tests exercise the full end-to-end path from outbox JSON parsing through hermes send and stubbed Focalboard curl calls, including idempotency markers, credential-missing warnings, and backward compatibility for old entries.
Evidence: fm-mattermost-outbox-watch test results
ok - Mattermost outbox watcher posts a PR once and records a durable marker ok - missing outbox is a safe no-op ok - malformed JSON fails safely without sending ok - unresolved Mattermost target fails closed ok - explicit Mattermost target bypasses target listing ok - target_channel_id and summary fields are parsed and used ok - Focalboard comment and move request construction is correct ok - missing Focalboard credentials warn without blocking Mattermost ok - duplicate scans suppress Mattermost and Focalboard side effectsPipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 4 issues found → auto-fixed (2) ✅
bin/fm-mattermost-outbox-watch.sh:319- Misleading warning message: 'Mattermost posting was still attempted' but at this call site post_mattermost already returned 0 (posting completed or idempotently skipped). 'attempted' implies uncertain outcome, when the reality is the post succeeded.bin/fm-mattermost-outbox-watch.sh:308- focalboard_curl discards API response body on HTTP errors. curl -fsS sends only 'curl: (22) The requested URL returned error: 4xx' to stderr; the API error JSON (e.g. {"error":"authentication failed"}) goes to stdout which is redirected to /dev/null. On misconfigured credentials or wrong base URL this gives no actionable body to diagnose against.bin/fm-mattermost-outbox-watch.sh:230- target_channel_id is read twice per file: once inside mattermost_target_for (to decide target) and again immediately afterward in post_mattermost (to decide the marker key). The double jq invocation is benign but the fields could be returned together in one pass or via a shared local variable.bin/fm-mattermost-outbox-watch.sh:68- with_lock now returns 1 on flock timeout, causing --watch mode to exit (via 'with_lock || exit 1') instead of logging and continuing the poll loop as before. The old mkdir spin-wait returned 0 on contention. The new behavior is cleaner for detecting duplicate watcher processes, but it is a behavioral change for any automated --watch use outside manual smoke tests.🔧 Fix: fix warning accuracy and log focalboard HTTP error body
1 warning still open:
bin/fm-mattermost-outbox-watch.sh:380- In --watch mode, a transient Focalboard API error (e.g. HTTP 503) causes sync_focalboard to return 1, which propagates through post_pr → with_lock → 'with_lock || exit 1', killing the watcher. The Mattermost marker for the current entry was already written, so Focalboard would retry on next run -- but no future outbox entries (including ones with no Focalboard fields) get processed until the daemon is manually restarted. Before this change, only Mattermost failures could terminate --watch mode; now a Focalboard outage does too. The --once / systemd path-activation mode is unaffected since path activation re-triggers on the next file change.🔧 Fix: make Focalboard sync errors non-fatal in --watch mode
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"bash tests/fm-mattermost-outbox-watch.test.shbash tests/fm-teardown.test.shbash tests/fm-pr-merge.test.sh✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.