chore(tech-debt): collapse five hand-copied patterns onto one definition each (#16415, #13579) by mrveiss · Pull Request #17314 · mrveiss/AutoBot-AI

mrveiss · 2026-09-23T15:23:03Z

Thinking Path

Four tech-debt issues, one branch, one CI run. Three are the same shape — N hand-written copies of one thing — and the fourth is a workflow that measured something for months and stored it nowhere.

The theme that emerged was not the cleanup. It was that the guard meant to find this duplication could not see most of it. Three separate measurements in this branch contradicted an issue's own premise, and each one is recorded below rather than quietly worked around.

What Changed

#16415 — the registration contract is stated once

Seven env_registry_* siblings restated the same import-as-side-effect contract. env_registry_llc.py already pointed at a sibling's copy and named this issue as the one that would consolidate it.

The contract could not move into env_registry.py — that file is at its grandfathered ceiling and may not grow (#14236) — and it did not need to: that file already states it from the importer's side. So this is deletion plus a reference, and nothing grows.

The first attempt did nothing, and measuring is the only reason I know that. Replacing the paragraph with a four-line reference took the clone pairs from 24/18/16 lines to 23/17/15 and eliminated none. jscpd needs 8 lines and 70 tokens, and the clone spans the whole docstring tail plus an irreducible code tail — closing quotes, from __future__, the registry import, the opening register_env_var(EnvVarSpec( that every sibling must have. Shortening prose only shortens the clone.

What works is cutting enough shared prose that the remaining identical run falls under the token floor, which the code tail alone never reaches.

autobot_shared scope:  9 clones -> 4 clones
env_registry pairs:    5        -> 0          (the acceptance criterion)
SLM scope total:       2918     -> 2831 duplicated lines

No behaviour change, asserted rather than assumed: base and branch both import to 257 registered variables with identical names.

The pin drops 2945 → 2831. Recorded beside it: the same run measured origin/main at 2918, so the 2945 pin had carried 27 lines of slack since 2026-09-11. The gate exits 0 when a count is under its pin, so a fall in duplication is never noticed and a later PR can spend it — #16324's exact shape, observed rather than argued.

#17313 — the durations now actually land

test-durations.yml was named "Measure and store per-test durations". Its last step was upload-artifact, and permissions: contents: read meant it could not commit. Every weekly run did 2–3h of real work and threw the result away.

.test_durations      last committed 2026-08-17  (37 days)  by #14413
.test_durations_slm  last committed 2026-08-03  (51 days)  by #13314

Both last written by ordinary feature PRs, never by the workflow that owns them. Across the last 100 workflow runs in this repository it has no completed run at all.

Not a breakage — pytest-split gives unknown tests the mean, so the split decays rather than fails. The cost is gradual shard imbalance across 12 python-suite and 12 coverage shards. It did not cause the shard 11/12 and 12/12 failures on #17303; those were a stale fixture dependency list and a reach floor, both separately diagnosed.

The fix is a separate land job, not contents: write on the existing one. That obvious version would put a repository-write token in the same job that runs the entire test suite, reachable by any test, fixture or transitive import. store-durations stays read-only and hands over an artifact; land never runs a test.

It fails closed on the way in: an empty file, or one under 100 timings, is refused rather than committed over good data. And it distinguishes the two states the old workflow could not — "completed, nothing changed" now says so instead of looking identical to "never landed anything".

#17166 — one filtered-install command, not five

Five roles hand-wrote the same build-filtered-requirements.sh invocation in role_registry.py. These are deploy commands: nothing executes them until a real deploy, so a wrong character survives every test in the suite.

Proven unchanged, not asserted. All ten post_sync_cmd values were extracted from the AST before and after — evaluating the f-strings, and on the after side actually calling the helper — and diffed: identical, byte for byte. The five strings are now pinned as literals in a test, so it is an identity the suite enforces rather than one this description claims.

tag is a parameter rather than derived from the working directory, because backend writes /tmp/requirements-filtered-slm.txt while slm-backend writes ...-slm-backend.txt. That reads like a typo and is not one; deriving it — the obvious simplification — silently changes two live deploy commands. A test pins the asymmetry so the next reader does not "fix" it.

#13579 — one translation from a rejected path to an HTTP refusal

Six call sites did this by hand and disagreed: three 400, two 403, one 404. The issue said five — api/data_storage.py was not on its list. Found by walking every module that calls either validator rather than trusting the enumeration.

The status is 400, and the reasoning is already recorded. THREAT_MODEL holds the ruling for session ownership (#14012): creating over an existing id returns 409 identically for "owned by someone else" and "no recorded owner", because a 403 on the first would confirm who owns it. Same shape. validate_path refuses for several reasons — decoding failure, .., absolute, drive qualifier, containment — and deliberately does not say which. 403 breaks that: it asserts "a real target you may not have", which a lexical validator never established, and it lets a caller separate "outside the roots" from "malformed" and map the boundary one request at a time.

The detail is a fixed "Invalid path". The offending value is logged and returned to nobody. api/files.py had been answering 403 "Path outside allowed directories", which named the boundary it had hit.

One site is deliberately not converged, and it was already right. transcriber/routes/recordings.py answers 404, and the next line returns 404 for a missing file — so anything else for an out-of-bounds path would separate "not yours" from "not there". 404 for both is strictly more opaque than the shared default, so folding it in would have been a regression. It keeps its own handler with the reason at the call site, rather than the helper growing a status-code parameter that reopens the per-endpoint decision it exists to close.

The helper sits beside path_validator, not in it: that module has no framework import and should keep none, or every consumer of validation takes a FastAPI dependency.

#16908 — nothing stopped two modules owning the same route

FastAPI matches in registration order, so the first wins and the second is unreachable. Nothing checked which one won; the answer was decided by the order of two lists in two files.

The issue's table is stale. All four pairs it lists are resolved — feature_routers.py:552 records the scheduler shadowing being deleted under this very issue. Re-measuring rather than trusting the table found a different collision it does not mention:

POST /api/voice/realtime/tools/call
  api.voice:realtime_router      (core, registered first -- serves)
  api.realtime_session:router    (feature -- dead)

Two scanner bugs had to be fixed before that verdict meant anything, and both would otherwise have been reported as fact:

Attribution by file, not by router object. from api.voice import realtime_router as voice_realtime_router registers realtime_router, not the module's router. api.voice registers both at /voice, so attributing every decorator in the file to each registration made the module collide with itself — noise that buried the real cross-module collision.
api.redis_mcp is a package, not a module file. The scan could not resolve it and said so; counting it as "no routes" would be the familiar defect of reading "could not look" as "found nothing". The guard now fails on an unresolvable module rather than skipping it.

before: 1522 pairs, 1 unresolved, 1 duplicate (the wrong one)
after:  1518 pairs, 0 unresolved, 1 duplicate (real)

The collision is recorded, not resolved, and that is deliberate. The two handlers differ in declared auth: the winner takes Depends(get_current_user) and checks is_admin_role (#12717); the dead one declares neither and passes a session_id through realtime_mcp_bridge, which the winner has no equivalent for. Deleting the dead one drops a capability; re-pathing it makes a handler with no declared auth reachable. That is a judgement about a voice feature's security surface, not a cleanup, so it is asked on the issue.

Stated precisely: I could not establish from app_factory.py whether other middleware would cover the dead handler, so "no declared auth" is what the decorators say — not a claim that it is unauthenticated.

It is the same accidental-correctness shape the issue already noted for knowledge_search: the safe implementation wins, and nothing makes it win.

Verification

Every guard here was mutation-checked. A test that has not failed on purpose has not been shown to test anything.

#16415  reference left at 4 lines           -> 9 clones, unchanged (why the fix went further)
#17166  tag derived from workdir            -> 2 FAILED
        a sixth hand-written call           -> FAILED ..._no_role_still_hand_writes_...
#17313  landing job deleted                 -> 4 FAILED
        no longer runs on the cron          -> FAILED ..._runs_on_the_scheduled_refresh
        suite job handed a write token      -> FAILED ..._never_holds_a_write_token
        emptiness check removed             -> FAILED ..._empty_or_truncated_..._refused
#13579  status flipped to 403               -> FAILED test_the_shared_status_is_not_403
        detail echoes the path              -> FAILED 4 tests
#16908  a planted duplicate route           -> FAILED, naming BOTH modules
        a stale baseline entry              -> FAILED ..._only_lists_duplicates_that_still_exist

Stated plainly: the 403 mutation kills one test, not several. The behavioural assertions compare against the constant, so they follow it. That is the intended design — the constant is the decision and one test pins the decision — but it is not broad coverage and should not read as such.

Identity checks, which are the load-bearing ones here:

#17166  all 10 post_sync_cmd values, AST-extracted before and after  -> IDENTICAL
#16415  registered env vars, base vs branch                          -> 257 == 257, same names
#13579  hand-written translations                                    -> 6 -> 1 (the documented 404)

Ratchets lowered in the same change, because an unlowered ceiling re-licenses the lines just cut:

SLM_MAX_DUP_LINES                      2945 -> 2831
autobot-slm-backend/services/role_registry.py   715 -> 713
autobot-backend/api/files.py                   1368 -> 1365
autobot-backend/api/logs.py                    1029 -> 1027

What this does NOT do

#17166's third criterion cannot be ticked, and I left it unchecked. It asks for the duplication pin to be "lowered to reflect the reduction". Measured on the SLM scope with the workflow's exact flags, before and after:

2831 duplicated lines  ->  2831 duplicated lines

The five blocks were never clones. They differ in working directory and temp-file name, so no 8-line/70-token identical run existed between them. Five near-copies of one deploy command, invisible to the guard whose job is duplication. That is why this says Refs #17166 and not Closes.

The general case is filed as #17312: the four redaction implementations are 1164 lines across four modules and 0 clone lines between them. The metric rewards divergence — letting a clone rot until it no longer matches makes the number go down.

#16908's collision is recorded, not fixed — the guard stops the next one, and the existing pair needs a decision about a voice endpoint's auth surface. Hence Refs.

#17313's regeneration criterion is also outstanding. Committing refreshed durations needs a completed measurement run, and the workflow has none. The automation is what this PR delivers; the data follows from the first scheduled run after it merges. Hence Refs #17313.

Model Used

Claude Opus 5 (1M context)

Closes #16415
Closes #13579
Refs #17166
Refs #17313
Refs #16324
Refs #16908
Refs #17312

Summary by CodeRabbit

New Features
- Scheduled and manually triggered runs can now automatically open or update a pull request with refreshed test-duration data.
- Path-validation refusals now use a consistent error response without revealing the rejected path.
Bug Fixes
- File and path operations now return consistent responses when paths are invalid or disallowed.
Chores
- Tightened the duplication threshold and added checks for duplicate API routes and test-duration freshness.

Seven env_registry_* siblings restated the same import-as-side-effect contract, and env_registry_llc.py already pointed at a sibling's copy of it while naming #16415 as the issue that would consolidate it. The contract could not move INTO env_registry.py -- that file is at its grandfathered size ceiling and may not grow (#14236) -- but it did not need to: env_registry.py already states it from the importer's side, beside the imports it performs. The siblings were restating it from the other side. So this is deletion plus a reference, and nothing grows. MEASURED, because the first attempt did nothing. Replacing the paragraph with a four-line reference took the clone pairs from 24/18/16 lines down to 23/17/15 and eliminated NONE of them: jscpd's threshold is 8 lines AND 70 tokens, and the clone spans the whole docstring tail plus an irreducible code tail -- the closing quotes, `from __future__`, the registry import, and the opening `register_env_var(EnvVarSpec(` that every sibling must have. Shortening prose only shortens the clone. What works is removing enough shared PROSE that the remaining identical run falls under the 70-token floor, which the code tail alone does not reach. The reference is now one line, and the #15710 provenance paragraph that backend_services restated verbatim from agent_runtime is collapsed to a reference too. autobot_shared scope: 9 clones -> 4 clones env_registry pairs: 5 -> 0 (the acceptance criterion) SLM scope total: 2918 -> 2831 duplicated lines No behaviour change, asserted rather than assumed: base and branch both import to 257 registered variables with identical names. Pin lowered 2945 -> 2831. Recorded alongside it: the same local run measured origin/main at 2918, so the 2945 pin had carried 27 lines of slack since 2026-09-11. The gate exits 0 when a count is UNDER its pin, so a fall in duplication is never noticed and a later PR can spend it -- that is #16324's exact shape, observed here rather than argued. Closes #16415 Refs #16324

`test-durations.yml` was named "Measure and store per-test durations". Its last step was upload-artifact. Nothing committed anything, so every weekly run did 2-3h of real work and threw the result away: .test_durations last committed 2026-08-17 (37 days) by #14413 .test_durations_slm last committed 2026-08-03 (51 days) by #13314 Both last written by ordinary feature PRs, never by the workflow that owns them. Across the last 100 workflow runs in this repository it has no completed run at all. Not a breakage -- pytest-split assigns the mean duration to any test it has no timing for, so the split decays rather than failing. The cost is gradual shard imbalance across 12 python-suite and 12 coverage shards, growing with every test added since August. It did NOT cause the shard 11/12 and 12/12 failures on #17303; those were a stale fixture dependency list and a reach floor, both separately diagnosed. A SEPARATE `land` job, not `contents: write` on the existing one. The obvious fix would put a repository-write token in the same job that runs the entire test suite, within reach of any test, fixture or transitive import. `store-durations` stays read-only and hands over an artifact; `land` never runs a test. It fails closed on the way in: an empty file, or one with fewer than 100 timings, is refused rather than committed over good data -- pytest-split would then give every test the mean, which is worse than the stale file it replaced. And it distinguishes the two states the old workflow could not: "completed, nothing changed" now says so, instead of looking identical from outside to "never landed anything". The measuring job is renamed to "Measure per-test durations", because that is what it does. Guards, each mutation-verified: landing job deleted -> 4 FAILED no longer runs on the cron -> FAILED ..._runs_on_the_scheduled_refresh suite job handed a write token -> FAILED ..._never_holds_a_write_token emptiness check removed -> FAILED ..._empty_or_truncated_..._refused The age check uses a 90-day rot floor rather than a tight one: a threshold that fires on ordinary lateness gets muted, and a muted guard is worse than none. The automation is pinned alongside it, because an age check on its own would take three months to notice the landing job had been deleted. Refs #17313

…17166) Five roles hand-wrote the same `build-filtered-requirements.sh` invocation in role_registry.py. These are deploy commands: nothing executes them until a real deploy, so a wrong character survives every test in the suite. They now come from one `_filtered_pip_install` helper. PROVEN unchanged, not asserted. All ten `post_sync_cmd` values were extracted from the AST before and after -- evaluating the f-strings, and on the after side actually calling the helper -- and diffed: IDENTICAL -- all 10 post_sync_cmd values byte-for-byte unchanged The five strings are pinned as literals in a test, so the refactor is an identity the suite enforces rather than one this message claims. `tag` is an argument rather than derived from the working directory: the backend role writes /tmp/requirements-filtered-slm.txt while slm-backend writes ...-slm-backend.txt. That reads like a typo and is not one; deriving it -- the obvious simplification -- silently changes two live deploy commands. A test pins the oddity so the next reader does not "fix" it. Mutation-verified: tag derived from workdir -> 2 FAILED a sixth hand-written call -> FAILED ..._no_role_still_hand_writes_... The new tests are their own module: test_role_registry.py is grandfathered at 846 lines and may not grow, and it owns 100 lines of sys.modules surgery to get the real registry past conftest's stubs. The new module imports that bootstrap rather than copying it, which would have been this issue's own defect in the test tree. role_registry.py came out at 713 against a 715 ceiling, so the ceiling is lowered to 713 in both places that record it -- the ratchet only turns down, and an unlowered ceiling re-licenses the lines just cut. AC3 OF THE ISSUE CANNOT BE TICKED, and that is the finding. It asks for the duplication-guard pin to be "lowered in the same PR to reflect the reduction". There is no reduction to reflect. Measured on the SLM scope with the workflow's exact flags, before and after: 2831 duplicated lines -> 2831 duplicated lines The five blocks were never clones. They differ in working directory and temp-file name, so no 8-line/70-token identical run existed and jscpd never counted them. Five near-copies of one command, invisible to the guard whose job is duplication -- the same shape as #16415, where prose had to be cut past the token floor before the pin moved at all, and the general case filed as #17312. So the criterion is left unticked with its measurement rather than ticked to tidy the issue, and this says Refs, not Closes. Refs #17166, #17312

#13579) Six call sites turned a path-validator ValueError into an HTTP error by hand, and disagreed: three 400, two 403, one 404. The status a caller saw for the same rejection depended on which endpoint they hit. The issue said five. It is six -- api/data_storage.py was not on its list. Counted by walking every module that calls either validator and matching `except ValueError` windows that raise, rather than by trusting the enumeration. THE STATUS IS 400, and the reasoning is not new. THREAT_MODEL already records the ruling for session ownership (#14012): creating over an existing id returns 409 identically for "owned by someone else" and "no recorded owner", because a 403 on the first would confirm who owns it. Same shape here. `validate_path` refuses for several reasons -- decoding failure, `..`, absolute, drive qualifier, containment -- and deliberately does not say which; containment is the sole authority and the rest are defence in depth. 403 breaks that: it asserts "a real target you may not have", which a lexical validator never established, and it lets a caller separate "outside the roots" from "malformed" and map the boundary one request at a time. 400 is true of every refusal reason and distinguishes none of them. It was already the majority, three of six. The detail is a fixed "Invalid path". The offending value is logged and returned to nobody: it reaches the client, and echoing it answers the probe it was sent to make. api/files.py was returning 403 "Path outside allowed directories", which said precisely which boundary had been hit. ONE SITE IS NOT CONVERGED, deliberately, and this is the interesting half. transcriber/routes/recordings.py answers 404 -- and it was already right. The next line returns 404 for a missing file, so answering anything else for an out-of-bounds path would separate "not yours" from "not there" and leak the upload directory's shape. 404 for both is strictly MORE opaque than the shared default, so folding it in would have been a regression. It keeps its own handler with the reason written at the call site, rather than the helper growing a status-code parameter that would re-open the per-endpoint decision the helper exists to close. The helper lives beside path_validator, not in it: path_validator has no framework import and should keep none, or every consumer of validation takes a FastAPI dependency. Verified: hand-written translations 6 -> 1 (the documented 404) status flipped to 403 -> FAILED test_the_shared_status_is_not_403 detail echoes the path -> FAILED 4 tests pyflakes on all 5 touched modules -> clean Both files that shrank have their ceilings lowered in the same change -- files.py 1368 -> 1365, logs.py 1029 -> 1027 -- because an unlowered ceiling re-licenses the lines just cut. One test corrected mid-flight rather than shipped wrong: `~/secrets` is NOT rejected, and should not be. `~` is shell expansion, not a path feature, so it is an ordinary directory name that stays inside the base. `resolve_within_sandbox` does forbid it -- a different function with a stricter contract -- and the distinction is easy to misread as a gap. Closes #13579

Registration order was the only thing deciding which of two implementations served a shared (method, path), and nothing checked it. FastAPI matches in order, so the first wins and the second is unreachable -- dead code that reads as a live endpoint. The issue's table is STALE, which is the first finding. All four pairs it lists are resolved; feature_routers.py:552 records the scheduler shadowing being deleted under this issue. Re-measuring rather than trusting the table found a different collision the issue does not mention: POST /api/voice/realtime/tools/call api.voice:realtime_router (core, registered first -- serves) api.realtime_session:router (feature -- dead) Two scanner bugs had to be fixed before that verdict meant anything, and both would otherwise have been reported as fact: * Attribution by FILE, not by router OBJECT. `from api.voice import realtime_router as voice_realtime_router` registers realtime_router, not the module's `router`. api.voice registers BOTH at /voice, so attributing every decorator in the file to each registration made the module collide with itself -- noise that buried the real cross-module collision. * `api.redis_mcp` is a package, not a module file. The scan could not resolve it and said so; counting it as "no routes" would have been the familiar defect of reading "could not look" as "found nothing". The guard now fails on an unresolvable module rather than skipping it. before: 1522 pairs, 1 unresolved, 1 duplicate (the wrong one) after: 1518 pairs, 0 unresolved, 1 duplicate (real) THE COLLISION IS RECORDED, NOT RESOLVED, and that is deliberate. The two handlers are different implementations and they differ in DECLARED AUTH: the winner takes `Depends(get_current_user)` and checks `is_admin_role` (#12717); the dead one declares neither and passes a `session_id` through `realtime_mcp_bridge`, which the winner has no equivalent for. Deleting the dead one drops a capability; re-pathing it makes a handler with no declared auth reachable. That is a judgement about a voice feature's security surface, not a cleanup, so it is asked on the issue rather than decided here. Stated precisely: I could not establish from app_factory.py whether other middleware would cover the dead handler, so "no declared auth" is what the decorators say, not a claim that it is unauthenticated. Same accidental-correctness shape the issue already noted for knowledge_search: the safe implementation wins, and nothing makes it win. Mutation-proved, as the issue asks: a planted duplicate route -> FAILED, naming BOTH modules: "GET /api/voice/realtime/tools registered by api.voice:realtime_router, api.voice:router" a stale baseline entry -> FAILED ..._only_lists_duplicates_that_still_exist The baseline is shrink-only: entries leave when fixed and none may be added. Refs #16908

coderabbitai · 2026-09-23T15:23:32Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 10 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: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: f20190e9-7037-46aa-81e6-1baba78feee1

📥 Commits

Reviewing files that changed from the base of the PR and between 83fac12 and c80722f.

📒 Files selected for processing (14)

.github/filters/python-paths.yml
.github/workflows/test-durations.yml
autobot-backend/api/data_storage.py
autobot-backend/api/files.py
autobot-backend/api/logs.py
autobot-backend/api/merge_conflict_resolution.py
autobot-slm-backend/tests/services/test_code_sync_preserves_install_14275.py
autobot_shared/security/path_http.py
autobot_shared/security/path_http_test.py
docs/features/PHASE_8_ENHANCED_INTERFACE.md
repo_tests/durations_are_landed_and_fresh_17313_test.py
repo_tests/hooks_path_override_15961_test.py
repo_tests/import_hermeticity_test.py
repo_tests/no_duplicate_route_registration_16908_test.py

📝 Walkthrough

Walkthrough

This PR adds automated landing for test-duration files, introduces shared HTTP path-refusal helpers, and updates backend callers. It also factors deployment command construction, adds duplicate-route registration checks, shortens environment registry docstrings, and lowers the duplication guard limit.

Changes

Environment registry documentation

Layer / File(s)	Summary
Registry contract references and duplication pin `.github/workflows/duplication-guard.yml`, `autobot_shared/env_registry_*.py`	Registry sibling docstrings refer to `env_registry` for the registration contract. The SLM duplication limit changes from 2945 to 2831.

Test-duration landing automation

Layer / File(s)	Summary
Measure and land duration files `.github/workflows/test-durations.yml`, `repo_tests/durations_are_landed_and_fresh_17313_test.py`	The workflow adds a scheduled and manually triggered landing job. It checks that duration files exist and contain enough timings, then creates or updates a pull request when the files change. Tests check the job conditions, permissions, and duration-file freshness.

Shared HTTP path refusal

Layer / File(s)	Summary
Shared path-refusal helpers `autobot_shared/security/path_http.py`, `autobot_shared/security/path_http_test.py`	New helpers convert refused absolute and relative paths to the shared 400 response with detail `Invalid path`. Tests cover refusals, accepted paths, and response details.
Backend path-validation callers `autobot-backend/api/data_storage.py`, `autobot-backend/api/files.py`, `autobot-backend/api/logs.py`, `autobot-backend/api/merge_conflict_resolution.py`, `autobot-backend/api/merge_conflict_resolution_test.py`, `autobot-backend/transcriber/routes/recordings.py`	Backend callers use the shared helpers instead of local error translations. The recordings route retains its local 404 handling, and tests check the shared response and remaining local translations.

Deployment command construction

Layer / File(s)	Summary
Filtered-requirements install commands `autobot-slm-backend/services/role_registry.py`, `autobot-slm-backend/tests/services/test_role_registry_post_sync_17166.py`	Five role definitions use a shared command builder. Tests check the generated commands, role-specific temporary-file names, and the requirements-builder invocation count.

Duplicate route-registration guard

Layer / File(s)	Summary
Route discovery and duplicate checks `repo_tests/no_duplicate_route_registration_16908_test.py`	A new test parses registered routers and their routes, then checks for duplicate method and path pairs, unresolved modules, and stale allow-list entries.

Merge Risk: 🟡 Moderate · up to 83fac

The runtime backend and deployment changes look safe. The new automation for landing test durations will stop working after its first run or first merged PR, and its freshness check can pass on stale data. The new duplicate-route guard does not see several router registrations, so it misses existing duplicates. Fix these before relying on either safeguard.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The pull request also changes test-duration landing workflows, deployment-command definitions, and duplicate-route detection. These changes address referenced issues such as [`#17166`], [`#17313`], and [#…	Remove the unrelated workflow, deployment-command, and route-registration changes from this pull request, or assess them under their own directly linked issues in a separate pull request.
Docstring Coverage	⚠️ Warning	Docstring coverage is 74.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 39 functions across 20 files. (2 skipped:…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarises the main change: consolidating repeated patterns into shared definitions. It is specific, concise, and related to the pull request objectives.
Linked Issues check	✅ Passed	The changes satisfy the coding requirements in [`#16415`] and [`#13579`]. The environment-registry siblings now refer to one registration contract. The duplication limit decreases from 2945 to 2831. The n…

Full details: Out of Scope Changes check

Explanation

The pull request also changes test-duration landing workflows, deployment-command definitions, and duplicate-route detection. These changes address referenced issues such as [#17166], [#17313], and [#16908], but they do not implement [#16415] or [#13579]. They are outside the scope of the two directly linked issues assessed here.

Full details: Docstring Coverage

Explanation

Docstring coverage is 74.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 39 functions across 20 files. (2 skipped: 2 unsupported.)

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

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.}

github-actions · 2026-09-23T15:25:25Z

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

actionlint failed the branch: SC2016, "expressions don't expand in single quotes", twice. The lines it names carry Markdown backticks -- `test-durations.yml`, `.test_durations` -- and shellcheck reads a backtick inside single quotes as command substitution that will not expand. It is right about the shell and wrong about the intent, and the intent is what has to change: printf with per-line quoting is the wrong tool for prose that contains shell metacharacters. A quoted heredoc has no expansion at all, so the body can hold backticks, and the one dynamic value is a __RUN_URL__ placeholder substituted after. Two details that had to be measured rather than assumed, because both fail silently into a malformed PR body: * The first attempt put the heredoc at column 0, which terminates the YAML block scalar. The file stopped parsing -- caught locally, not in CI. * The body is indented to sit inside that block and the indent is stripped back off. `sed 's/^ //'` was wrong: the heredoc lines carry 14 raw spaces against the block's base of 10, so 4 survive parsing and 2 would have remained on every line. Verified by rendering the body and checking the four required headings match exactly, rather than by reading the widths off the screen. Refs #17313

coderabbitai

Actionable comments posted: 9

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: b5e5d163-f1a3-4355-8c66-0d810f8fc604

📥 Commits

Reviewing files that changed from the base of the PR and between 6680ed7 and 83fac12.

⛔ Files ignored due to path filters (2)

repo_tests/python_file_size_ratchet_baseline.py is excluded by !repo_tests/python_file_size_ratchet_baseline.py
scripts/python_file_size_known_large.py is excluded by !scripts/python_file_size_known_large.py

📒 Files selected for processing (22)

.github/workflows/duplication-guard.yml
.github/workflows/test-durations.yml
autobot-backend/api/data_storage.py
autobot-backend/api/files.py
autobot-backend/api/logs.py
autobot-backend/api/merge_conflict_resolution.py
autobot-backend/api/merge_conflict_resolution_test.py
autobot-backend/transcriber/routes/recordings.py
autobot-slm-backend/services/role_registry.py
autobot-slm-backend/tests/services/test_role_registry_post_sync_17166.py
autobot_shared/env_registry_agent_runtime.py
autobot_shared/env_registry_ai.py
autobot_shared/env_registry_backend_services.py
autobot_shared/env_registry_llc.py
autobot_shared/env_registry_logging.py
autobot_shared/env_registry_slm.py
autobot_shared/env_registry_terminal.py
autobot_shared/env_registry_testing.py
autobot_shared/security/path_http.py
autobot_shared/security/path_http_test.py
repo_tests/durations_are_landed_and_fresh_17313_test.py
repo_tests/no_duplicate_route_registration_16908_test.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

coderabbitai · 2026-09-23T15:32:54Z

+
+      - name: Open or update the durations PR
+        env:
+          GH_TOKEN: ${{ github.token }}


🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

A PR opened with github.token gets no CI, so it cannot merge under required checks.

GitHub does not start new workflow runs from events created with GITHUB_TOKEN. The only exceptions are workflow_dispatch and repository_dispatch. The PR from gh pr create and each force-push to automation/test-durations produce no pull_request runs. If branch protection requires status contexts, the landing PR waits on "Expected" permanently. A human must then close and reopen it, or push to it. That contradicts the "without a human step" goal.

gh pr create also fails unless the setting "Allow GitHub Actions to create and approve pull requests" is enabled.

Fix: use a GitHub App token, for example from actions/create-github-app-token, or a fine-grained PAT for the push and gh pr create. Otherwise, document the manual close/reopen step.

coderabbitai · 2026-09-23T15:32:54Z

+          git checkout -B "$BRANCH"
+          git add .test_durations .test_durations_slm
+          git commit -m "chore(ci): refresh per-test split durations (#17313)"
+          git push --force-with-lease origin "$BRANCH"


🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

--force-with-lease rejects every push after the first.

actions/checkout at Line 173 fetches only the triggering ref at depth 1. No refs/remotes/origin/automation/test-durations exists locally. With no remote-tracking ref, bare --force-with-lease expects the remote branch to be absent. When the branch already exists from an earlier run, git rejects the push with "stale info". set -e then fails the job. The "update the existing PR" path never succeeds.

The bot owns this branch, so pass an explicit lease from ls-remote, or use --force.

🐛 Proposed fix

- git push --force-with-lease origin "$BRANCH" + remote_sha="$(git ls-remote --heads origin "$BRANCH" | cut -f1)" + git push --force-with-lease="refs/heads/$BRANCH:${remote_sha}" origin "HEAD:refs/heads/$BRANCH"

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

git push --force-with-lease origin "$BRANCH"

remote_sha="$(git ls-remote --heads origin "$BRANCH" | cut -f1)"

git push --force-with-lease="refs/heads/$BRANCH:${remote_sha}" origin "HEAD:refs/heads/$BRANCH"

coderabbitai · 2026-09-23T15:32:54Z

+          if gh pr view "$BRANCH" --json number >/dev/null 2>&1; then
+            echo "::notice::updated the existing durations PR"


🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

gh pr view "$BRANCH" finds merged PRs, so no new PR opens after the first merge.

gh pr view <branch> prefers an open PR for that head branch. If no open PR exists, it returns the most recent closed or merged PR. After the first landing PR merges, every later run pushes the branch and logs "updated the existing durations PR". No PR opens, and the durations stop landing again. This is the #17313 failure in a new form.

Query open PRs only.

🐛 Proposed fix

- if gh pr view "$BRANCH" --json number >/dev/null 2>&1; then + open_prs="$(gh pr list --head "$BRANCH" --base main --state open --json number --jq 'length')" + if [ "$open_prs" -gt 0 ]; then echo "::notice::updated the existing durations PR"

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

if gh pr view "$BRANCH" --json number >/dev/null 2>&1; then

echo "::notice::updated the existing durations PR"

open_prs="$(gh pr list --head "$BRANCH" --base main --state open --json number --jq 'length')"

if [ "$open_prs" -gt 0 ]; then

echo "::notice::updated the existing durations PR"

coderabbitai · 2026-09-23T15:32:54Z

+    allowed = {"autobot-backend/transcriber/routes/recordings.py"}
+    unexpected = [o for o in offenders if o.rsplit(":", 1)[0] not in allowed]


🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Limit the recordings exemption to its existing 404 handler.

If another path-validator handler in autobot-backend/transcriber/routes/recordings.py returns 403, this scan excludes it solely because of its filename. The scan also passes if the existing handler stops returning 404. Identify the permitted handler specifically, and assert that its missing-file and rejected-path responses remain identical.

coderabbitai · 2026-09-23T15:32:54Z

+def _last_committed(path: str) -> datetime.date:
+    out = subprocess.run(
+        ["git", "-C", str(repo_root()), "log", "-1", "--format=%ad", "--date=short", "--", path],
+        capture_output=True,
+        text=True,
+        env=scrubbed_git_env(),
+    )
+    stamp = out.stdout.strip()
+    # A check that cannot look must not report clean: no commit date means the
+    # file is untracked or the log failed, and neither is "the file is fresh".
+    assert stamp, f"could not read a commit date for {path} -- the freshness of this file is UNKNOWN, not fine"
+    return datetime.date.fromisoformat(stamp)


🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

#!/bin/bash # Check whether the workflows that collect repo_tests use a shallow checkout. rg -n -B2 -A6 'actions/checkout' .github/workflows | rg -n 'fetch-depth|checkout|^--' rg -n -C2 '\brepo_tests\b' .github/workflows

Repository: mrveiss/AutoBot-AI

Length of output: 35681

🏁 Script executed:

#!/bin/bash set -u printf '%s\n' '--- test file ---' sed -n '1,130p' repo_tests/durations_are_landed_and_fresh_17313_test.py printf '%s\n' '--- test-durations workflow 1 ---' sed -n '45,125p' .github/workflows/test-durations.yml printf '%s\n' '--- test-durations workflow 2 ---' sed -n '150,205p' .github/workflows/test-durations.yml printf '%s\n' '--- changed paths ---' git diff --stat 6680ed7923512a26e92eb03bd2c061bb1e01c463 83fac124a850663b6f90f47f63f844f72bad425f printf '%s\n' '--- relevant diff ---' git diff --unified=15 6680ed7923512a26e92eb03bd2c061bb1e01c463 83fac124a850663b6f90f47f63f844f72bad425f -- repo_tests/durations_are_landed_and_fresh_17313_test.py .github/workflows/test-durations.yml

Repository: mrveiss/AutoBot-AI

Length of output: 25996

Use full Git history for the freshness check.

store-durations runs repo_tests after an actions/checkout@v7 step with the default shallow history. For a tracked duration file, _last_committed() then reports the checkout's boundary commit date, so a stale file can appear fresh. The empty-stamp assertion does not detect this.

Set fetch-depth: 0 on every checkout that runs this test.

Suggested checkout fix

- uses: actions/checkout@v7 + with: + fetch-depth: 0

🧰 Tools

🪛 ast-grep (0.45.3)

[error] 57-62: Command coming from incoming request
Context: subprocess.run(
["git", "-C", str(repo_root()), "log", "-1", "--format=%ad", "--date=short", "--", path],
capture_output=True,
text=True,
env=scrubbed_git_env(),
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').

(subprocess-from-request)

🪛 Ruff (0.16.5)

[error] 58-58: subprocess call: check for execution of untrusted input

(S603)

[warning] 58-58: subprocess.run without explicit check argument

Add explicit check=False

(PLW1510)

[error] 59-59: Starting a process with a partial executable path

(S607)

Source: Path instructions

coderabbitai · 2026-09-23T15:32:54Z

+    tree = ast.parse(ct)
+    alias_to_module = {}
+    for node in ast.walk(tree):
+        if isinstance(node, ast.ImportFrom) and node.module and node.module.startswith("api"):


🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Include every registered core router in discovery.

autobot-backend/initialization/router_registry/core_routers.py registers routers imported from plugin_manager and services.knowledge_sync_service. The api filter excludes both before _routes runs. Their routes cannot contribute to total or trigger a duplicate finding. Resolve imports for registered router names regardless of module prefix, then report any source that cannot be resolved. (raw.githubusercontent.com)

coderabbitai · 2026-09-23T15:32:54Z

+            continue
+        for verb, sub in r:
+            full = re.sub(r"/+", "/", f"/api{prefix}{sub}")
+            seen[(verb, full)].add(f"{module}:{var}")


🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Keep each registration occurrence instead of deduplicating owners.

If a registry mounts the same router twice at the same prefix, both route occurrences have the same module:var value. The set keeps one value, so len(v) > 1 never detects that duplicate. Store occurrences in a list; the existing sorted diagnostic can then show both registrations. (fastapi.tiangolo.com)

Proposed change

- seen = collections.defaultdict(set) + seen = collections.defaultdict(list) ... - seen[(verb, full)].add(f"{module}:{var}") + seen[(verb, full)].append(f"{module}:{var}")

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

seen[(verb, full)].add(f"{module}:{var}")

seen[(verb, full)].append(f"{module}:{var}")

coderabbitai · 2026-09-23T15:32:54Z

+def test_the_expansion_attributes_routes_to_the_registered_router() -> None:
+    """api.voice registers two different router objects at the same prefix.
+
+    If decorators were attributed by file, both registrations would claim every
+    route in voice.py and the module would collide with itself -- noise that
+    buries the real cross-module collision.
+    """
+    main = _routes("api.voice", "router")
+    realtime = _routes("api.voice", "realtime_router")
+    assert main and realtime, "both routers in api.voice should expand to routes"
+    assert not (set(main) & set(realtime)), "the two routers in api.voice must not share a route"
+    assert ("POST", "/realtime/tools/call") in realtime
+    assert ("POST", "/realtime/tools/call") not in main


📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add a contrast fixture pair for the collision detector.

These assertions check router attribution in the live registry. They do not prove that a planted collision fails or that two distinct routes pass. Add both fixtures so the detector remains tested after the known collision is removed. As per path instructions: “Every detector needs a contrast pair: a fixture that SHOULD trip it and one that should not.”

🧰 Tools

🪛 Ruff (0.16.5)

[warning] 166-166: Assertion should be broken down into multiple parts

(PT018)

Source: Path instructions

…ved on (#16843) The doc advertised 32 endpoints under /api/control/. No router registers /control; api.advanced_control is registered at /advanced-control (feature_routers.py:380), so every documented path 404s as written. Verified per path rather than by search-and-replace confidence: all 18 distinct sub-paths in the doc were checked against the `@router.<verb>` decorators in api/advanced_control.py before the rewrite and all 18 resolve; re-checked after, and all 32 rewritten occurrences resolve to a declared route. This is #16843's third acceptance criterion. Its first -- populate `affected_tasks` from the task tracker -- is already satisfied on main: services/emergency_stop.py:49 reads `get_task_tracker().get_active_tasks().keys()`. That landed via the v0.9.1 vehicle consolidation (#17194, 245c1af), which is why the issue's own PR #16854 shows as closed-unmerged. The second criterion -- wire real task execution to check the paused-task set -- is NOT addressed here and the issue stays open for it. Refs #16843

) The heredoc rewrite two commits ago shipped a script that does not parse. `bash -n` on the extracted run block: warning: here-document at line 40 delimited by end-of-file (wanted `BODY') syntax error: unexpected end of file Two bugs. The BODY terminator sat at 12 spaces against the block scalar's base indent of 10, so after YAML dedents the block it still carried two leading spaces and `<<'BODY'` (no dash) never matched it. And the `if gh pr view` was closed twice -- if/fi counted 2 against 3. Not cosmetic. The step dies at runtime on the schedule and dispatch paths, which are the only paths that matter here: the durations PR would never have opened, and the symptom would have been exactly the silence #17313 exists to fix -- a workflow that appears to run and lands nothing. Everything already in this file's guard inspects the workflow AS DATA: the YAML parses, the job is wired to the cron, the body renders with its four headings. None of that executes a shell, so all of it passed on a script bash refuses. The guard now runs `bash -n` on the landing script, and the mutation confirms it: restoring the 12-space terminator fails test_the_landing_script_is_valid_shell and nothing else. Found by peer session autobot-ai-63 reviewing the branch, and verified here independently before applying -- the diagnosis named both bugs, the line numbers and the mechanism, and all three checked out. Refs #17313

…#17166, #17313) Shards 7/12 and 12/12. Five failures, all caused by changes on this branch, and two of them are the guards working exactly as designed. 1. THE EXTRACTION BROKE A GUARD THAT READS SOURCE, NOT BEHAVIOUR (#17166). test_code_sync_preserves_install_14275 AST-parses role_registry.py and reads `post_sync_cmd` string literals. Moving the build-filtered-requirements.sh invocation into `_filtered_pip_install` left a Call node where a Constant was, so the guard found the rewrite in zero entries and reported its own rule vacuous -- "only [] route through the rewrite". The commands are byte-for-byte unchanged; the guard's method was what no longer reached them. It now executes the module's helpers and evaluates the call, so the check runs against the string the role will actually execute. That is strictly stronger than matching a literal: it would catch a helper that builds the WRONG command, which a literal match never could. Rule 7 says grep the behaviour, not the symbol, on an extraction PR -- this is the guard side of that same rule. 2. MY NAMING POLLUTED A DOCUMENTED AUDIT (#13579). schema_validator_names_13518 requires `grep "def validate_path"` to match exactly one thing -- the canonical containment helper -- because #13518 exists over false hits on that grep making a reviewer read schema validators as containment. `validate_path_or_http` matched it. The guard was right and the name was wrong, so the helpers are now `require_contained_path` / `require_contained_relative_path`, which say what they enforce and stay out of those results. 3. MY NEW GUARD WAS OUTSIDE THE FILTER THAT RUNS IT (#17313). durations_are_landed_and_fresh_17313_test.py reads .github/workflows/test-durations.yml by concrete literal, and python-paths.yml did not cover it -- so a change confined to that workflow computes `python != 'true'`, the required-context shim publishes green, and the guard never runs. Covered rather than recorded as uncovered: MAX_UNCOVERED_READS stays 42 because the read is now genuinely covered, not because it was excused. 4 & 5. TWO REACH FLOORS. hooks-path-override 6531 -> 6536 (measured 6936) and import-hermeticity 601 -> 602 (measured 662). This branch adds four counting files and one module respectively; main had spent the rest of both allowances before the branch existed. That is the fifteenth re-pin of the first one and the sixth in two days -- see #17142, which is about the allowance and not the floor. 206 passed, 1 skipped (reach_declarations + import_hermeticity) 15 passed (code_sync_preserves_install) 16 passed (schema_validator_names + path_http) 13 passed (python_filter_covers_its_guards) Refs #13579, #17166, #17313, #17142

…ng it (#16908, #17325) My guard reads `@router.<verb>` decorators out of the AST, so it cannot see a route mounted with `router.include_router(other)`. Sixteen non-test modules under autobot-backend/api/ compose that way -- agent_terminal.py:218, auth.py:302 and fourteen others. Its "1518 pairs, 1 duplicate" is a FLOOR, not a total, and reporting it as a total would be the exact defect this repository governs: a count the guard cannot establish. So the blind spot is now measured and asserted. If a seventeenth module starts composing that way the guard fails and says the gap grew, rather than the gap growing unobserved. I FOUND THIS BY AUDITING LEFTOVER BRANCHES, and what it turned up is worse than the blind spot. Commit f4e2d6b on the abandoned branch `issue-16908-duplicate-routes` had already rebuilt this guard on REAL router objects -- load_core_routers()/load_optional_routers(), effective_routes() resolving nested include_router() with an inner router's own prefix, and a population-collapse assertion so a broken import cannot pass as "no duplicates". 228 lines, complete, removed from that branch by a later commit ("split guard out of #16918 per review") and never landed. So I rewrote the project's own prior work, worse, because I did not look for it first. That is rule 1, and the remedy is to reuse it -- filed as #17325 with f4e2d6b named as the starting point rather than a third implementation. Not adopted here because I could not run it once: importing real routers pulls llc.scheduler.base, which refuses on Python 3.11-minus and this machine is 3.10. CI is 3.14 and would execute it. Shipping 228 unverified lines is not an improvement on shipping a lesser guard I did run, so the lesser one ships with its limit written down. The pin is 16 and was 77 in my first draft -- that grep counted `*_test.py`. The test computes the number and asserts it rather than trusting the shell, which is how the miscount surfaced within a minute instead of becoming a comment nobody rechecks. Mutation-verified: a planted module containing include_router fails the pin. Refs #16908, #17325

…#17142, #16324) Rebased onto 3131039. Three merges landed within the hour (#17314, #17321, #17328) and moved every one of these. REACH FLOORS. hooks-path-override 6536 -> 6538 (measured 6938) and audio-extension-allowlist 5450 -> 5752 (measured 6152). The first is the sixteenth re-pin and 6536 was measured correctly barely an hour ago -- it did not survive three merges. The second had never tripped before today and trips now for the same reason. That is #17142's argument as a data point rather than an argument: a 400-file allowance on a ~6900-file tree is spent faster than a branch can be reviewed, so a floor pinned correctly at measurement time is already stale at merge time. DUPLICATION, MAIN SCOPE. 11723 -> 11707. Sixteen lines of slack, and the gate exits 0 while it sits there, so a fall in duplication is invisible and a later PR can spend it. That is #16324. DUPLICATION, SLM SCOPE: DELIBERATELY UNCHANGED, and this is the part worth reading. A figure of 2918 was reported for it from the guard's run on #17321's merged head. That run predated #17314, which had already lowered the pin from 2945 to 2831. Applying the reported number would have moved the pin from 2831 to 2918 -- RAISING a ratchet, which is the one direction it must never go, and it would have re-licensed 87 lines of duplication while looking like housekeeping. Re-measured here instead, on the actual merged tree: main scope: 11707 duplicated lines, 672 clones, 4288 files -> pin 11707 SLM scope: 2831 duplicated lines, 123 clones, 798 files -> pin 2831, exact Both figures come from running the detector with the workflow's own flags, not from reading a report. The report was accurate about its own tree and wrong about this one, which is the entire hazard of carrying a measurement across a merge. Refs #17317, #17142, #16324

…m for code-consumed verdicts (#17307, #17308) #17307 — a judge parse failure approved the workflow step it was asked to gate. `judges/__init__.py` indexed `overall_score`/`recommendation`/`confidence` out of a single-shot reply, any key or enum drift raised, `make_judgment` turned that into an error judgment, and `step_evaluator._check_judge_errors` turned THAT into `should_proceed: True`. The security judge failing to parse therefore approved the command it was assessing. The retry-and-validate loop `structured_ops.extract()` already had now lives in `llm_shared/validated_llm.py` behind a *completer*, so one loop serves every transport: `llm_service` for the judges, the claim verifier and the autoresearch scorer, a direct local Ollama call for `rlm/evaluator.py`, and whatever backend the decision seam is given. It also sends the schema to the provider as `json_schema` (#17305), so native schema mode constrains the reply before the retry is needed. The gate is now a named policy: `AUTOBOT_JUDGE_FAIL_CLOSED` (default open, as #1464 chose), and the response carries `judge_available: False` plus a `degradation` code, counted through the existing approval and error counters — `approved_judge_unavailable` is a different metric label from `approved`. `_build_evaluation_error_response` and `workflow_step_judge.quick_approval_check` had the same hard-coded approval and now follow the same policy. Two parse-miss defaults of the #17306 shape went with it: `rlm/evaluator._extract_float` returned `0.5` when the SCORE line was missing (compared against `quality_threshold` immediately after), and `scorers._parse_rating` fell back to a regex that would find "7" inside a refusal. Both are gone; an unreadable reply is an error result or the existing INDETERMINATE verdict, never a number nobody produced. #17308 — `llm_shared/decisions.py` is the seam: `decide(state, questions)` with choice, score and boolean primitives, answered in one round trip against one state, validated against a generated schema. `claim_verifier.classify_agreement` and the autoresearch scorer are migrated with their bespoke parsers deleted; the judges go through the same loop with `JUDGMENT_SCHEMA` because their payload carries per-dimension scores the three primitives cannot express. The backend is pluggable and defaults to the local small-model path, so the seam needs no new outbound dependency and none is added. Probabilities are reported as `Calibration.SELF_REPORTED`, not calibrated: nobody here has measured them. `scripts/benchmark_decision_backends.py` plus a labelled sample is the gate that would change that — it reports accuracy, p50/p95 latency and the separation between mean probability on right and wrong answers, and promotes nothing. The exclusion list is a test, not a comment: `decisions_exclusions_test.py` asserts that tier routing, the fast-path classifiers, reranking, injection detection and the pre-action verifier gate do not import the seam. `services/claim_verifier.py` shrank 835 -> 830, so both ratchet copies are lowered. `AUTOBOT_JUDGE_FAIL_CLOSED` is registered in `env_registry_agent_runtime.py` with `ENV_VARS.md` regenerated, as the env-var hook requires — both are hub files #17314 also touches. Closes #17307 Closes #17308

…t carries (#17317) (#17318) * fix(provision): stop a long ansible line from replacing the failure it carries (#17317) Reported from a live provisioning run: backend : Install filtered backend requirements fatal: [...]: FAILED! => {"changed": false, "cmd": [".../pip3", "install", ...], "msg": "\n:stderr: ER Error: Separator is found, but chunk is longer than limit Two failures stacked and the second hid the first. `pip install` failed, and ansible reported it the way it reports everything: ONE `fatal:` line holding the whole task result as JSON, with pip's entire stderr inside it. That line was past asyncio's default 64 KiB StreamReader limit, so `readline()` raised `ValueError: Separator is found, but chunk is longer than limit`. Nothing caught it, the generator died mid-run, and that ValueError stood in for the pip error it had just swallowed. The operator was shown the reader's failure instead of the provisioning failure, and the real cause was never written down anywhere. THE PART THAT DECIDES THE FIX: `readline()` DESTROYS the line before raising. CPython's implementation clears `self._buffer` on LimitOverrunError and re-raises as a bare ValueError, so catching it recovers nothing however careful the handler is -- verified directly, `read()` afterwards returns 0 bytes. My first attempt did exactly that and its own test caught it. So the reader now uses `readuntil(b"\n")`, which raises LimitOverrunError with the buffer INTACT and `consumed` pointing at how much is readable. The head is kept, the remainder of that line is discarded so the next read starts on a clean boundary, and the yielded line carries a marker so a cut line is never mistaken for a whole one. An over-long line is truncated and reported -- never dropped, never fatal. A line too big to read is still evidence. Also: the spawn now passes `limit=PIPE_LINE_LIMIT` (10 MiB, env-backed and clamped). Raising the constant without passing it to create_subprocess_exec would have left the 64 KiB default in place, which is the version of this fix that looks right and changes nothing. Mutation-verified, each against the real stdlib rather than a mock -- the bug lives in what asyncio raises when its buffer is exceeded, so the buffer has to be exceeded: back to readline() -> 4 FAILED drop limit= from the spawn -> FAILED ..._spawned_with_that_limit skip the discard -> FAILED ..._no_fragment_..._leaks_as_its_own_line That last one is worth its own note. Removing the discard does not crash and does not spin -- it yields the unread tail as an extra blank line between the truncated line and the next real one, quietly feeding the progress parser a line ansible never emitted. Only a shape assertion catches it, and my first pass at these tests did not. WHAT THIS DOES NOT DO: it does not fix the pip failure. That error is still unknown, because it was destroyed before anyone could read it. This change is what makes the next run say what actually went wrong. The reader moved to its own module, services/playbook_output_stream.py. playbook_executor.py is at its grandfathered ceiling and may not grow, and the rule is split rather than raise -- it came out at 1402 against a 1406 ceiling, so the ceiling drops to 1402 in both places that record it. It is also the better seam: this is stream decoding and the executor is orchestration. Closes #17317 Refs #17038 * chore(ratchets): re-pin four counters against the merged tree (#17317, #17142, #16324) Rebased onto 3131039. Three merges landed within the hour (#17314, #17321, #17328) and moved every one of these. REACH FLOORS. hooks-path-override 6536 -> 6538 (measured 6938) and audio-extension-allowlist 5450 -> 5752 (measured 6152). The first is the sixteenth re-pin and 6536 was measured correctly barely an hour ago -- it did not survive three merges. The second had never tripped before today and trips now for the same reason. That is #17142's argument as a data point rather than an argument: a 400-file allowance on a ~6900-file tree is spent faster than a branch can be reviewed, so a floor pinned correctly at measurement time is already stale at merge time. DUPLICATION, MAIN SCOPE. 11723 -> 11707. Sixteen lines of slack, and the gate exits 0 while it sits there, so a fall in duplication is invisible and a later PR can spend it. That is #16324. DUPLICATION, SLM SCOPE: DELIBERATELY UNCHANGED, and this is the part worth reading. A figure of 2918 was reported for it from the guard's run on #17321's merged head. That run predated #17314, which had already lowered the pin from 2945 to 2831. Applying the reported number would have moved the pin from 2831 to 2918 -- RAISING a ratchet, which is the one direction it must never go, and it would have re-licensed 87 lines of duplication while looking like housekeeping. Re-measured here instead, on the actual merged tree: main scope: 11707 duplicated lines, 672 clones, 4288 files -> pin 11707 SLM scope: 2831 duplicated lines, 123 clones, 798 files -> pin 2831, exact Both figures come from running the detector with the workflow's own flags, not from reading a report. The report was accurate about its own tree and wrong about this one, which is the entire hazard of carrying a measurement across a merge. Refs #17317, #17142, #16324 * fix(provision): route the second playbook reader through the same guard (#17317) Review of this PR found the extraction had left a live twin. api/infrastructure.py `_stream_process_output` ran the identical unguarded `readline()` against a pipe spawned without a `limit=`, so the default 64 KiB applied. It is reached from POST /api/execute and runs the same pip-heavy provisioning playbooks -- setup-ai-stack.yml, setup-npu-worker.yml, provision-fleet-roles.yml -- that produce the oversized `fatal:` JSON line this PR exists to survive. Its failure mode was the worse of the two. `readline` clears the buffer and raises a bare ValueError; `_run_playbook`'s broad `except Exception` catches it and reports "Internal server error". So the endpoint answered with strictly less than the original bug report, which at least surfaced the ValueError text. Both halves are needed and neither is sufficient: the shared iterator cannot salvage a line the spawn already capped at 64 KiB, and a raised limit changes nothing while the reader still uses `readline`. Both are asserted, and the reader assertion is written against the BEHAVIOUR (`process.stdout.readline()` absent, `iter_pipe_lines` present) rather than the helper's name, so renaming `_stream_process_output` cannot silently retire the check. Also corrects two stale references to the pre-extraction name `_iter_pipe_lines` left in a comment and a test docstring. * fix(ratchets): pin the hooks reach floor mid-window, not at its bottom (#17317, #17142) Sixteen re-pins of this floor, six in the last two days, and #17142 concluded the growth allowance is too small. The measurement says the allowance is not the cause and raising it would not have helped. The floor has two bounds. verify_floor needs population - floor <= skips + growth (401), so floor >= 6535. completed() needs floor <= what the guard finishes, and skips=1 is this file alone, so that ceiling is population - 1 = 6935. The legal window is 6535..6935, four hundred wide. Every previous re-pin used floor = population - growth, which lands on the very bottom of that window. Slack is then always exactly growth against an allowance of growth + 1, leaving ONE file of headroom no matter what growth is set to -- doubling growth to 800 would re-pin the floor 400 lower and leave the same single file. That is the treadmill, and it is a property of the formula rather than of the number. 6736 is population - 200: 201 files of headroom before the allowance is breached, 199 files of shrink before completed() is. It is also a stricter floor than 6538 rather than a looser one, because the floor asserts how much of the tree the guard actually reached; only the gap check cares about the distance. Main measures 6936, confirmed by two independent branches rather than asserted: #17323 adds 3 counted files and CI read 6939, #17330 adds 2 and read 6938. Counted additions in flight total +22, and #17327 alone (+14) would have breached the previous 6538 four times over.

…m for code-consumed verdicts (#17307, #17308) #17307 — a judge parse failure approved the workflow step it was asked to gate. `judges/__init__.py` indexed `overall_score`/`recommendation`/`confidence` out of a single-shot reply, any key or enum drift raised, `make_judgment` turned that into an error judgment, and `step_evaluator._check_judge_errors` turned THAT into `should_proceed: True`. The security judge failing to parse therefore approved the command it was assessing. The retry-and-validate loop `structured_ops.extract()` already had now lives in `llm_shared/validated_llm.py` behind a *completer*, so one loop serves every transport: `llm_service` for the judges, the claim verifier and the autoresearch scorer, a direct local Ollama call for `rlm/evaluator.py`, and whatever backend the decision seam is given. It also sends the schema to the provider as `json_schema` (#17305), so native schema mode constrains the reply before the retry is needed. The gate is now a named policy: `AUTOBOT_JUDGE_FAIL_CLOSED` (default open, as #1464 chose), and the response carries `judge_available: False` plus a `degradation` code, counted through the existing approval and error counters — `approved_judge_unavailable` is a different metric label from `approved`. `_build_evaluation_error_response` and `workflow_step_judge.quick_approval_check` had the same hard-coded approval and now follow the same policy. Two parse-miss defaults of the #17306 shape went with it: `rlm/evaluator._extract_float` returned `0.5` when the SCORE line was missing (compared against `quality_threshold` immediately after), and `scorers._parse_rating` fell back to a regex that would find "7" inside a refusal. Both are gone; an unreadable reply is an error result or the existing INDETERMINATE verdict, never a number nobody produced. #17308 — `llm_shared/decisions.py` is the seam: `decide(state, questions)` with choice, score and boolean primitives, answered in one round trip against one state, validated against a generated schema. `claim_verifier.classify_agreement` and the autoresearch scorer are migrated with their bespoke parsers deleted; the judges go through the same loop with `JUDGMENT_SCHEMA` because their payload carries per-dimension scores the three primitives cannot express. The backend is pluggable and defaults to the local small-model path, so the seam needs no new outbound dependency and none is added. Probabilities are reported as `Calibration.SELF_REPORTED`, not calibrated: nobody here has measured them. `scripts/benchmark_decision_backends.py` plus a labelled sample is the gate that would change that — it reports accuracy, p50/p95 latency and the separation between mean probability on right and wrong answers, and promotes nothing. The exclusion list is a test, not a comment: `decisions_exclusions_test.py` asserts that tier routing, the fast-path classifiers, reranking, injection detection and the pre-action verifier gate do not import the seam. `services/claim_verifier.py` shrank 835 -> 830, so both ratchet copies are lowered. `AUTOBOT_JUDGE_FAIL_CLOSED` is registered in `env_registry_agent_runtime.py` with `ENV_VARS.md` regenerated, as the env-var hook requires — both are hub files #17314 also touches. Closes #17307 Closes #17308

…m for code-consumed verdicts (#17307, #17308) (#17327) * chore: claim worktree for #17307, #17308 * fix(judges,agent-seam): one validated loop and one typed-decision seam for code-consumed verdicts (#17307, #17308) #17307 — a judge parse failure approved the workflow step it was asked to gate. `judges/__init__.py` indexed `overall_score`/`recommendation`/`confidence` out of a single-shot reply, any key or enum drift raised, `make_judgment` turned that into an error judgment, and `step_evaluator._check_judge_errors` turned THAT into `should_proceed: True`. The security judge failing to parse therefore approved the command it was assessing. The retry-and-validate loop `structured_ops.extract()` already had now lives in `llm_shared/validated_llm.py` behind a *completer*, so one loop serves every transport: `llm_service` for the judges, the claim verifier and the autoresearch scorer, a direct local Ollama call for `rlm/evaluator.py`, and whatever backend the decision seam is given. It also sends the schema to the provider as `json_schema` (#17305), so native schema mode constrains the reply before the retry is needed. The gate is now a named policy: `AUTOBOT_JUDGE_FAIL_CLOSED` (default open, as #1464 chose), and the response carries `judge_available: False` plus a `degradation` code, counted through the existing approval and error counters — `approved_judge_unavailable` is a different metric label from `approved`. `_build_evaluation_error_response` and `workflow_step_judge.quick_approval_check` had the same hard-coded approval and now follow the same policy. Two parse-miss defaults of the #17306 shape went with it: `rlm/evaluator._extract_float` returned `0.5` when the SCORE line was missing (compared against `quality_threshold` immediately after), and `scorers._parse_rating` fell back to a regex that would find "7" inside a refusal. Both are gone; an unreadable reply is an error result or the existing INDETERMINATE verdict, never a number nobody produced. #17308 — `llm_shared/decisions.py` is the seam: `decide(state, questions)` with choice, score and boolean primitives, answered in one round trip against one state, validated against a generated schema. `claim_verifier.classify_agreement` and the autoresearch scorer are migrated with their bespoke parsers deleted; the judges go through the same loop with `JUDGMENT_SCHEMA` because their payload carries per-dimension scores the three primitives cannot express. The backend is pluggable and defaults to the local small-model path, so the seam needs no new outbound dependency and none is added. Probabilities are reported as `Calibration.SELF_REPORTED`, not calibrated: nobody here has measured them. `scripts/benchmark_decision_backends.py` plus a labelled sample is the gate that would change that — it reports accuracy, p50/p95 latency and the separation between mean probability on right and wrong answers, and promotes nothing. The exclusion list is a test, not a comment: `decisions_exclusions_test.py` asserts that tier routing, the fast-path classifiers, reranking, injection detection and the pre-action verifier gate do not import the seam. `services/claim_verifier.py` shrank 835 -> 830, so both ratchet copies are lowered. `AUTOBOT_JUDGE_FAIL_CLOSED` is registered in `env_registry_agent_runtime.py` with `ENV_VARS.md` regenerated, as the env-var hook requires — both are hub files #17314 also touches. Closes #17307 Closes #17308 * fix(eval,autoresearch): update the doubles the validated verdict contract replaced (#17307, #17308) The five failures on #17327 that were not the shared CI floor, all one cause: test doubles still speaking the format the schema replaced. - eval/tests (harness, candidate_wiring, hardening): the RLM evaluator stub returned the three-line SCORE:/CRITIQUE:/HINT: reply, so every scored trajectory now failed validation and classified as 'unmeasured' instead of 'unchanged'. The stub speaks JSON. - tests/test_autoresearch_m3.py: the judge double still returned {"rating": 7}, and its two sibling doubles had a truthy MagicMock .error, which the completer correctly reads as a failed call. - pipeline-scripts/hardcoded_values_baseline.txt: five entries went stale because the "system"/"user" role literals left structured_ops.py, scorers.py and claim_verifier.py when those call sites moved onto the shared completer's CategoryDefaults. Pruned with --prune-baseline (removal-only) and re-audited clean. * test(guards): re-pin four reach floors this stack's file count moves past (#17307, #17308) Arithmetic, not a regression, and measured rather than assumed. Main at 3131039 leaves all four floors sitting at EXACTLY their declared allowance, so any branch that adds files trips them -- the #17142 shape the hooks-path-override comment block already records thirteen times. Attribution, by counting both trees with the same globs rather than adding to what CI last reported: `git ls-tree -r --name-only` gives 6905 sh/py/yml files on origin/main and 6919 on this HEAD, and 6150 vs 6164 python files. The +14 is this stack's own new modules and tests. hooks-path-override 6536 -> 6550 (population 6950, growth 400) audio-extension-allowlist 5450 -> 5764 (population 6164, growth 400) import-hermeticity 602 -> 604 (population 664, growth 60) prompt-injection-detector-strict-mode 2956 -> 2960 (population 3260, growth 300) Each pinned at `population - growth`, which is the stricter direction for a reach floor: it asserts the sweep must reach MORE, unlike a size ceiling, which may only come down. * test(guards): give the decision-seam exclusion guard a positive control (#17308) Found by applying tonight's rule to my own branch: would this green test still pass if the thing under test were deleted? It would. Every case in `test_excluded_module_does_not_use_the_decision_seam` asserts an ABSENCE. `path.is_file()` catches a renamed target and `test_the_repo_root_resolves_to_this_checkout` catches a wrong root, but nothing proved `_imports()` can DETECT a seam import. A changed AST node type, a silent exception or a walk that stops early would have made all nine cases pass while reporting "nothing found" for "did not look" -- the exact conflation MEASUREMENT_DISCIPLINE.md opens with. The new case asserts the detector finds the seam import in `services/claim_verifier.py`, a production caller migrated onto the seam in #17308 rather than a fixture that could drift out of the shape it stands for. Mutation-verified rather than asserted: with `_imports` blinded to return an empty set, the suite goes 1 failed / 11 passed, and the one failure is this new control. Before it, the same blinding produced 11 green. * fix(research): update the agreement double the typed-decision contract replaced (#17307, #17308) The red CI found and my own consumer sweep missed. `services/research/orchestrator_test.py` stubs the LLM with an `AGREEMENT:` line, so with `classify_agreement` on the decision seam every source came back UNRELATED, no fact was corroborated, and two promotion tests failed on `update_fact` never being awaited. Why the sweep missed it, because the selector is the lesson: I grepped for the migrated SYMBOLS (`classify_agreement`, `LLMJudgeScorer`, `make_judgment`, ...) across test files. This file never names any of them -- it drives `ResearchOrchestrator`, which calls `ClaimVerifier` two layers down. A grep for the symbol can only find direct namers, which is the "query narrower than its reading" shape MEASUREMENT_DISCIPLINE.md opens with. The instrument that would have found it is a grep for the REPLY FORMAT rather than the caller: `AGREEMENT:`, `SCORE:`, `{"rating": N}`. Re-run that way, the remaining hits are all unrelated (marketplace ratings, skill-health fixtures, a judge prompt template), so this was the last one. Two failures, both real assertions rather than flakes -- exactly what the shape said: a named failing step at 9.3 minutes, not an empty step list at the 60 minute ceiling.

mrveiss added 5 commits September 23, 2026 16:54

mrveiss mentioned this pull request Sep 23, 2026

bug(api): four endpoints are registered twice — the second registration is dead code #16908

Closed

3 of 4 tasks

coderabbitai Bot reviewed Sep 23, 2026

View reviewed changes

mrveiss added 3 commits September 23, 2026 18:48

mrveiss mentioned this pull request Sep 23, 2026

tech-debt(ci): the duplicate-route guard is AST-based and blind to include_router — a better one exists, abandoned #17325

Open

5 tasks

mrveiss mentioned this pull request Sep 23, 2026

fix(judges,agent-seam): one validated loop and one typed-decision seam for code-consumed verdicts (#17307, #17308) #17327

Merged

9 of 10 tasks

mrveiss merged commit 3131039 into main Sep 23, 2026
82 of 85 checks passed

mrveiss deleted the issue-16415-sibling-boilerplate branch September 23, 2026 18:30

	git push --force-with-lease origin "$BRANCH"
	remote_sha="$(git ls-remote --heads origin "$BRANCH" \| cut -f1)"
	git push --force-with-lease="refs/heads/$BRANCH:${remote_sha}" origin "HEAD:refs/heads/$BRANCH"

		if gh pr view "$BRANCH" --json number >/dev/null 2>&1; then
		echo "::notice::updated the existing durations PR"

		allowed = {"autobot-backend/transcriber/routes/recordings.py"}
		unexpected = [o for o in offenders if o.rsplit(":", 1)[0] not in allowed]

	seen[(verb, full)].add(f"{module}:{var}")
	seen[(verb, full)].append(f"{module}:{var}")

Conversation

mrveiss commented Sep 23, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thinking Path

What Changed

#16415 — the registration contract is stated once

#17313 — the durations now actually land

#17166 — one filtered-install command, not five

#13579 — one translation from a rejected path to an HTTP refusal

#16908 — nothing stopped two modules owning the same route

Verification

What this does NOT do

Model Used

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

❌ Failed checks (2 warnings)

Uh oh!

github-actions Bot commented Sep 23, 2026

✅ SSOT Configuration Compliance: Passing

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mrveiss commented Sep 23, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Sep 23, 2026 •

edited

Loading