Files
zeekayandHanzo Dev b497511c04 no hogs: stamphog and review_hog are stamp and review
Both are Django apps, and both pin `label` in apps.py — "stamp" and "review".
django_migrations keys a row on (app_label, name), and app_label comes from that
pin and not from the import path, so moving the directory does not orphan a
single migration. All six stay where they are, untouched.

  products/stamphog -> products/stamp     name = products.stamp.backend
  products/review_hog -> products/review  name = products.review.backend
  StamphogScene/ -> StampScene/, and stamphogSceneLogic.ts with it

189 files follow. An earlier attempt at this move failed twice for reasons worth
recording: `git mv A B` where B exists puts A INSIDE B, and a stray
products/stamp/node_modules from an abandoned run was enough to make B exist —
the app ended up nested two deep. And `git ls-files` does not list a file moved
with plain mv and not yet staged, so a pass over "every tracked file" silently
skips it; that is how StampScene.tsx kept importing ./stampSceneLogic after the
file beside it had been renamed back.

bin/check-imports-are-committed: all first-party imports across 15,843 files
resolve.

Co-authored-by: Hanzo Dev <[email protected]>
2026-08-09 10:18:18 -07:00

12 KiB

stamp — invariants for agents

Read README.md for the product shape first. This file is the contract: the invariants below were each earned through a real review finding — do not relax one without understanding what it closes, and hold new code to all of them.

The stale-approval invariant (the big one)

No stamp approval may remain standing over commits it didn't review. GitHub never auto-dismisses approvals, so every path that skips, supersedes, or abandons a review after a head-changing event must retract standing approvals itself:

  • The workflow runs dismiss_stale_approvals FIRST, and it voids EVERY standing approval — same-head included, since a re-review means fresh judgment is pending (fail-closed: if any later step crashes, the prior approval is already gone). The sweep's same-head exclusion is only for skip paths, where no new verdict is coming. A base retarget gets its own explicit all-heads retraction — it changes the diff without moving the head.
  • Every Celery skip path that never reaches the workflow (trigger-label absent, author below write permission, untrusted association, disabled repo) retracts via _retract_stale_approvals_on_skip.
  • post_verdict persists posted_review_id the moment GitHub accepts the review, and any path that abandons the run after posting (lost terminal save, superseded/head-moved guards on retry) dismisses its own orphan (_dismiss_orphaned_approval). Before posting it also ADOPTS-then-persists: if this App already has an active APPROVE pinned to exactly run.head_sha on GitHub (a prior attempt that posted, then crashed before the immediate persist), it takes that review's id instead of stacking a second approval the DB-keyed sweep could never see.
  • The sweep in logic/approvals.py keys off posted_review_id alone — filtering on a saved verdict would miss crashed runs' orphans.
  • dismiss_stale_approvals also runs a GitHub-side belt-and-braces sweep after the DB sweep: it lists this App's still-active APPROVED reviews and dismisses every one regardless of head, catching an orphan that has no ReviewRun row at all (the DB sweep is blind to it). Both write-adjacent paths (this sweep and post_verdict's adopt) identify "ours" via the exact <slug>[bot] login and do NOTHING when STAMPFN_GITHUB_APP_SLUG is unset — a fuzzy "any Bot" match must never dismiss or adopt another bot's review.
  • A run that reaches a non-approve terminal (post_verdict's gated/refused/escalate/wait branch, or mark_review_failed) re-runs the GitHub-side own-approvals sweep at its own end (_sweep_orphan_approvals_at_terminal). This closes the supersession race: an older run can clear post_verdict's final guards, get superseded, then land its GitHub approval AFTER the newer run's STARTUP sweep — and if the newer run refuses (or fails), nothing lists our approvals again, so the orphan stands over a refusing verdict. The keep-set excludes every OTHER live run's persisted posted_review_id (writer-pinned read), so a slow run at terminal never dismisses a newer run's legit approval. UNLIKE the fail-closed startup sweep, the terminal sweep is fail-open: a GitHub error must not block the terminal save (the integrity gap on error is the pre-existing exposure, no worse).

Supersession and terminal states

A newer relevant delivery supersedes older non-terminal runs. Rules that keep this sound:

  • Terminal states (TERMINAL_STATUSES in facade/enums.py) are never rewritten — mark_review_failed must not clobber a delivered outcome, and terminal saves are conditional (.exclude(status=SUPERSEDED).update(...)), never plain save().
  • post_verdict guards before ANY GitHub write: superseded status, current head vs run head, and a last fresh status read. Losing the final conditional update means dismiss-your-own-approval, not "log and return".
  • Out-of-order webhook deliveries are dropped by the payload_updated_at clock — checked before the transaction AND re-checked under the row lock, and the descriptive-field refresh is gated on the same clock inside the UPDATE's WHERE clause.

Reader-lag: pin decision reads to the writer

The product DB has replicas. Any read that gates a side effect (GitHub write, Slack post, supersession, run creation) must be pinned via .using(router.db_for_write(Model)) — a lagged reader silently skips retractions, resurrects disabled channels, or strands queued runs. If you add a read-then-act path, pin it; this class of bug has been found on five separate paths.

Sandbox credentials and egress

  • The sandbox holds NO long-lived secret. _mint_reviewer_gateway_token mints a per-run OAuth token under the repo's connecting user with exactly ["llm_gateway:read", "internal_run:read"] and include_internal_scopes=False. Never switch to include_internal_scopes=True — that drags task:write into a sandbox running an LLM over untrusted PR content. The internal_run:read marker is what satisfies the gateway route's requires_server_credential.
  • The raw-Anthropic fallback is for the Action runtime only; hosted runs fail closed without a gateway. No ANTHROPIC_API_KEY may enter the sandbox environment.
  • Egress is an explicit domain allowlist (_sandbox_egress_allowlist). Additions go through STAMPFN_SANDBOX_EXTRA_EGRESS_DOMAINS, not code edits.
  • Everything posted to GitHub goes through _scrub_credentials AND _neutralize_active_markdown (GitHub's camo proxy auto-fetches images — a markdown image URL is an exfiltration channel).

The self-driving inbox carve-out (the one exception to the bot-author refusal)

Bot-authored PRs are refused at every layer — the webhook pre-filter (_review_skip_reason), the engine (review_pr.py::_refuse_bot_author, mirrored by review_local.py), and the Action's job gates — with ONE deliberate exception: a PR positively linked to a self-driving Inbox implementation run (a signal-report-carrying TaskRun at ai_stage="implementation", matched through the tasks facade), one of whose assigned reviewers opted in via Review's per-user stamp_review_inbox_prs toggle. Rules that keep the exception narrow:

  • Identification is server-attested task linkage plus server-attested PR identity — both required, neither trusted alone. The task link is unforgeable by construction: signals' auto_start generates the head branch name (insights-self-driving/<slug>-<hex>) before the agent runs and stamps it into PATCH-protected run state (state.self_driving_head_branch), and find_signal_implementation_run matches the PR's GitHub-attested head ref against that stamp — never against the API-writable output.pr_url / output.head_branch. The identity half, _is_self_driving_pr, requires two facts only GitHub attests — the PR is authored by this instance's Insights GitHub App machine user (<GITHUB_APP_SLUG>[bot]) on a repo-native head (never a fork) — enforced on both the receiver leg (process_inbox_pr_review) and the webhook leg (_inbox_rereview_carve_out), failing closed when the App slug is unconfigured. The fork exclusion is load-bearing for the linkage too: a head ref is only GitHub's word when the head lives in the base repo. This is a positive App-identity match, not the general "any bot" rule: github.py::is_bot_author must not be weakened, and dependabot / renovate / insights-bot / any foreign App fail it.
  • What a malicious teammate can still do, and what they can't. Rewriting output.pr_url, TaskRun.branch, Task.repository, or suggested_reviewers no longer aims the carve-out at a different PR: matching reads none of them, and the stamped branch plus ai_stage and signal_report_id are not PATCHable. What remains team-internal and accepted: an opted-in assigned reviewer's existence is still the gate (see the reviewer-authorization discussion on PR #72680), and a teammate who can edit the report before auto-start still influences what the implementation agent builds — that is the product working as designed, reviewed as any PR.
  • The engine flag (self_driving_review in the hosted context JSON → Pipeline(self_driving=...)) defaults closed and the Action never sets it, so Action behavior is unchanged by construction. It relaxes exactly two gates — the bot-author refusal and the draft prerequisite (the verdict must exist at Inbox triage time, while the PR is still a draft) — and swaps human-author trust context for a TRUSTED provenance block in the prompt. The hosted server sets the flag exclusively from the run's persisted inbox provenance (ReviewRun.output["inbox_review"]), which only the two linkage-verified trigger paths stamp.
  • The initial review is the receiver leg (process_inbox_pr_review, entered via the queue_inbox_pr_review facade after review checked the toggles); the webhook leg re-reviews only on synchronize / reopen / base retarget, re-checking them through the facade/inbox_hooks.py resolver (registered by review at app-ready — a direct import of review would be a dependency cycle). No registered resolver means fail-closed: no re-review. The gate is any assigned reviewer's opt-in, and both legs must resolve it identically: if one fired while the other saw "opted out", a push would retract a standing approval with somebody still opted in.
  • Dismissal is never preference-gated. Every toggle switched off mid-PR stops new runs, but the skip paths' head-changing retraction still voids the standing approval (the stale-approval invariant above is untouched), and every carved-out run still enters the workflow through dismiss_stale_approvals first.
  • These runs bypass the review-mode and author-write-permission gates: the reviewers' toggle is the gate, and the App's machine user is not a collaborator (the permission lookup would always deny). review_mode keeps governing human PRs only.

Trust boundaries

  • Review policy is read from the repo's default branch, never the PR head — a PR must not be able to rewrite the policy that gates it. Same for the digest: channel declaration.
  • A manually-created repo config (blank installation_id) binds disabled when a sync adopts it: its flags were set by someone who never proved GitHub access. Reinstall rebinds keep settings — those were configured under a verified binding.
  • Name-matched Slack digest channels provision disabled pending a human enable (a workspace member can squat a channel named like a team slug). Only repo-declared channels auto-enable.
  • PR content — title, body, diff, comments, reactions — is untrusted input everywhere, including in reviewer prompts and error messages persisted to API-readable fields (run.error keeps only a truncated first line for exactly this reason).

Engine parity (tools/pr-approval-agent)

review_local.py (hosted) must mirror review_pr.py (Action) semantics wherever both apply: gate order, review filtering (bare COMMENTED reviews dropped, non-empty ones kept), in-flight bot-reviewer WAIT behavior (TRUSTED_REACTOR_BOTS mirrored in temporal/constants.py), and ownership summaries (individual owners count, not just teams). When you change one runtime, check the other; divergence here has produced real approve-when-should-wait findings.

Temporal specifics

  • Registering a new activity requires adding it to temporal/registry.py — the registry-completeness test guards this, don't bypass it.
  • Workflow bodies follow the repo-wide determinism rules (workflow.patched() for new commands).
  • Activity payloads stay small; large context rides in run.output, not through the workflow.

Tests

insightscli test products/stamp/backend/tests/ (Django; --reuse-db after the first run) plus tools/pr-approval-agent/ tests for engine changes. The integration tests drive the full chain through fakes (tests/fakes.py, tests/conftest.py) — extend the fakes rather than mocking internals, and prefer adding a parameterized case to an existing test over a new function.