Skip to content

fix(api-keys): backfill new limit usage#748

Closed
Komzpa wants to merge 7 commits into
Soju06:mainfrom
Komzpa:revive/pr-522
Closed

fix(api-keys): backfill new limit usage#748
Komzpa wants to merge 7 commits into
Soju06:mainfrom
Komzpa:revive/pr-522

Conversation

@Komzpa
Copy link
Copy Markdown
Collaborator

@Komzpa Komzpa commented May 21, 2026

Summary

Revives #522 on a maintainer-owned branch because the original PR head has maintainerCanModify=false.

Credit: original implementation by @mgwals in #522; this branch rebases and updates that work on current main.

  • Backfills newly added API key limits from successful request-log usage in the active lookback window.
  • Preserves explicit resetUsage behavior and updates the backfill to sum per-request truncated microdollars so historical cost counters match stored request accounting.

Validation

  • uv run pytest tests/integration/test_api_keys_trends_api.py -q passed: 13 passed
  • uv run pytest tests/unit/test_api_keys_service.py -q passed: 47 passed
  • uv run pytest tests/unit/test_api_keys_repository.py -q passed: 6 passed
  • uv run ruff check app/modules/api_keys tests/integration/test_api_keys_trends_api.py tests/unit/test_api_keys_service.py passed
  • git diff --check origin/main...HEAD passed

OpenSpec

  • backfill-api-key-limit-current-usage remains included and updated.

Revives #522

Fixes #518

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a57cfba708

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread app/modules/api_keys/service.py Outdated
@Soju06 Soju06 added the 🤖 codex: needs work [@codex review] raised an issue label May 21, 2026
@Soju06 Soju06 removed the 🤖 codex: needs work [@codex review] raised an issue label May 21, 2026
@Soju06
Copy link
Copy Markdown
Owner

Soju06 commented May 21, 2026

@codex review

1 similar comment
@Komzpa
Copy link
Copy Markdown
Collaborator Author

Komzpa commented May 22, 2026

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ef82e9dcd6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread app/modules/api_keys/repository.py Outdated
@Soju06 Soju06 added the 🤖 codex: needs work [@codex review] raised an issue label May 22, 2026
@Komzpa Komzpa removed the 🤖 codex: needs work [@codex review] raised an issue label May 22, 2026
@Komzpa
Copy link
Copy Markdown
Collaborator Author

Komzpa commented May 22, 2026

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@Soju06 Soju06 added the 🤖 codex: ok [@codex review] says no issues found. label May 22, 2026
@Soju06
Copy link
Copy Markdown
Owner

Soju06 commented May 27, 2026

Merge-gate hold: code/tests look good, but this appears to resolve #518 while the PR body only says Revives #522 and closingIssuesReferences is empty.

Given the repo merge gate for issue-resolving PRs, please add Fixes #518 or Closes #518 to the PR body (assuming #518 is indeed intended to be closed by this backfill fix). Focused tests/OpenSpec passed in review; this looks merge-ready after that bookkeeping fix.

@Soju06 Soju06 added the hermes: needs-followup Hermes left a blocker/comment that needs follow-up observation label May 27, 2026
@Komzpa Komzpa removed the hermes: needs-followup Hermes left a blocker/comment that needs follow-up observation label May 27, 2026
@Soju06
Copy link
Copy Markdown
Owner

Soju06 commented Jun 1, 2026

Merge-gate blocker: the implementation/tests look good, but the OpenSpec delta leaves the API-key SSOT contradictory after archive/sync.

Evidence:

  • openspec/changes/backfill-api-key-limit-current-usage/specs/api-keys/spec.md:6 adds the new behavior: a newly submitted limit rule without resetUsage initializes current_value from successful request-log usage in the current window.
  • Existing main spec openspec/specs/api-keys/spec.md:323-329 still says under “Limit update with usage state preservation”: New rule (no match): current_value=0 and fresh reset_at.
  • uv run openspec change show backfill-api-key-limit-current-usage --json --deltas-only reports only one parsed delta, modifying Requirement: API Key update; it does not modify the existing “Limit update with usage state preservation” requirement.

So the code would satisfy #518, but the spec archive would leave two normative requirements saying opposite things for the same PATCH behavior. Please update the OpenSpec change to also modify the existing “Limit update with usage state preservation” requirement so the “New rule” case reflects backfill behavior, with resetUsage=true as the explicit zero-reset path.

@Soju06 Soju06 added the hermes: needs-followup Hermes left a blocker/comment that needs follow-up observation label Jun 1, 2026
@Soju06 Soju06 removed the 🤖 codex: ok [@codex review] says no issues found. label Jun 1, 2026
@Soju06
Copy link
Copy Markdown
Owner

Soju06 commented Jun 1, 2026

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@Soju06 Soju06 added the 🤖 codex: ok [@codex review] says no issues found. label Jun 1, 2026
@Soju06 Soju06 removed the 🤖 codex: ok [@codex review] says no issues found. label Jun 1, 2026
@Komzpa
Copy link
Copy Markdown
Collaborator Author

Komzpa commented Jun 1, 2026

Addressed in b826e9e.

  • The OpenSpec delta now also modifies Requirement: Limit update with usage state preservation.
  • openspec change show backfill-api-key-limit-current-usage --json --deltas-only now reports two parsed deltas, including that requirement.
  • uv run openspec validate backfill-api-key-limit-current-usage --strict passes.

This keeps the archive result consistent: new unmatched limits backfill current-window successful request-log usage unless resetUsage is true, which remains the explicit zero-reset path.

@Komzpa
Copy link
Copy Markdown
Collaborator Author

Komzpa commented Jun 1, 2026

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@Soju06 Soju06 added the 🤖 codex: ok [@codex review] says no issues found. label Jun 1, 2026
@Komzpa Komzpa removed 🤖 codex: ok [@codex review] says no issues found. hermes: needs-followup Hermes left a blocker/comment that needs follow-up observation labels Jun 1, 2026
@Soju06
Copy link
Copy Markdown
Owner

Soju06 commented Jun 1, 2026

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@Komzpa
Copy link
Copy Markdown
Collaborator Author

Komzpa commented Jun 2, 2026

#748 has been folded into #832.

The surviving API-key usage PR is #832 at f0c54e194a6f3355f2591aa47771f27e2201d3ad; its body now lists #748 under folded PRs and keeps the backfill behavior in the same API-key usage feature bucket.

Closing this one to keep the open queue non-overlapping.

@Komzpa Komzpa closed this Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖 codex: ok [@codex review] says no issues found.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Limits are not applying after add limit rule to existing API key with current usage

3 participants