Skip to content

[workspace-migration] refactor: rename REPO/REPO_DIR/_REPO → WORKSPACE in 7 files (name-lies cleanup, no behavior change) - #1333

Closed
liususan091219 wants to merge 1 commit into
mainfrom
fix/cosmetic-rename-repo-to-workspace
Closed

liususan091219 wants to merge 1 commit into
mainfrom
fix/cosmetic-rename-repo-to-workspace

Conversation

@liususan091219

Copy link
Copy Markdown
Collaborator

Summary

Pure variable rename. 60 insertions, 60 deletions. No behavior change, no logic touched — every line is just s/\bREPO\b/WORKSPACE/.

7 sites where a variable was named REPO/REPO_DIR/_REPO/repo_resolved but its value came from resolve_workspace() / resolveWorkspace() / WORKSPACE_DIR.resolve(). The misleading names made the v3 workspace-contract audit harder than it should have been — readers (humans and LLMs running cwd-resolution audits) had to trace each REPO back to its assignment site to discover it actually held workspace state. After this PR WORKSPACE means workspace, REPO_DIR means repo, and you can tell which from the name alone.

Renames

File Old New Uses
src/agent-api.py:496-498 repo_resolved workspace_resolved 3
src/send_allowlist.py:51+ _REPO _WORKSPACE 5
src/slack-bridge.py:59+ REPO WORKSPACE 12
src/dm-result.py:38, 187 REPO WORKSPACE 2
src/discord-bridge.py:59+ REPO WORKSPACE 16
src/telegram-bridge.py:37+ REPO WORKSPACE 15
src/task-bridge.ts:18+ REPO_DIR WORKSPACE_DIR 7

src/discord-bridge.py:L3685-3687 also has a comment referring to "REPO" by name when explaining why dm-result.py is resolved via __file__ — comment updated to read "WORKSPACE" so it stays in sync with the variable; the historical reference to PR #762 is preserved verbatim.

What is NOT renamed

  • src/agent-api.py:93 REPO_DIR = Path(__file__).parent.parent — the name is accurate (it actually holds the repo path). Kept as-is.
  • scripts/{presenter-mode,query-conversation,results-health}.sh — audit-flagged as cosmetic but these are part of PR [workspace-migration] fix(scripts): 5 sh scripts read/write workspace state, not repo (P1 batch) #1276 (5-scripts workspace-anchor fix), where the rename happens alongside the actual path fix. Bundling there keeps the test+script change together.
  • 81 other "cosmetic" audit findings — different bug classes (LLM prose paths, test fixtures writing to real workspace, dual-read patterns). Not addressable by a simple rename; tracked separately in workspace/notes/cwd-audit-v3-2026-05-27.md.

Verification

$ git diff --stat
 src/agent-api.py       |  6 +++---
 src/discord-bridge.py  | 32 ++++++++++++++++----------------
 src/dm-result.py       |  4 ++--
 src/send_allowlist.py  | 10 +++++-----
 src/slack-bridge.py    | 24 ++++++++++++------------
 src/task-bridge.ts     | 14 +++++++-------
 src/telegram-bridge.py | 30 +++++++++++++++---------------
 7 files changed, 60 insertions(+), 60 deletions(-)
$ for f in src/agent-api.py src/send_allowlist.py src/slack-bridge.py src/dm-result.py src/discord-bridge.py src/telegram-bridge.py src/task-bridge.ts; do
    cnt=$(grep -cE '\b(REPO|REPO_DIR|_REPO|repo_resolved)\b' "$f")
    echo "$f: $cnt stray REPO-ish bare uses"
  done
src/agent-api.py: 4 remaining REPO-ish bare uses  ← all legitimate (real REPO_DIR at L93+, used at L129, L440)
src/send_allowlist.py: 0
src/slack-bridge.py: 0
src/dm-result.py: 0
src/discord-bridge.py: 0
src/telegram-bridge.py: 0
src/task-bridge.ts: 0

npx tsc --noEmit clean.

Method

Word-boundary regex substitution: re.sub(r'\b{OLD}\b', '{NEW}', src). This ensures docstring/comment text like "the repo" / "repo_root" / "REPO_DIR-elsewhere" (legitimate) wasn't touched. Each file was eyeballed before substitution to confirm no other identifier collides with the rename token.

