Repository navigation
feat: attest legacy AWS scope for retained reconciliation - #2861
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d28ada9997
ℹ️ 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".
|
|
||
| function attestedAWSLegacyScope(lease: LeaseRecord, account?: string): string | undefined { | ||
| if ( | ||
| lease.provider === "aws" && | ||
| lease.providerScope === undefined && |
There was a problem hiding this comment.
This adds AWS-specific account formatting, region and instance-ID eligibility, and rollout-cutover policy directly to coordinator reconciliation. The repository requires provider-specific reconciliation, resource identity, and rollout compatibility to live behind provider adapters, so this logic should be exposed through an AWS adapter capability while core consumes a provider-neutral result.
AGENTS.md reference: AGENTS.md:L24-L28
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
P2
|
Codex review: needs real behavior proof before merge. What this changesThis PR adds optional operator attestation of the original AWS account so eligible legacy lease records can complete retained reconciliation without resource deletion. Example: A released AWS lease created on 2026-09-12T23:59:59Z has no providerScope, and the operator attests account 123456789012.
Review scores
ProductKind: Feature · Worth it: Yes · Fix scope: Complete Merge readiness⛔ Blocked before merge - 5 items remain Keep open: current main lacks this maintainer-directed capability, but the new AWS policy violates the repository’s provider boundary. Priority: P2 Before merge
Findings
Tests
Agent review detailsHow this fits togetherCoordinator maintenance reads retained terminal leases from the current index, verifies AWS resource absence, and transactionally publishes cleanup facts and index changes. flowchart TD
A[Coordinator maintenance] --> B[Current lease index]
B --> C[Legacy eligibility and attestation]
C --> D[AWS identity and exact resource reads]
D --> E[Two absence observations]
E --> F[Transactional lease publication]
F --> G[Cleanup facts and current index]
Technical reviewBest possible solution: Support narrowly scoped audit-only legacy attestation through an AWS-owned capability while preserving recorded authority and deletion fences. Do we have a high-confidence way to reproduce the issue? Current-main source deterministically classifies an otherwise eligible AWS lease without providerScope as missing_original_scope; this is an intentional safety restriction that the feature narrowly relaxes. Is this the best way to solve the issue? The audit-only approach addresses records excluded by existing CloudTrail recovery, but AWS eligibility and cutover policy should live behind the provider adapter. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 05eded31b4d8. Merge-risk optionsMaintainer options:
Provenance checked
TestingProof path: unit tests only. SecurityNeeds attention: No reachable unauthorized deletion was found, but the new persisted authority path needs final-effect qualification. EvidenceSecurity concerns:
What I checked:
LabelsLabel changes:
Label justifications:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 16, 2026, 1:01 AM ET / 05:01 UTC. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a1f4d797b
ℹ️ 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".
| lease.region && | ||
| sanitizeAWSRegion(lease.region) === lease.region && | ||
| /^i-[a-f0-9]{8,17}$/.test(lease.cloudID) && | ||
| Date.parse(lease.createdAt) < awsLegacyScopeCutoverMs |
There was a problem hiding this comment.
When a stored legacy record has a malformed createdAt such as "0", "1", or "2026-09-12junk", JavaScript's permissive Date.parse still returns a pre-cutover date, so the record becomes eligible for attestation without valid evidence that it predates the scope-persistence rollout. This can incorrectly publish cleanup and persist the attested account for malformed records; validate the expected timestamp format before comparing it with the cutoff.
Useful? React with 👍 / 👎.
c68d993 to
122f9e0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 122f9e0ff3
ℹ️ 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".
| (lease.providerScope ?? | ||
| (!retainedUnresolvedReason(lease, legacyAccount) | ||
| ? attestedAWSLegacyScope(lease, legacyAccount) | ||
| : undefined)) !== `aws:account:${identity.account}` || |
There was a problem hiding this comment.
When the configured legacy account differs from the STS identity, this classifies every eligible legacy record as scope_mismatch; maintainRetainedLeases then increments the shared state.backoff.aws for each record, and subsequent passes skip observations for every AWS scope while that backoff is active. A typo in this optional rollout setting can therefore repeatedly suspend reconciliation of correctly scoped AWS records for up to the one-hour cap, so legacy-attestation mismatches should not activate the provider-wide backoff.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
1 participant
Legacy terminal AWS leases created before scope persistence could never leave the retained index because their original account was missing. Add an explicit operator attestation for AWS records created strictly before 2026-09-13T00:00:00Z, preserving every existing ownership and cleanup check.
The optional
CRABBOX_RETAINED_RECONCILE_AWS_LEGACY_ACCOUNTruntime setting accepts exactly 12 digits. It never overrides a present scope. STS must match the attested account; exact-ID and companion reads, credential pinning, and two complete observations at least ten minutes apart still gate completion. The attested scope is written only with cleanup completion throughputLeaseRecord, withcleanupConfirmedBy: retained-reconcile-legacy-attested. Attested leases cannot enter the deletion path even when deletion is enabled.Admin status exposes the
legacy_scope_attestedcounter and a configured boolean without the account value. Documentation covers the cutover and the risk of an incorrect attestation. No account value, credential, deployment, or runtime configuration change is included.The Go admin CLI preserves the configured boolean and attestation counter in both JSON and human-readable output.
Validation on Node 24:
go test ./internal/cli -run '^TestAdminRetained' -count=1.