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]>
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_approvalsFIRST, 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_verdictpersistsposted_review_idthe 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 exactlyrun.head_shaon 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.pykeys offposted_review_idalone — filtering on a saved verdict would miss crashed runs' orphans. dismiss_stale_approvalsalso 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 noReviewRunrow at all (the DB sweep is blind to it). Both write-adjacent paths (this sweep andpost_verdict's adopt) identify "ours" via the exact<slug>[bot]login and do NOTHING whenSTAMPFN_GITHUB_APP_SLUGis 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, ormark_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 clearpost_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 persistedposted_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_STATUSESinfacade/enums.py) are never rewritten —mark_review_failedmust not clobber a delivered outcome, and terminal saves are conditional (.exclude(status=SUPERSEDED).update(...)), never plainsave(). post_verdictguards 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_atclock — 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_tokenmints a per-run OAuth token under the repo's connecting user with exactly["llm_gateway:read", "internal_run:read"]andinclude_internal_scopes=False. Never switch toinclude_internal_scopes=True— that dragstask:writeinto a sandbox running an LLM over untrusted PR content. Theinternal_run:readmarker is what satisfies the gateway route'srequires_server_credential. - The raw-Anthropic fallback is for the Action runtime only; hosted runs fail closed without a
gateway. No
ANTHROPIC_API_KEYmay enter the sandbox environment. - Egress is an explicit domain allowlist (
_sandbox_egress_allowlist). Additions go throughSTAMPFN_SANDBOX_EXTRA_EGRESS_DOMAINS, not code edits. - Everything posted to GitHub goes through
_scrub_credentialsAND_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), andfind_signal_implementation_runmatches the PR's GitHub-attested head ref against that stamp — never against the API-writableoutput.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_authormust 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, orsuggested_reviewersno longer aims the carve-out at a different PR: matching reads none of them, and the stamped branch plusai_stageandsignal_report_idare 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_reviewin 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 thequeue_inbox_pr_reviewfacade after review checked the toggles); the webhook leg re-reviews only on synchronize / reopen / base retarget, re-checking them through thefacade/inbox_hooks.pyresolver (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_approvalsfirst. - 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_modekeeps 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.errorkeeps 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.