diff --git a/bin/fm-decision-hold.sh b/bin/fm-decision-hold.sh index a3f7d316dd..1d0bc97614 100755 --- a/bin/fm-decision-hold.sh +++ b/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/.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 diff --git a/bin/fm-pr-check-migrate.sh b/bin/fm-pr-check-migrate.sh index b8b1fb7942..fd16c977b9 100755 --- a/bin/fm-pr-check-migrate.sh +++ b/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 diff --git a/bin/fm-pr-lib.sh b/bin/fm-pr-lib.sh index 47e8be648f..04040ffa21 100755 --- a/bin/fm-pr-lib.sh +++ b/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/.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 diff --git a/docs/gitlab-merge-watch.md b/docs/gitlab-merge-watch.md index 0540ed296d..ecf29c6fe6 100644 --- a/docs/gitlab-merge-watch.md +++ b/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: ``` diff --git a/tests/fm-pr-check-security.test.sh b/tests/fm-pr-check-security.test.sh index 1fdb9ee9a6..1603d1f086 100755 --- a/tests/fm-pr-check-security.test.sh +++ b/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