Repository navigation
fix(bin): treat null classic required checks as empty - #7347
Open
mehulbhagwani wants to merge 6 commits into
Open
mehulbhagwani wants to merge 6 commits into
mehulbhagwani wants to merge 6 commits into
Conversation
Contributor
Author
CI evidence (head
|
Owner
|
Speaking as Kun's firstmate: reviewed head Contract-class: restore — tip vs main in VISION:
Security FYI: this is a merge-authority loosening — branches that previously refused on CI: serial 2 cancelled at the ~30m ceiling; re-ran failed jobs on Waiting on you: no-mistakes marked live validation inconclusive (0 of 4 scenarios driven live). Could you live-test the null-classic / unreadable / green / never-reported paths against a real protected branch (or equivalent live forge evidence) and post the evidence here, or re-raise through no-mistakes once that is done? I will not run a live/lab test from triage. Thanks, @mehulbhagwani. |
mehulbhagwani
changed the title
Owner
|
Speaking as Kun's firstmate: thank you @mehulbhagwani. Running the real script against real GitHub behind a write-blocking wrapper is exactly the kind of evidence I was hoping for, and the 200-repo shape scan answers why the null case can't be found live. Delta since my 2026-10-15 stamp ( CI: all green except "Behavior tests (Herdr)": Contract-class: restore (unchanged). An explicit VISION.md: one captain aligns · authority aligns with care (only documented emptiness proceeds) · scripts aligns · restart n/a · spine aligns (the configured merge authority still stands) · vendor aligns · scope aligns. Security FYI (unchanged): this loosens the merge gate for review-only protected branches. So once the Herdr re-run is green, this goes to Kun as a merge-or-leave flag rather than an automatic merge. Nothing more is needed from you unless CI says otherwise. |
Contributor
Author
|
Fresh forge follow-through: head 7a4a1be is now fully green (19/19 checks passed). The previously posted live green/never-reported and hybrid null/unreadable evidence remains accepted and is bound to this head. This is ready for Kun's merge-or-leave security decision; the null classic required-checks behavior remains intentionally captain-gated. |
Owner
|
Speaking as Kun's firstmate: thank you @mehulbhagwani for following up. Your 2026-10-23 note is right. Head Re-checked at main Contract-class: restore (unchanged). VISION.md: one captain aligns · authority aligns with care · scripts aligns · restart n/a · spine aligns · vendor aligns · scope aligns. Where it stands: this is now with Kun as a merge-or-leave decision, because it loosens the merge gate for review-only protected branches (security FYI, unchanged). It is not waiting on you. Nothing more is needed unless main moves under it. |
A branch protected only by review rules can report protected:true with required_status_checks:null. That is readable emptiness for classic checks, not an unreadable producer that must refuse the merge. Keep non-object non-null shapes, a missing protection object, and a protection object without the required_status_checks key as unreadable. Fixes kunchenguid#7343
* fix(spawn): keep a recorded PR the task record's tail across relaunch
A control-plane relaunch republished the record with control_relaunch_tx=
after the preserved pr= lines, and the trace-context and Polytoken session
rewrites appended after them too. fm_pr_metadata_identity_parse refuses any
other line after pr=, so the watcher stopped running that lane's merge poll
("rejected unauthenticated state checks") and its merge went unseen. Every
spawn-side rewrite now writes the PR trailer last.
Refs kunchenguid#7938
* feat: park ship lanes on review and clean them up on merge
Add bin/fm-park.sh: a clean-state gate (GitHub and GitLab) that parks a lane
only when CI is green on the current head, review bots have posted on it or a
quiet window passed, no thread is unresolved, nothing conflicts, and the PR is
not a draft; a park that re-reads the forge bound to the recorded head before
stopping the agent through fm-control, durably leases the Treehouse slot (a
re-park reuses the lane's own lease), and records the lane; a watcher tick that
relaunches a fresh worker on review activity and reports closed PRs once; and
merge-time cleanup through fm-teardown from the watcher's merged path. The
watcher starts fm-park only when a lane is parked or parking is configured.
Teach teardown's landed proof to read GitLab merge requests and fetch
refs/merge-requests/<n>/head, so squash and merge-train merges are recognized
from the MR head.
Local proof:
- tests/fm-park.test.sh (13 tests) covers each unclean gate condition, a head
change after the verdict, park preflight refusals, the lease, every relaunch
trigger, closed reporting, the opt-ins and merge cleanup.
- An isolated lab home (Herdr lab session, faked forge, real Treehouse pool)
ran a full cycle through the real fm-pr-check, fm-park, fm-watch and
fm-teardown: park with the slot leased (a concurrent get received another
slot), a review comment relaunched a fresh worker in the same copy, re-park,
and on merge the watcher tore the lane down and returned the copy.
- A read-only gate run against live lanes in other homes refused lanes with
pending CI, unresolved threads or uncomputed mergeability, passed clean ones,
and read a merge-train squash-merged GitLab MR (main got a commit other than
the MR head) as merged.
Refs kunchenguid#7938
* no-mistakes(review): Harden park gate, bound tick, default merge cleanup
* no-mistakes(document): Inventory park-on-review config flags and state records
* no-mistakes(ci): I made fixes for all six Greptile findings (ci-1 to ci-6). One existing watcher test now fails because of the ci-1 change and needs your decision. Fixes: - **ci-1, merge cleanup skipped** (bin/fm-watch.sh): the merge poll now runs `fm-park.sh land` even when bin/fm-pr-merge.sh recorded the merge first. The cleanup outcome wakes the captain under the queue key `land-<id>`. New test `test_self_merged_lane_is_still_cleaned_up_by_the_poll` in tests/fm-pr-check-security.test.sh. - **ci-2, old CI approves parking** (GitLab gate): `head_pipeline` counts only when its sha equals the MR's current sha. - **ci-3, bot progress permits parking**: a reviewer bot counts as settled only when it has submitted a review bound to the current head; otherwise the quiet window decides. GitLab bot notes carry no such signal, so GitLab lanes now always wait for the quiet window (default 15 minutes after CI finishes). - **ci-4, edited feedback unanswered**: the relaunch trigger uses the later of a comment's creation and edit time. - **ci-5, later lanes starved**: a new `visit_lanes` helper starts each tick at the lane the previous tick did not reach (state/.review-park-tick-next and state/.review-park-tick-next-unparked) and wraps around. - **ci-6, unfinished park looks complete**: `cmd_park` writes state/<id>.review-park only after `fm-control.sh exit` confirms the stop. I updated the header docs in bin/fm-park.sh, docs/configuration.md and the operational-home-layout skill to match. Verification: - tests/fm-park.test.sh: all 18 cases pass, including the new regression cases for ci-2 to ci-6. - shellcheck is clean on all changed scripts and tests. - I did not run the new tests against the old code to see them fail first. Open problem: `test_merged_poll_reregistration_after_notification_is_absorbed` now fails. It re-arms a merge poll for a merge that was already reported, and in that test teardown refuses (the task has no project identity). The second watcher cycle now retries cleanup and wakes the captain with the same refusal again. I did not change this. The choice is: - keep repeating the refusal, which is arguably right because the lane still needs a decision, and update that test; or - wake only when cleanup succeeded in the already-recorded case, which would hide a refusal after a merge done through fm-pr-merge.sh and partly undo ci-1. The watcher subset stopped at that failure, so three tests never ran: the new ci-1 test, `test_self_merge_and_poll_publish_one_outcome`, and `test_merged_poll_row_names_no_authority_when_no_record_grants_one`. `test_merged_poll_retires_once` passed. I deleted the temporary subset file tests/.subset-sec.test.sh
* fix(park): write the park record pending before the stop and settle it on tick
cmd_park now writes state/<id>.review-park with pending=1 before
fm-control.sh exit and drops the flag once the stop is confirmed. A park
that dies after the stop, or whose final write fails, still leaves the
lane watched and resumable; crew state and list read it as parking. Each
tick promotes a pending record to parked once its agent is confirmed dead
and drops it while the agent is still alive. A record that cannot be
written refuses the park before the agent is stopped.
The re-armed merged-poll test now expects the repeated teardown-refusal
wake while the merge notice itself is not repeated.
* fix(park): bound the pending-park agent check by the tick budget
settle_pending asked the backend whether a pending lane's agent is alive
with no timeout, before the lane loop that honours the tick deadline, so
a hung backend (for example an unanswering tmux server) held the whole
tick and the watcher with it. The check now runs under fm_run_timed cut
to what the tick has left, and settling happens inside watch_parked, so
a tick that runs out of time saves the next lane in the existing cursor
and the next tick resumes there. An unanswered check leaves the record
pending.
* fix(park): require every required check reported, relaunch on inline replies
The GitHub park gate now refuses while a check the base branch requires has
not reported on the head (or the requirement cannot be read), using the
required-check reading bin/fm-pr-merge.sh does, moved into bin/fm-pr-lib.sh
so both share it. Review-thread comments join the activity list, so an
inline reply in an existing thread relaunches a parked worker.
* fix(park): run lane actions detached so no budget cuts them short
Park, relaunch, and merge-time cleanup now run as detached single-flight
actions under the lane lock instead of inside the tick or the watcher's
merged-poll iteration. The tick holds only forge reads and action starts,
so its budget can no longer kill a slow but successful relaunch, and the
merged-poll path only starts the teardown. Each action publishes its
outcome for the next tick to report, and a merge-time cleanup waits for an
action already under way on the lane.
The ship-landing skill now names the watcher as the one owner of a merged
lane's teardown, including merges made through fm-pr-merge.sh, so
firstmate no longer tears such a lane down a second time.
* no-mistakes(document): Document review parking and merge cleanup
* no-mistakes(ci): Fixed tick-bounded idle checks, reserved half the tick for unparked lanes, and persisted merge-cleanup requests before retiring the merge poll; pending requests retry on later ticks. Updated the park-on-review documentation and state-file list. `tests/fm-park.test.sh`, shell syntax checks, and `git diff --check` pass. The requested new regression tests for the three findings were not added, so those specific scenarios are not yet verified
* no-mistakes(ci): Fixed the requested behavior: unparked merge cleanup now requires config/auto-land; pending cleanup is recorded before poll retirement and dispatched after releasing the control lock; temporary lifecycle-lock refusals preserve the request for retry. Updated docs and behavioral tests. Full suite’s failure was only its maintainer-label gate; ci.yml was left unchanged. tests/fm-park.test.sh and git diff --check pass. ShellCheck reported existing source-following/unused-variable diagnostics; the lint partition and combined shellcheck command timed out, and the merge-poll tests were not completed
* no-mistakes(ci): Added behavioral regression tests for pending-cleanup recovery after watcher restart and retry after a temporary lifecycle-lock refusal. `FM_LIVE=0 bash tests/fm-park.test.sh`, `shellcheck tests/fm-park.test.sh`, and `git diff --check` pass
* test(park): wait for the pending cleanup request to clear before asserting
A detached land publishes its report before it removes the pending
request, so waiting only for the report let the cleanup tests assert
request removal too early. wait_land_settled waits for the request to
clear, which implies the report exists. The control-lock retry test now
ticks until the retry runs instead of sleeping, failing if any tick
reports the refusal as an outcome.
* fix(park): bind cleanup requests to their PR and record wakes before removing reports
A merge-time cleanup request now records the merged PR, and land drops a
request whose PR is no longer the lane's under the lane lock, so a stale
request cannot tear down a lane that moved on to another PR. Teardown also
removes a lane's cleanup request.
The tick now records its outcome and close lines as one wake before it
removes the reports behind them or marks a close as reported; when the wake
cannot be recorded it removes and prints nothing, so the next tick retries.
The watcher only rings.
The two tick-budget tests run under a 6s tick with 8s slow stubs: under a
2s tick the parked-lane budget was one whole second, so the first forge read
was skipped whenever setup crossed a second boundary.
* no-mistakes(document): Document park-on-review safety behavior
* fix(park): keep the cleanup request until its outcome is saved
Teardown no longer deletes a lane's merge cleanup request. Only the
detached cleanup removes it, after its outcome report is saved, so a
cleanup killed partway through its teardown is retried by the next tick
instead of being lost. A request left behind by a manual teardown is still
dropped by the tick once the task record is gone, or by land when the lane
now records a different PR.
* fix(bin): keep the PR merge watch working when a worker with a PR is relaunched (kunchenguid#7996)
* Preserve PR poll authentication on relaunch
* no-mistakes(review): Keep trace and polytoken meta ahead of PR lines
* fix(spawn): keep a recorded PR the task record's tail across relaunch
A control-plane relaunch republished the record with control_relaunch_tx=
after the preserved pr= lines, and the trace-context and Polytoken session
rewrites appended after them too. fm_pr_metadata_identity_parse refuses any
other line after pr=, so the watcher stopped running that lane's merge poll
("rejected unauthenticated state checks") and its merge went unseen. Every
spawn-side rewrite now writes the PR trailer last.
Refs kunchenguid#7938
* fix(spawn): reuse main PR metadata rewrite helper
* no-mistakes(review): Count unresolved GitLab notes individually
* no-mistakes(review): Relaunch parked lanes on incomplete GitHub threads
* no-mistakes(document): Document park-on-review behavior
* fix(park): paginate GitHub comments and fix Linux mode test
* no-mistakes(review): Set private permissions on cleanup requests
* no-mistakes(document): Correct park-on-review cleanup documentation
* fix(park): serialize inbox delivery with review parking
* no-mistakes(ci): Fixed `Lint 1`: removed the unused local variables in `tests/fm-park.test.sh` and exported `FM_HOME`, which the sourced inbox library reads. `bin/fm-lint.sh tests/fm-park.test.sh` passed. The Full suite failure was an aggregate of failed lanes; its log reports `FM_CI_LANES_FAILED=true`, not the underlying lane failure, so I can’t confirm this lint fix resolves it. The park test started passing scenarios but exceeded the 120-second timeout before completing
* fix(spawn): keep a recorded PR the task record's tail across relaunch
A control-plane relaunch republished the record with control_relaunch_tx=
after the preserved pr= lines, and the trace-context and Polytoken session
rewrites appended after them too. fm_pr_metadata_identity_parse refuses any
other line after pr=, so the watcher stopped running that lane's merge poll
("rejected unauthenticated state checks") and its merge went unseen. Every
spawn-side rewrite now writes the PR trailer last.
Refs kunchenguid#7938
* feat: park ship lanes on review and clean them up on merge
Add bin/fm-park.sh: a clean-state gate (GitHub and GitLab) that parks a lane
only when CI is green on the current head, review bots have posted on it or a
quiet window passed, no thread is unresolved, nothing conflicts, and the PR is
not a draft; a park that re-reads the forge bound to the recorded head before
stopping the agent through fm-control, durably leases the Treehouse slot (a
re-park reuses the lane's own lease), and records the lane; a watcher tick that
relaunches a fresh worker on review activity and reports closed PRs once; and
merge-time cleanup through fm-teardown from the watcher's merged path. The
watcher starts fm-park only when a lane is parked or parking is configured.
Teach teardown's landed proof to read GitLab merge requests and fetch
refs/merge-requests/<n>/head, so squash and merge-train merges are recognized
from the MR head.
Local proof:
- tests/fm-park.test.sh (13 tests) covers each unclean gate condition, a head
change after the verdict, park preflight refusals, the lease, every relaunch
trigger, closed reporting, the opt-ins and merge cleanup.
- An isolated lab home (Herdr lab session, faked forge, real Treehouse pool)
ran a full cycle through the real fm-pr-check, fm-park, fm-watch and
fm-teardown: park with the slot leased (a concurrent get received another
slot), a review comment relaunched a fresh worker in the same copy, re-park,
and on merge the watcher tore the lane down and returned the copy.
- A read-only gate run against live lanes in other homes refused lanes with
pending CI, unresolved threads or uncomputed mergeability, passed clean ones,
and read a merge-train squash-merged GitLab MR (main got a commit other than
the MR head) as merged.
Refs kunchenguid#7938
* no-mistakes(review): Harden park gate, bound tick, default merge cleanup
* no-mistakes(document): Inventory park-on-review config flags and state records
* no-mistakes(ci): I made fixes for all six Greptile findings (ci-1 to ci-6). One existing watcher test now fails because of the ci-1 change and needs your decision. Fixes: - **ci-1, merge cleanup skipped** (bin/fm-watch.sh): the merge poll now runs `fm-park.sh land` even when bin/fm-pr-merge.sh recorded the merge first. The cleanup outcome wakes the captain under the queue key `land-<id>`. New test `test_self_merged_lane_is_still_cleaned_up_by_the_poll` in tests/fm-pr-check-security.test.sh. - **ci-2, old CI approves parking** (GitLab gate): `head_pipeline` counts only when its sha equals the MR's current sha. - **ci-3, bot progress permits parking**: a reviewer bot counts as settled only when it has submitted a review bound to the current head; otherwise the quiet window decides. GitLab bot notes carry no such signal, so GitLab lanes now always wait for the quiet window (default 15 minutes after CI finishes). - **ci-4, edited feedback unanswered**: the relaunch trigger uses the later of a comment's creation and edit time. - **ci-5, later lanes starved**: a new `visit_lanes` helper starts each tick at the lane the previous tick did not reach (state/.review-park-tick-next and state/.review-park-tick-next-unparked) and wraps around. - **ci-6, unfinished park looks complete**: `cmd_park` writes state/<id>.review-park only after `fm-control.sh exit` confirms the stop. I updated the header docs in bin/fm-park.sh, docs/configuration.md and the operational-home-layout skill to match. Verification: - tests/fm-park.test.sh: all 18 cases pass, including the new regression cases for ci-2 to ci-6. - shellcheck is clean on all changed scripts and tests. - I did not run the new tests against the old code to see them fail first. Open problem: `test_merged_poll_reregistration_after_notification_is_absorbed` now fails. It re-arms a merge poll for a merge that was already reported, and in that test teardown refuses (the task has no project identity). The second watcher cycle now retries cleanup and wakes the captain with the same refusal again. I did not change this. The choice is: - keep repeating the refusal, which is arguably right because the lane still needs a decision, and update that test; or - wake only when cleanup succeeded in the already-recorded case, which would hide a refusal after a merge done through fm-pr-merge.sh and partly undo ci-1. The watcher subset stopped at that failure, so three tests never ran: the new ci-1 test, `test_self_merge_and_poll_publish_one_outcome`, and `test_merged_poll_row_names_no_authority_when_no_record_grants_one`. `test_merged_poll_retires_once` passed. I deleted the temporary subset file tests/.subset-sec.test.sh
* fix(park): write the park record pending before the stop and settle it on tick
cmd_park now writes state/<id>.review-park with pending=1 before
fm-control.sh exit and drops the flag once the stop is confirmed. A park
that dies after the stop, or whose final write fails, still leaves the
lane watched and resumable; crew state and list read it as parking. Each
tick promotes a pending record to parked once its agent is confirmed dead
and drops it while the agent is still alive. A record that cannot be
written refuses the park before the agent is stopped.
The re-armed merged-poll test now expects the repeated teardown-refusal
wake while the merge notice itself is not repeated.
* fix(park): bound the pending-park agent check by the tick budget
settle_pending asked the backend whether a pending lane's agent is alive
with no timeout, before the lane loop that honours the tick deadline, so
a hung backend (for example an unanswering tmux server) held the whole
tick and the watcher with it. The check now runs under fm_run_timed cut
to what the tick has left, and settling happens inside watch_parked, so
a tick that runs out of time saves the next lane in the existing cursor
and the next tick resumes there. An unanswered check leaves the record
pending.
* fix(park): require every required check reported, relaunch on inline replies
The GitHub park gate now refuses while a check the base branch requires has
not reported on the head (or the requirement cannot be read), using the
required-check reading bin/fm-pr-merge.sh does, moved into bin/fm-pr-lib.sh
so both share it. Review-thread comments join the activity list, so an
inline reply in an existing thread relaunches a parked worker.
* fix(park): run lane actions detached so no budget cuts them short
Park, relaunch, and merge-time cleanup now run as detached single-flight
actions under the lane lock instead of inside the tick or the watcher's
merged-poll iteration. The tick holds only forge reads and action starts,
so its budget can no longer kill a slow but successful relaunch, and the
merged-poll path only starts the teardown. Each action publishes its
outcome for the next tick to report, and a merge-time cleanup waits for an
action already under way on the lane.
The ship-landing skill now names the watcher as the one owner of a merged
lane's teardown, including merges made through fm-pr-merge.sh, so
firstmate no longer tears such a lane down a second time.
* no-mistakes(document): Document review parking and merge cleanup
* no-mistakes(ci): Fixed tick-bounded idle checks, reserved half the tick for unparked lanes, and persisted merge-cleanup requests before retiring the merge poll; pending requests retry on later ticks. Updated the park-on-review documentation and state-file list. `tests/fm-park.test.sh`, shell syntax checks, and `git diff --check` pass. The requested new regression tests for the three findings were not added, so those specific scenarios are not yet verified
* no-mistakes(ci): Fixed the requested behavior: unparked merge cleanup now requires config/auto-land; pending cleanup is recorded before poll retirement and dispatched after releasing the control lock; temporary lifecycle-lock refusals preserve the request for retry. Updated docs and behavioral tests. Full suite’s failure was only its maintainer-label gate; ci.yml was left unchanged. tests/fm-park.test.sh and git diff --check pass. ShellCheck reported existing source-following/unused-variable diagnostics; the lint partition and combined shellcheck command timed out, and the merge-poll tests were not completed
* no-mistakes(ci): Added behavioral regression tests for pending-cleanup recovery after watcher restart and retry after a temporary lifecycle-lock refusal. `FM_LIVE=0 bash tests/fm-park.test.sh`, `shellcheck tests/fm-park.test.sh`, and `git diff --check` pass
* test(park): wait for the pending cleanup request to clear before asserting
A detached land publishes its report before it removes the pending
request, so waiting only for the report let the cleanup tests assert
request removal too early. wait_land_settled waits for the request to
clear, which implies the report exists. The control-lock retry test now
ticks until the retry runs instead of sleeping, failing if any tick
reports the refusal as an outcome.
* fix(park): bind cleanup requests to their PR and record wakes before removing reports
A merge-time cleanup request now records the merged PR, and land drops a
request whose PR is no longer the lane's under the lane lock, so a stale
request cannot tear down a lane that moved on to another PR. Teardown also
removes a lane's cleanup request.
The tick now records its outcome and close lines as one wake before it
removes the reports behind them or marks a close as reported; when the wake
cannot be recorded it removes and prints nothing, so the next tick retries.
The watcher only rings.
The two tick-budget tests run under a 6s tick with 8s slow stubs: under a
2s tick the parked-lane budget was one whole second, so the first forge read
was skipped whenever setup crossed a second boundary.
* no-mistakes(document): Document park-on-review safety behavior
* fix(park): keep the cleanup request until its outcome is saved
Teardown no longer deletes a lane's merge cleanup request. Only the
detached cleanup removes it, after its outcome report is saved, so a
cleanup killed partway through its teardown is retried by the next tick
instead of being lost. A request left behind by a manual teardown is still
dropped by the tick once the task record is gone, or by land when the lane
now records a different PR.
* fix(bin): keep the PR merge watch working when a worker with a PR is relaunched (kunchenguid#7996)
* Preserve PR poll authentication on relaunch
* no-mistakes(review): Keep trace and polytoken meta ahead of PR lines
* fix(spawn): keep a recorded PR the task record's tail across relaunch
A control-plane relaunch republished the record with control_relaunch_tx=
after the preserved pr= lines, and the trace-context and Polytoken session
rewrites appended after them too. fm_pr_metadata_identity_parse refuses any
other line after pr=, so the watcher stopped running that lane's merge poll
("rejected unauthenticated state checks") and its merge went unseen. Every
spawn-side rewrite now writes the PR trailer last.
Refs kunchenguid#7938
* fix(spawn): reuse main PR metadata rewrite helper
* no-mistakes(review): Count unresolved GitLab notes individually
* no-mistakes(review): Relaunch parked lanes on incomplete GitHub threads
* no-mistakes(document): Document park-on-review behavior
* fix(park): paginate GitHub comments and fix Linux mode test
* no-mistakes(review): Set private permissions on cleanup requests
* no-mistakes(document): Correct park-on-review cleanup documentation
* fix(park): serialize inbox delivery with review parking
* no-mistakes(ci): Fixed `Lint 1`: removed the unused local variables in `tests/fm-park.test.sh` and exported `FM_HOME`, which the sourced inbox library reads. `bin/fm-lint.sh tests/fm-park.test.sh` passed. The Full suite failure was an aggregate of failed lanes; its log reports `FM_CI_LANES_FAILED=true`, not the underlying lane failure, so I can’t confirm this lint fix resolves it. The park test started passing scenarios but exceeded the 120-second timeout before completing
* no-mistakes(review): Document GitLab pagination completeness invariant
* no-mistakes(review): Remove duplicate temporary park test suite
* no-mistakes(review): Strengthen relaunch and inbox lock regressions
* no-mistakes(review): Keep merge poll armed when cleanup queueing fails
* no-mistakes(review): Preserve pending parks and merge cleanup retries
* no-mistakes(review): Test merged-poll cleanup queue retries
* no-mistakes(review): Retain cleanup request when outcome reporting fails
* no-mistakes(document): Fix park-on-review documentation links
* fix(park): relaunch a pending park only once its stop is confirmed
A pending record now stays pending while its agent reads alive, so feedback could relaunch over a worker whose park never finished. Hold every trigger until the stop is confirmed, for a live agent and an unreadable one alike.
Update the preflight test to expect the record to stay pending and expire at the stop bound, fix gh_fixture's review count for extra_reviews, and make the interrupted-alive and interrupted-unreadable cases wait out the detached relaunch they rule out.
* no-mistakes(review): Fix same-second relaunches and synchronize detached tests
* no-mistakes(document): Clarify park-on-review documentation ownership
* no-mistakes(ci): Applied the GitLab incomplete-discussions handling and outage distinction in `bin/fm-park.sh`, with corresponding test updates. Verification failed: `FM_LIVE=0 bash tests/fm-park.test.sh` reported 3/32 failures; specifically, the failed-page parked-lane relaunch was not observed within the test timeout. Fix/test remains incomplete
* test(park): write real newlines in the GitLab relaunch fixtures
The fixtures used a doubled backslash, so the record held one line, parked_at could not be read, and the tick skipped the lane. The relaunch assertion could not pass, and the outage assertion passed without reaching the forge read.
* no-mistakes(review): Synchronize detached teardown timing test
* no-mistakes(ci): Added the measured `fm-park.test.sh` serial hint and provenance; moved closed-PR reporting ahead of the idle-worker check and added a busy-worker regression. Coverage guard passed (43/290 unhinted), and `FM_LIVE=0 bash tests/fm-park.test.sh` passed. `tests/fm-test-run.test.sh` reached the shard-coverage cases but timed out during a later case; `git diff --check` passed
* fix(teardown): gate GitLab merged-MR landed-work proof on park-on-review opt-in
Teardown accepts a merged GitLab merge request (squash or merge train) as
landed work only when config/park-on-review or config/auto-land exists.
Without either flag a GitLab MR goes through the same read as on main.
* no-mistakes(ci): Fixed park preflight to reject backends lacking verified stop and replacement-agent support before lease or record creation. Added a Zellij regression test; `bash tests/fm-park.test.sh` and `git diff --check` pass
* no-mistakes(ci): Fixed CI coverage by moving fm-park into the proven portable parallel set and rebalancing its lanes; retained teardown status-4 slot/path details in saved land reports with a regression test. Updated scheduling evidence docs. Verified coverage guard (27 parallel; max 850004 ms), fm-park tests, and doc audience check. fm-test-run tests passed scheduling/coverage checks but timed out at 600s before the suite completed
---------
Co-authored-by: Ian Brown <742554+zestysoft@users.noreply.github.com>
A branch protected only by review rules can report protected:true with required_status_checks:null. That is readable emptiness for classic checks, not an unreadable producer that must refuse the merge. Keep non-object non-null shapes, a missing protection object, and a protection object without the required_status_checks key as unreadable. Fixes kunchenguid#7343
mehulbhagwani
force-pushed
the
fm/u4932-null-checks
branch
from
November 13, 2026 12:27
7a4a1be to
3eb2bbf
Compare
mehulbhagwani
changed the title
 [High impact] The PR appears safe to merge. Reviews (1) · Last reviewed commit: "no-mistakes(document): Clarify null clas..." · Reviewed by Greptile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
So do the small action and the ones which are conflicting with main what is required on them, tell me what needs to be done and do that. This applies to the existing pull request #7347 ("treat null classic required checks as empty in fm-pr-merge"), authored by mehulbhagwani from the fork branch fm/u4932-null-checks on mehulbhagwani/firstmate. Kun’s latest triage (2026-10-24): green, with Kun as a merge-or-leave decision; “Nothing more is needed unless main moves under it.” Main moved and the PR is in merge conflict.
What Changed
required_status_checks: nullas no classic required checks, allowing otherwise eligible PRs to merge while still rejecting missing or malformed protection data.Risk Assessment
✅ Low: The narrow null-handling fix preserves malformed-data refusals, independent ruleset enforcement, and existing merge safeguards.
Testing
Both targeted suites passed. Live preflight and readiness checks, simulated-response cases, and the before/after regression comparison passed; CLI transcripts were saved. Null and malformed forge shapes remain untested live. No remote writes occurred.
Evidence: Live CLI evidence
Source: Live CLI evidence
Evidence: Simulated-response and before/after evidence
Source: Simulated-response and before/after evidence
Evidence: CLI verification driver
Source: CLI verification driver
Evidence: External-write guard and response injection
Source: External-write guard and response injection
Evidence: Null regression comparison
Source: Null regression comparison
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
🔧 **Rebase** - 1 issue found → auto-fixed ✅
bin/fm-pr-merge.sh- merge conflict rebasing onto refs/remotes/no-mistakes-push/fm/u4932-null-checks🔧 Fix applied.
✅ Re-checked - no issues remain.
🔧 **Review** - 1 issue found → auto-fixed ✅
bin/fm-pr-merge.sh:1141- Simplification: This new definition shadows the shared reader already sourced from bin/fm-pr-lib.sh:1144, which this diff updates with identical null handling. No intent requirement needs a second parser. Remove the local definition and retain the fix in the shared reader, so merging and review parking continue using one implementation.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bin/fm-test-run.sh tests/fm-pr-merge.test.sh tests/fm-park.test.shcompleted successfully.bin/fm-pr-merge.sh live-x1 https://github.com/cli/cli/pull/14846with real GitHub reads and external writes blocked.bin/fm-pr-merge.sh live-x1 https://github.com/cli/cli/pull/15046verified missing-check refusal.bin/fm-pr-merge.sh live-x1 https://github.com/cli/cli/pull/13788verified failed-check refusal.bin/fm-pr-merge.sh live-x1against Fix UI label mismatch in workflow template instructions github/docs#46933 and Document description, default and options for workflow_dispatch inputs github/docs#46300 verified real ruleset requirements.bin/fm-park.sh gate live-x1against cli/cli#14846 and cli/cli#15046 verified clean and missing-check outcomes.drive.shexercised injected null, missing-protection, missing-key, and malformed responses through both public commands, plus null with failed or missing ruleset checks.compare-null.sh "$PWD"reproduced the null failure at the base commit and verified success at the target commit.Write-guard refusal checks andverify-evidence.pyvalidated captured outputs, head binding, expected refusals, and unchanged live responses.Confirmed temporary validation directories were removed and the working tree remained clean at d71fd58a17df4a315550aa088dd84dd221e1b94c.✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.