fix: keep decision metadata from invalidating PR polls

bin/fm-decision-hold.sh

@@ -32,6 +32,8 @@
 # no unresolved captain decision. Later review passes may add keys; a live task's
 # metadata inventory is unioned idempotently. A post-teardown visual review can
 # complete against the surviving report and holds without recreating task state.
+# `complete` may append its metadata keys after a task's PR fields; bin/fm-pr-lib.sh
+# owns the PR parser contract and does not reserve a state/<id>.meta tail.
 # `verify` is read-only and is called by scout teardown so teardown cannot erase a
 # source before this gate has succeeded. A resolved captain hold that retention
 # moved into data/done-archive.md remains a durable completion record, but only

bin/fm-pr-check-migrate.sh

@@ -506,7 +506,7 @@ metadata_pr_is_canonical() {
   MIGRATION_HOST=
   MIGRATION_PATH=
   MIGRATION_NUMBER=
-  fm_pr_metadata_identity_parse "$meta" || return 1
+  fm_pr_legacy_metadata_identity_parse "$meta" || return 1
   MIGRATION_PROVIDER=$FM_PR_META_PROVIDER
   MIGRATION_URL=$FM_PR_META_URL
   MIGRATION_HOST=$FM_PR_META_HOST

bin/fm-pr-lib.sh

@@ -11,6 +11,15 @@
 # instances, so the host is part of that identity rather than a constant. Every
 # consumer re-derives the identity from the stored URL and refuses any record
 # whose parts do not reconstruct that exact URL.
+#
+# state/<id>.meta is shared append-only key=value metadata, and this library
+# owns only the PR-specific identity contract for that file. A current
+# authenticated poll requires exactly one canonical pr= line, validates every
+# pr_head= value it sees, and otherwise accepts well-formed metadata keys before
+# or after pr=. No script may rely on pr= reserving the file tail. The
+# non-executing legacy migration uses the stricter legacy predicate below because
+# pre-authentication check files were runnable shell and must stay quarantined
+# when their post-pr bytes are ambiguous.
 
 FM_PR_PROVIDER=
 FM_PR_URL=
@@ -253,8 +262,20 @@ fm_pr_regular_destination_on_device_or_absent() {
   [ ! -e "$path" ] || [ "$(fm_pr_file_device "$path")" = "$device" ]
 }
 
+fm_pr_metadata_line_key_valid() {
+  local line=${1-} key
+  case "$line" in
+    *$'\r'*) return 1 ;;
+    *=*) key=${line%%=*} ;;
+    *) return 1 ;;
+  esac
+  case "$key" in
+    ''|*[!A-Za-z0-9._-]*) return 1 ;;
+  esac
+}
+
 fm_pr_metadata_identity_parse() {
-  local file=$1 line value pr_count=0 seen_pr=0 post_pr_invalid=0
+  local file=$1 line value pr_count=0 invalid=0
   FM_PR_META_PROVIDER=
   FM_PR_META_URL=
   FM_PR_META_HOST=
@@ -263,6 +284,10 @@ fm_pr_metadata_identity_parse() {
   [ -f "$file" ] && [ ! -L "$file" ] || return 1
   [ "$(fm_pr_file_link_count "$file")" = 1 ] || return 1
   while IFS= read -r line || [ -n "$line" ]; do
+    fm_pr_metadata_line_key_valid "$line" || {
+      invalid=1
+      continue
+    }
     case "$line" in
       pr=*)
         pr_count=$((pr_count + 1))
@@ -275,24 +300,31 @@ fm_pr_metadata_identity_parse() {
           FM_PR_META_PATH=$FM_PR_PATH
           FM_PR_META_NUMBER=$FM_PR_NUMBER
         fi
-        seen_pr=1
         ;;
       pr_head=*)
-        if [ "$seen_pr" -eq 1 ]; then
-          value=${line#pr_head=}
-          fm_pr_head_valid "$value" || post_pr_invalid=1
-        fi
-        ;;
-      x_request=*|x_request_ts=*|x_followups=*|x_platform=*|x_reply_max_chars=*)
+        value=${line#pr_head=}
+        fm_pr_head_valid "$value" || invalid=1
         ;;
+    esac
+  done < "$file"
+  [ "$pr_count" -eq 1 ] || return 1
+  [ "$invalid" -eq 0 ] || return 1
+  [ -n "$FM_PR_META_URL" ]
+}
+
+fm_pr_legacy_metadata_identity_parse() {
+  local file=$1 line seen_pr=0 post_pr_invalid=0
+  fm_pr_metadata_identity_parse "$file" || return 1
+  while IFS= read -r line || [ -n "$line" ]; do
+    case "$line" in
+      pr=*) seen_pr=1 ;;
+      pr_head=*|x_request=*|x_request_ts=*|x_followups=*|x_platform=*|x_reply_max_chars=*) ;;
       *)
         [ "$seen_pr" -eq 0 ] || post_pr_invalid=1
         ;;
     esac
   done < "$file"
-  [ "$pr_count" -eq 1 ] || return 1
-  [ "$post_pr_invalid" -eq 0 ] || return 1
-  [ -n "$FM_PR_META_URL" ]
+  [ "$post_pr_invalid" -eq 0 ]
 }
 
 # Sidecar layout: provider, url, host, path, number, one per line. A sidecar

docs/gitlab-merge-watch.md

@@ -166,6 +166,7 @@ armed: state/e6.check.sh
 
 The stored record gained the provider tag, so its version moved to `fm-pr-poll-registration-v2` and a record written by the previous release no longer parses.
 The existing non-executing migration handles that: it never runs the old artifact, and rebuilds the poll from the task's recorded pull request URL.
+Because those old check files were runnable shell, the migration rebuilds only when the metadata is safe to classify; ambiguous bytes after the recorded pull request leave the legacy artifact quarantined and unarmed.
 Starting from a poll armed exactly as the previous release wrote it:
 
 ```

tests/fm-pr-check-security.test.sh

@@ -14,6 +14,7 @@ set -u
 
 PR_CHECK="$ROOT/bin/fm-pr-check.sh"
 PR_MERGE="$ROOT/bin/fm-pr-merge.sh"
+DECISION_HOLD="$ROOT/bin/fm-decision-hold.sh"
 MIGRATE="$ROOT/bin/fm-pr-check-migrate.sh"
 POLL="$ROOT/bin/fm-pr-poll.sh"
 WATCH="$ROOT/bin/fm-watch.sh"
@@ -634,6 +635,48 @@ SH
   pass "valid direct and merge flows record exact metadata and reject multiline head metadata"
 }
 
+test_decision_completion_preserves_poll_validation() {
+  local dir state id hold
+  command -v tasks-axi >/dev/null 2>&1 || {
+    pass "skipped: tasks-axi not found for decision completion PR poll regression"
+    return
+  }
+  id=task-a
+  dir=$(make_case decision-completion-poll)
+  state="$dir/home/state"
+  cp "$ROOT/.tasks.toml" "$dir/home/.tasks.toml"
+  cat > "$dir/home/data/backlog.md" <<'EOF'
+## In flight
+
+## Queued
+
+## Done
+EOF
+  write_task_meta "$dir" "$id"
+
+  run_check_entry "$dir" "$id" https://github.com/o/r/pull/10 >/dev/null 2> "$dir/pr-check.err" \
+    || fail "could not arm a PR poll before recording a decision: $(cat "$dir/pr-check.err")"
+  fm_pr_poll_artifacts_valid "$state" "$id" "$POLL" \
+    || fail "PR poll did not validate immediately after arming"
+
+  hold=$(FM_ROOT_OVERRIDE="$ROOT" FM_HOME="$dir/home" \
+    FM_STATE_OVERRIDE="$state" FM_DATA_OVERRIDE="$dir/home/data" \
+    FM_CONFIG_OVERRIDE="$dir/home/config" PATH="$dir/fakebin:$PATH" \
+    "$DECISION_HOLD" hold "$id" route --title "Choose the sample route" \
+      --reason "captain route choice pending" --repo sample) \
+    || fail "could not record a decision hold"
+  [ "$hold" = "$id-decision-route" ] || fail "decision hold identity was not deterministic: $hold"
+  FM_ROOT_OVERRIDE="$ROOT" FM_HOME="$dir/home" \
+    FM_STATE_OVERRIDE="$state" FM_DATA_OVERRIDE="$dir/home/data" \
+    FM_CONFIG_OVERRIDE="$dir/home/config" PATH="$dir/fakebin:$PATH" \
+    "$DECISION_HOLD" complete "$id" route >/dev/null \
+    || fail "could not complete the decision inventory"
+
+  fm_pr_poll_artifacts_valid "$state" "$id" "$POLL" \
+    || fail "decision completion invalidated an authenticated PR poll"
+  pass "decision completion leaves an authenticated PR poll armed"
+}
+
 run_watcher_bounded() {
   local home=$1 fakebin=$2 check_interval=${FM_TEST_CHECK_INTERVAL:-0} watch_root=${FM_TEST_WATCH_ROOT:-$ROOT}
   shift 2
@@ -2848,6 +2891,7 @@ test_parser_matrix
 test_gitlab_merge_watch
 test_invalid_entrypoints_have_zero_side_effects
 test_valid_recording_and_merge_derivation
+test_decision_completion_preserves_poll_validation
 test_rejected_metacharacter_bytes_are_inert
 test_static_poll_contract
 test_atomic_interruption_leaves_no_partial_artifact