Skip to content

fix(orb): filter contributor-draft implementation requirements through the public-safe path - #9755

Merged
JSONbored merged 1 commit into
JSONbored:mainfrom
shin-core:fix/contributor-draft-testexpectations-parity-9704
Jul 29, 2026
Merged

fix(orb): filter contributor-draft implementation requirements through the public-safe path#9755
JSONbored merged 1 commit into
JSONbored:mainfrom
shin-core:fix/contributor-draft-testexpectations-parity-9704

Conversation

@shin-core

Copy link
Copy Markdown
Contributor

What & why

Closes #9704.

wantedPathCandidate in contributor-issue-draft.ts built two sections from the same
manifest.testExpectations array but handled them differently:

  • testingRequirements ran it through isFocusManifestPublicSafe + formatContributorIssueDraftTestExpectation.
  • implementationRequirements, four lines away, mapped the raw entries into Run ${entry} before requesting review. — no public-safe filter, a different format.

So a public-unsafe expectation was dropped from one contributor-facing list but rendered verbatim in
the other, and the two "Run" spellings diverged.

Change

Extract the shared computation into one private publicSafeTestExpectations(manifest) helper, used by
both buildContributorIssueDraftTestingRequirements and wantedPathCandidate, so the filter+format is
applied identically and a third consumer cannot repeat the divergence. The GENERIC_TESTING_REQUIREMENTS
fallback stays specific to testingRequirements; implementationRequirements keeps its
contribute-nothing-when-empty behaviour. No change to isFocusManifestPublicSafe,
formatContributorIssueDraftTestExpectation, or isContributorIssueDraftPublicSafe.

Validation

  • A public-unsafe expectation ("wallet seed phrase") is filtered out of both lists and never
    reaches the draft (the draft is produced, not skipped, and carries no such text).
  • A wanted-path candidate's implementationRequirements carry the identical public-safe-filtered
    expectations as testingRequirements.
  • Bug-catch verified: reverting implementationRequirements to the raw map fails both new tests.
  • 100% patch coverage on the changed lines.

…h the public-safe expectation path

wantedPathCandidate built two sections from the same manifest.testExpectations
array but handled them differently. testingRequirements ran the array through
isFocusManifestPublicSafe + formatContributorIssueDraftTestExpectation (via
buildContributorIssueDraftTestingRequirements), while implementationRequirements
four lines away mapped the raw entries into `Run ${entry} before requesting
review.` -- no public-safe filter and a different format. A public-unsafe
expectation was dropped from one contributor-facing list but rendered verbatim in
the other, and the two "Run" spellings diverged.

Extract the shared computation into one private publicSafeTestExpectations(manifest)
helper and use it from both buildContributorIssueDraftTestingRequirements and
wantedPathCandidate, so the filter+format is applied identically and a third
consumer cannot repeat the divergence. The GENERIC_TESTING_REQUIREMENTS fallback
stays specific to testingRequirements, and implementationRequirements keeps its
contribute-nothing-when-empty behaviour. No change to isFocusManifestPublicSafe,
formatContributorIssueDraftTestExpectation, or isContributorIssueDraftPublicSafe.

Adds tests: a public-unsafe expectation is filtered out of BOTH lists (never
reaches the draft), and a wanted-path candidate's implementationRequirements carry
the identical public-safe-filtered expectations as testingRequirements.

Closes JSONbored#9704
@shin-core
shin-core requested a review from JSONbored as a code owner July 29, 2026 06:30
@superagent-security

Copy link
Copy Markdown
Contributor

🚨 Contributor flagged. Click here for more info: Superagent Dashboard

@superagent-security superagent-security Bot added the contributor:flagged Contributor flagged for review by trust analysis. label Jul 29, 2026
@loopover-orb

loopover-orb Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-29 06:38:56 UTC

2 files · 1 AI reviewer · no blockers · CI green · unstable

⏸️ Suggested Action - Manual Review

  • Required AI review was skipped by a submitter-reputation downgrade: This repository requires blocking AI review, and the submitter's recent-submission signal downgraded this PR to deterministic-only checks. Those checks do not read code semantics, so the gate is held for human review instead of passing automatically.

Review summary
Extracts the duplicated filter+format logic into a shared publicSafeTestExpectations helper so testingRequirements and implementationRequirements apply the same public-safe filter and formatting to manifest.testExpectations, closing the divergence bug in #9704. The fix is correctly traced to the source (the raw map at the old implementationRequirements line is replaced with the shared helper), tests assert both lists are identical and that an unsafe entry is dropped from both, and a revert test confirms the fix is load-bearing rather than can't-occur. Behavior for empty testExpectations is preserved (GENERIC_TESTING_REQUIREMENTS stays scoped to testingRequirements; implementationRequirements contributes nothing when empty), matching the stated intent.

