Skip to content

feat(hooks): add local extension points via config/hooks lifecycle hooks and bin-local - #531

Closed
julius-retzer wants to merge 20 commits into
kunchenguid:mainfrom
julius-retzer:fm/fm-extension-points-f3
Closed

julius-retzer wants to merge 20 commits into
kunchenguid:mainfrom
julius-retzer:fm/fm-extension-points-f3

Conversation

@julius-retzer

@julius-retzer julius-retzer commented Jul 13, 2026 •

Copy link
Copy Markdown

What Changed

  • Added bin/fm-hooks-lib.sh, a bash-3.2-compatible library that fires lifecycle hooks from config/hooks/, and wired hook points into fm-spawn.sh, fm-teardown.sh, fm-pr-check.sh, fm-pr-merge.sh, and the new fm-merge-local.sh; hooks run last, get detached/byte-capped stdio, and are bounded by a shell watchdog (with process-group kill) when timeout is absent.
  • Introduced bin-local/ as an overlay for local scripts (gitignored, preserved by updatefirstmate), and gated the pr-ready marker behind an installed hook so it fires once via a durable meta marker after merge.
  • Documented the extension points across docs/extension-points.md, docs/configuration.md, docs/architecture.md, README.md, AGENTS.md, and CONTRIBUTING.md, and added tests/fm-hooks-lib.test.sh, tests/fm-bash32.test.sh, plus teardown/lifecycle test coverage.

Risk Assessment

✅ Low: Purely additive, gitignored, ships-no-hooks feature that is a no-op by default; the runner is defensively correct and the non-hook refactors are semantics-preserving.

Testing

Completed 1 recorded test check.

  • Outcome: ⚠️ 1 error across 1 run (51m20s)

Pipeline

Updates from git push no-mistakes

⏭️ **intent** - skipped

✅ No issues found.

🔧 **Rebase** - 4 issues found → auto-fixed ✅
  • ⚠️ AGENTS.md - merge conflict rebasing onto origin/main
  • ⚠️ bin/fm-pr-check.sh - merge conflict rebasing onto origin/main
  • ⚠️ bin/fm-pr-merge.sh - merge conflict rebasing onto origin/main
  • ⚠️ docs/scripts.md - merge conflict rebasing onto origin/main

🔧 Fix applied.
✅ Re-checked - no issues remain.

⚠️ **Review** - 1 info
  • ℹ️ bin/fm-hooks-lib.sh:284 - fm_hook_run unconditionally grabs fds 6/7/8/9 (exec 7>... 9<... 8<>... 6<...) and releases them by closing, not by save/restore. The same scripts source fm-pr-lib.sh (uses fd 8) and, on the check path, fm-check-lib.sh (uses fd 9). Those uses are transient inside their own functions and don't overlap the end-of-script hook calls, so all current callers are safe. But this is a shared library meant to be sourced widely; a future caller that invokes fm_hook_run while holding fd 6-9 open (a lock fd, an open read loop) would have it silently closed. Worth a header note pinning the fd range as reserved.
⚠️ **Test** - 1 error
  • 🚨 tests failed with exit code 1
  • 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"
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

@julius-retzer
julius-retzer force-pushed the fm/fm-extension-points-f3 branch from b542b0c to 7fabefc Compare July 14, 2026 17:39
@julius-retzer julius-retzer changed the title feat: add local extension points (config/hooks/ lifecycle hooks, bin-local/) feat: add local extension points via config/hooks lifecycle hooks and bin-local Jul 14, 2026
@julius-retzer
julius-retzer marked this pull request as ready for review July 14, 2026 21:44
@kunchenguid

kunchenguid commented Jul 14, 2026 •

Copy link
Copy Markdown
Owner

Thanks for the PR! This branch currently has a merge conflict with the base branch.

When you get a chance, please rebase onto (or merge) the latest base branch, resolve the conflict, and push. After that, checks will re-run and the PR will get looked at again.

Noted for firstmate#531 at 7fabefc5.

@julius-retzer
julius-retzer force-pushed the fm/fm-extension-points-f3 branch from 7fabefc to d36a9ef Compare July 15, 2026 14:34
@julius-retzer julius-retzer changed the title feat: add local extension points via config/hooks lifecycle hooks and bin-local feat: add local lifecycle hooks and bin-local extension points Jul 15, 2026
@julius-retzer

Copy link
Copy Markdown
Author

Thanks for the PR! This branch currently has a merge conflict with the base branch.

When you get a chance, please rebase onto (or merge) the latest base branch, resolve the conflict, and push. After that, checks will re-run and the PR will get looked at again.

