rasterstate/fj
rasterstate/fj
Watch
8
0
Fork
You've already forked fj
 0
Code Issues 23 Pull requests Releases 7 Activity Actions

pr: cover negative system reviewer ids #247

Merged
stephen merged 1 commit from fix/246-negative-user-ids-pr-list into main 2026-08-09 20:54:11 +00:00
Conversation 2 Commits 1 Files changed 4 +125 -4
stephen commented 2026-08-09 20:00:01 +00:00
Owner
Copy link

Why

fj#246 showed that fj pr list --repo rasterstate/fj can fail to decode a successful Forgejo response when a PR contains Forgejo's forgejo-actions system account as a requested reviewer. Current main already has the signed User.id: i64 model fix from #231, but there was no PR-list fixture covering the real shape that broke fleet visibility, and list decode errors still did not name the endpoint or field.

What

  • Add a PR-list regression fixture with requested_reviewers: [{ "id": -2, "login": "forgejo-actions" }].
  • Decode list responses through serde_path_to_error, so failures name the list endpoint and JSON field path.
  • Add a regression for the improved message, e.g. GET /api/v1/repos/o/r/pulls?... and field [0].id.

Sweep

  • User.id is already signed on main and is the shared model for PR authors, requested reviewers, issue assignees, comments, collaborators, and repo owners. That covers the Forgejo ghost/system-account negative IDs.
  • Remaining u64 IDs in src/api are object identifiers: PRs, issues, labels, milestones, releases/assets, hooks, deploy keys, workflow runs/jobs/artifacts, orgs/teams, and notifications. I did not find another user-id field typed unsigned.
  • Existing null-list hardening is present on the high-risk collection fields touched by #241/#245: PR labels/requested reviewers, issue labels/assignees, branch-protection arrays, run-view arrays, and combined-status statuses.
  • I left object IDs unsigned in this PR; there is no evidence in #246 or the current API fixtures that Forgejo emits negative values for those object domains.

Testing

  • RUSTC_WRAPPER= cargo test pull_list_
  • RUSTC_WRAPPER= cargo test deserializes_negative_system_actor_id
  • RUSTC_WRAPPER= cargo test
  • RUSTC_WRAPPER= cargo clippy --all-targets -- -D warnings
  • RUSTC_WRAPPER= cargo run --quiet -- pr list --repo rasterstate/fj --no-pager
## Why fj#246 showed that `fj pr list --repo rasterstate/fj` can fail to decode a successful Forgejo response when a PR contains Forgejo's `forgejo-actions` system account as a requested reviewer. Current `main` already has the signed `User.id: i64` model fix from #231, but there was no PR-list fixture covering the real shape that broke fleet visibility, and list decode errors still did not name the endpoint or field. ## What - Add a PR-list regression fixture with `requested_reviewers: [{ "id": -2, "login": "forgejo-actions" }]`. - Decode list responses through `serde_path_to_error`, so failures name the list endpoint and JSON field path. - Add a regression for the improved message, e.g. `GET /api/v1/repos/o/r/pulls?...` and `field [0].id`. ## Sweep - `User.id` is already signed on `main` and is the shared model for PR authors, requested reviewers, issue assignees, comments, collaborators, and repo owners. That covers the Forgejo ghost/system-account negative IDs. - Remaining `u64` IDs in `src/api` are object identifiers: PRs, issues, labels, milestones, releases/assets, hooks, deploy keys, workflow runs/jobs/artifacts, orgs/teams, and notifications. I did not find another user-id field typed unsigned. - Existing null-list hardening is present on the high-risk collection fields touched by #241/#245: PR labels/requested reviewers, issue labels/assignees, branch-protection arrays, run-view arrays, and combined-status statuses. - I left object IDs unsigned in this PR; there is no evidence in #246 or the current API fixtures that Forgejo emits negative values for those object domains. ## Testing - `RUSTC_WRAPPER= cargo test pull_list_` - `RUSTC_WRAPPER= cargo test deserializes_negative_system_actor_id` - `RUSTC_WRAPPER= cargo test` - `RUSTC_WRAPPER= cargo clippy --all-targets -- -D warnings` - `RUSTC_WRAPPER= cargo run --quiet -- pr list --repo rasterstate/fj --no-pager`
pr: cover negative system reviewer ids
All checks were successful
Forseti review / forseti review (advisory) (pull_request_target) Successful in 43s
Details
ci / check (pull_request) Successful in 11m52s
Details
ci / coverage (pull_request) Successful in 2m5s
Details
ci / live-e2e (pull_request) Successful in 2m15s
Details
9ed3126bfa
forgejo-actions commented 2026-08-09 20:00:42 +00:00
Copy link

Forseti review

No blocking findings from the lead reviewer.

Findings

  • P2 src/client/mod.rs:685 — This replaces serde_json::from_str, which validates that the entire string is consumed, with a raw serde_json::Deserializer passed to serde_path_to_error::deserialize. That deserializes the first Vec<T> value but does not by itself call Deserializer::end(), so a malformed successful response like [{...}] trailing can be accepted as a valid page. That weakens the API response contract and can hide server/proxy corruption that was previously reported as a decode error.

  • PR: rasterstate/fj#247

  • Head SHA: 9ed3126bfae6

  • Review job: sha256:ad988286ca4da7c77867148af6775a9339c3c1722b5a355418d73efc4d7d461d

  • Provider pair: openai:gpt-5.5+anthropic:claude-haiku-4-5-20251001

  • Blocking findings: 0

  • Inline findings: 1

  • Model tokens: 44877 in / 3865 out (25138 from cache) ≈ $0.1529

  • Token source: GITHUB_TOKEN

  • Runner: 54957114493c

  • Run: https://rasterhub.com/rasterstate/fj/actions/runs/449

