Migrate PR review to openrouter with pinned 0.11.0 action #23
marvin8
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/issue-93-openrouter-review"
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?
Migrates the wuming PR review to the openrouter backend with the fleet-standard two-tier model mapping (code/tests/performance on deepseek-v4-pro, security on kimi-k2-thinking, config/docs/shell on qwen3.7-flash) and pins the action to
0.11.0.Tracking: marvin8/wuming#93
59b1b7c65600f16cffedWuMing
Found 1 issue(s). See inline comments below.
@ -13,3 +13,3 @@if: github.event_name == 'pull_request'steps:- uses: https://forge.marvin8.zone/marvin8/wuming@main- uses: https://forge.marvin8.zone/marvin8/wuming@0.11.0security [MEDIUM]
A08: Action pinned to mutable version tag (0.11.0) instead of SHA digest. Tags can be moved to point to compromised code, enabling supply chain attacks. Pin to immutable SHA digest for integrity verification.
#23 (comment)
🔴 Declined (fleet-wide maintainer decision, same as the identical findings across this migration — arvak, rainlog, zaojun, feed2fedi, archiving_imap): tag pinning was chosen so a wuming release update stays a one-line change per repo across ~17 consumer repos. The repointed-tag threat requires write access to the wuming repo itself — a single-maintainer repo on this forge. SHA pinning is the right follow-up only if that trust boundary grows.
WuMing
Found 1 issue(s). See inline comments below.
@ -720,3 +719,4 @@in_4_weeks = now.add(weeks=4, days_assumed_24h_ok=True)token_expires = Instant.from_timestamp(token_created_at + token_expires_in)_log = log.bind(phase="startup")_log.debug(f"token_expires={token_expires}")security [LOW]
A09: Logging token expiration times can leak security-sensitive timing information that may aid attackers in planning token refresh/reuse attacks. Consider redacting or removing this debug log from production code.
#23 (comment)
🔴 Declined (project-wide stance, established in wuming's review of the same finding class):
log.debugoutput is an opt-in diagnostics feature — it renders only for the operator running the TUI with debug enabled, in their own console. Token-expiry timing in operator-local debug logs is not attacker-visible, and redacting it would defeat the purpose of the diagnostic.WuMing
Found 2 issue(s). See inline comments below.
@ -717,3 +717,3 @@now = Instant.now()in_4_weeks = now.to_system_tz().add(weeks=4)in_4_weeks = now.add(weeks=4, days_assumed_24h_ok=True)code [LOW]
The change from now.to_system_tz().add(weeks=4) to now.add(weeks=4, days_assumed_24h_ok=True) may alter DST handling. The old code added weeks in the local timezone (potentially adjusting for DST transitions), while the new code adds a fixed duration on an Instant. Confirm that the token refresh calculation is intended to be UTC‑based and that ignoring DST shifts is safe.
tests [LOW]
No test file appears in the diff for this source file change. The logic change from
to_system_tz().add(weeks=4)toadd(weeks=4, days_assumed_24h_ok=True)modifies timezone handling and adds a new parameter, which should be covered by unit tests.src/kokopelli/tui.py
🔴 Intentional, and self-documenting via the flag:
days_assumed_24h_ok=Truestates exactly this trade-off. The comparison is a token-renewal heuristic — deciding whether a credential expires within the next four weeks — where a potential one-hour DST delta is immaterial. Whenever 0.11 deprecatedto_system_tz(), so the old construction was no longer available in that form.src/kokopelli/tui.py
🔴 The change is an API adaptation to the bumped
whenever(deprecation ofto_system_tz()), not a logic change: the produced instant differs from the old one by at most a DST hour, and the surrounding_maybe_renew_tokenbehaviour is exercised by the passing pytest sessions (3.12/3.14) in CI. A dedicated test for a deprecation-driven parameter would pin the library's API rather than kokopelli's logic.