Skip to content

Fail closed on branch eligibility for linked-issue scoring - #891

Merged
JSONbored merged 1 commit into
mainfrom
codex/fix-branch-eligibility-vulnerability
Jun 19, 2026
Merged

Fail closed on branch eligibility for linked-issue scoring#891
JSONbored merged 1 commit into
mainfrom
codex/fix-branch-eligibility-vulnerability

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • Prevent caller-controlled or missing branch-eligibility evidence from enabling the standard linked-issue multiplier, which previously treated missing/unknown eligibility as allowed and weakened score integrity.
  • Normalize caller-supplied eligibility claims so self-asserted eligible provenance cannot be mistaken for server-verified GitHub/local metadata.
  • Avoid public PR packet wording that claims local/GitHub verification when eligibility is only user-supplied.

Description

  • Require confirmed, provided, non-stale branch eligibility before applying the standard issue multiplier by replacing the permissive check with isConfirmedBranchEligible and using branchEligibilityFailureReason to produce fail-closed reasons. (changes in src/scoring/preview.ts).
  • Normalize API/MCP caller-provided branch eligibility by downgrading status: "eligible" to status: "unknown" and forcing source: "user_supplied" for incoming claims in src/api/routes.ts and src/mcp/server.ts so unverified input cannot be treated as authoritative.
  • Adjust public PR packet wording to only claim GitHub/local metadata checks when the branch eligibility source is not user_supplied and surface a conservative message otherwise (changes in src/signals/local-branch.ts).
  • Update unit and integration tests to reflect the fail-closed behavior and the new normalization of caller-supplied eligibility. (updated tests under test/unit and test/integration).

Testing

  • Ran typecheck with npm run typecheck and it succeeded.
  • Ran unit tests npm test -- --run test/unit/scoring.test.ts test/unit/local-branch.test.ts and they passed.
  • Ran integration tests npm test -- --run test/integration/api.test.ts and they passed.
  • Ran the combined test run npm test -- --run test/unit/scoring.test.ts test/unit/local-branch.test.ts test/integration/api.test.ts test/unit/mcp-cli.test.ts with all tests passing (all updated suites green).

Codex Task

@ghost

ghost commented Jun 18, 2026

Copy link
Copy Markdown

Note

Gittensory Gate skipped

PR closed before full evaluation. No late first comment was created.

Signal Result Evidence Action
Gate result ⚠️ Skipped #891 is no longer open. No action.

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

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

@ghost ghost added the gittensory:reviewed label Jun 18, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.68%. Comparing base (5d9d73a) to head (6a31462).

Files with missing lines Patch % Lines
src/scoring/preview.ts 62.50% 2 Missing and 1 partial ⚠️
src/signals/local-branch.ts 33.33% 1 Missing and 1 partial ⚠️
src/api/routes.ts 0.00% 0 Missing and 1 partial ⚠️
src/mcp/server.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #891      +/-   ##
==========================================
- Coverage   96.71%   96.68%   -0.04%     
==========================================
  Files         108      108              
  Lines       14641    14653      +12     
  Branches     5298     5304       +6     
==========================================
+ Hits        14160    14167       +7     
- Misses        102      106       +4     
- Partials      379      380       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ghost

ghost commented Jun 18, 2026

Copy link
Copy Markdown

gittensory · advisory review

Reviewed 7 changed file(s) — two independent AI reviewers.

Changed files: src/api/routes.ts, src/mcp/server.ts, src/scoring/preview.ts, src/signals/local-branch.ts, test/integration/api.test.ts, test/unit/local-branch.test.ts, test/unit/scoring.test.ts

Suggested action: 🛠️ Request changes. (reviewers split: merge / request changes) Address the suggestions below before merging.

Reviewer A · gpt-oss-120b — recommends ✅ merge
The PR tightens branch‑eligibility handling by failing closed when eligibility evidence is missing, stale, or user‑supplied, adding helper functions and schema transforms, and updates scoring logic and tests accordingly.

Suggestions

  • Add a unit test covering the stale‑evidence path to guarantee the new fail‑closed behavior.
  • Update any API documentation to note that client‑provided "eligible" status will be treated as unknown and ignored.

Worth double-checking

  • Clients that previously sent "eligible" may now see reduced multipliers; ensure this is an intended change.
  • Confirm that all internal callers still supply a full BranchEligibilityResult (required field) to avoid runtime undefined errors.

Reviewer B · mistral-small-3.1-24b-instruct — recommends 🛠️ request changes
This PR updates the logic for determining branch eligibility in linked-issue scoring, ensuring that branch eligibility is strictly enforced. The changes are well-documented and include updates to both the scoring logic and the corresponding tests.

Suggestions

  • Consider adding a comment explaining why the branchEligibility status is transformed to 'unknown' in the callerBranchEligibilitySchema. This will help future maintainers understand the rationale behind this transformation.
  • Ensure that the new branchEligibilityFailureReason function covers all possible edge cases for branch eligibility statuses.

Worth double-checking

  • Verify that the transformation of branchEligibility status to 'unknown' in the callerBranchEligibilitySchema does not introduce any unintended side effects in other parts of the codebase.
  • Check that the updated tests cover all possible scenarios for branch eligibility, including edge cases.

@JSONbored
JSONbored merged commit 42cf7d2 into main Jun 19, 2026
18 of 20 checks passed
@JSONbored
JSONbored deleted the codex/fix-branch-eligibility-vulnerability branch June 19, 2026 05:50
@github-actions github-actions Bot mentioned this pull request Jun 19, 2026
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant