feat: add subscription CLI providers and devloopd automation - #894
Closed
albert-einshutoin wants to merge 36 commits into
Closed
albert-einshutoin wants to merge 36 commits into
albert-einshutoin wants to merge 36 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthrough
Changessubscription-only プロバイダと設定
devloopd サイドカーCLI
publishing 失敗の記録
Sequence Diagram(s)sequenceDiagram
participant User as ユーザー/CI
participant devloopd as devloopd start
participant Doctor as runDevloopDoctor
participant Scanner as scanIssues(gh)
participant TAKT as takt --pipeline
participant Ledger as importTaktRun
participant Git as git push (non-interactive)
rect rgba(100, 150, 200, 0.5)
Note over devloopd,Scanner: サイクル開始
devloopd->>Scanner: gh issue list で候補分類
Scanner-->>devloopd: candidates / skipped
devloopd->>Doctor: subscription-only 検証
Doctor-->>devloopd: passed / failed
end
rect rgba(100, 200, 150, 0.5)
Note over devloopd,Git: issue 実行フロー
devloopd->>TAKT: --pipeline --issue --workflow
TAKT->>Git: git push (GIT_TERMINAL_PROMPT=0)
alt 認証失敗
Git-->>TAKT: push error
TAKT-->>devloopd: pr_failed として記録
else 成功
Git-->>TAKT: pushed
TAKT-->>devloopd: completed
end
devloopd->>Ledger: ledger.jsonl に takt_run_imported 追記
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b04eaaf62
ℹ️ 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".
Comment on lines
+119
to
+120
| if (!isRawSubscriptionOnlyEnabled(rawConfig)) { | ||
| return; |
There was a problem hiding this comment.
Because this check returns unless the same raw object contains subscription_only: true, credentials in a different config layer are missed: for example, global openai_api_key plus project subscription_only: true, or project provider_options.*.apiKey plus global subscription-only. devloopd doctor checks both files, but a direct takt run only executes these per-file checks and can start under an effective subscription-only policy with forbidden API-key config still loaded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 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 `@bin/devloopd`:
- Around line 1-24: The devloopd CLI wrapper is still named without an
extension, so Node 20 will not treat it as ESM and the top-level
import/import.meta/await logic in the devloopd entrypoint will fail. Rename the
executable wrapper to use a .js or .mjs extension and update the package.json
bin mapping to point to the new filename, keeping the existing CLI loading logic
in devloopd intact.
In `@builtins/en/config.yaml`:
- Around line 13-16: The sample subscription-only allowlist is missing mock,
which can cause local mock-based validation to be rejected when users copy this
config. Update the commented allowed_providers example in config.yaml to include
mock alongside the existing CLI providers, and keep the sample aligned with the
actual default allowlist used by the subscription-only flow.
In `@src/__tests__/devloopd-merge-gate.test.ts`:
- Around line 77-85: Add a regression test in the merge gate test suite to cover
path-policy edge cases that `.github/**` misses: ensure `mergeIfSafe` denies
changes to a root-level `.env` file and to a nested path like
`src/middleware/auth.ts`, and that the report still shows the forbidden-path
rejection without invoking the merge runner. Use the existing `makeRunner`,
`mergeIfSafe`, and `formatMergeGateReport` test helpers so the new cases are
easy to locate alongside the current `denies forbidden paths before attempting
merge` test.
In `@src/core/models/config-schemas.ts`:
- Around line 91-93: `forbidden_providers` is still validated as arbitrary
non-empty strings, so update the schema in `config-schemas.ts` to use
`ProviderTypeSchema` just like `allowed_providers` in the same config object.
Adjust the `forbidden_providers` field definition so it only accepts the
supported provider enum values, keeping the existing optional/array shape and
ensuring `subscription_only` rules can match correctly at runtime.
In `@src/core/subscription-only/policy.ts`:
- Around line 99-113: `findForbiddenSubscriptionOnlyConfigKeyPaths` currently
stops at arrays because it only recurses through `isRecord`, so forbidden keys
nested inside array elements are missed. Update this helper in `policy.ts` to
also detect arrays and traverse each element while preserving the existing path
format, so keys like `api_key` inside nested list entries are reported by
`FORBIDDEN_CONFIG_KEYS` checks.
In `@src/devloopd/doctor.ts`:
- Around line 266-269: The visibleChecks condition in doctor.ts is incorrectly
using report.passed as part of the same ternary condition as options.verbose, so
passing checks are shown even when --verbose is not set. Update the logic around
visibleChecks to depend only on options.verbose for showing all checks, and
otherwise filter out pass checks so that a passed report still shows only the
summary and non-pass checks. Use the visibleChecks expression in doctor.ts as
the place to adjust the operator grouping/condition.
In `@src/devloopd/issueScanner.ts`:
- Around line 241-247: `parseIssues` is allowing `JSON.parse` failures to escape
and crash the `startDevloop` supervisor loop. Update `scanIssues`/`parseIssues`
to catch invalid-JSON stdout from `gh`, and return a structured failure report
instead of throwing, matching the existing rate-limit and gh-error handling with
a `failureKind` value (for example `gh_error`). Use the `parseIssues`,
`scanIssues`, and `startDevloop` symbols to keep the error path consistent and
prevent the daemon from exiting.
- Around line 161-224: `classifyIssue` is still returning the full
`RawIssueInput` via spread, which leaves the untrusted `body` field on the
runtime `IssueCandidate` object even though the type hides it. Update
`classifyIssue` to stop using `...issue` in every return path and instead build
the candidate explicitly from only the allowed fields, keeping `title` sanitized
with `sanitizeText` and ensuring `body` is never copied into the result. If you
introduce or reuse a helper such as `baseCandidate`, make it the single source
for assembling the sanitized `IssueCandidate` shape.
In `@src/devloopd/ledger.ts`:
- Around line 135-145: The `selectRunSlug` logic currently makes the
`options.latest` check meaningless because the earlier `options.runSlug` early
return leaves `!options.runSlug` always true afterward. Update `selectRunSlug`
in `ledger.ts` so it only falls back to `listRecentRuns(repoPath)[0]?.slug` when
`options.latest === true`, and otherwise returns `undefined` when no `runSlug`
is provided. Keep the explicit `options.runSlug` handling intact and make the
intent of `latest` vs. explicit slug selection clear in the `selectRunSlug`
branch structure.
- Around line 151-166: The sorting logic in listRunSlugs is re-reading each
run’s meta.json repeatedly via readRunMetaBySlug inside the compare callback,
causing avoidable I/O during sort. Precompute and cache each run’s startTime
once before sorting, then sort using that cached value so the comparison in
listRunSlugs does not call readRunMetaBySlug for every pair.
- Around line 255-259: The ledger writer in appendLedgerEvent currently creates
the parent directory and appends to the file with default umask-based
permissions, which is inconsistent with writeMemory in memory.ts. Update
appendLedgerEvent to explicitly set the same access restrictions as writeMemory
by ensuring the ledger directory is created with owner-only permissions and the
ledger file is written with owner-only permissions as well, keeping the behavior
aligned for the same task and issue metadata.
In `@src/devloopd/mergeGate.ts`:
- Around line 87-89: The merge gate path patterns in mergeGate.ts are too narrow
and only match top-level files like src/middleware.ts, not nested files such as
src/middleware/auth.ts. Update the path matching entries near the existing
src/middleware*, src/routes*, and src/config* symbols so they explicitly include
the directories and all descendant files, ensuring the safety gate catches human
review targets anywhere under those trees.
- Around line 126-134: `globToRegExp` の `**/` 変換がルート直下にマッチしないため、`**/.env*` や
`**/*secret*` が先頭階層の対象を取りこぼしています。`globToRegExp` で `segments` を正規表現化する際に、`**`
を単なる任意文字列ではなく「0個以上の階層」に対応させ、区切りスラッシュを含む場合でも先頭直下に一致するように修正してください。特に
`mergeGate.ts` 内の `globToRegExp` の `**` 置換ロジックを見直し、`POLICY_DENY` 判定で `.env` や
`secret.txt` が除外されないことを確認してください。
- Around line 317-322: `merge-if-safe` in `mergeGate.ts` currently calls `gh pr
checks --watch` via `runner.exec` without any upper bound, so it can hang
forever on pending CI. Update the `args`/`runner.exec` flow in `mergeGate` to
either use a non-watching snapshot check instead of `--watch`, or pass an
explicit timeout through `commandRunner.exec` so the wait is bounded; keep the
repo handling and exitCode check intact.
- Around line 166-168: The review decision routing in mergeGate’s handling of
input.pr.reviewDecision is too broad and currently treats REVIEW_REQUIRED as a
change request. Update the condition so requestChanges only receives
CHANGES_REQUESTED, and route REVIEW_REQUIRED into humanReview instead; use the
existing mergeGate logic around input.pr.reviewDecision and
requestChanges/humanReview to keep the classification precise.
In `@src/features/tasks/execute/postExecution.ts`:
- Around line 58-71: The publish failure message built by
buildPublishAuthFailureMessage only includes branch, commit, and detail, so add
the task identifier (and issue number if available) to the returned message.
Update the message assembly in this helper so the failure text clearly names the
affected task/issue for quick retry identification, while keeping the existing
branch and commit context intact.
In `@src/infra/config/project/projectConfig.ts`:
- Around line 272-274: `projectConfig.ts` の `savePayload`
生成ロジックで、`allowedProviders` と `forbiddenProviders`
が空配列のときも保存されてしまう非対称を直してください。`serializeGlobalConfig` と揃えて、`projectConfig`
の該当ループ（`camel`/`snake` のマッピング処理）で配列型は `length > 0` の場合のみ `savePayload`
に入れるようにし、`allowed_providers: []` や `forbidden_providers: []` が書き出されないようにします。
In `@src/infra/subscription-cli/client.ts`:
- Around line 394-437: The codex-cli setup path in callSubscriptionCli currently
builds the invocation before the try/catch, so errors from
buildSubscriptionCliInvocation or mapCodexCliSandboxMode bypass
buildErrorResponse and can also leave the temporary directory uncleaned. Move
the tempDir creation and buildSubscriptionCliInvocation call inside the try
block in callSubscriptionCli, keep the existing finally cleanup, and ensure any
thrown error is always converted through toExecError and buildErrorResponse so
provider failures surface as AgentResponse.error.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a5a20738-9e16-4d43-8043-255af11a887f
📒 Files selected for processing (69)
.gitignore.npmignoreCHANGELOG.mdREADME.mdbin/devloopdbuiltins/en/config.yamldocs/CHANGELOG.ja.mddocs/README.ja.mddocs/ci-cd.ja.mddocs/ci-cd.mddocs/cli-reference.ja.mddocs/cli-reference.mddocs/configuration.ja.mddocs/configuration.mddocs/devloopd.ja.mddocs/devloopd.mddocs/task-management.ja.mddocs/task-management.mdpackage.jsonsrc/__tests__/devloopd-active-runs.test.tssrc/__tests__/devloopd-doctor.test.tssrc/__tests__/devloopd-issue-scanner.test.tssrc/__tests__/devloopd-issue-selector.test.tssrc/__tests__/devloopd-ledger.test.tssrc/__tests__/devloopd-memory.test.tssrc/__tests__/devloopd-merge-gate.test.tssrc/__tests__/devloopd-run.test.tssrc/__tests__/devloopd-supervisor.test.tssrc/__tests__/postExecution.test.tssrc/__tests__/provider-contract-docs.test.tssrc/__tests__/relay-push.test.tssrc/__tests__/subscription-cli-provider.test.tssrc/__tests__/subscription-only-policy.test.tssrc/__tests__/taskGit.test.tssrc/__tests__/taskResultHandler.test.tssrc/app/cli/program.tssrc/app/devloopd/index.tssrc/core/models/config-schemas.tssrc/core/models/config-types.tssrc/core/models/provider-profiles.tssrc/core/models/schema-base.tssrc/core/subscription-only/policy.tssrc/core/workflow/engine/WorkflowValidator.tssrc/core/workflow/types.tssrc/devloopd/activeRuns.tssrc/devloopd/commandRunner.tssrc/devloopd/doctor.tssrc/devloopd/issueScanner.tssrc/devloopd/issueSelector.tssrc/devloopd/ledger.tssrc/devloopd/memory.tssrc/devloopd/mergeGate.tssrc/devloopd/run.tssrc/devloopd/supervisor.tssrc/features/tasks/execute/postExecution.tssrc/features/tasks/execute/taskResultHandler.tssrc/features/tasks/execute/workflowExecution.tssrc/features/tasks/execute/workflowExecutionBootstrap.tssrc/infra/config/global/globalConfigCore.tssrc/infra/config/global/globalConfigSerializer.tssrc/infra/config/loaders/workflowFileLoader.tssrc/infra/config/project/projectConfig.tssrc/infra/config/traced/tracedConfigSchema.tssrc/infra/providers/index.tssrc/infra/providers/subscription-cli.tssrc/infra/subscription-cli/client.tssrc/infra/task/git.tssrc/infra/task/taskLifecycleService.tssrc/shared/types/provider.ts
Author
|
Addressed the Codex review findings in
Verification after the fix:
|
Author
|
Verification update for PR #894:
No code changes needed from this side; maintainer merge is required. |
Author
|
Closing this upstream PR because the work is moving to a personal private repository instead of the official nrslib/takt repository. |
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
1 participant
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.
Summary
Verification
Fixes #866
Summary by CodeRabbit
New Features
devloopdCLI を追加し、診断、実行、レジャー、タイムライン、メモリ、マージ判定、課題スキャン/選択、常駐監視などの操作ができるようになりました。Bug Fixes
Documentation