Repository navigation
Conversation
|
@codex review |
github-actions
Bot
added
commands
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (2)
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
Timesheet commands accept untrusted URL hosts and ignore URL account IDs, risking operations against the wrong account, while create also accepts URLs without a recording ID and can target a project ID instead.
2 open issues
What changed in this PR
Adds individual timesheet entry management alongside the existing reports, exposing four previously unused SDK operations through the CLI.
Changes:
- Adds
timesheet entry create,show,update, anddelete, with input validation and confirmation for permanent deletion. - Updates command discovery, API coverage, help text, and agent guidance.
- Adds unit, end-to-end, and live-account lifecycle tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
skills/basecamp/SKILL.md |
Documents entry commands and time-logging behavior. |
internal/commands/timesheet.go |
Registers the entry group and updates help. |
internal/commands/timesheet_entries.go |
Implements entry operations, validation, and output. |
internal/commands/timesheet_entries_test.go |
Tests requests, validation, and deletion safeguards. |
internal/commands/commands.go |
Adds entry management to the command catalog. |
e2e/timesheet.bats |
Tests local refusals through the CLI binary. |
e2e/smoke/smoke_timesheet_write.bats |
Adds a live-account entry lifecycle test. |
e2e/smoke/run_smoke.sh |
Includes the timesheet lifecycle in mutation tests. |
API-COVERAGE.md |
Corrects timesheet coverage to seven endpoints. |
.surface |
Records the new commands, arguments, and flags. |
Review effort: Balanced
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f7ec1a62d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 2f7ec1a62d
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
|
@codex review |
There was a problem hiding this comment.
🟡 Changes recommended
Create accepts scheme-less URLs that bypass host and account validation and person URLs misidentified as recordings, risking time entries in the wrong account or against an unrelated recording.
3 open issues
2 resolved since last review
Review effort: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6d9d29e7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: a910aa68e7
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
The CLI read timesheets through the three reports but couldn't log time: SDK v0.29.1's TimesheetService Get, Create, Update and Destroy went unused, while API-COVERAGE.md claimed the whole set. timesheet entry create logs hours against an item (date defaults to today), show and update address one entry by id or URL, and delete removes it for good behind --force or a confirmation, since bc3 never trashes an entry.
A URL on a look-alike host, or from another account than the configured one, is refused before any request, and create refuses project and listing URLs, which name no item.
…lifecycle The delete breadcrumb carries --force when machine output is reading it, as chat delete's does. The smoke lifecycle runs its tests in order, and its timesheets-off gaps are allowlisted like the read-only ones.
Anything but a bare id is a URL that has to be http(s) on a trusted Basecamp host. create takes only the item kinds bc3 logs time against, so a person's or project's URL can't send its id out as a recording's. URL accounts compare numerically, and the follow-up commands carry an account adopted from the URL.
A path written after a # or ? is not the URL's path, so entry URLs are parsed first and only their path is matched, and create routes only the scheme, host and path. URLs carrying userinfo are refused.
a910aa6 to
badc84c
Compare
|
Rebased onto main (now SDK v0.31.0, after #973). The only conflict was |
Styled and Markdown output skip the nested person, so the summary carries the person's name and id.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Why
The CLI reads timesheets through the three reports (
timesheet report,project,item) but can't log, change or remove time. SDK v0.29.1, the CLI's pin, has hadTimesheetServiceGet,Create,UpdateandDestroyfor a while; the CLI called none of them, whileAPI-COVERAGE.mdclaimed "list, show, create, update, delete". The operation coverage report from basecamp/basecamp-sdk#1055 found the gap.What changed
A
timesheet entrygroup. bc3 routes entries by account alone, so no verb takes or needs a project.CreateTimesheetEntry(POST /recordings/{id}/timesheet/entries.json)basecamp timesheet entry create <item-id|url> --hours 1.5 [--date] [--description] [--person]GetTimesheetEntry(GET /timesheet_entries/{id})basecamp timesheet entry show <id|url>UpdateTimesheetEntry(PUT /timesheet_entries/{id})basecamp timesheet entry update <id|url> [--hours] [--date] [--description] [--person]DestroyTimesheetEntry(DELETE /timesheet_entries/{id})basecamp timesheet entry delete <id|url> [--force]--hoursis checked locally as a decimal orh:mm, the two forms bc3 takes.--dategoes throughdateparseand must come out as a real ISO date; on create it defaults to today.--persontakes an ID, name, email orme.deleteis permanent: bc3 never trashes or archives an entry. So it asks for confirmation, and machine output needs--force, the same assubtasks delete.…/timesheet/entries/<id>, or either API form). Any other URL, such as the to-do the time is logged against, is refused rather than read as an entry ID, sodeletecan't aim at the wrong thing.createrefuses a project's timesheet URL, which carries the project's ID and not the timesheet recording's.entryaction),.surface, theAPI-COVERAGE.mdrow (now 7 endpoints, listed), a "Timesheet entries" section in the basecamp skill, the timesheet group's agent notes (they named abasecamp clockshortcut that no longer exists), and a smoke lifecycle file.basecamp mcpneeds nothing: its model already servescreate_timesheet_entry,get_timesheet_entry,update_timesheet_entryanddestroy_timesheet_entry.Tests
internal/commands/timesheet_entries_test.go: each verb's method, path and body over the recording transport. Covers entry URLs in all three forms, a default date of today,--person meresolution, fields left out when unset, local refusals that send no request (bad hours, bad dates, empty values, nothing to update, non-IDs, item and project-timesheet URLs on every per-entry verb),deleteneeding--forcewithout a terminal, and a 403 on delete returned as an error.e2e/timesheet.bats: the local refusals through the built binary.e2e/smoke/smoke_timesheet_write.bats: create, show, update, delete against a real account. A 403 from an account without timesheets counts as unverifiable.bin/cipasses.Gap card: https://app.basecamp.com/2914079/buckets/48521764/card_tables/cards/10447986600
Summary by cubic
Adds a
timesheet entrycommand group so the CLI can log, view, change, and delete timesheet entries. The CLI previously read timesheets only through the three report commands; the pinned SDK's entry operations went unused whileAPI-COVERAGE.mdclaimed them.createlogs hours against an item (--hoursrequired,--datedefaults to today,--persontakes an ID, name, email, orme).createtakes only the item kinds bc3 logs time against; project, listing, and person URLs are refused too.show,update, anddeletetake the entry ID or its own URL; any other URL is refused sodeletecan't target the item the time is logged against.deleteis permanent, so it prompts for confirmation and needs--forcein machine-output or non-interactive sessions; the delete breadcrumb carries--forcefor machine readers..surface,API-COVERAGE.md, the skill file, and the timesheet group's agent notes, and adds unit, e2e, and a serial smoke lifecycle whose timesheets-off gaps are allowlisted. The entry summary carries the person's name and ID, which styled and Markdown output would otherwise skip.Written for commit 87ddf41. Summary will update on new commits.