Skip to content

Tighten opportunity queue flow#47

Open
mrmsoc09 wants to merge 1 commit into
mainfrom
codex/opportunity-queue-flow
Open

Tighten opportunity queue flow#47
mrmsoc09 wants to merge 1 commit into
mainfrom
codex/opportunity-queue-flow

Conversation

@mrmsoc09

Copy link
Copy Markdown
Owner

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a hunter-account inventory system that imports Proton Pass CSV exports into Vault, maps them to runtime environment overrides, and ranks scan opportunities based on account coverage. It also adds automated database migrations on startup, unifies the cryptographic signing identity to kaisonai@pm.me, and implements light/dark theme support in the frontend operator console. The review feedback highlights several important issues: a parsing bug in _row_timestamp when handling ISO 8601 timestamps, unprofessional placeholder language in .env.example, a duplicate import at the end of startup_migrations.py, and a potential runtime crash in credentials.py if scope_domains is missing.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +106 to +111
def _row_timestamp(row: dict[str, str]) -> int:
raw = (row.get("createTime") or row.get("modifyTime") or "").strip()
try:
return int(raw)
except ValueError:
return 0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Proton Pass exports typically format createTime and modifyTime as ISO 8601 strings (e.g., 2023-10-27T14:20:00Z). Attempting to directly cast these strings to int will always raise a ValueError and return 0, rendering the timestamp-based sorting in _pick_latest_row ineffective. We should parse ISO 8601 strings using datetime.fromisoformat as a fallback.

Suggested change
def _row_timestamp(row: dict[str, str]) -> int:
raw = (row.get("createTime") or row.get("modifyTime") or "").strip()
try:
return int(raw)
except ValueError:
return 0
def _row_timestamp(row: dict[str, str]) -> int:
raw = (row.get("createTime") or row.get("modifyTime") or "").strip()
if not raw:
return 0
try:
return int(raw)
except ValueError:
try:
from datetime import datetime
return int(datetime.fromisoformat(raw.replace("Z", "+00:00")).timestamp())
except ValueError:
return 0

Comment thread .env.example
K1_PGP_KEY_VAULT_PATH=secret/k1/auth/pgp/kaisonai

# Optional hunter-account import source and inventory index.
KAI_HUNTER_ACCOUNTS_CSV=/home/k1-admin/Documents/Proton Shit/Proton Pass_export_2026-06-11_1781164711.csv

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The example path contains unprofessional language (Proton Shit) and a highly specific local home directory (/home/k1-admin). Example environment files should use generic, clean placeholder paths to maintain professionalism and ease of configuration for other developers.

KAI_HUNTER_ACCOUNTS_CSV=/srv/kai/artifacts/hunter-accounts/proton_pass_export.csv

Comment on lines +136 to +137
logger.info("Database migrations applied successfully: %s", ", ".join(refreshed_heads))
from types import SimpleNamespace

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

There is a duplicate and misplaced import of SimpleNamespace at the very end of the file. Since it is already imported on line 6, this line should be removed.

Suggested change
logger.info("Database migrations applied successfully: %s", ", ".join(refreshed_heads))
from types import SimpleNamespace
logger.info("Database migrations applied successfully: %s", ", ".join(refreshed_heads))

matching_accounts.append(display_name)
continue

if source_url and any(domain and domain.lower() in source_url for domain in opportunity.scope_domains):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

If opportunity.scope_domains is None or missing, iterating over it will raise a TypeError or AttributeError. Use getattr with a default empty list to ensure defensive programming and avoid runtime crashes.

Suggested change
if source_url and any(domain and domain.lower() in source_url for domain in opportunity.scope_domains):
if source_url and any(domain and domain.lower() in source_url for domain in getattr(opportunity, "scope_domains", []) or []):

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

Copy link
Copy Markdown

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: f397accf8e

ℹ️ 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".

return suggestions[:limit]


@router.get("/hunter-accounts", response_model=HunterAccountInventoryResponse)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Register fixed credential routes before UUID catch-all

With the router still declaring GET /{program_id} above this, FastAPI/Starlette will try to satisfy GET /api/v1/credentials/hunter-accounts (and the similarly added /scan-suggestions) with the UUID program_id route before these handlers, so the new frontend calls get a 422 instead of the inventory/suggestion payload. Move these fixed paths above the single-segment dynamic route or otherwise disambiguate the catch-all.

Useful? React with 👍 / 👎.

const subjectKey = program?.handle ?? program?.program_key ?? program?.name ?? item.opportunity_id;
return {
id: item.opportunity_id,
programId: item.opportunity_id,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Resolve catalog suggestions to DB program IDs

When a scan suggestion is not already matched to a ranking row, this fallback copies the catalog opportunity id (for example opportunity_catalog ids such as hackerone:shopify) into programId. handleLaunchQueued sends that as program_id to /api/v1/scan-pool/queue-batch, whose schema expects a Program UUID and rejects non-UUIDs, so queueing account-ready suggestions from the new backend API fails with Invalid program_id UUID. Resolve/import the Program row first or dispatch through the catalog opportunity scan endpoint.

Useful? React with 👍 / 👎.

Comment on lines +123 to +127
handleAddVisibleToQueue();
await new Promise<void>((resolve) => {
requestAnimationFrame(() => resolve());
});
await handleLaunchQueued();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Launch from the queue after applying additions

In the Add visible + launch click path, addItem schedules React state updates, but this async function continues to call the old handleLaunchQueued closure, which still reads the pre-click items array. If the queue was empty, the just-added suggestions are not included and the user sees No queued items to launch; if it was non-empty, only the old queue is dispatched. Build the batch from the rows being added or from a synchronous next-queue value.

Useful? React with 👍 / 👎.

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