Skip to content

docs(adr): replace heuristic timeout allocator with statistical measurement contract - #1020

Open
seonghobae wants to merge 7 commits into
mainfrom
adr/0125-evidence-based-per-model-timeout-allocator
Open

docs(adr): replace heuristic timeout allocator with statistical measurement contract#1020
seonghobae wants to merge 7 commits into
mainfrom
adr/0125-evidence-based-per-model-timeout-allocator

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Current exact-head outcome — 2026-09-02

Current head: 12b16e8368f28de11a3f7fbb79cfb41f037fc904 on adr/0125-evidence-based-per-model-timeout-allocator. This is a Proposed design lane, not an executable timeout allocator and not merge-authoritative while exact-head checks/research prerequisites remain incomplete.

RCA

The predecessor design attempted to replace unsupported timeout constants with a sample-size-gated quantile allocator. Fresh review showed that this simply moved the no-heuristics violation into different constants and fallback policy: fixed sample floors, a fixed raw-sample retention count, percentile-class-specific estimator switching, an example EVT/POT threshold, and a model→profile/provider aggregate fallback without a fitted hierarchical model. Those rules affected whether a timeout value would be produced, but no executable statistical decision model identified those exact choices.

RED → repaired decision contract

RED contract commit 55d2d9a3a90b0f2f5246af86b6fbdd8fa36c910c rejects those fixed sample/retention/fallback rules. Repair commit 12b16e8368f28de11a3f7fbb79cfb41f037fc904 changes ADR 0125 into latency measurement without automatic timeout allocation:

  • application/Agent/Gateway model timeout remains null by default;
  • uncensored latency uses an explicit empirical distribution/quantile estimand and exact Binomial/order-statistic confidence construction for caller-supplied analysis parameters;
  • externally terminated observations are right-censored rather than treated as completions or silently dropped;
  • Kaplan–Meier plus Brookmeyer–Crowley inference is permitted only when the relevant independent-censoring assumptions are defensible, and non-identifiable requested quantiles return null;
  • no cross-model/provider borrowing occurs without a separately fitted and validated hierarchical model;
  • no EVT/POT extrapolation occurs without an executable threshold-selection/model-checking procedure;
  • no automatic timeout recommendation is emitted without a separately identified loss/utility model, authoritative standard, or validated experimental decision rule; and
  • every result binds the exact observations, analysis parameters, software/source identity, method, and deterministic provenance.

This lane does not revive #1010's enforcement wiring and does not claim that a descriptive latency percentile is timeout decision authority.

Ponytail / implementation prerequisite — still open

The repository's canonical docs/library_research.md contract requires every new subsystem design to record the existing libraries researched, the selected maintained library or stdlib alternative, the custom implementation deliberately skipped, and supporting evidence before implementation starts. ADR 0125's current branch does not yet add that latency/survival-analysis research-ledger entry.

Therefore no implementation should begin from this PR until that canonical ledger is updated. In particular, the branch must not silently hand-roll Kaplan–Meier, Brookmeyer–Crowley, exact Binomial inversion, or a tail model merely because the formulas are documented. The implementation owner must first evaluate maintained Rust-capable statistical libraries against the exact estimands, censoring semantics, uncertainty/recovery requirements, deterministic provenance, and fail-closed behavior; if no library has the required contract, that absence and the bounded custom-core decision/removal condition must be documented before code is introduced.

Exact-head verification boundary

Fresh PR-triggered runs on 12b16e8… remain non-terminal or failed at workflow startup: CodeQL PR 33627548381 is startup_failure; OSV-Scanner PR 33627547222, Security Scan 33627546628, SAST Semgrep 33627546619, Scorecard PR 33627546768, Fuzz 33627546712, Security 33627546744, and Tests 33627546693 are queued/pending. The CodeQL startup failure created no source-test success evidence, and predecessor results do not transfer.

Do not merge/auto-merge until the canonical library-research prerequisite is repaired and one unchanged exact head has terminal required evidence plus ordinary current-head review/protection. No heuristic substitute, force push, destructive rebase, self-approval, protection bypass, or fabricated status is authorized.

PR #1010 (per-model timeout admin panel) was closed by the repo owner
because its 1-14400s bounds were "picked by analogy, not research," with
an explicit reuse condition: "If a research-/standard-backed timeout
allocator with executable provenance is later implemented, the
UI/persistence work can be selectively reused behind that owner."

This ADR is that allocator. It is honest about what exists vs. what is
proposed: no latency-percentile telemetry exists in this repo today
(model_group.py collapses every call into a single EWMA scalar with no
raw retention; cost_ledger has no latency column; admin.py/orchestrator.py
carry only hardcoded/static placeholder percentile values) -- so Phase 0
(schema + retention, fed from timing calls that already exist) is a real
prerequisite before Phases 1-3's quantile math can run on real traffic.

Phase 1 gates any percentile estimate on a literature-derived sample-size
floor (Ialongo, 2019a/b; David & Nagaraja, 2003) and uses the
Harrell-Davis / trimmed-Harrell-Davis estimator (Harrell & Davis, 1982;
Akinshin, 2021, 2022) rather than a naive percentile. Phase 2 uses the
TTFT/TPOT decomposition diagnostically only, not as the timeout formula
itself (Wang et al., 2024/2025, show that shape is gameable), and
degrades gracefully for reasoning models whose completion time is
empirically bimodal rather than merely heavy-tailed (Oladri et al., 2026;
Li et al., 2025; DeepSeek-AI, 2025) by always surfacing a non-convergence
rate alongside the suggestion instead of inventing an unfounded cutoff.
Phase 3's fallback ladder ends in "no suggestion" (null), matching this
org's own standing no-fixed-wall-clock-timeout policy
(.github's docs/adr/0003 2026-08-31 amendment; contextual-orchestrator#971)
rather than inventing a conservative constant.

No MIN/MAX platform ceiling is reintroduced -- none of the three research
tracks behind this design establish one, and #971 has since made "no
bound" the library default. The admin surface stays read-only/optional,
writing through #1010's existing set_model_timeout/audit path only when
an operator opts in -- the exact reuse the closing comment invited.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 17 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 5e2d9b2a-9cbc-4187-a6f3-8e7176e00b45

📥 Commits

Reviewing files that changed from the base of the PR and between c594b68 and 862ab64.

📒 Files selected for processing (2)
  • docs/planning/adrs/0127-evidence-based-per-model-timeout-allocator.md
  • tests/test_timeout_allocator_no_heuristics_contract.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

This report is out of date. Scroll down for Devin Review's latest report on this PR.

Devin Review found 4 potential issues.

Devin Review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Subsystem research record is missing

This new telemetry and estimation subsystem lacks the mandatory library-research entry. Record evaluated libraries, the selected approach, and skipped custom code before implementation.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment thread docs/planning/adrs/0125-evidence-based-per-model-timeout-allocator.md Outdated
Comment thread docs/planning/adrs/0125-evidence-based-per-model-timeout-allocator.md Outdated
Comment thread docs/planning/adrs/0125-evidence-based-per-model-timeout-allocator.md Outdated
@seonghobae seonghobae added documentation Improvements or additions to documentation priority: medium Normal-priority or P2 work status: needs-review Open pull request requiring current-head review or checks type: docs Documentation, ADR, PRD, or technical writing labels Sep 2, 2026 — with ChatGPT Codex Connector
devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae seonghobae changed the title docs(adr): propose evidence-based per-model LLM timeout allocator docs(adr): replace heuristic timeout allocator with statistical measurement contract Sep 2, 2026

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

This report is out of date. Scroll down for Devin Review's latest report on this PR.

Devin Review found 2 new potential issues.

Devin Review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Research artifacts remain undocumented

The ADR adds no paper PDFs or redistribution rationale. The required citation-link-summary fallback is also incomplete for the order-statistics book.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +12 to +32
forbidden = (
"~20",
"~60",
"~120",
"most recent 2,000",
"borrowed aggregate",
"coarser aggregate",
"sample-size gated",
)
for token in forbidden:
assert token not in text


def test_timeout_design_fails_closed_without_identified_statistical_decision_model() -> None:
"""Require explicit uncertainty math and null output when a timeout decision is unidentified."""
text = ADR.read_text(encoding="utf-8")
assert "Binomial" in text
assert "Kaplan" in text and "Meier" in text
assert "Brookmeyer" in text and "Crowley" in text
assert "operator-supplied" in text
assert "no automatic timeout recommendation" in text.lower()

@devin-ai-integration devin-ai-integration Bot Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Lexical checks miss contract regressions

Exact phrase checks allow reworded heuristics and accept required terms anywhere. The ADR can lose its substantive guarantees while these tests still pass.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

main already carries an unrelated ADR at
docs/planning/adrs/0125-domain-neutral-rater-observation-context.md
(from #917). Merging main into this branch left two different ADRs
both numbered 0125, which is a same-number collision per this repo's
convention ("a rename, not a redesign") and fails the executable
uniqueness contract in tests/test_planning_adr_identifiers.py.
Renumber this branch's ADR to the next free number, 0127, and update
its self-reference and the matching contract test's path/title
reference. No content change beyond the identifier.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4

Copy link
Copy Markdown
Contributor Author

Brought this branch current with main (it was stuck behind against the stale base 8839081, abandoned days ago while main advanced to 212ff43+).

  • CI checks on the pre-merge head were all queued/skipped/cancelled at query time — a review-infrastructure artifact of the stale/superseded head (the startup_failure/pending runs referenced in the PR description are from an earlier, now-superseded run), not a current code failure.
  • git merge origin/main --no-edit: no textual conflicts, but the merge exposed a same-number ADR collision: main already carries docs/planning/adrs/0125-domain-neutral-rater-observation-context.md (merged via fix(rater): preserve generic v1 and add criterion-bound wrapper #917), and this branch's own docs/planning/adrs/0125-evidence-based-per-model-timeout-allocator.md collides on the 0125 identifier. tests/test_planning_adr_identifiers.py (the executable uniqueness contract referenced in CLAUDE.md) fails on this exact case. Per this repo's documented convention ("a same-number collision is a rename, not a redesign"), I renumbered this branch's ADR to the next free identifier, 0127 (0126 is already taken by another already-merged ADR), updating only the file's own title line and the matching path/title reference in tests/test_timeout_allocator_no_heuristics_contract.py — no content change.
  • Verified locally in a fresh venv (pip install --require-hashes -r requirements.lock && pip install --no-deps -e .):
    • test_planning_adr_identifiers.py, test_timeout_allocator_no_heuristics_contract.py, test_paper_contracts.py, test_repository_security_metadata.py (docs/ADR contract suites): all pass after the rename
    • Merge-diff-touched suites (test_rater_observation*.py, test_review_gateway*.py, test_admin_contract.py): 83 passed, 1 pre-existing failure (test_admin_contract.py::test_model_group_mutations_refresh_audit_events, NameError: name 'json' is not defined, already present on main HEAD, unrelated to this PR, tracked/fixed separately in fix(admin): repair test_model_group_mutations_refresh_audit_events #1029)
  • Pushed the merge commit plus the renumbering fix commit directly to adr/0125-evidence-based-per-model-timeout-allocator (no force-push). Note the branch name itself still says 0125 — left as-is since renaming the branch would change the PR's identity; only the file/test content was renumbered.

Generated by Claude Code

devin-ai-integration[bot]

This comment was marked as resolved.

seonghobae pushed a commit that referenced this pull request Sep 2, 2026
)

PR #1012 and PR #1020 both independently renumbered their new ADR to 0127
after each collided separately with main's #917. That left both PRs
claiming 0127, which would collide again on merge.

Since main currently tops out at 0126, 0127 stays genuinely free and PR
#1020 keeps it; this PR moves to 0128, the next free number, verified
against current main and cross-checked against PR #1020's own branch for
any other numbers it introduces.

Renames docs/planning/adrs/0127-openai-chat-responses-shape-translation.md
to 0128-openai-chat-responses-shape-translation.md and updates every
in-repo reference (ADR front matter id, provider_api_version.py and
chat_responses_shape.py docstrings, two orchestrator.py comments, and the
CHANGELOG entry).

Verified tests/test_planning_adr_identifiers.py passes standalone and
also passes with PR #1020's 0127 ADR file temporarily copied in alongside
this branch's 0128 file (coexistence simulation only, not merged).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
seonghobae pushed a commit that referenced this pull request Sep 2, 2026
PR #1020 (#1020,
`docs/planning/adrs/0127-evidence-based-per-model-timeout-allocator.md`)
independently claimed ADR number 0127 before this PR was created, and
PR #1012 (#1012) already holds 0128 from an earlier same-day collision
fix. Per the ADR-uniqueness convention in this repo's CLAUDE.md
("a same-number collision is a rename, not a redesign") and the
tie-breaker already applied once today (the more-recently-created/
updated PR bumps further), this PR's ADR renumbers to 0129 -- verified
free against origin/main and all 38 currently open PRs by walking each
PR's head tree for new docs/planning/adrs/*.md files not present on
main.

- Rename docs/planning/adrs/0127-canonical-immutable-release.md to
  0129-canonical-immutable-release.md; update its front-matter id.
- Update every in-repo reference to the old number:
  .github/workflows/release.yml, tests/test_release_workflow_contract.py,
  conductor/tracks.md, CHANGELOG.md, docs/product-technical-gap-baseline.md,
  docs/RELEASING.md.

Verified: tests/test_release_notes.py, tests/test_release_workflow_contract.py,
tests/test_planning_adr_identifiers.py, tests/test_product_planning_contract.py
(30 passed); python -m interrogate -c pyproject.toml . (100.0%); a scratch-
directory side-by-side check against PR #1020's 0127 file confirmed no
residual collision at the new number.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
seonghobae pushed a commit that referenced this pull request Sep 2, 2026
PR #972 (feat(discovery): record parallel tool-call capability and
exclude single-tool models from orchestrator/free) independently added
docs/planning/adrs/0042-parallel-tool-call-capability.md, colliding
with this PR's 0042-opencode-go-provider-discovery.md -- neither
number exists on main yet. Per this repo's CLAUDE.md, "a same-number
collision is a rename, not a redesign," and PR #972 was updated
earlier than this PR, so this PR's ADR renumbers instead.

0130 is the next free number after checking docs/planning/adrs/ on
current origin/main (highest: 0126) and every other open PR's added
ADR files (0127 PR #1020, 0128 PR #1012, 0129 PR #1030, plus PR #972's
untouched 0042).

Renamed docs/planning/adrs/0042-opencode-go-provider-discovery.md to
0130-opencode-go-provider-discovery.md, updated its front-matter id,
and updated the three in-repo prose references to "ADR 0042" for this
ADR (contextual_orchestrator/model_discovery.py comment,
tests/test_model_discovery.py docstring, docs/kv-credentials.md,
CHANGELOG.d/opencode-go-provider-discovery.md). Left the coincidental
"0042" substring in docs/planning/adrs/0004-pr-review-merge-loop.md
(part of an unrelated git SHA) untouched.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
Devin's review found that running this test file directly (python
tests/test_timeout_allocator_no_heuristics_contract.py) exits successfully
without invoking either contract test -- pytest collects them fine, but
the repository's own documented per-file workflow ("each test file is a
directly runnable script", CLAUDE.md/README.md "Check") validates nothing
for this file specifically, unlike every sibling contract test (e.g.
test_paper_contracts.py, test_repository_security_metadata.py), which end
with an `if __name__ == "__main__":` block that calls each test and prints
"ok".

RED confirmed: reverted the change and reran `python3
tests/test_timeout_allocator_no_heuristics_contract.py` directly -- exit 0,
no output, neither assertion ever executed.

GREEN: same direct invocation now prints "ok" (both contracts actually ran);
`pytest tests/test_timeout_allocator_no_heuristics_contract.py -q` still
2 passed, no behavior change under pytest collection.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4

Copy link
Copy Markdown
Contributor Author

Fixed the one concrete, unaddressed finding: contract file wasn't runnable as a direct script

Triaged this PR's open threads. Most of the remaining open ones (missing docs/library_research.md ledger entry, paper-PDF/citation grounding for the order-statistics book, the lexical-check robustness note) are explicitly acknowledged as open prerequisites in this PR's own description ("Ponytail / implementation prerequisite — still open... no implementation should begin from this PR until that canonical ledger is updated") — this PR is self-described as a non-merge-authoritative "Proposed design lane," so those are correctly left to the owner rather than bundled into an unrelated push.

One finding was concrete and independently verifiable, though: Devin flagged that running tests/test_timeout_allocator_no_heuristics_contract.py directly exits successfully without invoking either contract test. Confirmed real — unlike every sibling contract test in this repo (test_paper_contracts.py, test_repository_security_metadata.py, etc.), this file was missing the if __name__ == "__main__": block that actually calls each test function, so it silently validated nothing when run the way this repo's own documented workflow ("each test file is a directly runnable script") calls for.

