Conversation
- github-codespace: document REST PATCH fallback when 'gh pr edit' lacks read:project scope (device-code gho_ token works; ghu_ App token 403s) - codespace-persistent-symlinks: add test-stub-leak pitfall (never cp runtime stub back into tracked config; Greptile caught defaultProvider:'x' shipped this way) and 'name config dirs after the tool not the skill' guidance
|
| Filename | Overview |
|---|---|
| .devcontainer/skills/codespace-persistent-symlinks/SKILL.md | Adds symlink and testing lessons, but commits unresolved merge-conflict markers around the entire new section. |
| .devcontainer/skills/github-codespace/SKILL.md | Adds documentation for editing pull-request metadata through the REST API when the normal CLI command lacks scope. |
Prompt To Fix All With AI
### Issue 1
.devcontainer/skills/codespace-persistent-symlinks/SKILL.md:67-87
**Unresolved conflict markers remain**
When this skill is read or validated, the committed Git conflict markers expose unresolved merge metadata around the new guidance, leaving the intended document state ambiguous and causing checks that reject conflict markers to fail.
```suggestion
- **Mixed-state runtime dirs need per-file symlinks, not whole-folder.** `~/.pi/agent/`
is the canonical case: symlink only `models.json`/`settings.json` into
`.devcontainer/pi-config/`; never symlink the whole dir (it drags in `auth.json`,
`sessions/`, locks). Add a `start-hermes.sh` guard to re-link past the tool's own
first-launch stub, and verify the guard replaces a plain stub with the symlink.
- **Test-stub leak: never `cp` from runtime back to tracked during guard testing.** When
simulating a rebuild, the runtime file (e.g. `~/.pi/agent/settings.json`) may contain
the tool's own stub (`defaultProvider: "x"`). If you `cp` it to the tracked location
(`.devcontainer/pi-config/settings.json`), you commit the stub instead of the intended
config. Instead: write the correct canonical content to the tracked file directly, then
verify the guard correctly relinks from tracked → runtime. The guard's job is
tracked→runtime; the test's job is to ensure tracked has the RIGHT content. (Observed:
Greptile caught `defaultProvider: "x"` shipped in a PR because a rebuild-sim had copied the
pi stub back into the tracked file.)
- **Name tracked config dirs after the tool, not the skill.** A folder named after the
installing skill (e.g. `firstmate/` holding pi's LM config) is misleading to reviewers and
drifts from the PR's actual purpose. Name it for what it holds (`pi-config/`), and keep the
PR title scoped to the real change (persisting pi config), not the skill install.
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "docs(skills): capture PR-editing + symli..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+67
to
+87
| <<<<<<< Updated upstream | ||
| ======= | ||
| - **Mixed-state runtime dirs need per-file symlinks, not whole-folder.** `~/.pi/agent/` | ||
| is the canonical case: symlink only `models.json`/`settings.json` into | ||
| `.devcontainer/pi-config/`; never symlink the whole dir (it drags in `auth.json`, | ||
| `sessions/`, locks). Add a `start-hermes.sh` guard to re-link past the tool's own | ||
| first-launch stub, and verify the guard replaces a plain stub with the symlink. | ||
| - **Test-stub leak: never `cp` from runtime back to tracked during guard testing.** When | ||
| simulating a rebuild, the runtime file (e.g. `~/.pi/agent/settings.json`) may contain | ||
| the tool's own stub (`defaultProvider: "x"`). If you `cp` it to the tracked location | ||
| (`.devcontainer/pi-config/settings.json`), you commit the stub instead of the intended | ||
| config. Instead: write the correct canonical content to the tracked file directly, then | ||
| verify the guard correctly relinks from tracked → runtime. The guard's job is | ||
| tracked→runtime; the test's job is to ensure tracked has the RIGHT content. (Observed: | ||
| Greptile caught `defaultProvider: "x"` shipped in a PR because a rebuild-sim had copied the | ||
| pi stub back into the tracked file.) | ||
| - **Name tracked config dirs after the tool, not the skill.** A folder named after the | ||
| installing skill (e.g. `firstmate/` holding pi's LM config) is misleading to reviewers and | ||
| drifts from the PR's actual purpose. Name it for what it holds (`pi-config/`), and keep the | ||
| PR title scoped to the real change (persisting pi config), not the skill install. | ||
| >>>>>>> Stashed changes |
There was a problem hiding this comment.
When this skill is read or validated, the committed Git conflict markers expose unresolved merge metadata around the new guidance, leaving the intended document state ambiguous and causing checks that reject conflict markers to fail.
Suggested change
| <<<<<<< Updated upstream | |
| ======= | |
| - **Mixed-state runtime dirs need per-file symlinks, not whole-folder.** `~/.pi/agent/` | |
| is the canonical case: symlink only `models.json`/`settings.json` into | |
| `.devcontainer/pi-config/`; never symlink the whole dir (it drags in `auth.json`, | |
| `sessions/`, locks). Add a `start-hermes.sh` guard to re-link past the tool's own | |
| first-launch stub, and verify the guard replaces a plain stub with the symlink. | |
| - **Test-stub leak: never `cp` from runtime back to tracked during guard testing.** When | |
| simulating a rebuild, the runtime file (e.g. `~/.pi/agent/settings.json`) may contain | |
| the tool's own stub (`defaultProvider: "x"`). If you `cp` it to the tracked location | |
| (`.devcontainer/pi-config/settings.json`), you commit the stub instead of the intended | |
| config. Instead: write the correct canonical content to the tracked file directly, then | |
| verify the guard correctly relinks from tracked → runtime. The guard's job is | |
| tracked→runtime; the test's job is to ensure tracked has the RIGHT content. (Observed: | |
| Greptile caught `defaultProvider: "x"` shipped in a PR because a rebuild-sim had copied the | |
| pi stub back into the tracked file.) | |
| - **Name tracked config dirs after the tool, not the skill.** A folder named after the | |
| installing skill (e.g. `firstmate/` holding pi's LM config) is misleading to reviewers and | |
| drifts from the PR's actual purpose. Name it for what it holds (`pi-config/`), and keep the | |
| PR title scoped to the real change (persisting pi config), not the skill install. | |
| >>>>>>> Stashed changes | |
| - **Mixed-state runtime dirs need per-file symlinks, not whole-folder.** `~/.pi/agent/` | |
| is the canonical case: symlink only `models.json`/`settings.json` into | |
| `.devcontainer/pi-config/`; never symlink the whole dir (it drags in `auth.json`, | |
| `sessions/`, locks). Add a `start-hermes.sh` guard to re-link past the tool's own | |
| first-launch stub, and verify the guard replaces a plain stub with the symlink. | |
| - **Test-stub leak: never `cp` from runtime back to tracked during guard testing.** When | |
| simulating a rebuild, the runtime file (e.g. `~/.pi/agent/settings.json`) may contain | |
| the tool's own stub (`defaultProvider: "x"`). If you `cp` it to the tracked location | |
| (`.devcontainer/pi-config/settings.json`), you commit the stub instead of the intended | |
| config. Instead: write the correct canonical content to the tracked file directly, then | |
| verify the guard correctly relinks from tracked → runtime. The guard's job is | |
| tracked→runtime; the test's job is to ensure tracked has the RIGHT content. (Observed: | |
| Greptile caught `defaultProvider: "x"` shipped in a PR because a rebuild-sim had copied the | |
| pi stub back into the tracked file.) | |
| - **Name tracked config dirs after the tool, not the skill.** A folder named after the | |
| installing skill (e.g. `firstmate/` holding pi's LM config) is misleading to reviewers and | |
| drifts from the PR's actual purpose. Name it for what it holds (`pi-config/`), and keep the | |
| PR title scoped to the real change (persisting pi config), not the skill install. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: .devcontainer/skills/codespace-persistent-symlinks/SKILL.md
Line: 67-87
Comment:
**Unresolved conflict markers remain**
When this skill is read or validated, the committed Git conflict markers expose unresolved merge metadata around the new guidance, leaving the intended document state ambiguous and causing checks that reject conflict markers to fail.
```suggestion
- **Mixed-state runtime dirs need per-file symlinks, not whole-folder.** `~/.pi/agent/`
is the canonical case: symlink only `models.json`/`settings.json` into
`.devcontainer/pi-config/`; never symlink the whole dir (it drags in `auth.json`,
`sessions/`, locks). Add a `start-hermes.sh` guard to re-link past the tool's own
first-launch stub, and verify the guard replaces a plain stub with the symlink.
- **Test-stub leak: never `cp` from runtime back to tracked during guard testing.** When
simulating a rebuild, the runtime file (e.g. `~/.pi/agent/settings.json`) may contain
the tool's own stub (`defaultProvider: "x"`). If you `cp` it to the tracked location
(`.devcontainer/pi-config/settings.json`), you commit the stub instead of the intended
config. Instead: write the correct canonical content to the tracked file directly, then
verify the guard correctly relinks from tracked → runtime. The guard's job is
tracked→runtime; the test's job is to ensure tracked has the RIGHT content. (Observed:
Greptile caught `defaultProvider: "x"` shipped in a PR because a rebuild-sim had copied the
pi stub back into the tracked file.)
- **Name tracked config dirs after the tool, not the skill.** A folder named after the
installing skill (e.g. `firstmate/` holding pi's LM config) is misleading to reviewers and
drifts from the PR's actual purpose. Name it for what it holds (`pi-config/`), and keep the
PR title scoped to the real change (persisting pi config), not the skill install.
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Skill-library updates distilled from the PR #42 session (persisting pi-agent LM config across Codespace rebuilds):
gh pr editlacks scope" section.gh pr editcan fail with missingread:projectscope; fallback is a REST PATCH using the device-code OAuth token (gho_) stored in~/.config/gh/hosts.yml. Don't use the VS Code server App token (ghu_) — it 403s on PR PATCH.cpa runtime file back into the tracked location while testing a rebuild guard; the tool's own stub (e.g.defaultProvider: "x") gets committed. Greptile caught this exact bug in PR refactor: persist pi-agent LM config across Codespace rebuilds #42.firstmate/folder holding pi's config is misleading; rename topi-config/and scope the PR title to the real change.Verification
ci-lint-check.sh: ALL CHECKS PASSED (markdownlint + SKILL.md validation).🤖 Generated with Hermes Agent.