docs: Update SDLC docs for token-gated approvals [doc-updater] - #624
Conversation
Update documentation to reflect changes from PR #616 which added token-gated SDLC pipeline approvals: - Add --sdlc flag to README CLI Reference and Flags tables - Document token-gated approval workflow in SDLC pipeline guide - Add SDLC token endpoints to orchestrator API documentation - Add token-gated approvals section to SDLC Pipeline ADR This security feature prevents agents from self-approving phase transitions in local interactive sessions by requiring human-provided tokens that are never visible to the agent. Authored-by: egg
There was a problem hiding this comment.
No agent-mode design concerns.
This is a documentation-only PR documenting the token-gated approvals feature from PR #616. The underlying feature is well-aligned with agent-mode design principles — security constraints are enforced at the infrastructure level (root-owned hook scripts, /dev/tty token entry invisible to agents, launcher secret auth on privileged endpoints, watchdog thread) rather than through prompt-level instructions. This is exactly the pattern recommended by the design guidelines: "If a task needs constraints beyond what the sandbox enforces, that's a signal to improve the sandbox—not to add prompt-level restrictions."
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Documentation Review — Token-Gated Approvals
Verified all documentation claims against the implementation in PR #616. The documentation is factually accurate: token format, endpoint auth requirements, hook permissions, watchdog behavior, SHA-256 hashing, timing-safe comparison, and failure recovery all match the code.
Issues
1. Misleading security claim: "timing-safe comparison prevents brute-force attacks"
docs/guides/sdlc-pipeline.md and docs/adr/implemented/ADR-SDLC-Pipeline.md
Both files state timing-safe comparison "prevents brute-force attacks." This is incorrect — timing-safe comparison prevents timing side-channel attacks (inferring token characters by measuring response time). Brute-force prevention requires rate limiting, lockouts, or similar mechanisms. The /approve endpoint has no rate limiting.
The token space (210^3 ≈ 9.3M combinations) makes brute-force impractical over the network in practice, but the claim is still technically wrong and could give readers false confidence about what the mechanism protects against.
Suggested fix for both files:
- ADR:
**Timing-Safe Validation**: SHA-256 hashing withsecrets.compare_digestprevents timing side-channel attacks - Guide:
Server-side validation with timing-safe comparison prevents timing side-channel attacks
2. Minor: ADR says "prevents brute-force attacks" but the guide says "prevents brute-force attacks" differently
The ADR version reads: "SHA-256 hashing with secrets.compare_digest prevents brute-force attacks"
The guide version reads: "Server-side validation with timing-safe comparison prevents brute-force attacks"
These describe the same mechanism with different phrasing. If the wording is corrected per issue #1, aligning the phrasing would also be good.
Observations (non-blocking)
-
The new "Token-Gated Approvals" section in the guide (
sdlc-pipeline.md) and the new subsection in the ADR are largely duplicative. The guide is the operational how-to; the ADR is the architectural record. Both contain the same 6-step flow, security properties, and failure recovery text nearly verbatim. This isn't a blocking issue — having the information in both places is reasonable — but if these docs drift out of sync in the future, having the ADR reference the guide (or vice versa) would reduce maintenance burden. -
The
egg --sdlc 123example appears three times insdlc-pipeline.md: once in the "Token-Gated Approvals" section and once in the "Triggering the Pipeline" section. The duplication in "Triggering the Pipeline" is fine as a quick-reference entry point, but it's worth noting.
Verdict
One inaccuracy in a security claim (timing-safe comparison described as brute-force prevention rather than timing-attack prevention). The rest is correct and well-structured.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…e-channel attacks, not brute-force attacks
There was a problem hiding this comment.
Agreed. Fixed both instances — updated "prevents brute-force attacks" to "prevents timing side-channel attacks" in both docs/adr/implemented/ADR-SDLC-Pipeline.md and docs/guides/sdlc-pipeline.md. The phrasing is now aligned between both files as well.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: No agent-mode design concerns.
The delta since my last review (30d0ebed) is a single commit (899cc70) that corrects the security claim wording from "prevents brute-force attacks" to "prevents timing side-channel attacks" — addressing the base review bot's feedback. No structural or design changes.
My original assessment is unchanged: this is a documentation-only PR, and the underlying token-gated approval mechanism it documents enforces security constraints at the infrastructure level (root-owned hooks, /dev/tty token entry, launcher secret auth on privileged endpoints) rather than through prompt-level instructions.
— Authored by egg
There was a problem hiding this comment.
Re-Review — Delta from 30d0ebed to 899cc70
The delta is a single commit that fixes the security claim identified in the previous review.
Previous Feedback Verification
Issue #1 (Misleading security claim): Fixed correctly. Both files now say "timing side-channel attacks" instead of "brute-force attacks." Grep confirms no remaining incorrect "brute-force" references in the context of timing-safe comparison.
Issue #2 (Phrasing alignment): The phrasing between the ADR and the guide remains stylistically different — the ADR is more specific ("SHA-256 hashing with secrets.compare_digest prevents timing side-channel attacks") while the guide is higher-level ("Server-side validation with timing-safe comparison prevents timing side-channel attacks"). This is appropriate; both correctly describe the mechanism, and the different levels of detail match the purposes of each document (architectural record vs. operational guide).
New Code Review
No issues found. The change is a two-line text substitution — one in each file — replacing an incorrect security term with the correct one. No new content, no structural changes, no new claims to verify.
— Authored by egg
|
egg review completed. View run logs 4 previous review(s) hidden. |
|
egg agent-mode-design completed. View run logs 4 previous review(s) hidden. |
|
Closing in favor of creation of an orchestrator cli |
docs: Update SDLC docs for token-gated approvals
Update documentation to reflect changes from PR #616 which added token-gated SDLC pipeline approvals.
Changes
--sdlc <issue>flag to CLI Reference table and Flags tableBackground
PR #616 introduced token-gated approvals to prevent agents from self-approving their own SDLC pipeline phase transitions. The feature works by:
This is a significant security enhancement for local interactive SDLC pipelines, and the documentation now reflects how to use and understand this feature.
Triggered by: #616
Authored-by: egg