Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe remote test offload document now uses generic remote runner terminology. It removes host-specific access and transfer commands, adds generic validation commands, and retains the validation results and push explanation. ChangesRemote runner documentation
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Other Merge Risk: ⚪ Minimal · up to The PR removes operational infrastructure details while preserving valid validation commands and historical results, with no runtime or configuration changes. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
added
the
documentation
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
리뷰 · 우선순위 50 / 80설명 이 PR(작성자 luvs01, draft)은 역사 기록 바뀌는 내용은 문서 하나뿐이다. 제목을 'macmini-cf' 대신 '원격 러너'로 바꾸고, Host 블록·경로·전송/실행 명령을 빼며, 대신 '저장소 밖 운영 구성에서 관리한다'는 문장과 일반 검증 네 줄( 현재 우선순위 50은 'tip 노출 제거는 당장 가치 있음'과 '히스토리·다른 문서 잔존·draft'를 같이 반영한 점수다. 런타임·테스트 변경이 없고 privacy:scan도 통과했다고 하니, Ready만 맞으면 부담 없이 넣을 수 있는 문서 PR이다. 다만 메인테이너가 '히스토리에 남은 동일 블록을 어떻게 볼지'와 '다른 macmini-cf 언급을 후속 레닥션할지'를 짧게 정해 두는 편이 좋다. 라인 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
2cc47f1 to
4507e62
Compare
abhisheksharma2411
left a comment
abhisheksharma2411
left a comment
There was a problem hiding this comment.
Checked the redaction for completeness rather than reading the diff, and it holds — with one gap that isn't in this PR's scope but is the reason it was needed.
The sensitive values are gone, repo-wide. Grepped the whole tree at 4507e622, not just the changed file:
| files at PR head | |
|---|---|
ssh-macmini.lidgeai.com |
0 |
User junny / junny |
0 |
~/ci/opencodex |
0 |
So the hostname, the account, the Cloudflare ProxyCommand and the checkout path are all actually gone, not just gone from this document. That's the part that mattered.
One thing I checked and it's a false alarm, worth saying so you don't chase it: lidgeai.com still appears in three files — SPONSORS.md, scripts/privacy-scan.ts, tests/ci-workflows/privacy-scan-meta-key.test.ts. That's the published sponsorship contact (jun@lidgeai.com), deliberately allowlisted by SPONSORSHIP_CONTACT_FILES. Not a leak, not related.
The host alias macmini-cf survives in 43 devlog files. I don't think that's a blocker — an SSH config nickname without the HostName, User or ProxyCommand is close to meaningless, and rewriting 43 historical devlogs is a lot of churn for it. Flagging it so it's a decision rather than an oversight. The PR title says "remove remote runner access details", and the alias is arguably not an access detail.
The gap worth fixing separately
privacy:scan passes on the unredacted content. I ran it on untouched origin/dev, where that file still contains all three values:
$ git show origin/dev:devlog/.../022_remote_test_offload.md | grep -cE "ssh-macmini.lidgeai.com|User junny|ProxyCommand"
3
$ bun run privacy:scan
Privacy scan passed
scripts/privacy-scan.ts has rules for tokens, emails and home-directory usernames, and no rule for hostnames, SSH config blocks or ProxyCommand — I grepped it for all three and there's nothing. So this document was publishable, and the next devlog that pastes an SSH block will be too.
That makes this PR a one-time cleanup of something the gate can't hold. A rule keyed on ProxyCommand / HostName / Host <alias> inside a fenced block would close it, and MAINTAINER_HOME_USERNAME is already the precedent for "this specific operator value must not ship".
Happy to open that as its own PR if you'd like — it's orthogonal to this one, and this one shouldn't wait for it.
On the description: "it does not claim a historical purge" is the right disclaimer and I'd keep it. Worth stating the practical consequence too, for whoever reads this later: the values remain reachable in git history and in GitHub's UI for the old commits, so if any of them are still live credentials-adjacent — an internal hostname behind Cloudflare Access is — rotating or renaming is the only thing that actually retires them. The doc change limits new exposure, not existing.
privacy:scan knew about tokens, emails and home paths, and nothing about infrastructure. A working Host block was therefore publishable: the scan passes on dev today, where devlog/_fin/260731_pr_merge_round/022_remote_test_offload.md still carries a real HostName, account and Cloudflare ProxyCommand. lidge-jun#4623 removes them by hand; nothing stops the next devlog reintroducing them. Two detectors, both anchored to the SSH config grammar: HostName value must be the whole rest of the line ProxyCommand command line, matched separately `User` is deliberately not matched. It is an ordinary English word and even line-anchored it fires on wrapped prose — "…the\nuser configuration." and "…the\nuser notice." both matched during development, as did `hostname === undefined ? ...` in a test file before the value was anchored. The username is also the least sensitive part of a Host block, and MAINTAINER_HOME_USERNAME already covers it in path form. A ProxyCommand containing %h is NOT allowlisted. Only a bare %h is. The substitution token does not make the binary path, the access method or the tunnel any less of a leak — allowing it would have passed the exact line this exists to catch. Also moves the repo scan behind import.meta.main. It ran at module scope, so `import { scanText }` executed a full scan as a side effect and a failing scan called process.exit(1), killing the importing test process. Invisible while the tree is clean; adding the detector above broke privacy-scan-meta-key.test.ts, which does nothing but import the seam this file exports for testing. Refs lidge-jun#4623
… real host Two problems with this PR as it stood, both the same shape as the leak it exists to catch. The report printed the `ssh-endpoint` value while redacting the `ProxyCommand`. This scan runs in CI on a public repository, so a finding would have republished the endpoint into a public log — the scanner leaking what it was written to detect. `file:line` already locates it for whoever removes it, which is what the ProxyCommand kind has relied on all along. The regression fixture and the rationale comment both spelled out the real hostname and login. lidge-jun#4623 removes those from the devlog; keeping them here would have undone that cleanup and made this file their permanent home. The fixture now uses a synthetic endpoint — the regex cannot tell the difference — and the comment describes the incident without restating the values.
privacy:scan knew about tokens, emails and home paths, and nothing about infrastructure. A working Host block was therefore publishable: the scan passes on dev today, where devlog/_fin/260731_pr_merge_round/022_remote_test_offload.md still carries a real HostName, account and Cloudflare ProxyCommand. lidge-jun#4623 removes them by hand; nothing stops the next devlog reintroducing them. Two detectors, both anchored to the SSH config grammar: HostName value must be the whole rest of the line ProxyCommand command line, matched separately `User` is deliberately not matched. It is an ordinary English word and even line-anchored it fires on wrapped prose — "…the\nuser configuration." and "…the\nuser notice." both matched during development, as did `hostname === undefined ? ...` in a test file before the value was anchored. The username is also the least sensitive part of a Host block, and MAINTAINER_HOME_USERNAME already covers it in path form. A ProxyCommand containing %h is NOT allowlisted. Only a bare %h is. The substitution token does not make the binary path, the access method or the tunnel any less of a leak — allowing it would have passed the exact line this exists to catch. Also moves the repo scan behind import.meta.main. It ran at module scope, so `import { scanText }` executed a full scan as a side effect and a failing scan called process.exit(1), killing the importing test process. Invisible while the tree is clean; adding the detector above broke privacy-scan-meta-key.test.ts, which does nothing but import the seam this file exports for testing. Refs lidge-jun#4623
… real host Two problems with this PR as it stood, both the same shape as the leak it exists to catch. The report printed the `ssh-endpoint` value while redacting the `ProxyCommand`. This scan runs in CI on a public repository, so a finding would have republished the endpoint into a public log — the scanner leaking what it was written to detect. `file:line` already locates it for whoever removes it, which is what the ProxyCommand kind has relied on all along. The regression fixture and the rationale comment both spelled out the real hostname and login. lidge-jun#4623 removes those from the devlog; keeping them here would have undone that cleanup and made this file their permanent home. The fixture now uses a synthetic endpoint — the regex cannot tell the difference — and the comment describes the incident without restating the values.
3 participants
Summary
Remove remote runner access details from public offload notes while retaining generic operational guidance. This updates the checked-in document only; it does not claim a historical purge or changes to live remote systems.
Current author verification
4507e622f77133e118b75ebaff35def9ee4d3690.Review readiness checklist
The validation checkbox refers to the explicit scope above. Historical run IDs and prior local results are not represented as new-head full-suite execution.
Summary by CodeRabbit