Noted for firstmate#531 at 7fabefc5.

Done

@kunchenguid kunchenguid removed the wheelhouse:pending-contributor-action Managed by Wheelhouse label Jul 15, 2026
@kunchenguid

kunchenguid commented Jul 16, 2026 •

Copy link
Copy Markdown
Owner

Automated reminder: thanks for the PR! This branch currently has a merge conflict with the base branch.

When you get a chance, please rebase onto (or merge) the latest base branch, resolve the conflict, and push. After that, checks will re-run and the PR will get looked at again.

Noted for firstmate#531 at d36a9ef9.

…local/)

Adds a shared best-effort hook runner (bin/fm-hooks-lib.sh) and four
gitignored-local lifecycle hook points: post-spawn, pr-ready, post-merge,
and post-teardown, plus a bin-local/ home dir for personal helper scripts.
Absent hooks are silent no-ops; failing or hanging hooks warn and never
block the calling flow. docs/extension-points.md owns the contract.

Convention-compatible with PR kunchenguid#371's post-worktree-create seam (same
config/hooks/ dir, runner semantics, FM_HOOK_TIMEOUT, args+env mirroring).
@julius-retzer
julius-retzer force-pushed the fm/fm-extension-points-f3 branch from d36a9ef to b8277d1 Compare July 20, 2026 23:41
@julius-retzer julius-retzer changed the title feat: add local lifecycle hooks and bin-local extension points feat(hooks): add local extension points via config/hooks lifecycle hooks and bin-local Jul 20, 2026
@julius-retzer julius-retzer reopened this Jul 20, 2026
@kunchenguid

kunchenguid commented Jul 21, 2026 •

Copy link
Copy Markdown
Owner

Automated reminder: thanks for the PR! This branch currently has a merge conflict with the base branch.

When you get a chance, please rebase onto (or merge) the latest base branch, resolve the conflict, and push. After that, checks will re-run and the PR will get looked at again.

Noted for firstmate#531 at b8277d1d.

@ami230392

Copy link
Copy Markdown

Re-review after the fix round — verdict: approve

Reviewed d628a80, new diff only (git diff 6b757c7..d628a80, 7 files, +95/−22). Read-only: nothing pushed, nothing edited.

1. Hindi-scan guard — fixed. router.ts:57-71 adds isProvenSafeForClaude; :81-83 throws for any anthropic route unless the caller proves safety (language === "en" or isScanned === false). Default is deny, so the shape every real call site uses today — resolveAiRoute(job) with no context — now throws. Five unsafe shapes covered explicitly (no context, {}, hi+scanned, hi+scan-unknown, scanned+language-unknown). pnpm --filter shared test → 219 passed, router file 14 → 17.

On the exact ask, "a test that fails if a call site could route a Hindi scan to Claude": no Anthropic call site exists (bidprep.draft has zero callers), so that test isn't constructible. Default-deny is the right substitute — the first bid-prep caller who copies the one-argument call shape throws instead of silently violating never-#4. That's the teeth.

Residual, not blocking: the proof is caller-asserted and unvalidated — {language:"en"} on a document that is actually a Hindi scan still routes to Claude. Worth a line in the bid-prep issue.

2. Retry cap — fixed. ai-analysis.ts:109 and api/ask/route.ts:95 now carry httpOptions: { timeout, retryOptions: { attempts: 1 } }, same placement (inside config) as the two worker sites. grep -rn httpOptions apps/worker/src apps/web/src → all four Gemini call sites capped. One metered unit = at most one provider request. No unit test asserts it (SDK retry isn't observable without a transport fake); the grep is the receipt and the convention is uniform enough to be greppable.

3. GOOGLE_API_KEY — fixed. OPERATIONS.md:47-53 states it's required on Railway before ENRICHMENT_ENABLED flips, notes it isn't set yet, and that Vercel needs the same key. Partly addressed: the retired ANTHROPIC_MODEL / AI_TASTE_MODEL / ENRICHMENT_MODEL are still not listed as retired — zero hits in OPERATIONS.md and .env.example, so no doc is wrong, the stale values just sit unlabelled in the two dashboards. Cosmetic; tell the operator rather than block.

CI: 5 passed, 0 failed, 1 pending (Vercel deploy). Wiki freshness green.

Still open (not this round's scope): items 4–8 from the first review. Worth a follow-up issue — especially #4 (lint rule escapable via template literal and models/ vendor-path form) and #7 (gemini-3.5-flash-lite never smoke-tested live; a wrong id is a simultaneous outage of extraction, analysis and Ask-AI).

Full report: data/rereview-531/report.md.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants