Deepen the observable Workspace donor study - #16
Conversation
📝 WalkthroughWalkthroughAdds a v2 Workspace operations donor study with candidate artifacts, a strict deterministic validator, adversarial regression tests, and a GitHub Actions workflow that binds validation results to the exact PR head and retained evidence artifacts. ChangesWorkspace operations study
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant Repository
participant RegressionTests
participant Validator
participant EvidenceStep
participant ArtifactStore
GitHubActions->>Repository: checkout exact PR head SHA
GitHubActions->>RegressionTests: run 26 regression cases
GitHubActions->>Validator: generate validation report
Validator-->>EvidenceStep: validation JSON
EvidenceStep->>EvidenceStep: verify status, counts, flags, and digest
EvidenceStep->>ArtifactStore: upload validation and regression evidence
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
|
Direct review completed on exact head
No direct-review blocker found. |
jaydumisuni
left a comment
jaydumisuni
left a comment
There was a problem hiding this comment.
Direct review: exact nine-file boundary matches the declared donor/profile/test/workflow scope. All 26 cases and eleven exact-head workflows passed. No contract, runtime, P01 or ADR authority change is present. No blocking finding.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
design/donors/openai-chatgpt-workspace-deep-study.md (1)
45-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the study’s evidence window.
The supplement says “Official documentation inspected” and studies the “current Workspace,” but does not record an observation or retrieval date. The existing donor has explicit dates, while the referenced official Library and Apps pages are independently updated over time. (help.openai.com) Add an
Observed/Sources retrieveddate and, ideally, retained snapshots or hashes for the cited pages and direct-observation run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@design/donors/openai-chatgpt-workspace-deep-study.md` around lines 45 - 60, Update the “Public and observable sources” section to record the study’s evidence window with an explicit Observed or Sources retrieved date. Include the retrieval/observation date for the cited official documentation and direct-observation run, and add retained snapshots or hashes for those sources where available.Source: MCP tools
tools/test_check_workspace_operations_donor_v2.py (2)
14-25: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFull repo copied on every test (26x).
setUp()runsshutil.copytreeof the entire repository for each of the 26 tests, including.gitand any other unrelated directories. Given the catalog alone reports 346 schemas / 99 state machines, this is meaningful, avoidable I/O overhead in CI.♻️ Quick win: exclude `.git` from the copy
def setUp(self) -> None: self.tempdir = tempfile.TemporaryDirectory() self.root = Path(self.tempdir.name) / "repo" - shutil.copytree(self.source_root, self.root) + shutil.copytree( + self.source_root, + self.root, + ignore=shutil.ignore_patterns(".git"), + )For a larger win, consider a
setUpClass-built shared base copy plus per-test overlay of only the files each test mutates, sincevalidate()only ever reads a fixed, known set of paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/test_check_workspace_operations_donor_v2.py` around lines 14 - 25, Reduce test setup I/O in WorkspaceOperationsStudyTests by excluding the .git directory from the shutil.copytree call in setUp. Preserve the existing temporary repository behavior and cleanup while avoiding unnecessary repository metadata and unrelated files; a shared base copy and per-test overlays are optional and not required.
149-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick wintest_16/17/18 don't reach the check they're meant to exercise.
Each of these mutates a single gap mapping's
classificationand adjusts thesummarycounts, but the adjusted summary (e.g.covered_by_neutral_substrate: 15or17) no longer equals the validator's hardcodedEXPECTED_SUMMARY(16/6/0/6). That trips the generic "gap map summary mismatch" check first, so the tests never actually reach:
- the dedicated
candidate_core_extensioncount guard (test_16's intent), or- the "semantic function is not caller-owned" per-capability guard (test_17/test_18's intent).
assertRaises(StudyError)still passes, but a bug specifically in the per-capability caller-ownership check would go undetected. To exercise the intended branch, compensate with a second mapping swap so the aggregatesummarystill matchesEXPECTED_SUMMARYwhile the targeted capability's classification is still wrong, e.g.:def test_17_context_mapping_to_core_fails(self) -> None: path = "design/candidates/workspace-operations-gap-map-v2.json" data = self.load(path) item = next(x for x in data["mappings"] if x["capability"] == "context selection and relevance") item["classification"] = "covered_by_neutral_substrate" # compensate so aggregate summary still matches EXPECTED_SUMMARY other = next( x for x in data["mappings"] if x["classification"] == "covered_by_neutral_substrate" and x["capability"] != "context selection and relevance" ) other["classification"] = "caller_application_composition" self.save(path, data) self.assert_invalid()Apply the analogous compensating swap to test_16 and test_18.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/test_check_workspace_operations_donor_v2.py` around lines 149 - 176, Update test_16_gap_extension_fails, test_17_context_mapping_to_core_fails, and test_18_approval_mapping_to_core_fails to compensate each targeted classification mutation with a swap on a different mapping, preserving EXPECTED_SUMMARY. Keep each target capability’s invalid classification intact so assert_invalid reaches the intended candidate_core_extension or caller-ownership validation branch rather than failing on summary mismatch.tools/check_workspace_operations_donor_v2.py (1)
118-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAmbiguous Unicode dash in required literal match.
"no reason to reopen WP01–WP14"uses an EN DASH rather than a hyphen. Since this is matched via exact substringrequire()against donor prose, a future edit that "normalizes" this character (e.g., an editor auto-correct, or copy/paste from a different source) would silently break the check with a generic "missing required text" error, and the visual similarity makes the root cause hard to spot.♻️ Optional: make the literal unambiguous or comment it
- "no reason to reopen WP01–WP14", + "no reason to reopen WP01–WP14", # NOTE: EN DASH (U+2013), intentional to match donor markdown verbatim🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/check_workspace_operations_donor_v2.py` around lines 118 - 130, Update the required literal in the token list used by the workspace donor check so the WP01–WP14 range uses an unambiguous, normalization-resistant representation; preserve the exact intended matching behavior in require() and make the character choice explicit if the literal must remain unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/phase0c-workspace-deep-study-v2.yml:
- Around line 5-16: Align the workflow path filter with the validator’s actual
inputs: add README.md so README-only changes rerun the check, and either update
the validator to load and hash PTAH-WORKSPACE-OPERATIONS-PROFILE-V2.md or remove
that Markdown profile from the workflow’s paths. Keep the workflow and validator
validation scope consistent.
- Around line 43-47: Update the “Run 26 valid and adversarial cases” step and
the evidence-generation step to verify that the executed unittest summary
reports exactly 26 tests with zero skipped cases before recording
regression_case_count: 26. Capture and inspect the test output or emit a
machine-readable count, and fail the workflow when the count or skipped-case
condition does not match.
- Around line 26-30: Update the actions/checkout@v4 configuration in “Check out
exact pull-request head” to set persist-credentials to false, while preserving
the existing ref and fetch-depth settings.
In `@design/donors/openai-chatgpt-workspace-deep-study.md`:
- Around line 161-164: Define an explicit unknown-effect/indeterminate state for
external-side-effect Attempts and update the retry semantics so a missing
completion response cannot blindly create a new Attempt. Require either an
idempotency key/provider deduplication mechanism or verify-before-retry
reconciliation, and update the profile, validator, and retry fixtures to assert
this behavior while preserving failed Attempt and partial Artifact history.
---
Nitpick comments:
In `@design/donors/openai-chatgpt-workspace-deep-study.md`:
- Around line 45-60: Update the “Public and observable sources” section to
record the study’s evidence window with an explicit Observed or Sources
retrieved date. Include the retrieval/observation date for the cited official
documentation and direct-observation run, and add retained snapshots or hashes
for those sources where available.
In `@tools/check_workspace_operations_donor_v2.py`:
- Around line 118-130: Update the required literal in the token list used by the
workspace donor check so the WP01–WP14 range uses an unambiguous,
normalization-resistant representation; preserve the exact intended matching
behavior in require() and make the character choice explicit if the literal must
remain unchanged.
In `@tools/test_check_workspace_operations_donor_v2.py`:
- Around line 14-25: Reduce test setup I/O in WorkspaceOperationsStudyTests by
excluding the .git directory from the shutil.copytree call in setUp. Preserve
the existing temporary repository behavior and cleanup while avoiding
unnecessary repository metadata and unrelated files; a shared base copy and
per-test overlays are optional and not required.
- Around line 149-176: Update test_16_gap_extension_fails,
test_17_context_mapping_to_core_fails, and
test_18_approval_mapping_to_core_fails to compensate each targeted
classification mutation with a swap on a different mapping, preserving
EXPECTED_SUMMARY. Keep each target capability’s invalid classification intact so
assert_invalid reaches the intended candidate_core_extension or caller-ownership
validation branch rather than failing on summary mismatch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cc44da6e-68d4-44e0-94ae-29a0dafb6bd3
📒 Files selected for processing (9)
.github/workflows/phase0c-workspace-deep-study-v2.ymldesign/candidates/PTAH-WORKSPACE-OPERATIONS-PROFILE-V2.mddesign/candidates/fixtures/workspace-operations-fixtures-v2.jsondesign/candidates/workspace-operations-gap-map-v2.jsondesign/candidates/workspace-operations-profile-v2.jsondesign/donors/openai-chatgpt-projects-work.mddesign/donors/openai-chatgpt-workspace-deep-study.mdtools/check_workspace_operations_donor_v2.pytools/test_check_workspace_operations_donor_v2.py
| paths: | ||
| - "design/donors/openai-chatgpt-workspace-deep-study.md" | ||
| - "design/candidates/PTAH-WORKSPACE-OPERATIONS-PROFILE-V2.md" | ||
| - "design/candidates/workspace-operations-profile-v2.json" | ||
| - "design/candidates/workspace-operations-gap-map-v2.json" | ||
| - "design/candidates/fixtures/workspace-operations-fixtures-v2.json" | ||
| - "tools/check_workspace_operations_donor_v2.py" | ||
| - "tools/test_check_workspace_operations_donor_v2.py" | ||
| - ".github/workflows/phase0c-workspace-deep-study-v2.yml" | ||
| - "design/donors/openai-chatgpt-projects-work.md" | ||
| - "design/candidates/ai-project-workspace-profile.json" | ||
| - "contracts/generated/catalog-index.json" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the trigger paths with the validator’s actual inputs.
The validator reads README.md, but this path filter omits it, so a README-only boundary change can merge without rerunning this check. Conversely, design/candidates/PTAH-WORKSPACE-OPERATIONS-PROFILE-V2.md triggers the workflow but is not loaded or hashed by the validator, allowing the human-readable profile to drift from the JSON contract while CI remains green. Add README.md and validate/bind the Markdown profile, or remove the latter from the claimed validation scope.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/phase0c-workspace-deep-study-v2.yml around lines 5 - 16,
Align the workflow path filter with the validator’s actual inputs: add README.md
so README-only changes rerun the check, and either update the validator to load
and hash PTAH-WORKSPACE-OPERATIONS-PROFILE-V2.md or remove that Markdown profile
from the workflow’s paths. Keep the workflow and validator validation scope
consistent.
| - name: Check out exact pull-request head | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.sha || github.sha }} | ||
| fetch-depth: 1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow file =="
if [ -f .github/workflows/phase0c-workspace-deep-study-v2.yml ]; then
nl -ba .github/workflows/phase0c-workspace-deep-study-v2.yml | sed -n '1,220p'
else
echo "workflow not found"
fi
echo "== checkout token references =="
rg -n "github-token|GITHUB_TOKEN|git config|persist-credentials|upload-artifact|artifact|validator-output|github.event.pull_request.head.sha|pull_request|pull_request_target" .github/workflows/phase0c-workspace-deep-study-v2.yml || true
echo "== workflow files mentioning checkout@v4 persist =="
rg -n "actions/checkout@v4|persist-credentials" .github/workflows || trueRepository: jaydumisuni/Ptah-space
Length of output: 220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow file =="
if [ -f .github/workflows/phase0c-workspace-deep-study-v2.yml ]; then
sed -n '1,220p' .github/workflows/phase0c-workspace-deep-study-v2.yml | awk '{printf "%6d %s\n", NR, $0}'
else
echo "workflow not found"
fi
echo "== checkout token references =="
grep -nE "github-token|GITHUB_TOKEN|git config|persist-credentials|upload-artifact|artifact|validator-output|github.event.pull_request.head.sha|pull_request|pull_request_target" .github/workflows/phase0c-workspace-deep-study-v2.yml || true
echo "== workflow files mentioning checkout@v4 persist =="
grep -nE "actions/checkout@v4|persist-credentials" .github/workflows || true
echo "== changed workflow status =="
git diff --stat -- .github/workflows/phase0c-workspace-deep-study-v2.yml || trueRepository: jaydumisuni/Ptah-space
Length of output: 5799
🌐 Web query:
actions/checkout v4 persist-credentials default true GitHub token .git/config
💡 Result:
In actions/checkout v4, the persist-credentials input defaults to true [1][2]. When this setting is enabled, the action configures the Git access token (typically the GITHUB_TOKEN) into the local repository's.git/config file, allowing subsequent steps in your workflow to run authenticated Git commands automatically [3][4][5]. Specifically, in v4, the credential is written directly into the.git/config file as an http.https://github.com/.extraheader value [5]. Because the token is stored within the.git directory, it is included if the workspace (including the.git folder) is uploaded as a build artifact, which creates a potential credential exposure risk [5]. The token is removed by the action during the post-job cleanup phase [3][4]. If your workflow does not require subsequent authenticated Git commands, you can opt out of this behavior by setting persist-credentials to false [3][5]: - uses: actions/checkout@v4 with: persist-credentials: false Setting this to false prevents the credential from being written to the configuration file, thereby reducing the surface area for potential leaks [5].
Citations:
- 1: https://github.com/actions/checkout/blob/v4/action.yml
- 2: https://github.com/actions/checkout/blob/v4.1.1/action.yml
- 3: https://github.com/actions/checkout/blob/v4/README.md
- 4: https://github.com/actions/checkout
- 5: Set
persist-credentials: falseon checkout steps cli/go-gh#225
Disable checkout credential persistence before executing PR code.
actions/checkout@v4 persists the workflow token in .git/config by default, and this workflow subsequently runs PR-controlled Python and uploads validator-output.txt, exact-head.txt, exact-head-evidence.json, workspace-deep-study-validation.json, and regressions.txt. A malicious change in the checked-out PR branch can access that token before job cleanup and expose it in logs or artifacts. Set persist-credentials: false; no later authenticated Git operation is needed.
Proposed fix
with:
ref: $${{ github.event.pull_request.head.sha || github.sha }}
fetch-depth: 1
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Check out exact pull-request head | |
| uses: actions/checkout@v4 | |
| with: | |
| ref: ${{ github.event.pull_request.head.sha || github.sha }} | |
| fetch-depth: 1 | |
| - name: Check out exact pull-request head | |
| uses: actions/checkout@v4 | |
| with: | |
| ref: ${{ github.event.pull_request.head.sha || github.sha }} | |
| fetch-depth: 1 | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 26-30: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/phase0c-workspace-deep-study-v2.yml around lines 26 - 30,
Update the actions/checkout@v4 configuration in “Check out exact pull-request
head” to set persist-credentials to false, while preserving the existing ref and
fetch-depth settings.
Source: Linters/SAST tools
| - name: Run 26 valid and adversarial cases | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
| python3 tools/test_check_workspace_operations_donor_v2.py -v 2>&1 | tee regressions.txt |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Bind the reported regression count to the executed tests.
The evidence hard-codes regression_case_count: 26, but the workflow never verifies that 26 tests actually ran or that none were skipped. A reduced or skipped suite could still exit successfully and produce false 26/26 evidence. Assert the unittest summary and zero skipped cases, or emit and consume a machine-readable count before writing the evidence file.
Proposed minimum check
python3 tools/test_check_workspace_operations_donor_v2.py -v 2>&1 | tee regressions.txt
+ grep -Eq '^Ran 26 tests in ' regressions.txt
+ ! grep -Eq '^OK \(skipped=' regressions.txtAlso applies to: 87-89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/phase0c-workspace-deep-study-v2.yml around lines 43 - 47,
Update the “Run 26 valid and adversarial cases” step and the evidence-generation
step to verify that the executed unittest summary reports exactly 26 tests with
zero skipped cases before recording regression_case_count: 26. Capture and
inspect the test output or emit a machine-readable count, and fail the workflow
when the count or skipped-case condition does not match.
| - truthful estimate only when one is available. | ||
|
|
||
| Failure after partial production must retain both the failed Attempt and every valid partial Artifact. Retry creates a new Attempt linked to the failed one; it does not erase history. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define safe retry semantics for unknown external effects.
When an external-side-effect operation receives no completion response, creating a new Attempt can duplicate a non-idempotent write if the first request already committed. The profile has no canonical unknown_effect/indeterminate state, and the retry fixture permits not_run_or_unknown_effect. Require an idempotency key/provider deduplication mechanism or verify-before-retry reconciliation, then make the profile, validator, and fixtures assert that behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design/donors/openai-chatgpt-workspace-deep-study.md` around lines 161 - 164,
Define an explicit unknown-effect/indeterminate state for external-side-effect
Attempts and update the retry semantics so a missing completion response cannot
blindly create a new Attempt. Require either an idempotency key/provider
deduplication mechanism or verify-before-retry reconciliation, and update the
profile, validator, and retry fixtures to assert this behavior while preserving
failed Attempt and partial Artifact history.
1 participant
Purpose
Study the current ChatGPT Workspace more deeply as an observable behavioural donor, using ten primary specialist lanes and ten independent verifier lanes.
This does not use or require OpenAI private source code. It borrows interaction, operation and reliability patterns only.
Improvements retained
Fixed Ptah boundary
Exact-head proof
Exact head:
bf4ae98b9d492ad688644fd6a330aaf435ac70c130087967851;8594496859;sha256:aea4fde3f600a6e4c3fc2f6ff3614918a5f714c6f8ebbf6ab3fb3cb29ccaf12b;329262e7bb12e0841f1884664e713ab8e55a58e45a2430d4abf77ccdde65ecbe;The backend-signature lane initially retained two upstream HTTP 504 failures for the locked libarchive detached-signature URL. A third independent attempt passed without changing source or weakening proof.
Non-operative boundary
Summary by CodeRabbit
New Features
Bug Fixes
Chores