shoko/jujutsu-skills
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases Wiki Activity

security(polymarket-browse): validate --detail argument and show error if out of range #33

Closed
shoko wants to merge 0 commits from security/7-detail-defaults into master
pull from: security/7-detail-defaults
merge into: shoko:master
Conversation 5 Commits - Files Changed -
shoko commented 2026-03-26 20:10:59 +01:00
Owner
Copy Link

Summary

Validate --detail index and show error message instead of silently defaulting to first event.

Changes

  • Add sys import for stderr/exit
  • Validate --detail index before accessing array
  • Show error with available range (e.g., "available: 1-5")
  • Exit with code 1 if --detail is out of range

Before

--detail 100 with only 5 events would silently show event 1.

After

--detail 100 with only 5 events shows:

Error: --detail 100 is out of range (available: 1-5).

Testing

70/70 tests passing

## Summary Validate --detail index and show error message instead of silently defaulting to first event. ## Changes - Add `sys` import for stderr/exit - Validate --detail index before accessing array - Show error with available range (e.g., "available: 1-5") - Exit with code 1 if --detail is out of range ## Before `--detail 100` with only 5 events would silently show event 1. ## After `--detail 100` with only 5 events shows: ``` Error: --detail 100 is out of range (available: 1-5). ``` ## Testing 70/70 tests passing
shoko added 1 commit 2026-03-26 20:10:59 +01:00
security(polymarket-browse): validate --detail argument and show error if out of range 3928cdef7c 
- Add sys import for stderr/exit
- Validate --detail index before accessing array
- Show error with available range instead of silent fallback to first event
- Exit with code 1 if --detail is out of range
shoko reviewed 2026-03-27 01:51:02 +01:00
shoko left a comment
Author
Owner
Copy Link

Review requested

Review requested
shoko added the need review label 2026-03-27 01:52:45 +01:00
han commented 2026-03-27 02:05:45 +01:00
Collaborator
Copy Link

I don't understand the issue. can you provide me like the case example what was happened before the patch and what will happen after the patch? I think we should verify that with unit test cases if there is no any yet. what do you think?

I don't understand the issue. can you provide me like the case example what was happened before the patch and what will happen after the patch? I think we should verify that with unit test cases if there is no any yet. what do you think?
han added need adjustmentneed clarification and removed need review labels 2026-03-27 02:06:25 +01:00
shoko commented 2026-03-27 02:06:33 +01:00
Author
Owner
Copy Link

Great question! Here is the case example:

Before the Patch

User runs: polymarket-browse --category "Counter Strike" --detail 100

Scenario: API returns only 5 match events.

What happened: The code silently defaulted to event #1. User expected to see details for event #100 but got event #1 without any warning. Confusing UX.

# Old code (lines 1226-1230)
if idx < 0 or idx >= len(result["match_events"]):
    idx = 0  # SILENTLY defaults to first event

After the Patch

User runs same command with only 5 events:

What happens now: Error message shown, exits with code 1.

Error: --detail 100 is out of range (available: 1-5).

Unit Tests

I agree - we should add unit tests. Currently there are no specific tests for this behavior. Should I:

  • (A) Add unit tests for --detail bounds validation to this PR
  • (B) Handle it separately after merge

What do you think?

Great question! Here is the case example: ## Before the Patch User runs: `polymarket-browse --category "Counter Strike" --detail 100` **Scenario**: API returns only 5 match events. **What happened**: The code silently defaulted to event #1. User expected to see details for event #100 but got event #1 without any warning. Confusing UX. ```python # Old code (lines 1226-1230) if idx < 0 or idx >= len(result["match_events"]): idx = 0 # SILENTLY defaults to first event ``` ## After the Patch User runs same command with only 5 events: **What happens now**: Error message shown, exits with code 1. ``` Error: --detail 100 is out of range (available: 1-5). ``` ## Unit Tests I agree - we should add unit tests. Currently there are no specific tests for this behavior. Should I: - **(A)** Add unit tests for --detail bounds validation to this PR - **(B)** Handle it separately after merge What do you think?
shoko added need review and removed need adjustmentneed clarification labels 2026-03-27 02:16:00 +01:00
shoko commented 2026-03-27 02:24:56 +01:00
Author
Owner
Copy Link

You raise a valid design concern. The current --detail N design is indeed flawed:

  1. List position is not stable - Position 3 can become position 7 in the next query
  2. No one wants detail for 10 markets - Why would you want --detail 1 through --detail 10?
  3. Wrong API endpoint - Uses public-search when there might be a better detail endpoint

However, this is out of scope for this PR. This PR only fixes the UX issue of silent fallback when --detail is out of range.

New Issue Created

I created Issue #39 to track this design discussion:
#39

Proposed approaches:

  • Option A: Separate CLI argument (--detail-by-id <event_id>)
  • Option B: Separate command/script (polymarket-detail --id <event_id>)
  • Option C: Change --detail to accept ID instead of index

Would you like to review the current PR as-is, or wait until we resolve Issue #39 to redesign the feature?

You raise a valid design concern. The current `--detail N` design is indeed flawed: 1. **List position is not stable** - Position 3 can become position 7 in the next query 2. **No one wants detail for 10 markets** - Why would you want `--detail 1` through `--detail 10`? 3. **Wrong API endpoint** - Uses `public-search` when there might be a better detail endpoint However, **this is out of scope for this PR**. This PR only fixes the UX issue of silent fallback when --detail is out of range. ## New Issue Created I created **Issue #39** to track this design discussion: https://git.fbrns.co/shoko/jujutsu-skills/issues/39 Proposed approaches: - Option A: Separate CLI argument (`--detail-by-id <event_id>`) - Option B: Separate command/script (`polymarket-detail --id <event_id>`) - Option C: Change `--detail` to accept ID instead of index Would you like to review the current PR as-is, or wait until we resolve Issue #39 to redesign the feature?
han approved these changes 2026-03-27 02:26:19 +01:00
han left a comment
Collaborator
Copy Link

lgtm

lgtm
han added approved and removed need review labels 2026-03-27 02:26:28 +01:00
shoko closed this pull request 2026-03-27 04:09:43 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
han
Labels
Clear labels
approved
bug
Bug fix
 critical
Critical severity - stop using until fixed
 enhancement
Enhancement or feature
 high
High severity - fix ASAP
 in review
low
Low severity
 medium
Medium severity
 need adjustment
need clarification
need review
security
Security-related issues
No Label approved
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
han shoko
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: shoko/jujutsu-skills#33
Delete Branch "security/7-detail-defaults"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?