-
Notifications
You must be signed in to change notification settings - Fork 0
docs(doctoring): verify loop-brief items 4, 15-18, 38, 39 already resolved #1758
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
seonghobae
merged 7 commits into
main
from
claude/workflow-orchestrator-improvements-b16824
Sep 3, 2026
+216
−2
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
b93ff94
docs(doctoring): verify loop-brief items 15-18 already resolved
seonghobae 139216c
fix(doctoring): address Devin review on items 15-18 verification note
seonghobae cb80a65
docs(doctoring): verify loop-brief items 4/39 already resolved
seonghobae 275e77c
fix(doctoring): stop overstating item-15's rate-limit conclusion
seonghobae af7759c
fix(doctoring): qualify cross-repo contextual-orchestrator#1008 refer…
seonghobae 2da8e6d
docs(doctoring): verify loop-brief item 38 already resolved
seonghobae e68bb67
fix(codeql): stop dropping CodeQL coverage for non-standard default b…
seonghobae File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
205 changes: 205 additions & 0 deletions
205
docs/doctoring/loop-brief-items-15-18-verification-20260903.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,205 @@ | ||
| # Loop-brief items 4, 15-18, 38, 39: verified already resolved, no further change needed | ||
|
|
||
| ## Context | ||
|
|
||
| The 2026-09-03 standing-loop brief asked to confirm whether several specific | ||
| workflow-consolidation and telemetry items were complete, since the queue felt | ||
| like it was growing rather than shrinking. This records what was checked and | ||
| why each item needed no further code change as of this branch's base commit | ||
| (`4f95abc`). | ||
|
|
||
| ## Items 4 / 39 — opaque 900-second Noema "Repair" timeout, no telemetry on why | ||
|
|
||
| Reproduced from the linked evidence: | ||
| `ContextualWisdomLab/html4tree` run `33560972491`, job `100033086428` | ||
| ("Required Noema Review ...#595"), step 13 "Prepare Noema model verdict" | ||
| failed with `NoemaRepairDeadlineExceeded: Noema repair exceeded 900-second | ||
| absolute wall-clock deadline` on 2026-09-02T02:28 UTC — no further specifics, | ||
| matching the complaint exactly. The item-39 example | ||
| (`contextual-orchestrator` run `33580381913`, ContextualWisdomLab/contextual-orchestrator#1008) is the same class of | ||
| failure, same day. | ||
|
|
||
| Already fixed on this branch's base, same day: PR (`a28fc2f`, | ||
| "fix(noema): remove caller repair deadline and duplicate model call") found | ||
| the 900-second bound had "no owner-specified or measured basis" and, deeper, | ||
| that Noema was duplicating a repair/failover responsibility | ||
| `contextual-orchestrator` already owns — turning one gateway failure into two | ||
| expensive calls. The fix: Noema now sends exactly one structured-output | ||
| request to the gateway, with no caller-side deadline, retry, or temperature; | ||
| every gateway call now emits a passive Actions annotation carrying attempt | ||
| count, elapsed duration, active phase, and a sanitized serving-model | ||
| identifier (see `docs/doctoring/noema-repair-attempt-telemetry.md`, PR | ||
| `86ef3e7` for the doc's own later clarification pass). A permanent contract | ||
| test (`tests/test_noema_repair_has_no_fixed_wall_clock_deadline.py`) forbids | ||
| `NOEMA_REPAIR_DEADLINE_SECONDS`, `NoemaRepairDeadlineExceeded`, | ||
| `signal.setitimer`, and a caller-authored retry/temperature from ever | ||
| reappearing; ran it plus `tests/test_noema_repair_attempt_telemetry.py` | ||
| locally (25 passed) to confirm it holds on this branch. | ||
|
|
||
| The item-39 PR (ContextualWisdomLab/contextual-orchestrator#1008, head `f35ee58d`) is still | ||
| `mergeable_state: blocked`, but its Noema check now shows a fresh attempt | ||
| queued at `2026-09-02T19:32:21Z` — after the fix merged — sitting `queued` | ||
| with no conclusion yet. That is the already-documented org-wide Actions | ||
| job-queue ceiling (#1754), not a recurrence of the repair-deadline bug; no | ||
| separate action taken here. | ||
|
|
||
| ## Item 38 — auto-PR CodeQL into every new repository | ||
|
|
||
| Checked whether new repositories actually get CodeQL coverage, and how. Two | ||
| mechanisms exist, deliberately not overlapping: | ||
|
|
||
| - GitHub's native org-level "code scanning default setup" (org code-security | ||
| configuration id `17`, "GitHub recommended") is attached to exactly 3 | ||
| repositories: `noema`, `feelanet-adfs`, `pg-llm-batch` | ||
| (`gh api orgs/ContextualWisdomLab/code-security/configurations/17/repositories`). | ||
| `noema` needs this because it is one of the ruleset's own exclusions below. | ||
| - The org required-workflow ruleset (`18156473`) requires `codeql-pr.yml` | ||
| (among others) on `repository_name: {include: ["~ALL"], exclude: ["noema", | ||
| ".github", "IRT-bibliography-set"]}` — `~ALL` is a *dynamic* match, so a | ||
| brand-new repository is covered from its very first pull request with zero | ||
| manual or automated action, the moment that PR exists. `.github` runs | ||
| `codeql-pr.yml` directly on its own `pull_request` trigger instead of via | ||
| the ruleset (excluding a ruleset's own source repo from being its own | ||
| target avoids a self-referential double-trigger). `IRT-bibliography-set` | ||
| has neither mechanism, consistent with its name suggesting a non-code data | ||
| repository CodeQL would not apply to anyway. | ||
|
|
||
| The `~ALL` dynamic-target mechanism is a better answer than a bot-authored | ||
| PR *when it actually fires* — but it didn't always. Devin's review on this | ||
| PR correctly caught that `codeql-pr.yml`'s own `on: pull_request: branches: | ||
| [main, master, develop]` filter is a second, narrower gate underneath the | ||
| ruleset's dynamic target, and it silently produced **zero** CodeQL checks for | ||
| a repository whose default branch has a different name. Verified live before | ||
| the review comment arrived at concluding text: `j-planner` (default branch | ||
| `gh-pages`, real open PR #2 as of this writing) received every other | ||
| required check — `opencode-review`, `noema-review`, `strix`, the | ||
| `security-scan.yml`-bundled `osv-scan`/`trivy-fs`/`scorecard`/`Semgrep | ||
| OSS`/`dependency-review` (that workflow deliberately has no branch | ||
| restriction, "Do not restrict the base ref" per its own comment) — but not | ||
| one `Detect CodeQL languages` or `Analyze (...)` check of any kind. Three | ||
| additional org repositories (`argos`, `OmniRoute`, `graphify` — all forks, | ||
| default branches `developmental`, `release/v3.8.50`, `v8` respectively) were | ||
| equally exposed. | ||
|
|
||
| **Fixed**, not just documented: removed the `branches: [main, master, | ||
| develop]` restriction from `codeql-pr.yml`'s `pull_request` trigger, matching | ||
| `security-scan.yml`'s own established "do not restrict the base ref" | ||
| precedent — the ruleset's `ref_name: ["~DEFAULT_BRANCH"]` condition is | ||
| already the authoritative gate for which branch qualifies, so the workflow's | ||
| own hardcoded list was pure redundant risk, not a second layer of intended | ||
| protection. Updated the one contract-test assertion that pinned the old | ||
| line (`tests/test_codeql_pr_workflow_contract.py:19`); the workflow's other | ||
| 17 assertions, the CodeQL-action-version-pin test, and the SARIF-gate | ||
| behavioral test all still pass, `actionlint` reports no errors, and the file | ||
| still parses as valid YAML. | ||
|
|
||
| **Not fixed here** (Devin's second, independent catch, correct but out of | ||
| this PR's scope): the language-detection matrix in the same workflow only | ||
| recognizes GitHub Actions, JavaScript/TypeScript, Python, and Java/Kotlin — | ||
| CodeQL also supports C/C++, C#, Go, Ruby, and Swift, none of which this | ||
| matrix detects; a repository containing only one of those falls back to | ||
| scanning `actions` alone rather than its real source. That is a larger, | ||
| separately-scoped change (new per-language file-detection heuristics plus | ||
| matching contract-test coverage) rather than a one-line fix, and is tracked | ||
| as a follow-up rather than rushed into this PR. | ||
|
|
||
| ## Item 15 — remove `org-queue-sweep` if plain GitHub Actions syntax can do it | ||
|
|
||
| `org-queue-sweep` (`.github/workflows/pr-review-merge-scheduler.yml:591`) walks | ||
| every organization repository looking for PRs that became mergeable after | ||
| their last triggering event fired (event-driven scheduler runs do not retry on | ||
| their own). GitHub Actions has no native primitive for "enumerate every org | ||
| repository's PR queue and act on each" — this requires the GitHub API calls | ||
| the job already makes; it is not something a `schedule:`/`concurrency:` block | ||
| alone could replace. | ||
|
|
||
| What plain Actions syntax *can* control, it already does: the schedule trigger | ||
| is deduplicated by workflow's own top-level `concurrency:` group | ||
| (`schedule-${{ github.event.schedule }}`), and the job carries a `timeout-minutes: 60` | ||
| ceiling plus several already-hard-won budget knobs | ||
| (`ORG_SWEEP_MAX_PRS`, `ORG_SWEEP_REVIEW_DISPATCH_LIMIT`, | ||
| `ORG_SWEEP_MAX_UNAVAILABLE`, rotation logic) whose comments cite the specific | ||
| production incidents that shaped them (#1219, #1223). | ||
|
|
||
| "Rate limit" covers at least two distinct resources here, and this item's | ||
| "rate limit issues" symptom should not be collapsed into one cause: | ||
|
|
||
| - The org's Actions **plan-level 60-concurrent-*job*** ceiling (#1754, | ||
| docs-only, merged) — a billing-tier constraint on how many jobs (of any | ||
| kind, any repo) can run at once. This is the one that best matches the | ||
| general "queue piles up instead of shrinking" symptom this loop-brief | ||
| opened with, and no workflow-file change can fix it. | ||
| - A separate, already-documented **LLM-provider rate limit** — a | ||
| `litellm.RateLimitError` storm against the shared NVIDIA NIM key from too | ||
| many *concurrent Strix/review callers* (`.github` PR #1297, 2026-08-23/24; | ||
| see `.github` PR #1661 / | ||
| `docs/doctoring/strix-cross-pr-concurrency-starvation-20260902.md`, not yet | ||
| merged to `main`). That is why `strix.yml`'s scan job deliberately | ||
| serializes per repository instead of per PR — a different mechanism, a | ||
| different resource, and not something `org-queue-sweep` itself triggers | ||
| directly (it can *dispatch* reviews, but it does not call an LLM provider | ||
| on its own). | ||
|
|
||
| `org-queue-sweep`'s own GitHub REST calls are subject to a third resource | ||
| (GitHub's per-token API rate limit), which is why it already paginates | ||
| conservatively and fails closed past `ORG_SWEEP_MAX_UNAVAILABLE` rather than | ||
| retrying harder. Two of the three resources already have a workflow-level | ||
| mitigation in place today (`strix.yml`'s per-repository serialization for the | ||
| LLM-provider limit; `org-queue-sweep`'s own pagination/budget ceilings for | ||
| its GitHub API calls) — this item is asking whether a *further* edit is | ||
| needed, not claiming no edit exists. Only the plan-level 60-job ceiling is | ||
| structurally outside any workflow file's reach, since it caps total | ||
| concurrent jobs org-wide regardless of how any single workflow is written. | ||
| No action taken; removing or rewriting `org-queue-sweep` would re-litigate an | ||
| already-evidenced design without touching any of the three resources. | ||
|
|
||
| ## Item 16 — consolidate the per-repo hourly-review-repair caller shown in the linked run | ||
|
|
||
| The linked run (`ContextualWisdomLab/.github` run `33524178483`, job | ||
| `99910668839`, workflow `governance-risk-compliance-hourly-review-repair.yml`) | ||
| failed at "Validate scheduler target and dispatch authority" because | ||
| `governance-risk-compliance` was hardcoded into the scheduler in a way the | ||
| validator rejected. Both problems are already fixed on this branch's base: | ||
|
|
||
| - The per-repo caller file itself no longer exists — consolidated into the | ||
| shared `hourly-review-repair.yml` matrix by PR #1673 | ||
| (`29b931e`, "refactor(actions): consolidate hourly review-repair callers"). | ||
| - The hardcode that made that specific run fail was replaced with an | ||
| org-variable admission path by PR #1743 (`8c08583`, already at the tip of | ||
| `main` this branch is based on; doctoring: this commit's own message and | ||
| `4f95abc`). | ||
|
|
||
| No action taken; the cited failure predates both fixes. | ||
|
|
||
| ## Item 17 — maximize GitHub Actions file consolidation org-wide | ||
|
|
||
| Already swept: `docs/doctoring/ci-workflow-duplication-audit-20260902.md` | ||
| (PR #1731, `9330d41`) re-checked all 63 non-archived/non-fork org repositories | ||
| (255 workflow files) for duplication beyond the hourly-review-repair, | ||
| R-CMD-check, and dependency-review consolidations already completed. Verdict: | ||
| 18 of 19 filename-collision groups are genuinely different policies (different | ||
| language/toolchain, security posture, thresholds, trust model, or job | ||
| topology — evidenced per group), and the one true near-duplicate | ||
| (`hourly-pr-maintenance.yml` in DiagramWeave/ThreadWeave) is already two | ||
| ~20-30 line thin callers of a shared reusable workflow, differing only by a | ||
| deliberate cron stagger — wrapping that further would be an unrequested | ||
| abstraction over two already-small files. No action taken; re-running this | ||
| audit from scratch would duplicate #1731 rather than extend it. | ||
|
|
||
| ## Item 18 — GitHub App installation token format change (`ghs_...`, ~520 chars, stateless) | ||
|
|
||
| Searched every `.py` and `.sh` file under `scripts/ci/` and `.github/` | ||
| (workflows, and the one composite action at | ||
| `.github/actions/orchestrator-free-sidecar/action.yml`), then re-checked the | ||
| whole repository tree (this repo has no `.yaml`-suffixed files, and | ||
| `opencode.jsonc` and the pinned `requirements-*.txt` files carry nothing | ||
| token-shaped either), for any assumption about installation-token length or | ||
| prefix shape: no fixed-length checks (`len(token) == N`, `token[:N]`), no | ||
| prefix/length regexes matching the old `ghs_` format, and no truncating | ||
| display logic keyed to a specific length. The only token-shaped regexes | ||
| present (`noema_review_gate.py:240,245`, `pr_review_merge_scheduler.py:254`) | ||
| are secret-redaction patterns (`token\s+<anything-non-whitespace>` -> `***`) | ||
| that mask a token of any length or format when logging — they do not depend | ||
| on the token being any particular size. No action taken; this repository has | ||
| nothing that would break under the announced longer, stateless | ||
| installation-token format. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 New repositories can miss CodeQL
Repositories using another default-branch name or source language receive no applicable scan from
codeql-pr.yml. The dynamic ruleset cannot provide universal coverage.Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed and fixed both catches:
j-planner's open PR 🛡️ Sentinel: Add organization-wide default security policy #2 (default branchgh-pages) received every other required check but zero CodeQL checks of any kind. Removedbranches: [main, master, develop]from codeql-pr.yml's pull_request trigger, matching security-scan.yml's own "do not restrict the base ref" precedent, and updated the one contract-test assertion that pinned the old line.🤖 Addressed by Claude Code