fix(github): read app token from mounted secret - #370
Merged
Conversation
Author
|
@allyblockcast please review head c8039fa. Focus: GitHub App token freshness path for Paperclip k8s agent jobs; ensure no token is exposed in process args and the Secret mount/wrapper approach is scoped. |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
reviewed head: c8039fa
Looks good. I did not find any high-confidence Critical or Important issues in this diff.
Review Notes
- The token is no longer rendered into
GH_TOKEN/GITHUB_PERSONAL_ACCESS_TOKENon the StatefulSet env, so the pod spec and spawned process command lines avoid carrying the installation token directly. - The
ghandgithub-mcp-serverwrappers read the mounted Secret file at command startup, then pass token material through environment variables to the child process rather than through process args. - The
gitwrapper scopes the credential helper tohttps://github.comand returns the token via Git credential-helper stdout, which avoids embedding the token in remote URLs or argv. - The Secret volume is mounted read-only at
/paperclip/.secrets/github-token, while the wrapper scripts live under the shared/paperclip/.local/binpath that the values file prepends toPATH.
Suggestions (0)
None.
Strengths
- Removes stale token snapshots from Helm-rendered env while keeping existing tools compatible.
- Fails loudly when the mounted token file is absent or empty, which is safer than silently falling back to stale cached GitHub credentials.
- The PR body includes useful Helm render, dry-run, shell syntax, and lint verification.
Recommended Action
- Merge after normal CI and deployment-owner review.
13 tasks
12 tasks
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.
Thinking Path
What Changed
paperclip-github-mcp-tokenas a read-only Secret volume at/paperclip/.secrets/github-tokenfor the Blockcast Paperclip pods.GITHUB_PERSONAL_ACCESS_TOKEN/GH_TOKENenv injection from Blockcast values and setPATHplusPAPERCLIP_GITHUB_TOKEN_FILEinstead./paperclip/.local/binwrappers forgh,git, andgithub-mcp-server; each wrapper reads the mounted token file at command startup.https://github.comso GitHub HTTPS operations can read the same mounted token without putting token material in process args./paperclip/.local/bin/github-mcp-server.Verification
helm template paperclip deploy/helm/paperclip -n paperclip -f deploy/helm/paperclip/values.blockcast.yaml > /tmp/paperclip-rendered.yamlkubectl apply --dry-run=client -f /tmp/paperclip-rendered.yamlyq -r '.spec.template.spec.initContainers[]? | select(.name == "seed") | .command[2]' /tmp/paperclip-rendered.yaml > /tmp/paperclip-seed.sh && sh -n /tmp/paperclip-seed.shhelm lint deploy/helm/paperclip -f deploy/helm/paperclip/values.blockcast.yamlPATHandPAPERCLIP_GITHUB_TOKEN_FILE, do not renderGH_TOKEN/GITHUB_PERSONAL_ACCESS_TOKEN, and mount thegithub-mcp-tokenSecret volume.Risks
/usr/bin/gh,/usr/bin/git, or/usr/local/bin/github-mcp-serverbypass the wrappers; normal PATH-based use and the seeded MCP config use wrappers.hosts.ymlcredentials./papercliprelies on Kubernetes' standard overlapping volume mount behavior; the chart render and client dry-run accept the spec.Model Used
Checklist