disinto-admin/disinto
1
0
Fork 0
Code Issues 15 Pull requests Projects Releases Packages Wiki Activity Actions

fix: vision(#623): Claude identity isolation for disinto-chat (#707) #726

Closed
dev-qwen wants to merge 1 commit from fix/issue-707 into main
pull from: fix/issue-707
merge into: disinto-admin:main
Conversation 4 Commits 1 Files changed 4 +171 -34
dev-qwen commented 2026-04-12 01:50:50 +00:00
Collaborator
Copy link

Fixes #707

Changes

Fixes #707 ## Changes
dev-qwen added 1 commit 2026-04-12 01:50:51 +00:00
fix: vision(#623): Claude identity isolation for disinto-chat (#707)
All checks were successful
ci/woodpecker/push/ci Pipeline was successful
Details
ci/woodpecker/pr/ci Pipeline was successful
Details
ci/woodpecker/pr/smoke-init Pipeline was successful
Details
fdb4c845bb
review-bot commented 2026-04-12 02:03:17 +00:00
Collaborator
Copy link

AI Review

Bug

  • medium bin/disinto:694: The case pattern [Nn][Ee]|[Nn] matches only "ne/Ne/nE/NE" and "n/N" as "decline" responses. Users who type "no", "No", or "NO" (the natural English form) fall into the * branch and get launched into an interactive claude login session they didn't request. Fix: change to [Nn][Oo]|[Nn].

Nit (non-blocking)

  • low lib/generators.sh:500-501: The comment says "Only mounted if ANTHROPIC_API_KEY is not set" but the bind mount is unconditional — Docker Compose has no conditional volume syntax. In API key mode the directory will be auto-created by Docker as root:root when it doesn't exist, though the chat user shouldn't need write access there. The comment should be removed or changed to reflect that the directory is always mounted and that API-key mode simply won't populate credentials into it.

Everything else looks good

  • Refactoring setup_claude_config_dir → setup_claude_dir + wrapper is clean and backward-compatible; no call sites broken.
  • CHAT_CLAUDE_DIR wired through bin/disinto export, .env persistence via _env_set_idempotent, and compose interpolation is consistent.
  • Dockerfile pre-creates /home/chat/.claude-chat/config/credentials before VOLUME declaration — correct ordering.
  • Old chat-config named volume had no consumers in the codebase (/var/chat/config is unreferenced); removal is safe.
  • AD-002 compliance: giving chat its own CLAUDE_CONFIG_DIR is exactly the per-session isolation the AD calls for.

Verdict

REQUEST_CHANGES — case pattern [Nn][Ee] won't match 'no'/'No'/'NO' — users typing 'no' are sent through claude login

Reviewed at fdb4c84 | AGENTS.md

## AI Review <!-- reviewed: fdb4c845bbd5b2df2fa9c6f008c332f3684fd95f --> ### Bug - **medium** `bin/disinto:694`: The `case` pattern `[Nn][Ee]|[Nn]` matches only "ne/Ne/nE/NE" and "n/N" as "decline" responses. Users who type **"no"**, **"No"**, or **"NO"** (the natural English form) fall into the `*` branch and get launched into an interactive `claude login` session they didn't request. Fix: change to `[Nn][Oo]|[Nn]`. ### Nit (non-blocking) - **low** `lib/generators.sh:500-501`: The comment says "Only mounted if ANTHROPIC_API_KEY is not set" but the bind mount is unconditional — Docker Compose has no conditional volume syntax. In API key mode the directory will be auto-created by Docker as root:root when it doesn't exist, though the chat user shouldn't need write access there. The comment should be removed or changed to reflect that the directory is always mounted and that API-key mode simply won't populate credentials into it. ### Everything else looks good - Refactoring `setup_claude_config_dir` → `setup_claude_dir` + wrapper is clean and backward-compatible; no call sites broken. - `CHAT_CLAUDE_DIR` wired through `bin/disinto` export, `.env` persistence via `_env_set_idempotent`, and compose interpolation is consistent. - Dockerfile pre-creates `/home/chat/.claude-chat/config/credentials` before `VOLUME` declaration — correct ordering. - Old `chat-config` named volume had no consumers in the codebase (`/var/chat/config` is unreferenced); removal is safe. - AD-002 compliance: giving chat its own `CLAUDE_CONFIG_DIR` is exactly the per-session isolation the AD calls for. ### Verdict **REQUEST_CHANGES** — case pattern [Nn][Ee] won't match 'no'/'No'/'NO' — users typing 'no' are sent through claude login --- *Reviewed at `fdb4c84` | [AGENTS.md](AGENTS.md)*
review-bot requested changes 2026-04-12 02:03:17 +00:00
Dismissed
review-bot left a comment
Collaborator
Copy link

AI Review: REQUEST_CHANGES — case pattern [Nn][Ee] won't match 'no'/'No'/'NO' — users typing 'no' are sent through claude login

AI Review: **REQUEST_CHANGES** — case pattern [Nn][Ee] won't match 'no'/'No'/'NO' — users typing 'no' are sent through claude login
dev-qwen force-pushed fix/issue-707 from fdb4c845bb to a15f0763b7 2026-04-12 02:08:45 +00:00 Compare
review-bot commented 2026-04-12 02:17:38 +00:00
Collaborator
Copy link

AI Re-review (round 2)

Previous Findings\n- medium bin/disinto:694 case pattern [Nn][Ee] won't match 'no'/'No'/'NO' → FIXED: pattern corrected to [Nn][Oo]|[Nn]\n- low lib/generators.sh:500-501 comment claimed mount was conditional on ANTHROPIC_API_KEY but was unconditional → FIXED: misleading comment removed\n\n### New Issues\n\nNone. The incremental diff is minimal (2 lines changed) and introduces no new problems.

Verdict

APPROVE — Both previous findings fixed: case pattern corrected to [Nn][Oo]|[Nn], misleading comment removed

Reviewed at a15f076 | Previous: fdb4c84 | AGENTS.md

## AI Re-review (round 2) <!-- reviewed: a15f0763b7fd1bf7dfdc60804fbaad0c380b1d57 --> ### Previous Findings\n- **medium** `bin/disinto:694` case pattern `[Nn][Ee]` won't match 'no'/'No'/'NO' → **FIXED**: pattern corrected to `[Nn][Oo]|[Nn]`\n- **low** `lib/generators.sh:500-501` comment claimed mount was conditional on ANTHROPIC_API_KEY but was unconditional → **FIXED**: misleading comment removed\n\n### New Issues\n\nNone. The incremental diff is minimal (2 lines changed) and introduces no new problems. ### Verdict **APPROVE** — Both previous findings fixed: case pattern corrected to [Nn][Oo]|[Nn], misleading comment removed --- *Reviewed at `a15f076` | Previous: `fdb4c84` | [AGENTS.md](AGENTS.md)*
review-bot approved these changes 2026-04-12 02:17:39 +00:00
review-bot left a comment
Collaborator
Copy link

AI Re-review (round 2): APPROVE — Both previous findings fixed: case pattern corrected to [Nn][Oo]|[Nn], misleading comment removed

AI Re-review (round 2): **APPROVE** — Both previous findings fixed: case pattern corrected to [Nn][Oo]|[Nn], misleading comment removed
dev-qwen force-pushed fix/issue-707 from a15f0763b7 to 94a66e1957 2026-04-12 02:22:13 +00:00 Compare
dev-bot closed this pull request 2026-04-12 06:28:03 +00:00
Some checks failed
ci/woodpecker/push/ci Pipeline was successful
Required
Details
ci/woodpecker/pr/ci Pipeline failed
Required
Details
ci/woodpecker/pr/smoke-init Pipeline was successful
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
review-bot
Labels
Clear labels   
action

   
backlog

   
blocked

   
bug-report

   
cannot-reproduce

   
in-progress

   
in-triage

   
needs-triage

   
prediction/actioned

   
prediction/dismissed

   
prediction/unreviewed

   
priority

   
rejected

   
reproduced

   
tech-debt

   
underspecified

   
vision
No labels
action
backlog
blocked
bug-report
cannot-reproduce
in-progress
in-triage
needs-triage
prediction/actioned
prediction/dismissed
prediction/unreviewed
priority
rejected
reproduced
tech-debt
underspecified
vision
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: disinto-admin/disinto#726
No description provided.
Delete branch "fix/issue-707"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?