Nits — 6 non-blocking
  • The JSDoc comment on publicSafeTestExpectations (contributor-issue-draft.ts:127-129) references issue orb(drafting): apply the public-safe filter and the expectation formatter on both draft paths #9704 by number inline in a doc comment rather than a linked constant/reference — minor, but the external brief flags the bare numeric literal as slightly unusual style for this file.
  • contributor-issue-draft.ts is now ~498 lines; consider whether wantedPathCandidate/policyWarningCandidate/upstreamDriftCandidate could move to a separate module in a future pass, though this PR doesn't need to do that itself.
  • The renamed test title changes assertion semantics (now expects skipped_unsafe===false) — worth double-checking no other test or downstream consumer still expects the old skipped_unsafe-on-unsafe-expectation behavior for implementationRequirements specifically.
  • Consider adding a short comment or test asserting that formatContributorIssueDraftTestExpectation's idempotent-format branches (already-'Run '-prefixed, 'must pass', trailing '.') are exercised via the shared helper path, not just the default 'Run X before requesting review' case, since implementationRequirements previously never touched this formatter at all.
  • The PR's shared helper is a good pattern to prevent a third consumer from re-diverging — call this out explicitly as the main value of the change in the PR description if not already emphasized enough for reviewers unfamiliar with orb(drafting): apply the public-safe filter and the expectation formatter on both draft paths #9704.
  • Required AI review was skipped by a submitter-reputation downgrade — Review this PR manually, or run AI review with a trusted override, before merging.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ⚠️ Gate result — Not blocking (Advisory; not blocking this PR.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #9704
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ❌ 5/25 Preflight is holding this PR: the review lane is unavailable, so it is not ready for automated review.
Contributor workload ✅ 10/10 Author activity: 38 registered-repo PR(s), 26 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor shin-core; Gittensor profile; 38 PR(s), 0 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Linked issue satisfaction

Partially addressed
The diff correctly extracts a shared publicSafeTestExpectations helper and applies it to both implementationRequirements and testingRequirements, which fixes the core divergence and is validated by a new identical-list test and an updated skipped_unsafe test showing the unsafe entry no longer leaks. However, the issue explicitly required named regression tests for the doubled-"Run" formatting case

Review context
  • Author: shin-core
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is registered but has no active allocation in the current snapshot.
  • Public profile languages: not available
  • Official Gittensor activity: 38 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Await review-lane availability.
  • Then work through the remaining 2 steps in the Signals table above.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

Decision record
  • action: hold · clause: ai_review_inconclusive
  • config: a7a687ac2633933328bd51c416081538e701641d421c4573461cab42d788a54b · pack: oss-anti-slop · ci: passed
  • record: 464101ef225040e5d40a2944aed6851c8e21e2469ab3e634d9ee5fb03f9671cf (schema v5, head 4fc6b74)

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 29, 2026
@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.63%. Comparing base (5ecb771) to head (4fc6b74).

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #9755       +/-   ##
===========================================
- Coverage   90.27%   76.63%   -13.65%     
===========================================
  Files         904      283      -621     
  Lines      113177    59574    -53603     
  Branches    26840     8516    -18324     
===========================================
- Hits       102171    45653    -56518     
- Misses       9676    13633     +3957     
+ Partials     1330      288     -1042     
Flag Coverage Δ
backend 93.54% <100.00%> (-2.00%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/services/contributor-issue-draft.ts 93.54% <100.00%> (-3.23%) ⬇️

... and 755 files with indirect coverage changes

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 29, 2026
@JSONbored
JSONbored merged commit 67eddea into JSONbored:main Jul 29, 2026
7 of 8 checks passed
JSONbored added a commit that referenced this pull request Jul 29, 2026
One import conflict in src/api/routes.ts: main widened the ledger-anchor
import with `publicAnchorStatus` (#9755's empty-anchor-list reason) while
this branch added the proof-summary and proof-badge imports beside it.
Both sides kept -- neither change displaces the other, and
`publicAnchorStatus` is used at routes.ts:1362 on main's side of the merge.

Verified after resolving rather than assuming a clean textual merge means
a clean semantic one: regenerated the OpenAPI spec (no drift), and ran
both sides' suites -- proof-summary, ledger-anchor-persistence, the
route/spec ratchet and auth -- 98 passing. docs-drift, engine-parity and
dead-source-files all clean.
JSONbored added a commit that referenced this pull request Jul 29, 2026
…nd ship the missing keypair runbook (#9719) (#9761)

#9719 diagnosed the topology precisely and it holds: hosted review
execution is retired (src/index.ts:61-75 acks and drops review jobs), so
api.loopover.ai never decides anything, its ledger is permanently empty,
and `runScheduledLedgerAnchor` early-returns `empty_ledger` on every tick
forever. Verified against the code rather than taken on faith.

DECISION on the issue's item 1 -- anchoring lives on self-host, and the
docs say so. The alternative (ingest decision records centrally so the
hosted Worker has something to anchor) was rejected on the merits: the
central chain would be a re-chained AGGREGATE of many instances' records,
so anchoring it proves nothing about the per-instance chain a verifier
actually cares about, and every real chain would still be unanchored. It
would produce a MORE authoritative-looking artifact carrying a WEAKER
guarantee -- the exact "looks verified, isn't" failure this epic exists to
prevent.

The user-facing half of that was a genuine false claim: what-you-can-verify
told verifiers to `curl https://api.loopover.ai/v1/public/decision-ledger/
anchors`, an endpoint that returns `[]` forever, as THE anchor walkthrough.
Every command now runs against `$ORB` -- the instance that reviewed the PR
-- with the boundary and the reasoning stated where the reader hits it.

Item 2 was simply missing: no file outside src/ and test/ mentioned
LEDGER_ANCHOR at all, so both halves had to be hand-produced in exact
encodings, with a keyId that is computeAnchorKeyId of the public half.
`npm run ledger:anchor-keygen` now emits both, deriving the id with the
same function the runtime uses. The derivation is exported and returns
values rather than only printing, because a generator that emits a key the
runtime REFUSES would reproduce the original failure while looking like a
fix -- six tests run the generated values through the real
parse/select/sign/verify path, including that a signature does not verify
against a different run's key.

Plus the operator runbook that did not exist: what to set, which half is
secret, how to read the four `status` values, and the rotation rule that a
retired key is closed with notAfter and never removed.

Item 3 (make the silence visible) was already closed by #9755's
publicAnchorStatus -- checked before duplicating it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:flagged Contributor flagged for review by trust analysis. gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants