Skip to content

settings(flags): LOOPOVER_DUPLICATE_WINNER and LOOPOVER_OPEN_PR_FILE_COLLISION reject the repo's own truthy convention #10054

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

This codebase has one documented truthy convention for a LOOPOVER_* boolean env flag: a trimmed,
case-insensitive
/^(1|true|yes|on)$/i. It is named as "the codebase convention" in the flag modules
themselves —

// src/settings/automation-bot-skip.ts:5
/** Truthy convention matches the rest of this codebase (`/^(1|true|yes|on)$/i`, e.g. isReputationEnabled), ... */

— and is what every other flag reader actually does: src/review/reputation-wire.ts:37,
src/review/safety.ts:28, src/review/impact-map-wire.ts:26, src/review/e2e-test-gen-wire.ts:17,
src/review/maintainer-recap-wire.ts:37, src/review/public-stats.ts:82,
src/settings/repository-settings.ts:12, src/selfhost/ai.ts:1640, src/selfhost/redis-cache.ts:150,
src/auth/security.ts:185, src/mcp/server.ts:790, src/selfhost/private-config.ts:337. src/env.d.ts:301
even documents the regex verbatim as the contract for a flag.

Two flags do not follow it, and each asserts in its own JSDoc that its behaviour is the convention:

// src/settings/duplicate-winner-mode.ts:3
/** Truthy convention matches the rest of this codebase's `LOOPOVER_*` flags (exact `"true"` string, e.g. the
 *  raw checks this replaces) -- ... */
export function isDuplicateWinnerEnabledGlobally(env: { LOOPOVER_DUPLICATE_WINNER?: string | undefined }): boolean {
  return env.LOOPOVER_DUPLICATE_WINNER === "true";
}
// src/settings/open-pr-file-collision-mode.ts:3
/** Truthy convention matches the rest of this codebase's `LOOPOVER_*` flags (exact `"true"` string) -- opt-in
 *  and default OFF: ... */
export function isOpenPrFileCollisionEnabledGlobally(env: { LOOPOVER_OPEN_PR_FILE_COLLISION?: string | undefined }): boolean {
  return env.LOOPOVER_OPEN_PR_FILE_COLLISION === "true";
}

Both JSDoc claims are false: the exact-"true" form is used by these two flags and nowhere else among the
LOOPOVER_* flags. The consequence is a silent operator footgun on exactly the surfaces where the value is
hand-written:

  • LOOPOVER_DUPLICATE_WINNER=1 — the value form the convention regex accepts first, and the form
    src/env.d.ts:301 publishes as valid for a sibling flag — reads as OFF. env.d.ts:737 describes the
    flag as "when truthy", so nothing tells the operator that 1 is not truthy here.
  • A trailing space or a TRUE / True spelling reads as OFF. Docker/compose .env files routinely carry
    trailing whitespace; every convention-following flag trims and lowercases, these two do not.
  • There is no warning on either path. The flag simply behaves as unset, and the observable effect is a policy
    difference — with LOOPOVER_DUPLICATE_WINNER off, every duplicate sibling gets the duplicate_pr_risk
    blocker and the "duplicate of another open PR" close reason instead of the earliest claimant being spared
    (src/env.d.ts:729src/env.d.ts:736). An operator who set =1 and observed sibling PRs being closed has
    no signal that the flag never took effect.

A third restatement of the wrong contract sits in a JSDoc a planner author will copy:

// src/rules/advisory.ts:379
 *  caller sets this to `env.LOOPOVER_DUPLICATE_WINNER === "true"`. */

Every real caller in fact routes through isDuplicateWinnerEnabledGlobally (src/queue/processors.ts:1611,
src/queue/duplicate-detection.ts:95, src/api/routes.ts:2803, src/mcp/server.ts:2386), so this comment
documents a form nothing uses and re-teaches the wrong convention.