<!-- forseti:review {"version":2,"repo":"rasterstate/fj","pr":247,"head_sha":"9ed3126bfae62407045bec826bda8d44ebdc03c9","provider_pair":"openai:gpt-5.5+anthropic:claude-haiku-4-5-20251001","policy_version":"stub-policy-v1","prompt_version":"prompt-v2","context_fingerprint":"fnv64:0ead570aa568a398","review_job_key":"sha256:ad988286ca4da7c77867148af6775a9339c3c1722b5a355418d73efc4d7d461d","base_sha":"ba27179161a593d32874ae0a0f79e2e3a57620f9","role":"summary","status":"current"} --> ## Forseti review No blocking findings from the lead reviewer. ### Findings - **P2** `src/client/mod.rs:685` — This replaces `serde_json::from_str`, which validates that the entire string is consumed, with a raw `serde_json::Deserializer` passed to `serde_path_to_error::deserialize`. That deserializes the first `Vec<T>` value but does not by itself call `Deserializer::end()`, so a malformed successful response like `[{...}] trailing` can be accepted as a valid page. That weakens the API response contract and can hide server/proxy corruption that was previously reported as a decode error. - PR: `rasterstate/fj#247` - Head SHA: `9ed3126bfae6` - Review job: `sha256:ad988286ca4da7c77867148af6775a9339c3c1722b5a355418d73efc4d7d461d` - Provider pair: `openai:gpt-5.5+anthropic:claude-haiku-4-5-20251001` - Blocking findings: `0` - Inline findings: `1` - Model tokens: `44877 in / 3865 out` (`25138` from cache) ≈ `$0.1529` - Token source: `GITHUB_TOKEN` - Runner: `54957114493c` - Run: https://rasterhub.com/rasterstate/fj/actions/runs/449
forgejo-actions approved these changes 2026-08-09 20:00:43 +00:00
forgejo-actions left a comment
Copy link

Forseti review

No blocking findings from the lead reviewer.

Findings

  • P2 src/client/mod.rs:685 — This replaces serde_json::from_str, which validates that the entire string is consumed, with a raw serde_json::Deserializer passed to serde_path_to_error::deserialize. That deserializes the first Vec<T> value but does not by itself call Deserializer::end(), so a malformed successful response like [{...}] trailing can be accepted as a valid page. That weakens the API response contract and can hide server/proxy corruption that was previously reported as a decode error.

  • PR: rasterstate/fj#247

  • Head SHA: 9ed3126bfae6

  • Review job: sha256:ad988286ca4da7c77867148af6775a9339c3c1722b5a355418d73efc4d7d461d

  • Provider pair: openai:gpt-5.5+anthropic:claude-haiku-4-5-20251001

  • Blocking findings: 0

  • Inline findings: 1

  • Model tokens: 44877 in / 3865 out (25138 from cache) ≈ $0.1529

  • Token source: GITHUB_TOKEN

  • Runner: 54957114493c

  • Run: https://rasterhub.com/rasterstate/fj/actions/runs/449

<!-- forseti:review {"version":2,"repo":"rasterstate/fj","pr":247,"head_sha":"9ed3126bfae62407045bec826bda8d44ebdc03c9","provider_pair":"openai:gpt-5.5+anthropic:claude-haiku-4-5-20251001","policy_version":"stub-policy-v1","prompt_version":"prompt-v2","context_fingerprint":"fnv64:0ead570aa568a398","review_job_key":"sha256:ad988286ca4da7c77867148af6775a9339c3c1722b5a355418d73efc4d7d461d","base_sha":"ba27179161a593d32874ae0a0f79e2e3a57620f9","role":"summary","status":"current"} --> ## Forseti review No blocking findings from the lead reviewer. ### Findings - **P2** `src/client/mod.rs:685` — This replaces `serde_json::from_str`, which validates that the entire string is consumed, with a raw `serde_json::Deserializer` passed to `serde_path_to_error::deserialize`. That deserializes the first `Vec<T>` value but does not by itself call `Deserializer::end()`, so a malformed successful response like `[{...}] trailing` can be accepted as a valid page. That weakens the API response contract and can hide server/proxy corruption that was previously reported as a decode error. - PR: `rasterstate/fj#247` - Head SHA: `9ed3126bfae6` - Review job: `sha256:ad988286ca4da7c77867148af6775a9339c3c1722b5a355418d73efc4d7d461d` - Provider pair: `openai:gpt-5.5+anthropic:claude-haiku-4-5-20251001` - Blocking findings: `0` - Inline findings: `1` - Model tokens: `44877 in / 3865 out` (`25138` from cache) ≈ `$0.1529` - Token source: `GITHUB_TOKEN` - Runner: `54957114493c` - Run: https://rasterhub.com/rasterstate/fj/actions/runs/449
stephen deleted branch fix/246-negative-user-ids-pr-list 2026-08-09 20:54:11 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
backlog
Accepted but not scheduled

  
blocked
Blocked or stalled waiting on external progress

  
converted
Converted into tracked work

  
forseti:changes-requested
Forseti review found unaddressed findings; cleared on a clean re-review at the current head.

  
opportunity
Potential product or business opportunity

  
p1
Priority 1

  
p2
Priority 2

  
p3
Priority 3

  
parked
Paused intentionally until later

No labels
backlog
blocked
converted
forseti:changes-requested
opportunity
p1
p2
p3
parked
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
rasterstate/fj!247
No description provided.