Refs v3 workspace-contract audit, P3 cosmetic batch (88 sites flagged in audit; 7 are simple variable-name-lies addressable by rename — this PR. The rest fall under PR #1276 or remain in the audit backlog as their own bug classes).

🤖 Generated with Claude Code

…PACE where the value is workspace, not repo

[workspace-migration] Pure variable rename — 60 insertions, 60 deletions, no behavior change.

7 sites where a variable was named REPO (or REPO_DIR / _REPO / repo_resolved) but its
value came from resolve_workspace() / resolveWorkspace() / WORKSPACE_DIR.resolve().
Misleading names made it hard to reason about path safety during the v3 workspace-contract
audit — readers thought "REPO" meant repo root and missed that workspace-anchored state
was actually being constructed.

Renames:
- src/agent-api.py:496-498   repo_resolved   → workspace_resolved   (3 uses)
- src/send_allowlist.py:51+  _REPO           → _WORKSPACE           (5 uses)
- src/slack-bridge.py:59+    REPO            → WORKSPACE            (12 uses)
- src/dm-result.py:38,187    REPO            → WORKSPACE            (2 uses)
- src/discord-bridge.py:59+  REPO            → WORKSPACE            (16 uses)
- src/telegram-bridge.py:37+ REPO            → WORKSPACE            (15 uses)
- src/task-bridge.ts:18+     REPO_DIR        → WORKSPACE_DIR        (7 uses)

src/discord-bridge.py L3685-3687 also has a comment referencing "REPO" by name explaining
why dm-result.py is resolved via __file__ — comment updated to "WORKSPACE" to stay
accurate; the historical reference to PR #762 is preserved.

Not touched:
- src/agent-api.py:93 REPO_DIR = Path(__file__).parent.parent — actual repo path, kept
- scripts/{presenter-mode,query-conversation,results-health}.sh — audit-tagged as cosmetic
  but they're part of PR #1276 (5-scripts workspace-anchor fix), where the rename happens
  alongside the path fix.
- 81 other "cosmetic" audit findings — different bug classes (LLM prose, test fixtures
  writing to real workspace, dual-read patterns) not addressable by simple rename.

Verification:
- `git diff --stat`: 60 +, 60 - (pure substitution).
- `grep -wE '\b(REPO|REPO_DIR|_REPO|repo_resolved)\b'` on the 7 files: 0 stray refs.
- `npx tsc --noEmit`: clean.
- Word-boundary substitution via `re.sub(r'\bREPO\b', ...)` so docstring "the repo" /
  "repo_root" / "REPO_DIR-elsewhere" wouldn't be touched.

Refs v3 workspace-contract audit, P3 cosmetic batch (88 sites flagged, 7 renamable).

@chetanunadkat chetanunadkat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the rename. The 4 files agent-api.py, send_allowlist.py, slack-bridge.py, task-bridge.ts are clean — every renamed REPO/_REPO is resolve_workspace()-derived and all paths (tasks/, results/, state/, etc.) are genuine workspace state. Faithful rename, no behavior change. ✅

⚠️ Collision with #1274 on 3 lines — flagging before merge. This PR renames REPO → WORKSPACE at the same three .env-read sites that #1274 ("read .env from source tree, not workspace") is fixing as a bug:

  • discord-bridge.py — env_file = REPO / ".env" → here becomes WORKSPACE / ".env"
  • dm-result.py — REPO / ".env"
  • telegram-bridge.py — load_dotenv(REPO / ".env")

.env lives in the source-tree root, not the workspace, so WORKSPACE / ".env" is the wrong target (#1274's point: these silently fail to load DISCORD_BOT_TOKEN / TEAM_TIER_OWNER / the Telegram token). The two PRs touch these exact lines with opposite intent:

  • #1274 fixes the path (workspace → source tree), which makes the REPO name honest.
  • #1333 renames the variable (REPO → WORKSPACE), cementing the workspace path.

They can't both land cleanly:

  • #1274 first → this PR then renames a now-source-tree REPO to WORKSPACE (reverse name-lie + re-breaks .env loading).
  • #1333 first → #1274 must rebase onto the new WORKSPACE name.

Suggest: drop the 3 .env sites from this rename (leave them to #1274, which makes REPO honest by pointing it at the source tree), or land #1274 first and rebase. The other 4 files are unaffected either way.

(Cold review from the Sutando proactive loop — context: verified against current main + #1274's diff.)

@amkunte

amkunte commented May 29, 2026

Copy link
Copy Markdown
Contributor

LGTM — pure rename, +60/-60, zero behavior change. The name-truth audit is exactly the kind of cleanup that's easy to put off until a future debugger spends an hour tracing REPO_DIR back to a resolve_workspace() call site.

Strong points:

  • The table in the PR body listing every site's old/new/uses field makes the change auditable in one read. Pure mechanical s/REPO/WORKSPACE/ in places where the value was already a workspace path.
  • All 7 files cleanly bucket "value comes from resolve_workspace()" → no false-positives renaming an actual repo path.
  • Doesn't touch REPO_DIR-named-things that actually hold repo paths (e.g. install-claude-hooks.sh:54's REPO_DIR). Discipline.

One thing to verify post-merge (non-blocking):
If anything outside this PR's diff was grep-importing REPO symbols from these files, the rename would break it. Diff is all module-local variables (not from X import REPO), so very unlikely — but worth a grep -r "from agent_api import REPO\|from discord_bridge import REPO\|..." across the repo as a 10-second sanity check before merge. Nothing in the immediate diff suggests cross-module use.

Excellent maintenance PR — costs nothing to land, makes every future audit cheaper. 👍

@sonichi

sonichi commented May 29, 2026

Copy link
Copy Markdown
Owner

LGTM — verified the "no behavior change" claim holds across all 7 files.

Since this renames the variable at its definition (REPO/_REPO = resolve_workspace() → WORKSPACE/_WORKSPACE), the only real risk is an unrenamed straggler reference (would become an undefined-name crash) or a name collision with a pre-existing WORKSPACE. Checked both on the branch head:

  • Zero leftover bare REPO/_REPO tokens in any of the 7 files (excluding the unrelated SUTANDO_REPO_DIR/REPO_DIR/REPO_ROOT names) — rename is complete, no dangling refs.
  • No pre-existing WORKSPACE/_WORKSPACE in any of the 7 files on main — no shadowing/collision introduced.
  • Value is identical (resolve_workspace()), and the dm-fallback comment text in discord-bridge.py was updated in lockstep (REPO point at... → WORKSPACE point at...), so the comment no longer lies either.

Per-file convention is preserved (module-private _WORKSPACE in send_allowlist.py mirroring its original _REPO; plain WORKSPACE elsewhere). Pure cosmetic cleanup that removes the long-standing "this var says REPO but holds the workspace path" foot-gun. No blockers. (Reviewing as Sutando/Pro — no merge authority; flagging for Chi.)

@liususan091219

Copy link
Copy Markdown
Collaborator Author

Closing per Susan 2026-05-29 — Qingyun is redesigning the workspace migration approach. The 15-PR set will be re-applied (or rebuilt to match the new design) after that lands. Branch is preserved; can be reopened or cherry-picked when ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants