feat(bin): gate landings on unresolved routing promotions - #2757
feat(bin): gate landings on unresolved routing promotions#2757npayette84 wants to merge 7 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4369dec0f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| # Before any durable recording or forge call: a promotion the fleet never answered | ||
| # means this PR was reviewed and staffed at a tier its own diff already disproved. | ||
| fm_promotion_gate "$STATE" "$ID" "$PROMOTION_OVERRIDE" || exit 1 |
There was a problem hiding this comment.
Captain, when --promotion-override is supplied, this gate writes the durable override record before fm-pr-check.sh and gh-axi pr merge run. If either later step fails, and the promotion is subsequently resolved normally before a successful retry, teardown still preserves a record falsely indicating that the task landed over an unanswered promotion. Defer finalizing the override record until the merge succeeds, or distinguish pending attempts from completed overrides.
Useful? React with 👍 / 👎.
|
Speaking as Kun's firstmate: VISION verdict: the landing refusal aligns (strengthens a refusal path; the obligation survives a restart via the shared open-decisions fold). The change is not opt-in: both landing paths now refuse by default when a
Class: default-behavior. Reviewed the full thread and the full diff vs base. Fail-closed unreadable/symlinked ledgers, directory-and-symlink refusal on the override path, rename rather than redirect, and task-id sanitization look right. The skill correctly leaves a declined relaunch open and escalates. Security: no exploit. The override is a documented escape hatch, not a hidden bypass. This is waiting on the captain, not the author, for the default landing gate and the reason-only override. Also waiting on CI including no-mistakes. Not merge-eligible. Captain-flag NOW: NO, because CI is not green. |
A task's effective tier is the higher of two resolutions: the one the first mate makes at intake, and the one the worker can only make once its own diff exists. The second pass runs inside the worker, after the first mate has already frozen every tier-dependent dispatch decision - harness, effort, reviewer count and strength, delivery mode. The worker may not repin itself and the first mate had no trigger to re-staff, so a task dispatched at a low tier kept that rigor even after its own diff proved it needed more. Add a `promoted` status verb that opens a durable keyed record through the SAME open-decisions fold as needs-decision and blocked, rather than a parallel mechanism. That reuse is what makes the re-staff obligation survive a restart and supplies the promotion-handled gate for free: an open promoted record IS an unhandled promotion, and a matching keyed `resolved` line closes it once the first mate has re-staffed. The verb is deliberately nonterminal, so the worker keeps working while the first mate re-staffs around it, and it is surfaced unconditionally - ahead of the FM_CAPTAIN_RE override every other verb defers to - because it is an obligation to act rather than a question of what is worth reporting. It is fixed rather than configurable: a rename would stop the fold matching records already written with the old spelling, silently disarming everything below it. It appears in the ship scaffold alone, since a scout produces a report and a charter governs a domain, so neither has a diff to re-resolve. Landing is the last moment stale rigor can still be corrected, so both landing paths refuse while a promotion is open: fm-pr-merge.sh before it records anything or reaches the forge, and fm-merge-local.sh before it touches the project. bin/fm-promotion-gate-lib.sh owns that check, its refusal, and its override once, so the two paths cannot drift on what unanswered means. Neither closure route is new. Answering the worker through fm-send --resolve-key closes the record as it delivers, and transferring the promotion to a captain hold closes it through the other ledger, so doing the real work clears the gate as a side effect. Because answering is the cheaper route, fm-send now names the re-staff obligation when the key it closes is a promotion, so the cheap path cannot quietly substitute for the expensive one. The single override exists for the one case neither route serves: a worker already gone in a home whose backlog cannot take a hold. It demands a stated reason, refuses blank and flag-shaped values, and records the reason beside the task through a rename rather than a redirect so a planted symlink cannot divert the write. That record deliberately survives teardown, because the status log holding the unanswered promotion survives too, and deleting the justification while keeping the finding would leave the worse half of the pair. An existing but unreadable or symlinked status ledger fails closed rather than reading as consent, while a genuinely absent one still lands: absence means the worker has reported nothing yet, but a ledger nobody can read is not evidence of anything. The matching race between that check and the fold is deliberately not chased, and the reasoning is recorded beside it. .agents/skills/routing-promotion/SKILL.md owns the re-staff procedure, and AGENTS.md carries only its load trigger.
4369dec to
affca20
Compare
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (2): Last reviewed commit: "no-mistakes: apply CI fixes" | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: affca20956
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this task's actual changed paths raises its tier above the one you were dispatched at, append | ||
| \`promoted [key=<slug>]: <tier> <factor> - <what raised it>\` as soon as you know, then KEEP WORKING. |
There was a problem hiding this comment.
Captain, every generated ship brief asks the worker to report when the changed paths raise the task above “the one you were dispatched at,” but neither this scaffold nor task metadata supplies that baseline tier or the rules for resolving it; a repo-wide search finds the two-pass tier contract only in comments added by this commit. In the standard dispatch flow, the worker therefore cannot determine whether it was promoted, so no promoted record is emitted and both landing gates silently remain inactive. Include the initial tier and a usable resolver in the generated brief or durable task context.
AGENTS.md reference: AGENTS.md:L504-L506
Useful? React with 👍 / 👎.
| case "$verb" in | ||
| "$FM_CLASSIFY_PROMOTION_VERB") return 0 ;; |
There was a problem hiding this comment.
Captain, this only makes a promotion actionable when it is the status file's latest nonblank line: signal_reason_is_actionable and the heartbeat scan both call last_status_line, and the watcher intentionally waits through SIGNAL_GRACE before classifying. Because the new brief explicitly tells the worker to keep working, a subsequent working: phase update during that grace window replaces the promotion for triage, the busy worker's signal is absorbed, and re-staffing may not happen until a much later drain or landing refusal. Classify the unseen status span/open-decision fold rather than only its last line so every promoted wake reaches the required handler.
AGENTS.md reference: AGENTS.md:L532-L532
Useful? React with 👍 / 👎.
| # Same gate as the PR path, before anything in the project is touched. | ||
| fm_promotion_gate "$STATE" "$ID" "$PROMOTION_OVERRIDE" || exit 1 |
There was a problem hiding this comment.
Captain, when --promotion-override is used on a local-only task, this call writes the durable override before checking that the task branch exists, the primary checkout is clean and on its default branch, or the branch can fast-forward. Any later refusal therefore leaves a permanent record claiming the task was landed over an unanswered promotion even though no merge occurred; if the promotion is then resolved before a successful retry, that false audit record survives teardown. Defer finalizing the record until the fast-forward succeeds, or distinguish attempted overrides from completed landings.
Useful? React with 👍 / 👎.
| needs-decision|blocked) | ||
| [ "$( _fm_open_set_verb "$open" "$key")" = "$FM_CLASSIFY_PROMOTION_VERB" ] \ | ||
| && { printf '%s' "$open"; return 0; } |
There was a problem hiding this comment.
Captain, when a worker opens needs-decision or blocked with the same key as an existing promotion, these lines discard the new record and retain only the promotion. The brief allows an arbitrary <slug> rather than reserving a promotion namespace, so a reasonable sequence such as promoted [key=schema] followed by needs-decision [key=schema] silently loses the human question; resolving the promotion then removes the only folded record and the unanswered decision never appears in OPEN DECISIONS. Store promotions independently or require a collision-proof reserved key instead of suppressing the later opener.
AGENTS.md reference: AGENTS.md:L514-L515
Useful? React with 👍 / 👎.
| if [ -n "$reason" ]; then | ||
| fm_promotion_override_record "$state" "$id" "$reason" || return 1 | ||
| echo "notice: landing $id over an unanswered routing promotion; reason recorded: $reason" >&2 | ||
| return 0 |
There was a problem hiding this comment.
Captain, even when an override successfully lands the work, this path records the reason but leaves the promoted entry open in the status fold. Teardown deliberately preserves that status log while removing the task metadata, so every later OPEN DECISIONS scan continues surfacing the already-landed promotion, but fm-send --resolve-key can no longer resolve it because the target metadata is gone; the separate override file is not read by any listing or reconciliation path. Record an explicit closing transition after the landing succeeds, or teach the fold and presentation paths to reconcile a completed override.
AGENTS.md reference: AGENTS.md:L532-L532
Useful? React with 👍 / 👎.
| # The routing-promotion verb is fixed at "promoted" and appears in the SHIP | ||
| # scaffold only: it reports that a task's own diff re-resolved its tier upward, | ||
| # which presupposes a diff to resolve. A scout produces a report and a charter | ||
| # governs a domain, so neither can raise one, and listing a verb those briefs |
There was a problem hiding this comment.
Captain, the new reporting contract is emitted only by the initial ship scaffold, but bin/fm-promote.sh converts a running scout to kind=ship while deliberately retaining its loaded scout instructions and prints only a short follow-up about branching, implementation, and done. Because scout scaffolds explicitly omit promoted, every task entering the supported scout-to-ship flow remains unaware of the new status verb and can land without ever reporting a higher tier. Have promotion send the full current ship status contract or regenerate and deliver the applicable ship instructions.
AGENTS.md reference: AGENTS.md:L504-L506
Useful? React with 👍 / 👎.
| case "$resolve_open_set" in | ||
| "$k"$'\t'"$FM_CLASSIFY_PROMOTION_VERB"$'\t'*|*$'\n'"$k"$'\t'"$FM_CLASSIFY_PROMOTION_VERB"$'\t'*) | ||
| RESOLVE_PROMOTION_KEYS="${RESOLVE_PROMOTION_KEYS}${RESOLVE_PROMOTION_KEYS:+ }$k" |
There was a problem hiding this comment.
Captain, fm-send snapshots only the key and promotion verb here, then performs the potentially slow backend delivery before appending an unconditional resolved line. Since the worker is told to keep working and successive promotions deliberately reuse one key, a replacement can append a higher promotion after this snapshot but before the close; the stale answer for the earlier tier then closes the newer record too, allowing landing without acting on the latest tier. Bind the close to the specific folded promotion that was answered and leave the key open when its verb or note changed during delivery.
AGENTS.md reference: AGENTS.md:L514-L515
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5be82e6745
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| # Before any durable recording or forge call: a promotion the fleet never answered | ||
| # means this PR was reviewed and staffed at a tier its own diff already disproved. | ||
| fm_promotion_gate "$STATE" "$ID" "$PROMOTION_OVERRIDE" || exit 1 |
There was a problem hiding this comment.
Captain, if the worker appends promoted after this check while fm-pr-check.sh or the GitLab verification is running, the subsequent forge call at lines 285/293 still merges without observing it; the brief explicitly tells promoted workers to keep working, and the same check-before-validation window exists in fm-merge-local.sh. Require a terminal/quiesced worker or otherwise serialize the final promotion check with landing so a late promotion cannot bypass the new guard.
AGENTS.md reference: AGENTS.md:L329-L329
Useful? React with 👍 / 👎.
3 participants
Intent
Build the receiving half of firstmate's routing-promotion channel. The producer half already existed in the project's design discussion: a worker can re-resolve its own risk tier upward once its diff exists, but the first mate has by then frozen every tier-dependent dispatch decision, so a task dispatched at a low tier kept that rigor even after its own diff proved it needed more. The project's own review reports named the receiving half as unbuilt: status vocabulary, parser, re-staff obligation, and the brief-text reporting exemption.
Deliberate design decisions, so they are choices rather than oversights: (1) the promotion record reuses the SAME open-decisions fold as needs-decision and blocked instead of a parallel mechanism, which is what makes the obligation survive a restart and gives the promotion-handled gate for free; (2) BOTH landing paths refuse while a promotion is open, not just the PR path, because a hole in one of three delivery modes is harder to reason about than all or none; (3) re-staffing means relaunching onto a stronger runtime, never rewriting a running worker's instructions, because the spawn guard requires the brief and the durable record to agree - a delivery-path raise escalates to the human instead; (4) the verb is FIXED rather than configurable, because a rename would stop the fold matching records already written with the old spelling and silently disarm the gate; (5) the verb is surfaced unconditionally, ahead of the FM_CAPTAIN_RE override that every other verb defers to, because it is an obligation to act rather than a question of what is worth reporting; (6) the verb appears in the ship scaffold only, since a scout produces a report and a charter governs a domain, so neither has a diff to re-resolve; (7) there is exactly one override for the case neither closure route serves, and it demands a stated reason, records it via rename rather than redirect so a planted symlink cannot divert the write, and that record deliberately survives teardown because the status log holding the unanswered promotion survives too; (8) an existing but unreadable or symlinked status ledger fails closed while a genuinely absent one still lands; (9) the TOCTOU race between that ledger check and the fold is deliberately NOT closed, and the reasoning is recorded beside the check - a writer who can swap the ledger can already append a keyed resolved line that clears the same gate, so a swap-specific defense adds nothing against that principal.
The change has already been through three rounds of adversarial review by two independent reviewers; ten findings were fixed and each fix is pinned by a regression verified to fail against the pre-fix code.
What Changed
promotedship-status verb to the shared open-decisions fold so upward routing-tier changes remain visible until resolved.Risk Assessment
✅ Low: The promotion path is bounded and the current remediation commits preserve the open-promotion gate across same-key blockers while correctly fail-closing unsafe ledgers and override destinations.
Testing
Targeted behavioral suites passed, including the previously reported Herdr preflight slice. The CLI evidence shows the intended end-user flow: an unanswered promotion blocks local landing without moving the branch, resolving its key records the re-staff decision, and the same landing command then succeeds. This is CLI-only behavior, so no rendered UI evidence applies.
Evidence: End-to-end routing-promotion CLI transcript
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (3) ✅
bin/fm-promotion-gate-lib.sh:166- Captain: the intent limits the sole override to when “neither closure route serves” (the worker is gone and the hold path is unavailable). This hunk accepts any nonempty reason and proceeds without checking either condition, so a live worker in a tasks-axi-backed home can pass--promotion-overrideand land without re-staffing or creating a captain hold. Restrict the override to that stated exceptional state, or explicitly approve a trusted reason-only escape hatch.🔧 Fix: Clarify promotion override escape hatch
1 error still open:
.agents/skills/routing-promotion/SKILL.md:31- The required criterion says a re-staff is a relaunch onto a stronger runtime. This newly added procedure instead permits keeping the current worker when a relaunch seems too costly, then its following step instructs closing the record; that makes an unresolved higher-tier promotion landable without a stronger runtime. Remove this opt-out, or explicitly authorize it as a containment path that leaves the record open and escalates.🔧 Fix: Keep declined promotions open
3 errors still open:
bin/fm-send.sh:480- The required criterion says “re-staffing means relaunching onto a stronger runtime.” But any confirmedfm-send <task> --resolve-key <promotion-key> <arbitrary text>appendsresolvedand clears the gate without a relaunch record or stronger-runtime check. Sequence:promoted [key=tier], generic resolve-key send, then either landing path proceeds. Decide whether this closure is intentionally discipline-only; otherwise bind promotion closure to a completed, stronger re-staff receipt at the shared resolution boundary.bin/fm-classify-lib.sh:369- A laterneeds-decisionorblockedline using the promotion’s key replaces the openpromotedrow. The landing gate filters only rows whose current verb ispromoted, sopromoted [key=tier]followed byblocked [key=tier]permits both landing paths without a keyed resolution. Preserve an open promotion until its matchingresolved/captain-heldtransition instead of allowing another opener to overwrite it.bin/fm-promotion-gate-lib.sh:135- The override writer rejects a symlink but not an existing directory. On BSD/macOS,mv -f "$tmp" "$path"then moves the temp file inside that directory, returns success, and lets landing continue, while the documentedstate/<id>.promotion-overriderecord is not readable at that path. Refuse a directory destination before the move so a successful override always leaves the promised auditable file.🔧 Fix: Preserve promotions across keyed blockers
✅ Re-checked - no issues remain.
🔧 **Test** - 1 issue found → auto-fixed ✅
tests/fm-teardown.test.sh:1599-tests/fm-teardown.test.shfails its Herdrmissing-adapterpreflight case: teardown continues when the required adapter is removed from the isolated test root. This is a merge-blocking targeted-test failure, although the exercised preflight slice is unchanged between the supplied base and target commits.bin/fm-test-run.sh tests/fm-brief.test.sh tests/fm-classify-routing-promotion.test.sh tests/fm-crew-state.test.sh tests/fm-promotion-gate.test.sh tests/fm-send-resolve-key.test.sh tests/fm-teardown.test.sh— completed cleanly through the promotion-gate suite; the remaining suites were re-run with a persisted result.bin/fm-test-run.sh tests/fm-send-resolve-key.test.sh tests/fm-teardown.test.sh—fm-send --resolve-keycompleted successfully;fm-teardownfailed the Herdr missing-adapter preflight scenario (exit 1 overall).bin/fm-merge-local.sh promotion-demoin an isolated evidence fixture — verified an openpromoted [key=tier]record refuses before movingmain; a stated--promotion-overridethen fast-forwarded the actual fixture branch and persisted its reason.bin/fm-merge-local.sh resolved-demoin an isolated evidence fixture — verified a matching keyedresolvedrecord permits a normal fast-forward without the override.🔧 Fix: Fix Herdr teardown adapter preflight
✅ Re-checked - no issues remain.
git diff --name-only 4d2cb0ca5a85d700f8a89e354f9e0eb42b38a64b fa89dec6b85fd6f6b85375ec87b8dc8933784074bash tests/fm-promotion-gate.test.shbash tests/fm-classify-routing-promotion.test.shbash tests/fm-crew-state.test.shbash tests/fm-brief.test.shbash tests/fm-send-resolve-key.test.shbash tests/fm-teardown.test.shIsolated CLI fixture: open promotion →fm-merge-local.shrefusal with unchanged default branch → realfm-send.sh --resolve-keythrough a fake transport → successful same-command local landing.✅ **Document** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.