docs: document credential injection and the fork's npm package - #5
Merged
Conversation
AGENTS.md had no entry for token mode, even though it is the fork's reason to exist. Adds it to Key Design Decisions, covering the parts an agent or contributor can break without noticing: - olk is a pure consumer of an injected delegated Graph token: no keyring, no account files, no refresh, no persistence; process-scoped lifetime. The reason is stated (a container-local keyring leaks a durable credential and races Microsoft's refresh-token rotation across concurrent containers). - newTokenMode() fails closed before any network call or credential-store access. - Expiry maps to exit 77 via the errTokenExpired sentinel; do not fold it into the generic exit 1. - nominalTokenLifetime (30 min) exists because a zero expiry makes the Azure SDK refuse to send the request. - auth login|logout|clean|list refuse to run in this mode; --account is rejected and --mailbox still works. - The token must never reach output, errors, verbose logs, or MCP argv; token_test.go asserts it. Also records token.go in the project structure, token_test.go in the coverage list, and the test-bootstrap-npm.sh coverage in the CI section. README's npm section pointed at upstream's unscoped `olkcli`, so a reader of this fork would install the wrong package. It now documents @planmonster/olkcli, notes the -pm.N suffix and the need to pin an exact version in an agent runner, and explains that only one platform binary is downloaded. Adds an ephemeral-runner example to the access-token section: npx plus environment-injected credentials, with the token kept out of argv, and an exit code table (0, 77, 80, 1) an orchestrator can branch on. docs/npm-publishing.md gains a troubleshooting section for the three failures hit during the real bootstrap: npm view 404 for ~3.5 min after publish (the packument is the slowest read path; do not re-publish), the git-shorthand misparse of a bare relative tarball path, and the prerelease dist-tag rule.
📝 WalkthroughWalkthroughDocumentation updates cover token-mode rules, forked npm installation and publishing constraints, ephemeral ChangesToken Mode and npm Distribution
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Comment on lines
+270
to
+278
| Read the JSON envelope from stdout; `stderr` carries human hints only. Exit codes | ||
| an orchestrator should branch on: | ||
|
|
||
| | Exit | Meaning | Orchestrator action | | ||
| |------|---------|---------------------| | ||
| | `0` | success | parse stdout | | ||
| | `77` | injected token expired, **no request was made** | mint a fresh token, retry | | ||
| | `80` | usage error (bad flag or command) | fix the invocation; do not retry | | ||
| | `1` | runtime failure (Graph error, network) | inspect stderr, retry if transient | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Qualify exit code 77 as explicit-expiry validation only.
The implementation maps code 77 to an already-passed OLK_ACCESS_TOKEN_EXPIRES_AT; tokens without supplied expiry use the nominal lifetime and a later Graph 401 can remain a runtime failure. Update the table so orchestrators do not assume every expired token produces 77.
Proposed wording
-| `77` | injected token expired, **no request was made** | mint a fresh token, retry |
+| `77` | supplied token expiry was already passed; **no request was made** | mint a fresh token, retry |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Read the JSON envelope from stdout; `stderr` carries human hints only. Exit codes | |
| an orchestrator should branch on: | |
| | Exit | Meaning | Orchestrator action | | |
| |------|---------|---------------------| | |
| | `0` | success | parse stdout | | |
| | `77` | injected token expired, **no request was made** | mint a fresh token, retry | | |
| | `80` | usage error (bad flag or command) | fix the invocation; do not retry | | |
| | `1` | runtime failure (Graph error, network) | inspect stderr, retry if transient | | |
| Read the JSON envelope from stdout; `stderr` carries human hints only. Exit codes | |
| an orchestrator should branch on: | |
| | Exit | Meaning | Orchestrator action | | |
| |------|---------|---------------------| | |
| | `0` | success | parse stdout | | |
| | `77` | supplied token expiry was already passed; **no request was made** | mint a fresh token, retry | | |
| | `80` | usage error (bad flag or command) | fix the invocation; do not retry | | |
| | `1` | runtime failure (Graph error, network) | inspect stderr, retry if transient | |
Comment on lines
+280
to
+285
| For a tool-calling agent, run the MCP server instead — read-only by default, with | ||
| untrusted-content wrapping forced on: | ||
|
|
||
| ```bash | ||
| npx -y @planmonster/olkcli@1.10.0-pm.1 mcp | ||
| ``` |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pass token and safety variables to the MCP example.
The preceding env ... npx assignment applies only to that one process. This standalone command therefore does not inject OLK_ACCESS_TOKEN or the documented guard variables, so it may use stored-account/keyring mode instead of the promised ephemeral token mode.
Repeat the environment block here or provide a wrapper command that preserves it.
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
1 participant
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.
AGENTS.md — token mode was undocumented
Credential injection is the reason this fork exists, but
AGENTS.mdhad no entry for it. Added to Key Design Decisions, focused on the invariants that are easy to break silently:newTokenMode()fails closed before any network call or credential-store accesserrTokenExpiredsentinel; must not collapse into exit 1nominalTokenLifetime(30 min) exists because a zero expiry makes the Azure SDK refuse to send the requestauth login|logout|clean|listrefuse to run in this mode;--accountrejected,--mailboxstill workstoken_test.goasserts itAlso records
token.goin the project structure,token_test.goin the coverage list, and thetest-bootstrap-npm.shcoverage in the CI section. Corrects the stale0.9.5-pm.1example to1.10.0-pm.1and notes that all seven npm names now exist.README — pointed at the wrong package
The npm section said
npm install -g olkcli/npx olkcli, which installs upstream's package. Now documents@planmonster/olkcli, notes the-pm.Nsuffix and the need to pin an exact version in an agent runner, and explains that only one platform binary is downloaded (~23 MB compressed, not all six).Adds an ephemeral-runner example to the access-token section:
npxplus environment-injected credentials, token deliberately kept out of argv, and an exit-code table an orchestrator can branch on:docs/npm-publishing.md — troubleshooting
Documents the three real failures from the bootstrap:
npm view404 for ~3.5 min after publish (packument is the slowest read path — do not re-publish, check dist-tags or search instead), the git-shorthand misparse of a bare relative tarball path, and the prerelease dist-tag rule.Testing
Docs-only, but the full gate set was run:
go build,go test -raceoninternal/cmd,golangci-lint run(0 issues),test-npm-package.shPASS,test-bootstrap-npm.shPASS.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.