Fix (commit 63ecf03b): added the standard if __name__ == "__main__": ... print("ok") block, matching this repo's existing idiom exactly.

RED confirmed: reverted the change and ran python3 tests/test_timeout_allocator_no_heuristics_contract.py directly — exit 0, zero output, neither assertion ever ran.

GREEN: same direct invocation now prints ok. pytest tests/test_timeout_allocator_no_heuristics_contract.py -q — 2 passed, unchanged under pytest collection (this was never a pytest-collection bug, only a direct-execution one).

Pushed non-force on top of the existing branch (0806973e63ecf03b).


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Autonomous loop note: exact head 63ecf03b31f8ca9be60160e9c4d094cc6819f009 (base main@212ff437, pre-dates the json import fix) failed three checks:

  • Full unit and contract suite (run 33692092712): known tests/test_admin_contract.py::test_model_group_mutations_refresh_audit_events - NameError: name 'json' is not defined — already fixed on main (212ff43739a4348e); not caused by this PR's docs-only diff.
  • CodeQL analysis (run 33692092707): "CodeQL job status was configuration error" — the known org-wide advanced-configuration-vs-default-setup conflict tracked centrally in .github.
  • noema-review (run 33692092251): 502 Bad Gateway, phase=connecting — transient gateway infra. Re-ran the failed job.

Called update_pull_request_branch to merge current main into this branch so the stale-fixture failure clears; this is routine CI hygiene only and does not constitute merge authorization — the PR body's own "not merge-authoritative" / library-research-prerequisite gate stands untouched.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation priority: medium Normal-priority or P2 work status: needs-review Open pull request requiring current-head review or checks type: docs Documentation, ADR, PRD, or technical writing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants