feat(access): add short-lived opaque grant domain - #506
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough프레임워크 중립적인 단기 접근 권한 도메인을 추가했다. Changes단기 접근 권한 도메인
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟠 High · up to The new grant domain can accept an invalid consume result as successful because it does not verify the one-time transition, allowing a repeated redemption to be authorized. The PR is not merge-ready until the consumed-state checks and corresponding test are fixed. Sequence Diagram(s)sequenceDiagram
participant Client
participant AccessGrantService
participant ProjectAuthorizationPort
participant AccessGrantRepository
participant MembershipRevocationPort
participant AccessGrantAuditSink
Client->>AccessGrantService: mint 요청
AccessGrantService->>ProjectAuthorizationPort: 프로젝트 발급 권한 확인
AccessGrantService->>AccessGrantRepository: grant 해시 저장
AccessGrantService->>AccessGrantAuditSink: 발급 감사 기록
Client->>AccessGrantService: redeem 요청
AccessGrantService->>AccessGrantRepository: 해시로 grant 조회
AccessGrantService->>MembershipRevocationPort: 멤버십 재검증
AccessGrantService->>AccessGrantRepository: 원자적 1회 소비
AccessGrantService->>AccessGrantService: 소비 결과 바인딩 재검증
AccessGrantService->>AccessGrantAuditSink: 소비 감사 기록
AccessGrantService-->>Client: 검증된 grant 결과
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
@opencode-agent review Please submit a formal review for exact current head |
6f2cbf3 to
59c1008
Compare
Rebase onto current #506 dropped the return-boundary entries from package.json and the coverage-script contract. Restore them so the isolated atomic-return regression stays in test:unit and c8. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head mergeability evidence and changed-file flow before approval, then found merge conflicts on the affected path.
Findings
1. HIGH Merge Conflict Guidance - Resolve the PR branch against the latest base branch
- Problem: GitHub reports mergeStateStatus
DIRTYfor this pull request. - Root cause: Branch
feat/access-grant-domain-413cannot be merged cleanly intodevelop; the changed-file flow below shows which review/runtime path is blocked by the conflict. - Fix: Merge or rebase the latest
developintofeat/access-grant-domain-413, resolve conflict markers in the PR branch, rerun the focused checks, and push the same branch. - Repair commands:
gh pr checkout 506 --repo ContextualWisdomLab/scopeweave
git fetch origin develop
git merge --no-ff origin/develop # or: git rebase origin/develop
git status --short
# resolve files, then git add <resolved-files>
# merge path: git commit
# rebase path: git rebase --continue
git push origin HEAD:feat/access-grant-domain-413
# rebase path only: git push --force-with-lease origin HEAD:feat/access-grant-domain-413- Regression test: Keep OpenCode approval gated on mergeability so model-output failures cannot approve a conflicted PR.
Merge Conflict Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> Conflict["Merge conflict blocks this path"]
Conflict --> V1["required checks"]
Evidence --> S2["Docs: short-lived-access-grant-domain.md"]
S2 --> I2["operator or user guidance"]
I2 --> Conflict["Merge conflict blocks this path"]
Conflict --> V2["docs review"]
Evidence --> S3["Test (3 files)"]
S3 --> I3["regression suite"]
I3 --> Conflict["Merge conflict blocks this path"]
Conflict --> V3["targeted test run"]
- Result: REQUEST_CHANGES
- Reason: mergeStateStatus is
DIRTY; mergeable isCONFLICTING. - Head SHA:
59c10080696972b2955b229edf37a4e9a649dbbc - Workflow run: 32188776233
- Workflow attempt: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> Conflict["Merge conflict blocks this path"]
Conflict --> V1["required checks"]
Evidence --> S2["Docs: short-lived-access-grant-domain.md"]
S2 --> I2["operator or user guidance"]
I2 --> Conflict["Merge conflict blocks this path"]
Conflict --> V2["docs review"]
Evidence --> S3["Test (3 files)"]
S3 --> I3["regression suite"]
I3 --> Conflict["Merge conflict blocks this path"]
Conflict --> V3["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode reviewed the current-head mergeability evidence and changed-file flow before approval, then found merge conflicts on the affected path. Findings1. HIGH Merge Conflict Guidance - Resolve the PR branch against the latest base branch
gh pr checkout 506 --repo ContextualWisdomLab/scopeweave
git fetch origin develop
git merge --no-ff origin/develop # or: git rebase origin/develop
git status --short
# resolve files, then git add <resolved-files>
# merge path: git commit
# rebase path: git rebase --continue
git push origin HEAD:feat/access-grant-domain-413
# rebase path only: git push --force-with-lease origin HEAD:feat/access-grant-domain-413
Merge Conflict Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> Conflict["Merge conflict blocks this path"]
Conflict --> V1["required checks"]
Evidence --> S2["Docs: short-lived-access-grant-domain.md"]
S2 --> I2["operator or user guidance"]
I2 --> Conflict["Merge conflict blocks this path"]
Conflict --> V2["docs review"]
Evidence --> S3["Test (3 files)"]
S3 --> I3["regression suite"]
I3 --> Conflict["Merge conflict blocks this path"]
Conflict --> V3["targeted test run"]
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> Conflict["Merge conflict blocks this path"]
Conflict --> V1["required checks"]
Evidence --> S2["Docs: short-lived-access-grant-domain.md"]
S2 --> I2["operator or user guidance"]
I2 --> Conflict["Merge conflict blocks this path"]
Conflict --> V2["docs review"]
Evidence --> S3["Test (3 files)"]
S3 --> I3["regression suite"]
I3 --> Conflict["Merge conflict blocks this path"]
Conflict --> V3["targeted test run"]
Merge Conflict Guidance
gh pr checkout 506 --repo ContextualWisdomLab/scopeweave
git fetch origin develop
git merge --no-ff origin/develop # or: git rebase origin/develop
git status --short
# resolve files, then git add <resolved-files>
# merge path: git commit
# rebase path: git rebase --continue
git push origin HEAD:feat/access-grant-domain-413
# rebase path only: git push --force-with-lease origin HEAD:feat/access-grant-domain-413 |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
opencode-agent
Bot
added
priority: medium
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This changes-requested verdict is anchored to predecessor head 59c1008 and solely reported a then-live merge conflict. Current exact head f3ae1f1 is based on protected develop@2c328875e00e86537df3e965170be80532571cad and GitHub currently reports the PR mergeable. The historical conflict finding is therefore stale; dismissal removes predecessor review state without manufacturing approval. Current-head checks, findings, and independent approval remain required.
seonghobae
added
the
enhancement
ChatGPT Codex Connector
1 participant
Buyer and security impact
Issue #413 identifies a material enterprise-security gap: ScopeWeave still carries the general session JWT in URL query parameters for browser mechanisms that cannot reliably supply an
Authorizationheader. This bounded root slice establishes the framework-neutral security domain required to replace that broad URL credential with short-lived, purpose-bound opaque grants without prematurely changing protected runtime transports.This PR does not close #413. It adds no route, database migration, calendar subscription, SSE client migration, attachment-view migration, or UI. Existing protected behavior remains unchanged until those follow-up slices are independently implemented and verified.
Exact current scope
develop@2c328875e00e86537df3e965170be80532571cad;f3ae1f1f4ee66024776507d75f8fb752b2bf748c;develop@2c328875...;server/access_grant_domain.mjs;tests/unit/access-grant-domain.test.mjs;tests/unit/access-grant-domain-edge.test.mjs;tests/unit/coverage-script-contract.test.mjs;package.json;docs/doctoring/short-lived-access-grant-domain.md;CHANGELOG.md.The reconciliation preserves protected application behavior outside those seven paths. No existing auth/session implementation, Hono route, database schema, Clearfolio, attachment refresh, billing, workflow, dependency, lockfile, or protected-branch policy is changed by the effective diff.
Any contributor-head, protected-base, review, or required-gate movement invalidates revision-sensitive evidence until freshly refetched and reconciled.
Security and domain contract
grant_idcorrelation identifier, never derived from the secret or token hash;stream→scopeweave:stream,attachment_view→scopeweave:attachment-view;consumeGrantAtomically(...), which production adapters must validate atomically;now >= expires_at);used_at_ms, so a faulty adapter cannot turn an unconsumed record into a successful principal;The module is independent of Hono, SQLite, Clearfolio, and browser code and accepts explicit repository, clock, random-source, audit, project-authorization, and membership-revocation ports.
TDD and review-driven hardening
The first contract commit imported an absent
server/access_grant_domain.mjs; focused Node execution failed RED withERR_MODULE_NOT_FOUNDbefore production implementation existed.CodeRabbit identified predecessor-head concerns around independent grant correlation identity, atomic membership authority at consume, audit-failure semantics after durable transitions, edge-test coverage registration, and consumed-state validation. Current source addresses those findings and their enumerated threads are resolved. Membership-version hardening rejects missing, negative/NaN, blank, control-character, overlength, and object versions.
Focused tests cover hash-only persistence, secret-free audit events, purpose/audience/resource isolation, TTL boundaries, inaccessible projects, membership revocation and its check/consume race, audit-sink failure after durable transitions, malformed/unknown secrets, replay rejection, concurrent two-consumer one-time use, consumed-state adapter failures, and membership-version adapter failures.
The dismissed OpenCode
CHANGES_REQUESTEDreview is predecessor-head evidence on59c10080696972b2955b229edf37a4e9a649dbbcand reported the then-live merge conflict. Current head is mechanically mergeable against the live protected base, so that predecessor verdict does not transfer. Current Devin unresolved threads are informational observations about the explicit repository-port contract, fail-closed authorization error mapping, grant-bound membership inputs, and ordering assumptions; none presently demonstrates a new current-head product defect and they remain visible rather than being status-cleaned.Current exact-head evidence boundary
All six repository workflow runs currently associated with exact contributor head
f3ae1f1f4ee66024776507d75f8fb752b2bf748care terminal GitHub-success:32562230719— success;32562230702— success;32562230689— success;32562230686— success;32562230943— success; and32562230734— success.Those labels are regression evidence, not immutable contributor-head merge authority while the known revision-evidence defects remain unresolved. ScopeWeave #523 owns repository-native exact-head Server Tests/coverage integrity.
ContextualWisdomLab/.github#1222owns the reusable required SAST/Security exact-head checkout repair through that repository's existing dedicated writer/control path. No currentCodeQL Requiredworkflow run was returned for this contributor head, so required CodeQL evidence remains absent/non-passing rather than implicitly successful.The organization-central evidence defects tracked by
.github#1002(required review-verdict evidence) and.github#955(provider-unavailability false-green behavior) also remain separate owner boundaries. ScopeWeave does not add leaf workarounds or promote synthetic/status-only/model-only evidence.There is no qualifying independent current-head/last-push approval. Do not self-approve or manufacture one.
Standards, rollback, and integration boundary
docs/doctoring/short-lived-access-grant-domain.mdrecords active-PR versus protected-shipped truth, threat and port boundaries, membership-race and audit-durability contracts, rollback, and APA 7 references to RFC 6750, RFC 9700/BCP 240, and RFC 8725.Rollback removes the domain module, focused tests, coverage registrations, doctoring record, and changelog entries together. Because this slice changes no route or persisted schema, rollback requires no data migration.
This PR does not close #413. SQLite/PostgreSQL adapters, authenticated exchange routes, stream/attachment client migrations, revocation hooks, calendar-subscription lifecycle, Figma/UI flows, browser acceptance, and eventual rejection of full session JWT query transport remain separate bounded work. The current calendar domain child is #539, its persistence child is #541, and runtime integration continues in #549.
Merge gate
Do not merge or enable auto-merge until this unchanged exact contributor head remains reconciled to freshly resolved protected
develop, the repository and central exact-head evidence controls are protected-shipped where applicable, every applicable repository/organization CI/browser/owned statement-branch-function-line coverage/docstring/CodeQL/SAST/security/dependency/supply-chain/package/provenance/required-review gate is substantively terminal-passing under corrected evidence contracts, valid unresolved findings are zero, and the qualifying independent current-head/last-push approval required by live governance genuinely exists.Pending, queued, skipped-required, cancelled, absent, neutral-required, failed, stale, predecessor, synthetic-only, status-only, author-only, model-only, rate-limited, or infrastructure-only evidence is non-passing.
Refs #413