Skip to content

orb(drafting): apply the public-safe filter and the expectation formatter on both draft paths #9704

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

wantedPathCandidate in src/services/contributor-issue-draft.ts builds one candidate whose sections object contains two fields derived from the same manifest.testExpectations array, handled differently.

implementationRequirements:486:

...(manifest.testExpectations.length > 0 ? manifest.testExpectations.map((entry) => `Run ${entry} before requesting review.`) : []),

testingRequirements, four lines later in the same literal, calls buildContributorIssueDraftTestingRequirements(manifest):127-128:

const policyExpectations = manifest.testExpectations.filter(isFocusManifestPublicSafe).map(formatContributorIssueDraftTestExpectation);

Two mechanical divergences follow:

  1. No isFocusManifestPublicSafe filter. A single unsafe testExpectation in a repo's .loopover.yml is embedded in implementationRequirements, and the whole-body check at :281 then flips the candidate to skipped_unsafe — the entire wanted-path issue is dropped instead of the one offending line, which is precisely what the filter at :128 exists to prevent.
  2. No formatContributorIssueDraftTestExpectation. An expectation already phrased as a command produces doubled text: "Run npm run test:ci" becomes "Run Run npm run test:ci before requesting review." The formatter's guard at :139-141 exists specifically to avoid this, and testingRequirements in the same object gets it right.

Requirements

  • src/services/contributor-issue-draft.ts:486 filters through isFocusManifestPublicSafe and maps through formatContributorIssueDraftTestExpectation, exactly as :128 does. Both helpers are already in this module.
  • Because the two fields would then compute the identical list, extract that computation into one private helper in this file (e.g. publicSafeTestExpectations(manifest): string[]) used by both buildContributorIssueDraftTestingRequirements and wantedPathCandidate, so a third consumer cannot repeat the divergence.
  • buildContributorIssueDraftTestingRequirements's GENERIC_TESTING_REQUIREMENTS fallback when the filtered list is empty (:129) is unchanged and applies only to testingRequirementsimplementationRequirements keeps its existing "contribute nothing when the list is empty" behaviour (:486's : [] arm).
  • No change to isFocusManifestPublicSafe, formatContributorIssueDraftTestExpectation, or isContributorIssueDraftPublicSafe.

⚠️ Required pattern: buildContributorIssueDraftTestingRequirements at src/services/contributor-issue-draft.ts:127-133 is the correct handling to mirror, and the shared private helper is what stops the two paths re-diverging. It does NOT satisfy this issue to copy the .filter(...).map(...) chain inline at :486, leaving two hand-written copies; to fix only the doubled-"Run" formatting and leave the missing public-safe filter; or to change isContributorIssueDraftPublicSafe to be less strict about the whole body.

Deliverables

  • One private helper in src/services/contributor-issue-draft.ts produces the public-safe, formatted expectation list, and manifest.testExpectations is not read anywhere else in the file (grep-verifiable).
  • Named regression test: a manifest whose testExpectations includes one unsafe entry and one safe entry yields a wanted-path candidate whose status is not skipped_unsafe, and whose implementationRequirements contains the safe entry and not the unsafe one.
  • Named regression test: testExpectations: ["Run npm run test:ci"] yields an implementationRequirements entry equal to "Run npm run test:ci" — no "Run Run" — and a testingRequirements entry with the identical string.
  • testExpectations: ["npm run test:ci"] (no leading verb) yields "Run npm run test:ci before requesting review." in both fields — the formatter's other arm.
  • A manifest with an empty testExpectations still yields GENERIC_TESTING_REQUIREMENTS in testingRequirements and no expectation lines in implementationRequirements.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example adding the filter without the formatter, so the doubled-"Run" text survives — does not resolve this issue.

Test Coverage Requirements

99%+ Codecov patch coverage, branch-counted, on src/**; src/services/contributor-issue-draft.ts is inside coverage.include. Both arms of the formatter's guard (:139-141) and both arms of the public-safe filter must be exercised from the new shared helper, plus the empty-list fallback arm.

Expected Outcome

A repo whose .loopover.yml carries one unsafe test expectation still gets its wanted-path contributor issue drafted, with only the offending line removed. Expectations already phrased as commands render once, identically in both sections of the drafted issue.

Links & Resources

src/services/contributor-issue-draft.ts:120-142, :275-295, :470-500; src/signals/focus-manifest.ts.

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