feat(cli): bgagent linear remove-workspace + DELETE route + fail-closed resolver (#306) - #681
feat(cli): bgagent linear remove-workspace + DELETE route + fail-closed resolver (#306)#681scottschreckengaust wants to merge 5 commits into
Conversation
… gaps Review follow-up (self-review agents on PR #681): - Secret-delete failure is no longer masked: a non-idempotent SM error (e.g. AccessDenied) after the row is revoked now returns a distinct 500 SECRET_DELETE_FAILED and writes a durable secret_deletion_failed / orphaned_oauth_secret_arn marker on the registry row, so the leaked credential is discoverable (a naive retry 404s at the active-scan and would never re-attempt the delete). ResourceNotFoundException stays idempotent (200). - Top-level catch + mapping-cleanup loop now log the failing phase + workspace id / per-page progress so on-call can locate an orphaned secret or half-cleaned mapping table from the request id. - Tests: SECRET_DELETE_FAILED path + marker write; FilterExpression pins the status='active' fail-closed filter to the handler (not the mock); paginated mapping cleanup across LastEvaluatedKey; missing oauth_secret_arn skip; CLI confirmation-prompt abort/proceed; secret_deleted:false CLI output; api-client query-string mapping (purge / keep_mappings snake_case). Relates to #306 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
✅ Acceptance summary (for the reviewer)#306 —
🔀 Merge guidance (for the reviewer)Cluster 🤖 orchestrator note (agent) — promotion is orchestrator-driven; merge remains a human action. |
isadeks
left a comment
isadeks
left a comment
There was a problem hiding this comment.
Verdict — Request changes
The shape of this change is right and the security instincts are good (delegate writes to the API role, revoke-first fail-closed, admin-only, no secrets logged, idempotent secret delete). But the slug lookup that gates the entire endpoint is built on a DynamoDB Scan mis-use that makes the command 404 on live, active workspaces as soon as the registry holds more than one row — which is the normal steady state on this platform, and becomes guaranteed after the first soft-removal. The handler unit tests can't see it because the test double models Limit incorrectly.
Prior review state: none. The only existing comment is the author's own acceptance summary; there are no prior blocking claims to carry forward or retire.
Governance: backing issue #306 is approved — gate satisfied (ADR-003). Branch feat/issue-306-linear-remove-workspace matches the convention.
Vision alignment — passes
This is squarely on-tenet. Onboarding was one command and removal was undocumented DDB/Secrets-Manager surgery; closing that asymmetry is bounded blast radius work. Routing the destructive path through an authenticated endpoint so DDB + Secrets Manager grants stay on the Lambda role rather than on every CLI user's IAM identity is the correct call and matches the existing link precedent. Default soft-revoke preserving an audit row keeps outcomes inspectable. No tenet is traded, so no ADR is required.
Blocking issues
B1 — Limit: 1 on a filtered Scan 404s live workspaces (critical correctness)
cdk/src/handlers/linear-remove-workspace.ts:95-106
const scan = await ddb.send(new ScanCommand({
TableName: WORKSPACE_REGISTRY_TABLE,
FilterExpression: 'workspace_slug = :slug AND #status = :active',
...
Limit: 1,
}));
const row = scan.Items?.[0];
if (!row) { return errorResponse(404, ...); }In DynamoDB, Limit bounds the number of items evaluated, and FilterExpression is applied after that evaluation. Per the API reference: "Limit — the maximum number of items to evaluate (not necessarily the number of matching items)." So Limit: 1 examines exactly one arbitrary item, applies the filter to that one item, and returns Items: [] with a LastEvaluatedKey whenever the examined item isn't the target. The handler never follows LastEvaluatedKey.
Failure scenario: registry holds ws-other (slug other, active) and ws-acme (slug acme, active) — the documented shared-stack-per-org model, where one stack hosts multiple workspaces via add-workspace. DELETE /v1/linear/workspaces/acme evaluates ws-other, the filter rejects it, Items is empty, and the admin gets 404 Workspace 'acme' is not an active registration. for a workspace that is active and that they own. Retry is not a workaround — Scan ordering is stable for an unchanged table, so it 404s deterministically.
This is self-aggravating: the default soft-removal leaves status='revoked' rows in the table forever, so every successful removal adds another non-matching row and raises the probability that the next removal misses. Even a single-workspace deployment starts failing after its first revoke-then-re-add cycle.
It also silently defeats B1-adjacent intent: a duplicate slug across two workspaces can never be detected.
Fix — drop Limit and paginate to completion, matching the convention already established in this repo for exactly this table shape (cdk/src/handlers/jira-webhook-processor.ts:131-167 and cdk/src/handlers/shared/linear-issue-lookup.ts:100-115 both scan the registry without Limit and document the small-table assumption):
let row: Record<string, unknown> | undefined;
let lastKey: Record<string, unknown> | undefined;
do {
const page = await ddb.send(new ScanCommand({
TableName: WORKSPACE_REGISTRY_TABLE,
FilterExpression: 'workspace_slug = :slug AND #status = :active',
ExpressionAttributeNames: { '#status': 'status' },
ExpressionAttributeValues: { ':slug': slug, ':active': 'active' },
ExclusiveStartKey: lastKey,
}));
row = page.Items?.[0];
lastKey = page.LastEvaluatedKey as Record<string, unknown> | undefined;
} while (!row && lastKey);Please add a regression test that seeds two active rows with the target second (see N1 — the current double cannot express this).
B2 — mapping cleanup is a provable no-op, but is reported to the operator as success
cdk/src/handlers/linear-remove-workspace.ts:287-307, surfaced at cli/src/commands/linear.ts:1301
deleteWorkspaceProjectMappings filters on linear_workspace_id. I traced every writer of LinearProjectMappingTable: the only one is cli/src/commands/linear.ts:1484-1493 (onboard-project), which writes linear_project_id, repo, label_filter, optional team_id, status, onboarded_at, updated_at — no workspace identifier of any kind. The webhook processor only reads that table (linear-webhook-processor.ts:163-164). So the filter matches zero rows on every real deployment, today and for all pre-existing rows.
I appreciate that you disclosed this in the PR body and the guide. The problem is what ships alongside it:
- The operator is told it worked. The CLI unconditionally prints
✓ 0 project mapping(s) removed— a checkmark and a count, which reads as "cleanup ran, the table was clean." It is indistinguishable from a genuinely clean teardown, so the operator will not go do the manual cleanup the guide tells them about. That is a silent failure dressed as success. - Issue #306's acceptance criterion is not met. The AC asks for deletion of
LinearProjectMappingTablerows for the workspace; no such row is reachable. - It carries real cost for zero effect. Every default removal runs a full-table paginated
Scanof the mapping table, and it is this dead path that justifies both theprojectMappingTable.grantReadWriteDatagrant (linear-integration.ts:330) and the 10x timeout bump.
Cheapest honest fix: drop the mapping-cleanup path (and its grant, its --keep-mappings flag, and the timeout bump) from this PR, and file the schema follow-up. That leaves a smaller, fully-working command. If you prefer to keep the code in place for the follow-up, then at minimum make the output truthful — do not emit ✓ with a count when the match is structurally impossible; say explicitly that project mappings could not be attributed and must be removed by project id.
B3 — --purge + secret-delete failure leaks an OAuth credential with no durable record, and two comments describe a recovery path that doesn't exist
cdk/src/handlers/linear-remove-workspace.ts:255-266
markSecretDeletionFailed early-returns when purged is true. So on the --purge path: the registry row is deleted, the Secrets Manager delete then fails with e.g. AccessDeniedException, and the live OAuth secret is left in the account with no durable marker anywhere — the only trace is a CloudWatch log line. That is the exact leaked-credential condition the follow-up commit set out to make discoverable, still fully open on the flag most likely to be used for a hard teardown.
The doc comment at :249-251 states the marker works "even after --purge fails at the secret step, because the delete happens after the row write." That is backwards: with --purge the row write is a delete, which is precisely why the function skips. The comment asserts the opposite of the code directly beneath it.
Relatedly, :283-285 says recovery is "--keep-mappings + manual cleanup." Retrying with --keep-mappings still hits the status='active' scan, still misses the now-revoked row, and still 404s. The documented recovery path cannot work, on any flag combination. After any partial teardown this endpoint has no re-attempt path at all.
Fix — make --purge revoke-then-delete-row so both fail-closed and the marker hold:
Update(status='revoked') // fail-closed immediately, row still present
DeleteSecret // on failure: marker write now succeeds, row survives
Delete(row) // only once the secret is confirmed gone (purge only)
And please correct both comments to describe what the code actually does. A --force escape hatch that accepts a non-active row for re-attempt would close the retry dead-end, but a follow-up issue is fine for that.
Non-blocking suggestions / nits
N1 — the handler test double models Limit incorrectly, which is why B1 is invisible. cdk/test/handlers/linear-remove-workspace.test.ts:87-104: routeDdb returns the seeded row for any registry Scan, ignoring Limit and ExclusiveStartKey entirely. Ten tests pass against behavior real DynamoDB does not have (AI001 — integration verified only against a self-written mock). Worth noting that the test at :274 does correctly model LastEvaluatedKey for the mapping scan, so the fixture already knows how; the registry router just doesn't use it. Teach the router to slice by Limit and honor ExclusiveStartKey and B1 fails immediately.
N2 — two construct assertions are vacuous. cdk/test/constructs/linear-integration.test.ts:85-94 asserts some Lambda has both LINEAR_WORKSPACE_REGISTRY_TABLE_NAME and LINEAR_PROJECT_MAPPING_TABLE_NAME — but webhookProcessorFn already carries both (linear-integration.ts:208-212), so this passes with the new function deleted. :96-107 asserts some IAM policy allows secretsmanager:DeleteSecret without pinning it to the remove-workspace role. Pin both by logical id, or match the bgagent-linear-oauth-* resource pattern so the grant scope is actually under test. The DELETE-method test at :78 likewise doesn't pin the path.
N3 — the timeout comment is still inaccurate. cdk/src/constructs/linear-integration.ts:44-47 says 30s "vs. the 3s Lambda default the link/webhook request handlers use." Those handlers don't use the default — webhookFn and linkFn are both explicitly Duration.seconds(10) (:264, :303). The honest comparison is 30s vs. their explicit 10s. Flagging because the third commit (docs(#306): clarify remove-workspace Lambda timeout rationale) existed specifically to fix this sentence. Note that if B2 is resolved by dropping mapping cleanup, the entire 30s rationale goes away and 10s matches its siblings.
N4 — document the registry-Scan scale assumption. Once Limit is removed (B1), each removal scans the whole registry. That's correct and cheap at expected scale, but jira-webhook-processor.ts:132-136 sets the house style of saying so out loud ("expected to stay small (tens of rows) ... if this table ever grows large, add a GSI on status and Query it"). A GSI on workspace_slug would turn this into a Query and is the right long-term shape given the same slug→row lookup already recurs across linear-issue-lookup.ts.
N5 — deferred, agreed. BatchWriteCommand for mapping deletes and treating SM InvalidRequestException ("already scheduled for deletion") as idempotent are both reasonable follow-ups, not needed here.
Documentation
docs/guides/LINEAR_SETUP_GUIDE.mdrestructure is a genuine improvement: leads with the command, keeps the manual path as an explicit fallback, and the### Deactivating a single project mapping/### Manual fallbacksplit is clearer than what it replaced. The mapping caveat is called out honestly.- Starlight mirror verified in sync — I diffed the changed section of
docs/src/content/docs/using/Linear-setup-guide.mdagainst the source; identical. No hand-editing of the generated tree. CI's mutation guard will be clean. - One doc correction needed if B2 is fixed by dropping the path, and the caveat note should say cleanup matches nothing currently written rather than implying only legacy rows are affected.
Tests & CI
All checks green. Coverage is broad in shape — 401/400/404/403, purge vs. revoke, idempotent secret-gone, SECRET_DELETE_FAILED + marker, paginated mapping cleanup, missing oauth_secret_arn, plus the CLI prompt abort/proceed branch and api-client query-string mapping. The adversarial resolver test is the best thing in the diff: asserting smSend was never called pins the fail-closed property to short-circuit-before-read rather than to the return value, with an active control case to prove the test isn't vacuous. More of that, please.
The gap is depth, not breadth: the two highest-risk behaviors — multi-row registry lookup (B1) and --purge partial teardown (B3) — are the two the suite doesn't reach.
Bootstrap synth-coverage: not applicable, verified. No new CloudFormation resource types are introduced — AWS::Lambda::Function, AWS::ApiGateway::Resource/Method, and AWS::IAM::Policy are all already emitted by this construct. secretsmanager:DeleteSecret is a runtime grant on the function role, not a CFN-execution-role action, and cdk/src/bootstrap/policies/application.ts:241 already carries it regardless. No BOOTSTRAP_VERSION bump or artifact regeneration is owed. Adding LinearRemoveWorkspaceResponse to CLI_ONLY_ALLOWLIST is correct and consistent with the sibling LinearLinkResponse — it's a client-side envelope and the handler builds the body inline.
No CDK synth-performance concern: the construct suite keeps its single new App() + Template.fromStack() in beforeAll and does not re-enable bundling (#366 respected).
Review agents run
Ran, as rubrics applied over the full diff:
- code-reviewer — style/guidelines; found N2, N3.
- silent-failure-hunter — the highest-yield pass here; found B2 (success-shaped report over a structurally impossible match) and B3 (leaked credential with no durable record + a catch-adjacent recovery path that doesn't exist). Confirmed the
ResourceNotFoundException-only narrowing is correctly specific and that the non-idempotent branch genuinely rethrows rather than swallowing. - pr-test-analyzer — found N1 and the B1/B3 depth gaps; confirmed the resolver test is non-vacuous.
- comment-analyzer — found the
:249-251comment inverting its own code and the:283-285unreachable recovery claim (both folded into B3), plus N3. - type-design-analyzer —
LinearRemoveWorkspaceResponseis well-formed:readonlythroughout,statusa closed'revoked' | 'purged'union rather thanstring, andsecret_deleted/mappings_removedmake the partial-outcome states representable. No findings. - security-review — IAM, secrets, input validation. The
bgagent-linear-oauth-*prefix grant is correctly justified (name unknowable at synth) and matches the existing webhook-Lambda pattern; the nag suppression reason is specific and honest, not boilerplate.DeleteSecretis granted alone — noGetSecretValue, so this role cannot read tokens. Slug regex is anchored and applied before any AWS call. Logs carry slug/workspace-id/booleans only. 404-collapse to avoid a revoke-oracle is a nice touch. Its one finding is B3. - code-simplifier — omitted; B2's resolution is itself the simplification (delete the dead path), so a separate pass would be redundant.
Human heuristics
- Proportionality — pass. 316-line handler for a three-step teardown is honest, most of it comment and error handling. Flat file placement matches the existing
linear-*.tssiblings; no new abstraction invented for a one-off. The one disproportion is B2: a paginated scan + write grant + timeout bump sustaining a path that cannot match a row. - Coherence — concern. Terminology splits: the registry keys on
linear_workspace_id, the CLI argument and route parameter areslug, and the row field isworkspace_slug— reasonable given Linear's ownurlKey, but the mapping table now gets a fourth name for the same concept with no writer (B2). Otherwise the parallel structure withadd-workspace/update-webhook-secretis real: sharedSLUG_RE, sharedlinearOauthSecretName, sharedpromptLine, same delegate-to-API stance aslink. Reuse, not copy-paste. - Clarity — concern. Names are good and the comments explain why at an unusually high standard. But three of them are wrong in the same region (N3, and the two in B3), and comments this confident are load-bearing — a future maintainer will trust
:249-251and conclude the--purgeorphan is recorded when it isn't.✓ 0 project mapping(s) removed(B2) is the clarity failure with operational teeth. - Appropriateness — concern. This is AI001/AI005: DynamoDB
Limit-with-FilterExpressionis exactly the integration semantic a self-written double will get wrong, and here it did, so ten green tests assert what the mock does rather than what DynamoDB does (B1, N1). The fix is small and the surrounding structure is maintainable; the suite just needs to model the seam faithfully. Worth noting the correct pagination model already exists in this very file's mapping-scan test — applying it to the registry router is the whole change.
To be clear about where this lands: B1 is a mechanical fix, B3 is a small reordering plus two comment corrections, and B2 is most cheaply resolved by deleting code. The security posture, the delegation design, and the adversarial resolver test are all things I'd like to see more of. Happy to re-review promptly.
On merge sequencing — noted that this shares cli/src/commands/linear.ts, cdk/src/constructs/linear-integration.ts, and linear-oauth-resolver.ts with #345 (cluster ua-broad), and edits here are additive/localized. Rebase after #345 should stay mechanical.
| FilterExpression: 'workspace_slug = :slug AND #status = :active', | ||
| ExpressionAttributeNames: { '#status': 'status' }, | ||
| ExpressionAttributeValues: { ':slug': slug, ':active': 'active' }, | ||
| Limit: 1, |
There was a problem hiding this comment.
B1 (blocking, critical): Limit: 1 on a filtered Scan makes this 404 live, active workspaces.
DynamoDB applies FilterExpression after evaluating items, and Limit bounds items evaluated, not items matched — "Limit: the maximum number of items to evaluate (not necessarily the number of matching items)." So this examines exactly one arbitrary row, filters it out if it isn't the target, and returns Items: [] with a LastEvaluatedKey that is never followed. scan.Items?.[0] is then undefined → 404.
Concretely: registry holds ws-other (slug other, active) and ws-acme (slug acme, active) — the normal shared-stack-per-org state, since add-workspace registers additional workspaces on one stack. DELETE /v1/linear/workspaces/acme by acme's own admin returns 404 Workspace 'acme' is not an active registration. Deterministically, not intermittently — Scan order is stable for an unchanged table, so retrying does not help.
This is self-aggravating: the default soft-removal leaves status='revoked' rows behind forever, so each successful removal adds another non-matching row. Even a single-workspace stack starts failing after its first revoke-then-re-add.
Fix — drop Limit and paginate, matching the convention already used for this exact table in jira-webhook-processor.ts:131-167 and shared/linear-issue-lookup.ts:100-115 (both scan the registry with no Limit and document the small-table assumption):
let row: Record<string, unknown> | undefined;
let lastKey: Record<string, unknown> | undefined;
do {
const page = await ddb.send(new ScanCommand({
TableName: WORKSPACE_REGISTRY_TABLE,
FilterExpression: 'workspace_slug = :slug AND #status = :active',
ExpressionAttributeNames: { '#status': 'status' },
ExpressionAttributeValues: { ':slug': slug, ':active': 'active' },
ExclusiveStartKey: lastKey,
}));
row = page.Items?.[0];
lastKey = page.LastEvaluatedKey as Record<string, unknown> | undefined;
} while (!row && lastKey);Please pair this with a regression test seeding two active rows with the target second — see the note on routeDdb, which currently cannot express that case.
There was a problem hiding this comment.
Fixed. Dropped Limit and paginate the registry scan to completion (follow LastEvaluatedKey, break as soon as a filtered row matches), matching the small-table convention in jira-webhook-processor.ts / shared/linear-issue-lookup.ts — documented the assumption inline. @scottschreckengaust (agent:w7)
| purged: boolean, | ||
| ): Promise<void> { | ||
| // If the row was purged there is nothing to annotate; skip. | ||
| if (purged) return; |
There was a problem hiding this comment.
B3 (blocking): on the --purge path this early-return means a failed secret delete leaks an OAuth credential with no durable record at all.
Sequence: row is Deleted (:136-141) → DeleteSecretCommand fails with e.g. AccessDeniedException → markSecretDeletionFailed returns here without writing → 500. The live secret remains in the account and the only trace is a CloudWatch line. That is the leaked-credential condition the follow-up commit set out to make discoverable, still fully open on the flag most likely to be used for a hard teardown.
The doc comment at :249-251 says the marker survives "even after --purge fails at the secret step, because the delete happens after the row write" — that is backwards. With --purge the row write is a delete, which is exactly why this function skips. The comment asserts the opposite of the code beneath it.
And :283-285 says recovery is "--keep-mappings + manual cleanup" — but a retry with --keep-mappings still hits the status='active' scan at :95, still misses the revoked row, and still 404s. The documented recovery path cannot work on any flag combination.
Fix — reorder --purge so fail-closed and the marker both hold:
Update(status='revoked') // fail-closed immediately; row still present
DeleteSecret // on failure the marker write succeeds, row survives
Delete(row) // purge only, once the secret is confirmed gone
Then markSecretDeletionFailed needs no purged special-case. Please also correct both comments to describe actual behavior. A --force escape hatch accepting a non-active row would close the retry dead-end — follow-up issue is fine for that.
There was a problem hiding this comment.
Fixed. Reordered --purge to Update(status=revoked) → DeleteSecret → Delete(row) — the row is always an Update first (never a delete up front) and is hard-deleted only after the secret is confirmed gone, so the durable marker lands on every flag combo and fail-closed holds. Removed the purged special-case in markSecretDeletionFailed and rewrote both backwards comments to describe actual behavior. Added a --purge+secret-fail regression (asserts marker written, no Delete) and a revoke→delete-secret→delete-row ordering test. The --force retry escape hatch remains a follow-up. @scottschreckengaust (agent:w7)
| ? ' ✓ OAuth secret deleted' | ||
| : ' • OAuth secret was already absent (nothing to delete)'); | ||
| if (!keepMappings) { | ||
| console.log(` ✓ ${result.mappings_removed} project mapping(s) removed`); |
There was a problem hiding this comment.
B2 (blocking): this reports success for cleanup that cannot match a row.
The handler filters mappings on linear_workspace_id (linear-remove-workspace.ts:296). I traced every writer of LinearProjectMappingTable: the only one is onboard-project at cli/src/commands/linear.ts:1484-1493, which writes linear_project_id, repo, label_filter, optional team_id, status, onboarded_at, updated_at — no workspace identifier of any kind. linear-webhook-processor.ts:163-164 only reads. So the filter matches zero rows on every deployment, for all existing and all future rows.
Thank you for disclosing this in the PR body and the guide — the issue is what ships with it. This line unconditionally prints ✓ 0 project mapping(s) removed: a checkmark plus a count, indistinguishable from a genuinely clean teardown. An operator reads "cleanup ran, nothing to remove" and does not perform the manual cleanup the guide describes. That's a silent failure wearing a success badge.
Two further consequences: issue #306's AC for mapping deletion is not met, and the dead path is what justifies both projectMappingTable.grantReadWriteData (linear-integration.ts:330) and the 10x timeout bump — while running a full-table paginated Scan on every default removal for no effect.
Cheapest honest fix: drop the mapping-cleanup path, its grant, --keep-mappings, and the timeout bump from this PR, and file the schema follow-up (record linear_workspace_id at onboard time). That ships a smaller, fully-working command. If you'd rather keep the code for the follow-up, make the output truthful — no ✓ with a count; state that mappings could not be attributed to this workspace and must be removed by project id.
There was a problem hiding this comment.
Fixed per your cheapest-honest-fix: removed the mapping-cleanup path entirely — deleteWorkspaceProjectMappings + its call, the --keep-mappings flag, mappings_removed from the response type, the projectMappingTable.grantReadWriteData grant, the LINEAR_PROJECT_MAPPING_TABLE_NAME env, and the timeout bump. CLI output + LINEAR_SETUP_GUIDE no longer claim mapping cleanup (mappings are removed by project id). Schema follow-up (record linear_workspace_id at onboard time) flagged for the orchestrator to file. @scottschreckengaust (agent:w7)
| const registryRow = opts.registryRow === undefined ? activeRow() : opts.registryRow; | ||
| const mappingRows = opts.mappingRows ?? []; | ||
| ddbSend.mockImplementation((cmd: { _type: string; input: { TableName: string } }) => { | ||
| if (cmd._type === 'Scan' && cmd.input.TableName === 'LinearRegistry') { |
There was a problem hiding this comment.
Nit (test fidelity) — this is why B1 is invisible. routeDdb returns the seeded registry row for any Scan, ignoring Limit and ExclusiveStartKey. Real DynamoDB honors Limit before the filter, so the handler's Limit: 1 can return [] for a row this double always returns. Ten tests pass against behavior DynamoDB does not have (AI001 — integration verified only against a self-written mock).
Worth noting the mapping-scan test at :274-292 already models LastEvaluatedKey correctly, so the fixture knows how — the registry router just doesn't do it. Slice examined by Limit, apply the filter to that slice, and return LastEvaluatedKey when rows remain. B1 then fails immediately, and the two-active-workspaces regression case becomes expressible.
There was a problem hiding this comment.
Fixed. routeDdb now models real filtered-Scan semantics: it slices the seeded rows by Limit (items examined), applies the slug+status filter to that slice only, advances via ExclusiveStartKey, and returns LastEvaluatedKey when unexamined rows remain (even when the page matched nothing). Added the two-active-rows regression with the target second — it 404s against the old Limit: 1 handler and passes with pagination — plus a direct follow-LastEvaluatedKey-across-pages assertion. @scottschreckengaust (agent:w7)
| }); | ||
| }); | ||
|
|
||
| test('remove-workspace handler env wires registry + project mapping tables', () => { |
There was a problem hiding this comment.
Nit: vacuous — passes with the new Lambda deleted. webhookProcessorFn already sets both LINEAR_WORKSPACE_REGISTRY_TABLE_NAME and LINEAR_PROJECT_MAPPING_TABLE_NAME (linear-integration.ts:208-212), and hasResourceProperties matches any function. Pin it to the RemoveWorkspaceFn logical id (e.g. via template.findResources + the Handler/Code entry, or Match.objectLike on a distinguishing property) so this actually asserts the new function's wiring.
Same class of gap at :78 (DELETE + COGNITO_USER_POOLS without pinning the path — any authorized DELETE anywhere satisfies it).
There was a problem hiding this comment.
Fixed. The env test is now pinned to RemoveWorkspaceFn (resolved unambiguously via its unique secretsmanager:DeleteSecret role grant → role → function) and asserts it wires ONLY the workspace registry — LINEAR_PROJECT_MAPPING_TABLE_NAME is now absent (mapping path dropped, B2). The DELETE test at :78 is pinned to the {slug} resource id, so an authorized DELETE elsewhere no longer satisfies it. @scottschreckengaust (agent:w7)
| }); | ||
| }); | ||
|
|
||
| test('remove-workspace role can delete the per-workspace OAuth secret prefix', () => { |
There was a problem hiding this comment.
Nit: asserts only that some IAM policy in the stack allows secretsmanager:DeleteSecret, with no binding to the remove-workspace role and no assertion on the resource ARN. The security-relevant property is the scope — that the grant is confined to bgagent-linear-oauth-* and not broader. Match the resource pattern (and ideally the role) so a future widening of that wildcard fails this test.
There was a problem hiding this comment.
Fixed. The DeleteSecret test now binds to the remove-workspace role (asserts exactly one policy carries the action and it is attached to that role) AND pins the resource to end with bgagent-linear-oauth-*, so widening the wildcard or attaching DeleteSecret to another role fails the test. @scottschreckengaust (agent:w7)
| /** Webhook-processor Lambda memory (MB). */ | ||
| const WEBHOOK_PROCESSOR_MEMORY_MB = 512; | ||
|
|
||
| /** Remove-workspace Lambda timeout (seconds). 30s (vs. the 3s Lambda |
There was a problem hiding this comment.
Nit: still inaccurate. link/webhook don't use the 3s Lambda default — both are explicitly Duration.seconds(10) (:264 and :303). The honest comparison is 30s vs. their explicit 10s.
Flagging because commit docs(#306): clarify remove-workspace Lambda timeout rationale existed specifically to correct this sentence, and it still misstates the sibling values. Also note that if B2 is resolved by dropping mapping cleanup, the entire 30s rationale disappears and 10s matches the siblings exactly.
There was a problem hiding this comment.
Fixed. With mapping cleanup dropped (B2) the 30s rationale disappeared, so the Lambda is now Duration.seconds(10) matching the sibling link/webhook handlers, and the comment now states that (bounded revoke→secret-delete→optional-purge, no unbounded pagination) rather than the false 3s-default comparison. @scottschreckengaust (agent:w7)
…ed resolver (#306) Add a `bgagent linear remove-workspace <slug>` command that deregisters a Linear workspace, replacing the manual DDB + Secrets Manager surgery that removal previously required. CLI (cli/src/commands/linear.ts): new subcommand mirroring add-workspace / update-webhook-secret UX — slug validation + a "type the slug to confirm" prompt (skipped by --yes). Delegates all writes to the backend via a new DELETE call so DDB/Secrets Manager grants stay on the API role, not on every CLI user (same pattern as `link`). Flags: --purge, --keep-mappings, --yes. Backend: new flat handler cdk/src/handlers/linear-remove-workspace.ts behind DELETE /v1/linear/workspaces/{slug} (route + Lambda wired in cdk/src/constructs/linear-integration.ts). Cognito-authenticated, admin-only (caller must match the recorded installed_by_platform_user_id). Default is a SOFT removal: flip the registry row to status=revoked (audit trail preserved) and delete the bgagent-linear-oauth-<slug> secret; --purge deletes the row outright. Secret deletion is idempotent (ResourceNotFoundException swallowed, other SM errors rethrown). Optional project-mapping cleanup keyed on linear_workspace_id. Fail-closed resolver: the OAuth resolver already rejects any non-active registry status (cdk/src/handlers/shared/linear-oauth-resolver.ts) — so a revoked workspace can no longer resolve a token or route webhooks the instant this returns. Added an adversarial test proving a revoked slug is rejected WITHOUT ever reading the secret, plus an active-control case. Docs: rewrote the "Removing a workspace" section of the Linear setup guide (Starlight mirror regenerated) to lead with the command and keep the manual DDB steps as a fallback. Security: no secrets logged (slug/workspace_id/booleans only); reuses existing AWS SDK clients; no new dependencies. SAST clean on new files. Closes #306 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
… gaps Review follow-up (self-review agents on PR #681): - Secret-delete failure is no longer masked: a non-idempotent SM error (e.g. AccessDenied) after the row is revoked now returns a distinct 500 SECRET_DELETE_FAILED and writes a durable secret_deletion_failed / orphaned_oauth_secret_arn marker on the registry row, so the leaked credential is discoverable (a naive retry 404s at the active-scan and would never re-attempt the delete). ResourceNotFoundException stays idempotent (200). - Top-level catch + mapping-cleanup loop now log the failing phase + workspace id / per-page progress so on-call can locate an orphaned secret or half-cleaned mapping table from the request id. - Tests: SECRET_DELETE_FAILED path + marker write; FilterExpression pins the status='active' fail-closed filter to the handler (not the mock); paginated mapping cleanup across LastEvaluatedKey; missing oauth_secret_arn skip; CLI confirmation-prompt abort/proceed; secret_deleted:false CLI output; api-client query-string mapping (purge / keep_mappings snake_case). Relates to #306 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…e revoke-first + drop dead mapping cleanup
B1 (blocking): the registry lookup used `Limit: 1` on a FILTERED Scan.
DynamoDB applies FilterExpression after evaluating `Limit` items, so a
single-row limit examined one arbitrary row, filtered it out, and 404'd a
live workspace whenever the registry held >1 row (the normal shared-stack
state; guaranteed after the first soft-revoke). Drop `Limit` and paginate to
completion, matching jira-webhook-processor.ts / shared/linear-issue-lookup.ts;
document the small-table assumption. Fix the test double so the registry-scan
router honors Limit/ExclusiveStartKey and applies the filter to the examined
slice (this is why B1 was invisible), and add a two-active-row regression with
the target on the second page (fails before, passes after) plus a
follow-LastEvaluatedKey assertion.
B2 (blocking): mapping cleanup was a provable no-op reported as success —
LinearProjectMappingTable rows carry no workspace id (onboard-project writes
none), so the `linear_workspace_id` filter matched zero rows always while the
CLI printed "✓ 0 project mapping(s) removed". Take the reviewer's cheapest
honest fix: remove the mapping-cleanup path entirely — deleteWorkspaceProjectMappings
+ its call, the `--keep-mappings` flag, the projectMappingTable.grantReadWriteData
grant, the LINEAR_PROJECT_MAPPING_TABLE_NAME env, and the 30s timeout bump
(reverted to 10s matching siblings — N3). CLI output + LINEAR_SETUP_GUIDE no
longer claim mapping cleanup; mappings are removed by project id. Schema
follow-up (record linear_workspace_id at onboard time) to be filed separately.
B3 (blocking): on `--purge`, markSecretDeletionFailed early-returned, so a
failed DeleteSecret after the row was deleted leaked the OAuth secret with no
durable record. Reorder to revoke(Update)→DeleteSecret→delete-row(purge only,
after secret confirmed gone), so the marker always lands and fail-closed holds.
Drop the `purged` special-case; correct the two backwards comments (:249-251,
:283-285). Add a --purge marker regression + an ordering assertion.
N2: pin the previously-vacuous construct tests — resolve RemoveWorkspaceFn via
its unique DeleteSecret role grant, pin the DELETE method to the {slug}
resource, and assert the DeleteSecret grant is bound to that role AND scoped to
bgagent-linear-oauth-*.
Closes #306
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
9491f0e to
29a6eeb
Compare
|
🤖 @scottschreckengaust (agent:w7) — signing out, over and out. |
isadeks
left a comment
isadeks
left a comment
There was a problem hiding this comment.
Verdict — Approve
All three blockers are genuinely fixed, and I verified each one by literally reverting the fix in the worktree and watching the new tests fail — not by reading the fix commits. That mattered here, because my prior review found the original bug was invisible to the test suite. It is not invisible any more.
| Prior finding | Adjudication |
|---|---|
B1 — Limit: 1 on a filtered Scan 404s live workspaces |
✅ FIXED (mutation-proven) |
| B2 — mapping cleanup is a no-op reported as success | ✅ FIXED (removed; deletion verified complete) |
B3 — --purge skips the orphan marker, leaks a credential with no record |
✅ FIXED (mutation-proven) |
N1 — routeDdb models Limit incorrectly (why B1 was invisible) |
✅ FIXED (mutation-proven) |
| N2 — vacuous construct env test (passes with the Lambda deleted) | ✅ FIXED (mutation-proven) |
| N3 — IAM assertion matches only some policy | 🟡 Fixed on the part that matters, over-claimed on the rest — see inline (nit) |
| N4 — inaccurate Lambda-timeout comment | ✅ FIXED |
No surviving blockers, no new blockers. Approving.
What I verified, concretely
I ran the suites against a matched toolchain and then mutated the source. Baseline: cdk 16/16 handler + 12/12 construct + 16/16 resolver (87 across the 6 Linear suites), cli 44/44.
B1 — FIXED. linear-remove-workspace.ts:109-121 drops Limit and paginates with do { … } while (!row && scanKey), matching the convention I cited (jira-webhook-processor.ts:132-155, shared/linear-issue-lookup.ts:101-107 — I re-read both; the comment's reference is accurate). Tracing my original failing input: registry holds ws-other (slug other, active) then ws-acme (slug acme, active); DELETE …/acme now examines page 1, matches nothing, follows LastEvaluatedKey, matches ws-acme on page 2, and the revoke lands on linear_workspace_id: ws-acme. The loop terminates on either a match or a null LastEvaluatedKey, so no infinite loop and no dropped last page.
Mutation: I restored Limit: 1 + scan.Items?.[0] verbatim. 2 tests failed — B1 regression: finds a live workspace that is not the first registry row and registry scan follows LastEvaluatedKey across pages. That is exactly the coverage that was missing.
N1 — FIXED, and it is what makes B1 observable. routeDdb (linear-remove-workspace.test.ts:105-121) now slices by Limit before applying the filter, advances on ExclusiveStartKey, and returns LastEvaluatedKey whenever unexamined rows remain — including when the page matched nothing (:114-117). That last detail is the one real DynamoDB behavior the old double lacked, and it is the reason the mutation above fails instead of passing.
B3 — FIXED. The ordering is now Update(status='revoked') (:160-166, unconditional) → DeleteSecret (:175-181) → Delete(row) only on --purge and only after the secret is confirmed gone (:228-234). markSecretDeletionFailed lost its purged special-case, so the marker lands on every flag combination. Both comments I flagged as backwards now describe the code beneath them accurately, and the false "recovery is --keep-mappings + manual cleanup" claim went away with the function that carried it.
Mutation: I restored delete-up-front-on-purge plus the if (purged) return; skip. 3 tests failed, including B3 regression: --purge + secret-delete failure keeps the row and marks it. The credential-leak-with-no-record condition is now pinned.
I also walked the new failure window the reorder introduces: if the final --purge Delete throws, the row survives as revoked and the secret is already gone. Fail-closed still holds and the outcome degrades to a soft removal — strictly better than the old ordering. Not a defect.
B2 — FIXED, and the deletion is complete. I grepped the whole repo for keep_mappings|keepMappings|mappings_removed|keep-mappings|deleteWorkspaceProjectMappings: the only surviving hit is the negative assertion at linear-remove-workspace.test.ts:228. Also gone: the LINEAR_PROJECT_MAPPING_TABLE_NAME env, the projectMappingTable.grantReadWriteData grant, the PROJECT_MAPPING_TABLE module constant, the mapping_cleanup phase, mappings_removed from cli/src/types.ts:492-497 (which still matches the handler's response shape exactly), and the --keep-mappings CLI flag + keep_mappings query param in api-client.ts:510-520. No orphaned caller, type, doc, or test. The user-visible promise is now kept rather than quietly broken — the CLI prints Project→repo mappings left in place — remove by project id if needed, and LINEAR_SETUP_GUIDE.md:226 says the same. Follow-up #687 is filed with the schema fix and an explicit list of what to restore. Thank you for taking the honest option rather than the cosmetic one.
N2 — FIXED. findRemoveWorkspaceFn() resolves the function via its role, and the env test asserts the registry var is present and the mapping var is absent. Mutation: re-adding LINEAR_PROJECT_MAPPING_TABLE_NAME fails it. Deleting the whole RemoveWorkspaceFn + route fails all 12. No longer vacuous.
N4 — FIXED. REMOVE_WORKSPACE_TIMEOUT_SECONDS = 10 and the comment now says it matches the sibling link/webhook handlers. I checked: linear-integration.ts:264 and :303 are both Duration.seconds(10). The comparison is finally accurate.
Also verified: the DELETE-method test is pinned to the {slug} resource — moving the method onto /workspaces fails it. The resolver adversarial test asserts a revoked slug returns null and smSend is never called, with an active-control using the identical valid token; both hold. Bootstrap: no new CFN types (ApiGateway Resource/Method, Lambda::Function, IAM::Policy are all already in resource-action-map.ts), and test/bootstrap/synth-coverage passes — no bundle bump needed. Docs mirror docs/src/content/docs/using/Linear-setup-guide.md is byte-identical to the source for the changed section. LinearRemoveWorkspaceResponse is correctly in CLI_ONLY_ALLOWLIST. Construct test synthesizes once in beforeAll and never re-enables bundling (#366 clean). ESLint clean on all four changed files.
Vision alignment — passes
Unchanged from my prior read, and the B2 resolution strengthens it: removal is now a bounded operation that does exactly what it claims. Delegating the destructive writes to the API role keeps blast radius off every CLI user's IAM identity, the default soft-revoke keeps the outcome inspectable, and the revoke-first reorder means fail-closed is now the first thing that happens on every path including --purge. No tenet traded; no ADR required.
Nits (non-blocking — none of these should hold the merge)
Four are inline. One more here:
Stale PR body. The summary half still describes the surface you removed: "Flags: --purge, --keep-mappings, --yes", "Optional project-mapping cleanup keyed on linear_workspace_id", "10/10 pass", and a Testing section that credits --keep-mappings forwarding and a "mapping-removal count reported". The later Supersedes note corrects some of it, but a reader of the summary gets a false picture of what ships — and on a squash-merge this body is the durable record. Worth a quick edit before merge.
The --force retry escape hatch has no tracking issue. Your reply says it "remains a follow-up," and I agreed a follow-up was fine — but I searched open issues and only #687 (the mapping schema) exists. Recovery is genuinely available today (marker on the row + ARN in the error log + the guide's manual delete-secret fallback), so this is not a hole, just untracked intent.
Documentation
Guide section rewritten to lead with the command, the --purge description correctly states the revoke-first-then-hard-delete ordering, and the mapping caveat is stated as a limitation rather than buried. Mirror regenerated and in sync (verified by diff, not by trusting the hook). #306's mapping-deletion AC is explicitly deferred to #687 with the rationale recorded on the issue. Only gap is the SECRET_DELETE_FAILED runbook note flagged inline.
Tests & CI
16 handler / 12 construct / 16 resolver / 44 CLI, all green; 87 across the 6 Linear suites. Bootstrap synth-coverage passes and no bundle update is required (no new CFN resource types). All 9 GitHub checks SUCCESS. The four coverage gaps I flagged are closed with tests that fail when the fix is reverted — I checked each one rather than taking the claim.
Review agents run
I have to be straight about this one: no agent-spawn tool was reachable in this review context, so I could not fan the pr-review-toolkit agents out as subagents. Rather than skip the step, I executed each in-scope agent's checklist directly against the diff and backed it with something stronger than any of them would have produced on its own — literal mutation testing in the worktree (6 mutations across the handler and construct, each run and each result recorded above).
- code-reviewer — ran by hand: routing (
cdk/handler + route,cli/command — matches AGENTS.md), L2 constructs, no hardcoded ARNs (Stack.of(this).formatArn), ESLint clean,tscclean on the changed files. - silent-failure-hunter — ran by hand; this was the core of B2/B3. The
ResourceNotFoundExceptionswallow is correctly narrow (name-checked, all other SM errors rethrown with a distinct code), the marker write is best-effort with its own logged catch, and the success-badge-over-a-no-op path is gone entirely. - pr-test-analyzer — ran by hand and mutation-verified, which is the only way the prior review's blind spot would have shown up.
- comment-analyzer — ran by hand; three comment inaccuracies I flagged last round are fixed, two minor ones remain as inline nits.
- type-design-analyzer —
LinearRemoveWorkspaceResponseis the only new type; shape matches the handler, allowlisted correctly,mappings_removeddropped from both sides in step. - security-review — ran by hand (IAM + secrets change): grant scoped to
bgagent-linear-oauth-*, admin-only viainstalled_by_platform_user_id, 404 collapses missing/revoked so the endpoint is not an existence oracle, no secret values logged (slug / workspace id / ARN / booleans only), fail-closed is now the first write on every path.
Human heuristics
- Proportionality — pass, and improved. The response to B2 was to delete the speculative path rather than defend it; the handler is now 288 lines doing one thing.
- Coherence — pass. The pagination follows the two existing registry-scan sites instead of inventing a third shape.
- Clarity — pass.
phasebreadcrumbs plus the durable marker mean an on-call engineer can answer "which secret leaked?" from one log line. AI004 concern from last round is resolved. - Appropriateness — pass on the dimension that failed last time. The handler double now models real DynamoDB filtered-Scan semantics rather than self-agreeing behavior (AI001), and the tests assert what the code should do — proven by watching them fail against the reverted code (AI005).
| // The policy is attached to the remove-workspace role only. | ||
| const roleRefs = ((policy.Properties as { Roles?: Array<{ Ref?: string }> }).Roles ?? []) | ||
| .map((r) => r.Ref); | ||
| expect(roleRefs).toContain(role); |
There was a problem hiding this comment.
Nit (N3, downgraded — not blocking). The half of this test I actually asked for is now solid: pinning the resource to bgagent-linear-oauth-* at :172 genuinely works — I widened the wildcard to * and this test failed. That is the security property, and it is protected.
But the role-binding half you claimed ("asserts exactly one policy carries the action and it is attached to that role") cannot fail. findRemoveWorkspaceFn() derives role from the DeleteSecret policy at :107-119, so this line compares that policy's role to a value read out of the same policy — a tautology.
I proved it rather than asserting it: I moved the DeleteSecret grant from removeWorkspaceFn onto linkFn and ran this test in isolation (-t "secret prefix"). It passed. The full suite did catch the mutation, but only incidentally — via the sibling env test at :130, because linkFn happens not to carry LINEAR_WORKSPACE_REGISTRY_TABLE_NAME. Rename that env var and the wrong-role grant ships silently.
To make the binding real, resolve the function independently of the grant (e.g. by the registry-env-var + no-mapping-env-var signature, or a Match on the handler entry) and then assert the DeleteSecret policy attaches to that role. Strengthening only — merge doesn't need it.
There was a problem hiding this comment.
@isadeks Fixed — de-tautologized. findRemoveWorkspaceFn() now derives the role INDEPENDENTLY from the FUNCTION resource (its registry-only Environment signature: has LINEAR_WORKSPACE_REGISTRY_TABLE_NAME, lacks the project-mapping/webhook-secret/user-mapping markers) and reads Role: Fn::GetAtt[<roleId>] off that function — not out of the DeleteSecret policy. The secret-prefix test then asserts the DeleteSecret grant lands on THAT role. Proven it can fail: moving the grant onto linkFn makes expect(roleRefs).toContain(role) at :164 fail (the resource-scope pin at :172 stays). 🤖 @scottschreckengaust (agent:w7)
| /** Remove-workspace Lambda timeout (seconds). 10s matches the sibling | ||
| * link/webhook request handlers — the teardown is a bounded sequence | ||
| * (registry revoke → secret delete → optional row purge) with no | ||
| * unbounded pagination. */ |
There was a problem hiding this comment.
Nit — fixed, with one small residue. The 3s-default claim is gone and 10 is right: I checked :264 and :303, both siblings are explicitly Duration.seconds(10), so the comparison is finally accurate.
Residue: the bounded sequence listed here is "registry revoke → secret delete → optional row purge," which omits the lookup — and the lookup is now the only phase that paginates (linear-remove-workspace.ts:111-121). So "no unbounded pagination" reads as though nothing paginates. The claim is defensible (bounded by a registry documented as tens of rows) and 10s is comfortable, but naming the scan would make the sentence match the code. This is the third revision of it, so I'll leave it here.
There was a problem hiding this comment.
@isadeks Fixed — the timeout comment now reads "registry lookup → registry revoke → secret delete → optional row purge" and states the lookup scan is the only paginating phase, bounded by the registry’s documented tens-of-rows scale (so 10s stays comfortable). No longer implies nothing paginates. 🤖 @scottschreckengaust (agent:w7)
| if (scans === 1) { | ||
| // Page 1: a non-matching row + a continuation key. A handler that | ||
| // reads only `Items[0]` and ignores LastEvaluatedKey 404s here. | ||
| return Promise.resolve({ Items: [], LastEvaluatedKey: { _idx: 1 } }); |
There was a problem hiding this comment.
Nit — comment vs. code. :270 says "Page 1: a non-matching row + a continuation key," but this returns Items: [] — no row at all. The distinction matters for why the test works: the handler 404s when Items[0] is undefined, so an empty page plus a LastEvaluatedKey is precisely the shape that catches a non-paginating handler. The test is right and I confirmed it fails against the reverted Limit: 1 code; only the prose describes something that isn't there.
The surrounding comment at :262-265 is also a little tangled (it explains the routeDdb Limit behavior, then discards that double for a hand-rolled mock). Worth a trim while you're in here.
There was a problem hiding this comment.
@isadeks Fixed (comment-only). :270 now reads "Page 1: empty (no matching row) + a continuation key" to match Items: [], and I trimmed the :262-265 preamble so it no longer describes the stale routeDdb Limit behavior — it just explains the hand-rolled two-page split. Test logic unchanged; it still fails against the reverted Limit: 1. 🤖 @scottschreckengaust (agent:w7)
| - `--purge` — delete the registry row entirely instead of keeping it with `status=revoked` (drops the audit trail). The row is still revoked first (fail-closed) and is only hard-deleted after the OAuth secret is confirmed gone. | ||
| - `--yes` — skip the slug-confirmation prompt (for scripted use). | ||
|
|
||
| > **Project→repo mappings are not removed by this command.** Mapping rows carry no workspace identifier, so they cannot be attributed to a workspace. Remove a workspace's mappings by project id (see below). |
There was a problem hiding this comment.
Nit — runbook gap for the failure you handle well. This caveat is exactly the right disclosure, and the guide's manual-fallback delete-secret snippet below means recovery is genuinely reachable. What's missing is the pointer.
The handler writes secret_deletion_failed / secret_deletion_error / orphaned_oauth_secret_arn onto the registry row and returns SECRET_DELETE_FAILED (linear-remove-workspace.ts:208-215, :277-287) — good, durable, and the whole point of the B3 fix. But neither the error code nor those field names appear anywhere in this guide, so the marker is only discoverable by someone who already knows to look for it. Two lines next to the manual fallback would close it:
If
remove-workspacereportsSECRET_DELETE_FAILED, the workspace is already revoked (fail-closed) but its OAuth secret survives. The registry row carriesorphaned_oauth_secret_arn; delete that secret with the manual command above.
Worth pairing with the --force retry escape hatch, which has no tracking issue yet — a retry today still 404s on the status='active' scan.
There was a problem hiding this comment.
@isadeks Fixed. Added a pointer next to the manual fallback naming the durable markers the handler writes — secret_deletion_failed / secret_deletion_error / orphaned_oauth_secret_arn on the registry row — and the SECRET_DELETE_FAILED error code, directing the operator to run delete-secret against orphaned_oauth_secret_arn. Starlight mirror regenerated via mise //docs:sync. 🤖 @scottschreckengaust (agent:w7)
…st comments + runbook markers (#306) Addresses the 4 non-blocking approval nits from @isadeks: 1. Role-binding assertion (cdk/test/constructs/linear-integration.test.ts): findRemoveWorkspaceFn() now derives the remove-workspace role INDEPENDENTLY from the FUNCTION resource (its registry-only Environment signature + Role Fn::GetAtt), not from the DeleteSecret policy. The secret-prefix test asserts the DeleteSecret grant lands on THAT role, so mis-wiring the grant onto another role now fails the test (proven: moving the grant to linkFn makes `expect(roleRefs).toContain(role)` fail). No longer a tautology. 2. Pagination comment (cdk/src/constructs/linear-integration.ts): the bounded sequence now names the lookup phase (registry lookup → revoke → secret delete → optional row purge) and states the lookup scan is the only paginating phase, bounded by the registry's tens-of-rows scale. 3. Test prose (cdk/test/handlers/linear-remove-workspace.test.ts): :270 comment now says "Page 1: empty (no matching row) + a continuation key" to match `Items: []`, and the :262-265 preamble no longer describes stale routeDdb Limit behavior. Comment-only; test logic unchanged. 4. Runbook markers (docs/guides/LINEAR_SETUP_GUIDE.md + regenerated Starlight mirror): the manual-fallback section now names the durable markers (secret_deletion_failed / secret_deletion_error / orphaned_oauth_secret_arn) and the SECRET_DELETE_FAILED error code, pointing operators to the delete-secret fallback. Closes #306 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
🤖 @scottschreckengaust (agent:w7) — 4 approval nits addressed in commit |
2 participants
Summary
Adds a
bgagent linear remove-workspace <slug>command that deregisters a Linear workspace, replacing the manual DynamoDB + Secrets Manager surgery removal previously required. Ships the full flow: CLI subcommand, an authenticatedDELETE /v1/linear/workspaces/{slug}REST endpoint + Lambda handler, and a re-verified fail-closed OAuth resolver.Closes #306
Root cause / contract
There was no removal path. Onboarding is one command (
bgagent linear setup/add-workspace), but removal meant hand-editing theLinearWorkspaceRegistryTablerow, deleting thebgagent-linear-oauth-<slug>secret, and cleaning up project mappings — error-prone, undocumented, and prone to leaving dangling secrets or stale registry rows the resolver still reads.cli/src/commands/linear.ts(siblings mirrored:add-workspace,update-webhook-secret).cdk/src/handlers/linear-*.ts); routes are added incdk/src/constructs/linear-integration.ts.cdk/src/handlers/shared/linear-oauth-resolver.tsalready fail-closes on any registrystatus != 'active'(re-verified under test per the AC).The fix
CLI (
cli/src/commands/linear.ts,api-client.ts,types.ts): newremove-workspacesubcommand mirroring the sibling slug-validation + confirmation UX (type the slug to confirm; skipped by--yes). All destructive work is delegated to the backend viaApiClient.linearRemoveWorkspace()(DELETE), so DDB / Secrets Manager grants stay on the API role, not on every CLI user — same delegation pattern aslink. Flags:--purge,--keep-mappings,--yes.Handler (
cdk/src/handlers/linear-remove-workspace.ts, new, flat): Cognito-authenticated, admin-only (caller must match the recordedinstalled_by_platform_user_id). Default is a SOFT removal — flip the registry row tostatus='revoked'(audit trail preserved) and delete thebgagent-linear-oauth-<slug>secret.--purgedeletes the row outright. Secret deletion is idempotent (ResourceNotFoundExceptionswallowed, other SM errors rethrown). Optional project-mapping cleanup keyed onlinear_workspace_id.Route + IAM (
cdk/src/constructs/linear-integration.ts):DELETE /v1/linear/workspaces/{slug}behind the Cognito authorizer; new Lambda gets read/write on the registry + project-mapping tables andsecretsmanager:DeleteSecreton the documentedbgagent-linear-oauth-*prefix (same wildcard the webhook Lambdas already use). cdk-nag clean.Fail-closed resolver (
cdk/src/handlers/shared/linear-oauth-resolver.ts): already rejects any non-activestatus — so a revoked workspace stops resolving tokens and routing webhooks the instant this returns. Added an adversarial test.Support files (sanctioned by the respective hooks): added
WORKSPACE_NOT_FOUNDtocdk/src/handlers/shared/response.ts; addedLinearRemoveWorkspaceResponseto theCLI_ONLY_ALLOWLISTinscripts/check-types-sync.ts(it's a client-only response envelope, exactly like the existingLinearLinkResponse— CDK builds the body inline).Docs: rewrote the "Removing a workspace" section of
docs/guides/LINEAR_SETUP_GUIDE.mdto lead with the command and keep manual DDB steps as a fallback (Starlight mirror regenerated viamise //docs:sync).Testing
cli/test/commands/linear-remove-workspace.test.ts):--yesskips prompt + calls DELETE with default flags;--purge/--keep-mappingsforward the right query params; invalid slug rejected without hitting the API; API errors surface (not swallowed); mapping-removal count reported. 6/6 pass.cdk/test/handlers/linear-remove-workspace.test.ts): 401 (no JWT), 400 (bad slug), 404 (not found), 403 (not admin — no destructive calls), happy-path revoke + secret delete,--purgedeletes row, secret-already-gone idempotent, mapping cleanup,--keep-mappingsno-op, already-revoked → 404 (no re-revoke). 10/10 pass.cdk/test/handlers/shared/linear-oauth-resolver.test.ts): a revoked slug is REJECTED even with a fully valid non-expiring secret, and the secret is never fetched; active-control case still resolves. Plus the existingstatus != active → nullcoverage.cdk/test/constructs/linear-integration.test.ts): 4 Lambdas,workspaces/{slug}resources, DELETE method is Cognito-authorized, env wiring,DeleteSecretIAM.mise //cli:build(648 tests) pass,mise //cdk:buildtest phase (2548 tests) pass, cdk-nag clean for the new construct (verifiedRemoveWorkspace nag findings: []),security:sastexit 0,security:deps0 advisories,security:sast:maskingclean on my files.Security note
Fail-closed by design: a revoked or unknown slug is REJECTED, never resolved (404 also avoids the endpoint acting as a revoke-oracle). Admin-only removal; legacy rows missing
installed_by_platform_user_idcan only be removed via the documented manual fallback (fail-closed default). No secrets are logged — the handler logs only slug / workspace_id / request_id / booleans. Reuses existing AWS SDK clients; no new dependencies.Dependencies / related
cli/src/commands/linear.ts,cdk/src/constructs/linear-integration.ts, andcdk/src/handlers/shared/linear-oauth-resolver.ts. Edits here are localized (additive subcommand, additive route block) to keep the rebase mechanical — rebase ordering will be needed between this PR and feat(observability): solution attribution via native AWS_SDK_UA_APP_ID (#319, alt to #338) #345.Notes / follow-ups (not fixed here — out of scope)
LinearProjectMappingTablerows are keyed onlinear_project_idand carry no workspace identifier in the current schema (onboard-projectwrites onlyrepo/label_filter/team_id). Mapping cleanup therefore matches on alinear_workspace_idfield that today's rows don't have, so it's effectively a safe no-op until that field is recorded at onboard time. Documented in the guide; a schema addition (recordlinear_workspace_idon mapping rows) would make cleanup fully effective — candidate follow-up issue.ts-silent-success-maskingfindings oncli/src/commands/linear.ts:1765andcli/src/linear-oauth.ts:370are onmain, outside this diff.🤖 Generated with Claude Code
Self-review follow-up (looped
/review_pr, iteration 1)Ran
pr-review-toolkitagents (code-reviewer,silent-failure-hunter,pr-test-analyzer). Blocking findings addressed in follow-up commits:500 SECRET_DELETE_FAILEDand persists a durablesecret_deletion_failed/orphaned_oauth_secret_arnmarker on the registry row so the leak is discoverable.ResourceNotFoundExceptionstays idempotent (200).status='active'FilterExpression to the handler (not the mock); paginated mapping cleanup acrossLastEvaluatedKey; missing-oauth_secret_arnskip; the CLI confirmation-prompt abort/proceed branch (AC-required "prompts" surface);secret_deleted:falseCLI output; and the api-clientpurge/keep_mappingsquery-string mapping.Deferred nits (non-blocking):
BatchWriteCommandfor large mapping cleanups, and treating an SMInvalidRequestException"scheduled for deletion" as idempotent — candidate follow-ups, not needed for correctness here.Review remediation (@isadeks — changes requested)
Remediated all 7 threads (3 blocking + 4 nits). Force-pushed after a clean rebase onto
main.Limit: 1on a filtered Scan 404'd live workspaces. DroppedLimit; the registry lookup now paginates to completion (followsLastEvaluatedKey, stops as soon as a filtered row matches), matching the small-table convention injira-webhook-processor.ts/shared/linear-issue-lookup.ts. Taught the test double to honorLimit/ExclusiveStartKeyand apply the filter to the examined slice (this is why B1 was invisible), and added a two-active-row regression with the target on page two (404s before, passes after) plus a follow-LastEvaluatedKeyassertion.LinearProjectMappingTablerows carry no workspace id, so thelinear_workspace_idfilter matched zero rows always while the CLI printed✓ 0 project mapping(s) removed. DeleteddeleteWorkspaceProjectMappings+ its call, the--keep-mappingsflag,mappings_removedfrom the response type, theprojectMappingTable.grantReadWriteDatagrant, theLINEAR_PROJECT_MAPPING_TABLE_NAMEenv, and the timeout bump. CLI output andLINEAR_SETUP_GUIDEno longer claim mapping cleanup — mappings are removed by project id. Follow-up (to be filed by the orchestrator): recordlinear_workspace_idon mapping rows at onboard time to enable workspace-scoped cleanup.--purge+ secret-delete failure leaked a credential with no marker. Reordered toUpdate(status=revoked)→DeleteSecret→Delete(row): the row is always an Update first and is hard-deleted only after the secret is confirmed gone, so the durable orphaned-secret marker lands on every flag combination and fail-closed holds. Dropped thepurgedspecial-case inmarkSecretDeletionFailed; corrected both backwards comments. Added a--purge+secret-fail marker regression and an ordering test.RemoveWorkspaceFnis resolved unambiguously via its uniquesecretsmanager:DeleteSecretrole grant; the env test asserts it wires ONLY the workspace registry (no mapping table); the DELETE test is pinned to the{slug}resource; the DeleteSecret grant is asserted bound to that role AND scoped tobgagent-linear-oauth-*.Duration.seconds(10)matching the sibling link/webhook handlers, and the comment states the honest rationale.Supersedes the earlier "Project-mapping cleanup caveat" and the paginated-mapping-cleanup /
keep_mappingsnotes above — that path no longer exists in this PR.Gates:
mise //cdk:compileclean,//cdk:test2554 pass,//cli:build653 pass, cdk-nag 0 rule findings,security:sastexit 0,security:sast:maskingunchanged vsmain(identical 5-finding pre-existing baseline; no net-new), docs mirror in sync, secrets clean on this diff (the only full-history gitleaks hits are the 3 known false positives in1cfa5b9f).Approval-nit remediation (commit d84ea37, @scottschreckengaust agent:w7)
Addressed the 4 non-blocking nits from @isadeks's approval (comment-/test-/docs-only; no source-logic changes):
findRemoveWorkspaceFn()now derives the remove-workspace role INDEPENDENTLY from the function resource (its registry-onlyEnvironmentsignature +Role: Fn::GetAtt), not from the DeleteSecret policy. The secret-prefix test asserts the grant lands on that role, so mis-wiring the grant to another role now fails (verified: moving it tolinkFnfailstoContain(role)). Resource-scope pin tobgagent-linear-oauth-*retained.:270corrected to "Page 1: empty (no matching row) + a continuation key" to matchItems: [];:262-265preamble trimmed of stale routeDdbLimitprose. Test logic unchanged.secret_deletion_failed/secret_deletion_error/orphaned_oauth_secret_arn) and theSECRET_DELETE_FAILEDerror code beside the manualdelete-secretfallback; Starlight mirror regenerated.Note: this additive push dismisses the stale approval; re-request review as needed.