Requirements

  • isDuplicateWinnerEnabledGlobally (src/settings/duplicate-winner-mode.ts:7) and
    isOpenPrFileCollisionEnabledGlobally (src/settings/open-pr-file-collision-mode.ts:7) must both read
    their flag as /^(1|true|yes|on)$/i.test((env.<VAR> ?? "").trim()) — byte-identical in shape to
    isSelfTuneEnabled/selfTuneFlagOn at src/settings/repository-settings.ts:12.
  • Both JSDoc blocks must stop claiming exact-"true" is the codebase convention and instead name the real
    one, exactly as src/settings/automation-bot-skip.ts:5 does.
  • The stale JSDoc at src/rules/advisory.ts:379 must be corrected to name
    isDuplicateWinnerEnabledGlobally as the resolver, rather than restating a raw comparison.
  • "true" must still resolve to true and unset/empty must still resolve to false: this widens the
    accepted set, it does not change the default or the meaning of any value that works today.
  • Must NOT change: resolveDuplicateWinnerEnabled / resolveOpenPrFileCollisionEnabled
    (src/settings/duplicate-winner-mode.ts:17, src/settings/open-pr-file-collision-mode.ts:17) — the
    per-repo inherit/off/enabled override resolution is correct and load-bearing and must keep its exact
    current behaviour; the wrangler.jsonc values (wrangler.jsonc:253, wrangler.jsonc:262) are already
    "true" and must keep working unchanged.

⚠️ Required pattern: mirror selfTuneFlagOn at src/settings/repository-settings.ts:12
/^(1|true|yes|on)$/i.test((env.X ?? "").trim()). What does NOT satisfy this issue: (a) introducing a new
shared isTruthyFlag() helper and refactoring the ~12 existing convention-following call sites onto it —
that is a repo-wide blast radius for a two-function defect and is explicitly out of scope here; (b) a
doc-only PR that fixes the three JSDoc comments and leaves the two comparisons as === "true"; (c) making
the two flags default ON, or changing what resolve*Enabled does with "off"/"enabled".

Deliverables

  • isDuplicateWinnerEnabledGlobally (src/settings/duplicate-winner-mode.ts) returns true for each of
    "1", "true", "TRUE", "yes", "on", " true " and false for each of undefined, "",
    "0", "false", "off", "no", "maybe" — asserted in test/unit/duplicate-winner-mode.test.ts.
  • isOpenPrFileCollisionEnabledGlobally (src/settings/open-pr-file-collision-mode.ts) satisfies the
    identical table — asserted in test/unit/open-pr-file-collision-mode.test.ts.
  • The JSDoc on both functions names /^(1|true|yes|on)$/i as the convention and no longer claims exact
    "true" is it.
  • src/rules/advisory.ts:379's comment names isDuplicateWinnerEnabledGlobally instead of the raw
    === "true" comparison.
  • A regression test named for this bug in test/unit/duplicate-winner-mode.test.ts asserting
    resolveDuplicateWinnerEnabled(isDuplicateWinnerEnabledGlobally({ LOOPOVER_DUPLICATE_WINNER: "1" }), "inherit")
    is true — i.e. the =1 form an operator naturally writes actually enables the feature end-to-end
    through the resolver, not just through the global reader.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example one
that fixes isDuplicateWinnerEnabledGlobally and leaves isOpenPrFileCollisionEnabledGlobally on
=== "true", or one that only edits comments — does not resolve this issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's coverage.include
covers src/**/*.ts and packages/loopover-engine/src/**/*.ts; both src/settings/duplicate-winner-mode.ts
and src/settings/open-pr-file-collision-mode.ts are measured. Each rewritten function introduces one new
branch — the ?? "" nullish fallback — and both arms need a test: the var absent (undefined, taking the
"" arm) and the var present (taking the supplied-value arm). The regex itself is one expression but its
true and false results must each be exercised, which the value tables above already do.

Expected Outcome

LOOPOVER_DUPLICATE_WINNER=1 and LOOPOVER_OPEN_PR_FILE_COLLISION=on enable their features, the same way
every other LOOPOVER_* flag in this codebase behaves, and a stray trailing space or an uppercase TRUE no
longer silently disables a policy an operator believes they turned on. The three JSDoc comments stop teaching
a convention the repo does not use.

Links & Resources

  • src/settings/duplicate-winner-mode.ts:3 — the false convention claim and the === "true" read
  • src/settings/open-pr-file-collision-mode.ts:3 — the same, for the second flag
  • src/settings/repository-settings.ts:12 — the pattern to mirror
  • src/settings/automation-bot-skip.ts:5 — the correct convention statement, in a sibling settings module
  • src/rules/advisory.ts:379 — the third restatement of the wrong contract
  • src/env.d.ts:729, src/env.d.ts:738 — the two flags' documented "when truthy" semantics

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions