Repository navigation
fix: add conditional GitHub REST reads and quota-aware sweeps - #72
Closed
MrGTV-love wants to merge 9 commits into
Closed
MrGTV-love wants to merge 9 commits into
MrGTV-love wants to merge 9 commits into
Conversation
…and contributions sweeps Add bin/fm-gh-rest.sh: GET with If-None-Match from a per-URL ETag cache under state/ (a 304 serves the cached body and is not counted against the rate limit), recording X-RateLimit-* from every response. Route fm-contributions, fm_open_loops, fm-pr-state and fm-pr-reviewers REST reads through it. Below the floor (FM_GH_RATE_FLOOR_PERCENT, default 15) the contributions poll keeps each row's last observation marked stale with the reset time, and the ledger republishes its last rows marked stale instead of publishing partial rows.
…ral regression coverage
MrGTV-love
force-pushed
the
fm/fm-github-conditional-reads
branch
from
October 9, 2026 13:22
97f644d to
505614c
Compare
…s-observer unregistration to the shared bounded-watcher fixture, preventing unrelated observer wakes from preempting PR-security scenarios. Removed redundant per-case cleanup and documented the isolation. The previously failing self-merge scenario and focused transition, retry, and concurrent-publication scenarios pass. Check-unregister tests, shell syntax, and documentation audience checks pass. The scoped CI runner progressed through the reported failure and subsequent retirement/authority cases, but the full security suite remains red at a later, separate teardown fixture failure: it records a nonexistent project, preventing nested-worktree ownership verification. That fixture and teardown implementation are unchanged from the base commit; they were left untouched. Production behavior was not changed
Owner
Author
|
Superseded by #88, which merged. |
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
I would like all bugs to be fixed so tomorrow can be focused entirely to Vernant and not problems preventning Vernant from getting built.
Context: GitHub rate limiting has repeatedly stopped fleet work (no-mistakes push and CI steps failing on gh pr view, a Vernant lane PR report published incomplete with rows missing). The measured scout report data/fm-fleet-github-graphql-budget/report.md (in the main firstmate home) found: the core (REST) bucket runs at about 80 percent of 5000/hour with no unusual load; the open-work ledger in 7 lane homes spent 88 calls per run until those homes were updated to PR #56 (done 2026-10-08); the contributions poll spends about 1150 core calls/hour re-reading 12 unchanged PRs; conditional GETs with If-None-Match that return 304 do not count against the limit; nothing in tracked code reads the remaining quota or degrades when it is low; gh api rate_limit reads wrong on this account - use X-RateLimit-* headers. GraphQL was exhausted on 2026-10-06 ~21:26Z, 2026-10-08 02:53Z and ~04:45Z by a burst of about 8x baseline whose consumer is unknown (the ledger and no-mistakes are ruled out).
What Changed
Risk Assessment
✅ Low: The change is bounded to conditional REST reads and explicit quota-stale handling, with no substantiated material defects found in the complete source review.
Testing
Inspected the change and exercised ten live CLI/API scenarios using real firstmate commands and installed gh against an isolated local REST/GraphQL service. Final scenarios passed after correcting disposable setup issues; supplemental shim and deadline regressions also passed, with longer isolated timeouts needed to reach the named deadline readers. Saved command transcripts, HTTP responses, and persisted state, then removed temporary setup. No tracked files, production credentials, or fleet sessions were changed; no full suite, lint, or static analysis was run.
Evidence: Live product command transcripts and HTTP response sequences
Source: Live product command transcripts and HTTP response sequences
Evidence: Supplemental PR-check deadline: named requests, trigger, and degraded ledger
Source: Supplemental PR-check deadline: named requests, trigger, and degraded ledger
Evidence: Supplemental PR-state deadline: named requests, trigger, and degraded ledger
Source: Supplemental PR-state deadline: named requests, trigger, and degraded ledger
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
🔧 **Rebase** - 2 issues found → auto-fixed ✅
bin/fm-test-run.sh- merge conflict rebasing onto origin/maintests/fm-contributions.test.sh- merge conflict rebasing onto origin/main🔧 Fix applied.
✅ Re-checked - no issues remain.
🔧 **Review** - 9 issues found → auto-fixed (4) ✅
bin/fm-gh-rest.sh:79- The lowest-remaining guarantee is not atomic. The six parallel contribution reads can both read remaining=800, then stage responses reporting 749 and 750 for the same reset; if the 750 writer renames last, guard permits reads despite having observed quota below the 15% floor. Serialize the entire read/merge/publication at record_rate, and prevent an older reset window from replacing a newer one. Related sites: bin/fm-gh-rest.sh:82 (minimum calculation), :89 (publication), bin/fm-contributions.sh:295-307 (concurrent writers), and tests/fm-gh-rest.test.sh:179-186 (the 'parallel' regression currently executes both requests sequentially).bin/fm-gh-rest.sh:193- The floor is checked once per command, not before every network read. If page 1 records 500/5000 remaining and supplies a next link, the pagination loop still fetches subsequent pages. A contribution wave can likewise finish successfully with low quota recorded, then execute the unconditional GraphQL head check. Enforce the floor immediately before each new request and preserve exit 75 through callers so the existing stale handling runs. Related sites: bin/fm-gh-rest.sh:206 (page fetch and exit-code conversion), bin/fm_open_loops.py:480-483 (separate guard followed by a get without --floor; must propagate QuotaLow), and bin/fm-contributions.sh:235-238 and :309 (REST-only guard leaves the subsequent GraphQL read unguarded).bin/fm-gh-rest.sh:134- A 304 always uses the cached next link and ignores the current response's Link header. Given a cached full comments page with next=null, followed by an appended comment on page 2 and a 304 for unchanged page 1 carrying rel="next", the helper returns only page 1 successfully. Contributions then advances checked_at without discovering the new signal. The new fixture permits exactly this response sequence at tests/fm-gh-rest.test.sh:40-44. Merge current pagination metadata at the shared 304 boundary while retaining cached metadata when omitted. Related sites: bin/fm-gh-rest.sh:148-151 (200 metadata parsing/storage), :208 (traversal), bin/fm-contributions.sh:295-303 and :333-335 (paginated observations), bin/fm_open_loops.py:483, bin/fm-pr-state.sh:137, and bin/fm-pr-reviewers.sh:69 (other paginated consumers).bin/fm-contributions.sh:440- Quota refusal marks only the current URL, while the next iteration still requires the full observation reserve. With two recently checked PRs, a first observation that spends more than five seconds before detecting low quota leaves fewer than 15 seconds of the default 20-second budget; the second URL is skipped without a stale error. Its snapshot row remains checked until its old timestamp expires, despite the sweep now being unable to verify it. Separate local quota-stale handling from the network-observation reservation and apply it to every remaining live owner without changing observations or checked_at. Related sites: bin/fm-contributions.sh:434 (early break), :255-269 (per-owner marking), and :475 (announcement); bin/fm-contributions.jq:53-60 explains the resulting incorrect freshness.bin/fm-contributions.sh:264- Announcement suppression compares the entire reason, which includes remaining quota. After a poll announces 700 remaining, another REST reader can record 600 in the same reset window; the next poll treats the changed reason as a new episode and prints another notification. Continuing foreground reads therefore repeatedly surface the same low-quota episode. Update each row's reason as needed, but determine announcement eligibility from the low-quota window rather than exact message equality. Related sites: bin/fm-contributions.sh:267-269 (reason update and announcement flag), :475 (emission), and bin/fm-gh-rest.sh:52-53 (mutable remaining value embedded in the reason).bin/fm-gh-rest.sh:227- Simplification: the new status command adds an unused public mode. Neither conditional reads nor the shared quota floor requires a separate bucket-dump interface, and no tracked caller uses it. Recommend removing cmd_status, its dispatch at :238, and its help entries at :7 and :23 rather than extending this additional interface.bin/fm-gh-rest.sh:220- Simplification: guard's new --resource option expands the floor interface beyond the core bucket used by every intended caller. Recording response resource headers remains useful, but configurable non-core guarding is not required by the stated purpose. Recommend removing this option and its help entry at :6; a core-only guard is the narrower sufficient interface.bin/fm-gh-rest.sh:180- Simplification: -F is an additional, undocumented alias for -f, with no typed-field behavior and no remaining tracked caller. All migrated query-field callers use -f. Conditional REST reads do not require accepting both spellings; recommend removing -F and retaining the documented -f interface.bin/fm-gh-rest.sh:204- Simplification: the new MAX_PAGES cutoff introduces unrequired truncation semantics. When the limit is reached with a next endpoint still present, the helper emits the collected prefix and exits successfully, without disclosing incomplete data. Conditional reads and quota protection do not require silently limiting result sets. Recommend removing this component, including its definition at :32, rather than preserving and hardening an additional truncation mode.🔧 Fix applied.
12 issues (4 errors, 8 warnings) still open:
bin/fm-gh-rest.sh:79- The lowest-remaining guarantee is not atomic. The six parallel contribution reads can both read remaining=800, then stage responses reporting 749 and 750 for the same reset; if the 750 writer renames last, guard permits reads despite having observed quota below the 15% floor. Serialize the entire read/merge/publication at record_rate, and prevent an older reset window from replacing a newer one. Related sites: bin/fm-gh-rest.sh:82 (minimum calculation), :89 (publication), bin/fm-contributions.sh:295-307 (concurrent writers), and tests/fm-gh-rest.test.sh:179-186 (the 'parallel' regression currently executes both requests sequentially).bin/fm-gh-rest.sh:193- The floor is checked once per command, not before every network read. If page 1 records 500/5000 remaining and supplies a next link, the pagination loop still fetches subsequent pages. A contribution wave can likewise finish successfully with low quota recorded, then execute the unconditional GraphQL head check. Enforce the floor immediately before each new request and preserve exit 75 through callers so the existing stale handling runs. Related sites: bin/fm-gh-rest.sh:206 (page fetch and exit-code conversion), bin/fm_open_loops.py:480-483 (separate guard followed by a get without --floor; must propagate QuotaLow), and bin/fm-contributions.sh:235-238 and :309 (REST-only guard leaves the subsequent GraphQL read unguarded).bin/fm-gh-rest.sh:134- A 304 always uses the cached next link and ignores the current response's Link header. Given a cached full comments page with next=null, followed by an appended comment on page 2 and a 304 for unchanged page 1 carrying rel="next", the helper returns only page 1 successfully. Contributions then advances checked_at without discovering the new signal. The new fixture permits exactly this response sequence at tests/fm-gh-rest.test.sh:40-44. Merge current pagination metadata at the shared 304 boundary while retaining cached metadata when omitted. Related sites: bin/fm-gh-rest.sh:148-151 (200 metadata parsing/storage), :208 (traversal), bin/fm-contributions.sh:295-303 and :333-335 (paginated observations), bin/fm_open_loops.py:483, bin/fm-pr-state.sh:137, and bin/fm-pr-reviewers.sh:69 (other paginated consumers).bin/fm-contributions.sh:440- Quota refusal marks only the current URL, while the next iteration still requires the full observation reserve. With two recently checked PRs, a first observation that spends more than five seconds before detecting low quota leaves fewer than 15 seconds of the default 20-second budget; the second URL is skipped without a stale error. Its snapshot row remains checked until its old timestamp expires, despite the sweep now being unable to verify it. Separate local quota-stale handling from the network-observation reservation and apply it to every remaining live owner without changing observations or checked_at. Related sites: bin/fm-contributions.sh:434 (early break), :255-269 (per-owner marking), and :475 (announcement); bin/fm-contributions.jq:53-60 explains the resulting incorrect freshness.bin/fm-contributions.sh:264- Announcement suppression compares the entire reason, which includes remaining quota. After a poll announces 700 remaining, another REST reader can record 600 in the same reset window; the next poll treats the changed reason as a new episode and prints another notification. Continuing foreground reads therefore repeatedly surface the same low-quota episode. Update each row's reason as needed, but determine announcement eligibility from the low-quota window rather than exact message equality. Related sites: bin/fm-contributions.sh:267-269 (reason update and announcement flag), :475 (emission), and bin/fm-gh-rest.sh:52-53 (mutable remaining value embedded in the reason).bin/fm-gh-rest.sh:227- Simplification: the new status command adds an unused public mode. Neither conditional reads nor the shared quota floor requires a separate bucket-dump interface, and no tracked caller uses it. Recommend removing cmd_status, its dispatch at :238, and its help entries at :7 and :23 rather than extending this additional interface.bin/fm-gh-rest.sh:220- Simplification: guard's new --resource option expands the floor interface beyond the core bucket used by every intended caller. Recording response resource headers remains useful, but configurable non-core guarding is not required by the stated purpose. Recommend removing this option and its help entry at :6; a core-only guard is the narrower sufficient interface.bin/fm-gh-rest.sh:180- Simplification: -F is an additional, undocumented alias for -f, with no typed-field behavior and no remaining tracked caller. All migrated query-field callers use -f. Conditional REST reads do not require accepting both spellings; recommend removing -F and retaining the documented -f interface.bin/fm-gh-rest.sh:204- Simplification: the new MAX_PAGES cutoff introduces unrequired truncation semantics. When the limit is reached with a next endpoint still present, the helper emits the collected prefix and exits successfully, without disclosing incomplete data. Conditional reads and quota protection do not require silently limiting result sets. Recommend removing this component, including its definition at :32, rather than preserving and hardening an additional truncation mode.bin/fm-gh-rest.sh:139- The body served for a 304 is reread from the shared cache, rather than bound to the ETag sent. Concrete concurrent sequence: request A has a delayed 200 containing reviews V2; request B receives V3 and caches E3/V3; another invocation sends E3 and receives 304, but A publishes E2/V2 before that invocation executes line 139. It successfully returns V2 despite GitHub validating V3, potentially omitting the current blocking reviewer. Round 1's cached-pagination-metadata fix left this race, and its metadata rewrite can also attach an old response's links to a replacement entry. Snapshot and validate one cache generation before requesting, serve that snapshot on 304, and prevent metadata updates from modifying another generation. Related sites: bin/fm-gh-rest.sh:116-118 (validation/ETag reads), :141-145 (pagination read/update), :163 (200 replacement); bin/fm-contributions.sh:300 (core), :304-314 (PR read wave), :342-344 (issue read wave); bin/fm_open_loops.py:482 (ledger); bin/fm-pr-state.sh:137 (reviews); bin/fm-pr-reviewers.sh:53, :69, :83 (core/files/commits).bin/fm-contributions.sh:447- Round 1's quota-stale-tail fix only latches quota degradation after an explicit request refusal. An issue observation can take six seconds, receive below-floor headers on its final single-page comments/events responses, and finish successfully. With 14 seconds left, the next iteration breaks before checking quota, leaving every tail owner's previously fresh row without the promised quota-stale reason. A PR wave that records low quota and exhausts the deadline has the same outcome because forge checks budget before quota. Check the local quota record at the shared poll boundary before budget exits, including after an observation whose budget refusal could hide the quota transition, and mark unmeasured owners without changing their observations or checked timestamps. Related sites: bin/fm-contributions.sh:230 (budget-first refusal), :342-356 (successful final issue wave), :443-454 (quota latch), :456 (budget skip), :258-278 (owner marking); docs/configuration.md:695 states the budget-independent invariant.bin/fm-contributions.sh:270- Round 1's quota-episode-announcement correction still decides episode eligibility separately for each owner. After one poll announces a low-quota window and marks existing owners, registering another task for the same PR during that window gives the new owner no prior error. The next poll sets QUOTA_ANNOUNCE again and repeats the same notification, even though existing rows already record that resource/reset episode. Determine announcement eligibility from matching saved rows across the sweep before updating individual owners; continue marking new owners and refreshing reasons silently. Related sites: bin/fm-contributions.sh:264-266 (new-owner fallback), :275 (announcement flag), :440 (per-poll initialization), :488 (emission); docs/configuration.md:695 promises one announcement per resource/reset window. tests/fm-contributions.test.sh:1239-1256 covers remaining-value and window changes but not late-owner registration.🔧 Fix applied.
✅ Re-checked - no issues remain.
tests/assets/gh-http-shim.sh:34- When gh-backend fails, the preceding!makes$?zero, so the shim exits successfully instead of propagating the failure. For example, both advisory fixtures exit 91 on unexpected requests, but their installed HTTP shim turns that into exit 0. Capture the original status using an unnegated conditional and an else branch. Related consumers: tests/lib.sh:556 installs the shim; tests/fm-contributions.test.sh:145, tests/fm-pr-state.test.sh:68, and tests/fm-pr-reviewers.test.sh:62 use it. The production helper also checks HTTP status, so this is a test-double fidelity defect, not a demonstrated production bypass.🔧 Fix applied.
1 warning still open:
tests/fm-open-loops.test.sh:1168- The pr-checks and pr-state deadline cases can pass without reaching their named reader. With fresh state, fm-gh-rest first invokes the fault-injected cat for its cache snapshot, recording attempt 1. After gh successfully returns the PR list, Round 1's response-locking change invokes cat again during lock acquisition, recording attempt 2 and signalling the collector's deadline. The assertions accept this degraded report and never verify that a check or individual PR-state request occurred. Thus these regressions could still pass if deadline propagation through those readers were broken. Install the cat fault stub only for the questions case, and require the targeted failing request to be reached before accepting the deadline result. Related sites: tests/fm-open-loops.test.sh:1116 (both affected cases), :1159-1163 (untargeted attempt counting and alarm), :1174-1179 (false-positive acceptance); bin/fm-gh-rest.sh:128 (snapshot cat), :114 (lock acquisition).🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
gh --version— installed gh 2.101.0.bash tests/fm-gh-rest.test.sh— initial 240-second command bound expired; rerun with a 1200-second bound completed successfully.python3 .live-validation/drive.py— exercised real CLI commands against a disposable HTTPS GitHub-compatible endpoint; resumed after the initial runner bound expired.env LIVE_RESUME=1 LIVE_RUN_SUFFIX=-continued python3 .live-validation/drive.pyenv LIVE_ONLY=contributions,blockers LIVE_TRACE=1 LIVE_RUN_SUFFIX=-diagnostic python3 .live-validation/drive.py— diagnosed read deadlines and corrected disposable GraphQL responses.env LIVE_ONLY=contributions,stale_owners,waves,blockers LIVE_RUN_SUFFIX=-corrected python3 .live-validation/drive.pyenv LIVE_ONLY=timingdiag,contributions LIVE_BINARY_PATH=1 LIVE_RUN_SUFFIX=-native python3 .live-validation/drive.pyenv LIVE_ONLY=contributions,stale_owners,waves LIVE_DIRECT=1 LIVE_BINARY_PATH=1 LIVE_RUN_SUFFIX=-direct python3 .live-validation/drive.pyenv LIVE_ONLY=stale_owners,waves,timingdiag LIVE_DIRECT=1 LIVE_BINARY_PATH=1 LIVE_RUN_SUFFIX=-stateproof python3 .live-validation/drive.pyenv LIVE_ONLY=contributions,waves LIVE_DIRECT=1 LIVE_BINARY_PATH=1 LIVE_RUN_SUFFIX=-stabilized python3 .live-validation/drive.pyenv LIVE_ONLY=concurrentbounds LIVE_DIRECT=1 LIVE_BINARY_PATH=1 LIVE_RUN_SUFFIX=-comparison python3 .live-validation/drive.py— compared actual six-way gh and helper reads under the existing five-second bound.env LIVE_ONLY=contributions,recovery,waves LIVE_DIRECT=1 LIVE_BINARY_PATH=1 LIVE_RUN_SUFFIX=-acceptance python3 .live-validation/drive.py— completed contribution acceptance using direct loopback HTTPS and workspace-local symlinks to real installed tools, without bypassing product deadlines.Publishedacceptance-scenarios.jsonfrom the observed successful scenario evidence; stopped local services and ranrm -rf -- .live-validation.✅ No issues found.
git diff --stat 0a787b17347d5207ee9af3b710dbadc737b19126 2463a3c0e4f563f1c62b08839ec393dca1d3ccceand focused runtime diffs.python3 .conditional-live-validation/drive.py: realfm-gh-rest.shand installedghagainst a disposable loopback HTTPS API; cache reuse, changed data, pagination, corrupt-cache fallback, concurrent replacement, HTTP errors, and quota boundaries.python3 .conditional-live-validation/consumers.py: real contribution polling, appended-page signals, durable wakes, late-owner announcement suppression, and reset recovery.python3 .conditional-live-validation/consumers.pyafter correcting disposable URL ordering and service-delay isolation.python3 .conditional-live-validation/wave-retry.py: seeded prior observations, delayed REST wave, GraphQL refusal, and preservation of every unmeasured owner's observation and timestamp.python3 .conditional-live-validation/readers.py: real ledger publication, conditional reads, retained stale rows, missing-prior degradation, reset recovery, and mid-pagination refusal.python3 .conditional-live-validation/advisory-retry.py: re-drove PR advisories after correcting the disposable GraphQL schema and isolating gh caches.bash tests/fm-gh-rest.test.shwith worktree-local temporary storage.bash tests/fm-pr-reviewers.test.sh, including backend exit-status propagation through the HTTP shim.Executed only the four deadline-reader cases fromtests/fm-open-loops.test.sh:1115-1189in isolated throwaway runners; verified named requests, matching deadline triggers, and persisted degraded JSON. Short-timeout setups expired before the check reader; longer disposable timeouts reached and passed the exact assertions.bash -x <fixture-copy>/bin/fm-gh-rest.sh get '/repos/test/project/pulls?state=open&per_page=100' --floor --paginate --slurp: diagnosed the deadline fixture's prerequisite.python3 .conditional-live-validation/collect-evidence.py && rm -rf .conditional-live-validation; confirmed no disposable validation directories remained.🔧 **Document** - 1 issue found ✅
docs/fm-test-isolation-proof.md:168- Recorded concurrency evidence predates the newly admitted fm-gh-rest suite and shared REST-reader changes. The validation phase must refresh the pr-forge family proof and record its results; generating execution evidence is outside this document phase.✅ No issues found.
🔧 **Lint** - 1 issue found → auto-fixed ✅
🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
✅ No issues found.