fix(jira): validate repo onboarding at map time (#369)#374
Conversation
bgagent jira map (and linear onboard-project) accepted and persisted a project->repo mapping for a repo with no active Blueprint. Every label trigger then failed at task creation with 422 REPO_NOT_ONBOARDED, with no visible operator feedback. Add a shared checkRepoOnboarding helper (cli/src/repo-onboarding.ts) that reads the RepoTable via the RepoTableName stack output and reports whether the repo has a status='active' row -- mirroring the runtime gate in repo-config.ts. Both map commands now refuse (with actionable guidance) to persist a mapping for a non-onboarded repo, unless --skip-onboarding-check is passed. An inconclusive check (missing output / IAM gap) warns and proceeds rather than blocking a valid mapping. Linear's onboard-project gets the same guard since it shares the create-task-core onboarding gate. Docs: JIRA_SETUP_GUIDE.md and LINEAR_SETUP_GUIDE.md state the Blueprint-onboarding prerequisite and document --skip-onboarding-check; Starlight mirrors regenerated. Tests: new repo-onboarding.test.ts plus map-guard cases in jira/linear command tests (423 pass).
ayushtr-aws
left a comment
There was a problem hiding this comment.
Code Review: PR #374 — fix(jira): validate repo onboarding at map time (#369)
Overview
Adds a pre-write onboarding check to bgagent jira map and bgagent linear onboard-project. Before persisting a project→repo mapping, the command confirms the target repo has a status='active' row in the RepoTable — the same condition the runtime task-submit gate enforces (422 REPO_NOT_ONBOARDED). Inconclusive checks (no stack output, IAM gap) warn and proceed rather than block. Adds a shared cli/src/repo-onboarding.ts helper, a --skip-onboarding-check escape hatch, docs updates with regenerated Starlight mirrors, and tests.
Well-scoped, well-reasoned fix. "Fail loud at config time instead of silently at trigger time" is the right call, and the design decisions (Linear shares the gate; warn-and-proceed on inconclusive) are documented in both code and the PR body.
Correctness — verified against the codebase
- ✅ RepoTable key schema — partition key is
repo(cdk/src/constructs/repo-table.ts:65); the helper'sKey: { repo: opts.repo }matches. - ✅ Stack output key —
RepoTableNameexists (cdk/src/stacks/agent.ts:564). - ✅ Gate semantics —
lookupRepoinrepo-config.tstreats missing and status≠active as not-onboarded, active as onboarded. The CLI'smissing/inactive/onboardedverdicts mirror this exactly. - ✅ Dev-fallback parity — runtime returns
onboarded:truewhenREPO_TABLE_NAMEis unset; the CLI returnsunverifiable→warn-and-proceed when there's noRepoTableNameoutput. Same net effect (allow). Good alignment. - ✅ Error handling —
getStackOutputonly swallowsValidationError/"does not exist"; other AWS errors rethrow and surface asunverifiable.--repois validated before the check, so no malformed key reaches DynamoDB.
Suggestions (nits — none blocking)
- Duplicated
getStackOutput+ doubleDescribeStacks.repo-onboarding.tsintroduces agetStackOutputthat is a near-verbatim copy of the one already injira.ts:309(sameValidationError/"does not exist" handling). At runtime themapaction now callsDescribeStackstwice on the same stack — once forJiraProjectMappingTableName, once insidecheckRepoOnboardingforRepoTableName. Consider exporting the existing helper (or fetching all outputs once and passing them in) to remove both the code duplication and the redundant API call. Minor latency/cost. - New IAM requirement.
mapnow needsdynamodb:GetItemon the RepoTable in addition to its prior CFN-read + mapping-table-write. Theunverifiable→warn-and-proceed path degrades gracefully when the grant is missing, which is the right behavior, and the docs already note "your IAM principal cannot read the table." Fully covered — just flagging the expanded permission surface.
Test coverage — strong
repo-onboarding.test.ts exercises all four verdicts plus both rethrow/IAM-error paths. Command tests assert GetItem-then-PutItem ordering, that --skip-onboarding-check avoids the read, and that the guidance contains REPO_NOT_ONBOARDED. PR reports 423 tests pass with the coverage threshold met.
Security
No injection surface (key is the pre-validated owner/repo); read-only DynamoDB/CFN access; errors surface as informative unverifiable strings, not silent empties or leaked secrets. Author flagged they couldn't run the masking semgrep locally — the catch blocks return detail messages, not credentials, so no masking concern.
Verdict
Approve. Faithful to the runtime gate, good tests and docs, sound design. The two nits (collapse the duplicated getStackOutput / redundant DescribeStacks; note the expanded IAM need) are optional polish, not merge blockers.
Summary
Closes #369.
bgagent jira map <cloud-id> <PROJECT-KEY> --repo owner/repoaccepted and persisted a project→repo mapping for a repo with no active Blueprint. Every subsequent label trigger then failed at task creation with422 REPO_NOT_ONBOARDED— and, because the failure-feedback comment also fails, the operator saw nothing. There was no signal atmaptime that the repo would never work.This adds an onboarding check at
map/onboard-projecttime so the misconfiguration surfaces immediately, with actionable guidance.Changes
cli/src/repo-onboarding.ts:checkRepoOnboarding()reads theRepoTableNamestack output and does aGetItemon the RepoTable keyed byowner/repo, mirroring the runtime gate's exact semantics (repo-config.tslookupRepo). Returnsonboarded/not-onboarded(missing vs inactive) /unverifiable.notOnboardedGuidance()builds the shared remediation message (deploy a Blueprint → re-run; mentions the escape hatch).jira map,linear onboard-project): validate before thePutItem; fail loudly when not onboarded. Added--skip-onboarding-checkfor the "deploying the Blueprint momentarily" case.JIRA_SETUP_GUIDE.mdandLINEAR_SETUP_GUIDE.mdnow state the Blueprint-onboarding prerequisite (with the422 REPO_NOT_ONBOARDEDexplanation + onboarding steps linking to QUICK_START) and document the new flag. Starlight mirrors regenerated viasync-starlight.mjs.repo-onboarding.test.ts(all verdicts + error/rethrow paths) plus map-guard cases injira.test.tsandlinear.test.ts.Acceptance criteria
bgagent jira maprejects a non-onboarded repo with actionable guidance instead of silently persisting a dead mapping.cli/.onboard-projectgets the same guard — it shares thecreate-task-coreonboarding gate. Documented in code comments and both guides.Testing
cd cli && mise run build→ 423 tests pass, eslint clean, coverage threshold met.cd docs && node scripts/sync-starlight.mjs→ stable (idempotent on re-run).Notes
--no-verify) on commit/push becausecore.hooksPathis set in this environment, which blocksprek install. CI hooks still run.unverifiableresults surfaced to the operator (not silent empty defaults), andgetStackOutputrethrows unexpected errors — consistent with the existing pattern injira.ts/linear.ts.