feat(bin): add opt-in Herdr completed-task static views - #3063
feat(bin): add opt-in Herdr completed-task static views#3063NathanAW24 wants to merge 4 commits into
Conversation
Confidence Score: 4/5The stale-view reuse defect should be fixed before merging because a later task incarnation can be presented with an earlier task's completion details. Completed views are keyed only by reusable task ID and live presentation identity, so teardown can accept an old parked view without regenerating the summary for the task currently completing. Files Needing Attention: bin/fm-completed-view-lib.sh, bin/fm-teardown.sh, tests/fm-completed-view.test.sh Reviews (1): Last reviewed commit: "no-mistakes(review): export LC_ALL=C for..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if [ -e "$record" ] || [ -L "$record" ]; then | ||
| if ! fm_completed_view_record_load "$state" "$id" \ | ||
| || [ "$FM_COMPLETED_VIEW_RECORD_VERSION" != 2 ] \ | ||
| || [ "$FM_COMPLETED_VIEW_RECORD_HOME" != "$home" ] \ | ||
| || [ "$FM_COMPLETED_VIEW_RECORD_SESSION" != "$session" ] \ | ||
| || [ "$FM_COMPLETED_VIEW_RECORD_WORKSPACE_ID" != "$workspace" ] \ | ||
| || [ ! -f "$summary" ] || [ -L "$summary" ] \ | ||
| || ! fm_completed_view_live_identity_matches \ | ||
| "$session" "$workspace" "$FM_COMPLETED_VIEW_RECORD_TAB_ID" \ | ||
| "$FM_COMPLETED_VIEW_RECORD_PANE_ID" "$FM_COMPLETED_VIEW_RECORD_LABEL"; then | ||
| echo "error: completed-task view state for $id is ambiguous; preserving the task and retained state for inspection" >&2 | ||
| return 1 | ||
| fi | ||
| FM_COMPLETED_VIEW_PREPARED=1 | ||
| FM_COMPLETED_VIEW_REUSED=1 | ||
| FM_COMPLETED_VIEW_PHASE=$FM_COMPLETED_VIEW_RECORD_PHASE | ||
| FM_COMPLETED_VIEW_SESSION=$session | ||
| FM_COMPLETED_VIEW_WORKSPACE_ID=$workspace | ||
| FM_COMPLETED_VIEW_TAB_ID=$FM_COMPLETED_VIEW_RECORD_TAB_ID | ||
| FM_COMPLETED_VIEW_PANE_ID=$FM_COMPLETED_VIEW_RECORD_PANE_ID | ||
| return 0 |
There was a problem hiding this comment.
When a new Herdr task reuses an ID whose earlier parked view remains in the same home, session, and workspace, this branch accepts the old live record without binding it to the current spawn generation or rewriting its summary, causing the new completion to display the previous task's outcome, PR, head, report, and validation details.
1 participant
Intent
Implement opt-in parked static completed-task views for Firstmate's Herdr presentation and ship exactly one PR from feature/completed-task-views to NathanAW24/firstmate:nathan-main, without merging it. Keep the shared default off and document safe manual local enablement only after the change lands. Preserve all existing landing, report, unresolved-captain-decision, unlanded-work, focus, endpoint-ownership, presentation-identity, and cleanup safety checks. For an eligible successful completion, stop the agent and ensure no process holds the disposable worktree before returning it to the pool. Never retain raw terminal scrollback by default; retain only a bounded, deterministic, sanitized static summary with the outcome and available durable references such as full PR/report links, reviewed or delivered head, and validation/test result. Move retained presentation outside the disposable worktree, remove the task from active monitoring, and leave a clearly completed/parked Herdr workspace or tab, or the smallest safe replacement static view, until explicit dismissal. Provide safe list and explicit dismiss operations plus bounded retention. Inspect all supported runtime backends and make unsupported-backend behavior an explicit safe fallback; retained Herdr presentation must not alter cleanup semantics for tmux, Zellij, Orca, cmux, or unsupported/ambiguous environments. Extend existing lifecycle owners instead of creating a parallel control plane, keeping data formats and state-machine contracts in one authoritative owner with concise cross-references elsewhere. Add executable behavioral tests through public interfaces, update the correct public/operator and maintainer docs and current verification pointers, and run bin/fm-doc-audience-check.sh and bin/fm-lint.sh as applicable. Any live Herdr lifecycle validation must use the brief's named non-default isolated Herdr lab helper contract. Do not modify private/home-local configuration as a substitute for tracked implementation. Keep the change direct and minimal, and do not merge the PR.
What Changed
bin/fm-completed-view-lib.shandbin/fm-completed-view.shimplementing opt-in, bounded static completed-task views for the Herdr backend: whenconfig/herdr-completed-task-viewsis set to"on", a successful non-forced teardown parks a sanitized summary tab outside the disposable worktree (capped at 8 views) with safe list and explicit dismiss operations; all other backends (tmux, Zellij, Orca, cmux, unknown) fall through to the existing cleanup path unchanged.bin/fm-teardown.shvia a prepare/commit/rollback pattern that preserves all existing landing, report, focus, endpoint-ownership, and cleanup safety checks; added 314-line behavioral test suite intests/fm-completed-view.test.sh.nathan-coding-loopagent skill for mandatory second-mate code review coordination, and updated docs (docs/configuration.md,docs/herdr-backend.md,docs/verification/runtime-backends.md) with enablement instructions, backend support matrix, and verification pointers.Risk Assessment
✅ Low: The fix commit is a single-line, surgical change (
LC_ALL=C→export LC_ALL=C) that correctly resolves the locale-ordering finding from round 1 with no new logic, no scope changes, and no new risks introduced.Testing
All 3 targeted test suites (fm-completed-view, fm-brief, fm-teardown) pass with 0 failures across 49 behavioral assertions, exercising the feature end-to-end through public shell interfaces with a stateful fake Herdr CLI — no implementation source text was asserted.
Evidence: fm-completed-view targeted test run
Source: fm-completed-view targeted test run
ok - completed-task views default off and an unsupported backend keeps ordinary cleanup ok - opt-in teardown parks only a bounded sanitized summary, retires active monitoring, and supports exact list/dismiss ok - completed-task view retention is hard-bounded without implicit eviction FM_TEST_SUMMARY total=1 failed=0 skipped_gate=0 duration_ms=6391Evidence: All targeted test suites combined
Source: All targeted test suites combined
FM_TEST_SUMMARY total=3 failed=0 skipped_gate=0 duration_ms=97012 FM_TEST_SUMMARY_FAMILY family=backend-dispatch count=1 duration_ms=5888 failed=0 FM_TEST_SUMMARY_FAMILY family=pr-forge count=1 duration_ms=88891 failed=0 FM_TEST_SUMMARY_FAMILY family=pure-contract-unit count=1 duration_ms=1776 failed=0Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 3 issues found → auto-fixed ✅
bin/fm-completed-view-lib.sh:483- Infm_completed_view_list,LC_ALL=Cat line 483 is a plain shell-variable assignment withoutexport. Bash's built-in glob expansion reads locale from the exported environment, so this assignment does not make the"$dir"/*"$FM_COMPLETED_VIEW_RECORD_SUFFIX"glob order deterministic across locales. The list output order is therefore locale-dependent; if stable ordering matters (e.g. for scripted consumers), the assignment should beexport LC_ALL=Cor the glob expansion replaced with a sorted enumeration.bin/fm-completed-view.sh:38-fm_backend_source herdr >/dev/nullin thelistsubcommand (line 38 offm-completed-view.sh) suppresses stdout but does not check the return value. If the herdr adapter is absent, subsequentfm_backend_herdr_pane_presence_statecalls insidefm_completed_view_listwill fail silently per record, producing garbled presence fields without any operator-visible error. Thedismisspath at line 62 has the same pattern but degrades safely because the next call (fm_backend_herdr_presentation_session_lock_path) will fail with an explicit exit 1. Forlist, consider a guard and warning if sourcing fails.bin/fm-teardown.sh:2910- The completed-view registry lock (COMPLETED_VIEW_LOCK) is released at line 2910 offm-teardown.sh, afterMETA_LOCKis released at line 2908. The Herdr session presentation lock (held viaTEARDOWN_HERDR_LOCK_RECORDS) is released only via the EXIT trap (teardown_release_herdr_locks). This means the registry lock is briefly released while the session lock is still held on normal exit, which is the reverse of acquisition order (registry first, then session). Acquisition order is registry → session; release order is META → registry → (trap) session. This reversal is safe because no new acquisition of these locks occurs after teardown completion, but the asymmetry is worth noting for future maintenance.🔧 Fix: export LC_ALL=C for deterministic glob ordering in fm_completed_view_list
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash bin/fm-test-run.sh tests/fm-completed-view.test.sh— 3 behavioral scenarios: default-off/unsupported-backend fallback, opt-in teardown parks sanitized summary with list/dismiss lifecycle, hard retention capbash bin/fm-test-run.sh tests/fm-brief.test.sh— 16 fm-brief contract tests including new Herdr lab contract coveragebash bin/fm-test-run.sh tests/fm-teardown.test.sh— 29 teardown behavioral tests verifying the integration with the new completed-view path and that existing endpoint safety and leaked-process reap behaviors are preserved✅ **Document** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.