Add agent skills, starting with time-off-sync - #41
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe release adds agent skill documentation, a Agent skills
Token validity diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/commands_test.rs (1)
564-601: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftAssert the exit-code contract in this integration test.
The test ignores
_codeand uses unreachable API endpoints, so connectivity checks fail by design. It can pass whilehubstaff checkstill exits1. Run the command with successful or intentionally skipped non-token checks, then assertcode == 0while retaining theWARNand no-remediation assertions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/commands_test.rs` around lines 564 - 601, Update cli_check_warns_instead_of_failing_for_long_lived_token_without_expiry to exercise check with successful or intentionally skipped non-token checks despite the unreachable endpoints, then assert the returned code is 0. Keep the existing WARN, explanatory detail, and no-set-pat assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/time-off-sync/SKILL.md`:
- Around line 18-30: Update the metadata.commands list in SKILL.md to include
time_off_policies archive create, ensuring the manifest declares the
schema-derived command used by the skill and allows the offline guard to
validate it.
In `@src/command_index.rs`:
- Around line 775-806: Update the frontmatter parsing around `strip_prefix`,
`front`, and the `in_commands` state so manifests without a closing `---`
delimiter are rejected rather than treated as valid. Track the indentation level
of `metadata:` and only enter command parsing for a nested `commands:` key at
that level; reject or ignore top-level `commands:` entries while preserving
existing name and description parsing.
---
Nitpick comments:
In `@tests/commands_test.rs`:
- Around line 564-601: Update
cli_check_warns_instead_of_failing_for_long_lived_token_without_expiry to
exercise check with successful or intentionally skipped non-token checks despite
the unreachable endpoints, then assert the returned code is 0. Keep the existing
WARN, explanatory detail, and no-set-pat assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e45b479-43b5-452e-9c11-18fece75d062
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
CHANGELOG.mdCONTRIBUTING.mdCargo.tomlREADME.mdskills/CONTRIBUTING.mdskills/README.mdskills/time-off-sync/SKILL.mdskills/time-off-sync/examples/balance-adjustment.jsonskills/time-off-sync/examples/request-approve.jsonskills/time-off-sync/examples/request-create.jsonskills/time-off-sync/reference/endpoints.mdskills/time-off-sync/reference/gotchas.mdsrc/check.rssrc/command_index.rstests/commands_test.rs
serg-kovalev
force-pushed
the
feat/agent-skills-time-off-sync
branch
from
July 31, 2026 11:42
01024b9 to
a854bfc
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/time-off-sync/reference/endpoints.md`:
- Around line 14-16: Clarify the organization_id requirement in the endpoint
documentation to apply only to organization-scoped routes, noting that
single-record commands may omit it. In the endpoint tables and surrounding
prose, standardize the path placeholder terminology by using one name
consistently or explicitly defining {org} as organization_id, while preserving
the existing option sources and error wording.
- Line 50: Update the “Restoring renames the policy” sentence so the code span
contains only “(Restored YYYY-MM-DD)” without a leading space, while keeping the
separating space in the surrounding prose.
In `@skills/time-off-sync/reference/gotchas.md`:
- Around line 20-21: Update the preflight guidance in “Fix” to make the
multiple-user_id check advisory by using inspect or warn instead of assert. Keep
the time_off_balances list probe for 403 as the hard access gate, and do not
treat a 200 response as proof of access.
In `@skills/time-off-sync/SKILL.md`:
- Around line 69-70: Update the confirmation rule in the time-off
synchronization instructions to require showing the exact command and JSON body
and obtaining confirmation before every mutating call, including create, update,
status, and delete. Preserve the existing human-supervision requirement.
- Around line 108-110: Update the balance-probe guidance near the organization
access check so access is granted only when time_off_balances list returns a
successful, valid response; treat 401, 403, 429, 5xx, network errors, empty
error responses, and every other non-success outcome as failures that must stop
the flow, while retaining the permission-fix instruction for 403/error_code
14705.
- Around line 202-203: Update the existing-request handling in the
reconciliation flow to compare the source request’s desired status and relevant
fields with the matched request before skipping. Skip only when all desired
values already match; otherwise perform the necessary transition or field
updates, including requests left submitted after an approval failure.
- Around line 86-93: Update the token-validity guidance in the CLI verification
section to document that both raw tokens and organization access tokens
(hsoat_…) may validly report WARN because they are non-expiring. Preserve the
existing API reachability and Organization access checks, and clarify that older
CLI versions may incorrectly report FAIL for either token type.
- Around line 15-17: Clarify the verification statement in the skill
frontmatter: remove the claim that payloads in examples/ were run unless
substituted copies were actually tested; if they were, state that substituted
payloads were used and specify the verified scope. Keep the remaining API
verification and idempotency claims unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f72c6d6a-f78c-460a-9db1-76f9f5500d4f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
CHANGELOG.mdCONTRIBUTING.mdCargo.tomlskills/CONTRIBUTING.mdskills/README.mdskills/time-off-sync/SKILL.mdskills/time-off-sync/examples/balance-adjustment.jsonskills/time-off-sync/examples/request-approve.jsonskills/time-off-sync/examples/request-create.jsonskills/time-off-sync/reference/endpoints.mdskills/time-off-sync/reference/gotchas.mdsrc/command_index.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- CHANGELOG.md
- skills/time-off-sync/examples/request-approve.json
- CONTRIBUTING.md
- skills/time-off-sync/examples/balance-adjustment.json
- skills/README.md
- Cargo.toml
- src/command_index.rs
- skills/time-off-sync/examples/request-create.json
There was a problem hiding this comment.
Pull request overview
Adds a new “agent skills” documentation surface (starting with a time-off sync playbook) and introduces an offline drift-guard test to ensure skill-declared CLI commands continue to resolve against the committed OpenAPI schema fixture. It also adjusts hubstaff check so long-lived, non-refreshable tokens without expires_at produce WARN instead of FAIL, aligning the CLI’s preflight guidance with real-world unattended token usage.
Changes:
- Introduce
skills/with contributing guidance and the initialtime-off-syncskill (plus references and JSON templates). - Add an offline test guard to validate
metadata.commandsin eachskills/*/SKILL.mdagainsttests/fixtures/schema.json. - Fix token validity diagnostics for long-lived tokens without expiry, update tests/docs, and bump to
0.5.0.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/commands_test.rs | Adds integration coverage asserting check reports Token validity as WARN for long-lived tokens without expires_at. |
| src/command_index.rs | Adds a unit-test “drift guard” that parses skill frontmatter and verifies declared commands resolve against the schema fixture. |
| src/check.rs | Changes missing-expires_at handling to WARN when non-refreshable, with corresponding unit-test updates. |
| skills/time-off-sync/SKILL.md | Adds the main time-off sync playbook with required frontmatter and declared commands list. |
| skills/time-off-sync/reference/gotchas.md | Adds a “silent failure”/edge-case reference guide for time-off sync workflows. |
| skills/time-off-sync/reference/endpoints.md | Adds command-to-endpoint mapping and operational notes for time-off-related commands. |
| skills/time-off-sync/examples/request-create.json | Adds a JSON template for creating time-off requests. |
| skills/time-off-sync/examples/request-approve.json | Adds a JSON template for approving requests via the status endpoint. |
| skills/time-off-sync/examples/balance-adjustment.json | Adds a JSON template for idempotent balance “replace” adjustments. |
| skills/README.md | Documents what skills are, requirements, installation, and where they work. |
| skills/CONTRIBUTING.md | Defines the skill authoring workflow, verification expectations, and acceptance checklist. |
| README.md | Updates the hubstaff check table to reflect the new WARN behavior for missing expires_at in long-lived tokens. |
| CONTRIBUTING.md | Adds a section describing the new skills/ area and why it lives in-repo. |
| CHANGELOG.md | Adds 0.5.0 release notes reflecting skills + drift guard + token validity behavior change. |
| Cargo.toml | Bumps package version to 0.5.0. |
| Cargo.lock | Updates locked dependencies consistent with the release bump. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
serg-kovalev
force-pushed
the
feat/agent-skills-time-off-sync
branch
2 times, most recently
from
July 31, 2026 12:00
29a3a98 to
93f9758
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/CONTRIBUTING.md`:
- Around line 37-39: Add language tags to all four fenced code blocks: use text
or console for the /skill-creator block in skills/CONTRIBUTING.md lines 37-39,
text for the directory tree in skills/CONTRIBUTING.md lines 79-84, text for the
replace example in skills/time-off-sync/SKILL.md lines 263-266, and text for the
error output in skills/time-off-sync/reference/gotchas.md lines 107-109.
In `@skills/time-off-sync/reference/endpoints.md`:
- Around line 6-8: Update the Bash examples in the endpoint documentation:
replace the angle-bracket placeholder in the hubstaff help command with COMMAND,
and quote the bracketed --starts_at[start] filter so Bash does not perform
redirection or pathname expansion.
In `@skills/time-off-sync/reference/gotchas.md`:
- Around line 3-4: Update the introductory claim in gotchas.md to say “Many
entries” instead of “Every entry,” while preserving the existing distinction
that loud failures are documented in SKILL.md.
In `@skills/time-off-sync/SKILL.md`:
- Around line 69-70: Update the write-confirmation instruction in the time-off
sync skill so it requires showing the exact command and its JSON body when a
body is present, while explicitly stating when the mutating command has no body.
Preserve confirmation before all create, update, status, delete, archive, and
restore operations, including the bodyless time_off_policies archive create
invocation.
- Around line 205-208: The reconciliation guidance in
skills/time-off-sync/SKILL.md lines 205-208 must use the persisted external
mapping or stable message reference as the primary request identity, rather than
relying on user_id, time_off_policy_id, and date range alone; stop and report
collisions instead of updating or creating against an ambiguous match. Apply the
same collision-handling guidance to the fallback path in
skills/time-off-sync/reference/gotchas.md lines 228-230.
- Line 320: Update the 401 guidance table entry to include raw tokens alongside
organization access tokens as non-refreshable credentials. State that a 401 for
either token type indicates revocation and requires replacing the token, while
preserving the existing PAT auto-refresh guidance.
- Around line 337-342: Update the retry flow for exit code 4 so it re-reads the
mapping and current server state before retrying any non-idempotent
time_off_requests create operation. Reconcile an existing submitted request and
continue its workflow when present; only issue create when no matching request
exists, preventing duplicate requests after ambiguous network failures.
- Around line 294-299: Extend the rescan guidance to define source-deletion
handling: when a missing record should delete the corresponding Hubstaff
request, when a frozen record must instead trigger an alert, and how the related
balance is reconciled. Anchor these rules to the outer-window rescan and ensure
stale leave cannot remain unresolved in Hubstaff.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef4ab224-ba61-4f2f-8cca-e1dbc960ca5b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
CHANGELOG.mdCONTRIBUTING.mdCargo.tomlskills/CONTRIBUTING.mdskills/README.mdskills/time-off-sync/SKILL.mdskills/time-off-sync/examples/balance-adjustment.jsonskills/time-off-sync/examples/request-approve.jsonskills/time-off-sync/examples/request-create.jsonskills/time-off-sync/reference/endpoints.mdskills/time-off-sync/reference/gotchas.mdsrc/command_index.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- skills/time-off-sync/examples/request-create.json
- CHANGELOG.md
- CONTRIBUTING.md
- Cargo.toml
- skills/time-off-sync/examples/request-approve.json
- skills/README.md
- src/command_index.rs
- skills/time-off-sync/examples/balance-adjustment.json
Contributor
Author
|
@coderabbitai please check your comments. I think they were addressed |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="NetsoftHoldings/hubstaff-cli"
pr_number="41"
echo "=== CodeRabbit review comments ==="
gh api --paginate \
"/repos/${repo}/pulls/${pr_number}/comments?per_page=100" \
--jq '.[] | select(.user.login == "coderabbitai") | {
id,
path,
line,
original_line,
commit_id,
body,
html_url,
created_at
}'
echo
echo "=== Review threads (including resolution status) ==="
gh api graphql \
-F owner="NetsoftHoldings" \
-F name="hubstaff-cli" \
-F number="$pr_number" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
originalLine
comments(first: 50) {
nodes {
author { login }
body
url
createdAt
}
}
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]; .author.login == "coderabbitai"))
| {
isResolved,
path,
line,
originalLine,
comments: [
.comments.nodes[]
| {author: .author.login, body, url, createdAt}
]
}'Length of output: 51525 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== File outlines ==="
ast-grep outline skills/CONTRIBUTING.md --view expanded || true
ast-grep outline skills/time-off-sync/SKILL.md --view expanded || true
ast-grep outline skills/time-off-sync/reference/endpoints.md --view expanded || true
ast-grep outline skills/time-off-sync/reference/gotchas.md --view expanded || true
echo
echo "=== skills/CONTRIBUTING.md: fenced blocks ==="
nl -ba skills/CONTRIBUTING.md | sed -n '30,90p'
echo
echo "=== skills/time-off-sync/SKILL.md: frontmatter, confirmation, reconciliation ==="
nl -ba skills/time-off-sync/SKILL.md | sed -n '1,25p;60,80p;195,215p'
echo
echo "=== skills/time-off-sync/SKILL.md: rescan, 401, retries ==="
nl -ba skills/time-off-sync/SKILL.md | sed -n '285,350p'
echo
echo "=== skills/time-off-sync/reference/endpoints.md ==="
nl -ba skills/time-off-sync/reference/endpoints.md | sed -n '1,30p'
echo
echo "=== skills/time-off-sync/reference/gotchas.md: intro, preflight, fenced block ==="
nl -ba skills/time-off-sync/reference/gotchas.md | sed -n '1,30p;95,115p'
echo
echo "=== skills/time-off-sync/reference/gotchas.md: request identity fallback ==="
nl -ba skills/time-off-sync/reference/gotchas.md | sed -n '220,240p'Length of output: 1835 🏁 Script executed: #!/bin/bash
set -euo pipefail
show_range() {
local file="$1"
local range="$2"
echo "=== ${file}: ${range} ==="
sed -n "${range}p" "$file" | cat -n
echo
}
show_range skills/CONTRIBUTING.md '30,90'
show_range skills/time-off-sync/SKILL.md '1,25'
show_range skills/time-off-sync/SKILL.md '60,80'
show_range skills/time-off-sync/SKILL.md '195,215'
show_range skills/time-off-sync/SKILL.md '285,350'
show_range skills/time-off-sync/reference/endpoints.md '1,30'
show_range skills/time-off-sync/reference/gotchas.md '1,30'
show_range skills/time-off-sync/reference/gotchas.md '95,115'
show_range skills/time-off-sync/reference/gotchas.md '220,240'Length of output: 17297
One unresolved thread is addressed:
Six unresolved threads still need changes:
The remaining open comments are valid. You are interacting with an AI system. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
skills/time-off-sync/SKILL.md (2)
213-213: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language tag to this fenced block.
The request-body fence has no language tag. Add the correct tag, such as
jsonif the block contains JSON.Proposed fix
-``` +```json🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/time-off-sync/SKILL.md` at line 213, Add the appropriate language tag to the request-body fenced code block in the documentation, using json because the block contains JSON, while leaving its contents unchanged.
56-57: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not state that every entry fails quietly.
The skill later documents explicit API errors, including
422responses. Change this sentence to say that entries can fail quietly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/time-off-sync/SKILL.md` around lines 56 - 57, Update the guidance near the first write call in the skill documentation to state that entries can fail quietly, rather than asserting that every entry does. Preserve the instruction to read reference/gotchas.md before writing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@skills/time-off-sync/SKILL.md`:
- Line 213: Add the appropriate language tag to the request-body fenced code
block in the documentation, using json because the block contains JSON, while
leaving its contents unchanged.
- Around line 56-57: Update the guidance near the first write call in the skill
documentation to state that entries can fail quietly, rather than asserting that
every entry does. Preserve the instruction to read reference/gotchas.md before
writing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd6dbbb8-80bc-4839-8762-9d511c28e502
📒 Files selected for processing (4)
skills/CONTRIBUTING.mdskills/time-off-sync/SKILL.mdskills/time-off-sync/reference/endpoints.mdskills/time-off-sync/reference/gotchas.md
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/CONTRIBUTING.md
- skills/time-off-sync/reference/gotchas.md
skojin
requested changes
Jul 31, 2026
skojin
approved these changes
Aug 3, 2026
serg-kovalev
force-pushed
the
feat/agent-skills-time-off-sync
branch
from
August 4, 2026 10:30
ef01b7a to
67ccb96
Compare
serg-kovalev
force-pushed
the
feat/agent-skills-time-off-sync
branch
from
August 4, 2026 11:40
67ccb96 to
1d86bd9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Publishes agent skills as Markdown in this repo so anyone who solves a problem with the CLI can contribute a playbook by PR. First skill is time off sync.
What's added
skills/— Markdown playbooks that teach an AI agent to accomplish a task with the CLI. No MCP server, no extra credentials; a skill is copied into~/.claude/skills/or a project's.claude/skills/.skills/time-off-sync/— mirroring time off from an external HR system into Hubstaff. Records the decisions the OpenAPI schema can't express — write ordering, idempotency, change detection — and deliberately doesn't restate whathubstaff <command> --helpalready prints.skills/CONTRIBUTING.md— authoring workflow, required verification, and the acceptance checklist for contributed skills.Bug fix included
hubstaff checkfailedToken validityfor any long-lived token with noexpires_at— raw tokens fromconfig set token, and organization access tokens. Both are non-refreshable by design, so it exited1on a working setup and suggestedconfig set-pat, which would have replaced the credential. NowWARN.It's here because the skill's preflight tells readers to run
hubstaff check, and that was broken for exactly the token type an unattended sync uses. Released as0.5.0rather than a patch becausecheck's exit code changes.Verification
Every command was executed against the API — policy and request creation, approval, denial, balance adjustment, and a repeat adjustment confirming
replaceis idempotent. Theexamples/payloads were run, not just written.Also benchmarked against a baseline agent with the CLI but no access to
skills/, over three planning tasks:Worth being straight about it: the value is efficiency, not error prevention. A careful agent reading
--helpreaches the same conclusions, it just takes about twice the exploration. The one baseline miss was429/Retry-Afterhandling in the nightly-job design.Notes
descriptionclaims a hand-rolled sync "silently corrupts balances", which the benchmark doesn't support against a careful agent. Left as-is rather than quietly edited — worth a decision.CODEOWNERSforskills/deliberately deferred.Summary by CodeRabbit
time-off-syncagent skill for supervised synchronization of time-off policies, requests, approvals, and balances.