Repository navigation
Conversation
hidb4ai
left a comment
hidb4ai
left a comment
There was a problem hiding this comment.
I found four issues in collection receipts, the shipped DSH bundle, restore coverage, and Codex diagnostics. Please address the inline findings before merging.
| if target["instance_id"] != instance["instance_id"]: | ||
| raise ManagementError("forbidden", "The target belongs to another instance", 403) | ||
| refs = [r.model_dump(mode="json") for r in report.source_refs] | ||
| target["source_refs"] = list({canonical(r): r for r in target.get("source_refs", []) + refs}.values()) |
There was a problem hiding this comment.
[P2] Keep collection receipts within the channel limit
This accumulates references without a bound, while receipt() returns the entire list through a channel capped at 2 MiB. Using synthetic, protocol-valid reports, eight accepted batches of 1,000 references with 256-character source IDs produced a 2,432,285-byte reply. Both status and repeated add then returned 503 result-unconfirmed; the control view still worked. Continuous collection can therefore make a persisted target permanently unqueryable through its result API. Paginate or otherwise bound returned receipts while preserving stored submission evidence.
There was a problem hiding this comment.
Clarified the collect result contract: status retains only the most recently received per-item submission report, with its SourceRef and matching timestamp. The manager no longer accumulates historical references, and collect add/repeated add return only the target identity and control state. Saved Sources, actual submission receipts, and checkpoint semantics are unchanged. The contract, guides, and HTTP/MCP and CLI tests now cover these distinct behaviors.
| "merge_atomic_memories": { | ||
| "readOnly": false | ||
| }, | ||
| "pc_source_add": { |
There was a problem hiding this comment.
[P2] Rebuild the shipped DSH bundle
The six new tools are absent from the committed lib/index.js, which is the package's main/export entry. scripts/prepare.mjs skips rebuilding when that file exists, so source installs keep the old policy and deny all six tools before sending an MCP request. Directly invoking the bundled policy reproduced kind: deny for every new tool, and the dsh-package CI build reports the missing bundle changes. Rebuild and commit lib/ alongside these generated declarations.
There was a problem hiding this comment.
Rebuilt the checked-in DSH bundle from the current sources. The shipped catalog now includes all six Source tools; the compiled policy allows the read-only tools and requests approval for mutating tools. The DSH tests pass, and repeating the build produces identical output.
| ) | ||
|
|
||
| SOURCE_WRITE_PLUGINS_TABLE = Table( | ||
| "pc_source_write_plugins", |
There was a problem hiding this comment.
[P2] Add plugin registrations to the documented restore plans
Both docs/en/docs/operate/troubleshoot.md and its Chinese counterpart omit pc_source_write_plugins from their obloader table lists. Following the documented OceanBase data-only migration therefore leaves this table empty, although SourceWritePlugins.restore() depends on it to restore loaded write capabilities after restart. Add the table to the appropriate restore layer in both locales so plugin registrations survive the documented migration.
There was a problem hiding this comment.
Added pc_source_write_plugins to Layer 1 of the restore table lists in both language versions. The existing coverage test passes, including complete table coverage, matching locale lists, and parent-first ordering.
| id = "capture_content_source" | ||
| capabilities = ["source_capture"] | ||
| [[toolsets.tools]] | ||
| id = "pc_source_capabilities" |
There was a problem hiding this comment.
[P2] Synchronize Codex doctor with the new tool declarations
These six tools are now capability-bearing requirements, but _CODEX_FULL_MCP_TOOLS in src/powercontext/cli/system.py still omits all six. Removing any one from the discovered MCP catalog still makes doctor codex --json report mcp_full_profile as OK and exit successfully, so a host missing the new Source workflow is reported as fully compatible. The existing parameterized diagnostic test reproduces this for each new tool. Include the new declarations in the actual full-profile check.
There was a problem hiding this comment.
Added all six Source tools to the full MCP checklist used by doctor. The existing parameterized tests verify that omitting any of these tools is reported as a missing capability.
hireview001
left a comment
hireview001
left a comment
There was a problem hiding this comment.
- Resolve merge conflicts with the target branch.
- CI failed: validate (failure).
- CI failed: controlled-cli (ubuntu-latest) (failure).
- CI failed: controlled-cli (windows-latest) (failure).
- CI failed: tests (3.14, unit) (failure).
- CI failed: tests (3.12, e2e) (failure).
- CI failed: tests (3.13, e2e) (failure).
- CI failed: tests (3.14, e2e) (failure).
- CI failed: tests (3.11, e2e) (failure).
- CI failed: tests (3.12, unit) (failure).
- CI failed: tests (3.13, unit) (failure).
- Additional CI issues: PR checks.
| open_builtin_access_control( | ||
| resolved.database, | ||
| bootstrap_administrators=(static_principal,) if legacy_static_admin else (), | ||
| bootstrap_administrators=((static_principal,) + ((source_deployer,) if source_deployer else ())) |
There was a problem hiding this comment.
[P1] Separate Scope presets from administrator bootstrapping
Passing these principals to open_builtin_access_control makes Scope authorization depend on administrator count: a sole identity-map administrator receives automatic viewer, contributor, reviewer and delegator bindings on denied HTTP Scope requests, while adding the automatic Source deployer disables the legacy token's Scope bootstrap. Seed Server administrator roles separately from the legacy static Scope preset so identity-map users require explicit Scope grants and legacy-token deployments retain their existing behavior.
There was a problem hiding this comment.
Separated Server administrator role initialization from the legacy static-token Scope preset. Identity-map administrators now require explicit Scope grants regardless of administrator count, while adding the Source deployer preserves the legacy token behavior. Regression coverage checks both authentication modes with and without Source deployment.
| try: | ||
| await self.manager() | ||
| except Exception: | ||
| logger.error("Source manager startup failed; inspect local service configuration") # noqa: TRY400 - no secret-bearing traceback |
There was a problem hiding this comment.
[P2] Local deployment has no non-Linux path, and both startup failures are logged without that fact
process_action refuses every platform except Linux (src/powercontext/source_management/background.py:62-63), and local-service goes through _systemctl, which carries the same guard plus a systemctl --user requirement (src/powercontext/source_management/service.py:62-63); _alive additionally reads /proc/sys/kernel/random/boot_id and /etc/machine-id. Both permitted values of source_management.process_mode are therefore Linux-only.
These two handlers turn that into a platform-neutral message. An operator on macOS or Windows who sets source_plugins_dir or source_management.autostart gets Source manager startup failed; inspect local service configuration (this line) and Source instance preparation failed; inspect local configuration (:354), with no indication that the platform is unsupported, while /v1/source-management/capabilities keeps answering management: not-configured or unavailable exactly as if the manager were merely down.
The user guide only qualifies the default — "On Linux, the manager and Runtimes use background processes by default" (docs/en/docs/workflows/source-plugins.md:62, docs/zh/docs/workflows/source-plugins.md:62) — which reads as a Linux-specific default rather than a requirement, and the Troubleshooting section does not cover it either.
Suggestion: state the Linux requirement for local deployment in both guides and in the process_mode setting documentation, and include the ManagementError code in both log lines so background-unsupported and service-unavailable reach the operator instead of being reduced to "inspect local configuration".
There was a problem hiding this comment.
Addressed in 6849156. Both guides and the process_mode setting now explicitly state that local collect deployment requires Linux, with systemctl --user additionally required for local-service. Both startup handlers preserve background-unsupported and service-unavailable in the existing error_code field via log_safely; other errors remain generic to avoid exposing private details. The background process guard also runs before importing fcntl. Verified the diagnostic codes in JSON and console output without exception-message or traceback leakage. This clarifies the existing platform restriction; it does not add macOS or Windows local deployment support.
There was a problem hiding this comment.
在 753b1a1d（本 PR head，相对上轮只多一个 "sync master" 合并）复核，该问题仍在：
src/powercontext/source_management/background.py:60-61仍是if sys.platform != "linux": raise ManagementError("background-unsupported", "Source background processes currently require Linux")；src/powercontext/source_management/deployment.py:343-357在autostart时捕获该失败，只记录Source manager startup failed; inspect local service configuration，error_code归入background-unsupported/service-unavailable白名单后不再向上传播，文案与平台无关；- 同一 PR 的
persistence_sha256门禁在 head 一致（27173e62），所以这不是基线滞后问题。
也就是说：在非 Linux 主机上把 process_mode 配成非默认值并开启 autostart，Server 启动照常成功、只有一条与平台无关的 ERROR，用户没有任何指向"该模式仅支持 Linux"的线索。建议二选一：在校验期直接拒绝非 Linux 的该模式，或让日志与文档明确写出平台限制。
There was a problem hiding this comment.
更正上一条：文档一侧已经补上了，我原先"没有任何指向平台限制的线索"的判断不成立。
在 753b1a1d 上核对 docs/en/docs/workflows/source-plugins.md:161 与 docs/zh/docs/workflows/source-plugins.md:159，故障排查表已按 error_code 列出：
Startup logs report
background-unsupportedorservice-unavailable| Local collect deployment requires Linux;local-servicealso requires a usablesystemctl --user.
也就是说"哪种 process_mode 需要 Linux / systemctl"已有中英文说明，且与日志里的 error_code 对得上。于是原 P2 的文档半部分作废，只剩下日志文案本身仍是平台中性的 Source manager startup failed; inspect local service configuration——这一条按可维护性只能算提示性，不阻塞合并，我不再坚持。
hireview001
left a comment
hireview001
left a comment
There was a problem hiding this comment.
- Resolve merge conflicts with the target branch.
done |
hireview001
left a comment
hireview001
left a comment
There was a problem hiding this comment.
- CI failed: Upgrade release gate (failure).
- CI failed: upgrade-acceptance / Prepare upgrade artifacts and matrix (failure).
- CI failed: Main (failure). Failed steps: upgrade-acceptance / Prepare upgrade artifacts and matrix / Run upgrade gate regressions; Upgrade release gate / Require the selected parallel acceptance workflow.
hidb4ai
left a comment
hidb4ai
left a comment
There was a problem hiding this comment.
The four findings from my previous review are addressed on this commit. Two DSH host-policy regressions remain in the newly enabled Source tools; please address the inline comments before merging. I exercised the checked-in bundled policy and the public HTTP save/read path, without launching a real DSH host.
| ContractVersion: | ||
| type: object | ||
| Use the discovered write or collect capability and version. Collect capabilities require an explicit instance_id. Write runs in the Server using loaded Definition and Projection implementations, without an instance or management service. Write waits for Source references or an error in the original call; no durable request or status query is created. Collect establishes or reuses a persistent target and never resumes a stopped target. No mode or idempotency_key is accepted; write retries use the underlying Source identity rules. | ||
| x-powercontext-scope-mode: none |
There was a problem hiding this comment.
[P2] Bind Scope-dependent Source calls to the host Scope
Declaring this operation as none generates scopeMode: 'none' with no path parameters, so DSH's usesScope() skips its authoritative session-Scope check. Executing the shipped policy with host Scope A and arguments for Scope B lets pc_source_capabilities, pc_source_status, and business-view pc_source_instances continue without resolving the host Scope; pc_source_add reaches ordinary mutation approval instead of being denied. An existing get_artifact call for B is correctly denied. If the credential can access both projects, the public add endpoint accepts and persists the write in B, violating the documented host-Scope boundary. Bind Scope-dependent Source operations to the current host Scope while keeping Scope-free administrator controls separate.
| "merge_atomic_memories": { | ||
| "readOnly": false | ||
| }, | ||
| "pc_source_add": { |
There was a problem hiding this comment.
[P2] Apply the existing secret guard to Source writes
This enables another content-save entry point, but hasSecretContent() in src/secrets.ts does not recognize pc_source_add in either PAYLOAD_WRITES or CONTENT_FIELDS. The checked-in bundled handler returns ask for content.inline with input.text = 'sk-review-fixture-only', while capture_content_source with the same synthetic marker returns deny with secret_rejected before approval. Ordinary mutation approval therefore allows content that the existing write path refuses; a public HTTP probe confirmed that it is saved and readable unchanged. Apply the content guard to the new Source input before approval or dispatch, and rebuild the shipped bundle.
Which issue or RFC does this PR close?
Related RFC: Source plugin framework
Rationale for this change
Allow users to deploy custom Source types for synchronous writes and continuous collection through the shared public MCP endpoint, while reusing Server-side Scope authorization and the existing Source / Projection storage contracts.
What changes are included in this PR?
Are there any user-facing changes?
powercontext sourcedeployment and management commands, along with public MCP interfaces for capability discovery, writes, and collection management.server.adminfor plugin loading; ordinary operations continue to enforce Scope permissions.How was this change tested?
make api-generate,make js-api-generate, andmake contract-test; all 56 contract tests passed.make checkand pre-commit static checks.AI usage statement
OpenAI Codex assisted with implementation, documentation, testing, and code review. Changes underwent independent subagent review before each commit.