Add persistent human names and names-first routing - #6
Conversation
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
codeant-ai
Bot
added
the
size:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| if RESOLVED_ID=$(fm_identity_resolve_selector "$STATE" "$RAW_TARGET"); then | ||
| TARGET_SELECTOR=$RESOLVED_ID | ||
| else | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Suggestion: Use a persistent callsign index for selector resolution, while resolving exact task IDs directly from their metadata; retain the full identity scan only for index recovery or validation. [custom_rule]
Severity Level: Major
Why it matters? ⭐
For non-colon selectors that are not exact task IDs or legacy fm- labels, fm_identity_resolve_selector scans every identity record and reads each record's current and retired callsigns. This makes callsign-based peeks perform work proportional to the number of identity records, so a persistent callsign index would avoid repeated filesystem scans on this commonly used path while preserving the direct metadata lookup paths for exact task IDs.
Rule source 📖
CodeAnt dashboard (rule "agent-persona-performance")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-peek.sh
**Line:** 31:35
**Comment:**
*Custom Rule: Use a persistent callsign index for selector resolution, while resolving exact task IDs directly from their metadata; retain the full identity scan only for index recovery or validation.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if [ -z "$meta" ]; then | ||
| set +e | ||
| id=$(fm_identity_resolve_selector "$STATE" "$raw") | ||
| identity_rc=$? | ||
| set -e | ||
| case "$identity_rc" in | ||
| 0) meta="$STATE/$id.meta" ;; | ||
| *) return 1 ;; | ||
| esac | ||
| else | ||
| resolved_id=$(fm_identity_resolve_selector "$STATE" "$raw") || return 1 | ||
| [ "$resolved_id" = "$id" ] || { | ||
| fm_identity_error "selector '$raw' resolved to task $resolved_id but backend metadata selected task $id; refusing to guess" | ||
| return 1 | ||
| } | ||
| fi | ||
| identity_record=$(fm_identity_task_record "$id") | ||
| identity_status=$(fm_identity_record_value "$identity_record" status 2>/dev/null || true) | ||
| if [ "$identity_status" != active ]; then | ||
| fm_identity_ensure_task_from_meta "$meta" "$id" legacy >/dev/null || return 1 | ||
| fi |
There was a problem hiding this comment.
Suggestion: Extract the identity selector resolution and legacy activation into a dedicated helper that returns the resolved metadata and task id, keeping backend routing separate from identity migration and reducing the nested branching in this resolver. [custom_rule]
Severity Level: Major
Why it matters? ⭐
The suggestion identifies a genuine maintainability concern: fm_send_resolve_target mixes selector resolution, consistency validation, identity migration, and backend routing, with nested branching and shell error-mode manipulation. Extracting the identity-specific portion into a helper would reduce coupling and make the backend-routing path easier to read and change without altering behavior.
Rule source 📖
CodeAnt dashboard (rule "agent-persona-maintainability")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-send.sh
**Line:** 243:263
**Comment:**
*Custom Rule: Extract the identity selector resolution and legacy activation into a dedicated helper that returns the resolved metadata and task id, keeping backend routing separate from identity migration and reducing the nested branching in this resolver.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| (.identities // []) as $identities | ||
| | def callsign_for($id): ([ $identities[] | select(.id == $id) | .callsign ][0] // $id); |
There was a problem hiding this comment.
Suggestion: Build an id-to-callsign lookup object once from identities, then perform constant-time lookups in callsign_for instead of scanning the entire identities array for every landed, in-flight, and report row. [custom_rule]
Severity Level: Major
Why it matters? ⭐
The jq function scans the entire identities array for every callsign lookup. It is invoked while projecting multiple landed, in-flight, and report rows, causing repeated linear work proportional to the number of output rows times the identity count. Building an id-to-callsign object once would make each lookup constant-time and avoid this repeated computation on larger fleets.
Rule source 📖
CodeAnt dashboard (rule "agent-persona-performance")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-bearings-snapshot.sh
**Line:** 322:323
**Comment:**
*Custom Rule: Build an id-to-callsign lookup object once from `identities`, then perform constant-time lookups in `callsign_for` instead of scanning the entire identities array for every landed, in-flight, and report row.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| callsign=$(fm_identity_display_callsign "$id") | ||
| status=legacy | ||
| if fm_identity_record_core_valid "$record" "$id" 2>/dev/null; then | ||
| status=$(fm_identity_record_value "$record" status) | ||
| fi |
There was a problem hiding this comment.
Suggestion: Reuse the callsign and status already resolved while building task rows, and batch identity lookups for backlog-only and report-only ids instead of re-reading each identity record in this per-id shell loop. [custom_rule]
Severity Level: Major
Why it matters? ⭐
The identity projection performs multiple shell/helper operations for every unique backlog, task, or report id, including reading and validating an identity record and separately resolving its callsign. The task-row path already resolves callsigns, while backlog-only and report-only ids are processed in this loop; batching or reusing projections would avoid repeated per-id file parsing and subprocess overhead on commonly sized inventories.
Rule source 📖
CodeAnt dashboard (rule "agent-persona-performance")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-fleet-snapshot.sh
**Line:** 1397:1401
**Comment:**
*Custom Rule: Reuse the callsign and status already resolved while building task rows, and batch identity lookups for backlog-only and report-only ids instead of re-reading each identity record in this per-id shell loop.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| fm_identity_human_name_matches_any() { # <name> [record-to-ignore] | ||
| local wanted ignore=${2:-} record value line | ||
| wanted=$(fm_identity_fold "$1") | ||
| if [ "$FM_IDENTITY_HOME_RECORD" != "$ignore" ] && [ -f "$FM_IDENTITY_HOME_RECORD" ]; then | ||
| while IFS= read -r line || [ -n "$line" ]; do | ||
| case "$line" in name=*|previous_name=*) value=${line#*=} ;; *) continue ;; esac | ||
| [ "$(fm_identity_fold "$value")" = "$wanted" ] && return 0 | ||
| done < "$FM_IDENTITY_HOME_RECORD" | ||
| fi | ||
| for record in "$FM_IDENTITY_DIR"/*.identity; do | ||
| [ -f "$record" ] && [ ! -L "$record" ] || continue | ||
| [ "$record" = "$ignore" ] && continue | ||
| while IFS= read -r line || [ -n "$line" ]; do | ||
| case "$line" in callsign=*|retired_callsign=*) value=${line#*=} ;; *) continue ;; esac | ||
| [ "$(fm_identity_fold "$value")" = "$wanted" ] && return 0 | ||
| done < "$record" | ||
| done | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
Suggestion: Build a single normalized name/index view once per operation instead of rescanning every identity record and refolding every stored value for each candidate callsign. [custom_rule]
Severity Level: Major
Why it matters? ⭐
The helper reads every identity file and refolds every stored name for each lookup. Callsign allocation invokes collision checks repeatedly for candidates, so the same registry history is rescanned multiple times; with a growing task history this creates real repeated I/O and shell-process work. Building one normalized index per operation is a valid leaner alternative while preserving collision semantics.
Rule source 📖
CodeAnt dashboard (rule "agent-persona-performance")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-identity-lib.sh
**Line:** 206:224
**Comment:**
*Custom Rule: Build a single normalized name/index view once per operation instead of rescanning every identity record and refolding every stored value for each candidate callsign.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| schema=$(fm_identity_record_value "$record" schema 2>/dev/null || true) | ||
| home=$(fm_identity_record_value "$record" home 2>/dev/null || true) | ||
| task=$(fm_identity_record_value "$record" task_id 2>/dev/null || true) | ||
| callsign=$(fm_identity_record_value "$record" callsign 2>/dev/null || true) | ||
| status=$(fm_identity_record_value "$record" status 2>/dev/null || true) |
There was a problem hiding this comment.
Suggestion: Parse each identity record once and validate all required fields from that single read, rather than calling the full-file scanner separately for every field. [custom_rule]
Severity Level: Major
Why it matters? ⭐
Each call to fm_identity_record_value opens and scans the entire record, so validating five fields performs five full reads of the same file. This repeated parsing occurs on task operations and registry scans and is a genuine avoidable performance cost as record count and lifecycle activity grow; one read can populate and validate all required fields.
Rule source 📖
CodeAnt dashboard (rule "agent-persona-performance")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-identity-lib.sh
**Line:** 484:488
**Comment:**
*Custom Rule: Parse each identity record once and validate all required fields from that single read, rather than calling the full-file scanner separately for every field.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| case "$raw" in | ||
| fm-*) | ||
| id=${raw#fm-} | ||
| if [ -f "$state/$id.meta" ]; then | ||
| if fm_identity_selector_conflicts_with_other_record "$raw" "$id"; then | ||
| fm_identity_error "selector '$raw' conflicts with another task's current or historical callsign; refusing to guess" | ||
| return 2 | ||
| fi | ||
| record=$(fm_identity_task_record "$id") | ||
| if [ -e "$record" ] || [ -L "$record" ]; then | ||
| fm_identity_exact_task_record_routes "$record" "$state/$id.meta" "$id" || { | ||
| fm_identity_error "task '$id' has conflicting or unsafe callsign identity; refusing to route" | ||
| return 2 | ||
| } | ||
| fi | ||
| printf '%s' "$id" | ||
| return 0 | ||
| fi | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
Suggestion: Extract the shared exact-task resolution and validation block into one helper, parameterizing only the selector spelling, so the direct task-id and fm-prefixed paths cannot drift apart. [custom_rule]
Severity Level: Major
Why it matters? ⭐
The direct task-id branch immediately above and this fm-prefixed branch both perform selector conflict checking, record lookup, exact routing validation, error handling, and return the task id. Maintaining these parallel blocks creates meaningful drift risk when routing rules change; extracting a shared helper is a concrete maintainability improvement.
Rule source 📖
CodeAnt dashboard (rule "agent-persona-maintainability")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-identity-lib.sh
**Line:** 791:810
**Comment:**
*Custom Rule: Extract the shared exact-task resolution and validation block into one helper, parameterizing only the selector spelling, so the direct task-id and fm-prefixed paths cannot drift apart.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| for meta in "$FM_IDENTITY_STATE"/*.meta; do | ||
| [ -f "$meta" ] && [ ! -L "$meta" ] || continue | ||
| id=${meta##*/}; id=${id%.meta} | ||
| fm_identity_ensure_task_from_meta "$meta" "$id" 1 >/dev/null || return 1 | ||
| done | ||
| for status in "$FM_IDENTITY_STATE"/*.status; do | ||
| [ -f "$status" ] && [ ! -L "$status" ] || continue | ||
| id=${status##*/}; id=${id%.status} | ||
| [ -f "$FM_IDENTITY_STATE/$id.meta" ] && continue | ||
| fm_identity_ensure_legacy_archive "$id" >/dev/null || return 1 | ||
| done |
There was a problem hiding this comment.
Suggestion: Refactor the migration loop to acquire the registry lock once and reuse a preloaded identity index while processing all metadata and status files, instead of invoking the full per-task migration path independently. [custom_rule]
Severity Level: Major
Why it matters? ⭐
Migration invokes a full per-task path for every metadata and status file, and those paths independently acquire and release the registry lock and rescan identity files for name collisions. For homes with many tasks this causes repeated filesystem scans and lock round trips on a commonly executed migration path. A batch-level lock and preloaded index can reduce that measurable repeated work while retaining the same validation and write semantics.
Rule source 📖
CodeAnt dashboard (rule "agent-persona-performance")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-identity-lib.sh
**Line:** 941:951
**Comment:**
*Custom Rule: Refactor the migration loop to acquire the registry lock once and reuse a preloaded identity index while processing all metadata and status files, instead of invoking the full per-task migration path independently.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| ;; | ||
| resolve) | ||
| [ "$#" -eq 1 ] || { usage >&2; exit 2; } | ||
| id=$(fm_identity_resolve_selector "$STATE" "$1") |
There was a problem hiding this comment.
Suggestion: Use a maintained callsign-to-task index, or otherwise avoid rescanning every identity record for each resolution; the current resolver performs a full registry scan with repeated record parsing, external grep/sed calls, and per-character folding. [custom_rule]
Severity Level: Major
Why it matters? ⭐
The called resolver scans every identity record when the selector is not a direct task id, validates and parses each record, invokes external grep and sed operations, and folds strings character by character for comparisons. This work grows with the number of retained identity records and is repeated for each resolution or rename, creating a real performance cost on commonly used paths; an index or consolidated lookup would avoid the repeated full-registry scan.
Rule source 📖
CodeAnt dashboard (rule "agent-persona-performance")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-name.sh
**Line:** 62:62
**Comment:**
*Custom Rule: Use a maintained callsign-to-task index, or otherwise avoid rescanning every identity record for each resolution; the current resolver performs a full registry scan with repeated record parsing, external `grep`/`sed` calls, and per-character folding.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if [ ! -f "$(fm_identity_task_record "$id")" ]; then | ||
| if [ -f "$meta" ]; then | ||
| CALLSIGN=$(fm_identity_ensure_task_from_meta "$meta" "$id" legacy) || { | ||
| fm_lock_release "$registry_lock" || true | ||
| fm_lock_release "$SPAWN_TASK_LOCK" || true | ||
| return 1 | ||
| } | ||
| else | ||
| CALLSIGN=$(fm_identity_reserve_fresh_task "$id") || { | ||
| fm_lock_release "$registry_lock" || true | ||
| fm_lock_release "$SPAWN_TASK_LOCK" || true | ||
| return 1 | ||
| } | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Suggestion: Extract the repeated identity-reservation failure cleanup into a single helper that releases both locks and returns the failure status, so future lock changes cannot diverge between the legacy-metadata and fresh-reservation branches. [custom_rule]
Severity Level: Major
Why it matters? ⭐
The legacy-metadata and fresh-reservation branches contain identical lock-release and failure-return blocks. This is genuine maintainability duplication in lock-management code: a future change to the required cleanup could be applied to one branch but not the other, so a shared helper would reduce divergence risk without changing behavior.
Rule source 📖
CodeAnt dashboard (rule "agent-persona-maintainability")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-spawn.sh
**Line:** 529:543
**Comment:**
*Custom Rule: Extract the repeated identity-reservation failure cleanup into a single helper that releases both locks and returns the failure status, so future lock changes cannot diverge between the legacy-metadata and fresh-reservation branches.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| CALLSIGN= | ||
| IDENTITY_FRESH_RESERVED=0 | ||
| IDENTITY_REBIND_ALLOWED=0 | ||
| if [ "$RELAUNCH" -eq 0 ]; then | ||
| if [ "$KIND" = secondmate ] && [ -f "$STATE/$ID.meta" ]; then | ||
| # Bootstrap's established secondmate recovery path predates --relaunch and | ||
| # intentionally invokes an ordinary --secondmate spawn against the durable | ||
| # task record. It is a continuation, not a fresh task: validate/adopt the | ||
| # existing binding now and publish its replacement endpoint below. | ||
| CALLSIGN=$(fm_identity_ensure_task_from_meta "$STATE/$ID.meta" "$ID" 1) || exit 1 | ||
| IDENTITY_REBIND_ALLOWED=1 | ||
| else | ||
| CALLSIGN=$(fm_identity_reserve_fresh_task "$ID") || exit 1 | ||
| IDENTITY_FRESH_RESERVED=1 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Suggestion: Move the callsign lifecycle branching into a dedicated identity-publication helper and use named mode values consistently instead of coordinating behavior through multiple global flags and the numeric 1 alias. [custom_rule]
Severity Level: Major
Why it matters? ⭐
The identity lifecycle is coordinated through multiple global flags that are later interpreted in a separate publication branch, and the call uses the unexplained numeric mode value 1 while other calls use named values such as legacy and rebind. This creates avoidable coupling and makes the state transitions harder to understand or safely modify, so a dedicated helper with named modes is a meaningful maintainability improvement.
Rule source 📖
CodeAnt dashboard (rule "agent-persona-maintainability")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-spawn.sh
**Line:** 1040:1055
**Comment:**
*Custom Rule: Move the callsign lifecycle branching into a dedicated identity-publication helper and use named mode values consistently instead of coordinating behavior through multiple global flags and the numeric `1` alias.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| write_tmux_meta task-a fm-home-restarted:fm-task-a thread-a | ||
| [ "$(fm_identity_ensure_task_from_meta "$STATE/task-a.meta" task-a rebind)" = "$EXPLICIT_A" ] \ | ||
| || fail "tmux restart changed the callsign" | ||
| [ "$(field "$REC_A" endpoint)" = fm-home-restarted:fm-task-a ] || fail "relaunch endpoint was not updated" | ||
| [ "$(field "$REC_A" harness_session_id)" = thread-a ] || fail "exact-thread resume id was not preserved" | ||
| write_herdr_meta task-b fm-lab:pane-b-restarted thread-b | ||
| [ "$(fm_identity_ensure_task_from_meta "$STATE/task-b.meta" task-b rebind)" = "$CALL_B" ] \ | ||
| || fail "Herdr restart changed the callsign" | ||
| [ "$(field "$REC_B" endpoint)" = fm-lab:pane-b-restarted ] || fail "Herdr replacement endpoint was not updated" | ||
| [ "$(field "$REC_B" harness_session_id)" = thread-b ] || fail "Herdr exact-thread id was not recorded" |
There was a problem hiding this comment.
Suggestion: Extract the repeated restart/rebind assertions into a parameterized helper that accepts the metadata writer, task id, expected callsign, and record fields, then use it for both tmux and Herdr cases. [custom_rule]
Severity Level: Major
Why it matters? ⭐
The test contains two backend-specific restart/rebind sequences with the same structure: write metadata, rebind the task, verify the callsign, and verify endpoint and harness-session fields. Extracting that shared workflow into a parameterized helper would reduce duplicated test logic and make future backend coverage less error-prone while retaining backend-specific inputs.
Rule source 📖
CodeAnt dashboard (rule "agent-persona-maintainability")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/fm-identity.test.sh
**Line:** 138:147
**Comment:**
*Custom Rule: Extract the repeated restart/rebind assertions into a parameterized helper that accepts the metadata writer, task id, expected callsign, and record fields, then use it for both tmux and Herdr cases.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| RAW_SELECTOR=${1:-} | ||
| [ -n "$RAW_SELECTOR" ] || { echo "usage: fm-crew-state.sh <task-id-or-callsign>" >&2; exit 2; } | ||
| set +e | ||
| ID=$(fm_identity_resolve_selector "$STATE" "$RAW_SELECTOR") |
There was a problem hiding this comment.
Suggestion: Use an indexed callsign-to-task lookup or a resolver that returns the callsign together with the task id, rather than scanning and validating the entire identity registry on every state read. [custom_rule]
Severity Level: Major
Why it matters? ⭐
For callsign selectors, fm_identity_resolve_selector iterates over every identity record and validates each candidate, and even exact task-id resolution performs a registry-wide conflict scan. This helper is used on every state read, so repeated polling can cause avoidable filesystem scans and shell processing as the number of retained identity records grows. An indexed lookup or a resolver that returns the resolved identity in one operation would reduce that repeated work while preserving the ambiguity and historical-name checks.
Rule source 📖
CodeAnt dashboard (rule "agent-persona-performance")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-crew-state.sh
**Line:** 83:83
**Comment:**
*Custom Rule: Use an indexed callsign-to-task lookup or a resolver that returns the callsign together with the task id, rather than scanning and validating the entire identity registry on every state read.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| print_agents_refresh_if_required "$REBUILDING_SESSION_PID" | ||
|
|
||
| if [ "$READ_ONLY" -eq 0 ]; then | ||
| if ! fm_identity_migrate_home; then |
There was a problem hiding this comment.
Suggestion: Guard this migration behind a durable migration marker or make it skip already-valid records so normal session starts do not reacquire the identity lock and process every task. [custom_rule]
Severity Level: Major
Why it matters? ⭐
fm_identity_migrate_home performs a home check and then scans every state/.meta and state/.status file, ensuring or archiving each identity record. Calling it on every normal writable session start repeats filesystem scans and identity-lock work even after records have already been migrated, which is a real performance cost on homes with many tasks. A durable migration marker or a no-op path for already-valid records would avoid that repeated work.
Rule source 📖
CodeAnt dashboard (rule "agent-persona-performance")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-session-start.sh
**Line:** 648:648
**Comment:**
*Custom Rule: Guard this migration behind a durable migration marker or make it skip already-valid records so normal session starts do not reacquire the identity lock and process every task.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| callsign=$(fm_identity_display_callsign "$id") | ||
| printf '\n--- %s (%s) ---\n' "$callsign" "$id" |
There was a problem hiding this comment.
Suggestion: Build a single callsign lookup from the identity directory before iterating over metadata, or cache each task's resolved callsign, instead of validating and rereading the identity record for every task individually. [custom_rule]
Severity Level: Major
Why it matters? ⭐
fm_identity_display_callsign validates and reads the task's identity record for each metadata entry, so a startup with many tasks performs a separate set of record filesystem reads per task. Caching already-resolved callsigns or loading the identity directory once would reduce repeated startup I/O. The impact is workload-dependent, but it is a genuine repeated per-task filesystem operation on this commonly executed startup path.
Rule source 📖
CodeAnt dashboard (rule "agent-persona-performance")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-session-start.sh
**Line:** 810:811
**Comment:**
*Custom Rule: Build a single callsign lookup from the identity directory before iterating over metadata, or cache each task's resolved callsign, instead of validating and rereading the identity record for every task individually.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| @@ -2439,6 +2441,15 @@ fi | |||
| # pruned code root. Best effort - a sweep failure never blocks this teardown. | |||
| "$SCRIPT_DIR/fm-remote-job-reap-orphans.sh" >&2 || true | |||
There was a problem hiding this comment.
Suggestion: Do not run a machine-wide process scan during every task teardown; move orphan-worker cleanup to a periodic/explicit maintenance path or gate it behind evidence that remote job workers are configured or present. [custom_rule]
Severity Level: Major
Why it matters? ⭐
The invoked script unconditionally scans every process owned by the account using ps, even when no remote job workers exist. Running this machine-wide process enumeration on every teardown creates avoidable work on a commonly executed lifecycle path, so moving it to periodic maintenance or gating it on relevant worker configuration or evidence is a genuine performance improvement.
Rule source 📖
CodeAnt dashboard (rule "agent-persona-performance")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-teardown.sh
**Line:** 2442:2442
**Comment:**
*Custom Rule: Do not run a machine-wide process scan during every task teardown; move orphan-worker cleanup to a periodic/explicit maintenance path or gate it behind evidence that remote job workers are configured or present.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| [ -n "$callsign" ] && assert_contains "$out" "$callsign ($id)" \ | ||
| "spawn result was not names-first" |
There was a problem hiding this comment.
Suggestion: Assert that callsign is non-empty separately, then perform the names-first output assertion unconditionally so this regression test cannot silently pass when no callsign is persisted. [custom_rule]
Severity Level: Major
Why it matters? ⭐
The test conditionally performs the output assertion only when callsign is non-empty. If callsign persistence fails, the condition returns false but the test can continue successfully because the command is part of an AND list and its failure is not explicitly asserted. This weakens the regression test and is a meaningful maintainability issue because the intended callsign contract is not enforced in one clear, unconditional assertion.
Rule source 📖
CodeAnt dashboard (rule "agent-persona-maintainability")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/fm-spawn-pool-base-freshen.test.sh
**Line:** 104:105
**Comment:**
*Custom Rule: Assert that `callsign` is non-empty separately, then perform the names-first output assertion unconditionally so this regression test cannot silently pass when no callsign is persisted.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| ARCHIVED_CALLSIGN=$(fm_identity_ensure_task_from_meta "$META" "$ID" 1) || { | ||
| echo "error: task $ID's persistent callsign could not be validated; nothing was changed" >&2 | ||
| exit 1 | ||
| } |
There was a problem hiding this comment.
Suggestion: The remote-secondmate teardown path returns before reaching this identity archival call, so it removes the remote task metadata while leaving the local callsign record in the active state. Subsequent history reports the callsign as active and routing treats it as archived-only because its meta file is gone. Archive the identity as part of the remote teardown path before removing the metadata, or move the archival operation into shared teardown finalization. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Remote secondmate callsigns remain active after teardown.
- ⚠️ Names-first history can show stale live-task state.
- ⚠️ Deleted remote routes leave durable identity records inconsistent.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-teardown.sh
**Line:** 2449:2452
**Comment:**
*Incomplete Implementation: The remote-secondmate teardown path returns before reaching this identity archival call, so it removes the remote task metadata while leaving the local callsign record in the active state. Subsequent history reports the callsign as active and routing treats it as archived-only because its meta file is gone. Archive the identity as part of the remote teardown path before removing the metadata, or move the archival operation into shared teardown finalization.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| history) | ||
| [ "$#" -eq 0 ] || { usage >&2; exit 2; } | ||
| fm_identity_history | while IFS=$'\t' read -r callsign id status; do | ||
| if [ "$callsign" = UNSAFE ]; then | ||
| printf 'UNSAFE (%s) %s\n' "$id" "$status" | ||
| else | ||
| printf '%s (%s) %s\n' "$callsign" "$id" "$status" | ||
| fi | ||
| done |
There was a problem hiding this comment.
Suggestion: The history command only reads already-created identity records and never migrates legacy .meta and status-only task records. On an existing Firstmate home, invoking fm-name.sh history immediately after this upgrade therefore omits historical tasks until an unrelated session-start migration happens, contradicting the command's purpose of displaying persistent identity history. Run the identity migration before listing history, or make the command explicitly include legacy records. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ `fm-name.sh history` omits identities on upgraded homes.
- ⚠️ Status-only historical tasks remain undiscoverable.
- ⚠️ Users must run session startup before viewing complete history.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-name.sh
**Line:** 66:74
**Comment:**
*Incomplete Implementation: The history command only reads already-created identity records and never migrates legacy `.meta` and status-only task records. On an existing Firstmate home, invoking `fm-name.sh history` immediately after this upgrade therefore omits historical tasks until an unrelated session-start migration happens, contradicting the command's purpose of displaying persistent identity history. Run the identity migration before listing history, or make the command explicitly include legacy records.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| if [ -z "$meta" ]; then | ||
| set +e | ||
| id=$(fm_identity_resolve_selector "$STATE" "$raw") | ||
| identity_rc=$? | ||
| set -e | ||
| case "$identity_rc" in | ||
| 0) meta="$STATE/$id.meta" ;; | ||
| *) return 1 ;; | ||
| esac | ||
| else |
There was a problem hiding this comment.
Suggestion: The new selector resolution returns immediately when the raw target is not a callsign or task selector. This prevents the existing fm_backend_meta_for_window fallback below from handling valid bare recorded window or terminal names, so commands that previously sent to a metadata-owned bare endpoint now fail with an unresolved-selector error. Preserve the backend window/terminal lookup as a fallback when identity resolution reports that the target is not an identity selector. [api mismatch]
Severity Level: Major ⚠️
- ❌ Bare recorded window sends fail before backend dispatch.
- ⚠️ Orca terminal-name compatibility is bypassed.
- ⚠️ Existing explicit endpoint fallback becomes unreachable.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-send.sh
**Line:** 243:252
**Comment:**
*Api Mismatch: The new selector resolution returns immediately when the raw target is not a callsign or task selector. This prevents the existing `fm_backend_meta_for_window` fallback below from handling valid bare recorded window or terminal names, so commands that previously sent to a metadata-owned bare endpoint now fail with an unresolved-selector error. Preserve the backend window/terminal lookup as a fallback when identity resolution reports that the target is not an identity selector.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if ! fm_identity_migrate_home; then | ||
| printf 'IDENTITY: persistent home/task name migration failed; names-first routing will refuse unsafe records until repaired.\n' >&2 | ||
| fi |
There was a problem hiding this comment.
Suggestion: The migration scans state/*.meta without taking the per-task metadata or spawn lifecycle lock. A fresh spawn can publish its metadata after reserving a provisioning identity but before calling fm_identity_activate_reserved_task_from_meta; migration can then see the complete metadata and convert that reservation to active, causing the activation call to reject the task because it is no longer provisioning. This can make an otherwise valid concurrent spawn fail and leave the task partially provisioned. Run migration under the same task/lifecycle locks or make reservation activation tolerate and verify this exact concurrent transition. [race condition]
Severity Level: Major ⚠️
- ❌ Concurrent fresh spawns can fail during identity activation.
- ⚠️ Endpoint metadata may remain after spawn failure.
- ⚠️ Session startup can race task lifecycle publication.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bin/fm-session-start.sh
**Line:** 648:650
**Comment:**
*Race Condition: The migration scans `state/*.meta` without taking the per-task metadata or spawn lifecycle lock. A fresh spawn can publish its metadata after reserving a `provisioning` identity but before calling `fm_identity_activate_reserved_task_from_meta`; migration can then see the complete metadata and convert that reservation to `active`, causing the activation call to reject the task because it is no longer `provisioning`. This can make an otherwise valid concurrent spawn fail and leave the task partially provisioned. Run migration under the same task/lifecycle locks or make reservation activation tolerate and verify this exact concurrent transition.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix* Add persistent human callsigns * Close persistent callsign safety gaps --------- Co-authored-by: wenkxu <v-wenkxu@expediagroup.com>
* fix(composer): stop a blocked pi pane from proving an empty composer (kunchenguid#2811) A pi worker parked on an interactive prompt - a permission dialog, a question menu, a trust dialog - reports agent_status=blocked, because it is waiting on a human keystroke. Pi draws that menu above its separator pair, so the composer region between the rules is blank and structure alone looks like a free composer. _fm_composer_pi_verdict admitted blocked alongside idle and done, so the shared classifier reported an affirmatively empty composer for exactly the pane where typing is unsafe. Every "is it safe to type here?" consumer reads that verdict and proceeds only on an affirmative empty, so both are told yes on a parked prompt: the away-mode injection guard in bin/fm-supervise-daemon.sh, and fm-send's pre-type refusal. The keys then answer the menu instead of composing a message - the highlighted default is selected, the text is discarded, and the record attributes a decision to a human who never made it. blocked now defers to unknown, which every consumer already treats as fail-closed. idle and done still prove an empty composer, so ordinary steering is unchanged, and Cursor is unaffected because its always-blocked panes never reach this pi-only branch. Regression coverage lands first at both levels: the verdict owner (a blocked pi defers) and the herdr adapter (a parked pi prompt is not an empty composer). * fix(bin): require project clone roots during fleet sync (kunchenguid#2849) * fix(bin): require a clone root before fleet-sync touches a project Git repository discovery walks upward, so `git -C projects/<dir>` on a plain directory nested under projects/ resolves to the enclosing repository - in a firstmate home, the firstmate checkout itself. fm-fleet-sync.sh guarded its candidates with `rev-parse --is-inside-work-tree`, which such a directory passes, so every later git call read, pruned and fast-forwarded firstmate's own default branch and reported it under the project directory's label. A running session's AGENTS.md changed underneath it, and the report named a project that had nothing to do with the change. Require each candidate to be the root of its own work tree before any other git command: compare `rev-parse --show-toplevel` against the directory's own physical path. Both sides are physical, so a symlinked clone still compares equal. Anything else is skipped by name, naming the repository that would have been touched, and bootstrap relays that as a FLEET_SYNC line. Regression coverage reproduces the wrong-repo fast-forward against a home nested inside another repository, in both the whole-fleet and single-project forms, and pins that a symlinked clone dir still syncs. * no-mistakes(review): Keep enclosing fixture clean during clone-root regression * fix(bin): retry transient Lavish poll interruptions (kunchenguid#2846) * fix(procevent): retry a transient Lavish poll interruption quietly A live Lavish listener can be cut short by the server with exactly error: Lavish Editor poll response was interrupted code: SERVER_ERROR while the session's marks remain available. Firstmate registered raw `lavish-axi poll` output, so the generic process-event runner captured that transient response as a result and woke the whole fleet over what is really an internal retry. The Lavish adapter now registers its own listener command, which reruns the published blocking poll up to 12 times at 5 second intervals for that one exact two-line response. The match is deliberately narrow: real feedback, ended and missing sessions, any other SERVER_ERROR, and the same interruption still standing once the bound is spent all pass straight through and are captured and announced as before. The retry is a Lavish fact, so the generic runner stays adapter-agnostic. `FM_LAVISH_POLL_RETRY_DELAY` is a bounded 0 to 60 second override for the interval only, refused rather than rounded when malformed, so a test can exercise the real bound without waiting it out. * no-mistakes(review): Harden Lavish retry matching, validation, and cleanup * no-mistakes(review): Bound Lavish retry staging and stabilize regression * no-mistakes(document): docs: explain Lavish retry adoption * no-mistakes(lint): Restore Lavish trap ShellCheck suppression * fix(brief): stop the documented {TASK} fill from corrupting the Herdr gate (kunchenguid#2838) The unguarded Herdr declaration quoted `{TASK}` in its own prose while the scaffold instructs firstmate to replace every `{TASK}` placeholder. The documented global replace therefore spliced the whole task body into the middle of the safety gate's sentence, silently destroying the one contract that exists precisely because the scaffold cannot inspect the task text. Reword the gate to refer to the task text filled in above, leaving the placeholder only at its genuine fill site. Rewording rather than renaming the token keeps the unfilled-charter guards in fm-home-seed.sh and fm-remote-home-seed.sh working unchanged. Add a regression test that performs the documented global fill on ship and scout scaffolds and asserts the body lands once and the gate survives. * fix(bin): resolve the busy-state lock mtime with the platform's own stat form (kunchenguid#2837) The writer lock's stale-lock branch read the lock's mtime with `stat -f %m ... || stat -c %Y ...`. On GNU coreutils `-f` is filesystem stat, so it consumed the format string as a path, complained on stderr, printed a partial filesystem dump (" File: ...") on stdout, and still exited 0. The GNU form in the fallback therefore never ran, and the following arithmetic evaluated the word `File`, aborting the writer under `set -u` with "File: unbound variable". fm-teardown.sh died there after returning the worktree, leaving state/<id>.meta, .status, .busy-gen, .busy-state, .busy-state.lock/ and .turn-ended behind. The surviving metadata kept the watcher monitoring an endpoint whose agent was gone, so a finished task produced stale wakes forever, and every re-run died identically because the abandoned lock was never broken. Detect the platform once and pick the right stat form, the pattern bin/fm-watch.sh already documents, and treat any non-numeric result as "just created" so a future portability surprise degrades to a lock-timeout refusal rather than killing teardown mid-way. * fix(stow): add opt-in pass horizon for memory decay (kunchenguid#2850) * fix(stow): give memory decay a per-pass horizon so the clock fires The tiered decay clocks were wall-clock only, while admission is per-pass: each /stow admits the findings that pass produced. In a home that stows daily those two rates diverge by the stow cadence, an entry the fleet keeps exercising never reaches 30 days unreinforced, and memory only grows while the pass reports decay evaluated. Give each dated marker an optional unreinforced-pass counter and make both tiers stale at whichever horizon comes first: 10 passes or 30 days for aging, 3 passes or 7 days for perishable. Reinforcement clears the counter and nothing else does, so the existing evidence-based restamp rule stays the only way an entry renews its lease. An absent /N means zero, so entries that stay exercised carry no extra marker bytes, and a rarely stowed home keeps its current behaviour through the unchanged date horizon. * no-mistakes(document): Align stow workflow with dual decay clocks * fix(stow): make the per-pass decay horizon opt-in The unreinforced-pass horizon shipped as a new default archival cadence, which is a product default rather than a restoration of the existing wall-clock contract. Keep the 30-day and 7-day horizons as the only default clock, and put the 10-pass and 3-pass horizons behind an explicit opt-in: config/stow-pass-horizon for the firstmate home, and the file's own header pointer for the public skill. With the opt-in absent no counter is written and no counter is read, so a home that does not ask for it decays exactly as it does today. * no-mistakes(review): Preserve frozen counters and correct archive provenance * test(watcher): stop fixture confirmation budgets racing real child startup (kunchenguid#2876) tests/fm-watcher-lock.test.sh passed in isolation but failed intermittently under full-suite and ambient concurrent load. bin/fm-watch-arm.sh computes its confirmation deadline immediately after forking the real child watcher, so the child's entire fork, exec, lock acquisition and beacon publication has to land inside that wall clock. Two cases shrank that budget to one second, leaving a two-second window for work measured at 3.1-4.9s under CPU oversubscription, so the arm honestly reported "FAILED - no live watcher with a fresh beacon" and their premises collapsed. A third case ran on the production budget, but its child must also execute a registered check before exiting: measured at 1.9-2.3s idle and 9.1-13.1s under load, against an 11s budget. The two cases that must confirm a real child now hold the arm to production's own budget instead of a shrunken fixture one, the immediate-wake case gets an explicit budget with headroom over its measured loaded cost, and the two waits for the arm's typed failure are sized off the largest production default rather than a fixed eight seconds. No bin/ change and no default behavior change: the lock's fail-closed semantics, SIGSTOP handling, stale-heartbeat detection and the arm's typed failures are untouched. Verified 4/4 green at 3x CPU oversubscription (loadavg 75-80) after 3/3 red before the change, and CONTRIBUTING.md records the convention. * fix(bin): deterministically order remote tool paths (kunchenguid#2870) * fix(bin): order discovered tool installs by the shell's own expansion fm_remote_job_compose_operator_path built the asdf and mise install directories with `compgen -G`, which does not sort. Bash sorts glob matches in pathexp.c, on the shell's own pathname-expansion path only; `compgen -G` reaches the same glob_filename through pcomplete.c, which sorts nothing. On bash 3.2 (macOS /bin/bash) and every bash before 5.3 that handed the composition raw readdir order, so which install of a multi-version tool a remote job resolved was decided by directory order on disk rather than by this composition. Expand the globs at the call sites and let the function take the matches, so the composition and the documented portable-PATH contract are the same operation. Quoting the account home at the call site also stops a home whose name contains glob metacharacters from being reinterpreted. The colocated regression pins both the order and the mechanism: bash 5.3 moved sorting into the glob library, so an order-only assertion cannot see the defect there. * no-mistakes(review): Remove source-reading PATH regression guard * fix(bin): prevent routed secondmate work from stranding (kunchenguid#2848) * fix: surface stalled secondmate queues and wake handoffs * no-mistakes(review): Make handoff wakes retryable and stall alerts crash-safe * no-mistakes(review): Prevent duplicate handoff wakes and cover remote delivery * no-mistakes(review): Serialize local handoffs and preserve pre-move wake intent * no-mistakes(review): Serialize teardown with handoffs and retain remote wake confirmation * no-mistakes(review): Reconcile correlated handoff wake delivery after crashes * no-mistakes(review): Keep failed wakes retryable and isolate stall receipts * no-mistakes(review): Reset known-undelivered wake attempts for durable retries * no-mistakes(review): Refuse duplicate sends for unresolved delivery attempts * no-mistakes(review): Atomically restore retryability after reconciled send failures * no-mistakes(review): Serialize delivery confirmation with reconciliation * no-mistakes(document): Document routed wake and stall supervision * no-mistakes(lint): Fix ShellCheck expansion and subshell warnings * no-mistakes: apply CI fixes * no-mistakes: apply CI fixes * no-mistakes(review): Retire stale wake state and defer pre-move wakes * no-mistakes(review): Secure markers, bind batches, and preserve teardown routes * no-mistakes(review): Preserve unresolved prepared wakes across unrelated handoffs * no-mistakes(review): Preserve prepared wakes before unrelated moving handoffs * no-mistakes(document): Document prepared wake batch ownership * no-mistakes: apply CI fixes * no-mistakes: apply CI fixes * no-mistakes(review): Make local wake retirement recoverable * no-mistakes(document): Clarify handoff recovery and teardown documentation * fix: make macOS inbox test path portable (kunchenguid#2857) * feat(bin): deliver local steers through durable task inboxes (kunchenguid#2856) * feat(bin): steer local tasks by durable inbox record plus constant doorbell Stage 1 (local steers) of the captain-adopted reframe in data/fm-send-reliability-reframe-s1/report.md: an ordinary fm-send text steer to a task recorded in this home is appended as a sequenced durable record under state/<id>.inbox/ and the terminal receives only one constant self-describing doorbell line, best-effort. The worker acknowledges by moving the record into handled/; the watcher re-rings an unacknowledged message on an idle pane and escalates once as an ordinary stale wake. --resolve-key closes decisions at enqueue time, because the durable enqueue IS delivery to the task's record. bin/fm-task-inbox-lib.sh owns the record format, doorbell line, and re-ring ladder. The typed plane remains for what must reach the terminal itself: lifecycle keys, harness-native slash and codex $-skill invocations, explicit backend targets, and the remote secondmate leg (unchanged until the remote inbox leg ships separately). The composer classifier is demoted from delivery proof to an advisory ring guard that skips only on a proven pending verdict. Verified live against claude, codex, opencode, pi, grok, and muse: each real worker read its record, acted, and acked with the mv (docs/verification/runtime-backends.md "Steering-inbox doorbell"). * docs(verification): flag the grok 1.0.5 composer-matrix staleness observed by the doorbell run * test(captain-hold): read the chat-channel answer from the durable inbox record * test: migrate fm-control's marker contrast to the inbox record and fix macOS wc padding in the tool-update suite * no-mistakes(review): Harden inbox locking, teardown races, and acknowledgements * no-mistakes(review): Serialize watcher actions with inbox acknowledgements * no-mistakes(review): Bound metadata locking and tighten acknowledgement rechecks * no-mistakes(review): Preserve exact inbox bytes and harden delivery recovery * no-mistakes(review): Harden watcher bookkeeping against concurrent inbox teardown * no-mistakes(document): Update inbox and typed-plane documentation * no-mistakes: apply CI fixes * no-mistakes: apply CI fixes * revert(pipeline): keep parser-native secondmate marking and the both-failed exit out of stage 1 The CI monitor's fix changed the secondmate marking contract for parser-native invocations (appending the marker after the text) and softened the both-commit-and-marker-failed branch to exit 0. The merge authority ruled the marking question out of scope for this stage-1 transport PR (follow-up: fm-send-secondmate-harness-invocation-r1) and ruled the both-failed case a loud nonzero local failure. Restore both, keeping the monitor's legitimate migrations and hardening. * no-mistakes(document): Document inbox and typed-plane boundaries * no-mistakes(document): Scope backend transport docs to typed plane * no-mistakes(document): Clarify inbox attempt-budget documentation * no-mistakes: apply CI fixes * fix(send): the durable record alone governs the inbox exit status Captain-refined ruling on the F2/Greptile finding: the durable inbox record is what delivers the steer, so pending-reply bookkeeping trouble after a successful enqueue never exits nonzero - a resend-inviting status would make automated callers enqueue the delivered instruction again under a new sequence. With the recovery marker stored the watcher reconciles silently; with the commit and marker both lost the send surfaces a distinct reply-tracking-degraded do-not-resend warning and still exits 0. Nonzero remains only where nothing was delivered (or a decision close needs its manual command). Regression: record durable + both bookkeeping writes lost -> exit 0, one record, no duplicate. * no-mistakes(review): Preserve inbox ordering with drain-all doorbells * no-mistakes(review): Surface unwritable inbox ladder bookkeeping * no-mistakes(review): Silence ladder failures after inbox acknowledgement * no-mistakes(document): Update steering inbox documentation * no-mistakes: apply CI fixes * feat(bin): add fast local lint mode (kunchenguid#2891) * feat: add fast local lint mode * fix: preserve complete fm-lint help * fix: isolate fast lint mode * no-mistakes(document): Clarify lint mode documentation ownership * no-mistakes: apply CI fixes * feat(bin): deliver remote steers through durable inboxes (kunchenguid#2901) * feat(bin): deliver remote secondmate steers through durable task inboxes Stage 2 of the inbox+doorbell steer channel (stage 1: kunchenguid#2856). A remote secondmate steer now crosses fm-on.sh as a durable record written idempotently into the remote home's steering inbox plus a best-effort remote doorbell, and the last typed-payload steer transport is deleted: - fm-remote-secondmate-control.sh cmd_send writes the record via the new fm_task_inbox_write_idempotent and rings the doorbell; it no longer types the payload through an inner fm-send at an explicit pane target. - fm-send.sh routes every remote text steer (harness-native included, which marking already reduced to chat) onto the remote inbox leg, retries the identical leg once on ssh 255, closes --resolve-key decisions at enqueue for remote too, and preserves a marked request's reply expectation when completion stays unknown. The exit-3-as- delivered remap, the 255 do-not-resend trap, and the remote typed submit block are removed. - fm-task-inbox-lib.sh owns the idempotent enqueue: an exact-body re-run lands on the existing record, handled or not, so an ambiguous transport can always be safely re-run. - Tests pin the new contract end to end (record + doorbell + no typed payload across ssh, one-record idempotence under an ambiguous transport, enqueue-time decision close, loud real failures, and the deleted typed-payload behaviors gone), and AGENTS.md plus docs/remote-secondmates.md describe the remote leg's new semantics. * no-mistakes(review): Harden remote inbox delivery against lifecycle races * no-mistakes(review): Enable correlation-preserving remote steer resends * no-mistakes(review): Fail closed on stale correlation resends * no-mistakes(review): Include home context in remote resend commands * no-mistakes(review): Lock and revalidate remote parent routes * no-mistakes(document): Clarify remote steer retry documentation * no-mistakes: apply CI fixes * feat: add persistent Pi supervision branch (kunchenguid#2858) * wip: forked supervision on Pi (checkpoint before docs) * fix(pi-branch): harden mirror delivery, fallback encoding, and session replacement Peek-then-shift mirror flush so a failed append retries instead of dropping; durable mirror cursor commits only after delivery into the branch; the main fallback wake is operational-encoded like every watcher injection; session_shutdown quiesces the generation and session_start re-arms, so /new and /resume no longer kill the branch permanently. Registers the extension in the strict typecheck, adds the dispatch handshake test, the branch extension suite, the bash-level regression suite, the session-start replay test, and the opt-in real-SDK live guard. * test(fixtures): carry the branch-dispatch lib and lease lib into isolated fixtures The watcher extension now imports lib/fm-branch-dispatch.ts and fm-teardown sources fm-lease-lib.sh, so every fixture that copies or symlinks those files in isolation gains the new sibling. * no-mistakes(review): Prevent shutdown wake loss and serialize lease claims * no-mistakes(review): Durably hand off wakes and retain portable leases * no-mistakes(review): Require durable reports and clear disposed branch leases * no-mistakes(review): Enforce per-wake outcomes and quiescent lease cleanup * no-mistakes(review): Require wake acknowledgements and tighten branch lifecycle boundaries * no-mistakes(review): Require complete acknowledgements and replay cleanup failures * no-mistakes(review): Bind supervision to lock ownership and durable delivery * no-mistakes(review): Activate branch lazily after session lock acquisition * no-mistakes(review): Preserve undelivered mirror context across extension rebinds * no-mistakes(review): Acknowledge startup replay only after main delivery * no-mistakes(review): Isolate replay metadata from untrusted digest content * no-mistakes(review): Reject duplicate reports for active wake sequences * no-mistakes(review): Retain failed fallbacks and deduplicate outcome replay * no-mistakes(review): Deduplicate durable outcomes and cache delivery receipts * no-mistakes(review): Anchor wake sequence matching to outcome fields * no-mistakes(document): Clarify Pi supervision durability contracts * no-mistakes(lint): Fix ShellCheck issues in branch supervision scripts * no-mistakes: apply CI fixes * no-mistakes: apply CI fixes * no-mistakes: apply CI fixes * no-mistakes: apply CI fixes * no-mistakes: apply CI fixes * refactor(pi-branch): collapse to confused-agent-grade guards per captain decision Captain decision A: the lease/actor guards target the CONFUSED-AGENT threat model bin/fm-gate-refuse-lib.sh already documents; adversarial-grade separation is impossible in the shared-process design and is filed as separate follow-up work. Rip out the machinery that chased it: the generation fence and shell-provenance markers, the wrapper-tagged ancestry walks, guard auto-claim with per-script release traps, the pending-wake files and ack-receipt correlation (the durable wake queue already re-presents anything unacknowledged), the delivery-receipt store with contiguous cursor advancement, the session-start replay-metadata channel, and the branch tool quiescence counters. Keep the behaviors the board requires, each on its simplest implementation: lazy per-action session-lock ownership (cold start activates after the lock lands; a secondary session stays inert), mirror durability across extension rebinds via the durable cursor, replay-exactly-once from the one read cursor, the awaited operational-encoded fallback, per-generation stray-lease cleanup, session-lock-bound lease liveness (a recycled pid or a non-Pi home never honors a leftover lease), the loud accidental-override guards (readonly actor prelude, cross-actor claim refusal), and the role-partition refinements (no forced teardown, no direct relaunch for the branch). Default-on-for-Pi is unchanged. * no-mistakes(review): Enforce lock ownership and serialize lease mutations * no-mistakes(review): Synchronize guard cleanup and bind leases to lock owner * no-mistakes(review): Report outcomes before acknowledging durable wakes * no-mistakes(review): Restrict leases to Pi and instruct main claims * no-mistakes(review): Reject malformed lease locks and torn outcome tails * no-mistakes(review): Validate complete outcome tails before appending * no-mistakes(review): Guard branch side effects across session replacements * no-mistakes(document): Update Pi supervision durability and lease documentation * no-mistakes(lint): Suppress intentional nested-shell expansion warning * no-mistakes: apply CI fixes * fix(pi-branch): authorize lease releases by caller * fix(lint): break redundant source-analysis path in fm-lease-lib.sh fm-lease-lib.sh's lazy fallback source of fm-wake-lib.sh gave ShellCheck's --external-sources traversal a second path into an already 1540-line file that fm-send.sh and fm-teardown.sh also source directly, blowing up the recursive analysis past CI's lint timeout. Mark it a source=/dev/null analysis boundary, matching the existing fm-task-inbox-lib.sh convention. Also restores bin/fm-lint.sh and tests/fm-lint.test.sh to the shared serial-lint definition (dropping an unrelated parallel-sharding change that was itself hanging and masked this root cause). * no-mistakes(document): Correct lease caller-authorization documentation * no-mistakes: apply CI fixes * no-mistakes: apply CI fixes * no-mistakes: apply CI fixes * fix(bin): parallelize startup network sweeps (kunchenguid#2927) * feat(bin): parallelize session-start remote secondmate network sweeps Run per-secondmate liveness and convergence probes concurrently and overlap clone refresh, while replaying each mate's fail-closed diagnostic in original order. Ignore scratchpad* so untracked scratch no longer blocks remote sync. Co-authored-by: Cursor <cursoragent@cursor.com> * no-mistakes(document): Document parallel startup network sweeps * no-mistakes(lint): Fix empty environment assignment lint warning * no-mistakes: apply CI fixes --------- Co-authored-by: Cursor <cursoragent@cursor.com> * test: handle absent watcher wake queues (kunchenguid#2845) * fix(tests): count declared-pause wakes without crashing on an absent queue The exited-declared-pause case counts queued stale wakes by handing state/.wake-queue straight to awk. A watcher that queues nothing never creates that file, and awk aborts on a missing path before its END rule runs, so the count collapses to the empty string. The next comparison then fails as an integer-expression error and surfaces as a wake flood with no number, hiding the real contract breach the following grep names. Read the queue the way the drain-count assertion at the end of this file already does: silence awk's open error and default an absent queue to zero. Applied to all four counts in this case, including the live external-decision gate pair whose queue an acknowledged drain can also leave behind. An absent queue now reports "did not use the bounded paused recheck", while a genuine flood still fails with its real count. Fixes kunchenguid#2628 * no-mistakes: apply CI fixes * no-mistakes: apply CI fixes * style(pi): distinguish routine and captain supervision merge notes by icon (kunchenguid#2934) * style(pi): restyle supervision merge notes with a sailboat and matching pad Secondary-session notes were flush against the TUI edge and fully tinted. Use the sailboat prefix, Pi's default outputPad, boat-only color, and dim remainder so they sit like real messages. * style(pi): distinguish routine and captain merge notes by icon only Visible notes now lead with a sailboat or anchor, then only the dim outcome. Drop the branch-merged wording and verdict brackets so the icon is the only kind signal. * docs(pi): add the approved multi-brain architecture poster (kunchenguid#2938) The markdown contract stays the owner; the still is only the visual of the idea. * feat(pi): default branch supervision and route heartbeats (kunchenguid#2939) * fix(bin): bound remote job worker supervisor restarts (kunchenguid#2942) * fix(bin): bound remote worker supervisors * no-mistakes(review): release incumbent supervisor before starting its replacement * no-mistakes(review): wait out a healthy same-root supervisor instead of replacing it * no-mistakes(review): narrow remote worker change to restart accounting only * no-mistakes(document): clarify supervisor restart guard is a lifetime total * fix: safely split supervision wake handling by actor (kunchenguid#2953) * feat(bin,pi): per-actor wake consume, silent success gating, merge-poll dedup Three related fixes to the shared wake-drain and Pi supervision-branch dispatch machinery so a routine success is never main-blocking and a mixed queue can safely split between actors. 1. Successful routine results no longer create main-blocking wake rows. fm-startup-network.sh only enqueues a check: startup-network wake when the deferred result is actionable (state is not "done", or the report carries a bootstrap-diagnostics actionable prefix); a clean success stays durable in the report file without ever waking the agent. 2. Per-actor wake-drain consume contract. bin/fm-wake-drain.sh now scopes presentation and --ack-through to the current actor (bin/fm-lease-lib.sh's fm_lease_actor): main keeps the original whole-queue cutoff behavior, unaffected. A branch actor (FM_SUPERVISION_ACTOR=branch, set only inside the Pi supervision branch's own bash tool calls) is scoped to an explicit eligible-row snapshot instead of a cutoff comparison, so it can never remove a row it was not granted - the fix for the swallow risk that used to force an all-or-nothing whole-queue fallback to main. .pi/extensions/lib/fm-branch-dispatch.ts's scopeForUnreadWake is the single owner of eligibility: a check-kind row (merge-confirmation polls, Relay mentions, credential/auth failures) is now excluded rather than vetoing the whole scan for a non-heartbeat wake, while a heartbeat review keeps its original all-or-nothing rule unchanged. writeEligibleRowsSnapshot publishes the exact eligible sequence numbers before every branch prompt; fm-primary-pi-watch.ts's offer still refuses a check-kind trigger outright so a main-only close is never itself routed to the branch. 3. A repeat identical merged-PR-poll result for an already-notified task is absorbed instead of enqueued again. A poll's own retirement state is scoped to one registration and cannot see a prior registration's outcome, so a task re-registered after its merge was already surfaced would otherwise wake main a second time for the same event. bin/fm-pr-lib.sh's new per-task pr-poll-merge-notified marker survives across re-registrations to catch that case; the first notification for a task still reaches main unchanged. Regression tests colocated in tests/fm-startup-network.test.sh, tests/fm-wake-queue.test.sh (including the mixed-queue no-swallow property), tests/fm-pi-branch-extension.test.sh, and tests/fm-pr-check-security.test.sh. docs/watcher-continuity.md and docs/pi-supervision-branch.md updated for the new contracts. * no-mistakes(review): Bind merge deduplication to canonical PR identity * no-mistakes(review): Serialize wake row ownership across main and branch * no-mistakes(review): Bind branch grants and deduplicate within actor claims * no-mistakes(review): Fallback main-owned wake claims to main delivery * no-mistakes(review): Clarify silent startup success guidance * no-mistakes(review): Release residual branch grants after settled prompts * no-mistakes(review): Reject truncated wake rows as corrupted * no-mistakes(document): Document per-actor routing and silent startup success * no-mistakes(lint): Fix ShellCheck findings in wake grant and startup test * no-mistakes: apply CI fixes * fix(pi): hide branch outcomes tool rows in Calm (kunchenguid#3024) * Hide branch outcome tool in Pi Calm * no-mistakes(review): Preserve stock outcomes rendering and document tool audit * no-mistakes(review): Document branch read tool audit disposition * no-mistakes(review): Match stock outcomes output sanitization * no-mistakes(document): Document Calm custom-tool visibility * no-mistakes(review): Secure Pi wake batches and complete orchestrator documentation * no-mistakes(document): Correct current orchestrator comparison facts * Add opt-in resumable Codex crewmates (#4) * Add opt-in resumable Codex crewmates * fix(bin): restore parked Codex binding after crash --------- Co-authored-by: wenkxu <v-wenkxu@expediagroup.com> * Add persistent human names and names-first routing (#6) * Add persistent human callsigns * Close persistent callsign safety gaps --------- Co-authored-by: wenkxu <v-wenkxu@expediagroup.com> * Bind tmux task routing to stable pane identity (#7) * Bind tmux task routes to stable pane identity * Handle failed tmux identity binding --------- Co-authored-by: wenkxu <v-wenkxu@expediagroup.com> * fix: gate Codex checkpoints on genuine idle (#12) Co-authored-by: wenkxu <v-wenkxu@expediagroup.com> * feat: register external workspace routes (#13) Co-authored-by: wenkxu <v-wenkxu@expediagroup.com> * feat(codex): propagate assigned display titles (#14) * feat(codex): propagate assigned display titles * test(codex): keep title resume guard isolated --------- Co-authored-by: wenkxu <v-wenkxu@expediagroup.com> * no-mistakes(review): Secure Pi wake batches and complete orchestrator documentation * no-mistakes(document): Correct current orchestrator comparison facts --------- Co-authored-by: Christopher McKay <101884182+karotkriss@users.noreply.github.com> Co-authored-by: Kun Chen <3233006+kunchenguid@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: stanzhang <thinking.chang@gmail.com> Co-authored-by: wenkxu <v-wenkxu@expediagroup.com>
1 participant
User description
Closes #2
Summary
Final local verification
tests/fm-identity.test.sh(full executable identity/routing suite)test_codex_exact_session_resume_and_ordinary_switch_retire_bindingfromtests/fm-control-relaunch.test.shtest_callsigns_accompany_live_results_and_historyfromtests/fm-bearings-snapshot.test.shtests/fm-send-strict.test.sh(full strict routing suite)bash -n bin/fm-identity-lib.sh bin/fm-send.sh bin/fm-fleet-snapshot.sh bin/fm-bearings-snapshot.sh tests/fm-identity.test.sh tests/fm-control-relaunch.test.sh tests/fm-bearings-snapshot.test.shgit diff --checkThe two initial identity reruns failed only because old test fixtures used endpoint shapes the new shared ownership validator correctly refuses; after correcting those fixtures to real
session:fm-taskownership and removing the deliberately unsafe fixture before migration, the full identity suite passed. No installation, release, production, browser, worker, or live-application action was performed.CodeAnt-AI Description
Add persistent callsigns and names-first task routing
What Changed
fm-name.sh; task IDs remain visible beside callsigns.Impact
✅ Names-first worker routing✅ Stable task identity across relaunches✅ Fewer commands sent to the wrong endpoint💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.