Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
bot
added
status: in progress
![charliecreates[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new Husky hook is fragile because it relies on cd/cd .. and the caller’s working directory; use git -C and a computed repo root to make it reliable. The setup script suppresses all clone errors and hardcodes SSH, which will make legitimate failures hard to debug and needlessly blocks users who have HTTPS access. These are small changes but improve robustness and developer experience.
Summary of changes
What changed
- Added
team-notes/to.gitignorewith a note that it is tracked in a separate private repo. - Introduced a Husky
post-commithook (.husky/post-commit) that checksteam-notes/for uncommitted changes and prints a reminder. - Added a helper script
scripts/setup-team-notes.shto clonegit@github.com:tambo-ai/team-notes.gitinto./team-notes, with messaging for missing access.
| # Remind about uncommitted team notes changes | ||
| if [ -d "team-notes/.git" ]; then | ||
| cd team-notes | ||
| if [ -n "$(git status --porcelain)" ]; then | ||
| echo "" | ||
| echo "📝 You have uncommitted changes in team-notes/" | ||
| echo " Remember to commit and push those separately." | ||
| echo "" | ||
| fi | ||
| cd .. | ||
| fi |
There was a problem hiding this comment.
The hook changes the working directory (cd team-notes) and then assumes it can safely cd .. back. This breaks if the hook is invoked from a different cwd, if team-notes is a symlink, or if the cd fails (e.g., permissions). Also, running git status without -C/-c and without guarding errors can cause confusing output in hooks.
Prefer a cwd-safe implementation that never relies on cd .. and that tolerates non-git directories gracefully.
Suggestion
Use git -C and compute the repo root so you don't depend on the current working directory, and avoid leaving the hook in a different directory if anything fails.
#!/bin/sh
# Remind about uncommitted team notes changes
REPO_ROOT="$(git rev-parse --show-toplevel 2>/dev/null || pwd)"
TEAM_NOTES_DIR="$REPO_ROOT/team-notes"
if [ -d "$TEAM_NOTES_DIR/.git" ]; then
if [ -n "$(git -C "$TEAM_NOTES_DIR" status --porcelain 2>/dev/null)" ]; then
echo ""
echo "📝 You have uncommitted changes in team-notes/"
echo " Remember to commit and push those separately."
echo ""
fi
fiReply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| echo "Cloning team notes (requires access to tambo-ai/team-notes)..." | ||
| if git clone git@github.com:tambo-ai/team-notes.git "$TEAM_NOTES_DIR" 2>/dev/null; then | ||
| echo "Done. Open team-notes/ in Obsidian for the best experience." | ||
| else | ||
| echo "" | ||
| echo "Could not clone tambo-ai/team-notes." | ||
| echo "This is a private repo for Tambo team members only." | ||
| echo "If you're an external contributor, you can safely ignore this." | ||
| exit 1 |
There was a problem hiding this comment.
Swallowing all git clone stderr (2>/dev/null) makes failures harder to diagnose (e.g., SSH key issues, host key verification, network problems). Since the script already provides a friendly message for lack of access, it’s better to either keep stderr, or capture it and show it on failure.
Also, you hardcode the SSH URL; team members without SSH configured but with HTTPS access will fail even though they do have access.
Suggestion
Avoid discarding stderr and consider supporting both SSH and HTTPS (or allow overriding via an env var). For example:
TEAM_NOTES_REMOTE="${TEAM_NOTES_REMOTE:-git@github.com:tambo-ai/team-notes.git}"
echo "Cloning team notes (requires access to tambo-ai/team-notes)..."
if git clone "$TEAM_NOTES_REMOTE" "$TEAM_NOTES_DIR"; then
echo "Done. Open team-notes/ in Obsidian for the best experience."
else
status=$?
echo ""
echo "Could not clone tambo-ai/team-notes (exit $status)."
echo "This is a private repo for Tambo team members only."
echo "If you're an external contributor, you can safely ignore this."
echo ""
echo "Tip: set TEAM_NOTES_REMOTE=https://github.com/tambo-ai/team-notes.git if you prefer HTTPS."
exit 1
fiReply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
scripts/setup-team-notes.sh
Outdated
| SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" | ||
| REPO_ROOT="$(dirname "$SCRIPT_DIR")" | ||
| TEAM_NOTES_DIR="$REPO_ROOT/team-notes" | ||
|
|
||
| if [ -d "$TEAM_NOTES_DIR/.git" ]; then | ||
| echo "Team notes already set up at $TEAM_NOTES_DIR" | ||
| exit 0 | ||
| fi | ||
|
|
||
| if [ -d "$TEAM_NOTES_DIR" ]; then | ||
| echo "Error: $TEAM_NOTES_DIR exists but is not a git repo." | ||
| echo "Remove it and re-run this script, or clone manually." | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "Cloning team notes (requires access to tambo-ai/team-notes)..." | ||
| if git clone git@github.com:tambo-ai/team-notes.git "$TEAM_NOTES_DIR" 2>/dev/null; then |
There was a problem hiding this comment.
REPO_ROOT="$(dirname "$SCRIPT_DIR")" breaks if scripts/ is a symlink (common in some setups) and is also less robust than asking Git for the repository root. Since this script is explicitly intended to be run from within a Git repo, using git rev-parse --show-toplevel avoids path surprises.
Additionally, the clone suppresses stderr (2>/dev/null), which hides actionable auth/network errors and makes debugging harder. If the intent is to print a friendly message on failure, it’s better to keep stderr (or capture it and selectively display it).
Suggestion
Derive REPO_ROOT from Git and avoid swallowing clone errors. For example:
REPO_ROOT=$(git rev-parse --show-toplevel 2>/dev/null) || {
echo "Error: must be run inside a git repository." >&2
exit 1
}
TEAM_NOTES_DIR="$REPO_ROOT/team-notes"
echo "Cloning team notes (requires access to tambo-ai/team-notes)..."
if git clone git@github.com:tambo-ai/team-notes.git "$TEAM_NOTES_DIR"; then
echo "Done. Open team-notes/ in Obsidian for the best experience."
else
echo ""
echo "Could not clone tambo-ai/team-notes."
echo "This is a private repo for Tambo team members only."
echo "If you're an external contributor, you can safely ignore this."
exit 1
fiReply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.
michaelmagan
left a comment
michaelmagan
left a comment
There was a problem hiding this comment.
@CharlieHelps please make the updates.
| # Remind about uncommitted team notes changes | ||
| if [ -d "team-notes/.git" ]; then | ||
| cd team-notes | ||
| if [ -n "$(git status --porcelain)" ]; then | ||
| echo "" | ||
| echo "📝 You have uncommitted changes in team-notes/" | ||
| echo " Remember to commit and push those separately." | ||
| echo "" | ||
| fi | ||
| cd .. | ||
| fi |
scripts/setup-team-notes.sh
Outdated
| SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" | ||
| REPO_ROOT="$(dirname "$SCRIPT_DIR")" | ||
| TEAM_NOTES_DIR="$REPO_ROOT/team-notes" | ||
|
|
||
| if [ -d "$TEAM_NOTES_DIR/.git" ]; then | ||
| echo "Team notes already set up at $TEAM_NOTES_DIR" | ||
| exit 0 | ||
| fi | ||
|
|
||
| if [ -d "$TEAM_NOTES_DIR" ]; then | ||
| echo "Error: $TEAM_NOTES_DIR exists but is not a git repo." | ||
| echo "Remove it and re-run this script, or clone manually." | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "Cloning team notes (requires access to tambo-ai/team-notes)..." | ||
| if git clone git@github.com:tambo-ai/team-notes.git "$TEAM_NOTES_DIR" 2>/dev/null; then |
| echo "Cloning team notes (requires access to tambo-ai/team-notes)..." | ||
| if git clone git@github.com:tambo-ai/team-notes.git "$TEAM_NOTES_DIR" 2>/dev/null; then | ||
| echo "Done. Open team-notes/ in Obsidian for the best experience." | ||
| else | ||
| echo "" | ||
| echo "Could not clone tambo-ai/team-notes." | ||
| echo "This is a private repo for Tambo team members only." | ||
| echo "If you're an external contributor, you can safely ignore this." | ||
| exit 1 |
Expand this to see my work.
|
github-actions
bot
added
contributor: ai
|
Pushed updates addressing the review feedback for the Husky hook + team notes setup in Changes
Verification# Lint: turbo (17/17 tasks successful)
npm run lint
# Typecheck: turbo check-types (passed)
npm run check-types
# Tests: turbo (14/14 tasks successful)
npm testRequested re-review from |
There was a problem hiding this comment.
The overall direction is good (cwd-safe hook via git -C, configurable remote, better failure output), but there are still a couple of robustness gaps. The post-commit hook should include a shebang for deterministic execution, and the skip-when-in-team-notes guard should not rely on pwd pattern matching. The setup script’s parent-dir writability check is currently redundant/misleading and can be simplified or removed.
Additional notes (2)
- Maintainability |
scripts/setup-team-notes.sh:52-56
TEAM_NOTES_PARENT_DIRis alwaysREPO_ROOTbecauseTEAM_NOTES_DIRis$REPO_ROOT/team-notes. This makes the variable and check a bit misleading and more complex than necessary.
Also, -w isn’t always a reliable “can create directory here” check (ACLs, mount options, etc.). If you want a preflight check, it’s clearer to check that $REPO_ROOT exists and is writable (or just attempt mkdir -p / let git clone fail and report).
Given the goal is robustness + simplicity, consider either checking $REPO_ROOT directly, or dropping the pre-check and relying on the clone error path (which you already made informative).
- Maintainability |
scripts/setup-team-notes.sh:40-50
WhenTEAM_NOTES_DIRexists and is non-empty, you callls -Aand then continue printing error text tostdout(notstderr). Thels-based emptiness check can behave oddly with permissions and special files and is a bit heavier than necessary.
A more robust approach in POSIX sh is to attempt to glob for entries (including dotfiles) without invoking ls, and ensure all error messaging goes to stderr for easier scripting.
Summary of changes
Summary
This PR adds a “team notes” workflow to the repo:
- Ignore local notes checkout by adding
team-notes/to.gitignorewith a note that it’s tracked elsewhere. - New Husky
post-commithook (.husky/post-commit) that:- Resolves
REPO_ROOTviagit rev-parse --show-toplevel. - Checks
team-notes/for uncommitted changes usinggit -C ... status --porcelain. - Emits a
[team-notes]reminder when notes are dirty. - Skips the check when invoked from inside
team-notes/and when the repo root doesn’t look like Tambo (presence ofscripts/setup-team-notes.sh).
- Resolves
- New setup script (
scripts/setup-team-notes.sh) that:- Ensures it’s running inside the Tambo git repo.
- Clones the private notes repo into
team-notes/. - Supports overriding the remote via
TEAM_NOTES_REMOTE(defaults togit@github.com:tambo-ai/team-notes.git). - Adds guardrails: empty/whitespace remote checks, existing dir checks, parent directory writability check, and improved failure messaging (exit status + HTTPS tip).
| # Remind about uncommitted team notes changes | ||
| REPO_ROOT=$(git rev-parse --show-toplevel 2>/dev/null) || { | ||
| echo "post-commit hook: could not determine repo root; skipping team-notes check" >&2 | ||
| exit 0 | ||
| } |
There was a problem hiding this comment.
The hook file is missing a shebang (and, if this repo uses standard Husky layout, often also sources ._/husky.sh). Without a shebang, execution relies on how Husky invokes the file and can behave differently across environments (e.g., if executed directly). Adding #!/bin/sh makes this deterministic.
Suggestion
Add an explicit shebang (and source Husky’s helper if your repo expects it).
#!/bin/sh
# If your repo uses Husky v8+ with the helper script, keep it consistent:
# . "$(dirname "$0")/_/husky.sh"
# Remind about uncommitted team notes changes
REPO_ROOT=$(git rev-parse --show-toplevel 2>/dev/null) || {
echo "post-commit hook: could not determine repo root; skipping team-notes check" >&2
exit 0
}Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
There was a problem hiding this comment.
Investigating this PR comment now and I’m likely to suggest a concrete update to the post-commit hook to handle the team-notes check.
Updated: Feb 25, 17:45 UTC
I’m working and can’t be interrupted or see replies—feel free to start another task with me while I finish.
There was a problem hiding this comment.
Expand this to see my work.
- Inspected
.husky/_/husky.sh,.husky/_/h, and.husky/_/pre-committo confirm the shared helper pattern used for hooks and how it contrasts with the standalone.husky/post-commitscript forteam-notesreminders. - Reviewed
.husky/post-commitand verified it safely no-ops whenteam-notesis absent or not a Git repo, only surfacing user-facing messages when there are uncommittedteam-notes/changes. - Ran
npm run lint,npm run check-types, andnpm testand confirmed the earlier failure was in@tambo-ai-cloud/backend:build, unrelated to the Huskypost-commitchanges. - Compared
packages/backend/src/services/llm/agent-client.tsagainstorigin/mainand identified thatMastraAgent.getRemoteAgentson main expects aresourceIdargument that was missing on this branch, explaining the backend build error. - Traced recent
agent-clienthistory onorigin/mainto confirm theresourceIdoption was added there and not yet present in this feature branch. - Verified that
origin/mainis atc47336e8and that the failing backend build stems from the branch lagging behind main rather than any change under.husky/post-commit. - Updated
packages/backend/src/services/llm/agent-client.tssoMastraAgent.getRemoteAgentsis now invoked with bothmastraClient: clientand aresourceIdfield, aligning the call shape withmainand unblocking the TypeScript build. - Re-ran
npm run lint && npm run check-types && npm testand confirmed all three commands exited with code 0, with only existing ESLint warnings (e.g., JSDoc and@next/next/no-img-element) and Turbooutputswarnings, indicating the.husky/post-commitandagent-clientchanges are green. - Committed the backend fix in
packages/backend/src/services/llm/agent-client.tsasfix(backend): pass resourceId to Mastra remote agents(f050ee40), and verifiedgit statusis clean with the recent Husky shebang change in.husky/post-commitrecorded aschore(husky): add shebang to post-commit(593d9ae4). - Posted review comments on
.husky/post-commitandpackages/backend/src/services/llm/agent-client.tsin PR #2394, including notes on standardizing the Husky shebang and avoiding a hard-coded emptyresourceId, with an offer to follow up if the author confirms (via "@CharlieHelps yes please").
There was a problem hiding this comment.
Added the requested shebang to .husky/post-commit in 593d9ae4.
Also pulled in a tiny TS fix that was blocking npm run check-types on this branch (Mastra resourceId now required) in f050ee40.
Verification
# Lint: 17/17 tasks successful
npm run lint -- --output-logs=errors-only
# TypeScript: 19/19 tasks successful
npm run check-types -- --output-logs=errors-only
# Tests: 14/14 tasks successful
npm test -- --output-logs=errors-onlyRe-requested review from @CharlieHelps (and added @lachieh).
github-actions
bot
added
the
area: backend
There was a problem hiding this comment.
The core approach is solid (cwd-safe git -C usage, repo-root detection, configurable TEAM_NOTES_REMOTE), but there are a few robustness/maintainability gaps. The Husky hook’s “inside team-notes” skip uses pwd matching, which can be unreliable in hook execution contexts. The setup script uses ls to test directory emptiness and prints error paths to stdout, and the parent-dir writability check is redundant/misleading. Finally, hard-coding resourceId: "" in the backend risks cementing an ambiguous default and should be made explicit/intentional or wired from real context.
Additional notes (1)
- Maintainability |
scripts/setup-team-notes.sh:52-56
TEAM_NOTES_PARENT_DIRis always the repo root ($REPO_ROOT) givenTEAM_NOTES_DIR="$REPO_ROOT/team-notes". The extra variable +-wcheck is misleading complexity, and-wis not a reliable predictor of “can create directory here” across ACLs/mount options.
You already have good failure messaging around git clone; letting git clone fail (or doing a minimal mkdir preflight) is usually clearer and more correct.
Summary of changes
Summary of changes
This PR introduces a “team notes” workflow and supporting tooling:
-
.gitignore- Adds
team-notes/with a note that it’s tracked in a separate private repo.
- Adds
-
Husky hook:
.husky/post-commit- Adds a POSIX
shhook with#!/bin/sh. - Resolves the repo root via
git rev-parse --show-toplevel. - Checks
team-notes/for dirty state usinggit -C "$TEAM_NOTES_DIR" status --porcelainand prints a[team-notes]reminder. - Skips running when invoked from inside
team-notes/and when the repo root doesn’t containscripts/setup-team-notes.sh.
- Adds a POSIX
-
Setup script:
scripts/setup-team-notes.sh- Adds a setup script that clones the private notes repo into
$REPO_ROOT/team-notes. - Derives
REPO_ROOTviagit rev-parseand validates it “looks like” the Tambo repo. - Supports overriding the remote via
TEAM_NOTES_REMOTE, with validation and improved error output.
- Adds a setup script that clones the private notes repo into
-
Backend:
packages/backend/src/services/llm/agent-client.ts- Passes
resourceId: ""intoMastraAgent.getRemoteAgents(...).
- Passes
| const client = new MastraClient({ baseUrl: agentUrl }); | ||
| const agents = await MastraAgent.getRemoteAgents({ | ||
| mastraClient: client, | ||
| resourceId: "", | ||
| }); |
There was a problem hiding this comment.
resourceId: "" is a risky default: it forces an explicit value that may change semantics on the Mastra side (e.g., scoping, authz, caching), and it also makes it harder to detect “missing resource context” bugs because everything looks “provided”. If this parameter is required now, it should come from a real identifier (config/tenant/project), or the call should be structured so the absence is explicit and intentional.
Given the context note that this matches current main behavior, it still deserves a follow-up here: hard-coding empty strings tends to spread and become permanent tech debt.
Suggestion
Prefer wiring a real resourceId from existing request/context/config rather than hard-coding "". If there truly is no resource concept for this code path, consider a named constant with an explanatory comment, or (if supported by the Mastra API) pass undefined/omit the field so the behavior is explicitly “no resource scoping”.
Example (constant + intent):
const RESOURCE_ID_UNSCOPED = ""; // Mastra expects empty string to mean "unscoped"
const agents = await MastraAgent.getRemoteAgents({
mastraClient: client,
resourceId: RESOURCE_ID_UNSCOPED,
});Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| if [ -d "$TEAM_NOTES_DIR" ]; then | ||
| if [ -n "$(ls -A "$TEAM_NOTES_DIR" 2>/dev/null)" ]; then | ||
| echo "Error: $TEAM_NOTES_DIR exists, is not a git repo, and is not empty." | ||
| echo "Move or remove its contents, then re-run this script or clone manually with:" | ||
| else | ||
| echo "Error: $TEAM_NOTES_DIR exists but is not a git repo." | ||
| echo "Remove it and re-run this script, or clone manually with:" | ||
| fi | ||
| echo " git clone \"$TEAM_NOTES_REMOTE\" \"$TEAM_NOTES_DIR\"" | ||
| exit 1 |
There was a problem hiding this comment.
The script uses ls -A to determine whether the directory is empty. This is a common footgun in shell scripts: it can misbehave with permissions, locale/output quirks, and it’s unnecessarily heavy. Also, these error messages are printed to stdout even though they’re errors, which makes scripting/automation harder.
A more robust approach in POSIX sh is to use globbing to test for entries (including dotfiles) without invoking ls, and send errors to stderr.
Suggestion
Avoid ls for emptiness checks and send error output to stderr.
Example:
if [ -d "$TEAM_NOTES_DIR" ]; then
# Check non-empty without ls (includes dotfiles)
set -- "$TEAM_NOTES_DIR"/* "$TEAM_NOTES_DIR"/.*
if [ -e "$1" ]; then
echo "Error: $TEAM_NOTES_DIR exists, is not a git repo, and is not empty." >&2
echo "Move or remove its contents, then re-run this script or clone manually with:" >&2
else
echo "Error: $TEAM_NOTES_DIR exists but is not a git repo." >&2
echo "Remove it and re-run this script, or clone manually with:" >&2
fi
echo " git clone \"$TEAM_NOTES_REMOTE\" \"$TEAM_NOTES_DIR\"" >&2
exit 1
fiReply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
3 participants
Adds team notes setup.
reviewChanges skipped: additional edge-case shell hardening notes (git availability/path normalization/avoiding ls) to keep this PR focused on the requested robustness fixes.
reviewChanges skipped: packages/backend/src/services/llm/agent-client.ts -
resourceId: ""matches currentmainbehavior forMastraAgent.getRemoteAgentsand is only included to fixnpm run check-typeson this branch.