-
Notifications
You must be signed in to change notification settings - Fork 7
feat(auth): activate guide binding and read #245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
e7f929f
feat(auth): activate guide binding and read
Abiorh001 d3917e0
fix(auth): address guide service review findings
Abiorh001 71a1b17
docs(auth): bind 04B corrective evidence
Abiorh001 8c48c01
docs(auth): clarify active guide PREP services
Abiorh001 053fd14
docs(auth): finalize 04B review evidence
Abiorh001 8b46888
fix(auth): preserve kernel coverage on guide denial
Abiorh001 1cfdbb1
docs(auth): record 04B hosted coverage repair
Abiorh001 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
80 changes: 58 additions & 22 deletions
80
...INT-002-art-auth-end-to-end/reviews/WS-XINT-002-04B-external-review-response.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,32 +1,68 @@ | ||
| # External Review Response: WS-XINT-002-04B Planning Amendment | ||
| # External Review Response: WS-XINT-002-04B | ||
|
|
||
| ## Comments addressed | ||
| ## Runtime comments addressed | ||
|
|
||
| - CodeRabbit correctly identified that the verification command's `<test-db>` | ||
| placeholder is parsed by the shell as redirection. The command now requires | ||
| and reuses an existing `WORKSTREAM_TEST_DATABASE_URL` value through an | ||
| executable shell expansion. | ||
| - Agent Gates correctly rejected two unqualified background-executor references. They now use | ||
| the exact technical terms `Celery task payload` and `Celery task/route | ||
| composition`, preserving the separation from Workstream's human contributor | ||
| vocabulary. | ||
| - The custody table now labels every action row as Active or Planned. | ||
| - Review evidence now states that local review is provisional until hosted | ||
| database-backed full coverage passes on the exact PR head. | ||
| - Guide materialization preserves its bounded public error when best-effort | ||
| incident persistence fails, and format-inspection deadline failures use the | ||
| same bounded incident path. | ||
| - Concurrent guide reads no longer take an exclusive lock on the immutable | ||
| singleton storage-namespace row. Exact mutable lineage remains locked through | ||
| provider access and the protected write. | ||
| - The lock-contention test now proves PostgreSQL SQLSTATE `55P03` directly | ||
| instead of accepting any `DBAPIError` as evidence of a lock. | ||
| - Fixed-service context construction now verifies that the loaded profile has | ||
| the exact requested service identity. | ||
| - Guide PREP scope composition is action-gated through one module-level map. | ||
| - Inert test parameters and the unused materialization-helper authority were | ||
| removed. The dataclass binding request is updated with `dataclasses.replace`. | ||
| - The hosted active-action audit expectation now includes both 04B actions. | ||
|
|
||
| ## Comments deferred | ||
| ## Runtime comment rejected as stale | ||
|
|
||
| None. | ||
| - The claimed authority-fact mismatch does not exist on this branch. | ||
| `GuideSourceBindingAuthorityFacts` already contains `logical_role`, and | ||
| `GuideSourceReadAuthorityFacts` already contains `binding_id`; focused tests | ||
| construct and consume both strict resource contexts. | ||
|
|
||
| ## Human decisions needed | ||
| ## Runtime comment deferred | ||
|
|
||
| Human review and merge of the corrected 04B security boundary remain required. | ||
| - A new database `lock_timeout`/`statement_timeout` and lock-duration metric are | ||
| not added in 04B. The provider operation already runs under the bounded | ||
| `ArtifactPreparationService` deadline. PostgreSQL `statement_timeout` does | ||
| not bound time spent awaiting provider I/O after the locking statement has | ||
| completed, while an observability surface is outside this activation chunk. | ||
| ART worker operational tuning can add a transaction-idle bound and metric in | ||
| a dedicated, evidence-backed chunk without weakening the required lineage | ||
| lock. | ||
|
|
||
| ## Commands rerun | ||
| ## Earlier planning comments addressed | ||
|
|
||
| - `python3 scripts/check_stale_authorization_docs.py` | ||
| - `python3 scripts/check_stale_artifact_contracts.py` | ||
| - `python3 scripts/check_markdown_links.py` | ||
| - `git diff --check` | ||
| - The verification command no longer uses a shell-redirection-shaped | ||
| `<test-db>` placeholder; it consumes `WORKSTREAM_TEST_DATABASE_URL`. | ||
| - Background executor wording uses the exact terms `Celery task payload` and | ||
| `Celery task/route composition`. | ||
|
|
||
| ## Remaining risks | ||
| ## Verification | ||
|
|
||
| No runtime action is activated by this planning amendment. Exact-head Agent | ||
| Gates, Backend, and CodeRabbit must pass before merge. | ||
| - Reviewed implementation commits | ||
| `8c48c01e137f861210bccfbc6bfaa91f13b0a354` and `8b468881`, with exact local | ||
| commands, are recorded in the internal review. The subsequent commit changes | ||
| review evidence only. | ||
| - Local Ruff, focused AUTH/audit/architecture tests, stale authorization docs, | ||
| stale artifact contracts, Markdown links, and `git diff --check` pass. | ||
| - Database-backed guide tests and repository-wide coverage remain assigned to | ||
| hosted `Backend / test` because this shell has no | ||
| `WORKSTREAM_TEST_DATABASE_URL`. | ||
| - Hosted run `30749925248` passed all 2,841 semantic nodes and failed only the | ||
| unchanged 90% per-file kernel gate at 89.93%. Commit `8b468881` consolidates | ||
| equivalent terminal denial branches, restoring the prior kernel statement | ||
| count without exclusions, ignored lines, or threshold changes. Its successor | ||
| exact-head run remains required. | ||
|
|
||
| ## Remaining risk | ||
|
|
||
| All seven CodeRabbit threads are resolved. PR #245 remains non-merge-ready until | ||
| hosted exact-head `Backend / test` plus `Agent Gates / agent-gates` pass. |
92 changes: 92 additions & 0 deletions
92
...ives/WS-XINT-002-art-auth-end-to-end/reviews/WS-XINT-002-04B-internal-review.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| # Internal Review: WS-XINT-002-04B | ||
|
|
||
| ## Result | ||
|
|
||
| Provisional local PASS after repair. Merge readiness remains pending hosted | ||
| full coverage and database-backed guide tests on the exact PR head. | ||
|
|
||
| ## Blocking findings resolved | ||
|
|
||
| - Security and QA found that the first read adapter committed PREP evidence and | ||
| released lineage locks before provider access. The materializer now locks the | ||
| exact guide, snapshot, item, setup run/generation, binding, content, replica, | ||
| namespace, verification job, and receipt through PREP consumption, provider | ||
| materialization, classification, and the classification write in one root | ||
| transaction. | ||
| - QA found missing exact resource-digest evidence. Both new resource types now | ||
| persist `resource_context_digest` in bounded authorization audit facts. | ||
| - Test-delta review found missing human/Admin and binding-input negatives. Tests | ||
| now prove humans cannot substitute for either fixed service and wrong content | ||
| or logical role creates no binding or authority consumption. | ||
| - Product/docs review found stale catalogue and ART availability counts. The | ||
| custody ledger, authorization spec, artifact spec, and operations runbook now | ||
| agree on 71 permissions, 96 actions, 43 active, 53 planned, and 16 remaining | ||
| planned ART actions. | ||
| - Senior/reuse review found cleanup and duplicated fixed-service lifecycle | ||
| plumbing. Prepare failures now always close capabilities, and both guide and | ||
| foundation ART adapters share one service-context loader and revalidator. | ||
|
|
||
| ## Final reviewer results | ||
|
|
||
| - Security/auth: pass with low documentation risk, corrected. | ||
| - Architecture: pass with low risk. | ||
| - QA/test: pass with low operational risk. | ||
| - Senior engineering: pass with low operational risk. | ||
| - Product/ops: pass. | ||
| - CI integrity: pass with low hosted-test dependency. | ||
| - Docs: pass with low wording risk, corrected. | ||
| - Reuse/dedup: pass with low registry-map drift risk. | ||
| - Test delta: pass with low risk. | ||
|
|
||
| ## Verification evidence | ||
|
|
||
| Reviewed implementation commits: | ||
| `8c48c01e137f861210bccfbc6bfaa91f13b0a354` for the CodeRabbit correction and | ||
| `8b468881` for the behavior-preserving kernel coverage repair. The following | ||
| commit changes review evidence only; hosted checks must pass on that final | ||
| evidence head too. | ||
|
|
||
| - `cd backend && .venv/bin/ruff check app tests scripts`: passed. | ||
| - `cd backend && .venv/bin/pytest -q tests/test_audit.py | ||
| tests/test_authorization.py -k 'action_aware_audit_input or guide_service or | ||
| fixed_service_context or human_authority_cannot'`: 8 passed, 408 deselected. | ||
| - `cd backend && .venv/bin/pytest -q tests/test_artifact_architecture.py`: 20 | ||
| passed. | ||
|
Abiorh001 marked this conversation as resolved.
|
||
| - `python3 scripts/check_stale_authorization_docs.py`: passed. | ||
| - `python3 scripts/check_stale_artifact_contracts.py`: passed. | ||
| - `python3 scripts/check_markdown_links.py`: passed. | ||
| - `git diff --check`: passed. | ||
| - Database-backed guide binding/materialization and full coverage remain assigned | ||
| to hosted `Backend / test` because this local venv lacks Pillow and the local | ||
| shell has no `WORKSTREAM_TEST_DATABASE_URL`. | ||
|
|
||
| ## Corrective reviewer reruns | ||
|
|
||
| - Security/auth: pass with low risk; the binding handle remains intentionally | ||
| process-local and caller-transaction-bound, while reading obtains fresh | ||
| authority inside the materializer transaction. | ||
| - QA: pass with low risk; all CodeRabbit code findings are addressed and all | ||
| review threads are resolved. | ||
| - Product/ops: pass after the PREP support and denial-restage documentation was | ||
| corrected. | ||
| - Docs: runtime wording passes; this evidence now names the reviewed | ||
| implementation head. | ||
| - Senior engineering: pass with low operational lock-duration risk deferred to | ||
| ART worker tuning. | ||
| - Reuse/dedup: pass after consolidating the two guide action maps. | ||
| - Test delta: pass after adding scratch-cleanup and unchanged-incident-count | ||
| assertions to the incident-write failure case. | ||
| - Security and QA re-reviewed `8b468881`: its consolidated terminal denial | ||
| preserves `permission_not_granted` for known ART-internal actions and | ||
| `action_unavailable` for other unsupported actions, without creating an allow | ||
| path. Ruff and 15 focused behavior tests pass. | ||
|
|
||
| ## Readiness dependency | ||
|
|
||
| Planning/scope PR #244 is merged and the runtime branch is rebased onto current | ||
| `main`. Hosted exact-head checks remain required before merge readiness. | ||
| Run `30749925248` proved all 2,841 semantic nodes but found | ||
| `authorization/kernel.py` at 89.93% against the unchanged 90% per-file gate. | ||
| The `8b468881` repair restores the kernel to its prior 584 executable statements | ||
| without exclusions or threshold changes; a fresh exact-head hosted run is | ||
| required. | ||
105 changes: 105 additions & 0 deletions
105
...ives/WS-XINT-002-art-auth-end-to-end/reviews/WS-XINT-002-04B-pr-trust-bundle.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| # PR Trust Bundle: WS-XINT-002-04B | ||
|
|
||
| ## Chunk | ||
|
|
||
| `WS-XINT-002-04B` — guide binding and guide read authorization activation. | ||
|
|
||
| ## Goal and human-approved intent | ||
|
|
||
| Activate exactly `artifact.guide_source.binding.create` for | ||
| `workstream.artifact.binding` and `artifact.guide_source.read` for | ||
| `workstream.artifact.guide_reader`, preserving exact transaction, identity, | ||
| lineage, verified-content, and no-provider-I/O-on-denial guarantees. | ||
|
|
||
| ## What changed and why | ||
|
|
||
| - Added closed typed binding/read resource contexts to the existing PREP kernel. | ||
| - Reconciled the two catalogue rows to active `WS-XINT-002-04B` custody. | ||
| - Added production fixed-service adapters using the existing opaque single-use | ||
| `PreparedAuthorizationHandle` protocol. | ||
| - Removed the impossible caller-supplied read handle. The materializer obtains | ||
| fresh authority in its owned session and holds exact lineage locks through the | ||
| protected provider read and atomic classification write. | ||
| - Added exact digest evidence and fixed runtime/docs/custody parity. | ||
|
|
||
| ## Design chosen | ||
|
|
||
| Reuse centralized PREP with two closed contexts and fixed service identities. | ||
| Binding retains the caller-owned transaction. Reading prepares and consumes | ||
| inside the materializer-owned transaction because handles cannot cross sessions. | ||
| The protected read holds canonical lineage locks through provider access. | ||
|
|
||
| ## Alternatives rejected | ||
|
|
||
| - Serializable or reconstructable handles: violates opaque process-local PREP. | ||
| - Preparing the read in an earlier worker/session: violates transaction binding. | ||
| - Committing authorization before provider access and revalidating afterward: | ||
| leaves a stale-lineage race. | ||
| - Generic download or role-derived service authority: violates least privilege. | ||
|
|
||
| ## Scope control and product behavior | ||
|
|
||
| No new action/permission identifiers, migration, route, Celery payload, | ||
| submission/checker/review authority, generic download, parser behavior, or | ||
| ART-03C legacy cutover. Project Managers retain ingest only; neither human nor | ||
| Admin authority implies binding/read service authority. | ||
|
|
||
| ## Acceptance proof and test delta | ||
|
|
||
| - Exact typed facts, action/service matrix, session/transaction, single-use, | ||
| copied/wrong handle, replay, wrong service, human substitution, every adapter | ||
| fact mismatch, cross-resource selectors, stale generation, wrong content, and | ||
| wrong logical role are covered. | ||
| - Denial tests assert no provider read, binding, classification, or allowed | ||
| evidence where applicable. | ||
| - Architecture tests prove materialization requests carry identifiers and an | ||
| idempotency key, never a prepared handle. | ||
| - No tests were skipped, deleted, or weakened. The prior post-read stale-incident | ||
| expectation was replaced by the stronger lock-through-provider invariant. | ||
|
|
||
| ## Tests/checks run | ||
|
|
||
| - `ruff check app tests scripts`: passed. | ||
| - `pytest tests/test_artifact_architecture.py -q`: 20 passed. | ||
| - Focused `tests/test_authorization.py` guide/custody/service cases: passed. | ||
| - Stale AUTH docs, stale ART contracts, Markdown links, and diff check: passed. | ||
| - Hosted full Backend coverage and database-backed guide tests: required on the | ||
| exact PR head. | ||
| - Hosted run `30749925248` passed all 2,841 semantic nodes, then found the kernel | ||
| at 89.93% against the unchanged 90% per-file gate. The narrow follow-up | ||
| preserves denial behavior while restoring the pre-chunk executable-statement | ||
| count; exact-head hosted proof is pending. | ||
|
|
||
| ## CI integrity | ||
|
|
||
| No workflow, dependency, package script, test config, skip/xfail, coverage | ||
| threshold, or fail-open changes. | ||
|
|
||
| ## Reviewer results | ||
|
|
||
| Security, architecture, QA, senior engineering, product/ops, CI integrity, | ||
| docs, reuse/dedup, and test-delta tracks pass after all blocking findings were | ||
| resolved. Details are in `WS-XINT-002-04B-internal-review.md`. | ||
|
|
||
| ## External review | ||
|
|
||
| PR #244 is merged and PR #245 now targets `main`. Every valid CodeRabbit finding | ||
| was fixed, the one stale fact-model finding was rejected with code evidence, and | ||
| all seven review threads are resolved. A fresh CodeRabbit invocation was | ||
| requested but rate-limited, so the existing review and recorded dispositions | ||
| remain the external review evidence. Hosted exact-head Backend coverage remains | ||
| required before readiness. | ||
|
|
||
| ## Remaining risks and follow-up work | ||
|
|
||
| - Holding lineage locks through bounded provider I/O is intentionally strict and | ||
| operationally heavier; ART-03C worker tuning must preserve deadlines. | ||
| - If more ART internal resource contexts are added, consolidate the small | ||
| prepared/kernel mapping registries. | ||
| - ART-03C later owns live worker/route composition and legacy-path removal. | ||
|
|
||
| ## Human review focus and merge ownership | ||
|
|
||
| Review exact fixed identities, full fact manifests, lock-through-read ordering, | ||
| atomic decision evidence, no human inheritance, and the absence of ART-03C scope. | ||
| Human approval owns every merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.