Remove SSH Agent deep dive - #828
Conversation
Deploying contributing-docs with
|
| Latest commit: |
f8b9821
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f1ca023e.contributing-docs.pages.dev |
| Branch Preview URL: | https://dn-pm-32892-update-ssh-agent.contributing-docs.pages.dev |
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR removes the SSH Agent deep-dive documentation, deleting Code Review DetailsNo findings. Verified the removal is complete:
The two remaining unresolved review threads reference |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
withinfocus
left a comment
withinfocus
left a comment
There was a problem hiding this comment.
I wasn't able to see this until now but it seems we have not gotten the word out sufficiently on where documentation should go -- see https://bitwarden.atlassian.net/wiki/spaces/EN/pages/1774977070/Documentation+Patterns. There is essentially a moratorium on expanding these deep dive pages and they should exist "close to code" so that they can aid other engineers and AI more directly. This being in GitHub will also empower you to render your diagrams natively.
I don't know how heavily linked this is, but the page(s) should leave entirely or be more of a launch point, and deep dives that do remain are more of a necessary evil when the spread of a feature goes beyond a single repo.
| @@ -0,0 +1,188 @@ | |||
| # Bitwarden SSH Agent architecture | |||
|
|
|||
|  | |||
There was a problem hiding this comment.
❌ We use Mermaid for all documentation diagrams (via code) now.
There was a problem hiding this comment.
done in the clients changes
There was a problem hiding this comment.
@withinfocus , I attempted to convert all three of my diagrams to Mermaid. I spent a fair amount of time on it both manually and using Claude.
https://github.com/bitwarden/clients/pull/21791/changes
I'm not really pleased with the results especially of the third diagram (in section "The SSH Agent") , in comparison with the PNG files they look objectively worse. Do you have any recommendations to improve them?
Seems like one hard limitation I hit was https://mermaid.ai/open-source/syntax/flowchart.html#limitation
|
👋 Per feedback from @withinfocus , I will shift this PR to just remove the existing ssh agent deep dive docs. The v2 architecture has moved to https://github.com/bitwarden/clients/pull/21791/changes Note that I have open questions about how to make the Mermaid diagrams look decent, at present they look worse than the PNGs from draw.io |
neuronull
changed the title
|
|
||
| #### agent | ||
|
|
||
| The agent crate itself is a the orchestrator of all the above components. It starts and stops the |
There was a problem hiding this comment.
🎨 SUGGESTED: Grammar errors in newly-added prose (cspell won't catch these since all words are individually valid).
Details and fix
- Line 76: "is a the orchestrator" → "is the orchestrator"
- Line 25: "handle any each new client connection" → "handle each new client connection"
- Line 22: "it's keystore populated" → "its keystore populated" (possessive)
- Line 106: "based on it's authorization policy" → "based on its authorization policy" (possessive)
These are the same its/it's class of typos flagged earlier; a couple instances remain in the current file.
withinfocus
left a comment
withinfocus
left a comment
There was a problem hiding this comment.
A link to the documentation over in the repo is still valuable, if we want that. Is there no value in the agent having a mention here?
This comment: #828 (review) , noted that documentation should move closer to code and be here for things that span repos. The agent is isolated to clients repo. Combined with this comment : #828 (comment) , if I move the new v2 architecture documentation over to clients (in my linked PR) , then all that is left is the present key and agent background info, which I would tend to agree with the prior comment stating that it's not really beneficial info that users couldn't get elsewhere. Somewhat feeling like getting conflicting feedback 🤔 Can totally link to the client repo's README but it seems to clash with the earlier feeback. |
withinfocus
left a comment
withinfocus
left a comment
There was a problem hiding this comment.
The agent is isolated to clients repo.
That's the one key piece of feedback that answers my question. I at least wondered if it needed broader documentation, but if that's not the case this can go just like it is.
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-32892
📔 Objective
With the v2 of the SSH Agent, we completely re-architected it to be our own native solution and not depend on bitwarden-russh fork.
The existing deep dive on the subject is mostly generalized background information for on SSH key and agent mechanics.
bitwarden/clients#21791 presents an actual documentation of the SSH Agent v2 architecture.
The present deep dive is essentially a background context on SSH keys and an SSH Agent, which we don't need to explain to users.