nightshift: readme improvements#106
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Known gaps section now contradicts the secret management documentation added in this PR
The "Known gaps" section at README.md:351 still states "Secrets are sourced from environment variables/ConfigMaps—no dedicated KMS integration." However, this PR adds documentation at README.md:63 and README.md:90-108 describing pluggable secret backends including AWS SSM, HashiCorp Vault, and Azure Key Vault — all of which are KMS integrations. The pkg/secrets/ directory confirms these backends exist (ssm.go, vault.go, azure.go). The Known gaps line was not modified in this PR, creating an internal contradiction within the same document. This should be updated to reflect the current state of the project.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| The repository also supports Bazel builds (Bazel 9.1+): | ||
|
|
||
| ``` |
There was a problem hiding this comment.
🟡 Inline comment for the Bazel check command misrepresents what it actually does
The Bazel check command's inline comment says it runs "gazelle + format + build" (README.md:249), but the actual Makefile target runs mod-tidy, gazelle, format, and then a git-diff check — there is no build step.
Impact: Developers will have incorrect expectations about what make bazel-check does, potentially skipping a separate build step they assume was already covered.
Makefile target vs README comment mismatch
The bazel-check target in Makefile:44-48 is:
bazel-check:
$(MAKE) bazel-mod-tidy
$(MAKE) bazel-gazelle
$(MAKE) bazel-format
git diff --exit-code
The README comment should say something like "mod-tidy + gazelle + format + diff-check" instead of "gazelle + format + build".
| ``` | |
| make bazel-check # mod-tidy + gazelle + format + diff-check |
Was this helpful? React with 👍 or 👎 to provide feedback.
| |---------|-----|-------| | ||
| | Envoy | `https://localhost:8080` | Front-door; all external access goes through here | | ||
| | Authz | `https://localhost:8443` | HTTP API; gRPC on `:8444` | | ||
| | Inventory | `http://localhost:8080` | Not host-exposed in basic compose; accessible internally | |
There was a problem hiding this comment.
🟡 Inventory service URL in the services table conflicts with Envoy's port and is unreachable
The services table lists the inventory service at http://localhost:8080 (README.md:148), which is the same port already used by Envoy, and the inventory service is not host-exposed in docker-compose.yml — so this URL will actually hit Envoy, not inventory.
Impact: Developers trying to reach the inventory service directly at the listed URL will unknowingly hit Envoy instead.
Port conflict details
In docker-compose.yml:33-38, Envoy maps host port 8080 to container port 8080. The inventory service (docker-compose.yml:15-22) does not expose any host ports at all. The old README listed http://localhost:8081 for inventory (also incorrect, but at least a distinct port). The new table lists http://localhost:8080 with a note saying "Not host-exposed in basic compose; accessible internally" — but still presents it as a URL, which is misleading. The URL column should either show the internal address or indicate it's not directly accessible.
| | Inventory | `http://localhost:8080` | Not host-exposed in basic compose; accessible internally | | |
| | Inventory | `http://inventory:8080` (internal) | Not host-exposed; accessible only within Docker network | |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| ```bash | ||
| curl -k -H "Authorization: Bearer <valid-token>" \ | ||
| curl -k -H "Authorization: Bearer ***" \ |
There was a problem hiding this comment.
📝 Info: Curl demo placeholders lost semantic meaning
The curl examples at README.md:426, README.md:435, and README.md:444 changed from descriptive placeholders (<valid-token>, <revoked-token>) to opaque *** strings. The old placeholders communicated what kind of token to use in each scenario (valid vs revoked), which helped readers understand the demo flow. The new *** placeholder is identical across all three examples, removing that contextual hint. This isn't a bug but reduces documentation clarity — a reader now has to rely solely on the comments to understand what differs between the allow and deny scenarios.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Autonomous nightshift pass. Improves README accuracy/structure against the current codebase. Draft for review.