✨ Stage-aware Hub token revocation, sandbox /tmp fix, and skill commit hygiene (#74) - #86
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds stage-aware Hub token revocation, injects token IDs into workflow resources, mounts writable ChangesHub token lifecycle
Sandbox temporary storage
Developer tooling hygiene
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AgentWorkflowRun
participant MigrationHarness
participant HubClient
AgentWorkflowRun->>MigrationHarness: Provide HUB_TOKEN_ID and workflow stage
MigrationHarness->>MigrationHarness: Evaluate revocation eligibility
MigrationHarness->>HubClient: Revoke token on standalone or final-stage exit
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
/hold for #80 to merge |
Harness revokes Hub API token on exit for standalone runs and last workflow stage; intermediate stages skip. Reads KONVEYOR_WORKFLOW_STAGE, KONVEYOR_WORKFLOW_STAGE_COUNT, and HUB_TOKEN_ID from env. Assisted-By: Claude Code <noreply@anthropic.com> Signed-off-by: Savitha Raghunathan <saveetha13@gmail.com>
Assisted-By: Claude Code <noreply@anthropic.com> Signed-off-by: Savitha Raghunathan <saveetha13@gmail.com>
e32ded7 to
8f45db4
Compare
savitharaghunathan
changed the title
Assisted-By: Claude Code <noreply@anthropic.com> Signed-off-by: Savitha Raghunathan <saveetha13@gmail.com>
Assisted-By: Claude Code <noreply@anthropic.com> Signed-off-by: Savitha Raghunathan <saveetha13@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
harness/cmd/migration-harness/main.go (1)
64-80: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRegister token cleanup before Hub resolution.
resolveFromHubcan fail before it returnshubClient. In that path, the deferred revocation is never registered. The minted token then remains valid until its TTL expires.Create the Hub client and register stage-aware cleanup after configuration loads. Pass that client into
resolveFromHubsoFetchAppandFetchGitCredsfailures still attempt revocation.🤖 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 `@harness/cmd/migration-harness/main.go` around lines 64 - 80, Move Hub client creation and the stage-aware token revocation defer to immediately after configuration loads, before calling resolveFromHub. Update resolveFromHub to accept and reuse that client so FetchApp and FetchGitCreds failures still trigger cleanup, while preserving the existing shouldRevokeToken behavior and revocation logging.
🤖 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 `@CONTEXT.md`:
- Around line 153-155: Update the Hub environment contract to use APP_ID
consistently instead of HUB_APP_ID. In CONTEXT.md lines 153-155, replace
HUB_APP_ID with APP_ID; in
docs/adr/0006-hub-addon-pattern-for-agent-resources.md lines 37-38 and the
related references at lines 5 and 39, update all connectivity-variable
references to APP_ID.
In `@harness/cmd/migration-harness/main.go`:
- Around line 315-325: Update shouldRevokeToken in
harness/cmd/migration-harness/main.go:315-325 to parse positive workflow stage
and count values, treating standalone runs only when both fields are empty and
revoking workflow tokens only when 1 <= stage <= count and stage equals count;
reject malformed or equal invalid values such as 0/0 and abc/abc. In the cleanup
setup at harness/cmd/migration-harness/main.go:72-84, parse HUB_TOKEN_ID before
scheduling revocation and skip with a warning when the token ID or workflow
metadata is invalid. Update tests in
harness/cmd/migration-harness/main_test.go:121-160 to expect count-only metadata
to skip revocation and cover malformed stage/count and token-ID cases.
In `@internal/controller/agentrun_controller_test.go`:
- Around line 369-376: Update the volume-mount assertions in the test loop over
spec.Containers[0].VolumeMounts to explicitly verify that the mount matching
tmpVolumeName and MountPath "/tmp" has ReadOnly set to false, while preserving
the existing presence assertion.
In `@internal/controller/agentrun_controller.go`:
- Around line 349-353: Set the tmp EmptyDir volume’s SizeLimit in the volume
construction using the policy-approved ephemeral-storage capacity, matching the
existing workspace limit where appropriate. Update the relevant agent-run
controller test assertions to verify the tmp volume exposes the expected size
limit, while preserving the existing volume setup.
In `@skills/execute/SKILL.md`:
- Line 44: Replace placeholder or broad staging commands in
skills/execute/SKILL.md lines 44-44 with the exact target file path from the
current PLAN.md step. In skills/verify/SKILL.md lines 39-39, stage only the
compiler-reported file; at lines 75-75, stage only the intended verification
files and exclude generated outputs.
---
Outside diff comments:
In `@harness/cmd/migration-harness/main.go`:
- Around line 64-80: Move Hub client creation and the stage-aware token
revocation defer to immediately after configuration loads, before calling
resolveFromHub. Update resolveFromHub to accept and reuse that client so
FetchApp and FetchGitCreds failures still trigger cleanup, while preserving the
existing shouldRevokeToken behavior and revocation logging.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0cc8b9ea-5bb8-402e-9178-495f2ba95a87
📒 Files selected for processing (20)
CONTEXT.mdchanges/unreleased/74-stage-aware-token-revocation.yamlchanges/unreleased/91-sandbox-tmp-emptydir.yamlchanges/unreleased/92-skill-commit-hygiene.yamldocs/adr/0006-hub-addon-pattern-for-agent-resources.mdhack/harness-test/setup.shhack/harness-test/workflow-resources.yamlhack/setup-e2e.shharness/cmd/migration-harness/main.goharness/cmd/migration-harness/main_test.goharness/internal/config/config.goharness/internal/config/config_test.goharness/internal/hub/client.goharness/internal/hub/client_test.goimages/agent-base/Containerfileinternal/controller/agentrun_controller.gointernal/controller/agentrun_controller_test.goskills/execute/SKILL.mdskills/plan/SKILL.mdskills/verify/SKILL.md
- Fix HUB_APP_ID → APP_ID in CONTEXT.md and ADR 0006 - Validate token ID, stage, and count in shouldRevokeToken - Register token revocation defer before resolveFromHub - Set 1Gi SizeLimit on /tmp EmptyDir volume - Assert ReadOnly: false on /tmp mount in tests Assisted-By: Claude Code <noreply@anthropic.com> Signed-off-by: Savitha Raghunathan <saveetha13@gmail.com>
djzager
left a comment
djzager
left a comment
There was a problem hiding this comment.
Looks good overall. The latest fixup commit addressed the major issues (numeric validation in shouldRevokeToken, SizeLimit on /tmp, APP_ID naming, early defer registration). Four minor/nit comments remain — nothing blocking.
| - Report which reference you used in the Goal section of PLAN.md | ||
| - When done, run: `git add PLAN.md graph.json && git commit -m "Add migration plan and code graph"` | ||
| - When done, run: `git add PLAN.md && git commit -m "Add migration plan"` | ||
| - Do NOT commit `graphify-out/`, `.goose/`, or other generated artifacts — they are gitignored |
There was a problem hiding this comment.
Was dropping graph.json from the commit intentional? It's not gitignored (only graphify-out/ is), and it was useful for auditing what the plan was based on. Later stages read PLAN.md so it's not blocking, but it's lost after the plan stage in a workflow since each stage gets a fresh pod.
| 3. Write the modified file | ||
| 4. Run: git add -A && git commit -m "<describe the migration change>" | ||
| 4. Run: git add <modified files> && git commit -m "<describe the migration change>" | ||
| 5. Move to the next step immediately |
There was a problem hiding this comment.
Nit: The plan skill uses a concrete example (git add PLAN.md), but execute/verify use angle-bracket placeholders like git add <modified files>. Some LLM agents might emit these literally. Consider example style: git add path/to/Modified.java AnotherFile.xml or add a note like "(replace with actual file paths)".
| }, | ||
| { | ||
| name: "second of two stages — skip", | ||
| hubTokenID: "1", |
There was a problem hiding this comment.
Nit: This test case is named "second of two stages" but sets workflowStage: "1" with workflowStageCount: "2". Since the controller uses 1-based indexing, this is actually the first of two stages. Consider renaming to "first of two stages — skip".
| hubClient := hub.NewClient(cfg.HubBaseURL, cfg.HubToken) | ||
| if shouldRevokeToken(cfg) { | ||
| tokenID, _ := strconv.ParseUint(cfg.HubTokenID, 10, 64) | ||
| defer func() { |
There was a problem hiding this comment.
Nit: HubTokenID is parsed in shouldRevokeToken for validation, then parsed again here with the error discarded. Since shouldRevokeToken gates entry, this is safe, but you could avoid the double-parse by having shouldRevokeToken return the parsed tokenID (e.g., shouldRevokeToken(cfg) (uint64, bool)).
- Restore graph.json to plan skill commit (not gitignored, useful for audit) - Use concrete file path examples in skill git add instructions - Fix test case name: "first of two stages" not "second" - Return parsed tokenID from shouldRevokeToken to avoid double-parse Assisted-By: Claude Code <noreply@anthropic.com> Signed-off-by: Savitha Raghunathan <saveetha13@gmail.com>
- Remove redundant stage <= count guard in shouldRevokeToken - Remove dead hubClient != nil check (client is always created now) - Remove stale duplicate comment in setup script - Log warning for malformed workflow metadata instead of treating as valid stage Assisted-By: Claude Code <noreply@anthropic.com> Signed-off-by: Savitha Raghunathan <saveetha13@gmail.com>
…ollers Upstream konveyor#86 moved writable-/tmp to a controller-mounted emptyDir and dropped the image-level chmod. On clusters still running an older controller (no /tmp mount), goose panics with EACCES creating temp files under OpenShift's random UID. Belt-and-suspenders in the image keeps the :tee test images runnable under both controller vintages. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
2 participants
Summary
KONVEYOR_WORKFLOW_STAGE,KONVEYOR_WORKFLOW_STAGE_COUNT, andHUB_TOKEN_IDfrom env./tmpin sandbox pods so tools can write temp files at runtime. Removed stalechmod 1777 /tmpfrom Containerfile (overlay FS doesn't preserve it). Added unit tests for /tmp and workspace volumes.Assisted-By: Claude Code noreply@anthropic.com
Summary by CodeRabbit
Summary by CodeRabbit
New Features
/tmpdirectory.Bug Fixes
/tmppermission handling.Documentation