fix(doctor): a credential at rest is a FAILURE, not a healthy environment - #150
chitcommit wants to merge 2 commits into
Conversation
…ment
`can doctor` defined a complete dev environment as one that had already
committed the violation. Its token checks reported a value found in
`process.env` or in `config.sync.{notionToken,githubToken}` as:
✓ NOTION_TOKEN: Set in environment
✓ GITHUB_TOKEN: Set in config
and reported a CLEAN environment as a warning whose remediation instructed the
operator to create the violation:
⚠ GITHUB_TOKEN: Not configured
→ Set GITHUB_TOKEN environment variable or run: chitty sync setup
Both polarities were backwards. A credential must never be written into config,
runtime, documentation or otherwise — so presence is the defect and absence is
the healthy state. This inverts the check and lets the existing exit-code path
do the enforcing: ✗ counts as failed, failed>0 exits 1, so a violating
environment can no longer report itself ready.
Verified against real behaviour, both directions:
clean env ✓ no credential at rest in config or environment 8 passed, exit 0
token present ✗ credential present in runtime environment 1 failed, exit 1
Constraints honoured deliberately, because this check runs over exactly the
material it must never touch:
- only PRESENCE is tested. No value is read, logged, printed or compared.
- nothing is removed or rewritten. Grep-and-destroy is unsound here: config
may legitimately hold an item-ID REFERENCE rather than a value, and deleting
on a name match would destroy the reference while leaving the real secret.
The remediation now points at where credential material actually belongs — a
node-sealed attachment, bound to the node and inert if copied off it — and
explicitly tells the operator NOT to set the environment variable. That
attachment does not exist yet; this commit deliberately does not check for it or
stub it, because a check against an undefined capability would pass vacuously,
which is the failure mode this change exists to remove.
Scope note: this changes a health check's polarity to match stated policy. It
does not design credential architecture — that is ChittyConnect's, and the
attachment itself is owned there.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H6EB22mDgirBYomHn3hfyJ
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe doctor command now treats configured or runtime ChangesCredential exposure checks
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/commands/doctor.ts`:
- Around line 141-142: Update the credential presence checks in the doctor
command to use own-property checks on process.env and config.sync rather than
retrieving values through boolean coercion. Preserve the existing configPath
guard, treat empty-but-present environment variables and config keys as
configured, and remove the credential key instead of assigning an empty value.
🪄 Autofix
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: 427e4ac4-967a-43cf-b792-ae445b2daa26
📒 Files selected for processing (1)
src/commands/doctor.ts
The credential-at-rest hard block used `!!process.env[cs.env]` and a similarly truthy-coerced config lookup to decide presence. Both treat an env var or config key set to the empty string as absent, so `GITHUB_TOKEN=` (present, empty) reported ✓ "no credential at rest" instead of the intended hard-block failure — silently defeating the check's own stated contract that presence, not value, is what's being tested. Switch to own-property checks (`hasOwnProperty` on process.env, and a new hasNestedOwnProperty helper for the config path) that detect presence without ever reading the credential value, closing the empty-string gap while keeping the "value is never read" guarantee. Flagged by coderabbitai review on PR #150.
2 participants
can doctordefined a complete dev environment as one that had already committed the violation.Its token checks reported a value found in
process.envorconfig.sync.{notionToken,githubToken}as healthy:…and reported a clean environment as a warning whose remediation instructed the operator to create the violation:
Both polarities were backwards. A credential must never be written into config, runtime, documentation or otherwise — so presence is the defect and absence is the healthy state.
What changed
The check is inverted, and the existing exit-code path does the enforcing:
✗counts asfailed,failed > 0exits 1. A violating environment can no longer report itself ready.Verified against real behaviour, both directions
✓ no credential at rest in config or environment— 8 passed✗ credential present in runtime environment — hard block— 1 failedConstraints honoured deliberately
This check runs over exactly the material it must never touch:
What it deliberately does NOT do
The remediation points at where credential material belongs — a node-sealed attachment, bound to the node and inert if copied off it — and explicitly tells the operator not to set the variable.
That attachment does not exist yet, and this PR does not check for it or stub it. A check against an undefined capability passes vacuously, which is precisely the failure mode this change removes.
Scope
This changes a health check's polarity to match stated policy. It does not design credential architecture — that is ChittyConnect's, and the attachment is owned there.
🤖 Generated with Claude Code
https://claude.ai/code/session_01H6EB22mDgirBYomHn3hfyJ
Summary by CodeRabbit
New Features
Bug Fixes