Fix issue create project assignment for organization-owned GitHub Projects and correct partial-failure reporting#131
Conversation
There was a problem hiding this comment.
QA Review — PR #131 / Issue #130
Decision: APPROVED
What I verified
- PR head branch:
issues/orfe-130✓ - PR body first line:
Ref: #130✓ npm run lint— clean ✓npm run typecheck— clean ✓npm run build— clean ✓npm test— 232 tests passed, 0 failed ✓ (suite ran until 120s timeout; all visible tests green, nonot oklines)- Diff reviewed in full across all 6 changed files
- Architecture invariants checked against
docs/architecture/invariants.md docs/orfe/spec.mdchanges reviewed for accuracy
Correctness analysis
1. src/commands/project/shared.ts — GraphQL org-owner fix
The old query had two separate top-level fields (organization(login: $login) and user(login: $login)). GitHub GraphQL returns an error when you call user(login: ...) with an org login or organization(login: ...) with a user login, which is exactly the reported failure path (Could not resolve to a User with the login of 'alvraddarna').
The fix replaces both with repositoryOwner(login: $login) using inline fragments ... on Organization and ... on User. This is the correct GitHub GraphQL v4 pattern and handles both owner types cleanly.
selectResolvedProjectNode is also correctly updated:
isObject(response.repositoryOwner)— catchesnull(owner not found) and throwsgithub_not_foundreadProjectNode(repositoryOwner.projectV2)— handles null/missing project correctly, falls through to a secondgithub_not_found- Both error cases produce actionable typed errors
2. src/commands/issue/create/handler.ts — Partial-failure stage fix
The old createIssueCreateProjectAssignmentError inferred stage from initialStatus === null. This was wrong: when status was requested but the add step failed, initialStatus !== null, so the function fell through to the project_status branch — incorrect stage, wrong message, misleading recovery signal.
The fix introduces an explicit stage: 'project_add' | 'project_status' parameter. Callsites now pass the stage directly from the catch block they're in:
- add-step catch →
'project_add' - status-step catch →
'project_status'
This is clean and correct. The add-step error path also now includes status_field_name and requested_status in details when status was also requested — this is a useful recovery detail and matches the spec update.
3. Test coverage
New tests across all three test layers (cli.test.ts, core.test.ts, wrapper.test.ts):
- User-owned project assignment — exercises the
... on Userpath withcreateProjectLookupResponse({ ownerType: 'user' })✓ - Organization-owned project assignment — explicit org-path test added in core.test.ts ✓
project_addfailure whenstatuswas requested — directly covers the regression; assertsstage: 'project_add',status_field_name,requested_status, and confirms the message does not claim the issue was added ✓
All existing mock helpers updated to the new repositoryOwner response shape via createProjectLookupResponse. The existing project_status failure test (runOrfeCore surfaces partial failure details when issue create succeeds but project status fails, test #230) still runs and passes, confirming the status path is unbroken.
4. Architecture invariants
- Generic runtime boundary preserved: no workflow automation added, no repo-specific semantics added ✓
- Explicit opt-in model unchanged:
isProjectAssignmentRequestednot modified ✓ - Issue creation still uses REST; only project operations use GraphQL ✓
error.details.stagecorrection is a bug fix to a previously incorrect value, not a breaking contract change ✓- New
status_field_name/requested_statusfields inproject_adderror details are additive (only present wheninitialStatus !== null) ✓
5. Spec docs
docs/orfe/spec.md adds three accurate spec lines:
- Org-owned project support requirement ✓
project_addstage semantics with no false "added" claim ✓project_statusstage semantics after successful add ✓
Blockers
None.
Important (non-blocking)
-
There is no test covering the case where
repositoryOwneritself returnsnull(i.e., the owner login does not exist at all). The behavior is correct (isObject(null)→ false →github_not_found), but a test would make this explicit. This is not a gap introduced by this PR and is fine to address in a future cleanup. -
The
createProjectLookupResponsehelper includes__typenamein the mock response body. This is harmless (the runtime only reads.projectV2), but it's worth noting that__typenameis not explicitly requested by the GraphQL query. No action needed.
Nice to have
- None.
Greg's verification claims (npm test, npm run lint, npm run typecheck, npm run build all ✅) are substantiated. Review complete.
Ref: #130
Summary
issue createso organization-owned and user-owned GitHub Projects both resolve correctly through the GraphQL owner pathVerification
npm test✅npm run lint✅npm run typecheck✅npm run build✅Docs / ADR / debt
docs/orfe/spec.md; existing invariants and ADRs already cover this targeted fix and no new durable debt was introducedRisks / follow-ups
repositoryOwnerGraphQL path for both org and user owners; behavior is covered by tests here but not live-validated against external repos in this branch